fix(claude): guard cost/health bookkeeping so a mid-stream claude exit can't crash the tick (#272) #276

Merged

joshtronic merged 1 commit from fix/claude-cost-record-crash into master

2026-06-27 13:41:14 +00:00

igor commented

2026-06-27 13:33:30 +00:00

Collaborator

Fixes the overnight #272 crash — and corrects its premise: it was never OOM.

Evidence it was not memory: 31 GiB RAM (27 free, swap untouched), exit code 1 not 137 (an OOM-kill is SIGKILL→137), and zero kernel OOM-killer entries. The 822M peak was just Chromium/Playwright's normal footprint.

The actual bug: after the claude pipe, claude_run_with_cost re-enables set -e and calls cost_record_cli unguarded. When claude exits nonzero mid-stream, the stream JSONL is truncated → the cost parse fails → set -e turns that into a raw status=1 that exits the whole tick before return $rc, bypassing the caller's set +e guard and its claude exited N handling. So a recoverable model crash became a unit FAILURE + a from-scratch redo (exactly the "exit 1, no claude exited line" signature in the journal).

The else-branch health classification was already || true everywhere ("never kill the tick") — cost_record_cli and the rc==0 claude_health_record_ok were simply missed. Now both are guarded.

bash -n + check-sync green. No unit test (the claude invocation isn't a sourceable seam, same as the worktree/tick paths). Closes #272. No reviewer.

Fixes the overnight #272 crash — and corrects its premise: **it was never OOM.** **Evidence it was not memory:** 31 GiB RAM (27 free, swap untouched), exit code **1** not **137** (an OOM-kill is SIGKILL→137), and zero kernel OOM-killer entries. The 822M peak was just Chromium/Playwright's normal footprint. **The actual bug:** after the claude pipe, `claude_run_with_cost` re-enables `set -e` and calls `cost_record_cli` **unguarded**. When claude exits nonzero mid-stream, the stream JSONL is truncated → the cost parse fails → `set -e` turns that into a raw `status=1` that exits the whole tick *before* `return $rc`, bypassing the caller's `set +e` guard and its `claude exited N` handling. So a recoverable model crash became a unit FAILURE + a from-scratch redo (exactly the "exit 1, no `claude exited` line" signature in the journal). The else-branch health classification was already `|| true` everywhere ("never kill the tick") — `cost_record_cli` and the rc==0 `claude_health_record_ok` were simply missed. Now both are guarded. `bash -n` + check-sync green. No unit test (the claude invocation isn't a sourceable seam, same as the worktree/tick paths). Closes #272. No reviewer.

igor added 1 commit

2026-06-27 13:33:30 +00:00

fix(claude): bookkeeping can't crash the tick on a mid-stream claude exit

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

Details

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

Details

7f0e8ef90a

The REAL cause of the overnight #272 crash -- NOT OOM (31G RAM, exit code 1 not
137, no kernel OOM-killer; 822M was just Chromium's normal footprint).

After the claude pipe, claude_run_with_cost re-enables set -e and runs
cost_record_cli UNGUARDED. When claude exits nonzero mid-stream the JSONL is
truncated, so the cost parse fails -> set -e turns that into a raw status=1 that
exits the whole tick BEFORE 'return $rc' -- bypassing the caller's set+e guard and
its 'claude exited N' handling, so the work is discarded and redone (the exact
signature: status=1, no 'claude exited' line). The else-branch health code was
already '|| true' everywhere ('never kill the tick'), but cost_record_cli and the
rc==0 claude_health_record_ok were missed. Now both are guarded, so a recoverable
model crash returns its exit code gracefully instead of crashing the unit.

Closes #272.

No unit test (the claude invocation isn't a sourceable seam, like the other
tick/worktree paths); bash -n + check-sync green.

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

igor referenced this pull request

2026-06-27 13:34:47 +00:00

agent.service exited 1 mid-rework on porksicle.com#89 (822M peak, work lost and redone) #272

igor commented

2026-06-27 13:37:12 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 7f0e8ef9: success

Summary: Adds two || true guards in claude_run_with_cost (cost_record_cli and the rc==0 claude_health_record_ok) so a mid-stream claude crash that truncates the stream JSONL can't turn a bookkeeping failure into a set -e exit that bypasses the caller's claude exited N handling. Correct, tightly scoped, and consistent with the existing "bookkeeping never kills the tick" design.

Findings:

Logic is sound: rc is captured from PIPESTATUS[0] before the guards, and return $rc later preserves claude's actual exit code. The guards only suppress failures in the two non-critical bookkeeping side-effects, which is exactly the intent. The change can only make the path more error-tolerant — it has essentially no regression surface.
set -e semantics check out: cmd || true is a tested compound command, so neither bookkeeping call can abort the function, and the swallowed nonzero status is intentional.
Scope is clean (8 lines, one function, on-topic for #272). No CI-config or unrelated changes.
No fabricated checklist — the description is prose and its claims (guards added to the two previously-unguarded calls) match the diff exactly. The OOM-vs-exit-1 diagnosis is well-argued and doesn't affect the correctness of the code change.

Caveats I'm trusting (non-blocking):

No regression test. The author justifies this by claude_run_with_cost not being a sourceable/mockable seam (the real claude invocation precedes the guarded lines), consistent with the worktree/tick paths. Plausible, but unverifiable from the diff — if the function were sourceable with a stubbed cost_record_cli, a set -e-doesn't-exit test would be the ideal regression guard for a bug that is fundamentally about set -e behavior. Worth keeping in mind for the next time this path is refactored.
I can't see the full else-branch to independently confirm "|| true everywhere," but the in-code comment corroborates it and, since this change is purely additive guarding, that claim doesn't affect this diff's correctness.

CI is green; change is safe to merge as-is.

_{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 `7f0e8ef9`: **success** **Summary:** Adds two `|| true` guards in `claude_run_with_cost` (`cost_record_cli` and the rc==0 `claude_health_record_ok`) so a mid-stream claude crash that truncates the stream JSONL can't turn a bookkeeping failure into a `set -e` exit that bypasses the caller's `claude exited N` handling. Correct, tightly scoped, and consistent with the existing "bookkeeping never kills the tick" design. **Findings:** - Logic is sound: `rc` is captured from `PIPESTATUS[0]` before the guards, and `return $rc` later preserves claude's actual exit code. The guards only suppress failures in the two non-critical bookkeeping side-effects, which is exactly the intent. The change can only make the path *more* error-tolerant — it has essentially no regression surface. - `set -e` semantics check out: `cmd || true` is a tested compound command, so neither bookkeeping call can abort the function, and the swallowed nonzero status is intentional. - Scope is clean (8 lines, one function, on-topic for #272). No CI-config or unrelated changes. - No fabricated checklist — the description is prose and its claims (guards added to the two previously-unguarded calls) match the diff exactly. The OOM-vs-exit-1 diagnosis is well-argued and doesn't affect the correctness of the code change. **Caveats I'm trusting (non-blocking):** - No regression test. The author justifies this by `claude_run_with_cost` not being a sourceable/mockable seam (the real claude invocation precedes the guarded lines), consistent with the worktree/tick paths. Plausible, but unverifiable from the diff — if the function were sourceable with a stubbed `cost_record_cli`, a `set -e`-doesn't-exit test would be the ideal regression guard for a bug that is fundamentally about `set -e` behavior. Worth keeping in mind for the next time this path is refactored. - I can't see the full else-branch to independently confirm "`|| true` everywhere," but the in-code comment corroborates it and, since this change is purely additive guarding, that claim doesn't affect this diff's correctness. CI is green; change is safe to merge as-is. --- <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-27 13:37:12 +00:00

1 minute 15 seconds

igor requested review from joshtronic

2026-06-27 13:37:13 +00:00