From 45d53712507cfc5153bd1dd922dadba15604b81a Mon Sep 17 00:00:00 2001 From: 4gray Date: Tue, 8 Sep 2026 06:00:46 +0200 Subject: [PATCH] fix(downloads): require durable ownership before cleanup relocation --- .../database/download-catchup-cleanup.spec.ts | 4 +- .../database/download-catchup-cleanup.ts | 3 +- .../database/download-catchup-removal.spec.ts | 23 +++++++ .../database/download-catchup-removal.ts | 9 +-- .../download-catchup-reservation.spec.ts | 64 +++++++++++++++++-- .../database/download-catchup-reservation.ts | 19 ++++-- docs/architecture/download-manager.md | 12 ++-- 7 files changed, 113 insertions(+), 21 deletions(-) diff --git a/apps/electron-backend/src/app/events/database/download-catchup-cleanup.spec.ts b/apps/electron-backend/src/app/events/database/download-catchup-cleanup.spec.ts index 69dc1edc3..3b442bbdd 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-cleanup.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-cleanup.spec.ts @@ -78,7 +78,9 @@ it.each(['EEXIST', 'ENOTSUP'])( .spyOn(console, 'warn') .mockImplementation(() => undefined); try { - await cleanupCatchupFile(path, identity); + await expect(cleanupCatchupFile(path, identity)).rejects.toThrow( + 'cannot restore' + ); const quarantine = (await readdir(directory)).find((entry) => entry.startsWith('.iptvnator-cleanup-') ); diff --git a/apps/electron-backend/src/app/events/database/download-catchup-cleanup.ts b/apps/electron-backend/src/app/events/database/download-catchup-cleanup.ts index 9df6aa236..fb38e1f9e 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-cleanup.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-cleanup.ts @@ -25,11 +25,12 @@ export async function cleanupCatchupFile( try { await link(captured, path); await unlink(captured); - } catch { + } catch (error) { console.warn( '[Downloads] Replaced file retained for recovery:', captured ); + throw error; } } } finally { diff --git a/apps/electron-backend/src/app/events/database/download-catchup-removal.spec.ts b/apps/electron-backend/src/app/events/database/download-catchup-removal.spec.ts index 5ba32d383..36c4a092d 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-removal.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-removal.spec.ts @@ -337,3 +337,26 @@ it('never truncates a preexisting file without expected ownership', async () => expect(record).not.toHaveBeenCalled(); expect(readFileSync(filePath + '.part', 'utf8')).toBe('owned bytes'); }); + +it.each([false, true])( + 'does not relocate an unproved fallback target (replaced=%s)', + async (replaced) => { + writeFileSync(filePath, ''); + const createdIdentity = lstatSync(filePath); + if (replaced) { + renameSync(filePath, join(directory, 'empty-original')); + writeFileSync(filePath, 'foreign final'); + } + const db = { + select: () => ({ from: () => ({ where: async () => [] }) }), + }; + jest.mocked(renameSync).mockClear(); + await expect( + cleanupStoredCatchupFinal(db as never, 1, filePath, createdIdentity) + ).resolves.toBe(false); + expect(readFileSync(filePath, 'utf8')).toBe( + replaced ? 'foreign final' : '' + ); + expect(renameSync).not.toHaveBeenCalled(); + } +); diff --git a/apps/electron-backend/src/app/events/database/download-catchup-removal.ts b/apps/electron-backend/src/app/events/database/download-catchup-removal.ts index 4e3c509ff..b86f3234b 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-removal.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-removal.ts @@ -1,4 +1,3 @@ -import { cleanupCatchupFile } from './download-catchup-cleanup'; import { cleanupArchiveCapture } from './download-catchup-capture'; import { lstatSync, @@ -133,11 +132,9 @@ export async function cleanupStoredCatchupFinal( (createdIdentity && !sameArchiveFileIdentity(createdIdentity, proof.finalIdentity)) ) { - // Only the exclusively created empty target can precede final proof; - // no copy bytes are written until its identity has committed. - if (createdIdentity) - await cleanupCatchupFile(filePath, createdIdentity); - return true; + // No copy bytes precede final proof. Preserve an unproved empty + // destination in place: relocation needs a durable capture pointer. + return !createdIdentity; } cleanupArchiveCapture(proof); removeOwnedEntry(filePath, proof.finalIdentity, (path) => diff --git a/apps/electron-backend/src/app/events/database/download-catchup-reservation.spec.ts b/apps/electron-backend/src/app/events/database/download-catchup-reservation.spec.ts index 4dd4f708f..1ebad021e 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-reservation.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-reservation.spec.ts @@ -201,8 +201,8 @@ it.each([false, true])( }, }; const failure = new Error('SQLite write failed'); - jest.mocked(recordArchiveReservation).mockImplementationOnce( - async () => { + jest.mocked(recordArchiveReservation) + .mockImplementationOnce(async () => { if (replaced) { await rename( filePath + '.part', @@ -211,8 +211,22 @@ it.each([false, true])( await writeFile(filePath + '.part', 'foreign bytes'); } throw failure; - } - ); + }) + .mockImplementationOnce(async (_db, id, path, identity) => { + jest.mocked(readArchiveFinalizations).mockResolvedValue( + new Map([ + [ + id, + { + version: 1, + phase: 'transfer', + filePath: path, + partialIdentity: identity, + }, + ], + ]) + ); + }); try { await expect( reserveTarget({} as DownloadsDatabase, task) @@ -231,3 +245,45 @@ it.each([false, true])( } } ); + +it.each([false, true])( + 'never relocates an unjournaled reservation when SQLite remains unavailable (replaced=%s)', + async (replaced) => { + const directory = await mkdtemp(join(tmpdir(), 'archive-no-journal-')); + const filePath = join(directory, 'show.ts'); + const task: DownloadTask = { + id: 21, + directory, + fileName: 'show.ts', + url: 'https://provider.test/archive.ts', + catchup: { + channelName: 'News', + startTimestamp: 100, + stopTimestamp: 200, + }, + }; + const failure = new Error('SQLite unavailable'); + jest.mocked(recordArchiveReservation) + .mockImplementationOnce(async () => { + if (replaced) { + await rename( + filePath + '.part', + join(directory, 'original') + ); + await writeFile(filePath + '.part', 'foreign bytes'); + } + throw failure; + }) + .mockRejectedValueOnce(failure); + try { + await expect( + reserveTarget({} as DownloadsDatabase, task) + ).rejects.toBe(failure); + expect(await readFile(filePath + '.part', 'utf8')).toBe( + replaced ? 'foreign bytes' : '' + ); + } finally { + await rm(directory, { recursive: true, force: true }); + } + } +); diff --git a/apps/electron-backend/src/app/events/database/download-catchup-reservation.ts b/apps/electron-backend/src/app/events/database/download-catchup-reservation.ts index 6e6988bad..e0b6e1dcf 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-reservation.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-reservation.ts @@ -1,6 +1,5 @@ import { closeSync, fstatSync, openSync } from 'node:fs'; import { lstat } from 'node:fs/promises'; -import { cleanupCatchupPartial } from './download-catchup-cleanup'; import { cleanupStoredCatchupPartial } from './download-catchup-removal'; import { clearArchiveFinalization, @@ -77,9 +76,21 @@ export async function reserveOwnedCatchupTarget( reservation.filename ); } catch (error) { - // No media bytes have been written. Without a usable journal, remove - // only the empty reservation whose identity came from our descriptor. - await cleanupCatchupPartial(reservation.path, identity); + // Retry persistence once before cleanup. Never relocate an unjournaled + // entry: if SQLite remains unavailable, leave the empty placeholder in + // place rather than risk losing the recovery path of a replacement. + try { + await recordArchiveReservation( + db, + task.id, + reservation.path, + identity, + reservation.filename + ); + await cleanupStoredCatchupPartial(db, task.id, reservation.path); + } catch { + /* no media bytes exist before the first ownership commit */ + } throw error; } return reservation; diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index c77f49843..cd6149ce5 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -123,11 +123,13 @@ filesystems without hardlinks. Cleanup remains synchronous after the runtime guard, so a completion transition cannot interleave with unlink. Missing archive re-downloads claim a fresh reservation instead of inheriting the completed file's identity. -If the initial reservation journal write fails, descriptor-owned cleanup removes -the empty partial while preserving a replacement. A kill between exclusive -reservation/copy-file creation and its identity journal commit (or a failure of -that unjournaled cleanup) can leave an unowned **empty** destination: no bytes -are written before the commit. +If the initial reservation journal write fails, persistence is retried once +before journal-backed cleanup. If SQLite remains unavailable, the empty +reservation stays at its public path; no unjournaled entry is relocated. An +empty fallback-copy target whose final proof cannot commit is likewise preserved. +A kill between exclusive reservation/copy-file creation and its identity journal +commit, or persistent SQLite failure before that commit, can leave an unowned +**empty** destination: no bytes are written before the commit. Recovery preserves that file rather than guessing ownership; Retry uses a numbered free destination. SQLite and filesystem creation cannot commit atomically, and portable rename cannot guarantee no-clobber publication on the