diff --git a/skills/sshx/CODEX_WORKER_SPEC.md b/skills/sshx/CODEX_WORKER_SPEC.md index dd03e612..1853dc93 100644 --- a/skills/sshx/CODEX_WORKER_SPEC.md +++ b/skills/sshx/CODEX_WORKER_SPEC.md @@ -178,9 +178,10 @@ when that wait's own return status equals the recorded signal status. It joins every recorded child before publication. It then publishes the report with `interrupted: true` and exits nonzero when a signal was recorded. This preserves ownership of recorded children but does not promise prompt cancellation or -carrier teardown. The runner may itself defer traps during its synchronous -carrier call and does not propagate signals, so teardown of the whole job tree -remains the host's responsibility. +carrier teardown from a dispatcher-only signal. The host signals each runner +for cancellation; on a trappable `INT` or `TERM`, each runner tears down its own +carrier tree before publishing `INTERRUPTED`. Time limits and signalling the +runners remain the host's responsibility. The dispatcher's `INT` half has the same inherited-disposition limit as its children: if the host starts it with `SIGINT` ignored, Bash cannot make its @@ -363,23 +364,35 @@ terminal status intentionally leaves its flight ineligible. ## Carrier After `jq`, directory, brief, log, and executable `codex` preflight checks, the -runner performs one synchronous foreground call: +runner launches one carrier as its own background job, records `carrier_pid=$!`, +and joins it with the Bash `wait` builtin: ```text codex exec --json -C --sandbox \ --skip-git-repo-check -o - ``` -The command's stdout and stderr go to the fixed diagnostic logs. There is no -timeout, supervisor, helper, PID state, signal propagation, process-group -handling, or KILL escalation. The carrier wait status is written atomically to +The command's stdout and stderr go to the fixed diagnostic logs, and stdin +comes from the same brief. The runner remains attached to its host job while +waiting. On normal return, the carrier wait status is written atomically to `carrier.exit`. A missing executable is `LAUNCH_FAILED`; once invoked, any nonzero carrier status is `CARRIER_EXIT_NONZERO`. -`INT` and `TERM` traps set `reason_code=INTERRUPTED` and exit through the -normal `EXIT` trap. Bash may defer these traps while the synchronous foreground -command is running. The runner does not promise signal reachability in every -phase and does not tear down descendants. +`pgrep` is required before carrier launch. On a trappable `INT` or `TERM`, the +runner ignores further `INT` and `TERM`, records `reason_code=INTERRUPTED`, and +recursively collects the carrier's descendants with `pgrep -P`. It retains that +snapshot before sending individual `TERM` signals to descendants, deepest +first, and then to the carrier. It polls the recorded PIDs for up to fifty +0.1-second grace intervals, sends individual `KILL` signals to those still +present, and reaps its direct carrier before the existing `finish` path +publishes terminal status. It does not signal process groups or processes +outside the recorded carrier tree. A descendant-discovery or signal-delivery +failure emits a diagnostic; `pgrep` exit 1 means no children. + +Teardown does not write `carrier.exit` or replace `carrier_exit` with its own +wait result. The normal artifact validation and completion predicate retain +their existing order. A signal during terminal publication remains ignored so +publication cannot be interrupted. There is no runner-owned time limit. ## Status Projection @@ -392,6 +405,15 @@ references, `started_at`, `finished_at`, `duration_seconds`, `work_target`, both lookups succeed. A failed time lookup writes `null` and does not fail the flight. +An interrupted run additionally contains `teardown`, an object with integer +`descendants_signalled` and `killed_after_grace` counts. The first counts +successful descendant `TERM` sends, excluding the carrier; the second counts +successful `KILL` sends after grace, including the carrier. These are signal +send counts, not proof that every process is reaped: Bash reaps only its direct +carrier, and an orphan zombie can remain present until the system reaper runs. +A normal run omits `teardown`. The timestamped teardown log line carries the +same counts. + Exit code is the sole authority: `0` means complete, `1` means not complete, and `64` means usage error. `status.json` is a terminal, machine-readable projection for callers that need structured data. stdout is a human-readable @@ -401,9 +423,9 @@ parseable, and is not byte-for-byte identical to any file. `status.json` and the batch report are mechanical projections only. Neither is a completion or verdict source under `SKILL.md`. -Before the synchronous carrier call, stdout reports all then-known invocation -identity and derived artifact paths followed by `carrier starting`. After the -call returns, it reports the carrier exit status; terminal cleanup then reports +Before carrier launch, stdout reports all then-known invocation identity and +derived artifact paths followed by `carrier starting`. After the normal wait +returns, it reports the carrier exit status; terminal cleanup then reports `status`, `reason_code`, `verdict`, and duration. Every stdout line begins with a UTC ISO-8601 timestamp. stdout write failure is diagnostic only and cannot change the authoritative exit decision. @@ -433,7 +455,8 @@ contains invocation identity, `status` (`COMPLETE` or `NOT_COMPLETE`), `reason_code`, `carrier_exit` (or `null`), derived artifact references, diagnostic log references, the six fields `started_at`, `finished_at`, `duration_seconds`, `work_target`, `sandbox`, and `brief_ref`, -and a verdict only when the stage has a verdict mapping. Before either +a verdict only when the stage has a verdict mapping, and the interruption-only +`teardown` counts described above. Before either runner-owned projection is written, its temporary and final target must be absent or a non-symbolic-link regular file. After each rename, the fixed `carrier.exit` or `status.json` path must be a non-symbolic-link @@ -458,17 +481,20 @@ the POSIX text-line contract. ## Teardown Prerequisite -Verified in two independent Claude Code harness experiments: `TaskStop` -terminates the entire process tree, including a child that actively ignores -`TERM` and `INT`; this indicates the harness uses SIGKILL or process-group -teardown. The runner therefore does not propagate signals. - -Codex, Cursor, and Gemini host teardown behavior is unverified. An interactive -Ctrl-C normally sends `INT` to the foreground process group, which includes the -synchronous carrier. A default `TERM` sent only to the runner PID may be -deferred by Bash until the foreground carrier returns. An uncatchable `SIGKILL` -sent only to the runner PID can leave the carrier orphaned and running. The -runner does not attempt to compensate for any of these host behaviors. +The host owns time limits and delivery of cancellation signals to the runner +itself, including each runner in a batch. The runner owns teardown of its own +carrier tree on trappable `INT` and `TERM`; the background carrier and builtin +`wait` let the handler run while the carrier is alive. A `TERM` sent only to +the runner PID therefore initiates carrier-tree teardown without waiting for +normal carrier exit. + +Bash cannot trap a signal inherited as ignored, including `INT` on a normal +non-job-control background launch. The host uses a trappable signal for runner +cancellation. An uncatchable `SIGKILL` sent only to the runner PID can leave the +carrier orphaned and running; host teardown remains necessary for uncatchable +termination. The runner contract does not assume that any particular host +already kills descendants. Codex, Cursor, and Gemini host teardown behavior +remains unverified. ## Threat Model @@ -478,18 +504,21 @@ missing artifacts, and accidental projection-path type collisions, but it is not a sandbox against a hostile carrier. This design does not defend TOCTOU races, including caller-brief replacement after the dispatcher's readability probe or flight changes after cleanup's repeated eligibility check; those checks -narrow their respective windows but do not make object identity immutable. It -also does not defend an active `setsid` escape, forged runner artifact paths, or -deliberate replacement of files inside the owned attempt directory. An +narrow their respective windows but do not make object identity immutable. +Process-tree discovery is a PID snapshot, not an atomic process-ownership +boundary: concurrent forks, reparenting, and PID reuse are outside its +guarantee. It also does not defend an active `setsid` escape, forged runner +artifact paths, or deliberate replacement of files inside the owned attempt directory. An untrusted carrier or a requirement to cover those attacks requires a new design review rather than more checks in this runner. ## Boundaries The runner has no git, GitHub, label, release, host lifecycle, cleanup, or -global-state authority. Time limits and whole-job teardown belong to the -caller harness. Power-loss durability is not guaranteed. Deletion authority -lives only in `clean-codex-worker-runs.sh` and is bounded to terminal-only, +global-state authority. Time limits, signalling the runner itself, and teardown +after uncatchable termination belong to the caller harness. The runner's +catchable-signal teardown authority is limited to its own carrier tree. +Power-loss durability is not guaranteed. Deletion authority lives only in `clean-codex-worker-runs.sh` and is bounded to terminal-only, whole-flight artifact retirement with dry-run default and no force override. No other skill may depend on these mechanisms. To reverse the exception diff --git a/skills/sshx/SKILL.md b/skills/sshx/SKILL.md index 2203fd2f..9c5347d0 100644 --- a/skills/sshx/SKILL.md +++ b/skills/sshx/SKILL.md @@ -131,13 +131,13 @@ Every worker dispatch must create a prompt-level `SshxWorkerFlightRecord` before While any `SshxWorkerFlightRecord` for the same `work_target` is `in-flight` or `retrying`, the caller is read-only for that target. The caller is non-mutating for that target and its external resources. The caller must not take over the same `work_target` because a process snapshot, log text, or workspace state appears quiet. -For each `codex-cli` attempt, before launch the caller must choose a unique `flight_id` and `attempt` and pass them to `skills/sshx/scripts/run-codex-worker.sh`; the runner derives and owns every artifact path, parallel attempts receive disjoint derived paths, and the caller must not supply arbitrary result, sentinel, log, or state paths. Every formal `codex-cli` flight must use this runner rather than a parallel direct-launch path. The command, sandbox, path, direct-process, and collection mechanics are owned by `CODEX_WORKER_SPEC.md`; the required dispatch shape is the runner's default `danger-full-access` sandbox, so the caller passes no sandbox selection unless the maintainer explicitly directs a narrower one. Time limits and final teardown of the whole job tree are the caller AI harness's responsibility. The caller must not poll worker artifact paths while the runner is active. The caller records `result_envelope_ref` and `completion_sentinel_ref` on the matching flight only if the runner reports completion and the envelope and sentinel validate. Completion and verdict recognition stay governed by the `## Worker Completion Contract`. +For each `codex-cli` attempt, before launch the caller must choose a unique `flight_id` and `attempt` and pass them to `skills/sshx/scripts/run-codex-worker.sh`; the runner derives and owns every artifact path, parallel attempts receive disjoint derived paths, and the caller must not supply arbitrary result, sentinel, log, or state paths. Every formal `codex-cli` flight must use this runner rather than a parallel direct-launch path. The command, sandbox, path, direct-process, and collection mechanics are owned by `CODEX_WORKER_SPEC.md`; the required dispatch shape is the runner's default `danger-full-access` sandbox, so the caller passes no sandbox selection unless the maintainer explicitly directs a narrower one. Time limits, signalling the runner itself, and teardown after uncatchable termination are the caller AI harness's responsibility; the runner owns teardown of its own carrier tree on trappable `INT` and `TERM` as specified in `CODEX_WORKER_SPEC.md`. The caller must not poll worker artifact paths while the runner is active. The caller records `result_envelope_ref` and `completion_sentinel_ref` on the matching flight only if the runner reports completion and the envelope and sentinel validate. Completion and verdict recognition stay governed by the `## Worker Completion Contract`. The caller must launch the runner through a host-provided background job mechanism that notifies the caller when the carrier process exits. It must not use shell `&` to background the runner, because that detaches the process from host tracking and can leave an init-adopted carrier running without ever notifying the caller of completion. It must not monitor files or logs to poll for completion; doing so conflicts with the no-polling rule above. `skills/sshx/scripts/run-codex-worker-batch.sh` is the permitted one-call fan-out alternative for the `codex-cli` subset of a multi-seat stage. It never covers a whole stage because the `nyxid-oracle` and `isolated-token-subagent` seats reserved by the dispatch-time composition above remain outside the batch. The dispatcher obtains every worker artifact path from the runner's pure path projection; worker artifact paths remain runner-derived and are never caller-supplied. -Internal shell `&` followed by `wait` is permitted inside that one named batch script because it remains the foreground process of one host-tracked job, records every child, and joins every recorded child before publishing a report; its signal handling, interruption reporting, and inherited-disposition limits are owned by `CODEX_WORKER_SPEC.md` and the script's behavior tests, and whole-job-tree teardown remains the host's responsibility. Caller-authored `&`, `nohup`, `disown`, and `setsid` remain forbidden. Batching degrades host completion notification from per-carrier to per-batch. Launching one host job per seat remains permitted and is the form on which per-seat retry and fallback latency depends; batching is an alternative, not a mandate. +Internal shell `&` followed by `wait` is permitted inside `skills/sshx/scripts/run-codex-worker.sh` for its own carrier and inside the named batch script for its runners; each script remains attached to one host-tracked job, and the batch script records every child, and joins every recorded child before publishing a report; its signal handling, interruption reporting, and inherited-disposition limits are owned by `CODEX_WORKER_SPEC.md` and the script's behavior tests, and signalling each runner for batch cancellation remains the host's responsibility. Caller-authored `&`, `nohup`, `disown`, and `setsid` remain forbidden. Batching degrades host completion notification from per-carrier to per-batch. Launching one host job per seat remains permitted and is the form on which per-seat retry and fallback latency depends; batching is an alternative, not a mandate. The caller may invoke `skills/sshx/scripts/read-codex-worker-status.sh` only after host completion notification. Status reading is a one-shot, after-terminal collection convenience and is not authorization to poll while any runner is active. The batch report is dispatcher-owned orchestration evidence, not a worker artifact, and neither it nor the status projection changes completion or verdict routing. diff --git a/skills/sshx/formal/Sshx/Clauses/Delegation.lean b/skills/sshx/formal/Sshx/Clauses/Delegation.lean index fed42d61..298aeb2e 100644 --- a/skills/sshx/formal/Sshx/Clauses/Delegation.lean +++ b/skills/sshx/formal/Sshx/Clauses/Delegation.lean @@ -311,8 +311,9 @@ inductive TeardownOwner | runner deriving DecidableEq, Repr --- SKILL[def]: "Time limits and final teardown of the whole job tree are the caller AI harness's responsibility." -def teardownOwner : TeardownOwner := .callerHarness +-- SKILL[def]: "Time limits, signalling the runner itself, and teardown after uncatchable termination are the caller AI harness's responsibility; the runner owns teardown of its own carrier tree on trappable `INT` and `TERM` as specified in `CODEX_WORKER_SPEC.md`." +def teardownOwner (trappableCarrierCancellation : Bool) : TeardownOwner := + if trappableCarrierCancellation then .runner else .callerHarness -- SKILL[ref]: "The caller records `result_envelope_ref` and `completion_sentinel_ref` on the matching flight only if the runner reports completion and the envelope and sentinel validate." abbrev refsOnlyOnCompletion := @Behavior.collectEffect @@ -334,8 +335,8 @@ theorem batch_never_covers_whole_stage (seats : Nat) (h : 2 ≤ seats) : -- SKILL[ref]: "The dispatcher obtains every worker artifact path from the runner's pure path projection; worker artifact paths remain runner-derived and are never caller-supplied." abbrev batchPathsFromRunner := artifactPathOwner --- SKILL[def]: "Internal shell `&` followed by `wait` is permitted inside that one named batch script because it remains the foreground process of one host-tracked job, records every child, and joins every recorded child before publishing a report; its signal handling, interruption reporting, and inherited-disposition limits are owned by `CODEX_WORKER_SPEC.md` and the script's behavior tests, and whole-job-tree teardown remains the host's responsibility." -def batchInternalWaitPermitted : Bool := true +-- SKILL[def]: "Internal shell `&` followed by `wait` is permitted inside `skills/sshx/scripts/run-codex-worker.sh` for its own carrier and inside the named batch script for its runners; each script remains attached to one host-tracked job, and the batch script records every child, and joins every recorded child before publishing a report; its signal handling, interruption reporting, and inherited-disposition limits are owned by `CODEX_WORKER_SPEC.md` and the script's behavior tests, and signalling each runner for batch cancellation remains the host's responsibility." +def runnerAndBatchInternalWaitPermitted : Bool := true inductive NotificationGranularity | perCarrier diff --git a/skills/sshx/scripts/run-codex-worker.sh b/skills/sshx/scripts/run-codex-worker.sh index eac99d77..c3a81a1d 100644 --- a/skills/sshx/scripts/run-codex-worker.sh +++ b/skills/sshx/scripts/run-codex-worker.sh @@ -4,6 +4,7 @@ umask 077 reason=INTERNAL_ERROR code=1 carrier_exit=null +carrier_pid=; carrier_active=0; teardown=null started_at=; started_epoch=; finished_at=; finished_epoch=; duration_seconds= run_dir= run_dir_owned=0 @@ -31,16 +32,16 @@ finish() { printf '%s\n' 'run-codex-worker: INTERNAL_ERROR: invalid status target' >&2 status_target_valid=0 elif [ "$result_valid" -eq 1 ]; then - "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg status "$terminal_status" --arg reason_code "$reason" --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" --slurpfile result "$result_ref" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:$status,reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},verdict:$result[0].conclusion.verdict,started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref}' > "$status_tmp" + "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg status "$terminal_status" --arg reason_code "$reason" --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" --argjson teardown "$teardown" --slurpfile result "$result_ref" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:$status,reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},verdict:$result[0].conclusion.verdict,started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref} + (if $teardown == null then {} else {teardown:$teardown} end)' > "$status_tmp" else - "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg status "$terminal_status" --arg reason_code "$reason" --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:$status,reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref}' > "$status_tmp" + "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg status "$terminal_status" --arg reason_code "$reason" --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" --argjson teardown "$teardown" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:$status,reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref} + (if $teardown == null then {} else {teardown:$teardown} end)' > "$status_tmp" fi render_rc=$?; render_ok=0; mv_rc=0 [ "$render_rc" -eq 0 ] && [ -s "$status_tmp" ] && render_ok=1 if [ "$status_target_valid" -eq 1 ] && [ "$render_ok" -eq 0 ]; then reason=INTERNAL_ERROR; code=1 printf '%s\n' 'run-codex-worker: INTERNAL_ERROR: cannot render status' >&2 - "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg reason_code INTERNAL_ERROR --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:"NOT_COMPLETE",reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref}' > "$status_tmp" + "$jq_path" -n --argjson schema_version 1 --arg flight_id "$flight_id" --argjson attempt "$attempt" --arg stage "$stage" --arg reason_code INTERNAL_ERROR --arg carrier_exit "$carrier_exit" --arg run_dir "$run_dir" --arg result_ref "$result_ref" --arg sentinel_ref "$sentinel_ref" --arg stdout_ref "$stdout_ref" --arg stderr_ref "$stderr_ref" --arg last_message_ref "$last_message_ref" --arg started_at "$started_at" --arg finished_at "$finished_at" --arg duration_seconds "$duration_seconds" --arg work_target "$work_target" --arg sandbox "$sandbox" --arg brief_ref "$brief_ref" --argjson teardown "$teardown" '{schema_version:$schema_version,flight_id:$flight_id,attempt:$attempt,stage:$stage,status:"NOT_COMPLETE",reason_code:$reason_code,carrier_exit:(if $carrier_exit=="null" then null else ($carrier_exit|tonumber) end),run_dir:$run_dir,result_ref:$result_ref,completion_sentinel_ref:$sentinel_ref,log_refs:{stdout:$stdout_ref,stderr:$stderr_ref,last_message:$last_message_ref},started_at:(if $started_at=="" then null else $started_at end),finished_at:(if $finished_at=="" then null else $finished_at end),duration_seconds:(if $duration_seconds=="" then null else ($duration_seconds|tonumber) end),work_target:$work_target,sandbox:$sandbox,brief_ref:$brief_ref} + (if $teardown == null then {} else {teardown:$teardown} end)' > "$status_tmp" fallback_rc=$? if [ "$fallback_rc" -eq 0 ] && [ -s "$status_tmp" ]; then render_ok=1; else printf '%s\n' 'run-codex-worker: INTERNAL_ERROR: cannot render failure status' >&2 @@ -65,7 +66,69 @@ finish() { } trap finish EXIT -interrupt() { reason=INTERRUPTED; code=1; finish; } +collect_carrier_descendants() { + local parent=$1 children child discovery_rc + children=$(pgrep -P "$parent"); discovery_rc=$? + case "$discovery_rc" in + 0) ;; + 1) return ;; + *) printf '%s\n' "run-codex-worker: teardown: cannot enumerate children of $parent (pgrep rc=$discovery_rc)" >&2; return ;; + esac + for child in $children; do + collect_carrier_descendants "$child" + teardown_pids[${#teardown_pids[@]}]=$child + done +} + +teardown_carrier() { + local pid descendants_signalled=0 killed_after_grace=0 poll survivors + local teardown_pids=() + if [ "$carrier_active" -eq 1 ] && [ -n "$carrier_pid" ]; then + # Snapshot before signalling: parents may exit and reparent their children. + collect_carrier_descendants "$carrier_pid" + teardown_pids[${#teardown_pids[@]}]=$carrier_pid + for pid in "${teardown_pids[@]}"; do + if kill -TERM "$pid" 2>/dev/null; then + [ "$pid" = "$carrier_pid" ] || descendants_signalled=$((descendants_signalled + 1)) + elif kill -0 "$pid" 2>/dev/null; then + printf '%s\n' "run-codex-worker: teardown: cannot send TERM to $pid" >&2 + fi + done + for ((poll = 0; poll < 50; poll++)); do + survivors=0 + for pid in "${teardown_pids[@]}"; do + kill -0 "$pid" 2>/dev/null && survivors=1 + done + [ "$survivors" -eq 1 ] || break + sleep 0.1 + done + for pid in "${teardown_pids[@]}"; do + if kill -0 "$pid" 2>/dev/null; then + if kill -KILL "$pid" 2>/dev/null; then + killed_after_grace=$((killed_after_grace + 1)) + elif kill -0 "$pid" 2>/dev/null; then + printf '%s\n' "run-codex-worker: teardown: cannot send KILL to $pid" >&2 + fi + fi + done + # Reap our direct child without changing carrier.exit or carrier_exit. + wait "$carrier_pid" 2>/dev/null + carrier_pid=; carrier_active=0 + fi + teardown="{\"descendants_signalled\":$descendants_signalled,\"killed_after_grace\":$killed_after_grace}" + log_event "teardown descendants_signalled=$descendants_signalled killed_after_grace=$killed_after_grace" +} + +interrupt() { + trap '' INT TERM + # A signal can arrive after launch but before the PID assignment. + if [ "$carrier_active" -eq 1 ] && [ -z "$carrier_pid" ]; then + carrier_pid=$(jobs -p) + fi + reason=INTERRUPTED; code=1 + teardown_carrier + finish +} trap interrupt INT TERM usage_error() { reason=USAGE_ERROR; code=64; printf '%s\n' "run-codex-worker: USAGE_ERROR: $1" >&2; return 1; } require_value() { [ "$2" -ge 2 ] || usage_error "missing value for $1"; } @@ -243,10 +306,14 @@ EOF log_field last_message "$last_message_ref"; log_field result "$result_ref"; log_field sentinel "$sentinel_ref" log_field carrier_exit "$carrier_exit_ref"; log_field status_file "$status_ref" if ! codex_path=$(command -v codex 2>/dev/null) || [ ! -x "$codex_path" ]; then reason=LAUNCH_FAILED; return 1; fi + command -v pgrep >/dev/null 2>&1 || { printf '%s\n' 'run-codex-worker: INTERNAL_ERROR: pgrep is required for carrier teardown' >&2; return 1; } log_event 'carrier starting' + carrier_active=1 "$codex_path" exec --json -C "$work_target" --sandbox "$sandbox" --skip-git-repo-check -o "$last_message_ref" - \ - < "$brief_ref" > "$stdout_ref" 2> "$stderr_ref" - carrier_exit=$? + < "$brief_ref" > "$stdout_ref" 2> "$stderr_ref" & + carrier_pid=$! + wait "$carrier_pid" + carrier_exit=$? carrier_pid= carrier_active=0 log_event "carrier exited rc=$carrier_exit" regular_or_absent "$carrier_exit_ref" && regular_or_absent "$carrier_exit_ref.tmp" || return 1 printf '%s\n' "$carrier_exit" > "$carrier_exit_ref.tmp" && mv -f "$carrier_exit_ref.tmp" "$carrier_exit_ref" && [ -f "$carrier_exit_ref" ] && [ ! -L "$carrier_exit_ref" ] || return 1 diff --git a/skills/sshx/tests/test_codex_worker_tools.py b/skills/sshx/tests/test_codex_worker_tools.py index f3c7ba8f..7ab1e502 100644 --- a/skills/sshx/tests/test_codex_worker_tools.py +++ b/skills/sshx/tests/test_codex_worker_tools.py @@ -639,13 +639,13 @@ def test_batch_signal_during_launch_gives_every_runner_the_same_term_disposition self.assertEqual(len(runner_pids), 2) for runner_pid in runner_pids: os.kill(int(runner_pid), signal.SIGTERM) - for marker in flights: - self.release_carrier(marker) _, stderr = process.communicate(timeout=WATCHDOG_SECONDS) self.assertEqual(process.returncode, 1, stderr) document = json.loads(report.read_text()) self.assertTrue(document["interrupted"]) self.assertEqual([item["runner_exit_code"] for item in document["workers"]], [1, 1]) + for item in document["workers"]: + self.assertEqual(json.loads(Path(item["status_ref"]).read_text())["reason_code"], "INTERRUPTED") def test_batch_retains_colliding_child_status_and_ignores_second_recovery_signal(self) -> None: bash_wrapper = self.bin_dir / "bash" diff --git a/skills/sshx/tests/test_run_codex_worker.py b/skills/sshx/tests/test_run_codex_worker.py index 97bf6c4d..219ce2d9 100644 --- a/skills/sshx/tests/test_run_codex_worker.py +++ b/skills/sshx/tests/test_run_codex_worker.py @@ -6,6 +6,7 @@ import signal import subprocess import tempfile +import time import unittest from pathlib import Path @@ -62,6 +63,21 @@ carrier_exit_write_failure) write_result; write_sentinel; mkdir "$run_dir/carrier.exit.tmp" ;; artifacts_then_wait) write_result; write_sentinel; printf '%s\n' ready > "$FAKE_READY"; command cat "$FAKE_RELEASE" >/dev/null; printf '%s\n' exited > "$FAKE_EXITED" ;; interrupt_wait) printf '%s\n' "$$" > "$FAKE_CARRIER_PID"; printf '%s\n' ready > "$FAKE_READY"; command cat "$FAKE_RELEASE" >/dev/null ;; + process_tree|ignore_term_tree|tree_success) + [ "$FAKE_MODE" != ignore_term_tree ] || trap '' TERM + sleep 300 & + leaf_pid=$! + sh -c ' + sleep 300 & + printf "%s\n" "$$" "$!" > "$FAKE_NESTED_PIDS.tmp" + mv "$FAKE_NESTED_PIDS.tmp" "$FAKE_NESTED_PIDS" + wait + ' & + while [ ! -f "$FAKE_NESTED_PIDS" ]; do sleep 0.01; done + { printf '%s\n' "$$" "$leaf_pid"; command cat "$FAKE_NESTED_PIDS"; } > "$FAKE_TREE_PIDS.tmp" + mv "$FAKE_TREE_PIDS.tmp" "$FAKE_TREE_PIDS" + if [ "$FAKE_MODE" = tree_success ]; then write_result; write_sentinel; else wait; fi + ;; invalid_verdict_missing_sentinel) verdict=unexpected; write_result ;; exit_127) exit 127 ;; projection_collision) @@ -242,12 +258,17 @@ def test_status_contract_source_regression(self) -> None: self.assertIn(f"`{field}`", spec) def test_trap_contract_source_regression(self) -> None: - trap_lines = [line.strip() for line in RUNNER.read_text().splitlines() if line.strip().startswith("trap")] + source = RUNNER.read_text() + trap_lines = [line.strip() for line in source.splitlines() if line.strip().startswith("trap")] self.assertIn("trap finish EXIT", trap_lines) self.assertIn("trap interrupt INT TERM", trap_lines) self.assertIn("trap - EXIT", trap_lines) self.assertIn("trap '' INT TERM", trap_lines) self.assertNotIn("trap - EXIT INT TERM", trap_lines) + interrupt = source.split("interrupt() {", 1)[1].split("\n}", 1)[0] + self.assertLess(interrupt.index("trap '' INT TERM"), interrupt.index("teardown_carrier")) + self.assertLess(interrupt.index("teardown_carrier"), interrupt.index("finish")) + self.assertIn('carrier_pid=$!\n wait "$carrier_pid"', source) def test_terminal_status_publish_failure_installs_no_projection(self) -> None: result = self.run_worker("projection_collision", extra_env={"FAKE_COLLISION_TARGET": "status.json.tmp", "FAKE_COLLISION_SHAPE": "directory", "FAKE_OUTSIDE": str(self.temp_dir / "unused")}) @@ -334,8 +355,6 @@ def run_interrupted_runner(self, signal_number: int) -> RunResult: self.assertEqual(ready_signal.read().strip(), "ready") carrier_pid = int(carrier_pid_ref.read_text()) os.kill(process.pid, signal_number) - with release.open("w") as release_signal: - release_signal.write("release\n") stdout, stderr = process.communicate(timeout=10) result = RunResult(subprocess.CompletedProcess(process.args, process.returncode, stdout, stderr), self.expected_run_dir(flight)) self.assert_terminal(result, "INTERRUPTED") @@ -349,6 +368,119 @@ def test_sigterm_to_runner_writes_interrupted_terminal_status(self) -> None: def test_sigint_to_runner_writes_interrupted_terminal_status(self) -> None: self.run_interrupted_runner(signal.SIGINT) + def process_is_alive(self, pid: int) -> bool: + snapshot = subprocess.run(["ps", "-o", "stat=", "-p", str(pid)], capture_output=True, text=True, timeout=2) + self.assertIn(snapshot.returncode, (0, 1), snapshot.stderr) + # An orphan awaiting the system reaper is no longer executing work. + state = snapshot.stdout.strip() + return bool(state) and not state.startswith("Z") + + def await_tree_pids(self, pid_ref: Path, process: subprocess.Popen[str]) -> list[int]: + deadline = time.monotonic() + 10 + while not pid_ref.is_file(): + self.assertIsNone(process.poll(), "carrier exited before recording its tree") + self.assertLess(time.monotonic(), deadline, "carrier tree readiness timed out") + time.sleep(0.02) + pids = [int(pid) for pid in pid_ref.read_text().splitlines()] + self.assertEqual(len(pids), 4, "expected carrier, leaf, nested shell, and nested leaf") + self.assertEqual(len(set(pids)), 4) + return pids + + def kill_recorded_pids(self, pids: list[int]) -> None: + for pid in reversed(pids): + try: + os.kill(pid, signal.SIGKILL) + except ProcessLookupError: + pass + + def run_carrier_tree(self, mode: str, *, interrupt: bool, repeat_signal: bool = False) -> RunResult: + flight = self.next_flight(mode) + pid_ref = self.temp_dir / f"{flight}.pids" + pids: list[int] = [] + # Shares the caller's process group; teardown must never reach it. + unrelated = subprocess.Popen(["sleep", "300"]) + process = subprocess.Popen( + self.command(flight), stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True, env=self.environment(mode, FAKE_TREE_PIDS=str(pid_ref), FAKE_NESTED_PIDS=str(self.temp_dir / f"{flight}-nested.pids")), + ) + try: + pids = self.await_tree_pids(pid_ref, process) + started = time.monotonic() + if interrupt: + self.assertTrue(all(self.process_is_alive(pid) for pid in pids)) + process.send_signal(signal.SIGTERM) + if repeat_signal: + time.sleep(0.2) + self.assertIsNone(process.poll(), "TERM-ignoring tree must receive a grace period") + self.assertFalse((self.expected_run_dir(flight) / "status.json").exists()) + process.send_signal(signal.SIGTERM) + process.send_signal(signal.SIGINT) + stdout, stderr = process.communicate(timeout=10 - (time.monotonic() - started)) + result = RunResult(subprocess.CompletedProcess(process.args, process.returncode, stdout, stderr), self.expected_run_dir(flight)) + self.assert_terminal(result, "INTERRUPTED" if interrupt else "COMPLETE") + self.assertIsNone(unrelated.poll(), "teardown signalled a process outside the carrier tree") + assert result.status is not None + if interrupt: + while any(self.process_is_alive(pid) for pid in pids) and time.monotonic() - started < 10: + time.sleep(0.02) + self.assertFalse(any(self.process_is_alive(pid) for pid in pids), f"carrier tree survived: {pids}") + self.assertIsNone(result.status["carrier_exit"]) + self.assertFalse((result.run_dir / "carrier.exit").exists()) + self.assertFalse((result.run_dir / "result.json").exists()) + self.assertFalse((result.run_dir / "completion.sentinel").exists()) + self.assertEqual(set(result.status["teardown"]), {"descendants_signalled", "killed_after_grace"}) + self.assertGreater(result.status["teardown"]["descendants_signalled"], 0) + self.assertLessEqual(result.status["teardown"]["descendants_signalled"], 3) + if mode == "ignore_term_tree": + self.assertGreaterEqual(time.monotonic() - started, 4.5) + self.assertEqual(result.status["teardown"]["killed_after_grace"], 4) + self.assertLess(time.monotonic() - started, 10) + else: + self.assertNotIn("teardown", result.status) + self.assertNotIn("teardown descendants_signalled=", stdout) + self.assertTrue(all(self.process_is_alive(pid) for pid in pids[1:]), "normal success tears down descendants") + self.assertEqual((result.run_dir / "carrier.exit").read_text(), "0\n") + return result + finally: + if not pids and pid_ref.is_file(): + pids = [int(pid) for pid in pid_ref.read_text().splitlines()] + self.kill_recorded_pids(pids) + if process.poll() is None: + process.kill() + process.communicate(timeout=10) + unrelated.terminate() + unrelated.wait(timeout=10) + + def test_sigterm_tears_down_carrier_process_tree(self) -> None: + self.run_carrier_tree("process_tree", interrupt=True) + + def test_sigterm_kills_term_ignoring_carrier_tree_after_grace(self) -> None: + self.run_carrier_tree("ignore_term_tree", interrupt=True, repeat_signal=True) + + def test_success_does_not_teardown_carrier_descendants(self) -> None: + self.run_carrier_tree("tree_success", interrupt=False) + + def test_signal_at_carrier_launch_boundaries(self) -> None: + bash_env = self.temp_dir / "launch-signal.bash" + bash_env.write_text( + "set -T\n" + "trap '\n" + ' if [ "$0" = "$FAKE_RUNNER" ] && [ "${carrier_active:-0}" -eq 1 ]; then\n' + ' case "$FAKE_LAUNCH_GATE:$BASH_COMMAND" in\n' + ' before:\\"\\$codex_path\\"\\ exec\\ *|after:carrier_pid=\\$!) kill -TERM "$$" ;;\n' + " esac\n" + " fi\n" + "' DEBUG\n" + ) + for gate in ["before", "after"]: + with self.subTest(gate=gate): + result = self.run_worker(extra_env={"BASH_ENV": str(bash_env), "FAKE_RUNNER": str(RUNNER), "FAKE_LAUNCH_GATE": gate}) + self.assert_terminal(result, "INTERRUPTED") + self.assertIsNone(result.status["carrier_exit"]) + self.assertFalse((result.run_dir / "carrier.exit").exists()) + self.assertIn("teardown", result.status) + self.assertNotIn("unbound variable", result.process.stderr) + def test_time_lookup_failure_is_diagnostic_only(self) -> None: date = self.bin_dir / "date" date.write_text("#!/bin/bash\nexit 1\n") diff --git a/skills/sshx/tests/test_sshx_contract.py b/skills/sshx/tests/test_sshx_contract.py index 1d1143f5..456f94bb 100644 --- a/skills/sshx/tests/test_sshx_contract.py +++ b/skills/sshx/tests/test_sshx_contract.py @@ -186,8 +186,10 @@ "GoalArtifact", "GoalArtifact.success_criteria", "HEAD", + "INT", "InlineConsensusProtocol", "MEMORY.md", + "TERM", "SshxResultEnvelope", "SshxResultEnvelope.conclusion", "SshxResultEnvelope.log_ref", @@ -309,7 +311,7 @@ "When a repair consumes the reserved capacity, the caller may add evaluation units after seeing " "the repair result so the mandatory rerun review and termination roster remain reachable." ) -CANONICAL_NORMATIVE_DOCUMENT_SHA256 = "392920e019a92b8ac4c1376958156941679022687595fa16e229065277e8e916" +CANONICAL_NORMATIVE_DOCUMENT_SHA256 = "8514822fbb9a98266ea9529546422bdce77828e1100fc0e5cdd497f2bfd0a8f9" JsonValue: TypeAlias = None | bool | int | float | str | list["JsonValue"] | dict[str, "JsonValue"] GapOwnerAssignment: TypeAlias = tuple[JsonValue, JsonValue] @@ -2549,7 +2551,7 @@ def test_sshx_batch_signal_semantics_are_spec_owned(self) -> None: for required in [ "records every child, and joins every recorded child before publishing a report", "its signal handling, interruption reporting, and inherited-disposition limits are owned by `CODEX_WORKER_SPEC.md` and the script's behavior tests", - "whole-job-tree teardown remains the host's responsibility", + "signalling each runner for batch cancellation remains the host's responsibility", "Caller-authored `&`, `nohup`, `disown`, and `setsid` remain forbidden", ]: self.assertIn(required, worker_delegation) @@ -2789,7 +2791,7 @@ def test_worker_spec_source_regression_has_executable_rollback_and_threat_bounda "TOCTOU races", "an active `setsid` escape", "forged runner artifact paths", - "A default `TERM` sent only to the runner PID may be deferred", + "A `TERM` sent only to the runner PID therefore initiates carrier-tree teardown", "An uncatchable `SIGKILL` sent only to the runner PID", "Before either runner-owned projection is written", "temporary and final target must be absent or a non-symbolic-link regular file",