diff --git a/CHANGELOG.md b/CHANGELOG.md index 085b94f..c1e7d0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (under its original archive name for imports, or a `_P.pak` name for catalog downloads, where the original name is unrecoverable) and redeployed. +- 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 diff --git a/cmd/lmm/uninstall_test.go b/cmd/lmm/uninstall_test.go index 4214e13..4f9db07 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 87b40ed..a8e2efb 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 bc18fe3..2f5da25 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,9 +431,27 @@ 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 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 { - return fmt.Errorf("listing cached files: %w", err) + 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 74c9894..fb35cd2 100644 --- a/internal/core/installer_test.go +++ b/internal/core/installer_test.go @@ -373,6 +373,69 @@ 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") +} + +// 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. diff --git a/internal/core/merged_pak_test.go b/internal/core/merged_pak_test.go index 3f64ca3..2bc39bf 100644 --- a/internal/core/merged_pak_test.go +++ b/internal/core/merged_pak_test.go @@ -216,6 +216,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.