From 9f2e827e55161e928b52fff9aa0033e990c3b2dc Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 2 Aug 2026 22:25:32 +0200 Subject: [PATCH] fix(downloads): preserve retained partial ownership --- CLAUDE.md | 10 +- .../database/download-partial-cleanup.spec.ts | 79 +++++++++ .../database/download-partial-cleanup.ts | 110 ++++++++++++ .../events/database/download-requests.spec.ts | 157 +++++++++--------- .../app/events/database/download-requests.ts | 22 +-- docs/architecture/download-manager.md | 10 +- 6 files changed, 298 insertions(+), 90 deletions(-) create mode 100644 apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts create mode 100644 apps/electron-backend/src/app/events/database/download-partial-cleanup.ts diff --git a/CLAUDE.md b/CLAUDE.md index 776c8cf31..4b3e852d3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -958,9 +958,13 @@ engine` (restart required) or settles, so later callers have independent bounded waits without duplicating stalled native work. Only `ENOENT` and `ENOTDIR` prove absence; permission, I/O, and other filesystem errors remain unknown and cannot clear a completed - row. 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 + 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 + 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 snapshot succeeds. - Episode ownership uses normalized `episode.id` as the canonical `xtreamId` 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 new file mode 100644 index 000000000..2c5a05f73 --- /dev/null +++ b/apps/electron-backend/src/app/events/database/download-partial-cleanup.spec.ts @@ -0,0 +1,79 @@ +import { + createPartialDownloadCleanup, + removePartialDownloadFileWithTimeoutAsync, +} from './download-partial-cleanup'; + +describe('bounded partial download cleanup', () => { + it.each([ + ['EACCES', 'unknown'], + ['EIO', 'unknown'], + ['ENOENT', 'missing'], + ['ENOTDIR', 'missing'], + ] as const)( + 'classifies an %s unlink result as %s', + async (code, expected) => { + const error = Object.assign(new Error(code), { code }); + const unlink = jest.fn(async () => { + throw error; + }); + const cleanup = createPartialDownloadCleanup(unlink); + + await expect( + removePartialDownloadFileWithTimeoutAsync( + '/downloads/episode.mp4', + 25, + cleanup + ) + ).resolves.toBe(expected); + expect(unlink).toHaveBeenCalledWith('/downloads/episode.mp4.part'); + } + ); + + it('removes the partial asynchronously', async () => { + const unlink = jest.fn(async () => undefined); + const cleanup = createPartialDownloadCleanup(unlink); + + await expect( + removePartialDownloadFileWithTimeoutAsync( + '/downloads/episode.mp4', + 25, + cleanup + ) + ).resolves.toBe('removed'); + expect(unlink).toHaveBeenCalledWith('/downloads/episode.mp4.part'); + }); + + it('bounds each caller while retaining and coalescing the raw cleanup', async () => { + jest.useFakeTimers(); + try { + let finish: (() => void) | undefined; + const unlink = jest.fn( + () => + new Promise((resolve) => { + finish = resolve; + }) + ); + const cleanup = createPartialDownloadCleanup(unlink, 1); + const first = removePartialDownloadFileWithTimeoutAsync( + '/downloads/episode.mp4', + 25, + cleanup + ); + const second = removePartialDownloadFileWithTimeoutAsync( + '/downloads/episode.mp4', + 50, + cleanup + ); + + await jest.advanceTimersByTimeAsync(25); + await expect(first).resolves.toBe('unknown'); + expect(unlink).toHaveBeenCalledTimes(1); + + finish?.(); + await expect(second).resolves.toBe('removed'); + 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 new file mode 100644 index 000000000..a038fd2a6 --- /dev/null +++ b/apps/electron-backend/src/app/events/database/download-partial-cleanup.ts @@ -0,0 +1,110 @@ +import { unlink } from 'node:fs/promises'; +import { getPartialDownloadPath } from './download-file-path'; + +export type DownloadPartialCleanupResult = 'removed' | 'missing' | 'unknown'; + +export type DownloadAsyncUnlink = (filePath: string) => Promise; + +export type DownloadPartialCleanup = ( + filePath: string +) => Promise; + +const DEFAULT_MAX_CONCURRENT_PARTIAL_CLEANUPS = 4; +const DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS = 1_000; + +function isMissingFileSystemError(error: unknown): boolean { + if (!error || typeof error !== 'object' || !('code' in error)) { + return false; + } + const code = (error as { code?: unknown }).code; + return code === 'ENOENT' || code === 'ENOTDIR'; +} + +export function createPartialDownloadCleanup( + asyncUnlink: DownloadAsyncUnlink = unlink, + maxConcurrent = DEFAULT_MAX_CONCURRENT_PARTIAL_CLEANUPS +): DownloadPartialCleanup { + const concurrency = Math.max(1, Math.floor(maxConcurrent)); + const pending: Array<() => void> = []; + const inFlight = new Map>(); + let active = 0; + + const acquire = (): Promise => { + if (active < concurrency) { + active += 1; + return Promise.resolve(); + } + return new Promise((resolve) => pending.push(resolve)); + }; + + const release = (): void => { + const next = pending.shift(); + if (next) { + next(); + } else { + active -= 1; + } + }; + + const remove = async ( + filePath: string + ): Promise => { + await acquire(); + try { + await asyncUnlink(getPartialDownloadPath(filePath)); + return 'removed'; + } catch (error) { + return isMissingFileSystemError(error) ? 'missing' : 'unknown'; + } finally { + release(); + } + }; + + return (filePath: string) => { + const existing = inFlight.get(filePath); + if (existing) { + return existing; + } + + const cleanup = remove(filePath); + inFlight.set(filePath, cleanup); + const clear = () => { + if (inFlight.get(filePath) === cleanup) { + inFlight.delete(filePath); + } + }; + void cleanup.then(clear, clear); + return cleanup; + }; +} + +const cleanupPartialDownloadFile = createPartialDownloadCleanup(); + +export async function removePartialDownloadFileWithTimeoutAsync( + filePath: string | null | undefined, + timeoutMs = DEFAULT_PARTIAL_CLEANUP_TIMEOUT_MS, + cleanup: DownloadPartialCleanup = cleanupPartialDownloadFile +): Promise { + if (!filePath) { + return 'missing'; + } + + const boundedTimeoutMs = + Number.isFinite(timeoutMs) && timeoutMs >= 0 + ? timeoutMs + : 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]); + } 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 65c8c6e71..ef463c463 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 @@ -887,82 +887,91 @@ describe('download requests resume', () => { }); }); - it('deletes the retained partial before re-downloading a failed row from scratch', async () => { - jest.resetModules(); + it.each(['failed', 'canceled'] as const)( + 'deletes the retained partial asynchronously before re-downloading a %s row from scratch', + async (status) => { + jest.resetModules(); - const failedRow = { - contentType: 'vod', - filePath: '/downloads/movie.mp4', - id: 42, - playlistId: 'playlist-1', - status: 'failed', - title: 'Movie', - url: 'https://example.test/movie.mp4', - xtreamId: 7, - }; - const limit = jest - .fn() - .mockResolvedValueOnce([{ id: 'playlist-1' }]) - .mockResolvedValueOnce([failedRow]); - const set = jest.fn<{ where: jest.Mock }, [Record]>( - () => ({ + const terminalRow = { + contentType: 'vod', + filePath: '/downloads/movie.mp4', + id: 42, + playlistId: 'playlist-1', + status, + title: 'Movie', + url: 'https://example.test/movie.mp4', + xtreamId: 7, + }; + const limit = jest + .fn() + .mockResolvedValueOnce([{ id: 'playlist-1' }]) + .mockResolvedValueOnce([terminalRow]); + const set = jest.fn< + { where: jest.Mock }, + [Record] + >(() => ({ where: jest.fn().mockResolvedValue(undefined), - }) - ); - const db = { - select: jest.fn(() => ({ - from: jest.fn(() => ({ - where: jest.fn(() => ({ limit })), + })); + const db = { + select: jest.fn(() => ({ + from: jest.fn(() => ({ + where: jest.fn(() => ({ limit })), + })), })), - })), - update: jest.fn(() => ({ set })), - }; - const enqueueDownload = jest.fn(); - const removePartialDownloadFile = jest.fn(); - const authorizer = { - requireAuthorized: jest.fn(async (directory: string) => directory), - } as unknown as DownloadDirectoryAuthorizer; + update: jest.fn(() => ({ set })), + }; + const enqueueDownload = jest.fn(); + const removePartialDownloadFileWithTimeoutAsync = jest.fn( + async () => 'removed' as const + ); + const authorizer = { + requireAuthorized: jest.fn( + async (directory: string) => directory + ), + } as unknown as DownloadDirectoryAuthorizer; - jest.doMock('../../database/connection', () => ({ - getDatabase: jest.fn().mockResolvedValue(db), - })); - jest.doMock('../url-safety', () => ({ - assertRemoteUrlAllowed: jest.fn().mockResolvedValue(undefined), - })); - jest.doMock('./download-file-path', () => ({ - removePartialDownloadFile, - })); - jest.doMock('./download-runtime', () => ({ - enqueueDownload, - })); + jest.doMock('../../database/connection', () => ({ + getDatabase: jest.fn().mockResolvedValue(db), + })); + jest.doMock('../url-safety', () => ({ + assertRemoteUrlAllowed: jest.fn().mockResolvedValue(undefined), + })); + jest.doMock('./download-partial-cleanup', () => ({ + removePartialDownloadFileWithTimeoutAsync, + })); + jest.doMock('./download-runtime', () => ({ + enqueueDownload, + })); - const { startDownloadRequest } = await import('./download-requests'); + const { startDownloadRequest } = + await import('./download-requests'); - await expect( - startDownloadRequest( - { - contentType: 'vod', - downloadFolder: '/downloads', - playlistId: 'playlist-1', - title: 'Movie', - url: 'https://example.test/movie.mp4', - xtreamId: 7, - }, - authorizer - ) - ).resolves.toEqual({ id: 42, success: true }); + await expect( + startDownloadRequest( + { + contentType: 'vod', + downloadFolder: '/downloads', + playlistId: 'playlist-1', + title: 'Movie', + url: 'https://example.test/movie.mp4', + xtreamId: 7, + }, + authorizer + ) + ).resolves.toEqual({ id: 42, success: true }); - expect(removePartialDownloadFile).toHaveBeenCalledWith( - '/downloads/movie.mp4' - ); - expect(set).toHaveBeenCalledWith( - expect.objectContaining({ - filePath: null, - resumeValidator: null, - status: 'queued', - }) - ); - }); + expect( + removePartialDownloadFileWithTimeoutAsync + ).toHaveBeenCalledWith('/downloads/movie.mp4'); + expect(set).toHaveBeenCalledWith( + expect.objectContaining({ + filePath: null, + resumeValidator: null, + status: 'queued', + }) + ); + } + ); it('fails the re-download when the retained partial cannot be deleted', async () => { jest.resetModules(); @@ -993,9 +1002,9 @@ describe('download requests resume', () => { update: jest.fn(() => ({ set })), }; const enqueueDownload = jest.fn(); - const removePartialDownloadFile = jest.fn(() => { - throw new Error('EPERM: locked'); - }); + const removePartialDownloadFileWithTimeoutAsync = jest.fn( + async () => 'unknown' as const + ); const authorizer = { requireAuthorized: jest.fn(async (directory: string) => directory), } as unknown as DownloadDirectoryAuthorizer; @@ -1006,8 +1015,8 @@ describe('download requests resume', () => { jest.doMock('../url-safety', () => ({ assertRemoteUrlAllowed: jest.fn().mockResolvedValue(undefined), })); - jest.doMock('./download-file-path', () => ({ - removePartialDownloadFile, + jest.doMock('./download-partial-cleanup', () => ({ + removePartialDownloadFileWithTimeoutAsync, })); 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 908c6548f..6db2bb7f7 100644 --- a/apps/electron-backend/src/app/events/database/download-requests.ts +++ b/apps/electron-backend/src/app/events/database/download-requests.ts @@ -10,8 +10,8 @@ import { getDatabase } from '../../database/connection'; import * as schema from '../../database/schema'; import { assertRemoteUrlAllowed } from '../url-safety'; import { DownloadDirectoryAuthorizer } from './download-directory-authorization'; -import { removePartialDownloadFile } from './download-file-path'; import { getDownloadFileAvailabilityWithTimeoutAsync } from './download-file-availability'; +import { removePartialDownloadFileWithTimeoutAsync } from './download-partial-cleanup'; import { resolveExistingDownloadIdentity } from './download-request-identity'; import { resolveStoredDownloadHeaders } from './download-request-headers'; import { @@ -175,17 +175,19 @@ export async function startDownloadRequest( }; } - if (item.status === 'failed' && item.filePath) { - // A failed row can still reference a retained .part; delete it + if ( + ['completed', 'failed', 'canceled'].includes(item.status) && + item.filePath + ) { + // A terminal row can still reference a retained .part; delete it // before the restart clears filePath, or the file is orphaned. - // A locked .part must keep its database owner, so fail the - // restart instead of proceeding without the cleanup. - try { - removePartialDownloadFile(item.filePath); - } catch (error) { + // An unavailable or slow .part must keep its database owner. + const cleanup = await removePartialDownloadFileWithTimeoutAsync( + item.filePath + ); + if (cleanup === 'unknown') { console.error( - '[Downloads] Failed to delete retained partial before re-download:', - error + '[Downloads] Could not verify retained partial cleanup' ); return { error: 'Could not delete the previous partial file', diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index 38a0868f2..50baef993 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -109,9 +109,13 @@ variants, contextual buttons, and theme-aware styling. actually settles, so later callers get independent bounded waits without duplicating stalled native work. Only `ENOENT` and `ENOTDIR` are authoritative absence; permission, I/O, and other probe errors remain unknown, so - `DOWNLOADS_START` leaves the completed row and file path untouched. The - coordinator counts both stable duplicate reasons as skipped. There is no - batch IPC, + `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, parallel transfer, or queue reordering: destination authorization, persisted header handling, and the backend's one-active-transfer FIFO semantics remain unchanged.