Catch OSError when persisting heartbeat to queue (carry of #113)

[TimeToBuildBob]

Rebased carry of #113 by @rtsdque onto current master (conflicted with the register_bucket rework). Original commit and authorship preserved; no code changes beyond conflict resolution. tests/test_requestqueue.py: 5 passed.

Thanks @rtsdque. I can't push to your fork, so this is a carry PR; feel free to rebase #113 from carry-113 instead.

Review note (non-blocking): add_request logs a warning on every dropped heartbeat, so a full disk will log once per heartbeat. Rate-limiting it (log first failure, then every N) would avoid a flood.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Adds retry logic for heartbeat queue writes when disk operations fail.

The PR appears safe to merge, with a non-blocking diagnostic gap when the retry buffer evicts heartbeats.

Findings

1.  Buffer Evictions Go Unreported ▶

Summary

The PR catches queue-persistence failures, retains heartbeats for ordered retry, and caps each bucket’s in-memory retry buffer.

• Added tests for failed writes, recovery, retry ordering, and bounded buffering.
• Buffer eviction is not reported, making the amount of lost activity difficult to diagnose.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  H[Heartbeat] --> Q{Queue write succeeds?}
  Q -- Yes --> D[Durable queue]
  Q -- No --> B[Per-bucket retry buffer]
  B --> R{Buffer full?}
  R -- Yes --> E[Oldest entry evicted]
  R -- No --> W[Wait for next commit]
  W --> D

Loading

_{Reviews (4) · Last reviewed commit: "fix(client): bound the in-memory buffer ..."}

[TimeToBuildBob]

Thanks for the review.

I pushed 022b4d5, which addresses finding 3: the test now captures the log with caplog and asserts the Failed to queue request warning, instead of only checking that nothing raises. The contributor's commit is unchanged.

On the other two findings:

• P1 — pending heartbeats are lost. This is the contributor's deliberate, documented trade-off rather than an oversight: Catch OSError when persisting heartbeat to queue #113's description states "a dropped heartbeat is an acceptable tradeoff compared to the whole process going down." This carry is conflict-resolution only, so I did not change the enqueue contract. The suggested alternative — retain last_heartbeat when a queue write fails so the interval can be retried once storage recovers — would require add_request (currently -> None) to report whether the write succeeded, and heartbeat() to advance last_heartbeat only on success. That is a real behavior change (on a persistently full disk the in-memory merge window grows rather than the heartbeat being dropped), so it belongs with the contributor or a maintainer, not silently inside a carry of their fix. @rtsdque, if you'd like it in your PR I can prepare it.
• P2 — repeated failures flood logs. Already flagged in the PR description as non-blocking. A "log the first failure, then every N" throttle is easy to add if wanted; I left the contributor's code unchanged here for the same scope reason.

CI was green on the earlier head; 022b4d5 triggers a fresh run.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Two more changes on this carry:

• faf1db2 — addresses Greptile's P1/P2: a failed queue write no longer drops the pending heartbeat interval (it stays in last_heartbeat and is retried by the next heartbeat), and the warning fires once per failure streak.
• eb0de47 — the original except OSError never caught a real full disk. persistqueue's SQLite backend raises sqlite3.OperationalError: database or disk is full from put(), and that is not an OSError subclass. Verified by capping a real queue with PRAGMA max_page_count; the new test fills it, asserts the write is reported as failed, then checks writes succeed once space returns.

All new tests fail without the fix. black and mypy clean.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Review findings are addressed through 62ce54b: failed writes are retried in order, the per-bucket buffer is bounded, and evictions are reported. The idle/exit durability concern was withdrawn by the reviewer; the buffer remains best-effort in memory while disk writes fail.

Current head has six passing checks; macOS tests are still queued. No further review round or code change needed. Waiting for the remaining CI check and maintainer review/merge (I have pull-only access here).