fix(agent): cancel a persistent-session command by resetting the session

docs/adr/0011-session-cancel-resets-session.md

@@ -0,0 +1,147 @@
+# 0011 — Cancelling a persistent-session command resets the session
+
+Date: 2026-09-18
+Status: accepted
+
+## Context
+
+`wanctl exec --target <dev> "sleep 600"` on the persistent-session path, then
+Ctrl-C: the controller exited and the `sleep` on the device ran to completion
+(issue #46). PR #44 had fixed the same thing for `--oneshot` and deliberately
+left this alone, because a one-shot owns its shell and a session does not: the
+session's shell holds the working directory and environment that later
+connections rely on.
+
+The issue listed three options. (a) kill the foreground child and keep the
+shell, (b) tear the session down and build a fresh one on the next connect,
+(c) document that persistent sessions do not cancel. (a) is what a terminal
+does and was implemented first. An adversarial review of that implementation
+(PR #109, 2026-09-18) found it structurally unfixable, not buggy.
+
+**The agent submits a line, not a process.** `ShellSession` runs a command by
+writing it to the shell's stdin, so the shell owns the whole line. Killing the
+child it happens to be running now leaves the rest of the line to execute in the
+surviving shell. Measured: cancelling
+`sleep 600; echo AFTER_CANCEL > f; cd d; export X=changed` while the sleep ran
+killed the sleep, and the shell then wrote the file, changed directory and set
+the variable — while the caller was told `context.Canceled`. A second `sleep` in
+the same line would simply be waited on again. There is no shell-side protocol
+to skip the remainder without adding one, and the marker protocol has no channel
+to carry it.
+
+**A pid/ppid snapshot cannot prove descent.** The implementation found "the
+foreground child" by walking a process table. A parent pid is a number that
+outlives its owner: process 100 starts 200 and exits, the new session shell
+gets pid 100, and 200 — unrelated, older than the shell — is now in the kill
+list. Between taking the snapshot and sending the signal the target can exit and
+its pid be reused, so even a correct tree kills the wrong process. This is not a
+race window to narrow; process identity is simply not what pid/ppid carries.
+
+**`set -e` destroyed the session anyway.** With errexit set, SIGKILL of the
+child made the non-interactive shell exit on the non-zero status without
+printing the end-of-command marker. The mechanism whose entire purpose was to
+keep the shell lost it, silently, in a common configuration.
+
+**The snapshot was not portable and its failure was invisible.** `ps -A -o
+pid,ppid` is not in BusyBox builds without DESKTOP, which OpenWrt disables by
+default. The error was discarded, so the cancel did nothing and the caller was
+still told the command had been stopped.
+
+A fifth, subtler one: a cancellation goroutine armed by request A could still be
+running when A finished and B started, and kill B's child. B reported exit 137
+with nothing cancelled.
+
+## Decision
+
+**Option (b): a cancelled session is destroyed, and the next command on that
+target builds a fresh one.** The unit of cancellation is the session, because
+the session is the thing the operating system can name.
+
+**The shell runs inside an OS process container.** On Unix the session shell is
+started with `SysProcAttr.Setpgid`, making it the leader of a new process group
+whose id is its own pid; cancelling sends `SIGKILL` to `-pgid`. On Windows the
+shell is created suspended, assigned to a job object made without
+`JOB_OBJECT_LIMIT_BREAKAWAY_OK`, and only then resumed, so a shell that could
+not be contained never runs an instruction; cancelling calls
+`TerminateJobObject`. Both are single kernel operations on a set the kernel
+maintains, so neither needs `ps` and neither leaves part of the submitted line
+running — the shell that would have run it is gone. The job also carries
+`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so an agent that exits without closing its
+sessions still takes them with it.
+
+**The two containers are not equally strong, and the weaker one is fenced rather
+than trusted.** A job handle names its job; a process-group id is only a number,
+and once the group is gone that number can be given to another group. So the
+Unix container signals at most once in its lifetime, and never after the shell
+has been reaped. Until the reap the shell is at worst a zombie, and the kernel
+does not reuse a group leader's pid while its zombie exists, so the number still
+names this group and nothing else. `reap` is called the moment `cmd.Wait`
+returns, under the same lock the kill takes, so the two cannot interleave. The
+earlier claim in this ADR that the mechanism "cannot be defeated by pid reuse"
+was too strong: what holds is that the one signal it sends is sent while the
+number is still unambiguous.
+
+**Cancelling does not wait for the shell's descendants.** The shell's stdout is
+an OS pipe inherited by everything it forks, so a process that escaped the
+container still holds the write end after the container is killed. The copier
+feeding the session's reader then never sees EOF. If the cancellation waited for
+it, the command would never return, `Closed()` would never answer, and — because
+the agent asks `Closed()` while holding the lock that guards every session on
+the device — one escaped process would stall every session on the machine. The
+cancellation therefore closes the read half itself, and the reaper carries a
+`WaitDelay` so its own goroutine and the pipe's descriptors are released on a
+bound rather than never. Cutting the output is also what makes a kill that
+*failed* reach the caller immediately instead of whenever the command it could
+not stop happens to end.
+
+**A cancellation is bound to the request that armed it, by generation.**
+Disarming takes the same lock the firing path holds, so it waits for an
+in-flight cancellation rather than merely signalling it to stop, and it happens
+while the request still holds the session lock. That alone is not enough: a
+watcher can wake *after* its own request disarmed and after the next one armed,
+and a shared "is anything armed" flag reads true in that state — measured at 50
+misfires in 100 runs. So every arming takes a generation number, the watcher
+carries the one it was armed with, and firing does nothing unless that
+generation is still the live one. A request whose context is already cancelled
+when it takes the session lock runs nothing.
+
+**A request that was holding a session someone else cancelled gets a fresh
+one.** The session distinguishes "closed before your command was submitted"
+from every other failure, and that single case is answered by acquiring a new
+session and running once. Nothing else is retried: every other error leaves open
+the possibility that the command did run, and running it twice is worse than
+reporting it once.
+
+**A kill that fails is reported.** The error reaches the caller attached to the
+cancellation, because "the command was cancelled" is the one answer that must
+never be returned when the device could not stop anything.
+
+## Consequences
+
+- **Cancelling loses the session's working directory and environment.** That is
+  the trade: a cancel that reliably stops everything, for state that does not
+  survive it. The `exec` catalog entry and `docs/contract.md` say so, and point
+  at `--oneshot` for work you may want to abort and keep nothing from.
+- `--oneshot` and `exec_async` are untouched. A one-shot already owns its shell
+  and dies with it; an async job is detached from any session on purpose and is
+  not cancelled by a controller leaving.
+- **What still escapes on Unix:** anything that leaves the process group. That
+  is `setsid()`, daemons that double-fork out of it, and — more easily reached
+  than either — `setpgid()`, including the `set -m` that turns on job control
+  and puts each background job in a group of its own. `set -m; sleep 600 &` in a
+  session survives the cancel. That is the documented boundary of a process
+  group, and it is the same boundary the `--oneshot` cancel hook has had since
+  #37. What such a process can no longer do is hold the session open: the
+  cancellation stops waiting on it.
+- **Nothing created by the contained shell escapes the job object on Windows**,
+  because the job has no breakaway rights and the shell is assigned to it before
+  it runs. What is outside that promise is a process the shell asks some other
+  service to create on its behalf — `Win32_Process.Create` over WMI is the
+  documented example — because the creating process is that service, not
+  anything in the job. Nested jobs are supported from Windows 8, but not under
+  every ancestor job configuration; an assignment that is refused fails the
+  session start rather than quietly producing an uncontained shell.
+- A shell builtin that forks nothing is no longer a special case. It used to be
+  uncancellable because there was no child to kill; now the shell running it is
+  what dies.
+- The Windows path is cross-compiled and vetted but has not been run on Windows.

docs/contract.md

@@ -345,7 +345,7 @@ background the command on the device instead.
 | `script` | `--script <local-file>` | string | no | Script SOURCE to run on the device (not a file path). Sent encoded, so quoting and character-set rules do not apply: $, backticks, nested quotes and non-ASCII text all arrive literally. Requires 'interp'. Use this for multi-statement work; it is the same single call as 'command'. Scripts over ~9KB must be pushed as a file and run by path instead. **On the CLI:** Path to a script FILE on this machine. Its contents are sent base64-encoded and run on the device, so quoting and character-set rules do not apply: $, backticks, nested quotes and non-ASCII text all arrive literally. The interpreter comes from the extension (.ps1 → PowerShell, .sh or none → sh) unless --interp says otherwise. This is the CLI spelling of the MCP `script` argument, which carries the source itself rather than a path. |
 | `interp` | `--interp powershell\|sh` | string | no | Interpreter for 'script': 'powershell' for Windows devices, 'sh' for Unix/macOS/Android. Required when 'script' is set. **On the CLI:** Override the interpreter --script would infer from the file extension: powershell \| sh. |
 | `cwd` | — | string | no | Working directory on the device for this command (also the policy scope). |
-| `oneshot` | `--oneshot` | boolean | no | Run in a fresh shell with no persistent session state. Default false — successive exec calls share cwd/env like a real terminal. |
+| `oneshot` | `--oneshot` | boolean | no | Run in a fresh shell with no persistent session state. Default false — successive exec calls share cwd/env like a real terminal. Note that aborting a command (Ctrl-C, or losing the connection) RESETS the shared session: the shell is killed together with every process still in its process group (Unix) or job (Windows), and the next exec starts in the default directory with a default environment. A process that detached itself from that group or job — setsid, setpgid, a shell with job control ('set -m'), or one created through an external service — can survive the abort. Use --oneshot for anything you may want to abort, so there is no shared state to lose. |
 | `elevate` | `--elevate` | boolean | no | Android only. Run with elevated privilege (uid 0 or the adb shell uid 2000) instead of the app sandbox the agent normally lives in. This is what makes `pm`, `am`, `input`, `screencap`, `dumpsys`, `settings`, `wm` and `svc` work at all — without it they fail with permission errors or empty output. Elevated commands need their OWN policy rule on the device; a device in bypass mode still refuses them until a human approves, so expect a 'PAIRING/approval' style rejection the first time. |
 | `via` | `--via su\|adb` | string | no | Pin the elevation channel: 'su' (rooted device) or 'adb' (device's own wireless debugging). Default empty = let the device pick whichever is available. Naming an unavailable channel fails instead of quietly running unprivileged. |
 

internal/agent/agent.go

@@ -11,6 +11,7 @@ import (
 	"crypto/tls"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -917,6 +918,10 @@ func (a *Agent) doExecAuthorized(conn *tls.Conn, fp, peerName string, m protocol
 	// goes away mid-command (Ctrl-C) or sends a cancel frame cancels it, and
 	// the per-platform cancel hook kills the shell and its children instead of
 	// leaving an orphan running to completion on the device (#37).
+	//
+	// A persistent session cancels the same way, and the session goes with it:
+	// the shell is fed through stdin, so stopping one command in it and keeping
+	// the rest is not something the device can promise (#46, ADR 0011).
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	pending := watchPeer(conn, cancel)
@@ -975,19 +980,21 @@ func (a *Agent) doExecAuthorized(conn *tls.Conn, fp, peerName string, m protocol
 		} else if m.OneShot {
 			code, err = server.RunOneShotContext(ctx, a.opts.Shell, m.Command, m.Cwd, out)
 		} else {
-			sess, serr := a.session(fp)
+			var serr error
+			code, err, serr = a.execInSession(ctx, fp, m, out)
 			if serr != nil {
 				spill.Close()
 				protocol.WriteMessage(conn, protocol.Message{Kind: protocol.KindError, Reason: serr.Error()})
 				return pending
 			}
-			code, err = sess.ExecInDir(m.Command, m.Cwd, out)
 		}
 	}
 	if err != nil {
-		if ctx.Err() != nil {
+		if ctx.Err() != nil && !errors.Is(err, server.ErrSessionCancelled) {
 			// Say who ended it: the audit line and a controller that is still
-			// listening must not read this as the command itself failing.
+			// listening must not read this as the command itself failing. A
+			// session cancellation already says so, and may carry a kill that
+			// failed, so it is passed through untouched.
 			err = fmt.Errorf("command cancelled by the controller")
 		}
 		a.logSessionEvent(audit, eventlog.Event{Type: "exec", PeerFP: fp, PeerName: peerName, Detail: m.Command, Cwd: m.Cwd, Decision: decision, Via: string(ranVia)})
@@ -1196,6 +1203,47 @@ func (a *Agent) serveSessionHTTP(ctx context.Context, base string, open sessiona
 	a.handleSession(ctx, nc, open)
 }
 
+// execInSession runs a command on this controller's persistent session. serr is
+// set only when no session could be built at all; anything the command itself
+// reported comes back in err.
+//
+// A session can be cancelled and dropped by another request between this one
+// acquiring it and running, which used to surface as "session closed" for a
+// command that never reached the device. The session says so precisely, and
+// that one error is answered by acquiring a fresh session and running once.
+// Nothing else is retried: every other failure leaves open the possibility that
+// the command did run, and running it twice is worse than reporting it once.
+func (a *Agent) execInSession(ctx context.Context, fp string, m protocol.Message, out io.Writer) (code int, err, serr error) {
+	for attempt := 0; ; attempt++ {
+		sess, sessErr := a.session(fp)
+		if sessErr != nil {
+			return -1, nil, sessErr
+		}
+		code, err = sess.ExecInDirContext(ctx, m.Command, m.Cwd, out)
+		if sess.Closed() {
+			// Cancelling a session command destroys the session by design
+			// (#46). Drop it so the next command on this target builds a fresh
+			// shell rather than finding a dead one.
+			a.dropSession(fp, sess)
+		}
+		if errors.Is(err, server.ErrSessionUnusable) && attempt == 0 {
+			continue
+		}
+		return code, err, nil
+	}
+}
+
+// dropSession forgets a session and tears it down, so the next command for this
+// controller starts a new shell.
+func (a *Agent) dropSession(fp string, sess *server.ShellSession) {
+	a.sessMu.Lock()
+	if a.sessions[fp] == sess {
+		delete(a.sessions, fp)
+	}
+	a.sessMu.Unlock()
+	sess.Close()
+}
+
 func (a *Agent) session(fp string) (*server.ShellSession, error) {
 	a.sessMu.Lock()
 	defer a.sessMu.Unlock()