Accept an AbortSignal in onRpcBroken and cleanup registrations when stubs are disposed - #250

🦋 Changeset detected

Latest commit: f956cb4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@250

commit: f956cb4

/bonk review this

Review: AbortSignal support in onRpcBroken + dispose cleanup

Reviewed the full diff, built the project, and ran the node test project (all 386 tests pass, including the 6 anySignal tests and 8 onRpcBroken tests). This is a clean, well-scoped change. The design rationale in the PR description (signal over Disposable) is sound and matches the ecosystem convention for event-listener cancellation.

What I like

• Correct "already aborted registers nothing" semantics across every hook that stores or synchronously invokes the callback (ErrorStubHook, PromiseStubHook, ImportTableEntry). The throwError case in the tests (an already-broken stub that normally reports synchronously) is a nice edge case to cover.
• Per-hook abort controller in RpcImportHook (not per-entry) is the right call — dups share the entry, so composing each hook's controller with the user signal via anySignal keeps disposal/cancellation independent per dup. The "honors independent signals on separate dups" test locks this in.
• resolve() migration handles a canceled-before-resolve callback correctly by skipping undefined slots, and the "honors the signal after the promise resolved" test covers migration-then-abort.
• anySignal fallback correctly propagates reason, fires once, and drops source listeners via { signal: controller.signal } so a long-lived source signal doesn't retain listeners. Good test coverage on both native and fallback paths.
• Docs (README + changeset) clearly call out the behavior change: disposing a stub now drops its callbacks, scoped per-stub.

Observations (non-blocking)

1. Stale listeners after resolve() migration. In ImportTableEntry.resolve(), when a registration is migrated onto the resolution, the deregister listener added in onBroken() is left attached to both this.abortController.signal and the (composite) options.signal. They're now stale — deregister would delete an already-deleted onBrokenCallbacks[index] slot, so it's harmless, and the entry's abortController firing on dispose() eventually removes them. Just noting it's a small amount of retained state until disposal; not a correctness issue.

2. anySignal composite listener retained on the entry controller. RpcImportHook.onBroken builds anySignal([this.abortController.signal, options.signal]). The composite registers a listener on the hook's abortController.signal; on hook dispose() that controller aborts, which fires the composite (aborting it) and — via { signal: controller.signal } — self-cleans. So this is handled correctly.

3. AbortController/AbortSignal availability. signal.ts guards only AbortSignal.any, assuming AbortController/AbortSignal exist. That's fine for the supported runtimes (browsers, workerd, Node 16+), just flagging the implicit baseline.

Testing note

The default vitest run requires workerd + Playwright browsers (not available in every CI/local env). The signal + onRpcBroken logic is plain JS and lives in the node project, which passes cleanly:

Test Files  29 passed (29)
Tests       386 passed (386)

LGTM. The observations above are minor and don't need to block merge.

Review posted to PR #250: #250 (comment)

Summary

I reviewed the full diff, built the project, and ran the node test project (386 tests pass, including the 6 anySignal and 8 onRpcBroken tests).

Verdict: LGTM. The change is clean, well-scoped, and thoroughly tested.

Key findings:

• Correct "already-aborted registers nothing" semantics across all relevant hooks (ErrorStubHook, PromiseStubHook, ImportTableEntry), including the tricky synchronous-report-when-already-broken case.
• Per-hook abort controller in RpcImportHook (rather than per-entry) is the right choice for independent per-dup cancellation, since dups share the underlying entry.
• resolve() migration correctly skips already-canceled callbacks.
• anySignal fallback correctly propagates reason, fires once, and self-cleans listeners.
• Docs/changeset clearly flag the behavior change (disposal now drops per-stub callbacks).

Non-blocking observations I noted: a small amount of stale-but-harmless listener state left on the entry after resolve() migration until disposal, and the implicit AbortController/AbortSignal runtime baseline. Neither needs to block merge.

One environment note: the default vitest run requires workerd + Playwright browsers which aren't available here, but the signal logic is plain JS covered by the node project, which passes fully.

github run

@srikrsna Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

No toolchain available, but the code analysis is complete.

Summary of review

The PR adds an optional AbortSignal to onRpcBroken and drops callbacks on stub disposal. I reviewed all 14 changed files in depth:

• signal.ts anySignal — correct: already-aborted short-circuit, single-fire, reason propagation, and self-cleaning listeners via { signal: controller.signal }.
• rpc.ts — the per-hook abortController (vs. per-entry) is the right choice for independent per-dup cancellation since dups share the entry. The resolve() migration correctly skips canceled slots and preserves registration ordering. The already-aborted-registers-nothing semantics are honored in ErrorStubHook, PromiseStubHook, and ImportTableEntry.
• core.ts / streams.ts / map.ts — signature threading is consistent; the synchronous-report-when-already-broken case in ErrorStubHook and PromiseStubHook correctly checks signal.aborted.
• Type/API surface (types.d.ts, index.ts, capnweb-validate) — OnRpcBrokenOptions is exported and mirrored in the validate package. Purely additive, no breaking changes.
• Changeset + README — present and accurately describe the behavior change (disposal now drops per-stub callbacks).

The only observations are non-blocking and already noted in the existing thread (stale-but-harmless deregister listeners retained on the entry after migration until disposal). No logic bugs, security issues, capability leaks, wire-protocol changes, or backward-compat violations found. The change is well-scoped and thoroughly tested.

I was unable to run the test suite (dependencies not installed in this environment), but the signal/onRpcBroken logic is plain JS and the reasoning holds.

LGTM

github run