From 402d43ae55a516c8189496e8140c535fe2904b89 Mon Sep 17 00:00:00 2001 From: userFRM Date: Thu, 30 Jul 2026 21:34:18 +0200 Subject: [PATCH] hmds: release the connection when the transport is given up on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Giving up on the HMDS transport set the disconnected flag and left the dead socket in place. `maybe_spawn_hmds_reconnect` returns early while a connection is present, so it never scheduled an attempt and the transport was stuck for the life of the process. The heartbeat block is guarded on the same pair, so nothing further was sent on it either, and historical requests issued afterwards targeted a connection nothing was going to replace. Both routes there did it: the liveness timeout, and the ordinary receive error — a closed peer, or any other failure out of `try_recv`, which is how the transport usually dies. The flag and the connection now move together in one method that both call, so setting one without the other is not something a caller can express. A reconnect no longer inherits the dead session's outstanding probe. The liveness check sends a TestRequest only when none is pending, so a stale one suppressed every probe on the new connection and it went from silent to declared dead without being asked anything. The warm-up window starts with the new session as well, rather than being counted from the old one and already expired. CCP and the farm gate their reconnects on the flag alone and were unaffected. Closes #367. --- src/engine/hot_loop/hmds.rs | 14 +++++- src/engine/hot_loop/mod.rs | 91 ++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/src/engine/hot_loop/hmds.rs b/src/engine/hot_loop/hmds.rs index e5a24e39..6f823a59 100644 --- a/src/engine/hot_loop/hmds.rs +++ b/src/engine/hot_loop/hmds.rs @@ -75,6 +75,18 @@ impl HmdsState { } } + /// Give up on the transport. + /// + /// The flag and the connection move together. Setting the flag alone left + /// the dead socket in place, and the reconnect scheduler returns early + /// while a connection is present — so the transport was stuck for the life + /// of the process, on both the liveness timeout and the ordinary + /// receive-error path (ibx#367). + pub(crate) fn disconnect(&mut self, hmds_conn: &mut Option) { + self.disconnected = true; + *hmds_conn = None; + } + pub(crate) fn poll( &mut self, hmds_conn: &mut Option, @@ -91,7 +103,7 @@ impl HmdsState { Ok(0) => {} Err(e) => { log::error!("HMDS connection lost: {}", e); - self.disconnected = true; + self.disconnect(hmds_conn); return; } Ok(n) => { diff --git a/src/engine/hot_loop/mod.rs b/src/engine/hot_loop/mod.rs index 2b33d09c..f6ce4d7d 100644 --- a/src/engine/hot_loop/mod.rs +++ b/src/engine/hot_loop/mod.rs @@ -740,7 +740,7 @@ impl HotLoop { if warmed_up && since_recv > LIVENESS_TEST_SECS { if since_recv > LIVENESS_DEAD_SECS { log::error!("HMDS liveness timeout ({}s silent) — connection lost", since_recv); - self.hmds.disconnected = true; + self.hmds.disconnect(&mut self.hmds_conn); } else if self.hb.pending_hmds_test.is_none() { let test_id = self.hb.next_test_id(); let _ = conn.send_fix(&[ @@ -1039,6 +1039,14 @@ impl HotLoop { self.hmds.disconnected = false; self.hb.last_hmds_recv = Instant::now(); self.hb.last_hmds_sent = Instant::now(); + // The probe that went unanswered belonged to the dead session. + // Leaving it pending suppressed the next one — the liveness + // check only sends a TestRequest when none is outstanding — so + // a fresh connection went silent-to-dead without ever being + // probed. The warm-up starts here too, or the new session is + // already past it (ibx#367). + self.hb.pending_hmds_test = None; + self.hb.hmds_up_since = Instant::now(); self.hmds_reconnect_attempt = 0; self.hmds_next_attempt_at = None; self.pending_hmds_reconnect = None; @@ -1463,6 +1471,87 @@ fn extract_text_tag(msg: &[u8], tag: u32) -> Option { #[cfg(test)] mod tests { + + /// A reconnected transport must not inherit the dead session's outstanding + /// probe. The liveness check only sends a TestRequest when none is pending, + /// so a stale one suppressed every probe on the new connection and it went + /// from silent to declared dead without being asked anything. The warm-up + /// has to restart with it, or the new session is already past it. + #[test] + fn an_hmds_reconnect_does_not_inherit_the_dead_session_probe() { + let mut hl = HotLoop::new(Arc::new(SharedState::new()), None, None); + hl.hb.pending_hmds_test = Some(("t-1".to_string(), Instant::now())); + hl.hb.hmds_up_since = Instant::now() - Duration::from_secs(600); + hl.hmds.disconnected = true; + + // A reconnect that has just succeeded. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let sock = std::net::TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let (_peer, _) = listener.accept().unwrap(); + let (tx, rx) = crossbeam_channel::bounded(1); + tx.send(Ok(crate::protocol::connection::Connection::new_raw(sock).unwrap())).unwrap(); + hl.pending_hmds_reconnect = Some(rx); + + hl.poll_hmds_reconnect(); + + assert!(hl.hmds_conn.is_some(), "the connection is installed"); + assert!(!hl.hmds.disconnected, "and the transport is live again"); + assert!( + hl.hb.pending_hmds_test.is_none(), + "the dead session's probe is dropped, or it suppresses every probe on the new one", + ); + assert!( + hl.hb.hmds_up_since.elapsed() < Duration::from_secs(LIVENESS_WARMUP_SECS), + "and the new session starts inside its warm-up", + ); + } + + /// ibx#367: the liveness timeout set the disconnected flag and left the + /// dead socket in place, and the reconnect scheduler returns early while a + /// connection is present — so the first HMDS liveness timeout was permanent + /// for the life of the process. Historical requests issued afterwards + /// targeted the abandoned socket and never completed. + #[test] + fn an_hmds_disconnect_lets_its_reconnect_run() { + let mut hl = HotLoop::new(Arc::new(SharedState::new()), None, None); + hl.set_reconnect_auth(crate::gateway::ReconnectAuth { + host: "gw.example".into(), + username: "u".into(), + password: zeroize::Zeroizing::new(String::new()), + paper: true, + session_key: Default::default(), + session_token: Default::default(), + server_session_id: String::new(), + hw_info: String::new(), + encoded: String::new(), + hmds_host: "hmds.example".into(), + hmds_farm: "hfarm".into(), + }); + + // A live transport, so releasing it is observable. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let sock = std::net::TcpStream::connect(listener.local_addr().unwrap()).unwrap(); + let (_peer, _) = listener.accept().unwrap(); + hl.hmds_conn = Some(crate::protocol::connection::Connection::new_raw(sock).unwrap()); + hl.hmds.disconnected = false; + assert!(hl.hmds_next_attempt_at.is_none(), "nothing scheduled while it is up"); + + // Driven through the receive path rather than by calling the helper: + // the peer is gone, so `try_recv` errors and the poll gives up on the + // transport. This is the ordinary way HMDS dies. + drop(_peer); + hl.hmds.poll(&mut hl.hmds_conn, &SharedState::new(), &None, &mut hl.hb); + + assert!(hl.hmds.disconnected, "the transport is marked dead"); + assert!(hl.hmds_conn.is_none(), "and the socket is released with it"); + + // The scheduler now gets past its early return and arms the first attempt. + hl.maybe_spawn_hmds_reconnect(); + assert!( + hl.hmds_next_attempt_at.is_some(), + "an HMDS reconnect is scheduled rather than skipped forever", + ); + } use super::*; use std::sync::Arc; use crate::bridge::{Event, SharedState};