fix(review): time out hung git fetch and state-blob requests - #1175
fix(review): time out hung git fetch and state-blob requests#1175SebTardif wants to merge 1 commit into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 9:23 PM ET / August 30, 2026, 01:23 UTC. ClawSweeper reviewWhat this changesThe branch adds time limits to review-blob Git commands and state-blob Worker POST retries, with focused timeout tests. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain Keep open: the earlier exact review-checkout fetch concern is already addressed on the current base, and the remaining patch is narrow with no blocking correctness defect found. It still needs real behavior proof at the actual Git hydration and State Blob HTTP boundaries before merge. Priority: P2 Review scores
Verification
How this fits togetherReview workers hydrate changed Git blobs before restricted code review and publish durable records through the State Blob Worker. These I/O steps feed review completion and canonical state storage, so a stalled subprocess or request can consume a worker slot. flowchart LR
A[Pull request changes] --> B[Review blob hydration]
B --> C[Bounded Git inspection]
C --> D[Restricted code review]
E[State records] --> F[State blob publisher]
F --> G[Worker and R2 storage]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the focused deadline change and add redacted after-fix traces showing the actual hydration helper and a controlled real State Blob HTTP transport fail closed and preserve their intended retry/result behavior. Do we have a high-confidence way to reproduce the issue? No high-confidence production-boundary reproduction was supplied. The current-head trace confirms timeout primitives and an injected fetch, but not the exact Git hydration fetch or a real State Blob HTTP transport. Is this the best way to solve the issue? Yes, the focused deadlines reuse the existing failure and retry paths and are a maintainable repair. Merge readiness remains contingent on real boundary evidence for the chosen limits. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fa535d54a032. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
9f1a93e to
73352c0
Compare
|
@SebTardif, thank you—your contribution is welcome. If you would like to continue with this PR, please bring it up to date and ready for maintainer look. If you would prefer Martin to take it over as a maintainer, please say so. This note does not indicate approval or a merge decision. |
Review blob hydration's origin fetch used spawnSync without a timeout, so a stalled git remote blocked the review worker. State blob POST retries used fetch without AbortSignal, so a hung Worker blocked each attempt. Bound git fetch to 180s (local git to 60s) and each blob request to 15s while keeping the existing four-attempt backoff. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
73352c0 to
42871da
Compare
We want to continue. The branch is now replayed onto current Current Please take another look when you have time. |
What Problem This Solves
Fixes an issue where a review worker could hang forever on two I/O paths. Blob hydration runs
git fetch origin --stdinthroughspawnSyncwith notimeout, so a stalled origin never returns. The state-blob client retries a Worker POST up to four times with noAbortSignal, so a hung Worker blocks each attempt without a deadline.Why This Change Was Made
Review blob hydration now gives every
gitspawnSynca deadline: 180s for the network fetch, 60s for localls-tree/rev-list/cat-file. The existinghydrated: falsepath still applies when Node returnsETIMEDOUT. State-blob POSTs now passsignal: AbortSignal.timeout(15000)on each attempt and keep the current four-attempt backoff. Media-proofffmpeg/ffprobestays on its own PR.User Impact
A stalled git origin or Worker no longer pins a review worker until an outer job timeout. Hydration fails closed (
hydrated: false). A hung blob publish fails after four timed attempts instead of blocking the process.OpenClaw Bay Impact
Unaffected. This change is review-worker I/O deadlines only. It does not alter Bay lifecycle, queue, status, telemetry, or dashboard data contracts.
Documentation Lifecycle
No documentation lifecycle changes.
Evidence
Before this change,
src/clawsweeper-review-blobs.tspassedcwd/encoding/maxBuffertogit fetchwith notimeout, andsrc/state-blob-client.tscalledfetchwith nosignal. After the change, a livenodeimport of the compiled helpers prints fetch 180000 / local 60000.sleep 30withspawnSynctimeout 80ms returnsETIMEDOUTin 82ms.AbortSignal.timeout(80)firesTimeoutError. A hung state-blobfetchImplthat waits onsignal.abortis called four times with requestedMs=15000 and rejects withThe operation was aborted.in 1713ms.Related: unbounded
git fetchlanded in #982 (2026-07-31). State-blob POST without a signal landed in #936 (2026-07-29). Repair git helpers already have this bound in #508. Media-proof spawn timeout is a separate surface in #1173. Sibling Worker POSTs already useAbortSignal.timeout(5s to 20s).Real behavior proof
Behavior or issue addressed: Review-worker blob hydration could hang forever on
git fetch origin --stdinbecausespawnSynchad notimeout. State-blob publish could hang forever on each Worker POST becausefetchhad noAbortSignal.Real environment tested: macOS, Node v26.7.0, worktree
/tmp/oc-impl-clawsweeper-io-timeoutat the patched commit.Exact steps or command run after this patch:
The script imported
reviewBlobGitSpawnTimeoutsForTestandpublishStateBlobfrom the compiled helpers, ranspawnSync("sleep", ["30"], { timeout: 80 }), waited onAbortSignal.timeout(80), then calledpublishStateBlobwith afetchImplthat never resolves and only rejects wheninit.signalaborts. The client still requestedAbortSignal.timeout(15000)per attempt; the script shortened that timer to 50ms so the hang path could be observed.Evidence after fix: terminal output from the live
nodecommand above. Compiled review-blob timeouts arefetchMs: 180000andlocalMs: 60000. Hungsleep 30returnsstatus: null,signal: SIGTERM,error.code: ETIMEDOUTin 82ms.AbortSignal.timeout(80)reportsTimeoutError/The operation was aborted due to timeout. The hung Worker POST is attempted four times withrequestedMs=15000and ends withThe operation was aborted.in 1713ms.Observed result after fix: Node kills the hung
sleepinstead of waiting 30 seconds. The state-blob client no longer waits on a fetch that never settles. It aborts each attempt and keeps the existing four-attempt backoff, then throws.What was not tested: A live hung
git fetchagainst a real origin and a live hung Cloudflare Worker. Media-proofffmpeg/ffproberemains on fix(media-proof): time out hung ffmpeg and ffprobe #1173.Keep Allow edits from maintainers enabled.