fix: awaited release on connect-inventory failures; kill grace outlasts teardown

cli/src/daemon.rs

@@ -934,13 +934,16 @@ pub async fn kill_lingering_helpers() -> usize {
     for pid in &pids {
         signal_pid(*pid, false);
     }
-    // Wait for graceful exits before escalating. SIGTERM now routes daemons
-    // through shutdown(), which awaits the remote-session release (bounded at
-    // ~5s in the core) — so the grace window must outlast that bound, or the
-    // SIGKILL would land mid-release and the session would run out its full
-    // server-side timeout. Healthy daemons exit in well under a second, so
-    // the poll usually ends on its first iterations.
-    const KILL_GRACE: Duration = Duration::from_secs(6);
+    // Wait for graceful exits before escalating. SIGTERM routes daemons
+    // through browser teardown, whose worst-case chain is bounded in the
+    // core: owned-tab close (≤5s, local browsers only) + awaited remote
+    // release (≤5s) + the connect-settle wait for a mid-flight connect
+    // attempt (≤8s) ≈ 18s. The grace must outlast that whole chain — a
+    // SIGKILL landing inside it recreates the timed-out-session leak this
+    // teardown exists to prevent. Healthy daemons exit in well under a
+    // second, so the poll usually ends on its first iterations; the full
+    // wait is only ever paid for genuinely wedged processes.
+    const KILL_GRACE: Duration = Duration::from_secs(20);
     const KILL_POLL: Duration = Duration::from_millis(200);
     let deadline = Instant::now() + KILL_GRACE;
     while Instant::now() < deadline {

core/src/cdp/lifecycle.rs

@@ -129,9 +129,15 @@ impl Cdp {
         // observes the swap at its next checkpoint and releases what it
         // acquired (awaited, see `try_connect_once`); waiting on the connect
         // lock keeps quit-path callers alive long enough for that release to
-        // land. Bounded: deep in a stalled handshake the next checkpoint can
-        // be ~INVENTORY_TIMEOUT away, and a quit must not hang that long —
-        // the server-side session timeout backstops that residual window.
+        // land. Bounded, which leaves two accepted residuals when the bound
+        // expires with the attempt still running — a handshake stalled up to
+        // ~INVENTORY_TIMEOUT before its next checkpoint, or the attempt's own
+        // bounded release still in the air. In both, process exit cancels the
+        // attempt, its still-armed owner falls back to the Drop-spawned
+        // release (dead on exit), and the server-side session timeout is the
+        // backstop. Closing them would need release ownership to outlive the
+        // connect task (a parked-owner handoff) — machinery disproportionate
+        // to a seconds-wide window behind that backstop.
         let _ = tokio::time::timeout(CONNECT_SETTLE_TIMEOUT, self.connect_lock().lock()).await;
     }
 }
@@ -147,15 +153,24 @@ async fn release_owner_now(owner: BrowserOwner) {
         // `Local` drops here, killing the managed chrome as before.
         return;
     };
-    let Some(session_id) = session.take_session_id() else {
+    if session.session_id.is_empty() {
         return;
-    };
-    match tokio::time::timeout(
+    }
+    // Clone the id instead of taking it up front: some callers run inside
+    // the connect attempt's budget timeout, and a take-first release
+    // cancelled mid-flight would drop a *disarmed* `RemoteSession` — losing
+    // even the Drop-spawned backstop. With the id left armed, cancellation
+    // falls back to `Drop`; the disarm below runs only once the await has
+    // actually completed (success or definitive failure, where the
+    // server-side session timeout takes over).
+    let session_id = session.session_id.clone();
+    let outcome = tokio::time::timeout(
         RELEASE_AWAIT_TIMEOUT,
         crate::cloud::release_browser_session(&session_id),
     )
-    .await
-    {
+    .await;
+    let _ = session.take_session_id();
+    match outcome {
         Ok(Ok(())) => debug!(session_id, "remote browser session released"),
         Ok(Err(err)) => warn!(
             session_id,
@@ -253,19 +268,30 @@ async fn try_connect_once(
 ) -> anyhow::Result<ConnectAttempt> {
     let OpenEndpoint { endpoint, owner } = open_endpoint(options).await?;
 
-    let inventory =
-        match tokio::time::timeout(INVENTORY_TIMEOUT, connect_inventory(&endpoint)).await {
-            Ok(result) => result.map_err(|err| redact_endpoint_in_error(&endpoint, err))?,
-            // Dropping `owner` on the way out releases a hosted session that was
-            // minted for a browser websocket which never answered.
-            Err(_) => {
-                anyhow::bail!(
-                    "browser websocket {} did not respond within {}s",
-                    endpoint.display_ws_url(),
-                    INVENTORY_TIMEOUT.as_secs()
-                )
-            }
-        };
+    let inventory = match tokio::time::timeout(INVENTORY_TIMEOUT, connect_inventory(&endpoint))
+        .await
+    {
+        Ok(Ok(inventory)) => inventory,
+        // Both error exits release a just-minted hosted session awaited
+        // before surfacing the error. A Drop-spawned release could still be
+        // in flight when `run_connect` gives up and frees the connect lock —
+        // at which point a shutdown waiting in `disconnect()` proceeds to
+        // process exit and aborts it. Awaiting preserves the invariant that
+        // a free connect lock means no release from this attempt is still
+        // in the air.
+        Ok(Err(err)) => {
+            release_owner_now(owner).await;
+            return Err(redact_endpoint_in_error(&endpoint, err));
+        }
+        Err(_) => {
+            release_owner_now(owner).await;
+            anyhow::bail!(
+                "browser websocket {} did not respond within {}s",
+                endpoint.display_ws_url(),
+                INVENTORY_TIMEOUT.as_secs()
+            )
+        }
+    };
     let monitor_task = spawn_target_poll_loop(cdp.clone(), inventory.browser_client.clone());
 
     {