fix(reliability,execution): make cancellation, goldens, diffs, and leak accounting real - #4630
Merged
Merged
Conversation
…ak accounting real
Six defects found reviewing the execution/comparison stack:
1. Cancelled queued jobs could still execute. `ExecutionSession.create()`
starts the kernel, and the WS runner checked the persisted "cancelled"
status only afterwards, so the workflow ran (side effects included) and
the branch never cancelled the session. Persistence now runs before the
session is created. The regression test asserts the executor never ran,
not just the DB row.
2. Golden fixtures never reached the verdict. `compareJourney` evaluated
invariants and the cross-surface diff only, so every surface returning
the same wrong answer passed. `core/golden.ts` compares terminal outputs
and the normalized stream (per node-channel) against `expected/`, and
fails the surface on a mismatch; faulted runs skip with a stated reason.
`nodetool reliability update-goldens <journey>` recaptures fixtures from
an unfaulted kernel run; linear-text-pipeline's stale shape is refreshed
(constant-node frames now filtered by normalization, node_name is the
node's name).
3. Nested payload differences vanished from the diff. `canonicalKey` used
`JSON.stringify(msg, keys)`, whose array replacer is a key whitelist at
every depth, so `{result:{output:"A"}}` and `{result:{output:"B"}}`
serialized identically. Replaced with a recursive stable serializer.
4. Every production run built an unbounded, unread message queue.
`ExecutionSession` always constructed `MessageStream`; no production
caller iterates it. Capture is now opt-in (`captureMessages`) and
bounded (`limits.messageBufferLimit`), and overflow throws instead of
retaining.
5. `cleanup-leaks` measured nothing. No driver populated
`resourceCounters`, and `checkLeaks` returned success on absence. The
kernel driver now reads `ExecutionSession.resourceCounters()` (live
actors, pending control responses, timers, bridge requests) and the
ws-server driver reads the runner's slot counters after disconnect;
a missing post-run snapshot is a violation.
6. The ws-server driver hid duplicate terminal frames. It dropped every
repeated `job_update`, leaving "exactly one terminal update"
undetectable on the one surface with a wire. Repeats are recorded and,
only when they match this surface's documented shapes (eager
running/cancel acks, authoritative terminal snapshot), tagged
`redundant` — discounted by normalization and terminal-uniqueness.
Any other duplicate now fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dx19BNKrnpQNVD2W8eoA5y
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the workflow execution facade and the reliability harness so that cancellation, golden fixtures, diffs, duplicate terminal frames, and leak accounting are all actually enforced (i.e., previously “could not fail” checks now fail when they should).
Changes:
- Prevent queued-but-cancelled jobs from ever starting execution by moving persistence/status checks ahead of
ExecutionSession.create()in the WS runner, and strengthen the regression test accordingly. - Make golden fixtures decide the verdict by introducing per-surface comparisons for terminal outputs and normalized stream shape, plus a recursive stable JSON serializer to avoid “nested payloads compare equal” bugs.
- Make message capture opt-in and bounded in
ExecutionSession, and add measurable leak counters (kernel actors/control responses, python bridge pending requests, WS slot counters) socleanup-leaksasserts real data.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| reliability/journeys/linear-text-pipeline/expected/stream.shape.json | Updates the shipped stream-shape golden to match new normalization (drops trivial const frames; uses node name in node_name). |
| reliability/harness/tests/invariants/terminal-uniqueness.test.ts | Adds coverage for tagged (documented) redundant terminal frames vs untagged duplicates. |
| reliability/harness/tests/invariants/leaks.test.ts | Updates leak invariant expectations so “not instrumented” becomes a failure instead of silently passing. |
| reliability/harness/tests/golden.test.ts | Adds focused tests for golden output/stream-shape comparison and integration into compareJourney. |
| reliability/harness/tests/diff.test.ts | Adds regression tests ensuring nested payload differences and deep key-order invariance are detected correctly. |
| reliability/harness/tests/compare.test.ts | Adjusts compare tests to disable goldens when using stub-only job_update fixtures. |
| reliability/harness/src/index.ts | Exports stable-json and golden utilities for external/CLI use. |
| reliability/harness/src/drivers/ws-server.ts | Records duplicate job_update frames (tagging only documented redundancies) and captures post-disconnect WS slot counters for leak checks. |
| reliability/harness/src/drivers/kernel.ts | Enables bounded message capture for the kernel oracle driver and records measured resource counters after runs. |
| reliability/harness/src/core/stable-json.ts | Introduces recursive, key-order-stable JSON serialization to prevent nested-diff elision. |
| reliability/harness/src/core/record.ts | Adds RunFrame.redundant tagging to preserve duplicates while letting normalization/invariants discount documented repeats. |
| reliability/harness/src/core/normalize.ts | Drops frames tagged redundant during normalization so cross-surface diffs aren’t polluted by wire-only redundancy. |
| reliability/harness/src/core/invariants/terminal-uniqueness.ts | Discounts redundant frames when asserting single terminal job_update and no post-terminal updates. |
| reliability/harness/src/core/invariants/leaks.ts | Treats missing post-run counter snapshots as a violation (leaks.not-instrumented). |
| reliability/harness/src/core/golden.ts | Implements golden assertions for terminal outputs and per-channel stream shape with fault-aware skipping. |
| reliability/harness/src/core/diff.ts | Switches canonical diff keying to the new stable serializer to make nested differences visible. |
| reliability/harness/src/compare.ts | Integrates golden checks into per-surface results and verdict/issue formatting. |
| reliability/harness/src/cli.ts | Adds updateJourneyGoldens to rewrite expected fixtures from a fresh completed kernel run. |
| packages/websocket/tests/unified-websocket-runner.test.ts | Strengthens cancellation regression test to assert executor never runs and no completed frame is sent. |
| packages/websocket/src/unified-websocket-runner.ts | Moves job persistence/cancellation gate before kernel start; exposes slotCounters for leak accounting. |
| packages/websocket/src/test-ui-server.ts | Adds onRunnerCreated hook to expose per-connection runner (needed for WS slot leak measurements). |
| packages/runtime/src/python-bridge-base.ts | Exposes pendingRequestCount for leak accounting of outstanding python bridge requests. |
| packages/kernel/src/runner.ts | Adds measured leak counters (liveActorCount, pendingControlResponseCount) by tracking live actors and pending control responses. |
| packages/execution/tests/session-run.test.ts | Updates tests to opt into message capture when iterating session.messages. |
| packages/execution/tests/message-capture.test.ts | Adds tests validating capture is opt-in and bounded overflow is surfaced as an error. |
| packages/execution/src/types.ts | Adds captureMessages and limits.messageBufferLimit options with documentation for bounded retention. |
| packages/execution/src/session.ts | Makes message capture optional (stream nullable), adds resourceCounters() for leak accounting, and enforces messages access only when enabled. |
| packages/execution/src/message-stream.ts | Adds a bounded queue with overflow behavior (stop queueing + iterator throws) to prevent unbounded retention. |
| packages/execution/src/index.ts | Exports DEFAULT_MESSAGE_BUFFER_LIMIT. |
| packages/execution/README.md | Documents captureMessages and bounded buffering behavior for session.messages. |
| packages/cli/src/commands/reliability.ts | Adds nodetool reliability update-goldens <journey> command to recapture fixtures. |
| docs/RELIABILITY_ARCHITECTURE.md | Documents that declared goldens decide the verdict, redundant terminal frames are tagged/discounted, and leak counters must be measured. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes six defects in the execution facade and the reliability comparison stack. Four were checks that could not fail, and one let a cancelled job run.
1. Cancelled queued jobs could still execute (P1)
ExecutionSession.create()starts the kernel. The WS runner read the persisted job status after creating the session, so a job cancelled while queued (via the DB-only tRPCjobs.cancelpath, which leaves it injobQueue) executed anyway — side effects included — and the cancelled branch never calledsession.cancel().The persistence block now runs before the session is created. The existing regression test only asserted the DB row still read
cancelled, which the old code satisfied while running the workflow; it now asserts the executor was never invoked and that nocompletedframe was sent.2. Golden fixtures never reached the verdict (P1)
compareJourneyevaluated invariants and the cross-surface diff, and never touchedjourney.expected.outputs/journey.expected.streamShape. Kernel and ws-server could return the same wrong result and every journey passed.New
core/golden.tscompares terminal outputs and the normalized stream againstexpected/, per surface, and fails the surface on a mismatch. The stream comparison is per node-channel (the actor model legitimately interleaves across nodes, so a global index-wise compare would flag non-regressions). Faulted runs skip the comparison with a stated reason — a golden describes the unfaulted contract, and the fault journeys that declareoutputsdo so precisely because the run is supposed to fail under them.nodetool reliability update-goldens <journey>recaptures fixtures from a fresh unfaulted kernel run (refusing a run that didn't complete).linear-text-pipeline'sstream.shape.jsonwas stale and is refreshed: constant/input frames are now dropped by normalization, andnode_namecarries the node's name rather than its type. Values, statuses, edge updates and the terminal are unchanged.3. Nested payload differences vanished from the diff (P1)
canonicalKeyusedJSON.stringify(message, Object.keys(message).sort()). The array replacer is a key whitelist applied at every nesting level, so every nested object serialized as{}—{result:{output:"A"}}and{result:{output:"B"}}compared equal, hiding output, metadata and nested error divergences. Replaced with a recursive stable serializer (core/stable-json.ts), shared with the golden comparison.4. Every run built an unbounded, unread message queue (P1)
ExecutionSessionalways constructed aMessageStream, whose listener queued every message with no limit. No production caller iteratessession.messages— they awaitsession.result— so long-running runs retained the whole message history, including the real-time audio chunks the kernel deliberately avoids holding.Capture is now opt-in (
captureMessages) and bounded (limits.messageBufferLimit, default 10k); overflow stops queueing and makes the iterator throw rather than silently retaining. The reliability harness's kernel driver — the one consumer that drains the stream — opts in.5.
cleanup-leaksmeasured nothing (P2)checkLeaksreturned success whenresourceCounterswas absent, and no driver populated it, so every journey declaringcleanup-leaksasserted nothing.WorkflowRunnerexposesliveActorCount/pendingControlResponseCount;PythonBridgeBaseexposespendingRequestCount;ExecutionSession.resourceCounters()reports them plus its own timer.UnifiedWebSocketRunnerexposesslotCounters;createTestUiServergains anonRunnerCreatedhook so the harness can reach the per-connection runner, and the ws-server driver snapshots slots after the connection is torn down.6. The ws-server driver hid duplicate terminal frames (P2)
The driver dropped every repeated
job_updatestatus, so "exactly one terminal update" could not fail on the only surface with a real wire. Repeats are now recorded, and taggedredundantonly when they match this surface's documented shapes (the eagerrunningack, the eagercancelledack for a cancel this driver requested, and the authoritative terminal snapshot carryingresult). Tagged frames are dropped from cross-surface normalization and discounted byterminal-uniqueness; any other duplicate is untagged and fails both.Testing
reliability/harness: 144 passed, 5 skipped (25 files) — includes newgolden.test.ts, nested-payload and key-order diff cases,terminal-uniquenesstagged/untagged duplicate cases, andleaksnot-instrumented cases.packages/execution13,packages/kernel934,packages/websocket1992,packages/runtime2634,packages/cli444 — all passing.npm run typecheckclean for web/electron/packages (mobile fails only on uninstalled Expo deps in this environment);tsc --noEmitclean for every touched package.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dx19BNKrnpQNVD2W8eoA5y
Generated by Claude Code