feat(auth): give the secrets file a real cross-process lock

[cliffhall]

Closes #2082

The decision

Adopt an OS-backed lock. Of the four options weighed in the issue, this PR takes the first: proper-lockfile, the library npm itself locks with.

The framing that settles it is not "lock versus no lock" — #1950 already tried a lock and pulled it — but "borrow versus hand-roll".

What it does and does not close, stated exactly

Closed: two running Inspectors. mkdir is atomic, and a live holder refreshes its lock's mtime every stale / 2 for as long as it lives, so its lock never becomes eligible for takeover. One holds, the other waits. That is the case #1950 actually lost updates in, and the case this issue was filed for.

Not closed: takeover of a lock whose holder died. acquireLock stats on EEXIST, and if stale rmdirs and re-mkdirs — without checking the directory it removed is the one it found stale. So a slow waiter can still delete a fast waiter's fresh lock and claim a replacement, with both proceeding. That is the identical race the hand-rolled election could not close, and it is not closable with what Node exposes: single-winner takeover needs compare-and-swap on a directory entry (renameat2(RENAME_EXCHANGE)), and there is no binding for it and no flock.

And the library's own detection is weaker than it looks (round 4 caught this). updateLock compares mtime only on its refresh tick — stale / 2, so 5s here — while an ordinary mutation is a read, an scrypt derivation and an atomic write, comfortably under a second. In the common case the tick never runs and nobody is told. Worse, release is an unconditional rmdir with no ownership check, so a holder whose lock had been replaced deletes the winner's lock on the way out — turning one compromised holder into two unprotected writers.

So withSecretFileLock supplies a guarded options.fs whose directory removal refuses to delete a lock that is no longer the one it created, identified by inode and birth time.

Two design points, both from round 5:

• It lives in options.fs, not in a check around release(). That is the one seam both removal paths route through — the release path, and the library's signal-exit handler, which rmdirSyncs every registered lock with no ownership check of its own. A guard around release alone leaves a process exit at the wrong moment free to delete the winner's directory. (A test drives a real child to show that handler really does remove a replacement lock.)
• Inode and birth time rather than mtime. Both survive the library's utimes refresh — an mtime comparison would call our own healthy long-held lock compromised — and both change on a delete-and-recreate.

It narrows the destructive window; it does not close it, and round 5 was right to push back on the first version of this claim. The guard is a statSync immediately followed by an rmdirSync, so nothing in this process interleaves — but it is still check-then-act against other processes, and closing that needs the same compare-and-swap Node does not expose. It also rests on filesystem metadata that not every filesystem reports; where absent it degrades to the library's unaided behaviour rather than to a false alarm. Best-effort throughout: the destructive case becomes rare, not impossible.

The window is at least narrow and conditional — it opens only after a holder dies without releasing, since nothing else lets a lock go stale.

This is why the optimistic verify stays underneath, and why it is documented as covering exactly that window rather than as belt-and-braces.

The other three options were not taken, for the reasons the issue states: a generation counter narrows the window without closing it, per-secret hashed files regress a real security property (the file set would leak how many secrets exist and let a guessed account name be confirmed — exactly what encrypting the map as one envelope prevents), and "do nothing" leaves set's persistence contract unmet.

What changed

core/auth/node/file-lock.ts (new) — withSecretFileLock(filePath, fn). Lock at <filePath>.lock, 10s stale window (the library's own default; it refreshes at stale / 2 while the holder lives, so this is how long after a holder dies the file stays unwritable, not how long a mutation may take), and a ~15.3s retry budget for a waiter.

Five things in it are load-bearing:

• realpath: false. By default the library resolves its target through fs.realpath, which is ENOENT on a secrets.json that does not exist yet — i.e. on the very first set, the one call with nothing to fall back on. Resolving lexically instead lets a file be locked into existence.
• The storage directory is created before locking. writeStoreFile creates it, but from inside the locked section — so on a fresh install proper-lockfile failed ENOENT and the first save degraded to an unlocked write. That is the save most likely to be racing another.
• The retry budget outlasts the stale window (~15.3s vs 10s), computed from the schedule rather than written down. Refusing on a held lock (below) is only defensible because a crashed holder resolves by takeover first; a shorter budget would turn one Inspector killed mid-save into a failed save for everyone else on the box until someone deleted the lock by hand.
• ELOCKED refuses; everything else degrades. A lock held past the budget is evidence the lock is working, so proceeding would enter the exact interleaving it exists to prevent, knowingly — set fails instead (delete stays silent per its contract). But a lock that cannot be created is different: this store exists for boxes where the usual mechanism is missing (KeyringSecretStore throws from the AsyncEntry constructor, defeating its own degradation contract (500 on GET /api/servers) #1848 container without D-Bus, Inspector v2.0.0 fails to start on Android/Termux because @napi-rs/keyring is imported unconditionally #1905 Android/Termux), so a read-only $HOME or a mount owned by another uid runs the body anyway with the Secret persistence without an OS keychain: a file-backed (or explicitly in-memory) SecretStore for containers and unsupported platforms #1950 optimistic behaviour underneath, warning once per reason per process. Acquire is probe-then-wait for this reason: proper-lockfile retries on any error, so without a retry-less probe first, a read-only $HOME would burn the full budget re-issuing an identical failing mkdir on every save.
• onCompromised. The library's default throws, from a timer with no caller on the stack, so it lands as an uncaught exception and takes the session down. Replaced with a warning. It is the library's own (slow, tick-based) signal for the stale-takeover race; the release-time ownership check above is what covers the fast case.

FileSecretStore.mutate — now withSecretFileLock(...) around the whole read-modify-write, with the previous body extracted to mutateLocked.

The optimistic verify stays, and is not redundant. A lock is an advisory convention between the processes that take it, so it says nothing about a writer that does not — an editor, a restored backup, a jq one-liner, an Inspector predating this PR. It is also what covers the degrade path above. The two mechanisms answer different questions, and the doc comment now says which is which rather than reading as belt-and-braces.

The in-process queue stays too, and gains a second job: proper-lockfile is not reentrant, so a second lock() from the same process fails ELOCKED indistinguishably from a genuine remote holder. Serializing per resolved path means the lock is only ever contended between processes, which is the only contention it is asked to arbitrate.

Reads still take no lock. writeStoreFile is atomic (write-temp-then-rename), so a reader sees the old file or the new one, never a torn one; locking them would serialize every GET /api/servers behind whatever else holds it, to prevent nothing.

absorbFileSecretsIntoKeyring — orphan adoption and the atomic claim both move the live path around, so they now run under the same lock. The issue named this as the second instance of the same shape.

Its "never throws" contract is preserved explicitly: a lock failure here is caught, warned, and the file left for the next run. It is awaited directly by both resolveSecretStore branches, so an escaping error would fail store resolution and with it the session — refusing is right for a set, where a user is waiting on that value, and wrong at startup.

It gets a lock-free fast path first: one readdir asking whether a secrets.json or a secrets.json.migrating-* orphan exists at all. Without it, the overwhelmingly common startup — keychain available, no file ever written — would create and remove a lock directory on every run, and on a box whose storage directory does not exist yet the lock could not be created at all, so the degrade path would warn about unprotected writes on every single run with nothing to protect. The check is racy by construction and that is fine: it can only be wrong by saying "nothing here" about a file created a moment later, which is a file the next run migrates — the same outcome as a writer that recreates the path after the claim. Everything that acts re-checks under the lock. It is a readdir rather than a stat of secrets.json because an orphan is exactly the case where the live file is absent and there is still everything to migrate.

Dependency placement

proper-lockfile goes in root dependencies and @types/proper-lockfile in root devDependencies, per the repo rule: core/ imports it at runtime, the client builds externalize npm packages, and a published install resolves them from the root manifest. Single install, so verify:dep-lockstep sees no candidate. Imported as a default (import properLockfile from "proper-lockfile") rather than named — it is CJS, and named-import lexer detection is something esbuild and rollup disagree about.

One thing the gate caught

proper-lockfile is root-declared by rule, and tsup externalizes what the client's package.json declares — so it was silently bundled into all three Node builds. Inlining a CJS module into an ESM bundle leaves esbuild's Dynamic require of "path" is not supported shim, which throws at import time: mcp-inspector --cli died before parsing a flag. Caught by the CLI's out-of-process e2e.test.ts.

Fixed by naming it in clients/cli/tsup.config.ts, clients/tui/tsup.config.ts, and clients/web/tsup.runner.config.ts, beside @napi-rs/keyring, which is root-only for exactly the same reason. The rule was undocumented, so it is now stated in AGENTS.md next to the dependency-placement rule that creates it, and mirrored into .github/copilot-instructions.md.

Tests

clients/web/src/test/integration/auth/node/file-lock.test.ts (12 tests, new). The issue notes the existing suite structurally cannot reach this: serialize is one process-wide queue per path, so two in-process callers are ordered before the lock ever sees them. So these spawn a real second process that takes the lock and holds it, resolving on the child's own "acquired" line rather than on a sleep — a pass cannot come from the parent simply getting there first.

• waits for a lock another process holds, then runs (and does not report a degraded write)
• locks a file that does not exist yet — the realpath: false property
• FileSecretStore.set waits on a lock another process holds — asserts the parent's set has not settled 300ms into a child's 700ms hold. Not-yet-resolved is the observation only a real lock produces; asserting merely that both keys survive passes with the lock removed entirely, because the optimistic verify repairs the clobber
• the storage directory is created so the very first save is locked too — same not-yet-settled assertion, target under a directory that does not exist
• runs the body anyway when the lock cannot be created, warning exactly once across two calls
• refuses the save rather than writing alongside a live holder — set rejects SecretStoreUnavailableError, and nothing is written behind the holder's back
• stays silent per the delete contract when the lock is held, leaving the entry intact
• takes over the lock of a holder that died rather than failing the save — pins the RETRY > STALE_MS invariant that makes refusing defensible; stages the dead holder directly (mkdir + backdated mtime) rather than racing a real kill
• does not delete the winner's lock after being taken over — the destructive half of the race, and the fast case the library's tick misses; asserts the replacement directory is the same inode and birth time afterwards, so it was left alone rather than deleted and recreated
• the exit handler this guards against really does delete a lock — a real child takes the lock, has it replaced, and exits without releasing; the replacement is gone. Uses an empty replacement directory on purpose: a non-empty one makes rmdirSync fail ENOTEMPTY and the test would pass for the wrong reason, which is how its first draft fooled itself
• tells the operator to clear a lock that cannot expire on its own — a stray file makes rmdir fail ENOTEMPTY while the directory is still identifiably ours, so the ownership guard cannot swallow a genuine release failure; asserts the message says "by hand" and does not promise expiry
• the lock directory removed and the body kept alive past the refresh tick: the library's own compromise detection fires, and warns rather than crashing the process

Plus three in secret-store-selection.test.ts: no lock and no warning when there is nothing to migrate, an orphan still adopted when the live file is absent, and absorbFileSecretsIntoKeyring resolving rather than throwing when another process holds the lock (the startup-breaking case).

The lock tests are checked by mutation, not by reading. With withSecretFileLock removed from mutate, 4 of them fail. Before review round 1, 0 did.

Verification

npm run ci — clean.

No UI surface, so no screenshots.

Docs

README.md, specification/v2_servers_file.md, and AGENTS.md all carried the "there is deliberately no lock" decision with its rationale. All three now describe the lock, why it is borrowed rather than hand-rolled, why the verify stays underneath it, and why the unavailable case degrades.

[Copilot]

Pull request overview

Adds OS-backed cross-process locking for file-based secret persistence across Web, CLI, and TUI clients.

Changes:

• Adds proper-lockfile around secret mutations and migration claims.
• Adds cross-process integration tests and bundler configuration.
• Updates dependency rules and secret-storage documentation.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
core/auth/node/file-lock.ts	Implements lock acquisition, retries, degradation, and warnings.
core/auth/node/file-secret-store.ts	Locks file mutations.
core/auth/node/secret-store-selection.ts	Locks migration claims and adds a fast path.
clients/web/src/test/integration/auth/node/file-lock.test.ts	Tests lock behavior across processes.
clients/web/src/test/integration/auth/node/secret-store-selection.test.ts	Tests migration fast-path behavior.
clients/cli/tsup.config.ts	Externalizes proper-lockfile.
clients/tui/tsup.config.ts	Externalizes proper-lockfile.
clients/web/tsup.runner.config.ts	Externalizes proper-lockfile.
package.json	Adds runtime and type dependencies.
package-lock.json	Locks new dependencies.
README.md	Documents cross-process locking.
specification/v2_servers_file.md	Updates secret-store concurrency specification.
AGENTS.md	Documents architecture and dependency rules.
.github/copilot-instructions.md	Mirrors the new review rule.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot review round 1 — all four addressed (16613b1)

Mirroring here because inline replies get hidden once the threads go outdated.

#	Comment	Outcome
1	Lock fails ENOENT when the storage directory does not exist, so the first save degrades to unlocked	Fixed — withSecretFileLock creates the parent directory before locking
2	readdir catch treats every failure as "nothing to migrate"	Fixed — only ENOENT/ENOTDIR; everything else falls through to the under-lock checks
3	Missing vitest.shared.mts root pin	Added, with a note on why I read the rule as permitting rather than requiring it
4	The cross-process test does not prove mutate takes the lock	Fixed — now asserts the parent's set has not settled while a child holds the lock

#1 was a genuine bug, not a test gap. writeStoreFile creates the storage directory, but from inside the locked section — so on a fresh install proper-lockfile failed ENOENT and every first save fell to the unlocked path with a warning. That is the save most likely to be racing another (two Inspectors started together both reach it), so the lock was missing from precisely the interleaving it was added to close.

#4 was right about the test having no teeth, and the fix is checked by mutation rather than by reading: with withSecretFileLock removed from mutate, 3 tests now fail. Before this round, 0 did — the optimistic verify repaired the clobber and the old assertion (both keys survive) held regardless.

npm run ci passes clean.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

[cliffhall]

Copilot review round 2 — both upheld (dcd70e2)

Mirroring at PR level since inline threads go outdated.

1. ELOCKED was degrading to an unlocked write — fixed

Backwards, as flagged: exhausting retries on a held lock is evidence the lock is working, and proceeding entered the exact interleaving it exists to prevent, knowingly. set now fails (…the value you just entered was not saved); delete stays silent per its contract.

Two changes came with it:

• Retry budget 3.26s → ~15s, so it outlasts the 10s stale window. Refusing is only defensible because a crashed holder resolves by takeover first — with the old budget, one Inspector killed mid-save would have failed every later save on the box until someone removed the lock by hand. Pinned by a new test staging a dead holder (mkdir + backdated mtime).
• Probe-then-wait. Raising retries exposed a second bug: proper-lockfile retries on any error, so a read-only $HOME would have burned 15s re-issuing an identical failing mkdir on every save. One retry-less probe now separates "held" from "unavailable"; only the latter degrades.

2. The comment overclaimed what proper-lockfile provides — corrected

I verified against lib/lockfile.js@4.1.2 rather than trusting either side. The finding is right: acquireLock rmdirs a stale lock and re-mkdirs without checking the directory it removed was the one it found stale, and mtimePrecision.probe takes the mtime it reads rather than comparing. So "stale-takeover is precisely the problem it has already solved" was false, and it was mirrored into four files — all now corrected.

Two things the finding slightly overshoots, and the corrected text says both:

• The mtime ownership check does exist, in updateLock (isMtimeOurs → ECOMPROMISED), just on the refresh tick rather than at acquire. So it is detection, not prevention — which is still strictly more than the hand-rolled version, which had the same window silently.
• The window only opens after a holder dies without releasing. mkdir is atomic and a live holder refreshes its mtime, so two running Inspectors — the case Secret persistence without an OS keychain: a file-backed (or explicitly in-memory) SecretStore for containers and unsupported platforms #1950 lost updates in, and the case this PR exists for — are genuinely serialized.

On "use an implementation that makes stale takeover single-winner": I do not believe one exists here. It needs renameat2(RENAME_EXCHANGE), Node exposes neither that nor flock, and that impossibility is the premise #2082 was filed on. Pointers welcome; short of one the choice is this or nothing.

I did not add the two-process stale-takeover race test — it would test the library, not this code, and is inherently nondeterministic. The honest response was to stop claiming the race is closed.

---

Also fixed a test that was quietly asserting the opposite of its name: its holder used stale: 60_000 while the waiter used 10_000, and staleness is judged by the waiter's threshold — so the holder was declared stale after 10s and the save succeeded by takeover.

Teeth re-checked by mutation: removing withSecretFileLock from mutate now fails 4 tests (3 last round, 0 before). npm run ci passes clean.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.

[cliffhall]

Copilot review round 3 — all three addressed (833a6ba)

#	Comment	Outcome
1	Round 2's ELOCKED throw escapes absorbFileSecretsIntoKeyring, breaking startup	Fixed — caught, warned, file left for the next run
2	Refusal message says 20s for a ~15.3s schedule	Fixed — RETRY_BUDGET_MS, computed from the schedule
3	PR description still describes the pre-round-2 design	Rewritten

#1 was a regression I introduced in round 2, and the worst of the three because the symptom is "the Inspector will not start". Rejecting on a held lock is right for set — a user is waiting on that value — and wrong at startup, where the hand-off is awaited directly by both resolveSecretStore branches. A stuck writer anywhere on the box would have failed store resolution and taken the session with it. Now caught, with a test that holds the lock and asserts the hand-off resolves; it runs ~15s, confirming it really spends the retry budget and hits the throw rather than passing for some other reason.

#2 — retries * maxTimeout reads 20s for a schedule summing to 15,260ms, since the early attempts are the ramp not the cap. Made the number real rather than rewording around it, so it cannot drift if the policy is retuned.

#3 — the description now carries a What it does and does not close section stating the narrower true claim, the ~15.3s budget with its reason, the ELOCKED-refuses/everything-else-degrades split, and the updated test list. It is marked as revised after round 2 so a later reader can see the claim was corrected rather than wonder which version to believe.

Coverage

The round-3 changes cost secret-store-selection.ts two branches and it fell to 89.62%. Recovered honestly rather than by annotation: the ENOTDIR arm of anythingToMigrate now has a real test (a path whose parent is a file — the other shape of "nothing there"), and the new catch types its parameter Error rather than re-narrowing, since withSecretFileLock rejects only with the SecretStoreUnavailableError it constructs one call away.

npm run ci passes clean — 5948 web tests, 115 Storybook files, all smokes, no coverage errors.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

core/auth/node/file-lock.ts:37

• This overstates compromise detection. In proper-lockfile@4.1.2, onCompromised is reached only from the scheduled updateLock tick (5 seconds with this configuration); the release path does not compare mtime/ownership and directly removes the current lock directory. A normal secret mutation usually finishes before that first tick, so after a stale-takeover race the original holder can remove the winner's lock during release and neither holder is necessarily told. Treat detection as best-effort, or add an explicit ownership check before release; the unconditional “loser finds out” claim is not accurate.

 * What the library adds over the hand-rolled one is that the loser finds
 * out. Its refresh tick compares the lock's mtime against the value it
 * recorded at acquire, so a holder whose directory was replaced is marked
 * `ECOMPROMISED` and told — see `onCompromised` below. The window is also
 * narrow and conditional: it opens only after a holder *dies without
 * releasing*, since nothing else lets a lock go stale.

README.md:507

• The claim that a replaced holder is always marked compromised is too strong. proper-lockfile@4.1.2 checks ownership only on its periodic update tick (5 seconds here), while release removes the lock directory without an ownership check. Since ordinary mutations finish before that tick, a stale-takeover loser can complete and remove the winner's lock without this warning firing. Please describe this as best-effort detection rather than a guaranteed notification.

Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either; what it adds is that the loser is **told** (it compares the lock's mtime against the value it recorded, so a holder whose lock was replaced is marked compromised and warns). The window opens only after a holder dies without releasing.

specification/v2_servers_file.md:264

• ECOMPROMISED is not guaranteed to surface in this race. Version 4.1.2 performs the ownership comparison only on the scheduled refresh tick; its release path does not compare ownership and directly removes the lock directory. A typical mutation can finish before the first 5-second tick, so a stale-takeover loser may complete and delete the winner's lock with no compromise callback. This specification should call detection best-effort unless release-time ownership verification is added.

  - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; what it adds over the hand-rolled version is *detection* — its refresh tick compares the lock's mtime against the value recorded at acquire, so a compromised holder is told (`ECOMPROMISED`, surfaced as a warning) instead of proceeding silently. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being *unavailable*: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one.

AGENTS.md:77

• “Makes the loser detectable” is not unconditional with proper-lockfile@4.1.2. Detection runs only on the periodic update tick (5 seconds here), but release removes the current lock directory without checking that it is still the holder's. Fast mutations can therefore complete after stale takeover and remove the winner's lock before either process receives ECOMPROMISED. Document this as best-effort detection, or add release-time ownership verification.

│   │   │                               #     citing it: it makes two LIVE Inspectors exclusive, and
│   │   │                               #     does NOT make stale takeover single-winner — it makes the
│   │   │                               #     loser detectable (ECOMPROMISED), which is the honest claim.

[cliffhall]

Copilot review round 4 — upheld, and it was a bug (91e1392)

All four comments were the same finding applied to the four files carrying the claim, and they arrived in the Suppressed comments block rather than inline — worth noting, since a reader skimming the Conversation tab sees a review with nothing under it.

The finding

I had written that a holder whose lock was replaced "is told". Checked against proper-lockfile@4.1.2, that is false in the case that actually happens:

• updateLock compares mtime only on its refresh tick — stale / 2, so 5s here — while an ordinary mutation is a read, an scrypt derivation and an atomic write, comfortably under a second. The tick never runs, so nothing fires.
• release → unlock → removeLock is an unconditional rmdir with no ownership check. So a compromised holder goes on to delete the winner's lock on the way out, ending the winner's exclusion too.

That second half is a correctness bug, not a documentation defect — one compromised writer silently becomes two unprotected ones — and unlike the takeover race itself it is fixable here.

The fix

withSecretFileLock records the lock directory's inode and birth time at acquire and re-checks them before releasing. On a mismatch it declines to release and warns; leaving the new holder's lock alone costs nothing, since ours is already gone.

Inode and birth time rather than mtime, deliberately:

• both survive the library's own utimes refresh, so a lock held past one tick is not accused of being compromised — an mtime comparison would have that false positive;
• both change on a delete-and-recreate, which is exactly the event to detect;
• where a filesystem reports neither, the two reads agree and the check concludes "ours", degrading to the library's unaided behaviour rather than to a false alarm. An unreadable baseline at acquire likewise answers "ours" — with nothing to compare against, accusing a healthy lock is worse than releasing it.

On the wording

Taken: detection is now described as best-effort in all four files, not as a guarantee. AGENTS.md carries the negative instruction as well ("do not write that a compromised holder is always told"), since this is the second round spent on the same overclaim and the next person to touch this file should not have to rediscover it.

Tests

• does not delete the winner's lock after being taken over — replaces the lock inside the body, asserts the replacement survives with the same inode and birth time, i.e. was left alone rather than deleted and recreated. Waits for nothing: this is the fast case the tick misses. Verified by mutation — forcing stillOurs to always answer true fails it.
• warns instead of throwing when releasing a lock that is ours fails — a stray file makes rmdir fail ENOTEMPTY while the directory is still identifiably ours, so the ownership check cannot swallow a genuine release failure.
• the existing tick-based test is retained for the slow path, where the library's own detection fires.

npm run ci passes clean — no coverage errors, 115 Storybook files, all smokes.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

[cliffhall]

Copilot review round 5 — both upheld (b159c75)

1. The ownership guard had the race it was meant to close

A real hole in round 4's own fix, in both orderings:

• Check-then-act — stat and rmdir were separate async steps, so a waiter could replace the directory between them and we deleted the winner's fresh lock anyway.
• The exit window — skipping release() on a mismatch left the record in proper-lockfile's locks map, and onExit does for (const file in locks) { rmdirSync(...) } with no ownership check. An exit in that window deleted the winner too.

The guard moved into options.fs, the single seam both removal paths route through. It refuses to remove a directory whose inode and birth time are not the ones recorded at acquire, and reports the refusal as success so the library forgets the lock either way — leaving it registered is exactly what hands the exit handler a record pointing at the winner's directory. Removal of another holder's stale directory during acquireLock passes through untouched, since the identity is only recorded after we acquire.

It narrows the window; it does not close it — and that correction matters more than the code change. It is still check-then-act across processes, which needs the CAS Node does not expose. All four files now say "narrows", and AGENTS.md carries the negative instruction beside round 4's. Three of five rounds have now been spent on me overclaiming a guarantee, which is why the rule is written as a prohibition rather than as corrected prose.

2. The release-failure message promised expiry that never comes

Right, and reachable rather than hypothetical: stale takeover reclaims through the same rmdir, which also cannot remove a non-empty directory. So an ENOTEMPTY lock is cleared by nothing, every later save fails ELOCKED against it, and the old wording sent the operator away to wait for something that would never happen. releaseAdvice now branches — by-hand removal for ENOTEMPTY, expiry for everything else — and the test asserts the message says "by hand" and does not say "expires on its own", so collapsing the branch back to one string fails.

A test that fooled itself

Worth flagging since it is the kind of thing that would otherwise sit there looking green: the first draft of the new exit-handler test used a non-empty replacement directory. That makes rmdirSync fail ENOTEMPTY, so the winner survived for a reason unrelated to the guard and the test passed vacuously. It now uses an empty directory, and demonstrates the library's unguarded handler really does remove the replacement.

npm run ci passes clean.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

core/auth/node/file-lock.ts:418

• This fallback also receives failures from removing an existing stale lock, not only failures to create a lock. In proper-lockfile@4.1.2, a stale directory that cannot be removed (for example ENOTEMPTY) rejects with that non-ELOCKED error; if it is already stale, this first probe immediately runs fn() unlocked. Multiple Inspectors can then all bypass the same stuck lock and race their writes, contradicting the nearby claim that later saves keep failing. Distinguish lock-infrastructure failure from stale-lock cleanup failure and refuse the latter rather than degrading.

    warnOnce(
      `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`,
    );
    return fn();

core/auth/node/file-lock.ts:306

• The expiry advice is still false for persistent removal errors other than ENOTEMPTY. For example, EACCES, EPERM, or EROFS prevents the later stale-takeover path from performing the same rmdir, so waiting 10 seconds does not clear the directory. Give manual permission/removal guidance for persistent rmdir failures instead of promising expiry for every other error.

  return code === "ENOTEMPTY"
    ? `It has something inside it, which stale takeover cannot clear either, so saves will keep failing until you remove ${lockPathOf(target)} by hand.`
    : `It expires on its own after ${STALE_MS / 1000}s.`;

core/auth/node/file-lock.ts:422

• The identity is captured after the asynchronous acquire has completed. During the acknowledged stale-takeover race, another waiter can replace this process's newly created directory before this stat, causing this process to record the winner's identity as its own; the release guard then accepts and removes the winner's lock. Capture the identity at the successful lock-creation/acquisition seam before the lock is exposed, and cover replacement in this interval rather than only replacement inside fn.

  // Now that we hold it, record which directory is ours so the guard above
  // can refuse to delete anyone else's.
  owned.id = identifySync(lockPathOf(target));

[cliffhall]

Copilot review round 6 — all five addressed (09e1564)

Two inline, three suppressed. Flagging that split again since the suppressed ones included the most serious finding of the round.

1. A stale lock that could not be cleared caused unlocked writes (suppressed)

The sharpest one. acquireLock does not only create directories — on finding a stale one it removes it and retries, and that removal can fail: ENOTEMPTY for a lock with anything inside it, EACCES/EROFS for one we may not touch. Those arrive as ordinary non-ELOCKED errors, which my probe read as "locks do not work here" and degraded on. So every Inspector on the box quietly bypassed the same stuck lock and raced its writes — while the release-failure message added in round 5 was telling the operator saves would keep failing until they cleared it. Directly contradictory, and the degrade was the wrong half.

isStuckOrHeld now discriminates on whether the lock directory exists, not on an errno taxonomy: if it is there, something holds it → refuse; if not, we could not create one → degrade, which is the documented trade for the boxes this store exists for. Reading the state the decision is actually about beats enumerating error codes per platform — and an enumeration is precisely what let ENOTEMPTY through.

2. Identity captured after the acquire settled (suppressed)

Also right. That span covers the library's own utimes/stat probe, so a waiter replacing our directory inside it would have us record the winner's identity as our own — after which the guard would accept and delete their lock, the exact failure it was added to prevent. Now recorded in the mkdir callback, the moment the directory becomes ours and synchronous with respect to this process.

3. Expiry advice still wrong for EACCES/EPERM/EROFS (suppressed)

Right, and it turned out the branch was dead code: since the guard stats before removing, everything reaching the release catch is a refused rmdir, and stale takeover reclaims through that same call. So the ENOENT arm was unreachable too. Branch removed, advice unconditional.

4. proper-lockfile missing from NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE

A fourth externalization surface I had missed — the tsup lists configure the production bundles, not vite dev. Added, with a comment saying so explicitly. While extending vite-base-config.test.ts I found its assertion was also missing chokidar and @napi-rs/keyring, so it would not have caught either going missing; both added.

5. A test named for something it did not exercise

"does not treat an unlistable directory as an empty one" was driving ENOTDIR, which is deliberately treated as the fresh-install case — so the new behaviour was untested. Split into an honest ENOTDIR test and a real EACCES one (chmod 0o300: readdir denied, known name still reachable). Real mode rather than a stub, since vi.spyOn cannot redefine an ESM namespace export; skipped as root and on Windows where the mode would not bite.

Both new tests verified by mutation. npm run ci passes clean.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

[cliffhall]

Copilot review round 7 — both upheld (e956d49)

1. An in-progress migration snapshot could be adopted by another startup

Real, and I fixed it by the second suggestion rather than the first — worth showing the working, because the first one was worse.

Holding the main lock through the hand-off was tried and reverted. It closes the race, and it broke the existing test claims the file atomically, so a later write is not deleted: the concurrent set waits the full ~15s retry budget and then fails, because the migration holds the lock it needs. That regresses #1950's guarantee that a write completing after the claim survives, and it hits ordinary writers on every migration — whereas the race needs a second Inspector to start inside the hand-off window. The existing test caught it, not me.

So the hand-off locks the snapshot instead, and recoverOrphanedSnapshots skips an orphan whose lock is held — the suggested ownership/liveness metadata, with the lock itself as the metadata.

Deliberately not the pid already in the filename: a pid outlives its process and recurs (pid 1 on every container start — the documented reason the staged name carries a nonce in the first place), so a pid check would fail to release genuinely abandoned snapshots and, after reuse, wrongly protect them. A lock expires on its own, so an abandoned snapshot becomes adoptable with nothing to clean up.

2. A compromised lock reported ERELEASED — and we told the operator to delete the winner's lock

The dangerous one. Confirmed in the source: setLockAsCompromised sets released = true and drops the registry entry before calling onCompromised, and the release closure then short-circuits with ERELEASED without touching the filesystem. So after a takeover we printed "saves will keep failing until you remove <target>.lock by hand" — pointing at what is by then the winner's live lock. An operator following that destroys the exclusion of a process that did nothing wrong.

Now handled separately and silently, since onCompromised has already reported it. Written as a conditional, not an early return — a return inside finally discards whatever the body was returning or throwing, which no-unsafe-finally and a TS2322 both caught on the first attempt.

The compromise test now asserts the absence of "by hand" and "Could not release the lock", so reintroducing that advice on this path fails.

---

Both new tests verified by mutation. npm run ci passes clean.

A note on where this stands. Seven rounds, 20 findings. The rate is not tapering, and several rounds have found defects in the previous round's fix — including this one, where my own first attempt regressed a tested #1950 guarantee. The lock now works, and each individual fix is tested, but I would not read the round count as evidence of convergence; a human read of the final file-lock.ts and secret-store-selection.ts is worth more than another automated pass.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

[cliffhall]

Copilot review round 8 — both upheld (edddc6c)

Also merged v2/main into the branch (b35db18f, no conflicts) — notably picking up #2090's --max-warnings 0, which the gate passes cleanly across all five lint scopes.

1. The snapshot was unprotected between two awaits

A real gap in round 7's fix. withSecretFileLock releases on return, so the snapshot lock was taken after the main lock was already gone — a second startup could slip into that window, see the staged file unlocked, and adopt it. Narrower than the race round 7 closed; the same race.

openSecretFileLock is now split out: it takes the lock and returns its release, which is what allows holding one across another's release. The claim acquires the snapshot's lock before returning (inside the main lock), and the hand-off runs outside the main lock while still holding it — the ordering suggested, and it preserves what round 7 was protecting (no keychain round-trip under the writers' lock). withSecretFileLock is now a thin wrapper over the same function, so refuse-vs-degrade is decided once and the two entry points cannot drift.

2. Both scans matched the lock directories themselves

secrets.json.migrating-<pid>-<uuid>.lock passes the plain prefix test. The consequence is self-sustaining: the liveness probe asks about a nonexistent .lock.lock, answers "not held", recovery tries to hard-link a directory onto the secrets path, fails, and prints the orphan warning — every startup, forever, since a liveness check never clears a stale lock directory. isSnapshotName now gates both scans on the prefix and a .lock exclusion.

New test verified by mutation. npm run ci passes clean.

---

Where I think this actually stands

Eight rounds, 22 findings. Rounds 5, 6, 7 and 8 each found a defect in the previous round's fix — including this one. Every individual fix is tested and mutation-verified, and the lock does now do what the PR claims, but the round count is not evidence of convergence and I would not treat another clean automated pass as one either.

The recurring root cause is worth stating plainly for whoever reviews this: nearly every serious finding traces to reasoning about proper-lockfile from its shape rather than its source — that it closes the stale-takeover race, that it tells the loser, that a non-ELOCKED error means locking is unavailable, that release is the only path that removes a directory. Each was plausible and wrong, and each needed reading lib/lockfile.js to settle. AGENTS.md now carries those as prohibitions so the next person does not re-derive them.

A human read of the final core/auth/node/file-lock.ts and the migration path in secret-store-selection.ts is worth more here than another round.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

package.json:97

• The runtime dependency is declared with a caret even though this lock adapter explicitly depends on proper-lockfile@4.1.2 internals: both release paths routing through options.fs, registry removal before onCompromised, and the ELOCKED/ERELEASED behavior. A published install does not use this repository's lockfile to constrain transitive resolution, so it may load a future 4.x release whose semver-compatible public API no longer preserves those private lifecycle details, bypassing the ownership guard or changing failure handling. Pin proper-lockfile to exactly 4.1.2 (and update the lockfile) unless these assumptions are replaced by public API guarantees.

    "proper-lockfile": "^4.1.2",

clients/web/src/test/integration/auth/node/file-lock.test.ts:354

• This test does not enforce the retry-budget invariant described above it. The lock mtime is already 60 seconds old before set starts, so acquisition removes it immediately on the first attempt; the test still passes if RETRY is shortened below STALE_MS. Exercise a newly abandoned, unrefreshed lock from before it becomes stale (or directly assert the computed schedule exceeds the stale threshold) so a regression in that load-bearing relationship fails the suite.

    const target = filePath();
    const lockPath = `${target}.lock`;
    await fs.mkdir(path.dirname(target), { recursive: true });
    await fs.mkdir(lockPath);
    const longDead = new Date(Date.now() - 60_000);
    await fs.utimes(lockPath, longDead, longDead);

[cliffhall]

Round 9 — 4 of 5 are already fixed; 1 is real

Round 9 was requested just before this PR was called done, and it re-reports findings against an earlier state of the files. Verified each against the current head (edddc6c9) rather than assuming:

Finding	Status
secret-store-selection.ts:555 — releasing does not isolate the snapshot	Fixed in e956d498 / edddc6c9 — hand-off holds the snapshot's own lock, and recoverOrphanedSnapshots skips an orphan whose lock is held (isFileLockHeld(orphan))
file-lock.ts:505 — compromised lock arrives as ERELEASED	Fixed in e956d498 — handled separately and silently
secret-store-selection.ts:619 — snapshot unprotected between the two awaits	Fixed in edddc6c9 — openSecretFileLock(staged) is acquired inside the main lock, so it is held across its release
secret-store-selection.ts:783 — scans match the .lock directories	Fixed in edddc6c9 — isSnapshotName gates both scans on the prefix and a .lock exclusion
file-lock.ts:503 — by-hand advice assumes every rmdir failure is permanent	Open — and correct

The one that stands

proper-lockfile forwards any filesystem error from removal; transient failures can clear before stale takeover. Another Inspector may then legitimately acquire this path, and telling the operator to remove it unconditionally can delete that live holder's lock.

This is right, and it is the same class of hazard as round 7's ERELEASED finding — guidance that can destroy an innocent process's exclusion. Round 5 replaced a blanket "it expires on its own" with a blanket "remove it by hand", and both are wrong for the same reason: the failure that blocked our rmdir is not necessarily permanent.

It is a message-wording defect, not a behavioural one — the lock itself is correct, and nothing is destroyed by the Inspector. The fix is to make the guidance conditional (remove it only if saves keep failing and no Inspector is running) rather than an unconditional instruction.

Left open deliberately: the maintainer has called this PR done, and this is a one-line message change that does not affect locking behaviour. It should be a follow-up rather than a ninth round — flagged here so it is not lost.

[cliffhall]

Round 9's open finding is now fixed (31feed9)

The one that was real. Worth recording why it was, because the message has now been wrong in both directions:

• Round 5 replaced "it expires on its own after 10s" — false whenever the same rmdir that blocked us also blocks stale takeover, which reclaims through that identical call.
• With "remove it by hand" — false the other way. proper-lockfile forwards whatever the filesystem returned, so a transient failure can clear, another Inspector can legitimately acquire the path afterwards, and an operator following the advice deletes a live holder's lock.

That second failure is the same class as round 7's ERELEASED finding: guidance that destroys the exclusion of a process that did nothing wrong.

Nothing in this code can tell a permanent refusal from a passing one, so it no longer claims to. It now reports what happened, says the lock may or may not clear and why, and states the two conditions the operator can check themselves — saves still failing, and no other Inspector running — instead of asserting an outcome this code does not know. The test asserts both conditions are present and that the expiry promise is absent, so neither blanket version can return.

Behavioural only in what it tells a human; the locking path is unchanged. npm run ci clean.

---

No further review round requested — this PR is done from my side. Nine rounds, 23 findings, all addressed or verified already-fixed. The standing recommendation from my round 8 note holds: rounds 5–8 each found a defect in the previous round's fix, so a human read of core/auth/node/file-lock.ts and the migration path in secret-store-selection.ts is worth more than another automated pass.

[cliffhall]

Smoke test report — 9bc6ce1a on a clean worktree (macOS 24.6.0, Node 26.7.0)

Fresh git worktree at the PR head, full root npm install, then a mix of (a) the repo's own gates and (b) out-of-band tests driving the real FileSecretStore from real, independent OS processes — including an A/B against the same harness built against origin/v2/main, so every claim below is a measured difference rather than a reading of the diff.

Verdict: the PR does what it says. npm run ci and npm run pack:verify are both clean, the lock is demonstrably load-bearing under multi-process contention, and every refusal/recovery path behaves as documented. Three observations at the end, none of them blockers.

How the out-of-band harness was built (so the numbers below are reproducible)

// harness.ts — bundled twice with esbuild, once with --alias:@inspector/core=./core
// (PR) and once against a checkout of origin/v2/main (baseline). proper-lockfile,
// @napi-rs/keyring and atomically left external so the real root install is used.
import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js";
const store = new FileSecretStore({ filePath: file });
for (let i = 0; i < count; i++) await store.set(`srv-${prefix}`, `field${i}`, `value-${prefix}-${i}`);

Each writer is a separate node process, so FileSecretStore.serialize's in-process queue is out of the picture and the lock is the only thing arbitrating — which is the same reason the PR's own new tests spawn a child.

---

1. The headline: concurrent writers, PR vs. v2/main

N processes each write M distinct secrets to one file; the count of surviving keys is compared against N × M.

Scenario	Baseline v2/main	PR #2088
4 procs × 25 writes, run 1	56 of 100 lost	0 lost
4 procs × 25 writes, run 2	61 of 100 lost	0 lost
4 procs × 25 writes, run 3	61 of 100 lost	0 lost
8 procs × 50 writes, run 1	304 of 400 lost	0 lost
8 procs × 50 writes, run 2	302 of 400 lost	0 lost

=== BASELINE v2/main (no lock) — 4 processes x 25 writes each, 3 runs ===
  run1: expected=100 surviving=44 LOST=56 reported_errors=0
  run2: expected=100 surviving=39 LOST=61 reported_errors=0
  run3: expected=100 surviving=39 LOST=61 reported_errors=0
=== PR #2088 (proper-lockfile) — 4 processes x 25 writes each, 3 runs ===
  run1: expected=100 surviving=100 LOST=0 reported_errors=0
  run2: expected=100 surviving=100 LOST=0 reported_errors=0
  run3: expected=100 surviving=100 LOST=0 reported_errors=0

Note reported_errors=0 on the baseline rows: every one of those 56–61 lost secrets was reported to its caller as saved. That is precisely the failure #2082 describes, and the PR closes it — 5 runs, 1000 writes, zero losses.

Being fair to the baseline: at genuinely human pacing the optimistic verify does converge. 2 procs × 4 writes 100 ms apart with a randomised 0–90 ms start stagger lost 0 of 64 on both branches. The loss shows up when writers are in lockstep (two Inspectors launched together, both running the same startup migration — 24/48 lost on baseline, 0/48 on the PR) or doing bulk writes. So the PR is not fixing a hypothetical, but it is fixing the burst case rather than the idle case.

Uncontended cost is not measurable. 1 process × 50 writes: baseline 466/362/563 ms vs. PR 459/414/356 ms. The extra mkdir+rmdir disappears into scrypt.

2. Refuse-vs-degrade, against a real live holder

A second process takes the lock with proper-lockfile exactly as the Inspector does and holds it.

--- PR store attempts a write while the holder is ALIVE ---
{"ok":0,"errors":["SecretStoreUnavailableError: Could not save to the secrets file at
 /tmp/crash-FXEF/secrets.json: its lock (/tmp/crash-FXEF/secrets.json.lock) was still held
 after the 15 seconds this save waited. Its secrets are intact; the value you just entered
 was not saved. If no other Inspector is running, remove that lock and try again."],"ms":15306}
--- file after: (no secrets.json — nothing written behind the holder's back) ---

15.31 s measured against the documented ~15.3 s budget, and nothing was written. Same result for a stray regular file sitting at secrets.json.lock (15.29 s, refusal, file untouched) — isStuckOrHeld's "does the lock path exist" discriminator holds up outside the unit tests.

Crash recovery — the invariant that makes refusing defensible. kill -9 the holder, leaving its lock behind:

holder SIGKILLed; stale lock left behind: drwxr-xr-x  /tmp/crash-FXEF/secrets.json.lock
{"prefix":"survivor","ok":2,"errors":[],"ms":5291}     <- takeover, save succeeded
--- lock dir left behind? ---  (lock released and removed)

Waited 5.29 s (remaining stale window), took over, saved, cleaned up. RETRY_BUDGET_MS > STALE_MS is doing exactly the job the comment claims.

Fresh install (realpath: false + pre-lock mkdir). With the target at <tmp>/nested/deeper/secrets.json — no directory at all — and a holder on that path, the first save waited the full 15.31 s and then refused, rather than degrading to an unlocked write. That is the specific property the PR calls out, confirmed on a real filesystem.

Degrade path. chmod 555 on the storage directory:

[mcp-inspector] Could not take a lock on the secrets file at /tmp/ro-sBxO/secrets.json
 (EACCES: permission denied, mkdir '/tmp/ro-sBxO/secrets.json.lock'), so writes to it are
 not protected against another process writing at the same moment.

Warned once, body ran anyway (here it then failed on the unrelated read-only write). No retry budget burned — the probe-then-wait split works.

Reads take no lock: a get against a populated file created no .lock directory.

3. End-to-end through the real prod web backend

Built launcher, --web, MCP_INSPECTOR_SECRET_STORE=file, a temp catalog holding a stdio server with a plaintext env secret.

Step	Result
GET /api/servers, no lock held	plaintext migrated into secrets.json, stripped from mcp.json, no lock dir left behind
GET /api/servers, lock held by another process	HTTP 200 in 15.30 s, server alive, migration abandoned, plaintext preserved on disk (nothing lost), secrets.json untouched
GET /api/servers, lock held, nothing to migrate	HTTP 200 in 8 ms — no lock attempted
GET /api/servers after release	migrated in 23 ms
PUT /api/servers/:id with a new secret, lock held	HTTP 503 carrying the operator message verbatim; neither secrets.json nor mcp.json mutated

And it surfaces properly in the UI. Editing the server and saving while another process holds the lock — the modal stays open, the values are still there, the error is actionable:

Release the lock, save the same edit again — modal closes, and NEW_TOKEN lands in the file:

{ "version": 1, "encryption": "none",
  "secrets": { "locktest:env:API_KEY": "super-secret-value",
               "locktest:env:DB_PASSWORD": "another-secret",
               "locktest:env:NEW_TOKEN": "typed-by-user" } }

Zero pageerrors in either run. --web --dev (Vite) also boots clean and serves /api/servers from the file store — no browser-externalization warnings, so the vite-base-config.ts optimizeDeps exclusion is doing its job.

4. absorbFileSecretsIntoKeyring (driven against a fake keyring — no OS keychain touched)

Case	Result
Fresh install, nothing to migrate	{"absorbed":{},"warnings":[]} — no lock directory, no storage directory created at all. The lock-free fast path is real.
A real secrets.json	absorbed, file removed, one accurate warning
Lock held by another process	resolves (exit 0), does not throw — startup survives; file left intact; warned
Orphan snapshot whose lock is held	left alone, no warning
Same orphan once unlocked	adopted and absorbed
A stray secrets.json.migrating-1-x.lock directory	PR: silent. Baseline code given the same input: a bogus "Found secrets from an interrupted migration…" warning, repeated on every run forever.

That last row is worth calling out — the .lock exclusion in isSnapshotName isn't tidiness, it prevents a permanent false-migration loop that the introduction of locking would otherwise create. Reproduced both arms.

5. Build / packaging

• proper-lockfile is a bare import (external) in all three Node bundles — clients/cli/build/index.js, clients/tui/build/index.js, clients/web/build/index.js. Not inlined anywhere.
• Counter-proof that the tsup external entry is load-bearing: removed "proper-lockfile" from clients/cli/tsup.config.ts, rebuilt, ran the binary → Error: Dynamic require of "path" is not supported, exactly as the PR describes. Restored and rebuilt.
• npm run pack:verify → OK. The published tarball installs into a clean consumer and the real bin drives web/cli/tui end to end, so the root-only declaration resolves correctly for an npx user.
• Dependency footprint: proper-lockfile@4.1.2, single root install, transitive graceful-fs / retry / signal-exit, npm audit --omit=dev → 0 vulnerabilities.

6. Gates

npm run ci → exit 0, everything green: 5982 web tests, 304 cli, 315 tui, 5 launcher, 486 Storybook, every smoke including smoke:web:app. file-lock.ts coverage 97.4 / 95.45 / 100 / 98.52 (stmt/branch/func/line).

The mutation claim checks out, and is understated. With withSecretFileLock removed from mutate, the two touched suites go to 5 failures plus an unhandled ECOMPROMISED — the PR says 4. The new tests really do detect the lock's absence rather than passing on the verify-and-retry underneath.

---

Observations (non-blocking)

1. A stuck lock costs 15.3 s of startup, once. absorbFileSecretsIntoKeyring waits the full budget before giving up. Correct — it must not skip a migration on a maybe-transient lock — but the warning it prints nests set's message inside it, so a user who typed nothing is told "the value you just entered was not saved":

   Could not lock the secrets file at … to migrate it into the OS keychain (Could not save to the secrets file at …: its lock … the value you just entered was not saved. …), so it has been left in place.

   Cosmetic, and only on a path that already went wrong. Worth a sentence if you touch it again.

2. A stray regular file at secrets.json.lock never expires. It is correctly treated as "held" and refuses forever (a regular file can't go stale), and the message does say to remove it — so this is right, just noting that this one input has no self-healing path.

3. The 7.7 s on the read-only-directory case is pre-existing, not this PR — baseline measured 7.6 s on identical input (it's atomically's own retry, not the lock's).

[cliffhall]

Re-verified at 89b01d01 (the v2/main merge)

The report above was run against 9bc6ce1a. Confirming it still applies after the merge:

Nothing this PR owns changed.

$ git diff --stat 9bc6ce1a..89b01d01 -- core/auth clients/*/tsup*.ts \
    clients/web/server/vite-base-config.ts package.json package-lock.json \
    vitest.shared.mts clients/web/src/test/integration/auth
(empty)

The incoming commits are #2092's cleanups — the smoke-web-app helper extraction into scripts/lib/announced-child.mjs, a useServers test refactor, .npmignore and docs. The lock implementation, its tests, the three tsup externals and the dependency wiring are byte-identical to what was smoke-tested, so every finding stands as written.

Two things re-run rather than assumed:

• The A/B baseline is still the same code. v2/main moved 30113e9d → d459a08e, but git diff over core/auth between them is empty, so the comparison is unaffected.

• Headline concurrency, re-measured at 89b01d01 — 4 processes × 25 writes:

  === BASELINE v2/main @ d459a08e ===
    run1: expected=100 surviving=37 LOST=63 reported_errors=0
    run2: expected=100 surviving=33 LOST=67 reported_errors=0
  === PR head 89b01d01 ===
    run1: expected=100 surviving=100 LOST=0 reported_errors=0
    run2: expected=100 surviving=100 LOST=0 reported_errors=0
  

npm run ci on the merged head → exit 0, same totals as before (5982 web / 304 cli / 315 tui / 5 launcher / 486 Storybook), file-lock.ts still 97.4 / 95.45 / 100 / 98.52, and all seven smokes green including the refactored smoke:web:app and smoke:web:elicit.