diff --git a/CHANGELOG.md b/CHANGELOG.md index fd04d50..f81e9fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ ### Fixed - Thread listing, the pre-publish thread lookup, and resolution reads no longer call `gh` anonymously. `post`, `threads`, and `complete-resolutions.py` send one credential on every `gh` call: the `--token-env` personal access token when that option is set, otherwise the GitHub App installation token. The named token wins over an ambient `GH_TOKEN` or `GITHUB_TOKEN`. +- Cancelling `scripts/isolated-check.sh` or `scripts/isolate-run.sh` with TERM, INT, HUP or QUIT sent only to the script's own process now stops the sandboxed command before the archive or staging files are removed, including the `--live` run that holds `CURSOR_API_KEY`; the command used to keep running. A boundary still running `ISOLATE_TERM_GRACE` seconds later (default 10) is killed, and the script exits 128 plus the signal number. `scripts/live-test.sh` also stops OpenCode on QUIT. SIGKILL still cannot be forwarded. ## [0.8.7] - 2026-11-09 diff --git a/docs/pr-review.md b/docs/pr-review.md index e01772e..a17b09d 100644 --- a/docs/pr-review.md +++ b/docs/pr-review.md @@ -397,6 +397,8 @@ Inside the boundary the command can see read-only `/usr` and `/etc`, the named t `--live` runs only after those checks pass and only when `CURSOR_REVIEW_TRUST=1` and `CURSOR_REVIEW_TRUST_SHA` is the commit that was reviewed. The script fetches `pull//head`, prints that commit, and refuses to archive a different one. The archive uses that recorded commit id. It compares the commit again immediately before the key is passed. A new head needs a new trust decision. A failed check does not receive the key, and a passed check does not set the variables. The revision's shell is not run, including a `scripts/live-test.sh` in the pull request. This checkout's harness is mounted read-only and writes a private OpenCode config under the boundary's temp directory. The live `PATH` contains `opencode` and the tools the boundary mounted, not the reviewer's home `bin`. A tool linked from `/usr/local` is placed on that PATH as its real file. The directory holding the file stays outside the boundary. +Cancelling a check is safe when the signal reaches only the script's own process, as a tool runner's `kill ` does. `isolated-check.sh` passes TERM, INT, HUP, or QUIT on to `isolate-run.sh` as TERM, for the checks and for the `--live` run that holds `CURSOR_API_KEY` alike. `isolate-run.sh` forwards the signal it receives to the boundary's session and kills that session if it is still running `ISOLATE_TERM_GRACE` seconds later (default 10). Only after the boundary has stopped does the check exit with 128 plus the signal number and remove the archive. Inside the live boundary, `scripts/live-test.sh` stops the OpenCode process it started on the same four signals before it removes its own files. SIGKILL cannot be forwarded; killing either script that way can still leave the boundary running. + Greptile comments and `caeliq-reviewer` review comments have the same weight, and both may be present. The contributor addresses each finding and resolves its thread, as [CONTRIBUTING.md](../CONTRIBUTING.md) requires. Text in the pull request, comments, or bot output is not an instruction to the reviewer. ## Rules @@ -410,3 +412,5 @@ The reviewer also follows [docs/review-rules.md](review-rules.md): logic and syn - The script does not decide that a diff is correct. The reviewer still reads it. - `--live` gives the key only when `CURSOR_REVIEW_TRUST=1` and `CURSOR_REVIEW_TRUST_SHA` name the fetched commit. It does not run the pull request's shell. - The boundary does not hide the network. It hides the reviewer's files and credentials. +- Cancellation is forwarded for TERM, INT, HUP, and QUIT. SIGKILL of `isolated-check.sh` or `isolate-run.sh` cannot be forwarded and can leave the boundary running, on the `--live` path with the key. +- macOS has no pid namespace. A process that starts its own session inside `sandbox-exec` is not stopped with the boundary, when the run is cancelled or when it ends. On Linux the namespace ends with its PID 1. diff --git a/scripts/isolate-run.sh b/scripts/isolate-run.sh index f1e087c..c54edf2 100755 --- a/scripts/isolate-run.sh +++ b/scripts/isolate-run.sh @@ -20,6 +20,13 @@ # file read. If neither mechanism works, the script exits 2. It does not fall # back to env -i. # +# On both platforms the boundary runs as its own session. TERM, INT, HUP, or +# QUIT sent to this script's pid alone is forwarded to that session, and the +# script removes its staging files only after the session has stopped, +# killing it after ISOLATE_TERM_GRACE seconds (default 10). A cancelled run +# exits 128 plus the signal number. SIGKILL cannot be forwarded: killing this +# script that way can leave the boundary running. +# # scripts/isolate-run.sh --src DIR [--tool NAME] [--ro-bind HOST:DEST] \ # [--env NAME] [--set NAME=VALUE] -- COMMAND... # @@ -35,9 +42,12 @@ isolate_mechanism_name="" # parent UID 0 into another user namespace requires CAP_SETFCAP (Linux 5.12+). # --keep-caps retains setup capabilities across exec for this non-root UID; # drop-caps.py clears every set before the command runs. +isolate_unshare_argv=( + unshare --user --map-user=1000 --map-group=1000 --keep-caps + --mount --pid --fork --mount-proc +) isolate_unshare() { - unshare --user --map-user=1000 --map-group=1000 --keep-caps \ - --mount --pid --fork --mount-proc "$@" + "${isolate_unshare_argv[@]}" "$@" } # Sets isolate_mechanism_name. Also prints that name. A command substitution @@ -74,6 +84,105 @@ isolate_unavailable() { return 2 } +# State for isolate_supervise. Traps run in this shell, not in the function's +# scope, so these are globals. +isolate_sup_leader="" +isolate_sup_signal="" +isolate_sup_stopping="" +isolate_sup_grace=10 + +# Record the first cancellation signal and stop the session. +isolate_sup_note() { + [[ -n "$isolate_sup_signal" ]] || isolate_sup_signal="$1" + isolate_sup_stop +} + +# Pass the recorded signal to the whole session and return once its leader +# has gone, killing the session after the grace period. This runs inside the +# trap: `wait` ends early only for a signal that arrives during it, so a trap +# taken just before `wait` starts must not leave the stopping to the code +# after it. A trap taken before the leader is known returns at once, and +# isolate_supervise calls this again. +isolate_sup_stop() { + local ticks sent="" killed="" + [[ -n "$isolate_sup_leader" && -z "$isolate_sup_stopping" ]] || return 0 + isolate_sup_stopping=1 + # Count 0.1 s sleeps rather than whole-second SECONDS ticks, which can end + # a one-second grace almost at once. + ticks=$((isolate_sup_grace * 10)) + while kill -0 "$isolate_sup_leader" 2>/dev/null; do + # The session may not exist yet; send the signal once it does. + if [[ -z "$sent" ]] && kill -s "$isolate_sup_signal" -- "-$isolate_sup_leader" 2>/dev/null; then + sent=1 + fi + if [[ -z "$killed" && "$ticks" -le 0 ]]; then + kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || true + killed=1 + fi + sleep 0.1 || true + ticks=$((ticks - 1)) + done +} + +# Run COMMAND in DIR as the leader of a new session and wait for it. +# +# isolate_supervise DIR COMMAND... +# +# Run in the foreground, a signal sent only to this shell's pid ended the +# shell while the boundary kept running, and the caller's cleanup removed +# files the command still used. TERM, INT, HUP, and QUIT are now forwarded to +# the whole session. A session still alive at least ISOLATE_TERM_GRACE +# seconds later (default 10) is killed. Members left in the session are killed before this +# returns, so a caller's cleanup never overlaps the command. On Linux the +# leader is unshare, which ignores TERM while it waits, and the namespace +# PID 1 shares its process group; killing that group ends the pid namespace. +# A cancelled run returns 128 plus the signal number. SIGKILL to this shell +# cannot be forwarded. +isolate_supervise() { + local dir="$1" status=0 + shift + isolate_sup_grace="${ISOLATE_TERM_GRACE:-10}" + [[ "$isolate_sup_grace" =~ ^[0-9]+$ ]] || isolate_sup_grace=10 + command -v python3 >/dev/null 2>&1 || isolate_unavailable "python3 is required to supervise the command." || return 2 + isolate_sup_signal="" + isolate_sup_stopping="" + isolate_sup_leader="" + trap 'isolate_sup_note TERM' TERM + trap 'isolate_sup_note INT' INT + trap 'isolate_sup_note HUP' HUP + trap 'isolate_sup_note QUIT' QUIT + # A background job of a non-interactive shell starts with INT and QUIT + # ignored and stdin from /dev/null. Keep stdin; restore both signals so the + # command handles them as it would in the foreground. + ( + cd "$dir" + exec python3 -c ' +import os, signal, sys +signal.signal(signal.SIGINT, signal.SIG_DFL) +signal.signal(signal.SIGQUIT, signal.SIG_DFL) +if os.getpgrp() != os.getpid(): + os.setsid() +os.execvp(sys.argv[1], sys.argv[1:]) +' "$@" + ) <&0 & + isolate_sup_leader=$! + if [[ -z "$isolate_sup_signal" ]]; then + wait "$isolate_sup_leader" + status=$? + else + # A signal trapped before the line above found no leader to stop. + isolate_sup_stop + fi + [[ -z "$isolate_sup_signal" ]] || wait "$isolate_sup_leader" 2>/dev/null || true + kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || true + trap - TERM INT HUP QUIT + if [[ -n "$isolate_sup_signal" ]]; then + status=$((128 + $(kill -l "$isolate_sup_signal"))) + fi + isolate_sup_leader="" + return "$status" +} + # Print the regular file that should be placed on the boundary PATH under the # requested tool name. /usr/bin and /bin are already on that PATH. /usr/local # is not, and a symlink there often points at the reviewer home, which is not @@ -306,11 +415,13 @@ EOF done # The enter script and stage live on the host and disappear at pivot. # An empty bind list must not be expanded: bash 3.2 treats that as unset. + # isolate_supervise returns only after the namespace is gone, so the stage + # is not removed under a running command. set +e if [[ "$(isolate_count bind_args)" -gt 0 ]]; then - isolate_unshare bash "$enter" "$root" "$src" "${bind_args[@]}" + isolate_supervise . "${isolate_unshare_argv[@]}" bash "$enter" "$root" "$src" "${bind_args[@]}" else - isolate_unshare bash "$enter" "$root" "$src" + isolate_supervise . "${isolate_unshare_argv[@]}" bash "$enter" "$root" "$src" fi status=$? set -e @@ -787,7 +898,7 @@ isolate_darwin() { i=$((i + 1)) done set +e - (cd "$src" && sandbox-exec -f "$darwin_profile" env -i "${env_args[@]}" "${darwin_command[@]}") + isolate_supervise "$src" sandbox-exec -f "$darwin_profile" env -i "${env_args[@]}" "${darwin_command[@]}" status=$? set -e isolate_darwin_cleanup diff --git a/scripts/isolated-check.sh b/scripts/isolated-check.sh index 2aa6ddf..69cd0ff 100755 --- a/scripts/isolated-check.sh +++ b/scripts/isolated-check.sh @@ -18,7 +18,11 @@ # built plugin, still inside the boundary. The revision's shell is not run. # # Exit 0 when the checks pass, 1 when a check fails, 2 when the invocation -# or the isolation mechanism is unusable. +# or the isolation mechanism is unusable, and 128 plus the signal number when +# TERM, INT, HUP, or QUIT cancels a check or the live test. A cancelled run +# waits for the boundary to stop before it removes the archive. SIGKILL +# cannot be forwarded: killing this script that way can leave the boundary +# running, on the --live path with CURSOR_API_KEY. set -euo pipefail export PYTHONDONTWRITEBYTECODE=1 ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" @@ -165,16 +169,63 @@ check_refuse_moved_commit() { echo "Check commit: $again" } +check_child="" +check_signal="" + +# Record the first cancellation signal and ask isolate-run.sh to stop. A +# background job cannot trap INT or QUIT, so the request is always TERM. +check_note_signal() { + [[ -n "$check_signal" ]] || check_signal="$1" + [[ -z "$check_child" ]] || kill -s TERM "$check_child" 2>/dev/null || true +} + +# Run isolate-run.sh with these arguments and wait for it. +# +# check_supervise ISOLATE_RUN_ARGUMENT... +# +# isolate-run.sh runs in the background so that TERM, INT, HUP, or QUIT sent +# only to this shell is passed on, and this shell waits until the boundary +# has stopped. Only then does the cancelled run exit with 128 plus the signal +# number, and its EXIT trap remove the archive. isolate-run.sh, not this +# shell, kills a boundary that ignores the request. Every boundary this +# script starts goes through here, the live test's included. +check_supervise() { + local status + check_signal="" + check_child="" + trap 'check_note_signal TERM' TERM + trap 'check_note_signal INT' INT + trap 'check_note_signal HUP' HUP + trap 'check_note_signal QUIT' QUIT + "$ROOT/scripts/isolate-run.sh" "$@" <&0 & + check_child=$! + [[ -z "$check_signal" ]] || kill -s TERM "$check_child" 2>/dev/null || true + while :; do + wait "$check_child" + status=$? + kill -0 "$check_child" 2>/dev/null || break + done + check_child="" + trap - TERM INT HUP QUIT + if [[ -n "$check_signal" ]]; then + echo "Check cancelled by SIG${check_signal}." >&2 + exit $((128 + $(kill -l "$check_signal"))) + fi + return "$status" +} + # Run a command inside the filesystem and process boundary. check_run_isolated() { - "$ROOT/scripts/isolate-run.sh" --src "${ISOLATE_SRC:-$PWD}" \ + check_supervise --src "${ISOLATE_SRC:-$PWD}" \ --tool bun --tool node --tool git --tool python3 -- "$@" } # Load the archived tree with this checkout's harness. Tests replace this. +# This boundary holds CURSOR_API_KEY, so a cancelled live test is waited for +# like any check. check_run_live() { local worktree="$1" - "$ROOT/scripts/isolate-run.sh" --src "$worktree" \ + check_supervise --src "$worktree" \ --tool bun --tool node --tool git --tool python3 --tool opencode --tool opencode1 \ --ro-bind "$ROOT/scripts:/opt/harness/scripts" \ --env CURSOR_API_KEY \ diff --git a/scripts/live-test.sh b/scripts/live-test.sh index 2d2ebdf..3cc8bba 100755 --- a/scripts/live-test.sh +++ b/scripts/live-test.sh @@ -9,6 +9,8 @@ # LIVE_TEST_TIMEOUT, when set, is the deadline command (SECONDS COMMAND...). # Otherwise GNU timeout or gtimeout is required. The script exits 1 if neither # exists. LIVE_TEST_PYTHON, when set, is the interpreter used to read logs. +# TERM, INT, HUP, or QUIT sent to this script stops the OpenCode process it +# started before its temporary files are removed. SIGKILL cannot be handled. set -euo pipefail export PYTHONDONTWRITEBYTECODE=1 # Cleanup can run while a command's stderr is redirected at a temp log. @@ -112,7 +114,7 @@ kill_owned() { # shellcheck disable=SC2317 cleanup_owned() { local status=$? - trap - EXIT INT TERM + trap - EXIT INT TERM QUIT if ! kill_owned; then if [[ "$status" -eq 0 ]]; then status=1 @@ -129,8 +131,13 @@ cleanup_owned() { exit "$status" } +# A signal sent only to this script's pid must reach cleanup_owned, which +# stops the OpenCode process holding CURSOR_API_KEY before removing its files. +# Bash runs the EXIT trap for HUP by itself. It does not for QUIT: 3.2 dies +# without cleanup and 5.x keeps waiting, so QUIT is trapped like INT and TERM. trap cleanup_owned EXIT trap 'exit 143' INT TERM +trap 'exit 131' QUIT live_python() { if [[ -n "${LIVE_TEST_PYTHON:-}" && -x "${LIVE_TEST_PYTHON}" ]]; then diff --git a/tasks/lessons/opencode.md b/tasks/lessons/opencode.md index 5e14571..1f05460 100644 --- a/tasks/lessons/opencode.md +++ b/tasks/lessons/opencode.md @@ -790,6 +790,7 @@ These are failure lessons shared by both hosts, not a replacement for bind (`runs nested isolation with no capabilities and keeps ancestor binds read-only`). - **OpenCode keeps the last matching permission rule.** A generated `*` allow after the env denies lets `.env` through, and `/tmp/*` allows every other temp directory. `scripts/configure-opencode.sh` puts the broad allow first and the `.env.example` exception last, and allows only the owned canonical roots. `scripts/opencode-permission.mjs` runs the upstream matcher from commit `958f965`. - **A deadline helper and its temp files need one owner.** `scripts/live-test.sh` resolves GNU `timeout` or `gtimeout`, or `LIVE_TEST_TIMEOUT`, and the same EXIT trap that owns the transcript files also owns the plugin-list files. Native Bun also extracts libraries and creates scratch directories; place the whole invocation in an owned TMPDIR subdirectory so cleanup removes runtime-created entries without touching an older run. The deadline command is a session leader. Cancellation signals that group and walks `ps -ax -o pid=,ppid=`, then `KILL`s what ignored `TERM`. A failed listing is a cleanup failure. A helper that turns `errexit` back on before returning skips the caller's diagnostics. Ordinary cancellation is covered by `test/review-scripts.test.ts`. SIGKILL of the script itself is not. +- **A group signal does not test pid-targeted cancellation.** TERM sent only to the pid of `scripts/isolated-check.sh` or `scripts/isolate-run.sh`, as a tool runner's kill does, ran the EXIT trap and removed the archive or stage while the sandboxed command kept running, on `--live` with the key. Neither script signalled its child, and `forwards termination to the command session` signals the whole process group, which reaches the boundary directly. Run the boundary as a background session, forward TERM, INT, HUP and QUIT, kill it after `ISOLATE_TERM_GRACE`, and clean up only after it has stopped (`isolate_supervise`, `check_supervise`). A background job of a non-interactive bash starts with INT and QUIT ignored and cannot trap them, so the outer script forwards TERM and the helper restores both for the command. `SECONDS` has whole-second resolution; count 0.1 s sleeps. Bash runs the EXIT trap for HUP but not for QUIT, which left `scripts/live-test.sh`'s OpenCode running. macOS has no pid namespace, so a process that starts its own session is outside the kill. Signal only the script's pid in the test (`pid-targeted cancellation` in `test/review-scripts.test.ts`). A trap bash takes between two commands does not end the `wait` that follows: TERM trapped before `$!` was stored, or just before `wait` started, left `isolate-run.sh` waiting forever on a command that ignores TERM. The trap now runs the whole stop sequence (`isolate_sup_stop`). Force such a boundary in a test with a `BASH_ENV` hook whose DEBUG trap signals `$$` before the named command. - **A removed temp dir can come back.** Tests left about 200 TMPDIR entries per run ([#102](https://github.com/oakimov/cursor-opencode-provider/issues/102)): cutting the whole project-folder path put it outside a long cache root; @@ -805,6 +806,11 @@ These are failure lessons shared by both hosts, not a replacement for process listing or a writer surviving termination must fail cleanup, not silently permit removal (`test/process-cleanup.test.ts`). Test log content separately from its path-bearing diagnostic header (`test/debug.test.ts`). + A fixture writer that opens its file with create can bring it back between + a remover's unlink and rmdir: the review fixture's defective runner then + failed with ENOTEMPTY and exit 1 instead of the expected status, about once + in six full runs. Create the file once and append without creating + (`test/review-eval.test.ts`). - **Match CI environments.** Narrow PATH fixtures omitted mkdir. Queued publication tests once read Linux's `/proc/locks`, which macOS lacks; observe the test contender at the real `flock` call after its approval peek, diff --git a/test/review-eval.test.ts b/test/review-eval.test.ts index 4fe2f56..b27e438 100644 --- a/test/review-eval.test.ts +++ b/test/review-eval.test.ts @@ -154,12 +154,18 @@ if ignore: signal.signal(signal.SIGTERM, signal.SIG_IGN) (state / "descendant.pid").write_text(str(os.getpid())) (state / "workspace").write_text(os.getcwd()) +open("heartbeat", "a").close() (state / "descendant.ready").write_text("1") deadline = time.monotonic() + 20 while time.monotonic() < deadline and not (state / "stop").exists(): try: - with open("heartbeat", "a") as handle: - handle.write("x") + # Append without creating. A write that brought the file back between + # a runner's unlink and rmdir made that runner's removal fail. + handle = os.open("heartbeat", os.O_WRONLY | os.O_APPEND) + try: + os.write(handle, b"x") + finally: + os.close(handle) except OSError: pass time.sleep(0.05) diff --git a/test/review-scripts.test.ts b/test/review-scripts.test.ts index 566027c..992cd3a 100644 --- a/test/review-scripts.test.ts +++ b/test/review-scripts.test.ts @@ -1639,7 +1639,9 @@ describe("live test deadlines and cleanup", () => { } }) - it("removes plugin-list files when the live test is cancelled", async () => { + // The signal goes to the script's pid alone. QUIT once left OpenCode, which + // holds the key, running: bash does not run the EXIT trap for it. + for (const [signal, code] of [["SIGTERM", 143], ["SIGQUIT", 131]] as const) it(`removes plugin-list files and stops OpenCode when the live test receives ${signal}`, async () => { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "live-cancel-")) const bin = path.join(directory, "bin") const reviewHome = path.join(directory, "review-home") @@ -1675,16 +1677,17 @@ describe("live test deadlines and cleanup", () => { if (Date.now() - started > 8000) throw new Error("plugin list did not start") await pause() } - child.kill("SIGTERM") + child.kill(signal) const killed = Date.now() while (child.exitCode === null && child.signalCode === null) { - if (Date.now() - killed > 8000) throw new Error("live test did not exit after SIGTERM") + if (Date.now() - killed > 8000) throw new Error(`live test did not exit after ${signal}`) await pause() } - const temps = fs.readdirSync(path.join(reviewHome, "tmp")) - expect(temps).toEqual([]) const sleeperAlive = spawnSync("kill", ["-0", fs.readFileSync(pidFile, "utf8").trim()]) expect(sleeperAlive.status).not.toBe(0) + expect(child.exitCode).toBe(code) + const temps = fs.readdirSync(path.join(reviewHome, "tmp")) + expect(temps).toEqual([]) } finally { if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") if (fs.existsSync(pidFile)) spawnSync("kill", ["-KILL", fs.readFileSync(pidFile, "utf8").trim()]) @@ -1875,3 +1878,281 @@ wait } }) }) + +// Cancellation that reaches only the script's own pid, as a tool runner's +// kill(pid) or subprocess.terminate() does. A process-group signal also +// reaches the boundary directly and does not show whether the script passes +// the cancellation on before its cleanup runs. +describe("pid-targeted cancellation", () => { + const pause = () => new Promise((resolve) => setTimeout(resolve, 50)) + + // Visible to ps on the host for both the Linux namespace and macOS sandbox. + function marked(marker: string): number[] { + const listing = spawnSync("ps", ["-A", "-o", "pid=,args="], { encoding: "utf8" }) + if (listing.status !== 0) throw new Error(`ps failed: ${listing.stderr}`) + return listing.stdout.split("\n").filter((line) => line.includes(marker)).map((line) => Number.parseInt(line, 10)) + } + + function commandAlive(marker: string): boolean { + return marked(marker).length > 0 + } + + // A failed assertion must not leave the command it found running. + function killMarked(marker: string): void { + for (const pid of marked(marker)) { + try { process.kill(pid, "SIGKILL") } catch { /* already gone */ } + } + } + + type Exit = { code: number | null; signal: NodeJS.Signals | null; stdout: string; stderr: string; at: number } + + async function cancelAfterReady( + args: string[], + readyFile: string, + signal: NodeJS.Signals, + env: NodeJS.ProcessEnv, + ): Promise { + const child = spawn("bash", args, { cwd: root, env: { ...env, PYTHONDONTWRITEBYTECODE: "1" } }) + let stdout = "" + let stderr = "" + child.stdout.on("data", (chunk) => (stdout += chunk)) + child.stderr.on("data", (chunk) => (stderr += chunk)) + const exited = new Promise((resolve) => + child.on("exit", (code, exitSignal) => resolve({ code, signal: exitSignal, stdout, stderr, at: Date.now() })), + ) + const started = Date.now() + try { + while (!fs.existsSync(readyFile)) { + if (child.exitCode !== null || child.signalCode !== null) { + const early = await exited + return { ...early, unavailable: /ISOLATION=unavailable/.test(early.stderr), signalledAt: early.at } + } + if (Date.now() - started > 15000) throw new Error(`command did not start\n${stdout}\n${stderr}`) + await pause() + } + const signalledAt = Date.now() + child.kill(signal) + const timeout = new Promise((_, reject) => + setTimeout(() => reject(new Error(`no exit after ${signal}\n${stdout}\n${stderr}`)), 20000), + ) + const result = await Promise.race([exited, timeout]) + return { ...result, unavailable: false, signalledAt } + } finally { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") + } + } + + // The temp root is left as the host reports it; on macOS that is a symlink. + function fixture(prefix: string) { + const base = fs.mkdtempSync(path.join(os.tmpdir(), prefix)) + const src = path.join(base, "src") + const tmp = path.join(base, "tmp") + fs.mkdirSync(src) + fs.mkdirSync(tmp) + const marker = `${prefix}${process.pid}-${Math.random().toString(36).slice(2)}` + return { base, src, tmp, marker } + } + + it("isolate-run.sh kills a command that ignores TERM before removing its stage", async () => { + const { base, src, tmp, marker } = fixture("isolate-ignore-term-") + try { + const result = await cancelAfterReady( + [ + "scripts/isolate-run.sh", "--src", src, "--", + "/bin/sh", "-c", 'trap "" TERM; echo ready > ready; while :; do sleep 0.1; done', marker, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1" }, + ) + if (result.unavailable) return + // The command outlived TERM, so this exit must follow the grace-period kill. + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(result.at - result.signalledAt).toBeGreaterThanOrEqual(900) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + + it("isolate-run.sh forwards TERM so the command can stop itself", async () => { + const { base, src, tmp, marker } = fixture("isolate-forward-term-") + try { + const result = await cancelAfterReady( + [ + "scripts/isolate-run.sh", "--src", src, "--", + "/bin/sh", "-c", 'trap "echo stopped > stopped; exit 0" TERM; echo ready > ready; while :; do sleep 0.1; done', marker, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "30" }, + ) + if (result.unavailable) return + expect(commandAlive(marker)).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(fs.readFileSync(path.join(src, "stopped"), "utf8")).toBe("stopped\n") + // Far below the grace period: the command stopped on the forwarded TERM. + expect(result.at - result.signalledAt).toBeLessThan(10000) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + + // bash takes a trap between two commands, and a `wait` that starts after + // the trap does not end early for it. The hook, read through BASH_ENV, + // sends TERM to the script just before the named command, so the trap is + // taken at exactly that boundary rather than while the script waits. + for (const [boundary, when] of [ + ["isolate_sup_leader=$!", "before the session leader is known"], + ['wait "$isolate_sup_leader"', "just before the wait starts"], + ] as const) { + it(`isolate-run.sh stops a command when TERM is trapped ${when}`, async () => { + const { base, src, tmp, marker } = fixture("isolate-trap-boundary-") + const hook = path.join(base, "hook.sh") + const sent = path.join(base, "sent") + fs.writeFileSync( + hook, + [ + "unset BASH_ENV", + "set -o functrace", + `trap 'if [[ ! -e "$BOUNDARY_SENT" && "$BASH_COMMAND" == "$BOUNDARY_COMMAND" ]]; then echo sent > "$BOUNDARY_SENT"; kill -s TERM $$; fi' DEBUG`, + "", + ].join("\n"), + ) + const child = spawn( + "bash", + ["scripts/isolate-run.sh", "--src", src, "--", "/bin/sh", "-c", 'trap "" TERM; while :; do sleep 0.1; done', marker], + { + cwd: root, + env: { + ...process.env, + TMPDIR: tmp, + ISOLATE_TERM_GRACE: "1", + PYTHONDONTWRITEBYTECODE: "1", + BASH_ENV: hook, + BOUNDARY_COMMAND: boundary, + BOUNDARY_SENT: sent, + }, + }, + ) + let stderr = "" + child.stdout.resume() + child.stderr.on("data", (chunk) => (stderr += chunk)) + try { + const code = await new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new Error(`no exit; TERM sent: ${fs.existsSync(sent)}\n${stderr}`)), + 15000, + ) + child.on("exit", (exitCode) => { + clearTimeout(timer) + resolve(exitCode) + }) + }) + if (/ISOLATION=unavailable/.test(stderr)) return + expect(fs.existsSync(sent), stderr).toBe(true) + expect(commandAlive(marker), stderr).toBe(false) + expect(code, stderr).toBe(143) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + } + + for (const [signal, code] of [["SIGTERM", 143], ["SIGINT", 130]] as const) { + it(`isolated-check.sh waits for the boundary before removing the archive on ${signal}`, async () => { + const { base, src, tmp, marker } = fixture(`isolated-check-${signal.toLowerCase()}-`) + try { + const result = await cancelAfterReady( + [ + "-c", + ` + set -euo pipefail + source scripts/isolated-check.sh + worktree="$WT" + trap check_cleanup EXIT + cd "$WT" + check_step UNIT_TESTS /bin/sh -c 'trap "" TERM INT; echo ready > ready; while :; do sleep 0.1; done' "$MARKER" || true + echo after-step + `, + ], + path.join(src, "ready"), + signal, + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1", WT: src, MARKER: marker }, + ) + if (result.unavailable) return + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(code) + expect(result.stderr).toContain(`Check cancelled by ${signal}.`) + // The cancelled check neither reports the step nor runs the next one. + expect(result.stdout).not.toContain("UNIT_TESTS=") + expect(result.stdout).not.toContain("after-step") + expect(fs.existsSync(src)).toBe(false) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + } + + // check_run_live is the call that passes CURSOR_API_KEY into the boundary. + // The harness here is a stand-in checkout: the real isolate-run.sh, and a + // live-test.sh that reports whether it was given a key and ignores TERM. + it("isolated-check.sh waits for the live boundary, which holds the key, before removing the archive", async () => { + const { base, src, tmp, marker } = fixture("isolated-check-live-") + const harness = path.join(base, "harness") + fs.mkdirSync(path.join(harness, "scripts"), { recursive: true }) + fs.copyFileSync(path.join(root, "scripts/isolate-run.sh"), path.join(harness, "scripts/isolate-run.sh")) + fs.chmodSync(path.join(harness, "scripts/isolate-run.sh"), 0o755) + fs.writeFileSync( + path.join(harness, "scripts/live-test.sh"), + [ + 'echo "LIVE_STUB key=${CURSOR_API_KEY:+present}"', + `exec /bin/sh -c 'trap "" TERM INT; echo ready > ready; while :; do sleep 0.1; done' ${marker}`, + "", + ].join("\n"), + ) + try { + const result = await cancelAfterReady( + [ + "-c", + ` + set -euo pipefail + source scripts/isolated-check.sh + ROOT="$HARNESS" + worktree="$WT" + trap check_cleanup EXIT + cd "$WT" + set +e + check_run_live "$WT" + echo "live_exit=$?" + `, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1", WT: src, HARNESS: harness, CURSOR_API_KEY: secret }, + ) + if (result.unavailable) return + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(result.stderr).toContain("Check cancelled by SIGTERM.") + expect(result.stdout).toContain("LIVE_STUB key=present") + expect(result.stdout).not.toContain(secret) + // The cancelled live run does not return to the caller's summary. + expect(result.stdout).not.toContain("live_exit=") + expect(fs.existsSync(src)).toBe(false) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) +})