Skip to content

fix(cache): keep a converted pak's raw-fallback copy through prune; heal pruned entries (#250) - #261

Merged
dyoung522 merged 2 commits into
developfrom
dyoung522/fix-250-prune-converted-pak
Aug 8, 2026
Merged

fix(cache): keep a converted pak's raw-fallback copy through prune; heal pruned entries (#250)#261
dyoung522 merged 2 commits into
developfrom
dyoung522/fix-250-prune-converted-pak

Conversation

@dyoung522

Copy link
Copy Markdown
Collaborator

Fixes #250.

The bug

cache.PruneUnclaimed conflated two causes of "unclaimed": stale/superseded content — #210's actual target — and a converted pak's deployable copy, which is unclaimed only because the merged pak claims its content (members=nil, reconcilePakManifests' participating branch) while it remains the designated raw-fallback artifact. Any sibling-file ingest into the same version directory opened the prune gate (commitStagedCacheWithMarkerPruneUnclaimed(stagePath); staging is seeded from the existing entry, so every marker reads Recorded and the retained source is present) and deleted the copy. If the user later opted out of conversion, or the next merge failed for that mod, the raw-fallback branch found no member to claim, recorded an empty member set, and deployed nothing — silently, with the exact bytes still sitting in .lmm-source-<fileID>.

The fix — both directions from the issue

Prevent (internal/storage/cache): PruneUnclaimed now exempts any unclaimed file whose content matches a retained source (size compared first, MD5 only on a size match, each side hashed at most once). This is the same content-identity attribution rawPakMembers uses (#241) — the one identity that survives the convert flip erasing the ingest-time manifest. Genuinely stale files (including same-size/different-bytes ones) are still pruned; #210's behavior is otherwise unchanged.

Heal (internal/core): the raw-fallback branch of reconcilePakManifests restores the deployable copy from the fileID's retained source when no cache member matches it, then claims and deploys it — so entries already damaged by released versions converge on the next sync instead of staying silently broken forever. Restore naming:

  • import-path fileIDs restore name-exact (the fileID is the archive filename, and ingest names the deployable copy identically);
  • download-path fileIDs (the literal icarus "pak") get a deterministic <mod-id>_P.pak name — the original URL-derived name is durably recorded nowhere (the convert flip erased the manifest that carried it, and the DB stores only fileIDs), which I verified across domain/DB/fingerprint before settling on synthesis.

The restore refuses to overwrite an existing same-named file (it necessarily holds different content and could be a sibling's claimed member) — failing loudly beats corrupting it, and the next reconcile pass retries.

Tests (written first, run failing before any implementation)

  • TestPruneUnclaimed_KeepsUnclaimedFileMatchingRetainedSource — unit-level prevent: the suppressed copy survives, a same-size/different-content stale file is still pruned (content, not size, decides).
  • TestService_DownloadMod_SiblingReingestKeepsConvertedPakCopy — the issue's repro shape end-to-end through DownloadMod: pak ingest → convert flip → sibling exmodz ingest → copy survives.
  • TestReconcilePakManifests_RawFallback_RestoresPrunedDeployableCopy — table-driven heal over both fileID shapes, asserting manifest claim, restored cache bytes, actual deploy, and second-pass convergence.
  • TestSyncMergedPak_OptOutAfterPrunedConvertedPak_RestoresRawDeploy — the already-damaged case through the full SyncMergedPak opt-out flow (merged entry seeded present, as in any real damaged state).

Adjacent bug found and filed, not fixed here

Writing the sync-level test surfaced a pre-existing, unrelated bug: syncMergedPak's uninstall-to-zero branch hard-errors whenever the merged-pak cache entry is absent — which is the steady state after its own first zero-sources pass deletes the entry. Filed as #260 (in the zero branch since #197/v1.28.0; reproduced with a one-line probe). It interacts with #250's opt-out leg (sync 1 heals fine; later syncs on a fully opted-out profile error loudly) but is a separate defect with its own fix-direction trade-offs, so it stays out of this PR.

Verification

go build ./... && go vet ./... && gofmt -l . && go test ./... — all green, gofmt -l empty, trunk check on changed files: no new issues. CHANGELOG entry added under [Unreleased]; no version bump per repo convention.

🤖 Generated with Claude Code

…eal pruned entries (#250)

PruneUnclaimed conflated two causes of "unclaimed": stale/superseded
content (#210's target) and a converted pak's deployable copy, which is
unclaimed only because the merged pak claims its content (members=nil)
while it remains the designated raw-fallback artifact. Any sibling-file
ingest into the same version directory opened the prune gate and deleted
the copy; a later conversion opt-out or merge failure then marked an
empty member set and deployed nothing, silently, with the bytes still
sitting in the retained source.

Two-part fix, matching the issue's two directions:

- Prevent: PruneUnclaimed exempts any unclaimed file whose content
  matches a retained source (size-then-MD5, the same content-identity
  attribution rawPakMembers uses in #241 - the one identity that
  survives the convert flip erasing the ingest-time manifest).
- Heal: reconcilePakManifests' raw-fallback branch restores the
  deployable copy from the retained source when no cache member matches
  it, then claims and deploys it - entries already damaged by released
  versions converge instead of staying silently broken. Import-path
  fileIDs restore name-exact (the fileID IS the archive filename);
  download-path fileIDs get a deterministic <mod-id>_P.pak name, since
  the original URL-derived name is durably recorded nowhere.

The sync-level heal test seeds the merged entry as present (the only
realistic damaged state - a merge must have succeeded for the manifest
to flip); an absent entry trips the pre-existing, unrelated #260.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 8, 2026 18:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a cache-pruning regression affecting converted .pak mods by ensuring a converted pak’s raw-fallback deployable copy is not deleted during cache.PruneUnclaimed, and by adding self-healing in the raw-fallback reconcile path to restore previously-pruned deployable copies from retained sources.

Changes:

  • Update cache.PruneUnclaimed to exempt unclaimed files whose bytes match any retained source (size check first; MD5 only on size match, hashed at most once per side).
  • Add a heal path in reconcilePakManifests raw-fallback to restore a missing deployable pak copy from the retained source and claim/deploy it.
  • Add unit + integration tests covering both “prevent” (prune exemption) and “heal” (restore + converge) behaviors, plus a CHANGELOG entry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/storage/cache/cache.go Adds retained-source content matching to prevent pruning suppressed raw-fallback copies.
internal/storage/cache/cache_test.go Adds a focused unit test ensuring same-size/different-bytes debris is still pruned while matching-bytes copy is retained.
internal/core/service_test.go Adds an end-to-end DownloadMod repro test ensuring sibling ingest doesn’t delete a converted pak’s deployable fallback copy.
internal/core/merged_pak.go Adds restore-and-claim logic when raw fallback has no matching cache member (heals already-pruned entries).
internal/core/merged_pak_test.go Adds table-driven heal tests for both fileID shapes and a full SyncMergedPak opt-out recovery test.
CHANGELOG.md Documents the #250 fix and healing behavior under [Unreleased].

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…prune-converted-pak

# Conflicts:
#	CHANGELOG.md
Copilot AI review requested due to automatic review settings August 8, 2026 19:21
@dyoung522
dyoung522 merged commit 91a55c3 into develop Aug 8, 2026
2 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

dyoung522 added a commit that referenced this pull request Aug 8, 2026
…colliding files named in the rejection (#256)

Item 1: #261's rawPakRestoreName held the last live format knowledge in
core - a '.pak' extension test and the synthesized '<mod-id>_P.pak'
Icarus override name. The extension test was already expressed by the
seam (IsConvertibleArtifact: EqualFold-on-Ext and suffix-on-lowered are
equivalent for every filename), so it reuses that; the synthesized name
is genuinely new vocabulary and becomes the tenth method,
RestoredArtifactName(modID) - the per-mod analogue of
MergedArtifactName, byte-identical output ('<modID>_P.pak', core Base's
the input) so healed installs keep their on-disk names. The
import-vs-download provenance split STAYS in core: it follows from how
ingest keys fileIDs, which is uniform across games - only the two format
questions inside it moved. #261's heal/prune tests pass unmodified.

Item 2: the mixed-selection rejection now names the actual colliding
files ('Mod_P.pak and Mod.exmodz are alternate forms of the same mod -
select one') - format-agnostic because the vocabulary comes from the
selection itself, and strictly more useful than both prior wordings.
The trigger is unchanged (booleans, not name-emptiness, so a file with
an empty FileName still trips it). Variant-exclusivity expectations
updated deliberately; cmd/lmm/install_test.go's local fake updated to
mirror the production message shape it stands in for.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dyoung522
dyoung522 deleted the dyoung522/fix-250-prune-converted-pak branch August 8, 2026 23:01
@dyoung522 dyoung522 mentioned this pull request Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants