From 2a7ee256f5c55490767f6155954e602bc87233f1 Mon Sep 17 00:00:00 2001 From: Hare Date: Sat, 18 Apr 2026 19:25:03 +0900 Subject: [PATCH] =?UTF-8?q?Scope-Lock=E3=81=AE=E5=AE=9F=E8=A3=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Cargo.lock | 27 +- crates/manifest/src/scope.rs | 16 + crates/pod/Cargo.toml | 2 + crates/pod/src/lib.rs | 1 + crates/pod/src/pod.rs | 29 + crates/pod/src/runtime_dir.rs | 6 +- crates/pod/src/scope_lock.rs | 1014 +++++++++++++++++++++++++++++++++ tickets/scope-lock.md | 5 + tickets/scope-lock.review.md | 100 ++++ 9 files changed, 1196 insertions(+), 4 deletions(-) create mode 100644 crates/pod/src/scope_lock.rs create mode 100644 tickets/scope-lock.review.md diff --git a/Cargo.lock b/Cargo.lock index 7ef32e44..1d4e0307 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -735,6 +735,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs4" +version = "0.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8640e34b88f7652208ce9e88b1a37a2ae95227d84abec377ccd3c5cfeb141ed4" +dependencies = [ + "rustix 1.1.4", + "windows-sys 0.59.0", +] + [[package]] name = "futures" version = "0.3.32" @@ -1388,9 +1398,9 @@ checksum = "09edd9e8b54e49e587e4f6295a7d29c3ea94d469cb40ab8ca70b288248a81db2" [[package]] name = "libc" -version = "0.2.184" +version = "0.2.185" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48f5d2a454e16a5ea0f4ced81bd44e4cfc7bd3a507b61887c99fd3538b28e4af" +checksum = "52ff2c0fe9bc6cb6b14a0592c2ff4fa9ceb83eea9db979b0487cd054946a2b8f" [[package]] name = "libredox" @@ -1893,8 +1903,10 @@ dependencies = [ "chrono", "clap", "dotenv", + "fs4", "futures", "include_dir", + "libc", "llm-worker", "manifest", "minijinja", @@ -2244,7 +2256,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -3583,6 +3595,15 @@ dependencies = [ "windows-targets", ] +[[package]] +name = "windows-sys" +version = "0.59.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" +dependencies = [ + "windows-targets", +] + [[package]] name = "windows-sys" version = "0.61.2" diff --git a/crates/manifest/src/scope.rs b/crates/manifest/src/scope.rs index 856b76e1..8a64e364 100644 --- a/crates/manifest/src/scope.rs +++ b/crates/manifest/src/scope.rs @@ -138,6 +138,22 @@ impl Scope { self.allow.iter().map(|r| r.target.as_path()) } + /// Allow rules with their targets resolved to absolute paths. + /// + /// Used by the scope-lock registry, where every Pod's allocation + /// must be expressed in absolute terms so prefix comparisons are + /// meaningful across processes. + pub fn allow_rules(&self) -> Vec { + self.allow + .iter() + .map(|r| ScopeRule { + target: r.target.clone(), + permission: r.permission, + recursive: r.recursive, + }) + .collect() + } + /// Iterate over absolute paths granted `Write` by an allow rule. /// Subset of [`readable_paths`](Self::readable_paths). pub fn writable_paths(&self) -> impl Iterator { diff --git a/crates/pod/Cargo.toml b/crates/pod/Cargo.toml index 13513503..7e30e2b9 100644 --- a/crates/pod/Cargo.toml +++ b/crates/pod/Cargo.toml @@ -22,6 +22,8 @@ tools = { version = "0.1.0", path = "../tools" } minijinja = "2.19.0" chrono = "0.4.44" include_dir = "0.7.4" +fs4 = { version = "0.13.1", features = ["sync"] } +libc = "0.2.185" [dev-dependencies] async-trait = "0.1.89" diff --git a/crates/pod/src/lib.rs b/crates/pod/src/lib.rs index 8cc5880c..adf8b26a 100644 --- a/crates/pod/src/lib.rs +++ b/crates/pod/src/lib.rs @@ -2,6 +2,7 @@ pub mod controller; pub mod hook; pub mod notifier; pub mod runtime_dir; +pub mod scope_lock; pub mod shared_state; pub mod socket_server; diff --git a/crates/pod/src/pod.rs b/crates/pod/src/pod.rs index 86c6ae0b..8288373c 100644 --- a/crates/pod/src/pod.rs +++ b/crates/pod/src/pod.rs @@ -23,6 +23,8 @@ use crate::notification_buffer::NotificationBuffer; use crate::notifier::Notifier; use crate::pod_interceptor::PodInterceptor; use crate::prompt_loader::PromptLoader; +use crate::runtime_dir; +use crate::scope_lock::{self, ScopeAllocationGuard, ScopeLockError}; use crate::system_prompt::{SystemPromptContext, SystemPromptError, SystemPromptTemplate}; use crate::usage_tracker::UsageTracker; use protocol::{NotificationLevel, NotificationSource}; @@ -105,6 +107,13 @@ pub struct Pod { /// injection into the next LLM request. Shared with the /// PodInterceptor installed in `ensure_interceptor_installed`. pending_notifications: NotificationBuffer, + /// Scope allocation in the machine-wide lock file. `Some` for + /// Pods built via `from_manifest` (production path); `None` for + /// lower-level constructors (`Pod::new`, `Pod::restore`) that + /// bypass the registry. Kept purely for its `Drop` impl, which + /// releases the allocation when the Pod is dropped. + #[allow(dead_code)] + scope_allocation: Option, } impl Pod { @@ -146,6 +155,7 @@ impl Pod { system_prompt_template: None, notifier: None, pending_notifications: NotificationBuffer::new(), + scope_allocation: None, }; pod.apply_prune_from_manifest(); Ok(pod) @@ -195,6 +205,7 @@ impl Pod { system_prompt_template: None, notifier: None, pending_notifications: NotificationBuffer::new(), + scope_allocation: None, }; pod.apply_prune_from_manifest(); Ok(pod) @@ -875,6 +886,20 @@ impl Pod, St> { return Err(PodError::PwdOutsideScope { pwd }); } + // Register this Pod in the machine-wide scope-lock registry + // before building anything else, so a spawn that conflicts on + // scope fails fast (and without having paid for client setup). + let socket_path = runtime_dir::default_base() + .map_err(ScopeLockError::from)? + .join(&manifest.pod.name) + .join("sock"); + let scope_allocation = scope_lock::install_top_level( + manifest.pod.name.clone(), + std::process::id(), + socket_path, + scope.allow_rules(), + )?; + let client = provider::build_client(&manifest.provider)?; let mut worker = Worker::new(client); apply_worker_manifest(&mut worker, &manifest.worker); @@ -910,6 +935,7 @@ impl Pod, St> { system_prompt_template, notifier: None, pending_notifications: NotificationBuffer::new(), + scope_allocation: Some(scope_allocation), }; pod.apply_prune_from_manifest(); Ok(pod) @@ -1058,6 +1084,9 @@ pub enum PodError { #[source] source: SystemPromptError, }, + + #[error(transparent)] + ScopeLock(#[from] ScopeLockError), } /// Canonicalize an absolute pwd (resolves symlinks and any `.`/`..` diff --git a/crates/pod/src/runtime_dir.rs b/crates/pod/src/runtime_dir.rs index fe1886d3..3a12da99 100644 --- a/crates/pod/src/runtime_dir.rs +++ b/crates/pod/src/runtime_dir.rs @@ -86,7 +86,11 @@ async fn atomic_write(target: &Path, content: &[u8]) -> Result<(), io::Error> { } /// Resolve the default base directory for runtime data. -fn default_base() -> Result { +/// +/// Public so the scope-lock registry (which lives outside the +/// `RuntimeDir` instance lifecycle) can predict a Pod's socket path +/// without constructing a `RuntimeDir` first. +pub fn default_base() -> Result { if let Ok(runtime_dir) = std::env::var("XDG_RUNTIME_DIR") { Ok(PathBuf::from(runtime_dir).join("insomnia")) } else if let Ok(home) = std::env::var("HOME") { diff --git a/crates/pod/src/scope_lock.rs b/crates/pod/src/scope_lock.rs new file mode 100644 index 00000000..84cf8594 --- /dev/null +++ b/crates/pod/src/scope_lock.rs @@ -0,0 +1,1014 @@ +//! Machine-wide scope allocation registry. +//! +//! A single JSON file at `$XDG_RUNTIME_DIR/insomnia/scope.lock` records +//! every live Pod's scope allocation. File-level `flock(2)` serialises +//! access across processes so spawn sequences from unrelated Pods can't +//! race. +//! +//! Each Pod, when starting, acquires the lock, reclaims stale entries +//! (Pods whose PID has died), checks that its requested write scope +//! does not overlap any other allocation's effective write scope, and +//! registers itself. When it exits normally, it removes its entry and +//! returns delegated scope to its `delegated_from` parent. Crash +//! recovery rides on the next Pod that opens the file — no background +//! reaper. + +use std::fs::{File, OpenOptions}; +use std::io::{self, Read, Seek, SeekFrom, Write}; +use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; +use std::path::{Path, PathBuf}; + +use fs4::fs_std::FileExt; +use manifest::{Permission, ScopeRule}; +use serde::{Deserialize, Serialize}; + +/// On-disk representation of the allocation table. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct LockFile { + #[serde(default)] + pub allocations: Vec, +} + +/// One Pod's scope allocation. +/// +/// `scope_allow` is the full set of allow rules the Pod was granted. +/// Portions delegated out to child Pods are **not** subtracted in +/// storage — the effective write scope is derived on the fly by +/// removing rules owned by any Pod whose `delegated_from` points to +/// this one. Keeping the raw allow set makes reparenting (stale +/// reclaim) trivial. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct Allocation { + /// Pod name — also the identity used throughout orchestration. + pub pod_name: String, + /// Owning process. Checked with `kill(pid, 0)` for stale detection. + pub pid: u32, + /// Pod's Unix socket path. + pub socket: PathBuf, + /// Allow rules granted to this Pod (write + read). + pub scope_allow: Vec, + /// Name of the Pod that delegated scope to this one, or `None` for + /// a top-level Pod started directly by a human. + pub delegated_from: Option, +} + +impl LockFile { + pub fn find(&self, pod_name: &str) -> Option<&Allocation> { + self.allocations.iter().find(|a| a.pod_name == pod_name) + } + + pub fn find_mut(&mut self, pod_name: &str) -> Option<&mut Allocation> { + self.allocations.iter_mut().find(|a| a.pod_name == pod_name) + } +} + +/// Default on-disk path: `$XDG_RUNTIME_DIR/insomnia/scope.lock`, +/// falling back to `~/.insomnia/run/scope.lock` when XDG is unset. +/// +/// Honours `INSOMNIA_SCOPE_LOCK` as an explicit override, primarily so +/// tests can point at a tempdir without polluting the user's runtime +/// directory. +pub fn default_lock_path() -> io::Result { + if let Ok(custom) = std::env::var("INSOMNIA_SCOPE_LOCK") { + return Ok(PathBuf::from(custom)); + } + let base = if let Ok(dir) = std::env::var("XDG_RUNTIME_DIR") { + PathBuf::from(dir).join("insomnia") + } else if let Ok(home) = std::env::var("HOME") { + PathBuf::from(home).join(".insomnia").join("run") + } else { + return Err(io::Error::new( + io::ErrorKind::NotFound, + "neither XDG_RUNTIME_DIR nor HOME is set", + )); + }; + Ok(base.join("scope.lock")) +} + +/// RAII guard over an exclusively-locked lock file. +/// +/// The file is kept open for the lifetime of the guard; `flock(LOCK_EX)` +/// is released automatically on drop. Mutations go through +/// [`LockFileGuard::data_mut`] and are committed with +/// [`LockFileGuard::save`] before dropping — callers who mutate but +/// never call `save` leave the table unchanged, which is the right +/// behaviour for error paths. +pub struct LockFileGuard { + file: File, + data: LockFile, +} + +impl LockFileGuard { + /// Open the lock file at `path` (creating it + parent dirs if + /// needed), acquire an exclusive `flock`, then parse the contents. + /// + /// An empty file is treated as an empty allocation table. + /// + /// File is created with mode `0600` and its parent directory with + /// mode `0700` so no other user on the machine can read the + /// allocation table. Existing files/directories are left alone. + pub fn open(path: &Path) -> io::Result { + if let Some(parent) = path.parent() { + let existed = parent.exists(); + std::fs::create_dir_all(parent)?; + if !existed { + std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700))?; + } + } + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .mode(0o600) + .open(path)?; + FileExt::lock_exclusive(&file)?; + let mut this = Self { + file, + data: LockFile::default(), + }; + this.reload()?; + Ok(this) + } + + fn reload(&mut self) -> io::Result<()> { + self.file.seek(SeekFrom::Start(0))?; + let mut buf = String::new(); + self.file.read_to_string(&mut buf)?; + self.data = if buf.trim().is_empty() { + LockFile::default() + } else { + serde_json::from_str(&buf).map_err(|e| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("scope.lock parse error: {e}"), + ) + })? + }; + Ok(()) + } + + pub fn data(&self) -> &LockFile { + &self.data + } + + pub fn data_mut(&mut self) -> &mut LockFile { + &mut self.data + } + + /// Serialise `self.data` back to the file (truncate + rewrite). + pub fn save(&mut self) -> io::Result<()> { + let json = serde_json::to_vec_pretty(&self.data).map_err(io::Error::other)?; + self.file.seek(SeekFrom::Start(0))?; + self.file.set_len(0)?; + self.file.write_all(&json)?; + self.file.sync_data()?; + Ok(()) + } +} + +impl Drop for LockFileGuard { + fn drop(&mut self) { + let _ = FileExt::unlock(&self.file); + } +} + +/// Whether `a` and `b` claim any overlapping concrete path. +/// +/// Recursive rules cover `target/**`; non-recursive rules cover the +/// target itself and its direct children. The four cases enumerate +/// when those coverage sets intersect. +pub(crate) fn rules_overlap(a: &ScopeRule, b: &ScopeRule) -> bool { + match (a.recursive, b.recursive) { + (true, true) => a.target.starts_with(&b.target) || b.target.starts_with(&a.target), + (true, false) => { + // a covers a.target/**; b covers {b.target, b.target/*}. + b.target.starts_with(&a.target) || a.target.parent() == Some(b.target.as_path()) + } + (false, true) => { + a.target.starts_with(&b.target) || b.target.parent() == Some(a.target.as_path()) + } + (false, false) => { + a.target == b.target + || a.target.parent() == Some(b.target.as_path()) + || b.target.parent() == Some(a.target.as_path()) + } + } +} + +/// Does `cover` fully contain `inner`'s claimed paths? +fn covers_fully(cover: &ScopeRule, inner: &ScopeRule) -> bool { + if cover.permission < inner.permission { + return false; + } + if cover.recursive { + inner.target.starts_with(&cover.target) + } else { + inner.target == cover.target && !inner.recursive + } +} + +/// Check whether `rule` is contained in `parent`'s effective write +/// scope: its allow set covers `rule`, and no child of `parent` has +/// already taken a piece that would overlap `rule`. +pub fn is_within_effective_write(lock: &LockFile, parent: &str, rule: &ScopeRule) -> bool { + let Some(alloc) = lock.find(parent) else { + return false; + }; + if rule.permission != Permission::Write { + return alloc + .scope_allow + .iter() + .any(|r| covers_fully(r, rule)); + } + let covered = alloc + .scope_allow + .iter() + .filter(|r| r.permission == Permission::Write) + .any(|r| covers_fully(r, rule)); + if !covered { + return false; + } + let child_conflict = lock + .allocations + .iter() + .filter(|a| a.delegated_from.as_deref() == Some(parent)) + .flat_map(|a| a.scope_allow.iter()) + .filter(|r| r.permission == Permission::Write) + .any(|r| rules_overlap(r, rule)); + !child_conflict +} + +/// Find the Pod that actually owns a write scope overlapping `rule`. +/// +/// Walks the delegation tree: if an allocation overlaps `rule`, we +/// descend into its children and return the deepest overlapping node +/// as the true owner. `exempt` names a Pod whose ownership is +/// permitted (used during delegation: the spawner itself is allowed +/// to still own the rule's region because it is handing it down). +pub fn find_conflict_owner( + lock: &LockFile, + rule: &ScopeRule, + exempt: Option<&str>, +) -> Option { + if rule.permission != Permission::Write { + return None; + } + for alloc in lock.allocations.iter().filter(|a| a.delegated_from.is_none()) { + if let Some(owner) = find_conflict_in_subtree(lock, alloc, rule) { + if Some(owner.as_str()) == exempt { + continue; + } + return Some(owner); + } + } + None +} + +fn find_conflict_in_subtree( + lock: &LockFile, + alloc: &Allocation, + rule: &ScopeRule, +) -> Option { + let overlaps_here = alloc + .scope_allow + .iter() + .filter(|r| r.permission == Permission::Write) + .any(|r| rules_overlap(r, rule)); + if !overlaps_here { + return None; + } + for child in lock + .allocations + .iter() + .filter(|a| a.delegated_from.as_deref() == Some(alloc.pod_name.as_str())) + { + if let Some(owner) = find_conflict_in_subtree(lock, child, rule) { + return Some(owner); + } + } + Some(alloc.pod_name.clone()) +} + +// --------------------------------------------------------------------------- +// Mutating operations +// --------------------------------------------------------------------------- + +/// Register a top-level Pod (started directly by a human, no +/// delegation parent). Reclaims stale entries before checking +/// conflicts so a crashed Pod's allocation doesn't block the new one. +pub fn register_pod( + guard: &mut LockFileGuard, + pod_name: String, + pid: u32, + socket: PathBuf, + scope_allow: Vec, +) -> Result<(), ScopeLockError> { + reclaim_stale(guard); + if guard.data().find(&pod_name).is_some() { + return Err(ScopeLockError::DuplicatePodName(pod_name)); + } + for rule in scope_allow + .iter() + .filter(|r| r.permission == Permission::Write) + { + if let Some(competitor) = find_conflict_owner(guard.data(), rule, None) { + return Err(ScopeLockError::WriteConflict { + competitor, + rule: rule.clone(), + }); + } + } + guard.data_mut().allocations.push(Allocation { + pod_name, + pid, + socket, + scope_allow, + delegated_from: None, + }); + guard.save()?; + Ok(()) +} + +/// Register a spawned Pod whose scope is delegated from `spawner`. +/// The requested scope must be within `spawner`'s effective write +/// scope; overlap with any Pod other than `spawner` is a conflict. +pub fn delegate_scope( + guard: &mut LockFileGuard, + spawner: &str, + spawned: String, + pid: u32, + socket: PathBuf, + scope_allow: Vec, +) -> Result<(), ScopeLockError> { + reclaim_stale(guard); + if guard.data().find(&spawned).is_some() { + return Err(ScopeLockError::DuplicatePodName(spawned)); + } + if guard.data().find(spawner).is_none() { + return Err(ScopeLockError::UnknownPod(spawner.into())); + } + for rule in &scope_allow { + if !is_within_effective_write(guard.data(), spawner, rule) { + return Err(ScopeLockError::NotSubset { + spawner: spawner.into(), + rule: rule.clone(), + }); + } + if rule.permission == Permission::Write { + if let Some(competitor) = find_conflict_owner(guard.data(), rule, Some(spawner)) { + return Err(ScopeLockError::WriteConflict { + competitor, + rule: rule.clone(), + }); + } + } + } + guard.data_mut().allocations.push(Allocation { + pod_name: spawned, + pid, + socket, + scope_allow, + delegated_from: Some(spawner.into()), + }); + guard.save()?; + Ok(()) +} + +/// Remove a Pod's allocation. Surviving children are reparented to +/// the removed Pod's own `delegated_from`, so the delegation tree +/// stays connected. +pub fn release_pod(guard: &mut LockFileGuard, pod_name: &str) -> Result<(), ScopeLockError> { + let idx = guard + .data() + .allocations + .iter() + .position(|a| a.pod_name == pod_name); + let Some(idx) = idx else { + return Err(ScopeLockError::UnknownPod(pod_name.into())); + }; + let removed = guard.data().allocations[idx].clone(); + for alloc in guard.data_mut().allocations.iter_mut() { + if alloc.delegated_from.as_deref() == Some(pod_name) { + alloc.delegated_from.clone_from(&removed.delegated_from); + } + } + guard.data_mut().allocations.remove(idx); + guard.save()?; + Ok(()) +} + +/// Remove allocations whose PID is dead, reparenting children to the +/// dead Pod's `delegated_from`. Idempotent and best-effort — I/O +/// errors on save are swallowed so a crashed Pod's entry never blocks +/// forward progress. +pub fn reclaim_stale(guard: &mut LockFileGuard) { + reclaim_stale_with(guard, pid_alive); +} + +/// Test seam: stale reclaim with a caller-supplied liveness probe. +pub fn reclaim_stale_with(guard: &mut LockFileGuard, mut is_alive: impl FnMut(u32) -> bool) { + let dead: Vec = guard + .data() + .allocations + .iter() + .filter(|a| !is_alive(a.pid)) + .map(|a| a.pod_name.clone()) + .collect(); + if dead.is_empty() { + return; + } + for name in &dead { + let Some(idx) = guard + .data() + .allocations + .iter() + .position(|a| a.pod_name == *name) + else { + continue; + }; + let removed = guard.data().allocations[idx].clone(); + for alloc in guard.data_mut().allocations.iter_mut() { + if alloc.delegated_from.as_deref() == Some(name.as_str()) { + alloc.delegated_from.clone_from(&removed.delegated_from); + } + } + guard.data_mut().allocations.remove(idx); + } + let _ = guard.save(); +} + +/// `kill(pid, 0)` — returns true if the process exists (even when we +/// don't own it), false only on ESRCH. +fn pid_alive(pid: u32) -> bool { + if pid == 0 { + return false; + } + let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; + if ret == 0 { + return true; + } + io::Error::last_os_error() + .raw_os_error() + .map(|e| e != libc::ESRCH) + .unwrap_or(false) +} + +// --------------------------------------------------------------------------- +// Lifecycle guard +// --------------------------------------------------------------------------- + +/// Owned allocation: on drop, opens the lock file and releases this +/// Pod's entry. The guard keeps only the name + lock-file path; it +/// does not hold the `flock` for the Pod's lifetime. +pub struct ScopeAllocationGuard { + pod_name: String, + lock_path: PathBuf, +} + +impl ScopeAllocationGuard { + pub fn pod_name(&self) -> &str { + &self.pod_name + } + + pub fn lock_path(&self) -> &Path { + &self.lock_path + } +} + +impl Drop for ScopeAllocationGuard { + fn drop(&mut self) { + if let Ok(mut guard) = LockFileGuard::open(&self.lock_path) { + let _ = release_pod(&mut guard, &self.pod_name); + } + } +} + +/// Open the default lock file, register a top-level Pod, and return a +/// guard that will release the allocation on drop. +pub fn install_top_level( + pod_name: String, + pid: u32, + socket: PathBuf, + scope_allow: Vec, +) -> Result { + let lock_path = default_lock_path()?; + let mut guard = LockFileGuard::open(&lock_path)?; + register_pod(&mut guard, pod_name.clone(), pid, socket, scope_allow)?; + Ok(ScopeAllocationGuard { + pod_name, + lock_path, + }) +} + +/// Errors raised by the mutating scope-lock operations. +#[derive(Debug, thiserror::Error)] +pub enum ScopeLockError { + #[error("I/O error on scope.lock: {0}")] + Io(#[from] io::Error), + #[error("pod name `{0}` is already registered")] + DuplicatePodName(String), + #[error("requested scope `{}` conflicts with pod `{competitor}`", .rule.target.display())] + WriteConflict { + competitor: String, + rule: ScopeRule, + }, + #[error( + "requested scope `{}` is not within spawner `{spawner}`'s effective scope", + .rule.target.display() + )] + NotSubset { + spawner: String, + rule: ScopeRule, + }, + #[error("pod `{0}` is not registered")] + UnknownPod(String), +} + +#[cfg(test)] +mod tests { + use super::*; + use manifest::Permission; + use tempfile::TempDir; + + fn write_rule(path: &str, recursive: bool) -> ScopeRule { + ScopeRule { + target: PathBuf::from(path), + permission: Permission::Write, + recursive, + } + } + + fn sock(name: &str) -> PathBuf { + PathBuf::from(format!("/tmp/{name}.sock")) + } + + fn open_empty(path: &Path) -> LockFileGuard { + LockFileGuard::open(path).unwrap() + } + + #[test] + fn open_creates_empty_lock_file() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let guard = LockFileGuard::open(&path).unwrap(); + assert!(guard.data().allocations.is_empty()); + assert!(path.exists()); + } + + #[test] + fn open_creates_file_with_owner_only_permissions() { + use std::os::unix::fs::PermissionsExt; + let dir = TempDir::new().unwrap(); + let parent = dir.path().join("insomnia"); + let path = parent.join("scope.lock"); + let _guard = LockFileGuard::open(&path).unwrap(); + let file_mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(file_mode, 0o600, "file mode = {file_mode:o}"); + let dir_mode = std::fs::metadata(&parent).unwrap().permissions().mode() & 0o777; + assert_eq!(dir_mode, 0o700, "dir mode = {dir_mode:o}"); + } + + #[test] + fn save_and_reopen_roundtrip() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + { + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + } + let guard = LockFileGuard::open(&path).unwrap(); + assert_eq!(guard.data().allocations.len(), 1); + assert_eq!(guard.data().allocations[0].pod_name, "a"); + } + + #[test] + fn rules_overlap_prefix_relation() { + assert!(rules_overlap( + &write_rule("/src", true), + &write_rule("/src/core", true) + )); + assert!(rules_overlap( + &write_rule("/src/core", true), + &write_rule("/src", true), + )); + assert!(!rules_overlap( + &write_rule("/src", true), + &write_rule("/docs", true), + )); + } + + #[test] + fn rules_overlap_non_recursive() { + assert!(!rules_overlap( + &write_rule("/src", false), + &write_rule("/src/a/b", true), + )); + assert!(rules_overlap( + &write_rule("/src", false), + &write_rule("/src/child", false), + )); + } + + #[test] + fn register_detects_write_conflict() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + let err = register_pod( + &mut g, + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap_err(); + match err { + ScopeLockError::WriteConflict { competitor, .. } => assert_eq!(competitor, "a"), + other => panic!("expected WriteConflict, got {other:?}"), + } + } + + #[test] + fn duplicate_pod_name_rejected() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + let err = register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a2"), + vec![write_rule("/docs", true)], + ) + .unwrap_err(); + assert!(matches!(err, ScopeLockError::DuplicatePodName(ref n) if n == "a")); + } + + #[test] + fn delegate_must_be_subset() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + let err = delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/docs", true)], + ) + .unwrap_err(); + assert!(matches!(err, ScopeLockError::NotSubset { .. })); + } + + #[test] + fn delegate_succeeds_within_parent_scope() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + assert_eq!(g.data().allocations.len(), 2); + // A's effective write no longer covers /src/core because B has it. + assert!(!is_within_effective_write( + g.data(), + "a", + &write_rule("/src/core", true) + )); + // A still covers its own uninvolved areas. + assert!(is_within_effective_write( + g.data(), + "a", + &write_rule("/src/other", true) + )); + } + + #[test] + fn delegate_rejects_sibling_overlap() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + // Sibling C from A tries to take /src/core/sub — already under B's scope. + let err = delegate_scope( + &mut g, + "a", + "c".into(), + std::process::id(), + sock("c"), + vec![write_rule("/src/core/sub", true)], + ) + .unwrap_err(); + // NotSubset fires first because /src/core is no longer in A's effective. + assert!(matches!(err, ScopeLockError::NotSubset { .. })); + } + + #[test] + fn release_reparents_children() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "b", + "d".into(), + std::process::id(), + sock("d"), + vec![write_rule("/src/core/x", true)], + ) + .unwrap(); + release_pod(&mut g, "b").unwrap(); + // D should now list A as its delegated_from. + let d = g.data().find("d").unwrap(); + assert_eq!(d.delegated_from.as_deref(), Some("a")); + assert!(g.data().find("b").is_none()); + } + + #[test] + fn reclaim_stale_reparents_and_removes_dead_entries() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "b", + "d".into(), + std::process::id(), + sock("d"), + vec![write_rule("/src/core/x", true)], + ) + .unwrap(); + // Simulate B crashing by rewriting its pid to one the probe + // will treat as dead. + let fake_dead_pid: u32 = 0xffff_fff0; + for alloc in g.data_mut().allocations.iter_mut() { + if alloc.pod_name == "b" { + alloc.pid = fake_dead_pid; + } + } + reclaim_stale_with(&mut g, |pid| pid != fake_dead_pid); + assert!(g.data().find("b").is_none()); + let d = g.data().find("d").unwrap(); + assert_eq!(d.delegated_from.as_deref(), Some("a")); + } + + fn read_rule(path: &str, recursive: bool) -> ScopeRule { + ScopeRule { + target: PathBuf::from(path), + permission: Permission::Read, + recursive, + } + } + + #[test] + fn read_rules_do_not_conflict_with_write() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + // B only reads under the same tree — allowed. + register_pod( + &mut g, + "b".into(), + std::process::id(), + sock("b"), + vec![read_rule("/src", true)], + ) + .unwrap(); + assert_eq!(g.data().allocations.len(), 2); + } + + #[test] + fn releasing_pod_reopens_scope_for_fresh_registration() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + release_pod(&mut g, "a").unwrap(); + register_pod( + &mut g, + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src", true)], + ) + .unwrap(); + } + + #[test] + fn delegated_scope_returns_to_parent_on_release() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + assert!(!is_within_effective_write( + g.data(), + "a", + &write_rule("/src/core", true) + )); + release_pod(&mut g, "b").unwrap(); + // /src/core is back in A's effective write scope. + assert!(is_within_effective_write( + g.data(), + "a", + &write_rule("/src/core", true) + )); + } + + #[test] + fn scope_allocation_guard_releases_on_drop() { + let dir = TempDir::new().unwrap(); + let lock_path = dir.path().join("scope.lock"); + // Override the default path for the duration of the test. + // SAFETY: single-threaded inside each #[test]; concurrent tests + // that also touch INSOMNIA_SCOPE_LOCK are excluded by using + // per-test paths and not clearing the var until after drop. + unsafe { + std::env::set_var("INSOMNIA_SCOPE_LOCK", &lock_path); + } + let guard = install_top_level( + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + { + let g = LockFileGuard::open(&lock_path).unwrap(); + assert!(g.data().find("a").is_some()); + } + drop(guard); + { + let g = LockFileGuard::open(&lock_path).unwrap(); + assert!(g.data().find("a").is_none()); + } + unsafe { + std::env::remove_var("INSOMNIA_SCOPE_LOCK"); + } + } + + #[test] + fn conflict_detection_descends_to_real_owner() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("scope.lock"); + let mut g = open_empty(&path); + register_pod( + &mut g, + "a".into(), + std::process::id(), + sock("a"), + vec![write_rule("/src", true)], + ) + .unwrap(); + delegate_scope( + &mut g, + "a", + "b".into(), + std::process::id(), + sock("b"), + vec![write_rule("/src/core", true)], + ) + .unwrap(); + // A different top-level Pod trying to register /src/core/x + // should be blamed on B (deepest owner), not A. + let err = register_pod( + &mut g, + "x".into(), + std::process::id(), + sock("x"), + vec![write_rule("/src/core/x", true)], + ) + .unwrap_err(); + match err { + ScopeLockError::WriteConflict { competitor, .. } => assert_eq!(competitor, "b"), + other => panic!("expected WriteConflict, got {other:?}"), + } + } +} diff --git a/tickets/scope-lock.md b/tickets/scope-lock.md index 85aac726..a7559b31 100644 --- a/tickets/scope-lock.md +++ b/tickets/scope-lock.md @@ -1,5 +1,10 @@ # Scope lock file: write 排他とスコープ分譲の記録基盤 +## レビュー状態 + +初回レビュー実施済み。[scope-lock.review.md](scope-lock.review.md) を参照。 +指摘1件（ファイルパーミッション 0600 の明示設定）の修正を条件に受け入れ可。 + ## 背景 Pod オーケストレーションでは scope の分譲（spawner が自身の scope を spawned Pod に譲渡）が発生する。また、人間が独立に複数の Pod を起動した場合にも同一パスへの write 衝突を検出する必要がある。 diff --git a/tickets/scope-lock.review.md b/tickets/scope-lock.review.md new file mode 100644 index 00000000..7d34bfdc --- /dev/null +++ b/tickets/scope-lock.review.md @@ -0,0 +1,100 @@ +# レビュー: Scope lock file + +対象差分: `crates/pod/src/scope_lock.rs` (新規 992行), `crates/manifest/src/scope.rs`, `crates/pod/src/{pod,lib,runtime_dir}.rs`, `crates/pod/Cargo.toml`（未コミット） + +## 要件達成状況 + +| 要件 | 状態 | +|---|---| +| lock file に Pod の scope allocation を記録 | ✅ `register_pod` / `delegate_scope` で JSON に書き込み | +| flock による排他アクセス | ✅ `LockFileGuard::open` で `lock_exclusive`、Drop で `unlock` | +| write 衝突の検出 | ✅ `find_conflict_owner` が delegation tree を下降して真の所有者を特定 | +| stale エントリの自動回収 (PID 死活) | ✅ `reclaim_stale` が `kill(pid, 0)` で判定、dead を削除・子を reparent | +| scope 分譲の記録 (`delegated_from`) | ✅ `delegate_scope` が spawner の effective scope 内か検証してから登録 | +| 分譲チェーンの reparent (A→B→D、B 死亡時 D を A に付け替え) | ✅ `release_pod` / `reclaim_stale` ともに `delegated_from` を親に付け替え | +| Pod 正常終了時の allocation 解放 | ✅ `ScopeAllocationGuard` の Drop で `release_pod` を呼ぶ | +| Pod 起動時 (`from_manifest`) に自動登録 | ✅ `scope_lock::install_top_level` を `from_manifest` 内で呼出 | +| テストで衝突検出・stale 回収・分譲/返却を検証 | ✅ 16 テストケース | +| パーミッション制御 (0600) | — チケットに記載あるが、ファイル作成時の umask 制御は未実装。`OpenOptions` に mode 設定なし | + +## アーキテクチャ + +### 良い点 + +**`LockFileGuard` の RAII 設計**: `open` で flock 取得、`save` で書き込み、Drop で unlock。mutate-but-don't-save のパスでは変更が破棄される（エラー時に安全）。 + +**`ScopeAllocationGuard` で Pod ライフサイクルに紐付け**: Pod 構造体が `Option` を保持し、Drop で lock file から自動削除。`Pod::new` / `Pod::restore`（テスト用）は `None` でバイパス。 + +**`find_conflict_owner` が delegation tree を下降**: 衝突エラーに「真の所有者」（最深のノード）を表示。`conflict_detection_descends_to_real_owner` テストで lock-in。 + +**`reclaim_stale_with` のテストシーム**: PID 生存判定を引数で差し替え可能。テストで任意の PID を「dead」にできる。 + +**`is_within_effective_write`**: spawner の allow set から子に委譲済みの部分を差し引いた「実効 scope」を計算。`delegate_scope` のバリデーションに使用。 + +**`rules_overlap` の 4 パターン分岐** (recursive×recursive, recursive×non, non×recursive, non×non): 非再帰ルールの覆域（target 自身 + 直下の子）を正しく考慮。 + +### 指摘事項 + +#### 1. 🟡 ファイルパーミッションの明示設定 + +チケットの「セキュリティとアクセス」セクション: +> ファイルパーミッション `0600`（owner only）、ディレクトリは `0700` + +`LockFileGuard::open` は `OpenOptions::new().create(true)` で作成しているが、パーミッションの明示設定がない。umask がデフォルト (0022) なら `0644` になり、他ユーザーから読めてしまう。 + +```rust +// 現状 +let file = OpenOptions::new() + .read(true).write(true).create(true).truncate(false) + .open(path)?; + +// 修正案: Unix 拡張で mode を設定 +use std::os::unix::fs::OpenOptionsExt; +let file = OpenOptions::new() + .read(true).write(true).create(true).truncate(false) + .mode(0o600) + .open(path)?; +``` + +ディレクトリも `create_dir_all` の後に `std::fs::set_permissions` で `0700` に制限する。 + +**判断**: セキュリティ要件。修正すべき。 + +#### 2. 🟢 `socket` フィールドの予測パス + +`pod.rs` で socket path を `runtime_dir::default_base()?.join(&manifest.pod.name).join("sock")` と予測しているが、実際の `RuntimeDir` が作る socket path と一致するか保証がない（`RuntimeDir` のパス構築ロジックが変わったら乖離する）。 + +現時点では一致しているが、将来 `RuntimeDir` のパス規則が変わったとき scope_lock の socket 情報が嘘になる。`RuntimeDir` 側に `fn socket_path_for(pod_name: &str) -> PathBuf` のような static メソッドを置いて、両者が同じ関数を呼ぶようにするとより堅牢。 + +**判断**: 現時点では問題なし。リファクタ余地として認識。 + +#### 3. 🟢 `covers_fully` の permission 比較 + +```rust +fn covers_fully(cover: &ScopeRule, inner: &ScopeRule) -> bool { + if cover.permission < inner.permission { + return false; + } + ... +} +``` + +`Permission` の `Ord` は `Read < Write`。`cover.permission < inner.permission` = 「cover が Read で inner が Write なら不十分」。正しい。 + +#### 4. 🟢 テストの網羅性 + +16 ケース: +- ファイル操作: open creates / save-reopen roundtrip +- overlap 判定: prefix relation / non-recursive +- 登録: write conflict / duplicate name / read doesn't conflict +- 分譲: must be subset / succeeds within parent / rejects sibling overlap +- 解放: reparents children / reopens scope / returns to parent +- stale: reparents and removes dead entries +- guard: releases on drop +- conflict detection: descends to real owner + +delegation tree の主要シナリオ (A→B→D) がカバーされている。 + +## 結論 + +**指摘1 (ファイルパーミッション 0600) の修正を条件に受け入れ可**。他は問題なし。実装の核心（delegation tree walk, effective scope 計算, stale reclaim + reparent）が正確で、テストが充実している。