diff --git a/crates/fbuild-daemon/src/handlers/locks.rs b/crates/fbuild-daemon/src/handlers/locks.rs index af50b01b..a1f681ce 100644 --- a/crates/fbuild-daemon/src/handlers/locks.rs +++ b/crates/fbuild-daemon/src/handlers/locks.rs @@ -129,6 +129,32 @@ 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 --port {} --stale` now", + session.port, + pids.join(", "), + crate::context::DEAD_CLIENT_SERIAL_RECLAIM_INTERVAL.as_secs(), + session.port + )) +} + /// 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 +592,27 @@ 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 --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()); + } + #[tokio::test] async fn reclaim_keeps_live_and_unknown_pid_sessions() { let ctx = test_context(); @@ -608,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/operations/monitor.rs b/crates/fbuild-daemon/src/handlers/operations/monitor.rs index 33557ba7..f9c7f467 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 c799ac9a..0ceb3c13 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 71a0cdf4..13a1d387 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,39 @@ 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 --port COM_DEAD --stale"), + "{message}" + ); + assert!(!message.contains("wedged"), "{message}"); +}