Skip to content

fix(QueryRunner): adjust retry logic to correctly account for maxRetries#782

Open
podrivo wants to merge 3 commits into
gamedig:masterfrom
podrivo:fix-maxretries
Open

fix(QueryRunner): adjust retry logic to correctly account for maxRetries#782
podrivo wants to merge 3 commits into
gamedig:masterfrom
podrivo:fix-maxretries

Conversation

@podrivo

@podrivo podrivo commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Closes #640

Problem

In v5 the maxAttempts option was renamed to maxRetries, but the underlying logic was never changed — so maxRetries still meant "total number of tries." This caused the issues reported in #640:

  1. The name is misleading. maxRetries: 1 performed a single try, when the name implies one initial try plus one retry.
  2. maxRetries: 0 was impossible. The value was read with ||, so 0 fell back to the default of 1. There was no way to request a single try with no retries.
  3. The error count was confusing. Failed all 3 attempts at maxRetries: 1 mixed two concepts: each try runs every candidate port (query/game/cached), so "attempts" = tries × ports. This multiplication is intended (givenPortOnly restricts to one port), but the message gave no way to tell the two apart.

Fix

  • Treat maxRetries as the number of retries after the first attempt (total tries per port = maxRetries + 1).
  • Read it with ?? instead of ||, so an explicit 0 is honored.
  • Clarify the failure message to report tries x ports.
// maxRetries is the number of retries *after* the first attempt, so the
// total number of tries per port is maxRetries + 1. Use ?? (not ||) so an
// explicit maxRetries of 0 (a single try, no retries) is honored.
const numRetries = userOptions.maxRetries ?? gameOptions.maxRetries ?? defaultOptions.maxRetries

const tries = Array.from({ length: numRetries + 1 }, (x, i) => i)

const attemptOrder = []
if (optionsCollection.noBreadthOrder) {
  attempts.forEach(attempt => tries.forEach(retry => attemptOrder.push({ attempt, retry })))
} else {
  tries.forEach(retry => attempts.forEach(attempt => attemptOrder.push({ attempt, retry })))
}

// ...

const err = new Error('Failed all ' + errors.length + ' attempts (' + tries.length + ' tries x ' + attempts.length + ' ports)')

Examples

Attempt counts (tries × candidate ports), measured by stubbing the per-attempt call to always fail:

Scenario Before After
maxRetries: 0, single port (givenPortOnly) 1 (0 coerced to 1) 1
maxRetries: 1, single port (givenPortOnly) 1 2
maxRetries: 1, two candidate ports 2 4
maxRetries: 3, single port 3 4

Error message now: Failed all 3 attempts (1 tries x 3 ports).

Compatibility

  • Multi-port behavior preserved: the candidate-port list, the port multiplication, and noBreadthOrder ordering are unchanged — only the number of tries changes.
  • Behavior change for explicit values: code passing maxRetries: N now performs N + 1 tries per port. This is the intended correction of the option's meaning, and is called out in the CHANGELOG. To get the old "exactly one try" behavior, pass maxRetries: 0 (now possible), optionally with givenPortOnly: true to also skip extra ports.
  • Default stays 1, now meaning "try once, then retry once."

Testing

  • npm run lint:check — no new errors (pre-existing errors in unrelated protocols/*.js are untouched).
  • Verified before/after attempt counts, the new error message, and noBreadthOrder ordering with a local harness that stubs the attempt call (no live server needed).

podrivo added 3 commits June 20, 2026 10:58
Updated the retry logic in QueryRunner to ensure that the total number of attempts includes the initial try. Changed the calculation of the number of retries to use the nullish coalescing operator (??) for better handling of explicit zero values. Updated related variable names for clarity.
…message format

Added a note to the CHANGELOG explaining that `maxRetries` now represents the number of retries after the first attempt, and that `maxRetries: 0` allows for a single try. Updated the error message in QueryRunner to provide clearer information on the number of tries and ports involved.
Updated the description of the `maxRetries` parameter to specify that it represents the number of retries after the first attempt, with `0` allowing for a single try. This aligns with recent changes in the retry logic and enhances user understanding.
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.

bug: maxRetries will always try at least twice

1 participant