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