Mark connections broken on protocol I/O failures

[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

Medium Risk
Touches core connection/pooling behavior on every command; behavior change is intentional but could alter how often pools discard connections compared to before.

Overview
Hardens Connection so pooled clients are not reused after partial or corrupt RESP I/O. On send, flush, read, and push paths, JedisConnectionException and Error now mark the connection broken (via markBroken); already-broken sockets fail fast on further writes/reads without touching the wire.

sendCommand connects first, rejects writes when broken, marks broken before the optional Redis error-line diagnostic (so enrichment cannot leave a suspect connection healthy), and still wraps write failures with enrichWithRedisErrorLine. Valid Redis JedisDataException replies and post-read builder bugs do not mark the connection broken.

Adds ConnectionErrorHandlingTest (fake sockets + single-connection pool) covering serialization/write/read/push failures, guards, and pool invalidation vs reuse on data errors. Registers that test in pom.xml formatter includes.

^{Reviewed by Cursor Bugbot for commit cd826b2. 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.

[lh0156]

Addressed in 16f174b by adding the broken-write guard after connect() and before Protocol.sendCommand(...), so reconnect can still clear the broken state. I also added focused coverage for direct sendCommand rejection, no bytes written, no argument serialization/diagnostic read, reconnect preservation, and pooled invalidation on close.

[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

[atakavci]

nit:

Suggested change

	setBroken();
	throw enrichWithRedisErrorLine(ex);
	throw markBroken(enrichWithRedisErrorLine(ex));

and other uses of it same way.

[atakavci]

leaving connect outside of try-block rings the bells... Didnt investigated throguh the possible use-cases but it would leave the door open to an Error escaping from both connect and sendCommand without marking it broken.

[ggivo]

I will second @atakavci here.

[cursor]

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

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

[cursor]

Connect bypasses markBroken handlers

Medium Severity

In sendCommand, connect() runs outside the try that marks the connection broken on RuntimeException and Error. Unchecked failures during socket setup therefore skip markBroken when the broken flag is still false, so close() may return the connection to the pool as healthy despite a failed command attempt.

 

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