Skip to content

[Feat] Record a death certificate when the OpenCode server exits unexpectedly - #145

Merged
daniel-lxs merged 1 commit into
developfrom
feat/opencode-death-certificate
Jul 10, 2026
Merged

[Feat] Record a death certificate when the OpenCode server exits unexpectedly#145
daniel-lxs merged 1 commit into
developfrom
feat/opencode-death-certificate

Conversation

@daniel-lxs

Copy link
Copy Markdown
Member

PR 2 of the observability pair — #140 (the diagnostic rail) is merged; this is the first producer on it. Replaces #141, which GitHub auto-closed when the stacked base branch was deleted.

What

The worker keeps a bounded ring (last 50 lines, 300 chars each) of the OpenCode subprocess's output while it runs. If the process exits while the task is not being cancelled, it records one durable opencode_unexpected_exit diagnostic event: exit code, signal, uptime, output tail, and a /proc/meminfo + worker-RSS snapshot taken at the instant of death — plus a Sentry capture so we get alerted. Reconnect-spawned processes are certified individually.

This is the fact that decides the open mystery: exit 137 = killed for memory (the 128 MiB request theory), a panic stack = OpenCode bug, clean exit = something else. The last two investigations couldn't make that distinction because the evidence died with the sandbox.

Guarantees (per the requirements)

  • Performance: constant cost per output line (ring append with caps); certification work happens only on unexpected exits.
  • Behavior: observer-only. The exit hook cannot alter the harness lifecycle — cancellation-driven exits are excluded, callback failures are caught and logged, reconnect behavior is untouched.
  • Privacy: the output tail flows through the rail's secret scrubbing (keys, bearer tokens, JWTs, credential assignments) and per-line/message caps before persisting; memory snapshot is numbers only.

Tests

Collector: exit facts + tagged tail, ring/line caps, blank-line and missing-/proc safety. Full harnesses + run-task suites: 423 tests pass; typecheck / eslint / prettier / knip clean.

Not auto-merged — ready for review.

🤖 Generated with Claude Code

…pectedly

When the OpenCode subprocess dies mid-task today, the worker silently
reconnects and every fact about the death vanishes with the sandbox.
The worker now stands witness: a bounded ring of the subprocess's last
output lines is kept while it runs, and an exit while the task is not
being cancelled records a durable diagnostic run event with the exit
code, signal, uptime, output tail, and a memory snapshot taken at the
instant of death, plus a Sentry capture for alerting. Exit code 137
versus a panic stack versus a clean exit is the difference between
"killed for memory", "OpenCode bug", and "something else" — the
distinction the last two investigations could not make.

Collection cost is constant per output line, certification runs only
on unexpected exits, and the callback is observer-only: its failures
are logged and never touch the harness lifecycle. Output lines pass
through the diagnostic recorder's secret scrubbing before persisting.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@roomote-roomote

roomote-roomote Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Reviewed the change. 2 findings to consider (both non-blocking; finding 1 is partly acknowledged in the PR discussion). See task

  • apps/worker/src/sandbox-server/lib/harnesses/opencode-server/start.ts:258-299 — Reconnect/restart-driven SIGTERM terminations are certified as unexpected exits and also raise a Sentry alert on every routine reconnect.
  • apps/worker/src/sandbox-server/lib/harnesses/opencode-server/exit-certificate.ts:78-85 — Memory snapshot is captured after the process has already exited, so the "instant of death" OOM evidence is weakened.

Review findings

# Severity File Lines Issue
1 🟡 Medium apps/worker/src/sandbox-server/lib/harnesses/opencode-server/start.ts 258-299 Every deliberate reconnect/restart is certified as an unexpected exit and fires a Sentry alert.
2 🟢 Low apps/worker/src/sandbox-server/lib/harnesses/opencode-server/exit-certificate.ts 78-85 Memory snapshot is read post-exit and reflects the witness process, not the dead OpenCode process.

1. Reconnect/restart terminations produce false certificates and Sentry noise.
certifyExit only suppresses exits when cancelSignal.aborted is true or execa reports isCanceled. On a normal task completion this is safe, because awaitSubprocess calls controller.abort() (backing cancelSignal) before the subprocess exits (apps/worker/src/run-task/run-task.ts:1798-1807). But ReconnectableHarness.terminateSubprocess tears the process down with a direct subprocess.kill('SIGTERM') (apps/worker/src/run-task/reconnectable-harness.ts:443-463) during disconnected, restartRequested, and requestReconnect (e.g. the MCP env-var/reconnect paths). That kill sets neither cancelSignal.aborted nor execa's isCanceled, so certifyExit runs and records an opencode_unexpected_exit diagnostic and calls captureWorkerException for each routine reconnect. You already flagged the diagnostic-event SIGTERM noise in the PR discussion; worth calling out that the Sentry captureException alert fires on the same path (fingerprinted per task), which directly competes with the OOM signal this feature is meant to surface. A cheap guard (e.g. an "expected restart" flag set right before terminateSubprocess, or skipping the Sentry capture for SIGTERM-without-exit-code) would remove most of it.

2. Memory snapshot is taken after death, not at it.
readMemorySnapshot() runs inside build(), which is only invoked from certifyExit after the subprocess promise has already settled — i.e. after the process exited and the kernel reclaimed its pages. For the OOM case this is exactly when /proc/meminfo MemAvailable recovers, so the snapshot understates the pressure that killed the process. Additionally workerRssBytes is process.memoryUsage().rss of the sandbox-server witness, not the OpenCode subprocess, so it doesn't reflect the victim's footprint. Exit code 137/SIGKILL remains the reliable OOM signal, so this is a documentation/expectations nit more than a defect — the field comment and PR wording ("snapshot taken at the instant of death") slightly oversell what is captured.

@daniel-lxs

Copy link
Copy Markdown
Member Author

Carrying over the known-noise caveat from #141: deliberate harness restarts (e.g. the MCP-reconnect respawn path) terminate opencode without the task-cancel signal, so they will be certified as SIGTERM exits. The killer signatures that matter (exit 137 / SIGKILL / panic stacks) are unambiguous; a follow-up can thread an expected-restart flag to silence the SIGTERM entries.

@daniel-lxs
daniel-lxs merged commit 9b3fcee into develop Jul 10, 2026
1 check passed
@daniel-lxs
daniel-lxs deleted the feat/opencode-death-certificate branch July 10, 2026 22:40
daniel-lxs added a commit that referenced this pull request Jul 10, 2026
…icate (#147)

* [Fix] Address review findings on the diagnostic rail and death certificate

Review follow-up for #140 and #145, which merged before their bot
reviews were read:

- Deliberate harness terminations (reconnect, restart, teardown) are no
  longer certified as unexpected exits: ReconnectableHarness marks the
  subprocess before killing it, and certification skips marked exits,
  which also silences the Sentry alert that fired on every routine
  reconnect.
- The death tail no longer depends on the startup URL wait leaving its
  line listeners attached: dedicated readers feed the ring for the
  whole subprocess lifetime.
- The run-events reader returns the newest events under its limit
  (descending query, reversed to chronological), so a busy task cannot
  push the diagnostics it exists to surface out of the window.
- A caller-supplied details.kind can no longer override the recorder's
  classification.
- Hash-shaped values (git SHAs, digests) keep an 8-character prefix
  when redacted, preserving their evidentiary value without exposing
  credential-length material.
- The memory reading is named and documented as what it is: the sandbox
  observed just after the exit, by the surviving worker process — the
  exit code/signal remains the authoritative OOM signal.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Mark expected exits only while the subprocess is alive

A crash surfaces as a disconnect first, and the disconnect cleanup then
kills the already-dead process; marking it there raced the exit
certification and could suppress the certificate for exactly the crash
the rail exists to record. The mark now applies only to a process that
is still alive at kill time, so deliberate teardowns stay quiet and
self-inflicted deaths stay certified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant