From 0c0db7a7e6286edef02fd641576768f2411b7a86 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:06:58 +0200 Subject: [PATCH 1/5] test(fs): pin every publication helper's contract before folding them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine helpers in eight modules stage a sibling and rename it over a state-plane record, and between them they answer the parts that matter — exclusive creation, staged-file mode, cleanup, how far durability is pushed — four different ways. Before folding them into one primitive, each contract gets a test that can fail, because a fold whose only proof is "it still compiles" ships whichever answers the folder happened to prefer. What each new test pins: - `context`, `status`, `message` (create-once and replace), `request`, `harness_state`: the target ends up carrying the complete new bytes, no staged sibling survives, and the published mode is whatever an ordinary `fs::write` produces. That last assertion is the umask-independent way to say "as readable as any other file this process writes", and it is the property the fold tightens to 0600. - `harness_state`: the staging directory is the one the CALLER named, and an unusable one fails the publication rather than falling back to staging beside the record — that fallback would put a staged name inside the replicated `agents` namespace (INVARIANTS row 29, HC-R05). Proven with a staging path that is a regular file, so no uid can satisfy it. - `status`: the staging-name grammar `{prefix}.tmp-{pid}-{counter}`, which six catalog and publication walkers match by prefix. - `park`: the strict end of the range — a parent directory that cannot be opened for its fsync FAILS the publication. - `delivery_ledger`, `driver_diagnostic`: the lenient end of the same edge — the record is already renamed into place when the directory sync runs, so a parent that cannot be opened for it still reports success. The two directory-sync tests are the ones that make the levels genuinely different rather than a comment, and they are meaningful only for a non-root uid: the hermetic gate runs as the sandbox's unprivileged build user (uid 1000 `nixbld`), and a local root run skips the edge instead of asserting what root cannot observe. Also fixes a test that had stopped testing: the ledger's residue filter matched `name.ends_with(".tmp")`, which is this helper's own current spelling of the staging name rather than the grammar every other module uses, so it would go silently vacuous the moment the spelling changed. It now filters by prefix. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af --- src/context.rs | 33 +++++++++++++++++++++++++++ src/delivery_ledger.rs | 43 ++++++++++++++++++++++++++++++++++- src/driver_diagnostic.rs | 41 +++++++++++++++++++++++++++++++++ src/harness_state.rs | 48 +++++++++++++++++++++++++++++++++++++++ src/message.rs | 49 ++++++++++++++++++++++++++++++++++++++++ src/park.rs | 47 ++++++++++++++++++++++++++++++++++++++ src/request.rs | 36 +++++++++++++++++++++++++++++ src/status.rs | 39 ++++++++++++++++++++++++++++++++ 8 files changed, 335 insertions(+), 1 deletion(-) diff --git a/src/context.rs b/src/context.rs index 87d95264..9c7c87cc 100644 --- a/src/context.rs +++ b/src/context.rs @@ -222,6 +222,39 @@ fn iso_utc_now() -> String { mod tests { use super::*; + /// [`write_atomic`]'s publication contract, pinned before the helper is folded into one + /// shared primitive: the target ends up carrying the complete new bytes, no staged sibling + /// survives a successful write, and the published file's mode is whatever an ordinary write + /// produces. That last assertion is the umask-independent way to say "as readable as any + /// other file this process writes" — the property the fold deliberately tightens. + #[test] + fn a_context_write_replaces_the_target_and_leaves_no_staged_sibling() { + use std::os::unix::fs::PermissionsExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let dir = context_dir(tmp.path()); + let path = now_file(&dir); + write_atomic(&path, "first\n").unwrap(); + write_atomic(&path, "second\n").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "second\n"); + + let reference = dir.join("ordinary-write"); + fs::write(&reference, b"x").unwrap(); + let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode(&path), + mode(&reference), + "the context record is published at the mode an ordinary write produces" + ); + + let staged = fs::read_dir(&dir) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".ctx")) + .collect::>(); + assert!(staged.is_empty(), "staging residue left behind: {staged:?}"); + } + #[test] fn missing_context_reads_empty() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/delivery_ledger.rs b/src/delivery_ledger.rs index e767cee4..f67e23eb 100644 --- a/src/delivery_ledger.rs +++ b/src/delivery_ledger.rs @@ -786,11 +786,52 @@ mod tests { let residue = fs::read_dir(tmp.path()) .unwrap() .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) - .filter(|name| name.ends_with(".tmp")) + // By staging-name PREFIX, not by a `.tmp` suffix: the suffix is this helper's own + // spelling, and a filter that only matches its current spelling stops testing the + // moment the spelling changes. + .filter(|name| name.starts_with(".delivery-ledger")) .collect::>(); assert!(residue.is_empty(), "temp residue left behind: {residue:?}"); } + /// The directory sync is best-effort today: the record is already renamed into place when it + /// runs, so a parent that cannot be opened for syncing does not fail the publication. + /// + /// Pinned because it is a real difference from `park`, which fails that same edge, and a + /// difference nothing observes is a difference nobody can review changing. The denial is + /// real only for a non-root uid; the hermetic gate runs as the sandbox's unprivileged build + /// user, and a local root run skips the edge instead of asserting what root cannot observe. + #[test] + fn a_directory_that_cannot_be_synced_does_not_fail_the_publication() { + use std::os::unix::fs::PermissionsExt as _; + + if unsafe { libc::geteuid() } == 0 { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("agent"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join(LEDGER_FILE); + let record = Record { + schema: LEDGER_SCHEMA.to_owned(), + harness: "codex".to_owned(), + agent: "h.worker".to_owned(), + runtime_id: "runtime".to_owned(), + entries: Vec::new(), + }; + + // Write and traverse, but not read: staging and renaming still work, opening the + // directory to sync it does not. + fs::set_permissions(&dir, fs::Permissions::from_mode(0o300)).unwrap(); + let published = atomic_json(&path, &record); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)).unwrap(); + assert!( + published.is_ok(), + "the ledger's directory sync is best-effort: {published:?}" + ); + assert!(path.exists(), "the record still landed"); + } + #[test] fn positive_evidence_never_downgrades() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/driver_diagnostic.rs b/src/driver_diagnostic.rs index 47a237e7..2ff10f34 100644 --- a/src/driver_diagnostic.rs +++ b/src/driver_diagnostic.rs @@ -961,4 +961,45 @@ mod tests { .collect::>(); assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); } + + /// Same best-effort directory sync as `delivery_ledger`, and pinned for the same reason: the + /// record is already renamed into place when the sync runs, so a parent that cannot be opened + /// for it reports success. `park` fails that edge, so the two levels genuinely differ, and a + /// difference nothing observes is a difference nobody can review changing. Real only for a + /// non-root uid; the hermetic gate runs as the sandbox's unprivileged build user, and a local + /// root run skips the edge instead of asserting what root cannot observe. + #[test] + fn a_directory_that_cannot_be_synced_does_not_fail_the_publication() { + use std::os::unix::fs::PermissionsExt as _; + + if unsafe { libc::geteuid() } == 0 { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let agent = tmp.path().join("agents/h/worker"); + fs::create_dir_all(&agent).unwrap(); + let path = path(&agent); + let record = Record { + schema: SCHEMA.to_owned(), + driver: Driver::OpenCode, + stage: Stage::Seed, + reason: Reason::UnknownStatus, + source: Source::StatusSnapshot, + producer_version: None, + support: Support::Supported, + observed_at: 100, + recovery: RECOVERY.to_owned(), + }; + + // Write and traverse, but not read: staging and renaming still work, opening the + // directory to sync it does not. + fs::set_permissions(&agent, fs::Permissions::from_mode(0o300)).unwrap(); + let published = atomic_json(&path, &record); + fs::set_permissions(&agent, fs::Permissions::from_mode(0o700)).unwrap(); + assert!( + published.is_ok(), + "the diagnostic's directory sync is best-effort: {published:?}" + ); + assert!(path.exists(), "the record still landed"); + } } diff --git a/src/harness_state.rs b/src/harness_state.rs index 69f49b4f..56b9acee 100644 --- a/src/harness_state.rs +++ b/src/harness_state.rs @@ -916,6 +916,54 @@ static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); mod tests { use super::*; + /// [`write_json_atomic`]'s contract, pinned before the helper is folded into one shared + /// primitive: one newline-terminated JSON record, replaced whole, and staged in the directory + /// the CALLER named. The staging directory is not a detail — the harness-context record + /// stages in the catalog control plane precisely because a staged name inside the replicated + /// `agents` namespace becomes a durable replicated key (INVARIANTS row 29, HC-R05) — so an + /// unusable staging directory must fail the publication instead of quietly staging beside the + /// record. Proven with a staging path that is a regular file, which no uid can turn into a + /// directory. + #[test] + fn a_record_is_one_json_line_staged_in_the_directory_the_caller_named() { + use std::os::unix::fs::PermissionsExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let agent_dir = tmp.path().join("agents/hetz/worker"); + let path = harness_state_path(&agent_dir); + let staging = tmp.path().join("staging"); + let record = serde_json::json!({"schema": "test"}); + + write_json_atomic(&path, &record, &staging, ".harness-state").unwrap(); + write_json_atomic(&path, &record, &staging, ".harness-state").unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"{\"schema\":\"test\"}\n"); + + let reference = agent_dir.join("ordinary-write"); + fs::write(&reference, b"x").unwrap(); + let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode(&path), + mode(&reference), + "the driver record is published at the mode an ordinary write produces" + ); + + for dir in [&agent_dir, &staging] { + let residue = fs::read_dir(dir) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".harness-state.tmp-")) + .collect::>(); + assert!(residue.is_empty(), "staging residue in {dir:?}: {residue:?}"); + } + + let blocked = tmp.path().join("blocked"); + fs::write(&blocked, b"not a directory").unwrap(); + assert!( + write_json_atomic(&path, &record, &blocked, ".harness-state").is_err(), + "an unusable staging directory must fail the publication, not fall back" + ); + } + fn writer(dir: &Path) -> Writer { Writer::new(dir, "hetz.worker", "codex", Some("worker".to_string())) } diff --git a/src/message.rs b/src/message.rs index 092b4250..9d667841 100644 --- a/src/message.rs +++ b/src/message.rs @@ -2569,6 +2569,55 @@ fn remove_inbox_duplicate(source: &Path, filename: &str) -> anyhow::Result<()> { mod tests { use super::*; + /// [`atomic_create_file`]'s create-once contract, pinned before the helper is folded into one + /// shared primitive. It is a hardlink, not a rename, and that is the whole point: the first + /// publication wins, a second reports `false` instead of replacing the winner's bytes, and + /// neither leaves a staged sibling behind for the four `.message.tmp-` walkers to trip over. + #[test] + fn a_create_once_message_write_keeps_the_first_bytes_and_reports_the_duplicate() { + use std::os::unix::fs::PermissionsExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("nested/record.json"); + assert!(atomic_create_file(&path, b"first").unwrap()); + assert!(!atomic_create_file(&path, b"second").unwrap()); + assert_eq!(fs::read(&path).unwrap(), b"first"); + + let reference = path.with_file_name("ordinary-write"); + fs::write(&reference, b"x").unwrap(); + let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode(&path), + mode(&reference), + "the record is published at the mode an ordinary write produces" + ); + + let residue = fs::read_dir(path.parent().unwrap()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".message.tmp-")) + .collect::>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + } + + /// [`atomic_replace_file`]'s contract, pinned for the same fold: replacement is unconditional + /// and complete, and the staged sibling never survives it. + #[test] + fn a_replacing_message_write_lands_the_complete_bytes_and_leaves_no_staged_sibling() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join(SENT_HEAD); + atomic_replace_file(&path, b"first").unwrap(); + atomic_replace_file(&path, b"second").unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"second"); + + let residue = fs::read_dir(tmp.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".message.tmp-")) + .collect::>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + } + #[test] fn filename_grammar() { assert!(is_message_filename("1784649988123-abc23z.md")); diff --git a/src/park.rs b/src/park.rs index 13310e9e..cd71f4ac 100644 --- a/src/park.rs +++ b/src/park.rs @@ -470,6 +470,53 @@ pub fn take_unpark_requests(dir: &Path) -> (Vec, Vec) { mod tests { use super::*; + /// [`write_json_atomically`] is the strict end of the publication range and the reference the + /// shared primitive is unified onto: the record's bytes are fsynced before the rename and the + /// parent directory entry after it, and a directory that cannot be opened for that sync + /// FAILS the publication rather than reporting success for a write that may not survive a + /// crash. That failure edge is the only observable difference between a best-effort sync and + /// a strict one, which is why it is pinned here. + /// + /// The mode denial is real only for a non-root uid; the hermetic gate runs as the sandbox's + /// unprivileged build user, and a local root run skips the edge rather than asserting + /// something root cannot observe. + #[test] + fn a_park_publication_is_owner_only_and_fails_when_its_directory_cannot_be_synced() { + use std::os::unix::fs::PermissionsExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("park"); + let path = marker_path(&dir, "runtime"); + write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park.").unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"{\"schema\":\"test\"}\n"); + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the park marker is published owner-only" + ); + + let residue = fs::read_dir(&dir) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".park.")) + .collect::>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + + if unsafe { libc::geteuid() } == 0 { + return; + } + // Write and traverse, but not read: staging and renaming still work, opening the + // directory to sync it does not. + fs::set_permissions(&dir, fs::Permissions::from_mode(0o300)).unwrap(); + let refused = + write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park."); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)).unwrap(); + assert!( + refused.is_err(), + "a directory that cannot be synced must fail the publication" + ); + } + fn projection(dir: &Path) -> ParkProjection { ParkProjection::current(dir.to_path_buf()).expect("this process has a generation") } diff --git a/src/request.rs b/src/request.rs index c1cfa53e..0319ce5a 100644 --- a/src/request.rs +++ b/src/request.rs @@ -504,3 +504,39 @@ fn read_inbox_or_archive(agent_dir: &Path, filename: &str) -> anyhow::Result>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + } +} diff --git a/src/status.rs b/src/status.rs index 39250a7f..7e5d1e80 100644 --- a/src/status.rs +++ b/src/status.rs @@ -317,6 +317,45 @@ mod tests { use super::*; use std::time::{Duration as Dur, SystemTime}; + /// [`write_atomic`]'s publication contract, pinned before the helper is folded into one + /// shared primitive. The staging name is part of the contract, not decoration: six catalog + /// and publication walkers match `.status.tmp-` by prefix, so the grammar + /// `{prefix}.tmp-{pid}-{counter}` is asserted here as well as by those walkers' own tests. + #[test] + fn a_status_write_replaces_the_target_and_leaves_no_staged_sibling() { + use std::os::unix::fs::PermissionsExt as _; + + let tmp = tempfile::tempdir().unwrap(); + let path = status_path(tmp.path()); + write_atomic(&path, "available\n").unwrap(); + write_atomic(&path, "working\n").unwrap(); + assert_eq!(fs::read_to_string(&path).unwrap(), "working\n"); + + let staging = tmp_name(); + let (pid, counter) = staging + .strip_prefix(".status.tmp-") + .and_then(|rest| rest.split_once('-')) + .expect("the staging grammar is `.status.tmp--`"); + assert!(pid.bytes().all(|byte| byte.is_ascii_digit()) && !pid.is_empty()); + assert!(counter.bytes().all(|byte| byte.is_ascii_digit()) && !counter.is_empty()); + + let reference = tmp.path().join("ordinary-write"); + fs::write(&reference, b"x").unwrap(); + let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode(&path), + mode(&reference), + "the status record is published at the mode an ordinary write produces" + ); + + let residue = fs::read_dir(tmp.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(".status.tmp-")) + .collect::>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + } + #[test] fn missing_is_offline() { let tmp = tempfile::tempdir().unwrap(); From f99b1a1638d4507a6b04d8723b6a5bce140da488 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:17:07 +0200 Subject: [PATCH 2/5] refactor(fs): one fsatomic module, and publish the state plane at 0600 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine helpers in eight modules staged a sibling and renamed it over a state-plane record. `src/fsatomic.rs` is the one primitive they now share: `replace` and `create_once`, two `Durability` levels, and a `Staging` prefix the caller still owns. −95 lines, and — the point — the parts that decide whether a publication is safe are now decided once. Three things stay with the caller on purpose: serialization (three of the absorbed helpers serialize differently, so a `json` entry point here would carry the difference rather than remove it), the staging-name prefix (six catalog walkers match `.status.tmp-`, four sent-record walkers `.message.tmp-`, and `harness_context::is_legacy_staging_name` parses `.harness-context.tmp--` digit by digit under INVARIANTS row 29), and error context (`io::Result` throughout, so every callsite keeps its exact context string). TWO DELIBERATE BEHAVIOUR CHANGES, neither of them a dedupe: 1. Six absorbed callers publish at 0600 instead of 0644-and-umask: `context`, `status`, `message` (both helpers), `request`, `harness_state`. Three were already 0600 (`delivery_ledger`, `driver_diagnostic`, `park`), and 0600 is what the two most recently reviewed publishers in this tree chose. These are per-agent records in per-agent directories; the readers are st2 and the agent itself, both the same uid on this fleet. The one pair that carries a cross-repository contract is `harness-state`/`harness-context`, whose names a replication transport's include list is expected to name (HC-R05): NO such transport runs on the fleet today — that is what `DQ-C1`/`DQ-H2` record as unmeasurable — the include list is not in this repository, and the only cross-uid reader of the live catalog is root's resource collector, which 0600 does not affect. The mode is now stated in HC-T08 so whoever adopts a transport sees the constraint. Pinned per module by a `0600` assertion that replaces the "same mode as an ordinary write" assertion the previous commit pinned. 2. The directory fsync is strict everywhere: `delivery_ledger` and `driver_diagnostic` used to swallow it. `Ledger::persist` can now fail on an edge it used to report success for (the bytes still land — the rename happens first — so the error is a report about durability, not about contents), and `driver_diagnostic::Publisher::persist` logs one warning line where it used to log none. The alternative was keeping a durability level nothing can be made to fail, which is a promise no test can keep. Both flips are pinned by the tests the previous commit wrote against the old behaviour. Two latent bugs die with the fold. `context` staged at `.ctx.tmp-{pid}-{now_ms()}`, so two writers in the same millisecond shared a staging path and the second truncated the first's staged bytes; the shared primitive counts instead of reading a clock and creates exclusively, which turns that race into an impossible `AlreadyExists`. And every absorbed caller now refuses a planted staging path instead of following it — `fs::write` follows symlinks, `O_EXCL` does not. `message` keeps `tmp_name`, now delegating to `fsatomic::staging_name`, because two of its publications stage their own file for reasons the module does not cover (one renames into a name it must search for, the other compares bytes on collision) and both must draw from the SAME counter as everything else staging under `.message.tmp-` in that directory. Two counters for one prefix collide, which is the failure exclusive creation then reports as an error. Six publishers deliberately stay out, each holding a strictly stronger primitive than a path-based stage-and-rename: `catalog_transaction` and `agent_publish` (retained control-directory fd, `EXDEV` fault injection, error classification), `resource_profile` and `event` (`openat`/`renameat` against a directory capability), and `codex_app_server` (chmods its state dir to 0700 per write, pretty JSON — its doc claimed a control-dir fd it does not have; corrected). `pretrust` stays out for the opposite reason: it publishes files st2 does not own, where tightening a foreign config's mode has no argument behind it. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af --- docs/vrs/08-harness-context/requirements.md | 6 +- src/codex_app_server.rs | 9 + src/context.rs | 47 ++- src/delivery_ledger.rs | 58 ++-- src/driver_diagnostic.rs | 89 ++--- src/fsatomic.rs | 349 ++++++++++++++++++++ src/harness_state.rs | 52 ++- src/lib.rs | 3 + src/message.rs | 83 ++--- src/park.rs | 35 +- src/request.rs | 46 +-- src/status.rs | 66 ++-- 12 files changed, 555 insertions(+), 288 deletions(-) create mode 100644 src/fsatomic.rs diff --git a/docs/vrs/08-harness-context/requirements.md b/docs/vrs/08-harness-context/requirements.md index c82153bf..c5860c65 100644 --- a/docs/vrs/08-harness-context/requirements.md +++ b/docs/vrs/08-harness-context/requirements.md @@ -103,7 +103,11 @@ record. other purposes, buys nothing a named include entry does not — bounded by st2-side test that pins the names it expects (HC-R05), and by the fact that no correctness property here depends on the transport at all: everything works - with no replication, and remote visibility is what is lost. + with no replication, and remote visibility is what is lost. The pair is + published mode `0600`, like every other state-plane record, so a transport + that reads the catalog as a different unprivileged uid needs that decision + taken deliberately rather than inherited from a default; no such transport + runs today, which is what leaves the wire-cost half of `DQ-C1` unmeasured. ## Requirements diff --git a/src/codex_app_server.rs b/src/codex_app_server.rs index c7e444b8..a474f488 100644 --- a/src/codex_app_server.rs +++ b/src/codex_app_server.rs @@ -2975,6 +2975,15 @@ fn acquire_owner_lock(state_dir: &Path) -> Result { Ok(file) } +/// Stage-and-rename this runtime's own state files, deliberately NOT through the shared +/// `fsatomic` primitive. +/// +/// Two reasons, and neither is the durability: [`secure_dir`] re-establishes `0700` on the state +/// directory on EVERY write, because this directory holds the Codex socket and its owner lock and +/// a mode drifting open there is a takeover surface rather than a readability question; and the +/// bytes are `to_writer_pretty`, because these files are read by humans debugging a live runtime. +/// The shared primitive owns neither, and giving it a "chmod the parent" mode would hand every +/// caller a directory-permissions policy it has no business having. fn atomic_json(path: &Path, value: &impl Serialize) -> Result<()> { let parent = path.parent().context("state file has no parent")?; secure_dir(parent)?; diff --git a/src/context.rs b/src/context.rs index 9c7c87cc..733c6a03 100644 --- a/src/context.rs +++ b/src/context.rs @@ -181,20 +181,19 @@ fn trim_trailing_period(s: &str) -> &str { s.strip_suffix('.').unwrap_or(s) } -/// Atomic write: tmp sibling + rename. +/// Atomic write: staged sibling + rename. +/// +/// The staging name used to end in `now_ms()`, so two writers in the same millisecond shared one +/// staging path and the second truncated the first's staged bytes before renaming it. The shared +/// primitive names the sibling with a counter and creates it exclusively, which turns that race +/// into an impossible `AlreadyExists`. fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> { - let dir = path.parent().unwrap_or(Path::new(".")); - fs::create_dir_all(dir)?; - let tmp = dir.join(format!( - ".ctx.tmp-{}-{}", - std::process::id(), - message::now_ms() - )); - fs::write(&tmp, content)?; - if let Err(e) = fs::rename(&tmp, path) { - let _ = fs::remove_file(&tmp); - return Err(e.into()); - } + crate::fsatomic::replace( + path, + content.as_bytes(), + crate::fsatomic::Staging::new(".ctx"), + crate::fsatomic::Durability::Rename, + )?; Ok(()) } @@ -222,11 +221,13 @@ fn iso_utc_now() -> String { mod tests { use super::*; - /// [`write_atomic`]'s publication contract, pinned before the helper is folded into one - /// shared primitive: the target ends up carrying the complete new bytes, no staged sibling - /// survives a successful write, and the published file's mode is whatever an ordinary write - /// produces. That last assertion is the umask-independent way to say "as readable as any - /// other file this process writes" — the property the fold deliberately tightens. + /// [`write_atomic`]'s publication contract: the target ends up carrying the complete new + /// bytes, no staged sibling survives a successful write, and the record is owner-only. + /// + /// The mode is the deliberate change of the fold onto `fsatomic` — this record used to be + /// published at whatever an ordinary write produces (`0644` under the fleet's umask), and the + /// shared primitive stages exclusively at `0600`. An agent's context is its own working + /// notes; nothing but st2 and that agent has ever read it. #[test] fn a_context_write_replaces_the_target_and_leaves_no_staged_sibling() { use std::os::unix::fs::PermissionsExt as _; @@ -237,14 +238,10 @@ mod tests { write_atomic(&path, "first\n").unwrap(); write_atomic(&path, "second\n").unwrap(); assert_eq!(fs::read_to_string(&path).unwrap(), "second\n"); - - let reference = dir.join("ordinary-write"); - fs::write(&reference, b"x").unwrap(); - let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; assert_eq!( - mode(&path), - mode(&reference), - "the context record is published at the mode an ordinary write produces" + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the context record is published owner-only" ); let staged = fs::read_dir(&dir) diff --git a/src/delivery_ledger.rs b/src/delivery_ledger.rs index f67e23eb..24576960 100644 --- a/src/delivery_ledger.rs +++ b/src/delivery_ledger.rs @@ -12,7 +12,6 @@ //! The translation itself lives outside this module, behind the one seam in [`Ledger::open`]. use std::fs; -use std::io::Write as _; use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result}; @@ -608,37 +607,18 @@ pub(crate) fn asserted_entries(path: &Path) -> Result { /// Durable replacement: file bytes reach disk before rename, then the directory entry is synced. /// -/// The temp file is created exclusively at `0600` under a name unique to this process and write, -/// so a stale or adversarial path cannot be followed or truncated and two writes cannot collide. +/// The directory sync is now STRICT — a parent that cannot be opened for it fails +/// [`Ledger::persist`], where it used to be swallowed. A ledger whose directory entry may not +/// survive a crash is exactly the state the ledger exists to prevent being invisible, and a +/// failure edge nothing can observe is a guarantee nothing can review. fn atomic_json(path: &Path, value: &impl Serialize) -> Result<()> { - use std::os::unix::fs::OpenOptionsExt as _; - use std::sync::atomic::{AtomicU64, Ordering}; - - static WRITE: AtomicU64 = AtomicU64::new(0); - let bytes = serde_json::to_vec(value)?; - let parent = path.parent().context("ledger file has no parent")?; - fs::create_dir_all(parent)?; - let temp = parent.join(format!( - ".delivery-ledger.{}.{}.tmp", - std::process::id(), - WRITE.fetch_add(1, Ordering::Relaxed) - )); - let mut file = fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(&temp)?; - file.write_all(&bytes)?; - file.sync_all()?; - drop(file); - if let Err(error) = fs::rename(&temp, path) { - let _ = fs::remove_file(&temp); - return Err(error.into()); - } - if let Ok(dir) = fs::File::open(parent) { - let _ = dir.sync_all(); - } + crate::fsatomic::replace( + path, + &bytes, + crate::fsatomic::Staging::new(".delivery-ledger"), + crate::fsatomic::Durability::FsyncFileAndDir, + )?; Ok(()) } @@ -794,15 +774,15 @@ mod tests { assert!(residue.is_empty(), "temp residue left behind: {residue:?}"); } - /// The directory sync is best-effort today: the record is already renamed into place when it - /// runs, so a parent that cannot be opened for syncing does not fail the publication. + /// The directory sync is strict since the fold onto `fsatomic`: a parent that cannot be + /// opened for it fails the publication, where it used to be swallowed. This is the deliberate + /// behaviour change of that fold on this caller — `Ledger::persist` can now fail on an edge it + /// previously reported success for. /// - /// Pinned because it is a real difference from `park`, which fails that same edge, and a - /// difference nothing observes is a difference nobody can review changing. The denial is - /// real only for a non-root uid; the hermetic gate runs as the sandbox's unprivileged build + /// Real only for a non-root uid; the hermetic gate runs as the sandbox's unprivileged build /// user, and a local root run skips the edge instead of asserting what root cannot observe. #[test] - fn a_directory_that_cannot_be_synced_does_not_fail_the_publication() { + fn a_directory_that_cannot_be_synced_fails_the_publication() { use std::os::unix::fs::PermissionsExt as _; if unsafe { libc::geteuid() } == 0 { @@ -826,9 +806,11 @@ mod tests { let published = atomic_json(&path, &record); fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)).unwrap(); assert!( - published.is_ok(), - "the ledger's directory sync is best-effort: {published:?}" + published.is_err(), + "the ledger's directory sync is strict: {published:?}" ); + // The bytes did land — the rename happens before the sync — so the failure is a report + // about durability, not about the record's contents. assert!(path.exists(), "the record still landed"); } diff --git a/src/driver_diagnostic.rs b/src/driver_diagnostic.rs index 2ff10f34..12b98e1c 100644 --- a/src/driver_diagnostic.rs +++ b/src/driver_diagnostic.rs @@ -7,7 +7,6 @@ use std::array; use std::fs; -use std::io::Write as _; use std::path::{Path, PathBuf}; use std::time::{SystemTime, UNIX_EPOCH}; @@ -624,62 +623,22 @@ fn emit( ); } -/// Create the staging sibling exclusively at `0600`. -/// -/// An existing regular file, a directory, or a symlink an agent planted at this path is refused -/// with `AlreadyExists` rather than followed or truncated. This directory is agent-writable, so -/// that refusal is the whole security property. -fn create_staging(path: &Path) -> std::io::Result { - use std::os::unix::fs::OpenOptionsExt as _; - - fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(path) -} - /// Durable replacement: the record's bytes reach disk before the rename and the directory entry is /// synced after it. /// -/// The staging file is created exclusively at `0600` under a name unique to this process *and* -/// write, so a stale or adversarial path cannot be followed or truncated and two writes to the -/// same agent directory cannot collide. Its sibling in `delivery_ledger` documents why that -/// matters; this helper used to be the one that did not do it. +/// The directory sync is now STRICT — a parent that cannot be opened for it makes this fail, where +/// it used to be swallowed. [`Publisher::persist`] already logs a failed publication and carries +/// on, so the visible consequence is one warning line, and the alternative was keeping a +/// durability level nothing can be made to fail. fn atomic_json(path: &Path, value: &impl Serialize) -> std::io::Result<()> { - use std::sync::atomic::{AtomicU64, Ordering}; - - static WRITE: AtomicU64 = AtomicU64::new(0); - - let Some(parent) = path.parent() else { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "diagnostic path has no parent", - )); - }; - fs::create_dir_all(parent)?; - let tmp = parent.join(format!( - ".driver-diagnostic.tmp-{}-{}", - std::process::id(), - WRITE.fetch_add(1, Ordering::Relaxed) - )); - let mut file = create_staging(&tmp)?; - let staged = (|| -> std::io::Result<()> { - serde_json::to_writer(&mut file, value).map_err(std::io::Error::other)?; - file.write_all(b"\n")?; - file.sync_all() - })(); - drop(file); - if let Err(error) = staged.and_then(|()| fs::rename(&tmp, path)) { - let _ = fs::remove_file(&tmp); - return Err(error); - } - // Best-effort, exactly like `delivery_ledger`: the record is already durable, and a directory - // that cannot be synced must not turn a published diagnostic into a reported failure. - if let Ok(dir) = fs::File::open(parent) { - let _ = dir.sync_all(); - } - Ok(()) + let mut bytes = serde_json::to_vec(value).map_err(std::io::Error::other)?; + bytes.push(b'\n'); + crate::fsatomic::replace( + path, + &bytes, + crate::fsatomic::Staging::new(".driver-diagnostic"), + crate::fsatomic::Durability::FsyncFileAndDir, + ) } fn now_ms() -> u64 { @@ -921,7 +880,7 @@ mod tests { let planted = agent.join(".driver-diagnostic.tmp-planted"); symlink(&victim, &planted).unwrap(); - let refused = create_staging(&planted).unwrap_err(); + let refused = crate::fsatomic::create_staging(&planted).unwrap_err(); assert_eq!( refused.kind(), std::io::ErrorKind::AlreadyExists, @@ -962,14 +921,16 @@ mod tests { assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); } - /// Same best-effort directory sync as `delivery_ledger`, and pinned for the same reason: the - /// record is already renamed into place when the sync runs, so a parent that cannot be opened - /// for it reports success. `park` fails that edge, so the two levels genuinely differ, and a - /// difference nothing observes is a difference nobody can review changing. Real only for a - /// non-root uid; the hermetic gate runs as the sandbox's unprivileged build user, and a local - /// root run skips the edge instead of asserting what root cannot observe. + /// The directory sync is strict since the fold onto `fsatomic`: a parent that cannot be opened + /// for it fails the publication, where it used to be swallowed. This is the deliberate + /// behaviour change of that fold on this caller — [`Publisher::persist`] already logs a failed + /// publication and carries on, so the visible consequence is one warning line for a record + /// whose bytes did land. + /// + /// Real only for a non-root uid; the hermetic gate runs as the sandbox's unprivileged build + /// user, and a local root run skips the edge instead of asserting what root cannot observe. #[test] - fn a_directory_that_cannot_be_synced_does_not_fail_the_publication() { + fn a_directory_that_cannot_be_synced_fails_the_publication() { use std::os::unix::fs::PermissionsExt as _; if unsafe { libc::geteuid() } == 0 { @@ -997,9 +958,11 @@ mod tests { let published = atomic_json(&path, &record); fs::set_permissions(&agent, fs::Permissions::from_mode(0o700)).unwrap(); assert!( - published.is_ok(), - "the diagnostic's directory sync is best-effort: {published:?}" + published.is_err(), + "the diagnostic's directory sync is strict: {published:?}" ); + // The bytes did land — the rename happens before the sync — so the failure is a report + // about durability, not about the record's contents. assert!(path.exists(), "the record still landed"); } } diff --git a/src/fsatomic.rs b/src/fsatomic.rs new file mode 100644 index 00000000..2c3cd0e4 --- /dev/null +++ b/src/fsatomic.rs @@ -0,0 +1,349 @@ +//! One stage-and-rename publication primitive for the state-plane records. +//! +//! Nine helpers in eight modules each staged a sibling and renamed it over the target, with their +//! own temp-name scheme and their own answer to the parts that actually matter: whether the +//! staging file is created exclusively, what mode it carries, and whether a failed staging is +//! cleaned up. The line count was never the problem — deciding the same security question nine +//! times and getting nine answers was, most visibly as the defect fixed in #502 (a predictable +//! staging name, created non-exclusively, in an agent-writable directory). +//! +//! Three things stay with the caller, deliberately: +//! +//! - **Serialization.** Callers pass bytes. The absorbed helpers serialize three different ways +//! (`to_vec`, `to_vec` plus a newline, `to_writer` plus a newline); a `json` entry point here +//! would have to reproduce all three to keep every record byte-identical, so the module would +//! carry the difference instead of removing it. +//! - **The staging-name prefix.** The grammar is `{prefix}.tmp-{pid}-{counter}` and it is +//! load-bearing, not decorative: `.status.tmp-` is matched by prefix in six catalog and +//! publication walkers, `.message.tmp-` in four sent-record walkers, and +//! `.harness-context.tmp--` is parsed digit by digit by +//! [`crate::harness_context::is_legacy_staging_name`] under INVARIANTS row 29. A module that +//! invented its own names would silently turn staged files into durable replicated keys. +//! - **Error context.** Every function returns [`io::Result`], never `anyhow`, so each callsite +//! keeps the exact context string it already had. +//! +//! Five publishers deliberately do **not** use this module, because each holds a strictly stronger +//! primitive than a path-based stage-and-rename can express: +//! `catalog_transaction::atomic_replace_file` and `agent_publish::{atomic_write_spec, +//! atomic_publish_staged_bundle}` publish through a retained control-directory fd with `EXDEV` +//! fault injection and error classification; `resource_profile::atomic_replace_at` and +//! `event::write_record` use `openat`/`renameat` against a retained directory capability; +//! `codex_app_server::atomic_json` chmods its state directory to `0700` on every write and emits +//! pretty JSON. `pretrust::write_atomic` also stays out for the opposite reason: it publishes +//! files st2 does not own (`~/.claude.json`, `~/.codex/config.toml`), where tightening a foreign +//! config's mode has no argument behind it. + +use std::fs; +use std::io::{self, Write as _}; +use std::os::unix::fs::OpenOptionsExt as _; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +/// One counter for the whole process: the staging name only has to be unique per write, and the +/// pid already separates processes. +static STAGING_SERIAL: AtomicU64 = AtomicU64::new(0); + +/// How far a publication is pushed before it reports success. +/// +/// There is no `FsyncFile` variant: no caller wants one, and an in-process test cannot observe an +/// fsync that nothing can be made to fail, so shipping the variant would ship a promise no test +/// keeps. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Durability { + /// Stage, then rename. A concurrent reader sees the old bytes or the new bytes and never a + /// partial file; a crash can lose the write entirely. Correct where a lost write reads as + /// "unknown" rather than as something false. + Rename, + /// Stage, fsync the staged file, rename, then fsync the parent directory, so both the bytes + /// and the directory entry naming them survive a crash. Strict: a parent directory that + /// cannot be opened for that sync fails the publication. + FsyncFileAndDir, +} + +/// Where the staged sibling lives and what it is called. +/// +/// `dir` exists for exactly one caller pair: the `harness-state` record stages beside itself, +/// while `harness-context` stages in the catalog control plane, because a staged name inside the +/// replicated `agents` namespace becomes a durable replicated key (HC-R05). +pub(crate) struct Staging<'a> { + prefix: &'a str, + dir: Option<&'a Path>, +} + +impl<'a> Staging<'a> { + /// Stage beside the target, under `{prefix}.tmp-{pid}-{counter}`. + pub(crate) const fn new(prefix: &'a str) -> Self { + Self { prefix, dir: None } + } + + /// Stage in `dir` instead of beside the target. The caller owns the guarantee that `dir` is on + /// the target's filesystem — a rename across filesystems is `EXDEV`, not an atomic publish. + pub(crate) const fn in_dir(self, dir: &'a Path) -> Self { + Self { + prefix: self.prefix, + dir: Some(dir), + } + } +} + +/// Replace `path` with `bytes`, atomically for readers of `path`. +pub(crate) fn replace( + path: &Path, + bytes: &[u8], + staging: Staging<'_>, + durability: Durability, +) -> io::Result<()> { + let parent = parent_of(path)?; + let staged = prepare(parent, &staging)?; + let landed = (|| -> io::Result<()> { + let mut file = create_staging(&staged)?; + file.write_all(bytes)?; + if durability == Durability::FsyncFileAndDir { + file.sync_all()?; + } + drop(file); + fs::rename(&staged, path) + })(); + if let Err(error) = landed { + // Best-effort: the staging name is unique per write, so a leftover is inert rather than a + // path a later write could collide with. + let _ = fs::remove_file(&staged); + return Err(error); + } + if durability == Durability::FsyncFileAndDir { + fs::File::open(parent)?.sync_all()?; + } + Ok(()) +} + +/// Publish `bytes` at `path` only if nothing holds that name yet, reporting whether this call is +/// the one that created it. +/// +/// A hardlink rather than a rename, because the name being taken is the answer the caller wants +/// rather than a failure: both callers use the boolean to tell a replay from a first publication. +/// One durability level, because both callers have one — an fsync arm here would have no caller +/// and therefore no test. +pub(crate) fn create_once(path: &Path, bytes: &[u8], staging: Staging<'_>) -> io::Result { + let parent = parent_of(path)?; + let staged = prepare(parent, &staging)?; + let mut file = create_staging(&staged)?; + let written = file.write_all(bytes); + drop(file); + let created = match written.and_then(|()| fs::hard_link(&staged, path)) { + Ok(()) => Ok(true), + // `hard_link` reports `AlreadyExists` for a taken name, but a target that is already a + // regular file is the same answer whatever the error says. + Err(_) if path.is_file() => Ok(false), + Err(error) => Err(error), + }; + let _ = fs::remove_file(&staged); + created +} + +/// Create the staging sibling exclusively at `0600`. +/// +/// An existing regular file, a directory, or a symlink an agent planted at this path is refused +/// with `AlreadyExists` rather than followed or truncated. State-plane records live in +/// agent-writable directories, so that refusal is the whole security property; `0600` is the other +/// half, because a record readable by anyone who can reach the directory is a record an +/// unprivileged reader can harvest. +pub(crate) fn create_staging(path: &Path) -> io::Result { + fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) +} + +/// Make both directories exist and name the staging file. Creating the target's parent is part of +/// every absorbed helper's contract: publication paths are derived, not authored, so the first +/// write to an agent is what creates its directory. +fn prepare(parent: &Path, staging: &Staging<'_>) -> io::Result { + fs::create_dir_all(parent)?; + let dir = staging.dir.unwrap_or(parent); + if dir != parent { + fs::create_dir_all(dir)?; + } + Ok(dir.join(staging_name(staging.prefix))) +} + +/// One staged name, `{prefix}.tmp-{pid}-{counter}`. +/// +/// Exposed because two message publications stage their own file for reasons this module does not +/// cover — one renames into a name it has to search for, the other compares bytes on collision — +/// and they must draw from the SAME counter as everything else that stages under `.message.tmp-` +/// in the same directory. Two counters for one prefix would collide, which is precisely the class +/// of failure exclusive creation then reports as an error. +pub(crate) fn staging_name(prefix: &str) -> String { + format!( + "{prefix}.tmp-{}-{}", + std::process::id(), + STAGING_SERIAL.fetch_add(1, Ordering::Relaxed) + ) +} + +/// A bare relative name stages in the current directory, which is where its rename lands too. +fn parent_of(path: &Path) -> io::Result<&Path> { + match path.parent() { + Some(parent) if parent.as_os_str().is_empty() => Ok(Path::new(".")), + Some(parent) => Ok(parent), + None => Err(io::Error::new( + io::ErrorKind::InvalidInput, + "publication path has no parent directory", + )), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::fs::{PermissionsExt as _, symlink}; + + fn mode_of(path: &Path) -> u32 { + fs::metadata(path).unwrap().permissions().mode() & 0o777 + } + + fn entries(dir: &Path) -> Vec { + let mut names = fs::read_dir(dir) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect::>(); + names.sort(); + names + } + + /// The grammar six catalog walkers and `harness_context::is_legacy_staging_name` parse. Two + /// successive names differ in the counter, not in a clock reading: the helper this replaced in + /// `context` used `now_ms()`, so two writers in the same millisecond shared a staging path and + /// the second truncated the first's staged bytes. + #[test] + fn a_staging_name_is_prefix_pid_counter_and_never_repeats() { + let first = staging_name(".status"); + let second = staging_name(".status"); + assert_ne!(first, second); + for name in [&first, &second] { + let rest = name + .strip_prefix(".status.tmp-") + .expect("the grammar is `{prefix}.tmp-{pid}-{counter}`"); + let (pid, counter) = rest.split_once('-').expect("pid and counter are separated"); + assert_eq!(pid, std::process::id().to_string()); + assert!(!counter.is_empty() && counter.bytes().all(|byte| byte.is_ascii_digit())); + } + } + + #[test] + fn a_replacement_lands_owner_only_and_leaves_no_staged_sibling() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("nested/record"); + replace(&path, b"first", Staging::new(".record"), Durability::Rename).unwrap(); + replace( + &path, + b"second", + Staging::new(".record"), + Durability::FsyncFileAndDir, + ) + .unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"second"); + assert_eq!(mode_of(&path), 0o600); + assert_eq!(entries(path.parent().unwrap()), vec!["record".to_owned()]); + } + + #[test] + fn a_create_once_publication_keeps_the_first_bytes_and_reports_the_duplicate() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("nested/record"); + assert!(create_once(&path, b"first", Staging::new(".record")).unwrap()); + assert!(!create_once(&path, b"second", Staging::new(".record")).unwrap()); + assert_eq!(fs::read(&path).unwrap(), b"first"); + assert_eq!(mode_of(&path), 0o600); + assert_eq!(entries(path.parent().unwrap()), vec!["record".to_owned()]); + } + + /// The staging directory is a caller argument because one caller pair needs it, so an unusable + /// one must fail the publication instead of quietly staging beside the record — that fallback + /// would put a staged name inside the replicated namespace (HC-R05). Proven with a staging + /// path that is a regular file, which no uid can turn into a directory. + #[test] + fn staging_happens_in_the_directory_the_caller_named() { + let tmp = tempfile::tempdir().unwrap(); + let record = tmp.path().join("agents/host/worker/harness-context"); + let staging = tmp.path().join("control/staging"); + replace( + &record, + b"{}\n", + Staging::new(".harness-context").in_dir(&staging), + Durability::Rename, + ) + .unwrap(); + assert_eq!(fs::read(&record).unwrap(), b"{}\n"); + assert!(entries(&staging).is_empty()); + assert_eq!( + entries(record.parent().unwrap()), + vec!["harness-context".to_owned()] + ); + + let blocked = tmp.path().join("blocked"); + fs::write(&blocked, b"not a directory").unwrap(); + assert!( + replace( + &record, + b"{}\n", + Staging::new(".harness-context").in_dir(&blocked), + Durability::Rename, + ) + .is_err() + ); + } + + /// The refusal that makes a staging file in an agent-writable directory safe: an agent can + /// plant a symlink there, and following it would aim st2's own privilege at a file the agent + /// cannot write. + #[test] + fn a_planted_symlink_at_the_staging_path_is_refused_not_followed() { + let tmp = tempfile::tempdir().unwrap(); + let victim = tmp.path().join("authored"); + fs::write(&victim, b"authored bytes").unwrap(); + let planted = tmp.path().join(".record.tmp-planted"); + symlink(&victim, &planted).unwrap(); + + let refused = create_staging(&planted).unwrap_err(); + assert_eq!(refused.kind(), io::ErrorKind::AlreadyExists); + assert_eq!(fs::read(&victim).unwrap(), b"authored bytes"); + } + + /// [`Durability::FsyncFileAndDir`] is observable exactly here: a parent directory that cannot + /// be opened for its sync fails the publication, while [`Durability::Rename`] — which never + /// opens the directory — succeeds against the same directory. Real only for a non-root uid; + /// the hermetic gate runs as the sandbox's unprivileged build user, and a local root run skips + /// the edge instead of asserting what root cannot observe. + #[test] + fn a_directory_that_cannot_be_synced_fails_only_the_strict_level() { + if unsafe { libc::geteuid() } == 0 { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("agent"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("record"); + + // Write and traverse, but not read: staging and renaming still work, opening the directory + // to sync it does not. + fs::set_permissions(&dir, fs::Permissions::from_mode(0o300)).unwrap(); + let strict = replace( + &path, + b"strict", + Staging::new(".record"), + Durability::FsyncFileAndDir, + ); + let lenient = replace(&path, b"lenient", Staging::new(".record"), Durability::Rename); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)).unwrap(); + + assert_eq!( + strict.unwrap_err().kind(), + io::ErrorKind::PermissionDenied, + "a directory that cannot be synced must fail a strict publication" + ); + assert!(lenient.is_ok(), "{lenient:?}"); + assert_eq!(fs::read(&path).unwrap(), b"lenient"); + } +} diff --git a/src/harness_state.rs b/src/harness_state.rs index 56b9acee..f7e62dfc 100644 --- a/src/harness_state.rs +++ b/src/harness_state.rs @@ -718,21 +718,12 @@ pub(crate) fn write_json_atomic( ) -> anyhow::Result<()> { let mut bytes = serde_json::to_vec(value)?; bytes.push(b'\n'); - if let Some(dir) = path.parent() { - fs::create_dir_all(dir)?; - } - fs::create_dir_all(staging_dir)?; - let tmp = staging_dir.join(format!( - "{tmp_prefix}.tmp-{}-{}", - std::process::id(), - TMP_COUNTER.fetch_add(1, Ordering::Relaxed) - )); - fs::write(&tmp, &bytes)?; - // rename over the target — atomic on the same filesystem. - if let Err(e) = fs::rename(&tmp, path) { - let _ = fs::remove_file(&tmp); // best-effort cleanup - return Err(e.into()); - } + crate::fsatomic::replace( + path, + &bytes, + crate::fsatomic::Staging::new(tmp_prefix).in_dir(staging_dir), + crate::fsatomic::Durability::Rename, + )?; Ok(()) } @@ -916,14 +907,19 @@ static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); mod tests { use super::*; - /// [`write_json_atomic`]'s contract, pinned before the helper is folded into one shared - /// primitive: one newline-terminated JSON record, replaced whole, and staged in the directory - /// the CALLER named. The staging directory is not a detail — the harness-context record - /// stages in the catalog control plane precisely because a staged name inside the replicated - /// `agents` namespace becomes a durable replicated key (INVARIANTS row 29, HC-R05) — so an - /// unusable staging directory must fail the publication instead of quietly staging beside the - /// record. Proven with a staging path that is a regular file, which no uid can turn into a - /// directory. + /// [`write_json_atomic`]'s contract: one newline-terminated JSON record, replaced whole, + /// staged in the directory the CALLER named, and owner-only. The staging directory is not a + /// detail — the harness-context record stages in the catalog control plane precisely because a + /// staged name inside the replicated `agents` namespace becomes a durable replicated key + /// (INVARIANTS row 29, HC-R05) — so an unusable staging directory must fail the publication + /// instead of quietly staging beside the record. Proven with a staging path that is a regular + /// file, which no uid can turn into a directory. + /// + /// The mode is the deliberate change of the fold onto `fsatomic`: this pair used to be + /// published at whatever an ordinary write produces (`0644` under the fleet's umask). These + /// are the two records a replication transport's include list names (HC-R05), and no such + /// transport runs on the fleet today (`DQ-C1`/`DQ-H2`), so nothing reads them as another uid; + /// the tightening is recorded against HC-T08 for whoever adopts one. #[test] fn a_record_is_one_json_line_staged_in_the_directory_the_caller_named() { use std::os::unix::fs::PermissionsExt as _; @@ -937,14 +933,10 @@ mod tests { write_json_atomic(&path, &record, &staging, ".harness-state").unwrap(); write_json_atomic(&path, &record, &staging, ".harness-state").unwrap(); assert_eq!(fs::read(&path).unwrap(), b"{\"schema\":\"test\"}\n"); - - let reference = agent_dir.join("ordinary-write"); - fs::write(&reference, b"x").unwrap(); - let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; assert_eq!( - mode(&path), - mode(&reference), - "the driver record is published at the mode an ordinary write produces" + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the driver record is published owner-only" ); for dir in [&agent_dir, &staging] { diff --git a/src/lib.rs b/src/lib.rs index ccd81be0..c5b9e121 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,6 +27,9 @@ pub mod event; pub mod exec_backend; pub mod expand; pub mod flapping; +/// Private on purpose: every consumer is a sibling module in this crate, and the publishers that +/// deliberately keep their own primitive are documented in the module itself. +mod fsatomic; pub mod harness_context; pub mod harness_state; pub mod harness_version; diff --git a/src/message.rs b/src/message.rs index 9d667841..79119303 100644 --- a/src/message.rs +++ b/src/message.rs @@ -16,7 +16,6 @@ use std::fs::{self, File, OpenOptions}; use std::io::{Read, Write as _}; use std::os::unix::fs::OpenOptionsExt as _; use std::path::{Component, Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; use anyhow::Context as _; @@ -400,14 +399,12 @@ pub fn materialize_message_once( result } -static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); +/// The staging-name prefix for everything this module stages. Four sent-record and inbox walkers +/// match `.message.tmp-` by prefix, so the name is a contract with them, not a local detail. +const TMP_PREFIX: &str = ".message"; fn tmp_name() -> String { - format!( - ".message.tmp-{}-{}", - std::process::id(), - TMP_COUNTER.fetch_add(1, Ordering::Relaxed) - ) + crate::fsatomic::staging_name(TMP_PREFIX) } /// Read a canonical entry that was already returned by `read_dir`. Removing a message concurrently @@ -2306,29 +2303,24 @@ fn write_sent_head(root: &Path, head: &SentHead) -> anyhow::Result<()> { atomic_replace_file(&root.join(SENT_HEAD), &serde_json::to_vec(head)?) } +/// Publish `bytes` at `path` unless the name is already taken, reporting whether this call +/// created it. The staged sibling is hardlinked rather than renamed, so the name being taken is an +/// answer instead of a failure — that boolean is how a caller tells a replay from a first send. fn atomic_create_file(path: &Path, bytes: &[u8]) -> anyhow::Result { - let parent = path.parent().context("atomic file has no parent")?; - fs::create_dir_all(parent)?; - let temporary = parent.join(tmp_name()); - fs::write(&temporary, bytes)?; - let result = match fs::hard_link(&temporary, path) { - Ok(()) => Ok(true), - Err(_) if path.is_file() => Ok(false), - Err(error) => Err(error.into()), - }; - let _ = fs::remove_file(temporary); - result + Ok(crate::fsatomic::create_once( + path, + bytes, + crate::fsatomic::Staging::new(TMP_PREFIX), + )?) } fn atomic_replace_file(path: &Path, bytes: &[u8]) -> anyhow::Result<()> { - let parent = path.parent().context("atomic file has no parent")?; - fs::create_dir_all(parent)?; - let temporary = parent.join(tmp_name()); - fs::write(&temporary, bytes)?; - if let Err(error) = fs::rename(&temporary, path) { - let _ = fs::remove_file(&temporary); - return Err(error.into()); - } + crate::fsatomic::replace( + path, + bytes, + crate::fsatomic::Staging::new(TMP_PREFIX), + crate::fsatomic::Durability::Rename, + )?; Ok(()) } @@ -2569,10 +2561,15 @@ fn remove_inbox_duplicate(source: &Path, filename: &str) -> anyhow::Result<()> { mod tests { use super::*; - /// [`atomic_create_file`]'s create-once contract, pinned before the helper is folded into one - /// shared primitive. It is a hardlink, not a rename, and that is the whole point: the first - /// publication wins, a second reports `false` instead of replacing the winner's bytes, and - /// neither leaves a staged sibling behind for the four `.message.tmp-` walkers to trip over. + /// [`atomic_create_file`]'s create-once contract. It is a hardlink, not a rename, and that is + /// the whole point: the first publication wins, a second reports `false` instead of replacing + /// the winner's bytes, and neither leaves a staged sibling behind for the four `.message.tmp-` + /// walkers to trip over. + /// + /// The mode is the deliberate change of the fold onto `fsatomic` — these records used to be + /// published at whatever an ordinary write produces (`0644` under the fleet's umask). Bus + /// records are per-agent state in the agent's own directory; a reader that is not st2 or that + /// agent was never a supported reader. #[test] fn a_create_once_message_write_keeps_the_first_bytes_and_reports_the_duplicate() { use std::os::unix::fs::PermissionsExt as _; @@ -2582,14 +2579,10 @@ mod tests { assert!(atomic_create_file(&path, b"first").unwrap()); assert!(!atomic_create_file(&path, b"second").unwrap()); assert_eq!(fs::read(&path).unwrap(), b"first"); - - let reference = path.with_file_name("ordinary-write"); - fs::write(&reference, b"x").unwrap(); - let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; assert_eq!( - mode(&path), - mode(&reference), - "the record is published at the mode an ordinary write produces" + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the record is published owner-only" ); let residue = fs::read_dir(path.parent().unwrap()) @@ -2771,11 +2764,21 @@ mod tests { fs::create_dir_all(&inbox).unwrap(); let victim = tmp.path().join("victim"); fs::write(&victim, "must remain unchanged").unwrap(); - let start = TMP_COUNTER.load(Ordering::Relaxed); - for counter in start..start + 4096 { + // The counter now lives in `fsatomic`, so the next names are predicted from a probe + // rather than read off a module-local static: one call consumes `start`, so the writes + // this test blocks are the 4096 after it. + let probe = tmp_name(); + let start = probe + .rsplit_once('-') + .and_then(|(_, counter)| counter.parse::().ok()) + .expect("the staging grammar ends in the counter"); + for counter in start + 1..start + 1 + 4096 { symlink( &victim, - inbox.join(format!(".message.tmp-{}-{counter}", std::process::id())), + inbox.join(format!( + "{TMP_PREFIX}.tmp-{}-{counter}", + std::process::id() + )), ) .unwrap(); } diff --git a/src/park.rs b/src/park.rs index cd71f4ac..7745f953 100644 --- a/src/park.rs +++ b/src/park.rs @@ -28,7 +28,6 @@ use std::collections::{BTreeMap, BTreeSet}; use std::fs; -use std::io::Write; use std::os::unix::ffi::OsStrExt; use std::path::{Path, PathBuf}; @@ -378,31 +377,27 @@ impl ParkProjection { .context("timestamping park")?, reason: reason.to_string(), }; - write_json_atomically(&path, &record, ".park.") + write_json_atomically(&path, &record, ".park") } } +/// The strictest publication level in the state plane, and deliberately so: a park marker is what +/// tells the next supervisor generation that a runtime is deliberately down, so a marker lost to a +/// crash reads as "nobody parked this" and the runtime comes back up. fn write_json_atomically( path: &Path, value: &T, temp_prefix: &str, ) -> anyhow::Result<()> { - let parent = path - .parent() - .ok_or_else(|| anyhow::anyhow!("{} has no parent", path.display()))?; - fs::create_dir_all(parent)?; let mut bytes = serde_json::to_vec(value)?; bytes.push(b'\n'); - let mut temp = tempfile::Builder::new() - .prefix(temp_prefix) - .tempfile_in(parent)?; - temp.write_all(&bytes)?; - temp.as_file().sync_all()?; - temp.persist(path) - .map_err(|error| error.error) - .with_context(|| format!("publishing {}", path.display()))?; - fs::File::open(parent)?.sync_all()?; - Ok(()) + crate::fsatomic::replace( + path, + &bytes, + crate::fsatomic::Staging::new(temp_prefix), + crate::fsatomic::Durability::FsyncFileAndDir, + ) + .with_context(|| format!("publishing {}", path.display())) } /// Reject anything that is not a plain filename, so an operator's typo (or a hostile argument) cannot @@ -487,7 +482,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let dir = tmp.path().join("park"); let path = marker_path(&dir, "runtime"); - write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park.").unwrap(); + write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park").unwrap(); assert_eq!(fs::read(&path).unwrap(), b"{\"schema\":\"test\"}\n"); assert_eq!( fs::metadata(&path).unwrap().permissions().mode() & 0o777, @@ -509,7 +504,7 @@ mod tests { // directory to sync it does not. fs::set_permissions(&dir, fs::Permissions::from_mode(0o300)).unwrap(); let refused = - write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park."); + write_json_atomically(&path, &serde_json::json!({"schema": "test"}), ".park"); fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)).unwrap(); assert!( refused.is_err(), @@ -633,7 +628,7 @@ mod tests { let path = marker_path(dir.path(), "a"); let mut record: ParkRecord = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap(); record.supervisor_start_time_ticks = record.supervisor_start_time_ticks.wrapping_add(1); - write_json_atomically(&path, &record, ".park.").unwrap(); + write_json_atomically(&path, &record, ".park").unwrap(); let batch = DirParkObserver::new(dir.path().to_path_buf()).observe(&desired(&["a"])); assert_eq!( @@ -692,7 +687,7 @@ mod tests { parked_at: "2026-08-09T10:00:00.000Z".to_string(), reason: "crash-looped".to_string(), }; - write_json_atomically(&marker_path(dir.path(), runtime_id), &record, ".park.").unwrap(); + write_json_atomically(&marker_path(dir.path(), runtime_id), &record, ".park").unwrap(); } let observer = DirParkObserver::new(dir.path().to_path_buf()); diff --git a/src/request.rs b/src/request.rs index 0319ce5a..33f694ef 100644 --- a/src/request.rs +++ b/src/request.rs @@ -3,7 +3,6 @@ use std::collections::{BTreeMap, HashSet}; use std::fs; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; use anyhow::Context; use kdl::KdlDocument; @@ -14,7 +13,6 @@ use sha2::{Digest, Sha256}; use crate::message; const REQUEST_VERSION: u32 = 1; -static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); #[derive(Debug, Clone, PartialEq, Eq)] pub struct ServicePrincipal { @@ -452,22 +450,15 @@ fn publish_once( }) } +/// Reserve the idempotency record, reporting whether this call is the one that created it. +/// Hardlinked rather than renamed for that boolean: the caller replays an interrupted send when +/// the name was already taken, and replaces nothing. fn atomic_create(path: &Path, bytes: &[u8]) -> anyhow::Result { - let parent = path.parent().context("state record has no parent")?; - fs::create_dir_all(parent)?; - let temporary = parent.join(format!( - ".request-state.tmp-{}-{}", - std::process::id(), - TMP_COUNTER.fetch_add(1, Ordering::Relaxed) - )); - fs::write(&temporary, bytes)?; - let result = match fs::hard_link(&temporary, path) { - Ok(()) => Ok(true), - Err(_) if path.is_file() => Ok(false), - Err(error) => Err(error.into()), - }; - let _ = fs::remove_file(temporary); - result + Ok(crate::fsatomic::create_once( + path, + bytes, + crate::fsatomic::Staging::new(".request-state"), + )?) } fn record_path(directory: &Path, key: &str) -> PathBuf { @@ -509,10 +500,13 @@ fn read_inbox_or_archive(agent_dir: &Path, filename: &str) -> anyhow::Result anyhow::Result ) } -/// Atomic write: a temp sibling + rename, so a concurrent reader sees either the old bytes or the new -/// bytes, never a partial file. +/// Atomic write: a staged sibling + rename, so a concurrent reader sees either the old bytes or +/// the new bytes, never a partial file. +/// +/// Deliberately the lenient durability level: this record is rewritten per agent per refresh +/// tick, and a lost write reads as `unknown` rather than as a wrong state, so buying crash +/// durability with two fsyncs per tick per agent would be paying for nothing. fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> { - let dir = path.parent().unwrap_or(Path::new(".")); - fs::create_dir_all(dir)?; - let tmp = dir.join(tmp_name()); - fs::write(&tmp, content)?; - // rename over the target — atomic on the same filesystem. - if let Err(e) = fs::rename(&tmp, path) { - let _ = fs::remove_file(&tmp); // best-effort cleanup - return Err(e.into()); - } + crate::fsatomic::replace( + path, + content.as_bytes(), + crate::fsatomic::Staging::new(".status"), + crate::fsatomic::Durability::Rename, + )?; Ok(()) } -static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); - -/// A per-write-unique temp filename (pid + a process-local counter — no collisions within a process, -/// and the pid separates processes). -fn tmp_name() -> String { - format!( - ".status.tmp-{}-{}", - std::process::id(), - TMP_COUNTER.fetch_add(1, Ordering::Relaxed) - ) -} - #[cfg(test)] mod tests { use super::*; use std::time::{Duration as Dur, SystemTime}; - /// [`write_atomic`]'s publication contract, pinned before the helper is folded into one - /// shared primitive. The staging name is part of the contract, not decoration: six catalog - /// and publication walkers match `.status.tmp-` by prefix, so the grammar - /// `{prefix}.tmp-{pid}-{counter}` is asserted here as well as by those walkers' own tests. + /// [`write_atomic`]'s publication contract: the target ends up carrying the complete new + /// bytes, no staged sibling survives, and the record is owner-only. + /// + /// The mode is the deliberate change of the fold onto `fsatomic` — this record used to be + /// published at whatever an ordinary write produces (`0644` under the fleet's umask). The + /// staging-name grammar `{prefix}.tmp-{pid}-{counter}` is unchanged and stays load-bearing: + /// six catalog and publication walkers match `.status.tmp-` by prefix, and the grammar itself + /// is asserted by `fsatomic`'s own test. #[test] fn a_status_write_replaces_the_target_and_leaves_no_staged_sibling() { use std::os::unix::fs::PermissionsExt as _; @@ -330,22 +322,10 @@ mod tests { write_atomic(&path, "available\n").unwrap(); write_atomic(&path, "working\n").unwrap(); assert_eq!(fs::read_to_string(&path).unwrap(), "working\n"); - - let staging = tmp_name(); - let (pid, counter) = staging - .strip_prefix(".status.tmp-") - .and_then(|rest| rest.split_once('-')) - .expect("the staging grammar is `.status.tmp--`"); - assert!(pid.bytes().all(|byte| byte.is_ascii_digit()) && !pid.is_empty()); - assert!(counter.bytes().all(|byte| byte.is_ascii_digit()) && !counter.is_empty()); - - let reference = tmp.path().join("ordinary-write"); - fs::write(&reference, b"x").unwrap(); - let mode = |path: &Path| fs::metadata(path).unwrap().permissions().mode() & 0o777; assert_eq!( - mode(&path), - mode(&reference), - "the status record is published at the mode an ordinary write produces" + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the status record is published owner-only" ); let residue = fs::read_dir(tmp.path()) From beefb67a5866c109478a2bf4ba23e5e7ca0fb555 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:18:56 +0200 Subject: [PATCH 3/5] fix(harness-state): stage the sequence floor under a per-writer name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The floor sidecar was staged at the FIXED literal `.harness-state.seq.tmp` and written with `fs::write`, which truncates and follows symlinks. Two consequences, one defect: - Two writers persisting a floor for the same agent shared that one staging path, so either could truncate and then rename the other's half-written bytes into place. A torn floor defeats exactly the failure the floor exists for: it is the safety net for the record itself going unreadable, and a claim reading a torn floor cannot tell it is reading garbage. One caller holds the record's lock; the other is the token-only virgin-record path, whose lock coverage is not established here, so the staging name must not depend on it. - The staging path sits in an agent-writable directory and `fs::write` follows symlinks, so a planted `.harness-state.seq.tmp` aimed st2's write at a file of the agent's choosing. Both die with one change: publish through `fsatomic`, whose staging name carries the pid and a counter and whose staging file is created exclusively. Same durability level as before (stage-and-rename, no fsync) and the same "log, never fail the claim" contract — losing a floor only matters if the record later goes unreadable. Pinned by a test that plants a symlink at the old literal staging path and asserts the floor still lands, at 0600, with the victim's bytes intact. Found while counting publication helpers for the `fsatomic` fold; it is a defect in its own right, so it lands in its own commit. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af --- src/harness_state.rs | 56 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/src/harness_state.rs b/src/harness_state.rs index f7e62dfc..19fff571 100644 --- a/src/harness_state.rs +++ b/src/harness_state.rs @@ -834,12 +834,22 @@ fn claim_locked(writer: &Writer, token: &str) -> anyhow::Result { /// keeps a torn write from corrupting the current floor, and a failed write is logged — the /// ownership still stands (losing the floor only matters if the record later becomes /// unreadable), but never silently. +/// +/// The staging name carries the writer's pid and a counter for the same reason every other +/// publication's does. It used to be the fixed literal `.harness-state.seq.tmp`, written with a +/// truncating, symlink-following `fs::write`: two writers persisting a floor for one agent shared +/// that one path, so each could truncate and rename the other's half-written bytes — a torn floor, +/// which defeats exactly the failure the floor exists for. One caller holds the record's lock; the +/// other is the token-only virgin-record path, whose lock coverage is not established here, so the +/// staging name must not depend on it. fn persist_floor(record_path: &Path, seq: u64) { let floor_path = record_path.with_file_name(SEQ_FLOOR_NAME); - let staged = floor_path.with_file_name(".harness-state.seq.tmp"); - if let Err(error) = - fs::write(&staged, format!("{seq}\n")).and_then(|()| fs::rename(&staged, &floor_path)) - { + if let Err(error) = crate::fsatomic::replace( + &floor_path, + format!("{seq}\n").as_bytes(), + crate::fsatomic::Staging::new(SEQ_FLOOR_NAME), + crate::fsatomic::Durability::Rename, + ) { tracing::warn!( "st2 harness-state: writing the sequence floor {} failed: {error}", floor_path.display() @@ -2040,6 +2050,44 @@ mod tests { assert_eq!(record.reason.as_deref(), Some("superseded")); } + /// The floor is staged under a per-writer name, created exclusively, so a path an agent + /// planted is refused instead of followed and two writers cannot tear each other's bytes. + /// + /// Both are the same defect: the staging name used to be the fixed literal + /// `.harness-state.seq.tmp` written with a truncating, symlink-following `fs::write`. A + /// symlink there aimed st2's write at a file of the agent's choosing, and two writers for one + /// agent could rename each other's half-written floor into place — a torn floor, which is + /// exactly the failure the floor exists to survive. + #[test] + fn the_sequence_floor_refuses_a_planted_staging_path() { + use std::os::unix::fs::{PermissionsExt as _, symlink}; + + let tmp = tempfile::tempdir().unwrap(); + let agent_dir = tmp.path().join("agents/hetz/worker"); + fs::create_dir_all(&agent_dir).unwrap(); + let victim = tmp.path().join("authored"); + fs::write(&victim, b"authored bytes").unwrap(); + symlink(&victim, agent_dir.join(".harness-state.seq.tmp")).unwrap(); + + persist_floor(&harness_state_path(&agent_dir), 7); + + let floor = agent_dir.join(SEQ_FLOOR_NAME); + assert_eq!( + fs::read_to_string(&floor).unwrap().trim(), + "7", + "the floor landed despite the planted staging path" + ); + assert_eq!( + fs::metadata(&floor).unwrap().permissions().mode() & 0o777, + 0o600 + ); + assert_eq!( + fs::read(&victim).unwrap(), + b"authored bytes", + "the planted symlink was followed and its target was truncated" + ); + } + /// The virgin token-only path also establishes initial ownership (sequence one), so it /// persists the floor sidecar too: if that first record later goes unreadable, a /// replacement claim continues PAST the lingering writer instead of colliding with it. From 375bac9ee09550a5b28ccc0c18e4e28823f7553b Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:23:11 +0200 Subject: [PATCH 4/5] refactor(pretrust): fold the second, uncited copy of the config write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pretrust_codex_at` had its own inline copy of the six lines `write_atomic` already owned — same staging name, same `create_dir_all`, same two context strings — differing only in that one serializes a `Value` and the other already holds a `String`. Splitting `write_atomic_str` out gives both one implementation with no behaviour change: identical staging path, identical error context, identical mode. `pretrust` stays out of the shared `fsatomic` primitive, and the extracted helper now says why: these are files st2 does not own (`~/.claude.json`, `~/.codex/config.toml`), so the staged file must inherit the umask a harness's own config carries rather than st2's 0600, and the staging path must sit beside the config under a name its owner will recognize. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af --- src/pretrust.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/pretrust.rs b/src/pretrust.rs index 129fa477..92a90717 100644 --- a/src/pretrust.rs +++ b/src/pretrust.rs @@ -92,18 +92,9 @@ pub fn pretrust_codex_at(config: &Path, dirs: &[PathBuf]) -> Result { n += 1; } if !appended.is_empty() { - if let Some(parent) = config.parent() { - std::fs::create_dir_all(parent).ok(); - } let mut out = existing; out.push_str(&appended); - // Atomic replace so a crashed write never corrupts the codex config. - let mut tmp = config.as_os_str().to_owned(); - tmp.push(format!(".st2trust.{}", std::process::id())); - let tmp = PathBuf::from(tmp); - std::fs::write(&tmp, out).with_context(|| format!("writing {}", tmp.display()))?; - std::fs::rename(&tmp, config) - .with_context(|| format!("renaming {} into {}", tmp.display(), config.display()))?; + write_atomic_str(config, &out)?; } Ok(n) } @@ -197,17 +188,28 @@ fn canonical_key(dir: &Path) -> String { .into_owned() } -/// Write `value` to `config` atomically (temp in the same dir + rename), so a crashed write never -/// corrupts the real config. The temp name carries the pid so concurrent pretrusts don't collide. +/// Write `value` to `config` atomically, so a crashed write never corrupts the real config. fn write_atomic(config: &Path, value: &Value) -> Result<()> { + let rendered = serde_json::to_string_pretty(value).context("serializing claude config")?; + write_atomic_str(config, &rendered) +} + +/// Stage-and-rename `contents` over `config`. The staging name carries the pid so concurrent +/// pretrusts do not collide. +/// +/// Deliberately NOT the shared `fsatomic` primitive, and the reason is the same one that keeps +/// this module out of it: these are files st2 does not own — `~/.claude.json` and +/// `~/.codex/config.toml` — so the staged file must inherit the umask a harness's own config +/// carries rather than st2's `0600`, and the staging path must sit beside the config under a name +/// its owner will recognize. +fn write_atomic_str(config: &Path, contents: &str) -> Result<()> { let mut tmp = config.as_os_str().to_owned(); tmp.push(format!(".st2trust.{}", std::process::id())); let tmp = PathBuf::from(tmp); if let Some(parent) = config.parent() { std::fs::create_dir_all(parent).ok(); } - let s = serde_json::to_string_pretty(value).context("serializing claude config")?; - std::fs::write(&tmp, s).with_context(|| format!("writing {}", tmp.display()))?; + std::fs::write(&tmp, contents).with_context(|| format!("writing {}", tmp.display()))?; std::fs::rename(&tmp, config) .with_context(|| format!("renaming {} into {}", tmp.display(), config.display()))?; Ok(()) From 849ccd7b46dc1808b40c88d7f9cba752e18254bb Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:49:45 +0200 Subject: [PATCH 5/5] fix(fs): narrow create_once's duplicate arm and single-source the staging prefixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four corrections from independent review of the fold, each a real gap rather than a wording fix: 1. `create_once` evaluated `written.and_then(|()| hard_link(…))`, so its `Err(_) if path.is_file() => Ok(false)` arm also caught failures from the staged WRITE. An `ENOSPC`/`EIO` on the staged bytes was then reported as "this key was already published" whenever the target happened to exist — bytes that were never written are nobody's publication. Only the hardlink may answer that question now. The absorbed callers propagated a failed write unconditionally, so this restores their contract. No test can force the edge in-process (there is no write-fault injection in this tree), so it is review-pinned. 2. `replace` removed the staged file on a REFUSED creation, which meant st2 unlinked whatever an agent had planted at the staging path instead of merely refusing to follow it — and asymmetric with `create_once`, which returns before its cleanup. The creation now returns before the cleanup scope in both. 3. Folding the staging name into a module made each caller's prefix a bare string argument, and the assertion that pinned `.status.tmp-` went with `tmp_name`. The walkers that skip staged files by prefix now read the writer's own const — `status::TMP_STAGING_PREFIX` for the six in `catalog`, `catalog_transaction` and `agent_publish`, `message::TMP_STAGING_PREFIX` for the four in `message`, and `harness_context::is_legacy_staging_name` derives its prefix from the `TMP_PREFIX` the writer passes — so a walker can no longer drift from the writer. Each module also asserts its const's VALUE, which is the part a shared const cannot protect: renaming both sides together would leave every already-staged file on the fleet unrecognized. 4. `reserved_message_temporary_symlinks_are_never_followed` predicted staging names from one probe of a counter that is now process-global, so a sibling test advancing it past the planted window would have turned the test intermittent. It now plants, re-probes, and only proceeds once the very next name is one it has blocked. Also corrects the HC-T08 sentence added by the fold: bus message files are state-plane records too and stay at the writing process's umask, so "like every other state-plane record" was wrong. agent-identity: dev3.direct.omp.43sz6ujq agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.1.7 agent-runtime: OMP 18.1.7 tooling-profile: dotfiles@39a19af --- docs/vrs/08-harness-context/requirements.md | 12 ++-- src/agent_publish.rs | 2 +- src/catalog.rs | 4 +- src/catalog_transaction.rs | 6 +- src/fsatomic.rs | 25 ++++--- src/harness_context.rs | 8 ++- src/message.rs | 72 ++++++++++++++------- src/status.rs | 22 ++++++- 8 files changed, 103 insertions(+), 48 deletions(-) diff --git a/docs/vrs/08-harness-context/requirements.md b/docs/vrs/08-harness-context/requirements.md index c5860c65..fe0ef169 100644 --- a/docs/vrs/08-harness-context/requirements.md +++ b/docs/vrs/08-harness-context/requirements.md @@ -103,11 +103,13 @@ record. other purposes, buys nothing a named include entry does not — bounded by st2-side test that pins the names it expects (HC-R05), and by the fact that no correctness property here depends on the transport at all: everything works - with no replication, and remote visibility is what is lost. The pair is - published mode `0600`, like every other state-plane record, so a transport - that reads the catalog as a different unprivileged uid needs that decision - taken deliberately rather than inherited from a default; no such transport - runs today, which is what leaves the wire-cost half of `DQ-C1` unmeasured. + with no replication, and remote visibility is what is lost. Both records are + published mode `0600` — as is every record st2's shared publication primitive + writes, though not the bus message files, which stay at the writing process's + umask — so a transport reading the catalog as a different unprivileged uid + needs that decision taken deliberately rather than inherited from a default. + No such transport runs today, which is also what leaves the wire-cost half of + `DQ-C1` unmeasured. ## Requirements diff --git a/src/agent_publish.rs b/src/agent_publish.rs index 15f73456..5365814d 100644 --- a/src/agent_publish.rs +++ b/src/agent_publish.rs @@ -676,7 +676,7 @@ fn copy_filtered_catalog( name_text.as_ref(), "resources" | "archive" | "inbox" | "status" ) - || declaration_parent && name_text.starts_with(".status.tmp-")) + || declaration_parent && name_text.starts_with(crate::status::TMP_STAGING_PREFIX)) { continue; } diff --git a/src/catalog.rs b/src/catalog.rs index ba0de8c2..844cc549 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -641,7 +641,7 @@ pub(crate) fn validate_catalog_relative_profile_module_path(relative: &Path) -> matches!( *name, ".workspace" | "resources" | "archive" | "inbox" | "status" - ) || name.starts_with(".status.tmp-") + ) || name.starts_with(crate::status::TMP_STAGING_PREFIX) }); let reserved_template_subtree = first == "_templates" && components.iter().skip(1).any(|name| { @@ -657,7 +657,7 @@ pub(crate) fn validate_catalog_relative_profile_module_path(relative: &Path) -> | "archive" | "inbox" | "status" - ) || name.starts_with(".status.tmp-") + ) || name.starts_with(crate::status::TMP_STAGING_PREFIX) }); anyhow::ensure!( !(reserved_control || reserved_root || reserved_agent_state || reserved_template_subtree), diff --git a/src/catalog_transaction.rs b/src/catalog_transaction.rs index 1f85d3b9..f0d4d7cb 100644 --- a/src/catalog_transaction.rs +++ b/src/catalog_transaction.rs @@ -2033,7 +2033,7 @@ fn collect_bundle_files( | ".harness-state.seq" | ".harness-state.lock" | ".harness-context.lock" - ) || name_text.starts_with(".status.tmp-"); + ) || name_text.starts_with(crate::status::TMP_STAGING_PREFIX); if first.is_some() && relative_to_bundle.components().count() == 1 && state { if source == ProjectionSource::Prepared { anyhow::bail!( @@ -2244,7 +2244,7 @@ fn reject_state_children(identity_path: &Path) -> Result<()> { let name = entry.file_name(); let name = name.to_str().context("identity path is not UTF-8")?; if matches!(name, "resources" | "archive" | "inbox" | "status") - || name.starts_with(".status.tmp-") + || name.starts_with(crate::status::TMP_STAGING_PREFIX) { anyhow::bail!( "prepared catalog contains state-plane path: {}", @@ -2934,7 +2934,7 @@ fn validate_declaration_leaf_path(path: &str) -> Result<()> { !matches!( components[3], ".workspace" | "resources" | "archive" | "inbox" | "status" - ) && !components[3].starts_with(".status.tmp-"), + ) && !components[3].starts_with(crate::status::TMP_STAGING_PREFIX), "catalog apply marker contains a workspace or state-plane path" ); } diff --git a/src/fsatomic.rs b/src/fsatomic.rs index 2c3cd0e4..7b330720 100644 --- a/src/fsatomic.rs +++ b/src/fsatomic.rs @@ -95,18 +95,20 @@ pub(crate) fn replace( ) -> io::Result<()> { let parent = parent_of(path)?; let staged = prepare(parent, &staging)?; - let landed = (|| -> io::Result<()> { - let mut file = create_staging(&staged)?; + // A refused creation returns BEFORE the cleanup scope: the thing occupying a + // `{prefix}.tmp-{pid}-{counter}` path is not ours, and refusing to follow it must not turn + // into unlinking it. + let mut file = create_staging(&staged)?; + let staged_bytes = (|| -> io::Result<()> { file.write_all(bytes)?; if durability == Durability::FsyncFileAndDir { file.sync_all()?; } - drop(file); - fs::rename(&staged, path) + Ok(()) })(); - if let Err(error) = landed { - // Best-effort: the staging name is unique per write, so a leftover is inert rather than a - // path a later write could collide with. + drop(file); + if let Err(error) = staged_bytes.and_then(|()| fs::rename(&staged, path)) { + // Best-effort, and only for the file this call created. let _ = fs::remove_file(&staged); return Err(error); } @@ -127,15 +129,18 @@ pub(crate) fn create_once(path: &Path, bytes: &[u8], staging: Staging<'_>) -> io let parent = parent_of(path)?; let staged = prepare(parent, &staging)?; let mut file = create_staging(&staged)?; - let written = file.write_all(bytes); + let staged_bytes = file.write_all(bytes); drop(file); - let created = match written.and_then(|()| fs::hard_link(&staged, path)) { + // A failed staged write is never a duplicate: `Ok(false)` claims somebody else published + // these bytes, and bytes that were never written are nobody's publication. Only the hardlink + // may answer that question. + let created = staged_bytes.and_then(|()| match fs::hard_link(&staged, path) { Ok(()) => Ok(true), // `hard_link` reports `AlreadyExists` for a taken name, but a target that is already a // regular file is the same answer whatever the error says. Err(_) if path.is_file() => Ok(false), Err(error) => Err(error), - }; + }); let _ = fs::remove_file(&staged); created } diff --git a/src/harness_context.rs b/src/harness_context.rs index e0f9514c..bfb9a7d2 100644 --- a/src/harness_context.rs +++ b/src/harness_context.rs @@ -392,8 +392,14 @@ pub(crate) fn is_legacy_harness_context_staging_file( Ok(true) } +/// The matcher and the writer must agree on the prefix, so both read [`TMP_PREFIX`]: this name is +/// the one INVARIANTS row 29 says current-catalog identity walkers overlook, and a writer that +/// drifted from this matcher would leave a staged file the walkers no longer recognize. fn is_legacy_staging_name(name: &str) -> bool { - let Some(suffix) = name.strip_prefix(".harness-context.tmp-") else { + let Some(suffix) = name + .strip_prefix(TMP_PREFIX) + .and_then(|rest| rest.strip_prefix(".tmp-")) + else { return false; }; let Some((pid, counter)) = suffix.split_once('-') else { diff --git a/src/message.rs b/src/message.rs index 79119303..9a58ab33 100644 --- a/src/message.rs +++ b/src/message.rs @@ -399,10 +399,14 @@ pub fn materialize_message_once( result } -/// The staging-name prefix for everything this module stages. Four sent-record and inbox walkers -/// match `.message.tmp-` by prefix, so the name is a contract with them, not a local detail. +/// The staging-name prefix for everything this module stages. const TMP_PREFIX: &str = ".message"; +/// The full staged-name prefix four inbox and sent-record walkers skip by prefix, spelled ONCE so +/// a walker cannot drift from the writer. Tied to [`TMP_PREFIX`] by +/// `the_staging_prefix_is_the_one_the_inbox_walkers_skip`. +const TMP_STAGING_PREFIX: &str = ".message.tmp-"; + fn tmp_name() -> String { crate::fsatomic::staging_name(TMP_PREFIX) } @@ -711,7 +715,7 @@ fn read_sent_records(directory: &Path) -> anyhow::Result> { let Some(name) = name.to_str() else { anyhow::bail!("sent record filename is not UTF-8"); }; - if name.starts_with(".message.tmp-") { + if name.starts_with(TMP_STAGING_PREFIX) { continue; } anyhow::ensure!(name.ends_with(".json"), "unexpected sent record entry"); @@ -744,7 +748,7 @@ fn read_pending_records(directory: &Path) -> anyhow::Result> { let Some(name) = name.to_str() else { anyhow::bail!("pending sent record filename is not UTF-8"); }; - if name.starts_with(".message.tmp-") { + if name.starts_with(TMP_STAGING_PREFIX) { continue; } let digest = name @@ -804,7 +808,7 @@ fn read_sent_commits(directory: &Path) -> anyhow::Result anyhow::Result> let Some(name) = name.to_str() else { anyhow::bail!("sent key filename is not UTF-8"); }; - if name.starts_with(".message.tmp-") { + if name.starts_with(TMP_STAGING_PREFIX) { continue; } let digest = name @@ -2561,6 +2565,18 @@ fn remove_inbox_duplicate(source: &Path, filename: &str) -> anyhow::Result<()> { mod tests { use super::*; + /// The staged name is a contract with four walkers that skip it by prefix (`inbox_dir` and the + /// three sent-record scans), all of which now match [`TMP_STAGING_PREFIX`] so none can drift + /// from the writer. What a shared const cannot catch is both sides being renamed together, + /// which would leave every already-staged file on the fleet unrecognized — hence the value + /// assertion. + #[test] + fn the_staging_prefix_is_the_one_the_inbox_walkers_skip() { + assert_eq!(TMP_PREFIX, ".message"); + assert_eq!(TMP_STAGING_PREFIX, format!("{TMP_PREFIX}.tmp-")); + assert!(!is_message_filename(&tmp_name()), "a staged name must never look like a message"); + } + /// [`atomic_create_file`]'s create-once contract. It is a hardlink, not a rename, and that is /// the whole point: the first publication wins, a second reports `false` instead of replacing /// the winner's bytes, and neither leaves a staged sibling behind for the four `.message.tmp-` @@ -2588,7 +2604,7 @@ mod tests { let residue = fs::read_dir(path.parent().unwrap()) .unwrap() .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) - .filter(|name| name.starts_with(".message.tmp-")) + .filter(|name| name.starts_with(TMP_STAGING_PREFIX)) .collect::>(); assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); } @@ -2606,7 +2622,7 @@ mod tests { let residue = fs::read_dir(tmp.path()) .unwrap() .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) - .filter(|name| name.starts_with(".message.tmp-")) + .filter(|name| name.starts_with(TMP_STAGING_PREFIX)) .collect::>(); assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); } @@ -2764,23 +2780,29 @@ mod tests { fs::create_dir_all(&inbox).unwrap(); let victim = tmp.path().join("victim"); fs::write(&victim, "must remain unchanged").unwrap(); - // The counter now lives in `fsatomic`, so the next names are predicted from a probe - // rather than read off a module-local static: one call consumes `start`, so the writes - // this test blocks are the 4096 after it. - let probe = tmp_name(); - let start = probe - .rsplit_once('-') - .and_then(|(_, counter)| counter.parse::().ok()) - .expect("the staging grammar ends in the counter"); - for counter in start + 1..start + 1 + 4096 { - symlink( - &victim, - inbox.join(format!( - "{TMP_PREFIX}.tmp-{}-{counter}", - std::process::id() - )), - ) - .unwrap(); + // The counter lives in `fsatomic` and is shared with every other staging site, so a + // sibling test running in parallel advances it too. Plant a window, then PROBE again: the + // loop only exits once the very next name is one this test has already blocked, so the + // assertion below cannot become "the call happened to pick a free name". + let counter_of = |name: &str| { + name.rsplit_once('-') + .and_then(|(_, counter)| counter.parse::().ok()) + .expect("the staging grammar ends in the counter") + }; + let mut planted_through = 0; + loop { + let probe = counter_of(&tmp_name()); + if probe < planted_through { + break; + } + for counter in probe + 1..=probe + 512 { + symlink( + &victim, + inbox.join(format!("{TMP_PREFIX}.tmp-{}-{counter}", std::process::id())), + ) + .unwrap(); + } + planted_through = probe + 512; } let error = materialize_message_once(&inbox, "1784649988123-symlnk.md", "must not escape") diff --git a/src/status.rs b/src/status.rs index 476698e2..41c00356 100644 --- a/src/status.rs +++ b/src/status.rs @@ -284,6 +284,14 @@ fn write_record(path: &Path, state: State, written_at_ms: u64) -> anyhow::Result ) } +/// The staging-name prefix this module hands [`crate::fsatomic`]. +const TMP_PREFIX: &str = ".status"; + +/// The full staged-name prefix six catalog and publication walkers match, spelled ONCE here so a +/// walker can never drift from the writer. Tied to [`TMP_PREFIX`] by +/// `the_staging_prefix_is_the_one_the_catalog_walkers_match`. +pub(crate) const TMP_STAGING_PREFIX: &str = ".status.tmp-"; + /// Atomic write: a staged sibling + rename, so a concurrent reader sees either the old bytes or /// the new bytes, never a partial file. /// @@ -294,7 +302,7 @@ fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> { crate::fsatomic::replace( path, content.as_bytes(), - crate::fsatomic::Staging::new(".status"), + crate::fsatomic::Staging::new(TMP_PREFIX), crate::fsatomic::Durability::Rename, )?; Ok(()) @@ -305,6 +313,18 @@ mod tests { use super::*; use std::time::{Duration as Dur, SystemTime}; + /// The staged name is a contract with six walkers that skip it by prefix, in three other + /// modules: `catalog.rs`, `catalog_transaction.rs` and `agent_publish.rs` all match + /// [`TMP_STAGING_PREFIX`], so a changed writer prefix cannot desynchronize them. What a const + /// cannot catch is the prefix being renamed on BOTH sides at once — a staged status file would + /// then still be skipped locally, but the fleet's existing records and any other reader of the + /// old name would not. That is what this assertion is for. + #[test] + fn the_staging_prefix_is_the_one_the_catalog_walkers_match() { + assert_eq!(TMP_PREFIX, ".status"); + assert_eq!(TMP_STAGING_PREFIX, format!("{TMP_PREFIX}.tmp-")); + } + /// [`write_atomic`]'s publication contract: the target ends up carrying the complete new /// bytes, no staged sibling survives, and the record is owner-only. ///