Skip to content

upgrade: rebase patch series onto v24.20.0 - #11

Merged
gmaclennan merged 8 commits into
recipefrom
upgrade/v24.20.0
Sep 2, 2026
Merged

upgrade: rebase patch series onto v24.20.0#11
gmaclennan merged 8 commits into
recipefrom
upgrade/v24.20.0

Conversation

@gmaclennan

@gmaclennan gmaclennan commented Sep 1, 2026

Copy link
Copy Markdown
Member

Re-bases the patch series onto upstream v24.20.0. No behaviour change of our own — the recipe still reconstructs byte-for-byte, at tree fb503ea45e0582c9ec6c9011905597f9633c5869.

Two conflicts, both of the kinds UPGRADING.md already names. The fork README.md replaces the upstream remainder, so it resolves to ours; the upstream churn behind it was collaborator-list edits only. The upstream-CI removal patch hit delete/modify on the 28 workflows v24.20.0 touched, where the intent is still deletion. v24.20.0 also adds one workflow, test-linux-quic.yml, now deleted and assigned to the removal patch in files.map so the materialized tree keeps carrying zero of them.

The same commit adds 40 .status skips for tests v24.20.0 introduced: child-process spawners, the test-permission-drop-* family that gives up fs.write before the harness writes its verdict, the new debugger-probe --cond cases on iOS (Android has a glob for the family), and the JSPI test, which iOS runs against the polywasm polyfill. The ~250 new QUIC tests need no entries — the build does not pass --experimental-quic, so each self-skips, which scores PASS. These entries are inferred from reading the test sources, not measured. The full device suite this PR triggers is what confirms them, and a green run is the evidence; if it turns up more, they go in as a follow-up commit here.

Verified locally: reconstruction from a fresh upstream clone prints OK on the recorded hash, the tree carries zero workflow files, audit-test-edits.sh passes, and every added .status name resolves to a real test file. An iOS full simulator build is running to exercise the TARGET_OS_OSX guards in crypto_context.cc — upstream changed 238 lines in that file, though none of it touches the Security-framework calls the guards wrap. Android cannot be built on macOS, so CI is the first cross-compile.

The second commit is documentation, from two things that cost real time here. prepare.sh clones --depth 1, so a patch touching a file upstream has modified since the old base fails with sha1 information is lacking or useless rather than a conflict — --3way has no pre-image blob to merge against. Fetching the old base tag into the tree supplies them and turns it back into an ordinary resolvable conflict. And prepare.sh refuses to run when out/ exists, so it cannot resume: after a conflict you apply the rest of the series yourself and redo the mobile-src overlay, which is easy to miss and leaves regenerate-patches.py reading every fork-only file as deleted. UPGRADING.md also gains a section on triaging the tests an upstream release adds, since the full suite gates this PR and a minor release can add hundreds.

The version header stays at 24.19.0-2, so this lands and can be exercised before anyone decides to ship it.

Two conflicts, both routine: the fork README replaces the upstream
remainder (resolved to ours; upstream churn was collaborator-list only),
and the upstream-CI removal patch hit delete/modify on the 28 workflows
v24.20.0 touched.

v24.20.0 adds one workflow, test-linux-quic.yml, now deleted and owned by
the removal patch so the materialized tree keeps carrying zero of them.

Also skips the new tests that cannot be scored on a device: child-process
spawners, the test-permission-drop-* family that gives up fs.write before
the harness writes its verdict, the new debugger-probe --cond cases on iOS
(Android has a glob), and the JSPI test against the polywasm polyfill. The
~250 new QUIC tests need no entries — the build does not pass
--experimental-quic, so each one self-skips, which scores PASS. These are
inferred from the test sources; the full device suite this PR triggers is
what confirms them.
Both bit during the v24.20.0 bump. prepare.sh clones --depth 1, so a patch
touching a file upstream has modified since the old base fails with "sha1
information is lacking or useless" rather than a conflict: --3way has no
pre-image blob to merge against. Fetching the old base tag into the tree
supplies them and turns it back into an ordinary resolvable conflict.

prepare.sh also refuses to run when out/ exists, so it cannot resume. After
a conflict you apply the rest of the series yourself and redo the mobile-src
overlay, which is easy to miss and leaves regenerate-patches.py reading
every fork-only file as deleted.

Adds a section on triaging the tests an upstream release adds, since the
full device suite gates the merge on an upgrade PR and a minor release can
add hundreds. Also notes that entries inferred this way do not belong in
the blocks headed "measured on a full device sweep".
v24.20.0 moves ncrypto::EnginePointer out of ncrypto.cc into a new
deps/ncrypto/engine.cc, built as its own static_library target that
ncrypto depends on. gyp picks that dependency up on Android, but the iOS
framework links a hand-maintained list of archives, so libnode_base's
references to EnginePointer::init, loadPrivateKey, setClientCertEngine and
friends had nothing to resolve against and both iOS legs failed at Ld.

The archive is already built — only the copy list and the pbxproj needed
to learn about it. Same shape as the libnode_base split in v24.19.0, and
common to both flavors: the target is gated on OpenSSL >= 3, not on any
of the flags lite drops.
The iOS framework links a hand-maintained archive list, so a new gyp
static_library is invisible to it until Ld fails on symbols nobody
touched — an hour into the build, after every compile has passed. Twice
in two releases now.
@gmaclennan

Copy link
Copy Markdown
Member Author

Both iOS legs failed the first run at Ld with undefined ncrypto::EnginePointer::* symbols referenced from libnode_base.a. Not a guard problem and not a flake: v24.20.0 moves EnginePointer out of ncrypto.cc into a new deps/ncrypto/engine.cc, built as its own static_library target that ncrypto depends on. gyp follows that dependency on Android, but the iOS framework links a hand-maintained list of archives, so nothing knew to link libncrypto_engine.a — the archive was being built and then ignored.

Fixed by adding it to outputs_common and to the four NodeMobile.xcodeproj entries. Same shape as the libnode_base split in v24.19.0, which that file already carries a note about, and it belongs in outputs_common rather than outputs_full_only because the target is gated on OpenSSL >= 3, not on anything lite drops. Tree hash moves to 821c5305e0945674cfc6167a9ab7895e5623739a; reconstruction from a clean clone verified.

A third commit adds this to UPGRADING.md, since it is now twice in two releases that an upstream target split has surfaced as a link failure an hour into the build.

The Android legs had not finished when the new run superseded them, so they are re-running from scratch.

v24.20.0 appends --experimental-wasm-jspi to the V8 argument list
unconditionally. iOS builds V8 with --v8-lite-mode, which turns off
V8_ENABLE_WEBASSEMBLY and compiles out the whole --experimental-wasm-*
family, so V8 leaves the flag unconsumed, node reports "bad option" and
returns kInvalidCommandLineArgument before running a line of JS. Every
iOS launch failed: the boot smoke got a FAIL verdict with no output, and
the curated suite could not even complete test.py's `-p process.arch`
probe, so it reported "No tests to run".

Guarded on the platform rather than on V8_ENABLE_WEBASSEMBLY: node.gyp
never includes v8_gypfiles/features.gypi, so that define reaches V8's
targets and not node's own. Testing it here would read as false
everywhere and drop the flag on Android too, where V8 is not in lite
mode and WebAssembly works. Android is unaffected and its builds and
both boot smokes passed on the run that caught this.
@gmaclennan

Copy link
Copy Markdown
Member Author

Second real v24.20.0 break, this one on iOS only.

smoke-ios (full) and curated-tests-ios (full) both failed. The curated job never ran a test — it died in tools/test.py's -p process.arch probe with Can't determine the arch and No tests to run; the smoke job got a FAIL verdict with completely empty app output. Same underlying cause: node exits before running any JS.

v24.20.0 appends --experimental-wasm-jspi to the V8 argument list unconditionally (src/node.cc, ProcessGlobalArgsInternal). iOS builds V8 with --v8-lite-mode, which turns off V8_ENABLE_WEBASSEMBLY and compiles out the entire --experimental-wasm-* family — flag-definitions.h guards that whole block. SetFlagsFromCommandLine therefore leaves the flag unconsumed, and node turns any leftover into bad option: ... and returns kInvalidCommandLineArgument. That happens before stdio redirection is useful, which is why the app printed nothing at all.

Guarded on the platform rather than on V8_ENABLE_WEBASSEMBLY, deliberately: node.gyp never includes tools/v8_gypfiles/features.gypi, so that define is set on V8's targets and not on node's own translation units. Testing it in node.cc would read as false on every platform and silently drop the flag on Android too, where V8 is not in lite mode and WebAssembly works. The guard matches the #if !(defined(__APPLE__) && TARGET_OS_IPHONE) idiom already used a few hundred lines up in the same file.

Corroborating evidence arrived while I was writing the fix: all six Android builds, both combines and both Android boot smokes passed on the same run. Exactly what the diagnosis predicts.

Tree hash moves to 5b78a8155a21da69eefee62935bddb4f2c996621, reconstruction from a clean clone verified. This looks like an upstream bug for any v8_enable_webassembly=0 build, not something specific to this fork — probably worth reporting to nodejs/node.

The full Android suite found what static triage missed. All three spawn a
child node process, which cannot work on Android where process.execPath is
app_process64 -- the child's stderr comes back as "Error changing
dalvik-cache ownership: Permission denied".

None of them names child_process. test-stream-iter-disabled and
test-stream-iter-readable-interop-disabled call spawnPromisified
destructured off require('../common'), and test-runner-entry-file spawns
through node:test's run({ isolation: 'process' }).

Android only: all three pass on the iOS simulator, where posix_spawn is
permitted, and the iOS suite is green with them running. Skipping a test
that currently passes would cost coverage for nothing.
The recipe as written missed all three tests the Android suite then
caught, because none of them imports child_process: two spawn through
spawnPromisified destructured off common, one through node:test's
run({ isolation: 'process' }). Names the patterns that actually find
them, and records that such tests fail on Android while still passing on
the iOS simulator, so the skip belongs on one platform and not both.
@gmaclennan

Copy link
Copy Markdown
Member Author

The full device suite did its job. iOS came back completely green — all four shards, plus the curated gate and the boot smoke, so both earlier fixes hold. Android failed two of four shards on exactly three tests:

  • test-runner-entry-file
  • test-stream-iter-disabled
  • test-stream-iter-readable-interop-disabled

All three are new in v24.20.0 and all three spawn a child node process, which cannot work on Android — the child stderr comes back as Error changing dalvik-cache ownership : Permission denied, the app_process64 signature the existing .status comments describe.

My static triage missed them, and the reason is worth recording: none of them names child_process. The two stream-iter ones call spawnPromisified destructured off require("../common"), and test-runner-entry-file spawns inside node:test via run({ isolation: "process" }). Grepping for child_process / spawnSync / execFile finds neither shape. Re-scanning all 94 new non-QUIC tests with the corrected patterns turns up no further gaps — the only other spawners are already covered by the test-cli-* and test-debugger-probe-* globs, which the suite run independently confirms.

Skipped on Android only. All three pass on the iOS simulator, where posix_spawn is permitted, and the iOS suite is green with them running — skipping a passing test would cost coverage for nothing. A device sweep could revisit that, per the existing convention for this class.

A second commit widens the triage recipe in UPGRADING.md so the next upgrade greps for process.execPath, spawnPromisified and isolation rather than just child_process.

Tree hash moves to e421e4206e44d197e0396328a44bf623efaad9d2, clean-clone reconstruction verified.

Skipping them on Android alone was wrong. test-stream-iter-disabled then
failed an iOS shard on the very next run, having passed the one before:
posix_spawn is permitted on the simulator, so a spawner there is flaky
rather than working. Neither outcome is worth acting on, since none of
these can run on a real device.

Corrects the guidance added alongside the Android skips, which read a
single green iOS run as evidence the tests were sound.
@gmaclennan

Copy link
Copy Markdown
Member Author

Correcting myself: skipping those three on Android alone was the wrong call.

On this run, Android went green — all four shards — but full-suite-ios / ios (3) failed on test-stream-iter-disabled, one of the same three, which had passed on iOS the run before. So on the simulator these are flaky rather than passing: posix_spawn is permitted there, so a child-process spawner often works and sometimes does not. I read one green iOS run as evidence the tests were sound; it was not.

They are now skipped on both platforms, in the same cause-named section as the other inferred child-process entries. None of them can run on a real iOS device regardless, which is why parallel.status already carries a block describing this class as "not coverage worth keeping". The UPGRADING.md guidance I added alongside the Android skips said the opposite and is corrected in the same commit.

Tree hash fbc10b7d21282ad5b4654f335263bcd40026eb48, reconstruction verified.

Everything else on that run was green: all eight builds, all three combines, all three boot smokes, both curated gates, and all four Android suite shards.

@gmaclennan
gmaclennan merged commit 49296f1 into recipe Sep 2, 2026
47 of 56 checks passed
@gmaclennan
gmaclennan deleted the upgrade/v24.20.0 branch September 2, 2026 15:00
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.

1 participant