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

[dexcompiler]

Problem

Two SimulatedTimeProvider timer defects in 1.4.0, both found while evaluating Clockworks as the test clock for SkyLark.

1. ITimer.Change corrupts the timer queue. ScheduledTimer.Change set DueAtTicks on a timer that was already in PriorityQueue<ScheduledTimer, ScheduledTimer>. That queue's comparer reads the field, and Change then queued a second copy. A timer moved later, or stopped with Infinite, kept its heap slot under the new key. It sat above earlier timers, so Advance stopped at it.

var time = new SimulatedTimeProvider();
var fired = new List<string>();
using var a = time.CreateTimer(_ => fired.Add("a"), null, TimeSpan.FromSeconds(10), Timeout.InfiniteTimeSpan);
using var b = time.CreateTimer(_ => fired.Add("b"), null, TimeSpan.FromSeconds(20), Timeout.InfiniteTimeSpan);
a.Change(TimeSpan.FromSeconds(25), Timeout.InfiniteTimeSpan);
time.Advance(TimeSpan.FromSeconds(20));  // expected [b]   1.4.0: []
time.Advance(TimeSpan.FromSeconds(5));   // expected [b,a] 1.4.0: [a,b]

Stopping the earliest timer (Change(Infinite, Infinite)) blocked every later timer for good. In one test, a later timer had fired 0 times after Advance(365 days). The BCL reaches Change through:

• CancellationTokenSource.CancelAfter and TryReset on a CancellationTokenSource(delay, provider);
• PeriodicTimer.Period.

For example: new CancellationTokenSource(1 s, time), then Task.Delay(2 s, time), then CancelAfter(5 s), then Advance(3 s). The delay is still pending.

2. A timer disposed by an earlier callback of the same Advance still fires. Advance collected the due callbacks under its lock and invoked them later without re-checking. One-shot timers were marked disposed at collection, so Statistics cannot see this.

Raised in review

3. Stale entries accumulated behind a long-lived timer. The first fix left a superseded (Change) or abandoned (Dispose) entry in the heap until it reached the head. Behind a long-lived timer, such as a heartbeat, it never did. Arming, rescheduling, firing and disposing 1,000 timers behind one periodic timer left 1,002 entries, each pinning its timer's callback and state.

The Change half was new in this PR: 1.4.0 moved the queued entry's key in place, which is defect 1, so it left no copy behind. The Dispose half was already in 1.4.0: a disposed timer stayed queued, pinning its state, until it reached the head.

4. A throwing callback orphaned the due one-shot timers after it. In 1.4.0 those timers were spent when Advance collected them. Once the first fix moved spending to firing time, they were left neither queued nor disposed. Change then returned true and re-armed a timer that had silently missed its occurrence.

Fix

• Queue entries are (timer, version) with an immutable (due, id) priority. Order is unchanged: due time, then creation order.
• Change increments the timer's version and queues a new entry. It never reorders a queued one.
• Advance discards stale and disposed entries as it reaches them.
• Each collected callback is re-checked under the lock just before it runs. A timer disposed or rescheduled since Advance found it due does not fire for that occurrence.
• A one-shot timer is now spent when its callback runs, not when it is collected. A reschedule by an earlier callback of the same advance therefore takes effect. Change after the callback has run still returns false, as before.
• Each timer records whether its current entry is queued, so the provider counts current entries in O(1). When stale entries outnumber current ones, the queue is rebuilt from the current entries in O(n). Each stale entry is dropped once, so the cost is O(1) amortized per change or disposal, and the queue holds at most about twice the live timers.
• A disposed or spent timer drops its callback and state, so a stale entry kept between rebuilds pins neither. Dispose now takes the provider's lock to unqueue the timer.
• If a callback throws, the one-shot timers that Advance found due and did not reach are spent, as in 1.4.0. Periodic ones keep their next occurrence.

Tests

• Regression tests in tests/SimulatedTimeProviderTests.cs:

  • the earliest timer moved later;
  • the earliest timer stopped with Infinite;
  • the BCL CancelAfter route;
  • dispose by an earlier callback in the same advance;
  • reschedule by an earlier callback in the same advance.

  All five fail on main and pass here.

• From review:

  Test	Checks	1.4.0	922ac86 (first fix)
  Rescheduled_timers_do_not_accumulate_behind_a_live_periodic_timer (disabled, far-future first schedule)	1,000 timers armed, rescheduled, fired and disposed behind a 1 s heartbeat; MaxQueueLength ≤ 4 and no payload reachable	pass	fail (1,002 entries)
  A_finished_timer_releases_its_state_while_a_stale_entry_for_it_is_queued (disposed, fired)	four live timers keep one stale entry queued; the finished timer's state is collectable	disposed fails	both fail
  A_throwing_callback_leaves_the_other_due_one_shot_timers_of_its_advance_spent	after a throw, a due one-shot refuses Change and never fires; a periodic one fires at its next occurrence	pass	fail

• The property in property-tests/SimulatedTimeProviderProperties.fs drives the provider and a naive reference model through random create, change, dispose and advance operations. It compares the fired order and Change's return values.

  • On main it is falsified within 103–129 cases (3 of 3 runs). FsCheck shrinks it to four operations: two timers due now, the first changed to periodic at the same due time, then Advance(0). The second timer does not fire.
  • It now also checks that MaxQueueLength never exceeds twice the most timers live at once. On 922ac86 this is falsified within 34–63 cases (3 of 3 runs). FsCheck shrinks it to one periodic timer changed twice: three entries for one live timer. The a73fbd8 commit message calls this "three creates"; that is wrong, and the case is one create and two changes.
  • Here it passes 500 cases.

• Mutation checks. Each mutant was killed:

  Mutant	Killed by
  no compaction	both accumulation cases and the queue bound
  Release() a no-op	both finished-timer cases
  no Release() in MarkDisposed	finished timer, fired
  no Release() in Dispose	finished timer, disposed
  throwing callback does not spend the remaining one-shots	the throwing-callback test

• The three review tests passed 20 of 20 repeated runs. Debug passes too.

Checks

On SDK 10.0.303 and 11.0.100-rc.1, following CI:

• dotnet build Clockworks.sln -c Release: 0 errors. The 4 warnings are existing NU1902/NU1903 package advisories in src and the integration demo, unchanged by this PR.
• dotnet test tests/Clockworks.Tests.csproj -c Release: 168/168.
• dotnet test property-tests/Clockworks.PropertyTests.fsproj -c Release: 77/77.

Each of the three new commits builds and passes on its own.

Not changed

• The package version. Releases bump it, as in "Prepare 1.4.0 release". This PR adds [Unreleased] changelog entries only.
• Change on a one-shot timer after it has fired still returns false. System.Threading.Timer would re-arm it.
• Periodic coalescing on a large Advance is unchanged.
• A callback that throws still stops its Advance, as in 1.4.0. The exception propagates, and the callbacks after it in that advance do not run. This is a separate decision for the maintainer, so this PR does not change it. One option is to run every due callback and then rethrow, as an AggregateException when several throw. On real timers, an unhandled exception in a System.Threading.Timer callback ends the process, so there is no BCL behaviour to follow.
• Disabled (Infinite) schedules are still queued, so the existing QueueEnqueues and MaxQueueLength statistics properties hold. Compaction bounds them with the rest.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-08T07:46:20.648330Z	922ac86	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 922ac865f0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Finalize due timers when a callback throws

When a callback throws, this foreach exits before later due entries are processed. Those later one-shot timers were already dequeued, but this change no longer marks them disposed until their individual iteration, leaving them live with no queue entry: future Advance calls cannot fire them, while Change unexpectedly still returns true. This occurs through an existing supported path such as Timeouts.CreateTimeout after its returned CTS is disposed, whose callback throws ObjectDisposedException; any later one-shot due in the same advance is orphaned.

Useful? React with 👍 / 👎.

[dexcompiler]

In-depth review of head 922ac865f0c2bf2109e572ff595ac0ff0575f896 against base/merge-base a57b92f236070c19892ac5f18685c1826f40411c.

One actionable finding: [P2] Bound stale-entry retention after moving timers earlier. Inline reproduction below. The immutable priorities fix ordering, but stale entries retain completed timers, callbacks and state behind any live earlier priority. In a differential probe with one periodic heartbeat and 1,000 disabled timers individually armed, fired and disposed, the base retained 0 completed payloads and 1 queue entry; this head retained 1,000 completed payloads and 1,001 queue entries after full GC. Recommend addressing this before merge.

Validation on .NET SDK 10.0.112, using an isolated archive of the exact head:

• Full Release solution build: passed, 0 errors. Existing package advisories remain; archive builds additionally warn about unavailable SourceLink metadata.
• Unit tests: 163/163 passed.
• Property tests: 77/77 passed, including the new 500-case model property.
• Read all four changed files and surrounding timer implementation, statistics, BCL integration tests, timeout helpers, documentation and CI configuration.
• GitHub build/test, documentation build and CodeQL checks passed on this head; docs deployment was skipped. gh pr checks --required reported no required checks.

The new property checks fired order and Change results, but does not check object retention or queue growth. Add a bounded-queue/GC regression with a persistent live timer while repeatedly moving other timers earlier.

Existing exception comment: independently reproduced the scenario in #85 (comment) against both base and head. A throwing first callback prevents the second callback on both revisions; that callback-loss behavior is pre-existing. This PR changes the abandoned one-shot's lifecycle: base reports 2 disposed timers and rejects its Change; head reports 1 disposed timer and accepts Change. Exception cleanup deserves an explicit policy/test, but I am not counting the pre-existing callback loss as a second newly introduced defect.

No repository source files were changed.

[dexcompiler]

[P2] Bound stale-entry retention when timers move earlier

Root-only cleanup leaves stale entries at their original immutable priority. Keep one 1-second periodic timer alive, then repeatedly create a disabled one-shot, Change it to 1 second, and Advance by 1 second. Every one-shot fires and is disposed, but its original long.MaxValue entry remains behind the heartbeat's next occurrence forever, strongly retaining its callback/state. After 1,000 iterations and full GC, this head retained all 1,000 payloads and had 1,001 queue entries; the base retained zero payloads and only the heartbeat entry. Long-lived simulations therefore grow memory with total completed timers rather than live timers, and heap operations grow with that accumulated history. Use bounded stale-entry compaction or removal/replacement of superseded entries; also avoid enqueuing disabled schedules where possible. Add a regression that keeps a live periodic timer throughout the workload.

Minimal workload (supply a fresh state payload per iteration to observe retention):

var tp = new SimulatedTimeProvider();
using var heartbeat = tp.CreateTimer(_ => { }, null,
    TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
for (var i = 0; i < 1000; i++)
{
    using var timer = tp.CreateTimer(_ => { }, new byte[16 * 1024],
        Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan);
    timer.Change(TimeSpan.FromSeconds(1), Timeout.InfiniteTimeSpan);
    tp.Advance(TimeSpan.FromSeconds(1));
}

The same retention mechanism applies to far-future finite schedules moved earlier; merely omitting Infinite entries does not bound that case.