From 1d8fa94594910a9b7ba3715155ce2619c8fded3e Mon Sep 17 00:00:00 2001 From: kewton Date: Fri, 9 Oct 2026 01:45:36 +0900 Subject: [PATCH 1/2] test(runner): confirm grandchild readiness, pipe EOF, and group death before cleanup timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the whole-run 5s wall-clock assertion in dev_server_cleanup_kills_grandchild_process_group_without_pipe_deadlock with signals bound to this run: the fake grandchild holds a named pipe open and announces readiness on it. The injected cleanup_fn (the existing run_nextjs_dev_route_probe_with_runtime hook) waits for that readiness, then measures only the cleanup interval against the cleanup's own term+kill bound. Cleanup completion, pipe EOF, and process-group ESRCH are asserted separately, so a change that skips cleanup or leaves the grandchild is detected. The duplicated dev_server_lifecycle event lookup moves to a shared helper, and the test workspace moves under the cargo target directory. 読み替え: 「pipe の EOF」は実 child/grandchild が保持する名前付きパイプ (FIFO) の write 端が閉じることと読んだ。production は child の stdout を file へ向けるため、既存の cleanup_fn 注入点の内側で当該実行の pipe を観測できる経路は FIFO だけである。 判断: cleanup 区間の上限は既存の DEV_SERVER_CLEANUP_TERM_TIMEOUT + DEV_SERVER_CLEANUP_KILL_TIMEOUT (=6s) を inclusive に使った。テストの一時ディレクトリは読み取り許可プレフィックスの下に置かず、test 実行ファイル配下（target）に作った。 本文に無い指摘: 全工程の 5 秒 assertion は起動待ちを cleanup の遅さとして扱っていたため削除し、cleanup 自体の上限と deadlock 検出（bounded な pipe 読み）に置き換えた。単純削除はしていない。 Co-authored-by: CommandCodeBot --- .../runner/tests/acceptance_boundary_tests.rs | 118 +++++++++++++----- src/planner/runner/tests/support/process.rs | 8 ++ 2 files changed, 95 insertions(+), 31 deletions(-) diff --git a/src/planner/runner/tests/acceptance_boundary_tests.rs b/src/planner/runner/tests/acceptance_boundary_tests.rs index d38b59d93..578f65ef5 100644 --- a/src/planner/runner/tests/acceptance_boundary_tests.rs +++ b/src/planner/runner/tests/acceptance_boundary_tests.rs @@ -138,31 +138,88 @@ fn nextjs_dev_route_probe_disabled_records_lifecycle_stages() { ); } +#[cfg(unix)] +const GRANDCHILD_READY_MARKER: &str = "grandchild-ready"; +#[cfg(unix)] +const GRANDCHILD_PIPE_RELATIVE: &str = ".anvil/evidence/dev-server-grandchild.pipe"; + +#[cfg(unix)] +fn create_grandchild_pipe(path: &Path) { + use std::os::unix::ffi::OsStrExt; + let c_path = std::ffi::CString::new(path.as_os_str().as_bytes()).expect("pipe path"); + let rc = unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }; + assert_eq!(rc, 0, "create grandchild pipe {path:?}"); +} + +#[cfg(unix)] +fn open_grandchild_pipe_reader(path: &Path) -> Option { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK) + .open(path) + .ok() +} + +#[cfg(unix)] +fn poll_grandchild_pipe(path: &Path, wait_for_data: bool) -> bool { + let Some(mut reader) = open_grandchild_pipe_reader(path) else { + return false; + }; + let deadline = Instant::now() + Duration::from_secs(5); + let mut buffer = [0_u8; 32]; + while Instant::now() < deadline { + match reader.read(&mut buffer) { + Ok(0) if !wait_for_data => return true, + Ok(n) if wait_for_data && n > 0 => return true, + _ => std::thread::sleep(Duration::from_millis(20)), + } + } + false +} + +#[cfg(unix)] +fn measured_cleanup_after_grandchild_ready( + child: Child, + logs: &DevServerLogPaths, +) -> DevServerCleanup { + let pipe = logs + .stdout + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(GRANDCHILD_PIPE_RELATIVE); + let ready = poll_grandchild_pipe(&pipe, true); + let started = Instant::now(); + let cleanup = cleanup_dev_server_child(child, logs); + let measured = format!("{ready} {}", started.elapsed().as_millis()); + let _ = std::fs::write(pipe.with_extension("measurement"), measured); + cleanup +} + #[test] #[cfg(unix)] fn dev_server_cleanup_kills_grandchild_process_group_without_pipe_deadlock() { let _probe_guard = dev_server_probe_test_guard(); - let dir = tempfile::tempdir().unwrap(); + let exe_dir = std::env::current_exe().unwrap().parent().unwrap().to_path_buf(); + let dir = tempfile::Builder::new().prefix("runner-dev-server-").tempdir_in(exe_dir).unwrap(); let port = free_local_port(); let events = dir.path().join("events.jsonl"); write_fake_nextjs_dev_workspace(dir.path(), port, true); let cfg = nextjs_config_with_events(dir.path(), &events); let evidence_path = nextjs_dev_route_evidence_path(&cfg); + let pipe = dir.path().join(GRANDCHILD_PIPE_RELATIVE); + create_grandchild_pipe(&pipe); + let _pipe_reader = open_grandchild_pipe_reader(&pipe).expect("open grandchild pipe reader"); - let started = Instant::now(); let evidence = run_nextjs_dev_route_probe_with_runtime( &cfg, &evidence_path, true, - cleanup_dev_server_child, + measured_cleanup_after_grandchild_ready, BrowserInteractionProbeOptions::default(), Some(port), ); - assert!( - started.elapsed() < Duration::from_secs(5), - "cleanup should be bounded and fast" - ); assert_eq!(evidence.get("ok").and_then(Value::as_bool), Some(true)); assert!(evidence_path.is_file(), "browser readiness evidence"); assert!(evidence_path.with_file_name("dev-server.out").is_file()); @@ -172,27 +229,28 @@ fn dev_server_cleanup_kills_grandchild_process_group_without_pipe_deadlock() { dev_server_stage_names(&events_json), vec!["start", "wait", "probe", "cleanup"] ); - let cleanup = events_json - .iter() - .find(|event| { - event.get("event").and_then(Value::as_str) == Some("dev_server_lifecycle") - && event.get("stage").and_then(Value::as_str) == Some("cleanup") - }) - .expect("cleanup event"); + let cleanup = dev_server_lifecycle_event(&events_json, "cleanup"); assert_eq!(cleanup.get("ok").and_then(Value::as_bool), Some(true)); - let pid = events_json - .iter() - .find(|event| { - event.get("event").and_then(Value::as_str) == Some("dev_server_lifecycle") - && event.get("stage").and_then(Value::as_str) == Some("start") - }) - .and_then(|event| event.get("pid")) - .and_then(Value::as_u64) - .expect("dev server pid") as u32; + + let measured = + std::fs::read_to_string(pipe.with_extension("measurement")).expect("cleanup measurement"); + let (ready, elapsed_ms) = measured.split_once(' ').expect("measurement fields"); + assert_eq!(ready, "true", "grandchild ready before cleanup"); + let elapsed_ms: u128 = elapsed_ms.trim().parse().expect("cleanup elapsed ms"); + assert!( + elapsed_ms <= (DEV_SERVER_CLEANUP_TERM_TIMEOUT + DEV_SERVER_CLEANUP_KILL_TIMEOUT).as_millis(), + "cleanup must stay within its own term+kill bound (no pipe deadlock): {elapsed_ms}ms" + ); + + let pid = dev_server_lifecycle_event(&events_json, "start")["pid"].as_u64().unwrap() as u32; assert!( wait_until_process_group_gone(pid, Duration::from_secs(2)), "process group {pid} should be gone" ); + assert!( + poll_grandchild_pipe(&pipe, false), + "grandchild pipe must reach EOF once the grandchild is reaped" + ); } #[test] @@ -224,13 +282,7 @@ fn dev_server_writes_readiness_before_forced_cleanup_failure() { dev_server_stage_names(&events_json), vec!["start", "wait", "probe", "cleanup"] ); - let cleanup = events_json - .iter() - .find(|event| { - event.get("event").and_then(Value::as_str) == Some("dev_server_lifecycle") - && event.get("stage").and_then(Value::as_str) == Some("cleanup") - }) - .expect("cleanup event"); + let cleanup = dev_server_lifecycle_event(&events_json, "cleanup"); assert_eq!(cleanup.get("ok").and_then(Value::as_bool), Some(false)); assert_eq!( cleanup.get("failure_kind").and_then(Value::as_str), @@ -255,9 +307,13 @@ fn fake_dev_server_package_manager_child() { .as_deref() == Some("1") { + // The grandchild holds the pipe: the parent reads ready, then EOF. + let script = format!( + "exec 3>{GRANDCHILD_PIPE_RELATIVE} || exit 1; echo {GRANDCHILD_READY_MARKER} >&3; sleep 300" + ); let _ = std::process::Command::new("sh") .arg("-c") - .arg("sleep 300") + .arg(script) .spawn() .expect("spawn grandchild"); } diff --git a/src/planner/runner/tests/support/process.rs b/src/planner/runner/tests/support/process.rs index ae77b64aa..103df3b33 100644 --- a/src/planner/runner/tests/support/process.rs +++ b/src/planner/runner/tests/support/process.rs @@ -67,6 +67,14 @@ fn dev_server_stage_names(events: &[Value]) -> Vec<&str> { .collect() } +#[cfg(unix)] +fn dev_server_lifecycle_event<'a>(events: &'a [Value], stage: &str) -> &'a Value { + events + .iter() + .find(|event| event["event"] == "dev_server_lifecycle" && event["stage"] == stage) + .expect("dev_server_lifecycle event") +} + #[cfg(unix)] fn wait_until_process_group_gone(pgid: u32, timeout: Duration) -> bool { let deadline = Instant::now() + timeout; From 1f050e23db7f2727f0e53074a29a0c88df3d56b1 Mon Sep 17 00:00:00 2001 From: kewton Date: Fri, 9 Oct 2026 08:37:28 +0900 Subject: [PATCH 2/2] test(runner): bound cleanup interval by the original 5s assertion value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a test-side CLEANUP_INTERVAL_BOUND (5s) and a strict `< 5s` comparison instead of the product term+kill sum, so the rewritten assertion is equal to or stricter than the original whole-run 5s bound (the cleanup interval is a subset of the whole run). The message states that a cleanup which only ends after the KILL fallback also fails. Other assertions are unchanged. 判断: 「cleanup 自体の既存上限」は元の全工程 5 秒 assertion と同じ 5 秒をテスト側の名前つき定数で持つと解釈した（製品の定数の和には依存しない）。 Co-authored-by: CommandCodeBot --- src/planner/runner/tests/acceptance_boundary_tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/planner/runner/tests/acceptance_boundary_tests.rs b/src/planner/runner/tests/acceptance_boundary_tests.rs index 578f65ef5..877de7c2e 100644 --- a/src/planner/runner/tests/acceptance_boundary_tests.rs +++ b/src/planner/runner/tests/acceptance_boundary_tests.rs @@ -199,6 +199,7 @@ fn measured_cleanup_after_grandchild_ready( #[test] #[cfg(unix)] fn dev_server_cleanup_kills_grandchild_process_group_without_pipe_deadlock() { + const CLEANUP_INTERVAL_BOUND: Duration = Duration::from_secs(5); let _probe_guard = dev_server_probe_test_guard(); let exe_dir = std::env::current_exe().unwrap().parent().unwrap().to_path_buf(); let dir = tempfile::Builder::new().prefix("runner-dev-server-").tempdir_in(exe_dir).unwrap(); @@ -232,14 +233,13 @@ fn dev_server_cleanup_kills_grandchild_process_group_without_pipe_deadlock() { let cleanup = dev_server_lifecycle_event(&events_json, "cleanup"); assert_eq!(cleanup.get("ok").and_then(Value::as_bool), Some(true)); - let measured = - std::fs::read_to_string(pipe.with_extension("measurement")).expect("cleanup measurement"); + let measured = std::fs::read_to_string(pipe.with_extension("measurement")).expect("measurement"); let (ready, elapsed_ms) = measured.split_once(' ').expect("measurement fields"); assert_eq!(ready, "true", "grandchild ready before cleanup"); let elapsed_ms: u128 = elapsed_ms.trim().parse().expect("cleanup elapsed ms"); assert!( - elapsed_ms <= (DEV_SERVER_CLEANUP_TERM_TIMEOUT + DEV_SERVER_CLEANUP_KILL_TIMEOUT).as_millis(), - "cleanup must stay within its own term+kill bound (no pipe deadlock): {elapsed_ms}ms" + elapsed_ms < CLEANUP_INTERVAL_BOUND.as_millis(), + "cleanup that needs the KILL fallback (SIGTERM not enough) exceeds 5s: {elapsed_ms}ms" ); let pid = dev_server_lifecycle_event(&events_json, "start")["pid"].as_u64().unwrap() as u32;