Skip to content

fix: stop estimating ChatGPT subscription token speed - #270

Merged
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
xDenside:codex/fix-chatgpt-token-speed
Sep 7, 2026
Merged

fix: stop estimating ChatGPT subscription token speed#270
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
xDenside:codex/fix-chatgpt-token-speed

Conversation

@xDenside

@xDenside xDenside commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • skip the wall-clock TPS fallback for subscription providers when no provider-reported decode timing exists
  • keep the valid output-token count visible when TPS is unavailable
  • add regression coverage for the 1,116 tokens / 3.837 seconds failure mode and the token-only footer
  • record the metric policy in the decision log

Why

The ChatGPT subscription bridge maps Responses API output_tokens into Chat Completions usage. That count can include hidden reasoning generated before the first visible delta, while the frontend fallback timer starts at the first visible text or reasoning-summary delta. Dividing those unlike values produced impossible rates such as 290.9 tok/s and 916.7 tok/s.

The subscription endpoint does not expose reliable decode timing, so omitting TPS is more accurate than manufacturing it. Provider-reported llama.cpp/MLX TPS and the existing fallback for other providers are unchanged.

Verification

  • focused transport and footer tests: 25 passed
  • web TypeScript build: passed
  • affected source and test lint: passed
  • full web suite: all relevant tests passed; one unrelated provider-picker assertion remains failing (DropdownModelProvider.connected.test.tsx expects an empty-key OpenAI provider to be hidden)

Vect0rM commented Sep 6, 2026

Copy link
Copy Markdown
Member

Thanks for this one, @xDenside. "Omitting the metric is more accurate than manufacturing it" is the right call and not the obvious one — the tempting fix is to clamp the number to something plausible, which would have hidden the problem instead of removing it. The root-cause write-up is precise too: output_tokens counting hidden reasoning while the timer starts at the first visible delta explains 290.9 tok/s exactly, and it's a mismatch no amount of arithmetic fixes.

Verified on your branch merged onto current main (conflict resolved locally, see below):

  • tsc -b — exit 0.
  • Full vitest run — 284 files, 2775 passed, 16 skipped. Baseline on main is 2771, so your 4 land clean with nothing else disturbed.

Conflict against main

docs/decisions/INDEX.md, one hunk. #268 landed its own 2026-09-04 ADR line at the same position. Both sides add one bullet; keeping both is the whole resolution. The section header and record-count lines auto-merge but end up numerically behind — main's counts are already drifted, so match whatever main says rather than trying to make them exact. Note #272 and #273 touch the same lines, so whichever of the three lands first leaves the other two with this same trivial conflict.

Two substantive things.

1. Dropping the whole tokenSpeed object also drops durationMs

The metadata block is now emitted only when tokenSpeed > 0. That takes durationMs and tokenCount with it, and chat-telemetry.ts:251 reads decode duration straight out of that object:

decode_duration_ms: num(speed.durationMs),
tps: num(speed.tokenSpeed),

So every ChatGPT subscription turn now reports decode_duration_ms: null. But the wall-clock duration is a perfectly good measurement — it's only the ratio against a token count measuring something else that's meaningless. You're discarding the honest half along with the dishonest half, on exactly the provider whose timing behaviour you'd most want data about.

There's a fix that costs nothing: keep emitting the object with tokenSpeed: 0. I checked that the UI is identical either way — your own displaySpeed > 0 guard already hides the gauge chip, so rendering with { tokenSpeed: 0, tokenCount: 1116, durationMs: 3837 } produces "1116 tokens" and no "tok/sec", the same as omitting it. Same visible result, telemetry preserved, and it keeps the metadata shape stable for anything reading it later.

(The draftTokens* fields ride along in the same object. In practice a turn with draft tokens is a llama.cpp turn that also reports TPS, so they'd survive anyway — mentioning it only so the coupling is on the record.)

2. Why is the boundary chatgpt specifically?

This is the one I'd most like an answer on, because I think the diagnosis is broader than the fix.

tokensPerSecond comes from providerMetadata.tokensPerSecond, which only local engines populate — mlx-vlm's generation_tps, llama.cpp / dflash predicted_per_second. Every cloud provider therefore takes the wall-clock fallback, and isSubscriptionProvider is ['chatgpt'] alone.

Your two premises — the reported output-token count includes reasoning tokens the user never sees, and the timer can only start at the first visible delta — hold verbatim for o-series through a plain OpenAI API key, for Anthropic with extended thinking, and for Gemini thinking models. Those all produce the same inflated rate by the same mechanism; they just aren't the one you happened to be looking at. Meanwhile the ADR's stated justification ("the endpoint does not expose reliable decode timing") is true of essentially every cloud endpoint, not only the subscription bridge.

So either:

  • the boundary should be "no provider-reported decode timing" — drop the wall-clock fallback wherever the provider didn't report a real rate, which is what the ADR's own reasoning implies; or
  • it stays ChatGPT-only, and the ADR says explicitly why — e.g. that for non-reasoning cloud models the wall-clock figure is close enough to be worth keeping, and you'd rather show an approximate number than nothing.

I lean toward the first, but the second is defensible and I'd take it. What I don't want is the narrow gate landing without the reasoning recorded, because the next person to hit 900 tok/s on an o-series key will have to redo this whole investigation.

3. Minor: the streaming branch you touched is dead code

shouldShow = streaming || displaySpeed > 0 || displayTokenCount > 0 — the streaming term never fires. MessageItem.tsx:610 is the only call site and passes streaming={false}, and nothing in the app calls setTokenSpeed or updateTokenSpeed, so useAppState.tokenSpeed is always undefined and both streamingTokenSpeed and streamingTokenCount are always 0. The exported StreamingTokenSpeedCallback type has no consumer either.

Nothing to fix here — your change is correct regardless, and the displaySpeed > 0 || displayTokenCount > 0 half is what does the work. Flagging it so you don't rely on the streaming path in a follow-up, and because the dead store actions are worth a separate cleanup issue.

Nit

No CI has run on this branch, so my local run is the only signal.

Answer 2, take the one-line change in 1, and I'll merge this 📊


Generated by Claude Code

# Conflicts:
#	docs/decisions/INDEX.md
@Vect0rM

Vect0rM commented Sep 7, 2026

Copy link
Copy Markdown
Member

Synced this branch with main (merge commit, no rebase, so your commits stay intact).

The only conflict was docs/decisions/INDEX.md: main had meanwhile added Bound streaming reasoning render cost (#268) to the same UI / UX section. Resolved by keeping both entries and bumping the counters to 233 records / ## UI / UX (31). No source changes were needed — custom-chat-transport.ts and the harness test auto-merged.

Re-verified on the merged tree:

  • vitest --run — 256 files, 2644 passed, 11 skipped, 0 failed. The DropdownModelProvider.connected.test.tsx assertion you flagged as failing has since been fixed on main, so the suite is fully green now.
  • tsc -b — no errors.
  • eslint on the changed files — clean.

One behaviour note for the record: when resolveTokenSpeed returns 0 the whole tokenSpeed metadata object is omitted, not just the rate. That is fine for every consumer — TokenSpeedIndicator and ContextSizeControl both already read the count from usage.outputTokens first, and chat-telemetry guards with meta.tokenSpeed ?? {} — so the token count stays visible exactly as the PR describes.

Ready to merge.

@Vect0rM
Vect0rM merged commit 3b058f6 into AtomicBot-ai:main Sep 7, 2026
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.

2 participants