diff --git a/.oxlintrc.json b/.oxlintrc.json index 2d9c6dc91..e92294a55 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -19,7 +19,8 @@ "local/no-sync-filesystem": "error", "local/no-yield-in-finally": "error", "local/prefer-effection-operation": "error", - "local/prefer-effection-result": "error" + "local/prefer-effection-result": "error", + "local/require-scope-bound-event-registration": "error" }, "overrides": [ diff --git a/AGENTS.md b/AGENTS.md index 5d324ab6c..efdc6f047 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -447,6 +447,28 @@ the corpus or enabling in-shard concurrency is not an answer to a miss. structural values for composition data and a contextual Api for operations. Security enforcement, durable identity, and reconciliation never trust replaceable context state. +16. An event listener an Effection operation installs has that operation's + lifetime. Wait for one event with `once()` from `@effectionx/node/events`, + never `emitter.once()` or `addEventListener(..., { once: true })`: cleanup + that waits for the event is no cleanup for a wait that is cancelled. A + longer subscription binds a stable handler and removes that same handler, + from that same receiver and event, in the owner's own teardown — `.off()` + for Node, `.removeEventListener()` with the matching capture mode for the + DOM. Removal is synchronous, so it belongs in a `finally` around the + subscription, an `ensure()` that **completed before** the subscription, or + the cleanup an `action()` returns; where teardown must wait on the event + itself, keep the handler through the wait and remove it in a synchronous + `finally` inside that same `ensure()`. `yield* ensure(...)` is itself a + suspension: an owner halted while it registers unwinds with no cleanup on + it at all, so an `ensure()` yielded *after* the subscription has not + established anything — nor may a native resource be created before the + cleanup that releases it, and only the resource's own closing event proves + it is finished, never an assigned exit status. The listener ordering is + enforced by the `local/require-scope-bound-event-registration` Oxlint rule + (`scripts/oxlint-rules/`), which does not autofix: which owner, which + handler and which order are lifecycle decisions. The resource half — a + child spawned before the cleanup that reaps it — is not something that rule + can see, and is held by each owner's focused lifecycle regression instead. ## Writing Guide diff --git a/architecture.md b/architecture.md index 8d50192f0..b227d8814 100644 --- a/architecture.md +++ b/architecture.md @@ -2422,6 +2422,16 @@ hidden inside library objects that accumulate. One exception: metadata an author declares at module evaluation, about a value the author owns, may live on that value. +A callback registered with something outside the process — a listener on a +socket, a child process, a stream or a DOM target — is state of exactly this +kind, and the source it is attached to knows nothing about the operation that +attached it. The event arriving is not teardown: a cancelled wait is precisely +the case where it never arrives, and a losing race arm is one that will never +be told. So the owner detaches its handlers, on completion, failure, halt and +race loss alike, before it is considered closed, and an event delivered after +that reaches nothing and changes nothing. `local/require-scope-bound-event-registration` +holds source to it. + A Repository selection is composition data and is therefore replaceable: a document may bind one, render one, hand one to a child, and construct one that looks exactly like it. Nothing a repository provider does is authorized by the diff --git a/deno.json b/deno.json index 7b0e6ff0b..d3e68f866 100644 --- a/deno.json +++ b/deno.json @@ -33,7 +33,7 @@ "@effectionx/fetch": "npm:@effectionx/fetch@0.2.1", "@effectionx/fs": "npm:@effectionx/fs@0.3.0", "@effectionx/middleware": "npm:@effectionx/middleware@0.1.1", - "@effectionx/node": "npm:@effectionx/node@0.2.4", + "@effectionx/node": "npm:@effectionx/node@0.2.5", "@effectionx/process": "npm:@effectionx/process@0.8.1", "@effectionx/scope-eval": "npm:@effectionx/scope-eval@0.1.3", "@effectionx/stream-helpers": "npm:@effectionx/stream-helpers@0.8.3", diff --git a/deno.lock b/deno.lock index a0c581cff..7772922ea 100644 --- a/deno.lock +++ b/deno.lock @@ -48,7 +48,7 @@ "npm:@effectionx/fetch@0.2.1": "0.2.1_effection@4.1.0", "npm:@effectionx/fs@0.3.0": "0.3.0_effection@4.1.0", "npm:@effectionx/middleware@0.1.1": "0.1.1", - "npm:@effectionx/node@0.2.4": "0.2.4_effection@4.1.0", + "npm:@effectionx/node@0.2.5": "0.2.5_effection@4.1.0", "npm:@effectionx/process@0.8.1": "0.8.1_effection@4.1.0", "npm:@effectionx/scope-eval@0.1.3": "0.1.3_effection@4.1.0", "npm:@effectionx/stream-helpers@0.8.3": "0.8.3_effection@4.1.0", @@ -548,11 +548,17 @@ "effection" ] }, + "@effectionx/node@0.2.5_effection@4.1.0": { + "integrity": "sha512-hL8mROda8Lx375MVS+Ubu86+yMht/I0wOZG5VR6Pel0XUA5ReObQDYvNS6ocW0cNFKnEmvlVLWGWbcjJ+VkVhA==", + "dependencies": [ + "effection" + ] + }, "@effectionx/process@0.8.1_effection@4.1.0": { "integrity": "sha512-xyXlFja0Ill80lQ3IYfksXtJkqVmWuUOogRn/qlHWCAGlZj+MGGF8gOFbyzk/3Kx4pj14riVGgF/cyT5XCzqDw==", "dependencies": [ "@effectionx/context-api", - "@effectionx/node", + "@effectionx/node@0.2.4_effection@4.1.0", "@effectionx/scope-eval", "cross-spawn", "ctrlc-windows", @@ -3966,7 +3972,7 @@ "npm:@effectionx/fetch@0.2.1", "npm:@effectionx/fs@0.3.0", "npm:@effectionx/middleware@0.1.1", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/process@0.8.1", "npm:@effectionx/scope-eval@0.1.3", "npm:@effectionx/stream-helpers@0.8.3", @@ -3997,7 +4003,7 @@ "npm:@effectionx/fetch@0.2.1", "npm:@effectionx/fs@0.3.0", "npm:@effectionx/middleware@0.1.1", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/process@0.8.1", "npm:@effectionx/scope-eval@0.1.3", "npm:@effectionx/stream-helpers@0.8.3", @@ -4081,7 +4087,7 @@ "npm:@effectionx/fetch@0.2.1", "npm:@effectionx/fs@0.3.0", "npm:@effectionx/middleware@0.1.1", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/process@0.8.1", "npm:@effectionx/scope-eval@0.1.3", "npm:@effectionx/stream-helpers@0.8.3", @@ -4117,7 +4123,7 @@ "npm:@effectionx/context-api@0.6.0", "npm:@effectionx/fetch@0.2.1", "npm:@effectionx/fs@0.3.0", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/process@0.8.1", "npm:effection@4.1.0" ] @@ -4126,7 +4132,7 @@ "packages/test-agent": { "dependencies": [ "npm:@agentclientprotocol/sdk@1.3.0", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/scope-eval@0.1.3", "npm:@effectionx/stream-helpers@0.8.3", "npm:acorn@^8.16.0", @@ -4136,7 +4142,7 @@ "packageJson": { "dependencies": [ "npm:@agentclientprotocol/sdk@1.3.0", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@effectionx/scope-eval@0.1.3", "npm:@effectionx/stream-helpers@0.8.3", "npm:acorn@^8.16.0", @@ -4185,7 +4191,7 @@ "packageJson": { "dependencies": [ "npm:@effectionx/fs@0.3.0", - "npm:@effectionx/node@0.2.4", + "npm:@effectionx/node@0.2.5", "npm:@fontsource/montserrat@5.3.0", "npm:@fontsource/space-mono@5.3.0", "npm:@rjsf/core@6.7.1", diff --git a/package.json b/package.json index cd4385566..d17b1b1fe 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "@effectionx/fetch": "0.2.1", "@effectionx/fs": "0.3.0", "@effectionx/middleware": "0.1.1", - "@effectionx/node": "0.2.4", + "@effectionx/node": "0.2.5", "@effectionx/process": "0.8.1", "@effectionx/scope-eval": "0.1.3", "@effectionx/stream-helpers": "0.8.3", diff --git a/packages/acp/tests/adapter-protocol.test.ts b/packages/acp/tests/adapter-protocol.test.ts index 461e539ee..e5cb3ce02 100644 --- a/packages/acp/tests/adapter-protocol.test.ts +++ b/packages/acp/tests/adapter-protocol.test.ts @@ -18,9 +18,18 @@ */ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { all, ensure, resource, spawn as effectionSpawn, until, withResolvers } from "effection"; +import { + all, + ensure, + resource, + scoped, + spawn as effectionSpawn, + until, + withResolvers, +} from "effection"; import type { Operation } from "effection"; import { rm } from "@effectionx/fs"; +import { Buffer } from "node:buffer"; import { spawn } from "node:child_process"; import type { ChildProcess } from "node:child_process"; import { mkdtemp } from "node:fs/promises"; @@ -36,6 +45,8 @@ const FAKE_CLAUDE = join(FIXTURES, "fake-claude-cli.cjs"); /** One ACP conversation with a spawned adapter. */ interface Adapter { + /** The adapter process, so a case can say what the resource still observes. */ + readonly child: ChildProcess; request(method: string, params: unknown): Operation>; /** * Every `session/update` notification the adapter sent, in arrival order. @@ -75,7 +86,7 @@ function useAdapter(provider: string, environment: Record): Oper let next = 1; let buffer = ""; - child.stdout?.on("data", (chunk: Buffer) => { + const onStdout = (chunk: Buffer): void => { buffer += chunk.toString("utf8"); let index = buffer.indexOf("\n"); while (index >= 0) { @@ -110,9 +121,20 @@ function useAdapter(provider: string, environment: Record): Oper child.stdin?.write(`${JSON.stringify({ jsonrpc: "2.0", id, result: {} })}\n`); } } + }; + + // Detached before the kill above, because destructors unwind in reverse: + // the adapter stops being read before the process it is reading is ended. + // Established before the subscription, because `yield* ensure(...)` is + // itself a suspension an owner can be halted at. + yield* ensure(() => { + child.stdout?.off("data", onStdout); }); + child.stdout?.on("data", onStdout); + yield* provide({ + child, updates, *request(method: string, params: unknown): Operation> { const id = next++; @@ -330,4 +352,33 @@ describe("Tier EA — the embedded adapters' prompt-response metadata", () => { expect(metaOf(first, "codex")).toEqual({ turnId: `turn:${sessionId}:1` }); expect(metaOf(second, "codex")).toEqual({ turnId: `turn:${sessionId}:2` }); }); + /** + * The adapter resource reads one child's stdout for as long as it holds it. + * The count is read after the resource has been torn down and before the + * event is replayed, because a handler removed by its own event would leave + * the same count behind as one the resource released. + */ + it("releases the adapter's output handler with the resource", function* () { + let child: ChildProcess | undefined; + let live = 0; + let before = 0; + + yield* scoped(function* () { + const adapter = yield* useAdapter("claude", {}); + child = adapter.child; + before = 0; + live = adapter.child.stdout?.listenerCount("data") ?? 0; + }); + + if (!child) { + throw new Error("the adapter never started"); + } + + expect(live).toBeGreaterThanOrEqual(before + 1); + expect(child.stdout?.listenerCount("data") ?? 0).toBe(live - 1); + + child.stdout?.emit("data", Buffer.from("after the adapter was torn down")); + + expect(child.stdout?.listenerCount("data") ?? 0).toBe(live - 1); + }); }); diff --git a/packages/acp/tests/fixtures/claude-native-launch-proof.ts b/packages/acp/tests/fixtures/claude-native-launch-proof.ts index 051835220..717991d94 100644 --- a/packages/acp/tests/fixtures/claude-native-launch-proof.ts +++ b/packages/acp/tests/fixtures/claude-native-launch-proof.ts @@ -354,30 +354,52 @@ function runChild( options.live.delete(running.pid); }); - child = spawnChild(command, args, { cwd: options.cwd, stdio: ["pipe", "pipe", "pipe"] }); - if (child.pid) { - options.live.add(child.pid); + const started = spawnChild(command, args, { + cwd: options.cwd, + stdio: ["pipe", "pipe", "pipe"], + }); + child = started; + if (started.pid) { + options.live.add(started.pid); } let stdout = ""; let stderr = ""; - child.stdout?.on("data", (chunk: Buffer) => { + + const onStdout = (chunk: Buffer): void => { stdout += chunk.toString(); - }); - child.stderr?.on("data", (chunk: Buffer) => { + }; + const onStderr = (chunk: Buffer): void => { stderr += chunk.toString(); - }); - child.once("error", (error: Error) => failed.reject(error)); - child.once("close", (code: number | null, signal: string | null) => { - if (child?.pid) { - options.live.delete(child.pid); + }; + const onError = (error: Error): void => failed.reject(error); + const onClose = (code: number | null, signal: string | null): void => { + if (started.pid) { + options.live.delete(started.pid); } settled.resolve({ code, signal, stdout, stderr }); + }; + + // Registered after the cleanup above and so torn down before it: the child + // stops being observed before it is signalled, and a race this arm loses + // leaves nothing attached to a process somebody else is still reading. + // Established before the subscriptions, because `yield* ensure(...)` is + // itself a suspension an owner can be halted at. + yield* ensure(() => { + started.stdout?.off("data", onStdout); + started.stderr?.off("data", onStderr); + started.off("error", onError); + started.off("close", onClose); }); + started.stdout?.on("data", onStdout); + started.stderr?.on("data", onStderr); + started.on("error", onError); + started.on("close", onClose); + if (options.input !== undefined) { - child.stdin?.write(options.input); + started.stdin?.write(options.input); } - child.stdin?.end(); + started.stdin?.end(); return yield* race([settled.operation, failed.operation]); })(); @@ -502,16 +524,17 @@ function ptyRun( options.live.delete(running.pid); }); - child = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], { + const started = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], { cwd: options.cwd, env: options.env, stdio: ["pipe", "pipe", "pipe"], }); - if (child.pid) { - options.live.add(child.pid); + child = started; + if (started.pid) { + options.live.add(started.pid); } - const react = (chunk: Buffer) => { + const react = (chunk: Buffer): void => { text += chunk.toString(); const waiter = pending; if (waiter && waiter.predicate(text.slice(consumed))) { @@ -520,17 +543,31 @@ function ptyRun( waiter.resolve(""); } }; - child.stdout?.on("data", react); - child.stderr?.on("data", react); - child.once("error", (error: Error) => failed.reject(error)); - child.once("close", (status: number | null) => { + const onError = (error: Error): void => failed.reject(error); + const onClose = (status: number | null): void => { code = status ?? -1; - if (child?.pid) { - options.live.delete(child.pid); + if (started.pid) { + options.live.delete(started.pid); } settled.resolve(); + }; + + // Registered after the interrupt cleanup above and so torn down before it: + // the terminal stops being read before the process holding it is signalled. + // Established before the subscriptions, because `yield* ensure(...)` is + // itself a suspension an owner can be halted at. + yield* ensure(() => { + started.stdout?.off("data", react); + started.stderr?.off("data", react); + started.off("error", onError); + started.off("close", onClose); }); + started.stdout?.on("data", react); + started.stderr?.on("data", react); + started.on("error", onError); + started.on("close", onClose); + const write = (bytes: string) => { // Everything already on screen belongs to the surface being answered, so // the next wait reads only what this write provoked. diff --git a/packages/acp/tests/fixtures/claude-native-to-acp-proof.ts b/packages/acp/tests/fixtures/claude-native-to-acp-proof.ts index 1012dd822..b218ecc1a 100644 --- a/packages/acp/tests/fixtures/claude-native-to-acp-proof.ts +++ b/packages/acp/tests/fixtures/claude-native-to-acp-proof.ts @@ -371,34 +371,53 @@ function runChild( options.live.delete(running.pid); }); - child = spawnChild(command, args, { + const started = spawnChild(command, args, { cwd: options.cwd, stdio: ["pipe", "pipe", "pipe"], ...(options.env === undefined ? {} : { env: options.env }), }); - if (child.pid) { - options.live.add(child.pid); + child = started; + if (started.pid) { + options.live.add(started.pid); } let stdout = ""; let stderr = ""; - child.stdout?.on("data", (chunk: Buffer) => { + + const onStdout = (chunk: Buffer): void => { stdout += chunk.toString(); - }); - child.stderr?.on("data", (chunk: Buffer) => { + }; + const onStderr = (chunk: Buffer): void => { stderr += chunk.toString(); - }); - child.once("error", (error: Error) => failed.reject(error)); - child.once("close", (code: number | null, signal: string | null) => { - if (child?.pid) { - options.live.delete(child.pid); + }; + const onError = (error: Error): void => failed.reject(error); + const onClose = (code: number | null, signal: string | null): void => { + if (started.pid) { + options.live.delete(started.pid); } settled.resolve({ code, signal, stdout, stderr }); + }; + + // Registered after the cleanup above and so torn down before it: the child + // stops being observed before it is signalled, and a race this arm loses + // leaves nothing attached to a process somebody else is still reading. + // Established before the subscriptions, because `yield* ensure(...)` is + // itself a suspension an owner can be halted at. + yield* ensure(() => { + started.stdout?.off("data", onStdout); + started.stderr?.off("data", onStderr); + started.off("error", onError); + started.off("close", onClose); }); + started.stdout?.on("data", onStdout); + started.stderr?.on("data", onStderr); + started.on("error", onError); + started.on("close", onClose); + if (options.input !== undefined) { - child.stdin?.write(options.input); + started.stdin?.write(options.input); } - child.stdin?.end(); + started.stdin?.end(); return yield* race([settled.operation, failed.operation]); })(); @@ -523,16 +542,17 @@ function ptyRun( options.live.delete(running.pid); }); - child = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], { + const started = spawnChild("/usr/bin/script", ["-q", "/dev/null", command, ...args], { cwd: options.cwd, env: options.env, stdio: ["pipe", "pipe", "pipe"], }); - if (child.pid) { - options.live.add(child.pid); + child = started; + if (started.pid) { + options.live.add(started.pid); } - const react = (chunk: Buffer) => { + const react = (chunk: Buffer): void => { text += chunk.toString(); const waiter = pending; if (waiter && waiter.predicate(text.slice(consumed))) { @@ -541,17 +561,31 @@ function ptyRun( waiter.resolve(""); } }; - child.stdout?.on("data", react); - child.stderr?.on("data", react); - child.once("error", (error: Error) => failed.reject(error)); - child.once("close", (status: number | null) => { + const onError = (error: Error): void => failed.reject(error); + const onClose = (status: number | null): void => { code = status ?? -1; - if (child?.pid) { - options.live.delete(child.pid); + if (started.pid) { + options.live.delete(started.pid); } settled.resolve(); + }; + + // Registered after the interrupt cleanup above and so torn down before it: + // the terminal stops being read before the process holding it is signalled. + // Established before the subscriptions, because `yield* ensure(...)` is + // itself a suspension an owner can be halted at. + yield* ensure(() => { + started.stdout?.off("data", react); + started.stderr?.off("data", react); + started.off("error", onError); + started.off("close", onClose); }); + started.stdout?.on("data", react); + started.stderr?.on("data", react); + started.on("error", onError); + started.on("close", onClose); + const write = (bytes: string) => { // Everything already on screen belongs to the surface being answered, so // the next wait reads only what this write provoked. diff --git a/packages/cli/tests/fetch-cli.test.ts b/packages/cli/tests/fetch-cli.test.ts index afdf68ded..879df568e 100644 --- a/packages/cli/tests/fetch-cli.test.ts +++ b/packages/cli/tests/fetch-cli.test.ts @@ -48,7 +48,23 @@ function useLoopback(): Operation { }); const listening = withResolvers(); - server.on("error", (error: Error) => listening.reject(error)); + // Removed with the resource rather than after the first error: a listening + // server outlives its bind, and a handler left behind would still be + // holding a rejected resolver when the next test binds its own. + // + // Established before the handler exists, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all, so nothing may be attached until it has completed. + let onError: ((error: Error) => void) | undefined; + + yield* ensure(() => { + if (onError) { + server.off("error", onError); + } + }); + + onError = (error: Error) => listening.reject(error); + server.on("error", onError); server.listen(0, "127.0.0.1", () => listening.resolve()); yield* listening.operation; diff --git a/packages/cli/tests/stdin-cli.test.ts b/packages/cli/tests/stdin-cli.test.ts index 4d9d27fc5..b1e7f630f 100644 --- a/packages/cli/tests/stdin-cli.test.ts +++ b/packages/cli/tests/stdin-cli.test.ts @@ -10,7 +10,10 @@ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { runCli } from "@executablemd/test-support/launch"; +import { runCli, runShell } from "@executablemd/test-support/launch"; +import { when } from "@effectionx/converge"; +import { Buffer } from "node:buffer"; +import type { ChildProcess } from "node:child_process"; import { createContext, Err, @@ -132,6 +135,17 @@ const CONFLICT_FIXTURE = { ].join("\n"), }; +/** What the shared launcher attaches to a child it owns, and to its pipes. */ +function attached(child: ChildProcess): number[] { + return [ + child.listenerCount("error"), + child.listenerCount("close"), + child.stdout?.listenerCount("data") ?? 0, + child.stderr?.listenerCount("data") ?? 0, + child.stdin?.listenerCount("error") ?? 0, + ]; +} + const STDIN_EFFECT = 'x\n\nSTDIN_MARKER\n'; describe( @@ -547,11 +561,25 @@ describe( "", ].join("\n"); + // The child this launcher owns, what its streams carried before the + // launcher attached anything, and what the run has captured from it. + let child: ChildProcess | undefined; + let before: number[] = []; + let captured: () => { stdout: string; stderr: string } = () => ({ + stdout: "", + stderr: "", + }); + const run = yield* spawn(function* () { yield* runCli(["run", "-", "--raw"], { cwd: dir, stdin: document, timeout: 60_000, + observeChild: (started, reader) => { + child = started; + before = attached(started); + captured = reader; + }, }).join(); }); @@ -564,10 +592,42 @@ describe( const [escaped, owned] = yield* waitForIds(idsPath); yield* ensure(() => endEscapedGroup(escaped)); + // Synchronized on the command having run, so every handler is attached + // and the child is still alive when the run is cancelled underneath it. + if (!child) { + throw new Error("the launcher never owned a child"); + } + const live = attached(child); + const seen = captured(); + + // At least what this owner attached, not exactly it: a runtime may hold + // handlers of its own on a child and its pipes, so the release below is + // measured against what was live rather than against the baseline. + live.forEach((count, index) => { + expect(count).toBeGreaterThanOrEqual(before[index] + 1); + }); + const at = Date.now(); yield* run.halt(); const elapsed = Date.now() - at; + // Cancelled while all five were live, and back to what the runtime had + // before the launcher attached anything. Read before the events are + // replayed, because a handler removed by its own event would leave the + // same counts behind as one the launcher released. + const released = live.map((count) => count - 1); + + expect(attached(child)).toEqual(released); + + child.stdout?.emit("data", Buffer.from("after the run was cancelled")); + child.stderr?.emit("data", Buffer.from("after the run was cancelled")); + child.emit("close", 0, null); + + // And nothing is still accumulating: the capture the run abandoned did + // not grow. + expect(captured()).toEqual(seen); + expect(attached(child)).toEqual(released); + // A signal that has been sent is not a process that is gone: teardown // returning as soon as it signalled would come back at once. Nor may it // wait forever on a child that is never going to answer SIGTERM — the @@ -580,6 +640,59 @@ describe( }); }); + /** + * Teardown's boundary is the child's `close`, not the signal it was sent + * and not an exit status the handle has recorded. This child traps the + * interrupt, writes on its way out, and only then leaves — so the marker + * can only be here if the capture was still attached while it stopped and + * the cancellation waited for the pipes to end. + */ + it("SI12b: what a cancelled child writes while it stops is still captured", function* () { + let child: ChildProcess | undefined; + let before: number[] = []; + let captured: () => { stdout: string; stderr: string } = () => ({ + stdout: "", + stderr: "", + }); + + const run = yield* spawn(function* () { + // A large trailing write, so what is asserted below is the whole of it + // rather than the first line to arrive. + yield* runShell("trap 'yes LATE | head -20000; exit 0' TERM; echo READY; sleep 20", { + stdin: "", + timeout: 60_000, + observeChild: (started, reader) => { + child = started; + before = attached(started); + captured = reader; + }, + }).join(); + }); + + // Synchronized on the child running, so the halt lands on a live process + // with every handler attached. + yield* when(function* () { + expect(captured().stdout).toContain("READY"); + }); + + if (!child) { + throw new Error("the launcher never owned a child"); + } + const live = attached(child); + + yield* run.halt(); + + live.forEach((count, index) => { + expect(count).toBeGreaterThanOrEqual(before[index] + 1); + }); + // All of it, written after the signal and before `close`: the capture was + // still attached while the child stopped, and teardown did not return + // until the pipes had ended. + expect(captured().stdout.split("LATE").length - 1).toBe(20_000); + // And released only once that had happened. + expect(attached(child)).toEqual(live.map((count) => count - 1)); + }); + it("SI9b: the stream adapter's listeners belong to the read's own scope", function* () { const stream = new PassThrough(); const read = yield* spawn(() => readInputStream(stream)); diff --git a/packages/cli/tests/stdout-delivery.test.ts b/packages/cli/tests/stdout-delivery.test.ts index b24b63b55..5c9183652 100644 --- a/packages/cli/tests/stdout-delivery.test.ts +++ b/packages/cli/tests/stdout-delivery.test.ts @@ -14,7 +14,7 @@ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { scoped, spawn, withResolvers } from "effection"; +import { ensure, scoped, spawn, withResolvers } from "effection"; import { EventEmitter } from "node:events"; import process from "node:process"; import { deliverWhole } from "../src/stdout-delivery.ts"; @@ -108,8 +108,13 @@ describe("Tier SDL — the listener lives for one delivery", () => { it("SDL2: the trailing event still finds the listener, which is gone after it", function* () { const sink = new RecordingSink(CALLBACK_THEN_EVENT); // A sentinel, so that detaching too early is this row's assertion rather - // than an unhandled `error` event thrown from a timer. - sink.on("error", () => {}); + // than an unhandled `error` event thrown from a timer. It belongs to the + // test, and comes off with it. + const sentinel = (): void => {}; + yield* ensure(() => { + sink.off("error", sentinel); + }); + sink.on("error", sentinel); const before = listeners(sink); const result = yield* deliverWhole("catalog", sink); @@ -170,7 +175,13 @@ describe("Tier SDL — the listener lives for one delivery", () => { it("SDL7: no finished delivery absorbs a later, unrelated failure", function* () { const sink = new RecordingSink(ACCEPTS); const seen: Error[] = []; - sink.on("error", (error: Error) => seen.push(error)); + const sentinel = (error: Error): void => { + seen.push(error); + }; + yield* ensure(() => { + sink.off("error", sentinel); + }); + sink.on("error", sentinel); yield* deliverWhole("catalog", sink); sink.emit("error", OTHER); diff --git a/packages/cli/tests/support/pull-request-server.ts b/packages/cli/tests/support/pull-request-server.ts index 86e8157e4..af3a3c2c2 100644 --- a/packages/cli/tests/support/pull-request-server.ts +++ b/packages/cli/tests/support/pull-request-server.ts @@ -9,7 +9,8 @@ */ import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; -import { ensure, type Operation, resource, until } from "effection"; +import { each, ensure, type Operation, resource, until, useScope } from "effection"; +import { fromReadable } from "@effectionx/node"; /** One request this server received, as an assertion reads it. */ export interface ServedRequest { @@ -63,9 +64,18 @@ export function usePullRequestServer(options: ServerOptions = {}): Operation { - incoming.resume(); - incoming.on("end", () => { + scope.run(function* () { + for (const _chunk of yield* each(fromReadable(incoming))) { + yield* each.next(); + } + const url = new URL(incoming.url ?? "/", "http://127.0.0.1"); const authorization = typeof incoming.headers["authorization"] === "string" diff --git a/packages/cli/tests/workflow-fetch.test.ts b/packages/cli/tests/workflow-fetch.test.ts index 1780035b1..cf4f34ec6 100644 --- a/packages/cli/tests/workflow-fetch.test.ts +++ b/packages/cli/tests/workflow-fetch.test.ts @@ -55,7 +55,23 @@ function useLoopback(): Operation { }); const listening = withResolvers(); - server.on("error", (error: Error) => listening.reject(error)); + // Removed with the resource rather than after the first error: a listening + // server outlives its bind, and a handler left behind would still be + // holding a rejected resolver when the next test binds its own. + // + // Established before the handler exists, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all, so nothing may be attached until it has completed. + let onError: ((error: Error) => void) | undefined; + + yield* ensure(() => { + if (onError) { + server.off("error", onError); + } + }); + + onError = (error: Error) => listening.reject(error); + server.on("error", onError); server.listen(0, "127.0.0.1", () => listening.resolve()); yield* listening.operation; diff --git a/packages/core/package.json b/packages/core/package.json index dd131db7c..8dadac530 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -13,7 +13,7 @@ "@effectionx/fetch": "0.2.1", "@effectionx/fs": "0.3.0", "@effectionx/middleware": "0.1.1", - "@effectionx/node": "0.2.4", + "@effectionx/node": "0.2.5", "@effectionx/process": "0.8.1", "@effectionx/scope-eval": "0.1.3", "@effectionx/stream-helpers": "0.8.3", diff --git a/packages/runtime/launcher.ts b/packages/runtime/launcher.ts index dc875fad0..2a11e374d 100644 --- a/packages/runtime/launcher.ts +++ b/packages/runtime/launcher.ts @@ -28,7 +28,8 @@ */ import { type Api, createApi } from "@effectionx/context-api"; -import { ensure, race, resource, scoped, until, withResolvers } from "effection"; +import { ensure, race, resource, scoped, until } from "effection"; +import { once } from "@effectionx/node/events"; import type { Operation } from "effection"; import { spawn as spawnChild } from "node:child_process"; import type { ChildProcess } from "node:child_process"; @@ -219,8 +220,6 @@ function runForeground(request: NativeLaunchRequest): Operation(); - const failed = withResolvers(); let child: ChildProcess | undefined; // Interrupt, then insist. A cancelled document may not continue — or @@ -233,31 +232,44 @@ function runForeground(request: NativeLaunchRequest): Operation failed.reject(error)); - child.once("exit", (code: number | null, signal: string | null) => { - const outcome: NativeLaunchOutcome = {}; - if (code !== null) { - outcome.exitCode = code; - } - if (signal !== null) { - outcome.signal = signal; - } - settled.resolve(outcome); - }); - - return yield* race([settled.operation, failed.operation]); + child = started; + + // Raced inline, in the same synchronous run as the spawn, so both arms are + // attached before the child can report anything — a spawned race attaches + // a turn later. Whichever loses is halted, which is what detaches it. + return yield* race([ + (function* (): Operation { + const [code, signal] = yield* once<[number | null, string | null]>(started, "exit"); + const outcome: NativeLaunchOutcome = {}; + if (code !== null) { + outcome.exitCode = code; + } + if (signal !== null) { + outcome.signal = signal; + } + return outcome; + })(), + (function* (): Operation { + const [error] = yield* once<[Error]>(started, "error"); + throw error; + })(), + ]); }); } /** * End one foreground child and wait for it to be gone. * + * Exported for `packages/runtime/tests/native-launcher.test.ts` and not from + * `mod.ts`: the listener this installs belongs to a bounded Promise, and the + * only way to observe that it is released on every settlement path is to hold + * the child. + * * Deliberately one promise rather than an Effection race: this runs while the * scope is already being dismantled, and the cheapest correct thing to do * there is to wait on the process's own events instead of starting more @@ -267,7 +279,7 @@ function runForeground(request: NativeLaunchRequest): Operation { +export function reap(child: ChildProcess): Promise { const pid = child.pid; if (pid === undefined || child.exitCode !== null || child.signalCode !== null) { return Promise.resolve(); @@ -290,6 +302,10 @@ function reap(child: ChildProcess): Promise { clearInterval(poll); clearTimeout(escalation); clearTimeout(deadline); + // The one funnel every settlement goes through — the exit event, the + // reachability poll, the escalation deadline, and the refusal that + // rejects — so the handler comes off however this ends. + child.off("exit", onExit); // Deno's `node:child_process` stops reporting a child's exit once a // signal that child ignored has been delivered, and holds the runtime // open on the handle it will now never settle. Dropping the reference is @@ -306,8 +322,9 @@ function reap(child: ChildProcess): Promise { } resolve(); }; + const onExit = (): void => done(); - child.once("exit", () => done()); + child.on("exit", onExit); // Reachability rather than the exit event, because that is the fact this // has to establish and the event is not dependable across runtimes here. const poll = setInterval(() => { diff --git a/packages/runtime/package.json b/packages/runtime/package.json index 97bf87c20..98d09a957 100644 --- a/packages/runtime/package.json +++ b/packages/runtime/package.json @@ -12,7 +12,7 @@ "@effectionx/context-api": "0.6.0", "@effectionx/fetch": "0.2.1", "@effectionx/fs": "0.3.0", - "@effectionx/node": "0.2.4", + "@effectionx/node": "0.2.5", "@effectionx/process": "0.8.1", "effection": "4.1.0" } diff --git a/packages/runtime/tests/fetch.test.ts b/packages/runtime/tests/fetch.test.ts index c0798f023..6227d4985 100644 --- a/packages/runtime/tests/fetch.test.ts +++ b/packages/runtime/tests/fetch.test.ts @@ -19,7 +19,7 @@ import { ensure, resource, scoped, sleep, spawn, withResolvers } from "effection import type { Operation } from "effection"; import { when } from "@effectionx/converge"; import { createServer } from "node:http"; -import type { IncomingMessage, ServerResponse } from "node:http"; +import type { IncomingMessage, Server, ServerResponse } from "node:http"; import { API, fetch } from "../apis.ts"; import type { RuntimeFetchResponse } from "../apis.ts"; import { Config } from "../config.ts"; @@ -27,6 +27,8 @@ import { Config } from "../config.ts"; interface Loopback { /** Where the server is listening. */ readonly origin: string; + /** The listener itself, so a case can say what it is still observing. */ + readonly server: Server; /** One entry per request the server accepted, in order. */ readonly requests: Array<{ method: string; path: string; headers: Record }>; } @@ -48,7 +50,23 @@ function useLoopback( }); const listening = withResolvers(); - server.on("error", (error: Error) => listening.reject(error)); + // Removed with the resource rather than after the first error: a listening + // server outlives its bind, and a handler left behind would still be + // holding a rejected resolver when the next test binds its own. + // + // Established before the handler exists, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all, so nothing may be attached until it has completed. + let onError: ((error: Error) => void) | undefined; + + yield* ensure(() => { + if (onError) { + server.off("error", onError); + } + }); + + onError = (error: Error) => listening.reject(error); + server.on("error", onError); server.listen(0, "127.0.0.1", () => listening.resolve()); yield* listening.operation; @@ -66,7 +84,7 @@ function useLoopback( throw new Error("the loopback server reported no TCP address"); } - yield* provide({ origin: `http://127.0.0.1:${address.port}`, requests }); + yield* provide({ origin: `http://127.0.0.1:${address.port}`, requests, server }); }); } @@ -359,4 +377,47 @@ describe("Tier FR — the chainable calling shape", () => { expect(failure?.message).toContain("timed out after 60ms"); }); + /** + * The listener's error observer belongs to the loopback resource, not to the + * first error it sees. The count is read after the resource has been torn + * down and before the event is replayed, because an observer removed by its + * own event would leave the same count behind as one that was released. + */ + it("releases the loopback's error observer with the resource", function* () { + let observed: Server | undefined; + let live = 0; + let baseline = 0; + + yield* scoped(function* () { + const loopback = yield* useLoopback((_request, response) => response.end("{}")); + observed = loopback.server; + baseline = 0; + live = loopback.server.listenerCount("error"); + }); + + if (!observed) { + throw new Error("the loopback never came up"); + } + + expect(live).toBeGreaterThanOrEqual(baseline + 1); + expect(observed.listenerCount("error")).toBe(live - 1); + + // Delivered through a sentinel of this case's own, because an `error` an + // emitter has no listener for is thrown rather than dropped — so the + // replay needs one observer, and exactly one is what it must find. + let seen = 0; + const sentinel = (): void => { + seen += 1; + }; + + observed.on("error", sentinel); + try { + observed.emit("error", new Error("after the loopback was torn down")); + } finally { + observed.off("error", sentinel); + } + + expect(seen).toBe(1); + expect(observed.listenerCount("error")).toBe(live - 1); + }); }); diff --git a/packages/runtime/tests/native-launcher.test.ts b/packages/runtime/tests/native-launcher.test.ts index cd6bc1b53..c39546b73 100644 --- a/packages/runtime/tests/native-launcher.test.ts +++ b/packages/runtime/tests/native-launcher.test.ts @@ -22,11 +22,13 @@ import { randomUUID } from "node:crypto"; import * as path from "node:path"; import * as os from "node:os"; import process from "node:process"; +import { spawn as spawnChild } from "node:child_process"; import { flushOutput, installForegroundLauncher, nativeLaunch, NO_TERMINAL, + reap, reserveTerminal, } from "../launcher.ts"; @@ -239,6 +241,60 @@ describe("Tier FL — the foreground native launcher", () => { }); }); +describe("native launcher — the reaper's own listener", () => { + /** + * The reaper waits on the child's `exit` from inside a bounded Promise, so + * its handler is not covered by an Effection scope: `done()` is the only + * funnel out — the event itself, the reachability poll, the escalation + * deadline, and the refusal that rejects — and it is where the handler comes + * off. The count is read after the reap has settled and before the event is + * replayed, because a handler that removed itself on `exit` would leave the + * same count behind as one that was released. + */ + it("NLR1: releases the exit handler when the reap settles, and a later exit changes nothing", function* () { + const dir = yield* useTempDir(); + // Deliberately deaf to the interrupt, so the reap is still in flight while + // its handler is counted, and settles through the escalation rather than + // through the event — which is the path a self-removing listener would not + // have been released by. + const fake = yield* useFake(dir, "stubborn", { ignoreInterrupt: true, hang: true }); + const child = spawnChild(fake.command, [], { stdio: "ignore" }); + const before = child.listenerCount("exit"); + + // Not spawned: a Promise executor runs synchronously, so the handler is + // attached by the time `reap` has returned, and the count below is read + // with the reap unambiguously in flight rather than a turn after it. + const reaping = reap(child); + + // At least one more, not exactly one: a runtime may hold handlers of its + // own on this source, so the release below is measured against what was + // live rather than against the baseline. + const live = child.listenerCount("exit"); + expect(live).toBeGreaterThanOrEqual(before + 1); + + yield* until(reaping); + + expect(child.listenerCount("exit")).toBe(live - 1); + + child.emit("exit", 0, null); + + expect(child.listenerCount("exit")).toBe(live - 1); + }); + + /** A child already gone is answered without observing anything at all. */ + it("NLR2: installs nothing for a child that has already exited", function* () { + const dir = yield* useTempDir(); + const fake = yield* useFake(dir, "brief", { exitCode: 0 }); + const child = spawnChild(fake.command, [], { stdio: "ignore" }); + const before = child.listenerCount("exit"); + + yield* until(reap(child)); + yield* until(reap(child)); + + expect(child.listenerCount("exit")).toBe(before); + }); +}); + /** How many times the fake has beaten, or zero before its first beat. */ function* beats(file: string): Operation { try { diff --git a/packages/test-agent/deno.json b/packages/test-agent/deno.json index 9d9927717..4bd84d35d 100644 --- a/packages/test-agent/deno.json +++ b/packages/test-agent/deno.json @@ -6,7 +6,7 @@ }, "imports": { "@agentclientprotocol/sdk": "npm:@agentclientprotocol/sdk@1.3.0", - "@effectionx/node": "npm:@effectionx/node@0.2.4", + "@effectionx/node": "npm:@effectionx/node@0.2.5", "@effectionx/scope-eval": "npm:@effectionx/scope-eval@0.1.3", "@effectionx/stream-helpers": "npm:@effectionx/stream-helpers@0.8.3", "acorn": "npm:acorn@^8.16.0", diff --git a/packages/test-agent/package.json b/packages/test-agent/package.json index 8a5368c52..856665edf 100644 --- a/packages/test-agent/package.json +++ b/packages/test-agent/package.json @@ -8,7 +8,7 @@ }, "dependencies": { "@agentclientprotocol/sdk": "1.3.0", - "@effectionx/node": "0.2.4", + "@effectionx/node": "0.2.5", "@effectionx/scope-eval": "0.1.3", "@effectionx/stream-helpers": "0.8.3", "@executablemd/acp": "workspace:*", diff --git a/packages/test-agent/src/net.ts b/packages/test-agent/src/net.ts index 149f9ff6b..7fbb3aef9 100644 --- a/packages/test-agent/src/net.ts +++ b/packages/test-agent/src/net.ts @@ -9,6 +9,7 @@ import { createChannel, each, ensure, race, resource, spawn, withResolvers } from "effection"; import type { Operation, Stream, Task } from "effection"; import { fromReadable, on } from "@effectionx/node"; +import { once } from "@effectionx/node/events"; import { lines } from "@effectionx/stream-helpers"; import { connect, createServer } from "node:net"; import type { Socket } from "node:net"; @@ -34,21 +35,29 @@ export interface LineSocket { export function useLineSocket(socket: Socket): Operation { return resource(function* (provide) { const closed = withResolvers(); - socket.once("close", () => closed.resolve()); - yield* ensure(() => { + const onClose = (): void => closed.resolve(); + + // A lexical finalizer rather than `ensure()`: entering the `try` is + // synchronous, so there is no instant at which this socket is observed and + // the release is not yet armed. Detached first, because the destroy below + // emits the event this was observing. + try { + socket.on("close", onClose); + yield* provide({ + lines: lines()(fromReadable(socket)), + send(line) { + socket.write(line); + }, + end() { + socket.end(); + }, + closed: closed.operation, + }); + } finally { + socket.off("close", onClose); socket.destroy(); closed.resolve(); // settle on cancellation even if no 'close' follows - }); - yield* provide({ - lines: lines()(fromReadable(socket)), - send(line) { - socket.write(line); - }, - end() { - socket.end(); - }, - closed: closed.operation, - }); + } }); } @@ -81,24 +90,35 @@ export function useLineServer( } // Attach the close listener before close() (no missed-event race), and // only when the server actually came up, so a never-started or - // already-closed server leaves no pending operation. + // already-closed server leaves no pending operation. It stays attached + // through the wait — it is what the wait is for — and comes off + // synchronously afterwards, whether that wait settled or was halted. if (server.listening) { const closed = withResolvers(); - server.once("close", () => closed.resolve()); - server.close(); - yield* closed.operation; + const onClose = (): void => closed.resolve(); + + server.on("close", onClose); + try { + server.close(); + yield* closed.operation; + } finally { + server.off("close", onClose); + } } }); - // Attach the readiness listeners before listen, so the event is never - // missed by a later subscription. - const listening = withResolvers(); - server.once("listening", () => listening.resolve()); - server.once("error", (error) => { - listening.reject(error instanceof Error ? error : new Error(String(error))); - }); + // Raced inline, in the same synchronous run as `listen`, so both arms are + // attached before either event can be delivered — `listen` never emits in + // the turn it was called in, and a spawned race would attach a turn late. + // The loser is halted, which is what detaches it. server.listen(0, host); - yield* listening.operation; + yield* race([ + once(server, "listening"), + (function* (): Operation { + const [error] = yield* once(server, "error"); + throw error instanceof Error ? error : new Error(String(error)); + })(), + ]); const address = server.address(); if (!address || typeof address !== "object") { @@ -140,14 +160,24 @@ export function useLineClient( ): Operation> { return resource(function* (provide) { const socket = connect(port, host); - // Attach connect/error listeners before yielding, so neither is missed. - const connected = withResolvers(); - socket.once("connect", () => connected.resolve()); - socket.once("error", (error) => { - connected.reject(error instanceof Error ? error : new Error(String(error))); + // Owned before the handshake is awaited: a connect that fails has to leave + // no socket behind, and `useLineSocket` cannot take ownership of one until + // the handshake has settled. + yield* ensure(() => { + socket.destroy(); }); + + // Raced inline, in the same synchronous run as `connect`, for the reason + // given in `useLineServer`. + yield* race([ + once(socket, "connect"), + (function* (): Operation { + const [error] = yield* once(socket, "error"); + throw error instanceof Error ? error : new Error(String(error)); + })(), + ]); + const connection = yield* useLineSocket(socket); - yield* connected.operation; const inbound = createChannel(); const subscription = yield* inbound; // subscribe before the pump: no loss diff --git a/packages/test-agent/src/worker/acp-server.ts b/packages/test-agent/src/worker/acp-server.ts index 0b2a14681..e8dddf16e 100644 --- a/packages/test-agent/src/worker/acp-server.ts +++ b/packages/test-agent/src/worker/acp-server.ts @@ -222,17 +222,21 @@ export function useProcessStdio(): Operation { // already settled } }; - process.stdin.on("data", onData); - process.stdin.on("end", onEnd); - process.stdin.on("close", onClose); - process.stdin.on("error", onError); - yield* ensure(() => { + // A lexical finalizer rather than `ensure()`: entering the `try` is + // synchronous, so there is no instant at which standard input is observed + // and the release is not yet armed. + try { + process.stdin.on("data", onData); + process.stdin.on("end", onEnd); + process.stdin.on("close", onClose); + process.stdin.on("error", onError); + + yield* provide({ input, output }); + } finally { process.stdin.off("data", onData); process.stdin.off("end", onEnd); process.stdin.off("close", onClose); process.stdin.off("error", onError); - }); - - yield* provide({ input, output }); + } }); } diff --git a/packages/test-agent/tests/net.test.ts b/packages/test-agent/tests/net.test.ts index bcff1c324..97078f4fc 100644 --- a/packages/test-agent/tests/net.test.ts +++ b/packages/test-agent/tests/net.test.ts @@ -6,11 +6,11 @@ */ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { each, ensure, race, scoped, spawn, suspend, withResolvers } from "effection"; +import { each, ensure, race, scoped, sleep, spawn, suspend, withResolvers } from "effection"; import type { Operation } from "effection"; import { once } from "@effectionx/node"; import { connect } from "node:net"; -import { useLineClient, useLineServer } from "../src/net.ts"; +import { useLineClient, useLineServer, useLineSocket } from "../src/net.ts"; import type { LineSocket } from "../src/net.ts"; describe("Tier NR — line-socket adapter", () => { @@ -93,4 +93,52 @@ describe("Tier NR — line-socket adapter", () => { ]); expect(outcome).toBe("refused"); }); + + /** + * The socket adapter's own listener, rather than what it provides. + * + * A cancelled acquisition is the case the event never comes for, so the + * handler cannot be waiting for it to leave. What proves that is the count + * on the socket itself: one while the resource is live, back to where it + * started once the owner is halted, and unmoved by a `close` afterwards — + * a listener that removed itself when the event finally arrived would leave + * the same count behind as one that was never there. + */ + it("NR4: halting a line socket detaches it, and a later close changes nothing", function* () { + const server = yield* useLineServer("127.0.0.1", function* () { + yield* suspend(); + }); + + const socket = connect(server.port, "127.0.0.1"); + const connected = withResolvers(); + const onConnect = (): void => connected.resolve(); + + socket.on("connect", onConnect); + try { + yield* connected.operation; + } finally { + socket.off("connect", onConnect); + } + + const before = socket.listenerCount("close"); + const owner = yield* spawn(function* () { + yield* useLineSocket(socket); + yield* suspend(); + }); + yield* sleep(0); + + // At least one more, not exactly one: a runtime may hold handlers of its + // own on this source, so the release below is measured against what was + // live rather than against the baseline. + const live = socket.listenerCount("close"); + expect(live).toBeGreaterThanOrEqual(before + 1); + + yield* owner.halt(); + + expect(socket.listenerCount("close")).toBe(live - 1); + + socket.emit("close"); + + expect(socket.listenerCount("close")).toBe(live - 1); + }); }); diff --git a/packages/test-agent/tests/public-api.test.ts b/packages/test-agent/tests/public-api.test.ts index 082bd9832..57f4dcdeb 100644 --- a/packages/test-agent/tests/public-api.test.ts +++ b/packages/test-agent/tests/public-api.test.ts @@ -7,10 +7,10 @@ */ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { ensure, scoped, withResolvers } from "effection"; +import { ensure, race, scoped } from "effection"; import type { Operation } from "effection"; import { connect } from "node:net"; -import { once } from "@effectionx/node"; +import { once } from "@effectionx/node/events"; import * as os from "node:os"; import { useTestAgentController } from "../mod.ts"; import { parseRoute } from "../src/protocol.ts"; @@ -26,16 +26,24 @@ function* reachable(route: string): Operation { // The socket is closed, and its close observed, before the probe answers, // so a later probe never races the previous connection's teardown. return yield* scoped(function* () { - const settled = withResolvers(); const socket = connect({ host: parsed.value.host, port: parsed.value.port }); yield* ensure(function* () { socket.destroy(); yield* once(socket, "close"); }); - socket.once("connect", () => settled.resolve(true)); - socket.once("error", () => settled.resolve(false)); - return yield* settled.operation; + // Raced inline, in the same synchronous run as `connect`: a spawned race + // attaches its arms a turn later, and the socket can settle in that turn. + return yield* race([ + (function* (): Operation { + yield* once(socket, "connect"); + return true; + })(), + (function* (): Operation { + yield* once(socket, "error"); + return false; + })(), + ]); }); } diff --git a/packages/test-support/launch.ts b/packages/test-support/launch.ts index 49bc4f01f..c7cfc37ab 100644 --- a/packages/test-support/launch.ts +++ b/packages/test-support/launch.ts @@ -92,6 +92,16 @@ export interface CliRunOptions { * of file rather than nothing at all. */ stdin?: string; + /** + * Handed the child this launcher owns, before anything is attached to it, + * together with a reader for what has been captured from it so far. + * + * Package-private, for the cancellation regression: the listeners are on a + * value this operation owns and does not otherwise hand out, and what a + * cancelled run must prove is that they are gone *and* that a later chunk + * reaches nothing that is still accumulating. + */ + observeChild?: (child: ChildProcess, captured: () => { stdout: string; stderr: string }) => void; } /** A bounded run of `xmd`, synchronized like any `@effectionx/process` exec. */ @@ -222,44 +232,78 @@ function* withInput( input: string, ): Operation { const settled = withResolvers>(); - const closed = withResolvers(); - const child = spawnChild(launch.command, launch.arguments ?? [], { - detached: true, - shell: launch.shell, - cwd: options.cwd, - env: cliEnv(options), - stdio: "pipe", - }); - - child.stdout?.on("data", (chunk: Uint8Array) => { + // `close`, and nothing else, is what says this child and its pipes are done. + let closed = false; + // Declared before the cleanup below and assigned after it: a child that + // exists before its release is registered can be stranded, because `yield* + // ensure(...)` is itself a suspension and an owner halted while it registers + // unwinds with nothing on it. + let child: ChildProcess | undefined; + + const onStdout = (chunk: Uint8Array): void => { partial.stdout += text(chunk); - }); - child.stderr?.on("data", (chunk: Uint8Array) => { + }; + const onStderr = (chunk: Uint8Array): void => { partial.stderr += text(chunk); - }); + }; // A child that never started closes through this rather than through `close`, // so teardown has an end either way. - child.on("error", (error: Error) => { + const onError = (error: Error): void => { settled.resolve(Err(error)); - closed.resolve(); - }); + }; // A pipe the child could not use is the launcher's problem and not the run's: // the close below is what this whole path exists for, and a broken one would // otherwise raise on a process that is already reporting why. - child.stdin?.on("error", () => {}); - child.on("close", (code: number | null, signal: string | null) => { + const onStdinError = (): void => {}; + const onClose = (code: number | null, signal: string | null): void => { + closed = true; settled.resolve( Ok({ ...(code === null ? {} : { code }), ...(signal === null ? {} : { signal }), }), ); - closed.resolve(); + }; + + // Established before the child exists, so a run cancelled anywhere below + // still ends with its process group gone — and so that no instant exists in + // which a child is running with no cleanup registered for it. + // + // Teardown keeps every handler attached through the reap: `close` is what + // says the process is finished, and the capture must still be reading what + // the child writes on its way out. They come off synchronously once that + // wait has settled, whichever way it did. + yield* ensure(function* () { + if (child === undefined) { + return; + } + + try { + yield* reap(child, () => closed); + } finally { + child.stdout?.off("data", onStdout); + child.stderr?.off("data", onStderr); + child.off("error", onError); + child.stdin?.off("error", onStdinError); + child.off("close", onClose); + } + }); + + child = spawnChild(launch.command, launch.arguments ?? [], { + detached: true, + shell: launch.shell, + cwd: options.cwd, + env: cliEnv(options), + stdio: "pipe", }); - // Registered before the first suspension point, so a run cancelled anywhere - // below still ends with its process group gone. - yield* ensure(() => reap(child, closed.operation)); + options.observeChild?.(child, () => ({ stdout: partial.stdout, stderr: partial.stderr })); + + child.stdout?.on("data", onStdout); + child.stderr?.on("data", onStderr); + child.on("error", onError); + child.stdin?.on("error", onStdinError); + child.on("close", onClose); child.stdin?.end(input); @@ -289,24 +333,38 @@ function* withInput( * could be delivered to while the child is still reachable ends the run with * that fact rather than waiting on a `close` that is never coming. */ -function* reap(child: ChildProcess, closed: Operation): Operation { - const pid = child.pid; - if (pid === undefined || child.exitCode !== null || child.signalCode !== null) { - yield* closed; - return; - } +function* reap(child: ChildProcess, hasClosed: () => boolean): Operation { + const gone = withResolvers(); + const onGone = (): void => gone.resolve(); + + // `close`, and nothing else. An assigned `exitCode` or `signalCode` says the + // process ended; it does not say the pipes this run inherited have, and the + // capture is still reading them. + child.on("close", onGone); + try { + if (hasClosed()) { + return; + } - end(pid, "SIGTERM"); - const graceful = yield* timebox(TERMINATION_GRACE, () => closed); - if (!graceful.timeout) { - return; - } + const pid = child.pid; + + if (pid !== undefined) { + end(pid, "SIGTERM"); + const graceful = yield* timebox(TERMINATION_GRACE, () => gone.operation); + if (!graceful.timeout) { + return; + } + + const killed = end(pid, "SIGKILL"); + if (killed === "refused" && isReachable(pid)) { + throw new Error(`the launched process ${pid} could not be stopped: SIGKILL was refused`); + } + } - const killed = end(pid, "SIGKILL"); - if (killed === "refused" && isReachable(pid)) { - throw new Error(`the launched process ${pid} could not be stopped: SIGKILL was refused`); + yield* gone.operation; + } finally { + child.off("close", onGone); } - yield* closed; } /** What one signal delivery established about what it was aimed at. */ diff --git a/packages/web/package.json b/packages/web/package.json index 54e333f6a..15caf8b37 100644 --- a/packages/web/package.json +++ b/packages/web/package.json @@ -7,7 +7,7 @@ ".": "./mod.ts" }, "dependencies": { - "@effectionx/node": "0.2.4", + "@effectionx/node": "0.2.5", "@executablemd/core": "workspace:*", "@executablemd/durable-streams": "workspace:*", "@executablemd/runtime": "workspace:*", diff --git a/packages/web/src/responder.ts b/packages/web/src/responder.ts index da28282c1..b961228c1 100644 --- a/packages/web/src/responder.ts +++ b/packages/web/src/responder.ts @@ -15,9 +15,10 @@ */ import { type Api, createApi, type Operations } from "@effectionx/context-api"; -import { action } from "effection"; +import { ensure, scoped, withResolvers } from "effection"; import type { Operation } from "effection"; import { request } from "node:http"; +import type { ClientRequest, IncomingMessage } from "node:http"; import type { Json } from "./json.ts"; @@ -53,15 +54,75 @@ export interface FormResponse { * sends, and the server still checks it. `Host` is left to `node:http` to derive * from the URL, so it names where the request is actually going. */ -export function submitForm(url: string, data: Json): Operation { - return postJson(new URL("submit", url), JSON.stringify(data)); +export function submitForm( + url: string, + data: Json, + observe?: (request: ClientRequest) => void, +): Operation { + return postJson(new URL("submit", url), JSON.stringify(data), observe); } -function postJson(url: URL, body: string): Operation { - return action((resolve, reject) => { +function postJson( + url: URL, + body: string, + observe?: (request: ClientRequest) => void, +): Operation { + return scoped(function* () { const payload = new TextEncoder().encode(body); + const settled = withResolvers(); + const closed = withResolvers(); + // `close`, and nothing else. A destroyed request has not necessarily + // finished emitting: the peer's hang-up arrives afterwards, and this is + // what says there is nothing left to observe. + let finished = false; - const outgoing = request( + let received = ""; + let incoming: IncomingMessage | undefined; + let outgoing: ClientRequest | undefined; + + const onData = (chunk: string): void => { + received += chunk; + }; + const onEnd = (): void => { + settled.resolve({ status: incoming?.statusCode ?? 0, body: received }); + }; + const onFailure = (error: Error): void => settled.reject(error); + const onClose = (): void => { + finished = true; + closed.resolve(); + }; + + // Established before the request exists, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all. + // + // The error observer stays attached across the destroy. `destroy()` on a + // live request makes the peer's end arrive as an asynchronous `error` — + // "socket hang up" — and an `error` an emitter has no listener for is + // thrown, not dropped. So the wait for `close` is what says the request can + // no longer emit, and only then is anything detached. + yield* ensure(function* () { + try { + if (outgoing && !finished) { + if (!outgoing.destroyed) { + outgoing.destroy(); + } + yield* closed.operation; + } + } finally { + if (incoming) { + incoming.off("data", onData); + incoming.off("end", onEnd); + incoming.off("error", onFailure); + } + if (outgoing) { + outgoing.off("error", onFailure); + outgoing.off("close", onClose); + } + } + }); + + outgoing = request( { protocol: url.protocol, hostname: url.hostname, @@ -74,25 +135,20 @@ function postJson(url: URL, body: string): Operation { Origin: url.origin, }, }, - (incoming) => { - let received = ""; - incoming.setEncoding("utf8"); - incoming.on("data", (chunk: string) => { - received += chunk; - }); - incoming.on("end", () => { - resolve({ status: incoming.statusCode ?? 0, body: received }); - }); - incoming.on("error", (error: Error) => reject(error)); + (response) => { + incoming = response; + response.setEncoding("utf8"); + response.on("data", onData); + response.on("end", onEnd); + response.on("error", onFailure); }, ); - outgoing.on("error", (error: Error) => reject(error)); + observe?.(outgoing); + outgoing.on("error", onFailure); + outgoing.on("close", onClose); outgoing.end(payload); - // Runs however the action ends, so a halted responder leaves no socket. - return () => { - outgoing.destroy(); - }; + return yield* settled.operation; }); } diff --git a/packages/web/src/response-channel.ts b/packages/web/src/response-channel.ts index 84ef5b9ad..5ffcd5b34 100644 --- a/packages/web/src/response-channel.ts +++ b/packages/web/src/response-channel.ts @@ -14,7 +14,7 @@ * when it chooses. */ -import { withResolvers } from "effection"; +import { ensure, resource, withResolvers } from "effection"; import type { Operation } from "effection"; import type { ServerResponse } from "node:http"; @@ -41,37 +41,47 @@ export class ResponseClosedError extends Error { } /** - * Wrap a Node response. + * Wrap a Node response, for as long as the request task that acquires it runs. * - * The listeners are armed here, when the channel is built, rather than when - * `finished` is first awaited. `finish` can fire between `end()` and the - * `yield*` that observes it, and a listener attached after the fact would wait - * for an event that already happened. Arming once, before anything is written, - * removes that window without depending on `writableFinished` being reported the - * same way by every runtime. + * The listeners are armed on acquisition, before anything is written, rather + * than when `finished` is first awaited. `finish` can fire between `end()` and + * the `yield*` that observes it, and a listener attached after the fact would + * wait for an event that already happened. Arming once removes that window + * without depending on `writableFinished` being reported the same way by every + * runtime. + * + * Both handlers come off when the task ends, however it ends. The first + * settlement is the answer — a `close` after a `finish` changes nothing — so + * the two no longer need to remove each other, and a request abandoned before + * either event leaves nothing attached to a response the server is about to + * destroy. */ -export function nodeResponseChannel(res: ServerResponse): ResponseChannel { - const settled = withResolvers(); +export function nodeResponseChannel(res: ServerResponse): Operation { + return resource(function* (provide) { + const settled = withResolvers(); - const onFinish = (): void => { - res.removeListener("close", onClose); - settled.resolve(); - }; - const onClose = (): void => { - res.removeListener("finish", onFinish); - settled.reject(new ResponseClosedError()); - }; + const onFinish = (): void => settled.resolve(); + const onClose = (): void => settled.reject(new ResponseClosedError()); - res.once("finish", onFinish); - res.once("close", onClose); + // A lexical finalizer rather than `ensure()`: entering the `try` is + // synchronous, so there is no instant at which this response is observed + // and the release is not yet armed. + try { + res.on("finish", onFinish); + res.on("close", onClose); - return { - head(status: number, headers: Record): void { - res.writeHead(status, headers); - }, - end(body?: string): void { - res.end(body); - }, - finished: settled.operation, - }; + yield* provide({ + head(status: number, headers: Record): void { + res.writeHead(status, headers); + }, + end(body?: string): void { + res.end(body); + }, + finished: settled.operation, + }); + } finally { + res.off("finish", onFinish); + res.off("close", onClose); + } + }); } diff --git a/packages/web/src/server.ts b/packages/web/src/server.ts index b90ca33e1..1ad8b44b6 100644 --- a/packages/web/src/server.ts +++ b/packages/web/src/server.ts @@ -88,7 +88,15 @@ export interface FormServer { * always run for real. */ export interface FormServerSeams { - responseChannel?(res: ServerResponse): ResponseChannel; + responseChannel?(res: ServerResponse): Operation; + /** + * Handed every accepted socket as it arrives. + * + * Package-private, for the cancellation regression: the listeners are on a + * value this operation owns and does not otherwise hand out, and their + * release is the thing under test. + */ + observeSocket?(socket: Socket): void; /** * After the listener is up, before `provide()`. * @@ -150,9 +158,20 @@ export function useFormServer( const server = createServer(); const channelFor = seams.responseChannel ?? nodeResponseChannel; + // One close handler per accepted socket, kept so teardown can detach them + // all before it destroys the connections they are attached to. + const closers = new Map void>(); const onConnection = (socket: Socket): void => { + // Before this server attaches anything, so a case can measure what the + // runtime was already holding on the connection. + seams.observeSocket?.(socket); sockets.add(socket); - socket.once("close", () => sockets.delete(socket)); + const onClose = (): void => { + sockets.delete(socket); + closers.delete(socket); + }; + closers.set(socket, onClose); + socket.on("close", onClose); }; // One long-lived observer, not a readiness-only one: an error handler that // stopped mattering once the server came up would leave a caller waiting on @@ -168,18 +187,26 @@ export function useFormServer( const onListening = (): void => listening.resolve(); - // Attached before `listen`, not after: an event emitted before its listener - // exists is simply lost, and readiness is emitted immediately. - server.once("listening", onListening); - server.on("connection", onConnection); - server.on("error", onError); - - // Registered before `listen`, so a server that fails to bind is still torn - // down by the same path as one that served for an hour. + // Registered before anything is attached and before `listen`, so a server + // that fails to bind is torn down by the same path as one that served for + // an hour — and so that no instant exists in which this server is observed + // and the release is not yet armed. `yield* ensure(...)` is itself a + // suspension, and an owner halted while it registers unwinds with nothing + // on it. yield* ensure(function* () { - server.removeListener("listening", onListening); - server.removeListener("connection", onConnection); - server.removeListener("error", onError); + server.off("listening", onListening); + server.off("connection", onConnection); + server.off("error", onError); + // Every accepted socket's close handler comes off in the same + // uninterrupted run as the listener's own, before this teardown + // suspends: `connection` is no longer observed, so nothing can be + // accepted and left untracked after this point. + for (const [socket, onClose] of closers) { + socket.off("close", onClose); + } + closers.clear(); + // Request tasks end before their connections do, so a response in flight + // is abandoned by its own scope rather than by a destroyed socket. if (acceptor) { yield* acceptor.halt(); } @@ -191,12 +218,23 @@ export function useFormServer( sockets.clear(); if (server.listening) { const closed = withResolvers(); - server.once("close", () => closed.resolve()); - server.close(); - yield* closed.operation; + const onClose = (): void => closed.resolve(); + server.on("close", onClose); + try { + server.close(); + yield* closed.operation; + } finally { + server.off("close", onClose); + } } }); + // Attached before `listen`, not after: an event emitted before its listener + // exists is simply lost, and readiness is emitted immediately. + server.on("listening", onListening); + server.on("connection", onConnection); + server.on("error", onError); + server.listen(0, HOST); yield* listening.operation; ready = true; @@ -210,7 +248,7 @@ export function useFormServer( const prefix = `/f/${token}/`; function* handle(req: IncomingMessage, res: ServerResponse): Operation { - const channel = channelFor(res); + const channel = yield* channelFor(res); const route = routeFor(req, prefix); if (route === undefined || req.headers.host !== `${HOST}:${port}`) { diff --git a/packages/web/tests/http-client.ts b/packages/web/tests/http-client.ts index 583ace9a6..a25664d9f 100644 --- a/packages/web/tests/http-client.ts +++ b/packages/web/tests/http-client.ts @@ -27,6 +27,15 @@ export interface HttpResponse { } export interface HttpConnection { + /** The connection itself, so a case can say what it is still observing. */ + socket: Socket; + /** + * How many times this client's own handlers have run. + * + * A cancelled connection must stop counting: a chunk delivered after + * teardown that still moved this would be a handler nobody released. + */ + callbacks(): number; /** Send text, encoded as UTF-8. */ write(text: string): void; /** @@ -53,9 +62,13 @@ export interface HttpConnection { * provokes one would fail for the wrong reason. A reset and a clean close settle * `ended` alike, because after a mid-stream refusal the peer may present either. */ -export function useConnection(port: number): Operation { +export function useConnection( + port: number, + observe?: (socket: Socket) => void, +): Operation { return resource(function* (provide) { const socket = connect(port, "127.0.0.1"); + observe?.(socket); yield* ensure(() => { socket.destroy(); }); @@ -63,11 +76,11 @@ export function useConnection(port: number): Operation { yield* action((resolve, reject) => { const onConnect = (): void => resolve(); const onError = (error: Error): void => reject(error); - socket.once("connect", onConnect); - socket.once("error", onError); + socket.on("connect", onConnect); + socket.on("error", onError); return () => { - socket.removeListener("connect", onConnect); - socket.removeListener("error", onError); + socket.off("connect", onConnect); + socket.off("error", onError); }; }); @@ -87,19 +100,32 @@ export function useConnection(port: number): Operation { notify?.(); }; - socket.on("data", (chunk: Buffer) => { + const onData = (chunk: Buffer): void => { buffer += chunk.toString("utf8"); advance(); - }); - socket.on("error", (error: Error) => { + }; + const onError = (error: Error): void => { ended.resolve(`error:${error.message}`); advance(); - }); - socket.on("close", () => { + }; + const onClose = (): void => { ended.resolve("close"); advance(); + }; + + // Established before the handlers exist, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all. + yield* ensure(() => { + socket.off("data", onData); + socket.off("error", onError); + socket.off("close", onClose); }); + socket.on("data", onData); + socket.on("error", onError); + socket.on("close", onClose); + function* untilAfter(seen: number): Operation { if (version !== seen) { return; @@ -113,6 +139,10 @@ export function useConnection(port: number): Operation { } yield* provide({ + socket, + callbacks(): number { + return version; + }, write(text: string): void { socket.write(new Uint8Array(new TextEncoder().encode(text))); }, diff --git a/packages/web/tests/responder.test.ts b/packages/web/tests/responder.test.ts index 0b2609a9b..251ddbc8a 100644 --- a/packages/web/tests/responder.test.ts +++ b/packages/web/tests/responder.test.ts @@ -1,8 +1,10 @@ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { scoped, spawn, suspend, withResolvers } from "effection"; +import { ensure, scoped, sleep, spawn, suspend, withResolvers } from "effection"; import type { Operation } from "effection"; +import { createServer } from "node:http"; +import type { ClientRequest } from "node:http"; import { FormResponder, respond, submitForm } from "../src/responder.ts"; import type { FormResponse } from "../src/responder.ts"; import { useFormServer } from "../src/server.ts"; @@ -258,3 +260,128 @@ describe("responder: subject to the protocol, not exempt from it", () => { expect(later.body).toBe(""); }); }); + +describe("responder: the request it opens is its own", () => { + /** + * A submission cancelled before its response arrives is the case the events + * never come for. The server here accepts and answers nothing, so the halt + * lands while the request is live: the counts are read before the halt, again + * after it and before the event is replayed, and the replay must reach + * neither the abandoned operation nor anything still accumulating. + */ + it("releases the outgoing request's handlers when the submission is cancelled", function* () { + const server = createServer(() => {}); + const listening = withResolvers(); + const onError = (error: Error): void => listening.reject(error); + + yield* ensure(() => { + server.off("error", onError); + }); + server.on("error", onError); + + server.listen(0, "127.0.0.1", () => listening.resolve()); + yield* listening.operation; + yield* ensure(() => { + server.closeAllConnections(); + server.close(); + }); + + const address = server.address(); + if (address === null || typeof address === "string") { + throw new Error("the silent server did not listen on a TCP port"); + } + + let outgoing: ClientRequest | undefined; + let before = 0; + let settled = false; + + const owner = yield* spawn(function* () { + yield* submitForm( + `http://127.0.0.1:${address.port}/f/x/`, + { decision: "approve" }, + (request) => { + outgoing = request; + before = request.listenerCount("error"); + }, + ); + settled = true; + }); + + // A turn after the executor ran, so its handler is attached and its + // cleanup — the one `action()` returns — is the executor's own return + // value rather than something still being registered. + yield* sleep(0); + if (!outgoing) { + throw new Error("the responder opened no request"); + } + + // At least one more, not exactly one: a runtime may hold handlers of its + // own on this source, so the release below is measured against what was + // live rather than against the baseline. + const live = outgoing.listenerCount("error"); + expect(live).toBeGreaterThanOrEqual(before + 1); + expect(settled).toBe(false); + + yield* owner.halt(); + + expect(outgoing.listenerCount("error")).toBe(live - 1); + + let seen = 0; + const sentinel = (): void => { + seen += 1; + }; + + outgoing.on("error", sentinel); + try { + outgoing.emit("error", new Error("after the submission was cancelled")); + } finally { + outgoing.off("error", sentinel); + } + + expect(seen).toBe(1); + expect(outgoing.listenerCount("error")).toBe(live - 1); + expect(settled).toBe(false); + }); + + /** + * `postJson` observes its outgoing request and the response it brings back + * for exactly as long as the `action` runs. The count is read after the + * submission has settled and before the event is replayed, because a handler + * removed by its own event would leave the same count behind as one the + * action released. + */ + it("releases the outgoing request's error handler when the submission settles", function* () { + const server = yield* useFormServer(formInput()); + let outgoing: ClientRequest | undefined; + let before = 0; + + const response = yield* submitForm(server.url, { decision: "approve" }, (request) => { + outgoing = request; + before = request.listenerCount("error"); + }); + + expect(response.status).toBe(204); + if (!outgoing) { + throw new Error("the responder opened no request"); + } + + // The case above reads the count while it is live; this one is the + // completion path, where what matters is that settling released it. + expect(outgoing.listenerCount("error")).toBe(before); + + let seen = 0; + const sentinel = (): void => { + seen += 1; + }; + + outgoing.on("error", sentinel); + try { + outgoing.emit("error", new Error("after the submission settled")); + } finally { + outgoing.off("error", sentinel); + } + + expect(seen).toBe(1); + expect(outgoing.listenerCount("error")).toBe(before); + }); +}); diff --git a/packages/web/tests/server-lifecycle.test.ts b/packages/web/tests/server-lifecycle.test.ts index 2758fe828..4292def65 100644 --- a/packages/web/tests/server-lifecycle.test.ts +++ b/packages/web/tests/server-lifecycle.test.ts @@ -1,10 +1,16 @@ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; -import { scoped, spawn, withResolvers } from "effection"; +import { ensure, resource, scoped, sleep, spawn, suspend, withResolvers } from "effection"; import type { Operation } from "effection"; +import { once } from "@effectionx/node/events"; +import { when } from "@effectionx/converge"; +import { Buffer } from "node:buffer"; +import { createServer } from "node:http"; +import type { ServerResponse } from "node:http"; import { connect } from "node:net"; import type { Socket } from "node:net"; +import { nodeResponseChannel } from "../src/response-channel.ts"; import type { ResponseChannel } from "../src/response-channel.ts"; import { useFormServer } from "../src/server.ts"; import type { FormServer } from "../src/server.ts"; @@ -20,30 +26,31 @@ interface HeldChannel { closeWithoutFinish: () => void; } -function heldChannel(): { seam: () => ResponseChannel; held: HeldChannel } { +function heldChannel(): { seam: () => Operation; held: HeldChannel } { const ends: { status: number; body: string | undefined }[] = []; const releases: { finish: () => void; fail: (error: Error) => void }[] = []; const firstEnd = withResolvers(); return { - seam: () => { - const settled = withResolvers(); - let status = 0; - releases.push({ - finish: () => settled.resolve(), - fail: (error: Error) => settled.reject(error), - }); - return { - head(next: number): void { - status = next; - }, - end(body?: string): void { - ends.push({ status, body }); - firstEnd.resolve(); - }, - finished: settled.operation, - }; - }, + seam: () => + resource(function* (provide) { + const settled = withResolvers(); + let status = 0; + releases.push({ + finish: () => settled.resolve(), + fail: (error: Error) => settled.reject(error), + }); + yield* provide({ + head(next: number): void { + status = next; + }, + end(body?: string): void { + ends.push({ status, body }); + firstEnd.resolve(); + }, + finished: settled.operation, + }); + }), held: { ends: () => ends, ended: firstEnd.operation, @@ -106,6 +113,7 @@ describe("form server: the submission resolves only after its response is sent", it("fails rather than resolving when the response closes before finishing", function* () { let refusedPort = 0; let leftover: KeepAlive | undefined; + const keepAlives = yield* useKeepAlives(); yield* scoped(function* () { const { seam, held } = heldChannel(); @@ -114,7 +122,7 @@ describe("form server: the submission resolves only after its response is sent", refusedPort = addressOf(server.url).port; // A keep-alive connection that must not survive teardown. - leftover = yield* openKeepAlive(refusedPort); + leftover = yield* keepAlives.open(refusedPort); yield* submitValid(server); yield* held.ended; @@ -145,13 +153,14 @@ describe("form server: failure reaches the caller", () => { let recordedPort = 0; let keepAlive: KeepAlive | undefined; let acquired = false; + const keepAlives = yield* useKeepAlives(); yield* scoped(function* () { try { yield* useFormServer(formInput(), { *afterListen(address) { recordedPort = address.port; - keepAlive = yield* openKeepAlive(address.port); + keepAlive = yield* keepAlives.open(address.port); throw new Error("setup failed after the listener came up"); }, }); @@ -205,11 +214,12 @@ describe("form server: teardown", () => { it("closes the port and its connections after a successful submission", function* () { let port = 0; let keepAlive: KeepAlive | undefined; + const keepAlives = yield* useKeepAlives(); yield* scoped(function* () { const server = yield* useFormServer(formInput()); port = addressOf(server.url).port; - keepAlive = yield* openKeepAlive(port); + keepAlive = yield* keepAlives.open(port); yield* submitValid(server); yield* server.submission; }); @@ -233,11 +243,12 @@ describe("form server: teardown", () => { */ it("releases the listener when the owning task is halted mid-wait", function* () { const ready = withResolvers<{ port: number; keepAlive: KeepAlive }>(); + const keepAlives = yield* useKeepAlives(); const owner = yield* spawn(function* () { const server = yield* useFormServer(formInput()); const port = addressOf(server.url).port; - ready.resolve({ port, keepAlive: yield* openKeepAlive(port) }); + ready.resolve({ port, keepAlive: yield* keepAlives.open(port) }); // Still waiting for a submission that never comes when the halt lands. yield* server.submission; }); @@ -256,6 +267,94 @@ describe("form server: teardown", () => { }); }); +/** + * A loopback that hands the test the live `ServerResponse` of one request. + * + * The response channel's listeners are what this measures, and they can only + * be counted on a response Node itself made. + */ +function useResponseUnderTest(): Operation { + return resource(function* (provide) { + const arrived = withResolvers(); + const server = createServer((incoming, outgoing) => { + incoming.resume(); + arrived.resolve(outgoing); + }); + + const listening = withResolvers(); + const onError = (error: Error): void => listening.reject(error); + + yield* ensure(() => { + server.off("error", onError); + }); + server.on("error", onError); + + server.listen(0, "127.0.0.1", () => listening.resolve()); + yield* listening.operation; + + yield* ensure(function* () { + server.closeAllConnections(); + const closed = withResolvers(); + const onClose = (): void => closed.resolve(); + + server.on("close", onClose); + try { + server.close(); + yield* closed.operation; + } finally { + server.off("close", onClose); + } + }); + + const address = server.address(); + if (address === null || typeof address === "string") { + throw new Error("the response fixture did not listen on a TCP port"); + } + + const connection = yield* useConnection(address.port); + connection.write(requestText({ method: "GET", path: "/", host: `127.0.0.1:${address.port}` })); + + yield* provide(yield* arrived.operation); + }); +} + +describe("form server: a response channel belongs to its request task", () => { + /** + * A request abandoned before it answered is the case `finish` never comes + * for, so the channel cannot be relying on it to let go. The counts are read + * on the response itself, and the one after the halt is read before the + * event is emitted: a handler that removed itself when `finish` finally + * arrived would leave the same count behind as one that was never there. + */ + it("detaches from the response when the task is halted, and a later finish changes nothing", function* () { + const res = yield* useResponseUnderTest(); + const counts = (): number[] => [res.listenerCount("finish"), res.listenerCount("close")]; + const before = counts(); + + const owner = yield* spawn(function* () { + yield* nodeResponseChannel(res); + yield* suspend(); + }); + yield* sleep(0); + + // Anchored to what was live, not to a delta from the baseline: how many + // handlers a runtime keeps on its own `ServerResponse`, and when it adds + // them, is the runtime's business — Bun attaches one to `finish` that the + // others do not. What this case owns is the pair the channel added. + const live = counts(); + expect(live[0]).toBeGreaterThan(before[0]); + expect(live[1]).toBeGreaterThan(before[1]); + + yield* owner.halt(); + + expect(counts()).toEqual([live[0] - 1, live[1] - 1]); + + res.emit("finish"); + + expect(counts()).toEqual([live[0] - 1, live[1] - 1]); + }); +}); + /** The message `submission` fails with; throws if it succeeds instead. */ function* failureOf(server: FormServer): Operation { try { @@ -291,16 +390,154 @@ export interface KeepAlive { * the server failed to destroy would hang teardown outright. The test returning * is the evidence, and `portRefuses` confirms the port went with it. */ -function* openKeepAlive(port: number): Operation { - const socket = connect(port, "127.0.0.1"); - socket.on("error", () => {}); - // Flowing rather than paused, so the connection behaves like a real client's - // and never holds unread bytes against teardown. - socket.resume(); - - const opened = withResolvers(); - socket.once("connect", () => opened.resolve()); - yield* opened.operation; - - return { socket, establishedWith: socket.remoteAddress }; +/** + * Somewhere to open keep-alive connections whose error observers outlive the + * server that destroys them. + * + * The reset a destroyed connection delivers must still reach a listener, and + * it arrives *during* the server's teardown. A connection acquired inside the + * server's own scope would have its observer removed first — destructors run + * in reverse order of registration — and the reset would surface as an + * uncaught error. So the holder is acquired before the server, which is what + * puts its cleanup after the server's. + */ +function useKeepAlives(): Operation<{ + open(port: number): Operation; + count(): number; + listeners(socket: Socket): number; +}> { + return resource(function* (provide) { + const observers = new Map void>(); + + yield* ensure(() => { + for (const [socket, onError] of observers) { + socket.off("error", onError); + } + observers.clear(); + }); + + // Declared here rather than inside `open` below, so the subscription and + // the teardown that walks it belong to the same owner: a nested generator + // is a scope of its own, and it ends long before this resource does. + const observe = (socket: Socket): void => { + const onError = (): void => {}; + + socket.on("error", onError); + observers.set(socket, onError); + }; + + yield* provide({ + *open(port: number): Operation { + const socket = connect(port, "127.0.0.1"); + + observe(socket); + // Flowing rather than paused, so the connection behaves like a real + // client's and never holds unread bytes against teardown. + socket.resume(); + + // Interpreted in the same synchronous run as `connect`, so the event + // cannot land before the wait is attached. + yield* once(socket, "connect"); + + return { socket, establishedWith: socket.remoteAddress }; + }, + count: () => observers.size, + listeners: (socket: Socket) => socket.listenerCount("error"), + }); + }); } + +/** What a raw client's own socket is carrying. */ +function clientCounts(socket: Socket): number[] { + return [ + socket.listenerCount("data"), + socket.listenerCount("error"), + socket.listenerCount("close"), + ]; +} + +describe("form server: accepted sockets and raw clients", () => { + /** + * One close handler per accepted socket, and a raw client's own three, both + * cancelled while they are live. Each count is measured against what the + * runtime was already holding, read once while the owner runs, again after + * it is torn down and before the events are replayed, and once more + * afterwards — together with what the client had received, so a late chunk + * reaching a still-accumulating buffer would show. + */ + it("releases the accepted socket's close handler and the client's own on cancellation", function* () { + const accepted: { socket: Socket; before: number }[] = []; + let client: Socket | undefined; + let clientBefore: number[] = []; + let acceptedLive = 0; + let clientLive: number[] = []; + let received: () => string = () => ""; + let callbacks: () => number = () => 0; + + const owner = yield* spawn(function* () { + const server = yield* useFormServer(formInput(), { + observeSocket: (socket) => accepted.push({ socket, before: socket.listenerCount("close") }), + }); + const { port } = addressOf(server.url); + const connection = yield* useConnection(port, (socket) => { + client = socket; + clientBefore = clientCounts(socket); + }); + + received = () => connection.receivedSoFar(); + callbacks = () => connection.callbacks(); + connection.write(requestText({ method: "GET", path: "/", host: `127.0.0.1:${port}` })); + yield* connection.response(); + + acceptedLive = accepted[0]?.socket.listenerCount("close") ?? 0; + clientLive = client ? clientCounts(client) : []; + yield* suspend(); + }); + + // Synchronized on the exchange having happened, so both owners are live + // with everything attached when the halt lands. + yield* when(function* () { + expect(clientLive.length).toBeGreaterThan(0); + }); + + const first = accepted[0]; + if (!first || !client) { + throw new Error("no connection was accepted"); + } + + // At least what each owner attached, not exactly it: how many handlers a + // runtime keeps on its own sockets, and when it adds them, is the + // runtime's business — Bun on Linux holds one the others do not. What + // these cases own is the pair each added, so the release below is measured + // against what was live rather than against the baseline. + expect(acceptedLive).toBeGreaterThanOrEqual(first.before + 1); + clientLive.forEach((count, index) => { + expect(count).toBeGreaterThanOrEqual(clientBefore[index] + 1); + }); + + // Empty, because the exchange above consumed it — which is what makes the + // replayed chunk below visible if anything is still appending. The + // callback count is not: the exchange ran the client's handlers, and a + // replayed event reaching one would move it again. + const seen = received(); + const ran = callbacks(); + + expect(ran).toBeGreaterThan(0); + + yield* owner.halt(); + + const released = clientLive.map((count) => count - 1); + + expect(first.socket.listenerCount("close")).toBe(acceptedLive - 1); + expect(clientCounts(client)).toEqual(released); + + first.socket.emit("close"); + client.emit("data", Buffer.from("after the client was cancelled")); + client.emit("close"); + + expect(first.socket.listenerCount("close")).toBe(acceptedLive - 1); + expect(clientCounts(client)).toEqual(released); + expect(received()).toBe(seen); + expect(callbacks()).toBe(ran); + }); +}); diff --git a/packages/web/tests/server-support.ts b/packages/web/tests/server-support.ts index 0724eabdc..e8cd9cf2b 100644 --- a/packages/web/tests/server-support.ts +++ b/packages/web/tests/server-support.ts @@ -8,8 +8,9 @@ * point of the refusal paths. */ -import { spawn, withResolvers } from "effection"; +import { race, spawn, withResolvers } from "effection"; import type { Operation } from "effection"; +import { once } from "@effectionx/node/events"; import { connect } from "node:net"; import { compileForm } from "../src/compile.ts"; @@ -90,12 +91,22 @@ export function* watchSubmission(server: FormServer): Operation<() => Submission /** Whether a fresh connection to this port is refused. */ export function* portRefuses(port: number): Operation { const socket = connect(port, "127.0.0.1"); - const settled = withResolvers(); - socket.once("connect", () => settled.resolve(false)); - socket.once("error", () => settled.resolve(true)); - const refused = yield* settled.operation; - socket.destroy(); - return refused; + try { + // Raced inline, in the same synchronous run as `connect`: a spawned race + // attaches its arms a turn later, and the socket can settle in that turn. + return yield* race([ + (function* (): Operation { + yield* once(socket, "connect"); + return false; + })(), + (function* (): Operation { + yield* once(socket, "error"); + return true; + })(), + ]); + } finally { + socket.destroy(); + } } /** A JSON body whose UTF-8 encoding is exactly `bytes` long. */ diff --git a/packages/workflow/src/deno/composition/subprocess.ts b/packages/workflow/src/deno/composition/subprocess.ts index 050e584af..7895602b4 100644 --- a/packages/workflow/src/deno/composition/subprocess.ts +++ b/packages/workflow/src/deno/composition/subprocess.ts @@ -11,6 +11,8 @@ import { ensure, type Operation, withResolvers } from "effection"; import { spawn as spawnChild } from "node:child_process"; +import type { ChildProcessByStdio } from "node:child_process"; +import type { Readable, Writable } from "node:stream"; import process from "node:process"; /** What one invocation reported. A nonzero exit is an answer, not a throw. */ @@ -34,6 +36,18 @@ export interface ProcessInvocation { * to put either one nor a boundary that carries one unchanged. */ readonly input?: string; + /** + * Handed the child as it is spawned, before anything is attached to it, + * together with a reader for what has been captured from it so far. + * + * Package-private, for the cancellation regression: this is the only place a + * baseline can be measured, and the cleanup is already established by the + * time it is called. + */ + readonly observe?: ( + child: ChildProcessByStdio, + captured: () => { stdout: string; stderr: string }, + ) => void; } export function* runProcess({ @@ -42,6 +56,7 @@ export function* runProcess({ cwd, env, input, + observe, }: ProcessInvocation): Operation { // `node:child_process` rather than the runtime's own global: this adapter is // selected by the host, not written against one, and `spawn` replaces the @@ -55,14 +70,56 @@ export function* runProcess({ // waited for below would never arrive and a cancelled operation would hang // instead of tearing down. const options = { cwd, env: { ...env }, detached: true }; - const child = - input === undefined - ? spawnChild(command, [...args], { ...options, stdio: ["ignore", "pipe", "pipe"] }) - : spawnChild(command, [...args], { ...options, stdio: ["pipe", "pipe", "pipe"] }); + // One type for both spawns, so the child's own listeners can be removed + // through it: the two `stdio` shapes differ only in whether standard input is + // a pipe, and TypeScript's overloads make the union's `off` unresolvable. + // + // Declared before the cleanup below and assigned after it, because a child + // that exists before its release is registered can be stranded: `yield* + // ensure(...)` is itself a suspension, and an owner halted while it registers + // unwinds with nothing on it. + let child: ChildProcessByStdio | undefined; + + let settled = false; + // `close`, and nothing else. An assigned `exitCode` or `signalCode` says the + // process ended; it does not say the pipes this operation inherited have. + let closed = false; + let stdout = ""; + let stderr = ""; + // Declared after what it reads, so an observer may call it at once rather + // than only from a later turn. + const captured = () => ({ stdout, stderr }); + + const outcome = withResolvers(); - // Registered with no suspension point between spawning and registering, so a - // halt cannot land between the two and leave a process running after the - // scope that started it is gone. + // A pipe the command stops reading is the command's answer, and its exit + // status is what says so — but the write still fails, and an unhandled + // stream error would take the process down rather than this operation. + // Reporting it here lets `close` settle first when there is an exit to + // report. + const onStdinError = (error: Error): void => { + outcome.reject(error); + }; + const onStdout = (chunk: string): void => { + stdout += chunk; + }; + const onStderr = (chunk: string): void => { + stderr += chunk; + }; + // `close` rather than `exit`: it is the event that fires once both pipes have + // ended, so what is read here is everything the command wrote rather than + // whatever had arrived when it stopped. + const onExit = (code: number | null): void => { + closed = true; + outcome.resolve({ code: code ?? -1, stdout, stderr }); + }; + const onFailure = (error: Error): void => { + outcome.reject(error); + }; + + // Established before anything is attached, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup on it, leaving the process running and every handler in place. // // Teardown waits for the child to close rather than only signalling it. // `kill` returns once the signal is queued, not once it has been delivered, @@ -70,58 +127,74 @@ export function* runProcess({ // finish while the process it started is still alive — and the disposable // directory it is working in is removed moments later. `close` is the event // that fires once the process is gone and both pipes have ended, which is - // what makes cancellation complete rather than merely started. - let settled = false; - const closed = withResolvers(); - child.on("close", () => closed.resolve()); + // what makes cancellation complete rather than merely started. It observes + // that with a listener of its own, so this cleanup depends on nothing the + // body below may not have reached. yield* ensure(function* () { - if (settled) { + if (child === undefined) { return; } - // The group, so the transport helper goes with the command that started it. - // A group that is already gone raises, and that is the same answer as a - // group that was killed — the wait below is what decides either way. + try { - if (child.pid !== undefined) { - process.kill(-child.pid, "SIGKILL"); + if (!settled) { + // The group, so the transport helper goes with the command that + // started it. A group that is already gone raises, and that is the + // same answer as a group that was killed — the wait below is what + // decides either way. + try { + if (child.pid !== undefined) { + process.kill(-child.pid, "SIGKILL"); + } + } catch { + child.kill("SIGKILL"); + } + + // Only `close` ends this wait. The handlers above stay attached + // through it, so whatever the command writes on its way out is still + // captured and a failure still reports. + if (!closed) { + const gone = withResolvers(); + const onGone = (): void => gone.resolve(); + + child.on("close", onGone); + try { + if (!closed) { + yield* gone.operation; + } + } finally { + child.off("close", onGone); + } + } } - } catch { - child.kill("SIGKILL"); + } finally { + // After the wait, and synchronously. Removing a handler that was never + // attached is a no-op, which is what lets this be armed before the child + // exists. + child.off("close", onExit); + child.off("error", onFailure); + child.stdout.off("data", onStdout); + child.stderr.off("data", onStderr); + child.stdin?.off("error", onStdinError); } - yield* closed.operation; }); - const outcome = withResolvers(); + child = + input === undefined + ? spawnChild(command, [...args], { ...options, stdio: ["ignore", "pipe", "pipe"] }) + : spawnChild(command, [...args], { ...options, stdio: ["pipe", "pipe", "pipe"] }); + + observe?.(child, captured); + + child.stdout.setEncoding("utf8"); + child.stderr.setEncoding("utf8"); + child.on("close", onExit); + child.on("error", onFailure); + child.stdout.on("data", onStdout); + child.stderr.on("data", onStderr); if (input !== undefined && child.stdin !== null) { - // A pipe the command stops reading is the command's answer, and its exit - // status is what says so — but the write still fails, and an unhandled - // stream error would take the process down rather than this operation. - // Reporting it here lets `close` settle first when there is an exit to - // report. - child.stdin.on("error", (error: Error) => { - outcome.reject(error); - }); + child.stdin.on("error", onStdinError); child.stdin.end(input, "utf8"); } - let stdout = ""; - let stderr = ""; - child.stdout.setEncoding("utf8"); - child.stderr.setEncoding("utf8"); - child.stdout.on("data", (chunk: string) => { - stdout += chunk; - }); - child.stderr.on("data", (chunk: string) => { - stderr += chunk; - }); - // `close` rather than `exit`: it is the event that fires once both pipes have - // ended, so what is read here is everything the command wrote rather than - // whatever had arrived when it stopped. - child.on("close", (code: number | null) => { - outcome.resolve({ code: code ?? -1, stdout, stderr }); - }); - child.on("error", (error: Error) => { - outcome.reject(error); - }); const result = yield* outcome.operation; settled = true; diff --git a/packages/workflow/tests/ambient-authentication.test.ts b/packages/workflow/tests/ambient-authentication.test.ts index 75d601486..70a2dc3aa 100644 --- a/packages/workflow/tests/ambient-authentication.test.ts +++ b/packages/workflow/tests/ambient-authentication.test.ts @@ -61,6 +61,8 @@ import { remoteBranch, remoteRefs, useBareRemote } from "./support/git-remotes.t import { useGitHttpRemote } from "./support/git-http.ts"; import type { GitHttpRemote } from "./support/git-http.ts"; import { useHomeWithoutAuthentication, useInvokingHome } from "./support/credential-home.ts"; +import { once } from "@effectionx/node/events"; +import { connect } from "node:net"; import { credential, useIssueTrackerServer } from "./support/issue-tracker-server.ts"; import { fixture as pullRequestFixture, @@ -1394,6 +1396,57 @@ describe("workflow GitHub source sessions", () => { // the request carried this process's credential rather than printing it. expect(server.requests[0]?.authorization === `Bearer ${credential()}`).toBe(true); }); + + /** + * A request the fixture is still answering is a task of the server's own + * scope, so tearing the server down ends it. Held on the server side rather + * than timed, and observed through the task's own cleanup, so what is proved + * is that the task was halted rather than that a socket closed. + */ + it("halts an in-flight request task when the server is torn down", function* () { + const reached = withResolvers(); + let cancelled = false; + let answered = false; + + yield* scoped(function* () { + const server = yield* useIssueTrackerServer({ + *hold() { + yield* ensure(() => { + cancelled = true; + }); + reached.resolve(); + yield* suspend(); + }, + }); + const { port } = new URL(server.url); + + const socket = connect(Number(port), "127.0.0.1"); + const onError = (): void => {}; + const onData = (): void => { + answered = true; + }; + + yield* ensure(() => { + socket.off("error", onError); + socket.off("data", onData); + socket.destroy(); + }); + + socket.on("error", onError); + socket.on("data", onData); + + yield* once(socket, "connect"); + socket.write( + `GET /repos/octo/project/issues/7 HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `Authorization: Bearer ${credential()}\r\nConnection: close\r\n\r\n`, + ); + + yield* reached.operation; + }); + + expect(cancelled).toBe(true); + expect(answered).toBe(false); + }); }); describe("workflow GitHub ambient credentials", () => { diff --git a/packages/workflow/tests/credential-helper.test.ts b/packages/workflow/tests/credential-helper.test.ts index 33d97f1e8..3c40530c5 100644 --- a/packages/workflow/tests/credential-helper.test.ts +++ b/packages/workflow/tests/credential-helper.test.ts @@ -17,8 +17,11 @@ */ import { describe, it } from "@executablemd/test-support/bdd"; +import type { ChildProcessByStdio } from "node:child_process"; +import type { Readable, Writable } from "node:stream"; +import { runProcess } from "../src/deno/composition/subprocess.ts"; import { expect } from "@executablemd/test-support/expect"; -import { scoped, until, type Operation } from "effection"; +import { scoped, spawn, until, withResolvers, type Operation } from "effection"; import { chmod, readFile, stat, writeFile } from "node:fs/promises"; import { spawnSync } from "node:child_process"; import { join } from "node:path"; @@ -482,4 +485,101 @@ describe("workflow credential injected infrastructure faults", () => { }); expect(String(raised)).toContain("injected marker-read failure"); }); + /** + * The child a command owns carries its handlers for as long as the scope + * that acquired it runs — `runProcess` registers with `ensure()`, and + * delegation gives a generator no frame of its own. Cancelling that scope + * while the child is still alive is the case the event never comes for, so + * the counts are read while they are live, again after the halt, and once + * more after the events are replayed. + */ + it("releases every handler when the command's owner is cancelled", function* () { + let child: ChildProcessByStdio | undefined; + let before: number[] = []; + let settled = false; + let captured: () => { stdout: string; stderr: string } = () => ({ stdout: "", stderr: "" }); + const spawned = withResolvers(); + + const owner = yield* spawn(function* () { + yield* runProcess({ + command: "sh", + args: ["-c", "sleep 30"], + cwd: ".", + env: {}, + observe: (started, reader) => { + child = started; + before = attachedToChild(started); + captured = reader; + spawned.resolve(); + }, + }); + settled = true; + }); + + // Resumed from inside the operation's own synchronous prefix, so the halt + // below lands at its very first suspension — the boundary at which an + // `ensure()` yielded after the subscriptions would not yet have been + // established. The cleanup here is armed before the child is reachable, so + // this is precisely where it has to hold. + yield* spawned.operation; + if (!child) { + throw new Error("the command never owned a child"); + } + + // One `close`, one `error`, and one on each output pipe — the reap keeps no + // standing listener of its own. Nothing on stdin, which this invocation + // left closed. + const mine = [1, 1, 1, 1, 0]; + const live = attachedToChild(child); + + live.forEach((count, index) => { + expect(count).toBeGreaterThanOrEqual(before[index] + mine[index]); + }); + + const seen = captured(); + + yield* owner.halt(); + + const released = live.map((count, index) => count - mine[index]); + + expect(attachedToChild(child)).toEqual(released); + + // Delivered through a sentinel, because an `error` an emitter has no + // listener for is thrown rather than dropped — so the replay needs one + // observer, and exactly one is what it must find. + let reached = 0; + const sentinel = (): void => { + reached += 1; + }; + + child.emit("close", 0, null); + child.stdout.emit("data", "after the owner was cancelled"); + child.stderr.emit("data", "after the owner was cancelled"); + child.on("error", sentinel); + try { + child.emit("error", new Error("after the owner was cancelled")); + } finally { + child.off("error", sentinel); + } + + expect(reached).toBe(1); + expect(attachedToChild(child)).toEqual(released); + // Nothing is still accumulating, and the cancelled operation produced no + // result that a later event could have resumed. + expect(captured()).toEqual(seen); + expect(settled).toBe(false); + }); }); + +/** What one owned child is carrying, across itself and its pipes. */ +function attachedToChild( + child: ChildProcessByStdio, +): number[] { + return [ + child.listenerCount("error"), + child.listenerCount("close"), + child.stdout.listenerCount("data"), + child.stderr.listenerCount("data"), + child.stdin?.listenerCount("error") ?? 0, + ]; +} diff --git a/packages/workflow/tests/git-push-crash.test.ts b/packages/workflow/tests/git-push-crash.test.ts index 75e8d7459..8f0e5b05e 100644 --- a/packages/workflow/tests/git-push-crash.test.ts +++ b/packages/workflow/tests/git-push-crash.test.ts @@ -18,11 +18,24 @@ import { describe, it } from "@executablemd/test-support/bdd"; import { expect } from "@executablemd/test-support/expect"; +import { once } from "@effectionx/node/events"; +import { Buffer } from "node:buffer"; +import { connect } from "node:net"; +import type { ChildProcess } from "node:child_process"; import process from "node:process"; import { DatabaseSync } from "node:sqlite"; import { fileURLToPath } from "node:url"; import { exec as execProcess } from "@effectionx/process"; -import { call, type Operation, race, scoped, spawn, withResolvers } from "effection"; +import { + call, + ensure, + type Operation, + race, + scoped, + spawn, + suspend, + withResolvers, +} from "effection"; import { WorkflowRunStorage } from "../mod.ts"; import { GIT_HOST_EFFECT } from "../src/git-host/effect.ts"; import { parseGitHostReconciliationRecord } from "../src/git-host/records.ts"; @@ -45,6 +58,17 @@ import { denoRepositoryHost } from "../src/deno/composition/host.ts"; import { denoGitAuthentication } from "../src/deno/composition/authentication.ts"; import { TEST_HELPER } from "./support/composition.ts"; +/** What one backend child is carrying on the streams the fixture observes. */ +function attached(child: ChildProcess): number[] { + return [ + child.listenerCount("error"), + child.listenerCount("close"), + child.stdout?.listenerCount("data") ?? 0, + child.stdout?.listenerCount("end") ?? 0, + child.stdin?.listenerCount("error") ?? 0, + ]; +} + const REPOSITORY = fileURLToPath(new URL("../../..", import.meta.url)); const CRASH_CHILD = fileURLToPath(new URL("./support/git-crash-child.ts", import.meta.url)); @@ -187,11 +211,17 @@ describe("workflow Git.Push across a process boundary", () => { it("adopts a protected publication under authentication it acquired afresh", function* () { const root = yield* useStorageRoot(); const bare = yield* useBareRemote(REMOTE); + // Every backend the fixture spawns, with what its streams were carrying + // before the request task attached anything — the runtime keeps listeners + // of its own on a child's stdout, so the baseline is measured rather than + // assumed to be nothing. + const backends: { child: ChildProcess; before: number[] }[] = []; const served = yield* useGitHttpRemote({ remote: bare, label: "protected", username: "crash-user", password: "crash-secret", + observeBackend: (child) => backends.push({ child, before: attached(child) }), }); const crashHome = yield* useInvokingHome([ { host: served.host, path: "remote.git", username: "crash-user", password: "crash-secret" }, @@ -345,5 +375,113 @@ describe("workflow Git.Push across a process boundary", () => { expect(remoteBranch(bare, PUSH_BRANCH)).toBe(pushedCommit); }); + + // Each backend belonged to the request task that spawned it, and that task + // has ended. The counts are read before the events are replayed, because a + // handler removed by its own event would leave the same counts behind as + // one the task released. + expect(backends.length).toBeGreaterThan(0); + for (const { child, before } of backends) { + expect(attached(child)).toEqual(before); + } + + // And nothing the fixture kept can still be reached through them. + const answered = served.requests.length; + for (const { child } of backends) { + child.emit("close", 0, null); + child.stdout?.emit("data", Buffer.from("late")); + child.stdout?.emit("end"); + } + + expect(served.requests.length).toBe(answered); + for (const { child, before } of backends) { + expect(attached(child)).toEqual(before); + } + }); + /** + * A backend the fixture is still talking to belongs to the request task that + * spawned it, and that task belongs to the server. Held open on the server + * side rather than timed, so the teardown below lands while the child is + * alive and every handler is attached: what it must prove is that the task + * killed and reaped the child and released all five before the events are + * replayed. + */ + it("kills and releases a backend the server was still talking to", function* () { + const bare = yield* useBareRemote(REMOTE); + const held = withResolvers(); + const backends: { child: ChildProcess; before: number[] }[] = []; + let live: number[] = []; + let answered = false; + + yield* scoped(function* () { + const served = yield* useGitHttpRemote({ + remote: bare, + label: "held", + username: "held-user", + password: "held-secret", + observeBackend: (child) => backends.push({ child, before: attached(child) }), + *holdBackend() { + held.resolve(); + yield* suspend(); + }, + }); + + const [host, port] = served.host.split(":"); + const name = served.locator.slice(served.locator.lastIndexOf("/") + 1); + const socket = connect(Number(port), host ?? "127.0.0.1"); + const onError = (): void => {}; + const onData = (): void => { + answered = true; + }; + + yield* ensure(() => { + socket.off("error", onError); + socket.off("data", onData); + socket.destroy(); + }); + + socket.on("error", onError); + socket.on("data", onData); + + yield* once(socket, "connect"); + const authorization = Buffer.from("held-user:held-secret").toString("base64"); + socket.write( + `GET /${name}/info/refs?service=git-upload-pack HTTP/1.1\r\n` + + `Host: ${served.host}\r\nAuthorization: Basic ${authorization}\r\n` + + `Connection: close\r\n\r\n`, + ); + + yield* held.operation; + + const observed = backends[0]; + if (!observed) { + throw new Error("the fixture spawned no backend"); + } + live = attached(observed.child); + }); + + const first = backends[0]; + if (!first) { + throw new Error("the fixture spawned no backend"); + } + + // Five: `error` and `close` on the child, `data` and `end` on its stdout, + // and `error` on its stdin — each on top of what the runtime already held. + live.forEach((count, index) => { + expect(count).toBeGreaterThanOrEqual(first.before[index] + 1); + }); + + // The task killed it and waited for it to be gone before letting go. + expect(first.child.exitCode !== null || first.child.signalCode !== null).toBe(true); + const released = live.map((count) => count - 1); + + expect(attached(first.child)).toEqual(released); + + first.child.emit("close", 0, null); + first.child.stdout?.emit("data", Buffer.from("after the server was torn down")); + + expect(attached(first.child)).toEqual(released); + // And the client was never answered, because the request never finished. + expect(answered).toBe(false); }); }); diff --git a/packages/workflow/tests/support/git-http.ts b/packages/workflow/tests/support/git-http.ts index a68fc8ede..28f81f862 100644 --- a/packages/workflow/tests/support/git-http.ts +++ b/packages/workflow/tests/support/git-http.ts @@ -14,11 +14,22 @@ */ import { spawn, spawnSync } from "node:child_process"; +import type { ChildProcess, ChildProcessByStdio } from "node:child_process"; +import type { Readable, Writable } from "node:stream"; import { Buffer } from "node:buffer"; import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; +import type { Socket } from "node:net"; import { dirname } from "node:path"; import process from "node:process"; -import { ensure, type Operation, resource, until } from "effection"; +import { + ensure, + type Operation, + resource, + scoped, + until, + useScope, + withResolvers, +} from "effection"; import { git, type BareRemote } from "./git-remotes.ts"; /** One request this remote received, as a suite asserts on it. */ @@ -105,6 +116,23 @@ export interface GitHttpOptions { * authentication cleanup that must follow it. */ readonly closed?: () => void; + /** + * Handed every backend child as it is spawned. + * + * Package-private, for the cancellation regression: the listeners this + * fixture installs are on a process it owns and does not otherwise hand out, + * and their release is the thing under test. + */ + readonly observeBackend?: (child: ChildProcess) => void; + /** + * Run inside the request task, after the backend's cleanup is registered and + * before its input is forwarded. + * + * Package-private, for the cancellation regression: a backend is only + * observable while its request is still running, and this is what holds one + * there. + */ + readonly holdBackend?: () => Operation; } /** Where Git keeps `git-http-backend`, asked of Git rather than guessed. */ @@ -167,7 +195,16 @@ function headerEnd(buffered: Buffer): { at: number; width: number } | undefined return plain < 0 ? undefined : { at: plain, width: 2 }; } -/** Hand one request to `git-http-backend` and its answer back to the client. */ +/** + * Hand one request to `git-http-backend` and its answer back to the client, as + * a task of the server's own scope. + * + * The backend is a child process with four listeners on it, and both belong to + * this request rather than to the fixture: cancelling the server ends the + * request, which kills the backend, waits for it to be gone, and detaches + * every handler synchronously afterwards. A request that simply finishes takes + * the same path. + */ function serve( incoming: IncomingMessage, outgoing: ServerResponse, @@ -176,63 +213,129 @@ function serve( user: string, segment: string, directory: string, -): void { - const child = spawn(backend, [], { - env: cgiEnvironment(incoming, root, user, segment, directory), - stdio: ["pipe", "pipe", "pipe"], - }); - // A client that hung up mid-body closes this pipe under the backend. It is - // the client's answer rather than a fault of this fixture's, and an unhandled - // stream error here would take the whole suite process down. - child.stdin.on("error", () => {}); - incoming.pipe(child.stdin); + observe?: (child: ChildProcess) => void, + hold?: () => Operation, +): Operation { + return scoped(function* () { + // Declared before the cleanup below and assigned after it: a backend that + // exists before its release is registered can be stranded, because `yield* + // ensure(...)` is itself a suspension and an owner halted while it + // registers unwinds with nothing on it. + let child: ChildProcessByStdio | undefined; - let buffered = Buffer.alloc(0); - let started = false; - child.stdout.on("data", (chunk: Buffer) => { - if (started) { - outgoing.write(chunk); - return; - } - buffered = Buffer.concat([buffered, chunk]); - const end = headerEnd(buffered); - if (end === undefined) { - return; - } - let status = 200; - const headers: Record = {}; - for (const line of buffered.subarray(0, end.at).toString("utf8").split(/\r?\n/)) { - const separator = line.indexOf(":"); - if (separator < 0) { - continue; + let buffered = Buffer.alloc(0); + let started = false; + const answered = withResolvers(); + const closed = withResolvers(); + // `close`, and nothing else, is what says the backend and its pipes are + // done; an assigned exit status says only that the process ended. + let finished = false; + + // A client that hung up mid-body closes this pipe under the backend. It is + // the client's answer rather than a fault of this fixture's, and an + // unhandled stream error here would take the whole suite process down. + const onStdinError = (): void => {}; + const onStdout = (chunk: Buffer): void => { + if (started) { + outgoing.write(chunk); + return; } - const name = line.slice(0, separator).trim(); - const value = line.slice(separator + 1).trim(); - if (name.toLowerCase() === "status") { - status = Number.parseInt(value, 10) || 200; - } else { - headers[name] = value; + buffered = Buffer.concat([buffered, chunk]); + const end = headerEnd(buffered); + if (end === undefined) { + return; } - } - outgoing.writeHead(status, headers); - started = true; - const rest = buffered.subarray(end.at + end.width); - if (rest.length > 0) { - outgoing.write(rest); - } - }); - child.stdout.on("end", () => { - if (!started) { - outgoing.writeHead(500); - } - outgoing.end(); - }); - child.on("error", () => { - if (!started) { - outgoing.writeHead(500); + let status = 200; + const headers: Record = {}; + for (const line of buffered.subarray(0, end.at).toString("utf8").split(/\r?\n/)) { + const separator = line.indexOf(":"); + if (separator < 0) { + continue; + } + const name = line.slice(0, separator).trim(); + const value = line.slice(separator + 1).trim(); + if (name.toLowerCase() === "status") { + status = Number.parseInt(value, 10) || 200; + } else { + headers[name] = value; + } + } + outgoing.writeHead(status, headers); started = true; + const rest = buffered.subarray(end.at + end.width); + if (rest.length > 0) { + outgoing.write(rest); + } + }; + const onStdoutEnd = (): void => { + if (!started) { + outgoing.writeHead(500); + } + outgoing.end(); + answered.resolve(); + }; + const onChildError = (): void => { + if (!started) { + outgoing.writeHead(500); + started = true; + } + outgoing.end(); + answered.resolve(); + }; + const onClose = (): void => { + finished = true; + closed.resolve(); + }; + + // Established before the backend exists, so a request cancelled anywhere + // below still ends the process it started — and so that no instant exists + // in which a backend is running with no cleanup registered for it. + // + // Teardown keeps every handler attached through the close wait, so the + // answer this fixture was streaming is still being written while the + // backend ends. They come off, and the request is unpiped, synchronously + // once that wait has settled. + yield* ensure(function* () { + if (child === undefined) { + return; + } + + try { + child.kill("SIGKILL"); + + if (!finished) { + yield* closed.operation; + } + } finally { + incoming.unpipe(child.stdin); + child.stdin.off("error", onStdinError); + child.stdout.off("data", onStdout); + child.stdout.off("end", onStdoutEnd); + child.off("error", onChildError); + child.off("close", onClose); + } + }); + + child = spawn(backend, [], { + env: cgiEnvironment(incoming, root, user, segment, directory), + stdio: ["pipe", "pipe", "pipe"], + }); + + observe?.(child); + + child.stdin.on("error", onStdinError); + child.stdout.on("data", onStdout); + child.stdout.on("end", onStdoutEnd); + child.on("error", onChildError); + child.on("close", onClose); + + if (hold) { + yield* hold(); } - outgoing.end(); + + incoming.pipe(child.stdin); + + yield* answered.operation; }); } @@ -283,6 +386,10 @@ export function useGitHttpRemote(options: GitHttpOptions): Operation void>(); const server = createServer((incoming: IncomingMessage, outgoing: ServerResponse) => { const url = new URL(incoming.url ?? "/", "http://127.0.0.1"); const header = incoming.headers.authorization; @@ -310,8 +417,15 @@ export function useGitHttpRemote(options: GitHttpOptions): Operation options.closed?.()); + // is where that is observed. The handler is remembered so teardown can + // take it off before it destroys the connection it is attached to. + const held = incoming.socket; + const onHeldClose = (): void => { + holds.delete(held); + options.closed?.(); + }; + holds.set(held, onHeldClose); + held.on("close", onHeldClose); incoming.resume(); return; } @@ -323,11 +437,29 @@ export function useGitHttpRemote(options: GitHttpOptions): Operation + serve( + incoming, + outgoing, + backend, + entry.root, + entry.user, + segment, + entry.directory, + options.observeBackend, + options.holdBackend, + ), + ); }); yield* until(new Promise((resolve) => server.listen(0, "127.0.0.1", () => resolve()))); yield* ensure(function* () { + // Before the first suspension below, so a teardown halted part-way + // leaves no handler on a connection this server is finished with. + for (const [socket, onHeldClose] of holds) { + socket.off("close", onHeldClose); + } + holds.clear(); server.closeAllConnections(); yield* until(new Promise((resolve) => server.close(() => resolve()))); }); diff --git a/packages/workflow/tests/support/github.ts b/packages/workflow/tests/support/github.ts index 469325040..8ecb41251 100644 --- a/packages/workflow/tests/support/github.ts +++ b/packages/workflow/tests/support/github.ts @@ -18,7 +18,8 @@ import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; import type { AddressInfo } from "node:net"; -import { ensure, type Operation, resource, until } from "effection"; +import { each, ensure, type Operation, resource, until, useScope } from "effection"; +import { fromReadable } from "@effectionx/node"; import type { GitHubAccess, GitHubHttpRequest, @@ -651,10 +652,20 @@ export function mutations(store: GitHubStore): string[] { */ export function useGitHubServer(store: GitHubStore): Operation { return resource(function* (provide) { + // Each request body is read by a task of this server's own scope, so + // tearing the server down ends the reads still in progress rather than + // leaving their listeners on sockets it is about to destroy. `fromReadable` + // is the scope-bound adapter for that: it attaches and detaches the + // stream's own handlers with the task. + const scope = yield* useScope(); const server = createServer((incoming: IncomingMessage, outgoing: ServerResponse) => { - const chunks: Buffer[] = []; - incoming.on("data", (chunk: Buffer) => chunks.push(chunk)); - incoming.on("end", () => { + scope.run(function* () { + const chunks: Uint8Array[] = []; + for (const chunk of yield* each(fromReadable(incoming))) { + chunks.push(chunk); + yield* each.next(); + } + const headers: Record = {}; for (const [name, value] of Object.entries(incoming.headers)) { headers[name === "authorization" ? "Authorization" : name] = String(value); diff --git a/packages/workflow/tests/support/issue-tracker-server.ts b/packages/workflow/tests/support/issue-tracker-server.ts index 54cabb51d..720182fd2 100644 --- a/packages/workflow/tests/support/issue-tracker-server.ts +++ b/packages/workflow/tests/support/issue-tracker-server.ts @@ -13,7 +13,8 @@ */ import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; -import { ensure, type Operation, resource, until } from "effection"; +import { each, ensure, type Operation, resource, until, useScope } from "effection"; +import { fromReadable } from "@effectionx/node"; /** * The credential this tracker requires, held here and never in a document. @@ -93,6 +94,14 @@ export interface ServerOptions { readonly issues?: readonly ServedIssue[]; /** The credential every request must carry, so a scenario can prove one was. */ readonly token?: string; + /** + * Run inside the request task, after the body is read and before the answer + * is written. + * + * Package-private, for the cancellation regression: a request task is only + * observable while it is still running, and this is what holds one there. + */ + readonly hold?: () => Operation; } /** @@ -112,10 +121,24 @@ export function useIssueTrackerServer(options: ServerOptions = {}): Operation(); let origin = ""; + // Each request body is read by a task of this server's own scope, so + // tearing the server down ends the reads still in progress rather than + // leaving their listeners on sockets it is about to destroy. `fromReadable` + // is the scope-bound adapter for that: it attaches and detaches the + // stream's own handlers with the task. + const scope = yield* useScope(); const server = createServer((incoming: IncomingMessage, outgoing: ServerResponse) => { - const chunks: Buffer[] = []; - incoming.on("data", (chunk: Buffer) => chunks.push(chunk)); - incoming.on("end", () => { + scope.run(function* () { + const chunks: Uint8Array[] = []; + for (const chunk of yield* each(fromReadable(incoming))) { + chunks.push(chunk); + yield* each.next(); + } + + if (options.hold) { + yield* options.hold(); + } + const raw = Buffer.concat(chunks).toString("utf8"); const url = new URL(incoming.url ?? "/", origin); let body: unknown; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 81786c4b8..5ea97ff75 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -27,8 +27,8 @@ importers: specifier: 0.1.1 version: 0.1.1 '@effectionx/node': - specifier: 0.2.4 - version: 0.2.4(effection@4.1.0) + specifier: 0.2.5 + version: 0.2.5(effection@4.1.0) '@effectionx/process': specifier: 0.8.1 version: 0.8.1(effection@4.1.0) @@ -222,8 +222,8 @@ importers: specifier: 0.1.1 version: 0.1.1 '@effectionx/node': - specifier: 0.2.4 - version: 0.2.4(effection@4.1.0) + specifier: 0.2.5 + version: 0.2.5(effection@4.1.0) '@effectionx/process': specifier: 0.8.1 version: 0.8.1(effection@4.1.0) @@ -306,8 +306,8 @@ importers: specifier: 0.3.0 version: 0.3.0(effection@4.1.0) '@effectionx/node': - specifier: 0.2.4 - version: 0.2.4(effection@4.1.0) + specifier: 0.2.5 + version: 0.2.5(effection@4.1.0) '@effectionx/process': specifier: 0.8.1 version: 0.8.1(effection@4.1.0) @@ -321,8 +321,8 @@ importers: specifier: 1.3.0 version: 1.3.0(zod@4.3.6) '@effectionx/node': - specifier: 0.2.4 - version: 0.2.4(effection@4.1.0) + specifier: 0.2.5 + version: 0.2.5(effection@4.1.0) '@effectionx/scope-eval': specifier: 0.1.3 version: 0.1.3(effection@4.1.0) @@ -408,8 +408,8 @@ importers: packages/web: dependencies: '@effectionx/node': - specifier: 0.2.4 - version: 0.2.4(effection@4.1.0) + specifier: 0.2.5 + version: 0.2.5(effection@4.1.0) '@executablemd/core': specifier: workspace:* version: link:../core @@ -566,6 +566,11 @@ packages: peerDependencies: effection: ^3 || ^4 + '@effectionx/node@0.2.5': + resolution: {integrity: sha512-hL8mROda8Lx375MVS+Ubu86+yMht/I0wOZG5VR6Pel0XUA5ReObQDYvNS6ocW0cNFKnEmvlVLWGWbcjJ+VkVhA==} + peerDependencies: + effection: ^3 || ^4 + '@effectionx/process@0.8.1': resolution: {integrity: sha512-xyXlFja0Ill80lQ3IYfksXtJkqVmWuUOogRn/qlHWCAGlZj+MGGF8gOFbyzk/3Kx4pj14riVGgF/cyT5XCzqDw==} peerDependencies: @@ -2733,6 +2738,10 @@ snapshots: dependencies: effection: 4.1.0 + '@effectionx/node@0.2.5(effection@4.1.0)': + dependencies: + effection: 4.1.0 + '@effectionx/process@0.8.1(effection@4.1.0)': dependencies: '@effectionx/context-api': 0.6.0(effection@4.1.0) diff --git a/scripts/oxlint-plugin.js b/scripts/oxlint-plugin.js index c034db388..1ee11cd75 100644 --- a/scripts/oxlint-plugin.js +++ b/scripts/oxlint-plugin.js @@ -5,6 +5,7 @@ import { noSyncFilesystem } from "./oxlint-rules/no-sync-filesystem.js"; import { noYieldInFinally } from "./oxlint-rules/no-yield-in-finally.js"; import { preferEffectionOperation } from "./oxlint-rules/prefer-effection-operation.js"; import { preferEffectionResult } from "./oxlint-rules/prefer-effection-result.js"; +import { requireScopeBoundEventRegistration } from "./oxlint-rules/require-scope-bound-event-registration.js"; export default { meta: { name: "executablemd" }, @@ -16,5 +17,6 @@ export default { "no-yield-in-finally": noYieldInFinally, "prefer-effection-operation": preferEffectionOperation, "prefer-effection-result": preferEffectionResult, + "require-scope-bound-event-registration": requireScopeBoundEventRegistration, }, }; diff --git a/scripts/oxlint-rules/require-scope-bound-event-registration.js b/scripts/oxlint-rules/require-scope-bound-event-registration.js new file mode 100644 index 000000000..1c16909b2 --- /dev/null +++ b/scripts/oxlint-rules/require-scope-bound-event-registration.js @@ -0,0 +1,1279 @@ +/** + * `local/require-scope-bound-event-registration` — a listener outlives nothing. + * + * An event listener an Effection operation installs is state that operation + * owns, and the architecture's ownership contract says owned state is released + * when the owner is. Nothing about an event source enforces that. A handler + * attached to a socket, a child process or a DOM target stays attached after + * the operation that attached it has returned, failed, been halted, or lost a + * `race()`; the source keeps calling it, and it keeps writing to state the + * abandoned owner left behind. + * + * Cleanup that runs when the event fires is not cleanup. A cancelled wait is + * precisely the case where the event never arrives, so `emitter.once()` and + * `addEventListener(..., { once: true })` are reported wherever an Effection + * owner reaches them. A one-event wait uses the scope-bound helper instead: + * + * const [event] = yield* once(socket, "close"); + * + * A subscription that outlives one event binds a stable handler and removes + * that same handler, from that same receiver and event, in the owner's own + * teardown. Removal is synchronous, so it belongs in one of four places: the + * `finally` of a `try` around the subscription, an `ensure()` established + * before the owner can suspend, a synchronous `finally` inside an `ensure()` + * whose wait needs the listener still attached, or the cleanup an `action()` + * executor returns. Node removes with `.off()`; the DOM removes with + * `.removeEventListener()`, matching capture mode included. + * + * Recognition is by binding rather than by spelling, because `on` and `once` + * are ordinary method names. What the rule resolves is: + * + * - values and types imported from `node:events`, `node:stream`, + * `node:child_process`, `node:net`, `node:http`, `node:https` and + * `node:process`; + * - a binding holding what one of those constructors or factories returned, + * and the `stdin`, `stdout`, `stderr` and `socket` members of such a binding; + * - a parameter or property annotated with one of those imported types; + * - a class extending a recognized emitter, and `this` inside it; + * - a local interface that declares a paired `on`/`off` or + * `addEventListener`/`removeEventListener` surface; + * - `process` and its streams while they still name the host global; and + * - DOM targets reached through `XMLHttpRequest`, `Worker`, `EventTarget`, + * `AbortController`/`AbortSignal`, `document`, `window`, `self` and + * `globalThis`. + * + * An owner is a generator function, or the executor of `action()` imported + * from `effection`. A callback declared inside one inherits it, because it + * closes over that scope. Nothing else is an owner: a standalone fixture + * process, a browser-lifetime callback and a plain Promise helper install + * listeners for a lifetime Effection does not manage, and are accepted. A + * helper that ought to be scope-bound is refactored into an operation rather + * than left as a plain function to escape this rule. + * + * There is no autofix. Choosing the owner, the handler binding and the order + * of teardown changes what the program does when it is cancelled, which is the + * decision this rule exists to make visible. + */ + +/** Node modules whose event sources the rule recognizes, by specifier. */ +const NODE_MODULES = new Map([ + ["events", { values: ["EventEmitter"], types: ["EventEmitter"] }], + [ + "stream", + { + values: ["Readable", "Writable", "Duplex", "Transform", "PassThrough", "Stream"], + types: ["Readable", "Writable", "Duplex", "Transform", "PassThrough", "Stream"], + }, + ], + [ + "child_process", + { + values: ["spawn", "exec", "execFile", "fork", "ChildProcess"], + types: ["ChildProcess", "ChildProcessWithoutNullStreams", "ChildProcessByStdio"], + }, + ], + [ + "net", + { + values: ["connect", "createConnection", "createServer", "Socket", "Server"], + types: ["Socket", "Server"], + }, + ], + [ + "http", + { + values: ["createServer", "request", "get", "Server", "ClientRequest"], + types: ["Server", "IncomingMessage", "ServerResponse", "ClientRequest"], + }, + ], + [ + "https", + { + values: ["createServer", "request", "get", "Server"], + types: ["Server", "IncomingMessage", "ServerResponse"], + }, + ], + ["process", { values: [], types: [] }], + ["worker_threads", { values: ["Worker", "MessagePort"], types: ["Worker", "MessagePort"] }], +]); + +/** DOM constructors whose instances are event targets. */ +const DOM_CONSTRUCTORS = new Set([ + "AbortController", + "EventSource", + "EventTarget", + "MessageChannel", + "WebSocket", + "Worker", + "XMLHttpRequest", +]); + +/** DOM globals that are event targets in their own right. */ +const DOM_GLOBALS = new Set(["document", "globalThis", "self", "window"]); + +/** DOM types a parameter or property may be annotated with. */ +const DOM_TYPES = new Set([ + "AbortSignal", + "EventSource", + "EventTarget", + "MessagePort", + "WebSocket", + "Worker", + "XMLHttpRequest", +]); + +/** Members of a recognized source that are themselves event sources. */ +const SOURCE_MEMBERS = new Set(["stdin", "stdout", "stderr", "socket", "signal", "port1", "port2"]); + +const REGISTER = new Set(["on", "once", "addEventListener"]); +const REMOVE = new Set(["off", "removeEventListener", "removeListener"]); + +/** Nodes that stop a search for suspensions belonging to one owner. */ +const FUNCTIONS = new Set(["FunctionDeclaration", "FunctionExpression", "ArrowFunctionExpression"]); + +/** The module a specifier names, with any `node:` prefix removed. */ +function moduleOf(source) { + return typeof source === "string" ? source.replace(/^node:/u, "") : ""; +} + +/** The property a member expression reads, whether named or spelled out. */ +function memberName(node) { + if (node.type !== "MemberExpression") { + return null; + } + + if (!node.computed) { + return node.property.type === "Identifier" ? node.property.name : null; + } + + return typeof node.property.value === "string" ? node.property.value : null; +} + +/** The names a type annotation refers to, for the simple forms the rule reads. */ +function typeNames(annotation) { + const reference = annotation?.typeAnnotation ?? annotation; + + if (reference?.type === "TSUnionType") { + return reference.types.flatMap((member) => typeNames(member)); + } + + const name = typeName(reference); + + return name === null ? [] : [name]; +} + +/** The name a single type reference refers to. */ +function typeName(annotation) { + const reference = annotation?.typeAnnotation ?? annotation; + + if (!reference) { + return null; + } + + if (reference.type === "TSTypeReference") { + const name = reference.typeName; + if (name?.type === "Identifier") { + return name.name; + } + if (name?.type === "TSQualifiedName" && name.right?.type === "Identifier") { + return name.right.name; + } + } + + return null; +} + +function isStringLiteral(node) { + return node?.type === "Literal" && typeof node.value === "string"; +} + +/** The name a property declares, whether written plainly or as a string. */ +function propertyName(property) { + if (property.type !== "Property") { + return null; + } + + if (!property.computed && property.key?.type === "Identifier") { + return property.key.name; + } + + return isStringLiteral(property.key) ? property.key.value : null; +} + +/** + * How a listener options object spells a boolean member, when it says so + * statically. `{ "once": true }` and `{ once: ONCE }` for a `const ONCE = true` + * are the same registration as `{ once: true }`. + */ +function staticFlag(node, name, resolve) { + if (node?.type !== "ObjectExpression") { + return undefined; + } + + const property = node.properties.find((entry) => propertyName(entry) === name); + + if (property === undefined) { + return undefined; + } + + const value = property.value; + + if (value?.type === "Literal" && typeof value.value === "boolean") { + return value.value; + } + + return resolve(value); +} + +/** + * How a listener argument list spells its capture mode. `{ capture: true }` + * and a bare `true` are the same registration, so both normalize to the same + * value and a removal has to agree with it. + */ +function captureOf(node, resolve) { + if (node === undefined) { + return false; + } + + if (node.type === "Literal" && typeof node.value === "boolean") { + return node.value; + } + + return staticFlag(node, "capture", resolve) === true; +} + +export const requireScopeBoundEventRegistration = { + meta: { + type: "problem", + messages: { + rawOnce: + '{{receiver}}.once("{{event}}") removes its listener only when the event arrives, which a cancelled wait never gets. Wait with the scope-bound once() from @effectionx/node/events.', + onceOption: + 'addEventListener("{{event}}", …, { once: true }) removes its listener only when the event arrives, which a cancelled wait never gets. Wait with the scope-bound once() from @effectionx/node/events.', + anonymous: + 'This listener on "{{event}}" has no name, so nothing can remove it. Bind the handler and remove that binding in the owner\'s teardown.', + dynamicEvent: + "This listener's event name is computed, so the pairing with its removal cannot be established. Register each event under a literal name.", + missing: + 'Nothing removes {{handler}} from {{receiver}} "{{event}}" when this operation ends. Remove it in a finally around the subscription, an ensure() established before the owner suspends, a synchronous finally inside that ensure(), or the cleanup returned by action().', + mismatched: + 'The cleanup for {{receiver}} "{{event}}" does not name the same receiver, event, handler and capture mode this registration used, so the handler stays attached. Remove {{handler}} from {{receiver}} "{{event}}".', + selfRemoving: + '{{handler}} removes itself only when "{{event}}" arrives, which a cancelled owner never gets. Remove it in the owner\'s teardown as well.', + late: "This cleanup is registered after the owner can already suspend, so a failure in between leaves {{handler}} attached to {{receiver}}. Establish it in the same synchronous prefix as the subscription.", + unarmed: + "This ensure() is yielded after {{handler}} was attached, and entering it is itself a suspension: an owner halted there unwinds with no cleanup registered at all, leaving {{handler}} on {{receiver}}. Establish the ensure() before the subscription, or put both inside a try whose finally removes it.", + suspended: + "Teardown suspends before it removes {{handler}} from {{receiver}}, so a halt during that wait leaves the handler attached. Remove it in a synchronous finally around the wait.", + removeListener: + "removeListener() is not the removal this rule pairs with. Use .off() so the subscription and its cleanup read as one pair.", + }, + }, + + create(context) { + const source = context.sourceCode; + const text = source.text; + + /** Imported bindings, by local name, that name a recognized module value. */ + const values = new Map(); + /** Imported bindings, by local name, that name a recognized module type. */ + const types = new Map(); + /** Local interfaces whose declared surface is a paired event source. */ + const structural = new Set(); + /** Classes extending a recognized emitter, by declaration range. */ + const emitterClasses = []; + const registrations = []; + const removals = []; + /** `collection.set(receiver, handler)` calls, which pair a source with its handler. */ + const records = []; + const reports = []; + + function variableOf(node) { + for (let scope = source.getScope(node); scope; scope = scope.upper) { + const found = scope.variables.find((variable) => variable.name === node.name); + + if (found) { + return found; + } + } + + return null; + } + + /** Whether an identifier still names what the host put there. */ + function isGlobal(node) { + if (node.type !== "Identifier") { + return false; + } + + const variable = variableOf(node); + return variable === null || variable.defs.length === 0; + } + + /** The declaration an identifier resolves to, or null. */ + function definitionOf(node) { + if (node.type !== "Identifier") { + return null; + } + + const variable = variableOf(node); + return variable?.defs?.[0] ?? null; + } + + /** Whether an identifier resolves to `name` imported from `module`. */ + function importsFrom(node, module, name) { + const definition = definitionOf(node); + + if (definition?.type !== "ImportBinding") { + return false; + } + + const declaration = definition.parent; + + if (declaration.importKind === "type" || definition.node.importKind === "type") { + return false; + } + + return ( + moduleOf(declaration.source.value) === module && + definition.node.type === "ImportSpecifier" && + definition.node.imported.name === name + ); + } + + function isProcessGlobal(node) { + if (isGlobal(node) && node.name === "process") { + return true; + } + + const definition = definitionOf(node); + + return ( + definition?.type === "ImportBinding" && + moduleOf(definition.parent.source.value) === "process" && + definition.node.type === "ImportDefaultSpecifier" + ); + } + + /** Whether a callee names a constructor or factory of a Node event source. */ + function isNodeFactory(node) { + if (node.type === "MemberExpression") { + const name = memberName(node); + const object = node.object; + + if (object.type !== "Identifier" || name === null) { + return false; + } + + const definition = definitionOf(object); + + if (definition?.type !== "ImportBinding") { + return false; + } + + const module = moduleOf(definition.parent.source.value); + const recognized = NODE_MODULES.get(module); + + return Boolean(recognized?.values.includes(name)); + } + + if (node.type !== "Identifier") { + return false; + } + + const definition = definitionOf(node); + + if (definition?.type !== "ImportBinding") { + return false; + } + + const module = moduleOf(definition.parent.source.value); + const recognized = NODE_MODULES.get(module); + + return Boolean( + recognized && + definition.node.type === "ImportSpecifier" && + recognized.values.includes(definition.node.imported.name), + ); + } + + /** Whether a callee names a DOM constructor whose instances are targets. */ + function isDomConstructor(node) { + return node.type === "Identifier" && DOM_CONSTRUCTORS.has(node.name) && isGlobal(node); + } + + /** Whether an expression produces an event source. */ + function producesSource(node) { + if (!node) { + return false; + } + + if (node.type === "NewExpression") { + return ( + isNodeFactory(node.callee) || + isDomConstructor(node.callee) || + isEmitterSubclass(node.callee) + ); + } + + if (node.type === "CallExpression") { + return isNodeFactory(node.callee); + } + + if (node.type === "AwaitExpression" || node.type === "TSNonNullExpression") { + return producesSource(node.expression); + } + + // A binding chosen between two sources is still a source; one whose + // alternative is something else is not. + if (node.type === "ConditionalExpression") { + return producesSource(node.consequent) && producesSource(node.alternate); + } + + return isSource(node); + } + + /** Whether a callee names a class the file declares as an emitter. */ + function isEmitterSubclass(node) { + if (node.type !== "Identifier") { + return false; + } + + const definition = definitionOf(node); + const declaration = definition?.node; + + return ( + declaration?.type === "ClassDeclaration" && + emitterClasses.some((range) => range[0] === declaration.range[0]) + ); + } + + /** Whether an annotation names an event source, in any member of a union. */ + function isSourceType(annotation, at) { + return typeNames(annotation).some((name) => { + if (DOM_TYPES.has(name) || structural.has(name)) { + return true; + } + + const binding = types.get(name); + return binding !== undefined && binding <= at; + }); + } + + /** + * Whether an expression names an event source. Aliases and members are + * followed, so `child.stdout` and a binding that merely holds a socket both + * resolve to what produced them. + */ + function isSource(node) { + if (!node) { + return false; + } + + if (node.type === "MemberExpression") { + const name = memberName(node); + + if (name === null) { + return false; + } + + if (SOURCE_MEMBERS.has(name)) { + return isSource(node.object) || isProcessGlobal(node.object); + } + + if (isGlobal(node.object) && node.object.name === "globalThis") { + return DOM_GLOBALS.has(name) || name === "process"; + } + + return false; + } + + if (node.type === "ThisExpression") { + return inEmitterClass(node); + } + + if (node.type !== "Identifier") { + return false; + } + + if (isProcessGlobal(node)) { + return true; + } + + if (isGlobal(node) && DOM_GLOBALS.has(node.name)) { + return true; + } + + const definition = definitionOf(node); + + if (!definition) { + return false; + } + + if (definition.type === "Parameter") { + const parameter = definition.name ?? definition.node; + + if (isSourceType(parameter?.typeAnnotation, node.range[0])) { + return true; + } + + // A default value is the only thing a parameter says about itself when + // it carries no annotation. + const pattern = parameter?.parent; + + return ( + pattern?.type === "AssignmentPattern" && + pattern.left === parameter && + producesSource(pattern.right) + ); + } + + if (definition.type === "Variable" || definition.type === "VariableDeclarator") { + const declarator = definition.node; + + if (isSourceType(declarator.id?.typeAnnotation, node.range[0])) { + return true; + } + + // Only a binding that holds the source itself resolves: reassignment + // and conditional initialization say nothing about what it now holds. + return producesSource(declarator.init); + } + + return false; + } + + /** Whether a node sits inside a class that extends a recognized emitter. */ + function inEmitterClass(node) { + return emitterClasses.some((range) => range[0] <= node.range[0] && node.range[1] <= range[1]); + } + + /** The normalized text of a receiver expression, used as its identity. */ + function receiverKey(node) { + return text.slice(node.range[0], node.range[1]).replace(/\s+/gu, ""); + } + + /** A `const`-bound boolean, when the binding says so and nothing reassigns it. */ + function constantBoolean(node) { + if (node?.type !== "Identifier") { + return undefined; + } + + const variable = variableOf(node); + const definition = variable?.defs?.[0]; + + if (definition?.type !== "Variable" && definition?.type !== "VariableDeclarator") { + return undefined; + } + + const initializer = definition.node.init; + + return definition.parent?.kind === "const" && + initializer?.type === "Literal" && + typeof initializer.value === "boolean" + ? initializer.value + : undefined; + } + + /** Constructs that can decide not to run what is nested inside them. */ + const BRANCHES = new Set([ + "IfStatement", + "SwitchStatement", + "SwitchCase", + "ForStatement", + "ForOfStatement", + "ForInStatement", + "WhileStatement", + "DoWhileStatement", + "CatchClause", + "ConditionalExpression", + "LogicalExpression", + ]); + + /** + * Whether cleanup at `to` is reached whenever the registration at `from` + * has happened, with nothing suspending in between. + * + * Cleanup nested in a branch the registration is not nested in may simply + * not run, which is a listener left attached rather than a pair. + */ + function unconditionallyReached(from, to) { + const path = []; + + for (let parent = to.parent; parent; parent = parent.parent) { + if (contains(parent.range, from)) { + break; + } + path.push(parent); + } + + if (path.some((node) => BRANCHES.has(node.type))) { + return false; + } + + return adjacent(from, to); + } + + /** The identifier a member chain is rooted in, or null. */ + function rootIdentifier(node) { + let current = node; + + while (current?.type === "MemberExpression") { + current = current.object; + } + + return current?.type === "Identifier" ? current : null; + } + + /** + * Whether two expressions name the same value, rather than merely being + * spelled the same. Two connections both called `socket` are two sockets, + * and removing a handler from one says nothing about the other. + */ + function sameValue(left, right) { + if (receiverKey(left) !== receiverKey(right)) { + return false; + } + + const first = rootIdentifier(left); + const second = rootIdentifier(right); + + if (!first || !second) { + return true; + } + + return variableOf(first) === variableOf(second); + } + + /** + * The nearest enclosing Effection owner. + * + * Nearest, and nothing wider. A nested generator or `action()` executor is + * a scope of its own and ends before the one around it, so cleanup an + * outer owner performs runs too late to be this registration's pair — + * whether it names the handler directly or walks a collection the handler + * was recorded in. A plain callback declared inside an owner is not a + * scope, so it inherits that owner and may register on its behalf. + */ + function ownerOf(node) { + for (let parent = node.parent; parent; parent = parent.parent) { + if ( + (parent.type === "FunctionDeclaration" || parent.type === "FunctionExpression") && + parent.generator + ) { + return { node: parent, kind: "generator" }; + } + + if (FUNCTIONS.has(parent.type) && isActionExecutor(parent)) { + return { node: parent, kind: "action" }; + } + } + + return null; + } + + /** Whether a function is the executor `action()` was given. */ + function isActionExecutor(node) { + const call = node.parent; + + return ( + call?.type === "CallExpression" && + call.arguments[0] === node && + importsFrom(call.callee, "effection", "action") + ); + } + + function contains(range, node) { + return range[0] <= node.range[0] && node.range[1] <= range[1]; + } + + /** Suspensions performed between two positions of one block. */ + function suspendsBetween(block, from, to) { + let found = false; + + walk(block, (node) => { + if (node.type === "YieldExpression" && node.range[0] >= from && node.range[1] <= to) { + found = true; + } + }); + + return found; + } + + /** The statement of its own block that contains `node`. */ + function statementOf(node) { + let current = node; + + for (let parent = node.parent; parent; current = parent, parent = parent.parent) { + if (parent.type === "BlockStatement" || parent.type === "Program") { + return current; + } + } + + return null; + } + + /** The innermost block that holds both nodes. */ + function commonBlock(first, second) { + for (let parent = first.parent; parent; parent = parent.parent) { + if ( + (parent.type === "BlockStatement" || parent.type === "Program") && + contains(parent.range, second) + ) { + return parent; + } + } + + return null; + } + + /** The statement of `block` that holds `node`. */ + function statementIn(block, node) { + let current = node; + + for (let parent = node.parent; parent; current = parent, parent = parent.parent) { + if (parent === block) { + return current; + } + } + + return null; + } + + /** + * Whether two nodes are reached in one uninterrupted synchronous run: the + * statements holding them, in the innermost block holding both, with + * nothing that suspends in between. + */ + function adjacent(first, second) { + const block = commonBlock(first, second); + + if (!block) { + return false; + } + + const earlier = statementIn(block, first); + const later = statementIn(block, second); + + if (!earlier || !later || earlier === later) { + return true; + } + + const [start, end] = earlier.range[0] < later.range[0] ? [earlier, later] : [later, earlier]; + + return !suspendsBetween(block, start.range[1], end.range[0]); + } + + /** Visit every node under `root` that the same function evaluates. */ + function walk(root, visit) { + if (!root || typeof root.type !== "string") { + return; + } + + visit(root); + + for (const key of Object.keys(root)) { + if (key === "parent" || key === "loc" || key === "range") { + continue; + } + + const value = root[key]; + + for (const child of Array.isArray(value) ? value : [value]) { + if (child && typeof child === "object" && typeof child.type === "string") { + if (FUNCTIONS.has(child.type)) { + continue; + } + walk(child, visit); + } + } + } + } + + return { + ImportDeclaration(node) { + const module = moduleOf(node.source.value); + const recognized = NODE_MODULES.get(module); + + if (!recognized) { + return; + } + + for (const specifier of node.specifiers) { + if (specifier.type !== "ImportSpecifier") { + continue; + } + + const imported = specifier.imported.name; + + if (recognized.values.includes(imported)) { + values.set(specifier.local.name, node.range[0]); + } + + if (recognized.types.includes(imported)) { + types.set(specifier.local.name, node.range[0]); + } + } + }, + + TSInterfaceDeclaration(node) { + const members = node.body.body + .filter((member) => member.type === "TSMethodSignature" && !member.computed) + .map((member) => member.key?.name); + + if ( + (members.includes("on") && members.includes("off")) || + (members.includes("addEventListener") && members.includes("removeEventListener")) + ) { + structural.add(node.id.name); + } + }, + + ClassDeclaration(node) { + const parent = node.superClass; + + if (parent && (isNodeFactory(parent) || isDomConstructor(parent))) { + emitterClasses.push(node.range); + } + }, + + CallExpression(node) { + const callee = node.callee; + + if (callee.type !== "MemberExpression") { + return; + } + + const method = memberName(callee); + + if (method === null) { + return; + } + + if (REMOVE.has(method)) { + removals.push({ node, method, receiver: callee.object }); + return; + } + + if (method === "set" && node.arguments.length === 2) { + records.push({ node, collection: callee.object }); + return; + } + + if (!REGISTER.has(method)) { + return; + } + + registrations.push({ node, method, receiver: callee.object }); + }, + + "Program:exit"() { + for (const entry of registrations) { + classify(entry); + } + + reports.sort((left, right) => left.node.range[0] - right.node.range[0]); + + for (const entry of reports) { + context.report(entry); + } + }, + }; + + /** Report what one registration is missing, if anything. */ + function classify(entry) { + const { node, method, receiver } = entry; + + if (!isSource(receiver)) { + return; + } + + const owner = ownerOf(node); + + if (!owner) { + return; + } + + const [event, handler, options] = node.arguments; + const receiverText = receiverKey(receiver); + + if (!isStringLiteral(event)) { + reports.push({ node, messageId: "dynamicEvent", data: {} }); + return; + } + + if (method === "once") { + reports.push({ + node, + messageId: "rawOnce", + data: { receiver: receiverText, event: event.value }, + }); + return; + } + + if (method === "addEventListener" && staticFlag(options, "once", constantBoolean) === true) { + reports.push({ node, messageId: "onceOption", data: { event: event.value } }); + return; + } + + if (handler?.type !== "Identifier") { + reports.push({ node, messageId: "anonymous", data: { event: event.value } }); + return; + } + + const capture = method === "addEventListener" ? captureOf(options, constantBoolean) : false; + const remover = method === "addEventListener" ? "removeEventListener" : "off"; + + // Only the owner's own cleanup counts. Two operations in one file often + // name their socket `socket`, and a removal in one of them says nothing + // about what the other leaves attached. + const owned = removals.filter((removal) => contains(owner.node.range, removal.node)); + + const pairs = owned.filter( + (removal) => + sameValue(removal.receiver, receiver) && + isStringLiteral(removal.node.arguments[0]) && + removal.node.arguments[0].value === event.value && + removal.node.arguments[1]?.type === "Identifier" && + sameValue(removal.node.arguments[1], handler) && + (method !== "addEventListener" || + captureOf(removal.node.arguments[2], constantBoolean) === capture), + ); + + const data = { receiver: receiverText, event: event.value, handler: handler.name }; + const candidates = pairs.filter((removal) => removal.method === remover); + + if (candidates.length === 0) { + if (pairs.length > 0) { + reports.push({ node, messageId: "removeListener", data }); + return; + } + + if (detachedInBulk(entry, receiverText, handler, event.value, owner)) { + return; + } + + const near = owned.some( + (removal) => + receiverKey(removal.receiver) === receiverText || + (removal.node.arguments[1]?.type === "Identifier" && + removal.node.arguments[1].name === handler.name), + ); + + reports.push({ node, messageId: near ? "mismatched" : "missing", data }); + return; + } + + const verdicts = candidates.map((removal) => placement(removal, node, owner, handler)); + + if (verdicts.includes("accepted")) { + return; + } + + const verdict = verdicts.find((value) => value !== "unrelated") ?? "missing"; + + reports.push({ + node, + messageId: verdict === "unrelated" ? "missing" : verdict, + data, + }); + } + + /** + * Whether the owner remembers this pair in a collection and detaches that + * whole collection in its teardown. + * + * One handler per accepted connection cannot be named individually, so the + * collection is the pairing: the registration records the receiver and its + * handler together, and teardown walks the same collection removing each + * handler from the receiver it was recorded against. + */ + function detachedInBulk(entry, receiverText, handler, event, owner) { + const collections = records + .filter( + (record) => + contains(owner.node.range, record.node) && + sameValue(record.node.arguments[0], entry.receiver) && + record.node.arguments[1]?.type === "Identifier" && + sameValue(record.node.arguments[1], handler) && + // Recorded in the same synchronous run as the subscription: a pair + // written down after the owner could suspend is one the teardown + // in between would not have found. + unconditionallyReached(entry.node, record.node), + ) + .map((record) => record.collection); + + if (collections.length === 0) { + return false; + } + + return removals.some((removal) => { + if ( + removal.method !== + (entry.method === "addEventListener" ? "removeEventListener" : "off") || + !contains(owner.node.range, removal.node) || + !isStringLiteral(removal.node.arguments[0]) || + removal.node.arguments[0].value !== event + ) { + return false; + } + + const loop = iterationOf(removal.node); + + if (!loop || !collections.some((collection) => sameValue(collection, loop.right))) { + return false; + } + + const pattern = loop.left.declarations?.[0]?.id ?? loop.left; + + return ( + pattern?.type === "ArrayPattern" && + pattern.elements[0]?.name === receiverKey(removal.receiver) && + pattern.elements[1]?.name === removal.node.arguments[1]?.name && + releasedBeforeSuspending(removal.node, owner) + ); + }); + } + + /** The `for…of` whose body contains this node, within one function. */ + function iterationOf(node) { + for (let parent = node.parent; parent; parent = parent.parent) { + if (parent.type === "ForOfStatement") { + return parent; + } + + if (FUNCTIONS.has(parent.type)) { + return null; + } + } + + return null; + } + + /** Whether a bulk detach runs in teardown, before that teardown suspends. */ + function releasedBeforeSuspending(removal, owner) { + const cleanup = ensureCleanup(removal, owner); + + if (cleanup) { + return !suspendsBefore(cleanup, removal, protectedBy(removal)?.range); + } + + return protectedBy(removal) !== null; + } + + /** + * Where a matching removal sits relative to the registration, and whether + * that place runs when the owner ends. + */ + function placement(removal, registration, owner, handler) { + // Only a handler this file declares can remove itself. A handler that + // arrived as a parameter is somebody else's function, and its + // declaration is the enclosing signature rather than a body. + const definition = definitionOf(handler); + const handlerNode = + definition?.type === "Variable" || definition?.type === "VariableDeclarator" + ? definition.node.init + : definition?.type === "FunctionName" + ? definition.node + : null; + + if ( + handlerNode && + FUNCTIONS.has(handlerNode.type) && + contains(handlerNode.range, removal.node) + ) { + return "selfRemoving"; + } + + for (let parent = removal.node.parent; parent; parent = parent.parent) { + if ( + parent.type === "TryStatement" && + parent.finalizer && + contains(parent.finalizer.range, removal.node) + ) { + // The `finally` of a `try` that covers the subscription runs on every + // way out of it, which is the whole guarantee this rule is after. A + // subscription made just before that `try` is covered too, as long + // as nothing between the two can suspend. + if (contains(parent.block.range, registration)) { + return "accepted"; + } + + if (guards(parent, registration)) { + return "accepted"; + } + + // Otherwise the `finally` still protects whatever its `try` suspends + // on, which is what an ensure() with a listener-dependent wait needs. + const cleanup = ensureCleanup(removal.node, owner); + + if (cleanup) { + return suspendsBefore(cleanup, removal.node, parent.range) + ? "suspended" + : armed(cleanup, registration); + } + + if (statementOf(parent)?.parent === statementOf(registration)?.parent) { + return "late"; + } + } + + if (parent === owner.node) { + break; + } + } + + const cleanup = ensureCleanup(removal.node, owner); + + if (cleanup) { + if (suspendsBefore(cleanup, removal.node, protectedBy(removal.node)?.range)) { + return "suspended"; + } + + return armed(cleanup, registration); + } + + if (owner.kind === "action" && returnedCleanup(removal.node, owner.node)) { + return "accepted"; + } + + return "unrelated"; + } + + /** + * Whether a `try` statement covers a subscription made just before it, + * with no suspension in between to fail through. + */ + function guards(statement, registration) { + const subscription = statementOf(registration); + const guard = statementOf(statement); + + if (!subscription || !guard || subscription === guard) { + return false; + } + + // A lexical finalizer only covers what runs in its own frame. A `try` + // inside an `ensure()` cleanup is a different function, reached only once + // that `ensure()` has been established — which is the very thing the + // ordering rule in `armed()` is about. + if (functionOf(statement) !== functionOf(registration)) { + return false; + } + + return ( + subscription.range[1] <= guard.range[0] && unconditionallyReached(registration, statement) + ); + } + + /** The nearest function a node runs in, or null at module level. */ + function functionOf(node) { + for (let parent = node.parent; parent; parent = parent.parent) { + if (FUNCTIONS.has(parent.type)) { + return parent; + } + } + + return null; + } + + /** The `ensure()` cleanup function a node sits inside, within this owner. */ + function ensureCleanup(node, owner) { + for (let parent = node.parent; parent; parent = parent.parent) { + if ( + FUNCTIONS.has(parent.type) && + parent.parent?.type === "CallExpression" && + parent.parent.arguments[0] === parent && + importsFrom(parent.parent.callee, "effection", "ensure") + ) { + return parent; + } + + if (parent === owner.node) { + return null; + } + } + + return null; + } + + /** + * Whether the cleanup suspends before it reaches the removal. A suspension + * inside a `try` whose `finally` performs that removal does not count: it + * is the wait the listener is being kept alive for. + */ + function suspendsBefore(cleanup, removal, protectedRange) { + let found = false; + + walk(cleanup.body, (node) => { + if (node.type !== "YieldExpression" || node.range[1] > removal.range[0]) { + return; + } + + if (protectedRange && contains(protectedRange, node)) { + return; + } + + found = true; + }); + + return found; + } + + /** The `try` whose `finally` performs this removal, if there is one. */ + function protectedBy(removal) { + for (let parent = removal.parent; parent; parent = parent.parent) { + if ( + parent.type === "TryStatement" && + parent.finalizer && + contains(parent.finalizer.range, removal) + ) { + return parent; + } + } + + return null; + } + + /** + * Whether the `ensure()` holding this cleanup was established in the same + * uninterrupted synchronous run as the subscription. + */ + function armed(cleanup, registration) { + const call = cleanup.parent; + + if (!unconditionallyReached(registration, call)) { + return "late"; + } + + // Established means finished. `yield* ensure(...)` is itself a + // suspension, so an owner halted while it is registering unwinds with no + // cleanup on it at all — measured: the listener stays attached and the + // cleanup never runs. Only an `ensure()` that completed before the + // subscription existed has covered it. + return call.range[1] <= registration.range[0] ? "accepted" : "unarmed"; + } + + /** Whether a node sits in a function the action executor returns. */ + function returnedCleanup(node, executor) { + for (let parent = node.parent; parent; parent = parent.parent) { + if (FUNCTIONS.has(parent.type)) { + const owner = parent.parent; + + if (owner?.type === "ReturnStatement" && contains(executor.range, owner)) { + return true; + } + + if (executor.body === parent.parent || parent === executor) { + return false; + } + } + + if (parent === executor) { + return executor.body === node || contains(executor.range, node); + } + } + + return false; + } + }, +}; diff --git a/scripts/smoke-fetch.ts b/scripts/smoke-fetch.ts index b981babe1..4db587632 100644 --- a/scripts/smoke-fetch.ts +++ b/scripts/smoke-fetch.ts @@ -42,7 +42,23 @@ function useLoopback(): Operation { }); const listening = withResolvers(); - server.on("error", (error: Error) => listening.reject(error)); + // Removed with the resource rather than after the first error: a listening + // server outlives its bind, and a handler left behind would still be + // holding a rejected resolver when the next test binds its own. + // + // Established before the handler exists, because `yield* ensure(...)` is + // itself a suspension: an owner halted while it registers unwinds with no + // cleanup at all, so nothing may be attached until it has completed. + let onError: ((error: Error) => void) | undefined; + + yield* ensure(() => { + if (onError) { + server.off("error", onError); + } + }); + + onError = (error: Error) => listening.reject(error); + server.on("error", onError); server.listen(0, "127.0.0.1", () => listening.resolve()); yield* listening.operation; diff --git a/scripts/tests/fixtures/event-registration-bindings.ts b/scripts/tests/fixtures/event-registration-bindings.ts new file mode 100644 index 000000000..dba98ba82 --- /dev/null +++ b/scripts/tests/fixtures/event-registration-bindings.ts @@ -0,0 +1,92 @@ +/** Values whose `on`, `once` and `addEventListener` are somebody else's. */ +import { action as act, ensure as guard } from "effection"; +import type { Operation } from "effection"; +import { EventEmitter as NodeEmitter } from "node:events"; +import { connect } from "./event-registration-not-net.ts"; + +const handle = () => {}; + +/** A router of this application's own, not an event source. */ +const routes = { + on(_path: string, _handler: () => void) {}, + once(_path: string, _handler: () => void) {}, +}; + +export function* anApplicationRouter(): Operation { + routes.on("/health", handle); + routes.once("/ready", handle); +} + +export function* aBindingFromAnotherModule(): Operation { + const client = connect("postgres://localhost"); + client.on("notice", handle); + client.once("end", handle); +} + +/** Declared with `on` alone, which pairs with nothing. */ +interface Unsubscribable { + on(event: string, listener: () => void): unknown; +} + +export function* anUnpairedInterface(stream: Unsubscribable): Operation { + stream.on("data", handle); +} + +/** A class of the same name as the Node export, extending nothing. */ +class EventEmitter { + on(_event: string, _handler: () => void) {} + once(_event: string, _handler: () => void) {} +} + +export function* aShadowedConstructor(): Operation { + const emitter = new EventEmitter(); + emitter.on("ready", handle); + emitter.once("ready", handle); +} + +export function* aShadowedProcess(): Operation { + const process = { on: (_event: string, _handler: () => void) => {} }; + process.on("SIGINT", handle); +} + +export function* aShadowedDomGlobal(): Operation { + const document = { addEventListener: (_event: string, _handler: () => void) => {} }; + document.addEventListener("click", handle); +} + +export function* aParameterWithNoType(source: unknown): Operation { + (source as { on(event: string, handler: () => void): void }).on("data", handle); +} + +export function* aReassignedBinding(): Operation { + let socket; + socket = routes; + socket.on("data", handle); +} + +/** + * `ensure` and `action` are resolved through their import, not their spelling, + * so a renamed one still establishes the owner and its cleanup. + */ +export function* anAliasedEnsurePairs(): Operation { + const emitter = new NodeEmitter(); + let onReady: (() => void) | undefined; + + yield* guard(() => { + if (onReady) { + emitter.off("ready", onReady); + } + }); + + onReady = () => {}; + emitter.on("ready", onReady); +} + +export function nextReadyUnderAnAliasedAction(emitter: NodeEmitter): Operation { + return act((resolve) => { + const onReady = (): void => resolve(); + + emitter.on("ready", onReady); + return () => emitter.off("ready", onReady); + }); +} diff --git a/scripts/tests/fixtures/event-registration-exempted.ts b/scripts/tests/fixtures/event-registration-exempted.ts new file mode 100644 index 000000000..d5f9ffbf4 --- /dev/null +++ b/scripts/tests/fixtures/event-registration-exempted.ts @@ -0,0 +1,13 @@ +/* oxlint-disable local/require-scope-bound-event-registration */ +/** A file-wide directive states an invariant for nothing, and is rejected. */ +import type { Operation } from "effection"; +import { EventEmitter } from "node:events"; + +const handle = () => {}; + +export function* twoSubscriptions(): Operation { + const emitter = new EventEmitter(); + + emitter.on("ready", handle); + emitter.on("failed", handle); +} diff --git a/scripts/tests/fixtures/event-registration-not-net.ts b/scripts/tests/fixtures/event-registration-not-net.ts new file mode 100644 index 000000000..d7fecc33d --- /dev/null +++ b/scripts/tests/fixtures/event-registration-not-net.ts @@ -0,0 +1,7 @@ +/** A `connect` that is not the one `node:net` exports. */ +export function connect(_url: string) { + return { + on(_event: string, _handler: () => void) {}, + once(_event: string, _handler: () => void) {}, + }; +} diff --git a/scripts/tests/fixtures/event-registration-paired.ts b/scripts/tests/fixtures/event-registration-paired.ts new file mode 100644 index 000000000..cc52bbfad --- /dev/null +++ b/scripts/tests/fixtures/event-registration-paired.ts @@ -0,0 +1,143 @@ +/** The four teardown shapes that release a listener with its owner. */ +import { action, ensure, resource, withResolvers } from "effection"; +import type { Operation } from "effection"; +import { once } from "@effectionx/node/events"; +import { spawn as spawnChild } from "node:child_process"; +import { createServer } from "node:net"; +import type { Socket } from "node:net"; + +export function* finallyAroundTheSubscription(child = spawnChild("cat", [])): Operation { + const chunks: string[] = []; + const onData = (chunk: string) => chunks.push(chunk); + + try { + child.stdout.on("data", onData); + yield* action((resolve) => { + const onExit = () => resolve(); + child.on("exit", onExit); + return () => child.off("exit", onExit); + }); + } finally { + child.stdout.off("data", onData); + } + + return chunks.join(""); +} + +export function* ensureEstablishedBeforeTheSubscription(): Operation { + const server = createServer(); + const accepted: Socket[] = []; + let onConnection: ((socket: Socket) => void) | undefined; + + yield* ensure(() => { + if (onConnection) { + server.off("connection", onConnection); + } + }); + + onConnection = (socket: Socket) => accepted.push(socket); + server.on("connection", onConnection); + + yield* once(server, "listening"); +} + +export function* listenerDependentTeardown(): Operation { + const server = createServer(); + + // The wait needs a listener, so the cleanup attaches its own rather than + // depending on one the body established: registering it here and pairing it + // in this same frame's `finally` is what makes the whole thing atomic. + yield* ensure(function* () { + const closed = withResolvers(); + const onClose = () => closed.resolve(); + + server.on("close", onClose); + try { + server.close(); + yield* closed.operation; + } finally { + server.off("close", onClose); + } + }); + + yield* once(server, "listening"); +} + +export function useSocketErrors(socket: Socket): Operation { + return resource(function* (provide) { + const errors: Error[] = []; + let onError: ((error: Error) => void) | undefined; + + yield* ensure(() => { + if (onError) { + socket.off("error", onError); + } + }); + + onError = (error: Error) => errors.push(error); + socket.on("error", onError); + + yield* provide(errors); + }); +} + +export function nextMessage(port: MessagePort): Operation { + return action((resolve) => { + const onMessage = (event: MessageEvent) => resolve(event.data); + + port.addEventListener("message", onMessage); + return () => port.removeEventListener("message", onMessage); + }); +} + +export function nextCapturedClick(target: EventTarget): Operation { + return action((resolve) => { + const onClick = () => resolve(); + + target.addEventListener("click", onClick, true); + return () => target.removeEventListener("click", onClick, true); + }); +} + +export function* theTryCoversASubscriptionJustBeforeIt(socket: Socket): Operation { + const errors: Error[] = []; + const onError = (error: Error) => errors.push(error); + + socket.on("error", onError); + try { + yield* once(socket, "close"); + } finally { + socket.off("error", onError); + } +} + +export function* aCollectionIsThePairingForOnePerConnection(): Operation { + const server = createServer(); + const closers = new Map void>(); + + let onConnection: ((accepted: Socket) => void) | undefined; + + yield* ensure(function* () { + if (onConnection) { + server.off("connection", onConnection); + } + for (const [socket, onClose] of closers) { + socket.off("close", onClose); + } + closers.clear(); + yield* once(server, "close"); + }); + + onConnection = (accepted: Socket): void => { + const onClose = (): void => closers.delete(accepted); + closers.set(accepted, onClose); + accepted.on("close", onClose); + }; + server.on("connection", onConnection); + + yield* once(server, "listening"); +} + +export function* theCorrectedHelperIsTheOneEventWait(socket: Socket): Operation { + yield* once(socket, "close"); +} diff --git a/scripts/tests/fixtures/event-registration-raw-once.ts b/scripts/tests/fixtures/event-registration-raw-once.ts new file mode 100644 index 000000000..2692fd331 --- /dev/null +++ b/scripts/tests/fixtures/event-registration-raw-once.ts @@ -0,0 +1,45 @@ +/** One-event APIs whose cleanup depends on the event arriving. */ +import { action, ensure } from "effection"; +import type { Operation } from "effection"; +import { EventEmitter } from "node:events"; +import { connect } from "node:net"; + +export function* waitForClose(): Operation { + const socket = connect(1234, "localhost"); + + yield* action((resolve) => { + socket.once("close", () => resolve()); + return () => {}; + }); +} + +export function* waitForReady(): Operation { + const emitter = new EventEmitter(); + + emitter.once("ready", handle); + + function handle() {} + + yield* ensure(() => { + emitter.off("ready", handle); + }); +} + +export function* watchTheDocument(): Operation { + const target = new EventTarget(); + + target.addEventListener("ready", handle, { once: true }); + + function handle() {} + + yield* ensure(() => { + target.removeEventListener("ready", handle); + }); +} + +export function* alsoOnceThroughAnAlias(): Operation { + const socket = connect(1234, "localhost"); + const alias = socket; + + alias.once("error", () => {}); +} diff --git a/scripts/tests/fixtures/event-registration-sources.ts b/scripts/tests/fixtures/event-registration-sources.ts new file mode 100644 index 000000000..21bfb758d --- /dev/null +++ b/scripts/tests/fixtures/event-registration-sources.ts @@ -0,0 +1,106 @@ +/** One unpaired subscription per source family the rule recognizes. */ +import type { Operation } from "effection"; +import { EventEmitter } from "node:events"; +import { spawn as spawnChild } from "node:child_process"; +import { createServer as createNetServer, connect } from "node:net"; +import { createServer as createHttpServer } from "node:http"; +import type { IncomingMessage } from "node:http"; +import { PassThrough } from "node:stream"; +import type { Readable } from "node:stream"; +import process from "node:process"; + +const handle = () => {}; + +export function* fromAConstructor(): Operation { + const emitter = new EventEmitter(); + emitter.on("ready", handle); +} + +export function* fromAFactory(): Operation { + const server = createNetServer(); + server.on("connection", handle); +} + +export function* fromAConnection(): Operation { + const socket = connect(1234, "localhost"); + socket.on("data", handle); +} + +export function* fromAnHttpServer(): Operation { + const server = createHttpServer(); + server.on("request", handle); +} + +export function* fromAChildStream(): Operation { + const child = spawnChild("cat", []); + child.stdout.on("data", handle); +} + +export function* fromAStream(): Operation { + const stream = new PassThrough(); + stream.on("data", handle); +} + +export function* fromAnAlias(): Operation { + const emitter = new EventEmitter(); + const alias = emitter; + alias.on("ready", handle); +} + +export function* fromAnAnnotatedParameter(request: IncomingMessage): Operation { + request.on("data", handle); +} + +export function* fromAnAnnotatedReadable(stream: Readable): Operation { + stream.on("data", handle); +} + +export function* fromTheProcessGlobal(): Operation { + process.on("SIGINT", handle); +} + +export function* fromAProcessStream(): Operation { + process.stdin.on("data", handle); +} + +class Bus extends EventEmitter { + *listen(): Operation { + this.on("ready", handle); + } +} + +export const bus = new Bus(); + +export function* fromAnEmitterSubclass(): Operation { + const own = new Bus(); + own.on("ready", handle); +} + +/** A structural surface the file declares, paired the way the policy requires. */ +export interface InputStream { + on(event: "data", listener: () => void): unknown; + off(event: "data", listener: () => void): unknown; +} + +export function* fromAStructuralInterface(stream: InputStream): Operation { + stream.on("data", handle); +} + +export function* fromAnXhr(): Operation { + const request = new XMLHttpRequest(); + request.addEventListener("load", handle); +} + +export function* fromAnAbortSignal(): Operation { + const controller = new AbortController(); + controller.signal.addEventListener("abort", handle); +} + +export function* fromADomGlobal(): Operation { + globalThis.addEventListener("unhandledrejection", handle); +} + +export function* fromAWorker(): Operation { + const worker = new Worker("./worker.js"); + worker.addEventListener("message", handle); +} diff --git a/scripts/tests/fixtures/event-registration-suppressed.ts b/scripts/tests/fixtures/event-registration-suppressed.ts new file mode 100644 index 000000000..d0ced695d --- /dev/null +++ b/scripts/tests/fixtures/event-registration-suppressed.ts @@ -0,0 +1,14 @@ +/** A narrow directive covers one line, and one only. */ +import type { Operation } from "effection"; +import { EventEmitter } from "node:events"; + +const handle = () => {}; + +export function* twoSubscriptions(): Operation { + const emitter = new EventEmitter(); + + // The stated invariant this directive stands on would go here. + // oxlint-disable-next-line local/require-scope-bound-event-registration + emitter.on("ready", handle); + emitter.on("failed", handle); +} diff --git a/scripts/tests/fixtures/event-registration-unowned.ts b/scripts/tests/fixtures/event-registration-unowned.ts new file mode 100644 index 000000000..2b51c6db2 --- /dev/null +++ b/scripts/tests/fixtures/event-registration-unowned.ts @@ -0,0 +1,30 @@ +/** + * Listeners whose lifetime is the process or the page, installed where no + * Effection scope owns them. A standalone fixture program lives exactly as + * long as its listeners do; there is nothing to release them from. + */ +import { EventEmitter } from "node:events"; +import { createServer } from "node:net"; +import process from "node:process"; + +const handle = () => {}; + +process.on("SIGINT", handle); +globalThis.addEventListener("unhandledrejection", handle); + +export function serveForTheProcessLifetime() { + const server = createServer(); + server.on("connection", handle); + return server; +} + +export function waitForReady(emitter: EventEmitter): Promise { + return new Promise((resolve) => { + emitter.on("ready", () => resolve()); + }); +} + +export function subscribeForThePageLifetime() { + const target = new EventTarget(); + target.addEventListener("ready", handle); +} diff --git a/scripts/tests/fixtures/event-registration-unpaired.ts b/scripts/tests/fixtures/event-registration-unpaired.ts new file mode 100644 index 000000000..dbb4cef18 --- /dev/null +++ b/scripts/tests/fixtures/event-registration-unpaired.ts @@ -0,0 +1,294 @@ +/** Subscriptions whose cleanup does not release what they attached. */ +import { ensure, resource, sleep } from "effection"; +import type { Operation } from "effection"; +import { spawn as spawnChild } from "node:child_process"; +import { connect, createServer } from "node:net"; +import type { Socket } from "node:net"; + +export function* anonymousHandler(): Operation { + const server = createServer(); + + server.on("connection", () => {}); +} + +export function* noCleanupAtAll(): Operation { + const server = createServer(); + const onConnection = () => {}; + + server.on("connection", onConnection); +} + +export function* differentEvent(): Operation { + const server = createServer(); + const onConnection = () => {}; + + server.on("connection", onConnection); + + yield* ensure(() => { + server.off("close", onConnection); + }); +} + +export function* differentHandler(): Operation { + const server = createServer(); + const onConnection = () => {}; + const onClose = () => {}; + + server.on("connection", onConnection); + + yield* ensure(() => { + server.off("connection", onClose); + }); +} + +export function* differentReceiver(): Operation { + const server = createServer(); + const other = createServer(); + const onConnection = () => {}; + + server.on("connection", onConnection); + + yield* ensure(() => { + other.off("connection", onConnection); + }); +} + +export function* armedTooLate(): Operation { + const child = spawnChild("cat", []); + const onExit = () => {}; + + child.on("exit", onExit); + + yield* sleep(1); + + yield* ensure(() => { + child.off("exit", onExit); + }); +} + +export function* removesItselfOnly(): Operation { + const child = spawnChild("cat", []); + const onExit = () => { + child.off("exit", onExit); + }; + + child.on("exit", onExit); +} + +export function* removesAfterASuspension(): Operation { + const child = spawnChild("cat", []); + const onExit = () => {}; + + child.on("exit", onExit); + + yield* ensure(function* () { + yield* sleep(1); + child.off("exit", onExit); + }); +} + +export function* removesWithRemoveListener(): Operation { + const child = spawnChild("cat", []); + const onExit = () => {}; + + child.on("exit", onExit); + + yield* ensure(() => { + child.removeListener("exit", onExit); + }); +} + +export function* dynamicEventName(event: string): Operation { + const server = createServer(); + const onEvent = () => {}; + + server.on(event, onEvent); + + yield* ensure(() => { + server.off(event, onEvent); + }); +} + +export function* aTryOpenedAfterASuspension(socket: Socket): Operation { + const errors: Error[] = []; + const onError = (error: Error) => errors.push(error); + + socket.on("error", onError); + + yield* sleep(1); + + try { + yield* sleep(1); + } finally { + socket.off("error", onError); + } +} + +export function* recordedButNeverDetached(): Operation { + const server = createServer(); + const closers = new Map void>(); + + const onConnection = (accepted: Socket): void => { + const onClose = (): void => closers.delete(accepted); + closers.set(accepted, onClose); + accepted.on("close", onClose); + }; + + server.on("connection", onConnection); + yield* ensure(() => { + server.off("connection", onConnection); + closers.clear(); + }); +} + +export function* detachedFromAnotherCollection(): Operation { + const server = createServer(); + const closers = new Map void>(); + const others = new Map void>(); + + const onConnection = (accepted: Socket): void => { + const onClose = (): void => closers.delete(accepted); + closers.set(accepted, onClose); + accepted.on("close", onClose); + }; + + server.on("connection", onConnection); + yield* ensure(() => { + server.off("connection", onConnection); + for (const [socket, onClose] of others) { + socket.off("close", onClose); + } + }); +} + +export function* mismatchedCapture(): Operation { + const target = new EventTarget(); + const onReady = () => {}; + + target.addEventListener("ready", onReady, true); + + yield* ensure(() => { + target.removeEventListener("ready", onReady); + }); +} + +export function* cleanupBehindACondition(flag: boolean): Operation { + const server = createServer(); + const onConnection = () => {}; + + server.on("connection", onConnection); + + if (flag) { + yield* ensure(() => { + server.off("connection", onConnection); + }); + } +} + +export function* cleanupInAnOuterOwner(): Operation { + const server = createServer(); + const onConnection = () => {}; + + const subscribe = function* (): Operation { + server.on("connection", onConnection); + }; + + yield* subscribe(); + yield* ensure(() => { + server.off("connection", onConnection); + }); +} + +export function* recordedAfterASuspension(): Operation { + const closers = new Map void>(); + const accepted = connect(1234, "localhost"); + const onClose = (): void => closers.delete(accepted); + + accepted.on("close", onClose); + + yield* sleep(1); + + closers.set(accepted, onClose); + yield* ensure(() => { + for (const [socket, handler] of closers) { + socket.off("close", handler); + } + }); +} + +export function* recordedInAShadowedCollection(): Operation { + const server = createServer(); + const onConnection = (accepted: Socket): void => { + const closers = new Map void>(); + const onClose = (): void => closers.delete(accepted); + accepted.on("close", onClose); + closers.set(accepted, onClose); + }; + const closers = new Map void>(); + + server.on("connection", onConnection); + yield* ensure(() => { + server.off("connection", onConnection); + for (const [socket, onClose] of closers) { + socket.off("close", onClose); + } + }); +} + +const ALWAYS = true; + +export function* onceSpelledAsAString(): Operation { + const target = new EventTarget(); + const onReady = () => {}; + + target.addEventListener("ready", onReady, { "once": true }); + + yield* ensure(() => { + target.removeEventListener("ready", onReady); + }); +} + +export function* onceBoundToAConstant(): Operation { + const target = new EventTarget(); + const onReady = () => {}; + + target.addEventListener("ready", onReady, { once: ALWAYS }); + + yield* ensure(() => { + target.removeEventListener("ready", onReady); + }); +} + +export function useSocketObservers(): Operation<{ watch(port: number): Operation }> { + const observers = new Map void>(); + + return resource(function* (provide) { + yield* ensure(() => { + for (const [socket, onError] of observers) { + socket.off("error", onError); + } + observers.clear(); + }); + + yield* provide({ + *watch(port: number): Operation { + const socket = connect(port, "localhost"); + const onError = (): void => {}; + + socket.on("error", onError); + observers.set(socket, onError); + yield* sleep(1); + }, + }); + }); +} + +export function* ensureYieldedAfterTheSubscription(): Operation { + const server = createServer(); + const onConnection = () => {}; + + server.on("connection", onConnection); + yield* ensure(() => { + server.off("connection", onConnection); + }); +} diff --git a/scripts/tests/oxlint-policy.test.ts b/scripts/tests/oxlint-policy.test.ts index 275679d0d..82c43e63a 100644 --- a/scripts/tests/oxlint-policy.test.ts +++ b/scripts/tests/oxlint-policy.test.ts @@ -57,6 +57,7 @@ const GATE_RULES = [ "local/no-yield-in-finally", "local/prefer-effection-operation", "local/prefer-effection-result", + "local/require-scope-bound-event-registration", ]; interface Override { diff --git a/scripts/tests/scope-bound-event-registration.test.ts b/scripts/tests/scope-bound-event-registration.test.ts new file mode 100644 index 000000000..68df0b148 --- /dev/null +++ b/scripts/tests/scope-bound-event-registration.test.ts @@ -0,0 +1,493 @@ +/** + * The lifecycle contract this repository requires of the one-event helper it + * standardises on, `once()` from `@effectionx/node/events`. + * + * A listener is scoped state. The helper therefore has to behave like every + * other Effection resource: register nothing until somebody interprets it, + * register exactly once when they do, and detach on every way the interpreting + * scope can end — the event arriving, a halt, or losing a `race()`. Cleanup + * that runs only when the event fires is not cleanup, because the event is + * exactly what a cancelled wait never gets. + * + * Node and the DOM count listeners differently, so both families are proved + * here: `EventEmitter` answers `listenerCount()` directly, and `EventTarget` + * has no equivalent, so the target below counts its own registrations. + * + * Tracked upstream as thefrontside/effectionx#251. Until a release that fixes + * it is published, this file fails on construction being eager, which is the + * defect. + */ +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { once } from "@effectionx/node/events"; +import { readTextFile, walk } from "@effectionx/fs"; +import { each, race, sleep, spawn } from "effection"; +import type { Operation, Task } from "effection"; +import { EventEmitter } from "node:events"; +import path from "node:path"; + +import { oxlint, ROOT, violations } from "./oxlint.ts"; + +/** + * An `EventTarget` that reports how many listeners it is holding. The platform + * exposes no count, so the registrations are tallied as they are made. + */ +class CountingTarget extends EventTarget { + #live = new Map(); + + override addEventListener( + type: string, + listener: EventListenerOrEventListenerObject | null, + options?: AddEventListenerOptions | boolean, + ): void { + this.#live.set(type, (this.#live.get(type) ?? 0) + 1); + super.addEventListener(type, listener, options); + } + + override removeEventListener( + type: string, + listener: EventListenerOrEventListenerObject | null, + options?: EventListenerOptions | boolean, + ): void { + this.#live.set(type, (this.#live.get(type) ?? 0) - 1); + super.removeEventListener(type, listener, options); + } + + listenerCount(type: string): number { + return this.#live.get(type) ?? 0; + } +} + +/** Give a spawned child its first turn, so its registrations have happened. */ +function started(): Operation { + return sleep(0); +} + +/** Let a settled child's teardown finish before its listeners are counted. */ +function settled(): Operation { + return sleep(0); +} + +describe("@effectionx/node once() is scope-bound", () => { + describe("with a Node EventEmitter", () => { + it("registers nothing until the operation is interpreted", function* () { + const emitter = new EventEmitter(); + + once(emitter, "ready"); + + expect(emitter.listenerCount("ready")).toBe(0); + }); + + it("registers exactly one listener when interpreted", function* () { + const emitter = new EventEmitter(); + + yield* spawn(function* () { + yield* once(emitter, "ready"); + }); + yield* started(); + + expect(emitter.listenerCount("ready")).toBe(1); + }); + + it("settles once with the emitted arguments and detaches", function* () { + const emitter = new EventEmitter(); + const waiter: Task = yield* spawn(function* () { + return yield* once(emitter, "ready"); + }); + yield* started(); + + emitter.emit("ready", "first", 2); + const args = yield* waiter; + + expect(args).toEqual(["first", 2]); + expect(emitter.listenerCount("ready")).toBe(0); + }); + + it("detaches when the interpreting scope halts before the event", function* () { + const emitter = new EventEmitter(); + const waiter = yield* spawn(function* () { + yield* once(emitter, "ready"); + }); + yield* started(); + + yield* waiter.halt(); + + expect(emitter.listenerCount("ready")).toBe(0); + }); + + it("detaches the arm that loses a race", function* () { + const emitter = new EventEmitter(); + const winner: Task = yield* spawn(function* () { + return yield* race([once(emitter, "ready"), once(emitter, "failed")]); + }); + yield* started(); + + expect(emitter.listenerCount("failed")).toBe(1); + + emitter.emit("ready", "won"); + yield* winner; + yield* settled(); + + expect(emitter.listenerCount("failed")).toBe(0); + }); + + /** + * A leaked listener is not inert: it still holds a resolver for work + * nobody is waiting on, and the next interpretation would see two. An + * event after the halt therefore has to reach nothing at all, and a fresh + * wait has to observe the event after it rather than the one it missed. + */ + it("ignores an event emitted after the wait was abandoned", function* () { + const emitter = new EventEmitter(); + const abandoned = yield* spawn(function* () { + yield* once(emitter, "ready"); + }); + yield* started(); + yield* abandoned.halt(); + + // Asserted before the emit, not after: a leaked listener that removes + // itself when the event finally arrives leaves the same count behind as + // one that was never there. + expect(emitter.listenerCount("ready")).toBe(0); + + emitter.emit("ready", "ignored"); + + const waiter: Task = yield* spawn(function* () { + return yield* once(emitter, "ready"); + }); + yield* started(); + + expect(emitter.listenerCount("ready")).toBe(1); + + emitter.emit("ready", "observed"); + + expect(yield* waiter).toEqual(["observed"]); + }); + }); + + describe("with a DOM EventTarget", () => { + it("registers nothing until the operation is interpreted", function* () { + const target = new CountingTarget(); + + once(target, "ready"); + + expect(target.listenerCount("ready")).toBe(0); + }); + + it("registers exactly one listener when interpreted", function* () { + const target = new CountingTarget(); + + yield* spawn(function* () { + yield* once(target, "ready"); + }); + yield* started(); + + expect(target.listenerCount("ready")).toBe(1); + }); + + it("settles once with the dispatched event and detaches", function* () { + const target = new CountingTarget(); + const waiter: Task = yield* spawn(function* () { + return yield* once(target, "ready"); + }); + yield* started(); + + target.dispatchEvent(new Event("ready")); + const [event] = yield* waiter; + + expect(event.type).toBe("ready"); + expect(target.listenerCount("ready")).toBe(0); + }); + + it("detaches when the interpreting scope halts before the event", function* () { + const target = new CountingTarget(); + const waiter = yield* spawn(function* () { + yield* once(target, "ready"); + }); + yield* started(); + + yield* waiter.halt(); + + expect(target.listenerCount("ready")).toBe(0); + }); + + it("detaches the arm that loses a race", function* () { + const target = new CountingTarget(); + const winner: Task = yield* spawn(function* () { + return yield* race([once(target, "ready"), once(target, "failed")]); + }); + yield* started(); + + expect(target.listenerCount("failed")).toBe(1); + + target.dispatchEvent(new Event("ready")); + yield* winner; + yield* settled(); + + expect(target.listenerCount("failed")).toBe(0); + }); + + it("ignores an event dispatched after the wait was abandoned", function* () { + const target = new CountingTarget(); + const abandoned = yield* spawn(function* () { + yield* once(target, "ready"); + }); + yield* started(); + yield* abandoned.halt(); + + // Asserted before the dispatch, for the reason given above. + expect(target.listenerCount("ready")).toBe(0); + + target.dispatchEvent(new Event("ready")); + + const waiter: Task = yield* spawn(function* () { + return yield* once(target, "ready"); + }); + yield* started(); + + expect(target.listenerCount("ready")).toBe(1); + + const dispatched = new Event("ready"); + target.dispatchEvent(dispatched); + + expect((yield* waiter)[0]).toBe(dispatched); + }); + }); +}); + +const RULE = "require-scope-bound-event-registration"; + +/** The directories `deno task lint` passes to oxlint. */ +const LINTED = ["packages", "scripts", ".reviews/components"]; + +/** What the lint task's `--ignore-pattern` arguments keep out. */ +const UNLINTED = [ + `${path.sep}npm${path.sep}`, + path.join("scripts", "tests", "fixtures"), + path.join("packages", "workflow", "vendor", "cloudflare-computer-dofs"), + `${path.sep}node_modules${path.sep}`, +]; + +/** A whole-file directive, as opposed to `-next-line` or `-line`. */ +const FILE_WIDE = new RegExp(`(?:oxlint|eslint)-disable(?!-next-line|-line)[^\\n]*${RULE}`, "u"); + +const NARROW = new RegExp(`oxlint-disable-next-line[^\\n]*local/${RULE}`, "u"); + +function reported(fixture: string): Operation { + return violations(`scripts/tests/fixtures/${fixture}`, RULE); +} + +/** The rule's own diagnostics for a fixture, in source order. */ +function* diagnostics(fixture: string): Operation<{ line: number; message: string }[]> { + const output = yield* oxlint(["--format=json", `scripts/tests/fixtures/${fixture}`]); + const report: { + diagnostics: { code: string; message: string; labels: { span: { line: number } }[] }[]; + } = JSON.parse(output); + + return report.diagnostics + .filter((entry) => entry.code === `local(${RULE})`) + .map((entry) => ({ line: entry.labels[0].span.line, message: entry.message })) + .sort((left, right) => left.line - right.line); +} + +/** Every file `deno task lint` actually reads. */ +function* linted(): Operation { + const files: string[] = []; + for (const directory of LINTED) { + const entries = walk(path.join(ROOT, directory), { + includeDirs: false, + skip: [/node_modules/u, /[/\\]npm[/\\]/u], + }); + for (const entry of yield* each(entries)) { + if ( + /\.(?:ts|tsx|js|mjs|cjs)$/u.test(entry.path) && + !UNLINTED.some((fragment) => entry.path.includes(fragment)) + ) { + files.push(entry.path); + } + yield* each.next(); + } + } + return files; +} + +describe("local/require-scope-bound-event-registration", () => { + /** + * In fixture order: `.once()` on a socket inside an `action()`, on an + * emitter whose `ensure()` would otherwise pair it, `{ once: true }` on a + * DOM target, and `.once()` reached through an alias of the socket. + */ + it("reports one-event APIs whose cleanup waits for the event", function* () { + expect(yield* reported("event-registration-raw-once.ts")).toEqual([11, 19, 31, 44]); + }); + + it("names the scope-bound helper as the replacement for a one-event wait", function* () { + const report = yield* diagnostics("event-registration-raw-once.ts"); + + expect(report[0].message).toContain("@effectionx/node/events"); + expect(report[0].message).toContain("cancelled wait never gets"); + }); + + /** + * In fixture order: an inline handler, no cleanup at all, a cleanup naming a + * different event, a different handler and a different receiver, an + * `ensure()` armed after the owner has already suspended, a handler that + * only removes itself, a teardown that suspends before removing, a + * `removeListener()` where `.off()` belongs, a computed event name, a + * `try` opened only after the owner has suspended, a per-connection handler + * recorded in a collection nothing walks and one walked from a different + * collection, a DOM removal whose capture mode disagrees with the + * registration, cleanup behind a condition that may not run, cleanup in an + * outer owner rather than the one that subscribed, a pair recorded only + * after the owner suspended, a pair recorded into a shadowed collection of + * the same name, and a one-shot option spelled as a string key or bound to + * a constant, a pair a nested generator records that only the resource + * around it walks, and an `ensure()` yielded after the subscription — whose + * own registration is a suspension an owner can be halted at. + */ + it("reports every way a subscription outlives its owner", function* () { + expect(yield* reported("event-registration-unpaired.ts")).toEqual([ + 11, 18, 25, 37, 49, 60, 75, 82, 94, 105, 116, 134, 137, 152, 155, 168, 179, 193, 207, 224, + 229, 244, 255, 278, 290, + ]); + }); + + it("distinguishes a missing cleanup from one that names the wrong pair", function* () { + const report = yield* diagnostics("event-registration-unpaired.ts"); + const at = (line: number) => report.find((entry) => entry.line === line)?.message ?? ""; + + expect(at(18)).toContain("Nothing removes onConnection"); + expect(at(25)).toContain("does not name the same receiver, event, handler and capture mode"); + expect(at(60)).toContain("registered after the owner can already suspend"); + expect(at(75)).toContain("removes itself only when"); + expect(at(82)).toContain("Teardown suspends before it removes"); + expect(at(94)).toContain("removeListener()"); + expect(at(105)).toContain("event name is computed"); + expect(at(116)).toContain("registered after the owner can already suspend"); + expect(at(134)).toContain("Nothing removes onClose"); + expect(at(152)).toContain("does not name the same receiver, event, handler and capture mode"); + expect(at(179)).toContain("registered after the owner can already suspend"); + expect(at(193)).toContain("Nothing removes onConnection"); + expect(at(207)).toContain("Nothing removes onClose"); + expect(at(224)).toContain("does not name the same receiver, event, handler and capture mode"); + expect(at(244)).toContain("{ once: true }"); + expect(at(255)).toContain("{ once: true }"); + expect(at(278)).toContain("Nothing removes onError"); + expect(at(229)).toContain("yielded after onConnection was attached"); + expect(at(290)).toContain("Establish the ensure() before the subscription"); + }); + + /** + * A `finally` around the subscription, an `ensure()` armed in the same + * synchronous prefix, an `ensure()` whose listener-dependent wait is closed + * by a synchronous inner `finally`, the cleanup an `action()` returns, and + * the corrected helper itself. + */ + it("accepts the four teardown shapes and the scope-bound helper", function* () { + expect(yield* reported("event-registration-paired.ts")).toEqual([]); + }); + + /** + * The acceptance above is only worth something if the rule was looking. One + * registration per source family, each with no cleanup at all, is reported: + * constructors, factories, connections, child streams, aliases, annotated + * parameters, the `process` global and its streams, an emitter subclass and + * its `this`, a paired structural interface, and the DOM families. + */ + it("recognizes every source family it claims to", function* () { + expect(yield* reported("event-registration-sources.ts")).toEqual([ + 16, 21, 26, 31, 36, 41, 47, 51, 55, 59, 63, 68, 76, 86, 91, 96, 100, 105, + ]); + }); + + /** + * The near misses of that list: a router with an `on()` of its own, a + * `connect` from another module, an interface declaring `on` and no `off`, a + * class named `EventEmitter` that extends nothing, a local `process` and a + * local `document`, an unannotated parameter, and a binding that was + * reassigned after it was declared. `ensure` and `action` imported under + * other names still pair and still own, because both are resolved through + * their import rather than their spelling. + */ + it("accepts values whose on/once belong to somebody else", function* () { + expect(yield* reported("event-registration-bindings.ts")).toEqual([]); + }); + + it("accepts listeners installed where no Effection scope owns them", function* () { + expect(yield* reported("event-registration-unowned.ts")).toEqual([]); + }); + + it("suppresses the line a narrow directive covers, and no other", function* () { + expect(yield* reported("event-registration-suppressed.ts")).toEqual([13]); + }); + + /** + * A file-wide directive silences every subscription below it while stating + * an invariant for none of them. Nothing is reported here, which is exactly + * why the form is forbidden and why the repository is checked for it below. + */ + it("is silenced entirely by a file-wide exemption", function* () { + expect(yield* reported("event-registration-exempted.ts")).toEqual([]); + }); + + it("finds no broad exemption anywhere on the lint surface", function* () { + const exempted: string[] = []; + for (const file of yield* linted()) { + if (FILE_WIDE.test(yield* readTextFile(file))) { + exempted.push(path.relative(ROOT, file)); + } + } + + expect(exempted).toEqual([]); + }); + + it("finds no configuration entry turning the rule off for a path", function* () { + for (const config of [".oxlintrc.json", "oxlint.shared.json", ".reviews/.oxlintrc.json"]) { + const source = yield* readTextFile(path.join(ROOT, config)); + const off = new RegExp(`"local/${RULE}":\\s*(?:"off"|\\["off")`, "u").test(source); + expect([config, off]).toEqual([config, false]); + } + }); + + it("keeps every suppression narrow and explained", function* () { + const unexplained: string[] = []; + for (const file of yield* linted()) { + const lines = (yield* readTextFile(file)).split("\n"); + for (const [index, line] of lines.entries()) { + if (!NARROW.test(line)) { + continue; + } + const preceding = lines + .slice(Math.max(0, index - 8), index) + .filter((entry) => /^\s*(?:\/\/|\*|\/\*)/u.test(entry)) + .filter((entry) => !NARROW.test(entry)); + if (preceding.length === 0) { + unexplained.push(`${path.relative(ROOT, file)}:${index + 1}`); + } + } + } + + expect(unexplained).toEqual([]); + }); + + /** + * Non-vacuous on both sweeps: the surface has files to read, and the pattern + * that finds no broad exemption above does find the one the fixture carries. + */ + it("sweeps a populated lint surface with a pattern that matches", function* () { + const files = yield* linted(); + + expect(files.length).toBeGreaterThan(100); + + const exempted = yield* readTextFile( + path.join(ROOT, "scripts/tests/fixtures/event-registration-exempted.ts"), + ); + expect(FILE_WIDE.test(exempted)).toBe(true); + expect(NARROW.test(exempted)).toBe(false); + + const suppressed = yield* readTextFile( + path.join(ROOT, "scripts/tests/fixtures/event-registration-suppressed.ts"), + ); + expect(NARROW.test(suppressed)).toBe(true); + }); +});