From ec8dda1cb763662e0398230cce2679d118fc9c9b Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:15:46 -0400 Subject: [PATCH 1/3] fix: tolerate an absent cache entry in Installer.Uninstall (#260) 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 --- CHANGELOG.md | 10 ++++++ cmd/lmm/uninstall_test.go | 15 ++++++--- internal/core/flows_test.go | 22 ++++++++----- internal/core/installer.go | 8 ++++- internal/core/installer_test.go | 29 +++++++++++++++++ internal/core/merged_pak_test.go | 56 ++++++++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd312517..679e5f00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- 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 ### Added diff --git a/cmd/lmm/uninstall_test.go b/cmd/lmm/uninstall_test.go index 4214e13a..4f9db070 100644 --- a/cmd/lmm/uninstall_test.go +++ b/cmd/lmm/uninstall_test.go @@ -104,11 +104,12 @@ func TestUninstallCmd_GameNotFound(t *testing.T) { } // setupDoUninstallTest builds a *core.Service plus a mod that will fail to -// undeploy (its cache directory was never created, so -// Installer.Uninstall's cache.ListFiles call fails deterministically) and -// resets the uninstall command's package-level flag globals to sane -// defaults for calling doUninstall directly. Callers set globals.verbose -// themselves. +// undeploy (a regular file sits at the deploy destination, so the symlink +// linker's Undeploy fails deterministically with "not a symlink" - an absent +// cache entry no longer works as the failure fixture, since #260 made that a +// documented no-op) and resets the uninstall command's package-level flag +// globals to sane defaults for calling doUninstall directly. Callers set +// globals.verbose themselves. func setupDoUninstallTest(t *testing.T) (*core.Service, *domain.Game) { t.Helper() @@ -130,6 +131,10 @@ func setupDoUninstallTest(t *testing.T) (*core.Service, *domain.Game) { UpdatePolicy: domain.UpdateNotify, Enabled: true, })) + require.NoError(t, svc.GetGameCache(game).Store("g1", "src", "1", "1.0", "plugin.esp", []byte("data"))) + // The undeploy obstruction: a foreign regular file where the symlink + // linker expects its own link. + require.NoError(t, os.WriteFile(filepath.Join(gameDir, "plugin.esp"), []byte("not a symlink"), 0644)) pm := svc.NewProfileManager() _, err = pm.Create("g1", "default") require.NoError(t, err) diff --git a/internal/core/flows_test.go b/internal/core/flows_test.go index 87b40ed8..a8e2efbe 100644 --- a/internal/core/flows_test.go +++ b/internal/core/flows_test.go @@ -806,22 +806,26 @@ func TestService_UninstallMod_ProfileDesyncWarnsAndContinues(t *testing.T) { // TestService_UninstallMod_UndeployFailure_RecordedAsNoteWithHistoricalPrefix // guards the exact text (including its historical "Warning: " prefix) of the -// undeploy-failure diagnostic. The mod is never actually cached, so -// Installer.Uninstall's cache.ListFiles call fails deterministically -// (directory does not exist) without relying on filesystem permissions. The -// profile is pre-seeded so profile removal succeeds silently, isolating this -// one diagnostic. +// undeploy-failure diagnostic. A regular file sits where the symlink linker +// expects its own link, so linker.Undeploy fails deterministically ("not a +// symlink") without relying on filesystem permissions. (An absent cache +// entry no longer works as the failure fixture here: since #260 that is a +// documented no-op, not an error.) The profile is pre-seeded so profile +// removal succeeds silently, isolating this one diagnostic. func TestService_UninstallMod_UndeployFailure_RecordedAsNoteWithHistoricalPrefix(t *testing.T) { svc := newFlowsTestService(t) gameDir := t.TempDir() game := &domain.Game{ID: "g1", Name: "Game", ModPath: gameDir, LinkMethod: domain.LinkSymlink} - // No cache files stored (files: nil) - Installer.Uninstall's - // cache.ListFiles call fails because the mod's cache directory was - // never created. - seedInstalledMod(t, svc, game, "src", "1", "1.0", true, nil) + seedInstalledMod(t, svc, game, "src", "1", "1.0", true, map[string][]byte{ + "plugin.esp": []byte("data"), + }) seedProfileWithMod(t, svc, "g1", "default", "src", "1", "1.0") + // A foreign regular file at the deploy destination: Undeploy refuses to + // remove anything that is not its own symlink. + require.NoError(t, os.WriteFile(filepath.Join(gameDir, "plugin.esp"), []byte("not a symlink"), 0644)) + result, err := svc.UninstallMod(context.Background(), game, "default", "src", "1", core.UninstallOptions{}) require.NoError(t, err, "an undeploy failure must not fail the uninstall") require.NotNil(t, result) diff --git a/internal/core/installer.go b/internal/core/installer.go index bc18fe3b..93f8f8f6 100644 --- a/internal/core/installer.go +++ b/internal/core/installer.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io/fs" "path/filepath" "github.com/DonovanMods/linux-mod-manager/internal/domain" @@ -430,8 +431,13 @@ func (i *Installer) Uninstall(ctx context.Context, game *domain.Game, mod *domai // removal must cover anything that might ever have been linked, including // stale unclaimed files a pre-fix deploy linked. Narrowing this would // strand those links forever. + // + // An absent cache entry is "nothing to undeploy", not an error (#260): + // uninstall must stay idempotent when the entry is already gone - the + // steady state syncMergedPak's zero branch and purge --uninstall leave + // behind - while still clearing tracking rows and sweeping empty dirs. 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) } diff --git a/internal/core/installer_test.go b/internal/core/installer_test.go index 74c98949..b7929c55 100644 --- a/internal/core/installer_test.go +++ b/internal/core/installer_test.go @@ -373,6 +373,35 @@ func (f *conditionalFailingLinker) Deploy(src, dst string) error { return f.Linker.Deploy(src, dst) } +// TestInstaller_Uninstall_AbsentCacheEntry_CleansTrackingWithoutError (#260): +// uninstalling a mod whose cache entry no longer exists must treat the empty +// entry as "nothing to undeploy" - succeeding, and still clearing any stale +// DB tracking rows - rather than failing on ListFiles' lstat error. This is +// the steady state syncMergedPak's zero branch lands in after its first +// zero-pass deletes the merged-pak entry. +func TestInstaller_Uninstall_AbsentCacheEntry_CleansTrackingWithoutError(t *testing.T) { + modCache := cache.New(t.TempDir()) + gameDir := t.TempDir() + database, err := db.New(":memory:") + require.NoError(t, err) + defer func() { _ = database.Close() }() + + game := &domain.Game{ID: "g", ModPath: gameDir, LinkMethod: domain.LinkSymlink} + mod := &domain.Mod{ID: "1", SourceID: "src", Version: "1.0", GameID: "g"} + + // A stale tracking row left behind by a deploy whose cache entry has + // since been removed out from under it. + require.NoError(t, database.SaveDeployedFile("g", "default", "a.esp", "src", "1")) + + inst := core.NewInstaller(modCache, linker.New(domain.LinkSymlink), database) + require.NoError(t, inst.Uninstall(context.Background(), game, mod, "default"), + "an absent cache entry means nothing to undeploy, not an error") + + rows, err := database.GetDeployedFilesForMod("g", "default", "src", "1") + require.NoError(t, err) + require.Empty(t, rows, "stale tracking rows must still be cleared when the cache entry is gone") +} + func TestInstaller_Install_DeployFailureRollsBackAndClearsDB(t *testing.T) { // When Deploy fails after some files are deployed, roll back all deployed // files and clear DB records so disk and DB stay consistent. diff --git a/internal/core/merged_pak_test.go b/internal/core/merged_pak_test.go index 7a6da2ab..af154538 100644 --- a/internal/core/merged_pak_test.go +++ b/internal/core/merged_pak_test.go @@ -211,6 +211,62 @@ func TestSyncMergedPak_ZeroEnabledMods_UninstallsExistingPak(t *testing.T) { require.True(t, os.IsNotExist(err), "disabling the last exmodz mod must remove the deployed merged pak") } +// TestSyncMergedPak_ZeroEnabledMods_SecondZeroSyncSucceeds (#260 repro +// shape 1): the first zero-pass uninstalls the pak AND deletes the merged +// cache entry, so "zero sources + no merged entry" is the steady state after +// disabling the last merge source. Every later mutation flow re-runs +// syncMergedPak on that profile - the second zero-pass must succeed, not +// fail on the absent entry. (The test above stops after the first zero-pass +// and so never saw this.) +func TestSyncMergedPak_ZeroEnabledMods_SecondZeroSyncSucceeds(t *testing.T) { + svc, game, _ := newMergedPakTestGame(t) + seedEnabledExmodzMod(t, svc, game, "fake-compiler", "bear-mount", "1.0", "exmodz-file", []byte("bear-bytes")) + + _, err := svc.SyncMergedPak(context.Background(), game, "default") + require.NoError(t, err) + + require.NoError(t, svc.SetModEnabled("fake-compiler", "bear-mount", game.ID, "default", false)) + + _, err = svc.SyncMergedPak(context.Background(), game, "default") + require.NoError(t, err, "first zero-pass: undeploys the pak and deletes the merged entry") + + warnings, err := svc.SyncMergedPak(context.Background(), game, "default") + require.NoError(t, err, "second zero-pass must tolerate the already-absent merged entry") + require.Empty(t, warnings) +} + +// TestSyncMergedPak_NeverMerged_ZeroSources (#260 repro shape 2): a fresh +// DeployCompile game whose first synced profile has zero merge sources (e.g. +// the first install is a plain non-pak file) has never had a merged-pak +// cache entry at all - the very first sync must succeed. +func TestSyncMergedPak_NeverMerged_ZeroSources(t *testing.T) { + svc, game, _ := newMergedPakTestGame(t) + + warnings, err := svc.SyncMergedPak(context.Background(), game, "default") + require.NoError(t, err, "a profile that never merged has no entry to remove - not an error") + require.Empty(t, warnings) +} + +// TestPurgeMergedPak_AbsentCacheEntry (#260 repro shape 3): PurgeMergedPak +// routes through the same Installer.Uninstall and must likewise tolerate an +// absent merged-pak cache entry (e.g. purge --uninstall already deleted it, +// or the profile never merged). +func TestPurgeMergedPak_AbsentCacheEntry(t *testing.T) { + svc, game, _ := newMergedPakTestGame(t) + + require.NoError(t, svc.PurgeMergedPak(context.Background(), game, "default", true), + "purging a never-merged profile must be a no-op, not an error") + + // And again after a full merge/purge cycle: --uninstall deletes the + // entry, so a repeat purge sees the same absent-entry state. + seedEnabledExmodzMod(t, svc, game, "fake-compiler", "bear-mount", "1.0", "exmodz-file", []byte("bear-bytes")) + _, err := svc.SyncMergedPak(context.Background(), game, "default") + require.NoError(t, err) + require.NoError(t, svc.PurgeMergedPak(context.Background(), game, "default", true)) + require.NoError(t, svc.PurgeMergedPak(context.Background(), game, "default", true), + "a repeat purge after --uninstall must tolerate the already-deleted entry") +} + // TestSyncMergedPak_RegeneratesOnBaseHashChange proves a base-pak refresh // (the "Friday problem", generalized from #196 to the merged model) still // triggers regeneration. From 0f715d499380c08fa727aac91f185e0ca16986ba Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 14:22:46 -0400 Subject: [PATCH 2/3] fix: fall back to DB-tracked paths when uninstalling an absent cache 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 --- internal/core/installer.go | 25 ++++++++++++++++++------ internal/core/installer_test.go | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/internal/core/installer.go b/internal/core/installer.go index 93f8f8f6..2f5da251 100644 --- a/internal/core/installer.go +++ b/internal/core/installer.go @@ -432,13 +432,26 @@ func (i *Installer) Uninstall(ctx context.Context, game *domain.Game, mod *domai // stale unclaimed files a pre-fix deploy linked. Narrowing this would // strand those links forever. // - // An absent cache entry is "nothing to undeploy", not an error (#260): - // uninstall must stay idempotent when the entry is already gone - the - // steady state syncMergedPak's zero branch and purge --uninstall leave - // behind - while still clearing tracking rows and sweeping empty dirs. + // An absent cache entry is not an error (#260): uninstall must stay + // idempotent when the entry is already gone - the steady state + // syncMergedPak's zero branch and purge --uninstall leave behind. The + // deployment can still be fully on disk, though (a copy/hardlink deploy + // owns real files, not links back into the cache), so fall back to the + // DB's tracked deployed paths rather than orphaning them while erasing + // the only record that they were ours. Ownership rows upsert on + // overwrite ("new mod takes ownership"), so the fallback never removes + // a path another mod has since claimed. files, err := i.cache.ListFiles(game.ID, mod.SourceID, mod.ID, mod.Version) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("listing cached files: %w", err) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("listing cached files: %w", err) + } + files = nil + if i.db != nil { + if files, err = i.db.GetDeployedFilesForMod(game.ID, profileName, mod.SourceID, mod.ID); err != nil { + return fmt.Errorf("listing tracked deployed files: %w", err) + } + } } // Undeploy each file diff --git a/internal/core/installer_test.go b/internal/core/installer_test.go index b7929c55..fb35cd20 100644 --- a/internal/core/installer_test.go +++ b/internal/core/installer_test.go @@ -402,6 +402,40 @@ func TestInstaller_Uninstall_AbsentCacheEntry_CleansTrackingWithoutError(t *test require.Empty(t, rows, "stale tracking rows must still be cleared when the cache entry is gone") } +// TestInstaller_Uninstall_AbsentCacheEntry_RemovesTrackedDeployedFiles +// (#260 review follow-up): with the cache entry gone, the deployment can +// still be fully on disk - a copy/hardlink deploy owns real files, not +// links back into the cache. Uninstall must fall back to the DB's tracked +// deployed paths and remove them, not silently orphan them while erasing +// the only record that they were ours. +func TestInstaller_Uninstall_AbsentCacheEntry_RemovesTrackedDeployedFiles(t *testing.T) { + modCache := cache.New(t.TempDir()) + gameDir := t.TempDir() + database, err := db.New(":memory:") + require.NoError(t, err) + defer func() { _ = database.Close() }() + + game := &domain.Game{ID: "g", ModPath: gameDir, LinkMethod: domain.LinkCopy} + mod := &domain.Mod{ID: "1", SourceID: "src", Version: "1.0", GameID: "g"} + + // A copy-linked deployment whose cache entry has since been removed: + // the deployed file is real and still present. + deployedPath := filepath.Join(gameDir, "data", "a.esp") + require.NoError(t, os.MkdirAll(filepath.Dir(deployedPath), 0755)) + require.NoError(t, os.WriteFile(deployedPath, []byte("copied"), 0644)) + require.NoError(t, database.SaveDeployedFile("g", "default", "data/a.esp", "src", "1")) + + inst := core.NewInstaller(modCache, linker.New(domain.LinkCopy), database) + require.NoError(t, inst.Uninstall(context.Background(), game, mod, "default")) + + _, err = os.Stat(deployedPath) + require.True(t, os.IsNotExist(err), "the tracked deployed file must be removed via the DB fallback") + + rows, err := database.GetDeployedFilesForMod("g", "default", "src", "1") + require.NoError(t, err) + require.Empty(t, rows) +} + func TestInstaller_Install_DeployFailureRollsBackAndClearsDB(t *testing.T) { // When Deploy fails after some files are deployed, roll back all deployed // files and clear DB records so disk and DB stay consistent. From d4e5029c377876169909bd65a4c277a5f0cd7b2a Mon Sep 17 00:00:00 2001 From: "Donovan C. Young" Date: Sat, 8 Aug 2026 15:55:54 -0400 Subject: [PATCH 3/3] docs: restore blank line before v1.30.0 header lost in merge resolution Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0662db9f..696a4849 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 ### Added