feat(routing): bound recovery with a half-open probe lease and honour Retry-After in full (#4546) - #4626
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change revises ChangesRetry-After deadline handling
Transient probe control
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a89f0470
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **A transient hold is probed half-open, never opened all at once.** While a bound account is | ||
| held for a 5xx streak, one in-flight probe may test it and every other request keeps the | ||
| remembered detour; the lease carries a deadline and a generation so a late answer from a | ||
| probe that already lost cannot overwrite a newer binding or failure state. When every | ||
| candidate is held the caller gets a typed withheld outcome, not a send. Recovery dispatches | ||
| (retries and probes, never a new request's initial send) sit under a pool-wide ratio ceiling | ||
| measured over a sliding window (`src/routing/probe-lease.ts`). |
There was a problem hiding this comment.
A repo-wide search at this commit finds no production import of src/routing/probe-lease.ts; src/codex/routing.ts still returns either the detour or the failing bound account, and no send path records or limits recovery dispatches. Consequently none of the probe, withheld, or pool-wide ceiling behavior described here can execute. Remove this present-tense contract until the routing/send integration lands, or wire the module into those paths in this change.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| kind: "withheld", | ||
| boundAccountId: input.boundAccountId, | ||
| ...(input.detourAccountId !== undefined ? { detourAccountId: input.detourAccountId } : {}), | ||
| retryAt: nextProbeAt(input.boundAccountId, now, input.minProbeIntervalMs), |
There was a problem hiding this comment.
When probe eligibility succeeds but tryPermitProbeDispatch refuses because the pool allowance is exhausted, this computes retryAt solely from the account lease state. For a fresh account it returns now, even though another attempt at that time will be refused again; a caller honoring this field can therefore immediately loop while the recovery budget remains exhausted. Include the limiter's next admission time in retryAt, or otherwise return a conservative future retry time after a backpressure refusal.
Useful? React with 👍 / 👎.
| if (!state || state.leaseId !== lease.leaseId || state.generation !== lease.generation) { | ||
| return "stale"; | ||
| } | ||
| if (now > lease.expiresAt) return "expired"; |
There was a problem hiding this comment.
If a probe settles at exactly expiresAt, liveLease already considers the lease dead because it requires leaseExpiresAt > now, but this check still applies the outcome because it only rejects now > expiresAt. Since timestamps have millisecond precision, a completion on that exact tick can be accepted after the lease has become eligible for replacement. Use now >= lease.expiresAt so settlement and acquisition agree on the deadline boundary.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 68 / 80설명 이 PR(#4626, 새 파일 두 번째 축은 68점 이유: #4616의 정직한 후속이고 계약·fence·withheld가 분명하다. 다만 미배선·Retry-After 의미 변화(#4616 ceiling clamp 제거)·스택 의존(wpc→wpe→wpf) 때문에 70대 초반으로는 올리지 않았다. 라인 settleTransientProbe / probe-lease.ts - leaseId·generation 불일치면 stale, 시간 초과면 expired. applied일 때만 lease를 비운다. expired 경로에서 lease 슬롯을 누가 치우는지는 호출부/만료 재획득 경로에 달려 있어, 배선 시 ‘expired 후 liveLease가 남아 interval만 막는’ 장면을 테스트로 고정하는 편이 좋다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 37a89f047043c13767043a925e1157ddda32b507. The half-open lease and full Retry-After direction are sound, but two deterministic timing contracts need correction before the routing layer consumes them.
-
A probe refused only by pool backpressure can return
kind:"withheld", retryAt: now. With a fresh account state,nextProbeAt()knows nothing about the limiter and returns the current timestamp. A caller honoring that value can immediately retry, repeatedly hit the limiter and form a local busy loop. The limiter needs to expose (or the resolver must conservatively compute) the next meaningful permit time, and the zero-allowance/no-detour test should assertretryAt > nowand eventual progress after the window. -
Lease expiry uses inconsistent boundaries.
liveLease()considers the lease dead atnow === leaseExpiresAt, butsettleTransientProbe()expires it only whennow > lease.expiresAt. At the exact deadline, the old result can be reported asappliedeven though acquisition already considers that lease non-live. Use one predicate (now >= expiresAt) and pin the equality case.
Please also remove or explicitly relocate the now-unused RetryBackoffOptions.retryAfterCeilingMs contract; retryBackoffDelayMs() no longer consults it, while the actual one-minute decision is hard-coded in fetchWithTransientRetry. A caller-visible option that appears to set a deadline but has no effect will create the next mismatch.
Exact-head hosted CI is green, but no current test exercises the two contradicted edges. This PR is also stacked on #4625/#4624, both currently blocked by requested changes.
ef3eb73 to
56cd54a
Compare
37a89f0 to
ba04d72
Compare
56cd54a to
1d5d299
Compare
… Retry-After in full (#4546) Part of the stacked delivery closing the remaining OCX-4546 cost-guard scope. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
…e state (#4546) Review findings on the probe-lease layer: fetchWithTransientRetry ignored the documented retryAfterCeilingMs, probeStates retained every account ever probed, and the lease expiry boundary disagreed between liveLease and settleTransientProbe. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
ba04d72 to
4b0e415
Compare
…later (lidge-jun#4546) [skip ci] The transient-hold resolver and the pool-wide recovery limiter added in lidge-jun#4626 still have no production caller, so lidge-jun#4701 is not closed here. Wiring them turned up a defect in the thing being wired, and that has to be fixed first. A withheld dispatch promises the caller a retry time. It was computed from the probe pacing alone. When the RATIO limiter is what refused, the account usually has no probe state at all -- nothing was ever granted for it -- so nextProbeAt returned now, and the refusal told the caller to try again immediately. A withheld dispatch that busy-loops puts the same load on an already-failing pool as the dispatch it refused, which is the opposite of what the limiter is for. It also violates the Retry-After half of lidge-jun#4701's completion criteria directly. The limiter is the only thing that knows when its own window moves, so it now says: nextRecoveryAt returns now while the allowance is unspent, and otherwise the moment the oldest bucket still inside the window falls out. Every such bucket started after now - windowMs, so the answer is always strictly in the future, and it is a real change point rather than a guessed delay. The withheld result takes the later of that and the probe pacing. The existing zero-allowance test asserted only that the result was withheld, which is why the defect survived the unit suite that was written to cover this module. It now asserts the time as well. Refs lidge-jun#4701
2 participants
Summary
A held account now recovers half-open instead of all at once, and a provider's
Retry-Afteris no longer shortened into an early retry.The probe lease. #4616 kept a thread's binding during a 5xx streak and promotes a healthy detour when the hold expires, but nothing bounded the trial that decides whether the original account is actually back. A soft-avoided account receives no traffic, so the only way to find out was the hold expiring and handing the still-failing account to every pinned thread at once.
src/routing/probe-lease.tsadds a single-holder lease keyed on the health domain: exactly one in-flight probe may test a held account while everyone else keeps the remembered detour, so a failed trial costs the caller nothing. The lease carries a deadline and a generation epoch, and a late answer from a probe that already lost its lease settles as stale and mutates nothing — it cannot overwrite a newer binding or a newer failure state.Withheld is a real answer. When every candidate is held, the caller receives a typed
{ kind: "withheld" }outcome — binding remembered, dispatch refused — instead of sending to an account already known to be failing. The requirement was never only "do not forget the account"; it is "in a state where it must not send, actually do not send".Retry-Afteris a lower bound, in full. This is a deliberate behaviour change.retryBackoffDelayMsclamped an honoured instruction withMath.min(retryAfter, maxDelayMs), and the same-target 429 wait capped at 60s, which retries earlier than the provider said it would accept. The instruction is now preserved; when honouring it would exceed the wait deadline the request ends with the upstream answer and itsRetry-Afterintact rather than retrying early, which is the exhaustion contract #3294/#3606 and040_send_budget.mdalready settled. The local maximum still bounds our own backoff when the provider gave no instruction.Pool-wide backpressure. Per-request caps do not prevent a retry storm when many requests fail at once. A pool-scoped limiter sits above the per-request budget and admits recovery dispatches — retries and probes, never a new request's initial send — only while they stay under a configurable ratio of observed initial sends over a sliding window (default 20% over 10s, with a floor of 3 so a quiet pool can still recover). Its state is exposed for diagnostics.
Stacked on #4625. Roadmap and stack order:
devlog/_plan/260914_cost_guard_stabilization/090_remaining_stack.md.Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The only proof for this unit is hosted CI at the exact final head SHA; this push used--no-verify.New and changed coverage:
tests/routing/probe-lease.test.tspins single-holder admission, deadline and epoch expiry, a stale settle mutating nothing, the withheld outcome, and the backpressure ratio with its floor.tests/lib/upstream-retry.test.tshas its clamping assertion rewritten to the preserved-instruction contract, plus two tests for ending with the upstream answer instead of retrying early. The new file is registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json.Known open: the module is landed tested and not yet called. The transient hold state lives in
src/codex/routing.ts, owned by a later layer of this stack, which is obliged to callresolveHeldAccountDispatchfrom the detour branch and route the withheld outcome to the caller. The backpressure knob is likewise not yet wired to config or to the send sites.Checklist
Summary by CodeRabbit
Bug Fixes
Retry-Afterinstructions are now honored in full instead of being capped by a local maximum.Routing