fix(site-memory): lock site memory writes across processes

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 });
+  }
+}

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);
+}

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;
+}

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');
 }