[insaf021]

MultiNodePipelineBase.sync() only logged a node's JedisConnectionException: sync() returned normally with that node's responses left unset, and the cleanup ran on the worker thread through the calling thread's iterator, so it removed whichever node that thread had last visited instead of the failed one. A caller that did not read every Response never learned the batch was partly lost; one that did got IllegalStateException from Response.get(), and the next command routed to the failed node threw NullPointerException in appendCommand.

Implements the contract agreed in #4760:

1. every node is still read to completion, so the healthy nodes' responses are set;
2. each response of a failed node is completed with the connection error, so Response.get() rethrows it and the commands left in doubt can be identified;
3. the failed nodes are dropped from pipelinedResponses and connections on the calling thread, so the pipeline stays consistent and the next command for such a node takes a fresh connection while the other nodes keep theirs;
4. sync() rethrows the first failed node's JedisConnectionException in pipeline order, with further node failures attached as suppressed. close() surfaces it the way Pipeline.close() does.

Single-node Pipeline.sync()/syncAndReturnAll() already propagated the error but left their buffered responses unset. They now complete them the same way and drop them, so a later sync() does not try to read those replies again, and Response.get() behaves identically on both pipelines.

Response gained a package-private setFailure(JedisException) for this; a connection failure means no reply was read, so it is kept separate from the JedisDataException carried by an error reply. No public API change.

Unit tests cover the deferred cleanup, the response completion, both the worker and the inline single-node read path, two nodes failing in one sync(), and the single-node pipeline. Behaviour change in 4 is in the 8.1.0 release notes, and the failure contract is documented under Pipelining in docs/advanced-usage.md.

Closes #4760.

---

Note

Medium Risk
Behavior change for ClusterPipeline.sync()/close() callers that assumed success after a partial read failure; exception handling and retry logic may need updates.

Overview
Pipeline sync()/close() now surface reply-read failures instead of failing silently or leaving responses in an ambiguous state.

For cluster pipelines, a connection loss during sync() used to be logged only: sync() returned normally, Response.get() on the failed node raised IllegalStateException, and follow-up commands could hit NullPointerException. The implementation now still reads every other node, marks each lost reply via Response.completeExceptionally(), drops only the failed nodes (so reuse can open fresh connections), and rethrows the first failure in pipeline order with additional node failures as suppressed exceptions.

Single-node Pipeline already threw from sync(), but buffered Responses stayed “not synced”; they are completed with the same failure, cleared from the pipeline buffer, and get() behaves like the cluster path.

Response gains package-private completion for read failures (distinct from Redis error replies). 8.1.0 release notes and advanced-usage document the contract: callers must handle sync()/close() exceptions and must not blindly replay commands that may already have run on the server; cluster pipelines do not refresh the slot cache after failover.

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

[ggivo]

Thanks for the PR. Let's settle the intended behaviour on the issue first — see my questions on #4760 — before reviewing the implementation here.

[insaf021]

Answered on #4760. I'll leave this as it is until the behaviour is agreed there, then rework it to match.

[insaf021]

Reworked to the contract from #4760, all four points in this PR.

Failed nodes are recorded by their workers and dropped from both maps on the calling thread after the reads, and each of their pending responses is completed with the JedisConnectionException - getMany discards the replies it had already read, so none of them was set. sync() then rethrows the first failure in pipeline order (not completion order, so it's deterministic) with any further node failures suppressed; close() already surfaces whatever sync() throws. Healthy nodes are unaffected and still set their responses.

On your point about the single-node pipeline: Pipeline.sync() and syncAndReturnAll() now complete their buffered responses the same way before propagating, and drop them so a later sync() doesn't try to read those replies off a dead connection again. Response.get() behaves identically on both pipelines now.

That needed somewhere to put the error: Response.exception is a JedisDataException from an error reply, and a connection failure means no reply was read at all, so I added a package-private setFailure(JedisException) rather than widening that field. No public API change.

Tests cover the deferred cleanup, the response completion, the inline single-node read path, two nodes failing in one sync(), and the single-node pipeline. Release note for point 4 is in 8.1.0 and the contract is documented under Pipelining in docs/advanced-usage.md. Full unit suite green on JDK 11 (3468 run, 0 failures).

One unrelated note: the doctests check is red on QueryAggExample.run:241 expected: <5> but was: <4>, which also fails intermittently on master and doesn't touch this path.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 3ccfcac. Configure here.}

[insaf021]

CI on the reworked branch: everything green except doctests, which now fails on QueryRangeExample.run:239 expected: <3> but was: <2> and HomeJsonExample.run:226 IndexOutOfBounds rather than the QueryAggExample I mentioned above. Different search examples each run, all of them index-visibility races on FT.SEARCH with no pipeline in the path, and the job fails intermittently on master too.

[ggivo]

Pushed fc403da on top of your branch with a few changes:

• Keyless commands are pipelined under a null node key, which the ConcurrentHashMap rejected (Bugbot's finding): a failed read threw NPE in the worker and could leave syncing stuck. failures is now a synchronized HashMap, and syncing is reset in a finally.
• The worker catches Throwable, not just JedisConnectionException, so other failures no longer die in the executor while sync() returns normally. sync() rethrows the original unchanged; the first failure in pipeline order wins, the rest are suppressed.
• Response.setFailure is now completeExceptionally(Throwable): Jedis exceptions are kept as is, anything else is wrapped in a JedisException, so get() never rethrows an Error. It has no effect on a response that is already completed. The protected exception field keeps its type for compatibility; merging the two fields is tracked in Response cleanup for 9.0: remove setDependency, make the exception field private #4793.
• Release note and advanced-usage.md tightened, including that a single-node pipeline's connection is broken after a failed sync().

Covered by new cases in MultiNodePipelineSyncTest and PipelineSyncFailureTest, plus ResponseTest.
Let me know if anything looks off.