From 26cb1f0193e6fe16fdb6c813b0c1ecb44c70d714 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 3 Sep 2026 23:53:10 -0700 Subject: [PATCH] Live server: stop ends the process, SSE skips the mutation lane Two Rust-only regressions found while investigating #719, both of which can leave a tab waiting on a broadcast that never comes. /stop ran shutdown() but never set shutting_down, and the accept loop only breaks on that flag or a signal, so a stopped server kept its port and kept answering while its server.json was already deleted. The next `impeccable live` then booted a second server on another port and a tab could reattach to the zombie. Node's shutdown() ended in process.exit(0). The flag is now set after the response is written, so `stop` still reads "stopping" instead of a reset connection, and the accept loop (already non-blocking) exits on its next pass. GET /events took a turnstile ticket and waited its turn before registering, even though handle_sse releases that ticket two statements later and needs no arrival ordering. A peer that stalls mid-request holds the lane for the whole READ_REQUEST_DEADLINE, so a reconnecting stream could sit unregistered for up to 10 seconds (measured 9.71s against 0.00s on Node); broadcast is fire-and-forget, so a `done` landing in that window reaches an empty client set and is gone. Registering early can only make a stream see more broadcasts. The one cost is that the connected frame's activeSessions snapshot may miss a mutation still in flight, and the browser treats that snapshot as a hint. Preflights still take a turn: answering those out of order reorders the POSTs the browser issues behind them. The route classification moved into releases_ticket_up_front so it can be unit tested. tests/live-server-leak.test.mjs gains a guard that a stopped server's pid is gone and its port is free. Co-Authored-By: Claude Code Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --- crates/live/src/live_server.rs | 88 ++++++++++++++++++++++++++++----- tests/live-server-leak.test.mjs | 55 +++++++++++++++++++++ 2 files changed, 132 insertions(+), 11 deletions(-) diff --git a/crates/live/src/live_server.rs b/crates/live/src/live_server.rs index 501eb9396..38539c1d1 100644 --- a/crates/live/src/live_server.rs +++ b/crates/live/src/live_server.rs @@ -549,6 +549,39 @@ fn is_loopback_origin(origin: &str) -> bool { host == "localhost" || host == "127.0.0.1" || host == "::1" || host == "[::1]" } +/// Routes that give their turnstile ticket up before touching server state. +/// +/// Static assets and read-only file routes need no arrival ordering. Neither +/// does the SSE stream: `handle_sse` releases its ticket two statements after +/// it registers anyway, but taking a turn first means the registration waits +/// behind every connection accepted before it, and a peer that stalls +/// mid-request holds the lane for the whole `READ_REQUEST_DEADLINE` (10s). +/// A `done` broadcast that lands in that window reaches an empty client set +/// and is gone for good, which is one way a reconnecting tab sits at the +/// generating loader forever (issue #719). Registering early can only make a +/// stream see MORE broadcasts; the one thing it costs is that the `connected` +/// frame's `activeSessions` snapshot may miss a mutation still in flight, and +/// the browser treats that snapshot as a hint, not as truth. +/// +/// CORS preflights deliberately do NOT release: the browser issues the real +/// POSTs as their preflights are answered, so answering preflights out of +/// order would reorder the POSTs (checkpoint before generate) at the source. +fn releases_ticket_up_front(path: &str, method: &str) -> bool { + matches!( + (path, method), + ("/live.js", _) + | ("/detect.js", _) + | ("/", _) + | ("/modern-screenshot.js", _) + | ("/health", _) + | ("/status", _) + | ("/design-system.json", _) + | ("/design-system/raw", _) + | ("/source", _) + | ("/events", "GET") + ) +} + fn respond(stream: &mut TcpStream, cors: &[(String, String)], res: Response) { send_response(stream, cors, &res); } @@ -586,17 +619,10 @@ fn handle_connection(shared: Shared, mut stream: TcpStream, mut ticket: Ticket) // CORS preflights do take a turn: the browser issues the real POSTs as // their preflights are answered, so answering preflights out of order // would reorder the POSTs (checkpoint before generate) at the source. - match (req.path.as_str(), req.method.as_str()) { - ("/live.js", _) - | ("/detect.js", _) - | ("/", _) - | ("/modern-screenshot.js", _) - | ("/health", _) - | ("/status", _) - | ("/design-system.json", _) - | ("/design-system/raw", _) - | ("/source", _) => ticket.release(), - _ => ticket.wait_turn(), + if releases_ticket_up_front(&req.path, &req.method) { + ticket.release(); + } else { + ticket.wait_turn(); } let token_now = lock(&shared).token.clone(); let mut cors: Vec<(String, String)> = Vec::new(); @@ -904,6 +930,14 @@ fn handle_connection(shared: Shared, mut stream: TcpStream, mut ticket: Ticket) &cors, text_res(200, Some("text/plain"), "stopping"), ); + // JS: shutdown() ends in `process.exit(0)`. Without this the + // accept loop never sees a reason to stop, so a stopped server + // keeps the port and keeps answering while its `server.json` is + // already gone: the next `impeccable live` boots a second server + // on another port and a tab can reattach to the zombie. Set the + // flag after the response is written so `stop` still reads + // `stopping` rather than a reset connection. + lock(&shared).shutting_down = true; } ("/poll", "GET") => handle_poll_get(&shared, stream, &cors, &req, token_ok, &mut ticket), ("/poll", "POST") => { @@ -2585,4 +2619,36 @@ mod content_type_tests { .iter() .any(|(k, v)| k == "Content-Type" && v == "application/javascript; charset=utf-8")); } + + #[test] + fn sse_stream_does_not_take_a_turn_in_the_mutation_lane() { + // The stream releases its ticket right after it registers anyway, so + // waiting for a turn first buys nothing and can park the registration + // behind a stalled peer for the whole read deadline (issue #719). + assert!(releases_ticket_up_front("/events", "GET")); + // Everything that mutates state still passes through the lane in + // arrival order, preflights included: answering those out of order + // reorders the POSTs the browser issues behind them. + assert!(!releases_ticket_up_front("/events", "POST")); + assert!(!releases_ticket_up_front("/events", "OPTIONS")); + assert!(!releases_ticket_up_front("/poll", "POST")); + assert!(!releases_ticket_up_front("/stop", "GET")); + } + + #[test] + fn read_only_and_asset_routes_still_skip_the_lane() { + for path in [ + "/live.js", + "/detect.js", + "/", + "/modern-screenshot.js", + "/health", + "/status", + "/design-system.json", + "/design-system/raw", + "/source", + ] { + assert!(releases_ticket_up_front(path, "GET"), "{path}"); + } + } } diff --git a/tests/live-server-leak.test.mjs b/tests/live-server-leak.test.mjs index 0ef074971..97cd76359 100644 --- a/tests/live-server-leak.test.mjs +++ b/tests/live-server-leak.test.mjs @@ -37,6 +37,8 @@ const WINDOWS = process.platform === 'win32'; const REPO_ROOT = fileURLToPath(new URL('..', import.meta.url)); const PORT = 8591; +// A second port so the stop guard never collides with the reaper guard. +const STOP_PORT = 8592; // The server under test is the engine's `live-server` verb, not a script: this // guarantee has to hold for whatever binary the harness is pointed at. const ENGINE_BIN = findEngineBinary(); @@ -150,6 +152,59 @@ describe('live server leak guard', () => { } }); + it('ends the server process on stop, so the port is free again', { + skip: NO_ENGINE, + }, async () => { + // Node's shutdown() finished with process.exit(0). The engine's did not, + // and nothing set `shutting_down`, so a stopped server kept the port and + // kept answering while its server.json was already gone: the next + // `impeccable live` booted a second server elsewhere and a tab could + // reattach to the zombie and never hear another broadcast (issue #719). + const cwd = mkdtempSync(join(tmpdir(), 'impeccable-stop-')); + let server; + try { + writeFileSync(join(cwd, 'package.json'), '{"name":"stop-fixture"}\n'); + server = spawn(ENGINE_BIN, ['live-server', `--port=${STOP_PORT}`], { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + IMPECCABLE_SKILL_DIR: join(REPO_ROOT, 'skill'), + IMPECCABLE_SELF: ENGINE_BIN, + }, + }); + const up = await waitUntil(async () => { + try { + const res = await fetch(`http://127.0.0.1:${STOP_PORT}/health`); + return res.ok || res.status === 401; + } catch { return false; } + }, { timeoutMs: 20_000 }); + assert.equal(up, true, 'live server never came up'); + + const stop = spawn(ENGINE_BIN, ['live-server', 'stop'], { + cwd, + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + IMPECCABLE_SKILL_DIR: join(REPO_ROOT, 'skill'), + IMPECCABLE_SELF: ENGINE_BIN, + }, + }); + await new Promise((resolve) => stop.on('exit', resolve)); + + const gone = await waitUntil(() => !alive(server.pid), { timeoutMs: 15_000 }); + assert.equal(gone, true, `live server pid ${server.pid} survived its own stop`); + + const stillServing = await fetch(`http://127.0.0.1:${STOP_PORT}/health`) + .then(() => true) + .catch(() => false); + assert.equal(stillServing, false, 'stopped server is still answering on its port'); + } finally { + if (server && server.exitCode == null) server.kill('SIGKILL'); + rmSync(cwd, { recursive: true, force: true }); + } + }); + it('scopes a sweep to the marker, never to the name alone', () => { // No marker, no match: a sweep can never take out a live server that some // other checkout, or the user's own session, is running.