Make the SSE reader emit exactly one terminal outcome; add ModelUsage parity - #14
Merged
Merged
Conversation
readSSEStream fired callbacks opportunistically rather than modelling a terminal state, which produced four distinct defects: - A terminal chunk carrying both `error` and `done` invoked onComplete and onError, reporting the same failed run as both success and failure. - When the body closed after a `done` chunk, onComplete fired a second time. - An aborted stream broke out of the loop and invoked no callback at all, leaving optimistic messages pending forever. - A stream that closed without any terminal event was reported as success, even though the run's outcome was unknown. Every exit path now settles through a single guard that delivers exactly one of onComplete, onError, or the new onAbort, and reading stops as soon as a terminal chunk arrives. A body that ends without a terminal event is an error. The terminal chunk is still handed to onChunk first, since consumers read messageId and conversationId from it. useChat had matching problems. It classified error chunks in onChunk while also receiving onError from the reader, so consumers saw every failure twice; that duplicate is removed. Failed and cancelled runs now settle both optimistic messages instead of leaving them pending: the user message is finalized, because the server persists it before streaming, and the partial assistant message is dropped, because the server discards partial output and keeping it would vanish on reload. The settle helper is keyed off the temp IDs rather than streamingMessageRef, since stopStream() clears that ref before the abort reaches the handler, and it is declared before the request so an abort during fetch settles too. Also adds ModelUsage to the React types to close a protocol gap: Python's StreamChunk has carried a `usage` field that the TypeScript contract never exposed. total_tokens is a computed property that does not cross the wire, so it ships as a totalTokens() helper. One existing test asserted that a stream closing with no terminal event calls onComplete. That encoded the bug, so it is replaced by tests for both the correct success path and the early-close error.
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.
Second of the two P0 follow-up PRs. Independent of #13 — this touches only
packages/llmpane-react, so the two can merge in either order.The SSE reader had no terminal state
readSSEStreamfired callbacks opportunistically as it walked the buffer, which produced four distinct defects:erroranddoneonCompleteandonError— the same failed run reported as success and failuredonechunkonCompletefires a second timeEvery exit path now settles through a single guard that delivers exactly one of
onComplete,onError, or the newonAbort, and reading stops as soon as a terminal chunk arrives. A body ending without a terminal event is an error. The terminal chunk is still delivered toonChunkfirst, since consumers readmessageIdandconversationIdoff it.useChat had matching problems
It classified error chunks inside
onChunkwhile also receivingonErrorfrom the reader, so consumers saw every failure twice. That duplicate is removed.Failed and cancelled runs also left both optimistic messages pending forever. They now settle deliberately:
Two subtleties the tests forced out:
streamingMessageRef, becausestopStream()clears that ref before the abort reaches the handler. Keying off the ref left the assistant bubble pending — caught by the cancellation test.fetch(rather than mid-stream) settles too.Protocol parity
Python's
StreamChunkhas carried ausage: ModelUsagefield that the TypeScript contract never exposed.ModelUsageis now part of the React types and exported from the package entry point.total_tokensis a computed property on the Python model and does not cross the wire, so it ships as atotalTokens(usage)helper rather than a phantom field.Verification
ModelUsageandtotalTokensactually appear in the builtdist/index.d.ts.errorInfo, early EOF, empty body, non-ok response, malformed events,[DONE]marker, a terminal split across a trailing partial line, and cancellation.One existing test changed, deliberately.
"calls onComplete when stream ends"fed a stream with no terminaldonechunk and assertedonComplete— it encoded the bug. It is replaced by two tests: one for the real success path, one asserting an early close is an error.Compatibility
onAbortis additive and optional. The behavioural changes are the point of the PR, and worth noting for anyone upgrading:onChunk, you were previously gettingonErroras well; that double-call is gone.onAbortinstead of nothing.Checked against a production consumer: it passes neither
onErrornoronComplete, reading only the returnederrorstate and callingstopStream(), so this is a strict improvement there with no API break — cancelled runs stop leaving permanently pending bubbles in its message list.