fix(exec): signal executions through a verifiable exit claim - #1185
Conversation
An execution's registry entry is only ever removed by release_ephemeral, which just the SSH paths call, so an SDK execution outlives its process for the life of the guest. Its ExecutionState keeps the ExecHandle, and with it the leader pid, indefinitely. ExecHandle::kill then sent a bare kill(pid, signal) with nothing checking the pid still belonged to that execution, so once the kernel recycled the number, Kill on a finished execution would signal an unrelated process. shutdown_all had the same shape one level up: it probed with kill(pid, None) and signalled after, leaving a window in which the pid could change owner between the two. Give every spawn a claim token instead. The reaper now hands out an ExitClaim -- pid plus a monotonic token -- and signals only while that exact token still owns a live, unsettled slot, holding the reap fence across the check so delivery and recycling cannot land in between. ExecutionState routes single-pid kills and shutdown through the claim, and shutdown's wait loop polls claim liveness rather than kill(pid, None). Container init carries the claim as a signal target only: the container lifecycle owns it and runs its own SIGTERM/SIGKILL, so registry shutdown must leave init alone while an explicit Kill may still reach it. An explicit role on the claim expresses that, rather than two same-typed Options a call site could transpose. Kill with process_group still reaches kill(-pid) through the handle's own group-leader check, which is not claim-guarded; closing that needs a claim-guarded group primitive on Reaper and is left out here.
📦 BoxLite review — couldn't completepowered by BoxLite |
|
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:
📝 WalkthroughWalkthroughThe PR adds PID start-time validation through ChangesProcess lifecycle safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionStartup
participant ProcessInstance
participant Reaper
participant ExecutionState
participant TimeoutTarget
ExecutionStartup->>ProcessInstance: capture leader PID and start time
ExecutionStartup->>Reaper: register PID and spawn timestamp
Reaper-->>ExecutionStartup: return ExitSlot
ExecutionStartup->>ExecutionState: store ProcessInstance and ExitSlot
ExecutionState->>ProcessInstance: signal current process
ProcessInstance-->>ExecutionState: return signal result
ExecutionStartup->>TimeoutTarget: create from ProcessInstance
TimeoutTarget->>ProcessInstance: send SIGTERM or SIGKILL
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/guest/src/reaper.rs (6)
301-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping
asyncfrom the claim registry methods.
register_claim,register, andrelease_claimno longer await anything. They lock astd::sync::Mutexsynchronously. Theasyncsignature suggests a suspension point that does not exist, and it forces every caller into an async context.This change touches call sites in
src/guest/src/service/exec/mod.rsandsrc/guest/src/service/exec/state.rs, so it can also be deferred to a follow-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 `@src/guest/src/reaper.rs` around lines 301 - 330, Remove async from the claim registry methods register_claim, register, and release_claim, since they perform only synchronous mutex operations and no awaits. Update their callers in service exec mod.rs and state.rs to call them synchronously while preserving the existing return values and claim/release behavior.
1176-1193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact signal that reached the child.
status.code()returnsNonefor death by any signal. The test therefore passes if a SIGKILL from elsewhere reaches the child. The equivalent test insrc/guest/src/service/exec/timeout.rs(lines 99-120) asserts the signal number, which is stronger.💚 Proposed change to pin the signal
+ use std::os::unix::process::ExitStatusExt; + assert!(r .signal_leader_if_live(&claim, leader, Signal::SIGTERM) .expect("signal the live leader")); let _fence = reap_fence(); let status = child.wait().expect("wait for terminated child"); - assert_eq!(status.code(), None); + assert_eq!(status.signal(), Some(Signal::SIGTERM as i32));🤖 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/reaper.rs` around lines 1176 - 1193, Strengthen signal_leader_terminates_the_live_claimed_process by asserting that the waited child status indicates termination specifically by SIGTERM, not merely any signal. Use the platform-appropriate signal-status accessor and compare it with SIGTERM while preserving the existing wait and termination flow.
346-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
leader_pid != claim.pidmismatch instead of returningOk(false)silently.A mismatch between
leader_pidandclaim.pidis a wiring defect, not a dead process. The current code returns the sameOk(false)for both cases. Callers insrc/guest/src/service/exec/state.rsandsrc/guest/src/service/exec/timeout.rscannot tell them apart, so a mis-wired target silently stops signalling.🔍 Proposed change to separate the two cases
let _fence = reap_fence(); - let is_live = leader_pid == claim.pid - && self + if leader_pid != claim.pid { + warn!( + leader = leader_pid.as_raw(), + claim = claim.pid.as_raw(), + "reaper: signal target does not match its claim; refusing to signal" + ); + return Ok(false); + } + let is_live = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) .slots .get(&claim.pid) .is_some_and(|slot| { slot.claim_token == Some(claim.token) && slot.settled_at.is_none() });🤖 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/reaper.rs` around lines 346 - 375, Update signal_leader_if_live to detect leader_pid != claim.pid separately from the live-slot check and log this mismatch before returning Ok(false). Preserve the existing silent false result for claims whose slot is no longer live, and keep signal delivery and ESRCH handling unchanged.
1165-1174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes even without the
settled_atguard.Pid
2_002is fabricated.killanswersESRCHfor it, sosignal_leader_if_livereturnsOk(false)whether or not the settled check runs. The test does not bind the guard it names.Apply the same technique used at lines 1120-1122: spawn a real child, claim it, call
deliverto settle the claim while the process is still alive, then assert the refusal and assert the child is still running.🤖 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/reaper.rs` around lines 1165 - 1174, Update signal_leader_refuses_a_settled_claim to spawn a real child process and use its PID for registration, following the setup pattern around lines 1120-1122. Deliver the claim while the child remains alive, then assert signal_leader_if_live refuses the settled claim and separately verify the child is still running before cleaning it up.
377-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
claim_is_liveis advisory.
claim_is_livedoes not take the reap fence. The result can change before the caller acts on it.signal_leader_if_liveholds the fence across the check and thekill, so it stays correct. Add a doc comment that states callers must not useclaim_is_liveas a precondition for a separate signal call, or the guard this PR introduces is bypassed.🤖 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/reaper.rs` around lines 377 - 384, Add a Rust doc comment for Reaper::claim_is_live stating that its result is advisory and may change before use; callers must not treat it as a precondition for a separate signal call, and should use signal_leader_if_live to retain the reap fence across validation and signaling.
386-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared claim-creation block into one helper.
Lines 406-417 repeat the recycle guard, the token increment, and the slot setup from
register_claim(lines 313-324) exactly. Both blocks carry the same PID-reuse correctness rule. A later change to one block can silently leave the other stale.♻️ Proposed helper on `Inner`
impl Inner { + /// Drop a slot left by a previous owner of this pid, then claim it with a + /// fresh token. Returns the token and the claimed slot. + fn claim(&mut self, pid: Pid, spawned_at: Instant) -> (u64, &mut Slot) { + if self + .slots + .get(&pid) + .is_some_and(|slot| slot.settled_at.is_some_and(|at| at < spawned_at)) + { + self.slots.remove(&pid); + } + self.next_claim_token = self.next_claim_token.wrapping_add(1).max(1); + let token = self.next_claim_token; + let slot = self.slot(pid, None); + slot.stray_since = None; + slot.claim_token = Some(token); + (token, slot) + } + /// Get or create this pid's slot. fn slot(&mut self, pid: Pid, stray_since: Option<Instant>) -> &mut Slot {Then both
register_claimandon_exitcallinner.claim(pid, spawned_at).🤖 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/reaper.rs` around lines 386 - 436, Extract the duplicated recycle guard, claim-token generation, and slot initialization from register_claim and on_exit into an Inner::claim(pid, spawned_at) helper. Have both methods call this helper and use its returned slot/token state, preserving the existing PID-reuse handling and claim semantics.src/guest/src/service/exec/state.rs (1)
426-475: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConfirm the group-kill path stays unguarded on purpose.
killguards only the single-PID path with the claim. Theprocess_grouppath still callshandle.kill_process_group(signal). The PR objectives state that group signalling is deferred. The inline comment records the same limitation. No change is required in this PR.🤖 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 426 - 475, Keep the process_group branch of ProcessState::kill unchanged and unguarded, continuing to call handle.kill_process_group(signal); only the single-PID path should use signal_leader_if_live for claim validation.
🤖 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.
Nitpick comments:
In `@src/guest/src/reaper.rs`:
- Around line 301-330: Remove async from the claim registry methods
register_claim, register, and release_claim, since they perform only synchronous
mutex operations and no awaits. Update their callers in service exec mod.rs and
state.rs to call them synchronously while preserving the existing return values
and claim/release behavior.
- Around line 1176-1193: Strengthen
signal_leader_terminates_the_live_claimed_process by asserting that the waited
child status indicates termination specifically by SIGTERM, not merely any
signal. Use the platform-appropriate signal-status accessor and compare it with
SIGTERM while preserving the existing wait and termination flow.
- Around line 346-375: Update signal_leader_if_live to detect leader_pid !=
claim.pid separately from the live-slot check and log this mismatch before
returning Ok(false). Preserve the existing silent false result for claims whose
slot is no longer live, and keep signal delivery and ESRCH handling unchanged.
- Around line 1165-1174: Update signal_leader_refuses_a_settled_claim to spawn a
real child process and use its PID for registration, following the setup pattern
around lines 1120-1122. Deliver the claim while the child remains alive, then
assert signal_leader_if_live refuses the settled claim and separately verify the
child is still running before cleaning it up.
- Around line 377-384: Add a Rust doc comment for Reaper::claim_is_live stating
that its result is advisory and may change before use; callers must not treat it
as a precondition for a separate signal call, and should use
signal_leader_if_live to retain the reap fence across validation and signaling.
- Around line 386-436: Extract the duplicated recycle guard, claim-token
generation, and slot initialization from register_claim and on_exit into an
Inner::claim(pid, spawned_at) helper. Have both methods call this helper and use
its returned slot/token state, preserving the existing PID-reuse handling and
claim semantics.
In `@src/guest/src/service/exec/state.rs`:
- Around line 426-475: Keep the process_group branch of ProcessState::kill
unchanged and unguarded, continuing to call handle.kill_process_group(signal);
only the single-PID path should use signal_leader_if_live for claim validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0d4a480-e032-47fe-8542-e7afcd1adf2a
📒 Files selected for processing (7)
src/guest/src/reaper.rssrc/guest/src/service/container.rssrc/guest/src/service/exec/exec_handle.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
💤 Files with no reviewable changes (1)
- src/guest/src/service/exec/exec_handle.rs
signal_leader_if_live answers Ok(false) both when the claim check refuses and when kill reports ESRCH, so a test that signals a fabricated pid cannot tell the two apart. signal_leader_refuses_a_settled_claim used pid 2002 and passed with the settled_at guard deleted; it now signals a real child and asserts the child survives, which the guard is the only thing preventing. signal_leader_terminates_the_live_claimed_process asserted status.code() is None, which holds for death by any signal. It now pins SIGTERM. Drops ExitClaim::pid, whose only caller was the fabricated-pid assertion.
|
Thanks — two of the seven are acted on in 505a8af, and one of them was the useful catch. Acted on
Not in this PR
|
|
Alternative considered: the pre-pidfd identity classic — capture It solves the identity half more cheaply than the token: one
Where it may earn its keep: the declared |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/guest/src/service/exec/state.rs (2)
399-403: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
killdiscards the errno.
unwrap_or(false)maps everyErrnoto the same result as "the process already exited". A permission failure or an invalid signal is then indistinguishable from a normal exit for the caller and leaves no log entry.shutdown_allinregistry.rsalready logs its errors; makekillconsistent.♻️ Proposed change
pub async fn kill(&self, signal: nix::sys::signal::Signal, process_group: bool) -> bool { - self.signal_if_current(signal, process_group) - .await - .unwrap_or(false) + match self.signal_if_current(signal, process_group).await { + Ok(sent) => sent, + Err(error) => { + tracing::warn!(%error, ?signal, process_group, "kill signal failed"); + false + } + } }🤖 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 399 - 403, Update State::kill to preserve and log errors returned by signal_if_current instead of collapsing every Errno through unwrap_or(false). Make its error handling consistent with shutdown_all in registry.rs, while retaining the existing false result for successfully determining that the process has already exited.
536-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
ProcessIdentitytests intoidentity.rs.These three tests exercise
ProcessIdentity::signaldirectly. They do not construct anExecutionState.identity.rsalready owns a test module for the same type, andprocess_group_kill_refuses_a_changed_start_timeoverlaps withprocess_group_signal_refuses_a_non_leaderthere. Keeping the identity tests in one module makes the coverage of the guard easier to read.A test that drives
ExecutionState::killandsignal_owned_process_if_currentagainst a stale identity would be a better fit for this file, because those wrappers add thereleasedandshutdown_managedgates that the current tests do not cover.🤖 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 536 - 642, Move the three tests process_identity_refuses_a_changed_start_time, process_identity_signals_the_matching_process, and process_group_kill_refuses_a_changed_start_time into identity.rs alongside the existing ProcessIdentity tests, removing them from the current module. Preserve their assertions and setup, and leave this file focused on ExecutionState wrapper behavior such as released and shutdown_managed gating.src/guest/src/service/exec/identity.rs (2)
47-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a dead process from a transient
/procread failure.
start_time_formaps every error toNone.is_currentthen returnsfalseandsignalreportsOk(false), which callers read as "the process already exited". A transient failure (for example, EMFILE or ENOMEM while opening/proc/<pid>/stat) therefore suppresses SIGTERM and the later SIGKILL escalation intimeout.rs, and the process keeps running with no error logged.Consider returning a
Resultso that "no such process" and "cannot read" are separate outcomes, and log or propagate the second case.🤖 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/identity.rs` around lines 47 - 53, Update start_time_for and its callers, including is_current and signal, to return or propagate a Result that distinguishes a missing/dead process from transient /proc access failures. Preserve the existing false/Ok(false) behavior only for confirmed process absence; propagate or log other errors so timeout.rs can continue SIGTERM/SIGKILL escalation instead of treating read failures as an exited process.
20-41: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftUse a pidfd-backed signal path with an explicit kernel baseline.
is_current()followed bykill()still has a PID-reuse race. The guest locksnix0.29.0, which does not expose the pidfd APIs. Use raw syscalls or upgradenix. Process-group signaling throughpidfd_send_signalrequires Linux 6.9. Define the guest kernel baseline and handle older kernels explicitly.🤖 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/identity.rs` around lines 20 - 41, Update Identity::signal to use pidfd-backed signaling instead of the is_current/kill sequence, using raw syscalls or an upgraded nix version that exposes pidfd APIs. Preserve process-group targeting, define the minimum supported guest kernel version required for pidfd process-group signaling (Linux 6.9), and explicitly handle older kernels with the intended fallback or error 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/service/container.rs`:
- Around line 372-375: Update the init identity-registration flow around
ProcessIdentity::capture(handle.pid()) in the container service and the
corresponding flow in exec::mod.rs to detect a None result and emit a warning.
Preserve the existing registration behavior, but make the capture failure
explicit in logs so failed init identity capture is diagnosable.
In `@src/guest/src/service/exec/mod.rs`:
- Around line 320-327: Log a warning whenever process identity capture returns
None so failed signalling is diagnosed: in src/guest/src/service/exec/mod.rs
lines 320-327, update the execution setup around ProcessIdentity::capture to
include the execution ID and PID; in src/guest/src/service/container.rs lines
372-375, add the same warning for the init handle including the container ID;
and in src/guest/src/service/exec/timeout.rs lines 33-38, log from the None arm
of signal_if_live while preserving its Ok(false) result.
---
Nitpick comments:
In `@src/guest/src/service/exec/identity.rs`:
- Around line 47-53: Update start_time_for and its callers, including is_current
and signal, to return or propagate a Result that distinguishes a missing/dead
process from transient /proc access failures. Preserve the existing
false/Ok(false) behavior only for confirmed process absence; propagate or log
other errors so timeout.rs can continue SIGTERM/SIGKILL escalation instead of
treating read failures as an exited process.
- Around line 20-41: Update Identity::signal to use pidfd-backed signaling
instead of the is_current/kill sequence, using raw syscalls or an upgraded nix
version that exposes pidfd APIs. Preserve process-group targeting, define the
minimum supported guest kernel version required for pidfd process-group
signaling (Linux 6.9), and explicitly handle older kernels with the intended
fallback or error behavior.
In `@src/guest/src/service/exec/state.rs`:
- Around line 399-403: Update State::kill to preserve and log errors returned by
signal_if_current instead of collapsing every Errno through unwrap_or(false).
Make its error handling consistent with shutdown_all in registry.rs, while
retaining the existing false result for successfully determining that the
process has already exited.
- Around line 536-642: Move the three tests
process_identity_refuses_a_changed_start_time,
process_identity_signals_the_matching_process, and
process_group_kill_refuses_a_changed_start_time into identity.rs alongside the
existing ProcessIdentity tests, removing them from the current module. Preserve
their assertions and setup, and leave this file focused on ExecutionState
wrapper behavior such as released and shutdown_managed gating.
🪄 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: 7ddec03e-ac10-4425-81ab-29e9726a325a
📒 Files selected for processing (8)
src/guest/src/reaper.rssrc/guest/src/service/container.rssrc/guest/src/service/exec/exec_handle.rssrc/guest/src/service/exec/identity.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
💤 Files with no reviewable changes (1)
- src/guest/src/service/exec/exec_handle.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/guest/src/service/exec/registry.rs
| start_time: u64, | ||
| } | ||
|
|
||
| impl ProcessIdentity { |
There was a problem hiding this comment.
there is already a ProcessIdentity in this repo. you could move it to the shared util dir so guest can reuse it
There was a problem hiding this comment.
Checked this: the existing ProcessIdentity is the host-side PID-file validation enum (Verified / Legacy / Absent), while the guest type stores a PID/start-time fingerprint and validates signal delivery. They are not a drop-in shared abstraction. I renamed the guest type to ProcessInstance to remove the ambiguity; extracting a shared PID/start-time helper would broaden host/guest platform dependencies without another consumer.
There was a problem hiding this comment.
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/identity.rs (1)
165-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify the descendant before
SIGKILLescalation.At Line 170, the test checks only the group leader. It does not prove that the background descendant received
SIGTERM; the laterSIGKILLcheck can still pass ifSIGTERMgroup delivery regresses. Make the fixture’s descendant terminate onSIGTERM, then assertis_gone_or_zombie(descendant)before Line 172.🤖 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/identity.rs` around lines 165 - 170, Update the process-group termination test around ProcessSignalTarget::capture and the descendant fixture so the background descendant exits when it receives SIGTERM. After the SIGTERM signal and brief wait, assert is_gone_or_zombie(descendant) before proceeding to SIGKILL escalation, while retaining the existing group-leader check.
🤖 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.
Outside diff comments:
In `@src/guest/src/service/exec/identity.rs`:
- Around line 165-170: Update the process-group termination test around
ProcessSignalTarget::capture and the descendant fixture so the background
descendant exits when it receives SIGTERM. After the SIGTERM signal and brief
wait, assert is_gone_or_zombie(descendant) before proceeding to SIGKILL
escalation, while retaining the existing group-leader check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fdf6778-0454-4404-8ffd-89e5fe8fad10
📒 Files selected for processing (5)
src/guest/src/service/container.rssrc/guest/src/service/exec/identity.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/guest/src/service/exec/mod.rs
- src/guest/src/service/container.rs
- src/guest/src/service/exec/state.rs
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/exec/state.rs`:
- Around line 381-388: Update the method containing the released check and
ProcessInstance::signal call to retain the inner mutex guard through the signal
operation. Acquire the guard once, check released and obtain the process from
that guarded state, then call process.signal before releasing the guard,
preserving the existing false returns.
🪄 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: 8debb0b1-9f39-49dc-bc28-2584cfda8220
📒 Files selected for processing (5)
src/guest/src/service/container.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/process_instance.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/exec/timeout.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/guest/src/service/container.rs
## 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 `Attach`
gets 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 repeat `Wait` gets a
different answer than the first. Meanwhile the entry itself is never
removed at all: only the SSH bridges call `release_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
```text
attach_execution (GuestServer · src/guest/src/service/exec/mod.rs:66)
└─ get (ExecutionRegistry · registry.rs:44) ← BUG: nothing removes SDK entries; the map grows for the life of the guest
└─ attach (ExecutionState · state.rs:293) ← BUG: streams the live pipes, already at EOF — the produced output is unreachable
wait_execution (GuestServer · mod.rs:94)
└─ wait_exit (ExecutionState · state.rs:253)
└─ check_container_death (state.rs:144) ← BUG: diagnose_exit drains init's pipes, so the second Wait answers differently
```
After
```text
observe_terminal (ExecutionRegistry · registry.rs:539) — one task per SDK exec
└─ wait_exit (ExecutionState · state.rs:335)
└─ cancel_timeout_task (state.rs:306) — the deadline is moot once the process is gone
└─ wait_terminal_output_summary (state.rs:271) — drains, then seals the buffer
└─ retain (registry.rs:344) — Live → Retained; its cap eviction spares a session with a reader
↓ retain grace · tombstone TTL · entry cap · byte cap · LRU
└─ prune_inner (registry.rs:443) — Retained → Tombstone → removed, deferred while a reader streams
attach_execution (mod.rs:67)
└─ lookup (registry.rs:218) — Live | Retained | Tombstone | absent
├─ Live → attach (state.rs:382)
├─ Retained → attach_retained (state.rs:389) — replays the buffered bytes
└─ Tombstone → terminal_output_receiver(snapshot.output) — summary only
wait_execution (mod.rs:118)
└─ wait_exit (state.rs:335)
└─ terminal_exit OnceCell → classify_exit (state.rs:342) — classified once; every later Wait reads that same result
```
## Changes
- `ExecutionRegistry` entries become `Live` → `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.
- `ExecutionState` classifies its exit once into a `OnceCell`, which is
what makes a repeat `Wait` return the same container-death diagnosis
instead of an emptied one.
- Terminal output is drained into a summary and then sealed, so a
retained session replays bytes rather than re-reading dead pipes. A
displaced forwarder is joined instead of leaked. Neither retirement path
— cap eviction nor grace expiry — drops a session while a reader is
streaming it, since aborting that forwarder would truncate the replay
indistinguishably from a normal end of output.
- Shutdown signals the retention pruner and waits for it instead of
aborting it: `prune_inner` tombstones entries under the lock and
releases their resources after dropping it, so an abort in between left
an entry tombstoned while its resources were still live.
- A reservation is taken before spawn and published after, closing the
window where an execution that fails to register escapes as an orphan;
one that cannot be published is SIGKILLed and torn down.
- The timeout watcher returns its handle so a session that exits first
cancels it, rather than leaving a task parked on a deadline.
- Execution IDs are issued by the guest; a caller-supplied id is
rejected on the normal exec path.
## How to verify
```bash
make test:unit:guest
```
318 tests pass. The retention behavior is covered by
`terminal_observer_retains_a_completed_live_execution`,
`retained_entry_exposes_its_terminal_snapshot_before_tombstoning`, and
`a_tombstone_keeps_its_terminal_snapshot_repeatable`; the bounds by the
grace/TTL/LRU/byte-cap eviction tests in `registry.rs`; the
repeatability fix by `repeated_wait_caches_the_init_exit_diagnosis`,
which counts `diagnose_exit` calls and fails on `main` because the
second `Wait` re-drains init's pipes; and the reader-versus-retirement
rule by `eviction_spares_a_retained_session_with_an_active_reader` and
`grace_expiry_spares_a_retained_session_with_an_active_reader`, one per
retirement path.
## 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
`ProcessInstance` identity check, so this branch was re-ported onto the
merged design rather than rebased hunk-by-hunk; it no longer touches
`reaper.rs` or `exec_handle.rs` at all.
Two gaps are left open deliberately.
`release_ephemeral` returning the reaper slot has no unit test. That
path goes through the global `REAPER`, which `main` also leaves
untested, and making it testable means re-adding API surface this branch
does not otherwise need.
Neither retirement path drops a retained session while a reader is
streaming it — the caps skip it and the grace defers it — but the two
paths that still abort a forwarder, `release_ephemeral` and shutdown,
give the client no signal: the stream simply ends, which is
indistinguishable from a normal end of output. Both are cases where the
caller knowingly tore the session down, so this is a missing diagnostic
rather than a surprise. Supplying one needs `release_resources` to
finish cooperatively so a `Status` can go out over the existing
`Result<ExecOutput, Status>` channel, which changes the contract for all
four of its callers; that belongs in its own change rather than here.
Co-authored-by: BatmanByte <300328404+BatmanByte@users.noreply.github.com>
PR boxlite-ai#1185 guarded every signal path with a /proc/<pid>/stat start-time fingerprint before kill(pid), but the comparison and the kill are not kernel-atomic: a PID can recycle between them and the kill lands on the new owner. The issue's kernel repro reached it in ~9.5 minutes of forks. Capture a pidfd at spawn (pidfd_open, Linux 5.3+) and signal via pidfd_send_signal, which is kernel-atomic: ESRCH (or EPERM on some configurations) once the incarnation is gone, never signals a recycled owner. Applies to both the single-pid path and the group path's leader probe (pidfd_send_signal(fd, 0) before getpgid + kill(-pgid)). On EPERM, re-check the /proc start-time to distinguish a reaped incarnation (the kernel returns EPERM, not the documented ESRCH, for a reaped pidfd under the Rust runtime on kernel 5.4) from a real permission denial (child setuid'd to a different user after capture). The /proc start-time path remains the ENOSYS fallback on kernels without pidfd_open. Fixes boxlite-ai#1184 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
2 participants
Summary
Use a
/proc/<pid>/statstart-time fingerprint to avoid signalling a later process that reused an exec PID. This replaces the reaper-coupled ExitClaim signal path with one identity check shared by direct Kill, timeouts, and registry shutdown.Call graph
Before
spawn_execution/ container init → execution state, timeout, or shutdown → numeric PID signal ← BUG: a recycled PID can name a different processAfter
spawn_execution/ container init →ProcessInstance::capture(pid)→ execution state, timeout, or shutdown → compare/procstart time → signal only the matching processSIGCHLD→Reaper::deliver→ExitSlot::get— exit delivery remains separate from process identity.Changes
How to verify
make fmt:check:rustmake guestmake test:unit:gueston LinuxRisks / rollout
/proccomparison andkillare not kernel-atomic, so a PID can still recycle after a successful comparison. Eliminating that residual window requires creation-time pidfd handling and is intentionally outside this PR.Summary by CodeRabbit