[beardthelion]

Summary

db_bounded now returns BoundedDbError::Elapsed before polling when the deadline is already spent. This makes discovery_record_elapsed_leaves_neither_row deterministic.

Motivation & context

tokio::time::timeout polls the inner future before it checks the timer, so a zero budget still started the work and could return a value. The test failed in CI on test (beta) for #196 with Ok(()) where Elapsed was expected.

Closes #585

Kind of change

• Bug fix
• Feature
• Security fix
• Docs
• Tests / CI
• Refactor (no behavior change)
• Breaking or protocol change (issue required first)

What changed

• gitlawb-node: db_bounded in ipfs_pin.rs returns Elapsed without polling when no time is left.
• gitlawb-node: new test db_bounded_spent_deadline_does_not_poll_a_ready_future, which uses a future with no await point so the result does not depend on timing.

How a reviewer can verify

The new test fails on main and passes with this change. discovery_record_elapsed_leaves_neither_row failed 10 of 60 runs idle and 46 of 60 with 8 CPU-bound busy loops on a 4-core machine before the change. After it, 0 of 40 failed idle and 0 of 40 loaded.

cargo test -p gitlawb-node --lib -- db_bounded_spent_deadline discovery_record_elapsed_leaves_neither_row

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally
• New behavior is covered by tests (required for fixes)
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (feat(...), fix(...), docs(...))
• Docs / .env.example updated if behavior or config changed (or N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

N/A, none of these apply: this changes an internal timeout helper and its tests only.

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
• Discussed in an issue before implementation (filed as db_bounded polls the future when the deadline is already spent, making discovery_record_elapsed_leaves_neither_row flaky #585)
• Backward-compatible with existing nodes and previously signed history (no wire or stored-data change)

Notes for reviewers

• Checks I ran: cargo fmt --check, cargo clippy -p gitlawb-node --all-targets --locked -- -D warnings and cargo test -p gitlawb-node --lib --locked (2025 passed, 0 failed). I did not run --workspace, so that box is unticked.
• This does not touch the red main in CI is red on main #584, which failed sweep_persists_last_attempted_not_planned_tail_on_mid_batch_narrow and is a different test.
• db_record_deadline floors the budget, so the spent-deadline branch is close to unreachable in production. The change affects tests and any caller that passes a raw spent deadline.

Summary by CodeRabbit

• Bug Fixes
  • Operations that reach their deadline now consistently report a timeout, even when their work could complete immediately. This makes timeout results more predictable.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

db_bounded now returns BoundedDbError::Elapsed when its deadline has no time remaining, before polling the future. A regression test verifies the result and confirms the future was not polled.

Changes: Deadline handling

Layer / File(s)	Summary
Spent-deadline guard and regression test crates/gitlawb-node/src/ipfs_pin.rs	db_bounded checks the remaining duration before calling tokio::time::timeout. A regression test checks that a spent deadline returns Elapsed without polling the future.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: kevincodex1

Merge Risk: Minimal · up to 6c916

The change makes a spent deadline return a timeout without starting the work. It mainly stabilizes a flaky test, and production reach is described as near-unreachable. No merge-blocking risk remains.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: db_bounded now returns Elapsed without polling the future when the deadline is already spent. It matches the raw summary and the PR objectives. It uses the conventiona…
Description check		The description follows the repository template. It has a clear summary, motivation with the CI failure and a linked issue (#585), the kind of change checked as Bug fix and Tests/CI, a list of concret…
Linked Issues check		Issue #585 requires that a spent deadline return BoundedDbError::Elapsed without starting the work, including for a future that is ready on its first poll. The diff at the reviewed head adds an early …
Out of Scope Changes check		The whole-PR diff against the merge base 6f99019 touches only crates/gitlawb-node/src/ipfs_pin.rs. The production change (+5 lines) adds the zero-budget guard in db_bounded. The other additions (+25 …

Full details: Title check

Explanation

The title names the main change: db_bounded now returns Elapsed without polling the future when the deadline is already spent. It matches the raw summary and the PR objectives. It uses the conventional fix(node) prefix and is 80 characters, which is long but still readable.

Full details: Description check

Explanation

The description follows the repository template. It has a clear summary, motivation with the CI failure and a linked issue (#585), the kind of change checked as Bug fix and Tests/CI, a list of concrete changes, reviewer verification commands with measured results, and a completed pre-review checklist. It also has a protocol impact section marked N/A with a reason. The Summary section is one short paragraph, as the template asks. The unticked cargo test --workspace item is explained in the notes. Only the Notes for reviewers section is extra, and the template allows it.

Full details: Linked Issues check

Explanation

Issue #585 requires that a spent deadline return BoundedDbError::Elapsed without starting the work, including for a future that is ready on its first poll. The diff at the reviewed head adds an early return in db_bounded (crates/gitlawb-node/src/ipfs_pin.rs). When left.is_zero(), the function returns Err(BoundedDbError::Elapsed) before tokio::time::timeout polls fut. The new test db_bounded_spent_deadline_does_not_poll_a_ready_future checks both the Elapsed result and that the future was never polled, using a future with no await point. This covers the coding requirement. The issue's verification step (loaded repro rerun) is a test-run procedure, and the PR reports those results. The PR does not change the separate failure in #584, which the issue also excludes. Only coding requirements apply. Note: the early return covers exactly zero budget; a deadline in the past yields zero via saturating_duration_since, so this matches the issue's 'already spent' case.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base 6f99019 touches only crates/gitlawb-node/src/ipfs_pin.rs. The production change (+5 lines) adds the zero-budget guard in db_bounded. The other additions (+25 lines) are one regression test in the existing tests module. Both changes are directly tied to issue #585. No unrelated code, configuration, or documentation changes appear in the reviewed diff.

Finishing Touches

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: bb405145-3e0e-4c79-97d0-75721cb866da

Commits

Reviewing files that changed from the base of the PR and between 6f99019 and 6c91652.

Files selected for processing: 1

• crates/gitlawb-node/src/ipfs_pin.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[jatmn]

The db_bounded early return matches #585 and the new ready-future test. I do not have code changes to request on this diff.

Merge readiness

• Duplicate PR: #587 is open with the same is_zero() guard, the same new unit test, and an extra fix to the stale discovery_record_elapsed_leaves_neither_row doc comment. Merge one of #586 / #587 and close or rebase the other so #585 is not fixed twice.
• CI test (beta): Failed on this run for reconciliation::tests::sweep_persists_last_attempted_not_planned_tail_on_mid_batch_narrow (reconciliation.rs), which is not in this diff (same class of red main called out in the PR body / #584). test (stable) passed. Confirm repo policy on whether beta-only unrelated reds block merge.
• Checklist: PR body leaves cargo test --workspace unchecked; acceptable if required gates are the PR Checks workflow, not full local workspace.

Findings

🟢 Thanks for the contribution. I do not see any actionable issues from my review of this diff.