diff --git a/internal/admin/keyviz_handler.go b/internal/admin/keyviz_handler.go index 222c8fa25..bf8c34934 100644 --- a/internal/admin/keyviz_handler.go +++ b/internal/admin/keyviz_handler.go @@ -88,7 +88,9 @@ type KeyVizRow struct { // from_unix_ms - lower bound in unix ms; 0 or omitted means unbounded // on that side (NOT the Unix epoch) // to_unix_ms - upper bound in unix ms; same 0 = unbounded contract -// rows - row budget; 0 means no cap, capped at 1024 (default: 0) +// rows - row budget; default and maximum is 1024 (design §4.1). +// Omitted / 0 / negative all yield the cap; explicit +// values above the cap are silently clamped down. // // Returns 503 codes.Unavailable when no sampler is configured so the // SPA can distinguish "keyviz disabled" from "no data yet" (the @@ -166,7 +168,12 @@ type keyVizParams struct { } func parseKeyVizParams(r *http.Request) (keyVizParams, error) { - p := keyVizParams{series: keyVizDefaultSeries} + // rows defaults to the cap so a normal SPA poll without the + // query parameter still respects the budget — leaving it at the + // zero value would let applyKeyVizRowBudget fall through to "no + // cap" and return every tracked route in one payload (Codex + // round-3 P1 on PR #660). + p := keyVizParams{series: keyVizDefaultSeries, rows: keyVizRowBudgetCap} q := r.URL.Query() if err := setKeyVizSeriesParam(&p, q.Get("series")); err != nil { return keyVizParams{}, err @@ -209,13 +216,17 @@ func setKeyVizTimeParam(dst *time.Time, name, raw string) error { func setKeyVizRowsParam(dst *int, raw string) error { if raw == "" { + // Caller pre-set dst to the default cap; preserve it. return nil } n, err := strconv.Atoi(raw) - if err != nil || n < 0 { - return errors.New("rows must be a non-negative integer") + if err != nil { + return errors.New("rows must be an integer") } - if n > keyVizRowBudgetCap { + if n <= 0 || n > keyVizRowBudgetCap { + // Explicit 0 / negative / above-cap all collapse to the cap + // (same as omitting the param) so callers can't disable the + // budget by passing pathological values. n = keyVizRowBudgetCap } *dst = n diff --git a/internal/admin/keyviz_handler_test.go b/internal/admin/keyviz_handler_test.go index 319461543..ad947365f 100644 --- a/internal/admin/keyviz_handler_test.go +++ b/internal/admin/keyviz_handler_test.go @@ -291,6 +291,69 @@ func TestKeyVizHandlerRowsBudgetTieBreakDeterministic(t *testing.T) { } } +// TestKeyVizHandlerOmittedRowsAppliesDefaultCap pins Codex round-3 P1 +// on PR #660: when the SPA polls without ?rows=, the handler must +// still apply the keyVizRowBudgetCap default — leaving p.rows at +// zero would let applyKeyVizRowBudget fall through to "no cap" and +// return every tracked route in one payload. +func TestKeyVizHandlerOmittedRowsAppliesDefaultCap(t *testing.T) { + t.Parallel() + srv := newKeyVizTestServer(t, &fakeKeyVizSource{cols: []keyviz.MatrixColumn{ + {At: time.Unix(1_700_000_000, 0), Rows: stagedRowsForBudgetTest()}, + }}) + defer srv.Close() + + for _, query := range []string{"", "?rows=0", "?rows=-1"} { + resp := keyVizGet(t, srv.URL+query) + require.Equal(t, http.StatusOK, resp.StatusCode) + var matrix KeyVizMatrix + require.NoError(t, json.NewDecoder(resp.Body).Decode(&matrix)) + require.NoError(t, resp.Body.Close()) + require.Len(t, matrix.Rows, keyVizRowBudgetCap, + "omitted/0/negative rows must apply the default cap (query=%q)", query) + } +} + +// TestKeyVizHandlerClampsRowsBudgetToCap pins the above-cap branch of +// setKeyVizRowsParam: an explicit rows= value greater than +// keyVizRowBudgetCap must be silently clamped down so callers cannot +// bypass the resource guard by asking for more rows than the cap. +func TestKeyVizHandlerClampsRowsBudgetToCap(t *testing.T) { + t.Parallel() + srv := newKeyVizTestServer(t, &fakeKeyVizSource{cols: []keyviz.MatrixColumn{ + {At: time.Unix(1_700_000_000, 0), Rows: stagedRowsForBudgetTest()}, + }}) + defer srv.Close() + + resp := keyVizGet(t, srv.URL+"?rows=9999") + require.Equal(t, http.StatusOK, resp.StatusCode) + var matrix KeyVizMatrix + require.NoError(t, json.NewDecoder(resp.Body).Decode(&matrix)) + require.NoError(t, resp.Body.Close()) + require.Len(t, matrix.Rows, keyVizRowBudgetCap, + "rows=9999 must clamp down to keyVizRowBudgetCap") +} + +// stagedRowsForBudgetTest builds keyVizRowBudgetCap+5 distinct rows so +// any test that exercises the budget cap can confirm truncation +// occurred. The loop counter is uint64 to avoid an int→uint64 +// conversion that would need a gosec suppression; Start / End encode +// the index as a 2-byte big-endian key. +func stagedRowsForBudgetTest() []keyviz.MatrixRow { + const total uint64 = keyVizRowBudgetCap + 5 + rows := make([]keyviz.MatrixRow, total) + for i := uint64(0); i < total; i++ { + n := i + 1 + rows[i] = keyviz.MatrixRow{ + RouteID: n, + Start: []byte{byte(i >> 8), byte(i)}, + End: []byte{byte(n >> 8), byte(n)}, + Writes: n, + } + } + return rows +} + // TestKeyVizHandlerTimeBoundsParam exercises the from_unix_ms / // to_unix_ms query parameters: a non-zero pair filters columns to the // requested half-open window, while 0 means "unbounded on that side"