fix(server): refuse to start a second server against a live data directory#8442

_{P1 Badge} Reclaim stale locks without unlinking a successor

When two servers concurrently start after a crash left a stale lock, both can read the stale holder, then the first can remove it and create its own lock before the second reaches this unconditional removal. The second process then unlinks the first process's newly acquired lock and succeeds on its next wx attempt, leaving both servers running against the same state directory. Reclamation must conditionally remove the exact stale object or use a locking primitive/protocol that cannot delete a successor's claim.

Useful? React with 👍 / 👎.

Right, and the interleaving is real: starter A reads a dead holder, starter B removes and recreates the file, then A's unconditional remove unlinks B's live claim. Fixed in 5a3f75679.

Reclaim no longer unlinks after one stale read. This effect's FileSystem surface has no conditional unlink, so instead a candidate dead lock is mtime-refreshed and re-observed across MAX_CLAIM_ATTEMPTS rounds, and only a file that stayed untouched through every round is removed. The holder side is what makes that sound: every live holder runs a heartbeat that refreshes the lock's mtime inside one observation round, so a live recreation can never sit still long enough to be reclaimed, and a dead holder still recycles its lock within a second of startup. Releases also no longer remove anything they cannot decode as their own.

Tests that fail before and pass after: reclaims a stale lock without unlinking a concurrently live successor and a starter cannot reclaim a lock a live holder still owns in serverSingleton.test.ts. Suite and typecheck numbers on the sibling thread.

_{P1 Badge} Check legacy runtime state before claiming the lock

When this release starts while any pre-lock T3 version is still running against the same data directory—the desktop auto-update scenario described by this change—there is no server.lock, so claimLock succeeds and both servers continue writing shared state. The previous version already persists its live PID in server-runtime.json via serverRuntimeState.ts, so the first lock-enabled release needs to reject a live legacy runtime state before creating the new lock; otherwise the rollout does not fix the incident until every running server has already upgraded once.

Useful? React with 👍 / 👎.

_{P1 Badge} Update the lock metadata atomically

When another server attempts startup while the holder records its bound port, this ordinary writeFileString opens the lock with truncation before rewriting the JSON. A contender can fail its exclusive create during that window, decode the empty or partial file as an undefined holder, remove it as stale, and acquire the directory while the original server remains live. Write the metadata through an atomic replacement so readers never mistake an in-progress update for a crashed owner.

Useful? React with 👍 / 👎.

Right — this is the same root as the macroscope finding on this line, and it is the one that re-opens the exact bug the PR exists to close. Fixed in 5a3f75679 by writing the port through the repo's existing write-temp-then-rename helper (atomicWrite.ts) instead of truncating in place. A reader now always sees either the pre-update holder or the post-update holder, which is what makes the multi-round reclaim in the sibling fix safe to believe.

Test: a partial metadata update is never readable as an empty owner in serverSingleton.test.ts asserts both that the updated lock still refuses as live with the recorded port, and that the update's on-disk shape is the atomic one — none of the temp staging the rename uses is left behind, and an in-place truncate cannot produce that shape. Full apps/server suite: 246 files passed / 2 skipped, 2829 tests passed / 10 skipped. pnpm run typecheck exit 0.

🟠 High src/serverSingleton.ts:183

recordServerLockPort can cause two live servers to own the same data directory: its in-place rewrite temporarily leaves lockPath empty or partially written, so a concurrent claimLock treats the live lock as stale, removes it, and acquires a replacement lock. Update the holder atomically (for example, write a temporary file and rename it) so readers never observe an incomplete lock.

✅ Resolved in 5a3f7567992a9a2a64688fad1cb33b12d9245492

Taken seriously — the in-place rewrite did temporarily leave the lock empty or partial mid-update, and a concurrent claimLock in that window decoded an empty holder, treated the live lock as crashed, removed it, and acquired a replacement: two live servers against one data directory, the exact corruption this PR exists to prevent. Fixed in 5a3f75679 with write-temp-then-rename via the repo's atomicWrite.ts, so readers never observe an incomplete lock.

The two "also found" locations are the call sites of this exact code path (server.ts:533 calls recordServerLockPort; server.ts:713 provides the layer that acquires the lock), so both are fixed by the same change rather than edited independently.

Test: a partial metadata update is never readable as an empty owner in serverSingleton.test.ts — the updated lock still refuses as live with its recorded port, and the update leaves no temp-staging shape an in-place truncate could produce. Full apps/server suite: 246 files passed / 2 skipped, 2829 tests passed / 10 skipped. pnpm run typecheck exit 0.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High src/serverSingleton.ts:153

A stale-lock reclaim can delete a new lock owned by another starter, so both servers proceed from claimLock while only one lock file remains. After A reads a dead holder, B can remove and recreate the file, then A's unconditional fs.remove(input.lockPath) removes B's lock before A creates its own. Reclaim only the same file instance that was inspected (or otherwise make stale-lock removal conditional) before retrying.

No longer relevant as of a678dfcec487cad4e0d1995cf67d16a3dbd918d5

Right, same finding as the P1 thread above and fixed the same way in 5a3f75679: reclaim is no longer an unconditional unlink after one read. A candidate dead lock is mtime-refreshed and re-observed across several rounds before it is removed, and every live holder's heartbeat refreshes the file inside one round, so the file a reclaimer finally removes is provably still a dead one — a successor that won the re-race in the gap always shows its heartbeat and is never evicted.

Test: reclaims a stale lock without unlinking a concurrently live successor in serverSingleton.test.ts (red against the pre-fix unlink, green after). Suite and typecheck numbers on the P1 thread above.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

src/serverSingleton.ts:89

reason is a single fixed prose string (the only construction site passes "the lock was repeatedly reclaimed by another starting server") and the message is built from it, so the message text is stored as data next to an already-specific error tag, and no cause is available when the failure came from a real filesystem error. Consider modelling the exhaustion structurally and keeping an optional cause, deriving message from those attributes.

The construction site then becomes new ServerLockUnavailableError({ lockPath: input.lockPath, attempts: 2 }).

No longer relevant as of 5a3f7567992a9a2a64688fad1cb33b12d9245492

Done in 5a3f75679 — the error now models the outcome structurally, { lockPath, attempts, cause? }, and derives its message from those attributes, exactly the shape suggested. The count is the observation rounds the starter was forced through before the lock provably stayed dead, which reads more usefully than a fixed prose sentence when it does surface.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

src/serverSingleton.ts:137

Effect.orElseSucceed(() => false) erases the underlying PlatformError, so a non-AlreadyExists failure (permission denied, read-only or missing parent, out of disk) is silently reinterpreted as "the lock is taken" and surfaces as ServerAlreadyRunningError/ServerLockUnavailableError with no cause and a message that names the wrong problem. Consider treating only the structural AlreadyExists reason as "not created" and letting other platform errors propagate (typed) as packages/shared/src/relayClient.ts does for its install lock.

No longer relevant as of 5a3f7567992a9a2a64688fad1cb33b12d9245492

Right — Effect.orElseSucceed(() => false) turned a permission-denied or read-only-parent into "the lock is taken", and the user would have been told to stop a server that does not exist. Fixed in 5a3f75679 with the relayClient.ts shape: only the structural AlreadyExists reason maps to "not created", and every other platform error propagates as itself with its cause intact.

Live lock stolen while file unreadable

Medium Severity

claimLock reclaims whenever the lock file is missing or not valid JSON, without proving the owner is gone. Exclusive create and recordServerLockPort's truncating overwrite both leave the file empty or partial while the holder is still alive, so a second process can delete it and start. releaseServerLock also removes a lock it cannot decode, which can evict a successor in that same window. Two live servers then share state.sqlite and settings.json.

^{Reviewed by Cursor Bugbot for commit 7a006bdbdbfca579e35c005f2a183d728e73a1a2. Configure here.}

src/serverSingleton.ts:365

Known tagged failures should be recovered with Effect.catchTags({ ... }), including when only one tag is handled; Effect.catchTag is not used in this codebase's convention. Consider switching to the object form.

No longer relevant as of d9c354df66a76c225d94ae8d47be013911f6ee03

Done in d9c354df6 — switched the single handler to the object Effect.catchTags({ LiveLegacyServerRuntime: ... }) form to match the codebase convention. Behaviour unchanged; the module had been the only catchTag in the touched set.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High src/serverSingleton.ts:233

After reclaiming a confirmed stale lock, claimLock exits with ServerLockUnavailableError instead of retrying wx, so the first restart after a crash fails even when no competing starter exists. A shutdown race also reaches fs.utimes after readHolder observes the lock as missing, causing a filesystem error. Retry the exclusive create after successful removal, and restart the loop when the candidate disappears before utimes.

✅ Resolved in 99c8417efbf4707c379c07d6484a76aba2436fd7

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Lock heartbeat dies on first error

Medium Severity

holdServerLock places Effect.catch outside Effect.repeat, so the first failed readHolder or utimes stops the heartbeat for the rest of the process. The comment says a spaced repeat should skip a tick and keep refreshing through transient filesystem errors; Effect.repeat ends on failure, so that recovery never happens.

^{Reviewed by Cursor Bugbot for commit d9c354df66a76c225d94ae8d47be013911f6ee03. Configure here.}

Right — the recovery lived outside the repeat, and Effect.repeat terminates a failing effect, so the heartbeat died with the first transient readHolder or utimes error and its live lock could then be reclaimed out from under it. The comment even said recovery never happens; the code now matches. Fixed in a678dfcec: each round carries its own catch, and the repeat wraps the recovered round. serverSingleton suite 14/14, typecheck exit 0.

Vanished lock makes reclaim fail startup

Medium Severity

After another starter removes a stale server.lock, this starter still utimes that path. Unlike readHolder and the later remove({ force: true }), utimes does not treat NotFound as “file is gone, try wx again,” so acquire fails with a raw filesystem error while the directory is actually free.

^{Reviewed by Cursor Bugbot for commit d9c354df66a76c225d94ae8d47be013911f6ee03. Configure here.}

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Make ServerSingletonLive a dependency of the persistence layers.

RuntimeDependenciesLive provides ServerSettingsLayerLive and PersistenceLayerLive, which both reach SqlitePersistenceLayerLive. That layer constructs NodeSqlite.DatabaseSync(dbPath) and runs migrations. ServerSingletonLive is provided only inside HttpServerLive, so the branches have no structural ordering. Effect may initialize SQLite before acquireServerSingleton rejects the second process. Provide ServerSingletonLive above all state-directory consumers.

✅ Addressed in commit 455dcf4

Right — ServerSingletonLive was provided only into HttpServerLive, so RuntimeDependenciesLive (settings + SQLite + migrations) could initialize in parallel and open state.sqlite before a second process refused.

The same ServerSingletonLive layer is now also Layer.provided into RuntimeDependenciesLive. Effect memoizes by identity, so there is still one acquire, and both the HTTP bind and persistence wait on it.

Fixed in 455dcf4b2.

@NoahLinckeScout, verified. RuntimeDependenciesLive now provides ServerSingletonLive, and HttpServerLive receives the same layer identity. This makes lock acquisition a prerequisite for both SQLite initialization and HTTP binding.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Use an ownership-safe stale-lock takeover.

Two starters can both complete the final stale-holder check. The first starter can remove the stale lock and create its lock before the second starter executes fs.remove. The second starter then removes the first starter's live lock and can claim the path itself.

The heartbeat cannot prevent this sequence because the second starter does not read the lock again before removal. Both servers can report successful acquisition and access the same data directory.

Use an atomic ownership primitive or serialize stale takeover. Add a test that pauses two reclaimers after their final stale read and resumes both removals.

✅ Addressed in commit 2feefe6

Right — after the last stale read, fs.remove was still unconditional, so two reclaimers could both pass that check and the second could delete the first's fresh claim.

Takeover is now serialized with an exclusive server.lock.reclaim gate. Only the winner re-reads, then removes and recreates; a live holder discovered after taking the gate is refused. The gate is released on the way out, and a leftover gate whose pid is dead is itself reclaimed.

Test that fails before and passes after: only one of two concurrent reclaimers acquires a stale directory in serverSingleton.test.ts. Suite: 15/15.

Fixed in 455dcf4b2.

@NoahLinckeScout, the new test covers a fresh reclaim gate. It does not cover stale reclaim-gate takeover.

takeReclaimGate still has a check-then-remove race at Line 172 through Line 180:

1. Two starters read the same dead server.lock.reclaim.
2. The first starter removes it and creates a new live gate.
3. The second starter then executes fs.remove(gatePath, { force: true }).
4. The second starter removes the first starter’s live gate and creates its own gate.

Both starters can then enter the protected takeover section. The original server.lock race remains possible.

Make stale reclaim-gate takeover ownership-safe as well. Add a test that starts with a dead server.lock.reclaim and forces two reclaimers through this interleaving.

You are interacting with an AI system.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Record the port immediately after the socket binds.

runtimeStateLayer waits for awaitActivation before this update. During that interval, the server is listening but a second launch receives an error without the listening port.

Move the update into httpListeningLayer after it acquires HttpServer.HttpServer.

✅ Addressed in commit 455dcf4

Right — recordServerLockPort sat after awaitActivation in runtimeStateLayer, so a second start during that window got a refusal with no listening port.

Moved the stamp into httpListeningLayer immediately after HttpServer.HttpServer is acquired, so the lock names the port as soon as the socket binds. Runtime-state persistence is unchanged.

Fixed in 455dcf4b2.

@NoahLinckeScout, confirmed. httpListeningLayer now records the bound port before awaitActivation can delay startup. runtimeStateLayer keeps only runtime-state persistence. Thank you.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Make stale reclaim-gate cleanup ownership-safe.

takeReclaimGate checks the gate PID, removes ${lockPath}.reclaim by pathname, and then creates it with wx. A crash can leave a dead gate behind before the Effect.ensuring cleanup runs. Two reclaimers can therefore both read the dead gate, remove each other’s gate, and return true.

The post-gate lock re-read does not close this race. One reclaimer can read the dead server.lock, while the other creates a live lock. The first can then remove that live lock by pathname and create its own lock. Both claimLock calls can return successfully, allowing two servers to use the same state directory and overwrite shared state.

Use a locking primitive that atomically validates gate ownership during deletion. Add a concurrency test that seeds a dead gate and pauses both reclaimers after they read it.

✅ Addressed in commit 2feefe6

Right — takeReclaimGate had the same unlink-by-name race we just closed on server.lock. Two reclaimers that both saw a crashed gate could each remove then wx, and the second deleted the first's fresh gate.

A dead gate is now renamed off the path (server.lock.reclaim.dead.<pid>) instead of unlinked. Only one rename of that inode can succeed; the loser backs off. The sidecar is ours to delete.

Test: only one reclaimer wins when a dead reclaim gate is left behind in serverSingleton.test.ts.

Fixed in 2feefe653.

@NoahLinckeScout, verified.

takeReclaimGate now renames a dead reclaim gate before it deletes the sidecar. Only one reclaimer can rename the original inode. A reclaimer that loses the rename receives NotFound and backs off.

The new test seeds a dead reclaim gate, starts two concurrent acquisition attempts, and asserts one success and one failure. This addresses the reported stale reclaim-gate cleanup race.

🐇 ✓

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Add an elapsed-time grace period before reclaiming an undecodable lock.

claimLock uses Effect.yieldNow between its three observation rounds. RECLAIM_OBSERVATION_DELAY_MILLIS applies only to the holder heartbeat, so the reclaimer has no minimum lock age. The code also does not compare mtimes.

If the creator is paused after fs.writeFileString(..., { flag: "wx" }) creates the path, readHolder returns undefined. The post-gate reread helps only if the write completes before that read. If the write completes after the reread, fs.remove can unlink the path while the creator's write is still active. claimLock then returns success to that creator, and acquireServerSingleton continues to holdServerLock; a competing starter can create the replacement lock and run concurrently.

Require a real minimum age for undecodable locks, then revalidate the same file identity before removal.

✅ Addressed in commit 2feefe6

Right — observation rounds are Effect.yieldNow, not elapsed time, so a creator paused after the exclusive create (empty or half-written lock) could look like a crash fragment.

Undecodable locks are no longer mtime-refreshed (that would hide the age). Takeover now requires the lock file's mtime to be at least one heartbeat interval old, checked again immediately before unlink. The existing half-written-crash test already ages the file and still passes.

Fixed in 2feefe653. vp test run src/serverSingleton.test.ts → 16 passed.

🟠 High src/serverSingleton.ts:278

A live lock can be reclaimed when the servers share --base-dir across separate Linux PID namespaces. processIsAlive(holder.pid) calls process.kill using the second container's PID namespace, so it returns ESRCH for the holder and the code removes its lock before opening the same SQLite and settings files. The lock must use a namespace-independent ownership check or a kernel lock primitive to preserve exclusion across containers.

This is the documented tradeoff, not an accidental hole.

The file header already rejects flock because Node has no binding and a native dependency for one lock is worse than handling staleness here. Liveness is signal 0 in the caller's PID namespace. Two containers that share --base-dir and not a PID namespace can therefore see ESRCH for a live holder and reclaim — same class of limitation as an unmodified pre-lock binary, which this PR also does not try to exclude.

I am not switching the primitive on this PR. Cross-namespace exclusion needs a kernel lock or a namespace-independent owner check; that is a follow-up, not a rewrite of an already-reviewed pid-file lock.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.