ExecBackend::remove deletes the exec log as part of GC, so a crash-looping sidecar erases its own diagnostic on every relaunch

[schickling-assistant]

Verified at 46ff6ee1db2cae1b52b7730ae75ba781e0bb6605, which was main HEAD at the time of filing — you pushed it a couple of hours before we looked, so this may already be handled in unpushed work. Happy for it to be closed as a duplicate if so.

The path

remove() is documented as the GC path, and it deletes the log alongside the pidfile:

/// Remove an exec task's runner-state (pid + log) — the GC path.
pub fn remove(&self, id: &str) -> anyhow::Result<()> {
    let _ = fs::remove_file(self.pid_path(id));
    let _ = fs::remove_file(self.log_path(id));
    Ok(())
}

src/exec_backend.rs:177-182

A dead-but-still-desired exec task goes into both the GC set and the launch set in the same plan:

SessionState::Dead if target.keep => {}
SessionState::Dead => {
    plan.gc.push(target.pty_id.clone());
    to_launch.push(target);
}

src/reconcile.rs:176-180

and execute reaps before respawning:

// Reap the corpse first (a dead session blocks respawn), then respawn.
if gc_set.contains(target.pty_id.as_str()) {
    match runner.remove(&target.pty_id) { ... }
}
match runner.spawn(target, spec_dir) { ... }

src/run.rs:478-491

So GC is not only the retirement path — it is on the ordinary crash-relaunch path. With st2 up's default --interval 30, a crashed sidecar's output is readable for at most ~30s (a crash produces no folder change, so the timer is what fires).

Why we think this is the wrong side of a tradeoff st2 already makes elsewhere

Every other branch treats the corpse as evidence. This is the one that doesn't:

• RestartDecision::GaveUp (mode=fail exhausted) → continue before GC, commented "Parked (mode=fail exhausted): surface it, leave the corpse as evidence." (src/run.rs:462)
• Delaying / RateLimited → continue, "transient — skip quietly, retry a later pass, keep the corpse." (src/run.rs:474)
• keep pins state against GC — but SessionState::Dead if target.keep => {} also skips the relaunch (src/reconcile.rs:176), so there is no configuration that yields retention and restart. keep parks the task instead.
• log_path's own doc comment states the intent: "Execs run as detached sidecars, so without this a wedged/crashed ding's output vanishes; here it stays inspectable after the fact." (src/exec_backend.rs:44-47)
• spawn opens the log .append(true) and never truncates (src/exec_backend.rs:60-64), which reads as an intent for output to accumulate across generations. remove() is the only thing that defeats that.

Net effect: retention is inverted relative to need. A task parked after giving up keeps its log; a task actively crash-looping — the case where the log matters most — deletes it on every relaunch.

Reproduction

Exec-only scratch catalog, two one-shot passes, no daemon. h1 / probe are invented names.

// agents/h1/probe/agent.kdl
agent "probe" {
  host "h1"
  exec "sidecar" {
    command "echo GEN-LINE-config-error >&2; exit 1"
  }
}

$ st2 up --catalog "$CATALOG" --host h1 --once
  launched (1): h1.probe.sidecar
# logs/h1.probe.sidecar.log → 1 line: GEN-LINE-config-error

$ st2 up --catalog "$CATALOG" --host h1 --once
  launched (1): h1.probe.sidecar
  gc (1): h1.probe.sidecar
# logs/h1.probe.sidecar.log → still 1 line

Three passes leave the log at exactly 1 line. Since spawn appends and never truncates, retention would give 3. Note that gc and launched name the same id in the same pass.

Direct check that this is a delete rather than an append-over — append a sentinel, then run one pass:

before pass: lines=2 sentinel=1
  launched (1): h1.probe.sidecar
  gc (1): h1.probe.sidecar
after  pass: lines=1 sentinel=0

The GaveUp / RateLimited carve-outs above are read from source only: up_once builds a throwaway FlappingCap, so separate one-shot passes never accumulate history to reach them, and we did not run the looping form.

Why it cost us

We hit this while diagnosing a sidecar that was failing with a clear one-line cause in its log. We read the log; a subsequent reconcile tick GC'd it, and by the next time we looked the diagnostic was gone and the file held only the current generation's output. We spent the following stretch chasing a reconcile race that did not exist. The failure mode that most needs a log is the one guaranteed to erase it.

Suggestions — not a prescription, your call

Any of these would have been enough for us:

• exclude the log from remove() and let it accumulate (append mode already supports it);
• roll it aside on GC (<id>.log → <id>.log.1);
• keep the last N generations;
• or split remove() into "reap for respawn" (pid only) and "retire" (pid + log), so retirement GC still cleans up fully.

Happy to send a patch for whichever shape you prefer.

[myobie]

Completed by 6166008.

Exec crash relaunch now uses a dedicated restart-reap path: the finished log is rotated to one bounded prior generation before a fresh current log is opened, while final retirement removes the PID plus current/prior logs. A failed rotation blocks respawn so evidence cannot be appended over. Retirement regressions cover restart, crash-loop parking, final cleanup, and doctor now requires every retired task record (live or dead) to be absent without requiring presence.

Verified with focused tests, cargo test --all-targets, warnings-as-errors clippy, local nix flake check --print-build-logs, and exact-SHA GitHub Actions: https://github.com/compoundingtech/st2/actions/runs/30248476576. No rollout or live runtime mutation was performed.