fix: preserve non-transient cooldown reasons

[teamleaderleo]

Summary

PR #15856 fixed Claude cooldown deadlines with GREATEST, but regressed the failure reason: a shorter invalid_credential cooldown could no longer replace a longer rate_limited reason. This PR fixes that regression and the pre-existing native asymmetry in one shared SQL helper.

Both account tables now use the same rule in one UPDATE statement:

• The deadline remains GREATEST(COALESCE(existing, new), new).
• A non-transient reason such as invalid_credential outranks a transient reason, even when its deadline is shorter.
• Otherwise, the reason follows the deadline that wins.
• updated_at is always bumped.

Sequence	Before this PR	After this PR
429 5h rate_limited, then 401 15m invalid_credential	Claude: 5h, rate_limited (wrong). Native: 5h, invalid_credential	Claude and native: 5h, invalid_credential
401 15m invalid_credential, then 503 20s upstream_unavailable	Claude: 15m, invalid_credential. Native: 15m, upstream_unavailable (wrong)	Claude and native: 15m, invalid_credential
429 5h rate_limited, then 503 20s upstream_unavailable	Claude: 5h, rate_limited. Native: 5h, upstream_unavailable (wrong)	Claude and native: 5h, rate_limited
503 20s upstream_unavailable, then 429 5h rate_limited	Claude and native: 5h, rate_limited	Claude and native: 5h, rate_limited

The reason is load-bearing, not just display metadata. Claude's capacityRetryAfter excludes invalid_credential accounts so the proxy does not hold a request for a credential that needs human intervention. Native nextCapacityAvailableAt applies the same exclusion when deciding whether capacity is coming back. The shared cooldownWrite.ts module now owns the taxonomy and precedence rule, and the native query uses the same predicate instead of a separate hardcoded string.

Tests

The focused command was red before the fix at commit b6bad0dd81e:

$ cd web && bun test tests/coderouter-claude-cooldown-db-behavior.test.ts
5 pass
6 fail
65 expect() calls

The failures included Claude losing invalid_credential after a longer rate_limited wall and native overwriting a surviving non-transient reason with upstream_unavailable.

The same command is green at commit 11c369f2b0b:

$ cd web && bun test tests/coderouter-claude-cooldown-db-behavior.test.ts
11 pass
0 fail
77 expect() calls

Additional validation:

• bun x tsc --noEmit passed.
• bun run lint:complexity passed with 42 findings matched to the unchanged baseline.
• The test covers all four sequences for both Claude and native stores, plus the capacity consumers.
• CI runs these DB behavior tests in the web / web-db-migrations job through bun run test:db:behavior. web/scripts/run-db-behavior-tests.sh discovers every DB-gated test file, runs them serially, and fails if a file runs zero tests or skips one. That job passed on fix: preserve longest Claude upstream cooldown #15856, so the merged test ran in CI. The local real-Postgres run is the faster loop for this follow-up.

There is no fleet dogfood because PR #8029 disabled Vercel branch previews, so an unmerged web change has no preview URL.

Changelog

• Fixed: Preserve non-transient cooldown reasons across concurrent Claude and native account failures.

🤖 Generated with Claude Code

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Fixes the cooldown failure-reason regression where a shorter invalid_credential cooldown could no longer replace a longer rate_limited reason. Claude and native account stores now share one SQL helper (cooldownWrite) that keeps the longest deadline and gives non-transient reasons precedence over transient ones while their deadline is live; once expired, a fresh transient failure replaces them. It also fixes the native asymmetry where upstream_unavailable could overwrite a surviving non-transient reason. The stored reason now always describes the live cooldown, so capacity consumers (Claude's capacityRetryAfter and native nextCapacityAvailableAt) exclude invalid_credential correctly, and a NULL reason on a live deadline stays untouched. Tests cover all sequence orders, expired-reason recovery, NULL-reason handling, and the capacity paths.

^{Written for commit b20de6b. Summary will update on new commits.}

[coderabbitai]

Warning

Review limit reached

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e3d3560f-9c17-4afa-b2cf-e0f73ebaa293

📥 Commits

Reviewing files that changed from the base of the PR and between 1bd5083 and b20de6b.

📒 Files selected for processing (4)

• web/services/coderouter/claudeUpstream.ts
• web/services/coderouter/cooldownWrite.ts
• web/services/coderouter/repository.ts
• web/tests/coderouter-claude-cooldown-db-behavior.test.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[teamleaderleo]

Review

Reviewed at 11c369f2b0b64092e53fbaa76f7a716edfa362a9 (merge base 8599250a7cd), 4 files, +184/-22, by me plus an independent review agent. One finding blocks merge.

Blocking: the new precedence has no time bound, and the Claude table can never clear the flag

The rule "a stored non-transient reason outranks a new transient reason" is right while the account is still cooling for that reason. It is wrong once that cooldown has expired, and nothing bounds it.

On coderouter_claude_accounts the only two writers of last_failure_code are insert (writes null) and markCooldown. ClaudeAccountStore.update's patch type accepts only label, state and identifier, and touchUsed sets only lastUsedAt. There is no equivalent of the native table's four clear-on-repair sites. So the flag is set once and stays for the life of the row.

The sequence that breaks, with every link checked in the code rather than assumed:

1. Anthropic returns one 401/403 for account X, which happens for an auth blip or an org permission flap, not only for a genuinely revoked key. classifyAttempt gives markCooldown(X, t+15m, "invalid_credential").
2. Fifteen minutes later the cooldown expires. select()'s healthy filter reads only cooldown_until, so X is routed to again and succeeds. That is the designed recovery. last_failure_code is still invalid_credential.
3. Days later X gets a 503: markCooldown(X, t+20s, "upstream_unavailable"). Under this PR the stored reason wins, so the account still reads invalid_credential while the deadline moves to t+20s.
4. Every account is cooling, so select() returns exhausted. capacityRetryAfter (claudeUpstream.ts:459) skips X because the reason is non-transient, so capacityRetryAfterSeconds is null. claudeProxy.ts:536 passes that null through, and CapacityHold.wait opens with if (retryAfterMs === null) return false. The client gets an immediate failure instead of being held the twenty seconds it would have taken X to come back.

Before this PR the later transient write overwrote the stale reason, so step 4 held correctly. This is a regression on the capacity-hold path the change exists to serve.

The fix is to bound the precedence to a live cooldown: a stored non-transient reason outranks a new transient one only while cooldown_until is still in the future. Once it has expired the account has already been retried, so the fresh reason is the better description. That keeps every behaviour this PR adds, including the case it was opened for (a revocation arriving during a long rate-limit wall still wins), and it fixes both tables in the shared helper rather than only the Claude one.

Two smaller items going in with it:

• nonTransientFailureCodePredicate renders nothing for an empty code set, which produces NOT () and a Postgres syntax error at runtime. tsc does not catch it. It needs a false guard.
• The test's epoch comparisons put a millisecond JS Date against extract(epoch from cooldown_until) * 1000, which carries microsecond precision. It passes today only because both writers happen to pass a JS Date; any sub-millisecond write turns it into a hard failure rather than a flake. Rounding to milliseconds fixes it. The file also leaves its rows behind, so it gets an afterAll.

A test for the regressing sequence is going in first, red, before the fix.

What I verified, and found correct

• bun x tsc --noEmit exits 0. bun run lint:complexity exits 0 at "42 findings matched the grandfathered baseline", no new entries.
• The DB behavior test is green against a local Postgres 16: 11 pass, 0 fail, 77 assertions. Reverse-applying only the production side of 11c369f2b0b gives 5 pass / 6 fail, and the six failures are exactly the sequences the fix targets, on both pools. It is a genuine regression test, not a rubber stamp.
• Both consumers are covered, not just the column: the Claude branch asserts selection.capacityRetryAfterSeconds, the native branch asserts nextCapacityAvailableAt.
• Parameterisation is clean. The rendered SQL binds the failure code and every timestamp; nothing is interpolated. The one raw fragment, account."last_failure_code" in nextCapacityAvailableAt, is a compile-time literal.
• The nextCapacityAvailableAt rewrite is exactly equivalent to the is distinct from 'invalid_credential' it replaces. IS NOT DISTINCT FROM never yields NULL, so the enclosing NOT (...) is safe for rows with no recorded reason, which a plain <> would have dropped.
• Concurrency is unchanged: still one statement per call, nothing reads before writing. Raced two updates against the row and the deadline and the reason always came from the same row version.
• The native half of the fix is correct and is a real bug fix: the old code wrote lastFailureCode unconditionally, so rate_limited(5h) followed by upstream_unavailable(20s) left the reason describing the losing write.
• The native non-transient branch is unreachable today. The only producer of invalid_credential is claudeProxy.ts:861, which writes only to the Claude table. That is fine as defence, and it means the blocking finding above is Claude-only right now.

Also

The rationale comment explaining why this write is a single statement was lost when both call sites moved to the helper. Restored in f568a595834.

Filed as a consequence, not a blocker: #15876. Even with the time bound, the Claude pool still has no operator action that clears a cooldown and its reason after a credential is repaired, and the dashboard renders last_failure_code for Claude accounts unconditionally where the native row gates on state !== "active".

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[teamleaderleo]

Review resolved

New head b20de6becd73b2ddcb90bedee7bc0737408856ce, two commits on top of the reviewed head.

Fixed

• The blocking finding. ce5dd48d60a adds the failing test first, b20de6becd7 adds the fix: a stored non-transient reason now outranks a new transient one only while its own cooldown is still in the future, through AND COALESCE(${cooldownUntil} > now(), false) on the precedence term. Once the deadline has lapsed the account has already been retried, so the fresh reason is the better description and capacityRetryAfter counts the account again. It lands in the shared helper, so both tables get it. The COALESCE is what keeps it NULL-safe: a bare cooldownUntil > now() would yield NULL for a row with no deadline, NOT(NULL) is NULL, the CASE would fall to ELSE, and that row would wrongly keep its old reason.
• nonTransientFailureCodePredicate now returns false for an empty code set instead of rendering nothing and producing NOT ().
• The epoch reads are round(extract(epoch from cooldown_until) * 1000)::bigint, so a sub-millisecond write can no longer turn a toBe into a hard failure.
• The test file cleans up after itself: afterAll deletes both teams' rows before closing the pool.

Verified, by me, at the new head

• bun x tsc --noEmit exits 0.
• bun run lint:complexity exits 0 at "42 findings matched the grandfathered baseline", no new entries.
• The db behavior test is green against a local Postgres 16: 15 pass, 0 fail, 88 assertions.
• Red before green, checked by reverse-applying only the production hunk of b20de6becd7 and leaving both tests in place: 13 pass, 2 fail, and the two failures are exactly claude cooldown replaces an expired non-transient reason and native cooldown replaces an expired non-transient reason. Neither existing sequence changed behaviour, which is the intent: a new non-transient reason still always wins, including when it arrives during a long rate-limit wall.

Left

• The second new test, cooldown keeps a NULL reason on a live longer deadline, passes with and without the fix. It pins pre-existing behaviour rather than the NULL-safety of the COALESCE. The case that would exercise it, a NULL cooldown_until next to a non-null reason, is not reachable through any code path: insert writes both as null and markCooldown always writes both together. The guard is defence, and I left it untested rather than pin an unreachable state.
• The native non-transient branch stays unreachable today. The only producer of invalid_credential is claudeProxy.ts:861, which writes only to the Claude table.
• A Claude upstream account behind a long cooldown has no way back except delete and re-add #15876 stands and is not addressed here. With the time bound, a stale reason can no longer disable capacity-holding forever, but the Claude pool still has no operator action that clears a cooldown and its reason after a credential is repaired, and the dashboard renders last_failure_code for Claude accounts unconditionally where the native row gates on state !== "active". Both are design calls, not part of this fix.

Enabling auto-merge. This is a fix, so it does not go to the team design tracker.

[github-actions]

Merge receipt for b20de6becd: every check was green at merge (21 verified; 20 skipped by policy). Full suite runs on main after merge.