mirror of
https://gitcode.com/JianFeeeee/ModelRouter.git
synced 2026-11-07 09:47:23 +00:00
fix(gateway): ?key= 传完整 key 会让 /api/stats panic
给 usage 插件探路时撞出来的,与插件无关。
Snapshot 的统计 map 以**掩码** key id 为键(keyID(req.Key))——by_key 返回什么、
面板就传什么。而 exportKey 对 admin 允许 ?key= 任意值,于是:
byKey = map[string]*Stat{key: s.byKey[key]} // 传完整 key → 零值 nil *Stat
...
rows() 里 Stat: *v // 解引用 nil → panic
实测(改动前,线上):
/api/stats?period=day&limit=1&key=sk-gw-7ffe12c5cbacec40bd39a7291df9a9ed
→ curl: (52) Empty reply from server
→ 日志: http: panic serving …: invalid memory address or nil pointer dereference
llmsproxy/internal/gateway.rows stats.go:771
net/http 会 recover 单个连接所以进程不死,但客户端拿到空响应,日志里每次一条
panic 栈。任何「这个 key 用了多少」的问法(key 已配但当天无流量、period 把它
的历史滤掉了)都会命中 —— 而零正是那个问题的答案,不是错误。
三处防御,因为同一个错误形状有四个入口:
- rows() 跳过 nil 而不是解引用(调用方不止 Snapshot)
- byStatus 的两处遍历同理
- PeriodSnapshot 的 buckets[k] 同理
by_key 仍保留该 key 的一行零值:面板按行存在与否渲染,缺行会让 key 整个消失。
判据:internal/gateway/stats_unknown_key_test.go(5 个测试)。反向验证过——
把 Snapshot 与 rows 的修复撤掉,TestSnapshotUnknownKeyDoesNotPanic 立刻 panic。
(cherry picked from commit af977efbf7)
This commit is contained in:
@ -765,9 +765,20 @@ func (s *Stats) WindowTokens(model, source string, sec int64) int64 {
|
||||
return total
|
||||
}
|
||||
|
||||
// rows flattens a map[string]*Stat into a reportable, descending-by-requests
|
||||
// list.
|
||||
//
|
||||
// A nil value is skipped rather than dereferenced. The maps this is called with
|
||||
// are built by several callers (Snapshot, PeriodSnapshot, the audit replay) and a
|
||||
// nil entry is a legitimate representation of "this dimension has no data" —
|
||||
// `map[string]*Stat{key: s.byKey[key]}` produced exactly that whenever the
|
||||
// queried key had no recorded traffic, and `*v` then panicked the handler.
|
||||
func rows(m map[string]*Stat) []StatsRow {
|
||||
out := make([]StatsRow, 0, len(m))
|
||||
for k, v := range m {
|
||||
if v == nil {
|
||||
continue
|
||||
}
|
||||
out = append(out, StatsRow{Name: k, Stat: *v})
|
||||
}
|
||||
for i := 1; i < len(out); i++ {
|
||||
@ -1185,7 +1196,26 @@ func (s *Stats) Snapshot(limit int, key string) map[string]interface{} {
|
||||
if key == "" {
|
||||
byKey, byModel, bySrc = s.byKey, s.byModel, s.bySrc
|
||||
} else {
|
||||
byKey = map[string]*Stat{key: s.byKey[key]}
|
||||
// The map is keyed by the MASKED key id (keyID(req.Key)), which is what
|
||||
// by_key reports and what the dashboard sends as ?key=. An admin may
|
||||
// pass anything in that query parameter, including the full key, and
|
||||
// `s.byKey[key]` then returns the zero value — a nil *Stat.
|
||||
//
|
||||
// rows() below does `*v` on every value, so a nil entry panicked the
|
||||
// handler: any ?key=<full key> request killed the response with an empty
|
||||
// reply, and logged a full panic stack. Reproduced against the live
|
||||
// gateway: /api/stats?period=day&limit=1&key=sk-gw-... → curl exit 52.
|
||||
//
|
||||
// A key with no recorded traffic is a legitimate question ("what did this
|
||||
// key use?"), and its answer is zero — so an absent entry is an empty
|
||||
// result, not an error, and not a crash.
|
||||
st := s.byKey[key]
|
||||
byKey = make(map[string]*Stat, 1)
|
||||
if st != nil {
|
||||
byKey[key] = st
|
||||
} else {
|
||||
byKey[key] = &Stat{}
|
||||
}
|
||||
byModel = s.byKeyModel[key]
|
||||
bySrc = s.byKeySrc[key]
|
||||
}
|
||||
@ -1202,8 +1232,11 @@ func (s *Stats) Snapshot(limit int, key string) map[string]interface{} {
|
||||
}
|
||||
}
|
||||
bs := make([]agrRow, 0, len(s.byStatus))
|
||||
for code := range s.byStatus {
|
||||
bs = append(bs, agrRow{Name: strconv.Itoa(code), Stat: *s.byStatus[code]})
|
||||
for code, st := range s.byStatus {
|
||||
if st == nil {
|
||||
continue
|
||||
}
|
||||
bs = append(bs, agrRow{Name: strconv.Itoa(code), Stat: *st})
|
||||
}
|
||||
sort.Slice(bs, func(i, j int) bool {
|
||||
ci, _ := strconv.Atoi(bs[i].Name)
|
||||
|
||||
@ -174,6 +174,9 @@ func (s *Stats) PeriodSnapshot(p Period, key string, now time.Time) PeriodSnapsh
|
||||
out.Srcs = rows(bySrc)
|
||||
bs := make([]agrRow, 0, len(byStatus))
|
||||
for code, st := range byStatus {
|
||||
if st == nil {
|
||||
continue
|
||||
}
|
||||
bs = append(bs, agrRow{Name: code, Stat: *st})
|
||||
}
|
||||
sort.Slice(bs, func(i, j int) bool {
|
||||
@ -192,7 +195,11 @@ func (s *Stats) PeriodSnapshot(p Period, key string, now time.Time) PeriodSnapsh
|
||||
sort.Strings(ks)
|
||||
out.Bucket = make([]PeriodBucket, 0, len(ks))
|
||||
for _, k := range ks {
|
||||
out.Bucket = append(out.Bucket, PeriodBucket{Bucket: k, Stat: *buckets[k]})
|
||||
// Defensive for the same reason rows() is: ks comes from the bucket map's
|
||||
// keys, and a nil value would panic on the deref below.
|
||||
if st := buckets[k]; st != nil {
|
||||
out.Bucket = append(out.Bucket, PeriodBucket{Bucket: k, Stat: *st})
|
||||
}
|
||||
}
|
||||
|
||||
// The window is only "complete" if the audit walk actually reached back
|
||||
|
||||
140
internal/gateway/stats_unknown_key_test.go
Normal file
140
internal/gateway/stats_unknown_key_test.go
Normal file
@ -0,0 +1,140 @@
|
||||
package gateway
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
// TestSnapshotUnknownKeyDoesNotPanic covers a nil-dereference that killed the
|
||||
// response for any /api/stats request carrying a ?key= the statistics had no
|
||||
// entry for.
|
||||
//
|
||||
// The maps in Snapshot are keyed by the MASKED key id (keyID(req.Key)) — that
|
||||
// is what by_key reports and what the dashboard sends. An admin may pass anything
|
||||
// in the query parameter, including the full key, and `s.byKey[key]` then yields
|
||||
// the zero value: a nil *Stat. rows() dereferenced every value, so the handler
|
||||
// panicked and the client got an empty reply plus a panic stack in the log.
|
||||
//
|
||||
// Reproduced against the live gateway before the fix:
|
||||
//
|
||||
// /api/stats?period=day&limit=1&key=sk-gw-7ffe12c5cbacec40bd39a7291df9a9ed
|
||||
// → curl: (52) Empty reply from server
|
||||
// → log: http: panic serving …: invalid memory address or nil pointer dereference
|
||||
// llmsproxy/internal/gateway.rows stats.go:771
|
||||
//
|
||||
// A key with no recorded traffic is a legitimate question, and zero is its
|
||||
// answer — not a crash.
|
||||
func TestSnapshotUnknownKeyDoesNotPanic(t *testing.T) {
|
||||
s := NewStats(16)
|
||||
// One recorded key, masked the way Record masks it.
|
||||
s.Record(Req{Key: "***aaaaaa", Type: "chat", Model: "m", Prompt: 5, Compl: 2})
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
key string
|
||||
}{
|
||||
{"masked key that exists", "***aaaaaa"},
|
||||
{"masked key that does not exist", "***zzzzzz"},
|
||||
{"full key rather than the mask", "sk-gw-full-key-value"},
|
||||
{"arbitrary text", "not-a-key-at-all"},
|
||||
}
|
||||
for _, c := range cases {
|
||||
t.Run(c.name, func(t *testing.T) {
|
||||
// A panic here fails the subtest by unwinding through the test
|
||||
// goroutine; rows() used to take the whole process with it.
|
||||
out := s.Snapshot(10, c.key)
|
||||
byKey, ok := out["by_key"].([]StatsRow)
|
||||
if !ok {
|
||||
t.Fatalf("by_key is %T, want []StatsRow", out["by_key"])
|
||||
}
|
||||
total, _ := out["total"].(Stat)
|
||||
t.Logf("key=%q → by_key %d 条, total.reqs=%d", c.key, len(byKey), total.Reqs)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestSnapshotUnknownKeyReportsZeroNotAbsence pins the semantics: a key with no
|
||||
// traffic answers zero, and the by_key row still exists so a dashboard that keys
|
||||
// off its presence renders a row of zeros rather than dropping the key.
|
||||
func TestSnapshotUnknownKeyReportsZeroNotAbsence(t *testing.T) {
|
||||
s := NewStats(16)
|
||||
out := s.Snapshot(10, "***zzzzzz")
|
||||
byKey, _ := out["by_key"].([]StatsRow)
|
||||
if len(byKey) != 1 {
|
||||
t.Fatalf("by_key has %d rows, want 1: the queried key must appear with zeros", len(byKey))
|
||||
}
|
||||
if byKey[0].Name != "***zzzzzz" {
|
||||
t.Errorf("row name = %q, want ***zzzzzz", byKey[0].Name)
|
||||
}
|
||||
if byKey[0].Reqs != 0 {
|
||||
t.Errorf("reqs = %d, want 0", byKey[0].Reqs)
|
||||
}
|
||||
}
|
||||
|
||||
// TestPeriodSnapshotUnknownKeyDoesNotPanic is the same guard on the period view,
|
||||
// which builds its own buckets and calls the same flattening helpers.
|
||||
func TestPeriodSnapshotUnknownKeyDoesNotPanic(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
audit := dir + "/audit.jsonl"
|
||||
writeAuditFile(t, audit, 3, "***aaaaaa", time.Now().Add(-time.Hour).UnixMilli())
|
||||
s := periodStats(t, audit)
|
||||
|
||||
for _, key := range []string{"***aaaaaa", "***zzzzzz", "sk-gw-full-key-value"} {
|
||||
out := s.PeriodSnapshot(PeriodAll, key, time.Now())
|
||||
t.Logf("key=%q → total.reqs=%d byKey=%d buckets=%d",
|
||||
key, out.Total.Reqs, len(out.ByKey), len(out.Bucket))
|
||||
}
|
||||
}
|
||||
|
||||
// TestRowsSkipsNilValues pins the defensive behaviour directly, so a future
|
||||
// caller that reintroduces a nil entry fails here rather than in production.
|
||||
func TestRowsSkipsNilValues(t *testing.T) {
|
||||
in := map[string]*Stat{
|
||||
"has-traffic": {Reqs: 5},
|
||||
"no-entry": nil,
|
||||
}
|
||||
got := rows(in)
|
||||
if len(got) != 1 {
|
||||
t.Fatalf("rows() returned %d rows, want 1 (the nil entry must be skipped): %+v", len(got), got)
|
||||
}
|
||||
if got[0].Name != "has-traffic" {
|
||||
t.Errorf("kept %q, want has-traffic", got[0].Name)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRowsEmptyIsEmptyNotNil keeps the JSON shape honest: an all-nil map yields
|
||||
// an empty list, which encodes as [] rather than null — the frontend does
|
||||
// Array.isArray checks on these fields.
|
||||
func TestRowsEmptyIsEmptyNotNil(t *testing.T) {
|
||||
got := rows(map[string]*Stat{"a": nil})
|
||||
if got == nil {
|
||||
t.Fatal("rows() returned nil; the report field must stay [] instead of null")
|
||||
}
|
||||
if len(got) != 0 {
|
||||
t.Fatalf("got %d rows, want 0", len(got))
|
||||
}
|
||||
}
|
||||
|
||||
// TestStatsAPIUnknownKeyReturnsJSON exercises the HTTP path so the assertion
|
||||
// covers the reported symptom (an empty reply), not just the helper.
|
||||
func TestStatsAPIUnknownKeyReturnsJSON(t *testing.T) {
|
||||
g := newTestGateway(t)
|
||||
srv := httptest.NewServer(g.Handler())
|
||||
defer srv.Close()
|
||||
|
||||
req, err := http.NewRequest("GET", srv.URL+"/api/stats?period=day&limit=1&key=sk-gw-full-key", nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
req.Header.Set("Authorization", "Bearer sk-test")
|
||||
resp, err := srv.Client().Do(req)
|
||||
if err != nil {
|
||||
t.Fatalf("request failed (an empty reply is what the live gateway produced): %v", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("status = %d, want 200", resp.StatusCode)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user