From 187e1df52c707665190c9d6826a1dccde8a5f309 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 23 Mar 2025 22:31:16 +0100 Subject: [PATCH 1/3] fix: reduce the time during which the database stays open * During get/upload, close the database while reading/writing so it does not stay open for longer than necessary. This may be helpful when uploads run in parallel. * Be more informative when returning error 500 * Make useCache handle errors * Return 500 immediately when writing the cache fails instead of falling through to 200 Refs: https://code.forgejo.org/forgejo/runner/issues/509 --- act/artifactcache/handler.go | 46 ++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 2da6febd..72d480e8 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -263,7 +263,7 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout cache := &Cache{} db, err := h.openDB() if err != nil { - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) return } defer db.Close() @@ -272,13 +272,13 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 400, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) return } // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, ErrValidation) + h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } @@ -286,16 +286,20 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key)) return } - defer db.Close() + db.Close() start, _, err := parseContentRange(r.Header.Get("Content-Range")) if err != nil { - h.responseJSON(w, r, 400, err) + h.responseJSON(w, r, 400, fmt.Errorf("cache parseContentRange(%s): %w", r.Header.Get("Content-Range"), err)) return } if err := h.storage.Write(cache.ID, start, r.Body); err != nil { - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache storage.Write: %w", err)) + return + } + if err := h.useCache(id); err != nil { + h.responseJSON(w, r, 500, fmt.Errorf("cache useCache: %w", err)) + return } - h.useCache(db, cache) h.responseJSON(w, r, 200) } @@ -332,7 +336,7 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, ErrValidation) + h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } @@ -385,26 +389,29 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter. cache := &Cache{} db, err := h.openDB() if err != nil { - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) return } - defer db.Close() + db.Close() if err := db.Get(id, cache); err != nil { if errors.Is(err, bolthold.ErrNotFound) { h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) return } // Should not happen if cache.Repo != repo { - h.responseJSON(w, r, 500, ErrValidation) + h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } - h.useCache(db, cache) + if err := h.useCache(id); err != nil { + h.responseJSON(w, r, 500, fmt.Errorf("cache useCache: %w", err)) + return + } h.storage.Serve(w, r, id) } @@ -478,9 +485,18 @@ func insertCache(db *bolthold.Store, cache *Cache) error { return nil } -func (h *Handler) useCache(db *bolthold.Store, cache *Cache) { +func (h *Handler) useCache(id uint64) error { + db, err := h.openDB() + if err != nil { + return err + } + defer db.Close() + cache := &Cache{} + if err := db.Get(id, cache); err != nil { + return err + } cache.UsedAt = time.Now().Unix() - _ = db.Update(cache.ID, cache) + return db.Update(cache.ID, cache) } const ( From 639b83c26c01352b8441ffb2c4597a1eaec8cbcb Mon Sep 17 00:00:00 2001 From: Kwonunn Date: Mon, 24 Mar 2025 10:17:04 +0100 Subject: [PATCH 2/3] fix: do not immediately close the db after opening it --- act/artifactcache/handler.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 72d480e8..c068beca 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -387,19 +387,21 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter. } cache := &Cache{} - db, err := h.openDB() - if err != nil { - h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) - return - } - db.Close() - if err := db.Get(id, cache); err != nil { - if errors.Is(err, bolthold.ErrNotFound) { - h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) + { + db, err := h.openDB() + if err != nil { + h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) + return + } + defer db.Close() + if err := db.Get(id, cache); err != nil { + if errors.Is(err, bolthold.ErrNotFound) { + h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) + return + } + h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) return } - h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) - return } // Should not happen From 835a9d2068bdd1a622d88ebe514d7b81f2346e64 Mon Sep 17 00:00:00 2001 From: Kwonunn Date: Mon, 24 Mar 2025 10:48:28 +0100 Subject: [PATCH 3/3] fix: move reading cache to separate function --- act/artifactcache/handler.go | 60 +++++++++++++++--------------------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index c068beca..c48bb1a9 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -260,16 +260,10 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout return } - cache := &Cache{} - db, err := h.openDB() + cache, err := h.readCache(id) if err != nil { - h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) - return - } - defer db.Close() - if err := db.Get(id, cache); err != nil { if errors.Is(err, bolthold.ErrNotFound) { - h.responseJSON(w, r, 400, fmt.Errorf("cache %d: not reserved", id)) + h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) @@ -286,7 +280,6 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key)) return } - db.Close() start, _, err := parseContentRange(r.Header.Get("Content-Range")) if err != nil { h.responseJSON(w, r, 400, fmt.Errorf("cache parseContentRange(%s): %w", r.Header.Get("Content-Range"), err)) @@ -318,19 +311,13 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout return } - cache := &Cache{} - db, err := h.openDB() + cache, err := h.readCache(id) if err != nil { - h.responseJSON(w, r, 500, err) - return - } - defer db.Close() - if err := db.Get(id, cache); err != nil { if errors.Is(err, bolthold.ErrNotFound) { - h.responseJSON(w, r, 400, fmt.Errorf("cache %d: not reserved", id)) + h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } - h.responseJSON(w, r, 500, err) + h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) return } @@ -345,8 +332,6 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout return } - db.Close() - size, err := h.storage.Commit(cache.ID, cache.Size) if err != nil { h.responseJSON(w, r, 500, err) @@ -355,7 +340,7 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout // write real size back to cache, it may be different from the current value when the request doesn't specify it. cache.Size = size - db, err = h.openDB() + db, err := h.openDB() if err != nil { h.responseJSON(w, r, 500, err) return @@ -386,22 +371,14 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter. return } - cache := &Cache{} - { - db, err := h.openDB() - if err != nil { - h.responseJSON(w, r, 500, fmt.Errorf("cache openDB: %w", err)) - return - } - defer db.Close() - if err := db.Get(id, cache); err != nil { - if errors.Is(err, bolthold.ErrNotFound) { - h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) - return - } - h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) + cache, err := h.readCache(id) + if err != nil { + if errors.Is(err, bolthold.ErrNotFound) { + h.responseJSON(w, r, 404, fmt.Errorf("cache %d: not reserved", id)) return } + h.responseJSON(w, r, 500, fmt.Errorf("cache Get: %w", err)) + return } // Should not happen @@ -487,6 +464,19 @@ func insertCache(db *bolthold.Store, cache *Cache) error { return nil } +func (h *Handler) readCache(id uint64) (*Cache, error) { + db, err := h.openDB() + if err != nil { + return nil, err + } + defer db.Close() + cache := &Cache{} + if err := db.Get(id, cache); err != nil { + return nil, err + } + return cache, nil +} + func (h *Handler) useCache(id uint64) error { db, err := h.openDB() if err != nil {