From 991ee5d05c2cc1e7985d91347029ba2948525875 Mon Sep 17 00:00:00 2001 From: Nicolas Meienberger Date: Fri, 5 Jun 2026 18:03:30 +0200 Subject: [PATCH] refactor: get shared lock after semaphore take --- .../__tests__/snapshot-tree-browser.test.tsx | 23 ------- .../__tests__/repositories.service.test.ts | 46 ++++++++++++++ .../repositories/repositories.service.ts | 63 ++++++++++--------- 3 files changed, 80 insertions(+), 52 deletions(-) diff --git a/app/client/components/file-browsers/__tests__/snapshot-tree-browser.test.tsx b/app/client/components/file-browsers/__tests__/snapshot-tree-browser.test.tsx index 352e167f..23df6252 100644 --- a/app/client/components/file-browsers/__tests__/snapshot-tree-browser.test.tsx +++ b/app/client/components/file-browsers/__tests__/snapshot-tree-browser.test.tsx @@ -224,29 +224,6 @@ describe("SnapshotTreeBrowser", () => { }); }); - test("prefetches using the query path when display and query roots differ", async () => { - const requests = mockListSnapshotFiles(); - - renderSnapshotTreeBrowser(); - - const row = await screen.findByRole("button", { name: "project" }); - const initialRequestCount = requests.length; - - await userEvent.hover(row); - - await waitFor(() => { - expect(requests.length).toBe(initialRequestCount + 1); - }); - - expect(requests.at(-1)).toEqual({ - shortId: "repo-1", - snapshotId: "snap-1", - path: "/mnt/project", - offset: "0", - limit: "500", - }); - }); - test("shows the query root contents when display and query roots differ", async () => { mockListSnapshotFiles(); diff --git a/app/server/modules/repositories/__tests__/repositories.service.test.ts b/app/server/modules/repositories/__tests__/repositories.service.test.ts index 2395529c..9d50bf5b 100644 --- a/app/server/modules/repositories/__tests__/repositories.service.test.ts +++ b/app/server/modules/repositories/__tests__/repositories.service.test.ts @@ -218,7 +218,11 @@ describe("repositoriesService.listSnapshotFiles", () => { let active = 0; let maxActive = 0; let releaseAll = false; + let exclusiveAcquired = false; + let releaseExclusive: (() => void) | undefined; + let exclusivePromise: Promise<() => void> | undefined; const releaseWaiters: Array<() => void> = []; + const exclusiveController = new AbortController(); const releaseWaitingCommands = () => { const waiters = releaseWaiters.splice(0); @@ -227,6 +231,25 @@ describe("repositoriesService.listSnapshotFiles", () => { } }; + const resolveWithin = async (promise: Promise, timeoutMs: number) => { + return await new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + reject(new Error(`Expected promise to resolve within ${timeoutMs}ms`)); + }, timeoutMs); + + promise.then( + (value) => { + clearTimeout(timeout); + resolve(value); + }, + (error) => { + clearTimeout(timeout); + reject(error); + }, + ); + }); + }; + const lsSpy = vi.spyOn(restic, "ls").mockImplementation((_config, snapshotId, _path, options) => Effect.promise(async () => { active++; @@ -277,8 +300,23 @@ describe("repositoriesService.listSnapshotFiles", () => { }); expect(maxActive).toBe(2); + exclusivePromise = repoMutex + .acquireExclusive(repository.id, "delete", exclusiveController.signal) + .then((release) => { + exclusiveAcquired = true; + releaseExclusive = release; + return release; + }); + releaseWaitingCommands(); + releaseExclusive = await resolveWithin(exclusivePromise, 2000); + expect(exclusiveAcquired).toBe(true); + expect(active).toBe(0); + + releaseExclusive(); + releaseExclusive = undefined; + await waitForExpect(() => { expect(releaseWaiters).toHaveLength(2); }); @@ -287,9 +325,17 @@ describe("repositoriesService.listSnapshotFiles", () => { releaseWaitingCommands(); await Promise.all(calls); } finally { + if (releaseExclusive) { + releaseExclusive(); + } else { + exclusiveController.abort(); + } releaseAll = true; releaseWaitingCommands(); await Promise.allSettled(calls); + if (exclusivePromise) { + await Promise.allSettled([exclusivePromise]); + } } expect(lsSpy).toHaveBeenCalledTimes(4); diff --git a/app/server/modules/repositories/repositories.service.ts b/app/server/modules/repositories/repositories.service.ts index 1b9ce296..c0343ede 100644 --- a/app/server/modules/repositories/repositories.service.ts +++ b/app/server/modules/repositories/repositories.service.ts @@ -103,7 +103,7 @@ const updateActiveRestoreTask = (restoreId: string, eventName: string, update: ( const getLsLimiter = (repositoryId: string) => { let limiter = lsLimiters.get(repositoryId); if (!limiter) { - limiter = Effect.unsafeMakeSemaphore(2); + limiter = Effect.runSync(Effect.makeSemaphore(2)); lsLimiters.set(repositoryId, limiter); } return limiter; @@ -479,38 +479,43 @@ const listSnapshotFiles = async ( }; } - const releaseLock = await repoMutex.acquireShared(repository.id, `ls:${snapshotId}`); + const limiter = getLsLimiter(repository.id); + await runEffectPromise(limiter.take(1)); + try { - const result = await runEffectPromise( - restic - .ls(repository.config, snapshotId, path, { organizationId, offset, limit }) - .pipe(getLsLimiter(repository.id).withPermits(1)), - ); + const releaseLock = await repoMutex.acquireShared(repository.id, `ls:${snapshotId}`); + try { + const result = await runEffectPromise( + restic.ls(repository.config, snapshotId, path, { organizationId, offset, limit }), + ); - if (!result.snapshot) { - throw new NotFoundError("Snapshot not found or empty"); + if (!result.snapshot) { + throw new NotFoundError("Snapshot not found or empty"); + } + + const response = { + snapshot: { + id: result.snapshot.id, + short_id: result.snapshot.short_id, + time: result.snapshot.time, + hostname: result.snapshot.hostname, + paths: result.snapshot.paths, + }, + files: result.nodes, + offset: result.pagination.offset, + limit: result.pagination.limit, + total: result.pagination.total, + hasMore: result.pagination.hasMore, + }; + + cache.set(cacheKey, result); + + return response; + } finally { + releaseLock(); } - - const response = { - snapshot: { - id: result.snapshot.id, - short_id: result.snapshot.short_id, - time: result.snapshot.time, - hostname: result.snapshot.hostname, - paths: result.snapshot.paths, - }, - files: result.nodes, - offset: result.pagination.offset, - limit: result.pagination.limit, - total: result.pagination.total, - hasMore: result.pagination.hasMore, - }; - - cache.set(cacheKey, result); - - return response; } finally { - releaseLock(); + await runEffectPromise(limiter.release(1)); } };