Repository: nats-io/nats-server
Pull request #3413 (2022-08-30 00:35:59)

FSS state could skew on expire on recover with no msgs left.
Also added in sanity check on server start.

Signed-off-by: Derek Collison <derek@nats.io>

/cc @nats-io/core



Patch:
diff --git a/server/filestore.go b/server/filestore.go
index d2bea361..727400d6 100644
--- a/server/filestore.go
+++ b/server/filestore.go
@@ -1235,6 +1235,8 @@ func (fs *fileStore) expireMsgsOnRecover() {
 				// Do this part by hand since not deleting one by one.
 				mb.first.seq, mb.first.ts = mb.last.seq+1, 0
 				mb.closeAndKeepIndex()
+				// Clear any global subject state.
+				fs.psim = make(map[string]*psi)
 			} else {
 				mb.dirtyCloseWithRemove(true)
 				deleted++
@@ -3065,12 +3067,17 @@ func (fs *fileStore) checkMsgs() *LostStreamData {
 
 	fs.checkAndFlushAllBlocks()
 
+	// Clear any global subject state.
+	fs.psim = make(map[string]*psi)
+
 	for _, mb := range fs.blks {
 		if ld, err := mb.rebuildState(); err != nil && ld != nil {
 			// Rebuild fs state too.
 			mb.fs.rebuildStateLocked(ld)
 		}
+		fs.populateGlobalPerSubjectInfo(mb)
 	}
+
 	return fs.ld
 }
 
@@ -4990,6 +4997,12 @@ func (mb *msgBlock) closeAndKeepIndex() {
 	// Close
 	mb.dirtyCloseWithRemove(false)
 
+	// Make sure to remove fss state.
+	mb.fss = nil
+	if mb.sfn != _EMPTY_ {
+		os.Remove(mb.sfn)
+	}
+
 	// If we are encrypted we should reset our bek counter.
 	if mb.bek != nil {
 		if bek, err := genBlockEncryptionKey(mb.fs.fcfg.Cipher, mb.seed, mb.nonce); err == nil {
@@ -5191,6 +5204,7 @@ func (mb *msgBlock) ensurePerSubjectInfoLoaded() error {
 }
 
 // Called on recovery to populate the global psim state.
+// Lock should be held.
 func (fs *fileStore) populateGlobalPerSubjectInfo(mb *msgBlock) {
 	mb.mu.Lock()
 	defer mb.mu.Unlock()
@@ -5198,6 +5212,20 @@ func (fs *fileStore) populateGlobalPerSubjectInfo(mb *msgBlock) {
 	if err := mb.readPerSubjectInfo(true); err != nil {
 		return
 	}
+
+	// Quick sanity check.
+	// TODO(dlc) - This is here to auto-clear a bug.
+	fssMsgs := uint64(0)
+	for subj, ss := range mb.fss {
+		if len(subj) > 0 {
+			fssMsgs += ss.Msgs
+		}
+	}
+	// If we are off rebuild.
+	if fssMsgs != mb.msgs {
+		mb.generatePerSubjectInfo(true)
+	}
+
 	// Now populate psim.
 	for subj, ss := range mb.fss {
 		if len(subj) > 0 {


Test patch:
diff --git a/server/filestore_test.go b/server/filestore_test.go
index 07a7eafd..23b25da5 100644
--- a/server/filestore_test.go
+++ b/server/filestore_test.go
@@ -4510,3 +4510,79 @@ func TestFileStoreNoFSSAfterRecover(t *testing.T) {
 		t.Fatalf("Expected no fss post recover")
 	}
 }
+
+func TestFileStoreFSSCloseAndKeepOnExpireOnRecoverBug(t *testing.T) {
+	storeDir := createDir(t, JetStreamStoreDir)
+	defer os.RemoveAll(storeDir)
+
+	ttl := 100 * time.Millisecond
+	fs, err := newFileStore(
+		FileStoreConfig{StoreDir: storeDir},
+		StreamConfig{Name: "zzz", Subjects: []string{"foo"}, Storage: FileStorage, MaxAge: ttl},
+	)
+	require_NoError(t, err)
+	defer fs.Stop()
+
+	_, _, err = fs.StoreMsg("foo", nil, nil)
+	require_NoError(t, err)
+
+	fs.Stop()
+
+	time.Sleep(2 * ttl)
+
+	fs, err = newFileStore(
+		FileStoreConfig{StoreDir: storeDir},
+		StreamConfig{Name: "zzz", Subjects: []string{"foo"}, Storage: FileStorage, MaxAge: ttl},
+	)
+	require_NoError(t, err)
+	defer fs.Stop()
+
+	if state := fs.State(); state.NumSubjects != 0 {
+		t.Fatalf("Expected no subjects with no messages, got %d", state.NumSubjects)
+	}
+}
+
+func TestFileStoreFSSBadStateBug(t *testing.T) {
+	storeDir := createDir(t, JetStreamStoreDir)
+	defer os.RemoveAll(storeDir)
+
+	fs, err := newFileStore(
+		FileStoreConfig{StoreDir: storeDir},
+		StreamConfig{Name: "zzz", Subjects: []string{"foo"}, Storage: FileStorage},
+	)
+	require_NoError(t, err)
+	defer fs.Stop()
+
+	_, _, err = fs.StoreMsg("foo", nil, nil)
+	require_NoError(t, err)
+	_, _, err = fs.StoreMsg("foo", nil, nil)
+	require_NoError(t, err)
+
+	// Force write of fss.
+	mb := fs.getFirstBlock()
+	mb.mu.Lock()
+	mb.writePerSubjectInfo()
+	fssFile := filepath.Join(storeDir, msgDir, fmt.Sprintf(fssScan, 1))
+	buf, err := os.ReadFile(fssFile)
+	require_NoError(t, err)
+	mb.mu.Unlock()
+
+	// Now remove one of them.
+	fs.RemoveMsg(1)
+	fs.Stop()
+
+	// Now put back wrong fss with msgs == 2
+	err = os.WriteFile(fssFile, buf, defaultFilePerms)
+	require_NoError(t, err)
+
+	fs, err = newFileStore(
+		FileStoreConfig{StoreDir: storeDir},
+		StreamConfig{Name: "zzz", Subjects: []string{"foo"}, Storage: FileStorage},
+	)
+	require_NoError(t, err)
+	defer fs.Stop()
+
+	if fss := fs.SubjectsState("foo")["foo"]; fss.Msgs != 1 {
+		t.Fatalf("Got bad state on restart: %+v", fss)
+	}
+}