Repository navigation
fix: data-integrity and responsiveness fixes from the 1.18.2 audit - #72
Merged
Merged
Conversation
open_for_write copied a lower file straight onto its final Overwrite path, so a copy that failed part-way (ENOSPC) or was killed left a truncated file there. Later calls saw it existed and never copied again, so it shadowed the intact lower file for good, and an index rebuild during a copy could serve the half-written file to readers. The copy now goes to a hidden sibling temp (.mohidden), gets its mode and metadata, then is renamed into place; the temp is removed on error. Also satisfies clippy unnecessary_unwrap in a test.
… list Removing mods deleted their folders but left their lines in modlist.txt, so the trust check counted them as lost: a batch past the "unmounted drive" threshold made every later save, install registration and launch refuse, and other profiles listing those mods were wedged the same way. Remove now drops exactly the deleted names from every profile's modlist.txt before saving, and no longer hides a refused save behind "Removed N mod(s)".
Reconciliation pushed folders missing from modlist.txt after every listed row in highest-first file order, so after the display reverse they became the LOWEST priority: once enabled, every other mod overrode them, and a mod created by capture_overwrite_into_mod (which only enables that entry in place) lost every conflict. Splice them in at the front instead, as the comment and MO2 intend; they stay disabled.
…ncomplete pack The walk records symlinks, unreadable folders and names 7-Zip cannot take in plan.left, but the pack report never carried them, so a mods/ or downloads/ symlinked to another drive was dropped whole while the GUI said "Packed" and `eidos pack` exited 0. Transfer::pack now adds one warning listing those losses (capped at 20), which both front ends already treat as NOT complete. Deliberate exclusions (prefix, logs, caches) stay out of it.
section_header used str::trim, which keeps U+FEFF, so a game INI saved as "UTF-8 with BOM" lost its first section: set_key appended a duplicate [General]/[Archive] at the end that Wine never reads (root mode, OMOD EditINI, INI tweaks, BSA invalidation). section_header now skips a leading BOM. plugin_state_dir reads bUseMyGamesDirectory through eidos_ini::get_key (first value wins, like the engine) and archive_plan uses section_header.
…ing it The collection driver seeded and read plugin state from AppData, and the GUI Plugins tab wrote without seeding at all. For Morrowind and root-mode Oblivion the state lives in the install root, so a fresh profile was founded on an all-enabled default, and a Morrowind write built a [Game Files]-only stub INI that every later launch deployed. Both now seed from plugin_state_dir like launch and sort; the collection shadow write skips Timestamp games so it never lands in the install, and CLI/collection FOMOD contexts use the same fallback.
…nged The window wrote its cached mod list and plugin order wholesale, read minutes earlier. A CLI install, collection or sort in between was erased by the next click (installed mods came back disabled, the order reverted), and the plugin snapshot was refreshed to the revert. The window now keeps a content hash of modlist.txt and of the profile's plugin state, checks it under the lock before writing, and reloads on mismatch instead.
… saves The cloud sync checked only the current profile's manifest, so two profiles alternating on a fixed save name imported each other's copy as a new orphan group on every sync. Copies any sibling profile pushed now count as known. Rescued orphans keep their source mtime instead of sorting above the live save, and launch warns about prefix saves made after seeding (another device, a launch without Eidos) that the profile bind would otherwise hide silently.
…s folders The ownership marker and the state file are per revision, so installing revision N+1 of an installed collection started from nothing: every member collided with its own previous copy, was extracted again as "Name (2)", and the old copies, including members the update dropped, stayed enabled. Before reserving anything, both the CLI and the window now hand each folder still carrying another revision's exact marker to the matching member (same key, or the only member of the same Nexus page when its file was updated), moving its receipt with it, and report folders no revision member uses.
Each toggle or reorder reruns diagnostics on the UI thread, which built two throwaway indexed LayerStacks (plugin visibility and the SKSE scan), each a full recursive walk of every enabled mod. Unmanaged DLC rows (FILE paths) were also passed as layers, so the walk failed after finishing and every plugin fell back to a per-layer scan. Add LayerStack::new_unindexed for one-shot stacks, answer plugin visibility from one merged root listing, and drop unmanaged rows.
A filter walks the whole merged tree, and each entry's provider was found by a starts_with scan over every enabled mod: O(entries x mods), about 4.5 s of frozen UI at 347 mods, repeated after every mod toggle. Every redraw then deep-cloned each cached directory listing again. The layer roots now sit in a map built once per view generation beside the LayerStack, looked up by walking the path's ancestors (same longest-prefix rule), and listings are shared through an Rc; non-matching files are skipped before they are cloned.
Removing the last mods deleted modlist.txt, but the window saves right after, and save_modlist wrote an empty file back. An empty list next to the next installed folder reads as truncated, so that install, every later save and the launch were refused until the file was deleted by hand. save_modlist now removes the file (and the legacy flat one it shadowed) when there are no rows, so "no order yet" is always an absent file.
… mod folder Rename, Overwrite-into-mod, Add separator and New mod change mods/ first and save after. When another process had rewritten modlist.txt, the new stale check refused that save only after the folder moved, and the reload appended the renamed or new mod DISABLED at the top while the status claimed success. The check now runs before the folder changes, and the success status is set before the save so a refusal is no longer hidden.
….ini For Morrowind the plugin state hash covers the profile's Morrowind.ini, which the window's own INI editor writes. The save left the cached load order and its hash stale, so the next plugin click or LOOT sort was refused as "another Eidos process". Saving a file inside the profile's plugin state directory now invalidates the plugin list, which re-reads both.
After a newer revision took over a collection's folders, re-running the older revision skipped every key it already recorded, so each member failed verification and every folder was listed as one it does not use. A key recorded in the same folder the other revision holds is now transferred back, and a folder this revision records is never reported as unused.
…sion
Adoption seeded only the folder, so an unchanged member started Pending and
was recovered as Approximate ("interrupted status save") unless it had clone
hashes: nearly every revision update read as not faithful, on every run. A
key-matched member now takes the other revision's Installed or Approximate
status with its folder, still verified before it is trusted, and is
registered in the active profile as the recovery path did.
Adoption only paired member keys, so the "<name> - INI Tweaks" folder was never handed over: the new revision installed it again as "(2)" and the old one was reported as unused. When the new revision still ships INI Tweaks, its aux:ini-tweaks key is now paired like a member's, so apply_ini_tweaks reuses the same folder.
… can go ahead Both callers adopted the previous revision's folders before install::run, so a revision stopped by the recipe check or a game runtime mismatch had already rewritten every marker without saving any state: the revision the user stayed on could no longer verify its members. Adoption is now a Hooks step run after both gates, and it saves the claimed folders before any marker moves.
…ew revision --no-optional marks optional members Skipped before the install starts, and adoption then handed them their previous folder anyway: owned by the new revision, still enabled, left out of the ordering pass and of the leftover list, and reported only as skipped. A Skipped member is no longer adopted, so its old folder keeps its marker and is named as unused.
mods/ is shared by every profile, so adopting a folder another profile enables replaced an updated member's files under that profile, which then loaded the new revision's files with its old order and settings. Adoption now skips any folder a profile other than the active one enables, the new revision installs its own copy as before, and the leftover note says why.
…d it The leftover note tells the user to disable or remove a folder the new revision does not use, and it keeps the install short of faithful, but it only checked the old marker: disabling the mod changed nothing, so the note and the window's error came back on every run. A leftover is now named only while the active profile still enables it.
…ke a folder back Going back to an older revision kept its claim on a folder the newer one had taken over even when the take-back was refused (another profile enables it, or it holds installer answers). The folder carries the newer marker, so the member failed verification on every run and never installed again. The claim is now dropped and the member reset to Pending, so it installs a fresh copy beside the kept folder. The leftover note no longer tells the user to remove a folder another profile enables (removing it strips it from that profile too); it names that profile and asks only to disable it here. A member skipped with --no-optional, or kept for its installer answers, is described as such instead of as one this revision does not use. Adoption now registers every member whose status is Installed or Approximate in its folder on each pass, so a run stopped part-way no longer leaves seeded members verified but disabled in the profile. An unreadable record of another revision is skipped with a note instead of aborting the install with advice meant for the current revision.
…vision A revision that took over the previous revision's INI Tweaks folder only copied its own fragments in, so fragments the author dropped stayed on disk and stayed selected in meta.ini, and were merged into the deployed INIs at every launch. Fragment files the revision does not ship are now removed from the owned folder and dropped from the selection. The folder is also no longer paired when the new revision's INI Tweaks directory holds no fragment files: nothing was installed then, and the old fragments stayed owned by the new revision with no note.
Renaming a mod renamed the shared folder but only the active profile's modlist.txt, so every other profile lost the mod's state and order and enough renames got it judged an unmounted drive. Instance::rename_mod now renames the line in every profile under the lock, as forget_mods does. Enable/disable selected, Enable all, Send to priority, Fix save mods and an aimed install set their success text after mods_changed and hid its refusal. mods_changed now says whether the save landed and they only report success then. Add separator and New mod check staleness under the lock and open the rename editor only after a saved list; Remove and batch Remove check staleness before deleting, so the refusal never covers a delete that already happened. The INI editor opened an unseeded Morrowind.ini empty and saving it founded a stub that every launch deployed. It now seeds from the game's real state first, and a save refuses to write over a file another process changed since it was opened.
The warning naming prefix saves the profile bind hides repeated on every launch until the files were moved, burying itself in the log. The set it last reported is now recorded beside .seeded in the profile's saves, and the warning only comes again when that set changes; an empty set clears the record so the same names turning up later are reported again.
A downloads/ symlinked to another drive is often deliberate, yet every pack reported it as missing and exited 1 with no way out given, so a `pack && ...` script failed on every run. The warning now points at --no-downloads (or the window's Include downloads/ box), which makes it a deliberate exclusion that does not count as a loss.
The guide did not say that unlisted folders now appear disabled at the highest priority, that Remove and Rename apply to every profile, that an emptied list leaves no modlist.txt, or that the window refuses and reloads an edit when another Eidos process changed the list. It also said nothing about per-profile saves and the hidden-saves warning, how collection leftovers and --no-optional skips are reported, what a failed takeover says, or how to pack without a linked downloads/.
Mirroring the INI Tweaks folder removed every file this revision does not ship, on every run, so a fragment the user added there (by hand or with MO2's Create Tweak) was deleted and dropped from the selection without a word. meta.ini now records the fragments a collection run installed (eidosCollectionIniFragments); only those are removed when a later revision drops them. A folder written before that record only has unshipped fragments deselected, and the report names what was removed or deselected.
The usage guide gained sections on the mod list, profile saves and collection revision takeover, and reworded the incomplete-backup notes. Apply the same changes to all 15 translations and restamp them with scripts/i18n-check.sh --fix.
…ing it A new revision takes over the previous revision's folders and replaces an updated member, or one that fails its receipt check, in place. The replace used ReplaceOwned, which drops the old folder, so files the user had added or edited inside it (Sync to Mods output, an edited INI) were deleted with no backup. On main a new revision installed "Name (2)" beside the old folder, so this was a regression for revision updates. Before replacing, compare the folder with its install receipt. When it holds files the receipt does not vouch for, or has content and no readable receipt, publish with ReplaceOwnedWithBackup so the old folder is kept as "<name>_backup", which is never deployed. An untouched folder or a fresh reservation is still replaced outright, so an update does not double the disk use. The kept folders are listed in a new report section in both the CLI and the window. A batch enable and the Saves tab's "enable the mods that provide it" could tick a backup folder and write "+X_backup" into modlist.txt; both now skip backups, as the single-row toggle already did.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the data-integrity and responsiveness defects confirmed by the audit of 1.18.2 (051277b). Each commit was reviewed adversarially (correctness and regression), then the whole branch was reviewed again for interactions between fixes and for user-visible behaviour.
What is fixed
Data integrity
eidos playkilled by Steam's Stop) left a truncated file that shadowed the intact mod or game file in every later session. Copy-up now goes through a hidden temp file and an atomic rename.modlist.txtfirst. Rename keeps every profile in step the same way.eidos sort, a collection run, a Steam session or a second window. It now refuses, reloads and asks to redo the change.Morrowind.inithat replaced the real game INI at every launch. The profile is now seeded from the game's real state before the first write.<name>_backupbefore it is replaced, and the report lists it.orphan-*copies on every switch, and rescued saves keep their original date.eidos packexits 1.Responsiveness
Behaviour changes users will notice
modlist.txtinstead of saving it empty.modlist.txtshow at the bottom of the list (highest priority), still disabled.<name>_backup(never deployed) and listed in the report.eidosCollectionIniFragments.saves/gets a.hidden-saves-warnedfile, so the hidden prefix saves warning is logged once per change.--no-downloadswhendownloads/is a link to another drive._backupfolder.eidos packexits 1 when content was left out of the backup.These are documented in
docs/guide/usage.md.Checks
Every data-integrity fix carries a regression test that fails without it. The two responsiveness fixes are covered for correctness only; no test measures their speed.
cargo +1.94.1 clippy --workspace --locked -- -D warnings: clean.cargo clippy --all-targetswarnings: 24 (26 on main).nif_preview::tests::real_nif_helper_parses_both_native_fixtures_and_rejects_invalid_input, which already fails on main.Not in this PR