The one genuinely new finding from Copilot round 9; the other four were
re-reports of rounds 7 and 8 against an earlier state of the files, verified
already fixed in e956d498 / edddc6c9.
This 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". That is 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 then deletes a *live* holder's lock. Same class
of hazard as the `ERELEASED` case in round 7: guidance that destroys the
exclusion of a process that did nothing wrong.
Nothing here can tell a permanent refusal from a passing one, so it no longer
claims to. It reports what happened, says the lock may or may not clear and
why, and states the two conditions the operator can check for themselves —
saves still failing, and no other Inspector running — rather than asserting
an outcome this code does not know.
Behavioural only in what it tells a human; the locking path is unchanged.
The test asserts both conditions are present and that the expiry promise is
absent, so neither blanket version can come back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 — two findings, both upheld. The first is a gap in
round 7's own fix.
**The snapshot was unprotected between two awaits.** Returning from
`withSecretFileLock(filePath, ...)` releases the main lock *before*
`handOffStagedSecrets` acquired the snapshot's, so a second startup could
take the main lock in that window, see the staged file unlocked, and adopt
and re-claim it — leaving this process reading a path that no longer exists.
Narrower than the race round 7 closed, but the same one.
`openSecretFileLock` is now split out of `withSecretFileLock`: it takes the
lock and returns its release, so a caller can hold one *across* another's
release. The claim acquires the snapshot's lock before returning — inside the
main lock — and the hand-off then runs outside the main lock, still holding
it. `withSecretFileLock` is a thin wrapper over the same function, so which
failures refuse and which degrade is decided in one place and the two entry
points cannot drift.
Locking the snapshot degrades rather than refusing (`null` → hand off
anyway): the exposure is one uniquely-named snapshot, which is a better trade
than refusing to migrate at all on a box that cannot lock.
**Both migration scans matched the lock directories themselves.**
`secrets.json.migrating-<pid>-<uuid>.lock` passes the plain prefix test, and
treating it as a snapshot is self-sustaining damage: the liveness probe asks
about a nonexistent `<name>.lock.lock` and so answers "not held", recovery
tries to hard-link a *directory* onto the secrets path, fails, and prints the
orphan warning. Because a liveness *check* never clears a stale lock
directory — only a would-be acquirer does — that false migration repeats on
every startup forever, including long after the real snapshot was recovered.
`isSnapshotName` now excludes `.lock` for both `anythingToMigrate` and
`recoverOrphanedSnapshots`.
New test verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7 — two findings, both upheld.
**An in-progress migration snapshot could be adopted by another startup.**
The fast path counts every `*.migrating-*` as work and
`recoverOrphanedSnapshots` adopts them — including one whose owner is still
reading it. It link/unlinks the snapshot back to the live path and re-claims
it under a new name, the owner's hand-off then fails `ENOENT`, and if the
adopting process exits before copying, that healthy session starts with none
of those secrets, recoverable only on some later run.
Fixed the second way the reviewer suggested, not the first. Holding the main
lock across the hand-off also closes it and was tried — it broke "claims the
file atomically, so a later write is not deleted", because it blocks every
ordinary writer for the whole migration and fails their save past the retry
budget. That is a worse regression than the race, since #1950 guarantees a
write completing after the claim survives. The existing test caught it.
So the hand-off takes a lock on the **snapshot** instead, and
`recoverOrphanedSnapshots` skips an orphan whose lock is held. Liveness comes
from the lock rather than from the pid already in the filename: a pid both
outlives its process and recurs — pid 1 on every container start, which
#1950 documents as the reason the name carries a nonce — whereas a lock
expires by itself if its owner dies, so a genuinely abandoned snapshot
becomes adoptable with nothing to clean up. `isFileLockHeld` answers `false`
when it cannot tell, since the alternative to a wrong `false` is never
recovering an abandoned file.
**A compromised lock arrives as `ERELEASED`, and we told the operator to
delete the lock.** `setLockAsCompromised` marks the lock released and drops
its registry entry *before* calling `onCompromised`, so `release()` answers
`ERELEASED` without touching the filesystem — and the directory at that path
is by then the **winner's live lock**. "Remove it by hand" would destroy the
exclusion of a process that did nothing wrong. Handled separately and
silently; `onCompromised` has already reported what happened. A conditional
rather than an early return, since a `return` inside `finally` discards
whatever the body was returning or throwing.
Both new tests verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6 — five findings (two inline, three suppressed), all
valid.
**A stale lock that cannot be cleared caused unlocked writes.** `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 surface as
ordinary non-`ELOCKED` errors, which the 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
was telling the operator saves would keep failing until they cleared it.
`isStuckOrHeld` now discriminates on **whether the lock directory exists**
rather than on an errno taxonomy: if it is there, something holds it and we
refuse; if it is not, we genuinely could not create one and degrading is the
documented trade (#1848, #1905). Reading the state the decision is about
avoids enumerating error codes per platform — and an enumeration is exactly
what let `ENOTEMPTY` through.
**Identity was captured after the acquire promise settled.** That span
includes 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 removal guard would accept and delete their lock. It is now
recorded in the `mkdir` callback — the moment the directory becomes ours, and
synchronous with respect to this process, so there is no window.
**The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the
guard `stat`s before removing, everything reaching the release catch is an
`rmdir` that was refused — and stale takeover reclaims through that same
`rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch
was dead code: removed, and the advice is now unconditional.
**`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A
fourth externalization surface — the tsup lists configure the production
bundles, not `vite dev`, whose dep scanner would otherwise walk its
CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts`
now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also
absent from that assertion.
**A test was 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 actual new behaviour
was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using
a `--x` directory — `readdir` denied, a known name still reachable, which is
the asymmetry that makes "unlistable" different from "empty". Driven with a
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 and the test would assert nothing.
Both new tests verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5. Both comments upheld; the first is a real hole in
round 4's own fix.
**The ownership guard was in the wrong place, and overclaimed.**
Round 4 checked ownership and then called `release()`. Two problems:
- Check-then-act. The `stat` and the `rmdir` were separate async steps, so a
waiter could replace the directory in between and we deleted the winner's
fresh lock anyway — the very thing the check was added to prevent.
- Skipping `release()` on a mismatch left proper-lockfile's record registered
in its `locks` map, and its `signal-exit` handler `rmdirSync`s every
registered lock with **no ownership check of its own**. An exit during that
window deleted the winner too.
The guard now lives in `options.fs`, which is the single seam *both* removal
paths route through — `removeLock` on release, and the exit handler. 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's
bookkeeping forgets the lock either way (leaving it registered is what hands
the exit handler a record pointing at the winner's directory).
The check is a `statSync` immediately followed by an `rmdirSync`, with no
`await` between them, so nothing in this process can interleave. **It still
does not close the race** — it is check-then-act across processes, which needs
the compare-and-swap Node does not expose — and saying it "closes the
destructive half outright" was wrong. file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say it narrows the
window rather than closing it. AGENTS.md carries the negative instruction
alongside round 4's, since this is the third round spent on an overclaim.
**"It expires on its own after 10s" is false for the ENOTEMPTY case** the new
test exercises. Stale takeover reclaims through the same `rmdir`, which also
cannot remove a non-empty directory — so nothing clears it, and every later
save fails ELOCKED against it. `releaseAdvice` now branches: by-hand removal
for ENOTEMPTY, expiry for everything else.
Tests: a real child process demonstrates the exit handler removing a
replacement lock (with an **empty** directory — a non-empty one makes
`rmdirSync` fail ENOTEMPTY, which is how the first draft of this test passed
for the wrong reason). The release-failure test now asserts the message says
"by hand" and does not promise expiry. The shim's `rmdirSync` arm carries a
justified `v8 ignore`: it is reachable only from a real process exit, and its
logic is `removeIfMine`, covered through the async arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 — four comments, all the same finding against the four
files carrying the claim, and it was more than a wording problem.
The comment said 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 the compromised holder goes on to delete the
*winner's* lock on the way out, ending the winner's exclusion too: one
compromised writer silently becomes two unprotected ones.
The second half is a correctness bug, not a documentation defect, and it is
fixable here even though the takeover race itself is not.
`withSecretFileLock` now records the lock directory's inode and birth time at
acquire and re-checks them before releasing. On a mismatch it **declines to
release** — leaving the new holder's lock alone costs nothing, ours is
already gone — and warns.
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, and 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
would be worse than releasing it.
Detection is therefore **best-effort**, and file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say so rather than
promising the loser is told. AGENTS.md carries the negative instruction too,
since this is the second round spent on the same overclaim.
Tests: "does not delete the winner's lock after being taken over" replaces the
lock inside the body and asserts the replacement survives with the *same*
inode and birth time — it was left alone, not deleted and recreated. It waits
for nothing, being the fast case the library's tick misses. Verified by
mutation: forcing `stillOurs` to always answer true fails it. A second test
covers a release that fails while the lock genuinely *is* ours (a stray file
makes `rmdir` fail ENOTEMPTY), so the ownership check cannot swallow a real
release failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
- **`absorbFileSecretsIntoKeyring` must never throw, and round 2 broke that.**
Making `withSecretFileLock` reject on a held lock was right for `set` — a
user is waiting on that value — and wrong here: this is awaited directly by
both `resolveSecretStore` branches, so a stuck writer elsewhere on the box
would have failed store resolution and with it the whole session. The lock
failure is now caught, warned, and the file left for the next run. Covered
by a test that holds the lock from another handle and asserts the hand-off
resolves, the file survives, and the lock-specific warning is emitted (it
asserts that message rather than the shared "left in place" tail, which the
pre-existing claim-failure path also prints and would have let it pass
without the lock ever being reached).
- **The refusal message overstated how long it waited.** `retries *
maxTimeout` reads 20s for a schedule that sums to 15.26s — the early
attempts are the exponential ramp, not the cap. Replaced with
`RETRY_BUDGET_MS`, computed from the schedule rather than written down, so
it cannot drift from the thing it describes. `retry` applies no jitter by
default, so it is exact rather than an estimate.
- **The PR description still described the pre-round-2 design** — the
single-winner claim and the ~3s backoff. Rewritten to match the code.
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` instead of re-narrowing, since `withSecretFileLock` rejects
only with the `SecretStoreUnavailableError` it constructs one call away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2, both comments upheld.
**ELOCKED must not degrade.** Exhausting the retries on a held lock means the
lock is *working* and something else demonstrably holds it — so running the
mutation anyway entered the exact interleaving the lock exists to prevent,
and entered it knowing a concurrent writer was there. `set` now fails
instead. Two supporting changes:
- The retry budget went from ~3.3s to ~15s so it outlasts the 10s stale
window. Refusing is only defensible because a *crashed* holder resolves by
takeover first; with the shorter budget, one Inspector killed mid-save
would have failed every later save on the box until someone deleted the
lock by hand. Pinned by a new test that stages a dead holder's lock
directly (mkdir + backdated mtime) and asserts the save succeeds.
- Acquire is now probe-then-wait. `proper-lockfile` drives its whole acquire
through `retry`, which re-attempts on *any* error — so with a 20-retry
budget a read-only `$HOME` would have spent the full 15s re-issuing an
identical failing `mkdir` on every save before degrading. One retry-less
probe separates "held" (worth waiting out) from "unavailable" (not).
**The comment overclaimed what proper-lockfile provides, and the reviewer was
right about the mechanism.** Verified against lib/lockfile.js@4.1.2: on
EEXIST it stats, and if stale it 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 both proceed. That is the same race the
hand-rolled version could not close, and it is not closable without
renameat2(RENAME_EXCHANGE).
What the library actually adds is that the loser is *detected*: its refresh
tick compares the lock's mtime against the value recorded at acquire, so a
compromised holder is told rather than proceeding silently. And what is
genuinely 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 serialized. The residual window opens only after a holder dies
without releasing, and the optimistic verify still covers it.
file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md all
carried the overclaim; all four now state the narrower, true version.
Also fixes 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.
Not done: a two-process stale-takeover race test. It would be testing the
library rather than this code, and is inherently nondeterministic; the honest
response to that finding was to stop claiming the race is closed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
- Create the storage directory before locking. `writeStoreFile` creates it,
but from inside the locked section — so on a fresh install, where
`~/.mcp-inspector` does not exist, proper-lockfile failed ENOENT and every
*first* save degraded to an unlocked write with a warning. That is the save
most likely to be racing another (two Inspectors started together both
reach it), so the lock was absent from precisely the interleaving it exists
to close.
- `anythingToMigrate` no longer reads every `readdir` failure as "nothing to
migrate". Only ENOENT/ENOTDIR proves the fresh-install case; a directory
that denies listing can still permit access to the known `secrets.json`,
and returning false there selected the keychain and left those secrets
invisible with nothing said. Other errors fall through to the under-lock
checks, which is what happened before the fast path existed.
- Pin `proper-lockfile` to the repo-root install in `vitest.shared.mts`,
beside express and yaml: it is reached only through root-owned `core/`.
Resolution already finds the root copy since no client declares it; the pin
is what stops that depending on it never arriving as some client's
transitive dependency, which would give a test two copies of a module whose
whole job is one registry of held locks.
- Make the cross-process test prove `mutate` takes the lock. The previous one
asserted only that both secrets survived, which the optimistic verify
delivers with the lock removed entirely. It now asserts the parent's `set`
has *not* settled while a child holds the lock — an observation only a real
lock produces. Verified by removing `withSecretFileLock` from `mutate`:
three tests fail, where none did before.
The degrade test's "lock cannot be created" case is now a path whose parent
is a *file* rather than a missing directory, since a missing directory is no
longer that case. It also fails identically for root, so it cannot pass
locally and flake in a container.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
#1950 shipped FileSecretStore without cross-process mutual exclusion, by
decision: an earlier revision hand-rolled a `mkdir` election with an owner
stamp, a heartbeat and a stale-takeover, and three consecutive review rounds
found a real race in it. The last one is not closable with what Node exposes
— claiming a stale lock atomically needs compare-and-swap on a directory
entry (`renameat2(RENAME_EXCHANGE)`) — so it was replaced with optimistic
verify-and-retry and the residual documented.
#2082 settles that as "borrow, don't hand-roll". `proper-lockfile` is what
npm itself locks with, and stale-takeover is precisely the problem it has
already solved: it re-stats the lock directory after claiming it and gives
the lock up when the mtime is not the one it wrote, so the loser of a
takeover race releases instead of proceeding.
- core/auth/node/file-lock.ts: `withSecretFileLock`. `realpath: false` so a
file can be locked into existence (the very first `set` has no
`secrets.json`, and the library's default resolves through `fs.realpath`);
a warning in place of the library's `onCompromised`, which throws from a
timer and would take the session down; and a degrade-with-one-warning path
rather than a throw when no lock can be taken — this store exists for boxes
missing the usual mechanism (#1848, #1905) and must not gain a new way to
be unavailable.
- FileSecretStore.mutate holds it across the whole read-modify-write. The
optimistic verify stays underneath and is not redundant: a lock is advisory
between the processes that take it, so the verify covers a writer outside
this codebase and covers the degrade path. The in-process queue stays too,
and gains a second job — proper-lockfile is not reentrant, so serializing
per path keeps ELOCKED meaning "another process".
- absorbFileSecretsIntoKeyring takes the same lock around orphan adoption and
the atomic claim, behind a lock-free `readdir` fast path so the common
startup (keychain available, no file ever written) neither creates a lock
directory nor warns about one it could not create.
proper-lockfile is a root `dependency` per the placement rule, and is named
in all three bundler `external` lists: tsup externalizes what the *client's*
manifest declares, so a root-only CJS package was being inlined into the ESM
bundles, leaving esbuild's `Dynamic require of "path" is not supported` shim
that killed `--cli` at import time. That rule was undocumented; it is now in
AGENTS.md beside the placement rule that creates it, and mirrored into
.github/copilot-instructions.md.
Tests: 7 new in file-lock.test.ts driving a real second process (the existing
suite structurally cannot — `serialize` orders in-process callers before the
lock sees them), plus two for the migration fast path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall