onRpcBroken callbacks are retained for the session's lifetime after their import is disposed (and fire at teardown)

[martijnwalraven]

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):

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.