Skip to content

fix: persist precomputed t/s so reloaded sessions match live rate - #47

Merged
Blankeos merged 2 commits into
mainfrom
fix/tps-reload-parity
Sep 3, 2026
Merged

fix: persist precomputed t/s so reloaded sessions match live rate#47
Blankeos merged 2 commits into
mainfrom
fix/tps-reload-parity

Conversation

@Blankeos

@Blankeos Blankeos commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Live header showed ~145t/s but reopening the same session showed ~3292t/s.

Root cause: tokens_per_sec was computed live from generation samples but never persisted, and the reload fallback divided token_count (billed total incl. prompt + cache) by decode time instead of output tokens.

Changes:

  • Add messages.tokens_per_sec (migration v4), round-tripped through history DAO + session conversions (plus 2 regression tests).
  • Backfill output_tokens from billed usage.output buckets for pre-existing rows; never derive from tokens_used total.
  • Render (chat.rs:6633) + notification (app.rs:1732) paths: t/s fallback uses output tokens only; total-only legacy rows show duration without t/s.

Note: test_edit_tool_renders_codex_style_diff_summary fails in worktrees (hardcodes main checkout path) — pre-existing, unrelated.

Live header showed 145t/s but reopening the same session showed
3292t/s. Root cause: tokens_per_sec was never persisted, and the
reload fallback divided token_count (billed total incl. prompt +
cache) by decode time instead of output tokens.

- Add messages.tokens_per_sec (migration v4), round-trip through
  history DAO + session conversions
- Backfill output_tokens from billed usage.output buckets for
  pre-existing rows; never derive from tokens_used total
- Render + notification paths: t/s fallback uses output_tokens
  only; total-only legacy rows show duration without t/s
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 3, 2026

Copy link
Copy Markdown

Deploying crabcode with  Cloudflare Pages  Cloudflare Pages

Latest commit: eb0a66f
Status: ✅  Deploy successful!
Preview URL: https://f94becfb.crabcode.pages.dev
Branch Preview URL: https://fix-tps-reload-parity.crabcode.pages.dev

View logs

… decode)

Keep our pause-adjusted per-sample rate for the live provisional, but
compute the finalized/persisted rate like opencode #46108
(packages/core/src/session/tokens.ts): round(billed_output /
((completed - firstToken) / 1000)), 250ms floor, no (n-1) inter-token
adjustment, tool-call steps included via accumulated billed usage.

Provider output already counts reasoning tokens, so no separate
bucket is needed. Reload fallback in message_tokens_per_sec and the
notification path use the same formula, so live, reloaded, and
notified rates agree.
@Blankeos

Blankeos commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

PR Review #47 — fix/tps-reload-parity -> main

Branch: fix/tps-reload-parity -> main
Merge confidence: 4/5
PR: #47 — "fix: persist precomputed t/s so reloaded sessions match live rate" (MERGEABLE, CLEAN)

PR Comment Body (paste to GitHub)

Fixes live (~145t/s) vs reload (~3292t/s) mismatch by persisting precomputed tokens_per_sec (migration v4) and restricting all t/s fallbacks to output tokens only. Finalized rate now matches opencode upstream #46108 (billed output / decode, 250ms floor). Legacy total-only rows show duration without t/s. Targeted tests + fmt --check + cargo check pass locally.

What Changed (5 files, +216/-39)

  • src/persistence/migrations.rs:189 — v4: ALTER TABLE messages ADD COLUMN tokens_per_sec REAL (idempotent via let _ =), fresh v1 schema also includes it.
  • src/persistence/history.rs:65,442,493,561,584Message.tokens_per_sec, round-tripped on both INSERT paths; SELECT uses row.get(14).unwrap_or(None) so v3 DBs still read.
  • src/persistence/conversions.rs:113,189,242 — persist tokens_per_sec (finite+>0 filter); backfill output_tokens from usage.output buckets, never from tokens_used total. +2 tests.
  • src/ui/components/chat.rs:458,485,2660,6648,6682 — new upstream_tokens_per_sec(); finalize_streaming_metrics prefers billed recorded_usage().output over sample estimate; render uses output-only; total-only legacy shows duration, no t/s. Test expectations 49→50, 99→100 + new test_finalize_prefers_billed_output_over_sample_estimate.
  • src/app.rs:1711,1731,1747 — notification path: same upstream formula, 250ms floor, output-only, total-only legacy guard.

Regressions?

No blocking regressions found. Expected behavior shifts (intentional, not bugs):

  • All t/s numbers shift slightly up: n/duration vs old (n-1)/duration (e.g. 49→50, 99→100).
  • Fast responses (<250ms decode) now show no t/s where old code showed one if tokens≥2. Matches upstream floor.
  • Reloaded sessions with only token_count+duration (no output tokens) lose their t/s display (duration-only). This is the fix for the 3292t/s inflation.

No dead-code issue: MIN_TPS_SAMPLE_TOKENS (chat.rs:409) still used by GenerationSample (chat.rs:446); new path correctly uses the 250ms floor instead.

Migrations?

Yes — v4. Safe:

  • ADD COLUMN is nullable, idempotent, no backfill needed (fallback recomputes from billed buckets).
  • Old binaries reading new DB: unknown column ignored (explicit column lists). New binary reading old DB: ALTER runs on open; SELECT tolerates missing col.
  • No data loss, no downgrade path needed.

Pre-Merge Checklist

  • Run full cargo test (only 3 targeted tests run here; PR notes test_edit_tool_renders_codex_style_diff_summary fails in worktrees — confirm pre-existing on main)
  • Manual: stream a response, note live t/s → /new → reopen session → confirm same t/s
  • Manual: open a pre-v4 session with large prompt + small output → confirm no inflated t/s, duration-only or correct rate
  • Confirm notification (app.rs) rate matches chat header on completion

Confidence Bumps

4/5 → 5/5 if: full cargo test green (minus known worktree failure) + one manual live-vs-reload screenshot/numbers posted on the PR. No code changes needed — this is verification-only.

Checks Run (read-only)

  • gh pr view/checks: OPEN, MERGEABLE, CLEAN; CI plan pass (others skipped by design)
  • cargo fmt --check: clean (exit 0)
  • cargo check: zero warnings/errors
  • cargo test -- precomputed_tps billed_output finalize_prefers_billed: 3 passed

@Blankeos
Blankeos merged commit 5b63536 into main Sep 3, 2026
7 checks passed
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