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();