Skip to content

Commit bc718e0

Browse files
frristclaude
andcommitted
fix(s3): release shipped segment blobs on DeleteBucket
A shipped catalog segment registers TWO blobs in the bucket's space: the sealed CAR and its sharded-dag-index (both blob/added by SubmitShard). Hilt refuses to delete a space that still holds registrations, so any bucket that lived past the catalog seal age with a successful ship could never be deleted: DeleteBucket returned BucketNotEmpty with no objects left. Surfaced by promoting CompleteMultipartUpload/should_verify_final_composite_ checksum — the first conformance case long enough (~11s of commits) to ship a segment before its teardown. The index blob's digest was recorded nowhere, so ship now persists it: SubmitShard returns the index digest, the flush func hands it to MarkSegmentShipped, and ingot.segments gains an index_digest column (00010). DeleteBucket asks the log manager for every shipped segment's (CAR, index) digests and releases both from the space before the hilt delete; releases are idempotent, so retries are safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 9b3ba35 commit bc718e0

15 files changed

Lines changed: 217 additions & 68 deletions

File tree

blockstore/staging_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func (f *fakeMeta) MarkSegmentSealed(_ context.Context, plane blockstore.Plane,
4242
f.roots = append(f.roots, opRoots...)
4343
return nil
4444
}
45-
func (f *fakeMeta) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, _ []blockstore.OpRoot) error {
45+
func (f *fakeMeta) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, _ []byte, _ []blockstore.OpRoot) error {
4646
if r, ok := f.rows[seq]; ok {
4747
r.ShippedAt = shippedAt
4848
}
@@ -86,7 +86,7 @@ func (f *fakeMeta) RehydrateSegment(_ context.Context, m logstore.SegmentMeta) e
8686

8787
// nopFlush is the per-plane ship callback for these tests: the store
8888
// owns the ship-state transition, so the closure is a no-op.
89-
func nopFlush(_ context.Context, _ *logstore.Segment) error { return nil }
89+
func nopFlush(_ context.Context, _ *logstore.Segment) ([]byte, error) { return nil, nil }
9090

9191
// noopBase satisfies blockstore.BlockReader but always returns
9292
// errUnknownBase so we can detect when a GetBlock falls through

inmem/segments_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func TestMarkSegmentShipped_GuardsForgeRootOnRoot(t *testing.T) {
4444

4545
// Ship with the stale op-root LAST: unconditionally (the old behavior) it
4646
// would win as the last write; the guard must skip it and keep `committed`.
47-
if err := m.MarkSegmentShipped(ctx, blockstore.PlaneCatalog, seq, 100, []blockstore.OpRoot{
47+
if err := m.MarkSegmentShipped(ctx, blockstore.PlaneCatalog, seq, 100, nil, []blockstore.OpRoot{
4848
{Bucket: "bk", Root: committed},
4949
{Bucket: "bk", Root: stale},
5050
}); err != nil {
@@ -65,7 +65,7 @@ func TestMarkSegmentShipped_GuardsForgeRootOnRoot(t *testing.T) {
6565
_ = m2.CASRoot(ctx, "bk2", cid.Undef, committed)
6666
seq2, _ := m2.NextSegmentSeq(ctx)
6767
_ = m2.InsertSegmentOpen(ctx, blockstore.PlaneCatalog, seq2, "bk2")
68-
if err := m2.MarkSegmentShipped(ctx, blockstore.PlaneCatalog, seq2, 100, []blockstore.OpRoot{
68+
if err := m2.MarkSegmentShipped(ctx, blockstore.PlaneCatalog, seq2, 100, nil, []blockstore.OpRoot{
6969
{Bucket: "bk2", Root: stale},
7070
}); err != nil {
7171
t.Fatalf("MarkSegmentShipped(stale only): %v", err)

inmem/store.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -287,11 +287,12 @@ func (m *MemStore) MarkSegmentSealed(_ context.Context, plane blockstore.Plane,
287287
return nil
288288
}
289289

290-
func (m *MemStore) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, opRoots []blockstore.OpRoot) error {
290+
func (m *MemStore) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, indexDigest []byte, opRoots []blockstore.OpRoot) error {
291291
m.mu.Lock()
292292
defer m.mu.Unlock()
293293
if r, ok := m.segments[seq]; ok {
294294
r.ShippedAt = shippedAt
295+
r.IndexDigest = append([]byte(nil), indexDigest...)
295296
}
296297
if plane == blockstore.PlaneCatalog {
297298
for _, opr := range opRoots {
@@ -375,8 +376,8 @@ func (NopBaseReader) OpenBlob(_ context.Context, _ did.DID, _ multihash.Multihas
375376
// network, so the spool's local copy serves all reads.
376377
type NopUploader struct{}
377378

378-
func (NopUploader) SubmitShard(_ context.Context, _ blockstore.Plane, _ did.DID, _ uploader.CARShard) (uploader.BlobLocation, error) {
379-
return uploader.BlobLocation{}, nil
379+
func (NopUploader) SubmitShard(_ context.Context, _ blockstore.Plane, _ did.DID, _ uploader.CARShard) (uploader.BlobLocation, multihash.Multihash, error) {
380+
return uploader.BlobLocation{}, nil, nil
380381
}
381382

382383
// UploadBlob accepts immediately, even with WithConclude(false) — there is

logstore/config.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,12 @@ type PlaneConfig struct {
6060
}
6161

6262
// FlushFunc is the contract for shipping one sealed segment's CAR to
63-
// Forge. The segment is single-plane, so no plane argument is needed.
64-
type FlushFunc func(ctx context.Context, seg *Segment) error
63+
// Forge. The segment is single-plane, so no plane argument is needed. On
64+
// success it returns the digest of the shipped sharded-dag-index blob (the
65+
// ship registers the CAR and its index in the bucket's space; DeleteBucket
66+
// releases both), or nil when nothing registered (header-only segment, or a
67+
// non-publishing uploader).
68+
type FlushFunc func(ctx context.Context, seg *Segment) ([]byte, error)
6569

6670
func (c *Config) validate() error {
6771
if c.Dir == "" {

logstore/manager.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
block "github.com/ipfs/go-block-format"
1313
"github.com/ipfs/go-cid"
14+
"github.com/multiformats/go-multihash"
1415
"go.uber.org/zap"
1516

1617
"github.com/fil-forge/ingot/blockstore"
@@ -219,6 +220,32 @@ func (m *Manager) Close(ctx context.Context) error {
219220
return nil
220221
}
221222

223+
// ShippedSegmentDigests returns the multihash of every blob the bucket's
224+
// catalog segments registered in its space: each shipped segment's CAR and
225+
// its sharded-dag-index blob. DeleteBucket must release them all before the
226+
// space itself can be deleted (the tenant service refuses to delete a space
227+
// that still holds registrations). Segments whose ship registered nothing —
228+
// unshipped, header-only, or shipped through a non-publishing uploader —
229+
// carry no IndexDigest and contribute nothing.
230+
func (m *Manager) ShippedSegmentDigests(ctx context.Context, bucket string) ([][]byte, error) {
231+
rows, err := m.meta.ListSegments(ctx, blockstore.PlaneCatalog, bucket)
232+
if err != nil {
233+
return nil, fmt.Errorf("logstore: manager: list segments for %q: %w", bucket, err)
234+
}
235+
var out [][]byte
236+
for _, r := range rows {
237+
if r.ShippedAt == 0 || len(r.IndexDigest) == 0 || len(r.SHA256) == 0 {
238+
continue
239+
}
240+
carDigest, err := multihash.Encode(r.SHA256, multihash.SHA2_256)
241+
if err != nil {
242+
return nil, fmt.Errorf("logstore: manager: encode segment %d sha: %w", r.Seq, err)
243+
}
244+
out = append(out, carDigest, r.IndexDigest)
245+
}
246+
return out, nil
247+
}
248+
222249
// RemoveBucketLog deletes bucket's log entirely: closes its store (dropping
223250
// queued-but-unshipped segments — a deleted bucket's history has nowhere to
224251
// ship), unlinks its directory, and removes its segment rows. Used by

logstore/manager_test.go

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,23 @@ import (
1515
)
1616

1717
// recordingFlush counts flushes per bucket so tests can assert each
18-
// bucket's segments ship through that bucket's own closure.
18+
// bucket's segments ship through that bucket's own closure. Every flush
19+
// reports fakeIndexDigest as the shipped index blob, as a real ship would.
1920
type recordingFlush struct {
2021
mu sync.Mutex
2122
byBkt map[string]int
2223
}
2324

25+
var fakeIndexDigest = []byte("fake-index-digest-multihash")
26+
2427
func newRecordingFlush() *recordingFlush { return &recordingFlush{byBkt: map[string]int{}} }
2528

2629
func (r *recordingFlush) forBucket(bucket string) FlushFunc {
27-
return func(context.Context, *Segment) error {
30+
return func(context.Context, *Segment) ([]byte, error) {
2831
r.mu.Lock()
2932
defer r.mu.Unlock()
3033
r.byBkt[bucket]++
31-
return nil
34+
return fakeIndexDigest, nil
3235
}
3336
}
3437

@@ -181,6 +184,63 @@ func TestManager_RemoveBucketLog(t *testing.T) {
181184
}
182185
}
183186

187+
// TestManager_ShippedSegmentDigests: a shipped segment contributes its CAR
188+
// multihash and index-blob digest — the space registrations DeleteBucket
189+
// must release — and a bucket with no shipped segments contributes nothing.
190+
func TestManager_ShippedSegmentDigests(t *testing.T) {
191+
dir := t.TempDir()
192+
meta := newFakeMeta()
193+
flush := newRecordingFlush()
194+
m := openTestManager(t, dir, meta, flush)
195+
196+
appendFor(t, m, "alpha", "shipped-content")
197+
198+
// Wait for alpha's segment to seal + flush; the flush marks it shipped
199+
// with the recorded index digest.
200+
deadline := time.Now().Add(2 * time.Second)
201+
for time.Now().Before(deadline) {
202+
if flush.count("alpha") > 0 {
203+
break
204+
}
205+
time.Sleep(10 * time.Millisecond)
206+
}
207+
var shipped bool
208+
rows, err := meta.ListSegments(context.Background(), blockstore.PlaneCatalog, "alpha")
209+
if err != nil {
210+
t.Fatalf("ListSegments: %v", err)
211+
}
212+
for _, r := range rows {
213+
if r.ShippedAt != 0 {
214+
shipped = true
215+
}
216+
}
217+
if !shipped {
218+
t.Skip("segment did not ship within the deadline; nothing to assert")
219+
}
220+
221+
digests, err := m.ShippedSegmentDigests(context.Background(), "alpha")
222+
if err != nil {
223+
t.Fatalf("ShippedSegmentDigests: %v", err)
224+
}
225+
if len(digests) == 0 || len(digests)%2 != 0 {
226+
t.Fatalf("expected (CAR, index) digest pairs, got %d entries", len(digests))
227+
}
228+
for i := 0; i < len(digests); i += 2 {
229+
// The CAR digest is the multihash-encoded sha256 (2-byte prefix +
230+
// 32-byte digest); the index digest is whatever the flush reported.
231+
if len(digests[i]) != 34 {
232+
t.Fatalf("CAR digest %d: expected 34-byte sha2-256 multihash, got %d", i, len(digests[i]))
233+
}
234+
if string(digests[i+1]) != string(fakeIndexDigest) {
235+
t.Fatalf("index digest %d: got %q", i+1, digests[i+1])
236+
}
237+
}
238+
// A bucket with no shipped segments reports none.
239+
if got, err := m.ShippedSegmentDigests(context.Background(), "gamma"); err != nil || len(got) != 0 {
240+
t.Fatalf("gamma: got %d digests, err %v", len(got), err)
241+
}
242+
}
243+
184244
// TestManager_RejectsUnsafeBucketNames: the bucket→directory mapping must
185245
// not be escapable.
186246
func TestManager_RejectsUnsafeBucketNames(t *testing.T) {

logstore/planelog.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,11 +314,11 @@ func (pl *PlaneLog) flushOne(seg *Segment) {
314314
backoff := time.Second
315315

316316
for attempt := 1; attempt <= maxAttempts; attempt++ {
317-
err := pl.pc.Flush(ctx, seg)
317+
indexDigest, err := pl.pc.Flush(ctx, seg)
318318
if err == nil {
319319
now := time.Now().Unix()
320320
seg.markShipped(now)
321-
if merr := pl.meta.MarkSegmentShipped(ctx, pl.plane, seg.Seq(), now, seg.OpRoots()); merr != nil {
321+
if merr := pl.meta.MarkSegmentShipped(ctx, pl.plane, seg.Seq(), now, indexDigest, seg.OpRoots()); merr != nil {
322322
pl.logger.Error("logstore: mark shipped",
323323
zap.Stringer("plane", pl.plane), zap.Uint64("seq", seg.Seq()), zap.Error(merr))
324324
}

logstore/store_test.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func (f *fakeMeta) MarkSegmentSealed(_ context.Context, plane blockstore.Plane,
7575
return nil
7676
}
7777

78-
func (f *fakeMeta) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, opRoots []blockstore.OpRoot) error {
78+
func (f *fakeMeta) MarkSegmentShipped(_ context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, indexDigest []byte, opRoots []blockstore.OpRoot) error {
7979
f.mu.Lock()
8080
defer f.mu.Unlock()
8181
m, ok := f.segments[seq]
@@ -86,6 +86,7 @@ func (f *fakeMeta) MarkSegmentShipped(_ context.Context, plane blockstore.Plane,
8686
return nil
8787
}
8888
m.ShippedAt = shippedAt
89+
m.IndexDigest = append([]byte(nil), indexDigest...)
8990
f.shipped = append(f.shipped, shipEvent{seq: seq, plane: plane})
9091
return nil
9192
}
@@ -173,9 +174,9 @@ func newTestStore(t *testing.T, sealBytes int64, sealAge time.Duration, retain i
173174
meta := newFakeMeta()
174175
flushCalls := &atomicCounter{}
175176
logger := zaptest.NewLogger(t)
176-
flush := func(_ context.Context, _ *Segment) error {
177+
flush := func(_ context.Context, _ *Segment) ([]byte, error) {
177178
flushCalls.add(1)
178-
return nil
179+
return nil, nil
179180
}
180181
cfg := Config{
181182
Dir: dir,
@@ -335,7 +336,7 @@ func TestForceSealRecoveredOpenOnRestart(t *testing.T) {
335336
cfg := Config{
336337
Dir: dir,
337338
Meta: meta,
338-
Catalog: PlaneConfig{SealBytes: 1 << 30, SealAge: time.Hour, Ship: true, Flush: func(context.Context, *Segment) error { return nil }, Retain: 6},
339+
Catalog: PlaneConfig{SealBytes: 1 << 30, SealAge: time.Hour, Ship: true, Flush: func(context.Context, *Segment) ([]byte, error) { return nil, nil }, Retain: 6},
339340
Logger: logger,
340341
}
341342
s, err := Open(context.Background(), cfg)
@@ -512,7 +513,7 @@ func TestCatalogNeverShips(t *testing.T) {
512513
SealBytes: 1,
513514
SealAge: 20 * time.Millisecond,
514515
Ship: false,
515-
Flush: func(context.Context, *Segment) error { ships.add(1); return nil },
516+
Flush: func(context.Context, *Segment) ([]byte, error) { ships.add(1); return nil, nil },
516517
},
517518
Logger: logger,
518519
}

logstore/types.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ type SegmentMeta struct {
6868
// plane stays 0 forever.
6969
ShippedAt int64
7070

71+
// IndexDigest is the multihash of the shipped sharded-dag-index blob.
72+
// Non-nil exactly when the ship registered blobs in the bucket's space
73+
// (the CAR + this index) — the registrations DeleteBucket must release.
74+
IndexDigest []byte
75+
7176
// OpRoots are the per-batch (bucket, root) records. Populated only
7277
// for catalog-plane segments — op-roots are MST roots.
7378
OpRoots []blockstore.OpRoot
@@ -100,7 +105,7 @@ type Meta interface {
100105
// to Forge, stamping shipped_at, and advances forge_root_cid in
101106
// ingot.buckets for every op-root recorded against this segment (catalog
102107
// roots are the MST roots durable on Forge), all in one transaction.
103-
MarkSegmentShipped(ctx context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, opRoots []blockstore.OpRoot) error
108+
MarkSegmentShipped(ctx context.Context, plane blockstore.Plane, seq uint64, shippedAt int64, indexDigest []byte, opRoots []blockstore.OpRoot) error
104109

105110
// DeleteSegment removes a segment row (cascades to op-root rows).
106111
// Used by retention after the on-disk files are unlinked.
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
-- +goose Up
2+
-- A shipped segment registers TWO blobs in the bucket's space: the CAR and
3+
-- its sharded-dag-index. index_digest records the index blob's multihash at
4+
-- ship time so DeleteBucket can release both registrations (the tenant
5+
-- service refuses to delete a space that still holds any). NULL means the
6+
-- segment registered nothing (unshipped, header-only, or a non-publishing
7+
-- uploader).
8+
ALTER TABLE ingot.segments ADD COLUMN index_digest bytea;
9+
10+
-- +goose Down
11+
ALTER TABLE ingot.segments DROP COLUMN index_digest;

0 commit comments

Comments
 (0)