[world-local] Fix per-step AbortSignal latency and O(world) chunk polling#2807
Conversation
…ling Two companion fixes for local-world stream performance: - core: the inline step executor now tears down abort-stream readers after user code (like the non-inline path already did). A serialized AbortSignal opened a real-time reader whose read() never settled, so every signal-bearing step lost the 500ms ops-settle race, reported hasPendingOps, and took a full queue round-trip instead of running inline (#2795). setupAbortStreamReader now cancels (not just releases) the reader so a polling world doesn't leak a tail reader per step. - world-local: shard stream chunks into a directory per stream so a tail reader's poll lists only that stream's chunks instead of the whole world's on every 100ms tick (#2797), and reliably release emitter listeners + poll timer when a reader is cancelled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 1e3af9d The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 |
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
|
No benchmark result files found in benchmark-results |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
The previous change awaited reader.cancel() before propagating a received abort packet. On a service-backed world (world-vercel) cancel() does a network round-trip that can block, so awaiting it delayed — and when it stalled, dropped — real-time abort delivery to the in-flight step (8 AbortController E2E tests timed out on both world-vercel prod lanes). Now the abort is propagated first via a synchronous releaseLock(), and the underlying stream is only cancelled on the no-abort teardown path, fire-and-forget so it can't block the ops-settle window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Verified the latency claim empirically and reviewed the mechanism — details below, plus two findings as inline comments.
What I checked:
- A/B on the new regression workload (5 sequential signal-bearing steps, embedded local world): with this branch, 1 flow invocation, 511ms; with only
step-executor.tsreverted tomain(everything else identical, rebuilt), 6 invocations, 3377ms — ~575ms/step, matching the PR's ~521ms/step claim. - Mechanism: confirmed
setupAbortStreamReaderpushes a never-settlingread()intoops, the inline ops-flush race times out at 500ms →hasPendingOps→ queue continuation per step;cancelAbortReaderssettles it viareaderCancel, and the newexecuteStepcall is identical to whatstep-handleralready did. - Streamer teardown:
teardown()is wired synchronously before the firstawaitinstart(), and thestreamClosedre-check before arming the poll closes the cancel-during-disk-read race (the check→setIntervalwindow is synchronous). - Suites locally:
@workflow/world-local436/436, corestep-handler/serialization234/234, new inline-execution test passes. - CI: the 2 failing checks are the known-flaky
nextjs-webpack canaryE2E lane + its aggregator, unrelated to this diff.
| * world). A tail reader polling for new chunks would otherwise `readdir` the | ||
| * entire global chunks directory every 100ms — see vercel/workflow#2797. | ||
| */ | ||
| function chunkDirForStream(chunksBaseDir: string, name: string): string { |
There was a problem hiding this comment.
The tag-scoped cleanup() in packages/world-local/src/index.ts (~L168-177, not in this diff so commenting here) still lists the flat streams/chunks dir via listTaggedFilesByExtension, which is a non-recursive readdir (fs.ts ~L263). After this sharding, that directory contains only per-stream subdirectories, so the .{tag}.bin filter matches nothing and tagged chunk deletion silently no-ops — the vitest plugin's per-tag cleanup now leaks chunk files across test sessions. Suggest iterating the per-stream subdirs there (and pruning emptied ones).
There was a problem hiding this comment.
(AI) Good catch — fixed in dec64b3. clear() now iterates each per-stream subdirectory under streams/chunks/ and deletes the .{tag}.bin files within, instead of listing the (now subdirectory-only) top-level chunks dir. Added a regression test in tag.test.ts ("should clear tagged stream chunks in the sharded layout") that writes tagged chunks and asserts clear() removes them.
| '@workflow/world-local': patch | ||
| --- | ||
|
|
||
| Shard stream chunks into a directory per stream so a tail reader's poll no longer lists every chunk in the world on each tick, and reliably release its emitter listeners and poll timer when the reader is cancelled. |
There was a problem hiding this comment.
There's no fallback for the legacy flat layout: chunks written by an older version (streams/chunks/<name>-<chunkId>.bin) become invisible to the new reader, so a run in flight across the upgrade would hang waiting on a stream it can never see, and old runs' streams won't show in workflow web. Probably an acceptable tradeoff for local dev data, but worth a sentence here (e.g. "existing .workflow-data stream chunks from older versions won't be read") — and note the old flat files are never cleaned up.
There was a problem hiding this comment.
(AI) Added a note to the changeset in dec64b3 spelling out the tradeoff: chunks are now under streams/chunks/<streamName>/, chunk files written by an older version to the flat layout are not read back, and stale flat files are left in place rather than cleaned up.
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
The two fixes are well-designed and well-tested — I verified the core reasoning and ran all the suites (world-local 436, core 1363, world-testing 11, all passing, including the new invocation-count regression test). But the chunk-layout change misses one consumer of the old flat layout, which I think should be fixed in this PR since it silently breaks an existing invariant:
Blocking: the tagged-world reset still lists the flat chunks directory (packages/world-local/src/index.ts:169). The cleanup does listTaggedFilesByExtension(chunksDir, tag, '.bin') against streams/chunks/ and unlinks matches at the top level. Post-sharding, that directory contains only per-stream subdirectories, so the readdir returns no matching files and the "Delete tagged stream chunks" step becomes a silent no-op — every tagged world reset now leaks all of its chunk data. Since vitest worker tags (e.g. vitest-0) are reused across runs against a persistent data dir, this accumulates indefinitely — which is ironic given #2797 is specifically about pathologies from chunk accumulation. The fix needs to walk the per-stream subdirectories (and ideally remove emptied stream dirs). A regression test asserting reset actually removes tagged chunks under the sharded layout would lock it in.
Everything else looks good — details of what I verified:
- #2795 fix:
executeStep's capture/cleanup/rethrow brings the inline path to parity with the handler path's existing pattern, and the follow-up commit correctly covers the throwing-step leak (per-attempt reader leak on retries). The new step-handler tests cover both success and failure paths. - Delivery-vs-teardown ordering in
setupAbortStreamReader: releasing (sync) on the abort-arrival path socontroller.abort()isn't gated on a potentially-network-boundcancel(), while cancelling (fire-and-forget) on the no-abort path to tear down polling-world tail readers — both directions are reasoned correctly, and the no-abort path is the one that matters for the leak. - Sharding: write/read/poll/getInfo/get all consistently moved to
streams/chunks/<name>/;assertSafeEntityIdretained on the name before it becomes a path segment; absent dirs read as empty via the pre-existing ENOENT handling (so no-chunk abort streams stay cheap). I checked for other consumers of the on-disk layout — the CLI and web read streams through the world API, so the reset path above is the only miss. - Reader teardown hardening: the hoisted
streamClosed+ unconditionalteardown()correctly closes the cancel-during-start()races, including the orphaned-interval case (the re-check sits synchronously before the interval is armed, so there's no interleave window). - world-testing regression test asserting
invocations === 1for a 5-step signal-bearing workflow is exactly the right level to pin this — it fails as N+1 without the fix.
Non-blocking notes:
- No back-compat for pre-upgrade chunks: existing flat-layout chunk files become invisible to sharded readers. For a dev-oriented world that's probably acceptable, but a run suspended pre-upgrade with an in-flight stream will stall or miss data after upgrading. Worth a sentence in the changeset advising a data-dir reset (or, if cheap, a legacy fallback in
listChunkFilesForStreamwhen the sharded dir is absent). - The 2 failing Benchmark Postgres CI lanes are 1h-timeout cancellations, and the same benchmark workflow shows cancelled runs on
mainitself and unrelated branches in the same window — baseline infra instability, not this PR (same lanes are also merely pending on #2808).
Address #2807 review: the tag-scoped clear() listed only the top-level streams/chunks directory, which after per-stream sharding holds only subdirectories — so the .{tag}.bin filter matched nothing and the vitest plugin's per-tag cleanup silently leaked chunk files across sessions. Iterate each per-stream directory instead. Also note the legacy flat-layout tradeoff in the changeset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address #2808 review: - The claimant loop now force-deletes a claim file that persists but never parses (after a few observations) so a corrupt/orphan claim can't block its token forever — the releaser correctly leaves such files alone, so nothing else reaped them. A live hook's claim is still rebuilt from the event log, so reaping a corrupt claim can't steal a token from a live hook. - hook_created now builds the claim path via hookTokenClaimPath() instead of inline, so the layout can't drift. - Drop the 20-round resume-vs-disposal soak test: both orderings are valid so it passed even with the fix reverted (a soak, not a regression guard). The deterministic mid-teardown test remains the real guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
(AI) Addressed review feedback + related #2808 feedback (merged `main` in first): #2807 review
Related #2808 feedback (addressed here)
|
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Re-reviewed after the review-feedback commits. All previous comments are addressed — approving.
Follow-up verification:
- Tag-cleanup gap (my comment #1): fixed in dec64b3 exactly as suggested —
clear()now iterates the per-stream subdirectories and deletes.{tag}.binfiles within each (ENOENT-tolerant), with a regression test intag.test.ts. Verified the code and ran it. - Legacy-layout note (my comment #2): changeset now documents the flat-layout tradeoff (old chunks not read back, stale files left in place).
- Bonus: bc1e567 also picks up the #2808 review observations that apply here post-merge — unparseable-claim reaping in the claimant loop (with the live-hook protection via event-log rebuild, both covered by new tests),
hookTokenClaimPath()unification, and dropping the 20-round soak test that passed even with the fix reverted. The reaping's 3-observation threshold plus rebuild-from-log makes stealing a live token effectively impossible; the residual read→delete window is the same accepted TOCTOU class as the rest of the module. - Suites on the merged branch:
@workflow/world-local441/441, core runtime/serialization 234/234, and the #2795 inline regression test (1 flow invocation for signal-bearing steps) passes.
CI failures are all pre-existing/infra, not this PR:
nextjs-webpack canary+E2E Required Check: the known-flaky lane (failing on unrelated PRs too).E2E Vercel Prod (nextjs-webpack): singlewebhookWorkflow120s timeout (149/153 passed, including all 28 AbortController + 9 streaming tests); main concurrently had six Vercel Prod lanes failing on unrelated tests (e.g.instanceMethodStepWorkflowon nuxt), so this is prod-infra instability tonight.Vitest Plugin Tests: the documented pre-existinghook-token-reusefast-handoff flake from #2779. Since bc1e567 touched the claim loop after the PR body's measurement, I re-measured locally on this branch: 12/12 pass — no regression signal (the changed path only fires on null claim reads; the handoff race reads valid claims).
The "next run can claim the token right after the previous run disposed it" test raced ~10% because it started the next claimant immediately after resumeHook, which only enqueues the previous run's continuation — it does not wait for that run to process the resume, dispose the hook, and complete. The next run then legitimately observed the previous run still holding the token and returned a (correct) conflict, failing the assertion. Per #2778's guarantee ("disposes AND completes"), drive each run to completion before the next reuses the token. Still catches the regression (a lingering post-completion claim resolves returnValue with a conflict value and fails the race), and the reclaim-lingering-claim path stays covered by the storage tests. Verified 50/50 green (previously ~2/20 failed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TooTallNate
left a comment
TooTallNate
left a comment
There was a problem hiding this comment.
Re-reviewed after the update — the blocking item is fixed exactly as requested, and the two follow-on commits are solid additions. Approving.
Blocking item resolved: the tag-scoped clear() now iterates per-stream subdirectories, with a regression test that writes tagged chunks into two stream dirs and asserts both are actually emptied by clear() — precisely the guard that would have caught the original miss. The changeset also now documents the legacy flat-layout tradeoff, which covers my earlier back-compat note.
On the two additional commits:
- Unparseable-claim reaping (
bc1e5675e): this closes a real gap — and corrects a statement in my #2808 review, where I said unreadable debris "still gets reaped by the claimant-side force-release": it didn't, since that path needs a parsed claim to evaluate releasability, so a corrupt claim file blocked its token permanently with nothing left to reap it. The reap-then-rebuild design is the right shape: it's safe to delete an unparseable file at the canonical path precisely becausewriteExclusivepublishes atomically (a live claim is always whole JSON) and because the follow-up rebuild is event-log-driven — a live hook's claim gets regenerated and the duplicate now conflicts with the realconflictingRunId, which the new test pins down nicely. One theoretical corner: after 3 vanished-claim observations, thedeleteJSONcould unlink a fresh foreign claim written in the read→delete window; reaching it requires three consecutive cross-process claim/release interleavings plus a mid-window claim, so it's practically unreachable — but a cheap raw-read-and-reparse immediately before the delete (skip if it now parses) would shrink it to the same irreducible residual as the guarded-release path. Non-blocking. - Test de-flake (
8611a690a): the reasoning is correct —resumeHookonly enqueues the continuation, so the old test asserted a stronger guarantee than #2778 makes ("disposes AND completes"); the observed conflicts were legitimate. The rewritten test still fails on the actual regression (a lingering post-completion claim resolvesreturnValuewith a conflict), and the dropped soak test's justification (passed with the fix reverted → not a guard) is documented in the commit.
Verified locally: world-local 441/441 (including the new sharded-clear and corrupt-claim tests), core 1363/1363, world-testing 11/11.
CI: all 9 benchmark lanes show as failed, but every one is a 60/90-minute hard-timeout cancellation, and the Local lanes were killed during "Install dependencies" — they never executed any PR code. That's runner-infra congestion (consistent with the benchmark-workflow cancellations visible on main and unrelated branches since yesterday), not a regression signal.
One small ask, fine as a fast-follow: the claim-reap fix is user-visible world-local behavior but rides on this PR's chunk-sharding changeset; a one-line @workflow/world-local changeset entry for it would keep the changelog accurate.
|
Backport to To resolve manually, push a backport branch and open a PR against git fetch origin stable
git checkout -b backport/pr-2807-to-stable origin/stable
git cherry-pick -S e7e5a0e56d10778554b0ea23d0d66ff9feb66bd9 # -S signs the commit
# Fix conflicts, then:
git add -A
git cherry-pick --continue
git push -u origin backport/pr-2807-to-stable
gh pr create --base stable --head backport/pr-2807-to-stable \
--title "Backport #2807: <original PR title>" \
--body "Manual backport of #2807 (cherry-pick e7e5a0e56d10) to \`stable\`." |
3 participants
Fixes two companion issues about local-world stream performance. Closes #2795. Closes #2797.
#2795 — AbortSignal in a step adds ~0.5s fixed latency per step
Passing an
AbortSignalinto a step's arguments cost a flat ~500ms per invocation onworld-local(a ~7× slowdown for a trivial step).Root cause. Reviving a (non-aborted)
AbortSignalon the step side opens a real-time abort-stream reader for the step's duration and pushes itsread()into the step'sops. The inline step executor (executeStep) never cancelled that reader after user code — so theread()(on a stream that has no chunks and never will) never settled, the post-step ops-settle check always lost its 500ms race, the step reportedhasPendingOps, and the inline loop queued a continuation. Result: one full queue round-trip per signal-bearing step instead of running inline.Fix.
executeStepnow callscancelAbortReaders(...)after user code, matching what the non-inlinestep-handlerpath already did. The reader settles immediately and the step runs inline again.setupAbortStreamReadernow cancels (rather than only releasing the lock on) the reader, so a polling world tears down its tail reader instead of leaking one per step.Measured on the world-testing embedded harness (20 sequential trivial steps): +521ms/step → −4ms/step, and flow invocations for a 10-step signal workflow 11 → 1.
#2797 — tail readers poll with a full readdir of the global chunks dir
Every open
world-localtail reader re-listed the entire world-globalstreams/chunks/directory every 100ms (O(total chunks in the world) per tick per reader), then prefix-filtered. Combined with the leaked readers from #2795, a long single-session workload degraded superlinearly.Fix.
streams/chunks/<streamName>/<chunkId>.bin), so a poll lists only that stream's chunks (O(stream) instead of O(world)). An abort stream that never receives a chunk reads an empty/absent directory.cancel()reliably removes the emitter listeners and clears the poll timer even if it racesstart().Testing
@workflow/world-testinginline-execution suite: a signal-bearing sequential workflow completes in exactly 1 flow invocation.@workflow/world-localstreamer tests: reader teardown releases listeners + timer on cancel; chunk listing is scoped to the stream (unaffected by thousands of unrelated chunks).@workflow/world-local(434) and@workflow/coreruntime/serialization suites pass.AbortController(28) and streaming (9) E2E tests pass against a localnextjs-turbopackapp — confirming real-time abort propagation and cross-process stream reads still work with the new teardown and sharded layout.Relationship to #2779 and the turn-cancellation investigation
This PR comes out of the same investigation as #2779 ("Fix hook token reuse after dispose()", closes #2777 / #2778) — debugging correctness and performance of turn cancellation on
world-local(a long single-session loop that creates a durableAbortControllerper turn and threads its signal into steps). That investigation surfaced several independent facets, tracked as #2777, #2778, #2780, #2781, #2795, #2797:AbortControllerregisters a system hook per controller, and reusing/disposing that token across turns conflicted (Recreating a hook token after dispose() in the same run self-conflicts: suspension processing validates the new hook_created before the pending hook_disposed persists #2777 same-run, Hook token claim is not released in order with hook_disposed: a fast next-run claimant gets HookConflictError against an already-disposed hook #2778 cross-run handoff).So #2779 and #2807 are complementary fixes to the same durable-cancellation pattern: #2779 makes the hook side reusable, this PR makes the signal/stream side cheap. Neither depends on the other to land.