From e2ccbcfaa3d72f779f4358ffb8688f9e1fa3754f Mon Sep 17 00:00:00 2001 From: ldm0 Date: Fri, 2 Oct 2026 21:45:48 +0800 Subject: [PATCH] fix(timers): enforce stored deadlines A one-millisecond timer could become runnable immediately after registration. Remove the early dispatch allowance from the shared scheduler so registered and rescheduled timers become runnable only at or after their actual monotonic deadline. All readiness queries use the same rule. Regression coverage checks registration-time and pre-deadline queries, exact-deadline dispatch through filtered and unfiltered paths, and interval rescheduling without advancing the new deadline. --- moli-renderer-v8/src/host/timers.rs | 56 +++--- moli-time/src/lib.rs | 2 +- moli-time/src/timers.rs | 259 +++++++++++++--------------- 3 files changed, 147 insertions(+), 170 deletions(-) diff --git a/moli-renderer-v8/src/host/timers.rs b/moli-renderer-v8/src/host/timers.rs index c019569cb9..ca64fe9e45 100644 --- a/moli-renderer-v8/src/host/timers.rs +++ b/moli-renderer-v8/src/host/timers.rs @@ -22,7 +22,7 @@ use crate::{ get_private_value, script_nonce_from_host_defined_options, }, }; -use moli_time::{TimerId, TimerReadyAllowance, TimerScheduler}; +use moli_time::{TimerId, TimerScheduler}; use moli_webapi_declare::WebApiObject; #[derive(WebApiObject)] @@ -211,7 +211,6 @@ impl ScheduledTimerTask { } } -const MIN_DELAY_TIMER_READY_EARLY_ALLOWANCE: Duration = Duration::from_millis(1); const TIMER_CALLBACK_WATCHDOG_TIMEOUT: Duration = Duration::from_secs(8); #[derive(Default)] @@ -469,18 +468,17 @@ impl HostTimeoutScheduler { ) else { return 0; }; - self.scheduler - .schedule_after( - ScheduledTimerTask { - callback: ScheduledTimerCallback::WindowWebIdl(callback), - owner, - is_interval, - extra_args, - }, - u64::from(delay_ms), - Instant::now(), - ) - .get() + let task = ScheduledTimerTask { + callback: ScheduledTimerCallback::WindowWebIdl(callback), + owner, + is_interval, + extra_args, + }; + let now = Instant::now(); + let id = self + .scheduler + .schedule_after(task, u64::from(delay_ms), now); + id.get() } pub(crate) fn queue_source_once_with_receiver<'s>( @@ -737,11 +735,12 @@ impl HostTimeoutScheduler { selection: RendererPageTimerSelection, ) -> HostTimeoutRunResult { let now_ms = crate::window_host::current_time_ms(); - let Some(timer) = self.scheduler.take_next_ready_matching( - Instant::now(), - min_delay_ready_allowance(), - |task| task.matches_selection(selection, now_ms), - ) else { + let Some(timer) = self + .scheduler + .take_next_ready_matching(Instant::now(), |task| { + task.matches_selection(selection, now_ms) + }) + else { return HostTimeoutRunResult::Idle; }; self.run_timer(scope, timer) @@ -749,8 +748,7 @@ impl HostTimeoutScheduler { #[cfg(test)] pub(crate) fn has_ready_timer(&self) -> bool { - self.scheduler - .has_ready_timer(Instant::now(), min_delay_ready_allowance()) + self.scheduler.has_ready_timer(Instant::now()) } pub(crate) fn next_ready_timer_deadline( @@ -758,11 +756,10 @@ impl HostTimeoutScheduler { selection: RendererPageTimerSelection, ) -> Option { let now_ms = crate::window_host::current_time_ms(); - self.scheduler.next_ready_deadline_matching( - Instant::now(), - min_delay_ready_allowance(), - |task| task.matches_selection(selection, now_ms), - ) + self.scheduler + .next_ready_deadline_matching(Instant::now(), |task| { + task.matches_selection(selection, now_ms) + }) } fn run_timer( @@ -1076,13 +1073,6 @@ fn timer_callback_relevant_context_is_current( }) } -fn min_delay_ready_allowance() -> TimerReadyAllowance { - TimerReadyAllowance { - max_delay_ms: 1, - allowance: MIN_DELAY_TIMER_READY_EARLY_ALLOWANCE, - } -} - fn run_window_timer_callback( scope: &mut v8::PinScope<'_, '_>, callback: &ScheduledTimerCallback, diff --git a/moli-time/src/lib.rs b/moli-time/src/lib.rs index b5cafac5f9..6cd8699d74 100644 --- a/moli-time/src/lib.rs +++ b/moli-time/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; mod timers; -pub use timers::{ReadyTimer, TimerId, TimerReadyAllowance, TimerScheduler}; +pub use timers::{ReadyTimer, TimerId, TimerScheduler}; pub fn unix_epoch_millis() -> f64 { SystemTime::now() diff --git a/moli-time/src/timers.rs b/moli-time/src/timers.rs index da0e3fe32f..76f45280b9 100644 --- a/moli-time/src/timers.rs +++ b/moli-time/src/timers.rs @@ -17,19 +17,6 @@ impl TimerId { } } -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct TimerReadyAllowance { - pub max_delay_ms: u32, - pub allowance: Duration, -} - -impl TimerReadyAllowance { - pub const NONE: Self = Self { - max_delay_ms: 0, - allowance: Duration::ZERO, - }; -} - #[derive(Debug)] pub struct ReadyTimer { pub id: TimerId, @@ -46,6 +33,12 @@ struct ScheduledTimer { payload: T, } +impl ScheduledTimer { + fn is_ready_at(&self, now: Instant) -> bool { + self.run_at <= now + } +} + impl Ord for ScheduledTimer { fn cmp(&self, other: &Self) -> Ordering { match self.run_at.cmp(&other.run_at).reverse() { @@ -69,6 +62,7 @@ impl PartialEq for ScheduledTimer { } } +/// Timers become runnable only at or after their stored monotonic deadline. #[derive(Debug)] pub struct TimerScheduler { pending: BinaryHeap>, @@ -137,18 +131,14 @@ impl TimerScheduler { .flatten() } - pub fn take_next_ready( - &mut self, - now: Instant, - allowance: TimerReadyAllowance, - ) -> Option> { + pub fn take_next_ready(&mut self, now: Instant) -> Option> { loop { let timer = self.pending.peek()?; if !self.active.contains(&timer.id) { let _ = self.pending.pop(); continue; } - if !timer_ready(timer.run_at, timer.delay_ms, now, allowance) { + if !timer.is_ready_at(now) { return None; } @@ -168,13 +158,12 @@ impl TimerScheduler { pub fn take_next_ready_matching( &mut self, now: Instant, - allowance: TimerReadyAllowance, predicate: F, ) -> Option> where F: FnMut(&T) -> bool, { - let selected = self.next_ready_matching_timer(now, allowance, predicate)?; + let selected = self.next_ready_matching_timer(now, predicate)?; let selected_id = selected.id; let selected_sequence = selected.sequence; @@ -203,19 +192,12 @@ impl TimerScheduler { }) } - pub fn has_ready_matching( - &self, - now: Instant, - allowance: TimerReadyAllowance, - mut predicate: F, - ) -> bool + pub fn has_ready_matching(&self, now: Instant, mut predicate: F) -> bool where F: FnMut(&T) -> bool, { self.pending.iter().any(|timer| { - self.active.contains(&timer.id) - && timer_ready(timer.run_at, timer.delay_ms, now, allowance) - && predicate(&timer.payload) + self.active.contains(&timer.id) && timer.is_ready_at(now) && predicate(&timer.payload) }) } @@ -239,23 +221,17 @@ impl TimerScheduler { true } - pub fn has_ready_timer(&self, now: Instant, allowance: TimerReadyAllowance) -> bool { - self.pending.iter().any(|timer| { - self.active.contains(&timer.id) - && timer_ready(timer.run_at, timer.delay_ms, now, allowance) - }) + pub fn has_ready_timer(&self, now: Instant) -> bool { + self.pending + .iter() + .any(|timer| self.active.contains(&timer.id) && timer.is_ready_at(now)) } - pub fn next_ready_deadline_matching( - &self, - now: Instant, - allowance: TimerReadyAllowance, - predicate: F, - ) -> Option + pub fn next_ready_deadline_matching(&self, now: Instant, predicate: F) -> Option where F: FnMut(&T) -> bool, { - self.next_ready_matching_timer(now, allowance, predicate) + self.next_ready_matching_timer(now, predicate) .map(|timer| timer.run_at) } @@ -309,22 +285,17 @@ impl TimerScheduler { fn next_ready_matching_timer( &self, now: Instant, - allowance: TimerReadyAllowance, mut predicate: F, ) -> Option<&ScheduledTimer> where F: FnMut(&T) -> bool, { - let mut first_non_ready = None; let mut selected = None; for timer in &self.pending { if !self.active.contains(&timer.id) { continue; } - if !timer_ready(timer.run_at, timer.delay_ms, now, allowance) { - if first_non_ready.is_none_or(|current| timer_precedes(timer, current)) { - first_non_ready = Some(timer); - } + if !timer.is_ready_at(now) { continue; } if predicate(&timer.payload) @@ -334,11 +305,7 @@ impl TimerScheduler { } } - let selected = selected?; - if first_non_ready.is_some_and(|barrier| timer_precedes(barrier, selected)) { - return None; - } - Some(selected) + selected } } @@ -350,21 +317,58 @@ fn timer_precedes(left: &ScheduledTimer, right: &ScheduledTimer) -> boo } } -fn timer_ready( - run_at: Instant, - delay_ms: u64, - now: Instant, - allowance: TimerReadyAllowance, -) -> bool { - run_at <= now - || (delay_ms <= u64::from(allowance.max_delay_ms) - && run_at.duration_since(now).le(&allowance.allowance)) -} - #[cfg(test)] mod tests { use super::*; + #[test] + fn timers_wait_until_their_deadline_in_every_ready_query() { + let now = Instant::now(); + for delay_ms in [1, 2, 100] { + for consume_matching in [false, true] { + let mut scheduler = TimerScheduler::default(); + let id = scheduler.schedule_after("timer", delay_ms, now); + let deadline = now + Duration::from_millis(delay_ms); + + for before_deadline in [now, deadline - Duration::from_nanos(1)] { + assert!(!scheduler.has_ready_timer(before_deadline)); + assert!(!scheduler.has_ready_matching(before_deadline, |_| true)); + assert_eq!( + scheduler.next_ready_deadline_matching(before_deadline, |_| true), + None + ); + assert!(scheduler.take_next_ready(before_deadline).is_none()); + assert!( + scheduler + .take_next_ready_matching(before_deadline, |_| true) + .is_none() + ); + assert_eq!(scheduler.next_deadline(), Some(deadline)); + assert!(scheduler.ms_to_next(before_deadline).unwrap() > 0); + assert_eq!(scheduler.pending_count(), 1); + } + + assert!(scheduler.has_ready_timer(deadline)); + assert!(scheduler.has_ready_matching(deadline, |_| true)); + assert_eq!( + scheduler.next_ready_deadline_matching(deadline, |_| true), + Some(deadline) + ); + assert_eq!(scheduler.ms_to_next(deadline), Some(0)); + let ready = if consume_matching { + scheduler.take_next_ready_matching(deadline, |_| true) + } else { + scheduler.take_next_ready(deadline) + } + .unwrap(); + assert_eq!(ready.id, id); + assert_eq!(ready.payload, "timer"); + scheduler.finish_running(id); + assert_eq!(scheduler.pending_count(), 0); + } + } + } + #[test] fn ready_timers_fire_by_deadline_then_sequence() { let now = Instant::now(); @@ -374,14 +378,14 @@ mod tests { let second = scheduler.schedule_after("second", 10, now); let ready = scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .expect("first timer should be ready"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .expect("second timer should be ready"); assert_eq!(ready.id, second); assert_eq!(ready.payload, "second"); @@ -389,12 +393,12 @@ mod tests { assert!( scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .is_none() ); let ready = scheduler - .take_next_ready(now + Duration::from_millis(20), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(20)) .expect("slow timer should be ready"); assert_eq!(ready.id, slow); assert_eq!(ready.payload, "slow"); @@ -411,15 +415,10 @@ mod tests { assert_eq!(scheduler.next_deadline(), Some(deadline)); assert!( scheduler - .take_next_ready( - deadline - Duration::from_millis(1), - TimerReadyAllowance::NONE - ) + .take_next_ready(deadline - Duration::from_millis(1)) .is_none() ); - let ready = scheduler - .take_next_ready(deadline, TimerReadyAllowance::NONE) - .unwrap(); + let ready = scheduler.take_next_ready(deadline).unwrap(); assert_eq!(ready.id, id); assert_eq!(ready.delay_ms, delay_ms); assert!(scheduler.reschedule_running_after(id, ready.payload, delay_ms, deadline)); @@ -442,16 +441,12 @@ mod tests { scheduler.cancel(cancelled); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("kept timer should be ready"); assert_eq!(ready.id, kept); assert_eq!(ready.payload, "kept"); scheduler.finish_running(ready.id); - assert!( - scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) - .is_none() - ); + assert!(scheduler.take_next_ready(now).is_none()); } #[test] @@ -461,7 +456,7 @@ mod tests { let interval = scheduler.schedule_after("tick", 0, now); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("interval should be ready"); scheduler.cancel(interval); assert!(!scheduler.reschedule_running_after( @@ -493,7 +488,7 @@ mod tests { assert_eq!(scheduler.pending_count(), 1); let ready = scheduler - .take_next_ready(now + Duration::from_millis(12), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(12)) .expect("kept timer should become ready"); assert_eq!(ready.id, kept); scheduler.finish_running(ready.id); @@ -509,40 +504,40 @@ mod tests { scheduler.cancel(cancelled); assert_eq!( - scheduler - .next_ready_deadline_matching(now, TimerReadyAllowance::NONE, |payload| *payload - == "selected",), + scheduler.next_ready_deadline_matching(now, |payload| *payload == "selected",), None ); assert_eq!( - scheduler.next_ready_deadline_matching( - now + Duration::from_millis(1), - TimerReadyAllowance::NONE, - |payload| *payload == "selected", - ), + scheduler + .next_ready_deadline_matching(now + Duration::from_millis(1), |payload| *payload + == "selected",), Some(now + Duration::from_millis(1)) ); } #[test] - fn matching_ready_deadline_respects_an_earlier_non_ready_barrier() { + fn matching_ready_deadline_does_not_admit_a_future_timer() { let now = Instant::now(); let mut scheduler = TimerScheduler::default(); - scheduler.schedule_after("barrier", 2, now); + scheduler.schedule_after("earlier", 2, now); scheduler.schedule_after("selected", 1, now + Duration::from_micros(1_500)); - let allowance = TimerReadyAllowance { - max_delay_ms: 1, - allowance: Duration::from_millis(1), - }; - + let before_selected = now + Duration::from_micros(2_499); + assert!(scheduler.has_ready_timer(before_selected)); assert_eq!( - scheduler.next_ready_deadline_matching( - now + Duration::from_micros(1_500), - allowance, - |payload| *payload == "selected", - ), - None, - "a later timer admitted by early allowance must not overtake the heap head" + scheduler + .next_ready_deadline_matching(before_selected, |payload| *payload == "selected"), + None + ); + assert!( + scheduler + .take_next_ready_matching(before_selected, |payload| *payload == "selected") + .is_none() + ); + assert_eq!( + scheduler.next_ready_deadline_matching(now + Duration::from_micros(2_500), |payload| { + *payload == "selected" + },), + Some(now + Duration::from_micros(2_500)) ); } @@ -564,25 +559,25 @@ mod tests { } #[test] - fn early_allowance_only_applies_to_short_delays() { + fn rescheduled_short_timer_waits_for_its_new_deadline() { let now = Instant::now(); let mut scheduler = TimerScheduler::default(); - let short = scheduler.schedule_after("short", 1, now); - scheduler.schedule_after("long", 2, now); + let id = scheduler.schedule_after("tick", 1, now); + let first_deadline = now + Duration::from_millis(1); + let ready = scheduler.take_next_ready(first_deadline).unwrap(); + assert!(scheduler.reschedule_running_after(id, ready.payload, 1, first_deadline)); - let allowance = TimerReadyAllowance { - max_delay_ms: 1, - allowance: Duration::from_millis(1), - }; - let just_before = now + Duration::from_micros(500); - assert!(scheduler.has_ready_timer(just_before, allowance)); - let ready = scheduler - .take_next_ready(just_before, allowance) - .expect("short timer should be ready within allowance"); - assert_eq!(ready.id, short); - scheduler.finish_running(ready.id); - - assert!(!scheduler.has_ready_timer(just_before, allowance)); + let next_deadline = first_deadline + Duration::from_millis(1); + for before_deadline in [first_deadline, next_deadline - Duration::from_nanos(1)] { + assert!(!scheduler.has_ready_timer(before_deadline)); + assert!(scheduler.take_next_ready(before_deadline).is_none()); + assert_eq!(scheduler.next_deadline(), Some(next_deadline)); + } + let ready = scheduler.take_next_ready(next_deadline).unwrap(); + assert_eq!(ready.id, id); + assert_eq!(ready.payload, "tick"); + scheduler.finish_running(id); + assert_eq!(scheduler.pending_count(), 0); } #[test] @@ -593,29 +588,23 @@ mod tests { let selected = scheduler.schedule_after("selected", 0, now); let second = scheduler.schedule_after("second", 0, now); - assert!( - scheduler.has_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) - ); + assert!(scheduler.has_ready_matching(now, |payload| { *payload == "selected" })); let ready = scheduler - .take_next_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) + .take_next_ready_matching(now, |payload| *payload == "selected") .expect("selected timer should be ready"); assert_eq!(ready.id, selected); assert_eq!(ready.payload, "selected"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("first timer should remain pending"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("second timer should remain pending"); assert_eq!(ready.id, second); assert_eq!(ready.payload, "second"); @@ -633,16 +622,14 @@ mod tests { let selected = scheduler.schedule_after("selected", 0, now); let ready = scheduler - .take_next_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) + .take_next_ready_matching(now, |payload| *payload == "selected") .expect("selected timer should be ready after skipped timers"); assert_eq!(ready.id, selected); assert_eq!(ready.payload, "selected"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("first timer should remain first after matching drain"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); @@ -650,7 +637,7 @@ mod tests { for skipped_id in skipped { let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("skipped timer should remain pending"); assert_eq!(ready.id, skipped_id); assert_eq!(ready.payload, "skipped");