diff --git a/CLAUDE.md b/CLAUDE.md index 4b3e852d3..956af1aad 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -959,10 +959,12 @@ engine` (restart required) or stalled native work. Only `ENOENT` and `ENOTDIR` prove absence; permission, I/O, and other filesystem errors remain unknown and cannot clear a completed row. Before a completed-missing, failed, or canceled row clears its retained - path, the start IPC asynchronously removes any `.part` through a separate - one-second, same-path-coalesced, four-operation cap. Timeout or non-absence - errors keep the row's ownership intact; `ENOENT` and `ENOTDIR` safely proceed. - Episode and season download actions require an authoritative global list. A + path, the start IPC asynchronously removes any `.part` through a separate, + same-path-coalesced, four-operation cap. A one-second admission deadline + rejects queued work before unlink starts; started work is awaited so it cannot + mutate after a failure response. Non-absence errors keep the row's ownership + intact; `ENOENT` and `ENOTDIR` safely proceed. Episode and season download + actions require an authoritative global list. A successful snapshot remains authoritative while a later background refresh is in flight; a latest refresh failure leaves loading/empty-state resolution intact but disables starts until another diff --git a/apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts b/apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts index 2c5a05f73..4a9f7994b 100644 --- a/apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts @@ -1,6 +1,6 @@ import { createPartialDownloadCleanup, - removePartialDownloadFileWithTimeoutAsync, + removePartialDownloadFileAsync, } from './download-partial-cleanup'; describe('bounded partial download cleanup', () => { @@ -19,7 +19,7 @@ describe('bounded partial download cleanup', () => { const cleanup = createPartialDownloadCleanup(unlink); await expect( - removePartialDownloadFileWithTimeoutAsync( + removePartialDownloadFileAsync( '/downloads/episode.mp4', 25, cleanup @@ -34,7 +34,7 @@ describe('bounded partial download cleanup', () => { const cleanup = createPartialDownloadCleanup(unlink); await expect( - removePartialDownloadFileWithTimeoutAsync( + removePartialDownloadFileAsync( '/downloads/episode.mp4', 25, cleanup @@ -43,7 +43,7 @@ describe('bounded partial download cleanup', () => { expect(unlink).toHaveBeenCalledWith('/downloads/episode.mp4.part'); }); - it('bounds each caller while retaining and coalescing the raw cleanup', async () => { + it('waits for a started unlink and coalesces same-path callers', async () => { jest.useFakeTimers(); try { let finish: (() => void) | undefined; @@ -54,26 +54,67 @@ describe('bounded partial download cleanup', () => { }) ); const cleanup = createPartialDownloadCleanup(unlink, 1); - const first = removePartialDownloadFileWithTimeoutAsync( + const first = removePartialDownloadFileAsync( '/downloads/episode.mp4', 25, cleanup ); - const second = removePartialDownloadFileWithTimeoutAsync( + const second = removePartialDownloadFileAsync( '/downloads/episode.mp4', 50, cleanup ); + let firstSettled = false; + void first.then(() => { + firstSettled = true; + }); await jest.advanceTimersByTimeAsync(25); - await expect(first).resolves.toBe('unknown'); + expect(firstSettled).toBe(false); expect(unlink).toHaveBeenCalledTimes(1); finish?.(); + await expect(first).resolves.toBe('removed'); await expect(second).resolves.toBe('removed'); expect(unlink).toHaveBeenCalledTimes(1); } finally { jest.useRealTimers(); } }); + + it('times out queued cleanup without unlinking it later', async () => { + jest.useFakeTimers(); + try { + let finishFirst: (() => void) | undefined; + const unlink = jest.fn((partialPath: string) => + partialPath.includes('first') + ? new Promise((resolve) => { + finishFirst = resolve; + }) + : Promise.resolve() + ); + const cleanup = createPartialDownloadCleanup(unlink, 1); + const first = removePartialDownloadFileAsync( + '/downloads/first.mp4', + 25, + cleanup + ); + const queued = removePartialDownloadFileAsync( + '/downloads/queued.mp4', + 25, + cleanup + ); + + await jest.advanceTimersByTimeAsync(25); + await expect(queued).resolves.toBe('unknown'); + expect(unlink).toHaveBeenCalledTimes(1); + + finishFirst?.(); + await expect(first).resolves.toBe('removed'); + await Promise.resolve(); + expect(unlink).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } + }); }); diff --git a/apps/electron-backend/src/app/events/database/download-partial-cleanup.ts b/apps/electron-backend/src/app/events/database/download-partial-cleanup.ts index a038fd2a6..ea7af5846 100644 --- a/apps/electron-backend/src/app/events/database/download-partial-cleanup.ts +++ b/apps/electron-backend/src/app/events/database/download-partial-cleanup.ts @@ -6,7 +6,8 @@ export type DownloadPartialCleanupResult = 'removed' | 'missing' | 'unknown'; export type DownloadAsyncUnlink = (filePath: string) => Promise; export type DownloadPartialCleanup = ( - filePath: string + filePath: string, + admissionTimeoutMs?: number ) => Promise; const DEFAULT_MAX_CONCURRENT_PARTIAL_CLEANUPS = 4; @@ -26,15 +27,39 @@ export function createPartialDownloadCleanup( ): DownloadPartialCleanup { const concurrency = Math.max(1, Math.floor(maxConcurrent)); const pending: Array<() => void> = []; + // Only admission can time out. Once unlink starts, every coalesced caller + // awaits its authoritative result so no late deletion can race a retry. const inFlight = new Map>(); let active = 0; - const acquire = (): Promise => { + const acquire = (timeoutMs: number): Promise => { if (active < concurrency) { active += 1; - return Promise.resolve(); + return Promise.resolve(true); } - return new Promise((resolve) => pending.push(resolve)); + return new Promise((resolve) => { + let settled = false; + const start = () => { + if (settled) { + return; + } + settled = true; + clearTimeout(timeout); + resolve(true); + }; + const timeout = setTimeout(() => { + if (settled) { + return; + } + settled = true; + const index = pending.indexOf(start); + if (index >= 0) { + pending.splice(index, 1); + } + resolve(false); + }, timeoutMs); + pending.push(start); + }); }; const release = (): void => { @@ -47,9 +72,12 @@ export function createPartialDownloadCleanup( }; const remove = async ( - filePath: string + filePath: string, + admissionTimeoutMs: number ): Promise => { - await acquire(); + if (!(await acquire(admissionTimeoutMs))) { + return 'unknown'; + } try { await asyncUnlink(getPartialDownloadPath(filePath)); return 'removed'; @@ -60,13 +88,16 @@ export function createPartialDownloadCleanup( } }; - return (filePath: string) => { + return ( + filePath: string, + admissionTimeoutMs = DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS + ) => { const existing = inFlight.get(filePath); if (existing) { return existing; } - const cleanup = remove(filePath); + const cleanup = remove(filePath, admissionTimeoutMs); inFlight.set(filePath, cleanup); const clear = () => { if (inFlight.get(filePath) === cleanup) { @@ -80,31 +111,23 @@ export function createPartialDownloadCleanup( const cleanupPartialDownloadFile = createPartialDownloadCleanup(); -export async function removePartialDownloadFileWithTimeoutAsync( +export async function removePartialDownloadFileAsync( filePath: string | null | undefined, - timeoutMs = DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS, + admissionTimeoutMs = DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS, cleanup: DownloadPartialCleanup = cleanupPartialDownloadFile ): Promise { if (!filePath) { return 'missing'; } - const boundedTimeoutMs = - Number.isFinite(timeoutMs) && timeoutMs >= 0 - ? timeoutMs + const boundedAdmissionTimeoutMs = + Number.isFinite(admissionTimeoutMs) && admissionTimeoutMs >= 0 + ? admissionTimeoutMs : DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS; - let timeout: ReturnType | undefined; - const timedOut = new Promise<'unknown'>((resolve) => { - timeout = setTimeout(() => resolve('unknown'), boundedTimeoutMs); - }); try { - return await Promise.race([cleanup(filePath), timedOut]); + return await cleanup(filePath, boundedAdmissionTimeoutMs); } catch { return 'unknown'; - } finally { - if (timeout !== undefined) { - clearTimeout(timeout); - } } } diff --git a/apps/electron-backend/src/app/events/database/download-requests.spec.ts b/apps/electron-backend/src/app/events/database/download-requests.spec.ts index ef463c463..6a24e7481 100644 --- a/apps/electron-backend/src/app/events/database/download-requests.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-requests.spec.ts @@ -921,7 +921,7 @@ describe('download requests resume', () => { update: jest.fn(() => ({ set })), }; const enqueueDownload = jest.fn(); - const removePartialDownloadFileWithTimeoutAsync = jest.fn( + const removePartialDownloadFileAsync = jest.fn( async () => 'removed' as const ); const authorizer = { @@ -937,7 +937,7 @@ describe('download requests resume', () => { assertRemoteUrlAllowed: jest.fn().mockResolvedValue(undefined), })); jest.doMock('./download-partial-cleanup', () => ({ - removePartialDownloadFileWithTimeoutAsync, + removePartialDownloadFileAsync, })); jest.doMock('./download-runtime', () => ({ enqueueDownload, @@ -960,9 +960,9 @@ describe('download requests resume', () => { ) ).resolves.toEqual({ id: 42, success: true }); - expect( - removePartialDownloadFileWithTimeoutAsync - ).toHaveBeenCalledWith('/downloads/movie.mp4'); + expect(removePartialDownloadFileAsync).toHaveBeenCalledWith( + '/downloads/movie.mp4' + ); expect(set).toHaveBeenCalledWith( expect.objectContaining({ filePath: null, @@ -1002,7 +1002,7 @@ describe('download requests resume', () => { update: jest.fn(() => ({ set })), }; const enqueueDownload = jest.fn(); - const removePartialDownloadFileWithTimeoutAsync = jest.fn( + const removePartialDownloadFileAsync = jest.fn( async () => 'unknown' as const ); const authorizer = { @@ -1016,7 +1016,7 @@ describe('download requests resume', () => { assertRemoteUrlAllowed: jest.fn().mockResolvedValue(undefined), })); jest.doMock('./download-partial-cleanup', () => ({ - removePartialDownloadFileWithTimeoutAsync, + removePartialDownloadFileAsync, })); jest.doMock('./download-runtime', () => ({ enqueueDownload, diff --git a/apps/electron-backend/src/app/events/database/download-requests.ts b/apps/electron-backend/src/app/events/database/download-requests.ts index 6db2bb7f7..0ac39c23d 100644 --- a/apps/electron-backend/src/app/events/database/download-requests.ts +++ b/apps/electron-backend/src/app/events/database/download-requests.ts @@ -11,7 +11,7 @@ import * as schema from '../../database/schema'; import { assertRemoteUrlAllowed } from '../url-safety'; import { DownloadDirectoryAuthorizer } from './download-directory-authorization'; import { getDownloadFileAvailabilityWithTimeoutAsync } from './download-file-availability'; -import { removePartialDownloadFileWithTimeoutAsync } from './download-partial-cleanup'; +import { removePartialDownloadFileAsync } from './download-partial-cleanup'; import { resolveExistingDownloadIdentity } from './download-request-identity'; import { resolveStoredDownloadHeaders } from './download-request-headers'; import { @@ -182,9 +182,7 @@ export async function startDownloadRequest( // A terminal row can still reference a retained .part; delete it // before the restart clears filePath, or the file is orphaned. // An unavailable or slow .part must keep its database owner. - const cleanup = await removePartialDownloadFileWithTimeoutAsync( - item.filePath - ); + const cleanup = await removePartialDownloadFileAsync(item.filePath); if (cleanup === 'unknown') { console.error( '[Downloads] Could not verify retained partial cleanup' diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index 50baef993..9bc2e7c05 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -111,11 +111,13 @@ variants, contextual buttons, and theme-aware styling. absence; permission, I/O, and other probe errors remain unknown, so `DOWNLOADS_START` leaves the completed row and file path untouched. Before a completed-missing, failed, or canceled row clears its path, the same start IPC - asynchronously removes any retained `.part`. This cleanup also has a - one-second per-caller deadline, coalesces same-path work, and allows at most - four underlying unlinks; timeout, permission, and I/O failures keep the row's - ownership intact, while `ENOENT` and `ENOTDIR` safely proceed. The coordinator - counts both stable duplicate reasons as skipped. There is no batch IPC, + asynchronously removes any retained `.part`. Cleanup coalesces same-path work + and allows at most four underlying unlinks. A one-second admission deadline + rejects queued work before it can mutate the filesystem; once an unlink + starts, the request awaits its authoritative result so no late side effect can + race a retry. Permission and I/O failures keep the row's ownership intact, + while `ENOENT` and `ENOTDIR` safely proceed. The coordinator counts both stable + duplicate reasons as skipped. There is no batch IPC, parallel transfer, or queue reordering: destination authorization, persisted header handling, and the backend's one-active-transfer FIFO semantics remain unchanged.