From bbe8e9ae79a75b89b76335db65294f4fa7f4b7d4 Mon Sep 17 00:00:00 2001 From: Pavan Kumar VH Date: Sun, 6 Sep 2026 01:26:49 +0530 Subject: [PATCH] Fix NaN handling in ttftBucketIndex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- common/src/util/__tests__/ttft-histogram.test.ts | 10 ++++++++++ common/src/util/ttft-histogram.ts | 7 ++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/common/src/util/__tests__/ttft-histogram.test.ts b/common/src/util/__tests__/ttft-histogram.test.ts index e0a3b7f7db..ab02e71e74 100644 --- a/common/src/util/__tests__/ttft-histogram.test.ts +++ b/common/src/util/__tests__/ttft-histogram.test.ts @@ -70,6 +70,16 @@ describe('ttftBucketIndex', () => { expect(ttftBucketIndex(60 * 60 * 1000)).toBeLessThan(last) }) + it('routes NaN to bucket 0, keeps Infinity behavior correct', () => { + // Only NaN should default to 0. Negative Infinity should also go to 0 + // because Math.max(-Infinity, 1) = 1, log(1) = 0, then bucket 0 + expect(ttftBucketIndex(NaN)).toBe(0) + expect(ttftBucketIndex(-Infinity)).toBe(0) + // Positive Infinity flows through Math.max/Math.log and clamps to top + const last = TTFT_HISTOGRAM_BUCKET_COUNT - 1 + expect(ttftBucketIndex(Infinity)).toBe(last) + }) + it('keeps every reported value within half a bucket, plus ms rounding', () => { // The geometric half-width is the real guarantee; the extra 0.5ms is // ttftBucketMs rounding to whole milliseconds, which only matters at diff --git a/common/src/util/ttft-histogram.ts b/common/src/util/ttft-histogram.ts index f6c1820010..61212ea6ef 100644 --- a/common/src/util/ttft-histogram.ts +++ b/common/src/util/ttft-histogram.ts @@ -37,7 +37,12 @@ const LN_BASE = Math.log(TTFT_HISTOGRAM_BASE) * Sub-millisecond and zero samples land in bucket 0 rather than at -Infinity. */ export function ttftBucketIndex(ttftMs: number): number { - const index = Math.floor(Math.log(Math.max(ttftMs, 1)) / LN_BASE) + // Only special-case NaN. Infinity naturally flows through Math.max/Math.log + // and gets clamped to the top bucket by the Math.min below, which is the + // correct behavior. Treating Infinity as 0 would route it to the wrong end + // of the histogram. + const safeTtftMs = Number.isNaN(ttftMs) ? 0 : ttftMs + const index = Math.floor(Math.log(Math.max(safeTtftMs, 1)) / LN_BASE) return Math.min(TTFT_HISTOGRAM_BUCKET_COUNT - 1, Math.max(0, index)) }