From b05db7ca2137ea671295badd5e30e31372118fd4 Mon Sep 17 00:00:00 2001 From: 4gray Date: Tue, 8 Sep 2026 05:47:07 +0200 Subject: [PATCH] fix(downloads): retain captures until replacement restoration succeeds --- .../database/download-catchup-capture.ts | 44 +++++++++++----- .../database/download-catchup-removal.spec.ts | 50 +++++++++++++++++++ .../database/download-catchup-removal.ts | 3 +- docs/architecture/download-manager.md | 5 +- 4 files changed, 87 insertions(+), 15 deletions(-) diff --git a/apps/electron-backend/src/app/events/database/download-catchup-capture.ts b/apps/electron-backend/src/app/events/database/download-catchup-capture.ts index 39cb8748d..a717422dc 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-capture.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-capture.ts @@ -1,4 +1,4 @@ -import { lstatSync, rmdirSync, unlinkSync } from 'node:fs'; +import { linkSync, lstatSync, rmdirSync, unlinkSync } from 'node:fs'; import { dirname } from 'node:path'; import { sameArchiveFileIdentity, @@ -11,29 +11,47 @@ export function cleanupArchiveCapture( proof: ArchiveDownloadProof | undefined ): void { if (!proof) return; - cleanupCapture(proof.partialCleanupPath, proof.partialIdentity); + cleanupCapture( + proof.partialCleanupPath, + proof.partialIdentity, + proof.filePath + '.part' + ); if (proof.phase !== 'transfer') - cleanupCapture(proof.finalCleanupPath, proof.finalIdentity); + cleanupCapture( + proof.finalCleanupPath, + proof.finalIdentity, + proof.filePath + ); } function cleanupCapture( path: string | undefined, - identity: ArchiveFileIdentity + identity: ArchiveFileIdentity, + publicPath: string ): void { if (!path) return; + let file; try { - const file = lstatSync(path); - if (file.isFile() && sameArchiveFileIdentity(file, identity)) { - unlinkSync(path); - } else { - console.warn( - '[Downloads] Replaced file retained for recovery:', - path - ); - } + file = lstatSync(path); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; } + if (file) { + if (!(file.isFile() && sameArchiveFileIdentity(file, identity))) { + // Restore without clobbering. If a prior restore linked successfully + // but unlink failed, the matching public entry permits cleanup retry. + try { + linkSync(path, publicPath); + } catch (error) { + if ( + (error as NodeJS.ErrnoException).code !== 'EEXIST' || + !sameArchiveFileIdentity(lstatSync(publicPath), file) + ) + throw error; + } + } + unlinkSync(path); + } try { rmdirSync(dirname(path)); } catch { 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 16a8b41da..5ba32d383 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 @@ -76,6 +76,56 @@ it('restores a replacement captured at the cleanup boundary', () => { removeJournaledCatchupPartial(filePath, proof, recordCapture); expect(readFileSync(filePath + '.part', 'utf8')).toBe('unrelated bytes'); }); +it('retains a replacement capture until no-clobber restoration succeeds', () => { + jest.mocked(renameSync).mockImplementationOnce((from, to) => { + actual.renameSync(from, join(directory, 'original')); + writeFileSync(from, 'foreign bytes'); + actual.renameSync(from, to); + }); + jest.mocked(linkSync).mockImplementation(() => { + throw Object.assign(new Error('hardlinks unavailable'), { + code: 'ENOTSUP', + }); + }); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('hardlinks unavailable'); + const capture = proof.partialCleanupPath!; + expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('hardlinks unavailable'); + expect(proof.partialCleanupPath).toBe(capture); + jest.mocked(linkSync).mockImplementationOnce(() => { + throw Object.assign(new Error('destination parent missing'), { + code: 'ENOENT', + }); + }); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('destination parent missing'); + expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + jest.mocked(linkSync).mockImplementation(actual.linkSync); + writeFileSync(filePath + '.part', 'newer public bytes'); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow(); + expect(readFileSync(filePath + '.part', 'utf8')).toBe('newer public bytes'); + expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + actual.unlinkSync(filePath + '.part'); + jest.mocked(unlinkSync).mockImplementationOnce(() => { + throw new Error('restored capture still locked'); + }); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('restored capture still locked'); + expect(readFileSync(filePath + '.part', 'utf8')).toBe('foreign bytes'); + expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + removeJournaledCatchupPartial(filePath, proof, recordCapture); + expect(readFileSync(filePath + '.part', 'utf8')).toBe('foreign bytes'); + expect(() => lstatSync(capture)).toThrow(); +}); + it('retries a durable capture after an I/O error without needing hardlinks', () => { jest.mocked(unlinkSync).mockImplementationOnce(() => { throw Object.assign(new Error('locked'), { code: 'EACCES' }); 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 251042906..4e3c509ff 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 @@ -65,11 +65,12 @@ function removeOwnedEntry( try { linkSync(captured, path); unlinkSync(captured); - } catch { + } catch (error) { console.warn( '[Downloads] Replaced file retained for recovery:', captured ); + throw error; } } } finally { diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index cfa587936..c77f49843 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -114,7 +114,10 @@ startup recovery use the same journal-backed cleanup as manual actions. Cancel, failure and removal of an unfinished attempt also clean its journaled final target; removing a completed row preserves its media. Retry cleans an incomplete owned final before reserving a destination. A failed -unlink keeps that durable pointer; Remove/Clear, Retry/Resume and fresh +unlink or replacement restoration keeps that durable pointer and blocks journal +deletion/replacement. Later cleanup retries no-clobber restoration of captured +foreign entries; an occupied public path or unsupported hardlinks preserves both +the capture and its journal for recovery. Remove/Clear, Retry/Resume and fresh reservations retry identity-verified cleanup, including after restart and on filesystems without hardlinks. Cleanup remains synchronous after the runtime guard, so a completion transition cannot interleave with unlink.