Paginate writeToStreamMulti to respect server's chunk limit#1626
Merged
VaguelySerious merged 3 commits intomainfrom Apr 6, 2026
Merged
Paginate writeToStreamMulti to respect server's chunk limit#1626VaguelySerious merged 3 commits intomainfrom
VaguelySerious merged 3 commits intomainfrom
Conversation
Prevent 400 errors when users write more than 1000 chunks within a flush interval. The server rejects batches exceeding MAX_CHUNKS_PER_BATCH (1000 chunks). Two safety mechanisms: - scheduleFlush triggers immediate flush when buffer hits the limit - flush splits oversized buffers into sub-batches of MAX_CHUNKS_PER_FLUSH Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
🦋 Changeset detectedLatest commit: 054e325 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests💻 Local Development (1 failed)sveltekit-stable (1 failed):
🌍 Community Worlds (64 failed)mongodb (4 failed):
redis (3 failed):
turso (57 failed):
Details by Category✅ ▲ Vercel Production
❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
TooTallNate
requested changes
Apr 6, 2026
Member
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Would it make more sense to implement this in world-vercel's writeToStreamMulti() function instead?
Move batch-splitting from core into world-vercel where the 1000-chunk server limit actually applies. writeToStreamMulti now sends in pages of MAX_CHUNKS_PER_REQUEST (1000) to prevent 400 errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
VaguelySerious
changed the title
TooTallNate
reviewed
Apr 6, 2026
Member
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Review
Clean, well-scoped fix for a real problem. The pagination logic is correct — sequential await preserves chunk ordering, and chunks.slice(i, i + MAX_CHUNKS_PER_REQUEST) correctly splits into pages. The fix is correctly placed in the transport layer where the server constraint lives.
One thing worth understanding before merging: this changes the atomicity characteristics of writeToStreamMulti for the world-vercel backend. See inline comment.
What looks good
- Correct pagination: Sequential
awaitin the loop preserves ordering.slicecorrectly pages the array. - Good placement: The limit is a
world-verceltransport concern, not a core concern. - Empty-chunks early return (
if (chunks.length === 0) return) is a minor behavioral change from the old code (which would send an empty PUT), but clearly an improvement. - Changeset is properly scoped to
@workflow/world-vercelonly. - Tests cover the key boundary cases (at limit, one over, and multi-page).
Nits (non-blocking)
See inline comments.
| headers: httpConfig.headers, | ||
| } | ||
| ); | ||
| await response.text(); |
Member
There was a problem hiding this comment.
Partial failure concern (non-blocking, worth understanding)
The caller (WorkflowServerWritableStream.flush() in packages/core/src/serialization.ts:475) treats writeToStreamMulti as all-or-nothing: if the promise rejects, the buffer is retained; if it resolves, the buffer is cleared.
With pagination, if fetch throws a network error on page 2 of 3:
- Page 1's chunks are already persisted on the server
- The error propagates, so the caller retains the entire buffer (all 3 pages worth)
- On the next flush attempt, page 1's chunks are re-sent and duplicated
This is a reasonable trade-off vs. the alternative (400 error on all >1000 chunk flushes), and it's a very unlikely scenario (sequential requests to the same endpoint within milliseconds). But it's worth documenting that atomicity is relaxed for large batches.
Separately, a pre-existing concern: none of the write methods (writeToStream, writeToStreamMulti, closeStream) check response.ok. The await response.text() just consumes the body. With pagination, this means a failed page (e.g. server 500) is silently swallowed and subsequent pages still get sent. Adding a guard like:
if (!response.ok) {
const text = await response.text();
throw new Error(`Stream write failed: HTTP ${response.status}: ${text}`);
}would at least fail fast, which would be consistent with readFromStream and listStreamsByRunId (which do check response.ok).
Member
Author
There was a problem hiding this comment.
Added response.ok checks to all three write methods (writeToStream, writeToStreamMulti, closeStream) — now consistent with the read methods. Also added a comment documenting the relaxed atomicity for multi-page batches.
| MAX_CHUNKS_PER_REQUEST, | ||
| } from './streamer.js'; | ||
|
|
||
| vi.mock('./utils.js', () => ({ |
Member
There was a problem hiding this comment.
Nit: This module-level vi.mock now applies to all tests in the file, including the pre-existing encodeMultiChunks tests that don't use getHttpConfig. Not harmful (those tests are pure functions), but it means future tests added to this file inherit the mock silently. Consider scoping it inside the writeToStreamMulti pagination describe block instead.
Member
Author
There was a problem hiding this comment.
Moved the vi.mock call to right above the pagination describe block with a comment explaining that vitest hoists it regardless of placement, so it cannot be truly scoped — but the intent is now clear and the encodeMultiChunks tests are unaffected (pure functions).
| await streamer.writeToStreamMulti?.('s', 'run-1', chunks); | ||
|
|
||
| // 3 requests: MAX, MAX, 5 | ||
| expect(bodies).toHaveLength(3); |
Member
There was a problem hiding this comment.
Nit: This test verifies the correct number of requests (3) but doesn't verify the chunk counts per page. Decoding each body and asserting the chunk counts (1000, 1000, 5) would make this more robust — otherwise the test would pass even if the split was wrong (e.g. 1500, 500, 5).
Member
Author
There was a problem hiding this comment.
Updated the test to decode each request body and assert chunk counts per page: [MAX_CHUNKS_PER_REQUEST, MAX_CHUNKS_PER_REQUEST, 5].
TooTallNate
approved these changes
Apr 6, 2026
Member
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Approving — the pagination logic is correct, the fix is well-scoped, and the non-blocking nits from my earlier review still apply but aren't blockers.
- Add response.ok guards to writeToStream, writeToStreamMulti, and closeStream — consistent with readFromStream/listStreamsByRunId - Document relaxed atomicity for multi-page batches - Move vi.mock next to pagination tests with comment about hoisting - Verify per-page chunk counts (MAX, MAX, 5) not just request count Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MAX_CHUNKS_PER_REQUEST = 1000inworld-vercel, matching the server'sMAX_CHUNKS_PER_BATCHwriteToStreamMultinow paginates into multiple HTTP requests when the chunk count exceeds this limitworld-vercel) where the server constraint applies, rather than coreTest plan
encodeMultiChunkstests pass (13/13)WorkflowServerWritableStreamtests unaffected (18/18)🤖 Generated with Claude Code