From 169c953725bf068a36eb64e7714d58f6d192619e Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Thu, 20 Aug 2026 23:29:20 -0500 Subject: [PATCH 1/2] fix(desktop): reclaim pre-update state.db backups even when the backup fails `preflightStateDb` copies `state.db` to `state.db.pre-update-emergency-.bak` before an update, then prunes older copies. The prune was nested inside the `try` whose first statement was the `copyFileSync`, so reclaiming old backups only happened when writing a new one succeeded. Every path that prevents the copy therefore also skipped the cleanup: * ENOSPC, because the disk is full - and it is full partly because these backups were never reclaimed; * EBUSY/EPERM from another process holding `state.db`, which on Windows is the ordinary state during a failed self-update and is exactly the scenario #91229 is filed about; * `state.db` missing or too small to be a database, both of which `return` before the copy is attempted; * `statSync` throwing, which lands in the outer catch. The janitor only ran on the days nothing needed cleaning. #91229 reports 2.5-3 GB of accumulated .bak files against a database of ~650-750 MB, which is what an unbounded count of full-size copies looks like. Hoist the sweep into `pruneEmergencyStateDbBackups` and call it unconditionally at the top of `preflightStateDb`, before any early return and before the copy. Selection moves to `emergency-backup-retention.ts` so it can be tested without booting Electron, matching how the other extracted electron helpers are structured. Two behaviours are preserved on purpose. Retention stays at three files, which is what the old code actually did (its comment said two, but it excluded the just-written backup before slicing) - lowering it would delete recovery data users have today, so the number is now an explicit named constant and the choice is left to maintainers. And the sweep still never throws: this is disk hygiene on the update path, and failing to reclaim must not abort an update. The reported `win-unpacked.bak` is deliberately NOT touched. That is a single rollback copy preserved by before-pack (#53040), replaced on each pack rather than accumulated, and deleting it would remove a recovery path. --- .../emergency-backup-retention.test.ts | 154 ++++++++++++++++++ .../electron/emergency-backup-retention.ts | 78 +++++++++ apps/desktop/electron/main.ts | 73 ++++++--- 3 files changed, 281 insertions(+), 24 deletions(-) create mode 100644 apps/desktop/electron/emergency-backup-retention.test.ts create mode 100644 apps/desktop/electron/emergency-backup-retention.ts diff --git a/apps/desktop/electron/emergency-backup-retention.test.ts b/apps/desktop/electron/emergency-backup-retention.test.ts new file mode 100644 index 000000000000..6c7660561b2f --- /dev/null +++ b/apps/desktop/electron/emergency-backup-retention.test.ts @@ -0,0 +1,154 @@ +// Retention policy for pre-update emergency state.db backups (#91229). +// +// The reported symptom was 2.5-3 GB of `state.db.pre-update-emergency-*.bak` +// files that "are never cleaned up". A prune did exist, but it was nested +// inside the `try` whose first statement wrote the new backup, so it only ran +// when that write succeeded. These tests pin the selection rule; the +// unconditional-call half is asserted structurally at the bottom. + +import { readFileSync } from 'node:fs' + +import { describe, expect, it } from 'vitest' + +import { + EMERGENCY_BACKUP_PREFIX, + EMERGENCY_BACKUP_RETENTION, + EMERGENCY_BACKUP_SUFFIX, + isEmergencyBackup, + selectEmergencyBackupsToDelete, +} from './emergency-backup-retention' + +/** Build the exact filename `preflightStateDb` writes for a given instant. */ +function backupName(iso: string): string { + return `${EMERGENCY_BACKUP_PREFIX}${iso.replace(/[:.]/g, '-')}${EMERGENCY_BACKUP_SUFFIX}` +} + +const OLDEST = backupName('2026-08-15T03:11:52.004Z') +const OLDER = backupName('2026-08-17T09:02:00.900Z') +const NEWER = backupName('2026-08-20T22:45:10.120Z') +const NEWEST = backupName('2026-08-21T01:30:00.000Z') + +describe('isEmergencyBackup', () => { + it('accepts exactly the filenames preflightStateDb writes', () => { + expect(isEmergencyBackup(NEWEST)).toBe(true) + }) + + it('leaves the live database and its sidecars alone', () => { + // The single most important negative: this sweep runs in HERMES_HOME, so + // a loose predicate deletes the database the backups exist to protect. + expect(isEmergencyBackup('state.db')).toBe(false) + expect(isEmergencyBackup('state.db-wal')).toBe(false) + expect(isEmergencyBackup('state.db-shm')).toBe(false) + }) + + it('leaves other .bak files alone', () => { + // The Python-level snapshot is a different mechanism with a different + // lifetime; this policy does not own it. + expect(isEmergencyBackup('state.db.bak')).toBe(false) + expect(isEmergencyBackup('config.yaml.bak')).toBe(false) + expect(isEmergencyBackup(`${EMERGENCY_BACKUP_PREFIX}2026-08-21.tmp`)).toBe(false) + }) + + it('does not match a prefixed name embedded mid-filename', () => { + expect(isEmergencyBackup(`copy-of-${NEWEST}`)).toBe(false) + }) +}) + +describe('selectEmergencyBackupsToDelete', () => { + it('keeps the newest RETENTION backups and returns the rest', () => { + const doomed = selectEmergencyBackupsToDelete([OLDER, NEWEST, OLDEST, NEWER]) + + expect(EMERGENCY_BACKUP_RETENTION).toBe(3) + expect(doomed).toEqual([OLDEST]) + }) + + it('returns nothing while at or under the budget', () => { + expect(selectEmergencyBackupsToDelete([])).toEqual([]) + expect(selectEmergencyBackupsToDelete([NEWEST])).toEqual([]) + expect(selectEmergencyBackupsToDelete([NEWEST, NEWER, OLDER])).toEqual([]) + }) + + it('counts the just-written backup toward the budget', () => { + // The old code excluded the new file before slicing, so three survived + // while the comment said two. Whatever the number is, the newest file has + // to be inside it or the budget is off by one forever. + const listing = [OLDEST, OLDER, NEWER, NEWEST] + const kept = listing.filter(f => !selectEmergencyBackupsToDelete(listing).includes(f)) + + expect(kept).toHaveLength(EMERGENCY_BACKUP_RETENTION) + expect(kept).toContain(NEWEST) + }) + + it('orders by name, not by array order or mtime', () => { + // Sorting is lexicographic because the ISO timestamp is fixed-width; a + // shuffled listing (readdir gives no ordering guarantee) must not change + // which files survive. + const shuffled = [NEWER, OLDEST, NEWEST, OLDER] + + expect(selectEmergencyBackupsToDelete(shuffled)).toEqual([OLDEST]) + }) + + it('ignores unrelated files when choosing what to delete', () => { + const doomed = selectEmergencyBackupsToDelete([ + 'state.db', + 'state.db-wal', + 'config.yaml', + OLDEST, + OLDER, + NEWER, + NEWEST, + ]) + + expect(doomed).toEqual([OLDEST]) + }) + + it('deletes everything when told to retain none', () => { + expect(selectEmergencyBackupsToDelete([NEWEST, OLDEST], 0)).toEqual([NEWEST, OLDEST]) + }) + + it('treats a nonsensical retention as retain-none rather than deleting at random', () => { + expect(selectEmergencyBackupsToDelete([NEWEST, OLDEST], -1)).toEqual([NEWEST, OLDEST]) + expect(selectEmergencyBackupsToDelete([NEWEST, OLDEST], Number.NaN)).toEqual([NEWEST, OLDEST]) + }) + + it('scales past the budget without dropping any candidate', () => { + const many = Array.from({ length: 40 }, (_, i) => + backupName(`2026-08-21T0${Math.floor(i / 10)}:${String(i % 10).padStart(2, '0')}:00.000Z`) + ) + + const doomed = selectEmergencyBackupsToDelete(many) + + expect(doomed).toHaveLength(many.length - EMERGENCY_BACKUP_RETENTION) + // Nothing kept is also deleted, and nothing is listed twice. + expect(new Set(doomed).size).toBe(doomed.length) + }) +}) + +describe('preflightStateDb wiring', () => { + // The selection rule above is only half the fix. The bug was WHERE the + // sweep was called from, and that is a property of main.ts, which cannot be + // imported here (it boots Electron). So assert it against the source, the + // same way the repo pins other cross-module invariants. + const main = readFileSync(new URL('./main.ts', import.meta.url), 'utf8') + const body = main.slice(main.indexOf('function preflightStateDb(')) + + it('sweeps before the first early return, not after the copy succeeds', () => { + const call = body.indexOf('pruneEmergencyStateDbBackups(hermesHome, rememberLog)') + // Anchor on the guard itself, not on the word "return": the surrounding + // comments say "early return" and would satisfy a naive search. + const firstGuard = body.indexOf('if (!fileExists(stateDbPath))') + const copy = body.indexOf('fs.copyFileSync(stateDbPath, emergencyPath)') + + expect(call).toBeGreaterThan(-1) + expect(firstGuard).toBeGreaterThan(-1) + // Before the first guard => a missing or too-small state.db still reclaims. + expect(call).toBeLessThan(firstGuard) + // Before the copy => ENOSPC and EBUSY still reclaim. This is the assertion + // that fails if anyone re-nests the sweep inside the copy's try block. + expect(call).toBeLessThan(copy) + }) + + it('no longer carries the inline prune that was coupled to the copy', () => { + expect(body).not.toContain("f.startsWith('state.db.pre-update-emergency-')") + }) +}) diff --git a/apps/desktop/electron/emergency-backup-retention.ts b/apps/desktop/electron/emergency-backup-retention.ts new file mode 100644 index 000000000000..92c2faf6b8b3 --- /dev/null +++ b/apps/desktop/electron/emergency-backup-retention.ts @@ -0,0 +1,78 @@ +// Retention policy for the pre-update emergency copies of `state.db`. +// +// Before an update, `preflightStateDb` copies `state.db` to +// `state.db.pre-update-emergency-.bak` so a torn update can be +// recovered from. Those copies are the size of the live database, which on a +// busy install is comfortably several hundred MB, so they have to be reclaimed +// or they eat the disk (issue #91229 reported 2.5-3 GB of accumulated .bak +// files and ~6.5 GB of week-over-week C: growth). +// +// The sweep used to live inline in `preflightStateDb`, nested inside the `try` +// whose first statement was the `copyFileSync`. That coupled reclaiming old +// backups to successfully writing a new one, so every failure mode that +// *prevents* the copy also skipped the cleanup: +// +// * the copy throws ENOSPC because the disk is full -- and it is full partly +// because these backups were never reclaimed; +// * the copy throws EBUSY/EPERM because another process holds `state.db`, +// which on Windows is the ordinary state during a failed self-update and +// the exact scenario #91229 is filed about; +// * `state.db` is missing or too small to be a real database, both of which +// `return` before the copy is ever attempted. +// +// In other words the janitor only ran on the days nothing needed cleaning. +// Splitting the selection out here lets `preflightStateDb` run it +// unconditionally, and lets it be tested without Electron or a real disk. + +/** Filename prefix written by `preflightStateDb`. */ +export const EMERGENCY_BACKUP_PREFIX = 'state.db.pre-update-emergency-' + +/** Filename suffix written by `preflightStateDb`. */ +export const EMERGENCY_BACKUP_SUFFIX = '.bak' + +/** + * How many emergency backups to keep, newest first, counting the one just + * written. + * + * This preserves the effective behaviour of the previous implementation + * rather than the behaviour its comment claimed. That comment said "Prune to + * the 2 most recent", but the filter excluded the backup it had just created + * before slicing, so three files survived: the new one plus two older. At the + * reported ~650-750 MB apiece that is the difference between ~1.4 GB and + * ~2.1 GB retained, so it is not a rounding error, and picking the smaller + * number would delete recovery data users currently have. Deciding which + * number is actually wanted is a maintainer call; this change only makes the + * number explicit and honest. + */ +export const EMERGENCY_BACKUP_RETENTION = 3 + +/** True when *name* is one of the emergency backups this module manages. */ +export function isEmergencyBackup(name: string): boolean { + return ( + typeof name === 'string' && + name.startsWith(EMERGENCY_BACKUP_PREFIX) && + name.endsWith(EMERGENCY_BACKUP_SUFFIX) + ) +} + +/** + * Given every filename in the Hermes home directory, return the emergency + * backups that should be deleted -- everything past the newest *retain*. + * + * Ordering is lexicographic and deliberately so: the timestamp is + * `new Date().toISOString()` with `:` and `.` replaced by `-`, which is + * fixed-width and zero-padded, so byte order and chronological order agree. + * That keeps the sweep independent of file mtimes, which a backup/restore or + * a sync client can rewrite. + * + * Non-backup files are ignored, so this is safe to hand a whole directory + * listing. Returns names, not paths; the caller owns the directory. + */ +export function selectEmergencyBackupsToDelete( + names: readonly string[], + retain: number = EMERGENCY_BACKUP_RETENTION +): string[] { + const keep = Number.isFinite(retain) && retain > 0 ? Math.floor(retain) : 0 + + return names.filter(isEmergencyBackup).sort().reverse().slice(keep) +} diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index eaf5ad999176..f667ad4459ee 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -132,6 +132,10 @@ import { uninstallArgsForMode } from './desktop-uninstall' import { describeDevCdpDecision, resolveDevCdpPort } from './dev-cdp' +import { + EMERGENCY_BACKUP_RETENTION, + selectEmergencyBackupsToDelete, +} from './emergency-backup-retention' import { installEmbedReferer } from './embed-referer' import { createEventDeduper } from './event-dedupe' import { @@ -3898,9 +3902,48 @@ function runningAppBundle() { // desktop Electron process itself, before the backend is killed and // before the updater is spawned — a separate safety net from the // Python-level pre-update snapshot inside `hermes update`. +// Delete emergency state.db backups past the retention budget. Never throws: +// this is disk hygiene on the update path, and a failure to reclaim must not +// be able to abort an update. +function pruneEmergencyStateDbBackups(hermesHome, rememberLog) { + try { + const stale = selectEmergencyBackupsToDelete(fs.readdirSync(hermesHome), EMERGENCY_BACKUP_RETENTION) + + let reclaimed = 0 + + for (const name of stale) { + const target = path.join(hermesHome, name) + + try { + const bytes = fs.statSync(target).size + + fs.unlinkSync(target) + reclaimed += bytes + } catch { + // Held open, already gone, or not ours to delete. The next update + // tries again; one unremovable file must not stop the rest. + void 0 + } + } + + if (reclaimed > 0) { + rememberLog(`[updates] reclaimed ${reclaimed} bytes from ${stale.length} old state.db backup(s)`) + } + } catch { + void 0 + } +} + function preflightStateDb(hermesHome, rememberLog) { const stateDbPath = path.join(hermesHome, 'state.db') + // Unconditionally, and BEFORE any early return. Every reason this function + // gives up (no state.db, too small to be a database, stat throws) and every + // reason the copy below fails (ENOSPC on a disk these very files filled, + // EBUSY from the running app on Windows) used to skip the sweep, so the + // backups accumulated precisely when reclaiming mattered most (#91229). + pruneEmergencyStateDbBackups(hermesHome, rememberLog) + if (!fileExists(stateDbPath)) { rememberLog('[updates] state.db pre-flight: not found (fresh install?)') @@ -3943,30 +3986,12 @@ function preflightStateDb(hermesHome, rememberLog) { rememberLog(`[updates] emergency state.db backup: ${emergencyPath} ` + `(${emergStat.size} bytes)`) - // Prune to the 2 most recent emergency backups. - try { - const homeDir = fs.readdirSync(hermesHome) - - const backups = homeDir - .filter( - f => - f.startsWith('state.db.pre-update-emergency-') && - f.endsWith('.bak') && - f !== path.basename(emergencyPath) - ) - .sort() - .reverse() - - for (const old of backups.slice(2)) { - try { - fs.unlinkSync(path.join(hermesHome, old)) - } catch { - void 0 - } - } - } catch { - void 0 - } + // Reclaim now that a new backup exists, so the newest one counts + // toward the retention budget. The unconditional sweep at the top of + // preflightStateDb is what guarantees this happens at all; this second + // call only keeps the budget from being exceeded by one until the next + // update runs. + pruneEmergencyStateDbBackups(hermesHome, rememberLog) } catch (copyErr) { rememberLog(`[updates] emergency state.db backup failed: ${copyErr.message}`) } From 075546f58cbf4f117bae57955fc740152dc282fc Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:47:26 -0500 Subject: [PATCH 2/2] fix(desktop): order the emergency-backup-retention import per perfectionist/sort-imports check:lint enforces perfectionist/sort-imports, and './embed-referer' sorts before './emergency-backup-retention' (emb < eme). Linting only the two new files locally missed it because the violation is in main.ts. --- apps/desktop/electron/main.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index f667ad4459ee..0292159f6f37 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -132,11 +132,11 @@ import { uninstallArgsForMode } from './desktop-uninstall' import { describeDevCdpDecision, resolveDevCdpPort } from './dev-cdp' +import { installEmbedReferer } from './embed-referer' import { EMERGENCY_BACKUP_RETENTION, selectEmergencyBackupsToDelete, } from './emergency-backup-retention' -import { installEmbedReferer } from './embed-referer' import { createEventDeduper } from './event-dedupe' import { buildTerminalScript,