Skip to content

fix(core): preserve new.target in the deterministic Date override so Date subclasses work - #3372

Merged
TooTallNate merged 5 commits into
vercel:mainfrom
ar-tama:fix/date-subclass-vm
Aug 10, 2026
Merged

fix(core): preserve new.target in the deterministic Date override so Date subclasses work#3372
TooTallNate merged 5 commits into
vercel:mainfrom
ar-tama:fix/date-subclass-vm

Conversation

@ar-tama

@ar-tama ar-tama commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #3371

Problem

createContext() replaces the VM's global Date with a plain function. A plain function has no [[Construct]] behavior that forwards new.target, so when user code does class X extends Date, super() returns a fresh plain Date object which becomes this — the subclass instance loses its identity, methods, and fields.

This silently breaks every library that models a date by subclassing Date, e.g. TZDate from @date-fns/tz (the officially recommended way to do time-zone-aware math with date-fns v4). Inside a workflow body, a TZDate degrades to a plain Date, so every zone-aware getter falls back to the container's time zone. Since the epoch value stays correct, this presents as a silent, production-only off-by-one-day bug. Full analysis in #3371.

Fix

Replace the plain-function override with a class so new.target is preserved:

(g as any).Date = class Date extends Date_ {
  constructor(...args: any[]) {
    if (args.length === 0) {
      super(fixedTimestamp);
    } else {
      // @ts-expect-error - Args is `Date` constructor arguments
      super(...args);
    }
  }
};
g.Date.now = () => fixedTimestamp;

Both of the previous fix-ups become unnecessary, because extends already sets up the whole prototype chain:

  • (g as any).Date.prototype = Date_.prototype — with extends the real chain is in place (and the assignment would be illegal on a class, whose prototype is non-writable).
  • Object.setPrototypeOf(g.Date, Date_)extends already makes the statics (Date.parse, Date.UTC) inherited; only the Date.now override stays as an own property.

Determinism is unchanged: zero-arg construction still returns the fixed timestamp, and Date.now() is still overridden. Covered by the existing determinism tests plus two new ones (subclassing, statics).

One behavioral note: calling Date() without new now throws (classes are not callable), where the old override returned a Date object — itself already a deviation from the spec, which returns a string. If callable Date() needs to keep working, the alternative is a plain function that branches on new.target and uses Reflect.construct(Date_, args, new.target); happy to switch to that if preferred.

Tests

  • should support subclassing \Date` — subclass keeps identity (instanceof`), methods, fields; constructor args are forwarded; zero-arg subclass construction still gets the fixed timestamp
  • should preserve \Date` static methodsDate.parse/Date.UTC` still inherited

All 36 tests in packages/core/src/vm/index.test.ts pass, pnpm typecheck is clean. (The runtime.test.ts failure re-routes a misrouted lazy hook resume also fails on an unmodified checkout of main, so it is unrelated.)

🤖 Generated with Claude Code

@ar-tama
ar-tama requested a review from a team as a code owner August 6, 2026 07:51
@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4c7cd3c

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

This PR includes changesets to release 16 packages
Name Type
@workflow/core Patch
@workflow/builders Patch
@workflow/cli Patch
@workflow/next Patch
@workflow/nitro Patch
@workflow/vitest Patch
@workflow/web-shared Patch
@workflow/web Patch
workflow Patch
@workflow/world-testing Patch
@workflow/astro Patch
@workflow/nest Patch
@workflow/rollup Patch
@workflow/sveltekit Patch
@workflow/vite Patch
@workflow/nuxt 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 Aug 6, 2026

Copy link
Copy Markdown
Contributor

@ar-tama is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

Comment thread packages/core/src/vm/index.ts Outdated

@TooTallNate TooTallNate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at 922287c (base is current main, 0 behind). Build green; all 37 vm tests pass; full core suite green except the one pre-existing failure you correctly identified (more on that below).

Verified:

  • The final Reflect.construct(Date_, args, new.target) design is the right call over the class approach from the PR description: a plain function keeps [[Call]], so bare Date() stays callable — and now returns the spec-correct string instead of the old non-spec object. Construction semantics all check out: a plain function retains [[Construct]], super() from a subclass forwards new.target through to Date_, and the constructed object (with the subclass's prototype and a real Date internal slot) becomes this — identity, methods, and fields preserved, which is exactly what TZDate needs.
  • Determinism is fully preserved: zero-arg construction and zero-arg super() both pin to fixedTimestamp, Date.now stays overridden (and is inherited by subclass statics via the retained setPrototypeOf), and fixedTimestamp is a live let binding that updateTimestamp advances during replay — the closure reads the current value at each construction, same as before.
  • Cross-engine alignment bonus: the QuickJS engine (WORKFLOW_VM=quickjs) never wraps Date at all — it intercepts the WASI clock_time_get syscall, so its Date is fully native and spec-correct. That means QuickJS never had this subclassing bug, and your bare-Date() string change brings node:vm into agreement with QuickJS where the old object-return diverged. No QuickJS-side work needed.
  • One honest behavioral note for the record: a workflow that called bare Date() and used the old non-spec object (e.g. .getTime() on it) would diverge when an in-flight run is replayed across this upgrade. That pattern is vanishingly rare, was already broken outside the sandbox, and the new behavior is what both the spec and the other engine do — the right trade.

Two asks before merge:

  1. DCO check is failing — the commits need sign-off (rebase with --signoff, or follow the DCO bot's remediation instructions).
  2. The changeset text is stale: it describes the override as "now a class that preserves new.target", but the final implementation is a plain function using Reflect.construct (your head commit changed the approach to keep callability). Changesets become release notes — please reword, and consider mentioning that bare Date() now returns the spec time string.

On the unrelated runtime.test.ts failure: confirmed — re-routes a misrouted lazy hook resume with its payload intact fails on an unmodified checkout of main. It's an interaction between two recently merged PRs (#2960's deployment-affinity guard and #3345's lazy-hook fast path); I've reported it with analysis on #3345. Thank you for isolating it precisely instead of ignoring it — that note made the triage immediate.

Clean, well-reasoned fix with exactly the right tests (subclass identity + zero-arg determinism, bare-call string, static inheritance). Approving.

ar-tama and others added 5 commits August 7, 2026 09:33
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>
… so `Date` subclasses work in workflow functions

The VM's `Date` override was a plain function, so `class X extends Date`
lost the subclass identity: `super()` returned a fresh plain `Date` that
became `this`, dropping the subclass's methods and fields. This silently
broke `Date` subclasses like `TZDate` from `@date-fns/tz`.

Using `class Date extends Date_` keeps `new.target` intact, and `extends`
already wires up the prototype chain and statics, so the manual
`prototype` assignment and `Object.setPrototypeOf` fix-ups are no longer
needed. Determinism is unchanged: zero-arg construction still returns the
fixed timestamp and `Date.now()` is still overridden.

Fixes vercel#3371

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>
Use a plain function that branches on `new.target` and constructs via
`Reflect.construct(Date_, args, new.target)` instead of a class: subclassing
still works (`new.target` is forwarded), and calling `Date()` without `new`
now matches the spec — arguments are ignored and the (fixed) time string is
returned, where the previous override returned a `Date` object.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>
…entation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>
@ar-tama
ar-tama force-pushed the fix/date-subclass-vm branch from 922287c to 4c7cd3c Compare August 7, 2026 00:33
@ar-tama

ar-tama commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Both asks addressed: rebased with --signoff (DCO now passing) and reworded the changeset to describe the final Reflect.construct implementation, including the bare Date() spec-string behavior. Thanks for the review!

@ar-tama
ar-tama requested a review from TooTallNate August 10, 2026 08:03

@TooTallNate TooTallNate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed at the rebased head 4c7cd3c. Both asks from my previous review are addressed:

  1. DCO now passes — all five commits carry sign-off.
  2. Changeset reworded accurately — it now describes the Reflect.construct implementation and explicitly calls out the bare-Date() spec-string behavior change. Exactly what release notes need.

Content parity confirmed: the vm/index.ts diff is byte-identical to the head I approved (Reflect.construct(Date_, args, new.target) with the deterministic zero-arg pin and spec-string bare call); the only new material is the changeset reword. The rebase landed on a base ~9 commits behind current main, so I also verified a test-merge with today's main: builds clean and the full core suite passes — 92 files, 2013 passed / 3 expected fail (including the previously-red misrouted-lazy-resume test, now fixed on main by #3374, so your "pre-existing failure" note is resolved too).

CI: since fork PRs don't get the full suite here, I've pushed your exact head commit (no modifications) to an internal branch and opened draft #3419 to run the complete matrix against it. Fast checks are already green — Unit Tests on both ubuntu and windows, Biome — with the long e2e matrix in progress; results will accumulate there. (The one failure it shows, workbench-python-workflow deploy, is a known baseline hitting every PR right now.)

Approval stands. Once the #3419 matrix completes clean, this is ready to land — thanks for the fast, thorough turnaround on both items, and for a textbook contribution overall.

@TooTallNate

Copy link
Copy Markdown
Member

Full CI results for this PR are in (run against your exact head 4c7cd3c via draft #3419):

155 checks passed, 0 pending. The 5 reported failures are all known-baseline, none related to this change:

Check Verdict
nextjs-webpack - stable quickjs Fails on recent main runs too — known baseline
E2E Windows Tests (quickjs) Fails on recent main runs too — known baseline
workbench-python-workflow deploy Failing on every open PR right now — infra baseline
sveltekit - quickjsstepWinsRaceWorkflow Timing-sensitive race test on a historically flaky lane; the same test passed on every other node/quickjs lane in this run. Nothing in this PR touches sleep/step timing — only bare-Date() calls and subclass construction
E2E Required Check Aggregate of the above

Notably green: all determinism-relevant lanes — every other quickjs lane (whose native Date this change now matches), Unit Tests on ubuntu and windows, and the complete Vercel/local prod matrices otherwise.

This is ready to land from my side. Maintainers: #3419 can be closed and its ci/pr-3372-4c7cd3c branch deleted once this merges.

@TooTallNate
TooTallNate merged commit 1a64f68 into vercel:main Aug 10, 2026
291 of 296 checks passed
@TooTallNate

Copy link
Copy Markdown
Member

Merged, thank you!

github-actions Bot added a commit that referenced this pull request Aug 10, 2026
… so `Date` subclasses work (#3372)

* test: add failing test for Date subclassing in workflow VM

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>

* fix(core): preserve `new.target` in the deterministic `Date` override so `Date` subclasses work in workflow functions

The VM's `Date` override was a plain function, so `class X extends Date`
lost the subclass identity: `super()` returned a fresh plain `Date` that
became `this`, dropping the subclass's methods and fields. This silently
broke `Date` subclasses like `TZDate` from `@date-fns/tz`.

Using `class Date extends Date_` keeps `new.target` intact, and `extends`
already wires up the prototype chain and statics, so the manual
`prototype` assignment and `Object.setPrototypeOf` fix-ups are no longer
needed. Determinism is unchanged: zero-arg construction still returns the
fixed timestamp and `Date.now()` is still overridden.

Fixes #3371

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>

* test: add failing test for calling `Date()` without `new`

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>

* fix(core): keep `Date()` callable without `new`

Use a plain function that branches on `new.target` and constructs via
`Reflect.construct(Date_, args, new.target)` instead of a class: subclassing
still works (`new.target` is forwarded), and calling `Date()` without `new`
now matches the spec — arguments are ignored and the (fixed) time string is
returned, where the previous override returned a `Date` object.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>

* chore: update changeset to match the final `Reflect.construct` implementation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ar_tama <arata.makoto@gmail.com>

---------

Signed-off-by: ar_tama <arata.makoto@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Makoto Arata <arata.makoto@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Backport PR opened against stable: #3434. (backport job run)

TooTallNate pushed a commit that referenced this pull request Aug 10, 2026
… so `Date` subclasses work (#3372) (#3434)

* test: add failing test for Date subclassing in workflow VM




* fix(core): preserve `new.target` in the deterministic `Date` override so `Date` subclasses work in workflow functions

The VM's `Date` override was a plain function, so `class X extends Date`
lost the subclass identity: `super()` returned a fresh plain `Date` that
became `this`, dropping the subclass's methods and fields. This silently
broke `Date` subclasses like `TZDate` from `@date-fns/tz`.

Using `class Date extends Date_` keeps `new.target` intact, and `extends`
already wires up the prototype chain and statics, so the manual
`prototype` assignment and `Object.setPrototypeOf` fix-ups are no longer
needed. Determinism is unchanged: zero-arg construction still returns the
fixed timestamp and `Date.now()` is still overridden.

Fixes #3371




* test: add failing test for calling `Date()` without `new`




* fix(core): keep `Date()` callable without `new`

Use a plain function that branches on `new.target` and constructs via
`Reflect.construct(Date_, args, new.target)` instead of a class: subclassing
still works (`new.target` is forwarded), and calling `Date()` without `new`
now matches the spec — arguments are ignored and the (fixed) time string is
returned, where the previous override returned a `Date` object.




* chore: update changeset to match the final `Reflect.construct` implementation




---------

Signed-off-by: ar_tama <arata.makoto@gmail.com>
Signed-off-by: Makoto Arata <arata.makoto@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request Aug 10, 2026
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.

Deterministic Date override in the workflow VM breaks Date subclasses (e.g. TZDate from @date-fns/tz)

2 participants