From e257b77fca8b7792711780a98dc0a7e0961103b9 Mon Sep 17 00:00:00 2001 From: 4gray Date: Tue, 8 Sep 2026 05:35:36 +0200 Subject: [PATCH] fix(downloads): commit archive reservation and ownership atomically --- AGENTS.md | 3 +- CLAUDE.md | 3 +- .../database/download-catchup-journal.spec.ts | 88 +++++++++++++++++++ .../database/download-catchup-journal.ts | 39 ++++++-- .../download-catchup-reservation.spec.ts | 26 +++--- .../database/download-catchup-reservation.ts | 10 ++- .../database/download-catchup-runtime.spec.ts | 2 +- docs/architecture/download-manager.md | 4 +- 8 files changed, 154 insertions(+), 21 deletions(-) create mode 100644 apps/electron-backend/src/app/events/database/download-catchup-journal.spec.ts diff --git a/AGENTS.md b/AGENTS.md index e07beac69..fc4316002 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1045,7 +1045,8 @@ so Retry can reserve a fresh path. A synchronous completion-commit boundary rejects late pause/cancel commands before awaited cleanup and persistence. Archive ownership includes device, inode and positive creation time to reject reused inodes after unlink; old proofs without creation time remain untrusted. -Fresh reservations capture and journal ownership before the initial HTTP wait; +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. Private cleanup captures are journaled before relocation, keeping failed Remove/Clear/cancel cleanup retryable across restarts without hardlinks. Active diff --git a/CLAUDE.md b/CLAUDE.md index 22095d486..103e57ac1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1856,7 +1856,8 @@ so Retry can reserve a fresh path. A synchronous completion-commit boundary rejects late pause/cancel commands before awaited cleanup and persistence. Archive ownership includes device, inode and positive creation time to reject reused inodes after unlink; old proofs without creation time remain untrusted. -Fresh reservations capture and journal ownership before the initial HTTP wait; +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. Private cleanup captures are journaled before relocation, keeping failed Remove/Clear/cancel cleanup retryable across restarts without hardlinks. Active diff --git a/apps/electron-backend/src/app/events/database/download-catchup-journal.spec.ts b/apps/electron-backend/src/app/events/database/download-catchup-journal.spec.ts new file mode 100644 index 000000000..86b1e38e2 --- /dev/null +++ b/apps/electron-backend/src/app/events/database/download-catchup-journal.spec.ts @@ -0,0 +1,88 @@ +import { execFileSync } from 'node:child_process'; +import { createRequire } from 'node:module'; +import { resolve } from 'node:path'; +import { pathToFileURL } from 'node:url'; + +function runReservationScenario(scenario: string): { + row: { file_path: string | null; file_name: string }; + proofs: { download_id: number; proof: string }[]; + error?: string; +} { + const journalUrl = pathToFileURL( + resolve(__dirname, 'download-catchup-journal.ts') + ).href; + const script = ` + const { default: Database } = await import('better-sqlite3'); + const { drizzle } = await import('drizzle-orm/better-sqlite3'); + const { recordArchiveReservation } = await import(${JSON.stringify(journalUrl)}); + const sqlite = new Database(':memory:'); + sqlite.exec(\` + CREATE TABLE downloads (id INTEGER PRIMARY KEY, file_path TEXT, file_name TEXT, updated_at TEXT); + CREATE TABLE download_archive_finalizations (download_id INTEGER PRIMARY KEY REFERENCES downloads(id), proof TEXT NOT NULL); + INSERT INTO downloads (id, file_name) VALUES (1, 'show.ts'); + \`); + const db = drizzle(sqlite); + const identity = { dev: 1, ino: 2, birthtimeMs: 1000 }; + let error; + ${scenario} + console.log(JSON.stringify({ error, + row: sqlite.prepare('SELECT file_path, file_name FROM downloads WHERE id=1').get(), + proofs: sqlite.prepare('SELECT * FROM download_archive_finalizations').all() + })); + sqlite.close(); + `; + return JSON.parse( + execFileSync( + createRequire(__filename)('electron'), + ['--import', 'tsx', '--eval', script], + { + cwd: process.cwd(), + encoding: 'utf8', + env: { + ...process.env, + ELECTRON_RUN_AS_NODE: '1', + TSX_TSCONFIG_PATH: resolve( + process.cwd(), + 'tsconfig.base.json' + ), + }, + } + ) + ); +} + +it('commits the reservation path and ownership together before transfer setup', () => { + const result = runReservationScenario(` + await recordArchiveReservation(db, 1, '/archive/show (1).ts', identity, 'show (1).ts'); + `); + expect(result.row).toEqual({ + file_path: '/archive/show (1).ts', + file_name: 'show (1).ts', + }); + expect(JSON.parse(result.proofs[0].proof)).toMatchObject({ + phase: 'transfer', + filePath: result.row.file_path, + partialIdentity: { dev: 1, ino: 2, birthtimeMs: 1000 }, + }); +}); + +it('rolls back the row path when the ownership write fails', () => { + const result = runReservationScenario(` + sqlite.exec("CREATE TRIGGER reject_proof BEFORE INSERT ON download_archive_finalizations BEGIN SELECT RAISE(ABORT, 'journal unavailable'); END"); + try { await recordArchiveReservation(db, 1, '/archive/new.ts', identity, 'new.ts'); } + catch (failure) { error = failure.message; } + `); + expect(result.error).toContain('journal unavailable'); + expect(result.row).toEqual({ file_path: null, file_name: 'show.ts' }); + expect(result.proofs).toEqual([]); +}); + +it('rejects a reservation whose download row disappeared', () => { + const result = runReservationScenario(` + try { await recordArchiveReservation(db, 2, '/archive/new.ts', identity, 'new.ts'); } + catch (failure) { error = failure.message; } + `); + expect(result.error).toContain('row is unavailable'); + expect(result.proofs).toEqual([]); + expect(result.row.file_path).toBeNull(); +}); diff --git a/apps/electron-backend/src/app/events/database/download-catchup-journal.ts b/apps/electron-backend/src/app/events/database/download-catchup-journal.ts index 5272bd875..31699bb00 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-journal.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-journal.ts @@ -1,5 +1,5 @@ import { cleanupArchiveCapture } from './download-catchup-capture'; -import { eq, inArray } from 'drizzle-orm'; +import { eq, inArray, sql } from 'drizzle-orm'; import { lstatSync } from 'node:fs'; import { isAbsolute } from 'node:path'; import * as schema from '../../database/schema'; @@ -46,6 +46,35 @@ export async function recordArchivePartial( }); } +/** The row path and its ownership proof must become recoverable together. */ +export async function recordArchiveReservation( + db: DownloadsDatabase, + downloadId: number, + filePath: string, + partialIdentity: ArchiveFileIdentity, + fileName: string +): Promise { + db.transaction((tx) => { + const result = tx + .update(schema.downloads) + .set({ + filePath, + fileName, + updatedAt: sql`CURRENT_TIMESTAMP`, + }) + .where(eq(schema.downloads.id, downloadId)) + .run(); + if (result.changes !== 1) + throw new Error('Archive reservation row is unavailable'); + writeArchiveProof(tx, downloadId, { + version: 1, + phase: 'transfer', + filePath, + partialIdentity, + }).run(); + }); +} + export async function recordArchiveFinalization( db: DownloadsDatabase, downloadId: number, @@ -54,11 +83,11 @@ export async function recordArchiveFinalization( await writeArchiveProof(db, downloadId, proof); } -async function writeArchiveProof( - db: DownloadsDatabase, +function writeArchiveProof( + db: Pick, downloadId: number, proof: ArchiveDownloadProof -): Promise { +) { const serialized = JSON.stringify({ ...proof, partialIdentity: archiveFileIdentity(proof.partialIdentity), @@ -68,7 +97,7 @@ async function writeArchiveProof( } : {}), }); - await db + return db .insert(schema.downloadArchiveFinalizations) .values({ downloadId, proof: serialized }) .onConflictDoUpdate({ 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 baf3f2731..4dd4f708f 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 @@ -13,7 +13,7 @@ import { reserveTarget } from './download-runtime-reservation'; import { reserveFreshCatchupTarget } from './download-catchup-reservation'; import { clearArchiveFinalization, - recordArchivePartial, + recordArchiveReservation, readArchiveFinalizations, } from './download-catchup-journal'; import type { DownloadsDatabase, DownloadTask } from './download-task'; @@ -23,7 +23,7 @@ jest.mock('./download-catchup-journal', () => ({ clearArchiveFinalization: jest.fn().mockResolvedValue(undefined), readArchiveFinalizations: jest.fn(), recordArchiveCleanupPath: jest.fn(), - recordArchivePartial: jest.fn().mockResolvedValue(undefined), + recordArchiveReservation: jest.fn().mockResolvedValue(undefined), })); it.each([false, true])( @@ -156,11 +156,12 @@ it('binds a fresh reservation before a replacement can arrive during the HTTP wa }; try { const reservation = await reserveTarget({} as DownloadsDatabase, task); - expect(recordArchivePartial).toHaveBeenCalledWith( + expect(recordArchiveReservation).toHaveBeenCalledWith( expect.anything(), task.id, reservation.path, - task.catchupExpectedPartialIdentity + task.catchupExpectedPartialIdentity, + reservation.filename ); await rename(reservation.partialPath, join(directory, 'original')); await writeFile( @@ -200,13 +201,18 @@ it.each([false, true])( }, }; const failure = new Error('SQLite write failed'); - jest.mocked(recordArchivePartial).mockImplementationOnce(async () => { - if (replaced) { - await rename(filePath + '.part', join(directory, 'original')); - await writeFile(filePath + '.part', 'foreign bytes'); + jest.mocked(recordArchiveReservation).mockImplementationOnce( + async () => { + if (replaced) { + await rename( + filePath + '.part', + join(directory, 'original') + ); + await writeFile(filePath + '.part', 'foreign bytes'); + } + throw failure; } - throw failure; - }); + ); try { await expect( reserveTarget({} as DownloadsDatabase, task) 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 2ed6d59f9..6e6988bad 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 @@ -4,7 +4,7 @@ import { cleanupCatchupPartial } from './download-catchup-cleanup'; import { cleanupStoredCatchupPartial } from './download-catchup-removal'; import { clearArchiveFinalization, - recordArchivePartial, + recordArchiveReservation, } from './download-catchup-journal'; import { ArchivePartialReplacedError, @@ -69,7 +69,13 @@ export async function reserveOwnedCatchupTarget( if (!identity) throw new Error('Archive reservation identity is unavailable'); try { - await recordArchivePartial(db, task.id, reservation.path, identity); + await recordArchiveReservation( + db, + task.id, + reservation.path, + identity, + 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. diff --git a/apps/electron-backend/src/app/events/database/download-catchup-runtime.spec.ts b/apps/electron-backend/src/app/events/database/download-catchup-runtime.spec.ts index dfac94785..5030f2699 100644 --- a/apps/electron-backend/src/app/events/database/download-catchup-runtime.spec.ts +++ b/apps/electron-backend/src/app/events/database/download-catchup-runtime.spec.ts @@ -49,7 +49,7 @@ function mockRememberCapture( } jest.mock('./download-catchup-journal', () => ({ ...jest.requireActual('./download-catchup-journal'), - recordArchivePartial: jest.fn( + recordArchiveReservation: jest.fn( async (_db, id, filePath, partialIdentity) => { mockProofs.set(id, { version: 1, diff --git a/docs/architecture/download-manager.md b/docs/architecture/download-manager.md index 34bb702de..cfa587936 100644 --- a/docs/architecture/download-manager.md +++ b/docs/architecture/download-manager.md @@ -104,7 +104,9 @@ unknown or replaced entries are preserved. Ownership includes a positive file creation timestamp as well as device/inode, so inode reuse after unlink cannot bless a new entry; proofs lacking creation time remain untrusted. Fresh reservations capture this identity from their exclusive creation descriptor and -journal it before the HTTP wait. Existing partials without matching expected +commit it together with the downloads row path/name in one SQLite transaction +before the HTTP wait. A committed reservation is therefore recoverable even if +the process exits before transfer setup. Existing partials without matching expected ownership are never truncated, including before the first response. Before capture, a synchronous SQLite write records `partialCleanupPath` (or `finalCleanupPath` for failed promotion) in the existing ownership proof. Active cancellation/failure, promotion and