fix(backup): cleanup orphan .tmp on db.backup() failure + concurrency note
Code review I1: wrap snapshot's backup+rename in try/catch, unlink orphan tmp file on failure so the next run does not encounter a confusing 'existing file is not a database' error from sqlite. Code review I2: JSDoc note that snapshot() is not safe for concurrent calls — callers should serialize via runDaily()'s marker. New unit test injects a fake db whose backup() rejects after a partial write, asserts no .tmp / .sqlite remains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -27,13 +27,25 @@ export class BackupService {
|
||||
private now: () => Date = () => new Date()
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Atomic snapshot of the SQLite DB to `inkling-YYYY-MM-DD.sqlite` (KST).
|
||||
*
|
||||
* NOT safe for concurrent calls — two parallel calls race on the same
|
||||
* tmp/final path. Callers should serialize via {@link runDaily}, which
|
||||
* gates on the `.last-snapshot` marker.
|
||||
*/
|
||||
async snapshot(): Promise<SnapshotResult> {
|
||||
await mkdir(this.backupDir, { recursive: true });
|
||||
const dateKey = toKstDateKey(this.now());
|
||||
const finalPath = join(this.backupDir, `inkling-${dateKey}.sqlite`);
|
||||
const tmpPath = `${finalPath}.tmp`;
|
||||
await this.db.backup(tmpPath);
|
||||
await rename(tmpPath, finalPath);
|
||||
try {
|
||||
await this.db.backup(tmpPath);
|
||||
await rename(tmpPath, finalPath);
|
||||
} catch (e) {
|
||||
await unlink(tmpPath).catch(() => { /* tmp may not exist */ });
|
||||
throw e;
|
||||
}
|
||||
const st = await stat(finalPath);
|
||||
return { path: finalPath, bytes: st.size };
|
||||
}
|
||||
|
||||
@@ -75,4 +75,24 @@ describe('BackupService.snapshot', () => {
|
||||
const r = await svc.snapshot();
|
||||
expect(statSync(r.path).size).toBeGreaterThan(100);
|
||||
});
|
||||
|
||||
it('cleans up orphan .tmp file when db.backup() fails', async () => {
|
||||
// Inject a fake db whose backup() rejects to simulate disk-full / IO-error.
|
||||
const fakeDb = {
|
||||
backup: async (tmpPath: string) => {
|
||||
// Simulate a partial write before the failure (real failures may leave bytes).
|
||||
await import('node:fs/promises').then((fs) =>
|
||||
fs.writeFile(tmpPath, 'partial-bytes')
|
||||
);
|
||||
throw new Error('simulated disk full');
|
||||
}
|
||||
} as unknown as Database.Database;
|
||||
const svc = new BackupService(fakeDb, dir, () => new Date('2026-04-26T12:00:00Z'));
|
||||
await expect(svc.snapshot()).rejects.toThrow('simulated disk full');
|
||||
// No leftover .tmp file
|
||||
const remaining = readdirSync(dir);
|
||||
expect(remaining.filter((f) => f.endsWith('.tmp'))).toEqual([]);
|
||||
// No final file either (write never completed)
|
||||
expect(remaining.filter((f) => f.endsWith('.sqlite'))).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user