fix(sandbox): retry a spawn whose launcher interpreter is mid-rebuild

[rnoack1]

Problem / Motivation

wrap_argv prepends sys.executable to every sandboxed argv, so that interpreter
path is on the critical path of every sandboxed spawn. When it is a symlink into a
directory tree that can be rebuilt underneath a live process, the path can vanish for
about a second and then come back.

Rebuilding a managed install tree deletes and re-creates its entries, including the
interpreter symlink itself. A spawn landing inside that window dies with ENOENT on a path
that both existed before it and exists after it. Any packaging that relinks an
interpreter in place reaches this: an environment rebuild, a toolchain reinstall, a
swapped container layer.

Observed symptom: three scheduled script jobs sharing one 900-second tick all fired
inside a single rebuild window. Two died with FileNotFoundError: [Errno 2] No such file or directory naming the interpreter, and each recorded a hard job failure plus a strike
toward auto-pause — for a condition that had already healed by the time anyone looked.

Why it matters

The caller cannot distinguish this from a broken install, so a self-healing one-second
blip is recorded as a real failure. Cron auto-pause is latching and only clears on a
later success, so transient strikes accumulate against jobs that are fine. Because the
window hits every sandboxed spawn at once, jobs sharing a tick fail together, which
reads like a systemic outage rather than a moment of filesystem churn.

What changed (motivation → approach → change)

Root cause is a spawn against a path that is briefly absent, so the fix is to retry that
one spawn rather than to surface it.

1. The retry itself (sandbox.py). popen_limited retries on a 0.25 / 0.5 / 1.0 /
2.0 s backoff (5 attempts, ~3.75 s total). The loop is inline in popen_limited
rather than extracted into a helper, and deliberately so: both spawn audits key on
<relpath>::<enclosing function>, so moving the Popen into its own function migrates
its audit key — stranding the popen_limited entries in _SYNC_ALLOWED and
BENIGN_SPAWNS as stale while the relocated call reads as a brand-new unrouted spawn.
An extraction was tried first and broke four guard tests at once;
TestTheSpawnStaysInsidePopenLimited now asserts the shape by AST so the regression
fails at source.

The gate is narrow, because ENOENT from Popen is ambiguous — it is also raised for a
missing cwd, and a genuinely absent user binary must still fail on the first attempt.
_is_transient_interpreter_enoent therefore accepts only an ENOENT whose filename
is cmd[0] and whose cmd[0] is this process's own sys.executable. A
missing-cwd ENOENT names the directory and a missing user binary names that binary, so
both fall through untouched. filename being populated is not guaranteed by every
platform, so when it is absent the fallback is a live check that the interpreter really
is gone from disk — an observation rather than an assumption that this ENOENT must be
ours.

Two deliberate properties:

• The final attempt is unguarded. Whatever it raises reaches the caller unchanged, so
  a permanently broken install still reports exactly the error it reports today, ~4 s
  later. The retry cannot convert a real failure into a different one.
• Every retry logs a warning. An install tree that has genuinely stopped converging
  stays visible instead of being silently absorbed.

2. The backoff had to be made cancellable (cron_script.py). A retry that waits
seconds is a window in which a cancellation was previously lost, not delayed:
kill_running_process keys on a registered child, and during the backoff none
exists, so it returned False and recorded nothing — the retry then launched the work
and the run reported ok. This rider is the larger half of the diff and is listed
explicitly because it is not implied by "retry a spawn":

• _SPAWNING_JOBS marks a job whose spawn is in flight, so kill_running_process can
  record a cancel in that window instead of dropping it. That is a semantic change:
  it now returns True for a job that is only spawning (one consumer, cron.py).
• popen_limited gained an abort_retry hook, consulted after each backoff, which
  re-raises instead of spawning when a cancel has landed. It is wired at the two cron
  spawn sites. It matters only where cancellation is mediated by a registry keyed on the
  live child; a caller holding the Popen handle and polling a stop flag (such as
  auto_improvement.spine.agent_runner, which calls _terminate_group on the handle)
  loses nothing by omitting it, and the docstring says so.
• _finish_spawn moves a job from spawning to registered under a single hold of
  _PROCS_LOCK, so a cancel can never fall between the two states. When a cancel raced
  the successful Popen, the child is live and unregistered, so the caller kills it and
  reports cancelled — reporting a stop that did not happen would be worse than the
  original bug. _abandon_spawn clears the flag when a spawn produced no child, so it
  cannot leak into that job's next run.

3. Cron output keeps locale decoding, now declared deliberate. Relocating the two
popen_limited calls into their try: blocks made the repo's subprocess-encoding gate
count them as newly added lines. An earlier revision satisfied that by pinning them with
**UTF8_TEXT (text=True, encoding="utf-8", errors="replace") — that was wrong and has
been reverted. A cron runs an arbitrary command, so its output carries the host's
encoding; forcing UTF-8 with errors="replace" turned every non-UTF-8 byte into U+FFFD
before the output was persisted and delivered, which is irreversible corruption of the
thing the user asked to see. Both calls therefore read `text=True,

subprocess-encoding: locale` — the gate's own opt-out marker for deliberate locale

decoding — and TestCommandOutputUsesLocaleDecoding asserts that neither call pins
encoding or errors. Runtime decoding behaviour is unchanged from the base.

Either remedy takes those two calls out of the gate's violation set, so its per-file count
drops the same way, and the gate is a ratchet: the one-line count for cron_script.py in
.github/subprocess-encoding-baseline.txt must move 4 -> 2 or the gate fails with
"1 entry to prune". That is the only .github/** path this PR touches, and it is why the
Fork workflow-change guard lane is red: it needs the allow-fork-workflow-change label
from a maintainer, and no push can clear it.

4. An overlapping wake of the same job is refused. _begin_spawn returns false when the
job is already spawning or registered, and the caller returns without touching the other
run's state. This is required by items 2 and 3 rather than optional: every cancellation
surface here is keyed on the job id alone (kill_running_process takes only an id,
_RUNNING_PROCS holds one child per job), so two concurrent runs make "cancel this job"
ambiguous and whichever finishes its spawn first consumes the flag — a cancel aimed at a run
still in its backoff could be eaten by a rerun that was never cancelled. It also backstops a
documented pre-existing gap: when a deadline fires with a worker already claimed, the
scheduler's own overlap guard clears while the child runs on.

The refusal reports status: "skipped", not "error", and both cron dispatch paths
intercept it before the strike branch. An overlapping wake is a scheduling condition, not a
job defect, so counting it toward auto-pause would inflate strikes for exactly the transient
class this change exists to stop striking. That matches the disposition of the scheduler's
own pre-existing guard, which logs "previous execution still running, skipping" and returns
without counting anything.

5. A raced-cancel command run reports the child's returncode, not -1. When a cancel
races a successful spawn the child is killed and the run reported cancelled; it now carries
proc.returncode (the signal that stopped it), matching the existing post-communicate
cancellation contract rather than inventing a synthetic value. The spawn-failed case still
reports -1, because no child exists and there is no signal to report.

Scope — now all three spawn wrappers, not one. The named cause is wrap_argv prepending
sys.executable to every sandboxed argv, and spawn_shim_argv puts it at cmd[0] for
popen_limited, run_limited (17 call sites, 15 files) and create_subprocess_limited
(41 call sites, 26 files) alike — so the exposure was identical and a point fix on one of
three was not honest to the root cause. All three now retry on the same budget, through one
shared discriminator (_retry_interpreter_enoent): one definition, three call sites, no
copies. The async wrapper backs off with asyncio.sleep, never time.sleep — blocking the
event loop for up to ~3.75s is the exact hazard that wrapper exists to avoid. The final
attempt stays unguarded in every wrapper, so a genuinely broken install reports its original
error, just ~4s later. abort_retry stays on popen_limited alone — it is the only
wrapper with consumers (the two cron spawn sites), and a sibling backoff is therefore not
cancellable: neither sibling hands back a handle a cancellation registry could key on, so the
lost-cancel window the hook closes has no consumer there. An AST test pins that placement.

What deliberately did not move: the three subprocess calls themselves stay lexically
inside their own wrappers. Both spawn audits key on <relpath>::<enclosing function>, so
hoisting any of them into a shared helper would migrate its audit key — stranding the
_SYNC_ALLOWED and BENIGN_SPAWNS entries as stale while the relocated call read as a
brand-new unrouted spawn. An AST test pins each spawn to its own wrapper, and another pins the
async wrapper to an async sleep.

This also cures, rather than defers, the _shell_is_posix_strict latch flagged in review:
that probe runs through run_limited and catches OSError into result = False, which it
caches in _POSIX_STRICT_CACHE for the process lifetime — so a ~1s blip used to make every
later caller believe the shell expands braces. FileNotFoundError is an OSError, so the
retry now absorbs the blip before it reaches that clause. A test pins it, with a negative
control (empty delay budget) proving the latch reappears without the retry.

Two deferrals remain, and no tracking issue is filed for them — deliberately, since filing on
the upstream repo would notify its watchers and that is not mine to do; they are recorded here
instead. First, the broader _POSIX_STRICT_CACHE design still caches a permanent failure, so
a tree down longer than the ~3.75s budget still latches; only the transient blip is covered.
Second, a child that starts and then fails its own imports mid-rebuild (a transient
ModuleNotFoundError for a kiro_crew submodule) is a different phase this does not address.

Tests

New file test/test_sandbox_interpreter_enoent_retry.py, 56 tests in fourteen classes
plus one end-to-end case:

• TestIsTransientInterpreterEnoent — the discriminator, both directions: our own
  interpreter retries; a missing cwd, a missing user binary, and a non-ENOENT OSError
  do not.
• TestPopenLimitedToleratesAnAbsentInterpreter — the budget is spent and then the final
  unguarded attempt raises; a mid-budget success returns that handle.
• TestTheSpawnStaysInsidePopenLimited — AST assertion that the Popen call is
  lexically inside popen_limited, pinning the audit-key constraint above.
• TestAbortRetryClosesTheCancellationWindow — an aborting hook stops after one attempt;
  a False hook does not shorten the retry (negative control).
• TestSpawnToRegisteredIsAtomic — _finish_spawn registers and clears atomically; a
  raced cancel is reported and the child deliberately left unregistered so the caller
  kills it; a kill inside the spawn window records the cancel, and one outside it records
  nothing (negative control); _abandon_spawn cannot leak a flag.
• TestCommandCronSpawnFailureIsCancellable — the spawn-failure arm consumes a recorded
  cancel and reports cancelled; without one the run is still an error (negative control).
• TestOverlappingRunCannotEatTheCancel — a second run is refused while the first is
  spawning and while it is registered; a refused rerun leaves the pending cancel intact; a
  distinct job is not refused, so this is not a global lock (negative control); the slot is
  reusable once the run ends.
• TestCommandOutputUsesLocaleDecoding — the command spawn passes neither encoding nor
  errors; forcing UTF-8 with replacement really does destroy the bytes irrecoverably
  (mechanism, with a positive control); and an AST guard fails at source if either cron
  spawn is re-pinned.
• TestAnOverlappingWakeCostsNoFailureStrike — a refused overlap reports skipped rather
  than error, leaves the other run's claim and pending cancel untouched, and both
  dispatch paths intercept the status before the strike branch.
• TestARefusedOverlapIsNotPersistedAsASuccessfulRun — the refusal sets last_status
  and run_never_started so _execute cannot write a fabricated ok and reset the
  auto-pause budget; it calls neither record_failure nor record_success (negative
  control); and a positive control asserts _execute really does treat a non-error
  status as success, so the other assertions cannot pass vacuously.
• TestRunLimitedToleratesAnAbsentInterpreter — the sibling sync wrapper, same shape as
  the popen_limited class: retries mid-budget, re-raises when the budget is spent, does
  not retry a missing cwd or a missing user binary, and still
  reports the caller's own argv in a CalledProcessError (the retry nests inside that
  handler). No abort_retry case — the hook is not on this wrapper.
• TestCreateSubprocessLimitedToleratesAnAbsentInterpreter — the async wrapper: retries
  mid-budget, re-raises when spent, does not retry a user-binary ENOENT,
  and never calls time.sleep (asserted directly), because blocking the
  loop is the hazard this wrapper exists to avoid. No abort_retry case here either.
• TestAllThreeWrappersShareOneDiscriminator — one _retry_interpreter_enoent definition
  with three call sites and no inline re-implementation of the predicate (negative
  control); an AST assertion that each of the three spawns still lives in its own wrapper,
  pinning the audit-key constraint; an AST assertion that the async wrapper sleeps with
  asyncio, not time; and test_abort_retry_exists_only_on_the_wrapper_with_consumers,
  which pins the hook to popen_limited and asserts its absence on both siblings, so a
  zero-consumer parameter cannot creep back.
• TestTheStrictShellProbeNoLongerLatchesOnABlip — a transient blip during the
  _shell_is_posix_strict probe no longer caches a wrong False for the process lifetime,
  with a negative control (empty delay budget) that reproduces the latch.

Gate results on the current head:

• mypy src/kiro_crew/ with the repo's pinned mypy==1.14.1: Success: no issues found in 1261 source files. An earlier revision failed this lane on
  cron_script.py (discard given str | None); a single-file mypy run had reported
  clean, so the whole-package command is what catches it.
• flake8 src/kiro_crew test conftest.py xdist_budget.py: 0 findings from this change.
• scripts/check_black_formatting.py, scripts/check_subprocess_encoding.py,
  scripts/check_sync_io_in_async.py, scripts/scrub-lint.sh: all pass with the change
  in diff scope. The diff-scoped gates must be run with the upstream base ref; scoped
  against a fork main that is behind, the encoding gate reported a pass it should not
  have.
• 479 passed / 2 skipped across the new file plus test_spawn_preexec_guard.py,
  test_spawn_audit.py, test_cron_script.py, test_cron_script_more_coverage.py and
  test_sandbox_argv.py; 3329 passed across every test/*sandbox*, test/*spawn* and
  test/*cron* file, which is the scope covering all three wrappers and the
  slack/gateway.py dispatch change. The 16 spawn-audit tests pass unchanged, confirming no
  audit key moved.

Screenshots / video

Why no screenshot: no user-visible surface changes — this touches subprocess spawn
wrappers and cron spawn bookkeeping, and adds a unit test file.

Related Issues

N/A — no tracking issue; found while diagnosing scheduled-job failures.

Pattern harvest

Rule candidate: review-prompt
Pattern: a retry that waits is a cancellation window. Adding a backoff to a spawn whose
cancellation is mediated by a registry keyed on the live child converts "cancel
arrives slightly late" into "cancel is discarded", because during the backoff there is no
child to look up and the canceller records nothing. Widening a retry therefore requires
auditing the cancel path, not just the failure path — and the two state changes
(no-longer-spawning, now-registered) must happen under one lock hold or the same cancel
falls through the gap between them.

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 013aa435f277ca28efe0ff2240826a8adc8bd4b4 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Every claim in the description that the base tree can confirm, does: the probe latch at cron_script.py:1874, spawn audits keyed on <relpath>::<enclosing function> in test/test_spawn_audit.py, the success-fabrication hazard at cron.py:3836, the scheduler's own overlap guard at gateway.py:3581, and zero production spawns of sys.executable outside the three wrappers (grepped Popen(\[?sys.executable|create_subprocess_exec(sys.executable|subprocess.run([sys.executable — 1 hit, a test). Final review follows.

First-Principles-Verdict: PASS

A reported cron-killing ENOENT blip, fixed at the one choke point every sandboxed spawn shares, with each rider forced by the retry window the fix itself opens.

What this change ships

Intent: stop a one-second interpreter-relink blip from recording hard cron failures and auto-pause strikes — a FIX.

1. Sandboxed spawns ride out a transient interpreter ENOENT instead of failing — justified
2. A blip during the shell probe no longer permanently latches "no POSIX shell" — justified, same root cause
3. Cancelling a cron mid-spawn-backoff now stops it instead of being silently dropped — rides along, required
4. A second wake of an already-starting job is refused, no strike — rides along, required
5. A refused overlap records "never started", not a fabricated success — justified
6. Raced-cancel command runs report the child's returncode, not -1 — declared
7. Audit field renamed killed_subprocess → cancellation_accepted — derived from item 3
8. Encoding-gate baseline for cron_script.py drops 4 → 2 — mandated by the ratchet

The riders pass the tie-breaker test in reverse: the fix alone does not remove the defect safely, because the backoff it adds is a window where kill_running_process (keyed on a registered child) discards a cancel and the run reports ok after doing cancelled work. Items 3–5 close exactly that. abort_retry: 2 consumers, on the one wrapper that has them — the siblings correctly ship without it, pinned by test. "skipped": 2 producers, 2 consumers. All three wrappers put sys.executable at argv[0], so retrying all three is the general fix, not a point patch; no unrouted sibling exists.

Subtractions

• Delete the hand-rolled _ExitStack class in test/test_sandbox_interpreter_enoent_retry.py (2 uses) — its own docstring names the existing mechanism: contextlib.ExitStack.

[FIRST-PRINCIPLES-REVIEWED] 013aa43

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 013aa435f277ca28efe0ff2240826a8adc8bd4b4 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the load-bearing claims verify against the base tree: the last_status="error" + run_never_started=True + no-record_failure neutral disposition is the established starvation-path pattern (gateway.py:902-923), cron.py:3836 confirms _execute treats any non-error status as success (so the gateway interception was genuinely required, not decoration), the scheduler's own overlap guard exists at gateway.py:3581, and the two patched gateway dispatch paths are the only production consumers of the sandboxed-run results, so the new "skipped" status has no unhandled reader. The cancellation-window rider is a real consequence of adding a backoff (kill_running_process keys on a registered child), and every hunk — including the audit-key rename and the encoding-baseline ratchet — is accounted for by the description. The one asymmetry I found: the command path guards claim-release with a finally, while the script path relies on its narrow claim-to-spawn window staying exception-free.

Design-Verdict: PASS

A real transient-failure harm, fixed at the right layer with a narrow discriminator, and every rider (cancel window, overlap guard) is a verified consequence of the retry.

Suggestions

• Mirror the command path's finally claim-release backstop in run_script_sandboxed: your own comment says a leaked claim refuses every future wake for the process lifetime, yet the script path relies solely on its claim-to-spawn window staying exception-free under future edits.

[DESIGN-REVIEWED] 013aa43

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 013aa435f277ca28efe0ff2240826a8adc8bd4b4 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 013aa43

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 013aa435f277ca28efe0ff2240826a8adc8bd4b4 via the fork AI-review pipeline; updated in place on each push.

Review details

I've independently traced every load-bearing claim in this PR against the code at the PR commit:

• Spawn-claim bookkeeping, both paths. In run_command_sandboxed the spawn_claimed flag is cleared before every _finish_spawn/_abandon_spawn and the finally releases on all other exits (no-shell, sandbox-refusal, raised error); in run_script_sandboxed _begin_spawn is taken at the last statement before the try, so no exit between claim and _finish_spawn/_abandon_spawn can leak _SPAWNING_JOBS. _finish_spawn returning True leaves the child unregistered and the caller kills it; the communicate finally always _unregister_procs. No leak, no double-abandon.
• Gateway "skipped" handling. Confirmed against _execute: last_status="error" skips the success branch, record_success/record_failure are both absent so no strike is spent or refilled, and run_never_started=True matches the timeout path's neutral discriminator. Returning None alone would have been read as success — the branch avoids that.
• Retry discriminator. Narrow by construction: cmd[0] == sys.executable and (exc.filename == cmd[0] or a live os.path.exists check). In create_subprocess_limited only the interpreter is exec'd directly, so an ENOENT there is unambiguously the interpreter; a missing user binary or missing cwd still fails on attempt one. abort_retry is consulted only after the sleep and lives only on popen_limited.
• text=True is behaviorally unchanged from base (only a pragma comment added); the baseline count drop is a tightening. The killed_subprocess → cancellation_accepted change is audit-metadata only with no other reader.

Nothing survives to the 80+ bar, and I could not ground a new defect of my own.

No findings.

[OPUS-REVIEWED] 013aa43

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7414 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7414: KEEP. No behavioural disagreement, but the same statement is rewritten by both, so the second to land needs a deliberate rebase that keeps PR #7670's relocated claim boundary AND PR #7414's _command_argv call. Cheap to resolve; worth naming so it is not resolved by dropping one side. Files: src/kiro_crew/cron_script.py.
• This PR is OVERLAPPING with PR #7140. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7670: KEEP. Large independent feature touching the same two cron spawn lines; a sequencing conflict only, already cross-referenced by its own author. Files: src/kiro_crew/cron_script.py, src/kiro_crew/sandbox.py.
• This PR is OVERLAPPING with PR #7787. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7670: KEEP. Complementary changes contending for the same lines. Only sequencing is needed, not a decision between them. Files: src/kiro_crew/cron_script.py, src/kiro_crew/slack/gateway.py.
• This PR is OVERLAPPING with PR #8232. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7670: KEEP. Neighbouring edits in one function with no behavioural interaction. Files: src/kiro_crew/cron_script.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[rnoack1]

Verified all four relationship findings at the current head b95838831. Agreed on all four, including KEEP for this PR; nothing is owed here. One of them is worth pinning to exact coordinates so it cannot be resolved by dropping a side.

#7414 — the contended statement, and where the boundary now sits

Confirmed: both PRs rewrite the same argv statement in run_command_sandboxed. #7414 replaces it with a _command_argv(shell, command) call; this PR relocated it. At b95838831 the coordinates are:

• src/kiro_crew/cron_script.py:1198 — if not _begin_spawn(job_id):, the spawn claim, hoisted above the shell probe
• src/kiro_crew/cron_script.py:1246 — argv = [shell, "-c", command], the contended statement, now inside that claim
• src/kiro_crew/cron_script.py:1349 — the _abandon_spawn(job_id) release in the finally, closing the boundary

So the composition for whichever lands second is argv = _command_argv(shell, command) at 1246's position — inside the claim, not back above it. The boundary is load-bearing rather than cosmetic: the claim was hoisted above the probe because resolving the shell runs _shell_is_posix_strict through run_limited, which now carries an interpreter-ENOENT backoff. On a cold probe cache that sleeps for seconds while the job is registered nowhere, and a cancel arriving in that window was dropped rather than delayed, so the cancelled command still launched. Narrowing the boundary back to the spawn alone reopens that.

#8232 — agreed, no behavioural interaction. Noting for sequencing that it has merged and its merge commit is not yet an ancestor of this head, so it meets this branch's next rebase rather than being already under it.

#7140 and #7787 — agreed, sequencing only. Both currently read CONFLICTING against main on their own, so each carries that work independently of this PR.

[bolichen97]

Tech Lead review: code change verified correct and safe. Applied allow-fork-workflow-change label to clear the fork-workflow-change-guard block on the trusted .github/** path touch. Approving; will re-check CI after the guard re-runs before merging.