fix(routing): skip known-exhausted accounts at admission - #57
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
d5f1fdd to
8dfde55
Compare
|
Thanks. The candidate-loop replay safety work is useful, but this stacked branch needs revision before merge:
Please rerun the full gate on the rebased branch. |
c03687c to
15d11bb
Compare
|
Thanks for the revision. The clean rebase, main/sidebar identity handling, reset expiry, both-window evaluation, and production-selector coverage are now in place. One blocker remains: fallback quota is not identity-bound across re-login. Bind fallback cached and persisted quota to the stable fallback identity, then add production-loader regressions for fallback re-login identity isolation and secondary-only admission exhaustion. |
15d11bb to
e8fe6d5
Compare
|
Fixed in
Production-loader regressions added: fallback re-login identity isolation on both the Note for cross-PR consistency: #56 has been converged onto this same entry-based identity mechanism, so the two branches now share a byte-identical identity surface (QuotaManager internals + sidebar stamping) — the eventual merge of both is conflict-free. |
|
Thanks for the revision. The fallback re-login isolation is now wired through persisted quota, push, policy/admission reads, sidebar state, and regression coverage, so the prior stale-identity blocker is resolved. This is now a merge candidate. Because the PR also adds substantial shared-file admission routing, I am putting the integrated behavior through an adversarial review before merging. |
e8fe6d5 to
11264c1
Compare
|
I ran an adversarial pass over the integrated behavior, focused on whether the shared file can deny a request the wire would have served. The last-path safety valve holds up: with every account marked exhausted, 1. An unstamped file row is trusted for an account whose identity is known.
Require agreement when the live identity is known, and treat an unstamped row as absent rather than authoritative. Add a regression with a file row carrying quota but no 2.
Adding Two notes, neither blocking:
Sidebar read latency on the hot path is acceptable as-is, since it is memoized once per request and failures fail open. Worth revisiting only if a hung filesystem shows up in practice, as |
Each process learned an account was exhausted only by spending a request on it, because rate-limit marks live in per-process memory while the shared sidebar file already knows. With several sessions running, every one of them paid its own doomed probe. Admission now consults the shared file alongside memory, skipping an account whose quota is exhausted there and synthesizing a retryable 429 for an exhausted primary. Filtering never removes the last path: if it would leave no candidate, the original order is restored and the wire decides, so a stale or corrupt file cannot brick routing. A file row is trusted only when its identity matches the live caller's. An unstamped row - written by an older build, or caught mid-write - is treated as absent rather than authoritative, since applying it would let one account's exhaustion follow the internal slot onto whoever logs in next. That is the re-login bug already closed on the memory path, and the file must not reintroduce it. Unknown identity on either side still fails open. Blocking is also gated on the request being replayable. Both fallback paths require replay, so a non-replayable request with an exhausted main and a healthy fallback would have been denied outright with no wire check anywhere in it. Freshness now considers both windows. An account whose only fresh window is the secondary was judged on the older primary timestamp. The wire remains the authority only where filtering would strip the last candidate. While any fallback survives on paper, a stale exhaustion does skip main - the tradeoff that buys the saved probes.
11264c1 to
2c772a4
Compare
|
Both blockers fixed in 1. Unstamped row trusted for a known identity. - (fileAccountId === undefined || fileAccountId === currentAccountId) &&
+ fileAccountId === currentAccountId &&The asymmetry you named is the part I had missed: memory fails open on unknown identity while the file was failing closed by trusting its exhaustion. Same input, opposite defaults, and the file's default is the dangerous one because it denies rather than probes. An unstamped row is now treated as absent when the live identity is known; unknown identity on either side still fails open. Regression added: a file row carrying quota but no 2. Both notes folded in. The comment at
On the sidebar read latency: agreed it is fine as-is, memoized per request and failing open. One process note, since it is the kind of thing worth surfacing rather than burying: the fix touched five existing test expectations. I checked the diff specifically for weakened assertions before accepting it — 100 lines added, zero removed, and the updates were inside the new tests rather than relaxations of old ones. Changing existing expectations to make a change pass is the failure mode I would want caught here, so I would rather say that I looked than have you assume it. |
|
Verified both fixes at
One residual, non-blocking: when the live identity is also unknown, Ready to merge from my side. |
|
Thanks — and agreed on the residual, with one clarification worth recording since it is the third time identity handling has been the subtle part of this file. Both-sides-unknown admitting the row is deliberate. The asymmetry you found last round was that memory failed open while the file failed closed on the same signal; that is now aligned for the case that matters, where the live identity is known and the row is not. When neither side has an identity there is genuinely no discriminator, and the two options are: trust the exhaustion and skip a probe that may have been fine, or ignore it and spend a request that may be doomed. Skipping is the cheaper error, and it is self-correcting — the next response stamps an identity and the ambiguity disappears. I will add the comment when that line is next touched rather than pushing a comment-only change to a branch you are ready to merge. Ready on my side too. |
feat(routing): sticky-balanced placement, session reset, and cachekeep sustain Rebased onto the revised #57 so the admission identity rule and the replayability gate survive; freshestQuotaSnapshot keeps both windows' timestamps as the freshness key.
Why
Rate-limit marks are per-process and in-memory. With several concurrent opencode processes and
fallback-firstrouting pinned at an exhausted account, every process independently pays one doomed admission probe (admission-timeusage_limit_reached→ mark → retryable error → reroute) before learning what the machine-global sidebar file already knows: the account is at 100% with a reset days away. The operator sees each discovery as a visible retry/error flash.What
Admission-time candidate selection now consults quota before probing:
primary.checkedAt, then snapshotcheckedAt, then entrycheckedAt). The fresher source is selected — the file wins only when strictly newer, memory wins ties, and an empty in-memory cache (fresh process) defers to a valid file row.isQuotaExhausted(type-safe, fail-open) is applied only to the selected source.fallback-first) gate and the reactive iterator via a shared memoized selection.quota-exhausted,Retry-Afterfrom the account's ownresetsAt) so the reroute happens without the doomed probe — only when a non-exhausted fallback survives.admission skip: exhausted account {accountId, source, resetsAt}.Verification
client-alt, gotwork-alt), fresh-process file exhaustion, newer-exhausted-file vs stale-healthy-memory, exhausted primary reroute — all fail pre-fix; 7 fail-open characterizations proven non-vacuous by reverse-applying the src diff (they pass on reverted source, i.e. they pin non-interference).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Skip probes for accounts already known to be quota‑exhausted by consulting the freshest, identity‑matched quota from memory or the shared sidebar. When main is exhausted and a healthy fallback exists, short‑circuit with a precise 429 (
quota-exhausted) using the earliest future reset.New Features
getSidebarState, once per request) by windowcheckedAt→ snapshotcheckedAt→ entrycheckedAt; require identity match (mainAccountId/per‑fallbackaccountId); unstamped file rows fail open when the live identity is known; drop exhausted candidates; memoize for proactive/reactive paths; log skips; if filtering would remove the last path, keep the original probe order.Retry-Afterfrom the earliest exhausted window; only when the request is replayable and a non‑exhausted fallback survives.QuotaManager(peekFallbackForPolicy(id, identity?));setFallback(..., identity)clears mid‑stream marks on re‑login; machine writes includecheckedAt,mainAccountId, and per‑fallbackaccountId, merge quotas by freshness per account, and keep identity tied to the winning snapshot.Bug Fixes
setSidebarMachineState; a stale write can’t replace fresher on‑disk quota, and identity always follows the snapshot that wins the merge.getSidebarStateread per request.Written for commit 2c772a4. Summary will update on new commits.