Skip to content

Commit af1cffb

Browse files
committed
fix(reliability,execution): make cancellation, goldens, diffs, and leak 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
1 parent 48efc45 commit af1cffb

32 files changed

Lines changed: 1254 additions & 141 deletions

docs/RELIABILITY_ARCHITECTURE.md

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,13 @@ Rules that keep journeys trustworthy:
169169
not globally, because the actor model legitimately interleaves.
170170
- **Every journey states its invariant set explicitly** (§6) and the
171171
harness refuses a journey that asserts nothing beyond "completed".
172+
- **A declared golden decides the verdict.** `assertions.outputs` /
173+
`assertions.streamShape` compare every surface's record against
174+
`expected/` and fail the journey on a mismatch — invariants and the
175+
cross-surface diff both pass when every surface returns the same wrong
176+
answer. Faulted runs skip the comparison (and say so): a golden
177+
describes the unfaulted contract. `nodetool reliability update-goldens
178+
<journey>` recaptures the fixtures from an unfaulted kernel run.
172179

173180
## 5. Which workflows belong in the suite
174181

@@ -234,7 +241,11 @@ the checks that convert today's advisory logging into failures:
234241
- Terminal precedence is exactly cancel > suspend > failed > completed
235242
(`runner.ts:625`) — asserted, not assumed.
236243
- Exactly one terminal `job_update` per run, and nothing follows it on
237-
that job's stream.
244+
that job's stream. A driver records repeated frames rather than
245+
dropping them; a repeat its surface documents (the ws-server's eager
246+
`running`/`cancelled` acks and its authoritative terminal snapshot) is
247+
tagged `redundant` and discounted here and in the cross-surface diff.
248+
Any other duplicate is untagged, and fails.
238249

239250
**Cleanup and leaks.**
240251
- `_checkPendingInboxWork` finding pending work is a journey **failure**,
@@ -247,6 +258,11 @@ the checks that convert today's advisory logging into failures:
247258
repeated journey iterations (leak journeys run N=20 and compare).
248259
- Cancellation completes within a budget (e.g. 5 s) and `finalize()` ran
249260
for every started actor.
261+
- The counters above are **measured** — the kernel driver reads them off
262+
`ExecutionSession.resourceCounters()`, the ws-server driver off the
263+
runner's `slotCounters` once the connection is gone. A journey that
264+
declares `cleanup-leaks` on a surface whose driver produces no post-run
265+
snapshot fails on that: "nothing measured" must not read as "no leaks".
250266

251267
**Determinism.**
252268
- Two runs of the same journey with the same cassettes produce identical

packages/cli/src/commands/reliability.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,25 @@ export function registerReliabilityCommands(program: Command): void {
7070
}
7171
});
7272

73+
reliability
74+
.command("update-goldens <journey>")
75+
.description(
76+
"Rewrite a journey's expected/ fixtures from a fresh unfaulted kernel run — " +
77+
"for a golden that legitimately moved. Review the diff before committing"
78+
)
79+
.action(async (journeyName: string) => {
80+
try {
81+
const { updateJourneyGoldens } = await import(
82+
"@nodetool-ai/reliability-harness"
83+
);
84+
const { written } = await updateJourneyGoldens(journeyName);
85+
for (const file of written) console.log(`wrote ${file}`);
86+
} catch (e) {
87+
console.error(String(e));
88+
process.exit(1);
89+
}
90+
});
91+
7392
reliability
7493
.command("list")
7594
.description("List the journeys under reliability/journeys/")

packages/execution/README.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,14 @@ const session = await ExecutionSession.create({
8181
persistence: { onAccepted, onTerminal } | null,
8282
limits: { runTimeoutMs, bufferLimit }, // nodeTimeoutMs: not yet supported, see above
8383
requireTerminalResult: true, // optional — output-name rewriting
84+
captureMessages: true, // optional — required to read `messages` below
8485
});
8586

87+
// Only with `captureMessages: true`, and only if you actually drain it:
88+
// capture queues each message until a consumer pulls it, so a host that just
89+
// awaits `result` would hold the whole run's messages (audio chunks included)
90+
// for nothing. Reading `messages` without the flag throws; a consumer that
91+
// falls behind `limits.messageBufferLimit` makes the iterator throw.
8692
for await (const message of session.messages) { /* ProcessingMessage */ }
8793

8894
await session.pushInput("input_name", value);

packages/execution/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
export { ExecutionSession } from "./session.js";
55
export { normalizeGraph } from "./normalize-graph.js";
6+
export { DEFAULT_MESSAGE_BUFFER_LIMIT } from "./message-stream.js";
67
export type {
78
BridgeFactory,
89
Edge,

packages/execution/src/message-stream.ts

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,40 @@
33
* `AsyncIterable<ProcessingMessage>`. Modeled on the queue/wake pattern in
44
* `packages/workflow-runner/src/run.ts`'s `runWorkflow` generator — the one
55
* other place in the repo already streams a kernel run as an async sequence.
6+
*
7+
* The queue is bounded. A caller that opts into capture (see
8+
* `ExecutionSessionOptions.captureMessages`) but never iterates would
9+
* otherwise retain every message of the run — including real-time audio
10+
* chunks, which the kernel's own message retention deliberately avoids
11+
* holding (roughly 25 MB/minute). Past the limit the stream stops queueing
12+
* and the iterator throws, so a non-draining consumer fails loudly instead of
13+
* growing until the run ends.
614
*/
715
import type { ProcessingContext } from "@nodetool-ai/runtime";
816
import type { ProcessingMessage } from "@nodetool-ai/protocol";
917

18+
/** Queued messages allowed to pile up before the stream gives up. */
19+
export const DEFAULT_MESSAGE_BUFFER_LIMIT = 10_000;
20+
1021
export class MessageStream implements AsyncIterable<ProcessingMessage> {
1122
private readonly queue: ProcessingMessage[] = [];
1223
private waiter: (() => void) | null = null;
1324
private closed = false;
25+
private overflowedAt: number | null = null;
26+
private readonly limit: number;
1427
private readonly unsubscribe: () => void;
1528

16-
constructor(context: ProcessingContext) {
29+
/** `limit` of 0 (or a negative number) means unbounded. */
30+
constructor(context: ProcessingContext, limit = DEFAULT_MESSAGE_BUFFER_LIMIT) {
31+
this.limit = limit > 0 ? limit : Number.POSITIVE_INFINITY;
1732
this.unsubscribe = context.addMessageListener((message) => {
33+
if (this.queue.length >= this.limit) {
34+
if (this.overflowedAt === null) {
35+
this.overflowedAt = this.queue.length;
36+
this.unsubscribe();
37+
}
38+
return;
39+
}
1840
this.queue.push(message);
1941
this.wake();
2042
});
@@ -39,6 +61,15 @@ export class MessageStream implements AsyncIterable<ProcessingMessage> {
3961
while (this.queue.length > 0) {
4062
yield this.queue.shift()!;
4163
}
64+
if (this.overflowedAt !== null) {
65+
throw new Error(
66+
`ExecutionSession message stream overflowed after ${this.overflowedAt} ` +
67+
`queued messages (limit ${this.limit}) — the consumer fell behind, ` +
68+
"so messages past that point were dropped rather than retained. " +
69+
"Iterate `session.messages` as the run progresses, or raise " +
70+
"`limits.messageBufferLimit`."
71+
);
72+
}
4273
if (this.closed) return;
4374
await new Promise<void>((resolve) => {
4475
this.waiter = resolve;

packages/execution/src/session.ts

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ export class ExecutionSession {
3030
readonly graph: HydratedGraphData;
3131

3232
private readonly runner: WorkflowRunner;
33-
private readonly stream: MessageStream;
33+
/** Null unless the caller opted into `captureMessages` (see options). */
34+
private readonly stream: MessageStream | null;
3435
private readonly persistence: ExecutionSessionOptions["persistence"];
36+
private readonly bridge: { pendingRequestCount?: number } | null;
3537
private readonly resultPromise: Promise<RunResult>;
3638
private runTimeoutHandle: ReturnType<typeof setTimeout> | null = null;
3739
private _cancelReason: string | null = null;
@@ -45,15 +47,21 @@ export class ExecutionSession {
4547
persistence: ExecutionSessionOptions["persistence"];
4648
params: Record<string, unknown>;
4749
triggerEvent: ExecutionSessionOptions["triggerEvent"];
50+
bridge: { pendingRequestCount?: number } | null;
4851
closeBridge: () => void;
4952
runTimeoutMs: number | undefined;
53+
captureMessages: boolean;
54+
messageBufferLimit: number | undefined;
5055
}) {
5156
this.jobId = init.jobId;
5257
this.workflowId = init.workflowId;
5358
this.graph = init.graph;
5459
this.runner = init.runner;
5560
this.persistence = init.persistence;
56-
this.stream = new MessageStream(init.context);
61+
this.bridge = init.bridge;
62+
this.stream = init.captureMessages
63+
? new MessageStream(init.context, init.messageBufferLimit)
64+
: null;
5765

5866
if (init.runTimeoutMs && init.runTimeoutMs > 0) {
5967
this.runTimeoutHandle = setTimeout(() => {
@@ -80,7 +88,7 @@ export class ExecutionSession {
8088
// The terminal message was emitted synchronously before run()
8189
// resolved (ProcessingContext.emit() calls listeners inline), so the
8290
// stream can close now without dropping it.
83-
this.stream.close();
91+
this.stream?.close();
8492
});
8593

8694
this.resultPromise
@@ -206,13 +214,28 @@ export class ExecutionSession {
206214
persistence: options.persistence ?? null,
207215
params: options.params ?? {},
208216
triggerEvent: options.triggerEvent ?? null,
217+
bridge,
209218
closeBridge,
210-
runTimeoutMs: options.limits?.runTimeoutMs
219+
runTimeoutMs: options.limits?.runTimeoutMs,
220+
captureMessages: options.captureMessages === true,
221+
messageBufferLimit: options.limits?.messageBufferLimit
211222
});
212223
}
213224

214-
/** Validated, live message stream — closes once the run reaches a terminal state. */
225+
/**
226+
* Live message stream — closes once the run reaches a terminal state.
227+
* Requires `captureMessages: true` at `create()`; without it nothing is
228+
* queued (see that option) and reading this throws rather than handing back
229+
* a stream that would silently yield nothing.
230+
*/
215231
get messages(): AsyncIterable<ProcessingMessage> {
232+
if (!this.stream) {
233+
throw new Error(
234+
"ExecutionSession: `messages` requires `captureMessages: true` at " +
235+
"create() — message capture is opt-in so a host that only awaits " +
236+
"`result` never queues a run's messages unread."
237+
);
238+
}
216239
return this.stream;
217240
}
218241

@@ -221,6 +244,31 @@ export class ExecutionSession {
221244
return this.resultPromise;
222245
}
223246

247+
/**
248+
* Live resource counts, for leak accounting: after a terminal result every
249+
* one of these must be back to zero. Measured, not inferred — the
250+
* reliability harness's `cleanup-leaks` invariant asserts against these
251+
* numbers and reports a violation when a driver can't produce them.
252+
*/
253+
resourceCounters(): {
254+
liveActors: number;
255+
pendingControlResponses: number;
256+
pendingTimers: number;
257+
pythonBridgePendingRequests: number;
258+
} {
259+
return {
260+
liveActors: this.runner.liveActorCount,
261+
pendingControlResponses: this.runner.pendingControlResponseCount,
262+
// The session's own run-timeout timer is the only timer it owns; it is
263+
// cleared when the run settles.
264+
pendingTimers: this.runTimeoutHandle === null ? 0 : 1,
265+
pythonBridgePendingRequests:
266+
typeof this.bridge?.pendingRequestCount === "number"
267+
? this.bridge.pendingRequestCount
268+
: 0
269+
};
270+
}
271+
224272
/** The reason passed to the most recent `cancel()` call, if any. */
225273
get cancelReason(): string | null {
226274
return this._cancelReason;

packages/execution/src/types.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,14 @@ export interface ExecutionLimits {
6262
nodeTimeoutMs?: number;
6363
/** Forwarded to `WorkflowRunnerOptions.bufferLimit` (per-inbox cap). */
6464
bufferLimit?: number | null;
65+
/**
66+
* Cap on messages queued for an un-drained `session.messages` consumer
67+
* (default {@link DEFAULT_MESSAGE_BUFFER_LIMIT}); `0` disables the cap. Only
68+
* meaningful together with `captureMessages`. Past the cap the stream stops
69+
* queueing and the iterator throws — a consumer that fell behind fails
70+
* loudly instead of retaining the whole run's message history.
71+
*/
72+
messageBufferLimit?: number;
6573
}
6674

6775
export interface ExecutionSessionOptions {
@@ -144,6 +152,17 @@ export interface ExecutionSessionOptions {
144152
* surface and the one place strict mode is meant to be on by default.
145153
*/
146154
strict?: boolean;
155+
/**
156+
* Capture every emitted message into `session.messages` (default `false`).
157+
* Off by default because capture is retention: the queue holds each message
158+
* until a consumer pulls it, and a host that only awaits `session.result`
159+
* (every production caller today) would grow one unread queue per run — the
160+
* kernel is explicit about not retaining real-time chunks for exactly that
161+
* reason. Turn it on only when actually iterating the stream (the
162+
* reliability harness's kernel driver does); see `limits.messageBufferLimit`
163+
* for the cap that catches a consumer falling behind.
164+
*/
165+
captureMessages?: boolean;
147166
}
148167

149168
export type { NodeDescriptor, Edge, ProcessingMessage, RunResult };
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/**
2+
* Message capture is opt-in and bounded: a host that only awaits
3+
* `session.result` (every production caller) must not accumulate an unread
4+
* queue of the run's messages, and a consumer that opts in but falls behind
5+
* must fail loudly instead of retaining everything.
6+
*/
7+
import { describe, it, expect } from "vitest";
8+
import { ExecutionSession } from "../src/index.js";
9+
import { buildTestRegistry } from "./fixtures.js";
10+
11+
const NO_BRIDGE = async () => null;
12+
13+
const DOUBLE_GRAPH = {
14+
nodes: [
15+
{ id: "v", type: "nodetool.input.Value", properties: {} },
16+
{ id: "double", type: "test.execution.Double", properties: {} }
17+
],
18+
edges: [
19+
{ source: "v", sourceHandle: "output", target: "double", targetHandle: "value" }
20+
]
21+
};
22+
23+
describe("ExecutionSession — message capture", () => {
24+
it("queues nothing and refuses `messages` when capture is not requested", async () => {
25+
const registry = buildTestRegistry();
26+
const session = await ExecutionSession.create({
27+
graph: DOUBLE_GRAPH,
28+
registry,
29+
bridgeFactory: NO_BRIDGE,
30+
params: { v: 5 }
31+
});
32+
33+
expect(() => session.messages).toThrow(/captureMessages/);
34+
const result = await session.result;
35+
expect(result.status).toBe("completed");
36+
});
37+
38+
it("throws once an un-drained consumer exceeds the buffer limit", async () => {
39+
const registry = buildTestRegistry();
40+
const session = await ExecutionSession.create({
41+
graph: {
42+
nodes: [{ id: "loop", type: "test.execution.Loop", properties: {} }],
43+
edges: []
44+
},
45+
registry,
46+
bridgeFactory: NO_BRIDGE,
47+
captureMessages: true,
48+
limits: { runTimeoutMs: 500, messageBufferLimit: 3 }
49+
});
50+
51+
// Let the run emit well past the limit before pulling anything.
52+
await session.result;
53+
54+
const drain = async (): Promise<void> => {
55+
for await (const _message of session.messages) {
56+
// drain
57+
}
58+
};
59+
await expect(drain()).rejects.toThrow(/overflowed/);
60+
});
61+
});

packages/execution/tests/session-run.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ describe("ExecutionSession — messages", () => {
105105
},
106106
registry,
107107
bridgeFactory: NO_BRIDGE,
108-
params: { v: 5 }
108+
params: { v: 5 },
109+
captureMessages: true
109110
});
110111

111112
const seen: string[] = [];

0 commit comments

Comments
 (0)