Repository navigation
Scope proposal for #442 — output-rate still fails open when request duration is absent #486
Description
Scope proposal for #442. I've got a 14-line fix ready in turn-output-rate.ts with a regression test. Happy to open a PR from a fork, or hand the patch over for internal porting and sync, whichever is easier.
What's still broken after #454
#454 added a burst guard, but it can only fire when request_duration_ms is present:
// packages/tui/src/tui/controller/projection/turn-output-rate.ts:80
if (!nonnegativeFinite(requestMs) || decodeMs >= requestMs * BURST_MAX_DECODE_SHARE)
return false;Only a configured metrics client emits that field. turn.ts:202 gates the callback on deps.metrics, and metricsClient is optional. decode_duration_ms has no such dependency; it just needs includeDetailedUsage. So without a metrics client the guard never fires, and the reported rate counts a buffered burst as decode time.
Reproduction
Two samples that differ only in whether request_duration_ms is present:
rate.beginTurn('turn-1');
// present -> correctly suppressed by #454
rate.apply('turn-1', {
type: 'message',
message: { id: 'a', role: 'assistant',
usage: { outputTokens: 395, decodeDurationMs: 64, requestDurationMs: 4_000 } },
}); // undefined
// absent -> aggregated, and reported as 6172 tok/s
rate.apply('turn-1', {
type: 'message',
message: { id: 'b', role: 'assistant',
usage: { outputTokens: 395, decodeDurationMs: 64 } },
}); // 6171.875One caveat on those numbers. 6172 tok/s and the 395 output tokens are what the TUI actually reported. The 64 ms window is derived from them (395 / 6172), not captured from a log. The sample shape is right; treat the millisecond figure as illustrative.
The change
Check an absolute plausibility bound before the request-duration test, so the guard stops depending on an optional field:
const MAX_PLAUSIBLE_OUTPUT_TOKENS_PER_SECOND = 4_000;
function bufferedBurst(tokens, decodeMs, requestMs) {
if (decodeMs > 0 && tokens / (decodeMs / 1_000) > MAX_PLAUSIBLE_OUTPUT_TOKENS_PER_SECOND)
return true;
if (!nonnegativeFinite(requestMs) || decodeMs >= requestMs * BURST_MAX_DECODE_SHARE)
return false;
return tokens > (decodeMs / 1_000) * BURST_MIN_TOKENS_PER_SECOND;
}Two files, +36 lines including the test. 4000 sits well above the 1900 tok/s sample #454 deliberately preserves. Zero-duration samples keep folding the way they do today; an existing test pins that, so I left it alone.
The 4000 value is my judgement call, not something I pulled from telemetry. If you have real provider numbers for the ceiling, that is the one line worth swapping.
Evidence
12/12 in turn-output-rate.test.ts. Reverting only the source change makes the new case fail with expected 6171.875 to be undefined, so it pins the defect rather than passing vacuously. pnpm typecheck is clean.
On pnpm verify --profile platform, test:capabilities reports 3 failures: tui-keybindings, plus two update-runtime Homebrew-receipt cases. Running those files against unmodified main in the same environment gives the same 3, so they're pre-existing and unrelated. Full gate inventory available on request.
Activity
[-][Bug]: Scope proposal — output-rate fails open when request duration is absent (#442)[/-][+]Scope proposal for #442 — output-rate still fails open when request duration is absent[/+]