fix(provider-limits): close TOCTOU race in quota recovery clear (I2) - #6139
diegosouzapw merged 4 commits into
Conversation
…ring sanitization (diegosouzapw#6048) Port the release/v3.8.44 fix to main so the code-scanning alert closes on the default branch. Parse the URL and assert on the exact hostname instead of a substring match — `includes("www.kimi.com")` would also accept a hostile host like `www.kimi.com.evil.net` or `evil.net/?x=www.kimi.com` (js/incomplete-url-substring-sanitization).
…overs After a successful provider-limits quota refresh returns usable quota (remaining > 0, remainingPercentage > 0, or unlimited: true), clear stale transient cooldown/error state on the connection row so priority combos pick the primary connection again without manual Retest. The recovery is generic across providers and only fires when: - usage.quotas has at least one window with usable quota, and - the connection is NOT in a terminal status (credits_exhausted, banned, expired, deactivated), and - the connection currently has transient state (testStatus unavailable, rateLimitedUntil, lastError*, errorCode, or backoffLevel > 0). Reuses clearRecoveredProviderState from src/sse/services/auth.ts. syncExpiredStatusIfNeeded now returns the updated connection so a recovered-quota path cannot overwrite a freshly written "expired" terminal status. Tests cover: GLM transient cooldown recovery, terminal-status guard (credits_exhausted/banned/expired), and error-only quota response non-recovery.
maybeClearRecoveredQuotaState was calling clearRecoveredProviderState unconditionally. Between the start-of-call read and the clear UPDATE, a concurrent markAccountUnavailable (or connectionRecovery tick) could write a fresh error state that the clear then clobbered — making a just-failed connection look healthy, breaking the backoff escalation, and producing misleading dashboard state. clearRecoveredProviderState now takes an optional expectedState and, when present, routes through clearConnectionErrorIfUnchanged — a new atomic conditional UPDATE in src/lib/db/providers.ts keyed on (test_status, last_error_at, rate_limited_until). If the row changed since the caller's snapshot, the UPDATE matches 0 rows and the clear is skipped, preserving the freshest error state. last_error_at is the primary CAS signal — markAccountUnavailable always bumps it on every cooldown/error write, so an unchanged value reliably proves no concurrent write happened. Signature is backward-compatible: post-success callers (chat, embeddings, images, ...) keep the unconditional clear path because their credentials come from a request that just succeeded. Only the quota-recovery path opts into the conditional primitive. Tests cover: CAS hit (clears, returns true), CAS miss after concurrent mark (preserves fresh state, returns false), and an end-to-end quota- fetch race simulation that injects a concurrent write during the mocked usage fetch. Builds on diegosouzapw#6128 (I1 fix) — branches off fix/quota-cooldown-recovery so the diff auto-shrinks once that PR merges.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…recovery-toctou-cas # Conflicts: # src/lib/usage/providerLimits.ts # tests/unit/provider-limits-recovery.test.ts
|
Merged — thank you, @janeza2! 🙏 Excellent follow-up: this closes the TOCTOU race (flagged as I2 in the #6128 review) where |
c51693b
into
diegosouzapw:release/v3.8.44
…iegosouzapw#6139) Close TOCTOU race in quota recovery clear via CAS primitive (I2 from diegosouzapw#6128). Integrated into release/v3.8.44.
Summary
Closes the TOCTOU (Time-Of-Check-To-Time-Use) race in the quota-recovery path that was called out as I2 in the review of #6128.
The race
maybeClearRecoveredQuotaStatereads a connection snapshot at the start offetchLiveProviderLimitsWithOptions, fetches quota, then callsclearRecoveredProviderStateto clear the transient error state. Between that initial read and the clear UPDATE, a concurrentmarkAccountUnavailable(from a chat request that just hit a fresh 429) can write a newer error state — which the unconditional clear then overwrites. Result: a just-failed connection flips back toactivein the DB, breaking backoff escalation and producing misleading dashboard state.The fix
clearConnectionErrorIfUnchanged(id, expected)primitive insrc/lib/db/providers.ts— a single atomic conditionalUPDATEkeyed on(test_status, last_error_at, rate_limited_until). Returnstrueif applied,falseif a concurrent write changed the row.clearRecoveredProviderStatenow takes an optionalexpectedState. When provided, it routes through the CAS primitive; when omitted, the existing unconditional path runs (preserves all 13 post-success callers — chat/embeddings/images/etc. — unchanged).maybeClearRecoveredQuotaStatepasses(testStatus, lastErrorAt, rateLimitedUntil)from its in-memory snapshot as the CAS token.last_error_atis the primary signal becausemarkAccountUnavailablealways bumps it on every cooldown/error write.SQLite's single-statement UPDATE is atomic under WAL journaling, so no mutex is needed.
Test plan
New tests in
tests/unit/provider-limits-recovery.test.ts:CAS primitive clears when expected state matches— direct call toclearConnectionErrorIfUnchangedwith matching token returnstrueand clears the row.CAS primitive aborts when state changed concurrently— simulates a concurrentmarkAccountUnavailablewriting fresh state between read and clear; the CAS UPDATE returnsfalseand the fresh state is preserved.quota recovery path does NOT overwrite a concurrent mark (TOCTOU closed)— end-to-end: mocks the usage fetch to inject a concurrent DB write mid-fetch, runsfetchAndPersistProviderLimits, asserts the fresh mark survives.CI runs the full suite (
npm run test:unit). Local env couldn't runtsx(no node_modules), relied onReadLints+ visual review of diff. No new lint errors introduced — all reported errors are pre-existing environment issues (@types/nodemissing,process/unreftyping).Relationship to #6128
This PR branches off
fix/quota-cooldown-recovery(the I1 fix). The diff currently shows both I1 + I2 changes againstmainbecause #6128 is still open. Once #6128 merges, this PR's diff auto-shrinks to just the I2 incremental changes (the CAS primitive + signature change + caller wiring + 3 new tests).If preferred, I can also rebase this onto
mainafter #6128 merges.