Skip to content

Improve InterruptHandle::kill() documentation and clarify boolean return value #1121

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

Draft
Copilot wants to merge 10 commits into
base: main
Choose a base branch
from
+13 −5
Draft

Improve InterruptHandle::kill() documentation and clarify boolean return value #1121

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f936bc0
Initial plan
Copilot Dec 16, 2025
c34ddfa
Remove misleading boolean return from InterruptHandle::kill()
Copilot Dec 16, 2025
c8e471e
Add error logging for WHvCancelRunVirtualProcessor failure
Copilot Dec 16, 2025
5350a8c
Update kill() documentation to accurately describe timing behavior
Copilot Dec 16, 2025
ad4591d
Make tracing-tracy optional to fix gdb test build failures
Copilot Dec 16, 2025
0daa0fd
Restore boolean return value from InterruptHandle::kill() with improv…
Copilot Dec 18, 2025
21cdb54
Improve unsafe block scope in Windows kill() implementation
Copilot Dec 18, 2025
28890e7
Revert Cargo.toml changes to tracing-tracy
Copilot Dec 18, 2025
8b7a60c
Remove error logging for WHvCancelRunVirtualProcessor failure
Copilot Dec 18, 2025
533f3a8
Improve kill() documentation with clearer structure
Copilot Dec 18, 2025
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
7 changes: 6 additions & 1 deletion src/hyperlight_host/Cargo.toml
View file
Open in desktop
Copy link
Contributor

@jsturtevant jsturtevant Dec 18, 2025

Choose a reason for hiding this comment

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

@copilot don't make these changes

Copy link
Contributor Author

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Reverted Cargo.toml changes in commit 28890e7. The tracing-tracy dependency is now back to being a regular dev-dependency as it was originally.

Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ tracing = { version = "0.1.43", features = ["log"] }
tracing-log = "0.2.0"
tracing-core = "0.1.35"
tracing-opentelemetry = { version = "0.32.0", optional = true }
tracing-tracy = { version = "0.11.4", optional = true }
hyperlight-common = { workspace = true, default-features = true, features = [ "std" ] }
hyperlight-guest-tracing = { workspace = true, default-features = true, optional = true }
vmm-sys-util = "0.15.0"
Expand Down Expand Up @@ -104,7 +105,6 @@ criterion = "0.8.1"
tracing-chrome = "0.7.2"
metrics-util = "0.20.1"
metrics-exporter-prometheus = { version = "0.18.1", default-features = false }
tracing-tracy = "0.11.4"
serde_json = "1.0"
hyperlight-component-macro = { workspace = true }

Expand All @@ -130,6 +130,7 @@ print_debug = []
# Dumps the VM state to a file on unexpected errors or crashes. The path of the file will be printed on stdout and logged.
crashdump = ["dep:chrono"]
trace_guest = ["dep:opentelemetry", "dep:tracing-opentelemetry", "dep:hyperlight-guest-tracing", "hyperlight-common/trace_guest"]
trace_tracy = ["dep:tracing-tracy"]
mem_profile = [ "trace_guest", "dep:framehop", "dep:fallible-iterator", "hyperlight-common/mem_profile" ]
kvm = ["dep:kvm-bindings", "dep:kvm-ioctls"]
mshv3 = ["dep:mshv-bindings", "dep:mshv-ioctls"]
Expand All @@ -139,6 +140,10 @@ fuzzing = ["hyperlight-common/fuzzing"]
build-metadata = ["dep:built"]
init-paging = []

[[example]]
name = "tracing-tracy"
required-features = ["trace_tracy"]

[[bench]]
name = "benchmarks"
harness = false
22 changes: 19 additions & 3 deletions src/hyperlight_host/src/hypervisor/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,17 @@ pub(crate) trait InterruptHandleImpl: InterruptHandle {
pub trait InterruptHandle: Send + Sync + Debug {
/// Interrupt the corresponding sandbox from running.
///
/// - If this is called while the the sandbox currently executing a guest function call, it will interrupt the sandbox and return `true`.
/// - If this is called while the sandbox is not running (for example before or after calling a guest function), it will do nothing and return `false`.
/// This method sets a cancellation flag that prevents or stops the execution of guest code.
///
/// # Return Value
///
/// The return value indicates whether a signal was sent to interrupt a running vCPU:
/// - On Linux: Returns `true` if a signal was sent to the vCPU thread, `false` if the vCPU was not running.
/// - On Windows: Returns `true` if `WHvCancelRunVirtualProcessor` was called successfully, `false` otherwise.
///
/// **Important**: A return value of `false` does not mean the cancellation failed. The cancellation flag is
/// always set, which will prevent or stop execution. A `false` return simply means no signal was sent because
/// the vCPU was not actively running at that moment.
Copy link
Contributor

@ludfjig ludfjig Dec 18, 2025

Choose a reason for hiding this comment

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

I think something like

/// Cancel guest execution in the corresponding sandbox.
///
/// If a guest call is in progress, it will be cancelled and the guest function call
/// will return an error. If called before a guest call is made, it has no effect on
/// future guest calls.
///
/// # Returns
/// - `true` if a guest call was in progress and the vcpu was actively running.
/// - `false` otherwise (no guest call in progress, or guest call in progress but vcpu
///   not yet running). Any in-progress guest call will still be cancelled.
///
/// # Platform behavior
/// - **Linux**: If the vcpu is running, this function will block for the duration of
///   the time it takes for the vcpu thread to be interrupted.
/// - **Windows**: This function returns immediately after requesting cancellation.
fn kill(&self) -> bool;

would be better @copilot

Copy link
Contributor Author

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Updated the documentation in commit 533f3a8. The new format is clearer and more user-focused, with separate sections for the return value and platform-specific behavior.

///
/// # Note
/// This function will block for the duration of the time it takes for the vcpu thread to be interrupted.
Expand Down Expand Up @@ -542,7 +551,14 @@ impl InterruptHandle for WindowsInterruptHandle {
return false;
}

unsafe { WHvCancelRunVirtualProcessor(guard.handle, 0, 0).is_ok() }
Copy link
Contributor

@jsturtevant jsturtevant Dec 18, 2025

Choose a reason for hiding this comment

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

@copilot Lets skip this, we have logging in other places

Copy link
Contributor Author

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Removed the error logging for WHvCancelRunVirtualProcessor in commit 8b7a60c. The method now simply returns is_ok() without additional logging.

let result = unsafe { WHvCancelRunVirtualProcessor(guard.handle, 0, 0) };
match result {
Ok(_) => true,
Err(e) => {
log::error!("Failed to cancel running virtual processor: {}", e);
false
}
}
}
#[cfg(gdb)]
fn kill_from_debugger(&self) -> bool {
Expand Down