From 87db305547dd4bf9d00fc6cae8e1fd547e854eeb Mon Sep 17 00:00:00 2001 From: Taras Mankovski Date: Thu, 3 Sep 2026 22:59:27 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20Require=20scope-bound=20event=20?= =?UTF-8?q?listeners=20in=20Effection=20code=20(#748)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An event listener an Effection operation installs is state that operation owns, but nothing about an event source enforces that. A handler attached to a socket, a child process or a DOM target survived the operation that attached it, and kept writing to state the abandoned owner left behind. Cleanup that runs when the event fires is not cleanup: a cancelled wait is exactly the case where the event never arrives. `@effectionx/node` moves to 0.2.5, whose `once()` registers nothing until it is interpreted and detaches in a `finally` — after delivery, a halt, or losing a race. Every direct pin moves together; `@effectionx/process` stays at 0.8.1, which leaves one audited transitive 0.2.4 lock entry that nothing here resolves. `local/require-scope-bound-event-registration` makes the policy a blocking gate. It resolves sources, owners, `action` and `ensure` through their imports rather than their spelling, and an owner is the nearest generator or `action()` executor and nothing wider. Cleanup counts as established only when it is in place before the thing it releases exists: a lexical `finally` entered before the subscription, an `ensure()` that *completed* before it, or the cleanup an `action()` returns. An `ensure()` yielded afterwards is rejected, because entering it is itself a suspension — an owner halted there unwinds with no cleanup registered at all, measured, with the listener still attached. Every applicable registration is migrated onto one of those shapes. The three owners that own a child process register their cleanup before spawning it, and that cleanup keeps every handler attached until the child's `close` — the only event that says the process and the pipes this run inherited are finished. An assigned exit status is not that boundary, so none of them reads one: a cancelled command still captures what its child wrote on the way out, and still escalates SIGTERM to SIGKILL exactly as before. Readiness races are interpreted inline, in the same synchronous run as the `connect`, `listen` or `spawn` they watch, because a spawned race attaches its arms a turn later and the source can settle in that turn. The foreground reaper names its exit handler and releases it in the one funnel every settlement passes through. `nodeResponseChannel` becomes an `Operation`, so both of its handlers belong to the request task. The form responder keeps its request observed until that request's own `close`, so the peer's hang-up reaches a listener instead of the process. Web server teardown detaches every accepted socket's close handler before it suspends, then ends request tasks before destroying their connections. The workflow and CLI HTTP fixtures read request bodies in tasks of the server's own scope. Every distinct manual owner is cancelled while its listeners are live. Each case measures the baseline the runtime was already holding, reads the vector once with the owner running, halts it before the event that would have settled it, reads the vector again before replaying anything, then replays and proves no capture, callback or result moved. Where a source is a child process or accepted socket an operation does not hand out, a package-private observation seam carries it; no public package contract widened. This is a lifecycle change. Event ordering, output aggregation, response bytes, failure precedence, process signalling and every public result are unchanged. --- .oxlintrc.json | 3 +- AGENTS.md | 22 + architecture.md | 10 + deno.json | 2 +- deno.lock | 24 +- package.json | 2 +- packages/acp/tests/adapter-protocol.test.ts | 55 +- .../fixtures/claude-native-launch-proof.ts | 83 +- .../fixtures/claude-native-to-acp-proof.ts | 80 +- packages/cli/tests/fetch-cli.test.ts | 18 +- packages/cli/tests/stdin-cli.test.ts | 115 +- packages/cli/tests/stdout-delivery.test.ts | 19 +- .../cli/tests/support/pull-request-server.ts | 16 +- packages/cli/tests/workflow-fetch.test.ts | 18 +- packages/core/package.json | 2 +- packages/runtime/launcher.ts | 57 +- packages/runtime/package.json | 2 +- packages/runtime/tests/fetch.test.ts | 67 +- .../runtime/tests/native-launcher.test.ts | 56 + packages/test-agent/deno.json | 2 +- packages/test-agent/package.json | 2 +- packages/test-agent/src/net.ts | 92 +- packages/test-agent/src/worker/acp-server.ts | 20 +- packages/test-agent/tests/net.test.ts | 52 +- packages/test-agent/tests/public-api.test.ts | 20 +- packages/test-support/launch.ts | 132 +- packages/web/package.json | 2 +- packages/web/src/responder.ts | 98 +- packages/web/src/response-channel.ts | 68 +- packages/web/src/server.ts | 72 +- packages/web/tests/http-client.ts | 50 +- packages/web/tests/responder.test.ts | 129 +- packages/web/tests/server-lifecycle.test.ts | 309 +++- packages/web/tests/server-support.ts | 25 +- .../src/deno/composition/subprocess.ts | 169 ++- .../tests/ambient-authentication.test.ts | 53 + .../workflow/tests/credential-helper.test.ts | 102 +- .../workflow/tests/git-push-crash.test.ts | 140 +- packages/workflow/tests/support/git-http.ts | 246 +++- packages/workflow/tests/support/github.ts | 19 +- .../tests/support/issue-tracker-server.ts | 31 +- pnpm-lock.yaml | 29 +- scripts/oxlint-plugin.js | 2 + .../require-scope-bound-event-registration.js | 1279 +++++++++++++++++ scripts/smoke-fetch.ts | 18 +- .../fixtures/event-registration-bindings.ts | 92 ++ .../fixtures/event-registration-exempted.ts | 13 + .../fixtures/event-registration-not-net.ts | 7 + .../fixtures/event-registration-paired.ts | 143 ++ .../fixtures/event-registration-raw-once.ts | 45 + .../fixtures/event-registration-sources.ts | 106 ++ .../fixtures/event-registration-suppressed.ts | 14 + .../fixtures/event-registration-unowned.ts | 30 + .../fixtures/event-registration-unpaired.ts | 294 ++++ scripts/tests/oxlint-policy.test.ts | 1 + .../scope-bound-event-registration.test.ts | 493 +++++++ 56 files changed, 4621 insertions(+), 429 deletions(-) create mode 100644 scripts/oxlint-rules/require-scope-bound-event-registration.js create mode 100644 scripts/tests/fixtures/event-registration-bindings.ts create mode 100644 scripts/tests/fixtures/event-registration-exempted.ts create mode 100644 scripts/tests/fixtures/event-registration-not-net.ts create mode 100644 scripts/tests/fixtures/event-registration-paired.ts create mode 100644 scripts/tests/fixtures/event-registration-raw-once.ts create mode 100644 scripts/tests/fixtures/event-registration-sources.ts create mode 100644 scripts/tests/fixtures/event-registration-suppressed.ts create mode 100644 scripts/tests/fixtures/event-registration-unowned.ts create mode 100644 scripts/tests/fixtures/event-registration-unpaired.ts create mode 100644 scripts/tests/scope-bound-event-registration.test.ts 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); + }); +});