Skip to content

fix(runtime): preserve the abort reason across cascades - #820

Merged
drewstone merged 1 commit into
mainfrom
fix/abort-reasons
Aug 13, 2026
Merged

fix(runtime): preserve the abort reason across cascades#820
drewstone merged 1 commit into
mainfrom
fix/abort-reasons

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

Cascaded aborts discard the upstream reason.

linkSignals, mergeAbortSignals, and the sandbox-leaf cascade each listened for abort and called controller.abort() with no argument. So a parent abort that named its cause — e.g. 'root driver failed; child settle grace expired' — arrived at the child as a reasonless abort, and the runtime renders a reasonless AbortError with the generic message execution aborted. That string is what the child's down record carries.

The information was known and then thrown away in transit.

Measured on one fleet (281 runs): 65 of 146 children settled down (45%), and execution aborted is the single largest bucket at 18 — none diagnosable from the journal.

Fix

  • Every cascade forwards the firing signal's reason (linkSignals, mergeAbortSignals, sandbox-leaf).
  • Two abort sites that could kill a worker while staying silent are now named: the supervisor's root-failure path when no settle grace is configured (the grace-timer branch immediately above it already named the same event), and executor teardown.
  • A bare abort() sets a DOMException whose message is the platform placeholder "This operation was aborted" — no more diagnostic than the generic death it would replace — so the helpers treat it as reasonless and name the parent scope instead.

Verification

  • 5 new tests in src/runtime/supervise/abort-reason.test.ts: string reason forwarded, either-side firing, already-aborted-before-linking, Error unwrapped to its message, and the placeholder case.
  • pnpm run test → 2587 passed | 6 skipped (212 files).
  • pnpm run lint, npx tsc --noEmit → clean.

🤖 Generated with Claude Code

A cascaded abort dropped the upstream reason. linkSignals,
mergeAbortSignals and the sandbox-leaf cascade all listened for 'abort'
and called controller.abort() with no argument, so a parent abort that
DID name its cause ('root driver failed; child settle grace expired')
reached the child as a reasonless abort. The runtime then renders a
reasonless AbortError as the generic 'execution aborted', which is what
every downstream worker's down record ends up saying.

Measured impact on one fleet: 65 of 146 children down (45%), and
'execution aborted' is the single largest bucket at 18 — none of them
diagnosable from the journal, because the reason was discarded in
transit rather than never known.

Every cascade now forwards the firing signal's reason. Two abort sites
that could kill a worker while staying silent are named: the supervisor's
root-failure path with no settle grace (the grace-timer branch beside it
already named the same event) and executor teardown.

A bare abort() sets a DOMException carrying the platform placeholder
'This operation was aborted', which is no more diagnostic than the
generic death, so the helpers treat it as reasonless and name the parent
scope instead. 5 new tests pin the contract: string reasons, either-side
firing, already-aborted-before-linking, Error unwrapping, and the
placeholder case. Suite 2587/2593, lint and typecheck clean.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 560a934d

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-08-13T09:36:44Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Value Audit — better-approach-exists

Verdict better-approach-exists
Concerns 4 (1 strong-concern, 3 medium-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 96.7s (2 bridge agents)
Total 96.7s

💰 Value — better-approach-exists

A sound, well-motivated fix that forwards abort reasons through 3 of ~5 cascade helpers, but leaves the same defect in 2 sibling helpers — including the exported mergeAbortSignals sitting 15 lines from the new reason helper, feeding the primary executor.

  • What it does: Makes every cascaded abort (parent signal → child controller) carry the upstream reason string instead of calling controller.abort() with no argument. Adds an abortReasonOf() extractor that unwraps string/Error reasons and falls back to 'aborted by parent scope' when the platform placeholder DOMException would carry no information. Applies it to: the private mergeAbortSignals in environment-provid
  • Goals it achieves: Make child-worker mortality diagnosable from the journal. Before: a parent abort with a named cause arrived at the child reasonless, and the child's down record rendered the generic 'execution aborted'. After: the upstream cause (e.g. 'root driver failed; child settle grace expired') propagates end-to-end. The PR body's measured motivation (18 of 65 down-records on one fleet = 'execution aborted
  • Assessment: The change is coherent and in the grain of the codebase where it applies: it reuses the existing cascade structure, only swaps the abort() argument, and the abortReasonOf helper handles the three real reason shapes (string, Error, bare-abort placeholder) correctly. The tests pin the contract at the right level (the linking shape, not implementation details). However, the fix is incomplete: the PR
  • Better / existing approach: Consolidate to ONE shared reason-forwarding cascade helper and apply it to ALL five sites, rather than fixing three and leaving two. Searched: grepped all controller.abort()/c.abort() across src/ (37 hits), all mergeAbortSignals/linkSignals call sites (11 hits), and read each cascade helper. The exported mergeAbortSignals (runtime.ts:4766) is literally adjacent to the new abortReasonOf (runtime.ts
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — better-approach-exists

Correct fix for the right problem, but applied to only 3 of 5 identical cascade sites — the shared exported helper and a sibling private copy still drop the reason on the main executor turn loop and bridge paths.

  • Integration: All three modified helpers have live callers and are fully reachable. linkSignals (runtime.ts:4735) is called at runtime.ts:607; mergeAbortSignals (environment-provider.ts:1362) at environment-provider.ts:394; streamSandboxLeaf's inline cascade is the sandbox leaf itself (runtime.ts:1430, reached from :1375). The supervisor's named-abort edit (supervisor.ts) sits in drainLiveChildren, the
  • Fit with existing patterns: Follows the codebase's existing cascade idiom (per-signal listener, { once: true }, pre-aborted fast path). The reason-extraction rule (string passes; non-AbortError Error unwrapped to message; else fallback) is sound and matches how the runtime renders signal.reason. The streamSandboxLeaf rewrite also fixes a latent bug by giving the two listeners stable identities so removeEventListener
  • Real-world viability: Holds on concurrency and edge paths: either-side firing, pre-aborted-before-linking, and Error reasons are all handled; the stable-listener-identity fix makes teardown correct under fast-abort. One defensible edge: an Error whose name is 'AbortError' but whose message is meaningful gets replaced by the fallback — acceptable, since that name is the platform convention for a reasonless abort.
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟠 Fix lands on 3 of 5 identical cascade helpers; the exported shared one is still reasonless on the main executor paths [problem-fit] ``

The PR's own thesis is that a cascade calling controller.abort() with no argument renames every downstream death to the undiagnosable generic 'execution aborted'. Two siblings have the identical bug and were not touched: (1) the EXPORTED mergeAbortSignals at runtime.ts:4766 — still onAbort = () => c.abort() / c.abort() — whose doc comment says it exists 'so sibling leaf executors share the one portable implementation'. It is called by the routerInlineExecutor turn loop (runtime.ts:940),

💰 Value Audit

🔴 Primary executor path still drops abort reasons via the un-fixed exported mergeAbortSignals [better-architecture] ``

The exported mergeAbortSignals(...signals) at runtime.ts:4766 still calls c.abort() with no argument (line 4768, 4771). It is used by routerInlineExecutor (runtime.ts:940), streamBridgeSession (runtime.ts:2252), and bridgeWorktreeStream (runtime.ts:3939). routerInlineExecutor is the primary inline executor — the highest-traffic child-mortality path. This is the exact bug the PR eliminates elsewhere: child deaths through this executor will still render 'execution aborted' in the journal. The

🟠 worktree-cli-executor.ts:316 linkSignals is a verbatim pre-fix copy left untouched [duplication] ``

worktree-cli-executor.ts:316-327 defines a linkSignals identical to the pre-fix version in runtime.ts (reasonless c.abort() at lines 319, 323). It is used at worktree-cli-executor.ts:167 to link the caller signal with the executor controller — the same cascade the PR fixed in runtime.ts:4735. Same defect, same fix applies. This is a copy of the helper that should be replaced by the shared implementation rather than maintained separately.

🟠 Reason-extraction logic is implemented twice identically [duplication] ``

abortReasonOf (runtime.ts:4751-4761) and the inline reasonOf closure (environment-provider.ts:1366-1375) have identical bodies: string-length check, Error-with-non-AbortError-name-and-message check, and 'aborted by parent scope' fallback. environment-provider.ts cannot import from the supervise module without creating a layering concern, but the logic should live in one place both can reach (a shared runtime util) rather than being maintained in lockstep. If the extraction rule changes (e.g. to


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260813T094025Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 560a934d

Review health 100/100 · Reviewer score 69/100 · Confidence 65/100 · 12 findings (6 medium, 6 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 69 69 69 69
Confidence 65 65 65 65
Correctness 69 69 69 69
Security 69 69 69 69
Testing 69 69 69 69
Architecture 69 69 69 69

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Test re-implements the helper instead of importing production code — src/runtime/supervise/abort-reason.test.ts

The file defines its own link + reasonOf and imports nothing from runtime.ts/environment-provider.ts. Its header says it pins 'the CONTRACT at the level any linking helper must satisfy', but it exercises a local clone, not linkSignals, not the private mergeAbortSignals, not abortReasonOf. If someone reverts or alters the production abortReasonOf (fallback string, Error-unwrap, AbortError filter), this test stays green. Impact: the test certifies behavior the runtime may no longer have — the diagnostic invariant this PR exists to enforce is unguarded. Fix: import and call the real helpers (linkSignals is module-private — either export it or move the test to exercise mergeAbortSignals / the sandbox-leaf cascade via the public seam), or extract abortReasonOf to a shared util

🟠 MEDIUM Test re-implements the logic and never exercises production code — src/runtime/supervise/abort-reason.test.ts

abort-reason.test.ts imports only vitest and defines its own local link() and reasonOf() (lines 14-29) that duplicate the production logic verbatim. It never imports or invokes abortReasonOf/linkSignals from runtime.ts, mergeAbortSignals from environment-provider.ts, or drainLiveChildren from supervisor.ts. The test therefore pins a copy, not the source of truth: if someone reverts runtime.ts:4738 to c.abort() (dropping the reason) or changes the Error-unwrap branch, this suite still passes. The stated intent is 'pin the CONTRACT', but the contract is only pinned by coincidence of copy-paste. Fix: import the actual helpers and assert on the rea

🟠 MEDIUM Tests pin a copy of the logic, not the shipped code — src/runtime/supervise/abort-reason.test.ts

All five cases run against a local re-implementation link() (lines 9-28) that duplicates abortReasonOf/mergeAbortSignals, and the header admits it ('using the same shape the runtime's linkSignals / mergeAbortSignals / sandbox cascade implement'). If linkSignals (runtime.ts:4735), environment-provider mergeAbortSignals (1362), or the streamSandboxLeaf cascade regress — e.g. revert to bare abort() — every test still passes. Also none of the cases exercise the PR's stated user-visible outcome: a supervisor journal down record carrying the reason; they only assert on a standalone controller's .reason. Fix: export the helper under test (or impo

🟠 MEDIUM Fix incomplete: runtime's exported mergeAbortSignals still drops the reason — src/runtime/supervise/runtime.ts

The PR's headline symptom (cascaded deaths journaled as 'execution aborted') persists on the three live paths that use the N-ary mergeAbortSignals at runtime.ts:940 (routerToolsInlineExecutor external merge), 2252 (streamBridgeSession), and 3939 (cli bridgeWorktreeStream): its handler is still () => c.abort() with no reason (line 4768), so the downstream turnController/executor rejects with a bare AbortError and scope.ts:2360 falls back to 'execution aborted'. The PR fixed only linkSignals + the environment-provider copy. The test file's header claims to pin the contract 'any linking helper must satisfy' and names mergeAbortSignals, but the tested copy f

🟠 MEDIUM Fix is incomplete: sibling cascade helpers still drop the abort reason — src/runtime/supervise/runtime.ts

The PR adds abortReasonOf (line 4751) and wires it into linkSignals and streamSandboxLeaf, but the adjacent mergeAbortSignals (line 4766, body unchanged) still aborts with bare c.abort() (lines 4768 and 4771), dropping the reason. This helper is on real cascade paths: runtime.ts:940 (routerInlineExecutor/room loop), runtime.ts:2252 (streamBridgeSession), and runtime.ts:3939 (bridgeWorktree

🟠 MEDIUM Stated invariant only half-applied: exported mergeAbortSignals still drops the reason — src/runtime/supervise/runtime.ts

The PR rationale (environment-provider.ts:1364) is that dropping signal.reason 'renamed every cascaded death to the generic execution aborted'. Yet the EXPORTED mergeAbortSignals(...signals) at runtime.ts:4766-4777 still does const onAbort = () => c.abort() and c.abort() with no argument — unchanged. This helper feeds the three hot cascade points: the chat-with-tools turn loop (runtime.ts:940 mergeAbortSignals(signal, controller.signal)), and two more executors (2252, 3939). So the dominant cascades in this file still lose their upstream reason, and the diagnostic improvement is inconsistent — a worker killed via the turn-loop path still journals as a generic abort while a sibling killed via linkSignals journals the cause. Fix: route the exported helper through abortReasonOf

🟡 LOW Fallback 'aborted by parent scope' misattributes executor-owned bare aborts — src/runtime/environment-provider.ts

Eleven executors in runtime.ts still abort their OWN controller bare (lines 591, 892, 952, 1279, 1343, 1395, 1583, 1613, 1846, 1979, 4041), so their signal.reason is a bare DOMException. reasonOf()/abortReasonOf() classify any Error named AbortError as reasonless (environment-provider.ts:1371, runtime.ts:4757) and stamp 'aborted by parent scope' — but the abort did not come from a parent scope, and scope.ts:2357's abortError() builds real AbortError-named Errors that DO carry a diagnostic message which this name-based filter silently discards. Cosmetic today (no down-record consumer keys on that text), but the label can actively misdirect a journal reade

🟡 LOW Reason-extraction logic duplicated in three places — src/runtime/environment-provider.ts

abortReasonOf (runtime.ts:4751, with a fallback param), the inline reasonOf here (hardcoded fallback, no param), and the test's reasonOf are the same predicate. The two production copies can drift — e.g. environment-provider's copy cannot be called with a custom fallback and silently diverges if the runtime copy's filter changes. Fix: export abortReasonOf from runtime.ts (or a shared util) and import it here; delete the inline copy.

🟡 LOW Reason-extraction logic triplicated with divergent fallback signatures — src/runtime/environment-provider.ts

Three near-identical copies of the reason-extraction heuristic now exist: environment-provider.ts reasonOf (line 1366, hardcodes fallback 'aborted by parent scope'), runtime.ts abortReasonOf (line 4751, fallback is a parameter defaulting to the same string), and the test's reasonOf (line 15). The runtime version and the environment-provider version will drift (the fallback is alread

🟡 LOW Heuristic forwards AbortSignal.timeout() placeholder as a 'reason' — src/runtime/supervise/runtime.ts

The guard only special-cases reason.name === 'AbortError'. Node's AbortSignal.timeout() sets a DOMException with name 'TimeoutError' and message 'The operation was aborted due to timeout' (empirically confirmed: instanceof Error is true on this runtime). That value passes the name !== 'AbortError' branch and is forwarded as the reason — a platform placeholder message of the same flavor the PR comment itself calls 'no more diagnostic than the generic death'. Consequence is cosmetic: a timeout-driven cascade journals 'The operation was aborted due to timeout' instead of the named-scope fallback. Fix: also treat the TimeoutError placeholder (or check for the platform placeholder message) as reasonless.

🟡 LOW linkSignals listeners never removed; PR's own stability comment is applied inconsistently — src/runtime/supervise/runtime.ts

streamSandboxLeaf was rewritten in this PR to use stable per-signal closures precisely so its finally can removeEventListener by reference (runtime.ts:1432-1430, 1528-1529) — the PR's own comment names this contract. linkSignals registers { once: true } listeners on a and b but has no cleanup; if neither parent fires while the linked signal is in use, the listeners linger on long-lived parent signals for the linked controller's lifetime. Pre-existing, not introduced here, but the PR establishes the cleanup pattern one function over and leaves this one without it. Optional consistency fix: return an unlink helper or document why linkSignals callers never need explicit teardown.

🟡 LOW drainLiveChildren labels a successful drain as 'root driver failed' — src/runtime/supervise/supervisor.ts

This branch runs whenever the run reaches the join barrier with live children and no settle grace (call site supervisor.ts:705 in the finally of root execution), including when the root driver SUCCEEDED and only async children remain in-flight (grace is 0 unless actOutcome.ok === false, line 701). The new reason string asserts a driver failure that did not occur, writing a factually wrong cause into every torn-down child's journal. Classification is unaffected (executionAborted is captured at line 681 before the drain)


tangletools · 2026-08-13T09:42:25Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 12 non-blocking findings — 560a934d

Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 1/1 planned shots over 4 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-13T09:42:25Z · immutable trace

@drewstone
drewstone merged commit 2201968 into main Aug 13, 2026
4 checks passed
@drewstone
drewstone deleted the fix/abort-reasons branch August 13, 2026 10:18
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.

2 participants