Config and State saves have no file locking (watch races one-shot commands) #11

Merged

Breadway merged 2 commits from fix/crumb-10-config-state-lock into main

2026-09-11 18:22:17 +08:00

Breadway commented

2026-09-09 20:19:26 +08:00

Owner

Summary

Config::save() and State::save() serialised and then fs::write() with no lock. breadcrumbs watch runs continuously alongside one-shot commands (add, forget, scan, profile set, and flow::run's own credential clearing), each doing a load, modify, write cycle over the same breadcrumbs.toml / networks.toml / state.toml. Two genuinely concurrent cycles could interleave and the later write silently drop the earlier one's change.

Approach

An exclusive flock(2) on a sidecar .lock file, held for the whole load, modify, write cycle, so concurrent cycles queue instead of clobbering each other. Plus atomic writes so a lock-free reader never sees a half-written file.

src/lock.rs (new): acquire(dir) -> Guard, an exclusive std::fs::File::lock() on <dir>/.lock. No new dependency (File::lock is stable; bread-utils' own singleton module already uses File::try_lock). The .lock file is left on disk between acquisitions (unlinking it per use would race the path).
app::real_main takes the config-dir lock for the whole run of every command except:
- watch (long-lived; it locks per tick instead, see below)
- cd (no cycle) and edit (opens $EDITOR; holding the lock for an interactive session would stall the daemon, and the editor does its own write)
The watch loop takes the config-dir lock for the working part of each tick (reload + schedule switch + any flow::run_quiet) and releases it before the idle wait, so one-shot commands are not blocked between ticks. The pre-loop initial flow is covered too.
State::save takes the state-dir lock internally. It never calls State::load, so it cannot self-deadlock, and every caller that also holds the config lock takes it in config-then-state order.
Config::save / State::save now write atomically (temp file then rename; 0o600 set on the temp file up front for the two config files) via bread_utils::atomic. This replaces the post-write secure_permissions() chmod, which had a brief window where the file existed with default perms.

Config::load / save and State::load themselves stay lock-free; only the small number of cycle-owning call sites lock, so the same process never takes the config lock twice and deadlocks.

Tradeoff

A slow command that holds the config lock (init --wait N) can stall the watch daemon's next tick by up to that long. That is deliberate: a brief stall is better than a corrupt or dropped write, and the lock acquire failing at all is treated as non-fatal (log and proceed unlocked) so a weird filesystem never bricks the daemon.

Verification

cargo build --release --locked, cargo clippy --all-targets --locked -- -D warnings: clean
cargo test --release --locked: 69 lib (incl. 3 new lock tests, one of which spawns a thread and asserts the second acquire blocks until the first Guard drops) + 42 cli + 36 flow_watch, all pass
cargo fmt: my changes add zero new diffs (the tree has pre-existing fmt drift that this repo's CI does not gate; left untouched)

Closes #10

## Summary `Config::save()` and `State::save()` serialised and then `fs::write()` with no lock. `breadcrumbs watch` runs continuously alongside one-shot commands (`add`, `forget`, `scan`, `profile set`, and `flow::run`'s own credential clearing), each doing a load, modify, write cycle over the same `breadcrumbs.toml` / `networks.toml` / `state.toml`. Two genuinely concurrent cycles could interleave and the later write silently drop the earlier one's change. ## Approach An exclusive `flock(2)` on a sidecar `.lock` file, held for the whole load, modify, write cycle, so concurrent cycles queue instead of clobbering each other. Plus atomic writes so a lock-free reader never sees a half-written file. - **`src/lock.rs`** (new): `acquire(dir) -> Guard`, an exclusive `std::fs::File::lock()` on `<dir>/.lock`. No new dependency (`File::lock` is stable; `bread-utils`' own `singleton` module already uses `File::try_lock`). The `.lock` file is left on disk between acquisitions (unlinking it per use would race the path). - **`app::real_main`** takes the config-dir lock for the whole run of every command *except*: - `watch` (long-lived; it locks per tick instead, see below) - `cd` (no cycle) and `edit` (opens `$EDITOR`; holding the lock for an interactive session would stall the daemon, and the editor does its own write) - **The `watch` loop** takes the config-dir lock for the working part of each tick (reload + schedule switch + any `flow::run_quiet`) and releases it before the idle wait, so one-shot commands are not blocked between ticks. The pre-loop initial flow is covered too. - **`State::save`** takes the state-dir lock internally. It never calls `State::load`, so it cannot self-deadlock, and every caller that also holds the config lock takes it in config-then-state order. - **`Config::save` / `State::save` now write atomically** (temp file then rename; `0o600` set on the temp file up front for the two config files) via `bread_utils::atomic`. This replaces the post-write `secure_permissions()` chmod, which had a brief window where the file existed with default perms. `Config::load` / `save` and `State::load` themselves stay lock-free; only the small number of cycle-owning call sites lock, so the same process never takes the config lock twice and deadlocks. ### Tradeoff A slow command that holds the config lock (`init --wait N`) can stall the watch daemon's next tick by up to that long. That is deliberate: a brief stall is better than a corrupt or dropped write, and the lock acquire failing at all is treated as non-fatal (log and proceed unlocked) so a weird filesystem never bricks the daemon. ## Verification - `cargo build --release --locked`, `cargo clippy --all-targets --locked -- -D warnings`: clean - `cargo test --release --locked`: 69 lib (incl. 3 new `lock` tests, one of which spawns a thread and asserts the second `acquire` blocks until the first `Guard` drops) + 42 cli + 36 flow_watch, all pass - `cargo fmt`: my changes add zero new diffs (the tree has pre-existing fmt drift that this repo's CI does not gate; left untouched) Closes #10

Breadway added 1 commit

2026-09-09 20:19:26 +08:00

fix(config,state): serialise config/state writes with an advisory lock

check / check (push) Failing after 1m3s

Details

0d97c6377f

Config::save() and State::save() serialised and then fs::write() with no
lock. breadcrumbs watch runs continuously alongside one-shot commands
(add, forget, scan, profile set, and flow::run's credential clearing),
each doing a load, modify, write cycle over the same
breadcrumbs.toml / networks.toml / state.toml. Two concurrent cycles
could interleave and the later write silently drop the earlier one's
change.

- new src/lock.rs: an exclusive flock(2) on a sidecar .lock file in a
  directory, held via a Guard for the length of a load, modify, write
  cycle. std::fs::File::lock, no new dependency.
- app::real_main holds the config-dir lock for the whole run of every
  command except watch (long-lived, locks per tick), cd and edit (no
  cycle / interactive editor).
- the watch loop takes the config-dir lock for the working part of each
  tick and releases it for the idle wait, so one-shot commands are not
  blocked between ticks; the initial pre-loop flow is covered too.
- State::save takes the state-dir lock internally (it never calls
  State::load, so it cannot self-deadlock).
- Config::save and State::save now write atomically (temp file then
  rename, 0600 set up front for the config files) via
  bread_utils::atomic, so a reader that does not take the lock still
  never sees a half-written file. Replaces the post-write
  secure_permissions() chmod.

Closes #10

Breadway referenced this pull request

2026-09-09 20:19:36 +08:00

Config and State saves have no file locking (watch races one-shot commands) #10

Breadway referenced this pull request

2026-09-11 11:02:04 +08:00

Config and State saves have no file locking (watch races one-shot commands) #10

Breadway referenced this pull request from a commit

2026-09-11 11:26:25 +08:00

test(lock): fix cross-thread clock skew in the blocking-acquire test

Breadway added 1 commit

2026-09-11 11:26:25 +08:00

test(lock): fix cross-thread clock skew in the blocking-acquire test

check / check (push) Successful in 1m47s

Details

8bc74afd57

Timed the wait from the waiter thread's own clock, started only once the
OS scheduled it, instead of from a single shared instant captured before
spawn. The gap between those two clocks (tens of microseconds) could land
the measured wait a hair under the 150ms threshold even though the lock
blocked for the full duration, which is what failed CI on PR #11 (149.84ms
observed vs. a 150ms assertion).

Breadway merged commit d077ffb12d into main

2026-09-11 18:22:17 +08:00

Breadway deleted branch fix/crumb-10-config-state-lock

2026-09-11 18:22:17 +08:00

Breadway referenced this pull request from a commit

2026-09-11 18:22:18 +08:00

Config and State saves have no file locking (watch races one-shot commands) (#11)