cloudflare / cloudflare/capnweb
A single undecodable inbound frame aborts the whole session: `readLoop` has no per-frame error boundary
- Dominant language
- TypeScript
- Stars
- 4k
- Forks
- 143
- Avg merge
- 4d 6h
- Merged PRs (30d)
- 7
Description
### Summary
`readLoop` decodes every inbound frame under a single session-wide error handler, so any one frame that fails to decode takes down the whole session: every outstanding import is rejected and every export disposed. A per-frame boundary would make most of these failures cost one call instead of the session.
Observed on `0.12.0`.
### Where
`readLoop` (`dist/index.js:2236`) is a bare `while` loop around a `switch` on the frame type, and the only handler is at the call site:
```js
this.readLoop().catch((err) => this.abort(err)) // :1929
```
Anything any arm throws reaches `abort()`. Concretely:
- A bad export id in a `push` argument: `no such entry on exports table` (`:1586`, `:1618`, `:1628`).
- A duplicate or underflowing `release`: `no such export ID` (`:1969`, `:1983`) and `refcount would go negative` (`:1970`).
- An over-limit value: `maxDepth` (`:1437`) and `maxBigIntDigits` (`:1451`).
### Repro
Any peer bug that emits an undecodable frame will do. The one we hit is #265, a distinct defect in `getImport()`; its transcript shows the shape, where a single `["pipeline",1]` naming a released id ends the session:
```
client -> ["push",["pipeline",0,["use"],[["pipeline",1]]]]
client -> ["pull",3]
server -> ["abort",["error","Error","no such entry on exports table: 1"]]
SESSION DEAD: no such entry on exports table: 1
```
A limit violation behaves the same way, which is easier to reproduce standalone:
```js
const srv = new RpcSession(s.t, new Api(), { limits: { maxDepth: 8 } })
const cli = new RpcSession(c.t)
const api = cli.getRemoteMain()
await api.getThing() // fine
let deep = {}, cur = deep
for (let i = 0; i < 40; i++) { cur.a = {}; cur = cur.a }
await api.use(deep) // TypeError: Deserialization exceeded maximum allowed message depth of 8.
await api.getThing() // SESSION DEAD, same error
```
We understand the limits case is deliberate: `RpcSessionOptions.limits` documents it as "A message that exceeds a limit is rejected, aborting the session" and describes the limits as guarding "against resource-exhaustion attacks from untrusted peers" (`dist/index.d.ts:279-285`). It is included here only because it demonstrates the single handler, not because we think the documented behaviour is wrong. See the last section for why that makes this an opt-in question.
### Suggested fix
A boundary around each `switch` arm, so a frame that fails to decode fails only that frame, settling the named import with an error stub rather than aborting. Two things make this safer than it may sound:
1. **Nothing leaks.** `evaluateWithDepth` (`:1422`) already does `catch (err) { payload.dispose(); throw err }`, so the hooks and imports a failed decode created are released. That is existing behaviour, not something a boundary would need to add.
2. **Positional ids stay aligned if the arm still allocates.** A contained `push` or `stream` frame must *still* push its slot, or every subsequent id is off by one, which is a considerably worse failure than the abort. Any fix wants a test that fails if the allocation is dropped, and that asserts *subsequent* calls still resolve, not merely that the bad call rejects.
Arms differ in what containment should mean, which is the part worth agreeing on before writing code:
- **`push` / `stream`:** allocate the slot, install an error stub in it.
- **`pull`:** answer the peer with `["reject", exportId, ...]` rather than swallow. That frame is already in the vocabulary (emitted at `:2013` and `:2021`, handled at `:2294`), so an unpatched peer needs nothing new. Swallowing instead would leave a peer awaiting a resolution that never arrives.
- **`release`:** drop the error. Both throws precede any mutation (`entry.refcount -= refcount` is `:1971`), so containment leaves refcounts exactly as they were. The cost is one leaked export until the session ends, and no id-space disturbance, since `releaseExport` never appends.
- **`resolve` / `reject`:** settle the named import with an error stub, guarded with `if (imp)`. The no-import branch is reachable, and an unguarded settle there would be a `TypeError` inside the catch.
- **`abort`:** still abort, but with the peer's undecodable reason as `cause`, instead of masking why the peer gave up with our own decode error.
- **Everything outside the arm switch** (transport `receive()` failures, the top-level `maxMessageSize` check at `:2246`) stays fatal.
### The design question
Softening decode failures from session-fatal to call-fatal removes the coarse limits as a backstop, which is exactly the posture the `limits` doc comment describes for untrusted peers. Our own peers are all first-party processes we launch, so per-call rejection is strictly better for us, but that is not true for everyone, and `maxMessageSize` is only checked when `encodingLevel === "string"` (`:2246`) so it is not a backstop on the structured-clonable transports anyway.
So this may be better as opt-in, for instance a flag on `RpcSessionOptions` alongside `onSendError` and `limits`, leaving the default abort-on-decode-failure behaviour untouched. Happy to open a PR against whichever shape you prefer.
Contributor guide
Research direction
Start with readLoop in dist/index.js:2236 and its call site at :1929, then read the limits documentation in dist/index.d.ts:279-285. Trace the push, stream, pull, release, resolve, reject, and abort arms and the existing evaluateWithDepth cleanup. Done requires an agreed default or opt-in policy plus tests proving contained failures settle correctly and later calls still resolve.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100