Skip to content

perf: release the mod icon cache after leaving the page - #177

Merged
Zaldaryon merged 2 commits into
devfrom
perf/issue-170-release-mod-icon-cache
Aug 21, 2026
Merged

perf: release the mod icon cache after leaving the page#177
Zaldaryon merged 2 commits into
devfrom
perf/issue-170-release-mod-icon-cache

Conversation

@Zaldaryon

Copy link
Copy Markdown
Collaborator

Summary

Release decoded mod icon bytes when leaving the installed-Mods page, with a main-process fallback when all windows close. Protect the bounded cache from oversized entries and from repopulation by reads that were still in flight when the cache was cleared.

Type

  • Bug fix
  • Feature
  • Performance
  • Refactor or cleanup
  • Tests only
  • Docs or build

Checklist

Testing

  • Added cache tests for oversized entries, oversized replacement, and clear-during-read generation handling.
  • Added trusted and untrusted clear-request lifecycle tests.
  • Added a renderer test that verifies the cache is cleared on ManageMods unmount.
  • Full coverage suite: 109 test files, 1236 tests passed, 2 skipped.
  • Local build: npm run build:unpack passed.

Related issues

Fixes #170

Dependency

Depends on #176. This PR intentionally uses test/issue-171-cachemodimg-protocol as its base so the review contains only the #170 changes. After #176 merges, retarget this PR to dev and keep it ready for review.

@Zaldaryon
Zaldaryon requested a review from Pixnop August 21, 2026 13:58

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The mechanism is better than the issue asked for, and the strongest part deserves naming: the clearGeneration guard closes the real race (a read that began before the release repopulating the just-emptied cache), the generation is sampled with no intervening await so the check is atomic, and the disk-layer interaction is safe by construction rather than by luck, since the lstat runs before the memory lookup and names are content-addressed, a pruned icon can 404 but never serve stale. The trusted-sender guard follows the repo's ipcMain.on convention and the untrusted case is tested. Nine mutations were run against this; five killed cleanly.

Two changes before merge, both from surviving mutations.

First, the positive re-entry path is unpinned: a cache mutated to permanently stop caching after its first clear (generation check against zero) passes every test in the repo. That is the classic release-mechanism regression, releasing once and never working again, and the new tests only assert the negative (no repopulation mid-flight). One test closes it: seed, clear, read again, assert it repopulates and the next read serves from memory.

Second, the wiring is silently deletable: removing the ipcMain.on registration from index.ts, or the window-all-closed clear, leaves all 1236 tests green, because index.ts is coverage-excluded and unimportable. The repo already solved this class in tests/security-boundaries.test.ts with source-text assertions; two lines there pin both. Alternatively move the registration into modsHandlers.ts where every other MODS_MANAGER channel lives, which would also fix the placement oddity.

Blocking on process rather than code: this PR is based on the #176 branch and the Targets dev checklist box is honestly unchecked. #176 just got changes requested, so this one must be retargeted onto dev after #176 lands, not merged as-is.

For the record, not blocking: under React StrictMode in dev, the mount-cleanup-mount double invoke fires one clear on every mods-page mount, and with the generation guard the first batch of icon reads on each dev visit is read and thrown away; production is unaffected since StrictMode is stripped, but anyone profiling this cache in npm run dev will see it half-broken, worth a comment or a suppression. The modsManager adapter comment still says two bridge calls, there are three. And a renderer reload while on the mods grid skips React cleanups, leaving the cache resident until the next visit-and-leave; bounded by the 16 MB budget, self-healing, but unstated.

@Zaldaryon Zaldaryon mentioned this pull request Aug 21, 2026
@Zaldaryon
Zaldaryon requested a review from Pixnop August 21, 2026 18:07

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. Both surviving mutations from the last round are dead against 761c7ad: the permanently-stops-caching mutation (generation checked against zero) now fails the new repopulates-after-clear test, which pins the positive path release mechanisms classically regress on, and neutralizing the ipcMain registration in index.ts fails the new source-text assertion in security-boundaries, with the window-all-closed clear pinned by the same block. The stacking is now honestly documented in the body with the retarget instruction, which resolves the process point: approve stands for the content, and the merge order is #176 first, then retarget this onto dev. The StrictMode dev-mode note and the reload gap stay on the record as accepted behavior.

Base automatically changed from test/issue-171-cachemodimg-protocol to dev August 21, 2026 18:37
@Zaldaryon
Zaldaryon force-pushed the perf/issue-170-release-mod-icon-cache branch from 761c7ad to b66ebff Compare August 21, 2026 18:41
@Zaldaryon
Zaldaryon merged commit 66a557d into dev Aug 21, 2026
7 checks passed
@Zaldaryon
Zaldaryon deleted the perf/issue-170-release-mod-icon-cache branch August 21, 2026 18:45
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