[Bug]: Silent data loss in site memory due to cross-process race condition

[Kaushik2003]

Description

Site memory writes are protected by an in-memory promise chain, not by anything the filesystem can see. Two webcmd processes writing to the same site therefore overwrite each other's work, and the losing write disappears with no error and exit code 0.

withPathLock() is the guard, and it is a plain Map held in one process's heap:

// src/site-memory/local-store.ts:60
const writeChains = new Map<string, Promise<void>>();

// src/site-memory/local-store.ts:183
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);
  const settled = next.then(() => undefined, () => undefined);
  writeChains.set(target, settled);
  settled.then(() => {
    if (writeChains.get(target) === settled) writeChains.delete(target);
  });
  return next;
}

A second webcmd process starts with its own empty writeChains. It has no way to learn that another process is mid-write, so the two proceed in parallel.

That would be harmless if the writes were blind overwrites. They are not — updateText() and updateJson() are read-modify-write:

// src/site-memory/local-store.ts:148
async function updateText(
  site: string,
  path: 'notes.md',
  opts: LocalStoreOptions,
  update: (existing: string) => string,
): Promise<void> {
  const root = await ensureSiteRoot(site, opts);
  const target = join(root, path);
  return withPathLock(target, async () => {
    await atomicWrite(target, update(await readText(target)));
  });
}

That inner line is a read (readText), a modify (update), and a write (atomicWrite) — three steps that must not be split by another writer. updateJson() at :161 has the same shape.

So two processes interleave like this:

process A: read notes.md   -> "" (or existing content)
process B: read notes.md   -> same content
process A: write content + note-A
process B: write content + note-B      <- built from the stale read; note-A is gone

atomicWrite() (src/site-memory/local-store.ts:194) is written carefully — it writes to a temp file named with process.pid + randomUUID() and then rename()s it, so the file is never torn or half-written. That is correct as far as it goes, and it is probably why this looked safe. But rename() gives atomicity of a single write, not isolation across a read-modify-write pair. The result is last-writer-wins, and last-writer-wins on a read-modify-write means lost updates.

Reproduction

Twenty concurrent webcmd site note add calls against one site. Every process exits 0; only some of the notes survive.

#!/usr/bin/env bash
# Run from a webcmd checkout after `npm install && npm run build`
export HOME=$(mktemp -d)
WEBCMD="node $PWD/dist/src/main.js"

for i in $(seq 1 20); do
  $WEBCMD site note add example.com --text "note-$i" &
done
wait

echo "notes written  : 20"
echo "notes surviving: $(grep -c '^note-' "$HOME/.webcmd/sites/example.com/notes.md")"

Output across three consecutive runs:

notes written  : 20
notes surviving: 18
---
notes written  : 20
notes surviving: 19
---
notes written  : 20
notes surviving: 18

It is nondeterministic — it depends on how the reads and writes interleave — so it does occasionally pass. Surviving notes across ten runs: 20, 19, 19, 19, 18, 18, 18, 17, 17, 13 out of 20. Nine of the ten lost data, the worst run losing 35% of it. No run printed a warning, and no process exited non-zero.

The same failure hits endpoints.json through the updateJson() path:

for i in $(seq 1 20); do
  $WEBCMD site endpoint set example.com "ep$i" --url "https://example.com/api/$i" --method GET &
done
wait
# keys in endpoints.json across three runs: 17, 19, 19  (expected 20)

Affected files

Location	What is affected	Why
src/site-memory/local-store.ts:60, :183	writeChains, withPathLock()	The lock is a per-process in-memory Map. Nothing coordinates across processes — no lockfile, no O_EXCL, no flock.
src/site-memory/local-store.ts:148, :161	updateText(), updateJson()	The read-modify-write functions. These are the only two places where a stale read can silently erase a concurrent write.
src/site-memory/local-store.ts:63, :70, :87, :96	appendNote(), setEndpoint(), markEndpointStale(), addFieldMapping()	The four public writers routed through updateText/updateJson. All four can lose data.
src/site-memory/local-store.test.ts:138	"survives two concurrent appendNote calls"	Covers only the in-process case, which already works. See below.

Not affected, for scope clarity:

• writeSiteFile() (src/site-memory/local-store.ts:175), used by putVerifyFixture() and addResponseSample(), writes a complete body rather than modifying an existing one. Last-writer-wins there is defensible, and addResponseSample() generates a unique path per call anyway.
• Hosted mode is unaffected. src/hosted/runner.ts:495 registers a server-side backend; this is specific to the local backend registered at src/cli.ts:624.

Why the existing test does not catch it

// src/site-memory/local-store.test.ts:138
it('survives two concurrent appendNote calls', async () => {
  const homeDir = await tempHome();
  await Promise.all([
    appendNote({ ...base, homeDir, text: 'alpha' }),
    appendNote({ ...base, homeDir, text: 'beta' }),
  ]);

  const body = await readNotes(homeDir);
  expect(body).toContain('alpha');
  expect(body).toContain('beta');
});

Both calls run inside a single process via Promise.all, which is exactly the case writeChains handles correctly. The test passes, and its name reads as though concurrent writes are covered, while the cross-process case is untested and broken. Worth noting mainly because it explains how this got through review.

Why a fix is necessary

This is silent data loss in the feature whose entire purpose is to accumulate knowledge over time. Site memory is what an agent has learned about a site — verified endpoints, field meanings, notes — and losing it is not a cosmetic failure.

It does not require an exotic setup. Any of these hit it today:

• Two agent sessions in two terminals working on the same site.
• CI or scripted runs invoking webcmd tasks in parallel.
• One agent running several webcmd calls concurrently rather than sequentially.

Because there is no error and no non-zero exit, nothing surfaces the loss. A user only notices later, when an endpoint they recorded is missing, and at that point there is no signal pointing at concurrency as the cause.

Suggested fix

The data loss can be closed entirely inside local-store.ts, with no dependency and no change to the on-disk format:

1. Keep writeChains as the in-process fast path — it works and it avoids lock churn for the common single-process case.
2. Wrap the read-modify-write in updateText() and updateJson() in a cross-process lock: create <target>.lock with fs.open(..., 'wx') (O_EXCL, atomic create-if-absent), retry with backoff when it exists, and break locks older than a timeout by mtime so a crashed process cannot wedge site memory permanently. Release in a finally.
3. Critically, move the read inside the lock, so the sequence becomes acquire → read → modify → write → release. Locking only the write would not fix anything.

Sketch:

async function withCrossProcessLock<T>(target: string, fn: () => Promise<T>): Promise<T> {
  const lockPath = `${target}.lock`;
  // acquire: open(lockPath, 'wx') with retry/backoff; if EEXIST and the lock's
  // mtime is older than STALE_MS, unlink and retry once.
  try {
    return await fn();
  } finally {
    await unlink(lockPath).catch(() => undefined);
  }
}

withPathLock() then composes the two, so callers do not change:

return withPathLock(target, () => withCrossProcessLock(target, fn));

If a dependency is acceptable, proper-lockfile handles the stale-lock and retry logic and would be a smaller diff.

A test for the real case would need to spawn two child processes rather than use Promise.all — the reproduction script above is the shape of it, and it fails before the fix and passes after.

I am happy to open a PR for this if the approach sounds right.

Environment

• webcmd 0.7.1 (52da25b)
• Node.js v24.14.0
• Linux (Fedora, kernel 7.1.8)
• Local site-memory backend (default)