fix(engine): clamp rate-limit retryAfterMs to at most one window on a backward clock - #5855
Conversation
… backward clock evaluateLocalRateLimit's windowElapsed check only detects a FORWARD clock, so if nowMs steps backward relative to a bucket's windowStartMs (an NTP correction or container/VM clock reset), resetAtMs - now grows by the jump distance on top of windowMs -- a caller blocked right after a half-second backward step on a 60s window could be told to wait minutes (JSONbored#5829). Clamp retryAfterMs to windowMs so a rolling-window limiter never reports a wait longer than its own window; forward behavior is unchanged (the clamp is a no-op when resetAtMs - now <= windowMs). Add a dedicated test (the module had none) covering the allowed, blocked-forward, and backward-clock-clamp cases. Closes JSONbored#5829
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5855 +/- ##
=======================================
Coverage 95.11% 95.11%
=======================================
Files 587 587
Lines 46511 46511
Branches 14870 14870
=======================================
Hits 44240 44240
Misses 1515 1515
Partials 756 756
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-14 20:11:33 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 3 non-blocking
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
[BETA] Chat with GittensoryAsk Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
evaluateLocalRateLimit(the Governor's pure rolling-window rate-limit primitive) computesretryAfterMsfromresetAtMs - now, but itswindowElapsedcheck only detects a forward-moving clock:If
nowMsever steps backward relative to a bucket'swindowStartMs— a real possibility for aDate.now()-derived clock across an NTP correction or a container/VM clock reset —windowElapsedstaysfalse,effectiveWindowStartkeeps the oldwindowStartMs, andresetAtMs - nowgrows by the full backward-jump distance on top ofwindowMs. A caller blocked right after a half-second backward step on a 60-second window is told to wait minutes, worse the larger the jump (#5829).Fix: clamp
retryAfterMsto at most onewindowMs, so a rolling-window limiter can never report a wait longer than its own window:Forward behavior is unchanged — when
resetAtMs - now <= windowMs(the normal case) the clamp is a no-op. This is the minimal, targeted fix the issue asks for; it does not alter the allow/deny or window-reset logic.The module had no dedicated test (it was only exercised indirectly, never with
nowMsmoving backward). This addstest/unit/governor-rate-limit-backoff.test.tscovering the three branches of the changed line: allowed (no wait), blocked under a forward clock (clamp is a no-op), and blocked after a backward clock jump (clamped towindowMs, not the inflated value).Scope
CONTRIBUTING.md; nosite//CNAME/lovable.Validation
git diff --checknpm run typecheck(root +@loopover/enginebuild) cleannpm run test:coverage— the changed line (rate-limit.ts:71) is at 100% line + branch coverage (verified via the v8 JSON report: theallowed ? 0branch, and both sides of theMath.minclamp). 3 new tests pass.governor-*,chokepoint, andwrite-rate-limitroot-vitest suites pass (218 tests) — the clamp is a no-op on a forward clock.mainimmediately before pushing — no base conflict.If any required check was skipped, explain why:
actionlint,test:workers,ui:*,npm auditwere not run — this changes one engine source line + a test; no workflow, worker, UI, or dependency surface. The fullnpm run test:ciruns them on CI. (One local unit test,miner-mcp-governor-decisions, fails on this Windows box withEPERMon temp dirs — a filesystem env flake that does not import rate-limit and passes on Linux CI.)Safety
Auth/CORS/session, API/OpenAPI/MCP, and UI safety boxes are not applicable.