Skip to content

fix(world-postgres): page streams.get historical reads instead of materializing the whole stream - #3258

Open
lunareed720 wants to merge 1 commit into
vercel:mainfrom
lunareed720:fix/world-postgres-stream-get-pagination
Open

fix(world-postgres): page streams.get historical reads instead of materializing the whole stream#3258
lunareed720 wants to merge 1 commit into
vercel:mainfrom
lunareed720:fix/world-postgres-stream-get-pagination

Conversation

@lunareed720

Copy link
Copy Markdown

Description

Fixes #3254.

streams.get(runId, name, startIndex) read a stream's history with one unbounded SELECT ... ORDER BY chunk_id, materializing every chunk in memory and discarding the first startIndex rows in JS. On large streams (hundreds of MB / thousands of chunks) the whole stream buffered inside node-pg before the first byte reached the consumer, starving catch-up readers.

This PR makes the historical read pull-paced: reader state is hoisted out of start(); the first page positions itself with a count-bounded OFFSET (skip = min(startIndex, count(*)), remainder stays in the JS offset so a start index past the current tail keeps skipping live-buffered rows); subsequent pages keyset-paginate on chunk_id (WHERE chunk_id > cursor ORDER BY chunk_id LIMIT 64), mirroring the getChunks idioms already in streamer.ts; a negative startIndex resolves via count(*) WHERE eof = false before the first page (equivalent to the old in-memory count because close() writes the EOF marker last in ULID order); a short page ends history and flushes the live NOTIFY buffer; a new cancelled flag ensures a cancelled stream never enqueues from an in-flight page.

Verified-preserved properties (mirrors the list on #3254): live subscription and NOTIFY buffering; ULID-order dedup via lastChunkId (only advanced on processed rows, never on offset-skipped ones); uniform offset skip including the EOF marker row; eofcontroller.close(); byte-exact output for full reads, page-edge/mid-page/tail/past-tail/negative startIndex, and history→live handoff without duplicates or gaps.

How did you test your changes?

New packages/world-postgres/test/streamer.test.ts (same testcontainers harness as test/storage.test.ts): byte-exact 200-chunk read across three page boundaries; startIndex at a page edge, mid-page, at the tail, past the tail, negative, and negative-clamped; history→live handoff (append + close mid-read, no dup/no gap); a start index beyond the current tail applied to later live chunks; cancel mid-history. vitest run for the package: 160 tests pass. tsc --noEmit and biome check clean (warn count unchanged from main). test/spec.test.ts needs the Rust/wasm toolchain to build @workflow/world-testing and was not runnable in my environment; it should be covered by CI.

PR Checklist - Required to merge

  • pnpm changeset was run to create a changelog for this PR (patch bump for @workflow/world-postgres)
  • DCO sign-off (git commit --signoff)

🤖 Generated with Claude Code

…erializing the whole stream

streams.get read a stream's history with one unbounded SELECT, buffering
every chunk inside node-pg before the first byte reached the consumer and
discarding the first startIndex rows in JS. Large streams starved catch-up
readers past their deadlines.

The historical read is now pull-paced in pages of 64: the first page
positions itself with a count-bounded OFFSET (remainder spills to the JS
offset so a start index past the current tail keeps skipping live-buffered
rows), subsequent pages keyset-paginate on chunk_id like getChunks, and a
negative startIndex resolves via count(*) of data rows. Live NOTIFY
buffering, ULID-order dedup, uniform offset skipping (EOF marker row
included), and EOF close semantics are unchanged; a cancelled stream no
longer enqueues from an in-flight page.

Fixes vercel#3254

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: lunareed720 <lunareed720@users.noreply.github.com>
@lunareed720
lunareed720 requested review from a team and ijjk as code owners July 31, 2026 16:42
@changeset-bot

changeset-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f4bb34d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@workflow/world-postgres Patch

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

@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

@lunareed720 is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

@lunareed720

Copy link
Copy Markdown
Author

Ready for review — @vercel/workflow. Happy to adjust page size or split the test file if you prefer a different structure.

@karthikscale3 karthikscale3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Could you address these two issues:

  1. Offset/notification race: a chunk seen by both the historical query and PostgreSQL NOTIFY can decrement startIndex twice, making delivery begin too early. The test currently uses sleeps to avoid this race. Fix: record skipped chunks as observed by advancing the dedup cursor, or maintain separate “observed” and “delivered” cursors. Replace the sleeps with a deterministic race test.

  2. Closed stream beyond EOF: startIndex greater than the data count plus EOF skips every row, so nothing calls controller.close() and the reader waits forever.
    Fix: detect that the stream is closed when positioning the first page and close immediately when the requested index is beyond EOF. Add tests for startIndex = dataCount + 1 and larger values.

'@workflow/world-postgres': patch
---

Page the historical read in `streams.get` instead of materializing the entire stream: the previous implementation selected every chunk of a stream in a single unbounded query and discarded the first `startIndex` rows in JS, buffering the whole stream in memory before the first byte reached the consumer, which could stall catch-up readers on large streams. The read is now pull-paced in pages of 64: the first page positions itself with a count-bounded OFFSET (so a start index past the current tail still skips live-buffered rows), subsequent pages keyset-paginate on `chunk_id` like `getChunks`, and negative start indexes are resolved with a `count(*)` of data rows. Live NOTIFY buffering, ULID-order dedup, offset skipping (including the EOF marker row), and EOF close semantics are unchanged, and a cancelled stream no longer enqueues from an in-flight page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Page the historical read in `streams.get` instead of materializing the entire stream: the previous implementation selected every chunk of a stream in a single unbounded query and discarded the first `startIndex` rows in JS, buffering the whole stream in memory before the first byte reached the consumer, which could stall catch-up readers on large streams. The read is now pull-paced in pages of 64: the first page positions itself with a count-bounded OFFSET (so a start index past the current tail still skips live-buffered rows), subsequent pages keyset-paginate on `chunk_id` like `getChunks`, and negative start indexes are resolved with a `count(*)` of data rows. Live NOTIFY buffering, ULID-order dedup, offset skipping (including the EOF marker row), and EOF close semantics are unchanged, and a cancelled stream no longer enqueues from an in-flight page.
Page `world-postgres` stream history reads to reduce memory usage and improve time-to-first-byte for large streams. Historical chunks are now fetched in batches of 64 while preserving `startIndex` and live-stream handoff behavior.

@VaguelySerious

Copy link
Copy Markdown
Member

@lunareed720 Also, we require commits to be signed. Could you squash+sign the PR and force-push the branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[world-postgres] streams.get(name, startIndex) buffers the entire stream before serving the first byte

3 participants