fix(daemon): tear down serial session on ws early-return (#51)

crates/fbuild-daemon/src/handlers/websockets.rs

@@ -45,6 +45,32 @@ impl Drop for PendingAttachGuard {
     }
 }
 
+/// Unwinds any serial-session state that the ws_serial_monitor handler
+/// left on the shared manager: detach reader, release writer, and close
+/// the port if there are no remaining clients. Idempotent — safe to call
+/// on a partially-set-up session. See FastLED/fbuild#51.
+async fn cleanup_ws_serial_session(
+    ctx: &Arc<DaemonContext>,
+    port: &str,
+    client_id: &str,
+    writer_acquired: bool,
+) {
+    ctx.serial_manager.detach_reader(port, client_id);
+    if writer_acquired {
+        ctx.serial_manager.release_writer(port, client_id);
+    }
+    if !ctx.serial_manager.has_clients(port) {
+        if let Err(e) = ctx.serial_manager.close_port(port, client_id).await {
+            tracing::warn!(
+                client_id,
+                port,
+                "failed to close port on last detach: {}",
+                e
+            );
+        }
+    }
+}
+
 async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc<DaemonContext>) {
     // Mark this attach as pending so the self-eviction loop won't shut the
     // daemon down while we're waiting for `open_port` to finish (USB
@@ -103,6 +129,12 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc<DaemonContext>) {
         _ => return,
     };
 
+    // From this point on, `open_port` has created state on the shared
+    // manager (session + broadcaster) that MUST be torn down on every exit
+    // path — otherwise `has_clients()` keeps returning true and the
+    // daemon's self-eviction loop never fires. See FastLED/fbuild#51 for
+    // the leak that kept fbuild-daemon resident after autoresearch ended.
+
     // Pre-acquire writer if requested
     let writer_acquired = if pre_acquire_writer {
         ctx.serial_manager
@@ -123,6 +155,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc<DaemonContext>) {
             let _ = socket
                 .send(Message::Text(serde_json::to_string(&err_msg).unwrap()))
                 .await;
+            cleanup_ws_serial_session(&ctx, &port, &client_id, writer_acquired).await;
             return;
         }
     };
@@ -138,6 +171,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc<DaemonContext>) {
         .await
         .is_err()
     {
+        cleanup_ws_serial_session(&ctx, &port, &client_id, writer_acquired).await;
         return;
     }
 
@@ -247,20 +281,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc<DaemonContext>) {
     // reader keeps the OS file handle open, blocking other tools (e.g.
     // `pyserial.Serial(...)` from the same Python process) with
     // "Access is denied" until the daemon itself shuts down.
-    ctx.serial_manager.detach_reader(&port, &client_id);
-    if writer_acquired {
-        ctx.serial_manager.release_writer(&port, &client_id);
-    }
-    if !ctx.serial_manager.has_clients(&port) {
-        if let Err(e) = ctx.serial_manager.close_port(&port, &client_id).await {
-            tracing::warn!(
-                client_id,
-                port,
-                "failed to close port on last detach: {}",
-                e
-            );
-        }
-    }
+    cleanup_ws_serial_session(&ctx, &port, &client_id, writer_acquired).await;
 }
 
 // ---------------------------------------------------------------------------

crates/fbuild-serial/src/manager.rs

@@ -329,15 +329,21 @@ impl SharedSerialManager {
     }
 
     /// Attach a reader to receive broadcast output.
+    ///
+    /// All-or-nothing: returns `None` without mutating session state if
+    /// the port has no active broadcaster, so callers that fail to attach
+    /// don't leave a dangling `reader_client_ids` entry that would block
+    /// self-eviction. See FastLED/fbuild#51.
     pub fn attach_reader(
         &self,
         port: &str,
         client_id: &str,
     ) -> Option<broadcast::Receiver<String>> {
+        let rx = self.broadcasters.get(port).map(|tx| tx.subscribe())?;
         if let Some(mut session) = self.sessions.get_mut(port) {
             session.reader_client_ids.insert(client_id.to_string());
         }
-        self.broadcasters.get(port).map(|tx| tx.subscribe())
+        Some(rx)
     }
 
     /// Detach a reader.
@@ -541,4 +547,60 @@ mod tests {
             );
         });
     }
+
+    /// Regression guard for FastLED/fbuild#51: `attach_reader` used to
+    /// insert `client_id` into `session.reader_client_ids` even when the
+    /// broadcaster was missing (returning `None`). That left a dangling
+    /// reader id that kept `has_clients()` true forever, blocking
+    /// self-eviction and leaving `fbuild-daemon.exe` resident after the
+    /// autoresearch session ended.
+    ///
+    /// Contract: if `attach_reader` returns `None`, no session state may
+    /// be mutated.
+    #[test]
+    fn attach_reader_missing_broadcaster_does_not_mutate_session_state() {
+        let mgr = SharedSerialManager::new();
+        let port = "COM_TEST_NO_BROADCASTER";
+        let client = "client-1";
+
+        // Insert a bare session without a broadcaster — simulates the
+        // pathological "half-set-up" state.
+        mgr.sessions.insert(
+            port.to_string(),
+            super::SerialSession {
+                port: port.to_string(),
+                baud_rate: 115200,
+                is_open: false,
+                writer_client_id: None,
+                reader_client_ids: Default::default(),
+                output_buffer: Default::default(),
+                total_bytes_read: 0,
+                total_bytes_written: 0,
+                started_at: 0.0,
+                owner_client_id: None,
+                elf_path: None,
+                serial_handle: None,
+                reader_handle: None,
+                stop_flag: Arc::new(std::sync::atomic::AtomicBool::new(false)),
+            },
+        );
+
+        let result = mgr.attach_reader(port, client);
+        assert!(
+            result.is_none(),
+            "attach_reader must return None when broadcaster is absent"
+        );
+
+        let leaked = mgr
+            .sessions
+            .get(port)
+            .map(|s| s.reader_client_ids.contains(client))
+            .unwrap_or(false);
+        assert!(
+            !leaked,
+            "attach_reader must not mutate reader_client_ids when it \
+             returns None — regression of FastLED/fbuild#51 where the \
+             leaked id kept has_clients() true forever"
+        );
+    }
 }