fix(runtime): terminate stray sandbox processes between iterations

Makefile

@@ -26,7 +26,7 @@ help:
 	@echo "  go-vet               - Run go vet"
 	@echo "  go-tidy              - Run go mod tidy"
 	@echo "  lint-md-links        - Check markdown files for broken in-repo links and anchors"
-	@echo "  script-test          - Run shell script tests (reconcile-repos, topissues, gitlint-rules, artifact redaction)"
+	@echo "  script-test          - Run shell script tests (reconcile-repos, topissues, gitlint-rules, artifact redaction, kill_stray_processes)"
 	@echo "  test                 - Run all checks: lint-all, go-test, script-test, lint-eval-cases"
 	@echo "  e2e-test             - Run admin e2e tests (CI: OIDC mint; local: gh auth login or GH_TOKEN)"
 	@echo "  behaviour-test       - Run Gherkin behaviour tests (installs fullsend per-repo; CI: OIDC mint)"
@@ -196,6 +196,7 @@ script-test:
 	$(call run-timed,bash internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh)
 	$(call run-timed,bash internal/scaffold/fullsend-repo/.github/scripts/setup-agent-env-test.sh)
 	$(call run-timed,bash hack/gitlab-runner-vm/executor/prepare_validation_test.sh)
+	$(call run-timed,bash internal/runtime/kill_stray_processes_test.sh)
 	$(call run-timed,python3 skills/topissues/scripts/topissues_test.py)
 	$(call run-timed,python3 skills/nextwork/scripts/nextwork_test.py)
 	$(call run-timed,python3 skills/analyze-transcript/analyze_transcript_test.py)

docs/contributing/runtime-implementation.md

@@ -121,13 +121,15 @@ Harness `security.fail_mode` controls whether critical findings **block** the ru
 
 | Interface | Responsibility |
 |-----------|----------------|
-| `runtime.Runtime` | Name, config dir, env exports, bootstrap, run loop, per-iteration artifact cleanup |
+| `runtime.Runtime` | Name, config dir, env exports, bootstrap, run loop, per-iteration cleanup, user processes cleanup |
 | `runtime.BootstrapInput` | Portable agent name/path, skill dirs, and plugin dirs to upload |
 | `runtime.SandboxHooksBootstrap` | Optional `BootstrapInput` extension — runtime-neutral sandbox tool hook config (`security.SandboxHookConfig`); every runtime should honour it |
 | `runtime.TranscriptHandler` | Extract transcripts/debug logs; parse errors for CI annotations |
 | `runtime.DebugLogNamer` | Optional — names the per-iteration debug-log artifact (default `agent-debug.log`) |
 | `runtime.ContextBridger` | Optional — runtime auto-loads only `CLAUDE.md`, so the runner injects a `CLAUDE.md`→`AGENTS.md` pointer (Claude Code: yes; runtimes that read `AGENTS.md` natively: omit) |
 
+**Per-iteration cleanup contract.** When a validation retry reuses the sandbox, the runner calls `ClearIterationArtifacts` before the next iteration. Every runtime runs the shared `clearStrayProcesses` sweep first (it terminates the processes the previous iteration left running as the sandbox user, sparing the exec channel and the `sandbox.KeepAliveCommand` main process), then deletes the iteration's output, sessions and debug log. A failed sweep is reported as a warning and never fails the iteration. The runner holds its sandbox lock (`withSandboxLock` in `internal/cli/run.go`) across the call so the credential refreshers' uploads are never killed mid-write.
+
 A runtime whose `Bootstrap` does not type-assert `SandboxHooksBootstrap` will **not** install Tirith, SSRF, canary, or the other hook scripts. The primary security boundary is the OpenShell sandbox, its L7 egress policy, and credential placeholders (ADR 0017, ADR 0025); the hooks are defense-in-depth that every runtime should wire rather than silently drop ([ADR 0090](../ADRs/0090-runtime-neutral-sandbox-hooks-contract.md)). Fill in the matrix column above either way.
 
 ## Sandbox hook contract

docs/guides/dev/cli-internals.md

@@ -481,6 +481,8 @@ Vendoring commit messages use title + body (upload and stale delete). `github st
 │  │                                          │                   │
 │  │ Phase 1 — inline validation:             │                   │
 │  │ for i := 1; i <= max_iterations; i++ {   │                   │
+│  │   if i > 1: ClearIterationArtifacts      │                   │
+│  │     (sweep stray processes, clear output)│                   │
 │  │   run agent → extract output             │                   │
 │  │   SafeDownload repo (non-fatal on fail)  │                   │
 │  │   run validation script                  │                   │

docs/runtimes.md

@@ -34,6 +34,7 @@ sequenceDiagram
   R->>S: .env, host files
   R->>S: Bootstrap
   R->>S: OIDC token (4-min refresh)
+  R->>S: clean up stray processes (between iterations)
   R->>S: Run (per iteration)
   S->>A: start + hook wiring
   loop tool-use loop

internal/cli/run.go

@@ -1921,7 +1921,15 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep
 
 		// Clear sandbox-side output and transcripts so the next iteration starts fresh.
 		if iteration > 1 {
-			if clearErr := rt.ClearIterationArtifacts(sandboxName); clearErr != nil {
+			// Held across the sweep so a credential refresher's upload/exec
+			// is never caught mid-write (see sandboxMu).
+			clearErr := withSandboxLock(ctx, func(waited time.Duration) {
+				printer.StepInfo(fmt.Sprintf(
+					"Waiting %s for a credential refresh to finish before clearing the sandbox", waited))
+			}, func() error {
+				return rt.ClearIterationArtifacts(sandboxName)
+			})
+			if clearErr != nil {
 				printer.StepWarn("Failed to clear sandbox output: " + clearErr.Error())
 			}
 		}
@@ -3621,6 +3629,81 @@ func readOIDCAuthFile(path string) (string, error) {
 
 var oidcRefreshInterval = 4 * time.Minute
 
+// sandboxMu serializes the between-iteration stray-process sweep
+// (runtime.ClearIterationArtifacts TERM/KILLs every sandbox-user process
+// outside its own exec) against the background credential refreshers that
+// write into the sandbox from the host. refreshOIDCToken's
+// `openshell sandbox upload` runs a sandbox-user `/bin/bash -c 'mkdir -p …
+// && cat | tar xf - -C …'` chain (ppid 1, indistinguishable from a stray)
+// and tar truncates the target on open, so a kill mid-write would leave an
+// empty .gcp-oidc-token until the next 4-minute tick. The whole tick is the
+// expired-token window, not part of it: the truncation destroyed the token
+// that was there, and GHA OIDC tokens live 5 minutes. reseedOpenAIAuth's
+// seed exec is the other writer. Both hold the lock across the write; the
+// iteration loop holds it across ClearIterationArtifacts.
+//
+// Lock-hold budget. The iteration side is the long holder:
+// ClearIterationArtifacts is the sweep exec (runtime's 15s snippet timeout
+// plus sandbox.ExecContext's 10s slack) followed by the file removal (10s
+// plus the same 10s slack), so about 45s worst case. That is how long a
+// refresher can be held off, against a 4-minute OIDC tick and a 5-minute
+// token life. Raising either timeout, or adding a third exec to
+// ClearIterationArtifacts, has to be checked against that margin: once the
+// worst-case hold approaches the tick interval a refresh can miss its slot
+// and the token can expire before the next one lands. Take the lock through
+// withSandboxLock rather than directly, so a panic inside the critical
+// section cannot leave it held — the deferred oidcWg/refreshWg waits would
+// then hang the run instead of surfacing the panic.
+var sandboxMu sync.Mutex
+
+// sandboxLockWarnAfter is how long withSandboxLock waits for the lock
+// before telling the caller's notify that something else holds it;
+// sandboxLockPoll is how often every waiter retries meanwhile. Only the iteration
+// loop passes a notify: a sweep waiting on a refresher stalls visible
+// progress, while a refresher waiting on a sweep is routine.
+var (
+	sandboxLockWarnAfter = 5 * time.Second
+	sandboxLockPoll      = 100 * time.Millisecond
+)
+
+// withSandboxLock runs fn holding sandboxMu (see there for what it
+// protects and for the hold budget); the lock is released even if fn panics.
+// notify, when non-nil, is called once if the lock is still not free after
+// sandboxLockWarnAfter, with the time waited so far. A ctx cancelled while
+// waiting returns ctx.Err() without running fn, so a run shutting down is
+// not held up by a holder's in-flight sandbox exec.
+func withSandboxLock(ctx context.Context, notify func(waited time.Duration), fn func() error) error {
+	if err := acquireSandboxLock(ctx, notify); err != nil {
+		return err
+	}
+	defer sandboxMu.Unlock()
+	return fn()
+}
+
+func acquireSandboxLock(ctx context.Context, notify func(waited time.Duration)) error {
+	if sandboxMu.TryLock() {
+		return nil
+	}
+	// TryLock in a loop rather than Lock, so the wait can be reported while
+	// it is happening and abandoned when ctx is cancelled.
+	start := time.Now()
+	warned := false
+	for {
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-time.After(sandboxLockPoll):
+		}
+		if sandboxMu.TryLock() {
+			return nil
+		}
+		if waited := time.Since(start); notify != nil && !warned && waited >= sandboxLockWarnAfter {
+			notify(waited.Round(time.Second))
+			warned = true
+		}
+	}
+}
+
 func runOIDCRefresh(ctx context.Context, sandboxName, oidcURL, oidcAuth string, printer *ui.Printer) {
 	ticker := time.NewTicker(oidcRefreshInterval)
 	defer ticker.Stop()
@@ -3685,8 +3768,13 @@ func refreshOIDCToken(ctx context.Context, sandboxName, oidcURL, oidcAuth string
 	tmpFile.Close()
 
 	remotePath := sandbox.SandboxWorkspace + "/.gcp-oidc-token"
-	if err := sandbox.UploadFile(sandboxName, tmpFile.Name(), remotePath); err != nil {
-		return fmt.Errorf("copying token to sandbox: %w", err)
+	// The upload's in-sandbox tar truncates the token on open; hold the
+	// sandbox lock so the between-iteration sweep cannot kill it mid-write.
+	uploadErr := withSandboxLock(ctx, nil, func() error {
+		return sandbox.UploadFile(sandboxName, tmpFile.Name(), remotePath)
+	})
+	if uploadErr != nil {
+		return fmt.Errorf("copying token to sandbox: %w", uploadErr)
 	}
 
 	return nil

internal/cli/run_openai.go

@@ -136,8 +136,15 @@ var openAIDeleteBackoff = 3 * time.Second
 // sandboxOpenAIPlaceholder returns the OPENAI_API_KEY placeholder a new
 // process in the sandbox receives right now.
 func sandboxOpenAIPlaceholder(ctx context.Context, sandboxName string) (string, error) {
-	// ExecContext already wraps the command in `sh -c`.
-	out, stderr, code, err := sandbox.ExecContext(ctx, sandboxName, `printf %s "${OPENAI_API_KEY:-}"`, openAIPlaceholderExecTimeout)
+	// ExecContext already wraps the command in `sh -c`. Held under the
+	// sandbox lock so the between-iteration sweep never kills this exec.
+	var out, stderr string
+	var code int
+	err := withSandboxLock(ctx, nil, func() error {
+		var execErr error
+		out, stderr, code, execErr = sandbox.ExecContext(ctx, sandboxName, `printf %s "${OPENAI_API_KEY:-}"`, openAIPlaceholderExecTimeout)
+		return execErr
+	})
 	if err != nil {
 		return "", err
 	}
@@ -206,21 +213,39 @@ func reseedOpenAIAuth(ctx context.Context, h openAIProviderHandle, previous stri
 	// starting at this very moment seeds too (from its own exec
 	// environment, which may still carry the previous placeholder), and
 	// whichever write lands last wins. One re-seed closes that window.
-	for attempt := 0; attempt < 2; attempt++ {
-		_, stderr, code, err := sandbox.ExecContext(ctx, h.sandbox, h.authSeed, openAIPlaceholderExecTimeout)
-		if err != nil {
-			return "", fmt.Errorf("re-seeding pi auth.json: %w", err)
-		}
-		if code != 0 {
-			return "", fmt.Errorf("re-seeding pi auth.json: exit %d: %s", code, strings.TrimSpace(stderr))
-		}
-		if h.authFile == "" {
-			break
-		}
-		_, _, code, err = sandbox.ExecContext(ctx, h.sandbox, "command -p grep -qF "+shellQuote(current)+" "+shellQuote(h.authFile), openAIPlaceholderExecTimeout)
-		if err == nil && code == 0 {
-			break
+	// Only the write is taken under the sandbox lock, and one exec at a
+	// time: the seed writes atomically (mv -f) but the between-iteration
+	// sweep must not kill it mid-run, whereas the grep below only reads and
+	// costs nothing if it is killed. Each hold is one exec (30s + slack), so
+	// a sweep never waits on the whole retry loop — see sandboxMu's
+	// hold budget.
+	seed := func() error {
+		for attempt := 0; attempt < 2; attempt++ {
+			var stderr string
+			var code int
+			err := withSandboxLock(ctx, nil, func() error {
+				var execErr error
+				_, stderr, code, execErr = sandbox.ExecContext(ctx, h.sandbox, h.authSeed, openAIPlaceholderExecTimeout)
+				return execErr
+			})
+			if err != nil {
+				return fmt.Errorf("re-seeding pi auth.json: %w", err)
+			}
+			if code != 0 {
+				return fmt.Errorf("re-seeding pi auth.json: exit %d: %s", code, strings.TrimSpace(stderr))
+			}
+			if h.authFile == "" {
+				return nil
+			}
+			_, _, code, err = sandbox.ExecContext(ctx, h.sandbox, "command -p grep -qF "+shellQuote(current)+" "+shellQuote(h.authFile), openAIPlaceholderExecTimeout)
+			if err == nil && code == 0 {
+				return nil
+			}
 		}
+		return nil
+	}
+	if err := seed(); err != nil {
+		return "", err
 	}
 	printer.StepInfo("pi auth.json re-seeded with the refreshed OpenAI placeholder")
 	return current, nil

internal/cli/run_openai_test.go

@@ -1137,3 +1137,40 @@ func TestResolveOpenAICredential_ConfigFallback(t *testing.T) {
 		assert.Contains(t, err.Error(), "inference.openai in config.yaml")
 	})
 }
+
+// The re-seed's sandbox execs must wait for a between-iteration sweep in
+// progress (sandboxMu): none reaches the fake openshell while the
+// lock is held, and the seed lands once it is released.
+func TestReseedOpenAIAuth_WaitsForSandboxLock(t *testing.T) {
+	binDir := t.TempDir()
+	log := filepath.Join(binDir, "log")
+	script := "#!/bin/sh\ncase \"$*\" in *'printf %s'*) echo polled >> " + shellQuoteForTest(log) + "; printf '" + ph("v222_OPENAI_API_KEY") + "'; exit 0 ;; *auth.json*) echo seeded >> " + shellQuoteForTest(log) + "; exit 0 ;; esac; exit 1\n"
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "openshell"), []byte(script), 0o755))
+	t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+	h := openAIProviderHandle{sandbox: "fs-x", authSeed: `printf '{"openai":...}' > /sandbox/pi-config/auth.json`}
+
+	sandboxMu.Lock()
+	done := make(chan error, 1)
+	go func() {
+		_, err := reseedOpenAIAuth(context.Background(), h, ph("v111_OPENAI_API_KEY"), ui.New(io.Discard))
+		done <- err
+	}()
+	select {
+	case err := <-done:
+		sandboxMu.Unlock()
+		t.Fatalf("re-seed ran while the sweep held the lock (err=%v)", err)
+	case <-time.After(300 * time.Millisecond):
+	}
+	_, statErr := os.Stat(log)
+	assert.True(t, os.IsNotExist(statErr), "no exec reached the sandbox while the sweep held the lock")
+	sandboxMu.Unlock()
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(10 * time.Second):
+		t.Fatal("re-seed did not proceed once the lock was released")
+	}
+	data, err := os.ReadFile(log)
+	require.NoError(t, err)
+	assert.Equal(t, "polled\nseeded\n", string(data))
+}