feat(responses): a finite send ceiling per root workflow (#4546) - #4612
Conversation
Refs #4546. PRD R05/wp5, first slice. The per-request budget bounds how many times one request reaches upstream. It cannot bound a fan-out: a worker that spawns seven hundred children, each sending exactly once, never violates a per-request cap and still spends the account. That is the second half of the incident. src/lib/workflow-budget.ts tracks the root workflow -- the user-visible task, identified by the parent thread header -- and gives it a finite physical-send ceiling. Every send charged to the request budget is charged to the root as well, including the cross-account move, and a root that has spent its ceiling is refused before dispatch with workflow_budget_exhausted rather than a synthetic upstream error. An exhausted root is never evicted to make room. Dropping a live entry would hand the fan-out a fresh allowance, which is precisely the laundering the ceiling exists to stop, so eviction skips any root with work in flight. The ledger is process-local and in-memory. It bounds a single proxy process honestly and says nothing about a second process sharing the same account pool; that needs a shared durable store and is declared out of scope rather than implied. The concurrency ceiling and the interactive reserve are implemented in the module but not yet wired, because they need a release path tied to the turn lease.
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. |
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
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 (2)
📝 WalkthroughWalkthroughAdds an in-memory workflow budget ledger with admission, send, child, and root-tracking limits. The Responses handler charges transient and account-move sends to the root workflow and returns ChangesWorkflow budget enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ResponsesHandler
participant WorkflowBudgetLedger
participant UpstreamDispatch
ResponsesHandler->>WorkflowBudgetLedger: Check workflow send ceiling
alt ceiling reached
WorkflowBudgetLedger-->>ResponsesHandler: Ceiling reached
ResponsesHandler-->>UpstreamDispatch: Return 429 workflow_budget_exhausted
else ceiling available
WorkflowBudgetLedger-->>ResponsesHandler: Dispatch allowed
ResponsesHandler->>UpstreamDispatch: Send request
UpstreamDispatch-->>ResponsesHandler: Report physical sends
ResponsesHandler->>WorkflowBudgetLedger: Charge workflow sends
end
✨ 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 |
![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: 552b41060d
ℹ️ 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 state = roots.get(rootId); | ||
| if (!state) return; |
There was a problem hiding this comment.
For every request carrying x-codex-parent-thread-id, chargeWorkflowSends immediately returns because roots.get(rootId) is undefined: the only function that creates a state is admitWorkflowTurn, and a repository-wide search shows it has no call sites. Consequently the snapshot remains absent after any number of charges and workflowSendCeilingReached always returns false, so the new 256-send ceiling never rejects a request. Initialize/admit the root before checking or charging it, and add a focused test that reaches the ceiling through the Responses path.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
| const noteTransientSends = (used: number): void => { | ||
| const charged = Math.max(0, used); | ||
| sendBudget.used += charged; | ||
| chargeWorkflowSends(workflowRootId, charged); |
There was a problem hiding this comment.
Even after workflow state creation is wired, this callback is not a common dispatch hook: in the generic routed path, providers without transientRetryPolicy use fetchWithResetRetry without onSendsConsumed (core.ts around lines 7764-7794), while adapters implementing fetchResponse receive only the request budget. Thus ordinary successful requests for those providers never invoke chargeWorkflowSends, and a root can make unlimited physical sends without approaching the workflow ceiling. Charge immediately in the shared pre-dispatch path, or propagate root accounting through every send implementation, with focused coverage for a reset-only provider.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
…) (#4614) Refs #4546. PRD R06/wp5, second slice, completing the module landed in #4612. A fan-out shares the conversation it serves. Without a reserve, a worker burst takes every concurrency slot under its own root and the interactive turn that started it waits behind its own children. runAdmittedHttpTurn now admits each turn against the root workflow as well as the process-wide turn gate: a request that names a parent thread distinct from its own is treated as that fan-out and may not take the reserved slots, while a top-level request is the conversation and may. The refusal is a local queue-capacity answer, not a synthetic upstream error, and the lease is released on both the normal and the throwing path so a failed turn cannot leak a slot.
리뷰 · 우선순위 58 / 80이 PR은 #4546 비용 가드 열차의 R05 첫 슬라이스다. 지금 왜 필요한가. #4609·#4611까지는 한 논리 요청이 upstream에 몇 번 닿는지를 막는다. 그런데 워커가 자식을 일곱백 개 만들고, 자식마다 전송을 딱 한 번만 하면 요청당 상한은 절대 안 깨진다. 계정은 그래도 다 쓴다. 그게 #4546 사고의 다른 절반이다. 이 PR은 그 절반을 막으려고 루트 워크플로(클라이언트가 주는 부모 스레드 = 사용자에게 보이는 한 작업)에 물리 전송 천장( 모양이 어떻게 생겼나. 새 파일 다만 현재 라인 workflow-budget.ts 138-141 - 메인테이너의 판단이 필요한 지점
너의 추천 추가 머지 논의는 필요 없다. 이미 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
Refs #4546. PRD R05, first slice, following #4609 and #4611.
The per-request budget bounds how many times one request reaches upstream. It cannot bound a fan-out: a worker that spawns seven hundred children, each sending exactly once, never violates a per-request cap and still spends the account. That is the second half of the incident, and it needs a ceiling of its own.
src/lib/workflow-budget.tstracks the root workflow — the user-visible task, identified by the parent thread header — and gives it a finite physical-send ceiling. Every send charged to the request budget is charged to the root as well, including the cross-account move, and a root that has spent its ceiling is refused before dispatch withworkflow_budget_exhaustedrather than a synthetic upstream error.An exhausted root is never evicted to make room. Dropping a live entry would hand the fan-out a fresh allowance, which is precisely the laundering the ceiling exists to stop, so eviction skips any root with work in flight.
Scope and honest limits
The ledger is process-local and in-memory. It bounds a single proxy process and says nothing about a second process sharing the same account pool; that needs a shared durable store and is stated as out of scope rather than implied. The concurrency ceiling and the interactive reserve are implemented in the module but not yet wired, because they need a release path tied to the turn lease.
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
devSummary by CodeRabbit
New Features
Bug Fixes
429 workflow_budget_exhaustedresponse.