quak backup refuses to run while another backup of the same directory runs, exit 2 (closes #169)

lib.backup() takes a lock, backup.lock in its download directory, made
with proper-lockfile, before its refresh, and removes it when it ends. A
second backup of the directory fails at once with an error naming it;
quak backup prints it as one line and exits 2. A lock untouched for 10
seconds, left by a killed run, is taken over.

Deviation: yarn.lock was regenerated by yarn add in the pinned node image.
Judgement call: a run failing at its refresh leaves the directory, empty.
Judgement call: a lock removed mid-run stops that run with an uncaught
error, the library's default.

Model: opus-5-5

README.md

@@ -669,18 +669,17 @@ library's `lib.backup({ includeThumbnails: true })` also writes
 `backup.lock` keeps two backups of the same directory from running at once, such
 as a cron run that starts while the previous one is still going. A backup
 creates `<dir>` if it is missing and takes the lock before its refresh, and
-removes the lock when it ends, whether it succeeds or fails; `quak backup` takes
-it before it opens its library, so a refused run sends no request. The lock is a
+removes the lock when it ends, whether it succeeds or fails. The lock is a
 directory that
 [proper-lockfile](https://github.com/moxystudio/node-proper-lockfile) creates
 and keeps touching while the backup runs. A second backup of the directory, from
 another process or the same one, fails at once: `quak backup` prints
-`quak: another backup of <dir> is running` and exits with status 2. A run
-stopped with Ctrl-C or `kill` removes the lock as it exits. Only a run that
-cannot, such as one killed with SIGKILL or cut off by a crash or power loss,
-leaves it behind; once it has gone 10 seconds untouched, the next run takes it
-over, so nobody has to remove it. The lock is outside the date folders and
-`collections/`, so it is never taken for an original, and the removal of old
+`quak: another backup of <dir> is running` and exits with status 2. It opens its
+library before the backup takes the lock, so a refused run still waits for the
+refresh that opening the library starts before it exits. A run that is killed
+leaves the lock behind; once it has gone 10 seconds untouched, the next run
+takes it over, so nobody has to remove it. The lock is outside the date folders
+and `collections/`, so it is never taken for an original, and the removal of old
 album directories never touches it.
 
 A collection's directory and JSON are named after the collection, and a symlink
@@ -932,21 +931,17 @@ photos newest first). `lib.subscribe({ onChange })` delivers a `LibraryChange`
   a query vector the caller produced elsewhere.
 - `await lib.backup(opts?)` → `BackupResult`. It takes the lock in the download
   directory, and fails at once with an error whose `code` is `ELOCKED` while
-  another backup of it runs. A caller can instead take the lock itself, before
-  it opens its library, as `quak backup` does: `await lockBackupDirectory(dir)`
-  takes it, failing the same way, and returns the function that releases it, and
-  the caller passes `lockHeld: true` to the backup. It waits for a refresh as
-  `fresh()` does, puts every in-scope original not already at its save path
-  there as `photo.download()` does (and, with `includeThumbnails`, fetches
-  thumbnails) through the content cache, waits for an ML data fetch, and
-  rebuilds the on-disk backup tree, each file's JSON with its ML data and its
-  original's EXIF, XMP and dimensions, with a durable failure ledger. A fetched
-  original is written straight to its save path and not into the cache, which
-  then counts it as present; one the cache already held is copied from there.
-  `BackupOptions`: `downloadDirectory` (falls back to the library's),
-  `includeOriginals` (default `true`), `includeThumbnails` (default `false`),
-  `onlyAlbumNames`, `onProgress`, and `lockHeld` (default `false`). See Backup
-  layout above for the tree it writes.
+  another backup of it runs. It waits for a refresh as `fresh()` does, puts
+  every in-scope original not already at its save path there as
+  `photo.download()` does (and, with `includeThumbnails`, fetches thumbnails)
+  through the content cache, waits for an ML data fetch, and rebuilds the
+  on-disk backup tree, each file's JSON with its ML data and its original's
+  EXIF, XMP and dimensions, with a durable failure ledger. A fetched original is
+  written straight to its save path and not into the cache, which then counts it
+  as present; one the cache already held is copied from there. `BackupOptions`:
+  `downloadDirectory` (falls back to the library's), `includeOriginals` (default
+  `true`), `includeThumbnails` (default `false`), `onlyAlbumNames`, and
+  `onProgress`. See Backup layout above for the tree it writes.
 
 ### Request pools
 

TODO.md

@@ -29,11 +29,10 @@ declares one.
   `lib.backup()` takes a lock, `backup.lock` in its download directory, made
   with `proper-lockfile`, before its refresh, and removes it when it ends,
   whether it succeeds or fails. A second backup of the directory, from another
-  process or the same one, fails at once with an error naming the directory.
-  `quak backup` takes the lock before it opens its library, so a refused run
-  sends no request; it prints the error as one line and exits 2. A lock that has
-  gone 10 seconds untouched, left by a run that could not remove it, is taken
-  over by the next run.
+  process or the same one, fails at once with an error naming the directory;
+  `quak backup` prints it as one line and exits 2. A lock that has gone 10
+  seconds untouched, left by a run that was killed, is taken over by the next
+  run.
 
 - 2026-10-06: `quak backup` writes each original's EXIF, XMP and dimensions into
   the file's JSON as `imageMetadata`, what `backup-metadata --exif` records

src/backup.ts

@@ -1,13 +1,12 @@
 // The backup command, rebuilt on the library API (issue #51).
 //
-// `lib.backup()` takes the lock in `downloadDirectory` (unless its caller
-// already holds it), and fails at once when another backup of it holds the
-// lock. It waits for a completed refresh of the library (a failed one fails the
-// backup before any file is touched), then, for every file in scope, puts its
-// original at its save path under `downloadDirectory`, as `Photo.download()`
-// does, waits for an ML data fetch, and rebuilds the derived views (per-file
-// sidecars, per-collection symlink trees, per-collection JSON) from the model.
-// The on-disk layout:
+// `lib.backup()` takes the lock in `downloadDirectory`, and fails at once when
+// another backup of it holds the lock. It waits for a completed refresh of the
+// library (a failed one fails the backup before any file is touched), then, for
+// every file in scope, puts its original at its save path under
+// `downloadDirectory`, as `Photo.download()` does, waits for an ML data fetch,
+// and rebuilds the derived views (per-file sidecars, per-collection symlink
+// trees, per-collection JSON) from the model. The on-disk layout:
 //
 //     <downloadDirectory>/
 //         YYYY/YYYY-MM/YYYY-MM-DD/
@@ -93,10 +92,6 @@ export interface BackupOptions {
     // Restrict the backup to albums with these names; others are left untouched.
     onlyAlbumNames?: string[];
     onProgress?: ProgressCallback;
-    // The caller already holds the lock in `downloadDirectory`, taken with
-    // `lockBackupDirectory`, and releases it itself, so the backup does not
-    // take it. `quak backup` takes it before it opens its library.
-    lockHeld?: boolean;
 }
 
 export interface BackupError {
@@ -458,7 +453,7 @@ const writeAlbumJSON = (
     writeFileSync(path, JSON.stringify(album, null, 2));
 };
 
-// The backup itself, which `runBackup` below runs while the lock is held.
+// The backup itself, which `runBackup` below runs while it holds the lock.
 const runLockedBackup = async (
     lib: BackupLibrary,
     opts: BackupOptions,
@@ -740,32 +735,11 @@ const runLockedBackup = async (
     };
 };
 
-// Only one backup of a directory runs at a time, in this process or another.
-// Creates `downloadDirectory` if it is missing, takes the lock in it and
-// returns the function that releases it; while another backup holds the lock,
-// fails at once with an error whose `code` is `ELOCKED`. The lock is the
-// directory `backup.lock`, whose modification time proper-lockfile keeps
-// current while it is held. One it has not touched for 10 seconds was left by
-// a run that could not remove it, and is taken over.
-export const lockBackupDirectory = async (
-    downloadDirectory: string,
-): Promise<() => Promise<void>> => {
-    mkdirSync(downloadDirectory, { recursive: true });
-    try {
-        return await lockfile.lock(downloadDirectory, {
-            lockfilePath: join(downloadDirectory, "backup.lock"),
-        });
-    } catch (err) {
-        if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err;
-        throw Object.assign(
-            new Error(`another backup of ${downloadDirectory} is running`),
-            { code: "ELOCKED" },
-        );
-    }
-};
-
-// Runs the backup holding the lock, which it takes and releases itself unless
-// the caller already holds it (`opts.lockHeld`).
+// Only one backup of a directory runs at a time, in this process or another:
+// a second one fails at once with an error whose `code` is `ELOCKED`. The lock
+// is the directory `backup.lock`, whose modification time proper-lockfile
+// keeps current while the backup runs. One it has not touched for 10 seconds
+// was left by a run that was killed, and is taken over.
 export const runBackup = async (
     lib: BackupLibrary,
     opts: BackupOptions,
@@ -777,10 +751,19 @@ export const runBackup = async (
                 "open the library with one)",
         );
     }
-    if (opts.lockHeld) {
-        return runLockedBackup(lib, opts, downloadDirectory);
+    mkdirSync(downloadDirectory, { recursive: true });
+    let release: () => Promise<void>;
+    try {
+        release = await lockfile.lock(downloadDirectory, {
+            lockfilePath: join(downloadDirectory, "backup.lock"),
+        });
+    } catch (err) {
+        if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err;
+        throw Object.assign(
+            new Error(`another backup of ${downloadDirectory} is running`),
+            { code: "ELOCKED" },
+        );
     }
-    const release = await lockBackupDirectory(downloadDirectory);
     try {
         return await runLockedBackup(lib, opts, downloadDirectory);
     } finally {

src/cli-commands.ts

@@ -20,9 +20,9 @@ import {
     type ClientSnapshot,
     type LoginOptions,
 } from "./client.js";
-import { lockBackupDirectory } from "./backup.js";
 import { init } from "./crypto/index.js";
 import {
+    type BackupResult,
     defaultCacheDirectory,
     Library,
     type LibraryClient,
@@ -409,59 +409,52 @@ export const backupCommand = async (
     if (!client) return 3;
 
     ctx.stderr.write("Starting backup...\n");
-    // The lock is taken before the library opens and starts its refresh, so a
-    // run refused while another backup of `dir` runs sends no request.
-    let release: () => Promise<void>;
+    // The precache is off: the backup fetches what it needs, and must not
+    // also fill the cache with every thumbnail and the recent originals.
+    const lib = await Library.open({
+        client,
+        downloadDirectory: dir,
+        cacheDirectory: ctx.cacheDir,
+        precacheThumbnails: false,
+        precacheOriginals: false,
+    });
     try {
-        release = await lockBackupDirectory(dir);
-    } catch (err) {
-        if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err;
-        ctx.stderr.write(`quak: ${(err as Error).message}\n`);
-        return 2;
-    }
-    try {
-        // The precache is off: the backup fetches what it needs, and must not
-        // also fill the cache with every thumbnail and the recent originals.
-        const lib = await Library.open({
-            client,
-            downloadDirectory: dir,
-            cacheDirectory: ctx.cacheDir,
-            precacheThumbnails: false,
-            precacheOriginals: false,
-        });
+        let result: BackupResult;
         try {
-            const result = await lib.backup({
+            result = await lib.backup({
                 downloadDirectory: dir,
-                lockHeld: true,
                 onProgress: (msg) => {
                     if (!opts.json) ctx.stderr.write(msg + "\n");
                 },
             });
+        } catch (err) {
+            // Another backup of `dir` is running and holds its lock.
+            if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err;
+            ctx.stderr.write(`quak: ${(err as Error).message}\n`);
+            return 2;
+        }
 
-            if (opts.json) {
-                ctx.stdout.write(JSON.stringify(result, null, 2) + "\n");
-            } else {
-                ctx.stderr.write("\n--- Backup complete ---\n");
-                ctx.stderr.write(`  Total files: ${result.totalFiles}\n`);
-                ctx.stderr.write(`  Downloaded:  ${result.downloaded}\n`);
-                ctx.stderr.write(`  Skipped:     ${result.skipped}\n`);
-                ctx.stderr.write(`  Failed:      ${result.failed}\n`);
-                if (result.errors.length > 0) {
-                    ctx.stderr.write("\nFailed files:\n");
-                    for (const e of result.errors) {
-                        ctx.stderr.write(
-                            `  [${e.collection}] ${e.title} (id ${e.fileID}): ${e.error}\n`,
-                        );
-                    }
+        if (opts.json) {
+            ctx.stdout.write(JSON.stringify(result, null, 2) + "\n");
+        } else {
+            ctx.stderr.write("\n--- Backup complete ---\n");
+            ctx.stderr.write(`  Total files: ${result.totalFiles}\n`);
+            ctx.stderr.write(`  Downloaded:  ${result.downloaded}\n`);
+            ctx.stderr.write(`  Skipped:     ${result.skipped}\n`);
+            ctx.stderr.write(`  Failed:      ${result.failed}\n`);
+            if (result.errors.length > 0) {
+                ctx.stderr.write("\nFailed files:\n");
+                for (const e of result.errors) {
+                    ctx.stderr.write(
+                        `  [${e.collection}] ${e.title} (id ${e.fileID}): ${e.error}\n`,
+                    );
                 }
             }
-
-            return result.failed > 0 ? 1 : 0;
-        } finally {
-            await lib.close();
         }
+
+        return result.failed > 0 ? 1 : 0;
     } finally {
-        await release();
+        await lib.close();
     }
 };
 

src/index.ts

@@ -67,7 +67,6 @@ export {
     type EnsureOptions,
     type EnsureResult,
     type EnsureEvent,
-    lockBackupDirectory,
     runBackup,
     type BackupOptions,
     type BackupResult,

src/library/index.ts

@@ -94,7 +94,6 @@ import type { Collection, EnteFile } from "../model/types.js";
 import { runBackup, type BackupOptions, type BackupResult } from "../backup.js";
 
 export {
-    lockBackupDirectory,
     runBackup,
     type BackupOptions,
     type BackupResult,
@@ -567,9 +566,8 @@ export class Library {
 
     // Back up every in-scope file to `opts.downloadDirectory`, or else the
     // library's, each original at its save path, with a durable failure
-    // ledger (issue #51). Takes the lock in that directory first, unless
-    // `opts.lockHeld` says the caller holds it, failing at once while another
-    // backup of it runs (see `runBackup`). Waits for a
+    // ledger (issue #51). Takes the lock in that directory first, failing at
+    // once while another backup of it runs (see `runBackup`). Waits for a
     // completed refresh, as `fresh()` does, joining one already running, and
     // rejects before touching any file but the lock when it fails. Then puts
     // pending originals at their save paths as `Photo.download()` does (and

test/cli/commands.test.ts

@@ -704,7 +704,6 @@ describe("backup", () => {
                 "  Failed:      0\n",
         );
         expect(stdout.text).toBe("");
-        expect(existsSync(join(dir, "backup.lock"))).toBe(false);
     });
 
     // The backup opens its library with the precache off: it fetches the
@@ -828,7 +827,7 @@ describe("backup", () => {
         expect(readdirSync(dir)).toEqual([]);
     });
 
-    it("exits 2 with one line naming the directory, sending no request, while another backup of it runs", async () => {
+    it("exits 2 with one line naming the directory while another backup of it runs", async () => {
         const dir = join(root, "backup");
         // The lock another backup holds. Its modification time is set an hour
         // ahead, so it stays current however long this test takes.
@@ -836,27 +835,12 @@ describe("backup", () => {
         mkdirSync(lock, { recursive: true });
         const hourAhead = new Date(Date.now() + 3_600_000);
         utimesSync(lock, hourAhead, hourAhead);
-        // A client whose refresh never finishes, so a run that started one
-        // before it exits would never return.
-        let requests = 0;
-        const never = (): Promise<never> => {
-            requests++;
-            return new Promise(() => {});
-        };
-        const client = {
-            ...fakeClient(),
-            collectionsSince: never,
-            filesSince: never,
-        } as unknown as Client;
 
-        expect(await backupCommand(context(client), dir, {})).toBe(2);
-        expect(requests).toBe(0);
+        expect(await backupCommand(context(), dir, {})).toBe(2);
         expect(stderr.text).toBe(
             `Starting backup...\nquak: another backup of ${dir} is running\n`,
         );
         expect(readdirSync(dir)).toEqual(["backup.lock"]);
-        // Opening the library would have made its cache directory.
-        expect(existsSync(join(root, "cache"))).toBe(false);
     });
 });