Repository navigation
FileSecretStore has no cross-process mutual exclusion: decide whether to adopt an OS-backed lock #2082
Description
Problem
FileSecretStore (added in #1950) has no cross-process mutual exclusion, by decision. Within a process, mutations are serialized per resolved file path. Across processes they are optimistic: read the map, apply, write, read back, compare the whole map, and re-apply onto a concurrent writer's result if they differ.
That converges in every interleaving where the clobber lands before the verify — which is most of them — but it is not mutual exclusion, and the residual is real:
process A process B
--------- ---------
read M0
read M0
write MA
verify -> MA ok
write MB <- clobbers A's entry
verify -> MB ok
Both set calls resolve successfully and A's secret is gone. Nothing detects it afterwards.
The same shape affects absorbFileSecretsIntoKeyring: it re-reads the file immediately before deleting it and declines when the contents moved, but a write landing between that read and the rm is still lost.
Why it is like this
An earlier revision of #1950 did take a lock — a mkdir election with an owner stamp, a heartbeat and a stale-takeover. Copilot found a real race in it in three consecutive review rounds, and 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)). Without it, a waiter that loses the race can move the winner's fresh lock aside and enter alongside it.
So the choice was not "lock versus no lock" — it was "a lock that loses updates across a wider set of interleavings, in ~220 lines that three rounds failed to make correct" versus "a narrower window, no extra dependency, and a failure that mostly self-repairs". The maintainer chose the latter for #1950 and the residual is documented in core/auth/node/file-secret-store.ts, README.md and specification/v2_servers_file.md rather than hidden.
What to decide here
Whether the residual is worth closing, and if so how. The options weighed during #1950:
| Option | Cost | Leaves |
|---|---|---|
Adopt proper-lockfile (or similar) |
New root runtime dependency; it is what npm itself uses | Very little — stale-takeover is precisely the problem it has already solved |
| Keep optimistic, add a generation counter | Small | Narrows the window; does not close it without atomic CAS |
| Per-secret files, hashed names | Larger redesign | Regresses a security property: the file set leaks how many secrets exist and stable hashes let someone confirm a guessed account name, which is exactly what encrypting the map as one envelope prevents |
| Do nothing | None | The status quo, documented |
proper-lockfile is the obvious candidate if we act. Note the repo rule: core/ imports it at runtime, so it belongs in root dependencies and ships to consumers.
How to reproduce
Not reproducible by the existing suite on purpose — serialize is one process-wide queue per path, so in-process tests cannot produce the ordering. It needs two real processes writing the same secrets.json within the window between one's write and its read-back. A test would spawn two node -e children against a shared temp file with a barrier.
Notes
Filed as the follow-up carved out of #1950 review rounds 10 and 11, where the reviewer twice held that documenting the race does not satisfy set's persistence contract. That position is reasonable; this issue is where it gets decided rather than re-argued per round.
Activity
fix(auth): address Copilot review round 12
fix(auth): address Copilot review round 13
fix(auth): address Copilot review round 14
fix(auth): order /api/config loads so an older response cannot win
fix(auth): address Copilot review round 16
fix(auth): address Copilot review round 17
fix(auth): commit the utility round 17 wired in, and drop a prohibite…
fix(auth): address Copilot review round 1 on the secrets-file lock (#…
fix(auth): stop degrading on ELOCKED, and correct the claim about the…
fix(auth): keep the keychain hand-off non-throwing, and stop overstat…
fix(auth): stop deleting the winner's lock after a stale takeover (#2082
fix(auth): guard every lock removal, not just release, and stop promi…
fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (…
fix(auth): lock the snapshot during hand-off, keep quiet on ERELEASED (…
fix(auth): hold the snapshot lock across the main lock's release (#2082)
fix(auth): make the stuck-lock guidance conditional, not an instructi…
docs: drop the Claude Desktop import from the out-of-scope list (#2082)
docs: normalize emphasis markers in v2_servers_file.md (#2082)
feat(auth): give the secrets file a real cross-process lock (modelcon…
fix(proxy): route through undici's own fetch and keep undici external
chore: address Copilot review round 13