From 352e7d986481bae970afd7b194faaa8fd78d0c05 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Tue, 29 Sep 2026 23:32:58 -0700 Subject: [PATCH 1/6] fix(serial): name the port holder instead of blaming the driver on open timeout (#1429) --- .../src/handlers/operations/monitor.rs | 14 ++- .../fbuild-daemon/src/handlers/websockets.rs | 17 ++- crates/fbuild-serial/src/lib.rs | 1 + crates/fbuild-serial/src/port_holders.rs | 114 ++++++++++++++++++ 4 files changed, 139 insertions(+), 7 deletions(-) create mode 100644 crates/fbuild-serial/src/port_holders.rs diff --git a/crates/fbuild-daemon/src/handlers/operations/monitor.rs b/crates/fbuild-daemon/src/handlers/operations/monitor.rs index c58a6f42..3c60735c 100644 --- a/crates/fbuild-daemon/src/handlers/operations/monitor.rs +++ b/crates/fbuild-daemon/src/handlers/operations/monitor.rs @@ -282,10 +282,16 @@ pub async fn monitor( StatusCode::GATEWAY_TIMEOUT, Json(OperationResponse::fail( request_id, - format!( - "open_port({}) exceeded {}s — serial driver may be wedged", - port, SERIAL_OPEN_PORT_TIMEOUT_SECS - ), + match fbuild_serial::port_holders::describe_port_holders(&port) { + Some(holder) => format!( + "open_port({}) exceeded {}s — {} (EBUSY contention, not a driver wedge)", + port, SERIAL_OPEN_PORT_TIMEOUT_SECS, holder + ), + None => format!( + "open_port({}) exceeded {}s — serial driver may be wedged", + port, SERIAL_OPEN_PORT_TIMEOUT_SECS + ), + }, )), ); } diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index 7d5b9d7d..a6ad856c 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -136,6 +136,18 @@ async fn active_preemption_error(ctx: &DaemonContext, port: &str) -> Option String { + match fbuild_serial::port_holders::describe_port_holders(port) { + Some(holder) => format!( + "open_port({port}) exceeded {deadline}; {holder} (EBUSY contention, not a driver wedge)" + ), + None => format!("open_port({port}) exceeded {deadline}; serial driver may be wedged"), + } +} + async fn await_ws_serial_open_port( port: &str, open_future: F, @@ -147,10 +159,9 @@ where match tokio::time::timeout(timeout, open_future).await { Ok(Ok(())) => Ok(()), Ok(Err(e)) => Err(format!("failed to open port: {}", e)), - Err(_) => Err(format!( - "open_port({}) exceeded {}; serial driver may be wedged", + Err(_) => Err(open_port_timeout_message( port, - format_timeout_for_error(timeout) + &format_timeout_for_error(timeout), )), } } diff --git a/crates/fbuild-serial/src/lib.rs b/crates/fbuild-serial/src/lib.rs index 45185c35..501308b4 100644 --- a/crates/fbuild-serial/src/lib.rs +++ b/crates/fbuild-serial/src/lib.rs @@ -38,6 +38,7 @@ pub mod esp_reset; pub mod manager; pub mod messages; pub mod port_class; +pub mod port_holders; pub mod port_selector; pub mod ports; pub mod preemption; diff --git a/crates/fbuild-serial/src/port_holders.rs b/crates/fbuild-serial/src/port_holders.rs new file mode 100644 index 00000000..0d5a036e --- /dev/null +++ b/crates/fbuild-serial/src/port_holders.rs @@ -0,0 +1,114 @@ +//! Identify which process holds a serial port open (FastLED/fbuild#1429). +//! +//! An `open()` that fails with `EBUSY` was previously reported as +//! "serial driver may be wedged", which sent investigations to the device +//! and USB layer when the port was simply held by another fd (often the +//! daemon itself). This scans `/proc/*/fd` so the error can name the holder. +//! On hosts without `/proc` the scan finds nothing and callers fall back to +//! the generic message. + +use std::path::{Path, PathBuf}; + +/// A process that has the port's device node open. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PortHolder { + pub pid: u32, + pub name: String, +} + +/// Scan a `/proc`-shaped tree rooted at `proc_root` for processes holding +/// `port` open. Split out so tests can use a synthetic tree. +pub fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { + let target = std::fs::canonicalize(port).unwrap_or_else(|_| PathBuf::from(port)); + let mut holders = Vec::new(); + let Ok(entries) = std::fs::read_dir(proc_root) else { + return holders; + }; + for entry in entries.flatten() { + let Some(pid) = entry + .file_name() + .to_str() + .and_then(|s| s.parse::().ok()) + else { + continue; + }; + let Ok(fds) = std::fs::read_dir(entry.path().join("fd")) else { + continue; + }; + let holds = fds + .flatten() + .filter_map(|fd| std::fs::read_link(fd.path()).ok()) + .any(|link| link == target || link == Path::new(port)); + if holds { + let name = std::fs::read_to_string(entry.path().join("comm")) + .map(|s| s.trim().to_string()) + .unwrap_or_else(|_| "unknown".to_string()); + holders.push(PortHolder { pid, name }); + } + } + holders.sort_by_key(|h| h.pid); + holders +} + +/// Processes currently holding `port` open (Linux `/proc`; empty elsewhere). +pub fn find_port_holders(port: &str) -> Vec { + find_port_holders_in(Path::new("/proc"), port) +} + +/// Human-readable contention hint, e.g. `port held by fbuild-daemon (pid 42)`. +/// `None` when no holder is visible. +pub fn describe_port_holders(port: &str) -> Option { + let holders = find_port_holders(port); + if holders.is_empty() { + return None; + } + let list = holders + .iter() + .map(|h| format!("{} (pid {})", h.name, h.pid)) + .collect::>() + .join(", "); + Some(format!("port held by {list}")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(unix)] + #[test] + fn finds_holder_in_synthetic_proc_tree() { + let dir = tempfile::TempDir::new().unwrap(); + let node = dir.path().join("ttyFAKE"); + std::fs::write(&node, b"").unwrap(); + let node = std::fs::canonicalize(&node).unwrap(); + + let proc_root = dir.path().join("proc"); + for (pid, comm, holds) in [(10, "other", false), (42, "fbuild-daemon", true)] { + let fd_dir = proc_root.join(pid.to_string()).join("fd"); + std::fs::create_dir_all(&fd_dir).unwrap(); + std::fs::write( + proc_root.join(pid.to_string()).join("comm"), + format!("{comm}\n"), + ) + .unwrap(); + if holds { + std::os::unix::fs::symlink(&node, fd_dir.join("3")).unwrap(); + } + } + + let holders = find_port_holders_in(&proc_root, node.to_str().unwrap()); + assert_eq!( + holders, + vec![PortHolder { + pid: 42, + name: "fbuild-daemon".to_string() + }] + ); + } + + #[test] + fn missing_proc_root_yields_no_holders() { + let dir = tempfile::TempDir::new().unwrap(); + assert!(find_port_holders_in(&dir.path().join("nope"), "/dev/ttyX").is_empty()); + } +} From 282919797237ad46197aa03d364ab0b3c2ff43cf Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 30 Sep 2026 00:16:49 -0700 Subject: [PATCH 2/6] fix(serial): reopen instead of reusing a session whose reader exited (#1429) --- crates/fbuild-serial/src/manager.rs | 23 ++++++++++++++++++ crates/fbuild-serial/src/manager/tests.rs | 29 +++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/crates/fbuild-serial/src/manager.rs b/crates/fbuild-serial/src/manager.rs index cca6aa2a..431cf123 100644 --- a/crates/fbuild-serial/src/manager.rs +++ b/crates/fbuild-serial/src/manager.rs @@ -116,6 +116,17 @@ impl SharedSerialManager { client_metadata: Option, ) -> fbuild_core::Result<()> { let session_key = self.resolve_port_key(port); + // A reader that hit a read error (native-USB chips re-enumerate on + // DTR/RTS resets) exits but leaves the session "open" with the OS fd + // still held, so later opens reuse a dead session and other openers + // get EBUSY (FastLED/fbuild#1429). Tear it down and reopen. + if self.session_reader_exited(&session_key) { + tracing::warn!( + port, + "serial reader exited but session still open; closing stale session before reopening" + ); + self.close_port(port, client_id).await?; + } // Existing-session reuse is serialized with deploy/reset acquisition // for the same reason as new-session publication below: an attach // must not become visible after a preemption event was emitted. @@ -1302,6 +1313,18 @@ impl SharedSerialManager { }) } + /// True when `session_key` has an open session whose background reader + /// has already finished. + fn session_reader_exited(&self, session_key: &str) -> bool { + self.sessions.get(session_key).is_some_and(|session| { + session.is_open + && session + .reader_handle + .as_ref() + .is_some_and(|reader| reader.is_finished()) + }) + } + fn resolve_port_key(&self, port: &str) -> String { self.port_aliases .get(port) diff --git a/crates/fbuild-serial/src/manager/tests.rs b/crates/fbuild-serial/src/manager/tests.rs index 0c6722f4..9f6b7a3c 100644 --- a/crates/fbuild-serial/src/manager/tests.rs +++ b/crates/fbuild-serial/src/manager/tests.rs @@ -866,3 +866,32 @@ async fn concurrent_deploy_preemption_rejects_second_request_and_preserves_first manager.complete_deploy_preemption("COM1", "COM1").await; assert!(!manager.is_preempted("COM1").await); } + +/// FastLED/fbuild#1429: a session whose reader thread has exited must not be +/// reused (it still holds the OS fd); `open_port` closes it first. +#[tokio::test] +async fn open_port_closes_session_whose_reader_exited() { + let mgr = SharedSerialManager::new(); + let port = "COM_DEAD_READER"; + let reader = tokio::spawn(async {}); + while !reader.is_finished() { + tokio::task::yield_now().await; + } + let mut session = super::SerialSession::new(port.to_string(), 115200); + session.is_open = true; + session.reader_handle = Some(reader); + mgr.sessions.insert(port.to_string(), session); + assert!(mgr.session_reader_exited(port)); + + // The reopen itself cannot succeed (no such port); only the teardown of + // the stale session is under test, so bound the retry loop. + let _ = tokio::time::timeout( + std::time::Duration::from_millis(200), + mgr.open_port(port, 115200, "client", None, None), + ) + .await; + assert!( + !mgr.sessions.contains_key(port), + "stale session must be removed instead of reused" + ); +} From b7fdf880c82545b43ca6371dbc477755bf128a6f Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 30 Sep 2026 00:27:53 -0700 Subject: [PATCH 3/6] fix(daemon): always release the ws writer on cleanup so idle close can run (#1429) --- .../fbuild-daemon/src/handlers/websockets.rs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index a6ad856c..d74855d2 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -92,16 +92,12 @@ impl Drop for PendingAttachGuard { /// 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, -) { +async fn cleanup_ws_serial_session(ctx: &Arc, port: &str, client_id: &str) { ctx.serial_manager.detach_reader(port, client_id); - if writer_acquired { - ctx.serial_manager.release_writer(port, client_id); - } + // Unconditional: the recovery loop re-acquires the writer without + // recording it, and `release_writer` only clears a writer this client + // holds. A leaked writer blocks the idle close of the port (#1429). + ctx.serial_manager.release_writer(port, client_id); if !ctx.serial_manager.has_clients(port) { ctx.serial_manager .close_port_after_grace_if_idle(port, client_id, Duration::from_secs(2)); @@ -291,7 +287,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { let _ = socket .send(Message::Text(serialize_or_fallback(&err_msg))) .await; - cleanup_ws_serial_session(&ctx, &port, &client_id, writer_acquired).await; + cleanup_ws_serial_session(&ctx, &port, &client_id).await; return; } }; @@ -307,7 +303,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; + cleanup_ws_serial_session(&ctx, &port, &client_id).await; return; } drop(attach_guard); @@ -846,7 +842,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. - cleanup_ws_serial_session(&ctx, &port, &client_id, writer_acquired).await; + cleanup_ws_serial_session(&ctx, &port, &client_id).await; } // --------------------------------------------------------------------------- From f6edfdb62ea0d963c5b51bda8e5abe1b673241dd Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 30 Sep 2026 01:23:16 -0700 Subject: [PATCH 4/6] refactor(serial): make port-holder matching pure so it needs no cfg or symlink (#1429) --- crates/fbuild-serial/src/port_holders.rs | 98 ++++++++++++------------ 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/crates/fbuild-serial/src/port_holders.rs b/crates/fbuild-serial/src/port_holders.rs index 0d5a036e..861c3c44 100644 --- a/crates/fbuild-serial/src/port_holders.rs +++ b/crates/fbuild-serial/src/port_holders.rs @@ -16,38 +16,51 @@ pub struct PortHolder { pub name: String, } -/// Scan a `/proc`-shaped tree rooted at `proc_root` for processes holding -/// `port` open. Split out so tests can use a synthetic tree. -pub fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { +/// Pure matcher: `procs` yields `(pid, name, fd link targets)`; returns the +/// processes with an fd pointing at `port` (or its canonical path). +pub fn match_port_holders( + procs: impl IntoIterator)>, + port: &str, +) -> Vec { let target = std::fs::canonicalize(port).unwrap_or_else(|_| PathBuf::from(port)); - let mut holders = Vec::new(); + let mut holders: Vec = procs + .into_iter() + .filter(|(_, _, links)| { + links + .iter() + .any(|link| *link == target || link == Path::new(port)) + }) + .map(|(pid, name, _)| PortHolder { pid, name }) + .collect(); + holders.sort_by_key(|h| h.pid); + holders +} + +/// Read a `/proc`-shaped tree rooted at `proc_root` into `(pid, comm, fds)`. +fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { let Ok(entries) = std::fs::read_dir(proc_root) else { - return holders; + return Vec::new(); }; - for entry in entries.flatten() { - let Some(pid) = entry - .file_name() - .to_str() - .and_then(|s| s.parse::().ok()) - else { - continue; - }; - let Ok(fds) = std::fs::read_dir(entry.path().join("fd")) else { - continue; - }; - let holds = fds - .flatten() - .filter_map(|fd| std::fs::read_link(fd.path()).ok()) - .any(|link| link == target || link == Path::new(port)); - if holds { + entries + .flatten() + .filter_map(|entry| { + let pid = entry.file_name().to_str()?.parse::().ok()?; + let fds = std::fs::read_dir(entry.path().join("fd")).ok()?; + let links = fds + .flatten() + .filter_map(|fd| std::fs::read_link(fd.path()).ok()) + .collect(); let name = std::fs::read_to_string(entry.path().join("comm")) .map(|s| s.trim().to_string()) .unwrap_or_else(|_| "unknown".to_string()); - holders.push(PortHolder { pid, name }); - } - } - holders.sort_by_key(|h| h.pid); - holders + Some((pid, name, links)) + }) + .collect() +} + +/// Scan a `/proc`-shaped tree for processes holding `port` open. +pub fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { + match_port_holders(read_proc_tree(proc_root), port) } /// Processes currently holding `port` open (Linux `/proc`; empty elsewhere). @@ -74,31 +87,18 @@ pub fn describe_port_holders(port: &str) -> Option { mod tests { use super::*; - #[cfg(unix)] #[test] - fn finds_holder_in_synthetic_proc_tree() { - let dir = tempfile::TempDir::new().unwrap(); - let node = dir.path().join("ttyFAKE"); - std::fs::write(&node, b"").unwrap(); - let node = std::fs::canonicalize(&node).unwrap(); - - let proc_root = dir.path().join("proc"); - for (pid, comm, holds) in [(10, "other", false), (42, "fbuild-daemon", true)] { - let fd_dir = proc_root.join(pid.to_string()).join("fd"); - std::fs::create_dir_all(&fd_dir).unwrap(); - std::fs::write( - proc_root.join(pid.to_string()).join("comm"), - format!("{comm}\n"), - ) - .unwrap(); - if holds { - std::os::unix::fs::symlink(&node, fd_dir.join("3")).unwrap(); - } - } - - let holders = find_port_holders_in(&proc_root, node.to_str().unwrap()); + fn matches_only_processes_with_an_fd_on_the_port() { + let procs = vec![ + (10, "other".to_string(), vec![PathBuf::from("/dev/null")]), + ( + 42, + "fbuild-daemon".to_string(), + vec![PathBuf::from("/dev/ttyFAKE")], + ), + ]; assert_eq!( - holders, + match_port_holders(procs, "/dev/ttyFAKE"), vec![PortHolder { pid: 42, name: "fbuild-daemon".to_string() From 8a1e3256e5f98acf1f88f1be2cd2416f23fe059e Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 30 Sep 2026 02:03:17 -0700 Subject: [PATCH 5/6] fix(serial): use NormalizedPath and async canonicalize in port_holders to satisfy dylint (#1429) --- .../src/handlers/operations/monitor.rs | 2 +- .../fbuild-daemon/src/handlers/websockets.rs | 9 +-- crates/fbuild-serial/src/port_holders.rs | 62 ++++++++++++------- 3 files changed, 43 insertions(+), 30 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/operations/monitor.rs b/crates/fbuild-daemon/src/handlers/operations/monitor.rs index 3c60735c..818f0d58 100644 --- a/crates/fbuild-daemon/src/handlers/operations/monitor.rs +++ b/crates/fbuild-daemon/src/handlers/operations/monitor.rs @@ -282,7 +282,7 @@ pub async fn monitor( StatusCode::GATEWAY_TIMEOUT, Json(OperationResponse::fail( request_id, - match fbuild_serial::port_holders::describe_port_holders(&port) { + match fbuild_serial::port_holders::describe_port_holders(&port).await { Some(holder) => format!( "open_port({}) exceeded {}s — {} (EBUSY contention, not a driver wedge)", port, SERIAL_OPEN_PORT_TIMEOUT_SECS, holder diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index d74855d2..6c07d098 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -135,8 +135,8 @@ async fn active_preemption_error(ctx: &DaemonContext, port: &str) -> Option String { - match fbuild_serial::port_holders::describe_port_holders(port) { +async fn open_port_timeout_message(port: &str, deadline: &str) -> String { + match fbuild_serial::port_holders::describe_port_holders(port).await { Some(holder) => format!( "open_port({port}) exceeded {deadline}; {holder} (EBUSY contention, not a driver wedge)" ), @@ -155,10 +155,7 @@ where match tokio::time::timeout(timeout, open_future).await { Ok(Ok(())) => Ok(()), Ok(Err(e)) => Err(format!("failed to open port: {}", e)), - Err(_) => Err(open_port_timeout_message( - port, - &format_timeout_for_error(timeout), - )), + Err(_) => Err(open_port_timeout_message(port, &format_timeout_for_error(timeout)).await), } } diff --git a/crates/fbuild-serial/src/port_holders.rs b/crates/fbuild-serial/src/port_holders.rs index 861c3c44..90f7899f 100644 --- a/crates/fbuild-serial/src/port_holders.rs +++ b/crates/fbuild-serial/src/port_holders.rs @@ -7,7 +7,9 @@ //! On hosts without `/proc` the scan finds nothing and callers fall back to //! the generic message. -use std::path::{Path, PathBuf}; +use std::path::Path; + +use fbuild_core::path::{NormalizedPath, canonicalize_existing}; /// A process that has the port's device node open. #[derive(Debug, Clone, PartialEq, Eq)] @@ -17,19 +19,16 @@ pub struct PortHolder { } /// Pure matcher: `procs` yields `(pid, name, fd link targets)`; returns the -/// processes with an fd pointing at `port` (or its canonical path). +/// processes with an fd pointing at `port` or at its resolved `canonical` +/// path. pub fn match_port_holders( - procs: impl IntoIterator)>, - port: &str, + procs: impl IntoIterator)>, + port: &NormalizedPath, + canonical: &NormalizedPath, ) -> Vec { - let target = std::fs::canonicalize(port).unwrap_or_else(|_| PathBuf::from(port)); let mut holders: Vec = procs .into_iter() - .filter(|(_, _, links)| { - links - .iter() - .any(|link| *link == target || link == Path::new(port)) - }) + .filter(|(_, _, links)| links.iter().any(|link| link == canonical || link == port)) .map(|(pid, name, _)| PortHolder { pid, name }) .collect(); holders.sort_by_key(|h| h.pid); @@ -37,7 +36,7 @@ pub fn match_port_holders( } /// Read a `/proc`-shaped tree rooted at `proc_root` into `(pid, comm, fds)`. -fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { +fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { let Ok(entries) = std::fs::read_dir(proc_root) else { return Vec::new(); }; @@ -49,6 +48,7 @@ fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { let links = fds .flatten() .filter_map(|fd| std::fs::read_link(fd.path()).ok()) + .map(NormalizedPath::from) .collect(); let name = std::fs::read_to_string(entry.path().join("comm")) .map(|s| s.trim().to_string()) @@ -59,19 +59,23 @@ fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { } /// Scan a `/proc`-shaped tree for processes holding `port` open. -pub fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { - match_port_holders(read_proc_tree(proc_root), port) +pub async fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { + let port_path = NormalizedPath::new(port); + let canonical = canonicalize_existing(port) + .await + .unwrap_or_else(|_| port_path.clone()); + match_port_holders(read_proc_tree(proc_root), &port_path, &canonical) } /// Processes currently holding `port` open (Linux `/proc`; empty elsewhere). -pub fn find_port_holders(port: &str) -> Vec { - find_port_holders_in(Path::new("/proc"), port) +pub async fn find_port_holders(port: &str) -> Vec { + find_port_holders_in(Path::new("/proc"), port).await } /// Human-readable contention hint, e.g. `port held by fbuild-daemon (pid 42)`. /// `None` when no holder is visible. -pub fn describe_port_holders(port: &str) -> Option { - let holders = find_port_holders(port); +pub async fn describe_port_holders(port: &str) -> Option { + let holders = find_port_holders(port).await; if holders.is_empty() { return None; } @@ -90,15 +94,23 @@ mod tests { #[test] fn matches_only_processes_with_an_fd_on_the_port() { let procs = vec![ - (10, "other".to_string(), vec![PathBuf::from("/dev/null")]), + ( + 10, + "other".to_string(), + vec![NormalizedPath::new("/dev/null")], + ), ( 42, "fbuild-daemon".to_string(), - vec![PathBuf::from("/dev/ttyFAKE")], + vec![NormalizedPath::new("/dev/ttyFAKE")], ), ]; assert_eq!( - match_port_holders(procs, "/dev/ttyFAKE"), + match_port_holders( + procs, + &NormalizedPath::new("/dev/ttyFAKE"), + &NormalizedPath::new("/dev/ttyFAKE") + ), vec![PortHolder { pid: 42, name: "fbuild-daemon".to_string() @@ -106,9 +118,13 @@ mod tests { ); } - #[test] - fn missing_proc_root_yields_no_holders() { + #[tokio::test] + async fn missing_proc_root_yields_no_holders() { let dir = tempfile::TempDir::new().unwrap(); - assert!(find_port_holders_in(&dir.path().join("nope"), "/dev/ttyX").is_empty()); + assert!( + find_port_holders_in(&dir.path().join("nope"), "/dev/ttyX") + .await + .is_empty() + ); } } From 251e7ee874770f44a325e3f3e095f60cc452b88d Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Wed, 30 Sep 2026 05:01:19 -0700 Subject: [PATCH 6/6] =?UTF-8?q?fix(serial):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20keep=20alias=20on=20stale=20reopen,=20bound=20the=20/proc=20?= =?UTF-8?q?scan,=20don't=20claim=20EBUSY=20(#1429)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/handlers/operations/monitor.rs | 2 +- .../fbuild-daemon/src/handlers/websockets.rs | 6 ++-- crates/fbuild-serial/src/manager.rs | 13 ++++++++ crates/fbuild-serial/src/manager/tests.rs | 31 +++++++++++++++++++ crates/fbuild-serial/src/port_holders.rs | 29 ++++++++++++----- 5 files changed, 69 insertions(+), 12 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/operations/monitor.rs b/crates/fbuild-daemon/src/handlers/operations/monitor.rs index 818f0d58..fa4a3a37 100644 --- a/crates/fbuild-daemon/src/handlers/operations/monitor.rs +++ b/crates/fbuild-daemon/src/handlers/operations/monitor.rs @@ -284,7 +284,7 @@ pub async fn monitor( request_id, match fbuild_serial::port_holders::describe_port_holders(&port).await { Some(holder) => format!( - "open_port({}) exceeded {}s — {} (EBUSY contention, not a driver wedge)", + "open_port({}) exceeded {}s — {} (timeout cause unknown)", port, SERIAL_OPEN_PORT_TIMEOUT_SECS, holder ), None => format!( diff --git a/crates/fbuild-daemon/src/handlers/websockets.rs b/crates/fbuild-daemon/src/handlers/websockets.rs index 6c07d098..9e35d7f5 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -137,9 +137,9 @@ async fn active_preemption_error(ctx: &DaemonContext, port: &str) -> Option String { match fbuild_serial::port_holders::describe_port_holders(port).await { - Some(holder) => format!( - "open_port({port}) exceeded {deadline}; {holder} (EBUSY contention, not a driver wedge)" - ), + Some(holder) => { + format!("open_port({port}) exceeded {deadline}; {holder} (timeout cause unknown)") + } None => format!("open_port({port}) exceeded {deadline}; serial driver may be wedged"), } } diff --git a/crates/fbuild-serial/src/manager.rs b/crates/fbuild-serial/src/manager.rs index 431cf123..a53286ee 100644 --- a/crates/fbuild-serial/src/manager.rs +++ b/crates/fbuild-serial/src/manager.rs @@ -116,6 +116,7 @@ impl SharedSerialManager { client_metadata: Option, ) -> fbuild_core::Result<()> { let session_key = self.resolve_port_key(port); + let mut port = port; // A reader that hit a read error (native-USB chips re-enumerate on // DTR/RTS resets) exits but leaves the session "open" with the OS fd // still held, so later opens reuse a dead session and other openers @@ -125,7 +126,19 @@ impl SharedSerialManager { port, "serial reader exited but session still open; closing stale session before reopening" ); + // Logical names (e.g. the pre-renumber deploy port) alias the + // physical session. Keep them, and reopen the physical endpoint + // rather than a logical name that may no longer exist. + let aliases: Vec = self + .port_aliases + .iter() + .filter_map(|entry| (entry.value() == &session_key).then(|| entry.key().clone())) + .collect(); self.close_port(port, client_id).await?; + for alias in aliases { + self.port_aliases.insert(alias, session_key.clone()); + } + port = session_key.as_str(); } // Existing-session reuse is serialized with deploy/reset acquisition // for the same reason as new-session publication below: an attach diff --git a/crates/fbuild-serial/src/manager/tests.rs b/crates/fbuild-serial/src/manager/tests.rs index 9f6b7a3c..1e2ec642 100644 --- a/crates/fbuild-serial/src/manager/tests.rs +++ b/crates/fbuild-serial/src/manager/tests.rs @@ -895,3 +895,34 @@ async fn open_port_closes_session_whose_reader_exited() { "stale session must be removed instead of reused" ); } + +/// Reopening a stale aliased session must target the physical endpoint and +/// keep the logical alias (deploy renumber recovery, #1429 review). +#[tokio::test] +async fn stale_session_cleanup_keeps_logical_alias_to_physical_port() { + let mgr = SharedSerialManager::new(); + let (original, recovered) = ("COM_ORIGINAL", "COM_RECOVERED"); + let reader = tokio::spawn(async {}); + while !reader.is_finished() { + tokio::task::yield_now().await; + } + let mut session = super::SerialSession::new(recovered.to_string(), 115200); + session.is_open = true; + session.reader_handle = Some(reader); + mgr.sessions.insert(recovered.to_string(), session); + mgr.port_aliases + .insert(original.to_string(), recovered.to_string()); + + let _ = tokio::time::timeout( + std::time::Duration::from_millis(200), + mgr.open_port(original, 115200, "client", None, None), + ) + .await; + + assert!(!mgr.sessions.contains_key(recovered)); + assert_eq!( + mgr.port_aliases.get(original).map(|a| a.clone()), + Some(recovered.to_string()), + "logical alias must still resolve to the physical port" + ); +} diff --git a/crates/fbuild-serial/src/port_holders.rs b/crates/fbuild-serial/src/port_holders.rs index 90f7899f..54e3936e 100644 --- a/crates/fbuild-serial/src/port_holders.rs +++ b/crates/fbuild-serial/src/port_holders.rs @@ -58,18 +58,31 @@ fn read_proc_tree(proc_root: &Path) -> Vec<(u32, String, Vec)> { .collect() } -/// Scan a `/proc`-shaped tree for processes holding `port` open. -pub async fn find_port_holders_in(proc_root: &Path, port: &str) -> Vec { +/// Longest the diagnostic may delay an error response. +const SCAN_BUDGET: std::time::Duration = std::time::Duration::from_millis(500); + +/// Scan a `/proc`-shaped tree for processes holding `port` open. The scan +/// walks every process's fd table, so it runs on the blocking pool with a +/// bounded wait; `None` means the scan did not finish in time. +pub async fn find_port_holders_in( + proc_root: &NormalizedPath, + port: &str, +) -> Option> { let port_path = NormalizedPath::new(port); let canonical = canonicalize_existing(port) .await .unwrap_or_else(|_| port_path.clone()); - match_port_holders(read_proc_tree(proc_root), &port_path, &canonical) + let root = proc_root.clone(); + let scan = tokio::task::spawn_blocking(move || read_proc_tree(root.as_path())); + let procs = tokio::time::timeout(SCAN_BUDGET, scan).await.ok()?.ok()?; + Some(match_port_holders(procs, &port_path, &canonical)) } /// Processes currently holding `port` open (Linux `/proc`; empty elsewhere). pub async fn find_port_holders(port: &str) -> Vec { - find_port_holders_in(Path::new("/proc"), port).await + find_port_holders_in(&NormalizedPath::new("/proc"), port) + .await + .unwrap_or_default() } /// Human-readable contention hint, e.g. `port held by fbuild-daemon (pid 42)`. @@ -121,10 +134,10 @@ mod tests { #[tokio::test] async fn missing_proc_root_yields_no_holders() { let dir = tempfile::TempDir::new().unwrap(); - assert!( - find_port_holders_in(&dir.path().join("nope"), "/dev/ttyX") - .await - .is_empty() + let missing = NormalizedPath::new(dir.path().join("nope")); + assert_eq!( + find_port_holders_in(&missing, "/dev/ttyX").await, + Some(Vec::new()) ); } }