Skip to content

nvx-rs: reserve the launch of a leased clone, and document the lock order - #886

Merged
Pedro Henrique Penna (ppenna) merged 3 commits into
devfrom
esaurez/839-pr4-supervisor-seam-20261024
Oct 27, 2026
Merged

Pedro Henrique Penna (ppenna) merged 3 commits into
devfrom
esaurez/839-pr4-supervisor-seam-20261024

Conversation

@esaurez

@esaurez Enrique Saurez (esaurez) commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Chain: #885 → this PR → #884 → #887 → #888, on dev; #866, #882, #883, #876, #871, #873, #874, #879, and #881 have merged.

A clone's lease must outlive every process that may restore its generation, through a crash of the orchestrator at any step of the clone's launch, and a crash must never leave a VMM that no record names. #885 writes the lease, and the clone binding beside the sandbox's launch journal, before any VMM of the clone exists; this ties the lease to the launch that it reserves, and lets the supervisor launch under an identity that its caller reserved. It also documents one lock order for the template store, the sandbox, and the scratch image, and tests the crashes of a clone's orchestrator at each step, in processes of their own.

Part of #839: the last of its four pull requests. The first, #879, adds the compatibility key, the second, #881, the store, and the third, #885, eligibility, leases, tombstones, and retirement.

What changed

The supervisor seam (host/supervisor, host/store). supervisor::launch and LockedStore::begin_launch take the launch's identity from their caller, who reserves it before the launch begins, with LaunchId::generate, now public, a fresh one for each launch. The CLI, Instance, and the runtime adapter draw one just before they launch, so their launches do as before.

A lease reserves its clone's launch (template/store/leases.rs). Store::acquire draws the identity of the clone's launch, and records it, in the lease's record and in the clone binding, before it returns, so that the reservation is durable before any VMM of the clone exists. Lease::launch returns it. A lease allows one launch of its clone, under the lock that acquired it, whatever the launch's outcome: while a sandbox holds a lease, begin_launch refuses, with sandbox-leased, a launch under another identity, the reserved launch under another lock, and a second launch. The lock that acquire returns holds the grant in memory, and the launch's journal consumes it. A release ends the lease once the clone's VM has exited, and another acquisition, which checks the generation's eligibility again, reserves another launch.

Stale handles. Once the VMM exits, Vmm::stop and Vmm::terminate clear the runtime record only if it is still the VM's own record, field for field. A record that named the same launch used to suffice, but callers now choose launch identities and could use one again, so a stale handle could have cleared the record of another VM.

The lock order (supervisor module documentation, "Clones"). Transitions take their locks in one order: the template store's lock, then the sandbox's lifecycle lock, which guards both its clone binding and its launch journal, then the scratch image's claim. None waits for a lock while it holds a later one: an acquisition releases the store's lock before it returns, a release, a launch, a stop, and a reattach take no lock of the store, and a retirement takes no lock of a sandbox. No lock lasts for a VM's lifetime: the caller releases the sandbox's lock once the launch commits or ends, and the scratch image's claim belongs to the VMM. The documentation also lists what a crash at each step leaves, and which evidence of a VM's exit ends its lease.

Public API changes to review

  • LaunchId::generate is public.
  • LockedStore::begin_launch(&mut self, launch: &LaunchId, runtime, capability) and supervisor::launch(locked, reserved: &LaunchId, runtime, capability, command) take the launch's identity: a breaking change for their callers.
  • template::Lease::launch.
  • The lease's record and the clone binding gain a required launch field. STORE_FORMAT stays 1: nothing has released the records that nvx-rs: lease eligible templates to clones, and retire them #885 writes, which lack it.
  • sandbox-leased also refuses a launch that a sandbox's lease does not allow. Its documentation, in nvx::ErrorCode, the README, and doc/usage.md, says so.

Assumptions to review

  • One launch per lease. A launch that fails, even before its VMM runs, ends what the lease allows: the caller releases the lease, and acquires another.
  • The grant lives in the lock that acquire returns, so the caller keeps that lock from the acquisition to the launch, and claims the clone's scratch image under it. A crash in between leaves a lease that launches nothing, which a release ends at once.
  • The clone binding and the launch journal share the sandbox's lifecycle lock, so the issue's order, the index, then the binding, the journal, and the scratch claim, takes three locks.

Validation

At 7c97c9c, the head, rebased on #885's 3fbaf8a, on Windows and on Linux x86_64:

  • cargo fmt --check; cargo clippy with --all-targets --all-features, --lib, and --lib --no-default-features, -D warnings; cargo test with all, default, and no features, among them tests/host_modules.rs, the layering of nvx::host; cargo doc with -D warnings, also of private items on Windows; cargo +1.89 check;
  • tests/openvmm_vectors.rs with --ignored, against the OpenVMM submodule at its pin, e68871c;
  • the runtime adapter's protocol-only and host-only configurations, which call supervisor::launch, with their MSRV checks, and, on Linux, its guest-only configuration;
  • the tests of the bundled artifacts of nvx, against a synthetic bundle;
  • the template and sandbox-store tests repeatedly: twice more on Windows and three times on Linux;
  • scripts/test_nvx_tools.py's check that doc/usage.md documents every error code, and the check of the Markdown links;
  • on Linux, also the macOS check, and the guest binaries' check, build, and tests;
  • at each of its commits, on both hosts: cargo fmt --check, cargo clippy with --all-targets --all-features, --lib, and --lib --no-default-features, -D warnings, tests/host_modules.rs, and the runtime adapter's host-only Clippy, and, on Linux, the macOS check and the runtime adapter's guest-only Clippy; on Linux, Clippy with --all-targets --all-features fails at the first two commits, on the unused import that 7c97c9c removes, as below.

The lease tests check that a lease allows one launch of its clone, under the lock that acquired it, and that its records keep the reserved launch. The crash tests each run a clone's orchestrator in a process of its own, which exits at one step: after the lease's record, after the binding, once acquired, before the VMM's spawn, after it, after the VMM's identity is recorded, once the launch commits, and while it releases. Each test then checks what a release, a reattach, and a retirement do with what the crash left. The lease lasts while the clone's VMM may run, even one that the journal does not name yet, and a release ends it once that VMM has exited. The supervisor's tests check the evidence of a leased clone's exit: an earlier boot and a reused process ID end the lease; a process that cannot be examined keeps it; and so does a forced teardown that fails, until a teardown ends the VMM. They also check that a stop keeps the record of another VMM under the same launch identity.

Every check passed on both hosts at the head.

A final review by a separate model found no significant issues. It reviewed this change before its restack on #885's latest commits. The restack changed none of this change's code: it merged three of its documentation paragraphs with #885's new text, and the documentation of a clone's release now says that the release takes the sandbox's lock through the lease's path, as #885 requires. A later restack, onto #885's a7e68ad, which makes a removal of a sandbox remove its clone binding first and durably, merged one more paragraph, the documentation of Lease, and changed no code either.

Copilot's review then found that four lease tests began a launch, dropped it, and asserted that the next recovery ends it at once. On Linux, a process that a concurrent test forks holds a copy of the closed log until it executes its program, so such a recovery can be indeterminate. 26b78dc retries those recoveries until they end the launch, through test_support::released, as the sandbox store's tests do. Two of the four tests come from #885, whose versions of them have the same pattern.

The restack on #874, through #879, #881, and #885, resolved one conflict, in the runtime adapter's host, whose launch dev changed meanwhile. The host keeps dev's extendable deadline and its passing of the prepared launch's checked files to OpenVMM, and launches under the reserved identity through a named command closure. That closure's command now borrows the prepared launch, so its return type names an inferred lifetime rather than 'static: the only change to code. The second commit now changes only this pull request's two lease tests, since #885 makes the same change to its own. A final review of the restack by a separate model found no issues.

A second restack, on #874's fa45e48, which drops the configuration identity of activation records, through #879, #881, and #885, replayed this pull request's commits without conflicts, and changed no code. A final review of it by a separate model found no issues.

A third restack, onto #885's e69106b, which changes only a Windows test of #885, replayed this pull request's commits without conflicts, and changed no code. A final review of it by a separate model found no issues.

The rebase onto dev, after #879 merged, through #881 and #885, moves the lease that a sandbox's clone binding records, with the reading of the template store's records, into nvx::host::store, whose launch reads the binding, and re-exports them at their paths in nvx::host::template: dev checks that the modules of nvx::host depend on one another without cycles, and nvx::host::template depends on nvx::host::store. The public API, the records, their messages, and the lock order are unchanged. The rebase also keeps dev's import of the spawned VMM in the sandbox store, and drops this pull request's change to a test of a crate that dev removed. The race of a retirement and releases ran 320 times again at the head, in eight parallel copies, with no failure. A final review of the rebase by a separate model found no issues. Clippy on Linux then found that the lease tests imported the clone binding's description at their top, although only their Windows tests use it; 7c97c9c imports it in those tests alone, and a second review found no issues.

NVX CI does not run until this pull request targets dev; a run dispatched on the head passed: run 40558055011.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cross-platform crash-recovery guarantees and breaking lifecycle APIs warrant final human review, despite no concrete defects identified.

No open issues 🎉

What changed in this PR

This PR completes the supervisor and lock-order portion of #839, building on #885’s template leases. It ties each lease to one reserved clone launch and strengthens protection against stale VMM handles.

Changes:

  • Accept caller-reserved launch identities and restrict each lease to one launch under the acquiring lock.
  • Preserve another VM’s runtime record when stopping through a stale handle.
  • Document lock ordering and add tests for clone-launch crashes, recovery, and lease release.
File Description
tests/​runtime-adapter/​src/​bin/​runtime-adapter-host.rs Reserve an identity before launching through the supervisor.
nvx-rs/​tests/​supervisor.rs Update supervisor fixtures for explicit launch identities.
nvx-rs/​tests/​store.rs Update store integration tests for explicit launch identities.
nvx-rs/​src/​host/​template/​store/​leases/​tests.rs Test launch reservations and crashes across acquisition, launch, and release.
nvx-rs/​src/​host/​template/​store/​leases.rs Persist reserved launch identities and manage the acquiring lock’s grant.
nvx-rs/​src/​host/​template/​store.rs Document reservations and add a shared test publication helper.
nvx-rs/​src/​host/​supervisor/​tests.rs Test exit evidence, failed teardown, and stale-handle protection.
nvx-rs/​src/​host/​supervisor/​mod.rs Accept reserved identities, compare full runtime records, and document lock order.
nvx-rs/​src/​host/​store/​tests.rs Adapt store unit tests to caller-supplied identities.
nvx-rs/​src/​host/​store/​record.rs Make LaunchId::generate public and document single-launch use.
nvx-rs/​src/​host/​store/​mod.rs Enforce lease reservations and consume the grant when launch begins.
nvx-rs/​src/​host/​instance.rs Generate and pass an identity for instance launches.
nvx-rs/​src/​host/​cli/​sandbox/​lifecycle.rs Generate and pass an identity for CLI sandbox launches.
nvx-rs/​src/​error.rs Expand sandbox-leased documentation to cover launch restrictions.
nvx-rs/​README.md Describe reserved launches, lock ordering, and lease restrictions.
doc/​usage.md Update the documented meaning of sandbox-leased.
aci_edge_sandboxes/​tests/​openvmm_fake.rs Adapt fake-VMM tests to the changed launch API.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-platform crash-consistency guarantees warrant human review, and the lease test’s recovery assertions need bounded retries.

1 open issue

Review effort: Balanced

Comment thread nvx-rs/src/host/template/store/leases/tests.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-platform crash consistency and breaking lifecycle APIs warrant final human review despite no confirmed blocking defect.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-process lease durability and crash recovery across Linux and Windows require final human review, despite no confirmed defects.

No open issues 🎉

Review effort: Balanced

@esaurez
Enrique Saurez (esaurez) force-pushed the esaurez/839-pr3-leases-20261024 branch from 4a8b534 to c2aa75a Compare October 27, 2026 04:39
@esaurez
Enrique Saurez (esaurez) force-pushed the esaurez/839-pr4-supervisor-seam-20261024 branch from b7ac52f to 061e368 Compare October 27, 2026 04:39
@esaurez
Enrique Saurez (esaurez) requested a balanced review from Copilot October 27, 2026 04:40
@esaurez
Enrique Saurez (esaurez) force-pushed the esaurez/839-pr3-leases-20261024 branch from e69106b to 3fbaf8a Compare October 27, 2026 17:29
Enrique Saurez and others added 2 commits October 27, 2026 10:44
…rder

A clone's lease now reserves the launch of its clone. The acquisition draws
the launch's identity, records it in the lease's record and in the clone
binding before any VMM of the clone exists, and returns it as
Lease::launch. The supervisor takes the launch's identity from its caller:
supervisor::launch and LockedStore::begin_launch take a LaunchId, which
callers that launch no clone draw with LaunchId::generate, now public.

A lease allows one launch of its clone, under the lock that acquired it:
begin_launch refuses, with sandbox-leased, any other launch of a sandbox
that holds a lease, a launch under another lock, and a second launch, until
a release ends the lease. After a crash before the launch, a release ends
the lease at once, and another acquisition checks the generation's
eligibility again.

A stop or a termination clears the runtime record only if it is still the
record of its own VM, and not another VMM's under a launch identity that
its caller used again.

The supervisor's documentation describes the launch of a clone, its
crashes, and one lock order: the template store's lock, then the sandbox's
lifecycle lock, which guards its clone binding and launch journal, then the
scratch image's claim. No lock lasts for a VM's lifetime.

Tests crash a clone's orchestrator, in a process of its own, at each step
of an acquisition and of a release, and around the spawn of the clone's
VMM, and check what each evidence of a VM's exit means for its lease: an
earlier boot, a reused process ID, a process that cannot be examined, and a
forced teardown that fails.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc335726-838c-4538-a3c4-d6791aba5101
Four lease tests began a launch, dropped it, and asserted that the
next recovery ends it. On Linux, a process that a concurrent test
forks holds a copy of the closed log until it executes its program.
If that outlasts recovery's own wait, the recovery is indeterminate
and the test fails although nothing is wrong.

The tests now retry the recovery until it ends the launch, through
test_support::released, as the sandbox store's tests do.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc335726-838c-4538-a3c4-d6791aba5101

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-process crash recovery, lease durability, and breaking launch APIs warrant final human review despite no actionable defects identified.

No open issues 🎉

Review effort: Balanced

…ase tests

The clone binding's description, `BINDING_DESCRIPTION`, moved to
`nvx::host::store::lease` with the lease, so that `nvx::host::store`
does not depend on `nvx::host::template`. The lease tests then imported
it at their top, although only their Windows tests use it: on other
platforms the import was unused, and Clippy, which denies warnings,
failed. The Windows tests now import it themselves.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: bc335726-838c-4538-a3c4-d6791aba5101

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Cross-process crash consistency and breaking launch APIs warrant human review, with exact-head Linux and Windows validation still pending.

No open issues 🎉

Review effort: Balanced

Base automatically changed from esaurez/839-pr3-leases-20261024 to dev October 27, 2026 19:06
@ppenna
Pedro Henrique Penna (ppenna) marked this pull request as draft October 27, 2026 19:12
@ppenna
Pedro Henrique Penna (ppenna) marked this pull request as ready for review October 27, 2026 19:12
@ppenna
Pedro Henrique Penna (ppenna) merged commit 9346454 into dev Oct 27, 2026
86 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the esaurez/839-pr4-supervisor-seam-20261024 branch October 27, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants