daemon: gracefully shutdown if proxy wrapped command exits - #166
daemon: gracefully shutdown if proxy wrapped command exits#166patricklyc wants to merge 1 commit into
Conversation
ejj-agent
left a comment
ejj-agent
left a comment
There was a problem hiding this comment.
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.
I reviewed this for correctness, simplicity, diff hygiene, and test coverage. I did not find any substantive concerns worth posting. The daemon now routes wrapped-command completion through the existing shutdown path, and the added e2e coverage exercises zero exit, nonzero exit, signal termination, and nftables cleanup behavior.
Checks run locally: cargo test --no-run, cargo test --lib, and cargo fmt --check.
|
Comment directed by Ethan. Small thing: the commit message would be a little clearer if it mentioned that the wrapping is via |
| fn test_proxy_command_exit() { | ||
| let harness = Harness::new(); | ||
|
|
||
| let proxy_cmd = r"intermesh proxy -- sleep 1"; |
There was a problem hiding this comment.
Comment directed by Ethan.
I don’t think this first container-main sleep 1 check adds much value over the cleanup path below. Could we drop this block and just keep the lower test path for a successful wrapped-command exit?
There was a problem hiding this comment.
The agent advised me to test the exit code contract. If that is unnecessary I can remove it.
There was a problem hiding this comment.
Yeah I think chatgpt in particular is a little bit too aggressive about tests I think you can ditch it
| }); | ||
| } | ||
| let mut task_status: Option<i32> = None; | ||
| let mut command_handle = |
There was a problem hiding this comment.
Comment directed by Ethan.
I think this code is correct, but it isn't "obviously correct": the OptionFuture bit makes me work pretty hard to convince myself the no-command case is handled and that this branch only fires when a wrapped command exists.
Can we implement the same logic in a simpler way? For example, maybe keep the optional command task explicit and move the waiting/status decoding behind a small helper:
let mut command_handle = on_ready.map(|mut cmd| {
tokio::spawn(async move { cmd.status().await })
});
...
r = wait_for_wrapped_command(&mut command_handle) => {
task_status = Some(command_status_code(r));
}where wait_for_wrapped_command can make the None case explicit (e.g. it never completes when there is no wrapped command), and command_status_code handles the nested JoinHandle/io::Result cases. The exact shape isn't important; I just think the optional-command control flow should be easier to reason about here.
There was a problem hiding this comment.
I have tried wrapping command_handle in async closures and helper functions, but I couldn't find anyway to fit it in a select! arm without OptionFuture. Would it help if I broke it out into two lines?
There was a problem hiding this comment.
Yes I think breaking it into two lines and if it still isn't obviously correct adding a comment explaining it will help
patricklyc
changed the title
patricklyc
changed the title
patricklyc
changed the title
06990e6 to
9fe7fd6
Compare
Before this change, when the command wrapped by intermesh proxy exits, a detached task calls process::exit directly, skipping the daemon's structured shutdown, leaving the host misrouted. 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.
3 participants
Before this change, when the wrapped command in
intermesh proxy -- <cmd>exits, a detached task calls process::exit directly, skipping the daemon's structured shutdown, leaving the host misrouted.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.