Fix two intermittently failing library tests (closes #90)

Library.close() now returns a promise that resolves once the work it
started has finished: an in-flight refresh with its cache write, the ML
data fetch, and running precache sweeps. The interval test could see a
refresh's new state, close, and remove the directory while the write was
still running. Every library test now awaits close(), and a new test
holds a cache write open to prove close() waits for it.

The precache test waited for its stub source to be called, but the cache
records a file only after checking it on disk, so status() could lag. It
now waits for both fills to report "done".

Model: opus-5-5

README.md

@@ -564,9 +564,9 @@ and pass it. The three pools default to 10 / 5 / 25 (see Request pools below).
 refresh/ML times and errors, originals usage and effective limit, precache
 progress, and `closed`). `lib.close()` stops the background timer; it is
 idempotent, and an in-flight refresh is left to finish. The promise it returns
-resolves once that refresh, including its cache write, has finished, so the
-cache directory can then be removed; precache fetches already running are not
-waited for.
+resolves once that refresh (including its cache write), the ML data fetch and
+the precache fetches already running have all finished, so the cache directory
+can then be removed.
 
 ### Default reads vs. fresh reads
 

TODO.md

@@ -20,11 +20,17 @@ Tag v1.0.0.
 
 - 2026-09-23: Fixed two intermittently failing library tests (issue 90).
   `Library.close()` now returns a promise that resolves once an in-flight
-  refresh, including its cache write, has finished; the library tests await it,
-  so `afterEach` no longer removes the cache directory while a refresh is still
-  writing into it. The precache test waits for both fills to report "done"
-  instead of for its stub source to be called, which happened before the cache
-  recorded the file.
+  refresh (including its cache write), the ML data fetch and running precache
+  sweeps have finished; the library tests await it, so `afterEach` no longer
+  removes the cache directory while something is still writing into it. The
+  precache test waits for both fills to report "done" instead of for its stub
+  source to be called, which happened before the cache recorded the file.
+- 2026-09-23: Pinned three guards reviewers found untested (issue 89). The
+  download idle deadline's timer is unref'd, so it can never keep the process
+  alive, and a test checks no timer is left after a download completes or fails.
+  A test covers the rejection of `#` in a request path. The EXIF scan compares
+  the `Exif` header only in an APP1 segment of length 8 or more, so it never
+  reads the next segment's bytes, with a test for a short one.
 - 2026-09-23: Made the download deadline an idle deadline (issue 24).
   `downloadTimeoutMs` now aborts a file or thumbnail download only after no
   bytes have arrived for that long, default 60 seconds, instead of bounding the

src/api/client.ts

@@ -49,8 +49,8 @@ export interface StreamOptions {
 // An abort signal that fires once `ms` pass without a call to `restart`. It
 // aborts with a `TimeoutError`, the same reason `AbortSignal.timeout()` gives,
 // so the retry classifier treats an idle download exactly as it treats any
-// other deadline. `stop` must be called when the download ends, or the timer
-// keeps the process alive until it fires.
+// other deadline. `stop` must be called when the download ends. The timer is
+// unref'd, so even one left running never keeps the process alive.
 const idleDeadline = (ms: number) => {
     const controller = new AbortController();
     let timer: ReturnType<typeof setTimeout> | undefined;
@@ -65,6 +65,7 @@ const idleDeadline = (ms: number) => {
                 ),
             );
         }, ms);
+        timer.unref();
     };
     restart();
     return { signal: controller.signal, restart, stop };

src/library/index.ts

@@ -266,8 +266,9 @@ export class Library {
     // that, a fresh read propagates it.
     private cycle?: Promise<void>;
     // Guards the ML fetch pass so a slow backfill never runs twice at once; a
-    // refresh whose pass is still running kicks nothing new.
-    private mlFetching = false;
+    // refresh whose pass is still running kicks nothing new. Holds the running
+    // pass, so `close()` can wait for it.
+    private mlFetch?: Promise<void>;
     private closed = false;
     private lastRefreshAt?: number;
     private lastError?: string;
@@ -561,18 +562,20 @@ export class Library {
 
     // Stop the background timer. Idempotent. An in-flight refresh is left to
     // finish; it will not schedule another cycle once closed. The returned
-    // promise resolves once that refresh, including its cache write, has
-    // finished, so a caller can then remove the cache directory. A refresh
-    // failure is reported through `status()`, not thrown here. Precache
-    // fetches already running are not waited for.
+    // promise resolves once that refresh (including its cache write), the ML
+    // fetch pass and the precache fetches already running have all finished,
+    // so a caller can then remove the cache directory. A refresh failure is
+    // reported through `status()`, not thrown here.
     async close(): Promise<void> {
         this.closed = true;
-        this.precache?.close();
+        const precacheClosed = this.precache?.close();
         if (this.timer !== undefined) {
             clearTimeout(this.timer);
             this.timer = undefined;
         }
         await this.cycle?.catch(() => {});
+        await this.mlFetch;
+        await precacheClosed;
     }
 
     private scheduleNext(): void {
@@ -636,7 +639,9 @@ export class Library {
             // outside the refresh's success/failure so a fetch or disk problem
             // there never marks the metadata refresh failed, and it is not
             // awaited so it never stalls the refresh interval.
-            void this.runMLFetch();
+            this.mlFetch ??= this.runMLFetch().finally(() => {
+                this.mlFetch = undefined;
+            });
         } catch (err) {
             const error = err instanceof Error ? err.message : String(err);
             this.lastError = error;
@@ -750,13 +755,12 @@ export class Library {
         // Bind so the call keeps the client as its receiver when invoked
         // through the pool below.
         const fetchMLData = this.client.fetchMLData?.bind(this.client);
-        if (!mldata || !fetchMLData || this.closed || this.mlFetching) return;
+        if (!mldata || !fetchMLData || this.closed) return;
 
         const files = this.uniqueFiles();
         const needed = mldata.neededFor(files);
         if (needed.length === 0) return;
 
-        this.mlFetching = true;
         this.emit({ operation: "fetchMLData", status: "started" });
         try {
             const fileKeys = new Map<number, Uint8Array>();
@@ -789,8 +793,6 @@ export class Library {
             const error = err instanceof Error ? err.message : String(err);
             this.lastMLError = error;
             this.emit({ operation: "fetchMLData", status: "failed", error });
-        } finally {
-            this.mlFetching = false;
         }
     }
 

src/library/precache.ts

@@ -92,9 +92,10 @@ export class Precache {
     private pinned = new Set<number>();
 
     // A sweep runs at most once per fill at a time; a re-kick while one runs is
-    // a no-op, and the next refresh re-kicks after it finishes.
-    private thumbRunning = false;
-    private originalsRunning = false;
+    // a no-op, and the next refresh re-kicks after it finishes. Each holds the
+    // running sweep, so `close()` can wait for it.
+    private thumbSweep?: Promise<void>;
+    private originalsSweep?: Promise<void>;
     private readonly aborter = new AbortController();
     private closed = false;
 
@@ -191,15 +192,19 @@ export class Precache {
     }
 
     // Stop the fills. In-flight fetches are left to settle; queued ones drop.
-    close(): void {
+    // Resolves once both sweeps have finished, so nothing is still writing.
+    async close(): Promise<void> {
         this.closed = true;
         this.aborter.abort();
+        await Promise.all([
+            this.thumbSweep?.catch(() => {}),
+            this.originalsSweep?.catch(() => {}),
+        ]);
     }
 
     private kickThumbnails(): void {
-        if (this.thumbRunning) return;
-        this.thumbRunning = true;
-        void this.sweep(
+        if (this.thumbSweep) return;
+        this.thumbSweep = this.sweep(
             "precacheThumbnails",
             () => this.thumbOrder,
             (id) => this.cache!.pathsFor(id).thumbnailPath !== undefined,
@@ -211,14 +216,13 @@ export class Precache {
                     signal: this.aborter.signal,
                 }),
         ).finally(() => {
-            this.thumbRunning = false;
+            this.thumbSweep = undefined;
         });
     }
 
     private kickOriginals(): void {
-        if (this.originalsRunning) return;
-        this.originalsRunning = true;
-        void this.sweep(
+        if (this.originalsSweep) return;
+        this.originalsSweep = this.sweep(
             "precacheOriginals",
             () => this.originalsOrder,
             (id) => this.cache!.pathsFor(id).originalPath !== undefined,
@@ -229,7 +233,7 @@ export class Precache {
                     signal: this.aborter.signal,
                 }),
         ).finally(() => {
-            this.originalsRunning = false;
+            this.originalsSweep = undefined;
         });
     }
 

src/metadata-backup.ts

@@ -45,8 +45,11 @@ export const extractExifFromJpeg = (
                 error: `segment length ${len} at byte ${offset} runs past the end of the file`,
             };
         if (marker === 0xe1) {
-            // APP1 — check for "Exif\0\0" header
+            // APP1 — check for "Exif\0\0" header. A length under 8 cannot hold
+            // the six-byte header, so the segment is not EXIF; below 6 the
+            // bytes compared would also lie past the segment.
             if (
+                len >= 8 &&
                 buf[offset + 4] === 0x45 &&
                 buf[offset + 5] === 0x78 &&
                 buf[offset + 6] === 0x69 &&

test/api/client.test.ts

@@ -474,6 +474,16 @@ describe("ApiClient request URLs", () => {
         );
         expect(calls).toHaveLength(0);
     });
+
+    it("rejects a path that carries a fragment", async () => {
+        const { fetch, calls } = recordingFetch();
+        const client = new ApiClient({ fetch });
+
+        await expect(client.getJSON("/diff#top")).rejects.toThrow(
+            /must not contain "\?" or "#"/,
+        );
+        expect(calls).toHaveLength(0);
+    });
 });
 
 describe("ApiError", () => {
@@ -980,6 +990,52 @@ describe("ApiClient timeouts", () => {
         }
         expect(joined).toEqual(payload);
     });
+
+    it("leaves no timer pending after a download completes or fails", async () => {
+        vi.useFakeTimers();
+        try {
+            const { fetch } = scriptedFetch(
+                streamResponse(new Uint8Array([1, 2, 3])),
+                textResponse("gone", 404),
+            );
+            const client = new ApiClient({
+                fetch,
+                retry: { ...noWait, attempts: 1 },
+            });
+
+            expect(await readAll(await client.getFileStream(1))).toBe(3);
+            expect(vi.getTimerCount()).toBe(0);
+
+            await expect(client.getFileStream(2)).rejects.toBeInstanceOf(
+                ApiError,
+            );
+            expect(vi.getTimerCount()).toBe(0);
+        } finally {
+            vi.useRealTimers();
+        }
+    });
+
+    it("never lets the download timer keep the process alive", async () => {
+        const spy = vi.spyOn(globalThis, "setTimeout");
+        try {
+            const { fetch } = scriptedFetch(
+                streamResponse(new Uint8Array([1])),
+            );
+            const client = new ApiClient({
+                fetch,
+                downloadTimeoutMs: 12_345,
+                retry: noWait,
+            });
+
+            const stream = await client.getFileStream(1);
+            const i = spy.mock.calls.findIndex((call) => call[1] === 12_345);
+            const timer = spy.mock.results[i]!.value as NodeJS.Timeout;
+            expect(timer.hasRef()).toBe(false);
+            await stream.cancel();
+        } finally {
+            spy.mockRestore();
+        }
+    });
 });
 
 describe("ApiClient error typing", () => {

test/cli/metadata-exif.test.ts

@@ -51,6 +51,20 @@ describe("extractExifFromJpeg", () => {
         expect(extractExifFromJpeg(bytes(SOI, app0, SOS))).toEqual({});
     });
 
+    it("ignores an APP1 segment too short to hold the Exif header", () => {
+        // A length under 8 cannot hold the six-byte "Exif\0\0" header, so the
+        // segment is not EXIF. This one has length 7 and holds only "Exif\0",
+        // which the old code, lacking the length check, returned as EXIF.
+        const short = app1(EXIF_HEADER.slice(0, 5));
+        expect(extractExifFromJpeg(bytes(SOI, short, SOS))).toEqual({});
+    });
+
+    it("accepts an APP1 segment of length 8 holding just the Exif header", () => {
+        const scan = extractExifFromJpeg(bytes(SOI, app1(EXIF_HEADER), SOS));
+        expect(scan.error).toBeUndefined();
+        expect([...scan.exif!]).toEqual(EXIF_HEADER);
+    });
+
     it("reports a JPEG truncated inside a segment header", () => {
         const scan = extractExifFromJpeg(bytes(SOI, [0xff, 0xe1, 0x00]));
         expect(scan.exif).toBeUndefined();

test/library/content-library.test.ts

@@ -116,7 +116,7 @@ describe("Library content wiring", () => {
         expect(lib.photos.byID({ fileID: 1 })!.record().thumbnailPath).toBe(
             result.path,
         );
-        lib.close();
+        await lib.close();
     });
 
     it("drives thumbnails.ensure through the cache", async () => {
@@ -139,7 +139,7 @@ describe("Library content wiring", () => {
         expect(results).toEqual([
             { fileID: 1, path: join(root, "cache", "thumbnails", "1.jpg") },
         ]);
-        lib.close();
+        await lib.close();
     });
 
     it("throws from content methods when opened without a content source", async () => {
@@ -155,6 +155,6 @@ describe("Library content wiring", () => {
         await expect(
             lib.thumbnails.ensure({ fileIDs: [1], priority: "visible" }),
         ).rejects.toThrow(/content cache/i);
-        lib.close();
+        await lib.close();
     });
 });

test/library/fresh.test.ts

@@ -179,7 +179,7 @@ describe("Library.fresh", () => {
             // And the change is now live for the default namespaces too.
             expect(lib.photos.byID({ fileID: 1002 })?.fileID).toBe(1002);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -228,7 +228,7 @@ describe("Library.fresh", () => {
                 expect(reads.photos.byID({ fileID: 1002 })?.fileID).toBe(1002);
             }
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -266,7 +266,7 @@ describe("Library.fresh", () => {
             );
             expect(lib.status().lastError).toBeUndefined();
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 });

test/library/library.test.ts

@@ -17,7 +17,8 @@
  *    failure surfaces via `onProgress` ("failed") and `status()`, and a later
  *    success clears the error. `open()` itself resolves even when the first
  *    refresh fails (offline start from cache).
- * 5. `close()` stops the timer and is idempotent.
+ * 5. `close()` stops the timer and is idempotent, and its promise resolves
+ *    only once an in-flight refresh has written the cache file.
  * 6. `cacheDirectory` defaults to the env-paths cache dir plus the user id.
  * 7. `open()` branches on the cache: an empty cache awaits the first refresh
  *    (it has nothing to serve yet); an existing cache serves its copy at once
@@ -655,6 +656,65 @@ describe("Library.open and background refresh", () => {
         expect(client.collectionsSinceTimes.length).toBe(callsAfterOpen);
     });
 
+    it("close() resolves only after an in-flight refresh has written the cache", async () => {
+        const path = join(cacheDirectory, "metadata.json");
+        const seed = await MetadataStore.load(path);
+        seed.userID = USER_ID;
+        seed.collectionsSinceTime = 500;
+        seed.putCollection(collection(1, 400));
+        seed.putFile(file(1001, 1, 400));
+        await seed.save();
+
+        const client = new MockClient();
+        client.collectionsQueue.push({
+            collections: [collection(1, 600)],
+            deleted: [],
+            cursor: 600,
+        });
+        client.filesFor(1, {
+            files: [file(1002, 1, 600)],
+            deleted: [],
+            cursor: 600,
+        });
+
+        // Hold the refresh's cache write until the test releases it.
+        const realSave = MetadataStore.prototype.save;
+        let releaseSave: () => void = () => {};
+        const saveHeld = new Promise<void>((resolve) => {
+            releaseSave = resolve;
+        });
+        const saveSpy = vi
+            .spyOn(MetadataStore.prototype, "save")
+            .mockImplementation(async function (this: MetadataStore) {
+                await saveHeld;
+                return realSave.call(this);
+            });
+
+        const lib = await Library.open({ client, cacheDirectory });
+        try {
+            await vi.waitFor(() => expect(saveSpy).toHaveBeenCalled(), {
+                timeout: 2000,
+                interval: 5,
+            });
+
+            let closed = false;
+            const closing = lib.close().then(() => {
+                closed = true;
+            });
+            await new Promise((r) => setTimeout(r, 50));
+            expect(closed).toBe(false);
+
+            releaseSave();
+            await closing;
+            const reloaded = await MetadataStore.load(path);
+            expect(reloaded.getFile(1, 1002)?.id).toBe(1002);
+        } finally {
+            releaseSave();
+            await lib.close();
+            saveSpy.mockRestore();
+        }
+    });
+
     it("defaults cacheDirectory to the env-paths cache dir plus user id", async () => {
         const xdg = join(dir, "xdg-cache");
         const prev = process.env.XDG_CACHE_HOME;

test/library/mldata.test.ts

@@ -374,7 +374,7 @@ describe("Library ML-data fetch on refresh", () => {
             await new Promise((r) => setTimeout(r, FAST_INTERVAL * 1000 * 5));
             expect(client.mlFetchCalls.length).toBe(callsAfterFirst);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -443,7 +443,7 @@ describe("Library ML-data fetch on refresh", () => {
             expect(client.mlFetchCalls.length).toBeGreaterThan(callsBefore);
             expect(client.mlFetchCalls.flat()).toContain(1001);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 });

test/library/read.test.ts

@@ -531,7 +531,7 @@ describe("Library exposes the read surface over its live store", () => {
             expect(client.collectionsCalls).toBe(collectionsBefore);
             expect(client.filesCalls).toBe(filesBefore);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 });

test/library/snapshot.test.ts

@@ -159,7 +159,7 @@ describe("Library.snapshot and Library.subscribe", () => {
             for (const p of snap.photos) expect("key" in p).toBe(false);
             for (const a of snap.albums) expect("key" in a).toBe(false);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -219,7 +219,7 @@ describe("Library.snapshot and Library.subscribe", () => {
             expect(change.refreshedAt).toBeGreaterThan(0);
         } finally {
             unsubscribe();
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -251,7 +251,7 @@ describe("Library.snapshot and Library.subscribe", () => {
             expect(changes).toEqual([]);
         } finally {
             unsubscribe();
-            lib.close();
+            await lib.close();
         }
     });
 
@@ -297,7 +297,7 @@ describe("Library.snapshot and Library.subscribe", () => {
             );
             expect(changes).toEqual([]);
         } finally {
-            lib.close();
+            await lib.close();
         }
     });
 });