fix(responses): compact and the Kiro inner retries join the request send budget (#4546) - #4611
Conversation
…end budget (#4546) Refs #4546. PRD R04. Compact held its own allowance. Its normal send took a fresh transient three, the stored-pool 401 replay added one, and the 429 alternate added another -- and the guard meant to make those last two mutually exclusive keys on kind === 'pool', so a main-pool credential left it false and really could reach five. The recursive handoff child then forwarded the options object without a holder and minted its own, so one logical compact could reach ten. It now draws the shared remainder for the ladder and spends base-then-reserve for each single send, and the handoff child inherits the holder explicitly. Kiro was the larger multiplier. It nests a three-round throttle loop over a three-attempt reset ladder that can itself run twice per round, so one adapter entry could be eighteen upstream requests, and the text-fallback rebuild constructed a fresh context that dropped whatever core passed. AdapterFetchContext now carries an optional budget, every physical send inside the reset ladder is admitted against it, and the fallback rebuild carries it forward. The field is optional and unlimited when absent so an adapter unit test that calls the transport context-free keeps its own retry shape. Deliberately still out of scope: Cursor rides IncomingMeta rather than AdapterFetchContext, the compact routed fallback mints a fresh budget, and the generic OAuth hops keep their own per-request failover counters.
github-actions
Bot
added
the
intake: hygiene-blocked
|
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. |
✨ 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 |
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
리뷰 · 우선순위 70 / 80이 PR은 #4546 비용 가드 열차에서 #4609 바로 다음 칸이다. 지금 왜 필요한가. compact는 예전에 자기 허용을 따로 썼다. 정상 전송이 transient 3발을 새로 받고, stored-pool 401 재생이 1발, 429 대체가 또 1발이었다. 게다가 401/429를 서로 막으려던 가드가 고친 모양은 간단하다. 범위 밖으로 남긴 것도 본문에 숨기지 않았다. Cursor는 라인 compact.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 27609b2b86
ℹ️ 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".
| const decision = ctx.sendBudget?.reserveDispatch({ sendClass: "transient", targetKey: url }); | ||
| if (decision && (!decision.allowed || !decision.permit.use())) { | ||
| throw new SendBudgetExhaustedError(url); |
There was a problem hiding this comment.
When the canonical endpoint returns an endpoint-specific failure and the legacy endpoint then returns a transient 429, these URL-based target keys consume the policy's sole target transition (canonical → legacy). The next throttle round starts from the canonical URL again, so reserveDispatch rejects legacy → canonical before the third physical send, and a request that could recover instead fails with SendBudgetExhaustedError. Persist the selected legacy endpoint for subsequent throttle rounds, or otherwise avoid re-entering the rejected canonical endpoint.
AGENTS.md reference: src/AGENTS.md:L17-L19
Useful? React with 👍 / 👎.
| if (!decision.allowed) return Promise.reject(new SendBudgetExhaustedError(safeHostLabel(compactUrl))); | ||
| if (!decision.permit.use()) return Promise.reject(new SendBudgetExhaustedError(safeHostLabel(compactUrl))); |
There was a problem hiding this comment.
When the initial three-send ladder ends in a 401 for a main-pool credential, the refresh replay spends the fourth send and can return 429; the subsequent alternate attempt reaches this refusal after that 429 body has already been cancelled. localDispatchRefusal does not recognize SendBudgetExhaustedError, so the surrounding catch converts the quota response into a misleading 502 upstream_error. Check budget availability before cancelling the reusable response and preserve the 429, or map the refusal to the structured 429 contract used by core.ts.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| /** | ||
| * The logical request's send budget (#4546). Optional and unlimited when absent, so an | ||
| * adapter unit test that calls a transport context-free keeps its own retry shape. An | ||
| * adapter that retries internally must admit EVERY physical send against it: counting one | ||
| * adapter entry as one send is how a nested 3x3 ladder stayed invisible to a request cap. | ||
| */ | ||
| sendBudget?: RequestExecutionBudget; |
There was a problem hiding this comment.
This commit changes physical-send admission in both Kiro's nested retry ladder and native compact's refresh, alternate-account, and recursive-handoff paths, but it adds no tests. Existing Kiro retry tests omit sendBudget, so they continue exercising the explicitly unlimited compatibility path and cannot detect regressions such as legacy-endpoint throttling or compact budget exhaustion; add focused budgeted cases to the existing Kiro and compaction test files.
AGENTS.md reference: src/AGENTS.md:L22-L26
Useful? React with 👍 / 👎.
| // One holder for this logical compact, inherited by the handoff child so a second model | ||
| // does not start over with a fresh four. | ||
| const sendBudget: RequestExecutionBudget = options.sendBudget ?? createRequestExecutionBudget(); | ||
| const compactTargetKey = `${route.providerName}|${route.modelId}|compact`; |
There was a problem hiding this comment.
Introducing a request-wide adapter transport contract and changing Kiro/compact retry ownership modifies shared adapters and server transport behavior, but the commit leaves all mapped structure documents untouched, including structure/runtime.md, structure/transports/responses.md, structure/transports/inventory.md, and structure/adapters/registry.md. Update the applicable documents alongside this source change so the retry and send-budget invariants remain authoritative.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
1 participant
Summary
Refs #4546. PRD R04, following #4609.
Compact held its own allowance. Its normal send took a fresh transient three, the stored-pool 401 replay added one, and the 429 alternate added another — and the guard meant to make those last two mutually exclusive keys on
kind === 'pool', so a main-pool credential left it false and really could reach five. The recursive handoff child then forwarded the options object without a holder and minted its own, so one logical compact could reach ten. It now draws the shared remainder for the ladder and spends base-then-reserve for each single send, and the handoff child inherits the holder explicitly.Kiro was the larger multiplier. It nests a three-round throttle loop over a three-attempt reset ladder that can itself run twice per round, so one adapter entry could be eighteen upstream requests, and the text-fallback rebuild constructed a fresh context that dropped whatever core passed.
AdapterFetchContextnow carries an optional budget, every physical send inside the reset ladder is admitted against it, and the fallback rebuild carries it forward.The field is optional and unlimited when absent, so an adapter unit test that calls a transport context-free keeps its own retry shape.
Deliberately out of scope, recorded rather than hidden
Cursor rides
IncomingMetarather thanAdapterFetchContext; the compact routed fallback still mints a fresh budget; the generic OAuth and Anthropic hops keep their own per-request failover counters.Verification
No local suite, typecheck, install or build was run — this environment forbids them, so hosted CI at the exact final head is the only runtime proof. Local checks: NOT RUN.
Checklist
dev