fix: correct critical bugs in retry logic

lib/index.js

@@ -17,7 +17,7 @@ function retry(fn, opts) {
     // would be futile (e.g.: auth errors)
 
     function bail(err) {
-      reject(err || new Error("Aborted"))
+      reject(err || new Error("Retry aborted by user"))
     }
 
     function onError(err, num) {

lib/retry.js

@@ -24,18 +24,43 @@ exports.timeouts = function (options) {
     ...options,
   }
 
+  // Fail fast on non-numeric timeout options: they would otherwise produce
+  // NaN timeouts that silently break setTimeout.
+  for (const key of ["factor", "minTimeout", "maxTimeout"]) {
+    const value = opts[key]
+    if (typeof value !== "number" || Number.isNaN(value)) {
+      throw new Error(`${key} must be a number`)
+    }
+  }
+
   if (opts.minTimeout > opts.maxTimeout) {
     throw new Error("minTimeout is greater than maxTimeout")
   }
 
   const timeouts = []
-  let i
-  for (i = 0; i < opts.retries; i++) {
-    timeouts.push(this.createTimeout(i, opts))
+
+  // `retries` may be null/undefined (default) or Infinity (forever mode, as
+  // set by exports.operation) — both generate a finite base set of timeouts
+  // that forever mode then cycles through.
+  let { retries } = opts
+  if (retries === null || retries === undefined || retries === Infinity) {
+    retries = 10
+  } else if (
+    typeof retries !== "number" ||
+    Number.isNaN(retries) ||
+    retries < 0
+  ) {
+    throw new Error(
+      "retries must be a non-negative number, Infinity, or null/undefined"
+    )
+  }
+
+  for (let i = 0; i < retries; i++) {
+    timeouts.push(exports.createTimeout(i, opts))
   }
 
   if (options && options.forever && !timeouts.length) {
-    timeouts.push(this.createTimeout(i, opts))
+    timeouts.push(exports.createTimeout(0, opts))
   }
 
   // sort the array numerically ascending
@@ -46,13 +71,13 @@ exports.timeouts = function (options) {
 
 exports.createTimeout = function (attempt, opts) {
   const random = opts.randomize ? Math.random() + 1 : 1
+  const baseTimeout = Math.max(opts.minTimeout, 1)
+  const exponential = Math.max(opts.factor ** attempt, 0)
 
-  let timeout = Math.round(
-    random * Math.max(opts.minTimeout, 1) * opts.factor ** attempt
-  )
+  let timeout = Math.round(random * baseTimeout * exponential)
   timeout = Math.min(timeout, opts.maxTimeout)
 
-  return timeout
+  return Math.max(timeout, 1)
 }
 
 exports.wrap = function (obj, options, methods) {

lib/retry_operation.js

@@ -18,9 +18,7 @@ function RetryOperation(timeouts, options) {
   this._timer = null
   this._onNewTimeoutCallback = options && options.onNewTimeout
 
-  if (this._options.forever) {
-    this._cachedTimeouts = this._timeouts.slice(0)
-  }
+  this._cachedTimeouts = this._options.forever ? this._timeouts.slice(0) : null
 }
 module.exports = RetryOperation
 
@@ -44,13 +42,24 @@ RetryOperation.prototype.stop = function () {
 RetryOperation.prototype.retry = function (err) {
   if (this._timeout) {
     clearTimeout(this._timeout)
+    this._timeout = null
+  }
+  if (this._timer) {
+    clearTimeout(this._timer)
+    this._timer = null
   }
 
   if (!err) {
     return false
   }
+
+  // Convert to Error if not already
+  if (!(err instanceof Error)) {
+    err = new Error(String(err))
+  }
+
   const currentTime = new Date().getTime()
-  if (err && currentTime - this._operationStart >= this._maxRetryTime) {
+  if (currentTime - this._operationStart >= this._maxRetryTime) {
     this._errors.push(err)
     this._errors.unshift(new Error("RetryOperation timeout occurred"))
     return false
@@ -117,7 +126,11 @@ RetryOperation.prototype.attempt = function (fn, timeoutOps) {
 
   this._operationStart = new Date().getTime()
 
-  this._fn(this._attempts)
+  try {
+    this._fn(this._attempts)
+  } catch (err) {
+    this.retry(err)
+  }
 }
 
 RetryOperation.prototype._onNewTimeout = function (timeout) {
@@ -157,7 +170,7 @@ RetryOperation.prototype.mainError = function () {
 
   for (let i = 0; i < this._errors.length; i++) {
     const error = this._errors[i]
-    const { message } = error
+    const message = error.message || String(error)
     const count = (counts[message] || 0) + 1
 
     counts[message] = count