Repository navigation
T24 — End a box created for a run when the run ends, from the daemon's side - #1680
Conversation
A box created for a non-detached run ended only when its client destroyed it after the exec returned, so a min task run client killed mid-run stranded its session: the abandoned-launch reap covers only un-finalized sessions and there is no idle stop to catch one (NET-131). The task-run exec request now carries an owns-box flag, encoded as a --owns-box marker in front of the task name; the unflagged payload is byte-for-byte the legacy form, so an older client's request parses exactly as before. After the task-run bridge returns and the exit status is on the wire, the daemon destroys the session through the DestroySession RPC's own path and logs one info line naming the session, the run and the exit code — the line a diagnostic bundle's daemon-log tail reads the end of a run box from. min task run sets the flag and drops its client-side destroy after a run that bridged; the finalize-failure path, the interrupt handler, and --keep (which withholds the flag and keeps the session) are unchanged. min session run keeps sending the unflagged form: its box is an attachable session someone else keeps.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Reviewed against the T24 plan block on #1437. Taking it out of draft: the single commit is authored work, not a budget snapshot; the draft came from three files outside the block's layers, and all three are constructor updates forced by the new TaskRun { owns_box } variant (crates/minimal/src/cmd/session.rs, crates/minimal/src/tests.rs, crates/minvmd/tests/minimald_session_integration.rs), semantic no-ops.
Plan compliance.
- rpc:
TaskRun { task, owns_box }with the--owns-boxpayload marker (crates/minimald-rpc/src/exec.rs:216-267); the unflagged form is byte-identical to the legacy wire. - daemon:
ExecTask::runreturns the exit code andend_run_boxruns after the bridge when the flag is set (crates/minimald/src/exec.rs:341-442); it is theserve_destroy_sessionpath (get_recordthendelete_session) and logsrun box endedwith session, task and exit. - cli: flag set unless
--keep, post-run client destroy removed, the exec-open-failure and interrupt paths keep their client-side destroy (crates/minimal/src/task.rs:54-109,356-440,832). - Killed-client claim holds by construction:
handle_execis a detached spawn,bridge_oneends on write failure, andExecTask::runignores the eof/exit/close errors, so the daemon-side end fires either way. Not exercised by a test; the PR says so.
Test lines. All three present and at exit 0 in the task status; confirmed again in the CI tests job (run 35974482067): run_box_ends_when_its_run_ends, run_box_survives_when_run_keeps_it, task_run_leaves_destroy_to_daemon, plus the wire round-trip. The last of the three proves the flag encoding only; the removal of the client destroy is asserted by the deleted code rather than a test. Acceptable for a unit test.
Non-blocking.
set_global_default(...).unwrap()in the new daemon test (crates/minimald/src/exec.rs:2725) relies on nextest's one-process-per-test; a second global subscriber under plaincargo testwould panic.- Version skew: a new client against an old daemon sends
--owns-box build, the old daemon runs task"--owns-box build", the run fails and neither side destroys. Client and guest payload ship together, so this is the stale-VM case only. Worth a line in the PR body.
Approving; merge waits on the runtime review and CodeRabbit now that it is out of draft.
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none: |
T24 — End a box created for a run when the run ends, from the daemon's side
slice: S5
kind: task
covers: NET-131
size: M
est: 150 code, 180 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: the bundle's daemon log tail carries the end of each run box with its exit code
observability: one info log line when a run box ends, naming the session, the exec and the exit code
layers:
- rpc:
- crates/minimald-rpc/src/exec.rs: the task-run exec request carries a flag saying the run owns its box
test: cargo nextest run -p minimald run_box_ends_when_its_run_ends
- daemon:
- crates/minimald/src/exec.rs: after the task-run bridge returns and the exit status is sent, destroys the session when the flag is set
test: cargo nextest run -p minimald run_box_survives_when_run_keeps_it
- cli:
- crates/minimal/src/task.rs: sets the flag and drops the client-side destroy after a normal run, keeping the finalize-failure and interrupt paths
test: cargo nextest run -p minimal task_run_leaves_destroy_to_daemon
produces: none
consumes: none
proof: Run
min task run, kill the client mid-run, and the session is gone once the task exits; a normal run ends its session the same way with no client-side destroy.Proof
A
min task runsession now ends from the daemon's side of the exec's exit: the task-run request carries an owns-box flag, and once the task's exit status is on the wire the daemon destroys the session through the DestroySession path and logs one info line (session, run, exit code) — so a client killed mid-run leaves the session gone once the task exits, a normal run ends its box with no client-side destroy, and--keep(flag withheld) ormin session run(unflagged form) still keep the box for whoever holds it.Notes from the agent: One commit; the three layers are one protocol change. Touches outside the layers are compile-forced by the TaskRun variant gaining a field: cmd/session.rs sets owns_box=false (semantic no-op — min session run keeps the old behavior), tests.rs and the ignored minvmd integration test update constructions. Wire form is
min://task/run --owns-box <task>; the unflagged payload is byte-for-byte the legacy form so an older client's request parses unchanged, and the marker is read only when a task name follows it, so a task literally named --owns-box still round-trips. After the change, a Ctrl-C mid-run now races the daemon-side destroy: the interrupt handler's destroy wins, and the daemon's own end_run_box logs a warn when it finds the session already gone — the box ends either way. The exec-open-failure path keeps the client-side destroy (no run started, so no owns-box destroy is coming); the finalize-failure and interrupt paths are untouched. The rpc-layer wire tests (47) and the exec e2e module (10) also pass. The VM lanes (just e2e/just test-vm, the ignored minvmd integration) were not run — no VM in this sandbox; the killed-client-mid-run proof rests on the in-process minimald e2e plus the existing lost-exec-client behavior, and the session e2e would confirm it against a real microVM.Tests
cargo nextest run -p minimald run_box_ends_when_its_run_endscargo nextest run -p minimald run_box_survives_when_run_keeps_itcargo nextest run -p minimal task_run_leaves_destroy_to_daemonVerify
min task run agent-verifypassed ([tasks.verify]).Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimal/src/cmd/session.rscrates/minimal/src/tests.rscrates/minvmd/tests/minimald_session_integration.rsCloses #1619