fix(limits): enforcement-aware fallback TTL clamp

Re-review round on 5cc6814 found round 1's 48h fallback clamp
defective: it broke TTL > window-length for the MONTH window on
memoryStore, turning tokensPerMonth/costPerMonthUSD into a rolling
~48h budget (fail-open) whenever Redis is absent or down.

1. counterIncr gains enforceTTL time.Duration — the minimum retention
   window's own enforcement correctness requires (enforceTTLFor:
   min->2m, hour->2h, day->25h, month->32*24h, matching each window's
   pre-history-bump TTL). newCounterIncr builds every checkAndCount/
   account entry through one helper so a (window, ttl) pair can never
   reach incrMulti without its matching floor. memoryStore's clampTTL
   now takes (ttl, enforceTTL) and applies max(min(ttl,
   memoryStoreMaxTTL), enforceTTL): the 48h ceiling still bounds
   min/hour/day (unaffected/still-capped as before), but the floor
   wins for month, landing it back at ~32 days (1-2 live keys per
   scope/metric — negligible against the day-key savings that
   motivated the ceiling). redisStore ignores enforceTTL; incrBy's own
   single-key path passes enforceTTL=0 (ceiling only, unchanged
   behavior — it has no window to derive a floor from and has no
   production caller since the batching, see (3) below).

2. limits.go's TTL-invariant comment restated precisely: the consts
   still satisfy TTL > window-length exactly as applied to Redis; that
   did not automatically extend to memoryStore's own effective TTL
   after its ceiling, which is what round 1 broke and round 2 fixes.
   README: the Limits and accounting "Fallback" bullet now says
   enforcement stays correct on the fallback at every window
   (including tokensPerMonth/costPerMonthUSD), and that the 48h
   ceiling applies to usage-history CHARTING retention only.
   LimitsConfig's window table (tokensPerDay/tokensPerMonth rows)
   cross-links it. The "resets on a rebuild" bullet's stale
   limiter.storeIncrBy reference is corrected to storeIncrMulti and
   now distinguishes rebuild-loss from the fallback's own per-window
   TTL expiry.

3. counterStore.incrBy, limiter.storeIncrBy, and limiter.incrCounter's
   doc comments now state plainly they have no production caller since
   the batching (perf review, 2026-08-21) and are kept for direct
   counter seeding in tests — 5 call sites, all in admin_test.go's
   TestAdminUsage_MathAgainstSeededCounters, correcting the prior fix
   report's "dozens of tests" overclaim to the actual count.

Tests: TestClampTTL_ExactMathTable pins the ceiling-vs-floor
precedence for every window's real (ttl, enforceTTL) pair.
TestEnforceTTLFor_MatchesTheOriginalPreHistoryBumpValues pins the four
floor values directly. TestMemoryStore_IncrMulti_
MonthCounterSurvivesPast48hAndStillEnforces drives a real
TokensPerMonth budget through account/checkAndCount on the fallback
5 days in and confirms both survival and enforcement.
TestMemoryStore_IncrMulti_DayCounterStillClampsAt48h confirms day
still resets at the 48h ceiling (its own floor sits below it).

Gate: go test ./... -count=1, -race, gofmt, vet, make yaegi-check —
all green.
This commit is contained in:
2026-08-21 01:17:15 +01:00
parent 5cc681496c
commit f9fa14ef3f
3 changed files with 307 additions and 65 deletions
+118
View File
@@ -1256,3 +1256,121 @@ func TestTokenBudget_ExactBoundaryViolates(t *testing.T) {
t.Error("storeDown must be false for a real limit breach")
}
}
// --- review round 2, 2026-08-21: enforcement-aware fallback TTL clamp ---
// TestClampTTL_ExactMathTable pins clampTTL's ceiling-vs-floor precedence
// for every window's real (ttl, enforceTTL) pair. Round 1's clamp applied
// memoryStoreMaxTTL unconditionally; round 2 corrects it so a window's
// own enforceTTL floor always wins a conflict with that ceiling — this is
// the exact math that keeps min/hour/day capped at 48h while month lands
// back at its own ~32-day floor.
func TestClampTTL_ExactMathTable(t *testing.T) {
cases := []struct {
name string
ttl time.Duration
enforceTTL time.Duration
want time.Duration
}{
{"min: 2m sits under both ceiling and floor, unchanged", minWindowTTL, enforceTTLFor(windowMin), minWindowTTL},
{"hour: 48h ttl equals the 48h ceiling; its own 2h floor never applies", hourWindowTTL, enforceTTLFor(windowHour), memoryStoreMaxTTL},
{"day: 35d ttl clamped down to the 48h ceiling, still above the 25h floor", dayWindowTTL, enforceTTLFor(windowDay), memoryStoreMaxTTL},
{"month: 400d ttl would clamp to 48h, but the 32d floor wins instead", monthWindowTTL, enforceTTLFor(windowMonth), enforceTTLFor(windowMonth)},
{"enforceTTL=0 (incrBy's own single-key path): pure ceiling, no floor", 400 * 24 * time.Hour, 0, memoryStoreMaxTTL},
{"ttl already under both ceiling and floor passes through unchanged", 90 * time.Minute, time.Hour, 90 * time.Minute},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
if got := clampTTL(c.ttl, c.enforceTTL); got != c.want {
t.Errorf("clampTTL(%v, %v) = %v, want %v", c.ttl, c.enforceTTL, got, c.want)
}
})
}
}
// TestEnforceTTLFor_MatchesTheOriginalPreHistoryBumpValues pins
// enforceTTLFor's four values directly — they are not arbitrary, they
// match dayWindowTTL/monthWindowTTL's values from before the v0.2
// data-layer task's history-retention bump (25h/32d), which were
// themselves already windowLength+margin for correct single-window
// enforcement.
func TestEnforceTTLFor_MatchesTheOriginalPreHistoryBumpValues(t *testing.T) {
cases := []struct {
window string
want time.Duration
}{
{windowMin, 2 * time.Minute},
{windowHour, 2 * time.Hour},
{windowDay, 25 * time.Hour},
{windowMonth, 32 * 24 * time.Hour},
}
for _, c := range cases {
if got := enforceTTLFor(c.window); got != c.want {
t.Errorf("enforceTTLFor(%q) = %v, want %v", c.window, got, c.want)
}
}
}
// TestMemoryStore_IncrMulti_MonthCounterSurvivesPast48hAndStillEnforces
// is the review-round-2 regression case: a month counter written via
// incrMulti — the real production path, checkAndCount/account — must
// stay alive, not silently reset to 0, well past 48h, and a
// TokensPerMonth limit must still enforce correctly at that point. This
// is exactly what round 1's unconditional 48h ceiling broke: a month
// counter's key would have expired and reset roughly every two days,
// turning the budget into a rolling ~48h one instead of a true
// calendar-month one whenever Redis is absent or down.
func TestMemoryStore_IncrMulti_MonthCounterSurvivesPast48hAndStillEnforces(t *testing.T) {
l := newLimiter(nil, true) // nil store -> every operation uses the in-process fallback
now := time.Date(2026, 8, 1, 0, 0, 0, 0, time.UTC)
l.nowFn = func() time.Time { return now }
scopes := []limitScope{{kind: "group", id: "g", limits: &LimitsConfig{TokensPerMonth: 100}}}
l.account(scopes, usage{prompt: 60, completion: 20}, 0) // 80 tokens on day 1
// Advance 5 days: well past memoryStoreMaxTTL's 48h ceiling, far short
// of monthWindowTTL's own natural rollover.
now = now.Add(5 * 24 * time.Hour)
l.nowFn = func() time.Time { return now }
tokIn, ok := l.getCounter("group", "g", metricTokIn, windowMonth, now)
if !ok || tokIn != 60 {
t.Fatalf("tokin:month after 5 days = %d, ok=%v, want 60 (the key must not have expired at 48h)", tokIn, ok)
}
// Push the total over budget and confirm enforcement still fires —
// proves the surviving counter is actually read by
// tokenBudgetViolation, not merely present.
l.account(scopes, usage{completion: 21}, 0) // 60 + 41 = 101 > 100
if v := l.checkAndCount(scopes); v == nil {
t.Fatal("want a TokensPerMonth violation 5 days in; the round-1 clamp bug would have reset the counter well before this")
}
}
// TestMemoryStore_IncrMulti_DayCounterStillClampsAt48h asserts the
// round-1 clamp's original goal — bounding the fallback's own live-key
// growth — still holds for day, whose own enforcement floor (25h) sits
// below the 48h ceiling: a day counter written via incrMulti expires and
// resets once its key crosses 48h, well before dayWindowTTL's own 35-day
// retention would otherwise have kept it alive.
func TestMemoryStore_IncrMulti_DayCounterStillClampsAt48h(t *testing.T) {
m := newMemoryStore()
now := time.Date(2026, 8, 1, 0, 0, 0, 0, time.UTC)
m.nowFn = func() time.Time { return now }
entry := counterIncr{key: "k", delta: 10, ttl: dayWindowTTL, enforceTTL: enforceTTLFor(windowDay)}
if _, err := m.incrMulti([]counterIncr{entry}); err != nil {
t.Fatal(err)
}
now = now.Add(49 * time.Hour) // just past the 48h ceiling
m.nowFn = func() time.Time { return now }
got, err := m.get("k")
if err != nil {
t.Fatal(err)
}
if got != 0 {
t.Errorf("day counter at 49h past write = %d, want 0 (expired at the 48h ceiling; day's own 25h floor never raises it)", got)
}
}