fix(artwork): rewrite vanished duplicates and sweep stale mime variants

Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed.
This commit is contained in:
Deluan 2026-07-22 00:28:51 -04:00
parent bf614e66ad
commit 8147f7c40b
8 changed files with 99 additions and 31 deletions

View File

@ -56,8 +56,10 @@ func (s *ImageStore) Write(hash, mimeType string, r io.Reader) error {
if _, err := os.Stat(dst); err == nil {
// A touched mtime marks the file live so a concurrent prune spares it.
now := time.Now()
_ = os.Chtimes(dst, now, now)
return nil
if err := os.Chtimes(dst, now, now); err == nil {
return nil
}
// touch failed (file likely pruned concurrently) — fall through and write it
}
if err := os.MkdirAll(filepath.Dir(dst), 0755); err != nil {
return err
@ -104,7 +106,7 @@ func (s *ImageStore) Remove(hash, mimeType string, olderThan time.Time) error {
// Sweep removes store files not accepted by keep. Files modified after cutoff
// (including temp files) are always kept: their acquisition row may not be committed yet.
func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash string) bool) (int, error) {
func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash, ext string) bool) (int, error) {
removed := 0
err := filepath.WalkDir(s.root, func(path string, d fs.DirEntry, err error) error {
if err != nil || d.IsDir() {
@ -120,7 +122,8 @@ func (s *ImageStore) Sweep(cutoff time.Time, keep func(hash string) bool) (int,
name := d.Name()
remove := strings.HasPrefix(name, ".") // abandoned temp file past the grace window
if !remove {
remove = !keep(strings.TrimSuffix(name, filepath.Ext(name)))
ext := filepath.Ext(name)
remove = !keep(strings.TrimSuffix(name, ext), ext)
}
if remove {
// #nosec G122 -- path comes from WalkDir over our own store root, no attacker-controlled symlinks

View File

@ -65,6 +65,22 @@ var _ = Describe("ImageStore", func() {
Expect(info.ModTime()).To(BeTemporally(">", time.Now().Add(-time.Minute)))
})
It("rewrites the bytes when the existing file vanished before the liveness touch", func() {
data := []byte("vanishing")
h, _ := HashImage(bytes.NewReader(data))
for range 10 {
Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
Expect(os.Remove(store.path(h, "image/png"))).To(Succeed())
Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
rc, err := store.Open(h, "image/png")
Expect(err).ToNot(HaveOccurred())
got, _ := io.ReadAll(rc)
rc.Close()
Expect(got).To(Equal(data))
}
})
It("returns fs.ErrNotExist for missing images", func() {
_, err := store.Open("beefbeefbeefbeef", "image/jpeg")
Expect(os.IsNotExist(err)).To(BeTrue())
@ -107,7 +123,7 @@ var _ = Describe("ImageStore", func() {
old := time.Now().Add(-2 * time.Hour)
Expect(os.Chtimes(store.path(h2, "image/jpeg"), old, old)).To(Succeed())
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(h string) bool { return h == h1 })
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(h, _ string) bool { return h == h1 })
Expect(err).ToNot(HaveOccurred())
Expect(removed).To(Equal(1))
_, err = store.Open(h2, "image/jpeg")
@ -117,12 +133,34 @@ var _ = Describe("ImageStore", func() {
rc.Close()
})
It("sweeps a stale mime variant of a known hash, keeps the current one", func() {
data := []byte("same-bytes")
h, _ := HashImage(bytes.NewReader(data))
Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed())
old := time.Now().Add(-2 * time.Hour)
Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed())
Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed())
// The recorded mime is image/jpeg, so the .png variant is obsolete.
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(hash, ext string) bool {
return hash == h && ext == ".jpg"
})
Expect(err).ToNot(HaveOccurred())
Expect(removed).To(Equal(1))
_, err = store.Open(h, "image/png")
Expect(os.IsNotExist(err)).To(BeTrue())
rc, err := store.Open(h, "image/jpeg")
Expect(err).ToNot(HaveOccurred())
rc.Close()
})
It("keeps young unknown files inside the grace window", func() {
d := []byte("fresh-orphan")
h, _ := HashImage(bytes.NewReader(d))
Expect(store.Write(h, "image/jpeg", bytes.NewReader(d))).To(Succeed())
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string) bool { return false })
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return false })
Expect(err).ToNot(HaveOccurred())
Expect(removed).To(Equal(0))
rc, err := store.Open(h, "image/jpeg")
@ -139,7 +177,7 @@ var _ = Describe("ImageStore", func() {
freshTmp := filepath.Join(root, ".fresh.tmp")
Expect(os.WriteFile(freshTmp, []byte("y"), 0600)).To(Succeed())
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string) bool { return true })
removed, err := store.Sweep(time.Now().Add(-time.Hour), func(string, string) bool { return true })
Expect(err).ToNot(HaveOccurred())
Expect(removed).To(Equal(1))
Expect(oldTmp).ToNot(BeAnExistingFile())

View File

@ -49,17 +49,14 @@ func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error {
log.Info(ctx, "Prune: removed orphan artwork", "count", removed)
}
hashes, err := repo.GetAllHashes()
mimes, err := repo.GetAllMimes()
if err != nil {
return err
}
known := make(map[string]struct{}, len(hashes))
for _, h := range hashes {
known[h] = struct{}{}
}
removed, err := store.Sweep(cutoff, func(hash string) bool {
_, ok := known[hash]
return ok
removed, err := store.Sweep(cutoff, func(hash, ext string) bool {
// A known hash under a stale extension is a superseded mime variant — reclaim it.
m, ok := mimes[hash]
return ok && ext == extForMime(m)
})
if err != nil {
return err

View File

@ -17,7 +17,7 @@ type flakyGetArtworkRepo struct {
*tests.MockArtworkRepo
}
func (f *flakyGetArtworkRepo) GetAllHashes() ([]string, error) {
func (f *flakyGetArtworkRepo) GetAllMimes() (map[string]string, error) {
return nil, errors.New("db locked")
}
@ -108,6 +108,26 @@ var _ = Describe("Prune", func() {
Expect(os.IsNotExist(err)).To(BeTrue())
})
It("sweeps an obsolete mime variant of a reacquired hash", func() {
data := []byte("variant-bytes")
h, _ := HashImage(bytes.NewReader(data))
Expect(store.Write(h, "image/png", bytes.NewReader(data))).To(Succeed())
Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed())
old := time.Now().Add(-2 * time.Hour)
Expect(os.Chtimes(store.path(h, "image/png"), old, old)).To(Succeed())
Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed())
// The row records the current mime; the .png file is a superseded variant.
Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed())
Expect(Prune(context.Background(), ds, store)).To(Succeed())
_, err := store.Open(h, "image/png")
Expect(os.IsNotExist(err)).To(BeTrue())
rc, err := store.Open(h, "image/jpeg")
Expect(err).ToNot(HaveOccurred())
rc.Close()
})
It("never sweeps files on a transient DB error", func() {
ds.MockedArtwork = &flakyGetArtworkRepo{MockArtworkRepo: tests.CreateMockArtworkRepo()}

View File

@ -73,8 +73,8 @@ type ArtworkRepository interface {
DeleteForItem(kind, id string) error
// GetInfoForItems hydrates a page: one batched query, item_artwork joined to artwork.
GetInfoForItems(kind string, ids []string) (map[string]ItemArtworkInfo, error)
// GetAllHashes returns every stored artwork hash, for sweep membership checks.
GetAllHashes() ([]string, error)
// GetAllMimes returns hash -> current mime for every stored artwork, for sweep retention checks.
GetAllMimes() (map[string]string, error)
}
type ArtworkQueueRepository interface {

View File

@ -74,11 +74,20 @@ func (r *artworkRepository) GetImages(hashes []string) (map[string]model.Artwork
return res, nil
}
func (r *artworkRepository) GetAllHashes() ([]string, error) {
sel := Select("hash").From(r.tableName)
var hashes []string
err := r.queryAllSlice(sel, &hashes)
return hashes, err
func (r *artworkRepository) GetAllMimes() (map[string]string, error) {
sel := Select("hash", "mime").From(r.tableName)
var rows []struct {
Hash string
Mime string
}
if err := r.queryAll(sel, &rows); err != nil {
return nil, err
}
res := make(map[string]string, len(rows))
for _, row := range rows {
res[row.Hash] = row.Mime
}
return res, nil
}
func (r *artworkRepository) GetOrphanHashes(createdBefore time.Time) ([]string, error) {

View File

@ -76,12 +76,13 @@ var _ = Describe("ArtworkRepository", func() {
Expect(got["b2"].Mime).To(Equal("image/png"))
})
It("returns every stored hash", func() {
It("returns every stored hash with its current mime", func() {
Expect(repo.PutImage(&model.Artwork{Hash: "all1", Mime: "image/jpeg"})).To(Succeed())
Expect(repo.PutImage(&model.Artwork{Hash: "all2", Mime: "image/png"})).To(Succeed())
hashes, err := repo.GetAllHashes()
mimes, err := repo.GetAllMimes()
Expect(err).ToNot(HaveOccurred())
Expect(hashes).To(ConsistOf("all1", "all2"))
Expect(mimes).To(HaveKeyWithValue("all1", "image/jpeg"))
Expect(mimes).To(HaveKeyWithValue("all2", "image/png"))
})
It("finds orphans older than cutoff, honoring item_artwork references", func() {

View File

@ -58,15 +58,15 @@ func (m *MockArtworkRepo) GetOrphanHashes(createdBefore time.Time) ([]string, er
return m.OrphanHashes, m.Err
}
func (m *MockArtworkRepo) GetAllHashes() ([]string, error) {
func (m *MockArtworkRepo) GetAllMimes() (map[string]string, error) {
if m.Err != nil {
return nil, m.Err
}
hashes := make([]string, 0, len(m.Data))
for h := range m.Data {
hashes = append(hashes, h)
mimes := make(map[string]string, len(m.Data))
for h, a := range m.Data {
mimes[h] = a.Mime
}
return hashes, nil
return mimes, nil
}
func (m *MockArtworkRepo) DeleteOrphans(createdBefore time.Time, hashes []string) error {