Uh oh!
There was an error while loading. Please reload this page.
Fix non-faithful backport of #2305 to stable - #2344
Conversation
|
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (92 failed)mongodb (14 failed):
redis (10 failed):
turso (68 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.
Check the workflow run for details. |
The #2341 backport added wait-until.ts and runtime-import.test.ts but left static '@vercel/functions' imports in start.ts, step-handler.ts, suspension-handler.ts, and util.ts, causing the test to fail. Convert the remaining call sites to the lazy wait-until module, remove the now-dead waitedUntil from util.ts, and fix Biome formatting in step-handler.ts.
pranaygp
left a comment
There was a problem hiding this comment.
Verified end-to-end — this fixes the stable-wide breakage from the #2305 backport. We independently hit all three issues today and had opened #2347 (resume-hook import) and #2349 (the identical lockfile line) before finding this PR; both are closed in favor of this one.
Local verification on this branch: pnpm install --frozen-lockfile --ignore-scripts exits 0; pnpm --filter "@workflow/core..." build green; and notably runtime-import.test.ts (the no-@vercel/functions-at-module-eval invariant) goes red→green vs origin/stable HEAD. CI confirms the install step now succeeds across all jobs; the remaining E2E community/matrix failures fail in test execution, not setup, and can't be baselined against stable HEAD (which can't install at all).
One sequencing note: #2346 (ack-ordering backport) removes the only waitUntil call in suspension-handler.ts, so after both land, the import this PR adds there becomes unused — flagged on #2346 to handle in its rebase.
| import { getWorkflowQueueName } from './helpers.js'; | ||
| import { Run } from './run.js'; | ||
| import { waitedUntil} from '../util.js'; | ||
| import { waitedUntil, waitUntil } from './wait-until.js'; |
There was a problem hiding this comment.
Non-blocking observation: switching these call sites from the static @vercel/functions import to the lazy wrapper changes waitUntil registration from synchronous to asynchronous — wait-until.ts does void import('@vercel/functions').then(({waitUntil}) => waitUntil(promise)), so on a cold module the platform learns about the background promise only after the dynamic import resolves. If a handler returns before that microtask+module-load completes, the promise may go unregistered. The module is cached after first use so the window is tiny and this matches what main does — just worth knowing this is a (slightly) weaker guarantee than the static import for the start/step-handler/suspension-handler sites that previously had it.
There was a problem hiding this comment.
Agreed, your analysis is correct. wait-until.ts registers the keep-alive via import('@vercel/functions').then(({ waitUntil }) => waitUntil(promise)), so on a cold module the platform only learns about the background promise after the dynamic import resolves — a strictly weaker guarantee than the previous static import at the start/step-handler/suspension-handler sites.
That said, this PR's goal is to make the stable backport faithful to main, and main uses exactly this lazy wrapper, so this is an intentional, pre-existing tradeoff rather than something introduced here. The race window is bounded to the first call per module instance (the dynamic import is cached afterward), and the runtime sites that use it generally await other work after registering, so the response rarely resolves within that microtask. I'd leave it as-is for this PR to keep parity with main; if the cold-start window is a concern, a follow-up could eagerly warm the import (e.g. kick off import('@vercel/functions') at module load) — but that's orthogonal to fixing the backport.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The automated backport of #2305 to
stable(#2341, merged as 9c667e0) was not a faithful port of the original PR. This PR fixes the three issues found after auditing the backport against the original merge commit onmain(4670c4b).Issues fixed
1. Stale pnpm lockfile
The backport left
pnpm-lock.yamlinconsistent with the workspace manifests.2. Build error in
@workflow/coreThe backport removed the
@vercel/functionsimport fromresume-hook.tsbut left a danglingwaitUntilusage, breaking the build. Fixed by importing from the new./wait-until.jsmodule.3. Incomplete lazy
waitUntilconversionThe original PR bundled fix#2340, whose "Avoid loading Vercel functions during runtime import" commit moved all
waitUntilusage in the core runtime to a new lazy-loadingwait-until.tsmodule. The backport added the module and the enforcement test (runtime-import.test.ts, which mocks@vercel/functionsto throw if statically loaded), but only rewired one of five call sites — so the backported test was failing onstable. This PR completes the conversion:packages/core/src/runtime/start.ts— static@vercel/functionsimport removed;waitUntil/waitedUntilnow from./wait-until.jspackages/core/src/runtime/step-handler.ts— same conversion, plus Biome formatting the backport left broken (longgetWorkflowQueueName(workflowName, stepNamespace)lines were failingbiome check)packages/core/src/runtime/suspension-handler.ts— same conversionpackages/core/src/util.ts— removed now-deadwaitedUntiland its@vercel/functionsimport (matchingmain)Audited and confirmed as legitimate stable/main divergence (no action needed)
step-executor.ts— doesn't exist onstable(split out ofstep-handler.tslater onmain); the namespace logic was correctly adapted into stable's monolithic step handlernext/builder-deferred.ts— untouched by the backport, but stable's Next.js routes go throughbase-builder.ts, which did receivecreateWorkflowEntrypointOptionsCode()builders/request-converter.ts— backport-only change, needed for stable's regex-based route rewriting (astro/sveltekit)createStepQueueTriggerin builders constants — correct adaptation for stable's separate step routesmainaside from pre-existing divergences (queue concurrency default, attributes module)Verification
runtime-import.test.tsnow passes (was failing onstable)@workflow/core: 670/670 tests pass; build + typecheck clean@workflow/builders(150) and@workflow/world-local(396) tests passpnpm lintfailures are pre-existing onstable(.github/scripts), unrelated to the backport