Repository navigation
T31 — Keep terminal and detached execs past a client loss, stop one exec, and honour exec_enabled - #2874
Conversation
A stop ends one exec through a StopExec RPC: SIGTERM to its process group through the injection shim, a wait, then SIGKILL. The box and its other execs keep running (BOX-151). A box whose stored spec sets [io] exec_enabled = false refuses every command with exit 5, and the refusal names the key (BOX-153). The shim makes a tty exec lead a new session with its PTY as the controlling terminal. exec_started and exec_exited events now carry the principal and the argv beside the PTY flag and the exit code (BOX-152). A test pins that PTY and detached execs survive the loss of their client (BOX-150).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: 2 findings, none blocking at Address the comments on the diff. Clobber: not checked with git log. If that was not intentional, restore the earlier change. |
…nal-and-detached-execs-past-a-client-l
There was a problem hiding this comment.
arch-review: CONDITIONS · 4 conditions · the full ruling
The stop of one exec does not match line 497 of architecture.md. A tty exec now gets a controlling terminal, so a shell job can move to its own process group and survive the stop. C1 fixes this. An attached exec cannot be stopped at all, and C3 fixes this. The exec_enabled check lets the exec run when the stored spec cannot be read or parsed. C2 makes it refuse. File the two residuals in the PR body as issues and link them (C4).
Ruled at 3f85533 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
A stored spec that cannot be read or does not parse as JSON now refuses the exec with exit 5, in place of letting it run. The refusal says the spec cannot be read, and the daemon log names the cause.
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The exec_enabled gate now fails closed, and the events carry the fields that line 497 names. Two gaps remain in the stop of one exec (BOX-151). A tty exec now leads a session with the PTY as its terminal, so a shell runs jobs in other process groups. The shim kills only the first group, so those jobs survive a stop. C1 fixes this. A plain attached exec has an id but cannot be stopped, and the RPC says that it does not exist. C2 fixes this. C3 files the client path and the mfile field as residuals with issues.
Ruled at a8e3fe7 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A tty or detached exec keeps running after its client disconnects. A person can stop one exec by its id, and the box stays running. A box whose stored spec sets [io] exec_enabled = false refuses commands with exit 5. A box whose stored spec cannot be read or parsed now also refuses. Notes from the agent: Revise round 1. ExecBox::exec_disabled() is now exec_gate(), and it returns ExecGate { Enabled, Disabled, SpecUnreadable }. A spec that cannot be read or parsed refuses with exit 5 and its own message: "box does not run commands: its stored spec cannot be read, so the daemon cannot check [io] exec_enabled." The message gives no next step, because no command repairs a stored spec. A spec that parses but has a non-bool exec_enabled still leaves exec on, because only |
|
/revise Four fixes on
Already fixed at this head: the fail-closed |
…nal-and-detached-execs-past-a-client-l
A stop or a timeout of a tty exec now ends every process group of the exec's session, so a job a shell put in its own group ends too. An attached exec is registered by its id, so a stop or a wait finds it. The exec events store argv with its secrets masked. The output pump of a detached exec stops at the drain after the exec ends. The exec gate refuses a stored spec that is not a JSON object.
…nal-and-detached-execs-past-a-client-l
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 8 unresolved threads — minimal-arch[bot] crates/minimald/src/nsenter.rs:845, minimal-arch[bot] crates/minimald/src/exec/process.rs:1072, minimal-arch[bot] crates/minimald/src/exec/process.rs:1431, minimal-arch[bot] crates/minimald/src/rpc.rs:1446, minimal-foundry[bot] crates/minimald/src/exec/process.rs:1117, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The compare fixes the stop of a tty exec, the stop of an attached exec and the fail-closed gate, and those parts conform. One new item deviates. The exec events now store a masked argv. architecture.md line 497 says: "The argv is retained as typed". The BoxRead gate on the stream and the min secret rule are what protect it. C1 restores the argv as typed in the events. C2 names an issue for the mfile exec_enabled field and the min box stop --exec client, as residuals in the PR body.
Ruled at 60a5c0e against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
/revise Two changes on
|
…nal-and-detached-execs-past-a-client-l
architecture.md line 497 and gominimal/arch#76 keep the argv as typed in the box's event stream, behind the BoxRead gate. Remove scrubbed_argv. The daemon log still masks the argv with scrub_secrets.
|
Round 3 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A tty or detached exec keeps running after its client disconnects. A stop of one exec ends only that exec, and the box and its other execs keep running. The exec_started and exec_exited events store the argv exactly as the request sent it. Notes from the agent: This round stores facts.argv as typed under exec_data::ARGV and removes scrubbed_argv. The daemon log lines keep scrub_secrets. The test exec_events_carry_principal_argv_pty_code now asserts that both events store the argv exactly as the request sent it. Residuals
I cannot edit the PR body. Please add the Residuals section above to it. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The code conforms to architecture.md line 497 for BOX-150 to BOX-153. The new commit keeps the exec argv as typed in the events, as line 497 says. Two pieces of work stay undone, and no issue owns them. First, mfile cannot parse [io] exec_enabled, so a real box spec cannot set it (C1). Second, the StopExec types are not in minimald-rpc, and no min box stop --exec command calls them (C2). File the two issues and list their URLs in the PR body. No code change is necessary.
Ruled at 95c66b3 against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
/revise Three fixes on
|
|
That What the platform saidcould not start the coder: Error: there is no container instance that can be provided to this durable object Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
…nal-and-detached-execs-past-a-client-l
Register the stop token before the exec's entry, and write exec_started after both, on the attached and the managed paths. A stop of a stored, ended exec now answers that it already ended, with its code.
The case checks that the exec's PTY is its controlling terminal, and that a stop ends a background job in another process group of its session.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The compare keeps BOX-151 conformant. A stop is now registered before the exec is visible, and a stop of an ended exec answers from the stored exec_exited event (line 497). Nothing else changes from the ruling on 95c66b35. C1 (#2973) and C2 (#2913) stay open as filed residuals, and the change can merge with them. The new shim test runs in no lane because nothing sets MINIMALD_NSENTER_TEST. Do not count it as a proof leg.
Ruled at b8edbaa against gominimal/arch@7a86ec7bb3c0 by claude-opus-5-5.
|
Round 4 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A terminal or detached exec keeps running after its client goes away. A stop of one exec ends it and the box keeps running. That is also true when the stop arrives as soon as the exec_started event is written. A stop of an exec that ended before a daemon restart answers that it already ended, with its code. Notes from the agent: Item 1: both paths now register the stop token first, then the exec's entry, then write exec_started. A test-only hook in ExecBox::write_event (exec/process.rs, stop_at_start) stops the exec when its exec_started event is written. Tests use it for the attached, detached and tty modes. The attached test fails on the old order. The managed-path race was between register_exec and register_stop, and both came before the event, so the managed test pins the new order but does not reproduce the old race. Item 2: StopExecResponse gets already_ended (serde default). stop_exec_answer now takes the daemon state dir. It reads the stored exec_exited through BoxDir::find and stored_exec_exit when no live entry exists. A failed read of the box index or record answers that the end is not known, not 'no exec'. Item 3: tests/nsenter_integration.rs (outside the layers, accepted by the maintainer) gets tty_exec_pty_is_controlling_terminal_and_a_stop_ends_its_jobs. It is #[ignore] and gated on MINIMALD_NSENTER_TEST, like the other cases there. I ran it on this box with the gate set, and it passed. The two arch threads ask for issues to be filed and linked in the PR body. I have no credential to file issues, so a person must file them. just clippy-strict was not run. |
|
The runtime will not revise this pull request again on its own: the diff is +1646/-59 (1705 lines), past 3x the plan's estimate of 480 lines (est: 220 code, 260 tests, as #2406 reads now), at round 4 (the last trigger was a /revise note). A person decides now. Remove |
There was a problem hiding this comment.
Reviewed at b8edbaac.
- Earlier review: a whole-PR review at
95c66b35found no blockers. It ran the ten tests and mutation-tested the attached-exec stop. - This round: it lands the three requested fixes.
- The stop is registered before the exec is visible, on the attached and managed paths alike (
8972c6af), with first-sight stop tests. - A stop after a daemon restart answers from the stored
exec_exited(stored_exec_stop) and keeps "no such exec" for ids never started. - A tty exec runs through the real nsenter shim in
nsenter_integration.rs(b8edbaac).
- The stop is registered before the exec is visible, on the attached and managed paths alike (
- Design: argv is stored as typed in events, per architecture.md line 497 and arch#76, and log lines stay scrubbed.
- Residuals: #2973 (the mfile
exec_enabledfield) and #2913 (the RPC move and themin box stop --execclient). Follow-ups are on #2978. - The macOS
unitfailure was an unrelatedminvmdproxy-reclaim flake, and it passed on rerun.
T31 — Keep terminal and detached execs past a client loss, stop one exec, and honour exec_enabled
slice: S7
kind: task
covers: BOX-150, BOX-151, BOX-152, BOX-153
size: M
est: 220 code, 260 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: daemon log names exec stops and refusals
observability: exec events carry argv, flag and code
layers:
- exec:
- crates/minimald/src/exec.rs: survival; exec_enabled refusal
- crates/minimald/src/exec/process.rs: stops one exec
- crates/minimald/src/nsenter.rs: tty exec PTY is its controlling terminal
- crates/minimald/src/exec/tests.rs: tests
test: cargo nextest run -p minimald pty_or_detached_exec_survives_client_loss
test: cargo nextest run -p minimald stop_exec_ends_process_group
test: cargo nextest run -p minimald exec_disabled_refuses_exit5
test: cargo nextest run -p minimald tty_exec_pty_is_controlling_terminal
- events:
- crates/boxes/src/events.rs: exec event kinds
- crates/minimald/src/rpc.rs: exec stop handler
test: cargo nextest run -p minimald exec_events_carry_principal_argv_pty_code
produces: none
consumes:
- crates/boxes/src/events.rs
proof: A terminal exec survives its client; stopping one leaves the box.
depends on:
- blocked by #2404
- blocked by #2389
Proof
A tty or detached exec keeps running after its client goes away. A later attach, output read or wait by its exec id still reaches it. A StopExec request ends that one exec (SIGTERM to its process group, a wait, then SIGKILL) and leaves the box and its other execs running. A box whose stored spec sets [io] exec_enabled = false refuses every command with exit 5 and names the key. A tty exec's PTY is its controlling terminal. exec_started and exec_exited events carry the principal, argv, PTY flag and exit code.
Notes from the agent: 1) PTY and detached survival was already in the tree. This task adds only the test for it. 2) The StopExec RPC and its request and response types live in rpc.rs because the layers do not name crates/minimald-rpc. A client (min) cannot call the RPC until a later task moves the types into minimald-rpc and adds a command. 3) A stopped exec's exec_exited reason is 'exited'. A 'stopped' reason needs a new ExecExitReason variant in exec/bridge.rs, which is outside the layers. The daemon log has 'exec stop requested' and 'exec stopped' lines with the exec id. 4) exec_enabled is read raw from spec.json, in the same way as stored_spec_sets_pty. mfile::spec::BoxSpec has no exec_enabled field and uses deny_unknown_fields. So a box created from a spec with that key fails the BoxSpec parse at launch until mfile gets the field (outside the layers). The e2e test writes spec.json directly for this reason. 5) The refusal text is: 'box does not run commands: its spec sets [io] exec_enabled = false.' Exit 5 applies to every exec form (shell, argv, attached, tty and detached exec). 6) tty_exec_pty_is_controlling_terminal tests nsenter::lead_session_on_terminal, the pre-exec step that the shim runs when its stdin is a tty. No test runs the shim binary end to end. 7) Principal is the SSH login user of the connection (ssh_username). 8) open_terminal in exec/process.rs is now pub(super) so that the test can use it. 9) Beyond the test lines, I r…[truncated]
Tests
cargo nextest run -p minimald pty_or_detached_exec_survives_client_losscargo nextest run -p minimald stop_exec_ends_process_groupcargo nextest run -p minimald exec_disabled_refuses_exit5cargo nextest run -p minimald tty_exec_pty_is_controlling_terminalcargo nextest run -p minimald exec_events_carry_principal_argv_pty_codeVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2406
Residuals
exec_enabledtocrates/mfile/src/spec.rsDocIoand theBoxSpecprojection (defaulttrue, constrained tofalseforbuildandcontainer-build). Until it lands, a spec that sets the key fails thedeny_unknown_fieldsparse at launch.StopExec,StopExecRequestandStopExecResponsefromcrates/minimald/src/rpc.rsintocrates/minimald-rpcbesideStopBox, and add themin box stop <box> --exec <id>client (BCLI-055).Out-of-layer edit accepted by the maintainer (2026-11-04):
crates/minimald/tests/nsenter_integration.rsruns the real nsenter shim for the tty-exec case the maintainer asked for.