From 9e8ad383aab80157fd4b092d4f9743a8eaa3b240 Mon Sep 17 00:00:00 2001 From: Al Cutter Date: Wed, 16 Sep 2026 09:19:40 +0000 Subject: [PATCH 1/3] Add reproducer test for #1178 --- storage/posix/files_test.go | 104 +++++++++++++++++++++++------------- 1 file changed, 66 insertions(+), 38 deletions(-) diff --git a/storage/posix/files_test.go b/storage/posix/files_test.go index 8f54495b1..c590749e4 100644 --- a/storage/posix/files_test.go +++ b/storage/posix/files_test.go @@ -20,10 +20,10 @@ import ( "encoding/base64" "errors" "fmt" + "iter" "net/http" "os" "path/filepath" - "slices" "strings" "testing" "time" @@ -802,48 +802,33 @@ func TestMirrorWriter_UpdateCheckpointGeometry(t *testing.T) { t.Fatalf("MirrorWriter: %v", err) } - // Create 600 entries to span multiple tile levels. - entries := make([][]byte, 600) - for i := range 600 { - entries[i] = fmt.Appendf(nil, "entry %d", i) - } - - bundles := []*api.EntryBundle{ - {Entries: entries[0:256]}, - {Entries: entries[256:512]}, - {Entries: entries[512:600]}, - } - bundlesIter := func(yield func(*api.EntryBundle, error) bool) { - for _, b := range bundles { - if !yield(b, nil) { - return - } - } - } - - // Integrate to size 600. - // Tiles layout should then be: - // Level 1: [2] - // Level 0: [256] [256] [88] - // Entries: [256] [256] [88] - size, _, err := mw.IntegrateBundles(ctx, 0, bundlesIter) + // Create 66000 entries to span multiple tile levels and multiple tiles at level 1. + const treeSize = 66000 + size, _, err := mw.IntegrateBundles(ctx, 0, generateBundles(t, treeSize)) if err != nil { t.Fatalf("IntegrateBundles: %v", err) } - if size != 600 { - t.Fatalf("expected integrated size 600, got %d", size) + if size != treeSize { + t.Fatalf("expected integrated size %d, got %d", treeSize, size) } for _, test := range []struct { - size uint64 + size uint64 + // wantRHSizes[l] is the expected node count for the rightmost tile at level l: + // - 1..255: partial tile with that many nodes (read via .p/N) + // - 256: full tile (read via p=0, asserting 256 nodes) + // - 0: no tile exists at this coordinate for this checkpoint (asserts full tile file is absent) wantRHSizes []int }{ {0, []int{}}, {1, []int{1}}, {129, []int{129}}, {256, []int{256, 1}}, - {300, []int{44, 2}}, + {300, []int{44, 1}}, {600, []int{88, 2}}, + {65536, []int{256, 256, 1}}, + {65537, []int{1, 0, 1}}, + {66000, []int{208, 1, 1}}, } { t.Run(fmt.Sprintf("size_%d", test.size), func(t *testing.T) { h := make([]byte, 32) @@ -864,14 +849,38 @@ func TestMirrorWriter_UpdateCheckpointGeometry(t *testing.T) { if l := len(eb.Entries); l != test.wantRHSizes[0] { t.Fatalf("expected %d entries in partial bundle %d.%d, got %d", test.wantRHSizes[0], idx, p, l) } - if got, want := eb.Entries, bundles[idx].Entries[:test.wantRHSizes[0]]; !slices.EqualFunc(got, want, bytes.Equal) { - t.Errorf("entrybundle doesn't match:\ngot %v\nwant %v", got, want) - } - // Verify the existence/correctness of the partial tiles for the given size. - for level, p := range test.wantRHSizes { - tile := mustReadTile(t, lr, uint64(level), idx, uint8(p)) - if len(tile.Nodes) != p { - t.Errorf("expected %d nodes in partial tile %d/%d.%d, got %d", p, level, idx, p, len(tile.Nodes)) + + // Verify the existence/correctness of the partial tiles for the given size, + // and ensure that full tiles whose range extends beyond the tree are not created. + for level, wantNodes := range test.wantRHSizes { + fullTilePath := layout.TilePath(uint64(level), idx, 0) + treeP := layout.PartialTileSize(uint64(level), idx, treeSize) + + if wantNodes == 0 { + // Tile does not exist at this checkpoint size. + // A full tile _may_ legitimately exist in the tree if it's large enough. + // However, since our treeSize is 66000 we know that one cannot be present for + // these test cases, so we'll assert that it doesn't exist in order to verify that we + // haven't incorrectly created one. + if fi, err := os.Stat(filepath.Join(s.cfg.Path, fullTilePath)); err == nil { + t.Fatalf("Level %d tile %d: full tile %s should not exist for size %d (found %d bytes)", level, idx, fullTilePath, test.size, fi.Size()) + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("Failed to stat %s: %v", fullTilePath, err) + } + } else { + tile := mustReadTile(t, lr, uint64(level), idx, uint8(wantNodes)) + if len(tile.Nodes) != wantNodes { + t.Errorf("Expected %d nodes in partial tile %d/%d.%d, got %d", wantNodes, level, idx, wantNodes, len(tile.Nodes)) + } + // If the integrated tree does not have a full tile at this position (treeP > 0), + // no full tile file should have been written. + if treeP > 0 { + if fi, err := os.Stat(filepath.Join(s.cfg.Path, fullTilePath)); err == nil { + t.Errorf("Level %d tile %d: full tile %s should not exist for size %d (found %d bytes)", level, idx, fullTilePath, test.size, fi.Size()) + } else if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("Failed to stat %s: %v", fullTilePath, err) + } + } } idx >>= layout.TileHeight } @@ -928,3 +937,22 @@ func BenchmarkMarshalTlogEntryBundle(b *testing.B) { } } +func generateBundles(t *testing.T, totalEntries int) iter.Seq2[*api.EntryBundle, error] { + t.Helper() + + return func(yield func(*api.EntryBundle, error) bool) { + remaining := totalEntries + dummyEntry := []byte("a") + for remaining > 0 { + count := min(remaining, layout.EntryBundleWidth) + entries := make([][]byte, count) + for i := range entries { + entries[i] = dummyEntry + } + if !yield(&api.EntryBundle{Entries: entries}, nil) { + return + } + remaining -= count + } + } +} From c0479b0fc995243f577fc91a8d66af8cb7335333 Mon Sep 17 00:00:00 2001 From: Al Cutter Date: Wed, 16 Sep 2026 09:19:40 +0000 Subject: [PATCH 2/3] Avoid creating out-of-bounds tiles when trimming to match a mirrored checkpoint. --- storage/posix/files.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/storage/posix/files.go b/storage/posix/files.go index 3ad1e366f..fcb9b0c7f 100644 --- a/storage/posix/files.go +++ b/storage/posix/files.go @@ -1388,7 +1388,7 @@ func (m *MirrorWriter) UpdateCheckpoint(ctx context.Context, fn func(old []byte) // If an implied partial resource is not already present, this function will attempt to create // it from a strictly larger resource whose presence is implied by treeSize. func (m *MirrorWriter) ensureGeometry(ctx context.Context, cpSize, treeSize uint64) error { - if cpSize == 0 { + if cpSize == 0 || cpSize == treeSize { return nil } if cpSize > treeSize { @@ -1425,7 +1425,7 @@ func maxLevel(sz uint64) int { // If the implied partial entry bundle is not already present, this function will attempt to create // it from the entry bundle implied by treeSize. func (m *MirrorWriter) ensurePartialBundle(ctx context.Context, idx uint64, cpP, treeP uint8) error { - if cpP == treeP { + if cpP == 0 || cpP == treeP { return nil } @@ -1451,12 +1451,7 @@ func (m *MirrorWriter) ensurePartialBundle(ctx context.Context, idx uint64, cpP, return fmt.Errorf("failed to unmarshal entry bundle @%d.%d: %v", idx, treeP, err) } - // Then trim it down to the size implied by the checkpoint, and write it out. - // Handle cpP == 0 where a full-bundle is implied - we should never actually hit this case since cpP must - // equal treeP in this case, but it doesn't hurt to be defensive. - if cpP > 0 { - eb.Entries = eb.Entries[:cpP] - } + eb.Entries = eb.Entries[:cpP] d, err = marshalTlogEntryBundle(eb) if err != nil { return fmt.Errorf("failed to marshal entry bundle @%d.%d: %v", idx, cpP, err) @@ -1473,7 +1468,7 @@ func (m *MirrorWriter) ensurePartialBundle(ctx context.Context, idx uint64, cpP, // If the implied partial tile is not already present, this function will attempt to create // it from the tile implied by treeSize. func (m *MirrorWriter) ensurePartialTile(ctx context.Context, l uint64, idx uint64, cpP, treeP uint8) error { - if cpP == treeP { + if cpP == 0 || cpP == treeP { return nil } @@ -1499,12 +1494,7 @@ func (m *MirrorWriter) ensurePartialTile(ctx context.Context, l uint64, idx uint return fmt.Errorf("failed to unmarshal tile @%d/%d.%d: %v", l, idx, treeP, err) } - // Then trim it down to the size implied by the checkpoint, and write it out. - // Handle cpP == 0 where a full-tile is implied - we should never actually hit this case since cpP must - // equal treeP in this case, but it doesn't hurt to be defensive. - if cpP > 0 { - t.Nodes = t.Nodes[:cpP] - } + t.Nodes = t.Nodes[:cpP] d, err = t.MarshalText() if err != nil { return fmt.Errorf("failed to marshal tile @%d/%d.%d: %v", l, idx, cpP, err) From 498041b8a2a8452dda011b59a0aa92b3d5722299 Mon Sep 17 00:00:00 2001 From: Al Cutter Date: Wed, 16 Sep 2026 14:18:50 +0000 Subject: [PATCH 3/3] Address comments --- storage/posix/files.go | 13 +++++++++++-- storage/posix/files_test.go | 8 +++++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/storage/posix/files.go b/storage/posix/files.go index fcb9b0c7f..a60a768ac 100644 --- a/storage/posix/files.go +++ b/storage/posix/files.go @@ -1388,6 +1388,9 @@ func (m *MirrorWriter) UpdateCheckpoint(ctx context.Context, fn func(old []byte) // If an implied partial resource is not already present, this function will attempt to create // it from a strictly larger resource whose presence is implied by treeSize. func (m *MirrorWriter) ensureGeometry(ctx context.Context, cpSize, treeSize uint64) error { + // If cpSize is zero then no tree exists. + // If cpSize == treeSize the resources are guaranteed present by integration. + // In both cases there's nothing to do. if cpSize == 0 || cpSize == treeSize { return nil } @@ -1399,6 +1402,12 @@ func (m *MirrorWriter) ensureGeometry(ctx context.Context, cpSize, treeSize uint for l := uint64(0); l <= uint64(ml); l, idx = l+1, idx>>layout.TileHeight { treeP := layout.PartialTileSize(l, idx, treeSize) cpP := layout.PartialTileSize(l, idx, cpSize) + + if cpP == treeP { + // Nothing to be done at this level. + continue + } + if l == 0 { if err := m.ensurePartialBundle(ctx, idx, cpP, treeP); err != nil { return err @@ -1425,7 +1434,7 @@ func maxLevel(sz uint64) int { // If the implied partial entry bundle is not already present, this function will attempt to create // it from the entry bundle implied by treeSize. func (m *MirrorWriter) ensurePartialBundle(ctx context.Context, idx uint64, cpP, treeP uint8) error { - if cpP == 0 || cpP == treeP { + if cpP == 0 { return nil } @@ -1468,7 +1477,7 @@ func (m *MirrorWriter) ensurePartialBundle(ctx context.Context, idx uint64, cpP, // If the implied partial tile is not already present, this function will attempt to create // it from the tile implied by treeSize. func (m *MirrorWriter) ensurePartialTile(ctx context.Context, l uint64, idx uint64, cpP, treeP uint8) error { - if cpP == 0 || cpP == treeP { + if cpP == 0 { return nil } diff --git a/storage/posix/files_test.go b/storage/posix/files_test.go index c590749e4..0ba1c6b1e 100644 --- a/storage/posix/files_test.go +++ b/storage/posix/files_test.go @@ -870,7 +870,7 @@ func TestMirrorWriter_UpdateCheckpointGeometry(t *testing.T) { } else { tile := mustReadTile(t, lr, uint64(level), idx, uint8(wantNodes)) if len(tile.Nodes) != wantNodes { - t.Errorf("Expected %d nodes in partial tile %d/%d.%d, got %d", wantNodes, level, idx, wantNodes, len(tile.Nodes)) + t.Errorf("Expected %d nodes in partial tile %d/%d.%d, got %d", wantNodes, level, idx, uint8(wantNodes), len(tile.Nodes)) } // If the integrated tree does not have a full tile at this position (treeP > 0), // no full tile file should have been written. @@ -942,17 +942,19 @@ func generateBundles(t *testing.T, totalEntries int) iter.Seq2[*api.EntryBundle, return func(yield func(*api.EntryBundle, error) bool) { remaining := totalEntries - dummyEntry := []byte("a") + + N := 0 for remaining > 0 { count := min(remaining, layout.EntryBundleWidth) entries := make([][]byte, count) for i := range entries { - entries[i] = dummyEntry + entries[i] = fmt.Appendf(nil, "entry %d", N+i) } if !yield(&api.EntryBundle{Entries: entries}, nil) { return } remaining -= count + N += count } } }