daemon: gracefully shutdown if wrapped command exits

[patricklyc]

Before this change, when the wrapped command exits, a detached task calls process::exit directly, skipping the daemon's structured shutdown, leaving the host mis-routed.

This commit ensures that the daemon gracefully shuts down if the wrapped command errors, exits or is killed by a signal.

Add three e2e tests to make sure the daemon exits with the correct status codes and cleans up nftable rules in all three scenarios.

[ejj-agent]

Found two correctness regressions in the shutdown-path change:

1. command_handle is spawned even when on_ready is None; that task completes immediately, the new tokio::select! arm wins, and a plain intermesh daemon exits as soon as it starts.

2. The wrapped command's exit status is now only logged. After graceful teardown, run() returns Ok(()), so intermesh proxy -- <cmd> exits 0 even when <cmd> failed, contradicting the CLI contract and the previous behavior.

A safe shape is to keep the on-ready handle optional, select on it only when present, store the child status, perform teardown/final save, then exit/return with that status.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 5fbda6a

Thanks for the iteration — the on_ready=None and exit-code-propagation regressions flagged in the prior review are addressed. One new correctness regression to consider, plus a couple of smaller items around the same select! arm. Diff is otherwise tightly scoped to the intended behavior.

The main concern: because the new command_handler is an async || whose body has no .await, the future it returns resolves synchronously to Some(JoinHandle) on first poll whenever on_ready is Some. That makes the select! immediately enter the command arm and then await the spawned task outside the select!, so SIGTERM/SIGINT and unexpected-task-exit branches stop being watched while the wrapped command runs. Hoisting the tokio::spawn out of the closure and selecting on the JoinHandle itself (or an OptionFuture over Option<JoinHandle>) addresses this and removes the closure indirection.

No tests accompany this PR. Given how subtle the select! + JoinHandle + exit-code interaction is, a small integration test (wrapped command exits with a known nonzero code → daemon exits with that code; SIGTERM to daemon while wrapped command is running → graceful teardown) would catch exactly this class of regression.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 935156a

The OptionFuture refactor cleanly resolves the prior regressions: the on_ready = None case no longer enters the command arm immediately, the command-exit path no longer ?s out of run() before teardown, and signal-killed children now surface as a nonzero status. Diff stays scoped to the intended behavior.

Two smaller items in the new select! arm body — see inline. No new tests; an integration test that exercises wrapped-command-exits-nonzero and SIGTERM-during-wrapped-command would still catch this whole class of regression.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 09825b0

Thanks for continuing to tighten this path. The current shape addresses the earlier correctness regressions around on_ready = None, preserving teardown before propagating the wrapped command status, and signal-killed children surfacing as nonzero. I found one remaining cleanup in the new command-exit arm: the Err(_) branch drops the JoinError and logs "command exited with code 1", but a join error means the task panicked or was cancelled — the child may not have exited with any code at all, and 1 is just the synthetic status we chose. Bind the error and log it explicitly (for example Err(e) => { task_status = Some(1); error!(?e, "command task failed"); }) so this failure mode is distinguishable from a real process exit status.

A focused integration test for intermesh daemon -- <cmd> where <cmd> exits nonzero would still be useful, since that would have caught several of the status/teardown regressions in this PR series. Otherwise the diff is narrowly scoped to the intended behavior.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 87cacf2

Thanks for the iteration — the daemon-side correctness regressions called out across earlier reviews (on_ready=None, ?-out before teardown, the r.expect panic, signal-killed children silently mapping to exit 0, error-level logging of normal exits, dropped JoinError) all look resolved here. The remaining issues I see are in the new e2e coverage: it exercises teardown, but not the exit-code contract this PR is adding, and one of the scenarios doesn't actually create the intended long-running child to kill.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 9adcacd

The daemon-side shutdown/status handling looks resolved at this head, and the added tests now cover the clean, nonzero, and signal-killed wrapped-command paths. I found one test bug in the new exit-code assertions: see inline.

I ran cargo test --no-run, which completed successfully. I could not run the Docker e2e tests in this environment because the Docker daemon socket is unavailable.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 32fba12

The daemon-side shutdown/exit-code path looks resolved at this head. I found two remaining test-side issues in the new e2e coverage: the post-restart nftables check no longer verifies cleanup from the original daemon run, and the new docker wait helper should preserve Docker command failures instead of turning them into parse errors.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 6b8b3d1

The daemon-side shutdown/status path still builds successfully, and the prior docker wait parsing/error-handling issues look fixed. I found two remaining issues in the new e2e cleanup coverage where the tests do not actually exercise the intended failure/signal scenarios. I ran cargo test --no-run successfully; I could not run the Docker e2e tests because the Docker daemon socket is unavailable in this environment.

[ejj-agent]

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

AI-PR-Review: #161 e51012b

I reviewed the current head after the latest fixes and did not find any new substantive concerns worth posting. The current daemon path now waits on the wrapped command inside the main select!, records nonzero / signal / spawn failures as failures, falls through through the common graceful teardown path, and exits with the captured wrapped-command status after shutdown completes.

Checks run locally on the PR head:

• cargo check --tests
• cargo test (unit tests passed; docker/incus e2e/vm tests are ignored by default)

[ejj]

One basic thing could you squash all these commits ans write a good commit message? See the git log for examples

[patricklyc]

I'm having trouble with squashing because of interleaving commits from NetSys:main. I'll try again tomorrow. Alternatively, could we use github's squashing feature? Here's how I configured it on my fork.

I'll work on the commit message.

[ejj]

No it creates bad commit history. I know a lot of modern projects don't care so much about that but for serious security software I think its worth keeping these things clean.

you could also just make a new branch from main and copy your code over if you want ... might be easier at this point.