fix: stop `nextAsync` `advanceUntilModeChanges ` from leaking past uninstall by SimenB · Pull Request #571 · sinonjs/fake-timers

SimenB · 2026-05-05T20:24:27Z

@fatso83 hey again! I chucked Claude at #562 and it believes it found it, plus the #564 error. Here's its summary:

Summary

Fixes the process-hang bug (#564) and the flaky nextAsync test failures (#562).

Bug 1 — infinite loop after `uninstall()` (#564)

pauseAutoTickUntilFinished saves the current tick mode and restores it in .finally() when the async operation settles. The problem: if clock.uninstall() runs before .finally() fires (which happens whenever a timer callback calls done() or resolves a test promise — causing the test to finish and afterEach to uninstall the clock while the un-awaited tickAsync is still in flight), the .finally() calls setTickMode({ mode: "nextAsync" }) on the now-dead clock. This starts a new advanceUntilModeChanges loop whose counter can never change, spinning forever and keeping the process alive after the suite exits.

Fix: move clock.uninstall into createClock so it can close over a private let uninstalled = false flag. Setting uninstalled = true at the top of clock.uninstall is enough: pauseAutoTickUntilFinished's .finally() checks !uninstalled before restoring nextAsync mode.

Bug 2 — flaky `runToLastAsync` assertion (#562)

Mocha inserts macrotask boundaries between beforeEach hooks and before running the test body. The outer beforeEach calls setTickMode({ mode: "nextAsync" }), starting advanceUntilModeChanges (AUMC). Each time Mocha yields to schedule the next hook or the test body, AUMC can complete another iteration and call clock.next(). If AUMC fires timer 4 (at T=5, which dynamically schedules timer 5 at T=6) before the test body reaches runToLastAsync(), then lastTimer returns T=6 — and runToLastAsync includes timer 5 in its run, producing [1,2,3,4,5] instead of the expected [1,2,3,4].

Fix: in the inner beforeEach, reset to "manual" mode before adding timers so setup is in a known state. Then re-enable "nextAsync" as the first synchronous line of each test body — before any await — so AUMC has not had a chance to advance the clock when the async method is called.

After clock.uninstall(), pauseAutoTickUntilFinished's .finally() could still call setTickMode({ mode: "nextAsync" }), starting a new advanceUntilModeChanges loop on the dead clock. Since nothing ever increments the counter on an uninstalled clock, the loop spins forever, keeping the Node process alive after the test suite completes. Set clock.uninstalled = true before setTickMode("manual") in uninstall(), and guard the restore in .finally() so it is a no-op on an uninstalled clock. Fixes: sinonjs#564

Two test issues addressed: 1. Add regression test for issue sinonjs#564: tickAsync() called without await while in nextAsync mode caused an infinite advanceUntilModeChanges loop after uninstall(). The test triggers this by not awaiting tickAsync() and relying on afterEach to call uninstall() while the tick is in flight. 2. Fix the flaky race in "works with manual calls to async tick functions". Mocha inserts macrotask boundaries between beforeEach hooks and the test body, giving the AUMC time to pre-fire timers before runToLastAsync/ nextAsync/tickAsync are called. If AUMC fired timer 4 (which dynamically schedules timer 5) before runToLastAsync captured lastTimer, the test would unexpectedly see [1,2,3,4,5] instead of [1,2,3,4]. Fix: reset to "manual" mode in the inner beforeEach so timer setup is deterministic, then re-enable "nextAsync" as the first synchronous line of each test body — before any await — so AUMC has not had a chance to advance the clock when the async method is called. Relates to: sinonjs#562

Copilot

Pull request overview

This PR addresses two issues in the nextAsync tick mode implementation: (1) a process hang caused by an infinite loop when a clock is uninstalled while an async tick is still in flight, and (2) flaky tests caused by auto-ticking advancing time during test setup.

Changes:

Prevent pauseAutoTickUntilFinished(...).finally(...) from re-enabling nextAsync after the clock has been uninstalled.
Mark the clock as uninstalled during uninstall() to support the above guard.
Stabilize setTickMode/nextAsync tests by pausing auto-ticking during timer setup and re-enabling it at the start of each test body, plus add a regression test for #564.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/fake-timers-src.js`	Adds an `uninstalled` flag and skips restoring `nextAsync` tick mode after uninstall to avoid infinite loops/hangs.
`test/fake-timers-test.js`	Adds a regression test for uninstall-during-tickAsync and makes `nextAsync`-mode async tick tests deterministic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

SimenB · 2026-05-05T20:47:08Z

Hmm, copilot's comment is a good spot. I wonder if we can move things around to avoid the new property

SimenB · 2026-05-05T20:56:39Z

+        /**
+         * @returns {Timer[]}
+         */
+        clock.uninstall = function () {


implementation is the same - just moved to close over uninstalled

SimenB · 2026-05-05T20:56:54Z

            clock.setTickMode({ mode: "manual" });
            return promise.finally(() => {
-                clock.setTickMode({ mode: "nextAsync" });
+                if (!uninstalled) {


this is the fix

SimenB · 2026-05-05T20:59:50Z

+            uninstalled = true;
+            clock.setTickMode({ mode: "manual" });


these two lines are new - otherwise the same as before

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fatso83 · 2026-05-05T21:09:29Z

Might want to do a rebase to catch any type regressions 😛

SimenB · 2026-05-05T21:13:10Z

merged in main. still passing 🥳

fatso83 · 2026-05-05T21:18:08Z

Two biggos in one evening. Nais.

fatso83 · 2026-05-05T22:20:08Z

I think a test broke that this was supposed to fix?

  1) FakeTimers
       setTickMode
         nextAsync
           works with manual calls to async tick functions
             runToLastAsync:

      Error: [assert.equals] [ 1, 2, 3, 4, 5 ] expected to be equal to [ 1, 2, 3, 4 ]
      + expected - actual

         1
         2
         3
         4
      -  5
       ]

https://github.com/sinonjs/fake-timers/actions/runs/25405330189/job/74514771134

SimenB added 2 commits May 5, 2026 22:24

Copilot AI review requested due to automatic review settings May 5, 2026 20:24

Copilot started reviewing on behalf of SimenB May 5, 2026 20:25 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Comment thread src/fake-timers-src.js Outdated

SimenB changed the title ~~Maybe fix some stuff~~ fix: stop nextAsync advanceUntilModeChanges from leaking past uninstall May 5, 2026

do not assign to clock instance

b5e70e4

SimenB commented May 5, 2026

View reviewed changes

SimenB requested a review from Copilot May 5, 2026 21:05

Copilot started reviewing on behalf of SimenB May 5, 2026 21:05 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

Merge branch 'main' into maybe-fix-some-stuff

235090c

This was linked to issues May 5, 2026

Under "nextAsync" tick mode, using tickAsync() without awaiting can cause Mocha process to hang after suite completion #564

Closed

Race condition bug in nextAsync code? #562

Open

fatso83 approved these changes May 5, 2026

View reviewed changes

fatso83 merged commit 86a5a20 into sinonjs:main May 5, 2026
13 checks passed

SimenB deleted the maybe-fix-some-stuff branch May 5, 2026 21:29

SimenB mentioned this pull request May 5, 2026

test: fix runToLastAsync flake by avoiding AUMC race in nextAsync mode #575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: stop `nextAsync` `advanceUntilModeChanges` from leaking past uninstall#571

fix: stop `nextAsync` `advanceUntilModeChanges` from leaking past uninstall#571
fatso83 merged 4 commits intosinonjs:mainfrom
SimenB:maybe-fix-some-stuff

SimenB commented May 5, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

SimenB commented May 5, 2026

Uh oh!

SimenB May 5, 2026

Uh oh!

SimenB May 5, 2026

Uh oh!

SimenB May 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

fatso83 commented May 5, 2026

Uh oh!

SimenB commented May 5, 2026

Uh oh!

Uh oh!

fatso83 commented May 5, 2026

Uh oh!

fatso83 commented May 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SimenB commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug 1 — infinite loop after uninstall() (#564)

Bug 2 — flaky runToLastAsync assertion (#562)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

SimenB commented May 5, 2026

Uh oh!

SimenB May 5, 2026

Choose a reason for hiding this comment

Uh oh!

SimenB May 5, 2026

Choose a reason for hiding this comment

Uh oh!

SimenB May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

fatso83 commented May 5, 2026

Uh oh!

SimenB commented May 5, 2026

Uh oh!

Uh oh!

fatso83 commented May 5, 2026

Uh oh!

fatso83 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SimenB commented May 5, 2026 •

edited

Loading

Bug 1 — infinite loop after `uninstall()` (#564)

Bug 2 — flaky `runToLastAsync` assertion (#562)

fatso83 commented May 5, 2026 •

edited

Loading