From 3e1b4b42c209a9a2af8b398fb6ffecf760eed372 Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 4 Nov 2026 16:38:21 +0000 Subject: [PATCH] fix(cache): stop the cache server from running its volume out of space (#1377) Committing a cache upload copied every part into a new archive before deleting the parts, so a 5 GB save needed 10 GB free. Failed writes and assemblies left their partial bytes until the next stale sweep, up to an hour later, and every other save on the runner failed meanwhile. Nothing made room for an upload, so once the cache had filled its volume down to the free space floor, larger saves kept failing. - Assemble in place, deleting each part once appended. - Drop partial parts and archives on failure. - Before a part would cross the floor, evict the least recently used entries. - Reject uploads to reservations the sweep already deleted, which left files nothing removed. _Written by Claude (Opus 5.5)._ Reviewed-on: https://gitea.com/gitea/runner/pulls/1377 Reviewed-by: bircni Co-authored-by: silverwind --- act/artifactcache/handler.go | 27 ++++++------- act/artifactcache/handler_test.go | 24 +++++++----- act/artifactcache/handler_v2.go | 1 + act/artifactcache/handler_v2_test.go | 3 ++ act/artifactcache/storage.go | 52 ++++++++++++------------- internal/pkg/config/config.example.yaml | 4 +- 6 files changed, 60 insertions(+), 51 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 4c03f54a..2c6bdacc 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -124,7 +124,7 @@ type Handler struct { policy Policy - // freeDisk is a field so tests can drive evictForFreeSpace without a full volume. + // freeDisk is a field so tests can drive makeRoom without a full volume. freeDisk func(string) (uint64, error) } @@ -773,6 +773,7 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request) { h.responseJSON(w, r, 400, err) return } + h.makeRoom(r.ContentLength) if err := h.storage.Write(cache.ID, start, r.Body); err != nil { h.responseJSON(w, r, 500, err) return @@ -875,11 +876,8 @@ func (h *Handler) commitCache(cache *Cache) error { if h.s3 != nil { return nil // lifecycle rules on the bucket bound its size } - // A commit is the only thing that grows the store, so the only thing that can push the - // volume under the floor. h.evictRepo(db, cache.Repo) h.evictTotal(db) - h.evictForFreeSpace(db) return nil } @@ -1173,9 +1171,6 @@ func (h *Handler) touchCache(id uint64, requireIncomplete bool) error { defer db.Close() cache, err := db.get(id) - if errors.Is(err, errCacheNotFound) { - return nil - } if err != nil { return err } @@ -1260,23 +1255,29 @@ func (h *Handler) gcCache() { h.evictExpired(db) h.evictSuperseded(db) h.evictOversized(db) - h.evictForFreeSpace(db) + _ = db.Close() + h.makeRoom(0) } -// evictForFreeSpace bounds the volume itself, so it also covers bytes the cache never -// accounted for. -func (h *Handler) evictForFreeSpace(db *cacheDB) { +// makeRoom evicts the least recently used entries to keep need bytes above the floor, as GitHub saves new over old. +func (h *Handler) makeRoom(need int64) { free, err := h.freeDisk(h.dir) if err != nil { h.logger.Debugf("free disk check: %v", err) // unsupported platform, treat as unavailable rather than full return } - if free >= uint64(h.policy.MinFreeDisk) { + shortfall := h.policy.MinFreeDisk + max(need, 0) - int64(free) + if h.s3 != nil || shortfall <= 0 { return } + db, err := h.openDB() + if err != nil { + return + } + defer db.Close() caches := h.completedByUse(db) - total, shortfall := totalSize(caches), h.policy.MinFreeDisk-int64(free) + total := totalSize(caches) if total <= shortfall { // Say so, or shedding everything and still being short reads as the backstop working. h.logger.Warnf("cache volume is %d MiB short of the free space floor with only %d MiB of cache on it; something else is filling it", shortfall/miB, total/miB) diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 0f990b01..4a560eaa 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -18,6 +18,7 @@ import ( "strconv" "strings" "testing" + "testing/iotest" "testing/synctest" "time" @@ -673,7 +674,7 @@ func TestHandler_evictPolicy(t *testing.T) { } } -func TestHandler_evictForFreeSpace(t *testing.T) { +func TestHandler_makeRoom(t *testing.T) { t.Parallel() free := func(n int64) func(string) (uint64, error) { return func(string) (uint64, error) { return uint64(n), nil } @@ -682,14 +683,16 @@ func TestHandler_evictForFreeSpace(t *testing.T) { for _, tc := range []struct { name string freeDisk func(string) (uint64, error) + upload int64 kept []string }{ - {"ample free space evicts nothing", free(defaultMinFreeDisk), []string{"oldest", "middle", "newest"}}, - {"a small shortfall sheds one entry", free(defaultMinFreeDisk - 4*miB), []string{"middle", "newest"}}, - {"a shortfall the cache cannot cover sheds all of it", free(0), nil}, + {"ample free space evicts nothing", free(defaultMinFreeDisk), 0, []string{"oldest", "middle", "newest"}}, + {"room for an upload sheds the oldest entry", free(defaultMinFreeDisk), 4 * miB, []string{"middle", "newest"}}, + {"a shortfall the cache cannot cover sheds all of it", free(0), 0, nil}, { "an unreadable volume is treated as unavailable, not as full", func(string) (uint64, error) { return 0, errors.New("unsupported") }, + 0, []string{"oldest", "middle", "newest"}, }, } { @@ -702,11 +705,7 @@ func TestHandler_evictForFreeSpace(t *testing.T) { } handler := newTestHandler(t, Policy{}, entries...) handler.freeDisk = tc.freeDisk - - db, err := handler.openDB() - require.NoError(t, err) - handler.evictForFreeSpace(db) - require.NoError(t, db.Close()) + handler.makeRoom(tc.upload) assert.ElementsMatch(t, tc.kept, keptKeys(t, handler, entries)) }) @@ -888,6 +887,13 @@ func keptKeys(t *testing.T, handler *Handler, entries []*Cache) []string { return kept } +func TestStorageDropsFailedWrites(t *testing.T) { + storage, err := NewStorage(t.TempDir()) + require.NoError(t, err) + require.Error(t, storage.Write(1, 0, io.MultiReader(strings.NewReader("partial"), iotest.ErrReader(io.ErrUnexpectedEOF)))) + assert.NoFileExists(t, storage.tempName(1, 0)) +} + func writeBlob(t *testing.T, handler *Handler, id uint64) { t.Helper() require.NoError(t, handler.storage.Write(id, 0, strings.NewReader("a"))) diff --git a/act/artifactcache/handler_v2.go b/act/artifactcache/handler_v2.go index 07b5b5cb..b7ef6a32 100644 --- a/act/artifactcache/handler_v2.go +++ b/act/artifactcache/handler_v2.go @@ -218,6 +218,7 @@ func (h *Handler) v2UploadBlob(w http.ResponseWriter, r *http.Request) { h.responseJSON(w, r, http.StatusBadRequest, err) return } + h.makeRoom(r.ContentLength) query := r.URL.Query() switch strings.ToLower(query.Get("comp")) { diff --git a/act/artifactcache/handler_v2_test.go b/act/artifactcache/handler_v2_test.go index 06e809c1..2c2b76db 100644 --- a/act/artifactcache/handler_v2_test.go +++ b/act/artifactcache/handler_v2_test.go @@ -14,6 +14,7 @@ import ( "strconv" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -84,6 +85,8 @@ func TestCacheServiceV2RoundTrip(t *testing.T) { unsigned := fmt.Sprintf("%s%s/1", handler.ExternalURL(), blobPath) assert.Equal(t, http.StatusUnauthorized, putBlob(t, unsigned, content)) + unreserved := handler.signedURL(JobCredential{Repo: testRepo}, blobPath, blobUploadPurpose, 1, time.Now().Add(time.Minute)) + assert.Equal(t, http.StatusBadRequest, putBlob(t, unreserved, content)) finalized, uploadURL := saveV2(t, handler, "deps-v1", "abc123", content) require.Equal(t, true, finalized["ok"]) diff --git a/act/artifactcache/storage.go b/act/artifactcache/storage.go index e9f7795a..5eb6d823 100644 --- a/act/artifactcache/storage.go +++ b/act/artifactcache/storage.go @@ -9,6 +9,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json/v2" + "errors" "fmt" "io" "net/http" @@ -51,9 +52,10 @@ func (s *Storage) writeFile(name string, reader io.Reader) error { if err != nil { return err } - defer file.Close() - _, err = io.Copy(file, reader) + if err = errors.Join(err, file.Close()); err != nil { + _ = os.Remove(name) + } return err } @@ -102,50 +104,46 @@ func (s *Storage) Commit(id uint64, size int64) (int64, error) { return 0, err } written, err := assemble(name, tempNames) - if err != nil { - return 0, err + if err == nil && size >= 0 && written != size { // old clients like actions/cache@v2 send no size + err = fmt.Errorf("broken file: %v != %v", written, size) } - // If size is less than 0, it means the size is unknown. - // We can't check the size of the file, just skip the check. - // It happens when the request comes from old versions of actions, like `actions/cache@v2`. - if size >= 0 && written != size { + if err != nil { _ = os.Remove(name) - return 0, fmt.Errorf("broken file: %v != %v", written, size) + return 0, err } return written, nil } -// assemble concatenates the uploaded parts into name. A single part, which is what the v2 API -// produces below the client's block threshold, is already the whole archive and is moved. +// assemble appends the parts into the first, deleting each once copied, so it never needs twice the space. func assemble(name string, tempNames []string) (int64, error) { - if len(tempNames) == 1 { - info, err := os.Stat(tempNames[0]) - if err != nil { - return 0, err - } - return info.Size(), os.Rename(tempNames[0], name) + if len(tempNames) == 0 { + return 0, errors.New("nothing was uploaded") } - - file, err := os.Create(name) + if err := os.Rename(tempNames[0], name); err != nil { + return 0, err + } + file, err := os.OpenFile(name, os.O_WRONLY|os.O_APPEND, 0) if err != nil { return 0, err } defer file.Close() - - var written int64 - for _, v := range tempNames { - f, err := os.Open(v) + for _, v := range tempNames[1:] { + part, err := os.Open(v) if err != nil { return 0, err } - n, err := io.Copy(file, f) - _ = f.Close() + _, err = io.Copy(file, part) + _ = part.Close() if err != nil { return 0, err } - written += n + _ = os.Remove(v) } - return written, nil + info, err := file.Stat() + if err != nil { + return 0, err + } + return info.Size(), file.Close() } func (s *Storage) Serve(w http.ResponseWriter, r *http.Request, id uint64) { diff --git a/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index 9313b7c7..52703a71 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -206,8 +206,8 @@ cache: # How the cache server discards entries, ignored when external_server or s3 is set since # those apply their own. Leave a setting out for its default; 0s or 0 turns the three # limits off. Sizes accept 10GB, 512mb, 1TiB or a plain byte count, binary either way. - # Whatever these allow, the cache still sheds entries to keep free space on its volume - # above health_check.min_free_disk_space_mb when health checks are enabled. + # Whatever these allow, the cache still sheds entries to keep free space on its volume, room + # for uploads included, above health_check.min_free_disk_space_mb when health checks are enabled. # Remove entries nothing has read or written within this window. Only last access counts. #retention: 168h # Cap one repository, removing its least recently accessed entries until it fits. An entry