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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-08-01 09:18:57 -04:00
parent 14cf4f3e94
commit 2adb44707d
No known key found for this signature in database
2 changed files with 6 additions and 3 deletions

View file

@ -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;

View file

@ -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<()> {