From 2afc84ebbe787b2b5a972018443f367f11016492 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 07:22:03 -0500 Subject: [PATCH 1/4] fix(desktop): isolate process tests and close unlisted Apple child descriptors --- .github/workflows/desktop.yml | 18 +- apps/desktop/README.md | 25 +- apps/desktop/TESTING.md | 53 ++++ apps/desktop/core/src/lib.rs | 4 + apps/desktop/core/src/process.rs | 15 ++ apps/desktop/core/src/process/apple.rs | 331 +++++++++++++++++++++++++ apps/desktop/core/src/runner.rs | 12 +- apps/desktop/core/src/source.rs | 21 +- apps/desktop/core/src/test_support.rs | 19 ++ apps/desktop/core/tests/dump_state.rs | 21 +- apps/desktop/scripts/test.sh | 27 ++ 11 files changed, 511 insertions(+), 35 deletions(-) create mode 100644 apps/desktop/TESTING.md create mode 100644 apps/desktop/core/src/process.rs create mode 100644 apps/desktop/core/src/process/apple.rs create mode 100644 apps/desktop/core/src/test_support.rs create mode 100644 apps/desktop/scripts/test.sh diff --git a/.github/workflows/desktop.yml b/.github/workflows/desktop.yml index 6bfd362b3..383ef6029 100644 --- a/.github/workflows/desktop.yml +++ b/.github/workflows/desktop.yml @@ -1,6 +1,6 @@ # Hack desktop app (HACK-2214): unsupported preview. # Separate from the default gates and the release workflow, and not a -# required check. Linux and macOS builds of the app plus the `core` tests. +# required check. Linux and macOS builds plus isolated workspace tests. # The toolchain comes from apps/desktop/rust-toolchain.toml (1.98.1; macOS # needs it), not from the repo root pin. name: Desktop app @@ -10,11 +10,15 @@ on: paths: - "apps/desktop/**" - ".github/workflows/desktop.yml" + - "scripts/install-cargo-nextest.sh" + - ".config/nextest.toml" push: branches: [next] paths: - "apps/desktop/**" - ".github/workflows/desktop.yml" + - "scripts/install-cargo-nextest.sh" + - ".config/nextest.toml" permissions: contents: read @@ -52,10 +56,14 @@ jobs: restore-keys: desktop-${{ runner.os }}- - name: Format run: cargo fmt --all --check + - name: Install pinned per-test process runner + run: | + bash ../../scripts/install-cargo-nextest.sh "$RUNNER_TEMP/desktop-nextest" + echo "$RUNNER_TEMP/desktop-nextest" >> "$GITHUB_PATH" - name: Core tests (no daemon, no Docker) env: HACK_DAEMON_AUTOSTART_DISABLE: "1" - run: cargo test --locked -p hack-desktop-core + run: bash scripts/test.sh # Both: the screenshots feature turns on gpui test-support, which can # hide test-only APIs from a build that only uses that feature. - name: Clippy (default features) @@ -85,10 +93,14 @@ jobs: apps/desktop/target key: desktop-${{ runner.os }}-${{ hashFiles('apps/desktop/Cargo.lock') }} restore-keys: desktop-${{ runner.os }}- + - name: Install pinned per-test process runner + run: | + bash ../../scripts/install-cargo-nextest.sh "$RUNNER_TEMP/desktop-nextest" + echo "$RUNNER_TEMP/desktop-nextest" >> "$GITHUB_PATH" - name: Core tests (no daemon, no Docker) env: HACK_DAEMON_AUTOSTART_DISABLE: "1" - run: cargo test --locked -p hack-desktop-core + run: bash scripts/test.sh - name: Clippy (default features) run: cargo clippy --locked --all-targets -- -D warnings - name: Build app diff --git a/apps/desktop/README.md b/apps/desktop/README.md index 03fe99700..96ab408d7 100644 --- a/apps/desktop/README.md +++ b/apps/desktop/README.md @@ -25,7 +25,7 @@ libvulkan-dev`. ```sh cd apps/desktop -cargo test -p hack-desktop-core # no daemon, Docker or Hack home needed +bash scripts/test.sh # pinned nextest, separate process per test cargo run -p hack-desktop # live: matching hackd; otherwise needs R18 cargo run -p hack-desktop -- --list-fixtures cargo run -p hack-desktop -- --fixtures failed-start --theme dark @@ -81,3 +81,26 @@ degraded / stopping, failed-start, needs-attention): Logs, Env and Activity tabs, CPU and memory, Doctor view, onboarding, always-on, daemon actions and upgrades come in later units. The design and the CLI/daemon requests they need are in `DESIGN.md` (PR #800). + +The supported desktop test command uses the repository-pinned nextest release +(installed by `scripts/install-cargo-nextest.sh` at the repository root), with +zero retries and one process per test. Raw threaded `cargo test --workspace` is +not the macOS qualification command: fresh executable writes, concurrent process +creation, and short socket deadlines can interact across libtest threads. + +On macOS, the CLI runner uses `posix_spawn` with `POSIX_SPAWN_CLOEXEC_DEFAULT` +and explicit stdin/stdout/stderr actions. This prevents a CLI child from holding +an unrelated descriptor during another thread's non-atomic close-on-exec setup. +The process group, selected environment, cwd, timeout and retirement contracts +are preserved. Linux retains the standard process implementation. This product +boundary is needed independently of test isolation; a mutex around runner calls +would not cover descriptors opened by other GUI/library threads. + +For the macOS qualification loop, run +`HACK_DESKTOP_TEST_REPETITIONS=200 bash scripts/test.sh` from this directory. +Each iteration runs all workspace tests in separate processes with zero retries; +doctests run afterward. Both CI hosts use this same wrapper (one iteration per +job). Three consecutive green hosted macOS jobs and the lead's 200-run native +receipt are required for HACK-2243 acceptance; a Linux run or cross-check is not +that evidence. #843's independent generous retirement deadlines remain necessary +and should land before the final combined qualification. diff --git a/apps/desktop/TESTING.md b/apps/desktop/TESTING.md new file mode 100644 index 000000000..e31c5ac08 --- /dev/null +++ b/apps/desktop/TESTING.md @@ -0,0 +1,53 @@ +# Desktop process tests + +Run `bash scripts/test.sh` from `apps/desktop`. It checks the exact nextest pin +from the repository installer, runs the workspace tests in separate processes +with zero retries, then runs doctests. Each invocation creates and removes its +own HOME/HACK_HOME. Both desktop CI jobs use this command. To qualify a native +Mac, run `HACK_DESKTOP_TEST_REPETITIONS=200 bash scripts/test.sh`; preserve the +complete output and exact commit. Three consecutive green hosted macOS desktop +jobs are a separate acceptance gate. + +The default threaded libtest runner is not the qualification route. Tests can +write executables and create descriptors concurrently within a single process; +Rust 1.98.1 on Apple creates a pipe and sets close-on-exec in later calls. Linux +uses atomic `pipe2(O_CLOEXEC)`. Separate test processes prevent one test's open +fixture writer or pipe-creation window from being inherited by another test's +child. Executable fixtures are also written to a private pending filename, +flushed, closed, marked executable, then renamed into place before use. Existing +first-launch warmups remain outside timed observations. + +## Reported symptoms and what they prove + +| Symptom | Interpretation and coverage | +| --- | --- | +| Fake CLI `NotFound` | The runner received a spawn ENOENT, not proof that the fixture pathname was absent. An unavailable script interpreter or a transient executable-launch failure can also produce ENOENT. The precise origin in the reported Mac runs was not captured; do not label it proven descriptor leakage. Separate test processes and closed-before-publish scripts remove cross-test writer inheritance. | +| Fake CLI never started | No ready marker was written before a product timeout. This can reflect startup scheduling or failed launch. Warmups, process isolation and the ready marker distinguish startup from tested cleanup; the timeout contract is unchanged. | +| Fake daemon `WouldBlock` (Darwin error 35) | The socket read deadline expired before the request arrived. This is a socket scheduling symptom, not a spawn ENOENT. Version warmups precede the server's accept deadline; per-test processes remove cross-test descriptor interference. | +| Slow stream Drop / late pipe EOF | An inherited pipe writer can keep a reader alive after the intended writer exits. The Apple product boundary below closes unlisted descriptors. #843 separately replaces the tight elapsed-time oracle with reader completion and process retirement under a generous deadline. | + +These are evidence boundaries, not a claim that every observed failure has one +proven cause. Linux checks and Apple-target compilation do not establish Mac +reliability. The 200-run native result and three hosted results must be recorded +before HACK-2243 acceptance. + +## Product spawning + +Test isolation alone cannot protect the multi-threaded application. The Apple +CLI runner uses `posix_spawn` with `POSIX_SPAWN_CLOEXEC_DEFAULT`, explicitly +installing only null stdin and the two output pipes, a fresh process group, cwd, +and the authored environment overrides. It preserves the normal SIGPIPE default +and inherited signal mask. Pipe/action/attribute ownership uses RAII; waiting +reaps the owned child and caches its status. Linux keeps standard `Command`. + +A runner-only mutex is insufficient because GPUI or another library can open a +descriptor outside it. The native regression deliberately holds an inheritable +high-numbered descriptor open: the standard-spawn control must see it; the app's +spawn must not. A second native regression checks argv, environment, cwd, null +stdin, separate stderr and process-group identity. They use only fixed system +commands and owned fixtures, with no daemon, engine, services or network. + +The helper is an internal CLI-runner boundary, not a general replacement for +`Command`: it implements the runner's fixed null/piped stdio and process-group +contract. If a future runner adds environment clearing or other Command options, +it must carry those explicitly into this boundary as well. diff --git a/apps/desktop/core/src/lib.rs b/apps/desktop/core/src/lib.rs index 346879338..d28e0f3c7 100644 --- a/apps/desktop/core/src/lib.rs +++ b/apps/desktop/core/src/lib.rs @@ -13,7 +13,11 @@ pub mod daemon; pub mod fixtures; pub mod models; +mod process; pub mod runner; pub mod source; pub mod state; pub mod viewmodel; + +#[cfg(test)] +mod test_support; diff --git a/apps/desktop/core/src/process.rs b/apps/desktop/core/src/process.rs new file mode 100644 index 000000000..e2b5b050d --- /dev/null +++ b/apps/desktop/core/src/process.rs @@ -0,0 +1,15 @@ +//! Process creation boundary. Apple children close all unlisted descriptors, +//! including descriptors another thread has not yet marked close-on-exec. + +#[cfg(not(target_os = "macos"))] +pub(crate) use std::process::Child; + +#[cfg(not(target_os = "macos"))] +pub(crate) fn spawn(command: &mut std::process::Command) -> std::io::Result { + command.spawn() +} + +#[cfg(target_os = "macos")] +mod apple; +#[cfg(target_os = "macos")] +pub(crate) use apple::{Child, spawn}; diff --git a/apps/desktop/core/src/process/apple.rs b/apps/desktop/core/src/process/apple.rs new file mode 100644 index 000000000..5c1a10858 --- /dev/null +++ b/apps/desktop/core/src/process/apple.rs @@ -0,0 +1,331 @@ +//! `Command` cannot request Apple's CLOEXEC_DEFAULT spawn attribute. Use +//! posix_spawn with explicit stdio actions; a runner-only mutex would not +//! protect pipes/files opened by GPUI or other libraries on other threads. + +use std::collections::BTreeMap; +use std::ffi::{CString, OsString}; +use std::fs::File; +use std::io; +use std::mem::MaybeUninit; +use std::os::fd::{AsRawFd, FromRawFd}; +use std::os::unix::ffi::OsStrExt; +use std::os::unix::process::ExitStatusExt; +use std::process::{Command, ExitStatus}; + +fn check(code: libc::c_int) -> io::Result<()> { + if code == 0 { + Ok(()) + } else { + Err(io::Error::from_raw_os_error(code)) + } +} + +fn cstring(value: &std::ffi::OsStr) -> io::Result { + CString::new(value.as_bytes()) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "NUL in process argument")) +} + +struct Actions(libc::posix_spawn_file_actions_t); +impl Drop for Actions { + fn drop(&mut self) { + // SAFETY: initialized by posix_spawn_file_actions_init, owned once. + unsafe { + libc::posix_spawn_file_actions_destroy(&mut self.0); + } + } +} +struct Attributes(libc::posix_spawnattr_t); +impl Drop for Attributes { + fn drop(&mut self) { + // SAFETY: initialized by posix_spawnattr_init, owned once. + unsafe { + libc::posix_spawnattr_destroy(&mut self.0); + } + } +} + +fn pipe() -> io::Result<(File, File)> { + let mut fds = [-1; 2]; + // SAFETY: fds holds two writable descriptors; successful pipe transfers + // ownership. CLOEXEC_DEFAULT prevents inheritance during the setup window. + if unsafe { libc::pipe(fds.as_mut_ptr()) } != 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: each successful pipe descriptor is uniquely owned here. + let original = unsafe { (File::from_raw_fd(fds[0]), File::from_raw_fd(fds[1])) }; + let duplicate = |file: &File| { + // Keep action sources above stdio even when the parent has closed 0–2. + // SAFETY: file is live; returned descriptor has independent ownership. + let fd = unsafe { libc::fcntl(file.as_raw_fd(), libc::F_DUPFD_CLOEXEC, 3) }; + if fd < 0 { + Err(io::Error::last_os_error()) + } else { + // SAFETY: fcntl returned a new owned descriptor. + Ok(unsafe { File::from_raw_fd(fd) }) + } + }; + Ok((duplicate(&original.0)?, duplicate(&original.1)?)) +} + +pub(crate) struct Child { + pid: libc::pid_t, + status: Option, + pub(crate) stdout: Option, + pub(crate) stderr: Option, +} +impl Child { + pub(crate) fn id(&self) -> u32 { + self.pid as u32 + } + pub(crate) fn try_wait(&mut self) -> io::Result> { + self.wait_with(libc::WNOHANG) + } + pub(crate) fn wait(&mut self) -> io::Result { + self.wait_with(0)? + .ok_or_else(|| io::Error::other("waitpid returned no status")) + } + fn wait_with(&mut self, options: libc::c_int) -> io::Result> { + if self.status.is_some() { + return Ok(self.status); + } + loop { + let mut status = 0; + // SAFETY: pid is our unreaped child and status is writable. + let result = unsafe { libc::waitpid(self.pid, &mut status, options) }; + if result == self.pid { + self.status = Some(ExitStatus::from_raw(status)); + return Ok(self.status); + } + if result == 0 { + return Ok(None); + } + let error = io::Error::last_os_error(); + if error.kind() != io::ErrorKind::Interrupted { + return Err(error); + } + } + } + pub(crate) fn kill(&mut self) -> io::Result<()> { + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "child already reaped", + )); + } + // SAFETY: pid is the child this object owns, not an arbitrary input. + if unsafe { libc::kill(self.pid, libc::SIGKILL) } == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } + } +} + +/// The runner always supplies null stdin, piped outputs, and its own process +/// group. Environment overrides/removals and cwd come from the authored Command. +/// This boundary currently inherits the parent environment like CliRunner. +pub(crate) fn spawn(command: &mut Command) -> io::Result { + let program = cstring(command.get_program())?; + let args: Vec = std::iter::once(command.get_program()) + .chain(command.get_args()) + .map(cstring) + .collect::>()?; + let mut environment: BTreeMap = std::env::vars_os().collect(); + for (key, value) in command.get_envs() { + if let Some(value) = value { + environment.insert(key.into(), value.into()); + } else { + environment.remove(key); + } + } + let environment: Vec = environment + .into_iter() + .map(|(key, value)| { + let mut assignment = key; + assignment.push("="); + assignment.push(value); + cstring(&assignment) + }) + .collect::>()?; + let cwd = command + .get_current_dir() + .map(|p| cstring(p.as_os_str())) + .transpose()?; + let mut argv: Vec<*mut libc::c_char> = args.iter().map(|s| s.as_ptr().cast_mut()).collect(); + argv.push(std::ptr::null_mut()); + let mut envp: Vec<*mut libc::c_char> = + environment.iter().map(|s| s.as_ptr().cast_mut()).collect(); + envp.push(std::ptr::null_mut()); + let (out, out_write) = pipe()?; + let (err, err_write) = pipe()?; + let null = CString::new("/dev/null").expect("fixed path"); + // SAFETY: the init calls initialize the opaque values before use. The + // RAII wrappers destroy them on all later errors. All pointer buffers + // and descriptor sources live through the synchronous spawn call. + unsafe { + let mut actions = MaybeUninit::uninit(); + check(libc::posix_spawn_file_actions_init(actions.as_mut_ptr()))?; + let mut actions = Actions(actions.assume_init()); + let mut attributes = MaybeUninit::uninit(); + check(libc::posix_spawnattr_init(attributes.as_mut_ptr()))?; + let mut attributes = Attributes(attributes.assume_init()); + check(libc::posix_spawn_file_actions_addopen( + &mut actions.0, + 0, + null.as_ptr(), + libc::O_RDONLY, + 0, + ))?; + for (fd, target) in [(out_write.as_raw_fd(), 1), (err_write.as_raw_fd(), 2)] { + check(libc::posix_spawn_file_actions_adddup2( + &mut actions.0, + fd, + target, + ))?; + check(libc::posix_spawn_file_actions_addclose(&mut actions.0, fd))?; + } + if let Some(cwd) = &cwd { + check(libc::posix_spawn_file_actions_addchdir_np( + &mut actions.0, + cwd.as_ptr(), + ))?; + } + let mut defaults = MaybeUninit::::uninit(); + if libc::sigemptyset(defaults.as_mut_ptr()) != 0 { + return Err(io::Error::last_os_error()); + } + let mut defaults = defaults.assume_init(); + if libc::sigaddset(&mut defaults, libc::SIGPIPE) != 0 { + return Err(io::Error::last_os_error()); + } + check(libc::posix_spawnattr_setsigdefault( + &mut attributes.0, + &defaults, + ))?; + check(libc::posix_spawnattr_setflags( + &mut attributes.0, + (libc::POSIX_SPAWN_CLOEXEC_DEFAULT + | libc::POSIX_SPAWN_SETPGROUP + | libc::POSIX_SPAWN_SETSIGDEF) as _, + ))?; + check(libc::posix_spawnattr_setpgroup(&mut attributes.0, 0))?; + let mut pid = 0; + check(libc::posix_spawnp( + &mut pid, + program.as_ptr(), + &actions.0, + &attributes.0, + argv.as_ptr(), + envp.as_ptr(), + ))?; + Ok(Child { + pid, + status: None, + stdout: Some(out), + stderr: Some(err), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Read; + + #[test] + fn unrelated_inheritable_descriptor_is_closed_in_child() { + let file = File::open("/dev/null").unwrap(); + // Intentionally model another thread between opening an fd and + // setting CLOEXEC. A high fd avoids incidental stdio reuse. + // SAFETY: file is live; F_DUPFD returns an independently owned fd. + let fd = unsafe { libc::fcntl(file.as_raw_fd(), libc::F_DUPFD, 200) }; + assert!(fd >= 200); + // SAFETY: fd is a new owned descriptor, retained through spawn. + let inherited = unsafe { File::from_raw_fd(fd) }; + // SAFETY: the descriptor remains live and fcntl only inspects it. + assert_eq!( + unsafe { libc::fcntl(fd, libc::F_GETFD) } & libc::FD_CLOEXEC, + 0 + ); + let mut command = Command::new("/bin/sh"); + command.args([ + "-c", + &format!("if [ -e /dev/fd/{fd} ]; then printf inherited; else printf closed; fi"), + ]); + let control = command.output().unwrap(); + assert!(control.status.success()); + assert_eq!( + control.stdout, b"inherited", + "probe detects the old boundary" + ); + let mut child = spawn(&mut command).unwrap(); + let mut output = String::new(); + child + .stdout + .take() + .unwrap() + .read_to_string(&mut output) + .unwrap(); + assert!(child.wait().unwrap().success()); + assert_eq!(output, "closed"); + assert!( + inherited.metadata().is_ok(), + "parent still owns the descriptor" + ); + } +} + +#[cfg(test)] +mod contract_tests { + use super::*; + use std::io::Read; + use std::os::unix::fs::DirBuilderExt; + + #[test] + fn stdio_arguments_environment_cwd_and_process_group_are_preserved() { + let dir = std::env::temp_dir().join(format!( + "desktop-spawn-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::DirBuilder::new().mode(0o700).create(&dir).unwrap(); + let canonical = dir.canonicalize().unwrap(); + let mut command = Command::new("/bin/sh"); + command.current_dir(&canonical).env("HACK_DESKTOP_PROCESS_TEST", "checked value").args([ + "-c", "printf '%s\\n' \"$1\" \"$HACK_DESKTOP_PROCESS_TEST\" \"$PWD\"; if read -r unused; then printf unexpected; else printf 'eof\\n'; fi; /bin/ps -o pgid= -p $$; printf diagnostic >&2", + "fixture", "space and ' quote", + ]); + let mut child = spawn(&mut command).unwrap(); + let mut stdout = String::new(); + let mut stderr = String::new(); + child + .stdout + .take() + .unwrap() + .read_to_string(&mut stdout) + .unwrap(); + child + .stderr + .take() + .unwrap() + .read_to_string(&mut stderr) + .unwrap(); + assert!(child.wait().unwrap().success()); + let lines: Vec<_> = stdout.lines().collect(); + assert_eq!( + lines[..4], + [ + "space and ' quote", + "checked value", + canonical.to_str().unwrap(), + "eof" + ] + ); + assert_eq!(lines[4].trim().parse::().unwrap(), child.id()); + assert_eq!(stderr, "diagnostic"); + std::fs::remove_dir_all(dir).unwrap(); + } +} diff --git a/apps/desktop/core/src/runner.rs b/apps/desktop/core/src/runner.rs index e41886c2a..8f92e5dd4 100644 --- a/apps/desktop/core/src/runner.rs +++ b/apps/desktop/core/src/runner.rs @@ -8,9 +8,10 @@ //! - stderr is kept (last 8 KB) only for error messages; it is never parsed. //! - `HACK_NO_INTERACTIVE=1` is always set: the app never answers prompts. +use crate::process::Child; use std::io::{BufRead, BufReader, Read}; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; +use std::process::{Command, Stdio}; use std::sync::mpsc; use std::thread; use std::time::{Duration, Instant}; @@ -155,7 +156,7 @@ impl CliRunner { // briefly instead of reporting a spawn failure. let mut attempt = 0; loop { - match self.command(args, path).spawn() { + match crate::process::spawn(&mut self.command(args, path)) { Ok(c) => return Ok(c), Err(e) if e.raw_os_error() == Some(libc::ETXTBSY) && attempt < 20 => { attempt += 1; @@ -547,7 +548,6 @@ pub fn parse_version(text: &str) -> Option { #[cfg(test)] mod tests { use super::*; - use std::os::unix::fs::PermissionsExt; /// A fake `hack` in a fresh temp dir: a shell script with a fixed body. struct FakeHack { @@ -563,14 +563,12 @@ mod tests { )); std::fs::create_dir_all(&dir).unwrap(); let p = dir.join("hack"); - std::fs::write( + crate::test_support::script( &p, format!( "#!/bin/sh\nif [ \"$1\" = __warmup ]; then echo '{{}}'; exit 0; fi\n{body}\n" ), - ) - .unwrap(); - std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o755)).unwrap(); + ); let fake = Self { dir }; // macOS may scan a newly written executable on first launch. Keep // that delay outside every test's product-timeout measurement. diff --git a/apps/desktop/core/src/source.rs b/apps/desktop/core/src/source.rs index 1c97ea5f0..d7fbc10c3 100644 --- a/apps/desktop/core/src/source.rs +++ b/apps/desktop/core/src/source.rs @@ -555,7 +555,6 @@ mod tests { /// A trusted fake daemon provides rows; only native ps uses the fake CLI. #[test] fn live_source_reads_native_rows_with_native_backend() { - use std::os::unix::fs::PermissionsExt; let dir = std::env::temp_dir().join(format!( "hack-desktop-source-{}-{}", std::process::id(), @@ -566,7 +565,7 @@ mod tests { )); std::fs::create_dir_all(&dir).unwrap(); let hack = dir.join("hack"); - std::fs::write( + crate::test_support::script( &hack, r#"#!/bin/sh case "$1" in @@ -575,9 +574,7 @@ case "$1" in ps) if [ "$HACK_RUNTIME_BACKEND" = native ]; then echo '{"ok":true,"data":{"backend":"native","status":"pending","items":[]}}'; else echo '{"ok":false,"error":{"code":"E_NATIVE_PROJECT_UNSUPPORTED","message":"m"}}'; exit 1; fi ;; esac "#, - ) - .unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o755)).unwrap(); + ); // Complete macOS first-launch scanning before the fake daemon's // bounded accept window starts. --version has no fixture effects. CliRunner::new(&hack) @@ -616,7 +613,6 @@ esac #[test] fn live_source_rejects_a_payload_without_projects() { - use std::os::unix::fs::PermissionsExt; let dir = std::env::temp_dir().join(format!( "hack-desktop-source-empty-{}-{}", std::process::id(), @@ -627,12 +623,10 @@ esac )); std::fs::create_dir_all(&dir).unwrap(); let hack = dir.join("hack"); - std::fs::write( + crate::test_support::script( &hack, "#!/bin/sh\ncase \"$1\" in --version) echo 'hack v5.0.0' ;; projects) echo '{}' ;; esac\n", - ) - .unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o755)).unwrap(); + ); CliRunner::new(&hack) .version(Duration::from_secs(60)) .unwrap(); @@ -822,17 +816,14 @@ esac fn lifecycle_actions_run_to_completion_without_an_app_deadline() { // A Start slower than any read timeout still completes; Open keeps // its bounded limit. (The CLI owns lifecycle timeouts.) - use std::os::unix::fs::PermissionsExt; let b = base("lifecycle"); let p = b.join("p"); make_project(&p, ".hack", "docker-compose.yml"); let hack = b.join("hack"); - std::fs::write( + crate::test_support::script( &hack, "#!/bin/sh\nsleep 1\necho '{\"ok\":true,\"data\":{\"action\":\"up\",\"services\":{\"failed\":[]}}}'\n", - ) - .unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o755)).unwrap(); + ); let src = LiveSource { runner: CliRunner::new(&hack), daemon: None, diff --git a/apps/desktop/core/src/test_support.rs b/apps/desktop/core/src/test_support.rs new file mode 100644 index 000000000..b09978ba9 --- /dev/null +++ b/apps/desktop/core/src/test_support.rs @@ -0,0 +1,19 @@ +//! Publish executable fixtures only after their writer is closed. +use std::io::Write; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::path::Path; + +pub fn script(path: &Path, body: impl AsRef<[u8]>) { + let pending = path.with_extension("pending"); + let mut writer = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&pending) + .unwrap(); + writer.write_all(body.as_ref()).unwrap(); + writer.sync_all().unwrap(); + drop(writer); + std::fs::set_permissions(&pending, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::rename(pending, path).unwrap(); +} diff --git a/apps/desktop/core/tests/dump_state.rs b/apps/desktop/core/tests/dump_state.rs index 79135841f..6f62a9a04 100644 --- a/apps/desktop/core/tests/dump_state.rs +++ b/apps/desktop/core/tests/dump_state.rs @@ -2,7 +2,8 @@ //! and a private fake daemon, and checks the JSON. //! Hermetic: no real Hack home, daemon, Docker or network. -use std::os::unix::fs::PermissionsExt; +#[path = "../src/test_support.rs"] +mod test_support; use std::path::PathBuf; use std::process::Command; @@ -69,7 +70,7 @@ fn dump_state_reads_the_cli_like_the_app_and_prints_view_models() { let bin = temp_dir("bin"); let home = temp_dir("home"); let hack = bin.join("hack"); - std::fs::write( + test_support::script( &hack, r#"#!/bin/sh case "$1" in @@ -78,9 +79,7 @@ case "$1" in *) echo "unexpected: $*" >&2; exit 2 ;; esac "#, - ) - .unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o755)).unwrap(); + ); // Keep macOS first-launch scanning outside the fake daemon's accept // deadline, using the same bounded runner as the application. hack_desktop_core::runner::CliRunner::new(&hack) @@ -145,8 +144,10 @@ fn dump_state_without_hack_reports_the_missing_cli() { fn missing_daemon_never_runs_the_registry_writing_cli_listing() { let home = temp_dir("observe"); let hack = home.join("hack"); - std::fs::write(&hack, "#!/bin/sh\ncase \"$1\" in --version) echo 'hack v5.0.0';; *) echo called > \"$HACK_HOME/unexpected\"; exit 1;; esac\n").unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o700)).unwrap(); + test_support::script( + &hack, + "#!/bin/sh\ncase \"$1\" in --version) echo 'hack v5.0.0';; *) echo called > \"$HACK_HOME/unexpected\"; exit 1;; esac\n", + ); let out = Command::new(env!("CARGO_BIN_EXE_hack-desktop-state")) .env("PATH", &home) .env("HOME", &home) @@ -174,8 +175,10 @@ fn daemon_recovery_runs_only_the_requested_command_and_checks_its_exit() { use hack_desktop_core::source::{DaemonRecovery, LiveSource, Source}; let home = temp_dir("recover"); let hack = home.join("hack"); - std::fs::write(&hack, "#!/bin/sh\nprintf '%s' \"$*\" > \"$HACK_HOME/called\"\nif [ \"$2\" = restart ]; then echo 'daemon could not restart' >&2; exit 1; fi\n").unwrap(); - std::fs::set_permissions(&hack, std::fs::Permissions::from_mode(0o700)).unwrap(); + test_support::script( + &hack, + "#!/bin/sh\nprintf '%s' \"$*\" > \"$HACK_HOME/called\"\nif [ \"$2\" = restart ]; then echo 'daemon could not restart' >&2; exit 1; fi\n", + ); let source = LiveSource { runner: CliRunner::new(hack) .with_env("HOME", home.to_str().unwrap()) diff --git a/apps/desktop/scripts/test.sh b/apps/desktop/scripts/test.sh new file mode 100644 index 000000000..f2e4e680f --- /dev/null +++ b/apps/desktop/scripts/test.sh @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# Separate test processes prevent shared libtest fork/write/descriptor races. +set -euo pipefail +root=$(CDPATH= cd -- "$(dirname -- "$0")/../../.." && pwd -P) +cd "$root/apps/desktop" +version=$(bash "$root/scripts/install-cargo-nextest.sh" --print-version) +actual=$(cargo nextest --version 2>/dev/null) || actual="" +case "$actual" in + "cargo-nextest $version"|"cargo-nextest $version ("*) ;; + *) echo "Install pinned cargo-nextest: bash scripts/install-cargo-nextest.sh DESTINATION; add DESTINATION to PATH" >&2; exit 2 ;; +esac +repetitions=${HACK_DESKTOP_TEST_REPETITIONS:-1} +case "$repetitions" in 1|200) ;; *) echo 'HACK_DESKTOP_TEST_REPETITIONS must be 1 or 200.' >&2; exit 2 ;; esac +# Cargo keeps using its configured cache/toolchain; test children get private +# HOME/HACK_HOME. The directory is created by this invocation and removed on exit. +export CARGO_HOME=${CARGO_HOME:-$HOME/.cargo} +export RUSTUP_HOME=${RUSTUP_HOME:-$HOME/.rustup} +owned=$(mktemp -d "${TMPDIR:-/tmp}/hack-desktop-tests.XXXXXXXX") +trap 'rm -rf -- "$owned"' EXIT +mkdir "$owned/home" "$owned/hack" +export HOME="$owned/home" HACK_HOME="$owned/hack" +export HACK_DAEMON_AUTOSTART_DISABLE=1 +for ((attempt = 1; attempt <= repetitions; attempt++)); do + echo "Desktop workspace suite $attempt/$repetitions (no retries)" + cargo nextest run --locked --workspace --build-jobs 2 --config-file "$root/.config/nextest.toml" --profile ci "$@" +done +cargo test --locked --workspace --doc --jobs 2 From 3be0b4fa2308985744449fd3d5259ebdafcb3ea7 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 07:27:04 -0500 Subject: [PATCH 2/4] fix(desktop): clear worker signal masks before Apple CLI execution --- apps/desktop/TESTING.md | 10 ++- apps/desktop/core/src/process/apple.rs | 98 ++++++++++++++++++++++++-- 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/apps/desktop/TESTING.md b/apps/desktop/TESTING.md index e31c5ac08..ef569a467 100644 --- a/apps/desktop/TESTING.md +++ b/apps/desktop/TESTING.md @@ -37,7 +37,7 @@ Test isolation alone cannot protect the multi-threaded application. The Apple CLI runner uses `posix_spawn` with `POSIX_SPAWN_CLOEXEC_DEFAULT`, explicitly installing only null stdin and the two output pipes, a fresh process group, cwd, and the authored environment overrides. It preserves the normal SIGPIPE default -and inherited signal mask. Pipe/action/attribute ownership uses RAII; waiting +and an empty signal mask, so a GPUI worker cannot pass a blocked SIGTERM to the CLI. Pipe/action/attribute ownership uses RAII; waiting reaps the owned child and caches its status. Linux keeps standard `Command`. A runner-only mutex is insufficient because GPUI or another library can open a @@ -51,3 +51,11 @@ The helper is an internal CLI-runner boundary, not a general replacement for `Command`: it implements the runner's fixed null/piped stdio and process-group contract. If a future runner adds environment clearing or other Command options, it must carry those explicitly into this boundary as well. + +The Apple boundary marks initial pipe descriptors close-on-exec immediately, +then promotes sources above standard IO with atomic close-on-exec duplication. +A small initial pipe-to-fcntl window remains for external spawners that do not +use CLOEXEC_DEFAULT; the application boundary cannot change those libraries. +All CLI spawns through this boundary close unlisted descriptors regardless of +that window. Executable resolution uses the captured child PATH and absolute +`posix_spawn`, avoiding libc's process-global PATH lookup. diff --git a/apps/desktop/core/src/process/apple.rs b/apps/desktop/core/src/process/apple.rs index 5c1a10858..5dfaabbeb 100644 --- a/apps/desktop/core/src/process/apple.rs +++ b/apps/desktop/core/src/process/apple.rs @@ -53,6 +53,13 @@ fn pipe() -> io::Result<(File, File)> { } // SAFETY: each successful pipe descriptor is uniquely owned here. let original = unsafe { (File::from_raw_fd(fds[0]), File::from_raw_fd(fds[1])) }; + for file in [&original.0, &original.1] { + // SAFETY: the owned descriptor is live. Minimize the exposure window + // to external spawners that do not use CLOEXEC_DEFAULT. + if unsafe { libc::fcntl(file.as_raw_fd(), libc::F_SETFD, libc::FD_CLOEXEC) } < 0 { + return Err(io::Error::last_os_error()); + } + } let duplicate = |file: &File| { // Keep action sources above stdio even when the parent has closed 0–2. // SAFETY: file is live; returned descriptor has independent ownership. @@ -123,9 +130,11 @@ impl Child { /// The runner always supplies null stdin, piped outputs, and its own process /// group. Environment overrides/removals and cwd come from the authored Command. -/// This boundary currently inherits the parent environment like CliRunner. +/// This boundary inherits the parent environment like CliRunner. It does not +/// accept env_clear: the caller must use only Command options built by the +/// current runner. Future clear-environment support must be carried explicitly +/// here; Command offers no accessor for its environment-clear flag. pub(crate) fn spawn(command: &mut Command) -> io::Result { - let program = cstring(command.get_program())?; let args: Vec = std::iter::once(command.get_program()) .chain(command.get_args()) .map(cstring) @@ -138,6 +147,31 @@ pub(crate) fn spawn(command: &mut Command) -> io::Result { environment.remove(key); } } + // Resolve before entering libc: posix_spawnp consults process-global PATH, + // not the owned child environment. No environment pointer is shared with + // another thread during the actual spawn. + let requested = std::path::Path::new(command.get_program()); + let resolved = if requested.is_absolute() { + requested.to_owned() + } else if requested.components().count() > 1 { + let base = match command.get_current_dir() { + Some(cwd) if cwd.is_absolute() => cwd.to_owned(), + Some(cwd) => std::env::current_dir()?.join(cwd), + None => std::env::current_dir()?, + }; + base.join(requested) + } else { + crate::runner::resolve_on_path( + requested.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "non-UTF8 executable name") + })?, + environment + .get(std::ffi::OsStr::new("PATH")) + .map(OsString::as_os_str), + ) + .ok_or_else(|| io::Error::from_raw_os_error(libc::ENOENT))? + }; + let program = cstring(resolved.as_os_str())?; let environment: Vec = environment .into_iter() .map(|(key, value)| { @@ -195,6 +229,10 @@ pub(crate) fn spawn(command: &mut Command) -> io::Result { return Err(io::Error::last_os_error()); } let mut defaults = defaults.assume_init(); + check(libc::posix_spawnattr_setsigmask( + &mut attributes.0, + &defaults, + ))?; if libc::sigaddset(&mut defaults, libc::SIGPIPE) != 0 { return Err(io::Error::last_os_error()); } @@ -206,11 +244,12 @@ pub(crate) fn spawn(command: &mut Command) -> io::Result { &mut attributes.0, (libc::POSIX_SPAWN_CLOEXEC_DEFAULT | libc::POSIX_SPAWN_SETPGROUP - | libc::POSIX_SPAWN_SETSIGDEF) as _, + | libc::POSIX_SPAWN_SETSIGDEF + | libc::POSIX_SPAWN_SETSIGMASK) as _, ))?; check(libc::posix_spawnattr_setpgroup(&mut attributes.0, 0))?; let mut pid = 0; - check(libc::posix_spawnp( + check(libc::posix_spawn( &mut pid, program.as_ptr(), &actions.0, @@ -329,3 +368,54 @@ mod contract_tests { std::fs::remove_dir_all(dir).unwrap(); } } + +#[cfg(test)] +mod signal_tests { + use super::*; + + struct RestoreMask(libc::sigset_t); + impl Drop for RestoreMask { + fn drop(&mut self) { + // SAFETY: restore the mask captured on this same test thread. + unsafe { + libc::pthread_sigmask(libc::SIG_SETMASK, &self.0, std::ptr::null_mut()); + } + } + } + + #[test] + fn cli_unblocks_sigterm_inherited_from_worker() { + std::thread::spawn(|| { + let mut empty = MaybeUninit::::uninit(); + let mut original = MaybeUninit::::uninit(); + // SAFETY: initialized masks and writable storage, this thread only. + unsafe { + assert_eq!(libc::sigemptyset(empty.as_mut_ptr()), 0); + let empty = empty.assume_init(); + assert_eq!( + libc::pthread_sigmask(libc::SIG_SETMASK, &empty, original.as_mut_ptr()), + 0 + ); + let _restore = RestoreMask(original.assume_init()); + // Standard Command control on a known-unblocked thread. + let status = Command::new("/bin/sh") + .args(["-c", "kill -TERM $$; sleep 5; echo survived"]) + .status() + .unwrap(); + assert_eq!(status.signal(), Some(libc::SIGTERM)); + let mut blocked = empty; + assert_eq!(libc::sigaddset(&mut blocked, libc::SIGTERM), 0); + assert_eq!( + libc::pthread_sigmask(libc::SIG_BLOCK, &blocked, std::ptr::null_mut()), + 0 + ); + let mut command = Command::new("/bin/sh"); + command.args(["-c", "kill -TERM $$; sleep 5; echo survived"]); + let mut child = spawn(&mut command).unwrap(); + assert_eq!(child.wait().unwrap().signal(), Some(libc::SIGTERM)); + } + }) + .join() + .unwrap(); + } +} From bc00e94384b5dbf81d80df796a90d6248d516a28 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 07:47:51 -0500 Subject: [PATCH 3/4] test(desktop): share a generous fake daemon conversation deadline --- apps/desktop/TESTING.md | 11 +++++++- apps/desktop/core/src/source.rs | 36 +++++++++++++++++++++------ apps/desktop/core/src/test_support.rs | 36 +++++++++++++++++++++++++++ apps/desktop/core/tests/dump_state.rs | 10 +++----- 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/apps/desktop/TESTING.md b/apps/desktop/TESTING.md index ef569a467..328183284 100644 --- a/apps/desktop/TESTING.md +++ b/apps/desktop/TESTING.md @@ -23,7 +23,7 @@ first-launch warmups remain outside timed observations. | --- | --- | | Fake CLI `NotFound` | The runner received a spawn ENOENT, not proof that the fixture pathname was absent. An unavailable script interpreter or a transient executable-launch failure can also produce ENOENT. The precise origin in the reported Mac runs was not captured; do not label it proven descriptor leakage. Separate test processes and closed-before-publish scripts remove cross-test writer inheritance. | | Fake CLI never started | No ready marker was written before a product timeout. This can reflect startup scheduling or failed launch. Warmups, process isolation and the ready marker distinguish startup from tested cleanup; the timeout contract is unchanged. | -| Fake daemon `WouldBlock` (Darwin error 35) | The socket read deadline expired before the request arrived. This is a socket scheduling symptom, not a spawn ENOENT. Version warmups precede the server's accept deadline; per-test processes remove cross-test descriptor interference. | +| Fake daemon `WouldBlock` (Darwin error 35) | The socket read deadline expired before the request arrived. This is a socket scheduling symptom, not a spawn ENOENT. Version warmups precede a single 60-second fixture conversation deadline, shared by acceptance, complete header reads and response writes; no separate two/three-second fake-server read timer remains. Per-test processes remove cross-test descriptor interference. | | Slow stream Drop / late pipe EOF | An inherited pipe writer can keep a reader alive after the intended writer exits. The Apple product boundary below closes unlisted descriptors. #843 separately replaces the tight elapsed-time oracle with reader completion and process retirement under a generous deadline. | These are evidence boundaries, not a claim that every observed failure has one @@ -59,3 +59,12 @@ use CLOEXEC_DEFAULT; the application boundary cannot change those libraries. All CLI spawns through this boundary close unlisted descriptors regardless of that window. Executable resolution uses the captured child PATH and absolute `posix_spawn`, avoiding libc's process-global PATH lookup. + +Fake-daemon timing is fixture scheduling, not an assertion about production +latency. Both source fixtures and the headless-state fixture use the same +60-second conversation budget. Reads accept delayed and partial HTTP headers +(up to 4096 bytes), recalculate remaining time after each read, and report a +fixture-specific failure at the deadline. Response writes use the remaining +budget too. A regression deliberately waits beyond the old two-second deadline +before sending a split header. Product socket/CLI timeouts and nextest retries +are unchanged. diff --git a/apps/desktop/core/src/source.rs b/apps/desktop/core/src/source.rs index d7fbc10c3..c04e12e3d 100644 --- a/apps/desktop/core/src/source.rs +++ b/apps/desktop/core/src/source.rs @@ -503,7 +503,7 @@ mod tests { use super::*; fn fake_daemon(projects: &str) -> (DaemonClient, std::thread::JoinHandle<()>) { - use std::io::{Read, Write}; + use std::io::Write; use std::os::unix::{fs::DirBuilderExt, net::UnixListener}; let dir = PathBuf::from(format!( "/tmp/hds-{}-{}", @@ -519,8 +519,8 @@ mod tests { listener.set_nonblocking(true).unwrap(); let projects = projects.to_owned(); let handle = std::thread::spawn(move || { + let deadline = std::time::Instant::now() + crate::test_support::DAEMON_BUDGET; for body in [r#"{"status":"ok","version":"5.0.0"}"#.to_owned(), projects] { - let deadline = std::time::Instant::now() + Duration::from_secs(3); let (mut stream, _) = loop { match listener.accept() { Ok(pair) => break pair, @@ -533,12 +533,10 @@ mod tests { Err(e) => panic!("fake daemon accept failed or timed out: {e}"), } }; + crate::test_support::read_request(&mut stream, deadline); stream - .set_read_timeout(Some(Duration::from_secs(2))) + .set_write_timeout(Some(crate::test_support::remaining(deadline))) .unwrap(); - let mut buf = [0; 4096]; - let n = stream.read(&mut buf).unwrap(); - assert!(String::from_utf8_lossy(&buf[..n]).starts_with("GET /v1/")); write!( stream, "HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n{}", @@ -549,7 +547,31 @@ mod tests { } std::fs::remove_dir_all(dir).unwrap(); }); - (DaemonClient::new(socket, Duration::from_secs(2)), handle) + ( + DaemonClient::new(socket, crate::test_support::DAEMON_BUDGET), + handle, + ) + } + + #[test] + fn fake_daemon_waits_for_delayed_and_partial_request_headers() { + use std::io::Write; + let (mut server, mut client) = std::os::unix::net::UnixStream::pair().unwrap(); + let (ready, waiting) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let deadline = std::time::Instant::now() + crate::test_support::DAEMON_BUDGET; + ready.send(()).unwrap(); + crate::test_support::read_request(&mut server, deadline); + }); + waiting + .recv_timeout(crate::test_support::DAEMON_BUDGET) + .unwrap(); + // Exceed the former two-second socket read timeout deliberately. + std::thread::sleep(Duration::from_millis(2500)); + client.write_all(b"GET /v1/status HTTP/1.1\r\n").unwrap(); + std::thread::sleep(Duration::from_millis(10)); + client.write_all(b"Host: fixture\r\n\r\n").unwrap(); + worker.join().unwrap(); } /// A trusted fake daemon provides rows; only native ps uses the fake CLI. diff --git a/apps/desktop/core/src/test_support.rs b/apps/desktop/core/src/test_support.rs index b09978ba9..65db61392 100644 --- a/apps/desktop/core/src/test_support.rs +++ b/apps/desktop/core/src/test_support.rs @@ -17,3 +17,39 @@ pub fn script(path: &Path, body: impl AsRef<[u8]>) { std::fs::set_permissions(&pending, std::fs::Permissions::from_mode(0o700)).unwrap(); std::fs::rename(pending, path).unwrap(); } + +/// One scheduling budget for the entire fake-daemon conversation, not a +/// production socket timeout. Never pass zero to set_read_timeout. +pub const DAEMON_BUDGET: std::time::Duration = std::time::Duration::from_secs(60); + +pub fn remaining(deadline: std::time::Instant) -> std::time::Duration { + deadline + .checked_duration_since(std::time::Instant::now()) + .filter(|duration| !duration.is_zero()) + .expect("fake daemon exceeded its overall fixture deadline") +} + +/// Read a complete, bounded HTTP header, including delayed/partial writes. +/// Socket waits consume the same overall budget as listener acceptance. +pub fn read_request(stream: &mut std::os::unix::net::UnixStream, deadline: std::time::Instant) { + use std::io::Read; + let mut request = Vec::new(); + while !request.ends_with(b"\r\n\r\n") { + stream.set_read_timeout(Some(remaining(deadline))).unwrap(); + let mut chunk = [0; 512]; + let n = match stream.read(&mut chunk) { + Ok(n) => n, + Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, + Err(error) => { + panic!("fake daemon request did not arrive within fixture deadline: {error}") + } + }; + assert_ne!(n, 0, "client closed before completing fake daemon request"); + request.extend_from_slice(&chunk[..n]); + assert!( + request.len() <= 4096, + "fake daemon request header exceeds fixture bound" + ); + } + assert!(request.starts_with(b"GET /v1/")); +} diff --git a/apps/desktop/core/tests/dump_state.rs b/apps/desktop/core/tests/dump_state.rs index 6f62a9a04..c32c9fa59 100644 --- a/apps/desktop/core/tests/dump_state.rs +++ b/apps/desktop/core/tests/dump_state.rs @@ -23,7 +23,7 @@ fn temp_dir(tag: &str) -> PathBuf { } fn serve_snapshot(home: &std::path::Path) -> std::thread::JoinHandle<()> { - use std::io::{Read, Write}; + use std::io::Write; use std::os::unix::{fs::DirBuilderExt, net::UnixListener}; let dir = home.join("daemon"); std::fs::DirBuilder::new().mode(0o700).create(&dir).unwrap(); @@ -34,8 +34,8 @@ fn serve_snapshot(home: &std::path::Path) -> std::thread::JoinHandle<()> { r#"{"status":"ok","version":"5.0.0"}"#, r#"{"runtime_ok":true,"projects":[{"name":"alpha","status":"running","repo_root":"/work/alpha","defined_services":["api"],"runtime":{"services":[{"service":"api","containers":[{"state":"running","status":"Up 5 minutes"}]}]}},{"name":"beta","status":"stopped","repo_root":"/work/beta","defined_services":["web"]}]}"#, ]; + let deadline = std::time::Instant::now() + test_support::DAEMON_BUDGET; for body in bodies { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(3); let (mut stream, _) = loop { match listener.accept() { Ok(pair) => break pair, @@ -48,12 +48,10 @@ fn serve_snapshot(home: &std::path::Path) -> std::thread::JoinHandle<()> { Err(e) => panic!("fake daemon accept failed or timed out: {e}"), } }; + test_support::read_request(&mut stream, deadline); stream - .set_read_timeout(Some(std::time::Duration::from_secs(3))) + .set_write_timeout(Some(test_support::remaining(deadline))) .unwrap(); - let mut request = [0; 4096]; - let n = stream.read(&mut request).unwrap(); - assert!(String::from_utf8_lossy(&request[..n]).starts_with("GET /v1/")); write!( stream, "HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n{}", From e2c6cc67aced9d16d7ac79de416d946d18473818 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy <344842+roodboi@users.noreply.github.com> Date: Sat, 14 Nov 2026 07:51:13 -0500 Subject: [PATCH 4/4] test(desktop): normalize accepted fixture sockets to blocking mode --- apps/desktop/TESTING.md | 10 +++++- apps/desktop/core/src/source.rs | 44 +++++++++++++++++++++++++-- apps/desktop/core/src/test_support.rs | 10 ++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/apps/desktop/TESTING.md b/apps/desktop/TESTING.md index 328183284..d8ed53ff1 100644 --- a/apps/desktop/TESTING.md +++ b/apps/desktop/TESTING.md @@ -23,7 +23,7 @@ first-launch warmups remain outside timed observations. | --- | --- | | Fake CLI `NotFound` | The runner received a spawn ENOENT, not proof that the fixture pathname was absent. An unavailable script interpreter or a transient executable-launch failure can also produce ENOENT. The precise origin in the reported Mac runs was not captured; do not label it proven descriptor leakage. Separate test processes and closed-before-publish scripts remove cross-test writer inheritance. | | Fake CLI never started | No ready marker was written before a product timeout. This can reflect startup scheduling or failed launch. Warmups, process isolation and the ready marker distinguish startup from tested cleanup; the timeout contract is unchanged. | -| Fake daemon `WouldBlock` (Darwin error 35) | The socket read deadline expired before the request arrived. This is a socket scheduling symptom, not a spawn ENOENT. Version warmups precede a single 60-second fixture conversation deadline, shared by acceptance, complete header reads and response writes; no separate two/three-second fake-server read timer remains. Per-test processes remove cross-test descriptor interference. | +| Fake daemon `WouldBlock` (Darwin error 35) | Darwin/BSD can return this immediately when accept inherits a listener's nonblocking mode; it can also mean a timed read expired. Error 35 alone does not prove elapsed time or a spawn ENOENT. Fixtures explicitly restore blocking mode before reads. Version warmups precede a single 60-second fixture conversation deadline, shared by acceptance, complete header reads and response writes; no separate two/three-second fake-server read timer remains. Per-test processes remove cross-test descriptor interference. | | Slow stream Drop / late pipe EOF | An inherited pipe writer can keep a reader alive after the intended writer exits. The Apple product boundary below closes unlisted descriptors. #843 separately replaces the tight elapsed-time oracle with reader completion and process retirement under a generous deadline. | These are evidence boundaries, not a claim that every observed failure has one @@ -68,3 +68,11 @@ fixture-specific failure at the deadline. Response writes use the remaining budget too. A regression deliberately waits beyond the old two-second deadline before sending a split header. Product socket/CLI timeouts and nextest retries are unchanged. + +The shared request reader restores blocking mode immediately on entry after +accept, because Darwin/BSD accepted sockets can inherit O_NONBLOCK from a +nonblocking listener (Linux does not). WouldBlock is handled only while the +overall deadline remains; nextest still performs zero retries. The regression +now uses a nonblocking listener, forces the accepted nonblocking mode on Linux +too, sends a delayed split request, and checks that the accepted stream was +normalized back to blocking mode. diff --git a/apps/desktop/core/src/source.rs b/apps/desktop/core/src/source.rs index c04e12e3d..0c9351031 100644 --- a/apps/desktop/core/src/source.rs +++ b/apps/desktop/core/src/source.rs @@ -556,22 +556,62 @@ mod tests { #[test] fn fake_daemon_waits_for_delayed_and_partial_request_headers() { use std::io::Write; - let (mut server, mut client) = std::os::unix::net::UnixStream::pair().unwrap(); + use std::os::fd::AsRawFd; + use std::os::unix::{ + fs::DirBuilderExt, + net::{UnixListener, UnixStream}, + }; + let dir = PathBuf::from(format!( + "/tmp/hdr-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::DirBuilder::new().mode(0o700).create(&dir).unwrap(); + let socket = dir.join("s"); + let listener = UnixListener::bind(&socket).unwrap(); + listener.set_nonblocking(true).unwrap(); let (ready, waiting) = std::sync::mpsc::channel(); let worker = std::thread::spawn(move || { let deadline = std::time::Instant::now() + crate::test_support::DAEMON_BUDGET; + let mut server = loop { + match listener.accept() { + Ok((stream, _)) => break stream, + Err(error) + if error.kind() == std::io::ErrorKind::WouldBlock + && std::time::Instant::now() < deadline => + { + std::thread::sleep(Duration::from_millis(5)) + } + Err(error) => panic!("regression fixture accept: {error}"), + } + }; + // Force Darwin's inherited-mode case on Linux as well. + server.set_nonblocking(true).unwrap(); ready.send(()).unwrap(); crate::test_support::read_request(&mut server, deadline); + // SAFETY: the accepted stream remains live; inspect its flags. + let flags = unsafe { libc::fcntl(server.as_raw_fd(), libc::F_GETFL) }; + assert!(flags >= 0); + assert_eq!( + flags & libc::O_NONBLOCK, + 0, + "accepted stream must be blocking" + ); }); + let mut client = UnixStream::connect(&socket).unwrap(); waiting .recv_timeout(crate::test_support::DAEMON_BUDGET) .unwrap(); - // Exceed the former two-second socket read timeout deliberately. + // Exceed the former two-second timeout, then split the header. std::thread::sleep(Duration::from_millis(2500)); client.write_all(b"GET /v1/status HTTP/1.1\r\n").unwrap(); std::thread::sleep(Duration::from_millis(10)); client.write_all(b"Host: fixture\r\n\r\n").unwrap(); worker.join().unwrap(); + std::fs::remove_dir_all(dir).unwrap(); } /// A trusted fake daemon provides rows; only native ps uses the fake CLI. diff --git a/apps/desktop/core/src/test_support.rs b/apps/desktop/core/src/test_support.rs index 65db61392..4cc7d3e15 100644 --- a/apps/desktop/core/src/test_support.rs +++ b/apps/desktop/core/src/test_support.rs @@ -33,6 +33,9 @@ pub fn remaining(deadline: std::time::Instant) -> std::time::Duration { /// Socket waits consume the same overall budget as listener acceptance. pub fn read_request(stream: &mut std::os::unix::net::UnixStream, deadline: std::time::Instant) { use std::io::Read; + // Darwin/BSD accept inherits O_NONBLOCK from the listener. Linux does + // not, so normalize explicitly before installing a read timeout. + stream.set_nonblocking(false).unwrap(); let mut request = Vec::new(); while !request.ends_with(b"\r\n\r\n") { stream.set_read_timeout(Some(remaining(deadline))).unwrap(); @@ -40,6 +43,13 @@ pub fn read_request(stream: &mut std::os::unix::net::UnixStream, deadline: std:: let n = match stream.read(&mut chunk) { Ok(n) => n, Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, + Err(error) + if error.kind() == std::io::ErrorKind::WouldBlock + && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(5)); + continue; + } Err(error) => { panic!("fake daemon request did not arrive within fixture deadline: {error}") }