Conversation
…p fails
`preflightStateDb` copies `state.db` to
`state.db.pre-update-emergency-<ts>.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 NousResearch#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. NousResearch#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 (NousResearch#53040), replaced on each pack rather
than accumulated, and deleting it would remove a recovery path.
alt-glitch
added
type/bug
…ionist/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.
|
This improves failed-copy retention handling, but the successful-copy path still uses |
|
Agreed on all of it, and thanks for splitting it into #91636 rather than asking
The strongest corroboration is a comment you did not cite, # SQLite sidecar files — the backup takes a consistent snapshot of ``*.db``
# via ``sqlite3.backup()``, so shipping the live WAL / shared-memory /
# rollback-journal alongside would pair a fresh snapshot with stale sidecar
# state and produce a torn restore on the next open.So the codebase has already reasoned about exactly this and landed on your One thing worth carrying into #91636, since it sits ten lines above the copy: On the fix: This PR stays as-is. Its claim is that the retention sweep was nested inside |
This branch has not been deployed
3 participants
What does this PR do?
Fixes the residue half of #91229:
state.db.pre-update-emergency-*.bakfiles that accumulate to gigabytes.There is already a prune. The bug is where it sits. In
preflightStateDb:Reclaiming old backups is nested inside the success path of writing a new one. So every path that stops the copy also skips the cleanup:
ENOSPC— disk fullEBUSY/EPERM— another process holdsstate.dbstate.dbmissing, or ≤100 bytesreturnbefore the copy is attemptedstatSyncthrowsThe janitor only ran on the days nothing needed cleaning. With a ~650-750 MB database and a self-update that fails repeatedly (the reporter lists 8/15, 8/17, 8/20), that is exactly the reported 2.5-3 GB.
The fix
Hoist the sweep into
pruneEmergencyStateDbBackupsand call it unconditionally at the top ofpreflightStateDb— before any early return, before the copy. Both update entry points (main.ts:3565andmain.ts:4034) funnel through that function, so both reclaim.Selection logic moves to
emergency-backup-retention.tsso it can be tested without booting Electron, matching how the other extracted electron helpers (profile-delete-routing.ts,bundle-skew.ts, …) are structured.main.tskeeps the I/O.Two behaviours are preserved deliberately:
A maintainer decision I did not make for you
The old comment said "Prune to the 2 most recent emergency backups". The code kept three: the filter excluded the just-written backup before
.slice(2), so the new one plus two older ones survived.At ~700 MB apiece that is ~1.4 GB versus ~2.1 GB retained, so it isn't a rounding error. I kept the effective behaviour (three) rather than the documented one (two), because lowering it deletes recovery data users currently have, and that is a data-retention call rather than a bug fix. It's now a named constant with the discrepancy written down:
If maintainers want two, it's a one-line change and the tests read the constant rather than hardcoding counts, so they follow it. I'd rather surface the disagreement than silently pick.
What I deliberately did NOT fix, and why
The issue also reports
win-unpacked.bak(~380 MB) as uncleaned residue. It isn't residue.apps/desktop/scripts/before-pack.mjspreserves the previous unpacked tree as<appOutDir>.bakspecifically so a corrupt pack can be rolled back — that's #53040's rename-instead-of-delete, and_ensure_desktop_exe_launchablerestores from it. It is one copy, replaced on each pack, not an accumulating set.So "clean up the .bak files" would have deleted a recovery mechanism. Two artifacts that look alike, opposite lifecycles. Flagging it rather than acting on it.
The other two proposals in the issue — staged/atomic update and self-shutdown before replace — are the locking half and are not touched here. Note that #70477 (@JonthanaHanh) is already doing the stop-before-replace work in this same file; its hunks are at ~2494/2687 and never touch the prune at ~3843, so there's no conflict between them. That PR and this one address different halves of #91229 and are complementary.
Related Issue
Fixes #91229 — partially. The residue half is fixed here; the file-locking half remains open and is the larger piece. If maintainers would rather this not auto-close the issue, say so and I'll change the keyword to
Related to.Type of Change
Changes Made
apps/desktop/electron/emergency-backup-retention.ts— new.isEmergencyBackup,selectEmergencyBackupsToDelete, and the retention constant. Pure, nofs.apps/desktop/electron/main.ts— newpruneEmergencyStateDbBackups(does the I/O, never throws), called unconditionally at the top ofpreflightStateDb; the inline prune is removed. The post-copy call is kept so the newly written backup counts toward the budget immediately.apps/desktop/electron/emergency-backup-retention.test.ts— new, 14 tests.How to Test
Full
--project electronrun, this branch vs its base (533886c8b8), same machine (Windows 11):On the failure delta, stated honestly rather than rounded off. Diffing the sorted
FAILlists gives one branch-only failure —git-worktree-ops.test.ts > ensureGitRepo: inits a plain dir with a root commit— and two base-only ones. None is anywhere near this diff, and I did not want to wave that away, so I re-rangit-worktree-opsthree times against unchanged branch code: it passed, then failed, then the run timed out. It is flaky, not a regression. The remaining ~31 are Windows-environment failures (chmod/symlink permission semantics inhardening.test.ts, ssh control sockets inssh-connection.test.ts) that fail identically on the base commit.Mutation proof
The first mutation is the important one: it is exactly the state
mainis in today. Note that it only breaks one test, and that test is a source-level assertion rather than a behavioural one — the selection logic is completely correct in both states, because the defect was never in what to delete, only in when the sweep runs. A purely behavioural test suite would have passed the buggy code. That's why the wiring assertion exists and why it anchors onif (!fileExists(stateDbPath))rather than on the wordreturn, which appears in the surrounding comments.Checklist
Code
fix(desktop): ...)pre-update-emergencyprune (checked the diffs of feat: add release update channel #24938, fix(update): name a repair command when state.db fails its post-update check #89127, fix(kanban): honor manual orchestration dispatch gate #54295, fix(security): enforce 0600 on snapshot copies of secret files (#77470) #78288, fix(desktop): make Windows package rebuild transactional and self-healing #91079, all 0 hunks). fix(desktop): stop gateway before update + preserve release dir in ZIP fallback (#70337) #70477 is the nearest neighbour in this file and is analysed above.Documentation & Housekeeping
readdir/unlinkwith no platform branches and runs on every OS. The bug is worst on Windows (file locking makes the copy fail most often there) but the coupling was never Windows-specific: an ENOSPC on Linux disabled cleanup the same way.