fix: preserve longest Claude upstream cooldown

[teamleaderleo]

Summary

A concurrent shorter Claude provider cooldown no longer shortens a longer deadline already stored for the account. The Claude Postgres store now keeps cooldown_until authoritative with GREATEST, updates last_failure_code only when the new deadline wins, and bumps updated_at on every write.

This mirrors the native counterpart:

cooldownUntil: sql`GREATEST(COALESCE(${coderouterAccounts.cooldownUntil}, ${cooldownUntilIso}::timestamptz), ${cooldownUntilIso}::timestamptz)`,

In production, Anthropic can return a multi-hour retry-after on a 429. A concurrent upstream_unavailable result has a 20-second cooldown. Before this fix, that 20-second write could erase the multi-hour 429 wall, return the wrong retry-after to clients, and put the account back into rotation while it was still rate limited. The surviving failure code now remains rate_limited until a longer cooldown wins.

The two embedded NUL bytes in claudeUpstream.ts are also replaced with \\x00 escape sequences. This is runtime-identical and makes the file greppable.

Tests

The focused command was red before the fix at commit 1cb1a88a854:

$ bun test tests/coderouter-claude-cooldown-db-behavior.test.ts
(fail) preserves the longest Claude cooldown and failure reason [116.59ms]
Expected: 1790773256872
Received: 1790755276000
0 pass
1 fail
1 expect() calls

The same command is green at commit ebe305872ab:

$ bun test tests/coderouter-claude-cooldown-db-behavior.test.ts
(pass) preserves the longest Claude cooldown and failure reason [101.80ms]
1 pass
0 fail
7 expect() calls

Additional validation:

• bun x tsc --noEmit passed.
• bun run lint:complexity passed with 42 findings matched to the existing baseline.
• bun test tests/coderouter-claude-upstream.test.ts passed, 19 tests.
• bun test tests/coderouter-claude-proxy.test.ts passed, 44 tests.
• python3 scripts/verify-local.py passed all 15 selected checks.
• The focused dbTest drives the exported Claude store against Postgres and checks the native markAccountCooldown path as a control.

Dogfood

There is no fleet dogfood because PR #8029 disabled Vercel branch previews while keeping main deployments. An unmerged web change has no preview URL, and a fleet build would only exercise main. The real-Postgres dbTest is the evidence in its place.

Changelog

• Fixed: Preserve the longest Claude account cooldown and its failure reason during concurrent provider failures.

🤖 Generated with Claude Code

---

Summary by cubic

Fixes Claude cooldown handling so a concurrent shorter cooldown can no longer overwrite a longer deadline already stored for an account.

The Claude store now keeps cooldown_until authoritative using GREATEST, updates last_failure_code only when the new deadline wins, and bumps updated_at on every write. This prevents a 20-second cooldown from erasing a multi-hour Anthropic 429 rate limit, which could put a rate-limited account back into rotation and return the wrong retry-after to clients. Also replaces two embedded NUL bytes in claudeUpstream.ts with \x00 escape sequences (runtime-identical, makes the file greppable).

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

[coderabbitai]

Warning

Review limit reached

Next included review available in 10 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: b016f444-ae6b-4cc8-b31e-1edfaacf7dbd

📥 Commits

Reviewing files that changed from the base of the PR and between 02dac3c and ebe3058.

📒 Files selected for processing (2)

• web/services/coderouter/claudeUpstream.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.}

[github-actions]

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

[teamleaderleo]

Post-merge review. Auto-merge landed this before the review ran, so the findings and their resolution are here instead of in the pre-merge thread. That ordering is my mistake, not the author's, and the follow-up below fixes what it let through.

Verified independently, on this branch's merge commit 4eee1b1, against a local Postgres:

• The test is a genuine regression test, not a tautology. Green as merged (1 pass, 7 assertions). Reverse-applying only the claudeUpstream.ts hunk and re-running the same command makes it fail at the first deadline assertion (Expected: >= 1790775291351, Received: 1790757312000, which is the five-hour wall replaced by the twenty-second one). Restored after.
• GREATEST cannot block a legitimate clear. The only other writer of cooldown_until on this table is the insert default at claudeUpstream.ts:366, which writes null at account creation, so no code path needs to move the deadline earlier.
• markCooldown has exactly one production implementation. The other hit in tests/coderouter-claude-upstream.test.ts:82 is an in-memory fake, so no second store was left unguarded.
• The NUL replacement is runtime-identical in both places (a regex character class and a template literal), and the file is now greppable: grep -c over the tracked tree no longer skips it. This mattered more than it looks. The two NUL bytes are why the bug survived an earlier audit pass: GNU grep classified the file as binary and printed nothing with exit 1, which reads exactly like "no matches".

Finding, blocking, fixed forward in the follow-up: this fixed the deadline and regressed the reason.

last_failure_code is load-bearing, not display-only. capacityRetryAfter at claudeUpstream.ts:462 skips any account whose reason is invalid_credential ("A revoked or unauthorized credential needs a human, not a wait"), and nextCapacityAvailableAt at repository.ts:1369 does the same in SQL, gating whether the proxy holds a request for capacity.

claudeProxy.ts:613 records every reason through this one call, with durations that do not correlate with severity: rate_limited gets Anthropic's retry-after, which can be hours, while invalid_credential gets fifteen minutes. So a 429 followed by a revoked key now keeps the reason rate_limited for five hours, because fifteen minutes loses the GREATEST and the new CASE only writes the reason when the deadline wins. The proxy then holds requests waiting on a credential that needs a human. Before this change the reason was right and the deadline was wrong; now it is the other way round.

The same class of defect predates this PR on the native side: markAccountCooldown guards the deadline with GREATEST but writes lastFailureCode unconditionally, so a twenty-second transient failure erases an invalid_credential reason while the longer deadline stands.

The rule both tables need is one rule: the deadline takes the maximum, a non-transient reason outranks a transient one, and otherwise the winning deadline owns the reason.

Left for the follow-up PR (all four orderings covered as tests, shared precedence helper so the taxonomy stops being a bare string in two files): I will link it here when it is open.

Not dogfooded on the fleet for the structural reason: #8029 disabled Vercel branch previews while keeping main deployments, so an unmerged web change has no preview URL and a fleet build would only exercise main. These dbTests also skip unless CMUX_DB_TEST=1, so CI did not execute them here; the local real-Postgres red and green above is the evidence in their place.

[teamleaderleo]

Correction to my review above: I wrote that CI did not execute these dbTests here. That is wrong, and it understates the evidence behind this merge.

CI does run them. web / web-db-migrations in .github/workflows/ci-web.yml runs bun run test:db:behavior, and web/scripts/run-db-behavior-tests.sh discovers every file under web/tests/ that mentions process.env.CMUX_DB_TEST, runs each one with --max-concurrency=1, and fails the job if any file executes zero tests or skips any test. That job is SUCCESS on this PR, so the new test ran against Postgres in CI, not only on my box. The local run I described is the faster loop, not the only evidence.

What misled me is that a different job, web-database-tests in web-validation.yml, is gated on github.event_name != 'pull_request', so it shows as SKIPPED on every pull request. Its name is the one that looks like the database coverage, and the job that actually provides it is named after migrations.

Nothing else in the review changes. The finding about last_failure_code stands, and the follow-up is in progress.