Skip to content

fix(review): time out hung git fetch and state-blob requests - #1175

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/review-io-timeouts
Open

fix(review): time out hung git fetch and state-blob requests#1175
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/review-io-timeouts

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

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 --stdin through spawnSync with no timeout, so a stalled origin never returns. The state-blob client retries a Worker POST up to four times with no AbortSignal, so a hung Worker blocks each attempt without a deadline.

Why This Change Was Made

Review blob hydration now gives every git spawnSync a deadline: 180s for the network fetch, 60s for local ls-tree / rev-list / cat-file. The existing hydrated: false path still applies when Node returns ETIMEDOUT. State-blob POSTs now pass signal: AbortSignal.timeout(15000) on each attempt and keep the current four-attempt backoff. Media-proof ffmpeg/ffprobe stays 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.ts passed cwd / encoding / maxBuffer to git fetch with no timeout, and src/state-blob-client.ts called fetch with no signal. After the change, a live node import of the compiled helpers prints fetch 180000 / local 60000. sleep 30 with spawnSync timeout 80ms returns ETIMEDOUT in 82ms. AbortSignal.timeout(80) fires TimeoutError. A hung state-blob fetchImpl that waits on signal.abort is called four times with requestedMs=15000 and rejects with The operation was aborted. in 1713ms.

$ node io-timeout-proof.mjs
review blob git timeouts:
{
  "fetchMs": 180000,
  "localMs": 60000
}

hung sleep 30 with spawnSync timeout 80ms:
{
  "status": null,
  "signal": "SIGTERM",
  "error": {
    "name": "Error",
    "message": "spawnSync sleep ETIMEDOUT",
    "code": "ETIMEDOUT"
  },
  "elapsedMs": 82
}

AbortSignal.timeout(80) abort:
{
  "aborted": true,
  "reasonName": "TimeoutError",
  "reasonMessage": "The operation was aborted due to timeout",
  "elapsedMs": 82
}
AbortSignal.timeout requestedMs=15000
AbortSignal.timeout requestedMs=15000
AbortSignal.timeout requestedMs=15000
AbortSignal.timeout requestedMs=15000

hung state-blob fetch after patch:
{
  "attempts": 4,
  "message": "The operation was aborted.",
  "elapsedMs": 1713
}

Related: unbounded git fetch landed 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 use AbortSignal.timeout (5s to 20s).

Real behavior proof

  • Behavior or issue addressed: Review-worker blob hydration could hang forever on git fetch origin --stdin because spawnSync had no timeout. State-blob publish could hang forever on each Worker POST because fetch had no AbortSignal.

  • Real environment tested: macOS, Node v26.7.0, worktree /tmp/oc-impl-clawsweeper-io-timeout at the patched commit.

  • Exact steps or command run after this patch:

    node io-timeout-proof.mjs

    The script imported reviewBlobGitSpawnTimeoutsForTest and publishStateBlob from the compiled helpers, ran spawnSync("sleep", ["30"], { timeout: 80 }), waited on AbortSignal.timeout(80), then called publishStateBlob with a fetchImpl that never resolves and only rejects when init.signal aborts. The client still requested AbortSignal.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 node command above. Compiled review-blob timeouts are fetchMs: 180000 and localMs: 60000. Hung sleep 30 returns status: null, signal: SIGTERM, error.code: ETIMEDOUT in 82ms. AbortSignal.timeout(80) reports TimeoutError / The operation was aborted due to timeout. The hung Worker POST is attempted four times with requestedMs=15000 and ends with The operation was aborted. in 1713ms.

  • Observed result after fix: Node kills the hung sleep instead 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 fetch against a real origin and a live hung Cloudflare Worker. Media-proof ffmpeg/ffprobe remains on fix(media-proof): time out hung ffmpeg and ffprobe #1173.

Keep Allow edits from maintainers enabled.

@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 9:23 PM ET / August 30, 2026, 01:23 UTC.

ClawSweeper review

What this changes

The 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
Reviewed head: 42871da02497b663762cb4b053451f211b93efa3

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and appears correct, but the available proof is mocked rather than boundary-real behavior evidence.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed production owners are review-blob hydration and signed State Blob posting. The supplied terminal output imports compiled helpers, but exercises only a generic sleep timeout and publishStateBlob with an injected mocked fetch; it does not run the actual hydration fetch or a real HTTP transport through the production boundary. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owners are review-blob hydration and signed State Blob posting. The supplied terminal output imports compiled helpers, but exercises only a generic sleep timeout and publishStateBlob with an injected mocked fetch; it does not run the actual hydration fetch or a real HTTP transport through the production boundary. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Remaining Git hydration fetch is bounded: The introduced fetch of missing review blobs now uses a 180-second timeout and returns the existing fail-closed hydration result on command failure.
State Blob requests receive a per-attempt deadline: The signed POST path retains its four-attempt retry behavior and now supplies a 15-second AbortSignal to each request.
Earlier review-checkout finding is addressed on the base: The current base already sets a 30-second deadline for ensureReviewTreeCommit, so the prior P1 about the exact review-checkout fetch does not remain on this head.
Findings None None.
Security None None.

How this fits together

Review 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]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owners are review-blob hydration and signed State Blob posting. The supplied terminal output imports compiled helpers, but exercises only a generic sleep timeout and publishStateBlob with an injected mocked fetch; it does not run the actual hydration fetch or a real HTTP transport through the production boundary. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - A legitimate slow Git fetch or State Blob upload will now fail closed at 180 seconds or 15 seconds respectively; the supplied evidence does not show recovery through the real Git hydration or HTTP transport boundary.
  • Resolve merge risk (P2) - The terminal trace exercises timeout primitives and an injected fetch implementation, so it does not yet establish after-fix behavior for the production review checkout or a real State Blob request.
  • Complete next step (P2) - The remaining merge blocker is contributor-provided real behavior proof, not a concrete repair task for the automation lane.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +35/-24; tests +65 The small production refactor is paired with focused timeout coverage across both changed I/O paths.

Merge-risk options

Maintainer options:

  1. Add production-boundary timeout proof (recommended)
    Before merge, show redacted traces of the actual review hydration Git fetch and a controlled real State Blob HTTP request timing out and returning their intended failure behavior.
  2. Pause for a narrower operational decision
    Pause the PR if maintainers cannot accept the new fixed request deadlines without evidence that expected slow state publication remains safe.

Technical review

Best 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.

Labels

Label justifications:

  • P2: This improves bounded review-worker availability without evidence of an active user-facing outage.
  • merge-risk: 🚨 availability: New fixed deadlines can change whether slow review hydration or state publication completes or fails closed.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owners are review-blob hydration and signed State Blob posting. The supplied terminal output imports compiled helpers, but exercises only a generic sleep timeout and publishStateBlob with an injected mocked fetch; it does not run the actual hydration fetch or a real HTTP transport through the production boundary. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Remaining Git hydration fetch is bounded: The introduced fetch of missing review blobs now uses a 180-second timeout and returns the existing fail-closed hydration result on command failure. (src/clawsweeper-review-blobs.ts:388, 42871da02497)
  • State Blob requests receive a per-attempt deadline: The signed POST path retains its four-attempt retry behavior and now supplies a 15-second AbortSignal to each request. (src/state-blob-client.ts:104, 42871da02497)
  • Earlier review-checkout finding is addressed on the base: The current base already sets a 30-second deadline for ensureReviewTreeCommit, so the prior P1 about the exact review-checkout fetch does not remain on this head. (src/clawsweeper-review-blobs.ts:81, fa535d54a032)
  • Focused coverage exists but is transport-mocked: The new state-blob test verifies four aborting attempts using an injected fetch implementation; the Git test verifies configured timeout values and source wiring. (test/state-blob-client.test.ts:8, 42871da02497)
  • Relevant feature history: Recent main-history work on this review-blob path includes hydration robustness and checkout-access validation, supporting routing to the area contributors below. (src/clawsweeper-review-blobs.ts:228, 16e2959d529b)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Jesse Merhi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Martin Cleary: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add redacted after-fix traces for the actual review hydration fetch and a controlled real State Blob HTTP timeout; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-15T21:37:38.107Z sha 9f1a93e :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-24T08:12:01.735Z sha 9f1a93e :: needs real behavior proof before merge. :: [P1] Cover the current review-checkout fetch
  • reviewed 2026-08-25T09:18:01.008Z sha 73352c0 :: needs real behavior proof before merge. :: [P1] Bound the exact review-checkout Git fetch

@brokemac79

Copy link
Copy Markdown
Contributor

@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>
@SebTardif
SebTardif force-pushed the fix/review-io-timeouts branch from 73352c0 to 42871da Compare August 30, 2026 01:18
@SebTardif

Copy link
Copy Markdown
Contributor Author

@brokemac79

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.

We want to continue. The branch is now replayed onto current main (cherry-pick of the timeout commit, not a rebase of the old history). Review-blob hydration still bounds git fetch at 180s and local git at 60s. Each state-blob POST still uses AbortSignal.timeout(15000) and the existing four-attempt backoff.

Current main already gives ensureReviewTreeCommit a 30s fetch deadline, so that checkout path is no longer unbounded before hydration.

Please take another look when you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants