Skip to content

fix(usage): record streaming requests that end before their flush - #3513

Open
ntdat812 wants to merge 1 commit into
decolua:masterfrom
ntdat812:fix/aborted-stream-usage-3488
Open

ntdat812 wants to merge 1 commit into
decolua:masterfrom
ntdat812:fix/aborted-stream-usage-3488

Conversation

@ntdat812

Copy link
Copy Markdown
Contributor

Addresses bug 1 of #3488 (aborted streaming requests are never recorded).
Bug 2 is not touched — see the note at the end.

Root cause

buildOnStreamComplete's callback is invoked from the SSE transform's flush,
and flush only runs on a clean upstream EOF. When the client goes away the
pipeline is cancelled (createDisconnectAwareStream.cancel → handleDisconnect
→ AbortController.abort()), so flush never runs and all three side effects —
saveRequestDetail, saveUsageStats, the 📊 done line — are skipped. The same
hole swallows a mid-stream upstream failure, which errors the pipeline instead.

The provider has already generated (and billed) the partial response, so usage
accounting silently under-counts, and Recent Requests looks frozen for clients
that cancel often.

Change

  1. createSSEStream exposes getStreamSnapshot() — the content, thinking and
    usage accumulated so far. Usage is resolved exactly as the two flush paths
    resolve it, including the estimateUsage(...) fallback when the upstream had
    not sent a usage chunk yet, so an aborted stream is accounted for the same way
    a completed one is instead of being dropped.
  2. handleStreamingResponse wraps the stream controller (withAbortRecording)
    so a disconnect or a mid-stream error records that snapshot through a new
    onStreamAborted: an "aborted" request detail carrying the reason, written
    under the same streamDetailId so the dashboard row is updated rather than
    duplicated, plus the usage row.

The recording fires at most once per stream and a normal completion claims the
slot first, so the flush path and the abort path can never both write. When
onStreamAborted is absent the controller is passed through untouched, so
nothing changes for callers that do not opt in.

Tests

tests/unit/aborted-stream-usage-3488.test.js (new, 3 cases) drives the real
handleStreamingResponse with a fake upstream that stays open, reads one chunk
on the client side and then cancels the reader — which is what DISCONNECT: ResponseAborted is:

  • the abort is recorded once, with the partial content and a non-zero estimated
    completion_tokens;
  • exactly two detail writes (the start placeholder, then the abort update), both
    under the same id, with statuses ["success", "aborted"];
  • a stream that reaches [DONE] normally records no abort.

Mutation check — bypassing only the wrapper (recordingController = streamController)
fails the first two and leaves the third green.

Suite comparison (cd tests && npx vitest run unit/), same machine:

master      86 failed | 1439 passed | 24 skipped
this branch 86 failed | 1442 passed | 24 skipped   (+3 = the new file)

npx eslint clean on all changed files. tests/unit/responses-abort-terminal.test.js
(the other disconnect-path test) still passes.

Note on bug 2 (skipped usageHistory ids)

Not addressed here, and the suggested fix looks to be already in place:
PRAGMA busy_timeout = 5000 is part of PRAGMA_SQL in src/lib/db/schema.js,
which all four adapters (better-sqlite3, bun:sqlite, node:sqlite, sql.js)
exec on open. If the gap ids persist under that timeout, a retry wrapper around
the saveRequestUsage transaction would be the next step — worth its own PR.

`onStreamComplete` runs from the SSE transform's flush, and the flush
only runs on a clean upstream EOF. A client disconnect cancels the
pipeline instead, so the usage row, the request detail and the "done"
line were all skipped — silently, even though the provider had already
generated and billed the partial response. Agentic CLI clients cancel
aggressively, so Recent Requests can look frozen while traffic flows.

`createSSEStream` now exposes `getStreamSnapshot()`, which reads the
content, thinking and usage accumulated so far and resolves usage the
same way the flush paths do — including the estimate-from-content
fallback for an upstream that had not sent usage yet.

`handleStreamingResponse` wraps the stream controller so a disconnect or
a mid-stream upstream error records that snapshot as an "aborted" detail
under the same detail id, plus the usage row. The recording fires at most
once per stream and a normal completion claims the slot, so the flush
path and the abort path can never both write.

Addresses bug 1 of decolua#3488
@ntdat812
ntdat812 force-pushed the fix/aborted-stream-usage-3488 branch from 6bacd8b to bcc2567 Compare September 7, 2026 08:58
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