8000 [WIP] feat(minimald,minvmd): seeded cache disk + workspace upload for offline session compose by norrietaylor · Pull Request #573 · gominimal/minimal · GitHub
[go: up one dir, main page]

Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ google-cloud-storage = "1.7"
hakoniwa = { git = "https://github.com/souk4711/hakoniwa.git", rev = "41ce36e087fa1fcf28d17123042d150e71c09705", features = ["cgroups"] }
http-body = "1"
hex = "0.4"
ignore = "0.4"
indicatif = "0.18"
indoc = "^2.0"
jaq-all = "0.2.0"
Expand Down
8 changes: 8 additions & 0 deletions crates/minimal2/Cargo.toml
8000
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ edition.workspace = true
publish.workspace = true

[dependencies]
async-compression.workspace = true
async-tar.workspace = true
clap.workspace = true
clap_complete.workspace = true
camino.workspace = true
chrono.workspace = true
dirs.workspace = true
ignore.workspace = true
minimald-rpc.workspace = true
paths.workspace = true
russh.workspace = true
Expand All @@ -22,3 +25,8 @@ tracing-subscriber.workspace = true

minvmd = { path = "../minvmd" }
ot.workspace = true

[dev-dependencies]
tar.workspace = true
tempfile.workspace = true
zstd.workspace = true
194 changes: 193 additions & 1 deletion crates/minimal2/src/client.rs
< 8000 tr data-hunk="bcc4a063065aea3fbb76ecb1b36366bc8803b7cffd7e43994e1ba013dc5a3741" class="show-top-border">
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::path::Path;
use std::sync::Arc;
use std::time::Duration;

use minimald_rpc::OneshotSshRpc;
use minimald_rpc::{MINIMAL_SESSION_ID_ENV, OneshotSshRpc, STREAM_WORKSPACE_FILES};
use tokio::io::{AsyncReadExt, AsyncWriteExt};

/// Max retries when connecting to the daemon UDS.
Expand Down Expand Up @@ -128,6 +128,121 @@ impl Client {
serde_json::from_slice(&resp_buf)
.map_err(|e| format!("decode response for {}: {e}", R::NAME))
}

/// Stream the project directory into the session's workspace
/// (copy-on-activate). Builds an ignore-aware `tar.zst` of `project_dir`
/// and uploads it over the `WorkspaceFilesTarZst` subsystem; the daemon
/// unpacks it into the session worktree. Reports server-side unpack errors
/// (relayed on the channel's stderr stream) as `Err`.
pub async fn upload_workspace(
&mut self,
session_id: &str,
project_dir: &Path,
) -> Result<(), String> {
let payload = build_workspace_tar_zst(project_dir).await?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.


let mut channel = self
.handle
.channel_open_session()
.await
.map_err(|e| format!("open workspace channel: {e}"))?;

// The server reads the session id from the channel env, captured at
// subsystem-request time — so set_env must precede request_subsystem.
channel
.set_env(true, MINIMAL_SESSION_ID_ENV, session_id)
.await
.map_err(|e| format!("set_env {MINIMAL_SESSION_ID_ENV}: {e}"))?;
channel
.request_subsystem(true, STREAM_WORKSPACE_FILES)
.await
.map_err(|e| format!("request subsystem {STREAM_WORKSPACE_FILES}: {e}"))?;

channel
.data(&payload[..])
.await
.map_err(|e| format!("stream workspace tarball: {e}"))?;
channel
.eof()
.await
.map_err(|e| format!("half-close workspace channel: {e}"))?;

// Drain until the server closes the channel; it relays any unpack
// failure on extended-data stream 1 (stderr).
let mut stderr = Vec::new();
while let Some(msg) = channel.wait().await {
if let russh::ChannelMsg::ExtendedData { data, ext: 1 } = msg {
stderr.extend_from_slice(&data);
}
}
if !stderr.is_empty() {
return Err(String::from_utf8_lossy(&stderr).trim().to_owned());
}
Ok(())
}
}

/// Build an ignore-aware zstd-compressed tar of `project_dir`, with entry paths
/// relative to the project root. Honours `.gitignore`, `.ignore`, and a custom
/// `.minimalignore` (via the `ignore` crate's gitignore semantics) even outside
/// a git repo. Dotfiles are kept, but `.git` is always skipped. Symlinks are
/// not followed. File modes are preserved.
async fn build_workspace_tar_zst(project_dir: &Path) -> Result<Vec<u8>, String> {
use std::os::unix::fs::PermissionsExt;

// Collect the file set synchronously (the `ignore` walk is sync), then
// stream contents asynchronously below.
let mut files = Vec::new();
let walker = ignore::WalkBuilder::new(project_dir)
// Keep dotfiles (.github, .env, .gitignore, …) — they're part of the
// project — but never the VCS dir itself.
.hidden(false)
.filter_entry(|e| e.file_name() != ".git")
// Apply ignore files even when project_dir is not a git working tree.
.require_git(false)
.add_custom_ignore_filename(".minimalignore")
.build();
for entry in walker {
let entry = entry.map_err(|e| format!("walk project: {e}"))?;
if entry.file_type().is_some_and(|t| t.is_file()) {
files.push(entry.into_path());
}
}

let mut builder = async_tar::Builder::new(Vec::new());
for path in &files {
let rel = path
.strip_prefix(project_dir)
.map_err(|e| format!("relativize {}: {e}", path.display()))?;
let meta = std::fs::metadata(path).map_err(|e| format!("stat {}: {e}", path.display()))?;
let contents = tokio::fs::read(path)
.await
.map_err(|e| format!("read {}: {e}", path.display()))?;

let mut header = async_tar::Header::new_gnu();
header.set_size(contents.len() as u64);
header.set_mode(meta.permissions().mode() & 0o777);
builder
.append_data(&mut header, rel, &contents[..])
.await
.map_err(|e| format!("tar append {}: {e}", rel.display()))?;
}

let tar_bytes = builder
.into_inner()
.await
.map_err(|e| format!("finalize tar: {e}"))?;

let mut encoder = async_compression::tokio::write::ZstdEncoder::new(Vec::new());
encoder
.write_all(&tar_bytes)
.await
.map_err(|e| format!("zstd compress: {e}"))?;
encoder
.shutdown()
.await
.map_err(|e| format!("zstd finalize: {e}"))?;
Ok(encoder.into_inner())
}

/// Resolve the daemon socket path.
Expand Down Expand Up @@ -164,3 +279,80 @@ pub fn resolve_socket_path(
#[cfg(target_os = "macos")]
minvmd::sock::resolve_uds_path()
}

#[cfg(test)]
mod tests {
use std::collections::BTreeSet;

use super::build_workspace_tar_zst;

/// Decompress + read the tar produced by `build_workspace_tar_zst` and
/// return its entry paths.
fn entry_paths(tar_zst: &[u8]) -> BTreeSet<String> {
let tar_bytes = zstd::stream::decode_all(tar_zst).expect("zstd decode");
let mut archive = tar::Archive::new(&tar_bytes[..]);
archive
.entries()
.expect("entries")
.map(|e| {
e.expect("entry")
.path()
.expect("path")
.to_string_lossy()
.into_owned()
})
.collect()
}

fn write(root: &std::path::Path, rel: &str, contents: &str) {
let path = root.join(rel);
std::fs::create_dir_all(path.parent().unwrap()).unwrap();
std::fs::write(path, contents).unwrap();
}

#[tokio::test]
async fn workspace_tar_honours_ignore_files_keeps_dotfiles_skips_git() {
let dir = tempfile::tempdir().unwrap();
let root = dir.path();

// Kept: project files and non-git dotfiles.
write(root, "minimal.toml", "name = \"p\"\n");
write(root, "src/main.rs", "fn main() {}\n");
write(root, ".github/workflows/ci.yml", "on: push\n");

// .gitignore excludes build output; .minimalignore excludes a secret.
write(root, ".gitignore", "ignored.txt\n/target\n");
write(root, ".minimalignore", "secret.txt\n");
write(root, "ignored.txt", "nope");
write(root, "secret.txt", "nope");
write(root, "target/junk.o", "nope");

// The VCS dir is always skipped even though dotfiles are kept.
write(root, ".git/config", "[core]\n");

let paths = entry_paths(&build_workspace_tar_zst(root).await.unwrap());

assert!(paths.contains("minimal.toml"), "got {paths:?}");
assert!(paths.contains("src/main.rs"), "got {paths:?}");
assert!(
paths.contains(".github/workflows/ci.yml"),
"dotfiles kept; got {paths:?}"
);
// .gitignore and .minimalignore themselves ship (they're project files).
assert!(paths.contains(".gitignore"), "got {paths:?}");

Comment on lines +341 to +343
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
// .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.

assert!(!paths.contains("ignored.txt"), ".gitignore; got {paths:?}");
assert!(
!paths.contains("secret.txt"),
".minimalignore; got {paths:?}"
);
assert!(
!paths.iter().any(|p| p.starts_with("target/")),
"target/ gitignored; got {paths:?}"
);
assert!(
!paths.iter().any(|p| p.starts_with(".git/")),
".git skipped; got {paths:?}"
);
}
}
Loading
Loading
0