Don't cite a released import id after the reference has settled - #267
Open
Kanishkrawatt wants to merge 1 commit into
Open
Don't cite a released import id after the reference has settled#267Kanishkrawatt wants to merge 1 commit into
Kanishkrawatt wants to merge 1 commit into
Conversation
`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 cloudflare#265
🦋 Changeset detectedLatest commit: aa6182b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #265.
The problem
ImportTableEntry.resolve()stores the resolution and immediately callssendRelease(), so the import is dead on both sides. The entry keepsimportId— it has to, because the release accounting names the id through it — butgetImport()still returned that id unconditionally.So passing a settled
RpcPromiseback as an argument re-serialized a released id. The peer cannot find it and throws insidereadLoop. SincereadLoopis wrapped in a single session-wide.catch(err => this.abort(err)), a call-level fault tears down the whole session, not just the offending call.We hit this in production on a connection that multiplexes every service in an Electron desktop app: one stale reference took the entire connection down and the user got a "connection lost" screen.
The fix
getImport()declines a settled entry, so the caller exports a fresh stub instead. This is the same testdispose(),abort()andonBroken()already apply — they all branch onresolution;getImport()was the one that did not.The guard gates the use of
importIdrather than clearing it, so release accounting is unchanged. A test covers that specifically, because a guard that cleared the id would still emit a release frame — just one namingnull— and silently leak the peer's exports.awaitResolution()gets the same guard. It is not reachable with a settled entry today:RpcImportHook.pull()returns onentry.resolutionone line earlier, andsendStreamseedsactivePullviapulling = true, so thesendPullbranch never runs. It is defensive only — included so a future caller cannot regress into apullnaming a released id. Happy to drop it if you would rather not carry an unreachable branch.Tests
Two tests in
__tests__/index.test.ts. The first is load-bearing — verified to fail on unpatchedmainwithno such entry on exports table, and to pass with the fix. The second guards against the clear-the-id mis-fix described above.Full suite on this branch:
vitest run --project=nodevitest run --project=workerdnpm run test:typesI did not run the browser projects or
test:bunlocally (Playwright download blocked behind a corporate proxy); neither covers this path, but CI will.Scope note
There is a second, separate defect where the settled resolution is a non-promise stub — the library then cannot pull it and the call fails with
Tried to resolve a non-promise stub. That is unchanged by this PR and is out of scope here; this change only stops that case from being session-fatal rather than call-level. Reported separately in #266, which is aboutreadLoop's missing per-frame error boundary.Present in 0.10.0 and still in 0.12.0, so upgrading is not a workaround — we are currently carrying this as a local patch against the published bundle, which is what prompted the report.