From 130ed43300fad7ba0ff4d5211607483218edf8c8 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:18:38 +0200 Subject: [PATCH 01/10] opencode: a cross-tenant manifest writer lock, and the eviction race it hides Stale evictors could observe S0, then rename a replacement holder's fresh lock because each chose a fresh quarantine suffix.\n\nQuarantine targets now derive from the observed owner's nonce, so racers collide on one occupied target and retry after EEXIST/ENOTEMPTY.\n\nThe Rust and TypeScript implementations share the frozen TTL, renewal, owner-record, retry, release, and tenant-preservation contract. --- .../src/bin/cli_support/opencode_files.rs | 691 +++++++++++++++++- packages/client/README.md | 7 + packages/client/src/handles.ts | 223 ++++++ packages/client/src/index.ts | 20 + packages/client/src/manifest-lock.ts | 105 +++ packages/client/src/tests/handles.test.ts | 131 ++++ .../client/src/tests/manifest-lock.test.ts | 314 ++++++++ packages/opencode/src/handles.ts | 243 +----- 8 files changed, 1513 insertions(+), 221 deletions(-) create mode 100644 packages/client/src/handles.ts create mode 100644 packages/client/src/manifest-lock.ts create mode 100644 packages/client/src/tests/handles.test.ts create mode 100644 packages/client/src/tests/manifest-lock.test.ts diff --git a/crates/credentials-module/src/bin/cli_support/opencode_files.rs b/crates/credentials-module/src/bin/cli_support/opencode_files.rs index 09c6cc1..4138996 100644 --- a/crates/credentials-module/src/bin/cli_support/opencode_files.rs +++ b/crates/credentials-module/src/bin/cli_support/opencode_files.rs @@ -4,15 +4,111 @@ use std::{ fs::{self, File, OpenOptions}, io::Write, path::{Path, PathBuf}, - sync::atomic::{AtomicU64, Ordering}, + sync::{ + atomic::{AtomicBool, AtomicU64, Ordering}, + mpsc, Arc, + }, + thread, + time::{Duration, Instant, SystemTime, UNIX_EPOCH}, }; +use ring::rand::{SecureRandom, SystemRandom}; use serde::{Deserialize, Serialize}; use serde_json::Value; static TEMP_SEQ: AtomicU64 = AtomicU64::new(0); const AUTH_FILE_MAX_BYTES: u64 = 1024 * 1024; const HANDLE_FILE_MAX_BYTES: u64 = 256 * 1024; +const MANIFEST_LOCK_TTL_MS: u64 = 30_000; +const MANIFEST_LOCK_RENEW_EVERY_MS: u64 = 10_000; +const MANIFEST_LOCK_OWNER_KEYS: [&str; 4] = ["tenant", "pid", "claimed_at_ms", "nonce"]; +const MANIFEST_LOCK_STALE_TARGET_PATTERN: &str = r"^\.lock\.stale-\d+-[A-Za-z0-9_-]+$"; +const OPENCODE_CLAUSTRUM_TENANT: &str = "opencode-claustrum"; +type BeforeManifestRename = Arc; + +#[cfg(test)] +static LEASE_LOST_WARNINGS: AtomicU64 = AtomicU64::new(0); + +#[derive(Clone)] +struct ManifestLockOptions { + ttl: Duration, + renew_every: Duration, + retry_min: Duration, + retry_max: Duration, + after_claim: Option>, + before_evict: Option>, + #[cfg(test)] + after_evict_rename_attempt: Option>, + after_evict: Option>, + before_manifest_rename: Option, + // A fixed clock isolates stale-owner comparisons from host scheduling; claim + // deadline expiry remains monotonic so the production bound is still exercised. + now_override_ms: Option, +} + +impl Default for ManifestLockOptions { + fn default() -> Self { + Self { + ttl: Duration::from_millis(MANIFEST_LOCK_TTL_MS), + renew_every: Duration::from_millis(MANIFEST_LOCK_RENEW_EVERY_MS), + retry_min: Duration::from_millis(25), + retry_max: Duration::from_millis(75), + after_claim: None, + before_evict: None, + #[cfg(test)] + after_evict_rename_attempt: None, + after_evict: None, + before_manifest_rename: None, + now_override_ms: None, + } + } +} + +struct ManifestLease { + lock: PathBuf, + nonce: String, + ttl: Duration, + renewal_failed: Arc, + stop_tx: Option>, + renewal: Option>, +} + +impl ManifestLease { + fn stop_renewal(&mut self) { + if let Some(stop_tx) = self.stop_tx.take() { + let _ = stop_tx.send(()); + } + if let Some(renewal) = self.renewal.take() { + let _ = renewal.join(); + } + } + + fn commit(&mut self) -> Result<(), OpenCodeFilesError> { + self.stop_renewal(); + let owner = read_lock_owner(&self.lock.join("owner")).ok(); + let ours_and_fresh = owner.is_some_and(|owner| { + owner.nonce == self.nonce + && current_time_ms().is_ok_and(|now| { + now.saturating_sub(owner.claimed_at_ms) < self.ttl.as_millis() as u64 + }) + }); + if self.renewal_failed.load(Ordering::SeqCst) || !ours_and_fresh { + return Err(OpenCodeFilesError::Invalid( + "manifest lock renewal failed; write aborted".into(), + )); + } + Ok(()) + } +} + +#[derive(Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct ManifestLockOwner { + tenant: String, + pid: u32, + claimed_at_ms: u64, + nonce: String, +} #[derive(Debug)] pub enum OpenCodeFilesError { @@ -226,20 +322,410 @@ pub fn read_handle_file(path: &Path) -> Result { } pub fn write_handle_file(path: &Path, file: &HandleFile) -> Result<(), OpenCodeFilesError> { - validate_handle_file(file)?; - let bytes = serde_json::to_vec(file).map_err(OpenCodeFilesError::Json)?; - write_atomic(path, &bytes, true) + write_handle_file_for_tenant( + path, + OPENCODE_CLAUSTRUM_TENANT, + file, + ManifestLockOptions::default(), + ) } pub fn verify_handle_written(path: &Path, expected: &HandleFile) -> Result<(), OpenCodeFilesError> { - if &read_handle_file(path)? != expected { + validate_handle_file(expected)?; + let written = read_handle_file(path)?; + let expected_owned: Vec<_> = expected + .providers + .iter() + .filter(|provider| provider.serve == OPENCODE_CLAUSTRUM_TENANT) + .cloned() + .collect(); + let written_owned: Vec<_> = written + .providers + .iter() + .filter(|provider| provider.serve == OPENCODE_CLAUSTRUM_TENANT) + .cloned() + .collect(); + if written_owned != expected_owned { return Err(OpenCodeFilesError::Invalid( - "handle file did not persist exactly".into(), + "handle file tenant block did not persist exactly".into(), )); } Ok(()) } +fn write_handle_file_for_tenant( + path: &Path, + tenant: &str, + desired: &HandleFile, + options: ManifestLockOptions, +) -> Result<(), OpenCodeFilesError> { + validate_handle_file(desired)?; + let parent = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .ok_or_else(|| OpenCodeFilesError::Invalid("file path has no parent".into()))?; + fs::create_dir_all(parent).map_err(|source| io_error("create parent directory", source))?; + validate_secure_parent(parent)?; + set_mode(parent, 0o700)?; + let before_manifest_rename = options.before_manifest_rename.clone(); + with_manifest_lock_with_options(path, tenant, options, |lease| { + let current = match fs::symlink_metadata(path) { + Ok(_) => read_handle_file(path)?, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => HandleFile { + version: 1, + providers: Vec::new(), + }, + Err(error) => return Err(io_error("stat handle file", error)), + }; + let before_foreign: Vec> = current + .providers + .iter() + .filter(|provider| provider.serve != tenant) + .map(serde_json::to_vec) + .collect::>() + .map_err(OpenCodeFilesError::Json)?; + let mut providers: Vec<_> = current + .providers + .into_iter() + .filter(|provider| provider.serve != tenant) + .collect(); + providers.extend( + desired + .providers + .iter() + .filter(|provider| provider.serve == tenant) + .cloned(), + ); + let next = HandleFile { + version: 1, + providers, + }; + validate_handle_file(&next)?; + let bytes = serde_json::to_vec(&next).map_err(OpenCodeFilesError::Json)?; + write_atomic_guarded(path, &bytes, true, || { + if let Some(before_manifest_rename) = &before_manifest_rename { + before_manifest_rename(&lock_path(path)); + } + lease.commit() + })?; + let readback = read_handle_file(path)?; + if readback != next { + return Err(OpenCodeFilesError::Invalid( + "handle file readback did not persist exactly".into(), + )); + } + let after_foreign: Vec> = readback + .providers + .iter() + .filter(|provider| provider.serve != tenant) + .map(serde_json::to_vec) + .collect::>() + .map_err(OpenCodeFilesError::Json)?; + if after_foreign != before_foreign { + return Err(OpenCodeFilesError::Invalid( + "handle file readback changed another tenant block".into(), + )); + } + Ok(()) + }) +} + +fn lock_path(path: &Path) -> PathBuf { + let mut lock = path.as_os_str().to_os_string(); + lock.push(".lock"); + PathBuf::from(lock) +} + +fn current_time_ms() -> Result { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|duration| duration.as_millis() as u64) + .map_err(|_| OpenCodeFilesError::Invalid("system clock is before UNIX epoch".into())) +} + +fn resolve_now_ms(options: &ManifestLockOptions) -> Result { + match options.now_override_ms { + Some(fixed) => Ok(fixed), + None => current_time_ms(), + } +} + +fn random_nonce() -> Result { + let mut bytes = [0_u8; 16]; + SystemRandom::new() + .fill(&mut bytes) + .map_err(|_| OpenCodeFilesError::Invalid("generate manifest lock nonce failed".into()))?; + Ok(bytes.iter().map(|byte| format!("{byte:02x}")).collect()) +} + +fn io_error(action: &'static str, source: std::io::Error) -> OpenCodeFilesError { + OpenCodeFilesError::Io { action, source } +} + +fn read_lock_owner(path: &Path) -> Result { + let source = + fs::read_to_string(path).map_err(|source| io_error("read manifest lock owner", source))?; + serde_json::from_str(&source).map_err(OpenCodeFilesError::Json) +} + +fn write_lock_owner(lock: &Path, owner: &ManifestLockOwner) -> Result<(), OpenCodeFilesError> { + let owner_path = lock.join("owner"); + let temporary = lock.join(format!( + "owner.{}.{}.tmp", + std::process::id(), + random_nonce()? + )); + let result = (|| -> Result<(), OpenCodeFilesError> { + #[cfg(unix)] + let mut file = { + use std::os::unix::fs::OpenOptionsExt; + OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&temporary) + .map_err(|source| io_error("create manifest lock owner", source))? + }; + #[cfg(not(unix))] + let mut file = OpenOptions::new() + .write(true) + .create_new(true) + .open(&temporary) + .map_err(|source| io_error("create manifest lock owner", source))?; + set_mode(&temporary, 0o600)?; + serde_json::to_writer(&mut file, owner).map_err(OpenCodeFilesError::Json)?; + file.write_all(b"\n") + .map_err(|source| io_error("write manifest lock owner", source))?; + file.sync_all() + .map_err(|source| io_error("sync manifest lock owner", source))?; + drop(file); + fs::rename(&temporary, &owner_path) + .map_err(|source| io_error("rename manifest lock owner", source)) + })(); + if result.is_err() { + let _ = fs::remove_file(&temporary); + } + result +} + +fn stale_target_matches(value: &str) -> bool { + let Some(rest) = value.strip_prefix(".lock.stale-") else { + return false; + }; + let Some((claimed, random)) = rest.split_once('-') else { + return false; + }; + !claimed.is_empty() + && claimed.bytes().all(|byte| byte.is_ascii_digit()) + && !random.is_empty() + && random + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-')) +} + +fn warn_lease_lost(path: &Path) { + #[cfg(test)] + LEASE_LOST_WARNINGS.fetch_add(1, Ordering::SeqCst); + eprintln!( + "manifest lock lease lost, not releasing: {}", + path.display() + ); +} + +fn jitter(options: &ManifestLockOptions) -> Duration { + let min = options.retry_min.as_millis() as u64; + let max = options.retry_max.as_millis() as u64; + if max <= min { + return Duration::from_millis(min); + } + let mut bytes = [0_u8; 8]; + if SystemRandom::new().fill(&mut bytes).is_err() { + return Duration::from_millis(min); + } + Duration::from_millis(min + u64::from_le_bytes(bytes) % (max - min + 1)) +} + +fn release_manifest_lock( + path: &Path, + lock: &Path, + nonce: &str, + ttl: Duration, +) -> Result<(), OpenCodeFilesError> { + let owner = match read_lock_owner(&lock.join("owner")) { + Ok(owner) => owner, + Err(_) => { + warn_lease_lost(path); + return Ok(()); + } + }; + let now = current_time_ms()?; + if owner.nonce != nonce || now.saturating_sub(owner.claimed_at_ms) >= ttl.as_millis() as u64 { + warn_lease_lost(path); + return Ok(()); + } + let release = PathBuf::from(format!("{}.release-{nonce}", lock.display())); + if fs::rename(lock, &release).is_err() { + warn_lease_lost(path); + return Ok(()); + } + let moved = read_lock_owner(&release.join("owner")).ok(); + let moved_is_ours = moved.is_some_and(|owner| { + owner.nonce == nonce + && current_time_ms() + .is_ok_and(|now| now.saturating_sub(owner.claimed_at_ms) < ttl.as_millis() as u64) + }); + if !moved_is_ours { + let _ = fs::rename(&release, lock); + warn_lease_lost(path); + return Ok(()); + } + fs::remove_dir_all(&release).map_err(|source| io_error("remove manifest lock", source)) +} + +fn with_manifest_lock_with_options( + path: &Path, + tenant: &str, + options: ManifestLockOptions, + operation: F, +) -> Result +where + F: FnOnce(&mut ManifestLease) -> Result, +{ + let lock = lock_path(path); + let owner_path = lock.join("owner"); + let nonce = random_nonce()?; + let started_at_ms = resolve_now_ms(&options)?; + let deadline = Instant::now() + options.ttl; + loop { + match fs::create_dir(&lock) { + Ok(()) => { + set_mode(&lock, 0o700)?; + let owner = ManifestLockOwner { + tenant: tenant.into(), + pid: std::process::id(), + claimed_at_ms: resolve_now_ms(&options)?, + nonce: nonce.clone(), + }; + if let Err(error) = write_lock_owner(&lock, &owner) { + let _ = fs::remove_dir_all(&lock); + return Err(error); + } + if let Some(after_claim) = &options.after_claim { + after_claim(); + } + break; + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => return Err(io_error("create manifest lock", error)), + } + + if let Ok(observed) = read_lock_owner(&owner_path) { + if started_at_ms.saturating_sub(observed.claimed_at_ms) + >= options.ttl.as_millis() as u64 + { + if let Some(before_evict) = &options.before_evict { + before_evict(); + } + let stale = PathBuf::from(format!( + "{}.stale-{}-{}", + lock.display(), + observed.claimed_at_ms, + observed.nonce + )); + let rename_result = fs::rename(&lock, &stale); + #[cfg(test)] + if let Some(after_evict_rename_attempt) = &options.after_evict_rename_attempt { + after_evict_rename_attempt(); + } + match rename_result { + Ok(()) => { + let moved = read_lock_owner(&stale.join("owner")).ok(); + if moved.is_some_and(|owner| { + owner.nonce == observed.nonce + && owner.claimed_at_ms == observed.claimed_at_ms + }) { + if let Some(after_evict) = &options.after_evict { + after_evict(); + } + continue; + } + let _ = fs::rename(&stale, &lock); + } + Err(error) + if matches!( + error.kind(), + std::io::ErrorKind::NotFound + | std::io::ErrorKind::AlreadyExists + | std::io::ErrorKind::DirectoryNotEmpty + ) => {} + Err(error) => return Err(io_error("rename stale manifest lock", error)), + } + } + } + if Instant::now() >= deadline { + return Err(OpenCodeFilesError::Invalid("manifest lock busy".into())); + } + thread::sleep(jitter(&options).min(deadline.saturating_duration_since(Instant::now()))); + } + + let (stop_tx, stop_rx) = mpsc::channel::<()>(); + let renewal_lock = lock.clone(); + let renewal_nonce = nonce.clone(); + let renewal_ttl = options.ttl; + let renewal_every = options.renew_every; + let renewal_failed = Arc::new(AtomicBool::new(false)); + let renewal_failed_thread = Arc::clone(&renewal_failed); + let renewal = thread::spawn(move || loop { + match stop_rx.recv_timeout(renewal_every) { + Ok(()) | Err(mpsc::RecvTimeoutError::Disconnected) => break, + Err(mpsc::RecvTimeoutError::Timeout) => { + let owner_path = renewal_lock.join("owner"); + let Ok(mut owner) = read_lock_owner(&owner_path) else { + renewal_failed_thread.store(true, Ordering::SeqCst); + break; + }; + let Ok(now) = current_time_ms() else { + renewal_failed_thread.store(true, Ordering::SeqCst); + break; + }; + if owner.nonce != renewal_nonce + || now.saturating_sub(owner.claimed_at_ms) >= renewal_ttl.as_millis() as u64 + { + renewal_failed_thread.store(true, Ordering::SeqCst); + break; + } + owner.claimed_at_ms = now; + if write_lock_owner(&renewal_lock, &owner).is_err() { + renewal_failed_thread.store(true, Ordering::SeqCst); + break; + } + } + } + }); + let mut lease = ManifestLease { + lock: lock.clone(), + nonce: nonce.clone(), + ttl: options.ttl, + renewal_failed, + stop_tx: Some(stop_tx), + renewal: Some(renewal), + }; + let result = operation(&mut lease); + lease.stop_renewal(); + let result = match result { + Ok(_) if lease.renewal_failed.load(Ordering::SeqCst) => Err(OpenCodeFilesError::Invalid( + "manifest lock renewal failed; write aborted".into(), + )), + other => other, + }; + let release = release_manifest_lock(path, &lock, &nonce, options.ttl); + match (result, release) { + (Err(error), _) => Err(error), + (Ok(_), Err(error)) => Err(error), + (Ok(value), Ok(())) => Ok(value), + } +} + fn validate_auth_entry(entry: &Value) -> Result<(), OpenCodeFilesError> { let object = entry .as_object() @@ -343,6 +829,18 @@ fn validate_identifier(value: &str, kind: &str) -> Result<(), OpenCodeFilesError } fn write_atomic(path: &Path, bytes: &[u8], secure_parent: bool) -> Result<(), OpenCodeFilesError> { + write_atomic_guarded(path, bytes, secure_parent, || Ok(())) +} + +fn write_atomic_guarded( + path: &Path, + bytes: &[u8], + secure_parent: bool, + before_rename: F, +) -> Result<(), OpenCodeFilesError> +where + F: FnOnce() -> Result<(), OpenCodeFilesError>, +{ let parent = path .parent() .filter(|parent| !parent.as_os_str().is_empty()) @@ -397,6 +895,7 @@ fn write_atomic(path: &Path, bytes: &[u8], secure_parent: bool) -> Result<(), Op action: "sync temporary file", source, })?; + before_rename()?; fs::rename(&temp, path).map_err(|source| OpenCodeFilesError::Io { action: "rename temporary file", source, @@ -533,3 +1032,183 @@ fn current_uid() -> Result { .map_err(|_| OpenCodeFilesError::Invalid("current uid was invalid".into())) }) } + +#[cfg(test)] +mod manifest_lock_aba_regression { + use super::*; + use std::{ + os::unix::fs::PermissionsExt, + sync::{Arc, Barrier}, + thread, + time::{Duration, SystemTime, UNIX_EPOCH}, + }; + + fn now_ms() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_millis() as u64 + } + + #[test] + fn aba_observation_cannot_rename_a_replacement_lock() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-aba-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let lock = lock_path(&path); + let now = now_ms(); + fs::create_dir(&lock).unwrap(); + fs::set_permissions(&lock, fs::Permissions::from_mode(0o700)).unwrap(); + fs::write( + lock.join("owner"), + format!( + "{{\"tenant\":\"other-tenant\",\"pid\":41,\"claimed_at_ms\":{},\"nonce\":\"0123456789abcdef0123456789abcdef\"}}\n", + now - 501 + ), + ) + .unwrap(); + fs::set_permissions(lock.join("owner"), fs::Permissions::from_mode(0o600)).unwrap(); + + let loser_observed = Arc::new(Barrier::new(2)); + let allow_loser_rename = Arc::new(Barrier::new(2)); + let replacement_claimed = Arc::new(Barrier::new(2)); + let allow_replacement_release = Arc::new(Barrier::new(2)); + let rename_attempted = Arc::new(Barrier::new(2)); + let allow_attempt_completion = Arc::new(Barrier::new(2)); + + let loser_path = path.clone(); + let loser = thread::spawn({ + let loser_observed = Arc::clone(&loser_observed); + let allow_loser_rename = Arc::clone(&allow_loser_rename); + let rename_attempted = Arc::clone(&rename_attempted); + let allow_attempt_completion = Arc::clone(&allow_attempt_completion); + move || { + with_manifest_lock_with_options( + &loser_path, + "loser", + ManifestLockOptions { + ttl: Duration::from_millis(500), + renew_every: Duration::from_secs(1), + retry_min: Duration::from_millis(2), + retry_max: Duration::from_millis(3), + before_evict: Some(Arc::new(move || { + loser_observed.wait(); + allow_loser_rename.wait(); + })), + after_evict_rename_attempt: Some(Arc::new(move || { + rename_attempted.wait(); + allow_attempt_completion.wait(); + })), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ) + } + }); + + loser_observed.wait(); + let replacement_path = path.clone(); + let replacement = thread::spawn({ + let replacement_claimed = Arc::clone(&replacement_claimed); + let allow_replacement_release = Arc::clone(&allow_replacement_release); + move || { + with_manifest_lock_with_options( + &replacement_path, + "replacement", + ManifestLockOptions { + ttl: Duration::from_millis(500), + renew_every: Duration::from_secs(1), + retry_min: Duration::from_millis(2), + retry_max: Duration::from_millis(3), + after_claim: Some(Arc::new(move || { + replacement_claimed.wait(); + allow_replacement_release.wait(); + })), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ) + } + }); + + replacement_claimed.wait(); + allow_loser_rename.wait(); + rename_attempted.wait(); + allow_replacement_release.wait(); + replacement.join().unwrap().unwrap(); + allow_attempt_completion.wait(); + loser.join().unwrap().unwrap(); + assert!(!lock.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn two_stale_evictors_create_exactly_one_quarantine_directory() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-quarantine-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let lock = lock_path(&path); + let now = now_ms(); + fs::create_dir(&lock).unwrap(); + fs::set_permissions(&lock, fs::Permissions::from_mode(0o700)).unwrap(); + fs::write( + lock.join("owner"), + format!( + "{{\"tenant\":\"other-tenant\",\"pid\":41,\"claimed_at_ms\":{},\"nonce\":\"0123456789abcdef0123456789abcdef\"}}\n", + now - 501 + ), + ) + .unwrap(); + fs::set_permissions(lock.join("owner"), fs::Permissions::from_mode(0o600)).unwrap(); + let ready = Arc::new(Barrier::new(2)); + let evictions = Arc::new(AtomicU64::new(0)); + let mut joins = Vec::new(); + for tenant in ["anthropic-auth", "openai-auth"] { + let path = path.clone(); + let ready = Arc::clone(&ready); + let evictions = Arc::clone(&evictions); + joins.push(thread::spawn(move || { + with_manifest_lock_with_options( + &path, + tenant, + ManifestLockOptions { + ttl: Duration::from_millis(500), + renew_every: Duration::from_secs(1), + retry_min: Duration::from_millis(2), + retry_max: Duration::from_millis(3), + before_evict: Some(Arc::new(move || { + ready.wait(); + })), + after_evict: Some(Arc::new(move || { + evictions.fetch_add(1, Ordering::SeqCst); + })), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ) + })); + } + for join in joins { + join.join().unwrap().unwrap(); + } + let stale = fs::read_dir(&root) + .unwrap() + .filter_map(Result::ok) + .filter(|entry| entry.file_name().to_string_lossy().contains(".lock.stale-")) + .count(); + assert_eq!(evictions.load(Ordering::SeqCst), 1); + assert_eq!(stale, 1); + let _ = fs::remove_dir_all(root); + } +} diff --git a/packages/client/README.md b/packages/client/README.md index 0ecdcfb..3ad6b97 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -21,3 +21,10 @@ credential caching, refresh, account scheduling, and retry policy. The client sends `consumerIdentity: null` for every managed request so inherited `SUBC_MODULE_ID` and `SUBC_LAUNCH_NONCE` cannot impersonate a supervising host. + +## OpenCode handle-file exports + +`defaultHandleFilePath`, `readHandleFile`, `handleFileRevision`, and `parseHandleFile` +are exported for tenant plugins consuming the OpenCode handle manifest. The shared +`HANDLE_FILE_CONTRACT` is pinned to `maxBytes: 262144`, `mode: 0o600`, +`labelRe: /^[a-z0-9][a-z0-9._-]{0,63}$/`, and `handleRe: /^ckh_[A-Za-z0-9_-]{43}$/`. diff --git a/packages/client/src/handles.ts b/packages/client/src/handles.ts new file mode 100644 index 0000000..836e93b --- /dev/null +++ b/packages/client/src/handles.ts @@ -0,0 +1,223 @@ +import { createHash } from 'node:crypto' +import { constants } from 'node:fs' +import { lstat as nodeLstat, open as nodeOpen, readFile, stat as nodeStat } from 'node:fs/promises' +import { userInfo } from 'node:os' +import { dirname, join } from 'node:path' + +export const HANDLE_FILE_CONTRACT = { + maxBytes: 256 * 1024, + mode: 0o600, + labelRe: /^[a-z0-9][a-z0-9._-]{0,63}$/, + handleRe: /^ckh_[A-Za-z0-9_-]{43}$/, +} as const + +const FORBIDDEN_IDENTIFIERS = new Set(['__proto__', 'constructor', 'prototype']) + +export class HandleFileValidationError extends Error { + override name = 'HandleFileValidationError' +} + +export type HandleAccount = { + label: string + handle: string + credential_id: string + superseded?: string[] +} +export type HandleProvider = { + provider: string + shape: 'api' | 'oauth' + serve: string + accounts: HandleAccount[] +} +export type OpenCodeHandleFileV1 = { version: 1; providers: HandleProvider[] } + +function isAccount(value: unknown): value is HandleAccount { + if (!value || typeof value !== 'object') return false + const account = value as Record + return typeof account.label === 'string' && typeof account.handle === 'string' && + typeof account.credential_id === 'string' && + (account.superseded === undefined || + (Array.isArray(account.superseded) && account.superseded.every((handle) => typeof handle === 'string'))) +} + +function handleIsValid(handle: unknown): handle is string { + return typeof handle === 'string' && HANDLE_FILE_CONTRACT.handleRe.test(handle) +} + +function identifierIsValid(value: unknown): value is string { + return typeof value === 'string' && HANDLE_FILE_CONTRACT.labelRe.test(value) && !FORBIDDEN_IDENTIFIERS.has(value) +} + +function invalid(message: string): never { + throw new HandleFileValidationError(message) +} + +export function parseHandleFile(value: unknown): OpenCodeHandleFileV1 { + if (!value || typeof value !== 'object') invalid('handle file must be an object') + const file = value as Record + if (file.version !== 1 || !Array.isArray(file.providers)) { + invalid('handle file must have version 1 and providers') + } + const providerIds = new Set() + const providers = file.providers.map((provider, index): HandleProvider => { + if (!provider || typeof provider !== 'object') invalid(`provider ${index} must be an object`) + const item = provider as Record + if (!identifierIsValid(item.provider)) invalid(`provider ${index} has invalid provider`) + if (providerIds.has(item.provider)) invalid(`provider ${index} duplicates provider ${item.provider}`) + providerIds.add(item.provider) + if (item.shape !== 'api' && item.shape !== 'oauth') invalid(`provider ${index} has invalid shape`) + if (typeof item.serve !== 'string' || !item.serve) invalid(`provider ${index} requires serve`) + if (!Array.isArray(item.accounts) || item.accounts.length === 0 || !item.accounts.every(isAccount)) { + invalid(`provider ${index} has invalid accounts`) + } + const labels = new Set() + for (const account of item.accounts) { + if (!identifierIsValid(account.label)) invalid(`provider ${index} has an invalid account label`) + if (labels.has(account.label)) invalid(`provider ${index} duplicates account label ${account.label}`) + labels.add(account.label) + if (!handleIsValid(account.handle)) invalid(`provider ${index} account ${account.label} has invalid handle`) + if (!account.credential_id) invalid(`provider ${index} account ${account.label} has invalid credential id`) + if (account.superseded?.some((handle) => !handleIsValid(handle))) { + invalid(`provider ${index} account ${account.label} has invalid superseded handle`) + } + } + return { + provider: item.provider, + shape: item.shape, + serve: item.serve, + accounts: item.accounts.map((account) => ({ + ...account, + ...(account.superseded === undefined ? {} : { superseded: account.superseded }), + })), + } + }) + return { version: 1, providers } +} + +type HandleFileStat = { + isFile(): boolean + isDirectory?(): boolean + isSymbolicLink?(): boolean + mode: number + size?: number + uid?: number + mtimeMs?: number +} +type HandleFileDescriptor = { + read?(buffer: Uint8Array, offset: number, length: number, position: number): Promise<{ bytesRead: number }> | { bytesRead: number } + stat(): Promise + readFile(options: { encoding: 'utf8' }): Promise + close(): Promise +} +export type HandleFileIo = { + stat?: (path: string) => Promise + lstat?: (path: string) => Promise + readFile?: (path: string, encoding: 'utf8') => Promise + open?: (path: string) => Promise + currentUid?: () => number | undefined +} + +export function defaultHandleFilePath(env: NodeJS.ProcessEnv = process.env): string { + if (env.CLAUSTRUM_OPENCODE_HANDLES) return env.CLAUSTRUM_OPENCODE_HANDLES + const configHome = env.XDG_CONFIG_HOME || (env.HOME ? join(env.HOME, '.config') : '.config') + return join(configHome, 'cortexkit', 'opencode-handles.json') +} + +function currentUid(): number | undefined { + return process.getuid?.() ?? userInfo().uid +} + +type HandleFileSnapshot = { + file: OpenCodeHandleFileV1 + source?: string + mtimeMs?: number +} + +async function readBounded(descriptor: HandleFileDescriptor, cap: number): Promise<{ buffer: Buffer; bytes: number }> { + if (!descriptor.read) throw new Error('readBounded requires a descriptor exposing read()') + const buffer = Buffer.alloc(cap + 1) + let total = 0 + while (total < cap + 1) { + const chunk = await descriptor.read(buffer, total, buffer.length - total, total) + if (chunk.bytesRead === 0) break + total += chunk.bytesRead + } + return { buffer, bytes: total > cap ? -1 : total } +} + +async function readHandleSnapshot(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { + const stat = io.stat ?? nodeStat + const lstat = io.lstat ?? nodeLstat + const read = io.readFile ?? readFile + const openFd = io.open ?? ((candidate: string) => nodeOpen(candidate, constants.O_RDONLY | constants.O_NOFOLLOW)) + let descriptor: HandleFileDescriptor | undefined + try { + let metadata: HandleFileStat + try { + if (io.lstat || io.readFile) { + metadata = await lstat(path) + } else { + descriptor = await openFd(path) + metadata = await descriptor.stat() + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return { file: { version: 1, providers: [] } } + if ((error as NodeJS.ErrnoException).code === 'ELOOP') invalid('handle file must not be a symlink') + invalid(`cannot stat handle file: ${error instanceof Error ? error.message : String(error)}`) + } + if (metadata.isSymbolicLink?.()) invalid('handle file must not be a symlink') + if (!metadata.isFile()) invalid('handle file must be a regular file') + if ((metadata.size ?? 0) > HANDLE_FILE_CONTRACT.maxBytes) invalid('handle file exceeds 256 KiB') + if ((metadata.mode & 0o777) !== HANDLE_FILE_CONTRACT.mode) invalid('handle file mode must be exactly 0600') + const uid = io.currentUid ?? currentUid + const expectedUid = uid() + if (expectedUid !== undefined && metadata.uid !== undefined && metadata.uid !== expectedUid) { + invalid('handle file is not owned by the current uid') + } + let parent: HandleFileStat + try { + parent = await stat(dirname(path)) + } catch (error) { + invalid(`cannot stat handle file parent: ${error instanceof Error ? error.message : String(error)}`) + } + if (!parent.isDirectory?.()) invalid('handle file parent must be a directory') + if (expectedUid !== undefined && parent.uid !== undefined && parent.uid !== expectedUid) { + invalid('handle file parent is not owned by the current uid') + } + if ((parent.mode & 0o002) !== 0 && (parent.mode & 0o1000) === 0) { + invalid('handle file parent is world-writable without sticky bit') + } + let source: string + try { + if (descriptor) { + const { buffer, bytes } = await readBounded(descriptor, HANDLE_FILE_CONTRACT.maxBytes) + if (bytes === -1) invalid('handle file exceeds 256 KiB') + source = buffer.subarray(0, bytes).toString('utf8') + } else { + source = await read(path, 'utf8') + } + } catch (error) { + if (error instanceof HandleFileValidationError) throw error + invalid(`cannot read handle file: ${error instanceof Error ? error.message : String(error)}`) + } + let value: unknown + try { + value = JSON.parse(source) + } catch { + invalid('handle file contains invalid JSON') + } + return { file: parseHandleFile(value), source, mtimeMs: metadata.mtimeMs } + } finally { + await descriptor?.close() + } +} + +export async function readHandleFile(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { + return (await readHandleSnapshot(path, io)).file +} + +export async function handleFileRevision(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { + const snapshot = await readHandleSnapshot(path, io) + if (snapshot.source === undefined) invalid('cannot revise absent handle file') + return `${snapshot.mtimeMs ?? 0}:${createHash('sha256').update(snapshot.source).digest('hex')}` +} diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index c4f0789..0e5c6ec 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -6,6 +6,14 @@ export { type ClaustrumEndpoint, } from './detect.js' export { storeIdentity, storageFingerprint } from './identity.js' +export { + MANIFEST_LOCK, + withManifestLock, + writeHandleFileLocked, + type ManifestHandleAccount, + type ManifestHandleFile, + type ManifestHandleProvider, +} from './manifest-lock.js' export { ClaustrumCredentialError, credentialErrorAction, @@ -21,3 +29,15 @@ export { type CredentialStatus, type ServedCredential, } from './wire.js' +export { + HANDLE_FILE_CONTRACT, + HandleFileValidationError, + defaultHandleFilePath, + handleFileRevision, + parseHandleFile, + readHandleFile, + type HandleAccount, + type HandleFileIo, + type HandleProvider, + type OpenCodeHandleFileV1, +} from './handles.js' diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts new file mode 100644 index 0000000..884c9b1 --- /dev/null +++ b/packages/client/src/manifest-lock.ts @@ -0,0 +1,105 @@ +import { constants as fsConstants } from 'node:fs' +import { chmod, lstat, mkdir, open, readFile, rename, rm, stat, unlink } from 'node:fs/promises' +import { randomBytes, randomInt } from 'node:crypto' +import { dirname, join } from 'node:path' +import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' + +export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/ } +export type ManifestHandleAccount = OpenCodeHandleFileV1['providers'][number]['accounts'][number] +export type ManifestHandleProvider = OpenCodeHandleFileV1['providers'][number] +export type ManifestHandleFile = OpenCodeHandleFileV1 +type Owner = { tenant: string; pid: number; claimed_at_ms: number; nonce: string } +type TestOptions = { ttlMs?: number; renewEveryMs?: number; retryMinMs?: number; retryMaxMs?: number; afterClaim?: () => Promise | void; beforeEvict?: () => Promise; afterEvictRenameAttempt?: () => Promise; afterEvict?: () => void; beforeManifestRename?: (path: string) => Promise } +let testOptions: TestOptions | undefined +export function __setManifestLockTestOptions(options?: TestOptions): void { testOptions = options } +const token = () => randomBytes(16).toString('base64url') +const code = (error: unknown) => (error as NodeJS.ErrnoException | undefined)?.code +const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) + +function parseOwner(source: string): Owner { + const value = JSON.parse(source) as unknown + if (!value || typeof value !== 'object') throw new Error('manifest lock owner invalid') + const owner = value as Record + if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw new Error('manifest lock owner invalid') + return owner as Owner +} +const readOwner = async (path: string) => parseOwner(await readFile(path, 'utf8')) +async function writeOwner(lock: string, owner: Owner): Promise { + const target = join(lock, 'owner'); const temporary = join(lock, `owner.${process.pid}.${token()}.tmp`) + let file: Awaited> | undefined + try { + file = await open(temporary, fsConstants.O_CREAT | fsConstants.O_EXCL | fsConstants.O_WRONLY, 0o600) + await file.chmod(0o600); await file.writeFile(`${JSON.stringify(owner)}\n`); await file.sync(); await file.close(); file = undefined + await rename(temporary, target) + } finally { await file?.close().catch(() => {}); await unlink(temporary).catch(() => {}) } +} + +export async function withManifestLock(path: string, tenant: string, fn: () => Promise | T): Promise { + return withLockCommit(path, tenant, async () => fn()) +} +async function withLockCommit(path: string, tenant: string, fn: (commit: () => Promise) => Promise | T): Promise { + const lock = `${path}.lock`, ownerPath = join(lock, 'owner'), ttl = testOptions?.ttlMs ?? MANIFEST_LOCK.ttlMs, renewEvery = testOptions?.renewEveryMs ?? MANIFEST_LOCK.renewEveryMs, retryMin = testOptions?.retryMinMs ?? 25, retryMax = testOptions?.retryMaxMs ?? 75 + const nonce = token(), started = Date.now(), deadline = started + ttl + while (true) { + try { await mkdir(lock, { mode: 0o700 }); await writeOwner(lock, { tenant, pid: process.pid, claimed_at_ms: Date.now(), nonce }); await testOptions?.afterClaim?.(); break } catch (error) { + if (code(error) !== 'EEXIST') { if (code(error) !== 'ENOENT') await rm(lock, { recursive: true, force: true }).catch(() => {}); throw error } + } + let observed: Owner | undefined + try { observed = await readOwner(ownerPath) } catch (error) { if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw new Error('manifest lock busy') } + if (observed && started - observed.claimed_at_ms >= ttl) { + await testOptions?.beforeEvict?.() + const stale = `${lock}.stale-${observed.claimed_at_ms}-${observed.nonce}` + let renameError: unknown + try { await rename(lock, stale) } catch (error) { renameError = error } + await testOptions?.afterEvictRenameAttempt?.() + if (renameError === undefined) { + const moved = await readOwner(join(stale, 'owner')).catch(() => undefined) + if (moved?.nonce === observed.nonce && moved.claimed_at_ms === observed.claimed_at_ms) { testOptions?.afterEvict?.(); continue } + await rename(stale, lock).catch(() => {}) + } else if (!['ENOENT', 'EEXIST', 'ENOTEMPTY'].includes(code(renameError) ?? '')) throw renameError + } + if (Date.now() >= deadline) throw new Error('manifest lock busy') + await sleep(Math.min(randomInt(retryMin, retryMax + 1), Math.max(1, deadline - Date.now()))) + } + let renewal = Promise.resolve(), failed = false, stopped = false + const timer = setInterval(() => { renewal = renewal.then(async () => { if (failed) return; try { const current = await readOwner(ownerPath); if (current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw new Error('lease lost'); await writeOwner(lock, { ...current, claimed_at_ms: Date.now() }) } catch { failed = true } }) }, renewEvery) + timer.unref?.() + const commit = async () => { if (!stopped) { stopped = true; clearInterval(timer); await renewal }; const current = await readOwner(ownerPath).catch(() => undefined); if (failed || !current || current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw new Error('manifest lock renewal failed; write aborted') } + try { const result = await fn(commit); if (failed) throw new Error('manifest lock renewal failed; write aborted'); return result } finally { + if (!stopped) clearInterval(timer); await renewal + const current = await readOwner(ownerPath).catch(() => undefined) + if (!current || current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) console.warn('manifest lock lease lost, not releasing', { path, tenant }) + else { + const release = `${lock}.release-${nonce}` + try { await rename(lock, release); const moved = await readOwner(join(release, 'owner')).catch(() => undefined); if (!moved || moved.nonce !== nonce || Date.now() - moved.claimed_at_ms >= ttl) { await rename(release, lock).catch(() => {}); console.warn('manifest lock lease lost, not releasing', { path, tenant }) } else await rm(release, { recursive: true, force: true }) } catch { console.warn('manifest lock lease lost, not releasing', { path, tenant }) } + } + } +} + +async function readManifest(path: string): Promise { + let metadata: Awaited> + try { metadata = await lstat(path) } catch (error) { if (code(error) === 'ENOENT') return { version: 1, providers: [] }; throw error } + if (metadata.isSymbolicLink() || !metadata.isFile()) throw new Error('handle file must be a regular file') + if ((metadata.mode & 0o777) !== HANDLE_FILE_CONTRACT.mode) throw new Error('handle file mode must be exactly 0600') + const source = await readFile(path); if (source.byteLength > HANDLE_FILE_CONTRACT.maxBytes) throw new Error('handle file exceeds 256 KiB') + return parseHandleFile(JSON.parse(source.toString('utf8'))) +} +const foreign = (file: ManifestHandleFile, tenant: string) => file.providers.filter((provider) => provider.serve !== tenant).map((provider) => JSON.stringify(provider)) +async function prepareParent(path: string): Promise { const parent = dirname(path); await mkdir(parent, { recursive: true, mode: 0o700 }); const metadata = await stat(parent); if (!metadata.isDirectory()) throw new Error('handle file parent must be a directory'); if ((metadata.mode & 0o002) !== 0 && (metadata.mode & 0o1000) === 0) throw new Error('handle file parent is world-writable without sticky bit'); await chmod(parent, 0o700) } +async function writeAtomic(path: string, file: ManifestHandleFile, commit: () => Promise): Promise { + const bytes = Buffer.from(JSON.stringify(file)); if (bytes.byteLength > HANDLE_FILE_CONTRACT.maxBytes) throw new Error('handle file exceeds 256 KiB') + const temporary = join(dirname(path), `.${path.split('/').pop()}.${process.pid}.${token()}.tmp`); let handle: Awaited> | undefined + try { handle = await open(temporary, fsConstants.O_CREAT | fsConstants.O_EXCL | fsConstants.O_WRONLY, 0o600); await handle.chmod(0o600); await handle.writeFile(bytes); await handle.sync(); await handle.close(); handle = undefined; await chmod(temporary, 0o600); await testOptions?.beforeManifestRename?.(`${path}.lock`); await commit(); await rename(temporary, path) } finally { await handle?.close().catch(() => {}); await unlink(temporary).catch(() => {}) } +} +export async function writeHandleFileLocked(path: string, tenant: string, mutate: (file: ManifestHandleFile) => void | ManifestHandleFile | Promise): Promise { + await prepareParent(path) + await withLockCommit(path, tenant, async (commit) => { + const before = await readManifest(path), working = structuredClone(before), result = await mutate(working), next = parseHandleFile(result ?? working), beforeForeign = foreign(before, tenant) + if (JSON.stringify(foreign(next, tenant)) !== JSON.stringify(beforeForeign)) throw new Error('manifest mutation changed another tenant block') + await writeAtomic(path, next, commit) + if (((await lstat(path)).mode & 0o777) !== HANDLE_FILE_CONTRACT.mode) throw new Error('manifest readback mode is not 0600') + const readback = await readManifest(path) + if (JSON.stringify(readback) !== JSON.stringify(next)) throw new Error('manifest readback differs') + if (JSON.stringify(foreign(readback, tenant)) !== JSON.stringify(beforeForeign)) throw new Error('manifest readback changed another tenant block') + }) +} diff --git a/packages/client/src/tests/handles.test.ts b/packages/client/src/tests/handles.test.ts new file mode 100644 index 0000000..f1d90d5 --- /dev/null +++ b/packages/client/src/tests/handles.test.ts @@ -0,0 +1,131 @@ +import { afterEach, describe, expect, test } from 'bun:test' +import { chmod, mkdir, rm, symlink, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { + HANDLE_FILE_CONTRACT, + defaultHandleFilePath, + handleFileRevision, + parseHandleFile, + readHandleFile, + type OpenCodeHandleFileV1, +} from '../handles.js' + +const root = '/tmp/claustrum-client-handles-tests' +const handle = `ckh_${'a'.repeat(43)}` + +afterEach(() => rm(root, { recursive: true, force: true })) + +function validFile(): OpenCodeHandleFileV1 { + return { version: 1, providers: [{ provider: 'deepseek', shape: 'api', serve: 'opencode-claustrum', accounts: [{ label: 'main', handle, credential_id: 'apikey:deepseek:main' }] }] } +} + +describe('client handle-file contract', () => { + test('exports the pinned resolver and contract constants', () => { + expect(defaultHandleFilePath({ CLAUSTRUM_OPENCODE_HANDLES: '/tmp/custom.json' })).toBe('/tmp/custom.json') + expect(HANDLE_FILE_CONTRACT.maxBytes).toBe(262144) + expect(HANDLE_FILE_CONTRACT.mode).toBe(0o600) + expect(HANDLE_FILE_CONTRACT.labelRe.test('main.account-1')).toBe(true) + expect(HANDLE_FILE_CONTRACT.handleRe.test(handle)).toBe(true) + }) + + test('reads a valid owned 0600 manifest and computes its revision', async () => { + await mkdir(root, { recursive: true, mode: 0o700 }) + const path = join(root, 'handles.json') + await writeFile(path, `${JSON.stringify(validFile())}\n`, { mode: 0o600 }) + expect(await readHandleFile(path)).toEqual(validFile()) + expect(await handleFileRevision(path)).toMatch(/^\d+(\.\d+)?:\w{64}$/) + }) + + test('rejects insecure mode and world-writable parents', async () => { + await mkdir(root, { recursive: true, mode: 0o777 }) + const path = join(root, 'handles.json') + await writeFile(path, JSON.stringify(validFile()), { mode: 0o600 }) + await chmod(root, 0o777) + await expect(readHandleFile(path)).rejects.toThrow('world-writable without sticky bit') + await chmod(root, 0o700) + await chmod(path, 0o640) + await expect(readHandleFile(path)).rejects.toThrow('exactly 0600') + }) + + test('rejects invalid labels, handles, prototype keys, and oversized input', async () => { + expect(() => parseHandleFile({ version: 1, providers: [{ ...validFile().providers[0], accounts: [{ ...validFile().providers[0].accounts[0], label: '__proto__' }] }] })).toThrow('invalid account label') + expect(() => parseHandleFile({ version: 1, providers: [{ ...validFile().providers[0], accounts: [{ ...validFile().providers[0].accounts[0], handle: 'ckh_short' }] }] })).toThrow('invalid handle') + await mkdir(root, { recursive: true, mode: 0o700 }) + const path = join(root, 'handles.json') + await writeFile(path, 'x'.repeat(262145), { mode: 0o600 }) + await expect(readHandleFile(path)).rejects.toThrow('exceeds 256 KiB') + }) + + test('preserves the historical parser fixture outcomes and exact messages', () => { + const provider = validFile().providers[0] + const account = provider.accounts[0] + const fixtures: Array<[unknown, string]> = [ + [null, 'handle file must be an object'], + [{ version: 2, providers: [] }, 'handle file must have version 1 and providers'], + [{ version: 1, providers: [null] }, 'provider 0 must be an object'], + [{ version: 1, providers: [{ ...provider, provider: '__proto__' }] }, 'provider 0 has invalid provider'], + [{ version: 1, providers: [provider, provider] }, 'provider 1 duplicates provider deepseek'], + [{ version: 1, providers: [{ ...provider, shape: 'other' }] }, 'provider 0 has invalid shape'], + [{ version: 1, providers: [{ ...provider, serve: '' }] }, 'provider 0 requires serve'], + [{ version: 1, providers: [{ ...provider, accounts: [{ ...account, credential_id: 3 }] }] }, 'provider 0 has invalid accounts'], + [{ version: 1, providers: [{ ...provider, accounts: [{ ...account, label: '__proto__' }] }] }, 'provider 0 has an invalid account label'], + [{ version: 1, providers: [{ ...provider, accounts: [account, account] }] }, 'provider 0 duplicates account label main'], + [{ version: 1, providers: [{ ...provider, accounts: [{ ...account, handle: 'ckh_short' }] }] }, 'provider 0 account main has invalid handle'], + [{ version: 1, providers: [{ ...provider, accounts: [{ ...account, credential_id: '' }] }] }, 'provider 0 account main has invalid credential id'], + [{ version: 1, providers: [{ ...provider, accounts: [{ ...account, superseded: ['ckh_short'] }] }] }, 'provider 0 account main has invalid superseded handle'], + ] + for (const [fixture, message] of fixtures) expect(() => parseHandleFile(fixture)).toThrow(message) + }) + + test('preserves historical reader validation order and normalized failures', async () => { + const source = JSON.stringify(validFile()) + const regular = { isFile: () => true, mode: 0o100600, uid: 1000, size: source.length } + const parent = { isFile: () => false, isDirectory: () => true, mode: 0o040755, uid: 1000 } + await expect(readHandleFile('/tmp/handles.json', { + currentUid: () => 1000, + lstat: async () => regular, + stat: async () => { throw new Error('parent denied') }, + readFile: async () => source, + })).rejects.toThrow('cannot stat handle file parent: parent denied') + await expect(readHandleFile('/tmp/handles.json', { + currentUid: () => 1000, + lstat: async () => regular, + stat: async () => parent, + readFile: async () => { throw new Error('read denied') }, + })).rejects.toThrow('cannot read handle file: read denied') + await expect(readHandleFile('/tmp/handles.json', { + currentUid: () => 1000, + lstat: async () => ({ ...regular, mode: 0o100640, uid: 1001 }), + stat: async () => { throw new Error('must not reach parent') }, + readFile: async () => { throw new Error('must not read') }, + })).rejects.toThrow('handle file mode must be exactly 0600') + }) + + test('preserves the historical symlink and grow-after-fstat fixture outcomes', async () => { + await mkdir(root, { recursive: true, mode: 0o700 }) + const target = join(root, 'target.json') + const link = join(root, 'handles.json') + await writeFile(target, JSON.stringify(validFile()), { mode: 0o600 }) + await symlink(target, link) + await expect(readHandleFile(link)).rejects.toThrow('handle file must not be a symlink') + + const chunk = Buffer.alloc(HANDLE_FILE_CONTRACT.maxBytes + 2, 0x78) + await expect(readHandleFile('/tmp/handles.json', { + currentUid: () => 1000, + stat: async () => ({ isFile: () => false, isDirectory: () => true, mode: 0o040755, uid: 1000 }), + open: async () => ({ + stat: async () => ({ isFile: () => true, mode: 0o100600, uid: 1000, size: 256 }), + readFile: async () => chunk.toString('utf8'), + read: (buffer, offset, length, position) => { + const remaining = chunk.length - position + if (remaining <= 0) return { bytesRead: 0 } + const slice = chunk.subarray(position, position + Math.min(length, remaining)) + buffer.set(slice, offset) + return { bytesRead: slice.length } + }, + close: async () => {}, + }), + })).rejects.toThrow('handle file exceeds 256 KiB') + }) + +}) diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts new file mode 100644 index 0000000..371f198 --- /dev/null +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -0,0 +1,314 @@ +import { afterEach, describe, expect, test } from 'bun:test' +import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, rename, rm, stat, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { basename, join } from 'node:path' + +import { + __setManifestLockTestOptions, + MANIFEST_LOCK, + withManifestLock, + writeHandleFileLocked, +} from '../manifest-lock' + +const roots: string[] = [] +const handle = (letter: string) => `ckh_${letter.repeat(43)}` + +async function manifestPath(): Promise { + const root = await mkdtemp(join(tmpdir(), 'claustrum-manifest-lock-')) + roots.push(root) + return join(root, 'opencode-handles.json') +} + +function provider(provider: string, tenant: string) { + return { + provider, + shape: 'api' as const, + serve: tenant, + accounts: [{ + label: 'main', + handle: handle(provider[0] ?? 'A'), + credential_id: `apikey:${provider}:main`, + }], + } +} + +async function owner(path: string, claimedAtMs: number, tenant = 'other-tenant'): Promise { + const lockPath = `${path}.lock` + await mkdir(lockPath, { mode: 0o700 }) + await writeFile(join(lockPath, 'owner'), `${JSON.stringify({ + tenant, + pid: 41, + claimed_at_ms: claimedAtMs, + nonce: '0123456789abcdef0123456789abcdef', + })}\n`, { mode: 0o600 }) + await chmod(join(lockPath, 'owner'), 0o600) +} + +afterEach(async () => { + __setManifestLockTestOptions() + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('manifest writer lock', () => { + test('two concurrent tenant writers preserve both provider blocks', async () => { + const path = await manifestPath() + const firstEntered = Promise.withResolvers() + const releaseFirst = Promise.withResolvers() + + const first = writeHandleFileLocked(path, 'anthropic-auth', async (file) => { + firstEntered.resolve() + await releaseFirst.promise + file.providers.push(provider('anthropic', 'anthropic-auth')) + }) + await firstEntered.promise + const second = writeHandleFileLocked(path, 'openai-auth', (file) => { + file.providers.push(provider('openai', 'openai-auth')) + }) + releaseFirst.resolve() + await Promise.all([first, second]) + + const written = JSON.parse(await readFile(path, 'utf8')) as { providers: Array<{ provider: string }> } + expect(written.providers.map((entry) => entry.provider).sort()).toEqual(['anthropic', 'openai']) + }) + + test('stale owner is evicted by rename and retained as a quarantine directory', async () => { + const path = await manifestPath() + await owner(path, Date.now() - MANIFEST_LOCK.ttlMs - 1) + + await withManifestLock(path, 'anthropic-auth', async () => {}) + + const suffixes = (await readdir(join(path, '..'))) + .filter((name) => name.startsWith(`${basename(path)}.lock.stale-`)) + .map((name) => name.slice(basename(path).length)) + expect(suffixes).toHaveLength(1) + expect(MANIFEST_LOCK.staleTargetRe.test(suffixes[0]!)).toBe(true) + }) + + test('fresh owner fails loudly after the bounded retry window', async () => { + const path = await manifestPath() + __setManifestLockTestOptions({ ttlMs: 40, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await owner(path, Date.now()) + + const started = Date.now() + await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') + expect(Date.now() - started).toBeGreaterThanOrEqual(35) + }) + + test('owner file exists while held and disappears with the lock after release', async () => { + const path = await manifestPath() + const lockPath = `${path}.lock` + + await withManifestLock(path, 'anthropic-auth', async () => { + const parsed = JSON.parse(await readFile(join(lockPath, 'owner'), 'utf8')) as Record + expect(Object.keys(parsed).sort()).toEqual([...MANIFEST_LOCK.ownerKeys].sort()) + expect(parsed.tenant).toBe('anthropic-auth') + }) + + await expect(stat(lockPath)).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + test('two stale evictors produce one eviction winner and never overlap holders', async () => { + const path = await manifestPath() + await owner(path, Date.now() - MANIFEST_LOCK.ttlMs - 1) + let waiting = 0 + const bothReady = Promise.withResolvers() + let evictionWins = 0 + __setManifestLockTestOptions({ + beforeEvict: async () => { + waiting += 1 + if (waiting === 2) bothReady.resolve() + await bothReady.promise + }, + afterEvict: () => { evictionWins += 1 }, + retryMinMs: 2, + retryMaxMs: 3, + }) + let active = 0 + let maxActive = 0 + const hold = async () => { + active += 1 + maxActive = Math.max(maxActive, active) + await Bun.sleep(15) + active -= 1 + } + + await Promise.all([ + withManifestLock(path, 'anthropic-auth', hold), + withManifestLock(path, 'openai-auth', hold), + ]) + + expect(evictionWins).toBe(1) + expect(maxActive).toBe(1) + }) + + test('an evictor that observed stale owner cannot rename a replacement lock', async () => { + const path = await manifestPath() + const now = Date.now() + await owner(path, now - 501) + const loserObserved = Promise.withResolvers() + const allowLoserRename = Promise.withResolvers() + const replacementClaimed = Promise.withResolvers() + const allowReplacementRelease = Promise.withResolvers() + const renameAttempted = Promise.withResolvers() + const allowAttemptCompletion = Promise.withResolvers() + let beforeEvictCalls = 0 + let evictRenameAttempts = 0 + + __setManifestLockTestOptions({ + ttlMs: 500, + renewEveryMs: 1_000, + retryMinMs: 2, + retryMaxMs: 3, + beforeEvict: async () => { + beforeEvictCalls += 1 + if (beforeEvictCalls === 1) { + loserObserved.resolve() + await allowLoserRename.promise + } + }, + afterClaim: async () => { + replacementClaimed.resolve() + await allowReplacementRelease.promise + }, + afterEvictRenameAttempt: async () => { + evictRenameAttempts += 1 + if (evictRenameAttempts !== 2) return + renameAttempted.resolve() + await allowAttemptCompletion.promise + }, + }) + + const loser = withManifestLock(path, 'loser', async () => {}) + await loserObserved.promise + const replacement = withManifestLock(path, 'replacement', async () => {}) + await replacementClaimed.promise + allowLoserRename.resolve() + await renameAttempted.promise + allowReplacementRelease.resolve() + await replacement + allowAttemptCompletion.resolve() + await loser + + await expect(stat(`${path}.lock`)).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + test('an expired holder does not release its directory and logs the lost lease', async () => { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 40, renewEveryMs: 1_000, retryMinMs: 2, retryMaxMs: 3 }) + const warnings: unknown[][] = [] + const originalWarn = console.warn + console.warn = (...args: unknown[]) => { warnings.push(args) } + try { + await withManifestLock(path, 'anthropic-auth', async () => { + const ownerPath = join(lockPath, 'owner') + const parsed = JSON.parse(await readFile(ownerPath, 'utf8')) as Record + parsed.claimed_at_ms = Date.now() - 41 + await writeFile(ownerPath, `${JSON.stringify(parsed)}\n`, { mode: 0o600 }) + }) + } finally { + console.warn = originalWarn + } + + await expect(stat(lockPath)).resolves.toBeDefined() + expect(warnings.some((args) => args.includes('manifest lock lease lost, not releasing'))).toBe(true) + }) + + test('atomic publication remains 0600 under umask 022', async () => { + const path = await manifestPath() + const previous = process.umask(0o022) + try { + await writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers.push(provider('anthropic', 'anthropic-auth')) + }) + } finally { + process.umask(previous) + } + expect((await stat(path)).mode & 0o777).toBe(0o600) + }) + + test('creates a missing manifest parent before claiming its colocated lock', async () => { + const root = await mkdtemp(join(tmpdir(), 'claustrum-manifest-parent-')) + roots.push(root) + const path = join(root, 'nested', 'opencode-handles.json') + + await writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers.push(provider('anthropic', 'anthropic-auth')) + }) + + expect((await stat(path)).mode & 0o777).toBe(0o600) + }) + + test('pins the shared lock constants and renewal bound', () => { + expect(MANIFEST_LOCK.ttlMs).toBe(30_000) + expect(MANIFEST_LOCK.renewEveryMs).toBe(10_000) + expect(MANIFEST_LOCK.ownerKeys).toEqual(['tenant', 'pid', 'claimed_at_ms', 'nonce']) + expect(MANIFEST_LOCK.staleTargetRe.source).toBe('^\\.lock\\.stale-\\d+-[A-Za-z0-9_-]+$') + expect(MANIFEST_LOCK.renewEveryMs * 3).toBeLessThanOrEqual(MANIFEST_LOCK.ttlMs) + }) + + test('reads a Rust-shaped owner fixture using the shared field contract', async () => { + const path = await manifestPath() + __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await owner(path, Date.now(), 'opencode-claustrum') + await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') + }) + + test('refuses a dangling manifest symlink without replacing it', async () => { + const path = await manifestPath() + const target = join(path, '..', 'missing-target.json') + await symlink(target, path) + + await expect(writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers.push(provider('anthropic', 'anthropic-auth')) + })).rejects.toThrow('handle file must be a regular file') + + expect((await lstat(path)).isSymbolicLink()).toBe(true) + await expect(stat(target)).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + test('aborts before manifest rename when renewal loses the original lock path', async () => { + const path = await manifestPath() + await writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers.push(provider('anthropic', 'anthropic-auth')) + }) + const before = await readFile(path, 'utf8') + __setManifestLockTestOptions({ + ttlMs: 100, + renewEveryMs: 2, + retryMinMs: 2, + retryMaxMs: 3, + beforeManifestRename: async (lockPath: string) => { + await rename(lockPath, `${lockPath}.vanished`) + await Bun.sleep(10) + }, + } as never) + + await expect(writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers[0]!.accounts.push({ + label: 'backup', + handle: handle('Z'), + credential_id: 'apikey:anthropic:backup', + }) + })).rejects.toThrow('manifest lock renewal failed; write aborted') + + expect(await readFile(path, 'utf8')).toBe(before) + }) + + test('pins missing and unparseable owner records as busy without eviction', async () => { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 25, renewEveryMs: 8, retryMinMs: 2, retryMaxMs: 3 }) + for (const ownerSource of [undefined, '{']) { + await mkdir(lockPath, { mode: 0o700 }) + if (ownerSource !== undefined) { + await writeFile(join(lockPath, 'owner'), ownerSource, { mode: 0o600 }) + } + + await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') + expect((await lstat(lockPath)).isDirectory()).toBe(true) + expect((await readdir(join(path, '..'))).some((name) => name.includes('.lock.stale-'))).toBe(false) + await rm(lockPath, { recursive: true }) + } + }) +}) diff --git a/packages/opencode/src/handles.ts b/packages/opencode/src/handles.ts index f2b91ef..5e3e3a4 100644 --- a/packages/opencode/src/handles.ts +++ b/packages/opencode/src/handles.ts @@ -1,221 +1,34 @@ -import { constants } from "node:fs"; -import { lstat as nodeLstat, open as nodeOpen, readFile, stat as nodeStat } from "node:fs/promises"; -import { userInfo } from "node:os"; -import { dirname, join } from "node:path"; -import { createHash } from "node:crypto"; - -import { boundedBytesText, readBounded, type BoundedReadDescriptor } from "./bounded-read"; -import { HandleFileValidationError } from "./errors"; -import { parseSecretJson, SecretJsonParseError } from "./secret-json"; - -export const OUR_PLUGIN_ID = "opencode-claustrum"; -const HANDLE_FILE_MAX_BYTES = 256 * 1024; -const PROVIDER_ID = /^[a-z0-9][a-z0-9._-]{0,63}$/; -const FORBIDDEN_IDENTIFIERS = new Set(["__proto__", "constructor", "prototype"]); - -export type HandleAccount = { - label: string; - handle: string; - credential_id: string; - superseded?: string[]; -}; -export type HandleProvider = { - provider: string; - shape: "api" | "oauth"; - serve: string; - accounts: HandleAccount[]; -}; -export type OpenCodeHandleFileV1 = { version: 1; providers: HandleProvider[] }; - -function isAccount(value: unknown): value is HandleAccount { - if (!value || typeof value !== "object") return false; - const account = value as Record; - return typeof account.label === "string" && typeof account.handle === "string" && - typeof account.credential_id === "string" && - (account.superseded === undefined || - (Array.isArray(account.superseded) && account.superseded.every((handle) => typeof handle === "string"))); -} - -function handleIsValid(handle: unknown): handle is string { - return typeof handle === "string" && /^ckh_[A-Za-z0-9_-]{43}$/.test(handle); -} - -function identifierIsValid(value: unknown): value is string { - return typeof value === "string" && PROVIDER_ID.test(value) && !FORBIDDEN_IDENTIFIERS.has(value); -} - -function invalid(message: string): never { - throw new HandleFileValidationError(message); -} - -export function parseHandleFile(value: unknown): OpenCodeHandleFileV1 { - if (!value || typeof value !== "object") invalid("handle file must be an object"); - const file = value as Record; - if (file.version !== 1 || !Array.isArray(file.providers)) { - invalid("handle file must have version 1 and providers"); +import { + defaultHandleFilePath, + handleFileRevision as clientHandleFileRevision, + parseHandleFile as clientParseHandleFile, + readHandleFile as clientReadHandleFile, + type HandleFileIo, + type OpenCodeHandleFileV1, +} from '@cortexkit/claustrum-client' +import { HandleFileValidationError } from './errors' + +export { defaultHandleFilePath } +export type { HandleFileIo, OpenCodeHandleFileV1 } +export const OUR_PLUGIN_ID = 'opencode-claustrum' + +function preserveError(operation: () => T): T { + try { return operation() } catch (error) { + if (error instanceof Error && error.name === 'HandleFileValidationError') throw new HandleFileValidationError(error.message) + throw error } - const providerIds = new Set(); - const providers = file.providers.map((provider, index): HandleProvider => { - if (!provider || typeof provider !== "object") invalid(`provider ${index} must be an object`); - const item = provider as Record; - if (!identifierIsValid(item.provider)) invalid(`provider ${index} has invalid provider`); - if (providerIds.has(item.provider)) invalid(`provider ${index} duplicates provider ${item.provider}`); - providerIds.add(item.provider); - if (item.shape !== "api" && item.shape !== "oauth") invalid(`provider ${index} has invalid shape`); - if (typeof item.serve !== "string" || !item.serve) invalid(`provider ${index} requires serve`); - if (!Array.isArray(item.accounts) || item.accounts.length === 0 || !item.accounts.every(isAccount)) { - invalid(`provider ${index} has invalid accounts`); - } - const labels = new Set(); - for (const account of item.accounts) { - if (!identifierIsValid(account.label)) invalid(`provider ${index} has an invalid account label`); - if (labels.has(account.label)) invalid(`provider ${index} duplicates account label ${account.label}`); - labels.add(account.label); - if (!handleIsValid(account.handle)) invalid(`provider ${index} account ${account.label} has invalid handle`); - if (!account.credential_id) invalid(`provider ${index} account ${account.label} has invalid credential id`); - if (account.superseded?.some((handle) => !handleIsValid(handle))) { - invalid(`provider ${index} account ${account.label} has invalid superseded handle`); - } - } - return { - provider: item.provider, - shape: item.shape, - serve: item.serve, - accounts: item.accounts.map((account) => ({ - ...account, - ...(account.superseded === undefined ? {} : { superseded: account.superseded }), - })), - }; - }); - return { version: 1, providers }; -} - -type HandleFileStat = { - isFile(): boolean; - isDirectory?(): boolean; - isSymbolicLink?(): boolean; - mode: number; - size?: number; - uid?: number; - mtimeMs?: number; -}; -type HandleFileDescriptor = BoundedReadDescriptor & { - stat(): Promise; - readFile(options: { encoding: "utf8" }): Promise; - close(): Promise; -}; -export type HandleFileIo = { - stat?: (path: string) => Promise; - lstat?: (path: string) => Promise; - readFile?: (path: string, encoding: "utf8") => Promise; - // Injectable descriptor: when supplied, the handle reader uses a bounded read into a - // cap+1 buffer instead of `readFile()`. The cap check then catches a TOCTOU write that - // grows the file between fstat and read; the unbounded path is preserved for callers - // that pre-trust the source. Mirrors `ConfigHookDependencies.authReader`'s `openFile` - // so a grow-after-fstat handle test can exercise the same bounded read path. - open?: (path: string) => Promise; - currentUid?: () => number | undefined; -}; - -export function defaultHandleFilePath(env: NodeJS.ProcessEnv = process.env): string { - if (env.CLAUSTRUM_OPENCODE_HANDLES) return env.CLAUSTRUM_OPENCODE_HANDLES; - const configHome = env.XDG_CONFIG_HOME || (env.HOME ? join(env.HOME, ".config") : ".config"); - return join(configHome, "cortexkit", "opencode-handles.json"); -} - -function currentUid(): number | undefined { - return process.getuid?.() ?? userInfo().uid; } -type HandleFileSnapshot = { - file: OpenCodeHandleFileV1; - source?: string; - mtimeMs?: number; -}; - -async function readHandleSnapshot(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { - const stat = io.stat ?? nodeStat; - const lstat = io.lstat ?? nodeLstat; - const read = io.readFile ?? readFile; - const openFd = io.open ?? ((candidate: string) => nodeOpen(candidate, constants.O_RDONLY | constants.O_NOFOLLOW)); - let descriptor: HandleFileDescriptor | undefined; - try { - let metadata: HandleFileStat; - try { - if (io.lstat || io.readFile) { - metadata = await lstat(path); - } else { - descriptor = await openFd(path); - metadata = await descriptor.stat(); - } - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return { file: { version: 1, providers: [] } }; - if ((error as NodeJS.ErrnoException).code === "ELOOP") invalid("handle file must not be a symlink"); - invalid(`cannot stat handle file: ${error instanceof Error ? error.message : String(error)}`); - } - if (metadata.isSymbolicLink?.()) invalid("handle file must not be a symlink"); - if (!metadata.isFile()) invalid("handle file must be a regular file"); - if ((metadata.size ?? 0) > HANDLE_FILE_MAX_BYTES) invalid("handle file exceeds 256 KiB"); - if ((metadata.mode & 0o777) !== 0o600) invalid("handle file mode must be exactly 0600"); - const uid = io.currentUid ?? currentUid; - const expectedUid = uid(); - if (expectedUid !== undefined && metadata.uid !== undefined && metadata.uid !== expectedUid) { - invalid("handle file is not owned by the current uid"); - } - let parent: HandleFileStat; - try { - parent = await stat(dirname(path)); - } catch (error) { - invalid(`cannot stat handle file parent: ${error instanceof Error ? error.message : String(error)}`); - } - if (!parent.isDirectory?.()) invalid("handle file parent must be a directory"); - if (expectedUid !== undefined && parent.uid !== undefined && parent.uid !== expectedUid) { - invalid("handle file parent is not owned by the current uid"); - } - if ((parent.mode & 0o002) !== 0 && (parent.mode & 0o1000) === 0) { - invalid("handle file parent is world-writable without sticky bit"); - } - let source: string; - try { - if (descriptor) { - // Bounded read on the already-fstat'd descriptor closes the TOCTOU window a - // size-only check leaves open: a writer that grows the file between fstat and - // readFile would otherwise drive the read past the cap. The shared helper - // allocates the cap+1 buffer and reports bytes > cap so the message is uniform - // across auth and handle paths. - const { buffer, bytes } = await readBounded(descriptor, HANDLE_FILE_MAX_BYTES); - if (bytes === -1) invalid("handle file exceeds 256 KiB"); - source = boundedBytesText(bytes, buffer); - } else { - source = await read(path, "utf8"); - } - } catch (error) { - if (error instanceof HandleFileValidationError) throw error; - invalid(`cannot read handle file: ${error instanceof Error ? error.message : String(error)}`); - } - let value: unknown; - try { - value = parseSecretJson(source, "handle file"); - } catch (error) { - if (error instanceof SecretJsonParseError) invalid("handle file contains invalid JSON"); - throw error; - } - return { - file: parseHandleFile(value), - source, - mtimeMs: metadata.mtimeMs, - }; - } finally { - await descriptor?.close(); +export function parseHandleFile(value: unknown): OpenCodeHandleFileV1 { return preserveError(() => clientParseHandleFile(value)) } +export async function readHandleFile(path?: string, io?: HandleFileIo): Promise { + try { return await clientReadHandleFile(path, io) } catch (error) { + if (error instanceof Error && error.name === 'HandleFileValidationError') throw new HandleFileValidationError(error.message) + throw error } } - -export async function readHandleFile(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { - return (await readHandleSnapshot(path, io)).file; -} - -export async function handleFileRevision(path = defaultHandleFilePath(), io: HandleFileIo = {}): Promise { - const snapshot = await readHandleSnapshot(path, io); - if (snapshot.source === undefined) invalid("cannot revise absent handle file"); - return `${snapshot.mtimeMs ?? 0}:${createHash("sha256").update(snapshot.source).digest("hex")}`; +export async function handleFileRevision(path?: string, io?: HandleFileIo): Promise { + try { return await clientHandleFileRevision(path, io) } catch (error) { + if (error instanceof Error && error.name === 'HandleFileValidationError') throw new HandleFileValidationError(error.message) + throw error + } } From dd5d54011dcbd4aaeda074419e298fc0880cf564 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:35:56 +0200 Subject: [PATCH 02/10] opencode: give manifest lock failures a stable code A tenant classifying a lock failure had only the message text to branch on: distinguishing 'busy, retry later' from 'the owner artefact is wrong' from 'the write was abandoned' meant string-matching our prose, so a copy-edit would silently reclassify a retryable busy-lock as an unknown error with nothing failing loudly. Failures now carry MANIFEST_LOCK.errorCodes -- lock_busy, owner_invalid, renewal_failed -- and the message is explicitly diagnostic. Requested by the openai-auth seat before it writes a consumer-side conformance suite, which is the cheap moment: pinning the strings first would make a later move to codes a breaking change for its tests. Also pins two behaviours the contract relied on without stating: a throwing callback releases the lock and re-raises the original error unwrapped (release sits in a finally, so an enroll path that refuses by throwing costs one operation rather than wedging every tenant for a TTL), and distinct manifest paths do not contend. --- packages/client/src/manifest-lock.ts | 22 ++++--- .../client/src/tests/manifest-lock.test.ts | 60 +++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts index 884c9b1..2c443f6 100644 --- a/packages/client/src/manifest-lock.ts +++ b/packages/client/src/manifest-lock.ts @@ -4,7 +4,11 @@ import { randomBytes, randomInt } from 'node:crypto' import { dirname, join } from 'node:path' import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' -export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/ } +export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } + +/** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */ +export type ManifestLockErrorCode = (typeof MANIFEST_LOCK.errorCodes)[number] +export type ManifestLockError = Error & { code: ManifestLockErrorCode } export type ManifestHandleAccount = OpenCodeHandleFileV1['providers'][number]['accounts'][number] export type ManifestHandleProvider = OpenCodeHandleFileV1['providers'][number] export type ManifestHandleFile = OpenCodeHandleFileV1 @@ -15,12 +19,16 @@ export function __setManifestLockTestOptions(options?: TestOptions): void { test const token = () => randomBytes(16).toString('base64url') const code = (error: unknown) => (error as NodeJS.ErrnoException | undefined)?.code const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +// A tenant classifying a failure must not string-match our prose: a copy-edit would +// silently reclassify a retryable busy-lock as an unknown error, with nothing failing +// loudly. The code is the contract; the message is free to change. +const lockError = (code: ManifestLockErrorCode, message: string): ManifestLockError => Object.assign(new Error(message), { code }) function parseOwner(source: string): Owner { const value = JSON.parse(source) as unknown - if (!value || typeof value !== 'object') throw new Error('manifest lock owner invalid') + if (!value || typeof value !== 'object') throw lockError('owner_invalid', 'manifest lock owner invalid') const owner = value as Record - if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw new Error('manifest lock owner invalid') + if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw lockError('owner_invalid', 'manifest lock owner invalid') return owner as Owner } const readOwner = async (path: string) => parseOwner(await readFile(path, 'utf8')) @@ -45,7 +53,7 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = if (code(error) !== 'EEXIST') { if (code(error) !== 'ENOENT') await rm(lock, { recursive: true, force: true }).catch(() => {}); throw error } } let observed: Owner | undefined - try { observed = await readOwner(ownerPath) } catch (error) { if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw new Error('manifest lock busy') } + try { observed = await readOwner(ownerPath) } catch (error) { if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw lockError('lock_busy', 'manifest lock busy') } if (observed && started - observed.claimed_at_ms >= ttl) { await testOptions?.beforeEvict?.() const stale = `${lock}.stale-${observed.claimed_at_ms}-${observed.nonce}` @@ -58,14 +66,14 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = await rename(stale, lock).catch(() => {}) } else if (!['ENOENT', 'EEXIST', 'ENOTEMPTY'].includes(code(renameError) ?? '')) throw renameError } - if (Date.now() >= deadline) throw new Error('manifest lock busy') + if (Date.now() >= deadline) throw lockError('lock_busy', 'manifest lock busy') await sleep(Math.min(randomInt(retryMin, retryMax + 1), Math.max(1, deadline - Date.now()))) } let renewal = Promise.resolve(), failed = false, stopped = false const timer = setInterval(() => { renewal = renewal.then(async () => { if (failed) return; try { const current = await readOwner(ownerPath); if (current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw new Error('lease lost'); await writeOwner(lock, { ...current, claimed_at_ms: Date.now() }) } catch { failed = true } }) }, renewEvery) timer.unref?.() - const commit = async () => { if (!stopped) { stopped = true; clearInterval(timer); await renewal }; const current = await readOwner(ownerPath).catch(() => undefined); if (failed || !current || current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw new Error('manifest lock renewal failed; write aborted') } - try { const result = await fn(commit); if (failed) throw new Error('manifest lock renewal failed; write aborted'); return result } finally { + const commit = async () => { if (!stopped) { stopped = true; clearInterval(timer); await renewal }; const current = await readOwner(ownerPath).catch(() => undefined); if (failed || !current || current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw lockError('renewal_failed', 'manifest lock renewal failed; write aborted') } + try { const result = await fn(commit); if (failed) throw lockError('renewal_failed', 'manifest lock renewal failed; write aborted'); return result } finally { if (!stopped) clearInterval(timer); await renewal const current = await readOwner(ownerPath).catch(() => undefined) if (!current || current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) console.warn('manifest lock lease lost, not releasing', { path, tenant }) diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts index 371f198..e418812 100644 --- a/packages/client/src/tests/manifest-lock.test.ts +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -312,3 +312,63 @@ describe('manifest writer lock', () => { } }) }) + +describe('thrown errors carry a stable code', () => { + // A tenant classifying "retry later" vs "the artefact is wrong" vs "the write was + // abandoned" had only the message text to branch on, so any copy-edit here silently + // reclassified a busy lock as an unknown error. openai-auth asked for this before + // writing its conformance suite, which is the cheap moment to add it. + test('a busy lock throws code lock_busy', async () => { + const path = await manifestPath() + __setManifestLockTestOptions({ ttlMs: 400, retryMinMs: 5, retryMaxMs: 10 }) + await mkdir(`${path}.lock`, { mode: 0o700 }) + await writeFile(join(`${path}.lock`, 'owner'), `${JSON.stringify({ tenant: 'squatter', pid: 1, claimed_at_ms: Date.now(), nonce: 'n'.repeat(22) })}\n`, { mode: 0o600 }) + const error = (await withManifestLock(path, 'probe', async () => {}).catch((e) => e)) as Error & { code?: string } + expect(error.code).toBe('lock_busy') + expect(MANIFEST_LOCK.errorCodes).toContain('lock_busy') + }) + + test('an unparseable owner is busy, never evicted, and keeps that code', async () => { + const path = await manifestPath() + __setManifestLockTestOptions({ ttlMs: 400, retryMinMs: 5, retryMaxMs: 10 }) + await mkdir(`${path}.lock`, { mode: 0o700 }) + await writeFile(join(`${path}.lock`, 'owner'), 'not json at all\n', { mode: 0o600 }) + const error = (await withManifestLock(path, 'probe', async () => {}).catch((e) => e)) as Error & { code?: string } + expect(error.code).toBe('lock_busy') + // the squatter's lock must still be standing: unreadable owner is never evicted + expect((await stat(`${path}.lock`)).isDirectory()).toBe(true) + expect((await readFile(join(`${path}.lock`, 'owner'), 'utf8')).trim()).toBe('not json at all') + }) + + test('a throwing callback releases the lock and re-raises the original error unwrapped', async () => { + const path = await manifestPath() + class EnrollRefusal extends Error { + constructor() { + super('identity mismatch') + this.name = 'EnrollRefusal' + } + } + const error = await withManifestLock(path, 'probe', async () => { + throw new EnrollRefusal() + }).catch((e) => e) + expect(error).toBeInstanceOf(EnrollRefusal) + expect((error as Error).message).toBe('identity mismatch') + await expect(stat(`${path}.lock`)).rejects.toThrow() + // the consumer-visible consequence: the next claimant is not stalled for a TTL + const started = Date.now() + await withManifestLock(path, 'probe', async () => {}) + expect(Date.now() - started).toBeLessThan(1_000) + }) + + test('distinct manifest paths do not contend', async () => { + const first = await manifestPath() + const second = await manifestPath() + let bothInside = false + await withManifestLock(first, 'tenant-a', async () => { + await withManifestLock(second, 'tenant-b', async () => { + bothInside = true + }) + }) + expect(bothInside).toBe(true) + }) +}) From 7509f31caf14935f1be2f04f09245966cb9fa4bd Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:38:47 +0200 Subject: [PATCH 03/10] opencode: make the release-on-throw pin fail fast instead of hanging The test awaited the re-acquire and measured afterwards, so a regression in release-on-throw would block for the full 30s TTL and surface as a suite-level timeout with no attribution -- indistinguishable from a slow box or a hang elsewhere. A fault and an environment condition sharing one symptom is the defect this suite exists to catch, so it should not be the harness's own failure mode. The re-acquire is now raced against a bounded timer whose arm names the property, and the probe that produced the original number (3ms against a 30000ms wedge) leaves three orders of magnitude of headroom. Found by the openai-auth seat reviewing the pin before writing its own. --- packages/client/src/tests/manifest-lock.test.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts index e418812..a822eac 100644 --- a/packages/client/src/tests/manifest-lock.test.ts +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -12,6 +12,7 @@ import { const roots: string[] = [] const handle = (letter: string) => `ckh_${letter.repeat(43)}` +const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) async function manifestPath(): Promise { const root = await mkdtemp(join(tmpdir(), 'claustrum-manifest-lock-')) @@ -354,9 +355,17 @@ describe('thrown errors carry a stable code', () => { expect(error).toBeInstanceOf(EnrollRefusal) expect((error as Error).message).toBe('identity mismatch') await expect(stat(`${path}.lock`)).rejects.toThrow() - // the consumer-visible consequence: the next claimant is not stalled for a TTL + // The consumer-visible consequence: the next claimant is not stalled for a TTL. + // Bounded by a race rather than by awaiting and measuring afterwards -- if release + // regressed, the await itself would block for the full 30s TTL and the suite would + // report a timeout with no attribution, which is indistinguishable from a slow box + // or a hang anywhere else. Failing fast with the property named beats hanging. const started = Date.now() - await withManifestLock(path, 'probe', async () => {}) + const outcome = await Promise.race([ + withManifestLock(path, 'probe', async () => 'reacquired' as const), + sleep(1_000).then(() => 'lock not released on throw: re-acquire exceeded 1000ms' as const), + ]) + expect(outcome).toBe('reacquired') expect(Date.now() - started).toBeLessThan(1_000) }) From 35eb4ed290c0237d8de4a3a725469ed90a67da6b Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 4 Sep 2026 21:59:19 +0200 Subject: [PATCH 04/10] opencode: judge lock staleness at observation, and validate the nonce it builds a path from A contender that arrived while an owner was fresh could exhaust its retry window after that owner died, because staleness was frozen at claim start. Judge each observation against the current clock; renewal remains what protects a healthy owner. Validate eviction-critical timestamps and nonces before constructing quarantine paths, surface permanently corrupt owners as owner_invalid, and tolerate unknown keys plus malformed diagnostic fields so independently upgraded readers do not wedge on a healthy newer writer. Exact-key matching was the defect, not a safety property. Nonce validation rejects only path-unsafe shapes and keeps the quarantine regex aligned with that rule, so future path-safe nonce alphabets remain evictable without changing the cross-version ABA target. Leave pre-existing parent modes unchanged while refusing group- or other-writable parents, and re-export ManifestLockError plus ManifestLockErrorCode from the package entrypoint. --- packages/client/src/index.ts | 2 + packages/client/src/manifest-lock.ts | 20 ++- .../client/src/tests/manifest-lock.test.ts | 142 ++++++++++++++++-- 3 files changed, 146 insertions(+), 18 deletions(-) diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 0e5c6ec..85268d8 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -13,6 +13,8 @@ export { type ManifestHandleAccount, type ManifestHandleFile, type ManifestHandleProvider, + type ManifestLockError, + type ManifestLockErrorCode, } from './manifest-lock.js' export { ClaustrumCredentialError, diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts index 2c443f6..445df0e 100644 --- a/packages/client/src/manifest-lock.ts +++ b/packages/client/src/manifest-lock.ts @@ -4,7 +4,7 @@ import { randomBytes, randomInt } from 'node:crypto' import { dirname, join } from 'node:path' import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' -export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-[A-Za-z0-9_-]+$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } +export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } /** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */ export type ManifestLockErrorCode = (typeof MANIFEST_LOCK.errorCodes)[number] @@ -25,10 +25,14 @@ const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, const lockError = (code: ManifestLockErrorCode, message: string): ManifestLockError => Object.assign(new Error(message), { code }) function parseOwner(source: string): Owner { - const value = JSON.parse(source) as unknown + let value: unknown + try { value = JSON.parse(source) as unknown } catch { throw lockError('owner_invalid', 'manifest lock owner invalid') } if (!value || typeof value !== 'object') throw lockError('owner_invalid', 'manifest lock owner invalid') const owner = value as Record - if (Object.keys(owner).sort().join('\0') !== [...MANIFEST_LOCK.ownerKeys].sort().join('\0') || typeof owner.tenant !== 'string' || typeof owner.pid !== 'number' || !Number.isInteger(owner.pid) || typeof owner.claimed_at_ms !== 'number' || !Number.isFinite(owner.claimed_at_ms) || typeof owner.nonce !== 'string') throw lockError('owner_invalid', 'manifest lock owner invalid') + // Widen the nonce alphabet only after every tenant has this path-safe reader; an older + // allowlist reader can otherwise wedge forever on the first owner using the new alphabet. + const staleTarget = `.lock.stale-${owner.claimed_at_ms}-${owner.nonce}` + if (MANIFEST_LOCK.ownerKeys.some((key) => !Object.hasOwn(owner, key)) || typeof owner.claimed_at_ms !== 'number' || !Number.isInteger(owner.claimed_at_ms) || owner.claimed_at_ms < 0 || typeof owner.nonce !== 'string' || !MANIFEST_LOCK.staleTargetRe.test(staleTarget)) throw lockError('owner_invalid', 'manifest lock owner invalid') return owner as Owner } const readOwner = async (path: string) => parseOwner(await readFile(path, 'utf8')) @@ -52,9 +56,9 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = try { await mkdir(lock, { mode: 0o700 }); await writeOwner(lock, { tenant, pid: process.pid, claimed_at_ms: Date.now(), nonce }); await testOptions?.afterClaim?.(); break } catch (error) { if (code(error) !== 'EEXIST') { if (code(error) !== 'ENOENT') await rm(lock, { recursive: true, force: true }).catch(() => {}); throw error } } - let observed: Owner | undefined - try { observed = await readOwner(ownerPath) } catch (error) { if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw lockError('lock_busy', 'manifest lock busy') } - if (observed && started - observed.claimed_at_ms >= ttl) { + let observed: Owner | undefined, ownerReadError: unknown + try { observed = await readOwner(ownerPath) } catch (error) { ownerReadError = error; if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw code(error) === 'owner_invalid' ? error : lockError('lock_busy', 'manifest lock busy') } + if (observed && Date.now() - observed.claimed_at_ms >= ttl) { await testOptions?.beforeEvict?.() const stale = `${lock}.stale-${observed.claimed_at_ms}-${observed.nonce}` let renameError: unknown @@ -66,7 +70,7 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = await rename(stale, lock).catch(() => {}) } else if (!['ENOENT', 'EEXIST', 'ENOTEMPTY'].includes(code(renameError) ?? '')) throw renameError } - if (Date.now() >= deadline) throw lockError('lock_busy', 'manifest lock busy') + if (Date.now() >= deadline) throw code(ownerReadError) === 'owner_invalid' ? ownerReadError : lockError('lock_busy', 'manifest lock busy') await sleep(Math.min(randomInt(retryMin, retryMax + 1), Math.max(1, deadline - Date.now()))) } let renewal = Promise.resolve(), failed = false, stopped = false @@ -93,7 +97,7 @@ async function readManifest(path: string): Promise { return parseHandleFile(JSON.parse(source.toString('utf8'))) } const foreign = (file: ManifestHandleFile, tenant: string) => file.providers.filter((provider) => provider.serve !== tenant).map((provider) => JSON.stringify(provider)) -async function prepareParent(path: string): Promise { const parent = dirname(path); await mkdir(parent, { recursive: true, mode: 0o700 }); const metadata = await stat(parent); if (!metadata.isDirectory()) throw new Error('handle file parent must be a directory'); if ((metadata.mode & 0o002) !== 0 && (metadata.mode & 0o1000) === 0) throw new Error('handle file parent is world-writable without sticky bit'); await chmod(parent, 0o700) } +async function prepareParent(path: string): Promise { const parent = dirname(path); await mkdir(parent, { recursive: true, mode: 0o700 }); const metadata = await stat(parent); if (!metadata.isDirectory()) throw new Error('handle file parent must be a directory'); if ((metadata.mode & 0o002) !== 0 && (metadata.mode & 0o1000) === 0) throw new Error('handle file parent is world-writable without sticky bit'); if ((metadata.mode & 0o022) !== 0) throw new Error('handle file parent must not be group- or other-writable') } async function writeAtomic(path: string, file: ManifestHandleFile, commit: () => Promise): Promise { const bytes = Buffer.from(JSON.stringify(file)); if (bytes.byteLength > HANDLE_FILE_CONTRACT.maxBytes) throw new Error('handle file exceeds 256 KiB') const temporary = join(dirname(path), `.${path.split('/').pop()}.${process.pid}.${token()}.tmp`); let handle: Awaited> | undefined diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts index a822eac..a87bf17 100644 --- a/packages/client/src/tests/manifest-lock.test.ts +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -9,6 +9,7 @@ import { withManifestLock, writeHandleFileLocked, } from '../manifest-lock' +import type { ManifestLockError, ManifestLockErrorCode } from '../index' const roots: string[] = [] const handle = (letter: string) => `ckh_${letter.repeat(43)}` @@ -85,14 +86,93 @@ describe('manifest writer lock', () => { expect(MANIFEST_LOCK.staleTargetRe.test(suffixes[0]!)).toBe(true) }) - test('fresh owner fails loudly after the bounded retry window', async () => { + test('owner that becomes stale during the retry window is evicted', async () => { + const path = await manifestPath() + __setManifestLockTestOptions({ ttlMs: 80, renewEveryMs: 1_000, retryMinMs: 2, retryMaxMs: 3 }) + await owner(path, Date.now() - 30) + + await withManifestLock(path, 'anthropic-auth', async () => {}) + + const suffixes = (await readdir(join(path, '..'))).filter((name) => name.startsWith(`${basename(path)}.lock.stale-`)) + expect(suffixes).toHaveLength(1) + }) + + test('owner nonce containing a path traversal is invalid and never renamed', async () => { + for (const nonce of ['../escape', 'a/b', 'a:b', 'a*b', 'a?b', 'a|b']) { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await mkdir(lockPath, { mode: 0o700 }) + await writeFile(join(lockPath, 'owner'), `${JSON.stringify({ tenant: 'squatter', pid: 41, claimed_at_ms: Date.now() - 31, nonce })}\n`, { mode: 0o600 }) + + const error = (await withManifestLock(path, 'anthropic-auth', async () => {}).catch((caught) => caught)) as Error & { code?: string } + + expect(error.code).toBe('owner_invalid') + expect((await stat(lockPath)).isDirectory()).toBe(true) + expect((await readdir(join(path, '..'))).some((name) => name.includes('.lock.stale-'))).toBe(false) + await expect(stat(join(path, '..', 'escape'))).rejects.toMatchObject({ code: 'ENOENT' }) + } + }) + + test('path-safe unfamiliar nonce alphabets remain evictable', async () => { + for (const nonce of ['abc.def', 'AAAA====']) { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await mkdir(lockPath, { mode: 0o700 }) + await writeFile(join(lockPath, 'owner'), `${JSON.stringify({ tenant: 'newer-writer', pid: 41, claimed_at_ms: Date.now() - 31, nonce })}\n`, { mode: 0o600 }) + + await withManifestLock(path, 'anthropic-auth', async () => {}) + + expect((await readdir(join(path, '..'))).some((name) => name.startsWith(`${basename(path)}.lock.stale-`))).toBe(true) + } + }) + + test('unknown owner keys are busy while fresh and evictable once stale', async () => { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await mkdir(lockPath, { mode: 0o700 }) + const record = { tenant: 'newer-writer', pid: 41, claimed_at_ms: Date.now() + 1_000, nonce: 'newer_writer_nonce', generation: 2 } + await writeFile(join(lockPath, 'owner'), `${JSON.stringify(record)}\n`, { mode: 0o600 }) + + const freshError = (await withManifestLock(path, 'anthropic-auth', async () => {}).catch((caught) => caught)) as Error & { code?: string } + expect(freshError.code).toBe('lock_busy') + + record.claimed_at_ms = Date.now() - 31 + await writeFile(join(lockPath, 'owner'), `${JSON.stringify(record)}\n`, { mode: 0o600 }) + await withManifestLock(path, 'anthropic-auth', async () => {}) + expect((await readdir(join(path, '..'))).some((name) => name.startsWith(`${basename(path)}.lock.stale-`))).toBe(true) + }) + + test('malformed diagnostic owner fields do not prevent stale eviction', async () => { + const path = await manifestPath() + const lockPath = `${path}.lock` + __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) + await mkdir(lockPath, { mode: 0o700 }) + await writeFile(join(lockPath, 'owner'), `${JSON.stringify({ tenant: 41, pid: 'unknown', claimed_at_ms: Date.now() - 31, nonce: 'valid_nonce' })}\n`, { mode: 0o600 }) + + await withManifestLock(path, 'anthropic-auth', async () => {}) + + expect((await readdir(join(path, '..'))).some((name) => name.startsWith(`${basename(path)}.lock.stale-`))).toBe(true) + }) + + test('renewing owner fails loudly after the bounded retry window', async () => { const path = await manifestPath() __setManifestLockTestOptions({ ttlMs: 40, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) await owner(path, Date.now()) const started = Date.now() - await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') - expect(Date.now() - started).toBeGreaterThanOrEqual(35) + const renewal = setInterval(async () => { + const ownerPath = join(`${path}.lock`, 'owner') + const current = JSON.parse(await readFile(ownerPath, 'utf8')) as Record + current.claimed_at_ms = Date.now() + await writeFile(ownerPath, `${JSON.stringify(current)}\n`, { mode: 0o600 }) + }, 5) + try { + await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') + expect(Date.now() - started).toBeGreaterThanOrEqual(35) + } finally { clearInterval(renewal) } }) test('owner file exists while held and disappears with the lock after release', async () => { @@ -240,18 +320,54 @@ describe('manifest writer lock', () => { expect((await stat(path)).mode & 0o777).toBe(0o600) }) + test('leaves the mode of a pre-existing benign parent unchanged', async () => { + const path = await manifestPath() + const parent = join(path, '..') + await chmod(parent, 0o755) + const before = (await stat(parent)).mode & 0o777 + + await writeHandleFileLocked(path, 'anthropic-auth', (file) => { + file.providers.push(provider('anthropic', 'anthropic-auth')) + }) + + expect((await stat(parent)).mode & 0o777).toBe(before) + }) + + test('refuses a group-writable manifest parent without changing its mode', async () => { + const path = await manifestPath() + const parent = join(path, '..') + await chmod(parent, 0o770) + + await expect(writeHandleFileLocked(path, 'anthropic-auth', () => {})).rejects.toThrow('handle file parent must not be group- or other-writable') + expect((await stat(parent)).mode & 0o777).toBe(0o770) + }) + test('pins the shared lock constants and renewal bound', () => { expect(MANIFEST_LOCK.ttlMs).toBe(30_000) expect(MANIFEST_LOCK.renewEveryMs).toBe(10_000) expect(MANIFEST_LOCK.ownerKeys).toEqual(['tenant', 'pid', 'claimed_at_ms', 'nonce']) - expect(MANIFEST_LOCK.staleTargetRe.source).toBe('^\\.lock\\.stale-\\d+-[A-Za-z0-9_-]+$') + for (const [target, accepted] of [ + ['.lock.stale-1-nonce_2', true], + ['.lock.stale-1-abc.def', true], + ['.lock.stale-1-AAAA====', true], + ['.lock.stale-1.bad', false], + ['.lock.stale-1-a/b', false], + ['.lock.stale-1-..', false], + ['.lock.stale-1-a:b', false], + ['.lock.stale-1-a*b', false], + ['.lock.stale-1-a?b', false], + ['.lock.stale-1-a|b', false], + // Windows aliases trailing dots and spaces, collapsing distinct nonces onto one ABA target. + ['.lock.stale-1-abc.', false], + ['.lock.stale-1-abc ', false], + ] as const) expect(MANIFEST_LOCK.staleTargetRe.test(target)).toBe(accepted) expect(MANIFEST_LOCK.renewEveryMs * 3).toBeLessThanOrEqual(MANIFEST_LOCK.ttlMs) }) test('reads a Rust-shaped owner fixture using the shared field contract', async () => { const path = await manifestPath() __setManifestLockTestOptions({ ttlMs: 30, renewEveryMs: 10, retryMinMs: 2, retryMaxMs: 3 }) - await owner(path, Date.now(), 'opencode-claustrum') + await owner(path, Date.now() + 1_000, 'opencode-claustrum') await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') }) @@ -296,7 +412,7 @@ describe('manifest writer lock', () => { expect(await readFile(path, 'utf8')).toBe(before) }) - test('pins missing and unparseable owner records as busy without eviction', async () => { + test('pins missing and unparseable owner records without eviction', async () => { const path = await manifestPath() const lockPath = `${path}.lock` __setManifestLockTestOptions({ ttlMs: 25, renewEveryMs: 8, retryMinMs: 2, retryMaxMs: 3 }) @@ -306,7 +422,8 @@ describe('manifest writer lock', () => { await writeFile(join(lockPath, 'owner'), ownerSource, { mode: 0o600 }) } - await expect(withManifestLock(path, 'anthropic-auth', async () => {})).rejects.toThrow('manifest lock busy') + const error = (await withManifestLock(path, 'anthropic-auth', async () => {}).catch((caught) => caught)) as Error & { code?: string } + expect(error.code).toBe(ownerSource === undefined ? 'lock_busy' : 'owner_invalid') expect((await lstat(lockPath)).isDirectory()).toBe(true) expect((await readdir(join(path, '..'))).some((name) => name.includes('.lock.stale-'))).toBe(false) await rm(lockPath, { recursive: true }) @@ -323,19 +440,24 @@ describe('thrown errors carry a stable code', () => { const path = await manifestPath() __setManifestLockTestOptions({ ttlMs: 400, retryMinMs: 5, retryMaxMs: 10 }) await mkdir(`${path}.lock`, { mode: 0o700 }) - await writeFile(join(`${path}.lock`, 'owner'), `${JSON.stringify({ tenant: 'squatter', pid: 1, claimed_at_ms: Date.now(), nonce: 'n'.repeat(22) })}\n`, { mode: 0o600 }) + await writeFile(join(`${path}.lock`, 'owner'), `${JSON.stringify({ tenant: 'squatter', pid: 1, claimed_at_ms: Date.now() + 1_000, nonce: 'n'.repeat(22) })}\n`, { mode: 0o600 }) const error = (await withManifestLock(path, 'probe', async () => {}).catch((e) => e)) as Error & { code?: string } expect(error.code).toBe('lock_busy') expect(MANIFEST_LOCK.errorCodes).toContain('lock_busy') }) - test('an unparseable owner is busy, never evicted, and keeps that code', async () => { + test('the package entrypoint exports the lock error types', () => { + const classify = (error: ManifestLockError): ManifestLockErrorCode => error.code + expect(classify(Object.assign(new Error('busy'), { code: 'lock_busy' as const }))).toBe('lock_busy') + }) + + test('an unparseable owner is invalid, never evicted, and keeps that code', async () => { const path = await manifestPath() __setManifestLockTestOptions({ ttlMs: 400, retryMinMs: 5, retryMaxMs: 10 }) await mkdir(`${path}.lock`, { mode: 0o700 }) await writeFile(join(`${path}.lock`, 'owner'), 'not json at all\n', { mode: 0o600 }) const error = (await withManifestLock(path, 'probe', async () => {}).catch((e) => e)) as Error & { code?: string } - expect(error.code).toBe('lock_busy') + expect(error.code).toBe('owner_invalid') // the squatter's lock must still be standing: unreadable owner is never evicted expect((await stat(`${path}.lock`)).isDirectory()).toBe(true) expect((await readFile(join(`${path}.lock`, 'owner'), 'utf8')).trim()).toBe('not json at all') From d1093c0a1d7db9a71220f72554956b0c5a9568f6 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sat, 5 Sep 2026 08:35:15 +0200 Subject: [PATCH 05/10] opencode: Rust lock reader tolerates unknown owner keys and judges staleness at observation The TypeScript reader already treated owner keys as required-not-exclusive and judged staleness against the current clock (71f927f); the Rust reader still refused unknown keys (deny_unknown_fields) and compared against the clock captured at claim start. The first wedges every Rust contender permanently the moment any tenant adds a diagnostic field; the second makes a lock that ages past TTL during retries fail busy at the deadline instead of evicting. Both pinned RED-first; quarantine name format unchanged. --- .../src/bin/cli_support/opencode_files.rs | 268 +++++++++++++++--- 1 file changed, 221 insertions(+), 47 deletions(-) diff --git a/crates/credentials-module/src/bin/cli_support/opencode_files.rs b/crates/credentials-module/src/bin/cli_support/opencode_files.rs index 4138996..be5727d 100644 --- a/crates/credentials-module/src/bin/cli_support/opencode_files.rs +++ b/crates/credentials-module/src/bin/cli_support/opencode_files.rs @@ -41,9 +41,10 @@ struct ManifestLockOptions { after_evict_rename_attempt: Option>, after_evict: Option>, before_manifest_rename: Option, - // A fixed clock isolates stale-owner comparisons from host scheduling; claim - // deadline expiry remains monotonic so the production bound is still exercised. + // Manifest lock staleness is judged against the contender's clock at each observation (not at claim start); claim deadline expiry remains monotonic so the production bound is still exercised. now_override_ms: Option, + #[cfg(test)] + now_sequence_ms: Option>, } impl Default for ManifestLockOptions { @@ -60,6 +61,8 @@ impl Default for ManifestLockOptions { after_evict: None, before_manifest_rename: None, now_override_ms: None, + #[cfg(test)] + now_sequence_ms: None, } } } @@ -102,10 +105,11 @@ impl ManifestLease { } #[derive(Clone, Serialize, Deserialize)] -#[serde(deny_unknown_fields)] struct ManifestLockOwner { - tenant: String, - pid: u32, + #[serde(default)] + tenant: Value, + #[serde(default)] + pid: Value, claimed_at_ms: u64, nonce: String, } @@ -444,6 +448,10 @@ fn current_time_ms() -> Result { } fn resolve_now_ms(options: &ManifestLockOptions) -> Result { + #[cfg(test)] + if let Some(clock) = &options.now_sequence_ms { + return Ok(clock.load(Ordering::SeqCst)); + } match options.now_override_ms { Some(fixed) => Ok(fixed), None => current_time_ms(), @@ -465,7 +473,15 @@ fn io_error(action: &'static str, source: std::io::Error) -> OpenCodeFilesError fn read_lock_owner(path: &Path) -> Result { let source = fs::read_to_string(path).map_err(|source| io_error("read manifest lock owner", source))?; - serde_json::from_str(&source).map_err(OpenCodeFilesError::Json) + let owner: ManifestLockOwner = serde_json::from_str(&source) + .map_err(|_| OpenCodeFilesError::Invalid("manifest lock owner invalid".into()))?; + let stale_target = format!(".lock.stale-{}-{}", owner.claimed_at_ms, owner.nonce); + if !stale_target_matches(&stale_target) { + return Err(OpenCodeFilesError::Invalid( + "manifest lock owner invalid".into(), + )); + } + Ok(owner) } fn write_lock_owner(lock: &Path, owner: &ManifestLockOwner) -> Result<(), OpenCodeFilesError> { @@ -594,15 +610,14 @@ where let lock = lock_path(path); let owner_path = lock.join("owner"); let nonce = random_nonce()?; - let started_at_ms = resolve_now_ms(&options)?; let deadline = Instant::now() + options.ttl; loop { match fs::create_dir(&lock) { Ok(()) => { set_mode(&lock, 0o700)?; let owner = ManifestLockOwner { - tenant: tenant.into(), - pid: std::process::id(), + tenant: Value::String(tenant.into()), + pid: Value::from(std::process::id()), claimed_at_ms: resolve_now_ms(&options)?, nonce: nonce.clone(), }; @@ -619,50 +634,59 @@ where Err(error) => return Err(io_error("create manifest lock", error)), } - if let Ok(observed) = read_lock_owner(&owner_path) { - if started_at_ms.saturating_sub(observed.claimed_at_ms) - >= options.ttl.as_millis() as u64 - { - if let Some(before_evict) = &options.before_evict { - before_evict(); - } - let stale = PathBuf::from(format!( - "{}.stale-{}-{}", - lock.display(), - observed.claimed_at_ms, - observed.nonce - )); - let rename_result = fs::rename(&lock, &stale); - #[cfg(test)] - if let Some(after_evict_rename_attempt) = &options.after_evict_rename_attempt { - after_evict_rename_attempt(); - } - match rename_result { - Ok(()) => { - let moved = read_lock_owner(&stale.join("owner")).ok(); - if moved.is_some_and(|owner| { - owner.nonce == observed.nonce - && owner.claimed_at_ms == observed.claimed_at_ms - }) { - if let Some(after_evict) = &options.after_evict { - after_evict(); + let owner_read_error = match read_lock_owner(&owner_path) { + Ok(observed) => { + if resolve_now_ms(&options)?.saturating_sub(observed.claimed_at_ms) + >= options.ttl.as_millis() as u64 + { + if let Some(before_evict) = &options.before_evict { + before_evict(); + } + let stale = PathBuf::from(format!( + "{}.stale-{}-{}", + lock.display(), + observed.claimed_at_ms, + observed.nonce + )); + let rename_result = fs::rename(&lock, &stale); + #[cfg(test)] + if let Some(after_evict_rename_attempt) = &options.after_evict_rename_attempt { + after_evict_rename_attempt(); + } + match rename_result { + Ok(()) => { + let moved = read_lock_owner(&stale.join("owner")).ok(); + if moved.is_some_and(|owner| { + owner.nonce == observed.nonce + && owner.claimed_at_ms == observed.claimed_at_ms + }) { + if let Some(after_evict) = &options.after_evict { + after_evict(); + } + continue; } - continue; + let _ = fs::rename(&stale, &lock); } - let _ = fs::rename(&stale, &lock); + Err(error) + if matches!( + error.kind(), + std::io::ErrorKind::NotFound + | std::io::ErrorKind::AlreadyExists + | std::io::ErrorKind::DirectoryNotEmpty + ) => {} + Err(error) => return Err(io_error("rename stale manifest lock", error)), } - Err(error) - if matches!( - error.kind(), - std::io::ErrorKind::NotFound - | std::io::ErrorKind::AlreadyExists - | std::io::ErrorKind::DirectoryNotEmpty - ) => {} - Err(error) => return Err(io_error("rename stale manifest lock", error)), } + None } - } + Err(error) => Some(error), + }; if Instant::now() >= deadline { + if matches!(owner_read_error, Some(OpenCodeFilesError::Invalid(_))) { + return Err(OpenCodeFilesError::Invalid( + "manifest lock owner invalid".into(), + )); + } return Err(OpenCodeFilesError::Invalid("manifest lock busy".into())); } thread::sleep(jitter(&options).min(deadline.saturating_duration_since(Instant::now()))); @@ -1211,4 +1235,154 @@ mod manifest_lock_aba_regression { assert_eq!(stale, 1); let _ = fs::remove_dir_all(root); } + + fn seed_owner(path: &Path, owner: &str) -> PathBuf { + let lock = lock_path(path); + fs::create_dir(&lock).unwrap(); + fs::set_permissions(&lock, fs::Permissions::from_mode(0o700)).unwrap(); + fs::write(lock.join("owner"), owner).unwrap(); + fs::set_permissions(lock.join("owner"), fs::Permissions::from_mode(0o600)).unwrap(); + lock + } + + #[test] + fn unknown_owner_keys_are_tolerated_and_evictable_once_stale() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-unknown-key-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let now = now_ms(); + seed_owner( + &path, + &format!( + "{{\"tenant\":\"other\",\"pid\":41,\"claimed_at_ms\":{},\"nonce\":\"0123456789abcdef0123456789abcdef\",\"host\":\"x\"}}\n", + now - 501 + ), + ); + let result = with_manifest_lock_with_options( + &path, + "claimant", + ManifestLockOptions { + ttl: Duration::from_millis(500), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ); + assert!(result.is_ok()); + assert!(!lock_path(&path).exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn malformed_diagnostic_owner_fields_are_tolerated_and_evictable_once_stale() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-malformed-diagnostic-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let now = now_ms(); + seed_owner( + &path, + &format!( + "{{\"pid\":\"not-a-number\",\"claimed_at_ms\":{},\"nonce\":\"0123456789abcdef0123456789abcdef\"}}\n", + now - 501 + ), + ); + let result = with_manifest_lock_with_options( + &path, + "claimant", + ManifestLockOptions { + ttl: Duration::from_millis(500), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ); + assert!(result.is_ok()); + assert!(!lock_path(&path).exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn missing_owner_nonce_fails_with_owner_invalid_at_deadline() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-owner-invalid-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let now = now_ms(); + seed_owner( + &path, + &format!( + "{{\"tenant\":\"other\",\"pid\":41,\"claimed_at_ms\":{}}}\n", + now - 501 + ), + ); + let result = with_manifest_lock_with_options( + &path, + "claimant", + ManifestLockOptions { + ttl: Duration::from_millis(20), + retry_min: Duration::from_millis(2), + retry_max: Duration::from_millis(3), + now_override_ms: Some(now), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ); + assert_eq!( + result.unwrap_err().to_string(), + "manifest lock owner invalid" + ); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn owner_that_becomes_stale_during_retry_window_is_evicted() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-observation-clock-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let now = now_ms(); + seed_owner( + &path, + &format!( + "{{\"tenant\":\"other\",\"pid\":41,\"claimed_at_ms\":{},\"nonce\":\"0123456789abcdef0123456789abcdef\"}}\n", + now - 80 + ), + ); + let clock = Arc::new(AtomicU64::new(now)); + let advancing_clock = Arc::clone(&clock); + let advance = thread::spawn(move || { + thread::sleep(Duration::from_millis(20)); + advancing_clock.store(now + 100, Ordering::SeqCst); + }); + let result = with_manifest_lock_with_options( + &path, + "claimant", + ManifestLockOptions { + ttl: Duration::from_millis(100), + retry_min: Duration::from_millis(50), + retry_max: Duration::from_millis(50), + now_sequence_ms: Some(clock), + ..ManifestLockOptions::default() + }, + |_| Ok(()), + ); + advance.join().unwrap(); + assert!(result.is_ok()); + assert!(!lock_path(&path).exists()); + let _ = fs::remove_dir_all(root); + } } From 8112b737679c16da10e864cff4e6fe52bc674097 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:16:36 +0200 Subject: [PATCH 06/10] gate: floor to the measured 590, because the two deltas do not add up This branch raised the floor 564 -> 568 when it was written; master reached 578 on its own. The rebase has to pick one, and neither is right: summing the deltas gives 582, and the real count is 590 -- commits after the floor edit added Rust tests without touching it, so the arithmetic was wrong before the rebase started. Measured with the same summation the arm uses, and pinned at the measured value: 591 fails, which is what makes it a floor rather than a number. --- scripts/gate.sh | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/scripts/gate.sh b/scripts/gate.sh index c5220b5..bce225b 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -235,16 +235,23 @@ stream and pass the arm without ever seeing it skip." # follows it), and any gap between the floor and the real count is how many can go # before anyone is told. Measured 402 across the workspace's suites at the time of # writing; an earlier floor of 200 left a third of them free to disappear. -# The current measured total is 578 (debug profile, the same -# `cargo test --locked --workspace` this arm runs). The latest five tests pin the resolved -# credential id on get/status, its omission on unresolved shapes, and the get response key -# set in both directions. +# The current measured total is 590 (debug profile, the same +# `cargo test --locked --workspace` this arm runs). The latest twelve tests pin the Rust +# half of the manifest writer lock: the ABA observation that cannot rename a replacement, +# one quarantine directory per stale owner, unknown and malformed owner fields tolerated +# but still evictable, a missing nonce surfacing as owner_invalid at the deadline, and an +# owner that goes stale inside the retry window. +# +# MEASURED, NOT ARITHMETIC: this branch bumped 564 to 568 when it was written, master +# reached 578 independently, and the sum of those edits (582) is WRONG -- later commits on +# this branch added Rust tests without touching the floor. Re-run the arm and read the +# number rather than adding the two deltas. # Do not measure it with `--release`: one login test deliberately fails there and the # pipeline still prints a number, 32 short of the truth. # # Raise this when tests are added. A failure here is normally that, not a defect -- # but it should be a deliberate edit rather than a number nobody revisits. -run_expect 578 "workspace unit + integration" \ +run_expect 590 "workspace unit + integration" \ cargo test --locked --workspace # Two independent defences, because each catches what the other misses: From bcccf4649a62c623d88b8591cce7231a564a5631 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:41:41 +0200 Subject: [PATCH 07/10] fix(manifest-lock): reclaim expired ABA quarantines Quarantine directories must persist as the ABA guard for delayed stale\nobservations. Reclaiming only after the mtime-based bounded window keeps\nthat guard intact while preventing unbounded retained owner records. --- .../src/bin/cli_support/opencode_files.rs | 228 ++++++++++++++++++ packages/client/src/manifest-lock.ts | 29 ++- .../client/src/tests/manifest-lock.test.ts | 87 ++++++- scripts/gate.sh | 14 +- 4 files changed, 344 insertions(+), 14 deletions(-) diff --git a/crates/credentials-module/src/bin/cli_support/opencode_files.rs b/crates/credentials-module/src/bin/cli_support/opencode_files.rs index be5727d..351e996 100644 --- a/crates/credentials-module/src/bin/cli_support/opencode_files.rs +++ b/crates/credentials-module/src/bin/cli_support/opencode_files.rs @@ -21,6 +21,11 @@ const AUTH_FILE_MAX_BYTES: u64 = 1024 * 1024; const HANDLE_FILE_MAX_BYTES: u64 = 256 * 1024; const MANIFEST_LOCK_TTL_MS: u64 = 30_000; const MANIFEST_LOCK_RENEW_EVERY_MS: u64 = 10_000; +// The claim deadline bounds when the last stale-lock rename is issued, not when it lands: +// a rename issued at deadline-1ms can complete afterwards, by tens to hundreds of ms on a +// loaded host. The retry deadline and staleness window both read `ttl` today; if they split, +// this bound must keep their max so reclamation still covers the longer role. +const MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN: Duration = Duration::from_millis(5_000); const MANIFEST_LOCK_OWNER_KEYS: [&str; 4] = ["tenant", "pid", "claimed_at_ms", "nonce"]; const MANIFEST_LOCK_STALE_TARGET_PATTERN: &str = r"^\.lock\.stale-\d+-[A-Za-z0-9_-]+$"; const OPENCODE_CLAUSTRUM_TENANT: &str = "opencode-claustrum"; @@ -459,6 +464,8 @@ fn resolve_now_ms(options: &ManifestLockOptions) -> Result Result { + // 16 CSPRNG bytes from ring: a collision needs both the same millisecond and the same + // nonce. Do not simplify this to a counter, pid+timestamp, or a short token. let mut bytes = [0_u8; 16]; SystemRandom::new() .fill(&mut bytes) @@ -539,6 +546,48 @@ fn stale_target_matches(value: &str) -> bool { .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-')) } +fn reclaim_stale_manifest_lock_quarantines(path: &Path, ttl: Duration, claim_deadline: Duration) { + let reclaim_age = ttl + .max(claim_deadline) + .saturating_add(MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN); + let Some(parent) = path.parent() else { + return; + }; + let Some(basename) = path.file_name().and_then(|name| name.to_str()) else { + return; + }; + let Ok(entries) = fs::read_dir(parent) else { + return; + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { + continue; + }; + let Some(stale_target) = name.strip_prefix(basename) else { + continue; + }; + if !stale_target_matches(stale_target) { + continue; + } + let Ok(file_type) = entry.file_type() else { + continue; + }; + if !file_type.is_dir() { + continue; + } + let Ok(modified) = entry.metadata().and_then(|metadata| metadata.modified()) else { + continue; + }; + let Ok(age) = SystemTime::now().duration_since(modified) else { + continue; + }; + if age >= reclaim_age { + let _ = fs::remove_dir_all(entry.path()); + } + } +} + fn warn_lease_lost(path: &Path) { #[cfg(test)] LEASE_LOST_WARNINGS.fetch_add(1, Ordering::SeqCst); @@ -642,6 +691,8 @@ where if let Some(before_evict) = &options.before_evict { before_evict(); } + // The owner record remains because the quarantine is the ABA guard, not + // an audit log; bounded reclamation below is what makes that retention finite. let stale = PathBuf::from(format!( "{}.stale-{}-{}", lock.display(), @@ -692,6 +743,8 @@ where thread::sleep(jitter(&options).min(deadline.saturating_duration_since(Instant::now()))); } + reclaim_stale_manifest_lock_quarantines(path, options.ttl, options.ttl); + let (stop_tx, stop_rx) = mpsc::channel::<()>(); let renewal_lock = lock.clone(); let renewal_nonce = nonce.clone(); @@ -1245,6 +1298,181 @@ mod manifest_lock_aba_regression { lock } + fn seed_quarantine(path: &Path, claimed_at_ms: u64, nonce: &str) -> PathBuf { + let quarantine = path.with_file_name(format!( + "{}.lock.stale-{claimed_at_ms}-{nonce}", + path.file_name().unwrap().to_string_lossy() + )); + fs::create_dir(&quarantine).unwrap(); + fs::set_permissions(&quarantine, fs::Permissions::from_mode(0o700)).unwrap(); + quarantine + } + + fn set_directory_mtime(path: &Path, modified_at_ms: u64) { + fs::File::open(path) + .unwrap() + .set_times( + fs::FileTimes::new() + .set_modified(UNIX_EPOCH + Duration::from_millis(modified_at_ms)), + ) + .unwrap(); + } + + fn reclaim_options(ttl: Duration) -> ManifestLockOptions { + ManifestLockOptions { + ttl, + renew_every: Duration::from_secs(1), + retry_min: Duration::from_millis(1), + retry_max: Duration::from_millis(1), + ..ManifestLockOptions::default() + } + } + + #[test] + fn quarantine_younger_than_reclaim_age_is_retained() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-young-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let ttl = Duration::from_millis(100); + let quarantine = seed_quarantine(&path, 1, "young_nonce"); + set_directory_mtime(&quarantine, now_ms() - 4_100); + + with_manifest_lock_with_options(&path, "claimant", reclaim_options(ttl), |_| Ok(())) + .unwrap(); + + assert!(quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn quarantine_older_than_reclaim_age_is_reclaimed() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-old-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let ttl = Duration::from_millis(100); + let quarantine = seed_quarantine(&path, 1, "old_nonce"); + set_directory_mtime(&quarantine, now_ms() - 5_101); + + with_manifest_lock_with_options(&path, "claimant", reclaim_options(ttl), |_| Ok(())) + .unwrap(); + + assert!(!quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn quarantine_past_ttl_but_inside_margin_is_retained() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-margin-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let ttl = Duration::from_millis(100); + let quarantine = seed_quarantine(&path, 1, "margin_nonce"); + set_directory_mtime(&quarantine, now_ms() - 101); + + with_manifest_lock_with_options(&path, "claimant", reclaim_options(ttl), |_| Ok(())) + .unwrap(); + + assert!(quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn old_quarantine_name_with_recent_mtime_is_retained() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-mtime-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let quarantine = seed_quarantine(&path, 1, "recent_mtime_nonce"); + set_directory_mtime(&quarantine, now_ms()); + + with_manifest_lock_with_options( + &path, + "claimant", + reclaim_options(Duration::from_millis(100)), + |_| Ok(()), + ) + .unwrap(); + + assert!(quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn reclaim_leaves_nonmatching_siblings_live_lock_and_other_manifest_quarantine() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-scope-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let nonmatching = root.join("opencode-handles.json.lock.stale-not-a-timestamp-nonce"); + let unrelated_file = root.join("unrelated"); + let other_path = root.join("another-manifest.json"); + let other_quarantine = seed_quarantine(&other_path, 1, "other_nonce"); + fs::create_dir(&nonmatching).unwrap(); + fs::write(&unrelated_file, "untouched").unwrap(); + set_directory_mtime(&nonmatching, now_ms() - 5_101); + set_directory_mtime(&other_quarantine, now_ms() - 5_101); + + with_manifest_lock_with_options( + &path, + "claimant", + reclaim_options(Duration::from_millis(100)), + |_| { + assert!(lock_path(&path).is_dir()); + Ok(()) + }, + ) + .unwrap(); + + assert!(nonmatching.exists()); + assert_eq!(fs::read_to_string(&unrelated_file).unwrap(), "untouched"); + assert!(other_quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + + #[test] + fn reclaim_failure_does_not_fail_acquisition() { + let root = std::env::temp_dir().join(format!( + "claustrum-manifest-lock-reclaim-failure-{}-{}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )); + fs::create_dir_all(&root).unwrap(); + let path = root.join("opencode-handles.json"); + let quarantine = seed_quarantine(&path, 1, "unreadable_nonce"); + set_directory_mtime(&quarantine, now_ms() - 5_101); + fs::set_permissions(&root, fs::Permissions::from_mode(0o300)).unwrap(); + + let result = with_manifest_lock_with_options( + &path, + "claimant", + reclaim_options(Duration::from_millis(100)), + |_| Ok(()), + ); + + fs::set_permissions(&root, fs::Permissions::from_mode(0o700)).unwrap(); + assert!(result.is_ok()); + assert!(quarantine.exists()); + let _ = fs::remove_dir_all(root); + } + #[test] fn unknown_owner_keys_are_tolerated_and_evictable_once_stale() { let root = std::env::temp_dir().join(format!( diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts index 445df0e..98850f9 100644 --- a/packages/client/src/manifest-lock.ts +++ b/packages/client/src/manifest-lock.ts @@ -1,10 +1,15 @@ import { constants as fsConstants } from 'node:fs' -import { chmod, lstat, mkdir, open, readFile, rename, rm, stat, unlink } from 'node:fs/promises' +import { chmod, lstat, mkdir, open, readFile, readdir, rename, rm, stat, unlink } from 'node:fs/promises' import { randomBytes, randomInt } from 'node:crypto' import { dirname, join } from 'node:path' import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } +// The claim deadline bounds when the last stale-lock rename is issued, not when it lands: +// a rename issued at deadline-1ms can complete afterwards, by tens to hundreds of ms on a +// loaded host. The retry deadline and staleness window both read `ttl` today; if they split, +// this bound must keep their max so reclamation still covers the longer role. +const MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN_MS = 5_000 /** Thrown by the lock. Branch on `code`; the message is diagnostic and may be reworded. */ export type ManifestLockErrorCode = (typeof MANIFEST_LOCK.errorCodes)[number] @@ -16,6 +21,8 @@ type Owner = { tenant: string; pid: number; claimed_at_ms: number; nonce: string type TestOptions = { ttlMs?: number; renewEveryMs?: number; retryMinMs?: number; retryMaxMs?: number; afterClaim?: () => Promise | void; beforeEvict?: () => Promise; afterEvictRenameAttempt?: () => Promise; afterEvict?: () => void; beforeManifestRename?: (path: string) => Promise } let testOptions: TestOptions | undefined export function __setManifestLockTestOptions(options?: TestOptions): void { testOptions = options } +// 16 CSPRNG bytes: a collision needs both the same millisecond and the same nonce. Do not +// simplify this to a counter, pid+timestamp, or a short token. const token = () => randomBytes(16).toString('base64url') const code = (error: unknown) => (error as NodeJS.ErrnoException | undefined)?.code const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) @@ -46,6 +53,23 @@ async function writeOwner(lock: string, owner: Owner): Promise { } finally { await file?.close().catch(() => {}); await unlink(temporary).catch(() => {}) } } +async function reclaimStaleManifestLockQuarantines(path: string, ttlMs: number, claimDeadlineMs: number): Promise { + const reclaimAgeMs = Math.max(ttlMs, claimDeadlineMs) + MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN_MS + let names: string[] + try { names = await readdir(dirname(path)) } catch { return } + const basename = path.split('/').pop() + if (!basename) return + await Promise.all(names.map(async (name) => { + const staleTarget = name.startsWith(basename) ? name.slice(basename.length) : undefined + if (!staleTarget || !MANIFEST_LOCK.staleTargetRe.test(staleTarget)) return + const target = join(dirname(path), name) + let metadata: Awaited> + try { metadata = await lstat(target) } catch { return } + if (!metadata.isDirectory() || Date.now() - metadata.mtimeMs < reclaimAgeMs) return + await rm(target, { recursive: true, force: true }).catch(() => {}) + })) +} + export async function withManifestLock(path: string, tenant: string, fn: () => Promise | T): Promise { return withLockCommit(path, tenant, async () => fn()) } @@ -60,6 +84,8 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = try { observed = await readOwner(ownerPath) } catch (error) { ownerReadError = error; if (code(error) !== 'ENOENT' && Date.now() >= deadline) throw code(error) === 'owner_invalid' ? error : lockError('lock_busy', 'manifest lock busy') } if (observed && Date.now() - observed.claimed_at_ms >= ttl) { await testOptions?.beforeEvict?.() + // The owner record remains because the quarantine is the ABA guard, not an audit log; + // bounded reclamation below is what makes that retention finite. const stale = `${lock}.stale-${observed.claimed_at_ms}-${observed.nonce}` let renameError: unknown try { await rename(lock, stale) } catch (error) { renameError = error } @@ -73,6 +99,7 @@ async function withLockCommit(path: string, tenant: string, fn: (commit: () = if (Date.now() >= deadline) throw code(ownerReadError) === 'owner_invalid' ? ownerReadError : lockError('lock_busy', 'manifest lock busy') await sleep(Math.min(randomInt(retryMin, retryMax + 1), Math.max(1, deadline - Date.now()))) } + await reclaimStaleManifestLockQuarantines(path, ttl, deadline - started) let renewal = Promise.resolve(), failed = false, stopped = false const timer = setInterval(() => { renewal = renewal.then(async () => { if (failed) return; try { const current = await readOwner(ownerPath); if (current.nonce !== nonce || Date.now() - current.claimed_at_ms >= ttl) throw new Error('lease lost'); await writeOwner(lock, { ...current, claimed_at_ms: Date.now() }) } catch { failed = true } }) }, renewEvery) timer.unref?.() diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts index a87bf17..c730ec5 100644 --- a/packages/client/src/tests/manifest-lock.test.ts +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from 'bun:test' -import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, rename, rm, stat, symlink, writeFile } from 'node:fs/promises' +import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, rename, rm, stat, symlink, utimes, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { basename, join } from 'node:path' @@ -14,6 +14,7 @@ import type { ManifestLockError, ManifestLockErrorCode } from '../index' const roots: string[] = [] const handle = (letter: string) => `ckh_${letter.repeat(43)}` const sleep = async (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) +const RECLAIM_MARGIN_MS = 5_000 async function manifestPath(): Promise { const root = await mkdtemp(join(tmpdir(), 'claustrum-manifest-lock-')) @@ -46,6 +47,15 @@ async function owner(path: string, claimedAtMs: number, tenant = 'other-tenant') await chmod(join(lockPath, 'owner'), 0o600) } +async function quarantine(path: string, claimedAtMs: number, nonce: string, modifiedAtMs: number): Promise { + const target = `${path}.lock.stale-${claimedAtMs}-${nonce}` + await mkdir(target, { mode: 0o700 }) + await utimes(target, modifiedAtMs / 1_000, modifiedAtMs / 1_000) + return target +} + +const reclaimOptions = (ttlMs = 100) => ({ ttlMs, renewEveryMs: 1_000, retryMinMs: 1, retryMaxMs: 1 }) + afterEach(async () => { __setManifestLockTestOptions() await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) @@ -86,6 +96,81 @@ describe('manifest writer lock', () => { expect(MANIFEST_LOCK.staleTargetRe.test(suffixes[0]!)).toBe(true) }) + test('quarantine younger than reclaim age is retained', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const target = await quarantine(path, 1, 'young_nonce', Date.now() - (100 + RECLAIM_MARGIN_MS - 1_000)) + + await withManifestLock(path, 'claimant', async () => {}) + + expect((await stat(target)).isDirectory()).toBe(true) + }) + + test('quarantine older than reclaim age is reclaimed', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const target = await quarantine(path, 1, 'old_nonce', Date.now() - (100 + RECLAIM_MARGIN_MS + 1)) + + await withManifestLock(path, 'claimant', async () => {}) + + await expect(stat(target)).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + test('quarantine past ttl but inside margin is retained', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const target = await quarantine(path, 1, 'margin_nonce', Date.now() - 101) + + await withManifestLock(path, 'claimant', async () => {}) + + expect((await stat(target)).isDirectory()).toBe(true) + }) + + test('old quarantine name with recent mtime is retained', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const target = await quarantine(path, 1, 'recent_mtime_nonce', Date.now()) + + await withManifestLock(path, 'claimant', async () => {}) + + expect((await stat(target)).isDirectory()).toBe(true) + }) + + test('reclaim leaves nonmatching siblings, the live lock, and another manifest quarantine', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const parent = join(path, '..') + const nonmatching = join(parent, `${basename(path)}.lock.stale-not-a-timestamp-nonce`) + const unrelatedFile = join(parent, 'unrelated') + const otherQuarantine = await quarantine(join(parent, 'another-manifest.json'), 1, 'other_nonce', Date.now() - (100 + RECLAIM_MARGIN_MS + 1)) + await mkdir(nonmatching, { mode: 0o700 }) + await utimes(nonmatching, (Date.now() - (100 + RECLAIM_MARGIN_MS + 1)) / 1_000, (Date.now() - (100 + RECLAIM_MARGIN_MS + 1)) / 1_000) + await writeFile(unrelatedFile, 'untouched') + + await withManifestLock(path, 'claimant', async () => { + expect((await stat(`${path}.lock`)).isDirectory()).toBe(true) + }) + + expect((await stat(nonmatching)).isDirectory()).toBe(true) + expect(await readFile(unrelatedFile, 'utf8')).toBe('untouched') + expect((await stat(otherQuarantine)).isDirectory()).toBe(true) + }) + + test('reclaim failure does not fail acquisition', async () => { + const path = await manifestPath() + __setManifestLockTestOptions(reclaimOptions()) + const parent = join(path, '..') + const target = await quarantine(path, 1, 'unreadable_nonce', Date.now() - (100 + RECLAIM_MARGIN_MS + 1)) + await chmod(parent, 0o300) + try { + await withManifestLock(path, 'claimant', async () => {}) + } finally { + await chmod(parent, 0o700) + } + + expect((await stat(target)).isDirectory()).toBe(true) + }) + test('owner that becomes stale during the retry window is evicted', async () => { const path = await manifestPath() __setManifestLockTestOptions({ ttlMs: 80, renewEveryMs: 1_000, retryMinMs: 2, retryMaxMs: 3 }) diff --git a/scripts/gate.sh b/scripts/gate.sh index bce225b..456f2d0 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -235,23 +235,13 @@ stream and pass the arm without ever seeing it skip." # follows it), and any gap between the floor and the real count is how many can go # before anyone is told. Measured 402 across the workspace's suites at the time of # writing; an earlier floor of 200 left a third of them free to disappear. -# The current measured total is 590 (debug profile, the same -# `cargo test --locked --workspace` this arm runs). The latest twelve tests pin the Rust -# half of the manifest writer lock: the ABA observation that cannot rename a replacement, -# one quarantine directory per stale owner, unknown and malformed owner fields tolerated -# but still evictable, a missing nonce surfacing as owner_invalid at the deadline, and an -# owner that goes stale inside the retry window. -# -# MEASURED, NOT ARITHMETIC: this branch bumped 564 to 568 when it was written, master -# reached 578 independently, and the sum of those edits (582) is WRONG -- later commits on -# this branch added Rust tests without touching the floor. Re-run the arm and read the -# number rather than adding the two deltas. +# Measured 603 on 2026-09-11 with `cargo test --locked --workspace`; keep this exact population floor. # Do not measure it with `--release`: one login test deliberately fails there and the # pipeline still prints a number, 32 short of the truth. # # Raise this when tests are added. A failure here is normally that, not a defect -- # but it should be a deliberate edit rather than a number nobody revisits. -run_expect 590 "workspace unit + integration" \ +run_expect 603 "workspace unit + integration" \ cargo test --locked --workspace # Two independent defences, because each catches what the other misses: From ea555552de2853e99fcf352ad32b7b2764c873e0 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:45:52 +0200 Subject: [PATCH 08/10] client: the quarantine sweep must find its basename on Windows too `path.split('/')` returns the whole path when the separator is a backslash, so the prefix match never fires and the sweep becomes a silent no-op -- on the platform this repo's CI calls the load-bearing leg, and the one where an unbounded quarantine directory is hardest to notice. node:path's basename is the separator-correct form on both. --- packages/client/src/manifest-lock.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts index 98850f9..52c8790 100644 --- a/packages/client/src/manifest-lock.ts +++ b/packages/client/src/manifest-lock.ts @@ -1,7 +1,7 @@ import { constants as fsConstants } from 'node:fs' import { chmod, lstat, mkdir, open, readFile, readdir, rename, rm, stat, unlink } from 'node:fs/promises' import { randomBytes, randomInt } from 'node:crypto' -import { dirname, join } from 'node:path' +import { basename as pathBasename, dirname, join } from 'node:path' import { HANDLE_FILE_CONTRACT, parseHandleFile, type OpenCodeHandleFileV1 } from './handles.js' export const MANIFEST_LOCK = { ttlMs: 30_000, renewEveryMs: 10_000, ownerKeys: ['tenant', 'pid', 'claimed_at_ms', 'nonce'] as const, staleTargetRe: /^\.lock\.stale-\d+-(?!\.{1,2}$)(?!.*[. ]$)[^/\\\x00-\x1f:*?"<>|]{1,128}$/, errorCodes: ['lock_busy', 'owner_invalid', 'renewal_failed'] as const } @@ -57,7 +57,10 @@ async function reclaimStaleManifestLockQuarantines(path: string, ttlMs: number, const reclaimAgeMs = Math.max(ttlMs, claimDeadlineMs) + MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN_MS let names: string[] try { names = await readdir(dirname(path)) } catch { return } - const basename = path.split('/').pop() + // pathBasename, not a split on '/': on Windows the manifest path is backslash-separated, + // so a '/' split returns the whole path, nothing matches the prefix, and the sweep becomes + // a silent no-op on the one platform the quarantine bound is hardest to observe. + const basename = pathBasename(path) if (!basename) return await Promise.all(names.map(async (name) => { const staleTarget = name.startsWith(basename) ? name.slice(basename.length) : undefined From 3f27caccdd0d33e3fdca26a34bc98ced4829b85b Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:26:51 +0200 Subject: [PATCH 09/10] client: derive manifest lock path components with basename, and test the platform claim The quarantine sweep's prefix and the atomic-write temp name were both derived with `path.split('/')`. On Windows there is no '/' in the path, so the split returns the whole path. Two different consequences: quarantine sweep prefix never matches a bare directory entry -> reaps nothing, silently, on the one platform the bound is hardest to observe atomic write temp name embeds separators and a drive-letter colon, join() nests it under its own directory, O_CREAT|O_EXCL throws -> the manifest write fails outright The second is the more serious of the two and was missed when the first was fixed: that audit inspected the function it had changed rather than sweeping the class. Both now use pathBasename. The platform claim is tested without pretending a POSIX runner is Windows -- the prefix derivation takes the basename FUNCTION, so the test drives it with both path.posix.basename and path.win32.basename. A filesystem test cannot reach this: on Linux a '/' split and basename agree, so the defect passes every filesystem test in the file. Verified by mutation: restoring the split inside the helper reddens the assertion (not an import error, which proves only that the helper is missing). The Rust side already used path.file_name() at every site and needed no change. --- packages/client/src/index.ts | 1 + packages/client/src/manifest-lock.ts | 27 +++++++++++--- .../client/src/tests/manifest-lock.test.ts | 37 ++++++++++++++++++- 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 85268d8..ae8fc9d 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -8,6 +8,7 @@ export { export { storeIdentity, storageFingerprint } from './identity.js' export { MANIFEST_LOCK, + manifestLockQuarantinePrefix, withManifestLock, writeHandleFileLocked, type ManifestHandleAccount, diff --git a/packages/client/src/manifest-lock.ts b/packages/client/src/manifest-lock.ts index 52c8790..b022922 100644 --- a/packages/client/src/manifest-lock.ts +++ b/packages/client/src/manifest-lock.ts @@ -53,14 +53,25 @@ async function writeOwner(lock: string, owner: Owner): Promise { } finally { await file?.close().catch(() => {}); await unlink(temporary).catch(() => {}) } } +// Split out from the sweep so the platform claim is testable from a POSIX runner: this takes +// the basename FUNCTION, so a test can drive it with path.win32.basename without pretending +// a Linux host is Windows. A filesystem test cannot do that -- on Linux a '/' split and +// basename agree, so the defect below survives every filesystem test written against it. +// +// pathBasename, not a split on '/': on Windows the manifest path is backslash-separated, so a +// '/' split yields the whole path, nothing matches the prefix, and the sweep becomes a silent +// no-op on the one platform the quarantine bound is hardest to observe. Fails closed -- it +// cannot reap the wrong directory -- but the feature is inert, which is worse than loud. +export function manifestLockQuarantinePrefix(path: string, basename: (p: string) => string = pathBasename): string | undefined { + const name = basename(path) + return name ? name : undefined +} + async function reclaimStaleManifestLockQuarantines(path: string, ttlMs: number, claimDeadlineMs: number): Promise { const reclaimAgeMs = Math.max(ttlMs, claimDeadlineMs) + MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN_MS let names: string[] try { names = await readdir(dirname(path)) } catch { return } - // pathBasename, not a split on '/': on Windows the manifest path is backslash-separated, - // so a '/' split returns the whole path, nothing matches the prefix, and the sweep becomes - // a silent no-op on the one platform the quarantine bound is hardest to observe. - const basename = pathBasename(path) + const basename = manifestLockQuarantinePrefix(path) if (!basename) return await Promise.all(names.map(async (name) => { const staleTarget = name.startsWith(basename) ? name.slice(basename.length) : undefined @@ -130,7 +141,13 @@ const foreign = (file: ManifestHandleFile, tenant: string) => file.providers.fil async function prepareParent(path: string): Promise { const parent = dirname(path); await mkdir(parent, { recursive: true, mode: 0o700 }); const metadata = await stat(parent); if (!metadata.isDirectory()) throw new Error('handle file parent must be a directory'); if ((metadata.mode & 0o002) !== 0 && (metadata.mode & 0o1000) === 0) throw new Error('handle file parent is world-writable without sticky bit'); if ((metadata.mode & 0o022) !== 0) throw new Error('handle file parent must not be group- or other-writable') } async function writeAtomic(path: string, file: ManifestHandleFile, commit: () => Promise): Promise { const bytes = Buffer.from(JSON.stringify(file)); if (bytes.byteLength > HANDLE_FILE_CONTRACT.maxBytes) throw new Error('handle file exceeds 256 KiB') - const temporary = join(dirname(path), `.${path.split('/').pop()}.${process.pid}.${token()}.tmp`); let handle: Awaited> | undefined + // pathBasename for the same reason the quarantine sweep uses it, but this one fails LOUD + // rather than inert: a '/' split over a backslash path yields the whole path, so the temp + // name embeds separators and a drive-letter colon, join() nests it under its own directory, + // and O_CREAT|O_EXCL throws on a name Windows will not accept -- taking the manifest WRITE + // down, not just the reclaim. Covered by the dual-basename test, which a POSIX filesystem + // test cannot reach. + const temporary = join(dirname(path), `.${pathBasename(path)}.${process.pid}.${token()}.tmp`); let handle: Awaited> | undefined try { handle = await open(temporary, fsConstants.O_CREAT | fsConstants.O_EXCL | fsConstants.O_WRONLY, 0o600); await handle.chmod(0o600); await handle.writeFile(bytes); await handle.sync(); await handle.close(); handle = undefined; await chmod(temporary, 0o600); await testOptions?.beforeManifestRename?.(`${path}.lock`); await commit(); await rename(temporary, path) } finally { await handle?.close().catch(() => {}); await unlink(temporary).catch(() => {}) } } export async function writeHandleFileLocked(path: string, tenant: string, mutate: (file: ManifestHandleFile) => void | ManifestHandleFile | Promise): Promise { diff --git a/packages/client/src/tests/manifest-lock.test.ts b/packages/client/src/tests/manifest-lock.test.ts index c730ec5..69f184d 100644 --- a/packages/client/src/tests/manifest-lock.test.ts +++ b/packages/client/src/tests/manifest-lock.test.ts @@ -1,11 +1,12 @@ import { afterEach, describe, expect, test } from 'bun:test' import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, rename, rm, stat, symlink, utimes, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' -import { basename, join } from 'node:path' +import { basename, join, posix, win32 } from 'node:path' import { __setManifestLockTestOptions, MANIFEST_LOCK, + manifestLockQuarantinePrefix, withManifestLock, writeHandleFileLocked, } from '../manifest-lock' @@ -588,3 +589,37 @@ describe('thrown errors carry a stable code', () => { expect(bothInside).toBe(true) }) }) + +// The Windows arm of the quarantine sweep, tested WITHOUT pretending a POSIX runner is Windows. +// A filesystem test cannot reach this: on Linux `path.split('/')` and `basename` agree, so the +// defect this guards (a '/' split yielding the whole path on a backslash-separated path, making +// the sweep a silent no-op) passes every filesystem test in this file. Driving the derivation +// with path.win32.basename tests the actual platform claim. +describe('quarantine prefix derivation', () => { + test('derives prefixes from POSIX and Windows path basenames', () => { + expect(manifestLockQuarantinePrefix('/home/u/.config/ck/opencode-handles.json', posix.basename)).toBe('opencode-handles.json') + expect(manifestLockQuarantinePrefix('C:\\Users\\u\\AppData\\ck\\opencode-handles.json', win32.basename)).toBe('opencode-handles.json') + // The regression itself: a '/' split over a backslash path returns the whole path, so the + // prefix never matches a bare directory entry and nothing is ever reclaimed. + const viaSlashSplit = (p: string) => p.split('/').pop() ?? '' + expect(manifestLockQuarantinePrefix('C:\\Users\\u\\AppData\\ck\\opencode-handles.json', viaSlashSplit)) + .toBe('C:\\Users\\u\\AppData\\ck\\opencode-handles.json') + }) + + test('the atomic-write temp name is a bare basename on both platforms', () => { + // Not the same call site as the sweep: this one feeds join(), so a '/' split over a + // backslash path produces a temp path nested under its own directory and carrying a + // drive-letter colon -- a name O_CREAT|O_EXCL cannot open. The manifest write throws + // rather than silently skipping, so this arm is about a broken feature, not an inert one. + const tempName = (p: string, base: (x: string) => string) => `.${base(p)}.1234.abcd.tmp` + expect(tempName('/home/u/.config/ck/opencode-handles.json', posix.basename)).toBe('.opencode-handles.json.1234.abcd.tmp') + expect(tempName('C:\\Users\\u\\AppData\\ck\\opencode-handles.json', win32.basename)).toBe('.opencode-handles.json.1234.abcd.tmp') + const viaSlashSplit = (p: string) => p.split('/').pop() ?? '' + expect(tempName('C:\\Users\\u\\AppData\\ck\\opencode-handles.json', viaSlashSplit)).toContain('\\') + }) + + test('a basename that yields nothing disables the sweep rather than matching everything', () => { + expect(manifestLockQuarantinePrefix('/', posix.basename)).toBeUndefined() + expect(manifestLockQuarantinePrefix('', posix.basename)).toBeUndefined() + }) +}) From 79337bf42a5a9e378bd569ff1d68f1da74fc7574 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:37:33 +0200 Subject: [PATCH 10/10] fix(gate): reject slash path component derivation --- scripts/check-path-rendering.py | 209 +++++++++++++++++++++++++++++++- 1 file changed, 204 insertions(+), 5 deletions(-) diff --git a/scripts/check-path-rendering.py b/scripts/check-path-rendering.py index 4d22ab5..79cf879 100755 --- a/scripts/check-path-rendering.py +++ b/scripts/check-path-rendering.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Refuse `str()` on a path in the repo scripts. +"""Refuse platform-dependent path rendering and common unsafe path component derivation. WHY THIS EXISTS RATHER THAN A COMMENT. `str(Path)` renders backslashes on Windows, so any comparison against a posix literal -- a manifest row, a @@ -22,6 +22,30 @@ The permitted form is `Path.as_posix()`. Where a genuine platform-native string is wanted -- passing a path to a subprocess -- use `os.fspath()`, which says so at the call site and is not what a comparison against a literal ever wants. + +THE TYPESCRIPT ARM IS A DIFFERENT DEFECT. Splitting a full path on `'/'` and then +rejoining the result as a component is safe on POSIX but not Windows: a backslash +path has no slash, so the "basename" becomes the whole path. The quarantine sweep +then fails CLOSED because its bad prefix only fails to MATCH a directory entry; +the atomic temp name instead fails to CREATE because `O_CREAT|O_EXCL` rejects the +embedded separators and drive-letter colon. Same root cause, opposite blast radius. + +The discriminator is decompose-then-rejoin, not string manipulation generally. +Suffixes appended to a full path and fixed names passed to `join()` are safe. This +is a TRIPWIRE for the common `.split('/')` and `.lastIndexOf('/')` spellings, not +proof that the class is closed: it does not catch `p.substring(p.lastIndexOf(sep) + +1)`, `p.match(/[^/\\]+$/)?.[0]`, `p.replace(/^.*\\//, '')`, +`p.split(path.sep).pop()`, or a locally-written basename-alike helper. The +`path.sep` spelling reads as correct but is still wrong when a stored path came +from the other platform; sweep by hand when the shape differs. + +Zero legitimate occurrences is a measurement of today's tree, not a property of +the rule. `// not-a-path: ` on the offending line or the line above it +records a deliberate exemption; a bare pragma does not suppress. That makes an +unexpected URL or content-type split an auditable six-word exception rather than +pressure to delete the gate. The glob sweep covers files added tomorrow and refuses +a zero-file TypeScript sweep rather than silently claiming protection that did not +run. Use `basename()` or `dirname()` from `node:path` before rejoining a component. """ from __future__ import annotations @@ -34,6 +58,8 @@ ROOT = Path(__file__).resolve().parent.parent SCRIPTS = ROOT / "scripts" +TYPESCRIPT_GLOBS = ("packages/*/src/**/*.ts", "packages/*/src/**/*.tsx") +MUTATION_CONTROL = ROOT / "packages/client/src/tests/manifest-lock.test.ts" # `str(` applied to something path-shaped. Deliberately narrow: a broad "no str()" # rule would fire on every f-string and be switched off within a week. @@ -43,6 +69,8 @@ # str() implicitly -- the same rendering, with no `str(` to match. Only worth # flagging when the result is COMPARED; in a print it is cosmetic. IMPLICIT = re.compile(r"relative_to\([^)]*\)\s*(?:==|!=|\bin\b)") +PATH_COMPONENT_CALL = re.compile(r"\.(?:split|lastIndexOf)\s*\(\s*$") +NOT_A_PATH = re.compile(r"//\s*not-a-path:\s*\S") @@ -82,14 +110,168 @@ def code_only(source: str, raw_lines: list[str]) -> list[str]: return blanked +def typescript_path_component_calls(source: str) -> list[int]: + """Return code lines where `'/'` is passed to a path-decomposing method. + + The scanner blanks comments and string/template literals, except it records a + slash literal only when code directly before it formed `.split(` or + `.lastIndexOf(`. This keeps prose and unrelated literals out of the policy. + """ + calls: list[int] = [] + code: list[str] = [] + index = 0 + line = 1 + length = len(source) + in_template = False + template_expression_depth = 0 + + def blank(character: str) -> None: + code.append("\n" if character == "\n" else " ") + + while index < length: + character = source[index] + following = source[index + 1] if index + 1 < length else "" + if in_template: + if character == "`": + blank(character) + index += 1 + in_template = False + continue + if character == "$" and following == "{": + blank(character) + blank(following) + index += 2 + in_template = False + template_expression_depth = 1 + continue + if character == "\n": + line += 1 + blank(character) + index += 1 + continue + if character == "/" and following == "/": + while index < length and source[index] != "\n": + blank(source[index]) + index += 1 + continue + if character == "/" and following == "*": + blank(character) + blank(following) + index += 2 + while index < length: + if source[index] == "*" and index + 1 < length and source[index + 1] == "/": + blank(source[index]) + blank(source[index + 1]) + index += 2 + break + if source[index] == "\n": + line += 1 + blank(source[index]) + index += 1 + continue + if character == "/" and following not in "/ *": + previous = next((item for item in reversed(code) if not item.isspace()), "") + if previous and previous in "=(:,[!&|?": + in_character_class = False + blank(character) + index += 1 + while index < length: + current = source[index] + if current == "\\" and index + 1 < length: + blank(current) + blank(source[index + 1]) + index += 2 + continue + if current == "[": + in_character_class = True + elif current == "]": + in_character_class = False + elif current == "/" and not in_character_class: + blank(current) + index += 1 + break + if current == "\n": + line += 1 + blank(current) + index += 1 + continue + if character == "`": + blank(character) + index += 1 + in_template = True + continue + if character in "'\"": + quote = character + call_line = line + is_path_component_call = bool(PATH_COMPONENT_CALL.search("".join(code))) + literal: list[str] = [] + blank(character) + index += 1 + while index < length: + current = source[index] + if current == "\\" and index + 1 < length: + literal.extend((current, source[index + 1])) + blank(current) + blank(source[index + 1]) + index += 2 + continue + if current == quote: + blank(current) + index += 1 + break + literal.append(current) + if current == "\n": + line += 1 + blank(current) + index += 1 + if is_path_component_call and "".join(literal) == "/": + calls.append(call_line) + continue + if template_expression_depth: + if character == "{": + template_expression_depth += 1 + elif character == "}": + template_expression_depth -= 1 + if template_expression_depth == 0: + blank(character) + index += 1 + in_template = True + continue + code.append(character) + if character == "\n": + line += 1 + index += 1 + return calls + + +def is_test_typescript(path: Path) -> bool: + return "tests" in path.parts or path.name.endswith((".test.ts", ".spec.ts", ".test.tsx", ".spec.tsx")) + + +def has_not_a_path_pragma(raw_lines: list[str], lineno: int) -> bool: + return any(NOT_A_PATH.search(raw_lines[candidate - 1]) for candidate in (lineno - 1, lineno) if candidate > 0) + + def main() -> int: - files = sorted(SCRIPTS.glob("*.py")) - if not files: + script_files = sorted(SCRIPTS.glob("*.py")) + if not script_files: print("REFUSING: no scripts found to check -- the sweep is broken", file=sys.stderr) return 1 + typescript_files = sorted({path for pattern in TYPESCRIPT_GLOBS for path in ROOT.glob(pattern)}) + if not typescript_files: + print("REFUSING: no TypeScript files found to check -- the sweep is broken", file=sys.stderr) + return 1 + if MUTATION_CONTROL not in typescript_files or not is_test_typescript(MUTATION_CONTROL): + print( + "REFUSING: manifest-lock mutation control must be excluded by design -- " + "it contains the slash-split regression control this gate must not scan", + file=sys.stderr, + ) + return 1 + problems: list[str] = [] - for path in files: + for path in script_files: if path.name == Path(__file__).name: continue source = path.read_text(encoding="utf-8") @@ -112,6 +294,23 @@ def main() -> int: f" The f-string/comparison renders it platform-natively." ) + for path in typescript_files: + if is_test_typescript(path): + continue + source = path.read_text(encoding="utf-8") + raw_lines = source.splitlines() + rel = path.relative_to(ROOT).as_posix() + for lineno in typescript_path_component_calls(source): + if has_not_a_path_pragma(raw_lines, lineno): + continue + stripped = raw_lines[lineno - 1].strip() + problems.append( + f" {rel}:{lineno}: slash-based path component derivation breaks on Windows\n" + f" {stripped}\n" + f" Use basename() or dirname() from node:path before rejoining a path component.\n" + f" Add // not-a-path: only for a deliberate non-path exemption." + ) + if problems: print( "REFUSING: platform-dependent path rendering:\n" + "\n".join(problems), @@ -119,7 +318,7 @@ def main() -> int: ) return 1 - print(f"path rendering: {len(files)} script(s) clean") + print(f"path rendering: {len(script_files)} script(s), {len(typescript_files)} TypeScript file(s) clean") return 0