Skip to content

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

Open
Enrique Saurez (esaurez) wants to merge 2 commits into
esaurez/839-pr3-leases-20261024from
esaurez/839-pr4-supervisor-seam-20261024
Open

Enrique Saurez (esaurez) wants to merge 2 commits into
esaurez/839-pr3-leases-20261024from
esaurez/839-pr4-supervisor-seam-20261024

Conversation

@esaurez

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

Copy link
Copy Markdown
Contributor

Stacked on #885; this PR retargets to dev when the pull requests below it merge. The chain on dev is #883 → #876 → #871 → #873 → #874 → #879 → #881 → #885 → #886, after #863, #864, #865, #866, #867, #868, #869, #870, #872, #875, #878, and #882, which 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 061e368, the head, restacked on #885's c2aa75a, on Windows and on Linux x86_64:

  • cargo fmt --check; cargo clippy with --all-targets --all-features and --lib --no-default-features, -D warnings; cargo test with all, default, and no features; 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, a12d916;
  • 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 checks of aci_edge_sandboxes, which builds on nvx: cargo fmt --check, cargo clippy with all features and without default features, cargo test with all and default features, the agent's bundling, cargo doc, and its MSRV check, and, on Linux, its macOS check;
  • 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;
  • 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 and --lib --no-default-features, -D warnings, the runtime adapter's host-only Clippy, and aci_edge_sandboxes's Clippy with all features, and, on Linux, the macOS check and the runtime adapter's guest-only Clippy.

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.

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.

NVX CI does not run until this pull request targets dev.

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

Enrique Saurez and others added 2 commits October 26, 2026 21:08
…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
@esaurez
Enrique Saurez (esaurez) force-pushed the esaurez/839-pr3-leases-20261024 branch from 4a8b534 to c2aa75a Compare October 27, 2026 04:39

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 platform-specific crash recovery warrant final human review, despite no confirmed blocking defect.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
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