From 37f634fd31a00534d0ee438bc640f07589afe98c Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 1 Sep 2025 13:37:57 +0200 Subject: [PATCH] fix: cache: call fatal() on errors that are not recoverable - responseFatalJSON(w, r, err) replaces responseJSON(w, r, 500, err) and calls fatal() when the following fail because they are not recoverable. There may be other non-recoverable errors but it is difficult to be 100% sure they cannot be engineered by the caller of the API for DoS purposes. - openDB - findCache - cache.Repo != repo - wrap errors in - openDB() - it was missing - readCache() - it was missing - useCache() - it was missing - findCache() - some had identical messages - in gc - replace logger.Warnf with h.fatal - differentiate errors that have identical messages - call fatal if openDB fails instead of returning --- act/artifactcache/handler.go | 66 +++++++++++++++++++------------ act/artifactcache/handler_test.go | 11 +++++- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 429b661c..a325d68b 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -177,7 +177,8 @@ func (h *handler) isClosed() bool { } func (h *handler) openDB() (*bolthold.Store, error) { - return bolthold.Open(filepath.Join(h.dir, "bolt.db"), 0o644, &bolthold.Options{ + file := filepath.Join(h.dir, "bolt.db") + db, err := bolthold.Open(file, 0o644, &bolthold.Options{ Encoder: json.Marshal, Decoder: json.Unmarshal, Options: &bbolt.Options{ @@ -186,6 +187,10 @@ func (h *handler) openDB() (*bolthold.Store, error) { FreelistType: bbolt.DefaultOptions.FreelistType, }, }) + if err != nil { + return nil, fmt.Errorf("Open(%s): %v", file, err) + } + return db, nil } // GET /_apis/artifactcache/cache @@ -206,21 +211,21 @@ func (h *handler) find(w http.ResponseWriter, r *http.Request, params httprouter db, err := h.openDB() if err != nil { - h.responseJSON(w, r, 500, err) + h.responseFatalJSON(w, r, err) return } defer db.Close() cache, err := findCache(db, repo, keys, version, rundata.WriteIsolationKey) if err != nil { - h.responseJSON(w, r, 500, err) + h.responseFatalJSON(w, r, err) return } // If read was scoped to WriteIsolationKey and didn't find anything, we can fallback to the non-isolated cache read if cache == nil && rundata.WriteIsolationKey != "" { cache, err = findCache(db, repo, keys, version, "") if err != nil { - h.responseJSON(w, r, 500, err) + h.responseFatalJSON(w, r, err) return } } @@ -265,7 +270,7 @@ func (h *handler) reserve(w http.ResponseWriter, r *http.Request, params httprou cache := api.ToCache() db, err := h.openDB() if err != nil { - h.responseJSON(w, r, 500, err) + h.responseFatalJSON(w, r, err) return } defer db.Close() @@ -305,13 +310,13 @@ func (h *handler) upload(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) + h.responseFatalJSON(w, r, fmt.Errorf("cache Get: %w", err)) return } - // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) + // 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 { @@ -360,13 +365,13 @@ func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) + h.responseFatalJSON(w, r, fmt.Errorf("cache Get: %w", err)) return } - // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) + // 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 { @@ -381,7 +386,7 @@ func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprout size, err := h.storage.Commit(cache.ID, cache.Size) if err != nil { - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("commit(%v): %w", cache.ID, err)) return } // write real size back to cache, it may be different from the current value when the request doesn't specify it. @@ -389,7 +394,7 @@ func (h *handler) commit(w http.ResponseWriter, r *http.Request, params httprout db, err := h.openDB() if err != nil { - h.responseJSON(w, r, 500, err) + h.responseFatalJSON(w, r, err) return } defer db.Close() @@ -424,13 +429,13 @@ func (h *handler) get(w http.ResponseWriter, r *http.Request, params httprouter. h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) + h.responseFatalJSON(w, r, fmt.Errorf("cache Get: %w", err)) return } - // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) + // 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 @@ -481,7 +486,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version, writeIso And("Complete").Eq(true). SortBy("CreatedAt").Reverse()); err == nil || !errors.Is(err, bolthold.ErrNotFound) { if err != nil { - return nil, fmt.Errorf("find cache: %w", err) + return nil, fmt.Errorf("find cache entry equal to %s: %w", prefix, err) } return cache, nil } @@ -500,7 +505,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version, writeIso if errors.Is(err, bolthold.ErrNotFound) { continue } - return nil, fmt.Errorf("find cache: %w", err) + return nil, fmt.Errorf("find cache entry starting with %s: %w", prefix, err) } return cache, nil } @@ -526,7 +531,7 @@ func (h *handler) readCache(id uint64) (*Cache, error) { defer db.Close() cache := &Cache{} if err := db.Get(id, cache); err != nil { - return nil, err + return nil, fmt.Errorf("readCache: Get(%v): %w", id, err) } return cache, nil } @@ -539,10 +544,13 @@ func (h *handler) useCache(id uint64) error { defer db.Close() cache := &Cache{} if err := db.Get(id, cache); err != nil { - return err + return fmt.Errorf("useCache: Get(%v): %w", id, err) } cache.UsedAt = time.Now().Unix() - return db.Update(cache.ID, cache) + if err := db.Update(cache.ID, cache); err != nil { + return fmt.Errorf("useCache: Update(%v): %v", cache.ID, err) + } + return nil } const ( @@ -574,6 +582,7 @@ func (h *handler) gcCache() { db, err := h.openDB() if err != nil { + fatal(h.logger, err) return } defer db.Close() @@ -584,12 +593,12 @@ func (h *handler) gcCache() { Where("UsedAt").Lt(time.Now().Add(-keepTemp).Unix()). And("Complete").Eq(false), ); err != nil { - h.logger.Warnf("find caches: %v", err) + fatal(h.logger, fmt.Errorf("gc caches not completed: %v", err)) } else { for _, cache := range caches { h.storage.Remove(cache.ID) if err := db.Delete(cache.ID, cache); err != nil { - h.logger.Warnf("delete cache: %v", err) + h.logger.Errorf("delete cache: %v", err) continue } h.logger.Infof("deleted cache: %+v", cache) @@ -601,7 +610,7 @@ func (h *handler) gcCache() { if err := db.Find(&caches, bolthold. Where("UsedAt").Lt(time.Now().Add(-keepUnused).Unix()), ); err != nil { - h.logger.Warnf("find caches: %v", err) + fatal(h.logger, fmt.Errorf("gc caches old not used: %v", err)) } else { for _, cache := range caches { h.storage.Remove(cache.ID) @@ -618,7 +627,7 @@ func (h *handler) gcCache() { if err := db.Find(&caches, bolthold. Where("CreatedAt").Lt(time.Now().Add(-keepUsed).Unix()), ); err != nil { - h.logger.Warnf("find caches: %v", err) + fatal(h.logger, fmt.Errorf("gc caches too old: %v", err)) } else { for _, cache := range caches { h.storage.Remove(cache.ID) @@ -637,7 +646,7 @@ func (h *handler) gcCache() { bolthold.Where("Complete").Eq(true), "Key", "Version", ); err != nil { - h.logger.Warnf("find aggregate caches: %v", err) + fatal(h.logger, fmt.Errorf("gc aggregate caches: %v", err)) } else { for _, result := range results { if result.Count() <= 1 { @@ -663,6 +672,11 @@ func (h *handler) gcCache() { } } +func (h *handler) responseFatalJSON(w http.ResponseWriter, r *http.Request, err error) { + h.responseJSON(w, r, 500, err) + fatal(h.logger, err) +} + func (h *handler) responseJSON(w http.ResponseWriter, r *http.Request, code int, v ...any) { w.Header().Set("Content-Type", "application/json; charset=utf-8") var data []byte diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 5970ea1a..0c7160d6 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -16,10 +16,13 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" + "code.forgejo.org/forgejo/runner/v9/testutils" + "github.com/sirupsen/logrus" "github.com/timshannon/bolthold" "go.etcd.io/bbolt" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -59,6 +62,10 @@ var ( ) func TestHandler(t *testing.T) { + defer testutils.MockVariable(&fatal, func(_ logrus.FieldLogger, err error) { + t.Fatalf("unexpected call to fatal(%v)", err) + })() + dir := filepath.Join(t.TempDir(), "artifactcache") handler, err := StartHandler(dir, "", 0, "secret", nil) require.NoError(t, err)