From 2d53b8a61ecec05aa7d8d206f6fae424a5d50a8b Mon Sep 17 00:00:00 2001
From: Kaushik Samadder <kaushiksamadder2003@gmail.com>
Date: Sat, 15 Aug 2026 18:06:04 +0530
Subject: [PATCH] fix(site-memory): lock site memory writes across processes

writeChains only guards updateText/updateJson within one process, so two
webcmd processes writing to the same site interleave their read-modify-write
and the loser's write is silently dropped (exit 0, no error).

Add a cross-process lock (open(path, 'wx') on a sibling *.lock file, retry
with backoff, break locks left by a dead owner or older than 10s) and use it
to guard the read as well as the write in updateText/updateJson. writeSiteFile
is left on the in-process lock alone since it overwrites a full body rather
than modifying one.

Lock files are hidden from site memory readers the same way temp write files
already are.

Reproduced the issue's 20-concurrent-process repro against a real build
(was 17-19/20 surviving, now 20/20 for both notes.md and endpoints.json).
Added cross-process tests that spawn real child processes; verified they
fail on the old code and pass on the new one.
---
 .../__fixtures__/concurrent-writer.mts        |  20 ++
 src/site-memory/file-lock.test.ts             |  97 ++++++++++
 src/site-memory/file-lock.ts                  | 178 ++++++++++++++++++
 src/site-memory/local-store.test.ts           |  62 +++++-
 src/site-memory/local-store.ts                |  25 ++-
 5 files changed, 375 insertions(+), 7 deletions(-)
 create mode 100644 src/site-memory/__fixtures__/concurrent-writer.mts
 create mode 100644 src/site-memory/file-lock.test.ts
 create mode 100644 src/site-memory/file-lock.ts

diff --git a/src/site-memory/__fixtures__/concurrent-writer.mts b/src/site-memory/__fixtures__/concurrent-writer.mts
new file mode 100644
index 00000000..6a5d0639
--- /dev/null
+++ b/src/site-memory/__fixtures__/concurrent-writer.mts
@@ -0,0 +1,20 @@
+/**
+ * Child process for the cross-process site memory tests. Run through tsx:
+ *
+ *   node <tsx-cli> concurrent-writer.mts <homeDir> <site> note|endpoint <label> <count>
+ *
+ * Each run writes `count` times so several of these racing against one another
+ * interleave inside the read-modify-write window rather than only at startup.
+ */
+import { appendNote, setEndpoint } from '../local-store.js';
+
+const [homeDir, site, mode, label, count] = process.argv.slice(2);
+
+for (let index = 0; index < Number(count); index += 1) {
+  const name = `${mode}-${label}-${index}`;
+  if (mode === 'endpoint') {
+    await setEndpoint({ site, homeDir, name, url: `https://${site}/api/${name}`, method: 'GET' });
+  } else {
+    await appendNote({ site, homeDir, text: name });
+  }
+}
diff --git a/src/site-memory/file-lock.test.ts b/src/site-memory/file-lock.test.ts
new file mode 100644
index 00000000..d323eab2
--- /dev/null
+++ b/src/site-memory/file-lock.test.ts
@@ -0,0 +1,97 @@
+import { spawnSync } from 'node:child_process';
+import { mkdtemp, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises';
+import { hostname, tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { afterEach, describe, expect, it } from 'vitest';
+import { lockPathFor, withFileLock } from './file-lock.js';
+
+const tempDirs: string[] = [];
+
+afterEach(async () => {
+  await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true })));
+});
+
+describe('site memory file lock', () => {
+  it('runs overlapping critical sections one at a time', async () => {
+    const target = await tempTarget();
+    let inside = 0;
+    let overlapped = false;
+
+    await Promise.all([1, 2, 3, 4].map(() => withFileLock(target, async () => {
+      inside += 1;
+      if (inside > 1) overlapped = true;
+      await new Promise((resolve) => { setTimeout(resolve, 5); });
+      inside -= 1;
+    })));
+
+    expect(overlapped).toBe(false);
+    await expect(exists(lockPathFor(target))).resolves.toBe(false);
+  });
+
+  it('releases the lock when the critical section throws', async () => {
+    const target = await tempTarget();
+
+    await expect(withFileLock(target, () => Promise.reject(new Error('boom')))).rejects.toThrow('boom');
+
+    await expect(exists(lockPathFor(target))).resolves.toBe(false);
+  });
+
+  it('breaks a lock left behind by a process that is gone', async () => {
+    const target = await tempTarget();
+    await writeFile(lockPathFor(target), `${JSON.stringify({ pid: deadPid(), host: hostname(), token: 'stale' })}\n`);
+
+    await expect(withFileLock(target, async () => 'written', { staleMs: 60_000, timeoutMs: 1_000 }))
+      .resolves.toBe('written');
+  });
+
+  it('breaks a lock older than the stale window even when the owner is unknown', async () => {
+    const target = await tempTarget();
+    const lockPath = lockPathFor(target);
+    await writeFile(lockPath, 'not json\n');
+    const past = new Date(Date.now() - 60_000);
+    await utimes(lockPath, past, past);
+
+    await expect(withFileLock(target, async () => 'written', { staleMs: 10_000, timeoutMs: 1_000 }))
+      .resolves.toBe('written');
+  });
+
+  it('reports a live holder as SITE_MEMORY_BUSY instead of writing anyway', async () => {
+    const target = await tempTarget();
+    await writeFile(lockPathFor(target), `${JSON.stringify({ pid: process.pid, host: hostname(), token: 'held' })}\n`);
+    let ran = false;
+
+    await expect(withFileLock(target, async () => { ran = true; }, { staleMs: 60_000, timeoutMs: 50 }))
+      .rejects.toMatchObject({ code: 'SITE_MEMORY_BUSY', exitCode: 75 });
+
+    expect(ran).toBe(false);
+    await expect(readFile(lockPathFor(target), 'utf8')).resolves.toContain('held');
+  });
+
+  it('does not delete a lock that was broken and taken over by someone else', async () => {
+    const target = await tempTarget();
+    const lockPath = lockPathFor(target);
+
+    await withFileLock(target, async () => {
+      await writeFile(lockPath, `${JSON.stringify({ pid: process.pid, host: hostname(), token: 'other' })}\n`);
+    });
+
+    await expect(readFile(lockPath, 'utf8')).resolves.toContain('other');
+  });
+});
+
+async function tempTarget() {
+  const dir = await mkdtemp(join(tmpdir(), 'webcmd-file-lock-'));
+  tempDirs.push(dir);
+  return join(dir, 'notes.md');
+}
+
+/** A pid that has certainly exited: spawnSync reaps the child before returning. */
+function deadPid(): number {
+  const child = spawnSync(process.execPath, ['-e', '']);
+  if (typeof child.pid !== 'number') throw new Error('could not spawn a child to retire');
+  return child.pid;
+}
+
+async function exists(path: string) {
+  return stat(path).then(() => true, () => false);
+}
diff --git a/src/site-memory/file-lock.ts b/src/site-memory/file-lock.ts
new file mode 100644
index 00000000..5e1761f4
--- /dev/null
+++ b/src/site-memory/file-lock.ts
@@ -0,0 +1,178 @@
+/**
+ * Cross-process advisory lock for site memory read-modify-write updates.
+ *
+ * `atomicWrite()` keeps a single write from tearing, but site memory updates
+ * read the current file, modify it, and write it back. Two webcmd processes
+ * running that sequence against one file both read the same body and the second
+ * rename discards the first process's work. The in-process promise chain in
+ * local-store.ts cannot see the other process, so the critical section needs a
+ * marker the filesystem can see: `open(..., 'wx')` creates the lock file only
+ * when it does not already exist, atomically, on every platform we support.
+ *
+ * Abandoned locks never wedge site memory. A lock whose owner process is gone
+ * is broken on the next attempt, and any lock older than `staleMs` is broken
+ * regardless — the critical section itself is a small read plus a rename, which
+ * takes milliseconds. `timeoutMs` is deliberately longer than `staleMs` so an
+ * abandoned lock is always broken rather than surfaced to the user as an error.
+ */
+import { randomUUID } from 'node:crypto';
+import { open, readFile, stat, unlink } from 'node:fs/promises';
+import { hostname } from 'node:os';
+import { basename } from 'node:path';
+import { CliError, EXIT_CODES } from '../errors.js';
+import { isActionablePid, isPidAlive } from '../session-lease.js';
+
+/** A lock held longer than this is treated as abandoned by a crashed process. */
+export const LOCK_STALE_MS = 10_000;
+/** Total acquire budget. Longer than LOCK_STALE_MS so stale locks are broken, not reported. */
+export const LOCK_TIMEOUT_MS = 15_000;
+
+const RETRY_MIN_MS = 5;
+const RETRY_MAX_MS = 50;
+
+export interface FileLockOptions {
+  staleMs?: number;
+  timeoutMs?: number;
+}
+
+interface LockOwner {
+  pid?: number;
+  host?: string;
+  token?: string;
+  at?: string;
+}
+
+/** Lock marker for `target`, hidden from site memory readers by local-store. */
+export function lockPathFor(target: string): string {
+  return `${target}.lock`;
+}
+
+/** Run `fn` while holding the cross-process lock for `target`. */
+export async function withFileLock<T>(target: string, fn: () => Promise<T>, options: FileLockOptions = {}): Promise<T> {
+  const lockPath = lockPathFor(target);
+  const token = await acquire(lockPath, options);
+  try {
+    return await fn();
+  } finally {
+    await release(lockPath, token);
+  }
+}
+
+async function acquire(lockPath: string, options: FileLockOptions): Promise<string> {
+  const staleMs = options.staleMs ?? LOCK_STALE_MS;
+  const timeoutMs = options.timeoutMs ?? LOCK_TIMEOUT_MS;
+  const token = randomUUID();
+  const body = `${JSON.stringify({ pid: process.pid, host: hostname(), token, at: new Date().toISOString() })}\n`;
+  const deadline = Date.now() + timeoutMs;
+  let attempt = 0;
+
+  for (;;) {
+    if (await create(lockPath, body)) return token;
+    if (await breakIfAbandoned(lockPath, staleMs)) continue;
+    if (Date.now() >= deadline) throw busyError(lockPath, await readOwner(lockPath), timeoutMs);
+    await delay(backoffMs(attempt++));
+  }
+}
+
+/** Resolves true when this call created the lock, false when someone else holds it. */
+async function create(lockPath: string, body: string): Promise<boolean> {
+  let handle;
+  try {
+    handle = await open(lockPath, 'wx');
+  } catch (err) {
+    if (isNodeError(err) && err.code === 'EEXIST') return false;
+    throw err;
+  }
+  try {
+    await handle.writeFile(body, 'utf8');
+  } finally {
+    await handle.close();
+  }
+  return true;
+}
+
+/**
+ * Remove a lock left behind by a dead or wedged process, and report whether the
+ * caller should retry straight away. The mtime is re-read immediately before
+ * unlinking so a lock that was released and re-taken while we were deciding is
+ * left alone.
+ */
+async function breakIfAbandoned(lockPath: string, staleMs: number): Promise<boolean> {
+  const before = await statOrUndefined(lockPath);
+  if (!before) return true;
+  const owner = await readOwner(lockPath);
+  const expired = Date.now() - before.mtimeMs > staleMs;
+  const ownerGone = owner.host === hostname() && isActionablePid(owner.pid) && !isPidAlive(owner.pid);
+  if (!expired && !ownerGone) return false;
+
+  const after = await statOrUndefined(lockPath);
+  if (!after || after.mtimeMs !== before.mtimeMs) return true;
+  await unlink(lockPath).catch(ignoreMissing);
+  return true;
+}
+
+/** Never remove a lock we no longer own — it was broken and handed to another process. */
+async function release(lockPath: string, token: string): Promise<void> {
+  const owner = await readOwner(lockPath);
+  if (owner.token !== undefined && owner.token !== token) return;
+  await unlink(lockPath).catch(ignoreMissing);
+}
+
+async function readOwner(lockPath: string): Promise<LockOwner> {
+  let body: string;
+  try {
+    body = await readFile(lockPath, 'utf8');
+  } catch {
+    // The holder released (or broke) the lock between our attempt and this read.
+    return {};
+  }
+  try {
+    const parsed: unknown = JSON.parse(body);
+    return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed as LockOwner : {};
+  } catch {
+    // A truncated or hand-written lock file still blocks; it just has no owner.
+    return {};
+  }
+}
+
+function busyError(lockPath: string, owner: LockOwner, timeoutMs: number): CliError {
+  const file = basename(lockPath).replace(/\.lock$/, '');
+  const holder = isActionablePid(owner.pid) ? ` (pid ${owner.pid})` : '';
+  const stop = isActionablePid(owner.pid) && owner.host === hostname()
+    ? ` If it is stuck, run \`kill ${owner.pid}\`, or delete ${lockPath}.`
+    : ` If it is stuck, delete ${lockPath}.`;
+  return new CliError(
+    'SITE_MEMORY_BUSY',
+    `Site memory ${file} is locked by another webcmd process${holder}.`,
+    `Nothing was written. Wait for that process to finish and retry — it has held the lock for over ${Math.round(timeoutMs / 1000)}s.${stop}`,
+    EXIT_CODES.TEMPFAIL,
+  );
+}
+
+/** Exponential backoff with jitter so queued writers do not retry in lockstep. */
+function backoffMs(attempt: number): number {
+  const ceiling = Math.min(RETRY_MIN_MS * 2 ** attempt, RETRY_MAX_MS);
+  return RETRY_MIN_MS + Math.random() * (ceiling - RETRY_MIN_MS);
+}
+
+function delay(ms: number): Promise<void> {
+  return new Promise((resolve) => { setTimeout(resolve, ms); });
+}
+
+async function statOrUndefined(path: string) {
+  try {
+    return await stat(path);
+  } catch (err) {
+    if (isNodeError(err) && err.code === 'ENOENT') return undefined;
+    throw err;
+  }
+}
+
+function ignoreMissing(err: unknown): void {
+  if (isNodeError(err) && err.code === 'ENOENT') return;
+  throw err;
+}
+
+function isNodeError(err: unknown): err is NodeJS.ErrnoException {
+  return err instanceof Error && 'code' in err;
+}
diff --git a/src/site-memory/local-store.test.ts b/src/site-memory/local-store.test.ts
index e19b682c..7f514d55 100644
--- a/src/site-memory/local-store.test.ts
+++ b/src/site-memory/local-store.test.ts
@@ -1,7 +1,11 @@
+import { execFile } from 'node:child_process';
 import { createHash } from 'node:crypto';
 import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises';
+import { createRequire } from 'node:module';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import { promisify } from 'node:util';
 import { afterEach, describe, expect, it } from 'vitest';
 import {
   addFieldMapping,
@@ -136,7 +140,7 @@ describe('local site memory store', () => {
     await expect(readFileBody(homeDir, 'field-map.json')).resolves.toMatch(/"meaning": "other"/);
   });
 
-  it('survives two concurrent appendNote calls', async () => {
+  it('survives two concurrent in-process appendNote calls', async () => {
     const homeDir = await tempHome();
     await Promise.all([
       appendNote({ ...base, homeDir, text: 'alpha' }),
@@ -148,6 +152,37 @@ describe('local site memory store', () => {
     expect(body).toContain('beta');
   });
 
+  it('keeps every note when separate processes append at the same time', async () => {
+    const homeDir = await tempHome();
+
+    await runWriters(homeDir, 'note');
+
+    const body = await readNotes(homeDir);
+    for (const text of expectedWrites('note')) expect(body).toContain(text);
+  }, 120_000);
+
+  it('keeps every endpoint when separate processes write at the same time', async () => {
+    const homeDir = await tempHome();
+
+    await runWriters(homeDir, 'endpoint');
+
+    const endpoints = JSON.parse(await readFileBody(homeDir, 'endpoints.json'));
+    expect(Object.keys(endpoints).sort()).toEqual(expectedWrites('endpoint').sort());
+  }, 120_000);
+
+  it('hides write lock markers from readers', async () => {
+    const homeDir = await tempHome();
+    await appendNote({ ...base, homeDir, text: 'hello' });
+    await writeFile(join(homeDir, '.webcmd/sites', base.site, 'notes.md.lock'), '{"pid":1}\n');
+
+    await expect(showSiteMemory(base.site, { homeDir })).resolves.toEqual([
+      expect.objectContaining({ path: 'notes.md' }),
+    ]);
+    await expect(listSiteMemory(base.site, { homeDir })).resolves.toEqual([
+      expect.objectContaining({ path: 'notes.md' }),
+    ]);
+  });
+
   it('uses the injected home instead of the real home directory', async () => {
     const homeDir = await tempHome();
     process.env.HOME = await tempHome();
@@ -247,6 +282,31 @@ async function tempHome() {
   return dir;
 }
 
+const WRITER_PROCESSES = 6;
+const WRITES_PER_PROCESS = 4;
+const run = promisify(execFile);
+const require = createRequire(import.meta.url);
+
+/**
+ * The in-process promise chain cannot see another `webcmd` invocation, so the
+ * only way to cover the read-modify-write race is to run real processes. Each
+ * child writes several times to widen the window they can interleave in.
+ */
+async function runWriters(homeDir: string, mode: 'note' | 'endpoint') {
+  const script = fileURLToPath(new URL('./__fixtures__/concurrent-writer.mts', import.meta.url));
+  await Promise.all(Array.from({ length: WRITER_PROCESSES }, (_unused, index) => run(
+    process.execPath,
+    [require.resolve('tsx/cli'), script, homeDir, base.site, mode, `w${index}`, String(WRITES_PER_PROCESS)],
+    { env: { ...process.env, HOME: homeDir } },
+  )));
+}
+
+function expectedWrites(mode: 'note' | 'endpoint'): string[] {
+  return Array.from({ length: WRITER_PROCESSES }, (_unused, index) => index).flatMap((index) => (
+    Array.from({ length: WRITES_PER_PROCESS }, (_item, write) => `${mode}-w${index}-${write}`)
+  ));
+}
+
 async function readNotes(homeDir: string) {
   return readFileBody(homeDir, 'notes.md');
 }
diff --git a/src/site-memory/local-store.ts b/src/site-memory/local-store.ts
index efb883fa..012da86c 100644
--- a/src/site-memory/local-store.ts
+++ b/src/site-memory/local-store.ts
@@ -4,6 +4,7 @@ import { access, lstat, mkdir, readdir, readFile, realpath, rename, stat, unlink
 import { homedir } from 'node:os';
 import { basename, dirname, join, relative, resolve, sep } from 'node:path';
 import { validateFixture } from '../browser/verify-fixture.js';
+import { withFileLock } from './file-lock.js';
 
 type JsonObject = Record<string, unknown>;
 
@@ -59,6 +60,7 @@ export interface ResponseSampleInput extends VerifyFixtureInput {}
 
 const writeChains = new Map<string, Promise<void>>();
 const tempWritePattern = /^\..+\.\d+\.[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.tmp$/i;
+const lockWritePattern = /\.lock$/;
 
 export function appendNote(input: NoteInput): Promise<{ path: 'notes.md' }> {
   return updateText(input.site, 'notes.md', input, (existing) => {
@@ -153,7 +155,7 @@ async function updateText(
 ): Promise<void> {
   const root = await ensureSiteRoot(site, opts);
   const target = join(root, path);
-  return withPathLock(target, async () => {
+  return withWriteLock(target, async () => {
     await atomicWrite(target, update(await readText(target)));
   });
 }
@@ -166,7 +168,7 @@ async function updateJson(
 ): Promise<void> {
   const root = await ensureSiteRoot(site, opts);
   const target = join(root, path);
-  return withPathLock(target, async () => {
+  return withWriteLock(target, async () => {
     const existing = objectValue(JSON.parse(await readText(target) || '{}')) ?? {};
     await atomicWrite(target, `${JSON.stringify(update(existing), null, 2)}\n`);
   });
@@ -180,6 +182,15 @@ async function writeSiteFile(site: string, path: string, body: string, opts: Loc
   await withPathLock(target, () => atomicWrite(target, body));
 }
 
+/**
+ * Guard a read-modify-write. The in-process chain runs first because it is free
+ * and settles same-process callers without touching the disk; the file lock then
+ * covers the read as well as the write, which is what other processes need.
+ */
+function withWriteLock<T>(target: string, fn: () => Promise<T>): Promise<T> {
+  return withPathLock(target, () => withFileLock(target, fn));
+}
+
 async function withPathLock<T>(target: string, fn: () => Promise<T>): Promise<T> {
   const previous = writeChains.get(target) ?? Promise.resolve();
   const next = previous.catch(() => undefined).then(fn);
@@ -230,7 +241,7 @@ function siteRoot(site: string, opts: LocalStoreOptions): string {
 
 async function memoryPaths(root: string, requested?: string[]): Promise<string[]> {
   if (!await exists(root)) return [];
-  const paths = (requested ?? await walkFiles(root)).filter((path) => !isTempWritePath(path));
+  const paths = (requested ?? await walkFiles(root)).filter((path) => !isInternalWritePath(path));
   return Promise.all(paths.map((path) => readableRelativePath(root, path))).then((items) => items.sort());
 }
 
@@ -239,7 +250,7 @@ async function walkFiles(root: string, dir = root): Promise<string[]> {
   const nested = await Promise.all(entries.map(async (entry) => {
     const path = join(dir, entry.name);
     if (entry.isDirectory()) return walkFiles(root, path);
-    if (entry.isFile() && !isTempWritePath(entry.name)) return [relative(root, path)];
+    if (entry.isFile() && !isInternalWritePath(entry.name)) return [relative(root, path)];
     return [];
   }));
   return nested.flat();
@@ -260,8 +271,10 @@ function safeRelativePath(root: string, path: string): string {
   throw new Error(`Invalid site memory path: ${path}`);
 }
 
-function isTempWritePath(path: string): boolean {
-  return tempWritePattern.test(basename(path));
+/** In-flight write bookkeeping — temp files and lock markers are not site memory. */
+function isInternalWritePath(path: string): boolean {
+  const name = basename(path);
+  return tempWritePattern.test(name) || lockWritePattern.test(name);
 }
 
 function requiredHomeDir(opts: LocalStoreOptions): string {