fix(ai): stop a buffered-CLI timeout burning the full retry budget, and two subprocess leaks (#9476, #9479) - #9505
Merged
Merged
Conversation
…nd two subprocess leaks (#9476, #9479) claude --output-format json buffers its whole response, so ANY run exceeding its effort timeout has produced zero stdout when the deadline lands -- which trips the first-output watchdog and throws claude_stalled_no_output rather than subscription_cli_timeout. The retry-break tested for the latter by strict string equality, so it never matched: every timed-out review burned all three attempts (3x180s at default effort, 3x600s at the top tier) before the fallback model was tried at all. With QUEUE_CONCURRENCY defaulting to 8 that parks the whole queue during a provider slowdown, and the per-provider circuit breaker needs three FULL-LENGTH failures before it trips. Matched by prefix, since these errors carry a detail suffix -- the strict equality is the original bug. subscription_cli_timeout was effectively unreachable for claude-code, making #3987's fix dead code on this deployment. child.on("error") catches spawn failures only; it never receives stdio stream errors. A CLI that exits before draining stdin (an unknown flag on an upgraded binary, an auth abort, an OOM kill) made the ~250KB stdin write fail with EPIPE on an emitter with no error listener, which Node escalates to an uncaught exception -> exit(1), taking down every in-flight queue job in the container. Per-call temp dirs were never removed. Every AI review minted one and, where repo review instructions are configured, wrote the composed system prompt into it -- so they accumulated on the container's writable overlay layer until recreation, leaving those instructions on disk indefinitely. Removed in the same finally that records CLI usage metrics, best-effort so a cleanup failure can never turn a completed review into a thrown error. The systemAppend test now reads the appended-prompt file inside the spawn stub rather than after the call returns, and additionally asserts the directory does not outlive the call -- which is both the new behaviour and a closer match to how the file is actually used.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-28 00:43:41 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9505 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.91%
==========================================
Files 843 843
Lines 110073 110083 +10
Branches 26194 26197 +3
==========================================
- Hits 98573 97591 -982
- Misses 10238 11520 +1282
+ Partials 1262 972 -290
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…9504/#9505 merges (#9509) * test(gate): cover the fail-safe marker-clear path (#9462) Mirrors the sibling visual-capture-satisfied fail-safe test: a rejecting clear must not throw out of the pass, since a failed clear only means the gate stays deferred until the head moves. (cherry picked from commit efa6472) * refactor(gate): drop an unreachable spread arm and annotate the defensive head-SHA guard (#9462) screenshotTableEvidenceUnresolved is computed as a plain boolean, so the undefined arm of its conditional spread was dead code -- pass it directly. The head-SHA guard in the exhausted branch is genuinely defensive (a recapture-preview job is only minted for a PR that had one), matching the sibling mark write, so it carries a reason rather than a contrived test. (cherry picked from commit 4f59499) * test(ai): exclude two unreachable best-effort cleanup arms from coverage (#9479) rm with force:true does not throw for a missing path, so the catch needs a filesystem-level failure no unit test can portably induce; the unset-cwd guard is only reachable when mkdtemp itself threw, i.e. no directory was ever created. Both annotated with their reason, matching the file's existing convention. (cherry picked from commit bf3efe6) * test(ai): drive the real subprocess EPIPE path and the non-Error stall guard (#9476, #9479) The EPIPE regression uses the existing real-subprocess harness rather than a stubbed spawn, so the stdin error listener is genuinely exercised: a fake CLI exits immediately without draining stdin, and the test asserts no uncaughtException reaches the process. Verified to fail without the listener (it captures an uncaught EPIPE), which is precisely the crash that took down every in-flight queue job in the container. Also covers isStalledNoOutput rejecting a non-Error throw -- a provider adapter can reject with a string, and misclassifying that as a deadline signal would silently skip the retry budget. (cherry picked from commit c770cc1) * test(rate-limit): cover the inline-retry break for an over-budget Retry-After (#9494) Asserts the attempt COUNT, not just the classification: a 60s Retry-After yields exactly one attempt instead of four, and a short one still uses the full inline budget so the break stays narrow. (cherry picked from commit 92813bf)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three AI-subprocess defects from the 2026-07-27 audit: a retry-break that never matched the error it was written for, and two ways a review subprocess leaks or kills its worker.
Closes #9476
Partially addresses #9479 (see Scope below)
#9476 — every timed-out Claude review burned the full retry budget
claude --output-format jsonbuffers its entire response, so any run that exceeds its effort timeout has produced zero stdout bytes when the deadline lands. That trips the first-output watchdog (resolveClaudeFirstOutputTimeoutMs, deliberately clamped totimeoutMs - 1by #5053) rather than the plain timeout, so the adapter throwsclaude_stalled_no_output: <detail>— neversubscription_cli_timeout.The retry-break tested for that with strict string equality, so it never matched. Consequences:
QUEUE_CONCURRENCYdefaulting to 8, a provider slowdown parks the whole queue in that state.subscription_cli_timeoutis effectively unreachable for claude-code, making fix(selfhost): stop retrying a CLI provider after it times out once #3987's fix dead code on this deployment.Now matched by prefix, because these errors carry a
: detailsuffix — the strict equality is precisely the bug. Covers thecodex_stalled_no_outputsibling too.The existing test suite pinned the error classification but nothing pinned the retry behaviour, which is why this survived.
#9479a — unhandled stdin EPIPE killed the whole worker
child.on("error")catches spawn failures only; it never receives stdio stream errors. If the CLI exits before draining stdin — an unknown flag on an upgraded binary, an immediate auth abort, an OOM kill — the ~250KB stdin write fails withEPIPEon an emitter with no"error"listener. Node escalates that to an uncaught exception →installSelfHostCrashHandlers→exit(1), taking down every in-flight queue job in the container.One listener fixes it. The real failure is already surfaced by the exit-code and empty-output guards, so nothing diagnostic is lost.
#9479b — per-call temp dirs were never removed
Every AI review call mints
mkdtemp(/tmp/loopover-ai-*), and where repo review instructions are configured it writes the composed system prompt into that directory. Exhaustive search found no removal anywhere: nofinally, no retention sweep, notmpfiles.d, no tmpfs mount for the service, noTMPDIRin the Dockerfile. They accumulate on the container's writable overlay layer until it is recreated — and leave repo review instructions on disk indefinitely.Removed in the same
finallythat already records CLI usage metrics, for both the claude and codex paths. Best-effort by design: a cleanup failure must never turn a completed review into a thrown error, and container recreation still collects anything missed.Scope
#9479 also bundles force-push debounce (every dedup layer is head-SHA-keyed, so N pushes in a minute cost N full prologues and N LLM calls). That is a coalescing/scheduling change of a different shape and size, and it interacts with the ordering work in #9499 — it stays open for that PR rather than being half-done here. #9479 is therefore partially addressed; I've left it open deliberately rather than closing it on two of three.
#9477 (cache fingerprint omitting the prompt version) is not in this PR either — it needs a fingerprint bump plus a drift check, and belongs with its own migration of cached rows.
Validation
npx tsc --noEmit -p tsconfig.json— cleantest/unit/ai-review.test.ts+test/unit/selfhost-ai.test.ts— 449 passedNew tests:
claude_stalled_no_outputandcodex_stalled_no_outputstop after one attempt and fall through to the fallback (parameterised, using realistic messages with their detail suffix so prefix matching is genuinely exercised).ECONNRESETstill gets the full 3-attempt budget.One existing test changed: the
systemAppendtest readsystem-append.txtafter the call returned, which now correctly finds the directory gone. It reads the file inside the spawn stub instead — which also matches reality better, since the file exists exactly for the CLI's lifetime — and additionally asserts the directory does not outlive the call, pinning the new cleanup.