cloudflare / cloudflare/capnweb
onRpcBroken callbacks are retained for the session's lifetime after their import is disposed (and fire at teardown)
- Dominant language
- TypeScript
- Stars
- 4k
- Forks
- 143
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 7
Description
**Version:** capnweb 0.10.0 (tag `92baaed`), Node 24.
**Summary:** an `onRpcBroken` callback registered on an import entry whose `resolution` is unset — which includes ordinary remote capability stubs, not just pending promises — is retained in the session-level `RpcSessionImpl.onBrokenCallbacks` list after the capability is fully disposed and released. The callback then fires at session teardown with the teardown error, long after the capability it observed was cleanly disposed. Since session abort doesn't clear the list either, the closures stay reachable for as long as the `RpcSessionImpl` remains reachable.
**Mechanism** (at the tag, `src/rpc.ts`):
- `ImportTableEntry.onBroken()` pushes the callback onto `session.onBrokenCallbacks` and records the index in the entry's `onBrokenRegistrations`.
- On final disposal of an entry with no resolution, `dispose()` → `abort()` sets `this.onBrokenRegistrations = undefined` **without deleting the corresponding entries from `session.onBrokenCallbacks`**, then `sendRelease()` removes the entry.
- This is asymmetric with `resolve()`, which explicitly reconciles the registrations: it deletes the old session slot when the callback moves elsewhere, or removes a redundant newly-added same-session slot while preserving the original ordering. The disposal path performs no equivalent cleanup.
- The comment in `abort()` ("The RpcSession itself will have called all our callbacks") holds when the *session* aborts the entry, but not when local `dispose()` calls `abort()` first.
**Observable consequences:**
1. One strong callback reference (plus anything the closure strongly captures) accumulates per registration for as long as the `RpcSessionImpl` is reachable — unbounded on a long-lived session with capability churn.
2. Callbacks belonging to cleanly-disposed capabilities still fire at session teardown, and with the teardown error rather than any disposal-related reason.
3. The retention is invisible to `getStats()`, which reports only the import/export tables — those return to baseline.
**Reproduction** (self-contained, `node repro.mjs` in a project with capnweb 0.10.0 installed):
```js
const { RpcSession, RpcTarget } = await import("capnweb");
// Trivial in-memory transport pair.
function transportPair() {
const mk = () => ({ queue: [], waiter: null, closed: null });
const a = mk(), b = mk();
const side = (inbox, outbox) => ({
send(message) {
if (outbox.waiter) { outbox.waiter.resolve(message); outbox.waiter = null; }
else outbox.queue.push(message);
},
async receive() {
if (inbox.queue.length > 0) return inbox.queue.shift();
if (inbox.closed) throw inbox.closed;
inbox.waiter = Promise.withResolvers();
return inbox.waiter.promise;
},
abort(reason) {
inbox.closed = reason;
if (inbox.waiter) { inbox.waiter.reject(reason); inbox.waiter = null; }
},
breakNow(reason) { this.abort(reason); },
});
return [side(a, b), side(b, a)];
}
class Thing extends RpcTarget { poke() { return "ok"; } }
class Main extends RpcTarget { getThing() { return new Thing(); } }
const [clientTransport, serverTransport] = transportPair();
void new RpcSession(serverTransport, new Main());
const client = new RpcSession(clientTransport);
const main = client.getRemoteMain();
console.log("baseline stats:", client.getStats());
let disposedFires = 0;
for (let i = 0; i < 6; i += 1) {
const thing = await main.getThing(); // fresh capability, one handle
thing.onRpcBroken(() => { disposedFires += 1; });
await thing.poke();
thing[Symbol.dispose](); // fully released
}
await new Promise((r) => setTimeout(r, 50));
console.log("stats after 6 acquire/dispose cycles:", client.getStats()); // back to baseline
console.log("fires while session healthy:", disposedFires); // 0
clientTransport.breakNow(new Error("connection lost"));
await new Promise((r) => setTimeout(r, 100));
console.log("fires at session death, all from disposed imports:", disposedFires); // 6
```
Observed output: `getStats()` returns to `{ imports: 1, exports: 1 }` after the six acquire/dispose cycles, zero callbacks fire while the session is healthy, and **all six** fire when the transport later breaks.
**Why this looks like a bug rather than intent:** the README describes `onRpcBroken` as monitoring a stub's "brokenness" and says the callback runs if something causes all further method calls and property accesses on that stub to throw. The follows-the-resolution behavior (which the test suite pins, and which `resolve()`'s session-array reconciliation supports) stops applying once the capability is fully released. Either defensible disposal semantic — unregister on disposal, or treat disposal as breakage and fire immediately with the disposal reason — differs from the current behavior of staying silent at disposal and firing much later with the later session-abort error. No test or example appears to pin the current post-disposal late fire.
**Fix sketch:** mirror `resolve()`'s cleanup on final disposal — delete the entry's registrations from `session.onBrokenCallbacks` before discarding `onBrokenRegistrations`. One care point: the deletion should distinguish healthy local disposal from session-abort reentrancy (a callback that disposes another entry mid-iteration must not delete callbacks that haven't fired yet), and existing tests pin global registration order across entries.
**Context:** found while building a subscription surface (client-passed observer capabilities, one `onRpcBroken` per subscription for reaping) on a long-lived Unix-socket session. Our registrations are bounded per connection and the callbacks idempotent, so this isn't urgent for us — filing for correctness. Checked for prior reports: #153/#168 (receive-loop retention, fixed in 0.10.0), #154, #151, and #172 are adjacent but concern different lifecycles.
Contributor guide
Research direction
Run the self-contained node repro.mjs to observe the retained callbacks and late teardown fires. Then read src/rpc.ts, focusing on ImportTableEntry.onBroken(), resolve(), abort(), and dispose(); done means disposed imports no longer retain or later fire callbacks while session-abort ordering and existing registration behavior remain intact.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, backend-api-design
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100