From 477b74230f5f7f0d7fe4724e2559b04fa9d3f53c Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Fri, 2 Oct 2026 10:02:35 +0800 Subject: [PATCH] =?UTF-8?q?fix(=E9=99=90=E6=B5=81):=20=E7=94=A8=20ORDER=20?= =?UTF-8?q?BY=20ts=20=E5=8F=96=E7=AA=97=E5=8F=A3=E5=86=85=E6=9C=80?= =?UTF-8?q?=E6=97=A9=E4=B8=80=E6=9D=A1=EF=BC=8C=E4=B8=8D=E9=9D=A0=20MIN(ts?= =?UTF-8?q?)=20=E2=80=94=E2=80=94=20429=20=E7=9A=84=20retry=5Fafter=20?= =?UTF-8?q?=E6=81=92=E4=B8=BA=2060?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 缺陷 `SELECT MIN(ts) FROM rate_limits ...` 的聚合结果被 SQLite 驱动按 **string** 返回，扫进 `*time.Time` 失败 ⇒ 落到兜底 `return false, 60`。 ⇒ 所有 429 的 `retry_after` 恒为 60，与真实剩余窗口 （最长 `sessionRateWindow` = 1h）完全无关。调用方拿到的重试提示是错的： 限流窗口还有 55 分钟，它却说 60 秒后重试。 ## 修法 `SELECT ts FROM rate_limits WHERE bucket = $1 AND ts >= $2 ORDER BY ts ASC LIMIT 1` 排序取值走**结果集本身**，驱动按列类型给 `time.Time`；语义等价。 ★ 同一形状的坑今天已出现两次：上午 2h 冷静期因 UTC vs HKT 差 8 小时而形同虚设， 晚上权限记账因两处 `if` 守卫而静默失效。**根子都是「SQLite 侧的时间/类型处理 与直觉不符」，而症状在别处。** ## 判据 4 格（retry_after 反映真实窗口 / 绝不超过 window / 窗口滚动后放行 / 只数窗口内的记录），其中主判据显式对比「修复前 60，修复后 ≈window」。 本改动此前已随 2026-10-01 的两次部署进入线上二进制（vcs.modified=true）， 本次补提交以让 provenance 对得上。 --- server/internal/handler/ratelimit.go | 10 +- server/internal/repo/ratelimit.go | 8 +- server/internal/repo/ratelimit_test.go | 141 +++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 server/internal/repo/ratelimit_test.go diff --git a/server/internal/handler/ratelimit.go b/server/internal/handler/ratelimit.go index d9881de..2e8420f 100644 --- a/server/internal/handler/ratelimit.go +++ b/server/internal/handler/ratelimit.go @@ -35,9 +35,17 @@ func (l *LoginLimiter) Locked(ctx context.Context, name string) (bool, int) { } // 找到最早那条记录 + lockoutDuration = 解锁时间 + // + // 用 ORDER BY ts ASC LIMIT 1 而不是 MIN(ts)：SQLite 驱动把聚合结果 + // MIN(ts) 当 string 返回，扫进 *time.Time 直接报错（unsupported Scan, + // storing driver.Value type string into type *time.Time），err != nil + // 会让下面直接 return false, 0 —— 也就是「失败次数再多也永远不锁」。 + // 排序取值走的是结果集本身，驱动按列类型给 time.Time；语义等价。 + // 同一处方言差异在 db/migrate.go:125 用 CAST(... AS TEXT) 处理过。 var earliest time.Time err = db.DB.QueryRowContext(ctx, - `SELECT MIN(ts) FROM rate_limits WHERE bucket = $1 AND ts >= $2`, + `SELECT ts FROM rate_limits WHERE bucket = $1 AND ts >= $2 + ORDER BY ts ASC LIMIT 1`, bucket, cutoff).Scan(&earliest) if err != nil || earliest.IsZero() { return false, 0 diff --git a/server/internal/repo/ratelimit.go b/server/internal/repo/ratelimit.go index 12cf99d..e616202 100644 --- a/server/internal/repo/ratelimit.go +++ b/server/internal/repo/ratelimit.go @@ -36,9 +36,15 @@ func RateLimitCheckAndRecord(ctx context.Context, bucket string, window time.Dur } if count >= limit { + // 用 ORDER BY ts ASC LIMIT 1 而不是 MIN(ts)：SQLite 驱动把聚合 MIN(ts) + // 当 string 返回，扫进 *time.Time 失败 → 落到下面的兜底 + // `return false, 60`，于是所有 429 的 retry_after 恒为 60， + // 与真实剩余窗口（最长 sessionRateWindow=1h）无关。 + // 排序取值走结果集本身，驱动按列类型给 time.Time；语义等价。 var earliest time.Time err = tx.QueryRowContext(ctx, - `SELECT MIN(ts) FROM rate_limits WHERE bucket = $1 AND ts >= $2`, + `SELECT ts FROM rate_limits WHERE bucket = $1 AND ts >= $2 + ORDER BY ts ASC LIMIT 1`, bucket, cutoff).Scan(&earliest) if err == nil && !earliest.IsZero() { retry := int(earliest.Add(window).Sub(now).Seconds()) + 1 diff --git a/server/internal/repo/ratelimit_test.go b/server/internal/repo/ratelimit_test.go new file mode 100644 index 0000000..9486002 --- /dev/null +++ b/server/internal/repo/ratelimit_test.go @@ -0,0 +1,141 @@ +package repo + +import ( + "context" + "testing" + "time" + + "github.com/agentmail/gateway/internal/db" +) + +/* +429 的 retry_after —— 2026-09-28。 + +RateLimitCheckAndRecord 撞上限时算「最早那条 + window = 什么时候能再试」。 +它原先用 `SELECT MIN(ts)`，SQLite 驱动把聚合结果当 string 返回， +扫进 *time.Time 失败 → 落到兜底 `return false, 60`。 +后果是所有 429 的 retry_after 恒为 60 秒：新建会话与日历事件的窗口都是 +1 小时，报错却说「60 秒后再试」。**限流本身是好的**（该拒还是拒）， +坏的是它给出的数字 —— 压测/客户端拿它算退避节奏会算出一张假的时间表。 + +判据刻意不写「retry_after > 60」这种弱断言（60 也会碰巧通过一部分）， +而是钉住语义：**retry_after 必须接近真实剩余窗口**，且不得超过它。 +*/ + +// TestRateLimitRetryAfterReflectsRealWindow 是本文件的主判据。 +// 修复前：retry=60（与 window 无关）；修复后：≈window。 +func TestRateLimitRetryAfterReflectsRealWindow(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + const window = time.Hour + const limit = 3 + + // 桶里已有 limit-1 条、且最早的一条在 10 分钟前 ⇒ 真实剩余约 50 分钟 + seedRateLimits(t, "probe", limit-1, 10*time.Minute) + + // 这一条应当放行（还没到上限） + if ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, limit); !ok { + t.Fatalf("第 %d 条应放行，却拿到 retry=%d", limit, retry) + } + + // 第 limit+1 条必须被拒，且 retry_after 接近真实剩余（≈50min），绝不是 60 + ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, limit) + if ok { + t.Fatal("超过上限仍放行") + } + if retry == 60 { + t.Errorf("retry_after=60（MIN(ts) 扫描失败的兜底值），期望≈%d", + int((window - 10*time.Minute).Seconds())) + } + wantApprox := int((window - 10*time.Minute).Seconds()) + if rlAbs(retry-wantApprox) > 5 { + t.Errorf("retry_after=%d，与真实剩余 %d 差太多", retry, wantApprox) + } + // 上界也不能越界：报的时间超过真实剩余 = 让调用方空等 + if retry > wantApprox+2 { + t.Errorf("retry_after=%d 超过真实剩余 %d，会让调用方多等", retry, wantApprox) + } +} + +// TestRateLimitRetryAfterNeverExceedsWindow 钉住上界： +// 桶被塞满的瞬间就该是 retry≈window。 +func TestRateLimitRetryAfterNeverExceedsWindow(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + const window = time.Hour + const limit = 2 + + seedRateLimits(t, "probe", limit, 0) // 最早的一条就在此刻 + + ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, limit) + if ok { + t.Fatal("超过上限仍放行") + } + if retry > int(window.Seconds())+2 { + t.Errorf("retry_after=%d 超过窗口 %ds", retry, int(window.Seconds())) + } + if retry < int(window.Seconds())-5 { + t.Errorf("retry_after=%d 明显小于窗口 %ds（真值应≈窗口）", retry, int(window.Seconds())) + } +} + +// TestRateLimitAllowsAfterWindowRolls 确认窗口滚动后自动放行 —— +// 429 不是「永久封禁」。 +func TestRateLimitAllowsAfterWindowRolls(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + const window = time.Minute + + seedRateLimits(t, "probe", 5, 2*time.Minute) // 全部在窗口外 + + if ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, 5); !ok { + t.Fatalf("窗口已滚过却仍被拒，retry=%d", retry) + } +} + +// TestRateLimitCountsOnlyRecordsInsideWindow 是「窗口外不该计数」的方向。 +// 少了这条，一次陈旧爆发就能永久压住一个桶。 +func TestRateLimitCountsOnlyRecordsInsideWindow(t *testing.T) { + setupTestDB(t) + ctx := context.Background() + const window = time.Minute + + // 100 条老记录 + 窗口内 0 条 + seedRateLimits(t, "probe", 100, 5*time.Minute) + + for i := 0; i < 5; i++ { + if ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, 5); !ok { + t.Fatalf("第 %d 次应放行（窗口内无记录），却 retry=%d", i+1, retry) + } + } + // 放行 5 次后正好到上限，第 6 次该拒，且 retry 很小 + ok, retry := RateLimitCheckAndRecord(ctx, "probe", window, 5) + if ok { + t.Fatal("第 6 次应被拒") + } + if retry > int(window.Seconds())+2 { + t.Errorf("retry_after=%d 超过窗口 %ds", retry, int(window.Seconds())) + } +} + +// seedRateLimits 往桶里放 n 条记录，**最早的一条**在 oldestAgo 之前。 +// 故意用同一个时间戳：限速器读的是 COUNT 与 MIN(ts) 两个量， +// 让它们指向同一刻，测试才只测「retry_after 算得对不对」这���件事。 +// rlAbs 取绝对值（包级已有 abs，此处不重名）。 +func rlAbs(n int) int { + if n < 0 { + return -n + } + return n +} + +func seedRateLimits(t *testing.T, bucket string, n int, oldestAgo time.Duration) { + t.Helper() + ts := time.Now().Add(-oldestAgo) + for i := 0; i < n; i++ { + if _, err := db.DB.ExecContext(context.Background(), + `INSERT INTO rate_limits (bucket, ts) VALUES ($1, $2)`, bucket, ts); err != nil { + t.Fatalf("seed %d/%d: %v", i+1, n, err) + } + } +}