fix(view): bound remote image cache with LRU eviction (#532)

Krupesh-21 created 2 months ago

Change summary

go.mod            |  1 +
go.sum            |  2 ++
view/html.go      | 20 +++++++++++++++-----
view/html_test.go | 37 +++++++++++++++++++++++++++++++++++++
4 files changed, 55 insertions(+), 5 deletions(-)

Detailed changes

go.mod

@@ -17,6 +17,7 @@ require (
 	github.com/emersion/go-pgpmail v0.2.2
 	github.com/emersion/go-sasl v0.0.0-20241020182733-b788ff22d5a6
 	github.com/google/uuid v1.6.0
+	github.com/hashicorp/golang-lru/v2 v2.0.7
 	github.com/knadh/go-pop3 v1.0.2
 	github.com/yuin/goldmark v1.8.2
 	github.com/yuin/gopher-lua v1.1.2

go.sum

@@ -73,6 +73,8 @@ github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
 github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
 github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
+github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k=
+github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM=
 github.com/knadh/go-pop3 v1.0.2 h1:gbdtwzEYedLVos/vpebM2d73NTyZxEgjgRJ4S77HlzM=
 github.com/knadh/go-pop3 v1.0.2/go.mod h1:3gKw2jmrEa1lYLVtP1yEoo6bkkJ4XHDySPy8xaSjG0s=
 github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=

view/html.go

@@ -9,12 +9,12 @@ import (
 	"os"
 	"regexp"
 	"strings"
-	"sync"
 	"time"
 
 	"charm.land/lipgloss/v2"
 	"github.com/floatpane/matcha/clib"
 	"github.com/floatpane/matcha/theme"
+	lru "github.com/hashicorp/golang-lru/v2"
 )
 
 func linkStyle() lipgloss.Style {
@@ -264,8 +264,18 @@ func debugImageProtocol(format string, args ...interface{}) {
 	}
 }
 
+const remoteImageCacheSize = 20
+
 // remoteImageCache caches fetched remote images (URL -> base64 PNG string).
-var remoteImageCache sync.Map
+var remoteImageCache *lru.Cache[string, string]
+
+func init() {
+	c, err := lru.New[string, string](remoteImageCacheSize)
+	if err != nil {
+		panic(err) // only fails on size <= 0
+	}
+	remoteImageCache = c
+}
 
 // nextImageID is an auto-incrementing counter for Kitty image IDs.
 var nextImageID uint32 = 1000
@@ -283,9 +293,9 @@ func fetchRemoteBase64(url string) string {
 	}
 
 	// Check cache first
-	if cached, ok := remoteImageCache.Load(url); ok {
+	if cached, ok := remoteImageCache.Get(url); ok {
 		debugImageProtocol("remote cache hit url=%s", url)
-		return cached.(string)
+		return cached
 	}
 
 	client := &http.Client{Timeout: 5 * time.Second}
@@ -316,7 +326,7 @@ func fetchRemoteBase64(url string) string {
 
 	encoded := base64.StdEncoding.EncodeToString(result.PNGData)
 	debugImageProtocol("remote fetch ok url=%s len=%d", url, len(encoded))
-	remoteImageCache.Store(url, encoded)
+	remoteImageCache.Add(url, encoded)
 	return encoded
 }
 

view/html_test.go

@@ -1,6 +1,7 @@
 package view
 
 import (
+	"fmt"
 	"os"
 	"regexp"
 	"strings"
@@ -724,3 +725,39 @@ func TestProcessBody(t *testing.T) {
 		})
 	}
 }
+
+func TestRemoteImageCache_EvictsOldestWhenFull(t *testing.T) {
+	// Start with a clean cache so prior tests don't interfere.
+	remoteImageCache.Purge()
+	// cleaning up the current test's cache
+	defer remoteImageCache.Purge()
+
+	// overfilling the cache beyond its configured capacity.
+	overfillBy := 5
+	totalInserts := remoteImageCacheSize + overfillBy
+	for i := range totalInserts {
+		url := fmt.Sprintf("https://example.com/img%d.png", i)
+		remoteImageCache.Add(url, "fake-base64-data")
+	}
+
+	// cache should not be overfilled beyond it's capped size
+	if got := remoteImageCache.Len(); got != remoteImageCacheSize {
+		t.Errorf("expected cache size %d, got %d", remoteImageCacheSize, got)
+	}
+
+	// old entries should be evicted
+	for i := range overfillBy {
+		evictedURL := fmt.Sprintf("https://example.com/img%d.png", i)
+		if _, ok := remoteImageCache.Get(evictedURL); ok {
+			t.Errorf("expected %q to be evicted, but it's still in cache", evictedURL)
+		}
+	}
+
+	// The most recent entries should still be present.
+	for i := overfillBy; i < totalInserts; i++ {
+		keptURL := fmt.Sprintf("https://example.com/img%d.png", i)
+		if _, ok := remoteImageCache.Get(keptURL); !ok {
+			t.Errorf("expected %q to still be in cache", keptURL)
+		}
+	}
+}