From b60cff9344bac90c5bfc42c1739ddbe9c80723c9 Mon Sep 17 00:00:00 2001 From: Aditya Jain Date: Thu, 20 Aug 2026 19:32:59 -0700 Subject: [PATCH] fix(toolkit-lib): clean up the loser of noOlderThan()'s timer/resolver race fixes #1869 BackgroundStackRefresh.noOlderThan() raced a "wait for refresh" promise against a "reject after ms" setTimeout via Promise.race(), without cleaning up whichever side lost: - When the refresh landed first (the common case under cdk gc's polling loop), the losing setTimeout handle was never captured or cleared, keeping the event loop alive and its reject closure retained until it eventually fired on its own. - When the timeout won instead, the resolve callback already pushed into queuedPromises for the losing branch was never removed. Only justRefreshedStacks() drains that array, so the stale resolver sat there forever -- under many concurrent timed-out noOlderThan() calls, queuedPromises grows without bound. Restructure noOlderThan() around a single Promise executor that clears the timeout on a successful refresh, and removes its own entry from queuedPromises on a timeout, so the loser is always cleaned up regardless of which side wins. Added three regression tests: the timeout handle is cleared once a refresh lands, a single timed-out call leaves no dangling entry in queuedPromises, and 25 concurrent timed-out calls don't accumulate entries either. Co-Authored-By: Claude Sonnet 5 --- .../api/garbage-collection/stack-refresh.ts | 30 +++++++++--- .../garbage-collection.test.ts | 49 +++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/garbage-collection/stack-refresh.ts b/packages/@aws-cdk/toolkit-lib/lib/api/garbage-collection/stack-refresh.ts index 6cb2a3d5e..99e103169 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/garbage-collection/stack-refresh.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/garbage-collection/stack-refresh.ts @@ -202,12 +202,30 @@ export class BackgroundStackRefresh { return Promise.resolve(); } - // The last refresh happened earlier than the time frame - // We will wait for the latest refresh to land or reject if it takes too long - return Promise.race([ - new Promise(resolve => this.queuedPromises.push(resolve)), - new Promise((_, reject) => setTimeout(() => reject(new ToolkitError('StackRefreshTimeout', 'refreshStacks took too long; the background thread likely threw an error')), ms)), - ]); + // The last refresh happened earlier than the time frame. + // We will wait for the latest refresh to land or reject if it takes too long. + // + // Whichever side wins, we must clean up after the loser: an uncleared timeout + // handle keeps the process event loop alive and its reject closure retained + // until it eventually fires, and a resolve callback left behind in + // queuedPromises after a timeout would sit there forever (justRefreshedStacks() + // only ever drains it, so on a `cdk gc` run with many concurrent timed-out + // callers this array would otherwise grow without bound). + return new Promise((resolve, reject) => { + const onRefresh = () => { + clearTimeout(timeoutHandle); + resolve(undefined); + }; + this.queuedPromises.push(onRefresh); + + const timeoutHandle = setTimeout(() => { + const index = this.queuedPromises.indexOf(onRefresh); + if (index !== -1) { + this.queuedPromises.splice(index, 1); + } + reject(new ToolkitError('StackRefreshTimeout', 'refreshStacks took too long; the background thread likely threw an error')); + }, ms); + }); } public stop() { diff --git a/packages/@aws-cdk/toolkit-lib/test/api/garbage-collection/garbage-collection.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/garbage-collection/garbage-collection.test.ts index de088dec6..6b2d769b2 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/garbage-collection/garbage-collection.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/garbage-collection/garbage-collection.test.ts @@ -1064,6 +1064,55 @@ describe('BackgroundStackRefresh', () => { expect(setTimeoutSpy).not.toHaveBeenCalled(); expect(jest.getTimerCount()).toBe(0); }); + + test('noOlderThan() clears its timeout handle once the refresh lands (no leaked timer)', async () => { + void backgroundRefresh.start(); + await jest.runOnlyPendingTimersAsync(); // first refresh lands; lastRefreshTime = T0, next refresh scheduled for T0+300000 + + jest.advanceTimersByTime(299000); // T0+299000: 1s before the next background refresh fires + + const timerCountBefore = jest.getTimerCount(); + + // 100s is shorter than the 299s elapsed since the last refresh, so this must take + // the "wait for it" branch rather than resolving immediately -- and it's much + // longer than the 1s until the next background refresh, so that refresh (not + // this call's own timeout) is what resolves it. + const waitPromise = backgroundRefresh.noOlderThan(100000); + // A new timer was armed for this call's timeout race. + expect(jest.getTimerCount()).toBeGreaterThan(timerCountBefore); + + jest.advanceTimersByTime(1000); // T0+300000: the next background refresh lands and resolves us + await expect(waitPromise).resolves.toBeUndefined(); + + // The timeout side of the race must have been cleared, not left pending. + expect(jest.getTimerCount()).toBe(timerCountBefore); + }); + + test('noOlderThan() does not leave a dangling entry in queuedPromises after it times out', async () => { + void backgroundRefresh.start(); + await jest.runOnlyPendingTimersAsync(); + jest.advanceTimersByTime(120000); + + const waitPromise = backgroundRefresh.noOlderThan(0); + jest.advanceTimersByTime(120000); + await expect(waitPromise).rejects.toThrow('refreshStacks took too long; the background thread likely threw an error'); + + // A stale resolver left behind here would sit in the queue forever (only + // justRefreshedStacks() drains it), growing unboundedly under repeated timeouts. + expect((backgroundRefresh as any).queuedPromises).toHaveLength(0); + }); + + test('many concurrent noOlderThan() timeouts do not accumulate in queuedPromises', async () => { + void backgroundRefresh.start(); + await jest.runOnlyPendingTimersAsync(); + jest.advanceTimersByTime(120000); + + const waitPromises = Array.from({ length: 25 }, () => backgroundRefresh.noOlderThan(0)); + jest.advanceTimersByTime(120000); + await Promise.allSettled(waitPromises); + + expect((backgroundRefresh as any).queuedPromises).toHaveLength(0); + }); }); describe('ProgressPrinter', () => {