Uh oh!
There was an error while loading. Please reload this page.
[codex] Handle lazy stream key request failures - #2257
Merged
Conversation
Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Contributor
🦋 Changeset detectedLatest commit: f5e7c8d 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
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (5 failed)astro (2 failed):
fastify (1 failed):
nextjs-turbopack (1 failed):
nextjs-webpack (1 failed):
📦 Local Production (1 failed)fastify-stable (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
❌ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro 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
There was a problem hiding this comment.
Pull request overview
This PR prevents unobserved (potentially unhandled) run-encryption-key lookups by deferring key resolution in Run.getReadable() until stream data is actually deserialized, and improves Vercel transport diagnostics by surfacing correlation headers in both debug logs and response-backed errors.
Changes:
- Extend
EncryptionKeyParamto allow a lazy resolver function and addresolveEncryptionKey()to consistently resolve/cached keys at first use. - Update stream serialization/deserialization to resolve encryption keys lazily on first chunk processing (avoiding eager key fetch for empty/unused streams).
- Add Vercel response diagnostics (
x-vercel-id,x-vercel-error) toworld-vercelHTTP debug logs and error messages, plus regression tests for both behaviors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/world-vercel/src/utils.ts | Adds response diagnostic header extraction and appends diagnostics to HTTP debug logs and response-backed error messages. |
packages/world-vercel/src/utils.test.ts | Adds a regression test asserting Vercel correlation headers are included in thrown HTTP errors. |
packages/core/src/serialization/encryption.ts | Extends EncryptionKeyParam with a lazy resolver option and introduces resolveEncryptionKey() helper. |
packages/core/src/serialization.ts | Uses resolveEncryptionKey() to defer encryption key resolution until stream work actually happens (first chunk / abort write). |
packages/core/src/runtime/run.ts | Switches Run.getReadable() to pass a lazy key resolver to deserialization to avoid eager run/key lookup. |
packages/core/src/runtime/runs.test.ts | Adds a regression test ensuring empty readable streams do not trigger run/key fetching. |
.changeset/lazy-stream-key-request-diagnostics.md | Documents patch releases for @workflow/core and @workflow/world-vercel reflecting the behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TooTallNate
approved these changes
Jun 8, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Approve — defensible defensive improvement, but the PR body's framing is stronger than the evidence
The code change is sound and worth landing. The framing of "fixes the production 273s hang" deserves more skepticism than the PR body or codex session implies.
What the PR actually does (and does well)
Run.getReadable()no longer eagerly fetches the encryption key. Pre-PR, callinggetReadable()immediately startedworld.runs.get(runId)regardless of whether the stream would be consumed. Post-PR, the key fetch is deferred to the first chunk'stransform()/processFrames()via a() => Promise<CryptoKey>resolver. The plumbing is clean:EncryptionKeyParamgains a function variant; a smallresolveEncryptionKey()helper invokes it.Vercel correlation diagnostics:
x-vercel-idandx-vercel-errorare now included inWorkflowWorldErrormessages for HTTP failures (PARSE_ERROR, SCHEMA_VALIDATION, default error path) and in the debug log line. Useful when triaging production issues that surface viaWorkflowWorldError.Test coverage: A new
Run.getReadabletest assertsworld.runs.getandgetEncryptionKeyForRunare NOT called for an unused stream after a tick. A newmakeRequesttest asserts the new Vercel correlation suffix in error messages.
Where I'd push back on the framing
The PR body says:
For unused or empty readable streams, a rejected lookup was not observed by a consumer and could surface as an unhandled rejection.
This is technically accurate but practically narrow:
- The promise IS observed eventually, in the first chunk's
await cryptoKey. The unhandled-rejection window is the gap betweengetReadable()returning and the first chunk being processed. - For most calling patterns, that window is short (microseconds: the consumer reads chunks immediately).
- The promise is also stored in
this.#encryptionKeyPromise(the memoization cache), which means it has at least one reference to its rejection.
So the "could surface as an unhandled rejection" scenario requires:
getReadable()called- Stream never consumed (no chunks ever processed → no
await cryptoKey) world.runs.get()rejects- No other code observes
#encryptionKeyPromiseeither
The codex session's reasoning that this causes the 273-second Ash hang has a gap I can't follow: the screenshots say workflow-server records HTTP 200, roughly 54 ms later — meaning the GET succeeded. The 273-second timer fires later via AbortSignal.timeout(60_000), which is a per-request timeout. If the request already returned 200, that timeout signal shouldn't reject the already-resolved promise.
The corrected codex analysis (image 2) acknowledges the same — the actual root cause is the Ash lifecycle gap (runtime.run() constructs the event stream and starts draining via ReadableStream.start() while not being tracked by waitUntil). The codex framing softens to "Workflow PR #2257 removes the dangerous eager promise that makes the Ash gap fatal," which is a more defensible claim.
So why land it?
Even setting aside the 273s hang attribution, this PR is a real improvement on its own:
getReadable()should be cheap and side-effect-free. Starting an HTTP request just because someone calledgetReadable()is bad API hygiene. Some callers want to probe/conditionally use the stream; making them pay for aruns.get()they may never need is wrong.- Lazy resolution removes a class of unhandled-rejection footguns even if they're rare in practice. A consumer that creates a readable stream and decides not to consume it shouldn't blow up the process.
- The Vercel correlation diagnostics are genuinely useful for triaging real production issues.
What I verified locally
- Rebased onto current
maincleanly. The PR's diff against current main shows confusing "deletions" ofAPIConfig.dispatcher+getDispatcher(config)arg — those are stale-branch artifacts from #2235 landing on main after this PR was branched. Post-rebase the diff is exactly the 7 files described in the PR body. pnpm install --frozen-lockfile✓pnpm turbo run build --filter @workflow/core --filter @workflow/world-vercel✓- 215 core tests pass (
serialization.test.ts+runtime/runs.test.ts) - 116 world-vercel tests pass
- The new "does not fetch the run encryption key for an empty stream" test correctly exercises the new behavior
Small observation (non-blocker)
Only one new test in the Run.getReadable describe block — the negative case ("doesn't fetch when stream unused"). There's no positive test confirming that consumption DOES still trigger the fetch. Existing serialization.test.ts tests probably cover the resolution path indirectly, but a direct positive test would document the contract:
it('fetches the run encryption key when the stream is consumed',async()=>{// ... call getReadable(), consume a chunk, assert runs.get called exactly once});CI
10 failures, but most look pre-existing on main:
Unit Tests (ubuntu)failing withWorkflow replay diverged 4 times after 3 recovery replays— looks like a flaky issue from the #2208 retry mechanism, also showing up on main runs- Vercel Prod E2E failures (nextjs-turbopack, nextjs-webpack, astro, fastify) — same flakes as recent main runs
- Benchmarks — habitually flaky
Not seeing failures specific to this PR's changes.
Bottom line
Land it for the right reasons (clean API hygiene, removes a real footgun class, useful diagnostics) — not as "the fix for the 273s production hang." The corrected codex analysis acknowledges as much in its concluding paragraph.
Approving.
github-actionsBot
added a commit
that referenced
this pull request
Jun 8, 2026
Signed-off-by: Pranay Prakash <pranay.gp@gmail.com>
Contributor
Backport PR opened against |
pranaygp added a commit
that referenced
this pull request
Jun 8, 2026
* Handle lazy stream key request failures (#2257) Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> * fix(core): remove main-only world import Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> --------- Signed-off-by: Pranay Prakash <pranay.gp@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Pranay Prakash <pranay.gp@gmail.com>
This was referenced Jun 8, 2026
Merged
pranaygp added a commit
that referenced
this pull request
Jun 11, 2026
…-attributes-client * origin/main: (32 commits) feat(core): add optional namespace for queue topic prefix (#2305) [web-shared] Show precise durations in the new trace viewer (#2335) Validate unique workflow step IDs at build time (#2018) Move run attributes into their own detail card (#2327) [core] Forward-port stream reconnect to getReadable level (#2318) [docs] Add "Step executed multiple times" error page (#2310) Fix flickering on the detail panel when navigating the trace viewer (#2325) Version Packages (beta) (#2254) Patch vulnerable package dependencies (#2301) Deprecate DurableAgent and update it with WorkflowAgent in v5 docs (#2285) [codex] Fix partial world-local exclusive writes (#2296) [world-vercel] Validate ref resolve responses before use (#2035) fix: bump devalue to 5.8.1 (#2292) Handle lazy stream key request failures (#2257) fix(docs): declare Nitro auto-import globals for code samples (#2290) [core] Preserve event-log order in hook-vs-sleep replay races (#2171) (#2185) docs: add nitro changelog (#2232) fix(next): always apply turbopack content condition regardless of builder mode (#2253) Update @vercel/queue from 0.2.1 to 0.3.0 (#2255) [nest] Use AST-based CommonJS import rewriting (#2080) ... # Conflicts: # packages/world-local/src/storage/events-storage.ts
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Run.getReadable()encryption-key lookup until serialized stream data is actually readx-vercel-idandx-vercel-errorin response-backedworld-verceltransport errors and HTTP debug logsRoot cause
Run.getReadable()eagerly started resolving the run encryption key. That path callsworld.runs.get()(GET /v2/runs/:id?remoteRefBehavior=resolve) before any stream chunk requires deserialization. For unused or empty readable streams, a rejected lookup was not observed by a consumer and could surface as an unhandled rejection.The Vercel request headers are only available when an HTTP response is received. A local
AbortSignal.timeout()rejection still cannot include response headers because noResponseexists in that path.Validation
pnpm turbo build --filter=@workflow/core... --filter=@workflow/world-vercel...pnpm --dir packages/core exec vitest run src/serialization.test.ts src/runtime/runs.test.tspnpm --dir packages/world-vercel exec vitest run srcgit diff --check HEAD^ HEAD