From 4f1d3a1f84e7321586c6eff308e1e0f6b894cd31 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Thu, 16 Apr 2026 03:18:27 -0700
Subject: [PATCH] fix(mount): count manifest sizes in merge condition to
 prevent manifest accumulation

shouldMergeChunks only counted non-manifest chunk sizes toward the bloat
threshold, so overlapping manifests accumulated undetected across flush
cycles.

During sustained random writes (e.g. fio), each metadata flush compacts
non-manifest chunks and may group them into a new manifest via
MaybeManifestize, while carrying forward all existing manifests. Since
the merge condition only checked non-manifest totals against 2x file
size, the growing pile of redundant manifests never triggered a merge.

In a real workload: 4 GB file, 25 manifests each covering ~4 GB
(107 GB manifest data on volume servers), but shouldMergeChunks saw only
4.2 GB of non-manifest chunks vs the 8.6 GB threshold -- no merge.

Fix: include manifest coverage sizes in totalChunkSize. This correctly
detects the 111 GB total vs 8.6 GB threshold and triggers the merge,
which re-reads the file as clean chunks and lets the filer's MinusChunks
(which resolves manifests) garbage-collect all redundant sub-chunks.
---
 weed/mount/weedfs_file_sync.go      |  7 +++++
 weed/mount/weedfs_file_sync_test.go | 40 ++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/weed/mount/weedfs_file_sync.go b/weed/mount/weedfs_file_sync.go
index 0ca7bad57..9a4b205d6 100644
--- a/weed/mount/weedfs_file_sync.go
+++ b/weed/mount/weedfs_file_sync.go
@@ -268,6 +268,13 @@ func shouldMergeChunks(compactedChunks []*filer_pb.FileChunk, manifestChunks []*
 	for _, chunk := range compactedChunks {
 		totalChunkSize += chunk.Size
 	}
+	// Count manifest coverage toward stored total. Each manifest holds
+	// sub-chunks on volume servers that cover approximately Size bytes.
+	// Without this, overlapping manifests accumulate undetected because
+	// the merge condition only saw the (small) non-manifest chunk total.
+	for _, chunk := range manifestChunks {
+		totalChunkSize += chunk.Size
+	}
 	allChunks := make([]*filer_pb.FileChunk, 0, len(compactedChunks)+len(manifestChunks))
 	allChunks = append(allChunks, compactedChunks...)
 	allChunks = append(allChunks, manifestChunks...)
diff --git a/weed/mount/weedfs_file_sync_test.go b/weed/mount/weedfs_file_sync_test.go
index e5781007a..3d7d326d8 100644
--- a/weed/mount/weedfs_file_sync_test.go
+++ b/weed/mount/weedfs_file_sync_test.go
@@ -68,7 +68,8 @@ func TestShouldMergeChunks_JustOverDouble(t *testing.T) {
 }
 
 func TestShouldMergeChunks_ManifestExtendFileSize(t *testing.T) {
-	// Compacted chunks are small relative to file extended by manifest.
+	// One manifest extends the file. Total stored = 100 (regular) + 900
+	// (manifest) = 1000 vs 2*1000 = 2000 → no merge.
 	compacted := []*filer_pb.FileChunk{
 		{Offset: 0, Size: 100, FileId: "a", ModifiedTsNs: 1},
 	}
@@ -77,12 +78,13 @@ func TestShouldMergeChunks_ManifestExtendFileSize(t *testing.T) {
 	}
 	_, _, merge := shouldMergeChunks(compacted, manifest)
 	if merge {
-		t.Fatal("manifest-extended file should not merge when compacted chunks are small")
+		t.Fatal("single manifest extending file should not merge")
 	}
 }
 
-func TestShouldMergeChunks_ManifestChunksSizeIgnored(t *testing.T) {
-	// Only compacted chunk sizes count toward totalChunkSize.
+func TestShouldMergeChunks_ManifestSizesCounted(t *testing.T) {
+	// Manifest sizes must count toward totalChunkSize so that overlapping
+	// manifests trigger merge.
 	compacted := []*filer_pb.FileChunk{
 		{Offset: 0, Size: 100, FileId: "a", ModifiedTsNs: 1},
 	}
@@ -90,11 +92,35 @@ func TestShouldMergeChunks_ManifestChunksSizeIgnored(t *testing.T) {
 		{Offset: 0, Size: 100, FileId: "m", ModifiedTsNs: 2, IsChunkManifest: true},
 	}
 	total, _, merge := shouldMergeChunks(compacted, manifest)
-	if total != 100 {
-		t.Fatalf("totalChunkSize should only count compacted, got %d", total)
+	if total != 200 {
+		t.Fatalf("totalChunkSize should count compacted + manifests, got %d", total)
 	}
 	if merge {
-		t.Fatal("should not merge")
+		t.Fatal("2x total on 100-byte file should not merge (need >2x)")
+	}
+}
+
+func TestShouldMergeChunks_AccumulatedManifests(t *testing.T) {
+	// Simulates the real bug: multiple manifests each covering the full file
+	// accumulate across flush cycles. Without counting manifest sizes, the
+	// merge condition never fires and storage bloats indefinitely.
+	compacted := []*filer_pb.FileChunk{
+		{Offset: 0, Size: 1000, FileId: "a", ModifiedTsNs: 100},
+	}
+	// 5 manifests each covering the full file — successive flush cycles
+	var manifests []*filer_pb.FileChunk
+	for i := 0; i < 5; i++ {
+		manifests = append(manifests, &filer_pb.FileChunk{
+			Offset: 0, Size: 1000, FileId: string(rune('m' + i)),
+			IsChunkManifest: true,
+		})
+	}
+	total, fileSize, merge := shouldMergeChunks(compacted, manifests)
+	// total = 1000 (regular) + 5*1000 (manifests) = 6000
+	// fileSize = 1000
+	// 6000 > 2*1000 → merge
+	if !merge {
+		t.Fatalf("accumulated overlapping manifests should trigger merge (total=%d fileSize=%d)", total, fileSize)
 	}
 }