fix: tolerate an absent cache entry in Installer.Uninstall (#260) - #262
Merged
dyoung522 merged 5 commits intoAug 8, 2026
Merged
Conversation
syncMergedPak's uninstall-to-zero branch and PurgeMergedPak hard-errored whenever the merged-pak cache entry was absent, because Installer.Uninstall started with cache.ListFiles, which fails on a missing version directory. Since the zero branch deletes that entry on its first successful pass, "zero merge sources + no merged entry" is the steady state after disabling the last merge source - every later mutation flow on that profile errored forever. Treat a missing cache entry as "nothing to undeploy" (ListFiles ENOENT -> empty), keeping the DB tracking cleanup and empty-dir sweep. This makes Uninstall honor the idempotency merged_pak.go's docs already claimed it had, and fixes PurgeMergedPak for free. Structural obstructions (e.g. a regular file blocking the cache path, ENOTDIR) still error. The two tests that used an absent cache entry as their deterministic undeploy-failure fixture now obstruct linker.Undeploy directly instead (a foreign regular file where the symlink linker expects its own link). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes #260 by making Installer.Uninstall tolerate a missing cache entry (treating ListFiles ENOENT as “nothing to undeploy”), which unblocks steady-state DeployCompile profiles where the merged-pak cache entry is intentionally absent after uninstall-to-zero flows.
Changes:
- Update
Installer.Uninstallto ignorefs.ErrNotExistfromcache.ListFiles. - Add regression tests covering repeated zero-source sync, never-merged zero-source sync, and purge behavior when the merged-pak cache entry is absent.
- Update existing uninstall-failure fixtures to use a deterministic “not a symlink” obstruction instead of relying on an absent cache directory.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/core/installer.go | Treat missing cache entry as non-fatal during uninstall. |
| internal/core/installer_test.go | Add unit test ensuring uninstall with absent cache entry clears tracking without error. |
| internal/core/merged_pak_test.go | Add regression tests covering the three reported repro shapes. |
| internal/core/flows_test.go | Adjust uninstall warning fixture to use a “not a symlink” undeploy obstruction. |
| cmd/lmm/uninstall_test.go | Adjust CLI uninstall test fixture similarly to force deterministic undeploy failure. |
| CHANGELOG.md | Document behavior change under [Unreleased] for #260. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
439
to
442
| files, err := i.cache.ListFiles(game.ID, mod.SourceID, mod.ID, mod.Version) | ||
| if err != nil { | ||
| if err != nil && !errors.Is(err, fs.ErrNotExist) { | ||
| return fmt.Errorf("listing cached files: %w", err) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Valid — implemented in 0f715d4. Uninstall now falls back to GetDeployedFilesForMod for the undeploy set when ListFiles reports the entry absent, so a copy/hardlink deployment that outlived its cache entry is removed before its tracking rows are cleared. Ownership rows upsert on overwrite ("new mod takes ownership"), so the fallback can only remove paths this mod still owns; the merged-pak steady state deleted its rows on the first zero pass, so the #260 shapes remain clean no-ops. Covered by TestInstaller_Uninstall_AbsentCacheEntry_RemovesTrackedDeployedFiles (written first, watched fail on the orphaned file).
…entry Copilot review follow-up on #262: with the cache entry gone the deployment can still be fully on disk (copy/hardlink deploys own real files, not links into the cache). Skipping the undeploy loop while still deleting the deployed_files rows would orphan those files and erase the only record they were ours. Fall back to GetDeployedFilesForMod for the undeploy set instead; ownership rows upsert on overwrite, so the fallback never removes a path another mod has since claimed. The merged-pak steady state deleted its rows on the first zero pass, so the #260 shapes remain clean no-ops. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uninstall-absent-cache-entry # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CHANGELOG.md:38
- Markdown formatting: there should be a blank line between the final bullet in the list and the next version header. Without it, some renderers can treat the
## [1.30.0]heading as part of the list, and it also breaks the established spacing pattern used elsewhere in this CHANGELOG.
- Uninstalling a mod whose cache entry is absent is now a no-op removal
(still clearing tracking rows and sweeping empty directories) instead of
an error. In particular, a DeployCompile profile with zero merge sources
and no merged-pak cache entry — the steady state after disabling the
last exmodz/pak mod, or a profile that never merged — no longer fails
every subsequent sync/deploy/purge with a loud
`removing merged pak: ... no such file or directory` error (#260).
## [1.30.0] - 2026-08-08
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
Fixes #260.
syncMergedPak's uninstall-to-zero branch andPurgeMergedPakhard-errored whenever the merged-pak cache entry was absent: both callInstaller.Uninstallunconditionally, andUninstallstarted withcache.ListFiles, which fails withlstat ...: no such file or directoryon a missing version directory. Since the zero branch deletes that entry on its first successful pass, zero merge sources + no merged entry is the steady state after disabling the last merge source — every later mutation flow (lmm deploy, another disable, any install) on that profile errored loudly and forever. Pre-existing since #197 (v1.28.0).The fix
Treat a missing cache entry as "nothing to undeploy" in
Installer.Uninstall(ListFilesENOENT → empty set), keeping the DB tracking cleanup and empty-dir sweep. This is direction 1 from the issue:Uninstallactually honor the idempotencymerged_pak.go's own doc comments already claimed it had ("is idempotent when there is nothing deployed").PurgeMergedPakfor free — no per-call-site stat guards.internal/core/installer.goonly;merged_pak.gois untouched.ENOTDIR, notENOENT, and keeps its diagnostic (the existing obstruction tests confirm this).Caller audit: no production caller depends on the old error. Every real-mod call site treats an
Uninstallerror as a non-fatalWarning:note and continues; only the merged-pak zero branch andPurgeMergedPakmade it fatal — which was exactly the bug. The manually-gutted-cache-entry uninstall now succeeds and clears its stale tracking rows instead of warning and stranding them.Tests (written first, watched fail on the exact issue error)
TestInstaller_Uninstall_AbsentCacheEntry_CleansTrackingWithoutError— unit level: no error, stale DB rows cleared.TestSyncMergedPak_ZeroEnabledMods_SecondZeroSyncSucceeds— repro shape 1 (second zero-pass, the common one).TestSyncMergedPak_NeverMerged_ZeroSources— repro shape 2 (first-ever sync with zero sources).TestPurgeMergedPak_AbsentCacheEntry— repro shape 3 (never-merged purge + repeat purge after--uninstall).TestSyncMergedPak_ZeroEnabledMods_UninstallsExistingPakkept as-is (still passes); the new second-zero-sync test complements it per the issue's observation that it stopped one pass too early.Two existing tests used an absent cache entry as their deterministic undeploy-failure fixture (their comments say so; they guard diagnostic text, not absent-entry semantics). Re-pointed them at a still-valid failure: a foreign regular file where the symlink linker expects its own link (
not a symlink), equally deterministic and permission-free.Verification
go build ./... && go vet ./... && gofmt -l . && go test ./...— all pass,gofmt -lempty,trunk checkclean on changed files. No version bump (story PR); CHANGELOG entry under[Unreleased].🤖 Generated with Claude Code