Harden icon cache concurrency (T-606)

core/catalog/icon_cache.go

@@ -11,6 +11,7 @@ import (
 	_ "image/gif"
 	_ "image/jpeg"
 	_ "image/png"
+	"io"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -19,8 +20,11 @@ import (
 )
 
 const (
-	DefaultMaxIconBytes     int64 = 2 << 20
-	DefaultMaxIconDimension       = 2048
+	DefaultMaxIconBytes      int64 = 2 << 20
+	DefaultMaxIconDimension        = 2048
+	DefaultIconMemoryBytes   int64 = 32 << 20
+	DefaultIconMemoryEntries       = 256
+	UnknownIconContentLength int64 = -1
 )
 
 var (
@@ -29,6 +33,7 @@ var (
 	ErrIconHashMismatch     = errors.New("icon SHA-256 does not match reference")
 	ErrIconTooLarge         = errors.New("icon exceeds resource limits")
 	ErrIconImageInvalid     = errors.New("icon image is invalid")
+	ErrIconResponseInvalid  = errors.New("icon fetch response is invalid")
 	ErrIconCacheUnsafe      = errors.New("icon cache layout is unsafe")
 	ErrNoValidIcon          = errors.New("no valid icon available")
 )
@@ -39,18 +44,26 @@ type IconRequest struct {
 	DPI       int
 }
 
-// IconFetcher obtains icon bytes outside Gio Layout.
+// IconFetchResponse streams one icon while preserving its optional declared size.
+// Body ownership transfers to IconCache and reads must stop when the FetchIcon
+// context is canceled.
+type IconFetchResponse struct {
+	Body          io.ReadCloser
+	ContentLength int64
+}
+
+// IconFetcher opens an icon response outside Gio Layout.
 type IconFetcher interface {
-	FetchIcon(context.Context, IconRequest) ([]byte, error)
+	FetchIcon(context.Context, IconRequest) (IconFetchResponse, error)
 }
 
 // IconFetchFunc adapts a function to IconFetcher.
-type IconFetchFunc func(context.Context, IconRequest) ([]byte, error)
+type IconFetchFunc func(context.Context, IconRequest) (IconFetchResponse, error)
 
 func (function IconFetchFunc) FetchIcon(
 	ctx context.Context,
 	request IconRequest,
-) ([]byte, error) {
+) (IconFetchResponse, error) {
 	return function(ctx, request)
 }
 
@@ -96,7 +109,14 @@ type IconCache struct {
 	maxBytes     int64
 	maxDimension int
 	mu           sync.Mutex
-	memory       map[string][]byte
+	memory       *iconMemoryCache
+	inflight     map[string]*iconFlight
+}
+
+type iconFlight struct {
+	done   chan struct{}
+	result IconResult
+	err    error
 }
 
 // NewIconCache creates a cache with safe default resource limits.
@@ -106,35 +126,70 @@ func NewIconCache(root string, fetcher IconFetcher) *IconCache {
 		fetcher:      fetcher,
 		maxBytes:     DefaultMaxIconBytes,
 		maxDimension: DefaultMaxIconDimension,
-		memory:       make(map[string][]byte),
+		memory: newIconMemoryCache(
+			DefaultIconMemoryBytes,
+			DefaultIconMemoryEntries,
+		),
+		inflight: make(map[string]*iconFlight),
 	}
 }
 
 // Load resolves memory, verified disk, then verified remote bytes.
 func (cache *IconCache) Load(ctx context.Context, request IconRequest) (IconResult, error) {
-	cache.mu.Lock()
-	defer cache.mu.Unlock()
-
 	digest, key, err := cacheKey(request)
 	if err != nil {
 		return IconResult{}, err
 	}
-	if document, exists := cache.memory[key]; exists {
+
+	cache.mu.Lock()
+	if document, exists := cache.memory.get(key); exists {
+		cache.mu.Unlock()
 		return IconResult{
-			Bytes:  append([]byte(nil), document...),
+			Bytes:  document,
 			Source: IconSourceMemory,
 		}, nil
 	}
+	if flight, exists := cache.inflight[key]; exists {
+		cache.mu.Unlock()
+		select {
+		case <-flight.done:
+			return cloneIconResult(flight.result), flight.err
+		case <-ctx.Done():
+			return IconResult{}, ctx.Err()
+		}
+	}
+	flight := &iconFlight{done: make(chan struct{})}
+	cache.inflight[key] = flight
+	cache.mu.Unlock()
 
+	result, loadErr := cache.loadUncached(ctx, request, digest)
+
+	cache.mu.Lock()
+	if loadErr == nil {
+		cache.memory.put(key, result.Bytes)
+	}
+	flight.result = cloneIconResult(result)
+	flight.err = loadErr
+	delete(cache.inflight, key)
+	close(flight.done)
+	cache.mu.Unlock()
+
+	return cloneIconResult(result), loadErr
+}
+
+func (cache *IconCache) loadUncached(
+	ctx context.Context,
+	request IconRequest,
+	digest string,
+) (IconResult, error) {
 	filePath, pathErr := cache.filePath(digest, request.DPI)
 	if pathErr != nil {
 		return IconResult{}, pathErr
 	}
 	document, diskErr := cache.loadDisk(filePath, digest)
 	if diskErr == nil {
-		cache.memory[key] = append([]byte(nil), document...)
 		return IconResult{
-			Bytes:  append([]byte(nil), document...),
+			Bytes:  document,
 			Source: IconSourceDisk,
 		}, nil
 	}
@@ -157,7 +212,11 @@ func (cache *IconCache) Load(ctx context.Context, request IconRequest) (IconResu
 			Fetch: errors.New("icon fetcher is not configured"),
 		}
 	}
-	document, fetchErr := cache.fetcher.FetchIcon(ctx, request)
+	response, fetchErr := cache.fetcher.FetchIcon(ctx, request)
+	if fetchErr != nil {
+		return IconResult{}, &IconLoadError{Disk: diskErr, Fetch: fetchErr}
+	}
+	document, fetchErr = cache.readFetchedIcon(ctx, response)
 	if fetchErr != nil {
 		return IconResult{}, &IconLoadError{Disk: diskErr, Fetch: fetchErr}
 	}
@@ -166,14 +225,73 @@ func (cache *IconCache) Load(ctx context.Context, request IconRequest) (IconResu
 	}
 
 	storeErr := cache.storeDisk(filePath, document)
-	cache.memory[key] = append([]byte(nil), document...)
 	return IconResult{
-		Bytes:   append([]byte(nil), document...),
+		Bytes:   document,
 		Source:  IconSourceRemote,
 		Warning: storeErr,
 	}, nil
 }
 
+func (cache *IconCache) readFetchedIcon(
+	ctx context.Context,
+	response IconFetchResponse,
+) (document []byte, resultErr error) {
+	if response.Body == nil {
+		return nil, fmt.Errorf("%w: nil body", ErrIconResponseInvalid)
+	}
+	defer func() {
+		if closeErr := response.Body.Close(); resultErr == nil && closeErr != nil {
+			resultErr = fmt.Errorf("close icon response: %w", closeErr)
+		}
+	}()
+
+	if response.ContentLength < UnknownIconContentLength {
+		return nil, fmt.Errorf(
+			"%w: content length %d",
+			ErrIconResponseInvalid,
+			response.ContentLength,
+		)
+	}
+	maxBytes := cache.iconByteLimit()
+	if response.ContentLength > maxBytes {
+		return nil, fmt.Errorf(
+			"%w: declared bytes=%d limit=%d",
+			ErrIconTooLarge,
+			response.ContentLength,
+			maxBytes,
+		)
+	}
+	if err := ctx.Err(); err != nil {
+		return nil, err
+	}
+
+	document, readErr := io.ReadAll(io.LimitReader(response.Body, maxBytes+1))
+	if readErr != nil {
+		return nil, fmt.Errorf("read icon response: %w", readErr)
+	}
+	if int64(len(document)) > maxBytes {
+		return nil, fmt.Errorf(
+			"%w: bytes=%d limit=%d",
+			ErrIconTooLarge,
+			len(document),
+			maxBytes,
+		)
+	}
+	return document, nil
+}
+
+func (cache *IconCache) iconByteLimit() int64 {
+	if cache.maxBytes <= 0 {
+		return DefaultMaxIconBytes
+	}
+	return cache.maxBytes
+}
+
+func cloneIconResult(result IconResult) IconResult {
+	result.Bytes = append([]byte(nil), result.Bytes...)
+	return result
+}
+
 // DecodeIcon decodes already verified bytes outside Layout for ApplyIcon.
 func DecodeIcon(document []byte) (image.Image, error) {
 	decoded, _, err := image.Decode(bytes.NewReader(document))
@@ -184,10 +302,7 @@ func DecodeIcon(document []byte) (image.Image, error) {
 }
 
 func (cache *IconCache) validate(document []byte, digest string) error {
-	maxBytes := cache.maxBytes
-	if maxBytes <= 0 {
-		maxBytes = DefaultMaxIconBytes
-	}
+	maxBytes := cache.iconByteLimit()
 	if int64(len(document)) > maxBytes {
 		return fmt.Errorf(
 			"%w: bytes=%d limit=%d",

core/catalog/icon_cache_concurrency_test.go

@@ -0,0 +1,411 @@
+package catalog
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"os"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+type iconLoadOutcome struct {
+	result IconResult
+	err    error
+}
+
+func TestIconCacheMemoryHitDoesNotWaitForDifferentKey(t *testing.T) {
+	fastDocument := testPNG(t, 15, 15)
+	slowDocument := testPNG(t, 16, 16)
+	fastRequest := iconRequest(fastDocument, 96)
+	slowRequest := iconRequest(slowDocument, 96)
+	slowStarted := make(chan struct{})
+	releaseSlow := make(chan struct{})
+
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		ctx context.Context,
+		request IconRequest,
+	) (IconFetchResponse, error) {
+		if request.Reference == slowRequest.Reference {
+			close(slowStarted)
+			select {
+			case <-releaseSlow:
+			case <-ctx.Done():
+				return IconFetchResponse{}, ctx.Err()
+			}
+			return iconResponse(slowDocument), nil
+		}
+		return iconResponse(fastDocument), nil
+	}))
+
+	if _, err := cache.Load(context.Background(), fastRequest); err != nil {
+		t.Fatalf("Load(seed memory) error = %v", err)
+	}
+	slowOutcome := loadIconAsync(cache, context.Background(), slowRequest)
+	waitSignal(t, slowStarted, "slow fetch did not start")
+
+	fastOutcome := loadIconAsync(cache, context.Background(), fastRequest)
+	select {
+	case outcome := <-fastOutcome:
+		if outcome.err != nil || outcome.result.Source != IconSourceMemory {
+			t.Fatalf("memory outcome = %#v, %v", outcome.result, outcome.err)
+		}
+	case <-time.After(2 * time.Second):
+		close(releaseSlow)
+		t.Fatal("memory hit waited for an unrelated slow fetch")
+	}
+
+	close(releaseSlow)
+	if outcome := waitOutcome(t, slowOutcome); outcome.err != nil {
+		t.Fatalf("slow Load() error = %v", outcome.err)
+	}
+}
+
+func TestIconCacheFetchesDifferentKeysConcurrently(t *testing.T) {
+	documents := [][]byte{testPNG(t, 17, 16), testPNG(t, 18, 16)}
+	requests := []IconRequest{
+		iconRequest(documents[0], 96),
+		iconRequest(documents[1], 96),
+	}
+	documentByReference := map[string][]byte{
+		requests[0].Reference: documents[0],
+		requests[1].Reference: documents[1],
+	}
+	started := make(chan string, len(requests))
+	release := make(chan struct{})
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		ctx context.Context,
+		request IconRequest,
+	) (IconFetchResponse, error) {
+		started <- request.Reference
+		select {
+		case <-release:
+			return iconResponse(documentByReference[request.Reference]), nil
+		case <-ctx.Done():
+			return IconFetchResponse{}, ctx.Err()
+		}
+	}))
+
+	first := loadIconAsync(cache, context.Background(), requests[0])
+	waitSignal(t, started, "first fetch did not start")
+	second := loadIconAsync(cache, context.Background(), requests[1])
+	waitSignal(t, started, "second key did not fetch while first key was blocked")
+	close(release)
+
+	for index, outcomeChannel := range []<-chan iconLoadOutcome{first, second} {
+		outcome := waitOutcome(t, outcomeChannel)
+		if outcome.err != nil || outcome.result.Source != IconSourceRemote {
+			t.Fatalf("outcome %d = %#v, %v", index, outcome.result, outcome.err)
+		}
+	}
+}
+
+func TestIconCacheCoalescesSameKeyAndReturnsIndependentBytes(t *testing.T) {
+	document := testPNG(t, 19, 16)
+	request := iconRequest(document, 96)
+	started := make(chan struct{})
+	release := make(chan struct{})
+	var fetchCalls int32
+	root := t.TempDir()
+	cache := NewIconCache(root, IconFetchFunc(func(
+		ctx context.Context,
+		_ IconRequest,
+	) (IconFetchResponse, error) {
+		atomic.AddInt32(&fetchCalls, 1)
+		close(started)
+		select {
+		case <-release:
+			return iconResponse(document), nil
+		case <-ctx.Done():
+			return IconFetchResponse{}, ctx.Err()
+		}
+	}))
+
+	leader := loadIconAsync(cache, context.Background(), request)
+	waitSignal(t, started, "leader fetch did not start")
+
+	cache.mu.Lock()
+	follower := loadIconAsync(cache, context.Background(), request)
+	close(release)
+	cache.mu.Unlock()
+
+	leaderOutcome := waitOutcome(t, leader)
+	followerOutcome := waitOutcome(t, follower)
+	if leaderOutcome.err != nil || followerOutcome.err != nil {
+		t.Fatalf("coalesced errors = %v, %v", leaderOutcome.err, followerOutcome.err)
+	}
+	if got := atomic.LoadInt32(&fetchCalls); got != 1 {
+		t.Fatalf("fetch calls = %d, want 1", got)
+	}
+	if !bytes.Equal(leaderOutcome.result.Bytes, followerOutcome.result.Bytes) {
+		t.Fatal("coalesced callers received different content")
+	}
+	leaderOutcome.result.Bytes[0] ^= 0xff
+	if bytes.Equal(leaderOutcome.result.Bytes, followerOutcome.result.Bytes) {
+		t.Fatal("coalesced callers shared a mutable backing array")
+	}
+	entries, err := os.ReadDir(root)
+	if err != nil || len(entries) != 1 {
+		t.Fatalf("disk entries = %v, error=%v; want one", entries, err)
+	}
+}
+
+func TestIconCacheFollowerCancellationDoesNotCancelLeader(t *testing.T) {
+	document := testPNG(t, 20, 16)
+	request := iconRequest(document, 96)
+	started := make(chan struct{})
+	release := make(chan struct{})
+	var fetchCalls int32
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		ctx context.Context,
+		_ IconRequest,
+	) (IconFetchResponse, error) {
+		atomic.AddInt32(&fetchCalls, 1)
+		close(started)
+		select {
+		case <-release:
+			return iconResponse(document), nil
+		case <-ctx.Done():
+			return IconFetchResponse{}, ctx.Err()
+		}
+	}))
+
+	leader := loadIconAsync(cache, context.Background(), request)
+	waitSignal(t, started, "leader fetch did not start")
+	followerContext, cancelFollower := context.WithCancel(context.Background())
+	cancelFollower()
+	if _, err := cache.Load(followerContext, request); !errors.Is(err, context.Canceled) {
+		t.Fatalf("follower error = %v, want context canceled", err)
+	}
+	if got := atomic.LoadInt32(&fetchCalls); got != 1 {
+		t.Fatalf("fetch calls after follower cancel = %d, want 1", got)
+	}
+
+	close(release)
+	if outcome := waitOutcome(t, leader); outcome.err != nil {
+		t.Fatalf("leader error = %v", outcome.err)
+	}
+}
+
+func TestIconCacheLeaderCancellationClearsFlightForRetry(t *testing.T) {
+	document := testPNG(t, 21, 16)
+	request := iconRequest(document, 96)
+	firstStarted := make(chan struct{})
+	var fetchCalls int32
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		ctx context.Context,
+		_ IconRequest,
+	) (IconFetchResponse, error) {
+		call := atomic.AddInt32(&fetchCalls, 1)
+		if call == 1 {
+			close(firstStarted)
+			<-ctx.Done()
+			return IconFetchResponse{}, ctx.Err()
+		}
+		return iconResponse(document), nil
+	}))
+
+	leaderContext, cancelLeader := context.WithCancel(context.Background())
+	first := loadIconAsync(cache, leaderContext, request)
+	waitSignal(t, firstStarted, "cancelable leader did not start")
+	cancelLeader()
+	if outcome := waitOutcome(t, first); !errors.Is(outcome.err, context.Canceled) {
+		t.Fatalf("leader error = %v, want context canceled", outcome.err)
+	}
+
+	cache.mu.Lock()
+	flights := len(cache.inflight)
+	cache.mu.Unlock()
+	if flights != 0 {
+		t.Fatalf("in-flight entries after failure = %d, want 0", flights)
+	}
+	result, err := cache.Load(context.Background(), request)
+	if err != nil || result.Source != IconSourceRemote {
+		t.Fatalf("retry result = %#v, error=%v", result, err)
+	}
+	if got := atomic.LoadInt32(&fetchCalls); got != 2 {
+		t.Fatalf("fetch calls after retry = %d, want 2", got)
+	}
+}
+
+func TestIconCacheBoundsAndClosesFetchedBodies(t *testing.T) {
+	document := bytes.Repeat([]byte{0x42}, 32)
+	request := iconRequest(document, 96)
+	tests := []struct {
+		name          string
+		contentLength int64
+		wantRead      int64
+	}{
+		{name: "declared oversized", contentLength: 9, wantRead: 0},
+		{name: "unknown oversized", contentLength: UnknownIconContentLength, wantRead: 9},
+		{name: "underdeclared oversized", contentLength: 1, wantRead: 9},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			body := newTrackingIconBody(document)
+			cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+				context.Context,
+				IconRequest,
+			) (IconFetchResponse, error) {
+				return IconFetchResponse{
+					Body:          body,
+					ContentLength: test.contentLength,
+				}, nil
+			}))
+			cache.maxBytes = 8
+
+			_, err := cache.Load(context.Background(), request)
+			if !errors.Is(err, ErrIconTooLarge) {
+				t.Fatalf("Load() error = %v, want %v", err, ErrIconTooLarge)
+			}
+			if got := atomic.LoadInt64(&body.bytesRead); got != test.wantRead {
+				t.Fatalf("body bytes read = %d, want %d", got, test.wantRead)
+			}
+			if got := atomic.LoadInt32(&body.closed); got != 1 {
+				t.Fatalf("body close calls = %d, want 1", got)
+			}
+		})
+	}
+}
+
+func TestIconCacheClosesFetchedBodyOnValidationFailure(t *testing.T) {
+	document := testPNG(t, 25, 16)
+	request := iconRequest(testPNG(t, 26, 16), 96)
+	body := newTrackingIconBody(document)
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		context.Context,
+		IconRequest,
+	) (IconFetchResponse, error) {
+		return IconFetchResponse{
+			Body:          body,
+			ContentLength: int64(len(document)),
+		}, nil
+	}))
+
+	_, err := cache.Load(context.Background(), request)
+	if !errors.Is(err, ErrIconHashMismatch) {
+		t.Fatalf("Load() error = %v, want %v", err, ErrIconHashMismatch)
+	}
+	if got := atomic.LoadInt64(&body.bytesRead); got != int64(len(document)) {
+		t.Fatalf("body bytes read = %d, want %d", got, len(document))
+	}
+	if got := atomic.LoadInt32(&body.closed); got != 1 {
+		t.Fatalf("body close calls = %d, want 1", got)
+	}
+}
+
+func TestIconCacheCanceledContextClosesBodyWithoutReading(t *testing.T) {
+	document := testPNG(t, 27, 16)
+	request := iconRequest(document, 96)
+	body := newTrackingIconBody(document)
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		context.Context,
+		IconRequest,
+	) (IconFetchResponse, error) {
+		return IconFetchResponse{
+			Body:          body,
+			ContentLength: int64(len(document)),
+		}, nil
+	}))
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	_, err := cache.Load(ctx, request)
+	if !errors.Is(err, context.Canceled) {
+		t.Fatalf("Load() error = %v, want context canceled", err)
+	}
+	if got := atomic.LoadInt64(&body.bytesRead); got != 0 {
+		t.Fatalf("body bytes read = %d, want 0", got)
+	}
+	if got := atomic.LoadInt32(&body.closed); got != 1 {
+		t.Fatalf("body close calls = %d, want 1", got)
+	}
+}
+
+func TestIconCacheRejectsInvalidFetchResponses(t *testing.T) {
+	document := testPNG(t, 28, 16)
+	request := iconRequest(document, 96)
+	t.Run("nil body", func(t *testing.T) {
+		cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+			context.Context,
+			IconRequest,
+		) (IconFetchResponse, error) {
+			return IconFetchResponse{ContentLength: int64(len(document))}, nil
+		}))
+		_, err := cache.Load(context.Background(), request)
+		if !errors.Is(err, ErrIconResponseInvalid) {
+			t.Fatalf("Load() error = %v, want %v", err, ErrIconResponseInvalid)
+		}
+	})
+	t.Run("invalid content length", func(t *testing.T) {
+		body := newTrackingIconBody(document)
+		cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+			context.Context,
+			IconRequest,
+		) (IconFetchResponse, error) {
+			return IconFetchResponse{Body: body, ContentLength: -2}, nil
+		}))
+		_, err := cache.Load(context.Background(), request)
+		if !errors.Is(err, ErrIconResponseInvalid) {
+			t.Fatalf("Load() error = %v, want %v", err, ErrIconResponseInvalid)
+		}
+		if got := atomic.LoadInt64(&body.bytesRead); got != 0 {
+			t.Fatalf("body bytes read = %d, want 0", got)
+		}
+		if got := atomic.LoadInt32(&body.closed); got != 1 {
+			t.Fatalf("body close calls = %d, want 1", got)
+		}
+	})
+}
+
+type trackingIconBody struct {
+	reader    *bytes.Reader
+	bytesRead int64
+	closed    int32
+}
+
+func newTrackingIconBody(document []byte) *trackingIconBody {
+	return &trackingIconBody{reader: bytes.NewReader(document)}
+}
+
+func (body *trackingIconBody) Read(buffer []byte) (int, error) {
+	read, err := body.reader.Read(buffer)
+	atomic.AddInt64(&body.bytesRead, int64(read))
+	return read, err
+}
+
+func (body *trackingIconBody) Close() error {
+	atomic.AddInt32(&body.closed, 1)
+	return nil
+}
+
+func loadIconAsync(
+	cache *IconCache,
+	ctx context.Context,
+	request IconRequest,
+) <-chan iconLoadOutcome {
+	outcome := make(chan iconLoadOutcome, 1)
+	go func() {
+		result, err := cache.Load(ctx, request)
+		outcome <- iconLoadOutcome{result: result, err: err}
+	}()
+	return outcome
+}
+
+func waitSignal[T any](t *testing.T, signal <-chan T, failure string) T {
+	t.Helper()
+	select {
+	case value := <-signal:
+		return value
+	case <-time.After(2 * time.Second):
+		t.Fatal(failure)
+		var zero T
+		return zero
+	}
+}
+
+func waitOutcome(t *testing.T, outcome <-chan iconLoadOutcome) iconLoadOutcome {
+	t.Helper()
+	return waitSignal(t, outcome, "icon load did not finish")
+}

core/catalog/icon_cache_test.go

@@ -9,6 +9,7 @@ import (
 	"image"
 	"image/color"
 	"image/png"
+	"io"
 	"os"
 	"path/filepath"
 	"testing"
@@ -22,9 +23,9 @@ func TestIconCacheUsesMemoryAndOfflineDisk(t *testing.T) {
 	cache := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
+	) (IconFetchResponse, error) {
 		fetchCalls++
-		return document, nil
+		return iconResponse(document), nil
 	}))
 
 	result, err := cache.Load(context.Background(), request)
@@ -46,8 +47,8 @@ func TestIconCacheUsesMemoryAndOfflineDisk(t *testing.T) {
 	restarted := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
-		return nil, offline
+	) (IconFetchResponse, error) {
+		return IconFetchResponse{}, offline
 	}))
 	result, err = restarted.Load(context.Background(), request)
 	if err != nil {
@@ -65,9 +66,9 @@ func TestIconCacheSeparatesDPIKeys(t *testing.T) {
 	cache := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
+	) (IconFetchResponse, error) {
 		fetchCalls++
-		return document, nil
+		return iconResponse(document), nil
 	}))
 
 	for _, dpi := range []int{96, 144} {
@@ -133,8 +134,8 @@ func TestIconCacheRejectsUntrustedImages(t *testing.T) {
 			cache := NewIconCache(root, IconFetchFunc(func(
 				context.Context,
 				IconRequest,
-			) ([]byte, error) {
-				return test.document, nil
+			) (IconFetchResponse, error) {
+				return iconResponse(test.document), nil
 			}))
 			if test.configure != nil {
 				test.configure(cache)
@@ -161,8 +162,8 @@ func TestIconCacheRepairsCorruptDiskAndReportsOfflineFailure(t *testing.T) {
 	online := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
-		return document, nil
+	) (IconFetchResponse, error) {
+		return iconResponse(document), nil
 	}))
 	if _, err := online.Load(context.Background(), request); err != nil {
 		t.Fatalf("Load(seed) error = %v", err)
@@ -181,9 +182,9 @@ func TestIconCacheRepairsCorruptDiskAndReportsOfflineFailure(t *testing.T) {
 	repairing := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
+	) (IconFetchResponse, error) {
 		repairs++
-		return document, nil
+		return iconResponse(document), nil
 	}))
 	result, err := repairing.Load(context.Background(), request)
 	if err != nil {
@@ -199,8 +200,8 @@ func TestIconCacheRepairsCorruptDiskAndReportsOfflineFailure(t *testing.T) {
 	offline := NewIconCache(root, IconFetchFunc(func(
 		context.Context,
 		IconRequest,
-	) ([]byte, error) {
-		return nil, errors.New("offline")
+	) (IconFetchResponse, error) {
+		return IconFetchResponse{}, errors.New("offline")
 	}))
 	_, err = offline.Load(context.Background(), request)
 	if !errors.Is(err, ErrNoValidIcon) {
@@ -227,6 +228,13 @@ func iconRequest(document []byte, dpi int) IconRequest {
 	}
 }
 
+func iconResponse(document []byte) IconFetchResponse {
+	return IconFetchResponse{
+		Body:          io.NopCloser(bytes.NewReader(document)),
+		ContentLength: int64(len(document)),
+	}
+}
+
 func testPNG(t *testing.T, width, height int) []byte {
 	t.Helper()
 	source := image.NewNRGBA(image.Rect(0, 0, width, height))

core/catalog/icon_memory.go

@@ -0,0 +1,73 @@
+package catalog
+
+import "container/list"
+
+type iconMemoryEntry struct {
+	key      string
+	document []byte
+}
+
+type iconMemoryCache struct {
+	maxBytes   int64
+	maxEntries int
+	usedBytes  int64
+	entries    map[string]*list.Element
+	order      list.List
+}
+
+func newIconMemoryCache(maxBytes int64, maxEntries int) *iconMemoryCache {
+	return &iconMemoryCache{
+		maxBytes:   maxBytes,
+		maxEntries: maxEntries,
+		entries:    make(map[string]*list.Element),
+	}
+}
+
+func (memory *iconMemoryCache) get(key string) ([]byte, bool) {
+	element, exists := memory.entries[key]
+	if !exists {
+		return nil, false
+	}
+	memory.order.MoveToFront(element)
+	entry := element.Value.(*iconMemoryEntry)
+	return append([]byte(nil), entry.document...), true
+}
+
+func (memory *iconMemoryCache) put(key string, document []byte) {
+	if memory.maxBytes <= 0 || memory.maxEntries <= 0 {
+		return
+	}
+	stored := append([]byte(nil), document...)
+	if element, exists := memory.entries[key]; exists {
+		entry := element.Value.(*iconMemoryEntry)
+		memory.usedBytes -= int64(len(entry.document))
+		entry.document = stored
+		memory.usedBytes += int64(len(stored))
+		memory.order.MoveToFront(element)
+	} else {
+		entry := &iconMemoryEntry{key: key, document: stored}
+		element := memory.order.PushFront(entry)
+		memory.entries[key] = element
+		memory.usedBytes += int64(len(stored))
+	}
+
+	for memory.overLimit() {
+		memory.removeOldest()
+	}
+}
+
+func (memory *iconMemoryCache) overLimit() bool {
+	return memory.usedBytes > memory.maxBytes ||
+		len(memory.entries) > memory.maxEntries
+}
+
+func (memory *iconMemoryCache) removeOldest() {
+	element := memory.order.Back()
+	if element == nil {
+		return
+	}
+	entry := element.Value.(*iconMemoryEntry)
+	delete(memory.entries, entry.key)
+	memory.usedBytes -= int64(len(entry.document))
+	memory.order.Remove(element)
+}

core/catalog/icon_memory_test.go

@@ -0,0 +1,124 @@
+package catalog
+
+import (
+	"context"
+	"sync/atomic"
+	"testing"
+)
+
+func TestIconMemoryCacheEvictsOldestEntryAndPromotesHits(t *testing.T) {
+	memory := newIconMemoryCache(64, 2)
+	memory.put("a", []byte{1, 2})
+	memory.put("b", []byte{3, 4})
+	if _, exists := memory.get("a"); !exists {
+		t.Fatal("expected a memory hit")
+	}
+	memory.put("c", []byte{5, 6})
+
+	if _, exists := memory.get("b"); exists {
+		t.Fatal("least-recently-used entry b was retained")
+	}
+	if _, exists := memory.get("a"); !exists {
+		t.Fatal("promoted entry a was evicted")
+	}
+	if _, exists := memory.get("c"); !exists {
+		t.Fatal("new entry c was evicted")
+	}
+	if memory.usedBytes != 4 || len(memory.entries) != 2 || memory.order.Len() != 2 {
+		t.Fatalf(
+			"memory state = bytes:%d entries:%d order:%d",
+			memory.usedBytes,
+			len(memory.entries),
+			memory.order.Len(),
+		)
+	}
+}
+
+func TestIconMemoryCacheTracksReplacementAndByteLimit(t *testing.T) {
+	memory := newIconMemoryCache(5, 10)
+	memory.put("a", []byte{1, 2, 3})
+	memory.put("b", []byte{4, 5})
+	memory.put("a", []byte{6, 7, 8, 9})
+
+	if _, exists := memory.get("b"); exists {
+		t.Fatal("byte limit did not evict the oldest entry")
+	}
+	document, exists := memory.get("a")
+	if !exists || len(document) != 4 {
+		t.Fatalf("replacement = %v, exists=%v", document, exists)
+	}
+	document[0] = 0xff
+	again, _ := memory.get("a")
+	if again[0] == 0xff {
+		t.Fatal("memory hit exposed the stored backing array")
+	}
+	if memory.usedBytes != 4 || len(memory.entries) != 1 {
+		t.Fatalf("memory state = bytes:%d entries:%d", memory.usedBytes, len(memory.entries))
+	}
+}
+
+func TestIconCacheLRUEvictionReloadsVerifiedDisk(t *testing.T) {
+	documents := [][]byte{
+		testPNG(t, 22, 16),
+		testPNG(t, 23, 16),
+		testPNG(t, 24, 16),
+	}
+	requests := make([]IconRequest, len(documents))
+	documentByReference := make(map[string][]byte, len(documents))
+	for index, document := range documents {
+		requests[index] = iconRequest(document, 96)
+		documentByReference[requests[index].Reference] = document
+	}
+	var fetchCalls int32
+	cache := NewIconCache(t.TempDir(), IconFetchFunc(func(
+		_ context.Context,
+		request IconRequest,
+	) (IconFetchResponse, error) {
+		atomic.AddInt32(&fetchCalls, 1)
+		return iconResponse(documentByReference[request.Reference]), nil
+	}))
+	cache.memory.maxEntries = 2
+
+	for _, request := range requests[:2] {
+		if _, err := cache.Load(context.Background(), request); err != nil {
+			t.Fatalf("Load(seed) error = %v", err)
+		}
+	}
+	if result, err := cache.Load(context.Background(), requests[0]); err != nil || result.Source != IconSourceMemory {
+		t.Fatalf("promote result = %#v, error=%v", result, err)
+	}
+	if _, err := cache.Load(context.Background(), requests[2]); err != nil {
+		t.Fatalf("Load(evict) error = %v", err)
+	}
+	result, err := cache.Load(context.Background(), requests[1])
+	if err != nil || result.Source != IconSourceDisk {
+		t.Fatalf("reload result = %#v, error=%v", result, err)
+	}
+	if got := atomic.LoadInt32(&fetchCalls); got != 3 {
+		t.Fatalf("fetch calls = %d, want 3", got)
+	}
+}
+
+func TestNewIconCacheUsesBoundedMemoryDefaults(t *testing.T) {
+	cache := NewIconCache(t.TempDir(), nil)
+	if cache.memory.maxBytes != DefaultIconMemoryBytes ||
+		cache.memory.maxEntries != DefaultIconMemoryEntries {
+		t.Fatalf(
+			"memory limits = %d bytes/%d entries",
+			cache.memory.maxBytes,
+			cache.memory.maxEntries,
+		)
+	}
+}
+
+func TestIconMemoryCacheCanBeDisabledWithNonPositiveLimits(t *testing.T) {
+	for _, memory := range []*iconMemoryCache{
+		newIconMemoryCache(0, 1),
+		newIconMemoryCache(1, 0),
+	} {
+		memory.put("disabled", []byte{1})
+		if len(memory.entries) != 0 || memory.usedBytes != 0 {
+			t.Fatalf("disabled memory retained state: %#v", memory)
+		}
+	}
+}