fix: clamp setTimeout value to prevent negative timeout crash#39273
Open
veeceey wants to merge 2 commits intomicrosoft:mainfrom
Open
fix: clamp setTimeout value to prevent negative timeout crash#39273veeceey wants to merge 2 commits intomicrosoft:mainfrom
veeceey wants to merge 2 commits intomicrosoft:mainfrom
Conversation
When a Node.js process has been running for more than ~24.8 days, monotonicTime() exceeds 2^31-1 ms. This causes the computed timeout in raceAgainstDeadline() to become negative, which Node.js coerces to 1ms, resulting in an immediate spurious timeout. Clamp the timeout to [0, 2^31-1] to prevent both negative values and values exceeding setTimeout's maximum safe delay. Fixes microsoft#39166
| new Promise<{ timedOut: true }>(resolve => { | ||
| const kMaxDeadline = 2147483647; // 2^31-1 | ||
| const timeout = (deadline || kMaxDeadline) - monotonicTime(); | ||
| const rawTimeout = (deadline || kMaxDeadline) - monotonicTime(); |
Member
There was a problem hiding this comment.
It is better to not set a timer at all when there is no deadline, here and in other places across the code.
Author
There was a problem hiding this comment.
Good call -- updated to skip the timer entirely when there's no deadline instead of clamping. If deadline is 0, the promise executor returns early so no setTimeout is created at all. The race then just waits on the callback.
Instead of falling back to kMaxDeadline and clamping, just return early from the promise executor when deadline is 0. This avoids the negative timeout issue altogether by not creating a timer when none is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a Node.js process has been running for more than ~24.8 days (2^31-1 milliseconds),
monotonicTime()exceeds the max signed 32-bit integer. InraceAgainstDeadline(), the timeout is computed as:When
monotonicTime()exceedskMaxDeadline(e.g. no explicit timeout was set, sodeadlineis0and falls back tokMaxDeadline), this produces a negative value. Node.js then coerces it to 1ms and fires the timer immediately, causing operations likebrowserType.connect()to fail with:Fix
Clamp the computed timeout to
[0, kMaxDeadline]:0prevents negative values that Node.js coerces to 1mskMaxDeadline(2^31-1) keeps it within setTimeout's valid rangeHow I verified this
Reproduced the issue by overriding
performance.nowto return a value > 2^31-1, confirmingconnect()fails before the fix and succeeds after.Fixes #39166