feat(minimald): side-op subsystem, package builds - #960
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds session-managed build side operations, shared build-event rendering, and ChangesSession build operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SSHExec
participant SessionHandle
participant SideOp
participant BuildRenderer
participant SSHChannel
Client->>SSHExec: min build flags packages
SSHExec->>SessionHandle: start_build(rebuild, packages)
SessionHandle->>SideOp: spawn background build
SideOp-->>SSHExec: BuildUpdate events
SSHExec->>BuildRenderer: render BuildEvent
BuildRenderer-->>SSHExec: status or stderr line
SSHExec->>SSHChannel: write rendered output
SSHExec->>SSHChannel: eof, exit status, close
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
a2db69d to
54999fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minimald/src/session.rs (1)
605-611: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
IsBusyshould include active side-ops.
SessionInner::Active { host, .. } => host.is_some()reports the session as idle while a build side-op is running without an attached host, so an unforced shutdown can cancel in-flight work. Treat a non-emptysopslist as busy too.Proposed fix
- SessionInner::Active { host, .. } => host.is_some(), + SessionInner::Active { host, sops, .. } => host.is_some() || !sops.is_empty(),🤖 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.rs` around lines 605 - 611, Update the IsBusy handling in the SessionMessage match so SessionInner::Active reports busy when either host is present or the sops list is non-empty. Preserve the existing pending check for Draft sessions and use the Active variant’s sops field in the condition.
🤖 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 925-963: Update run_build to consume the final build outcome
propagated by session.start_build and distinguish successful, failed, and
cancelled builds after the event loop. Emit “msg:build finished” only for
success, and report the propagated failure or cancellation result instead of
unconditionally claiming completion.
In `@crates/minimald/src/exec.rs`:
- Around line 863-930: The exit status in run_build_exec currently reports
success whenever event streaming starts, even if the build later fails or is
cancelled. After new_build/SideOp exposes the build’s final result through the
event stream or completion handle, consume that result in run_build_exec and set
exit_status to a non-zero value for failure or cancellation while preserving the
existing channel-write failure handling.
- Around line 844-852: The path-instructions documentation for the `min attach
-c` exec handler must be updated to include the newly supported `min build ...`
command alongside `min run <task>` and the internal `git-receive-pack min://`
path. Keep the implementation in the `Some("build")` handler unchanged and
revise only the security-boundary wording to reflect all accepted forms.
- Around line 810-843: Prevent the `Some("run")` branch from panicking when
`rem` is exactly `"run"` by replacing the unchecked `strip_prefix("run ")`
unwrap in the `ExecTask` construction with the same safe empty-task behavior
used by the nearby `build` arm. Preserve the existing task text for requests
that include a trailing task name.
In `@crates/minimald/src/session_sop.rs`:
- Around line 122-155: The build task spawned in spawn_build currently logs
failures but does not propagate the terminal outcome, causing env.rs::run_build
and exec.rs::run_build_exec to report success after failures or cancellation.
Return or emit a final success/failure/cancellation result from the spawn_build
flow, then have both callers map unsuccessful outcomes to a non-zero exit while
preserving the existing event-stream shutdown behavior.
- Around line 106-117: Update the build error handling in
build_graph_with_cancel and its run_build/run_build_exec callers so graph
construction failures are propagated as terminal errors instead of only logged
before dropping the sink. Ensure the caller receives the error and reports build
failure rather than emitting “build finished” or exiting successfully, while
preserving normal successful-build behavior.
In `@crates/minimald/src/session.rs`:
- Around line 931-952: The start_build method must reap completed side-ops
before registering a new one, preventing finished JoinHandles from accumulating
in the session. In the SessionInner::Active branch, remove or retain entries
based on their JoinHandle::is_finished() status before pushing sop, while
preserving the existing Draft handling and active side-op registration.
---
Outside diff comments:
In `@crates/minimald/src/session.rs`:
- Around line 605-611: Update the IsBusy handling in the SessionMessage match so
SessionInner::Active reports busy when either host is present or the sops list
is non-empty. Preserve the existing pending check for Draft sessions and use the
Active variant’s sops field in the condition.
🪄 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: a319f209-dfd7-4099-b999-54746f43cb4e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/minimald/Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/env_min_helper.shcrates/minimald/src/exec.rscrates/minimald/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/session_sop.rscrates/mip/src/cmd_pkg.rscrates/orchestrator/src/lib.rscrates/orchestrator/src/render.rs
| /// Kicks off a background package build as a side-op and registers it on | ||
| /// this session, returning the receiver end of its event stream. The build | ||
| /// runs against a fresh workspace-rooted context (rebuilt per call so it | ||
| /// tracks `minimal.toml` edits). | ||
| /// | ||
| /// The returned receiver closes when the build ends. | ||
| async fn start_build( | ||
| &mut self, | ||
| rebuild: bool, | ||
| pkgs: Vec<String>, | ||
| ) -> Result<mpsc::Receiver<BuildEvent>, std::io::Error> { | ||
| let ctx = self.context(false).await.map_err(std::io::Error::other)?; | ||
| let (sop, rx) = SideOp::spawn_build(self.weak_self.clone(), rebuild, pkgs, ctx, 64).await?; | ||
| match &mut self.inner { | ||
| SessionInner::Active { sops, .. } => sops.push(sop), | ||
| SessionInner::Draft { .. } => { | ||
| sop.shutdown().await; | ||
| unreachable!("`context()` already rejected a `Draft`"); | ||
| } | ||
| } | ||
| Ok(rx) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tokio pin locations ==\n'
rg -n --hidden --glob '!*target*' 'tokio' Cargo.toml Cargo.lock crates -g '!**/target/**' | sed -n '1,160p'
printf '\n== session_sop file locations ==\n'
fd -a 'session_sop.rs|session.rs' crates/minimald/src
printf '\n== outline session.rs ==\n'
ast-grep outline crates/minimald/src/session.rs --view expanded | sed -n '1,220p'
printf '\n== outline session_sop.rs ==\n'
ast-grep outline crates/minimald/src/session_sop.rs --view expanded | sed -n '1,240p'Repository: gominimal/minimal
Length of output: 14306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tokio pin locations ==\n'
rg -n --hidden --glob '!*target*' 'tokio' Cargo.toml Cargo.lock crates -g '!**/target/**' | sed -n '1,200p'
printf '\n== file list ==\n'
fd -a 'session_sop.rs|session.rs' crates/minimald/src
printf '\n== outline session.rs ==\n'
ast-grep outline crates/minimald/src/session.rs --view expanded | sed -n '1,220p'
printf '\n== outline session_sop.rs ==\n'
ast-grep outline crates/minimald/src/session_sop.rs --view expanded | sed -n '1,240p'Repository: gominimal/minimal
Length of output: 17805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Cargo.lock tokio entries =='
python3 - <<'PY'
import re
from pathlib import Path
p = Path('Cargo.lock')
txt = p.read_text()
for m in re.finditer(r'(?ms)^\[\[package\]\]\n(?:.*\n)*?name = "tokio"\n(?:.*\n)*?version = "([^"]+)"', txt):
print(m.group(1))
PY
echo '== session_sop.rs and session.rs paths =='
git ls-files 'crates/minimald/src/session*.rs'
echo '== start_build and SideOp definitions =='
rg -n 'start_build|struct SideOp|impl SideOp|is_finished|shutdown|sops' crates/minimald/src/session.rs crates/minimald/src/session_sop.rsRepository: gominimal/minimal
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Cargo.toml tokio pin =='
sed -n '132,142p' Cargo.toml
echo '== Cargo.lock tokio package entry =='
sed -n '6739,6760p' Cargo.lock
echo '== session_sop.rs relevant sections =='
sed -n '1,180p' crates/minimald/src/session_sop.rs
echo '== session.rs start_build / stop_running =='
sed -n '931,980p' crates/minimald/src/session.rsRepository: gominimal/minimal
Length of output: 8647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== sops usage across minimald =='
rg -n 'sops|is_finished\(|retain\(' crates/minimald/src/session.rs crates/minimald/src/session_sop.rsRepository: gominimal/minimal
Length of output: 834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Cargo.toml tokio pin =='
sed -n '136,141p' Cargo.toml
echo '== Cargo.lock tokio package entry =='
sed -n '6739,6760p' Cargo.lock
echo '== session_sop.rs relevant sections =='
sed -n '1,180p' crates/minimald/src/session_sop.rs
echo '== session.rs start_build / stop_running =='
sed -n '931,980p' crates/minimald/src/session.rs
echo '== search for any pruning of sops =='
rg -n 'retain\(|remove\(|swap_remove\(|drain\(|is_finished\(' crates/minimald/srcRepository: gominimal/minimal
Length of output: 9720
Finished side-ops stay in sops until teardown. Long-lived sessions can accumulate completed SideOps and their join handles across repeated builds; reap finished ops before pushing a new one, e.g. by filtering on JoinHandle::is_finished().
🤖 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.rs` around lines 931 - 952, The start_build
method must reap completed side-ops before registering a new one, preventing
finished JoinHandles from accumulating in the session. In the
SessionInner::Active branch, remove or retain entries based on their
JoinHandle::is_finished() status before pushing sop, while preserving the
existing Draft handling and active side-op registration.
bryan-minimal
left a comment
There was a problem hiding this comment.
min build in the daemon is the #749/#751 direction landing — for our pkgs clean-room recipe it would drop the 29 MB mip upload entirely. Design reads well: actor-owned side-ops, WeakSessionHandle to avoid cycles, graph_from_all_packages on the blocking pool, cancellation tied to shutdown, non-blocking sinks, one shared BuildRenderer.
Two fixes inline. #1 is the blocker: a failed build reports success on both surfaces.
Minor: unknown --flags silently become package names; and I didn't see coverage for the new run_build / run_build_exec paths — the exit-status contract is worth pinning.
Reviewed via Claude Code.
| .build_graph_with_cancel(&graph, rebuild, Some(log_tx), cancel) | ||
| .await | ||
| { | ||
| tracing::warn!("session build failed: {e}"); |
There was a problem hiding this comment.
Build failure never leaves the daemon.
Result is warn-logged and dropped, then sinks.clear() closes the stream identically for pass and fail — so neither caller can tell them apart:
run_build_exec:statusonly becomes1on a channel write failure, so a real failure drains normally →exit_status(0).run_build: unconditionalmsg:build finished, noerror:terminator.
Also inverted: client disconnect → 1, broken build → 0.
Same class as min check exiting 0 on failing checkers (mctx/src/env.rs) — CI can only key on exit status, so a second instance is worth avoiding.
Suggest a terminal BuildEvent::Finished { ok } (or the result on SideOp) so both callers can set exit status / emit error:.
There was a problem hiding this comment.
Fixed - new message Finished which holds the outcome, plumbed through.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_min_helper.sh`:
- Line 152: Update the min build usage text in env_min_helper.sh to advertise
the supported --verbose and --rebuild flags alongside the optional packages
argument, while preserving the existing command description.
🪄 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: 8a5ea62d-e931-4b24-b5e1-49102c919e19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/minimald/Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/env_min_helper.shcrates/minimald/src/exec.rscrates/minimald/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/session_sop.rscrates/mip/src/cmd_pkg.rscrates/orchestrator/src/lib.rscrates/orchestrator/src/render.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/orchestrator/src/lib.rs
- crates/minimald/src/lib.rs
- crates/orchestrator/src/render.rs
- crates/minimald/src/rpc.rs
- crates/minimald/Cargo.toml
- crates/minimald/src/session_host.rs
- crates/minimald/src/env.rs
- crates/minimald/src/session_sop.rs
- crates/minimald/src/exec.rs
- crates/minimald/src/session.rs
54999fb to
be0c064
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/minimald/src/session_sop.rs (2)
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment omits the terminal
Finishedmessage.The doc for
spawn_buildsays the receiver "streams itsBuildEvents until the build completes (at which point the sender is dropped and the receiver closes)", but doesn't mention theBuildUpdate::Finishedoutcome sent just before the drop (lines 211-213). This is inconsistent with the more preciseBuildUpdatedoc (lines 31-38) and risks a caller assuming a closed channel alone signals completion without checking the final outcome — the exact class of bug flagged in earlier review rounds on this PR.✏️ Suggested wording
- /// Kicks off a build side-op wired to a single structured sink, returning - /// the op alongside the receiver end that streams its [`BuildEvent`]s until - /// the build completes (at which point the sender is dropped and the - /// receiver closes). The session actor stores the op and hands the receiver - /// to whoever requested the build so it can render progress. + /// Kicks off a build side-op wired to a single structured sink, returning + /// the op alongside the receiver end that streams [`BuildUpdate::Event`]s, + /// followed by exactly one terminal [`BuildUpdate::Finished`], after which + /// the sender is dropped and the receiver closes. The session actor stores + /// the op and hands the receiver to whoever requested the build so it can + /// render progress and report the outcome.🤖 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 109 - 113, Update the `spawn_build` doc comment to explicitly state that the receiver gets a terminal `BuildUpdate::Finished` message containing the build outcome immediately before the sender is dropped and the channel closes. Preserve the existing description of streaming `BuildEvent`s and clarify that callers must inspect this final outcome rather than relying on closure alone.
114-125: 🩺 Stability & Availability | 🔵 TrivialReminder: run daemon integration coverage for this change.
This introduces new session-actor build orchestration in
crates/minimald. As per coding guidelines, changes to VM or daemon paths should runjust e2eand/orjust test-vm, and shouldn't rely only on unit tests for this kind of concurrent/session-teardown behavior.🤖 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 114 - 125, Run the daemon integration coverage for the new session-actor build orchestration in spawn_build and its Self::new_build flow, using just e2e and/or just test-vm. Validate concurrent build updates and session teardown behavior beyond unit tests.Source: Coding guidelines
🤖 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/session_sop.rs`:
- Around line 193-202: Update the terminal outcome classification around
BuildOutcome so cancellation is considered only when build_err is present:
preserve BuildOutcome::Success whenever the build completed without an error,
even if cancel_flag.is_cancelled() becomes true during the subsequent pump.await
drain, while continuing to classify errored builds as Cancelled when
cancellation won over the failure.
- Around line 101-107: Update SessionSop::shutdown and the terminal
BuildUpdate::Finished delivery so shutdown cannot wait indefinitely on a
subscriber that stops receiving. Bound the final send with an appropriate
timeout or abort path, while preserving normal delivery for responsive consumers
and the existing join-error warning behavior.
---
Nitpick comments:
In `@crates/minimald/src/session_sop.rs`:
- Around line 109-113: Update the `spawn_build` doc comment to explicitly state
that the receiver gets a terminal `BuildUpdate::Finished` message containing the
build outcome immediately before the sender is dropped and the channel closes.
Preserve the existing description of streaming `BuildEvent`s and clarify that
callers must inspect this final outcome rather than relying on closure alone.
- Around line 114-125: Run the daemon integration coverage for the new
session-actor build orchestration in spawn_build and its Self::new_build flow,
using just e2e and/or just test-vm. Validate concurrent build updates and
session teardown behavior beyond unit tests.
🪄 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: 6b3ccb0b-b36b-4e99-8528-1b1a9efab107
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/minimald/Cargo.tomlcrates/minimald/src/env.rscrates/minimald/src/env_min_helper.shcrates/minimald/src/exec.rscrates/minimald/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/session_sop.rscrates/mip/src/cmd_pkg.rscrates/orchestrator/src/lib.rscrates/orchestrator/src/render.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/orchestrator/src/lib.rs
- crates/minimald/src/lib.rs
- crates/mip/src/cmd_pkg.rs
- crates/minimald/src/session_host.rs
- crates/orchestrator/src/render.rs
- crates/minimald/src/env.rs
- crates/minimald/src/rpc.rs
- crates/minimald/src/exec.rs
- crates/minimald/src/session.rs
be0c064 to
23cf6a3
Compare
23cf6a3 to
1c3174d
Compare
bryan-minimal
left a comment
There was a problem hiding this comment.
Both fixes verified at 1c3174dd — and handled more thoroughly than I flagged. 👍
Build outcome now propagates via terminal BuildUpdate::Finished(BuildOutcome):
run_build_exec:Success→ 0,Failed→ 1 (+ stderr),Cancelled→ 130,None→ 1. Only success exits 0.run_build: onlySuccessprintsbuild finished;Failed/Cancelled/Noneemiterror:.
Two things I didn't ask for and like: the None arm (channel closed without a terminal update) fails closed rather than silently passing, and Finished is explicitly protected from being dropped under sink backpressure — which is where this class of fix usually leaks.
Panic fixed: empty/missing task is rejected before the ack, no .unwrap().
Non-blocking nits, take or leave: still no coverage pinning the exit-status contract (that's the bit most likely to silently regress), and unknown --flags still fall through into the package list. CI has jobs pending — worth landing on green.
Reviewed via Claude Code.
SideOpsubsystem and lifecycle plumbed into session actormin build [--verbose] [--rebuild] <pkgs>...command in the session shellmin build [--verbose] [--rebuild] <pkgs>...command over an ssh exec requestNote
Add
min buildside-op subsystem tominimaldfor in-session package buildssession_sopmodule implementing cancellable background build side-ops, withBuildUpdate/BuildOutcomeevent types and aBuildSinkfor streaming progress to callers.SessionHandlewithstart_build()andpatches_upload_lock()via actor message passing; introducesWeakSessionHandleso in-sandboxmincommands can reference the owning session without creating a retain cycle.min build [--verbose] [--rebuild] pkgs...as an RPC command in env.rs and as an SSH exec command in exec.rs, both streaming build output and returning structured exit codes.BuildRendererin theorchestratorcrate to convertBuildEventstreams into attributed line output; reuses it inmipand bothminimaldbuild handlers.Session::stop_runningnow tears down all active side-ops before stopping the host process, fixing a leak where background build tasks could outlive the session.Macroscope summarized 1c3174d.
Summary by CodeRabbit
min buildsupport for in-sandbox builds, including--verboseand--rebuild.--verbose.exechandling to usemin <subcommand> ...(includingmin run ...andmin build ...), with clearer validation and routing.