From 5c02695d5c8783ef0e2c3bffc3afdaa5604f925d Mon Sep 17 00:00:00 2001 From: Sumit Kumar Date: Wed, 22 Jul 2026 16:26:51 +0530 Subject: [PATCH 1/2] fix(client): hold the device-registry cleanup task's client ref weakly The background cleanup task held a strong Arc, so the client - and thus its store's open SQLite file handle - stayed alive until the task observed the shutdown signal and exited asynchronously. A consumer deleting a session then raced that teardown and could fail to remove the store file on Windows, which refuses to unlink a file with an open handle. Hold the client Weak and take the 'static shutdown future without pinning it, matching the event-delivery drainer's downgrade. --- src/client/device_registry.rs | 18 ++++++++++++------ src/client/lifecycle.rs | 9 ++++----- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 8cf648491..891ca62a1 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -961,12 +961,18 @@ impl Client { .collect() } - /// Background loop placeholder for device registry cleanup. - /// Note: Cleanup functionality was removed as part of trait simplification. - /// Device registry entries are managed through normal update/get operations. - pub(super) async fn device_registry_cleanup_loop(&self) { - // Simply wait for shutdown signal - self.shutdown_notifier.listen().await; + /// Background task placeholder: waits for the shutdown signal, then exits. + /// (Cleanup was removed with trait simplification; entries are managed via + /// normal update/get.) Holds the client Weak, not strong, so it never keeps + /// the client - and its store's file handle - alive past teardown, exactly + /// like the event-delivery drainer's `Arc::downgrade`. + pub(super) async fn device_registry_cleanup_loop(client: std::sync::Weak) { + // Grab the 'static shutdown future without holding the client across the + // wait; if the client is already gone there is nothing to wait for. + let Some(shutdown) = client.upgrade().map(|c| c.shutdown_notifier.listen()) else { + return; + }; + shutdown.await; debug!( target: "Client/DeviceRegistry", "Shutdown signaled, exiting cleanup loop" diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index a4c3190a7..85cf1662e 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -314,12 +314,11 @@ impl Client { })) .detach(); - // Start background task to clean up stale device registry entries - let cleanup_arc = arc.clone(); + // Background shutdown-awaiter; Weak so it never pins the client (and its + // store handle) past teardown, like the event drainer's downgrade. + let cleanup_weak = Arc::downgrade(&arc); arc.runtime - .spawn(Box::pin(async move { - cleanup_arc.device_registry_cleanup_loop().await; - })) + .spawn(Box::pin(Self::device_registry_cleanup_loop(cleanup_weak))) .detach(); (arc, rx) From edc350c50f0dd2d44f68b5814bb845112e503b3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= <55464917+jlucaso1@users.noreply.github.com> Date: Wed, 22 Jul 2026 09:36:36 -0300 Subject: [PATCH 2/2] fix(client): remove obsolete registry cleanup task --- src/client/device_registry.rs | 18 ------------------ src/client/lifecycle.rs | 7 ------- src/client/tests.rs | 22 ++++++++++++++++++++++ 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index fb63a1e34..dedd18e93 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -988,24 +988,6 @@ impl Client { devices } - /// Background task placeholder: waits for the shutdown signal, then exits. - /// (Cleanup was removed with trait simplification; entries are managed via - /// normal update/get.) Holds the client Weak, not strong, so it never keeps - /// the client - and its store's file handle - alive past teardown, exactly - /// like the event-delivery drainer's `Arc::downgrade`. - pub(super) async fn device_registry_cleanup_loop(client: std::sync::Weak) { - // Grab the 'static shutdown future without holding the client across the - // wait; if the client is already gone there is nothing to wait for. - let Some(shutdown) = client.upgrade().map(|c| c.shutdown_notifier.listen()) else { - return; - }; - shutdown.await; - debug!( - target: "Client/DeviceRegistry", - "Shutdown signaled, exiting cleanup loop" - ); - } - /// Migrate device registry entries from PN key to LID key. #[cfg_attr( feature = "tracing", diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index f8e011a90..7acd8efcd 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -313,13 +313,6 @@ impl Client { })) .detach(); - // Background shutdown-awaiter; Weak so it never pins the client (and its - // store handle) past teardown, like the event drainer's downgrade. - let cleanup_weak = Arc::downgrade(&arc); - arc.runtime - .spawn(Box::pin(Self::device_registry_cleanup_loop(cleanup_weak))) - .detach(); - (arc, rx) } diff --git a/src/client/tests.rs b/src/client/tests.rs index 2098a64cc..84f6b1ebf 100644 --- a/src/client/tests.rs +++ b/src/client/tests.rs @@ -3409,6 +3409,28 @@ async fn terminal_disconnect_propagates_to_per_connection_signal() { ); } +/// Dropping the last owner must release persistence handles promptly. +#[tokio::test] +async fn dropping_fresh_client_releases_it_without_shutdown() { + let client = crate::test_utils::create_test_client().await; + let weak = Arc::downgrade(&client); + + drop(client); + + tokio::time::timeout(Duration::from_secs(5), async { + while weak.strong_count() != 0 { + tokio::task::yield_now().await; + } + }) + .await + .unwrap_or_else(|_| { + panic!( + "client is still retained by a background task (strong_count={})", + weak.strong_count() + ) + }); +} + /// Locks the zero-allocation property of the ack miss path: id resolution and /// the waiter probe must borrow from the node buffer. An `into_owned()` here /// costs one String per received ack, which the e2e dhat profile caught live.