fix(src): close KB watcher start/start TOCTOU race#72
Merged
Conversation
start_kb_watcher_impl previously performed the lifecycle steps in this order: 1. KbWatcher::new(&path) 2. watcher.start() (spawns notify threads, starts watching) 3. KB_WATCHER.lock() 4. *guard = Some(watcher) Between 2 and 3 the watcher is already running on the filesystem but unreferenced by the global slot. Two concurrent start_kb_watcher invocations — e.g., a settings "Start" click that double-fires, or a frontend that retries after a timeout while the first call is still in flight — could each pass their own check-then-act without observing each other, and both reach step 4 with their own watcher instance. The second guard assignment would overwrite the first, but the first watcher's notify threads keep running on the filesystem as an orphan with no handle for stop_kb_watcher to reach. Fix: hold the KB_WATCHER StdMutex guard across the full check/create/start/store sequence. A racing second starter now observes the populated slot and returns Ok(false) without ever constructing a second KbWatcher. KbWatcher::new and watcher.start() are synchronous, so holding the guard across them does not block other async tasks on the runtime; the tokio::spawn event-forwarder is moved outside the critical section so receiving events never contends with KB commands. Added a SAFETY-style explanatory comment inline; no API or behavior change for callers other than the correct race resolution. cargo check --all-targets clean; cargo test --lib passes 311/312 (one pre-existing #[ignore]'d model-download test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
start_kb_watcher_implconstructs + starts aKbWatcherbefore acquiring theKB_WATCHERStdMutex. Two concurrent starts can both construct and start their own watcher instances before either stores in the slot — the second assignment overwrites the first, and the first watcher's notify threads keep running on the filesystem as an unreferenced orphan thatstop_kb_watchercan't reach.Fix
Hold the
KB_WATCHERguard across the full check/create/start/store sequence. A racing second starter now observes the populated slot and returnsOk(false)without ever constructing a second watcher.KbWatcher::newandwatcher.start()are synchronous — holding the StdMutex guard across them is safe under tokiotokio::spawnevent-forwarder stays outside the critical section so emitting events never contends with KB commandsAudit context
The original 5-agent audit described this as "global state without
Arc<Mutex<…>>." On reading the file, the mutex was already in place — but the sequencing insidestart_kb_watcher_implleft a genuine TOCTOU gap that the audit's framing missed. This PR fixes the real race.Test plan
cargo check --all-targetscargo test --lib— 311/312 pass (one pre-existing#[ignore]'d model-download test)false🤖 Generated with Claude Code