Config and State saves have no file locking (watch races one-shot commands) #10
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Broken out from ECO-15 (bread-ecosystem audit, 2026-09-05). Verified still present on main as of 2026-09-09.
Config::save()(src/config.rs:360) andState::save()(src/state.rs:29) both serialize and thenfs::write, with no flock or lockfile.breadcrumbs watchruns continuously in the background. A concurrent one-shot invocation (add,forget,profile set, orflow::run's own credential clearing) can interleave a read, modify, write on the samebreadcrumbs.toml/networks.toml/state.toml, and whicheverfs::writelands last silently wins. The existing "every mutating command goes through one save method" discipline keeps the files internally consistent within a single process but does nothing across processes.Suggested fix: take an advisory lock (flock on a sidecar
.lockfile in the config dir and the state dir) around the load, modify, write sequence in both save paths, so a one-shot command and the watch daemon serialize instead of clobbering each other.Latent rather than a reproducible bug today (needs two genuinely concurrent invocations), but
watchis designed to run alongside one-shot commands, so it is worth closing.PR: #11
New
src/lock.rs: an exclusiveflock(2)(std::fs::File::lock, no new dependency) on a sidecar.lockfile in a directory, held via aGuardfor the length of a load, modify, write cycle.app::real_mainholds the config-dir lock for the whole run of every command exceptwatch(long-lived, locks per tick),cd, andedit(interactive editor).watchloop locks for the working part of each tick and releases it for the idle wait, so one-shot commands are not blocked between ticks.State::savetakes the state-dir lock internally (never callsState::load, so no self-deadlock; callers take config then state).Config::save/State::savenow also write atomically (temp then rename,0o600up front) viabread_utils::atomic, so a reader that skips the lock still never sees a half-written file. This replaces the post-write chmod.Tradeoff noted in the PR: a slow command holding the lock (
init --wait N) can stall the daemon's next tick by up to that long; a failed lock acquire is non-fatal (log + proceed unlocked).Verified:
cargo build --release --locked,cargo clippy --all-targets --locked -- -D warningsclean;cargo test --release --lockedall pass (69 lib incl. 3 new lock tests, one spawning a thread to assert the secondacquireblocks until the firstGuarddrops; 42 cli; 36 flow_watch). My changes add zero newcargo fmtdiffs (repo has pre-existing drift, not CI-gated, left alone).Not merged: CI is red on PR #11 (check / check (push) failed after about a minute on commit
0d97c637). Per the merge rules I don't merge on a failing check, so this stays in review rather than todo.For what it's worth, the diff itself reads clean and matches the issue: a sidecar flock in the new src/lock.rs held across the load-modify-write cycle in app::real_main (except watch, cd, edit, each with a stated reason), State::save locking internally with a documented no-deadlock argument, and both save paths switched to bread_utils::atomic::write_atomic. The concurrency test in lock.rs actually spawns a thread and asserts the second acquire blocks until the first guard drops, which is real behavioral coverage, not just structural. I pulled the branch locally and ran
cargo clippy --all-targets --locked -- -D warningsandcargo test --release --lockedagainst the same bread-utils v0.7.2 pin the PR uses: both clean, 69 lib plus 42 cli plus 36 flow_watch tests passing, matching the PR's own verification notes. So this may be a flaky runner or a stale docker cache on the self-hosted CI rather than a real problem in the code, but I can't confirm that from here. Worth a rerun or a look at the actual job log before this goes back to review.