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 (12)
📝 WalkthroughWalkthroughThe change adds budgeted physical dispatches for adapter requests. Command-code, Google, and MiMo retry paths use the new wrapper. The tests cover prepaid sends, retry admission, response preservation, account failover, JWT recovery, and send-budget accounting. ChangesAdapter send-budget flow
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Adapter
participant createAdapterPhysicalSend
participant SendBudget
participant Executor
Adapter->>createAdapterPhysicalSend: send request
createAdapterPhysicalSend->>SendBudget: reserve dispatch permit
createAdapterPhysicalSend->>Executor: apply pacing and dispatch
Executor-->>Adapter: return response or retryable failure
Adapter->>createAdapterPhysicalSend: request recovery send
createAdapterPhysicalSend->>SendBudget: admit recovery permit
createAdapterPhysicalSend->>Executor: dispatch recovery request
✨ 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
3/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
리뷰 · 우선순위 73 / 80이 PR은 luvs01의 어댑터 쪽 후속 작업이다. 닫힌 #4621이 남긴 핵심 가치, 즉 아직 요청 send budget을 우회하던 어댑터 소유 재시도 사다리를 핵심 설계는 세 사다리별 변화는 짧다. mimo-free는 401 JWT 재시도를 검증 주장도 구체적이다. 새/변환 회귀 133 pass, unmodified 우선순위 73/80으로 둔 이유: 예산 우회를 막는 방향이 맞고, 헬퍼 한 곳으로 세 사다리를 모은 구조도 읽기 쉬우며, 테스트가 계약(입장 전 준비 금지·prepaid 유지·exhaust 시 실응답)을 행동으로 고정한다. 점수를 더 올리지 않은 이유는 tip rebase 미완, Draft 체크리스트 미완, 그리고 google 루프의 pendingResponse/clone 경로가 유지보수 부담을 조금 키운다는 점이다. Kiro/Cursor 등 이미 physical send를 예약하던 경로는 손대지 않았다는 주장도 본문과 일치해 보인다. 경로/심볼 createAdapterPhysicalSend - 새 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…udget mimo-free's 401 JWT replay, command-code's reasoning-effort repair, and the google-http transient loop each issued bare fetches that never touched ctx.sendBudget, so a request holding only its final recovery permit still dispatched and a refused retry still paid the backoff sleep. Route each physical send through createAdapterPhysicalSend: admission precedes pacing, backoff and superseded-response cancellation, a credential hop's pending permit pays for the first send exactly once, and a refused retry returns the real upstream response instead of a synthetic error. Follow-up to lidge-jun#4621.
The 401 replay moved its drain behind admission so that a budget-refused replay can still return that same response with a readable body. Inside beforeDispatch it ran last, after resetMimoJwtCache and getMimoJwt. getMimoJwt issues its own bootstrap request and rejects on a failed or oversized response. When it did, fetchResponse threw and the 401 body was never released - a leak the pre-change code did not have, because it cancelled first and refreshed second. Draining first WITHIN beforeDispatch keeps both properties: it is still after admission, so a refusal returns the untouched response, and it no longer depends on the refresh succeeding.
22130fb to
31229b2
Compare
|
Post-merge follow-up on this branch:
|
2 participants
Summary
ctx.sendBudgetbefore dispatch.createAdapterPhysicalSend, one ordinal sequence per adapterfetchResponsecall. Admission precedes executor pacing slots, backoff sleeps and superseded-response cancellation, so a refused retry does not pay its retry preparation. A credential hop's pending permit pays for the adapter's first send exactly once through the existing dispatch view; a refused retry returns the real upstream response rather than a synthetic error.mimo-free, moving the drain behind admission also moved it after the JWT refresh, andgetMimoJwtissues its own bootstrap request and can reject — leaving the 401 body unreleased, which the pre-change code did not do. The drain now runs first insidebeforeDispatch: still after admission, so a budget-refused replay still returns that response with a readable body, but no longer dependent on the refresh succeeding.adapterDispatchBudget,pendingHopPermit,permit.assumeCharge()); this change is the remaining value: the adapter dispatch sites that still bypassed the budget entirely. The combo-refund portion of fix(responses): enforce shared send budgets across retries and recovery #4621 is dropped as no longer reproducible on current dev.Status
Merged into
devas squash commit5ea488f7c7a212d17a66fbb1fe94b1ce29ac084aon 2026-09-17. The branch continues as the post-merge follow-up vehicle: it now carries the merge of currentdevplus one review-note commit (the google-http 429 peek clone comment requested in review), which is not part of the merged diff.Verification
Exact source identity
40a103713ecdae2386775946e90cceb8ecaaa597(treee67a2e36f16fa9886ee7a269eebe77af016f8ed9).devtip6d19a07369f218c0a0d104ed23232c0c8c75a4fc(which contains the merged squash5ea488f7c). The merge was clean; no conflict resolution was involved.src/adapters/google-http.tsdocumenting the 429 peekres.clone()cost and failure fallback, per review.What the admission contract is, and where it is enforced
Three properties were checked by reading the source rather than by trusting the ladder shape.
One physical send is charged once.
reserveDispatchbooks the spend at reservation time, not atuse()— deliberately, persrc/lib/request-execution-budget.ts, because deciding and charging separately let two legs read the same remainder and both dispatch.createAdapterPhysicalSendreserves once per call, and the executor it hands the adapter holds adispatchedlatch besidepermit.use(), so a second call on one reservation throwsSendBudgetExhaustedErrorinstead of sending twice.onPhysicalSendis observation only:noteAdapterPhysicalSendrecords an attempt send and never touches the counter, so there is no double count.A send that never happens is never charged.
permit.release()in thefinallyreturns the booking whenever the permit was not used, and is a no-op once settled. Every exit before dispatch refunds: an aborted signal at entry, an abort observed after pacing, an abort observed afterbeforeDispatch, and a throw frombeforeDispatchitself.A refusal stays visible. Each ladder returns the last real upstream response with its status,
Retry-Afterand quota body intact, which is the established exhaustion contract. A refusal with no prior upstream answer propagates instead, andsrc/server/responses/adapter-dispatch.tsanswers it as429withSEND_BUDGET_EXHAUSTED_CODErather than502— which matters, because the Codex client retries5xxand does not retry429.One behaviour change worth naming
The google-http 429 peek now always clones (
res.clone()), where it previously readresdirectly unlessreturnRawErrorswas set. This is required —pendingResponsemay have to be returned later, so the original body has to survive the peek — and it is observationally identical on the quota-exhausted path, becauseformatMessagealready falls back withpayloadText || peek. Before the changenormalizeUpstreamHttpErrorResponsere-read an exhausted body and got"", then usedpeek; after it,payloadTextholds that same text. The post-merge head adds a one-line code comment recording the clone's extra buffered body read and that a failed peek returns""and falls through as a transient retry.Checks
31229b268success, every test, gate, keyring, npm-global, docker and macos leg green40a103713bun run typecheck,bun run structure:check,bun run privacy:scanall pass; 134 tests pass, 0 fail across the 5 touched test files (physical-send, google-vertex-http, mimo-free-provider, command-code-provider, responses-core-modules)6f50d2f1f8, which predates both the merge base and the mimo fixThe red-to-green evidence matters here: the prepaid-hop, refund and exhaustion cases exercise the reservation path, so they fail on a dev tree where these ladders still send without reserving.
The added regression,
a rejected JWT refresh still releases the first 401 body, drives the bootstrap to the oversized-response rejection the file already exercises and asserts the 401 stream was cancelled. It fails on the pre-fix ordering.Remaining gates
src/adapters/for retry ladders still issuing bare fetches outsidectx.sendBudget/createAdapterPhysicalSend: one candidate —src/adapters/cursor/live-models.ts(fetchCursorUsableModelsruns one bounded retry through the raw fetch seam on the discovery/management path, which carries no send budget). Single-shot credential/preflight hops remain bare fetches by design:src/adapters/mimo-free.ts:113(JWT bootstrap insidebeforeDispatchprep),src/adapters/devin/cloud-direct/auth.ts:101(GetUserJwt mint),src/adapters/devin/cloud-direct/catalog.ts:201(catalog preflight),src/adapters/devin/cloud-direct/chat.ts:1210(one send per call, counted by the outer admission).Checklist
src/adapters/is already a claimed ownership area sostructure:checkhas nothing new to bind.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing. Merged head
31229b26849a8d21524ad090d4cabf97d8f57939: cross-platform CI run 35201948739 concludedsuccess. Post-merge head40a103713ecdae2386775946e90cceb8ecaaa597: local typecheck/structure/privacy and 134 touched-file tests pass; fork CI run 35217752681 is in progress.I pushed my PR to the latest dev commit. Branch now merges
devtip6d19a07369f218c0a0d104ed23232c0c8c75a4fc.I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit