fix: clean up request context on stream error in deferUntilStreamConsumed#760
fix: clean up request context on stream error in deferUntilStreamConsumed#760NathanDrake2406 wants to merge 1 commit intocloudflare:mainfrom
Conversation
…umed deferUntilStreamConsumed's pull() handler had no error path — when the upstream RSC/SSR stream errored mid-consumption, the onFlush callback (clearRequestContext) was never invoked, leaking per-request state. The cancel hook handled client disconnect and the flush hook handled normal completion, but stream errors were uncovered. Adds an error handler that calls once() before propagating the error.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The error path in pull() was the only missing cleanup branch — flush (normal completion) and cancel (client disconnect) were already handled. The once() idempotency guard makes the three paths safely composable.
Tests are clear and cover both the mid-stream error and immediate error cases. All 11 tests pass.
One minor observation below, but not blocking.
| }, | ||
| (error) => { | ||
| once(); | ||
| controller.error(error); |
There was a problem hiding this comment.
Nit: after controller.error(error), the reader from the piped TransformStream is left in a locked state without being explicitly cancelled. In practice this is fine because the outer ReadableStream is now errored and will never call pull() or cancel() again, so the reader (and the underlying pipe) will be GC'd. But if you wanted to be defensive about releasing the lock eagerly, you could add reader.cancel() after controller.error(error). Not blocking — the current behavior is correct.
| it("calls onFlush only once when the stream errors then is cancelled", async () => { | ||
| const onFlush = vi.fn(); | ||
| const streamError = new Error("stream error"); | ||
|
|
||
| // Error on the very first pull — simulates immediate failure. | ||
| const source = new ReadableStream<Uint8Array>({ | ||
| pull(controller) { | ||
| controller.error(streamError); | ||
| }, | ||
| }); | ||
|
|
||
| const wrapped = deferUntilStreamConsumed(source, onFlush); | ||
| const reader = wrapped.getReader(); | ||
|
|
||
| // Reading the errored stream triggers the error handler. | ||
| await expect(reader.read()).rejects.toThrow("stream error"); | ||
|
|
||
| // The idempotent once() guard prevents double invocation — even if | ||
| // some code path triggered cleanup again, onFlush fires exactly once. | ||
| expect(onFlush).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
The test name says "errors then is cancelled" but the test doesn't actually exercise a cancel-after-error sequence — it only tests an immediate error on first pull and verifies onFlush is called once. The test is still valuable (it covers the immediate-error path and the once() guard), but the name is slightly misleading. Consider renaming to something like "calls onFlush exactly once on immediate stream error" to match what's actually being asserted.
|
Review submitted. Here's a summary: Approved the PR. The fix is correct and well-scoped:
I left two non-blocking comments:
|
2 participants
Summary
deferUntilStreamConsumed'spull()handler had no error path for upstream stream errorsclearRequestContext()was never called, leaking per-request state (headers, cookies, navigation context)Test plan