Repository navigation
fix(core): release every file lock at once, even while a command is starting - #355
Merged
Merged
Conversation
…tarting Reopening a session right after closing it could be refused as already open in another process, and other openmax processes could wait on the session index, trust.lock, or ledger.lock with no visible holder. It happened whenever a bash call, external tool, or hook was being started at the moment of release. Each of these locks is a flock(2), which belongs to the open file description, and the harness released it only by closing its descriptor. Those children are spawned through fork and exec (each needs a session of its own), and between the two a child holds a copy of every descriptor the harness has open, so the lock stayed held until the child exec'd. Every lock is now held through a guard that unlocks explicitly before the file closes: the session owner, the index lock, trust.lock (a refused change included), and ledger.lock. The lock taken and the order locks are taken in are unchanged, so older binaries still exclude this one.
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.
Why
Open Max guards sessions, the session index, trust.json, and the usage ledger with flock(2) locks. A flock belongs to the open file description, not to one descriptor, and a child that is being spawned (a bash call, a tool, a hook) holds a copy of every descriptor the harness has open until it execs. The session owner and the index lock were released only by closing the file, and trust.lock was too whenever a trust change returned early with an error. So a lock released while a command was starting stayed held by that child:
The ledger lock already unlocked explicitly, but not when a panic unwound past it.
Summary
FileLock, a guard that takes the same exclusive flock(2) and calls an explicit unlock when it is dropped, before the file closes. The unlock releases the lock no matter which spawning child still holds a copy of the descriptor.claim_session), the index lock (lock_indexandlock_store),trust::update(refused changes now release too), and both ledger lock sites (with_lockandrecord_usage).PausedSpawn, compiled only in Unix test builds) that runs a real child throughrun_processand holds it between fork and exec, so tests can release locks inside the window.raw_flocktest helper, which stands in for an older binary, now returns aFileLocktoo, so a child another test is spawning cannot hold its lock past the drop.Test Plan
sessions::tests::a_released_session_reopens_while_a_child_is_spawning: with a child held between fork and exec, a released session reopens from the same core and then from a second core, and a held session still refuses a second owner. Fails on main with the reopen refused as open in another process.sessions::tests::a_released_index_lock_is_free_while_a_child_is_spawning: a dropped index lock can be taken by an independent flock while a child is spawning. Fails on main.trust::tests::a_refused_trust_change_frees_its_lock_while_a_child_is_spawning: a trust change that returns an error releases trust.lock while a child is spawning. Fails on main.cargo test --workspace --locked: passes.cargo +1.97.0 clippy --workspace --all-targets --locked -- -D warnings: passes.Safe to merge based on the changed code and completed checks.
What we checked:
Summary
The PR adds an explicit-unlock guard for session, index, trust, and ledger file locks, plus Unix tests for lock release while a child is between fork and exec. A released session could be reopened during that window, and a refused trust change released its lock. Focused checks found no issue requiring a change before merging.
Reviews (1) · Last reviewed commit: "fix(core): release every file lock at on..."