fix(runtime): timeout.refresh() preserves ref state; Immediate has no numeric conversion

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`).

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;
         }

crates/perry-runtime/src/timer.rs

@@ -634,6 +634,17 @@ pub(crate) fn timer_constructor_value(id: i64) -> Option<f64> {
 
 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,14 +912,21 @@ 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();
 
     {
         let mut timers = CALLBACK_TIMERS.lock().unwrap();
         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);
     }
 }
 

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);
+    }
+}

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");