From 216bf6fb59bfa55f0477f0807d87142398dc0635 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 00:25:02 -0700 Subject: [PATCH 01/14] nvx-rs: give the store its callers' files, lifecycle locks, and removal Each orchestrator of a sandbox kept its configuration record beside the store, with its own lock and removal protocol around the store's lock. The store now owns that protocol, so that every orchestrator shares it: - CallerFiles declares the caller's configuration record, whose presence means that the sandbox is provisioned, and its other files there. - SandboxStore::lock_for_provision creates the directory, again if a concurrent removal deletes it, and SandboxStore::lock_existing fails with the new ErrorKind::NotFound once the directory is gone. - A holder that leaves the directory without the configuration record removes its own lock file as it releases the lock. - LockedStore::remove refuses while a VM or a launch is recorded, removes the store's files and the caller's, the configuration record last, and refuses and keeps foreign entries. Otherwise it removes its lock file while it still holds the lock, and then the directory, or returns a Lingering removal while lock files of other transitions keep the emptied directory. RemovalError names each failure. SandboxStore::create and open fail with ErrorKind::NotFound rather than ErrorKind::Io for a missing parent or directory. On Windows, a lock file that a file system without POSIX deletion keeps after its removal is taken for a replaced one rather than failing the lock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/README.md | 3 +- nvx-rs/src/error.rs | 9 + nvx-rs/src/host/python/fs.rs | 4 - nvx-rs/src/host/store/caller.rs | 353 +++++++++++ nvx-rs/src/host/store/lifecycle.rs | 491 +++++++++++++++ nvx-rs/src/host/store/lifecycle/tests.rs | 754 +++++++++++++++++++++++ nvx-rs/src/host/store/mod.rs | 211 +++++-- nvx-rs/src/host/store/tests.rs | 4 +- 8 files changed, 1772 insertions(+), 57 deletions(-) create mode 100644 nvx-rs/src/host/store/caller.rs create mode 100644 nvx-rs/src/host/store/lifecycle.rs create mode 100644 nvx-rs/src/host/store/lifecycle/tests.rs diff --git a/nvx-rs/README.md b/nvx-rs/README.md index ec1500309..fb656da71 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -74,7 +74,7 @@ nvx-rs/ | `nvx::host::network` | `NetworkPolicy` and `openvmm_arguments`: the egress default and rules, ingress, host-loopback access and forwards as OpenVMM options. Egress rules are lowered exactly into OpenVMM's IPv4 and IPv6 rules by the lowering of egress policy files, with at most 256 rules for each action | | `nvx::host::network::policy` | Egress policy files: `compile_policy_file`, `compile_policy_bytes`, and `compile_policy` compile one to a `CompiledEgressPolicy` of native rules; see [Egress policy files](#egress-policy-files) | | `nvx::host::filesystem` | The `ExportPlan` of host paths through OpenVMM's aggregate virtio-fs device, the host-mapping table, the reserved guest targets, and `guest_path` and `resolve_guest_path` (also at `nvx::`) | -| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated | +| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory | | `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive. Under the store's lock, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; and `LockedInstance::stop` stops the VM with a forced fallback. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | | `nvx::host::nvxs` | `HostConnection`: the host's role of the NVXS session over a platform transport, on which `nvx::host::session` and other host applications run their protocols. It attaches with the launch capability, waiting while the broker asks it to, sends application bytes in as many DATA records as they take, and returns each DATA payload from the guest, with deadlines; a receive that times out keeps a partial record | | `nvx::host::process` | Verified process references. A `ProcessRef` names one process by its ID, its start time, and the `BootId` of the host boot during which it started. `Process::open` checks a reference and holds its process through a pidfd on Linux or a process handle on Windows, whether or not the caller started it, so that waits sleep until the process exits and termination never reaches a process that reuses its ID. A reference from an earlier boot names an exited process, and a process whose identity this user cannot check, such as another user's, makes `Process::open` fail rather than report an exit. Linux hosts without pidfds poll the reference instead, and cannot terminate the process | @@ -624,6 +624,7 @@ as is, an `ErrorKind`, and an optional cause. | `Unsupported` | A well-formed request that NVX cannot represent or enforce, such as a policy beyond a limit of OpenVMM or the guest agent | | `Exited` | The VM's OpenVMM process exited | | `Io` | A host I/O failure, or state that NVX recorded on the host and cannot read | +| `NotFound` | State that an operation names and that does not exist on the host, such as the store of a sandbox that was never created or that a removal deleted | | `Protocol` | A control session that failed, or a request that the guest agent refused | | `TimedOut` | A deadline that passed | | `Other` | Any other failure | diff --git a/nvx-rs/src/error.rs b/nvx-rs/src/error.rs index db12019ff..e63479a0d 100644 --- a/nvx-rs/src/error.rs +++ b/nvx-rs/src/error.rs @@ -21,6 +21,9 @@ pub enum ErrorKind { /// A host I/O operation failed, or state that NVX recorded on the host is unreadable or /// malformed. Io, + /// The state that an operation names does not exist on the host, such as the store of a + /// sandbox that was never created or that a removal deleted. + NotFound, /// The control session failed: the peer violated the protocol, closed or reset the session, /// or refused a request. Protocol, @@ -38,6 +41,7 @@ impl ErrorKind { Self::Unsupported => "unsupported", Self::Exited => "exited", Self::Io => "io", + Self::NotFound => "not-found", Self::Protocol => "protocol", Self::TimedOut => "timed-out", Self::Other => "other", @@ -95,6 +99,11 @@ impl Error { Self::new(ErrorKind::Io, message) } + /// Creates an [`ErrorKind::NotFound`] error. + pub fn not_found(message: impl Into) -> Self { + Self::new(ErrorKind::NotFound, message) + } + /// Creates an [`ErrorKind::Protocol`] error. pub fn protocol(message: impl Into) -> Self { Self::new(ErrorKind::Protocol, message) diff --git a/nvx-rs/src/host/python/fs.rs b/nvx-rs/src/host/python/fs.rs index 6a25c9769..2ea2f7143 100644 --- a/nvx-rs/src/host/python/fs.rs +++ b/nvx-rs/src/host/python/fs.rs @@ -79,7 +79,6 @@ pub(crate) fn stat(path: &Path) -> io::Result { } /// Returns `os.lstat(path)`, which does not follow a link that `path` names. -#[cfg(feature = "cli")] pub(crate) fn lstat(path: &Path) -> io::Result { imp::lstat(path) } @@ -160,7 +159,6 @@ mod imp { } /// See [`super::lstat`]. - #[cfg(feature = "cli")] pub(super) fn lstat(path: &Path) -> io::Result { status(fs::symlink_metadata(path)?) } @@ -317,7 +315,6 @@ mod imp { } /// See [`super::lstat`]. - #[cfg(feature = "cli")] pub(super) fn lstat(path: &Path) -> io::Result { let status = win32::stat(path, false)?; Ok(Stat { @@ -512,7 +509,6 @@ mod imp { Err(unsupported()) } - #[cfg(feature = "cli")] pub(super) fn lstat(_path: &Path) -> io::Result { Err(unsupported()) } diff --git a/nvx-rs/src/host/store/caller.rs b/nvx-rs/src/host/store/caller.rs new file mode 100644 index 000000000..0c9ebc3c9 --- /dev/null +++ b/nvx-rs/src/host/store/caller.rs @@ -0,0 +1,353 @@ +//! The files that a store's caller keeps in the sandbox's directory beside the store's own, and the +//! names of the temporary files that the store owns there. + +use super::FILE_NAMES; +use crate::error::{Error, Result}; + +/// The files that the caller of a [`SandboxStore`](super::SandboxStore) keeps in the sandbox's +/// directory beside the store's own: the configuration record whose presence means that the +/// sandbox is provisioned, if the caller keeps one, and the caller's other files, such as the +/// socket of a boot console. +/// +/// The store's [lifecycle locks](super#lifecycle-locks) and its [removal](super#removal) take +/// the declaration: a holder that leaves the directory without the configuration record removes +/// its lock file as it releases the lock, and removal removes the caller's files with the store's, +/// the configuration record last. A caller that declares no configuration record, as +/// [`none`](Self::none) declares, keeps nothing that tells whether its sandbox is provisioned, +/// so the directory itself is the sandbox: its lock file stays when the lock is released. +/// +/// Each name is one plain file name, which [`new`](Self::new) and [`with`](Self::with) check, so +/// a declaration is valid before any store is created, opened, locked, or removed with it: +/// +/// - It is not empty, `.`, or `..`, and has no path separator, `/` or `\` on any host, so it +/// names no root, prefix, or path outside the directory. +/// - It has none of the characters that Windows refuses in a file name, a control character, +/// `"`, `*`, `:`, `<`, `>`, `?`, or `|`, no trailing dot or space, which Windows drops, and it is +/// not a name that Windows reserves for a device, such as `NUL` or `com1.json`, so it names the +/// same file on every host. +/// - It is none of the store's [`FILE_NAMES`], the lock included, does not have the form of a +/// temporary file that the store owns, `.NAME.NUMBER.tmp`, or of a lock file that a removal +/// waits for, `.nfs*`. +/// - It is declared once: the configuration record's name differs from the others. +/// +/// Names are compared without regard to case, as Windows and macOS file systems compare them. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct CallerFiles { + /// The configuration record, whose presence means that the sandbox is provisioned. + config: Option, + /// The caller's other files. + others: Vec, +} + +impl CallerFiles { + /// Declares no files: the caller keeps nothing in the sandbox's directory, which is itself the + /// sandbox. + pub fn none() -> Self { + Self::default() + } + + /// Declares `config`, the caller's configuration record, whose presence in the directory means + /// that the sandbox is provisioned. + /// + /// Fails with [`ErrorKind::Unsupported`](crate::ErrorKind::Unsupported) for a name that is not + /// one plain file name of the caller's, as the [type](Self) describes. + pub fn new(config: impl Into) -> Result { + let config = config.into(); + let mut files = Self::none(); + files.check(&config)?; + files.config = Some(config); + Ok(files) + } + + /// Adds `name`, another file of the caller's. + /// + /// Fails with [`ErrorKind::Unsupported`](crate::ErrorKind::Unsupported) for a name that is not + /// one plain file name of the caller's, as the [type](Self) describes, or that is declared + /// already. + pub fn with(mut self, name: impl Into) -> Result { + let name = name.into(); + self.check(&name)?; + self.others.push(name); + Ok(self) + } + + /// Returns the name of the caller's configuration record, if it keeps one. + pub fn config(&self) -> Option<&str> { + self.config.as_deref() + } + + /// Returns the names of the caller's other files. + pub fn others(&self) -> &[String] { + &self.others + } + + /// Returns every declared name, in the order in which removal removes them: the other files, + /// then the configuration record. + pub(super) fn names(&self) -> impl Iterator { + self.others + .iter() + .map(String::as_str) + .chain(self.config.as_deref()) + } + + /// Fails if `name` cannot be declared beside the names declared so far. + fn check(&self, name: &str) -> Result<()> { + match name_problem(name).or_else(|| self.collision(name)) { + Some(problem) => Err(Error::unsupported(format!( + "the caller's file name {name:?} {problem}" + ))), + None => Ok(()), + } + } + + /// Returns why `name` cannot be declared beside the store's files, its temporary files, and + /// the names declared so far, comparing names without regard to case. + fn collision(&self, name: &str) -> Option<&'static str> { + let folded = name.to_lowercase(); + if FILE_NAMES.contains(&folded.as_str()) { + Some("is the name of one of the store's files") + } else if is_store_temporary(&folded) { + Some("has the form of a temporary file that the store owns") + } else if folded.starts_with(".nfs") { + Some("has the form of a removed lock file that a removal waits for") + } else if self + .names() + .any(|declared| declared.to_lowercase() == folded) + { + Some("is declared already") + } else { + None + } + } +} + +/// Returns why `name` is not one plain file name that names the same file on every host, if it is +/// not. +fn name_problem(name: &str) -> Option<&'static str> { + if name.is_empty() { + Some("is empty") + } else if name == "." || name == ".." { + Some("names a directory rather than a file in it") + } else if name.contains(['/', '\\']) { + Some("has a path separator") + } else if name + .chars() + .any(|c| c.is_control() || matches!(c, '"' | '*' | ':' | '<' | '>' | '?' | '|')) + { + Some("has a character that Windows refuses in a file name") + } else if name.ends_with(['.', ' ']) { + Some("ends with a dot or a space, which Windows drops") + } else if reserves_device(name) { + Some("is a name that Windows reserves for a device") + } else { + None + } +} + +/// Returns whether Windows takes `name` for a device: a name whose part before its first dot, +/// without trailing spaces, is `CON`, `PRN`, `AUX`, `NUL`, `CONIN$`, `CONOUT$`, or `COM` or `LPT` +/// and one digit, which may be a superscript one, two, or three, in any case. +fn reserves_device(name: &str) -> bool { + let stem = name + .split('.') + .next() + .unwrap_or_default() + .trim_end_matches(' ') + .to_ascii_uppercase(); + let numbered = |prefix: &str| { + stem.strip_prefix(prefix).is_some_and(|rest| { + let mut digits = rest.chars(); + matches!(digits.next(), Some('0'..='9' | '¹' | '²' | '³')) && digits.next().is_none() + }) + }; + matches!( + stem.as_str(), + "CON" | "PRN" | "AUX" | "NUL" | "CONIN$" | "CONOUT$" + ) || numbered("COM") + || numbered("LPT") +} + +/// Returns whether `name` is a temporary file that the store leaves beside one of its files when a +/// replacement of the file is interrupted: `.NAME.NUMBER.tmp`. +pub(super) fn is_store_temporary(name: &str) -> bool { + name.strip_prefix('.') + .and_then(|name| name.strip_suffix(".tmp")) + .and_then(|name| name.rsplit_once('.')) + .is_some_and(|(file, number)| { + FILE_NAMES.contains(&file) + && !number.is_empty() + && number.bytes().all(|byte| byte.is_ascii_digit()) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ErrorKind; + + /// Returns the message with which declaring `config`, and then `others`, fails. + fn refusal(config: &str, others: &[&str]) -> String { + let declared = CallerFiles::new(config).and_then(|files| { + others + .iter() + .try_fold(files, |files, &name| files.with(name)) + }); + let error = declared.unwrap_err(); + assert_eq!(error.kind(), ErrorKind::Unsupported, "{error}"); + error.message().to_owned() + } + + #[test] + fn declarations_keep_their_names() { + let files = CallerFiles::new("sandbox.json") + .unwrap() + .with("boot.sock") + .unwrap() + .with(".config.json.1.tmp") + .unwrap(); + assert_eq!(files.config(), Some("sandbox.json")); + assert_eq!(files.others(), ["boot.sock", ".config.json.1.tmp"]); + assert_eq!( + files.names().collect::>(), + ["boot.sock", ".config.json.1.tmp", "sandbox.json"] + ); + let none = CallerFiles::none().with("boot.sock").unwrap(); + assert_eq!(none.config(), None); + assert_eq!(none.names().collect::>(), ["boot.sock"]); + assert_eq!(CallerFiles::none(), CallerFiles::default()); + } + + #[test] + fn names_that_leave_the_directory_are_refused_on_every_host() { + for (name, problem) in [ + ("", "is empty"), + (".", "names a directory rather than a file in it"), + ("..", "names a directory rather than a file in it"), + ("/etc/passwd", "has a path separator"), + ("../config.json", "has a path separator"), + ("state/config.json", "has a path separator"), + ("config.json/", "has a path separator"), + ("C:\\config.json", "has a path separator"), + ("..\\config.json", "has a path separator"), + ("\\\\?\\C:\\config.json", "has a path separator"), + ("\\", "has a path separator"), + ("C:", "has a character that Windows refuses in a file name"), + ( + "C:config.json", + "has a character that Windows refuses in a file name", + ), + ( + "config.json:stream", + "has a character that Windows refuses in a file name", + ), + ( + "config\0.json", + "has a character that Windows refuses in a file name", + ), + ( + "config\n.json", + "has a character that Windows refuses in a file name", + ), + ( + "config?.json", + "has a character that Windows refuses in a file name", + ), + ( + "config.json.", + "ends with a dot or a space, which Windows drops", + ), + ( + "config.json ", + "ends with a dot or a space, which Windows drops", + ), + ("NUL", "is a name that Windows reserves for a device"), + ("nul.json", "is a name that Windows reserves for a device"), + ("Com1.json", "is a name that Windows reserves for a device"), + ("lpt\u{b9}", "is a name that Windows reserves for a device"), + ("CON .json", "is a name that Windows reserves for a device"), + ("conout$", "is a name that Windows reserves for a device"), + ] { + let expected = format!("the caller's file name {name:?} {problem}"); + assert_eq!(refusal(name, &[]), expected); + assert_eq!(refusal("config.json", &[name]), expected); + } + // Names that only resemble those are plain file names. + for name in [ + "...json", + "com10", + "console.json", + "nul-file", + "lpt", + "a b.json", + ] { + CallerFiles::new(name).unwrap(); + } + } + + #[test] + fn names_that_collide_are_refused_whatever_their_case() { + for (name, problem) in [ + ("sandbox.lock", "is the name of one of the store's files"), + ("Runtime.JSON", "is the name of one of the store's files"), + ("outcome.json", "is the name of one of the store's files"), + ( + ".runtime.json.12345.tmp", + "has the form of a temporary file that the store owns", + ), + ( + ".SANDBOX.LOCK.1.TMP", + "has the form of a temporary file that the store owns", + ), + ( + ".nfs000000001", + "has the form of a removed lock file that a removal waits for", + ), + ( + ".NFS1", + "has the form of a removed lock file that a removal waits for", + ), + ] { + let expected = format!("the caller's file name {name:?} {problem}"); + assert_eq!(refusal(name, &[]), expected); + assert_eq!(refusal("config.json", &[name]), expected); + } + let twice = |name: &str| format!("the caller's file name {name:?} is declared already"); + assert_eq!( + refusal("config.json", &["config.json"]), + twice("config.json") + ); + assert_eq!( + refusal("config.json", &["CONFIG.json"]), + twice("CONFIG.json") + ); + assert_eq!( + refusal("config.json", &["boot.sock", "Boot.Sock"]), + twice("Boot.Sock") + ); + let none = CallerFiles::none().with("boot.sock").unwrap(); + assert_eq!( + none.with("BOOT.sock").unwrap_err().message(), + twice("BOOT.sock") + ); + } + + #[test] + fn store_temporaries_name_one_of_the_stores_files_and_a_number() { + for name in [ + ".runtime.json.0.tmp", + ".sandbox.lock.18446744073709551615.tmp", + ] { + assert!(is_store_temporary(name), "{name}"); + } + for name in [ + ".config.json.1.tmp", + ".runtime.json..tmp", + ".runtime.json.1a.tmp", + "runtime.json.1.tmp", + ".runtime.json.1.tmp.tmp", + ".Runtime.json.1.tmp", + ] { + assert!(!is_store_temporary(name), "{name}"); + } + } +} diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs new file mode 100644 index 000000000..acb077b7f --- /dev/null +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -0,0 +1,491 @@ +//! The lifecycle of a sandbox's store beside its caller's files: the locks of a provision and of an +//! existing sandbox, the release of a lock in a directory that holds no provisioned sandbox, and +//! the removal of the store and its directory. The [module documentation](super#lifecycle-locks) +//! describes their rules. + +use std::fmt; +use std::fs::{self, File}; +use std::io; +use std::path::{Path, PathBuf}; +use std::thread; +use std::time::Instant; + +use super::caller::is_store_temporary; +use super::{ + CallerFiles, FILE_NAMES, JOURNAL_NAME, LOCK_NAME, LOCK_RETRY_INTERVAL, LockedStore, + RUNTIME_NAME, SandboxStore, Taken, absolute, +}; +use crate::error::{Error, ErrorKind, Result}; +use crate::host::platform; +use crate::host::python::fs::lstat; +use crate::host::python::not_unicode; + +#[cfg(test)] +mod tests; + +impl SandboxStore { + /// Takes the lock of the sandbox in `dir` for its provision, by a caller that keeps `files` + /// in the directory, waiting for it until `deadline`, or for as long as it takes without one. + /// + /// The directory, whose parent must exist, is created if needed, private to the current user, + /// as [`create`](Self::create) creates it, and created again if a concurrent removal deletes + /// it before the lock is taken. The caller checks under the lock whether its configuration + /// record exists already, and writes it. + /// + /// Fails with [`ErrorKind::TimedOut`] when the deadline passes first, also while Windows + /// refuses to open a removed lock file, as [`lock_existing`](Self::lock_existing) describes, + /// with [`ErrorKind::NotFound`] if the parent does not exist, and as [`create`](Self::create) + /// fails for a directory that is not private. + pub fn lock_for_provision( + dir: impl Into, + files: &CallerFiles, + deadline: Option, + ) -> Result { + let dir = absolute(dir.into())?; + loop { + let store = Self::create(dir.clone())?; + match store.take_lifecycle_lock(deadline) { + Ok(Taken::Locked(lock)) => { + return Ok(LockedStore::new(store, lock, files.clone())); + } + // A removal deleted the lock file, and maybe the directory, while this waited, or + // the directory after it was created: it is created and locked again. + Ok(Taken::Replaced) => {} + Err(error) if error.kind() == ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + } + } + + /// Takes the lock of the existing sandbox in `dir`, by a caller that keeps `files` in the + /// directory, waiting for it until `deadline`, or for as long as it takes without one. + /// + /// Fails with [`ErrorKind::NotFound`] if the directory does not exist, also once a concurrent + /// removal deletes it while this waits: a lock file that is removed or replaced meanwhile + /// locks nothing, so the lock of the lock file at the path is taken again, as + /// [`lock`](Self::lock) does, which fails once the directory is gone. A directory that exists + /// is locked even if it holds no provisioned sandbox, as one that was never provisioned, or + /// that an interrupted removal emptied, or whose removal lingering lock files still keep: the + /// caller checks its configuration record under the lock, and the release then removes the + /// lock file again. + /// + /// On Windows, a file system without POSIX deletion keeps a removed lock file under its name + /// until its last handle closes, and refuses to open it until then, so the open is tried + /// again every 25 ms. Fails with [`ErrorKind::TimedOut`] when the deadline passes first, as + /// [`lock`](Self::lock) does. + pub fn lock_existing( + dir: impl Into, + files: &CallerFiles, + deadline: Option, + ) -> Result { + let store = Self::open(dir)?; + loop { + if let Taken::Locked(lock) = store.take_lifecycle_lock(deadline)? { + return Ok(LockedStore::new(store, lock, files.clone())); + } + } + } + + /// Takes the lock as [`take_lock`](Self::take_lock) does, and opens a lock file that Windows + /// refuses to open again every 25 ms until `deadline`, as a removed one that other handles + /// still hold, which a file system without POSIX deletion keeps until they close. + fn take_lifecycle_lock(&self, deadline: Option) -> Result { + loop { + match self.take_lock(deadline) { + Err(error) if cfg!(windows) && denied(&error) => { + if !pause(deadline) { + return Err(self.transition_in_progress()); + } + } + taken => return taken, + } + } + } +} + +/// Returns whether the store's `error` is an I/O error whose cause is a denied access. +fn denied(error: &Error) -> bool { + error.kind() == ErrorKind::Io + && std::error::Error::source(error) + .and_then(|source| source.downcast_ref::()) + .is_some_and(|source| source.kind() == io::ErrorKind::PermissionDenied) +} + +/// Waits before the next try of a step that `deadline` bounds: returns `false` at once if the +/// deadline has passed, and otherwise sleeps for [`LOCK_RETRY_INTERVAL`] or the time left, +/// whichever is shorter, and returns `true`. +fn pause(deadline: Option) -> bool { + let left = deadline.map(|deadline| deadline.saturating_duration_since(Instant::now())); + if left.is_some_and(|left| left.is_zero()) { + return false; + } + thread::sleep(left.map_or(LOCK_RETRY_INTERVAL, |left| left.min(LOCK_RETRY_INTERVAL))); + true +} + +impl Drop for LockedStore { + /// Releases the lock. A holder that leaves the directory without its caller's configuration + /// record removes its own lock file first, unless it removed it already, so that a directory + /// that holds no provisioned sandbox keeps no lock file, and empties. + fn drop(&mut self) { + let Some(config) = self.files.config() else { + return; + }; + if self.lock_removed + || !matches!( + fs::symlink_metadata(self.path(config)), + Err(error) if error.kind() == io::ErrorKind::NotFound + ) + { + return; + } + let path = self.path(LOCK_NAME); + let held = platform::opened_file_identity(&self.lock); + if matches!((held, platform::file_identity(&path)), (Ok(held), Ok(current)) if held == current) + { + let _ = fs::remove_file(&path); + } + } +} + +impl LockedStore { + /// Removes the sandbox's store, its caller's files, and its directory, which only a sandbox + /// that no VMM can run in allows, and releases the lock. + /// + /// The removal refuses, and removes nothing, while the store records a VM or a launch: the + /// caller reattaches to the VM and stops it, or recovers the launch, first, as + /// [`supervisor::reattach`](crate::host::supervisor::reattach) does. It then removes the + /// store's files but its lock, the caller's files, its configuration record last, so that an + /// interrupted removal leaves a provisioned sandbox, which can be removed again, and the + /// temporary files that the store owns. It refuses the entries that are neither the store's + /// nor the caller's, which it names in the order in which the directory lists them, and keeps + /// them and the directory; the lock file goes as the lock is released, since the configuration + /// record is gone. Otherwise it removes its lock file while it still holds the lock, so that a + /// transition that waited for the lock finds the sandbox gone rather than halfway removed, and + /// then the directory. + /// + /// Lock files of other transitions can keep the emptied directory: on NFS, a removed lock file + /// that a handle still holds is renamed to `.nfs*`; a Windows file system without POSIX + /// deletion keeps one under its own name until its last handle closes; and a transition that + /// opens the lock file between the removals of the lock file and of the directory creates a + /// new one. Each of those transitions finds no sandbox, and removes its lock file as it + /// releases its lock, so the directory empties. The removal then holds the directory's + /// identity, releases the lock, and returns the [`Lingering`] removal, whose + /// [`wait`](Lingering::wait) removes the directory once it empties. It returns `None` once the + /// directory is gone. + pub fn remove(self) -> std::result::Result, RemovalError> { + self.remove_contents()?; + self.remove_directory() + } + + /// Empties the directory but for the lock file, as [`remove`](Self::remove) does first: it + /// refuses while the store records a VM or a launch, removes the store's files and the + /// caller's, and the temporary files that the store owns, and refuses any other entry. + fn remove_contents(&self) -> std::result::Result<(), RemovalError> { + for name in [JOURNAL_NAME, RUNTIME_NAME] { + let path = self.path(name); + match fs::symlink_metadata(&path) { + Ok(_) => return Err(RemovalError::Launched), + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(RemovalError::Io { path, error }), + } + } + let owned: Vec = FILE_NAMES + .into_iter() + .filter(|&name| name != LOCK_NAME) + .chain(self.files.names()) + .map(|name| self.path(name)) + .collect(); + for path in owned { + remove_entry(path)?; + } + let listed = entries(self.dir()).map_err(|error| RemovalError::Io { + path: self.dir().to_owned(), + error, + })?; + let mut foreign = Vec::new(); + for name in listed { + if name == LOCK_NAME { + continue; + } + if is_store_temporary(&name) { + remove_entry(self.path(&name))?; + } else { + foreign.push(name); + } + } + if foreign.is_empty() { + Ok(()) + } else { + Err(RemovalError::Foreign(foreign)) + } + } + + /// Removes the lock file while it still holds the lock, and then the directory, as + /// [`remove`](Self::remove) does once the directory holds nothing else of the sandbox. + fn remove_directory(mut self) -> std::result::Result, RemovalError> { + let dir = self.dir().to_owned(); + let lock = self.path(LOCK_NAME); + fs::remove_file(&lock).map_err(|error| RemovalError::Io { path: lock, error })?; + self.lock_removed = true; + match fs::remove_dir(&dir) { + Ok(()) => return Ok(None), + Err(error) if directory_not_empty(&error) => {} + Err(error) => return Err(RemovalError::Io { path: dir, error }), + } + let (identity, held) = hold_directory(&dir).map_err(|error| RemovalError::Io { + path: dir.clone(), + error, + })?; + let config = self.files.config().map(str::to_owned); + drop(self); + Ok(Some(Lingering { + dir, + identity, + _held: held, + config, + retried_at_once: false, + })) + } +} + +/// The removal of a sandbox's directory that lock files of other transitions keep, which +/// [`LockedStore::remove`] returns once it has removed everything else and released the lock. +/// +/// It holds the identity of the directory that it removes: on Unix, its open handle keeps a +/// directory that replaces it from reusing its inode number, and NTFS and ReFS do not reuse file +/// IDs. The path may name another directory once this one is gone, such as one that a provision +/// created again, which is not this removal's to remove. +#[derive(Debug)] +pub struct Lingering { + /// The directory. + dir: PathBuf, + /// The identity of the directory, which `lstat` gives. + identity: (u64, u128), + /// The open directory on Unix, which keeps its identity. + _held: Option, + /// The caller's configuration record, which a provision of the path writes. + config: Option, + /// Whether the latest attempt was the one that followed at once a failure without remaining + /// entries. + retried_at_once: bool, +} + +impl Lingering { + /// Returns the directory that the removal waits for. + pub fn dir(&self) -> &Path { + &self.dir + } + + /// Removes the directory once lingering lock files no longer keep it, trying again every 25 + /// ms until `deadline`, or for as long as it takes without one. A deadline that has passed + /// still tries once, so a caller that waits on its own, as for an interruption, calls this + /// again, and a removal that timed out can go on. + /// + /// Returns once the directory is gone, whoever removed it, or once the path names another + /// directory. Fails, and keeps the directory, with [`RemovalError::ProvisionedAgain`] once + /// the caller's configuration record appears in it, as a provision that took the lock after + /// the removal released it writes it, with [`RemovalError::Foreign`] for other entries that + /// are not lingering lock files, sorted, and with [`RemovalError::TimedOut`] once the deadline + /// passes. + pub fn wait(&mut self, deadline: Option) -> std::result::Result<(), RemovalError> { + loop { + let error = match fs::remove_dir(&self.dir) { + Ok(()) => return Ok(()), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => error, + }; + if directory_status(&self.dir) != Some(self.identity) { + return Ok(()); + } + let remaining = match entries(&self.dir) { + Ok(remaining) => remaining, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(RemovalError::Io { + path: self.dir.clone(), + error, + }); + } + }; + let mut unexpected: Vec = remaining + .iter() + .filter(|name| !lingers(name)) + .cloned() + .collect(); + unexpected.sort(); + // A provision that took the lock after the removal released it, before the directory + // went, provisions the directory again. + if self + .config + .as_ref() + .is_some_and(|config| unexpected.contains(config)) + { + return Err(RemovalError::ProvisionedAgain); + } + if !unexpected.is_empty() { + return Err(RemovalError::Foreign(unexpected)); + } + if !directory_not_empty(&error) { + return Err(RemovalError::Io { + path: self.dir.clone(), + error, + }); + } + // Without remaining entries, the last lingering lock file went away after the removal + // failed, so the next attempt follows at once. Another such failure right after it + // pauses first, so that a file system that does not report emptiness right away + // cannot spin. + if remaining.is_empty() && !self.retried_at_once { + self.retried_at_once = true; + continue; + } + self.retried_at_once = false; + if !pause(deadline) { + return Err(RemovalError::TimedOut(error)); + } + } + } +} + +/// Why [`LockedStore::remove`] or [`Lingering::wait`] kept a sandbox's directory. +#[derive(Debug)] +#[non_exhaustive] +pub enum RemovalError { + /// The store records a VM or a launch, so a VMM of the sandbox may still run, and nothing was + /// removed. + Launched, + /// The directory holds entries that are neither the store's nor its caller's, which the + /// removal keeps, with the directory: their names, in the order in which the directory lists + /// them, or sorted once the removal waited for lingering lock files. + Foreign(Vec), + /// The sandbox was provisioned again while the removal waited for lingering lock files: the + /// caller's configuration record appeared in the directory, which the removal keeps. + ProvisionedAgain, + /// Lingering lock files still kept the directory when the deadline passed: the error with + /// which the latest attempt to remove it failed. + TimedOut(io::Error), + /// An operation on one of the sandbox's files, or on its directory, failed. + Io { + /// The file or directory. + path: PathBuf, + /// The error of the operation. + error: io::Error, + }, +} + +impl fmt::Display for RemovalError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Launched => { + formatter.write_str("the sandbox's store records a VM or a launch, which may run") + } + Self::Foreign(names) => write!( + formatter, + "the sandbox's directory holds entries that are neither the store's nor its \ + caller's: {}", + names.join(", ") + ), + Self::ProvisionedAgain => formatter + .write_str("the sandbox was provisioned again while its directory was removed"), + Self::TimedOut(_) => formatter + .write_str("lock files of other transitions still keep the sandbox's directory"), + Self::Io { path, .. } => write!(formatter, "cannot remove {}", path.display()), + } + } +} + +impl std::error::Error for RemovalError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::TimedOut(error) | Self::Io { error, .. } => Some(error), + _ => None, + } + } +} + +/// Keeps the message of a removal's error, and its I/O cause: a timeout is of +/// [`ErrorKind::TimedOut`], an I/O failure of [`ErrorKind::Io`], and every other refusal of +/// [`ErrorKind::Other`]. +impl From for Error { + fn from(error: RemovalError) -> Self { + let message = error.to_string(); + match error { + RemovalError::TimedOut(source) => Error::timed_out(message).with_source(source), + RemovalError::Io { error: source, .. } => Error::io(message).with_source(source), + _ => Error::other(message), + } + } +} + +/// Removes the file at `path` if it exists. +fn remove_entry(path: PathBuf) -> std::result::Result<(), RemovalError> { + match fs::remove_file(&path) { + Err(error) if error.kind() != io::ErrorKind::NotFound => { + Err(RemovalError::Io { path, error }) + } + _ => Ok(()), + } +} + +/// Returns whether `name` is a removed lock file that a handle still keeps: NFS renames a removed +/// file that is still open to `.nfs*`, and Windows file systems without POSIX deletion keep it +/// under its own name, as they keep the lock files that waiting transitions create until they +/// release them. +fn lingers(name: &str) -> bool { + name == LOCK_NAME || name.starts_with(".nfs") +} + +/// Returns whether `error` is how the removal of a directory reports one that is not empty. +fn directory_not_empty(error: &io::Error) -> bool { + #[cfg(windows)] + let not_empty = matches!(error.raw_os_error(), Some(145 | 80 | 183)); + #[cfg(unix)] + let not_empty = matches!( + error.raw_os_error(), + Some(code) if code == nix::errno::Errno::ENOTEMPTY as i32 + || code == nix::errno::Errno::EEXIST as i32 + ); + #[cfg(not(any(windows, unix)))] + let not_empty = error.kind() == io::ErrorKind::DirectoryNotEmpty; + not_empty +} + +/// Returns the names of the entries of `directory`, in the order in which it lists them. +fn entries(directory: &Path) -> io::Result> { + fs::read_dir(directory)? + .map(|entry| { + entry? + .file_name() + .into_string() + .map_err(|_| not_unicode("a file name")) + }) + .collect() +} + +/// Returns the identity of the directory `path`, as `lstat` gives it, and a handle that keeps it: +/// on Unix, the open directory keeps a directory that replaces it from reusing its inode number, +/// and NTFS and ReFS do not reuse file IDs. +fn hold_directory(path: &Path) -> io::Result<((u64, u128), Option)> { + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + + let file = fs::OpenOptions::new() + .read(true) + .custom_flags((nix::fcntl::OFlag::O_DIRECTORY | nix::fcntl::OFlag::O_NOFOLLOW).bits()) + .open(path)?; + // The open directory's status gives the identity that `lstat` gives for its path. + let metadata = file.metadata()?; + Ok(((metadata.dev(), u128::from(metadata.ino())), Some(file))) + } + #[cfg(not(unix))] + { + Ok((lstat(path)?.identity(), None)) + } +} + +/// Returns the identity that `lstat` gives for `path`, or `None` if it cannot be examined. +fn directory_status(path: &Path) -> Option<(u64, u128)> { + lstat(path).ok().map(|status| status.identity()) +} diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs new file mode 100644 index 000000000..dc8b2f7a4 --- /dev/null +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -0,0 +1,754 @@ +//! Tests of the lifecycle locks, their release, and the removal of a store, by threads of one +//! process, and by transitions in processes of their own, which [`transition_role`] runs. + +use std::env; +use std::io::{BufRead, BufReader, Read}; +use std::process::{Child, ChildStdout, Command, Stdio}; +use std::sync::mpsc; +use std::time::Duration; + +use super::*; +use crate::host::store::{CAPABILITY_NAME, CONSOLE_NAME, LOG_NAME, OUTCOME_NAME, SOCKET_NAME}; +use crate::host::test_support::released; + +/// The configuration record of these tests' caller. +const CONFIG: &str = "config.json"; +/// Another file of these tests' caller. +const BOOT: &str = "boot.sock"; +/// A name that NFS gives a removed lock file that a transition still holds open. +const RENAMED: &str = ".nfs00000000000000010000"; +/// The directory of the sandbox whose lock [`transition_role`] takes. +const ROLE_DIR: &str = "NVX_LIFECYCLE_TEST_DIR"; +/// The lock that [`transition_role`] takes: `provision` or `existing`. +const ROLE_LOCK: &str = "NVX_LIFECYCLE_TEST_LOCK"; +/// The line that [`transition_role`] prints before it waits for the lock. +const WAITING: &str = "nvx-lifecycle-test-waiting"; +/// The start of the line in which [`transition_role`] prints how its transition ended. +const OUTCOME: &str = "nvx-lifecycle-test-outcome "; + +/// Returns the files of these tests' caller. +fn files() -> CallerFiles { + CallerFiles::new(CONFIG).unwrap().with(BOOT).unwrap() +} + +/// Returns the deadline `seconds` from now. +fn within(seconds: u64) -> Option { + Some(Instant::now() + Duration::from_secs(seconds)) +} + +/// Returns the names of the entries of `dir`, sorted. +fn entries_of(dir: &Path) -> Vec { + let mut names = entries(dir).unwrap(); + names.sort(); + names +} + +/// Returns `names` as sorted strings. +fn sorted(names: &[&str]) -> Vec { + let mut names: Vec = names.iter().map(|&name| name.to_owned()).collect(); + names.sort(); + names +} + +/// Provisions a sandbox in a fresh temporary directory, and returns the directory of its store. +fn sandbox() -> (tempfile::TempDir, PathBuf) { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + let locked = SandboxStore::lock_for_provision(&dir, &files(), None).unwrap(); + assert!(!dir.join(CONFIG).exists()); + fs::write(dir.join(CONFIG), b"{}").unwrap(); + drop(locked); + (directory, dir) +} + +/// Takes the lock of the existing sandbox in `dir`, and starts its removal. +fn remove(dir: &Path) -> std::result::Result, RemovalError> { + SandboxStore::lock_existing(dir, &files(), None) + .unwrap() + .remove() +} + +/// Removes the sandbox in `dir` as [`LockedStore::remove`] does, while the lock file of another +/// transition, which NFS renamed as the transition still held it, keeps the emptied directory. +fn remove_lingering(dir: &Path) -> Lingering { + let locked = SandboxStore::lock_existing(dir, &files(), None).unwrap(); + locked.remove_contents().unwrap(); + fs::write(dir.join(RENAMED), b"").unwrap(); + locked + .remove_directory() + .unwrap() + .expect("a lingering lock file keeps the directory") +} + +/// Takes the lock of the existing sandbox in `dir` until `deadline` and releases it, and returns +/// whether the sandbox was provisioned. +fn transition(dir: &Path, deadline: Option) -> Result { + let locked = SandboxStore::lock_existing(dir, &files(), deadline)?; + Ok(locked.dir().join(CONFIG).exists()) +} + +/// A transition in a process of its own, which [`transition_role`] runs. +struct Transition { + /// The process. + child: Child, + /// Its standard output. + output: BufReader, +} + +impl Transition { + /// Starts a transition that takes the lock `lock` of the sandbox in `dir`, and returns once + /// it is about to wait for the lock. + fn start(dir: &Path, lock: &str) -> Self { + let mut child = Command::new(env::current_exe().unwrap()) + .args([ + "--exact", + "host::store::lifecycle::tests::transition_role", + "--ignored", + "--nocapture", + "--test-threads=1", + ]) + .env(ROLE_DIR, dir) + .env(ROLE_LOCK, lock) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .spawn() + .unwrap(); + let mut output = BufReader::new(child.stdout.take().unwrap()); + let mut line = String::new(); + while line.trim_end() != WAITING { + line.clear(); + let read = output.read_line(&mut line).unwrap(); + assert_ne!( + read, 0, + "the transition ended before it waited for the lock" + ); + } + Self { child, output } + } + + /// Returns whether the transition still runs. + fn runs(&mut self) -> bool { + self.child.try_wait().unwrap().is_none() + } + + /// Waits for the transition to end, and returns how it ended, as [`transition_role`] prints + /// it. + fn outcome(mut self) -> String { + let mut rest = String::new(); + self.output.read_to_string(&mut rest).unwrap(); + assert!(self.child.wait().unwrap().success(), "{rest}"); + rest.lines() + .find_map(|line| line.strip_prefix(OUTCOME)) + .expect("the transition printed its outcome") + .to_owned() + } +} + +impl Drop for Transition { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +/// A transition in a process of its own, which [`Transition::start`] runs: it takes the lock that +/// [`ROLE_LOCK`] names of the sandbox in [`ROLE_DIR`] within 30 s, and prints how it ended: +/// `provisioned` if it found the configuration record, `unprovisioned` if it did not, which a +/// provision then writes, and `gone` if the sandbox no longer exists. +#[test] +#[ignore = "a transition that the lifecycle tests run in a process of its own"] +fn transition_role() { + let (Some(dir), Ok(lock)) = (env::var_os(ROLE_DIR), env::var(ROLE_LOCK)) else { + return; + }; + // The test harness has printed the test's name without ending the line. + println!("\n{WAITING}"); + let locked = match lock.as_str() { + "provision" => SandboxStore::lock_for_provision(&dir, &files(), within(30)), + "existing" => SandboxStore::lock_existing(&dir, &files(), within(30)), + _ => panic!("unknown lock {lock}"), + }; + let outcome = match locked { + Ok(locked) if locked.dir().join(CONFIG).exists() => "provisioned", + Ok(locked) => { + if lock == "provision" { + fs::write(locked.dir().join(CONFIG), b"{}").unwrap(); + } + "unprovisioned" + } + Err(error) if error.kind() == ErrorKind::NotFound => "gone", + Err(error) => panic!("{error}"), + }; + println!("{OUTCOME}{outcome}"); +} + +#[test] +fn lifecycle_locks_exclude_other_holders_until_released() { + let (_directory, dir) = sandbox(); + let held = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let started = Instant::now(); + let deadline = Some(Instant::now() + Duration::from_millis(100)); + let error = SandboxStore::lock_existing(&dir, &files(), deadline).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); + assert!(started.elapsed() >= Duration::from_millis(100)); + let error = SandboxStore::lock_for_provision(&dir, &files(), Some(Instant::now())).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); + // A waiter takes the lock once its holder releases it. + let (sender, receiver) = mpsc::channel(); + let waiter = thread::spawn({ + let dir = dir.clone(); + move || { + let locked = SandboxStore::lock_existing(&dir, &files(), within(30)).map(drop); + sender.send(()).unwrap(); + locked + } + }); + assert!(receiver.recv_timeout(Duration::from_millis(100)).is_err()); + drop(held); + waiter.join().unwrap().unwrap(); + assert!(released(|| { + SandboxStore::lock_existing(&dir, &files(), Some(Instant::now())).is_ok() + })); + // A provisioned directory keeps its lock file. + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME])); +} + +#[test] +fn released_locks_leave_no_lock_file_in_unprovisioned_directories() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + // A provision that records nothing leaves the directory that it created empty, and so does + // a transition of a sandbox that was never provisioned. + drop(SandboxStore::lock_for_provision(&dir, &files(), None).unwrap()); + assert!(entries_of(&dir).is_empty()); + drop(SandboxStore::lock_existing(&dir, &files(), None).unwrap()); + assert!(entries_of(&dir).is_empty()); + // A directory that does not exist holds no sandbox: only a provision creates it, below a + // parent that exists. + let missing = directory.path().join("missing"); + let error = SandboxStore::lock_existing(&missing, &files(), None).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + assert!(!missing.exists()); + drop(SandboxStore::lock_for_provision(&missing, &files(), None).unwrap()); + assert!(missing.is_dir()); + let orphan = directory.path().join("absent").join("sandbox"); + let error = SandboxStore::lock_for_provision(&orphan, &files(), None).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + assert!(!directory.path().join("absent").exists()); + // A caller that keeps no configuration record keeps the lock file, as the store's own lock + // does. + drop(SandboxStore::lock_existing(&dir, &CallerFiles::none(), None).unwrap()); + assert_eq!(entries_of(&dir), [LOCK_NAME]); + drop(SandboxStore::open(&dir).unwrap().lock(None).unwrap()); + assert_eq!(entries_of(&dir), [LOCK_NAME]); +} + +#[test] +fn a_holder_removes_only_its_own_lock_file_on_release() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + // A holder that removed its lock file, as a removal does while it holds the lock, leaves the + // new lock file of a transition that took the lock meanwhile. + let mut held = SandboxStore::lock_for_provision(&dir, &files(), None).unwrap(); + fs::remove_file(dir.join(LOCK_NAME)).unwrap(); + held.lock_removed = true; + let other = SandboxStore::lock_existing(&dir, &files(), within(10)).unwrap(); + drop(held); + assert_eq!(entries_of(&dir), [LOCK_NAME]); + drop(other); + assert!(entries_of(&dir).is_empty()); + // So does a holder whose lock file another replaced. + let held = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + fs::remove_file(dir.join(LOCK_NAME)).unwrap(); + fs::write(dir.join(LOCK_NAME), b"").unwrap(); + drop(held); + assert_eq!(entries_of(&dir), [LOCK_NAME]); +} + +#[test] +fn directories_that_others_may_access_are_refused() { + let (_directory, dir) = sandbox(); + #[cfg(unix)] + let problem = { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); + "is accessible to other users (mode 755)" + }; + #[cfg(windows)] + let problem = { + platform::set_access(&dir, "D:(A;OICI;FA;;;WD)").unwrap(); + "allows other users" + }; + for error in [ + SandboxStore::lock_existing(&dir, &files(), None).unwrap_err(), + SandboxStore::lock_for_provision(&dir, &files(), None).unwrap_err(), + ] { + assert_eq!( + error.message(), + format!("the sandbox store {} {problem}", dir.display()) + ); + } +} + +#[test] +fn stale_directories_hold_no_sandbox_and_keep_no_lock_file() { + let directory = tempfile::tempdir().unwrap(); + // A directory that was never provisioned, and those that an interrupted removal left: empty, + // or with a lock file and the temporary file of an interrupted replacement. + let never = SandboxStore::create(directory.path().join("never")).unwrap(); + let emptied = SandboxStore::create(directory.path().join("emptied")).unwrap(); + let left = SandboxStore::create(directory.path().join("left")).unwrap(); + fs::write(left.dir().join(LOCK_NAME), b"").unwrap(); + fs::write(left.dir().join(".runtime.json.1.tmp"), b"{").unwrap(); + for store in [&never, &emptied, &left] { + let locked = SandboxStore::lock_existing(store.dir(), &files(), None).unwrap(); + assert!(!locked.dir().join(CONFIG).exists()); + drop(locked); + assert!(!store.dir().join(LOCK_NAME).exists()); + } + // A removal of such a directory finds nothing of a sandbox, and removes it. + assert!(remove(left.dir()).unwrap().is_none()); + assert!(!left.dir().exists()); +} + +#[test] +fn a_removal_removes_the_store_the_callers_files_and_the_directory() { + let (_directory, dir) = sandbox(); + for name in [ + LOG_NAME, + OUTCOME_NAME, + CONSOLE_NAME, + CAPABILITY_NAME, + SOCKET_NAME, + BOOT, + ".runtime.json.12345.tmp", + ".sandbox.lock.7.tmp", + ] { + fs::write(dir.join(name), b"left").unwrap(); + } + assert!(remove(&dir).unwrap().is_none()); + assert!(!dir.exists()); + // A transition that comes later finds no sandbox, and creates nothing. + let error = SandboxStore::lock_existing(&dir, &files(), None).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + assert!(!dir.exists()); +} + +#[test] +fn a_removal_refuses_foreign_entries_and_keeps_them() { + let (_directory, dir) = sandbox(); + fs::write(dir.join(LOG_NAME), b"log").unwrap(); + fs::write(dir.join(BOOT), b"").unwrap(); + // Entries that resemble the store's files and temporary files, or the caller's, are foreign, + // as are the caller's own temporary files, which it removes itself. + let foreign = [ + "notes.txt", + ".config.json.1.tmp", + ".runtime.json.1a.tmp", + "outcome.json.old", + "directory", + ]; + for name in &foreign[..4] { + fs::write(dir.join(name), b"kept").unwrap(); + } + fs::create_dir(dir.join("directory")).unwrap(); + let RemovalError::Foreign(mut names) = remove(&dir).unwrap_err() else { + panic!("foreign entries were not refused"); + }; + names.sort(); + assert_eq!(names, sorted(&foreign)); + // The store's files and the caller's went first, and the lock file as the lock was released, + // so the sandbox is gone and the foreign entries remain, in the directory. + assert_eq!(entries_of(&dir), sorted(&foreign)); + assert_eq!(fs::read(dir.join("notes.txt")).unwrap(), b"kept"); + let error = Error::from(RemovalError::Foreign(vec!["a".to_owned(), "b".to_owned()])); + assert_eq!(error.kind(), ErrorKind::Other); + assert_eq!( + error.message(), + "the sandbox's directory holds entries that are neither the store's nor its caller's: a, b" + ); + // Once they are gone, the directory goes too. + for name in &foreign[..4] { + fs::remove_file(dir.join(name)).unwrap(); + } + fs::remove_dir(dir.join("directory")).unwrap(); + assert!(remove(&dir).unwrap().is_none()); + assert!(!dir.exists()); +} + +#[test] +fn a_removal_is_refused_while_a_vm_or_a_launch_is_recorded() { + for name in [JOURNAL_NAME, RUNTIME_NAME] { + let (_directory, dir) = sandbox(); + fs::write(dir.join(name), b"{}").unwrap(); + fs::write(dir.join(LOG_NAME), b"log").unwrap(); + assert!( + matches!(remove(&dir), Err(RemovalError::Launched)), + "{name}" + ); + // Nothing was removed, and the provisioned sandbox keeps its lock file. + assert_eq!( + entries_of(&dir), + sorted(&[CONFIG, LOCK_NAME, LOG_NAME, name]) + ); + } +} + +#[test] +fn lingering_lock_files_keep_the_directory_until_they_go() { + let (_directory, dir) = sandbox(); + let mut lingering = remove_lingering(&dir); + assert_eq!(lingering.dir(), dir); + assert_eq!(entries_of(&dir), [RENAMED]); + // The removal released its lock, and the sandbox is gone. A transition takes the lock of a + // new lock file, which keeps the directory too, and finds no configuration record. + let waiting = SandboxStore::lock_existing(&dir, &files(), within(10)).unwrap(); + assert!(!dir.join(CONFIG).exists()); + let Err(RemovalError::TimedOut(error)) = lingering.wait(Some(Instant::now())) else { + panic!("the directory went while lock files kept it"); + }; + assert!(directory_not_empty(&error), "{error}"); + fs::remove_file(dir.join(RENAMED)).unwrap(); + assert!(matches!( + lingering.wait(Some(Instant::now())), + Err(RemovalError::TimedOut(_)) + )); + // The transition removes its lock file as it releases its lock, which empties the directory. + let removed = thread::spawn(move || lingering.wait(within(30))); + thread::sleep(Duration::from_millis(50)); + drop(waiting); + removed.join().unwrap().unwrap(); + assert!(!dir.exists()); +} + +#[test] +fn a_lingering_removal_reports_what_appears_in_the_directory() { + // A provision that takes the lock that the removal released provisions the directory again. + let (_directory, dir) = sandbox(); + let mut lingering = remove_lingering(&dir); + let provision = SandboxStore::lock_for_provision(&dir, &files(), within(10)).unwrap(); + fs::write(dir.join(CONFIG), b"{}").unwrap(); + drop(provision); + fs::remove_file(dir.join(RENAMED)).unwrap(); + assert!(matches!( + lingering.wait(within(10)), + Err(RemovalError::ProvisionedAgain) + )); + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME])); + // Other entries are foreign, sorted. + let mut lingering = remove_lingering(&dir); + for name in ["zebra.txt", "apple.txt"] { + fs::write(dir.join(name), b"").unwrap(); + } + let Err(RemovalError::Foreign(names)) = lingering.wait(within(10)) else { + panic!("foreign entries were not refused"); + }; + assert_eq!(names, ["apple.txt", "zebra.txt"]); + // The path may name another directory once the removal's is gone, which is not its to + // remove. + for name in [RENAMED, "zebra.txt", "apple.txt"] { + fs::remove_file(dir.join(name)).unwrap(); + } + fs::remove_dir(&dir).unwrap(); + fs::create_dir(&dir).unwrap(); + fs::write(dir.join(RENAMED), b"").unwrap(); + lingering.wait(Some(Instant::now())).unwrap(); + assert_eq!(entries_of(&dir), [RENAMED]); +} + +#[test] +fn a_provision_that_waits_through_a_removal_provisions_the_sandbox_again() { + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let (ready, waiting) = mpsc::channel(); + let provision = thread::spawn({ + let dir = dir.clone(); + move || { + ready.send(()).unwrap(); + let locked = SandboxStore::lock_for_provision(&dir, &files(), within(30)).unwrap(); + let provisioned = dir.join(CONFIG).exists(); + fs::write(dir.join(CONFIG), b"{}").unwrap(); + drop(locked); + provisioned + } + }); + waiting.recv().unwrap(); + thread::sleep(Duration::from_millis(100)); + // The removal completes before the provision takes the lock, which creates the directory + // again, or, should the provision open the lock file between the removals of the lock file + // and of the directory, reports the provision. + match locked.remove().unwrap() { + None => {} + Some(mut lingering) => { + assert!(matches!( + lingering.wait(within(30)), + Err(RemovalError::ProvisionedAgain) + )); + } + } + assert!(!provision.join().unwrap()); + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME])); +} + +#[test] +fn transitions_that_wait_through_a_removal_find_the_sandbox_gone() { + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let (ready, waiting) = mpsc::channel(); + let waiters: Vec<_> = [None, within(30)] + .into_iter() + .map(|deadline| { + let (dir, ready) = (dir.clone(), ready.clone()); + thread::spawn(move || { + ready.send(()).unwrap(); + transition(&dir, deadline) + }) + }) + .collect(); + for _ in &waiters { + waiting.recv().unwrap(); + } + thread::sleep(Duration::from_millis(100)); + let lingering = locked.remove().unwrap(); + let outcomes: Vec<_> = waiters + .into_iter() + .map(|waiter| waiter.join().unwrap()) + .collect(); + match lingering { + // The waiters wait for the lock of the removed lock file, which the removal releases + // once the directory is gone. + None => { + for outcome in outcomes { + let error = outcome.unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + } + } + // A waiter that started late opened the lock file between the removals of the lock file + // and of the directory, which created a new one, and found no sandbox. + Some(mut lingering) => { + for outcome in outcomes { + match outcome { + Ok(provisioned) => assert!(!provisioned), + Err(error) => assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"), + } + } + lingering.wait(within(30)).unwrap(); + } + } + assert!(!dir.exists()); +} + +#[test] +fn a_transition_that_waits_for_a_holder_that_provisions_nothing_finds_no_sandbox() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + let held = SandboxStore::lock_for_provision(&dir, &files(), None).unwrap(); + let waiter = thread::spawn({ + let dir = dir.clone(); + move || transition(&dir, within(30)) + }); + thread::sleep(Duration::from_millis(100)); + // The provision fails, and its release removes the lock file that the waiter waits for: the + // waiter takes the lock of a new one in the directory, which holds no sandbox. + drop(held); + assert!(!waiter.join().unwrap().unwrap()); + assert!(entries_of(&dir).is_empty()); +} + +#[test] +fn transitions_in_other_processes_that_wait_through_a_removal_find_the_sandbox_gone() { + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let mut transitions: Vec<_> = (0..2) + .map(|_| Transition::start(&dir, "existing")) + .collect(); + thread::sleep(Duration::from_millis(100)); + assert!(transitions.iter_mut().all(Transition::runs)); + let lingering = locked.remove().unwrap(); + let outcomes: Vec<_> = transitions.into_iter().map(Transition::outcome).collect(); + match lingering { + None => assert_eq!(outcomes, ["gone", "gone"]), + // A transition that started late opened the lock file between the removals of the lock + // file and of the directory, which created a new one, and found no sandbox. + Some(mut lingering) => { + for outcome in &outcomes { + assert!( + ["gone", "unprovisioned"].contains(&outcome.as_str()), + "{outcome}" + ); + } + lingering.wait(within(30)).unwrap(); + } + } + assert!(!dir.exists()); +} + +#[test] +fn provisions_in_other_processes_that_race_a_removal_provision_the_sandbox_again() { + // A provision that waits for the removal's lock creates the directory again once the removal + // completes. + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let mut provision = Transition::start(&dir, "provision"); + thread::sleep(Duration::from_millis(100)); + assert!(provision.runs()); + if let Some(mut lingering) = locked.remove().unwrap() { + assert!(matches!( + lingering.wait(within(30)), + Err(RemovalError::ProvisionedAgain) + )); + } + assert_eq!(provision.outcome(), "unprovisioned"); + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME])); + // One that takes the lock that the removal released while lingering lock files keep the + // directory provisions it again, which the removal reports. + let mut lingering = remove_lingering(&dir); + assert_eq!( + Transition::start(&dir, "provision").outcome(), + "unprovisioned" + ); + fs::remove_file(dir.join(RENAMED)).unwrap(); + assert!(matches!( + lingering.wait(within(10)), + Err(RemovalError::ProvisionedAgain) + )); + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME])); +} + +#[test] +fn refused_removals_keep_the_directory_for_transitions_in_other_processes() { + // A removal that is refused while the store records a VM removes nothing, and the waiting + // transition finds the sandbox. + let (_directory, dir) = sandbox(); + fs::write(dir.join(RUNTIME_NAME), b"{}").unwrap(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let mut transition = Transition::start(&dir, "existing"); + thread::sleep(Duration::from_millis(100)); + assert!(transition.runs()); + assert!(matches!(locked.remove(), Err(RemovalError::Launched))); + assert_eq!(transition.outcome(), "provisioned"); + // One that is refused for foreign entries removed the sandbox's files: the waiting transition + // finds no sandbox, and the directory keeps the foreign entries alone. + fs::remove_file(dir.join(RUNTIME_NAME)).unwrap(); + fs::write(dir.join("notes.txt"), b"kept").unwrap(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let mut transition = Transition::start(&dir, "existing"); + thread::sleep(Duration::from_millis(100)); + assert!(transition.runs()); + let Err(RemovalError::Foreign(names)) = locked.remove() else { + panic!("foreign entries were not refused"); + }; + assert_eq!(names, ["notes.txt"]); + assert_eq!(transition.outcome(), "unprovisioned"); + assert_eq!(entries_of(&dir), ["notes.txt"]); +} + +/// Tests of lock files that a Windows file system without POSIX deletion keeps under their name +/// until their last handle closes, which NTFS does for a file deleted without POSIX semantics. +#[cfg(windows)] +#[allow(unsafe_code)] +mod windows { + use std::os::windows::fs::OpenOptionsExt; + use std::os::windows::io::AsRawHandle; + + use windows_sys::Win32::Foundation::HANDLE; + use windows_sys::Win32::Storage::FileSystem::{ + DELETE, FILE_DISPOSITION_INFO, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, + FileDispositionInfo, SetFileInformationByHandle, + }; + + use super::*; + + /// Deletes the file `path` as a file system without POSIX deletion deletes a file that other + /// handles hold: its name stays, and opening it fails, until the last handle closes, of which + /// the returned one is the last unless others hold the file. + fn delete_pending(path: &Path) -> File { + let file = fs::OpenOptions::new() + .access_mode(DELETE) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) + .open(path) + .unwrap(); + let disposition = FILE_DISPOSITION_INFO { DeleteFile: true }; + // SAFETY: the handle is open, and the information is a FILE_DISPOSITION_INFO of its size. + let set = unsafe { + SetFileInformationByHandle( + file.as_raw_handle() as HANDLE, + FileDispositionInfo, + (&raw const disposition).cast(), + size_of::() as u32, + ) + }; + assert_ne!(set, 0, "{}", io::Error::last_os_error()); + file + } + + #[test] + fn locks_wait_for_a_removed_lock_file_that_a_handle_keeps() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + SandboxStore::create(&dir).unwrap(); + fs::write(dir.join(LOCK_NAME), b"").unwrap(); + let pending = delete_pending(&dir.join(LOCK_NAME)); + for error in [ + SandboxStore::lock_for_provision(&dir, &files(), Some(Instant::now())).unwrap_err(), + SandboxStore::lock_existing(&dir, &files(), Some(Instant::now())).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); + } + let provision = thread::spawn({ + let dir = dir.clone(); + move || SandboxStore::lock_for_provision(&dir, &files(), within(30)).map(drop) + }); + thread::sleep(Duration::from_millis(100)); + drop(pending); + provision.join().unwrap().unwrap(); + assert!(entries_of(&dir).is_empty()); + } + + #[test] + fn a_transition_whose_lock_file_lingers_waits_until_it_goes() { + let (_directory, dir) = sandbox(); + let held = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + let (ready, waiting) = mpsc::channel(); + let waiter = thread::spawn({ + let dir = dir.clone(); + move || { + ready.send(()).unwrap(); + transition(&dir, within(30)) + } + }); + waiting.recv().unwrap(); + thread::sleep(Duration::from_millis(100)); + // The holder removes the sandbox, on a file system that keeps the lock file while a + // handle holds it. The waiter takes its lock, finds it removed, and cannot open the lock + // file at the path again until it goes. + fs::remove_file(dir.join(CONFIG)).unwrap(); + let pending = delete_pending(&dir.join(LOCK_NAME)); + drop(held); + thread::sleep(Duration::from_millis(100)); + assert!(!waiter.is_finished()); + drop(pending); + assert!(!waiter.join().unwrap().unwrap()); + assert!(entries_of(&dir).is_empty()); + } + + #[test] + fn transitions_in_other_processes_wait_for_a_lingering_lock_file() { + let (_directory, dir) = sandbox(); + fs::remove_file(dir.join(CONFIG)).unwrap(); + for (lock, left) in [ + ("existing", &[][..]), + ("provision", &[CONFIG, LOCK_NAME][..]), + ] { + fs::write(dir.join(LOCK_NAME), b"").unwrap(); + let pending = delete_pending(&dir.join(LOCK_NAME)); + let mut transition = Transition::start(&dir, lock); + thread::sleep(Duration::from_millis(100)); + assert!(transition.runs(), "{lock}"); + drop(pending); + assert_eq!(transition.outcome(), "unprovisioned", "{lock}"); + assert_eq!(entries_of(&dir), sorted(left), "{lock}"); + } + } +} diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index f7d051762..ccf5296a6 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -12,6 +12,7 @@ //! openvmm.log the VMM's output, claimed by the launch that created it //! console.log the guest console of the latest launch, if the caller captures it //! outcome.json the VMM's outcome report of the latest launch +//! ... the caller's configuration record and other files, which it declares //! ``` //! //! The directory is private to the current user, since its records decide which process @@ -32,7 +33,8 @@ //! names, which a launch may replace while the lock is held. //! Records are replaced atomically, so readers that hold no lock, such as an execution, see whole //! records. The store holds runtime records only: configuration belongs to its caller, which may -//! keep it in the same directory under a name of its own. +//! keep it in the same directory under a name of its own, and declares it, with its other files +//! there, as [`CallerFiles`]. //! //! # Records //! @@ -147,9 +149,49 @@ //! The runtime record of a committed launch names its running VM, and //! [`supervisor::reattach`](super::supervisor::reattach) holds that VM again after its //! orchestrator restarts. - +//! +//! # Lifecycle locks +//! +//! A caller that keeps a configuration record in the sandbox's directory, whose presence means +//! that the sandbox is provisioned, declares it, with any other files of its own there, as +//! [`CallerFiles`], and takes the lock of each lifecycle transition with them. Provisions, other +//! transitions, and removals of the sandbox then keep its directory consistent, from any process: +//! +//! - [`SandboxStore::lock_for_provision`] creates the directory if needed, and again if a +//! concurrent removal deletes it before the lock is taken. +//! - [`SandboxStore::lock_existing`] fails with [`ErrorKind::NotFound`] once the directory is gone, +//! also when a concurrent removal deletes it while the lock is awaited. The caller checks its +//! configuration record under the lock: a directory that was never provisioned, that an +//! interrupted removal emptied, or whose removal lingering lock files still keep, holds none. +//! - A holder that leaves the directory without the configuration record removes its own lock +//! file as it releases the lock, unless it removed it already, so that such a directory keeps +//! no lock file. +//! +//! Both locks wait for another holder until the caller's deadline, trying again every 25 ms, and +//! a deadline that has passed still tries once, so that a caller that waits on its own, as for an +//! interruption, can retry them itself. +//! +//! # Removal +//! +//! [`LockedStore::remove`] removes a sandbox that no VMM can run in: it refuses while the store +//! records a VM or a launch, which the caller reattaches to, or recovers, first. It removes the +//! store's files and the caller's, the configuration record last, and the temporary files that an +//! interrupted replacement of one of the store's files leaves, `.NAME.NUMBER.tmp`. It then +//! refuses entries that are neither the store's nor the caller's, naming them, and keeps the +//! directory, or removes its lock file while it still holds the lock, and then the directory. +//! +//! Lock files that other transitions hold can keep the emptied directory for a while: NFS renames +//! a removed file that is still open to `.nfs*`, Windows file systems without POSIX deletion keep +//! it under its name until its last handle closes, and a transition that opens the lock file +//! after the removal removed it creates a new one. Each such transition finds no sandbox, and +//! removes its lock file as it releases its lock. The removal then holds the directory's identity, +//! releases its lock, and returns a [`Lingering`] removal, which waits until they go, and reports +//! a provision that took the lock meanwhile as [`RemovalError::ProvisionedAgain`]. + +mod caller; mod files; mod identity; +mod lifecycle; mod record; #[cfg(test)] mod tests; @@ -162,10 +204,12 @@ use std::path::{Path, PathBuf}; use std::thread; use std::time::{Duration, Instant}; +pub use self::caller::CallerFiles; use self::files::{io_error, remove_if_present}; pub use self::identity::{ ArtifactIdentity, IdentityCheck, IdentityPart, MAX_IDENTITY_LEN, RuntimeIdentity, }; +pub use self::lifecycle::{Lingering, RemovalError}; pub use self::record::{ LaunchId, LaunchJournal, LaunchState, LogClaim, MAX_ENDPOINT_LEN, RuntimeRecord, }; @@ -173,7 +217,7 @@ use super::filesystem::FileIdentity; use super::platform; use super::process::{BootId, Process, ProcessRef}; use super::supervisor::Launched; -use crate::error::{Error, Result}; +use crate::error::{Error, ErrorKind, Result}; use crate::protocol::CAPABILITY_LEN; /// Version of the store's records. @@ -238,6 +282,28 @@ fn absolute(dir: PathBuf) -> Result { ))) } +/// Builds an error mapper that keeps an I/O source under `context`, as an error of +/// [`ErrorKind::NotFound`] if the source says that a path does not exist, and of +/// [`ErrorKind::Io`] otherwise. +fn store_error(context: String) -> impl FnOnce(io::Error) -> Error { + move |error| { + let kind = match error.kind() { + io::ErrorKind::NotFound => ErrorKind::NotFound, + _ => ErrorKind::Io, + }; + Error::new(kind, context).with_source(error) + } +} + +/// How an attempt to take a store's lock ended, if it did not fail. +enum Taken { + /// The lock of the lock file that the path names. + Locked(File), + /// The lock of a lock file that was removed or replaced after it was opened, which locks + /// nothing. + Replaced, +} + /// The store of one sandbox: its private directory. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SandboxStore { @@ -255,9 +321,10 @@ impl SandboxStore { /// /// A relative `dir` is resolved against the current directory, so that the store's paths, and /// the control endpoint that a launch records, name the same files from any working directory. + /// Fails with [`ErrorKind::NotFound`] if the parent does not exist. pub fn create(dir: impl Into) -> Result { let dir = absolute(dir.into())?; - let problem = platform::create_store_dir(&dir).map_err(io_error(format!( + let problem = platform::create_store_dir(&dir).map_err(store_error(format!( "cannot create the sandbox store {}", dir.display() )))?; @@ -266,10 +333,11 @@ impl SandboxStore { /// Opens the store in the existing directory `dir`, which a relative path names from the /// current directory, and which must be private to the current user, as - /// [`create`](Self::create) makes it. + /// [`create`](Self::create) makes it. Fails with [`ErrorKind::NotFound`] if `dir` does not + /// exist. pub fn open(dir: impl Into) -> Result { let dir = absolute(dir.into())?; - let problem = platform::check_store_dir(&dir).map_err(io_error(format!( + let problem = platform::check_store_dir(&dir).map_err(store_error(format!( "cannot open the sandbox store {}", dir.display() )))?; @@ -357,56 +425,76 @@ impl SandboxStore { /// Takes the sandbox's lock, waiting for it until `deadline`, or for as long as it takes /// without one. /// - /// Fails with [`ErrorKind::TimedOut`](crate::ErrorKind::TimedOut) when the deadline passes - /// first. A lock file that is removed or replaced while this waits locks nothing, so the - /// lock is then taken again. A lock file that is not private to the current user is refused, - /// as a record is, since others could hold its lock, and one that is a link, or not a regular - /// file, is refused at once. + /// Fails with [`ErrorKind::TimedOut`] when the deadline passes first, and with + /// [`ErrorKind::NotFound`] if the directory does not exist. A lock file that is removed or + /// replaced while this waits locks nothing, so the lock is then taken again. A lock file that + /// is not private to the current user is refused, as a record is, since others could hold its + /// lock, and one that is a link, or not a regular file, is refused at once. + /// + /// This lock knows nothing of its caller's files. A caller that keeps a configuration record + /// in the directory, or removes the store, takes the [lifecycle locks](self#lifecycle-locks) + /// instead. pub fn lock(&self, deadline: Option) -> Result { - let path = self.path(LOCK_NAME); - let failed = |error| io_error(format!("cannot lock {}", path.display()))(error); loop { - let file = files::open_lock(&path).map_err(failed)?; - if let Some(problem) = platform::private_file_problem(&file).map_err(failed)? { - return Err(Error::io(format!( - "the sandbox lock {} {problem}", - path.display() - ))); + if let Taken::Locked(file) = self.take_lock(deadline)? { + return Ok(LockedStore::new(self.clone(), file, CallerFiles::none())); } - match deadline { - None => file.lock().map_err(failed)?, - Some(deadline) => loop { - match file.try_lock() { - Ok(()) => break, - Err(TryLockError::WouldBlock) => { - let left = deadline.saturating_duration_since(Instant::now()); - if left.is_zero() { - return Err(Error::timed_out(format!( - "another lifecycle transition of the sandbox in {} is still \ - in progress", - self.dir.display() - ))); - } - thread::sleep(left.min(LOCK_RETRY_INTERVAL)); + } + } + + /// Opens the lock file, which it creates if needed, takes its lock, waiting for it until + /// `deadline`, or for as long as it takes without one, and checks that the path still names + /// the locked file. + /// + /// Fails as [`lock`](Self::lock) does. On Windows, a file system without POSIX deletion keeps + /// a removed file under its name until its last handle closes, and refuses to open it until + /// then: the open then fails with an I/O error whose cause is the denied access, while a + /// removed lock file that the lock's own handle keeps is taken for a replaced one. + fn take_lock(&self, deadline: Option) -> Result { + let path = self.path(LOCK_NAME); + let failed = |error| store_error(format!("cannot lock {}", path.display()))(error); + let file = files::open_lock(&path).map_err(failed)?; + if let Some(problem) = platform::private_file_problem(&file).map_err(failed)? { + return Err(Error::io(format!( + "the sandbox lock {} {problem}", + path.display() + ))); + } + match deadline { + None => file.lock().map_err(failed)?, + Some(deadline) => loop { + match file.try_lock() { + Ok(()) => break, + Err(TryLockError::WouldBlock) => { + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return Err(self.transition_in_progress()); } - Err(TryLockError::Error(error)) => return Err(failed(error)), + thread::sleep(left.min(LOCK_RETRY_INTERVAL)); } - }, - } - let locked = platform::opened_file_identity(&file).map_err(failed)?; - match platform::file_identity(&path) { - Ok(current) if current == locked => { - return Ok(LockedStore { - store: self.clone(), - _lock: file, - }); + Err(TryLockError::Error(error)) => return Err(failed(error)), } - Ok(_) => {} - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) => return Err(failed(error)), + }, + } + let locked = platform::opened_file_identity(&file).map_err(failed)?; + match platform::file_identity(&path) { + Ok(current) if current == locked => Ok(Taken::Locked(file)), + Ok(_) => Ok(Taken::Replaced), + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(Taken::Replaced), + Err(error) if cfg!(windows) && error.kind() == io::ErrorKind::PermissionDenied => { + Ok(Taken::Replaced) } + Err(error) => Err(failed(error)), } } + + /// Returns the error of a wait for the lock that another transition holds past its deadline. + fn transition_in_progress(&self) -> Error { + Error::timed_out(format!( + "another lifecycle transition of the sandbox in {} is still in progress", + self.dir.display() + )) + } } /// The store of a sandbox whose lock this holds, through which its lifecycle transitions change @@ -414,13 +502,19 @@ impl SandboxStore { /// /// Every transition borrows it exclusively, and a [`Launch`] keeps it borrowed until the launch /// commits or is abandoned, so transitions never overlap, not even through one guard that -/// threads share. +/// threads share. A lock that a [lifecycle lock](self#lifecycle-locks) took also knows its +/// caller's files: as it is released, it removes its lock file if the caller's configuration +/// record is missing, and it can [remove](Self::remove) the store. #[derive(Debug)] pub struct LockedStore { /// The sandbox's store. store: SandboxStore, /// The locked file, whose lock lasts until it closes. - _lock: File, + lock: File, + /// The caller's files in the directory, which a store's own lock does not know. + files: CallerFiles, + /// Whether this holder removed its lock file, after which the path may name another's. + lock_removed: bool, } impl Deref for LockedStore { @@ -432,6 +526,23 @@ impl Deref for LockedStore { } impl LockedStore { + /// Returns the held lock of `store`, on its locked lock file `lock`, whose caller keeps + /// `files`. + fn new(store: SandboxStore, lock: File, files: CallerFiles) -> Self { + Self { + store, + lock, + files, + lock_removed: false, + } + } + + /// Returns the caller's files that the lock knows: those that a [lifecycle + /// lock](self#lifecycle-locks) was taken with, and none for [`SandboxStore::lock`]. + pub fn files(&self) -> &CallerFiles { + &self.files + } + /// Begins a launch of the runtime `runtime`, whose VMM receives `capability`. /// /// It stores the capability, replaces the log of the previous launch with a fresh one, which @@ -440,7 +551,7 @@ impl LockedStore { /// yet: the caller spawns the VMM with [`Launch::take_log`]. /// /// Fails if the sandbox's VM runs, or a launch was interrupted and is not recovered yet, and - /// with [`ErrorKind::Unsupported`](crate::ErrorKind::Unsupported) for an all-zero capability + /// with [`ErrorKind::Unsupported`] for an all-zero capability /// or an invalid runtime identity. pub fn begin_launch( &mut self, diff --git a/nvx-rs/src/host/store/tests.rs b/nvx-rs/src/host/store/tests.rs index 4ce0fb309..9f53586f4 100644 --- a/nvx-rs/src/host/store/tests.rs +++ b/nvx-rs/src/host/store/tests.rs @@ -324,7 +324,7 @@ fn a_lock_file_replaced_while_waiting_locks_nothing() { let locked = store.lock(None).unwrap(); // The waiter holds the lock of the file that the path names now. let current = platform::file_identity(&locked.path(LOCK_NAME)).unwrap(); - let held = platform::opened_file_identity(&locked._lock).unwrap(); + let held = platform::opened_file_identity(&locked.lock).unwrap(); assert_eq!(current, held); } }); @@ -634,7 +634,7 @@ fn stores_open_only_existing_directories() { let (directory, store) = store(); assert_eq!(SandboxStore::open(store.dir()).unwrap(), store); let missing = SandboxStore::open(directory.path().join("missing")).unwrap_err(); - assert_eq!(missing.kind(), ErrorKind::Io); + assert_eq!(missing.kind(), ErrorKind::NotFound); let file = directory.path().join("file"); fs::write(&file, b"").unwrap(); assert!( From 0a9d3674f818a8310408ead3d912d8116ad95639 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 00:27:41 -0700 Subject: [PATCH 02/14] nvx-rs: take the CLI's lifecycle lock and deprovision from the store The CLI's lifecycle lock is now a thin wrapper over the store's lifecycle locks, with config.json as the configuration record, and deprovision removes the sandbox through LockedStore::remove. The CLI's messages, and its goldens, are unchanged. Edge cases change: - The release removes the lock file only while it is still the holder's own, and only when config.json is definitely missing. - On Windows file systems without POSIX deletion, a transition other than provision that meets a removed lock file waits for it within its timeout rather than failing with access denied. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- doc/run.md | 6 +- nvx-rs/README.md | 9 +- nvx-rs/src/host/cli/sandbox/lifecycle.rs | 157 +------------- nvx-rs/src/host/cli/sandbox/lock.rs | 263 +++++++++++++---------- 4 files changed, 173 insertions(+), 262 deletions(-) diff --git a/doc/run.md b/doc/run.md index d4572ba68..2c21c9d25 100644 --- a/doc/run.md +++ b/doc/run.md @@ -1059,7 +1059,11 @@ for any other in progress, and of two overlapping starts, exactly one launches OpenVMM while the other then fails because the sandbox is already running. `exec` holds the lock only while it finds the running OpenVMM, so a long workload does not delay `stop`. A provisioned state directory keeps -`sandbox.lock`, and `deprovision` removes it with NVX's other files. On NFS, +`sandbox.lock`, and `deprovision` removes it with NVX's other files. A +transition that leaves a state directory without `config.json`, such as one of +a sandbox that was never provisioned, removes `sandbox.lock` as it releases the +lock. These are the rules of the sandbox store, which other orchestrators of +sandboxes share through `nvx-rs`. On NFS, and on Windows file systems without POSIX deletion such as ReFS, a removed lock file keeps the emptied directory until its last handle closes, so `deprovision` then releases the lock before it removes the directory. A transition that diff --git a/nvx-rs/README.md b/nvx-rs/README.md index fb656da71..a67ce3f05 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -414,7 +414,14 @@ refuses to start a sandbox that `nvx.py` provisioned, which has no binding: `provision`, `start`, `stop`, and `deprovision` hold the store's lock for their whole transition, and `exec` only while it finds the running VM, so that a long -workload cannot delay a `stop`. Every transition but `provision` finds the +workload cannot delay a `stop`. The CLI takes the store's +[lifecycle locks](src/host/store/mod.rs) with `config.json` as its +configuration record: a transition that leaves the state directory without it, +such as one of a sandbox that was never provisioned, removes the lock file as +it releases the lock, and `deprovision` removes the sandbox through the store, +which removes the store's files and `config.json`, refuses files that are not +NVX's, and removes the lock file, while it still holds the lock, before the +directory. Every transition but `provision` finds the sandbox's VM with `nvx::host::supervisor::reattach`, as an orchestrator does after it restarts within the same boot of the host: it ends a start that was interrupted, holds an OpenVMM that still runs through its verified process diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle.rs b/nvx-rs/src/host/cli/sandbox/lifecycle.rs index 1c80c9e0a..0becdac07 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle.rs @@ -39,14 +39,11 @@ //! that does not, without ending it otherwise. use std::ffi::OsString; -use std::fs::{File, OpenOptions}; +use std::fs::OpenOptions; use std::io::Write; use std::time::{Duration, Instant}; -use super::lock::{ - LifecycleLock, OsErrorClass, directory_not_empty, directory_status, entries, lingers, - os_error_class, -}; +use super::lock::{LifecycleLock, OsErrorClass, os_error_class}; use super::state::{ CONFIG_FORMATS, RecordedDistro, config_int, config_text, deserialize_binding, deserialize_launch, hex, member, random_bytes, read_json, serialize_launch, temporary_path, @@ -66,7 +63,7 @@ use crate::host::python::value::py_str; use crate::host::sandbox::distro::DISTRO_POLICY_REVISION; use crate::host::sandbox::{SandboxLaunch, require_file}; use crate::host::store::{ - self, IdentityPart, LOCK_NAME, LockedStore, OUTCOME_NAME, RUNTIME_NAME, Recovery, SandboxStore, + IdentityPart, LockedStore, OUTCOME_NAME, RUNTIME_NAME, Recovery, SandboxStore, }; use crate::host::supervisor::{self, Launched, Reattached, StopOutcome, Vmm, VmmCommand}; use crate::protocol::{CAPABILITY_LEN, GuestFeatures}; @@ -806,19 +803,6 @@ fn wait_for_exit(vmm: &Vmm, deadline: Deadline) -> Result { } } -/// Returns whether `name` is a temporary file that the store leaves beside one of its files when a -/// replacement of the file is interrupted: `.NAME.NUMBER.tmp`. -fn is_store_temporary(name: &str) -> bool { - name.strip_prefix('.') - .and_then(|name| name.strip_suffix(".tmp")) - .and_then(|name| name.rsplit_once('.')) - .is_some_and(|(file, number)| { - store::FILE_NAMES.contains(&file) - && !number.is_empty() - && number.bytes().all(|byte| byte.is_ascii_digit()) - }) -} - /// Removes a stopped sandbox and its state directory, as `deprovision` does. pub(crate) fn deprovision(state_path: &str, artifacts: &Artifacts<'_>, timeout: f64) -> Result { let state_dir = prepare_state_directory(state_path, false)?; @@ -829,134 +813,9 @@ pub(crate) fn deprovision(state_path: &str, artifacts: &Artifacts<'_>, timeout: return Err(Exit::error("sandbox must be stopped before deprovision")); } } - // No OpenVMM of the sandbox runs, and a start that was interrupted is over, so every file of - // the store goes but its lock, which goes last. - for name in store::FILE_NAMES - .into_iter() - .filter(|&name| name != LOCK_NAME) - .chain([CONFIG_NAME]) - { - unlink_missing_ok(&state_dir.join(name))?; - } - let mut unknown: Vec = entries(&state_dir) - .map_err(|error| os_error(&error, &state_dir))? - .into_iter() - .filter(|name| name != LOCK_NAME) - .collect(); - for name in unknown.extract_if(.., |name| is_store_temporary(name)) { - unlink_missing_ok(&state_dir.join(&name))?; - } - if !unknown.is_empty() { - // `iterdir` lists the names in the order of the directory, which the message keeps. - return Err(Exit::error(format!( - "sandbox state directory contains files not owned by NVX: {}", - unknown.join(", ") - ))); - } - // Holding the lock until the directory is gone keeps a transition that waits for it from - // finding the sandbox halfway removed. - lock.remove()?; - let lingering = match std::fs::remove_dir(state_dir.as_path()) { - Ok(()) => false, - Err(error) if directory_not_empty(&error) => true, - Err(error) => return Err(os_error(&error, &state_dir)), - }; - if !lingering { - return Ok(()); - } - // A removed lock file that a handle still keeps stays in the directory, as do the lock files of - // transitions that wait meanwhile. Each of them finds no sandbox, and removes its lock file as - // it releases the lock, so the directory empties after this call releases its own. - let (removing, held) = hold_directory(&state_dir)?; - lock.release(); - let removed = remove_emptied_state_directory(&state_dir, removing, timeout); - drop(held); - removed -} - -/// Returns the identity of a directory, and a handle that keeps it, as `_hold_directory` does: on -/// POSIX, the open directory keeps a directory that replaces it from reusing its inode number; -/// NTFS and ReFS do not reuse file IDs. -fn hold_directory(path: &PurePath) -> Result<((u64, u128), Option)> { - #[cfg(unix)] - { - use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; - - let file = OpenOptions::new() - .read(true) - .custom_flags((nix::fcntl::OFlag::O_DIRECTORY | nix::fcntl::OFlag::O_NOFOLLOW).bits()) - .open(path.as_path()) - .map_err(|error| os_error(&error, path))?; - // `os.fstat` gives the identity that `os.lstat` gives for the directory's path. - let metadata = file.metadata().map_err(Exit::from_os_error)?; - Ok(((metadata.dev(), u128::from(metadata.ino())), Some(file))) - } - #[cfg(not(unix))] - { - let status = fs::lstat(path.as_path()).map_err(|error| os_error(&error, path))?; - Ok((status.identity(), None)) - } -} - -/// Removes a state directory once no lingering lock file keeps it, as -/// `_remove_emptied_state_directory` does. A provision that waits for the removal may complete it -/// and provision the path again, which ends this attempt too. -fn remove_emptied_state_directory( - state_dir: &PurePath, - removing: (u64, u128), - timeout: f64, -) -> Result { - let deadline = Deadline::after(timeout); - let mut retried_at_once = false; - loop { - let error = match std::fs::remove_dir(state_dir.as_path()) { - Ok(()) => return Ok(()), - Err(error) if os_error_class(&error) == OsErrorClass::NotFound => return Ok(()), - Err(error) => error, - }; - if directory_status(state_dir) != Some(removing) { - return Ok(()); - } - let remaining = match entries(state_dir) { - Ok(remaining) => remaining, - Err(error) if os_error_class(&error) == OsErrorClass::NotFound => return Ok(()), - Err(error) => return Err(os_error(&error, state_dir)), - }; - let mut unexpected: Vec = remaining - .iter() - .filter(|name| !lingers(name)) - .cloned() - .collect(); - unexpected.sort(); - // A provision that starts after this deprovision removed the lock file, and before it - // removed the directory, provisions it again. - if unexpected.iter().any(|name| name == CONFIG_NAME) { - return Err(Exit::error( - "sandbox was provisioned again while deprovision removed it", - )); - } - if !unexpected.is_empty() { - return Err(Exit::error(format!( - "sandbox state directory contains files not owned by NVX: {}", - unexpected.join(", ") - ))); - } - if !directory_not_empty(&error) { - return Err(os_error(&error, state_dir)); - } - // Without remaining entries, the last lingering lock file went away after rmdir failed, - // so the next attempt follows at once. Another such failure right after it pauses first, - // so that a file system that does not report emptiness right away cannot spin. - if remaining.is_empty() && !retried_at_once { - retried_at_once = true; - if deadline.passed() { - return Err(os_error(&error, state_dir)); - } - continue; - } - retried_at_once = false; - if !deadline.pause(LOCK_RETRY_INTERVAL)? { - return Err(os_error(&error, state_dir)); - } - } + // No OpenVMM of the sandbox runs, and a start that was interrupted is over, so the store + // removes the sandbox: its files and the configuration, then refuses files that are not + // NVX's, and otherwise removes its lock file, while it still holds the lock, and the + // directory. + lock.remove(timeout) } diff --git a/nvx-rs/src/host/cli/sandbox/lock.rs b/nvx-rs/src/host/cli/sandbox/lock.rs index fb2dab88d..c60c3d96c 100644 --- a/nvx-rs/src/host/cli/sandbox/lock.rs +++ b/nvx-rs/src/host/cli/sandbox/lock.rs @@ -1,18 +1,21 @@ -//! The lifecycle lock of a state directory, which is the lock of the sandbox's -//! [`SandboxStore`]: its file `sandbox.lock`. +//! The lifecycle lock of a state directory: the store's +//! [lifecycle lock](crate::host::store#lifecycle-locks) of the sandbox, whose caller keeps its +//! configuration record, `config.json`, in the directory. //! //! - Provision, start, stop, and deprovision hold the lock for their whole transition, and exec //! while it finds the running VM. A caller that cannot take the lock retries every 25 ms //! within its timeout, and Ctrl-C ends the wait. -//! - The store takes the lock of the file that the path names when the lock is granted: a lock -//! file that its holder removed meanwhile locks nothing, so the store takes the lock of the -//! lock file at the path again, which it creates if needed. -//! - Deprovision removes the lock file before the directory. A transition that waited for it then -//! finds the directory gone, or takes the lock of a new lock file in the directory and finds no -//! sandbox there. Every transition that leaves the directory without a provisioned sandbox -//! removes its lock file as it releases the lock, so that the directory empties. +//! - The store takes the lock of the file that the path names when the lock is granted. A +//! provision whose directory deprovision removed meanwhile creates it again; any other +//! transition then fails, as for a path that names no directory. +//! - Deprovision [removes](crate::host::store#removal) the sandbox through the store, which +//! removes the lock file before the directory. A transition that waited for it then finds the +//! directory gone, or takes the lock of a new lock file in the directory and finds no sandbox +//! there. Every transition that leaves the directory without a provisioned sandbox removes its +//! lock file as it releases the lock, so that the directory empties. use std::io; +use std::path::Path; use std::time::Instant; use super::error_with_cause; @@ -20,9 +23,9 @@ use super::lifecycle::{CONFIG_NAME, LOCK_RETRY_INTERVAL, create_state_directory} use crate::ErrorKind; use crate::host::cli::deadline::Deadline; use crate::host::cli::{Exit, Result}; -use crate::host::python::fs::{self, os_error_message}; +use crate::host::python::fs::os_error_message; use crate::host::python::path::PurePath; -use crate::host::store::{LOCK_NAME, LockedStore, SandboxStore}; +use crate::host::store::{CallerFiles, LockedStore, RemovalError, SandboxStore}; /// How Python classifies an `OSError`, as far as the lifecycle tells the classes apart. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -67,57 +70,20 @@ pub(crate) fn transition_in_progress() -> Exit { Exit::error("another lifecycle transition of the sandbox is still in progress") } -/// Returns `os.lstat(path)`'s identity, or `None` if the path cannot be examined, as -/// `_directory_status` does. -pub(crate) fn directory_status(path: &PurePath) -> Option<(u64, u128)> { - fs::lstat(path.as_path()) - .ok() - .map(|status| status.identity()) +/// Returns the files that the CLI keeps in a state directory beside the store's: its +/// configuration record, whose presence means that the sandbox is provisioned. +fn caller_files() -> CallerFiles { + CallerFiles::new(CONFIG_NAME).expect("the configuration record has a plain file name") } -/// Returns whether `name` is a removed lock file that a handle still keeps, as `_lingers` does: -/// NFS renames a removed file that is still open to `.nfs*`, and Windows file systems without -/// POSIX deletion keep it under its own name, as they keep the lock files that waiting -/// transitions create until they release them. -pub(crate) fn lingers(name: &str) -> bool { - name == LOCK_NAME || name.starts_with(".nfs") -} - -/// Returns whether `error` is how `rmdir` reports a directory that is not empty. -pub(crate) fn directory_not_empty(error: &io::Error) -> bool { - #[cfg(windows)] - let not_empty = matches!(error.raw_os_error(), Some(145 | 80 | 183)); - #[cfg(unix)] - let not_empty = matches!( - error.raw_os_error(), - Some(code) if code == nix::errno::Errno::ENOTEMPTY as i32 - || code == nix::errno::Errno::EEXIST as i32 - ); - #[cfg(not(any(windows, unix)))] - let not_empty = error.kind() == io::ErrorKind::DirectoryNotEmpty; - not_empty -} - -/// Returns the names of the entries of `directory`. -pub(crate) fn entries(directory: &PurePath) -> io::Result> { - std::fs::read_dir(directory.as_path())? - .map(|entry| { - let name = entry?.file_name(); - name.into_string() - .map_err(|_| crate::host::python::not_unicode("a file name")) - }) - .collect() -} - -/// The exclusive lock that serializes the lifecycle transitions of a state directory: the lock -/// of its sandbox's store, through which the transition changes the store's records. +/// The exclusive lock that serializes the lifecycle transitions of a state directory: the +/// lifecycle lock of its sandbox's store, through which the transition changes the store's +/// records. Dropping it releases the lock. pub(crate) struct LifecycleLock { /// The state directory. state_dir: PurePath, - /// The sandbox's locked store, until the lock is released. - locked: Option, - /// Whether the holder removed its lock file, after which the path may name another's. - removed: bool, + /// The sandbox's locked store. + locked: LockedStore, } impl LifecycleLock { @@ -133,20 +99,21 @@ impl LifecycleLock { )); } let deadline = Deadline::after(timeout); + let files = caller_files(); loop { - let opened = if provision { - SandboxStore::create(state_dir.as_path()) - } else { - SandboxStore::open(state_dir.as_path()) - }; // The store tries the lock once for a deadline that has passed, so that this wait // checks for Ctrl-C between tries. - let error = match opened.and_then(|store| store.lock(Some(Instant::now()))) { + let now = Some(Instant::now()); + let taken = if provision { + SandboxStore::lock_for_provision(state_dir.as_path(), &files, now) + } else { + SandboxStore::lock_existing(state_dir.as_path(), &files, now) + }; + let error = match taken { Ok(locked) => { return Ok(Self { state_dir: state_dir.clone(), - locked: Some(locked), - removed: false, + locked, }); } Err(error) => error, @@ -160,6 +127,7 @@ impl LifecycleLock { "sandbox state path is not a plain directory: {state_dir}" ))); } + // The store creates the state directory again itself, once its parent exists. (_, Some(OsErrorClass::NotFound)) => { if deadline.passed() { return Err(transition_in_progress()); @@ -180,34 +148,63 @@ impl LifecycleLock { /// Returns the sandbox's locked store. pub(crate) fn locked(&mut self) -> &mut LockedStore { - self.locked.as_mut().expect("the lock is held") - } - - /// Removes the held lock file, as deprovision does before it removes the directory. - pub(crate) fn remove(&mut self) -> Result { - let path = self.state_dir.join(LOCK_NAME); - std::fs::remove_file(path.as_path()) - .map_err(|error| Exit::error(os_error_message(&error, Some(path.as_str()))))?; - self.removed = true; - Ok(()) + &mut self.locked } - /// Releases the lock. The lock file of a directory that holds no provisioned sandbox goes - /// first, unless the holder removed it already, so that the directory can be removed. - pub(crate) fn release(&mut self) { - let Some(locked) = self.locked.take() else { - return; + /// Removes the sandbox and its state directory through the store, as deprovision does once no + /// OpenVMM of the sandbox can run, waiting up to `timeout` seconds for lock files of other + /// transitions that keep the emptied directory, and releases the lock. + pub(crate) fn remove(self, timeout: f64) -> Result { + let Self { state_dir, locked } = self; + let dir = locked.dir().to_owned(); + let failed = |error| removal_error(error, &state_dir, &dir); + let Some(mut lingering) = locked.remove().map_err(failed)? else { + return Ok(()); }; - if !self.removed && !fs::lexists(self.state_dir.join(CONFIG_NAME).as_path()) { - let _ = std::fs::remove_file(self.state_dir.join(LOCK_NAME).as_path()); + // A removed lock file that a handle still keeps stays in the directory, as do the lock + // files of transitions that wait meanwhile. Each of them finds no sandbox, and removes + // its lock file as it releases the lock, so the directory empties after the removal + // released its own. The removal is tried once at a time, so that this wait checks for + // Ctrl-C between tries. + let deadline = Deadline::after(timeout); + loop { + match lingering.wait(Some(Instant::now())) { + Ok(()) => return Ok(()), + Err(RemovalError::TimedOut(error)) if !deadline.passed() => { + if !deadline.pause(LOCK_RETRY_INTERVAL)? { + return Err(failed(RemovalError::TimedOut(error))); + } + } + Err(error) => return Err(failed(error)), + } } - drop(locked); } } -impl Drop for LifecycleLock { - fn drop(&mut self) { - self.release(); +/// Returns the error of `nvx.py`'s deprovision for the store's removal of the sandbox in +/// `state_dir`, whose store names it `dir`, that `error` ended. +fn removal_error(error: RemovalError, state_dir: &PurePath, dir: &Path) -> Exit { + match error { + RemovalError::Launched => Exit::error("sandbox must be stopped before deprovision"), + RemovalError::Foreign(names) => Exit::error(format!( + "sandbox state directory contains files not owned by NVX: {}", + names.join(", ") + )), + RemovalError::ProvisionedAgain => { + Exit::error("sandbox was provisioned again while deprovision removed it") + } + RemovalError::TimedOut(error) => { + Exit::error(os_error_message(&error, Some(state_dir.as_str()))) + } + RemovalError::Io { path, error } => { + // The store names the directory, or one of its entries, by its own path. + let shown = match path.strip_prefix(dir).ok().map(Path::to_str) { + Some(Some("")) => state_dir.clone(), + Some(Some(name)) => state_dir.join(name), + _ => PurePath::native(&path.to_string_lossy()), + }; + Exit::error(os_error_message(&error, Some(shown.as_str()))) + } } } @@ -226,7 +223,7 @@ mod tests { let directory = tempfile::tempdir().unwrap(); let state = state_dir(&directory); std::fs::write(state.join(CONFIG_NAME).as_path(), "{}").unwrap(); - let mut held = LifecycleLock::acquire(&state, 10.0, false).unwrap(); + let held = LifecycleLock::acquire(&state, 10.0, false).unwrap(); let started = std::time::Instant::now(); let error = LifecycleLock::acquire(&state, 0.1, false).err().unwrap(); assert!(started.elapsed() >= std::time::Duration::from_millis(100)); @@ -234,42 +231,24 @@ mod tests { error.message(), Some("another lifecycle transition of the sandbox is still in progress") ); - held.release(); + drop(held); drop(LifecycleLock::acquire(&state, 10.0, false).unwrap()); - // A provisioned directory keeps its lock file. - assert!(state.join(LOCK_NAME).as_path().is_file()); } #[test] - fn released_locks_leave_no_file_in_unprovisioned_directories() { + fn missing_state_directories_are_created_by_provision_alone() { let directory = tempfile::tempdir().unwrap(); let state = state_dir(&directory); - drop(LifecycleLock::acquire(&state, 10.0, true).unwrap()); - assert!(!state.join(LOCK_NAME).as_path().exists()); let missing = state.join("missing"); let error = LifecycleLock::acquire(&missing, 1.0, false).err().unwrap(); assert_eq!( error.message().unwrap(), format!("sandbox state path is not a plain directory: {missing}") ); - // A provision creates the directory that deprovision removed meanwhile. - drop(LifecycleLock::acquire(&missing, 1.0, true).unwrap()); - assert!(missing.as_path().is_dir()); - } - - #[test] - fn a_removed_lock_file_is_not_removed_again_on_release() { - let directory = tempfile::tempdir().unwrap(); - let state = state_dir(&directory); - let mut lock = LifecycleLock::acquire(&state, 10.0, true).unwrap(); - lock.remove().unwrap(); - assert!(!state.join(LOCK_NAME).as_path().exists()); - // A transition that waited takes the lock of a new lock file, which stays its own. - let other = LifecycleLock::acquire(&state, 1.0, true).unwrap(); - lock.release(); - assert!(state.join(LOCK_NAME).as_path().is_file()); - drop(other); - assert!(!state.join(LOCK_NAME).as_path().exists()); + // A provision creates the directory that deprovision removed meanwhile, and its parents. + let nested = missing.join("nested"); + drop(LifecycleLock::acquire(&nested, 1.0, true).unwrap()); + assert!(nested.as_path().is_dir()); } #[cfg(unix)] @@ -286,4 +265,66 @@ mod tests { format!("the sandbox store {state} is accessible to other users (mode 755)") ); } + + #[test] + fn removals_keep_the_messages_of_deprovision() { + let directory = tempfile::tempdir().unwrap(); + let state = state_dir(&directory); + let dir = state.as_path().to_owned(); + let message = |error| { + removal_error(error, &state, &dir) + .message() + .unwrap() + .to_owned() + }; + assert_eq!( + message(RemovalError::Launched), + "sandbox must be stopped before deprovision" + ); + assert_eq!( + message(RemovalError::Foreign(vec![ + "b.txt".to_owned(), + "a.txt".to_owned() + ])), + "sandbox state directory contains files not owned by NVX: b.txt, a.txt" + ); + assert_eq!( + message(RemovalError::ProvisionedAgain), + "sandbox was provisioned again while deprovision removed it" + ); + let denied = || io::Error::from(io::ErrorKind::PermissionDenied); + let config = state.join(CONFIG_NAME); + for (path, shown) in [ + (dir.clone(), state.as_str()), + (dir.join(CONFIG_NAME), config.as_str()), + ] { + let expected = os_error_message(&denied(), Some(shown)); + assert_eq!( + message(RemovalError::Io { + path, + error: denied() + }), + expected + ); + } + assert_eq!( + message(RemovalError::TimedOut(denied())), + os_error_message(&denied(), Some(state.as_str())) + ); + } + + #[test] + fn removals_remove_the_state_directory() { + let directory = tempfile::tempdir().unwrap(); + let state = state_dir(&directory); + std::fs::write(state.join(CONFIG_NAME).as_path(), "{}").unwrap(); + let lock = LifecycleLock::acquire(&state, 10.0, false).unwrap(); + lock.remove(10.0).unwrap(); + assert!(!state.as_path().exists()); + let error = LifecycleLock::acquire(&state, 1.0, false).err().unwrap(); + assert_eq!( + error.message().unwrap(), + format!("sandbox state path is not a plain directory: {state}") + ); + } } From 91e05f5ac1598fbad608d002e5f9b3b15a33bee6 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 00:31:08 -0700 Subject: [PATCH 03/14] nvx-rs: declare instances' files and remove them through the store Instance::create and Instance::open take the files that the caller keeps beside the instance, Instance::lock takes the store's lifecycle lock of an existing sandbox, and the new LockedInstance::remove removes a stopped instance, with the caller's files and its directory, through the store. aci_edge_sandboxes declares sandbox.json as its configuration record, and removes sandboxes and the directories of stale IDs through the store, which replaces its own removal: - A deprovision that finds files that are not the backend's removes the sandbox's own files and its record first, as the CLI's does: the ID is stale from then on, and the directory keeps only those files. - Deprovision waits for lingering lock files without a deadline, as its lock does. - The directory of a stale ID goes with the store's leftovers, such as the temporary file of an interrupted replacement; a directory that is not private to the user is left alone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- aci_edge_sandboxes/README.md | 4 + aci_edge_sandboxes/src/openvmm/state.rs | 137 ++++++++++-------------- nvx-rs/README.md | 7 +- nvx-rs/src/host/instance.rs | 111 ++++++++++++++++--- nvx-rs/src/lib.rs | 5 +- nvx-rs/tests/fake_openvmm.rs | 8 +- 6 files changed, 168 insertions(+), 104 deletions(-) diff --git a/aci_edge_sandboxes/README.md b/aci_edge_sandboxes/README.md index 33b39237f..dbc06c5c1 100644 --- a/aci_edge_sandboxes/README.md +++ b/aci_edge_sandboxes/README.md @@ -965,6 +965,10 @@ egress default of `allow` and no deny rule that covers the guest network; see - Deprovisioning a running sandbox fails with `already_started`; stop it first. After deprovision, every call fails with `stale_id` without writing any state, as do calls with IDs that were never provisioned. +- Deprovision removes the sandbox's files, its record last, and then its state + directory. Files in that directory that are not the backend's stay: the + deprovision fails with `backend_error`, naming them, and the ID is stale + from then on. - Lifecycle transitions of one sandbox are serialized by a lock file. - Executions run one at a time: a concurrent exec waits up to `control_timeout` for the running one to finish. diff --git a/aci_edge_sandboxes/src/openvmm/state.rs b/aci_edge_sandboxes/src/openvmm/state.rs index 6c2fd3431..20ba9fb8b 100644 --- a/aci_edge_sandboxes/src/openvmm/state.rs +++ b/aci_edge_sandboxes/src/openvmm/state.rs @@ -20,11 +20,12 @@ use std::fs; use std::io; use std::path::{Path, PathBuf}; +use std::time::Instant; use nvx::host::filesystem::ExportPlan; use nvx::host::instance::{self, Instance, LockedInstance}; use nvx::host::platform; -use nvx::host::store::{FILE_NAMES, LOCK_NAME, LOG_NAME}; +use nvx::host::store::{CallerFiles, LOG_NAME, RemovalError}; use serde::{Deserialize, Serialize}; use crate::error::{Error, ErrorCode, Result}; @@ -73,31 +74,14 @@ pub(crate) struct StateStore { root: PathBuf, } -fn io_error(context: String) -> impl FnOnce(io::Error) -> Error { - move |error| Error::backend_error(context).with_source(error) -} - fn not_provisioned(sandbox_id: &SandboxId) -> Error { Error::stale_id(format!("sandbox {sandbox_id} is not provisioned")) } -/// Removes the directory `dir` of a sandbox whose configuration is gone, and returns whether it -/// is gone. -/// -/// The directory stays while it is not empty: a transition that waited for the sandbox's lock may -/// hold a lock file in it, and a file system without POSIX deletion keeps a removed lock file -/// there until its last handle closes. Each such transition finds no configuration, and removes -/// its lock file and the directory as it releases the lock. -fn remove_emptied(dir: &Path) -> Result { - match fs::remove_dir(dir) { - Ok(()) => Ok(true), - Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(true), - Err(error) if error.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(false), - Err(error) => Err(io_error(format!( - "cannot remove sandbox state {}", - dir.display() - ))(error)), - } +/// Returns the files that this crate keeps in a sandbox's directory beside the store of its +/// instance: the configuration, whose presence means that the sandbox is provisioned. +fn files() -> CallerFiles { + CallerFiles::new(RECORD_NAME).expect("the configuration has a plain file name") } impl StateStore { @@ -126,21 +110,20 @@ impl StateStore { /// /// An ID without configuration fails with [`ErrorCode::StaleId`], also when its state /// disappears while this call waits for the lock, as it does when a concurrent deprovision - /// removes it. A stale ID never becomes valid again, so the call removes the lock file that - /// it may have created, and the sandbox's directory, such as one that an interrupted removal - /// left, if that empties it: a stale ID leaves nothing behind. + /// removes it. A stale ID never becomes valid again, so the call removes what is left of the + /// sandbox's directory, such as one that an interrupted removal left, as the store removes a + /// sandbox: a stale ID leaves nothing behind but entries that are not the sandbox's. pub(crate) fn lock_and_load( &self, sandbox_id: &SandboxId, ) -> Result<(LockedInstance, SandboxRecord)> { let dir = self.dir(sandbox_id); - let locked = match Instance::open(&dir, sandbox_id.to_string()) + let locked = match Instance::open(&dir, sandbox_id.to_string(), files()) .and_then(|instance| instance.lock(None)) { Ok(locked) => locked, - // The directory or its lock file went away, or never existed, with the sandbox. + // The directory went away, or never existed, with the sandbox. Err(_) if matches!(dir.join(RECORD_NAME).try_exists(), Ok(false)) => { - let _ = remove_emptied(&dir); return Err(not_provisioned(sandbox_id)); } Err(error) => return Err(error.into()), @@ -148,9 +131,7 @@ impl StateStore { match self.load(sandbox_id) { Ok(record) => Ok((locked, record)), Err(error) if error.code() == ErrorCode::StaleId => { - let _ = fs::remove_file(dir.join(LOCK_NAME)); - drop(locked); - let _ = remove_emptied(&dir); + let _ = locked.remove(Some(Instant::now())); Err(error) } Err(error) => Err(error), @@ -168,7 +149,7 @@ impl StateStore { dir.display() ))); } - let created = Instance::create(&dir, sandbox_id.to_string()) + let created = Instance::create(&dir, sandbox_id.to_string(), files()) .and_then(|_| instance::write_json(&dir.join(RECORD_NAME), record)); if created.is_err() { let _ = fs::remove_dir_all(&dir); @@ -198,51 +179,28 @@ impl StateStore { }) } - /// Deletes the state of a stopped sandbox, whose lock `locked` holds, and releases the lock. + /// Deletes the state of a stopped sandbox, whose lock `locked` holds, through the store of its + /// instance, and releases the lock. /// - /// Unknown files abort the removal before anything is deleted. The configuration goes after - /// the instance's files, so an interrupted removal leaves a sandbox that can be deprovisioned - /// again, and the lock file goes last, while it is held, so that a transition that waits for - /// the lock finds no configuration. + /// The store removes its files and then the configuration, so an interrupted removal leaves a + /// sandbox that can be deprovisioned again. It then refuses entries that are neither the + /// store's nor this crate's, which it keeps with the directory, and the ID is stale from then + /// on. Otherwise it removes the lock file while it still holds the lock, so that a transition + /// that waits for the lock finds no sandbox, and then the directory, waiting for the lock + /// files of such transitions that keep it for a while, as it waits for the lock itself. pub(crate) fn remove(&self, sandbox_id: &SandboxId, locked: LockedInstance) -> Result<()> { - let dir = self.dir(sandbox_id); - let entries = fs::read_dir(&dir).map_err(io_error(format!( - "cannot list sandbox state {}", - dir.display() - )))?; - let mut owned = Vec::new(); - for entry in entries { - let entry = entry.map_err(io_error(format!( - "cannot list sandbox state {}", - dir.display() - )))?; - let name = entry.file_name(); - let name = name.to_string_lossy(); - let temporary = name.starts_with('.') && name.ends_with(".tmp"); - if name == RECORD_NAME || name == LOCK_NAME { - continue; - } - if FILE_NAMES.contains(&name.as_ref()) || temporary { - owned.push(entry.path()); - } else { - return Err(Error::backend_error(format!( - "sandbox state {} contains the unexpected entry {name:?}; remove it and retry", - dir.display() - ))); + locked.remove(None).map_err(|error| { + let message = format!( + "cannot remove sandbox state {}: {error}", + self.dir(sandbox_id).display() + ); + match error { + RemovalError::TimedOut(source) | RemovalError::Io { error: source, .. } => { + Error::backend_error(message).with_source(source) + } + _ => Error::backend_error(message), } - } - for path in owned { - instance::remove_if_present(&path)?; - } - instance::remove_if_present(&dir.join(RECORD_NAME))?; - instance::remove_if_present(&dir.join(LOCK_NAME))?; - // The directory goes while the lock is held where it can, and once it is released where - // the removed lock file stays until its last handle closes. - if !remove_emptied(&dir)? { - drop(locked); - remove_emptied(&dir)?; - } - Ok(()) + }) } } @@ -253,6 +211,7 @@ mod tests { use std::time::{Duration, Instant}; use nvx::ErrorKind; + use nvx::host::store::LOCK_NAME; use super::*; use crate::ErrorCode; @@ -320,16 +279,30 @@ mod tests { } #[test] - fn removal_preserves_unknown_files_and_the_sandbox() { + fn removal_refuses_unknown_files_and_keeps_them() { let root = tempfile::tempdir().unwrap(); let store = StateStore::open(root.path()).unwrap(); let id = SandboxId::generate().unwrap(); store.create(&id, &record()).unwrap(); - fs::write(store.dir(&id).join("keep.txt"), b"user data").unwrap(); + let dir = store.dir(&id); + fs::write(dir.join("openvmm.log"), b"left").unwrap(); + fs::write(dir.join("keep.txt"), b"user data").unwrap(); let (locked, _) = store.lock_and_load(&id).unwrap(); - assert!(store.remove(&id, locked).is_err()); - assert!(store.dir(&id).join("keep.txt").exists()); - assert_eq!(store.load(&id).unwrap(), record()); + let error = store.remove(&id, locked).unwrap_err(); + assert_eq!(error.code(), ErrorCode::BackendError); + assert!(error.message().ends_with(": keep.txt"), "{error}"); + // The sandbox's own files went first, its configuration last, so the ID is stale, and the + // directory keeps the unknown file alone. + assert_eq!(entries(&dir), ["keep.txt"]); + assert_eq!(fs::read(dir.join("keep.txt")).unwrap(), b"user data"); + let error = store.lock_and_load(&id).unwrap_err(); + assert_eq!(error.code(), ErrorCode::StaleId, "{error}"); + assert_eq!(entries(&dir), ["keep.txt"]); + // Once it is gone, the stale ID leaves nothing behind. + fs::remove_file(dir.join("keep.txt")).unwrap(); + let error = store.lock_and_load(&id).unwrap_err(); + assert_eq!(error.code(), ErrorCode::StaleId, "{error}"); + assert!(entries(root.path()).is_empty()); } #[test] @@ -431,7 +404,7 @@ mod tests { let (locked, loaded) = store.lock_and_load(&id).unwrap(); assert_eq!(loaded, record()); // The lock is the lock of the instance's store. - let contender = Instance::open(store.dir(&id), id.to_string()).unwrap(); + let contender = Instance::open(store.dir(&id), id.to_string(), files()).unwrap(); let error = contender.lock(Some(Instant::now())).unwrap_err(); assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); drop(locked); @@ -447,9 +420,9 @@ mod tests { // An interrupted removal can leave an empty directory, or one with only its lock file, // which are stale too. let emptied = SandboxId::generate().unwrap(); - fs::create_dir(store.dir(&emptied)).unwrap(); + Instance::create(store.dir(&emptied), emptied.to_string(), files()).unwrap(); let locked_only = SandboxId::generate().unwrap(); - Instance::create(store.dir(&locked_only), locked_only.to_string()).unwrap(); + Instance::create(store.dir(&locked_only), locked_only.to_string(), files()).unwrap(); fs::write(store.dir(&locked_only).join(LOCK_NAME), b"").unwrap(); for id in [&never_provisioned, &emptied, &locked_only] { let error = store.lock_and_load(id).unwrap_err(); diff --git a/nvx-rs/README.md b/nvx-rs/README.md index a67ce3f05..a2c0c76ad 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -75,7 +75,7 @@ nvx-rs/ | `nvx::host::network::policy` | Egress policy files: `compile_policy_file`, `compile_policy_bytes`, and `compile_policy` compile one to a `CompiledEgressPolicy` of native rules; see [Egress policy files](#egress-policy-files) | | `nvx::host::filesystem` | The `ExportPlan` of host paths through OpenVMM's aggregate virtio-fs device, the host-mapping table, the reserved guest targets, and `guest_path` and `resolve_guest_path` (also at `nvx::`) | | `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory | -| `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive. Under the store's lock, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; and `LockedInstance::stop` stops the VM with a forced fallback. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | +| `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive, and in which the caller keeps the files that it declares as `CallerFiles`. Under the store's lifecycle lock of an existing sandbox, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; `LockedInstance::stop` stops the VM with a forced fallback; and `LockedInstance::remove` removes a stopped instance, with the caller's files and its directory, through the store. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | | `nvx::host::nvxs` | `HostConnection`: the host's role of the NVXS session over a platform transport, on which `nvx::host::session` and other host applications run their protocols. It attaches with the launch capability, waiting while the broker asks it to, sends application bytes in as many DATA records as they take, and returns each DATA payload from the guest, with deadlines; a receive that times out keeps a partial record | | `nvx::host::process` | Verified process references. A `ProcessRef` names one process by its ID, its start time, and the `BootId` of the host boot during which it started. `Process::open` checks a reference and holds its process through a pidfd on Linux or a process handle on Windows, whether or not the caller started it, so that waits sleep until the process exits and termination never reaches a process that reuses its ID. A reference from an earlier boot names an exited process, and a process whose identity this user cannot check, such as another user's, makes `Process::open` fail rather than report an exit. Linux hosts without pidfds poll the reference instead, and cannot terminate the process | | `nvx::host::supervisor` | The supervisor of a sandbox's VMM, beneath the CLI, `Instance`, and external orchestrators, which keep the configuration, the authorization, and the application protocol. `launch` runs the store's launch steps under the sandbox's lock: it writes the journal's marker, spawns OpenVMM with the claimed log, records its `ProcessRef` right after, and returns a `Starting` launch that the caller commits once the VMM serves its control endpoint, or abandons. A launch that fails never leaves an unrecorded VMM running. `VmmCommand::inherit` lists the descriptors (Linux) or handles (Windows) that the VMM inherits under their numbers, besides its standard streams, and it inherits nothing else of the caller's: on Linux, they must be close-on-exec, a flag that only the VMM's own process clears; on Windows, they join the VMM's explicit handle list. `Vmm::stop` runs one graceful step of the caller's and waits for the VMM's exit event until a deadline, and takes a bounded forced step through the VMM's verified identity only on request. `reattach` recovers an interrupted launch, and holds the recorded VMM again after the orchestrator restarts within the same host boot: `Live`, `Mismatched` with another runtime identity, `Exited`, or `Indeterminate` | @@ -579,6 +579,7 @@ use std::time::Duration; use nvx::host::instance::{Found, Instance}; use nvx::host::launch::LaunchSpec; +use nvx::host::store::CallerFiles; use nvx::protocol::{Workload, WorkloadEnvironment}; use nvx::{Artifacts, Hypervisor}; @@ -586,7 +587,9 @@ fn main() -> Result<(), Box> { let artifacts = Artifacts::discover()?; let runtime = artifacts.runtime_identity()?; let spec = LaunchSpec::new(artifacts, Hypervisor::from_env_or_default()?); - let instance = Instance::create(std::env::temp_dir().join("nvx-example"), "example")?; + // The example keeps no files of its own beside the instance's store. + let directory = std::env::temp_dir().join("nvx-example"); + let instance = Instance::create(directory, "example", CallerFiles::none())?; // Lifecycle transitions hold the instance's lock; executions need none. let mut locked = instance.lock(None)?; let Found::Stopped = locked.find(|| Ok(runtime.clone()))? else { diff --git a/nvx-rs/src/host/instance.rs b/nvx-rs/src/host/instance.rs index d5909e600..544dff240 100644 --- a/nvx-rs/src/host/instance.rs +++ b/nvx-rs/src/host/instance.rs @@ -4,8 +4,9 @@ //! //! An instance lives in a [`SandboxStore`] of its own: a directory private to the current user, //! in which the store keeps the instance's lock, the journal of its launch, and the runtime -//! record of its VM, and in which the caller may keep configuration records of its own under -//! other names: +//! record of its VM, and in which the caller may keep files of its own under other names, which it +//! declares as [`CallerFiles`], such as the configuration record whose presence means that the +//! instance is provisioned: //! //! ```text //! / @@ -16,10 +17,13 @@ //! control.sock Linux control endpoint (running only) //! openvmm.log OpenVMM output of the latest start //! outcome.json OpenVMM outcome report of the latest run +//! ... the caller's files //! ``` //! //! Lifecycle transitions run under the instance's lock, through the [`LockedInstance`] that -//! [`Instance::lock`] returns, so that every operation can run in a different process: +//! [`Instance::lock`] returns, so that every operation can run in a different process. The lock is +//! the store's [lifecycle lock](super::store#lifecycle-locks) of an existing sandbox, which knows +//! the caller's files: //! //! - [`LockedInstance::find`] finds the instance's VM as an orchestrator does after it restarts //! within the same host boot, with [`supervisor::reattach`]: it ends a start whose caller died, @@ -36,6 +40,8 @@ //! cannot mount them all. //! - [`LockedInstance::stop`] asks the guest to shut down and falls back to terminating OpenVMM //! through its verified process identity. +//! - [`LockedInstance::remove`] removes a stopped instance, with the caller's files, and its +//! directory, through the store's [removal](super::store#removal). //! //! [`Instance::exec`] runs a workload through the control console of a [`Running`] VM without the //! lock, and [`Execution::pump`] streams its output live. The guest serializes executions: a @@ -59,8 +65,8 @@ use super::process::KILL_TIMEOUT; use super::report::{self, Report}; use super::session::{self, ControlSession, ExecEvent, SessionError}; use super::store::{ - IdentityPart, LockedStore, OUTCOME_NAME, Recovery, RuntimeIdentity, RuntimeRecord, SOCKET_NAME, - SandboxStore, + CallerFiles, IdentityPart, LockedStore, OUTCOME_NAME, Recovery, RemovalError, RuntimeIdentity, + RuntimeRecord, SOCKET_NAME, SandboxStore, }; use super::supervisor::{self, Reattached, Starting, StopOutcome, Vmm, VmmCommand}; use crate::error::{Error, ErrorKind, Result}; @@ -326,25 +332,39 @@ pub struct Instance { store: SandboxStore, /// Sandbox name used in diagnostics. name: String, + /// The files that the caller keeps in the instance's directory. + files: CallerFiles, } impl Instance { /// Creates the directory `dir` of a new instance, whose parent must exist, private to the /// current user, as [`SandboxStore::create`] does, and supervises the instance in it, which - /// messages call `sandbox NAME`. - pub fn create(dir: impl Into, name: impl Into) -> Result { + /// messages call `sandbox NAME`, and in which the caller keeps `files`. + pub fn create( + dir: impl Into, + name: impl Into, + files: CallerFiles, + ) -> Result { Ok(Self { store: SandboxStore::create(dir)?, name: name.into(), + files, }) } /// Supervises the instance in the existing directory `dir`, which must be private to the - /// current user, as [`SandboxStore::open`] requires, and which messages call `sandbox NAME`. - pub fn open(dir: impl Into, name: impl Into) -> Result { + /// current user, as [`SandboxStore::open`] requires, which messages call `sandbox NAME`, and + /// in which the caller keeps `files`. Fails with [`ErrorKind::NotFound`] if `dir` does not + /// exist. + pub fn open( + dir: impl Into, + name: impl Into, + files: CallerFiles, + ) -> Result { Ok(Self { store: SandboxStore::open(dir)?, name: name.into(), + files, }) } @@ -363,6 +383,11 @@ impl Instance { &self.name } + /// Returns the files that the caller keeps in the instance's directory. + pub fn files(&self) -> &CallerFiles { + &self.files + } + /// Returns the path of the OpenVMM log of the latest start. pub fn log_path(&self) -> PathBuf { self.store.log_path() @@ -374,10 +399,16 @@ impl Instance { } /// Takes the instance's lock, which serializes its lifecycle transitions, waiting for it - /// until `deadline`, or for as long as it takes without one, as [`SandboxStore::lock`] does. + /// until `deadline`, or for as long as it takes without one, as + /// [`SandboxStore::lock_existing`] does with the caller's files. + /// + /// Fails with [`ErrorKind::NotFound`] once the instance's directory is gone, as a removal + /// that this waited for leaves it, and with [`ErrorKind::TimedOut`] when the deadline passes + /// first. A caller that keeps a configuration record checks it under the lock: the lock of a + /// directory without it is released without a lock file. pub fn lock(&self, deadline: Option) -> Result { Ok(LockedInstance { - locked: self.store.lock(deadline)?, + locked: SandboxStore::lock_existing(self.dir(), &self.files, deadline)?, instance: self.clone(), }) } @@ -684,6 +715,23 @@ impl LockedInstance { report: report::read(&self.instance.report_path()), }) } + + /// Removes the stopped instance: its store's records and files, the caller's files that it + /// declared, the configuration record last, and its directory, through the store's + /// [`LockedStore::remove`], and releases the lock. Lock files of other transitions that keep + /// the emptied directory are then waited for until `deadline`, or for as long as it takes + /// without one, as [`Lingering::wait`](super::store::Lingering::wait) does. + /// + /// The caller has found the instance stopped with [`find`](Self::find): the removal refuses, + /// and removes nothing, while the store records a VM or a start. Entries in the directory + /// that are neither the store's nor the caller's are refused, and kept with the directory, + /// once the rest is gone. + pub fn remove(self, deadline: Option) -> std::result::Result<(), RemovalError> { + match self.locked.remove()? { + None => Ok(()), + Some(mut lingering) => lingering.wait(deadline), + } + } } /// Fails with [`ErrorKind::Unsupported`] when the duration that `name` describes is longer than @@ -883,12 +931,21 @@ mod tests { use super::super::*; use crate::host::launch::PROCESS_COMMAND_LINE_LIMIT; - use crate::host::store::{ArtifactIdentity, JOURNAL_NAME, RUNTIME_NAME}; + use crate::host::store::{ + ArtifactIdentity, JOURNAL_NAME, LOCK_NAME, LOG_NAME, RUNTIME_NAME, + }; + + /// The configuration record of these tests' caller. + const CONFIG: &str = "config.json"; fn instance() -> (tempfile::TempDir, Instance) { let directory = tempfile::tempdir().unwrap(); - let instance = - Instance::create(directory.path().join("vm"), "aci-edge-sandboxes:0123").unwrap(); + let instance = Instance::create( + directory.path().join("vm"), + "aci-edge-sandboxes:0123", + CallerFiles::new(CONFIG).unwrap(), + ) + .unwrap(); (directory, instance) } @@ -907,10 +964,34 @@ mod tests { .unwrap(); assert!(matches!(found, Found::Stopped), "{found:?}"); drop(locked); - let reopened = Instance::open(instance.dir(), instance.name()).unwrap(); + // The instance's caller recorded no configuration, so its lock file went too. + assert!(!instance.dir().join(LOCK_NAME).exists()); + let reopened = + Instance::open(instance.dir(), instance.name(), instance.files().clone()).unwrap(); assert_eq!(reopened, instance); } + #[test] + fn stopped_instances_are_removed_with_the_callers_files() { + let (_directory, instance) = instance(); + for name in [CONFIG, LOG_NAME, RUNTIME_NAME] { + fs::write(instance.dir().join(name), b"{}").unwrap(); + } + // An instance whose store records a VM is not removed. + let locked = instance.lock(None).unwrap(); + assert!(matches!(locked.remove(None), Err(RemovalError::Launched))); + assert!(instance.dir().join(CONFIG).is_file()); + fs::remove_file(instance.dir().join(RUNTIME_NAME)).unwrap(); + instance.lock(None).unwrap().remove(None).unwrap(); + assert!(!instance.dir().exists()); + // A transition that comes later finds the instance gone. + let error = instance.lock(None).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + let error = + Instance::open(instance.dir(), instance.name(), CallerFiles::none()).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + } + #[test] fn records_of_the_earlier_format_fail_naming_it() { // Before the store, instances kept runtime records and launch markers of format 3. diff --git a/nvx-rs/src/lib.rs b/nvx-rs/src/lib.rs index a71154ebd..ff820dcfd 100644 --- a/nvx-rs/src/lib.rs +++ b/nvx-rs/src/lib.rs @@ -49,6 +49,7 @@ //! //! use nvx::host::instance::{Found, Instance}; //! use nvx::host::launch::LaunchSpec; +//! use nvx::host::store::CallerFiles; //! use nvx::protocol::{Workload, WorkloadEnvironment}; //! use nvx::{Artifacts, Hypervisor}; //! @@ -57,7 +58,9 @@ //! let artifacts = Artifacts::discover()?; //! let runtime = artifacts.runtime_identity()?; //! let spec = LaunchSpec::new(artifacts, Hypervisor::from_env_or_default()?); -//! let instance = Instance::create(std::env::temp_dir().join("nvx-example"), "example")?; +//! // The example keeps no files of its own beside the instance's store. +//! let directory = std::env::temp_dir().join("nvx-example"); +//! let instance = Instance::create(directory, "example", CallerFiles::none())?; //! // Lifecycle transitions hold the instance's lock; executions need none. //! let mut locked = instance.lock(None)?; //! let Found::Stopped = locked.find(|| Ok(runtime.clone()))? else { diff --git a/nvx-rs/tests/fake_openvmm.rs b/nvx-rs/tests/fake_openvmm.rs index 365539324..b19dcb575 100644 --- a/nvx-rs/tests/fake_openvmm.rs +++ b/nvx-rs/tests/fake_openvmm.rs @@ -20,7 +20,7 @@ use std::time::Duration; use nvx::host::instance::{Found, Instance, Running, Started, Stopped, WorkloadOutcome}; use nvx::host::launch::{LaunchSpec, PROCESS_COMMAND_LINE_LIMIT}; -use nvx::host::store::{ArtifactIdentity, IdentityPart, RuntimeIdentity}; +use nvx::host::store::{ArtifactIdentity, CallerFiles, IdentityPart, RuntimeIdentity}; use nvx::protocol::{Workload, WorkloadEnvironment}; use nvx::{Artifacts, ErrorKind, Hypervisor}; @@ -56,7 +56,7 @@ fn spec(directory: &Path) -> LaunchSpec { /// Creates the fake guest artifacts and an instance directory below `directory`. fn setup(directory: &Path) -> (LaunchSpec, Instance) { - let instance = Instance::create(directory.join("vm"), "test").unwrap(); + let instance = Instance::create(directory.join("vm"), "test", CallerFiles::none()).unwrap(); (spec(directory), instance) } @@ -360,7 +360,7 @@ fn orchestrator_role() { return; }; let spec = spec(&env::current_dir().unwrap()); - let instance = Instance::create(PathBuf::from(dir), "test").unwrap(); + let instance = Instance::create(PathBuf::from(dir), "test", CallerFiles::none()).unwrap(); start(&instance, &spec).unwrap(); } @@ -385,7 +385,7 @@ fn a_restarted_orchestrator_reattaches_to_its_vm() { // The orchestrator restarts within the same host boot: it finds the VM that its earlier run // started, through the records of the store, and drives it. - let instance = Instance::open(&dir, "test").unwrap(); + let instance = Instance::open(&dir, "test", CallerFiles::none()).unwrap(); let idle = AtomicBool::new(false); let output = run(&instance, &workload(&["/bin/echo", "reattached"]), &idle); assert_eq!(output.outcome, WorkloadOutcome::Exited(0)); From cb8fc3b4aa747f35495dc2fa2904219325f287d9 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 00:31:18 -0700 Subject: [PATCH 04/14] nvx-rs: own the VMM's report staging file in the store A VMM that is killed while it writes its outcome report leaves the file in which it stages it, `.tmp` and six ASCII letters or digits, as the tempfile crate names it, which deprovision refused as a foreign file. The store now owns files of exactly that form. It removes them once no process of their launch runs: when a recovery or an abandoned launch ends it, unless that is indeterminate, in clear_runtime, when the next launch begins, and at removal. It keeps them while a process of the launch may still run. Other `.tmp*` names stay foreign, and callers cannot declare names of that form. The sandbox lifecycle acceptance now requires the outcome report of a refused managed start, which a leftover staging file no longer reveals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- doc/run.md | 14 ++-- nvx-rs/README.md | 2 +- nvx-rs/src/host/cli/sandbox/lifecycle.rs | 8 +-- nvx-rs/src/host/store/caller.rs | 47 +++++++++++-- nvx-rs/src/host/store/lifecycle.rs | 4 +- nvx-rs/src/host/store/lifecycle/tests.rs | 6 +- nvx-rs/src/host/store/mod.rs | 70 ++++++++++++++++---- nvx-rs/src/host/store/tests.rs | 42 ++++++++++++ scripts/nvx_tools/sandbox_lifecycle_tests.py | 4 ++ scripts/test_microvm_tests.py | 31 +++------ 10 files changed, 174 insertions(+), 54 deletions(-) diff --git a/doc/run.md b/doc/run.md index 2c21c9d25..8639a418b 100644 --- a/doc/run.md +++ b/doc/run.md @@ -1078,7 +1078,11 @@ the workload identity, OpenVMM closes the control endpoint and shuts down. `start` then waits up to `--timeout` seconds for OpenVMM to publish `outcome.json` and exit, and fails with the report's category and status, such as `guest-exit status 125`. It terminates OpenVMM only if OpenVMM outlives that -wait. Managed +wait. OpenVMM writes the report to a file of its own in the state directory, +`.tmp` and six ASCII letters or digits, before it publishes it: NVX removes +such a file, which an OpenVMM that is terminated while it writes the report +leaves, once no OpenVMM of the sandbox runs, and `deprovision` removes it with +NVX's other files. Managed workload arguments use the bounded control protocol rather than the kernel command line and may contain whitespace. Managed execution can select an absolute working directory and either repeated inline `KEY=VALUE` entries or a @@ -1140,10 +1144,10 @@ named. It checks that: - `provision` rejects a root `--workload-user` before it creates any state, and the guest refuses an identity that the Ubuntu image lacks before a one-shot workload starts, and in a managed `start`, which then fails with - `guest-exit status 125` from OpenVMM's outcome report and leaves only - `config.json`, `sandbox.lock`, `openvmm.log`, and that `outcome.json`, - without a runtime record, capability, control socket, report staging file, - or OpenVMM process; + `guest-exit status 125` from the outcome report that OpenVMM must publish + before it exits, and leaves only `config.json`, `sandbox.lock`, + `openvmm.log`, and that `outcome.json`, without a runtime record, + capability, control socket, report staging file, or OpenVMM process; - every operation fails on a state directory that was never provisioned, a repeated `provision` leaves the configuration unchanged, and `exec` and `stop` fail before `start`; diff --git a/nvx-rs/README.md b/nvx-rs/README.md index a2c0c76ad..47e517b53 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -74,7 +74,7 @@ nvx-rs/ | `nvx::host::network` | `NetworkPolicy` and `openvmm_arguments`: the egress default and rules, ingress, host-loopback access and forwards as OpenVMM options. Egress rules are lowered exactly into OpenVMM's IPv4 and IPv6 rules by the lowering of egress policy files, with at most 256 rules for each action | | `nvx::host::network::policy` | Egress policy files: `compile_policy_file`, `compile_policy_bytes`, and `compile_policy` compile one to a `CompiledEgressPolicy` of native rules; see [Egress policy files](#egress-policy-files) | | `nvx::host::filesystem` | The `ExportPlan` of host paths through OpenVMM's aggregate virtio-fs device, the host-mapping table, the reserved guest targets, and `guest_path` and `resolve_guest_path` (also at `nvx::`) | -| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory | +| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory. The store also owns the file in which the VMM stages its outcome report, which a VMM that is killed while it writes the report leaves, and removes it once no process of its launch runs | | `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive, and in which the caller keeps the files that it declares as `CallerFiles`. Under the store's lifecycle lock of an existing sandbox, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; `LockedInstance::stop` stops the VM with a forced fallback; and `LockedInstance::remove` removes a stopped instance, with the caller's files and its directory, through the store. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | | `nvx::host::nvxs` | `HostConnection`: the host's role of the NVXS session over a platform transport, on which `nvx::host::session` and other host applications run their protocols. It attaches with the launch capability, waiting while the broker asks it to, sends application bytes in as many DATA records as they take, and returns each DATA payload from the guest, with deadlines; a receive that times out keeps a partial record | | `nvx::host::process` | Verified process references. A `ProcessRef` names one process by its ID, its start time, and the `BootId` of the host boot during which it started. `Process::open` checks a reference and holds its process through a pidfd on Linux or a process handle on Windows, whether or not the caller started it, so that waits sleep until the process exits and termination never reaches a process that reuses its ID. A reference from an earlier boot names an exited process, and a process whose identity this user cannot check, such as another user's, makes `Process::open` fail rather than report an exit. Linux hosts without pidfds poll the reference instead, and cannot terminate the process | diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle.rs b/nvx-rs/src/host/cli/sandbox/lifecycle.rs index 0becdac07..3b9463e21 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle.rs @@ -543,8 +543,8 @@ fn start_locked( 0.0 }; let status = end_failed_start(starting.child(), grace, &command)?; - // OpenVMM has exited, so the launch ends: its capability, control socket, and journal go, - // and its log and outcome report stay. + // OpenVMM has exited, so the launch ends: its capability, control socket, report staging + // file, and journal go, and its log and outcome report stay. let unended = match starting.abandon() { Ok(Recovery::Indeterminate(reason)) => Some(reason), Ok(_) => None, @@ -647,8 +647,8 @@ fn extend_recorded_network_arguments(command: &mut Vec, config: &Documen /// Ends the OpenVMM process of a failed start, as `_end_failed_start` does: returns OpenVMM's /// status if it exits on its own within `grace` seconds, or terminates it and returns `None`. -/// Terminating OpenVMM while it publishes its outcome report would leave the report's staging -/// file in the state directory, which deprovision refuses to remove. +/// Terminating OpenVMM while it publishes its outcome report would lose the report, and leave +/// only its staging file, which the store removes as the launch ends. fn end_failed_start( launched: &mut Launched, grace: f64, diff --git a/nvx-rs/src/host/store/caller.rs b/nvx-rs/src/host/store/caller.rs index 0c9ebc3c9..aa499b6f9 100644 --- a/nvx-rs/src/host/store/caller.rs +++ b/nvx-rs/src/host/store/caller.rs @@ -25,9 +25,9 @@ use crate::error::{Error, Result}; /// `"`, `*`, `:`, `<`, `>`, `?`, or `|`, no trailing dot or space, which Windows drops, and it is /// not a name that Windows reserves for a device, such as `NUL` or `com1.json`, so it names the /// same file on every host. -/// - It is none of the store's [`FILE_NAMES`], the lock included, does not have the form of a -/// temporary file that the store owns, `.NAME.NUMBER.tmp`, or of a lock file that a removal -/// waits for, `.nfs*`. +/// - It is none of the store's [`FILE_NAMES`], the lock included, has neither form of a temporary +/// file that the store owns, and does not have the form of a lock file that a removal waits +/// for, `.nfs*`. /// - It is declared once: the configuration record's name differs from the others. /// /// Names are compared without regard to case, as Windows and macOS file systems compare them. @@ -106,7 +106,7 @@ impl CallerFiles { let folded = name.to_lowercase(); if FILE_NAMES.contains(&folded.as_str()) { Some("is the name of one of the store's files") - } else if is_store_temporary(&folded) { + } else if is_store_temporary(&folded) || is_report_staging(&folded) { Some("has the form of a temporary file that the store owns") } else if folded.starts_with(".nfs") { Some("has the form of a removed lock file that a removal waits for") @@ -180,6 +180,15 @@ pub(super) fn is_store_temporary(name: &str) -> bool { }) } +/// Returns whether `name` is the file in which the VMM stages its outcome report before it +/// publishes it as `outcome.json`, which a VMM that is killed while it writes the report leaves: +/// `.tmp` and six ASCII letters or digits, the name that the `tempfile` crate gives it. +pub(super) fn is_report_staging(name: &str) -> bool { + name.strip_prefix(".tmp").is_some_and(|suffix| { + suffix.len() == 6 && suffix.bytes().all(|byte| byte.is_ascii_alphanumeric()) + }) +} + #[cfg(test)] mod tests { use super::*; @@ -298,6 +307,14 @@ mod tests { ".SANDBOX.LOCK.1.TMP", "has the form of a temporary file that the store owns", ), + ( + ".tmpAbC123", + "has the form of a temporary file that the store owns", + ), + ( + ".TMP000000", + "has the form of a temporary file that the store owns", + ), ( ".nfs000000001", "has the form of a removed lock file that a removal waits for", @@ -345,9 +362,31 @@ mod tests { ".runtime.json.1a.tmp", "runtime.json.1.tmp", ".runtime.json.1.tmp.tmp", + ".tmpAbC123", ".Runtime.json.1.tmp", ] { assert!(!is_store_temporary(name), "{name}"); } } + + #[test] + fn report_staging_files_have_the_names_that_tempfile_gives_them() { + for name in [".tmpAbC123", ".tmp000000", ".tmpzzzzzz"] { + assert!(is_report_staging(name), "{name}"); + } + // Other names are not the VMM's, whatever they resemble. + for name in [ + ".tmpAbC12", + ".tmpAbC1234", + ".tmpAb-123", + ".tmpAb.123", + ".tmpAbC12\u{e9}", + ".TMPAbC123", + "tmpAbC123", + ".tmp", + ".runtime.json.1.tmp", + ] { + assert!(!is_report_staging(name), "{name}"); + } + } } diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index acb077b7f..e8cd43a4a 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -10,7 +10,7 @@ use std::path::{Path, PathBuf}; use std::thread; use std::time::Instant; -use super::caller::is_store_temporary; +use super::caller::{is_report_staging, is_store_temporary}; use super::{ CallerFiles, FILE_NAMES, JOURNAL_NAME, LOCK_NAME, LOCK_RETRY_INTERVAL, LockedStore, RUNTIME_NAME, SandboxStore, Taken, absolute, @@ -208,7 +208,7 @@ impl LockedStore { if name == LOCK_NAME { continue; } - if is_store_temporary(&name) { + if is_store_temporary(&name) || is_report_staging(&name) { remove_entry(self.path(&name))?; } else { foreign.push(name); diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index dc8b2f7a4..901bc2999 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -324,6 +324,7 @@ fn a_removal_removes_the_store_the_callers_files_and_the_directory() { BOOT, ".runtime.json.12345.tmp", ".sandbox.lock.7.tmp", + ".tmpAbC123", ] { fs::write(dir.join(name), b"left").unwrap(); } @@ -347,9 +348,10 @@ fn a_removal_refuses_foreign_entries_and_keeps_them() { ".config.json.1.tmp", ".runtime.json.1a.tmp", "outcome.json.old", + ".tmpAbC1234", "directory", ]; - for name in &foreign[..4] { + for name in &foreign[..5] { fs::write(dir.join(name), b"kept").unwrap(); } fs::create_dir(dir.join("directory")).unwrap(); @@ -369,7 +371,7 @@ fn a_removal_refuses_foreign_entries_and_keeps_them() { "the sandbox's directory holds entries that are neither the store's nor its caller's: a, b" ); // Once they are gone, the directory goes too. - for name in &foreign[..4] { + for name in &foreign[..5] { fs::remove_file(dir.join(name)).unwrap(); } fs::remove_dir(dir.join("directory")).unwrap(); diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index ccf5296a6..640fe51e2 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -12,6 +12,7 @@ //! openvmm.log the VMM's output, claimed by the launch that created it //! console.log the guest console of the latest launch, if the caller captures it //! outcome.json the VMM's outcome report of the latest launch +//! .tmpXXXXXX the outcome report while the VMM writes it, which it then publishes //! ... the caller's configuration record and other files, which it declares //! ``` //! @@ -150,6 +151,18 @@ //! [`supervisor::reattach`](super::supervisor::reattach) holds that VM again after its //! orchestrator restarts. //! +//! # The report staging file +//! +//! The VMM writes its outcome report to a file of its own in the directory, `.tmp` and six ASCII +//! letters or digits, as the `tempfile` crate names it, and then publishes it as `outcome.json`. +//! A VMM that is killed while it writes the report leaves that file. The store owns files of +//! exactly that form, and removes them wherever it clears a launch's files once no process of +//! the launch runs: when [`LockedStore::recover`] or [`Launch::abandon`] ends a launch, unless +//! that is [`Recovery::Indeterminate`], in [`LockedStore::clear_runtime`], when +//! [`LockedStore::begin_launch`] begins the next launch, and in a [removal](#removal). It keeps +//! them while a process of the launch may still run. A file of any other name is not the +//! store's. +//! //! # Lifecycle locks //! //! A caller that keeps a configuration record in the sandbox's directory, whose presence means @@ -175,8 +188,9 @@ //! //! [`LockedStore::remove`] removes a sandbox that no VMM can run in: it refuses while the store //! records a VM or a launch, which the caller reattaches to, or recovers, first. It removes the -//! store's files and the caller's, the configuration record last, and the temporary files that an -//! interrupted replacement of one of the store's files leaves, `.NAME.NUMBER.tmp`. It then +//! store's files and the caller's, the configuration record last, and the temporary files that the +//! store owns: those that an interrupted replacement of one of its files leaves, +//! `.NAME.NUMBER.tmp`, and the VMM's [report staging file](#the-report-staging-file). It then //! refuses entries that are neither the store's nor the caller's, naming them, and keeps the //! directory, or removes its lock file while it still holds the lock, and then the directory. //! @@ -205,6 +219,7 @@ use std::thread; use std::time::{Duration, Instant}; pub use self::caller::CallerFiles; +use self::caller::is_report_staging; use self::files::{io_error, remove_if_present}; pub use self::identity::{ ArtifactIdentity, IdentityCheck, IdentityPart, MAX_IDENTITY_LEN, RuntimeIdentity, @@ -243,7 +258,9 @@ pub const CONSOLE_NAME: &str = "console.log"; /// Name of the VMM's outcome report. pub const OUTCOME_NAME: &str = "outcome.json"; /// Every file that the store keeps in a sandbox's directory. Replacing a record leaves a -/// temporary file named `.NAME.NUMBER.tmp` beside it only if the replacement was interrupted. +/// temporary file named `.NAME.NUMBER.tmp` beside it only if the replacement was interrupted, and +/// a VMM that is killed while it writes its outcome report leaves the file in which it stages it; +/// see [the report staging file](self#the-report-staging-file). pub const FILE_NAMES: [&str; 8] = [ LOCK_NAME, JOURNAL_NAME, @@ -495,6 +512,25 @@ impl SandboxStore { self.dir.display() )) } + + /// Removes the files in which a VMM of the sandbox staged an outcome report that it never + /// published, which only a VMM that was killed while it wrote the report leaves. The caller + /// knows that no process of the sandbox's launches runs. + fn remove_report_staging(&self) -> Result<()> { + let failed = |error| { + io_error(format!( + "cannot list the sandbox store {}", + self.dir.display() + ))(error) + }; + for entry in std::fs::read_dir(&self.dir).map_err(failed)? { + let name = entry.map_err(failed)?.file_name(); + if name.to_str().is_some_and(is_report_staging) { + remove_if_present(&self.dir.join(name))?; + } + } + Ok(()) + } } /// The store of a sandbox whose lock this holds, through which its lifecycle transitions change @@ -547,8 +583,9 @@ impl LockedStore { /// /// It stores the capability, replaces the log of the previous launch with a fresh one, which /// it claims, removes the outcome report, console log, and control socket of the previous - /// launch, chooses a control endpoint, and writes the journal's marker. Nothing is spawned - /// yet: the caller spawns the VMM with [`Launch::take_log`]. + /// launch, and any outcome report that its VMM staged without publishing it, chooses a control + /// endpoint, and writes the journal's marker. Nothing is spawned yet: the caller spawns the + /// VMM with [`Launch::take_log`]. /// /// Fails if the sandbox's VM runs, or a launch was interrupted and is not recovered yet, and /// with [`ErrorKind::Unsupported`] for an all-zero capability @@ -580,6 +617,7 @@ impl LockedStore { for path in [&socket, &self.outcome_path(), &self.console_path()] { remove_if_present(path)?; } + self.remove_report_staging()?; let endpoint = platform::control_endpoint(&socket) .map_err(|error| Error::io("cannot choose a control endpoint").with_source(error))?; let (log, claim) = self.create_log()?; @@ -616,9 +654,9 @@ impl LockedStore { /// Recovery signals only a VMM that it holds through a verified [`Process`]: the one that the /// journal's [`ProcessRef`] names, or the one that serves the journal's control endpoint, as /// the [module documentation](self#recovery) describes. Once nothing of the launch runs, it - /// removes the launch's capability, control socket, and journal, and keeps its log. A - /// recovery that cannot prove that is [`Recovery::Indeterminate`] and keeps them all for a - /// later recovery. + /// removes the launch's capability, control socket, outcome report that its VMM staged + /// without publishing it, and journal, and keeps its log. A recovery that cannot prove that is + /// [`Recovery::Indeterminate`] and keeps them all for a later recovery. /// /// A journal that a committed runtime record of the same launch supersedes is removed. A /// runtime record of another launch fails as inconsistent state. @@ -649,11 +687,13 @@ impl LockedStore { None => self.end_unidentified(journal), }; if !matches!(recovery, Recovery::Indeterminate(_)) { - // The journal goes last, so that an interrupted removal leaves it for a later - // recovery to finish. - for name in [CAPABILITY_NAME, SOCKET_NAME, JOURNAL_NAME] { + for name in [CAPABILITY_NAME, SOCKET_NAME] { remove_if_present(&self.path(name))?; } + self.remove_report_staging()?; + // The journal goes last, so that an interrupted removal leaves it for a later + // recovery to finish. + remove_if_present(&self.path(JOURNAL_NAME))?; } Ok(recovery) } @@ -757,12 +797,14 @@ impl LockedStore { } /// Removes the runtime record, the launch capability, and the control socket of a VM that has - /// exited, the runtime record last. The log, console log, and outcome report outlive the run. + /// exited, and any outcome report that its VMM staged without publishing it, the runtime + /// record last. The log, console log, and outcome report outlive the run. pub fn clear_runtime(&mut self) -> Result<()> { - for name in [CAPABILITY_NAME, SOCKET_NAME, RUNTIME_NAME] { + for name in [CAPABILITY_NAME, SOCKET_NAME] { remove_if_present(&self.path(name))?; } - Ok(()) + self.remove_report_staging()?; + remove_if_present(&self.path(RUNTIME_NAME)) } } diff --git a/nvx-rs/src/host/store/tests.rs b/nvx-rs/src/host/store/tests.rs index 9f53586f4..71761d548 100644 --- a/nvx-rs/src/host/store/tests.rs +++ b/nvx-rs/src/host/store/tests.rs @@ -828,6 +828,48 @@ fn a_journal_that_its_runtime_record_supersedes_is_removed() { drop(locked.begin_launch(&runtime(), &CAPABILITY).unwrap()); } +#[test] +fn report_staging_files_go_once_nothing_of_their_launch_runs() { + let (_directory, store) = store(); + // The file in which a VMM that was killed staged its outcome report, and other names, which + // are not the store's. + let staging = store.path(".tmpAbC123"); + let others = [".tmpAbC1234", ".tmpAb-123", ".tmp"]; + for name in others { + fs::write(store.path(name), b"{").unwrap(); + } + fs::write(&staging, b"{").unwrap(); + let mut locked = store.lock(None).unwrap(); + // A launch begins without the staging file of an earlier one. + let mut launch = locked.begin_launch(&runtime(), &CAPABILITY).unwrap(); + assert!(!staging.exists()); + // While a process of the launch may still hold its log, as its VMM does, the file stays. + let log = launch.take_log().unwrap(); + drop(launch); + fs::write(&staging, b"{").unwrap(); + let recovery = locked.recover().unwrap(); + assert!( + matches!(recovery, Recovery::Indeterminate(_)), + "{recovery:?}" + ); + assert!(staging.exists()); + // Once nothing holds the log, recovery ends the launch and removes the file. + drop(log); + assert!(released(|| locked.recover().unwrap() == Recovery::Exited)); + assert!(!staging.exists()); + // An abandoned launch removes it, and so does clearing the runtime of a VM that exited. + let launch = locked.begin_launch(&runtime(), &CAPABILITY).unwrap(); + fs::write(&staging, b"{").unwrap(); + assert_eq!(launch.abandon().unwrap(), Recovery::Exited); + assert!(!staging.exists()); + fs::write(&staging, b"{").unwrap(); + locked.clear_runtime().unwrap(); + assert!(!staging.exists()); + for name in others { + assert!(store.path(name).exists(), "{name}"); + } +} + #[test] fn launch_identities_are_fresh_hexadecimal() { let first = LaunchId::generate().unwrap(); diff --git a/scripts/nvx_tools/sandbox_lifecycle_tests.py b/scripts/nvx_tools/sandbox_lifecycle_tests.py index 08c8be37f..e276aec16 100644 --- a/scripts/nvx_tools/sandbox_lifecycle_tests.py +++ b/scripts/nvx_tools/sandbox_lifecycle_tests.py @@ -505,6 +505,10 @@ def check_identity_admission(self) -> None: ) self.copy_evidence(self.identity_state / LOG, "identity-openvmm.log") self.copy_evidence(self.identity_state / OUTCOME, "identity-outcome.json") + # OpenVMM publishes its report before it exits, rather than being ended + # while it writes it, which left only the report's staging file (#438). + if not (self.identity_state / OUTCOME).is_file(): + raise RuntimeError("the refused managed start published no outcome report") self.require_entries( self.identity_state, STOPPED_ENTRIES, "after a refused managed start" ) diff --git a/scripts/test_microvm_tests.py b/scripts/test_microvm_tests.py index c3dfe0fac..5713cfffe 100644 --- a/scripts/test_microvm_tests.py +++ b/scripts/test_microvm_tests.py @@ -1261,6 +1261,10 @@ def _deprovision( "sandbox.lock", ): (state / name).unlink(missing_ok=True) + # The store owns the file in which OpenVMM stages its outcome report. + for path in state.iterdir(): + if re.fullmatch(r"\.tmp[A-Za-z0-9]{6}", path.name): + path.unlink() unknown = sorted(path.name for path in state.iterdir()) if unknown and "deprovision-removes-foreign" not in self.faults: return self._error( @@ -1454,6 +1458,11 @@ def test_acceptance_detects_lifecycle_regressions(self): "start returned 1, expected 1 with 'error: OpenVMM exited during " "startup: guest-exit status 125'" ), + # OpenVMM ended while it published its report (#438), which left + # only the report's staging file. + "failed-start-kills-reporter": ( + "the refused managed start published no outcome report" + ), "duplicate-start": "start returned 0, expected 1", "exec-loses-state": "the state-reading request returned status 1", "probe-fails": "exec returned 1, expected 0", @@ -1495,28 +1504,6 @@ def test_acceptance_detects_lifecycle_regressions(self): (root / "results" / "sandbox-lifecycle-checks.json").is_file() ) - def test_acceptance_detects_a_leaked_report_staging_file(self): - # A start that ends OpenVMM while it publishes its outcome report leaves - # the report's staging file, which deprovision refuses to remove, so the - # acceptance preserves its fixture, as on the WHP runners in issue #438. - with tempfile.TemporaryDirectory() as temporary: - root = Path(temporary) - fake = _FakeSandboxCli("failed-start-kills-reporter") - with self.assertRaises(RuntimeError) as raised: - self._run(root, fake) - fixtures = list(root.glob("nvx-public-lifecycle-*")) - - message = str(raised.exception) - for fragment in ( - "after a refused managed start: the state directory holds " - "['.tmpAbC123', 'config.json', 'openvmm.log', 'sandbox.lock']", - "sandbox state directory contains files not owned by NVX: .tmpAbC123", - "fixture preserved for recovery", - ): - self.assertIn(fragment, message) - self.assertEqual(len(fixtures), 1) - self.assertFalse(any(fake.running.values())) - def test_evidence_keeps_the_end_of_long_logs(self): with tempfile.TemporaryDirectory() as temporary: output_dir = self._run(Path(temporary), _FakeSandboxCli("noisy-log")) From 9c0a898e501a99e2217edbd9dcfc0fb9fb963e40 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 02:10:45 -0700 Subject: [PATCH 05/14] nvx-rs: remove only the held directory, and only with a config record Three races in the store's lifecycle protocol: - A removal removed the directory by its path before it checked that the path still named the directory that it held, so an empty directory that replaced it, once another removal finished this one, could go. Each attempt to remove the directory now checks the path's identity first, in LockedStore::remove and in Lingering::wait, and the removal holds the directory from before it removes its lock file. - lock_for_provision failed with NotFound when a concurrent removal deleted the directory after it was created and before it was checked. It now creates the directory again while its parent exists, at once the first time and then every 25 ms until the deadline. - Without a configuration record, a transition that waited through a removal could not tell that the sandbox was gone, and kept the lock file that it created, which a lingering removal then waited for forever. A removal now needs the caller's configuration record, and a lock taken without one refuses it with RemovalError::Undeclared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/README.md | 2 +- nvx-rs/src/host/cli/sandbox/lock.rs | 2 + nvx-rs/src/host/instance.rs | 12 ++- nvx-rs/src/host/store/caller.rs | 3 +- nvx-rs/src/host/store/lifecycle.rs | 126 +++++++++++++++++------ nvx-rs/src/host/store/lifecycle/tests.rs | 85 +++++++++++++++ nvx-rs/src/host/store/mod.rs | 28 ++--- 7 files changed, 210 insertions(+), 48 deletions(-) diff --git a/nvx-rs/README.md b/nvx-rs/README.md index 47e517b53..9caff6ba7 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -74,7 +74,7 @@ nvx-rs/ | `nvx::host::network` | `NetworkPolicy` and `openvmm_arguments`: the egress default and rules, ingress, host-loopback access and forwards as OpenVMM options. Egress rules are lowered exactly into OpenVMM's IPv4 and IPv6 rules by the lowering of egress policy files, with at most 256 rules for each action | | `nvx::host::network::policy` | Egress policy files: `compile_policy_file`, `compile_policy_bytes`, and `compile_policy` compile one to a `CompiledEgressPolicy` of native rules; see [Egress policy files](#egress-policy-files) | | `nvx::host::filesystem` | The `ExportPlan` of host paths through OpenVMM's aggregate virtio-fs device, the host-mapping table, the reserved guest targets, and `guest_path` and `resolve_guest_path` (also at `nvx::`) | -| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory. The store also owns the file in which the VMM stages its outcome report, which a VMM that is killed while it writes the report leaves, and removes it once no process of its launch runs | +| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in, for a caller that declares its configuration record: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory; both remove the directory only while the path still names it. The store also owns the file in which the VMM stages its outcome report, which a VMM that is killed while it writes the report leaves, and removes it once no process of its launch runs | | `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive, and in which the caller keeps the files that it declares as `CallerFiles`. Under the store's lifecycle lock of an existing sandbox, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; `LockedInstance::stop` stops the VM with a forced fallback; and `LockedInstance::remove` removes a stopped instance, with the caller's files and its directory, through the store. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | | `nvx::host::nvxs` | `HostConnection`: the host's role of the NVXS session over a platform transport, on which `nvx::host::session` and other host applications run their protocols. It attaches with the launch capability, waiting while the broker asks it to, sends application bytes in as many DATA records as they take, and returns each DATA payload from the guest, with deadlines; a receive that times out keeps a partial record | | `nvx::host::process` | Verified process references. A `ProcessRef` names one process by its ID, its start time, and the `BootId` of the host boot during which it started. `Process::open` checks a reference and holds its process through a pidfd on Linux or a process handle on Windows, whether or not the caller started it, so that waits sleep until the process exits and termination never reaches a process that reuses its ID. A reference from an earlier boot names an exited process, and a process whose identity this user cannot check, such as another user's, makes `Process::open` fail rather than report an exit. Linux hosts without pidfds poll the reference instead, and cannot terminate the process | diff --git a/nvx-rs/src/host/cli/sandbox/lock.rs b/nvx-rs/src/host/cli/sandbox/lock.rs index c60c3d96c..b52b3419d 100644 --- a/nvx-rs/src/host/cli/sandbox/lock.rs +++ b/nvx-rs/src/host/cli/sandbox/lock.rs @@ -185,6 +185,8 @@ impl LifecycleLock { /// `state_dir`, whose store names it `dir`, that `error` ended. fn removal_error(error: RemovalError, state_dir: &PurePath, dir: &Path) -> Exit { match error { + // The CLI always declares its configuration record. + error @ RemovalError::Undeclared => Exit::error(error.to_string()), RemovalError::Launched => Exit::error("sandbox must be stopped before deprovision"), RemovalError::Foreign(names) => Exit::error(format!( "sandbox state directory contains files not owned by NVX: {}", diff --git a/nvx-rs/src/host/instance.rs b/nvx-rs/src/host/instance.rs index 544dff240..9c3c990f4 100644 --- a/nvx-rs/src/host/instance.rs +++ b/nvx-rs/src/host/instance.rs @@ -723,9 +723,9 @@ impl LockedInstance { /// without one, as [`Lingering::wait`](super::store::Lingering::wait) does. /// /// The caller has found the instance stopped with [`find`](Self::find): the removal refuses, - /// and removes nothing, while the store records a VM or a start. Entries in the directory - /// that are neither the store's nor the caller's are refused, and kept with the directory, - /// once the rest is gone. + /// and removes nothing, while the store records a VM or a start, and for an instance whose + /// caller declared no configuration record. Entries in the directory that are neither the + /// store's nor the caller's are refused, and kept with the directory, once the rest is gone. pub fn remove(self, deadline: Option) -> std::result::Result<(), RemovalError> { match self.locked.remove()? { None => Ok(()), @@ -982,6 +982,12 @@ mod tests { assert!(matches!(locked.remove(None), Err(RemovalError::Launched))); assert!(instance.dir().join(CONFIG).is_file()); fs::remove_file(instance.dir().join(RUNTIME_NAME)).unwrap(); + // Nor is one whose caller declared no configuration record. + let undeclared = + Instance::open(instance.dir(), instance.name(), CallerFiles::none()).unwrap(); + let locked = undeclared.lock(None).unwrap(); + assert!(matches!(locked.remove(None), Err(RemovalError::Undeclared))); + assert!(instance.dir().join(CONFIG).is_file()); instance.lock(None).unwrap().remove(None).unwrap(); assert!(!instance.dir().exists()); // A transition that comes later finds the instance gone. diff --git a/nvx-rs/src/host/store/caller.rs b/nvx-rs/src/host/store/caller.rs index aa499b6f9..de5fc6f29 100644 --- a/nvx-rs/src/host/store/caller.rs +++ b/nvx-rs/src/host/store/caller.rs @@ -14,7 +14,8 @@ use crate::error::{Error, Result}; /// its lock file as it releases the lock, and removal removes the caller's files with the store's, /// the configuration record last. A caller that declares no configuration record, as /// [`none`](Self::none) declares, keeps nothing that tells whether its sandbox is provisioned, -/// so the directory itself is the sandbox: its lock file stays when the lock is released. +/// so the directory itself is the sandbox: its lock file stays when the lock is released, and the +/// store cannot be removed with the declaration. /// /// Each name is one plain file name, which [`new`](Self::new) and [`with`](Self::with) check, so /// a declaration is valid before any store is created, opened, locked, or removed with it: diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index e8cd43a4a..a30ca2d4d 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -13,7 +13,7 @@ use std::time::Instant; use super::caller::{is_report_staging, is_store_temporary}; use super::{ CallerFiles, FILE_NAMES, JOURNAL_NAME, LOCK_NAME, LOCK_RETRY_INTERVAL, LockedStore, - RUNTIME_NAME, SandboxStore, Taken, absolute, + RUNTIME_NAME, SandboxStore, Taken, absolute, transition_in_progress, }; use crate::error::{Error, ErrorKind, Result}; use crate::host::platform; @@ -29,31 +29,57 @@ impl SandboxStore { /// /// The directory, whose parent must exist, is created if needed, private to the current user, /// as [`create`](Self::create) creates it, and created again if a concurrent removal deletes - /// it before the lock is taken. The caller checks under the lock whether its configuration - /// record exists already, and writes it. + /// it before the lock is taken, also while it is created and checked. The caller checks under + /// the lock whether its configuration record exists already, and writes it. /// /// Fails with [`ErrorKind::TimedOut`] when the deadline passes first, also while Windows /// refuses to open a removed lock file, as [`lock_existing`](Self::lock_existing) describes, - /// with [`ErrorKind::NotFound`] if the parent does not exist, and as [`create`](Self::create) - /// fails for a directory that is not private. + /// or while concurrent removals keep deleting the directory, with [`ErrorKind::NotFound`] if + /// the parent does not exist, and as [`create`](Self::create) fails for a directory that is not + /// private. pub fn lock_for_provision( dir: impl Into, files: &CallerFiles, deadline: Option, ) -> Result { let dir = absolute(dir.into())?; + Self::provision_lock(&dir, files, deadline, |dir| Self::create(dir)) + } + + /// Takes the lock as [`lock_for_provision`](Self::lock_for_provision) does, with `create` + /// creating or checking the absolute directory `dir`, as [`create`](Self::create) does. + fn provision_lock( + dir: &Path, + files: &CallerFiles, + deadline: Option, + mut create: impl FnMut(&Path) -> Result, + ) -> Result { + // Whether the latest attempt followed at once one that found the directory gone. + let mut retried_at_once = false; loop { - let store = Self::create(dir.clone())?; - match store.take_lifecycle_lock(deadline) { - Ok(Taken::Locked(lock)) => { - return Ok(LockedStore::new(store, lock, files.clone())); - } - // A removal deleted the lock file, and maybe the directory, while this waited, or - // the directory after it was created: it is created and locked again. - Ok(Taken::Replaced) => {} - Err(error) if error.kind() == ErrorKind::NotFound => {} - Err(error) => return Err(error), + let error = match create(dir) { + Ok(store) => match store.take_lifecycle_lock(deadline) { + Ok(Taken::Locked(lock)) => { + return Ok(LockedStore::new(store, lock, files.clone())); + } + // A removal deleted the lock file while this waited for its lock, and maybe the + // directory: the directory is created and locked again. + Ok(Taken::Replaced) => continue, + Err(error) => error, + }, + Err(error) => error, + }; + // A removal deleted the directory after it was created, before it was checked or its + // lock file was opened: unless its parent is gone, it is created again, at once the + // first time. Another such failure right after it pauses first, until the deadline, + // so that a directory that keeps disappearing cannot spin. + if error.kind() != ErrorKind::NotFound || !dir.parent().is_some_and(Path::is_dir) { + return Err(error); + } + if retried_at_once && !pause(deadline) { + return Err(transition_in_progress(dir)); } + retried_at_once = !retried_at_once; } } @@ -94,7 +120,7 @@ impl SandboxStore { match self.take_lock(deadline) { Err(error) if cfg!(windows) && denied(&error) => { if !pause(deadline) { - return Err(self.transition_in_progress()); + return Err(transition_in_progress(self.dir())); } } taken => return taken, @@ -152,6 +178,12 @@ impl LockedStore { /// Removes the sandbox's store, its caller's files, and its directory, which only a sandbox /// that no VMM can run in allows, and releases the lock. /// + /// The removal needs the caller's configuration record: a lock that was taken without one, as + /// with [`CallerFiles::none`] or by [`SandboxStore::lock`], refuses with + /// [`RemovalError::Undeclared`], and removes nothing, since a transition that waits through + /// the removal could not tell that the sandbox is gone then, and would keep the lock file that + /// it creates. + /// /// The removal refuses, and removes nothing, while the store records a VM or a launch: the /// caller reattaches to the VM and stops it, or recovers the launch, first, as /// [`supervisor::reattach`](crate::host::supervisor::reattach) does. It then removes the @@ -162,7 +194,7 @@ impl LockedStore { /// them and the directory; the lock file goes as the lock is released, since the configuration /// record is gone. Otherwise it removes its lock file while it still holds the lock, so that a /// transition that waited for the lock finds the sandbox gone rather than halfway removed, and - /// then the directory. + /// then the directory, as long as the path still names it. /// /// Lock files of other transitions can keep the emptied directory: on NFS, a removed lock file /// that a handle still holds is renamed to `.nfs*`; a Windows file system without POSIX @@ -179,9 +211,13 @@ impl LockedStore { } /// Empties the directory but for the lock file, as [`remove`](Self::remove) does first: it - /// refuses while the store records a VM or a launch, removes the store's files and the - /// caller's, and the temporary files that the store owns, and refuses any other entry. + /// refuses without the caller's configuration record and while the store records a VM or a + /// launch, removes the store's files and the caller's, and the temporary files that the store + /// owns, and refuses any other entry. fn remove_contents(&self) -> std::result::Result<(), RemovalError> { + if self.files.config().is_none() { + return Err(RemovalError::Undeclared); + } for name in [JOURNAL_NAME, RUNTIME_NAME] { let path = self.path(name); match fs::symlink_metadata(&path) { @@ -225,18 +261,21 @@ impl LockedStore { /// [`remove`](Self::remove) does once the directory holds nothing else of the sandbox. fn remove_directory(mut self) -> std::result::Result, RemovalError> { let dir = self.dir().to_owned(); + // While the lock is held, its file keeps the directory, which the path then names. + let (identity, held) = hold_directory(&dir).map_err(|error| RemovalError::Io { + path: dir.clone(), + error, + })?; let lock = self.path(LOCK_NAME); fs::remove_file(&lock).map_err(|error| RemovalError::Io { path: lock, error })?; self.lock_removed = true; - match fs::remove_dir(&dir) { - Ok(()) => return Ok(None), + match remove_held_directory(&dir, identity) { + // The directory is gone: this removal removed it, or another one did once it emptied, + // after which the path may name another directory. + Ok(_) => return Ok(None), Err(error) if directory_not_empty(&error) => {} Err(error) => return Err(RemovalError::Io { path: dir, error }), } - let (identity, held) = hold_directory(&dir).map_err(|error| RemovalError::Io { - path: dir.clone(), - error, - })?; let config = self.files.config().map(str::to_owned); drop(self); Ok(Some(Lingering { @@ -283,15 +322,16 @@ impl Lingering { /// again, and a removal that timed out can go on. /// /// Returns once the directory is gone, whoever removed it, or once the path names another - /// directory. Fails, and keeps the directory, with [`RemovalError::ProvisionedAgain`] once + /// directory, which it checks before each attempt to remove it. Fails, and keeps the + /// directory, with [`RemovalError::ProvisionedAgain`] once /// the caller's configuration record appears in it, as a provision that took the lock after /// the removal released it writes it, with [`RemovalError::Foreign`] for other entries that /// are not lingering lock files, sorted, and with [`RemovalError::TimedOut`] once the deadline /// passes. pub fn wait(&mut self, deadline: Option) -> std::result::Result<(), RemovalError> { loop { - let error = match fs::remove_dir(&self.dir) { - Ok(()) => return Ok(()), + let error = match remove_held_directory(&self.dir, self.identity) { + Ok(_) => return Ok(()), Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), Err(error) => error, }; @@ -352,6 +392,9 @@ impl Lingering { #[derive(Debug)] #[non_exhaustive] pub enum RemovalError { + /// The lock was taken without the caller's configuration record, which the removal needs, and + /// nothing was removed: see [`LockedStore::remove`]. + Undeclared, /// The store records a VM or a launch, so a VMM of the sandbox may still run, and nothing was /// removed. Launched, @@ -377,6 +420,10 @@ pub enum RemovalError { impl fmt::Display for RemovalError { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + Self::Undeclared => formatter.write_str( + "the removal of a sandbox needs its caller's configuration record, which its lock \ + was not taken with", + ), Self::Launched => { formatter.write_str("the sandbox's store records a VM or a launch, which may run") } @@ -404,13 +451,14 @@ impl std::error::Error for RemovalError { } } -/// Keeps the message of a removal's error, and its I/O cause: a timeout is of -/// [`ErrorKind::TimedOut`], an I/O failure of [`ErrorKind::Io`], and every other refusal of -/// [`ErrorKind::Other`]. +/// Keeps the message of a removal's error, and its I/O cause: a removal without the caller's +/// configuration record is of [`ErrorKind::Unsupported`], a timeout of [`ErrorKind::TimedOut`], an +/// I/O failure of [`ErrorKind::Io`], and every other refusal of [`ErrorKind::Other`]. impl From for Error { fn from(error: RemovalError) -> Self { let message = error.to_string(); match error { + RemovalError::Undeclared => Error::unsupported(message), RemovalError::TimedOut(source) => Error::timed_out(message).with_source(source), RemovalError::Io { error: source, .. } => Error::io(message).with_source(source), _ => Error::other(message), @@ -489,3 +537,19 @@ fn hold_directory(path: &Path) -> io::Result<((u64, u128), Option)> { fn directory_status(path: &Path) -> Option<(u64, u128)> { lstat(path).ok().map(|status| status.identity()) } + +/// Removes the directory `dir` if the path still names the one that the caller holds, whose +/// identity, which `lstat` gives, is `identity`, so that a directory that replaced it stays. +/// Returns whether the path named it, and fails as the check of the path or the removal fails. +/// +/// The check and the removal are two steps: a directory that replaces the held one between them, +/// which only a concurrent removal of the held one and a creation of another at the path both +/// allow, still goes while it is empty, and its creator then finds it gone, as a provision does, +/// which creates it again. +fn remove_held_directory(dir: &Path, identity: (u64, u128)) -> io::Result { + if lstat(dir)?.identity() != identity { + return Ok(false); + } + fs::remove_dir(dir)?; + Ok(true) +} diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index 901bc2999..c911ac230 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -459,6 +459,91 @@ fn a_lingering_removal_reports_what_appears_in_the_directory() { assert_eq!(entries_of(&dir), [RENAMED]); } +#[test] +fn a_lingering_removal_keeps_an_empty_directory_that_replaced_its_own() { + let (_directory, dir) = sandbox(); + let mut lingering = remove_lingering(&dir); + // Another removal finishes this one, and the path is created again, still empty. + fs::remove_file(dir.join(RENAMED)).unwrap(); + fs::remove_dir(&dir).unwrap(); + fs::create_dir(&dir).unwrap(); + lingering.wait(Some(Instant::now())).unwrap(); + assert!(dir.is_dir()); + assert!(entries_of(&dir).is_empty()); +} + +#[test] +fn only_the_directory_that_the_path_still_names_is_removed() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + fs::create_dir(&dir).unwrap(); + let (identity, held) = hold_directory(&dir).unwrap(); + // Another removal removes the held directory, and another one replaces it, which stays. + fs::remove_dir(&dir).unwrap(); + fs::create_dir(&dir).unwrap(); + assert!(!remove_held_directory(&dir, identity).unwrap()); + assert!(dir.is_dir()); + drop(held); + let (identity, _held) = hold_directory(&dir).unwrap(); + assert!(remove_held_directory(&dir, identity).unwrap()); + assert!(!dir.exists()); + // A path that names nothing fails as its check does. + let error = remove_held_directory(&dir, identity).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::NotFound, "{error}"); +} + +#[test] +fn a_removal_needs_the_configuration_record() { + let (_directory, dir) = sandbox(); + fs::write(dir.join(LOG_NAME), b"log").unwrap(); + // Without it, a transition that waits through the removal could not tell that the sandbox + // is gone, so neither a lifecycle lock without one nor the store's own lock removes it. + let undeclared = SandboxStore::lock_existing(&dir, &CallerFiles::none(), None).unwrap(); + assert!(matches!(undeclared.remove(), Err(RemovalError::Undeclared))); + let own = SandboxStore::open(&dir).unwrap().lock(None).unwrap(); + assert!(matches!(own.remove(), Err(RemovalError::Undeclared))); + assert_eq!(entries_of(&dir), sorted(&[CONFIG, LOCK_NAME, LOG_NAME])); + let error = Error::from(RemovalError::Undeclared); + assert_eq!(error.kind(), ErrorKind::Unsupported, "{error}"); +} + +#[test] +fn a_provision_creates_again_a_directory_that_a_removal_deletes_while_it_is_created() { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + // A concurrent removal deletes the directory after it is created, before it is checked. + let mut attempts = 0; + let locked = SandboxStore::provision_lock(&dir, &files(), within(10), |dir| { + attempts += 1; + if attempts == 1 { + SandboxStore::create(dir).unwrap(); + fs::remove_dir(dir).unwrap(); + return SandboxStore::open(dir); + } + SandboxStore::create(dir) + }) + .unwrap(); + assert_eq!(attempts, 2); + assert!(!locked.dir().join(CONFIG).exists()); + assert_eq!(entries_of(&dir), [LOCK_NAME]); + drop(locked); + // A missing parent fails at once. + let orphan = directory.path().join("missing").join("sandbox"); + let error = SandboxStore::lock_for_provision(&orphan, &files(), within(10)).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); + // A directory that keeps disappearing fails once the deadline passes. + let gone = directory.path().join("gone"); + let mut attempts = 0; + let deadline = Some(Instant::now() + Duration::from_millis(200)); + let error = SandboxStore::provision_lock(&dir, &files(), deadline, |_| { + attempts += 1; + SandboxStore::open(&gone) + }) + .unwrap_err(); + assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); + assert!(attempts > 2, "{attempts}"); +} + #[test] fn a_provision_that_waits_through_a_removal_provisions_the_sandbox_again() { let (_directory, dir) = sandbox(); diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index 640fe51e2..73a08ba61 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -171,7 +171,7 @@ //! transitions, and removals of the sandbox then keep its directory consistent, from any process: //! //! - [`SandboxStore::lock_for_provision`] creates the directory if needed, and again if a -//! concurrent removal deletes it before the lock is taken. +//! concurrent removal deletes it before the lock is taken, even while it is created. //! - [`SandboxStore::lock_existing`] fails with [`ErrorKind::NotFound`] once the directory is gone, //! also when a concurrent removal deletes it while the lock is awaited. The caller checks its //! configuration record under the lock: a directory that was never provisioned, that an @@ -192,7 +192,9 @@ //! store owns: those that an interrupted replacement of one of its files leaves, //! `.NAME.NUMBER.tmp`, and the VMM's [report staging file](#the-report-staging-file). It then //! refuses entries that are neither the store's nor the caller's, naming them, and keeps the -//! directory, or removes its lock file while it still holds the lock, and then the directory. +//! directory, or removes its lock file while it still holds the lock, and then the directory. A +//! removal needs the caller's configuration record, whose absence tells the transitions that wait +//! through it that the sandbox is gone: a lock taken without one refuses it. //! //! Lock files that other transitions hold can keep the emptied directory for a while: NFS renames //! a removed file that is still open to `.nfs*`, Windows file systems without POSIX deletion keep @@ -200,7 +202,8 @@ //! after the removal removed it creates a new one. Each such transition finds no sandbox, and //! removes its lock file as it releases its lock. The removal then holds the directory's identity, //! releases its lock, and returns a [`Lingering`] removal, which waits until they go, and reports -//! a provision that took the lock meanwhile as [`RemovalError::ProvisionedAgain`]. +//! a provision that took the lock meanwhile as [`RemovalError::ProvisionedAgain`]. A removal +//! removes the directory only while the path still names it, which it checks before each attempt. mod caller; mod files; @@ -312,6 +315,15 @@ fn store_error(context: String) -> impl FnOnce(io::Error) -> Error { } } +/// Returns the error of a wait for another lifecycle transition of the sandbox in `dir` that is +/// still in progress past its deadline. +fn transition_in_progress(dir: &Path) -> Error { + Error::timed_out(format!( + "another lifecycle transition of the sandbox in {} is still in progress", + dir.display() + )) +} + /// How an attempt to take a store's lock ended, if it did not fail. enum Taken { /// The lock of the lock file that the path names. @@ -485,7 +497,7 @@ impl SandboxStore { Err(TryLockError::WouldBlock) => { let left = deadline.saturating_duration_since(Instant::now()); if left.is_zero() { - return Err(self.transition_in_progress()); + return Err(transition_in_progress(&self.dir)); } thread::sleep(left.min(LOCK_RETRY_INTERVAL)); } @@ -505,14 +517,6 @@ impl SandboxStore { } } - /// Returns the error of a wait for the lock that another transition holds past its deadline. - fn transition_in_progress(&self) -> Error { - Error::timed_out(format!( - "another lifecycle transition of the sandbox in {} is still in progress", - self.dir.display() - )) - } - /// Removes the files in which a VMM of the sandbox staged an outcome report that it never /// published, which only a VMM that was killed while it wrote the report leaves. The caller /// knows that no process of the sandbox's launches runs. From 58821f27c53e2bffa660dcd71b0cc26963a2d0b3 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 02:56:25 -0700 Subject: [PATCH 06/14] nvx-rs: refuse aliasing names, wait for .nfs, accept raced removals A caller's file name now has only ASCII letters and digits, `.`, `_`, and `-`, and names are compared by ASCII case. File systems compare other names by rules of their own: they fold the case of other letters, macOS's take a composed letter for its decomposed form, and Windows also opens a file by its short name, such as `RUNTIM~1.JSO` for `runtime.json`, so a declared name could open one of the store's files, or another declared one. A removal no longer refuses a `.nfs*` entry that it finds as it empties the directory. NFS keeps a removed file that a handle still holds under such a name until the handle closes, so the removal leaves it, as it leaves lock files, and waits for it. A removal whose emptied directory another removal deletes after it removed its lock file now succeeds, as `Lingering::wait` does, rather than failing because the directory is not found. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- aci_edge_sandboxes/src/openvmm/state.rs | 5 +- doc/run.md | 20 +++-- nvx-rs/README.md | 2 +- nvx-rs/src/host/cli/sandbox/lifecycle.rs | 2 +- nvx-rs/src/host/cli/sandbox/lock.rs | 14 +-- nvx-rs/src/host/instance.rs | 7 +- nvx-rs/src/host/store/caller.rs | 53 ++++++++++-- nvx-rs/src/host/store/lifecycle.rs | 103 +++++++++++++---------- nvx-rs/src/host/store/lifecycle/tests.rs | 47 +++++++++-- nvx-rs/src/host/store/mod.rs | 19 +++-- 10 files changed, 180 insertions(+), 92 deletions(-) diff --git a/aci_edge_sandboxes/src/openvmm/state.rs b/aci_edge_sandboxes/src/openvmm/state.rs index 20ba9fb8b..f88241d1f 100644 --- a/aci_edge_sandboxes/src/openvmm/state.rs +++ b/aci_edge_sandboxes/src/openvmm/state.rs @@ -186,8 +186,9 @@ impl StateStore { /// sandbox that can be deprovisioned again. It then refuses entries that are neither the /// store's nor this crate's, which it keeps with the directory, and the ID is stale from then /// on. Otherwise it removes the lock file while it still holds the lock, so that a transition - /// that waits for the lock finds no sandbox, and then the directory, waiting for the lock - /// files of such transitions that keep it for a while, as it waits for the lock itself. + /// that waits for the lock finds no sandbox, and then the directory, waiting for the open + /// files, such as the lock files of such transitions, that keep it for a while, as it waits + /// for the lock itself. pub(crate) fn remove(&self, sandbox_id: &SandboxId, locked: LockedInstance) -> Result<()> { locked.remove(None).map_err(|error| { let message = format!( diff --git a/doc/run.md b/doc/run.md index 8639a418b..815f18d92 100644 --- a/doc/run.md +++ b/doc/run.md @@ -1063,15 +1063,17 @@ workload does not delay `stop`. A provisioned state directory keeps transition that leaves a state directory without `config.json`, such as one of a sandbox that was never provisioned, removes `sandbox.lock` as it releases the lock. These are the rules of the sandbox store, which other orchestrators of -sandboxes share through `nvx-rs`. On NFS, -and on Windows file systems without POSIX deletion such as ReFS, a removed lock -file keeps the emptied directory until its last handle closes, so `deprovision` -then releases the lock before it removes the directory. A transition that -waited while `deprovision` removed the sandbox then fails as for a sandbox that -is not provisioned, except `provision`, which provisions the state directory -again. A `provision` that takes the lock after `deprovision` has removed the -lock file, but before it has removed the directory, provisions the directory -first, and `deprovision` then fails because the sandbox was provisioned again. +sandboxes share through `nvx-rs`. On NFS, where a removed file that is still +open, such as a lock file, stays as `.nfs*` until its last handle closes, and +on Windows file systems without POSIX deletion such as ReFS, where a removed +lock file stays under its name, such files keep the emptied directory, so +`deprovision` then releases the lock before it removes the directory. A +transition that waited while `deprovision` removed the sandbox then fails as +for a sandbox that is not provisioned, except `provision`, which provisions the +state directory again. A `provision` that takes the lock after `deprovision` +has removed the lock file, but before it has removed the directory, provisions +the directory first, and `deprovision` then fails because the sandbox was +provisioned again. `--timeout` must be finite and greater than 0. If the guest exits while `start` waits for it to become ready, for example because it refuses the workload identity, OpenVMM closes the control endpoint and shuts down. diff --git a/nvx-rs/README.md b/nvx-rs/README.md index 9caff6ba7..f3c7ed9ae 100644 --- a/nvx-rs/README.md +++ b/nvx-rs/README.md @@ -74,7 +74,7 @@ nvx-rs/ | `nvx::host::network` | `NetworkPolicy` and `openvmm_arguments`: the egress default and rules, ingress, host-loopback access and forwards as OpenVMM options. Egress rules are lowered exactly into OpenVMM's IPv4 and IPv6 rules by the lowering of egress policy files, with at most 256 rules for each action | | `nvx::host::network::policy` | Egress policy files: `compile_policy_file`, `compile_policy_bytes`, and `compile_policy` compile one to a `CompiledEgressPolicy` of native rules; see [Egress policy files](#egress-policy-files) | | `nvx::host::filesystem` | The `ExportPlan` of host paths through OpenVMM's aggregate virtio-fs device, the host-mapping table, the reserved guest targets, and `guest_path` and `resolve_guest_path` (also at `nvx::`) | -| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in, for a caller that declares its configuration record: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for lock files of other transitions that keep the emptied directory; both remove the directory only while the path still names it. The store also owns the file in which the VMM stages its outcome report, which a VMM that is killed while it writes the report leaves, and removes it once no process of its launch runs | +| `nvx::host::store` | `SandboxStore`: the runtime records of one sandbox in a private directory with one lock, which every host that supervises the sandbox shares. The `RuntimeRecord` names the running VM: the `ProcessRef` of its VMM, its control endpoint, and the `RuntimeIdentity` that it was launched with, the identities of its OpenVMM, kernel, and initramfs from install or staging metadata and its boot and control contract versions, which decide whether another build of the host may reattach to it. The `LaunchJournal` of a launch holds a marker written before the spawn, the VMM's `ProcessRef` right after it, and an inherited claim on the VMM's log, which proves that a launch that never published its endpoint has exited, so that recovery ends an interrupted launch and signals only a VMM that it holds through a verified `Process`. Records are JSON in one versioned format, and records in any other format fail rather than being migrated. The caller declares the files that it keeps in the directory, its configuration record, whose presence means that the sandbox is provisioned, and any others, as `CallerFiles`, and takes the lifecycle locks with them: `SandboxStore::lock_for_provision` creates the directory, again if a removal deletes it meanwhile, `SandboxStore::lock_existing` fails with `ErrorKind::NotFound` once it is gone, and a holder that leaves the directory without the configuration record removes its lock file as it releases the lock. `LockedStore::remove` removes a sandbox that no VMM can run in, for a caller that declares its configuration record: the store's files, the caller's, and the directory, unless it holds entries that are neither, which it refuses and keeps, and `Lingering` waits for open files, such as the lock files of other transitions, that keep the emptied directory; both remove the directory only while the path still names it. The store also owns the file in which the VMM stages its outcome report, which a VMM that is killed while it writes the report leaves, and removes it once no process of its launch runs | | `nvx::host::instance` | `Instance`: a VM in a sandbox store of its own, which `nvx::host::store`, `nvx::host::supervisor`, and `nvx::host::session` drive, and in which the caller keeps the files that it declares as `CallerFiles`. Under the store's lifecycle lock of an existing sandbox, `LockedInstance::find` reattaches to the VM, ends a start that was interrupted, and reports a VM of another runtime identity as `Mismatched` rather than driving it; `LockedInstance::start` launches OpenVMM through the supervisor with a fresh capability, under the caller's `RuntimeIdentity`, waits until the guest is ready and has mounted the mapped paths, and commits the launch; `LockedInstance::stop` stops the VM with a forced fallback; and `LockedInstance::remove` removes a stopped instance, with the caller's files and its directory, through the store. `Instance::exec` runs workloads without the lock and pumps their output. `write_json` and `read_json` keep the configuration records that callers store beside an instance | | `nvx::host::nvxs` | `HostConnection`: the host's role of the NVXS session over a platform transport, on which `nvx::host::session` and other host applications run their protocols. It attaches with the launch capability, waiting while the broker asks it to, sends application bytes in as many DATA records as they take, and returns each DATA payload from the guest, with deadlines; a receive that times out keeps a partial record | | `nvx::host::process` | Verified process references. A `ProcessRef` names one process by its ID, its start time, and the `BootId` of the host boot during which it started. `Process::open` checks a reference and holds its process through a pidfd on Linux or a process handle on Windows, whether or not the caller started it, so that waits sleep until the process exits and termination never reaches a process that reuses its ID. A reference from an earlier boot names an exited process, and a process whose identity this user cannot check, such as another user's, makes `Process::open` fail rather than report an exit. Linux hosts without pidfds poll the reference instead, and cannot terminate the process | diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle.rs b/nvx-rs/src/host/cli/sandbox/lifecycle.rs index 3b9463e21..7a490f958 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle.rs @@ -71,7 +71,7 @@ use crate::protocol::{CAPABILITY_LEN, GuestFeatures}; /// The launch configuration, the CLI's own record beside those of the store. pub(crate) const CONFIG_NAME: &str = "config.json"; /// How often a caller retries a lifecycle lock that another caller holds, and the removal of a -/// state directory that a removed lock file still keeps. +/// state directory that open files still keep. pub(crate) const LOCK_RETRY_INTERVAL: Duration = RETRY_INTERVAL; /// The version of outcome reports. pub(crate) const OUTCOME_SCHEMA_VERSION: u64 = 1; diff --git a/nvx-rs/src/host/cli/sandbox/lock.rs b/nvx-rs/src/host/cli/sandbox/lock.rs index b52b3419d..37c60aa6f 100644 --- a/nvx-rs/src/host/cli/sandbox/lock.rs +++ b/nvx-rs/src/host/cli/sandbox/lock.rs @@ -152,8 +152,8 @@ impl LifecycleLock { } /// Removes the sandbox and its state directory through the store, as deprovision does once no - /// OpenVMM of the sandbox can run, waiting up to `timeout` seconds for lock files of other - /// transitions that keep the emptied directory, and releases the lock. + /// OpenVMM of the sandbox can run, waiting up to `timeout` seconds for open files, such as the + /// lock files of other transitions, that keep the emptied directory, and releases the lock. pub(crate) fn remove(self, timeout: f64) -> Result { let Self { state_dir, locked } = self; let dir = locked.dir().to_owned(); @@ -161,11 +161,11 @@ impl LifecycleLock { let Some(mut lingering) = locked.remove().map_err(failed)? else { return Ok(()); }; - // A removed lock file that a handle still keeps stays in the directory, as do the lock - // files of transitions that wait meanwhile. Each of them finds no sandbox, and removes - // its lock file as it releases the lock, so the directory empties after the removal - // released its own. The removal is tried once at a time, so that this wait checks for - // Ctrl-C between tries. + // A removed file that a handle still keeps, such as a lock file, stays in the directory, + // as do the lock files of transitions that wait meanwhile. Each such transition finds no + // sandbox, and removes its lock file as it releases the lock, so the directory empties + // after the removal released its own, once the other handles close. The removal is tried + // once at a time, so that this wait checks for Ctrl-C between tries. let deadline = Deadline::after(timeout); loop { match lingering.wait(Some(Instant::now())) { diff --git a/nvx-rs/src/host/instance.rs b/nvx-rs/src/host/instance.rs index 9c3c990f4..41b3bb9dc 100644 --- a/nvx-rs/src/host/instance.rs +++ b/nvx-rs/src/host/instance.rs @@ -718,9 +718,10 @@ impl LockedInstance { /// Removes the stopped instance: its store's records and files, the caller's files that it /// declared, the configuration record last, and its directory, through the store's - /// [`LockedStore::remove`], and releases the lock. Lock files of other transitions that keep - /// the emptied directory are then waited for until `deadline`, or for as long as it takes - /// without one, as [`Lingering::wait`](super::store::Lingering::wait) does. + /// [`LockedStore::remove`], and releases the lock. Open files, such as the lock files of other + /// transitions, that keep the emptied directory are then waited for until `deadline`, or for + /// as long as it takes without one, as [`Lingering::wait`](super::store::Lingering::wait) + /// does. /// /// The caller has found the instance stopped with [`find`](Self::find): the removal refuses, /// and removes nothing, while the store records a VM or a start, and for an instance whose diff --git a/nvx-rs/src/host/store/caller.rs b/nvx-rs/src/host/store/caller.rs index de5fc6f29..6c2dcadf3 100644 --- a/nvx-rs/src/host/store/caller.rs +++ b/nvx-rs/src/host/store/caller.rs @@ -26,12 +26,18 @@ use crate::error::{Error, Result}; /// `"`, `*`, `:`, `<`, `>`, `?`, or `|`, no trailing dot or space, which Windows drops, and it is /// not a name that Windows reserves for a device, such as `NUL` or `com1.json`, so it names the /// same file on every host. +/// - It has only the portable file name characters, ASCII letters and digits, `.`, `_`, and `-`, +/// so that no file system takes it for another name: file systems ignore the case of other +/// letters by rules of their own, macOS's take a composed letter for the same letter followed +/// by a combining mark, and Windows also names files by short names, such as `RUNTIM~1.JSO` for +/// `runtime.json`. /// - It is none of the store's [`FILE_NAMES`], the lock included, has neither form of a temporary -/// file that the store owns, and does not have the form of a lock file that a removal waits +/// file that the store owns, and does not have the form of a removed file that a removal waits /// for, `.nfs*`. /// - It is declared once: the configuration record's name differs from the others. /// -/// Names are compared without regard to case, as Windows and macOS file systems compare them. +/// Names are compared without regard to ASCII case, as Windows and macOS file systems compare +/// them. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct CallerFiles { /// The configuration record, whose presence means that the sandbox is provisioned. @@ -102,18 +108,19 @@ impl CallerFiles { } /// Returns why `name` cannot be declared beside the store's files, its temporary files, and - /// the names declared so far, comparing names without regard to case. + /// the names declared so far, comparing names, which have only ASCII characters, without + /// regard to case. fn collision(&self, name: &str) -> Option<&'static str> { - let folded = name.to_lowercase(); + let folded = name.to_ascii_lowercase(); if FILE_NAMES.contains(&folded.as_str()) { Some("is the name of one of the store's files") } else if is_store_temporary(&folded) || is_report_staging(&folded) { Some("has the form of a temporary file that the store owns") } else if folded.starts_with(".nfs") { - Some("has the form of a removed lock file that a removal waits for") + Some("has the form of a removed file that a removal waits for") } else if self .names() - .any(|declared| declared.to_lowercase() == folded) + .any(|declared| declared.eq_ignore_ascii_case(name)) { Some("is declared already") } else { @@ -140,6 +147,11 @@ fn name_problem(name: &str) -> Option<&'static str> { Some("ends with a dot or a space, which Windows drops") } else if reserves_device(name) { Some("is a name that Windows reserves for a device") + } else if !name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-')) + { + Some("has a character other than ASCII letters and digits, `.`, `_`, and `-`") } else { None } @@ -288,12 +300,35 @@ mod tests { "console.json", "nul-file", "lpt", - "a b.json", + "a_b.json", ] { CallerFiles::new(name).unwrap(); } } + #[test] + fn names_that_a_file_system_could_take_for_others_are_refused() { + let problem = "has a character other than ASCII letters and digits, `.`, `_`, and `-`"; + for name in [ + // The uppercase forms of `ı` and `ſ` are `I` and `S`, which `to_lowercase` does not + // tell, so a file system that compares the uppercase forms of names could take these + // for the store's files. + "runt\u{131}me.json", + "control.\u{17f}ock", + // macOS's file systems take a composed letter for the same letter followed by a + // combining mark. + "caf\u{e9}.json", + "cafe\u{301}.json", + // Windows names `runtime.json` by this short name too on volumes with short names. + "RUNTIM~1.JSO", + "a b.json", + ] { + let expected = format!("the caller's file name {name:?} {problem}"); + assert_eq!(refusal(name, &[]), expected); + assert_eq!(refusal("config.json", &[name]), expected); + } + } + #[test] fn names_that_collide_are_refused_whatever_their_case() { for (name, problem) in [ @@ -318,11 +353,11 @@ mod tests { ), ( ".nfs000000001", - "has the form of a removed lock file that a removal waits for", + "has the form of a removed file that a removal waits for", ), ( ".NFS1", - "has the form of a removed lock file that a removal waits for", + "has the form of a removed file that a removal waits for", ), ] { let expected = format!("the caller's file name {name:?} {problem}"); diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index a30ca2d4d..c543bcf9e 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -91,9 +91,9 @@ impl SandboxStore { /// locks nothing, so the lock of the lock file at the path is taken again, as /// [`lock`](Self::lock) does, which fails once the directory is gone. A directory that exists /// is locked even if it holds no provisioned sandbox, as one that was never provisioned, or - /// that an interrupted removal emptied, or whose removal lingering lock files still keep: the - /// caller checks its configuration record under the lock, and the release then removes the - /// lock file again. + /// that an interrupted removal emptied, or whose removal open files still keep: the caller + /// checks its configuration record under the lock, and the release then removes the lock file + /// again. /// /// On Windows, a file system without POSIX deletion keeps a removed lock file under its name /// until its last handle closes, and refuses to open it until then, so the open is tried @@ -196,24 +196,25 @@ impl LockedStore { /// transition that waited for the lock finds the sandbox gone rather than halfway removed, and /// then the directory, as long as the path still names it. /// - /// Lock files of other transitions can keep the emptied directory: on NFS, a removed lock file - /// that a handle still holds is renamed to `.nfs*`; a Windows file system without POSIX - /// deletion keeps one under its own name until its last handle closes; and a transition that - /// opens the lock file between the removals of the lock file and of the directory creates a - /// new one. Each of those transitions finds no sandbox, and removes its lock file as it - /// releases its lock, so the directory empties. The removal then holds the directory's - /// identity, releases the lock, and returns the [`Lingering`] removal, whose + /// Lock files of other transitions, and on NFS other removed files that handles still hold, + /// can keep the emptied directory, and are not refused: NFS renames a removed file that is + /// still open to `.nfs*`; a Windows file system without POSIX deletion keeps a removed lock + /// file under its own name until its last handle closes; and a transition that opens the lock + /// file between the removals of the lock file and of the directory creates a new one. Each of + /// those transitions finds no sandbox, and removes its lock file as it releases its lock, and + /// the handles of other removed files close, so the directory empties. The removal then holds + /// the directory's identity, releases the lock, and returns the [`Lingering`] removal, whose /// [`wait`](Lingering::wait) removes the directory once it empties. It returns `None` once the - /// directory is gone. + /// directory is gone, whoever removed it. pub fn remove(self) -> std::result::Result, RemovalError> { self.remove_contents()?; self.remove_directory() } - /// Empties the directory but for the lock file, as [`remove`](Self::remove) does first: it - /// refuses without the caller's configuration record and while the store records a VM or a - /// launch, removes the store's files and the caller's, and the temporary files that the store - /// owns, and refuses any other entry. + /// Empties the directory but for lingering files, its own lock file included, as + /// [`remove`](Self::remove) does first: it refuses without the caller's configuration record + /// and while the store records a VM or a launch, removes the store's files and the caller's, + /// and the temporary files that the store owns, and refuses any other entry. fn remove_contents(&self) -> std::result::Result<(), RemovalError> { if self.files.config().is_none() { return Err(RemovalError::Undeclared); @@ -241,7 +242,9 @@ impl LockedStore { })?; let mut foreign = Vec::new(); for name in listed { - if name == LOCK_NAME { + // Lock files, and removed files that NFS keeps while handles hold them, go once their + // holders release them, which the removal waits for, if they keep the directory. + if lingers(&name) { continue; } if is_store_temporary(&name) || is_report_staging(&name) { @@ -259,7 +262,17 @@ impl LockedStore { /// Removes the lock file while it still holds the lock, and then the directory, as /// [`remove`](Self::remove) does once the directory holds nothing else of the sandbox. - fn remove_directory(mut self) -> std::result::Result, RemovalError> { + fn remove_directory(self) -> std::result::Result, RemovalError> { + self.remove_directory_after(|| {}) + } + + /// Removes the lock file and the directory as [`remove_directory`](Self::remove_directory) + /// does, and runs `unlinked` between the two, where the transitions that the removal races + /// can act. + fn remove_directory_after( + mut self, + unlinked: impl FnOnce(), + ) -> std::result::Result, RemovalError> { let dir = self.dir().to_owned(); // While the lock is held, its file keeps the directory, which the path then names. let (identity, held) = hold_directory(&dir).map_err(|error| RemovalError::Io { @@ -269,10 +282,12 @@ impl LockedStore { let lock = self.path(LOCK_NAME); fs::remove_file(&lock).map_err(|error| RemovalError::Io { path: lock, error })?; self.lock_removed = true; + unlinked(); match remove_held_directory(&dir, identity) { // The directory is gone: this removal removed it, or another one did once it emptied, - // after which the path may name another directory. + // after which the path may name another directory, or none. Ok(_) => return Ok(None), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), Err(error) if directory_not_empty(&error) => {} Err(error) => return Err(RemovalError::Io { path: dir, error }), } @@ -288,8 +303,9 @@ impl LockedStore { } } -/// The removal of a sandbox's directory that lock files of other transitions keep, which -/// [`LockedStore::remove`] returns once it has removed everything else and released the lock. +/// The removal of a sandbox's directory that open files, such as the lock files of other +/// transitions, keep, which [`LockedStore::remove`] returns once it has removed everything else +/// and released the lock. /// /// It holds the identity of the directory that it removes: on Unix, its open handle keeps a /// directory that replaces it from reusing its inode number, and NTFS and ReFS do not reuse file @@ -316,18 +332,17 @@ impl Lingering { &self.dir } - /// Removes the directory once lingering lock files no longer keep it, trying again every 25 - /// ms until `deadline`, or for as long as it takes without one. A deadline that has passed - /// still tries once, so a caller that waits on its own, as for an interruption, calls this - /// again, and a removal that timed out can go on. + /// Removes the directory once open files no longer keep it, trying again every 25 ms until + /// `deadline`, or for as long as it takes without one. A deadline that has passed still tries + /// once, so a caller that waits on its own, as for an interruption, calls this again, and a + /// removal that timed out can go on. /// /// Returns once the directory is gone, whoever removed it, or once the path names another /// directory, which it checks before each attempt to remove it. Fails, and keeps the - /// directory, with [`RemovalError::ProvisionedAgain`] once - /// the caller's configuration record appears in it, as a provision that took the lock after - /// the removal released it writes it, with [`RemovalError::Foreign`] for other entries that - /// are not lingering lock files, sorted, and with [`RemovalError::TimedOut`] once the deadline - /// passes. + /// directory, with [`RemovalError::ProvisionedAgain`] once the caller's configuration record + /// appears in it, as a provision that took the lock after the removal released it writes it, + /// with [`RemovalError::Foreign`] for other entries than lock files and the removed files that + /// NFS keeps, sorted, and with [`RemovalError::TimedOut`] once the deadline passes. pub fn wait(&mut self, deadline: Option) -> std::result::Result<(), RemovalError> { loop { let error = match remove_held_directory(&self.dir, self.identity) { @@ -372,10 +387,9 @@ impl Lingering { error, }); } - // Without remaining entries, the last lingering lock file went away after the removal - // failed, so the next attempt follows at once. Another such failure right after it - // pauses first, so that a file system that does not report emptiness right away - // cannot spin. + // Without remaining entries, the last open file went away after the removal failed, + // so the next attempt follows at once. Another such failure right after it pauses + // first, so that a file system that does not report emptiness right away cannot spin. if remaining.is_empty() && !self.retried_at_once { self.retried_at_once = true; continue; @@ -400,13 +414,13 @@ pub enum RemovalError { Launched, /// The directory holds entries that are neither the store's nor its caller's, which the /// removal keeps, with the directory: their names, in the order in which the directory lists - /// them, or sorted once the removal waited for lingering lock files. + /// them, or sorted once the removal waited for open files. Foreign(Vec), - /// The sandbox was provisioned again while the removal waited for lingering lock files: the - /// caller's configuration record appeared in the directory, which the removal keeps. + /// The sandbox was provisioned again while the removal waited for open files: the caller's + /// configuration record appeared in the directory, which the removal keeps. ProvisionedAgain, - /// Lingering lock files still kept the directory when the deadline passed: the error with - /// which the latest attempt to remove it failed. + /// Open files still kept the directory when the deadline passed: the error with which the + /// latest attempt to remove it failed. TimedOut(io::Error), /// An operation on one of the sandbox's files, or on its directory, failed. Io { @@ -435,8 +449,9 @@ impl fmt::Display for RemovalError { ), Self::ProvisionedAgain => formatter .write_str("the sandbox was provisioned again while its directory was removed"), - Self::TimedOut(_) => formatter - .write_str("lock files of other transitions still keep the sandbox's directory"), + Self::TimedOut(_) => { + formatter.write_str("files that are still open keep the sandbox's directory") + } Self::Io { path, .. } => write!(formatter, "cannot remove {}", path.display()), } } @@ -476,10 +491,10 @@ fn remove_entry(path: PathBuf) -> std::result::Result<(), RemovalError> { } } -/// Returns whether `name` is a removed lock file that a handle still keeps: NFS renames a removed -/// file that is still open to `.nfs*`, and Windows file systems without POSIX deletion keep it -/// under its own name, as they keep the lock files that waiting transitions create until they -/// release them. +/// Returns whether `name` is a lock file, or a removed file that a handle still keeps, which goes +/// once its holder releases or closes it: NFS renames a removed file that is still open to +/// `.nfs*`, and Windows file systems without POSIX deletion keep a removed lock file under its +/// own name, as they keep the lock files that waiting transitions create until they release them. fn lingers(name: &str) -> bool { name == LOCK_NAME || name.starts_with(".nfs") } diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index c911ac230..d2f1e0ce4 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -68,14 +68,11 @@ fn remove(dir: &Path) -> std::result::Result, RemovalError> { .remove() } -/// Removes the sandbox in `dir` as [`LockedStore::remove`] does, while the lock file of another -/// transition, which NFS renamed as the transition still held it, keeps the emptied directory. +/// Removes the sandbox in `dir` as [`LockedStore::remove`] does while the lock file of another +/// transition, which NFS renamed as the transition still held it, keeps the directory. fn remove_lingering(dir: &Path) -> Lingering { - let locked = SandboxStore::lock_existing(dir, &files(), None).unwrap(); - locked.remove_contents().unwrap(); fs::write(dir.join(RENAMED), b"").unwrap(); - locked - .remove_directory() + remove(dir) .unwrap() .expect("a lingering lock file keeps the directory") } @@ -355,6 +352,9 @@ fn a_removal_refuses_foreign_entries_and_keeps_them() { fs::write(dir.join(name), b"kept").unwrap(); } fs::create_dir(dir.join("directory")).unwrap(); + // A removed file that NFS keeps while a handle holds it is not foreign: it goes as the + // handle closes. + fs::write(dir.join(RENAMED), b"").unwrap(); let RemovalError::Foreign(mut names) = remove(&dir).unwrap_err() else { panic!("foreign entries were not refused"); }; @@ -362,7 +362,9 @@ fn a_removal_refuses_foreign_entries_and_keeps_them() { assert_eq!(names, sorted(&foreign)); // The store's files and the caller's went first, and the lock file as the lock was released, // so the sandbox is gone and the foreign entries remain, in the directory. - assert_eq!(entries_of(&dir), sorted(&foreign)); + let mut remaining = foreign.to_vec(); + remaining.push(RENAMED); + assert_eq!(entries_of(&dir), sorted(&remaining)); assert_eq!(fs::read(dir.join("notes.txt")).unwrap(), b"kept"); let error = Error::from(RemovalError::Foreign(vec!["a".to_owned(), "b".to_owned()])); assert_eq!(error.kind(), ErrorKind::Other); @@ -375,6 +377,7 @@ fn a_removal_refuses_foreign_entries_and_keeps_them() { fs::remove_file(dir.join(name)).unwrap(); } fs::remove_dir(dir.join("directory")).unwrap(); + fs::remove_file(dir.join(RENAMED)).unwrap(); assert!(remove(&dir).unwrap().is_none()); assert!(!dir.exists()); } @@ -459,6 +462,36 @@ fn a_lingering_removal_reports_what_appears_in_the_directory() { assert_eq!(entries_of(&dir), [RENAMED]); } +#[test] +fn a_removal_that_another_finishes_succeeds_and_keeps_what_replaced_the_directory() { + // Another removal, which takes the lock of a new lock file once this one has removed its own, + // finds the emptied directory, and removes it first. + let removed_by_another = |dir: &Path| { + let other = SandboxStore::lock_existing(dir, &files(), within(10)).unwrap(); + assert!(other.remove().unwrap().is_none()); + }; + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + locked.remove_contents().unwrap(); + let lingering = locked + .remove_directory_after(|| removed_by_another(&dir)) + .unwrap(); + assert!(lingering.is_none()); + assert!(!dir.exists()); + // A directory that replaces it then stays, even an empty one. + let (_directory, dir) = sandbox(); + let locked = SandboxStore::lock_existing(&dir, &files(), None).unwrap(); + locked.remove_contents().unwrap(); + let lingering = locked + .remove_directory_after(|| { + removed_by_another(&dir); + fs::create_dir(&dir).unwrap(); + }) + .unwrap(); + assert!(lingering.is_none()); + assert!(entries_of(&dir).is_empty()); +} + #[test] fn a_lingering_removal_keeps_an_empty_directory_that_replaced_its_own() { let (_directory, dir) = sandbox(); diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index 73a08ba61..0ffe2d3aa 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -175,7 +175,7 @@ //! - [`SandboxStore::lock_existing`] fails with [`ErrorKind::NotFound`] once the directory is gone, //! also when a concurrent removal deletes it while the lock is awaited. The caller checks its //! configuration record under the lock: a directory that was never provisioned, that an -//! interrupted removal emptied, or whose removal lingering lock files still keep, holds none. +//! interrupted removal emptied, or whose removal open files still keep, holds none. //! - A holder that leaves the directory without the configuration record removes its own lock //! file as it releases the lock, unless it removed it already, so that such a directory keeps //! no lock file. @@ -196,14 +196,15 @@ //! removal needs the caller's configuration record, whose absence tells the transitions that wait //! through it that the sandbox is gone: a lock taken without one refuses it. //! -//! Lock files that other transitions hold can keep the emptied directory for a while: NFS renames -//! a removed file that is still open to `.nfs*`, Windows file systems without POSIX deletion keep -//! it under its name until its last handle closes, and a transition that opens the lock file -//! after the removal removed it creates a new one. Each such transition finds no sandbox, and -//! removes its lock file as it releases its lock. The removal then holds the directory's identity, -//! releases its lock, and returns a [`Lingering`] removal, which waits until they go, and reports -//! a provision that took the lock meanwhile as [`RemovalError::ProvisionedAgain`]. A removal -//! removes the directory only while the path still names it, which it checks before each attempt. +//! Files that are still open can keep the emptied directory for a while: NFS renames a removed +//! file that is still open, such as the lock file of a waiting transition, to `.nfs*`, Windows +//! file systems without POSIX deletion keep a removed lock file under its name until its last +//! handle closes, and a transition that opens the lock file after the removal removed it creates +//! a new one. Each such transition finds no sandbox, and removes its lock file as it releases its +//! lock. The removal leaves those files, then holds the directory's identity, releases its lock, +//! and returns a [`Lingering`] removal, which waits until they go, and reports a provision that +//! took the lock meanwhile as [`RemovalError::ProvisionedAgain`]. A removal removes the directory +//! only while the path still names it, which it checks before each attempt. mod caller; mod files; From 1b42703281d1939b486942a4939278260d618ed7 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 03:21:52 -0700 Subject: [PATCH 07/14] nvx-rs: stop waiting for a lock file that Windows refuses for good On Windows, a lifecycle lock retried every denied open of the lock file, which a file system without POSIX deletion gives for a removed lock file that other handles still hold. A lock file that refuses access for good, such as a read-only one or a directory, gives the same error, so a lock without a deadline waited for it forever. Windows refuses every open of a removed file, also one without access rights, which opens the others, so the lock now probes the path that way. It waits for a removed lock file, and fails with the denied access once a second open at once is refused too, as one that went and was replaced meanwhile opens then. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/lifecycle.rs | 28 +++++++++++++- nvx-rs/src/host/store/lifecycle/tests.rs | 47 ++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index c543bcf9e..6456c43bb 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -98,7 +98,9 @@ impl SandboxStore { /// On Windows, a file system without POSIX deletion keeps a removed lock file under its name /// until its last handle closes, and refuses to open it until then, so the open is tried /// again every 25 ms. Fails with [`ErrorKind::TimedOut`] when the deadline passes first, as - /// [`lock`](Self::lock) does. + /// [`lock`](Self::lock) does. A lock file that Windows refuses to open for good, such as a + /// read-only one, still opens without access rights, unlike a removed one, and fails the lock + /// with the denied access. pub fn lock_existing( dir: impl Into, files: &CallerFiles, @@ -114,11 +116,25 @@ impl SandboxStore { /// Takes the lock as [`take_lock`](Self::take_lock) does, and opens a lock file that Windows /// refuses to open again every 25 ms until `deadline`, as a removed one that other handles - /// still hold, which a file system without POSIX deletion keeps until they close. + /// still hold, which a file system without POSIX deletion keeps until they close. A lock file + /// that Windows refuses to open although it opens without access rights is not a removed one, + /// and fails the lock once a second open at once is refused too. fn take_lifecycle_lock(&self, deadline: Option) -> Result { + // Whether the latest attempt was refused a lock file that opened without access rights. + let mut refused_for_good = false; loop { match self.take_lock(deadline) { Err(error) if cfg!(windows) && denied(&error) => { + if !pending_deletion(&self.path(LOCK_NAME)) { + // The lock file refuses access for good, such as a read-only one, unless + // the removed one went, and another replaced it, after the open failed. + if refused_for_good { + return Err(error); + } + refused_for_good = true; + continue; + } + refused_for_good = false; if !pause(deadline) { return Err(transition_in_progress(self.dir())); } @@ -137,6 +153,14 @@ fn denied(error: &Error) -> bool { .is_some_and(|source| source.kind() == io::ErrorKind::PermissionDenied) } +/// Returns whether the file at `path`, which Windows refused to open, is a removed one that other +/// handles still hold: Windows refuses every open of such a file, also one without access rights, +/// which opens a file whose access it refuses otherwise. +fn pending_deletion(path: &Path) -> bool { + platform::file_identity(path) + .is_err_and(|error| error.kind() == io::ErrorKind::PermissionDenied) +} + /// Waits before the next try of a step that `deadline` bounds: returns `false` at once if the /// deadline has passed, and otherwise sleeps for [`LOCK_RETRY_INTERVAL`] or the time left, /// whichever is shorter, and returns `true`. diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index d2f1e0ce4..a3b0beb7e 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -826,6 +826,53 @@ mod windows { assert!(entries_of(&dir).is_empty()); } + #[test] + fn locks_refuse_a_lock_file_that_refuses_access_for_good() { + // A read-only lock file, and a directory at its path, refuse an open for writing as a + // removed lock file does, but they open without access rights, so the locks fail at once, + // also without a deadline. + for read_only in [true, false] { + let directory = tempfile::tempdir().unwrap(); + let dir = directory.path().join("sandbox"); + SandboxStore::create(&dir).unwrap(); + let lock = dir.join(LOCK_NAME); + if read_only { + fs::write(&lock, b"").unwrap(); + set_read_only(&lock, true); + } else { + fs::create_dir(&lock).unwrap(); + } + let (sender, receiver) = mpsc::channel(); + thread::spawn({ + let dir = dir.clone(); + move || { + let provision = SandboxStore::lock_for_provision(&dir, &files(), None); + let existing = SandboxStore::lock_existing(&dir, &files(), None); + sender + .send([provision.map(drop), existing.map(drop)]) + .unwrap(); + } + }); + let taken = receiver + .recv_timeout(Duration::from_secs(10)) + .expect("a lock waited for a lock file that refuses access for good"); + for result in taken { + let error = result.unwrap_err(); + assert!(denied(&error), "{error}"); + } + if read_only { + set_read_only(&lock, false); + } + } + } + + /// Sets or clears the read-only attribute of the file `path`. + fn set_read_only(path: &Path, read_only: bool) { + let mut permissions = fs::metadata(path).unwrap().permissions(); + permissions.set_readonly(read_only); + fs::set_permissions(path, permissions).unwrap(); + } + #[test] fn a_transition_whose_lock_file_lingers_waits_until_it_goes() { let (_directory, dir) = sandbox(); From b875be5249957bb0dc5a3dfb67ea3ef64dbc28e9 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 03:53:08 -0700 Subject: [PATCH 08/14] nvx-rs: report a store directory that went as not found The removal of the files in which a VMM staged its outcome report failed with an error of kind `Io` when it could not list the sandbox's directory. A directory that is gone now fails as `NotFound`, as `SandboxStore::open` reports it. The validation's fake CLI ignores a staging file that is gone by the time that it removes it, as the store does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/mod.rs | 5 +++-- nvx-rs/src/host/store/tests.rs | 10 ++++++++++ scripts/test_microvm_tests.py | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index 0ffe2d3aa..81290a68f 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -520,10 +520,11 @@ impl SandboxStore { /// Removes the files in which a VMM of the sandbox staged an outcome report that it never /// published, which only a VMM that was killed while it wrote the report leaves. The caller - /// knows that no process of the sandbox's launches runs. + /// knows that no process of the sandbox's launches runs. Fails with [`ErrorKind::NotFound`] + /// if the directory is gone, as [`open`](Self::open) does. fn remove_report_staging(&self) -> Result<()> { let failed = |error| { - io_error(format!( + store_error(format!( "cannot list the sandbox store {}", self.dir.display() ))(error) diff --git a/nvx-rs/src/host/store/tests.rs b/nvx-rs/src/host/store/tests.rs index 71761d548..0ca806366 100644 --- a/nvx-rs/src/host/store/tests.rs +++ b/nvx-rs/src/host/store/tests.rs @@ -870,6 +870,16 @@ fn report_staging_files_go_once_nothing_of_their_launch_runs() { } } +#[test] +fn a_store_whose_directory_went_reports_it_missing_as_it_clears_a_runtime() { + let (_directory, store) = store(); + let mut locked = store.lock(None).unwrap(); + // Something other than a removal deletes the directory while the lock is held. + fs::remove_dir_all(store.dir()).unwrap(); + let error = locked.clear_runtime().unwrap_err(); + assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); +} + #[test] fn launch_identities_are_fresh_hexadecimal() { let first = LaunchId::generate().unwrap(); diff --git a/scripts/test_microvm_tests.py b/scripts/test_microvm_tests.py index 5713cfffe..a600162a4 100644 --- a/scripts/test_microvm_tests.py +++ b/scripts/test_microvm_tests.py @@ -1264,7 +1264,7 @@ def _deprovision( # The store owns the file in which OpenVMM stages its outcome report. for path in state.iterdir(): if re.fullmatch(r"\.tmp[A-Za-z0-9]{6}", path.name): - path.unlink() + path.unlink(missing_ok=True) unknown = sorted(path.name for path in state.iterdir()) if unknown and "deprovision-removes-foreign" not in self.faults: return self._error( From f369049441a31f4c99a7d2e0232fee43c5ae3167 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 04:24:19 -0700 Subject: [PATCH 09/14] nvx-rs: keep a lock's deadline while its lock file is replaced A lock whose lock file another transition removed or replaced after the lock opened it took the lock again at once, every time. Since transitions remove their lock files as they release their locks on directories without a sandbox, transitions that kept doing so could keep a waiter past its deadline, and the CLI from checking for its timeout and for Ctrl-C. The locks now take the lock again at once the first time, and after a pause, until the deadline, when that happens again right after it, as a provision already did for a directory that keeps disappearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/lifecycle.rs | 76 ++++++++++++++---------- nvx-rs/src/host/store/lifecycle/tests.rs | 74 +++++++++++++++++++---- nvx-rs/src/host/store/mod.rs | 51 ++++++++++++++-- 3 files changed, 152 insertions(+), 49 deletions(-) diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index 6456c43bb..7b45d14f2 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -7,13 +7,12 @@ use std::fmt; use std::fs::{self, File}; use std::io; use std::path::{Path, PathBuf}; -use std::thread; use std::time::Instant; use super::caller::{is_report_staging, is_store_temporary}; use super::{ - CallerFiles, FILE_NAMES, JOURNAL_NAME, LOCK_NAME, LOCK_RETRY_INTERVAL, LockedStore, - RUNTIME_NAME, SandboxStore, Taken, absolute, transition_in_progress, + CallerFiles, FILE_NAMES, JOURNAL_NAME, LOCK_NAME, LockedStore, RUNTIME_NAME, Races, + SandboxStore, Taken, absolute, pause, transition_in_progress, }; use crate::error::{Error, ErrorKind, Result}; use crate::host::platform; @@ -34,52 +33,63 @@ impl SandboxStore { /// /// Fails with [`ErrorKind::TimedOut`] when the deadline passes first, also while Windows /// refuses to open a removed lock file, as [`lock_existing`](Self::lock_existing) describes, - /// or while concurrent removals keep deleting the directory, with [`ErrorKind::NotFound`] if - /// the parent does not exist, and as [`create`](Self::create) fails for a directory that is not - /// private. + /// or while concurrent transitions keep deleting the directory or replacing the lock file, + /// with [`ErrorKind::NotFound`] if the parent does not exist, and as [`create`](Self::create) + /// fails for a directory that is not private. pub fn lock_for_provision( dir: impl Into, files: &CallerFiles, deadline: Option, ) -> Result { let dir = absolute(dir.into())?; - Self::provision_lock(&dir, files, deadline, |dir| Self::create(dir)) + Self::provision_lock( + &dir, + files, + deadline, + |dir| Self::create(dir), + Self::take_lifecycle_lock, + ) } /// Takes the lock as [`lock_for_provision`](Self::lock_for_provision) does, with `create` - /// creating or checking the absolute directory `dir`, as [`create`](Self::create) does. + /// creating or checking the absolute directory `dir`, as [`create`](Self::create) does, and + /// `take` taking the lock of its store, as [`take_lifecycle_lock`](Self::take_lifecycle_lock) + /// does. fn provision_lock( dir: &Path, files: &CallerFiles, deadline: Option, mut create: impl FnMut(&Path) -> Result, + mut take: impl FnMut(&Self, Option) -> Result, ) -> Result { - // Whether the latest attempt followed at once one that found the directory gone. - let mut retried_at_once = false; + let mut races = Races::default(); loop { let error = match create(dir) { - Ok(store) => match store.take_lifecycle_lock(deadline) { + Ok(store) => match take(&store, deadline) { Ok(Taken::Locked(lock)) => { return Ok(LockedStore::new(store, lock, files.clone())); } // A removal deleted the lock file while this waited for its lock, and maybe the - // directory: the directory is created and locked again. - Ok(Taken::Replaced) => continue, + // directory, or a transition replaced it as it released its lock: the + // directory is created and locked again. + Ok(Taken::Replaced) => { + if !races.lost(deadline) { + return Err(transition_in_progress(dir)); + } + continue; + } Err(error) => error, }, Err(error) => error, }; // A removal deleted the directory after it was created, before it was checked or its - // lock file was opened: unless its parent is gone, it is created again, at once the - // first time. Another such failure right after it pauses first, until the deadline, - // so that a directory that keeps disappearing cannot spin. + // lock file was opened: unless its parent is gone, it is created again. if error.kind() != ErrorKind::NotFound || !dir.parent().is_some_and(Path::is_dir) { return Err(error); } - if retried_at_once && !pause(deadline) { + if !races.lost(deadline) { return Err(transition_in_progress(dir)); } - retried_at_once = !retried_at_once; } } @@ -106,11 +116,25 @@ impl SandboxStore { files: &CallerFiles, deadline: Option, ) -> Result { - let store = Self::open(dir)?; + Self::existing_lock(Self::open(dir)?, files, deadline, Self::take_lifecycle_lock) + } + + /// Takes the lock as [`lock_existing`](Self::lock_existing) does, of the opened `store`, with + /// `take` taking it, as [`take_lifecycle_lock`](Self::take_lifecycle_lock) does. + fn existing_lock( + store: Self, + files: &CallerFiles, + deadline: Option, + mut take: impl FnMut(&Self, Option) -> Result, + ) -> Result { + let mut races = Races::default(); loop { - if let Taken::Locked(lock) = store.take_lifecycle_lock(deadline)? { + if let Taken::Locked(lock) = take(&store, deadline)? { return Ok(LockedStore::new(store, lock, files.clone())); } + if !races.lost(deadline) { + return Err(transition_in_progress(store.dir())); + } } } @@ -161,18 +185,6 @@ fn pending_deletion(path: &Path) -> bool { .is_err_and(|error| error.kind() == io::ErrorKind::PermissionDenied) } -/// Waits before the next try of a step that `deadline` bounds: returns `false` at once if the -/// deadline has passed, and otherwise sleeps for [`LOCK_RETRY_INTERVAL`] or the time left, -/// whichever is shorter, and returns `true`. -fn pause(deadline: Option) -> bool { - let left = deadline.map(|deadline| deadline.saturating_duration_since(Instant::now())); - if left.is_some_and(|left| left.is_zero()) { - return false; - } - thread::sleep(left.map_or(LOCK_RETRY_INTERVAL, |left| left.min(LOCK_RETRY_INTERVAL))); - true -} - impl Drop for LockedStore { /// Releases the lock. A holder that leaves the directory without its caller's configuration /// record removes its own lock file first, unless it removed it already, so that a directory diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index a3b0beb7e..9feff4c7e 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -5,6 +5,7 @@ use std::env; use std::io::{BufRead, BufReader, Read}; use std::process::{Child, ChildStdout, Command, Stdio}; use std::sync::mpsc; +use std::thread; use std::time::Duration; use super::*; @@ -546,15 +547,21 @@ fn a_provision_creates_again_a_directory_that_a_removal_deletes_while_it_is_crea let dir = directory.path().join("sandbox"); // A concurrent removal deletes the directory after it is created, before it is checked. let mut attempts = 0; - let locked = SandboxStore::provision_lock(&dir, &files(), within(10), |dir| { - attempts += 1; - if attempts == 1 { - SandboxStore::create(dir).unwrap(); - fs::remove_dir(dir).unwrap(); - return SandboxStore::open(dir); - } - SandboxStore::create(dir) - }) + let locked = SandboxStore::provision_lock( + &dir, + &files(), + within(10), + |dir| { + attempts += 1; + if attempts == 1 { + SandboxStore::create(dir).unwrap(); + fs::remove_dir(dir).unwrap(); + return SandboxStore::open(dir); + } + SandboxStore::create(dir) + }, + SandboxStore::take_lifecycle_lock, + ) .unwrap(); assert_eq!(attempts, 2); assert!(!locked.dir().join(CONFIG).exists()); @@ -568,15 +575,56 @@ fn a_provision_creates_again_a_directory_that_a_removal_deletes_while_it_is_crea let gone = directory.path().join("gone"); let mut attempts = 0; let deadline = Some(Instant::now() + Duration::from_millis(200)); - let error = SandboxStore::provision_lock(&dir, &files(), deadline, |_| { - attempts += 1; - SandboxStore::open(&gone) - }) + let error = SandboxStore::provision_lock( + &dir, + &files(), + deadline, + |_| { + attempts += 1; + SandboxStore::open(&gone) + }, + SandboxStore::take_lifecycle_lock, + ) .unwrap_err(); assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); assert!(attempts > 2, "{attempts}"); } +#[test] +fn locks_whose_lock_file_keeps_being_replaced_keep_their_deadline() { + // Transitions that keep removing their lock files as they release their locks replace the + // lock file each time that a waiter takes its lock. The waiter takes the lock again at once + // the first time, and after a pause when that repeats, until its deadline. + let (_directory, dir) = sandbox(); + for provision in [false, true] { + for (deadline, most) in [ + (Some(Instant::now()), 2), + (Some(Instant::now() + Duration::from_millis(200)), 20), + ] { + let mut taken = 0; + let replaced = |_: &SandboxStore, _: Option| { + taken += 1; + Ok(Taken::Replaced) + }; + let error = if provision { + SandboxStore::provision_lock( + &dir, + &files(), + deadline, + |dir| SandboxStore::create(dir), + replaced, + ) + } else { + let store = SandboxStore::open(&dir).unwrap(); + SandboxStore::existing_lock(store, &files(), deadline, replaced) + } + .unwrap_err(); + assert_eq!(error.kind(), ErrorKind::TimedOut, "{error}"); + assert!((2..=most).contains(&taken), "{provision} {most}: {taken}"); + } + } +} + #[test] fn a_provision_that_waits_through_a_removal_provisions_the_sandbox_again() { let (_directory, dir) = sandbox(); diff --git a/nvx-rs/src/host/store/mod.rs b/nvx-rs/src/host/store/mod.rs index 81290a68f..cc4f12703 100644 --- a/nvx-rs/src/host/store/mod.rs +++ b/nvx-rs/src/host/store/mod.rs @@ -182,7 +182,10 @@ //! //! Both locks wait for another holder until the caller's deadline, trying again every 25 ms, and //! a deadline that has passed still tries once, so that a caller that waits on its own, as for an -//! interruption, can retry them itself. +//! interruption, can retry them itself. A lock file that another transition removes or replaces +//! after an attempt opened it is locked again at once, and after a pause, until the deadline, when +//! that happens again right after it, so that transitions that keep replacing the lock file keep +//! no caller past its deadline. //! //! # Removal //! @@ -325,6 +328,41 @@ fn transition_in_progress(dir: &Path) -> Error { )) } +/// Waits before the next try of a step that `deadline` bounds: returns `false` at once if the +/// deadline has passed, and otherwise sleeps for [`LOCK_RETRY_INTERVAL`] or the time left, +/// whichever is shorter, and returns `true`. +fn pause(deadline: Option) -> bool { + let left = deadline.map(|deadline| deadline.saturating_duration_since(Instant::now())); + if left.is_some_and(|left| left.is_zero()) { + return false; + } + thread::sleep(left.map_or(LOCK_RETRY_INTERVAL, |left| left.min(LOCK_RETRY_INTERVAL))); + true +} + +/// The races that the attempts to take a lock lose to other transitions, which remove or replace +/// the lock file, or the directory, after an attempt opens it. The attempt after a lost race +/// follows at once, and one after another race lost right after it pauses first, until the +/// deadline, so that transitions that keep replacing the lock file can neither keep a caller past +/// its deadline nor make it spin. +#[derive(Default)] +struct Races { + /// Whether the latest attempt followed at once one that lost a race. + retried_at_once: bool, +} + +impl Races { + /// Records that an attempt lost a race, and returns whether the lock may be tried again + /// before `deadline`, which it pauses for first if the attempt followed one that lost too. + fn lost(&mut self, deadline: Option) -> bool { + if self.retried_at_once && !pause(deadline) { + return false; + } + self.retried_at_once = !self.retried_at_once; + true + } +} + /// How an attempt to take a store's lock ended, if it did not fail. enum Taken { /// The lock of the lock file that the path names. @@ -457,18 +495,23 @@ impl SandboxStore { /// /// Fails with [`ErrorKind::TimedOut`] when the deadline passes first, and with /// [`ErrorKind::NotFound`] if the directory does not exist. A lock file that is removed or - /// replaced while this waits locks nothing, so the lock is then taken again. A lock file that - /// is not private to the current user is refused, as a record is, since others could hold its - /// lock, and one that is a link, or not a regular file, is refused at once. + /// replaced while this waits locks nothing, so the lock is then taken again, at once the first + /// time, and after a pause, until the deadline, when that happens again right after it. A lock + /// file that is not private to the current user is refused, as a record is, since others could + /// hold its lock, and one that is a link, or not a regular file, is refused at once. /// /// This lock knows nothing of its caller's files. A caller that keeps a configuration record /// in the directory, or removes the store, takes the [lifecycle locks](self#lifecycle-locks) /// instead. pub fn lock(&self, deadline: Option) -> Result { + let mut races = Races::default(); loop { if let Taken::Locked(file) = self.take_lock(deadline)? { return Ok(LockedStore::new(self.clone(), file, CallerFiles::none())); } + if !races.lost(deadline) { + return Err(transition_in_progress(&self.dir)); + } } } From 61a63246eb73e372ddfcf97f526318a346754af3 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 14:44:37 -0700 Subject: [PATCH 10/14] nvx-rs: find a reprovisioned record by its path in a lingering removal A lingering removal told a provision that took the lock after it from a foreign file by looking for the configuration record's name among the directory's entries. A file system that ignores case keeps the name that the provision gave the record, which can differ in case from the removal's, so the removal took the record for a foreign file. It now looks the record up by its path, as the release of a lock does, which such a file system resolves, and one that heeds case keeps apart from other names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/lifecycle.rs | 27 +++++++++++++---------- nvx-rs/src/host/store/lifecycle/tests.rs | 28 ++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/nvx-rs/src/host/store/lifecycle.rs b/nvx-rs/src/host/store/lifecycle.rs index 7b45d14f2..7d2dbf938 100644 --- a/nvx-rs/src/host/store/lifecycle.rs +++ b/nvx-rs/src/host/store/lifecycle.rs @@ -377,8 +377,10 @@ impl Lingering { /// directory, which it checks before each attempt to remove it. Fails, and keeps the /// directory, with [`RemovalError::ProvisionedAgain`] once the caller's configuration record /// appears in it, as a provision that took the lock after the removal released it writes it, - /// with [`RemovalError::Foreign`] for other entries than lock files and the removed files that - /// NFS keeps, sorted, and with [`RemovalError::TimedOut`] once the deadline passes. + /// which it looks up by its path, so that a file system that ignores case finds it whatever + /// the case of the name that the provision gave it, with [`RemovalError::Foreign`] for other + /// entries than lock files and the removed files that NFS keeps, sorted, and with + /// [`RemovalError::TimedOut`] once the deadline passes. pub fn wait(&mut self, deadline: Option) -> std::result::Result<(), RemovalError> { loop { let error = match remove_held_directory(&self.dir, self.identity) { @@ -405,16 +407,19 @@ impl Lingering { .cloned() .collect(); unexpected.sort(); - // A provision that took the lock after the removal released it, before the directory - // went, provisions the directory again. - if self - .config - .as_ref() - .is_some_and(|config| unexpected.contains(config)) - { - return Err(RemovalError::ProvisionedAgain); - } if !unexpected.is_empty() { + // A provision that took the lock after the removal released it, before the + // directory went, provisions the directory again. Its record is looked up by its + // path, which a file system that ignores case resolves to the name that the + // provision gave it, and one that does not keeps apart from other names. + if let Some(config) = &self.config { + let path = self.dir.join(config); + match fs::symlink_metadata(&path) { + Ok(_) => return Err(RemovalError::ProvisionedAgain), + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(RemovalError::Io { path, error }), + } + } return Err(RemovalError::Foreign(unexpected)); } if !directory_not_empty(&error) { diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index 9feff4c7e..73af3107e 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -463,6 +463,34 @@ fn a_lingering_removal_reports_what_appears_in_the_directory() { assert_eq!(entries_of(&dir), [RENAMED]); } +#[test] +fn a_lingering_removal_finds_the_configuration_record_by_its_path() { + // A provision by a caller that declares the configuration record in another case writes the + // removal's record on a file system that ignores case, as Windows' do, and another file on + // one that does not. + let (_directory, dir) = sandbox(); + let mut lingering = remove_lingering(&dir); + let upper = CONFIG.to_ascii_uppercase(); + let files = CallerFiles::new(upper.as_str()).unwrap(); + let provision = SandboxStore::lock_for_provision(&dir, &files, within(10)).unwrap(); + fs::write(dir.join(&upper), b"{}").unwrap(); + drop(provision); + fs::remove_file(dir.join(RENAMED)).unwrap(); + let ignores_case = dir.join(CONFIG).exists(); + match lingering.wait(within(10)) { + Err(RemovalError::ProvisionedAgain) => assert!(ignores_case), + Err(RemovalError::Foreign(names)) => { + assert!( + !ignores_case, + "a provision in another case was taken for a foreign file" + ); + assert_eq!(names, std::slice::from_ref(&upper)); + } + other => panic!("{other:?}"), + } + assert_eq!(entries_of(&dir), sorted(&[upper.as_str(), LOCK_NAME])); +} + #[test] fn a_removal_that_another_finishes_succeeds_and_keeps_what_replaced_the_directory() { // Another removal, which takes the lock of a new lock file once this one has removed its own, From c32b50ee25a677b238f90588831bafbe011eb2e1 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 15:10:19 -0700 Subject: [PATCH 11/14] nvx-rs: let the staging test wait for a log that a test's child holds A process that a concurrent test spawns holds the test's descriptors from its fork until it executes its program, and with them the claim of the log that an abandoned launch closed, for longer than the claim release wait if the machine is loaded. The abandon then ends as indeterminate and keeps the launch's staging file. Accept that in the staging test, check that the file stays, and wait for the next recovery to end the launch with released(), as the test already does for the recovery before it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/tests.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/nvx-rs/src/host/store/tests.rs b/nvx-rs/src/host/store/tests.rs index 0ca806366..9459c2343 100644 --- a/nvx-rs/src/host/store/tests.rs +++ b/nvx-rs/src/host/store/tests.rs @@ -860,7 +860,16 @@ fn report_staging_files_go_once_nothing_of_their_launch_runs() { // An abandoned launch removes it, and so does clearing the runtime of a VM that exited. let launch = locked.begin_launch(&runtime(), &CAPABILITY).unwrap(); fs::write(&staging, b"{").unwrap(); - assert_eq!(launch.abandon().unwrap(), Recovery::Exited); + match launch.abandon().unwrap() { + Recovery::Exited => {} + // A process that a concurrent test spawned may still hold the log that the launch closed, + // until it executes its program, and the next recovery then ends the launch. + Recovery::Indeterminate(_) => { + assert!(staging.exists()); + assert!(released(|| locked.recover().unwrap() == Recovery::Exited)); + } + other => panic!("{other:?}"), + } assert!(!staging.exists()); fs::write(&staging, b"{").unwrap(); locked.clear_runtime().unwrap(); From b63694808c710990852aa9736894baad10245646 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 15:38:19 -0700 Subject: [PATCH 12/14] nvx-rs: remove a configuration's interrupted writes with its sandbox An interrupted write_private or write_json leaves .NAME.NUMBER.tmp beside the file. The store refuses such files of a caller's records, which are neither its own nor the files that the caller declares, so aci_edge_sandboxes, whose removal removed every .*.tmp before it moved onto the store, kept the one that a provision that died while it wrote sandbox.json leaves: a later call with the stale ID left it and the directory behind, and a deprovision would refuse it. Add host::instance::remove_interrupted_writes, which removes exactly the names that write_private gives the temporary files of a path, and call it under the lock before both removals of aci_edge_sandboxes: the cleanup of a stale ID and deprovision. Other .*.tmp names stay foreign. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- aci_edge_sandboxes/src/openvmm/state.rs | 64 ++++++++++++--- nvx-rs/src/host/instance.rs | 101 +++++++++++++++++++++++- 2 files changed, 150 insertions(+), 15 deletions(-) diff --git a/aci_edge_sandboxes/src/openvmm/state.rs b/aci_edge_sandboxes/src/openvmm/state.rs index f88241d1f..9bd7d1a9c 100644 --- a/aci_edge_sandboxes/src/openvmm/state.rs +++ b/aci_edge_sandboxes/src/openvmm/state.rs @@ -111,8 +111,9 @@ impl StateStore { /// An ID without configuration fails with [`ErrorCode::StaleId`], also when its state /// disappears while this call waits for the lock, as it does when a concurrent deprovision /// removes it. A stale ID never becomes valid again, so the call removes what is left of the - /// sandbox's directory, such as one that an interrupted removal left, as the store removes a - /// sandbox: a stale ID leaves nothing behind but entries that are not the sandbox's. + /// sandbox's directory, such as one that an interrupted removal left, or the temporary file of + /// a configuration that a provision did not finish writing, as [`remove`](Self::remove) + /// removes a sandbox: a stale ID leaves nothing behind but entries that are not the sandbox's. pub(crate) fn lock_and_load( &self, sandbox_id: &SandboxId, @@ -131,6 +132,7 @@ impl StateStore { match self.load(sandbox_id) { Ok(record) => Ok((locked, record)), Err(error) if error.code() == ErrorCode::StaleId => { + let _ = instance::remove_interrupted_writes(&dir.join(RECORD_NAME)); let _ = locked.remove(Some(Instant::now())); Err(error) } @@ -182,19 +184,19 @@ impl StateStore { /// Deletes the state of a stopped sandbox, whose lock `locked` holds, through the store of its /// instance, and releases the lock. /// - /// The store removes its files and then the configuration, so an interrupted removal leaves a - /// sandbox that can be deprovisioned again. It then refuses entries that are neither the - /// store's nor this crate's, which it keeps with the directory, and the ID is stale from then - /// on. Otherwise it removes the lock file while it still holds the lock, so that a transition - /// that waits for the lock finds no sandbox, and then the directory, waiting for the open - /// files, such as the lock files of such transitions, that keep it for a while, as it waits - /// for the lock itself. + /// The temporary files that interrupted writes of the configuration left go first, since the + /// store does not own them. The store then removes its files and the configuration, so an + /// interrupted removal leaves a sandbox that can be deprovisioned again. It then refuses + /// entries that are neither the store's nor this crate's, which it keeps with the directory, + /// and the ID is stale from then on. Otherwise it removes the lock file while it still holds + /// the lock, so that a transition that waits for the lock finds no sandbox, and then the + /// directory, waiting for the open files, such as the lock files of such transitions, that + /// keep it for a while, as it waits for the lock itself. pub(crate) fn remove(&self, sandbox_id: &SandboxId, locked: LockedInstance) -> Result<()> { + let dir = self.dir(sandbox_id); + instance::remove_interrupted_writes(&dir.join(RECORD_NAME))?; locked.remove(None).map_err(|error| { - let message = format!( - "cannot remove sandbox state {}: {error}", - self.dir(sandbox_id).display() - ); + let message = format!("cannot remove sandbox state {}: {error}", dir.display()); match error { RemovalError::TimedOut(source) | RemovalError::Io { error: source, .. } => { Error::backend_error(message).with_source(source) @@ -437,6 +439,42 @@ mod tests { assert_eq!(entries(&store.dir(&id)), [RECORD_NAME, LOCK_NAME]); } + #[test] + fn interrupted_configuration_writes_go_with_the_sandbox() { + let root = tempfile::tempdir().unwrap(); + let store = StateStore::open(root.path()).unwrap(); + // A provision that died while it wrote the configuration left its temporary file, and no + // configuration, so the ID is stale, and leaves nothing behind. + let interrupted = SandboxId::generate().unwrap(); + Instance::create(store.dir(&interrupted), interrupted.to_string(), files()).unwrap(); + fs::write( + store.dir(&interrupted).join(".sandbox.json.12345.tmp"), + b"{", + ) + .unwrap(); + let error = store.lock_and_load(&interrupted).unwrap_err(); + assert_eq!(error.code(), ErrorCode::StaleId, "{error}"); + assert!(entries(root.path()).is_empty()); + // Deprovision removes such files beside the configuration too. + let id = SandboxId::generate().unwrap(); + store.create(&id, &record()).unwrap(); + let dir = store.dir(&id); + fs::write(dir.join(".sandbox.json.18446744073709551615.tmp"), b"{").unwrap(); + let (locked, _) = store.lock_and_load(&id).unwrap(); + store.remove(&id, locked).unwrap(); + assert!(entries(root.path()).is_empty()); + // A name that only resembles theirs is not the sandbox's. + store.create(&id, &record()).unwrap(); + fs::write(dir.join(".sandbox.json.012.tmp"), b"{").unwrap(); + let (locked, _) = store.lock_and_load(&id).unwrap(); + let error = store.remove(&id, locked).unwrap_err(); + assert!( + error.message().ends_with(": .sandbox.json.012.tmp"), + "{error}" + ); + assert_eq!(entries(&dir), [".sandbox.json.012.tmp"]); + } + #[test] fn state_removed_while_waiting_for_the_lock_leaves_nothing_behind() { let root = tempfile::tempdir().unwrap(); diff --git a/nvx-rs/src/host/instance.rs b/nvx-rs/src/host/instance.rs index 41b3bb9dc..81e8e40f6 100644 --- a/nvx-rs/src/host/instance.rs +++ b/nvx-rs/src/host/instance.rs @@ -92,8 +92,18 @@ fn private_options() -> OpenOptions { options } +/// Returns the name of the temporary file through which [`write_private`] replaces the file `name` +/// for the random `number`: `.NAME.NUMBER.tmp`, which stays beside it if the replacement is +/// interrupted. +fn temporary_name(name: &str, number: u64) -> String { + format!(".{name}.{number}.tmp") +} + /// Atomically replaces `path` with `contents`, readable only by the current user, as callers do /// with the configuration records that they keep beside an instance's store. +/// +/// The contents go to a temporary file beside `path` first, which a replacement that is +/// interrupted leaves, and which [`remove_interrupted_writes`] removes. pub fn write_private(path: &Path, contents: &[u8]) -> Result<()> { let mut suffix = [0u8; 8]; getrandom::fill(&mut suffix) @@ -102,7 +112,7 @@ pub fn write_private(path: &Path, contents: &[u8]) -> Result<()> { .file_name() .map(|name| name.to_string_lossy().into_owned()) .unwrap_or_default(); - let temporary = path.with_file_name(format!(".{name}.{}.tmp", u64::from_le_bytes(suffix))); + let temporary = path.with_file_name(temporary_name(&name, u64::from_le_bytes(suffix))); let written = (|| -> io::Result<()> { let mut file = private_options().create_new(true).open(&temporary)?; file.write_all(contents)?; @@ -147,6 +157,50 @@ pub fn remove_if_present(path: &Path) -> Result<()> { } } +/// Removes the temporary files that interrupted replacements of `path` with [`write_private`] or +/// [`write_json`] left beside it, and keeps every other file. A directory that does not exist has +/// none. +/// +/// The store's removal refuses those files, which are neither the store's nor the files that its +/// caller declares, so a caller that writes its files with [`write_private`] removes them first, +/// under the instance's lock. +pub fn remove_interrupted_writes(path: &Path) -> Result<()> { + let Some(name) = path.file_name().map(|name| name.to_string_lossy()) else { + return Ok(()); + }; + let dir = match path.parent() { + Some(dir) if !dir.as_os_str().is_empty() => dir, + _ => Path::new("."), + }; + let unlisted = || io_error(format!("cannot list {}", dir.display())); + let entries = match fs::read_dir(dir) { + Ok(entries) => entries, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(unlisted()(error)), + }; + for entry in entries { + let entry = entry.map_err(unlisted())?; + if entry + .file_name() + .to_str() + .is_some_and(|entry| is_temporary_of(entry, &name)) + { + remove_if_present(&entry.path())?; + } + } + Ok(()) +} + +/// Returns whether `entry` is the name of a temporary file through which [`write_private`] +/// replaces the file `name`. +fn is_temporary_of(entry: &str, name: &str) -> bool { + entry + .strip_suffix(".tmp") + .and_then(|rest| rest.rsplit_once('.')) + .and_then(|(_, number)| number.parse::().ok()) + .is_some_and(|number| temporary_name(name, number) == entry) +} + /// Returns a fresh launch capability, which is never all zero. pub fn generate_capability() -> Result<[u8; CAPABILITY_LEN]> { let mut capability = [0u8; CAPABILITY_LEN]; @@ -726,7 +780,9 @@ impl LockedInstance { /// The caller has found the instance stopped with [`find`](Self::find): the removal refuses, /// and removes nothing, while the store records a VM or a start, and for an instance whose /// caller declared no configuration record. Entries in the directory that are neither the - /// store's nor the caller's are refused, and kept with the directory, once the rest is gone. + /// store's nor the caller's are refused, and kept with the directory, once the rest is gone, + /// among them the temporary files that interrupted writes of the caller's files left, which + /// the caller removes first with [`remove_interrupted_writes`]. pub fn remove(self, deadline: Option) -> std::result::Result<(), RemovalError> { match self.locked.remove()? { None => Ok(()), @@ -909,6 +965,47 @@ impl Execution { mod tests { use super::*; + #[test] + fn interrupted_writes_leave_temporary_files_that_only_their_removal_takes() { + let directory = tempfile::tempdir().unwrap(); + let names = || { + let mut names: Vec = fs::read_dir(directory.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name().into_string().unwrap()) + .collect(); + names.sort(); + names + }; + let path = directory.path().join("config.json"); + write_json(&path, &serde_json::json!({ "written": true })).unwrap(); + assert_eq!(names(), ["config.json"]); + // Replacements of the file that were interrupted left their temporary files, which other + // files only resemble. + let left = [0, 12345, u64::MAX].map(|number| temporary_name("config.json", number)); + let others = [ + ".config.json.tmp", + ".config.json..tmp", + ".config.json.012.tmp", + ".config.json.+12.tmp", + ".config.json.1a.tmp", + ".config.json.18446744073709551616.tmp", + ".config.json.1.tmp.tmp", + ".xconfig.json.1.tmp", + ".other.json.1.tmp", + "config.json.1.tmp", + ".tmpAbC123", + ]; + for name in left.iter().map(String::as_str).chain(others) { + fs::write(directory.path().join(name), b"{").unwrap(); + } + remove_interrupted_writes(&path).unwrap(); + let mut kept: Vec<&str> = others.into_iter().chain(["config.json"]).collect(); + kept.sort_unstable(); + assert_eq!(names(), kept); + // A directory that does not exist has none. + remove_interrupted_writes(&directory.path().join("gone").join("config.json")).unwrap(); + } + #[test] fn readiness_failures_explain_images_without_workload_accounts() { let hint = "predates workload account creation"; From 16a29ba623598002fadcb52cd5fa55944bfb9b86 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 16:25:12 -0700 Subject: [PATCH 13/14] nvx-rs: let removal tests accept late waiters when nothing lingers A transition that opens the lock file after a removal removed it creates a new one, takes its lock, finds no sandbox, and removes its lock file as it releases it. If it does all that before the removal removes the directory, the removal finds the directory empty and returns no lingering removal, while the transition reports no sandbox rather than the directory gone. The two tests of transitions that wait through a removal accepted that outcome only when the removal lingered. Accept it either way, and keep the checks that no transition finds the sandbox provisioned, and that the directory goes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/lifecycle/tests.rs | 53 ++++++++++-------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index 73af3107e..6df102287 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -711,27 +711,19 @@ fn transitions_that_wait_through_a_removal_find_the_sandbox_gone() { .into_iter() .map(|waiter| waiter.join().unwrap()) .collect(); - match lingering { - // The waiters wait for the lock of the removed lock file, which the removal releases - // once the directory is gone. - None => { - for outcome in outcomes { - let error = outcome.unwrap_err(); - assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"); - } - } - // A waiter that started late opened the lock file between the removals of the lock file - // and of the directory, which created a new one, and found no sandbox. - Some(mut lingering) => { - for outcome in outcomes { - match outcome { - Ok(provisioned) => assert!(!provisioned), - Err(error) => assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"), - } - } - lingering.wait(within(30)).unwrap(); + // The waiters wait for the lock of the removed lock file, and find the directory gone. A + // waiter that started late may open the lock file between the removals of the lock file and + // of the directory, which creates a new one, and find no sandbox: the directory then lingers + // until it releases that lock, unless it released it before the removal of the directory. + for outcome in outcomes { + match outcome { + Ok(provisioned) => assert!(!provisioned), + Err(error) => assert_eq!(error.kind(), ErrorKind::NotFound, "{error}"), } } + if let Some(mut lingering) = lingering { + lingering.wait(within(30)).unwrap(); + } assert!(!dir.exists()); } @@ -763,19 +755,16 @@ fn transitions_in_other_processes_that_wait_through_a_removal_find_the_sandbox_g assert!(transitions.iter_mut().all(Transition::runs)); let lingering = locked.remove().unwrap(); let outcomes: Vec<_> = transitions.into_iter().map(Transition::outcome).collect(); - match lingering { - None => assert_eq!(outcomes, ["gone", "gone"]), - // A transition that started late opened the lock file between the removals of the lock - // file and of the directory, which created a new one, and found no sandbox. - Some(mut lingering) => { - for outcome in &outcomes { - assert!( - ["gone", "unprovisioned"].contains(&outcome.as_str()), - "{outcome}" - ); - } - lingering.wait(within(30)).unwrap(); - } + // As with threads, a transition that started late may find no sandbox rather than the + // directory gone, whether the directory lingers or not. + for outcome in &outcomes { + assert!( + ["gone", "unprovisioned"].contains(&outcome.as_str()), + "{outcome}" + ); + } + if let Some(mut lingering) = lingering { + lingering.wait(within(30)).unwrap(); } assert!(!dir.exists()); } From 97827c95d2cf45a45253b8e793f8989f335080e8 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 22 Oct 2026 16:40:54 -0700 Subject: [PATCH 14/14] nvx-rs: let provision race tests accept a removal that finishes first Where a removed file stays while a handle holds it, as NFS keeps it under a .nfs name and a Windows file system without POSIX deletion under its own, a provision that waits for the lock of the removed lock file keeps the directory with its handle, and the removal lingers. Once the provision finds the lock file replaced and closes that handle, the directory empties, and the lingering removal can remove it before the provision creates it again, which ends the wait with Ok(()) rather than ProvisionedAgain. The two tests of provisions that race a removal accepted only ProvisionedAgain from a lingering removal. Accept both, and keep the checks of the provision's outcome and of the directory that it leaves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bba527e3-2680-407f-ba91-a21eeb2ea18a --- nvx-rs/src/host/store/lifecycle/tests.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/nvx-rs/src/host/store/lifecycle/tests.rs b/nvx-rs/src/host/store/lifecycle/tests.rs index 6df102287..f7b67f4d5 100644 --- a/nvx-rs/src/host/store/lifecycle/tests.rs +++ b/nvx-rs/src/host/store/lifecycle/tests.rs @@ -673,13 +673,16 @@ fn a_provision_that_waits_through_a_removal_provisions_the_sandbox_again() { thread::sleep(Duration::from_millis(100)); // The removal completes before the provision takes the lock, which creates the directory // again, or, should the provision open the lock file between the removals of the lock file - // and of the directory, reports the provision. + // and of the directory, reports the provision. Where a removed file stays while a handle + // holds it, as on NFS, the handle with which the provision waits for the removed lock file + // keeps the directory instead, which then goes once the provision closes it, before the + // provision creates the directory again. match locked.remove().unwrap() { None => {} Some(mut lingering) => { assert!(matches!( lingering.wait(within(30)), - Err(RemovalError::ProvisionedAgain) + Ok(()) | Err(RemovalError::ProvisionedAgain) )); } } @@ -778,10 +781,12 @@ fn provisions_in_other_processes_that_race_a_removal_provision_the_sandbox_again let mut provision = Transition::start(&dir, "provision"); thread::sleep(Duration::from_millis(100)); assert!(provision.runs()); + // A lingering removal reports the provision, or removes the directory that the provision's + // handle of the removed lock file kept, as in the threads' case. if let Some(mut lingering) = locked.remove().unwrap() { assert!(matches!( lingering.wait(within(30)), - Err(RemovalError::ProvisionedAgain) + Ok(()) | Err(RemovalError::ProvisionedAgain) )); } assert_eq!(provision.outcome(), "unprovisioned");