refactor(lock): typed file-lock guard for every hand-rolled flock site - #511
Merged
schickling-assistant merged 3 commits intoSep 7, 2026
Merged
Conversation
Eight sites in this tree hand-rolled the same three system calls — open a lock file, `flock` it, close it — each with its own `unsafe` block and, for four of them, its own `impl Drop`. `src/flock.rs` owns those three calls and nothing else: it returns `std::io::Result` so each caller keeps its own context string, `Ok(None)` means contention and nothing else, and it neither creates directories, nor fsyncs, nor names lock files, because the callers legitimately disagree about all three. `open` and `hold` stay split because the window between them is load-bearing. Defaults are `resource_profile::lock_publication`'s — the one site already correct on every axis — so they are house style rather than invention: `O_RDWR`, `O_NOFOLLOW`, `O_CLOEXEC`, create mode `0600`. `catalog_lock` is the first consumer, byte-identically: its two debug contention checkpoints still sit exactly between the open and the `flock`, its two context strings are unchanged, and its private `Mode`/`Wait` plus its hand-rolled `impl Drop` are now the transport's. 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
…uard The seven remaining hand-rolled sites now open and hold through `crate::flock`, leaving exactly one `unsafe` flock block in production code: `resource_profile::lock_publication`, which opens through `openat` on a retained directory capability, proves the opened lock is a regular file between the open and the `flock`, and returns a `CatchUpError` taxonomy the publication state machine matches on. `event::StreamLock`, `message::SentLock` and `pretrust::ConfigLock` lose their hand-rolled `impl Drop`; `SentLock`'s `file: Option<File>`, which was never `None`, collapses to one guard. Three deliberate behaviour changes ride along, each now pinned: - `O_NOFOLLOW` newly hardens five lock files that set neither flag before — `event`, the `harness_state` acquisition shared by `harness-state`, `harness-context` and `context`'s `now.md`, `message`, `pretrust` and `resource_observe`. Six new symlink-refusal tests, mirroring the existing `catalog_lock_refuses_a_symlinked_lock_file`. - Four of those five start being created `0600` instead of `0644`. Existing lock files keep their mode; `resource_observe` already re-tightened its own. - `message::shared_existing` opens `O_RDWR` where it opened `O_RDONLY`, so a reader with read-but-not-write access to a ledger lock would newly fail. Every file in the live catalog is owned by one uid and st2 runs as a systemd user unit with no `User=`, so no such reader exists. `harness_state`, `harness_context`, `context`, `message`, `pretrust` and `resource_observe` had no lock test at all. They get one each, using the oracle that `flock` locks the open file description: a second open of the same lock file in the same process observes a live holder without threads. The `message::inspect_sent` pin is the load-bearing one — its two sequential guards must both be released before it returns, or a doctor read would block every later publication by that sender. 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
… real scope The flock module has no external consumer: every caller is a sibling module in this crate, so it is declared `mod flock;` and matches the fsatomic transport's shape rather than adding an empty public surface. The inspect_sent guard test claimed to prove that both of the function's shared guards are released. It cannot: by the time it runs the lock file exists, so `shared_existing` answers `Some` and inspect_sent early-returns on its first guard. That first guard is the one the test's mutation check covers. Reaching the second needs `shared_existing` to answer `None` then `Some`, a race a single-threaded test cannot arrange, so the assertion text and doc comment now state exactly that instead of overstating coverage. 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.
Applies issue #504 §T1-LOCK. Two commits: extract the transport, then migrate the sites.
Eight places in this tree hand-rolled the same three system calls — open a lock file,
flockit,close it — each with its own
unsafeblock, and four of them with their ownimpl Drop. This PRputs those three calls in one module and routes seven of the eight through it.
Not a pure dedupe. The diff carries three deliberate behaviour changes, listed below with the
test that pins each. If you only read one section, read that one.
What landed
src/flock.rs—FileLockguard,Mode { Shared, Exclusive },Wait { Block, Now },Open { Create, CreateNew, Existing }. Design rules that made byte-identity achievable:std::io::Resultand neveranyhow, so every callsite keeps the exact context stringit reports today. Eight distinct diagnostics stay eight.
Ok(None)means contention and nothing else. Every other failure isErr, so a caller cannotmistake a lock file it may not open for a lock file somebody else holds.
fsync, and does not name lock files. The callerslegitimately disagree about all three (one creates the parent, one requires the parent to be
0700first, one names its lock by suffixing a foreign config path), so absorbing those wouldhide the disagreement rather than resolve it.
openandholdstay split, because the window between them is load-bearing:catalog_lockwrites its two debug contention checkpoints there (five tests readST2_TEST_CATALOG_LOCK_ATTEMPT), andresource_profileproves its opened lock is a regularfile there.
resource_profile::lock_publication's — the one site already correct on every axis,so they are house style rather than invention:
O_RDWR,O_NOFOLLOW,O_CLOEXEC(alreadyimplied by Rust's
OpenOptions; named so a reader does not go hunting for it), create mode0600.CatalogLockis the first consumer, byte-identically: same two context strings, same checkpointplacement, its private
Mode/Waitand its hand-rolledimpl Dropare now the transport's.Then the seven migrations:
catalog_transaction::initialize_bootstrap_control,codex_app_server::acquire_owner_lock,event::StreamLock,harness_state::lock_exclusive(rippling through
context::lock_nowand its 3 call sites),message::SentLock,pretrust::ConfigLock,resource_observe::lock_request_scope. Fourimpl Dropdeleted(
CatalogLock,StreamLock,SentLock,ConfigLock).SentLock'sfile: Option<File>, whichwas never
None, collapses to one guard.Exactly one
unsafeflock block is left in production code:resource_profile::lock_publication,deliberately not migrated. It opens via
openatagainst a retained directory capability, returnsa
CatchUpErrortaxonomy the publication state machine matches on, and proves the opened lock is aregular file between the open and the
flock. Also untouched:host_lock.rs(a pid file thatmust survive process death, not a flock site) and every test-side raw
flock(those areadversaries taking the lock from outside the process under test).
Behaviour changes, explicitly
1.
O_NOFOLLOWnewly hardens five lock filesevent, theharness_stateacquisition (shared by theharness-staterecord lock, theharness-contextrecord lock, andcontext'snow.mdlock — three paths, one acquisition),message,pretrustandresource_observeset neitherO_CLOEXECnorO_NOFOLLOWtoday. Asymlink planted at any of those lock paths currently redirects the lock domain — and the write that
follows — to its target. After this PR the open fails with
ELOOP.No current test and no live path reaches those lock files through a symlink, so nothing observes
the change today. Pins, mirroring the existing
catalog_lock_refuses_a_symlinked_lock_file:flock::tests::a_symlinked_lock_path_is_refused_instead_of_locking_its_targetsrc/flock.rsharness_state::tests::a_symlinked_record_lock_is_refused_instead_of_locking_its_targetsrc/harness_state.rscontext::tests::a_symlinked_now_lock_refuses_the_write_instead_of_locking_its_targetsrc/context.rsmessage::tests::a_symlinked_ledger_lock_is_refused_instead_of_locking_its_targetsrc/message.rspretrust::tests::a_symlinked_trust_lock_is_refused_instead_of_locking_its_targetsrc/pretrust.rsresource_observe::tests::a_symlinked_scope_lock_is_refused_instead_of_locking_its_targetsrc/resource_observe.rsEach asserts the errno is
ELOOP, that the symlink target's bytes are unchanged, and (where theentry point publishes) that nothing was published.
2. Four of those five lock files start being created
0600instead of0644event,harness_state(×3 paths),message,pretrustpassed nomodeand so created theirlock files
0644 & ~umask.resource_observealready used0600and additionally re-tightens apre-existing lock file with
set_permissions— that call is kept at the callsite, because thetransport only applies
modeat creation, so an older build's0644lock file would otherwisestay
0644.Existing lock files are not re-moded by this PR (except
resource_observe's, unchanged fromtoday). The live catalog does have
0644.lockfiles from older builds; they keep that mode andkeep working, since their owner has write access.
Pins:
pretrust::tests::the_trust_lock_is_created_private_and_excludes_a_second_holder,harness_state::tests::a_symlinked_record_lock_is_refused_instead_of_locking_its_target(asserts0600on a freshly created record lock),resource_observe::tests::the_scope_lock_excludes_a_second_holder_and_releases_on_drop,flock::tests::a_created_lock_file_is_private_to_its_owner.3.
message::shared_existingopensO_RDWRwhere it openedO_RDONLYThis is the one that needs a human's eye.
SentLock::shared_existingis doctor's read-only pathinto a sender's ledger (
message::inspect_sent, reached fromst2 doctor). It opens the ledgerlock
O_RDONLYtoday; the transport has one open shape, so it becomesO_RDWR.Stated plainly: a doctor running as a different uid against a
0644ledger lock would startfailing where it read successfully today. Read access is no longer sufficient; write access is
required.
Evidence that nothing in this repo or on the fleet does that:
~/.local/state/st2/default/catalog, including the archive) isowned by a single uid,
schickling—find -printf '%u'over the tree returns exactly one name.st2.serviceis a systemd user unit withUser=empty, so the supervisor and everything itlaunches run as
schickling.setuid,seteuid,sudo -u,runuserandUser=do not appear in any.rs,.nix,.kdlor.toml.So there is no non-root, non-
schicklingreader, and the narrowing is unobservable on this fleet.The reasoning is recorded on
shared_existingin the source, not just here.Smaller diagnostic notes
codex_app_server::acquire_owner_lock: contention is nowOk(None)mapped to the sameoperator-visible message,
"Codex runtime already has an owner at {path}". Previously thatmessage carried the
EWOULDBLOCKio::Erroras its source; now the contention arm carries nosource (the non-contention error arm still does). Pinned unchanged by the existing
codex_app_server::tests::runtime_owner_lock_is_nonblocking_and_released_on_close.harness_state::lock_exclusive,event::StreamLock::exclusiveandmessage::SentLockkeeptheir exact top-level messages and now attach the underlying
io::Erroras a source where theypreviously discarded it.
pretrust::ConfigLockkeeps its"locking {path}: {errno}"stringbyte-for-byte.
harness_state::lock_exclusivealso widensO_WRONLYtoO_RDWR, same single-uid argument asabove; it creates its own lock file
0600.Inert, but stated anyway
Two changes that the diff carries and that observably change nothing:
catalog_transaction.rs:1439,codex_app_server.rs:2957,harness_state.rs:697,resource_observe.rs:609— previously handed back a bareFileand released the lock only as aside effect of
close(2). They now hand back aFileLock, whoseDropissues an explicitLOCK_UNbefore the sameclose. That is identical unless the descriptor has been duplicated,because
closeon the last descriptor of the open file description releases the lock anyway. Nosite dups: none calls
try_clone/dup, none passes the descriptor to a child (all carryO_CLOEXEC), so the explicit unlock is redundant rather than new behaviour.src/lib.rsgains a module line forflock. Every item in it ispub(crate)and the moduleitself is declared
mod flock;(private, like the siblingfsatomictransport), so the crate'spublic surface is unchanged in both directions.
The thinnest safety net, filled in
harness_state,harness_context,context,message,pretrustandresource_observehadno dedicated lock test anywhere, and this PR changes their open mode, permissions and symlink
behaviour. The regression oracle is cheap and deterministic and needs no threads:
flocklocks theopen file description, so a second
openof the same lock file in the same process contendswith a live guard.
message::tests::inspect_sent_leaves_no_guard_held_and_creates_no_sender_state— theload-bearing lifetime in the set.
inspect_senttakes up to two sequential shared guards aroundone unlocked read; neither may outlive the call, or a doctor read would block every subsequent
publication by that sender. Scope, precisely: the test pins the first guard only. Once the
lock file exists,
shared_existinganswersSomeandinspect_sentearly-returns holding thatfirst guard, which is therefore the guard whose release is proven. Reaching the second guard
needs
shared_existingto answerNoneand thenSome— a race a single-threaded test cannotarrange — so it is left unpinned rather than fake-covered, and the assertion text and doc
comment say so. The test also asserts the read-only path creates no sender state.
message::tests::the_ledger_lock_excludes_a_second_holder_and_releases_on_droppretrust::tests::the_trust_lock_is_created_private_and_excludes_a_second_holderresource_observe::tests::the_scope_lock_excludes_a_second_holder_and_releases_on_dropflock::tests::contention_under_wait_now_is_ok_none_and_a_dropped_guard_releases,flock::tests::a_shared_holder_admits_a_second_reader_but_not_a_writer,flock::tests::create_new_refuses_an_existing_lock_file_and_existing_refuses_a_missing_oneEvery one of these was mutation-checked: with
O_NOFOLLOWremoved fromflock::open,SentLock::exclusiveweakened toMode::Shared, andinspect_sent's early-return guard (the onethe test actually exercises) leaked via
std::mem::forget, all eight relevant pins fail; with themutations reverted (diff verified identical) all eleven pass.
Not in this PR
resource_profile::lock_publication— see above.host_lock.rs— not a flock site.flockinsrc/run/tests.rs,tests/agent_presentation.rs,crates/st2-resource-providers/src/vista.rs— adversaries, deliberately raw.Independent review
A reviewer with a clean context was asked to falsify the exclusivity claim above. Verdict: the
three stated behaviour changes plus the four diagnostic notes are the only ones — no bug, no
deadlock, no blocking/non-blocking flip, no guard-lifetime regression. It re-derived
CatalogLock'sordering line by line (canonicalize,
.st2/creation, root fsync, controlopenat, open, BOTHdebug checkpoints,
flock, apply marker, generation intent, struct build, intent recovery — allunchanged, both context strings unchanged, and the checkpoints still strictly between the open and
the hold, so all six
ST2_TEST_CATALOG_LOCK_ATTEMPTconsumers observe what they observed), mappedevery site's
Mode/Wait/Open1:1 againstorigin/main, and confirmed every migrated guard is anamed binding rather than a temporary.
It surfaced two changes worth stating, both inert, now in the section above; and one honesty fix,
applied in the last commit: the
inspect_sentpin's assertion claimed to cover both sequentialguards, but the test's own setup creates the lock file first, so
shared_existinganswersSomeandthe function early-returns holding the FIRST guard — the second is never reached. Reaching it needs
NonethenSome, a race a single-threaded test cannot arrange, so the test now says exactly whatit proves instead of faking the coverage.
Posted on behalf of @schickling
agent_identitysessionagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile