quak collections reads the collection list alone (closes #210)

On an empty cache the read commands opened their library with a refresh
that fetched and decrypted the files of every collection in turn,
printing nothing and storing nothing until it had them all, so `quak
collections` seemed to hang on a large account. The new
`lib.freshCollections()` is the fresh read of the collection list alone:
one request, and nothing stored, so a later refresh still fetches every
collection's files. The read commands open their library with the new
`refreshOnOpen: false` and make only the refresh they ask for. A refresh
sends a "progress" event after each collection whose files it fetched;
the commands that wait on one print it on stderr, except with `--json`.

Model: opus-5-5

test/cli/commands.test.ts

@@ -53,11 +53,21 @@ import {
 } from "../../src/cli-commands.js";
 import { run } from "../../src/cli-run.js";
 import { loadSession } from "../../src/cli-session.js";
-import type { Client, ClientSnapshot, LoginOptions } from "../../src/client.js";
+import {
+    Client,
+    type ClientSnapshot,
+    type LoginOptions,
+} from "../../src/client.js";
 import { savePath, type ContentSource } from "../../src/library/content.js";
 import type { Collection, EnteFile } from "../../src/model/types.js";
 import { encryptBlob, init, toBase64 } from "../../src/crypto/index.js";
 import { defaultCacheDirectory } from "../../src/library/index.js";
+import {
+    buildAccount,
+    buildMetaFetch,
+    encryptSecretbox,
+    encryptStreamBlob,
+} from "../ente-server.js";
 import { HEIC_WITH_EXIF } from "../exif-heic.js";
 import {
     asLivePhoto,
@@ -171,6 +181,12 @@ const fakeClient = (
     return fake as unknown as Client;
 };
 
+// What a read command prints on stderr while the refresh it waits for fetches
+// the files of each of the fake's two albums, on an empty cache.
+const FETCHED_BOTH_ALBUMS =
+    "Fetched the files of 1 of 2 collections\n" +
+    "Fetched the files of 2 of 2 collections\n";
+
 // Collects everything written to it.
 class Output {
     text = "";
@@ -510,7 +526,9 @@ describe("files", () => {
 
     it("exits 1 for an unknown collection", async () => {
         expect(await filesCommand(context(), { collection: "9" })).toBe(1);
-        expect(stderr.text).toBe("Collection 9 not found\n");
+        expect(stderr.text).toBe(
+            FETCHED_BOTH_ALBUMS + "Collection 9 not found\n",
+        );
     });
 });
 
@@ -519,27 +537,27 @@ describe("get and get-thumb", () => {
         const out = join(root, "diagram.png");
         expect(await getCommand(context(), "200", { out })).toBe(0);
         expect(readFileSync(out)).toEqual(Buffer.alloc(7, 200));
-        expect(stderr.text).toBe(`7 bytes -> ${out}\n`);
+        expect(stderr.text).toBe(FETCHED_BOTH_ALBUMS + `7 bytes -> ${out}\n`);
     });
 
     it("get-thumb finds a file in any album without --collection", async () => {
         const out = join(root, "thumb.jpg");
         expect(await getThumbCommand(context(), "200", { out })).toBe(0);
         expect(readFileSync(out)).toEqual(Buffer.alloc(3, 200));
-        expect(stderr.text).toBe(`3 bytes -> ${out}\n`);
+        expect(stderr.text).toBe(FETCHED_BOTH_ALBUMS + `3 bytes -> ${out}\n`);
     });
 
     it("get exits 1 when no album has the file", async () => {
         const out = join(root, "x");
         expect(await getCommand(context(), "999", { out })).toBe(1);
-        expect(stderr.text).toBe("File 999 not found\n");
+        expect(stderr.text).toBe(FETCHED_BOTH_ALBUMS + "File 999 not found\n");
         expect(existsSync(out)).toBe(false);
     });
 
     it("get-thumb exits 1 when no album has the file", async () => {
         const out = join(root, "x");
         expect(await getThumbCommand(context(), "999", { out })).toBe(1);
-        expect(stderr.text).toBe("File 999 not found\n");
+        expect(stderr.text).toBe(FETCHED_BOTH_ALBUMS + "File 999 not found\n");
         expect(existsSync(out)).toBe(false);
     });
 });
@@ -549,6 +567,9 @@ describe("get and get-thumb", () => {
 // one live photo, 300, downloaded through the real download layer
 // (test/live-photo.ts).
 describe("a live photo", () => {
+    // What `get` prints on stderr while its refresh fetches Vacation's files.
+    const FETCHED_ONE_ALBUM = "Fetched the files of 1 of 1 collections\n";
+
     const livePhotoClient = async (image = IMAGE): Promise<Client> => {
         const { file: live, body } = await asLivePhoto(
             file(300, 1, "IMG_0300.HEIC"),
@@ -593,7 +614,8 @@ describe("a live photo", () => {
             Buffer.from(VIDEO),
         );
         expect(stderr.text).toBe(
-            `${IMAGE.length} bytes -> IMG_0300.heic\n` +
+            FETCHED_ONE_ALBUM +
+                `${IMAGE.length} bytes -> IMG_0300.heic\n` +
                 `${VIDEO.length} bytes -> IMG_0300.mov\n`,
         );
     });
@@ -609,7 +631,8 @@ describe("a live photo", () => {
         expect(readFileSync(out)).toEqual(Buffer.from(IMAGE));
         expect(readFileSync(video)).toEqual(Buffer.from(VIDEO));
         expect(stderr.text).toBe(
-            `${IMAGE.length} bytes -> ${out}\n${VIDEO.length} bytes -> ${video}\n`,
+            FETCHED_ONE_ALBUM +
+                `${IMAGE.length} bytes -> ${out}\n${VIDEO.length} bytes -> ${video}\n`,
         );
     });
 
@@ -623,7 +646,8 @@ describe("a live photo", () => {
         expect(existsSync(out)).toBe(false);
         expect(existsSync(join(root, "photo.mov"))).toBe(false);
         expect(stderr.text).toBe(
-            `File 300 is a live photo, and its video would also be written to ${out}\n`,
+            FETCHED_ONE_ALBUM +
+                `File 300 is a live photo, and its video would also be written to ${out}\n`,
         );
     });
 
@@ -1103,11 +1127,12 @@ describe("backup-metadata --exif", () => {
     });
 });
 
-// Each test first runs `collections` so the cache holds the account as it was,
-// then changes the server under it.
+// Each test first runs `files` so the cache holds the account as it was, then
+// changes the server under it. (`collections` would not do: it reads the
+// collection list alone and stores nothing.)
 describe("backup-metadata and the thumbnail helpers refresh first", () => {
     beforeEach(async () => {
-        expect(await collectionsCommand(context(), {})).toBe(0);
+        expect(await filesCommand(context(), { collection: "1" })).toBe(0);
         stdout.text = "";
         stderr.text = "";
     });
@@ -1292,3 +1317,169 @@ describe("an ID given on the command line", () => {
         },
     );
 });
+
+// An account with many albums (issue #210). On an empty cache `collections`
+// used to wait for a refresh that fetched and decrypted the files of every
+// album, one album after another, before it printed anything, so on such an
+// account it seemed to hang. The account here has 40 albums of one photo each,
+// encrypted as Ente's clients encrypt them, on the fake Ente server of
+// `test/ente-server.ts`, which also records the path of each request quak
+// sends. The session is the one logging in to that server saves.
+describe("an account with many albums", () => {
+    const ALBUMS = 40;
+    const PASSWORD = "many-albums";
+    const paths: string[] = [];
+    let server: typeof globalThis.fetch;
+    let snapshot: ClientSnapshot;
+
+    // Album n is named `Album n` and holds one photo, n * 100, titled
+    // `photo-n.jpg`.
+    beforeAll(async () => {
+        await sodium.ready;
+        const account = await buildAccount(PASSWORD);
+        const collections: Record<string, unknown>[] = [];
+        const filesByCollection: Record<number, Record<string, unknown>[]> = {};
+        for (let id = 1; id <= ALBUMS; id++) {
+            const albumKey = sodium.crypto_secretbox_keygen();
+            const albumKeyBox = encryptSecretbox(albumKey, account.masterKey);
+            const name = encryptSecretbox(
+                new TextEncoder().encode(`Album ${id}`),
+                albumKey,
+            );
+            collections.push({
+                id,
+                owner: { id: USER_ID },
+                encryptedKey: toBase64(albumKeyBox.ciphertext),
+                keyDecryptionNonce: toBase64(albumKeyBox.nonce),
+                encryptedName: toBase64(name.ciphertext),
+                nameDecryptionNonce: toBase64(name.nonce),
+                type: "album",
+                updationTime: 1,
+            });
+            const photoKey =
+                sodium.crypto_secretstream_xchacha20poly1305_keygen();
+            const photoKeyBox = encryptSecretbox(photoKey, albumKey);
+            const metadata = encryptStreamBlob(
+                new TextEncoder().encode(
+                    JSON.stringify({
+                        title: `photo-${id}.jpg`,
+                        fileType: 0,
+                        creationTime: 1000,
+                        modificationTime: 1000,
+                    }),
+                ),
+                photoKey,
+            );
+            filesByCollection[id] = [
+                {
+                    id: id * 100,
+                    collectionID: id,
+                    ownerID: USER_ID,
+                    encryptedKey: toBase64(photoKeyBox.ciphertext),
+                    keyDecryptionNonce: toBase64(photoKeyBox.nonce),
+                    metadata: {
+                        encryptedData: toBase64(metadata.ciphertext),
+                        decryptionHeader: toBase64(metadata.header),
+                    },
+                    // These tests fetch no original and no thumbnail.
+                    file: { decryptionHeader: "" },
+                    thumbnail: { decryptionHeader: "" },
+                    updationTime: 1,
+                },
+            ];
+        }
+        const ente = buildMetaFetch({
+            verifier: account.verifier,
+            srpAttributes: account.srpAttributes,
+            keyAttributes: account.keyAttributes,
+            encryptedToken: account.encryptedToken,
+            collections,
+            filesByCollection,
+            encryptedMLData: {},
+            fileCiphertexts: {},
+        });
+        server = (async (input: RequestInfo | URL, init?: RequestInit) => {
+            const url =
+                typeof input === "string"
+                    ? input
+                    : input instanceof URL
+                      ? input.href
+                      : input.url;
+            paths.push(new URL(url).pathname);
+            return ente(input, init);
+        }) as typeof globalThis.fetch;
+        const client = await Client.login({
+            email: "many@example.com",
+            password: PASSWORD,
+            apiOptions: { fetch: server },
+        });
+        snapshot = client.toJSON();
+    });
+
+    beforeEach(() => {
+        paths.length = 0;
+    });
+
+    // A context whose saved session sends its requests to the fake server.
+    const accountContext = (): CliContext => {
+        const ctx = context();
+        saveSession(ctx.sessionDir, snapshot);
+        return {
+            ...ctx,
+            loadSession: (path) => loadSession(path, { fetch: server }),
+        };
+    };
+
+    it("collections lists every album from one request, fetching no album's files", async () => {
+        const ctx = accountContext();
+        expect(await collectionsCommand(ctx, {})).toBe(0);
+
+        let albums = "";
+        for (let id = 1; id <= ALBUMS; id++)
+            albums += `${id}\talbum\tAlbum ${id}\n`;
+        expect(stdout.text).toBe(albums);
+        expect(stderr.text).toBe("");
+        expect(paths).toEqual(["/collections/v2"]);
+
+        // It stored nothing that would make a refresh skip an album's files:
+        // `files` after it still finds the album's photo.
+        stdout.text = "";
+        expect(await filesCommand(ctx, { collection: "7" })).toBe(0);
+        expect(stdout.text).toBe("700\timage\tphoto-7.jpg\n");
+    });
+
+    it("files counts on stderr each album whose files its refresh fetched", async () => {
+        expect(await filesCommand(accountContext(), { collection: "7" })).toBe(
+            0,
+        );
+
+        expect(stdout.text).toBe("700\timage\tphoto-7.jpg\n");
+        let progress = "";
+        for (let n = 1; n <= ALBUMS; n++) {
+            progress += `Fetched the files of ${n} of ${ALBUMS} collections\n`;
+        }
+        expect(stderr.text).toBe(progress);
+    });
+
+    // With `--json` the refresh's progress is not printed, as `backup` and
+    // the thumbnail helpers print none of theirs.
+    it("files --json prints no progress", async () => {
+        expect(
+            await filesCommand(accountContext(), {
+                collection: "7",
+                json: true,
+            }),
+        ).toBe(0);
+
+        expect(JSON.parse(stdout.text)).toEqual([
+            {
+                id: 700,
+                title: "photo-7.jpg",
+                fileType: "image",
+                creationTime: 1000,
+                collectionID: 7,
+            },
+        ]);
+        expect(stderr.text).toBe("");
+    });
+});

test/cli/read.test.ts

@@ -3,10 +3,13 @@
  * issue #36, issue #52).
  *
  * The `collections`, `files`, `get`, and `get-thumb` commands must answer for
- * current server state, not the local cache, so each helper forces a
- * `Library.fresh()` round-trip before it reads. The stand-in library below
- * serves nothing until `fresh()` has been awaited, so a helper that read
- * without refreshing would come back empty and fail here.
+ * current server state, not the local cache. `files`, `get` and `get-thumb`
+ * force a `Library.fresh()` round-trip before they read; the stand-in library
+ * below serves nothing until `fresh()` has been awaited, so a helper that read
+ * without refreshing would come back empty and fail here. `collections` asks
+ * the library for the collection list alone (`freshCollections()`, issue
+ * #210), and must not wait for a refresh, which would fetch the files of every
+ * collection.
  *
  * `collections` and `files` also list in the library's enumeration order
  * (`listCollections`/`listFiles`) — the order the pre-library CLI printed — not
@@ -60,10 +63,13 @@ const file = (
 });
 
 // A library that reveals its records only after `fresh()` has been awaited, and
-// serves them in the enumeration order it was given. `photos.byID` returns a
-// stand-in `Photo` carrying just the fileID the helper passes through.
+// serves them in the enumeration order it was given. `freshCollections()`
+// answers the collection list in that order without a refresh. `photos.byID`
+// returns a stand-in `Photo` carrying just the fileID the helper passes
+// through.
 class FakeLibrary implements FreshReadLibrary {
     freshCalls = 0;
+    freshCollectionsCalls = 0;
     private refreshed = false;
 
     constructor(
@@ -77,12 +83,14 @@ class FakeLibrary implements FreshReadLibrary {
         return {};
     }
 
-    listCollections(): Collection[] {
-        return this.refreshed ? this.collections : [];
+    async freshCollections(): Promise<Collection[]> {
+        this.freshCollectionsCalls++;
+        return this.collections;
     }
 
     getCollection(id: number): Collection | undefined {
-        return this.listCollections().find((c) => c.id === id);
+        if (!this.refreshed) return undefined;
+        return this.collections.find((c) => c.id === id);
     }
 
     listFiles(collectionID: number): EnteFile[] {
@@ -106,7 +114,7 @@ class FakeLibrary implements FreshReadLibrary {
 }
 
 describe("CLI read helpers (issue #36 amendment, issue #52)", () => {
-    it("freshCollections refreshes first, then lists in enumeration order", async () => {
+    it("freshCollections reads the collection list alone, in enumeration order", async () => {
         // Enumeration order 2, 1, 3; a newest-first sort would be 3, 2, 1.
         const lib = new FakeLibrary(
             [collection(2, 200), collection(1, 300), collection(3, 100)],
@@ -114,7 +122,8 @@ describe("CLI read helpers (issue #36 amendment, issue #52)", () => {
         );
         const rows = await freshCollections(lib);
 
-        expect(lib.freshCalls).toBe(1);
+        expect(lib.freshCollectionsCalls).toBe(1);
+        expect(lib.freshCalls).toBe(0);
         expect(rows.map((c) => c.id)).toEqual([2, 1, 3]);
         // The projection's newest-first order is a different sequence, so this
         // is not accidentally that order.

test/library/fresh.test.ts

@@ -14,6 +14,9 @@
  *     three.
  *  3. A refresh that fails rejects the fresh read (currency was unavailable),
  *     while the default reads stay silent and keep serving the last good copy.
+ *  4. `freshCollections()` reads the collection list alone, with one request
+ *     and no collection's files, and stores nothing, so the next refresh still
+ *     fetches the files of every collection (issue #210).
  *
  * The client is the same metadata-only mock the background-refresh tests use:
  * no crypto, no network, scripted pages, and a record of each call. A long
@@ -270,3 +273,70 @@ describe("Library.fresh", () => {
         }
     });
 });
+
+describe("Library.freshCollections", () => {
+    let dir: string;
+    let cacheDirectory: string;
+
+    beforeEach(() => {
+        dir = mkdtempSync(join(tmpdir(), "quak-fresh-collections-"));
+        cacheDirectory = join(dir, "cache");
+    });
+
+    afterEach(() => {
+        rmSync(dir, { recursive: true, force: true });
+    });
+
+    // A refresh fetches a collection's files only when the collection is new
+    // or its updationTime has advanced past the stored copy's. Had the
+    // collection list been stored, the next refresh would take every
+    // collection for one it already holds the files of, and fetch none.
+    it("asks for the collection list alone and stores nothing", async () => {
+        const client = new MockClient();
+        const page = {
+            collections: [collection(2, 100), collection(1, 110)],
+            deleted: [],
+            cursor: 110,
+        };
+        client.collectionsQueue.push(page, page);
+        client.filesFor(1, {
+            files: [file(1001, 1, 90)],
+            deleted: [],
+            cursor: 90,
+        });
+        client.filesFor(2, {
+            files: [file(2001, 2, 95)],
+            deleted: [],
+            cursor: 95,
+        });
+
+        const lib = await Library.open({
+            client,
+            cacheDirectory,
+            refreshOnOpen: false,
+            refreshIntervalSeconds: SLOW_INTERVAL,
+        });
+        try {
+            // One request, the whole list in the server's order, and no
+            // collection's files.
+            const collections = await lib.freshCollections();
+            expect(collections.map((c) => c.id)).toEqual([2, 1]);
+            expect(collections.map((c) => c.name)).toEqual([
+                "album-2",
+                "album-1",
+            ]);
+            expect(client.collectionsSinceTimes).toEqual([0]);
+            expect(client.filesCalls).toEqual([]);
+            expect(lib.listCollections()).toEqual([]);
+
+            // The next refresh still asks from the beginning and fetches the
+            // files of both collections.
+            const reads = await lib.fresh();
+            expect(client.collectionsSinceTimes).toEqual([0, 0]);
+            expect(reads.photos.byID({ fileID: 1001 })?.fileID).toBe(1001);
+            expect(reads.photos.byID({ fileID: 2001 })?.fileID).toBe(2001);
+        } finally {
+            await lib.close();
+        }
+    });
+});

test/library/library.test.ts

@@ -26,6 +26,9 @@
  *    opening.
  * 8. A save failure that leaves RAM ahead of disk keeps `status().lastError`
  *    set and keeps retrying the write; a later empty refresh does not clear it.
+ * 9. With `refreshOnOpen: false`, `open()` starts no refresh at all, and a
+ *    refresh reports through `onProgress` each collection whose files it
+ *    fetched.
  *
  * The client is a mock: no crypto, no network. It serves scripted pages and
  * records the `sinceTime` each call carried so cursor threading is provable.
@@ -581,6 +584,107 @@ describe("Library.open and background refresh", () => {
         }
     });
 
+    // A caller that asks for each refresh it needs, as the CLI's read commands
+    // do, opens without one: on an empty cache the refresh `open()` would
+    // await fetches the files of every collection, which `quak collections`
+    // never needs (issue #210).
+    it("starts no refresh as it opens when refreshOnOpen is false", async () => {
+        const client = new MockClient();
+        client.collectionsQueue.push({
+            collections: [collection(1, 100)],
+            deleted: [],
+            cursor: 100,
+        });
+
+        const lib = await Library.open({
+            client,
+            cacheDirectory,
+            refreshOnOpen: false,
+            refreshIntervalSeconds: Infinity,
+        });
+        try {
+            expect(client.collectionsSinceTimes).toEqual([]);
+            expect(lib.listCollections()).toEqual([]);
+            expect(lib.status().lastRefreshAt).toBeUndefined();
+
+            // The refresh comes when the caller asks for it.
+            await lib.fresh();
+            expect(client.collectionsSinceTimes).toEqual([0]);
+            expect(lib.listCollections().map((c) => c.id)).toEqual([1]);
+        } finally {
+            await lib.close();
+        }
+    });
+
+    // A refresh fetches the files of each collection that changed, one
+    // collection after another, and stores nothing until it has them all. On
+    // a large account that takes a while, so it reports each collection whose
+    // files it has fetched, and how many it will fetch in all (issue #210).
+    it("reports each collection whose files a refresh fetched", async () => {
+        const events: RefreshEvent[] = [];
+        const client = new MockClient();
+        client.collectionsQueue.push({
+            collections: [
+                collection(1, 100),
+                collection(2, 100),
+                collection(3, 100),
+            ],
+            deleted: [],
+            cursor: 100,
+        });
+
+        const lib = await Library.open({
+            client,
+            cacheDirectory,
+            refreshIntervalSeconds: Infinity,
+            onProgress: (e) => events.push(e),
+        });
+        try {
+            expect(events).toEqual([
+                { operation: "refresh", status: "started" },
+                {
+                    operation: "refresh",
+                    status: "progress",
+                    fetched: 1,
+                    total: 3,
+                },
+                {
+                    operation: "refresh",
+                    status: "progress",
+                    fetched: 2,
+                    total: 3,
+                },
+                {
+                    operation: "refresh",
+                    status: "progress",
+                    fetched: 3,
+                    total: 3,
+                },
+                { operation: "refresh", status: "done" },
+            ]);
+
+            // Only collection 2 changed since, so the next refresh fetches
+            // its files alone and counts one.
+            events.length = 0;
+            client.collectionsQueue.push({
+                collections: [collection(2, 200)],
+                deleted: [],
+                cursor: 200,
+            });
+            await lib.fresh();
+            expect(events.filter((e) => e.status === "progress")).toEqual([
+                {
+                    operation: "refresh",
+                    status: "progress",
+                    fetched: 1,
+                    total: 1,
+                },
+            ]);
+        } finally {
+            await lib.close();
+        }
+    });
+
     it("keeps a save failure visible until a save actually succeeds", async () => {
         const saveSpy = vi
             .spyOn(MetadataStore.prototype, "save")