Skip to content

Commit 535f8bf

Browse files
authored
🔒 Require scope-bound event listeners in Effection code (#748) (#757)
An event listener an Effection operation installs is state that operation owns, but nothing about an event source enforces that. A handler attached to a socket, a child process or a DOM target survived the operation that attached it, and kept writing to state the abandoned owner left behind. Cleanup that runs when the event fires is not cleanup: a cancelled wait is exactly the case where the event never arrives. `@effectionx/node` moves to 0.2.5, whose `once()` registers nothing until it is interpreted and detaches in a `finally` — after delivery, a halt, or losing a race. Every direct pin moves together; `@effectionx/process` stays at 0.8.1, which leaves one audited transitive 0.2.4 lock entry that nothing here resolves. `local/require-scope-bound-event-registration` makes the policy a blocking gate. It resolves sources, owners, `action` and `ensure` through their imports rather than their spelling, and an owner is the nearest generator or `action()` executor and nothing wider. Cleanup counts as established only when it is in place before the thing it releases exists: a lexical `finally` entered before the subscription, an `ensure()` that *completed* before it, or the cleanup an `action()` returns. An `ensure()` yielded afterwards is rejected, because entering it is itself a suspension — an owner halted there unwinds with no cleanup registered at all, measured, with the listener still attached. Every applicable registration is migrated onto one of those shapes. The three owners that own a child process register their cleanup before spawning it, and that cleanup keeps every handler attached until the child's `close` — the only event that says the process and the pipes this run inherited are finished. An assigned exit status is not that boundary, so none of them reads one: a cancelled command still captures what its child wrote on the way out, and still escalates SIGTERM to SIGKILL exactly as before. Readiness races are interpreted inline, in the same synchronous run as the `connect`, `listen` or `spawn` they watch, because a spawned race attaches its arms a turn later and the source can settle in that turn. The foreground reaper names its exit handler and releases it in the one funnel every settlement passes through. `nodeResponseChannel` becomes an `Operation`, so both of its handlers belong to the request task. The form responder keeps its request observed until that request's own `close`, so the peer's hang-up reaches a listener instead of the process. Web server teardown detaches every accepted socket's close handler before it suspends, then ends request tasks before destroying their connections. The workflow and CLI HTTP fixtures read request bodies in tasks of the server's own scope. Every distinct manual owner is cancelled while its listeners are live. Each case measures the baseline the runtime was already holding, reads the vector once with the owner running, halts it before the event that would have settled it, reads the vector again before replaying anything, then replays and proves no capture, callback or result moved. Where a source is a child process or accepted socket an operation does not hand out, a package-private observation seam carries it; no public package contract widened. This is a lifecycle change. Event ordering, output aggregation, response bytes, failure precedence, process signalling and every public result are unchanged.
1 parent 42732dd commit 535f8bf

56 files changed

Lines changed: 4621 additions & 429 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.oxlintrc.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
"local/no-sync-filesystem": "error",
2020
"local/no-yield-in-finally": "error",
2121
"local/prefer-effection-operation": "error",
22-
"local/prefer-effection-result": "error"
22+
"local/prefer-effection-result": "error",
23+
"local/require-scope-bound-event-registration": "error"
2324
},
2425

2526
"overrides": [

‎AGENTS.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,28 @@ the corpus or enabling in-shard concurrency is not an answer to a miss.
447447
structural values for composition data and a contextual Api for operations.
448448
Security enforcement, durable identity, and reconciliation never trust
449449
replaceable context state.
450+
16. An event listener an Effection operation installs has that operation's
451+
lifetime. Wait for one event with `once()` from `@effectionx/node/events`,
452+
never `emitter.once()` or `addEventListener(..., { once: true })`: cleanup
453+
that waits for the event is no cleanup for a wait that is cancelled. A
454+
longer subscription binds a stable handler and removes that same handler,
455+
from that same receiver and event, in the owner's own teardown — `.off()`
456+
for Node, `.removeEventListener()` with the matching capture mode for the
457+
DOM. Removal is synchronous, so it belongs in a `finally` around the
458+
subscription, an `ensure()` that **completed before** the subscription, or
459+
the cleanup an `action()` returns; where teardown must wait on the event
460+
itself, keep the handler through the wait and remove it in a synchronous
461+
`finally` inside that same `ensure()`. `yield* ensure(...)` is itself a
462+
suspension: an owner halted while it registers unwinds with no cleanup on
463+
it at all, so an `ensure()` yielded *after* the subscription has not
464+
established anything — nor may a native resource be created before the
465+
cleanup that releases it, and only the resource's own closing event proves
466+
it is finished, never an assigned exit status. The listener ordering is
467+
enforced by the `local/require-scope-bound-event-registration` Oxlint rule
468+
(`scripts/oxlint-rules/`), which does not autofix: which owner, which
469+
handler and which order are lifecycle decisions. The resource half — a
470+
child spawned before the cleanup that reaps it — is not something that rule
471+
can see, and is held by each owner's focused lifecycle regression instead.
450472

451473
## Writing Guide
452474

‎architecture.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2422,6 +2422,16 @@ hidden inside library objects that accumulate. One exception: metadata an
24222422
author declares at module evaluation, about a value the author owns, may live
24232423
on that value.
24242424

2425+
A callback registered with something outside the process — a listener on a
2426+
socket, a child process, a stream or a DOM target — is state of exactly this
2427+
kind, and the source it is attached to knows nothing about the operation that
2428+
attached it. The event arriving is not teardown: a cancelled wait is precisely
2429+
the case where it never arrives, and a losing race arm is one that will never
2430+
be told. So the owner detaches its handlers, on completion, failure, halt and
2431+
race loss alike, before it is considered closed, and an event delivered after
2432+
that reaches nothing and changes nothing. `local/require-scope-bound-event-registration`
2433+
holds source to it.
2434+
24252435
A Repository selection is composition data and is therefore replaceable: a
24262436
document may bind one, render one, hand one to a child, and construct one that
24272437
looks exactly like it. Nothing a repository provider does is authorized by the

‎deno.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
"@effectionx/fetch": "npm:@effectionx/fetch@0.2.1",
3434
"@effectionx/fs": "npm:@effectionx/fs@0.3.0",
3535
"@effectionx/middleware": "npm:@effectionx/middleware@0.1.1",
36-
"@effectionx/node": "npm:@effectionx/node@0.2.4",
36+
"@effectionx/node": "npm:@effectionx/node@0.2.5",
3737
"@effectionx/process": "npm:@effectionx/process@0.8.1",
3838
"@effectionx/scope-eval": "npm:@effectionx/scope-eval@0.1.3",
3939
"@effectionx/stream-helpers": "npm:@effectionx/stream-helpers@0.8.3",

‎deno.lock‎

Lines changed: 15 additions & 9 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
"@effectionx/fetch": "0.2.1",
3131
"@effectionx/fs": "0.3.0",
3232
"@effectionx/middleware": "0.1.1",
33-
"@effectionx/node": "0.2.4",
33+
"@effectionx/node": "0.2.5",
3434
"@effectionx/process": "0.8.1",
3535
"@effectionx/scope-eval": "0.1.3",
3636
"@effectionx/stream-helpers": "0.8.3",

‎packages/acp/tests/adapter-protocol.test.ts‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,18 @@
1818
*/
1919
import { describe, it } from "@executablemd/test-support/bdd";
2020
import { expect } from "@executablemd/test-support/expect";
21-
import { all, ensure, resource, spawn as effectionSpawn, until, withResolvers } from "effection";
21+
import {
22+
all,
23+
ensure,
24+
resource,
25+
scoped,
26+
spawn as effectionSpawn,
27+
until,
28+
withResolvers,
29+
} from "effection";
2230
import type { Operation } from "effection";
2331
import { rm } from "@effectionx/fs";
32+
import { Buffer } from "node:buffer";
2433
import { spawn } from "node:child_process";
2534
import type { ChildProcess } from "node:child_process";
2635
import { mkdtemp } from "node:fs/promises";
@@ -36,6 +45,8 @@ const FAKE_CLAUDE = join(FIXTURES, "fake-claude-cli.cjs");
3645

3746
/** One ACP conversation with a spawned adapter. */
3847
interface Adapter {
48+
/** The adapter process, so a case can say what the resource still observes. */
49+
readonly child: ChildProcess;
3950
request(method: string, params: unknown): Operation<Record<string, unknown>>;
4051
/**
4152
* Every `session/update` notification the adapter sent, in arrival order.
@@ -75,7 +86,7 @@ function useAdapter(provider: string, environment: Record<string, string>): Oper
7586
let next = 1;
7687
let buffer = "";
7788

78-
child.stdout?.on("data", (chunk: Buffer) => {
89+
const onStdout = (chunk: Buffer): void => {
7990
buffer += chunk.toString("utf8");
8091
let index = buffer.indexOf("\n");
8192
while (index >= 0) {
@@ -110,9 +121,20 @@ function useAdapter(provider: string, environment: Record<string, string>): Oper
110121
child.stdin?.write(`${JSON.stringify({ jsonrpc: "2.0", id, result: {} })}\n`);
111122
}
112123
}
124+
};
125+
126+
// Detached before the kill above, because destructors unwind in reverse:
127+
// the adapter stops being read before the process it is reading is ended.
128+
// Established before the subscription, because `yield* ensure(...)` is
129+
// itself a suspension an owner can be halted at.
130+
yield* ensure(() => {
131+
child.stdout?.off("data", onStdout);
113132
});
114133

134+
child.stdout?.on("data", onStdout);
135+
115136
yield* provide({
137+
child,
116138
updates,
117139
*request(method: string, params: unknown): Operation<Record<string, unknown>> {
118140
const id = next++;
@@ -330,4 +352,33 @@ describe("Tier EA — the embedded adapters' prompt-response metadata", () => {
330352
expect(metaOf(first, "codex")).toEqual({ turnId: `turn:${sessionId}:1` });
331353
expect(metaOf(second, "codex")).toEqual({ turnId: `turn:${sessionId}:2` });
332354
});
355+
/**
356+
* The adapter resource reads one child's stdout for as long as it holds it.
357+
* The count is read after the resource has been torn down and before the
358+
* event is replayed, because a handler removed by its own event would leave
359+
* the same count behind as one the resource released.
360+
*/
361+
it("releases the adapter's output handler with the resource", function* () {
362+
let child: ChildProcess | undefined;
363+
let live = 0;
364+
let before = 0;
365+
366+
yield* scoped(function* () {
367+
const adapter = yield* useAdapter("claude", {});
368+
child = adapter.child;
369+
before = 0;
370+
live = adapter.child.stdout?.listenerCount("data") ?? 0;
371+
});
372+
373+
if (!child) {
374+
throw new Error("the adapter never started");
375+
}
376+
377+
expect(live).toBeGreaterThanOrEqual(before + 1);
378+
expect(child.stdout?.listenerCount("data") ?? 0).toBe(live - 1);
379+
380+
child.stdout?.emit("data", Buffer.from("after the adapter was torn down"));
381+
382+
expect(child.stdout?.listenerCount("data") ?? 0).toBe(live - 1);
383+
});
333384
});

‎packages/acp/tests/fixtures/claude-native-launch-proof.ts‎

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -354,30 +354,52 @@ function runChild(
354354
options.live.delete(running.pid);
355355
});
356356

357-
child = spawnChild(command, args, { cwd: options.cwd, stdio: ["pipe", "pipe", "pipe"] });
358-
if (child.pid) {
359-
options.live.add(child.pid);
357+
const started = spawnChild(command, args, {
358+
cwd: options.cwd,
359+
stdio: ["pipe", "pipe", "pipe"],
360+
});
361+
child = started;
362+
if (started.pid) {
363+
options.live.add(started.pid);
360364
}
361365
let stdout = "";
362366
let stderr = "";
363-
child.stdout?.on("data", (chunk: Buffer) => {
367+
368+
const onStdout = (chunk: Buffer): void => {
364369
stdout += chunk.toString();
365-
});
366-
child.stderr?.on("data", (chunk: Buffer) => {
370+
};
371+
const onStderr = (chunk: Buffer): void => {
367372
stderr += chunk.toString();
368-
});
369-
child.once("error", (error: Error) => failed.reject(error));
370-
child.once("close", (code: number | null, signal: string | null) => {
371-
if (child?.pid) {
372-
options.live.delete(child.pid);
373+
};
374+
const onError = (error: Error): void => failed.reject(error);
375+
const onClose = (code: number | null, signal: string | null): void => {
376+
if (started.pid) {
377+
options.live.delete(started.pid);
373378
}
374379
settled.resolve({ code, signal, stdout, stderr });
380+
};
381+
382+
// Registered after the cleanup above and so torn down before it: the child
383+
// stops being observed before it is signalled, and a race this arm loses
384+
// leaves nothing attached to a process somebody else is still reading.
385+
// Established before the subscriptions, because `yield* ensure(...)` is
386+
// itself a suspension an owner can be halted at.
387+
yield* ensure(() => {
388+
started.stdout?.off("data", onStdout);
389+
started.stderr?.off("data", onStderr);
390+
started.off("error", onError);
391+
started.off("close", onClose);
375392
});
376393

394+
started.stdout?.on("data", onStdout);
395+
started.stderr?.on("data", onStderr);
396+
started.on("error", onError);
397+
started.on("close", onClose);
398+
377399
if (options.input !== undefined) {
378-
child.stdin?.write(options.input);
400+
started.stdin?.write(options.input);
379401
}
380-
child.stdin?.end();
402+
started.stdin?.end();
381403

382404
return yield* race([settled.operation, failed.operation]);
383405
})();
@@ -502,16 +524,17 @@ function ptyRun<T>(
502524
options.live.delete(running.pid);
503525
});
504526

505-
child = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], {
527+
const started = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], {
506528
cwd: options.cwd,
507529
env: options.env,
508530
stdio: ["pipe", "pipe", "pipe"],
509531
});
510-
if (child.pid) {
511-
options.live.add(child.pid);
532+
child = started;
533+
if (started.pid) {
534+
options.live.add(started.pid);
512535
}
513536

514-
const react = (chunk: Buffer) => {
537+
const react = (chunk: Buffer): void => {
515538
text += chunk.toString();
516539
const waiter = pending;
517540
if (waiter && waiter.predicate(text.slice(consumed))) {
@@ -520,17 +543,31 @@ function ptyRun<T>(
520543
waiter.resolve("");
521544
}
522545
};
523-
child.stdout?.on("data", react);
524-
child.stderr?.on("data", react);
525-
child.once("error", (error: Error) => failed.reject(error));
526-
child.once("close", (status: number | null) => {
546+
const onError = (error: Error): void => failed.reject(error);
547+
const onClose = (status: number | null): void => {
527548
code = status ?? -1;
528-
if (child?.pid) {
529-
options.live.delete(child.pid);
549+
if (started.pid) {
550+
options.live.delete(started.pid);
530551
}
531552
settled.resolve();
553+
};
554+
555+
// Registered after the interrupt cleanup above and so torn down before it:
556+
// the terminal stops being read before the process holding it is signalled.
557+
// Established before the subscriptions, because `yield* ensure(...)` is
558+
// itself a suspension an owner can be halted at.
559+
yield* ensure(() => {
560+
started.stdout?.off("data", react);
561+
started.stderr?.off("data", react);
562+
started.off("error", onError);
563+
started.off("close", onClose);
532564
});
533565

566+
started.stdout?.on("data", react);
567+
started.stderr?.on("data", react);
568+
started.on("error", onError);
569+
started.on("close", onClose);
570+
534571
const write = (bytes: string) => {
535572
// Everything already on screen belongs to the surface being answered, so
536573
// the next wait reads only what this write provoked.

0 commit comments

Comments
 (0)