fix(daemon): name the exited client when an attach times out on a held port (#1429)

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<String> {
+    let session = manager
+        .get_port_sessions()
+        .into_iter()
+        .find(|session| port_matches(&session.port, port) && all_clients_dead(session))?;
+    let pids: Vec<String> = 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 <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();

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
+                            ),
+                        }
                     },
                 )),
             );

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

@@ -135,7 +135,14 @@ async fn active_preemption_error(ctx: &DaemonContext, port: &str) -> Option<Stri
 /// Timeout text for a stalled `open_port`. When another process (often the
 /// daemon itself) visibly holds the port, report contention instead of a
 /// driver wedge (FastLED/fbuild#1429).
-async fn open_port_timeout_message(port: &str, deadline: &str) -> 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<F>(
+    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<DaemonContext>) {
                     // 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<DaemonContext>) {
                                     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,

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::<fbuild_core::Result<()>>(),
             std::time::Duration::from_millis(10),
@@ -446,6 +447,7 @@ async fn run_pending_attach_timeout_scope(ctx: std::sync::Arc<DaemonContext>) ->
     assert_eq!(ctx.pending_serial_attach_infos().len(), 1);
 
     let message = await_ws_serial_open_port(
+        &ctx.serial_manager,
         "COM_HUNG",
         std::future::pending::<fbuild_core::Result<()>>(),
         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::<fbuild_core::Result<()>>(),
+        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}");
+}