Fix Graphify writer lock races and pin query snapshots

[lopugit]

Concurrent Graphify queries could delete a replacement writer's lock while reclaiming an old owner, causing ENOENT during owner-file creation. Queries also released the lock before their Graphify child finished, allowing another command to prune the snapshot being read.

Publish a populated lock directory atomically, use unique owner records, and remove only the releasing owner's record plus an empty directory. Keep the selected snapshot locked through query completion. Preserve bounded timeouts and refuse to reclaim unknown/live ownership.

Validation:

• 21 Graphify tests pass, including deterministic delayed-reaper cases for legacy and new owner records, six processes/30 critical sections, SIGKILL recovery, malformed owners, timeouts, and reader lifetime.
• The delayed-reaper regression fails on the original implementation.
• Three actual concurrent Graphify queries against a fresh temporary repository all succeeded, performed exactly one rebuild, and left no lock behind.
• Syntax and patch hygiene checks pass. Generated graph output refreshed through the local semantic backend; snapshot hashes, node IDs, and edge endpoints verified. Upstream reported one semantic testing ID collision between TESTING.md and RecoveryCoreTests.swift; this warning is separate from locking.

Upgrade note: finish pre-upgrade Graphify processes in a checkout before replacing its router; old recursive cleanup cannot participate safely in the new protocol. The LLM proxy request-size change is being handled separately.

Control-plane counterpart: #644

[lopugit]

🧹 Develop S3 preview removed

The PR-specific alias and every workflow-created develop deployment were removed when this PR closed.

The ordinary generated Vercel Preview remains available on the shared development runtime.

[github-actions]

Lopu · review of the writer-lock rewrite — head a488cd22 vs develop@bc061f63

The lock change is correct, and the mechanism is the interesting part: acquisition moves from mkdir + then write owner.json to building a private .locks/.writer-<uuid>/ that already contains its owner-<uuid>.json, and rename()ing that populated directory onto .locks/writer.

That closes the window properly. rename(2) onto an existing directory succeeds only if the target is empty, and the published directory is never empty, so a live lock cannot be replaced. The old code had the inverse problem — rmSync(lockPath, { recursive: true }) on release could delete a successor's lock.

I walked the release interleaving that would break a naive version of this:

1. holder A unlinks owner-A.json → directory is now empty
2. waiter B renames its candidate on → B owns the lock
3. A's late rmdir fires → ENOTEMPTY → swallowed → B keeps it ✅

Non-recursive rmdir plus per-attempt owner filenames is exactly what makes step 3 safe. Recursive removal or a shared owner.json name would both lose here.

Two supporting changes are right for the same reason — they refuse to guess:

• ownerAlive returns true for an unparseable/missing owner record, so unknown ownership is never grounds for stealing.
• liveness widened from error.code === 'EPERM' to error.code !== 'ESRCH', so only a definitively-absent PID is reaped.

No deadlock from moving invokeGraphify inside the lock, which was my main worry with runRouted: it spawns the external graphify binary, not scripts/graphify, so the wrapper can't recurse into withRepositoryLock.

Two things worth knowing, neither blocking

Queries now serialise. Holding the writer lock across the whole child process is the right call — it's what stops pruneSnapshots deleting the snapshot a reader is mid-read on — but with DEFAULT_LOCK_TIMEOUT_MS at two hours, a long extract blocks concurrent queries for up to two hours before they fail. Deliberate and documented; flagging so the cost is chosen rather than discovered.

|| → ?? on GRAPHIFY_CAS_LOCK_TIMEOUT_MS. An empty-string value now parses to 0 — one acquisition attempt — instead of falling back to the default. Nothing outside graphify-cas.test.mjs sets this variable, so there's no live exposure today, and 0 is documented. It'd only bite if the variable is ever plumbed through a workflow input, where empty values are routine. A trim-and-fall-back-if-empty guard would remove the trap; I didn't add one, since inventing a caller that doesn't exist isn't a good reason to touch this file.

Changes I made

Only prose, in the two docs this PR already edits — no logic touched:

• README.md — the edited sentence landed as a 127-char line inside a paragraph hand-wrapped at ≤80. Rewrapped; peaks at 78 now.
• docs/graphify-content-addressed-snapshots.md — new section was added after a doubled blank line. Collapsed to the single blank line every other section uses.

Validation

node --test scripts/graphify-cas.test.mjs → 21/21 pass (4.0s) — delayed stale-reaper vs. replacement owner, six processes / 30 non-overlapping writes, SIGKILL recovery, callback-error release, malformed + legacy ownership under a bounded timeout, and a routed query holding its snapshot lock until the subprocess exits.

Checks on this head: 16 success, 66 skipped, 0 failures. CodeQL: 0 open alerts, nothing dismissed.

[github-actions]

Lopu repository review

Lopu reviewed this PR against develop as Thingtime's principal PR and repository manager. Using Claude Opus 5.

Lopu made justified improvements and pushed eba1224 to codex/graphify-writer-lock.

Lopu review — PR #645 · Fix Graphify writer lock races and pin query snapshots

Compared head a488cd22 against base develop @ bc061f63 (merge-base
04d77964, 1 commit ahead). Non-generated change surface: scripts/graphify-cas.mjs,
scripts/graphify-cas.test.mjs, README.md, TESTING.md,
docs/graphify-content-addressed-snapshots.md, remix/CHANGELOG.md.

Check state

No failing checks on a488cd22: 16 success, 66 skipped, 0 failure/cancelled/
timed-out. CodeQL alert snapshot for this head is empty, so
lopu-codeql-dispositions/645.json stays [].

What the change does

Replaces the mkdir + write owner.json lock acquisition with an atomic
publish: each attempt builds a private .locks/.writer-<uuid>/ directory,
writes its uniquely named owner-<uuid>.json into it, then rename()s that
populated directory onto .locks/writer.

Assessment — correctness

The core invariant holds. rename(2) onto an existing directory succeeds only
when the target is empty, and the published directory always contains its owner
record before the rename, so a live lock can never be replaced. Release is
correspondingly narrow: unlink of the caller's own owner file followed by a
non-recursive rmdir, with ENOTEMPTY/EEXIST swallowed. I walked the
interleaving where holder A unlinks its record, B renames onto the now-empty
directory, and A's late rmdir fires — A's rmdir fails ENOTEMPTY and is
ignored, so B keeps the lock. The old rmSync(lockPath, { recursive: true })
could destroy a successor's lock in that same window; that is genuinely fixed.

Two supporting changes are also right:

• ownerAlive now returns true for unparseable or missing owner records, so
  unknown ownership is never grounds for stealing a lock.
• process.kill(pid, 0) liveness widened from error.code === 'EPERM' to
  error.code !== 'ESRCH', so only a definitively absent process is reaped.

reapExitedOwner recognises a legacy owner.json but never recursively deletes
it, which keeps the pre-upgrade interop path safe. The mixed-version hazard that
remains — a legacy writer that has mkdired but not yet written its owner file
leaves the path momentarily empty and can be replaced — is called out in
docs/graphify-content-addressed-snapshots.md as a "finish running commands
before upgrading" caveat. That is an acceptable, documented migration boundary.

No re-entrancy risk from moving invokeGraphify inside the lock:
invokeGraphify spawns the external graphify binary, not scripts/graphify,
so the wrapper cannot recurse into withRepositoryLock.

Every symbol the new code needs is imported (randomUUID from node:crypto;
renameSync, rmdirSync, readdirSync, unlinkSync from node:fs).

Assessment — behaviour changes worth knowing

• Queries now serialise. runRouted holds the writer lock for the whole
  Graphify child process so pruneSnapshots cannot delete the snapshot a reader
  is using. That is the right correctness trade, but with
  DEFAULT_LOCK_TIMEOUT_MS at two hours a long extract will block concurrent
  queries for up to two hours before they fail. Deliberate and documented; noted
  so the operational cost is explicit rather than discovered.
• || → ?? on GRAPHIFY_CAS_LOCK_TIMEOUT_MS. An empty-string value now
  parses to 0 (one acquisition attempt) instead of falling back to the
  default. Nothing in the repository sets this variable outside
  scripts/graphify-cas.test.mjs, so there is no live exposure, and the 0
  semantics are documented. Flagging rather than changing: a .trim()-and-
  fall-back-if-empty guard would remove the trap if the variable is ever plumbed
  through a workflow input, where empty values are common.
• Pre-existing, not from this PR: THINGTIME_GRAPHIFY_CAS_CHILD is set in two
  places and read nowhere in the repository. Left alone — it is plausibly
  consumed by the external graphify binary.

Changes I made

Two formatting regressions in the prose this PR touched. No logic was changed.

1. README.md — the edited sentence produced a 127-character line inside a
   paragraph that is hand-wrapped at ≤80. Rewrapped the affected lines; the
   paragraph now peaks at 78 characters.
2. docs/graphify-content-addressed-snapshots.md — the new section was added
   after a doubled blank line. Collapsed to the single blank line used between
   every other section in the file.

Validation

• node --test scripts/graphify-cas.test.mjs → 21/21 pass (4.0 s), covering
  delayed stale-reaper vs. replacement owner, six-process contention with 30
  non-overlapping writes, SIGKILL recovery, callback-error release, malformed/
  legacy ownership with a bounded timeout, and a routed query holding its
  snapshot lock until the subprocess exits.
• Re-read the acquisition/release interleavings by hand as described above.

Verdict

The concurrency work is sound, well-tested, and the documentation matches the
implementation. No blocking findings.

View Lopu workflow run

[lopugit]

🤖 Promotion conflict resolution was queued automatically for promote/pr-645-graphify-writer-lock--to-main at exact base main (65b03ace91a3002962c92cb908be291a0d909c24).

Conflicted source paths: remix/CHANGELOG.md.

The trusted worker will reconstruct, verify, publish, and attest the review branch; no manual branch update is needed.

[github-actions]

⚠️ Lopu did not complete the promotion resolution before a reviewable PR was fully published.

Branch: promote/pr-645-graphify-writer-lock--to-main · base: main at e30df5dd506ff3fc1a04b31e90c5aa4e03a488d0 · plan: 313c56e753860ac329c15aaf009a4eca815466f323f113ba5c3c478e0a065970.

No newer branch work was overwritten or deleted. The same immutable snapshot will not spend AI again; a changed source/base snapshot retries automatically. Review the run.

[lopugit]

🤖 Promotion conflict resolution was queued automatically for promote/pr-645-graphify-writer-lock--to-main at exact base main (e30df5dd506ff3fc1a04b31e90c5aa4e03a488d0).

Conflicted source paths: remix/CHANGELOG.md.

The trusted worker will reconstruct, verify, publish, and attest the review branch; no manual branch update is needed.