From 5bd1174dddb409d4ccf0d322652a22eab8310dd2 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Thu, 17 Sep 2026 10:49:35 -0600 Subject: [PATCH] fix: isolate the adversary review-stack lock for parallel tests and non-POSIX platforms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The advisory flock in run_checks.py uses one system-wide lock path (~/.cache/coyote/review-stack.lock). Concurrent runs of the timeout/deadline tests contend on it: the losers sleep in the bounded retry loop, the total deadline (computed before lock acquisition) expires, and commands record 'skipped: deadline' instead of the expected timeout/kill behavior. - run_checks.py: ADVERSARY_STACK_LOCK_PATH env override relocates the lock file (isolation seam, documented alongside the other env seams); every other lock semantic (default path, bounded wait, proceed-unlocked warning, fd held for process lifetime, skip when no commands) is unchanged. - run_checks.py: guard the POSIX-only fcntl import — windows-latest CI has python3 and runs these tests, so the unconditional import crashed the script there; on non-POSIX the lock is now skipped with a stderr WARN, matching the script's documented non-POSIX limitations. - tests: the shared adversary_script_command helper points every spawned script at its own throwaway lock path, isolating the run_checks tests from each other and from any real review running on the same machine. - tests: adversary_run_checks_stack_lock_path_is_env_overridable pins the override (lock file appears at the overridden path across two sequential runs; no lock file is created when no commands are declared). --- assets/agents/adversary/scripts/run_checks.py | 20 ++++-- src/config/request_context.rs | 72 +++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/assets/agents/adversary/scripts/run_checks.py b/assets/agents/adversary/scripts/run_checks.py index c90d605..cbb9a48 100644 --- a/assets/agents/adversary/scripts/run_checks.py +++ b/assets/agents/adversary/scripts/run_checks.py @@ -31,7 +31,9 @@ in-script degradation would never fire. Each command gets min(PER_COMMAND_TIMEOUT_SECS, remaining budget); commands the deadline leaves no budget for are recorded as {"cmd", "skipped": "deadline"} entries, so the in-script handling stays authoritative. The env var -ADVERSARY_RUN_CHECKS_DEADLINE_SECS overrides the deadline (test seam). +ADVERSARY_RUN_CHECKS_DEADLINE_SECS overrides the deadline (test seam), and +ADVERSARY_STACK_LOCK_PATH overrides the review-stack lock file location +(isolation seam — see acquire_stack_lock). Process hygiene: commands run through the shell. On POSIX each command gets its own session (start_new_session) and a timeout SIGKILLs the whole process @@ -51,14 +53,20 @@ GRAPH_STATE_FILE, so an inherited live state file would silently win. import json import os import signal -import fcntl import subprocess import sys import time +try: + import fcntl +except ImportError: # non-POSIX (Windows): advisory flock unavailable + fcntl = None + PER_COMMAND_TIMEOUT_SECS = 900 TOTAL_DEADLINE_SECS = float(os.environ.get("ADVERSARY_RUN_CHECKS_DEADLINE_SECS") or 3300) -STACK_LOCK_PATH = os.path.expanduser("~/.cache/coyote/review-stack.lock") +STACK_LOCK_PATH = os.environ.get("ADVERSARY_STACK_LOCK_PATH") or os.path.expanduser( + "~/.cache/coyote/review-stack.lock" +) STACK_LOCK_WAIT_SECS = float(os.environ.get("ADVERSARY_STACK_LOCK_WAIT_SECS") or 1200) @@ -67,7 +75,11 @@ def acquire_stack_lock(): well-known path; the probe is instructed to flock the same file around stack boot/teardown). Bounded wait so a hogged lock can never turn into a false red verification: on timeout we proceed WITHOUT the lock and note it - on stderr. The fd is held for the process lifetime (released on exit).""" + on stderr. The fd is held for the process lifetime (released on exit). + On non-POSIX platforms (no fcntl) the lock is skipped entirely.""" + if fcntl is None: + sys.stderr.write("WARN: review-stack lock unsupported on this platform; running unlocked\n") + return None try: os.makedirs(os.path.dirname(STACK_LOCK_PATH), exist_ok=True) fd = os.open(STACK_LOCK_PATH, os.O_CREAT | os.O_RDWR) diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 7daf797..a495047 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -10443,13 +10443,24 @@ mod tests { // Scripts' load_state() prefers GRAPH_STATE_FILE over GRAPH_STATE, so an // inherited live state file (adversary verifying this repo) must not win. + // run_checks.py's review-stack flock defaults to ONE system-wide path; + // concurrent tests (or a real review running on this machine) would + // contend on it, and the losers' 5s-sleep retry loop burns the tight + // test deadlines. Every spawned script gets its own throwaway lock path. fn adversary_script_command(script: &str, state: &serde_json::Value) -> Command { + static STACK_LOCK_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("assets/agents/adversary/scripts") .join(script); + let lock_path = env::temp_dir().join(format!( + "coyote-adversary-test-stack-{}-{}.lock", + std::process::id(), + STACK_LOCK_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + )); let mut cmd = Command::new("python3"); cmd.arg(&path) .env("GRAPH_STATE", state.to_string()) + .env("ADVERSARY_STACK_LOCK_PATH", &lock_path) .env_remove("GRAPH_STATE_FILE"); cmd } @@ -11041,6 +11052,67 @@ mod tests { ); } + /// ADVERSARY_STACK_LOCK_PATH must relocate the review-stack flock: + /// without it, every concurrent run_checks.py (parallel tests, or a real + /// review on the same machine) contends on the one system-wide lock path + /// and the losers' retry loop burns the total deadline budget. + #[test] + #[cfg(unix)] + fn adversary_run_checks_stack_lock_path_is_env_overridable() { + if !cmd_available("python3") || !cmd_available("sh") { + eprintln!("skipping: python3 or sh not available"); + return; + } + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let tmp = env::temp_dir().join(format!( + "coyote-adversary-run-checks-lockpath-{}-{unique}", + std::process::id() + )); + create_dir_all(&tmp).unwrap(); + // A not-yet-existing subdir pins the makedirs-on-override behavior. + let lock_path = tmp.join("override").join("stack.lock"); + let lock_env = lock_path.to_str().unwrap(); + let state = json!({"verification_commands": ["true"]}); + + // Two sequential runs with the same override: the lock fd is held + // only for the process lifetime, so the second run must acquire + // immediately — and both use the OVERRIDDEN location. + for run in 0..2 { + let out = run_adversary_script_env( + "run_checks.py", + &state, + &[("ADVERSARY_STACK_LOCK_PATH", lock_env)], + ); + assert_eq!( + out["exec_results"][0]["exit"], 0, + "run {run}: the declared command must run to completion: {out}" + ); + assert!( + lock_path.exists(), + "run {run}: the stack lock must be taken at the overridden path {}", + lock_path.display() + ); + } + + // No declared commands → the lock is skipped entirely: a fresh + // override path must never be created. + let skipped = tmp.join("skipped.lock"); + let _ = run_adversary_script_env( + "run_checks.py", + &json!({}), + &[("ADVERSARY_STACK_LOCK_PATH", skipped.to_str().unwrap())], + ); + assert!( + !skipped.exists(), + "with no declared commands the lock must be skipped, but {} was created", + skipped.display() + ); + let _ = remove_dir_all(&tmp); + } + /// TOTAL_DEADLINE_SECS in run_checks.py and the run_checks node's /// `timeout:` in graph.yaml live in different files; if the script's /// deadline ever creeps past the node timeout, the executor kills the