Fix panic in manager when `attachment-cache-dir` is not set, fixes #617

docs/releases.md

@@ -2,6 +2,12 @@
 Binaries for all releases can be found on the GitHub releases pages for the [ntfy server](https://github.com/binwiederhier/ntfy/releases)
 and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/releases).
 
+## ntfy server v2.0.1 (UNRELEASED)
+
+**Bug fixes + maintenance:**
+
+* Avoid panic in manager when `attachment-cache-dir` is not set ([#617](https://github.com/binwiederhier/ntfy/issues/617), thanks to [@ksurl](https://github.com/ksurl))  
+
 ## ntfy server v2.0.0
 Released February 16, 2023
 
@@ -64,6 +70,11 @@ going. It'll only make ntfy better.
 * User account signup, login, topic reservations, access tokens, tiers etc. ([#522](https://github.com/binwiederhier/ntfy/issues/522))
 * `OPTIONS` method calls are not serviced when the UI is disabled ([#598](https://github.com/binwiederhier/ntfy/issues/598), thanks to [@enticedwanderer](https://github.com/enticedwanderer) for reporting)
 
+**Special thanks:**
+
+A big Thank-you goes to everyone who tested the user account and payments work. I very much appreciate all the feedback,
+suggestions, and bug reports. Thank you, @nwithan8, @deadcade, @xenrox, @cmeis, and the others who I forgot.
+
 ## ntfy server v1.31.0
 Released February 14, 2023
 
@@ -95,11 +106,6 @@ breaking-change upgrade, which required some work to get working again.
 
 * Portuguese (thanks to [@ssantos](https://hosted.weblate.org/user/ssantos/))
 
-**Special thanks:**
-
-A big Thank-you goes to everyone who tested the user account and payments work. I very much appreciate all the feedback,
-suggestions, and bug reports. Thank you, @nwithan8, @deadcade, and @xenrox.
-
 ## ntfy server v1.30.1
 Released December 23, 2022 🎅
 

server/message_cache.go

@@ -536,7 +536,7 @@ func (c *messageCache) ExpireMessages(topics ...string) error {
 	}
 	defer tx.Rollback()
 	for _, t := range topics {
-		if _, err := tx.Exec(updateMessagesForTopicExpiryQuery, time.Now().Unix(), t); err != nil {
+		if _, err := tx.Exec(updateMessagesForTopicExpiryQuery, time.Now().Unix()-1, t); err != nil {
 			return err
 		}
 	}

server/server_account.go

@@ -506,6 +506,7 @@ func (s *Server) maybeRemoveMessagesAndExcessReservations(r *http.Request, v *vi
 	if err := s.messageCache.ExpireMessages(topics...); err != nil {
 		return err
 	}
+	go s.pruneMessages()
 	return nil
 }
 

server/server_manager.go

@@ -116,29 +116,30 @@ func (s *Server) pruneTokens() {
 }
 
 func (s *Server) pruneAttachments() {
-	if s.fileCache != nil {
+	if s.fileCache == nil {
-		log.
+		return
-			Tag(tagManager).
-			Timing(func() {
-				ids, err := s.messageCache.AttachmentsExpired()
-				if err != nil {
-					log.Tag(tagManager).Err(err).Warn("Error retrieving expired attachments")
-				} else if len(ids) > 0 {
-					if log.Tag(tagManager).IsDebug() {
-						log.Tag(tagManager).Debug("Deleting attachments %s", strings.Join(ids, ", "))
-					}
-					if err := s.fileCache.Remove(ids...); err != nil {
-						log.Tag(tagManager).Err(err).Warn("Error deleting attachments")
-					}
-					if err := s.messageCache.MarkAttachmentsDeleted(ids...); err != nil {
-						log.Tag(tagManager).Err(err).Warn("Error marking attachments deleted")
-					}
-				} else {
-					log.Tag(tagManager).Debug("No expired attachments to delete")
-				}
-			}).
-			Debug("Deleted expired attachments")
 	}
+	log.
+		Tag(tagManager).
+		Timing(func() {
+			ids, err := s.messageCache.AttachmentsExpired()
+			if err != nil {
+				log.Tag(tagManager).Err(err).Warn("Error retrieving expired attachments")
+			} else if len(ids) > 0 {
+				if log.Tag(tagManager).IsDebug() {
+					log.Tag(tagManager).Debug("Deleting attachments %s", strings.Join(ids, ", "))
+				}
+				if err := s.fileCache.Remove(ids...); err != nil {
+					log.Tag(tagManager).Err(err).Warn("Error deleting attachments")
+				}
+				if err := s.messageCache.MarkAttachmentsDeleted(ids...); err != nil {
+					log.Tag(tagManager).Err(err).Warn("Error marking attachments deleted")
+				}
+			} else {
+				log.Tag(tagManager).Debug("No expired attachments to delete")
+			}
+		}).
+		Debug("Deleted expired attachments")
 }
 
 func (s *Server) pruneMessages() {
@@ -149,8 +150,10 @@ func (s *Server) pruneMessages() {
 			if err != nil {
 				log.Tag(tagManager).Err(err).Warn("Error retrieving expired messages")
 			} else if len(expiredMessageIDs) > 0 {
-				if err := s.fileCache.Remove(expiredMessageIDs...); err != nil {
+				if s.fileCache != nil {
-					log.Tag(tagManager).Err(err).Warn("Error deleting attachments for expired messages")
+					if err := s.fileCache.Remove(expiredMessageIDs...); err != nil {
+						log.Tag(tagManager).Err(err).Warn("Error deleting attachments for expired messages")
+					}
 				}
 				if err := s.messageCache.DeleteMessages(expiredMessageIDs...); err != nil {
 					log.Tag(tagManager).Err(err).Warn("Error marking attachments deleted")

server/server_manager_test.go

@@ -0,0 +1,28 @@
+package server
+
+import (
+	"github.com/stretchr/testify/require"
+	"testing"
+)
+
+func TestServer_Manager_Prune_Messages_Without_Attachments_DoesNotPanic(t *testing.T) {
+	// Tests that the manager runs without attachment-cache-dir set, see #617
+	c := newTestConfig(t)
+	c.AttachmentCacheDir = ""
+	s := newTestServer(t, c)
+
+	// Publish a message
+	rr := request(t, s, "POST", "/mytopic", "hi", nil)
+	require.Equal(t, 200, rr.Code)
+	m := toMessage(t, rr.Body.String())
+
+	// Expire message
+	require.Nil(t, s.messageCache.ExpireMessages("mytopic"))
+
+	// Does not panic
+	s.pruneMessages()
+
+	// Actually deleted
+	_, err := s.messageCache.Message(m.ID)
+	require.Equal(t, errMessageNotFound, err)
+}