cleanup images from deleted comments

Previously, images were deleted only from comments deleted
before EditDuration expiration. After this change, any deletion
of the comment deletes images if they are not used elsewhere
in comments under the same page.

backend/_example/memory_store/accessor/image.go

@@ -70,6 +70,17 @@ func (m *MemImage) Load(id string) ([]byte, error) {
 	return img, nil
 }
 
+// Delete image by ID
+func (m *MemImage) Delete(id string) error {
+	m.mu.Lock()
+	// delete key from permanent and staging storage
+	delete(m.images, id)
+	delete(m.insertTime, id)
+	delete(m.imagesStaging, id)
+	m.mu.Unlock()
+	return nil
+}
+
 // Commit moves image from staging to permanent
 func (m *MemImage) Commit(id string) error {
 	m.mu.RLock()

backend/_example/memory_store/accessor/image_test.go

@@ -18,7 +18,7 @@ import (
 )
 
 // gopher png for test, from https://golang.org/src/image/png/example_test.go
-const gopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwUVRzHf2" +
+const rawGopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwUVRzHf2" +
 	"+OPbo9d7tsWyiyaZti6eWGAhISoIGKECEKCAiJJkYTiUgTMYSIosYYBBIUIxoSPIINEBDi2VhwkQrVsj1ESgu9doHWdrul7ba" +
 	"73WNm3vOPtsseM9MdwvvrzTs+8/t95ze/33sI5BqiabU6m9En8oNjduLnAEDLUsQXFF8tQ5oxK3vmnNmDSMtrncks9Hhtt" +
 	"/qeWZapHb1ha3UqYSWVl2ZmpWgaXMXGohQAvmeop3bjTRtv6SgaK/Pb9/bFzUrYslbFAmHPp+3WhAYdr+7GN/YnpN46Opv55VDs" +
@@ -38,7 +38,9 @@ const gopher = "iVBORw0KGgoAAAANSUhEUgAAAEsAAAA8CAAAAAALAhhPAAAFfUlEQVRYw62XeWwU
 	"1y98c3D27eppUjsZ6fql3jcd5rUe7+ZIlLNQny3Rd+E5Tct3WVhTM5RBCEdiEK0b6B+/ca2gYU393nFj/n1AygRQxPIUA043M42u85+z2S" +
 	"nssKrPl8Mx76NL3E6eXc3be7OD+H4WHbJkKI8AU8irbITQjZ+0hQcPEgId/Fn/pl9crKH02+5o2b9T/eMx7pKoskYgAAAABJRU5ErkJggg=="
 
-func gopherPNG() io.Reader { return base64.NewDecoder(base64.StdEncoding, strings.NewReader(gopher)) }
+func gopherPNG() io.Reader {
+	return base64.NewDecoder(base64.StdEncoding, strings.NewReader(rawGopher))
+}
 
 func TestMemImage_LoadAfterSave(t *testing.T) {
 	svc := NewMemImageStore()
@@ -57,7 +59,8 @@ func TestMemImage_LoadAfterSave(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, gopher, img)
 
-	svc.ResetCleanupTimer(id)
+	err = svc.ResetCleanupTimer(id)
+	assert.NoError(t, err)
 
 	err = svc.Commit(id)
 	assert.NoError(t, err)
@@ -70,6 +73,26 @@ func TestMemImage_LoadAfterSave(t *testing.T) {
 	assert.Equal(t, gopher, img)
 }
 
+func TestMemImage_LoadAfterDelete(t *testing.T) {
+	svc := NewMemImageStore()
+	gopher, err := io.ReadAll(gopherPNG())
+	assert.NoError(t, err)
+
+	id := "test_img"
+	err = svc.Save(id, gopher)
+	assert.NoError(t, err)
+
+	err = svc.Delete(id)
+	assert.NoError(t, err)
+
+	img, err := svc.Load(id)
+	assert.EqualError(t, err, "image test_img not found")
+	assert.Empty(t, img)
+
+	err = svc.ResetCleanupTimer(id)
+	assert.EqualError(t, err, "image test_img not found")
+}
+
 func TestMemImage_CommitFail(t *testing.T) {
 	svc := NewMemImageStore()
 	err := svc.Commit("test_id")

backend/_example/memory_store/server/image.go

@@ -35,7 +35,6 @@ func (s *RPC) imgResetClnTimerHndl(id uint64, params json.RawMessage) (rr jrpc.R
 	}
 	err := s.img.ResetCleanupTimer(fileID)
 	return jrpc.EncodeResponse(id, nil, err)
-
 }
 
 func (s *RPC) imgLoadHndl(id uint64, params json.RawMessage) (rr jrpc.Response) {
@@ -47,6 +46,16 @@ func (s *RPC) imgLoadHndl(id uint64, params json.RawMessage) (rr jrpc.Response)
 	return jrpc.EncodeResponse(id, value, err)
 }
 
+func (s *RPC) imgDeleteHndl(id uint64, params json.RawMessage) (rr jrpc.Response) {
+	var fileID string
+	if err := json.Unmarshal(params, &fileID); err != nil {
+		return jrpc.Response{Error: err.Error()}
+	}
+	err := s.img.Delete(fileID)
+	return jrpc.EncodeResponse(id, nil, err)
+
+}
+
 func (s *RPC) imgCommitHndl(id uint64, params json.RawMessage) (rr jrpc.Response) {
 	var fileID string
 	if err := json.Unmarshal(params, &fileID); err != nil {

backend/_example/memory_store/server/image_test.go

@@ -158,4 +158,9 @@ func TestRPC_imgInfoHndl(t *testing.T) {
 	info, err = ri.Info()
 	assert.NoError(t, err)
 	assert.False(t, info.FirstStagingImageTS.IsZero())
+
+	err = ri.Delete("test_img")
+	assert.NoError(t, err)
+	_, err = ri.Load("test_img")
+	assert.EqualError(t, err, "image test_img not found")
 }

backend/_example/memory_store/server/rpc.go

@@ -60,6 +60,7 @@ func (s *RPC) addHandlers() {
 		"save_with_id":        s.imgSaveWithIDHndl,
 		"reset_cleanup_timer": s.imgResetClnTimerHndl,
 		"load":                s.imgLoadHndl,
+		"delete":              s.imgDeleteHndl,
 		"commit":              s.imgCommitHndl,
 		"cleanup":             s.imgCleanupHndl,
 		"info":                s.imgInfoHndl,

backend/app/store/image/bolt_store.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"time"
 
@@ -118,6 +119,18 @@ func (b *Bolt) Load(id string) ([]byte, error) {
 	return data, nil
 }
 
+// Delete image from storage
+func (b *Bolt) Delete(id string) error {
+	return b.db.Update(func(tx *bolt.Tx) error {
+		// deleting a non-existing key doesn't return an error, so joining errors from deleting an image
+		// from both buckets is safe and will return nil if there are no errors on the real delete
+		// or image is absent in both buckets
+		err := tx.Bucket([]byte(imagesBktName)).Delete([]byte(id))
+		err = errors.Join(err, tx.Bucket([]byte(imagesStagedBktName)).Delete([]byte(id)))
+		return err
+	})
+}
+
 // Cleanup runs scan of staging and removes old data based on ttl
 func (b *Bolt) Cleanup(_ context.Context, ttl time.Duration) error {
 	return b.db.Update(func(tx *bolt.Tx) error {

backend/app/store/image/bolt_store_test.go

@@ -58,6 +58,36 @@ func TestBoltStore_LoadAfterSave(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestBoltStore_LoadAfterDelete(t *testing.T) {
+	svc, teardown := prepareBoltImageStorageTest(t)
+	defer teardown()
+
+	// delete image from permanent storage
+	id := "test_img"
+	err := svc.Save(id, gopherPNGBytes())
+	assert.NoError(t, err)
+
+	err = svc.Commit(id)
+	require.NoError(t, err)
+
+	err = svc.Delete(id)
+	assert.NoError(t, err)
+
+	_, err = svc.Load(id)
+	assert.Error(t, err)
+
+	// delete staging image
+	id = "staging_img"
+	err = svc.Save(id, gopherPNGBytes())
+	assert.NoError(t, err)
+
+	err = svc.Delete(id)
+	assert.NoError(t, err)
+
+	_, err = svc.Load(id)
+	assert.Error(t, err)
+}
+
 func TestBoltStore_Cleanup(t *testing.T) {
 	svc, teardown := prepareBoltImageStorageTest(t)
 	defer teardown()

backend/app/store/image/fs_store.go

@@ -23,7 +23,8 @@ type FileSystem struct {
 	Staging    string
 	Partitions int
 
-	crc struct {
+	moveLock sync.Mutex // needed only for deleting images or moving them from staging to permanent storage
+	crc      struct {
 		*crc64.Table
 		sync.Once
 		mask    string
@@ -50,6 +51,8 @@ func (f *FileSystem) Save(id string, img []byte) error {
 
 // Commit file stored in staging location by moving it to permanent location
 func (f *FileSystem) Commit(id string) error {
+	f.moveLock.Lock()
+	defer f.moveLock.Unlock()
 	log.Printf("[DEBUG] Commit image %s", id)
 	stagingImage, permImage := f.location(f.Staging, id), f.location(f.Location, id)
 
@@ -107,11 +110,31 @@ func (f *FileSystem) Load(id string) ([]byte, error) {
 	return io.ReadAll(fh)
 }
 
+// Delete image from storage
+func (f *FileSystem) Delete(id string) error {
+	f.moveLock.Lock()
+	defer f.moveLock.Unlock()
+	staging := f.location(f.Staging, id)
+	// file doesn't exist on staging, delete from permanent location
+	if _, err := os.Stat(staging); os.IsNotExist(err) {
+		file := f.location(f.Location, id)
+		e := os.Remove(file)
+		_ = os.Remove(path.Dir(file)) // try to remove directory
+		return e
+	}
+	// delete file from staging
+	err := os.Remove(staging)
+	_ = os.Remove(path.Dir(staging)) // try to remove directory
+	return err
+}
+
 // Cleanup runs scan of staging and removes old files based on ttl
 func (f *FileSystem) Cleanup(_ context.Context, ttl time.Duration) error {
 	if _, err := os.Stat(f.Staging); os.IsNotExist(err) {
 		return nil
 	}
+	f.moveLock.Lock()
+	defer f.moveLock.Unlock()
 
 	// we can ignore context as on local FS remove is relatively fast operation
 	err := filepath.Walk(f.Staging, func(fpath string, info os.FileInfo, err error) error {

backend/app/store/image/fs_store_test.go

@@ -131,6 +131,31 @@ func TestFsStore_LoadAfterCommit(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestFsStore_LoadAfterDelete(t *testing.T) {
+	svc, teardown := prepareImageTest(t)
+	defer teardown()
+
+	id := "test_img"
+	err := svc.Save(id, gopherPNGBytes())
+	assert.NoError(t, err)
+	err = svc.Commit(id)
+	require.NoError(t, err)
+	err = svc.Delete(id)
+	require.NoError(t, err)
+
+	_, err = svc.Load(id)
+	assert.Error(t, err)
+
+	// create file on staging
+	err = svc.Save(id, gopherPNGBytes())
+	assert.NoError(t, err)
+	err = svc.Delete(id)
+	require.NoError(t, err)
+
+	_, err = svc.Load(id)
+	assert.Error(t, err)
+}
+
 func TestFsStore_location(t *testing.T) {
 	tbl := []struct {
 		partitions int

backend/app/store/image/image.go

@@ -70,6 +70,7 @@ type Store interface {
 	Info() (StoreInfo, error)         // get meta information about storage
 	Save(id string, img []byte) error // store image with passed id to staging
 	Load(id string) ([]byte, error)   // load image by ID
+	Delete(id string) error           // delete image by ID
 
 	ResetCleanupTimer(id string) error                    // resets cleanup timer for the image, called on comment preview
 	Commit(id string) error                               // move image from staging to permanent
@@ -212,6 +213,11 @@ func (s *Service) Load(id string) ([]byte, error) {
 	return s.store.Load(id)
 }
 
+// Delete wraps storage Delete function.
+func (s *Service) Delete(id string) error {
+	return s.store.Delete(id)
+}
+
 // Save wraps storage Save function, validating and resizing the image before calling it.
 func (s *Service) Save(userID string, r io.Reader) (id string, err error) {
 	id = path.Join(userID, guid())

backend/app/store/image/image_mock.go

@@ -4,9 +4,9 @@
 package image
 
 import (
-	context "context"
+	"context"
 	"sync"
-	time "time"
+	"time"
 )
 
 // Ensure, that StoreMock does implement Store.
@@ -25,6 +25,9 @@ var _ Store = &StoreMock{}
 //			CommitFunc: func(id string) error {
 //				panic("mock out the Commit method")
 //			},
+//			DeleteFunc: func(id string) error {
+//				panic("mock out the Delete method")
+//			},
 //			InfoFunc: func() (StoreInfo, error) {
 //				panic("mock out the Info method")
 //			},
@@ -50,6 +53,9 @@ type StoreMock struct {
 	// CommitFunc mocks the Commit method.
 	CommitFunc func(id string) error
 
+	// DeleteFunc mocks the Delete method.
+	DeleteFunc func(id string) error
+
 	// InfoFunc mocks the Info method.
 	InfoFunc func() (StoreInfo, error)
 
@@ -76,6 +82,11 @@ type StoreMock struct {
 			// ID is the id argument value.
 			ID string
 		}
+		// Delete holds details about calls to the Delete method.
+		Delete []struct {
+			// ID is the id argument value.
+			ID string
+		}
 		// Info holds details about calls to the Info method.
 		Info []struct {
 		}
@@ -99,6 +110,7 @@ type StoreMock struct {
 	}
 	lockCleanup           sync.RWMutex
 	lockCommit            sync.RWMutex
+	lockDelete            sync.RWMutex
 	lockInfo              sync.RWMutex
 	lockLoad              sync.RWMutex
 	lockResetCleanupTimer sync.RWMutex
@@ -173,6 +185,38 @@ func (mock *StoreMock) CommitCalls() []struct {
 	return calls
 }
 
+// Delete calls DeleteFunc.
+func (mock *StoreMock) Delete(id string) error {
+	if mock.DeleteFunc == nil {
+		panic("StoreMock.DeleteFunc: method is nil but Store.Delete was just called")
+	}
+	callInfo := struct {
+		ID string
+	}{
+		ID: id,
+	}
+	mock.lockDelete.Lock()
+	mock.calls.Delete = append(mock.calls.Delete, callInfo)
+	mock.lockDelete.Unlock()
+	return mock.DeleteFunc(id)
+}
+
+// DeleteCalls gets all the calls that were made to Delete.
+// Check the length with:
+//
+//	len(mockedStore.DeleteCalls())
+func (mock *StoreMock) DeleteCalls() []struct {
+	ID string
+} {
+	var calls []struct {
+		ID string
+	}
+	mock.lockDelete.RLock()
+	calls = mock.calls.Delete
+	mock.lockDelete.RUnlock()
+	return calls
+}
+
 // Info calls InfoFunc.
 func (mock *StoreMock) Info() (StoreInfo, error) {
 	if mock.InfoFunc == nil {

backend/app/store/image/remote_store.go

@@ -41,6 +41,12 @@ func (r *RPC) Load(id string) ([]byte, error) {
 	return io.ReadAll(base64.NewDecoder(base64.StdEncoding, strings.NewReader(rawImg)))
 }
 
+// Delete image from storage
+func (r *RPC) Delete(id string) error {
+	_, err := r.Call("image.delete", id)
+	return err
+}
+
 // Commit file stored in staging location by moving it to permanent location
 func (r *RPC) Commit(id string) error {
 	_, err := r.Call("image.commit", id)

backend/app/store/image/remote_store_test.go

@@ -41,6 +41,18 @@ func TestRemote_Load(t *testing.T) {
 	assert.Equal(t, gopherPNGBytes(), res)
 }
 
+func TestRemote_Delete(t *testing.T) {
+	ts := testServer(t, `{"method":"image.delete","params":"54321","id":1}`, `{}`)
+	defer ts.Close()
+	c := RPC{Client: jrpc.Client{API: ts.URL, Client: http.Client{}}}
+
+	var a Store = &c
+	_ = a
+
+	err := c.Delete("54321")
+	assert.NoError(t, err)
+}
+
 func TestRemote_Commit(t *testing.T) {
 	ts := testServer(t, `{"method":"image.commit","params":"gopher_id","id":1}`, `{"id":1}`)
 	defer ts.Close()

backend/app/store/service/service.go

@@ -5,6 +5,7 @@ package service
 import (
 	"fmt"
 	"math"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -796,6 +797,34 @@ func (s *DataStore) Delete(locator store.Locator, commentID string, mode store.D
 		s.repliesCache.Delete(commentID)
 		s.repliesCache.Delete(comment.ParentID)
 	}
+
+	// delete images from the comment if they are not reused elsewhere in comments to the same page
+	idsFn := func() []string { // get IDs of all images from the same URL to verify if image from deleted comment was reused
+		comments, e := s.Engine.Find(engine.FindRequest{Locator: locator})
+		if e != nil {
+			log.Printf("[WARN] can't get comments %s text for deleted comment image check, %v", comment.ID, err)
+			return nil
+		}
+		var imgIDs = []string{}
+		for _, cc := range comments {
+			// exclude the comment we are deleting
+			if cc.ID != commentID {
+				imgIDs = append(imgIDs, s.ImageService.ExtractPictures(cc.Text)...)
+			}
+		}
+		return imgIDs
+	}
+	commentImgIDs := s.ImageService.ExtractPictures(comment.Text)
+	pageImgIDs := idsFn()
+	for _, id := range commentImgIDs {
+		if !slices.Contains(pageImgIDs, id) {
+			if err := s.ImageService.Delete(id); err != nil {
+				log.Printf("[WARN] failed to delete image %s on comment %s deletion, %v", id, commentID, err)
+			}
+		}
+	}
+	log.Printf("[ERROR] commentImgIDs: %v, pageImgIDs: %v", commentImgIDs, pageImgIDs)
+
 	req := engine.DeleteRequest{Locator: locator, CommentID: commentID, DeleteMode: mode}
 	return s.Engine.Delete(req)
 }

backend/app/store/service/service_test.go

@@ -1406,6 +1406,80 @@ func TestService_Delete(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+func TestService_deleteImagesOnCommentDelete(t *testing.T) {
+	lgr.Setup(lgr.Debug, lgr.CallerFile, lgr.CallerFunc)
+
+	mockStore := image.StoreMock{
+		DeleteFunc:            func(id string) error { return nil },
+		CommitFunc:            func(id string) error { return nil },
+		ResetCleanupTimerFunc: func(id string) error { return nil },
+	}
+	imgSvc := image.NewService(&mockStore,
+		image.ServiceParams{
+			EditDuration: 50 * time.Millisecond,
+			ImageAPI:     "/images/dev/",
+			ProxyAPI:     "/non_existent",
+		})
+	defer imgSvc.Close(context.TODO())
+
+	// two comments for https://radio-t.com
+	eng, teardown := prepStoreEngine(t)
+	defer teardown()
+	b := DataStore{Engine: eng, EditDuration: 50 * time.Millisecond,
+		AdminStore: admin.NewStaticKeyStore("secret 123"), ImageService: imgSvc}
+
+	c := store.Comment{
+		ID:        "id-22",
+		Text:      `some text <img src="/images/dev/pic1.png"/> xx <img src="/images/dev/pic2.png"/>`,
+		Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local),
+		Locator:   store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"},
+		User:      store.User{ID: "user1", Name: "user name"},
+	}
+	_, err := b.Engine.Create(c) // create directly with engine, doesn't call submitImages
+	assert.NoError(t, err)
+	b.submitImages(c)
+	// reply to the first comment with one new image and one existing one
+	c = store.Comment{
+		ID:       "id-23",
+		ParentID: "id-22",
+		Text:     `some text <img src="/images/dev/pic2.png"/> xx <img src="/images/dev/pic3.png"/>`,
+		Locator:  store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"},
+		User:     store.User{ID: "user1", Name: "user name"},
+	}
+	_, err = b.Engine.Create(c) // create directly with engine, doesn't call submitImages
+	assert.NoError(t, err)
+	b.submitImages(c)
+
+	// verify that images are in staging store
+	assert.Equal(t, 4, len(mockStore.ResetCleanupTimerCalls()))
+	assert.Equal(t, "dev/pic1.png", mockStore.ResetCleanupTimerCalls()[0].ID)
+	assert.Equal(t, "dev/pic2.png", mockStore.ResetCleanupTimerCalls()[1].ID)
+	assert.Equal(t, "dev/pic2.png", mockStore.ResetCleanupTimerCalls()[2].ID)
+	assert.Equal(t, "dev/pic3.png", mockStore.ResetCleanupTimerCalls()[3].ID)
+	time.Sleep(b.EditDuration + 100*time.Millisecond)
+	// verify that they got into the main store
+	assert.Equal(t, 4, len(mockStore.CommitCalls()))
+	assert.Equal(t, "dev/pic1.png", mockStore.CommitCalls()[0].ID)
+	assert.Equal(t, "dev/pic2.png", mockStore.CommitCalls()[1].ID)
+	assert.Equal(t, "dev/pic2.png", mockStore.CommitCalls()[2].ID)
+	assert.Equal(t, "dev/pic3.png", mockStore.CommitCalls()[3].ID)
+
+	// delete the first comment
+	err = b.Delete(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "id-22", store.SoftDelete)
+	assert.NoError(t, err)
+	// verify that images are deleted from the main store
+	assert.Equal(t, 1, len(mockStore.DeleteCalls()))
+	assert.Equal(t, "dev/pic1.png", mockStore.DeleteCalls()[0].ID)
+
+	// delete the second comment
+	err = b.Delete(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "id-23", store.SoftDelete)
+	assert.NoError(t, err)
+	// verify that images are deleted from the main store
+	assert.Equal(t, 3, len(mockStore.DeleteCalls()))
+	assert.Equal(t, "dev/pic2.png", mockStore.DeleteCalls()[1].ID)
+	assert.Equal(t, "dev/pic3.png", mockStore.DeleteCalls()[2].ID)
+}
+
 // DeleteUser removes all comments from user
 func TestService_DeleteUser(t *testing.T) {
 	// two comments for https://radio-t.com, no reply