Skip to content

[Fix] Address review findings on the diagnostic rail and death certificate - #147

Merged
daniel-lxs merged 2 commits into
developfrom
fix/diagnostic-review-findings
Jul 10, 2026
Merged

[Fix] Address review findings on the diagnostic rail and death certificate#147
daniel-lxs merged 2 commits into
developfrom
fix/diagnostic-review-findings

Conversation

@daniel-lxs

Copy link
Copy Markdown
Member

Follow-up addressing all outstanding openmote review findings on #140 and #145 (merged before the reviews were read — my miss).

Finding Fix
🔴 #141/#145-1: deliberate reconnects/restarts certified as unexpected exits + Sentry alert on every routine reconnect terminateSubprocess marks the subprocess (WeakSet) before killing; certification skips marked exits. All deliberate teardown paths funnel through that one method.
🟡 #141-2: death tail silently depended on the URL-wait never detaching its line listeners Dedicated lifetime readers feed the ring; the onLine coupling is removed.
🟡 #140-1: reader returned the oldest 500 events, dropping end-of-run diagnostics on busy tasks Query descending with the limit, reverse to chronological before returning.
🟢 #140-2: caller details.kind could override the recorder's classification kind is applied last.
🟢 #140-3: 40-hex git SHAs / digests fully redacted, blunting post-mortem value Hash-shaped values keep an 8-char identifying prefix (a1b2c3d4…[redacted]); real key formats (sk-/ghp_/JWT/assignments) remain fully redacted.
🟢 #145-2: "memory at the instant of death" overstated what's captured Renamed to memoryAfterExit with honest docs: sandbox-wide reading after reclaim, worker RSS is the witness; exit code/signal is the authoritative OOM signal.

Tests

Two new (hash-prefix redaction, kind-override guard); full run-task + harnesses suites: 425 pass; typecheck (worker+web) / eslint / prettier / knip clean.

Waiting for bot review + Dan before merge.

🤖 Generated with Claude Code

…icate

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>
@roomote-roomote

roomote-roomote Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Roomote review — no new code issues found. The previously flagged issue is resolved by the latest commit. See task

  • apps/worker/src/run-task/reconnectable-harness.ts:336 — The disconnected cleanup path marked the subprocess as an expected exit, which could suppress the death certificate for a crash-induced disconnect. Resolved: markExpectedSubprocessExitIfAlive only marks a still-alive subprocess, so a crash (already-dead process) is left unmarked and still gets certified.

Review notes

The latest commit resolves the outstanding finding.

terminateSubprocess now calls markExpectedSubprocessExitIfAlive, which refuses to mark a subprocess whose exitCode is already set or that was already signalled (expected-exit.ts:14-24). A genuine crash surfaces as a disconnect only after the process is already dead, so the passive disconnected cleanup no longer suppresses its death certificate — while deliberate teardown of a live process (routine reconnect/restart, including a transient blip on a still-healthy server) stays quiet. That is actually more precise than unconditionally skipping the mark on the passive path, since it avoids spurious certificates for healthy-server reconnects. Covered by three focused unit tests.

One residual, low-severity observation (not blocking): the guard depends on the crashed child's exit being observed before the socket-close-driven disconnected handler runs markExpectedSubprocessExitIfAlive. Those are independent OS notifications, so a narrow scheduling window remains where exitCode is still null at mark time and the certificate could still be suppressed. This is the same mark-vs-certifyExit race as the original finding, substantially narrowed rather than fully eliminated — fine to ship as-is.

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>
@daniel-lxs

Copy link
Copy Markdown
Member Author

Fixed the 🟡 finding: the expected-exit mark is now gated on the subprocess being alive at kill time (markExpectedSubprocessExitIfAlive). A crash that manifests as a disconnect leaves the process already dead when the cleanup path runs, so it is no longer marked and its certificate + Sentry alert fire as intended; deliberate teardowns of live processes stay quiet. Covered by three new unit tests (live → marked; crashed → not marked; already-signalled → not marked).

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