numinnex commented on code in PR #3944:
URL: https://github.com/apache/iggy/pull/3944#discussion_r3852351058
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -358,6 +403,15 @@ impl TcpClient {
return Err(IggyError::CannotEstablishConnection);
Review Comment:
Fixed. The `reconnection.enabled` check moved below the candidate advance,
so it is the sweep that the reconnection settings apply to rather than a single
dial: with reconnection off every failover address still gets its one turn, and
only an exhausted sweep bails. Every early return now goes through
`fail_connect()`, which puts the state back to `Disconnected` and publishes the
event, so a later `connect()` dials instead of returning ok at the top.
Two unit tests, both red against the old code:
`a_client_with_reconnection_disabled_still_sweeps_its_failover_endpoints` and
`a_connect_that_exhausts_every_endpoint_leaves_the_client_disconnected` (which
asserts the second `connect()` fails again rather than reporting success).
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -387,6 +441,10 @@ impl TcpClient {
error!("Failed to establish TCP connection to the server:
{error}",);
IggyError::CannotEstablishConnection
})?;
+ // The endpoint that answered is where this client now lives:
+ // the leader check compares against it, and the next
+ // reconnect starts from it.
+ *self.current_server_address.lock().await =
server_address.clone();
Review Comment:
Fixed. The dial, the socket options and the TLS handshake moved into
`establish()`, and `current_server_address` / `client_address` are written only
after it hands back a usable stream, so a node that accepts TCP and fails TLS
leaves no trace for the next pass to lead with.
A handshake failure is now a failed dial: it maps to
`CannotEstablishConnection`, the sweep continues, and an exhausted sweep
returns `CannotEstablishConnection`. A configuration fault
(`InvalidTlsCertificatePath`, `InvalidTlsCertificate`, `InvalidTlsDomain`)
returns straight away instead of looping forever under `max_retries = None`, as
you asked.
While there: the bound now covers the handshake too (`establish_bounded`). A
peer that accepts TCP and never answers the ClientHello had no deadline
anywhere, and
`an_endpoint_that_never_answers_the_handshake_does_not_hold_up_the_sweep` hung
the test binary until that changed.
`an_endpoint_that_fails_the_tls_handshake_does_not_become_the_current_one` pins
the stickiness fix (red without it: the current address became the plaintext
endpoint).
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -155,20 +178,30 @@ impl BinaryTransport for TcpClient {
return Err(error);
}
+ // A stale-client eviction is the server ending this session
+ // authoritatively, like a logout: the remembered sign-in ends with it,
+ // so only a configured auto-login may bring the session back.
+ // Remembered credentials exist for transport loss, where the session
+ // died with the socket rather than by anyone's decision.
+ if matches!(error, IggyError::StaleClient) {
+ self.forget_session_credentials().await;
+ }
+
if !self.config.reconnection.enabled {
return Err(IggyError::Disconnected);
}
- if matches!(self.config.auto_login, AutoLogin::Disabled) &&
!is_login_register_code(code) {
- // Without auto-login a reconnect cannot re-establish the session,
- // so non-login requests fail fast. Login/register itself is the
+ if !is_login_register_code(code) &&
self.sign_in_credentials().await.is_none() {
Review Comment:
Restricted, rather than documented. After the reconnect the request is
replayed only when it provably never reached the log:
- the error was raised before the frame was written (`NotConnected`,
`CannotEstablishConnection`) or is an explicit server refusal that precedes
execution (`Unauthenticated`, `StaleClient`);
- or the operation is `Operation::NonReplicated`, which is never
deduplicated in the first place;
- or it is login/register, where the replay is the protocol (the server
stays silent on a transient register failure and relies on the client
resending).
Everything else — `Disconnected`, `EmptyResponse`, `TcpError` on a
replicated op — still reconnects, so the transport is healed for the next call,
then returns the original error with a warn instead of replaying it. Same rule
as the Go `canReplay` comment.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -320,34 +363,36 @@ impl TcpClient {
}
self.set_state(ClientState::Connecting).await;
- if let Some(connected_at) =
self.connected_at.lock().await.as_ref() {
- let now = IggyTimestamp::now();
- let elapsed = now.as_micros() - connected_at.as_micros();
- let interval =
self.config.reconnection.reestablish_after.as_micros();
- trace!(
- "Elapsed time since last connection: {}",
- IggyDuration::from(elapsed)
- );
- if elapsed < interval {
- let remaining = IggyDuration::from(interval - elapsed);
- info!("Trying to connect to the server in: {remaining}",);
- sleep(remaining.get_duration()).await;
- }
+ let candidates = self.dial_candidates().await;
+ // The reestablish delay paces reconnects to the one endpoint a
+ // single-address client has. With other endpoints known there is
+ // somewhere else to go, and pausing first only pushes the
+ // failover past the window the caller is willing to wait; the
+ // retry interval still paces the loop.
+ let reestablish_wait = if candidates.len() > 1 {
Review Comment:
Applied to the current endpoint only. When a `reestablish_after` window is
still pending and there are other candidates, the list is rotated so the paced
endpoint goes last, and the remaining window is waited out immediately before
dialing it — recomputed at that moment, so the time spent on the other
endpoints counts against it. A single-endpoint client behaves exactly as
before, so `with_reestablish_after` keeps its promise.
Both halves are pinned:
`a_pending_reestablish_pause_does_not_delay_dialing_another_endpoint` (10s
window, connect completes in under 2s on the survivor) and
`the_reestablish_pause_still_applies_to_the_endpoint_that_was_lost` (1s window,
at least 700ms elapsed). The second is red against the old code.
Go got the same semantics, with
`TestFailover_DoesNotSpendTheLostEndpointsPauseOnAnotherEndpoint` /
`TestFailover_KeepsTheReestablishPauseForTheEndpointThatWasLost`.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -155,20 +178,30 @@ impl BinaryTransport for TcpClient {
return Err(error);
}
+ // A stale-client eviction is the server ending this session
+ // authoritatively, like a logout: the remembered sign-in ends with it,
+ // so only a configured auto-login may bring the session back.
+ // Remembered credentials exist for transport loss, where the session
+ // died with the socket rather than by anyone's decision.
+ if matches!(error, IggyError::StaleClient) {
+ self.forget_session_credentials().await;
Review Comment:
Removed. You are right that a GC pause or a laptop sleep is not caller
intent, and wedging a manually signed-in `IggyConsumer` while an auto-login one
recovers is the asymmetry this PR is supposed to remove. `StaleClient` now
stays in the replay-safe set, so the reconnect signs in again with the
remembered credentials.
Flagging one inconsistency so it is a decision rather than an accident: Go,
C# and Java still drop the remembered sign-in on a stale-client eviction. C#
because `EvictedClient_WithoutAutoLogin_Should_FailFast_And_NotReconnect`
asserts exactly that contract, and Java's is now scoped to clients with no
configured credentials (a client built with credentials still re-authenticates,
which its own `shouldReplayTransientImplicitLoginAfterEviction` requires). If
you would rather have one rule everywhere, say which way and I will move the
other three.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -485,24 +543,24 @@ impl TcpClient {
};
// Handle auto-login
- let should_redirect = match &self.config.auto_login {
- AutoLogin::Disabled => {
- info!("Automatic sign-in is disabled.");
+ let should_redirect = match self.sign_in_credentials().await {
+ None => {
+ info!("No credentials to sign in with.");
// Only `IggyClient` redirects after a manual sign-in, so
// a raw transport can stay on a backup: its first
// replicated write gets `TransientNotAccepted`, the
// redirect drops the session, and the retry fails
// `Unauthenticated` until the caller signs in again.
false
}
- AutoLogin::Enabled(credentials) => {
+ Some(credentials) => {
if skip_auto_login {
info!("Skipping automatic sign-in for a retried
login/register request.");
false
} else {
info!("{NAME} client: {client_address} is signing
in...");
self.set_state(ClientState::Authenticating).await;
- match credentials {
+ match &credentials {
Credentials::UsernamePassword(username, password)
=> {
Review Comment:
Both halves fixed.
The triple login is gone: `binary_users.rs` and
`binary_personal_access_tokens.rs` now check `redirect_login_settled()` after
the reconnect and return the identity they already have when the client is
`Authenticated` and no `AutoLogin` is configured. It is not a plain
short-circuit — with `AutoLogin::Enabled(A)` plus a manual `login_user(B)` the
reconnect signed in A, so the login still runs and the client ends as B.
And a failed sign-in in `connect()` no longer leaves the state at
`Authenticating`: it goes back to `Connected` (the transport is up, the session
is not), and when the server rejected the credentials outright
(`InvalidCredentials`, `InvalidUsername`, `InvalidPassword`, `Unauthenticated`)
the remembered sign-in is dropped so a stale password is not replayed, and an
argon2 paid for, on every reconnect. Configured credentials stay as configured;
those are the caller's to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
