feat(minimald): support min materialize within a session - #1054
Conversation
📝 WalkthroughWalkthroughAdds a unified materialization operation for OCI and raw-file outputs, streams progress and artifact bytes through sessions, resolves secure sandbox destinations, and centralizes target architecture and graph top-level selection. ChangesMaterialize output pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Helper as min materialize
participant Channel as SessionChannel
participant Session as SessionHandle
participant SideOp as Materialize side-op
participant FS as Sandbox filesystem
Helper->>Channel: send materialize request with cwd and args
Channel->>Session: start_materialize(output_name, arch)
Session->>SideOp: spawn materialization
SideOp-->>Channel: stream chunks and events
Channel->>FS: create and write output artifact
Channel-->>Helper: render progress and completion
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
This is going to need documentation |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
crates/minimald/src/session_sop.rs (1)
797-834: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a larger byte pipeline for multi-hundred-MiB images.
4 KiB
BufWriter× a depth-3 channel means ~12 KiB in flight and oneblocking_send+Vecallocation per 4 KiB, i.e. tens of thousands of hand-offs for a sizeable OCI tar. Bumping the buffer to 64–256 KiB (channel depth unchanged) cuts the cross-thread traffic without weakening the backpressure property this design relies on.🤖 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/session_sop.rs` around lines 797 - 834, Increase the BufWriter capacity used in the Materialize sink within the spawn_blocking assembly flow from 4 KiB to a substantially larger buffer, such as 64–256 KiB, while keeping the byte channel depth at 3 to preserve backpressure.crates/op/src/materialize.rs (1)
220-234: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider atomic writes for
Sink::Pathdestinations.
std::fs::File::create(OCI path) andstd::fs::copy(raw-file path) write directly to the final destination path. If materialization fails partway (I/O error, disk full, process killed), the destination is left holding a partial/corrupt artifact rather than either the old file or nothing — a caller polling for the output path's existence could be misled.♻️ Suggested direction (write-to-temp + rename)
-Sink::Path(path) => { - let file = std::fs::File::create(&path).map_err(|e| { - Error::Other(anyhow!("creating {}: {e}", path.display())) - })?; - let mut w = image.write(opts, Counted::new(file), &events).await?; - w.flush()?; - w.bytes -} +Sink::Path(path) => { + let tmp = tmp_path_for(&path); + let file = std::fs::File::create(&tmp).map_err(|e| { + Error::Other(anyhow!("creating {}: {e}", tmp.display())) + })?; + let mut w = image.write(opts, Counted::new(file), &events).await?; + w.flush()?; + std::fs::rename(&tmp, &path) + .map_err(|e| Error::Other(anyhow!("finalizing {}: {e}", path.display())))?; + w.bytes +}A similar temp+rename pattern would apply to
deliver_file'sSink::Patharm.Also applies to: 295-320
🤖 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/op/src/materialize.rs` around lines 220 - 234, Make `Sink::Path` writes atomic in both the materialization match block and `deliver_file`: write the OCI or raw-file output to a temporary file in the destination directory, then rename it to the final path only after the write or copy succeeds. Preserve the existing error context and ensure temporary artifacts are cleaned up when the operation fails.
🤖 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/env.rs`:
- Around line 1013-1042: The materialization path must remove the destination
artifact when writing fails or MaterializeOutcome::summarize returns a non-zero
status. Update the write_err and status-error branches after the updates loop to
delete output before returning, while preserving existing error reporting and
successful completion behavior.
- Around line 1253-1269: Move the containment validation in the artifact path
handling before tokio::fs::create_dir_all(parent), so no directories are created
through a symlinked parent outside the session. Resolve the deepest existing
ancestor of parent and verify its canonical path remains under the canonicalized
base, while preserving the existing invalid-path and permission-denied errors;
only create missing directories after this check.
- Around line 1053-1062: Update the permission mask in the
MaterializeOutcome::Completed handling to use only 0o777 when calling
set_permissions, preventing setuid, setgid, and sticky bits from being
propagated while preserving ordinary read, write, and executable permissions.
In `@crates/minimald/src/session_sop.rs`:
- Around line 333-348: Update the chunk fan-out handling around the byte_rx
receiver to snapshot or clone the current sinks while holding inner.lock(), then
release the lock before awaiting any sink.send calls. Preserve delivery tracking
and the no-subscribers early return, while ensuring bounded subscriber
backpressure cannot block finish(), registration, or shutdown on the Inner
mutex.
In `@crates/op/src/materialize.rs`:
- Around line 179-320: Update Materialize::run so blocking materialization work,
including filesystem operations and rayon-backed image writing, runs through
Tokio’s spawn_blocking rather than on the async caller thread. Move the
synchronous logic currently in run, including extract_raw_file and deliver_file
usage, into a synchronous helper invoked by spawn_blocking, while preserving
event emission, error propagation, and the existing Report result.
---
Nitpick comments:
In `@crates/minimald/src/session_sop.rs`:
- Around line 797-834: Increase the BufWriter capacity used in the Materialize
sink within the spawn_blocking assembly flow from 4 KiB to a substantially
larger buffer, such as 64–256 KiB, while keeping the byte channel depth at 3 to
preserve backpressure.
In `@crates/op/src/materialize.rs`:
- Around line 220-234: Make `Sink::Path` writes atomic in both the
materialization match block and `deliver_file`: write the OCI or raw-file output
to a temporary file in the destination directory, then rename it to the final
path only after the write or copy succeeds. Preserve the existing error context
and ensure temporary artifacts are cleaned up when the operation fails.
🪄 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: 21c0006c-7df5-43c6-8fe6-d7e519e2689b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/mfile/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/env_min_helper.shcrates/minimald/src/session.rscrates/minimald/src/session_sop.rscrates/minimald/src/sessions.rscrates/mip/src/cmd_materialize.rscrates/op/src/lib.rscrates/op/src/materialize.rscrates/op/src/oci_image.rscrates/sandbox2/src/lib.rs
8c8ba85 to
caa54c6
Compare
caa54c6 to
d6dd623
Compare
|
Added docs to sandbox operations doc. Will do a follow-up to clean that up as well. |
opcrateminhelper in a sessionNote
Add
materializesubcommand to minimald sessions to stream build artifactsop::Materializein crates/op/src/materialize.rs that produces OCI image tarballs or raw files from a build graph, emitting structuredMaterializeEventprogress and returning aReportwith byte count and file mode.SideOp::new_materializein crates/minimald/src/session_sop.rs to run materialize as a background side-op, streamingMaterializeUpdate(events, chunks, and a terminalFinished) to subscribers with backpressure; the pump stops assembly immediately when all subscribers disconnect.SessionHandle::start_materializein crates/minimald/src/session.rs so callers can trigger a materialize run and receive the update stream.materializeRPC in crates/minimald/src/env.rs that resolves the--outputpath relative to the sandbox working directory, writes streamed bytes into the session workspace or home directory with symlink traversal protection, and sets file permissions from the reported mode.materializesubcommand to crates/minimald/src/env_min_helper.sh, forwarding the sandbox PWD and arguments to the daemon.mip'scmd_materializein crates/mip/src/cmd_materialize.rs to use the new unifiedop::Materialize.create_artifactopens destination files withO_NOFOLLOWand refuses paths outside the session workspace or home; symlink escapes at any path component are denied with an IO error.Macroscope summarized d6dd623.
Summary by CodeRabbit
min materializeto materializeminimal.tomloutputs as OCI image archives or extracted raw-file artifacts.--outputand--archoverrides, streams progress/events, reports completion, and applies executable permissions when produced by the build.materializesyntax and flags.