fix(mcp): acquire the mutation lock before the confirm-path preflight read (NRN-106)#111
Merged
Conversation
…-106) vault.set, vault.delete, vault.move, vault.new, vault.rewrite_wikilink, and vault.apply built their plan (graph-index load, preflight, target resolution) before acquiring the per-vault mutation lock on the confirm path, leaving a read-lock window where a concurrent norn writer could drift a file past both the plan-time hash checks and the applier's index-snapshot check. Reorders each tool to acquire the lock first, spanning index load through apply, mirroring vault.edit's NRN-99 fix and the CLI arms that already lock before their cache load. Dry-run stays lock-free. Human-facing output, exit codes, and envelope shapes are unchanged in the uncontended case.
Representative coverage for the confirm-path lock reorder: holds the vault's .mutation.lock via fs2 (same mechanism tests/mutation_lock.rs uses for the CLI arms), then calls handle() with a target that does not exist. A preflight-first ordering would surface "doc not found" before ever touching the lock; with the lock acquired first, the call instead times out on the lock-contention refusal. Proves order, not just presence.
Adds NORN_MUTATION_LOCK_TIMEOUT_MS, mirroring the cache write lock's NORN_CACHE_LOCK_TIMEOUT_MS knob exactly: the env read is #[cfg(debug_assertions)] and compiled out of release builds, so no environment can shrink the production 5s budget. Lets contention tests hit MutationLockTimeout in ~150ms instead of stalling the suite 5s per contended acquire.
…alidate set args pre-lock
- acquire_mutation_lock's doc is now the single canonical statement of
the NRN-99/NRN-106 ordering invariant (on confirm, lock before ANY
read that feeds the write; dry-run never locks; cheap read-free arg
validation stays pre-lock); the previous text still described the old
lock-after-plan order. The seven per-tool comments slim to one line
pointing at it.
- apply.rs and delete.rs guard spelling unified to the edit.rs shape
(`if p.confirm { Some(..) } else { None }`).
- set.rs runs expand_list_ops (pure push/pop argument validation) before
the lock, mirroring apply.rs's parse-before-lock pattern, so malformed
input never contends. No test pinned the old precedence (the refusal
tests assert Err + disk untouched, both preserved).
- CHANGELOG: note the deliberate tradeoff — the lock now spans planning
and the index load, so an external concurrent writer on a large cold
vault is more likely to hit the 5s timeout.
…on tools Replaces the single set-only ordering test with one parameterized test in mcp::mutate covering set, edit, delete, move, new, rewrite_wikilink, and apply: hold the vault's .mutation.lock via fs2, call each handler with confirm:true and a bogus target, and assert the lock-contention refusal fires without the error ever naming the target — proving the lock beats the preflight read wherever a bogus target is preflight- observable (set/edit/delete/move/new); rewrite_wikilink and apply pin contention-refusal-under-held-lock. Uses the debug-only NORN_MUTATION_LOCK_TIMEOUT_MS override (150ms), so all seven contended acquires cost ~1s total instead of 35s. A Drop guard removes the per-vault state dir so the lock file never leaks into the real XDG state home (env stays untouched: set_var is process-global and races sibling tests, per the server.rs isolation note).
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.
Summary
MCP mutation tools now acquire the per-vault mutation lock BEFORE the confirm-path preflight read, closing the same read→lock TOCTOU window NRN-99 fixed for
vault.edit(NRN-106, Phase 1.5 pre-routing hardening under NRN-223). This matters now because write-routing (NRN-229+) moves CLI mutations off the correct-order Direct arms onto these exact daemon paths.What changed
vault.set,vault.delete,vault.move,vault.new,vault.rewrite_wikilink,vault.applylock first onconfirm: true, spanning the index load through apply; dry-run stays completely lock-free.vault.applydeliberately parses/validates the plan (no vault reads) before locking;set's pure argument validation (expand_list_ops) moved pre-lock so malformed input fails fast without contending. The plan-time doc-hash CAS stays as defense-in-depth. Guard spelling unified across all seven handlers.acquire_mutation_lock's doc states it once — on confirm, the lock precedes ANY read that feeds the write; dry-run never locks; read-free arg validation stays pre-lock — and the per-tool comments point there (the old doc described, and effectively endorsed, the pre-fix ordering).NORN_MUTATION_LOCK_TIMEOUT_MSdebug-gated override (mirrorsNORN_CACHE_LOCK_TIMEOUT_MS; compiled out of release builds) — the seven contended acquires cost ~1.2s total, and the suite drops a previous 5s stall.Review
Adversarial review verified the reorder clean across all six tools (lock scoping/RAII, guard direction, no in-process double-acquire, daemon call-lock serialization unaffected) and drove the hardening: the timeout knob, hermetic tests (the original ordering test leaked a lock file into the real XDG state home), seven-tool order pinning, the canonical doc, and the fail-fast validation move. Deferred as tracked tasks: structural
with_confirm_lockextraction (NRN-233),move_doc --parentsmkdir into the applier (NRN-234).Verification
check --locked/clippy -D warnings/fmt --check;Cargo.lockunmodifiedcargo test --workspace: 2349 passed, 0 failed