From 530ceeaf76acd5a76597d3b884f9cda86c4fdde9 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 05:45:00 +0000 Subject: [PATCH] Make backup wait for the server refresh and fail when it fails (closes #99) lib.backup() refreshed through the background loop's refresh, which returns at once when one is already running and swallows a failure, so a backup could run on the previous file list, or on an empty cache, and exit 0. It now uses the refresh fresh() uses: it joins a running refresh or starts one, and rejects before touching any file when it fails. The CLI's error wrapper prints that as one line and exits 1. Model: opus-5-5 --- TODO.md | 7 ++++ src/backup.ts | 9 ++-- src/library/index.ts | 14 ++++--- test/cli/backup.test.ts | 87 +++++++++++++++++++++++++++++++++++++++ test/cli/commands.test.ts | 30 ++++++++++++++ 5 files changed, 137 insertions(+), 10 deletions(-) diff --git a/TODO.md b/TODO.md index 04d5c47..f1b3a4c 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ Tag v1.0.0. # Completed Steps +- 2026-09-23: `quak backup` waits for the server refresh and fails when it fails + (issue 99). `lib.backup()` joins a refresh already running or starts one, as + `fresh()` does, and rejects before touching any file when it fails, leaving + `failures.json` as it was, so `quak backup` prints the error as one line and + exits 1 instead of backing up the previous run's file list, or nothing, and + exiting 0. + - 2026-09-23: CLI errors print a message instead of a stack trace (issue 102). An error a command throws is printed as one `quak: MESSAGE` line on stderr and the CLI exits 1 once output has drained. The wrapper that does this moved from diff --git a/src/backup.ts b/src/backup.ts index 5e90f8b..e20dce1 100644 --- a/src/backup.ts +++ b/src/backup.ts @@ -1,9 +1,10 @@ // The backup command, rebuilt on the library API (issue #51). // -// `lib.backup()` refreshes the library, then, for every file in scope, gets its -// original bytes onto disk under `downloadDirectory` and rebuilds the derived -// views (per-file sidecars, per-collection symlink trees, per-collection JSON) -// from the model. The on-disk layout is the historical one, unchanged: +// `lib.backup()` 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, +// gets its original bytes onto disk under `downloadDirectory` and rebuilds the +// derived views (per-file sidecars, per-collection symlink trees, +// per-collection JSON) from the model. The on-disk layout is the historical one, unchanged: // // / // originals/. the decrypted bytes diff --git a/src/library/index.ts b/src/library/index.ts index d8aa998..f6561f4 100644 --- a/src/library/index.ts +++ b/src/library/index.ts @@ -538,11 +538,13 @@ export class Library { } // Back up every in-scope file to `downloadDirectory` in the historical - // on-disk layout, with a durable failure ledger (issue #51). Refreshes - // first, fetches pending originals (and optional thumbnails) through the - // content cache and pools, then rebuilds the derived symlink/JSON views - // from the model. Throws before any network work when no download directory - // is available or no content cache backs the originals it must fetch. + // on-disk layout, with a durable failure ledger (issue #51). Waits for a + // completed refresh first, as `fresh()` does, joining one already running, + // and rejects before touching any file when it fails. Then fetches pending + // originals (and optional thumbnails) through the content cache and pools, + // and rebuilds the derived symlink/JSON views from the model. Throws before + // any network work when no download directory is available or no content + // cache backs the originals it must fetch. backup(opts?: BackupOptions): Promise { const downloadDirectory = opts?.downloadDirectory ?? this.downloadDirectory; @@ -566,7 +568,7 @@ export class Library { const cache = this.cache; return runBackup( { - refresh: () => this.runRefresh(), + refresh: () => this.refreshNow(), listCollections: () => this.store.listCollections(), listFiles: (id) => this.store.listFiles(id), original: (fileID) => cache!.original(fileID), diff --git a/test/cli/backup.test.ts b/test/cli/backup.test.ts index 6f4ed3d..ea3717e 100644 --- a/test/cli/backup.test.ts +++ b/test/cli/backup.test.ts @@ -625,6 +625,93 @@ describe("lib.backup", () => { }); }); +// Every refresh fails, as with an expired session or no network. +class FailingClient extends MockClient { + override async collectionsSince(): Promise { + throw new Error("HTTP 401 from server"); + } +} + +// Holds its refresh open until `release()` is called, then reports a third +// album, so a backup can be started while that refresh is still running. +class HeldClient extends MockClient { + release!: () => void; + private held = new Promise((resolve) => { + this.release = resolve; + }); + override async collectionsSince(): Promise { + await this.held; + return { + collections: [collection(3, "Later")], + deleted: [], + cursor: 2, + }; + } + override async filesSince(args: { + collectionID: number; + }): Promise { + if (args.collectionID !== 3) return super.filesSince(args); + return { files: [file(300, 3, "late.jpg")], deleted: [], cursor: 2 }; + } +} + +// Fill the library cache on disk, so the next open starts its refresh in the +// background instead of waiting for it. +const fillCache = async (): Promise => { + const lib = await openLibrary(stubSource()); + await lib.close(); +}; + +describe("the refresh before a backup", () => { + it("waits for a refresh already running and backs up what it found", async () => { + await fillCache(); + const client = new HeldClient(); + const lib = await openLibrary(stubSource(), client); + const outDir = join(root, "backup"); + + const backup = lib.backup({ downloadDirectory: outDir }); + client.release(); + const result = await backup; + + expect(result.totalFiles).toBe(4); + expect(existsSync(join(outDir, "originals", "300.jpg"))).toBe(true); + await lib.close(); + }); + + it("fails before any download when the refresh fails, leaving failures.json as it was", async () => { + await fillCache(); + const outDir = join(root, "backup"); + seedLedger(outDir, 100, "beach.jpg"); + const ledgerPath = join(outDir, "failures.json"); + const ledgerBefore = readFileSync(ledgerPath, "utf-8"); + const source = stubSource(); + const lib = await openLibrary(source, new FailingClient()); + + await expect(lib.backup({ downloadDirectory: outDir })).rejects.toThrow( + "HTTP 401 from server", + ); + + expect(source.originalCalls).toBe(0); + expect(readFileSync(ledgerPath, "utf-8")).toBe(ledgerBefore); + expect(existsSync(join(outDir, "originals"))).toBe(false); + await lib.close(); + }); + + it("fails when the refresh fails on an empty cache, instead of backing up nothing", async () => { + const source = stubSource(); + const lib = await openLibrary(source, new FailingClient()); + const outDir = join(root, "backup"); + + await expect(lib.backup({ downloadDirectory: outDir })).rejects.toThrow( + "HTTP 401 from server", + ); + + expect(source.originalCalls).toBe(0); + expect(existsSync(outDir)).toBe(false); + await lib.close(); + }); +}); + // The album folders under collections/, driven through `runBackup` with a // stand-in library whose albums a test changes between runs. describe("backup album folders", () => { diff --git a/test/cli/commands.test.ts b/test/cli/commands.test.ts index 5da826a..0f012c4 100644 --- a/test/cli/commands.test.ts +++ b/test/cli/commands.test.ts @@ -20,6 +20,7 @@ import { } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; +import { PassThrough } from "node:stream"; import { describe, it, expect, beforeAll, beforeEach, afterEach } from "vitest"; import { @@ -34,6 +35,7 @@ import { backupCommand, listMissingThumbnailsCommand, } from "../../src/cli-commands.js"; +import { run } from "../../src/cli-run.js"; import { loadSession } from "../../src/cli-session.js"; import type { Client, ClientSnapshot } from "../../src/client.js"; import type { ContentSource } from "../../src/library/content.js"; @@ -430,6 +432,34 @@ describe("backup", () => { expect(result.errors[0].fileID).toBe(101); expect(stderr.text).toBe("Starting backup...\n"); }); + + it("exits 1 with the error on one line when the refresh fails", async () => { + const client = { + ...fakeClient(), + collectionsSince: async () => { + throw new Error("HTTP 401 from server"); + }, + } as unknown as Client; + const dir = join(root, "backup"); + // Through `run`, as `bin/quak.ts` does, which prints a thrown error. + const runStderr = new PassThrough(); + let runText = ""; + runStderr.on("data", (chunk: Buffer) => { + runText += chunk.toString(); + }); + const code = await new Promise((resolve) => { + void run( + backupCommand(context(client), dir, {}), + new PassThrough(), + runStderr, + resolve, + ); + }); + expect(code).toBe(1); + expect(runText).toBe("quak: HTTP 401 from server\n"); + expect(stderr.text).toBe("Starting backup...\nRefreshing library...\n"); + expect(existsSync(join(dir, "originals"))).toBe(false); + }); }); describe("helper list-missing-thumbnails", () => {