Add method for recreating unrecoverable sandbox - #1751
Conversation
|
@syntactically first draft at #1747 |
ludfjig
added
the
kind/enhancement
ludfjig
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a public MultiUseSandbox::recreate() API in hyperlight-host to rebuild an unrecoverable sandbox from scratch using its existing configuration and the snapshot that triggered the unrecoverable restore failure (addressing #1747’s “replace this sandbox with a new one with the same configuration” request).
Changes:
- Track additional lifecycle state needed for recreation (store the failed-restore snapshot and sandbox config when transitioning to
Unrecoverable). - Add
MultiUseSandbox::recreate(self) -> Result<Self>plus tests validating what state/config is preserved vs not preserved. - Update errors, changelog, and test recipes to cover the new recreation flow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Thread sandbox configuration into MultiUseSandbox::from_uninit so it can be reused for recreation. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Introduce SandboxLifecycle, store recreation snapshot/config, and implement MultiUseSandbox::recreate() with accompanying tests. |
| src/hyperlight_host/src/sandbox/host_funcs.rs | Add an internal constructor to rebuild HostFunctions from a FunctionRegistry during recreation. |
| src/hyperlight_host/src/error.rs | Add recreation-specific error variants and adjust unrecoverable messaging. |
| Justfile | Ensure the new unrecoverable-mapping test is exercised in crashdump/coverage recipes. |
| CHANGELOG.md | Document the new MultiUseSandbox::recreate() API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// The sandbox cannot safely perform further operations and must be discarded. | ||
| #[error("The sandbox is unrecoverable and must be discarded")] | ||
| #[error("The sandbox is unrecoverable and must be recreated or discarded")] | ||
| UnrecoverableSandbox, |
| /// Recreation requires [`Unrecoverable`](SandboxStatus::Unrecoverable) | ||
| /// sandbox status and is unsupported when the `gdb` Cargo feature is | ||
| /// enabled. Any error consumes the sandbox. |
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
9e79d10 to
f806823
Compare
syntactically
left a comment
syntactically
left a comment
There was a problem hiding this comment.
I left a few comments inline.
I'm still trying to work out what the best API here is. It feels to me like it is a smaller/less breaking change to move this into restore() and add a new way to opt-out of the implicit recreate, but the possibility of recreation itself failing (which is probably pretty likely if we've recently failed to restore, since the relevant mapping failures seem likely to be resource exhaustion) is a little difficult to handle ergonomically. It seems inescapable to me that client code, if it is going to handle this locally, will need to do something like
while should_keep_trying() {
match sb.<restore/recreate/whatever>(snapshot) {
Ok(()) => break,
Err(HyperlightError::UnrecoverableSandbox) => {
// maybe try to free up resources somehow, try a different snapshot, etc
continue;
},
Err(e) => return e,
}which is just kind of nasty, whether you have one function for restore/recreate or multiple.
(The other option for clients I guess would be to just back out all the way to some top level retry when they get an unrecoverable error. That probably doesn't benefit at all from having recreate functionality at all, though...)
| self.0 | ||
| } | ||
|
|
||
| #[cfg(not(gdb))] |
There was a problem hiding this comment.
Why is so much stuff not(gdb) guarded?
| /// again on the rebuilt sandbox. | ||
| /// | ||
| /// Existing interrupt handles do not apply to the rebuilt sandbox. Call | ||
| /// [`interrupt_handle()`](Self::interrupt_handle) on the rebuilt sandbox to |
There was a problem hiding this comment.
This seems like a nasty footgun. Is there really no way to get around this? I guess we should just need to update the partition handle here on the WHP sandboxes? I would expect the linux/hvf ones might just work, since the tid update happens just before run? If the latter is true, maybe we can just check/update the partition id on the whp sandboxes before run so that the lifecycle is the same?
| if let Err(error) = self.restore_memory_and_mappings(&snapshot) { | ||
| self.status = SandboxStatus::Unrecoverable; | ||
| self.lifecycle = SandboxLifecycle::Unrecoverable { | ||
| recreation_snapshot: snapshot, |
There was a problem hiding this comment.
Why do we want to save the snapshot here rather than in self.snapshot?
| Poisoned, | ||
| Unrecoverable { | ||
| #[cfg_attr(gdb, allow(dead_code))] | ||
| recreation_snapshot: Arc<Snapshot>, |
There was a problem hiding this comment.
I think with the current lifecycle of MultiUseSandbox::snapshot, this means we can only ever transition into the unrecoverable state from restore(), since we are not guaranteed to have a valid snapshot around otherwise? That doesn't seem right to me---surely we should be able to end up in unrecoverable from e.g. map_region as well, even if we don't have a current valid snapshot? Like with poisoned, I would expect that the new snapshot should be supplied on the recreate operation?
Closes #1747