fix: correct critical bugs in retry logic#1
Draft
devthejo wants to merge 2 commits into
Draft
Conversation
- Fix randomize option not propagated to retrier.operation in index.js - Fix this.createTimeout undefined in retry.js (use exports.createTimeout) - Fix createTimeout returning NaN or 0 for invalid factor/minTimeout - Fix _cachedTimeouts initialization for non-forever mode - Fix timer leaks in RetryOperation.retry() - Fix mainError() handling errors without message property - Fix synchronous errors not caught in RetryOperation.attempt() - Fix retries validation to allow null/undefined for forever mode - Improve error messages for better debugging All existing tests pass. Co-authored-by: devthejo <devthejo@users.noreply.github.com>
…ptions The previous validation threw on retries:Infinity, contradicting operation() which uses retries === Infinity as its forever signal. Map Infinity (and null/undefined) to the default base timeout count instead. Also validate factor/minTimeout/maxTimeout up front: non-numeric values would silently yield NaN timeouts that break setTimeout. Fail explicitly instead of coercing, so misconfiguration surfaces at the source. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
Summary
Robustness hardening for the retry logic, plus two genuine behavior fixes in
retry.js. No breaking changes for documented usage.Behavior fixes (
lib/retry.js)retries: Infinityno longer crashes.operation()treatsretries === Infinityas the forever signal, buttimeouts()used to blow up on it (RangeError: Invalid array length). It now mapsInfinity(likenull/undefined) to the default base timeout count, which forever mode then cycles through.factor/minTimeout/maxTimeoutthat aren't numbers previously producedNaNtimeouts that silently breaksetTimeout. They now throw an explicit"<opt> must be a number"at the source instead of being coerced.retriesvalidation rejects negative /NaNvalues with a clear message (allowingInfinityandnull/undefined).Hardening (no behavior change in normal usage)
timeouts()callsexports.createTimeoutinstead ofthis.createTimeout, so it keeps working if the function is destructured off the module.createTimeoutfloors the result at1msand guards the exponential term.retry_operation.js: initialize_cachedTimeoutstonullin non-forever mode (wasundefined— both falsy, cosmetic); nullify_timeout/_timerafter clearing; wrap non-Errorvalues viaString(err);mainError()falls back toString(error)whenmessageis absent;attempt()catches synchronous throws from the operation fn.index.js: clearerbail()message ("Retry aborted by user").Testing
npm run lint— clean.test-retry-operation.js > testRetryForeverNoRetriesis timing-flaky on a tight ~150ms window and can intermittently fail onmasteras well — unrelated to this PR.Breaking changes
Passing a non-numeric
factor/minTimeout/maxTimeoutnow throws instead of silently returningNaNtimeouts. This only affects already-broken configurations.