Skip to content

fix: two augmentation-compat regressions vs Node 25.8.1 - #556

Open
colinhacks wants to merge 2 commits into
mainfrom
compat-regressions-v05
Open

fix: two augmentation-compat regressions vs Node 25.8.1#556
colinhacks wants to merge 2 commits into
mainfrom
compat-regressions-v05

Conversation

@colinhacks

@colinhacks colinhacks commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Two verified fixes from the v0.5.0 Deno-corpus regression audit. Nine-cause breakdown: wiki/research/nub-v0.5-augmentation-regressions.md.

  • fix(runtime): probe lazy MessageEvent/File with in, not typeof, so undici isn't loaded at startup (moduleLoadList 227→114; Node 110).
  • test(node-compat): reap the child's process group on timeout/exit (orphaned grandchildren killed the CI runner; job pulled 2026-06-03).

Copilot AI review requested due to automatic review settings July 24, 2026 22:08
@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview, Comment Jul 24, 2026 11:14pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot 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.

Important

The three fixes are sound and well-verified, but widening the source-maps gate to all of Node 26.x left two references describing the old 26.2.x-only behavior — including a user-facing docs page that now states the wrong versions.

Reviewed changes — three independent augmentation-compat fixes from the v0.5.0 Deno-corpus regression audit, plus a test-only process-group reap.

  • Widen the source-maps gate to all of Node 26.xsource_maps_safe now returns node_version.major() != 26; the doc comment and unit test are updated in-diff to reflect the 2026-07-23 sweep (26.0–26.5 all rethrow no-message assert.ok(false) as TypeError; 24.17/25.9 clean).
  • Stop materializing undici at preload time in polyfills.cjs — the File/Blob backfill and the MessageEvent.ports-freeze wrap now probe existence with in instead of typeof, and the MessageEvent wrap is additionally version-gated to skip on Node >= 22.3.0 (where the native undici-backed MessageEvent already freezes .ports).
  • Reap the child's process group on exit and timeout in the node_compat test — the child is spawned as its own process-group leader (process_group(0)) and a new reap_group SIGKILLs the negative pgid, so forked servers/workers/grandchildren aren't orphaned to PID 1 (the failure that pulled the compat corpus from CI).

⚠️ Docs still describe the old 26.2.x-only source-maps gate

The TypeScript runtime docs page still says source maps are withheld only on the "26.2.x patch band" and that "Every other version (18.19 through 26.1, and 26.3+) gets source maps." After this PR that's false — source maps are now withheld across all of 26.x, so 26.0 / 26.1 / 26.3 / 26.4 / 26.5 do NOT get remapped stack traces. A user on the recommended newest Node major will read behavior the binary no longer exhibits.

Technical details
# Docs describe the old 26.2.x-only source-maps gate

## Affected sites
- `site/content/docs/runtime/typescript.mdx:97` — "except the **26.2.x** patch band" and "Every other version (18.19 through 26.1, and 26.3+) gets source maps" now contradict `source_maps_safe` (`major() != 26`).

## Required outcome
- The page states that source maps are withheld across the entire Node 26.x line (not just 26.2.x), and does not claim 26.1/26.3+ get source maps.

## Open questions for the human (optional)
- Whether to keep the version specifics inline or generalize to "the Node 26 line" so the copy doesn't need re-editing when a fixed 26.x lands.

ℹ️ Stale code comment in compute_inject_flags

The inline comment at the ALWAYS_INJECT loop still reads "withheld on Node 26.2.x (see source_maps_safe): there it turns a no-message AssertionError into a TypeError." It restates the pre-PR gate; the authoritative doc comment on source_maps_safe itself was updated, but this second copy wasn't.

Technical details
# Stale 26.2.x comment at the injection site

## Affected sites
- `crates/nub-core/src/node/flags.rs:161-162` — "withheld on Node 26.2.x" / "there it turns a no-message AssertionError into a TypeError"; not in the diff, so it wasn't updated alongside the gate.

## Required outcome
- The comment reflects the widened gate (all of 26.x), or defers to `source_maps_safe`'s now-authoritative doc comment rather than duplicating the band.

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

`globalThis.MessageEvent` and `globalThis.File` are lazy undici-backed globals.
The preload probed them with `typeof`, and that value read synchronously
materializes undici plus its whole http/http2/tls/crypto/zlib/worker closure —
process.moduleLoadList went to 227 entries where plain node has 110, on every
nub startup.

Probe with `in` instead, which sees the lazy property without firing its getter,
and version-gate the MessageEvent.ports freeze: Node freezes `.ports` natively
from 22.3.0, so the wrap is a no-op across the entire fast tier (floor 22.15).
Only the pre-22.3 compat tier still installs it, where the load is the accepted
cost of the correctness it buys.

The block directly above already documented this exact trap for Temporal
("Touching globalThis.Temporal here would defeat that laziness"); MessageEvent
was missed.

Measured: moduleLoadList 227 -> 114 (node: 110), undici absent at startup, and
`.ports` still frozen on delivery. Also removes the startup cost that made
several profiler-sensitive Node compat tests exceed their harness budget, and
the per-Worker accumulation behind a 5.5x RSS growth over 60 workers.
The harness killed only the leader on timeout. Node compat tests routinely fork
servers, workers and `spawn(process.execPath)` grandchildren, which were then
orphaned to PID 1 and kept running; across ~2,554 tests at up to 16-way
parallelism they accumulated until the CI runner died. That is why the compat
corpus job was removed from CI (2026-06-03) and never restored, leaving the
suite with no automated signal.

Spawn each test as its own process-group leader and SIGKILL the negative pgid,
at both the timeout path and the clean-exit path — a leader can exit zero while
leaving a detached server behind. `tests/cross-runtime/run.mjs` already solved
this the same way; this ports it to the Rust harness.

Verified against a leaking fixture: a non-detached child left running by the
test is reaped by the group kill. A `detached: true` grandchild calls setsid and
escapes any pgid reap by construction; that is inherent and out of scope.
@colinhacks
colinhacks force-pushed the compat-regressions-v05 branch from 516f422 to 97379de Compare July 24, 2026 23:12
@colinhacks colinhacks changed the title fix: three augmentation-compat regressions vs Node 25.8.1 fix: two augmentation-compat regressions vs Node 25.8.1 Jul 24, 2026

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes — the PR narrowed from three fixes to two since the prior pullfrog review; the source-maps gate widening in flags.rs (and its typescript.mdx doc reference, which the prior review flagged) was dropped, so those findings no longer apply. The two remaining fixes are correct and self-contained.

  • Stop materializing undici at preload time in polyfills.cjs — the File/Blob backfill and the MessageEvent.ports-freeze wrap now probe existence with in instead of a value read, and the MessageEvent wrap is additionally version-gated to skip on Node >= 22.3.0 (where the native undici-backed MessageEvent already freezes .ports), so undici is no longer forced at startup on the fast tier.
  • Reap the child's process group on exit and timeout in the node_compat test — the child is spawned as its own process-group leader (process_group(0), #[cfg(unix)]) and a new reap_group SIGKILLs the negative pgid, so forked servers/workers/grandchildren aren't orphaned to PID 1 (the failure that pulled the compat corpus from CI).

Verification notes for this pass:

  • The File/Blob in-probe still runs the backfill on the floor (where the globals are genuinely absent) and skips it on modern tiers without firing the lazy getter; the inner per-name typeof … === "undefined" guard is correct on the floor.
  • The MessageEvent version gate (major > 22 || (major === 22 && minor >= 3)) skips the wrap across the entire fast tier (floor 22.15), matching the confirmed 22.3.0 native-freeze point (nodejs/node#52370).
  • The process-group reap ordering is correct on both paths (clean-exit reaps the leader via try_wait then signals surviving members; timeout does reap_groupchild.kill()child.wait()), and libc is a cfg(unix) dependency available to the integration test.

Pullfrog  | View workflow run | Using Claude Opus𝕏

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.

2 participants