From 3ec25393d891b3f1d81cbabcd445ee5ddcf6090c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 13:06:56 +0000 Subject: [PATCH 1/2] fix(runtime): timeout.refresh() preserves ref state; Immediate has no numeric conversion refresh() called set_timer_ref_state(id, true) unconditionally, so refreshing an unrefd Timeout/Interval re-refd it -- hasRef() flipped to true and the process stayed alive for a callback that had been deliberately detached from the event loop. Node refresh() reschedules only and never touches ref state; drop the forced ref-state write and let the existing entry (set at schedule time, updated by any ref()/unref() since, pinned while the timer is queued) stand. js_number_coerce gave setImmediate handles the same numeric-conversion shortcut as Timeout handles, so +setImmediate(...) returned a number instead of NaN. Node only gives Timeout (setTimeout/setInterval) a numeric conversion; Immediate has none. Add is_immediate_timer_id and gate the shortcut on it so an Immediate falls through to the generic toPrimitive/toString path, which already yields NaN. Fixes #10541 Fixes #10542 --- crates/perry-runtime/src/builtins/numbers.rs | 6 ++ crates/perry-runtime/src/timer.rs | 21 +++- .../perry-runtime/src/timer/tests_inline.rs | 71 ++++++++++++++ ...2_timer_refresh_ref_immediate_primitive.ts | 97 +++++++++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 test-files/test_gap_10541_10542_timer_refresh_ref_immediate_primitive.ts diff --git a/crates/perry-runtime/src/builtins/numbers.rs b/crates/perry-runtime/src/builtins/numbers.rs index 6c4c6bf065..d2f8760cd8 100644 --- a/crates/perry-runtime/src/builtins/numbers.rs +++ b/crates/perry-runtime/src/builtins/numbers.rs @@ -562,8 +562,14 @@ pub extern "C" fn js_number_coerce(value: f64) -> f64 { // identifiers, so test assertions like `typeof x === "number"` // hold). Gate on the timer registry so unrelated small handles // (UI widgets, drizzle, etc.) still fall through to toPrimitive. + // #10542: only a Timeout (setTimeout/setInterval) converts to its + // id this way -- an Immediate (setImmediate) has no numeric + // conversion in Node and must fall through to the generic + // toPrimitive/toString path below (which yields NaN, matching + // `+setImmediate(...)`). if crate::value::addr_class::is_small_handle(id as usize) && crate::timer::is_known_timer_id(id) + && !crate::timer::is_immediate_timer_id(id) { return id as f64; } diff --git a/crates/perry-runtime/src/timer.rs b/crates/perry-runtime/src/timer.rs index a52fc5a5db..1a598c0147 100644 --- a/crates/perry-runtime/src/timer.rs +++ b/crates/perry-runtime/src/timer.rs @@ -634,6 +634,17 @@ pub(crate) fn timer_constructor_value(id: i64) -> Option { pub use ref_states::is_known_timer_id; +/// Whether `id` is specifically a `setImmediate` handle, as opposed to a +/// `Timeout` (`setTimeout`/`setInterval`, which Node also names `Timeout`). +/// #10542: Node's `Timeout` has a numeric conversion (`+setTimeout(...)` is +/// its internal id) but `Immediate` does not (`+setImmediate(...)` is +/// `NaN`) -- `js_number_coerce` gates its Timeout-only numeric shortcut on +/// this so an Immediate falls through to the generic (object-shaped) +/// ToPrimitive path instead. +pub(crate) fn is_immediate_timer_id(id: i64) -> bool { + matches!(timer_handle_kind(id), Some(CallbackTimerKind::Immediate)) +} + fn throw_mock_timer_invalid_state(message: &str) -> ! { let msg = crate::string::js_string_from_bytes(message.as_ptr(), message.len() as u32); crate::node_submodules::register_error_code_pub(msg, "ERR_INVALID_STATE"); @@ -901,6 +912,14 @@ pub extern "C" fn js_timer_unref(timer_id: i64) { /// resets the next-deadline cursor to one full interval from now. #[no_mangle] pub extern "C" fn js_timer_refresh(timer_id: i64) { + // #10541: refresh() reschedules only -- it must NOT change ref state. + // Node's Timeout.refresh() "sets the timer's start time to the current + // time" and says nothing about ref/unref; a timer that was unref'd + // before refresh() stays unref'd (and a ref'd one stays ref'd). The + // id's ref-state entry is left untouched here -- it was set at + // schedule() time and by any ref()/unref() call since, and it cannot + // have been evicted while this timer is still queued (its + // `_scheduled: ScheduledTimerId` field pins the registry entry). let now = Instant::now(); { @@ -908,7 +927,6 @@ pub extern "C" fn js_timer_refresh(timer_id: i64) { if let Some(timer) = timers.iter_mut().find(|t| t.id == timer_id) { timer.deadline = now + Duration::from_millis(timer.delay_ms); timer.cleared = false; - set_timer_ref_state(timer_id, true); return; } } @@ -917,7 +935,6 @@ pub extern "C" fn js_timer_refresh(timer_id: i64) { if let Some(timer) = intervals.iter_mut().find(|t| t.id == timer_id) { timer.next_deadline = now + Duration::from_millis(timer.interval_ms); timer.cleared = false; - set_timer_ref_state(timer_id, true); } } diff --git a/crates/perry-runtime/src/timer/tests_inline.rs b/crates/perry-runtime/src/timer/tests_inline.rs index f365981d55..95587349bb 100644 --- a/crates/perry-runtime/src/timer/tests_inline.rs +++ b/crates/perry-runtime/src/timer/tests_inline.rs @@ -244,3 +244,74 @@ mod expired_batch_order_tests { ); } } + +#[cfg(test)] +mod refresh_and_immediate_primitive_tests { + use super::*; + + /// #10541: `refresh()` reschedules a timer but must not touch its ref + /// state -- neither re-ref an unref'd timer/interval nor unref a ref'd + /// one. Before the fix `js_timer_refresh` unconditionally called + /// `set_timer_ref_state(id, true)`. + #[test] + fn refresh_preserves_ref_state() { + let _serial = crate::gc::global_side_table_test_lock(); + test_clear_all_timer_scanner_roots(); + + let unrefd = js_set_timeout_callback(0, 50_000.0); + js_timer_unref(unrefd); + assert_eq!(js_timer_has_ref(unrefd), 0, "setup: unref() didn't take"); + js_timer_refresh(unrefd); + assert_eq!( + js_timer_has_ref(unrefd), + 0, + "refresh() re-ref'd an unref'd timeout" + ); + + let refd = js_set_timeout_callback(0, 50_000.0); + assert_eq!(js_timer_has_ref(refd), 1, "setup: new timer isn't ref'd"); + js_timer_refresh(refd); + assert_eq!( + js_timer_has_ref(refd), + 1, + "refresh() unref'd a ref'd timeout" + ); + + let unrefd_interval = setInterval(0, 50_000.0); + js_timer_unref(unrefd_interval); + js_timer_refresh(unrefd_interval); + assert_eq!( + js_timer_has_ref(unrefd_interval), + 0, + "refresh() re-ref'd an unref'd interval" + ); + + clearTimeout(unrefd); + clearTimeout(refd); + clearInterval(unrefd_interval); + } + + /// #10542: a `setImmediate` handle is distinguished from a + /// `setTimeout`/`setInterval` handle by kind, so `js_number_coerce` can + /// gate its Timeout-only numeric shortcut on it. + #[test] + fn immediate_kind_is_distinguished_from_timeout() { + let _serial = crate::gc::global_side_table_test_lock(); + test_clear_all_timer_scanner_roots(); + + let timeout = js_set_timeout_callback(0, 50_000.0); + let interval = setInterval(0, 50_000.0); + let immediate = js_set_immediate_callback(0); + + assert!(!is_immediate_timer_id(timeout), "setTimeout is a Timeout"); + assert!(!is_immediate_timer_id(interval), "setInterval is a Timeout"); + assert!( + is_immediate_timer_id(immediate), + "setImmediate is an Immediate" + ); + + clearTimeout(timeout); + clearInterval(interval); + clearImmediate(immediate); + } +} diff --git a/test-files/test_gap_10541_10542_timer_refresh_ref_immediate_primitive.ts b/test-files/test_gap_10541_10542_timer_refresh_ref_immediate_primitive.ts new file mode 100644 index 0000000000..dcd2934a46 --- /dev/null +++ b/test-files/test_gap_10541_10542_timer_refresh_ref_immediate_primitive.ts @@ -0,0 +1,97 @@ +// #10541 / #10542: `Timeout.refresh()` ref-state semantics, and +// `Immediate` vs `Timeout` numeric conversion. +// +// #10541: `refresh()` reschedules a timer using its original delay, but per +// Node does NOT touch ref/unref state. Perry's `js_timer_refresh` force-set +// the id ref'd (`set_timer_ref_state(id, true)`), so refreshing an unref'd +// timer re-ref'd it: `hasRef()` flipped to `true` and the (BUG-labelled) +// callback that had been deliberately detached from the event loop ran, +// keeping the process alive until it fired. +// +// #10542: Node's `Timeout` (setTimeout/setInterval) has a numeric +// conversion (`+t` is its internal id) but `Immediate` (setImmediate) does +// not (`+im` is `NaN`) -- Perry gave both handles the Timeout conversion. + +process.on("exit", () => console.log("exit")); + +function show(label: string, t: any): void { + const hasRef = typeof t.hasRef === "function" ? t.hasRef() : "missing"; + const ctor = t.constructor ? t.constructor.name : "missing"; + const primitive = +t; + console.log( + `${label}: hasRef=${hasRef} ctor=${ctor} typeof(+t)=${typeof primitive} isNaN(+t)=${Number.isNaN( + primitive, + )}`, + ); +} + +// --- #10541: refresh() must preserve ref state ------------------------------ + +// An unref'd timeout, refreshed: must stay unref'd. If the bug is present +// this callback runs (BUG) and keeps the process alive for ~1.2s. +const unrefTimeout = setTimeout( + () => console.log("BUG: unref'd refresh()'d timeout fired"), + 1200, +); +unrefTimeout.unref(); +show("unref'd timeout before refresh", unrefTimeout); +unrefTimeout.refresh(); +show("unref'd timeout after refresh", unrefTimeout); + +// An unref'd interval, refreshed: must stay unref'd. Cleared immediately +// (synchronously, before the event loop ever runs) so it never fires either +// way -- this only exercises the post-refresh() hasRef() state. +const unrefInterval = setInterval( + () => console.log("BUG: unref'd refresh()'d interval fired"), + 1200, +); +unrefInterval.unref(); +show("unref'd interval before refresh", unrefInterval); +unrefInterval.refresh(); +show("unref'd interval after refresh", unrefInterval); +clearInterval(unrefInterval); + +// A ref'd timeout, refreshed: must stay ref'd, and must still fire (proves +// refresh() itself -- the reschedule -- keeps working). +const refdTimeout = setTimeout( + () => console.log("ref'd refresh()'d timeout fired"), + 50, +); +show("ref'd timeout before refresh", refdTimeout); +refdTimeout.refresh(); +show("ref'd timeout after refresh", refdTimeout); + +// unref() then explicit ref() then refresh(): refresh() must not perturb an +// explicit re-ref either (guards a naive fix that always forces ref state +// to false instead of leaving it alone). +const reRefTimeout = setTimeout( + () => console.log("BUG: reRefTimeout should have been cleared"), + 5000, +); +reRefTimeout.unref(); +reRefTimeout.ref(); +show("reRef timeout after ref()", reRefTimeout); +reRefTimeout.refresh(); +show("reRef timeout after refresh", reRefTimeout); +clearTimeout(reRefTimeout); + +// --- #10542: Immediate has no numeric conversion; Timeout/Interval do ------- + +const immediate = setImmediate(() => + console.log("BUG: immediate should have been cleared"), +); +show("immediate", immediate); +console.log( + "Object.prototype.toString.call(immediate)", + Object.prototype.toString.call(immediate), +); + +const plainTimeout = setTimeout(() => {}, 5000); +show("plain timeout (unfired)", plainTimeout); + +// clearTimeout/clearImmediate must still work on the handles above. +clearTimeout(plainTimeout); +clearImmediate(immediate); +console.log("cleared plainTimeout and immediate"); + +console.log("main done"); From 5f26f9200b1587293b950be4953f143fd793c310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 13:08:42 +0000 Subject: [PATCH 2/2] changelog: add fragment for #10620 --- .../10620-timer-refresh-ref-and-immediate-primitive.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog.d/10620-timer-refresh-ref-and-immediate-primitive.md diff --git a/changelog.d/10620-timer-refresh-ref-and-immediate-primitive.md b/changelog.d/10620-timer-refresh-ref-and-immediate-primitive.md new file mode 100644 index 0000000000..35790bacd7 --- /dev/null +++ b/changelog.d/10620-timer-refresh-ref-and-immediate-primitive.md @@ -0,0 +1,7 @@ +**`timeout.refresh()` no longer re-refs an `unref()`'d timer, and `+setImmediate(...)` is `NaN` again** (#10541, #10542). + +`js_timer_refresh` called `set_timer_ref_state(id, true)` unconditionally after rescheduling both the Timeout and the Interval branch. Node's `Timeout.refresh()` resets the start time and reschedules using the original delay, but never touches ref state — an `unref()`'d timer that gets `refresh()`'d (a common idle-timeout/debounce pattern in HTTP agents, database pools and caches) is supposed to stay `unref()`'d. Perry's forced write meant `hasRef()` flipped back to `true` on refresh, so the timer kept the process alive and eventually ran a callback the program had deliberately detached from the event loop. The fix drops both forced writes; the id's ref-state entry (set at schedule time, updated by any `ref()`/`unref()` call since, and pinned by `ScheduledTimerId` while the timer is queued, per #10447) is left alone. + +`js_number_coerce`'s timer-handle numeric-conversion shortcut (`+t` → the handle's internal id) was gated only on `is_known_timer_id`, which is true for both `Timeout` and `Immediate` handles. Node gives `Timeout` (`setTimeout`/`setInterval`) a numeric conversion but not `Immediate` (`setImmediate`) — `+setImmediate(...)` is `NaN` in Node, a number in Perry. Added `crate::timer::is_immediate_timer_id`, a thin wrapper over the existing kind registry, and gated the shortcut on it; an `Immediate` now falls through to the pre-existing generic `toPrimitive`/`toString` path, which already stringifies to `"[object Object]"` and coerces to `NaN` — that path was simply unreachable for timer handles before. + +Validation: new gap test `test_gap_10541_10542_timer_refresh_ref_immediate_primitive` matches Node 26.5.1 byte for byte and reproduces both bugs on the pre-fix build (an unref'd timer re-refs after `refresh()` and its detached callback fires; `+immediate` is a number, not `NaN`). New `perry-runtime` unit tests cover `js_timer_refresh` preserving ref state for a Timeout, a ref'd Timeout, and an Interval, and `is_immediate_timer_id` distinguishing all three handle kinds. `RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --tests timer`: 32/32 pass. Instructions on a refresh()+`+timeout`-coercion churn loop (2,000,000 iterations): −0.34% vs the pre-fix build (both touched functions do slightly less work — one fewer ref-state write per `refresh()`, one added but cold kind check in `js_number_coerce`).