Skip to content

fix(stream): record the turn when a client hangs up mid-stream (#3488) - #3907

Open
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/stream-cancel-records-turn
Open

ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/stream-cancel-records-turn

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Closes the half of #3488 that is still open, rewritten against current master rather than replaying my earlier patch — master has since grown finalizeStream(), which does most of what that patch's finishStream() did, so the old diff no longer applies and should not.

What is left

finalizeStream() is called from flush(), and from transform() on a terminal event — the comment above it says why: "a client that closes right after the terminal event cancels the reader, and flush() never runs."

The transformer has no cancel. The Streams spec calls flush when the upstream ends and cancel when the readable is cancelled, one or the other and never both, so a client that goes away before any terminal event still records nothing: no usage row, no request detail, nothing in Recent Requests — while the provider has already generated, and been charged for, the partial answer.

The change is cancel() routed to the same finalizeStream(). Four lines of code; the rest is the comment explaining the runtime scope.

Runtime scope. This is the Node path. Every disconnect signal in Next descends from the response's 'close' event, which Bun's node:http ServerResponse does not emit on a client hangup, so under start:bun nothing cancels this stream and finalizeStream() still only ever runs from flush() — exactly as before. I would rather state that than let the fix look more complete than it is.

Verification

npx vitest@3 run --config tests/vitest.config.js tests/unit/stream-cancel-records-turn.test.js
→ 3 passed

(Both parts matter: without --config the @/ alias does not resolve, and vitest 2.x cannot load anything that reaches open-sse/translator/index.js.)

Checked by reverting: removing cancel() fails 2 of the 3 tests.

Two things I measured that are worth writing down, because both cost me time and both would mislead the next reader:

  • The tests keep a reader pumping for their whole body. This TransformStream is built without a readable queuing strategy, so its high-water mark is the default 0 and every writer.write() blocks until something reads. My first two attempts wrote and only then read, and deadlocked five seconds at a time — the failure looks like a hang in the code under test, and is not.
  • The third test does not pin the finalized guard, and says so in a comment. I removed the guard to check, and it still passed. It cannot fail: once transform() has seen [DONE] the readable is already closed, so cancel() is never invoked and there is no second call to suppress. The test still earns its place — it pins the user-visible property that a future cancel path firing unconditionally would break — but I am not claiming coverage I do not have.

…ua#3488)

createSSEStream recorded usage from flush() only. The Streams spec calls
flush when the upstream ends and cancel when the readable is cancelled --
one or the other, never both -- so a client that went away mid-answer
skipped recording entirely: no usage row, no request detail, nothing in
Recent Requests, even though the provider had already generated (and been
charged for) the partial answer.

finalizeStream() already exists and is idempotent, and transform() already
calls it on a terminal event for clients that close right after the answer.
The remaining hole is the disconnect that arrives before any terminal
event. Add cancel() and route it to the same function.

Node path only: every disconnect signal in Next descends from the
response's 'close', which Bun's node:http ServerResponse does not emit on a
client hangup, so under start:bun nothing cancels this stream and
finalizeStream() still only runs from flush().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant