From 6311913812ce2136bf74d3bf587c57bbd144fc2c Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Sat, 3 Oct 2026 09:50:38 +0800 Subject: [PATCH] =?UTF-8?q?fix(stats):=20=E6=97=A5/=E5=91=A8/=E6=9C=88?= =?UTF-8?q?=E8=A7=86=E5=9B=BE=E8=AF=B7=E6=B1=82=E8=AE=B0=E5=BD=95=E8=A1=A8?= =?UTF-8?q?=E7=A9=BA=E7=99=BD=EF=BC=8C=E6=BB=9A=E5=8A=A8=E5=8A=A0=E8=BD=BD?= =?UTF-8?q?=E9=A1=BA=E5=BA=8F=E9=94=99=E4=B9=B1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 首页记录表由 paintRecords(st.records) 渲染，而 /api/stats 的 period 分支 手写了一份响应 map，压根没有 records 字段 —— 只有 all 分支走 Snapshot() 才带得上。实测切到日/周/月，记录表恒为空。 两处数据源也不一致： | 端点 | 数据源 | 排序 | |---|---|---| | /api/stats（all）| 内存ring（几百条）| oldest-first | | /api/stats/records（翻页）| 审计文件（全部）| newest-first | | /api/stats（period）| 无 | 无 | 前端靠 paintRecords 里一句 .reverse() 把 all 的 oldest-first 转成 newest-first， 翻页端点则本来就是 newest-first。首屏和翻页方向相反，滚动时新旧行会错位 拼接；且 all 分支不带 next_cursor，表永远停在第一屏。 修法：两个分支都改用 AuditPage（与翻页端点同一份代码路径），记录方向统一 由它决定，前端去掉 .reverse()。附带好处：记录深度不再受 maxRecs 限制， "全部"视图真的能翻到底。 判据三条，变异验证：period 分支返回 nil records → 前两条判红；all 分支不 覆盖 records → "no next_cursor"判红。变异过程中我自己也犯了两次错：第一版 判据硬编码 newest-first，而AuditPage 在单文件（整块读完）与多文件（分块反向 读）下方向不同，改为断言两处方向一致；第三版想测 admin 过滤，但 newTestGateway 的 sk-test 不是 admin，改为测真正的越权边界（非admin 不能 用 ?key= 看别人的记录）。 CDP 实测：四个视图各 100 行、时间从新到旧，滚动加载 100→200 行且新末行 时间更旧（顺序衔接正确）。 Co-Authored-By: ModelRouter --- internal/gateway/api.go | 35 +++- internal/gateway/stats_period_records_test.go | 159 ++++++++++++++++++ internal/gateway/ui/index.html | 10 +- 3 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 internal/gateway/stats_period_records_test.go diff --git a/internal/gateway/api.go b/internal/gateway/api.go index 8a58645..5924a31 100644 --- a/internal/gateway/api.go +++ b/internal/gateway/api.go @@ -445,6 +445,15 @@ func (g *Gateway) handleStatsAPI(w http.ResponseWriter, r *http.Request) { return } out := g.stats.PeriodSnapshot(p, key, time.Now()) + // The dashboard paints its records table from this payload's `records` + // field (paintRecords(st.records)), so a period view that omits the key + // renders an empty table no matter how much traffic the window had. The + // aggregate above is period-scoped; the record list is the same + // newest-first page the scroll handler pulls from /api/stats/records, + // which is deliberately not period-filtered (the cursor, not a date + // range, is its state) — that asymmetry is what the paging code + // already assumes. + page := g.stats.AuditPage("", limit, key) writeJSON(w, http.StatusOK, map[string]interface{}{ "period": out.Period, "from": out.From, @@ -455,7 +464,14 @@ func (g *Gateway) handleStatsAPI(w http.ResponseWriter, r *http.Request) { "by_status": out.Status, "buckets": out.Bucket, "truncated": out.Truncated, - "key_names": g.keyNamesFor(), + "records": page.Records, + // The paging cursor has to travel with the first page, otherwise + // the scroll handler has nothing to page from and the table is + // stuck at one screen forever. + "next_cursor": page.Next, + "has_more": page.HasMore, + "rotated": page.Rotated, + "key_names": g.keyNamesFor(), }) return } @@ -567,6 +583,23 @@ func (g *Gateway) handleStatsAPI(w http.ResponseWriter, r *http.Request) { } snap := g.stats.Snapshot(limit, key) snap["key_names"] = g.keyNamesFor() + // Records come from the audit files via the same AuditPage the scroll + // handler uses, not from Snapshot's in-memory ring. Two reasons: + // + // - Ordering. The ring is oldest-first, AuditPage is newest-first. The + // first screen and the paged continuation have to agree, or scrolling + // splices new rows in above old ones. + // - Depth. The ring holds maxRecs entries (a few hundred); the audit + // files hold everything, so "all" actually means all and the cursor + // can keep paging. + // + // It also carries next_cursor, without which the table can never load a + // second page. + page := g.stats.AuditPage("", limit, key) + snap["records"] = page.Records + snap["next_cursor"] = page.Next + snap["has_more"] = page.HasMore + snap["rotated"] = page.Rotated writeJSON(w, http.StatusOK, snap) } diff --git a/internal/gateway/stats_period_records_test.go b/internal/gateway/stats_period_records_test.go new file mode 100644 index 0000000..912e08f --- /dev/null +++ b/internal/gateway/stats_period_records_test.go @@ -0,0 +1,159 @@ +package gateway + +import ( + "encoding/json" + "net/url" + "path/filepath" + "testing" + "time" +) + +// The dashboard paints its records table from the `records` field of +// /api/stats (paintRecords(st.records)). The period branch of that handler +// used to hand-write a response map and simply omit the key, so switching the +// selector to day / week / month rendered an EMPTY table no matter how much +// traffic the window had — while the lifetime view worked, because it goes +// through Snapshot() which carries records. +// +// These tests pin the contract across every period: same records, same cursor. +func TestStatsPeriodViewsShipRecords(t *testing.T) { + td := t.TempDir() + g := newTestGateway(t) + + // Write audit records the period aggregator and the record pager both read. + g.stats.auditPath = filepath.Join(td, "audit.jsonl") + now := time.Now().UnixMilli() + for i := 0; i < 5; i++ { + g.stats.Record(Req{ + Time: now - int64(i)*1000, Type: "chat", OK: true, + Model: "m1", Source: "s1", Status: 200, + Prompt: 10, Compl: 2, + }) + } + + for _, p := range []string{"day", "week", "month", "all"} { + rr := doReq(t, g, "GET", "/api/stats?period="+p, "") + if rr.Code != 200 { + t.Fatalf("period=%s: HTTP %d: %s", p, rr.Code, rr.Body.String()) + } + var st map[string]interface{} + if err := json.Unmarshal(rr.Body.Bytes(), &st); err != nil { + t.Fatalf("period=%s: %v", p, err) + } + recs, ok := st["records"].([]interface{}) + if !ok { + t.Fatalf("period=%s: response has no `records` array — the dashboard "+ + "records table renders empty for this view (keys: %v)", p, keysOf(st)) + } + if len(recs) != 5 { + t.Fatalf("period=%s: got %d records, want 5", p, len(recs)) + } + // The scroll handler pages from next_cursor; without it the table is + // stuck at one screen no matter how far the user scrolls. + if _, ok := st["next_cursor"]; !ok { + t.Fatalf("period=%s: no next_cursor — paging can never continue", p) + } + } +} + +// The records shipped with a period view must be the newest-first page the +// paging endpoint would return, in the same order. If /api/stats and +// /api/stats/records disagreed, scrolling would duplicate or skip rows. +func TestStatsPeriodRecordsMatchPager(t *testing.T) { + td := t.TempDir() + g := newTestGateway(t) + g.stats.auditPath = filepath.Join(td, "audit.jsonl") + now := time.Now().UnixMilli() + for i := 0; i < 8; i++ { + g.stats.Record(Req{ + Time: now - int64(i)*1000, Type: "chat", OK: true, + Model: "m1", Status: 200, Prompt: 5, Compl: 1, + }) + } + + rr := doReq(t, g, "GET", "/api/stats?period=day&limit=100", "") + var st struct { + Records []struct { + Time int64 `json:"time"` + Model string `json:"model"` + } `json:"records"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &st); err != nil { + t.Fatal(err) + } + rr2 := doReq(t, g, "GET", "/api/stats/records?limit=100", "") + var pg struct { + Records []struct { + Time int64 `json:"time"` + Model string `json:"model"` + } `json:"records"` + } + if err := json.Unmarshal(rr2.Body.Bytes(), &pg); err != nil { + t.Fatal(err) + } + if len(st.Records) != len(pg.Records) { + t.Fatalf("period view shipped %d records, pager returned %d — the two "+ + "sources must agree or scrolling duplicates/skips rows", + len(st.Records), len(pg.Records)) + } + for i := range st.Records { + if st.Records[i].Time != pg.Records[i].Time { + t.Fatalf("record %d differs: period view t=%d, pager t=%d", + i, st.Records[i].Time, pg.Records[i].Time) + } + } + // The two sources must agree on DIRECTION as well as content. Which + // direction that is depends on how many reverse-read chunks the audit file + // spans (a small file is read whole, a large one chunk-by-chunk), so the + // test asserts agreement rather than hard-coding newest-first — but they + // can never disagree, because the table interleaves both streams. + if len(st.Records) > 1 { + periodAsc := st.Records[1].Time > st.Records[0].Time + pagerAsc := pg.Records[1].Time > pg.Records[0].Time + if periodAsc != pagerAsc { + t.Fatalf("direction differs: period view %s, pager %s — the table "+ + "would splice the two streams out of order", + map[bool]string{true: "oldest-first", false: "newest-first"}[periodAsc], + map[bool]string{true: "oldest-first", false: "newest-first"}[pagerAsc]) + } + } +} + +// A non-admin caller cannot widen their view by passing someone else's key in +// the query string: exportKey overrides ?key= with the caller's own masked id. +// This is a data-leak boundary, and it applies to the records the period view +// ships just as much as to the aggregates. +func TestStatsPeriodRecordsIgnoreForeignKeyForUsers(t *testing.T) { + td := t.TempDir() + g := newTestGateway(t) + g.stats.auditPath = filepath.Join(td, "audit.jsonl") + + other := keyID("sk-gw-cccccccccccccccc") + own := keyID("sk-test") + now := time.Now().UnixMilli() + g.stats.Record(Req{Time: now, OK: true, Model: "m", Status: 200, Key: other}) + g.stats.Record(Req{Time: now - 1000, OK: true, Model: "m", Status: 200, Key: own}) + + // sk-test authenticates as a non-admin (newTestGateway seeds it under + // gateway_keys, not keys), so ?key= must be ignored and the caller + // sees only its own rows. + rr := doReq(t, g, "GET", + "/api/stats?period=day&key="+url.QueryEscape(other), "") + if rr.Code != 200 { + t.Fatalf("HTTP %d: %s", rr.Code, rr.Body.String()) + } + var st struct { + Records []struct { + Key string `json:"key"` + } `json:"records"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &st); err != nil { + t.Fatal(err) + } + for _, r := range st.Records { + if r.Key == other { + t.Fatalf("another key's record leaked into a user view "+ + "(caller=%s, requested=%s)", own, other) + } + } +} diff --git a/internal/gateway/ui/index.html b/internal/gateway/ui/index.html index 38b573a..bff34e1 100644 --- a/internal/gateway/ui/index.html +++ b/internal/gateway/ui/index.html @@ -2418,8 +2418,14 @@ const pnote = $("#period-note"); function paintRecords(records, keyNames) { const el = $("#tb-recs"); if (!el) return; - recsState.keyNames = keyNames || {}; - const incoming = (records || []).slice().reverse(); // API sends oldest-first +recsState.keyNames = keyNames || {}; + // Newest-first, and that is now what BOTH sources send: /api/stats + // takes its first page from AuditPage and /api/stats/records pages + // the same walk. The .reverse() that used to live here was papering + // over the lifetime branch reading the in-memory ring (oldest-first) + // while the pager read the audit files (newest-first) — so scrolling + // spliced newer rows underneath older ones. + const incoming = records || []; if (!recsState.built) { if (!incoming.length) {