[WIP] feat(minimald): run attach --command inside the session PTask sandbox (DM2) - #575
Closed
norrietaylor wants to merge 1 commit into
Closed
[WIP] feat(minimald): run attach --command inside the session PTask sandbox (DM2)#575norrietaylor wants to merge 1 commit into
norrietaylor wants to merge 1 commit into
Conversation
Previously a plain (non-`min run`, non-git) `attach --command <cmd>`
request was routed to `TokioExec`, which ran `/bin/sh -c <cmd>` as a
plain host process via `tokio::process` — no sandbox and no network
namespace. So `attach --command 'ip addr'` showed the host's
interfaces and had host egress, regardless of the session's
`NetworkMode`.
Replace that branch with a new `SandboxedExec` that builds the session
sandbox exactly like the interactive shell launcher and runs
`/bin/sh -c <argv>` inside it with piped stdio, bridged to the SSH
channel through the existing `bridge()`/`HakoniwaProcess` machinery so
stdin/stdout/stderr/exit-code still round-trip. A fresh sandbox is
built per command (no persistent PTask reuse yet).
The env construction shared with the launcher is factored into a
`build_session_env` helper in `session_host` (same package set, PS1,
`with_network_mode`/`with_username`), so the two paths cannot drift.
For `NetworkMode::OwnIp` the command's freshly-unshared netns is wired
onto the per-host gvproxy switch via the same `attach_own_ip` the
launcher uses (now `pub(crate)`), and the attachment guard is held
alongside `env` until the command exits (mirroring the `(env, own_ip)`
guard ownership and the kill-on-attach-failure handling). The switch is
reached through a new `pub(crate) ManagerHandle::net_switch()` accessor
(backed by a `GetNetSwitch` actor message); the session's network mode
and static ingress come from its `Record` via `get_record`.
`OwnIp` has a startup race the interactive shell avoids: the unshared
netns is empty until the tap is moved in, but a fast one-shot command
can exit in that window and invalidate the PID `move_tap_into_netns`
targets ("Invalid netns value"). For `OwnIp` the user's command is
gated behind a one-byte stdin read that the producer releases only
after the attach completes, keeping the netns-holding PID alive across
the attach. The piped command also drops the launcher's
`set_session_leader()` call, which would fail with `ENOTTY` on a pipe.
The struct's `Exec` impl is gated to production (a real sandbox needs a
package set unavailable in the unit-test tempdir, the same reason
`session_host` swaps in a mock launcher); under `cfg(test)` the
existing end-to-end exec bridge tests keep running through host
`/bin/sh` (`TokioExec`). A new `plain_command_carries_session_network_
mode` unit test covers the record→sandbox wiring for every mode.
Empirically verified against a live native daemon: `--network no-net`
shows only `lo` and curl fails (no egress); `--network own-ip` shows a
`100.64.0.0/16` switch address on the tap and no host eth0. Before this
change both showed the host's `192.168.5.x` eth0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPlain ChangesPlain command sandbox execution
Sequence Diagram(s)sequenceDiagram
participant handle_exec
participant run_plain_command
participant ManagerHandle
participant sessions_manager
participant SandboxedExec
participant build_session_env
participant GvproxySwitch
handle_exec->>run_plain_command: route plain attach --command
run_plain_command->>ManagerHandle: net_switch()
ManagerHandle->>sessions_manager: GetNetSwitch
sessions_manager-->>ManagerHandle: Arc<Mutex<GvproxySwitch>>
run_plain_command->>SandboxedExec: from_record(...)
SandboxedExec->>build_session_env: build_session_env(...)
SandboxedExec->>GvproxySwitch: attach OwnIp netns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
|
This pull request has no accompanying spec. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/exec.rs`:
- Around line 970-1002: The production plain-command path in run_plain_command
is dropping environment variables because _config is ignored before constructing
SandboxedExec. Thread config.env_vars (or the relevant env map) through
SandboxedExec::from_record and apply it when building the child process so exec
behavior matches the cfg(test) TokioExec fallback; if env vars are not meant to
be passed, filter them consistently in both paths. Also update the corresponding
fallback path around the same exec setup so the production and test/legacy
behavior stay aligned.
- Around line 483-613: The error path in the one-shot exec flow is waiting for a
second req_rx.recv() before sending failures, which can deadlock after the
consumer’s first pull; update the outcome/error handling in exec.rs so command
setup, spawn, or attach_own_ip errors are reported immediately after the first
request has been consumed. Use the existing req_rx, proc_tx, and outcome block
in the async exec pipeline to send Err(err) without waiting for another receive,
while preserving the normal success path that sends Ok(process) and then parks
until the consumer drops the stream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01b3aece-9f93-4748-b668-894a19d85d4f
📒 Files selected for processing (3)
crates/minimald/src/exec.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rs
Comment on lines
+483
to
+613
| let outcome: io::Result<()> = async { | ||
| let ctx = session.context().await.map_err(io::Error::other)?; | ||
| let paths = session.paths().await; | ||
|
|
||
| // Build the sandbox env exactly like the interactive launcher. | ||
| let mut env = crate::session_host::build_session_env( | ||
| ctx, | ||
| exec.name, | ||
| exec.username.unwrap_or_else(|| "user".to_string()), | ||
| paths, | ||
| exec.network_mode, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Unlike the interactive shell launcher, this one-shot command runs with | ||
| // piped (non-PTY) stdio, so the container is NOT made a session leader: | ||
| // `set_session_leader` acquires a controlling TTY (`TIOCSCTTY`), which | ||
| // fails with `ENOTTY` on a pipe. The `min run` path (`task_producer`) | ||
| // omits it for the same reason. | ||
| let container = env.container()?; | ||
|
|
||
| // Wait for the consumer to ask before spawning the child, so we never | ||
| // orphan a `hakoniwa::Child` the bridge will never pull. | ||
| if req_rx.recv().await.is_none() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let is_own_ip = matches!(exec.network_mode, sessions::NetworkMode::OwnIp); | ||
|
|
||
| // `OwnIp` has a startup race: the freshly-unshared netns is empty until | ||
| // `attach_own_ip` moves a tap into it, but a fast one-shot command can | ||
| // run and *exit* in that window, invalidating the PID `move_tap_into_ | ||
| // netns` targets ("Invalid netns value"). The interactive shell dodges | ||
| // this because `bash -l` blocks on its PTY; a piped one-shot command | ||
| // does not. So for `OwnIp` we gate the user's command behind a one-byte | ||
| // read from stdin and release it only after the attach completes, | ||
| // keeping the netns-holding PID alive across the attach. `NoNet`/ | ||
| // `HostNet` need no gate — their namespace is ready at spawn. | ||
| let argv = if is_own_ip { | ||
| // `IFS= read -r _` consumes exactly the go-line we write below; the | ||
| // user command then runs with the remaining (bridge-fed) stdin. | ||
| format!("IFS= read -r _gate; {}", exec.argv) | ||
| } else { | ||
| exec.argv.clone() | ||
| }; | ||
|
|
||
| let mut child = { | ||
| let mut cmd = env | ||
| .command(&container, "/bin/sh", ["-c", argv.as_str()]) | ||
| .map_err(|e| io::Error::other(format!("building command failed: {e}")))?; | ||
| cmd.stdin(hakoniwa::Stdio::piped()) | ||
| .stdout(hakoniwa::Stdio::piped()) | ||
| .stderr(hakoniwa::Stdio::piped()); | ||
| cmd.spawn() | ||
| .map_err(|e| io::Error::other(format!("command launch failed: {e}")))? | ||
| }; | ||
| // `command`/`container` no longer borrow `env`. | ||
| drop(container); | ||
|
|
||
| // For an `OwnIp` command, wire its freshly-unshared netns onto the | ||
| // per-host switch, exactly as `SandboxLauncher::launch` does. The | ||
| // attachment guard is held below until the child exits; its `Drop` | ||
| // detaches the PTask and removes any ingress forwards. | ||
| let _own_ip = if is_own_ip { | ||
| match crate::session_host::attach_own_ip( | ||
| &exec.net_switch, | ||
| child.id(), | ||
| exec.ingress.as_ref(), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(attachment) => Some(attachment), | ||
| Err(e) => { | ||
| // The attach failed: this command is aborting. A | ||
| // `hakoniwa::Child` is not terminated on drop, so kill and | ||
| // reap it explicitly (SIGKILL-then-wait) before propagating, | ||
| // mirroring the launcher's failure handling. | ||
| if let Err(kill_err) = child.kill() { | ||
| tracing::warn!( | ||
| error = %kill_err, | ||
| "killing sandbox command after OwnIp attach failure" | ||
| ); | ||
| } | ||
| if let Err(wait_err) = child.wait() { | ||
| tracing::warn!( | ||
| error = %wait_err, | ||
| "reaping sandbox command after OwnIp attach failure" | ||
| ); | ||
| } | ||
| return Err(e); | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| // Release an `OwnIp` command's stdin gate now that the tap is in its | ||
| // namespace: the prologue's `read` consumes this go-line, then the user | ||
| // command runs against the now-wired network. The byte sits at the front | ||
| // of the stdin pipe; the bridge's later writes append after it. Borrow | ||
| // (don't `take`) the stdin handle so the bridge still owns it. | ||
| if is_own_ip && let Some(stdin) = child.stdin.as_mut() { | ||
| use std::io::Write; | ||
| if let Err(e) = stdin.write_all(b"\n").and_then(|()| stdin.flush()) { | ||
| tracing::warn!(error = %e, "failed to release OwnIp command stdin gate"); | ||
| } | ||
| } | ||
|
|
||
| let process = HakoniwaProcess::new(child); | ||
| if let Err(send_err) = proc_tx.send(Ok(process)).await { | ||
| // Receiver dropped between our `recv` and our `send`; kill the child | ||
| // we just spawned so it doesn't outlive its sandbox rootfs. | ||
| if let Ok(mut proc) = send_err.0 { | ||
| let _ = proc.start_kill(); | ||
| } | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Park until the consumer drops the stream, keeping `env` and the | ||
| // `OwnIp` attachment alive while the bridge drives the child. | ||
| let _ = req_rx.recv().await; | ||
| drop(_own_ip); | ||
| Ok(()) | ||
| } | ||
| .await; | ||
|
|
||
| if let Err(err) = outcome | ||
| && req_rx.recv().await.is_some() | ||
| { | ||
| let _ = proc_tx.send(Err(err)).await; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report producer errors after the first pull instead of waiting for a second one.
Line 506 consumes the consumer’s initial request. If command setup, spawn, or attach_own_ip fails afterward, Lines 609-613 wait for another req_rx.recv() while the consumer is blocked on proc_rx.recv(), so the SSH exec can hang instead of receiving the error.
🐛 Suggested fix
async fn sandboxed_producer(
exec: SandboxedExec,
session: SessionHandle,
mut req_rx: mpsc::Receiver<()>,
proc_tx: mpsc::Sender<io::Result<HakoniwaProcess>>,
) {
+ let mut requested = false;
let outcome: io::Result<()> = async {
+ // Wait for the consumer to ask before doing fallible setup/spawning.
+ if req_rx.recv().await.is_none() {
+ return Ok(());
+ }
+ requested = true;
+
let ctx = session.context().await.map_err(io::Error::other)?;
let paths = session.paths().await;
// Build the sandbox env exactly like the interactive launcher.
let mut env = crate::session_host::build_session_env(
@@
- // Wait for the consumer to ask before spawning the child, so we never
- // orphan a `hakoniwa::Child` the bridge will never pull.
- if req_rx.recv().await.is_none() {
- return Ok(());
- }
-
let is_own_ip = matches!(exec.network_mode, sessions::NetworkMode::OwnIp);
@@
- if let Err(err) = outcome
- && req_rx.recv().await.is_some()
- {
- let _ = proc_tx.send(Err(err)).await;
+ if let Err(err) = outcome {
+ if requested {
+ let _ = proc_tx.send(Err(err)).await;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let outcome: io::Result<()> = async { | |
| let ctx = session.context().await.map_err(io::Error::other)?; | |
| let paths = session.paths().await; | |
| // Build the sandbox env exactly like the interactive launcher. | |
| let mut env = crate::session_host::build_session_env( | |
| ctx, | |
| exec.name, | |
| exec.username.unwrap_or_else(|| "user".to_string()), | |
| paths, | |
| exec.network_mode, | |
| ) | |
| .await?; | |
| // Unlike the interactive shell launcher, this one-shot command runs with | |
| // piped (non-PTY) stdio, so the container is NOT made a session leader: | |
| // `set_session_leader` acquires a controlling TTY (`TIOCSCTTY`), which | |
| // fails with `ENOTTY` on a pipe. The `min run` path (`task_producer`) | |
| // omits it for the same reason. | |
| let container = env.container()?; | |
| // Wait for the consumer to ask before spawning the child, so we never | |
| // orphan a `hakoniwa::Child` the bridge will never pull. | |
| if req_rx.recv().await.is_none() { | |
| return Ok(()); | |
| } | |
| let is_own_ip = matches!(exec.network_mode, sessions::NetworkMode::OwnIp); | |
| // `OwnIp` has a startup race: the freshly-unshared netns is empty until | |
| // `attach_own_ip` moves a tap into it, but a fast one-shot command can | |
| // run and *exit* in that window, invalidating the PID `move_tap_into_ | |
| // netns` targets ("Invalid netns value"). The interactive shell dodges | |
| // this because `bash -l` blocks on its PTY; a piped one-shot command | |
| // does not. So for `OwnIp` we gate the user's command behind a one-byte | |
| // read from stdin and release it only after the attach completes, | |
| // keeping the netns-holding PID alive across the attach. `NoNet`/ | |
| // `HostNet` need no gate — their namespace is ready at spawn. | |
| let argv = if is_own_ip { | |
| // `IFS= read -r _` consumes exactly the go-line we write below; the | |
| // user command then runs with the remaining (bridge-fed) stdin. | |
| format!("IFS= read -r _gate; {}", exec.argv) | |
| } else { | |
| exec.argv.clone() | |
| }; | |
| let mut child = { | |
| let mut cmd = env | |
| .command(&container, "/bin/sh", ["-c", argv.as_str()]) | |
| .map_err(|e| io::Error::other(format!("building command failed: {e}")))?; | |
| cmd.stdin(hakoniwa::Stdio::piped()) | |
| .stdout(hakoniwa::Stdio::piped()) | |
| .stderr(hakoniwa::Stdio::piped()); | |
| cmd.spawn() | |
| .map_err(|e| io::Error::other(format!("command launch failed: {e}")))? | |
| }; | |
| // `command`/`container` no longer borrow `env`. | |
| drop(container); | |
| // For an `OwnIp` command, wire its freshly-unshared netns onto the | |
| // per-host switch, exactly as `SandboxLauncher::launch` does. The | |
| // attachment guard is held below until the child exits; its `Drop` | |
| // detaches the PTask and removes any ingress forwards. | |
| let _own_ip = if is_own_ip { | |
| match crate::session_host::attach_own_ip( | |
| &exec.net_switch, | |
| child.id(), | |
| exec.ingress.as_ref(), | |
| ) | |
| .await | |
| { | |
| Ok(attachment) => Some(attachment), | |
| Err(e) => { | |
| // The attach failed: this command is aborting. A | |
| // `hakoniwa::Child` is not terminated on drop, so kill and | |
| // reap it explicitly (SIGKILL-then-wait) before propagating, | |
| // mirroring the launcher's failure handling. | |
| if let Err(kill_err) = child.kill() { | |
| tracing::warn!( | |
| error = %kill_err, | |
| "killing sandbox command after OwnIp attach failure" | |
| ); | |
| } | |
| if let Err(wait_err) = child.wait() { | |
| tracing::warn!( | |
| error = %wait_err, | |
| "reaping sandbox command after OwnIp attach failure" | |
| ); | |
| } | |
| return Err(e); | |
| } | |
| } | |
| } else { | |
| None | |
| }; | |
| // Release an `OwnIp` command's stdin gate now that the tap is in its | |
| // namespace: the prologue's `read` consumes this go-line, then the user | |
| // command runs against the now-wired network. The byte sits at the front | |
| // of the stdin pipe; the bridge's later writes append after it. Borrow | |
| // (don't `take`) the stdin handle so the bridge still owns it. | |
| if is_own_ip && let Some(stdin) = child.stdin.as_mut() { | |
| use std::io::Write; | |
| if let Err(e) = stdin.write_all(b"\n").and_then(|()| stdin.flush()) { | |
| tracing::warn!(error = %e, "failed to release OwnIp command stdin gate"); | |
| } | |
| } | |
| let process = HakoniwaProcess::new(child); | |
| if let Err(send_err) = proc_tx.send(Ok(process)).await { | |
| // Receiver dropped between our `recv` and our `send`; kill the child | |
| // we just spawned so it doesn't outlive its sandbox rootfs. | |
| if let Ok(mut proc) = send_err.0 { | |
| let _ = proc.start_kill(); | |
| } | |
| return Ok(()); | |
| } | |
| // Park until the consumer drops the stream, keeping `env` and the | |
| // `OwnIp` attachment alive while the bridge drives the child. | |
| let _ = req_rx.recv().await; | |
| drop(_own_ip); | |
| Ok(()) | |
| } | |
| .await; | |
| if let Err(err) = outcome | |
| && req_rx.recv().await.is_some() | |
| { | |
| let _ = proc_tx.send(Err(err)).await; | |
| } | |
| let mut requested = false; | |
| let outcome: io::Result<()> = async { | |
| // Wait for the consumer to ask before doing fallible setup/spawning. | |
| if req_rx.recv().await.is_none() { | |
| return Ok(()); | |
| } | |
| requested = true; | |
| let ctx = session.context().await.map_err(io::Error::other)?; | |
| let paths = session.paths().await; | |
| // Build the sandbox env exactly like the interactive launcher. | |
| let mut env = crate::session_host::build_session_env( | |
| ctx, | |
| exec.name, | |
| exec.username.unwrap_or_else(|| "user".to_string()), | |
| paths, | |
| exec.network_mode, | |
| ) | |
| .await?; | |
| // Unlike the interactive shell launcher, this one-shot command runs with | |
| // piped (non-PTY) stdio, so the container is NOT made a session leader: | |
| // `set_session_leader` acquires a controlling TTY (`TIOCSCTTY`), which | |
| // fails with `ENOTTY` on a pipe. The `min run` path (`task_producer`) | |
| // omits it for the same reason. | |
| let container = env.container()?; | |
| let is_own_ip = matches!(exec.network_mode, sessions::NetworkMode::OwnIp); | |
| // `OwnIp` has a startup race: the freshly-unshared netns is empty until | |
| // `attach_own_ip` moves a tap into it, but a fast one-shot command can | |
| // run and *exit* in that window, invalidating the PID `move_tap_into_ | |
| // netns` targets ("Invalid netns value"). The interactive shell dodges | |
| // this because `bash -l` blocks on its PTY; a piped one-shot command | |
| // does not. So for `OwnIp` we gate the user's command behind a one-byte | |
| // read from stdin and release it only after the attach completes, | |
| // keeping the netns-holding PID alive across the attach. `NoNet`/ | |
| // `HostNet` need no gate — their namespace is ready at spawn. | |
| let argv = if is_own_ip { | |
| // `IFS= read -r _` consumes exactly the go-line we write below; the | |
| // user command then runs with the remaining (bridge-fed) stdin. | |
| format!("IFS= read -r _gate; {}", exec.argv) | |
| } else { | |
| exec.argv.clone() | |
| }; | |
| let mut child = { | |
| let mut cmd = env | |
| .command(&container, "/bin/sh", ["-c", argv.as_str()]) | |
| .map_err(|e| io::Error::other(format!("building command failed: {e}")))?; | |
| cmd.stdin(hakoniwa::Stdio::piped()) | |
| .stdout(hakoniwa::Stdio::piped()) | |
| .stderr(hakoniwa::Stdio::piped()); | |
| cmd.spawn() | |
| .map_err(|e| io::Error::other(format!("command launch failed: {e}")))? | |
| }; | |
| // `command`/`container` no longer borrow `env`. | |
| drop(container); | |
| // For an `OwnIp` command, wire its freshly-unshared netns onto the | |
| // per-host switch, exactly as `SandboxLauncher::launch` does. The | |
| // attachment guard is held below until the child exits; its `Drop` | |
| // detaches the PTask and removes any ingress forwards. | |
| let _own_ip = if is_own_ip { | |
| match crate::session_host::attach_own_ip( | |
| &exec.net_switch, | |
| child.id(), | |
| exec.ingress.as_ref(), | |
| ) | |
| .await | |
| { | |
| Ok(attachment) => Some(attachment), | |
| Err(e) => { | |
| // The attach failed: this command is aborting. A | |
| // `hakoniwa::Child` is not terminated on drop, so kill and | |
| // reap it explicitly (SIGKILL-then-wait) before propagating, | |
| // mirroring the launcher's failure handling. | |
| if let Err(kill_err) = child.kill() { | |
| tracing::warn!( | |
| error = %kill_err, | |
| "killing sandbox command after OwnIp attach failure" | |
| ); | |
| } | |
| if let Err(wait_err) = child.wait() { | |
| tracing::warn!( | |
| error = %wait_err, | |
| "reaping sandbox command after OwnIp attach failure" | |
| ); | |
| } | |
| return Err(e); | |
| } | |
| } | |
| } else { | |
| None | |
| }; | |
| // Release an `OwnIp` command's stdin gate now that the tap is in its | |
| // namespace: the prologue's `read` consumes this go-line, then the user | |
| // command runs against the now-wired network. The byte sits at the front | |
| // of the stdin pipe; the bridge's later writes append after it. Borrow | |
| // (don't `take`) the stdin handle so the bridge still owns it. | |
| if is_own_ip && let Some(stdin) = child.stdin.as_mut() { | |
| use std::io::Write; | |
| if let Err(e) = stdin.write_all(b"\n").and_then(|()| stdin.flush()) { | |
| tracing::warn!(error = %e, "failed to release OwnIp command stdin gate"); | |
| } | |
| } | |
| let process = HakoniwaProcess::new(child); | |
| if let Err(send_err) = proc_tx.send(Ok(process)).await { | |
| // Receiver dropped between our `recv` and our `send`; kill the child | |
| // we just spawned so it doesn't outlive its sandbox rootfs. | |
| if let Ok(mut proc) = send_err.0 { | |
| let _ = proc.start_kill(); | |
| } | |
| return Ok(()); | |
| } | |
| // Park until the consumer drops the stream, keeping `env` and the | |
| // `OwnIp` attachment alive while the bridge drives the child. | |
| let _ = req_rx.recv().await; | |
| drop(_own_ip); | |
| Ok(()) | |
| } | |
| .await; | |
| if let Err(err) = outcome { | |
| if requested { | |
| let _ = proc_tx.send(Err(err)).await; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimald/src/exec.rs` around lines 483 - 613, The error path in the
one-shot exec flow is waiting for a second req_rx.recv() before sending
failures, which can deadlock after the consumer’s first pull; update the
outcome/error handling in exec.rs so command setup, spawn, or attach_own_ip
errors are reported immediately after the first request has been consumed. Use
the existing req_rx, proc_tx, and outcome block in the async exec pipeline to
send Err(err) without waiting for another receive, while preserving the normal
success path that sends Ok(process) and then parks until the consumer drops the
stream.
Comment on lines
+970
to
+1002
| async fn run_plain_command( | ||
| serv: ServerStateHandle, | ||
| conn: ConnectionHandle, | ||
| session_handle: SessionHandle, | ||
| session_id: SessionId, | ||
| id: ChannelId, | ||
| argv: String, | ||
| _config: ChannelConfig, | ||
| channel: Channel<Msg>, | ||
| ) { | ||
| let mngr = serv.sessions_manager().await; | ||
| // Read the session's network mode + static ingress from its record so the | ||
| // sandbox honours the same policy an interactive attach would; the switch | ||
| // is the one per-host switch the launcher attaches to. | ||
| let record = match mngr.get_record(SessionKeyPredicate::Id(session_id)).await { | ||
| Ok(Some(r)) => r, | ||
| Ok(None) => { | ||
| tracing::warn!(%id, "exec: session record vanished before launch"); | ||
| return; | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(%id, error = %e, "exec: failed to read session record"); | ||
| return; | ||
| } | ||
| }; | ||
| let net_switch = mngr.net_switch().await; | ||
| let exec_task = ExecTask { | ||
| conn, | ||
| serv, | ||
| session: session_handle, | ||
| channel_id: id, | ||
| exec: SandboxedExec::from_record(argv, &record, net_switch), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve SSH exec environment variables in the production sandbox path.
The production helper ignores _config, so config.env_vars never reaches SandboxedExec; the cfg(test) fallback still passes those vars to TokioExec. That makes env-dependent plain commands work in tests/old host execution but not in the new production sandbox path.
Thread the env map into SandboxedExec and apply it when building the child command, or intentionally filter it in both paths so production and tests match.
Also applies to: 1006-1028
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimald/src/exec.rs` around lines 970 - 1002, The production
plain-command path in run_plain_command is dropping environment variables
because _config is ignored before constructing SandboxedExec. Thread
config.env_vars (or the relevant env map) through SandboxedExec::from_record and
apply it when building the child process so exec behavior matches the cfg(test)
TokioExec fallback; if env vars are not meant to be passed, filter them
consistently in both paths. Also update the corresponding fallback path around
the same exec setup so the production and test/legacy behavior stay aligned.
| // reads (argv/name/username/ingress/net_switch) look unused; the wiring is | ||
| // still exercised by `plain_command_carries_session_network_mode`. | ||
| #[cfg_attr(test, allow(dead_code))] | ||
| pub(crate) struct SandboxedExec { |
Member
There was a problem hiding this comment.
The exec impl type for running a task already exists, see TaskExec
Member
Author
|
Closing as will not do. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
minimal2 attach <session> --command <cmd>ran<cmd>as a plain host process (TokioExec→/bin/sh -con the host), ignoring the session entirely — so it never entered the PTask sandbox or its network namespace. This routes it through a real sandbox2/hakoniwa container built with the session'sNetworkMode, mirroring the interactive-shell launch path.Scope: DM2 (native Linux) only
This is the native minimald exec-into-PTask path (DM2), where minimald spawns gvproxy locally and the OwnIp PTask attaches via fd-pass. The VM-boundary deployments (DM1 macOS, DM3/DM4 Linux+minvmd) need the host-gvproxy + per-PTask vsock shuttle from #572 — out of scope here. DM2 is "correct as-is" per #572.
Changes
exec.rs— newSandboxedExec(replacesTokioExecfor the plain-command branch undercfg(not(test))); builds anEnvfrom the session record with.with_network_mode(...), runs/bin/sh -c <cmd>in the container, bridges stdio through the existingbridge()/Processmachinery. ForNetworkMode::OwnIpit wires the namespace to the per-host switch viaattach_own_ipand holds the(env, own_ip)guard for the command's lifetime.min runand git-receive paths unchanged.session_host.rs— factored the launcher's env construction into a sharedpub(crate) build_session_env(exec and the interactive shell now build identical sandboxes); madeattach_own_ippub(crate)so exec reuses the exact tap/relay/ingress logic (no duplication).sessions.rs—pub(crate) ManagerHandle::net_switch()accessor so the exec path reaches the one per-hostGvproxySwitch.Two incidental fixes found while verifying: a piped one-shot command must not
set_session_leader()(ENOTTY on a pipe), and the OwnIp startup race (command exits before the tap is in the netns) is gated behind a one-byte stdin handshake released after the attach completes.Coordination with #572 — shared seam
This and #572 both touch the OwnIp attach path. Overlap is in
session_host.rsaroundattach_own_ip(here: madepub(crate)+ factoredbuild_session_env; #572: adds the DM1/3/4 tap→shuttle branch). Otherwise orthogonal (DM2 local-spawn+fd-pass vs DM1/3/4 host-gvproxy+vsock-shuttle). Whoever lands second rebases theattach_own_ipseam.Dependencies for end-to-end CLI testing
The exec code is independent and compiles/tests on
main, but driving it end-to-end fromminimal2needs:--network/--ingress) to create own-ip/ingress sessions from the CLI, andminimal.toml not found.Verification
cargo build/test/clippy/fmt -p minimald— clean (incl. the existingexec.rse2e tests + a newplain_command_carries_session_network_mode).attach --command→ onlylo, noeth0, curl fails; own-ip → switch tapmtap0_2+100.64.x.xIP (daemon log: "attached OwnIp PTask to gvproxy switch ip=100.64.0.3"). Before: both showed the host'seth0/192.168.5.x. (Worktree hand-seeded for the test pending [WIP] feat(minimald,minvmd): seeded cache disk + workspace upload for offline session compose #573.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
attach --commandrequests now run in the target session’s sandboxed environment.Bug Fixes