Introduce Unrecoverable sandbox state

[ludfjig]

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.

[jsturtevant]

LGTM

[jprendes]

I like this PR! Good job!
LGTM.

Just some tiny nits

[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?)

[ludfjig]

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?)

I do like the first idea, but I am a little bit worried about it doing too much automatically. It could be a major perf hit if say somebody runs it to it on each restore, and they wouldn't even know that it's happening.

I do like the second idea better, my only hesitation is I haven't looked through it enough to know if it's even possible right now? But it would be something we should consider adding, since it probably could be useful for users.

[syntactically]

I do like the first idea, but I am a little bit worried about it doing too much automatically. It could be a major perf hit if say somebody runs it to it on each restore, and they wouldn't even know that it's happening.

The "invisible perf hit" nature could be mitigated a little bit by emitting metrics tracking it? But I agree that it's not ideal. We could also update the existing restore and add a "restore_unless_unrecoverable" method or something?

I do like the second idea better, my only hesitation is I haven't looked through it enough to know if it's even possible right now? But it would be something we should consider adding, since it probably could be useful for users.

I think it might be worth checking if a really simple *self = Self::new(self.host_functions, self.rt_fg) equivalent works and adding it to this PR if it does, but up to you.

[jprendes]

restore_unless_unrecoverable

my 2 cents, I vote for keeping plain restore, every other method is basically _unless_poisoned_or_unrecoverable, but we don't make that explicit either.

[ludfjig]

I think it might be worth checking if a really simple *self = Self::new(self.host_functions, self.rt_fg) equivalent works and adding it to this PR if it does, but up to you.

Since it would be a pure addition, I'd rather merge this first and do that separate

[syntactically]

my 2 cents, I vote for keeping plain restore, every other method is basically _unless_poisoned_or_unrecoverable, but we don't make that explicit either.

The idea would be to keep restore (and have it emit a metric + automatically replace the sandbox if unrecoverable) as the default, simpler path for most users, and then find some sensible name for an alternative for users who care about handling it specially.

Since it would be a pure addition, I'd rather merge this first and do that separate

I suggested waiting mostly because we are planning a release soon and I would like to make sure we have thought through the quality of life (especially "should restore do anything by default", since that probably is a breaking change) before making a release with this, since most downstream consumers of hyperlight will probably make the change to replace unrecoverable sandboxes when we do this release and then be unlikely to move to take advantage of anything we do to abstract away the reinitialisation.