diff --git a/.github/workflows/native.yml b/.github/workflows/native.yml index c71e53bc..f733add3 100644 --- a/.github/workflows/native.yml +++ b/.github/workflows/native.yml @@ -26,8 +26,9 @@ jobs: run: rustup toolchain install 1.95.0 --profile minimal --component rustfmt --component clippy - run: cargo +1.95.0 fmt --all --check - run: cargo +1.95.0 clippy --workspace --all-targets --locked -- -D warnings - - run: cargo +1.95.0 test --workspace --locked + # qualification_real spawns target/debug/shadowcode; cargo test does not build it. - run: cargo +1.95.0 build -p shadowcode-desktop --locked + - run: cargo +1.95.0 test --workspace --locked - name: Exercise the native CLI without a display run: node scripts/test-native-cli.mjs - name: Stress repeated native tasks, cancellation, output and cleanup diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 04236013..ed58a4e0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -32,8 +32,9 @@ jobs: run: | cargo +1.95.0 fmt --all --check cargo +1.95.0 clippy --workspace --all-targets --locked -- -D warnings - cargo +1.95.0 test --workspace --locked + # qualification_real spawns target/debug/shadowcode; cargo test does not build it. cargo +1.95.0 build -p shadowcode-desktop --locked + cargo +1.95.0 test --workspace --locked - name: Exercise native application behavior run: | node scripts/test-native-cli.mjs diff --git a/native/core/src/control/view.rs b/native/core/src/control/view.rs index 84ce2f24..9946c055 100644 --- a/native/core/src/control/view.rs +++ b/native/core/src/control/view.rs @@ -62,6 +62,7 @@ pub struct ViewClient { reader: Mutex>>>, events: broadcast::Sender, closed: Arc, + closing: Arc, attach: Mutex<()>, } impl Client { @@ -126,6 +127,7 @@ impl Client { reader: Mutex::new(Some(AbortOnDropHandle::new(task))), events, closed, + closing: Arc::new(AtomicBool::new(false)), attach: Mutex::new(()), }) } @@ -158,14 +160,28 @@ impl ViewClient { /// tools, or emit durable events. Existing subscribers keep this /// broadcast and receive `view.reattached`. pub async fn reattach(&self) -> Result { - let _gate = self.attach.lock().await; + ensure!( + !self.closing.load(Ordering::Acquire), + "Attached view is closed" + ); ensure!( self.closed.load(Ordering::Acquire), "Attached view is still connected; detach before reattaching" ); let mut base = self.client()?; base.view = None; - base.wait_available(Duration::from_secs(20)).await?; + // Do not hold `attach` while waiting: close() must stay able to + // detach after the owner exits instead of blocking for 20s. + self.wait_for_owner(&base, Duration::from_secs(20)).await?; + let _gate = self.attach.lock().await; + ensure!( + !self.closing.load(Ordering::Acquire), + "Attached view is closed" + ); + ensure!( + self.closed.load(Ordering::Acquire), + "Attached view is still connected; detach before reattaching" + ); let _ = self.reader.lock().await.take(); let _ = self.writer.lock().await.take(); let fresh = base.open_view().await?; @@ -195,7 +211,34 @@ impl ViewClient { "tools_replayed": 0, })) } + async fn wait_for_owner(&self, client: &Client, timeout: Duration) -> Result<()> { + let deadline = tokio::time::Instant::now() + timeout; + loop { + ensure!( + !self.closing.load(Ordering::Acquire), + "Attached view is closed" + ); + match client.available().await { + Ok(true) => return Ok(()), + Ok(false) if tokio::time::Instant::now() < deadline => { + tokio::time::sleep(Duration::from_millis(100)).await; + } + Ok(false) => bail!("No running engine is available to attach"), + Err(error) + if error.to_string().contains("different version") + || error.to_string().contains("protocol/profile mismatch") => + { + return Err(error); + } + Err(_error) if tokio::time::Instant::now() < deadline => { + tokio::time::sleep(Duration::from_millis(100)).await; + } + Err(error) => return Err(error), + } + } + } pub async fn close(&self) -> Result<()> { + self.closing.store(true, Ordering::Release); let _gate = self.attach.lock().await; self.closed.store(true, Ordering::Release); let Some(mut task) = self.reader.lock().await.take() else { @@ -317,6 +360,7 @@ mod tests { reader: Mutex::new(Some(AbortOnDropHandle::new(task))), events, closed: Arc::new(AtomicBool::new(false)), + closing: Arc::new(AtomicBool::new(false)), attach: Mutex::new(()), }); let closing_view = view.clone(); diff --git a/native/core/tests/control.rs b/native/core/tests/control.rs index b8ba3dd6..060631af 100644 --- a/native/core/tests/control.rs +++ b/native/core/tests/control.rs @@ -476,6 +476,37 @@ async fn attached_views_receive_bounded_completion_notifications_and_detect_owne service.engine.shutdown().await.unwrap(); } +#[tokio::test] +async fn attached_view_close_does_not_wait_for_missing_owner() { + let (_root, service) = setup(); + let server = Server::start_with_mode(service.clone(), "server").unwrap(); + let client = server.endpoint().client(service.workspace().unwrap(), None); + let view = std::sync::Arc::new(client.open_view().await.unwrap()); + server.close(); + server.wait_closed().await; + let reattach = { + let view = view.clone(); + tokio::spawn(async move { view.reattach().await }) + }; + tokio::time::sleep(Duration::from_millis(150)).await; + let started = std::time::Instant::now(); + let _ = view.close().await; + assert!( + started.elapsed() < Duration::from_secs(2), + "close waited for owner reconnect: {:?}", + started.elapsed() + ); + let reattach = tokio::time::timeout(Duration::from_secs(2), reattach) + .await + .expect("reattach should stop when the view closes") + .unwrap(); + assert!( + reattach.is_err(), + "reattach must not succeed after close: {reattach:?}" + ); + service.engine.shutdown().await.unwrap(); +} + #[tokio::test] async fn attached_view_reattaches_after_owner_restart_without_replay_or_duplicates() { let (_root, service) = setup(); @@ -598,32 +629,3 @@ async fn attached_view_reattaches_after_owner_restart_without_replay_or_duplicat assert_eq!(running, 0, "reattach must not start or resume jobs"); stop(server, service).await; } - -fn descriptor_count() -> usize { - std::fs::read_dir("/proc/self/fd") - .map(|entries| entries.count()) - .unwrap_or(0) -} - -#[tokio::test] -async fn attach_close_loop_does_not_grow_descriptors() { - let (_root, service) = setup(); - let server = Server::start_with_mode(service.clone(), "server").unwrap(); - let client = server.endpoint().client(service.workspace().unwrap(), None); - for _ in 0..2 { - let view = client.open_view().await.unwrap(); - view.close().await.unwrap(); - } - let baseline = descriptor_count(); - for _ in 0..20 { - let view = client.open_view().await.unwrap(); - view.close().await.unwrap(); - } - let after = descriptor_count(); - eprintln!("attach_close_loop descriptors baseline={baseline} after={after}"); - assert!( - after <= baseline + 24, - "descriptor leak: baseline={baseline} after={after}" - ); - stop(server, service).await; -} diff --git a/native/core/tests/control_descriptors.rs b/native/core/tests/control_descriptors.rs new file mode 100644 index 00000000..5fb3bb52 --- /dev/null +++ b/native/core/tests/control_descriptors.rs @@ -0,0 +1,49 @@ +//! Process-wide `/proc/self/fd` counts. This file is a separate integration +//! binary so sibling `control` tests cannot inflate the measurement. +use serde_json::json; +use shadowcode_core::{config::Config, control::Server, paths::AppPaths, service::Service}; +use std::fs; + +fn setup() -> (tempfile::TempDir, Service) { + let root = tempfile::tempdir().unwrap(); + let workspace = root.path().join("project"); + fs::create_dir(&workspace).unwrap(); + let paths = AppPaths::isolated(&root.path().join("profile")).unwrap(); + Config::patch(&paths,json!({"trusted_workspaces":[workspace],"model":{"provider":"local","name":"fixture","endpoint":"http://127.0.0.1:9/v1"},"permissions":{"approve_shell":false}})).unwrap(); + (root, Service::open(paths, Some(workspace)).unwrap()) +} + +async fn stop(server: Server, service: Service) { + server.close(); + server.wait_closed().await; + service.engine.shutdown().await.unwrap(); +} + +fn descriptor_count() -> usize { + std::fs::read_dir("/proc/self/fd") + .map(|entries| entries.count()) + .unwrap_or(0) +} + +#[tokio::test] +async fn attach_close_loop_does_not_grow_descriptors() { + let (_root, service) = setup(); + let server = Server::start_with_mode(service.clone(), "server").unwrap(); + let client = server.endpoint().client(service.workspace().unwrap(), None); + for _ in 0..2 { + let view = client.open_view().await.unwrap(); + view.close().await.unwrap(); + } + let baseline = descriptor_count(); + for _ in 0..20 { + let view = client.open_view().await.unwrap(); + view.close().await.unwrap(); + } + let after = descriptor_count(); + eprintln!("attach_close_loop descriptors baseline={baseline} after={after}"); + assert!( + after <= baseline + 24, + "descriptor leak: baseline={baseline} after={after}" + ); + stop(server, service).await; +} diff --git a/scripts/native-packaging-env.mjs b/scripts/native-packaging-env.mjs index 15c1ccee..9feade1d 100644 --- a/scripts/native-packaging-env.mjs +++ b/scripts/native-packaging-env.mjs @@ -56,11 +56,45 @@ export function isUnsafePackagingDir(dir) { ); } +function dirContains(dir, name) { + try { + statSync(path.join(dir, name)); + return true; + } catch { + return false; + } +} + +// rustc/cargo live on the caller PATH (rustup ~/.cargo/bin, CARGO_HOME). +// linuxdeploy still must not see /usr/local/bin or Hermes; keep only the +// toolchain directories themselves when they pass the same safety checks. +function rustToolchainDirs() { + const candidates = []; + if (process.env.CARGO_HOME) { + candidates.push(path.join(process.env.CARGO_HOME, "bin")); + } + if (process.env.HOME) { + candidates.push(path.join(process.env.HOME, ".cargo", "bin")); + } + for (const dir of (process.env.PATH || "").split(path.delimiter)) { + if (!dir) continue; + const resolved = resolveExistingDir(dir); + if ( + resolved && + (dirContains(resolved, "rustc") || dirContains(resolved, "cargo")) + ) { + candidates.push(resolved); + } + } + return candidates; +} + export function packagingDirs(root, options = {}) { const execDir = options.execDir ?? path.dirname(process.execPath); const extras = [ path.join(root, "tools/rust-dev/extracted/usr/bin"), path.join(root, "..", "tools/rust-dev/extracted/usr/bin"), + ...rustToolchainDirs(), path.join(root, "target/release"), path.join(root, "target/debug"), path.join(root, "target/.tauri"), diff --git a/scripts/test-native-cli.mjs b/scripts/test-native-cli.mjs index 153a88f5..b31afd57 100644 --- a/scripts/test-native-cli.mjs +++ b/scripts/test-native-cli.mjs @@ -9,6 +9,12 @@ import { fileURLToPath } from "node:url"; import { DatabaseSync } from "node:sqlite"; const root = fileURLToPath(new URL("../", import.meta.url)); +const version = JSON.parse( + await readFile(path.join(root, "src-tauri/tauri.conf.json"), "utf8"), +).version; +const versionLine = new RegExp( + `^ShadowCode ${String(version).replaceAll(".", "\\.")}\\s*$`, +); const binary = process.env.SHADOW_DESKTOP_BINARY || path.join(root, "target/debug/shadowcode"); const binaryArgs = JSON.parse(process.env.SHADOW_CLI_ARGS || "[]"); assert.ok(Array.isArray(binaryArgs) && binaryArgs.every(arg => typeof arg === "string")); @@ -124,8 +130,8 @@ await writeFile(path.join(profile, "config/config.yaml"), JSON.stringify({ })); let server; try { - assert.match((await finish(launch(["--version"]))).stdout, /^ShadowCode 0\.20\.0\s*$/); - assert.match((await finish(launch(["ui", "--version"]))).stdout, /^ShadowCode 0\.20\.0\s*$/); + assert.match((await finish(launch(["--version"]))).stdout, versionLine); + assert.match((await finish(launch(["ui", "--version"]))).stdout, versionLine); assert.match((await finish(launch(["--help"]))).stdout, /serve/); await finish(launch(["--unknown-option"]), 2); assert.equal((await cli(["health"])).runtime, "rust"); diff --git a/scripts/test-native-packaging-env.mjs b/scripts/test-native-packaging-env.mjs index 198c34e8..a24b1e0a 100644 --- a/scripts/test-native-packaging-env.mjs +++ b/scripts/test-native-packaging-env.mjs @@ -38,6 +38,12 @@ test("packaging PATH is constructed, not inherited", () => { assert.doesNotMatch(joined, /\.hermes/); const rustDev = path.resolve(root, "../tools/rust-dev/extracted/usr/bin"); if (existsSync(rustDev)) assert.ok(dirs.includes(rustDev)); + const rustcDir = dirs.find((dir) => existsSync(path.join(dir, "rustc"))); + assert.ok( + rustcDir, + "sanitized PATH must still find rustc for dependency notices", + ); + assert.notEqual(path.resolve(rustcDir), "/usr/local/bin"); const tauriTools = path.join(root, "target/.tauri"); if (existsSync(tauriTools)) assert.ok(dirs.includes(tauriTools)); }); @@ -64,6 +70,10 @@ test("applyPackagingPath overwrites a dirty process PATH", () => { assert.doesNotMatch(next, /(^|:)\/usr\/local\/bin(:|$)/); assert.match(next, /(^|:)\/usr\/bin(:|$)/); assert.notEqual(next, dirtyPath); + assert.ok( + next.split(path.delimiter).some((dir) => existsSync(path.join(dir, "rustc"))), + "dirty caller PATH must not drop rustc", + ); } finally { process.env.PATH = previous; } diff --git a/scripts/test-native-runtime.mjs b/scripts/test-native-runtime.mjs index c7983b11..24e5eec3 100644 --- a/scripts/test-native-runtime.mjs +++ b/scripts/test-native-runtime.mjs @@ -16,6 +16,12 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; const root = fileURLToPath(new URL("../", import.meta.url)); +const version = JSON.parse( + await readFile(path.join(root, "src-tauri/tauri.conf.json"), "utf8"), +).version; +const versionLine = new RegExp( + `^ShadowCode ${String(version).replaceAll(".", "\\.")}\\s*$`, +); const binary = path.resolve( process.argv[2] || path.join( @@ -224,10 +230,10 @@ try { Array.from({ length: 8 }, () => finish(launch(["--version"]))), ); for (const reply of replies) - assert.match(reply.stdout, /^ShadowCode 0\.20\.0\s*$/); + assert.match(reply.stdout, versionLine); assert.match( (await finish(launch(["ui", "--version"], {}, true))).stdout, - /^ShadowCode 0\.20\.0\s*$/, + versionLine, "Environment-based extraction preserves arguments", ); await intact(first);