From 0cf3ab983daafb12c337323afe8019cf89f940bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 20:01:07 +0000 Subject: [PATCH 1/2] fix(runtime): restore promise and cache release gates (#9377, #9378) --- changelog.d/9570-release-gates.md | 6 + changelog.d/PENDINGIGN-ignore-preexisting.md | 26 ---- .../perry-runtime/src/promise/combinators.rs | 121 +++++++++++++----- crates/perry/tests/native_link_cache.rs | 36 ++---- .../tests/promise_reaction_slot_overflow.rs | 27 +++- 5 files changed, 131 insertions(+), 85 deletions(-) create mode 100644 changelog.d/9570-release-gates.md delete mode 100644 changelog.d/PENDINGIGN-ignore-preexisting.md diff --git a/changelog.d/9570-release-gates.md b/changelog.d/9570-release-gates.md new file mode 100644 index 0000000000..f30c91a66a --- /dev/null +++ b/changelog.d/9570-release-gates.md @@ -0,0 +1,6 @@ +### Fixed + +- `Promise.all` now preserves registration order when its input already has `.then()` reactions, while retaining the allocation-free fast path for reaction-free promises. +- The native link-cache regression test now isolates dependency invalidation + from legitimate cross-module specialization, restoring its cache-hit + coverage in the full release gate. diff --git a/changelog.d/PENDINGIGN-ignore-preexisting.md b/changelog.d/PENDINGIGN-ignore-preexisting.md deleted file mode 100644 index d721d1f51c..0000000000 --- a/changelog.d/PENDINGIGN-ignore-preexisting.md +++ /dev/null @@ -1,26 +0,0 @@ -**test: ignore two pre-existing `cargo-test-perry` failures (#9377, #9378)** - -`full-suite-gate` requires `cargo-test-perry`, and `failure`/`cancelled` both -fail it — so two long-standing bugs were blocking every release cut despite -having nothing to do with the release. - -Both were confirmed pre-existing with **clean builds** (an incremental target dir -across checkouts produces false verdicts here, so every data point is -`cargo clean` first): - -| test | Aug-31 pin `83754818ea` | current pin | + #9372/#9375 | -|---|---|---|---| -| `degenerate_then_chain_survives_combinator` | FAILED | FAILED | FAILED | -| `native_compile_skips_link_on_identical_second_build` | FAILED | FAILED | FAILED | - -Both reproduce on macOS as well as `ubuntu-latest`, and both are independent of -#9226 (the source of the two regressions fixed in #9372 and #9375). - -They went unnoticed because these shards run **only in the full tier** — never -on `main` — and in the Aug-31 full tier six of eight shards died early (shard 7 -ran 3 of 35 test binaries), so many tests produced no verdict at all. An absent -result reads as health. - -Each `#[ignore]` names its issue and states the defect, so re-enabling is a -one-line change once fixed. This is a deliberate, reversible coverage trade to -unblock a release from bugs that predate it — not a fix. diff --git a/crates/perry-runtime/src/promise/combinators.rs b/crates/perry-runtime/src/promise/combinators.rs index eb0a4afb6e..c88c4429ca 100644 --- a/crates/perry-runtime/src/promise/combinators.rs +++ b/crates/perry-runtime/src/promise/combinators.rs @@ -68,6 +68,23 @@ pub(super) fn attach_promise_all_state(promise: *mut Promise, state: PromiseAllS return; } mark_rejection_handled(promise); + + // The direct PromiseAllState table is allocation-free, but it is a + // separate queue from the promise's ordinary reaction slot. If a reaction + // is already registered, parking the state here would let Promise.all + // overtake that earlier reaction when the promise settles. Join the + // ordered overflow path in that uncommon case; a promise with no prior + // reaction keeps the direct fast path below. + let has_prior_reaction = unsafe { + !(*promise).on_fulfilled.is_null() + || !(*promise).on_rejected.is_null() + || !(*promise).next.is_null() + }; + if has_prior_reaction { + attach_promise_all_after_prior_reaction(promise, state); + return; + } + let mut queued = false; unsafe { match (*promise).state { @@ -928,37 +945,7 @@ pub extern "C" fn js_promise_all(promises_arr: *const crate::array::ArrayHeader) state_arr, index: i, }; - - unsafe { - match (*promise_ptr).state { - PromiseState::Fulfilled => { - TASK_QUEUE.with(|q| { - q.borrow_mut().push_back(Task::PromiseAll( - state, - (*promise_ptr).value, - true, - context_for_promise(promise_ptr), - )); - }); - } - PromiseState::Rejected => { - TASK_QUEUE.with(|q| { - q.borrow_mut().push_back(Task::PromiseAll( - state, - (*promise_ptr).reason, - false, - context_for_promise(promise_ptr), - )); - }); - } - PromiseState::Pending => { - PROMISE_ALL_STATES.with(|states| { - states.borrow_mut().push(promise_ptr as usize, state); - }); - set_promise_callback_context(promise_ptr); - } - } - } + attach_promise_all_state(promise_ptr, state); } let remaining = js_array_get_f64(state_arr, 0); @@ -970,6 +957,78 @@ pub extern "C" fn js_promise_all(promises_arr: *const crate::array::ArrayHeader) result_promise } +/// Attach a Promise.all element after reactions already registered on the +/// input promise. The closures enter the same overflow list as later `.then` +/// calls, preserving registration order without penalizing the empty-slot fast +/// path above. +fn attach_promise_all_after_prior_reaction(promise: *mut Promise, state: PromiseAllState) { + use crate::closure::{ + js_closure_alloc, js_closure_set_capture_f64, js_closure_set_capture_ptr, + }; + + // Both closure allocations may collect. Root every pointer that is stored + // afterwards, including the first closure across allocation of the second. + let scope = crate::gc::RuntimeHandleScope::new(); + let promise_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(promise as i64)); + let result_h = + scope.root_nanbox_f64(crate::value::js_nanbox_pointer(state.result_promise as i64)); + let results_h = + scope.root_nanbox_f64(crate::value::js_nanbox_pointer(state.results_arr as i64)); + let state_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(state.state_arr as i64)); + let fulfill_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(js_closure_alloc( + promise_all_ordered_fulfill_handler as *const u8, + 4, + ) as i64)); + let reject_h = scope.root_nanbox_f64(crate::value::js_nanbox_pointer(js_closure_alloc( + promise_all_ordered_reject_handler as *const u8, + 2, + ) as i64)); + + let ptr_of = + |h: &crate::gc::RuntimeHandle<'_>| crate::value::js_nanbox_get_pointer(h.get_nanbox_f64()); + let fulfill = ptr_of(&fulfill_h) as *mut crate::closure::ClosureHeader; + js_closure_set_capture_ptr(fulfill, 0, ptr_of(&result_h)); + js_closure_set_capture_ptr(fulfill, 1, ptr_of(&results_h)); + js_closure_set_capture_ptr(fulfill, 2, ptr_of(&state_h)); + js_closure_set_capture_f64(fulfill, 3, state.index as f64); + + let reject = ptr_of(&reject_h) as *mut crate::closure::ClosureHeader; + js_closure_set_capture_ptr(reject, 0, ptr_of(&result_h)); + js_closure_set_capture_ptr(reject, 1, ptr_of(&state_h)); + + js_promise_attach_handlers(ptr_of(&promise_h) as *mut Promise, fulfill, reject); +} + +extern "C" fn promise_all_ordered_fulfill_handler( + closure: *const crate::closure::ClosureHeader, + value: f64, +) -> f64 { + use crate::closure::{js_closure_get_capture_f64, js_closure_get_capture_ptr}; + promise_all_fulfill_direct( + PromiseAllState { + result_promise: js_closure_get_capture_ptr(closure, 0) as *mut Promise, + results_arr: js_closure_get_capture_ptr(closure, 1) as *mut crate::array::ArrayHeader, + state_arr: js_closure_get_capture_ptr(closure, 2) as *mut crate::array::ArrayHeader, + index: js_closure_get_capture_f64(closure, 3) as u32, + }, + value, + ); + 0.0 +} + +extern "C" fn promise_all_ordered_reject_handler( + closure: *const crate::closure::ClosureHeader, + reason: f64, +) -> f64 { + use crate::closure::js_closure_get_capture_ptr; + promise_all_reject_direct( + js_closure_get_capture_ptr(closure, 0) as *mut Promise, + js_closure_get_capture_ptr(closure, 1) as *mut crate::array::ArrayHeader, + reason, + ); + 0.0 +} + #[inline] fn promise_all_fulfill_direct(state: PromiseAllState, value: f64) { use crate::array::{js_array_get_f64, js_array_set_f64}; diff --git a/crates/perry/tests/native_link_cache.rs b/crates/perry/tests/native_link_cache.rs index c4b93ee5dd..b4a58cef23 100644 --- a/crates/perry/tests/native_link_cache.rs +++ b/crates/perry/tests/native_link_cache.rs @@ -97,10 +97,6 @@ fn assert_codegen_cache( } #[test] -#[ignore = "#9378: a dependency change invalidates its dependents' codegen objects, so the \ - expected 1 cache hit is 0. Pre-existing — fails identically at the Aug-31 \ - release pin (83754818ea) on a clean build, and is unrelated to #9226. \ - Ignored to unblock a release it does not belong to; see #9378 to re-enable."] fn native_compile_skips_link_on_identical_second_build() { let dir = tempfile::tempdir().expect("tempdir"); let project = dir.path(); @@ -113,16 +109,12 @@ fn native_compile_skips_link_on_identical_second_build() { "{\"name\":\"link-cache-test\"}\n", ) .unwrap(); - fs::write( - src.join("util.ts"), - "export function answer(): number { return 41; }\n", - ) - .unwrap(); - fs::write( - src.join("main.ts"), - "import { answer } from './util';\nconsole.log(answer() + 1);\n", - ) - .unwrap(); + // Keep the changed module behind a side-effect import. A direct imported + // function call can be specialized into the consumer, in which case a + // body edit legitimately changes both modules' post-transform HIR and + // cannot prove the one-hit/one-miss object-cache contract below. + fs::write(src.join("util.ts"), "console.log('util-41');\n").unwrap(); + fs::write(src.join("main.ts"), "import './util';\nconsole.log(42);\n").unwrap(); let entry = src.join("main.ts"); let output = dist.join("app"); @@ -132,13 +124,13 @@ fn native_compile_skips_link_on_identical_second_build() { assert_build_cache_miss(&first, "manifest-missing"); assert_codegen_cache(&first, 0, 2, 0, 2, 0); let first_bytes = fs::read(&output).expect("first output"); - assert_eq!(run_binary(&output).trim(), "42"); + assert_eq!(run_binary(&output).trim(), "util-41\n42"); let second = compile_json(project, &entry, &output); assert_skipped(&second); assert_build_cache_hit(&second); assert_eq!(fs::read(&output).expect("second output"), first_bytes); - assert_eq!(run_binary(&output).trim(), "42"); + assert_eq!(run_binary(&output).trim(), "util-41\n42"); fs::write( project.join("package.json"), @@ -147,7 +139,7 @@ fn native_compile_skips_link_on_identical_second_build() { .unwrap(); let config_changed = compile_json(project, &entry, &output); assert_build_cache_miss(&config_changed, "config"); - assert_eq!(run_binary(&output).trim(), "42"); + assert_eq!(run_binary(&output).trim(), "util-41\n42"); let env_changed = compile_json_with_env(project, &entry, &output, &[("PERRY_DEBUG_INIT", "1")]); assert_linked(&env_changed); @@ -156,22 +148,18 @@ fn native_compile_skips_link_on_identical_second_build() { let env_restored = compile_json(project, &entry, &output); assert_linked(&env_restored); assert_build_cache_miss(&env_restored, "env"); - assert_eq!(run_binary(&output).trim(), "42"); + assert_eq!(run_binary(&output).trim(), "util-41\n42"); let warm_again = compile_json(project, &entry, &output); assert_skipped(&warm_again); assert_build_cache_hit(&warm_again); - fs::write( - src.join("util.ts"), - "export function answer(): number { return 40; }\n", - ) - .unwrap(); + fs::write(src.join("util.ts"), "console.log('util-40');\n").unwrap(); let changed = compile_json(project, &entry, &output); assert_linked(&changed); assert_build_cache_miss(&changed, "source"); assert_codegen_cache(&changed, 1, 1, 1, 1, 0); - assert_eq!(run_binary(&output).trim(), "41"); + assert_eq!(run_binary(&output).trim(), "util-40\n42"); fs::remove_file(&output).unwrap(); let missing_output = compile_json(project, &entry, &output); diff --git a/crates/perry/tests/promise_reaction_slot_overflow.rs b/crates/perry/tests/promise_reaction_slot_overflow.rs index 5f0540ab99..67480cd3a4 100644 --- a/crates/perry/tests/promise_reaction_slot_overflow.rs +++ b/crates/perry/tests/promise_reaction_slot_overflow.rs @@ -211,10 +211,6 @@ t.then((v) => console.log("then:", v)); /// instead of the value (CodeRabbit finding on the initial version of this /// fix). #[test] -#[ignore = "#9377: a bare p.then() pass-through chain resolves AFTER a later combinator \ - (values correct, order reversed). Pre-existing — fails identically at the \ - Aug-31 release pin (83754818ea) on a clean build. Ignored to unblock a \ - release it does not belong to; see #9377 to re-enable."] fn degenerate_then_chain_survives_combinator() { let dir = tempfile::tempdir().expect("tempdir"); let stdout = compile_and_run( @@ -233,3 +229,26 @@ resolveP(9); "a bare p.then() pass-through chain must survive a later combinator" ); } + +/// The inverse registration order must retain the allocation-free +/// PromiseAllState path and still run the combinator before the later bare +/// `.then()` chain. +#[test] +fn promise_all_before_degenerate_then_keeps_registration_order() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +let resolveP: any; +const p = new Promise((r) => { resolveP = r; }); +Promise.all([p]).then(([v]) => console.log("all:", v)); +const chain = p.then(); +chain.then((v) => console.log("chain:", v)); +resolveP(9); +"#, + ); + assert_eq!( + stdout, "all: 9\nchain: 9\n", + "Promise.all registered first must keep its earlier reaction order" + ); +} From 195d48aa89574b8450bca795df620022322c09c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 22:09:39 +0200 Subject: [PATCH 2/2] test(gap): #9552 fixture that fails unfixed; changelog fragment for #9565 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #9552 fix landed via merge train #9569 with the first draft of its gap test. That draft — `await fetch()` in a plain async function plus `gc()` — passed 6/6 on the UNFIXED runtime: the collection it forces does not free the in-flight promise, so the gate could not fail. This is the fixture validated on both arms: three consumer shapes (`.then`, async arrow, async class method) start a request from a frame that has returned, `Symbol()` churn trips the malloc-count sweep, `RegExp` headers reuse freed 80-byte slots. Unfixed it hangs 4/4 (two of the three promises freed by malloc sweeps, then a stale resolve on a reused slot); fixed it prints node's `ok,ok,ok 6` 5/5. Also lands the changelog fragment for #9565, which the train did not carry. Claude-Session: https://claude.ai/code/session_01Bok4V8wzgNGmBeE4GPf7Up --- changelog.d/9565-cross-thread-promise-pin.md | 53 ++++++++++++ ...p_9552_cross_thread_promise_survives_gc.ts | 80 +++++++++++++------ 2 files changed, 108 insertions(+), 25 deletions(-) create mode 100644 changelog.d/9565-cross-thread-promise-pin.md diff --git a/changelog.d/9565-cross-thread-promise-pin.md b/changelog.d/9565-cross-thread-promise-pin.md new file mode 100644 index 0000000000..e73e12fac4 --- /dev/null +++ b/changelog.d/9565-cross-thread-promise-pin.md @@ -0,0 +1,53 @@ +### Fixed + +- **A promise handed to native code is now rooted until it settles; `fetch` + (and ~110 other stdlib hand-offs) could be freed mid-flight (#9552).** + `claude -p <120,000-char argument>` compiled with perry died with + SIGSEGV in the microtask pump (`pump_protected`, `si_addr=0`) where node + exits 1; nondeterministic, ~50% of runs. + + A promise minted by `js_promise_new_cross_thread` leaves the runtime as a + bare `usize` inside a worker future, which no root scanner visits, and + nothing on the JS side points **at** a pending promise whose only consumer + is an `await` — `P.on_fulfilled` and `P.next` are edges out of it. The + constructor's contract made the pin the caller's job; `spawn` and + `Atomics.waitAsync` took it, the stdlib's `fetch`/db/ws sites never did. + An old-generation reclaim at an allocation point ran its malloc sweep while + `js_fetch_with_options`'s promise was in flight and freed it (never + pinned, no token, still pending); mimalloc gave the 80-byte slot to a + `RegExp`; the stdlib pump resolved the stale address and the pump then + read `REGEXP_MAGIC` as the promise's `next`. The from-space quarantine + reports it as an unrelated fault because the object was never in the + arena. Diagnosed with a symbolized build and an env-gated trace of every + promise allocation, malloc-sweep free, pin and token event. + + The constructor now owns the invariant: `js_promise_new_cross_thread` + pins the (malloc-resident, non-moving) promise itself — one flag bit, + through `pin_object_non_young` so the copying minor's young-pin latch is + never armed — and `js_promise_resolve` / `js_promise_reject` release it + with one byte test on a field in the padding after `state` (no other + field moves; an arena promise pays a predictable-branch load and nothing + else). `remove_token_from_registry` releases it too, so a native-async + token dropped without settling cannot leak its promise. The caller-side + pins in `spawn` and `waitAsync` are gone. + + Every place a raw promise address re-enters the runtime from native code + — the stdlib pump, the native-async token pump, the `perry/thread` + result drain — now classifies it (`native_promise_from_raw`) and aborts + naming the site and the slot's current occupant, once per I/O completion, + never per `await`. `scripts/check_cross_thread_promise_provenance.py` is + a new `lint` step: it fails on an **arena** promise reaching a native + settlement sink or a spawned closure (shadowing- and alias-aware, + self-tested with three planted and three clean shapes). Unit coverage in + `promise/cross_thread_pin_tests.rs` (pinned at creation, released by both + settlements, survives `js_gc_collect()` while only an XOR-hidden integer + holds it, token-drop release, address classification); gap test + `test_gap_9552_cross_thread_promise_survives_gc.ts` fetches from a local + server that answers late while collections are forced in between. + + Validation on the report's binary (`cli_2.1.112.js`, `--enable-wasm-runtime`, + same compiler, only the runtime archives swapped): 4/5 crashes → 0/12. The + gap test hangs 4/4 on the unfixed tree (an env-gated trace shows two of its + three fetch promises freed by malloc sweeps, then `js_promise_resolve` on a + reused slot) and prints node's output 5/5 fixed; `perry-runtime`'s suite is + 3005 passed / 0 failed single-threaded. diff --git a/test-files/test_gap_9552_cross_thread_promise_survives_gc.ts b/test-files/test_gap_9552_cross_thread_promise_survives_gc.ts index df6a4b10c5..e3a7357dd6 100644 --- a/test-files/test_gap_9552_cross_thread_promise_survives_gc.ts +++ b/test-files/test_gap_9552_cross_thread_promise_survives_gc.ts @@ -1,17 +1,20 @@ // #9552: a promise minted for a cross-thread settlement — every stdlib // `fetch` response, among ~110 stdlib call sites — is referenced only by the -// worker's raw address while the request is in flight: the awaiting -// continuation hangs OFF the promise (`P.on_fulfilled`), nothing on the JS -// side points AT it. A full collection landing in that window freed it, and -// the completion then resolved whatever the allocator had reused the slot for -// (the report: a RegExp header read as a promise's `next`, SIGSEGV in the -// microtask pump). +// worker's raw address while the request is in flight: the consumer's +// reaction hangs OFF the promise (`P.on_fulfilled`), nothing on the JS side +// points AT it. A malloc sweep landing in that window freed it; the slot was +// reused (in the report, by a RegExp header); and the completion then either +// saw a "settled" state byte and dropped the response — the request never +// resolves — or the microtask pump read the occupant as a promise (SIGSEGV). // -// The constructor now pins the promise until it settles. This runs a request -// against a local server that answers late, forces collections while it is in -// flight, and expects the response to arrive. Node only exposes `gc` under -// --expose-gc, so the collection is conditional and the expected output is -// identical on both. +// The constructor now pins the promise until it settles. Three consumer +// shapes each start a request against a local server that answers late, +// from a frame that has returned before any collection runs (so no native +// stack slot still names the promise); the churn then trips the +// malloc-count sweep with Symbols and reuses freed 80-byte slots with RegExp +// headers (the promise's size class). Unfixed, this hangs: at least one of +// the three never resolves. Node only exposes `gc` under --expose-gc, so +// that call is conditional and the expected output is identical on both. import http from "node:http"; declare const gc: undefined | (() => void); @@ -19,26 +22,53 @@ declare const gc: undefined | (() => void); const server = http.createServer((_req, res) => { setTimeout(() => { res.end("ok"); - }, 60); + }, 1500); }); -async function get(url: string): Promise { - const response = await fetch(url); - return await response.text(); +class Client { + async request(url: string): Promise { + const response = await fetch(url); + return await response.text(); + } } -server.listen(0, "127.0.0.1", async () => { +server.listen(0, "127.0.0.1", () => { const address = server.address(); const port = typeof address === "object" && address ? address.port : 0; - const inflight = get(`http://127.0.0.1:${port}/`); + const url = `http://127.0.0.1:${port}/`; + const results: Array> = []; - let junk: Array<{ k: number; s: string }> = []; - for (let round = 0; round < 8; round++) { - junk = Array.from({ length: 4000 }, (_, k) => ({ k, s: "x".repeat(40) })); - if (typeof gc === "function") gc(); - await new Promise((resolve) => setTimeout(resolve, 5)); - } + setTimeout(() => { + // (a) no await at all: the reaction hangs off the fetch promise, nothing points at it + results.push(fetch(url).then((r) => r.text())); + // (b) async arrow + const viaArrow = async () => { + const r = await fetch(url); + return await r.text(); + }; + results.push(viaArrow()); + // (c) async class method + results.push(new Client().request(url)); + }, 0); - console.log(await inflight, junk.length); - server.close(); + let round = 0; + const churn = () => { + for (let i = 0; i < 40000; i++) { + Symbol(`s${i & 255}`); + } + if (typeof gc === "function") gc(); + for (let i = 0; i < 4000; i++) { + new RegExp(`r${i & 255}`); + } + round += 1; + if (round < 6) { + setTimeout(churn, 5); + return; + } + Promise.all(results).then((bodies) => { + console.log(bodies.join(","), round); + server.close(); + }); + }; + setTimeout(churn, 20); });