diff --git a/README.md b/README.md index 2fd00d2..b98882b 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ if (photo) { console.log(`original at ${path}`); } -lib.close(); +await lib.close(); ``` The lower-level `Client` (login, session serialization, and the raw @@ -563,7 +563,10 @@ and pass it. The three pools default to 10 / 5 / 25 (see Request pools below). `lib.status()` returns a `LibraryStatus` (collection/file counts, last 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. +idempotent, and an in-flight refresh is left to finish. The promise it returns +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 diff --git a/TODO.md b/TODO.md index 29a1c3f..5b17221 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ Tag v1.0.0. # Completed Steps +- 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), 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. diff --git a/src/cli-commands.ts b/src/cli-commands.ts index fc02d40..d72b607 100644 --- a/src/cli-commands.ts +++ b/src/cli-commands.ts @@ -194,7 +194,7 @@ export const collectionsCommand = async ( } return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -235,7 +235,7 @@ export const filesCommand = async ( } return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -272,7 +272,7 @@ export const getCommand = async ( ctx.stderr.write(`${result.bytes} bytes -> ${outPath}\n`); return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -309,7 +309,7 @@ export const getThumbCommand = async ( ctx.stderr.write(`${result.bytes} bytes -> ${outPath}\n`); return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -329,7 +329,7 @@ export const backupMetadataCommand = async ( }); return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -376,7 +376,7 @@ export const backupCommand = async ( return result.failed > 0 ? 1 : 0; } finally { - lib.close(); + await lib.close(); } }; @@ -411,7 +411,7 @@ export const listMissingThumbnailsCommand = async ( } return 0; } finally { - lib.close(); + await lib.close(); } }; @@ -481,6 +481,6 @@ export const fixMissingThumbnailsCommand = async ( return results.some((r) => r.status === "failed") ? 1 : 0; } finally { - lib.close(); + await lib.close(); } }; diff --git a/src/library/index.ts b/src/library/index.ts index 3b184cb..df1199c 100644 --- a/src/library/index.ts +++ b/src/library/index.ts @@ -266,8 +266,9 @@ export class Library { // that, a fresh read propagates it. private cycle?: Promise; // 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; private closed = false; private lastRefreshAt?: number; private lastError?: string; @@ -560,14 +561,21 @@ export class Library { } // Stop the background timer. Idempotent. An in-flight refresh is left to - // finish; it will not schedule another cycle once closed. - close(): void { + // finish; it will not schedule another cycle once closed. The returned + // 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 { 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 { @@ -631,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; @@ -745,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(); @@ -784,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; } } diff --git a/src/library/precache.ts b/src/library/precache.ts index f604eaa..472d627 100644 --- a/src/library/precache.ts +++ b/src/library/precache.ts @@ -92,9 +92,10 @@ export class Precache { private pinned = new Set(); // 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; + private originalsSweep?: Promise; 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 { 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; }); } diff --git a/test/library/content-library.test.ts b/test/library/content-library.test.ts index a89f47c..fe60236 100644 --- a/test/library/content-library.test.ts +++ b/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(); }); }); diff --git a/test/library/fresh.test.ts b/test/library/fresh.test.ts index 15b71b7..10b8ee4 100644 --- a/test/library/fresh.test.ts +++ b/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(); } }); }); diff --git a/test/library/library.test.ts b/test/library/library.test.ts index fe951c3..2669871 100644 --- a/test/library/library.test.ts +++ b/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 @@ -37,6 +38,11 @@ * short interval and `vi.waitFor`: a fake clock cannot settle the real * fsync-and-rename cache write, and empty diffs never write, so the eventual * state is stable to poll for. + * + * A refresh changes RAM before it writes the cache file, so a polled state can + * be visible while that write is still running. Every test therefore awaits + * `close()`, which waits for the in-flight refresh, before `afterEach` removes + * the directory. */ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; @@ -193,7 +199,7 @@ describe("Library.open and background refresh", () => { expect(reloaded.getFile(1, 1001)?.id).toBe(1001); expect(reloaded.collectionsSinceTime).toBe(100); } finally { - lib.close(); + await lib.close(); } }); @@ -224,7 +230,7 @@ describe("Library.open and background refresh", () => { expect(client.collectionsSinceTimes.length).toBe(collectionCalls); expect(client.filesCalls.length).toBe(fileCalls); } finally { - lib.close(); + await lib.close(); } }); @@ -271,7 +277,7 @@ describe("Library.open and background refresh", () => { { timeout: 2000, interval: 5 }, ); } finally { - lib.close(); + await lib.close(); } }); @@ -303,7 +309,7 @@ describe("Library.open and background refresh", () => { // never re-fetched. expect(client.filesCalls).toEqual([]); } finally { - lib.close(); + await lib.close(); } }); @@ -357,7 +363,7 @@ describe("Library.open and background refresh", () => { { timeout: 2000, interval: 5 }, ); } finally { - lib.close(); + await lib.close(); } }); @@ -402,7 +408,7 @@ describe("Library.open and background refresh", () => { await new Promise((r) => setTimeout(r, FAST_INTERVAL * 1000 * 4)); expect(saveSpy).toHaveBeenCalledTimes(2); } finally { - lib.close(); + await lib.close(); saveSpy.mockRestore(); } }); @@ -462,7 +468,7 @@ describe("Library.open and background refresh", () => { { timeout: 2000, interval: 5 }, ); } finally { - lib.close(); + await lib.close(); } }); @@ -488,7 +494,7 @@ describe("Library.open and background refresh", () => { ), ).toBe(true); } finally { - lib.close(); + await lib.close(); } }); @@ -502,9 +508,14 @@ describe("Library.open and background refresh", () => { seed.putFile(file(1001, 1, 400)); await seed.save(); - // The server never answers this run's first refresh. + // The server does not answer this run's first refresh until the test + // is done with it. + let answerFirstFetch: (page: CollectionsPage) => void = () => {}; const client = new MockClient(); - client.collectionsSince = () => new Promise(() => {}); + client.collectionsSince = () => + new Promise((resolve) => { + answerFirstFetch = resolve; + }); // open() must resolve from the cache without blocking on the network, // and reads must serve the seeded copy. @@ -517,7 +528,9 @@ describe("Library.open and background refresh", () => { expect(lib.status().lastRefreshAt).toBeUndefined(); expect(lib.status().lastError).toBeUndefined(); } finally { - lib.close(); + // close() waits for the outstanding refresh, so let it finish. + answerFirstFetch({ collections: [], deleted: [], cursor: 500 }); + await lib.close(); } }); @@ -564,7 +577,7 @@ describe("Library.open and background refresh", () => { expect(lib.listFiles(1).map((f) => f.id)).toEqual([1001]); expect(lib.status().lastRefreshAt).toBeGreaterThan(0); } finally { - lib.close(); + await lib.close(); } }); @@ -619,7 +632,7 @@ describe("Library.open and background refresh", () => { ); expect(reloaded.getFile(1, 1001)?.id).toBe(1001); } finally { - lib.close(); + await lib.close(); saveSpy.mockRestore(); } }); @@ -634,8 +647,8 @@ describe("Library.open and background refresh", () => { }); const callsAfterOpen = client.collectionsSinceTimes.length; - lib.close(); - lib.close(); // second close must not throw + await lib.close(); + await lib.close(); // second close must not throw expect(lib.status().closed).toBe(true); // No further refreshes fire once closed. @@ -643,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((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; @@ -659,7 +731,7 @@ describe("Library.open and background refresh", () => { expect(lib.cacheDirectory.startsWith(xdg)).toBe(true); expect(lib.cacheDirectory.endsWith(String(USER_ID))).toBe(true); } finally { - lib.close(); + await lib.close(); } } finally { if (prev === undefined) delete process.env.XDG_CACHE_HOME; diff --git a/test/library/mldata.test.ts b/test/library/mldata.test.ts index 6022db7..1dfa0c4 100644 --- a/test/library/mldata.test.ts +++ b/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,59 @@ 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(); + } + }); + + it("close() resolves only after a running ML data fetch has stored its payloads", async () => { + const client = new MLMockClient(); + client.collectionsQueue.push({ + collections: [collection(1, 100)], + deleted: [], + cursor: 100, + }); + client.filesFor(1, { + files: [file(1001, 1, 90)], + deleted: [], + cursor: 90, + }); + client.mlByFile.set(1001, payload([0.5, 0.25, 0.75])); + + // Hold the ML data fetch open until the test releases it. + let release!: () => void; + const held = new Promise((r) => (release = r)); + let fetchStarted!: () => void; + const started = new Promise((r) => (fetchStarted = r)); + const realFetch = client.fetchMLData.bind(client); + client.fetchMLData = async (args) => { + fetchStarted(); + await held; + return realFetch(args); + }; + + const lib = await Library.open({ + client, + cacheDirectory, + refreshIntervalSeconds: 3600, + }); + try { + await started; + + let closed = false; + const closing = lib.close().then(() => { + closed = true; + }); + await new Promise((r) => setTimeout(r, 20)); + expect(closed).toBe(false); + + release(); + await closing; + expect( + existsSync(join(cacheDirectory, "mldata", "1001.json")), + ).toBe(true); + } finally { + release(); + await lib.close(); } }); }); diff --git a/test/library/precache.test.ts b/test/library/precache.test.ts index 2580579..0c0e777 100644 --- a/test/library/precache.test.ts +++ b/test/library/precache.test.ts @@ -402,16 +402,64 @@ describe("Precache through Library.open", () => { } it("starts both precaches from open() and reports them in status()", async () => { - const thumbFetched = new Set(); - const origFetched = new Set(); const source: ContentSource = { - original: async ({ file: f, destination }) => { - origFetched.add(f.id); + original: async ({ destination }) => { await writeFile(destination, Buffer.alloc(10, 1)); return { bytesWritten: 10 }; }, - thumbnail: async ({ file: f, destination }) => { - thumbFetched.add(f.id); + thumbnail: async ({ destination }) => { + await writeFile(destination, Buffer.alloc(10, 1)); + return { bytesWritten: 10 }; + }, + }; + // Each fill reports "done" once the cache has recorded its files. The + // source returning is not enough: the cache records a file only after + // it has checked it on disk. + const finished = new Set(); + let bothFinished!: () => void; + const precached = new Promise((r) => (bothFinished = r)); + const lib = await Library.open({ + client: new MockClient(), + cacheDirectory: join(root, "cache"), + contentSource: source, + refreshIntervalSeconds: 3600, + onProgress: (e) => { + if ( + e.status === "done" && + (e.operation === "precacheThumbnails" || + e.operation === "precacheOriginals") + ) { + finished.add(e.operation); + if (finished.size === 2) bothFinished(); + } + }, + }); + + // Every file's thumbnail is precached; the favorite (file 3) and the + // week's files (1, 2) all have their originals precached. + await precached; + const status = lib.status(); + expect(status.thumbnailsTotal).toBe(3); + expect(status.thumbnailsCached).toBe(3); + expect(status.originalsPinned).toBe(3); + expect(status.originalsCached).toBe(3); + await lib.close(); + }); + + it("close() resolves only after a running precache fetch has written its file", async () => { + // Every thumbnail fetch waits until the test releases it. + let release!: () => void; + const held = new Promise((r) => (release = r)); + let fetchStarted!: (destination: string) => void; + const started = new Promise((r) => (fetchStarted = r)); + const source: ContentSource = { + original: async ({ destination }) => { + await writeFile(destination, Buffer.alloc(10, 1)); + return { bytesWritten: 10 }; + }, + thumbnail: async ({ destination }) => { + fetchStarted(destination); + await held; await writeFile(destination, Buffer.alloc(10, 1)); return { bytesWritten: 10 }; }, @@ -421,16 +469,24 @@ describe("Precache through Library.open", () => { cacheDirectory: join(root, "cache"), contentSource: source, refreshIntervalSeconds: 3600, + precacheOriginals: false, }); + try { + const destination = await started; - // Every file's thumbnail is precached; the favorite (file 3) and the - // week's files (1, 2) all have their originals precached. - await until(() => thumbFetched.size === 3 && origFetched.size === 3); - const status = lib.status(); - expect(status.thumbnailsTotal).toBe(3); - expect(status.thumbnailsCached).toBe(3); - expect(status.originalsPinned).toBe(3); - expect(status.originalsCached).toBe(3); - lib.close(); + let closed = false; + const closing = lib.close().then(() => { + closed = true; + }); + await new Promise((r) => setTimeout(r, 20)); + expect(closed).toBe(false); + + release(); + await closing; + expect(existsSync(destination)).toBe(true); + } finally { + release(); + await lib.close(); + } }); }); diff --git a/test/library/read.test.ts b/test/library/read.test.ts index 102bdee..2fe394a 100644 --- a/test/library/read.test.ts +++ b/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(); } }); }); diff --git a/test/library/snapshot.test.ts b/test/library/snapshot.test.ts index 511b4c4..c0f306c 100644 --- a/test/library/snapshot.test.ts +++ b/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(); } }); });