diff --git a/AGENTS.md b/AGENTS.md index 9a8702b83..e2b3dbab2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1057,6 +1057,8 @@ reused inodes after unlink; old proofs without creation time remain untrusted. Fresh reservations atomically commit their row path/name and captured ownership before the initial HTTP wait; no preexisting partial is truncated without matching expected ownership. +Captured foreign files retain their recovery copy and journal even after public +restoration, until the user explicitly removes the recovery copy. Private cleanup captures are journaled before relocation, keeping failed Remove/Clear/cancel cleanup retryable across restarts without hardlinks. Active failures, promotion and startup share that cleanup; Remove waits for active diff --git a/CLAUDE.md b/CLAUDE.md index 29fbc479f..5e555c70e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1868,6 +1868,8 @@ reused inodes after unlink; old proofs without creation time remain untrusted. Fresh reservations atomically commit their row path/name and captured ownership before the initial HTTP wait; no preexisting partial is truncated without matching expected ownership. +Captured foreign files retain their recovery copy and journal even after public +restoration, until the user explicitly removes the recovery copy. Private cleanup captures are journaled before relocation, keeping failed Remove/Clear/cancel cleanup retryable across restarts without hardlinks. Active failures, promotion and startup share that cleanup; Remove waits for active 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 fff60d720..c59309b69 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 @@ -39,20 +39,7 @@ function cleanupCapture( } 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( - readArchiveStatsSync(publicPath), - file - ) - ) - throw error; - } + restoreArchiveReplacement(path, publicPath); } unlinkSync(path); } @@ -62,3 +49,25 @@ function cleanupCapture( /* never recursively delete captures */ } } + +/** Never unlink a foreign capture after publishing a raceable public link. */ +export function restoreArchiveReplacement( + captured: string, + publicPath: string +): never { + const file = readArchiveStatsSync(captured); + try { + linkSync(captured, publicPath); + } catch (error) { + if ( + (error as NodeJS.ErrnoException).code !== 'EEXIST' || + !sameArchiveFileIdentity(readArchiveStatsSync(publicPath), file) + ) + throw error; + } + // A second process can remove the public link at any time. Only an explicit + // user cleanup of this recovery copy may release its durable journal entry. + throw new Error( + `Archive cleanup preserved an unrelated recovery file: ${captured}` + ); +} 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 3b442bbdd..06d34dc03 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 @@ -61,7 +61,9 @@ it('restores a replacement that arrives before atomic capture', async () => { await writeFile(from, 'replacement'); await actual.rename(from, to); }); - await cleanupCatchupFile(path, identity); + await expect(cleanupCatchupFile(path, identity)).rejects.toThrow( + 'preserved an unrelated recovery file' + ); expect(await readFile(path, 'utf8')).toBe('replacement'); expect(await readFile(join(directory, 'original'), 'utf8')).toBe('archive'); }); 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 0ee980642..a292bd8e3 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,7 +25,9 @@ export async function cleanupCatchupFile( // public entry. If restoration is unavailable, retain it privately. try { await link(captured, path); - await unlink(captured); + throw new Error( + `Archive cleanup preserved an unrelated recovery file: ${captured}` + ); } catch (error) { console.warn( '[Downloads] Replaced file retained for recovery:', 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 36c4a092d..cc24d0dbc 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 @@ -73,10 +73,15 @@ it('restores a replacement captured at the cleanup boundary', () => { writeFileSync(from, 'unrelated bytes'); actual.renameSync(from, to); }); - removeJournaledCatchupPartial(filePath, proof, recordCapture); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('preserved an unrelated recovery file'); expect(readFileSync(filePath + '.part', 'utf8')).toBe('unrelated bytes'); + expect(readFileSync(proof.partialCleanupPath!, 'utf8')).toBe( + 'unrelated bytes' + ); }); -it('retains a replacement capture until no-clobber restoration succeeds', () => { +it('retains a replacement capture until explicit recovery-copy cleanup', () => { jest.mocked(renameSync).mockImplementationOnce((from, to) => { actual.renameSync(from, join(directory, 'original')); writeFileSync(from, 'foreign bytes'); @@ -113,16 +118,20 @@ it('retains a replacement capture until no-clobber restoration succeeds', () => 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'); + ).toThrow('preserved an unrelated recovery file'); expect(readFileSync(filePath + '.part', 'utf8')).toBe('foreign bytes'); expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + // The restored public entry may disappear immediately; its recovery copy + // must survive and remain journaled across another cleanup attempt. + actual.unlinkSync(filePath + '.part'); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('preserved an unrelated recovery file'); + expect(readFileSync(capture, 'utf8')).toBe('foreign bytes'); + actual.unlinkSync(capture); // explicit recovery-copy cleanup by the user removeJournaledCatchupPartial(filePath, proof, recordCapture); - expect(readFileSync(filePath + '.part', 'utf8')).toBe('foreign bytes'); expect(() => lstatSync(capture)).toThrow(); }); @@ -360,3 +369,31 @@ it.each([false, true])( expect(renameSync).not.toHaveBeenCalled(); } ); + +it.each([false, true])( + 'preserves the last foreign link if the restored public entry is immediately removed (retry=%s)', + (retry) => { + if (retry) { + const capture = join(directory, 'foreign-capture'); + writeFileSync(capture, 'foreign bytes'); + proof.partialCleanupPath = capture; + actual.unlinkSync(filePath + '.part'); + } else { + jest.mocked(renameSync).mockImplementationOnce((from, to) => { + actual.renameSync(from, join(directory, 'original')); + writeFileSync(from, 'foreign bytes'); + actual.renameSync(from, to); + }); + } + jest.mocked(linkSync).mockImplementationOnce((from, to) => { + actual.linkSync(from, to); + actual.unlinkSync(to); + }); + expect(() => + removeJournaledCatchupPartial(filePath, proof, recordCapture) + ).toThrow('preserved an unrelated recovery file'); + expect(readFileSync(proof.partialCleanupPath!, 'utf8')).toBe( + 'foreign bytes' + ); + } +); 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 de6c2d788..1443957a5 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 @@ -2,14 +2,11 @@ import { readArchiveStatsSync, type ArchiveFileStats, } from './download-catchup-stats'; -import { cleanupArchiveCapture } from './download-catchup-capture'; import { - mkdtempSync, - renameSync, - linkSync, - unlinkSync, - rmdirSync, -} from 'node:fs'; + cleanupArchiveCapture, + restoreArchiveReplacement, +} from './download-catchup-capture'; +import { mkdtempSync, renameSync, unlinkSync, rmdirSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { readArchiveFinalizations, @@ -63,16 +60,7 @@ function removeOwnedEntry( if (matches(readArchiveStatsSync(captured), identity)) { unlinkSync(captured); } else { - try { - linkSync(captured, path); - unlinkSync(captured); - } catch (error) { - console.warn( - '[Downloads] Replaced file retained for recovery:', - captured - ); - throw error; - } + restoreArchiveReplacement(captured, path); } } finally { try { diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index c1b75e17e..c321fef78 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -117,7 +117,11 @@ final before reserving a destination. A failed 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 +the capture and its journal for recovery. Even after a foreign entry is restored, +its private recovery copy is never automatically unlinked: another process could +remove the public link first. The error names the recovery file; after the user +recovers it and explicitly removes that private copy, cleanup may release the +journal. Ordinary owned-file cleanup remains automatic. 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.