fix(command): return queued tx error - #3933
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fed095a489
ℹ️ 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".
The govulncheck workflow failed on the otel example modules because\ntheir locked indirect grpc dependency was still at v1.80.0, which is\naffected by GO-2026-6061.\n\nBump the example module lockfiles to grpc v1.82.1 so the CI vuln scan\npasses again.\n\nRefs redis#3933
Return the queued transaction error text after draining EXECABORT, while still wrapping the EXECABORT reply so transaction cleanup can recognize that EXEC ran and already cleared WATCH. Refs redis#3933
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ce450ab74
ℹ️ 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".
Keep the queued Redis error discoverable through typed helpers while still preserving the drained EXECABORT in the error chain. Also consume EXEC array replies before returning a queued error so sticky and pooled connections do not inherit buffered EXEC results. Refs redis#3933
|
@freeformz @kavu @geoffgarside @gmcintire review code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb7a08fa2f
ℹ️ 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".
|
Hey @feiguoL , thank you for this contribution. I will review soon and try to include it in the next release. |
Return the queued transaction error text after draining EXECABORT, while still wrapping the EXECABORT reply so transaction cleanup can recognize that EXEC ran and already cleared WATCH. Refs redis#3933
Keep the queued Redis error discoverable through typed helpers while still preserving the drained EXECABORT in the error chain. Also consume EXEC array replies before returning a queued error so sticky and pooled connections do not inherit buffered EXEC results. Refs redis#3933
eb7a08f to
548ee96
Compare
ndyakov
left a comment
ndyakov
left a comment
There was a problem hiding this comment.
Thanks for digging into this one. The root cause analysis matches what I see in #3800: the proxy there rejects the queued command but then answers EXEC with a short array like [OK], so pipelineReadCmds blocks waiting for a reply that never comes and the caller ends up with an i/o timeout. Returning the queued error and draining the EXEC reply is the right way to handle it, and the mock server tests are a nice touch.
I went through the error plumbing and the wrapping approach holds up. ReadLine parses -EXECABORT into the typed error, the multi error Unwrap keeps IsExecAbortError and IsOOMError working through errors.As, and the watchArmed logic in tx.go still clears the watch because it goes through IsExecAbortError. There is also a nice side effect for cluster: a MOVED that comes back during queueing is now visible to isMovedError through the wrapper, so ClusterClient.Watch retries on the right node instead of surfacing a bare EXECABORT.
There is one real problem left, in the loop that drains the EXEC array. ReadReply returns a Go error for Redis error elements too, so if the array contains an error the loop returns early and leaves the remaining elements buffered on the connection. The error we return is a Redis error, so releaseConn puts the connection back in the pool, and the next command on that connection reads a stale EXEC element as its own reply. I reproduced it with your mock server by changing the EXEC reply to
srv.execReply = "*2\r\n-WRONGTYPE Operation against a key holding the wrong kind of value\r\n+OK\r\n"Inside a Watch, Exec surfaced the WRONGTYPE error instead of the queued one, and the Ping right after it came back with OK because it consumed the leftover element from the EXEC array. This matters for exactly the kind of proxy this PR is about, since those execute the commands that did queue successfully and any of them can fail on its own. Real Redis is not affected because a queue error always turns EXEC into EXECABORT.
The fix is small and follows the same pattern pipelineReadCmds already uses, see my inline suggestion. With that change my repro passes and all four of your tests still pass under the race detector. It would be good to cover it with a fifth test, along these lines:
func TestTxPipelineExecDrainsExecArrayWithErrorElement(t *testing.T) {
srv := startTxQueueErrorServer(t)
srv.execReply = "*2\r\n-WRONGTYPE Operation against a key holding the wrong kind of value\r\n+OK\r\n"
defer func() { _ = srv.Close() }()
client := NewClient(&Options{
Addr: srv.Addr(),
DialTimeout: time.Second,
ReadTimeout: time.Second,
WriteTimeout: time.Second,
})
defer func() { _ = client.Close() }()
ctx := context.Background()
err := client.Watch(ctx, func(tx *Tx) error {
pipe := tx.TxPipeline()
pipe.Set(ctx, "a", 1, 0)
pipe.Set(ctx, "b", 1, 0)
_, err := pipe.Exec(ctx)
if err == nil {
t.Fatal("Exec() error = nil, want queued Redis error")
}
if got := err.Error(); !strings.Contains(got, "ERR in transaction context, keys must in same slot") {
t.Errorf("Exec() error = %q, want queued Redis error", got)
}
pong, pingErr := tx.Ping(ctx).Result()
if pingErr != nil {
t.Fatalf("Ping() error = %v, want nil", pingErr)
}
if pong != "PING" {
t.Fatalf("Ping() = %q, want %q", pong, "PING")
}
return nil
})
if err != nil {
t.Fatalf("Watch() error = %v, want nil", err)
}
}Two smaller things, neither of them blocking. If a server sends a queued error and then answers EXEC with a null array, isRedisError(Nil) is true, so we return the wrapper instead of TxFailedErr. Real Redis cannot produce that combination since a queue error always wins and EXEC replies with EXECABORT, so I am fine leaving it as is, just noting it. And one thing for the changelog rather than for this PR: against real Redis an aborted transaction now surfaces the queued error text instead of "EXECABORT Transaction discarded because of previous errors". IsExecAbortError and errors.As still work, but anyone matching on the string prefix will notice, so we should mention it in the release notes.
Return the queued transaction error text after draining EXECABORT, while still wrapping the EXECABORT reply so transaction cleanup can recognize that EXEC ran and already cleared WATCH. Refs redis#3933
Keep the queued Redis error discoverable through typed helpers while still preserving the drained EXECABORT in the error chain. Also consume EXEC array replies before returning a queued error so sticky and pooled connections do not inherit buffered EXEC results. Refs redis#3933
71eaa3f to
28524d2
Compare
Continue draining EXEC array replies when an element is a Redis error so queued transaction failures still return the original queued error and leave sticky connections clean. Refs redis#3933
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28524d2eee
ℹ️ 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".
Process pending RESP3 push notifications while discarding EXEC array replies after a queued transaction error so sticky and pooled connections do not inherit buffered push frames. Refs redis#3933
Convert a nil EXEC reply to TxFailedErr before wrapping queued transaction errors so WATCH cleanup and TxFailedErr matching keep working on queued-error paths. Refs redis#3933
ndyakov
left a comment
ndyakov
left a comment
There was a problem hiding this comment.
The error plumbing checks out: I verified the wrapper keeps redis error semantics through the chain-aware isRedisError (so no connection removal and no retry regression), and IsExecAbortError, IsOOMError and TxFailedErr all work through the multi unwrap. The drain loop consumes the exec array with push handling and the sticky connection tests prove the conn stays usable. Good work on that part and on the mock server suite.
Two things need attention before this merges though, see the inline comments: the cluster client has its own copy of this logic with the same flaw, and the no-EXEC-reply scenario from the PR description still drops the root cause.
Return the queued transaction error text after draining EXECABORT, while still wrapping the EXECABORT reply so transaction cleanup can recognize that EXEC ran and already cleared WATCH. Refs redis#3933
Keep the queued Redis error discoverable through typed helpers while still preserving the drained EXECABORT in the error chain. Also consume EXEC array replies before returning a queued error so sticky and pooled connections do not inherit buffered EXEC results. Refs redis#3933
Continue draining EXEC array replies when an element is a Redis error so queued transaction failures still return the original queued error and leave sticky connections clean. Refs redis#3933
Process pending RESP3 push notifications while discarding EXEC array replies after a queued transaction error so sticky and pooled connections do not inherit buffered push frames. Refs redis#3933
Convert a nil EXEC reply to TxFailedErr before wrapping queued transaction errors so WATCH cleanup and TxFailedErr matching keep working on queued-error paths. Refs redis#3933
8642cb9 to
c9f3e7d
Compare
Preserve HIMPORT post-batch bookkeeping when queued transaction errors still produce a successful EXEC array, avoid decoding non-HIMPORT discarded EXEC replies, and keep cluster redirect outcomes on that array path. Refs redis#3933
Mark successful cluster transaction outcomes that carried HIMPORT commands so the caller still runs himportAfterBatch after the read path changes made for queued EXEC arrays. Refs redis#3933
Keep queued transaction state visible across short EXEC arrays, cluster redirect drain failures, and RESP3 push-drain errors while preserving HIMPORT post-batch semantics on both standalone and cluster readers. Refs redis#3933
Keep queued transaction state visible across queue-reply push failures, short EXEC arrays, and cluster redirect drain failures while preserving HIMPORT post-batch semantics on both standalone and cluster readers. Refs redis#3933
Keep queued transaction state visible across queue-reply push failures, short EXEC arrays, and cluster redirect drain failures while preserving HIMPORT post-batch semantics on both standalone and cluster readers. Refs redis#3933
Preserve command replies that were already read from a successful EXEC array when surfacing a queued transaction error, instead of stamping the whole batch with the queued failure afterward. Refs redis#3933
Preserve handler context and exact HIMPORT bookkeeping while\nkeeping queued transaction drain failures sticky-safe across\nstandalone and cluster readers.\n\nRefs redis#3933
Handle RESP3 attribute frames while discarding successful EXEC arrays, stamp discarded replies with the queued error, and reject malformed negative EXEC array lengths. Refs redis#3933
Use the same EXEC discard helper in the cluster queued-array paths so RESP3 attribute frames are skipped before counting a discarded transaction result. Refs redis#3933
Do not overwrite successfully read HIMPORT command replies when a queued transaction error is surfaced after a successful EXEC array. Refs redis#3933
Discard non-HIMPORT EXEC-array results on queued transaction error paths while still preserving HIMPORT post-batch side effects for the slots that actually executed. Refs redis#3933
Cluster readTxPipelineReplies parsed the EXEC array length but did not guard n<0, so a malformed RESP frame could panic on cmds[readCount:]. Mirror the standalone n<0 guard on both the firstFatal and firstRedirect drain paths and cover it with a cluster test. Also rename tx_pipeline_issue3800_test.go to tx_pipeline_queued_error_test.go per review (behavior-based naming), keeping redis#3800 as a header reference.
Route the EXEC-line read error (both transport errors and typed Redis errors) through classifyExecError instead of handling the transport case inline in readTxPipelineReplies. The non-Redis prefix wraps any queued root cause in txQueuedReadError with unreadReplies so the connection is discarded, while Redis errors keep the connection reusable as before. Behavior is unchanged; the inline firstRedirect switch that duplicated classifyExecError is removed. The added doc comment records the N+2/N+3 reply invariant so the connection-reuse boundary is explicit. Per review by @cxljs.
When a queued Redis error was followed by an EOF/timeout reading a later +QUEUED reply, the failing command kept the raw read error while Exec returned the wrapped txQueuedReadError. setCmdsErr skips commands with a non-nil rawErr, so callers inspecting that command lost the queued root cause and typed-error helpers missed it. Store the wrapped error on the command before returning. Covered by TestTxPipelineExecQueuedReplyReadFailureStampsWrappedError.
When a queued command error was recorded but the EXEC reply was a non-array RESP frame (the malformed/proxy case this PR targets), the generic protocol error was returned without the queued root cause, so typed-error helpers (IsOOMError, IsExecAbortError, ...) could not see it. Wrap the protocol error in txQueuedReadError so the queued cause is preserved; the connection is discarded (forceBad, unreadReplies) since a non-array reply may leave bytes on the wire. Covered by TestTxPipelineExecQueuedErrorNonArrayReplyPreservesQueuedError and TestClusterTxPipelineQueuedErrorNonArrayReplyPreservesQueuedError.
Early exits inside the queued-error EXEC-array drain returned txQueuedReadError without himportedIndexes, so callers never ran himportAfterBatch for HIMPORT replies already decoded — losing side effects such as PREPARE root-cause swaps on HImportSet. Similarly, the cluster processTxPipeline txFatal branch stamped every command whose Err() was nil, overwriting successfully-drained HIMPORT slots with the queued fatal error. - Add himportedIndexes to txQueuedReadError; populate it on mid-drain failures; run himportAfterBatch and skip HIMPORT slots when stamping (standalone). - Set himportedIndexes on mid-drain txOutcome returns so the cluster caller runs himportAfterBatch. - Skip himportedIndexes in the cluster txFatal stamping loop. Covered by TestTxPipelineExecMidDrainFailurePreservesHImportAfterBatch and TestClusterTxPipelineMidDrainFailurePreservesHImportIndexes.
When a queue-phase Redis error or redirect had already set firstFatal / firstRedirect, a later transport error reading another +QUEUED reply returned bare txReadFatal, dropping the root cause. Standalone already wraps this case in txQueuedReadError; cluster now mirrors that so typed helpers and callers see the original queued cause. The connection is discarded (unreadReplies) since replies remain on the wire. Covered by TestClusterTxPipelineQueuedReplyReadFailurePreservesQueuedError.
…rors Three cursor bugbot findings: 1. Push-processor failures during the queued-reply loop and before the EXEC line returned txQueuedReadError without forceBad. When the processor error was a Redis-typed error (e.g. custom OOM), isBadConn kept the connection pooled while EXEC and later replies remained unread, desynchronizing the stream for the next borrower. Set forceBad on all push-drain failure paths (standalone). 2. Parse failures and negative EXEC array lengths dropped queuedErr / firstFatal / firstRedirect, returning a bare protocol error so typed helpers lost the queued root cause. Wrap these in txQueuedReadError (standalone and cluster) with forceBad and unreadReplies to preserve the root cause and discard the conn. 3. The cluster queue-reply IO failure was already fixed in 3294282; this commit also applies forceBad to the cluster classifyExecError transport paths (already done) and the parse/negative-length paths.
Three bot findings on the firstRedirect EXEC-array drain paths: 1. Malformed EXEC length (parse failure / negative) returned txFatal instead of the matching retry kind (MOVED/ASK/TRYAGAIN). Other desync paths with firstRedirect preserve the redirect; now this one does too, via a txRedirectOutcome helper. 2. The non-array EXEC reply branch stamped setCmdsErr on every command before returning a retry. If the redirected attempt later failed, callers saw the discarded attempt's protocol error instead of the final failure. Deferred stamping on retry outcomes. 3. The redirect drain loop discarded every EXEC element without tracking himportedIndexes, so HIMPORT side effects already executed (e.g. PREPARE/DISCARD) were never applied. Added HIMPORT-aware draining mirroring the firstFatal path, carrying himportedIndexes on the outcome so the caller runs himportAfterBatch. Covered by TestClusterTxPipelineRedirectPreservedOnMalformedExecArrayLen.
… indexes Four bot findings: 1. (P1) Once a redirect's EXEC array had any element consumed (HIMPORT reply or discarded result), retrying the whole TxPipeline could double-apply non-idempotent commands. Track readCount in the redirect drain; if readCount > 0, return txFatal. If a drain failure occurs before any element is consumed (readCount == 0), preserve the retry. 2. (cursor + codex P2) Redirect mid-drain failures now carry himportedIndexes on the txOutcome so processTxPipelineNodeConn runs himportAfterBatch for already-decoded HIMPORT replies, mirroring the firstFatal path. 3. (codex P2) Store the wrapped txQueuedReadError on the HIMPORT command before returning, so callers inspecting that command see the queued root cause instead of the raw read error (standalone + cluster). 4. himportedIndexes is now recorded after a successful HIMPORT readReply, not before, so a failed read does not register a side effect that never completed. Tests updated for the new P1 semantics; added TestClusterTxPipelineRedirectMidDrainFailurePreservesHImportIndexes.
A positive EXEC array header means the transaction may already have executed commands, even if the first element is truncated. Return a fatal dirty outcome instead of retrying the whole transaction. Preserve HIMPORT indexes and wrapped root causes on redirect drain failures, and keep HIMPORT read errors visible on their commands.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cde306f188
ℹ️ 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".
Two bot findings: 1. A custom push processor error during the +QUEUED loop before any queued Redis error was recorded returned the bare error. If that error was Redis-typed (e.g. custom OOM), isBadConn kept the connection pooled while EXEC and later replies remained unread. Introduce forceBadConnError and use it for pre-queued queued-loop / pre-EXEC push drain failures so the original error is preserved but the connection is always removed. 2. Release-time draining with VoidProcessor swallowed mid-frame ReadReply errors and reported success, allowing a partially consumed push frame to be re-pooled. Add VoidProcessor.ProcessPendingNotificationsBuffered that propagates mid-frame errors, and use it from drainPushNotificationsOnRelease. Covered by TestTxPipelineExecPushDrainFailureBeforeQueuedErrorForceBadConn and TestReleaseConnRemovesConnectionAfterVoidProcessorBufferedPartialPushRead.
The govulncheck workflow was using go-version "1.26.x", which resolved to Go 1.26.5 in CI and triggered standard-library vulnerabilities already fixed in 1.26.6. Pin the workflow to 1.26.6 so govulncheck runs against the patched stdlib.
Address the remaining bot findings around push draining: - Use a forceBadConnError wrapper when a custom push processor fails in the +QUEUED loop or right before the EXEC line before any queued Redis error has been recorded. This preserves the original error while forcing connection removal so unread replies cannot poison the next borrower. - Add a release-time Buffered variant for VoidProcessor and use it from drainPushNotificationsOnRelease so mid-frame errors are propagated instead of being swallowed. - Cover both cases with focused regression tests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2f048b2. Configure here.
The release-time VoidProcessor buffered drain should match the builtin Processor semantics: a PeekReplyType / PeekPushNotificationName timeout that consumes no bytes is not fatal and should leave the connection reusable. Only mid-frame errors are propagated so the connection is removed. Add a regression test for the empty wrapped-probe case with VoidProcessor, and keep the partial-frame tests proving real desync still removes the connection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e2f8db25e
ℹ️ 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".
3 participants
When Redis rejects a queued tx command, the server may not send an
EXECarray reply.TxPipeline.Execkept reading anyway, so callers saw an I/O timeout instead of the original Redis error.This returns the first queued Redis error immediately and adds a regression test for the timeout scenario from #3800.
Summary
EXECreply after queueing has already failedTesting
go test -run TestTxPipelineExecReturnsQueuedRedisError .go test -run TestGinkgoSuite . -ginkgo.focus="surface the triggering error on TxPipeline EXECABORT"(fails locally in BeforeSuite because the Redis test stack is not running:dial tcp 127.0.0.1:6390: connect: connection refused)Closes #3800
Note
High Risk
Changes core MULTI/EXEC and cluster transaction read/retry semantics, connection pooling on partial RESP3 reads, and error typing—high impact paths with broad regression coverage but real behavioral risk for tx and cluster users.
Overview
Fixes TxPipeline (and cluster tx) behavior when a command is rejected during
MULTIqueueing: callers get the first queued Redis error (and helpers likeIsExecAbortError/IsOOMError) instead of hanging or seeing a bare I/O timeout (#3800). The read path now drainsEXECreplies (arrays,EXECABORT, RESP3 pushes/attrs) while preserving the root cause via new wrapped errors (txQueuedReadError,txQueuedExecArrayError, etc.), marks desynced connections bad, and handles HIMPORT slots that were partially read before a failure.Connection release for RESP3 is stricter: release-time push draining uses buffered processors for built-in/void processors, treats drain failures as pool remove (including partial push frames and custom processor errors), with matching CSC tests.
shouldRetry/isBadConnunderstand the new tx wrappers; WATCH clears ontxQueuedExecArrayErroroutcomes. CI pins Go 1.26.6. Large mock-server regression suite added for tx queue/EXEC edge cases.Reviewed by Cursor Bugbot for commit 9e2f8db. Bugbot is set up for automated code reviews on this repo. Configure here.