fix(T13299): global backup lock LOWs: clock at acquisition, abandon lost locks, sweep orphaned copies

.changeset/global-backup-lock-lows.md

@@ -0,0 +1,22 @@
+---
+id: global-backup-lock-lows
+tasks: [T13299, T13293]
+kind: fix
+summary: "Global backup lock: stale clock starts at acquisition; a lost lock is abandoned, never removed; orphaned copies swept"
+---
+
+Three follow-ups to the single-flight session-end global backup (#1948 review):
+
+- The stale-window clock starts when the lock is taken, so a slow `db-heavy`
+  admission counts against it. A lock held past the window still discards
+  the copy instead of publishing it.
+- A lock that was lost, or held past its stale window, is abandoned rather
+  than released. proper-lockfile's release and its exit hook remove the lock
+  directory without checking who owns it, which could delete the lock of the
+  process that took it over. The new `acquireAbandonableLock` routes the
+  lock's file operations through a guarded fs, so after `abandon()` neither
+  path touches the directory. A directory that was still ours is already
+  stale, and the next taker reclaims it.
+- `cleo.db.<id>.tmp` copies (and their SQLite sidecars) left by a backup
+  killed mid-copy are swept from the global backup directory, under the lock,
+  once they are older than the stale window.

.changeset/t13293-global-backup-lock-stale.md

@@ -0,0 +1,24 @@
+---
+id: t13293-global-backup-lock-stale
+tasks: [T13293, T13286]
+kind: fix
+summary: the session-end global backup lock outlasts a blocking VACUUM INTO, so a long copy is never taken over by a second one
+---
+
+The single-flight lock around the session-end global backup (T13286) was stale after 60 s. Its
+`VACUUM INTO` is synchronous: while it runs, the event loop is blocked and proper-lockfile cannot
+refresh the lock. A copy longer than 60 s could therefore have its lock taken as stale by another
+session end, which then started a second full copy.
+
+**Timings.** Measured on a Mac mini (Mac17,16, Apple M5 Pro, internal APFS SSD) with synthetic
+stores: 70 MB copies in 0.29 s and 1 GB in 2.4 s. A slow or FUSE-mounted disk can be 20–50 times
+slower.
+
+**Fix.** The threshold is now `GLOBAL_BACKUP_LOCK_STALE_MS`: 10 minutes, at least three times the
+worst case, matching `EXODUS_LOCK_STALE_MS`.
+
+**Discard on compromise.** A lock reported compromised is recorded instead of thrown, and the copy
+made under it is discarded. So is a copy that outlasted the stale window: the copy blocks the event
+loop, so a compromise could surface only after it. The copy is written under a temporary name and
+published by a rename only if neither happened. A discarded copy is deleted with nothing renamed,
+listed or rotated, and a warning is logged.

packages/core/src/store/__tests__/backup-store-roundtrip.test.ts

@@ -18,16 +18,18 @@ import {
   mkdirSync,
   readdirSync,
   readFileSync,
+  utimesSync,
   writeFileSync,
 } from 'node:fs';
 import { dirname, join } from 'node:path';
 import { DatabaseSync } from 'node:sqlite';
-import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import {
   AUTO_GLOBAL_BACKUP_INTERVAL_MS,
   autoGlobalBackup,
   createBackup,
   createGlobalBackup,
+  GLOBAL_BACKUP_LOCK_STALE_MS,
   listGlobalBackups,
   listSystemBackups,
   restoreBackupById,
@@ -252,6 +254,8 @@ describe('backups restore onto the live cleo.db (T13245)', () => {
 });
 
 describe('the global store backs up and restores (T13245)', () => {
+  /** A stand-in held lock whose release and abandon are recorded. */
+  const heldLock = () => ({ release: vi.fn(async () => {}), abandon: vi.fn(async () => {}) });
   /** Row counts and checksums of the global brain and the nexus registry. */
   function globalFingerprint(file: string): Record<string, string> {
     const db = new DatabaseSync(file, { readOnly: true });
@@ -414,6 +418,96 @@ describe('the global store backs up and restores (T13245)', () => {
     }
   });
 
+  it('a lock whose holder is blocked in a long copy (no refresh for 90 s) is not stolen (T13293)', async () => {
+    const { path: gpath } = await globalDb();
+    const gdir = join(dirname(gpath), 'backups', 'sqlite');
+    mkdirSync(gdir, { recursive: true });
+    // A holder blocked in a synchronous VACUUM INTO cannot refresh the
+    // lock's mtime: the lock looks 90 s old.
+    mkdirSync(`${gdir}.lock`);
+    const old = new Date(Date.now() - 90_000);
+    utimesSync(`${gdir}.lock`, old, old);
+    expect(GLOBAL_BACKUP_LOCK_STALE_MS).toBeGreaterThanOrEqual(600_000);
+    const id = await autoGlobalBackup(new Date(), {
+      admit: async () => ({ release: async () => {} }),
+    });
+    expect(id).toBeNull();
+    expect(readdirSync(gdir).filter((f) => f.startsWith('cleo.db.auto-'))).toEqual([]);
+  });
+
+  it('a lock lost during the copy discards it: nothing published, listed or rotated (T13293)', async () => {
+    const { path: gpath } = await globalDb();
+    const gdir = join(dirname(gpath), 'backups', 'sqlite');
+    mkdirSync(gdir, { recursive: true });
+    const before = readdirSync(gdir).sort();
+    const held = heldLock();
+    const id = await autoGlobalBackup(new Date(), {
+      admit: async () => ({ release: async () => {} }),
+      // The lock is reported compromised while held (as proper-lockfile does
+      // when its refresh finds the lock taken).
+      lock: async (onCompromised) => {
+        onCompromised(new Error('lock taken as stale by another process'));
+        return held;
+      },
+    });
+    expect(id).toBeNull();
+    expect(readdirSync(gdir).sort()).toEqual(before);
+    expect(listGlobalBackups().filter((b) => b.type === 'auto')).toEqual([]);
+    // The lost lock may be another process's: abandoned, never removed (T13299).
+    expect(held.abandon).toHaveBeenCalledOnce();
+    expect(held.release).not.toHaveBeenCalled();
+  });
+
+  it('the admission wait counts against the stale window: a lock held past it discards the copy (T13299)', async () => {
+    const { path: gpath } = await globalDb();
+    const gdir = join(dirname(gpath), 'backups', 'sqlite');
+    mkdirSync(gdir, { recursive: true });
+    const held = heldLock();
+    const realNow = Date.now.bind(Date);
+    const id = await autoGlobalBackup(new Date(), {
+      lock: async () => held,
+      // The admission returns only after the whole stale window has passed.
+      admit: async () => {
+        vi.spyOn(Date, 'now').mockImplementation(() => realNow() + GLOBAL_BACKUP_LOCK_STALE_MS);
+        return { release: async () => {} };
+      },
+    });
+    vi.restoreAllMocks();
+    expect(id).toBeNull();
+    expect(readdirSync(gdir).filter((f) => f.startsWith('cleo.db.auto-'))).toEqual([]);
+    expect(held.abandon).toHaveBeenCalledOnce();
+    expect(held.release).not.toHaveBeenCalled();
+  });
+
+  it('a backup that kept its lock releases it, removing the lock directory (T13299)', async () => {
+    const { path: gpath } = await globalDb();
+    const gdir = join(dirname(gpath), 'backups', 'sqlite');
+    const id = await autoGlobalBackup(new Date(), {
+      admit: async () => ({ release: async () => {} }),
+    });
+    expect(id).not.toBeNull();
+    expect(existsSync(`${gdir}.lock`)).toBe(false);
+  });
+
+  it('copies a killed backup left behind are swept under the lock once stale; a live one is kept (T13299)', async () => {
+    const { path: gpath } = await globalDb();
+    const gdir = join(dirname(gpath), 'backups', 'sqlite');
+    mkdirSync(gdir, { recursive: true });
+    const old = new Date(Date.now() - GLOBAL_BACKUP_LOCK_STALE_MS - 60_000);
+    for (const name of ['cleo.db.auto-old.tmp', 'cleo.db.auto-old.tmp-journal', 'other.tmp']) {
+      writeFileSync(join(gdir, name), 'x');
+      utimesSync(join(gdir, name), old, old);
+    }
+    writeFileSync(join(gdir, 'cleo.db.auto-live.tmp'), 'x');
+    // A deferred admission copies nothing; the sweep already ran under the lock.
+    expect(await autoGlobalBackup(new Date(), { admit: async () => null })).toBeNull();
+    const left = readdirSync(gdir);
+    expect(left).not.toContain('cleo.db.auto-old.tmp');
+    expect(left).not.toContain('cleo.db.auto-old.tmp-journal');
+    expect(left).toContain('cleo.db.auto-live.tmp');
+    expect(left).toContain('other.tmp');
+  });
+
   it('a deferred db-heavy admission takes no global backup', async () => {
     const { path: gpath } = await globalDb();
     expect(await autoGlobalBackup(new Date(), { admit: async () => null })).toBeNull();

packages/core/src/store/__tests__/lock-abandon.test.ts

@@ -0,0 +1,50 @@
+/**
+ * T13299 — an abandoned lock never removes its directory: proper-lockfile's
+ * release removes `<file>.lock` without checking who owns it, so a lock lost
+ * to another process must be let go of, not released.
+ *
+ * @task T13299
+ */
+
+import { existsSync, mkdirSync, mkdtempSync, rmdirSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+import { acquireAbandonableLock } from '../lock.js';
+
+let dir: string;
+let file: string;
+
+beforeEach(() => {
+  dir = mkdtempSync(join(tmpdir(), 'cleo-t13299-lock-'));
+  file = join(dir, 'target');
+  writeFileSync(file, '');
+});
+
+afterEach(() => {
+  rmSync(dir, { recursive: true, force: true });
+});
+
+describe('acquireAbandonableLock (T13299)', () => {
+  it('release removes the lock directory', async () => {
+    const held = await acquireAbandonableLock(file, { retries: 0 });
+    expect(existsSync(`${file}.lock`)).toBe(true);
+    await held.release();
+    expect(existsSync(`${file}.lock`)).toBe(false);
+  });
+
+  it('abandon leaves a lock another process took meanwhile in place, and frees ours for re-acquisition', async () => {
+    const held = await acquireAbandonableLock(file, { retries: 0, onCompromised: () => {} });
+    // Another process took the lock as stale: removed ours, made its own.
+    rmdirSync(`${file}.lock`);
+    mkdirSync(`${file}.lock`);
+    await held.abandon();
+    expect(existsSync(`${file}.lock`)).toBe(true);
+    // Abandoning a second time (already released) never throws.
+    await held.abandon();
+    // This process no longer holds it: once the other holder lets go, it can be taken again.
+    rmdirSync(`${file}.lock`);
+    const again = await acquireAbandonableLock(file, { retries: 0 });
+    await again.release();
+  });
+});

packages/core/src/store/lock.ts

@@ -5,6 +5,7 @@
  * @task T4457
  */
 
+import * as fs from 'node:fs';
 import { ExitCode } from '@cleocode/contracts/exit-codes.js';
 import lockfile from 'proper-lockfile';
 import { CleoError } from '../errors.js';
@@ -53,6 +54,84 @@ export async function acquireLock(
   }
 }
 
+/**
+ * A held lock that can be released, or abandoned when it may no longer be
+ * ours (T13299).
+ */
+export interface AbandonableLock {
+  /** Release the lock: stop refreshing it and remove its directory. */
+  release: ReleaseFn;
+  /**
+   * Stop holding the lock WITHOUT removing its directory: for a lock that was
+   * lost or outlasted its stale window, which another process may have taken
+   * since. proper-lockfile's release and its exit hook remove the directory
+   * without checking who owns it; after `abandon` neither touches it. A
+   * directory that was still ours is already stale, so the next taker
+   * reclaims it. Never throws.
+   */
+  abandon: ReleaseFn;
+}
+
+/**
+ * {@link acquireLock}, returning a lock that can also be abandoned (see
+ * {@link AbandonableLock}). The lock's file operations go through node's fs,
+ * with the directory removal skipped once the lock is abandoned.
+ *
+ * @param filePath - The file to lock (the lock is `<filePath>.lock`).
+ * @param options - As for {@link acquireLock}.
+ * @returns The held lock.
+ * @throws CleoError (LOCK_TIMEOUT) when the lock cannot be acquired.
+ * @task T13299
+ */
+export async function acquireAbandonableLock(
+  filePath: string,
+  options?: { stale?: number; retries?: number; onCompromised?: (err: Error) => void },
+): Promise<AbandonableLock> {
+  let abandoned = false;
+  const guardedFs = {
+    ...fs,
+    rmdir(target: fs.PathLike, cb: (err: NodeJS.ErrnoException | null) => void): void {
+      if (abandoned) {
+        cb(null);
+        return;
+      }
+      fs.rmdir(target, cb);
+    },
+    rmdirSync(target: fs.PathLike): void {
+      if (!abandoned) fs.rmdirSync(target);
+    },
+  };
+  let release: ReleaseFn;
+  try {
+    release = await lockfile.lock(filePath, {
+      ...DEFAULT_LOCK_OPTIONS,
+      fs: guardedFs,
+      ...(options?.stale !== undefined && { stale: options.stale }),
+      ...(options?.onCompromised !== undefined && { onCompromised: options.onCompromised }),
+      ...(options?.retries !== undefined && {
+        retries: { ...DEFAULT_LOCK_OPTIONS.retries, retries: options.retries },
+      }),
+    });
+  } catch (err) {
+    // @sync-invariant none:local-only a file lock held by another local process; never replicated
+    throw new CleoError(ExitCode.LOCK_TIMEOUT, `Failed to acquire lock: ${filePath}`, {
+      fix: `Another process may be writing to this file. Wait and retry.`,
+      cause: err,
+    });
+  }
+  return {
+    release,
+    abandon: async () => {
+      abandoned = true;
+      try {
+        await release();
+      } catch {
+        // already released as compromised: nothing is held
+      }
+    },
+  };
+}
+
 /**
  * Check if a file is currently locked.
  *