fix(review): retry an OOM-killed harness and stop blaming the PR

Merged
KH
2mo ago
+372-6

Why

A customer's review run failed with The review harness exited with code 137 and they asked, reasonably, "is it on my side or your side?"

137 is 128+SIGKILL — the sandbox OOM-killed the harness on a ~1 GB checkout while the sandbox itself survived (the mount sidecar kept running and shut down cleanly, which is the cgroup OOM-killer signature). Nothing in the PR caused it and nothing in the PR can fix it. But the agent reported it through failReviewRun, which posts "No review was posted; this needs operator attention" on the PR — ambiguous about whose operator, and it reads as though the author must act.

Changes

Retry once, and only for 137/143. Those are the cases where the process was killed outright and no review work completed, so a second attempt costs nothing. A genuine non-zero harness exit is still never retried — the first pass may already have pushed mechanical fix commits, so re-running is not free. That distinction is the point of the change and is covered by a test.

Report an infra kill as infrastructure. abandonReviewRunToInfraKill says plainly on the PR that the sandbox ran out of resources, that it's our limit and not their code, and that the review runs again on the next push. It skips the Slack failure ping (an OOM isn't an actionable review result) but still throws, so the run is recorded as failed and reaches our alerting.

The policy is extracted as runReviewHarnessWithRetry, which takes the runner and the logger as arguments and performs no I/O — so it's tested without a sandbox, GitHub, or Slack.

Scope

This is mitigation, not the fix. It converts an unlucky run into a retried one and a confusing message into an honest one.

The durable fix is sandbox sizing, and that turns out to be harder than cloud#1988 assumed — see my comment there. The Daytona snapshot already requests { cpu: 4, memory: 8, disk: 10 }, and 8 GB is Daytona's hard per-sandbox ceiling (Memory request 16GB exceeds maximum allowed per sandbox (8GB)). Raising it needs a quota increase, not a code change.

Verification

  • 5 new tests in tests/review-agent.test.mjs covering: retry-then-succeed, retry-then-still-killed, no-retry on a real failure, no-retry on success, and exit-code classification.
  • npm test: 354 pass. The 2 failures are pre-existing and environmental — the sibling ../workforce/packages/harness-kit checkout has only dist/ and no package.json; they reproduce on a clean tree.
  • npx tsc --noEmit: clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BmMcdDakbnDkHBxG4pit7z

Summary by cubic

The review harness now retries once when the sandbox OOM-kills it (exit codes 137/143) and stops reporting those kills as PR-author failures, plus bounds harness memory so out-of-memory runs surface as readable errors instead of bare SIGKILLs.

  • Runs the harness under a 4 GiB V8 heap cap with serial execution pinned for Vitest/Turbo/Jest, so a memory-exhausted run throws "JavaScript heap out of memory" with a stack instead of dying silently.
  • Logs the tail of the harness output, stderr, and duration on failure so future kills are diagnosable; the diagnostic hook is owned by the retry helper so it can't be dropped from the failure path.
  • Only OS kills are retried; genuine harness failures still fail immediately because the first pass may already have pushed fix commits.
  • An infra kill now posts a message on the PR saying the sandbox ran out of resources on our side, that nothing needs fixing, and that the review reruns on the next push. It skips the Slack failure ping but still marks the run failed for alerting.

This is mitigation; the durable fix is raising Daytona's 8 GiB per-sandbox memory ceiling, which requires a quota increase rather than a code change.