fix(compare): bound aggregate decode memory

.github/testshards/internal-ui.tsv

@@ -5,7 +5,7 @@
 # baseline-run: 34627413846
 # baseline-attempts: 1
 # weights: single CI attempt; projected test sums, not measured shard runtimes
-# ui-1: 236 entries, 470.520s projected weight sum
+# ui-1: 237 entries, 470.520s projected weight sum
 # ui-2: 248 entries, 470.530s projected weight sum
 # ui-3: 202 entries, 504.870s projected weight sum
 # rebalance: 27 whole-test moves; slowest projected sum within 5% of mean
@@ -57,7 +57,8 @@ TestCompareLinkControl_CtrlLAndButtonShareReadyGate	ui-1
 TestCompareLinkToggle_CanvasOverlayOwnsPhysicalCtrlL	ui-1
 TestCompareLinkToggle_ChainsHookPersistsAndRelinksOnSecondPress	ui-1
 TestCompareLinkToggle_ZoomsOnlyTheLastHoveredPaneWithoutHeldModifier	ui-1
-TestCompareMemory_HoldsBothFullDecodesBeyondSharedCacheBudget	ui-1
+TestCompareMemory_LoadAdmission	ui-1
+TestCompareMemory_RejectsSourcesWhoseCombinedEstimateExceedsBudget	ui-1
 TestCompareOpenRefusal_DropDialogShortcutAndOpenWithAreDiscarded	ui-1
 TestComparePanInputs_CanvasDragAndShiftWheelStayInComparison	ui-1
 TestCompareRestoration_EscapeRevealsTheUnchangedFilteredGrid	ui-1

finished_refactorings/2026-09-16-bound-comparison-memory.md

@@ -0,0 +1,138 @@
+# Bound comparison memory
+
+## Problem
+
+Comparison starts both selected-source loads concurrently. Each load previously
+used the complete image-cache budget, so two decoded sources and unused GIF
+frames could remain live beyond the shared budget.
+
+## Acceptance criteria
+
+1. Each pane is admitted against half the current shared image-cache budget
+   before pixel decoding.
+   Verify: `go test -tags no_emoji,nodynamic ./internal/ui -run '^TestCompareMemory_' -count=1`
+2. Comparison decodes only the first frame of an animated GIF.
+   Verify: `go test -tags no_emoji,nodynamic ./internal/ui -run '^TestCompareAnimated_' -count=1`
+3. Existing comparison behavior remains green.
+   Verify: `go test -tags no_emoji,nodynamic ./internal/ui -run '^TestCompare' -count=1`
+
+## Non-goals and limit
+
+This does not downsample sources that exceed admission. It rejects them through
+the existing comparison failure path. Splitting the budget equally is
+deliberately conservative: it preserves concurrent loading without needing an
+allocation reservation service, at the cost of rejecting an asymmetric pair
+whose combined estimate might otherwise fit.
+
+## Tasks
+
+### Task 1 - Guard comparison decoding
+
+Owner: T0 inline
+Files: `internal/ui/compare.go`, `internal/imaging/gif.go`, existing UI tests
+Test: comparison budget and animated-source regressions in the existing test file
+Verify: acceptance commands above
+Budget: 0 spawns, 1 review round, full suite yes
+
+## Original implementation verification
+
+- The focused comparison and cache-writer regressions pass.
+- Imaging GIF fallback regressions pass.
+- `make fmt` and `make vet` pass.
+- `make verify` and `make check-test-shards` could not start because Docker is
+  not installed in the environment. `make test-native` reached the full UI
+  suite; it exposed and prompted correction of the animated-cache contract,
+  while an unrelated Explorer settings test failed and then passed in isolation.
+- GoLand/Qodana inspection tooling is unavailable in the environment.
+
+## PR #31 review follow-up
+
+Route: Standard. The lead owns the fixes and review. One read-only scout locates
+existing fixture and catalogue-verification patterns; no implementation or
+review is delegated. No dependencies or distribution obligations change.
+
+Acceptance criteria and tasks, all owned by T0:
+
+1. Replace the renamed comparison test in the shard manifest, assign any new
+   comparison regression, and keep the entry counts current.
+   Verify: `make check-test-shards`.
+2. Header-only decoded-pixel estimates cover eight-byte RGBA64/NRGBA64 pixels.
+   Cached comparisons charge the detached first-frame record's actual retained
+   bytes, including vector storage, and fresh decodes receive the same final
+   check. Cover accepted and refused tiny 16-bit sources with and without a
+   cache hit, and preserve complete cached animation records.
+   Verify: `go test -tags no_emoji,nodynamic ./internal/imaging ./internal/ui -run '^(TestEstimateDecodedBytes|TestImageBytes|TestCompare|TestImageCacheWriters_PreserveCompleteRecords)' -count=1`.
+3. Budget refusals reach a localized toast, with English/German catalogue keys.
+   Verify: the comparison memory regressions above and
+   `go test -tags no_emoji,nodynamic . -run '^TestTranslations_' -count=1`.
+
+Files: comparison adapter/tests, imaging byte accounting/tests, shard manifest,
+English/German catalogues, this evidence record and `todos.md`. Update the
+preload test's estimate comment because it uses the shared conservative helper.
+The shared helper also makes speculative preload admission more conservative;
+actual byte-cache weights remain format-specific. These are decoded-image
+admission limits, not a bound on all decoder scratch, renderer or process memory.
+
+Verification: targeted regressions first, then GoLand inspections and
+`make verify`; native amd64 CI supplies the complete suite if the local Docker
+daemon cannot meet the required platform. Commit and push are user-authorized.
+Budget: one scout, one final review, one complete-suite gate.
+
+### Follow-up verification evidence
+
+- Before the fix, the tiny-fixture tests failed for underestimated RGBA64 and
+  NRGBA64 storage, admission of a 16-bit source above its pane allowance on
+  both fresh and cached paths, and the untranslated budget-refusal toast.
+- After the fix, focused comparison, full-cache-record, byte-accounting,
+  preload and GIF-fallback regressions pass. The complete native `imaging`,
+  `ui/display` and `ui/compare` package suites also pass.
+- `go test -tags no_emoji,nodynamic . -run '^TestTranslations_' -count=1` passes.
+- `make check-test-shards` passes in Linux/amd64 Docker: 687 runnable tests,
+  three shards. Its earlier run failed on the unassigned comparison test.
+- `make verify-build` passes: formatting, TUF root, Qodana test exclusions,
+  generated assets/notices, vet and build. `git diff --check` is clean.
+- GoLand inspected all PR code files and both changed catalogues, including
+  weak warnings. The only findings are two unchanged duplicate setup fragments
+  in `imgcache_test.go`, in tests of different removal entry points. The exact
+  file is already excluded from `DuplicatedCode` in `qodana.yaml`; retain that
+  existing test-only exclusion. No production issue or new inspection finding
+  remains. The unused `assertFrozen` parameter introduced by the PR was removed
+  by using its existing assertion helper directly.
+- `make verify` refuses the local Docker daemon's `linux/aarch64` platform.
+  The full race suite runs in native Linux/amd64 CI; no isolation policy or
+  test was weakened to bypass this prerequisite. Current-head hosted results
+  and the final review disposition are recorded in
+  [PR #31](https://github.com/frathe/picfetch/pull/31).
+
+Actual cost: one read-only scout, one lead review, one full-gate attempt.
+The user authorized the fix commit and push to PR #31; the unrelated untracked
+GitHub AI scan failure report is excluded from the commit.
+
+## GitHub Codex review loop
+
+The user invoked the repository review loop after `8cdbb18`. No unresolved
+review threads existed at entry. Codex code/security reviews were already
+running for that head, so no duplicate request was posted.
+The initial Codex code and security reviews completed without findings; the
+connector posted its approval reaction after security finished. All eight CI jobs
+passed on merge `41d406d` (head `8cdbb18`, base `459663d`), including all four
+native Linux race partitions. Both CodeQL analyses passed with no new alerts.
+
+The post-suppression Qodana SARIF for `8cdbb18` reported one
+`GoUnusedExportedFunction` warning on `imaging.IsAnimatedGIF`. It is a false
+positive: `internal/ui/compare.go` calls the function before deciding whether
+to cache a decoded comparison source. Keep the function and scope the
+suppression to that declaration, with its comparison-cache purpose documented.
+Local GoLand inspection confirms the declaration and caller have no findings;
+hosted Qodana must verify the suppression on the next pushed commit.
+
+The separate GitHub Advanced Security AI scan failed before producing results
+with `CAPIError: 400 The requested model is not supported`. This service error
+is distinct from the Codex connector's security review and is not a code
+finding. Track its disposition separately in the PR's final check evidence.
+
+Acceptance remains a fresh finding-free Codex code review on the final pushed
+commit, completed Codex security review, no actionable Qodana/CodeQL findings,
+and passing required CI. Review replies and final job/report links in PR #31
+are the live evidence record, so recording outcomes does not create a new,
+unreviewed source commit. Merging and releasing remain outside this workflow.

internal/imaging/bytecache.go

@@ -335,7 +335,7 @@ func imageBytes(m image.Image) int64 {
 	}
 
 	// Anything else (a decoder's own image type, or a wrapper) falls back
-	// to the four-bytes-per-pixel ceiling - an over-estimate is the safe
+	// to the eight-bytes-per-pixel ceiling - an over-estimate is the safe
 	// direction for a budget.
 	return EstimateDecodedBytes(m.Bounds())
 }
@@ -376,14 +376,14 @@ func loadedImageBytes(l *LoadedImage) int64 {
 // EstimateDecodedBytes is the worst-case decoded size of an image whose
 // header declares these bounds - for callers deciding whether a decode is
 // worth starting at all, before there is any concrete image type to
-// measure. Deliberately the four-bytes-per-pixel ceiling: guessing low
-// here would let exactly the images this budget exists to bound slip
-// through the check.
+// measure. Eight bytes per pixel covers the RGBA64 and NRGBA64 outputs of
+// supported sixteen-bit formats as well as the smaller eight-bit outputs.
+// This estimates retained pixels, not transient decoder working memory.
 func EstimateDecodedBytes(b image.Rectangle) int64 {
 	w, h := int64(b.Dx()), int64(b.Dy())
 	if w <= 0 || h <= 0 {
 		return 0
 	}
 
-	return w * h * 4
+	return w * h * 8
 }

internal/imaging/bytecache_test.go

@@ -346,7 +346,7 @@ func TestImageBytes(t *testing.T) {
 		// block: 16 + 4 + 4. This is the case the type switch exists for -
 		// charging a JPEG 4 bytes per pixel would over-report it by 2.7x.
 		{"YCbCr 4:2:0", image.NewYCbCr(image.Rect(0, 0, 4, 4), image.YCbCrSubsampleRatio420), 16 + 4 + 4},
-		{"unknown type falls back to 4 bytes per pixel", unknownImage{image.Rect(0, 0, 4, 4)}, 4 * 4 * 4},
+		{"unknown type falls back to 8 bytes per pixel", unknownImage{image.Rect(0, 0, 4, 4)}, 4 * 4 * 8},
 	}
 
 	for _, c := range cases {
@@ -382,7 +382,7 @@ func TestEstimateDecodedBytes(t *testing.T) {
 		bounds image.Rectangle
 		want   int64
 	}{
-		{"ordinary bounds", image.Rect(0, 0, 100, 50), 100 * 50 * 4},
+		{"ordinary bounds", image.Rect(0, 0, 100, 50), 100 * 50 * 8},
 		{"empty bounds", image.Rect(0, 0, 0, 0), 0},
 		// Built as a literal rather than with image.Rect, which
 		// canonicalizes swapped corners - a negative Dx/Dy has to come out
@@ -397,6 +397,14 @@ func TestEstimateDecodedBytes(t *testing.T) {
 			}
 		})
 	}
+	for _, frame := range []image.Image{
+		image.NewRGBA64(image.Rect(0, 0, 3, 2)),
+		image.NewNRGBA64(image.Rect(0, 0, 3, 2)),
+	} {
+		if estimate, retained := EstimateDecodedBytes(frame.Bounds()), imageBytes(frame); estimate < retained {
+			t.Errorf("estimate for %T = %d, less than retained pixels %d", frame, estimate, retained)
+		}
+	}
 }
 
 // --- NewImgCache / NewThumbCache ---------------------------------------------

internal/imaging/gif.go

@@ -25,6 +25,17 @@ const (
 	gifPreviewFrameOverhead = 128
 )
 
+// IsAnimatedGIF reports whether data describes a GIF with more than one
+// image frame. It performs the same bounded structural probe used for
+// animation admission and does not decode pixels.
+// Comparison calls it to keep first-frame-only records out of the image cache.
+//
+//goland:noinspection GoUnusedExportedFunction
+func IsAnimatedGIF(data []byte) bool {
+	count, _, _, ok := probeGIF(data)
+	return ok && count > 1
+}
+
 // gifWorkingBytes estimates paletted source frames, per-frame storage,
 // decoder scratch and two full RGBA compositing canvases. Retained output
 // pixels are charged separately, since previews can be smaller than the source.

internal/ui/compare.go

@@ -2,14 +2,18 @@ package ui
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"image"
 
 	"fyne.io/fyne/v2"
 	"fyne.io/fyne/v2/lang"
 
 	"github.com/frathe/picfetch/internal/imaging"
 )
 
+var errComparisonMemoryBudget = errors.New("comparison memory budget exceeded")
+
 // comparisonActive is the composition-layer fact used by every ordinary
 // command entry. Feature packages stay independent: none of them needs to
 // know comparison exists, and comparison itself never learns about the
@@ -65,31 +69,65 @@ func (v *viewer) loadComparedImage(ctx context.Context, uri fyne.URI) (*imaging.
 		writer := v.imgCache.Capture()
 		if loaded, ok := v.imgCache.Get(uri.String()); ok {
 			if writer.Current() {
-				return loaded, nil
+				if loaded == nil || len(loaded.Frames) == 0 {
+					return loaded, nil
+				}
+				// Comparison displays only the first frame. Do not retain an
+				// animation's unused frames outside their existing cache owner.
+				frozen := *loaded
+				frozen.Frames = []image.Image{loaded.Frames[0]}
+				frozen.Delays = nil
+				if err := comparisonImageFits(&frozen, v.imgCache.Budget()); err != nil {
+					return nil, err
+				}
+				return &frozen, nil
 			}
 			continue
 		}
-		data, _, err := imaging.ReadAndProbe(ctx, uri)
+		data, bounds, err := imaging.ReadAndProbe(ctx, uri)
 		if err != nil {
 			if !writer.Current() {
 				continue
 			}
 			return nil, err
 		}
-		loaded, err := imaging.DecodeRecord(ctx, data, v.imgCache.Budget())
+		if imaging.EstimateDecodedBytes(bounds) > v.imgCache.Budget()/2 {
+			return nil, errComparisonMemoryBudget
+		}
+		// Comparison never animates, so decoding additional GIF frames would
+		// consume memory that cannot contribute to either pane.
+		loaded, err := imaging.DecodeRecord(ctx, data, 0)
 		if !writer.Current() {
 			continue
 		}
 		if err != nil {
 			return nil, err
 		}
-		if writer.Add(uri.String(), loaded) {
-			return loaded, nil
+		if err := comparisonImageFits(loaded, v.imgCache.Budget()); err != nil {
+			return nil, err
 		}
+		if !imaging.IsAnimatedGIF(data) {
+			writer.Add(uri.String(), loaded)
+		}
+		return loaded, nil
+	}
+}
+
+func comparisonImageFits(loaded *imaging.LoadedImage, budget int64) error {
+	if loaded == nil || len(loaded.Frames) == 0 {
+		return nil
+	}
+	if loaded.DecodedBytes() > budget/2 {
+		return errComparisonMemoryBudget
 	}
+	return nil
 }
 
 func (v *viewer) compareFailed(uri fyne.URI, err error) {
+	if errors.Is(err, errComparisonMemoryBudget) {
+		v.ShowToast(lang.L("Selected images exceed the comparison memory budget"))
+		return
+	}
 	v.ShowToast(fmt.Sprintf(lang.L("could not read %q: %v"), uri.Name(), err))
 }