quak files COLLECTION fetches that collection's files alone (closes #217)

`quak files COLLECTION` used to wait for a full refresh, which on an
empty cache fetched the files of every collection before it listed one.
It now reads the collection list, then calls the new
`Library.refreshCollection()`, which brings that one collection up to
date in the store as a refresh would, and saves it. It sends no request
for the files when the stored copy is current, and it keeps a newer copy
that a refresh stores while it fetches. It leaves the refresh cursor
alone, so the next refresh still fetches every other collection that
changed. `quak files` with no argument still waits for a refresh.

Model: opus-5-5

test/cli/commands.test.ts

@@ -535,9 +535,7 @@ describe("files", () => {
 
     it("exits 1 for an unknown collection", async () => {
         expect(await filesCommand(context(), "9", {})).toBe(1);
-        expect(stderr.text).toBe(
-            FETCHED_BOTH_ALBUMS + "Collection 9 not found\n",
-        );
+        expect(stderr.text).toBe("Collection 9 not found\n");
     });
 
     // Without a collection ID it lists every file in the account once (issue
@@ -1389,9 +1387,9 @@ describe("backup-metadata --exif", () => {
     });
 });
 
-// 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.)
+// Each test first runs `files 1` so the cache holds Vacation as it was, then
+// adds a file to Vacation on the server. (`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 filesCommand(context(), "1", {})).toBe(0);
@@ -1580,10 +1578,11 @@ 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.
+// account it seemed to hang. `files` with an album's ID did the same before it
+// listed that album (issue #217). 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 and query 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";
@@ -1664,7 +1663,8 @@ describe("an account with many albums", () => {
                     : input instanceof URL
                       ? input.href
                       : input.url;
-            paths.push(new URL(url).pathname);
+            const { pathname, search } = new URL(url);
+            paths.push(pathname + search);
             return ente(input, init);
         }) as typeof globalThis.fetch;
         const client = await Client.login({
@@ -1698,7 +1698,7 @@ describe("an account with many albums", () => {
             albums += `${id}\talbum\tAlbum ${id}\n`;
         expect(stdout.text).toBe(albums);
         expect(stderr.text).toBe("");
-        expect(paths).toEqual(["/collections/v2"]);
+        expect(paths).toEqual(["/collections/v2?sinceTime=0"]);
 
         // It stored nothing that would make a refresh skip an album's files:
         // `files` after it still finds the album's photo.
@@ -1707,33 +1707,59 @@ describe("an account with many albums", () => {
         expect(stdout.text).toBe("700\timage\tphoto-7.jpg\n");
     });
 
-    it("files counts on stderr each album whose files its refresh fetched", async () => {
+    it("files fetches the files of the album it lists and of no other", async () => {
         expect(await filesCommand(accountContext(), "7", {})).toBe(0);
 
         expect(stdout.text).toBe("700\timage\tphoto-7.jpg\n");
+        expect(stderr.text).toBe("");
+        expect(paths).toEqual([
+            "/collections/v2?sinceTime=0",
+            "/collections/v2/diff?collectionID=7&sinceTime=0",
+        ]);
+    });
+
+    // The first listing stored the album, so the second finds its stored copy
+    // current and asks for no album's files.
+    it("files run again for the same album sends no request for its files", async () => {
+        const ctx = accountContext();
+        expect(await filesCommand(ctx, "7", {})).toBe(0);
+        stdout.text = "";
+        paths.length = 0;
+
+        expect(await filesCommand(ctx, "7", {})).toBe(0);
+        expect(stdout.text).toBe("700\timage\tphoto-7.jpg\n");
+        expect(paths).toEqual(["/collections/v2?sinceTime=0"]);
+    });
+
+    it("files with no album's ID counts on stderr each album whose files its refresh fetched", async () => {
+        expect(await filesCommand(accountContext(), undefined, {})).toBe(0);
+
+        let photos = "";
         let progress = "";
         for (let n = 1; n <= ALBUMS; n++) {
+            photos += `${n * 100}\timage\tphoto-${n}.jpg\n`;
             progress += `Fetched the files of ${n} of ${ALBUMS} collections\n`;
         }
+        expect(stdout.text).toBe(photos);
         expect(stderr.text).toBe(progress);
     });
 
     // `--json` shapes stdout only, so the refresh's progress is still counted
     // on stderr.
-    it("files --json counts on stderr each album whose files its refresh fetched", async () => {
-        expect(await filesCommand(accountContext(), "7", { json: true })).toBe(
-            0,
-        );
+    it("files --json with no album's ID counts on stderr each album whose files its refresh fetched", async () => {
+        expect(
+            await filesCommand(accountContext(), undefined, { json: true }),
+        ).toBe(0);
 
-        expect(JSON.parse(stdout.text)).toEqual([
-            {
-                id: 700,
-                title: "photo-7.jpg",
-                fileType: "image",
-                creationTime: 1000,
-                collectionID: 7,
-            },
-        ]);
+        const rows = JSON.parse(stdout.text);
+        expect(rows).toHaveLength(ALBUMS);
+        expect(rows[6]).toEqual({
+            id: 700,
+            title: "photo-7.jpg",
+            fileType: "image",
+            creationTime: 1000,
+            collectionID: 7,
+        });
         let progress = "";
         for (let n = 1; n <= ALBUMS; n++) {
             progress += `Fetched the files of ${n} of ${ALBUMS} collections\n`;

test/cli/read.test.ts

@@ -3,13 +3,16 @@
  * issue #36, issue #52).
  *
  * The `collections`, `files`, `get`, and `get-thumb` commands must answer for
- * 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.
+ * current server state, not the local cache. `files` without a collection ID,
+ * `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. `files COLLECTION` reads that list
+ * too and then brings that one collection up to date
+ * (`refreshCollection()`, issue #217); the stand-in serves a collection's files
+ * once either has run.
  *
  * `collections` and `files` also list in the library's enumeration order
  * (`listCollections`/`listFiles`) — the order the pre-library CLI printed — not
@@ -64,12 +67,14 @@ const file = (
 
 // A library that reveals its records only after `fresh()` has been awaited, and
 // 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.
+// answers the collection list in that order without a refresh.
+// `refreshCollection()` reveals the files of that one collection.
+// `photos.byID` returns a stand-in `Photo` carrying just the fileID the helper
+// passes through.
 class FakeLibrary implements FreshReadLibrary {
     freshCalls = 0;
     freshCollectionsCalls = 0;
+    refreshCollectionIDs: number[] = [];
     private refreshed = false;
 
     constructor(
@@ -88,9 +93,8 @@ class FakeLibrary implements FreshReadLibrary {
         return this.collections;
     }
 
-    getCollection(id: number): Collection | undefined {
-        if (!this.refreshed) return undefined;
-        return this.collections.find((c) => c.id === id);
+    async refreshCollection(collection: Collection): Promise<void> {
+        this.refreshCollectionIDs.push(collection.id);
     }
 
     listCollections(): Collection[] {
@@ -98,7 +102,9 @@ class FakeLibrary implements FreshReadLibrary {
     }
 
     listFiles(collectionID: number): EnteFile[] {
-        return this.refreshed
+        const current =
+            this.refreshed || this.refreshCollectionIDs.includes(collectionID);
+        return current
             ? this.files.filter((f) => f.collectionID === collectionID)
             : [];
     }
@@ -138,18 +144,25 @@ describe("CLI read helpers (issue #36 amendment, issue #52)", () => {
         expect(rows.map((c) => c.id)).not.toEqual(newestFirst);
     });
 
-    it("freshFiles refreshes first, lists in enumeration order, keeps raw fields", async () => {
+    it("freshFiles brings the collection alone up to date, lists in enumeration order, keeps raw fields", async () => {
         // Enumeration order by id 10, 11, 12; creationTimes ascending, so a
-        // newest-first sort would reverse them.
+        // newest-first sort would reverse them. Collection 2's file is not
+        // listed.
         const files = [
             file(10, 1, 1_700_000_000_000_000),
             file(11, 1, 1_700_000_000_000_001),
             file(12, 1, 1_700_000_000_000_002),
+            file(20, 2, 1_700_000_000_000_003),
         ];
-        const lib = new FakeLibrary([collection(1, 100)], files);
+        const lib = new FakeLibrary(
+            [collection(1, 100), collection(2, 100)],
+            files,
+        );
 
         const rows = await freshFiles(lib, 1);
-        expect(lib.freshCalls).toBe(1);
+        expect(lib.freshCollectionsCalls).toBe(1);
+        expect(lib.refreshCollectionIDs).toEqual([1]);
+        expect(lib.freshCalls).toBe(0);
         expect(rows?.map((f) => f.id)).toEqual([10, 11, 12]);
 
         // Field values come from raw metadata: microsecond creationTime and the
@@ -206,7 +219,9 @@ describe("CLI read helpers (issue #36 amendment, issue #52)", () => {
     it("freshFiles returns undefined for an unknown collection", async () => {
         const lib = new FakeLibrary([collection(1, 100)], []);
         const rows = await freshFiles(lib, 999);
-        expect(lib.freshCalls).toBe(1);
+        expect(lib.freshCollectionsCalls).toBe(1);
+        expect(lib.refreshCollectionIDs).toEqual([]);
+        expect(lib.freshCalls).toBe(0);
         expect(rows).toBeUndefined();
     });
 

test/library/fresh.test.ts

@@ -17,6 +17,10 @@
  *  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).
+ *  5. `refreshCollection()` brings one collection up to date in the store and
+ *     leaves the stored cursor where it was, so the next refresh still fetches
+ *     the files of every other collection that changed (issue #217). A newer
+ *     copy that a refresh stores while it fetches the files is kept.
  *
  * 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
@@ -340,3 +344,132 @@ describe("Library.freshCollections", () => {
         }
     });
 });
+
+describe("Library.refreshCollection", () => {
+    let dir: string;
+    let cacheDirectory: string;
+
+    beforeEach(() => {
+        dir = mkdtempSync(join(tmpdir(), "quak-refresh-collection-"));
+        cacheDirectory = join(dir, "cache");
+    });
+
+    afterEach(() => {
+        rmSync(dir, { recursive: true, force: true });
+    });
+
+    // Collections 1 and 2 are stored at updationTime 100, with a stored cursor
+    // of 100, and then both change on the server.
+    it("fetches the files of one changed collection and leaves the others to the next refresh", async () => {
+        const client = new MockClient();
+        client.collectionsQueue.push({
+            collections: [collection(1, 100), collection(2, 100)],
+            deleted: [],
+            cursor: 100,
+        });
+        client.filesFor(1, {
+            files: [file(1001, 1, 90)],
+            deleted: [],
+            cursor: 90,
+        });
+        client.filesFor(2, {
+            files: [file(2001, 2, 90)],
+            deleted: [],
+            cursor: 90,
+        });
+        const lib = await Library.open({
+            client,
+            cacheDirectory,
+            refreshIntervalSeconds: SLOW_INTERVAL,
+        });
+        try {
+            client.filesCalls.length = 0;
+            client.filesFor(1, {
+                files: [file(1002, 1, 110)],
+                deleted: [],
+                cursor: 110,
+            });
+            client.filesFor(2, {
+                files: [file(2002, 2, 120)],
+                deleted: [],
+                cursor: 120,
+            });
+
+            // Collection 1's files alone, since the stored copy.
+            await lib.refreshCollection(collection(1, 110));
+            expect(client.filesCalls).toEqual([
+                { collectionID: 1, sinceTime: 100 },
+            ]);
+            expect(lib.listFiles(1).map((f) => f.id)).toEqual([1001, 1002]);
+            expect(lib.listFiles(2).map((f) => f.id)).toEqual([2001]);
+
+            // The stored copy is now current, so no request.
+            await lib.refreshCollection(collection(1, 110));
+            expect(client.filesCalls).toHaveLength(1);
+
+            // The next refresh asks from the stored cursor, 100, so it still
+            // sees collection 2 changed and fetches its files.
+            client.collectionsQueue.push({
+                collections: [collection(1, 110), collection(2, 120)],
+                deleted: [],
+                cursor: 120,
+            });
+            await lib.fresh();
+            expect(client.collectionsSinceTimes).toEqual([0, 100]);
+            expect(client.filesCalls).toEqual([
+                { collectionID: 1, sinceTime: 100 },
+                { collectionID: 2, sinceTime: 100 },
+            ]);
+            expect(lib.listFiles(2).map((f) => f.id)).toEqual([2001, 2002]);
+        } finally {
+            await lib.close();
+        }
+    });
+
+    // Collection 1 is stored at 100. `refreshCollection()` asks for it at 110,
+    // which adds file 1002. While that files request waits, a refresh stores
+    // it at 120, where 1002 is deleted again and 1003 added.
+    it("keeps a newer copy that a refresh stores while the files are fetched", async () => {
+        const { client, lib } = await openSeeded(cacheDirectory);
+        let release!: () => void;
+        const held = new Promise<void>((r) => (release = r));
+        try {
+            client.filesFor(
+                1,
+                { files: [file(1002, 1, 110)], deleted: [], cursor: 110 },
+                { files: [file(1003, 1, 120)], deleted: [1002], cursor: 120 },
+            );
+            client.collectionsQueue.push({
+                collections: [collection(1, 120)],
+                deleted: [],
+                cursor: 120,
+            });
+
+            // Hold the next files request, refreshCollection's, until the
+            // test releases it. The refresh's request after it is answered
+            // at once.
+            let fetchStarted!: () => void;
+            const started = new Promise<void>((r) => (fetchStarted = r));
+            const realFilesSince = client.filesSince.bind(client);
+            client.filesSince = async (args) => {
+                client.filesSince = realFilesSince;
+                const page = await realFilesSince(args);
+                fetchStarted();
+                await held;
+                return page;
+            };
+
+            const refreshing = lib.refreshCollection(collection(1, 110));
+            await started;
+            await lib.fresh();
+            release();
+            await refreshing;
+
+            expect(lib.getCollection(1)?.updationTime).toBe(120);
+            expect(lib.listFiles(1).map((f) => f.id)).toEqual([1001, 1003]);
+        } finally {
+            release();
+            await lib.close();
+        }
+    });
+});