From d14e95f90ec856d5997a4f4d301cf1d38af534de Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Sat, 31 Oct 2026 10:35:26 +1100 Subject: [PATCH] test: stop desktop, buzz-agent and mobile tests leaking temp files (#8748) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Six test files wrote into the shared temp dir on every normal run and never removed what they wrote. **Desktop and buzz-agent (first commit)** - `desktop/src/protectedFeatures/tauriCommand.test.mjs` wrote its fake Tauri CLI and three result files straight into `/tmp` under pid-suffixed names. They now live under one `mkdtempSync` root that an `after()` hook removes. - `crates/buzz-agent/tests/permission_boundary.rs` wrote a `buzz_perm_calllog_*` file for every test whose tool call reached `fake_mcp`, five per run. `call_log_path` now returns a `TempDir` guard with the log path inside it, so the log is deleted when the test ends, including when an assertion panics. - `cancel_kills_inflight_tool_via_mcp_notification` in `crates/buzz-agent/tests/regressions.rs` spawns `buzz-dev-mcp`, which the agent stops with SIGKILL, so the server's `buzz-dev-mcp-*` and `buzz-dev-mcp-session-*` TempDirs were never deleted. The test now passes the server a `TMPDIR` it owns and removes. **Mobile (second commit)** The three mobile test files left 12 `buzz-video-*` files and 4 `transcode_*` dirs per run. - `video_viewer_test.dart` and `video_controls_test.dart` pointed their fake `path_provider` at `Directory.systemTemp`. The viewer downloads each video there as `buzz-video-*`, and its async delete on close never completes under FakeAsync. Each test now gets its own temp dir for the fake, and an `addTearDown` removes it with `deleteSync`. In `video_controls_test.dart` that teardown is registered first, so it runs after the viewer is disposed. - Four `media_upload_test.dart` tests faked `transcodeVideoToMp4` by writing `out.mp4` into a new `systemTemp` `transcode_*` dir. The upload service deletes `out.mp4` but not its dir. The fakes now create that dir under the test's own temp root, which the test already deletes in `finally`. Production code is unchanged. **`buzz-dev-mcp`'s SIGKILL shutdown is left for a follow-up:** the agent still stops the server with SIGKILL, so outside tests the server's temp dirs are still left behind. Sending SIGTERM first is the real fix. ### Related issue None found. This continues the test-cleanup work in #8727. ### Testing Each run below was bracketed by listings of `/tmp` and the per-user temp dir (`/var/folders/.../T`), so new leftovers would show up. - `tauriCommand.test.mjs`: 3/3 pass, with no new `buzz-tauri-*` or `buzz-fake-tauri-*` entries. - `cargo test -p buzz-agent --test permission_boundary`: 9/9 pass, with no new `buzz_perm_calllog_*` files. The tests still check that the call log is written, so the log really is going into the guarded dir. - `cancel_kills_inflight_tool_via_mcp_notification` passes with `buzz-dev-mcp` built, so it ran rather than skipped, and left no new `buzz-dev-mcp-*` dirs. - Mobile: `video_viewer_test` (7 tests), `video_controls_test` (20) and `media_upload_test` (38) pass, each run on its own. With `TMPDIR` pointed at a scratch dir, the old versions left 12 `buzz-video-*` files and 4 `transcode_*` dirs, and the new versions left it empty. - After cherry-picking onto current main, these pass: - `cargo clippy -p buzz-agent --all-targets -- -D warnings` - biome and `node --check` on `tauriCommand.test.mjs` - `dart format` and `flutter analyze` on the three mobile files 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Matt Toohey Co-authored-by: Claude Opus 5.5 --- .../buzz-agent/tests/permission_boundary.rs | 38 +++++++++---------- crates/buzz-agent/tests/regressions.rs | 6 ++- .../protectedFeatures/tauriCommand.test.mjs | 26 ++++++------- .../video_controls_test.dart | 14 ++++++- .../media_viewer_page/video_viewer_test.dart | 18 +++++++-- .../test/shared/relay/media_upload_test.dart | 12 ++---- 6 files changed, 65 insertions(+), 49 deletions(-) diff --git a/crates/buzz-agent/tests/permission_boundary.rs b/crates/buzz-agent/tests/permission_boundary.rs index 2873ba14d6..b7bfc5494e 100644 --- a/crates/buzz-agent/tests/permission_boundary.rs +++ b/crates/buzz-agent/tests/permission_boundary.rs @@ -136,18 +136,16 @@ async fn drive( // Session init + call-log helpers // ───────────────────────────────────────────────────────────────────────────── -fn call_log_path(tag: &str) -> String { - let p = std::env::temp_dir().join(format!( - "buzz_perm_calllog_{tag}_{}_{:x}.log", - std::process::id(), - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos() - )); - let s = p.to_string_lossy().to_string(); - let _ = std::fs::remove_file(&s); - s +/// A fresh call-log path inside its own temp dir. Keep the returned `TempDir` +/// bound for the whole test: dropping it deletes the log, including when an +/// assertion panics. +fn call_log_path(tag: &str) -> (tempfile::TempDir, String) { + let dir = tempfile::Builder::new() + .prefix(&format!("buzz_perm_calllog_{tag}_")) + .tempdir() + .expect("create call-log temp dir"); + let path = dir.path().join("calls.log").to_string_lossy().into_owned(); + (dir, path) } /// Invoked tool names recorded by fake_mcp (bare names, one per `tools/call`). @@ -214,7 +212,7 @@ fn stop_reason(resp: &Value) -> String { /// the tally ends at one. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_allow_reaches_tool_exactly_once_and_not_before_approval() { - let log = call_log_path("allow"); + let (_log_dir, log) = call_log_path("allow"); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("done")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( @@ -250,7 +248,7 @@ async fn test_allow_reaches_tool_exactly_once_and_not_before_approval() { /// sees a permission-denied tool error (the turn continues to `end_turn`). #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_reject_option_never_invokes_tool() { - let log = call_log_path("reject"); + let (_log_dir, log) = call_log_path("reject"); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("understood")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( @@ -321,7 +319,7 @@ async fn test_adversarial_outcomes_never_invoke_tool() { ]; for (tag, build) in shapes { - let log = call_log_path(tag); + let (_log_dir, log) = call_log_path(tag); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("ok")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( @@ -352,7 +350,7 @@ async fn test_adversarial_outcomes_never_invoke_tool() { /// waiter; the correct id then authorizes exactly once. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_stale_id_ignored_then_real_id_authorizes() { - let log = call_log_path("staleid"); + let (_log_dir, log) = call_log_path("staleid"); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("done")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( @@ -406,7 +404,7 @@ async fn test_stale_id_ignored_then_real_id_authorizes() { /// by staying silent, and cancellation must not depend on that answer. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_cancel_while_waiting_executes_nothing_without_client_answer() { - let log = call_log_path("cancelwait"); + let (_log_dir, log) = call_log_path("cancelwait"); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("done")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( @@ -452,7 +450,7 @@ async fn test_cancel_while_waiting_executes_nothing_without_client_answer() { /// proves only the allowed (second) call reached MCP — order-agnostic. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_crossed_parallel_decisions_authorize_only_matching_call() { - let log = call_log_path("crossed"); + let (_log_dir, log) = call_log_path("crossed"); // Two distinct registered tools so the call log distinguishes them by name. let llm = spawn_capturing_llm(vec![ openai_tool_calls(&[ @@ -547,7 +545,7 @@ async fn test_load_skill_emits_no_permission_request() { /// objects once, forcing a hook invocation the test can observe. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_stop_hook_reaches_mcp_without_permission_ask() { - let log = call_log_path("stophook"); + let (_log_dir, log) = call_log_path("stophook"); // Text turn triggers the _Stop gate → hook objects once → agent loops → // second text turn, hook silent → end_turn. No model-issued tool call. let llm = spawn_capturing_llm(vec![ @@ -599,7 +597,7 @@ async fn test_stop_hook_reaches_mcp_without_permission_ask() { /// with an end-to-end proof that the negotiated version reaches the wire. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_v2_negotiation_emits_v2_request_shape() { - let log = call_log_path("v2shape"); + let (_log_dir, log) = call_log_path("v2shape"); let llm = spawn_capturing_llm(vec![shell_call("tc1"), openai_text("done")]).await; let mut h = Harness::spawn(&llm.url).await; let sid = init( diff --git a/crates/buzz-agent/tests/regressions.rs b/crates/buzz-agent/tests/regressions.rs index a6feb989b6..5d488d0dff 100644 --- a/crates/buzz-agent/tests/regressions.rs +++ b/crates/buzz-agent/tests/regressions.rs @@ -1425,6 +1425,10 @@ async fn cancel_kills_inflight_tool_via_mcp_notification() { ]) .await; + // The agent stops buzz-dev-mcp with SIGKILL, so the server never deletes + // its own temp dirs. Point its TMPDIR at a dir this test owns and removes. + // Declared before `h` so it drops after the harness. + let dev_mcp_tmp = tempfile::TempDir::new().expect("create buzz-dev-mcp TMPDIR"); let mut h = Harness::spawn(&llm.url).await; let sid = init_session( &mut h, @@ -1432,7 +1436,7 @@ async fn cancel_kills_inflight_tool_via_mcp_notification() { "name": "dev", "command": &dev_mcp_bin, "args": [], - "env": [] + "env": [{"name": "TMPDIR", "value": dev_mcp_tmp.path()}] }]), ) .await; diff --git a/desktop/src/protectedFeatures/tauriCommand.test.mjs b/desktop/src/protectedFeatures/tauriCommand.test.mjs index 0902be92b4..cc598df131 100644 --- a/desktop/src/protectedFeatures/tauriCommand.test.mjs +++ b/desktop/src/protectedFeatures/tauriCommand.test.mjs @@ -1,8 +1,8 @@ import assert from "node:assert/strict"; -import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; -import { test } from "node:test"; +import { after, test } from "node:test"; import { fileURLToPath } from "node:url"; import { spawn } from "node:child_process"; @@ -12,7 +12,11 @@ const desktopRoot = path.resolve( ); const configDir = path.join(desktopRoot, "src-tauri"); const wrapper = path.join(desktopRoot, "scripts/tauri-command.mjs"); -const fakeCli = path.join(tmpdir(), `buzz-fake-tauri-${process.pid}.mjs`); +// Everything these tests write lives under one private root, removed once the +// file's tests finish. +const tempRoot = mkdtempSync(path.join(tmpdir(), "buzz-tauri-command-test-")); +after(() => rmSync(tempRoot, { recursive: true, force: true })); +const fakeCli = path.join(tempRoot, "fake-tauri.mjs"); writeFileSync( fakeCli, @@ -65,10 +69,8 @@ function packageVariant(variant, result, runnerArguments = []) { } test("opposite Tauri package variants own private frontend artifacts", async () => { - const resultRoot = path.join(tmpdir(), `buzz-tauri-results-${process.pid}`); - mkdirSync(resultRoot, { recursive: true }); - const ossResult = path.join(resultRoot, "oss.json"); - const internalResult = path.join(resultRoot, "internal.json"); + const ossResult = path.join(tempRoot, "oss.json"); + const internalResult = path.join(tempRoot, "internal.json"); await Promise.all([ packageVariant("0", ossResult), @@ -83,10 +85,7 @@ test("opposite Tauri package variants own private frontend artifacts", async () }); test("private config precedes Cargo runner arguments", async () => { - const result = path.join( - tmpdir(), - `buzz-tauri-runner-arguments-${process.pid}.json`, - ); + const result = path.join(tempRoot, "runner-arguments.json"); await packageVariant("0", result, [ "--config", '{"bundle":{"active":false}}', @@ -106,10 +105,7 @@ test("private config precedes Cargo runner arguments", async () => { }); test("private frontendDist is never mistaken for a URL", async () => { - const result = path.join( - tmpdir(), - `buzz-tauri-frontend-dist-${process.pid}.json`, - ); + const result = path.join(tempRoot, "frontend-dist.json"); await packageVariant("0", result); const invocation = JSON.parse(readFileSync(result, "utf8")); diff --git a/mobile/test/features/channels/media_viewer_page/video_controls_test.dart b/mobile/test/features/channels/media_viewer_page/video_controls_test.dart index 35b2d1ea78..8e713374a4 100644 --- a/mobile/test/features/channels/media_viewer_page/video_controls_test.dart +++ b/mobile/test/features/channels/media_viewer_page/video_controls_test.dart @@ -19,8 +19,12 @@ import 'package:video_player/video_player.dart'; class _TempDirectory extends Fake with MockPlatformInterfaceMixin implements PathProviderPlatform { + _TempDirectory(this.path); + + final String path; + @override - Future getTemporaryPath() async => Directory.systemTemp.path; + Future getTemporaryPath() async => path; } class _Player extends VideoPlayerPlatform { @@ -136,8 +140,14 @@ Future<_Player> _pumpVideo( final oldPlayer = VideoPlayerPlatform.instance; final oldPath = PathProviderPlatform.instance; final player = _Player(size: videoSize); + // Registered first so it runs after the viewer is disposed. The viewer's + // async delete of its buzz-video-* file never completes under FakeAsync. + final temporaryDirectory = Directory.systemTemp.createTempSync( + 'video_controls_test_', + ); + addTearDown(() => temporaryDirectory.deleteSync(recursive: true)); VideoPlayerPlatform.instance = player; - PathProviderPlatform.instance = _TempDirectory(); + PathProviderPlatform.instance = _TempDirectory(temporaryDirectory.path); addTearDown(() { VideoPlayerPlatform.instance = oldPlayer; PathProviderPlatform.instance = oldPath; diff --git a/mobile/test/features/channels/media_viewer_page/video_viewer_test.dart b/mobile/test/features/channels/media_viewer_page/video_viewer_test.dart index 4da0ae486e..36daa96cad 100644 --- a/mobile/test/features/channels/media_viewer_page/video_viewer_test.dart +++ b/mobile/test/features/channels/media_viewer_page/video_viewer_test.dart @@ -57,9 +57,12 @@ import '../../../helpers/widget_helpers.dart'; class _FakePathProviderPlatform extends Fake with MockPlatformInterfaceMixin implements PathProviderPlatform { + _FakePathProviderPlatform(this.temporaryPath); + + final String temporaryPath; + @override - Future getTemporaryPath() async => - Directory.systemTemp.resolveSymbolicLinksSync(); + Future getTemporaryPath() async => temporaryPath; } /// Fake VideoPlayerPlatform that tracks `dispose` calls and optionally @@ -370,7 +373,16 @@ void main() { TestWidgetsFlutterBinding.ensureInitialized(); setUp(() { - PathProviderPlatform.instance = _FakePathProviderPlatform(); + // The viewer's async delete of its downloaded buzz-video-* file never + // completes under FakeAsync, so give each test its own temp dir and + // remove it synchronously. + final temporaryDirectory = Directory( + Directory.systemTemp.resolveSymbolicLinksSync(), + ).createTempSync('video_viewer_test_'); + addTearDown(() => temporaryDirectory.deleteSync(recursive: true)); + PathProviderPlatform.instance = _FakePathProviderPlatform( + temporaryDirectory.path, + ); }); // Transport: request sink must be closed before send(). diff --git a/mobile/test/shared/relay/media_upload_test.dart b/mobile/test/shared/relay/media_upload_test.dart index dd72efe73e..e162e5bebc 100644 --- a/mobile/test/shared/relay/media_upload_test.dart +++ b/mobile/test/shared/relay/media_upload_test.dart @@ -1417,7 +1417,7 @@ void main() { pickGalleryImage: () async => null, transcodeVideoToMp4: (path) async { transcodeCalled = true; - final outDir = await Directory.systemTemp.createTemp('transcode_'); + final outDir = await tempFile.parent.createTemp('transcode_'); final outFile = File('${outDir.path}/out.mp4'); await outFile.writeAsBytes(buildFtypHeader('isom')); return outFile.path; @@ -1466,7 +1466,7 @@ void main() { transcodeVideoToMp4: (path) async { transcodeCalled = true; // Mock transcoding: write an "MP4" file - final outDir = await Directory.systemTemp.createTemp('transcode_'); + final outDir = await tempFile.parent.createTemp('transcode_'); final outFile = File('${outDir.path}/out.mp4'); await outFile.writeAsBytes(buildFtypHeader('isom')); return outFile.path; @@ -1576,9 +1576,7 @@ void main() { pickGalleryVideo: () async => xfile, pickGalleryImage: () async => null, transcodeVideoToMp4: (path) async { - final outDir = await Directory.systemTemp.createTemp( - 'transcode_', - ); + final outDir = await tempFile.parent.createTemp('transcode_'); final outFile = File('${outDir.path}/out.mp4'); await outFile.writeAsBytes(buildFtypHeader('isom')); return outFile.path; @@ -1635,9 +1633,7 @@ void main() { pickGalleryVideo: () async => xfile, pickGalleryImage: () async => null, transcodeVideoToMp4: (path) async { - final outDir = await Directory.systemTemp.createTemp( - 'transcode_', - ); + final outDir = await tempFile.parent.createTemp('transcode_'); final outFile = File('${outDir.path}/out.mp4'); await outFile.writeAsBytes(buildFtypHeader('isom')); return outFile.path;