From 2adb44707de10814cf7edb097af9cadf218319f1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 1 Aug 2026 09:18:57 -0400 Subject: [PATCH] Address Copilot review comments Raise the replacement test's deadline to 20s. The server takes up to 1s to notice the replacement and then bounds its own shutdown at 5s, so the old 5s deadline sat below the worst case and could fail a healthy server on a loaded runner. A passing run still exits in about a second. Reword the SHUTDOWN_TIMEOUT comment. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 5 ++++- lib/apps/fabro-mcp-server/src/server.rs | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 253134714..e810bd4d9 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -555,7 +555,10 @@ fn stdio_server_exits_when_executable_is_replaced() { fs::write(&replacement, b"replacement").expect("replacement file should be written"); fs::rename(replacement, &executable).expect("Fabro executable should be replaced"); - let deadline = Instant::now() + Duration::from_secs(5); + // The server takes up to 1s to notice the replacement and then bounds its + // own shutdown at 5s, so 6s is the worst case. Allow more so a loaded runner + // cannot fail a healthy server; a passing run exits in about a second. + let deadline = Instant::now() + Duration::from_secs(20); let status = loop { if let Some(status) = child.try_wait().expect("MCP server should be polled") { break status; diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 924ddf9d5..6a9c9941d 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -29,8 +29,8 @@ pub(crate) struct FabroMcpServer { } /// How long to wait for the MCP service to stop after an upgrade is detected. -/// Bounded because the transport closes by writing to a stdout the host may -/// already have stopped reading. +/// The wait is bounded because the transport closes by writing to stdout, which +/// blocks if the host has stopped reading. const SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(5); pub async fn start(settings: FabroMcpServerSettings) -> Result<()> {