diff --git a/README.md b/README.md index 0a7e7a3..bc74537 100644 --- a/README.md +++ b/README.md @@ -761,9 +761,9 @@ drops it. `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 `` 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 -directory that +removes the lock when it ends, whether it succeeds or fails, unless it lost the +lock (see below); `quak backup` takes it before it opens its library, so a +refused run sends no request. 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 @@ -775,6 +775,16 @@ 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 running backup loses its lock when someone removes `backup.lock`, or when +another backup takes it over because the running one left it untouched for 10 +seconds, as a stopped or stalled process does. The backup finds this out the +next time it goes to touch the lock, which it does every 5 seconds, and then +stops before it starts another file. The original it is downloading at that +moment is still stored. It leaves `failures.json` and `backup.lock` as they are, +since another backup may be using them. `quak backup` prints +`quak: lost the lock on : ` as one line and exits 1, and +`lib.backup()` rejects with `lost the lock on : `. + A collection's directory and JSON are named after the collection, and a symlink after the file's title, both with unsafe characters replaced. When two collections would get the same name, or two symlinks in one collection the same @@ -1060,21 +1070,24 @@ photos newest first). `lib.subscribe({ onChange })` delivers a `LibraryChange` 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 with a durable failure ledger. Each file's - JSON holds its ML data and, for an image (for a live photo, its image), its - original's EXIF and XMP, with its dimensions for a JPEG only; a video's JSON - holds none of these three. 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`, `verify` (default - `false`), `onProgress`, and `lockHeld` (default `false`). See Backup layout - above for the tree it writes. + takes it, failing the same way, and returns a `BackupLock`, whose `release()` + removes the lock unless it was lost and whose `lost` holds why it was lost. + The caller passes it to the backup as `lock`, and releases it once the backup + ends. 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 with a durable failure + ledger. Each file's JSON holds its ML data and, for an image (for a live + photo, its image), its original's EXIF and XMP, with its dimensions for a JPEG + only; a video's JSON holds none of these three. 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`, `verify` + (default `false`), `onProgress`, and `lock`. See Backup layout above for the + tree it writes. Whether the backup took the lock or its caller did, once the + lock is lost the backup rejects with `lost the lock on : ` before + it starts another file. ### Request pools @@ -1134,7 +1147,7 @@ from a very old client, is stored unchecked. - `src/library/mlsearch.ts`: `MLDataAPI`, `SimilarResult` - `src/exif.ts`: `ExifTags`, `PhotoExif` - `src/library/pools.ts`: `RequestPools`, `RequestPoolsOptions`, `BoundedPool` -- `src/backup.ts`: `BackupOptions`, `BackupResult`, `BackupError` +- `src/backup.ts`: `BackupLock`, `BackupOptions`, `BackupResult`, `BackupError` - `src/client.ts`: `Client`, `LoginOptions`, `ClientSnapshot` - `src/api/client.ts`: `ApiClient`, `ApiClientOptions`, `StreamOptions` - `src/errors.ts`: `ApiError`, `TruncatedStreamError` diff --git a/TODO.md b/TODO.md index e170323..5f555ea 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,16 @@ declares one. # Completed Steps +- 2026-10-14: A backup whose lock is lost no longer dies with a stack trace + (issue 184). `lockBackupDirectory` gives proper-lockfile an `onCompromised` + that records why the lock was lost instead of throwing from its timer, and + returns the lock, which `quak backup` passes to `lib.backup()` as `lock`. The + backup then stops before it starts another file with + `lost the lock on DIR: REASON`, which `quak backup` prints as one line before + it exits 1. Releasing a lost lock leaves `backup.lock` alone. Tests in + `test/cli/backup-end-to-end.test.ts` remove the lock, or put another backup's + in its place, while a download is held open. + - 2026-10-14: An album or file quak cannot decrypt, or whose metadata is not JSON, no longer fails the refresh, so one bad record no longer stops `quak backup` from storing anything (issue 181). The refresh leaves it out, an diff --git a/src/backup.ts b/src/backup.ts index 5deae14..bd76d2f 100644 --- a/src/backup.ts +++ b/src/backup.ts @@ -1,8 +1,9 @@ // 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 +// passes one in), and fails at once when another backup of it holds the +// lock; when the lock is lost while it runs, it fails before it starts another +// file. 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 @@ -96,6 +97,14 @@ import type { Collection, EnteFile, FileMetadata } from "./model/types.js"; export type ProgressCallback = (message: string) => void; +// The lock in a backup directory, as `lockBackupDirectory` returns it. +export interface BackupLock { + // Removes `backup.lock`, unless the lock was lost. + release(): Promise; + // Why the lock was lost, once it is. + lost?: Error; +} + export interface BackupOptions { // Where the backup tree lives. `lib.backup()` defaults it to the library's // download directory; `runBackup` with none throws before any network @@ -112,10 +121,11 @@ export interface BackupOptions { // bytes do not match the content hash its metadata records. Default false. verify?: boolean; 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; + // The lock in `downloadDirectory`, when the caller took it with + // `lockBackupDirectory` and releases it itself. The backup then does not + // take the lock, and fails once this one is lost. `quak backup` takes it + // before it opens its library. + lock?: BackupLock; } export interface BackupError { @@ -510,11 +520,14 @@ 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 `lock` is held. Once +// the lock is lost, another backup may be writing to `downloadDirectory`, so +// this one fails before it starts another file. const runLockedBackup = async ( lib: BackupLibrary, opts: BackupOptions, downloadDirectory: string, + lock: BackupLock, ): Promise => { const includeOriginals = opts.includeOriginals ?? true; const includeThumbnails = opts.includeThumbnails ?? false; @@ -524,6 +537,7 @@ const runLockedBackup = async ( log("Refreshing library..."); await lib.refresh(); + if (lock.lost) throw lock.lost; const collectionsDir = join(downloadDirectory, "collections"); const thumbnailsDir = join(downloadDirectory, "thumbnails"); @@ -642,6 +656,7 @@ const runLockedBackup = async ( // that cannot be read is recorded as failed and left where it is. if (includeOriginals) { for (const [fileID, file] of distinct) { + if (lock.lost) throw lock.lost; let stored = storedAtSavePath(downloadDirectory, file); let mismatch = false; if (stored !== undefined && verify) { @@ -721,6 +736,7 @@ const runLockedBackup = async ( if (includeThumbnails) { for (const [fileID, file] of distinct) { + if (lock.lost) throw lock.lost; const dest = join(thumbnailsDir, `${fileID}.jpg`); if (isPresent(dest)) continue; try { @@ -753,6 +769,7 @@ const runLockedBackup = async ( log(`FAILED ML data: ${mlDataError}`); } for (const file of distinct.values()) { + if (lock.lost) throw lock.lost; const stored = storedAtSavePath(downloadDirectory, file); if (stored === undefined) continue; const path = withExtension( @@ -796,6 +813,7 @@ const runLockedBackup = async ( const albumDirNames = new Map( allCollections.map((c, i) => [c.id, dirNames[i]!]), ); + if (lock.lost) throw lock.lost; try { removeStaleAlbumDirs( collectionsDir, @@ -807,6 +825,7 @@ const runLockedBackup = async ( } for (const c of collections) { + if (lock.lost) throw lock.lost; const colDirName = albumDirNames.get(c.id)!; const colDir = join(collectionsDir, colDirName); mkdirSync(colDir, { recursive: true }); @@ -864,7 +883,9 @@ const runLockedBackup = async ( // its failure resolved; a file gone from the library (deleted) or outside // this run's scope is not something this run can resolve, so keeping its // stale entry would keep the exit code non-zero forever — a single - // since-deleted photo would fail every future scheduled backup. + // since-deleted photo would fail every future scheduled backup. A backup + // whose lock was lost leaves the ledger as it is. + if (lock.lost) throw lock.lost; for (const fileID of [...ledger.keys()]) { if (!failedThisRun.has(fileID)) ledger.delete(fileID); } @@ -885,18 +906,36 @@ 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. +// returns 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. +// +// proper-lockfile finds the lock lost when `backup.lock` is gone or is another +// backup's, and calls `onCompromised` from a timer, where throwing would end +// the process. The `onCompromised` given here sets the lock's `lost` instead, +// and the backup holding the lock stops before it starts another file (see +// `runLockedBackup`). Releasing a lost lock leaves `backup.lock` alone, since +// it may be another backup's. export const lockBackupDirectory = async ( downloadDirectory: string, -): Promise<() => Promise> => { +): Promise => { mkdirSync(downloadDirectory, { recursive: true }); + let release: () => Promise; + const lock: BackupLock = { + release: async () => { + if (lock.lost === undefined) await release(); + }, + }; try { - return await lockfile.lock(downloadDirectory, { + release = await lockfile.lock(downloadDirectory, { lockfilePath: join(downloadDirectory, "backup.lock"), + onCompromised: (err) => { + lock.lost = new Error( + `lost the lock on ${downloadDirectory}: ${err.message}`, + ); + }, }); } catch (err) { if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err; @@ -905,10 +944,11 @@ export const lockBackupDirectory = async ( { code: "ELOCKED" }, ); } + return lock; }; // Runs the backup holding the lock, which it takes and releases itself unless -// the caller already holds it (`opts.lockHeld`). +// the caller passes the lock it took (`opts.lock`). export const runBackup = async ( lib: BackupLibrary, opts: BackupOptions, @@ -920,13 +960,13 @@ export const runBackup = async ( "open the library with one)", ); } - if (opts.lockHeld) { - return runLockedBackup(lib, opts, downloadDirectory); + if (opts.lock) { + return runLockedBackup(lib, opts, downloadDirectory, opts.lock); } - const release = await lockBackupDirectory(downloadDirectory); + const lock = await lockBackupDirectory(downloadDirectory); try { - return await runLockedBackup(lib, opts, downloadDirectory); + return await runLockedBackup(lib, opts, downloadDirectory, lock); } finally { - await release(); + await lock.release(); } }; diff --git a/src/cli-commands.ts b/src/cli-commands.ts index 076d1ef..7a0c505 100644 --- a/src/cli-commands.ts +++ b/src/cli-commands.ts @@ -20,7 +20,7 @@ import { type ClientSnapshot, type LoginOptions, } from "./client.js"; -import { lockBackupDirectory } from "./backup.js"; +import { lockBackupDirectory, type BackupLock } from "./backup.js"; import { init } from "./crypto/index.js"; import { defaultCacheDirectory, @@ -416,9 +416,9 @@ export const backupCommand = async ( 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; + let lock: BackupLock; try { - release = await lockBackupDirectory(dir); + lock = await lockBackupDirectory(dir); } catch (err) { if ((err as NodeJS.ErrnoException).code !== "ELOCKED") throw err; ctx.stderr.write(`quak: ${(err as Error).message}\n`); @@ -440,7 +440,7 @@ export const backupCommand = async ( try { const result = await lib.backup({ downloadDirectory: dir, - lockHeld: true, + lock, verify: opts.verify, onProgress: (msg) => { if (!opts.json) ctx.stderr.write(msg + "\n"); @@ -477,7 +477,7 @@ export const backupCommand = async ( await lib.close(); } } finally { - await release(); + await lock.release(); } }; diff --git a/src/index.ts b/src/index.ts index 1328893..686dbef 100644 --- a/src/index.ts +++ b/src/index.ts @@ -69,6 +69,7 @@ export { type EnsureEvent, lockBackupDirectory, runBackup, + type BackupLock, type BackupOptions, type BackupResult, type BackupError, diff --git a/src/library/index.ts b/src/library/index.ts index 443f4d4..eb36410 100644 --- a/src/library/index.ts +++ b/src/library/index.ts @@ -96,6 +96,7 @@ import { runBackup, type BackupOptions, type BackupResult } from "../backup.js"; export { lockBackupDirectory, runBackup, + type BackupLock, type BackupOptions, type BackupResult, type BackupError, @@ -570,9 +571,10 @@ 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, unless the + // caller passes the lock it took as `opts.lock`, failing at once while + // another backup of it runs, and before it starts another file once the + // lock is lost (see `lockBackupDirectory`). 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 diff --git a/test/cli/backup-end-to-end.test.ts b/test/cli/backup-end-to-end.test.ts index 123c00a..886a1fc 100644 --- a/test/cli/backup-end-to-end.test.ts +++ b/test/cli/backup-end-to-end.test.ts @@ -9,7 +9,8 @@ * data this file encrypts with libsodium, as Ente's clients encrypt it. So * quak's own code fetches each original, decrypts it, checks it against the * hash its uploader recorded and unpacks a live photo, and quak's own retries - * repeat a failed request. + * repeat a failed request. Two tests run `lib.backup()` instead, as a program + * using quak as a library does. * * The account, user 42, has three albums: * @@ -24,6 +25,7 @@ import { existsSync, lstatSync, + mkdirSync, mkdtempSync, readdirSync, readFileSync, @@ -36,7 +38,16 @@ import { join } from "node:path"; import { Writable } from "node:stream"; import { gzipSync } from "node:zlib"; import sodium from "libsodium-wrappers-sumo"; -import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; import { backupCommand, @@ -47,6 +58,7 @@ import { run } from "../../src/cli-run.js"; import { loadSession } from "../../src/cli-session.js"; import { Client } from "../../src/client.js"; import { init, toBase64 } from "../../src/crypto/index.js"; +import { Library, lockBackupDirectory } from "../../src/library/index.js"; import { UNATTENDED_RETRY_OPTIONS } from "../../src/retry.js"; import { buildAccount, @@ -633,3 +645,188 @@ describe("quak backup", () => { expect(stderr).toContain(" Verified: 2\n Mismatched: 1\n"); }); }); + +// `backup.lock` is a directory that proper-lockfile touches every 5 seconds, +// from a timer, while the backup runs. Each time, it first looks at the lock +// on disk. The lock is lost when it is gone, or when another backup has made +// its own in its place, as one does with a lock nobody touched for 10 +// seconds. Another backup may then be writing to the directory, so the backup +// stops before it starts another file. +// +// The fake server holds one file's download open until the test lets it go, +// so the lock can be lost while the backup runs: beach.jpg's, the first file +// backed up, unless the test names another. Only `setTimeout` and +// `clearTimeout` are faked, so a test can fire proper-lockfile's timer without +// waiting 5 seconds. +describe("a backup whose lock is lost", () => { + let dir: string; + let lockPath: string; + let downloads: number[]; + let heldFile: number; + let heldRequested: Promise; + let releaseHeld: () => void; + let fetch: typeof globalThis.fetch; + + beforeEach(() => { + dir = mkdtempSync(join(root, "backup-")); + lockPath = join(dir, "backup.lock"); + downloads = []; + heldFile = BEACH.id; + let requested!: () => void; + heldRequested = new Promise((resolve) => { + requested = resolve; + }); + const released = new Promise((resolve) => { + releaseHeld = resolve; + }); + const recording = recordingDownloads(downloads); + fetch = async (input, init) => { + if (downloadOf(input) === heldFile) { + requested(); + await released; + } + return recording(input, init); + }; + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + // Fire proper-lockfile's timer, and wait for the look at the lock on disk + // that it starts to finish: Node lists a file system call in progress as + // an `FSReqCallback`. + const fireLockTimer = async (): Promise => { + vi.advanceTimersByTime(5_000); + while (process.getActiveResourcesInfo().includes("FSReqCallback")) { + await new Promise((resolve) => setImmediate(resolve)); + } + }; + + // beach.jpg, whose download was under way, is stored; no other file is + // started. `quak backup` prints why as one line, with no stack trace, and + // exits 1. + it("stops quak backup before its next file when backup.lock is removed, and exits 1", async () => { + const result = backup(dir, {}, fetch); + await heldRequested; + rmSync(lockPath, { recursive: true }); + await fireLockTimer(); + releaseHeld(); + const { code, stderr } = await result; + + expect(code).toBe(1); + expect(stderr).toBe( + "Starting backup...\n" + + "Refreshing library...\n" + + "Fetching original beach.jpg (100)...\n" + + `quak: lost the lock on ${dir}: ENOENT: no such file or directory, stat '${lockPath}'\n`, + ); + expect(downloads).toEqual([BEACH.id]); + expect(readFileSync(join(dir, DAY, "2024-03-01.100.jpg"))).toEqual( + Buffer.from(JPEG_WITH_EXIF), + ); + }); + + // Here the lock is lost while toast.mp4, the last original, downloads. + // All three originals are stored, beside what the backup wrote before it + // started them, but no file's JSON and no album is written. + it("stops quak backup before any file's JSON when the lock is lost during the last original", async () => { + heldFile = TOAST_VIDEO.id; + const result = backup(dir, {}, fetch); + await heldRequested; + rmSync(lockPath, { recursive: true }); + await fireLockTimer(); + releaseHeld(); + const { code, stderr } = await result; + + expect(code).toBe(1); + expect(stderr).toContain(`quak: lost the lock on ${dir}: `); + expect(listTree(dir)).toEqual([ + "2024", + "2024/2024-03", + "2024/2024-03/2024-03-01", + "2024/2024-03/2024-03-01/2024-03-01.100.jpg", + "2024/2024-03/2024-03-01/2024-03-01.200.mp4", + "2024/2024-03/2024-03-01/2024-03-01.300.heic", + "2024/2024-03/2024-03-01/2024-03-01.300.livephoto.json", + "2024/2024-03/2024-03-01/2024-03-01.300.mov", + "account.json", + "collections", + ]); + }); + + // Nothing touches the lock but quak, which finds it as it left it each + // time it looks, so the backup runs to the end and removes its lock. + it("lets quak backup run to the end while its lock stays", async () => { + const result = backup(dir, {}, fetch); + await heldRequested; + await fireLockTimer(); + await fireLockTimer(); + releaseHeld(); + const { code } = await result; + + expect(code).toBe(0); + expect(downloads.sort()).toEqual([100, 200, 300]); + expect(existsSync(lockPath)).toBe(false); + }); + + // The library a program using quak opens to back `dir` up. + const openLibrary = () => + Library.open({ + client: loadSession(join(sessionDir, "session.json"), { fetch })!, + cacheDirectory: `${dir}-cache`, + downloadDirectory: dir, + refreshIntervalSeconds: Infinity, + precacheThumbnails: false, + precacheOriginals: false, + }); + + // A program using quak as a library sees the same: `lib.backup()` rejects + // with the reason. Here another backup has made its own lock in place of + // quak's, and that lock is left where it is. + it("rejects lib.backup() when another backup has taken the lock over", async () => { + const lib = await openLibrary(); + try { + const result = lib.backup(); + await heldRequested; + rmSync(lockPath, { recursive: true }); + mkdirSync(lockPath); + await fireLockTimer(); + releaseHeld(); + + await expect(result).rejects.toThrow( + `lost the lock on ${dir}: Unable to update lock within the stale threshold`, + ); + expect(downloads).toEqual([BEACH.id]); + expect(existsSync(lockPath)).toBe(true); + } finally { + await lib.close(); + } + }); + + // A program can take the lock before it opens its library, as `quak + // backup` does: `lockBackupDirectory` returns the lock, which the program + // passes to `lib.backup()` and releases once the backup ends. The backup + // checks it as it checks a lock it took, and the lock's `lost` says why it + // was lost. + it("rejects lib.backup() given its caller's lock once that lock is lost", async () => { + const lock = await lockBackupDirectory(dir); + const lib = await openLibrary(); + try { + const result = lib.backup({ lock }); + await heldRequested; + rmSync(lockPath, { recursive: true }); + await fireLockTimer(); + releaseHeld(); + + const lost = `lost the lock on ${dir}: ENOENT: no such file or directory, stat '${lockPath}'`; + await expect(result).rejects.toThrow(lost); + expect(lock.lost?.message).toBe(lost); + expect(downloads).toEqual([BEACH.id]); + } finally { + await lib.close(); + await lock.release(); + } + }); +});