fix(engine,mac): release the single-instance flock explicitly (#324) - #326
Merged
Conversation
`single_instance::tests::drop_releases_so_next_acquire_succeeds` reds `ci-success` in roughly 3 of every 30 `main` runs, on both ubuntu-latest and macos-latest, panicking with `AlreadyHeld(<our own pid>)`. Root cause: flock(2) locks live on the open file description, not on the fd or the process. A fork()ed child inherits a duplicate of the lock fd and keeps that description — and the lock — alive until the fd closes at exec. Rust's `File` drop calls only close(2), never flock(LOCK_UN), so a sibling test in the same test binary that spawns a subprocess during the window we hold the lock makes our drop a no-op and the next acquire() see WouldBlock. The PID in the message is our own because we wrote it. This commit only pins the reproduction; the fix is the next commit. * `tools/repro/single-instance-flake.sh` loops the suite with thread and CPU-load knobs and reports a measurable failure rate (6/300 observed locally at the defaults). `--scope workspace` mirrors CI's `cargo test --workspace --exclude roost-linux`; the default `--scope engine` is ~60x cheaper per iteration and is where the race actually lives, because only forks from the SAME test binary can inherit the fd. * `drop_releases_even_when_a_forked_child_inherited_the_fd` is the deterministic form: it clears FD_CLOEXEC so the child provably keeps the description past exec. It is `#[ignore]`d here because it fails by design until the fix lands. codex review findings, all fixed: the failure classifier matched the test name in the passing-test listing too (now keys on libtest's failure-only `---- <test> stdout ----` header); `--scope workspace` did not mirror CI's `--exclude roost-linux`; the spawned child was not reaped on a panic path (now RAII). Refs #324. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
`close(2)` alone does not release a flock. The lock belongs to the open file description, so a `fork()`ed child that inherited the lock fd keeps it alive until that child execs — and in that window our release is a silent no-op. That is #324: `AlreadyHeld(<our own pid>)`, because the pid in the file is one we wrote ourselves. Both implementations now `flock(LOCK_UN)` explicitly before closing, and both grew a regression test that spawns a child holding the inherited fd. The Swift test uses raw `posix_spawn` rather than Foundation's `Process`: on Darwin `Process` spawns with POSIX_SPAWN_CLOEXEC_DEFAULT, which closes every fd in the child regardless of FD_CLOEXEC, so the test passed vacuously against the unfixed code. Falsifiable floor (plan 024 D3.5), measured with the repro committed in the previous commit: before 6/300 iterations failed (all classified as #324) after 0/400 iterations failed Both new tests were also verified to fail against their own fix reverted. Also here: * `InstanceLock::release()` is deleted (plan D3). It dropped the flock and then unlinked a path another process may already have opened by name, and it had no caller outside its own test. * `a_dead_process_releases_the_lock` covers process exit alongside the RAII drop test. Plan D3.4 keeps both shapes deliberately — one tests `Drop`, the other tests what the UI relies on after a crash. codex review findings: * fixed: `acquire` could drop a bare locked `File` on the PID-write error path, reopening the same close-only hole. The `InstanceLock` is now constructed before anything fallible, so every `?` releases via `Drop`. * documented, not fixed: if the process is SIGKILLed (so `Drop` never runs) while a just-forked child has not yet reached `exec`, that child keeps the lock until it does. The window is one fork→exec, it self-heals, and closing it would mean changing how every subprocess in the tree is spawned. Noted in the module docs. * by design: the cross-process test would also pass before this fix. It asserts a different property (plan D3.4); the inherited-fd tests are the regression guards. Closes #324. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
fs2 0.4.3 was a single-symbol dependency — `try_lock_exclusive` in one file — and std has had the same `flock(2)` wrapper since 1.89; the toolchain is pinned at 1.97.1. This is dependency hygiene, not a fix (plan 024 D3.3): both call the same syscall with the same semantics, so nothing about contention changes. The actual #324 fix was the previous commit's explicit LOCK_UN. Not mechanical: std signals contention through a `TryLockError` variant rather than an `io::ErrorKind::WouldBlock`, so the error match is rewritten rather than renamed. `File::unlock()` is std's flock(LOCK_UN), so `Drop` keeps its fix. Gate: `cargo test --workspace --exclude roost-linux` green; the #324 repro stayed at 0/200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces Rust ChangesSingle-instance locking
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mac/Sources/Roost/SingleInstance.swift`:
- Around line 76-84: Update the Darwin.write failure path in SingleInstance
initialization to call roost_flock(fd, LOCK_UN) before Darwin.close(fd). Ensure
the post-lock error cleanup releases the lock explicitly, since
SingleInstance.deinit is not guaranteed to run before initialization completes.
In `@tools/repro/single-instance-flake.sh`:
- Around line 146-148: Update the successful test iteration cleanup in the main
loop of single-instance-flake.sh so the per-iteration log is removed only when
the keep flag is 0. Preserve the existing behavior for failed iterations and
ensure --keep retains successful iteration logs through final cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bd55bc0-e565-4578-97e8-470bcabb471d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/roost-engine/Cargo.tomlcrates/roost-engine/src/single_instance.rsmac/Sources/Roost/SingleInstance.swiftmac/Tests/RoostTests/SingleInstanceTests.swifttools/README.mdtools/repro/README.mdtools/repro/single-instance-flake.sh
💤 Files with no reviewable changes (1)
- crates/roost-engine/Cargo.toml
* `SingleInstance.acquire` closed the fd without LOCK_UN when the PID write failed. No `SingleInstance` exists on that path, so `deinit`'s release can't run — the Swift twin of the codex finding already fixed on the Rust side. * The repro script deleted successful iterations' logs even under `--keep`, which promises to keep them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
This was referenced Aug 10, 2026
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
1 participant
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.
Plan 024 (pre-release quality for the iced Linux release), PR 1 of 6 — workstream W-C.
This lands first because
ci-successis the required check for every other PR in theplan and this flake reds it in roughly 3 of every 30
mainruns.Closes #324.
What was wrong
close(2)does not release aflock(2). The lock belongs to the open filedescription, not the fd and not the process, so a
fork()ed child that inheritedthe lock fd keeps the lock alive until that child reaches
exec. Rust'sFiledropand Swift's
Darwin.close(fd)both call onlyclose(2)— so during any fork→execwindow the release is a silent no-op, and the next
acquire()seesWouldBlock.That is exactly the reported symptom:
AlreadyHeld(<our own pid>), on bothubuntu-latestandmacos-latest, never reproducible from a filtered local run.The pid in the message is ours because we wrote it into the lock file ourselves; the
contending holder is our own forked child.
Confirmed at the syscall level with a standalone C program before any Rust was
touched:
What changed
7a5720e— commit the reproduction (plan D3.5 requires a committed repro, not aprose paragraph).
tools/repro/single-instance-flake.shloops the suite withthread-count and CPU-load knobs and reports a measurable failure rate.
--scope workspacemirrors CI'scargo test --workspace --exclude roost-linux; the default--scope engineis ~60× cheaper per iteration and is where the race actually lives —only forks from the same test binary can inherit the fd, and those are
roost-engine's own subprocess-spawning tests, not the PTY integration tests (whichare separate binaries). The script classifies each failure as "#324" vs "unrelated"
so an incidental red can't be misread as a reproduction.
dfda3e3— the fix.flock(LOCK_UN)explicitly before the file closes, inboth implementations.
LOCK_UNclears the lock on the description itself, whichevery inheriting fd shares, so release becomes unconditional.
49d65db—fs2→ stdFile::try_lock. Dependency hygiene, explicitly notpresented as the fix (plan D3.3): both call the same syscall with the same
semantics. Not mechanical, though — std signals contention with a
TryLockErrorvariant instead of
io::ErrorKind::WouldBlock, so the error match is rewritten.fs2is gone fromCargo.tomlandCargo.lock.Swift rides along
Per the plan's constraint 4 ("fix Swift too where a known fix exists"),
mac/Sources/Roost/SingleInstance.swifthad the identical defect — itsdeinitonly closed the fd, while the app
forkpty()s on every PTY spawn. Same one-line fix,same regression test.
The Swift test deliberately uses raw
posix_spawnrather than Foundation'sProcess: on DarwinProcessspawns withPOSIX_SPAWN_CLOEXEC_DEFAULT, whichcloses every fd in the child regardless of
FD_CLOEXEC— the test passedvacuously against the unfixed code until I switched it.
Verification
tools/repro/single-instance-flake.sh(400 iterations,-j 64,--load 4)single_instance::tests::drop_releases_even_when_a_forked_child_inherited_the_fdAlreadyHeld(6147)SingleInstanceTests.releaseOnDeinitSurvivesAForkedChildHoldingTheFDBoth regression tests were verified to fail against their own fix reverted, so
neither is vacuous.
cargo test --workspace --exclude roost-linuxgreen;swift testgreen (694 tests);cargo clippy --workspace --exclude roost-linux --all-targets -- -D warningsclean.The post-fix numbers are macOS numbers. Two of the three observed CI failures were on
Linux; the mechanism is identical (
flockOFD semantics are the same) but I did notre-measure the rate on Linux.
Accepted risk, stated plainly
One window survives and cannot be closed from this file: if the process is
SIGKILLed — so
Drop/deinitnever runs — while a just-forked child has not yetreached
exec, that child's inherited description keeps the lock until it does. Thewindow is one fork→exec, it self-heals, and closing it would mean changing how every
subprocess in the tree is spawned. Documented in the module docs. Found by the codex
review pass, dispositioned rather than fixed.
Review findings
acquirecould drop a bare lockedFileon the PID-write errorpath, reopening the same close-only hole. The
InstanceLockis now constructedbefore anything fallible, so every
?releases throughDrop.the passing-test listing too, so an unrelated red inside the same binary would
have been reported as a reproduction. It now keys on libtest's failure-only
---- <test> stdout ----header.--scope workspacedid not mirror CI's--exclude roost-linux.It asserts a different property — process exit vs RAII drop — and plan D3.4
deliberately keeps both shapes. The inherited-fd tests are the regression guards.
Also here
InstanceLock::release()is deleted (plan D3, "Also"). It dropped the flock and thenunlinked a path another process may already have opened by name, and it had no caller
outside its own test.
No impact on
Dependencies (net −1), privacy, secrets.
Cargo.lockshrinks.Summary by CodeRabbit
Bug Fixes
Tests
Documentation