From aa6182bac047d4fe8109273f794e7967842b1d9d Mon Sep 17 00:00:00 2001 From: Kanishk Rawat Date: Thu, 10 Sep 2026 00:16:39 +0530 Subject: [PATCH] Don't cite a released import id after the reference has settled `ImportTableEntry.resolve()` stores the resolution and immediately calls `sendRelease()`, so the import is dead on both sides -- but the entry keeps `importId`, because the release accounting names the id through it. `getImport()` nevertheless still returned that id, so passing a settled `RpcPromise` back into a later message re-serialized a released id. The peer cannot find it and throws inside `readLoop`, and since `readLoop` is wrapped in a single session-wide `.catch(err => this.abort(err))`, a call-level fault tore down the entire session. `getImport()` now declines a settled entry, so the caller exports a fresh stub instead. This matches `dispose()`, `abort()` and `onBroken()`, which already branch on `resolution`. The guard gates the *use* of `importId` rather than clearing it, so release accounting is untouched. `awaitResolution()` gets the same guard. It is not reachable with a settled entry today -- `RpcImportHook.pull()` returns on `entry.resolution` one line earlier, and `sendStream` seeds `activePull` via `pulling = true` -- so it is defensive only, and prevents a future caller from regressing into a `pull` that names a released id. Fixes #265 --- .changeset/settled-import-guard.md | 14 ++++++++++ __tests__/index.test.ts | 41 ++++++++++++++++++++++++++++++ src/rpc.ts | 11 +++++++- 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 .changeset/settled-import-guard.md diff --git a/.changeset/settled-import-guard.md b/.changeset/settled-import-guard.md new file mode 100644 index 0000000..e61faaa --- /dev/null +++ b/.changeset/settled-import-guard.md @@ -0,0 +1,14 @@ +--- +"capnweb": patch +--- + +Don't cite a released import id after the reference has settled. + +`ImportTableEntry.resolve()` stores the resolution and immediately calls `sendRelease()`, so the +import is dead on both sides — but the entry keeps `importId`, since the release accounting names +the id through it. `getImport()` still returned that id, so passing a settled `RpcPromise` back as +an argument re-serialized a released id. The peer could not find it and threw inside `readLoop`, +which is wrapped in a single session-wide `.catch(err => this.abort(err))` — so a call-level fault +destroyed the whole session. `getImport()` now declines a settled entry and the caller exports a +fresh stub instead, matching `dispose()`, `abort()` and `onBroken()`, which already branch on +`resolution`. diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index 01bfd65..c52c011 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -4227,3 +4227,44 @@ describe("deserialization and transport correctness", () => { expect(sentReason).toBe("a".repeat(MAX_CLOSE_REASON_BYTES - 1)); }); }); + +describe("settled import references", () => { + // Regression: ImportTableEntry.resolve() stores `resolution` and immediately calls + // sendRelease(), so the import id is dead on both sides -- but the entry keeps `importId`, + // because the release accounting names the id through it. getImport() then still handed out + // that dead id, so passing the settled promise object into a later message re-serialized a + // released id. The peer could not find it and threw inside readLoop, and because readLoop is + // wrapped in a single session-wide `.catch(err => this.abort(err))`, a call-level fault + // destroyed the entire session. + // + // dispose(), abort() and onBroken() all already branch on `resolution`; getImport() did not. + it("does not cite a released import id after the reference has settled", async () => { + let harness = new TestHarness(new TestTarget()); + + // Awaiting settles the entry: the resolution is stored and the import is released. + let promise = harness.stub.returnNumber(7); + await promise; + + // Passing the same *promise object* (not its awaited value) as an argument is what + // re-serialized the now-dead id. + expect(await harness.stub.square(promise)).toBe(49); + + // The load-bearing assertion: a failing call would be tolerable, a dead session is not. + expect(await harness.stub.returnNumber(3)).toBe(3); + + harness.stub.dispose(); + }); + + // A guard that cleared `importId` instead of gating its use would stop the release + // accounting naming the id, leaking the peer's exports. + it("still releases settled imports by id", async () => { + let harness = new TestHarness(new TestTarget()); + + await harness.stub.returnNumber(1); + await harness.stub.returnNumber(2); + + expect(await harness.stub.returnNumber(3)).toBe(3); + + harness.stub.dispose(); + }); +}); diff --git a/src/rpc.ts b/src/rpc.ts index 8896fec..8eeb354 100644 --- a/src/rpc.ts +++ b/src/rpc.ts @@ -247,6 +247,11 @@ class ImportTableEntry { } async awaitResolution(): Promise { + // If the entry has already settled, the import has been released (resolve() calls + // sendRelease()), so there is nothing left on the wire to pull. Read the stored + // resolution instead of sending a "pull" naming a released id. + if (this.resolution) return this.resolution.pull(); + if (!this.activePull) { this.session.sendPull(this.importId); this.activePull = Promise.withResolvers(); @@ -662,7 +667,11 @@ class RpcSessionImpl implements Importer, Exporter { } getImport(hook: StubHook): ImportId | undefined { - if (hook instanceof RpcImportHook && hook.entry && hook.entry.session === this) { + // A settled entry has already released its import (resolve() calls sendRelease()), so its + // importId no longer names anything on the peer. Fall through to exporting the resolution, + // the way dispose(), abort() and onBroken() all branch on `resolution`. + if (hook instanceof RpcImportHook && hook.entry && hook.entry.session === this && + !hook.entry.resolution) { return hook.entry.importId; } else { return undefined;