diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index f74b21f3..9a319d24 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -753,6 +753,7 @@ These exist for staged rollouts and the launcher wrappers. They are **internal** | `SOCKET_UPDATE_STATE_DIR` | Overrides the per-user dir holding `update-check.json` + `update.lock` (tests point it into a tempdir). | | `SOCKET_UPDATE_TIMEOUT_MS` | Caps the update fetches' connect/metadata/download budgets (defaults 10 s / 30 s / 300 s; the notice's fetch defaults to 2 s). Doubles as the slow-network escape hatch. | | `SOCKET_UPDATE_NOTIFIER_FORCE` | Test hook: bypasses the update notice's stderr-TTY guard — and nothing else (opt-out, offline, `--silent`, `--json`, CI all still win). | +| `SOCKET_UPDATE_GRACE_MS` | Test hook: overrides the notice's post-command join grace (default 500 ms — how long the run waits for the background check before abandoning it and exiting). Lets the e2e suite await the loopback fetch to completion so its observable effect is deterministic; production keeps the tight 500 ms ceiling. | ### Deprecated env vars diff --git a/crates/socket-patch-cli/src/update_notifier.rs b/crates/socket-patch-cli/src/update_notifier.rs index f6be28c7..1ecc22b0 100644 --- a/crates/socket-patch-cli/src/update_notifier.rs +++ b/crates/socket-patch-cli/src/update_notifier.rs @@ -264,6 +264,38 @@ fn format_notice( ) } +const DEFAULT_GRACE_MS: u64 = 500; + +/// How long `finish` waits for the background fetch before abandoning it. +/// +/// 500 ms in production — deliberately tight so a real command never stalls +/// on the notifier. The catch: `main` calls `std::process::exit` the instant +/// `finish` returns, which kills a still-running detached fetch before its +/// state write (or even its outbound request) can land. On a fast host the +/// loopback fetch finishes in milliseconds and comfortably beats the ceiling; +/// on a slow one (a loaded Windows CI runner, fsync latency, a cold TLS/HTTP +/// client) the fetch can miss the 500 ms window, the task is killed, and its +/// `latestSeen` write / `expect`-counted request simply never happens — an +/// e2e that asserts on that observable effect then fails intermittently. +/// +/// `SOCKET_UPDATE_GRACE_MS` lets the e2e suite lift the ceiling so the fetch +/// is awaited to completion instead of raced. Same shape as the +/// `SOCKET_UPDATE_TIMEOUT_MS` fetch-budget hook; the default is preserved, so +/// production behavior is byte-identical. +fn grace_budget() -> Duration { + grace_budget_from(std::env::var("SOCKET_UPDATE_GRACE_MS").ok().as_deref()) +} + +/// Pure core of [`grace_budget`]: parse the raw env value, falling back to +/// the production default on absence, emptiness, or garbage. +fn grace_budget_from(raw: Option<&str>) -> Duration { + let ms = raw + .filter(|v| !v.is_empty()) + .and_then(|v| v.parse::().ok()) + .unwrap_or(DEFAULT_GRACE_MS); + Duration::from_millis(ms) +} + /// Join the background fetch within the grace budget and print the notice /// if one is warranted. Runs after all command output; never touches /// stdout or the exit code. @@ -273,7 +305,7 @@ pub async fn finish(notifier: Option) { }; let fetched = match notifier.task { Some(handle) => { - match tokio::time::timeout(Duration::from_millis(500), handle).await { + match tokio::time::timeout(grace_budget(), handle).await { Ok(Ok(result)) => result, // Timed out (the task keeps running until process exit — // its own state write may still land) or panicked; either @@ -407,6 +439,18 @@ mod tests { } } + #[test] + fn grace_budget_defaults_preserved_and_override_honored() { + // Absence, emptiness, and garbage all keep the tight production + // ceiling — the override never silently changes shipped behavior. + assert_eq!(grace_budget_from(None), Duration::from_millis(500)); + assert_eq!(grace_budget_from(Some("")), Duration::from_millis(500)); + assert_eq!(grace_budget_from(Some("not-a-number")), Duration::from_millis(500)); + // A valid value lifts the ceiling (the e2e suite's escape hatch). + assert_eq!(grace_budget_from(Some("30000")), Duration::from_millis(30_000)); + assert_eq!(grace_budget_from(Some("0")), Duration::from_millis(0)); + } + #[test] fn notice_names_versions_hint_and_optout() { let current = semver::Version::new(3, 3, 0); diff --git a/crates/socket-patch-cli/tests/update_notifier_e2e.rs b/crates/socket-patch-cli/tests/update_notifier_e2e.rs index 8e887de5..09cbfdd0 100644 --- a/crates/socket-patch-cli/tests/update_notifier_e2e.rs +++ b/crates/socket-patch-cli/tests/update_notifier_e2e.rs @@ -87,6 +87,29 @@ fn eligible_kit(base_url: &str) -> Vec<(&str, &str)> { ] } +/// [`eligible_kit`] plus a lifted join grace, for every row that asserts on +/// the background check's *observable effect* — a rewritten `latestSeen`, a +/// notice on stderr, or a request an `expect_resolves` mock counts. +/// +/// The notifier's fetch runs in a detached task, and the child calls +/// `std::process::exit` the instant the notifier's `finish` returns. Under +/// the production 500 ms grace that is a genuine race: on a fast host the +/// loopback fetch lands its state write / request in a few ms and wins, but +/// on a slow one (a loaded Windows CI runner) it can miss the window, get +/// killed at exit, and never write `latestSeen` or reach the mock at all — +/// the "reads the STALE version" / "expect(1) unmet" Windows flake. Lifting +/// the ceiling via `SOCKET_UPDATE_GRACE_MS` makes `finish` await the fetch to +/// completion instead of racing it, so the effect is deterministic. It does +/// NOT weaken any assertion: the fetch still completes in milliseconds, only +/// the artificial cutoff is gone (`grace_budget_bounds_command_latency` +/// keeps the real 500 ms ceiling, since testing that cutoff is its whole +/// point). +fn eligible_kit_await_fetch(base_url: &str) -> Vec<(&str, &str)> { + let mut kit = eligible_kit(base_url); + kit.push(("SOCKET_UPDATE_GRACE_MS", "30000")); + kit +} + /// The notifier must never mutate the install or the project dir, on any /// path — every row re-proves it. fn assert_install_pristine(install: &StagedInstall) { @@ -110,7 +133,7 @@ async fn first_eligible_run_checks_and_notices() { .await; let (code, stdout, stderr) = - run_installed(&install, &["apply"], &eligible_kit(&release.base_url)); + run_installed(&install, &["apply"], &eligible_kit_await_fetch(&release.base_url)); assert_eq!(code, 0, "stdout:\n{stdout}\nstderr:\n{stderr}"); assert!( stderr.contains("Update available") && stderr.contains("9.9.9"), @@ -174,7 +197,7 @@ async fn stale_state_rechecks() { write_state(&install.state_dir, STALE, Some(CURRENT), None); let (code, stdout, stderr) = - run_installed(&install, &["apply"], &eligible_kit(&release.base_url)); + run_installed(&install, &["apply"], &eligible_kit_await_fetch(&release.base_url)); assert_eq!(code, 0, "stdout:\n{stdout}\nstderr:\n{stderr}"); let state = read_state(&install.state_dir); @@ -202,7 +225,8 @@ async fn up_to_date_prints_nothing() { .await; write_state(&install.state_dir, STALE, Some(CURRENT), None); - let (code, _, stderr) = run_installed(&install, &["apply"], &eligible_kit(&release.base_url)); + let (code, _, stderr) = + run_installed(&install, &["apply"], &eligible_kit_await_fetch(&release.base_url)); assert_eq!(code, 0); assert!( !stderr.contains("Update available"), @@ -265,7 +289,7 @@ async fn corrupt_state_recovers() { .unwrap(); let (code, stdout, stderr) = - run_installed(&install, &["apply"], &eligible_kit(&release.base_url)); + run_installed(&install, &["apply"], &eligible_kit_await_fetch(&release.base_url)); assert_eq!(code, 0, "stdout:\n{stdout}\nstderr:\n{stderr}"); assert!( !stderr.contains("panicked"), @@ -293,7 +317,7 @@ async fn future_timestamp_tolerated() { write_state(&install.state_dir, -48 * HOUR, Some("9.9.9"), None); let (code, stdout, stderr) = - run_installed(&install, &["apply"], &eligible_kit(&release.base_url)); + run_installed(&install, &["apply"], &eligible_kit_await_fetch(&release.base_url)); assert_eq!(code, 0, "stdout:\n{stdout}\nstderr:\n{stderr}"); assert_install_pristine(&install); } @@ -636,6 +660,10 @@ mod pty { ("CI", ""), ("GITHUB_ACTIONS", ""), ("SOCKET_UPDATE_BASE_URL", &release.base_url), + // Await the background fetch rather than race the child's exit — + // the notice only appears once the check lands (see + // `eligible_kit_await_fetch`). + ("SOCKET_UPDATE_GRACE_MS", "30000"), ]; let (code, output) = run_in_pty( &install.bin,