Repository navigation
fix(client): call a legacy callback once when it throws - #3546
Merged
nkaradzhov merged 1 commit intoOct 28, 2026
Merged
Conversation
Every legacy callback ran inside a .then whose rejection the following .catch handled, so an error thrown by the callback on success was passed back to the same callback as its error. Run the reply transform in its own .then and call back from a two-argument .then, so a transform error still reaches the callback and an error thrown by the callback is not fed back into it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Description
Legacy-mode callbacks are wired as
promise.then(reply => callback(null, reply)).catch(err => callback(err)), so when the callback throws on success, the.catchcalls it again with its own error. That's in all four places that call back: the generated commands (#createCommand),sendCommand,quitandmulti().exec.With a callback that throws, against Redis 8.10.2, on master:
With this change each one runs once.
The reply transform (
TRANSFORM_LEGACY_REPLY, andtransformRepliesfor exec) now runs in its own.then, and the callback is called from.then(reply => callback(null, reply), err => callback(err)), so a transform error like theMultiErrorReplyfrom exec still reaches the callback. A throw from the callback now ends as an unhandled rejection, same as master after the second call.Added a test per call site in
legacy-mode.spec.ts; all 4 fail on master.npm run build && npm testpasses and lint is clean, apart from the sentineldestroy() from an end listener tears down a reentrant reopencase, which failed once in the full run here and passes 5 out of 5 when run alone.Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Touches promise/callback wiring on all legacy API entry points; behavior change for throwing callbacks (no second err callback) but aligns with expected Node semantics and is well covered by new tests.
Overview
Fixes double invocation of legacy-mode Node callbacks when the user callback throws after a successful Redis reply.
Previously, success handlers ran inside
.then(...)with a following.catch(err => callback(err)), so a throw fromcallback(null, reply)was treated as a rejection and triggered a second call with that error.sendCommand, generated commands (e.g.set),quit, andmulti().execall used this pattern.The implementation now invokes the callback from a single
.then(onFulfilled, onRejected)so throws from the callback propagate as unhandled rejections instead of a second callback. WhereTRANSFORM_LEGACY_REPLYortransformRepliesapplies, the transform runs in a separate.thenfirst so transform/exec errors still reach the callback once.Regression coverage adds a shared
callsOfThrowingCallbackhelper and one test per call site, asserting exactly one[null, reply]and that the thrown error surfaces as unhandled.Reviewed by Cursor Bugbot for commit c1c7b2c. Bugbot is set up for automated code reviews on this repo. Configure here.