fix(sdk): keep an explicit disconnect until the next connect by TomPlanche · Pull Request #4388 · apache/iggy

TomPlanche · 2026-10-02T17:00:06Z

edit: after @jiengup's review, connect_inner refuses a reconnect while the disconnect holds, so a request in flight during disconnect() cannot reconnect through the recovery path. A unit test per transport covers it. A connect already past that check when disconnect() runs is problem 6 of #4287.
edit2: addressed @numinnex's review: per-step checks in all sweeps, redirect keeps the disconnect, heartbeat awaited, ClientShutdown kept after shutdown.

Which issue does this PR address?

Relates to #4287

Rationale

An explicit disconnect() must hold until the next connect(), as agreed on #4287: the test in disconnect_relogin.rs is the correct contract, not the IggyClient docs. Problems 2 and 3 of #4287 break this rule for a client with auto-login credentials. This PR fixes these two problems.

What changed?

After disconnect(), a client with auto-login credentials came back on its own. The heartbeat kept running and its ping reconnected the client and signed in again, and a direct ping() or get_stats() did the same. Only an authenticated request such as get_me() failed, because fail_if_not_authenticated rejects it before the transport.

Now the Client trait impl of disconnect() sets a caller-intent flag on TCP, QUIC and WebSocket, and the trait impl of connect() clears it. While the flag is set, send_raw_with_response fails with NotConnected at once: no packets on the wire, no wait, no reconnect. The flag lives only in the trait impl, because leader redirection and the reconnect path call the inherent disconnect(). A connection that drops without disconnect() leaves the flag clear, so that loss still heals. IggyClient::disconnect also stops the heartbeat, and connect starts it again. This removes the useless pings, and on QUIC it removes the redial that blocked disconnect() in endpoint.wait_idle().

The Client trait now documents the disconnect() contract. The IggyClient, Python and C++ docs no longer say that auto-login undoes a disconnect. The last two lifecycle tests from #4344 lose their #[ignore], so all 10 cases of client_lifecycle.rs now run.

Local Execution

I used Zed in devcontainer mode. The build and the tests ran in the devcontainer, and prek ran on macOS.

Passed: cargo fmt --all and cargo clippy -p iggy_common -p iggy -p integration --all-features --all-targets -- -D warnings.
Passed: cargo test -p iggy --lib, with 234 tests.

Passed: the lifecycle tests, with all 10 cases and none ignored, in 3 runs out of 3:

$ cargo test -p integration -- sdk::client_lifecycle
test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 991 filtered out

Passed: the sdk:: integration suite (69 passed, 0 ignored), which includes disconnect_relogin.rs, and the cluster:: suite (80 passed), because the flag sits in the reconnect path that failovers use.

Passed: the Python lifecycle tests against a local server:

$ uv run --no-sync pytest tests/test_connectivity.py -k Lifecycle
16 passed, 35 deselected

Passed: cargo run --bin stub_gen. The stub diff contains only the disconnect() docstring.
Passed: pre-commit hooks with SKIP=web-lint prek run --all-files. I skipped web-lint because this PR does not change web/.

AI Usage

None for the code, tried to write this PR body with Claude but finished with some 200+ lines slop.

github-actions · 2026-10-02T17:00:18Z

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

/ready - back to S-waiting-on-review after addressing feedback
/author - flip to S-waiting-on-author while you finish changes
/request-review @user-or-team - request a reviewer
/pin - exempt the PR from the stale bot, /unpin to undo

See CONTRIBUTING.md for details.

codecov · 2026-10-02T17:18:53Z

Codecov Report

❌ Patch coverage is 82.84314% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.88%. Comparing base (c5f21c0) to head (9d1521a).

Files with missing lines	Patch %	Lines
core/sdk/src/quic/quic_client.rs	70.58%	9 Missing and 1 partial ⚠️
core/sdk/src/tcp/tcp_client.rs	90.99%	5 Missing and 5 partials ⚠️
core/sdk/src/websocket/websocket_client.rs	70.58%	9 Missing and 1 partial ⚠️
core/sdk/src/clients/client.rs	82.35%	1 Missing and 2 partials ⚠️
core/sdk/src/client_wrappers/binary_client.rs	75.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4388      +/-   ##
============================================
- Coverage     88.45%   87.88%   -0.57%     
  Complexity     1607     1607              
============================================
  Files          1302     1302              
  Lines        252659   252861     +202     
  Branches     212824   213025     +201     
============================================
- Hits         223498   222236    -1262     
- Misses        23791    25272    +1481     
+ Partials       5370     5353      -17

Components	Coverage Δ
Rust Core	`89.56% <82.84%> (-0.02%)`	⬇️
Java SDK	`68.74% <ø> (-0.07%)`	⬇️
C# SDK	`63.56% <ø> (-15.19%)`	⬇️
Python SDK	`91.07% <ø> (ø)`
PHP SDK	`85.67% <ø> (ø)`
Node SDK	`96.62% <ø> (+0.10%)`	⬆️
Go SDK	`70.27% <ø> (+0.02%)`	⬆️
C++ SDK	`86.37% <ø> (ø)`

Files with missing lines	Coverage Δ
...e/sdk/src/clients/binary_personal_access_tokens.rs	`100.00% <ø> (ø)`
core/sdk/src/clients/binary_users.rs	`100.00% <ø> (ø)`
foreign/cpp/include/iggy.hpp	`86.43% <ø> (ø)`
foreign/python/src/client.rs	`99.76% <ø> (ø)`
core/sdk/src/client_wrappers/binary_client.rs	`87.50% <75.00%> (-12.50%)`	⬇️
core/sdk/src/clients/client.rs	`90.57% <82.35%> (-0.42%)`	⬇️
core/sdk/src/quic/quic_client.rs	`84.18% <70.58%> (-0.07%)`	⬇️
core/sdk/src/tcp/tcp_client.rs	`92.10% <90.99%> (+0.21%)`	⬆️
core/sdk/src/websocket/websocket_client.rs	`84.69% <70.58%> (-0.46%)`	⬇️

... and 102 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

TomPlanche · 2026-10-05T12:36:21Z

/request-review @jiengup

TomPlanche · 2026-10-05T12:36:46Z

/request-review @hubcio

TomPlanche · 2026-10-05T16:38:15Z

Thanks @numinnex for the great review. I checked each point against the code, and these five hold:

tcp_client.rs:659: the send guard turns a disconnect during an auto-login sweep into an endless redial, because fail_sign_in reads NotConnected as a lost connection.
tcp_client.rs:176: the leader redirect in login_user goes through the trait connect(), so it clears the flag and undoes the disconnect.
client.rs:1006: the aborted heartbeat can leave the client in Connecting, and the TCP disconnect() then returns early with no teardown.
tcp_client.rs:235: the send guard has no unit test, and after shutdown() it hides ClientShutdown behind NotConnected.
quic_client.rs:707: the QUIC and WebSocket ladders can still install a connection after the disconnect.

The doc and test nits are valid too.

So a flag checked at the entry points is not enough: it must be checked at every step of the reconnect sweep, on all three transports. That is the same code #4345 changes, and close to problems 5 to 8 of #4287 and to #3651. I asked on #4287 how you want it scoped before I change more of the state machine.

TomPlanche · 2026-10-05T20:39:17Z

/ready

TomPlanche · 2026-10-05T20:39:27Z

/request-review @numinnex

hubcio · 2026-10-10T18:07:31Z

@TomPlanche could you please rebase this?

TomPlanche · 2026-10-10T23:40:16Z

@hubcio Rebased onto master. The conflicts were with #4387 the sign-in guard now sits on top of its new error handling.

github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Oct 2, 2026

jiengup reviewed Oct 5, 2026

View reviewed changes

Comment thread core/sdk/src/tcp/tcp_client.rs Outdated

github-actions Bot requested review from hubcio and jiengup October 5, 2026 12:36

numinnex reviewed Oct 5, 2026

View reviewed changes

TomPlanche mentioned this pull request Oct 5, 2026

Rust SDK: the heartbeat undoes disconnect(), and a WebSocket client connects after shutdown() #4287

Open

2 tasks done

github-actions Bot requested a review from numinnex October 5, 2026 20:50

fix(sdk): keep an explicit disconnect until the next connect

9d1521a

TomPlanche force-pushed the fix/sdk-disconnect-holds branch from 78939bf to 9d1521a Compare October 10, 2026 23:39

Conversation

TomPlanche commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR address?

Rationale

What changed?

Local Execution

AI Usage

Uh oh!

github-actions Bot commented Oct 2, 2026

Uh oh!

codecov Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

TomPlanche commented Oct 5, 2026

Uh oh!

TomPlanche commented Oct 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomPlanche commented Oct 5, 2026

Uh oh!

TomPlanche commented Oct 5, 2026

Uh oh!

TomPlanche commented Oct 5, 2026

Uh oh!

hubcio commented Oct 10, 2026

Uh oh!

TomPlanche commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TomPlanche commented Oct 2, 2026 •

edited

Loading

codecov Bot commented Oct 2, 2026 •

edited

Loading