fix(lock): never rename over a lock that was just released or is still being written (#760)

[SaulMoro]

Summary

acquireLock renamed over any lock it judged stale, and it judged live locks stale. Now only a lock whose owner is provably dead is renamed over.

 exclusiveCreate
-  writeFile(lock, payload, { flag: 'wx' })      ← lock exists empty until written
+  writeFile(tmp, payload); link(tmp, lock)       ← never empty; no hard links → 'wx'

 lockState (was isLockStale)
-  missing · empty · garbage · unreadable · any kill error → stale → rename over it
+  missing                    → one more exclusiveCreate
+  empty · partial · garbage · unreadable · EPERM → live (warn names the file)
+  dead pid (ESRCH)           → stale → rename over it

A releases ─┐
B reads: missing → "stale" ─┐
C creates: holds it         │
B renames over C's lock  ←──┘   B and C both hold it   (was)
B creates again → EEXIST → busy                        (now)

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Evidence

16 processes × 2000 attempts on one lock, 3 runs:

• Before (main): 4 / 12 / 13 double holders · 13 / 16 / 20 renames over a live lock
• After: 0 / 0 / 0 · 0 / 0 / 0, with about 3× the acquisitions (3039–3160 vs 750–1107): contenders no longer see empty locks
• After, no hard links (link forced to fail, wx fallback): 0 / 0 / 0 · 0 / 0 / 0 (2123–2146 acquisitions)

Tests, red on main → green:

lock-atomic   a lock that vanished during the check is not renamed over
              under the sentinel, a stale lock that vanished and was re-created is not renamed over
              an empty lock, a partly written one, garbage: not reclaimed
              a live owner running as another user (EPERM) is not reclaimed
              a lock it cannot read (EACCES) is not reclaimed
              a creator stalled mid-create does not end up sharing the lock (with and without hard links)
migrate       lock artifacts (<lock>.<uuid>.tmp, .sentinel…, .new-<uuid>) are not copied; .update-lock.backup is

Green on both, kept as guards: a lock released between create and read is taken (not "busy"); without hard links the lock still works (O_EXCL fallback, no temp file left).

Stress harness (per worker; 16 run in parallel on one lock path)

import fs from 'node:fs';
import { acquireLock, releaseLock } from './src/update.ts';
const [dir, iters] = [process.argv[2], Number(process.argv[3])];
const lock = dir + '/test.lock', holder = dir + '/holder';
let acquired = 0, overlaps = 0;
for (let i = 0; i < iters; i++) {
  if (!(await acquireLock(lock))) continue;
  acquired++;
  // O_EXCL marker: EEXIST means a second holder at the same time.
  try { fs.writeFileSync(holder, String(process.pid), { flag: 'wx' }); }
  catch { overlaps++; await releaseLock(lock); continue; }
  fs.rmSync(holder);
  await releaseLock(lock);
}
console.log(JSON.stringify({ acquired, overlaps }));

Run with vite-node worker.ts <dir> 2000. "Renames over a live lock" counts, at fse.rename onto the lock, targets whose pid is alive and not the caller's.

Test Plan

All run on 4d9ebaa; 6740c9c only changes tests, comments and docs (unit suite re-run: 4313 passed).

• npx tsc --noEmit passes
• npx vitest run passes (4313 passed, 1 skipped)
• Added/updated tests for the change
• npm run test:e2e (225 passed, 26 skipped)
• E2E, real CLI, git provider: 8 concurrent teamai pull on one scope → 1 syncs, 7 skip, usage reported once (the race does not show at this scale on main either; this checks nothing regressed)

Related Issues

Fixes #760

Merge Danger

Door: two-way

Blast Radius: locking

Every pull, push, reports/learnings worktree write, learnings publish, migration, self-mode bootstrap and update check goes through this lock. A contender now sees busy where it used to take over a live lock. The trade-off: a lock that names no owner (empty, partly written, garbage, unreadable) is never reclaimed, so one left by a crash blocks pull/push until removed by hand; a warning names the file. With hard links a live teamai never exposes such a lock; one appears only after a crash or power loss mid-create (the temp file is not fsynced before link), or a crash between open and write without hard links. Mixed versions (an older teamai still running, e.g. mid auto-update) keep a residual race that no code on this side closes: the older processes race among themselves (#760 itself), and their releaseLock deletes a lock it cannot read without checking the owner. 8 main + 8 new processes on one lock, overlaps involving a new process in 3 runs: 1 / 0 / 0 on the link path, 2 / 5 / 3 on the wx fallback. A directory-lock fallback (an older teamai cannot rename a file over a directory) measured the same, 4 / 4 / 2, because the older releaseLock deletes the directory; not adopted. It disappears once every process runs this version.

Notes for Reviewers

• link failing for any reason other than EEXIST falls back to the previous wx create, so filesystems without hard links keep working as before.
• Four mock-based tests in update.test.ts pinned the old syscall (wx write on the lock, remove never called); they now assert the same contract through link (created on the lock path, EEXIST → reclaim by rename, the lock itself not removed, wx only when link fails).
• One test from main flips on purpose: a garbage lock is no longer reclaimed.
• parseLockContent returned a bare legacy PID (999999, valid JSON) as unparseable; that only worked while unparseable meant stale. Fixed.
• No multi-process stress in the unit suite (needs TS child processes, and is probabilistic); the tests pin each verdict instead.
• A temp file left by a process killed mid-acquire shows as untracked in a self-mode .teamai/ (the .gitignore lists lock names exactly); the reclaim's .sentinel / .new-* files already could. Not changed here.
• Not in this PR: the steal of a dead reclaimer's sentinel still renames without checking again (needs a reclaimer killed mid-reclaim plus two contenders).

[github-actions]

Findings

• [P1 blocking] src/update.ts:253 — Expiring an empty lock after five seconds breaks mutual exclusion. exclusiveCreate opens the file before writing; if the creator is paused or I/O stalls beyond five seconds, another process marks that still-live lock stale and replaces it. Because lockState also handles reclaim sentinels, two reclaimers can likewise both proceed. The original writer later returns success, leaving two processes believing they hold the lock.

Test Plan

• The PR description includes unit/type-check results, the full E2E suite, and a real-CLI concurrency record, so there is no separate test-description finding.

[github-actions]

Findings

• [P1 blocking] src/update.ts:254 — The earlier empty-lock race remains on filesystems where link fails. The fallback creator can hold an empty wx file for over five seconds; a contender then marks it stale and renames over it, after which both return success. The new stalled-writer test stalls the private temp write without forcing fallback, so it does not cover this race.
• [P1 blocking] src/update.ts:250 — Treating every read error except ENOENT as stale can still replace a live lock. For example, a root-owned 0600 lock in a user-writable directory returns EACCES; the user can rename over it and become a second holder. This contradicts the claim that cross-user live locks are preserved.

Test Plan

• The PR description includes unit, type-check, full E2E, and real-CLI concurrency records, so there is no separate test-description finding.

[github-actions]

Findings

• [P1 blocking] src/update.ts:265 — The wx fallback still permits two holders when a contender reads a partially written, non-empty payload. That content is immediately classified as stale, without the five-second grace; the original writer can finish within 2.5 seconds, read back its payload, and return success before the reclaimer performs its rename, after which the reclaimer also returns success. The tests only model a completely empty file.
• [P1 blocking] src/update.ts:259 — Expiring empty locks remains unsafe with older TeamAI processes. An older version can create the empty wx file, stall beyond five seconds, then resume and return success after this version has reclaimed it. The new writer-side yielding logic cannot protect writers running the previous implementation.
• [P1 blocking] src/update.ts:325 — A fallback write taking 2.5 seconds or longer returns false but leaves a valid lock containing this still-live process’s PID. Because ownership is never recorded, releaseLock cannot remove it, and every subsequent attempt remains blocked until the process exits.
• [P2 non-blocking] src/migrate.ts:54 — The migration filter skips every top-level entry beginning with .sync-lock. or .update-lock., not only the documented temporary patterns. A legitimate entry such as .update-lock.backup is silently omitted from the new partition; match the actual .tmp, .sentinel, .new-*, and reclaim artifact formats instead.

Earlier Findings

• The unreadable-lock/EACCES finding is resolved.
• The empty-file race is resolved for writers using this new implementation, but the partial-write and older-version cases above remain.

Test Plan

• The PR description includes unit tests, type checking, full E2E results, and a real-CLI concurrency record; no separate test-description finding.

[github-actions]

Findings

• [P1 blocking] src/update.ts:304 — The no-hard-link fallback still allows mixed-version double holders. This version creates an empty wx lock before writing; an already-running older TeamAI process treats that empty file as stale and renames over it. The new writer then completes and returns success, so both processes enter the protected operation. This affects every filesystem where link fails, despite the fallback being presented as supported.

Resolved

• Empty and partially written locks are no longer reclaimed by this implementation.
• Unreadable and cross-user EPERM locks are preserved.
• Slow fallback writers no longer return failure while leaving their lock behind.
• Migration now matches specific lock artifacts rather than every similarly prefixed entry.

Test Plan

• The PR description includes unit tests, type checking, the full E2E suite, and a real-CLI concurrency record; no separate test-description finding.

[SaulMoro]

On the mixed-version P1 (no hard links, an older teamai reclaims this version's empty wx lock):

The ordering is real, but no code on this side closes mixed versions, with or without hard links. The older processes race among themselves (that is #760), and their releaseLock deletes a lock it cannot read without checking the owner. Measured with 8 main + 8 new processes on one lock, 3 runs of 2000 attempts each:

new version's create	overlaps involving a new process	old vs old
link (normal filesystems)	1 / 0 / 0	14 / 5 / 19
wx fallback (this PR, no hard links)	2 / 5 / 3	8 / 4 / 5
directory lock (tried, not adopted)	4 / 4 / 2	25 / 18 / 16

A directory lock stops the exact rename you describe (a file cannot replace a directory), but the older releaseLock then deletes the directory, so it measured no better and added ~50 lines. Once every process runs this version, the new-only stress shows 0 overlaps on both paths. Merge Danger now states this limit.