From 3f7d96e09d01f78c9132dfca473c56246e863be8 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 3 Jul 2026 08:19:04 +0300 Subject: [PATCH] fix(git-sync): gate stale-lock removal by mtime + cover preflight self-heals (#119 F1-F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- packages/git-sync/src/engine/cycle.ts | 7 ++- packages/git-sync/src/engine/git.ts | 70 ++++++++++++++++-------- packages/git-sync/test/cycle.test.ts | 27 +++++++++ packages/git-sync/test/git.test.ts | 79 ++++++++++++++++++++++++++- 4 files changed, 153 insertions(+), 30 deletions(-) diff --git a/packages/git-sync/src/engine/cycle.ts b/packages/git-sync/src/engine/cycle.ts index c60068fc..04fd8f3c 100644 --- a/packages/git-sync/src/engine/cycle.ts +++ b/packages/git-sync/src/engine/cycle.ts @@ -120,9 +120,10 @@ export async function runCycle(deps: RunCycleDeps): Promise { // 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 diff --git a/packages/git-sync/src/engine/git.ts b/packages/git-sync/src/engine/git.ts index 105dc661..4f1b62de 100644 --- a/packages/git-sync/src/engine/git.ts +++ b/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 { + // 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 { 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 { - // 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 diff --git a/packages/git-sync/test/cycle.test.ts b/packages/git-sync/test/cycle.test.ts index 5706905a..160abff4 100644 --- a/packages/git-sync/test/cycle.test.ts +++ b/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); diff --git a/packages/git-sync/test/git.test.ts b/packages/git-sync/test/git.test.ts index e4b7b918..09da7938 100644 --- a/packages/git-sync/test/git.test.ts +++ b/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();