diff --git a/bdd/rust/tests/helpers/cluster.rs b/bdd/rust/tests/helpers/cluster.rs index 195bcb9a83..8a954c22af 100644 --- a/bdd/rust/tests/helpers/cluster.rs +++ b/bdd/rust/tests/helpers/cluster.rs @@ -17,6 +17,7 @@ use iggy::prelude::*; use std::env; +use std::net::{SocketAddr, ToSocketAddrs}; use std::sync::Arc; /// Resolves server address based on role and port, checking environment variables first @@ -49,6 +50,24 @@ pub async fn create_and_connect_client(addr: &str) -> IggyClient { IggyClient::create(ClientWrapper::Tcp(client), None, None) } +/// Whether two `host:port` spellings name the same endpoint. +/// +/// A client that never redirected still holds the address it was given (a +/// host name, in the BDD compose network), while a redirected one holds the +/// address the roster published (an IP). Both name the same node, so they +/// are compared once resolved, like the Go and Java suites do. +pub fn is_same_endpoint(left: &str, right: &str) -> Result { + let resolve = |address: &str| -> Result, String> { + address + .to_socket_addrs() + .map(Iterator::collect) + .map_err(|error| format!("Failed to resolve server address {address}: {error}")) + }; + let left = resolve(left)?; + let right = resolve(right)?; + Ok(left.iter().any(|candidate| right.contains(candidate))) +} + /// Verifies that a client is connected to the expected port pub async fn verify_client_connection( client: &IggyClient, diff --git a/bdd/rust/tests/steps/leader_redirection.rs b/bdd/rust/tests/steps/leader_redirection.rs index 1f6bcc5ad5..c035bd848b 100644 --- a/bdd/rust/tests/steps/leader_redirection.rs +++ b/bdd/rust/tests/steps/leader_redirection.rs @@ -288,10 +288,16 @@ async fn then_both_use_same_server(world: &mut LeaderContext) { let conn_info_a = client_a.get_connection_info().await; let conn_info_b = client_b.get_connection_info().await; - // Verify both clients are connected to the same server - assert_eq!( - conn_info_a.server_address, conn_info_b.server_address, - "Both clients should be connected to the same server" + // Verify both clients are connected to the same server. Client A holds + // the address it was configured with and client B the one the roster + // published for the leader, so the spellings differ even when the node + // is the same. + assert!( + cluster::is_same_endpoint(&conn_info_a.server_address, &conn_info_b.server_address) + .expect("Server addresses should resolve"), + "Both clients should be connected to the same server, got {} and {}", + conn_info_a.server_address, + conn_info_b.server_address ); // Verify both can communicate diff --git a/core/common/src/traits/binary_impls/personal_access_tokens.rs b/core/common/src/traits/binary_impls/personal_access_tokens.rs index 7e1299f284..8404bcc4c2 100644 --- a/core/common/src/traits/binary_impls/personal_access_tokens.rs +++ b/core/common/src/traits/binary_impls/personal_access_tokens.rs @@ -18,8 +18,9 @@ use crate::traits::binary_auth::fail_if_not_authenticated; use crate::wire_conversions::personal_access_tokens_from_wire; use crate::{ - BinaryClient, ClientState, DiagnosticEvent, IdentityInfo, IggyError, PersonalAccessTokenClient, - PersonalAccessTokenExpiry, PersonalAccessTokenInfo, RawPersonalAccessToken, + BinaryClient, ClientState, Credentials, DiagnosticEvent, IdentityInfo, IggyError, + PersonalAccessTokenClient, PersonalAccessTokenExpiry, PersonalAccessTokenInfo, + RawPersonalAccessToken, }; use iggy_binary_protocol::MAX_WIRE_NAME_LENGTH; use iggy_binary_protocol::WireName; @@ -134,6 +135,11 @@ impl PersonalAccessTokenClient for B { "authenticated against iggy server" ); self.set_state(ClientState::Authenticated).await; + self.remember_session_credentials( + Credentials::PersonalAccessToken(SecretString::from(token.to_string())), + wire_resp.user_id, + ) + .await; self.publish_event(DiagnosticEvent::SignedIn).await; Ok(IdentityInfo { user_id: wire_resp.user_id, diff --git a/core/common/src/traits/binary_impls/users.rs b/core/common/src/traits/binary_impls/users.rs index eb785109a6..38ee456f2d 100644 --- a/core/common/src/traits/binary_impls/users.rs +++ b/core/common/src/traits/binary_impls/users.rs @@ -18,8 +18,8 @@ use crate::traits::binary_auth::fail_if_not_authenticated; use crate::wire_conversions::{identifier_to_wire, permissions_to_wire, users_from_wire}; use crate::{ - BinaryClient, ClientState, DiagnosticEvent, Identifier, IdentityInfo, IggyError, Permissions, - UserClient, UserInfo, UserInfoDetails, UserStatus, UserUpdateOptions, + BinaryClient, ClientState, Credentials, DiagnosticEvent, Identifier, IdentityInfo, IggyError, + Permissions, UserClient, UserInfo, UserInfoDetails, UserStatus, UserUpdateOptions, }; use iggy_binary_protocol::codec::WireEncode; use iggy_binary_protocol::codes::LOGIN_REGISTER_CODE; @@ -174,6 +174,7 @@ impl UserClient for B { .to_bytes(), ) .await?; + self.refresh_session_password(user_id, new_password).await; Ok(()) } @@ -218,6 +219,14 @@ impl UserClient for B { "authenticated against iggy server" ); self.set_state(ClientState::Authenticated).await; + self.remember_session_credentials( + Credentials::UsernamePassword( + username.to_owned(), + SecretString::from(password.to_string()), + ), + wire_resp.user_id, + ) + .await; self.publish_event(DiagnosticEvent::SignedIn).await; Ok(IdentityInfo { user_id: wire_resp.user_id, @@ -229,6 +238,7 @@ impl UserClient for B { fail_if_not_authenticated(self).await?; self.send_raw_with_response(LOGOUT_USER_CODE, LogoutUserRequest.to_bytes()) .await?; + self.forget_session_credentials().await; self.reset_vsr_session().await?; self.set_state(ClientState::Connected).await; self.publish_event(DiagnosticEvent::SignedOut).await; diff --git a/core/common/src/traits/binary_transport.rs b/core/common/src/traits/binary_transport.rs index 4c12db04bc..91804ddb8a 100644 --- a/core/common/src/traits/binary_transport.rs +++ b/core/common/src/traits/binary_transport.rs @@ -15,7 +15,9 @@ // specific language governing permissions and limitations // under the License. -use crate::{ClientState, DiagnosticEvent, IggyError, NonZeroIggyDuration}; +use crate::{ + ClientState, Credentials, DiagnosticEvent, Identifier, IggyError, NonZeroIggyDuration, +}; use async_trait::async_trait; use bytes::Bytes; use std::sync::Arc; @@ -51,6 +53,26 @@ mod vsr_session_sealed { pub trait VsrSessionControl: vsr_session_sealed::Sealed + BinaryTransport { async fn bind_vsr_session(&self, session: u64) -> Result<(), IggyError>; async fn reset_vsr_session(&self) -> Result<(), IggyError>; + /// Keep the credentials a sign-in succeeded with, so a transport that + /// loses its connection can re-establish the session -- on this node or, + /// after failing over, on another one. A caller that signs in by hand is + /// otherwise less reconnectable than one that configures `AutoLogin`, + /// which is a surprising difference between two ways of doing the same + /// thing. Transports that cannot reconnect leave this a no-op. + async fn remember_session_credentials(&self, _credentials: Credentials, _user_id: u32) {} + /// Drop them: after an explicit logout there is no session to restore, + /// and a reconnect must not resurrect one. + async fn forget_session_credentials(&self) {} + /// A committed password change for `user`: when it is the signed-in user, + /// the credentials the next reconnect signs in with switch to the new + /// password, or that reconnect would replay the old one and fail an + /// unrelated request with `InvalidCredentials`. Other users' changes are + /// ignored. + /// + /// This covers a configured `AutoLogin` as well as a sign-in the caller + /// ran: the configured credentials still decide *who* the client signs in + /// as, and a committed change decides what that user's password is. + async fn refresh_session_password(&self, _user: &Identifier, _new_password: &str) {} /// SDK crate version sent in the login-register version prefix. /// Implemented by the transports so the value is the SDK crate's own /// `CARGO_PKG_VERSION` (`iggy` for Rust), not `iggy_common`'s. diff --git a/core/common/src/types/configuration/tcp_config/tcp_client_config.rs b/core/common/src/types/configuration/tcp_config/tcp_client_config.rs index b60f4b3ad0..44480c3377 100644 --- a/core/common/src/types/configuration/tcp_config/tcp_client_config.rs +++ b/core/common/src/types/configuration/tcp_config/tcp_client_config.rs @@ -26,6 +26,12 @@ use std::str::FromStr; pub struct TcpClientConfig { /// The address of the Iggy server. pub server_address: String, + /// Addresses of other nodes of the same cluster, dialed in order when + /// `server_address` cannot be reached. The roster the server reports is + /// remembered while the client is connected and dialed first, so these + /// seeds only have to be enough to reach the cluster once -- at the very + /// first connect, when nothing has been learned yet. + pub failover_addresses: Vec, /// Whether to use TLS when connecting to the server. pub tls_enabled: bool, /// The domain to use for TLS when connecting to the server. @@ -49,6 +55,7 @@ impl Default for TcpClientConfig { fn default() -> TcpClientConfig { TcpClientConfig { server_address: "127.0.0.1:8090".to_string(), + failover_addresses: Vec::new(), tls_enabled: false, tls_domain: "".to_string(), tls_ca_file: None, @@ -65,6 +72,10 @@ impl From> for TcpClientConfig { fn from(connection_string: ConnectionString) -> Self { TcpClientConfig { server_address: connection_string.server_address().into(), + // The connection-string grammar names a single host, so a client + // built from one starts with no seeds and learns the roster once + // it is connected. + failover_addresses: Vec::new(), auto_login: connection_string.auto_login().to_owned(), tls_enabled: connection_string.options().tls_enabled(), tls_domain: connection_string.options().tls_domain().into(), diff --git a/core/common/src/types/configuration/tcp_config/tcp_client_config_builder.rs b/core/common/src/types/configuration/tcp_config/tcp_client_config_builder.rs index 943b69f53e..08e50a9b6e 100644 --- a/core/common/src/types/configuration/tcp_config/tcp_client_config_builder.rs +++ b/core/common/src/types/configuration/tcp_config/tcp_client_config_builder.rs @@ -23,6 +23,7 @@ use crate::{ /// Builder for the TCP client configuration. /// Allows configuring the TCP client with custom settings or using defaults: /// - `server_address`: Default is "127.0.0.1:8090" +/// - `failover_addresses`: Default is empty. /// - `auto_login`: Default is AutoLogin::Disabled. /// - `reconnection`: Default is enabled unlimited retries and 1 second interval. /// - `tls_enabled`: Default is false. @@ -44,6 +45,13 @@ impl TcpClientConfigBuilder { self } + /// Sets the addresses of other nodes of the same cluster, dialed in order + /// when `server_address` cannot be reached. + pub fn with_failover_addresses(mut self, failover_addresses: Vec) -> Self { + self.config.failover_addresses = failover_addresses; + self + } + /// Sets the auto sign in during connection. pub fn with_auto_sign_in(mut self, auto_sign_in: AutoLogin) -> Self { self.config.auto_login = auto_sign_in; @@ -108,6 +116,10 @@ impl TcpClientConfigBuilder { pub fn build(mut self) -> Result { self.config.server_address = self.config.server_address.trim().to_owned(); validate_server_address(&self.config.server_address)?; + for failover_address in &mut self.config.failover_addresses { + *failover_address = failover_address.trim().to_owned(); + validate_server_address(failover_address)?; + } Ok(self.config) } @@ -185,6 +197,32 @@ mod tests { )); } + #[test] + fn valid_failover_addresses_should_succeed() { + let config = builder_with_address("127.0.0.1:8090") + .with_failover_addresses(vec![ + " 127.0.0.1:8091 ".to_string(), + "iggy-server-3:8090".to_string(), + ]) + .build() + .expect("build the configuration"); + + assert_eq!( + config.failover_addresses, + vec!["127.0.0.1:8091", "iggy-server-3:8090"] + ); + } + + #[test] + fn malformed_failover_address_should_fail() { + let builder = builder_with_address("127.0.0.1:8090") + .with_failover_addresses(vec!["127.0.0.1".to_string()]); + assert!(matches!( + builder.build(), + Err(IggyError::InvalidIpAddress(_, _)) + )); + } + #[test] fn docker_compose_service_name_should_succeed() { let builder = builder_with_address("iggy-server:8090"); diff --git a/core/integration/tests/cluster/failover_client_continuity.rs b/core/integration/tests/cluster/failover_client_continuity.rs index ab4c93f82f..9eeed6c0dd 100644 --- a/core/integration/tests/cluster/failover_client_continuity.rs +++ b/core/integration/tests/cluster/failover_client_continuity.rs @@ -15,19 +15,19 @@ // specific language governing permissions and limitations // under the License. -//! RED SPEC, expected to FAIL: client continuity across a primary SIGKILL. +//! Client continuity across a primary SIGKILL. //! //! A producing SDK client pinned to the primary must, after the primary dies, //! complete its next operation against the surviving quorum within a small -//! budget and without an authentication error. The SDK cannot: it has no -//! multi-endpoint failover. A client is built around a single -//! `server_address`, so it knows no other endpoint to dial; the transport's -//! fail-fast gate (auto-login disabled, the shape this harness client runs -//! with) returns errors without attempting a reconnect; and the -//! leader-redirect machinery that could reroute it needs a live connection to -//! read the cluster roster. Every retry therefore redials the dead endpoint -//! and fails with a connection error until the caller gives up. Surviving a -//! primary crash needs a seed roster of endpoints, not just a redirect. +//! budget and without an authentication error. Three separate pieces of +//! client state make that possible, and the test fails if any one of them is +//! lost: the endpoints the cluster roster named while the connection was +//! healthy (the roster is unreachable exactly when it is needed), the +//! credentials the sign-in succeeded with (this harness client signs in by +//! hand rather than configuring `AutoLogin`, and a reconnect has to +//! re-establish the session on whichever node answers), and a reconnect that +//! dials those endpoints in turn instead of redialing the address the client +//! was configured with. use std::time::Duration; @@ -64,8 +64,6 @@ fn build_message(payload: &str) -> IggyMessage { /// A producing client pinned to the primary; SIGKILL the primary mid-stream; /// the same client's next send must succeed against the surviving quorum /// within `RESUME_BUDGET` and must never surface Unauthenticated. -// TODO(hubcio): fix this test -#[ignore = "SDK has no multi-endpoint failover; client redials the dead primary forever"] #[iggy_harness(cluster_nodes = 3)] async fn given_a_client_producing_when_its_primary_is_killed_should_resume_without_hang_or_unauthenticated( harness: &mut TestHarness, @@ -97,6 +95,11 @@ async fn given_a_client_producing_when_its_primary_is_killed_should_resume_witho // Pin the producing client to the primary's own endpoint, the way a // leader-aware SDK ends up connected to whichever node answers as leader. let leader = disk::leader_node_index(harness).await; + let primary_endpoint = harness + .node(leader) + .tcp_addr() + .expect("leader exposes a TCP endpoint") + .to_string(); let producer = harness .node(leader) .tcp_client() @@ -106,6 +109,12 @@ async fn given_a_client_producing_when_its_primary_is_killed_should_resume_witho .await .expect("connect the producer to the primary"); + assert_eq!( + producer.get_connection_info().await.server_address, + primary_endpoint, + "the producer must be pinned to the node this test kills, or it proves nothing" + ); + let stream = Identifier::named(STREAM_NAME).unwrap(); let topic = Identifier::named(TOPIC_NAME).unwrap(); let partitioning = Partitioning::partition_id(PARTITION_ID); @@ -160,11 +169,15 @@ async fn given_a_client_producing_when_its_primary_is_killed_should_resume_witho assert!( resumed, "a client pinned to a killed primary must complete its next operation against \ - the surviving quorum within {RESUME_BUDGET:?}, but the SDK has no \ - multi-endpoint failover: it holds only the dead node's server_address, its \ - fail-fast gate (auto-login disabled) surfaces errors without reconnecting, \ - and the leader redirect that could reroute it needs a live connection to \ - read the roster, so every retry redialed the dead endpoint \ + the surviving quorum within {RESUME_BUDGET:?}: the roster learned while the \ + connection was healthy names the survivors, and the credentials the sign-in \ + succeeded with re-establish the session on whichever one answers \ ({attempt} attempts, last error: {last_error:?})" ); + assert_ne!( + producer.get_connection_info().await.server_address, + primary_endpoint, + "the send that resumed must have landed on a survivor, so the client has to \ + have moved off the killed primary's endpoint" + ); } diff --git a/core/integration/tests/sdk/disconnect_relogin.rs b/core/integration/tests/sdk/disconnect_relogin.rs new file mode 100644 index 0000000000..956e17c5a2 --- /dev/null +++ b/core/integration/tests/sdk/disconnect_relogin.rs @@ -0,0 +1,66 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! An explicit disconnect ends the session for good: the credentials a manual +//! sign-in remembered (for reconnecting across involuntary drops and +//! failovers) must not resurrect it. Pins at the Rust layer the contract the +//! C++ e2e suite asserts through the FFI (`DisconnectThenReconnectWithoutRelogin`, +//! `GetStatsBeforeLoginThrows`), so a regression fails here first instead of +//! three suites downstream. + +use iggy::prelude::*; +use integration::iggy_harness; + +#[iggy_harness] +async fn given_a_logged_in_client_when_explicitly_disconnected_should_require_a_fresh_login( + harness: &TestHarness, +) { + let client = harness.new_client().await.unwrap(); + client + .login_user(DEFAULT_ROOT_USERNAME, DEFAULT_ROOT_PASSWORD) + .await + .unwrap(); + client.get_me().await.expect("authenticated get_me works"); + + client.disconnect().await.unwrap(); + client.connect().await.unwrap(); + assert!( + matches!(client.get_me().await, Err(IggyError::Unauthenticated)), + "an explicit disconnect is caller intent, like a logout: the sign-in it ended \ + must not be silently replayed by the reconnect, so the client's own \ + authentication gate refuses the request before it is sent" + ); + + client.disconnect().await.unwrap(); + assert!( + matches!(client.get_stats().await, Err(IggyError::NotConnected)), + "an operation after an explicit disconnect must fail on the dead transport \ + instead of reconnecting into a resurrected session" + ); + + // The remembered sign-in exists for involuntary drops; a fresh manual + // login after the disconnect works exactly as before. + client.connect().await.unwrap(); + client + .login_user(DEFAULT_ROOT_USERNAME, DEFAULT_ROOT_PASSWORD) + .await + .unwrap(); + client + .get_me() + .await + .expect("a fresh login restores service"); +} diff --git a/core/integration/tests/sdk/mod.rs b/core/integration/tests/sdk/mod.rs index 3b934e8c95..065b257a76 100644 --- a/core/integration/tests/sdk/mod.rs +++ b/core/integration/tests/sdk/mod.rs @@ -18,6 +18,7 @@ mod consumer_group; mod consumer_group_membership; mod consumer_offset; +mod disconnect_relogin; mod hello_world; mod http_refresh; mod options; diff --git a/core/sdk/src/client_provider.rs b/core/sdk/src/client_provider.rs index 6b431562a3..82d6d82041 100644 --- a/core/sdk/src/client_provider.rs +++ b/core/sdk/src/client_provider.rs @@ -135,6 +135,9 @@ impl ClientProviderConfig { TransportProtocol::Tcp => { config.tcp = Some(Arc::new(TcpClientConfig { server_address: args.tcp_server_address, + // Command-line arguments name a single server; the roster + // is learned once the client is connected. + failover_addresses: Vec::new(), tls_enabled: args.tcp_tls_enabled, tls_domain: args.tcp_tls_domain, tls_ca_file: args.tcp_tls_ca_file, diff --git a/core/sdk/src/clients/binary_personal_access_tokens.rs b/core/sdk/src/clients/binary_personal_access_tokens.rs index ca19ce5e48..cfbe169d5e 100644 --- a/core/sdk/src/clients/binary_personal_access_tokens.rs +++ b/core/sdk/src/clients/binary_personal_access_tokens.rs @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +use crate::clients::redirect_login_settled; use crate::prelude::{ClientWrapper, IggyClient}; use async_trait::async_trait; use iggy_common::locking::IggyRwLockFn; @@ -77,6 +78,15 @@ impl PersonalAccessTokenClient for IggyClient { if should_redirect { info!("Redirected to leader, reconnecting and re-authenticating"); self.connect().await?; + // The reconnect signs in with the credentials this very call just + // remembered, so on a client without a configured `AutoLogin` the + // session is already this user's: signing in again would cost a + // logout and a second login (an argon2 each, on the server) for + // nothing. With `AutoLogin::Enabled` the reconnect signed in the + // configured user, who may not be this one, so the login runs. + if redirect_login_settled(&*self.client.read().await).await { + return Ok(identity); + } self.login_with_personal_access_token(token).await } else { Ok(identity) diff --git a/core/sdk/src/clients/binary_users.rs b/core/sdk/src/clients/binary_users.rs index cab16bcf61..606e11e445 100644 --- a/core/sdk/src/clients/binary_users.rs +++ b/core/sdk/src/clients/binary_users.rs @@ -16,6 +16,7 @@ // under the License. use crate::client_wrappers::client_wrapper::ClientWrapper; +use crate::clients::redirect_login_settled; use crate::prelude::IggyClient; use async_trait::async_trait; use iggy_common::UserUpdateOptions; @@ -116,6 +117,15 @@ impl UserClient for IggyClient { if should_redirect { info!("Redirected to leader, reconnecting and re-authenticating"); self.connect().await?; + // The reconnect signs in with the credentials this very call just + // remembered, so on a client without a configured `AutoLogin` the + // session is already this user's: signing in again would cost a + // logout and a second login (an argon2 each, on the server) for + // nothing. With `AutoLogin::Enabled` the reconnect signed in the + // configured user, who may not be this one, so the login runs. + if redirect_login_settled(&*self.client.read().await).await { + return Ok(identity); + } self.login_user(username, password).await } else { Ok(identity) diff --git a/core/sdk/src/clients/mod.rs b/core/sdk/src/clients/mod.rs index 3ad0df5a46..1a310450fa 100644 --- a/core/sdk/src/clients/mod.rs +++ b/core/sdk/src/clients/mod.rs @@ -15,6 +15,9 @@ // specific language governing permissions and limitations // under the License. +use crate::client_wrappers::client_wrapper::ClientWrapper; +use iggy_common::{BinaryTransport, ClientState}; + mod binary_cluster; mod binary_consumer_group; mod binary_consumer_offset; @@ -38,5 +41,35 @@ pub mod producer_error_callback; pub mod producer_sharding; const ORDERING: std::sync::atomic::Ordering = std::sync::atomic::Ordering::SeqCst; + +/// Whether the reconnect that followed a leader redirect already left this +/// client signed in as the user the redirected sign-in was for. +/// +/// The connect flow signs in with the credentials that sign-in just +/// remembered, so on a client without a configured `AutoLogin` the session on +/// the leader is already the right one: signing in again would run a logout +/// plus a second login, an argon2 each on the server, to arrive where the +/// client already is. With `AutoLogin::Enabled(a)` the reconnect signed in the +/// configured user instead, who need not be the one signing in here, so the +/// sign-in still has to run. +pub(crate) async fn redirect_login_settled(client: &ClientWrapper) -> bool { + let (state, auto_login_configured) = match client { + ClientWrapper::Tcp(tcp_client) => ( + tcp_client.get_state().await, + tcp_client.auto_login_configured(), + ), + ClientWrapper::Quic(quic_client) => ( + quic_client.get_state().await, + quic_client.auto_login_configured(), + ), + ClientWrapper::WebSocket(ws_client) => ( + ws_client.get_state().await, + ws_client.auto_login_configured(), + ), + _ => return false, + }; + + state == ClientState::Authenticated && !auto_login_configured +} const MAX_BATCH_LENGTH: usize = 1000000; const MIB: usize = 1_048_576; diff --git a/core/sdk/src/leader_aware.rs b/core/sdk/src/leader_aware.rs index 35ffce9fca..66cdad35f3 100644 --- a/core/sdk/src/leader_aware.rs +++ b/core/sdk/src/leader_aware.rs @@ -18,7 +18,7 @@ use iggy_binary_protocol::codes::GET_CLUSTER_METADATA_CODE; use iggy_common::ClusterClient; use iggy_common::{ - ClusterMetadata, ClusterNodeRole, ClusterNodeStatus, IggyError, TransportProtocol, + ClusterMetadata, ClusterNode, ClusterNodeRole, ClusterNodeStatus, IggyError, TransportProtocol, }; use std::net::SocketAddr; use std::str::FromStr; @@ -38,12 +38,33 @@ pub(crate) fn is_unauthenticated_metadata_probe(code: u32, error: &IggyError) -> code == GET_CLUSTER_METADATA_CODE && matches!(error, IggyError::Unauthenticated) } +/// What one leader check learned from the cluster roster. +pub struct LeaderCheck { + /// The leader's address, when it is not the node the client is on. + pub redirect: Option, + /// Every endpoint the roster named for this transport. A client keeps + /// them as failover candidates: the address it was configured with dies + /// with its node, and the roster is unreachable exactly when it is + /// needed, so it has to be remembered while the connection is healthy. + pub endpoints: Vec, +} + +impl LeaderCheck { + /// A check that learned nothing: stay where we are, remember no endpoint. + fn inconclusive() -> Self { + Self { + redirect: None, + endpoints: Vec::new(), + } + } +} + /// Check if we need to redirect to leader and return the leader address if redirection is needed pub async fn check_and_redirect_to_leader( client: &C, current_address: &str, transport: TransportProtocol, -) -> Result, IggyError> { +) -> Result { debug!("Checking cluster metadata for leader detection"); // A cluster can be transiently leaderless: a restarted node cedes the @@ -60,15 +81,31 @@ pub async fn check_and_redirect_to_leader( metadata.nodes.len(), metadata.name ); - match process_cluster_metadata(&metadata, current_address, transport) { - Outcome::Redirect(address) => return Ok(Some(address)), - Outcome::LeaderIsCurrent => return Ok(None), + let endpoints = transport_endpoints(&metadata, transport); + match process_cluster_metadata(&metadata, current_address, transport).await { + Outcome::Redirect(address) => { + return Ok(LeaderCheck { + redirect: Some(address), + endpoints, + }); + } + Outcome::LeaderIsCurrent => { + return Ok(LeaderCheck { + redirect: None, + endpoints, + }); + } Outcome::NoLeader => { if tokio::time::Instant::now() >= deadline { warn!( "No active leader found in cluster metadata within {LEADERLESS_WAIT_BUDGET:?}, connection will continue on server node {current_address}", ); - return Ok(None); + // A leaderless roster still names where the nodes + // are, and that is what failover needs. + return Ok(LeaderCheck { + redirect: None, + endpoints, + }); } tokio::time::sleep(LEADERLESS_POLL_INTERVAL).await; } @@ -82,14 +119,14 @@ pub async fn check_and_redirect_to_leader( debug!( "Cluster metadata answered Unauthenticated; the session is gone, connection will continue on server node {current_address}" ); - return Ok(None); + return Ok(LeaderCheck::inconclusive()); } Err(e) => { warn!( "Failed to get cluster metadata: {}, connection will continue on server node {}", e, current_address ); - return Ok(None); + return Ok(LeaderCheck::inconclusive()); } } } @@ -110,8 +147,31 @@ enum Outcome { NoLeader, } +/// Every node's address for `transport`, in roster order. A node that does +/// not expose the transport reports port 0 and is skipped: dialing it would +/// burn a failover attempt on an endpoint that cannot answer. +fn transport_endpoints(metadata: &ClusterMetadata, transport: TransportProtocol) -> Vec { + metadata + .nodes + .iter() + .filter_map(|node| { + let port = transport_port(node, transport); + (port != 0).then(|| format!("{}:{port}", node.ip)) + }) + .collect() +} + +fn transport_port(node: &ClusterNode, transport: TransportProtocol) -> u16 { + match transport { + TransportProtocol::Tcp => node.endpoints.tcp, + TransportProtocol::Quic => node.endpoints.quic, + TransportProtocol::Http => node.endpoints.http, + TransportProtocol::WebSocket => node.endpoints.websocket, + } +} + /// Process cluster metadata and determine if redirection is needed -fn process_cluster_metadata( +async fn process_cluster_metadata( metadata: &ClusterMetadata, current_address: &str, transport: TransportProtocol, @@ -132,12 +192,7 @@ fn process_cluster_metadata( match leader { Some(leader_node) => { - let leader_port = match transport { - TransportProtocol::Tcp => leader_node.endpoints.tcp, - TransportProtocol::Quic => leader_node.endpoints.quic, - TransportProtocol::Http => leader_node.endpoints.http, - TransportProtocol::WebSocket => leader_node.endpoints.websocket, - }; + let leader_port = transport_port(leader_node, transport); let leader_address = format!("{}:{}", leader_node.ip, leader_port); info!( @@ -145,7 +200,7 @@ fn process_cluster_metadata( leader_node.name, leader_address, transport ); - if !is_same_address(current_address, &leader_address) { + if !is_same_address(current_address, &leader_address).await { info!( "Current connection to {} is not the leader, will redirect to {}", current_address, leader_address @@ -160,15 +215,70 @@ fn process_cluster_metadata( } } -/// Check if two addresses refer to the same endpoint -/// Handles various formats like 127.0.0.1:8090 vs localhost:8090 -fn is_same_address(addr1: &str, addr2: &str) -> bool { +/// Whether two addresses are written the same way, up to canonicalization +/// (`localhost` and `[::]` spellings, and a literal address compared as an +/// address rather than as text). +/// +/// Cheap and non-blocking, which is the whole point: the resolving comparison +/// below is a `getaddrinfo`, and every caller reaches this first. +pub(crate) fn is_same_spelling(addr1: &str, addr2: &str) -> bool { match (parse_address(addr1), parse_address(addr2)) { (Some(sock1), Some(sock2)) => sock1.ip() == sock2.ip() && sock1.port() == sock2.port(), _ => normalize_address(addr1) == normalize_address(addr2), } } +/// Check if two addresses refer to the same endpoint +/// Handles various formats like 127.0.0.1:8090 vs localhost:8090 +/// +/// A host name and the address it resolves to are one endpoint too: a client +/// configured as `iggy-server:8090` whose roster advertises `10.0.0.5:8090` +/// would otherwise dial that node twice per failover sweep, and a single-node +/// deployment would be treated as a cluster. +/// +/// Resolution is the last resort, only when the spellings differ and at least +/// one side is not a literal address. It runs through the runtime's resolver +/// rather than `ToSocketAddrs`: name lookup is a blocking `getaddrinfo`, and +/// this is called from the connect and redirect paths, where stalling a +/// runtime worker on a slow resolver would stall every task sharing it. +pub(crate) async fn is_same_address(addr1: &str, addr2: &str) -> bool { + is_same_address_with(addr1, addr2, resolve_all).await +} + +/// [`is_same_address`] against a caller-provided resolver, so the fallback can +/// be exercised without depending on what the machine's resolver answers. +async fn is_same_address_with(addr1: &str, addr2: &str, resolve: R) -> bool +where + R: Fn(String) -> F, + F: Future>>, +{ + if is_same_spelling(addr1, addr2) { + return true; + } + + // Two literal addresses that did not compare equal are different + // endpoints; resolving them would only hand back what they already say. + if parse_address(addr1).is_some() && parse_address(addr2).is_some() { + return false; + } + + let (Some(first), Some(second)) = ( + resolve(addr1.to_owned()).await, + resolve(addr2.to_owned()).await, + ) else { + return false; + }; + first.iter().any(|resolved| second.contains(resolved)) +} + +/// Every socket address a host:port spelling resolves to, `None` when the +/// resolver does not know the name (which then compares unequal, at worst +/// costing one extra dial). +async fn resolve_all(addr: String) -> Option> { + let resolved: Vec = tokio::net::lookup_host(addr).await.ok()?.collect(); + (!resolved.is_empty()).then_some(resolved) +} + /// Parse address string to SocketAddr, handling various formats fn parse_address(addr: &str) -> Option { if let Ok(socket_addr) = SocketAddr::from_str(addr) { @@ -245,12 +355,84 @@ mod tests { )); } + fn node(name: &str, ip: &str, tcp: u16, role: ClusterNodeRole) -> ClusterNode { + ClusterNode { + name: name.to_string(), + ip: ip.to_string(), + endpoints: iggy_common::TransportEndpoints::new(tcp, 0, 3000, 3001), + role, + status: ClusterNodeStatus::Healthy, + } + } + #[test] - fn test_is_same_address() { - assert!(is_same_address("127.0.0.1:8090", "127.0.0.1:8090")); - assert!(is_same_address("localhost:8090", "127.0.0.1:8090")); - assert!(!is_same_address("127.0.0.1:8090", "127.0.0.1:8091")); - assert!(!is_same_address("192.168.1.1:8090", "127.0.0.1:8090")); + fn the_roster_names_every_node_that_exposes_the_transport() { + let metadata = ClusterMetadata { + name: "iggy".to_string(), + nodes: vec![ + node("iggy-1", "10.0.0.1", 8090, ClusterNodeRole::Leader), + node("iggy-2", "10.0.0.2", 8090, ClusterNodeRole::Follower), + node("iggy-3", "10.0.0.3", 8090, ClusterNodeRole::Follower), + ], + }; + + assert_eq!( + transport_endpoints(&metadata, TransportProtocol::Tcp), + vec!["10.0.0.1:8090", "10.0.0.2:8090", "10.0.0.3:8090"] + ); + // A node that does not expose the transport reports port 0; dialing + // it would burn a failover attempt on an endpoint that cannot answer. + assert!(transport_endpoints(&metadata, TransportProtocol::Quic).is_empty()); + } + + #[tokio::test] + async fn test_is_same_address() { + assert!(is_same_address("127.0.0.1:8090", "127.0.0.1:8090").await); + assert!(is_same_address("localhost:8090", "127.0.0.1:8090").await); + assert!(!is_same_address("127.0.0.1:8090", "127.0.0.1:8091").await); + assert!(!is_same_address("192.168.1.1:8090", "127.0.0.1:8090").await); + } + + /// A stand-in resolver: the BDD cluster's spelling of one node, which no + /// canonicalization rewrites, so only the resolving comparison can equate + /// the two. `None` for anything else, like a name the resolver does not + /// know. + async fn resolve_bdd_leader(addr: String) -> Option> { + match addr.as_str() { + "iggy-leader:8091" | "172.28.0.101:8091" => { + Some(vec![SocketAddr::from(([172, 28, 0, 101], 8091))]) + } + _ => None, + } + } + + // A host name and the address it resolves to name one endpoint. Exactly + // the case the BDD cluster hits: the client dials `iggy-leader:8091` and + // the roster advertises `172.28.0.101:8091`. + #[tokio::test] + async fn a_host_name_matches_the_address_it_resolves_to() { + assert!( + is_same_address_with("iggy-leader:8091", "172.28.0.101:8091", resolve_bdd_leader).await + ); + } + + // A name the resolver does not know compares unequal rather than + // erroring, and a resolvable name never matches another port. + #[tokio::test] + async fn an_unresolvable_name_or_another_port_is_a_different_endpoint() { + assert!( + !is_same_address_with( + "iggy-follower:8092", + "172.28.0.101:8091", + resolve_bdd_leader + ) + .await + ); + assert!( + !is_same_address_with("iggy-leader:8091", "172.28.0.101:8092", resolve_bdd_leader) + .await + ); + assert!(!is_same_address("no-such-host.invalid:8090", "127.0.0.1:8090").await); } #[test] diff --git a/core/sdk/src/quic/quic_client.rs b/core/sdk/src/quic/quic_client.rs index b253205548..b1f423a376 100644 --- a/core/sdk/src/quic/quic_client.rs +++ b/core/sdk/src/quic/quic_client.rs @@ -230,6 +230,12 @@ impl iggy_common::VsrSessionControl for QuicClient { impl BinaryClient for QuicClient {} impl QuicClient { + /// Whether an `AutoLogin` is configured on this client, which makes the + /// session after any connect the configured user's rather than whoever + /// signed in by hand. + pub(crate) fn auto_login_configured(&self) -> bool { + matches!(self.config.auto_login, AutoLogin::Enabled(_)) + } /// Creates a new QUIC client for the provided client and server addresses. pub fn new( client_address: &str, @@ -510,12 +516,15 @@ impl QuicClient { /// Returns true if redirection occurred and reconnection is needed. pub(crate) async fn handle_leader_redirection(&self) -> Result { let current_address = self.current_server_address.lock().await.clone(); + // The roster's other endpoints are dropped here: only the TCP client + // dials failover candidates so far. let leader_address = check_and_redirect_to_leader( self, ¤t_address, iggy_common::TransportProtocol::Quic, ) - .await?; + .await? + .redirect; if let Some(new_leader_address) = leader_address { let mut redirection_state = self.leader_redirection_state.lock().await; diff --git a/core/sdk/src/tcp/tcp_client.rs b/core/sdk/src/tcp/tcp_client.rs index 1edd3a029f..ba3080bc59 100644 --- a/core/sdk/src/tcp/tcp_client.rs +++ b/core/sdk/src/tcp/tcp_client.rs @@ -16,7 +16,8 @@ // under the License. use crate::leader_aware::{ - LeaderRedirectionState, check_and_redirect_to_leader, is_unauthenticated_metadata_probe, + LeaderRedirectionState, check_and_redirect_to_leader, is_same_address, + is_unauthenticated_metadata_probe, }; use crate::prelude::Client; use crate::prelude::TcpClientConfig; @@ -24,23 +25,29 @@ use crate::session::ConsensusSession; use crate::tcp::tcp_connection_stream::TcpConnectionStream; use crate::tcp::tcp_connection_stream_kind::ConnectionStreamKind; use crate::tcp::tcp_tls_connection_stream::TcpTlsConnectionStream; +use crate::vsr::operation_for_code; use async_broadcast::{Receiver, Sender, broadcast}; use async_trait::async_trait; use bytes::{Bytes, BytesMut}; use iggy_binary_protocol::codes::{LOGIN_REGISTER_CODE, LOGIN_REGISTER_WITH_PAT_CODE}; +use iggy_binary_protocol::consensus::Operation; +#[cfg(test)] +use iggy_common::TcpClientReconnectionConfig; use iggy_common::VsrSessionControl as _; use iggy_common::{ AutoLogin, ClientState, ConnectionString, ConnectionStringUtils, Credentials, DiagnosticEvent, - IggyDuration, IggyError, IggyTimestamp, NonZeroIggyDuration, TcpConnectionStringOptions, - TransportProtocol, + IdKind, Identifier, IggyDuration, IggyError, IggyTimestamp, NonZeroIggyDuration, + TcpConnectionStringOptions, TransportProtocol, }; use iggy_common::{BinaryClient, BinaryTransport, PersonalAccessTokenClient, UserClient}; use rustls::pki_types::{CertificateDer, ServerName, pem::PemObject}; -use secrecy::ExposeSecret; +use secrecy::{ExposeSecret, SecretString}; use std::net::SocketAddr; use std::str::FromStr; use std::sync::Arc; use std::sync::Mutex as StdMutex; +#[cfg(test)] +use tokio::net::TcpListener; use tokio::net::TcpStream; use tokio::sync::Mutex; use tokio::time::sleep; @@ -69,6 +76,13 @@ const NOT_READY_RETRY_INTERVAL: std::time::Duration = std::time::Duration::from_ /// overall. const TRANSIENT_FAILOVER_CHECK_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2); +/// Bound on one dial while the client has other endpoints to try. A host +/// that drops the SYN -- powered off, or partitioned away -- takes the OS +/// connect timeout to fail, which is minutes, and every other endpoint waits +/// behind it. A client that knows a single endpoint has nothing to starve, so +/// its dial stays unbounded. +const FAILOVER_DIAL_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(2); + /// TCP client for interacting with the Iggy API. /// It requires a valid server address. #[derive(Debug)] @@ -81,6 +95,15 @@ pub struct TcpClient { pub(crate) connected_at: Mutex>, leader_redirection_state: Mutex, pub(crate) current_server_address: Mutex, + /// Every endpoint the cluster roster named, refreshed on each leader + /// check. A node dies together with its address, and the roster is + /// unreachable exactly when it is needed, so the client has to have + /// remembered it while the connection was still healthy. + roster_endpoints: Mutex>, + /// Credentials a sign-in on this client succeeded with, so a reconnect -- + /// onto this node or, after a failover, another one -- can re-establish + /// the session instead of surfacing `Unauthenticated`. Cleared on logout. + session_credentials: Mutex>, // `std::sync::Mutex` (not `tokio::sync::Mutex`): the critical section // is `encode_request_header`, which is pure CPU and never awaits. The // tokio variant would pay a waker alloc + internal semaphore on @@ -90,6 +113,27 @@ pub struct TcpClient { consumer_group_state: Arc, } +/// The sign-in a manual login on this client succeeded with, and who it signed +/// in as. The user matters because a password change has to swap the new +/// password in here, and `change_password` may well target somebody else. +#[derive(Debug)] +struct RememberedSignIn { + credentials: Credentials, + user_id: u32, + /// Set by a committed password change for `user_id`. The configured + /// `AutoLogin` credentials cannot be rewritten -- the config is shared and + /// immutable -- so this marks the remembered copy as the newer one for + /// that same user, and [`TcpClient::sign_in_credentials`] prefers it. + password_refreshed: bool, +} + +/// A connection that completed every step of coming up, TLS included. +struct EstablishedConnection { + stream: ConnectionStreamKind, + client_address: SocketAddr, + remote_address: SocketAddr, +} + impl Default for TcpClient { fn default() -> Self { TcpClient::create(Arc::new(TcpClientConfig::default())).unwrap() @@ -103,7 +147,12 @@ impl Client for TcpClient { } async fn disconnect(&self) -> Result<(), IggyError> { - TcpClient::disconnect(self).await + // An explicit disconnect is caller intent, like a logout: the session + // it ends must not be resurrected by the next reconnect, so the + // remembered sign-in goes with it. Involuntary drops (a dead socket, + // a failover) go through `disconnect_transport` and keep it. + self.forget_session_credentials().await; + TcpClient::disconnect_transport(self).await } async fn shutdown(&self) -> Result<(), IggyError> { @@ -160,16 +209,30 @@ impl BinaryTransport for TcpClient { 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() { + // With no credentials -- neither configured nor remembered from a + // sign-in -- a reconnect cannot re-establish the session, so + // non-login requests fail fast. Login/register itself is the // exception: the server stays deliberately silent on transient // register failures (the server `surface_login_failure`) and // relies on the client timing out and replaying the request. return Err(error); } - self.disconnect().await?; + // Reconnecting heals the transport, but replaying the request over the + // new connection is a second attempt under a new session: + // `reset_vsr_session` drops the client id the server's dedup fence is + // keyed on, so a replicated write that committed before its reply was + // lost would apply a second time. Replay only what provably never + // reached the log -- the errors raised before the request was written, + // and the operations that never enter it. + // + // Login and register are the exception: the server stays deliberately + // silent on a transient register failure and relies on the client + // replaying, so that replay is the protocol rather than a retry. + let replay_after_reconnect = replay_is_safe(code, &error); + + self.disconnect_transport().await?; let skip_auto_login = is_login_register_code(code); if skip_auto_login { @@ -190,6 +253,16 @@ impl BinaryTransport for TcpClient { *self.skip_auto_login_once.lock().await = false; } reconnect?; + + if !replay_after_reconnect { + warn!( + "Reconnected, but command: {code} is replicated and its outcome is unknown: \ + replaying it under the new session could apply it twice, so the original \ + error is returned instead." + ); + return Err(error); + } + self.send_raw(code, payload).await } @@ -202,6 +275,59 @@ impl BinaryTransport for TcpClient { } } +/// Whether replaying `code` over a fresh connection cannot apply it twice. +/// +/// The reconnect registers a new client identity, so the server's dedup fence +/// no longer covers the original request: only requests that provably never +/// reached the log may be re-sent. +/// +/// - the errors raised before the frame was written, and the server's own +/// refusals, which precede execution; +/// - operations that never enter the log: a non-replicated read, and a logout, +/// which ends whatever session the connection carried -- the reconnect +/// brought a new one, and refusing the replay would strand +/// `logout_before_relogin`, whose failure aborts the sign-in that was about +/// to replace the session; +/// - login and register, where the replay is the protocol: the server stays +/// deliberately silent on a transient register failure and relies on the +/// client resending. +fn replay_is_safe(code: u32, error: &IggyError) -> bool { + is_login_register_code(code) + || matches!( + error, + IggyError::NotConnected + | IggyError::CannotEstablishConnection + | IggyError::Unauthenticated + | IggyError::StaleClient + ) + || matches!( + operation_for_code(code), + Operation::NonReplicated | Operation::Logout + ) +} + +/// Why a TLS handshake failed, as far as retrying is concerned. +/// +/// A certificate this client will never accept -- the wrong CA, a name it does +/// not cover, a peer that answers a ClientHello with something else -- says the +/// same thing on every attempt. Reported as a configuration fault it ends the +/// connect after one sweep; reported as a lost connection it would be redialed +/// every interval forever under `max_retries = None`, which is how a wrong CA +/// looks like a flaky network. +fn classify_handshake_failure(error: &std::io::Error) -> IggyError { + match error + .get_ref() + .and_then(|inner| inner.downcast_ref::()) + { + Some( + rustls::Error::InvalidCertificate(_) + | rustls::Error::NoCertificatesPresented + | rustls::Error::InvalidMessage(_), + ) => IggyError::InvalidTlsCertificate, + _ => IggyError::CannotEstablishConnection, + } +} + impl iggy_common::VsrSessionSealed for TcpClient {} #[async_trait::async_trait] @@ -231,6 +357,43 @@ impl iggy_common::VsrSessionControl for TcpClient { Ok(()) } + async fn remember_session_credentials(&self, credentials: Credentials, user_id: u32) { + self.session_credentials + .lock() + .await + .replace(RememberedSignIn { + credentials, + user_id, + password_refreshed: false, + }); + } + + async fn forget_session_credentials(&self) { + self.session_credentials.lock().await.take(); + } + + async fn refresh_session_password(&self, user: &Identifier, new_password: &str) { + let mut remembered = self.session_credentials.lock().await; + let Some(sign_in) = remembered.as_mut() else { + return; + }; + // A personal access token is not derived from the password. + let Credentials::UsernamePassword(username, password) = &mut sign_in.credentials else { + return; + }; + + let targets_session_user = match user.kind { + IdKind::Numeric => user.get_u32_value().is_ok_and(|id| id == sign_in.user_id), + IdKind::String => user + .get_cow_str_value() + .is_ok_and(|name| name.as_ref() == username), + }; + if targets_session_user { + *password = SecretString::from(new_password.to_owned()); + sign_in.password_refreshed = true; + } + } + fn sdk_version(&self) -> &'static str { crate::SDK_VERSION } @@ -293,6 +456,8 @@ impl TcpClient { connected_at: Mutex::new(None), leader_redirection_state: Mutex::new(LeaderRedirectionState::new()), current_server_address: Mutex::new(server_address), + roster_endpoints: Mutex::new(Vec::new()), + session_credentials: Mutex::new(None), consensus_session: Arc::new(StdMutex::new(ConsensusSession::new())), skip_auto_login_once: Mutex::new(false), consumer_group_state: Arc::new(iggy_common::ConsumerGroupClientState::new()), @@ -321,155 +486,111 @@ 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 mut candidates = self.dial_candidates().await; + // `reestablish_after` paces reconnects to the endpoint this client + // was last on, and to that one only: the other endpoints owe it no + // cooldown, and pausing before dialing them would push the failover + // past the window the caller is willing to wait. So when there is + // somewhere else to go, the paced endpoint goes last -- by which + // time its window has usually elapsed anyway -- instead of the wait + // being skipped outright. + let paced_endpoint = self.current_server_address.lock().await.clone(); + if candidates.len() > 1 && self.reestablish_wait().await.is_some() { + candidates.rotate_left(1); } - let tls_enabled = self.config.tls_enabled; let mut retry_count = 0; let connection_stream: ConnectionStreamKind; let remote_address; let client_address; + let mut candidate = 0; + // A fault no retry can fix, remembered rather than returned at + // once: it belongs to the endpoint that raised it (a certificate + // that names another host, a domain that will not parse), and the + // endpoints behind that one may be perfectly usable. + let mut config_fault: Option = None; loop { - let server_address = self.current_server_address.lock().await.clone(); - info!( - "{NAME} client is connecting to server: {}...", - server_address - ); - - let connection = TcpStream::connect(&server_address).await; - if let Err(err) = &connection { - error!( - "Failed to connect to server: {}. Error: {}", - server_address, err - ); - if !self.config.reconnection.enabled { - warn!("Automatic reconnection is disabled."); - return Err(IggyError::CannotEstablishConnection); - } - - let unlimited_retries = self.config.reconnection.max_retries.is_none(); - let max_retries = self.config.reconnection.max_retries.unwrap_or_default(); - let max_retries_str = - if let Some(max_retries) = self.config.reconnection.max_retries { - max_retries.to_string() - } else { - "unlimited".to_string() - }; + let server_address = candidates[candidate].clone(); + if server_address == paced_endpoint + && let Some(remaining) = self.reestablish_wait().await + { + info!("Trying to connect to the server: {server_address} in: {remaining}"); + sleep(remaining.get_duration()).await; + } - let interval_str = self.config.reconnection.interval.as_human_time_string(); - if unlimited_retries || retry_count < max_retries { - retry_count += 1; - info!( - "Retrying to connect to server ({retry_count}/{max_retries_str}): {} in: {interval_str}", - server_address, - ); - sleep(self.config.reconnection.interval.get_duration()).await; - continue; + info!("{NAME} client is connecting to server: {server_address}..."); + match self.establish_bounded(&server_address, &candidates).await { + Ok(connection) => { + // The endpoint that answered is where this client now + // lives: the leader check compares against it, and the + // next reconnect starts from it. Recorded only once the + // stream is usable, so a node that accepts TCP but + // fails the TLS handshake does not become sticky and + // shadow the endpoints behind it. + *self.current_server_address.lock().await = server_address; + client_address = connection.client_address; + remote_address = connection.remote_address; + self.client_address.lock().await.replace(client_address); + connection_stream = connection.stream; + break; } - - self.set_state(ClientState::Disconnected).await; - self.publish_event(DiagnosticEvent::Disconnected).await; - return Err(IggyError::CannotEstablishConnection); + Err(IggyError::CannotEstablishConnection) => {} + Err(error) => config_fault = Some(error), } - let stream = connection.map_err(|error| { - error!("Failed to establish TCP connection to the server: {error}",); - IggyError::CannotEstablishConnection - })?; - client_address = stream.local_addr().map_err(|error| { - error!("Failed to get the local address of the client: {error}",); - IggyError::CannotEstablishConnection - })?; - remote_address = stream.peer_addr().map_err(|error| { - error!("Failed to get the remote address of the server: {error}",); - IggyError::CannotEstablishConnection - })?; - self.client_address.lock().await.replace(client_address); - - if let Err(e) = stream.set_nodelay(self.config.nodelay) { - error!("Failed to set the nodelay option on the client: {e}, continuing...",); + // Every other endpoint gets its turn before the retry + // interval: the node just lost may be gone for good, and + // pausing on it helps nothing. + candidate += 1; + if candidate < candidates.len() { + continue; } - - if !tls_enabled { - connection_stream = - ConnectionStreamKind::Tcp(TcpConnectionStream::new(client_address, stream)); - break; + candidate = 0; + + // An unreadable CA file, a certificate that names another + // host, a domain that will not parse: no endpoint answered and + // at least one said why in a way that a retry cannot change, + // so the caller gets that reason instead of a retry loop that + // buries it (`max_retries = None` would otherwise redial it + // every interval forever). + if let Some(error) = config_fault { + self.fail_connect().await; + return Err(error); } - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + // The sweep is what reconnection settings apply to, not a + // single dial: with reconnection off there are no retries, but + // the failover endpoints were configured to be tried and they + // get their one turn first. + if !self.config.reconnection.enabled { + warn!("Automatic reconnection is disabled."); + self.fail_connect().await; + return Err(IggyError::CannotEstablishConnection); + } - let config = if self.config.tls_validate_certificate { - let mut root_cert_store = rustls::RootCertStore::empty(); - if let Some(certificate_path) = &self.config.tls_ca_file { - for cert in - CertificateDer::pem_file_iter(certificate_path).map_err(|error| { - error!("Failed to read the CA file: {certificate_path}. {error}",); - IggyError::InvalidTlsCertificatePath - })? - { - let certificate = cert.map_err(|error| { - error!( - "Failed to read a certificate from the CA file: {certificate_path}. {error}", - ); - IggyError::InvalidTlsCertificate - })?; - root_cert_store.add(certificate).map_err(|error| { - error!( - "Failed to add a certificate to the root certificate store. {error}", - ); - IggyError::InvalidTlsCertificate - })?; - } + let unlimited_retries = self.config.reconnection.max_retries.is_none(); + let max_retries = self.config.reconnection.max_retries.unwrap_or_default(); + let max_retries_str = + if let Some(max_retries) = self.config.reconnection.max_retries { + max_retries.to_string() } else { - root_cert_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); - } + "unlimited".to_string() + }; - rustls::ClientConfig::builder() - .with_root_certificates(root_cert_store) - .with_no_client_auth() - } else { - use crate::tcp::tcp_tls_verifier::NoServerVerification; - rustls::ClientConfig::builder() - .dangerous() - .with_custom_certificate_verifier(Arc::new(NoServerVerification)) - .with_no_client_auth() - }; - let connector = TlsConnector::from(Arc::new(config)); - let tls_domain = if self.config.tls_domain.is_empty() { - // Extract hostname/IP from server_address when tls_domain is not specified - server_address - .split(':') - .next() - .unwrap_or(&server_address) - .to_string() - } else { - self.config.tls_domain.to_owned() - }; - let domain = ServerName::try_from(tls_domain).map_err(|error| { - error!("Failed to create a server name from the domain. {error}",); - IggyError::InvalidTlsDomain - })?; - let stream = connector.connect(domain, stream).await.map_err(|error| { - error!("Failed to establish a TLS connection to the server: {error}",); - IggyError::CannotEstablishConnection - })?; - connection_stream = ConnectionStreamKind::TcpTls(TcpTlsConnectionStream::new( - client_address, - TlsStream::Client(stream), - )); - break; + if unlimited_retries || retry_count < max_retries { + retry_count += 1; + let interval_str = self.config.reconnection.interval.as_human_time_string(); + info!( + "Retrying to connect ({retry_count}/{max_retries_str}), \ + {} endpoint(s) in: {interval_str}", + candidates.len(), + ); + sleep(self.config.reconnection.interval.get_duration()).await; + continue; + } + + self.fail_connect().await; + return Err(IggyError::CannotEstablishConnection); } let now = IggyTimestamp::now(); @@ -486,9 +607,9 @@ 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 @@ -496,26 +617,71 @@ impl TcpClient { // `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 { - Credentials::UsernamePassword(username, password) => { - self.login_user(username, password.expose_secret()).await?; - info!( - "{NAME} client: {client_address} has signed in with the user credentials, username: {username}", - ); + let signed_in = match &credentials { + Credentials::UsernamePassword(username, password) => self + .login_user(username, password.expose_secret()) + .await + .map(|_| format!("the user credentials, username: {username}")), + Credentials::PersonalAccessToken(token) => self + .login_with_personal_access_token(token.expose_secret()) + .await + .map(|_| "a personal access token".to_owned()), + }; + match signed_in { + Ok(how) => { + info!("{NAME} client: {client_address} has signed in with {how}.") } - Credentials::PersonalAccessToken(token) => { - self.login_with_personal_access_token(token.expose_secret()) - .await?; - info!( - "{NAME} client: {client_address} has signed in with a personal access token.", - ); + Err(error) => { + // With the transport up and only the session + // missing, the state has to say so: left at + // `Authenticating` every gated operation fails + // client-side with `Disconnected`, `connect()` + // returns ok without dialing, and nothing short + // of an explicit `login_user` recovers. + // + // A sign-in can also fail because the socket + // died under it. Whatever is left of that + // connection cannot carry a request, so it goes + // rather than being kept behind a `Connected` + // that makes the next `connect()` a no-op and + // leaves every gated operation failing until + // someone calls `disconnect()` by hand. + if matches!( + error, + IggyError::Disconnected + | IggyError::EmptyResponse + | IggyError::NotConnected + | IggyError::CannotEstablishConnection + | IggyError::TcpError + | IggyError::StaleClient + ) { + self.disconnect_transport().await?; + } else if self.get_state().await == ClientState::Authenticating { + self.set_state(ClientState::Connected).await; + } + // A rejected credential does not become valid on + // the next reconnect, and replaying it costs an + // argon2 on the server every time. Configured + // credentials stay as configured -- they are the + // caller's to fix -- so only the remembered + // sign-in is dropped. + if matches!( + error, + IggyError::InvalidCredentials + | IggyError::InvalidUsername + | IggyError::InvalidPassword + | IggyError::Unauthenticated + ) { + self.forget_session_credentials().await; + } + return Err(error); } } @@ -541,14 +707,22 @@ impl TcpClient { /// Returns true if redirection occurred and reconnection is needed. pub(crate) async fn handle_leader_redirection(&self) -> Result { let current_address = self.current_server_address.lock().await.clone(); - let leader_address = check_and_redirect_to_leader( + let leader_check = check_and_redirect_to_leader( self, ¤t_address, iggy_common::TransportProtocol::Tcp, ) .await?; - if let Some(new_leader_address) = leader_address { + // Replaced wholesale rather than merged: the roster is the cluster's + // own answer about where its nodes are, so a node it dropped should + // stop being dialed. The configured seeds are kept separately and + // outlive it. + if !leader_check.endpoints.is_empty() { + *self.roster_endpoints.lock().await = leader_check.endpoints; + } + + if let Some(new_leader_address) = leader_check.redirect { let mut redirection_state = self.leader_redirection_state.lock().await; if !redirection_state.can_redirect() { warn!("Maximum leader redirections reached, continuing with current connection"); @@ -564,7 +738,7 @@ impl TcpClient { // Clear connected_at to avoid reestablish_after delay during redirection self.connected_at.lock().await.take(); - self.disconnect().await?; + self.disconnect_transport().await?; *self.current_server_address.lock().await = new_leader_address; Ok(true) @@ -574,7 +748,228 @@ impl TcpClient { } } - async fn disconnect(&self) -> Result<(), IggyError> { + /// Whether an `AutoLogin` is configured on this client, which makes the + /// session after any connect the configured user's rather than whoever + /// signed in by hand. + pub(crate) fn auto_login_configured(&self) -> bool { + matches!(self.config.auto_login, AutoLogin::Enabled(_)) + } + + /// Credentials to sign in with after connecting: the configured ones, or + /// else the ones a manual sign-in on this client succeeded with. A manual + /// sign-in is otherwise less reconnectable than a configured one, which + /// is a surprising difference between two ways of doing the same thing. + async fn sign_in_credentials(&self) -> Option { + let remembered = + self.session_credentials + .lock() + .await + .as_ref() + .map(|remembered: &RememberedSignIn| { + ( + remembered.credentials.clone(), + remembered.password_refreshed, + ) + }); + + match (&self.config.auto_login, remembered) { + // A committed password change for the configured user outranks the + // configured password: the config cannot be rewritten, and signing + // in with the password this client itself replaced would fail + // `InvalidCredentials` on every later reconnect. Same user either + // way -- `refresh_session_password` only marks a change that + // targeted the signed-in one. + ( + AutoLogin::Enabled(Credentials::UsernamePassword(configured_username, _)), + Some((Credentials::UsernamePassword(username, password), true)), + ) if configured_username == &username => { + Some(Credentials::UsernamePassword(username, password)) + } + (AutoLogin::Enabled(configured), _) => Some(configured.clone()), + (AutoLogin::Disabled, remembered) => remembered.map(|(credentials, _)| credentials), + } + } + + /// Endpoints to dial for one connect, likeliest first: where the client + /// currently is, then the roster it learned while connected, then the + /// configured seeds. + async fn dial_candidates(&self) -> Vec { + let mut candidates = vec![self.current_server_address.lock().await.clone()]; + let roster = self.roster_endpoints.lock().await.clone(); + for endpoint in roster.iter().chain(self.config.failover_addresses.iter()) { + let mut known = false; + for candidate in &candidates { + if is_same_address(candidate, endpoint).await { + known = true; + break; + } + } + if !known { + candidates.push(endpoint.clone()); + } + } + candidates + } + + /// Bring one endpoint all the way up: TCP connect, socket options, and the + /// TLS handshake when it is configured. Nothing about the connection is + /// recorded until this succeeds, so a half-usable endpoint leaves no trace + /// for the next connect to lead with. + /// + /// `CannotEstablishConnection` means this endpoint failed and the next one + /// is worth trying; any other error is a configuration fault that no + /// endpoint can satisfy. + async fn establish(&self, server_address: &str) -> Result { + let stream = TcpStream::connect(server_address).await.map_err(|error| { + error!("Failed to connect to server: {server_address}. Error: {error}"); + IggyError::CannotEstablishConnection + })?; + let client_address = stream.local_addr().map_err(|error| { + error!("Failed to get the local address of the client: {error}"); + IggyError::CannotEstablishConnection + })?; + let remote_address = stream.peer_addr().map_err(|error| { + error!("Failed to get the remote address of the server: {error}"); + IggyError::CannotEstablishConnection + })?; + + if let Err(error) = stream.set_nodelay(self.config.nodelay) { + error!("Failed to set the nodelay option on the client: {error}, continuing..."); + } + + if !self.config.tls_enabled { + return Ok(EstablishedConnection { + stream: ConnectionStreamKind::Tcp(TcpConnectionStream::new(client_address, stream)), + client_address, + remote_address, + }); + } + + let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + let config = if self.config.tls_validate_certificate { + let mut root_cert_store = rustls::RootCertStore::empty(); + if let Some(certificate_path) = &self.config.tls_ca_file { + for cert in CertificateDer::pem_file_iter(certificate_path).map_err(|error| { + error!("Failed to read the CA file: {certificate_path}. {error}"); + IggyError::InvalidTlsCertificatePath + })? { + let certificate = cert.map_err(|error| { + error!( + "Failed to read a certificate from the CA file: {certificate_path}. {error}", + ); + IggyError::InvalidTlsCertificate + })?; + root_cert_store.add(certificate).map_err(|error| { + error!( + "Failed to add a certificate to the root certificate store. {error}" + ); + IggyError::InvalidTlsCertificate + })?; + } + } else { + root_cert_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + } + + rustls::ClientConfig::builder() + .with_root_certificates(root_cert_store) + .with_no_client_auth() + } else { + use crate::tcp::tcp_tls_verifier::NoServerVerification; + rustls::ClientConfig::builder() + .dangerous() + .with_custom_certificate_verifier(Arc::new(NoServerVerification)) + .with_no_client_auth() + }; + + let connector = TlsConnector::from(Arc::new(config)); + let tls_domain = if self.config.tls_domain.is_empty() { + // Extract hostname/IP from server_address when tls_domain is not specified + server_address + .split(':') + .next() + .unwrap_or(server_address) + .to_string() + } else { + self.config.tls_domain.to_owned() + }; + let domain = ServerName::try_from(tls_domain).map_err(|error| { + error!("Failed to create a server name from the domain. {error}"); + IggyError::InvalidTlsDomain + })?; + let stream = connector.connect(domain, stream).await.map_err(|error| { + error!("Failed to establish a TLS connection to the server: {error}"); + classify_handshake_failure(&error) + })?; + + Ok(EstablishedConnection { + stream: ConnectionStreamKind::TcpTls(TcpTlsConnectionStream::new( + client_address, + TlsStream::Client(stream), + )), + client_address, + remote_address, + }) + } + + /// Give up on connecting. The state has to go back to `Disconnected`: + /// left at `Connecting`, the next `connect()` returns ok at the top + /// without ever dialing. + async fn fail_connect(&self) { + self.set_state(ClientState::Disconnected).await; + self.publish_event(DiagnosticEvent::Disconnected).await; + } + + /// [`Self::establish`], bounded while other endpoints are queued behind + /// this one (see `FAILOVER_DIAL_TIMEOUT`). The bound covers the handshake + /// as well as the connect: a peer that accepts TCP and then never answers + /// the ClientHello is exactly the kind of failure the survivors are there + /// for, and neither step has a deadline of its own. + async fn establish_bounded( + &self, + server_address: &str, + candidates: &[String], + ) -> Result { + if candidates.len() < 2 { + return self.establish(server_address).await; + } + + match tokio::time::timeout(FAILOVER_DIAL_TIMEOUT, self.establish(server_address)).await { + Ok(connection) => connection, + Err(_elapsed) => { + error!( + "Connecting to server: {server_address} took longer than \ + {FAILOVER_DIAL_TIMEOUT:?}" + ); + Err(IggyError::CannotEstablishConnection) + } + } + } + + /// What is left of the `reestablish_after` window since the last + /// successful connection, if any. + async fn reestablish_wait(&self) -> Option { + let connected_at = self + .connected_at + .lock() + .await + .as_ref() + .map(IggyTimestamp::as_micros)?; + let elapsed = IggyTimestamp::now().as_micros() - connected_at; + let interval = self.config.reconnection.reestablish_after.as_micros(); + trace!( + "Elapsed time since last connection: {}", + IggyDuration::from(elapsed) + ); + (elapsed < interval).then(|| IggyDuration::from(interval - elapsed)) + } + + /// Tear down the connection without touching the remembered sign-in. + /// + /// The reconnect and redirect paths use this: their disconnect is not + /// caller intent, and forgetting the credentials here would strand the + /// failover unauthenticated. The public [`Client::disconnect`] wraps this + /// and forgets them first. + async fn disconnect_transport(&self) -> Result<(), IggyError> { if self.get_state().await == ClientState::Disconnected { return Ok(()); } @@ -871,6 +1266,610 @@ const fn is_login_register_code(code: u32) -> bool { #[cfg(test)] mod tests { use super::*; + use iggy_binary_protocol::codes::{GET_ME_CODE, LOGOUT_USER_CODE, SEND_MESSAGES_CODE}; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + const SESSION_USER_ID: u32 = 7; + + fn client_with(server_address: &str, failover_addresses: Vec) -> TcpClient { + TcpClient::create(Arc::new(TcpClientConfig { + server_address: server_address.to_string(), + failover_addresses, + ..TcpClientConfig::default() + })) + .expect("create the client") + } + + /// A listener nothing ever accepts from: the kernel completes the TCP + /// handshake out of its backlog, which is all a dial needs to succeed. + async fn live_endpoint() -> (TcpListener, String) { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind a listener"); + let address = listener.local_addr().expect("listener address").to_string(); + (listener, address) + } + + /// An address with nothing behind it: the dial is refused at once. + async fn dead_endpoint() -> String { + let (listener, address) = live_endpoint().await; + drop(listener); + address + } + + /// A peer that accepts TCP and hangs up without a byte: enough for the + /// dial, never enough for a TLS handshake. + async fn endpoint_that_hangs_up() -> String { + let (listener, address) = live_endpoint().await; + tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + drop(stream); + } + }); + address + } + + // With reconnection off there are no retries, but the failover endpoints + // were configured to be tried and each still gets its one turn. + #[tokio::test] + async fn a_client_with_reconnection_disabled_still_sweeps_its_failover_endpoints() { + let (_listener, survivor) = live_endpoint().await; + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: dead_endpoint().await, + failover_addresses: vec![survivor.clone()], + reconnection: TcpClientReconnectionConfig { + enabled: false, + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + TcpClient::connect(&client).await.expect("connect"); + assert_eq!(*client.current_server_address.lock().await, survivor); + } + + // A connect that gives up has to leave the state at `Disconnected`: left + // at `Connecting`, the next `connect()` returns ok at the top without ever + // dialing, and the client is wedged for good. + #[tokio::test] + async fn a_connect_that_exhausts_every_endpoint_leaves_the_client_disconnected() { + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: dead_endpoint().await, + failover_addresses: vec![dead_endpoint().await], + reconnection: TcpClientReconnectionConfig { + enabled: false, + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + assert!(matches!( + TcpClient::connect(&client).await, + Err(IggyError::CannotEstablishConnection) + )); + assert_eq!(client.get_state().await, ClientState::Disconnected); + assert!( + matches!( + TcpClient::connect(&client).await, + Err(IggyError::CannotEstablishConnection) + ), + "a second connect has to dial again rather than report success" + ); + } + + // An endpoint that accepts TCP but fails the TLS handshake is not where + // this client lives: recording it would make the next connect lead with + // it and shadow every endpoint behind it. + #[tokio::test] + async fn an_endpoint_that_fails_the_tls_handshake_does_not_become_the_current_one() { + let configured = dead_endpoint().await; + // Plain TCP behind a TLS client: the dial succeeds, the handshake + // cannot. + let plaintext = endpoint_that_hangs_up().await; + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: configured.clone(), + failover_addresses: vec![plaintext], + tls_enabled: true, + tls_validate_certificate: false, + reconnection: TcpClientReconnectionConfig { + enabled: false, + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + assert!(matches!( + TcpClient::connect(&client).await, + Err(IggyError::CannotEstablishConnection) + )); + assert_eq!(*client.current_server_address.lock().await, configured); + } + + // A peer that accepts TCP and then never answers is what the other + // endpoints are there for; without a bound on the handshake the sweep + // waits on it forever. + #[tokio::test] + async fn an_endpoint_that_never_answers_the_handshake_does_not_hold_up_the_sweep() { + let (_listener, silent) = live_endpoint().await; + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: silent, + failover_addresses: vec![dead_endpoint().await], + tls_enabled: true, + tls_validate_certificate: false, + reconnection: TcpClientReconnectionConfig { + enabled: false, + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + let sweep = tokio::time::timeout( + FAILOVER_DIAL_TIMEOUT * 3, + std::pin::pin!(TcpClient::connect(&client)), + ) + .await + .expect("the sweep has to end on its own"); + assert!(matches!(sweep, Err(IggyError::CannotEstablishConnection))); + } + + /// A peer that accepts TCP and then answers a ClientHello with something + /// else: the handshake fails for a reason no retry changes. + async fn endpoint_that_speaks_no_tls() -> String { + let (listener, address) = live_endpoint().await; + tokio::spawn(async move { + while let Ok((mut stream, _)) = listener.accept().await { + tokio::spawn(async move { + let _ = stream.write_all(b"this is not a TLS record\n").await; + // Held open, so the failure is the handshake's verdict + // rather than a closed socket. + let mut sink = [0u8; 64]; + while stream.read(&mut sink).await.is_ok_and(|read| read > 0) {} + }); + } + }); + address + } + + // A certificate this client will never accept says the same thing on every + // attempt, so it has to reach the caller instead of being redialed every + // interval forever -- which is what `max_retries = None` did with it. + #[tokio::test] + async fn a_handshake_no_retry_can_fix_ends_the_connect() { + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: endpoint_that_speaks_no_tls().await, + tls_enabled: true, + tls_validate_certificate: false, + reconnection: TcpClientReconnectionConfig { + // Unlimited retries, so a transient classification never + // returns and this test times out instead of failing. + max_retries: None, + interval: NonZeroIggyDuration::from_str("100ms").expect("duration"), + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + let connect = tokio::time::timeout( + std::time::Duration::from_secs(10), + std::pin::pin!(TcpClient::connect(&client)), + ) + .await + .expect("the connect has to end on its own"); + assert!(matches!(connect, Err(IggyError::InvalidTlsCertificate))); + assert_eq!(client.get_state().await, ClientState::Disconnected); + } + + // A sign-in that lost its socket leaves nothing to send on, so the + // transport goes with it: kept behind a `Connected`, the next `connect()` + // would return ok without dialing and every gated operation would fail + // until someone disconnected by hand. + #[tokio::test] + async fn a_sign_in_that_lost_its_socket_leaves_the_client_disconnected() { + let (listener, address) = live_endpoint().await; + tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + // Accept, then hang up: the dial succeeds and the sign-in dies + // on the socket. + drop(stream); + } + }); + + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: address, + auto_login: AutoLogin::Enabled(Credentials::UsernamePassword( + "iggy".to_string(), + "iggy".into(), + )), + reconnection: TcpClientReconnectionConfig { + enabled: false, + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + + assert!(TcpClient::connect(&client).await.is_err()); + assert_eq!(client.get_state().await, ClientState::Disconnected); + assert!( + client.stream.lock().await.is_none(), + "a dead connection must not be kept for the next request to find" + ); + } + + // The reconnect registers a new client identity, so the server's dedup + // fence no longer covers the original request. + #[test] + fn only_requests_that_cannot_double_apply_are_replayed() { + // Never written, or refused before execution. + assert!(replay_is_safe(SEND_MESSAGES_CODE, &IggyError::NotConnected)); + assert!(replay_is_safe( + SEND_MESSAGES_CODE, + &IggyError::CannotEstablishConnection + )); + assert!(replay_is_safe(SEND_MESSAGES_CODE, &IggyError::StaleClient)); + // Written, and its outcome unknown: a replicated write must not be + // re-sent under a session the fence cannot match it against. + assert!(!replay_is_safe( + SEND_MESSAGES_CODE, + &IggyError::Disconnected + )); + assert!(!replay_is_safe( + SEND_MESSAGES_CODE, + &IggyError::EmptyResponse + )); + // A read never enters the log, and a logout ends a session the + // reconnect already replaced -- `logout_before_relogin` depends on it. + assert!(replay_is_safe(GET_ME_CODE, &IggyError::Disconnected)); + assert!(replay_is_safe(LOGOUT_USER_CODE, &IggyError::Disconnected)); + // The register replay is the protocol: the server stays silent on a + // transient failure and waits for the resend. + assert!(replay_is_safe( + LOGIN_REGISTER_CODE, + &IggyError::Disconnected + )); + } + + // `reestablish_after` paces reconnects to the endpoint that was lost. With + // somewhere else to go, that pause must not hold up the failover. + #[tokio::test] + async fn a_pending_reestablish_pause_does_not_delay_dialing_another_endpoint() { + let (_listener, survivor) = live_endpoint().await; + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: dead_endpoint().await, + failover_addresses: vec![survivor.clone()], + reconnection: TcpClientReconnectionConfig { + reestablish_after: IggyDuration::from_str("10s").expect("duration"), + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + client + .connected_at + .lock() + .await + .replace(IggyTimestamp::now()); + + let started = std::time::Instant::now(); + TcpClient::connect(&client).await.expect("connect"); + assert_eq!(*client.current_server_address.lock().await, survivor); + assert!( + started.elapsed() < std::time::Duration::from_secs(2), + "the failover waited out a pause it owed only the lost endpoint: {:?}", + started.elapsed() + ); + } + + // The other half of the same promise: `with_reestablish_after` is a + // cooldown on redialing the endpoint that was lost, and a known roster + // does not cancel it. + #[tokio::test] + async fn the_reestablish_pause_still_applies_to_the_endpoint_that_was_lost() { + let (_listener, current) = live_endpoint().await; + let client = TcpClient::create(Arc::new(TcpClientConfig { + server_address: current.clone(), + failover_addresses: vec![dead_endpoint().await], + reconnection: TcpClientReconnectionConfig { + reestablish_after: IggyDuration::from_str("1s").expect("duration"), + ..TcpClientReconnectionConfig::default() + }, + ..TcpClientConfig::default() + })) + .expect("create the client"); + client + .connected_at + .lock() + .await + .replace(IggyTimestamp::now()); + + let started = std::time::Instant::now(); + TcpClient::connect(&client).await.expect("connect"); + assert_eq!(*client.current_server_address.lock().await, current); + assert!( + started.elapsed() >= std::time::Duration::from_millis(700), + "the cooldown on the lost endpoint was skipped: {:?}", + started.elapsed() + ); + } + + #[tokio::test] + async fn dial_candidates_lead_with_the_current_endpoint_and_name_each_other_one_once() { + let client = client_with( + "127.0.0.1:8090", + vec!["127.0.0.1:8092".to_string(), "localhost:8090".to_string()], + ); + *client.roster_endpoints.lock().await = vec![ + "127.0.0.1:8090".to_string(), + "127.0.0.1:8091".to_string(), + "127.0.0.1:8092".to_string(), + ]; + + // The current endpoint leads, the roster follows, and neither the + // roster's copy of the current endpoint nor a seed that only spells + // the same endpoint differently earns a second dial. + assert_eq!( + client.dial_candidates().await, + vec![ + "127.0.0.1:8090".to_string(), + "127.0.0.1:8091".to_string(), + "127.0.0.1:8092".to_string(), + ] + ); + } + + #[tokio::test] + async fn a_client_that_learned_no_roster_still_dials_its_configured_seeds() { + let client = client_with("127.0.0.1:8090", vec!["127.0.0.1:8091".to_string()]); + + assert_eq!( + client.dial_candidates().await, + vec!["127.0.0.1:8090".to_string(), "127.0.0.1:8091".to_string()] + ); + } + + // The C++/Rust e2e contract: `login -> disconnect -> op` must fail until + // the caller signs in again. Only involuntary drops keep the sign-in. + #[tokio::test] + async fn an_explicit_disconnect_forgets_the_remembered_sign_in() { + let client = client_with("127.0.0.1:8090", Vec::new()); + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "iggy".into()), + SESSION_USER_ID, + ) + .await; + + Client::disconnect(&client).await.expect("disconnect"); + assert!( + client.sign_in_credentials().await.is_none(), + "an explicit disconnect ends the session for good, like a logout" + ); + } + + #[tokio::test] + async fn a_transport_drop_keeps_the_remembered_sign_in() { + let client = client_with("127.0.0.1:8090", Vec::new()); + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "iggy".into()), + SESSION_USER_ID, + ) + .await; + + client + .disconnect_transport() + .await + .expect("transport teardown"); + assert!( + client.sign_in_credentials().await.is_some(), + "an involuntary drop is what the failover exists for; the sign-in survives it" + ); + } + + #[tokio::test] + async fn a_sign_in_makes_a_client_without_auto_login_reconnectable() { + let client = client_with("127.0.0.1:8090", Vec::new()); + assert!(client.sign_in_credentials().await.is_none()); + + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "iggy".into()), + SESSION_USER_ID, + ) + .await; + assert!(client.sign_in_credentials().await.is_some()); + + // An explicit logout leaves no session to restore, and a reconnect + // must not resurrect one. + client.forget_session_credentials().await; + assert!(client.sign_in_credentials().await.is_none()); + } + + // A password change for the signed-in user has to reach the remembered + // sign-in, or the next reconnect replays the old password and fails an + // unrelated request with `InvalidCredentials`. + #[tokio::test] + async fn a_password_change_for_the_signed_in_user_updates_the_remembered_sign_in() { + let client = client_with("127.0.0.1:8090", Vec::new()); + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "old".into()), + SESSION_USER_ID, + ) + .await; + + for user in [ + Identifier::numeric(SESSION_USER_ID).expect("numeric identifier"), + Identifier::named("iggy").expect("named identifier"), + ] { + client.refresh_session_password(&user, "new").await; + match client.sign_in_credentials().await { + Some(Credentials::UsernamePassword(_, password)) => { + assert_eq!(password.expose_secret(), "new", "for user: {user}"); + } + other => panic!("expected the remembered user credentials, got {other:?}"), + } + // Put it back so the second identifier form starts from the same + // place as the first. + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "old".into()), + SESSION_USER_ID, + ) + .await; + } + } + + // `change_password` can target anyone the caller may manage, and those + // changes say nothing about the credentials this client reconnects with. + #[tokio::test] + async fn a_password_change_for_another_user_leaves_the_remembered_sign_in_alone() { + let client = client_with("127.0.0.1:8090", Vec::new()); + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "old".into()), + SESSION_USER_ID, + ) + .await; + + for user in [ + Identifier::numeric(SESSION_USER_ID + 1).expect("numeric identifier"), + Identifier::named("someone-else").expect("named identifier"), + ] { + client.refresh_session_password(&user, "new").await; + match client.sign_in_credentials().await { + Some(Credentials::UsernamePassword(_, password)) => { + assert_eq!(password.expose_secret(), "old", "for user: {user}"); + } + other => panic!("expected the remembered user credentials, got {other:?}"), + } + } + } + + // A personal access token is not derived from any password. + #[tokio::test] + async fn a_password_change_leaves_a_remembered_personal_access_token_alone() { + let client = client_with("127.0.0.1:8090", Vec::new()); + client + .remember_session_credentials( + Credentials::PersonalAccessToken("token".into()), + SESSION_USER_ID, + ) + .await; + + client + .refresh_session_password( + &Identifier::numeric(SESSION_USER_ID).expect("numeric identifier"), + "new", + ) + .await; + match client.sign_in_credentials().await { + Some(Credentials::PersonalAccessToken(token)) => { + assert_eq!(token.expose_secret(), "token"); + } + other => panic!("expected the remembered token, got {other:?}"), + } + } + + // The configured credentials cannot be rewritten, so a committed password + // change for the configured user has to reach the next reconnect through + // the remembered copy, or every later drop replays the password this very + // client replaced. + #[tokio::test] + async fn a_password_change_reaches_a_configured_auto_login() { + let client = TcpClient::create(Arc::new(TcpClientConfig { + auto_login: AutoLogin::Enabled(Credentials::UsernamePassword( + "iggy".to_string(), + "old".into(), + )), + ..TcpClientConfig::default() + })) + .expect("create the client"); + client + .remember_session_credentials( + Credentials::UsernamePassword("iggy".to_string(), "old".into()), + SESSION_USER_ID, + ) + .await; + + client + .refresh_session_password( + &Identifier::numeric(SESSION_USER_ID).expect("numeric identifier"), + "new", + ) + .await; + + match client.sign_in_credentials().await { + Some(Credentials::UsernamePassword(username, password)) => { + assert_eq!(username, "iggy"); + assert_eq!(password.expose_secret(), "new"); + } + other => panic!("expected the configured user with the new password, got {other:?}"), + } + } + + // A change for somebody else says nothing about the configured user's + // password, and a sign-in as another user does not get to replace it. + #[tokio::test] + async fn a_password_change_for_another_user_leaves_a_configured_auto_login_alone() { + let client = TcpClient::create(Arc::new(TcpClientConfig { + auto_login: AutoLogin::Enabled(Credentials::UsernamePassword( + "configured".to_string(), + "old".into(), + )), + ..TcpClientConfig::default() + })) + .expect("create the client"); + client + .remember_session_credentials( + Credentials::UsernamePassword("signed-in".to_string(), "old".into()), + SESSION_USER_ID, + ) + .await; + + client + .refresh_session_password( + &Identifier::numeric(SESSION_USER_ID).expect("numeric identifier"), + "new", + ) + .await; + + match client.sign_in_credentials().await { + Some(Credentials::UsernamePassword(username, password)) => { + assert_eq!(username, "configured"); + assert_eq!(password.expose_secret(), "old"); + } + other => panic!("expected the configured credentials, got {other:?}"), + } + } + + #[tokio::test] + async fn configured_credentials_outrank_the_ones_a_sign_in_remembered() { + let client = TcpClient::create(Arc::new(TcpClientConfig { + auto_login: AutoLogin::Enabled(Credentials::UsernamePassword( + "configured".to_string(), + "iggy".into(), + )), + ..TcpClientConfig::default() + })) + .expect("create the client"); + client + .remember_session_credentials( + Credentials::UsernamePassword("signed-in".to_string(), "iggy".into()), + SESSION_USER_ID, + ) + .await; + + match client.sign_in_credentials().await { + Some(Credentials::UsernamePassword(username, _)) => assert_eq!(username, "configured"), + other => panic!("expected the configured credentials, got {other:?}"), + } + } #[test] fn should_fail_with_a_zero_heartbeat_interval() { diff --git a/core/sdk/src/vsr.rs b/core/sdk/src/vsr.rs index 4a6bda52ec..9625b020c3 100644 --- a/core/sdk/src/vsr.rs +++ b/core/sdk/src/vsr.rs @@ -159,7 +159,7 @@ pub(crate) fn encode_request_header( /// the authority: an unmapped code is forwarded as non-replicated (the code /// rides `RequestHeader.reserved`, which that path already stamps) and the /// server answers with a proper error if it does not know it. -fn operation_for_code(code: u32) -> Operation { +pub(crate) fn operation_for_code(code: u32) -> Operation { if code == LOGOUT_USER_CODE { return Operation::Logout; } diff --git a/core/sdk/src/websocket/websocket_client.rs b/core/sdk/src/websocket/websocket_client.rs index 47f7f4ac6a..56261f6d32 100644 --- a/core/sdk/src/websocket/websocket_client.rs +++ b/core/sdk/src/websocket/websocket_client.rs @@ -225,6 +225,12 @@ impl iggy_common::VsrSessionControl for WebSocketClient { impl BinaryClient for WebSocketClient {} impl WebSocketClient { + /// Whether an `AutoLogin` is configured on this client, which makes the + /// session after any connect the configured user's rather than whoever + /// signed in by hand. + pub(crate) fn auto_login_configured(&self) -> bool { + matches!(self.config.auto_login, AutoLogin::Enabled(_)) + } /// Create a new WebSocket client with the provided configuration. pub fn create(config: Arc) -> Result { let (sender, receiver) = broadcast(1000); @@ -534,12 +540,15 @@ impl WebSocketClient { /// Returns true if redirection occurred and reconnection is needed. pub(crate) async fn handle_leader_redirection(&self) -> Result { let current_address = self.current_server_address.lock().await.clone(); + // The roster's other endpoints are dropped here: only the TCP client + // dials failover candidates so far. let leader_address = check_and_redirect_to_leader( self, ¤t_address, iggy_common::TransportProtocol::WebSocket, ) - .await?; + .await? + .redirect; if let Some(new_leader_address) = leader_address { let mut redirection_state = self.leader_redirection_state.lock().await; diff --git a/foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.Vsr.cs b/foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.Vsr.cs index 2985703fa6..ea41f1e0f3 100644 --- a/foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.Vsr.cs +++ b/foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.Vsr.cs @@ -70,6 +70,13 @@ public sealed partial class TcpMessageStream : ISessionGenerationProvider /// private const int VsrMaxLeaderRedirects = 3; + /// + /// Bound on one endpoint's dial while other endpoints are queued behind it. Neither the connect nor the + /// TLS handshake has a deadline of its own, so a node whose syns are dropped would hold the sweep for + /// the whole kernel connect timeout - minutes - while a survivor goes untried. Matches the Rust SDK. + /// + private const int FailoverDialTimeout = 2_000; + /// /// Attempts a consumer-group poll gets before it gives up and reports an empty poll: one re-sync after /// the coordinator fences a stale assignment, then one retry. @@ -401,6 +408,25 @@ private async Task RedirectAsync(CancellationToken token) return true; } + /// + /// Keeps every node the roster names as a dial candidate. Replaced wholesale rather than merged: the + /// roster is the cluster's own answer about where its nodes are, so a node it dropped stops being dialed. + /// The configured address is kept separately and outlives it. A node that does not expose the tcp + /// transport reports port 0 and is skipped, since dialing it would burn an attempt on an endpoint that + /// cannot answer. + /// + private void RememberRoster(ClusterMetadata clusterMetadata) + { + var endpoints = clusterMetadata.Nodes + .Where(node => node.Endpoints.Tcp != 0) + .Select(node => ServerAddress.HostPort(node.Ip, node.Endpoints.Tcp)) + .ToArray(); + if (endpoints.Length > 0) + { + _rosterAddresses = endpoints; + } + } + private async Task GetCurrentLeaderNodeAsync(CancellationToken token) { var leaderlessDeadline = Environment.TickCount64 + VsrLeaderlessWaitMs; @@ -414,6 +440,8 @@ private async Task RedirectAsync(CancellationToken token) return null; } + RememberRoster(clusterMetadata); + if (clusterMetadata.Nodes.Count() == 1) { return null; @@ -541,6 +569,11 @@ private async Task> SendRawAsync(int code, ReadOnlyMemory Volatile.Read(ref _isConnecting) != 0; private ConnectionState State => (ConnectionState)Volatile.Read(ref _stateValue); @@ -116,6 +128,10 @@ public void Dispose() _connection?.Dispose(); _connection = null; + // Nothing can reconnect after this, and the remembered sign-in holds a plain-string password + // or token. + _rememberedLogin = null; + SetConnectionState(ConnectionState.Disconnected); _connectionEvents.Clear(); } @@ -589,7 +605,7 @@ public Task ConnectAsync(CancellationToken token = default) if (_configuration.ReconnectionSettings.Enabled && !_configuration.AutoLoginSettings.Enabled) { _logger.LogWarning( - "Reconnection is enabled without auto login: a lost session cannot be restored, requests will fail until the client logs in again"); + "Reconnection is enabled without auto login: a lost session can only be restored once the client has signed in at least once"); } return ConnectAsync(true, token); @@ -710,8 +726,16 @@ public async Task ChangePasswordAsync(Identifier userId, string currentPassword, throw new NotConnectedException(); } - return await LoginRegisterAsync(CommandCodes.LOGIN_REGISTER_CODE, + var identity = await LoginRegisterAsync(CommandCodes.LOGIN_REGISTER_CODE, LoginRegister.Serialize(userName, password), token); + _rememberedLogin = new AutoLoginSettings + { + Enabled = true, + Username = userName, + Password = password + }; + + return identity; } /// @@ -725,6 +749,11 @@ public async Task LogoutUserAsync(CancellationToken token = default) { await ResetConsensusSessionAsync(); + // An explicit sign-out leaves no session to restore, so the sign-in this client remembered + // does not outlive it. Credentials configured as AutoLoginSettings are a different promise - + // they are what every connect signs in with - and a reconnect still uses them. + _rememberedLogin = null; + if (State == ConnectionState.Authenticated) { SetConnectionState(ConnectionState.Connected); @@ -774,8 +803,11 @@ public async Task DeletePersonalAccessTokenAsync(string name, CancellationToken /// public async Task LoginWithPersonalAccessTokenAsync(string token, CancellationToken ct = default) { - return await LoginRegisterAsync(CommandCodes.LOGIN_REGISTER_WITH_PAT_CODE, + var identity = await LoginRegisterAsync(CommandCodes.LOGIN_REGISTER_WITH_PAT_CODE, LoginRegister.SerializeWithPersonalAccessToken(token), ct); + _rememberedLogin = new AutoLoginSettings { Enabled = true, PersonalAccessToken = token }; + + return identity; } /// @@ -806,7 +838,10 @@ or ConnectionState.Authenticating return; } - if (_lastConnectionTime != DateTimeOffset.MinValue) + // The initial 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 dial loop's own delay still paces the retries. + if (_lastConnectionTime != DateTimeOffset.MinValue && DialCandidates().Length == 1) { await Task.Delay(_configuration.ReconnectionSettings.InitialDelay, token); } @@ -869,7 +904,7 @@ private async Task RunHeartbeatAsync(TimeSpan interval, CancellationToken token) // the ping is what brings an idle client back. var unrecoverable = State is ConnectionState.Disconnected or ConnectionState.Connecting && !(_configuration.ReconnectionSettings.Enabled - && _configuration.AutoLoginSettings.Enabled); + && SignInSettings() != null); if (IsConnecting || unrecoverable) { continue; @@ -999,22 +1034,26 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken var retryCount = 0; var redirects = 0; var delay = _configuration.ReconnectionSettings.InitialDelay; + Exception? configurationFault = null; + + if (string.IsNullOrEmpty(_currentAddress)) + { + _currentAddress = _configuration.BaseAddress; + } + + var candidates = DialCandidates(); + var candidate = 0; do { await DropConnectionAsync(); - if (string.IsNullOrEmpty(_currentAddress)) - { - _currentAddress = _configuration.BaseAddress; - } - if (!ServerAddress.TryParse(_currentAddress, out var host, out var port)) { throw new InvalidBaseAddressException(); } Socket? socket = null; - var dialed = false; + var established = false; try { socket = new Socket(ServerAddress.AddressFamilyOf(host), SocketType.Stream, ProtocolType.Tcp); @@ -1026,8 +1065,17 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken // trailing segment of a large request until the previous one is acked. socket.NoDelay = true; - await socket.ConnectAsync(host, port, token); - dialed = true; + // Neither ConnectAsync nor the TLS handshake has a deadline of its own, and nothing up the + // stack adds one: a node whose syns are dropped - or one that accepts TCP and then never + // answers the ClientHello - would hold the sweep, and the connection semaphore with it, for + // the whole kernel connect timeout while a survivor goes untried. + using var dialCancellation = candidates.Length > 1 + ? CancellationTokenSource.CreateLinkedTokenSource(token) + : null; + dialCancellation?.CancelAfter(FailoverDialTimeout); + var dialToken = dialCancellation?.Token ?? token; + + await socket.ConnectAsync(host, port, dialToken); _currentRemoteAddress = socket.RemoteEndPoint is IPEndPoint remote ? ServerAddress.HostPort(remote.Address.ToString(), (ushort)remote.Port) @@ -1038,9 +1086,14 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.TcpKeepAliveTime, 5); var connectionStream = _configuration.TlsSettings.Enabled - ? await CreateSslStreamAndAuthenticate(socket, _configuration.TlsSettings) + ? await CreateSslStreamAndAuthenticate(socket, _configuration.TlsSettings, dialToken) : new NetworkStream(socket, true); + // Established, not merely dialed: everything up to here belongs to this endpoint and the + // sweep may try the next one, while everything past it - auto login, a redirect, the leader + // lookup - fails the same way wherever the client lands. + established = true; + await _sendingSemaphore.WaitAsync(token); try { @@ -1061,9 +1114,9 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken // No pre-login roster read: the server auth-gates cluster metadata, so leadership settles after // a sign-in binds a session. A login dialed at a backup still succeeds because the server // forwards the register to the primary. - if (autoLogin && _configuration.AutoLoginSettings.Enabled) + if (autoLogin && SignInSettings() is { } signInSettings) { - await AutoLoginAsync(token); + await AutoLoginAsync(signInSettings, token); if (await RedirectAsync(token)) { @@ -1074,10 +1127,15 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken break; } - // Only a failed dial is worth another attempt. Everything past it - a rejected certificate, bad - // credentials, a leader that cannot be found - fails the same way every time, and with unlimited + // Only bringing an endpoint up is worth trying elsewhere. Everything past it - bad credentials, a + // leader that cannot be found - fails the same way wherever the client lands, and with unlimited // retries a caller would otherwise never get the error back. - catch (Exception e) when (dialed || e is OperationCanceledException || _disposed) + // + // A handshake the dial bound cut short is a failed attempt on this endpoint like any other, so it + // must not land here: only a cancellation the caller actually asked for is fatal. + catch (Exception e) when (established + || (e is OperationCanceledException && token.IsCancellationRequested) + || _disposed) { socket?.Dispose(); @@ -1095,6 +1153,37 @@ private async Task TryEstablishConnectionAsync(bool autoLogin, CancellationToken _logger.LogError(e, "Failed to connect"); + if (IsTlsConfigurationFault(e)) + { + // A fault no retry can fix, kept aside rather than thrown at once: it belongs to the + // endpoint that raised it - a certificate that names another host - and the endpoints + // behind that one may be perfectly usable. + configurationFault = e; + } + + // Every other endpoint gets its turn before the retry delay: the node just lost may be gone for + // good, and pausing on it helps nothing. + if (++candidate < candidates.Length) + { + _currentAddress = candidates[candidate]; + continue; + } + + candidate = 0; + _currentAddress = candidates[0]; + + // No endpoint answered and at least one said why in a way no retry changes: an unreadable CA + // file, a certificate this client will never accept. The caller gets that reason instead of a + // retry loop that buries it - unlimited retries would otherwise redial it forever. + if (configurationFault is not null) + { + SetConnectionState(ConnectionState.Disconnected); + throw configurationFault; + } + + // The sweep is what the reconnection budget applies to, not a single dial: checked per dial, + // the last round would try only the endpoint the client started on, and a client with + // reconnection turned off would never reach its other endpoints at all. if (!_configuration.ReconnectionSettings.Enabled || (_configuration.ReconnectionSettings.MaxRetries > 0 && retryCount >= _configuration.ReconnectionSettings.MaxRetries)) @@ -1137,6 +1226,10 @@ async Task BackoffOrThrowAsync() _logger.LogInformation("Following leader redirect {Redirect} to {Address}", redirects, _currentAddress); + // The redirect moved the client, so the endpoint it moved to leads the next dial. + candidates = DialCandidates(); + candidate = 0; + await Task.Delay(delay, token); } } @@ -1163,21 +1256,66 @@ private async Task DropConnectionAsync() } } - private async Task AutoLoginAsync(CancellationToken token) + private string[] DialCandidates() + { + return DialCandidates(_currentAddress, _configuration.BaseAddress, _rosterAddresses); + } + + /// + /// The endpoints one connect dials, likeliest first: where the client currently is, the address it was + /// configured with, then the roster it learned while connected. Duplicates are dropped, so an endpoint the + /// roster merely spells differently does not earn a second attempt. + /// + internal static string[] DialCandidates(string currentAddress, string baseAddress, string[] rosterAddresses) + { + var candidates = new List(); + if (!string.IsNullOrEmpty(currentAddress)) + { + candidates.Add(currentAddress); + } + + foreach (var endpoint in rosterAddresses.Prepend(baseAddress)) + { + if (!string.IsNullOrEmpty(endpoint) && + !candidates.Exists(known => ServerAddress.IsSame(known, endpoint))) + { + candidates.Add(endpoint); + } + } + + return candidates.ToArray(); + } + + private async Task AutoLoginAsync(AutoLoginSettings settings, CancellationToken token) { - var settings = _configuration.AutoLoginSettings; if (!string.IsNullOrEmpty(settings.PersonalAccessToken)) { - _logger.LogInformation("Auto login enabled. Trying to login with a personal access token"); + _logger.LogInformation("Signing in with a personal access token"); await LoginWithPersonalAccessTokenAsync(settings.PersonalAccessToken, token); return; } - _logger.LogInformation("Auto login enabled. Trying to login with credentials: {Username}", settings.Username); + _logger.LogInformation("Signing in with credentials: {Username}", settings.Username); await LoginUserAsync(settings.Username, settings.Password, token); } - private async Task CreateSslStreamAndAuthenticate(Socket socket, TlsSettings tlsSettings) + /// + /// The credentials a connect signs in with: the configured ones, or else the ones a sign-in on this client + /// succeeded with. Null when nothing has ever signed in, which is when a reconnect cannot restore a + /// session at all. + /// + private AutoLoginSettings? SignInSettings() + { + if (_configuration.AutoLoginSettings.Enabled) + { + return _configuration.AutoLoginSettings; + } + + return _rememberedLogin; + } + + private async Task CreateSslStreamAndAuthenticate(Socket socket, TlsSettings tlsSettings, + CancellationToken token) { ValidateCertificatePath(tlsSettings.CertificatePath); @@ -1185,12 +1323,34 @@ private async Task CreateSslStreamAndAuthenticate(Socket socket, TlsSett _customCaStore.ImportFromPemFile(tlsSettings.CertificatePath); var stream = new NetworkStream(socket, true); var sslStream = new SslStream(stream, false, RemoteCertificateValidationCallback); - - await sslStream.AuthenticateAsClientAsync(tlsSettings.Hostname); + try + { + // The token carries the dial bound when other endpoints are queued behind this one: a peer that + // accepts TCP and never answers the ClientHello has no deadline of its own here either. + await sslStream.AuthenticateAsClientAsync( + new SslClientAuthenticationOptions { TargetHost = tlsSettings.Hostname }, token); + } + catch + { + // A handshake that failed leaves the stream owning the socket, and the sweep moves on to the + // next endpoint: undisposed, both leak for as long as the client lives. + await sslStream.DisposeAsync(); + throw; + } return sslStream; } + /// + /// Whether bringing an endpoint up failed for a reason that says this client's own TLS configuration is + /// wrong: a CA file that cannot be read, or a certificate it will never accept. Neither changes on a + /// retry, so the sweep reports it instead of redialing forever. + /// + private static bool IsTlsConfigurationFault(Exception e) + { + return e is AuthenticationException or InvalidCertificatePathException; + } + private async Task SendAckAsync(int code, ReadOnlyMemory body, CancellationToken token) { using IMemoryOwner _ = await SendWithResponseAsync(code, body, token: token); @@ -1206,6 +1366,13 @@ private async Task> SendWithResponseAsync(int code, ReadOnlyM catch (Exception e) when (IsLostConnection(e) && !IsConnecting && !_disposed) { _logger.LogWarning("Connection lost"); + + // A server-side 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. + ForgetSessionAfterEviction(e); + if (!_configuration.ReconnectionSettings.Enabled) { _logger.LogWarning("Reconnection is disabled"); @@ -1213,11 +1380,12 @@ private async Task> SendWithResponseAsync(int code, ReadOnlyM throw; } - // Without auto login a reconnect cannot re-establish the session, so the request would only come - // back unauthenticated. Login and register are the exception: they re-authenticate themselves. - if (!_configuration.AutoLoginSettings.Enabled && autoLoginOnReconnect) + // With no credentials - neither configured nor remembered from a sign-in - a reconnect cannot + // re-establish the session, so the request would only come back unauthenticated. Login and register + // are the exception: they re-authenticate themselves. + if (SignInSettings() == null && autoLoginOnReconnect) { - _logger.LogWarning("Auto login is disabled, the session cannot be re-established"); + _logger.LogWarning("No credentials to sign in with, the session cannot be re-established"); SetConnectionState(ConnectionState.Disconnected); throw; } @@ -1234,6 +1402,18 @@ private static bool IsLostConnection(Exception e) || e is IggyInvalidStatusCodeException { StatusCode: VsrError.STALE_CLIENT, FromServer: true }; } + // An eviction ends this session authoritatively, like a logout: the sign-in this client remembered goes + // with it, so only a configured auto login may bring the session back. Called from the consensus layer, + // which sees the eviction whatever it interrupted - an eviction that landed on a replicated write is + // reported as VsrRequestOutcomeUnknownException and never reaches the lost-connection path at all. + private void ForgetSessionAfterEviction(Exception verdict) + { + if (verdict is IggyInvalidStatusCodeException { StatusCode: VsrError.STALE_CLIENT, FromServer: true }) + { + _rememberedLogin = null; + } + } + private async Task> HandleReconnectionAsync(int code, ReadOnlyMemory body, bool autoLogin, CancellationToken token) { diff --git a/foreign/csharp/Iggy_SDK_Tests/VsrTests/DialCandidatesTests.cs b/foreign/csharp/Iggy_SDK_Tests/VsrTests/DialCandidatesTests.cs new file mode 100644 index 0000000000..20e0e949bc --- /dev/null +++ b/foreign/csharp/Iggy_SDK_Tests/VsrTests/DialCandidatesTests.cs @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +using Apache.Iggy.IggyClient.Implementations; + +namespace Apache.Iggy.Tests.VsrTests; + +/// +/// Mirrors the Rust SDK's dial_candidates: a client that loses the node it is on has to dial the rest +/// of the cluster, and the two SDKs have to agree on which endpoints those are and in what order. +/// +public sealed class DialCandidatesTests +{ + [Fact] + public void LeadsWithTheCurrentEndpointThenNamesEachOtherOneOnce() + { + var candidates = TcpMessageStream.DialCandidates( + "127.0.0.1:8090", + "localhost:8090", + ["127.0.0.1:8090", "127.0.0.1:8091", "127.0.0.1:8092"]); + + // Neither the roster's copy of the current endpoint nor a configured address that only spells the same + // endpoint differently earns a second dial. + Assert.Equal(["127.0.0.1:8090", "127.0.0.1:8091", "127.0.0.1:8092"], candidates); + } + + /// + /// The configured address comes before the roster: it is the one endpoint the caller vouched for, and a + /// roster learned from a cluster that has since changed shape may name nodes that are gone. + /// + [Fact] + public void DialsTheConfiguredAddressBeforeTheLearnedRoster() + { + var candidates = TcpMessageStream.DialCandidates( + "127.0.0.1:8090", + "127.0.0.1:8099", + ["127.0.0.1:8091", "127.0.0.1:8092"]); + + Assert.Equal(["127.0.0.1:8090", "127.0.0.1:8099", "127.0.0.1:8091", "127.0.0.1:8092"], candidates); + } + + [Fact] + public void KeepsTheConfiguredAddressWhenNoRosterWasLearned() + { + var candidates = TcpMessageStream.DialCandidates("127.0.0.1:8091", "127.0.0.1:8090", []); + + Assert.Equal(["127.0.0.1:8091", "127.0.0.1:8090"], candidates); + } + + [Fact] + public void FallsBackToTheConfiguredAddressBeforeTheFirstConnect() + { + var candidates = TcpMessageStream.DialCandidates(string.Empty, "127.0.0.1:8090", []); + + Assert.Equal(["127.0.0.1:8090"], candidates); + } + + [Fact] + public void DialsOneEndpointWhenNothingElseIsKnown() + { + var candidates = TcpMessageStream.DialCandidates("127.0.0.1:8090", "127.0.0.1:8090", []); + + Assert.Equal(["127.0.0.1:8090"], candidates); + } +} diff --git a/foreign/csharp/Iggy_SDK_Tests/VsrTests/EndpointFailoverTests.cs b/foreign/csharp/Iggy_SDK_Tests/VsrTests/EndpointFailoverTests.cs new file mode 100644 index 0000000000..2d88801845 --- /dev/null +++ b/foreign/csharp/Iggy_SDK_Tests/VsrTests/EndpointFailoverTests.cs @@ -0,0 +1,556 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +using System.Buffers.Binary; +using System.Net; +using System.Net.Sockets; +using System.Text; +using Apache.Iggy.Configuration; +using Apache.Iggy.Contracts.Tcp; +using Apache.Iggy.Enums; +using Apache.Iggy.Exceptions; +using Apache.Iggy.IggyClient; +using Apache.Iggy.IggyClient.Implementations; +using Apache.Iggy.Vsr; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Apache.Iggy.Tests.VsrTests; + +/// +/// The node a client signed in on dies; its next request has to complete on a survivor the roster named, +/// under a session established there. Mirrors +/// core/integration/tests/cluster/failover_client_continuity.rs. +/// +public sealed class EndpointFailoverTests +{ + private const int HeaderSize = 256; + private const int SizeOffset = 48; + private const int CommandOffset = 60; + private const int RequestIdOffset = 168; + private const int RequestOperationOffset = 176; + private const int RequestReservedOffset = 196; + private const int ReplyRequestIdOffset = 200; + private const int ReplyOperationOffset = 208; + private const int ReplyStatusOffset = 216; + + private const byte CommandReply = 8; + private const byte CommandEviction = 13; + private const int EvictionReasonOffset = 255; + private const byte EvictionStaleClient = 13; + private const byte OperationRegister = 1; + private const byte OperationNonReplicated = 2; + private const int GetClusterMetadataCode = 12; + private const int PingCode = 1; + + [Fact] + public async Task ResumesOnASurvivorAfterTheSignedInNodeDies() + { + using var primary = new MockNode(); + using var survivor = new MockNode(); + + // The primary leads, so the sign-in settles there and the roster is only remembered - not acted on - + // until the node dies. + primary.Serve(request => request.Code == GetClusterMetadataCode + ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port, survivor.Port, primary.Port)) + : Answer(request)); + survivor.Serve(request => request.Code == GetClusterMetadataCode + ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port, survivor.Port, survivor.Port)) + : Answer(request)); + + var configuration = new IggyClientConfigurator + { + BaseAddress = $"127.0.0.1:{primary.Port}", + Protocol = Protocol.Tcp, + ReconnectionSettings = new ReconnectionSettings + { + Enabled = true, + MaxRetries = 4, + InitialDelay = TimeSpan.FromMilliseconds(20) + } + }; + using var client = new TcpMessageStream(configuration, NullLoggerFactory.Instance); + + await client.ConnectAsync(TestContext.Current.CancellationToken); + // No auto login: the credentials come from the caller's own sign-in, which is the shape that could not + // reconnect at all before. + await client.LoginUserAsync("iggy", "iggy", TestContext.Current.CancellationToken); + await client.PingAsync(TestContext.Current.CancellationToken); + Assert.Equal(1, primary.Pings); + + primary.Kill(); + + // The request in flight when the node died is allowed to fail; what is not allowed is never completing + // one, which is what a client that only knows the dead endpoint does. + var (resumed, lastError) = await ResumedWithin(client, TimeSpan.FromSeconds(10)); + Assert.True(resumed, + $"the client has to resume on the survivor the roster named ({lastError}, survivor saw " + + $"{survivor.Registrations} registrations and {survivor.Pings} pings)"); + Assert.True(survivor.Registrations >= 1, "the remembered credentials signed in again on the survivor"); + Assert.True(survivor.Pings >= 1, "the request landed on the survivor"); + } + + /// + /// Mirrors the integration contract (HeartbeatTests + /// EvictedClient_WithoutAutoLogin_Should_FailFast_And_NotReconnect): a server-side eviction ends the + /// session authoritatively, so the credentials a manual sign-in remembered must not resurrect it - the + /// evicted request surfaces the loss with no reconnect attempt. + /// + [Fact] + public async Task ServerEvictionForgetsTheRememberedSignIn() + { + using var node = new MockNode(); + var evict = false; + node.Serve(request => + { + if (request.Operation == OperationRegister) + { + return Reply(OperationRegister, RegisterBody(session: 128)); + } + + return evict + ? EvictionFrame(EvictionStaleClient) + : Reply(OperationNonReplicated, request.Code == GetClusterMetadataCode + ? ClusterMetadata(node.Port, node.Port, node.Port) + : []); + }); + + var configuration = new IggyClientConfigurator + { + BaseAddress = $"127.0.0.1:{node.Port}", + Protocol = Protocol.Tcp, + ReconnectionSettings = new ReconnectionSettings + { + Enabled = true, + MaxRetries = 2, + InitialDelay = TimeSpan.FromMilliseconds(20) + } + }; + using var client = new TcpMessageStream(configuration, NullLoggerFactory.Instance); + + await client.ConnectAsync(TestContext.Current.CancellationToken); + await client.LoginUserAsync("iggy", "iggy", TestContext.Current.CancellationToken); + await client.PingAsync(TestContext.Current.CancellationToken); + var connectionsBeforeEviction = node.Connections; + + evict = true; + var evicted = await Assert.ThrowsAsync(() => + client.PingAsync(TestContext.Current.CancellationToken)); + Assert.Equal(VsrError.STALE_CLIENT, evicted.StatusCode); + Assert.True(evicted.FromServer); + Assert.Equal(connectionsBeforeEviction, node.Connections); + + // The dropped connection leaves the next call transport-shaped, but the eviction forgot the remembered + // sign-in, so it must fail fast instead of reconnecting into a resurrected session. + await Assert.ThrowsAsync(() => + client.PingAsync(TestContext.Current.CancellationToken)); + Assert.Equal(connectionsBeforeEviction, node.Connections); + } + + /// + /// The same contract when the eviction lands on a replicated write: that request is reported as + /// outcome-unknown rather than as a lost connection, so it never passes through the lost-connection + /// path, and the session it evicted still has to be forgotten. + /// + [Fact] + public async Task ServerEvictionDuringAReplicatedWriteForgetsTheRememberedSignIn() + { + using var node = new MockNode(); + var evict = false; + node.Serve(request => + { + if (request.Operation == OperationRegister) + { + return Reply(OperationRegister, RegisterBody(session: 128)); + } + + return evict + ? EvictionFrame(EvictionStaleClient) + : Reply(OperationNonReplicated, request.Code == GetClusterMetadataCode + ? ClusterMetadata(node.Port, node.Port, node.Port) + : []); + }); + + var configuration = new IggyClientConfigurator + { + BaseAddress = $"127.0.0.1:{node.Port}", + Protocol = Protocol.Tcp, + ReconnectionSettings = new ReconnectionSettings + { + Enabled = true, + MaxRetries = 2, + InitialDelay = TimeSpan.FromMilliseconds(20) + } + }; + using var client = new TcpMessageStream(configuration, NullLoggerFactory.Instance); + + await client.ConnectAsync(TestContext.Current.CancellationToken); + await client.LoginUserAsync("iggy", "iggy", TestContext.Current.CancellationToken); + await client.PingAsync(TestContext.Current.CancellationToken); + var connectionsBeforeEviction = node.Connections; + + evict = true; + await Assert.ThrowsAsync(() => + client.CreateStreamAsync("evicted-mid-write", token: TestContext.Current.CancellationToken)); + Assert.Equal(connectionsBeforeEviction, node.Connections); + + await Assert.ThrowsAsync(() => + client.PingAsync(TestContext.Current.CancellationToken)); + Assert.Equal(connectionsBeforeEviction, node.Connections); + } + + /// + /// A survivor that only comes up after the first rotation still has to be found. The reconnection + /// budget counts rotations, not dials, so one retry is one full pass over every endpoint the client + /// knows rather than one dial of the endpoint it started from. + /// + [Fact] + public async Task ResumesOnASurvivorThatComesUpAfterTheFirstRotation() + { + // A port nothing listens on yet: the survivor comes up on it only after the first rotation has + // already failed on both endpoints. + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + var survivorPort = (ushort)((IPEndPoint)probe.LocalEndpoint).Port; + probe.Stop(); + + using var primary = new MockNode(); + primary.Serve(request => request.Code == GetClusterMetadataCode + ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port, survivorPort, primary.Port)) + : Answer(request)); + + var configuration = new IggyClientConfigurator + { + BaseAddress = $"127.0.0.1:{primary.Port}", + Protocol = Protocol.Tcp, + AutoLoginSettings = new AutoLoginSettings { Enabled = true, Username = "iggy", Password = "iggy" }, + ReconnectionSettings = new ReconnectionSettings + { + Enabled = true, + // One retry: the rotation after the first failure is the last one, which is where the + // budget check used to cut the sweep short. + MaxRetries = 1, + InitialDelay = TimeSpan.FromMilliseconds(600) + } + }; + using var client = new TcpMessageStream(configuration, NullLoggerFactory.Instance); + + await client.ConnectAsync(TestContext.Current.CancellationToken); + await client.LoginUserAsync("iggy", "iggy", TestContext.Current.CancellationToken); + await client.PingAsync(TestContext.Current.CancellationToken); + + primary.Kill(); + using var survivor = new MockNode(survivorPort); + var comesUp = Task.Run(async () => + { + await Task.Delay(250, TestContext.Current.CancellationToken); + survivor.Serve(request => request.Code == GetClusterMetadataCode + ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port, survivorPort, survivorPort)) + : Answer(request)); + }, TestContext.Current.CancellationToken); + + await client.PingAsync(TestContext.Current.CancellationToken); + await comesUp; + + Assert.True(survivor.Registrations >= 1, "the session was re-established on the survivor"); + } + + private static byte[] EvictionFrame(byte reason) + { + var frame = new byte[HeaderSize]; + BinaryPrimitives.WriteUInt32LittleEndian(frame.AsSpan(SizeOffset, 4), HeaderSize); + frame[CommandOffset] = CommandEviction; + frame[EvictionReasonOffset] = reason; + return frame; + } + + [Fact] + public async Task FailsFastWhenNothingEverSignedIn() + { + using var node = new MockNode(); + node.Serve(request => request.Code == GetClusterMetadataCode + ? Reply(OperationNonReplicated, ClusterMetadata(node.Port, node.Port, node.Port)) + : Answer(request)); + + var configuration = new IggyClientConfigurator + { + BaseAddress = $"127.0.0.1:{node.Port}", + Protocol = Protocol.Tcp, + ReconnectionSettings = new ReconnectionSettings + { + Enabled = true, + MaxRetries = 2, + InitialDelay = TimeSpan.FromMilliseconds(20) + } + }; + using var client = new TcpMessageStream(configuration, NullLoggerFactory.Instance); + + await client.ConnectAsync(TestContext.Current.CancellationToken); + await client.PingAsync(TestContext.Current.CancellationToken); + + // A reconnect announces itself by entering Connecting, so the absence of that transition is the + // assertion - no need to poll for a request that must never succeed. + var reconnected = false; + client.SubscribeConnectionEvents(args => + { + reconnected |= args.CurrentState == ConnectionState.Connecting; + return Task.CompletedTask; + }); + + node.Kill(); + + await Assert.ThrowsAnyAsync(() => client.PingAsync(TestContext.Current.CancellationToken)); + Assert.False(reconnected, "a client that never signed in cannot restore a session by reconnecting"); + } + + private static async Task<(bool Resumed, string LastError)> ResumedWithin(TcpMessageStream client, + TimeSpan budget) + { + var deadline = DateTimeOffset.UtcNow + budget; + var lastError = "none"; + var attempts = 0; + while (DateTimeOffset.UtcNow < deadline) + { + attempts++; + try + { + await client.PingAsync(TestContext.Current.CancellationToken); + + return (true, lastError); + } + catch (Exception error) + { + lastError = $"{attempts} attempts, last: {error.GetType().Name}: {error.Message}"; + await Task.Delay(50, TestContext.Current.CancellationToken); + } + } + + return (false, lastError); + } + + /// A reply for anything the roster read does not claim: a register, or an empty read. + private static byte[] Answer(MockRequest request) + { + return request.Operation == OperationRegister + ? Reply(OperationRegister, RegisterBody(session: 128)) + : Reply(OperationNonReplicated, []); + } + + private static byte[] Reply(byte operation, byte[] body) + { + var frame = new byte[HeaderSize + body.Length]; + BinaryPrimitives.WriteUInt32LittleEndian(frame.AsSpan(SizeOffset, 4), (uint)frame.Length); + frame[CommandOffset] = CommandReply; + frame[ReplyOperationOffset] = operation; + BinaryPrimitives.WriteUInt32LittleEndian(frame.AsSpan(ReplyStatusOffset, 4), 0); + body.CopyTo(frame.AsSpan(HeaderSize)); + + return frame; + } + + /// + /// A register reply carries a committed result section, so its four leading zero bytes announce zero + /// entries and the typed payload starts right after them. A non-replicated read carries none. + /// + private static byte[] RegisterBody(ulong session) + { + var serverVersion = Encoding.UTF8.GetBytes("0.0.0"); + var body = new byte[4 + 17 + serverVersion.Length]; + var payload = body.AsSpan(4); + BinaryPrimitives.WriteUInt32LittleEndian(payload[..4], 7); + BinaryPrimitives.WriteUInt64LittleEndian(payload[4..12], session); + BinaryPrimitives.WriteUInt32LittleEndian(payload[12..16], 11 << 10); + payload[16] = (byte)serverVersion.Length; + serverVersion.CopyTo(payload[17..]); + + return body; + } + + private static byte[] ClusterMetadata(ushort primaryPort, ushort survivorPort, ushort leaderPort) + { + var body = new List(); + WriteString(body, "test-cluster"); + body.AddRange(BitConverter.GetBytes(2u)); + WriteNode(body, "primary", primaryPort, primaryPort == leaderPort); + WriteNode(body, "survivor", survivorPort, survivorPort == leaderPort); + + return body.ToArray(); + } + + private static void WriteNode(List body, string name, ushort port, bool leader) + { + WriteString(body, name); + WriteString(body, "127.0.0.1"); + body.AddRange(BitConverter.GetBytes(port)); + body.AddRange(BitConverter.GetBytes((ushort)0)); + body.AddRange(BitConverter.GetBytes((ushort)0)); + body.AddRange(BitConverter.GetBytes((ushort)0)); + body.Add(leader ? (byte)0 : (byte)1); + body.Add(0); + } + + private static void WriteString(List body, string value) + { + var bytes = Encoding.UTF8.GetBytes(value); + body.AddRange(BitConverter.GetBytes((uint)bytes.Length)); + body.AddRange(bytes); + } + + private readonly record struct MockRequest(byte Operation, int Code, ulong RequestId); + + /// + /// A loopback VSR node. Killing it drops the live sockets and stops accepting, so a redial is refused the + /// way a dead process refuses one. + /// + private sealed class MockNode : IDisposable + { + private readonly TcpListener _listener; + private readonly List _accepted = []; + private volatile bool _killed; + private int _connections; + private int _pings; + private int _registrations; + + /// + /// A port to bind, for a node that has to come up on an address the client already knows. Zero + /// takes whatever the OS hands out. + /// + public MockNode(ushort port = 0) + { + _listener = new TcpListener(IPAddress.Loopback, port); + _listener.Start(); + Port = (ushort)((IPEndPoint)_listener.LocalEndpoint).Port; + } + + public ushort Port { get; } + + public int Pings => Volatile.Read(ref _pings); + + public int Registrations => Volatile.Read(ref _registrations); + + public int Connections + { + get + { + lock (_accepted) + { + return _connections; + } + } + } + + public void Serve(Func handler) + { + _ = Task.Run(async () => + { + while (!_killed) + { + TcpClient connection; + try + { + connection = await _listener.AcceptTcpClientAsync(); + } + catch (Exception) + { + return; + } + + lock (_accepted) + { + _accepted.Add(connection); + _connections++; + } + + _ = Task.Run(() => Exchange(connection, handler)); + } + }); + } + + public void Kill() + { + _killed = true; + lock (_accepted) + { + foreach (var connection in _accepted) + { + connection.Close(); + } + + _accepted.Clear(); + } + + _listener.Stop(); + } + + public void Dispose() + { + Kill(); + } + + private async Task Exchange(TcpClient connection, Func handler) + { + try + { + await using var stream = connection.GetStream(); + var header = new byte[HeaderSize]; + while (!_killed) + { + await ReadExactly(stream, header); + var size = BinaryPrimitives.ReadUInt32LittleEndian(header.AsSpan(SizeOffset, 4)); + var body = new byte[size - HeaderSize]; + await ReadExactly(stream, body); + + var request = new MockRequest(header[RequestOperationOffset], + BinaryPrimitives.ReadInt32LittleEndian(header.AsSpan(RequestReservedOffset, 4)), + BinaryPrimitives.ReadUInt64LittleEndian(header.AsSpan(RequestIdOffset, 8))); + if (request.Operation == OperationRegister) + { + Interlocked.Increment(ref _registrations); + } + else if (request.Code == PingCode) + { + Interlocked.Increment(ref _pings); + } + + var reply = handler(request); + BinaryPrimitives.WriteUInt64LittleEndian(reply.AsSpan(ReplyRequestIdOffset, 8), + request.RequestId); + await stream.WriteAsync(reply); + await stream.FlushAsync(); + } + } + catch (Exception) + { + // A killed node and a client that went away look the same here. + } + } + + private static async Task ReadExactly(NetworkStream stream, byte[] buffer) + { + var read = 0; + while (read < buffer.Length) + { + var chunk = await stream.ReadAsync(buffer.AsMemory(read)); + if (chunk == 0) + { + throw new EndOfStreamException("Connection closed"); + } + + read += chunk; + } + } + } +} diff --git a/foreign/go/client/tcp/tcp_connect_test.go b/foreign/go/client/tcp/tcp_connect_test.go index 822958b5b5..aa3ce3a12e 100644 --- a/foreign/go/client/tcp/tcp_connect_test.go +++ b/foreign/go/client/tcp/tcp_connect_test.go @@ -626,7 +626,7 @@ func TestConnect_ExchangesOverTLS(t *testing.T) { func TestCreateTLSConfig_ExtractsAnIPv6ServerName(t *testing.T) { client := NewIggyTcpClient(nil, WithServerAddress("[::1]:8090"), WithTLS()) - config, err := client.createTLSConfig() + config, err := client.createTLSConfig("[::1]:8090") require.NoError(t, err) assert.Equal(t, "::1", config.ServerName) } diff --git a/foreign/go/client/tcp/tcp_core.go b/foreign/go/client/tcp/tcp_core.go index fe662af53c..0ebb91c312 100644 --- a/foreign/go/client/tcp/tcp_core.go +++ b/foreign/go/client/tcp/tcp_core.go @@ -85,6 +85,14 @@ type IggyTcpClient struct { // loggedOut records an explicit sign-out, so a reconnect's automatic // sign-in does not silently reverse it; guarded by c.mtx. loggedOut bool + // rememberedLogin holds the credentials a manual sign-in succeeded with, + // so a reconnect -- on this node or, after a failover, another one -- can + // re-establish the session instead of surfacing an unauthenticated error. + // A caller that signs in by hand is otherwise less reconnectable than one + // that configures auto-login, which is a surprising difference between + // two ways of doing the same thing. Cleared on sign-out; guarded by + // c.mtx. + rememberedLogin AutoLogin // groups caches the consumer-group assignments this client polls with. groups groupAssignmentCache // topics caches what a send needs to resolve a partition locally. @@ -294,6 +302,11 @@ const ( // A node that stopped being primary answers transient forever, so // replaying alone never recovers. failoverCheckInterval = 2 * time.Second + // failoverDialTimeout bounds one endpoint's dial and handshake while other + // endpoints are queued behind it. Neither step has a deadline of its own, + // so a node whose syns are dropped would hold the sweep for the whole + // kernel connect timeout while a survivor goes untried. + failoverDialTimeout = 2 * time.Second ) // requestBufPool reuses wire-payload buffers across RPCs. A fresh buffer @@ -468,6 +481,19 @@ func (c *IggyTcpClient) exchange(ctx context.Context, code uint32, frame []byte) if err == nil || !isReconnectable(err) { return response, err } + + // 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. This runs before the gates + // below because they all return: with reconnection disabled the eviction + // would otherwise never be forgotten, and the next manual Connect would + // sign in with the evicted session's credentials. + if errors.Is(err, ierror.ErrStaleClient) { + c.forgetLogin() + } + var precondition *localPreconditionError if errors.As(err, &precondition) { return nil, err @@ -480,12 +506,13 @@ func (c *IggyTcpClient) exchange(ctx context.Context, code uint32, frame []byte) return nil, err } - // Without auto-login a reconnect cannot restore the session, so anything - // but a sign-in fails here instead of replaying unauthenticated. The - // sign-in itself is the exception: the server stays silent on a transient + // With no credentials -- neither configured nor remembered from a + // sign-in -- a reconnect cannot restore the session, so anything but a + // sign-in fails here instead of replaying unauthenticated. The sign-in + // itself is the exception: the server stays silent on a transient // register failure and expects the client to replay it. login := isRegisterCode(code) - if !c.config.autoLogin.enabled && !login { + if _, ok := c.signInCredentials(); !ok && !login { return nil, err } c.mtx.Lock() @@ -616,7 +643,23 @@ func (c *IggyTcpClient) sendFrame(ctx context.Context, code uint32, frame []byte return nil, redirectErr } if redirect { + // A connect-scoped request is issued from inside the sign-in + // transaction, which holds registerMtx: the automatic sign-in + // on the reconnect path would wait on that lock forever. The + // transaction signs in itself on the node it lands on, so the + // reconnect must not. + connectScopedRequest := ctx.Value(connectScoped{}) != nil + if connectScopedRequest { + c.mtx.Lock() + c.skipAutoLoginOnce = true + c.mtx.Unlock() + } if connectErr := c.Connect(ctx); connectErr != nil { + if connectScopedRequest { + c.mtx.Lock() + c.skipAutoLoginOnce = false + c.mtx.Unlock() + } return nil, connectErr } stamped = false @@ -890,18 +933,29 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error { connectedAt := c.connectedAt c.mtx.Unlock() - // handle reestablish interval - if !connectedAt.IsZero() { - now := time.Now() - elapsed := now.Sub(connectedAt) - reestablishAfter := c.config.reconnection.reestablishAfter - - c.logger.Debug("Elapsed time since last connection", slog.Duration("elapsed", elapsed)) - if elapsed < reestablishAfter { - remaining := reestablishAfter - elapsed - c.logger.Info("Trying to connect to the server", slog.Duration("remaining", remaining)) - time.Sleep(remaining) - } + candidates := c.connectionCandidates() + if len(candidates) == 0 { + // Nowhere to dial: a client configured with an empty server address + // and no roster. Reporting success here would leave every request + // answering ErrNotConnected while Connect keeps saying it is + // connected. + c.mtx.Lock() + c.transportState = iggcon.TransportStateDisconnected + c.mtx.Unlock() + c.logger.Error("No server address to connect to.") + return ierror.ErrCannotEstablishConnection + } + + // reestablishAfter paces reconnects to the endpoint this client was last + // on, and to that one only: the other endpoints owe it no cooldown, and + // pausing before dialing them would push the failover past the window the + // caller is willing to wait. So when there is somewhere else to go, the + // paced endpoint goes last -- by which time its window has usually + // elapsed anyway -- instead of the wait being skipped outright. + pacedEndpoint := candidates[0] + if !connectedAt.IsZero() && len(candidates) > 1 && + time.Since(connectedAt) < c.config.reconnection.reestablishAfter { + candidates = append(candidates[1:], pacedEndpoint) } attempts := uint(1) interval := time.Duration(0) @@ -909,10 +963,7 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error { attempts = uint(c.config.reconnection.maxRetries) interval = c.config.reconnection.interval } - - candidates := c.connectionCandidates() var conn net.Conn - var candidateIndex int if err := retry.New( retry.Context(ctx), retry.Attempts(attempts), @@ -923,46 +974,42 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error { }), ).Do( func() error { - address := candidates[candidateIndex%len(candidates)] - candidateIndex++ - c.logger.Info("Iggy client is connecting to server...", slog.String("server_address", address)) - connection, err := (&net.Dialer{}).DialContext(ctx, "tcp", address) - if err != nil { - c.logger.Error("Failed to establish TCP connection to the server", slog.Any("error", err)) - return ierror.ErrCannotEstablishConnection - } - - tc := connection.(*net.TCPConn) - if err := tc.SetNoDelay(c.config.noDelay); err != nil { - c.logger.Error("Failed to set the nodelay option on the client, continuing...", slog.Any("error", err)) - } - - c.mtx.Lock() - c.clientAddress = tc.LocalAddr().String() - c.currentServerAddress = address - c.mtx.Unlock() + // Every endpoint gets its turn inside one attempt, so a full pass + // over the cluster costs one retry rather than one per endpoint: + // a pass that stopped at the first refusal would never reach the + // survivors of a client configured for a single retry. + var lastErr error + // A fault no retry can fix, kept aside rather than returned at + // once: it belongs to the endpoint that raised it, and the + // endpoints behind that one may be perfectly usable. + var configFault error + for _, address := range candidates { + if address == pacedEndpoint { + c.awaitReestablish(ctx, connectedAt) + } + connection, err := c.dialCandidate(ctx, address, len(candidates) > 1) + if err != nil { + lastErr = err + if isTLSConfigFault(err) { + configFault = err + } + continue + } - if !c.config.tlsEnabled { conn = connection return nil } - // TLS logic - tlsConfig, err := c.createTLSConfig() - if err != nil { - _ = connection.Close() - return err - } - - tlsConn := tls.Client(connection, tlsConfig) - if err := tlsConn.HandshakeContext(ctx); err != nil { - c.logger.Error("Failed to establish a TLS connection to the server", slog.Any("error", err)) - _ = connection.Close() - return fmt.Errorf("TLS handshake failed: %w", err) + // An unreadable CA file, an unparsable domain, a certificate this + // client will never accept: no endpoint answered, and at least one + // said why in a way no retry changes. Reported as unrecoverable so + // the default unlimited retries do not redial it every interval + // forever and bury it. + if configFault != nil { + return retry.Unrecoverable(configFault) } - conn = tlsConn - return nil + return lastErr }); err != nil { c.mtx.Lock() c.transportState = iggcon.TransportStateDisconnected @@ -994,6 +1041,107 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error { return nil } +// isTLSConfigFault reports whether a dial failed for a reason that says the +// client's own TLS configuration is wrong -- an unreadable or unparsable CA +// file, a domain that yields no server name, or a certificate this client will +// never accept. None of those change on a retry. +func isTLSConfigFault(err error) bool { + if errors.Is(err, ierror.ErrInvalidTlsCertificatePath) || + errors.Is(err, ierror.ErrInvalidTlsCertificate) || + errors.Is(err, ierror.ErrInvalidTlsDomain) { + return true + } + + var certificateError *tls.CertificateVerificationError + var recordError tls.RecordHeaderError + return errors.As(err, &certificateError) || errors.As(err, &recordError) +} + +// awaitReestablish waits out what is left of the reestablishAfter window since +// the last successful connection, if any. +// +// The wait ends early on the caller's context or on Close: a sweep that found +// every other endpoint refused reaches the paced one in milliseconds, and +// sleeping the rest of the window regardless would hold the client in +// Connecting long past the deadline the caller gave it. +func (c *IggyTcpClient) awaitReestablish(ctx context.Context, connectedAt time.Time) { + if connectedAt.IsZero() { + return + } + + elapsed := time.Since(connectedAt) + c.logger.Debug("Elapsed time since last connection", slog.Duration("elapsed", elapsed)) + remaining := c.config.reconnection.reestablishAfter - elapsed + if remaining <= 0 { + return + } + + c.logger.Info("Trying to connect to the server", slog.Duration("remaining", remaining)) + timer := time.NewTimer(remaining) + defer timer.Stop() + select { + case <-timer.C: + case <-ctx.Done(): + case <-c.closed: + } +} + +// dialCandidate brings one endpoint all the way up, wrapping it in TLS when +// configured, and records the endpoint that answered: the leader check +// compares against it and the next reconnect starts from it. +// +// bounded caps the whole attempt at failoverDialTimeout, for when other +// endpoints are queued behind this one. Neither the dial nor the handshake has +// a deadline of its own, and a node whose syns are dropped -- or one that +// accepts TCP and then never answers the ClientHello -- would hold the sweep +// for minutes while a survivor goes untried. +func (c *IggyTcpClient) dialCandidate(ctx context.Context, address string, bounded bool) (net.Conn, error) { + c.logger.Info("Iggy client is connecting to server...", slog.String("server_address", address)) + if bounded { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, failoverDialTimeout) + defer cancel() + } + + connection, err := (&net.Dialer{}).DialContext(ctx, "tcp", address) + if err != nil { + c.logger.Error("Failed to establish TCP connection to the server", slog.Any("error", err)) + return nil, ierror.ErrCannotEstablishConnection + } + + tc := connection.(*net.TCPConn) + if err := tc.SetNoDelay(c.config.noDelay); err != nil { + c.logger.Error("Failed to set the nodelay option on the client, continuing...", slog.Any("error", err)) + } + + established := connection + if c.config.tlsEnabled { + tlsConfig, err := c.createTLSConfig(address) + if err != nil { + _ = connection.Close() + return nil, err + } + + tlsConn := tls.Client(connection, tlsConfig) + if err := tlsConn.HandshakeContext(ctx); err != nil { + c.logger.Error("Failed to establish a TLS connection to the server", slog.Any("error", err)) + _ = connection.Close() + return nil, fmt.Errorf("TLS handshake failed: %w", err) + } + established = tlsConn + } + + // Recorded only once the connection is usable: an endpoint that accepts + // TCP but fails the handshake is not where this client lives, and leading + // the next pass with it would shadow every endpoint behind it. + c.mtx.Lock() + c.clientAddress = tc.LocalAddr().String() + c.currentServerAddress = address + c.mtx.Unlock() + + return established, nil +} + func (c *IggyTcpClient) connectionCandidates() []string { c.mtx.Lock() defer c.mtx.Unlock() @@ -1025,8 +1173,9 @@ func (c *IggyTcpClient) connectionCandidates() []string { // backup: once the caller signs in, the first replicated request fails over // through the transient-deny path. func (c *IggyTcpClient) establishSession(ctx context.Context, skipAutoLogin bool) error { - if !c.config.autoLogin.enabled { - c.logger.Info("Automatic sign-in is disabled.") + credentials, ok := c.signInCredentials() + if !ok { + c.logger.Info("No credentials to sign in with.") return nil } if skipAutoLogin { @@ -1034,7 +1183,6 @@ func (c *IggyTcpClient) establishSession(ctx context.Context, skipAutoLogin bool return nil } - credentials := c.config.autoLogin.credentials if credentials.personalAccessToken != "" { _, err := c.LoginWithPersonalAccessToken(ctx, credentials.personalAccessToken) return err @@ -1043,7 +1191,42 @@ func (c *IggyTcpClient) establishSession(ctx context.Context, skipAutoLogin bool return err } -func (c *IggyTcpClient) createTLSConfig() (*tls.Config, error) { +// signInCredentials reports the credentials a reconnect signs in with: the +// configured ones, or else the ones a manual sign-in succeeded with. +func (c *IggyTcpClient) signInCredentials() (Credentials, bool) { + if c.config.autoLogin.enabled { + return c.config.autoLogin.credentials, true + } + c.mtx.Lock() + defer c.mtx.Unlock() + return c.rememberedLogin.credentials, c.rememberedLogin.enabled +} + +// rememberLogin keeps the credentials a sign-in succeeded with. Call it from +// under registerMtx (register does): remembered outside that lock, two +// concurrent sign-ins can leave A remembered while the session is B. +func (c *IggyTcpClient) rememberLogin(credentials Credentials) { + c.mtx.Lock() + defer c.mtx.Unlock() + c.rememberedLogin = NewAutoLogin(credentials) +} + +// forgetLogin drops them: after an explicit sign-out there is no session to +// restore, and a reconnect must not resurrect one. +func (c *IggyTcpClient) forgetLogin() { + c.mtx.Lock() + defer c.mtx.Unlock() + c.rememberedLogin = AutoLogin{} +} + +// createTLSConfig builds the client config for one dial. +// +// address is the candidate being dialed, which is where the SNI comes from +// when no domain is configured. Taking it from currentServerAddress instead +// would name the endpoint the client just lost: with validation on, a failover +// to a node with another name or address then fails the handshake against a +// certificate that never covered the old one. +func (c *IggyTcpClient) createTLSConfig(address string) (*tls.Config, error) { tlsConfig := &tls.Config{ InsecureSkipVerify: !c.config.tls.tlsValidateCertificate, } @@ -1051,9 +1234,9 @@ func (c *IggyTcpClient) createTLSConfig() (*tls.Config, error) { // Set server name for SNI serverName := c.config.tls.tlsDomain if serverName == "" { - host, _, err := net.SplitHostPort(c.currentServerAddress) + host, _, err := net.SplitHostPort(address) if err != nil { - host = c.currentServerAddress + host = address } serverName = host } diff --git a/foreign/go/client/tcp/tcp_failover_test.go b/foreign/go/client/tcp/tcp_failover_test.go new file mode 100644 index 0000000000..30badd8a34 --- /dev/null +++ b/foreign/go/client/tcp/tcp_failover_test.go @@ -0,0 +1,559 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package tcp + +import ( + "context" + "crypto/tls" + "log/slog" + "net" + "sync/atomic" + "testing" + "time" + + ierror "github.com/apache/iggy/foreign/go/errors" + "github.com/apache/iggy/foreign/go/internal/command" + "github.com/apache/iggy/foreign/go/internal/vsr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The node a client signed in on dies; its next request has to complete on a +// survivor the roster named, under the identity a fresh sign-in binds there. +// Mirrors `core/integration/tests/cluster/failover_client_continuity.rs`. +func TestFailover_ResumesOnASurvivorAfterTheSignedInNodeDies(t *testing.T) { + var survivor *testListener + var primary *testListener + var primaryDead atomic.Bool + + survivor = listenVSR(t, nil, func(_, _ int, read request) []byte { + switch { + case read.code() == uint32(command.GetClusterMetadataCode): + return clusterMetadataFrame(t, 1, primary.address(), survivor.address()) + case read.operation() == vsr.OperationRegister: + return registerReplyFrame(7, 512) + default: + return replyFrame(vsr.OperationNonReplicated, nil) + } + }) + + primary = listenVSR(t, nil, func(_, _ int, read request) []byte { + // A dead node answers nothing; returning nil drops the connection the + // way a killed process does. + if primaryDead.Load() { + return nil + } + switch { + case read.code() == uint32(command.GetClusterMetadataCode): + // The primary leads, so the sign-in settles here and the roster is + // only remembered -- not acted on -- until the node dies. + return clusterMetadataFrame(t, 0, primary.address(), survivor.address()) + case read.operation() == vsr.OperationRegister: + return registerReplyFrame(7, 128) + default: + return replyFrame(vsr.OperationNonReplicated, nil) + } + }) + + // No auto-login: the credentials come from the caller's own sign-in, which + // is the shape that could not reconnect at all before. + client := newDialingClient(t, primary.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "iggy", "iggy") + require.NoError(t, err) + require.NoError(t, client.Ping(ctx), "the live primary answers") + require.Equal(t, primary.address(), client.currentServerAddress) + + primaryDead.Store(true) + require.NoError(t, primary.listener.Close(), "stop accepting, so a redial is refused") + + require.NoError(t, client.Ping(ctx), + "the client has to resume on the survivor the roster named") + + assert.Equal(t, survivor.address(), client.currentServerAddress, + "the client moved off the dead endpoint") + assert.True(t, client.session.Bound(), "the session was re-established") + + var registers int + for _, read := range survivor.recorded() { + if read.operation() == vsr.OperationRegister { + registers++ + } + } + assert.Equal(t, 1, registers, + "the remembered credentials signed in again on the survivor") +} + +// Without any credentials there is nothing to sign in with, so a request on a +// dead node fails instead of reconnecting into an unauthenticated session. +func TestFailover_FailsFastWhenNothingEverSignedIn(t *testing.T) { + var server *testListener + var dead atomic.Bool + server = listenVSR(t, nil, func(_, _ int, read request) []byte { + if dead.Load() { + return nil + } + return singleNodeHandler(t, func() string { return server.address() })(0, 0, read) + }) + + client := newDialingClient(t, server.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + require.NoError(t, client.Ping(ctx)) + + dead.Store(true) + require.NoError(t, server.listener.Close()) + + assert.Error(t, client.Ping(ctx), + "a client that never signed in cannot restore a session by reconnecting") +} + +// A stale-client eviction is the server ending the session authoritatively, +// like a logout: the remembered sign-in must not resurrect it, so the evicted +// request surfaces the loss instead of reconnecting into a fresh session. +func TestFailover_ServerEvictionForgetsTheRememberedSignIn(t *testing.T) { + var server *testListener + var evict atomic.Bool + server = listenVSR(t, nil, func(_, _ int, read request) []byte { + if read.operation() == vsr.OperationRegister { + return registerReplyFrame(7, 128) + } + if evict.Load() { + return evictionFrame(vsr.EvictionStaleClient, 0, 0) + } + if read.code() == uint32(command.GetClusterMetadataCode) { + return clusterMetadataFrame(t, 0, server.address()) + } + return replyFrame(vsr.OperationNonReplicated, nil) + }) + + client := newDialingClient(t, server.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "iggy", "iggy") + require.NoError(t, err) + connectionsBefore := server.connections() + + evict.Store(true) + require.Error(t, client.Ping(ctx), "the evicted request surfaces the loss") + + _, remembered := client.signInCredentials() + assert.False(t, remembered, "the eviction forgot the remembered sign-in") + assert.Equal(t, connectionsBefore, server.connections(), + "no reconnect dial resurrected the evicted session") +} + +// An explicit sign-out is caller intent: the reconnect must not sign back in +// with the credentials the earlier sign-in used. +func TestFailover_DoesNotResurrectASignedOutSession(t *testing.T) { + var server *testListener + var dropSocket atomic.Bool + server = listenVSR(t, nil, func(_, _ int, read request) []byte { + // A dropped connection is what makes the client reconnect at all; nil + // ends it the way a killed process does. + if dropSocket.Load() { + return nil + } + return singleNodeHandler(t, func() string { return server.address() })(0, 0, read) + }) + + client := newDialingClient(t, server.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "iggy", "iggy") + require.NoError(t, err) + require.NoError(t, client.LogoutUser(ctx)) + + credentials, ok := client.signInCredentials() + require.False(t, ok, "the sign-out forgot them") + require.Empty(t, credentials.username) + + // The socket dies under a signed-out client: the reconnect has nothing to + // restore and must not invent a session. + dropSocket.Store(true) + assert.Error(t, client.Ping(ctx), "a signed-out client cannot replay through a sign-in") + dropSocket.Store(false) + + require.NoError(t, client.Connect(ctx)) + require.NoError(t, client.Ping(ctx), "the transport recovers on its own") + + var registers int + for _, read := range server.recorded() { + if read.operation() == vsr.OperationRegister { + registers++ + } + } + assert.Equal(t, 1, registers, + "only the caller's own sign-in registered; the reconnect added none") +} + +// A re-login over a dropped transport has to complete. The logout that ends +// the old session runs while the sign-in lock is held, so a logout that enters +// the reconnect path would reconnect, sign in with the remembered credentials, +// and deadlock on that same lock. +func TestFailover_ReLoginSurvivesALogoutTheTransportSwallowed(t *testing.T) { + var server *testListener + var dropLogout atomic.Bool + server = listenVSR(t, nil, func(_, _ int, read request) []byte { + if dropLogout.Load() && read.operation() == vsr.OperationLogout { + // The frame is swallowed and the connection ends, exactly as a + // node that dies mid-logout leaves it. + return nil + } + return singleNodeHandler(t, func() string { return server.address() })(0, 0, read) + }) + + client := newDialingClient(t, server.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "iggy", "iggy") + require.NoError(t, err) + + dropLogout.Store(true) + relogin := make(chan error, 1) + go func() { + _, err := client.LoginUser(ctx, "iggy", "iggy") + relogin <- err + }() + select { + case err := <-relogin: + require.NoError(t, err, "the sign-in has to replay on the new connection") + case <-time.After(15 * time.Second): + t.Fatal("the re-login deadlocked on the sign-in lock") + } + + assert.True(t, client.session.Bound(), "the replayed sign-in bound a session") + require.NoError(t, client.Ping(ctx)) +} + +// The other way a logout fails to land: the node answers it as not-admitted, +// which is what a node that stopped being primary does. The redirect that +// follows must not sign in on its own -- this goroutine holds the sign-in lock, +// and the reconnect's automatic sign-in would wait on it forever. +func TestFailover_ReLoginSurvivesALogoutTheOldPrimaryRefused(t *testing.T) { + var leader *testListener + var follower *testListener + var demoted atomic.Bool + + // The node the client is on: leader until the logout, then a follower that + // refuses it as not-admitted and points at the survivor. + follower = listenVSR(t, nil, func(_, _ int, read request) []byte { + switch { + case read.code() == uint32(command.GetClusterMetadataCode): + if demoted.Load() { + return clusterMetadataFrame(t, 1, follower.address(), leader.address()) + } + return clusterMetadataFrame(t, 0, follower.address(), leader.address()) + case read.operation() == vsr.OperationRegister: + return registerReplyFrame(7, 128) + case read.operation() == vsr.OperationLogout: + demoted.Store(true) + return statusReplyFrame(vsr.OperationLogout, + uint32(ierror.TransientNotAcceptedCode), nil) + default: + return replyFrame(vsr.OperationNonReplicated, nil) + } + }) + leader = listenVSR(t, nil, func(_, _ int, read request) []byte { + switch { + case read.code() == uint32(command.GetClusterMetadataCode): + return clusterMetadataFrame(t, 1, follower.address(), leader.address()) + case read.operation() == vsr.OperationRegister: + return registerReplyFrame(7, 256) + default: + return replyFrame(vsr.OperationNonReplicated, nil) + } + }) + + client := newDialingClient(t, follower.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "iggy", "iggy") + require.NoError(t, err) + + relogin := make(chan error, 1) + go func() { + _, err := client.LoginUser(ctx, "iggy", "iggy") + relogin <- err + }() + select { + case err := <-relogin: + require.NoError(t, err, "the sign-in has to settle on the node that leads") + case <-time.After(15 * time.Second): + t.Fatal("the re-login deadlocked on the sign-in lock") + } + + assert.True(t, client.session.Bound(), "the replayed sign-in bound a session") + assert.Equal(t, leader.address(), client.currentServerAddress) +} + +// A logout that never landed still ended the session it belonged to, so the +// credentials that established it must not outlive it: a sign-in that then +// fails would otherwise leave them for the next dropped request to replay, +// signing the old user back in after the caller asked for another one. +func TestFailover_ARejectedReLoginDoesNotResurrectThePreviousUser(t *testing.T) { + var server *testListener + var dropLogout atomic.Bool + var dropSocket atomic.Bool + var rejectLogin atomic.Bool + var registeredUsers atomic.Int32 + server = listenVSR(t, nil, func(_, _ int, read request) []byte { + if dropSocket.Load() { + return nil + } + if dropLogout.Load() && read.operation() == vsr.OperationLogout { + return nil + } + if read.operation() == vsr.OperationRegister { + registeredUsers.Add(1) + if rejectLogin.Load() { + return statusReplyFrame(vsr.OperationRegister, + uint32(ierror.InvalidCredentialsCode), nil) + } + return registerReplyFrame(7, 128) + } + return singleNodeHandler(t, func() string { return server.address() })(0, 0, read) + }) + + client := newDialingClient(t, server.address()) + ctx := context.Background() + require.NoError(t, client.Connect(ctx)) + _, err := client.LoginUser(ctx, "alice", "alice") + require.NoError(t, err) + + // The logout is swallowed and the sign-in that follows is rejected, so the + // client ends up with no session and no credentials it may use. + dropLogout.Store(true) + rejectLogin.Store(true) + _, err = client.LoginUser(ctx, "bob", "bob") + require.Error(t, err) + + _, remembered := client.signInCredentials() + assert.False(t, remembered, "the ended session's credentials must not survive it") + + // The socket dies with nothing remembered: the reconnect has no session to + // restore, and must not invent one out of the user who was signed in + // before. + dropLogout.Store(false) + dropSocket.Store(true) + registersBefore := registeredUsers.Load() + assert.Error(t, client.Ping(ctx), "there is no session left to restore") + assert.Equal(t, registersBefore, registeredUsers.Load(), + "the reconnect signed the previous user back in") +} + +// reestablishAfter is a cooldown on redialing the endpoint that was lost. It +// is owed to that endpoint alone, so a failover to another one must not sit +// through it. +func TestFailover_DoesNotSpendTheLostEndpointsPauseOnAnotherEndpoint(t *testing.T) { + var survivor *testListener + survivor = listenVSR(t, nil, singleNodeHandler(t, func() string { return survivor.address() })) + + client := newDialingClient(t, deadAddress(t)) + client.config.reconnection.reestablishAfter = time.Minute + client.knownServerAddresses = []string{survivor.address()} + client.connectedAt = time.Now() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + started := time.Now() + require.NoError(t, client.Connect(ctx)) + + assert.Equal(t, survivor.address(), client.currentServerAddress) + assert.Less(t, time.Since(started), 2*time.Second, + "the failover waited out a pause it owed only the lost endpoint") +} + +// The other half of the same promise: WithReestablishAfter is a cooldown on +// the endpoint that was lost, and a known roster does not cancel it. +func TestFailover_KeepsTheReestablishPauseForTheEndpointThatWasLost(t *testing.T) { + var current *testListener + current = listenVSR(t, nil, singleNodeHandler(t, func() string { return current.address() })) + + client := newDialingClient(t, current.address()) + client.config.reconnection.reestablishAfter = 500 * time.Millisecond + client.knownServerAddresses = []string{deadAddress(t)} + client.connectedAt = time.Now() + + started := time.Now() + require.NoError(t, client.Connect(context.Background())) + + assert.Equal(t, current.address(), client.currentServerAddress) + assert.GreaterOrEqual(t, time.Since(started), 350*time.Millisecond, + "the cooldown on the endpoint that was lost was skipped") +} + +// The cooldown is a pace limit, not a commitment: a caller that gave the +// connect a deadline has to get an answer inside it, and Close has to end the +// wait too. +func TestFailover_TheReestablishPauseHonoursTheCallersDeadline(t *testing.T) { + current := listenVSR(t, nil, func(_, _ int, read request) []byte { + return singleNodeHandler(t, func() string { return "127.0.0.1:8090" })(0, 0, read) + }) + + client := newDialingClient(t, current.address()) + client.config.reconnection.reestablishAfter = time.Minute + client.connectedAt = time.Now() + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + started := time.Now() + _ = client.Connect(ctx) + + assert.Less(t, time.Since(started), 5*time.Second, + "the cooldown outlived the deadline the caller gave the connect") +} + +// A node whose syns are dropped must not hold the sweep: without a bound on +// the dial the survivors behind it are never reached. A black-holed address +// cannot be arranged portably, so this pins the bound itself. +func TestFailover_BoundsTheDialWhenOtherEndpointsAreQueuedBehindIt(t *testing.T) { + assert.Equal(t, 2*time.Second, failoverDialTimeout, + "the dial bound has to match the other SDKs") + + var survivor *testListener + survivor = listenVSR(t, nil, singleNodeHandler(t, func() string { return survivor.address() })) + + // A listener that accepts and never answers: the dial completes out of the + // backlog, so only the bound ends the attempt. + silent, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = silent.Close() }) + + client := newDialingClient(t, silent.Addr().String(), + WithTLS(WithTLSValidateCertificate(false))) + client.knownServerAddresses = []string{survivor.address()} + + done := make(chan error, 1) + go func() { done <- client.Connect(context.Background()) }() + select { + case <-done: + case <-time.After(3 * failoverDialTimeout): + t.Fatal("the sweep never got past an endpoint that answers nothing") + } +} + +// An endpoint that accepts TCP but fails the handshake is not where this +// client lives: recording it would make the next pass lead with it and shadow +// every endpoint behind it. +func TestFailover_DoesNotSettleOnAnEndpointThatFailedTheHandshake(t *testing.T) { + hangup, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = hangup.Close() }) + go func() { + for { + connection, err := hangup.Accept() + if err != nil { + return + } + // Plain TCP behind a TLS client: the dial succeeds, the handshake + // cannot. + _ = connection.Close() + } + }() + + configured := deadAddress(t) + client := newDialingClient(t, configured, WithTLS(WithTLSValidateCertificate(false))) + client.config.reconnection.enabled = false + client.knownServerAddresses = []string{hangup.Addr().String()} + + require.Error(t, client.Connect(context.Background())) + assert.Equal(t, configured, client.currentServerAddress, + "the endpoint that failed the handshake became the current one") +} + +// The SNI of a dial belongs to the endpoint being dialed. Taken from the +// endpoint the client just lost, a failover to a node the certificate does not +// cover fails the handshake -- which is every failover, once the addresses +// differ. +func TestFailover_UsesTheDialedEndpointAsTheServerName(t *testing.T) { + certificate, caPath := selfSignedCert(t) + survivor := listenVSR(t, + func(conn net.Conn) net.Conn { + return tls.Server(conn, &tls.Config{Certificates: []tls.Certificate{certificate}}) + }, + singleNodeHandler(t, func() string { return "127.0.0.1:8090" })) + + // The certificate covers 127.0.0.1, and the endpoint the client starts on + // is 127.0.0.2, where nothing listens: with the server name taken from + // that endpoint, the handshake on the survivor is checked against the + // address that died. + _, port, err := net.SplitHostPort(survivor.address()) + require.NoError(t, err) + client := newDialingClient(t, "127.0.0.2:"+port, + WithTLS(WithTLSCAFile(caPath), WithTLSValidateCertificate(true))) + client.knownServerAddresses = []string{"127.0.0.1:" + port} + + require.NoError(t, client.Connect(context.Background())) + assert.Equal(t, "127.0.0.1:"+port, client.currentServerAddress) +} + +// A TLS configuration the client itself cannot satisfy says the same thing on +// every attempt, so it has to reach the caller instead of being redialed every +// interval forever -- which is what the default unlimited retries did with it. +func TestFailover_AConfigFaultEndsTheConnectInsteadOfRetryingForever(t *testing.T) { + certificate, _ := selfSignedCert(t) + server := listenVSR(t, + func(conn net.Conn) net.Conn { + return tls.Server(conn, &tls.Config{Certificates: []tls.Certificate{certificate}}) + }, + singleNodeHandler(t, func() string { return "127.0.0.1:8090" })) + + // A CA the server's certificate was not signed by: no retry makes that + // certificate acceptable. + _, unrelatedCA := selfSignedCert(t) + client := NewIggyTcpClient(slog.New(slog.DiscardHandler), + WithServerAddress(server.address()), + WithTLS(WithTLSCAFile(unrelatedCA), WithTLSValidateCertificate(true))) + t.Cleanup(func() { _ = client.Close() }) + client.config.reconnection.maxRetries = 0 // unlimited + client.config.reconnection.interval = 10 * time.Millisecond + + done := make(chan error, 1) + go func() { done <- client.Connect(context.Background()) }() + select { + case err := <-done: + require.Error(t, err) + case <-time.After(5 * time.Second): + t.Fatal("a connect that can never succeed has to end instead of retrying forever") + } +} + +// A client with nothing to dial must say so: reporting success would leave +// every request answering ErrNotConnected while Connect keeps claiming a +// connection. +func TestFailover_RejectsAConnectWithNoEndpointToDial(t *testing.T) { + client := NewIggyTcpClient(slog.New(slog.DiscardHandler), WithServerAddress("")) + t.Cleanup(func() { _ = client.Close() }) + + require.ErrorIs(t, client.Connect(context.Background()), ierror.ErrCannotEstablishConnection) + assert.Error(t, client.Ping(context.Background())) +} + +// deadAddress returns an address nothing listens on, so a dial to it is +// refused at once. +func deadAddress(t *testing.T) string { + t.Helper() + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + address := listener.Addr().String() + require.NoError(t, listener.Close()) + return address +} diff --git a/foreign/go/client/tcp/tcp_session_credentials_test.go b/foreign/go/client/tcp/tcp_session_credentials_test.go new file mode 100644 index 0000000000..c219d4f6a3 --- /dev/null +++ b/foreign/go/client/tcp/tcp_session_credentials_test.go @@ -0,0 +1,66 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package tcp + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newCredentialClient(autoLogin AutoLogin) *IggyTcpClient { + config := defaultTcpClientConfig() + config.autoLogin = autoLogin + return &IggyTcpClient{config: config} +} + +func TestSignInCredentials_AreAbsentUntilSomethingSignsIn(t *testing.T) { + client := newCredentialClient(AutoLogin{}) + + _, ok := client.signInCredentials() + assert.False(t, ok) +} + +func TestSignInCredentials_ComeFromAManualSignInWithoutAutoLogin(t *testing.T) { + client := newCredentialClient(AutoLogin{}) + + client.rememberLogin(NewUsernamePasswordCredentials("iggy", "secret")) + + credentials, ok := client.signInCredentials() + require.True(t, ok) + assert.Equal(t, "iggy", credentials.username) + assert.Equal(t, "secret", credentials.password) + + // An explicit sign-out leaves no session to restore, and a reconnect must + // not resurrect one. + client.forgetLogin() + _, ok = client.signInCredentials() + assert.False(t, ok) +} + +func TestSignInCredentials_PreferTheConfiguredOnes(t *testing.T) { + client := newCredentialClient(NewAutoLogin(NewUsernamePasswordCredentials("configured", "secret"))) + + client.rememberLogin(NewPersonalAccessTokenCredentials("signed-in-token")) + + credentials, ok := client.signInCredentials() + require.True(t, ok) + assert.Equal(t, "configured", credentials.username) + assert.Empty(t, credentials.personalAccessToken) +} diff --git a/foreign/go/client/tcp/tcp_session_management.go b/foreign/go/client/tcp/tcp_session_management.go index d409f7d9af..d2e5341161 100644 --- a/foreign/go/client/tcp/tcp_session_management.go +++ b/foreign/go/client/tcp/tcp_session_management.go @@ -33,7 +33,12 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password if err != nil { return nil, err } - return c.register(ctx, uint32(command.LoginRegisterCode), body) + return c.register( + ctx, + uint32(command.LoginRegisterCode), + body, + NewUsernamePasswordCredentials(username, password), + ) } func (c *IggyTcpClient) LoginWithPersonalAccessToken(ctx context.Context, token string) (*iggcon.IdentityInfo, error) { @@ -41,12 +46,28 @@ func (c *IggyTcpClient) LoginWithPersonalAccessToken(ctx context.Context, token if err != nil { return nil, err } - return c.register(ctx, uint32(command.LoginRegisterWithPATCode), body) + return c.register( + ctx, + uint32(command.LoginRegisterWithPATCode), + body, + NewPersonalAccessTokenCredentials(token), + ) } // register runs the sign-in handshake, binds the session the server assigned, -// and settles the connection on the cluster leader. -func (c *IggyTcpClient) register(ctx context.Context, code uint32, body []byte) (*iggcon.IdentityInfo, error) { +// settles the connection on the cluster leader, and remembers the credentials +// it succeeded with so a reconnect can re-establish the session. +// +// The credentials are remembered here rather than by the callers because this +// is what holds registerMtx: remembered outside it, two concurrent sign-ins +// could leave A remembered while the session is B, and the next reconnect +// would sign in as A. +func (c *IggyTcpClient) register( + ctx context.Context, + code uint32, + body []byte, + credentials Credentials, +) (*iggcon.IdentityInfo, error) { // One sign-in at a time. BeginRegister runs inside the exchange lock but // Bind runs after it, so two interleaved sign-ins would let the second // BeginRegister reset the identity the first is about to bind: one @@ -70,6 +91,7 @@ func (c *IggyTcpClient) register(ctx context.Context, code uint32, body []byte) if err != nil { return nil, err } + c.rememberLogin(credentials) if settled != nil { return settled, nil } @@ -165,6 +187,14 @@ func (c *IggyTcpClient) settleOnLeader(ctx context.Context, code uint32, body [] // endBoundSession logs out a live session before a re-login, so the server // drops its client-table entry instead of leaving it to be fenced. +// +// The logout runs connect-scoped, and a failure it could recover from is +// swallowed. Both because this call holds registerMtx: a logout that entered +// the reconnect path would reconnect, sign in with the remembered credentials, +// and deadlock on that lock. There is nothing to salvage either way -- a +// session whose logout cannot be delivered died with its socket, and the +// server fences what it left behind -- and the sign-in that follows replays +// through its own reconnect. func (c *IggyTcpClient) endBoundSession(ctx context.Context) error { c.mtx.Lock() bound := c.session.Bound() @@ -172,7 +202,29 @@ func (c *IggyTcpClient) endBoundSession(ctx context.Context) error { if !bound { return nil } - return c.LogoutUser(ctx) + + err := c.LogoutUser(context.WithValue(ctx, connectScoped{}, struct{}{})) + if err == nil { + return nil + } + if !isReconnectable(err) { + return err + } + + c.logger.Debug("The bound session's logout was not delivered; its socket ended it.", + slog.Any("error", err)) + c.mtx.Lock() + c.sessionState = iggcon.SessionStateUnauthenticated + c.session.Reset() + c.groups.clear() + c.topics.clearCounts() + c.mtx.Unlock() + // The session this sign-in belonged to is over either way, so the + // credentials that established it go with it. Kept, a sign-in that then + // fails would leave them behind for the next dropped request to replay -- + // signing the old user back in after the caller asked for another one. + c.forgetLogin() + return nil } func (c *IggyTcpClient) LogoutUser(ctx context.Context) error { @@ -188,6 +240,7 @@ func (c *IggyTcpClient) LogoutUser(ctx context.Context) error { c.groups.clear() c.topics.clearCounts() c.mtx.Unlock() + c.forgetLogin() return nil } diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClient.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClient.java index e2673d965a..ef2baa5ed7 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClient.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClient.java @@ -37,6 +37,7 @@ import org.apache.iggy.client.async.tcp.LeaderAwareness.LeaderRedirectionState; import org.apache.iggy.client.async.tcp.vsr.VsrFrameDecoder; import org.apache.iggy.config.RetryPolicy; +import org.apache.iggy.exception.IggyErrorCode; import org.apache.iggy.exception.IggyMissingCredentialsException; import org.apache.iggy.exception.IggyNotConnectedException; import org.apache.iggy.exception.IggyServerException; @@ -49,15 +50,19 @@ import java.io.File; import java.io.IOException; import java.time.Duration; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Stream; /** * Async TCP client for Apache Iggy message streaming, built on Netty. @@ -129,10 +134,38 @@ public class AsyncIggyTcpClient { private final Optional tlsCertificate; private final TcpConnectionPoolConfig poolConfig; private final ClientRoutingState routingState = new ClientRoutingState(); + private final LoginRoutingHook loginRoutingHook = new LoginRoutingHook() { + + @Override + public CompletableFuture loginOnLeader(Supplier> loginAttempt) { + return AsyncIggyTcpClient.this.loginOnLeader(loginAttempt); + } + + @Override + public void forgetLogin() { + rememberedLogin = null; + } + }; private final AtomicReference connection = new AtomicReference<>(); private final AtomicReference> loginChain = new AtomicReference<>(CompletableFuture.completedFuture(null)); private volatile ConnectionInfo connectionInfo; + /** + * Every node the roster named on the last leader check, kept as redial + * candidates. A node dies together with its address, and the roster is + * unreachable exactly when it is needed, so it has to have been + * remembered while the connection was still healthy. + */ + private volatile List rosterTargets = List.of(); + /** + * The login a successful sign-in ran, replayed after a redial so the + * session is re-established on whichever node answers. The supplier + * already carries the credentials it signed in with, so nothing new is + * stored. Cleared on an explicit sign-out, which leaves no session to + * restore. + */ + private volatile Supplier> rememberedLogin; + private volatile boolean closed; private MessagesClient messagesClient; private ConsumerGroupsClient consumerGroupsClient; @@ -235,9 +268,9 @@ public CompletableFuture connect() { consumerOffsetsClient = new ConsumerOffsetsTcpClient(currentConnection); streamsClient = new StreamsTcpClient(currentConnection); topicsClient = new TopicsTcpClient(currentConnection); - usersClient = new UsersTcpClient(currentConnection, this::loginOnLeader); + usersClient = new UsersTcpClient(currentConnection, loginRoutingHook); systemClient = new SystemTcpClient(currentConnection); - personalAccessTokensClient = new PersonalAccessTokensTcpClient(currentConnection, this::loginOnLeader); + personalAccessTokensClient = new PersonalAccessTokensTcpClient(currentConnection, loginRoutingHook); partitionsClient = new PartitionsTcpClient(currentConnection); }); } @@ -427,6 +460,10 @@ public ConsumerOffsetsClient consumerOffsets() { */ public CompletableFuture close() { closed = true; + // Closing is caller intent, like a logout: connect() clears `closed` + // again, and a session the caller ended must not come back with the + // credentials the earlier sign-in used. + rememberedLogin = null; AsyncTcpConnection currentConnection = connection.get(); if (currentConnection != null) { return currentConnection.close(); @@ -457,10 +494,50 @@ private AsyncTcpConnection openConnection(ConnectionInfo target) { heartbeatInterval, maxVsrFrameSize, this::retryTransientOnLeader, - routingState::clearAssignments, + this::onSessionReset, this::onConnectionFailure); } + /** + * A server-side eviction reached this client. The routing state it cached + * belonged to the evicted session, and a stale-client eviction is the + * server ending that session authoritatively, like a logout: nothing may + * revive it behind the caller's back. + * + * The captured login always goes, whatever is configured. The connection + * replays it to bring up a replacement channel, so keeping it would revive + * exactly the session the server ended -- and on a client whose caller + * signed in as somebody other than the configured user, it would revive a + * different user than a redial replays, making the outcome depend on which + * failure ran. + * + * What may re-establish the session is what every connect of this client + * signs in as: credentials configured on the builder. A sign-in the caller + * ran is theirs to repeat. + */ + private void onSessionReset(int errorCode) { + routingState.clearAssignments(); + if (errorCode != IggyErrorCode.STALE_CLIENT.getCode()) { + return; + } + AsyncTcpConnection currentConnection = connection.get(); + if (currentConnection != null) { + currentConnection.forgetCapturedLogin(); + } + if (username.isEmpty() || password.isEmpty()) { + log.warn("The server evicted this session as stale; the sign-in it ran will not be replayed"); + rememberedLogin = null; + return; + } + + log.info("The server evicted this session as stale; signing in again with the configured credentials"); + replayLogin().whenComplete((ignored, error) -> { + if (error != null) { + log.warn("Signing in again after the eviction failed: {}", error.getMessage()); + } + }); + } + /** * A not-accepted request was never admitted, so it is safe to recheck the * leader, restore authentication on a new connection, and retry it within @@ -554,10 +631,12 @@ private static void releasePayload(AtomicReference payload) { * Entry point of the background redial after a pool acquire failure or an * expired reply. Requests that were in flight stay failed (their outcome * is unknown); the redial only restores the client for subsequent calls. - * Alternates the current endpoint with the seed, paced by the configured - * retry policy, and replays the builder credentials on the restored - * connection. Personal-access-token logins cannot be replayed here; those - * clients must log in again themselves. + * Each rotation sweeps every endpoint the client knows -- where it was, + * the configured seed, then the roster it learned -- and only a rotation + * that reaches none of them waits out the retry policy's delay. The + * sign-in is replayed on whichever endpoint answers, whether it was + * configured on the builder or run by the caller, personal access tokens + * included. */ private void onConnectionFailure(Throwable cause) { if (closed || !isConnectionLoss(cause)) { @@ -585,42 +664,130 @@ private CompletableFuture redialAttempt(int attempt, RetryPolicy policy) { log.error("Redial gave up after {} attempts, next request will fail fast", policy.getMaxRetries()); return CompletableFuture.completedFuture(null); } - ConnectionInfo target = ReconnectPlan.target(connectionInfo, seedConnectionInfo, attempt); - Duration delay = ReconnectPlan.delay(policy, attempt); + List candidates = redialCandidates(); + // The delay paces rotations, not dials. The first rotation runs at once + // when there is somewhere else to go: pausing before dialing a survivor + // only pushes the failover past the window the caller waits in, and the + // node just lost may be gone for good. + Duration delay = attempt == 1 && candidates.size() > 1 ? Duration.ZERO : ReconnectPlan.delay(policy, attempt); Executor delayedExecutor = CompletableFuture.delayedExecutor(delay.toMillis(), TimeUnit.MILLISECONDS); - return CompletableFuture.supplyAsync(() -> null, delayedExecutor).thenCompose(ignored -> { - if (closed) { - return CompletableFuture.completedFuture(null); - } - log.info("Redial attempt {}/{} to {}", attempt, policy.getMaxRetries(), target.serverAddress()); - return retarget(target) - .thenCompose(retargeted -> replayLogin()) - .handle((ok, error) -> { - if (error == null) { - log.info("Reconnected to {}", target.serverAddress()); - return CompletableFuture.completedFuture(null); - } + return CompletableFuture.supplyAsync(() -> null, delayedExecutor) + .thenCompose(ignored -> sweepCandidates(candidates, 0, attempt, policy)); + } + + /** + * Dials one endpoint of a rotation and, if it does not come up, the next + * one. Every endpoint gets its turn inside one attempt, so a full pass over + * the cluster costs one retry rather than one per endpoint: with the + * default policy, rotating one endpoint per attempt would first dial a + * two-node survivor two delays in. + */ + private CompletableFuture sweepCandidates( + List candidates, int index, int attempt, RetryPolicy policy) { + if (closed) { + return CompletableFuture.completedFuture(null); + } + if (index >= candidates.size()) { + return redialAttempt(attempt + 1, policy); + } + ConnectionInfo target = candidates.get(index); + log.info( + "Redial attempt {}/{} to {} ({}/{})", + attempt, + policy.getMaxRetries(), + target.serverAddress(), + index + 1, + candidates.size()); + return retarget(target) + .handle((retargeted, dialError) -> { + if (dialError != null) { + log.warn("Redial to {} failed: {}", target.serverAddress(), dialError.getMessage()); + return sweepCandidates(candidates, index + 1, attempt, policy); + } + return replaySignInOn(target, candidates, index, attempt, policy); + }) + .thenCompose(Function.identity()); + } + + /** + * Re-establishes the session on an endpoint that just came up. + * + * A sign-in the server rejected -- a rotated password, an expired token -- + * ends the redial: the connection is up, no other endpoint would answer + * differently, and retrying would tear the working connection down on the + * next rotation and leave the client connected but unauthenticated anyway. + * The rejected credentials are dropped so nothing replays them. + */ + private CompletableFuture replaySignInOn( + ConnectionInfo target, List candidates, int index, int attempt, RetryPolicy policy) { + return replayLogin() + .handle((ok, loginError) -> { + if (loginError == null) { + log.info("Reconnected to {}", target.serverAddress()); + return CompletableFuture.completedFuture(null); + } + if (!isSignInRejection(unwrap(loginError))) { log.warn( - "Redial attempt {} to {} failed: {}", - attempt, + "The sign-in on {} did not complete: {}", target.serverAddress(), - error.getMessage()); - return redialAttempt(attempt + 1, policy); - }) - .thenCompose(Function.identity()); - }); + loginError.getMessage()); + return sweepCandidates(candidates, index + 1, attempt, policy); + } + log.error( + "Reconnected to {} but the sign-in was rejected: {}. The connection stands" + + " unauthenticated until the caller signs in again.", + target.serverAddress(), + loginError.getMessage()); + rememberedLogin = null; + return CompletableFuture.completedFuture(null); + }) + .thenCompose(Function.identity()); + } + + /** + * Whether the server answered the sign-in with a verdict no other endpoint + * would change: a rotated password, an expired token. + * + * Only that ends a redial. Everything else -- the channel closing before + * the reply, a timeout, a transient refusal from a node that is not the + * primary -- says nothing about the credentials, and treating it as a + * rejection would drop them and leave the client published on a node that + * is already gone, with every later call failing "not authenticated". + */ + static boolean isSignInRejection(Throwable error) { + if (!(error instanceof IggyServerException serverError)) { + return false; + } + int code = serverError.getRawErrorCode(); + return code != AsyncTcpConnection.TRANSIENT_NOT_ACCEPTED && code != AsyncTcpConnection.TRANSIENT_NOT_COMMITTED; + } + + private static Throwable unwrap(Throwable error) { + return error instanceof CompletionException && error.getCause() != null ? error.getCause() : error; } /** - * Replays the builder credentials on the freshly published connection. + * Re-establishes the session on the freshly published connection: with the + * credentials configured on the builder, or else the sign-in a caller ran + * by hand. Configured credentials win, as in the Rust and Go SDKs -- they + * are what every connect of this client is meant to sign in as, and a + * remembered sign-in exists to make a hand-run login as reconnectable as + * a configured one, not to override it. + * * The login runs through the users client, so leader discovery retargets * again before Register when the redialed node is not the leader. */ private CompletableFuture replayLogin() { - if (username.isEmpty() || password.isEmpty() || usersClient == null) { + if (username.isPresent() && password.isPresent() && usersClient != null) { + return usersClient.login(username.get(), password.get()).thenApply(identity -> null); + } + Supplier> replay = rememberedLogin; + if (replay == null) { return CompletableFuture.completedFuture(null); } - return usersClient.login(username.get(), password.get()).thenApply(identity -> null); + // Runs through loginOnLeader, so a redial that landed on a backup + // still settles on the leader before the session is used. + return loginOnLeader(replay).thenApply(identity -> null); } /** @@ -638,6 +805,12 @@ CompletableFuture loginOnLeader(Supplier callerFuture = new CompletableFuture<>(); transaction.whenComplete((identity, error) -> { gate.complete(null); + // Not after a close: a login still in flight when `close()` cleared + // this would set it again, and `connect()` clears `closed`, so the + // next loss would replay a sign-in the caller had ended. + if (error == null && !closed) { + rememberedLogin = loginAttempt; + } if (error != null) { callerFuture.completeExceptionally(error); } else { @@ -732,7 +905,62 @@ CompletableFuture> findLeaderElsewhere(ConnectionInfo c if (currentSystemClient == null) { return CompletableFuture.completedFuture(Optional.empty()); } - return LeaderAwareness.findLeaderElsewhere(currentSystemClient::getClusterMetadata, currentTarget); + return LeaderAwareness.findLeaderElsewhere(currentSystemClient::getClusterMetadata, currentTarget) + .thenApply(lookup -> { + rememberRoster(lookup); + return lookup.redirect(); + }); + } + + /** + * Keeps what a leader check learned about where the cluster's nodes are. + * + * Replaced wholesale rather than merged: the roster is the cluster's own + * answer, so a node it dropped stops being dialed. The configured seed is + * kept separately and outlives it. + * + * An inconclusive check -- an unreadable roster, a metadata read that + * failed -- names no endpoint, and that must leave the last roster + * standing: assigning it anyway would empty the redial candidates exactly + * when the cluster is unreachable, which is when they are needed. + */ + void rememberRoster(LeaderAwareness.LeaderLookup lookup) { + if (!lookup.endpoints().isEmpty()) { + rosterTargets = lookup.endpoints(); + } + } + + /** The roster this client would redial, for tests in this package. */ + List rosterTargets() { + return rosterTargets; + } + + /** Whether a sign-in is remembered for replay, for tests in this package. */ + boolean hasRememberedLogin() { + return rememberedLogin != null; + } + + /** The live connection, for tests in this package. */ + AsyncTcpConnection currentConnection() { + return connection.get(); + } + + /** + * Endpoints a redial rotates through, likeliest first: where the client + * currently is, the address it was configured with, then the roster it + * learned while connected. Duplicates are dropped by spelling, so an + * endpoint the roster merely writes differently does not earn a second + * attempt. Spelling only: this runs on the Netty event loop, where a + * resolver lookup per candidate pair would block it. + */ + private List redialCandidates() { + List candidates = new ArrayList<>(); + candidates.add(connectionInfo); + Stream.concat(Stream.of(seedConnectionInfo), rosterTargets.stream()) + .filter(endpoint -> + candidates.stream().noneMatch(candidate -> LeaderAwareness.isSameSpelling(candidate, endpoint))) + .forEach(candidates::add); + return List.copyOf(candidates); } CompletableFuture retarget(ConnectionInfo newTarget) { diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncTcpConnection.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncTcpConnection.java index 102e236fa9..94515314cf 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncTcpConnection.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/AsyncTcpConnection.java @@ -68,16 +68,13 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.IntConsumer; /** * Async TCP connection using Netty for non-blocking I/O. * Manages the connection lifecycle and request/response correlation. */ public class AsyncTcpConnection { - private static final Logger log = LoggerFactory.getLogger(AsyncTcpConnection.class); - private static final Duration DEFAULT_CONNECTION_TIMEOUT = Duration.ofMillis(3000); - // A missing reply must not hold the single VSR-pinned channel forever. - private static final Duration DEFAULT_REQUEST_TIMEOUT = Duration.ofSeconds(30); // Transient VSR denials (not-committed / not-accepted) are replayed with // the same encoded frame so the server's dedup sees the same request id. // A not-committed outcome is unknown, so it replays for the whole budget. @@ -85,8 +82,15 @@ public class AsyncTcpConnection { // so after a short same-node retry it is handed to the owning client for // a leader recheck and safe replay; mirrors TRANSIENT_FAILOVER_CHECK_INTERVAL // in core/sdk/src/tcp/tcp_client.rs. - private static final int TRANSIENT_NOT_COMMITTED = 57; - private static final int TRANSIENT_NOT_ACCEPTED = 58; + // + // Package-private: the client classifies a failed sign-in by these codes, + // and a transient one is not a rejected credential. + static final int TRANSIENT_NOT_COMMITTED = 57; + static final int TRANSIENT_NOT_ACCEPTED = 58; + private static final Logger log = LoggerFactory.getLogger(AsyncTcpConnection.class); + private static final Duration DEFAULT_CONNECTION_TIMEOUT = Duration.ofMillis(3000); + // A missing reply must not hold the single VSR-pinned channel forever. + private static final Duration DEFAULT_REQUEST_TIMEOUT = Duration.ofSeconds(30); private static final long TRANSIENT_RETRY_INTERVAL_MS = 50; private static final Duration TRANSIENT_RETRY_BUDGET = Duration.ofSeconds(30); private static final Duration NOT_ACCEPTED_RETRY_BUDGET = Duration.ofSeconds(2); @@ -97,7 +101,7 @@ public class AsyncTcpConnection { private final AtomicLong authGeneration = new AtomicLong(0); private final VsrRequestEncoder vsrEncoder; private final TransientFailoverHandler transientFailoverHandler; - private final Runnable sessionResetListener; + private final IntConsumer sessionResetListener; private final Consumer connectionFailureListener; private final long requestTimeoutNanos; private final long heartbeatIntervalNanos; @@ -127,7 +131,7 @@ public AsyncTcpConnection( Duration.ofSeconds(5), VsrFrameDecoder.DEFAULT_MAX_FRAME_SIZE, null, - () -> {}, + errorCode -> {}, ignored -> {}); } @@ -143,7 +147,7 @@ public AsyncTcpConnection( Duration heartbeatInterval, int maxVsrFrameSize, TransientFailoverHandler transientFailoverHandler, - Runnable sessionResetListener, + IntConsumer sessionResetListener, Consumer connectionFailureListener) { this.transientFailoverHandler = transientFailoverHandler; this.sessionResetListener = sessionResetListener; @@ -827,10 +831,29 @@ private void handlePostResponse(Channel channel, int commandCode, boolean isLogi * channel. Bumping the generation makes the replacement channel re-run * login and Register. The fresh session invalidates cached routing state * such as consumer-group assignments. + * + * The reason travels to the listener, which owns the question of whether + * the session may be re-established at all: only it knows whether the + * sign-in was configured on the client or run by a caller. */ - private void onSessionEvicted() { + private void onSessionEvicted(int errorCode) { authGeneration.incrementAndGet(); - sessionResetListener.run(); + sessionResetListener.accept(errorCode); + } + + /** + * Drops the captured sign-in, so the next channel comes up + * unauthenticated instead of replaying it. + * + * The channel replays the payload it captured to re-authenticate a + * replacement channel, which is right for a lost connection and wrong + * after an eviction the server decided on: that would resurrect the very + * session the server ended. + */ + void forgetCapturedLogin() { + authenticated = false; + authGeneration.incrementAndGet(); + releaseLoginPayload(); } private void captureLoginPayloadIfNeeded(int commandCode, ByteBuf payload) { @@ -894,7 +917,7 @@ private static final class PoolChannelHandler extends AbstractChannelPoolHandler private final SslContext sslContext; private final ConsensusSession consensusSession; private final int maxVsrFrameSize; - private final Runnable onEviction; + private final IntConsumer onEviction; PoolChannelHandler( String host, @@ -903,7 +926,7 @@ private static final class PoolChannelHandler extends AbstractChannelPoolHandler SslContext sslContext, ConsensusSession consensusSession, int maxVsrFrameSize, - Runnable onEviction) { + IntConsumer onEviction) { this.host = host; this.port = port; this.enableTls = enableTls; diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LeaderAwareness.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LeaderAwareness.java index f541e9de7c..c316733a26 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LeaderAwareness.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LeaderAwareness.java @@ -72,12 +72,12 @@ private LeaderAwareness() {} * exceptionally, so the redirection path cannot fail the login that * triggered it. */ - static CompletableFuture> findLeaderElsewhere( + static CompletableFuture findLeaderElsewhere( Supplier> fetchMetadata, ConnectionInfo currentTarget) { return findLeaderElsewhere(fetchMetadata, currentTarget, LEADERLESS_WAIT_BUDGET, LEADERLESS_POLL_INTERVAL); } - static CompletableFuture> findLeaderElsewhere( + static CompletableFuture findLeaderElsewhere( Supplier> fetchMetadata, ConnectionInfo currentTarget, Duration leaderlessWaitBudget, @@ -87,7 +87,7 @@ static CompletableFuture> findLeaderElsewhere( fetchMetadata, currentTarget, leaderlessWaitBudget, leaderlessPollInterval, electionDeadlineNanos); } - private static CompletableFuture> pollForLeader( + private static CompletableFuture pollForLeader( Supplier> fetchMetadata, ConnectionInfo currentTarget, Duration leaderlessWaitBudget, @@ -99,16 +99,18 @@ private static CompletableFuture> pollForLeader( } catch (RuntimeException fetchError) { fetched = CompletableFuture.failedFuture(fetchError); } - return fetched.>>handleAsync((metadata, error) -> { + return fetched.>handleAsync((metadata, error) -> { if (error != null) { log.warn( "Failed to get cluster metadata: {}, connection will continue on server node {}", error.getMessage(), currentTarget.serverAddress()); - return CompletableFuture.completedFuture(Optional.empty()); + return CompletableFuture.completedFuture(LeaderLookup.inconclusive()); } LeaderCheck check; + List endpoints; try { + endpoints = nodeTargets(metadata); check = checkLeader(metadata, currentTarget); } catch (RuntimeException selectionError) { log.warn( @@ -116,10 +118,11 @@ private static CompletableFuture> pollForLeader( + " on server node {}", selectionError.getMessage(), currentTarget.serverAddress()); - return CompletableFuture.completedFuture(Optional.empty()); + return CompletableFuture.completedFuture(LeaderLookup.inconclusive()); } if (check instanceof LeaderCheck.Redirect redirect) { - return CompletableFuture.completedFuture(Optional.of(redirect.target())); + return CompletableFuture.completedFuture( + new LeaderLookup(Optional.of(redirect.target()), endpoints)); } if (check instanceof LeaderCheck.NoLeader) { if (System.nanoTime() >= electionDeadlineNanos) { @@ -128,7 +131,9 @@ private static CompletableFuture> pollForLeader( + " continue on server node {}", leaderlessWaitBudget, currentTarget.serverAddress()); - return CompletableFuture.completedFuture(Optional.empty()); + // A leaderless roster still names where the nodes + // are, and that is what a redial needs. + return CompletableFuture.completedFuture(new LeaderLookup(Optional.empty(), endpoints)); } Executor retryAfterInterval = CompletableFuture.delayedExecutor( leaderlessPollInterval.toMillis(), TimeUnit.MILLISECONDS); @@ -142,11 +147,23 @@ private static CompletableFuture> pollForLeader( retryAfterInterval) .thenCompose(Function.identity()); } - return CompletableFuture.completedFuture(Optional.empty()); + return CompletableFuture.completedFuture(new LeaderLookup(Optional.empty(), endpoints)); }) .thenCompose(Function.identity()); } + /** + * Every node's target for the tcp transport, in roster order. A node that + * does not expose the transport reports port 0 and is skipped: dialing it + * would burn a redial attempt on an endpoint that cannot answer. + */ + static List nodeTargets(ClusterMetadata metadata) { + return metadata.nodes().stream() + .filter(node -> node.endpoints().tcp() != 0) + .map(node -> new ConnectionInfo(node.ip(), node.endpoints().tcp())) + .toList(); + } + /** * One leader-check verdict from a cluster-metadata snapshot. */ @@ -200,15 +217,26 @@ static LeaderCheck checkLeader(ClusterMetadata metadata, ConnectionInfo currentT * at worst costs one redirect hop back to the same node. */ static boolean isSameAddress(ConnectionInfo target1, ConnectionInfo target2) { + if (isSameSpelling(target1, target2)) { + return true; + } if (target1.port() != target2.port()) { return false; } - var host1 = canonicalHost(target1.host()); - var host2 = canonicalHost(target2.host()); - if (host1.equals(host2)) { - return true; - } - return resolveToSameHost(host1, host2); + return resolveToSameHost(canonicalHost(target1.host()), canonicalHost(target2.host())); + } + + /** + * Whether two targets are written the same way, up to canonicalization. + * + * The cheap half of {@link #isSameAddress}, for callers that must not + * block: resolution is a synchronous DNS lookup, and the redial dedup runs + * on the Netty event loop. Two spellings of one node that only resolution + * could equate cost one wasted dial per rotation, which is not worth + * stalling an event loop for. + */ + static boolean isSameSpelling(ConnectionInfo target1, ConnectionInfo target2) { + return target1.port() == target2.port() && canonicalHost(target1.host()).equals(canonicalHost(target2.host())); } private static String canonicalHost(String host) { @@ -245,6 +273,24 @@ private static boolean reachesOnlyLocalMachine(InetAddress[] addresses) { return Arrays.stream(addresses).allMatch(address -> address.isLoopbackAddress() || address.isAnyLocalAddress()); } + /** + * What one leader check learned from the roster: where to go, and every + * node the cluster named for this transport. A client keeps the latter as + * redial candidates, because the address it was configured with dies with + * its node and the roster is unreachable exactly when it is needed. + */ + record LeaderLookup(Optional redirect, List endpoints) { + + LeaderLookup { + endpoints = List.copyOf(endpoints); + } + + /** A check that learned nothing: stay put, remember no endpoint. */ + static LeaderLookup inconclusive() { + return new LeaderLookup(Optional.empty(), List.of()); + } + } + /** * One leader-check verdict from a cluster-metadata snapshot. */ diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LoginRoutingHook.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LoginRoutingHook.java index a6acb7dc09..904b492e2a 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LoginRoutingHook.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/LoginRoutingHook.java @@ -41,4 +41,10 @@ interface LoginRoutingHook { * @return the identity returned by the successful Register response */ CompletableFuture loginOnLeader(Supplier> loginAttempt); + + /** + * Drops any login kept for replay. Called on an explicit sign-out: there + * is no session left to restore, and a redial must not resurrect one. + */ + default void forgetLogin() {} } diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/ReconnectPlan.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/ReconnectPlan.java index ae596731f4..f82ee0c7d8 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/ReconnectPlan.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/ReconnectPlan.java @@ -19,32 +19,17 @@ package org.apache.iggy.client.async.tcp; -import org.apache.iggy.client.ConnectionInfo; import org.apache.iggy.config.RetryPolicy; import java.time.Duration; /** - * Pure redial planning: which address to dial on a given reconnect attempt - * and how long to wait before it. + * Pure redial planning: how long to wait before a given reconnect rotation. */ final class ReconnectPlan { private ReconnectPlan() {} - /** - * Alternates reconnect dials between the current endpoint and the - * configured seed. After a leader redirect the current endpoint may die - * with the leader, and the seed is the way back to the rest of the - * cluster. Attempts are 1-based; odd attempts dial the current endpoint. - */ - static ConnectionInfo target(ConnectionInfo current, ConnectionInfo seed, int attempt) { - if (current.equals(seed)) { - return current; - } - return attempt % 2 == 1 ? current : seed; - } - /** * The delay before the given 1-based attempt: the policy's initial delay * scaled by its multiplier per prior attempt, capped at its max delay. diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/UsersTcpClient.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/UsersTcpClient.java index b6e7ab1940..1bf1e7ceda 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/UsersTcpClient.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/UsersTcpClient.java @@ -190,6 +190,7 @@ public CompletableFuture logout() { return connection().send(CommandCode.User.LOGOUT.getValue(), payload).thenAccept(response -> { response.release(); + routingHook.forgetLogin(); log.debug("Logged out successfully"); }); } diff --git a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandler.java b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandler.java index 113c288e27..ac182227f9 100644 --- a/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandler.java +++ b/foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandler.java @@ -35,6 +35,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.IntConsumer; /** * Correlates multiplexed requests by operation and request id, decodes VSR @@ -54,10 +55,10 @@ public class VsrResponseHandler extends SimpleChannelInboundHandler { private final ConcurrentMap> pendingRequests = new ConcurrentHashMap<>(); private final ConsensusSession session; - private final Runnable onEviction; + private final IntConsumer onEviction; private final AtomicReference closeCause = new AtomicReference<>(); - public VsrResponseHandler(ConsensusSession session, Runnable onEviction) { + public VsrResponseHandler(ConsensusSession session, IntConsumer onEviction) { this.session = session; this.onEviction = onEviction; } @@ -162,7 +163,11 @@ private void handleEviction(ChannelHandlerContext ctx, ByteBuf frame) { IggyServerException error = VsrHeaders.evictionToException(frame); session.reset(); try { - onEviction.run(); + // The reason travels with the notification: an eviction the server + // decided on (a stale client) ends the session authoritatively, + // while the rest are transport-shaped, and the listener has to + // tell them apart. + onEviction.accept(error.getRawErrorCode()); } catch (RuntimeException listenerError) { log.warn("Eviction listener failed: {}", listenerError.getMessage()); } diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientEndpointFailoverTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientEndpointFailoverTest.java new file mode 100644 index 0000000000..fd8080755a --- /dev/null +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientEndpointFailoverTest.java @@ -0,0 +1,439 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iggy.client.async.tcp; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import org.apache.iggy.client.ConnectionInfo; +import org.apache.iggy.config.RetryPolicy; +import org.apache.iggy.exception.IggyConnectionException; +import org.apache.iggy.exception.IggyErrorCode; +import org.apache.iggy.exception.IggyServerException; +import org.junit.jupiter.api.Test; + +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The node a client signed in on dies; its next request has to complete on a + * survivor the roster named, under a session established there. Mirrors + * {@code core/integration/tests/cluster/failover_client_continuity.rs}. The + * mock VSR framing matches {@link AsyncIggyTcpClientTransientFailoverTest}, + * kept separate so a death mid-connection cannot disturb that suite's server. + */ +class AsyncIggyTcpClientEndpointFailoverTest { + private static final int HEADER_SIZE = 256; + private static final int SIZE_OFFSET = 48; + private static final int COMMAND_OFFSET = 60; + private static final int REQUEST_ID_OFFSET = 168; + private static final int REQUEST_OPERATION_OFFSET = 176; + private static final int REQUEST_CODE_OFFSET = 196; + private static final int REPLY_REQUEST_ID_OFFSET = 200; + private static final int REPLY_OPERATION_OFFSET = 208; + private static final int REPLY_STATUS_OFFSET = 216; + + private static final int COMMAND_REPLY = 8; + private static final int OPERATION_REGISTER = 1; + private static final int OPERATION_NON_REPLICATED = 2; + private static final int GET_CLUSTER_METADATA_CODE = 12; + private static final int PING_CODE = 1; + + @Test + void shouldResumeOnASurvivorAfterTheSignedInNodeDies() throws Exception { + InetAddress loopback = InetAddress.getLoopbackAddress(); + try (ServerSocket primarySocket = new ServerSocket(0, 4, loopback); + ServerSocket survivorSocket = new ServerSocket(0, 4, loopback)) { + int primaryPort = primarySocket.getLocalPort(); + int survivorPort = survivorSocket.getLocalPort(); + AtomicInteger survivorRegistrations = new AtomicInteger(); + AtomicInteger survivorPings = new AtomicInteger(); + + // The primary leads, so the sign-in settles there and the roster is + // only remembered -- not acted on -- until the node dies. + MockNode primary = MockNode.serve(primarySocket, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success( + OPERATION_NON_REPLICATED, clusterMetadata(primaryPort, survivorPort, primaryPort)); + } + if (request.operation() == OPERATION_REGISTER) { + return Response.success(OPERATION_REGISTER, registerBody(1)); + } + return Response.success(OPERATION_NON_REPLICATED, Unpooled.EMPTY_BUFFER); + }); + MockNode survivor = MockNode.serve(survivorSocket, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success( + OPERATION_NON_REPLICATED, clusterMetadata(primaryPort, survivorPort, survivorPort)); + } + if (request.operation() == OPERATION_REGISTER) { + survivorRegistrations.incrementAndGet(); + return Response.success(OPERATION_REGISTER, registerBody(2)); + } + if (request.is(PING_CODE, OPERATION_NON_REPLICATED)) { + survivorPings.incrementAndGet(); + } + return Response.success(OPERATION_NON_REPLICATED, Unpooled.EMPTY_BUFFER); + }); + + // No builder credentials: the replay can only come from the + // sign-in the caller ran, which is the shape that could not + // reconnect at all before. With credentials configured, the replay + // falls back to them and the test would pass without a remembered + // sign-in at all. + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(primaryPort) + .requestTimeout(Duration.ofSeconds(2)) + // A whole rotation dials every endpoint the client knows, so + // the survivor is reached before this delay is ever spent. + // One endpoint per attempt would need it first. + .retryPolicy(RetryPolicy.fixedDelay(8, Duration.ofSeconds(5))) + .build(); + try { + client.connect().get(5, TimeUnit.SECONDS); + client.users().login("iggy", "iggy").get(5, TimeUnit.SECONDS); + client.sendBinaryRequest(PING_CODE, new byte[0]).get(5, TimeUnit.SECONDS); + assertThat(client.getConnectionInfo().port()).isEqualTo(primaryPort); + + primary.kill(); + + // The request in flight when the node died is allowed to fail; + // what is not allowed is never completing one, which is what a + // client that only knows the dead endpoint does. + assertThat(resumeWithin(client, Duration.ofSeconds(4))) + .as("the client has to resume on the survivor inside the first rotation") + .isTrue(); + + assertThat(client.getConnectionInfo().port()) + .as("the client moved off the dead endpoint") + .isEqualTo(survivorPort); + assertThat(survivorRegistrations) + .as("the login was replayed on the survivor") + .hasValueGreaterThanOrEqualTo(1); + assertThat(survivorPings) + .as("the request landed on the survivor") + .hasValueGreaterThanOrEqualTo(1); + } finally { + client.close().get(5, TimeUnit.SECONDS); + survivor.close(); + } + } + } + + /** + * An explicit sign-out is caller intent, like a close: the redial after it + * must not sign back in with the credentials that sign-in used. + */ + @Test + void shouldNotResurrectASignedOutSessionOnASurvivor() throws Exception { + InetAddress loopback = InetAddress.getLoopbackAddress(); + try (ServerSocket primarySocket = new ServerSocket(0, 4, loopback); + ServerSocket survivorSocket = new ServerSocket(0, 4, loopback)) { + int primaryPort = primarySocket.getLocalPort(); + int survivorPort = survivorSocket.getLocalPort(); + AtomicInteger survivorRegistrations = new AtomicInteger(); + + MockNode primary = MockNode.serve(primarySocket, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success( + OPERATION_NON_REPLICATED, clusterMetadata(primaryPort, survivorPort, primaryPort)); + } + if (request.operation() == OPERATION_REGISTER) { + return Response.success(OPERATION_REGISTER, registerBody(1)); + } + return Response.success(request.operation(), Unpooled.EMPTY_BUFFER); + }); + MockNode survivor = MockNode.serve(survivorSocket, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success( + OPERATION_NON_REPLICATED, clusterMetadata(primaryPort, survivorPort, survivorPort)); + } + if (request.operation() == OPERATION_REGISTER) { + survivorRegistrations.incrementAndGet(); + return Response.success(OPERATION_REGISTER, registerBody(2)); + } + // Echoed, so a logout is answered as a logout: the client + // checks the reply's operation against the request's. + return Response.success(request.operation(), Unpooled.EMPTY_BUFFER); + }); + + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(primaryPort) + .requestTimeout(Duration.ofSeconds(2)) + .retryPolicy(RetryPolicy.fixedDelay(4, Duration.ofMillis(50))) + .build(); + try { + client.connect().get(5, TimeUnit.SECONDS); + client.users().login("iggy", "iggy").get(5, TimeUnit.SECONDS); + client.users().logout().get(5, TimeUnit.SECONDS); + + primary.kill(); + + // Every attempt is allowed to fail; none of them may register. + resumeWithin(client, Duration.ofSeconds(2)); + + assertThat(survivorRegistrations) + .as("a signed-out client has no session to restore") + .hasValue(0); + } finally { + client.close().get(5, TimeUnit.SECONDS); + survivor.close(); + } + } + } + + /** Retries until one request completes, or the budget runs out. */ + private static boolean resumeWithin(AsyncIggyTcpClient client, Duration budget) throws InterruptedException { + long deadline = System.nanoTime() + budget.toNanos(); + while (System.nanoTime() < deadline) { + try { + client.sendBinaryRequest(PING_CODE, new byte[0]).get(2, TimeUnit.SECONDS); + return true; + } catch (ExecutionException | TimeoutException stillDown) { + Thread.sleep(50); + } + } + return false; + } + + private static ByteBuf registerBody(long session) { + ByteBuf body = Unpooled.buffer(); + body.writeIntLE(0); + body.writeIntLE(1); + body.writeLongLE(session); + body.writeIntLE(11 << 10); + body.writeByte(0); + return body; + } + + private static ByteBuf clusterMetadata(int primaryPort, int survivorPort, int leaderPort) { + ByteBuf body = Unpooled.buffer(); + writeString(body, "test-cluster"); + body.writeIntLE(2); + writeNode(body, "primary", primaryPort, primaryPort == leaderPort); + writeNode(body, "survivor", survivorPort, survivorPort == leaderPort); + return body; + } + + private static void writeNode(ByteBuf body, String name, int port, boolean leader) { + writeString(body, name); + writeString(body, InetAddress.getLoopbackAddress().getHostAddress()); + body.writeShortLE(port); + body.writeShortLE(0); + body.writeShortLE(0); + body.writeShortLE(0); + body.writeByte(leader ? 0 : 1); + body.writeByte(0); + } + + private static void writeString(ByteBuf body, String value) { + byte[] bytes = value.getBytes(StandardCharsets.UTF_8); + body.writeIntLE(bytes.length); + body.writeBytes(bytes); + } + + /** + * A loopback VSR node that keeps serving every connection it accepts until + * it is killed, which drops the live sockets and stops accepting so a + * redial is refused the way a dead process refuses one. + * + *

Dedicated daemon threads, not {@code CompletableFuture.runAsync}: the + * accept loop and every connection handler block indefinitely, and parking + * them on the common pool starves it on a low-core CI runner (parallelism + * is cores minus one), which stalls the client's own async continuations + * and times the login out before the test does anything. + */ + private static final class MockNode { + private final ServerSocket server; + private final List accepted = new CopyOnWriteArrayList<>(); + private volatile boolean killed; + + private MockNode(ServerSocket server) { + this.server = server; + } + + static MockNode serve(ServerSocket server, RequestHandler handler) { + MockNode node = new MockNode(server); + Thread acceptor = new Thread( + () -> { + while (!node.killed) { + try { + Socket socket = server.accept(); + node.accepted.add(socket); + Thread exchange = new Thread(() -> node.exchange(socket, handler)); + exchange.setDaemon(true); + exchange.start(); + } catch (IOException accepted) { + return; + } + } + }, + "mock-vsr-acceptor-" + server.getLocalPort()); + acceptor.setDaemon(true); + acceptor.start(); + return node; + } + + private void exchange(Socket socket, RequestHandler handler) { + try (socket) { + InputStream input = socket.getInputStream(); + OutputStream output = socket.getOutputStream(); + Request request; + while (!killed && (request = readRequest(input)) != null) { + writeResponse(output, request, handler.handle(request)); + } + } catch (IOException closed) { + // A killed node and a client that went away look the same here. + } + } + + void kill() throws IOException { + killed = true; + for (Socket socket : accepted) { + socket.close(); + } + server.close(); + } + + void close() throws IOException { + kill(); + } + } + + private static Request readRequest(InputStream input) throws IOException { + byte[] header = input.readNBytes(HEADER_SIZE); + if (header.length == 0) { + return null; + } + if (header.length != HEADER_SIZE) { + throw new EOFException("Truncated VSR request header"); + } + ByteBuffer fields = ByteBuffer.wrap(header).order(ByteOrder.LITTLE_ENDIAN); + int size = fields.getInt(SIZE_OFFSET); + byte[] body = input.readNBytes(size - HEADER_SIZE); + if (body.length != size - HEADER_SIZE) { + throw new EOFException("Truncated VSR request body"); + } + return new Request( + Byte.toUnsignedInt(header[REQUEST_OPERATION_OFFSET]), + fields.getInt(REQUEST_CODE_OFFSET), + fields.getLong(REQUEST_ID_OFFSET)); + } + + private static void writeResponse(OutputStream output, Request request, Response response) throws IOException { + byte[] body = new byte[response.body().readableBytes()]; + response.body().readBytes(body); + response.body().release(); + byte[] header = new byte[HEADER_SIZE]; + ByteBuffer fields = ByteBuffer.wrap(header).order(ByteOrder.LITTLE_ENDIAN); + fields.putInt(SIZE_OFFSET, HEADER_SIZE + body.length); + header[COMMAND_OFFSET] = (byte) COMMAND_REPLY; + fields.putLong(REPLY_REQUEST_ID_OFFSET, request.requestId()); + header[REPLY_OPERATION_OFFSET] = (byte) response.operation(); + fields.putInt(REPLY_STATUS_OFFSET, 0); + output.write(header); + output.write(body); + output.flush(); + } + + private record Request(int operation, int commandCode, long requestId) { + boolean is(int expectedCode, int expectedOperation) { + return commandCode == expectedCode && operation == expectedOperation; + } + } + + private record Response(int operation, ByteBuf body) { + static Response success(int operation, ByteBuf body) { + return new Response(operation, body); + } + } + + @FunctionalInterface + private interface RequestHandler { + Response handle(Request request); + } + + /** + * A roster read that learned nothing must leave the last one standing: + * emptying the redial candidates when the cluster is unreachable takes them + * away exactly when they are needed. + */ + @Test + void shouldKeepTheLastRosterWhenALookupLearnedNothing() { + InetAddress loopback = InetAddress.getLoopbackAddress(); + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(8090) + .build(); + List roster = List.of(new ConnectionInfo("iggy-0", 8091), new ConnectionInfo("iggy-1", 8092)); + + client.rememberRoster(new LeaderAwareness.LeaderLookup(Optional.empty(), roster)); + assertThat(client.rosterTargets()).isEqualTo(roster); + + client.rememberRoster(LeaderAwareness.LeaderLookup.inconclusive()); + assertThat(client.rosterTargets()) + .as("an inconclusive check erased the endpoints the client still needs") + .isEqualTo(roster); + } + + /** + * Only the server answering "no" ends a redial. A channel that closed + * before the reply, or a node that refuses because it is not the primary, + * says nothing about the credentials -- treated as a rejection, they are + * dropped and the client is published on a node that is already gone. + */ + @Test + void shouldTreatOnlyANonTransientServerVerdictAsASignInRejection() { + assertThat(AsyncIggyTcpClient.isSignInRejection( + IggyServerException.fromTcpResponse(IggyErrorCode.INVALID_CREDENTIALS.getCode(), new byte[0]))) + .isTrue(); + assertThat(AsyncIggyTcpClient.isSignInRejection(new IggyConnectionException("channel closed"))) + .as("a channel that died mid sign-in is not a rejected credential") + .isFalse(); + assertThat(AsyncIggyTcpClient.isSignInRejection(new IOException("connection reset"))) + .isFalse(); + assertThat(AsyncIggyTcpClient.isSignInRejection( + IggyServerException.fromTcpResponse(AsyncTcpConnection.TRANSIENT_NOT_ACCEPTED, new byte[0]))) + .as("a node that is not the primary refuses transiently; another endpoint answers") + .isFalse(); + assertThat(AsyncIggyTcpClient.isSignInRejection( + IggyServerException.fromTcpResponse(AsyncTcpConnection.TRANSIENT_NOT_COMMITTED, new byte[0]))) + .isFalse(); + } +} diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientTransientFailoverTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientTransientFailoverTest.java index ba481bec98..d339cebf7b 100644 --- a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientTransientFailoverTest.java +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncIggyTcpClientTransientFailoverTest.java @@ -35,8 +35,11 @@ import java.nio.ByteOrder; import java.nio.charset.StandardCharsets; import java.time.Duration; +import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; @@ -159,6 +162,180 @@ void shouldReplayTransientImplicitLoginAfterEviction() throws Exception { } } + /** + * Closing is caller intent, like a logout. A sign-in still in flight when + * it happens must not put its credentials back: `connect()` clears the + * closed flag, so the next connection loss would replay a session the + * caller had ended. + */ + @Test + void shouldNotRememberASignInThatLandedAfterClose() throws Exception { + InetAddress loopback = InetAddress.getLoopbackAddress(); + try (ServerSocket serverSocket = new ServerSocket(0, 4, loopback)) { + AtomicInteger registrations = new AtomicInteger(); + CompletableFuture server = serve(serverSocket, 4, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success(OPERATION_NON_REPLICATED, singleNodeMetadata(serverSocket.getLocalPort())); + } + if (request.operation() == OPERATION_REGISTER) { + int attempt = registrations.incrementAndGet(); + if (attempt > 1) { + // The second sign-in is the one racing the close. + try { + Thread.sleep(300); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + } + } + return Response.success(OPERATION_REGISTER, registerBody(attempt)); + } + return Response.success(request.operation(), Unpooled.EMPTY_BUFFER); + }); + + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(serverSocket.getLocalPort()) + .requestTimeout(Duration.ofSeconds(5)) + .build(); + client.connect().get(5, TimeUnit.SECONDS); + client.users().login("iggy", "iggy").get(5, TimeUnit.SECONDS); + + CompletableFuture racing = client.users().login("iggy", "iggy"); + Thread.sleep(50); + client.close().get(5, TimeUnit.SECONDS); + racing.handle((ignored, error) -> null).get(5, TimeUnit.SECONDS); + + assertThat(client.hasRememberedLogin()) + .as("a sign-in that landed after the close put its credentials back") + .isFalse(); + server.completeExceptionally(new IllegalStateException("test over")); + } + } + + /** + * The user an eviction re-establishes must not depend on which failure + * ran. Configured credentials are what every connect of this client signs + * in as, so they are what comes back -- not whoever the caller signed in + * as by hand, which is what the connection's captured login would replay. + */ + @Test + void shouldSignInAsTheConfiguredUserAfterAnEviction() throws Exception { + InetAddress loopback = InetAddress.getLoopbackAddress(); + try (ServerSocket serverSocket = new ServerSocket(0, 4, loopback)) { + AtomicInteger registrations = new AtomicInteger(); + List registeredLogins = new CopyOnWriteArrayList<>(); + AtomicBoolean evict = new AtomicBoolean(true); + CompletableFuture server = serve(serverSocket, 6, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success(OPERATION_NON_REPLICATED, singleNodeMetadata(serverSocket.getLocalPort())); + } + if (request.operation() == OPERATION_REGISTER) { + registeredLogins.add(request.bodyAsText()); + return Response.success(OPERATION_REGISTER, registerBody(registrations.incrementAndGet())); + } + if (request.operation() == OPERATION_CREATE_STREAM && evict.compareAndSet(true, false)) { + return Response.eviction(EVICTION_STALE_CLIENT); + } + return Response.success(request.operation(), Unpooled.EMPTY_BUFFER); + }); + + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(serverSocket.getLocalPort()) + .credentials("configured", "configured") + .requestTimeout(Duration.ofSeconds(2)) + .build(); + try { + client.connect().get(5, TimeUnit.SECONDS); + client.login().get(5, TimeUnit.SECONDS); + client.users().login("handrun", "handrun").get(5, TimeUnit.SECONDS); + int registrationsBeforeEviction = registrations.get(); + + assertThatThrownBy(() -> client.sendBinaryRequest(CREATE_STREAM_CODE, new byte[0]) + .get(5, TimeUnit.SECONDS)) + .hasCauseInstanceOf(IggyServerException.class); + + // The sign-in that follows the eviction runs on its own, so + // give it a moment to land. + long deadline = System.nanoTime() + Duration.ofSeconds(5).toNanos(); + while (registrations.get() == registrationsBeforeEviction && System.nanoTime() < deadline) { + Thread.sleep(25); + } + assertThat(registrations.get()) + .as("the eviction was not followed by a sign-in") + .isGreaterThan(registrationsBeforeEviction); + assertThat(registeredLogins.get(registeredLogins.size() - 1)) + .as("the eviction revived the hand-run sign-in instead of the configured one") + .contains("configured") + .doesNotContain("handrun"); + } finally { + client.close().get(5, TimeUnit.SECONDS); + } + server.completeExceptionally(new IllegalStateException("test over")); + } + } + + /** + * A stale-client eviction is the server ending the session + * authoritatively, like a logout. A client whose credentials were + * configured signs in again on every connect and recovers (the test + * above); one whose session came from a caller's own sign-in must not have + * that session revived behind the caller's back. + */ + @Test + void shouldNotReviveAHandRunSignInAfterAStaleClientEviction() throws Exception { + InetAddress loopback = InetAddress.getLoopbackAddress(); + try (ServerSocket serverSocket = new ServerSocket(0, 4, loopback)) { + AtomicInteger registrations = new AtomicInteger(); + CompletableFuture server = serve(serverSocket, 4, request -> { + if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { + return Response.success(OPERATION_NON_REPLICATED, singleNodeMetadata(serverSocket.getLocalPort())); + } + if (request.operation() == OPERATION_REGISTER) { + return Response.success(OPERATION_REGISTER, registerBody(registrations.incrementAndGet())); + } + if (request.operation() == OPERATION_CREATE_STREAM) { + return Response.eviction(EVICTION_STALE_CLIENT); + } + return Response.success(request.operation(), Unpooled.EMPTY_BUFFER); + }); + + // No configured credentials: the only sign-in is the one run below. + AsyncIggyTcpClient client = AsyncIggyTcpClient.builder() + .host(loopback.getHostAddress()) + .port(serverSocket.getLocalPort()) + .requestTimeout(Duration.ofSeconds(2)) + .build(); + try { + client.connect().get(5, TimeUnit.SECONDS); + client.users().login("iggy", "iggy").get(5, TimeUnit.SECONDS); + int registrationsBeforeEviction = registrations.get(); + + assertThatThrownBy(() -> client.sendBinaryRequest(CREATE_STREAM_CODE, new byte[0]) + .get(5, TimeUnit.SECONDS)) + .hasCauseInstanceOf(IggyServerException.class); + + // Whatever the caller does next, nothing may sign this session + // back in on its own. + assertThatThrownBy(() -> client.sendBinaryRequest(CREATE_STREAM_CODE, new byte[0]) + .get(5, TimeUnit.SECONDS)) + .isNotNull(); + assertThat(registrations) + .as("the evicted session was signed back in") + .hasValue(registrationsBeforeEviction); + // The connection replays the login it captured to bring up a + // replacement channel, so that copy has to go too: kept, the + // next channel revives exactly the session the server ended. + assertThat(client.currentConnection().authenticationSnapshot()) + .as("the captured login outlived the session it established") + .isEmpty(); + } finally { + client.close().get(5, TimeUnit.SECONDS); + } + server.completeExceptionally(new IllegalStateException("test over")); + } + } + private static Response handleOldLeader( Request request, int oldLeaderPort, int newLeaderPort, AtomicInteger denials) { if (request.is(GET_CLUSTER_METADATA_CODE, OPERATION_NON_REPLICATED)) { @@ -235,7 +412,8 @@ private static Request readRequest(InputStream input) throws IOException { return new Request( Byte.toUnsignedInt(header[REQUEST_OPERATION_OFFSET]), fields.getInt(REQUEST_CODE_OFFSET), - fields.getLong(REQUEST_ID_OFFSET)); + fields.getLong(REQUEST_ID_OFFSET), + body); } private static void writeResponse(OutputStream output, Request request, Response response) throws IOException { @@ -310,10 +488,15 @@ private static void writeString(ByteBuf body, String value) { body.writeBytes(bytes); } - private record Request(int operation, int commandCode, long requestId) { + private record Request(int operation, int commandCode, long requestId, byte[] body) { boolean is(int expectedCode, int expectedOperation) { return commandCode == expectedCode && operation == expectedOperation; } + + /** The request body as text, for asserting which user a login names. */ + String bodyAsText() { + return new String(body, StandardCharsets.UTF_8); + } } private record Response(int command, int operation, int status, int evictionReason, ByteBuf body) { diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncTcpConnectionConcurrencyTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncTcpConnectionConcurrencyTest.java index 409e831474..354f09d95b 100644 --- a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncTcpConnectionConcurrencyTest.java +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/AsyncTcpConnectionConcurrencyTest.java @@ -111,7 +111,7 @@ void shouldCorrelateConcurrentPartitionResponsesInReverseOrder() throws Exceptio Duration.ofHours(1), 1024 * 1024, null, - () -> {}, + errorCode -> {}, ignored -> {}); try { connection.connect().get(5, TimeUnit.SECONDS); @@ -308,7 +308,7 @@ private static AsyncTcpConnection newConnection( heartbeatInterval, 1024 * 1024, null, - () -> {}, + errorCode -> {}, ignored -> {}); } diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/LeaderAwarenessTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/LeaderAwarenessTest.java index 00540cd23c..a847acb1c7 100644 --- a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/LeaderAwarenessTest.java +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/LeaderAwarenessTest.java @@ -206,6 +206,10 @@ class FindLeaderElsewhere { private final ConnectionInfo currentTarget = new ConnectionInfo("iggy-follower", 8092); private Optional findLeader(Supplier> fetch) { + return lookUpLeader(fetch).redirect(); + } + + private LeaderAwareness.LeaderLookup lookUpLeader(Supplier> fetch) { return LeaderAwareness.findLeaderElsewhere(fetch, currentTarget, BUDGET, INTERVAL) .orTimeout(30, TimeUnit.SECONDS) .join(); @@ -248,7 +252,8 @@ void shouldGiveUpOnLeaderlessClusterAfterBudget() { Duration.ofMillis(100), INTERVAL) .orTimeout(30, TimeUnit.SECONDS) - .join(); + .join() + .redirect(); assertThat(leader).isEmpty(); assertThat(fetchCount.get()).isGreaterThan(1); @@ -276,6 +281,22 @@ void shouldGiveUpWhenMetadataFetchThrowsSynchronously() { assertThat(leader).isEmpty(); } + @Test + void shouldRememberEveryNodeTheRosterNamesEvenWhileLeaderless() { + var lookup = LeaderAwareness.findLeaderElsewhere( + () -> CompletableFuture.completedFuture(leaderlessCluster()), + currentTarget, + Duration.ofMillis(100), + INTERVAL) + .orTimeout(30, TimeUnit.SECONDS) + .join(); + + // A leaderless roster still names where the nodes are, and that is + // what a redial needs. + assertThat(lookup.redirect()).isEmpty(); + assertThat(lookup.endpoints()).isNotEmpty(); + } + @Test void shouldStayWithoutPollingWhenAlreadyOnLeader() { var fetchCount = new AtomicInteger(); @@ -289,13 +310,87 @@ void shouldStayWithoutPollingWhenAlreadyOnLeader() { BUDGET, INTERVAL) .orTimeout(30, TimeUnit.SECONDS) - .join(); + .join() + .redirect(); assertThat(leader).isEmpty(); assertThat(fetchCount).hasValue(1); } } + @Nested + class NodeTargets { + + // A node with the tcp transport disabled cannot be dialed over tcp, so + // it must not join the redial candidates: dialing port 0 fails, and it + // would spend one turn of every rotation. + @Test + void shouldSkipNodesWithoutATcpEndpoint() { + var metadata = cluster( + node("tcp-node", "iggy-0", 8091, ClusterNodeRole.Leader, ClusterNodeStatus.Healthy), + node("http-only-node", "iggy-1", 0, ClusterNodeRole.Follower, ClusterNodeStatus.Healthy)); + + var targets = LeaderAwareness.nodeTargets(metadata); + + assertThat(targets).containsExactly(new ConnectionInfo("iggy-0", 8091)); + } + + // Unhealthy nodes stay: a node that is down now is where the cluster + // says it lives, and a redial candidate is a place to try, not a + // promise that it answers. + @Test + void shouldKeepUnhealthyNodesThatStillHaveATcpEndpoint() { + var metadata = cluster( + node("leader-node", "iggy-0", 8091, ClusterNodeRole.Leader, ClusterNodeStatus.Healthy), + node("down-node", "iggy-1", 8092, ClusterNodeRole.Follower, ClusterNodeStatus.Unreachable)); + + var targets = LeaderAwareness.nodeTargets(metadata); + + assertThat(targets).containsExactly(new ConnectionInfo("iggy-0", 8091), new ConnectionInfo("iggy-1", 8092)); + } + + // An inconclusive check names no endpoint, which is what lets the + // client keep the last roster it read: replacing it with an empty list + // would erase the candidates exactly when the cluster is unreachable. + @Test + void shouldNameNoEndpointWhenTheRosterCannotBeRead() { + var lookup = LeaderAwareness.LeaderLookup.inconclusive(); + + assertThat(lookup.redirect()).isEmpty(); + assertThat(lookup.endpoints()).isEmpty(); + } + } + + @Nested + class SameAddress { + + // The redial dedup runs on the Netty event loop, so it compares + // spellings only. A hostname and the address it resolves to are two + // candidates there, and one endpoint for the leader check. + @Test + void shouldCompareSpellingsWithoutResolving() { + assertThat(LeaderAwareness.isSameSpelling( + new ConnectionInfo("IGGY-0", 8090), new ConnectionInfo("iggy-0", 8090))) + .isTrue(); + assertThat(LeaderAwareness.isSameSpelling( + new ConnectionInfo("[::1]", 8090), new ConnectionInfo("::1", 8090))) + .isTrue(); + assertThat(LeaderAwareness.isSameSpelling( + new ConnectionInfo("localhost", 8090), new ConnectionInfo("127.0.0.1", 8090))) + .isFalse(); + assertThat(LeaderAwareness.isSameSpelling( + new ConnectionInfo("iggy-0", 8090), new ConnectionInfo("iggy-0", 8091))) + .isFalse(); + } + + @Test + void shouldTreatLoopbackSpellingsAsOneEndpointForTheLeaderCheck() { + assertThat(LeaderAwareness.isSameAddress( + new ConnectionInfo("localhost", 8090), new ConnectionInfo("127.0.0.1", 8090))) + .isTrue(); + } + } + @Nested class RedirectionState { diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/ReconnectPlanTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/ReconnectPlanTest.java index ce833a58cb..0c45587b77 100644 --- a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/ReconnectPlanTest.java +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/ReconnectPlanTest.java @@ -19,7 +19,6 @@ package org.apache.iggy.client.async.tcp; -import org.apache.iggy.client.ConnectionInfo; import org.apache.iggy.config.RetryPolicy; import org.junit.jupiter.api.Test; @@ -29,23 +28,6 @@ class ReconnectPlanTest { - private final ConnectionInfo seed = new ConnectionInfo("seed-node", 8090); - private final ConnectionInfo current = new ConnectionInfo("leader-node", 8090); - - @Test - void shouldAlternateBetweenCurrentAndSeed() { - assertThat(ReconnectPlan.target(current, seed, 1)).isEqualTo(current); - assertThat(ReconnectPlan.target(current, seed, 2)).isEqualTo(seed); - assertThat(ReconnectPlan.target(current, seed, 3)).isEqualTo(current); - assertThat(ReconnectPlan.target(current, seed, 4)).isEqualTo(seed); - } - - @Test - void shouldDialOnlyOneAddressWhenNeverRedirected() { - assertThat(ReconnectPlan.target(seed, seed, 1)).isEqualTo(seed); - assertThat(ReconnectPlan.target(seed, seed, 2)).isEqualTo(seed); - } - @Test void shouldKeepFixedDelayConstant() { var policy = RetryPolicy.fixedDelay(12, Duration.ofSeconds(5)); diff --git a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandlerTest.java b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandlerTest.java index e8069b9159..4a9e3bc2d6 100644 --- a/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandlerTest.java +++ b/foreign/java/java-sdk/src/test/java/org/apache/iggy/client/async/tcp/vsr/VsrResponseHandlerTest.java @@ -39,7 +39,11 @@ class VsrResponseHandlerTest { private final ConsensusSession session = new ConsensusSession(); private final AtomicInteger evictions = new AtomicInteger(); - private final VsrResponseHandler handler = new VsrResponseHandler(session, evictions::incrementAndGet); + private final AtomicInteger lastEvictionReason = new AtomicInteger(); + private final VsrResponseHandler handler = new VsrResponseHandler(session, errorCode -> { + evictions.incrementAndGet(); + lastEvictionReason.set(errorCode); + }); private final EmbeddedChannel channel = new EmbeddedChannel(handler); @AfterEach @@ -159,6 +163,9 @@ void shouldMapEvictionReasonAndResetSession() { assertThat(rawErrorCode(future)).isEqualTo(42); assertThat(session.isBound()).isFalse(); assertThat(evictions).hasValue(1); + // The reason reaches the listener, which has to tell an eviction the + // server decided on from a transport-shaped one. + assertThat(lastEvictionReason).hasValue(42); } @Test @@ -171,6 +178,7 @@ void shouldCloseChannelOnEvictionWithoutPendingRequest() { assertThat(session.isBound()).isFalse(); assertThat(evictions).hasValue(1); + assertThat(lastEvictionReason).hasValue(VsrHeaders.ERROR_STALE_CLIENT); assertThat(frame.refCnt()).isZero(); assertThat(channel.isActive()).isFalse(); } diff --git a/foreign/node/src/client/client.connection.test.ts b/foreign/node/src/client/client.connection.test.ts index ed1f6428fa..6e1dce2742 100644 --- a/foreign/node/src/client/client.connection.test.ts +++ b/foreign/node/src/client/client.connection.test.ts @@ -393,6 +393,233 @@ describe('IggyConnection', () => { } ); + it('rotates a redial through the roster it learned while connected', + async () => { + const seed = await startServer(); + const seedPort = (seed.address() as AddressInfo).port; + const connection = new IggyConnection(connectionConfig(seed)); + connection.on('error', () => undefined); + try { + connection.rememberRoster([ + { host: '127.0.0.1', port: seedPort }, + { host: '127.0.0.1', port: seedPort + 1 }, + { host: '127.0.0.1', port: seedPort + 2 } + ]); + // The endpoint the client is on leads, the roster follows, and the + // roster's copy of that endpoint does not earn a second attempt. + assert.deepEqual( + connection._redialCandidates().map((options) => options.port), + [seedPort, seedPort + 1, seedPort + 2] + ); + } finally { + connection._destroy(); + await new Promise((resolve) => seed.close(() => resolve())); + } + } + ); + + it('dials the endpoint it is on, then the seed, then the roster', + async () => { + const seed = await startServer(); + const seedPort = (seed.address() as AddressInfo).port; + const connection = new IggyConnection(connectionConfig(seed)); + connection.on('error', () => undefined); + try { + // A redirect moves the client off its seed; the seed is still the one + // endpoint the caller vouched for, so it comes before a roster the + // cluster may have reshaped since. + connection.config.options = { + ...connection.config.options, + port: seedPort + 9 + }; + connection.rememberRoster([{ host: '127.0.0.1', port: seedPort + 5 }]); + + assert.deepEqual( + connection._redialCandidates().map((options) => options.port), + [seedPort + 9, seedPort, seedPort + 5] + ); + } finally { + connection._destroy(); + await new Promise((resolve) => seed.close(() => resolve())); + } + } + ); + + it('counts endpoints that only differ in spelling once', + async () => { + const seed = await startServer(); + const seedPort = (seed.address() as AddressInfo).port; + const connection = new IggyConnection(connectionConfig(seed)); + connection.on('error', () => undefined); + try { + // The loopback aliases and an IPv4-mapped address all name the endpoint + // the client is already on, so none of them earns a dial of its own. + connection.rememberRoster([ + { host: 'localhost', port: seedPort }, + { host: '::1', port: seedPort }, + { host: '::ffff:127.0.0.1', port: seedPort }, + { host: '127.0.0.1', port: seedPort + 1 } + ]); + + assert.deepEqual( + connection._redialCandidates().map((options) => options.port), + [seedPort, seedPort + 1] + ); + } finally { + connection._destroy(); + await new Promise((resolve) => seed.close(() => resolve())); + } + } + ); + + it('skips the first backoff when another endpoint is known', + async () => { + // The endpoint the client is on is dead and a live one sits behind it in + // the roster: waiting out the interval before the first pass would push + // the failover past what the caller waits for, and the node just lost may + // be gone for good. + const dead = await startServer(); + const deadPort = (dead.address() as AddressInfo).port; + await new Promise((resolve) => dead.close(() => resolve())); + const live = await startServer(); + const livePort = (live.address() as AddressInfo).port; + + const interval = 3000; + const connection = new IggyConnection({ + transport: 'TCP', + options: { host: '127.0.0.1', port: deadPort }, + credentials: { username: 'iggy', password: 'iggy' }, + reconnect: { enabled: true, interval, maxRetries: 3 }, + maxResponseFrameSize: FRAME_LIMIT + }); + connection.on('error', () => undefined); + try { + connection.rememberRoster([{ host: '127.0.0.1', port: livePort }]); + const dialed = once(live, 'connection'); + const started = Date.now(); + void connection.connect().catch(() => undefined); + await dialed; + + assert.ok(Date.now() - started < interval, + 'the failover waited out the backoff before its first pass' + ); + } finally { + connection._destroy(); + await new Promise((resolve) => live.close(() => resolve())); + } + } + ); + + it('bounds a dial that never becomes usable when others are queued behind it', + async () => { + // Plain TCP behind a TLS client: the socket connects, so only a bound on + // the handshake ends the attempt. The endpoint behind it is dead, so the + // pass has to end on its own rather than hang on the first one. + const silent = await startServer(); + const silentPort = (silent.address() as AddressInfo).port; + const held: Socket[] = []; + silent.on('connection', (socket) => { held.push(socket); }); + const dead = await startServer(); + const deadPort = (dead.address() as AddressInfo).port; + await new Promise((resolve) => dead.close(() => resolve())); + + const connection = new IggyConnection({ + transport: 'TLS', + options: { + host: '127.0.0.1', + port: silentPort, + rejectUnauthorized: false + }, + credentials: { username: 'iggy', password: 'iggy' }, + reconnect: { enabled: true, interval: 10, maxRetries: 3 }, + maxResponseFrameSize: FRAME_LIMIT + }); + connection.on('error', () => undefined); + try { + connection.rememberRoster([{ host: '127.0.0.1', port: deadPort }]); + void connection.connect().catch(() => undefined); + + // Unbounded, the first dial never ends and this endpoint is dialed + // exactly once, forever. + const deadline = Date.now() + 8_000; + while (held.length < 2 && Date.now() < deadline) + await new Promise((resolve) => setTimeout(resolve, 50)); + + assert.ok(held.length >= 2, + 'a dial that never became usable held the pass' + ); + assert.equal(connection.connected, false); + } finally { + connection._destroy(); + held.forEach((socket) => socket.destroy()); + await new Promise((resolve) => silent.close(() => resolve())); + } + } + ); + + it('stops a redial pass that is destroyed part-way through', + async () => { + // The endpoint the client is on is dead, so every dial to it is refused + // - and the live roster endpoint behind it is what the pass would reach + // next, unless the destroy in between stops the pass. + const dead = await startServer(); + const deadPort = (dead.address() as AddressInfo).port; + await new Promise((resolve) => dead.close(() => resolve())); + const live = await startServer(); + const livePort = (live.address() as AddressInfo).port; + let accepted = 0; + live.on('connection', () => { accepted += 1; }); + + const connection = new IggyConnection({ + transport: 'TCP', + options: { host: '127.0.0.1', port: deadPort }, + credentials: { username: 'iggy', password: 'iggy' }, + reconnect: { enabled: true, interval: 10, maxRetries: 3 }, + maxResponseFrameSize: FRAME_LIMIT + }); + let destroyed = false; + let connectsAfterDestroy = 0; + connection.on('connect', () => { + if (destroyed) + connectsAfterDestroy += 1; + }); + try { + connection.rememberRoster([{ host: '127.0.0.1', port: livePort }]); + + // The first failure is the initial connect, which is what starts the + // redial pass; the next one is that pass's first candidate, so + // destroying there lands between two candidates rather than before the + // pass. + const destroyedMidPass = new Promise((resolve) => { + let failures = 0; + connection.on('error', () => { + failures += 1; + if (failures < 2 || destroyed) + return; + connection._destroy(); + destroyed = true; + resolve(); + }); + }); + + await connection.connect().catch(() => undefined); + await destroyedMidPass; + await new Promise((resolve) => setTimeout(resolve, 100)); + + assert.equal(accepted, 0, + 'a destroyed connection must not keep dialing the rest of the pass' + ); + assert.equal(connectsAfterDestroy, 0, + 'a destroyed connection must not announce a connection' + ); + assert.equal(connection.connected, false); + } finally { + connection._destroy(); + await new Promise((resolve) => live.close(() => resolve())); + } + } + ); + it('settles a dial in flight when a redirect replaces the socket', async () => { const seed = await startServer(); diff --git a/foreign/node/src/client/client.connection.ts b/foreign/node/src/client/client.connection.ts index 767da571d8..ea5c7dcbd9 100644 --- a/foreign/node/src/client/client.connection.ts +++ b/foreign/node/src/client/client.connection.ts @@ -67,9 +67,25 @@ const getTransport = (config: ClientConfig): Socket => { } }; +/** One node of the cluster, as a redial candidate. */ +export type Endpoint = { host: string, port: number }; + +/** + * Bound on one dial while other endpoints are queued behind it. Neither the + * connect nor the TLS handshake has a deadline of its own, so a node whose syns + * are dropped -- or one that accepts TCP and never answers the ClientHello -- + * would hold the whole pass. Matches the Rust SDK. + */ +const FAILOVER_DIAL_TIMEOUT_MS = 2_000; + /** * Default reconnection settings. - * Attempts reconnection every 5 seconds, up to 12 times. + * + * One retry is one full pass over every endpoint the client knows, so this is + * twelve passes rather than twelve dials, waiting 5 seconds between them. The + * first pass runs at once when more than one endpoint is known: the node just + * lost may be gone for good, and pausing before dialing a survivor only pushes + * the failover past the interval a caller is willing to wait. */ const DefaultReconnectOption: ReconnectOption = { enabled: true, @@ -118,6 +134,13 @@ export class IggyConnection extends EventEmitter { private reconnectPromise?: Promise; /** Endpoint the client was configured with, kept across leader redirects */ private readonly seedOptions: ClientConfig['options']; + /** + * Every node the roster named on the last read, kept as redial candidates. + * A node dies together with its address, and the roster is unreachable + * exactly when it is needed, so it has to have been remembered while the + * connection was still healthy. + */ + private rosterEndpoints: Endpoint[]; /** Incremental response frame decoder */ private responseDecoder: ResponseFrameDecoder; @@ -135,6 +158,7 @@ export class IggyConnection extends EventEmitter { this.ending = false; this.reconnectOption = { ...DefaultReconnectOption, ...config.reconnect }; this.seedOptions = { ...config.options }; + this.rosterEndpoints = []; this.reconnectCount = 0; this.connectPromise = undefined; this.reconnectPromise = undefined; @@ -173,7 +197,10 @@ export class IggyConnection extends EventEmitter { this.emit('error', err); }); - socket.once('connect', () => { + // The readiness event, not 'connect': on TLS the socket is only usable + // once the handshake completes, and writing a request before that would + // announce a connection the peer has not agreed to yet. + socket.once(this._readyEvent(), () => { if (this.socket !== socket) return; debug('socket/connect event'); @@ -217,7 +244,13 @@ export class IggyConnection extends EventEmitter { this.connecting = true; const socket = this.socket; - const connectPromise = this._waitForConnection(socket); + // Bounded here too when the client knows somewhere else to go: this dial + // is not part of a pass, so an endpoint that never becomes usable would + // hold it with no timer of its own and the redial pass would never start. + const connectPromise = this._dialWithin( + socket, + this._redialCandidates().length > 1 + ); this.connectPromise = connectPromise; const clearConnectPromise = () => { if (this.connectPromise === connectPromise) @@ -227,10 +260,57 @@ export class IggyConnection extends EventEmitter { return connectPromise; } + /** + * Waits for one dial, bounded while other endpoints are queued behind it. + * + * A socket has no connect deadline of its own and there is no 'timeout' + * listener on it, so a node whose syns are dropped holds the pass for the + * whole OS connect timeout -- and it leads every pass, because the current + * endpoint only moves on success. The bound covers the TLS handshake too, + * which is what `_readyEvent()` waits for and has no deadline of its own + * either. It matches the Rust, Go and C# SDKs'. + */ + private async _dialWithin(socket: Socket, bounded: boolean): Promise { + if (!bounded) + return this._waitForConnection(socket); + + let expire: NodeJS.Timeout | undefined; + const bound = new Promise((_resolve, reject) => { + expire = setTimeout(() => { + // Destroying it makes the pending dial settle and releases the handle; + // left alone it would keep the event loop alive. + socket.destroy(); + reject(new Error( + `dial exceeded ${FAILOVER_DIAL_TIMEOUT_MS}ms` + )); + }, FAILOVER_DIAL_TIMEOUT_MS); + expire.unref?.(); + }); + + try { + return await Promise.race([this._waitForConnection(socket), bound]); + } finally { + clearTimeout(expire); + } + } + + /** + * The event that says a socket can carry a request. + * + * On TLS that is 'secureConnect', not 'connect': the latter fires as soon as + * the TCP handshake completes, so waiting on it would treat a peer that + * never answers the ClientHello as connected and leave the handshake with no + * deadline at all. + */ + private _readyEvent(): 'connect' | 'secureConnect' { + return this.config.transport === 'TLS' ? 'secureConnect' : 'connect'; + } + private _waitForConnection(socket: Socket): Promise { + const ready = this._readyEvent(); return new Promise((resolve, reject) => { const cleanup = () => { - socket.removeListener('connect', resolveConnect); + socket.removeListener(ready, resolveConnect); socket.removeListener('error', rejectConnect); socket.removeListener('close', rejectClosed); }; @@ -247,7 +327,7 @@ export class IggyConnection extends EventEmitter { }; socket.once('error', rejectConnect); socket.once('close', rejectClosed); - socket.once('connect', resolveConnect); + socket.once(ready, resolveConnect); }); } @@ -300,11 +380,18 @@ export class IggyConnection extends EventEmitter { ): Promise { let lastError = initialError; let expectedSocket = this.socket; - let attempt = 0; + let firstPass = true; while (enabled && this.reconnectCount < maxRetries) { this.connecting = true; this.reconnectCount += 1; - await waitForReconnect(interval); + const candidates = this._redialCandidates(); + // The backoff paces retries against a single endpoint. With other + // endpoints known there is somewhere else to go, and pausing first only + // pushes the failover past the interval a caller is willing to wait; + // later passes still back off. + if (!firstPass || candidates.length === 1) + await waitForReconnect(interval); + firstPass = false; if (this.ending) throw new Error('connection is closed', { cause: lastError }); // A redirect may replace the socket at any point. Defer to the active @@ -312,24 +399,41 @@ export class IggyConnection extends EventEmitter { if (this.connected || this.socket !== expectedSocket) return this.connect(); - const options = this._reconnectTarget(attempt); - attempt += 1; - const socket = this._installSocket( - getTransport({ ...this.config, options }) - ); - this.socket = socket; - expectedSocket = socket; - try { - await this._waitForConnection(socket); - if (this.socket !== socket) + // Every endpoint gets its turn inside one attempt, so a full pass over + // the cluster costs one retry rather than one per endpoint: a pass that + // stopped at the first refusal would never reach the survivors of a + // client configured for a single retry. + for (const options of candidates) { + // Re-checked every iteration, not once above the loop: a destroy or a + // redirect mid-pass has to stop the pass. Left running, the next + // endpoint that answers would leave an open socket nobody closes, a + // 'connect' event after the destroy, and the process alive. + if (this.ending) + throw new Error('connection is closed', { cause: lastError }); + if (this.socket !== expectedSocket) return this.connect(); - this.config.options = options; - return this; - } catch (error) { - lastError = error instanceof Error - ? error - : new Error(String(error)); - debug('reconnect attempt failed', lastError); + + const socket = this._installSocket( + getTransport({ ...this.config, options }) + ); + this.socket = socket; + expectedSocket = socket; + try { + await this._dialWithin(socket, candidates.length > 1); + if (this.ending) { + socket.destroy(); + throw new Error('connection is closed', { cause: lastError }); + } + if (this.socket !== socket) + return this.connect(); + this.config.options = options; + return this; + } catch (error) { + lastError = error instanceof Error + ? error + : new Error(String(error)); + debug('reconnect attempt failed', lastError); + } } } @@ -341,16 +445,48 @@ export class IggyConnection extends EventEmitter { } /** - * Alternates reconnect dials between the current endpoint and the - * configured seed. After a leader redirect the current endpoint may die - * with the leader, and the seed is the way back to the rest of the cluster. + * Records the cluster roster as redial candidates. + * + * Replaced wholesale rather than merged: the roster is the cluster's own + * answer about where its nodes are, so a node it dropped stops being + * dialed. The configured seed is kept separately and outlives it. */ - private _reconnectTarget(attempt: number): ClientConfig['options'] { - const current = this.config.options; - if (this.seedOptions.host === current.host && - this.seedOptions.port === current.port) - return current; - return attempt % 2 === 0 ? current : this.seedOptions; + rememberRoster(endpoints: Endpoint[]): void { + if (endpoints.length === 0) + return; + this.rosterEndpoints = endpoints; + } + + /** + * Endpoints a redial rotates through, likeliest first: where the client + * currently is, the endpoint it was configured with, then the roster it + * learned while connected. After a leader redirect the current endpoint may + * die with the leader, and the rest of the list is the way back to the + * cluster. + * + * Duplicates are dropped by spelling: the loopback aliases and an + * IPv4-mapped IPv6 address collapse onto one endpoint. Names are not + * resolved, so a seed given as a DNS name and the roster's IP for the same + * node still count as two candidates -- one wasted dial per pass, not a + * correctness problem. + */ + _redialCandidates(): ClientConfig['options'][] { + const candidates = [this.config.options]; + const known = [ + this.seedOptions, + ...this.rosterEndpoints.map( + ({ host, port }) => ({ ...this.config.options, host, port }) + ) + ]; + for (const candidate of known) { + const duplicate = candidates.some( + (existing) => existing.port === candidate.port && + normalizeHost(existing.host) === normalizeHost(candidate.host) + ); + if (!duplicate) + candidates.push(candidate); + } + return candidates; } async redirect(host: string, port: number) { diff --git a/foreign/node/src/client/client.socket.test.ts b/foreign/node/src/client/client.socket.test.ts index 9605e8d86b..e938c7603a 100644 --- a/foreign/node/src/client/client.socket.test.ts +++ b/foreign/node/src/client/client.socket.test.ts @@ -504,6 +504,111 @@ describe('VSR client socket', () => { } }); + // The node a client authenticated on dies; its next command has to complete + // on a survivor the roster named, under a session established there. + // Mirrors `core/integration/tests/cluster/failover_client_continuity.rs`. + it('resumes on a survivor after the node it authenticated on dies', + async () => { + const primarySockets = new Set(); + let primaryDead = false; + + const survivor = await startVsrServer((frame, socket) => { + const operation = frame.readUInt8(REQUEST_OFFSET.operation); + if (operation === Operation.Register) { + socket.write(replyFrame(Operation.Register, registerReplyBody())); + return; + } + const code = frame.readUInt32LE(REQUEST_OFFSET.reserved); + if (code === COMMAND_CODE.GetClusterMetadata) { + // The survivor leads once the primary is gone. + socket.write(replyFrame( + Operation.NonReplicated, + twoNodeMetadataBody(primary.port, survivor.port) + )); + return; + } + socket.write(replyFrame(operation)); + }); + + const primary = await startVsrServer((frame, socket) => { + primarySockets.add(socket); + if (primaryDead) { + socket.destroy(); + return; + } + const operation = frame.readUInt8(REQUEST_OFFSET.operation); + if (operation === Operation.Register) { + socket.write(replyFrame(Operation.Register, registerReplyBody())); + return; + } + const code = frame.readUInt32LE(REQUEST_OFFSET.reserved); + if (code === COMMAND_CODE.GetClusterMetadata) { + // The primary leads, so the login settles here and the roster is + // only remembered, not acted on, until the node dies. + socket.write(replyFrame( + Operation.NonReplicated, + twoNodeMetadataBody(survivor.port, primary.port) + )); + return; + } + socket.write(replyFrame(operation)); + }); + + const config: ClientConfig = { + ...vsrConfig(primary.port), + reconnect: { enabled: true, interval: 1, maxRetries: 3 } + }; + const client = new CommandResponseStream(config); + try { + await client.authenticate(config.credentials); + await client.sendCommand(60_021, Buffer.alloc(0)); + assert.ok( + primary.frames.some( + (frame) => frame.readUInt32LE(REQUEST_OFFSET.reserved) === 60_021 + ), + 'the live primary answered the first command' + ); + + primaryDead = true; + for (const socket of primarySockets) + socket.destroy(); + await primary.close(); + + // The attempt in flight when the socket died is allowed to fail; the + // one after it has to land on the survivor. Two attempts, not a + // polling loop: the comment above promises at most one failed + // submission, and a loop of twenty would pass with nineteen failures. + let resumed = false; + let lastError: unknown; + for (let attempt = 0; attempt < 2 && !resumed; attempt += 1) { + try { + await client.sendCommand(60_021, Buffer.alloc(0)); + resumed = true; + } catch (error) { + lastError = error; + } + } + assert.ok(resumed, `the client never resumed: ${String(lastError)}`); + + const operations = survivor.frames.map( + (frame) => frame.readUInt8(REQUEST_OFFSET.operation) + ); + assert.ok( + operations.includes(Operation.Register), + 'the client signed in again on the survivor' + ); + assert.ok( + survivor.frames.some( + (frame) => frame.readUInt32LE(REQUEST_OFFSET.reserved) === 60_021 + ), + 'the command landed on the survivor the roster named' + ); + } finally { + client.destroy(); + await survivor.close(); + } + }); + it('keeps a single-node login on its node', async () => { const server = await startVsrServer( (frame, socket) => singleNodeHandler(server.port)(frame, socket) @@ -864,6 +969,38 @@ describe('VSR client socket', () => { } }); + it('keeps re-issuing a not-admitted request while the roster still names this node', + async () => { + const server = await startVsrServer((frame, socket) => { + if (frame.readUInt32LE(REQUEST_OFFSET.reserved) === 60_031) { + socket.write(replyFrame(Operation.NonReplicated, Buffer.alloc(0), 58)); + return; + } + singleNodeHandler(server.port)(frame, socket); + }); + const client = new CommandResponseStream(vsrConfig(server.port)); + try { + await client.authenticate(vsrConfig(server.port).credentials); + + // A refusal the roster cannot explain is a wait, not a verdict: an + // election may still be in flight, so the request keeps going for its + // whole budget instead of failing after the first re-check window. + const pending = client.sendCommand(60_031, Buffer.alloc(0)); + const outcome = await Promise.race([ + pending.then(() => 'answered', () => 'gave up'), + new Promise((resolve) => { + setTimeout(() => resolve('still trying'), 4_000).unref(); + }) + ]); + + assert.equal(outcome, 'still trying'); + } finally { + client.destroy(); + await server.close(); + } + } + ); + it('keeps a typed transient error and session at its retry deadline', async () => { const server = await startVsrServer((frame, socket) => { diff --git a/foreign/node/src/client/client.socket.ts b/foreign/node/src/client/client.socket.ts index 2f02764c6c..43d378b844 100644 --- a/foreign/node/src/client/client.socket.ts +++ b/foreign/node/src/client/client.socket.ts @@ -24,7 +24,7 @@ import type { } from '../client/client.type.js'; import { ResponseError, responseError } from '../wire/error.utils.js'; import { debug } from './client.debug.js'; -import { IggyConnection } from './client.connection.js'; +import { type Endpoint, IggyConnection } from './client.connection.js'; import { LOGIN, LOGIN_WITH_TOKEN, LOGOUT, PING } from '../wire/index.js'; import { GET_CLUSTER_METADATA } from '../wire/cluster/get-cluster-metadata.command.js'; import { COMMAND_CODE } from '../wire/command.code.js'; @@ -44,6 +44,23 @@ const LEADERLESS_POLL_INTERVAL_MS = 250; const MAX_LEADER_REDIRECTS = 3; const TRANSIENT_NOT_COMMITTED = 57; const TRANSIENT_NOT_ACCEPTED = 58; +/** + * How long a `TRANSIENT_NOT_ACCEPTED` request replays on the same connection + * before the roster is re-read. A node that stopped being primary refuses + * forever, so replaying alone never recovers. Matches the Rust SDK. + */ +const VSR_FAILOVER_CHECK_MS = 2_000; + +/** + * A request the current node keeps refusing as not-admitted. Carries the + * refusal so the caller can surface it when the roster turns out to still name + * this node as the leader. Never escapes `sendCommand`. + */ +class LeaderMovedError extends Error { + constructor(readonly refusal: ResponseError) { + super('the node refused the request as not-admitted; re-reading the roster'); + } +} /** * Command codes that can be executed without authentication. @@ -64,6 +81,8 @@ type Job = { payload: Buffer, /** Whether to parse the response */ handleResponse: boolean, + /** When the whole request gives up, however often it is re-issued */ + deadline: number, /** Promise resolve function */ resolve: (v: CommandResponse | PromiseLike) => void, /** Promise reject function */ @@ -99,6 +118,12 @@ export class CommandResponseStream extends EventEmitter { private authenticationPromise?: Promise; /** Whether a login is already being moved to the leader */ private settlingLeader: boolean; + /** + * Whether a refused request is already re-checking the leader. The roster + * read that re-check runs can be refused the same way, and answering a + * leader check with another leader check would recurse. + */ + private followingLeaderMove = false; /** How long a leaderless roster is polled before settling in place */ private leaderlessWaitBudget: number; /** Delay between roster reads while the cluster elects */ @@ -186,21 +211,44 @@ export class CommandResponseStream extends EventEmitter { if (!this.isAuthenticated && !this.isUnloggedCommand(command)) await this.authenticate(this.options.credentials); - const response = await new Promise( - (resolve, reject) => { - const job = { - command, - payload, - handleResponse, - resolve, - reject - }; - if (last) - this._execQueue.push(job); - else - this._execQueue.unshift(job); - this._processQueue(); - }); + // The roster read is itself a queued command and the queue is + // single-flighted, so the leader re-check cannot happen inside + // `_processVsr`. The refusal comes back out here instead, where the + // queue is free, and the command is re-issued on the node that now + // leads. + // + // A not-admitted refusal means the request was never applied, so it is + // re-issued for the whole request budget rather than given up on after + // one window: the roster can still name this node -- an election in + // flight, a leader that has not moved yet -- and that is a wait, not a + // verdict. + // One budget for the whole request: the transient replays on a + // connection, the leader re-checks, and the re-issues after a move all + // spend it, so a request cannot outlive it by moving. + const deadline = Date.now() + VSR_RESPONSE_TIMEOUT_MS; + let response: CommandResponse; + for (;;) { + try { + response = await this._queueCommand(command, payload, handleResponse, + last, deadline); + break; + } catch (error) { + if (!(error instanceof LeaderMovedError)) + throw error; + // The roster read itself is refused: it runs through this same + // path, and re-checking the leader to answer a leader check would + // recurse. Its caller reads a failure as "stay where you are". + if (this.followingLeaderMove || Date.now() >= deadline) + throw responseError(command, error.refusal.errorCode); + await this._followLeaderMove(); + // A move drops the session with the socket it was bound to, so the + // re-issue would otherwise go out under no session: a replicated + // command fails client-side, a non-replicated one goes out with + // session 0. + if (!this.isAuthenticated && !this.isUnloggedCommand(command)) + await this.authenticate(this.options.credentials); + } + } if (!isLoginCommand(command) || this.settlingLeader) return response; this.settlingLeader = true; @@ -216,6 +264,73 @@ export class CommandResponseStream extends EventEmitter { } } + private _queueCommand( + command: number, + payload: Buffer, + handleResponse: boolean, + last: boolean, + deadline: number + ): Promise { + return new Promise((resolve, reject) => { + const job = { + command, + payload, + handleResponse, + deadline, + resolve, + reject + }; + if (last) + this._execQueue.push(job); + else + this._execQueue.unshift(job); + this._processQueue(); + }); + } + + /** + * Re-reads the roster and moves to the leader it names. + * + * Best effort: an unreadable roster, or one that still names this node, + * leaves the client where it is and the refused request is re-issued here + * anyway. Guarded against re-entry, since the roster read is itself a + * command that can be refused the same way. + * + * @returns Whether the client moved + */ + private async _followLeaderMove(): Promise { + if (this.followingLeaderMove) + return false; + this.followingLeaderMove = true; + try { + const leader = await this._readLeaderEndpoint(); + if (!leader || this.connection.isConnectedTo(leader.host, leader.port)) + return false; + debug(`the leader moved to ${leader.host}:${leader.port}, following it`); + await this.connection.redirect(leader.host, leader.port); + return true; + } catch (error) { + debug('the leader could not be re-checked, staying on this node', error); + return false; + } finally { + this.followingLeaderMove = false; + } + } + + private _rememberRoster(response: CommandResponse): void { + try { + const metadata = GET_CLUSTER_METADATA.deserialize(response); + this.connection.rememberRoster( + metadata.nodes + .filter((node) => node.endpoints.tcp !== 0) + .map((node) => ({ host: node.ip, port: node.endpoints.tcp })) + ); + } catch (error) { + debug('an unreadable roster leaves the redial candidates as they are', + error); + } + } + /** * Processes queued commands sequentially. * Emits 'finishQueue' when all commands are processed. @@ -228,9 +343,9 @@ export class CommandResponseStream extends EventEmitter { while (this._execQueue.length > 0 && this.connection.socket.writable) { const next = this._execQueue.shift(); if (!next) break; - const { command, payload, handleResponse, resolve, reject } = next; + const { command, payload, handleResponse, deadline, resolve, reject } = next; try { - resolve(await this._processNext(command, payload, handleResponse)); + resolve(await this._processNext(command, payload, handleResponse, deadline)); } catch (err) { reject(err); } @@ -254,38 +369,46 @@ export class CommandResponseStream extends EventEmitter { * @param command - Command code * @param payload - Command payload * @param handleResp - Whether to parse the response + * @param deadline - When the whole request gives up, shared with the leader + * re-checks and the re-issues after a move * @returns Promise resolving to the command response */ _processNext( command: number, payload: Buffer, - handleResp = true + handleResp = true, + deadline = Date.now() + VSR_RESPONSE_TIMEOUT_MS ): Promise { if (isLoginCommand(command) && this.isAuthenticated) - return this._processVsrLogin(command, payload, handleResp); - return this._processVsr(command, payload, handleResp); + return this._processVsrLogin(command, payload, handleResp, deadline); + return this._processVsr(command, payload, handleResp, deadline); } private async _processVsrLogin( command: number, payload: Buffer, - handleResp: boolean + handleResp: boolean, + deadline: number ): Promise { - await this._processVsr(LOGOUT.code, LOGOUT.serialize(), true); - return this._processVsr(command, payload, handleResp); + await this._processVsr(LOGOUT.code, LOGOUT.serialize(), true, deadline); + return this._processVsr(command, payload, handleResp, deadline); } private async _processVsr( command: number, payload: Buffer, - handleResp: boolean + handleResp: boolean, + deadline: number ): Promise { let requestWritten = false; try { const prepared = prepareVsrCommand(command, payload); // A transient retry must preserve all request identity fields. const frame = this.vsrSession.encode(prepared.command, prepared.payload); - const deadline = Date.now() + VSR_RESPONSE_TIMEOUT_MS; + // Derived from the request's own budget rather than read off the clock, + // so one request spends one budget however many times it is re-issued. + const notAcceptedDeadline = + deadline - VSR_RESPONSE_TIMEOUT_MS + VSR_FAILOVER_CHECK_MS; let lastTransientError: ResponseError | undefined; let parsed: CommandResponse; while (true) { @@ -316,6 +439,16 @@ export class CommandResponseStream extends EventEmitter { !isTransientVsrError(error.errorCode)) throw error; lastTransientError = error; + // A not-admitted refusal is a statement about who leads, not about + // load: a node that stopped being primary refuses forever, so + // replaying on this connection never recovers. Hand it back for a + // roster re-read once the window is spent. Not-committed (57) stays + // here: the request is in flight on this very node, and its outcome + // is unknown anywhere else. + if (error.errorCode === TRANSIENT_NOT_ACCEPTED && + !isLoginCommand(command) && + Date.now() >= notAcceptedDeadline) + throw new LeaderMovedError(error); const retryDelay = Math.min( VSR_RETRY_INTERVAL_MS, Math.max(0, deadline - Date.now()) @@ -335,8 +468,18 @@ export class CommandResponseStream extends EventEmitter { if (prepared.command === COMMAND_CODE.LogoutUser) { this._resetSession(); } + // Every roster read feeds the redial candidates, whoever asked for it + // and whatever it says: a node dies together with its address, the + // roster is unreachable exactly when it is needed, and reading it only + // during a login would leave the candidates stale between logins. + if (handleResp && command === GET_CLUSTER_METADATA.code) + this._rememberRoster(parsed); return parsed; } catch (error) { + // A not-admitted refusal is an answer, so the session is not in doubt + // and the request was never applied. + if (error instanceof LeaderMovedError) + throw error; // Once bytes were handed to the socket, a local transport or decode // failure leaves the request outcome ambiguous. Register a fresh session // rather than replaying that request under a different client identity. @@ -454,8 +597,7 @@ export class CommandResponseStream extends EventEmitter { * died between the login and this read), keeps the client on its current * node instead of failing a login that already succeeded. */ - private async _readLeaderEndpoint(): - Promise<{ host: string, port: number } | undefined> { + private async _readLeaderEndpoint(): Promise { // A cluster can be transiently leaderless: a restarted node cedes the // primaryship its stale view assigns it, and the roster reports no leader // until the peers' election completes. That window is roughly one heartbeat @@ -477,6 +619,9 @@ export class CommandResponseStream extends EventEmitter { GET_CLUSTER_METADATA.serialize(), { last: false } ); + // The redial candidates are fed by `_processVsr` for every roster + // read, leaderless ones included: a roster with no leader still names + // where the nodes are. const metadata = GET_CLUSTER_METADATA.deserialize(response); if (metadata.nodes.length <= 1) return undefined; diff --git a/foreign/node/src/client/client.type.ts b/foreign/node/src/client/client.type.ts index 40c29bc2cc..63703005f2 100644 --- a/foreign/node/src/client/client.type.ts +++ b/foreign/node/src/client/client.type.ts @@ -100,9 +100,16 @@ export type TransportType = typeof Transports[number]; export type ReconnectOption = { /** Whether automatic reconnection is enabled */ enabled: boolean, - /** Interval between reconnection attempts in milliseconds */ + /** + * Milliseconds to wait between passes. The first pass runs at once when more + * than one endpoint is known. + */ interval: number, - /** Maximum number of reconnection attempts */ + /** + * Maximum number of passes over the known endpoints. One pass dials the + * endpoint the client is on, the endpoint it was configured with, and every + * node the roster named, so this counts passes rather than dials. + */ maxRetries: number }