From c28a98082b6162ce3338696adbcc56680bc18248 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Fri, 5 Sep 2025 15:00:38 +0200 Subject: [PATCH] chore: cache: move repo != cache.Repo in readCache - it only is used after calling readCache - add unit test it reduces the number of testcase to be considered in handler --- act/artifactcache/caches.go | 8 +++-- act/artifactcache/caches_test.go | 54 ++++++++++++++++++++++++++++++++ act/artifactcache/handler.go | 21 ++----------- 3 files changed, 63 insertions(+), 20 deletions(-) create mode 100644 act/artifactcache/caches_test.go diff --git a/act/artifactcache/caches.go b/act/artifactcache/caches.go index 2f5fc0cf..014a6011 100644 --- a/act/artifactcache/caches.go +++ b/act/artifactcache/caches.go @@ -20,7 +20,7 @@ import ( type caches interface { openDB() (*bolthold.Store, error) validateMac(rundata RunData) (string, error) - readCache(id uint64) (*Cache, error) + readCache(id uint64, repo string) (*Cache, error) useCache(id uint64) error setgcAt(at time.Time) gcCache() @@ -139,7 +139,7 @@ func insertCache(db *bolthold.Store, cache *Cache) error { return nil } -func (c *cachesImpl) readCache(id uint64) (*Cache, error) { +func (c *cachesImpl) readCache(id uint64, repo string) (*Cache, error) { db, err := c.openDB() if err != nil { return nil, err @@ -149,6 +149,10 @@ func (c *cachesImpl) readCache(id uint64) (*Cache, error) { if err := db.Get(id, cache); err != nil { return nil, fmt.Errorf("readCache: Get(%v): %w", id, err) } + if cache.Repo != repo { + return nil, fmt.Errorf("readCache: Get(%v): cache.Repo %s != repo %s", id, cache.Repo, repo) + } + return cache, nil } diff --git a/act/artifactcache/caches_test.go b/act/artifactcache/caches_test.go new file mode 100644 index 00000000..a08a9af7 --- /dev/null +++ b/act/artifactcache/caches_test.go @@ -0,0 +1,54 @@ +package artifactcache + +import ( + "testing" + "time" + + "github.com/sirupsen/logrus" + "github.com/timshannon/bolthold" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCacheReadWrite(t *testing.T) { + caches, err := newCaches(t.TempDir(), "secret", logrus.New()) + require.NoError(t, err) + t.Run("NotFound", func(t *testing.T) { + found, err := caches.readCache(456, "repo") + assert.Nil(t, found) + assert.ErrorIs(t, err, bolthold.ErrNotFound) + }) + + repo := "repository" + cache := &Cache{ + Repo: repo, + Key: "key", + Version: "version", + Size: 444, + } + now := time.Now().Unix() + cache.CreatedAt = now + cache.UsedAt = now + cache.Repo = repo + + t.Run("Insert", func(t *testing.T) { + db, err := caches.openDB() + require.NoError(t, err) + defer db.Close() + assert.NoError(t, insertCache(db, cache)) + }) + + t.Run("Found", func(t *testing.T) { + found, err := caches.readCache(cache.ID, cache.Repo) + require.NoError(t, err) + assert.Equal(t, cache.ID, found.ID) + }) + + t.Run("InvalidRepo", func(t *testing.T) { + invalidRepo := "INVALID REPO" + found, err := caches.readCache(cache.ID, invalidRepo) + assert.Nil(t, found) + assert.ErrorContains(t, err, invalidRepo) + }) +} diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index a4985ea3..d8a56e19 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -265,7 +265,7 @@ func (h *handler) upload(w http.ResponseWriter, r *http.Request, params httprout return } - cache, err := h.caches.readCache(id) + cache, err := h.caches.readCache(id, repo) if err != nil { if errors.Is(err, bolthold.ErrNotFound) { h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) @@ -275,11 +275,6 @@ func (h *handler) upload(w http.ResponseWriter, r *http.Request, params httprout return } - if cache.Repo != repo { - // can only happen if the cache is corrupted - h.responseFatalJSON(w, r, fmt.Errorf("cache repo is not valid")) - return - } if cache.WriteIsolationKey != rundata.WriteIsolationKey { h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey)) return @@ -320,7 +315,7 @@ func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprout return } - cache, err := h.caches.readCache(id) + cache, err := h.caches.readCache(id, repo) if err != nil { if errors.Is(err, bolthold.ErrNotFound) { h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) @@ -330,11 +325,6 @@ func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprout return } - if cache.Repo != repo { - // can only happen if the cache is corrupted - h.responseFatalJSON(w, r, fmt.Errorf("cache repo is not valid")) - return - } if cache.WriteIsolationKey != rundata.WriteIsolationKey { h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey)) return @@ -384,7 +374,7 @@ func (h *handler) get(w http.ResponseWriter, r *http.Request, params httprouter. return } - cache, err := h.caches.readCache(id) + cache, err := h.caches.readCache(id, repo) if err != nil { if errors.Is(err, bolthold.ErrNotFound) { h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) @@ -394,11 +384,6 @@ func (h *handler) get(w http.ResponseWriter, r *http.Request, params httprouter. return } - if cache.Repo != repo { - // can only happen if the cache is corrupted - h.responseFatalJSON(w, r, fmt.Errorf("cache repo is not valid")) - return - } // reads permitted against caches w/ the same isolation key, or no isolation key if cache.WriteIsolationKey != rundata.WriteIsolationKey && cache.WriteIsolationKey != "" { h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey))