Repository navigation
fix(worlds): allow deleting world backups and enforce backup limits - #659
Conversation
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Thanks for picking this up. A delete action per row and a cap on the record list are the right pieces, and going through deleteInstallationBackup is what the issue suggested. Two things have to change before this goes in: the new deletion routes skip the deletion policy, and the prune can lose backups or leave archives with no row, partly in ways the Installation prune was recently fixed to avoid. A third is about what the player is told.
Line numbers are at 8e0e4f2a. The P and D references are probes run against the real handlers and the real page through the existing test harnesses, in throwaway folders; each one's result is in the table under Validation.
Blocking
1. Both new routes delete whatever path a record holds
deleteWorldBackup hands backup.path to a port whose remove is a bare fse.remove (src/ipc/handlers/worldsHandlers.ts:410-420), and the prune calls fse.remove(existing.path) the same way (:153). fse.remove is recursive and neither route checks the path. The path comes from the stored config: normalizeWorldBackup only wants a non-empty string (src/config/configManager.ts:578-596), and getEntryGrants grants every world backup path by itself (src/ipc/pathPolicy.ts:118-121), so a record is its own authorization. On dev, the only code that deletes an archive a record names is Installation deletion, which goes through DELETE_PATH and assertManagedDeletionPath (src/ipc/handlers/pathsHandlers.ts:289). deleteWorld runs the same check (worldsHandlers.ts:249).
With a hand-edited config, or one carried in with a portable profile, this head does the following:
- P1a: Delete Backup on a record whose path is a folder outside every managed root deletes the folder and what is in it, and answers
{ ok: true }. - P1b: on a record whose path is the configured Backups folder, it deletes the whole folder, Installation archives included.
assertManagedDeletionPathrefuses that same path as protected. - P1c: records pointing at the live world in
Savesand at an Installation archive delete both. Those two passassertManagedDeletionPath, so the policy alone does not close this. - P2: the prune does the same on a plain "Back up this world" click once such a record falls past the limit. The player never asked for a delete.
- P11: the renderer can plant such a record as well.
SAVE_CONFIGkeeps the renderer'sworldBackupsfor an Installation id the stored config does not have yet (src/ipc/handlers/configHandlers.ts:35), andassertConfigPathsAuthorizedadmits the configured folders and anything under them. Saving a config that adds an Installation with a record at the Backups folder answered{ ok: true }, and Delete Backup on that record then removed the folder.
Every world archive the launcher has written since the feature landed is <Backups>/Worlds/<id>.tar.gz (worldsHandlers.ts:206-207). Both routes should go through one helper that refuses anything but a regular file named ${backup.id}.tar.gz in a folder named Worlds, then runs assertManagedDeletionPath for the protected roots and the symlink walk, and treats a missing file the way deleteInstallationBackup already does. Pinning the name and the parent folder, rather than the current Backups folder, keeps old records deletable after the Backups folder is changed in Config.
2. The prune can lose backups, and rows whose archives stay
#641 noted that world backups had no prune and so did not share #610. The prune this PR adds is written by hand, misses two rules the Installation prune follows, and brings two problems of its own:
- It reads unreachable as gone.
fse.pathExistsis false for an archive on an unplugged drive, an offline share or a Backups folder that moved, and the record is dropped and reported deleted. In P3 the drive comes back and the archive is there with no row.archiveState(src/domain/installations/backup.ts:99-121) exists for exactly this, and its tests say so ("keeps a record whose folder is not there, uncounted, and deletes nothing because of it", "leaves every record alone when the drive holding the archives is not connected"). - It drops the row when the delete fails.
.catch(() => undefined)swallows the error and the id is still reported deleted: in P10 the archive sits in a read-only folder, the file stays and the row goes. The Installation prune answersprune-failedand keeps the record. - It deletes before it commits. Old archives go inside
saveWorldBackupRecord, beforesaveConfig(:147-164). When that write fails (a lockedconfig.json, or a disk the new archive has just filled),makeWorldBackupremoves the new archive as well and answersoperation-failed, which the player reads as "The world operation failed. Nothing else was changed." In P4 (limit 2, two backups) the failed save leaveswb-2deleted from disk but still listed inconfig.json, and onlywb-1on disk. - The 100 slice (
:158, and the same.slice(0, 100)in the page) drops the oldest rows of any world once the Installation holds more than 100, without deleting their archives or reporting them. At the limit field's maximum of 10, eleven worlds are enough. In P5, backing upW10dropped the only row of a world the player had deleted after backing it up, and its archive stayed on disk. The same cap innormalizeInstallationtruncates, on the first launch after updating, any config that already holds more than 100 world backups from beta.12, where nothing capped them.
Three of these end where the issue started, turned around: an archive with no row, which nothing in the launcher can reach afterwards, since Installation deletion only removes archives that have a row. The commit order lands on the issue's original symptom instead, a listed backup whose archive is gone, and loses older backups on the way.
Pruning after the new archive is written is already better than the Installation flow, which prunes before it compresses. It only needs to move past the save: commit the new record first (what saveWorldBackupRecord did before this PR), then prune this world's older records, drop only the rows whose archive is confirmed gone, and save again. Exporting pruneOldestBackups from the domain and calling it with a main-process port whose remove is the helper from point 1 brings the three-state rule and the failed-delete handling with it. If that second save fails, the dead rows it leaves are cleared by the next backup or a manual delete, as #507 intended. And with a per-world limit doing the real work, the cap only has to stop a hand-edited config growing without end: a ceiling like the 1,000 the config uses for installations, and no slice in saveWorldBackupRecord or the page.
3. Nobody tells the player, and the first backup after updating clears everything past the limit
World backups had no limit until now, so a player may hold many of one world. In P6b, with 15 backups of a world and the default limit of 3, the next "Back up this world" deletes 13 archives. The confirmation asks "Back up World.vcdbs now?" and the toast says "World backup created." (D3). Nothing near the Backups limit field says it now caps each world too. Which limit applies is the question #620 carried over from #465, and sharing backupsLimit per world is a fair answer, but it has to show where it acts: a consequence line on the backup confirmation when older backups of that world will go, with the count, and the choice stated in the description. That means a new string in en-US and fr-FR and a regenerated docs/contribute/translation-status.md.
Smaller, same decision: backupsLimit 0. On dev it turns backups off: makeInstallationBackup refuses with backups-disabled, and the player is told the Backups max amount is set to 0. Here it makes each world backup delete every older backup of that world (P6a). The form will not save 0 (src/renderer/src/features/installations/pages/AddInstallation.tsx:72, EditInstallation.tsx:102), so it only comes from a hand-edited config, but Math.max(1, ...) does not change anything there: at 0, maxExisting is -1 and the loop already treats every older record as stale, and mutating the guard away survives the suite. Give 0 a meaning (no prune, or a refusal like Installation backups) and a test.
Not blocking
- When the record cannot be saved after the archive is gone (P7), the answer is
operation-failedwith the same "Nothing else was changed." and the page keeps the row.saveConfigkeeps the change in memory even when the write fails (configManager.ts:144), so for the rest of the session Delete and Restore on that row answer "World backup not found." (P7b), which is the issue's own symptom. Rare, but the message should not claim nothing changed. - The confirmation reuses the Installation backup strings, so it names neither the world nor the date, and every row button is called "Delete Backup". For a world that only exists as backups, deleting its last one ends the world for good after one generic prompt, while deleting a live world asks for its name. A question naming the world and the date, which says so when it is the last copy, would match
confirmRestore. - After a confirmed delete, focus falls to
body(D2) because the button that opened the dialog is gone. The rest of the dialog behaves (D1): the page behind it is hidden, Cancel comes before Delete, Escape closes it and focus comes back to the row. docs/get-started/usage/game-client/worlds.mdcould say that a backup can be deleted from its row and how many each world keeps.- Tests cover the success paths only. The coverage report leaves the failure branches of
deleteWorldBackup(lines 399 to 425) and of the record save (137-139, 165, 218-219) unrun. Of 28 mutants on the new lines, 17 survive. Three are equivalent (theMath.max, the slice thatsaveConfigrepeats, the refresh after a delete); the others go unnoticed: delete while playing, no string check on the id, no lease, a failed archive delete or record save reported as success,deletedBackupIdsreturned after a failed save, the new archive kept after a failed save, the page ignoringdeletedBackupIds, dropping its slice or removing the row after a failed delete, and the dialog's title, destructive variant and success toast. The description says the tests validate inputs; only the unknown id is there. - Sonar flags an
awaitin a loop and a nested promise on the prune loop (:149,:153). Both go away with the domain prune.
Validation
At 8e0e4f2a, whose parent is dev's tip 9013f93c (GitHub reports it mergeable):
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:cipasses with 0 errors and 12 warnings, all in files this PR does not touch.npm run test:coverage: 279 files, 5,023 tests passed, 4 skipped. 94.29% statements, 90.36% branches, 95.2% functions, 96.16% lines, above every floor.- CI is green at this head: build on Ubuntu and Windows, lint, typecheck, test, the four test-matrix jobs and sonarcloud (gate OK, coverage on new code 81.1%). The macOS build was skipped.
- No locale file changes, and all 14 round-trip identically through the Weblate writer.
- Concurrency and state behave: a delete while a backup of the same Installation runs answers
world-busyand keeps the archive (P8), and restore takes the same lease (worldsHandlers.ts:272). A row whose file is already gone deletes cleanly (the PR's own test). The page appliesdeletedBackupIdsthe way main does (D3), and sinceSAVE_CONFIGkeeps main'sworldBackupsfor an existing Installation (configHandlers.ts:31-37), the page and the stored config only part after a failed record save (P4, P7b). - Mutation: 28 mutants on the new lines of
worldsHandlers.ts,configManager.tsandManageInstallationWorlds.tsx, each run against the nine test files that exercise those modules: 11 killed, 17 survived, 3 of them equivalent.
| Probe | Setup | Observed |
|---|---|---|
| P1a | Delete Backup, record path is a folder outside every managed root | { ok: true }, folder and its file gone |
| P1b | record path is the configured Backups folder | the policy throws "Protected path"; the handler answers { ok: true } and the folder is gone with an Installation archive in it |
| P1c | records at Saves/World.vcdbs and at an Installation archive |
both pass the policy, both deleted |
| P2 | limit 2, second record of the world is an outside folder, Back up | deletedBackupIds: ["crafted"], folder gone |
| P3 | limit 3, one record in a folder that is missing, Back up, then the folder comes back | reported deleted and dropped; the archive is back with no row |
| P4 | limit 2, two archives, config.json not writable, Back up |
operation-failed; wb-2 deleted yet still listed on disk; new archive removed |
| P5 | limit 10, 99 records over ten worlds plus the only backup of a deleted world as the oldest, back up W10 |
deletedBackupIds: []; that world's row gone, its archive still on disk |
| P6a | limit 0, two archives, Back up | both deleted |
| P6b | limit 3, 15 archives, Back up | 13 deleted, 3 left |
| P7, P7b | Delete Backup with config.json not writable, then again once it is |
operation-failed with the archive deleted; then archive-not-found for Delete and for Restore |
| P8 | Delete Backup while a backup of the same Installation compresses | world-busy, archive kept |
| P9 | non-string id; unknown Installation | invalid-request; installation-not-found |
| P10 | limit 2, oldest archive in a read-only folder, Back up | reported deleted, row dropped, file still there |
| P11 | SAVE_CONFIG adding an Installation with a record at the Backups folder, then Delete Backup |
{ ok: true } twice, Backups folder gone |
| D1 | open the delete confirmation, press Escape | named "Delete Backup", aria-modal, page behind it aria-hidden, Cancel before Delete; Escape closes it without deleting, focus back on the row button |
| D2 | confirm a delete | focus on body |
| D3 | Back up answering two deletedBackupIds |
confirmation "Back up World.vcdbs now?", toast "World backup created.", one row left |
| D4 | Delete answering operation-failed |
row kept, error toast |
Verdict: changes requested. Points 1 and 2 can delete files nobody chose to delete, lose backups and leave archives with no row; point 3 changes what an existing setting does without telling the player.
8e0e4f2 to
a09b84e
Compare
Translation statusen-US is the source and carries 917 keys.
Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
|
Thanks for the detailed review and test probes. I have addressed the blocking points and nonblocking feedback:
|
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
The new commit settles most of round one. Both routes refuse every record the first round used, the prune is the domain one and runs after the commit, the cap is 1,000, limit 0 refuses, and the backup confirmation now says how many older backups go. Two things still stand in the way: removeWorldBackupArchive has three gaps, all reached through a crafted record, which is what point 1 was about, and docs/contribute/translation-status.md was not regenerated.
Line numbers are at a09b84ea. P and D keep their round-one names and run again on this head, with P4b, P12, D3b and D5 to D7 added; the round-one files themselves, rerun unchanged, now fail 10 of their 14 handler probes, which is the behaviour changing. B probes are new and try to get past the helper. Everything runs against the real handlers and the real page, in throwaway folders.
Round one, item by item
1. Both routes deleted whatever a record held
Settled for every case raised. P1a, P1b and P1c answer operation-failed, and the outside folder, the Backups folder, the live world and the Installation archive stay. The prune stops on the record of P2 without touching it, and the record planted through SAVE_CONFIG in P11 is refused. Links are refused whether they are the archive, the Worlds folder or a folder higher up (B1 to B3), and so are a folder named <id>.tar.gz (B6) and case variants (B7). A hard link loses only its name (B12). A record planted through SAVE_CONFIG now reaches only what DELETE_PATH already reaches (B11). A Worlds folder outside the configured roots stays deletable because the record grants its own path (B5), which is the trade round one suggested so old records survive a change of Backups folder.
Three gaps are left, each a line or two:
- The prune calls the helper without the id (
worldsHandlers.ts:228). In B9 a record ofWorld.vcdbswhose path is another world's archive gets that archive deleted on the next "Back up this world", and the other world's row stays listed with nothing behind it, the issue's own symptom. The delete route refuses the same record. Round one asked for${backup.id}.tar.gzon both routes. - The helper checks one path and deletes another.
assertManagedDeletionPathcollapses..before its symlink walk (pathPolicy.ts:161,validation.ts:228), while the file system follows links first, and the helper then lstats and removes the string it was handed (:141-143). In B4 a link<Backups>/linkpoints elsewhere: a record at<Backups>/link/../Worlds/b4.tar.gzpasses the policy as<Backups>/Worlds/b4.tar.gz, which only has to exist, but deletes theWorlds/b4.tar.gzthat sits next to the link's target, outside every managed root, on both routes (B4p).DELETE_PATHremoves the path the policy returns (pathsHandlers.ts:289-291). The helper should do the same and run its name checks on that path. Windows collapses..before it opens anything, so this one should be Linux and macOS only (not tried on Windows). Worldsis not only the world archive folder. An Installation named "Worlds" writes its archives to<Backups>/Installations/Worlds/(backup.ts:213-215), and in B8 a record there deletes one, leaving that Installation's backup row without its archive. Every world backup id has been arandomUUID()since the feature landed (:204), so requiring a UUID stem costs nothing in use and keeps Installation archives out.
2. The prune
Settled. An unreachable record stays and is not reported (P3). A delete that fails keeps its row and its file (P10). The new record is committed before anything is pruned, so a failed save leaves both older archives in place (P4). The cap is 1,000 in normalizeInstallation, the page no longer slices, and P5 keeps the deleted world's only row (101 rows). Both Sonar flags went with the hand-written loop.
3. Telling the player
Settled, apart from the status page. The confirmation carries the count, "The 13 oldest backups of this world will be deleted to stay within the backup limit." for 15 backups at a limit of 3, the singular at 3 and nothing at 2 (D3, D3b), and the French plural forms read right (D6). Limit 0 refuses with backups-disabled before anything is compressed (P6a), with a test. The new strings are in en-US and fr-FR only, at the same position, drafted.json is untouched, and all 14 locale files round-trip identically through the Weblate writer.
docs/contribute/translation-status.md is not in the diff, though the PR comment says it was refreshed. It still says en-US carries 902 keys where this head has 908, and the status comment on this PR flags it (npm run i18n:status -- --write docs/contribute/translation-status.md). The Backups max amount field still says nothing about worlds (BackupsSettingsSection.tsx:35). With the count on the confirmation and the docs page updated, that one can wait.
Not blocking items from round one
- P7 and P7b are unchanged: a failed record save on Delete still answers "Nothing else was changed." with the archive gone, after which Delete and Restore on that row answer "World backup not found." for the rest of the session. The cause,
saveConfigkeeping a failed write in memory (configManager.ts:142-144), now reaches two more paths. A failed record save on Back up removes the new archive but leaves its row in memory, and the next write saves that row (P4b). A failed second save leaves the older archives deleted anddeletedBackupIdsempty, so the page keeps rows main has dropped (P12); reporting the ids whatever that save answers is the domain's own rule (backup.ts:136). - Confirmation wording: settled. The question names the world and the date and says when it is the last copy (D1), with an apostrophe and the date left unescaped (D5). The line under it is still the Installation backup sentence about "your worlds, data and other info".
- Focus after a confirmed delete still lands on
body(D2). - Docs: settled.
- Tests: better, with playing, the lease, a non-string id, crafted paths, limit 0, unreachable records, the consequence line, the last copy and a failed delete. But all three records of the crafted-path test stop at the extension check, so no test reaches the id check, the
Worldscheck, the file-type check or the policy call on its own, and none runs the prune on a crafted record, a delete that fails or a save that fails. 24 of 44 mutants survive (see Validation). - Sonar: settled. One minor smell is new,
.findused as a boolean at:153.
New in this commit
The prune stops at the first archive it cannot delete, and nobody hears of it. With the records of P10 the confirmation promises that the 2 oldest backups will go, yet the world ends with four rows at a limit of 2, the toast says "World backup created.", and every later backup stops at the same file. The Installation flow refuses with pruneFailed in that case. Here the new backup already exists, so a warning saying how many older backups could not be deleted would do, and the domain already hands back failedBackupId.
At limit 0 a world with no backup can no longer be deleted from the page: Delete offers a backup first, that backup is refused, and the world stays (D7). Only a hand-edited config holds 0, so this is minor. Skipping the offer when backups are off would fix it.
The PR description still gives the cap as 100 and the limit as Math.max(1, backupsLimit).
Validation
At a09b84ea, rebased on dev's current tip 682751bd with the round-one commit unchanged (GitHub reports it mergeable):
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:cipasses with 0 errors and 12 warnings, all in files this PR does not touch.npm run test:coverage: 279 files, 5,045 tests passed, 4 skipped. 94.32% statements, 90.39% branches, 95.34% functions, 96.2% lines, above every floor. InworldsHandlers.tsthe helper's lines 134, 135, 142 and 146, the failed first save (217-218), the failed second save (237) and the failed record save on Delete (443) never run.- CI is green at this head: build on Ubuntu and Windows, lint, typecheck, test, the four test-matrix jobs and sonarcloud (gate OK, 86.7% coverage on new code). The macOS build was skipped.
- The domain changes are exports and wider parameter types.
makeInstallationBackupcalls the prune as before and its tests pass; restore is untouched. - Mutation, against the ten test files that exercise the changed modules: 16 of the 28 round-one mutants hit code this commit replaced; of the other 12, 7 are killed and 5 survive (2 equivalent). Of 32 mutants on the new lines, 13 are killed and 19 survive (2 equivalent). The suite still passes with any one of the helper's checks removed, with a refusal inside it read as success, with the prune bypassing it, or with the delete route dropping the id. It also passes when a failed record save on Back up or Delete is ignored or keeps the new archive, whatever is reported after a failed second save, without the guard on a missing Installation, with the consequence counted across every world, with the last-copy sentence ignoring a live world, with the page ignoring
deletedBackupIds, and without the dialog's title, date, destructive variant or success toast.
| Probe | Setup | Observed at a09b84ea |
|---|---|---|
| P1a, P1b, P1c | Delete Backup on records at an outside folder, the Backups folder, the live world, an Installation archive | operation-failed each time, everything kept |
| P2 | limit 2, oldest record of the world is an outside folder, Back up | folder kept, deletedBackupIds: [], 3 rows |
| P3 | limit 1, one record on a missing drive, Back up, the drive comes back | row kept, not reported, archive there |
| P4, P4b | limit 2, config.json not writable, Back up; then a write succeeds |
operation-failed, both older archives kept; the next write saves a row whose archive is gone |
| P5 | 100 records over ten worlds plus a deleted world's only backup, back up W10 |
101 rows, that row and its archive kept |
| P6a, P6b | limit 0 with two archives; limit 3 with 15 | backups-disabled, nothing compressed or deleted; 13 deleted, 3 left |
| P7, P7b | Delete with config.json not writable, then again |
unchanged: operation-failed with the archive gone, then archive-not-found for Delete and Restore |
| P8, P9 | Delete during a backup; non-string id; unknown Installation | world-busy; invalid-request; installation-not-found |
| P10 | limit 2, three older records, the oldest in a read-only folder | file and row kept, deletedBackupIds: [], 4 rows |
| P11 | SAVE_CONFIG adds an Installation with a record at the Backups folder, then Delete Backup |
saved, delete refused, folder kept |
| P12 | limit 2, the second save fails | wb-2 deleted, deletedBackupIds: [], Delete on its row answers archive-not-found |
| B1, B2, B3 | the archive is a link; Worlds is a link; a link higher up |
refused, targets kept |
| B4, B4p | record at <Backups>/link/../Worlds/b4.tar.gz, the link pointing outside; Delete, then the prune |
the policy vets <Backups>/Worlds/b4.tar.gz; { ok: true }, the file behind the link deleted, the vetted one kept |
| B5 | record at Worlds/<id>.tar.gz outside the configured roots |
admitted (the record grants itself), deleted |
| B6, B7 | a folder named b6.tar.gz, with and without a trailing separator; worlds/, B7U.tar.gz for id b7u, .TAR.GZ |
refused, kept |
| B8 | record at an archive of an Installation named "Worlds" | { ok: true }, archive deleted, that Installation's backup row left without it |
| B9 | limit 2, record of World.vcdbs whose path is another world's archive |
Delete refuses it; Back up deletes the other world's archive, its row stays |
| B10 | records at <Backups>/Worlds and at <Backups>, prune at limit 1 |
refused, the prune stops, everything kept |
| B11 | SAVE_CONFIG plants a record at another Installation's world archive |
deleted; assertManagedDeletionPath admits the same path |
| B12 | the archive is a hard link | { ok: true }, the other name kept |
| D1, D2 | open the delete confirmation, Escape; confirm a delete | named "Delete backup", modal, Cancel first, Escape gives focus back; after a delete, focus on body |
| D3, D3b | Back up with 15 backups at limit 3; with 3 and with 2 | "The 13 oldest backups...", 3 rows after; the singular; no line |
| D4 | Delete answering operation-failed |
now the PR's own test: row kept, error toast |
| D5, D6 | world Bob's World.vcdbs; fr-FR |
not escaped; "La plus ancienne sauvegarde...", "Les 13 plus anciennes sauvegardes...", "Supprimer définitivement cette sauvegarde de World.vcdbs du 04/10/2026 14:00:00 ?" |
| D7 | limit 0, delete a world with no backup | offer shown, backup refused, world kept |
Verdict: changes requested. The helper should check the id on both routes and remove the path it vetted, ideally with a UUID stem as well, and the status page has to be regenerated. Everything else from round one is settled or down to notes.
a09b84e to
7696c54
Compare
|
Updated to address review feedback:
Verification:
|
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Both blocking points from round two are settled. The helper now gets each record's own id on both routes, removes the path the policy vetted and refuses any id that is not a UUID, which on its own keeps every Installation archive out. The status page is regenerated. Nothing I found this round blocks. One line is worth taking out before this goes in (the Installations check, see New), and the rest are notes.
Line numbers are at 7696c545. The round-two probe files, rerun unchanged, fail 9 of their 36 probes: eight because their ids are not UUIDs, which the helper now refuses before anything else, and D7 by design. (The round-one files fail 16 of 18, on behaviour round two had already changed and on the button's new name.) So every P and B probe was run again on UUID ids, against the real handlers and the real page, in throwaway folders. B4o, B6f, B8n, B9u, B9i, B11o, B13, B14 and R1 are new.
Round two, item by item
(a) removeWorldBackupArchive. Settled, all three gaps:
- The prune hands each record's own id to the helper (
worldsHandlers.ts:232-238). In B9 a record ofWorld.vcdbsnaming another world's archive is refused by both routes, and that archive and its row stay. - The helper removes the path
assertManagedDeletionPathreturns and runs its name checks on it (:141-150). In B4 and B4p the file behind the link stays and<Backups>/Worlds/<id>.tar.gzis the one removed, which is whatDELETE_PATHwould do with the same string. - The UUID stem keeps Installation archives out by itself, since those are named
<name>_<stamp>.tar.gz(backup.ts:213). B8 is refused.
(b) docs/contribute/translation-status.md. Settled: 910 keys, and npm run i18n:status -- --write on this head leaves it as committed.
Round two's other items. D7 is settled: at limit 0, Delete goes straight to the world deletion once the name is typed, and at limit 3 the offer still comes up (D7, D7b). The PR description now gives the 1,000 cap and the limit 0 refusal. P12 is settled too, since the deletions are reported whatever the second save answers.
The prune still stops in silence at an archive it cannot delete. With three backups at limit 2, the oldest in a read-only folder, the confirmation says "The 2 oldest backups of this world will be deleted to stay within the backup limit.", main answers deletedBackupIds: [], the toast says "World backup created." and the world keeps 4 rows, then 5 after the next backup (P10, D8). pruneOutcome.ok and failedBackupId are already in hand at :241, so a warning toast would do. Not blocking, as in round two.
Unchanged and still not blocking: focus lands on body after a confirmed delete (D2), the line under the delete question is still the Installation backup sentence (D1), and P4b, P7 and P7b behave as in round two.
The tests grew, but they do not pin what this round fixed. The suite stays green with the prune reading the id off the file name (B9 back), with the helper removing the record's own path (B4 back), and without the UUID check, the regular-file check or the skip of the offer at limit 0. The helper's catch never runs, so a refusal read as success goes unnoticed as well, and failed record saves on Back up and on Delete are still untested. All five records of the crafted-path test stop at the UUID or the file name check. Three records would pin the fixes: one of World.vcdbs with its own UUID pointing at another world's UUID-named archive, one at <Backups>/link/../Worlds/<id>.tar.gz (Linux and macOS only), and one whose id is an Installation archive's stem, which only the UUID check stops once line 145 is gone. Not blocking, as in the earlier rounds.
New
Line 145 can go. With it, a Backups folder that is itself named "Installations" can no longer prune or delete a single world backup the launcher made: at limit 1 a second backup leaves both archives and both rows, and Delete on the first answers operation-failed (B8n). The check guards nothing the UUID stem does not already cover.
Left as notes, none of them blocking:
- A record that copies another backup's id along with its path still gets that archive deleted, by a backup of its own world (B9u, where both rows then go) or from another Installation (B9i, where the other row stays and its Restore answers
archive-not-found). Both need a hand-edited config, and the same file is already in reach ofDELETE_PATHand of deleting the Installation with its data, which removes every path its records name (delete.ts:81). - Three crafted shapes drop the row and keep the file: a real archive recorded with a trailing separator (B6f),
Worlds/<id>.tar.gz/../<id>.tar.gz(B14), andlink/../Worlds/<id>.tar.gzwhen nothing sits behind the link (B4o). The earlypathExistsat:138and the domain's own check look at the record's string, while the rest of the helper looks at the vetted path. Resolving the path once at the top would make all three either delete or refuse. No record the launcher writes has these shapes. - Swapping
Worldsfor a link between thelstatand the remove makesfse.removefollow it and delete what sits behind it, recursively (B13).DELETE_PATHhas the same window, and only a process that can already write in the Backups folder can use it. Since the helper has just checked for a regular file,fse.unlinkwould bring the worst case down to one file.
Validation
At 7696c545, two commits on dev's current tip ab5ad066 (a fast-forward; GitHub reports it mergeable):
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:cipasses with 0 errors and 12 warnings, all in files this PR does not touch.npm run test:coverage: 280 files, 5,065 tests passed, 4 skipped. 94.3% statements, 90.35% branches, 95.4% functions, 96.22% lines, above every floor. InworldsHandlers.tsthe helper'scatch(152), the failed first save (223-224), the prune'scatch(251-252) and the failed record save on Delete (452) never run.- CI is green at this head: build on Ubuntu and Windows, lint, typecheck, test, the four test-matrix jobs and sonarcloud (gate OK, 85.6% coverage on new code, no open issue). The macOS build was skipped.
- The new strings are in en-US and fr-FR only, at the same position (fr-FR's
_manysits between_oneand_other, as in its other plurals),drafted.jsonis untouched, and all 14 locale files round-trip identically through the Weblate writer. - Installation backups and restore: the domain change is still exports and wider parameter types,
makeInstallationBackupand its tests are unchanged, and a world restore works on a backup the prune kept and answersarchive-not-foundon one it removed (R1). - Mutation, against the ten test files that exercise the changed modules: 25 of the 60 earlier mutants hit code that has since been replaced; of the other 35, 19 are killed and 16 survive (4 equivalent). Of 15 mutants on this round's lines, 3 are killed and 12 survive (5 equivalent, mostly checks that the record's path and the vetted path each repeat).
- Not tried on Windows or macOS. On a case-insensitive file system, a record carrying another backup's id in upper case reaches that archive, which is B9u again, and
installationsgets past line 145, which the UUID stem covers. I found nothing else there.
| Probe | Setup | Observed at 7696c545 |
|---|---|---|
| P1a, P1b, P1c | records at an outside folder, the Backups folder, the live world, an Installation archive | operation-failed each time, everything kept |
| P2, P3 | limit 2 with an outside folder as the oldest record; limit 1 with a record on a missing drive | folder kept, deletedBackupIds: []; row kept, not reported |
| P4, P4b | config.json not writable on Back up, then a write succeeds |
operation-failed, older archives kept; the next write saves a row whose archive is gone |
| P5 | 100 records over ten worlds plus a deleted world's only backup | 101 rows, that row and its archive kept |
| P6a, P6b | limit 0; 15 backups at limit 3 | backups-disabled, nothing compressed; 13 deleted, 3 left |
| P7, P7b | Delete with config.json not writable, then again |
operation-failed with the archive gone, then archive-not-found for Delete and Restore |
| P8, P9 | Delete during a backup; non-string id; unknown Installation | world-busy; invalid-request; installation-not-found |
| P10 | limit 2, the oldest of three in a read-only folder, Back up twice | ok with deletedBackupIds: [], 4 rows, then 5 |
| P11 | SAVE_CONFIG plants a record at the Backups folder |
saved, delete refused, folder kept |
| P12 | limit 2, the second save fails | deletedBackupIds holds wb-2, the row is gone in memory and stays in config.json until the next write |
| R1 | Restore after a prune | the kept backup restores, the pruned one answers archive-not-found |
| B1, B2, B3 | the archive, Worlds or a folder higher up is a link |
refused, targets kept |
| B4, B4p | <Backups>/link/../Worlds/<id>.tar.gz, the link pointing outside; Delete, then the prune |
file behind the link kept, <Backups>/Worlds/<id>.tar.gz removed |
| B4o | the same record with nothing behind the link | { ok: true }, row dropped, <Backups>/Worlds/<id>.tar.gz kept |
| B5 | Worlds/<id>.tar.gz outside the configured roots |
admitted and deleted, the record grants itself (the round-one trade) |
| B6, B6f | a folder named <id>.tar.gz, with and without a trailing separator; a real archive recorded with one |
refused, content kept; { ok: true }, row dropped, file kept |
| B7 | worlds/, an upper-case name for a lower-case id, .TAR.GZ |
refused, kept |
| B8 | an Installation archive in Installations/Worlds; a UUID-named file planted beside it |
refused, both kept |
| B8n | Backups folder named Installations, limit 1, two backups, then Delete |
both kept, deletedBackupIds: []; Delete answers operation-failed |
| B9 | a record of World.vcdbs with its own id and another world's archive |
refused by both routes, archive and row kept |
| B9u | the same record carrying the other world's id | that archive deleted, both rows dropped |
| B9i | Installation A's record carrying B's id and path, A backs up at limit 1 | B's archive deleted, B's row kept, its Restore archive-not-found; DELETE_PATH admits the same file |
| B10 | records at Worlds and at the Backups folder, prune at limit 1 |
refused, kept |
| B11, B11o | SAVE_CONFIG plants a record at another Installation's archive; at a UUID file outside every root |
deleted, as DELETE_PATH would; refused with unauthorized-path |
| B12 | the archive is a hard link | only that name goes, the content stays |
| B13 | Worlds swapped for a link between the lstat and the remove |
{ ok: true }, the folder behind the link removed |
| B14 | Worlds/../Worlds/<id>, Worlds/<id>.tar.gz/../<id>, Worlds/../Installations/Worlds/<id>, Installations/Worlds/../../Worlds/<id> |
removed; row dropped and file kept; refused; removed |
| D1, D5, D6 | delete confirmation, a name with an apostrophe, fr-FR | as in round two |
| D2 | confirm a delete | focus on body |
| D3, D3b | 15 backups at limit 3; 3 and 2 at limit 3 | "The 13 oldest backups...", 3 rows after; the singular; no line |
| D7, D7b | delete a world with no backup at limit 0; at limit 3 | no offer, world deleted after the typed name; offer shown |
| D8 | the P10 records seen from the page | "The 2 oldest backups of this world will be deleted...", then "World backup created." and 4 rows |
Verdict: approve. Both blocking points are settled and nothing new blocks. I would still take line 145 out before this is merged, and the three test records above are worth adding at the same time.
7696c54 to
3285c08
Compare
World backups had no deletion mechanism from the launcher UI and lacked eviction limits, causing backup counts and disk space to grow without bound. - Add deleteWorldBackup and removeWorldBackupRecord in worldsHandlers - Expose deleteBackup via worldsManager preload bridge - Add individual backup delete button with confirmation dialog in ManageInstallationWorlds - Prune world backups exceeding backupsLimit and clean up missing archive files - Cap worldBackups at 100 in configManager normalization - Return deletedBackupIds in WorldBackupResult so renderer UI state stays synchronized - Add comprehensive unit and DOM tests for deletion and eviction limits - Fixes #620
…ng, and limit warnings - Enforce safe world backup deletion paths via removeWorldBackupArchive helper validating UUID format, archive filename match on both prune and delete routes, Worlds directory, and exclusion of Installations subfolder - Vet deletion target using assertManagedDeletionPath and remove the verified safe path - Always report deletedBackupIds from pruning to ensure renderer UI state stays synchronized - Skip offer to back up world before delete when backupsLimit is 0 - Fix Sonar smell in addWorldBackupRecord by checking presence via some - Regenerate docs/contribute/translation-status.md - Add tests covering UUID validation, path mismatch refusal, Installation folder exclusion, and prune protection against foreign world archives
3285c08 to
19d1476
Compare
2 participants
Summary
Allow individual world backups to be deleted one by one from the Manage Worlds page, and enforce retention limits on world backups to prevent unbounded disk growth.
deleteBackuptoworldsManagerIPC and preload bridge, usingdeleteInstallationBackupand removing the backup record from configuration.removeWorldBackupArchive: requires valid UUID format, archive filename match on both prune and delete routes, Worlds directory, and exclusion of Installations subfolders. Vets the deletion path usingassertManagedDeletionPathand removes the verified safe path.ManageInstallationWorlds.tsx, warning when deleting the last remaining copy of a world.makeWorldBackupbased oninstallation.backupsLimit, pruning oldest backups and deleting their archives. WhenbackupsLimitis 0, backup creation is refused withbackups-disabled.backupsLimitis 0.deletedBackupIdsinWorldBackupResultso the UI state stays synchronized when backups are pruned during creation.worldBackupsat 1,000 entries per installation innormalizeInstallationwithinconfigManager.ts.Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run build:unpackpasses.Testing
Ran repository validation suites:
npm run typecheck: Passed (code 0 across node, web, tests).npm run lint:ci: Passed (code 0, 0 errors, 12 warnings).npm run format:check: Passed (code 0).npm run build:unpack: Passed (code 0).tests/ipc/worldsHandlers.test.ts,tests/domain/worlds.test.ts,tests/renderer-dom/manageInstallationWorlds.test.tsx,tests/ipc/configManager.test.ts, andtests/domain/installations/backup.test.ts: Passed (247 passed, 4 skipped).tests/i18n: Passed (31 passed).Specific tests added:
tests/ipc/worldsHandlers.test.ts: Added unit tests verifyingdeleteWorldBackupdeletes archive files on disk, handles already-missing archives, validates inputs and UUID formatting, enforces Worlds folder and filename convention, prevents foreign world archive deletion during pruning, and tests eviction whenbackupsLimitis reached.tests/ipc/configManager.test.ts: Added tests verifyingworldBackupsvalidation and capping at 1,000 entries.tests/renderer-dom/manageInstallationWorlds.test.tsx: Added DOM tests for backup delete button, confirmation modal, cancellation, successful deletion, consequence line, and disabled state while running.Related issues
Fixes #620