fix(openai): bound tool-call arguments by bytes, not provider chunking - #3521
Conversation
Every scheduled run of at least one production project has been failing hourly with: veryfront-cloud request failed: invalid successful stream (tool call arguments exceeded 4096 fragments) `appendOpenAIStreamToolArgument` counted every fragment against `MAX_OPENAI_STREAM_TOOL_ARGUMENT_FRAGMENTS`, including the ones that carried bytes. At typical streaming delta sizes of 5-20 UTF-8 bytes, 4096 fragments is roughly 20-80 KB -- about 2-8% of the 1 MiB that `MAX_OPENAI_STREAM_TOOL_ARGUMENT_BYTES` nominally allows. The fragment cap therefore always fired first and the byte budget was unreachable. Fragment count is also the wrong thing to hold a caller to: it is chosen by the provider's tokenizer, not by the size or complexity of the arguments the agent produced, so the same tool call passed or failed depending on how the stream happened to be chunked. Both provider streams' existing tests show what the cap is actually for -- each floods `arguments: ""` / `delta: ""`. Zero-byte fragments never advance the byte budget, so they are the only ones that can arrive without bound. Count only those against the cap. Content is then bounded by bytes, and since every kept fragment is at least one byte, the chunk array cannot outgrow the byte budget either. Behaviour preserved: both existing flood tests still trip the cap unchanged, because they flood empty fragments. Adds direct unit tests for the budget, which had none -- it was only covered indirectly through the two stream parsers. The new test fails before this change with "expected no limit for 16384 small fragments". Fixes veryfront-issue-inbox#432. Note the parent run reports `completed` while the child fails, so this was invisible from run status; that reporting gap is worth a separate look.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7481c7464
ℹ️ 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".
| // unreachable. Fragment count is chosen by the provider's tokenizer rather | ||
| // than by the caller, so the same tool call passed or failed depending on how | ||
| // the stream happened to be chunked. | ||
| if (fragmentBytes === 0) { |
There was a problem hiding this comment.
When an OpenAI-compatible provider emits very small nonempty deltas, this condition bypasses the fragment cap entirely, so both stream parsers can retain up to 1,048,576 separately allocated strings before the byte budget fires. The payload is limited to 1 MiB, but the array and per-string overhead can consume tens of megabytes per concurrent request, whereas the previous implementation retained at most 4,096 chunks. Allow provider-controlled chunking without retaining every fragment separately, for example by coalescing chunks while maintaining the byte limit.
Useful? React with 👍 / 👎.
Fixes veryfront-issue-inbox#432.
Symptom
Every scheduled tick of
agentic-job-submission-processingin production produces a failed child run:The parent run reports
completedwhile the child fails, so it is invisible from run status — the schedule looks healthy from the top.Root cause
appendOpenAIStreamToolArgumentcounted every fragment againstMAX_OPENAI_STREAM_TOOL_ARGUMENT_FRAGMENTS, including the ones carrying bytes:The two budgets are wildly out of proportion. At typical streaming delta sizes of 5–20 UTF-8 bytes, 4096 fragments is roughly 20–80 KB — about 2–8% of the 1 MiB that
MAX_OPENAI_STREAM_TOOL_ARGUMENT_BYTESnominally allows. The fragment cap therefore always fired first, leaving the byte limit effectively unreachable.Fragment count is also the wrong thing to hold a caller to: it is chosen by the provider's tokenizer, not by the size or complexity of the arguments the agent produced. The same logical tool call passed or failed depending on how OpenAI happened to chunk it.
Fix
I did not guess at the cap's intent — the code's own tests state it. Both provider streams exercise it by flooding
arguments: ""/delta: "". Zero-byte fragments never advance the byte budget, so they are the only ones that can arrive without bound, and they are what the cap is for.So only zero-byte fragments count against it. Content is bounded by bytes, and since every kept fragment is at least one byte, the chunk array cannot outgrow the byte budget either.
Behaviour preserved
Both existing flood tests still trip the cap, unchanged — they flood empty fragments. The guard's real purpose is intact; only the false positive on ordinary traffic is gone.
Tests
Adds direct unit tests for the budget, which had none — it was covered only indirectly through the two stream parsers, which is precisely why a cap that rejected ordinary traffic went unnoticed.
The new test fails before this change with:
Also pinned: empty-fragment floods still rejected, the byte budget still enforced, and multi-byte characters counted by UTF-8 length rather than code units.
Verification
10/10 tests pass in
extensions/ext-llm-openai. lint, fmt,deno checkclean.The 5 failures in
src/provideron a full run are pre-existing and unrelated — I stashed this change and reproduced them identically on a clean tree (model-registry,veryfront-cloud/provider; they look network-dependent).Follow-up worth separate triage
A child run can fail while its parent reports
completed. That is why this ran hourly for as long as it did without surfacing.