fix(git-sync): gate stale-lock removal by mtime + cover preflight self-heals (#119 F1-F4)

clearStaleGitLocks() removed a hardcoded list of 9 git lock files
UNCONDITIONALLY — despite its name it never checked staleness. In a
multi-replica TTL-lapse window (a documented engine limitation), replica B's
preflight could rm a LIVE index.lock held by replica A mid `git add`/`commit`,
turning a safe fail-fast ("index.lock: File exists") into concurrent
index/ref writes and corruption.

F1: gate each removal by file age. A live git op is bounded by
GIT_EXEC_TIMEOUT_MS (120s), after which git is killed and the lock's mtime
freezes — so a lock older than 3× that (STALE_LOCK_MIN_AGE_MS = 360s) provably
has no live holder and is a genuine crash-leftover. Fresh locks (mtime within
the window) are preserved; missing locks are a no-op. Strictly safer than the
prior unconditional rm (the gate can only prevent removals, never add them).

F4: move clearStaleGitLocks (doc + body) below isMergeInProgress so the
mid-merge jsdoc re-attaches to its real owner; soften the doc (it clears a
fixed list of the engine's own index/ref locks, not "any *.lock").

F2: cycle.test.ts pins the preflight order
ensureRepo < clearStaleGitLocks < ensureMainBranch < ensureBranch (a refactor
that moved clearStaleGitLocks before ensureRepo — deleting ensureRepo's own
transient lock — would now fail the test).

F3: git.test.ts covers ensureMainBranch's HEAD-fallback branch (both main and
docmost gone → main recreated from HEAD) and the no-commit no-op.

Also: the D3-N3 test now backdates the planted lock's mtime so it is stale (and
still removed), plus a new test asserts a FRESH lock is PRESERVED (the
corruption-safety property — this would fail against the old code). cycle.ts's
preflight comment softened to match the mtime gating.

git-sync vitest: 711 passed | 1 expected-fail (+4 new tests). tsc clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

packages/git-sync/src/engine/cycle.ts

@@ -120,9 +120,10 @@ export async function runCycle(deps: RunCycleDeps): Promise<RunCycleResult> {
   //     hard crash / OOM-kill / abrupt container stop mid `git add`/`commit`/
   //     `checkout` leaves a `.git/index.lock` (or a ref `*.lock`); git then refuses
   //     every later op ("Unable to create '…/index.lock': File exists"), wedging the
-  //     space forever with no self-heal. The daemon holds the per-space Redis lock
-  //     and is the vault's only writer, so any leftover lock here is stale — remove
-  //     it before the merge check + any checkout/diff below.
+  //     space forever with no self-heal. Only locks OLDER than the staleness
+  //     threshold are removed (a fresh lock from a concurrent replica in the
+  //     TTL-lapse window is preserved), before the merge check + any checkout/diff
+  //     below.
   await vault.clearStaleGitLocks();
 
   // 1c. RESTORE a missing `main` branch (bug D3-N1). Ref-store damage can leave an

packages/git-sync/src/engine/git.ts

@@ -19,7 +19,7 @@
  *   - "nothing to commit" is treated as a graceful no-op, not an error.
  */
 import { execFile } from "node:child_process";
-import { mkdir, rm } from "node:fs/promises";
+import { mkdir, rm, stat } from "node:fs/promises";
 import { promisify } from "node:util";
 
 const execFileAsync = promisify(execFile);
@@ -30,6 +30,13 @@ const execFileAsync = promisify(execFile);
 // cycle (the same risk the http-backend watchdog guards on the server side).
 const GIT_EXEC_TIMEOUT_MS = 120_000;
 
+// A live git op holds a lock for at most GIT_EXEC_TIMEOUT_MS (then it is killed),
+// so a lock whose mtime is older than a few multiples of that cannot have a live
+// holder — it is a genuine crash-leftover, safe to remove. A fresh lock from a
+// concurrently-running replica (mtime within the timeout) is preserved so we never
+// delete a lock a live git process still holds.
+const STALE_LOCK_MIN_AGE_MS = GIT_EXEC_TIMEOUT_MS * 3; // 6 min, >> max git-op duration
+
 /** Bot identity used for engine-authored vault commits (SPEC §7.3). */
 export const BOT_AUTHOR_NAME = "Docmost Sync";
 export const BOT_AUTHOR_EMAIL = "docmost-sync@local";
@@ -345,17 +352,37 @@ export class VaultGit {
    * failure deep inside `checkout`. This is what makes re-runs converge
    * (resumability, SPEC §12).
    */
+  async isMergeInProgress(): Promise<boolean> {
+    // MERGE_HEAD exists exactly while a merge is in progress.
+    const mergeHead = await this.runRaw([
+      "rev-parse",
+      "--verify",
+      "--quiet",
+      "MERGE_HEAD",
+    ]);
+    if (mergeHead.code === 0 && mergeHead.stdout.trim().length > 0) return true;
+    // Fallback / belt-and-suspenders: any unmerged index entries also mean the
+    // working tree is mid-conflict and a checkout would refuse.
+    const unmerged = await this.runRaw(["ls-files", "-u"]);
+    return unmerged.code === 0 && unmerged.stdout.trim().length > 0;
+  }
+
   /**
    * Remove STALE git lock files left by an INTERRUPTED git operation (a hard
    * crash / OOM-kill / abrupt container stop mid `git add`/`commit`/`checkout`
    * leaves `.git/index.lock`; interrupted ref updates leave `*.lock` files). Git
    * then refuses EVERY subsequent operation ("Unable to create '…/index.lock':
    * File exists"), which WEDGES the space's sync loop indefinitely with no
-   * self-heal (bug D3-N3). The daemon holds the per-space Redis lock and is the
-   * vault's ONLY writer, so any leftover `*.lock` reaching a fresh cycle is
-   * necessarily stale (no live git process holds it) — clear them best-effort in
-   * the cycle preflight, alongside the mid-merge recovery. Missing files are a
-   * no-op (`force: true`).
+   * self-heal (bug D3-N3). We target the known/fixed set of index + ref lock
+   * files this engine itself writes (a hardcoded list, NOT a `*.lock` glob), and
+   * remove a lock ONLY when it is provably stale by mtime: a lock older than
+   * STALE_LOCK_MIN_AGE_MS cannot have a live holder (a live git op is killed
+   * after GIT_EXEC_TIMEOUT_MS), so it is a genuine crash-leftover. A FRESH lock —
+   * e.g. one a concurrently-running replica still holds during the documented
+   * multi-replica TTL-lapse window — is PRESERVED, so we never delete a lock a
+   * live git process is still using (which would corrupt the index/refs). Clear
+   * them best-effort in the cycle preflight, alongside the mid-merge recovery.
+   * Missing files are a no-op.
    */
   async clearStaleGitLocks(): Promise<void> {
     const gitDir = `${this.vaultPath}/.git`;
@@ -371,27 +398,22 @@ export class VaultGit {
       "refs/docmost/last-pushed.lock",
     ];
     await Promise.all(
-      locks.map((rel) =>
-        rm(`${gitDir}/${rel}`, { force: true }).catch(() => undefined),
-      ),
+      locks.map(async (rel) => {
+        const path = `${gitDir}/${rel}`;
+        try {
+          const stats = await stat(path);
+          // Only remove a lock old enough that no live git process can hold it.
+          // A fresh lock (mtime within the staleness window) is left in place.
+          if (Date.now() - stats.mtimeMs >= STALE_LOCK_MIN_AGE_MS) {
+            await rm(path, { force: true }).catch(() => undefined);
+          }
+        } catch {
+          // Missing lock (ENOENT) or unreadable — nothing to clear.
+        }
+      }),
     );
   }
 
-  async isMergeInProgress(): Promise<boolean> {
-    // MERGE_HEAD exists exactly while a merge is in progress.
-    const mergeHead = await this.runRaw([
-      "rev-parse",
-      "--verify",
-      "--quiet",
-      "MERGE_HEAD",
-    ]);
-    if (mergeHead.code === 0 && mergeHead.stdout.trim().length > 0) return true;
-    // Fallback / belt-and-suspenders: any unmerged index entries also mean the
-    // working tree is mid-conflict and a checkout would refuse.
-    const unmerged = await this.runRaw(["ls-files", "-u"]);
-    return unmerged.code === 0 && unmerged.stdout.trim().length > 0;
-  }
-
   /**
    * Commit the currently STAGED changes with an explicit author/committer
    * identity and the given trailers appended to the message body (SPEC §7.3

packages/git-sync/test/cycle.test.ts

@@ -133,6 +133,33 @@ describe("runCycle (composition)", () => {
     expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1);
   });
 
+  it("runs the self-heal preflight in a SAFE order: ensureRepo -> clearStaleGitLocks -> ensureMainBranch -> ensureBranch (bugs D3-N3/D3-N1)", async () => {
+    // The order is load-bearing for safety: clearStaleGitLocks MUST run AFTER
+    // ensureRepo (else it would delete ensureRepo's own transient lock) and
+    // BEFORE the checkout/diff; ensureMainBranch MUST run before the
+    // ensureBranch("docmost","main") + checkout that would otherwise throw on a
+    // missing `main`.
+    const vault = fakeVault();
+    const deps = baseDeps(vault);
+
+    const res = await runCycle(deps);
+    expect(res.ran).toBe(true);
+
+    const ensureRepoIdx = vault.order.indexOf("ensureRepo");
+    const clearStaleGitLocksIdx = vault.order.indexOf("clearStaleGitLocks");
+    const ensureMainBranchIdx = vault.order.indexOf("ensureMainBranch");
+    const ensureBranchIdx = vault.order.indexOf("ensureBranch:docmost,main");
+
+    expect(ensureRepoIdx).toBeGreaterThanOrEqual(0);
+    expect(clearStaleGitLocksIdx).toBeGreaterThanOrEqual(0);
+    expect(ensureMainBranchIdx).toBeGreaterThanOrEqual(0);
+    expect(ensureBranchIdx).toBeGreaterThanOrEqual(0);
+
+    expect(ensureRepoIdx).toBeLessThan(clearStaleGitLocksIdx);
+    expect(clearStaleGitLocksIdx).toBeLessThan(ensureMainBranchIdx);
+    expect(ensureMainBranchIdx).toBeLessThan(ensureBranchIdx);
+  });
+
   it("runs a SINGLE push planning pass (no dry-run; the delete-cap hook is gone)", async () => {
     const vault = fakeVault();
     const deps = baseDeps(vault);

packages/git-sync/test/git.test.ts

@@ -1,5 +1,5 @@
 import { execFile } from 'node:child_process';
-import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
+import { mkdir, mkdtemp, rm, writeFile, utimes } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { promisify } from 'node:util';
@@ -149,12 +149,18 @@ describe('VaultGit (integration; temp repo)', () => {
 
     // Simulate an interrupted git op: a stale index.lock left behind. Git now
     // refuses index-touching operations.
-    await writeFile(join(vault, '.git', 'index.lock'), '');
+    const lockPath = join(vault, '.git', 'index.lock');
+    await writeFile(lockPath, '');
+    // Backdate the lock beyond the staleness threshold so it is treated as a
+    // genuine crash-leftover (a live/fresh lock is preserved — see the test
+    // below); mtime gating only removes locks older than the threshold.
+    const anHourAgo = Date.now() / 1000 - 3600;
+    await utimes(lockPath, anHourAgo, anHourAgo);
     await expect(
       execFileAsync('git', ['add', '-A'], { cwd: vault }),
     ).rejects.toThrow(/index\.lock/);
 
-    // The preflight clears it (the daemon is the vault's sole writer, so it is stale).
+    // The preflight clears it (stale by mtime, no live git process holds it).
     await git.clearStaleGitLocks();
 
     // The lock is gone and git ops succeed again.
@@ -166,6 +172,26 @@ describe('VaultGit (integration; temp repo)', () => {
     await expect(git.clearStaleGitLocks()).resolves.toBeUndefined();
   });
 
+  it('clearStaleGitLocks PRESERVES a fresh index.lock (a concurrent replica may hold it) (bug D3-N3 / F1)', async () => {
+    if (!available) return;
+    const vault = await freshDir();
+    const git = new VaultGit(vault);
+    await git.ensureRepo();
+
+    // A FRESH lock (current mtime) — as a concurrently-running replica would
+    // hold mid `git add`/`commit` during the multi-replica TTL-lapse window.
+    // Removing it would corrupt the index/refs, so it MUST be preserved.
+    const lockPath = join(vault, '.git', 'index.lock');
+    await writeFile(lockPath, '');
+
+    await git.clearStaleGitLocks();
+
+    // Still there: a fresh (possibly live) lock is never deleted.
+    await expect(
+      execFileAsync('git', ['add', '-A'], { cwd: vault }),
+    ).rejects.toThrow(/index\.lock/);
+  });
+
   it('ensureMainBranch restores a deleted main from the docmost mirror (bug D3-N1)', async () => {
     if (!available) return;
     const vault = await freshDir();
@@ -192,6 +218,53 @@ describe('VaultGit (integration; temp repo)', () => {
     await expect(git.ensureMainBranch()).resolves.toBeUndefined();
   });
 
+  it('ensureMainBranch restores a deleted main from HEAD when docmost is gone too (bug D3-N1)', async () => {
+    if (!available) return;
+    const vault = await freshDir();
+    const git = new VaultGit(vault);
+    await git.ensureRepo();
+
+    // The reachable HEAD commit that main should be restored to.
+    const { stdout: headSha } = await execFileAsync(
+      'git',
+      ['rev-parse', 'HEAD'],
+      { cwd: vault },
+    );
+    const expectedSha = headSha.trim();
+
+    // Ref damage: BOTH main AND docmost are gone, only HEAD/a commit is
+    // reachable. Detach HEAD (so we can delete main), then delete both branches.
+    await execFileAsync('git', ['checkout', '--detach', 'HEAD'], { cwd: vault });
+    await git.ensureBranch('docmost', 'main');
+    await execFileAsync('git', ['branch', '-D', 'main', 'docmost'], {
+      cwd: vault,
+    });
+    expect(await git.branchExists('main')).toBe(false);
+    expect(await git.branchExists('docmost')).toBe(false);
+
+    // The preflight re-creates main from the HEAD commit.
+    await git.ensureMainBranch();
+    expect(await git.branchExists('main')).toBe(true);
+    const { stdout: restored } = await execFileAsync(
+      'git',
+      ['rev-parse', 'main'],
+      { cwd: vault },
+    );
+    expect(restored.trim()).toBe(expectedSha);
+  });
+
+  it('ensureMainBranch is a no-op on a repo with no commit at all (bug D3-N1)', async () => {
+    if (!available) return;
+    const vault = await freshDir();
+    // A bare `git init` — no commit, so no branch/HEAD to restore from.
+    await execFileAsync('git', ['init'], { cwd: vault });
+    const git = new VaultGit(vault);
+
+    // Nothing to do (ensureRepo's fresh-init path owns this case); must not throw.
+    await expect(git.ensureMainBranch()).resolves.toBeUndefined();
+    expect(await git.branchExists('main')).toBe(false);
+  });
+
   it('ensureRepo neutralizes correctness-affecting LOCAL config', async () => {
     if (!available) return;
     const vault = await freshDir();