Skip to content

fix(box_impl): offload blocking handler.stop() and metrics() to spawn_blocking #415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lilongen wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(box_impl): offload blocking handler.stop() and metrics() to spawn_blocking #415

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions src/boxlite/src/litebox/box_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub type SharedBoxImpl = Arc<BoxImpl>;
/// Separated from BoxImpl to allow operations like `info()` without initializing LiveState.
pub(crate) struct LiveState {
// VM process control
handler: std::sync::Mutex<Box<dyn VmmHandler>>,
handler: Arc<std::sync::Mutex<Box<dyn VmmHandler>>>,
guest_session: GuestSession,

// Metrics
Expand Down Expand Up @@ -78,7 +78,7 @@ impl LiveState {
#[cfg(target_os = "linux")] bind_mount: Option<BindMountHandle>,
) -> Self {
Self {
handler: std::sync::Mutex::new(handler),
handler: Arc::new(std::sync::Mutex::new(handler)),
guest_session,
metrics,
_container_rootfs_disk: container_rootfs_disk,
Expand Down Expand Up @@ -294,11 +294,15 @@ impl BoxImpl {
}

let live = self.live_state().await?;
let handler = live
.handler
.lock()
.map_err(|e| BoxliteError::Internal(format!("handler lock poisoned: {}", e)))?;
let raw = handler.metrics()?;
let handler = Arc::clone(&live.handler);
let raw = tokio::task::spawn_blocking(move || {
Copy link
Copy Markdown
Contributor

@uran0sH uran0sH Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this raw not being used?

let h = handler
.lock()
.map_err(|e| BoxliteError::Internal(format!("handler lock poisoned: {}", e)))?;
h.metrics()
})
.await
.map_err(|e| BoxliteError::Internal(format!("spawn_blocking join: {}", e)))??;
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The join error message from spawn_blocking is too generic; when this triggers it will be hard to tell whether the failure happened during metrics collection vs some other blocking task. Consider including operation context (e.g., metrics spawn_blocking join) and/or the box_id in the error message to make debugging easier.

Suggested change
.map_err(|e| BoxliteError::Internal(format!("spawn_blocking join: {}", e)))??;
.map_err(|e| {
BoxliteError::Internal(format!(
"metrics spawn_blocking join (box_id={}): {}",
self.config.id, e
))
})??;

Copilot uses AI. Check for mistakes.

Ok(BoxMetrics::from_storage(
&live.metrics,
Expand Down Expand Up @@ -355,10 +359,17 @@ impl BoxImpl {
tracing::warn!(box_id = %self.config.id, "Guest shutdown timed out after 10s");
}

// Stop handler
if let Ok(mut handler) = live.handler.lock() {
handler.stop()?;
}
// Stop handler (on blocking thread — ShimHandler::stop() polls with sleep)
let handler = Arc::clone(&live.handler);
tokio::task::spawn_blocking(move || {
if let Ok(mut h) = handler.lock() {
h.stop()
} else {
Ok(())
Comment on lines +365 to +368
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the handler mutex is poisoned, this path currently returns Ok(()) without attempting to stop the handler. That can leave the VM/shim running and leak resources. If the intent is to not fail shutdown on poisoning, consider recovering the guard via PoisonError::into_inner() and still calling stop(), while continuing to swallow the poisoning error itself.

Suggested change
if let Ok(mut h) = handler.lock() {
h.stop()
} else {
Ok(())
match handler.lock() {
Ok(mut h) => h.stop(),
Err(poisoned) => {
tracing::warn!("Handler mutex poisoned during shutdown; recovering guard to stop handler");
let mut h = poisoned.into_inner();
h.stop()
}

Copilot uses AI. Check for mistakes.
}
})
.await
.map_err(|e| BoxliteError::Internal(format!("spawn_blocking join: {}", e)))??;
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: the spawn_blocking join error message here doesn't indicate which operation failed. Adding context (e.g., stop spawn_blocking join) and/or the box_id would make operational debugging significantly easier.

Suggested change
.map_err(|e| BoxliteError::Internal(format!("spawn_blocking join: {}", e)))??;
.map_err(|e| {
BoxliteError::Internal(format!(
"stop spawn_blocking join (box_id={}): {}",
self.config.id, e
))
})??;

Copilot uses AI. Check for mistakes.
}

// Clean up PID file (single source of truth)
Expand Down