From c37904e76fd68a4de8ac1bbdbf935207b4d915e8 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Sat, 24 Oct 2026 22:07:02 -0700 Subject: [PATCH 1/2] fix(daemon): name the exited client when an attach times out on a held port (#1429) An attach that timed out against a port the daemon held for an exited client said 'serial driver may be wedged'. The WS attach and HTTP monitor timeout messages now name the dead client pid(s), the automatic reclaim interval and `fbuild daemon clear-locks --stale`. --- crates/fbuild-daemon/src/handlers/locks.rs | 43 +++++++++++++++++++ .../src/handlers/operations/monitor.rs | 25 +++++++---- .../fbuild-daemon/src/handlers/websockets.rs | 16 ++++++- .../src/handlers/websockets_tests.rs | 35 +++++++++++++++ 4 files changed, 109 insertions(+), 10 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/locks.rs b/crates/fbuild-daemon/src/handlers/locks.rs index af50b01b4..e1a8e38e7 100644 --- a/crates/fbuild-daemon/src/handlers/locks.rs +++ b/crates/fbuild-daemon/src/handlers/locks.rs @@ -129,6 +129,31 @@ fn all_clients_dead(session: &PortSessionInfo) -> bool { }) } +/// Describe a session on `port` that is held only for exited clients, for +/// error text: an attach that times out against one should not blame the +/// serial driver (FastLED/fbuild#1429). +pub fn dead_client_holder( + manager: &fbuild_serial::SharedSerialManager, + port: &str, +) -> Option { + let session = manager + .get_port_sessions() + .into_iter() + .find(|session| port_matches(&session.port, port) && all_clients_dead(session))?; + let pids: Vec = session + .clients + .iter() + .filter_map(|client| client.metadata.pid.map(|pid| pid.to_string())) + .collect(); + Some(format!( + "the daemon still holds {} for exited client pid(s) {}; it is released within {}s, \ + or run `fbuild daemon clear-locks --stale` now", + session.port, + pids.join(", "), + crate::context::DEAD_CLIENT_SERIAL_RECLAIM_INTERVAL.as_secs() + )) +} + /// Close serial sessions whose every client process has exited, so a port is /// not held open (EBUSY to every other opener) for a client that is gone /// (FastLED/fbuild#1429). Returns the ports closed. @@ -566,6 +591,24 @@ mod tests { assert!(ctx.serial_manager.get_port_sessions().is_empty()); } + #[test] + fn dead_client_holder_names_the_exited_pid_only_for_dead_sessions() { + let ctx = test_context(); + insert_serial_session(&ctx, "COM_TEST_DEAD", "client-dead", Some(u32::MAX)); + insert_serial_session( + &ctx, + "COM_TEST_LIVE", + "client-live", + Some(std::process::id()), + ); + + let hint = dead_client_holder(&ctx.serial_manager, "COM_TEST_DEAD").unwrap(); + assert!(hint.contains(&u32::MAX.to_string()), "{hint}"); + assert!(hint.contains("clear-locks --stale"), "{hint}"); + assert!(dead_client_holder(&ctx.serial_manager, "COM_TEST_LIVE").is_none()); + assert!(dead_client_holder(&ctx.serial_manager, "COM_NONE").is_none()); + } + #[tokio::test] async fn reclaim_keeps_live_and_unknown_pid_sessions() { let ctx = test_context(); diff --git a/crates/fbuild-daemon/src/handlers/operations/monitor.rs b/crates/fbuild-daemon/src/handlers/operations/monitor.rs index 33557ba70..f9c7f4670 100644 --- a/crates/fbuild-daemon/src/handlers/operations/monitor.rs +++ b/crates/fbuild-daemon/src/handlers/operations/monitor.rs @@ -284,15 +284,24 @@ pub async fn monitor( StatusCode::GATEWAY_TIMEOUT, Json(OperationResponse::fail( request_id, - match fbuild_serial::port_holders::describe_port_holders(&port).await { - Some(holder) => format!( - "open_port({}) exceeded {}s — {} (timeout cause unknown)", + if let Some(holder) = + crate::handlers::locks::dead_client_holder(&ctx.serial_manager, &port) + { + format!( + "open_port({}) exceeded {}s — {}", port, SERIAL_OPEN_PORT_TIMEOUT_SECS, holder - ), - None => format!( - "open_port({}) exceeded {}s — serial driver may be wedged", - port, SERIAL_OPEN_PORT_TIMEOUT_SECS - ), + ) + } else { + match fbuild_serial::port_holders::describe_port_holders(&port).await { + Some(holder) => format!( + "open_port({}) exceeded {}s — {} (timeout cause unknown)", + 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 c799ac9a1..0ceb3c132 100644 --- a/crates/fbuild-daemon/src/handlers/websockets.rs +++ b/crates/fbuild-daemon/src/handlers/websockets.rs @@ -135,7 +135,14 @@ async fn active_preemption_error(ctx: &DaemonContext, port: &str) -> Option String { +async fn open_port_timeout_message( + manager: &fbuild_serial::SharedSerialManager, + port: &str, + deadline: &str, +) -> String { + if let Some(holder) = super::locks::dead_client_holder(manager, port) { + return format!("open_port({port}) exceeded {deadline}; {holder}"); + } match fbuild_serial::port_holders::describe_port_holders(port).await { Some(holder) => { format!("open_port({port}) exceeded {deadline}; {holder} (timeout cause unknown)") @@ -145,6 +152,7 @@ async fn open_port_timeout_message(port: &str, deadline: &str) -> String { } async fn await_ws_serial_open_port( + manager: &fbuild_serial::SharedSerialManager, port: &str, open_future: F, timeout: Duration, @@ -155,7 +163,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(open_port_timeout_message(port, &format_timeout_for_error(timeout)).await), + Err(_) => { + Err(open_port_timeout_message(manager, port, &format_timeout_for_error(timeout)).await) + } } } @@ -209,6 +219,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { // Open port if needed if open_if_needed { let open_result = await_ws_serial_open_port( + &ctx.serial_manager, &port, ctx.serial_manager.open_port( &port, @@ -497,6 +508,7 @@ async fn handle_serial_ws(mut socket: WebSocket, ctx: Arc) { tokio::time::Instant::now() + WS_SERIAL_RECOVERY_TIMEOUT }); let opened = await_ws_serial_open_port( + &ctx.serial_manager, &recovered_port, ctx.serial_manager.open_port( &recovered_port, diff --git a/crates/fbuild-daemon/src/handlers/websockets_tests.rs b/crates/fbuild-daemon/src/handlers/websockets_tests.rs index 71a0cdf41..734906f89 100644 --- a/crates/fbuild-daemon/src/handlers/websockets_tests.rs +++ b/crates/fbuild-daemon/src/handlers/websockets_tests.rs @@ -415,6 +415,7 @@ async fn ws_open_port_timeout_returns_error_with_deadline() { let result = tokio::time::timeout( std::time::Duration::from_secs(1), await_ws_serial_open_port( + &fbuild_serial::SharedSerialManager::new(), "COM_HUNG", std::future::pending::>(), std::time::Duration::from_millis(10), @@ -446,6 +447,7 @@ async fn run_pending_attach_timeout_scope(ctx: std::sync::Arc) -> assert_eq!(ctx.pending_serial_attach_infos().len(), 1); let message = await_ws_serial_open_port( + &ctx.serial_manager, "COM_HUNG", std::future::pending::>(), std::time::Duration::from_millis(10), @@ -742,3 +744,36 @@ fn writer_flushes_data_before_non_data_event() { other => panic!("output[2] expected Data, got {:?}", other), } } + +/// FastLED/fbuild#1429: a hung attach against a port the daemon holds for an +/// exited client names that client instead of blaming the serial driver. +#[tokio::test] +async fn ws_open_port_timeout_names_a_dead_client_holder() { + let manager = fbuild_serial::SharedSerialManager::new(); + let mut session = fbuild_serial::SerialSession::new("COM_DEAD".to_string(), 115200); + session.is_open = true; + session.owner_client_id = Some("client-dead".to_string()); + session.client_metadata.insert( + "client-dead".to_string(), + fbuild_serial::SerialClientMetadata { + pid: Some(u32::MAX), + exe: None, + cwd: None, + argv: None, + }, + ); + manager.insert_session_for_test(session); + + let message = await_ws_serial_open_port( + &manager, + "COM_DEAD", + std::future::pending::>(), + std::time::Duration::from_millis(10), + ) + .await + .expect_err("hung open_port must time out"); + + assert!(message.contains(&u32::MAX.to_string()), "{message}"); + assert!(message.contains("clear-locks --stale"), "{message}"); + assert!(!message.contains("wedged"), "{message}"); +} From 7b868a3030c48c92f9e64cb6a9747db293850de0 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Sat, 24 Oct 2026 22:47:33 -0700 Subject: [PATCH 2/2] fix(daemon): make the dead-client recovery hint target the port `fbuild daemon clear-locks --stale` alone sends serial=false, so the unscoped matcher skips serial sessions. Print `--port --stale` and test that exact request closes the session (CodeRabbit review on #1731). --- crates/fbuild-daemon/src/handlers/locks.rs | 28 +++++++++++++++++-- .../src/handlers/websockets_tests.rs | 5 +++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/fbuild-daemon/src/handlers/locks.rs b/crates/fbuild-daemon/src/handlers/locks.rs index e1a8e38e7..a1f681ce2 100644 --- a/crates/fbuild-daemon/src/handlers/locks.rs +++ b/crates/fbuild-daemon/src/handlers/locks.rs @@ -147,10 +147,11 @@ pub fn dead_client_holder( .collect(); Some(format!( "the daemon still holds {} for exited client pid(s) {}; it is released within {}s, \ - or run `fbuild daemon clear-locks --stale` now", + or run `fbuild daemon clear-locks --port {} --stale` now", session.port, pids.join(", "), - crate::context::DEAD_CLIENT_SERIAL_RECLAIM_INTERVAL.as_secs() + crate::context::DEAD_CLIENT_SERIAL_RECLAIM_INTERVAL.as_secs(), + session.port )) } @@ -604,7 +605,10 @@ mod tests { let hint = dead_client_holder(&ctx.serial_manager, "COM_TEST_DEAD").unwrap(); assert!(hint.contains(&u32::MAX.to_string()), "{hint}"); - assert!(hint.contains("clear-locks --stale"), "{hint}"); + assert!( + hint.contains("clear-locks --port COM_TEST_DEAD --stale"), + "{hint}" + ); assert!(dead_client_holder(&ctx.serial_manager, "COM_TEST_LIVE").is_none()); assert!(dead_client_holder(&ctx.serial_manager, "COM_NONE").is_none()); } @@ -651,6 +655,24 @@ mod tests { assert_eq!(ctx.serial_manager.get_port_sessions().len(), 1); } + /// The recovery command `dead_client_holder` prints must work as typed: + /// `--port --stale` without `--serial`. + #[tokio::test] + async fn hinted_clear_locks_command_closes_the_dead_client_session() { + let ctx = test_context(); + insert_serial_session(&ctx, "COM_TEST_HINT", "client-dead", Some(u32::MAX)); + + let req = ClearLocksRequest { + port: Some("COM_TEST_HINT".to_string()), + stale: true, + ..Default::default() + }; + let Json(response) = clear_locks(State(ctx.clone()), Some(Json(req))).await; + + assert_eq!(response.cleared_serial_count, 1, "{:?}", response.refused); + assert!(ctx.serial_manager.get_port_sessions().is_empty()); + } + #[tokio::test] async fn clear_locks_stale_closes_dead_owner_serial_session() { let ctx = test_context(); diff --git a/crates/fbuild-daemon/src/handlers/websockets_tests.rs b/crates/fbuild-daemon/src/handlers/websockets_tests.rs index 734906f89..13a1d3873 100644 --- a/crates/fbuild-daemon/src/handlers/websockets_tests.rs +++ b/crates/fbuild-daemon/src/handlers/websockets_tests.rs @@ -774,6 +774,9 @@ async fn ws_open_port_timeout_names_a_dead_client_holder() { .expect_err("hung open_port must time out"); assert!(message.contains(&u32::MAX.to_string()), "{message}"); - assert!(message.contains("clear-locks --stale"), "{message}"); + assert!( + message.contains("clear-locks --port COM_DEAD --stale"), + "{message}" + ); assert!(!message.contains("wedged"), "{message}"); }