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

CHANGELOG.md

@@ -27,6 +27,7 @@ All notable changes to this project will be documented in this file. See [standa
 
 ### 🐛 Bug Fixes
 
+- The lock behind `pull`, `push`, the reports and learnings worktrees, learnings publishing, migration, self-mode bootstrap and the update check no longer hands one lock to two live processes. Reclaiming a stale lock renamed over whatever file was there once it had judged the lock stale, and it judged live locks stale: one that had just been released (and could be re-created by a third process before the rename), one whose owner had created it but not yet written it, one owned by a process running as another user (for example a `sudo teamai` run), and one it could not read. Under 16 processes contending on one lock, about 1% of acquisitions overlapped another holder, enough for two pulls to report the same usage twice. Now only a lock whose owner is provably gone is reclaimed; a lock that vanished gets one more exclusive create, and a new lock is published with its content already in place (written to a temp file, then hard-linked to the lock name; a filesystem without hard links falls back to the previous create). A lock that names no owner (empty, partly written, unreadable) is never reclaimed: if a crash left one, `pull` and `push` report busy until it is removed, and a warning names the file. Migration skips the lock's temporary files, which a contending pull creates and removes while the copy runs. With the change, the same stress run shows no overlap (for [#760](https://github.com/Tencent/teamai-cli/issues/760)).
 - Team hooks stay out of projects that never set up teamai. A project-scope install puts its hooks in the home directory, so they fire in every project on the machine, and with no config for the directory they used to run anyway: the end-of-session share reminder (shown there even with recall off, a case a configured team never sees), the TodoWrite recall nudge, and the local recording of sessions and skill usage that a later report from another project pushed to its team. A handler that needs a team now declares `requiresConfig`, and the dispatcher drops it when neither a project nor a user config resolves for the hook's `cwd`; only machine-level work runs there (CLI update check, session-start pull, local agent, package hints the pull stashed). A config that exists but fails to parse reads the same way, so it withholds team prompts rather than running all of them, and for hooks and skill usage an unreadable project config never falls back to the user scope; `teamai doctor` reports it. A host that sends no `cwd` (OpenClaw) resolves the project from the directory it runs the hook in, a `cwd` that no longer exists resolves to the user scope instead of failing the hook, and the legacy `teamai contribute-check` command that older installs still call follows the same rule (for [#748](https://github.com/Tencent/teamai-cli/issues/748)).
 - Skill usage stays with the team of the project it was recorded in. Every scope used to append to one `~/.teamai/usage.jsonl`, so whichever project pulled next reported every project's skills to its own team, including skills that exist only in an unrelated private repo. Usage now goes to the data directory of the scope that resolves for the session's directory (`<dataHome>/usage.jsonl`: the project partition, `<repo>/.teamai` in single-repo mode, `~/.teamai` for the user scope), each report reads and truncates only its own file, and `teamai stats` shows the current scope's usage. A machine's first user scope starts with an empty file, since the events it held by then cannot be attributed; on a machine with only project scopes, events recorded before the upgrade stay unreported. Stats already pushed are not rewritten (for [#748](https://github.com/Tencent/teamai-cli/issues/748)).
 - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)).

docs/designs/data-directory-layout.md

@@ -118,14 +118,23 @@ is a P1 concern. This keeps P0 independently reviewable (issue R7).
    processes could both observe "no lock" and both succeed, and `releaseLock()`
    unconditionally deleted the file — including a lock another process later
    acquired. Rewritten to:
-   - Acquire with an atomic exclusive create (`writeFile(path, payload, { flag: 'wx' })`
-     = `O_CREAT|O_EXCL`); payload is JSON `{ pid, startedAt, owner }` with a random
-     `owner` token.
-   - On `EEXIST`, reclaim only a **stale** lock (dead pid via `process.kill(pid,0)`,
-     or unparseable content). The reclaim is **serialized behind an atomically-created
-     reclaim sentinel** and finished with an atomic rename-into-place, so concurrent
-     reclaimers cannot each end up believing they hold the lock; a live holder returns
-     "busy".
+   - Acquire with an atomic exclusive create: the payload is written to a private
+     temp file and hard-linked to the lock name (`link` fails with `EEXIST` like
+     `O_CREAT|O_EXCL`), so the lock never exists without its content (#760); a
+     filesystem without hard links falls back to `writeFile(path, payload, { flag: 'wx' })`.
+     Payload is JSON `{ pid, startedAt, owner }` with a random `owner` token.
+   - On `EEXIST`, reclaim only a **stale** lock: one whose owner is provably gone
+     (`process.kill(pid,0)` fails with `ESRCH`). The reclaim is **serialized behind an
+     atomically-created reclaim sentinel** and finished with an atomic rename-into-place,
+     so concurrent reclaimers cannot each end up believing they hold the lock; a live
+     holder returns "busy". Anything that cannot name a dead owner is held (#760): a
+     lock that cannot be read (`EACCES`), an empty or partly written one (the `wx`
+     fallback and older teamai open the file before writing), and a pid owned by another
+     user (`EPERM`). A lock that names no owner, or cannot be read, stays until
+     removed by hand if a crash left it, and a warning names it. A lock that vanished before it could be read gets one more
+     exclusive create instead (a third process may already have re-created it).
+   - Migration skips the locks' transient artifacts (`<lock>.<uuid>.tmp`, `.sentinel`
+     and its temps, `.new-<uuid>`) along with the locks themselves.
    - `releaseLock()` returns early when this process holds no owner token for the
      path, and otherwise deletes only when the on-disk `owner` still matches the token
      this process recorded — never another process's lock.

src/__tests__/lock-atomic.test.ts

@@ -1,15 +1,17 @@
-import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
 import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
+import fse from 'fs-extra';
 import { acquireLock, releaseLock } from '../update.js';
 
 // ─── Real-filesystem tests for the atomic lock (issue #374 P0) ──────────────
 //
-// These exercise acquireLock/releaseLock against a real temp directory (no fs
-// mock), so the OS-level O_CREAT|O_EXCL ('wx') exclusivity and the on-disk owner
-// token are genuinely tested — the thing the previous check-then-write lock got
-// wrong.
+// These exercise acquireLock/releaseLock against a real temp directory, so the
+// OS-level exclusivity (link's EEXIST, O_CREAT|O_EXCL without hard links) and the
+// on-disk owner token are genuinely tested — the thing the previous
+// check-then-write lock got wrong. Spies on fs-extra only stage the interleavings
+// a single process cannot produce on its own (#760).
 
 let tmpDir: string;
 let lockPath: string;
@@ -72,10 +74,11 @@ describe('acquireLock (real fs)', () => {
     await releaseLock(lockPath);
   });
 
-  it('reclaims a lock whose contents are unparseable garbage', async () => {
+  it('does not reclaim a lock whose contents name no owner (garbage) (#760)', async () => {
+    // Nothing proves its owner dead; a crash that left it needs a hand removal.
     fs.writeFileSync(lockPath, 'not-json-not-a-pid');
-    expect(await acquireLock(lockPath)).toBe(true);
-    await releaseLock(lockPath);
+    expect(await acquireLock(lockPath)).toBe(false);
+    expect(fs.readFileSync(lockPath, 'utf-8')).toBe('not-json-not-a-pid');
   });
 
   it('grants the lock to exactly one of many concurrent reclaimers of a STALE lock', async () => {
@@ -97,6 +100,160 @@ describe('acquireLock (real fs)', () => {
   });
 });
 
+describe('acquireLock never takes over a lock another live process may hold (#760)', () => {
+  it('does not rename over a lock it could not read (released and re-created in between)', async () => {
+    // Another process holds the lock; this one only ever sees it vanish, as when
+    // the previous holder released it and a third process re-created it between
+    // this process's failed create and its read.
+    fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, owner: 'other', startedAt: 'x' }));
+    const readFile = vi.spyOn(fse, 'readFile').mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' }));
+    try {
+      expect(await acquireLock(lockPath)).toBe(false);
+    } finally {
+      readFile.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).owner).toBe('other');
+  });
+
+  it('takes a lock released between its failed create and its read, instead of reporting busy', async () => {
+    fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, owner: 'other', startedAt: 'x' }));
+    const readFile = vi.spyOn(fse, 'readFile').mockImplementationOnce(async () => {
+      fs.rmSync(lockPath); // the holder releases it
+      throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' });
+    });
+    try {
+      expect(await acquireLock(lockPath)).toBe(true);
+    } finally {
+      readFile.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).pid).toBe(process.pid);
+    await releaseLock(lockPath);
+  });
+
+  it('under the reclaim sentinel, does not rename over a stale lock that vanished and was re-created', async () => {
+    // The first pass sees a dead owner and takes the sentinel; by the second pass
+    // the dead lock was cleared and another process re-created it.
+    const dead = JSON.stringify({ pid: 999999, owner: 'dead', startedAt: 'x' });
+    fs.writeFileSync(lockPath, dead);
+    const readFile = vi.spyOn(fse, 'readFile')
+      .mockImplementationOnce(async () => dead)
+      .mockImplementationOnce(async () => {
+        fs.writeFileSync(lockPath, JSON.stringify({ pid: process.pid, owner: 'other', startedAt: 'x' }));
+        throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' });
+      });
+    try {
+      expect(await acquireLock(lockPath)).toBe(false);
+    } finally {
+      readFile.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).owner).toBe('other');
+  });
+
+  it('does not reclaim a lock whose owner is alive but belongs to another user (EPERM)', async () => {
+    fs.writeFileSync(lockPath, JSON.stringify({ pid: 424242, owner: 'root-owned', startedAt: 'x' }));
+    const kill = vi.spyOn(process, 'kill').mockImplementation(() => {
+      throw Object.assign(new Error('EPERM'), { code: 'EPERM' });
+    });
+    try {
+      expect(await acquireLock(lockPath)).toBe(false);
+    } finally {
+      kill.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).owner).toBe('root-owned');
+  });
+
+  it('a creator stalled mid-create does not end up sharing the lock', async () => {
+    // Model the syscalls: a write straight to the lock name is an O_EXCL create,
+    // which opens the file before writing it; any other write lands whole. The
+    // first acquirer stalls inside its write until a second acquirer has had its
+    // try.
+    let resume = (): void => {};
+    const stalled = new Promise<void>((resolve) => { resume = resolve; });
+    const writeFile = vi.spyOn(fse, 'writeFile').mockImplementationOnce(async (file, data) => {
+      if (file === lockPath) fs.writeFileSync(file, '', { flag: 'wx' });
+      await stalled;
+      fs.writeFileSync(file, data);
+    });
+    try {
+      const first = acquireLock(lockPath);
+      await vi.waitFor(() => expect(writeFile).toHaveBeenCalled());
+      const second = await acquireLock(lockPath);
+      resume();
+      expect([await first, second].filter(Boolean)).toHaveLength(1);
+    } finally {
+      resume();
+      writeFile.mockRestore();
+    }
+  });
+
+  it('still locks on a filesystem without hard links (falls back to O_EXCL)', async () => {
+    const link = vi.spyOn(fse, 'link').mockRejectedValue(Object.assign(new Error('EPERM'), { code: 'EPERM' }));
+    try {
+      expect(await acquireLock(lockPath)).toBe(true);
+      expect(await acquireLock(lockPath)).toBe(false);
+    } finally {
+      link.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).pid).toBe(process.pid);
+    expect(fs.readdirSync(tmpDir).filter((f) => f.endsWith('.tmp'))).toEqual([]);
+    await releaseLock(lockPath);
+  });
+
+  it('without hard links, a creator stalled for any time keeps its lock', async () => {
+    // O_EXCL fallback: the first acquirer opens the lock and stalls before
+    // writing; a second one finds it empty and must back off.
+    let resume = (): void => {};
+    const stalled = new Promise<void>((resolve) => { resume = resolve; });
+    let stalledOnce = false;
+    vi.spyOn(fse, 'link').mockRejectedValue(Object.assign(new Error('EPERM'), { code: 'EPERM' }));
+    vi.spyOn(fse, 'writeFile').mockImplementation(async (file, data) => {
+      if (file === lockPath && !stalledOnce) {
+        stalledOnce = true;
+        const fd = fs.openSync(file, 'wx');
+        await stalled;
+        fs.writeSync(fd, String(data));
+        fs.closeSync(fd);
+        return;
+      }
+      fs.writeFileSync(file, data, { flag: file === lockPath || String(file).endsWith('.sentinel') ? 'wx' : 'w' });
+    });
+    try {
+      const first = acquireLock(lockPath);
+      await vi.waitFor(() => expect(stalledOnce).toBe(true));
+      const second = await acquireLock(lockPath);
+      resume();
+      expect([await first, second]).toEqual([true, false]);
+    } finally {
+      resume();
+      vi.restoreAllMocks();
+    }
+    await releaseLock(lockPath);
+  });
+
+  it('does not reclaim a lock it cannot read (EACCES: e.g. a root-owned 0600 lock)', async () => {
+    fs.writeFileSync(lockPath, JSON.stringify({ pid: 424242, owner: 'root-owned', startedAt: 'x' }));
+    const readFile = vi.spyOn(fse, 'readFile').mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' }));
+    try {
+      expect(await acquireLock(lockPath)).toBe(false);
+    } finally {
+      readFile.mockRestore();
+    }
+    expect(JSON.parse(fs.readFileSync(lockPath, 'utf-8')).owner).toBe('root-owned');
+  });
+
+  it('does not reclaim an empty lock (its owner may still be writing it)', async () => {
+    fs.writeFileSync(lockPath, '');
+    expect(await acquireLock(lockPath)).toBe(false);
+    expect(fs.readFileSync(lockPath, 'utf-8')).toBe('');
+  });
+
+  it('does not reclaim a partly written lock (its creator is mid-write)', async () => {
+    fs.writeFileSync(lockPath, '{"pid":99');
+    expect(await acquireLock(lockPath)).toBe(false);
+    expect(fs.readFileSync(lockPath, 'utf-8')).toBe('{"pid":99');
+  });
+});
+
 describe('releaseLock (real fs)', () => {
   it('removes a lock this process owns', async () => {
     await acquireLock(lockPath);

src/__tests__/migrate.test.ts

@@ -280,6 +280,26 @@ describe('runMigration', () => {
     expect(await fse.pathExists(path.join(partition, '.update-lock'))).toBe(false);
   });
 
+  it('ignores lock artifacts that come and go while it copies (#760)', async () => {
+    await seedLegacyLayout();
+    // A contending pull's temp file for the exclusive create, a reclaim
+    // sentinel and a reclaim temp: each can appear or vanish mid-copy.
+    const uuid = '3f2a9c1e-7b4d-4e8a-9c6f-0d1e2f3a4b5c';
+    const artifacts = [
+      `.sync-lock.${uuid}.tmp`, '.sync-lock.sentinel', `.sync-lock.sentinel.reclaim-${uuid}`,
+      `.sync-lock.sentinel.${uuid}.tmp`, `.update-lock.new-${uuid}`,
+    ];
+    for (const name of [...artifacts, '.update-lock.backup']) {
+      await fse.writeFile(path.join(legacyDir, name), '{}');
+    }
+    const plan = await planMigration(repoRoot);
+    await runMigration(plan!);
+    const partition = projectDataHome(repoRoot);
+    // Only the lock artifacts stay behind; a look-alike entry of the user's travels.
+    expect((await fse.readdir(partition)).filter((n) => n.startsWith('.sync-lock') || n.startsWith('.update-lock')))
+      .toEqual(['.update-lock.backup']);
+  });
+
   it('carries contributions that are not published yet', async () => {
     await seedLegacyLayout();
     // Unlike a worktree, the queue holds work the member has already done and