fix(devin): wait out a stated rate-limit reset and replay the turn - #5041
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
6/12 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
9fde561 to
9a72d2b
Compare
리뷰 · 우선순위 71 / 80이 PR은 Devin/Cognition free-tier가 보내는 두 층입니다. (1) 테스트가 두껍습니다. 경로 src/adapters/devin/cloud-direct/stated-reset-retry.ts - zero-event 게이트 + bound wait. 핵심 안전장치. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Follow-up pushed as The commit changes 7 files: fixes compound/millisecond duration parsing and its Verification: 90 offline core checks passed; the 43 new cases passed through a Node/TypeScript full-module harness with external routing/state/RPC dependencies stubbed; strict typecheck passed for the parser and errors modules. This is not a claim that full Bun/repository checks or live Devin/Codex recovery passed. The newly triggered Cross-platform CI run |
Cognition free-tier caps answer resource_exhausted trailers that name their own recovery delay ("Your limit will reset in 35 seconds"). The delay was never parsed: clients got the synthetic 2s Retry-After and combo cooldowns fell back to the 60s default, so a retry fired straight back into the live cap and the turn died inside a window it could have waited out.
parseRetryAfterFromMessage now reads "reset in N <unit>" phrasing plus minute/hour units on the existing hints, so the stated delay drives the client Retry-After and the cooldown metadata. streamChatEventsWithResetRetry waits the stated delay and replays the identical request, but only while the stream has yielded zero events — after any output a replay could double billable side effects, so those failures keep their terminal path. Waits are bounded by a replay cap and a per-wait ceiling; longer stated windows surface with the parsed delay intact.
Parse complete and compound retry durations without treating milliseconds as minutes. Keep the errors.ts local parser binding as well as its public export. Bound scheduled waits cumulatively within a stated-reset wrapper invocation; preserve explicit refusal errors, cancellation, zero-wait opt-out and the no-output replay boundary. Store explicit Retry-After cooldowns without truncating them to the local ten-minute fallback. Add regression coverage for parser integration, cumulative waits, abort races, post-output refusals and authoritative numeric/HTTP-date cooldowns. Validation in this environment: 90 offline core checks; 43 proposed regression cases executed via a Node/TypeScript module harness with external routing/state/RPC dependencies stubbed; strict typecheck of errors.ts and retry-delay.ts. Full Bun/repository typecheck and live Devin validation remain pending. Long-lived SSE/preflight and cross-layer send/wait budgeting are not implemented by this commit.
`tests/test-layout-tooling.test.ts` failed its membership oracle: three of the new test files resolve through neither the explicit map nor a regex seed, so nothing placed them. `combo-authoritative-reset.test.ts` collides with the `combo-` seeds, which point at `routing`, and the `devin-stated-reset-*` pair has no seed at all. Registered in both `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`, which the oracle compares against each other as well as against the tree. Also rebased onto current `dev`. Co-authored-by: luvs01 <luvs01@users.noreply.github.com>
9144d91 to
6d435fa
Compare
|
Pushed a follow-up commit (
Verified after the rebase: |
|
Re-running the failed job. The Linux shards and every gate are green at this head. |
) * feat(devin): admit every inference send through the shared budget PR #5041 shipped a pre-output replay of a stated rate-limit reset and recorded what it left undone: those inner sends never reached the request-wide send budget, the provider fetch wrapper, or physical-attempt accounting. A turn could therefore make three real inference requests while the shared cap, the pacing slot and the request log each saw one. All three now go through the existing shared physical-send primitive, so a permit is reserved once per actual send, confirmed at the wire boundary, and refunded when admission succeeded but no request followed. Nothing increments a counter directly and no second counter exists. The initial send is not double-charged: the adapter reserves it as ordinal 1, and the request observer already ignores ordinal 1 because the caller records the entry send itself. A replay the budget refuses makes no inference request, records why recovery was withheld, and surfaces the provider's original 429 rather than a local budget error, so the server-stated reset and any outer cooldown behavior survive. Reservation still happens after the stated wait, not before it, so a one-hour wait does not hold a spend booking open for an hour. Catalog and JWT calls are not inference sends and keep the global fetch. * test(responses): admit the Devin row so the counted send actually happens The case asserted one GetChatMessage call and got none, in thirteen milliseconds with no adapter output: the turn was refused before the adapter ran. The devin registry entry declares authKind "oauth", and an omitted authMode inherits it, so the fixture's row demanded an OAuth credential while supplying an apiKey. It now states authMode "key", which is what the working sibling fixture in this file does and what the supplied credential actually is. Both numbers are now asserted together and carry the response status and body in the failure message, so a turn that never reaches the adapter says so instead of presenting as an empty URL list. * test(responses): route the Devin count case through a stored credential The authMode guess was wrong: the case still recorded no send. Devin is an OAuth-kind provider, and the key its adapter uses is injected onto the row from the stored credential, so a config carrying only apiKey never routes and the turn ends before the adapter. The case now seeds the credential the same way the working Devin fixture does, which is the path production takes, and the catalog is keyed to that same value. The assertion is unchanged: one GetChatMessage call and one recorded send, reported together with the response status and body so the next failure is self-describing. * fix(devin): count the first inference send where it is admitted Review found the accounting still describing an intention rather than a send. runTurnAttempt logs the attempt's first send before handing control to the adapter, which is right for a transport whose sends the caller performs. Devin now admits its own sends through the shared budget, so that first send can be refused - and once earlier combo or empty-recovery sends have spent the allowance, the log claimed a request the wire never made. An adapter that reports every physical send now says so, and for those the caller stops pre-logging and the observer counts ordinal 1 at the executor boundary that actually dispatched it. Every other adapter and call site is unchanged, including the ordinal-1 skip they rely on. An attempt-level recovery kind still labels that first send when the adapter supplies none of its own. * test(devin): pin that a refused first send is neither made nor counted The accounting fix needs the case that exposed it: an allowance already spent before this turn starts, which is what an earlier combo fan-out or empty-response recovery leaves behind. Nothing reaches GetChatMessage, nothing is observed as a physical send, and the budget records nothing - where the previous ordering would have logged a send the wire never made. The admitted case beside it still asserts exactly one call, one observation at ordinal 1, and one charged send, so the fix cannot be satisfied by counting less. * test(responses): pin the refused Devin send at the request boundary The direct-adapter case cannot see the defect it was written for. The phantom send was logged by executeResponsesRunTurn before the adapter ran, so only a case that goes through handleResponses with a RequestLogContext can prove the attempt records nothing. This one puts Devin last in a failover combo behind a chat target that spends the allowance first, which is the shape that leaves nothing for Devin's initial send. It asserts no GetChatMessage request and a Devin attempt sendCount of zero, and that the members which did send still account for themselves - so the fix removes a phantom rather than suppressing real counts. The status and body travel in the failure message. The direct-adapter case stays for what it does cover, the executor side, and no longer carries the claim that it pins the outer behavior. * test(responses): force the refusal instead of arranging it through a combo Review traced the arithmetic: a two-target combo has five total sends against a base of four, the first target settles a counted booking leaving three, the transition books the fourth, and Devin's initial send was still admitted at five. The case therefore never reached the denied path, and an optional attempt lookup let an absent attempt satisfy a zero count. The budget is now handed to handleResponses already spent, built by the real factory rather than inferred from combo behaviour. The assertions are the ones that prove a refusal: exactly one attempt, that attempt is Devin's and its sendCount is zero, no GetChatMessage request, zero total sends, and the response carries request_send_budget_exhausted. Status and body travel in the failure message. This row fails against the eager pre-log it was written for: that path recorded the attempt's send before the adapter ran, so sendCount would read one.
2 participants
Summary
Devin/Cognition free-tier caps answer
resource_exhaustedtrailers that name their own recovery delay:Your limit will reset in 35 seconds. That stated delay was never parsed — clients received the synthetic 2sRetry-Afterand combo cooldowns fell back to the 60s default, so a retry fired straight back into the live cap and the turn died inside a window it could have waited out. The Codex client does not retry 429s, so the failure surfaced asstream disconnected before completion.parseRetryAfterFromMessagereadsreset in N <unit>and the existing retry hints. Follow-up9144d91extracts the parser into a leaf module, handles compound durations and milliseconds, rejects misleading unit prefixes, and preserves the local parser binding inerrors.ts.streamChatEventsWithResetRetrywraps the Devin cloud stream: a pre-output 429 with a parseable stated delay can wait and replay the same logical request. Any yielded cloud event prevents replay. Ambiguous transport failures remain terminal.OPENCODEX_DEVIN_STATED_RESET_WAIT_MSup to 1 hour.0disables local waiting. The allowance is not reset for the second replay, and a server delay is never shortened to fit it. This is not a wall-clock deadline on model generation or a request-wide allowance shared across combo/OAuth re-entry.Retry-Aftercooldowns now retain their full duration in combo state rather than becoming selectable after 10 minutes. The local fallback cap and the combo live-wait setting remain unchanged. The separateresetAtfallback is not changed here.Validation of follow-up 9144d91
Performed in the editing environment:
src/lib/errors.tsandsrc/lib/retry-delay.tsonly.Not rerun here: complete repository Bun suites, repository-wide
tsc --noEmit, or a live Devin/Codex long-wait request. The original author's 10/10 stated-reset tests, 21/21 retry-after tests, 65/65 regression cases, and clean full typecheck were reported for the earlier head; they are not claimed as results for this follow-up.Run on the latest head:
Remaining long-wait integration work
This PR must not be described as complete one-hour automatic recovery yet:
waitForCooldownMsremains capped at 10 minutes. Persisting a cooldown does not imply that an HTTP connection can wait for that whole duration.No global timeout was disabled, replay-safety boundary weakened, or automatic paid-provider fallback introduced by the follow-up.
Declaration
Validation
Risk
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests