From 209acb54311c9414e2bd01078ff305f39c8431c5 Mon Sep 17 00:00:00 2001 From: Taras Mankovski <74687+taras@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:05:11 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=A4=96=20Add=20Scope-Bound=20Event=20?= =?UTF-8?q?Registration=20policy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Event listeners must be registered with on() using named handlers and removed with off() in the owning scope's teardown; never with once(), whose deregistration depends on the event firing. Captures cowboyd's review guidance from #247, including the ordering constraint discovered applying it: when teardown awaits a listener-resolved value, removal must come after that wait. Recommended rather than Experimental because the rule was stated explicitly and enforced in live review, like Async Teardown. --- .policies/index.md | 1 + .policies/scope-bound-event-registration.md | 124 ++++++++++++++++++++ AGENTS.md | 5 + 3 files changed, 130 insertions(+) create mode 100644 .policies/scope-bound-event-registration.md diff --git a/.policies/index.md b/.policies/index.md index c1fb9ba6..1a5ac878 100644 --- a/.policies/index.md +++ b/.policies/index.md @@ -21,6 +21,7 @@ This is the **single source of truth** for all policies in this repository. | [Stateless Stream Operations](./stateless-streams.md) | Recommended | Use `*[Symbol.iterator]` pattern for reusable stream operations | | [Version Bump](./version-bump.md) | Recommended | PRs changing package code must include a semantic version bump | | [Async Teardown](./async-teardown.md) | Recommended | Teardown that needs `yield*` must use `ensure()`, not a `finally` block | +| [Scope-Bound Event Registration](./scope-bound-event-registration.md) | Recommended | Listener lifetime depends on a scope, never on the event firing; no `once()` | | [Package.json Metadata](./package-json-metadata.md) | Strict | Every published package must include a description field | | [No Agent Marketing](./no-agent-marketing.md) | Strict | No AI tool promotional material in commits, PRs, issues, or comments | | [Code Comments](./code-comments.md) | Strict | Comments say what the code cannot; delete the ones that restate it | diff --git a/.policies/scope-bound-event-registration.md b/.policies/scope-bound-event-registration.md new file mode 100644 index 00000000..1b14ff37 --- /dev/null +++ b/.policies/scope-bound-event-registration.md @@ -0,0 +1,124 @@ +# Scope-Bound Event Registration Policy (Recommended) + +This document defines the recommended policy for registering event listeners on +emitters (Node `EventEmitter`, streams, `EventTarget`) inside Effection code. + +## Core Principle + +**An event listener's lifetime must depend solely on a scope, never on its event +firing.** + +## The Rule + +| Case / Condition | Required behavior | +| ------------------------------------------------------- | ------------------------------------------------------------------------ | +| Registering any listener | Use `.on()` / `.addEventListener()` with a **named** handler | +| `emitter.once(...)` or `{ once: true }` | Never; convert to `.on()` plus scope-bound removal | +| Removing the listener | `.off()` in the scope's own teardown (`finally` for sync-only, `ensure()` otherwise) | +| Teardown waits on a value the listener resolves | Deregister **after** that wait, in the same `ensure()` | + +The `once()` *operation* from `@effectionx/node/events` is compliant and +unaffected: it removes its listener when its scope exits, whether or not the +event ever fired. This policy is about `EventEmitter.prototype.once` and the +`{ once: true }` listener option. + +### Why + +`emitter.once()` deregisters the hook only when the event fires. If the event +never fires — the process never errors, the stream is destroyed instead of +ending, the scope is halted first — the listener outlives the scope that +created it. On emitters shared beyond a single owner (I/O pipes are the +canonical case: a child's stdio may be inherited by descendants and observed by +more than one consumer) the leaked listener keeps firing into a dead scope, +resolving resolvers nobody is waiting on and holding referenced objects alive. + +Symmetric registration and removal on scope entry and exit makes listener +lifetime deterministic — the same principle +[Structured Concurrency](./structured-concurrency.md) applies to tasks, applied +to event hooks. + +## Examples + +### Compliant: named handlers, removed in the scope's teardown + +```typescript +function useConnection(url: string): Operation { + return resource(function* (provide) { + let socket = connect(url); + let onMessage = (message: Message) => inbox.send(message); + socket.on("message", onMessage); + try { + yield* provide(ConnectionHandle(socket)); + } finally { + socket.off("message", onMessage); // sync-only: `finally` is fine + } + }); +} +``` + +### Compliant: teardown waits on a listener-resolved value first + +```typescript +function useChildProcess(create: () => ChildProcess): Operation { + return resource(function* (provide) { + let result = withResolvers>(); + let child: ChildProcess | undefined; + + let onClose = (code: number | null) => result.resolve(Ok(ExitStatus(code))); + + yield* ensure(function* () { + if (child) { + yield* result.operation; // needs `onClose` still attached + child.off("close", onClose); // removal ordered after the wait + } + }); + + child = create(); + child.on("close", onClose); + yield* provide(NativeProcess(child, result.operation)); + }); +} +``` + +### Non-Compliant: `once()` ties removal to the event firing + +```typescript +child.once("error", (error) => { + result.resolve(Err(error)); // VIOLATION: if "error" never fires, the +}); // listener outlives the scope +``` + +### Non-Compliant: registered but never removed + +```typescript +stream.on("data", (chunk) => signal.send(chunk)); // VIOLATION: no `.off()` on +yield* provide(handle); // any teardown path; leaks on +// emitters shared beyond this scope +``` + +## Verification Checklist + +Before marking a review complete, verify: + +- [ ] No `emitter.once(...)` or `{ once: true }` registration in the diff +- [ ] Every `.on()` / `.addEventListener()` has a named handler and a matching + `.off()` / `.removeEventListener()` on a teardown path of the same scope +- [ ] Removal that follows a `yield*` lives in `ensure()`, not `finally` + (see [Async Teardown](./async-teardown.md)) +- [ ] When teardown awaits a listener-resolved value, removal comes after the wait + +## Common Mistakes + +| Mistake | Fix | +| ------------------------------------------------------------ | ------------------------------------------------------------------- | +| `emitter.once("close", handler)` | `emitter.on("close", handler)` + `.off()` in the scope's teardown | +| Anonymous inline listener | Name it — `.off()` needs the same function reference | +| `.off()` in a `finally` while an `ensure()` still awaits the event | Move removal into that `ensure()`, after the wait | +| Skipping removal because "the emitter dies with the process" | Remove anyway; the emitter may be shared beyond this scope | + +## Related Policies + +- [Async Teardown](./async-teardown.md) - Where removal may live: `finally` only if sync-only +- [Structured Concurrency](./structured-concurrency.md) - The same lifetime principle, for tasks +- [Correctness Through Explicit Invariants](./correctness-invariants.md) - The never-fires path must be considered +- [Policies Index](./index.md) - Add your new policy to the Policy Documents table diff --git a/AGENTS.md b/AGENTS.md index 43f82624..796c4c20 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -78,6 +78,11 @@ When reviewing PRs: yields, the resume comes back as `iterator.next()`, which takes the frame out of return-mode and the halt is lost. Synchronous cleanup in `finally` is fine. See the [Async Teardown policy](.policies/async-teardown.md) +- **Never register event listeners with `emitter.once()`; listener lifetime must depend + on a scope, not on the event firing.** Register named handlers with `.on()` and remove + them with `.off()` in the scope's own teardown — after any teardown wait that needs the + listener still attached. See the + [Scope-Bound Event Registration policy](.policies/scope-bound-event-registration.md) - Prefer `Operation` for async operations - Use `*[Symbol.iterator]` pattern for reusable stream operations (see Stateless Streams policy) - Avoid `sleep()` for test synchronization (see No-Sleep Test Sync policy) From 3d34bab21e1571acaf627da9f6cde43e68da9521 Mon Sep 17 00:00:00 2001 From: Taras Mankovski <74687+taras@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:13:59 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=A4=96=20Harden=20the=20ordered-remov?= =?UTF-8?q?al=20example=20with=20a=20sync=20finally?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deregistration ordered after a teardown wait must not depend on that wait completing; a synchronous finally inside the ensure gives both the ordering and the guarantee. --- .policies/scope-bound-event-registration.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.policies/scope-bound-event-registration.md b/.policies/scope-bound-event-registration.md index 1b14ff37..79c9d0c9 100644 --- a/.policies/scope-bound-event-registration.md +++ b/.policies/scope-bound-event-registration.md @@ -15,7 +15,7 @@ firing.** | Registering any listener | Use `.on()` / `.addEventListener()` with a **named** handler | | `emitter.once(...)` or `{ once: true }` | Never; convert to `.on()` plus scope-bound removal | | Removing the listener | `.off()` in the scope's own teardown (`finally` for sync-only, `ensure()` otherwise) | -| Teardown waits on a value the listener resolves | Deregister **after** that wait, in the same `ensure()` | +| Teardown waits on a value the listener resolves | Deregister in a sync `finally` around that wait, in the same `ensure()` | The `once()` *operation* from `@effectionx/node/events` is compliant and unaffected: it removes its listener when its scope exits, whether or not the @@ -68,8 +68,11 @@ function useChildProcess(create: () => ChildProcess): Operation { yield* ensure(function* () { if (child) { - yield* result.operation; // needs `onClose` still attached - child.off("close", onClose); // removal ordered after the wait + try { + yield* result.operation; // needs `onClose` still attached + } finally { + child.off("close", onClose); // after the wait, but not dependent + } // on it completing — sync finally } }); @@ -105,7 +108,9 @@ Before marking a review complete, verify: `.off()` / `.removeEventListener()` on a teardown path of the same scope - [ ] Removal that follows a `yield*` lives in `ensure()`, not `finally` (see [Async Teardown](./async-teardown.md)) -- [ ] When teardown awaits a listener-resolved value, removal comes after the wait +- [ ] When teardown awaits a listener-resolved value, removal sits in a sync + `finally` around that wait — ordered after it, but not dependent on it + completing ## Common Mistakes @@ -113,7 +118,7 @@ Before marking a review complete, verify: | ------------------------------------------------------------ | ------------------------------------------------------------------- | | `emitter.once("close", handler)` | `emitter.on("close", handler)` + `.off()` in the scope's teardown | | Anonymous inline listener | Name it — `.off()` needs the same function reference | -| `.off()` in a `finally` while an `ensure()` still awaits the event | Move removal into that `ensure()`, after the wait | +| `.off()` in a `finally` while an `ensure()` still awaits the event | Move removal into that `ensure()`, in a sync `finally` around the wait | | Skipping removal because "the emitter dies with the process" | Remove anyway; the emitter may be shared beyond this scope | ## Related Policies