[lh0156]

Fixes #4278.

Summary

• Mark Connection broken when protocol send/read/push paths fail with JedisConnectionException or Error.
• Guard reads and writes after a connection has already been marked broken.
• Add focused unit coverage for serialization/write failures, protocol read failures, push read failures, pool invalidation, and Redis data errors that should not break the connection.

Motivation

If a pooled connection fails while RESP bytes are being written or read, the connection can retain partial or stale protocol state. Returning that connection to the pool as healthy can allow a later borrower to read stale data.

Redis error replies are still valid protocol responses, so JedisDataException paths continue to leave the connection reusable.

Notes

• The existing best-effort Redis error-line diagnostic in sendCommand is preserved.
• The connection is marked broken before the diagnostic read, so a failure during diagnostic enrichment cannot return a suspect connection to the pool.

Testing

• /Users/developseop/.m2/wrapper/dists/apache-maven-3.9.15/9925cc1d/bin/mvn -B -Dwith-param-names=true -Dtest=ConnectionErrorHandlingTest test
• /Users/developseop/.m2/wrapper/dists/apache-maven-3.9.15/9925cc1d/bin/mvn -B -Dwith-param-names=true -Dnet.bytebuddy.experimental=true test

---

Note

High Risk
Changes behavior on every command path through core Connection and connection-pool invalidation; misclassification of broken vs healthy could return stale connections to the pool or over-invalidate.

Overview
Hardens Connection so pooled clients are not reused after RESP I/O leaves the stream in an unknown state (fixes #4278).

Connection now records a brokenCause (first failure wins) and routes connect, send, flush, read, and push paths through markBroken, which sets the broken flag and rethrows the original exception. sendCommand and flush refuse further writes on a broken connection; read helpers refuse reads and attach brokenCause to guard **JedisConnectionException**s. JedisDataException from a fully parsed Redis error reply no longer marks the connection broken; transport/mid-read failures and **Error**s still do. Write failures still attempt the optional Redis error-line diagnostic after marking broken. executeCommand(CommandObject) only builds the reply after a successful read, so builder failures after a good read do not break the socket.

Adds ConnectionErrorHandlingTest (fake sockets, pool invalidation) and registers it in the formatter pom.xml include list.

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

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[ggivo]

Thanks for the PR — the team will take a look as soon as we have free cycles.

In the meantime, to get the formatting checks passing, please add the new files to the formatter config in pom.xml and run mvn format over them. That should resolve the current CI failures.

[lh0156]

Thanks for the guidance. I added the new test file to the formatter config, ran the formatter, and pushed the fix in 724d5ee.

I also verified locally with:

• mvn formatter:validate
• mvn -Dtest=ConnectionErrorHandlingTest test

The GitHub Actions workflows on the latest commit currently show as action_required, so I’m waiting for them to be approved/rerun.

[ggivo]

@lh0156
Thanks. Just triggered the CI

[Copilot]

Pull request overview

This PR hardens redis.clients.jedis.Connection error handling so that protocol I/O failures (write/flush/read/push paths) reliably mark a connection as broken, preventing pooled connections with potentially contaminated RESP state from being reused (fixes #4278).

Changes:

• Update Connection to mark broken on protocol send/flush/read/push failures (notably JedisConnectionException and Error) and to reject reads/writes once broken.
• Preserve the existing “read Redis error line if possible” diagnostic when sendCommand fails, while ensuring the connection is already marked broken first.
• Add comprehensive unit coverage for serialization/write failures, protocol read failures, push read failures, and pool invalidation behavior; register the test in the formatter plugin includes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/main/java/redis/clients/jedis/Connection.java	Marks connections broken on key protocol failure paths and adds broken-state guards.
src/test/java/redis/clients/jedis/ConnectionErrorHandlingTest.java	Adds focused tests covering broken-state behavior across send/flush/read/push and pool invalidation scenarios.
pom.xml	Registers the new test file in the formatter plugin includes list.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ggivo]

@lh0156
Made one round of review, I think the review looks good. Want to make another round over the tests introduced before final approval, since it does not seem that we had good coverage over error propagations.

adding also @atakavci for another pair of review

[atakavci]

i suggest to investigate if connect outside of tyry-block 💯 safe

[cursor]

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

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

[ggivo]

@lh0156

Nice work!

I did another review pass, and I think the main remaining gap is hardening the Error (OutOfMemoryError example) handling described in the original issue.

Specifically, there is still a window in executeCommand() where an Error can occur after the command has been successfully sent (sendCommand(args)) but before the response is consumed (getOne()). In that case, the connection may be left in an unknown protocol state and later returned to the pool without being marked as broken.

[lh0156]

Thanks for the review. I addressed the remaining executeCommand() window.

After a command is sent, reply consumption now goes through readReplyAfterCommandSent(), which marks the connection broken if an Error is thrown before or during reply consumption.

For CommandObject execution, the code now reads the reply first and only invokes the response builder after the reply has been consumed. That keeps the protocol-state boundary explicit: Errors while consuming the Redis reply mark the connection broken, but builder lookup/build failures after the reply is consumed do not.

I also added regression coverage for the builder lookup case and verified locally with:

mvn -B -Dwith-param-names=true -Dtest=ConnectionErrorHandlingTest test

[ggivo]

@lh0156

Did some tests with the provided reproduction example, suggested with the original issue, and confirmed that the buffer corruption was not observed.

Thanks for taking care of the unstable tests. They don't seem related to the current change, so I will suggest extracting them in a dedicated PR.

If you don't mind, I will take it from here, perform a small cleanup, and move on to merging it.

[ggivo]

tracking issue for flaky AllKindOfValuesCommandsTestBase.touch

[ggivo]

LGTM

[ggivo]

@lh0156

Merged! Thanks for seeing this through, @lh0156

I reverted "Stabilize touch idletime assertions" and opened a dedicated issue (#4591) to track that effort. Feel free to jump in there if you're interested.