Add restart policies and crash recovery - #515
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6465b09 to
d02ee90
Compare
78a9516 to
27e0f55
Compare
27e0f55 to
8133d6a
Compare
📝 WalkthroughWalkthroughThis PR adds in-process crash-restart support with new lifecycle states, restart policies, crash coordination, stable box handles, SDK configuration, status metadata, documentation, and integration tests. ChangesRestart policy feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BoxWatcher
participant CrashCoordinator
participant RuntimeImpl
participant BoxHandle
participant BoxImpl
BoxWatcher->>CrashCoordinator: notify shim death
CrashCoordinator->>RuntimeImpl: process box crash
RuntimeImpl->>RuntimeImpl: evaluate policy and backoff
RuntimeImpl->>BoxImpl: rebuild VM
RuntimeImpl->>BoxHandle: swap current implementation
Possibly related issues
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/litebox/box_impl.rs (1)
747-762: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize health status when health checks are auto-enabled
effective_health_check()can start monitoring viarestart_policy, butinit_health_status()still runs only for an explicithealth_check. That leaveshealth_statusatNoneuntil the first probe updates it. Gate the init oneffective_health_check().is_some()so the persisted state matches the active monitor.🤖 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/boxlite/src/litebox/box_impl.rs` around lines 747 - 762, The health check startup path in box_impl::BoxImpl should initialize health_status whenever monitoring is active, not only when an explicit health_check is configured. Update the logic around effective_health_check() so init_health_status() is called when effective_health_check().is_some(), then keep spawning the health task and storing it in health_check_task as before. This ensures the persisted state is set up consistently for restart_policy-driven health checks as well.
🧹 Nitpick comments (3)
src/boxlite/src/litebox/box_impl.rs (1)
371-379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
abort_health_check()here to avoid duplicating the take/log/abort logic.
abort_health_check()(Lines 181-189) was added for exactly this teardown pattern.stop()reimplements the samehealth_check_task.write().take()+ debug-log +abort()block, which risks drift if one path changes.♻️ Proposed change
- // Cancel health check task first (if running) - // This prevents the task from continuing after stop() completes - if let Some(task) = self.health_check_task.write().take() { - tracing::debug!( - box_id = %self.config.id, - "Aborting health check task" - ); - task.abort(); - } + // Cancel health check task first (if running) + // This prevents the task from continuing after stop() completes + self.abort_health_check();🤖 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/boxlite/src/litebox/box_impl.rs` around lines 371 - 379, The stop() teardown in BoxImpl duplicates the same health-check cancellation logic already centralized in abort_health_check(), so update stop() to call abort_health_check() instead of reimplementing the health_check_task.write().take(), tracing::debug!, and task.abort() sequence. Keep the existing behavior by reusing the BoxImpl::abort_health_check() helper directly so the cancellation path stays consistent and avoids future drift.src/boxlite/src/litebox/state.rs (1)
462-474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "Mark box as crashed" docstring on
mark_stop().This docstring predates the new
BoxStatus::Crashedvariant and is now misleading:mark_stop()is the plain graceful-stop path (e.g. used byshutdown_sync()for SIGTERM stops), whereas actual crash handling now goes through a distinctBoxStatus::Crashed+ customStopInfoset directly inrecord_crash_and_plan_restart(rt_impl.rs), not throughmark_stop(). Worth updating the comment so it doesn't conflate "stop" with "crash" now that these are separate concepts.📝 Suggested doc fix
- /// Mark box as crashed (sets status to Stopped since VM is no longer running). - /// - /// In our simplified state model, crashed VMs become Stopped - /// since the rootfs is preserved and can be restarted. - /// PID is cleared since the process is no longer alive. + /// Mark box as stopped (graceful/user-initiated stop, not a crash). + /// + /// Rootfs is preserved so the box can be restarted later. + /// PID is cleared since the process is no longer alive. + /// Actual crash handling uses `BoxStatus::Crashed` + a dedicated + /// `StopInfo` set directly by the crash coordinator, not this method.🤖 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/boxlite/src/litebox/state.rs` around lines 462 - 474, Update the doc comment on `mark_stop()` in `state.rs` so it describes the graceful-stop path only, not crashes. The current wording conflicts with the newer `BoxStatus::Crashed` flow used elsewhere; make it clear that `mark_stop()` is for normal shutdowns like `shutdown_sync()`/SIGTERM, while crash handling is done separately in `record_crash_and_plan_restart` with `BoxStatus::Crashed` and its own `StopInfo`. Keep the implementation unchanged and align the comment with the actual behavior of `mark_stop`, `BoxStatus`, and `StopCause`.src/boxlite/tests/restart_policy.rs (1)
345-401: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider adding a regression test for stop racing with a failing restart attempt.
The existing race test only covers stop() vs. a crash that leads to a successful restart. Given the TOCTOU found in
rt_impl.rs'srun_restart_looperror branch (state overwritten without an epoch check after a failed restart attempt), a test that forces the restart attempt itself to fail (e.g. corrupt rootfs momentarily) while racing a userstop()would directly validate that fix once applied.🤖 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/boxlite/tests/restart_policy.rs` around lines 345 - 401, Add a regression test that exercises the stop-vs-failed-restart race in restart_policy_unless_stopped_user_stop_race_stays_stopped, since the current test only covers a successful restart path. Update the test in restart_policy.rs to force the run_restart_loop failure branch in rt_impl.rs (for example by making the restart attempt fail briefly) while a user stop() is in flight, and assert the box remains Stopped with StopCause::Normal and no shim PID. Use the existing helpers BoxTestBase, wait_for_info_fast, and expect_status so the new test directly validates the epoch/state fix in run_restart_loop.
🤖 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/boxlite/src/litebox/handle.rs`:
- Around line 51-53: The `BoxHandle::info` name collision between the inherent
method and the `BoxBackend` trait implementation is a latent recursion trap.
Update the `<BoxHandle as BoxBackend>::info` implementation to avoid calling
`self.info()` and instead delegate explicitly through a different path (for
example, by accessing `self.current()` directly like the inherent method does or
by renaming one side), so the trait method cannot accidentally recurse if the
inherent wrapper changes.
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 736-790: The restart failure and shutdown cleanup paths in
rt_impl::restart are writing box state without re-checking lifecycle_epoch,
which can overwrite newer stop/remove transitions. Update the Err(e) branch, the
max-retries-exceeded branch, the shutdown_token.cancelled() branch, and
mark_restart_failed to reuse crash_restart_matches_plan (or the same
epoch/status guard used earlier in restart()) before calling force_status or
save_box. Keep the guard under the per-box lock so only the current lifecycle
attempt can commit state changes.
- Around line 752-767: The max-retries-exceeded terminal path in rt_impl.rs
leaves the strong restart_owned_handles_by_id entry alive, unlike the existing
mark_restart_denied flow. Update the "Max retries exceeded" branch to follow the
same cleanup path used by retire_cached_box_after_crash/invalidate_box_handle so
the BoxHandle and BoxImpl are released and their shutdown/health-check cleanup
is triggered when setting BoxStatus::Stopped and StopCause::MaxRetriesExceeded.
- Around line 88-91: Move the crash-path database and file I/O off the
coordinator task: `CrashCoordinator::run()` should not block on synchronous
`box_by_id()`, `save_box()`, or `read_box_exit_code()` while multiplexing
`crash_rx` futures. Update the crash handling flow in
`record_crash_and_plan_restart()`, `restart()`, and the deny/failure branches so
the SQLite/file work is done asynchronously or on a separate blocking task, then
return only the result needed by the coordinator. Keep the existing
`CrashCoordinator` and `read_box_exit_code()` entry points, but ensure they no
longer perform direct blocking I/O on the main crash-processing path.
In `@src/boxlite/src/runtime/types.rs`:
- Around line 318-325: The `BoxInfo::eq` implementation is missing the new
runtime fields, so boxes with different crash/restart state can still compare
equal. Update `BoxInfo::eq` in `types.rs` to include both `stop_info` and
`last_restart_error` in the equality check, and make sure
`crate::litebox::StopInfo` derives or implements `PartialEq` so `stop_info` can
participate in the comparison.
---
Outside diff comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 747-762: The health check startup path in box_impl::BoxImpl should
initialize health_status whenever monitoring is active, not only when an
explicit health_check is configured. Update the logic around
effective_health_check() so init_health_status() is called when
effective_health_check().is_some(), then keep spawning the health task and
storing it in health_check_task as before. This ensures the persisted state is
set up consistently for restart_policy-driven health checks as well.
---
Nitpick comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 371-379: The stop() teardown in BoxImpl duplicates the same
health-check cancellation logic already centralized in abort_health_check(), so
update stop() to call abort_health_check() instead of reimplementing the
health_check_task.write().take(), tracing::debug!, and task.abort() sequence.
Keep the existing behavior by reusing the BoxImpl::abort_health_check() helper
directly so the cancellation path stays consistent and avoids future drift.
In `@src/boxlite/src/litebox/state.rs`:
- Around line 462-474: Update the doc comment on `mark_stop()` in `state.rs` so
it describes the graceful-stop path only, not crashes. The current wording
conflicts with the newer `BoxStatus::Crashed` flow used elsewhere; make it clear
that `mark_stop()` is for normal shutdowns like `shutdown_sync()`/SIGTERM, while
crash handling is done separately in `record_crash_and_plan_restart` with
`BoxStatus::Crashed` and its own `StopInfo`. Keep the implementation unchanged
and align the comment with the actual behavior of `mark_stop`, `BoxStatus`, and
`StopCause`.
In `@src/boxlite/tests/restart_policy.rs`:
- Around line 345-401: Add a regression test that exercises the
stop-vs-failed-restart race in
restart_policy_unless_stopped_user_stop_race_stays_stopped, since the current
test only covers a successful restart path. Update the test in restart_policy.rs
to force the run_restart_loop failure branch in rt_impl.rs (for example by
making the restart attempt fail briefly) while a user stop() is in flight, and
assert the box remains Stopped with StopCause::Normal and no shim PID. Use the
existing helpers BoxTestBase, wait_for_info_fast, and expect_status so the new
test directly validates the epoch/state fix in run_restart_loop.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 370d859a-ac98-4a0d-9d7a-6f25931a13e9
📒 Files selected for processing (24)
docs/development/restart.mdsdks/c/src/info.rssdks/node/src/info.rssdks/node/src/lib.rssdks/node/src/options.rssdks/python/boxlite/__init__.pysdks/python/src/advanced_options.rssdks/python/src/info.rssdks/python/src/lib.rssdks/python/src/options.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/handle.rssrc/boxlite/src/litebox/init/mod.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/lock/mod.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/advanced_options.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/restart_policy.rs
| fn read_box_exit_code(box_home: &Path) -> Option<i32> { | ||
| let exit_file = box_home.join(EXIT_FILE); | ||
| ExitInfo::from_file(&exit_file).map(|info| info.exit_code()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the BoxManager/BoxStore backing implementation and check if box_by_id/save_box do blocking I/O.
fd -e rs . src/boxlite/src/db 2>/dev/null
rg -n -A8 'fn box_by_id|fn save_box' src/boxlite/src/db src/boxlite/src/runtime 2>/dev/null
rg -n 'sled::|rocksdb::|redb::' src/boxlite/Cargo.toml src/boxlite/src 2>/dev/null
rg -n 'spawn_blocking' src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 1421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the crash-handling paths mentioned in the review comment.
sed -n '500,820p' src/boxlite/src/runtime/rt_impl.rs
echo '---'
sed -n '2040,2215p' src/boxlite/src/runtime/rt_impl.rs
echo '---'
# Inspect the DB layer for whether the relevant APIs do synchronous disk I/O.
sed -n '1,260p' src/boxlite/src/db/boxes.rs
echo '---'
sed -n '1,240p' src/boxlite/src/db/base_disk.rs
echo '---'
# Find the concrete BoxManager methods used by the runtime.
rg -n 'fn (lookup_box|lookup_box_id|all_boxes|save_box|box_by_id|exists|record_crash|restart|mark_restart_failed|mark_restart_denied)' src/boxlite/src/db src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 37871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm how the crash coordinator drives these futures.
rg -n -A120 'FuturesUnordered|CrashCoordinator::run|crash_rx\.recv|handle_box_crash|record_crash_and_plan_restart|run_restart_loop' src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 30844
Move crash-path DB/file I/O off the coordinator task
CrashCoordinator::run() multiplexes crash futures through one FuturesUnordered, but record_crash_and_plan_restart(), restart(), and the deny/failure paths call synchronous box_by_id()/save_box() (and read_box_exit_code()) directly. A slow SQLite write or filesystem read can block crash_rx.recv() and stall crash handling for every other box.
🤖 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/boxlite/src/runtime/rt_impl.rs` around lines 88 - 91, Move the crash-path
database and file I/O off the coordinator task: `CrashCoordinator::run()` should
not block on synchronous `box_by_id()`, `save_box()`, or `read_box_exit_code()`
while multiplexing `crash_rx` futures. Update the crash handling flow in
`record_crash_and_plan_restart()`, `restart()`, and the deny/failure branches so
the SQLite/file work is done asynchronously or on a separate blocking task, then
return only the result needed by the coordinator. Keep the existing
`CrashCoordinator` and `read_box_exit_code()` entry points, but ensure they no
longer perform direct blocking I/O on the main crash-processing path.
|
|
||
| /// Stop info (valid when status is Stopped/Crashed/Restarting). | ||
| #[serde(default)] | ||
| pub stop_info: crate::litebox::StopInfo, | ||
|
|
||
| /// Last restart error message (if any). | ||
| #[serde(default)] | ||
| pub last_restart_error: Option<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B3 'pub struct StopInfo' src/boxlite/src/litebox/state.rs
rg -n 'impl PartialEq for BoxInfo|derive.*PartialEq' src/boxlite/src/runtime/types.rsRepository: boxlite-ai/boxlite
Length of output: 423
🏁 Script executed:
#!/bin/bash
sed -n '180,380p' src/boxlite/src/runtime/types.rs
sed -n '286,320p' src/boxlite/src/litebox/state.rsRepository: boxlite-ai/boxlite
Length of output: 7209
Include the new runtime fields in BoxInfo equality
BoxInfo::eq still ignores stop_info and last_restart_error, so boxes that differ only in crash/restart state compare equal. StopInfo also needs PartialEq before stop_info can be added to the comparison.
🤖 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/boxlite/src/runtime/types.rs` around lines 318 - 325, The `BoxInfo::eq`
implementation is missing the new runtime fields, so boxes with different
crash/restart state can still compare equal. Update `BoxInfo::eq` in `types.rs`
to include both `stop_info` and `last_restart_error` in the equality check, and
make sure `crate::litebox::StopInfo` derives or implements `PartialEq` so
`stop_info` can participate in the comparison.
8133d6a to
74b482c
Compare
📦 BoxLite review — 2 issues ·
|
| state.stop_info = crate::litebox::StopInfo { | ||
| cause: StopCause::CrashedNoPolicy, | ||
| exit_code, | ||
| exit_time: Some(Utc::now()), | ||
| restart_count: new_restart_count, | ||
| restarted_at: None, | ||
| }; |
There was a problem hiding this comment.
cause is hardcoded CrashedNoPolicy at crash-detection time regardless of eventual restart decision, and force_status(Restarting) never updates it, so BoxInfo queried during the Crashed/Restarting window shows a misleading cause until success/failure commit.
Introduce the restart policy model and persisted stop metadata needed for automatic crash recovery. This adds RestartPolicy variants, exponential backoff calculation, effective health-check selection, and the Crashed and Restarting BoxStatus values. It also extends BoxState and BoxInfo with StopInfo and last restart error fields so future runtime restart logic can persist crash context. SDK status string conversions are updated in the same commit because adding BoxStatus variants makes those matches exhaustive. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
Add BoxHandle as the stable backend used by LiteBox and move the runtime cache from BoxImpl values to BoxHandle values. The handle delegates normal box and snapshot operations to the current BoxImpl. This keeps the public LiteBox value stable while allowing a later restart path to replace the underlying VM implementation. No restart behavior is introduced here; this is a behavior-preserving cache and handle refactor. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
Add an owned lock guard and async lock acquisition helpers that move blocking waits onto Tokio's blocking pool instead of retrying with yield_now(). Use the helper for box stop and live-state initialization so box lifecycle paths no longer spin or block a Tokio worker while waiting for the per-box lock. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
Add runtime crash recovery for boxes whose shim process dies while the embedding process is still running. Health checks now report shim death to a crash coordinator instead of mutating box state directly. The coordinator deduplicates in-flight crash notifications, records crash metadata, evaluates restart policy, applies exponential backoff, and restarts eligible boxes through a fresh BoxImpl swapped into the stable BoxHandle. This also teaches the init pipeline to treat Restarting like Stopped so restart reuses the existing rootfs. Integration tests cover No, Always, OnFailure, and UnlessStopped policies. Startup-time recovery of persisted crashed boxes is intentionally not included in this commit. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
Expose restart policy configuration through the Python and Node SDK option types. Python gets a RestartPolicy class with no, always, on_failure, and unless_stopped constructors and AdvancedBoxOptions support. Node gets a JsRestartPolicy object shape and conversion into the Rust RestartPolicy model, including validation for on_failure maxRetries. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
Document the runtime crash-restart architecture, policy behavior, state model, backoff behavior, and SDK configuration examples. The document explicitly scopes this phase to in-process runtime crash handling. Startup-time recovery of persisted crashed boxes is deferred. Signed-off-by: Wenyu Huang <huangwenyuu@outlook.com>
74b482c to
19a42ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/boxlite/src/runtime/rt_impl.rs (1)
997-1018: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMax-retries terminal path still leaks the strong
restart_owned_handles_by_identry.
mark_restart_deniedretires the cached handle viaretire_cached_box_after_crash→invalidate_box_handle, but this branch reaches the same terminalStoppedoutcome and only commits DB state. TheArc<BoxHandle>and itsBoxImplstay strongly referenced for the runtime's lifetime, with the watcher task andshutdown_tokennever cancelled. This was raised on an earlier commit and appears unresolved.🔒 Proposed fix
).await { tracing::error!( box_id = %box_id, attempt, error = %e, "Failed to save max-retries-exceeded state" ); } + if let Ok(Some((config, state))) = this.box_manager.box_by_id(&box_id) { + this.retire_cached_box_after_crash( + &box_id, + config.name.as_deref(), + &state, + ); + } break; // Give 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/boxlite/src/runtime/rt_impl.rs` around lines 997 - 1018, Update the max-retries terminal branch in the restart handling flow, alongside commit_crash_restart_state, to retire and invalidate the cached box handle through the existing retire_cached_box_after_crash/invalidate_box_handle path before breaking. Preserve the Stopped state update and error logging while ensuring restart_owned_handles_by_id no longer retains the Arc<BoxHandle>, allowing its watcher and shutdown token to be released.
🧹 Nitpick comments (3)
src/boxlite/src/litebox/box_impl.rs (2)
234-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
abort_health_checkaborts the whole watcher, not just the probe — rename and reuse instop().The method kills the exit-observation arm too, so the name understates its effect. Lines 639-645 in
stop()also duplicate this exact block verbatim.♻️ Suggested rename + dedupe
- /// Abort the watcher task (including its optional health probe). + /// Abort the box watcher task (exit observation and its optional health probe). /// /// Used by restart() to retire this implementation before installing a fresh one. - pub(crate) fn abort_health_check(&self) { + pub(crate) fn abort_watcher(&self) {and in
stop():- if let Some(task) = self.watcher.get() { - tracing::debug!( - box_id = %self.config.id, - "Aborting box watcher" - ); - task.abort(); - } + self.abort_watcher();🤖 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/boxlite/src/litebox/box_impl.rs` around lines 234 - 245, Rename abort_health_check to reflect that it aborts the entire watcher task, then update all call sites, including restart(). In stop(), remove the duplicated watcher-abort block and reuse the renamed method so logging and abort behavior remain centralized.
712-751: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
was_persistedis now always true — theadd_boxfallback is unreachable.Lines 622-633 already guarantee
lock_idisSome(otherwise the function returned or errored), and nothing clears it in between, so theelsebranch at Lines 748-751 is dead. Either drop it or hoist the "never persisted" case into the earlierlock_idcomputation so the intent stays explicit.🤖 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/boxlite/src/litebox/box_impl.rs` around lines 712 - 751, The was_persisted branch in the stop-state update is unreachable because lock_id has already been established. Remove the redundant was_persisted check and unreachable add_box fallback, keeping the existing save_box handling and NotFound behavior for the persisted box path.src/boxlite/src/runtime/rt_impl.rs (1)
160-171: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
UnlessStoppedandAlwaysare both unreachable here but handled inconsistently.
should_restartreturnstrueunconditionally for both policies, so neither can reach a denial.Alwayslogs +debug_assert!s, whileUnlessStoppedquietly returnsStopCause::Normal— indistinguishable from a real user stop if the invariant ever breaks. Treat them the same way.♻️ Suggested change
- Some(RestartPolicy::UnlessStopped) => { - // UnlessStopped only denies restart when user explicitly stopped (cause == Normal) - // At this point, exit_code == 0 indicates a clean exit from user stop - StopCause::Normal - } + Some(RestartPolicy::UnlessStopped) => { + // UnlessStopped always restarts; a denial here means the caller + // bypassed `should_restart`. + tracing::error!( + "BUG: UnlessStopped policy should not reach stop_cause_when_restart_denied" + ); + debug_assert!(false, "UnlessStopped policy should never deny restart"); + StopCause::Unknown + }🤖 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/boxlite/src/runtime/rt_impl.rs` around lines 160 - 171, Update the RestartPolicy branches in stop_cause_when_restart_denied so RestartPolicy::UnlessStopped handles the unreachable invariant violation identically to RestartPolicy::Always: log the bug, trigger the debug assertion, and return StopCause::Unknown. Do not retain the quiet StopCause::Normal fallback.
🤖 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 `@docs/development/restart.md`:
- Around line 52-60: Update the outcome table and related text in the restart
documentation to state that CrashedNoPolicy is only possible when crash
monitoring is active through configured or auto-enabled health checks. Keep
no-policy boxes without health checks documented as Stopped, unless the
implementation explicitly monitors them; apply the same clarification to the
additional outcome-table section.
- Around line 125-132: Update the “Restart Policy Semantics” documentation to
explicitly distinguish Always from UnlessStopped: Always continues restarting
despite an explicit user stop(), while UnlessStopped respects that stop. Keep
the existing stale crash work and lifecycle epoch explanation alongside the new
distinction.
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 647-667: The stop() flow in the shutdown-token check incorrectly
treats any cancelled self.shutdown_token as a retired box, causing retries after
partial cleanup failures to skip required cleanup. Replace this condition with
an explicit retired/invalidated state used by restart() and crash cleanup, while
preserving the early return only for genuinely retired implementations; ensure
retrying stop() still performs PID removal, state persistence, cache
invalidation, and stopped-listener notification.
In `@src/boxlite/src/litebox/watcher.rs`:
- Around line 192-205: The restart-policy branch in the watcher must fall
through to the normal exit-recording path when crash_tx.send fails, rather than
returning after only logging the error. Preserve the early return when the
notification succeeds, and ensure the existing local state/DB update path
records the box as Crashed or Stopped on delivery failure.
- Around line 189-192: Update the crash coordinator’s stopped terminal path,
including mark_restart_denied and its retire_cached_box_after_crash flow, to
remove boxes configured with auto_delete > 0/auto_remove after restarts are
denied or retries are exhausted. Preserve existing stopped-state persistence and
handle invalidation, and ensure RestartPolicy::No cases sent by on_shim_exit
receive the same auto-removal behavior.
---
Duplicate comments:
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 997-1018: Update the max-retries terminal branch in the restart
handling flow, alongside commit_crash_restart_state, to retire and invalidate
the cached box handle through the existing
retire_cached_box_after_crash/invalidate_box_handle path before breaking.
Preserve the Stopped state update and error logging while ensuring
restart_owned_handles_by_id no longer retains the Arc<BoxHandle>, allowing its
watcher and shutdown token to be released.
---
Nitpick comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 234-245: Rename abort_health_check to reflect that it aborts the
entire watcher task, then update all call sites, including restart(). In stop(),
remove the duplicated watcher-abort block and reuse the renamed method so
logging and abort behavior remain centralized.
- Around line 712-751: The was_persisted branch in the stop-state update is
unreachable because lock_id has already been established. Remove the redundant
was_persisted check and unreachable add_box fallback, keeping the existing
save_box handling and NotFound behavior for the persisted box path.
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 160-171: Update the RestartPolicy branches in
stop_cause_when_restart_denied so RestartPolicy::UnlessStopped handles the
unreachable invariant violation identically to RestartPolicy::Always: log the
bug, trigger the debug assertion, and return StopCause::Unknown. Do not retain
the quiet StopCause::Normal fallback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11327b44-70fe-459d-862e-86997c28408b
📒 Files selected for processing (25)
docs/development/restart.mdsdks/c/src/info.rssdks/node/src/info.rssdks/node/src/lib.rssdks/node/src/options.rssdks/python/boxlite/__init__.pysdks/python/src/advanced_options.rssdks/python/src/info.rssdks/python/src/lib.rssdks/python/src/options.rssrc/boxlite/src/lib.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/handle.rssrc/boxlite/src/litebox/init/mod.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/litebox/watcher.rssrc/boxlite/src/lock/mod.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/advanced_options.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/restart_policy.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- sdks/c/src/info.rs
- sdks/python/src/info.rs
- sdks/node/src/lib.rs
- sdks/python/src/options.rs
- src/boxlite/src/runtime/options.rs
- src/boxlite/src/runtime/layout.rs
- sdks/python/src/lib.rs
- src/boxlite/src/rest/types.rs
- src/boxlite/src/lib.rs
- sdks/python/boxlite/init.py
- src/boxlite/src/litebox/init/mod.rs
- src/boxlite/src/litebox/mod.rs
- src/boxlite/src/litebox/handle.rs
- sdks/node/src/options.rs
- src/boxlite/src/runtime/types.rs
- sdks/python/src/advanced_options.rs
- src/boxlite/src/runtime/advanced_options.rs
- src/boxlite/src/lock/mod.rs
- src/boxlite/tests/restart_policy.rs
- src/boxlite/src/litebox/state.rs
| 1. A Box starts with a health-check task when health checks are configured or | ||
| auto-enabled by a restart policy. | ||
| 2. The health-check task periodically pings the guest. | ||
| 3. If the ping fails, the health-check task checks whether the shim process is | ||
| still alive. | ||
| 4. If the shim is alive, the task records health-check failure state. Guest | ||
| unresponsiveness alone does not trigger restart policy. | ||
| 5. If the shim process died, the task sends the Box ID to the runtime crash | ||
| coordinator and exits. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarify when “no policy” can produce CrashedNoPolicy.
The runtime flow says crash monitoring starts only when health checks are configured or auto-enabled by a restart policy, but the outcome table says boxes with no policy become Stopped with CrashedNoPolicy. With neither a policy nor an explicit health check, shim death is not observed, so that transition cannot occur. Qualify the table with the monitoring prerequisite or document that no-policy boxes are also monitored.
Also applies to: 176-179
🤖 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 `@docs/development/restart.md` around lines 52 - 60, Update the outcome table
and related text in the restart documentation to state that CrashedNoPolicy is
only possible when crash monitoring is active through configured or auto-enabled
health checks. Keep no-policy boxes without health checks documented as Stopped,
unless the implementation explicitly monitors them; apply the same clarification
to the additional outcome-table section.
| ## Restart Policy Semantics | ||
|
|
||
| | Policy | Restart condition | Retry limit | | ||
| |--------|-------------------|-------------| | ||
| | `No` | Never restart after a crash. | N/A | | ||
| | `Always` | Restart after detected crashes. Manual stop is respected. | Unlimited | | ||
| | `OnFailure { max_retries }` | Restart when the exit code is non-zero or unknown, while the current retry count is below `max_retries`. | `max_retries` | | ||
| | `UnlessStopped` | Restart after detected crashes. Manual stop is respected because stale crash work cannot commit after lifecycle epoch changes. | Unlimited | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Files matching restart policy docs/configs"
git ls-files | rg -i 'restart|policy|development|docs' | sed -n '1,120p'
echo
echo "## Target snippet"
sed -n '110,145p' docs/development/restart.md 2>/dev/null || true
echo
echo "## Search restart policy implementations/usages"
rg -n "UnlessStopped|Always|RestartPolicy|restart policy|OnFailure|max_retries|lifecycle epoch" . --glob '!**/node_modules/**' --glob '!**/.git/**' | sed -n '1,220p'Repository: boxlite-ai/boxlite
Length of output: 30282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Deterministic policy-text/implementation invariant probe"
python3 - <<'PY'
from pathlib import Path
import re
target = Path('docs/development/restart.md')
text = target.read_text() if target.exists() else ''
rows = []
for m in re.finditer(r'\| (`[^`]+`?)\)[ \t]*\|\s*([^\n|]+)\s*\|\s*([^\n|]+)\s*\|', text):
rows.append((m.group(1), m.group(2).strip(), m.group(3).strip()))
policies = {rows[i][0]: (rows[i][1], rows[i][2]) for i in range(len(rows))}
for policy in ['`Always`', '`UnlessStopped`']:
print(policy, ":", policies.get(policy, 'missing'))
def strip_backticks(s):
return s.replace('`','').strip().lower()
always = policies.get('`Always`', None)
unless = policies.get('`UnlessStopped`', None)
print("always_exists=", always is not None, "unless_exists=", unless is not None)
if always and unless:
condition_same = strip_backticks(always[0]) == strip_backticks(unless[0])
limit_same = strip_backticks(always[1]) == strip_backticks(unless[1])
print("condition_text_same=", condition_same)
print("limit_text_same=", limit_same)
PY
echo
echo "## Source occurrences around explicit semantics comments"
rg -n -C 3 "UnlessStopped|Always \b|RestartAfter|RestartWhen|stal|epoch|lifecycle" . --glob '!**/node_modules/**' --glob '!**/.git/**' | sed -n '1,260p'Repository: boxlite-ai/boxlite
Length of output: 18651
Document when to use Always vs UnlessStopped.
These policies both restart after crashes with unlimited retries, but Always should not respect an explicit user stop() while UnlessStopped does. Add that distinction alongside the existing “stale crash work” note.
🤖 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 `@docs/development/restart.md` around lines 125 - 132, Update the “Restart
Policy Semantics” documentation to explicitly distinguish Always from
UnlessStopped: Always continues restarting despite an explicit user stop(),
while UnlessStopped respects that stop. Keep the existing stale crash work and
lifecycle epoch explanation alongside the new distinction.
| { | ||
| let mut state = self.state.write(); | ||
| state.clear_health_status(); | ||
| } | ||
| let locker = self.runtime.lock_manager.retrieve(lock_id)?; | ||
| let _lock_guard = acquire_owned_lock(locker).await?; | ||
|
|
||
| // Cancel the token - signals all in-flight operations to abort | ||
| self.shutdown_token.cancel(); | ||
| if self.shutdown_token.is_cancelled() && !self.runtime.shutdown_token.is_cancelled() { | ||
| tracing::debug!( | ||
| box_id = %self.config.id, | ||
| "Ignoring stop on retired box implementation" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Clear health status (box is no longer running) | ||
| { | ||
| let mut state = self.state.write(); | ||
| state.clear_health_status(); | ||
| } | ||
|
|
||
| // Cancel the token - signals all in-flight operations to abort | ||
| self.shutdown_token.cancel(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Who cancels a BoxImpl's shutdown_token besides stop()?
rg -nP -C3 'shutdown_token\.cancel\(\)' src/boxlite/src
# Is there any existing retirement flag on BoxImpl?
ast-grep outline src/boxlite/src/litebox/box_impl.rs --items all | head -60Repository: boxlite-ai/boxlite
Length of output: 7549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect BoxImpl shutdown handling and the retired-stop detection without running repo code.
sed -n '600,760p' src/boxlite/src/litebox/box_impl.rs
printf '\n--- shared cancellation/token APIs usages in BoxImpl nearby\n'
rg -n -C2 'shutdown_token|Retiring|Retired|running|is_running|Status::Stopped|force_status|Clear health|clear_health_status' src/boxlite/src/litebox/box_impl.rsRepository: boxlite-ai/boxlite
Length of output: 21437
Do not treat a cancelled shutdown_token as "retired" inside stop().
stop() can return Err after canceling self.shutdown_token but before completing cleanup (handler.stop()? or save_box). A retry then satisfies self.shutdown_token.is_cancelled() && !self.runtime.shutdown_token.is_cancelled() and returns Ok(()) without removing the PID file, updating/persisting state, invalidating the cache, or notifying stopped listeners. Use an explicit retired/invalidated state for restart() / crash cleanup instead of relying on token 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/boxlite/src/litebox/box_impl.rs` around lines 647 - 667, The stop() flow
in the shutdown-token check incorrectly treats any cancelled self.shutdown_token
as a retired box, causing retries after partial cleanup failures to skip
required cleanup. Replace this condition with an explicit retired/invalidated
state used by restart() and crash cleanup, while preserving the early return
only for genuinely retired implementations; ensure retrying stop() still
performs PID removal, state persistence, cache invalidation, and
stopped-listener notification.
| // Restart-policy boxes are finalized by the central crash coordinator, | ||
| // which owns the Crashed/Restarting transitions and handle replacement. | ||
| // Leave the persisted state Running until it acquires the lifecycle lock. | ||
| if self.has_restart_policy { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does any crash-coordinator terminal path call remove_box / removes_on_stop?
rg -nP -C4 'removes_on_stop|remove_box\(' src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 4099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== watcher relevant sections =="
sed -n '160,280p' src/boxlite/src/litebox/watcher.rs
echo
echo "== crash coordinator / retire methods in watcher =="
rg -n -C5 'retire_cached_box_after_crash|mark_restart_denied|max.*retry|has_restart_policy|removes_on_stop|remove_box' src/boxlite/src/litebox/watcher.rs
echo
echo "== option methods around removes_on_stop =="
rg -n -C3 'removes_on_stop|auto_delete|RestartPolicy::No|RestartPolicy::Max' src/boxlite/src -g '*.rs'
echo
echo "== inspect BoxID/string conversion and remove_box call context if present in runtime =="
sed -n '2060,2140p' src/boxlite/src/runtime/rt_impl.rs
sed -n '1768,1810p' src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rust package manager availability =="
command -v cargo || true
command -v rustup || true
echo
echo "== parse remove-box handling around stop-critical paths =="
rg -n -C6 'fn retire_cached_box_after_crash|fn mark_restart_denied|max_retries_exhausted|StopCause::MaxRetriesExceeded|StopCause::CrashedNoPolicy|remove_box\(|remove_box' src/boxlite/src/runtime/rt_impl.rs src/boxlite/src/litebox/watcher.rs | sed -n '1,260p'
echo
echo "== read-only AST/text summary of terminal functions in rt_impl.rs =="
python3 - <<'PY'
from pathlib import Path
p = Path('src/boxlite/src/runtime/rt_impl.rs')
s = p.read_text()
for pat in ['fn retire_cached_box_after_crash', 'fn mark_restart_denied']:
idx = s.find(pat)
print(f'\n--- {pat} at {idx} ---')
print('\n'.join(s.splitlines()[max(0, idx//120-20): (idx//120)+160]))
PYRepository: boxlite-ai/boxlite
Length of output: 35882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact terminator implementations =="
rg -n -C8 'pub(crate) fn retire_cached_box_after_crash|fn mark_restart_denied|fn invalidate_box_handle' src/boxlite/src/runtime/rt_impl.rs
echo
echo "== line-numbered snippets =="
sed -n '1040,1125p' src/boxlite/src/runtime/rt_impl.rs
sed -n '1076,1150p' src/boxlite/src/runtime/rt_impl.rs
sed -n '1810,1860p' src/boxlite/src/runtime/rt_impl.rsRepository: boxlite-ai/boxlite
Length of output: 8948
Auto-remove ephemeral restart-policy boxes when restarts are denied.
on_shim_exit() sends all has_restart_policy cases, including RestartPolicy::No, to the crash coordinator. When the coordinator reaches the stopped terminal path (mark_restart_denied) for denials or max retries, it only persists stopped state and calls retire_cached_box_after_crash() → invalidate_box_handle(), without remove_box(). Boxes with auto_delete > 0/auto_remove, including the --rm default, can be left as stopped entries even though they are configured to be removed-on-stop.
🤖 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/boxlite/src/litebox/watcher.rs` around lines 189 - 192, Update the crash
coordinator’s stopped terminal path, including mark_restart_denied and its
retire_cached_box_after_crash flow, to remove boxes configured with auto_delete
> 0/auto_remove after restarts are denied or retries are exhausted. Preserve
existing stopped-state persistence and handle invalidation, and ensure
RestartPolicy::No cases sent by on_shim_exit receive the same auto-removal
behavior.
| if self.has_restart_policy { | ||
| let crash_tx = runtime.crash_sender(); | ||
| let box_id = self.box_id.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(error) = crash_tx.send(box_id.clone()).await { | ||
| tracing::error!( | ||
| box_id = %box_id, | ||
| error = %error, | ||
| "Crash handler channel closed, notification dropped" | ||
| ); | ||
| } | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
No fallback when the crash notification can't be delivered — the box stays persisted as Running behind a dead shim.
The early return skips every state write, so the coordinator is the only thing that can finalize this box. If crash_tx.send() fails (channel closed, coordinator already drained during shutdown) the code only logs an error, and both the in-memory BoxState and the DB row keep reporting Running with a stale PID until the next process restart runs recover_boxes. That is exactly the "Running behind a shim that exited hours ago" lie this watcher exists to prevent.
Consider falling through to the normal exit-recording path when the send fails, so the box is at least marked Crashed/Stopped locally.
🤖 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/boxlite/src/litebox/watcher.rs` around lines 192 - 205, The
restart-policy branch in the watcher must fall through to the normal
exit-recording path when crash_tx.send fails, rather than returning after only
logging the error. Preserve the early return when the notification succeeds, and
ensure the existing local state/DB update path records the box as Crashed or
Stopped on delivery failure.
Summary
This PR adds restart policy support for boxes and wires it into runtime crash recovery.
The runtime now detects shim death through health checks, records crash metadata, evaluates the configured restart policy, and restarts eligible boxes with backoff. Crash handling is centralized in a crash coordinator task instead of spawning detached per-crash tasks.
Changes
NoAlwaysOnFailure { max_retries }UnlessStoppedBoxHandlesupport so existing handles can observe a restarted box.First part of #32
Summary by CodeRabbit
no,always,on-failure,unless-stopped) with automatic health-check enablement when required.