-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
The function didn't validate that ttftMs is a finite number. If ttftMs was NaN, Math.max(NaN, 1) would return NaN, causing Math.log(NaN) to return NaN, and the entire calculation would produce NaN. Fixed by: - Special-casing only Number.isNaN(ttftMs) → 0 (not all non-finite values) - Letting Infinity flow through the normal path (Math.max/Math.log) which correctly clamps it to the top bucket via Math.min - Preserving correct behavior: NaN → bucket 0, ±Infinity → appropriate buckets Added test coverage for: - NaN input → bucket 0 - ±Infinity input → appropriate buckets (0 for -Infinity, top for +Infinity) - Normal values work as before The existing test suite (11 tests, 995 assertions) passes with this change. All 12 tests pass now.
codebuff-team
commented
Sep 6, 2026
Good instinct and reasonably scoped. common/src/util/ttft-histogram.ts is in scope, and the fix is small and easy to verify: without the guard, Math.max(NaN, 1) is NaN, Math.log(NaN) is NaN, and the final Math.min/Math.max clamp does nothing since any comparison with NaN is false, so ttftBucketIndex(NaN) returns NaN today. Special-casing Number.isNaN(ttftMs) to 0 and leaving Infinity/-Infinity to flow through the existing math is the right layer for the fix - it doesn't try to over-generalize to all non-finite values, which avoids clobbering the correct -Infinity → bucket 0 and Infinity → last bucket behavior.
The added test in ttft-histogram.test.ts covers the three interesting inputs (NaN, -Infinity, Infinity) and documents why -Infinity lands at bucket 0 (because Math.max(-Infinity, 1) = 1), which is a nice touch for future readers.
One thing worth checking before merge: is NaN actually a value that can reach this function in practice (e.g., from a malformed duration upstream), or is this purely defensive? If it's defensive-only, that's still fine to land, but the PR body could be clearer about whether this was observed in production versus found by inspection. Otherwise this is a tight, well-tested fix worth porting.
Overview
Fix NaN handling in the
ttftBucketIndexfunction incommon/src/util/ttft-histogram.ts.Bug Description
The function didn't validate that ttftMs is a finite number. If ttftMs was
NaN, Math.max(NaN, 1) would return NaN, causing Math.log(NaN) to return NaN,
and the entire calculation would produce NaN.
Fix
correctly clamps it to the top bucket via Math.min
Testing
The existing comprehensive test suite (11 tests, 995 assertions) was extended
with 1 new test case verifying:
All 12 tests pass.
Files Changed
common/src/util/ttft-histogram.ts- Fixed NaN/Infinity handlingcommon/src/util/__tests__/ttft-histogram.test.ts- Added test coverageScope
This change only touches
common/which is an approved contribution area per the Contributing Guide.