Skip to content

fix(usage): keep the client context buffer out of the recorded turn - #3913

Open
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/estimated-usage-buffer-leak
Open

ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/estimated-usage-buffer-leak

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Found while looking at #3890. The behaviour reported there is deliberate — but the investigation turned up a related defect that is not, and this PR fixes that one.

What #3890 reports is by design

BUFFER_TOKENS (2000) is a margin for the client's context arithmetic: inflating the input_tokens a client sees makes it compact slightly early rather than slightly late. stream.js says so in as many words on the non-estimated branch:

// Add buffer and filter usage for client (but keep original in state.usage for logging)

So a client seeing 2737 where the provider said 737 + 20992 cached is the margin working, not a translation bug. I did not change that.

What is actually broken

The estimated path does not keep that split.

formatUsage() wrapped both shapes in addBufferToUsage(), so estimateUsage() returned an already-buffered object — and all three call sites in stream.js assigned that same object to the variable that is later recorded:

site assigns to ends up in
finalizeStream() usage / state.usage logUsage(), onStreamComplete()
OpenAI passthrough finish chunk usage same
translate finish chunk state.usage same

So whenever a provider omitted usage and 9Router had to estimate it, the turn was recorded 2000 input tokens larger than it was — silently, and in the direction that inflates a quota. The line right below the third site is the one that states the invariant being broken.

The change

formatUsage / estimateUsage take { buffer } and still buffer by default, so open-sse/executors/cursor.js — which puts the estimate straight onto a response body — is untouched. The three stream.js sites ask for the raw estimate for recording and add the buffer only to the copy the client receives, which is exactly what the non-estimated branch beside them already does.

Evidence

tests/unit/estimated-usage-buffer-leak.test.js, 4 tests. The last one drives a real passthrough stream whose finish chunk carries no usage, then asserts the client copy and the recorded copy differ by exactly BUFFER_TOKENS, with the smaller one being what we record.

Two-way mutation, run locally:

Mutation Result
none 4 / 4 pass
restore the buffered estimateUsage at the passthrough site 1 fail — expected +0 to be 2000; the three option-level tests stay green

That second row is the point: the three cheap tests pass either way, so the last one is the one carrying the proof.

Not covered

The non-streaming handler estimates nothing, so it is unaffected. I did not touch cursor.js, which still emits a buffered estimate to its client by design — if you would rather its recorded usage were raw too, that is a separate and slightly larger change.

BUFFER_TOKENS is a margin for the client's own context arithmetic. On the
normal path stream.js says so and keeps the recorded value raw: "Add buffer
and filter usage for client (but keep original in state.usage for logging)".

The estimated path broke that. formatUsage() wrapped both shapes in
addBufferToUsage(), so estimateUsage() returned an already-buffered object,
and all three call sites in stream.js assigned that same object to the
variable that is later logged and handed to onStreamComplete. Whenever a
provider omitted usage, the turn was recorded 2000 input tokens larger than
it was -- silently, and in the direction that inflates a quota.

formatUsage/estimateUsage now take { buffer } and still buffer by default, so
open-sse/executors/cursor.js, which puts the estimate straight on a response
body, is unchanged. The three stream.js sites ask for the raw estimate for
recording and add the buffer only to the copy the client receives -- the same
split the non-estimated branch beside them already makes.
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