Introduce Unrecoverable sandbox state by ludfjig · Pull Request #1727 · hyperlight-dev/hyperlight

ludfjig · 2026-08-12T16:55:03Z

Some failures can leave sandboxes in a state that cannot be restored. The ones this PR is concerned with is if mapping/unmapping memory hv-calls. In certain cases, if these fails for example during call to restore() the state of the sandbox can be unknown, and we should prevent further use.

This PR introduces such state as Unrecoverable. Unrecoverable sandboxes can no longer be used at all, and users need to create new one. I don't expect that this will be common at all, but it was an existing gap in our codebase that should be addressed.

This pr also tries to be a bit more transactional in certain operations, meaning it either fully completes successfully or errors cleanly without leaving sandbox in invalid state.

Copilot

Pull request overview

This PR extends the Hyperlight host sandbox lifecycle model by introducing an explicit unrecoverable state for MultiUseSandbox instances when certain restore failures (notably VM base mapping updates) leave the sandbox unsafe to continue using. It also updates the public API and tests to query sandbox lifecycle via a new status() method returning SandboxStatus.

Changes:

Add SandboxStatus (Ready | Poisoned | Unrecoverable) and expose MultiUseSandbox::status() (deprecating poisoned()).
Make restore() treat base mapping update failures as terminal by marking the sandbox Unrecoverable and rejecting future operations.
Add hypervisor test fault-injection helpers and expand restore-related tests to cover recoverable vs unrecoverable failure modes.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/hyperlight_host/tests/sandbox_host_tests.rs	Update tests to use `status().is_poisoned()` instead of `poisoned()`.
src/hyperlight_host/tests/integration_test.rs	Update integration assertions to use `SandboxStatus`/`status()`.
src/hyperlight_host/src/sandbox/snapshot/file_tests.rs	Update snapshot tests to check poison via `status()`.
src/hyperlight_host/src/sandbox/mod.rs	Re-export `SandboxStatus` from the sandbox module.
src/hyperlight_host/src/sandbox/initialized_multi_use.rs	Introduce `SandboxStatus`, implement `status()`/`ensure_usable()`, update poison/restore behavior, and add new restore failure-mode tests.
src/hyperlight_host/src/lib.rs	Re-export `SandboxStatus` from the crate root.
src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs	Add test-only VM fault injection and base mapping state helpers.
src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs	Adjust snapshot/scratch mapping update logic and expose test support module.
src/hyperlight_host/src/error.rs	Add `HyperlightError::UnrecoverableSandbox` and classify it as non-poisoning.
CHANGELOG.md	Document the new lifecycle API and unrecoverable behavior.

Suppressed comments (1)

src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs:561

update_scratch_mapping has the same transactional issue as update_snapshot_mapping: it unmaps the old scratch region before mapping the new one, but only updates self.scratch_memory after the new map succeeds. If map_memory fails, scratch_memory still points at memory that has been unmapped in the VM, leaving the internal state inconsistent.

        if let Some(old_scratch) = self.scratch_memory.as_ref() {
            let old_base = hyperlight_common::layout::scratch_base_gpa(old_scratch.mem_size());
            let old_rgn = old_scratch.mapping_at(old_base, MemoryRegionType::Scratch);
            self.vm.unmap_memory((self.scratch_slot, &old_rgn))?;
        }
        unsafe { self.vm.map_memory((self.scratch_slot, &rgn))? };
        self.scratch_memory = Some(scratch);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

ludfjig · 2026-08-13T23:17:55Z

+        if let Some(old_snapshot) = self.snapshot_memory.as_ref() {
            let old_rgn = old_snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);
            self.vm.unmap_memory((self.snapshot_slot, &old_rgn))?;
        }
        unsafe { self.vm.map_memory((self.snapshot_slot, &rgn))? };
+        self.snapshot_memory = Some(snapshot);


Not an issue, the sandbox will is unrecoverable in this case

I think the comment from Copilot is valid, this is true now that we introduce unrecoverable errors, but if that ever changes for this operation, it would be a bug.
And especially since it's easy to fix, just before map_memory you can do

self.snapshot_memory = None;

jsturtevant · 2026-08-14T20:51:26Z

LGTM

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

jprendes

I like this PR! Good job!
LGTM.

Just some tiny nits

jprendes · 2026-08-19T08:25:58Z

        let rgn = snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);

-        if let Some(old_snapshot) = self.snapshot_memory.replace(snapshot) {
+        if let Some(old_snapshot) = self.snapshot_memory.as_ref() {


nit: could you add a comment here so that future us doesn't try to be smart and refactor it back to the old implementation?
same below for scratch.

jprendes · 2026-08-19T08:31:14Z

+        if let Some(old_snapshot) = self.snapshot_memory.as_ref() {
            let old_rgn = old_snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);
            self.vm.unmap_memory((self.snapshot_slot, &old_rgn))?;
        }
        unsafe { self.vm.map_memory((self.snapshot_slot, &rgn))? };
+        self.snapshot_memory = Some(snapshot);


I think the comment from Copilot is valid, this is true now that we introduce unrecoverable errors, but if that ever changes for this operation, it would be a bug.
And especially since it's easy to fix, just before map_memory you can do

self.snapshot_memory = None;

syntactically

This looks good. My one high-level question is whether it we could/it would make sense to handle the unrecoverable state transparently (making the API the same as poisoned) by changing the restore operation to do something like if self.status == SandboxStatus::Unrecoverable { *self = MultiUseSandbox::new(/* copy over host functions and runtime config */); }?

(Or, if that's too much to do automatically, is it worth providing an explicit helper for either the "replace this sandbox with a new one with the same configuration" and/or "reset to snapshot, first replacing this sandbox if needed" operations?)

syntactically · 2026-08-19T11:23:02Z

 pub type PtRootFinder = Box<dyn Fn(&[u8], &[u8], u64) -> Vec<u64> + Send>;

 impl MultiUseSandbox {
+    fn ensure_usable(&self) -> Result<()> {


ensure_usable is a little bit ambiguous as a name, since it could also mean a function that makes sure the present sandbox is in fact usable.

good point renamed to check_ready

syntactically · 2026-08-19T11:24:40Z

        })?;
-        // TODO (ludfjig): Go through the rest of possible errors in this `MultiUseSandbox::restore` function
-        // and determine if they should also poison the sandbox.
+


Is this moved after the sregs reset on purpose?

yes it is, I thought sregs check should fail first

syntactically · 2026-08-19T11:26:40Z

-        // and determine if they should also poison the sandbox.
+
+        if let Err(error) = self.restore_memory_and_mappings(&snapshot) {
+            self.status = SandboxStatus::Unrecoverable;


Would it be preferable to extract this out into a should_make_unrecoverable, like we have should_poison?

syntactically · 2026-08-19T11:27:18Z

            let path = simple_guest_as_pathbuf();
            let mut cfg = SandboxConfiguration::default();
            cfg.set_heap_size(0x20_000);
+            cfg.set_scratch_size(0x60_000);


Do you have any idea why we needed to change this?

whoops good catch we don't need this at all. This test is getting removed in next pr anyway

ludfjig added the kind/enhancement For PRs adding features, improving functionality, docs, tests, etc. label Aug 12, 2026

ludfjig force-pushed the ll/snapshot-restore-transaction branch 6 times, most recently from eb0b4b3 to da5b23c Compare August 13, 2026 21:13

ludfjig changed the base branch from main to ll/simplify_gdb August 13, 2026 21:17

ludfjig force-pushed the ll/snapshot-restore-transaction branch from da5b23c to c180248 Compare August 13, 2026 21:38

ludfjig force-pushed the ll/snapshot-restore-transaction branch from c180248 to b48dd56 Compare August 13, 2026 22:44

ludfjig marked this pull request as ready for review August 13, 2026 22:54

ludfjig requested review from danbugs and devigned as code owners August 13, 2026 22:54

Copilot AI lite review requested due to automatic review settings August 13, 2026 22:54

ludfjig requested review from andreiltd, dblnz, jprendes, jsturtevant, simongdavies, squillace and syntactically as code owners August 13, 2026 22:54

Copilot started reviewing on behalf of ludfjig August 13, 2026 22:58 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Base automatically changed from ll/simplify_gdb to main August 14, 2026 00:15

ludfjig force-pushed the ll/snapshot-restore-transaction branch from b48dd56 to 47aa821 Compare August 14, 2026 00:15

jsturtevant reviewed Aug 14, 2026

View reviewed changes

Comment thread src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs

ludfjig added the ready-for-review PR is ready for (re-)review label Aug 17, 2026

ludfjig force-pushed the ll/snapshot-restore-transaction branch 5 times, most recently from e6e0e69 to 34acaf1 Compare August 18, 2026 17:45

Add unrecoverable sandbox state

c78559a

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

ludfjig force-pushed the ll/snapshot-restore-transaction branch from 34acaf1 to c78559a Compare August 18, 2026 21:13

jprendes reviewed Aug 19, 2026

View reviewed changes

jprendes approved these changes Aug 19, 2026

View reviewed changes

syntactically reviewed Aug 19, 2026

View reviewed changes

Uh oh!

Conversation

ludfjig commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsturtevant commented Aug 14, 2026

Uh oh!

jprendes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syntactically left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ludfjig commented Aug 12, 2026 •

edited

Loading