Skip to content

Commit b2d78e3

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 quiesces the bucket's log first — joining any in-flight ship, so a segment can't register its blobs after the release pass has read the rows (on faster hosts the teardown lands exactly inside the ship window) — then releases every sealed segment's CAR and every shipped segment's index from the space before the hilt delete. Releases are idempotent (removing an unregistered blob is a no-op), so sealed-but-unshipped CARs are released defensively and retries are safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 601ec7e commit b2d78e3

15 files changed

Lines changed: 266 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: 58 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,63 @@ func (m *Manager) Close(ctx context.Context) error {
219220
return nil
220221
}
221222

223+
// QuiesceBucketLog stops bucket's flush pipeline and waits for any in-flight
224+
// ship to finish, so the segment rows ShippedSegmentDigests reads afterwards
225+
// are final: a ship that was mid-flight has either completed (registered its
226+
// blobs AND stamped shipped_at + index_digest) or aborted. Without this,
227+
// DeleteBucket races the flush goroutine — a segment whose CAR just
228+
// registered but wasn't yet marked shipped would be invisible to the release
229+
// pass, and the space delete would be refused. The closed store reopens
230+
// lazily on the bucket's next use, so a delete that fails downstream leaves
231+
// the bucket functional.
232+
func (m *Manager) QuiesceBucketLog(ctx context.Context, bucket string) error {
233+
if err := validBucketDir(bucket); err != nil {
234+
return err
235+
}
236+
m.mu.Lock()
237+
s, ok := m.stores[bucket]
238+
delete(m.stores, bucket)
239+
m.mu.Unlock()
240+
if !ok {
241+
return nil
242+
}
243+
if err := s.Close(ctx); err != nil {
244+
return fmt.Errorf("logstore: manager: quiesce log for %q: %w", bucket, err)
245+
}
246+
return nil
247+
}
248+
249+
// ShippedSegmentDigests returns the multihash of every blob the bucket's
250+
// catalog segments may have registered in its space: each shipped segment's
251+
// CAR and its sharded-dag-index blob, plus the CAR of any sealed-but-
252+
// unshipped segment (a flush aborted between the CAR's blob/add and the
253+
// shipped stamp leaves that registration behind; releasing an unregistered
254+
// blob is a no-op, so over-listing is safe). DeleteBucket must release them
255+
// all before the space itself can be deleted — the tenant service refuses to
256+
// delete a space that still holds registrations. Call QuiesceBucketLog first
257+
// so no ship is in flight while this reads.
258+
func (m *Manager) ShippedSegmentDigests(ctx context.Context, bucket string) ([][]byte, error) {
259+
rows, err := m.meta.ListSegments(ctx, blockstore.PlaneCatalog, bucket)
260+
if err != nil {
261+
return nil, fmt.Errorf("logstore: manager: list segments for %q: %w", bucket, err)
262+
}
263+
var out [][]byte
264+
for _, r := range rows {
265+
if r.State != StateSealed || len(r.SHA256) == 0 {
266+
continue
267+
}
268+
carDigest, err := multihash.Encode(r.SHA256, multihash.SHA2_256)
269+
if err != nil {
270+
return nil, fmt.Errorf("logstore: manager: encode segment %d sha: %w", r.Seq, err)
271+
}
272+
out = append(out, carDigest)
273+
if r.ShippedAt != 0 && len(r.IndexDigest) > 0 {
274+
out = append(out, r.IndexDigest)
275+
}
276+
}
277+
return out, nil
278+
}
279+
222280
// RemoveBucketLog deletes bucket's log entirely: closes its store (dropping
223281
// queued-but-unshipped segments — a deleted bucket's history has nowhere to
224282
// ship), unlinks its directory, and removes its segment rows. Used by

logstore/manager_test.go

Lines changed: 73 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,73 @@ func TestManager_RemoveBucketLog(t *testing.T) {
181184
}
182185
}
183186

187+
// TestManager_QuiesceAndShippedSegmentDigests: quiescing joins the flush
188+
// pipeline so the enumeration afterwards is final — every shipped segment
189+
// contributes its CAR multihash and index-blob digest, sealed-but-unshipped
190+
// segments contribute their CAR only (a flush aborted after the CAR's
191+
// blob/add may have registered it), and a bucket with no sealed segments
192+
// contributes nothing.
193+
func TestManager_QuiesceAndShippedSegmentDigests(t *testing.T) {
194+
dir := t.TempDir()
195+
meta := newFakeMeta()
196+
flush := newRecordingFlush()
197+
m := openTestManager(t, dir, meta, flush)
198+
199+
appendFor(t, m, "alpha", "shipped-content")
200+
201+
// Quiesce without waiting for the seal-age tick: it force-seals the open
202+
// segment and joins the flush pipeline, so the rows are final on return
203+
// regardless of whether the tail segment shipped or was dropped.
204+
if err := m.QuiesceBucketLog(context.Background(), "alpha"); err != nil {
205+
t.Fatalf("QuiesceBucketLog: %v", err)
206+
}
207+
208+
rows, err := meta.ListSegments(context.Background(), blockstore.PlaneCatalog, "alpha")
209+
if err != nil {
210+
t.Fatalf("ListSegments: %v", err)
211+
}
212+
shipped := map[uint64]bool{}
213+
sealed := 0
214+
for _, r := range rows {
215+
if r.State == StateSealed {
216+
sealed++
217+
shipped[r.Seq] = r.ShippedAt != 0
218+
}
219+
}
220+
if sealed == 0 {
221+
t.Fatalf("expected the quiesce to force-seal the open segment")
222+
}
223+
224+
digests, err := m.ShippedSegmentDigests(context.Background(), "alpha")
225+
if err != nil {
226+
t.Fatalf("ShippedSegmentDigests: %v", err)
227+
}
228+
// Every sealed segment contributes a 34-byte sha2-256 CAR multihash;
229+
// shipped ones additionally the flush's index digest.
230+
wantLen := 0
231+
for _, s := range shipped {
232+
wantLen++
233+
if s {
234+
wantLen++
235+
}
236+
}
237+
if len(digests) != wantLen {
238+
t.Fatalf("got %d digests, want %d (sealed=%d shipped=%v)", len(digests), wantLen, sealed, shipped)
239+
}
240+
for _, d := range digests {
241+
if len(d) != 34 && string(d) != string(fakeIndexDigest) {
242+
t.Fatalf("unexpected digest %q (len %d)", d, len(d))
243+
}
244+
}
245+
246+
// A bucket with no sealed segments reports none, and the quiesced
247+
// bucket reopens lazily for new appends.
248+
if got, err := m.ShippedSegmentDigests(context.Background(), "gamma"); err != nil || len(got) != 0 {
249+
t.Fatalf("gamma: got %d digests, err %v", len(got), err)
250+
}
251+
appendFor(t, m, "alpha", "post-quiesce-write")
252+
}
253+
184254
// TestManager_RejectsUnsafeBucketNames: the bucket→directory mapping must
185255
// not be escapable.
186256
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)