feat(exec): retain terminal sessions after exit - #1146
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe execution subsystem now tracks live, retained, and tombstone states. It supports terminal output summaries, exclusive attachment leases, ID validation, reservation-based startup, timeout cancellation, lifecycle pruning, shutdown cleanup, and explicit initialization registration failures. ChangesExecution lifecycle
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutionService
participant ExecutionRegistry
participant ExecutionState
participant OutputManager
Client->>ExecutionService: start execution
ExecutionService->>ExecutionRegistry: reserve execution ID
ExecutionService->>ExecutionState: spawn and configure execution
ExecutionService->>ExecutionRegistry: publish execution state
ExecutionRegistry->>ExecutionState: observe terminal state
ExecutionState->>OutputManager: collect terminal output summary
Client->>ExecutionService: attach or wait
ExecutionService->>ExecutionRegistry: look up lifecycle state
ExecutionRegistry-->>ExecutionService: return live, retained, or tombstone state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 BoxLite review — couldn't completepowered by BoxLite |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/guest/src/service/exec/state.rs (1)
416-462: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA displaced
output_taskcan trip the assert and leak its handle.
ConsumerLease::dropinsrc/guest/src/service/exec/output.rsLines 55-60 releases the lease when theAttachStreamis dropped. TheAttachStreamis owned by the forwarder task spawned at Line 437, so it drops as that task body ends, beforeJoinHandle::is_finished()reportstrue.In that window a second
attach_outputcall can:
- run
join_finished_output_task, which returnsNonebecause the previous handle is not finished yet,- pass the
releasedcheck,- claim the now-free consumer lease at Line 431 or Line 432,
- reach Line 451 with
inner.output_taskstillSome(..).
debug_assert!(inner.output_task.is_none())then panics in debug builds. In release builds Line 452 overwrites the previous handle, so that handle is dropped without being awaited.Take the existing handle instead of asserting it is absent, and await it after the lock is released.
🐛 Proposed fix to displace the previous handle safely
- let task_to_abort = { + let (task_to_abort, displaced) = { let mut inner = self.inner.lock().await; if inner.released { - Some(task) + (Some(task), None) } else { - debug_assert!(inner.output_task.is_none()); - inner.output_task = Some(task); - None + let displaced = inner.output_task.replace(task); + (None, displaced) } }; + if let Some(displaced) = displaced { + // The lease was released as the previous forwarder ended; let it finish. + let _ = displaced.await; + } if let Some(task) = task_to_abort { task.abort(); let _ = task.await; return Err(ExecutionError::HandleUnavailable); } Ok(rx)🤖 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 `@src/guest/src/service/exec/state.rs` around lines 416 - 462, Update attach_output to safely displace any existing inner.output_task instead of asserting it is absent. While holding the lock, replace the previous handle with the newly spawned task, then release the lock and await the displaced handle before returning or completing the attachment, preserving the released-state cleanup and HandleUnavailable behavior.
🧹 Nitpick comments (4)
src/guest/src/service/exec/output.rs (1)
253-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated summary construction.
terminal_summaryandsealed_terminal_summarybuild the identicalOutputTerminalSummaryfromstate; only the precondition differs. A private helper onOutputStateremoves the duplication and keeps the two preconditions visible.♻️ Proposed refactor
pub(crate) async fn terminal_summary(&self) -> Option<OutputTerminalSummary> { let state = self.inner.lock().await; if !state.stdout.finished || !state.stderr.finished { return None; } - - Some(OutputTerminalSummary { - stdout: OutputStreamSummary { - enabled: state.stdout.enabled, - total_bytes: state.stdout.total_bytes, - }, - stderr: OutputStreamSummary { - enabled: state.stderr.enabled, - total_bytes: state.stderr.total_bytes, - }, - reader_failure: state - .failure - .as_ref() - .map(|failure| failure.message.clone()), - }) + Some(state.summary()) } pub(crate) async fn sealed_terminal_summary(&self) -> Option<OutputTerminalSummary> { let state = self.inner.lock().await; if !state.sealed { return None; } - Some(OutputTerminalSummary { - stdout: OutputStreamSummary { - enabled: state.stdout.enabled, - total_bytes: state.stdout.total_bytes, - }, - stderr: OutputStreamSummary { - enabled: state.stderr.enabled, - total_bytes: state.stderr.total_bytes, - }, - reader_failure: state - .failure - .as_ref() - .map(|failure| failure.message.clone()), - }) + Some(state.summary()) }Add to
impl OutputState:fn summary(&self) -> OutputTerminalSummary { OutputTerminalSummary { stdout: OutputStreamSummary { enabled: self.stdout.enabled, total_bytes: self.stdout.total_bytes, }, stderr: OutputStreamSummary { enabled: self.stderr.enabled, total_bytes: self.stderr.total_bytes, }, reader_failure: self.failure.as_ref().map(|failure| failure.message.clone()), } }🤖 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 `@src/guest/src/service/exec/output.rs` around lines 253 - 294, Add a private OutputState::summary helper containing the shared OutputTerminalSummary construction, then update terminal_summary and sealed_terminal_summary to retain their existing precondition checks and return state.summary().src/guest/src/service/exec/mod.rs (1)
306-321: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCouple the channel capacity to the event count.
The capacity 2 at Line 309 is correct only because
output::terminal_eventsemits at most one event per stream. That bound lives insrc/guest/src/service/exec/output.rsLines 450-459. If a third terminal event is ever added,try_sendreturnsFulland theexpectat Line 316 panics on a request path.Build the events first and size the channel from their count.
♻️ Proposed refactor
fn terminal_output_receiver( summary: output::OutputTerminalSummary, ) -> mpsc::Receiver<Result<ExecOutput, Status>> { - let (tx, rx) = mpsc::channel(2); if let Some(failure) = summary.reader_failure { + let (tx, rx) = mpsc::channel(1); tx.try_send(Err(Status::internal(failure))) .expect("terminal attach receiver must be live"); - } else { - for event in output::terminal_events(&summary) { - tx.try_send(Ok(event)) - .expect("terminal attach receiver must be live"); - } + return rx; + } + let events = output::terminal_events(&summary); + let (tx, rx) = mpsc::channel(events.len().max(1)); + for event in events { + tx.try_send(Ok(event)) + .expect("terminal attach receiver must be live"); } rx }🤖 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 `@src/guest/src/service/exec/mod.rs` around lines 306 - 321, Update terminal_output_receiver to build the terminal events before creating the channel, then size the channel capacity from the resulting event count; preserve the reader_failure branch and existing try_send behavior while ensuring all events can be queued without expect panicking due to a full channel.src/guest/src/service/exec/registry.rs (2)
367-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicate test-only prune wrappers.
prune_atandprune_for_testare both#[cfg(test)], andprune_for_testonly forwards toprune_at. Keep one name.♻️ Proposed refactor
#[cfg(test)] - async fn prune_at(&self, now: Instant) { + async fn prune_for_test(&self, now: Instant) { Self::prune_inner(&self.inner, now).await; }Then remove the separate
prune_for_testwrapper at Lines 409-412.Also applies to: 409-412
🤖 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 `@src/guest/src/service/exec/registry.rs` around lines 367 - 371, Collapse the duplicate test-only wrappers by keeping a single #[cfg(test)] method, preferably prune_at, and removing prune_for_test and its forwarding call. Update all test call sites to use the retained method name.
323-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning every retained entry on the common no-eviction path.
Lines 323-342 clone the id, the
ExecutionState, and the wholeTerminalSnapshotfor every retained entry on each call, while the registry mutex is held.TerminalSnapshotcarries two diagnosticStringvalues, andMAX_RETAINED_ENTRIESis 64, so up to 64 snapshot clones run per completed execution. In the common case the limits are not exceeded and every clone is discarded.Collect only
(id, retained_bytes, last_access)for the sort, then remove and clone the full entry for the ids that are actually evicted.♻️ Proposed refactor sketch
- let mut retained: Vec<_> = inner - .entries - .iter() - .filter_map(|(id, entry)| match entry { - ExecutionEntry::Retained { - state, - snapshot, - retained_bytes, - last_access, - .. - } => Some(( - id.clone(), - state.clone(), - snapshot.clone(), - *retained_bytes, - *last_access, - )), - _ => None, - }) - .collect(); - retained.sort_by_key(|(_, _, _, _, last_access)| *last_access); - let mut total_bytes: usize = - retained.iter().map(|(_, _, _, bytes, _)| *bytes).sum(); + let mut retained: Vec<_> = inner + .entries + .iter() + .filter_map(|(id, entry)| match entry { + ExecutionEntry::Retained { + retained_bytes, + last_access, + .. + } => Some((id.clone(), *retained_bytes, *last_access)), + _ => None, + }) + .collect(); + retained.sort_by_key(|(_, _, last_access)| *last_access); + let mut total_bytes: usize = retained.iter().map(|(_, bytes, _)| *bytes).sum(); let mut evicted = Vec::new(); while retained.len() > MAX_RETAINED_ENTRIES || total_bytes > MAX_RETAINED_BYTES { - let (id, state, snapshot, bytes, _) = retained.remove(0); + let (id, bytes, _) = retained.remove(0); total_bytes -= bytes; + let Some(ExecutionEntry::Retained { state, snapshot, .. }) = + inner.entries.remove(&id) + else { + continue; + }; let access = next_access(&mut inner); inner.entries.insert( id, tombstone_entry(snapshot, Instant::now() + TOMBSTONE_TTL, access), ); evicted.push(state); }🤖 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 `@src/guest/src/service/exec/registry.rs` around lines 323 - 357, Update the retained-entry eviction logic to collect only each entry’s id, retained_bytes, and last_access for sorting, avoiding clones of ExecutionState and TerminalSnapshot on the no-eviction path. After determining eviction candidates in the existing while loop, remove each selected entry from inner.entries and clone the state and snapshot only for entries actually evicted, then create the tombstone and preserve the existing byte and tombstone-limit behavior.
🤖 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 `@src/guest/src/reaper.rs`:
- Around line 1133-1149: Serialize all three real-child tests with the shared
test guard to prevent concurrent interaction with REAP_LOCK: in
src/guest/src/reaper.rs lines 1133-1149, acquire the guard before reap_fence()
in signal_leader_terminates_the_live_claimed_process; in
src/guest/src/service/exec/state.rs lines 689-719, wrap child.wait() in
reap_fence() and execute it via spawn_blocking; in
src/guest/src/service/exec/timeout.rs lines 90-123, add the same guard while
retaining its existing reap_fence() and spawn_blocking usage.
In `@src/guest/src/service/exec/mod.rs`:
- Around line 441-463: In the execution registry branch following the
reservation publish path, replace the nested `else { if ... }` around
`server.registry.register` with an `else if` or equivalent single conditional
expression. Preserve the existing shutdown error response and
`state.abort_unpublished().await` behavior when registration fails.
In `@src/guest/src/service/exec/registry.rs`:
- Around line 1129-1162: Update
shutdown_releases_live_and_retained_state_resources to avoid assigning
fabricated live PIDs that shutdown_all may signal on the host. Use
guaranteed-nonexistent PIDs for the settled_state handles, or invoke
release_remaining_states directly while preserving the assertions that both
states release their resources and registry entries.
- Around line 520-557: Update shutdown_all to route both SIGTERM and SIGKILL
through each execution state’s ExitClaimOwner and Reaper::signal_leader_if_live,
matching the behavior used by TimeoutTarget::signal_if_live. Remove raw
state.get_pid() and kill(pid, None) probing for signal decisions, while
preserving the existing wait loop and graceful-exit handling.
- Around line 448-470: Update observe_terminal so wait_terminal_output_summary
is bounded by the execution’s existing deadline or timeout mechanism after the
leader exits, while preserving the normal completed-summary path. Ensure a stuck
stdout/stderr reader cannot leave the ExecutionEntry in Live indefinitely and
that the flow still reaches retain with the available output, producing Retained
or Tombstone as appropriate.
In `@src/guest/src/service/exec/state.rs`:
- Around line 278-293: Update wait_terminal_output_summary to await
output.seal() unconditionally, bind its boolean result, and apply debug_assert!
to that bound result so release builds still seal the OutputManager.
---
Outside diff comments:
In `@src/guest/src/service/exec/state.rs`:
- Around line 416-462: Update attach_output to safely displace any existing
inner.output_task instead of asserting it is absent. While holding the lock,
replace the previous handle with the newly spawned task, then release the lock
and await the displaced handle before returning or completing the attachment,
preserving the released-state cleanup and HandleUnavailable behavior.
---
Nitpick comments:
In `@src/guest/src/service/exec/mod.rs`:
- Around line 306-321: Update terminal_output_receiver to build the terminal
events before creating the channel, then size the channel capacity from the
resulting event count; preserve the reader_failure branch and existing try_send
behavior while ensuring all events can be queued without expect panicking due to
a full channel.
In `@src/guest/src/service/exec/output.rs`:
- Around line 253-294: Add a private OutputState::summary helper containing the
shared OutputTerminalSummary construction, then update terminal_summary and
sealed_terminal_summary to retain their existing precondition checks and return
state.summary().
In `@src/guest/src/service/exec/registry.rs`:
- Around line 367-371: Collapse the duplicate test-only wrappers by keeping a
single #[cfg(test)] method, preferably prune_at, and removing prune_for_test and
its forwarding call. Update all test call sites to use the retained method name.
- Around line 323-357: Update the retained-entry eviction logic to collect only
each entry’s id, retained_bytes, and last_access for sorting, avoiding clones of
ExecutionState and TerminalSnapshot on the no-eviction path. After determining
eviction candidates in the existing while loop, remove each selected entry from
inner.entries and clone the state and snapshot only for entries actually
evicted, then create the tombstone and preserve the existing byte and
tombstone-limit behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a51d5bb4-bde3-477d-a31c-d445aa547607
📒 Files selected for processing (7)
src/guest/src/reaper.rssrc/guest/src/service/container.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/output.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
Review dispositionAll six inline findings plus the outside-diff one are handled in 9db68d1 / 2ace576. Per-thread replies are inline; the two findings with no inline thread are covered here. Displaced let displaced = inner.output_task.replace(task);
// ... lock released ...
if let Some(displaced) = displaced { let _ = displaced.await; }The join is bounded, which is why it is safe inside the handler:
Deliberately out of scope: the unbounded terminal-summary wait ( Stated plainly, three parts carry no test: the Verification: 9db68d1 went green on all three Clippy platforms and all three Rust Tests platforms; guest unit tests are 305/305 on Linux, with the two new shutdown/kill tests demonstrated red-then-green against a full revert of the production changes. 2ace576 is a one-line |
2ace576 to
fc46370
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@src/guest/src/service/container.rs`:
- Around line 486-499: Update the failed registration branch in the container
initialization flow to pass state.clone() into registry.register, then await
state.abort_unpublished() before returning the error response. Preserve
reaper.release_slot(&exit) and the existing response behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7295d341-b2e1-47aa-bc8b-716d563366f9
📒 Files selected for processing (6)
src/guest/src/service/container.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/output.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/guest/src/service/exec/output.rs
- src/guest/src/service/exec/state.rs
- src/guest/src/service/exec/mod.rs
- src/guest/src/service/exec/registry.rs
An execution's registry entry disappeared as soon as its output stream ended, so a Wait or Attach arriving afterwards found nothing and the caller could not tell "never existed" from "already finished". The registry now holds three states per execution. Live behaves as before. Once an execution's exit and output summary are both in, it becomes Retained and its buffered output stays readable. After the retain grace it degrades to a Tombstone holding only the classified exit and a truncated diagnosis. Retention is bounded on grace, TTL, entry count and retained bytes, with LRU eviction, so a long-lived box cannot grow the registry without limit. Reservations close the window between spawn and register: an execution that cannot be published is SIGKILLed and torn down instead of escaping as an orphan. The timeout watcher now returns its handle so a session that already exited cancels it rather than leaving a task parked on a deadline.
fc46370 to
31bfe5d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
A finished execution loses two things it should still be able to answer for. Its output is gone the moment the stream ends — a late
Attachgets an empty stream over pipes already at EOF, not the bytes the execution produced. And its exit is only classifiable once, because the container-death diagnosis drains init's pipes, so a repeatWaitgets a different answer than the first. Meanwhile the entry itself is never removed at all: only the SSH bridges callrelease_ephemeral, so every SDK exec the guest ever ran stays in the map.This gives an execution three lifecycle states. It stays readable for a bounded window after it exits, then decays to a small record, then goes away.
Call graph
Before
After
Changes
ExecutionRegistryentries becomeLive→Retained→Tombstone. Retained keeps the buffered output readable; a tombstone keeps only the classified exit and a truncated diagnosis. Retention is bounded on grace, TTL, entry count and retained bytes, with LRU eviction, so the registry can no longer grow without limit.ExecutionStateclassifies its exit once into aOnceCell, which is what makes a repeatWaitreturn the same container-death diagnosis instead of an emptied one.How to verify
303 tests pass. The retention behavior is covered by
terminal_observer_retains_a_completed_live_execution,retained_entry_exposes_its_terminal_snapshot_before_tombstoning, anda_tombstone_keeps_its_terminal_snapshot_repeatable; the bounds by the grace/TTL/LRU/byte-cap eviction tests inregistry.rs; the repeatability fix byrepeated_wait_caches_the_init_exit_diagnosis, which countsdiagnose_exitcalls and fails onmainbecause the secondWaitre-drains init's pipes.Risks / rollout
Retained output is memory-bounded and expires. After eviction, terminal RPCs answer from the tombstone and then return NotFound — a caller that waits longer than the tombstone TTL sees a not-found where it previously saw a stale live entry.
Rebased onto #1185, which was split out of this branch and merged first. That PR's claim/token design was replaced during review by a
ProcessInstanceidentity check, so this branch was re-ported onto the merged design rather than rebased hunk-by-hunk; it no longer touchesreaper.rsorexec_handle.rsat all.One gap is left open deliberately:
release_ephemeralreturning the reaper slot has no unit test. That path goes through the globalREAPER, whichmainalso leaves untested, and making it testable means re-adding API surface this branch does not otherwise need.Summary by CodeRabbit
New Features
Bug Fixes