From dffe8c804f5ad3fc8458bee148c51abd8b1f8841 Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 14 Oct 2026 23:19:04 +0900 Subject: [PATCH 1/5] fix(desktop): close atomic writer before publishing CLI files --- desktop/src-tauri/src/cli_command_record.rs | 2 ++ structure/desktop-terminal-command.md | 2 ++ 2 files changed, 4 insertions(+) diff --git a/desktop/src-tauri/src/cli_command_record.rs b/desktop/src-tauri/src/cli_command_record.rs index 294c0d86346..0a1f4ea0a62 100644 --- a/desktop/src-tauri/src/cli_command_record.rs +++ b/desktop/src-tauri/src/cli_command_record.rs @@ -593,6 +593,8 @@ pub fn atomic(path: &Path, before: Option<&[u8]>, after: &[u8], mode: u32) -> Re if read_bytes(path)?.as_deref() != before { return Err("concurrent-edit".into()); } + // Publish only after releasing our writer; a terminal may execute the new shim immediately. + drop(f); replace(&tmp, path)?; #[cfg(unix)] io(fs::File::open(parent).and_then(|f| f.sync_all()))?; diff --git a/structure/desktop-terminal-command.md b/structure/desktop-terminal-command.md index 2003a27bd53..5adbe39f082 100644 --- a/structure/desktop-terminal-command.md +++ b/structure/desktop-terminal-command.md @@ -21,6 +21,8 @@ disabled intent wins over pending work. Invalid records and journal conflicts bl unreferenced journals are deleted only under the lock after a valid (or absent) record read. Every private path must be user-owned without extended ACLs (macOS/Linux) or carry a protected user/SYSTEM/Administrators DACL (Windows root; children inherit it), otherwise nothing is written. +Atomic replacement flushes and closes its write handle before publishing the new file by rename, +so the published executable is never held open for writing by the replacing thread. `desktop/src-tauri/src/cli_command_posix.rs` generates a fail-closed shim, shared PATH helper and final managed zsh/bash/fish blocks. Reconcile separates desired rc targets from recorded From bdbccc509b48f59351e9addfe08630a4845e64b3 Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 14 Oct 2026 23:22:55 +0900 Subject: [PATCH 2/5] test(desktop): exclude executable writers from concurrent forks Share a Unix test read/write lock between executable fixture writes and every synchronous subprocess spawn. Close-on-exec does not prevent a forked child from temporarily retaining a writer, which made direct shim execution return ETXTBSY during the CLI record lock stress test. Keep non-executable record writes and all four stress spawners concurrent, and assert that executable writes wait until spawn protection ends. --- desktop/src-tauri/src/cli_command.rs | 48 +++++++------- desktop/src-tauri/src/cli_command_posix.rs | 71 +++++++++++--------- desktop/src-tauri/src/cli_command_record.rs | 73 +++++++++++++++++---- desktop/src-tauri/src/login_registration.rs | 20 ++++-- structure/desktop-terminal-command.md | 4 ++ 5 files changed, 144 insertions(+), 72 deletions(-) diff --git a/desktop/src-tauri/src/cli_command.rs b/desktop/src-tauri/src/cli_command.rs index a89361e9fd7..1cbf3219ab2 100644 --- a/desktop/src-tauri/src/cli_command.rs +++ b/desktop/src-tauri/src/cli_command.rs @@ -454,18 +454,20 @@ mod tests { } fn selected_observation_case(case: &str) { let t = Temp::new(); - let result = std::process::Command::new(std::env::current_exe().unwrap()) - .args([ - "--exact", - "cli_command::tests::selected_observation_child", - "--nocapture", - ]) - .env("OCX_CLI_OBSERVATION_TEST", case) - .env("HOME", &t.0) - .env("ZDOTDIR", &t.0) - .env("XDG_CONFIG_HOME", t.0.join(".config")) - .output() - .unwrap(); + let result = record::tests::with_spawn(|| { + std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "cli_command::tests::selected_observation_child", + "--nocapture", + ]) + .env("OCX_CLI_OBSERVATION_TEST", case) + .env("HOME", &t.0) + .env("ZDOTDIR", &t.0) + .env("XDG_CONFIG_HOME", t.0.join(".config")) + .output() + }) + .unwrap(); assert!(result.status.success(), "selected-observation-failed"); assert!( String::from_utf8_lossy(&result.stdout).contains("1 passed"), @@ -578,16 +580,18 @@ mod tests { } } fn disabled_selector_case(action: &str) { - let result = std::process::Command::new(std::env::current_exe().unwrap()) - .args([ - "--exact", - "cli_command::tests::disabled_selector_child", - "--nocapture", - ]) - .env("OCX_CLI_SELECTOR_TEST", action) - .env("ZDOTDIR", "/invalid:zdotdir") - .output() - .unwrap(); + let result = record::tests::with_spawn(|| { + std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "cli_command::tests::disabled_selector_child", + "--nocapture", + ]) + .env("OCX_CLI_SELECTOR_TEST", action) + .env("ZDOTDIR", "/invalid:zdotdir") + .output() + }) + .unwrap(); assert!(result.status.success(), "disabled-selector-failed"); assert!( String::from_utf8_lossy(&result.stdout).contains("1 passed"), diff --git a/desktop/src-tauri/src/cli_command_posix.rs b/desktop/src-tauri/src/cli_command_posix.rs index 8ab0fcf8295..aa8b482f4a3 100644 --- a/desktop/src-tauri/src/cli_command_posix.rs +++ b/desktop/src-tauri/src/cli_command_posix.rs @@ -673,7 +673,7 @@ pub fn remove_plan(store: &Store, current: &Record) -> Result<(Record, Vec Temp { // Stable bundles cannot live under the OS temp directory. Keep the fixture @@ -701,13 +701,13 @@ mod tests { command.arg("-N"); } assert!( - command + with_spawn(|| command .arg(path) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) - .status() - .unwrap() - .success(), + .status()) + .unwrap() + .success(), "acl-command-failed" ); } @@ -969,7 +969,7 @@ mod tests { if let Some(path) = path { command.env("PATH", path); } - let output = command.output().unwrap(); + let output = with_spawn(|| command.output()).unwrap(); assert!(output.status.success()); assert_eq!(String::from_utf8(output.stdout).unwrap(), expected); } @@ -988,14 +988,16 @@ mod tests { 0o700, ) .unwrap(); - let out = std::process::Command::new(&shim) - .env_clear() - .env("ANTHROPIC_AUTH_TOKEN", "dummy") - .env("ANTHROPIC_API_KEY", "") - .env("OCX_NODE_LAUNCH_CONTEXT", "untrusted") - .arg("status") - .output() - .unwrap(); + let out = with_spawn(|| { + std::process::Command::new(&shim) + .env_clear() + .env("ANTHROPIC_AUTH_TOKEN", "dummy") + .env("ANTHROPIC_API_KEY", "") + .env("OCX_NODE_LAUNCH_CONTEXT", "untrusted") + .arg("status") + .output() + }) + .unwrap(); assert!(out.status.success()); let text = String::from_utf8(out.stdout).unwrap(); let lines: Vec<_> = text.lines().collect(); @@ -1011,9 +1013,7 @@ mod tests { assert!(!text.contains("dummy")); fs::remove_file(cli).unwrap(); assert_eq!( - std::process::Command::new(&shim) - .env_clear() - .output() + with_spawn(|| std::process::Command::new(&shim).env_clear().output()) .unwrap() .status .code(), @@ -1035,13 +1035,18 @@ mod tests { .replace("-N32", "-N1"), ] { record::atomic(&t.0.join("cli"), None, inspect, 0o700).unwrap(); - fs::write(&shim, rendered).unwrap(); - let out = std::process::Command::new(&shim) - .env_clear() - .env("OCX_NODE_LAUNCH_CONTEXT", "inherited") - .arg("status") - .output() - .unwrap(); + { + let _writer = record::tests::executable_write(); + fs::write(&shim, rendered).unwrap(); + } + let out = with_spawn(|| { + std::process::Command::new(&shim) + .env_clear() + .env("OCX_NODE_LAUNCH_CONTEXT", "inherited") + .arg("status") + .output() + }) + .unwrap(); assert!(out.status.success()); assert_eq!(out.stdout, b"\nstatus\n"); fs::remove_file(t.0.join("cli")).unwrap(); @@ -1197,15 +1202,17 @@ mod tests { // System rc files may print to stdout (Ubuntu's /etc/bash.bashrc sudo hint), so only // the marked result lines count; a failed lookup or CLI run still fails the shell, and // the end marker makes any extra CLI output line fail the comparison. - let output = command - .args([ - "-i", - "-c", - "p=$(command -v ocx) || exit 11; o=$(ocx hello) || exit 12; \ + let output = with_spawn(|| { + command + .args([ + "-i", + "-c", + "p=$(command -v ocx) || exit 11; o=$(ocx hello) || exit 12; \ printf 'OCX-PATH:%s\\nOCX-OUT:%s:OCX-END\\n' \"$p\" \"$o\"", - ]) - .output() - .expect("cannot start real shell"); + ]) + .output() + }) + .expect("cannot start real shell"); assert!(output.status.success(), "{shell}: command failed"); let stdout = String::from_utf8(output.stdout).expect("shell stdout is not UTF-8"); let marked = |tag: &str| { diff --git a/desktop/src-tauri/src/cli_command_record.rs b/desktop/src-tauri/src/cli_command_record.rs index 0a1f4ea0a62..ece5a429f1b 100644 --- a/desktop/src-tauri/src/cli_command_record.rs +++ b/desktop/src-tauri/src/cli_command_record.rs @@ -548,6 +548,8 @@ fn replace(from: &Path, to: &Path) -> Result<()> { io(fs::rename(from, to)) } pub fn atomic(path: &Path, before: Option<&[u8]>, after: &[u8], mode: u32) -> Result<()> { + #[cfg(all(test, unix))] + let _executable_write = (mode & 0o111 != 0).then(tests::executable_write); let parent = path.parent().ok_or("unsafe-file")?; check(parent, true)?; rc_acl_check(path)?; @@ -1147,6 +1149,51 @@ impl Store { #[cfg(test)] pub(crate) mod tests { use super::*; + #[cfg(unix)] + static EXECUTABLE_IO: std::sync::RwLock<()> = std::sync::RwLock::new(()); + #[cfg(unix)] + pub(crate) fn executable_write() -> std::sync::RwLockWriteGuard<'static, ()> { + EXECUTABLE_IO.write().unwrap() + } + #[cfg(unix)] + pub(crate) fn with_spawn(spawn: impl FnOnce() -> T) -> T { + // CLOEXEC closes descriptors at exec, not fork. Exclude executable writers until + // the child's exec has completed; allow the lock stress test's spawners to overlap. + let _guard = EXECUTABLE_IO.read().unwrap(); + spawn() + } + #[cfg(unix)] + #[test] + fn executable_writes_wait_for_process_spawns_but_record_writes_do_not() { + use std::{sync::mpsc, thread, time::Duration}; + let t = Temp::new(); + let path = t.0.join("shim"); + let (started_tx, started_rx) = mpsc::channel(); + let (done_tx, done_rx) = mpsc::channel(); + let (writer, early_completion) = with_spawn(|| { + // Non-executable record writes must still overlap the process-spawn stress test. + atomic(&t.0.join("record"), None, b"record", 0o600).unwrap(); + let path = path.clone(); + let writer = thread::spawn(move || { + started_tx.send(()).unwrap(); + let result = atomic(&path, None, b"#!/bin/sh\nexit 0\n", 0o700); + done_tx.send(()).unwrap(); + result + }); + started_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + let early_completion = done_rx.recv_timeout(Duration::from_millis(100)); + (writer, early_completion) + }); + if early_completion.is_err() { + done_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + } + writer.join().unwrap().unwrap(); + assert!(matches!( + early_completion, + Err(mpsc::RecvTimeoutError::Timeout) + )); + assert_eq!(fs::read(path).unwrap(), b"#!/bin/sh\nexit 0\n"); + } pub(crate) struct Temp(pub PathBuf); impl Temp { pub(crate) fn new() -> Self { @@ -1556,13 +1603,13 @@ pub(crate) mod tests { command.arg("-N"); } assert!( - command + with_spawn(|| command .arg(path) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) - .status() - .unwrap() - .success(), + .status()) + .unwrap() + .success(), "acl-command-failed" ); } @@ -1633,12 +1680,14 @@ pub(crate) mod tests { fn linux_acl_inheritance_clear_and_existing_child_refusal() { let t = Temp::new(); let set = |path: &Path, spec: &str| { - std::process::Command::new("setfacl") - .args(["-m", spec]) - .arg(path) - .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .status() + with_spawn(|| { + std::process::Command::new("setfacl") + .args(["-m", spec]) + .arg(path) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + }) }; let result = set(&t.0, "d:u:65534:rwx"); if result @@ -1908,7 +1957,9 @@ pub(crate) mod tests { ready.wait(); let mut spawns = 0; while !stop.load(Ordering::Relaxed) { - assert!(Command::new(TRUE).status().unwrap().success()); + assert!(with_spawn(|| Command::new(TRUE).status()) + .unwrap() + .success()); spawns += 1; } spawns diff --git a/desktop/src-tauri/src/login_registration.rs b/desktop/src-tauri/src/login_registration.rs index 43ea2ae71e2..7a2e3236dae 100644 --- a/desktop/src-tauri/src/login_registration.rs +++ b/desktop/src-tauri/src/login_registration.rs @@ -527,13 +527,19 @@ mod command { pub(super) fn run(program: &str, args: &[&str], timeout: Duration) -> Result { let slot = Slot::take().ok_or(RunError::Failed)?; let deadline = Instant::now() + timeout; - let mut child = Command::new(program) - .args(args) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .map_err(|_| RunError::Failed)?; + let spawn = || { + Command::new(program) + .args(args) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + }; + #[cfg(test)] + let child = crate::cli_command_record::tests::with_spawn(spawn); + #[cfg(not(test))] + let child = spawn(); + let mut child = child.map_err(|_| RunError::Failed)?; let readers = [drain(child.stdout.take()), drain(child.stderr.take())]; let status = loop { match child.try_wait() { diff --git a/structure/desktop-terminal-command.md b/structure/desktop-terminal-command.md index 5adbe39f082..00cecda8efa 100644 --- a/structure/desktop-terminal-command.md +++ b/structure/desktop-terminal-command.md @@ -23,6 +23,10 @@ Every private path must be user-owned without extended ACLs (macOS/Linux) or car user/SYSTEM/Administrators DACL (Windows root; children inherit it), otherwise nothing is written. Atomic replacement flushes and closes its write handle before publishing the new file by rename, so the published executable is never held open for writing by the replacing thread. +Unix Rust tests share a read/write lock between executable fixture writers and subprocess spawns. +Spawns may overlap each other, but no child can inherit a fixture's writable descriptor before exec +closes it; closing the parent's close-on-exec descriptor alone does not release a forked duplicate. +The lock exists only in test builds and leaves ordinary record writes concurrent with spawn stress. `desktop/src-tauri/src/cli_command_posix.rs` generates a fail-closed shim, shared PATH helper and final managed zsh/bash/fish blocks. Reconcile separates desired rc targets from recorded From 17c60c41287702ca574827eec58de418befa1f0d Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 14 Oct 2026 23:24:00 +0900 Subject: [PATCH 3/5] test(desktop): tolerate a poisoned executable-IO lock so one panic cannot cascade --- desktop/src-tauri/src/cli_command_record.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/desktop/src-tauri/src/cli_command_record.rs b/desktop/src-tauri/src/cli_command_record.rs index ece5a429f1b..c8bdd0a9aee 100644 --- a/desktop/src-tauri/src/cli_command_record.rs +++ b/desktop/src-tauri/src/cli_command_record.rs @@ -1153,13 +1153,17 @@ pub(crate) mod tests { static EXECUTABLE_IO: std::sync::RwLock<()> = std::sync::RwLock::new(()); #[cfg(unix)] pub(crate) fn executable_write() -> std::sync::RwLockWriteGuard<'static, ()> { - EXECUTABLE_IO.write().unwrap() + EXECUTABLE_IO + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner) } #[cfg(unix)] pub(crate) fn with_spawn(spawn: impl FnOnce() -> T) -> T { // CLOEXEC closes descriptors at exec, not fork. Exclude executable writers until // the child's exec has completed; allow the lock stress test's spawners to overlap. - let _guard = EXECUTABLE_IO.read().unwrap(); + let _guard = EXECUTABLE_IO + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner); spawn() } #[cfg(unix)] From 6a12ee331eb123f58aafeb69ec78c91b0a7cc8b3 Mon Sep 17 00:00:00 2001 From: GOAT Date: Thu, 15 Oct 2026 16:21:33 +0900 Subject: [PATCH 4/5] test(desktop): isolate command deadlines from fixture spawn contention --- desktop/src-tauri/src/login_registration.rs | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/desktop/src-tauri/src/login_registration.rs b/desktop/src-tauri/src/login_registration.rs index 7a2e3236dae..c88e7dd4e47 100644 --- a/desktop/src-tauri/src/login_registration.rs +++ b/desktop/src-tauri/src/login_registration.rs @@ -585,6 +585,29 @@ mod command { #[test] fn a_command_past_its_deadline_holds_the_only_slot_until_it_ends() { + // Measure command deadlines in a fresh process: the parent's executable-fixture + // writers may hold the test spawn lock longer than this test's timing budget. + const CHILD: &str = "OCX_LOGIN_COMMAND_DEADLINE_CHILD"; + if std::env::var_os(CHILD).is_none() { + let output = crate::cli_command_record::tests::with_spawn(|| { + Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "login_registration::command::tests::a_command_past_its_deadline_holds_the_only_slot_until_it_ends", + "--nocapture", + ]) + .env(CHILD, "1") + .output() + }) + .unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); + return; + } let started = Instant::now(); assert_eq!( run("/bin/sleep", &["1"], Duration::from_millis(50)), From e504c23ccbe3654d6d329cb64803c1dd03e69fd7 Mon Sep 17 00:00:00 2001 From: GOAT Date: Thu, 15 Oct 2026 22:20:00 +0900 Subject: [PATCH 5/5] fix(desktop): coordinate executable publication with production forks --- desktop/src-tauri/src/claim.rs | 7 +- desktop/src-tauri/src/cli_command.rs | 4 +- desktop/src-tauri/src/cli_command_posix.rs | 5 +- desktop/src-tauri/src/cli_command_record.rs | 21 +- desktop/src-tauri/src/exit.rs | 4 +- desktop/src-tauri/src/lib.rs | 1 + desktop/src-tauri/src/login_registration.rs | 7 +- desktop/src-tauri/src/process_spawn.rs | 209 ++++++++++++++++++ desktop/src-tauri/src/resolve.rs | 7 +- desktop/src-tauri/src/runtime_stop.rs | 7 +- desktop/src-tauri/src/sidecar.rs | 3 +- desktop/src-tauri/src/tray.rs | 7 +- desktop/src-tauri/src/tray_login.rs | 9 +- desktop/src-tauri/src/updater.rs | 2 +- desktop/src-tauri/src/window.rs | 4 +- structure/desktop-shell.md | 1 + structure/desktop-terminal-command.md | 18 +- tests/clients/desktop-cli-contracts.test.ts | 2 +- tests/clients/desktop-startup-surface.test.ts | 4 +- 19 files changed, 273 insertions(+), 49 deletions(-) create mode 100644 desktop/src-tauri/src/process_spawn.rs diff --git a/desktop/src-tauri/src/claim.rs b/desktop/src-tauri/src/claim.rs index 22d0cfcbc24..d7e2d53ef51 100644 --- a/desktop/src-tauri/src/claim.rs +++ b/desktop/src-tauri/src/claim.rs @@ -163,7 +163,12 @@ pub async fn run(app: &AppHandle, argv: Vec, deadline: Instant) -> Claim return ClaimResult::Failed(format!("the bundled CLI could not be started ({error})")) } }; - match timeout_at(deadline, command.output()).await { + match timeout_at( + deadline, + crate::process_spawn::with_spawn_async(command.output()), + ) + .await + { Ok(Ok(output)) => read(output.status.code(), &output.stdout, &output.stderr), Ok(Err(error)) => { ClaimResult::Failed(format!("the bundled CLI could not be run ({error})")) diff --git a/desktop/src-tauri/src/cli_command.rs b/desktop/src-tauri/src/cli_command.rs index 1cbf3219ab2..7e05e54f85e 100644 --- a/desktop/src-tauri/src/cli_command.rs +++ b/desktop/src-tauri/src/cli_command.rs @@ -454,7 +454,7 @@ mod tests { } fn selected_observation_case(case: &str) { let t = Temp::new(); - let result = record::tests::with_spawn(|| { + let result = crate::process_spawn::with_spawn(|| { std::process::Command::new(std::env::current_exe().unwrap()) .args([ "--exact", @@ -580,7 +580,7 @@ mod tests { } } fn disabled_selector_case(action: &str) { - let result = record::tests::with_spawn(|| { + let result = crate::process_spawn::with_spawn(|| { std::process::Command::new(std::env::current_exe().unwrap()) .args([ "--exact", diff --git a/desktop/src-tauri/src/cli_command_posix.rs b/desktop/src-tauri/src/cli_command_posix.rs index aa8b482f4a3..b3016bd9ff4 100644 --- a/desktop/src-tauri/src/cli_command_posix.rs +++ b/desktop/src-tauri/src/cli_command_posix.rs @@ -673,7 +673,8 @@ pub fn remove_plan(store: &Store, current: &Record) -> Result<(Record, Vec Temp { // Stable bundles cannot live under the OS temp directory. Keep the fixture @@ -1036,7 +1037,7 @@ mod tests { ] { record::atomic(&t.0.join("cli"), None, inspect, 0o700).unwrap(); { - let _writer = record::tests::executable_write(); + let _writer = crate::process_spawn::executable_write(); fs::write(&shim, rendered).unwrap(); } let out = with_spawn(|| { diff --git a/desktop/src-tauri/src/cli_command_record.rs b/desktop/src-tauri/src/cli_command_record.rs index c8bdd0a9aee..950993a3b4f 100644 --- a/desktop/src-tauri/src/cli_command_record.rs +++ b/desktop/src-tauri/src/cli_command_record.rs @@ -548,8 +548,8 @@ fn replace(from: &Path, to: &Path) -> Result<()> { io(fs::rename(from, to)) } pub fn atomic(path: &Path, before: Option<&[u8]>, after: &[u8], mode: u32) -> Result<()> { - #[cfg(all(test, unix))] - let _executable_write = (mode & 0o111 != 0).then(tests::executable_write); + #[cfg(unix)] + let _executable_write = (mode & 0o111 != 0).then(crate::process_spawn::executable_write); let parent = path.parent().ok_or("unsafe-file")?; check(parent, true)?; rc_acl_check(path)?; @@ -1150,22 +1150,7 @@ impl Store { pub(crate) mod tests { use super::*; #[cfg(unix)] - static EXECUTABLE_IO: std::sync::RwLock<()> = std::sync::RwLock::new(()); - #[cfg(unix)] - pub(crate) fn executable_write() -> std::sync::RwLockWriteGuard<'static, ()> { - EXECUTABLE_IO - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner) - } - #[cfg(unix)] - pub(crate) fn with_spawn(spawn: impl FnOnce() -> T) -> T { - // CLOEXEC closes descriptors at exec, not fork. Exclude executable writers until - // the child's exec has completed; allow the lock stress test's spawners to overlap. - let _guard = EXECUTABLE_IO - .read() - .unwrap_or_else(std::sync::PoisonError::into_inner); - spawn() - } + use crate::process_spawn::with_spawn; #[cfg(unix)] #[test] fn executable_writes_wait_for_process_spawns_but_record_writes_do_not() { diff --git a/desktop/src-tauri/src/exit.rs b/desktop/src-tauri/src/exit.rs index 8950c338638..9c3d0040881 100644 --- a/desktop/src-tauri/src/exit.rs +++ b/desktop/src-tauri/src/exit.rs @@ -535,7 +535,7 @@ async fn finish_and_exit_after(app: &AppHandle, reason: ExitReason) { match (reason, verdict) { (ExitReason::UserQuit, _) => app.exit(0), (ExitReason::CoordinatedRestart, DrainVerdict::Drained) => { - app.restart(); + crate::process_spawn::with_spawn(|| app.restart()); } (ExitReason::CoordinatedRestart, _) => { crate::logging::log_once("the update restart was refused", verdict.describe()); @@ -574,7 +574,7 @@ pub async fn prepare_restart(app: &AppHandle) -> RestartReadiness { /// Come back, once the installer has finished and returned. pub fn complete_restart(app: &AppHandle) -> ! { - app.restart() + crate::process_spawn::with_spawn(|| app.restart()) } /// Stop the runtime this app owns and confirm it is gone. diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index d4e3dec7e32..1aa62294d56 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -46,6 +46,7 @@ mod tray_login; #[allow(dead_code)] #[path = "popup.rs"] mod popup_portable_test; +mod process_spawn; mod proxy; mod resolve; mod runtime_stop; diff --git a/desktop/src-tauri/src/login_registration.rs b/desktop/src-tauri/src/login_registration.rs index c88e7dd4e47..4adf3f78550 100644 --- a/desktop/src-tauri/src/login_registration.rs +++ b/desktop/src-tauri/src/login_registration.rs @@ -535,10 +535,7 @@ mod command { .stderr(Stdio::piped()) .spawn() }; - #[cfg(test)] - let child = crate::cli_command_record::tests::with_spawn(spawn); - #[cfg(not(test))] - let child = spawn(); + let child = crate::process_spawn::with_spawn(spawn); let mut child = child.map_err(|_| RunError::Failed)?; let readers = [drain(child.stdout.take()), drain(child.stderr.take())]; let status = loop { @@ -589,7 +586,7 @@ mod command { // writers may hold the test spawn lock longer than this test's timing budget. const CHILD: &str = "OCX_LOGIN_COMMAND_DEADLINE_CHILD"; if std::env::var_os(CHILD).is_none() { - let output = crate::cli_command_record::tests::with_spawn(|| { + let output = crate::process_spawn::with_spawn(|| { Command::new(std::env::current_exe().unwrap()) .args([ "--exact", diff --git a/desktop/src-tauri/src/process_spawn.rs b/desktop/src-tauri/src/process_spawn.rs new file mode 100644 index 00000000000..94ad6ab5488 --- /dev/null +++ b/desktop/src-tauri/src/process_spawn.rs @@ -0,0 +1,209 @@ +//! Keep Unix forks from inheriting a writable descriptor for a soon-to-be-published executable. +//! CLOEXEC only closes it at exec; closing the parent's copy does not close a child's copy. + +// A queued writer must not block another spawner: a plugin launcher can wait for the main +// thread, where a second launcher may run. std::RwLock may prefer the queued writer and deadlock +// those two readers. Use reader preference; only an active writer excludes process creation. +#[cfg(unix)] +static EXECUTABLE_IO: Gate = Gate::new(); + +#[cfg(unix)] +struct Gate { + access: std::sync::Mutex, + changed: std::sync::Condvar, +} + +#[cfg(unix)] +struct Access { + spawners: usize, + writing: bool, +} + +#[cfg(unix)] +struct Guard<'a> { + gate: &'a Gate, + writing: bool, +} + +#[cfg(unix)] +impl Gate { + const fn new() -> Self { + Self { + access: std::sync::Mutex::new(Access { + spawners: 0, + writing: false, + }), + changed: std::sync::Condvar::new(), + } + } + + fn access(&self) -> std::sync::MutexGuard<'_, Access> { + self.access + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + + fn acquire(&self, writing: bool) -> Guard<'_> { + let mut access = self + .changed + .wait_while(self.access(), |s| s.writing || (writing && s.spawners != 0)) + .unwrap_or_else(std::sync::PoisonError::into_inner); + if writing { + access.writing = true; + } else { + access.spawners += 1; + } + Guard { + gate: self, + writing, + } + } +} + +#[cfg(unix)] +impl Drop for Guard<'_> { + fn drop(&mut self) { + let mut access = self.gate.access(); + if self.writing { + access.writing = false; + } else { + access.spawners -= 1; + } + self.gate.changed.notify_all(); + } +} + +#[cfg(unix)] +pub(crate) fn executable_write() -> impl Drop { + EXECUTABLE_IO.acquire(true) +} + +/// Cover process creation through exec, not the subsequent wait for the child's exit. +/// Never request an executable writer from a spawner, or call this from a writer/pre_exec hook. +pub(crate) fn with_spawn(spawn: impl FnOnce() -> T) -> T { + #[cfg(unix)] + let _guard = EXECUTABLE_IO.acquire(false); + spawn() +} + +/// The pinned shell plugin's output future spawns synchronously when polled. Guard each poll +/// so creation is covered even for a lazy future; release before Pending, never across await. +pub(crate) async fn with_spawn_async(future: F) -> F::Output { + let mut future = std::pin::pin!(future); + std::future::poll_fn(|cx| with_spawn(|| future.as_mut().poll(cx))).await +} + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use std::{ + future::{poll_fn, Future}, + sync::{mpsc, Arc}, + task::{Context, Poll, Wake, Waker}, + time::Duration, + }; + + #[test] + fn a_spawn_waits_for_the_writer_and_releases_before_child_exit() { + let writer = executable_write(); + let (started_tx, started_rx) = mpsc::channel(); + let (spawned_tx, spawned_rx) = mpsc::channel(); + let spawner = std::thread::spawn(move || { + started_tx.send(()).unwrap(); + let child = with_spawn(|| { + std::process::Command::new("/bin/cat") + .stdin(std::process::Stdio::piped()) + .spawn() + }); + spawned_tx.send(child).unwrap(); + }); + started_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + let early = spawned_rx.recv_timeout(Duration::from_millis(100)); + drop(writer); + let mut child = match early { + Err(mpsc::RecvTimeoutError::Timeout) => spawned_rx + .recv_timeout(Duration::from_secs(5)) + .unwrap() + .unwrap(), + Ok(Ok(mut child)) => { + drop(child.stdin.take()); + let _ = child.wait(); + panic!("spawn overlapped an executable writer"); + } + _ => panic!("spawn failed before the writer was released"), + }; + spawner.join().unwrap(); + // cat is still waiting for EOF, but executable publication must already be unblocked. + let writer = executable_write(); + assert!(child.try_wait().unwrap().is_none()); + drop(writer); + drop(child.stdin.take()); + assert!(child.wait().unwrap().success()); + } + + #[test] + fn queued_writer_does_not_block_a_launchers_main_thread_callback() { + // An independent instance of the production gate keeps unrelated fixture/spawn stress + // from consuming this scheduling test's deadline. + let gate = Arc::new(Gate::new()); + let launcher = gate.acquire(false); + let writer_gate = gate.clone(); + let (started_tx, started_rx) = mpsc::channel(); + let (done_tx, done_rx) = mpsc::channel(); + let writer = std::thread::spawn(move || { + started_tx.send(()).unwrap(); + let _writer = writer_gate.acquire(true); + done_tx.send(()).unwrap(); + }); + started_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + assert!(matches!( + done_rx.recv_timeout(Duration::from_millis(100)), + Err(mpsc::RecvTimeoutError::Timeout) + )); + // Model an install helper waiting for a main-thread callback that launches a URL. + let callback = gate.acquire(false); + assert_eq!(gate.access().spawners, 2); + drop(callback); + drop(launcher); + done_rx.recv_timeout(Duration::from_secs(5)).unwrap(); + writer.join().unwrap(); + } + + struct Noop; + impl Wake for Noop { + fn wake(self: Arc) {} + } + + #[test] + fn async_spawn_is_guarded_on_poll_without_holding_a_lock_across_pending() { + let mut polled = false; + let future = with_spawn_async(poll_fn(|_| { + assert!( + EXECUTABLE_IO.access().spawners > 0, + "async spawn poll must hold shared access" + ); + if polled { + Poll::Ready(42) + } else { + polled = true; + Poll::Pending + } + })); + fn assert_send(_: &impl Send) {} + assert_send(&future); + let mut future = std::pin::pin!(future); + let waker = Waker::from(Arc::new(Noop)); + let mut cx = Context::from_waker(&waker); + assert_eq!(future.as_mut().poll(&mut cx), Poll::Pending); + // A paused output reader cannot stop a reconcile from writing its shim. + drop(executable_write()); + assert_eq!(future.as_mut().poll(&mut cx), Poll::Ready(42)); + } + + #[test] + fn failed_spawns_release_the_guard() { + let result = with_spawn(|| std::process::Command::new("/ocx-missing-executable").spawn()); + assert!(result.is_err()); + drop(executable_write()); + } +} diff --git a/desktop/src-tauri/src/resolve.rs b/desktop/src-tauri/src/resolve.rs index 1f9ddcfdda0..7e590e53189 100644 --- a/desktop/src-tauri/src/resolve.rs +++ b/desktop/src-tauri/src/resolve.rs @@ -304,7 +304,12 @@ pub async fn run(app: &AppHandle, deadline: Instant) -> Resolution { return Resolution::Unknown(format!("the bundled CLI could not be started ({error})")) } }; - match timeout_at(deadline, command.output()).await { + match timeout_at( + deadline, + crate::process_spawn::with_spawn_async(command.output()), + ) + .await + { Ok(Ok(output)) => read(output.status.code(), &output.stdout, &output.stderr), Ok(Err(error)) => { Resolution::Unknown(format!("the bundled CLI could not be run ({error})")) diff --git a/desktop/src-tauri/src/runtime_stop.rs b/desktop/src-tauri/src/runtime_stop.rs index c92528a5f4b..664da927067 100644 --- a/desktop/src-tauri/src/runtime_stop.rs +++ b/desktop/src-tauri/src/runtime_stop.rs @@ -270,7 +270,12 @@ async fn run_with_args(app: &AppHandle, deadline: Instant, argv: Vec) -> return StopResult::Failed(format!("the bundled CLI could not be started ({error})")) } }; - match timeout_at(deadline, command.output()).await { + match timeout_at( + deadline, + crate::process_spawn::with_spawn_async(command.output()), + ) + .await + { Ok(Ok(output)) => read(output.status.code(), &output.stdout, &output.stderr), Ok(Err(error)) => StopResult::Failed(format!("the bundled CLI could not be run ({error})")), Err(_) => { diff --git a/desktop/src-tauri/src/sidecar.rs b/desktop/src-tauri/src/sidecar.rs index 71f16db8f0a..e601498fe0f 100644 --- a/desktop/src-tauri/src/sidecar.rs +++ b/desktop/src-tauri/src/sidecar.rs @@ -205,7 +205,8 @@ pub fn start( .args(["start", "--port", &endpoint.port.to_string()]) .env("OPENCODEX_GUI_DIST", gui_dist) .env(SUPERVISED_ENV, "1"); - let (events, child) = command.spawn().map_err(|error| error.to_string())?; + let (events, child) = + crate::process_spawn::with_spawn(|| command.spawn()).map_err(|error| error.to_string())?; let watch = watch.for_child(child.pid()); watch.follow(events); Ok(child) diff --git a/desktop/src-tauri/src/tray.rs b/desktop/src-tauri/src/tray.rs index c702bc7c240..ec76efdf4a7 100644 --- a/desktop/src-tauri/src/tray.rs +++ b/desktop/src-tauri/src/tray.rs @@ -224,9 +224,10 @@ pub fn install(app: &AppHandle) -> tauri::Result<()> { else { return; }; - let _ = app - .opener() - .open_url(format!("{}#/usage", endpoint.url("/")), None::); + let _ = crate::process_spawn::with_spawn(|| { + app.opener() + .open_url(format!("{}#/usage", endpoint.url("/")), None::) + }); } "start-at-login" => { #[cfg(target_os = "macos")] diff --git a/desktop/src-tauri/src/tray_login.rs b/desktop/src-tauri/src/tray_login.rs index 5126c1f44e7..3a1948ed855 100644 --- a/desktop/src-tauri/src/tray_login.rs +++ b/desktop/src-tauri/src/tray_login.rs @@ -151,10 +151,11 @@ fn perform(app: &AppHandle, action: ClickAction) { .map_err(|error| error.to_string()), ClickAction::Enable => crate::login_autostart::enable(app), // launchd keeps its override whatever the plist says; the person turns it back on. - ClickAction::OpenLoginItems => app - .opener() - .open_url(login_registration::LOGIN_ITEMS_SETTINGS, None::) - .map_err(|error| error.to_string()), + ClickAction::OpenLoginItems => crate::process_spawn::with_spawn(|| { + app.opener() + .open_url(login_registration::LOGIN_ITEMS_SETTINGS, None::) + }) + .map_err(|error| error.to_string()), ClickAction::Nothing => Ok(()), }; if let Err(error) = result { diff --git a/desktop/src-tauri/src/updater.rs b/desktop/src-tauri/src/updater.rs index bcaf3349533..cc02ae9e9fc 100644 --- a/desktop/src-tauri/src/updater.rs +++ b/desktop/src-tauri/src/updater.rs @@ -410,7 +410,7 @@ pub async fn install(app: &AppHandle, update: Update) -> Result<(), String> { )); } - if let Err(error) = update.install(package) { + if let Err(error) = crate::process_spawn::with_spawn(|| update.install(package)) { // The installer returned a failure. The runtime was stopped for an install that did not // happen, so the app brings one back instead of sitting drained. recover_after_failed_install(app); diff --git a/desktop/src-tauri/src/window.rs b/desktop/src-tauri/src/window.rs index 5c69ae17fe7..7b71614c64a 100644 --- a/desktop/src-tauri/src/window.rs +++ b/desktop/src-tauri/src/window.rs @@ -67,7 +67,9 @@ fn opens_in_default_browser(url: &Url) -> bool { pub fn open_in_default_browser(url: &Url) { if opens_in_default_browser(url) { - let _ = tauri_plugin_opener::open_url(url.as_str(), None::<&str>); + let _ = crate::process_spawn::with_spawn(|| { + tauri_plugin_opener::open_url(url.as_str(), None::<&str>) + }); } } diff --git a/structure/desktop-shell.md b/structure/desktop-shell.md index 9b86ae2b237..482aac3340a 100644 --- a/structure/desktop-shell.md +++ b/structure/desktop-shell.md @@ -55,6 +55,7 @@ a page with an overlay title bar and no strip cannot be dragged or zoomed at all Linux keep the native title bar: the shell ships no min/max/close widgets of its own, and the sidebar-top layout applies unchanged beneath it. +Process creation shares the Unix guard with executable publication; see [Desktop-owned terminal command](desktop-terminal-command.md). ## Startup, quit and the tray The window is created and shown before anything is registered, resolved, probed or started, and diff --git a/structure/desktop-terminal-command.md b/structure/desktop-terminal-command.md index 00cecda8efa..d4159c3ef0b 100644 --- a/structure/desktop-terminal-command.md +++ b/structure/desktop-terminal-command.md @@ -23,10 +23,20 @@ Every private path must be user-owned without extended ACLs (macOS/Linux) or car user/SYSTEM/Administrators DACL (Windows root; children inherit it), otherwise nothing is written. Atomic replacement flushes and closes its write handle before publishing the new file by rename, so the published executable is never held open for writing by the replacing thread. -Unix Rust tests share a read/write lock between executable fixture writers and subprocess spawns. -Spawns may overlap each other, but no child can inherit a fixture's writable descriptor before exec -closes it; closing the parent's close-on-exec descriptor alone does not release a forked duplicate. -The lock exists only in test builds and leaves ordinary record writes concurrent with spawn stress. +`desktop/src-tauri/src/process_spawn.rs` owns a production process-wide Unix read/write gate. +Executable atomic writes take it exclusively before opening the temporary file and release it after +closing and publishing; ordinary record and rc writes do not take it. Process creation takes a shared +guard through exec, so no child inherits the writable executable descriptor. Closing only the parent's +close-on-exec descriptor would leave a forked duplicate writable until that child's exec. +The same guard covers runtime start (`sidecar.rs`), resolve, claim and stop output futures, macOS +launchctl/plutil probes (`login_registration.rs`), browser and Login Items launchers, updater install +helpers and app restart. Shell-plugin output futures take the guard while being polled, including the +initial synchronous spawn, and release it before yielding; waiting for output never retains a guard. +The configured LaunchAgent registration and Linux autostart write files without spawning commands. +Windows uses the same call sites without a lock. Rust fixture writers and spawners use this production +mechanism too; tests cover exclusion in both directions, spawn errors and async Pending release. +A queued writer permits further spawns until existing launchers finish, avoiding deadlock when a plugin +waits for a main-thread callback that launches another process. Only an active writer blocks spawns. `desktop/src-tauri/src/cli_command_posix.rs` generates a fail-closed shim, shared PATH helper and final managed zsh/bash/fish blocks. Reconcile separates desired rc targets from recorded diff --git a/tests/clients/desktop-cli-contracts.test.ts b/tests/clients/desktop-cli-contracts.test.ts index a4934212d7a..edf9c884c9f 100644 --- a/tests/clients/desktop-cli-contracts.test.ts +++ b/tests/clients/desktop-cli-contracts.test.ts @@ -108,7 +108,7 @@ describe("desktop CLI contracts", () => { const body = runner.slice(0, runner.indexOf("\n}")); // A missing binary, a failed spawn and a deadline all answer the same way. expect(body.match(/Resolution::Unknown/g) || []).toHaveLength(3); - expect(body).toContain("timeout_at(deadline, command.output())"); + expect(body).toContain("crate::process_spawn::with_spawn_async(command.output())"); }); test("the startup sequence refuses to start on anything but a proven absence", () => { diff --git a/tests/clients/desktop-startup-surface.test.ts b/tests/clients/desktop-startup-surface.test.ts index 76cc454bb2a..41f869cae95 100644 --- a/tests/clients/desktop-startup-surface.test.ts +++ b/tests/clients/desktop-startup-surface.test.ts @@ -85,7 +85,7 @@ describe("desktop startup surface", () => { test("the spawn event stream is consumed rather than discarded", () => { const sidecar = code(SIDECAR); expect(sidecar).not.toContain("_events"); - expect(sidecar).toContain("let (events, child) = command.spawn()"); + expect(sidecar).toMatch(/let \(events, child\) =\s*crate::process_spawn::with_spawn\(\|\| command.spawn\(\)\)/); expect(sidecar).toContain("watch.follow(events)"); expect(sidecar).toContain("CommandEvent::Terminated(payload)"); }); @@ -121,7 +121,7 @@ describe("desktop startup surface", () => { expect(proxy).toContain("timeout_at(deadline, self.is_alive())"); // The stop is the bundled CLI's now, under its own deadline. const stop = code(repoPath("desktop/src-tauri/src/runtime_stop.rs")); - expect(stop).toContain("timeout_at(deadline, command.output())"); + expect(stop).toContain("crate::process_spawn::with_spawn_async(command.output())"); expect(stop).toContain("pub const DEADLINE: Duration"); });