diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index 8bcf1273..2caa4bca 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/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, + 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) { // 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) { _ => 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) { 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) { .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) { // 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; } // --------------------------------------------------------------------------- diff --git a/crates/fbuild-serial/src/manager.rs b/crates/fbuild-serial/src/manager.rs index 998180aa..b739998b 100644 --- a/crates/fbuild-serial/src/manager.rs +++ b/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> { + 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" + ); + } }