Skip to content

cap ttlAutopurge timer delay at 2^31-1 ms - #417

Open
OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-autopurge-timeout-overflow
Open

OsamaAnsar wants to merge 1 commit into
isaacs:mainfrom
OsamaAnsar:fix-autopurge-timeout-overflow

Conversation

@OsamaAnsar

Copy link
Copy Markdown

With ttlAutopurge and a ttl over 2^31-1 ms (about 24.8 days), the purge timer was setTimeout(..., ttl + 1) with no cap. Node turns delays over 2^31-1 into 1 ms (with a TimeoutOverflowWarning). The timer fired immediately, found the item not stale, and set a new timer for the still-huge remaining time, which fired immediately too, and so on. An idle cache with one entry kept spinning.

Repro: one entry, ttlAutopurge: true, a counting wrapper around setTimeout, observed for 1 second on Node v24.19.0 (all numbers below are from that run):

before (7e71a1f)
ttl=86400000:   setTimeout calls in 1s = 1,   max delay requested = 86400001
ttl=2147483646: setTimeout calls in 1s = 1,   max delay requested = 2147483647
ttl=2147483647: setTimeout calls in 1s = 3,   max delay requested = 2147483648
ttl=2592000000: setTimeout calls in 1s = 163, max delay requested = 2592000001, 82 of them over 2^31-1
                (plus a TimeoutOverflowWarning for each)

after
ttl=86400000:   setTimeout calls in 1s = 1, max delay requested = 86400001
ttl=2147483646: setTimeout calls in 1s = 1, max delay requested = 2147483647
ttl=2147483647: setTimeout calls in 1s = 1, max delay requested = 2147483647
ttl=2592000000: setTimeout calls in 1s = 1, max delay requested = 2147483647

The fix caps the delay at 2^31-1. The existing "not stale yet, set the timer again for the time left" logic then does the rest. One edge needed care: with ttl === 2^31-1 the capped timer fires exactly when the age equals the ttl, which is not stale yet (> ttl) and has 0 ms remaining, and a remaining time of 0 sets no timer, so the entry would have stayed until the next access. It now sets the timer for 1 ms in that case.

I looked for other setTimeout calls that can get a huge delay. The only other one is the ttlResolution timer, whose delay is the user's resolution setting rather than a TTL, so I left it alone.

Tests are in the new test/ttl-autopurge-max-delay.ts (it has its own fake clock, since advancing the shared one in test/ttl.ts would shift the timestamps in the other snapshots). They cover a 30 day ttl (delay capped, one more timer for what remains, not purged early, purged just after the ttl, no timer loop), a ttl of exactly 2^31-1, and a ttl that fits, which is unchanged. With only the source change reverted, 7 assertions fail; with only the || 1 part reverted, 2 fail; with the fix, all 17 pass.

Checked on Node v24.19.0 on Windows: full tap -t0 run (coverage enforcement left on) passes with 0 failures, oxlint src test reports 0 warnings and 0 errors, and prettier --check is clean. I did not run Linux or macOS, other Node versions, or a browser. (On this machine I ran tap with a small preload that falls back to junctions for symlink, and installed @tapjs/clock with --no-save, see #413. Neither is part of this PR.)

setTimeout() treats delays over 2^31-1 ms (about 24.8 days) as 1ms, and
warns with TimeoutOverflowWarning. With ttlAutopurge and a longer ttl,
the purge timer fired right away, found that the item was not stale, and
set another timer for the same huge remaining time, over and over.

The delay is capped, and the existing logic that sets the timer again if
the item is not stale yet waits for the rest. A ttl of exactly 2^31-1
fires at the moment the item becomes stale (not before), so that case
sets the timer for 1ms more instead of for 0.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant