[server] Fix concurrent requests bypassing adaptive throttling

Merged
AS
12h ago
+230-235

Concurrent catalogue requests already used the shared adaptive limiter, but its WATCH transactions shared a Redis connection. Four concurrent requests could all pass admission while Redis counted only one. A successful response could also overwrite rate-limit learning recorded concurrently. This caused the limiter to undercount and underpace catalogue traffic; DOFEK-SERVER-82 captured the upstream 429.

Replace the connection-scoped transaction with one parameterized Lua compare-and-set, shared by every adaptive state writer. Conflicts reload current state. Existing pacing, scope keys, sync-step budget rules, and expiry remain unchanged; no dependencies, retry settings, or infrastructure changes. The server-side fix applies to requests from both clients.

Validation:

  • Real Redis: three regressions failed before the fix and pass afterward, covering provider/user admissions and concurrent success/429 learning.
  • 305 focused unit tests pass across adaptive state, HTTP handling, catalogue composition, and sync cooldown/failure handling.
  • Full pnpm lint and root/server/web TypeScript checks pass. SQL lint required starting the workspace ClickHouse fixture.
  • Independent static review found no critical or important issues.

Production deployment and catalogue refresh verification remain outstanding. No resilience knobs were added; this restores the existing policy and does not establish the upstream provider's quota.

Summary by Sourcery

Restore reliable concurrent adaptive throttling for catalogue requests by replacing connection-scoped Redis transactions with atomic compare-and-set state updates.

Bug Fixes:

  • Prevent concurrent adaptive rate-limit admissions from bypassing shared pacing and request counting.
  • Preserve rate-limit learning when success and throttling updates occur concurrently.

Enhancements:

  • Use Redis compare-and-set updates shared across adaptive admission, success, and rate-limit state writers while retaining existing pacing, scope, budget, and expiry behavior.

Documentation:

  • Document the concurrent adaptive throttling incident, its resolution, validation results, and remaining production verification risks.

Tests:

  • Add real-Redis regression coverage for concurrent provider and user admissions and concurrent rate-limit learning.
  • Update adaptive rate-limit unit tests for compare-and-set conflict handling.

Summary by cubic

Fixes concurrent catalogue requests bypassing adaptive rate-limit throttling because connection-scoped WATCH transactions undercounted admissions and could erase concurrently learned rate-limit state, which Sentry DOFEK-SERVER-82 captured as an upstream 429.

Replaces those transactions with one parameterized Lua compare-and-set shared by every adaptive state writer. Conflicts reload current state and retry. Existing pacing, scope keys, sync-step budget rules, and expiry are unchanged; no dependencies or infrastructure change, so the server-side fix applies to requests from both clients.

Validation

  • Three real-Redis regressions failed before the fix and pass after; they now also assert that conflict retries reload competing state and that each consecutive admission gap keeps existing pacing.
  • 305 focused unit tests plus full lint and TypeScript checks pass.
  • Merging current main conflicted only on incident-note entries in docs/production-incident-baseline.md; the source fix is unchanged.
  • Production deployment and catalogue refresh verification remain outstanding. No resilience knobs were added; this restores the existing policy and does not establish the upstream provider's quota.

Summary by CodeRabbit

  • Bug Fixes
    • Concurrent provider requests now preserve shared request counts and throttle state, helping keep request pacing consistent. Rate-limit updates are applied consistently when requests overlap.
  • Documentation
    • Added a production incident record covering reported symptoms, validation results, local setup prerequisites, and remaining operational risks, including outstanding production verification and uncertainty about the upstream quota.
  • Tests
    • Added coverage for concurrent provider requests and overlapping rate-limit updates.