fix(rs-dapi-client,rs-sdk): fix 4 health check review findings

FIX 1: Break out of health_check_loop when cancellation is consumed
inside probe_and_update_batch. The FusedFuture becomes terminated,
causing the outer select_biased! to skip the stop branch forever.

FIX 2: Only store the new CancellationToken into the mutex after
confirming the tokio runtime exists and the task will be spawned.
Previously, is_health_check_running() returned true in FFI paths
where no task was actually running.

FIX 3: Don't re-ban already-banned nodes during health check
re-probes. Each ban() call increments ban_count with exponential
backoff; deterministic re-probing drove ban duration to absurd
levels (112 days after 12 cycles).

FIX 4: Hold the mutex for the entire start_health_check operation
to prevent concurrent calls from orphaning tasks. The previous
implementation released and re-acquired the mutex, allowing
interleaving.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: lklimek

packages/rs-dapi-client/src/health_check.rs

@@ -130,6 +130,14 @@ async fn health_check_loop(
                 &mut stop,
             )
             .await;
+
+            // FIX 1: If cancellation was consumed inside probe_and_update_batch,
+            // the fused future is now terminated and the outer select_biased!
+            // would skip it, making the loop unkillable. Break explicitly.
+            if stop.is_terminated() {
+                tracing::debug!("health check cancelled during probing");
+                break;
+            }
         }
 
         let sleep_duration = match address_list.get_next_ban_expiry() {
@@ -186,7 +194,14 @@ async fn probe_and_update_batch(
                         }
                     }
                     Err(error) => {
-                        address_list.ban(address);
+                        // Only ban on first failure; don't escalate already-banned nodes.
+                        // The health check re-probes on every cycle, and each ban() call
+                        // increments ban_count with exponential backoff. Without this guard,
+                        // deterministic re-probing drives ban duration to absurd levels
+                        // (e.g. 112 days after 12 cycles).
+                        if !address_list.is_banned(address) {
+                            address_list.ban(address);
+                        }
                         tracing::debug!(%address, error = %error, "health check: node is unhealthy, banned");
                     }
                 }
@@ -399,6 +414,47 @@ mod tests {
         );
     }
 
+    /// FIX-1: Verify that cancellation during active probing (not just during sleep)
+    /// actually stops the loop. Before the fix, FusedFuture termination inside
+    /// probe_and_update_batch made the outer loop skip the stop branch, running forever.
+    #[tokio::test]
+    async fn test_cancel_during_active_probing_stops_loop() {
+        let mut address_list = AddressList::new();
+        // Use multiple slow-to-probe addresses so probing takes a while
+        for port in 1..=5 {
+            let addr: Address = format!("http://192.0.2.1:{}", port).parse().unwrap();
+            address_list.add(addr);
+        }
+
+        let pool = ConnectionPool::new(10);
+        let config = HealthCheckConfig {
+            // Long timeout so probing is still in-flight when we cancel
+            probe_timeout: Duration::from_secs(10),
+            max_concurrent_probes: 1,
+            no_ban_idle_period: Duration::from_secs(300),
+        };
+        let cancel_token = CancellationToken::new();
+
+        let ct = cancel_token.clone();
+        let al = address_list.clone();
+
+        let handle = tokio::spawn(async move {
+            run_health_check(al, pool, config, cancel_token, None).await;
+        });
+
+        // Cancel while probing is in-flight (not during the sleep phase)
+        tokio::time::sleep(Duration::from_millis(100)).await;
+        ct.cancel();
+
+        // If the fix is missing, the loop becomes a zombie and this will timeout
+        let result = tokio::time::timeout(Duration::from_secs(5), handle).await;
+        assert!(
+            result.is_ok(),
+            "health check must stop within 5s when cancelled during active probing"
+        );
+        result.unwrap().unwrap();
+    }
+
     /// QA-004: Verify cancel_token stops the loop even when addresses have active bans
     /// (i.e., when sleeping until ban expiry).
     #[tokio::test]

packages/rs-sdk/src/sdk.rs

@@ -463,8 +463,17 @@ impl Sdk {
     /// If a health check is already running, it is stopped first.
     /// All `Sdk` clones share the same state, so starting from any clone
     /// starts it for all of them.
+    ///
+    /// The mutex is held for the entire operation to prevent concurrent calls
+    /// from orphaning tasks.
     pub fn start_health_check(&self, config: rs_dapi_client::HealthCheckConfig) {
-        self.stop_health_check();
+        let mut cancel_guard = self
+            .health_check_cancel
+            .lock()
+            .expect("health_check_cancel mutex poisoned");
+
+        // Stop existing health check (atomic: no mutex release between stop and start)
+        cancel_guard.cancel();
 
         let new_cancel = self.cancel_token.child_token();
 
@@ -478,14 +487,11 @@ impl Sdk {
             #[cfg(target_arch = "wasm32")]
             let hc_ca_cert: Option<()> = None;
 
-            *self
-                .health_check_cancel
-                .lock()
-                .expect("health_check_cancel mutex poisoned") = new_cancel;
-
             #[cfg(not(target_arch = "wasm32"))]
             {
                 if let Ok(runtime) = tokio::runtime::Handle::try_current() {
+                    *cancel_guard = new_cancel;
+                    drop(cancel_guard);
                     drop(
                         runtime.spawn(rs_dapi_client::health_check::run_health_check(
                             hc_address_list,
@@ -496,12 +502,17 @@ impl Sdk {
                         )),
                     );
                 } else {
+                    // No runtime: don't store the token so is_health_check_running()
+                    // correctly returns false.
+                    drop(cancel_guard);
                     tracing::warn!("no Tokio runtime found, cannot start health check");
                 }
             }
 
             #[cfg(target_arch = "wasm32")]
             {
+                *cancel_guard = new_cancel;
+                drop(cancel_guard);
                 wasm_bindgen_futures::spawn_local(rs_dapi_client::health_check::run_health_check(
                     hc_address_list,
                     hc_pool,