[WIP] feat(minimald,minvmd): seeded cache disk + workspace upload for offline session compose - #573
[WIP] feat(minimald,minvmd): seeded cache disk + workspace upload for offline session compose#573norrietaylor wants to merge 5 commits into
Conversation
`minimal activate` hardcoded NetworkMode::default() (HostNet) with empty policy, so NoNet/OwnIp and ingress port mappings had no CLI surface and were reachable only via the netns proofs constructing a Record directly. - `--network <no-net|host-net|own-ip>` (default host-net; no-flag behavior unchanged) via a local CliNetworkMode ValueEnum, keeping the sessions crate free of a clap dependency. - `--ingress EXT:INT[/PROTO]` (repeatable; PROTO defaults tcp) parsed into sessions::PortMapping; non-tcp/udp and malformed specs rejected at parse time. - Surface the daemon's typed CreateSession validation error (e.g. ingress on a non-own-ip session) instead of a generic failure line. Daemon-side validate_policy is unchanged and remains the enforcement point. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
Copy-on-activate: `minimal2 activate` now streams the project into the session worktree so the interactive shell (mctx compose) finds minimal.toml and the project files (conformance R§3.1.4). Without this the worktree is empty and interactive attach dies at "minimal.toml not found". - `Client::upload_workspace` builds an ignore-aware `tar.zst` of the project (honours .gitignore/.ignore/.minimalignore via the `ignore` crate, keeps dotfiles, always skips .git, preserves modes) and streams it over the existing `WorkspaceFilesTarZst` subsystem; the daemon-side receiver already unpacked it into the worktree. - Move the shared subsystem name + `MINIMAL_SESSION_ID` env-var const into the `minimald-rpc` wire-contract crate so client and server can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The macOS guest VM has no network and an empty cache, so composing a session sandbox failed with "no such package: base". Attach a pre-seeded cache disk so the guest resolves the package closure offline. - minvmd: optional `MINVMD_CACHE_PATH` attaches an ext4 image as a second block device (/dev/vdb). - minimald (guest pid-1): mount it at /run/minimal/cache, and set the state dir equal to the cache dir so the compose's package hardlinks stay on one filesystem (hardlinks can't cross fs; tmpfs state would EXDEV). Reset stale runtime state on boot, keeping the seeded cache dirs. - session context uses `.with_offline(true)` so a VCS/source cache miss is a clean error rather than a doomed git fetch on the networkless guest. - Mount devpts at /dev/pts (openpty for the interactive PTY) and a tmpfs at /tmp (hakoniwa stages its mount namespace there; the rootfs is read-only) — both surfaced once compose started working. - scripts/build-session-cache.sh builds the seeded ext4 (closure fetched from the remote cache, packed via loop-mount). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…slow boot reset The seeded cache disk is writable and co-locates the daemon state dir, so an unclean VM shutdown can corrupt it. Two boot-robustness fixes: - `Config::host_key`: when we own the key (`create_if_missing`), treat an unreadable/corrupt key file like a missing one — regenerate and overwrite, rather than returning an error. A truncated/garbage key (e.g. from an unclean shutdown) previously bricked startup with a PEM parse error. - `guest::mount_cache`: reset only `providers/` (the host key) on boot instead of `rm -rf`-ing every non-seed entry. The old reset was O(accumulated session state) and walked corrupt directory blocks, which could blow minimald's 5 s READY-marker timeout. Per-session `sandboxes`/`tasks` trees are left in place (stale entries are harmless; GC is separate); seed dirs are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds SSH workspace upload from ChangesSession and cache flow
Sequence Diagram(s)sequenceDiagram
participant cmd_activate
participant upload_workspace
participant russh_channel
participant STREAM_WORKSPACE_FILES
cmd_activate->>upload_workspace: created session id and project_dir
upload_workspace->>russh_channel: set MINIMAL_SESSION_ID_ENV
upload_workspace->>russh_channel: request STREAM_WORKSPACE_FILES
upload_workspace->>russh_channel: stream tar.zst bytes
russh_channel->>STREAM_WORKSPACE_FILES: deliver workspace payload
STREAM_WORKSPACE_FILES-->>upload_workspace: stderr data or close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
This pull request has no accompanying spec. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/build-session-cache.sh (1)
85-88: 🚀 Performance & Scalability | 🔵 TrivialKeep the prune/copy step inside Linux
cp -aon BSD/macOS does not preserve hard links, so this host-side staging can expand the cache before the container packs it. If these cache trees rely on deduped links, move the prune/copy step into the Linux container (or use a macOS copy tool that preserves hard links) so the final image size stays faithful.🤖 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 `@scripts/build-session-cache.sh` around lines 85 - 88, The staging copy step in the cache build flow currently runs on the host, where `cp -a` may not preserve hard links on BSD/macOS and can inflate the cache before packing. Move the prune/copy logic in `build-session-cache.sh` into the Linux container path used for staging, or switch to a macOS-compatible copy approach that preserves hard links, and keep the existing `mktemp`/`cp` flow associated with the cache trees (`built`, `vcs`, `lc`, `stdlib`) inside the containerized packaging step.
🤖 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/minimal2/src/client.rs`:
- Line 142: The workspace upload path in client.rs is buffering too much data in
memory because build_workspace_tar_zst fully materializes the archive before
upload_workspace can send it. Refactor the upload flow in the workspace
packaging and upload path (including build_workspace_tar_zst and
upload_workspace) to stream tar output through zstd directly into the SSH
channel, or spool to a temp file/chunked stream, so large workspaces no longer
require holding both uncompressed and compressed payloads in memory.
- Around line 341-343: The shipping test in client.rs only asserts that
.gitignore is included, so it can miss regressions where .minimalignore is
excluded. Update the assertion in the relevant test near the paths.contains
check to also verify .minimalignore is present, keeping the existing .gitignore
check so both project ignore files are covered.
In `@crates/minimal2/src/main.rs`:
- Around line 497-506: Move the session-id stdout print in main so it only
happens after client.upload_workspace(...).await succeeds; right now created.id
is printed before the upload result is known, which can mislead callers on
failure. Keep the existing error path in the upload_workspace handling block,
and emit println!("{}", created.id) only after the Err(()) branch is avoided.
In `@crates/minimald/src/server.rs`:
- Around line 86-91: The host-key reload logic in the key-loading match is too
broad because Err(e) if *create_if_missing currently regenerates on every read
failure, including transient I/O errors. Update the match in the host key
loading path to only regenerate for explicit missing/parse-corruption cases from
the read/parse step, and let other errors return instead of rotating the key.
Keep the regeneration behavior tied to the existing PrivateKey::random and
write_openssh_file flow, but narrow the error handling around the host-key read
branch so persistent identity is preserved.
---
Nitpick comments:
In `@scripts/build-session-cache.sh`:
- Around line 85-88: The staging copy step in the cache build flow currently
runs on the host, where `cp -a` may not preserve hard links on BSD/macOS and can
inflate the cache before packing. Move the prune/copy logic in
`build-session-cache.sh` into the Linux container path used for staging, or
switch to a macOS-compatible copy approach that preserves hard links, and keep
the existing `mktemp`/`cp` flow associated with the cache trees (`built`, `vcs`,
`lc`, `stdlib`) inside the containerized packaging step.
🪄 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: 6a35ffc1-6d87-4beb-95bc-7788bd9ae72e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/minimal2/Cargo.tomlcrates/minimal2/src/client.rscrates/minimal2/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/guest.rscrates/minimald/src/lib.rscrates/minimald/src/main.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/image.rscrates/minvmd/src/vm.rsscripts/build-session-cache.sh
| session_id: &str, | ||
| project_dir: &Path, | ||
| ) -> Result<(), String> { | ||
| let payload = build_workspace_tar_zst(project_dir).await?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Avoid buffering the whole workspace twice before upload.
build_workspace_tar_zst materializes the uncompressed tar and compressed zstd payload in memory, then upload_workspace sends it only after the full archive exists. Large workspaces can OOM or make activation fail before any streaming begins; consider streaming tar → zstd → SSH channel, or spooling/chunking through a temp file.
Also applies to: 212-245
🤖 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/minimal2/src/client.rs` at line 142, The workspace upload path in
client.rs is buffering too much data in memory because build_workspace_tar_zst
fully materializes the archive before upload_workspace can send it. Refactor the
upload flow in the workspace packaging and upload path (including
build_workspace_tar_zst and upload_workspace) to stream tar output through zstd
directly into the SSH channel, or spool to a temp file/chunked stream, so large
workspaces no longer require holding both uncompressed and compressed payloads
in memory.
| // .gitignore and .minimalignore themselves ship (they're project files). | ||
| assert!(paths.contains(".gitignore"), "got {paths:?}"); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert .minimalignore is shipped too.
The comment says both ignore files are included, but the test only checks .gitignore, so excluding .minimalignore would not fail this test.
Proposed test fix
// .gitignore and .minimalignore themselves ship (they're project files).
assert!(paths.contains(".gitignore"), "got {paths:?}");
+ assert!(paths.contains(".minimalignore"), "got {paths:?}");📝 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.
| // .gitignore and .minimalignore themselves ship (they're project files). | |
| assert!(paths.contains(".gitignore"), "got {paths:?}"); | |
| // .gitignore and .minimalignore themselves ship (they're project files). | |
| assert!(paths.contains(".gitignore"), "got {paths:?}"); | |
| assert!(paths.contains(".minimalignore"), "got {paths:?}"); |
🤖 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/minimal2/src/client.rs` around lines 341 - 343, The shipping test in
client.rs only asserts that .gitignore is included, so it can miss regressions
where .minimalignore is excluded. Update the assertion in the relevant test near
the paths.contains check to also verify .minimalignore is present, keeping the
existing .gitignore check so both project ignore files are covered.
| println!("{}", created.id); | ||
|
|
||
| // Copy-on-activate: stream the project into the session worktree so the | ||
| // interactive shell (mctx) finds minimal.toml and the project files. | ||
| if let Err(e) = client | ||
| .upload_workspace(&created.id.to_string(), &project_dir) | ||
| .await | ||
| { | ||
| eprintln!("Failed to upload workspace to session: {e}"); | ||
| return Err(()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Print the session id only after workspace upload succeeds.
If upload_workspace fails, the command returns Err(()) after already writing a session id to stdout, which can mislead scripts/users into treating a broken activation as successful.
Proposed fix
- println!("{}", created.id);
-
// Copy-on-activate: stream the project into the session worktree so the
// interactive shell (mctx) finds minimal.toml and the project files.
if let Err(e) = client
.upload_workspace(&created.id.to_string(), &project_dir)
.await
@@
eprintln!("Failed to upload workspace to session: {e}");
return Err(());
}
+
+ println!("{}", created.id);📝 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.
| println!("{}", created.id); | |
| // Copy-on-activate: stream the project into the session worktree so the | |
| // interactive shell (mctx) finds minimal.toml and the project files. | |
| if let Err(e) = client | |
| .upload_workspace(&created.id.to_string(), &project_dir) | |
| .await | |
| { | |
| eprintln!("Failed to upload workspace to session: {e}"); | |
| return Err(()); | |
| // Copy-on-activate: stream the project into the session worktree so the | |
| // interactive shell (mctx) finds minimal.toml and the project files. | |
| if let Err(e) = client | |
| .upload_workspace(&created.id.to_string(), &project_dir) | |
| .await | |
| { | |
| eprintln!("Failed to upload workspace to session: {e}"); | |
| return Err(()); | |
| } | |
| println!("{}", created.id); |
🤖 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/minimal2/src/main.rs` around lines 497 - 506, Move the session-id
stdout print in main so it only happens after client.upload_workspace(...).await
succeeds; right now created.id is printed before the upload result is known,
which can mislead callers on failure. Keep the existing error path in the
upload_workspace handling block, and emit println!("{}", created.id) only after
the Err(()) branch is avoided.
| Err(e) if *create_if_missing => { | ||
| tracing::warn!(error = %e, path = %path.display(), "host key unreadable; regenerating"); | ||
| let key = PrivateKey::random(&mut safe_rng(), russh::keys::Algorithm::Ed25519)?; | ||
| key.write_openssh_file(path, russh::keys::ssh_key::LineEnding::LF)?; | ||
| Ok(key) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What are the variants of russh::keys::Error (KeyError), and which variant represents an OpenSSH private-key parse/format failure vs an underlying I/O error?
💡 Result:
The russh::keys::Error enum (often defined as russh_keys::Error) contains numerous variants representing various failure modes during SSH key processing, including cryptographic, encoding, and I/O operations [1][2][3]. Key Variants for Distinguishing Failures: 1. OpenSSH Private-Key Parse/Format Failures: Failures related to the structure, formatting, or parsing of OpenSSH private keys are typically surfaced through specialized variants: * SshKey(Error): Represents errors stemming from the underlying ssh-key crate, which handles SSH key structures [1][4]. * SshEncoding(Error): Indicates errors encountered while decoding or encoding SSH-specific binary formats [1][4]. * Decode(DecodeError): Often associated with base64 decoding or other fundamental decoding steps required before format-specific parsing [1][4]. * KeyIsCorrupt: A general indicator that the key material does not conform to expected structural invariants [1][4]. * CouldNotReadKey: A broad variant often returned when the parsing logic fails to identify or successfully process the input key format [1][4][5]. 2. Underlying I/O Errors: * IO(Error): This variant explicitly wraps a standard library std::io::Error, representing failures encountered when interacting with the filesystem or network streams (e.g., file not found, permission denied, or interrupted reads) [1][2][4]. Note: The specific error categorization often depends on the underlying crate (such as ssh-key) being used by the version of russh you are consuming [4][6]. When debugging specific failures, such as those related to OpenSSH PEM formatting, developers often look for wrapping errors like SshKey or SshEncoding to identify if the issue is a format-compliance problem versus a low-level I/O failure [4][6].
Citations:
- 1: https://docs.rs/russh/latest/russh/keys/enum.Error.html
- 2: https://docs.rs/russh-keys/latest/i686-pc-windows-msvc/russh_keys/enum.Error.html
- 3: https://docs.rs/russh-keys/latest/russh_keys/enum.Error.html
- 4: https://docs.rs/russh-keys/latest/src/russh_keys/lib.rs.html
- 5: https://docs.rs/russh-keys/latest/src/russh_keys/format/mod.rs.html
- 6: Encrypted OpenSSH keys from puttygen fail with Encoding(Length) Eugeny/russh#619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server.rs excerpt ==\n'
sed -n '1,180p' crates/minimald/src/server.rs
printf '\n== search for russh key error type references ==\n'
rg -n "enum Error|type Error|CouldNotReadKey|KeyIsCorrupt|IO\\(|SshKey|SshEncoding|Decode" -S . --glob '!target' --glob '!node_modules' --glob '!dist' --glob '!build'Repository: gominimal/minimal
Length of output: 30277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== host key call sites ==\n'
rg -n "HostKey::OnDisk|create_if_missing|host_key:" crates/minimald -S
printf '\n== server.rs around host_key comment ==\n'
sed -n '40,110p' crates/minimald/src/server.rs
printf '\n== config/main setup around host key construction ==\n'
sed -n '1,260p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 13552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== host key call sites ==\n'
rg -n "HostKey::OnDisk|create_if_missing|host_key:" crates/minimald -S
printf '\n== server.rs around host_key comment ==\n'
sed -n '40,110p' crates/minimald/src/server.rs
printf '\n== config/main setup around host key construction ==\n'
sed -n '1,260p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 13552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== host key call sites ==\n'
rg -n "HostKey::OnDisk|create_if_missing|host_key:" crates/minimald -S
printf '\n== server.rs around host_key comment ==\n'
sed -n '40,110p' crates/minimald/src/server.rs
printf '\n== config/main setup around host key construction ==\n'
sed -n '1,260p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 13552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,335p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 1551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,335p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 1551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,335p' crates/minimald/src/main.rsRepository: gominimal/minimal
Length of output: 1551
Restrict host-key regeneration to parse/corruption failures
Err(e) if *create_if_missing also catches non-NotFound read errors, including transient I/O failures. On the persistent host-key path (create_if_missing: true in crates/minimald/src/main.rs), that can silently rotate the daemon’s identity and break existing trust. Match the read/parse-corruption cases explicitly and let other errors bubble up.
🤖 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/server.rs` around lines 86 - 91, The host-key reload
logic in the key-loading match is too broad because Err(e) if *create_if_missing
currently regenerates on every read failure, including transient I/O errors.
Update the match in the host key loading path to only regenerate for explicit
missing/parse-corruption cases from the read/parse step, and let other errors
return instead of rotating the key. Keep the regeneration behavior tied to the
existing PrivateKey::random and write_openssh_file flow, but narrow the error
handling around the host-key read branch so persistent identity is preserved.
Lets the macOS guest VM compose a session sandbox with no network, so interactive
minimal2 attachworks end-to-end.Problem
On macOS the session runs in a networkless libkrun VM with an empty cache, so two things blocked an interactive session:
minimal.toml not found(conformance R§3.1.4).no such package: base— the guest can't fetch the package closure.Changes (3 commits)
feat(minimal2,minimald): upload project workspace on activate— copy-on-activate.Client::upload_workspacestreams an ignore-awaretar.zstof the project (honours.gitignore/.ignore/.minimalignorevia theignorecrate, keeps dotfiles, skips.git, preserves modes) over the existingWorkspaceFilesTarZstsubsystem into the worktree. Shared subsystem/env-var consts moved into theminimald-rpcwire-contract crate.feat(minvmd,minimald): seeded cache disk for offline session compose—MINVMD_CACHE_PATHattaches a pre-seeded ext4 as/dev/vdb; minimald (guest pid-1) mounts it at/run/minimal/cacheand sets the state dir equal to the cache dir so compose's package hardlinks stay on one filesystem (cross-fs hardlink = EXDEV)..with_offline(true)on the session context. Guest-env fixes surfaced once compose worked: mount devpts (openptyfor the PTY) and a tmpfs at/tmp(hakoniwa stages there; rootfs is read-only). Newscripts/build-session-cache.shbuilds the seed (closure fetched from the remote cache, packed via loop-mount).fix(minimald): harden seeded cache disk against corrupt host key and slow boot reset— the writable image co-locates state, so an unclean shutdown can corrupt it:Config::host_keynow regenerates an unreadable/corrupt key (was: only a missing one → boot brick); the boot reset drops onlyproviders/instead ofrm -rf-ing all non-seed state (the old reset was O(session state) and could blow the 5 s READY timeout).Verification
attachcomposes the hakoniwa sandbox (CLONE_NEWNET) → own-ip PTask attaches to the switch (ip=100.64.0.2).cross build -p minimald --profile initramfs --features networking-proxy,networking-wggreen;minimal2builds + tests pass (incl. the new ignore-semantics test inclient.rs);cargo fmt,cargo clippy -p minvmd -D warningsclean. minimald is Linux-only (host can't build it) — verified via cross.Scope / follow-ups
minvmd-owns-vm-gvproxy) is tracked in DM1/3/4: run gvproxy on the host with a per-PTask vsock shuttle (minvmd-owns-vm-gvproxy) #572. A gvproxy-in-guest staging hack was tried and reverted.Notes
--network/--ingressflags). feat(minimal2): add --network and --ingress flags to activate #570 isn't onorigin/mainyet, so until it merges this PR's diff also shows feat(minimal2): add --network and --ingress flags to activate #570's commit; it cleans up once feat(minimal2): add --network and --ingress flags to activate #570 lands.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
/dev/ptssupport in the guest for better terminal behavior.