Keep the timer queue ordered when a timer is changed or disposed

CHANGELOG.md

@@ -7,6 +7,11 @@ and this project aims to follow [Semantic Versioning](https://semver.org/spec/v2
 
 ## [Unreleased]
 
+### Fixed
+- `SimulatedTimeProvider`: rescheduling a queued timer with `ITimer.Change` no longer corrupts the timer queue. Moving the earliest timer later, or stopping it with `Infinite`, hid every other due timer from `Advance` until that timer was reached, so `CancellationTokenSource.CancelAfter`, `TryReset` and `PeriodicTimer.Period` on a provider-made timer could stall unrelated delays and timeouts. Queue entries now carry an immutable `(due, id)` priority and a schedule version, and stale entries are discarded.
+- `SimulatedTimeProvider`: a timer disposed or rescheduled by an earlier callback of the same `Advance` no longer fires for that occurrence. A one-shot timer is spent when its callback runs rather than when `Advance` finds it due. If a callback throws, the one-shot timers that `Advance` found due and did not reach are spent, as before.
+- `SimulatedTimeProvider`: disposed timers and superseded schedules no longer accumulate in the timer queue behind a long-lived timer. The queue is rebuilt from its live entries whenever stale ones outnumber them, so it holds at most about twice the live timers, and a disposed or spent timer releases its callback and state.
+
 ## [1.4.0] - 2026-05-16
 
 ### Fixed

property-tests/SimulatedTimeProviderProperties.fs

@@ -192,3 +192,65 @@ let ``GetElapsedTime matches advanced time`` (advanceMs: uint16) =
 
     // Allow small rounding differences
     abs (elapsed.TotalMilliseconds - expectedElapsed.TotalMilliseconds) < 0.01
+
+/// Reference model for the timer queue: each advance fires every live timer that is due, once, in (due, id) order;
+/// a periodic timer is rescheduled one period after the advanced-to time; a disposed timer, or a one-shot timer
+/// that has fired, refuses Change. A due of None never comes. The queue never holds more than twice the most
+/// timers live at once, however many have been changed or disposed.
+type private ModelTimer = { Id: int; mutable Due: int64 option; mutable Period: int64; mutable Done: bool }
+
+/// Property: under any sequence of create, change, dispose and advance, timers fire as the reference model says
+[<Property(MaxTest = 500)>]
+let ``Timers fire as a reference model says under change and dispose`` (ops: (byte * byte * byte) list) =
+    let timeProvider = new SimulatedTimeProvider()
+    let timers = ResizeArray<System.Threading.ITimer>()
+    let model = ResizeArray<ModelTimer>()
+    let fired = ResizeArray<int>()
+    let mutable now = 0L
+    let mutable mostLive = 0
+    let mutable agrees = true
+
+    let dueOf (b: byte) = if b % 8uy = 7uy then None else Some (int64 (b % 50uy))
+    let periodOf (b: byte) = if b % 3uy = 0uy then int64 (b % 20uy) + 1L else 0L
+    let dueSpan due =
+        match due with
+        | Some ms -> TimeSpan.FromMilliseconds(float ms)
+        | None -> System.Threading.Timeout.InfiniteTimeSpan
+    let periodSpan period =
+        if period = 0L then System.Threading.Timeout.InfiniteTimeSpan else TimeSpan.FromMilliseconds(float period)
+
+    for (kind, a, b) in List.truncate 60 ops do
+        match kind % 4uy with
+        | 0uy ->
+            let id = model.Count
+            let due, period = dueOf a, periodOf b
+            model.Add({ Id = id; Due = due |> Option.map ((+) now); Period = period; Done = false })
+            timers.Add(timeProvider.CreateTimer((fun state -> fired.Add(unbox<int> state)), box id, dueSpan due, periodSpan period))
+            mostLive <- max mostLive (model |> Seq.filter (fun timer -> not timer.Done) |> Seq.length)
+        | 1uy when model.Count > 0 ->
+            let timer = model[int a % model.Count]
+            let due, period = dueOf b, periodOf (a ^^^ b)
+            let expected = not timer.Done
+            if expected then
+                timer.Due <- due |> Option.map ((+) now)
+                timer.Period <- period
+            agrees <- agrees && timers[timer.Id].Change(dueSpan due, periodSpan period) = expected
+        | 2uy when model.Count > 0 ->
+            let timer = model[int a % model.Count]
+            timer.Done <- true
+            timers[timer.Id].Dispose()
+        | _ ->
+            let by = int64 (a % 30uy)
+            now <- now + by
+            let due =
+                model
+                |> Seq.filter (fun timer -> not timer.Done && timer.Due |> Option.exists (fun d -> d <= now))
+                |> Seq.sortBy (fun timer -> timer.Due.Value, timer.Id)
+                |> Seq.toList
+            for timer in due do
+                if timer.Period > 0L then timer.Due <- Some (now + timer.Period) else timer.Done <- true
+            fired.Clear()
+            timeProvider.Advance(TimeSpan.FromMilliseconds(float by))
+            agrees <- agrees && List.ofSeq fired = (due |> List.map (fun timer -> timer.Id))
+
+    agrees && timeProvider.Statistics.MaxQueueLength <= 2L * int64 mostLive

src/SimulatedTimeProvider.cs

@@ -20,7 +20,15 @@ public sealed class SimulatedTimeProvider : TimeProvider
     private long _schedulerTicks;
 
     private long _nextId;
-    private readonly PriorityQueue<ScheduledTimer, ScheduledTimer> _queue;
+
+    // Each entry carries the timer's schedule version and an immutable (due, id) priority, so the heap order always
+    // holds. Change supersedes a timer's entry and Dispose abandons it, leaving the stale entry in place: Advance
+    // discards stale entries that reach the head, and the queue is rebuilt from live entries whenever stale ones
+    // outnumber them, so it never holds more than about twice the live timers.
+    private readonly PriorityQueue<(ScheduledTimer Timer, long Version), (long DueAtTicks, long Id)> _queue;
+
+    // Timers whose current entry is in the queue; every other queued entry is stale.
+    private int _queued;
 
     /// <summary>
     /// Gets lightweight counters that can be used to observe scheduling behavior during simulation.
@@ -38,7 +46,7 @@ public SimulatedTimeProvider(DateTimeOffset? startTime = null, TimeZoneInfo? loc
         _localTimeZone = localTimeZone ?? TimeZoneInfo.Utc;
 
         _schedulerTicks = 0;
-        _queue = new PriorityQueue<ScheduledTimer, ScheduledTimer>(new ScheduledTimerComparer());
+        _queue = new PriorityQueue<(ScheduledTimer Timer, long Version), (long DueAtTicks, long Id)>();
     }
 
     /// <summary>
@@ -135,60 +143,95 @@ public void Advance(TimeSpan by)
 
         Statistics.RecordAdvance(by);
 
-        List<(TimerCallback Callback, object? State)>? due = null;
+        List<(ScheduledTimer Timer, long Version)>? due = null;
 
         lock (_gate)
         {
             _utcNow = _utcNow.Add(by);
             _schedulerTicks += by.Ticks;
 
-            while (_queue.TryPeek(out var timer, out _))
+            while (_queue.TryPeek(out var entry, out var priority))
             {
-                if (timer.IsDisposed)
+                if (IsStale(entry))
                 {
                     _queue.Dequeue();
                     continue;
                 }
 
-                if (timer.DueAtTicks > _schedulerTicks)
+                if (priority.DueAtTicks > _schedulerTicks)
                 {
                     break;
                 }
 
                 _queue.Dequeue();
-
-                if (timer.IsDisposed)
-                {
-                    continue;
-                }
-
+                Unqueue(entry.Timer);
                 due ??= [];
-                due.Add((timer.Callback, timer.State));
+                due.Add(entry);
 
                 // Periodic timers: coalesce on jump; schedule next occurrence from "now".
-                if (timer.PeriodTicks > 0)
+                if (entry.Timer.PeriodTicks > 0)
                 {
-                    timer.DueAtTicks = _schedulerTicks + timer.PeriodTicks;
-                    _queue.Enqueue(timer, timer);
+                    entry.Timer.DueAtTicks = _schedulerTicks + entry.Timer.PeriodTicks;
+                    Enqueue(entry.Timer);
                     Statistics.RecordPeriodicReschedule(_queue.Count);
                 }
-                else
-                {
-                    timer.MarkDisposed();
-                }
             }
+
+            CompactIfMostlyStale();
         }
 
         if (due is null)
         {
             return;
         }
 
-        // Fire callbacks outside lock.
-        foreach (var (callback, state) in due)
+        // Fire callbacks outside lock. A callback may dispose or reschedule a timer that this advance has already
+        // found due; such a timer does not fire for this occurrence.
+        var next = 0;
+        try
         {
-            Statistics.RecordCallbackFired();
-            callback(state);
+            for (; next < due.Count; next++)
+            {
+                var (timer, version) = due[next];
+                TimerCallback? callback;
+                object? state;
+                lock (_gate)
+                {
+                    if (timer.IsDisposed || timer.Version != version)
+                    {
+                        continue;
+                    }
+
+                    callback = timer.Callback;
+                    state = timer.State;
+                    if (timer.PeriodTicks == 0)
+                    {
+                        timer.MarkDisposed();
+                    }
+                }
+
+                Statistics.RecordCallbackFired();
+                callback!(state);
+            }
+        }
+        finally
+        {
+            if (next < due.Count)
+            {
+                // A callback threw. The one-shot timers this advance found due and did not reach are spent, as they
+                // were when Advance marked them on collection; periodic ones keep their next occurrence.
+                lock (_gate)
+                {
+                    for (var i = next + 1; i < due.Count; i++)
+                    {
+                        var (timer, version) = due[i];
+                        if (timer.PeriodTicks == 0 && !timer.IsDisposed && timer.Version == version)
+                        {
+                            timer.MarkDisposed();
+                        }
+                    }
+                }
+            }
         }
     }
 
@@ -221,35 +264,54 @@ public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSp
             var periodTicks = period == Timeout.InfiniteTimeSpan ? 0 : period.Ticks;
 
             var timer = new ScheduledTimer(this, id, callback, state, dueTicks, periodTicks);
-            _queue.Enqueue(timer, timer);
+            Enqueue(timer);
             Statistics.RecordTimerCreated(_queue.Count);
             return timer;
         }
     }
 
-    private void Reschedule(ScheduledTimer timer)
+    // Queues the timer's current schedule. Callers hold _gate.
+    private void Enqueue(ScheduledTimer timer)
+    {
+        _queue.Enqueue((timer, timer.Version), (timer.DueAtTicks, timer.Id));
+        timer.Queued = true;
+        _queued++;
+    }
+
+    // The timer's queued entry has been dequeued, superseded or abandoned. Callers hold _gate.
+    private void Unqueue(ScheduledTimer timer)
     {
-        if (timer.IsDisposed)
+        if (timer.Queued)
         {
-            return;
+            timer.Queued = false;
+            _queued--;
         }
-
-        _queue.Enqueue(timer, timer);
-        Statistics.RecordQueueEnqueue(_queue.Count);
     }
 
-    private sealed class ScheduledTimerComparer : IComparer<ScheduledTimer>
+    // An entry is current only while its timer is queued at the entry's version. Callers hold _gate.
+    private static bool IsStale((ScheduledTimer Timer, long Version) entry) =>
+        !entry.Timer.Queued || entry.Version != entry.Timer.Version;
+
+    // Rebuilds the queue from its current entries once stale ones outnumber them, in O(n): each stale entry is
+    // dropped once, so the rebuilds cost O(1) per change or disposal amortized. Callers hold _gate.
+    private void CompactIfMostlyStale()
     {
-        public int Compare(ScheduledTimer? x, ScheduledTimer? y)
+        if (_queue.Count - _queued <= _queued)
         {
-            if (ReferenceEquals(x, y)) return 0;
-            if (x is null) return -1;
-            if (y is null) return 1;
+            return;
+        }
 
-            var cmp = x.DueAtTicks.CompareTo(y.DueAtTicks);
-            if (cmp != 0) return cmp;
-            return x.Id.CompareTo(y.Id);
+        var current = new List<((ScheduledTimer Timer, long Version) Entry, (long DueAtTicks, long Id) Priority)>(_queued);
+        foreach (var item in _queue.UnorderedItems)
+        {
+            if (!IsStale(item.Element))
+            {
+                current.Add(item);
+            }
         }
+
+        _queue.Clear();
+        _queue.EnqueueRange(current);
     }
 
     private sealed class ScheduledTimer : ITimer
@@ -278,13 +340,19 @@ public ScheduledTimer(
         /// </summary>
         public long Id { get; }
         /// <summary>
-        /// Gets the delegate to invoke when the timer fires.
+        /// Gets the delegate to invoke when the timer fires, or null once the timer is disposed or spent.
+        /// </summary>
+        public TimerCallback? Callback { get; private set; }
+        /// <summary>
+        /// Gets the state object passed to the timer callback, released with <see cref="Callback"/>.
         /// </summary>
-        public TimerCallback Callback { get; }
+        public object? State { get; private set; }
+
         /// <summary>
-        /// Gets the state object passed to the timer callback.
+        /// Gets or sets whether the timer's current entry is in the owner's queue. Read and written under the
+        /// owner's lock.
         /// </summary>
-        public object? State { get; }
+        public bool Queued { get; set; }
 
         /// <summary>
         /// Gets or sets the due time of the timer, in scheduler ticks.
@@ -295,6 +363,12 @@ public ScheduledTimer(
         /// </summary>
         public long PeriodTicks { get; set; }
 
+        /// <summary>
+        /// Gets the version of the timer's schedule, which <see cref="Change"/> increments. A queued entry with an
+        /// older version is stale. Read and written under the owner's lock.
+        /// </summary>
+        public long Version { get; private set; }
+
         /// <summary>
         /// Gets a value indicating whether this timer has been disposed.
         /// </summary>
@@ -332,8 +406,13 @@ public bool Change(TimeSpan dueTime, TimeSpan period)
                 DueAtTicks = dueTicks;
                 PeriodTicks = period == Timeout.InfiniteTimeSpan ? 0 : period.Ticks;
 
+                // Never reorder a queued entry: supersede it, and queue the new schedule.
+                _owner.Unqueue(this);
+                Version++;
                 _owner.Statistics.RecordTimerChange();
-                _owner.Reschedule(this);
+                _owner.CompactIfMostlyStale();
+                _owner.Enqueue(this);
+                _owner.Statistics.RecordQueueEnqueue(_owner._queue.Count);
                 return true;
             }
         }
@@ -343,10 +422,19 @@ public bool Change(TimeSpan dueTime, TimeSpan period)
         /// </summary>
         public void Dispose()
         {
-            if (Interlocked.Exchange(ref _disposed, 1) == 0)
+            lock (_owner._gate)
             {
-                _owner.Statistics.RecordTimerDisposed();
+                if (Interlocked.Exchange(ref _disposed, 1) != 0)
+                {
+                    return;
+                }
+
+                Release();
+                _owner.Unqueue(this);
+                _owner.CompactIfMostlyStale();
             }
+
+            _owner.Statistics.RecordTimerDisposed();
         }
 
         /// <summary>
@@ -358,12 +446,22 @@ public ValueTask DisposeAsync()
             return ValueTask.CompletedTask;
         }
 
+        // A one-shot timer that has fired, or been passed over by a throwing callback, is spent. Callers hold the
+        // owner's lock; the timer's entry is already out of the queue.
         internal void MarkDisposed()
         {
             if (Interlocked.Exchange(ref _disposed, 1) == 0)
             {
+                Release();
                 _owner.Statistics.RecordTimerDisposed();
             }
         }
+
+        // A disposed or spent timer drops its callback and state, so an entry still queued for it pins neither.
+        private void Release()
+        {
+            Callback = null;
+            State = null;
+        }
     }
 }