refactor(fs): one fsatomic module, and publish the state plane at 0600 - #508
Draft
schickling-assistant wants to merge 5 commits into
Draft
refactor(fs): one fsatomic module, and publish the state plane at 0600#508schickling-assistant wants to merge 5 commits into
schickling-assistant wants to merge 5 commits into
Conversation
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
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-<pid>-<counter>` 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
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
`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
…ging prefixes 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nine helpers in eight modules staged a sibling and renamed it over a state-plane record.
src/fsatomic.rsis the one primitive they now share. The line count was never the problem — the parts that decide whether a publication is safe (exclusive creation, staged-file mode, cleanup, how far durability is pushed) were decided nine times and came out four different ways, most visibly as the defect #502 fixed.This PR carries two deliberate behaviour changes and one defect fix. They are the reason it is not called a dedupe.
Behaviour change 1 — six callers publish at
0600instead of0644 & ~umaskcontext,status,message(both helpers),request,harness_state. Three absorbed callers were already0600(delivery_ledger,driver_diagnostic,park), and0600is what the two most recently reviewed publishers in this tree chose (#502, anddelivery_ledger's own test asserts it).These are per-agent records in per-agent directories; the readers are st2 and the agent itself, the same uid on this fleet.
The one pair carrying a cross-repository contract is
harness-state/harness-context, whose names a replication transport's include list is expected to name (HC-R05, INVARIANTS row 29). The previous scouting pass filed this as a human question. It resolves on evidence:DQ-C1records the wire-cost half as unmeasurable "because no replication transport runs on the fleet today — the catalog is a local filesystem and the transport's adoption record is still open" (DQ-H2leaves the same fleet-scale cost open without naming the transport's absence). A grep of the fleet configuration repository finds no include list, nosyncthing/unison/mutagen/rsync-of-catalog unit, and no reference to either record name outside a statusline comment.harness_context::the_replicated_driver_record_names_are_pinnedstill asserts["harness-state", "harness-context"]and that both readers derive their paths from those names. A mode is not a name.services.systemResourceCollector.agentTaskResourcesreads it withUser=unset (root), and root is not subject to0600. Live records before this change:644 schickling …/harness-state,644 schickling …/harness-context.Pinned per module by a
0600assertion that replaces the "same mode as an ordinaryfs::write" assertion the first commit pinned — the umask-independent way to characterize the old mode, so the flip is visible in the diff of the test rather than only in the diff of the code.Behaviour change 2 — the directory fsync is strict everywhere
delivery_ledgeranddriver_diagnosticswallowed it (if let Ok(dir) = File::open(parent) { let _ = dir.sync_all(); });parkandcodex_app_serveralready failed on it.Ledger::persistcan now return an error on an edge it used to report success for. The bytes still land — the rename happens before the sync — so the error is a report about durability, not about contents.driver_diagnostic::Publisher::persistalready logs a failed publication and carries on, so its visible consequence is one warning line where there used to be none.The alternative was keeping a durability level that nothing can be made to fail, which is a promise no test can keep. Both flips are pinned by the two tests the first commit wrote against the old behaviour:
a_directory_that_cannot_be_synced_does_not_fail_the_publication→…_fails_the_publication, in both modules. Those tests are meaningful only for a non-root uid; the hermetic gate runs as the sandbox's unprivileged build user (verified: uid 1000nixbld), and a local root run skips the edge instead of asserting what root cannot observe.Defect fix (its own commit) — the sequence floor staged at a fixed literal
harness_state::persist_floorstaged at.harness-state.seq.tmp— no pid, no counter — written withfs::write, which truncates and follows symlinks. Two writers persisting a floor for one agent shared that path and either could rename the other's half-written bytes into place: a torn floor defeats exactly the failure the floor exists for, since a claim reading a torn floor cannot tell. One caller holds the record's lock; the other is the token-only virgin-record path, whose lock coverage is not established, so the staging name must not depend on it. And the planted-symlink case aimed st2's write at a file of the agent's choosing. Pinned bythe_sequence_floor_refuses_a_planted_staging_path.Two latent bugs that die with the fold
contextstaged at.ctx.tmp-{pid}-{now_ms()}: 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 — an impossibleAlreadyExists.fs::writefollows symlinks;O_EXCLdoes not.What stays with the caller, deliberately
to_vec,to_vecplus a trailing newline,to_writerplus a trailing newline); ajsonentry point would have to reproduce all three to keep every record byte-identical, so the module would carry the difference instead of removing it.{prefix}.tmp-{pid}-{counter}is load-bearing:.status.tmp-is matched by prefix in six catalog and publication walkers,.message.tmp-in four sent-record walkers, and.harness-context.tmp-<pid>-<counter>is parsed digit by digit byharness_context::is_legacy_staging_nameunder INVARIANTS row 29. Asserted directly byfsatomic::tests::a_staging_name_is_prefix_pid_counter_and_never_repeats.io::Resultthroughout, neveranyhow, so every callsite keeps the context string it wraps the call with. Four.context("… has no parent")messages ARE gone, replaced by the module's oneInvalidInput— reachable only for a path with no parent at all, i.e./. Andpark'swith_context("publishing …"), which used to wrap only the persist step, now wraps the whole publication, so a failed directory fsync there reads differently than before.messagekeepstmp_name, now delegating tofsatomic::staging_name, because two of its publications stage their own file for reasons the module does not cover (send_to_inboxrenames into a name it must search for;materialize_message_oncecompares 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 exactly the failure exclusive creation then reports as an error.reserved_message_temporary_symlinks_are_never_followednow predicts the next names from a probe instead of reading a module-local static.Six publishers deliberately stay out
Each holds a strictly stronger primitive than a path-based stage-and-rename can express:
catalog_transaction::atomic_replace_file,agent_publish::{atomic_write_spec, atomic_publish_staged_bundle}EXDEVfault injection andcontrol_plane_rename_errorclassificationresource_profile::atomic_replace_at,event::write_recordopenat/renameatagainst a retained directory capabilitycodex_app_server::atomic_json0700on the state directory on every write (it holds the Codex socket and its owner lock) and emits pretty JSONpretrust::write_atomicstays 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. Its own second, inline, uncited copy of the same six lines is folded onto the helper instead (last commit, no behaviour change).Corrections to the earlier scouting pass
codex_app_server::atomic_jsondoes not publish through a control-dir fd; the real reasons aresecure_dirand pretty JSON. Its doc comment now says so.event::write_recordis a sixth keep-set member the census missed (it usesopenat/renameatwithO_NOFOLLOW).messagehas two further staging sites the census missed (write_message,materialize_message_once); both correctly stay out, and both now share the one counter.delivery_ledger's residue test filtered onname.ends_with(".tmp")— this helper's own spelling rather than the grammar — so it would have gone silently vacuous when the spelling changed. Fixed to filter by prefix in the first commit.Independent review, and what it changed
A reviewer with a clean context was asked to falsify the exclusivity claim above. It found four real
gaps, fixed in the last commit:
create_onceswallowed a failed staged write asOk(false)whenever the target alreadyexisted, because the write result and the hardlink result shared one
match.Ok(false)claimssomebody else published these bytes; bytes that were never written are nobody's publication. Only
the hardlink may answer that now. No in-process test can force the edge (this tree has no
write-fault injection), so it is review-pinned.
replaceunlinked a REFUSED staging path, so st2 deleted whatever an agent had planted thereinstead of merely refusing to follow it — and asymmetrically with
create_once. Both now returnbefore the cleanup scope.
prefix into a bare string argument, and the assertion pinning
.status.tmp-went withtmp_name.The walkers now read the writer's own const (
status::TMP_STAGING_PREFIXfor six walkers in threemodules,
message::TMP_STAGING_PREFIXfor four, andharness_context::is_legacy_staging_namederives its prefix from the writer's
TMP_PREFIX), and each module asserts its const's value —the part a shared const cannot protect.
probe of a counter that is now process-global. It plants, re-probes, and proceeds only once the
next name is one it has blocked.
Three further unstated-but-benign changes the review surfaced, stated here rather than fixed:
persist_floornow creates the record's parent directory (the shared primitive does, the barefs::writedid not). Unreachable today — the floor is written after the record.park's staging name went fromtempfilerandomness to the house grammar.park.tmp-{pid}-{counter}, and its prefix argument from".park."to".park". Both stillstart with
.park., so its own residue filter still matches, and no production walker reads thatname; the delta is that the name is predictable, which
O_EXCLturns into a refusal rather than afollow.
delivery_ledger's staging spelling genuinely changed,.delivery-ledger.{pid}.{n}.tmpto.delivery-ledger.tmp-{pid}-{n}; nothing in the tree matches.delivery-ledger*except theresidue test the first commit fixed.
The reviewer independently confirmed what could have gone wrong and did not: byte-identity of every
published record, per-caller durability (no fsync silently gained or lost),
is_legacy_staging_nameand INVARIANTS row 29 still matching, no other module minting a prefix
fsatomicalso mints, thekeep-set untouched,
pretrust's fold byte-for-byte, and the twoeuid == 0skips being live ratherthan vacuous in the hermetic gate.
Commits
test(fs)— every absorbed helper's contract pinned before the fold, including both ends of the directory-sync edge and the staging-directory argument. A fold whose only proof is "it still compiles" ships whichever answers the folder preferred.refactor(fs)— the module, the nine migrations, both behaviour changes, and the flipped pins.fix(harness-state)— the sequence-floor defect.refactor(pretrust)— the inline copy folded onto its helper.fix(fs)— the four review corrections above.Proof
nix build .#st2 --no-link -L(the hermetic suite) — see the merge comment for the verdict.cargo test --lib: 759 passed / 0 failed before commits 3–4; the touched modules re-run green after.axe vrs check --profile strict docs/vrs: ok.Not in scope, found while counting
The catalog declaration-plane walker (
catalog_transaction.rs:2026) tolerates state-plane children by an exact-name list plus.status.tmp-, so a staged.harness-state.tmp-…/.harness-context.tmp-…/.message.tmp-…file present during a snapshot enters the declaration bundle instead of being skipped. Pre-existing, unchanged by this PR (the floor's staging name was equally absent from that list before), and reported for its own issue rather than fixed inside a refactor.Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile