fix: settle commands stashed on reconnect when the client never becomes ready

[askalf]

Summary

• flushQueue() walks offlineQueue and commandQueue only. Commands that were in flight when a ready connection dropped are moved to a third queue, prevCommandQueue (lib/redis/event_handler.ts:379-381), and only readyHandler drains that stash (lib/redis/event_handler.ts:509-532).
• connectHandler calls resetCommandQueue(), so once a reconnect attempt starts, the stash is unreachable from every path except reaching "ready" again. A client that never gets there leaves those user command promises pending forever.
• Four real exits hit this: disconnect() mid-reconnect, retryStrategy returning a non-number, a connector failure on the reconnect attempt, and the maxRetriesPerRequest flush — the last of which README:893 documents as "all pending commands will be flushed with an error every 20 retry attempts. That makes sure commands won't wait forever when the connection is down." The stashed commands do wait forever, so this is also docs-vs-code drift.
• Fix: drain prevCommandQueue inside the existing if (options.commandQueue) block of flushQueue, rejecting with the same error as the rest of that queue, and declare the field that until now was only ever set untyped from event_handler. 12 lines, one file.
• Direct follow-on to merged PR fix: reject unfulfilled commands dropped on reconnect #2194 (d95d05a, the base commit here), whose own commit message states "flushQueue only walks offlineQueue and commandQueue, so nothing could settle those promises afterwards" — that statement is still true for every exit other than the autoResendUnfulfilledCommands: false case fix: reject unfulfilled commands dropped on reconnect #2194 fixed.

$ # BASE — lib/Redis.ts at d95d05a, current 7-test file applied
$ git checkout d95d05a -- lib/Redis.ts
$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/unfulfilledCommands.ts"

  unfulfilled commands of a client that never becomes ready again (RESP3)
    1) rejects them when the user disconnects mid-reconnect
    2) rejects them when the retry strategy gives up
    3) rejects them once maxRetriesPerRequest is reached
    ✔ (control) still resends them when the reconnect succeeds (66ms)
    4) rejects every stashed command, not just the first
    5) rejects them when the reconnect hits a fatal protocol error
    6) rejects a command stashed after an earlier resend emptied the stash

  unfulfilled commands of a client that never becomes ready again (RESP2)
    7) rejects them when the user disconnects mid-reconnect
    8) rejects them when the retry strategy gives up
    9) rejects them once maxRetriesPerRequest is reached
    ✔ (control) still resends them when the reconnect succeeds (62ms)
    10) rejects every stashed command, not just the first
    11) rejects them when the reconnect hits a fatal protocol error
    12) rejects a command stashed after an earlier resend emptied the stash


  2 passing (9s)
  12 failing

  1) unfulfilled commands of a client that never becomes ready again (RESP3)
       rejects them when the user disconnects mid-reconnect:

      AssertionError: expected 'pending' to equal 'rejected: Connection is closed.'
      + expected - actual

      -pending
      +rejected: Connection is closed.

      at Context.<anonymous> (test/unit/unfulfilledCommands.ts:105:37)

  10) unfulfilled commands of a client that never becomes ready again (RESP2)
       rejects every stashed command, not just the first:

      AssertionError: expected [ 'pending', 'pending', 'pending' ] to deeply equal [ …(3) ]
      + expected - actual

       [
      -  "pending"
      -  "pending"
      -  "pending"
      +  "rejected: Connection is closed."
      +  "rejected: Connection is closed."
      +  "rejected: Connection is closed."
       ]

      at Context.<anonymous> (test/unit/unfulfilledCommands.ts:229:59)

  11) unfulfilled commands of a client that never becomes ready again (RESP2)
       rejects them when the reconnect hits a fatal protocol error:
     Error: timed out waiting for the stashed command to settle
      at waitFor (test/unit/unfulfilledCommands.ts:65:13)
      at async Context.<anonymous> (test/unit/unfulfilledCommands.ts:272:7)

  12) unfulfilled commands of a client that never becomes ready again (RESP2)
       rejects a command stashed after an earlier resend emptied the stash:

      AssertionError: expected 'pending' to equal 'rejected: Connection is closed.'
      + expected - actual

      -pending
      +rejected: Connection is closed.

      at Context.<anonymous> (test/unit/unfulfilledCommands.ts:343:37)

$ # FIXED — head c542f30
$ git checkout HEAD -- lib/Redis.ts
$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/unfulfilledCommands.ts"

  unfulfilled commands of a client that never becomes ready again (RESP3)
    ✔ rejects them when the user disconnects mid-reconnect (158ms)
    ✔ rejects them when the retry strategy gives up (88ms)
    ✔ rejects them once maxRetriesPerRequest is reached (69ms)
    ✔ (control) still resends them when the reconnect succeeds (65ms)
    ✔ rejects every stashed command, not just the first (112ms)
    ✔ rejects them when the reconnect hits a fatal protocol error (64ms)
    ✔ rejects a command stashed after an earlier resend emptied the stash (167ms)

  unfulfilled commands of a client that never becomes ready again (RESP2)
    ✔ rejects them when the user disconnects mid-reconnect (109ms)
    ✔ rejects them when the retry strategy gives up (84ms)
    ✔ rejects them once maxRetriesPerRequest is reached (65ms)
    ✔ (control) still resends them when the reconnect succeeds (65ms)
    ✔ rejects every stashed command, not just the first (104ms)
    ✔ rejects them when the reconnect hits a fatal protocol error (64ms)
    ✔ rejects a command stashed after an earlier resend emptied the stash (164ms)


  14 passing (1s)

Decisions

lib/Redis.ts, 12 lines: declare private prevCommandQueue: Deque<CommandItem> | null = null; next to the existing offlineQueue field, and in flushQueue()'s options.commandQueue block, after draining commandQueue, drain prevCommandQueue the same way (same error, same while ((item = q.shift())) loop), then null it out.

Why this is minimal and correct:

• Same block, same error, same idiom. The stash is conceptually part of the command queue — it is the previous command queue — so it belongs under options.commandQueue, rejected with the same error and the same loop shape the two queues above it already use. No new option, no new error type, no signature change.
• It cannot pre-empt the resend path. readyHandler is the only other consumer and runs on "ready". flushQueue runs when the client is giving up on this connection attempt or ending. If a reconnect succeeds, readyHandler drains the stash first and flushQueue is not in that path — pinned by a control test.
• Double-settling is unreachable. readyHandler's abort branch nulls prevCommandQueue after rejecting; its resend branch shifts every item out. Either way the deque flushQueue can later see is empty or null.
• The field declaration is not a drive-by. prevCommandQueue was assigned from event_handler.ts (which types self as any) and never declared on the class, so this.prevCommandQueue would not type-check in Redis.ts without it.

Alternatives rejected:

• Drain it in connectHandler / resetCommandQueue() — would reject on every reconnect attempt, destroying autoResendUnfulfilledCommands, the whole point of the stash surviving the attempt.
• Merge the stash back into commandQueue on close — the two queues have different semantics (one is resent on ready, one is not); merging would change resend behaviour for everyone.
• Fix only the maxRetriesPerRequest call site — the same leak exists at four other call sites; fixing it in flushQueue covers all of them with less code.

Not run: the functional and cluster lanes, which need a live Redis server (Docker unavailable here). Those ran in this branch's own CI instead — 21/21 green, unit and functional, at the current head.

AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were executed as pasted. I'm responsible for the change and will handle review feedback.

---

Note

Medium Risk
Touches core reconnect and command-queue flushing; behavior change is narrow (terminal flush paths) but affects all clients that drop while commands are in flight.

Overview
Fixes stuck command promises when a ready connection drops, in-flight work moves to prevCommandQueue, and the client never reaches "ready" again (mid-reconnect disconnect(), retry give-up, maxRetriesPerRequest, etc.). Previously only readyHandler drained that stash, so flushQueue never rejected those commands.

flushQueue now rejects everything in prevCommandQueue (same error as commandQueue) when flushing with defaults, and FlushQueueOptions adds prevCommandQueue so fatal recovery can skip that drain. recoverFromFatalError passes prevCommandQueue: false so stashed commands can still be resent after a reconnect. The prevCommandQueue field is declared on Redis for typing.

New unit tests cover RESP2/RESP3: settlement on close/retry limits, multi-command stash, successful resend (control), fatal-error opt-out vs later disconnect, and re-stash after a prior resend.

^{Reviewed by Cursor Bugbot for commit 42e2d88. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c542f306bd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve stashed commands across recoverable reconnect failures

When a reconnect receives a fatal RESP decoding error, DataHandler.returnFatalError() calls recoverFromFatalError(..., { offlineQueue: false }), which invokes this block and then disconnect(true); the ensuing close handler schedules another reconnect rather than ending the client. This therefore rejects and clears prevCommandQueue after only one failed attempt, so even with the default autoResendUnfulfilledCommands behavior—or maxRetriesPerRequest: null—an original in-flight command cannot be resent when a later attempt succeeds. Preserve the stash for this nonterminal flush, or drain it only on terminal/max-retry paths.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 413facae53

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Flush the stash when disconnect cancels a retry

When redis.disconnect() is called while the client is still in the reconnecting delay—for example, directly from its close event—disconnect() clears the retry timer and calls connector.disconnect() on the already-closed stream, so no later closeHandler invokes flushQueue() and this new block never settles prevCommandQueue. The added test waits until the next connection reaches connect, missing this common timing; commands from the dropped ready connection therefore remain pending indefinitely. The reconnecting branch of disconnect() needs to perform the terminal flush (or otherwise enter close()) when it cancels the retry.

Useful? React with 👍 / 👎.

[askalf]

Good catch — you're right, and it's a regression this PR introduced rather than something pre-existing. recoverFromFatalError ends in disconnect(true), which leaves manuallyClosing unset, so closeHandler schedules another reconnect instead of ending the client. Draining prevCommandQueue there abandoned in-flight commands after a single failed attempt, exactly as described.

Fixed in 413faca by making the drain opt-out-able and only settling the stash on flushes the client cannot come back from:

• lib/DataHandler.ts:37-44 — new optional prevCommandQueue?: boolean on FlushQueueOptions.
• lib/Redis.ts:1019-1023 — defaulted true in flushQueue's defaults(), so every existing caller is unchanged.
• lib/Redis.ts:1047 — guard is now if (options.prevCommandQueue && this.prevCommandQueue).
• lib/Redis.ts:822 — recoverFromFatalError passes { ...options, prevCommandQueue: false }, the only in-tree opt-out.

The four terminal exits (close() via disconnect(), close() via retryStrategy giving up, maxRetriesPerRequest, connector failure) keep draining.

I'd also flag that the old test rejects them when the reconnect hits a fatal protocol error was asserting the wrong behaviour, so it's replaced by two: (control) keeps them across a fatal protocol error so a later reconnect can resend them (conn 1 hangs on GET, conn 2 replies unparseable @bogus\r\n, conn 3 answers bar, so resolved: bar is only reachable if the stash survived the fatal flush) and rejects them when a fatal protocol error is followed by the client ending (same fatal flush, then retryStrategy gives up, so the terminal close flush must settle it). The file is now 8 tests x RESP3/RESP2 = 16 cases.

[PavelPashov]

Thanks for investigating this. The underlying might be bug, but this change affects several reconnect and shutdown paths and overlaps with #2169. It falls outside the small, isolated bug-fix exception in our contribution guidelines and needs agreement on scope and approach before implementation.
I’m closing this PR for now. Please open an issue describing the remaining queue-settlement gap, link #2169, and wait for a maintainer’s go-ahead before submitting a revised implementation.