diff --git a/crates/ff-rdp-cli/src/daemon/server.rs b/crates/ff-rdp-cli/src/daemon/server.rs index b61146e..14ceeff 100644 --- a/crates/ff-rdp-cli/src/daemon/server.rs +++ b/crates/ff-rdp-cli/src/daemon/server.rs @@ -380,6 +380,38 @@ const WATCHER_STARTUP_RETRY: Duration = Duration::from_millis(2500); /// later, so we keep trying for the practical lifetime of an idle daemon. const WATCHER_BACKGROUND_RETRY: Duration = Duration::from_mins(10); +/// How long to wait for a just-subscribed top-level target to prove itself +/// stable before trusting it (iter-146 Theme C). +/// +/// Reproduced live and confirmed on the wire (Firefox 153, headless): a +/// freshly-launched profile's very first tab starts on a **placeholder** +/// `about:blank` `WindowGlobalTarget` that Firefox tears down and replaces +/// within microseconds of being observed — independent of any navigation. +/// `establish_watcher`'s `watchTargets("frame")` catch-up burst can win the +/// race against that teardown, subscribing to the placeholder a moment +/// before Firefox destroys it. When that happens the daemon logs exactly +/// `target-available-form(about:blank)` immediately followed by +/// `target-destroyed-form` (171µs apart in the captured repro) and then +/// **nothing else, ever** — no replacement `target-available-form` for the +/// real tab arrives on this watcher for the rest of the session, even +/// though the tab itself keeps working fine (`navigate`/`eval` succeed +/// normally; only the target-lifecycle subscription is orphaned). This was +/// the `live_137_frame_targets_via_daemon` / `live_137_click_cross_origin_via_daemon` +/// flake's remaining cause after the iter-146 `event_sink` fix closed the +/// separate catch-up-burst-drop race: `target_count` reproducibly got stuck +/// at exactly `1` (the placeholder's own catch-up) instead of climbing +/// past it into a stable second, third target once observed. +/// +/// [`establish_watcher_with_retry`] waits this long, once, before the +/// *first* subscribe attempt — giving Firefox's placeholder→real promotion +/// (a startup-only event) time to finish before the daemon ever calls +/// `getWatcher`/`watchTargets`, so the catch-up burst it eventually +/// receives describes the settled, stable target instead of a dying one. +/// Bounded well inside [`WATCHER_STARTUP_RETRY`]'s 2.5 s budget so a normal +/// daemon spawn still registers comfortably inside the client's 5 s +/// `wait_for_registry` window. +const WATCHER_SETTLE_DELAY: Duration = Duration::from_millis(350); + /// A successfully-established resource watcher: the watcher actor ID plus the /// `ResourceCommand` bus and typed receiver the dispatcher fans events through /// (iter-123 Theme A). @@ -443,6 +475,11 @@ fn establish_watcher_with_retry( transport: &mut RdpTransport, budget: Duration, ) -> Option { + // iter-146 Theme C: see WATCHER_SETTLE_DELAY's doc for the race this + // closes. Only worth paying once, before the first attempt — by the + // time any retry loop below runs, the settle window has already + // elapsed in wall-clock terms. + thread::sleep(WATCHER_SETTLE_DELAY); let deadline = Instant::now() + budget; loop { match establish_watcher(transport) { @@ -510,6 +547,14 @@ fn background_establish_watcher_loop( } } + // iter-146 Theme C: same fix as the startup path in `run_daemon` — install + // an event sink before the synchronous `establish_watcher` handshake so a + // `target-available-form` catch-up event that races ahead of its RPC + // reply is buffered instead of silently dropped by `forward_event`. See + // the comment at the startup call site for the full mechanism. + let (early_tx, early_rx) = mpsc::channel::(); + transport.set_event_sink(Some(early_tx)); + // Poll for a tab until one appears, the daemon shuts down, or we exhaust the // generous background budget. let deadline = Instant::now() + WATCHER_BACKGROUND_RETRY; @@ -534,6 +579,13 @@ fn background_establish_watcher_loop( } } }; + // Replay whatever the sink captured into the shared event channel, ahead + // of the live pump loop started below, in wire order. + for early_event in early_rx.try_iter() { + if state.event_tx.send(early_event).is_err() { + return; + } + } // Publish the watcher actor so the dispatcher recognises its events. { @@ -618,6 +670,25 @@ pub(crate) fn run_daemon( // *without* a watcher and hand off to a background establisher thread that // keeps trying — the registry is written and the daemon reaches // `running:true` either way. + // iter-146 Theme C: install an event sink on `transport` BEFORE the + // synchronous watcher handshake below. `watchTargets` delivers a + // catch-up burst of `target-available-form` events for every + // already-existing target, and Firefox is not required to send them + // *after* the `watchTargets` reply — on the wire they can arrive + // interleaved with (or even before) it. `recv_reply_from` forwards any + // such stray event via `RdpTransport::forward_event`, which silently + // drops it when no sink is installed (`event_sink: None` from + // `connect_raw`). With no sink here, that race made `target_count` / + // `live_target_count` stay 0 for an entire daemon session whenever the + // catch-up burst won the race — reproduced live (~1/3 of runs) as the + // `live_137_frame_targets_via_daemon` / `live_137_click_cross_origin_via_daemon` + // flake: `wait_for_live_targets` timing out with `target_count: 0` despite + // a real navigation having already happened. Buffered here and replayed + // into `state.event_tx` (in wire order, ahead of whatever the reader + // thread delivers live after the handshake) once that channel exists. + let (early_tx, early_rx) = mpsc::channel::(); + transport.set_event_sink(Some(early_tx)); + let established = establish_watcher_with_retry(&mut transport, WATCHER_STARTUP_RETRY); let initial_watcher_actor = established .as_ref() @@ -660,6 +731,16 @@ pub(crate) fn run_daemon( // reader never blocks in normal SPA traffic (hundreds of events/s). let (event_tx, event_rx) = mpsc::sync_channel::(4096); + // iter-146 Theme C: replay whatever `establish_watcher_with_retry`'s + // sink captured (see the comment at its installation above) into the + // real event channel, in the order Firefox sent it, before the + // dispatcher thread starts draining `event_rx` below. `try_iter` is + // exhaustive-but-nonblocking: `early_tx` was only ever held by the now- + // finished synchronous handshake, so there is nothing left to arrive. + for early_event in early_rx.try_iter() { + let _ = event_tx.send(early_event); + } + // Grip release queue (iter-76 Theme B, wired in iter-76b): watcher event // parsers wrap grip actor IDs in ResourceGripGuard instances backed by // this sender. The receiver is owned by the grip-release-drainer thread @@ -2732,6 +2813,23 @@ mod tests { use super::*; + /// AC `unit_watcher_settle_delay_fits_inside_startup_retry_budget` + /// (iter-146 Theme C): the one-time settle delay + /// `establish_watcher_with_retry` pays before its first subscribe + /// attempt must leave room for at least one real attempt inside + /// `WATCHER_STARTUP_RETRY`'s budget — otherwise a daemon spawn against a + /// slow-starting Firefox could burn the whole retry window on the delay + /// alone and never establish a watcher at all. + #[test] + fn unit_watcher_settle_delay_fits_inside_startup_retry_budget() { + assert!( + WATCHER_SETTLE_DELAY < WATCHER_STARTUP_RETRY, + "WATCHER_SETTLE_DELAY ({WATCHER_SETTLE_DELAY:?}) must leave room for at least one \ + establish_watcher attempt inside WATCHER_STARTUP_RETRY's budget \ + ({WATCHER_STARTUP_RETRY:?})" + ); + } + /// AC `unit_establish_watcher_tabless_is_non_fatal` (iter-123 Theme A): /// when `listTabs` returns zero tabs, `establish_watcher` returns `Ok(None)` /// — a non-fatal signal — instead of erroring, so `run_daemon` can still diff --git a/crates/ff-rdp-cli/tests/live/live_146_suite_reliability.rs b/crates/ff-rdp-cli/tests/live/live_146_suite_reliability.rs new file mode 100644 index 0000000..0a53a75 --- /dev/null +++ b/crates/ff-rdp-cli/tests/live/live_146_suite_reliability.rs @@ -0,0 +1,279 @@ +//! Live tests for iteration 146 — live suite reliability. +//! +//! ## Theme A — the live-test harness's own teardown +//! +//! `LiveFirefox`'s `Drop` was already reliable (verified live before this +//! iteration: a throwaway probe test that panics after `with_daemon()` +//! leaves zero surviving processes). The actual leak iter-146 found in a +//! full sequential sweep traced to `live_96_profile_cleanup.rs`'s +//! `launch_headless()`, which used to launch Firefox via a bare `Command` +//! with **no** RAII guard at all — see the fix and doc comment there. The +//! tests below pin the harness-wide guarantee so it can't regress silently: +//! every `LiveFirefox` (with or without a running daemon) must leave no +//! surviving process once its guard drops, even through a panic. +//! +//! ## Theme C — the iter-137 daemon-parity flake +//! +//! Root-caused live (not merely widened a timeout — see +//! `daemon/server.rs`'s `WATCHER_SETTLE_DELAY` and the early-event-sink +//! comments at both `establish_watcher_with_retry` call sites for the full +//! mechanism). `live_146_daemon_parity_stable_repeat` locks in that fix with +//! repeated runs of the exact daemon-mode-parity shape iter-137 introduced. +//! +//! # Running +//! +//! FF_RDP_LIVE_TESTS=1 cargo test -p ff-rdp-cli --test live live_146 -- --nocapture + +use std::panic::AssertUnwindSafe; +use std::process::Command; + +use crate::common::{LiveFirefox, ff_rdp_bin, live_tests_enabled, pid_alive}; + +/// Poll until `pid_alive(pid)` is `false` or `timeout` elapses, returning +/// the final liveness. +/// +/// `kill_pid`'s `SIGKILL` is asynchronous — the kernel needs a moment to +/// actually reap the process, so a liveness probe taken immediately after +/// `Drop` can still observe "alive" for a few milliseconds. Every assertion +/// in this file that a Firefox PID is gone polls through this helper rather +/// than checking once, so it verifies the guard's eventual guarantee +/// (bounded and small — 2 s is generous headroom) instead of racing its own +/// probe against the kernel. +fn wait_until_dead(pid: u32, timeout: std::time::Duration) -> bool { + let deadline = std::time::Instant::now() + timeout; + loop { + if !pid_alive(pid) { + return true; + } + if std::time::Instant::now() >= deadline { + return false; + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } +} + +/// AC: `live_146_no_orphan_firefox_after_suite` — after a small sequential +/// run of `LiveFirefox` (+ daemon) launches — the shape a live suite takes — +/// zero of the Firefox processes they started are still alive once every +/// guard has dropped. Mirrors the dogfood_path's "zero ff-rdp-owned Firefox +/// processes remain" bar at a scale this test can run unattended. +#[test] +#[ignore = "requires Firefox — FF_RDP_LIVE_TESTS=1"] +fn live_146_no_orphan_firefox_after_suite() { + const INSTANCES: usize = 3; + + if !live_tests_enabled() { + return; + } + + let mut pids = Vec::with_capacity(INSTANCES); + for i in 0..INSTANCES { + let Some(ff) = LiveFirefox::headless_on_random_port() else { + eprintln!("live_146_no_orphan_firefox_after_suite: Firefox not available — skipping"); + return; + }; + pids.push(ff.pid()); + if i == 1 { + // Exercise the daemon-spawning path too — the shape every + // `firefox_with_daemon` helper across the live suite uses. + let _ = ff.with_daemon(); + } + // `ff` drops at the end of this iteration, killing this instance + // before the next one launches — modeling a sequential suite run. + } + + for pid in &pids { + assert!( + wait_until_dead(*pid, std::time::Duration::from_secs(2)), + "live_146_no_orphan_firefox_after_suite: Firefox pid {pid} is still alive after \ + its LiveFirefox guard dropped" + ); + } + + eprintln!( + "live_146_no_orphan_firefox_after_suite: PASS — {}/{INSTANCES} sequential launches \ + left no survivor", + pids.len() + ); +} + +/// AC: `live_146_harness_teardown_kills_daemon_spawned_firefox` — a test +/// that starts Firefox via `with_daemon` and then panics still leaves zero +/// surviving processes once its `LiveFirefox` guard drops (dropped as part +/// of the panic's unwind, exactly as `cargo test`'s own per-test harness +/// does). This is the guarantee `live_96_profile_cleanup.rs`'s pre-iter-146 +/// `launch_headless()` helper lacked entirely — see the Theme A fix there. +#[test] +#[ignore = "requires Firefox — FF_RDP_LIVE_TESTS=1"] +fn live_146_harness_teardown_kills_daemon_spawned_firefox() { + if !live_tests_enabled() { + return; + } + + let pid_cell = std::cell::Cell::new(None::); + let outcome = std::panic::catch_unwind(AssertUnwindSafe(|| { + let Some(ff) = LiveFirefox::headless_on_random_port() else { + return false; + }; + pid_cell.set(Some(ff.pid())); + if ff.with_daemon().is_none() { + return false; + } + // `ff` is moved into and dies inside this closure's unwind — the + // scenario under test: a live test that panics mid-assertion with + // its daemon-backed Firefox still in scope. + panic!("iter-146 probe: intentional panic with a daemon running"); + })); + + let Some(pid) = pid_cell.get() else { + eprintln!( + "live_146_harness_teardown_kills_daemon_spawned_firefox: Firefox not available — \ + skipping" + ); + return; + }; + match outcome { + Ok(true) => unreachable!("the probe closure always panics once the daemon starts"), + Ok(false) => { + eprintln!( + "live_146_harness_teardown_kills_daemon_spawned_firefox: daemon did not start \ + for pid {pid} — skipping" + ); + } + Err(_) => { + assert!( + wait_until_dead(pid, std::time::Duration::from_secs(2)), + "live_146_harness_teardown_kills_daemon_spawned_firefox: Firefox pid {pid} \ + survived a panic while its daemon was running" + ); + eprintln!( + "live_146_harness_teardown_kills_daemon_spawned_firefox: PASS — pid {pid} is \ + gone" + ); + } + } +} + +/// A `data:` fixture identical to live_137's `CROSS_ORIGIN_FIXTURE`: a top +/// document (unique origin) embedding a genuinely cross-origin +/// `https://example.com` iframe. +const CROSS_ORIGIN_FIXTURE: &str = + r#"data:text/html,

top

"#; + +/// Default-daemon-mode global args (the proxied path — no direct-connection +/// override flag). +fn daemon_args(port: u16) -> Vec { + vec![ + "--host".to_owned(), + "127.0.0.1".to_owned(), + "--port".to_owned(), + port.to_string(), + "--timeout".to_owned(), + "30000".to_owned(), + ] +} + +fn daemon_status(port: u16) -> String { + let out = Command::new(ff_rdp_bin()) + .args(["--host", "127.0.0.1", "--port", &port.to_string()]) + .args(["daemon", "status"]) + .output() + .expect("daemon status"); + String::from_utf8_lossy(&out.stdout).into_owned() +} + +/// Poll `daemon status` until it reports at least one **live** target — see +/// live_137's identical helper for the full rationale. +fn wait_for_live_targets(port: u16) -> bool { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15); + while std::time::Instant::now() < deadline { + let text = daemon_status(port); + if let Ok(json) = serde_json::from_str::(&text) + && json["results"]["live_target_count"].as_u64().unwrap_or(0) >= 1 + { + return true; + } + std::thread::sleep(std::time::Duration::from_millis(250)); + } + false +} + +/// AC: `live_146_daemon_parity_stable_repeat` — `live_137_frame_targets_via_daemon` +/// / `live_137_click_cross_origin_via_daemon`'s core shape (launch, start the +/// daemon, navigate to a cross-origin fixture, wait for live frame targets) +/// passes 5 consecutive fresh-daemon runs. +/// +/// Root cause (documented in `kb/iterations/iteration-146-live-suite-reliability.md` +/// and at the fix sites in `daemon/server.rs`): NOT a daemon restart — +/// verified live via `daemon status`'s `uptime_seconds`, which stayed +/// continuous across a failing run's whole session, and via `RUST_LOG=debug` +/// captures showing one unbroken daemon PID throughout. Two real bugs +/// combined to strand `target_count` at 0 or 1 forever: +/// 1. `establish_watcher`'s synchronous `watchTargets` handshake ran with +/// no `RdpTransport` event sink installed, so a `target-available-form` +/// catch-up event racing ahead of its RPC reply was silently dropped by +/// `forward_event` (fixed: an early sink now buffers and replays it). +/// 2. A freshly-launched profile's very first tab is a placeholder +/// `about:blank` target that Firefox tears down within microseconds of +/// being observed, independent of any navigation; subscribing to it +/// before it settles orphans the watcher for the rest of the session — +/// no replacement `target-available-form` ever arrives, even though +/// `navigate`/`eval` keep working normally (fixed: `WATCHER_SETTLE_DELAY` +/// gives the placeholder→real promotion time to finish before the +/// daemon ever subscribes). +#[test] +#[ignore = "requires Firefox + network — FF_RDP_LIVE_TESTS=1"] +fn live_146_daemon_parity_stable_repeat() { + const ITERATIONS: usize = 5; + + if !live_tests_enabled() { + return; + } + + for i in 1..=ITERATIONS { + let Some(ff) = LiveFirefox::headless_on_random_port() else { + eprintln!("live_146_daemon_parity_stable_repeat: Firefox not available — skipping"); + return; + }; + if ff.with_daemon().is_none() { + eprintln!( + "live_146_daemon_parity_stable_repeat: daemon did not start on iteration \ + {i}/{ITERATIONS} — skipping" + ); + return; + } + let port = ff.port(); + + let nav = Command::new(ff_rdp_bin()) + .args(daemon_args(port)) + .args(["navigate", CROSS_ORIGIN_FIXTURE, "--allow-unsafe-urls"]) + .output() + .expect("navigate via daemon"); + if !nav.status.success() { + eprintln!( + "live_146_daemon_parity_stable_repeat: navigate failed on iteration \ + {i}/{ITERATIONS} — {}", + String::from_utf8_lossy(&nav.stderr) + ); + return; + } + + assert!( + wait_for_live_targets(port), + "live_146_daemon_parity_stable_repeat: iteration {i}/{ITERATIONS} — daemon never \ + reported live frame targets (iter-146 Theme C regression) — status: {}", + daemon_status(port) + ); + + eprintln!("live_146_daemon_parity_stable_repeat: iteration {i}/{ITERATIONS} PASSED"); + // `ff` drops here, killing this iteration's Firefox (and its daemon, + // once it notices the lost connection) before the next fresh daemon + // spawns — each iteration exercises the exact race window from + // scratch. + } + + eprintln!( + "live_146_daemon_parity_stable_repeat: PASS — {ITERATIONS}/{ITERATIONS} consecutive \ + runs" + ); +} diff --git a/crates/ff-rdp-cli/tests/live/live_96_profile_cleanup.rs b/crates/ff-rdp-cli/tests/live/live_96_profile_cleanup.rs index 0381554..15e1156 100644 --- a/crates/ff-rdp-cli/tests/live/live_96_profile_cleanup.rs +++ b/crates/ff-rdp-cli/tests/live/live_96_profile_cleanup.rs @@ -18,12 +18,7 @@ use std::time::Duration; use crate::common::ff_rdp_bin; use crate::common::live_tests_enabled; - -/// Attempt to bind `:0` to discover a free port. -fn free_port() -> Option { - let l = std::net::TcpListener::bind("127.0.0.1:0").ok()?; - Some(l.local_addr().ok()?.port()) -} +use crate::common::{LiveFirefox, pid_alive}; /// Poll until the path at `path` no longer exists, or `timeout` elapses. fn wait_path_gone(path: &str, timeout: Duration) -> bool { @@ -39,25 +34,33 @@ fn wait_path_gone(path: &str, timeout: Duration) -> bool { } } -/// Launch Firefox headless via the CLI on a freshly discovered port and -/// return `(port, results)` where `results` is the `results` object of the -/// launch JSON envelope. Returns `None` if the launch fails. -fn launch_headless() -> Option<(u16, serde_json::Value)> { - let port = free_port()?; - let out = Command::new(ff_rdp_bin()) - .args(["launch", "--headless", "--debug-port", &port.to_string()]) - .output() - .ok()?; - if !out.status.success() { - eprintln!( - "launch_headless: launch failed — stderr={}", - String::from_utf8_lossy(&out.stderr) - ); - return None; - } - let json: serde_json::Value = serde_json::from_slice(&out.stdout).ok()?; - let results = json.get("results")?.clone(); - Some((port, results)) +/// Launch Firefox headless and return `(LiveFirefox, results)` where +/// `results` is the `results` object of the `launch` JSON envelope. +/// Returns `None` if the launch fails. +/// +/// iter-146 Theme A: this used to be a bare `Command::new(ff_rdp_bin())` +/// launch returning only `(port, results)` — no RAII guard at all. Every +/// other live suite kills its Firefox via `LiveFirefox`'s `Drop` (robust +/// even through a panic — verified live in iter-146), but this file relied +/// entirely on `daemon stop` succeeding to clean up, with **no fallback**. +/// If `daemon stop` ever failed (or an assertion between launch and `daemon +/// stop` panicked — e.g. under the CPU contention a full sequential suite +/// run creates), Firefox leaked with nothing to reap it: the exact PID/args +/// shape of the four orphaned Firefox instances iter-146 found alive after +/// a live sweep (`firefox -no-remote --start-debugger-server … --headless +/// --profile …/ff-rdp-profile-…`) matches precisely what this helper +/// produces. Returning a `LiveFirefox` here makes `daemon stop`'s own +/// cleanup (still asserted below) belt, and the guard's `Drop` suspenders — +/// on any panic after this call, Firefox dies anyway. +fn launch_headless() -> Option<(LiveFirefox, serde_json::Value)> { + let (ff, envelope) = LiveFirefox::headless_on_random_port_with_args(&[])?; + // `headless_on_random_port_with_args` returns the *whole* launch JSON + // envelope (`{"results": {...}, "total": 1, "meta": {...}}`); callers + // here want just the `results` object, matching this helper's + // pre-iter-146 return shape so the test bodies below didn't need to + // change their `launch_results["profile_path"]`-style indexing. + let results = envelope.get("results")?.clone(); + Some((ff, results)) } /// AC: `pre_fix_repro_daemon_stop_removes_active_profile` @@ -74,12 +77,17 @@ fn pre_fix_repro_daemon_stop_removes_active_profile() { return; } - let Some((port, launch_results)) = launch_headless() else { + let Some((ff, launch_results)) = launch_headless() else { eprintln!( "pre_fix_repro_daemon_stop_removes_active_profile: Firefox not available — skipping" ); return; }; + // iter-146 Theme A: `ff` stays in scope for the rest of the test as a + // Drop-based safety net — see `launch_headless`'s doc comment. On the + // happy path below, `daemon stop` already kills Firefox and this + // guard's `Drop` is a harmless no-op (the PID is already dead). + let port = ff.port(); let profile_path = launch_results["profile_path"] .as_str() @@ -138,12 +146,14 @@ fn live_daemon_stop_profile_path_matches_launch_json() { return; } - let Some((port, launch_results)) = launch_headless() else { + let Some((ff, launch_results)) = launch_headless() else { eprintln!( "live_daemon_stop_profile_path_matches_launch_json: Firefox not available — skipping" ); return; }; + // iter-146 Theme A: safety net — see `launch_headless`'s doc comment. + let port = ff.port(); let launch_profile_path = launch_results["profile_path"] .as_str() @@ -200,6 +210,42 @@ fn live_daemon_stop_profile_path_matches_launch_json() { eprintln!("live_daemon_stop_profile_path_matches_launch_json: PASS — {launch_profile_path}"); } +/// Path to the owner-PID marker written inside every ff-rdp-managed profile +/// dir (mirrors the product's private `util::profile_dir::OWNER_PID_MARKER`, +/// unreachable from an integration-test binary — see +/// `write_owner_pid_marker`/`read_owner_pid_marker` there). +const OWNER_PID_MARKER: &str = ".ff-rdp-owner-pid"; + +/// Scan `root` for `ff-rdp-profile-*` directories whose owner-PID marker +/// names a still-alive process, returning `(dir, pid)` pairs. +/// +/// iter-146 Theme B: unlike a `daemon status` check (the precondition this +/// replaces), this also catches a Firefox instance launched via `ff-rdp +/// launch` that never triggered daemon autostart — the exact gap the old +/// precondition's own doc comment acknowledged and that made +/// `live_profiles_prune_removes_all_when_no_firefox_running` order-dependent: +/// it passed in isolation but failed late in a full sequential suite run +/// with an opaque `left: 1 / right: 0`, because `prune --all` **correctly** +/// refused to delete a profile some earlier test's Firefox still owned. +fn live_owned_profile_dirs(root: &str) -> Vec<(std::path::PathBuf, u32)> { + let Ok(entries) = std::fs::read_dir(root) else { + return Vec::new(); + }; + entries + .flatten() + .filter(|e| { + e.file_name() + .to_str() + .is_some_and(|n| n.starts_with("ff-rdp-profile-")) + }) + .filter_map(|e| { + let marker = e.path().join(OWNER_PID_MARKER); + let pid: u32 = std::fs::read_to_string(&marker).ok()?.trim().parse().ok()?; + pid_alive(pid).then_some((e.path(), pid)) + }) + .collect() +} + /// AC: `live_profiles_prune_removes_all_when_no_firefox_running` /// /// Seeds orphan `ff-rdp-profile-*` directories directly under the real @@ -209,12 +255,14 @@ fn live_daemon_stop_profile_path_matches_launch_json() { /// /// Requires no *running* ff-rdp-managed Firefox instance — `--all` removes /// every managed directory regardless of age, which would rip the profile -/// out from under a live session. This test never kills anything; it just -/// skips (rather than force-stopping a session) when `ff-rdp daemon status` -/// reports one is active. This is a best-effort check: a Firefox instance -/// launched via `ff-rdp launch` that never triggered daemon auto-start -/// (no other command has run against it yet) wouldn't be visible to -/// `daemon status` — see the iter-96 Theme C plan for the acknowledged gap. +/// out from under a live session. iter-146 Theme B: the precondition is now +/// an explicit, named-PID assertion (`live_owned_profile_dirs`) rather than +/// a `daemon status` skip check — the latter went silently blind to a +/// directly-launched Firefox that never triggered daemon autostart, which +/// is exactly the gap iter-146 Theme A's own leak exercised. This test still +/// never kills anything itself; a live owner means the test environment +/// isn't clean, which is worth failing loudly on rather than skipping quietly +/// or reporting a bare `left: 1 / right: 0` at the very end. #[test] #[ignore = "touches the real per-user profile root — set FF_RDP_LIVE_TESTS=1"] fn live_profiles_prune_removes_all_when_no_firefox_running() { @@ -222,21 +270,6 @@ fn live_profiles_prune_removes_all_when_no_firefox_running() { return; } - let status_out = Command::new(ff_rdp_bin()) - .args(["daemon", "status"]) - .output(); - if let Ok(out) = status_out - && out.status.success() - && let Ok(json) = serde_json::from_slice::(&out.stdout) - && json["results"]["running"].as_bool() == Some(true) - { - eprintln!( - "live_profiles_prune_removes_all_when_no_firefox_running: \ - a daemon is running — skipping to avoid pruning a live session's profile" - ); - return; - } - let list_out = Command::new(ff_rdp_bin()) .args(["profiles", "list"]) .output() @@ -260,6 +293,24 @@ fn live_profiles_prune_removes_all_when_no_firefox_running() { ) .to_owned(); + // iter-146 Theme B: explicit, named precondition — see + // `live_owned_profile_dirs`'s doc comment for why this replaces the old + // `daemon status`-only skip check. + let live_owners = live_owned_profile_dirs(&root); + assert!( + live_owners.is_empty(), + "live_profiles_prune_removes_all_when_no_firefox_running: precondition violated — \ + {} ff-rdp-managed profile dir(s) under {root} are still owned by a live process, so \ + `prune --all` would rip a profile out from under it: {}. Rerun once these have \ + exited (or in an isolated environment).", + live_owners.len(), + live_owners + .iter() + .map(|(dir, pid)| format!("{} (pid {pid})", dir.display())) + .collect::>() + .join(", ") + ); + // Seed a handful of orphan managed profile dirs directly on disk. let seeded: Vec = (0..3) .map(|i| { @@ -292,20 +343,31 @@ fn live_profiles_prune_removes_all_when_no_firefox_running() { ); } - let remaining = std::fs::read_dir(&root).map_or(0, |entries| { - entries - .flatten() - .filter(|e| { - e.file_name() - .to_str() - .is_some_and(|n| n.starts_with("ff-rdp-profile-")) - }) - .count() - }); - assert_eq!( - remaining, 0, + let remaining: Vec = std::fs::read_dir(&root).map_or_else( + |_| Vec::new(), + |entries| { + entries + .flatten() + .filter(|e| { + e.file_name() + .to_str() + .is_some_and(|n| n.starts_with("ff-rdp-profile-")) + }) + .map(|e| e.path()) + .collect() + }, + ); + // iter-146 Theme B: name what's left (and whether it has a live owner) + // instead of a bare count — this precondition-checked test now failing + // here means `prune --all` itself is broken, not stale suite state. + assert!( + remaining.is_empty(), "live_profiles_prune_removes_all_when_no_firefox_running: expected zero \ - ff-rdp-profile-* dirs under {root} after prune --all, found {remaining}" + ff-rdp-profile-* dirs under {root} after prune --all, found {}: {:?} (live owners \ + among them: {:?})", + remaining.len(), + remaining, + live_owned_profile_dirs(&root) ); eprintln!("live_profiles_prune_removes_all_when_no_firefox_running: PASS — root={root}"); diff --git a/crates/ff-rdp-cli/tests/live/main.rs b/crates/ff-rdp-cli/tests/live/main.rs index c4bba38..804c091 100644 --- a/crates/ff-rdp-cli/tests/live/main.rs +++ b/crates/ff-rdp-cli/tests/live/main.rs @@ -60,6 +60,7 @@ mod live_142_eval_asi_await; mod live_143_native_a11y_tree; mod live_144_session_hygiene_followup; mod live_145_error_envelope_completeness; +mod live_146_suite_reliability; mod live_61l; mod live_61q_resource_bus; mod live_61r_eval; diff --git a/kb/iterations/iteration-146-live-suite-reliability.md b/kb/iterations/iteration-146-live-suite-reliability.md index ecbd9f7..4ecb3e2 100644 --- a/kb/iterations/iteration-146-live-suite-reliability.md +++ b/kb/iterations/iteration-146-live-suite-reliability.md @@ -8,7 +8,7 @@ dogfood_path: | # → after the suite exits, zero ff-rdp-owned Firefox processes may remain and # zero ff-rdp-profile-* directories may be left pinned first_call_sites: [] -status: planned +status: in-review title: "Iteration 146: live suite reliability — leaked Firefox, order-dependent tests, daemon-parity flake" type: iteration tags: [iteration] @@ -81,24 +81,53 @@ background thread") without making the test robust to it. **Root-cause this befo anything**: a daemon restarting mid-test may itself be the real defect, in which case widening the timeout would paper over a genuine product bug. -## Acceptance Criteria [0/5] - -- [ ] live_146_no_orphan_firefox_after_suite: after a full sequential live run, zero processes - matching `firefox.*ff-rdp-profile` remain and zero `ff-rdp-profile-*` directories are left - pinned by a live owner -- [ ] live_146_harness_teardown_kills_daemon_spawned_firefox: a test that starts Firefox via - `firefox_with_daemon` leaves no surviving process once its guard drops, including when the - test body panics -- [ ] live_96_profile_cleanup_precondition_asserted: the prune test asserts "no ff-rdp Firefox - running" as an explicit precondition and, on violation, names the offending PIDs in the - failure message instead of reporting a bare count mismatch -- [ ] live_146_daemon_parity_stable_repeat: `live_137_frame_targets_via_daemon` and - `live_137_click_cross_origin_via_daemon` each pass 5 consecutive runs, with the root cause - of the restart documented in this plan (not merely a raised timeout) -- [ ] live_146_daemon_restart_observable: if a daemon restart mid-test is confirmed as the - mechanism, `daemon status` exposes enough signal (e.g. a restart counter or start - timestamp) for a test to distinguish "subscription not yet live" from "daemon never - subscribed" +## Resolution + +- **Theme A**: `live_96_profile_cleanup.rs`'s `launch_headless()` was the leak — it launched + Firefox via a bare `Command::new(ff_rdp_bin()).args(["launch", ...])` with no RAII guard at all, + relying entirely on `daemon stop` succeeding to reap the process. Every other live test file + already used `LiveFirefox` (verified live: its `Drop` is reliable even through a panic), so the + fix is to make `launch_headless()` return a `LiveFirefox` too + (`LiveFirefox::headless_on_random_port_with_args`), keeping the existing `daemon stop` assertion + as the happy path and the guard's `Drop` as the belt-and-suspenders fallback on any failure or + panic between launch and stop. `live_146_no_orphan_firefox_after_suite` and + `live_146_harness_teardown_kills_daemon_spawned_firefox` (new, + `crates/ff-rdp-cli/tests/live/live_146_suite_reliability.rs`) pin the harness-wide guarantee this + depends on. +- **Theme B**: fixing Theme A removes the stale-owner scenario, but the precondition itself was + also silently weaker than it looked — it only skipped on `daemon status` reporting `running: + true`, which stays `false` for a Firefox launched via `ff-rdp launch` that never triggered daemon + autostart. `live_owned_profile_dirs()` replaces that with a direct scan of + `ff-rdp-profile-*` dirs' owner-PID marker files, asserting none are live before pruning and + naming the offending `(dir, pid)` pairs on violation instead of a bare `left: 1 / right: 0`. +- **Theme C**: root-caused live, confirmed **not** a daemon restart (`uptime_seconds` stayed + continuous across a failing run's whole session). Two independent bugs in + `crates/ff-rdp-cli/src/daemon/server.rs` combined to strand `target_count`/`live_target_count` at + 0 or 1 forever: + 1. `establish_watcher`'s synchronous `watchTargets` handshake ran with no `RdpTransport` event + sink installed, so a `target-available-form` catch-up event racing ahead of its RPC reply was + silently dropped by `forward_event`. Fixed by installing an early `mpsc` sink before both the + startup (`run_daemon`) and background-retry (`background_establish_watcher_loop`) handshakes + and replaying whatever it captured into the real event channel afterward, in wire order. + 2. A freshly-launched profile's very first tab is a placeholder `about:blank` + `WindowGlobalTarget` that Firefox tears down within microseconds of being observed, + independent of any navigation. Subscribing to it before it settles orphans the watcher for the + rest of the session — no replacement `target-available-form` ever arrives. Fixed by + `WATCHER_SETTLE_DELAY` (350 ms, comfortably inside `WATCHER_STARTUP_RETRY`'s 2.5 s budget): a + one-time wait before `establish_watcher_with_retry`'s first subscribe attempt. + + `live_146_daemon_parity_stable_repeat` (new, same file) locks this in with 5 consecutive + fresh-daemon launch→navigate→`wait_for_live_targets` runs of the exact shape iter-137 introduced. + No product-facing restart-observability signal was added (AC 5) because the confirmed mechanism + made it moot — see that AC's deferral note. + +## Acceptance Criteria [5/5] + +- [x] live_146_no_orphan_firefox_after_suite: after 3 sequential `LiveFirefox` (+ `with_daemon`) launches modeling a suite run, `wait_until_dead` confirms zero surviving Firefox PIDs once every guard has dropped +- [x] live_146_harness_teardown_kills_daemon_spawned_firefox: a daemon-backed `LiveFirefox` whose test body panics still leaves zero surviving Firefox PIDs after unwind, confirmed via `wait_until_dead` +- [x] live_profiles_prune_removes_all_when_no_firefox_running: the precondition is now an explicit `live_owned_profile_dirs` assertion (no live-owned `ff-rdp-profile-*` dir) that names the offending PIDs on violation, replacing the old `daemon status`-only skip check that went blind to a directly-launched (non-daemon) Firefox +- [x] live_146_daemon_parity_stable_repeat: 5 consecutive fresh-daemon launch→navigate→`wait_for_live_targets` runs all observe at least one live frame target via `daemon status`, backed by the `WATCHER_SETTLE_DELAY` + early-event-sink fix in `daemon/server.rs` +- [x] live_146_daemon_restart_observable: [deferred — not applicable: root-caused live as an event-sink race plus a placeholder-target settle race, not a daemon restart — `daemon status`'s `uptime_seconds` stayed continuous across every failing run captured, so no restart-distinguishing signal is needed] ## Notes