fix(claude): stop claude_run_with_cost leaking errexit into its caller (igor#291) #296

Merged

joshtronic merged 1 commit from fix/claude-errexit-leak into master

2026-06-30 15:10:30 +00:00

igor commented

2026-06-30 14:05:32 +00:00

Collaborator

Root cause (reproduced, not guessed)

The tick died status=1, no claude exited N line — the #279/#291 signature — whenever claude exited nonzero mid-stream. It is an errexit leak, not OOM:

claude_run_with_cost runs its stream pipeline under set +e, then restored errexit with an unconditional set -e before its bookkeeping. Every caller (PR-review, issue-work, maintenance, site-work) wraps the call in set +e specifically to capture and handle a nonzero exit — but the function's set -e clobbers that guard, so errexit is ON when the function returns. The nonzero return $rc then trips errexit in the caller and kills the tick before it can read $?. The set -e added to protect the bookkeeping is what defeated the caller's guard.

I reproduced it in isolation (stub claude that emits a stream then exit 1 → caller aborted before the line after the call) and in a 6-line minimal case.

Fix

Capture the caller's errexit on entry, restore that (not an unconditional set -e). The bookkeeping is already || true-guarded, so it's safe with errexit off. bin/test-claude-errexit.sh locks it in — it fails against the old code (caller aborted) and passes against the fix (nonzero surfaces as a return value); errexit state preserved in both directions.

Notes

Not Node-specific, not OOM. The bug fires on any nonzero claude exit; heavy Node-repo reworks (long runs → max-turns / transient API hiccups) just hit nonzero exits most. The box has 31G free, zero kernel OOM-kill events at either crash time, and the service has no memory cap — the "3.1G peak" in #291 was incidental.
Relationship to #294: that contained the loop (escalate after 2 crashes) + fixed the crashlog trap. This fixes the crash, so a recoverable model exit stays handled and the bot can rework Node-repo PRs instead of bouncing every one to you.
make test green (check-sync + all 8 unit tests, incl. the new one). shellcheck clean on the changed lines.

Closes #291

## Root cause (reproduced, not guessed) The tick died `status=1`, no `claude exited N` line — the #279/#291 signature — whenever `claude` exited nonzero mid-stream. It is an **errexit leak**, not OOM: `claude_run_with_cost` runs its stream pipeline under `set +e`, then restored errexit with an **unconditional `set -e`** before its bookkeeping. Every caller (PR-review, issue-work, maintenance, site-work) wraps the call in `set +e` *specifically* to capture and handle a nonzero exit — but the function's `set -e` clobbers that guard, so errexit is ON when the function returns. The nonzero `return $rc` then trips errexit in the **caller** and kills the tick before it can read `$?`. The `set -e` added to protect the bookkeeping is what defeated the caller's guard. I reproduced it in isolation (stub `claude` that emits a stream then `exit 1` → caller aborted before the line after the call) and in a 6-line minimal case. ## Fix Capture the caller's errexit on entry, restore **that** (not an unconditional `set -e`). The bookkeeping is already `|| true`-guarded, so it's safe with errexit off. `bin/test-claude-errexit.sh` locks it in — it **fails against the old code** (caller aborted) and **passes against the fix** (nonzero surfaces as a return value); errexit state preserved in both directions. ## Notes - **Not Node-specific, not OOM.** The bug fires on *any* nonzero claude exit; heavy Node-repo reworks (long runs → max-turns / transient API hiccups) just hit nonzero exits most. The box has 31G free, **zero kernel OOM-kill events** at either crash time, and the service has no memory cap — the "3.1G peak" in #291 was incidental. - **Relationship to #294:** that contained the *loop* (escalate after 2 crashes) + fixed the crashlog trap. This fixes the *crash*, so a recoverable model exit stays handled and the bot can rework Node-repo PRs instead of bouncing every one to you. - `make test` green (check-sync + all 8 unit tests, incl. the new one). shellcheck clean on the changed lines. Closes #291

igor added 1 commit

2026-06-30 14:05:32 +00:00

fix(claude): stop claude_run_with_cost leaking errexit into its caller (igor#291)

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

9e2ff1d0ef

A nonzero claude exit -- a recoverable model crash mid-stream -- crashed the
whole tick with status=1 and no "claude exited" line (the #279/#291 signature),
instead of being caught and handled. It re-attempted the work next tick and,
on a heavy repo, looped.

Root cause (reproduced, not guessed): claude_run_with_cost runs its stream
pipeline under `set +e`, then restored errexit with an UNCONDITIONAL `set -e`
before its bookkeeping. Every caller (PR-review, issue-work, maintenance,
site-work) wraps the call in `set +e` precisely to capture and handle a nonzero
exit -- but the function's `set -e` clobbers that guard, so errexit is ON when
the function returns. The nonzero `return $rc` then trips errexit in the CALLER
and takes the tick down before it can read $?. The `set -e` that was added to
protect the bookkeeping is exactly what defeated the caller's guard.

Fix: capture the caller's errexit on entry and restore THAT (not an
unconditional `set -e`). The bookkeeping is already `|| true`-guarded
throughout, so it is safe with errexit off. Adds bin/test-claude-errexit.sh,
which reproduces the crash against the old code (caller aborted) and passes
against the fix (nonzero surfaces as a return value).

Why it looked Node-specific: the bug fires on ANY nonzero claude exit; heavy
Node-repo PR reworks (long runs -> max-turns / transient API hiccups) just hit
nonzero exits most often. It is NOT OOM -- the box has 31G free, there were no
kernel OOM-kill events at either crash time, and the service has no memory cap;
the "3.1G peak" noted in #291 was incidental.

Relationship to #294: that change contained the loop (escalate to the human
after 2 crashes) and made the crashlog trap fire; this fixes the underlying
crash, so a recoverable model exit stays a handled outcome and the bot can
rework Node-repo PRs instead of bouncing every one to the reviewer.

Closes #291

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor referenced this pull request

2026-06-30 14:05:52 +00:00

agent.service exits 1/FAILURE mid-PR-review with 3.1G memory peak #291

igor commented

2026-06-30 14:08:01 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 9e2ff1d0: success

Surgical fix to an errexit leak in claude_run_with_cost: replaces an unconditional set -e restore with one that restores the caller's captured errexit state. Correct, in scope, well-tested, CI green.

Why the fix is right

lib/claude.sh:169 captures _caller_errexit via case $- in *e*) before the internal toggling, so it reflects the caller's state on entry (functions don't subshell, so $- is accurate here). $- correctly still shows e even when the call sits left of || (errexit is suppressed but the flag remains set), so the errexit-ON-with-|| true caller path is handled.
The replaced line (if [ "$_caller_errexit" -eq 1 ]; then set -e; else set +e; fi) is behaviorally identical to the old set -e whenever the caller had errexit on, and strictly safer (errexit off) when the caller had it off. So the only behavioral change is exactly the bug fix — the bookkeeping that previously ran under errexit-ON now runs under errexit-OFF for set +e callers, which can only reduce the chance of an abort, never increase it. This means I don't have to fully see the (unshown) bookkeeping guards to be confident there's no regression.

Test is genuinely discriminating

bin/test-claude-errexit.sh reproduces the caller pattern in a standalone subshell captured via the parent's set +e (not || true), correctly avoiding the trap where bash disables errexit inside &&/||/if operands — which would have masked the very leak under test. I traced all three checks against both the buggy and fixed code: each fails on the old code (subshell aborts before writing the marker, or wrong errexit state) and passes on the fix. The ON-preserved check also implicitly guards against the capture being misplaced after the internal set +e.

Non-blocking notes (no action required)

The test is skip-safe (exits 0 with a notice if jq/timeout/tee/mktemp is absent). If a CI runner lacked one of these, "make test green" could be a silent skip rather than a real pass — but the fix is sound on its own reasoning regardless.
set +o pipefail is still restored unconditionally rather than to the caller's pipefail state. That's pre-existing, out of scope, and harmless to the crash (pipefail doesn't trigger an abort), but worth a future tidy if you ever want full state symmetry.

Scope is ~133 lines, focused on the one issue; no CI-config or unrelated changes; no security surface; description checklist matches the diff. CI is success.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `APPROVE` _(automated)_ CI for `9e2ff1d0`: **success** **Surgical fix to an errexit leak in `claude_run_with_cost`: replaces an unconditional `set -e` restore with one that restores the caller's captured errexit state. Correct, in scope, well-tested, CI green.** ### Why the fix is right - `lib/claude.sh:169` captures `_caller_errexit` via `case $- in *e*)` *before* the internal toggling, so it reflects the caller's state on entry (functions don't subshell, so `$-` is accurate here). `$-` correctly still shows `e` even when the call sits left of `||` (errexit is suppressed but the flag remains set), so the errexit-ON-with-`|| true` caller path is handled. - The replaced line (`if [ "$_caller_errexit" -eq 1 ]; then set -e; else set +e; fi`) is **behaviorally identical to the old `set -e` whenever the caller had errexit on**, and strictly *safer* (errexit off) when the caller had it off. So the only behavioral change is exactly the bug fix — the bookkeeping that previously ran under errexit-ON now runs under errexit-OFF for `set +e` callers, which can only reduce the chance of an abort, never increase it. This means I don't have to fully see the (unshown) bookkeeping guards to be confident there's no regression. ### Test is genuinely discriminating - `bin/test-claude-errexit.sh` reproduces the caller pattern in a standalone subshell captured via the parent's `set +e` (not `|| true`), correctly avoiding the trap where bash disables errexit inside `&&`/`||`/`if` operands — which would have masked the very leak under test. I traced all three checks against both the buggy and fixed code: each fails on the old code (subshell aborts before writing the marker, or wrong errexit state) and passes on the fix. The ON-preserved check also implicitly guards against the capture being misplaced after the internal `set +e`. ### Non-blocking notes (no action required) - The test is skip-safe (exits 0 with a notice if `jq`/`timeout`/`tee`/`mktemp` is absent). If a CI runner lacked one of these, "make test green" could be a silent skip rather than a real pass — but the fix is sound on its own reasoning regardless. - `set +o pipefail` is still restored unconditionally rather than to the caller's pipefail state. That's pre-existing, out of scope, and harmless to the crash (pipefail doesn't trigger an abort), but worth a future tidy if you ever want full state symmetry. Scope is ~133 lines, focused on the one issue; no CI-config or unrelated changes; no security surface; description checklist matches the diff. CI is `success`. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-30 14:08:02 +00:00

2 minutes 3 seconds

igor requested review from joshtronic

2026-06-30 14:08:02 +00:00