From 0b994ff1c3325cf78f278552da32cdb3cfb7e531 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 11:38:12 +0400 Subject: [PATCH] media store: a failed write gives its budget reservation back (V-584) Put and PutFile added the blob size to s.total before writing, and only the writeFile and os.Rename failure paths released it. A writeMeta failure in either, and a chmod failure on the spool in PutFile, kept the size, so a store that hit a full disk over-counted itself and could answer ErrStoreFull while the disk had room until the next Open re-measured. One defer per function now owns the release, disarmed on the success return, so a future early return cannot reintroduce the leak. Co-Authored-By: Claude Opus 5 --- internal/media/store.go | 26 +++++++++++--- internal/media/store_test.go | 67 ++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/internal/media/store.go b/internal/media/store.go index b931e9f..1d35b17 100644 --- a/internal/media/store.go +++ b/internal/media/store.go @@ -265,6 +265,16 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { } } + // Every return past the reservation has to give it back, so the defer owns + // that rather than each error path: a path that forgot over-counted the + // store until the next Open re-walked the directory. + stored := false + defer func() { + if fresh && !stored { + s.release(b.Size) + } + }() + // The sidecar goes first. Written second, a full disk or a crash between // the two left the bytes on disk with no sidecar, and List only sees // sidecars, so Prune could never collect them: Put returned an error and an @@ -274,11 +284,9 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { } if err := writeFile(blobPath, data); err != nil { _ = os.Remove(metaPath) - if fresh { - s.release(b.Size) - } return Blob{}, err } + stored = true return b, nil } @@ -331,12 +339,22 @@ func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { return Blob{}, err } } + // Same reasoning as Put: the reservation is released by one defer, not by + // whichever error path remembered to. + stored := false + defer func() { + if fresh && !stored { + s.release(b.Size) + } + }() + if err := writeMeta(metaPath, b); err != nil { return Blob{}, err } if !fresh { // Same bytes already here. Drop the spool copy. _ = os.Remove(src) + stored = true return b, nil } if err := os.Chmod(src, filePerm); err != nil { @@ -344,9 +362,9 @@ func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { } if err := os.Rename(src, blobPath); err != nil { _ = os.Remove(metaPath) - s.release(b.Size) return Blob{}, fmt.Errorf("media: move spool: %w", err) } + stored = true return b, nil } diff --git a/internal/media/store_test.go b/internal/media/store_test.go index fccb67f..0567822 100644 --- a/internal/media/store_test.go +++ b/internal/media/store_test.go @@ -287,6 +287,73 @@ func TestPutLeavesNothingWhenTheBytesCannotBeWritten(t *testing.T) { } } +// An over-counted store answers ErrStoreFull while the disk has room, and only +// the next Open corrects it. So every failed write has to give its reservation +// back, not just the one that remembered to. +func TestPutReleasesTheBudgetWhenTheSidecarCannotBeWritten(t *testing.T) { + s := testStore(t) + data := []byte("no sidecar for this") + blockSidecar(t, s, KindImage, data) + if _, err := s.Put(KindImage, "image/png", "web:upload", data); err == nil { + t.Fatal("put must fail") + } + if s.Total() != 0 { + t.Errorf("total = %d, want the failed put not counted", s.Total()) + } +} + +func TestPutFileReleasesTheBudgetWhenTheSidecarCannotBeWritten(t *testing.T) { + s := testStore(t) + data := []byte("no sidecar for this either") + blockSidecar(t, s, KindAudio, data) + src := filepath.Join(t.TempDir(), "capture.wav") + if err := os.WriteFile(src, data, 0o600); err != nil { + t.Fatal(err) + } + if _, err := s.PutFile(KindAudio, "audio/wav", "meeting", src); err == nil { + t.Fatal("put file must fail") + } + if s.Total() != 0 { + t.Errorf("total = %d, want the failed put not counted", s.Total()) + } +} + +// The chmod arm is PutFile's alone: Put never touches a spool file. +func TestPutFileReleasesTheBudgetWhenTheSpoolCannotBeChmodded(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can chmod a file it does not own") + } + // A symlink to a file owned by somebody else. Stat and the hash follow it + // and succeed; chmod follows it too and is refused. + src := filepath.Join(t.TempDir(), "capture.wav") + if err := os.Symlink("/etc/hosts", src); err != nil { + t.Fatal(err) + } + info, err := os.Stat(src) + if err != nil || info.Size() == 0 { + t.Skip("no readable /etc/hosts to point at") + } + s := testStore(t) + if _, err := s.PutFile(KindAudio, "audio/wav", "meeting", src); err == nil { + t.Fatal("put file must fail") + } + if s.Total() != 0 { + t.Errorf("total = %d, want the failed put not counted", s.Total()) + } +} + +// blockSidecar puts a directory where the sidecar for data has to go, so +// writeMeta fails while the blob path is still free. +func blockSidecar(t *testing.T, s *Store, kind Kind, data []byte) { + t.Helper() + sum := sha256.Sum256(data) + id := hex.EncodeToString(sum[:]) + bucket := filepath.Join(s.dir, string(kind), id[:2]) + if err := os.MkdirAll(filepath.Join(bucket, id+".json"), 0o700); err != nil { + t.Fatal(err) + } +} + // The per-blob cap bounds one call and nothing bounded their sum. 64 MiB per // call times unlimited calls inside a seven-day window fills the disk mavend's // database lives on. -- 2.52.0