Skip to content

Commit fe6220a

Browse files
frristclaude
andcommitted
review: stock-image gates, MinPartSize provenance, cleanup logging, drop ci-error.log
- The delete-finality and deferred-multipart gates run on the stock smelt-SDK images like every other itest: env gate and binary injection dropped. They need piri:main ≥ piri#30 and a hilt with hilt#36's blob.Abort grant; until those publish, the standard INGOT_ITEST_PIRI_IMAGE / (new) INGOT_ITEST_HILT_IMAGE overrides cover local runs. Validated end-to-end with branch-built piri+hilt images: both gates green, AbortRejects 0.12s. - Pin smelt at the smelt#19 branch head — its generated stack proofs carry the /blob/release + /blob/reject node delegations the gates exercise; re-pin on merge. - Complete's 5 MiB minimum-part floor now reads backend.MinPartSize: it is S3's protocol constant, not an operator knob. - Log the previously discarded errors: the post-commit latch to 'completed', and DeletePark / spool.Remove / DeleteIntent in cleanupPartBlobs. - Restore the captured-store mask in abortOpenSession with the corrected rationale: s3:DeleteBucket delegates no blob commands, so DeleteBucket's implicit abort must run on the authority captured at UploadPart (blob.Abort rides the write set per hilt#36); fix the xfail and architecture-doc comments that misattributed the grant. - Remove the stray ci-error.log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent abe6e03 commit fe6220a

10 files changed

Lines changed: 64 additions & 2572 deletions

File tree

ci-error.log

Lines changed: 0 additions & 2494 deletions
This file was deleted.

docs/architecture.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -840,9 +840,11 @@ paths below are exercised against the real stack by the smelt-based `itest/` har
840840
than `multipart_session_ttl` (default 7d) and reaps terminal session rows. A successful
841841
Complete retains its session in state `completed` so a duplicate Complete is idempotent
842842
per S3. `DeleteBucket` implicitly aborts the bucket's in-flight sessions before the space
843-
delete (upstream's conformance teardown never aborts them); its `/blob/abort` leg is gated
844-
on hilt granting `blob.Abort` for the bucket-delete operation. The network-side `/blob/abort`
845-
unwind remains a parking-flow concern (above).
843+
delete (upstream's conformance teardown never aborts them); because `s3:DeleteBucket`
844+
delegates no blob commands, the abort runs on the space authority captured at `UploadPart`,
845+
and its `/blob/abort` leg is gated on hilt delegating `blob.Abort` with the write set
846+
(fil-forge/hilt#36). The network-side `/blob/abort` unwind remains a parking-flow
847+
concern (above).
846848
847849
### Known correctness boundary
848850

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ require (
1010
github.com/fil-forge/hilt v0.0.1-0.20260724134448-ba71f843f6a4
1111
github.com/fil-forge/indexing-service v1.13.5-0.20260619142411-efe3f5fab717
1212
github.com/fil-forge/libforge v0.0.0-20260727220215-5e299c46f62f
13-
github.com/fil-forge/smelt v0.0.0-20260720130429-63116166a06c
13+
github.com/fil-forge/smelt v0.0.0-20260728210138-0a0154e6e6d0
1414
github.com/fil-forge/ucantone v0.0.0-20260727203046-ccb77059de44
1515
github.com/fil-forge/versitygw v0.0.0-20260716095011-7a65883d595a
1616
github.com/fxamacker/cbor/v2 v2.9.2

go.sum

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,8 @@ github.com/fil-forge/indexing-service v1.13.5-0.20260619142411-efe3f5fab717 h1:W
252252
github.com/fil-forge/indexing-service v1.13.5-0.20260619142411-efe3f5fab717/go.mod h1:wFcakLohOqpMRkJzWRdGFGHRFpGB+PpEMCm9wkt2cqU=
253253
github.com/fil-forge/libforge v0.0.0-20260727220215-5e299c46f62f h1:QzgMg8GIE4IhgOE/7DBHFU/4T5oU7J4vAKxbUPKJPGA=
254254
github.com/fil-forge/libforge v0.0.0-20260727220215-5e299c46f62f/go.mod h1:0kXihIQ4L2uZ00nR5XrZ/Y8Db7Ht/qQNuiWslwMJ95M=
255-
github.com/fil-forge/smelt v0.0.0-20260720130429-63116166a06c h1:WHvsleEU6ZiNYDFgLx6KtorXulD+IuLiorMgmp4Th8s=
256-
github.com/fil-forge/smelt v0.0.0-20260720130429-63116166a06c/go.mod h1:NM/mk/XiP1Kzsy9HWGQeLsosPUvVY3bIrkUzqswThpU=
255+
github.com/fil-forge/smelt v0.0.0-20260728210138-0a0154e6e6d0 h1:QmwxgO2W6bL6B9Ce2E2nIvxc8EyIdS5hkrDvSlsTQE0=
256+
github.com/fil-forge/smelt v0.0.0-20260728210138-0a0154e6e6d0/go.mod h1:czwgD0nnuQ8h3GOHdwQ2Guw3C9R3zYP3K/mE+DEyh+Y=
257257
github.com/fil-forge/ucantone v0.0.0-20260727203046-ccb77059de44 h1:ofvb2Qq7++VPRelGsLbtnd1ZMKVT4n4QGoa79BhJ6VQ=
258258
github.com/fil-forge/ucantone v0.0.0-20260727203046-ccb77059de44/go.mod h1:oFY5BfD0bDeodGlbBHh3/nK99MAS93rGXjoQz7s5qgE=
259259
github.com/fil-forge/versitygw v0.0.0-20260716095011-7a65883d595a h1:lDwnNmF4LNbevx/YYNCC0CazMiL4eIe38MvfqbRR3RA=

internal/reqscope/reqscope.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ func ProofStore(ctx context.Context) (ucanlib.ProofStore, bool) {
3737
return ps, ok
3838
}
3939

40+
// WithoutProofStore returns a context whose request-scoped proof store is
41+
// masked. Hilt delegates Forge commands per S3 permission, and some
42+
// permissions (s3:DeleteBucket) carry no blob commands at all — a flow that
43+
// must invoke blob capabilities from such a request (DeleteBucket's implicit
44+
// abort of in-flight multipart uploads) hides the request store so the
45+
// uploader falls back to the space authority captured at UploadPart.
46+
func WithoutProofStore(ctx context.Context) context.Context {
47+
return context.WithValue(ctx, proofStoreKey, nil)
48+
}
49+
4050
type requestContextKey struct{}
4151

4252
var requestKey any = requestContextKey{}

itest/forge_delete_test.go

Lines changed: 5 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ package itest
44

55
import (
66
"context"
7-
"os"
87
"strings"
98
"testing"
109
"time"
@@ -20,38 +19,14 @@ import (
2019
// piri /blob/release (claim release; deferred physical deletion once the
2120
// PDP aggregate root retires on-chain).
2221
//
23-
// The published piri/sprue images predate the removal handlers, so this
24-
// test injects working-tree builds of both and skips when they aren't
25-
// provided:
26-
//
27-
// (cd ../../piri && CGO_ENABLED=0 GOOS=linux go build -o /tmp/piri ./cmd)
28-
// (cd ../../sprue && CGO_ENABLED=0 GOOS=linux go build -o /tmp/sprue ./cmd/main.go)
29-
// ITEST_PIRI_BIN=/tmp/piri ITEST_SPRUE_BIN=/tmp/sprue \
30-
// go test -tags itest ./itest -run TestForgeDeleteReleasesNetworkBlob -v -timeout 900s
31-
//
32-
// Once images with the handlers publish, drop the env gate and the binary
33-
// injection.
22+
// Runs on the stock smelt-SDK images like every other itest. It needs
23+
// piri:main ≥ fil-forge/piri#30 (the /blob/release handler) and sprue:main ≥
24+
// fil-forge/sprue#33 (the forward); until piri#30 publishes, point
25+
// INGOT_ITEST_PIRI_IMAGE at a branch image (the forgeStack escape hatch).
3426
func TestForgeDeleteReleasesNetworkBlob(t *testing.T) {
35-
piriBin := os.Getenv("ITEST_PIRI_BIN")
36-
sprueBin := os.Getenv("ITEST_SPRUE_BIN")
37-
if piriBin == "" || sprueBin == "" {
38-
t.Skip("requires piri+sprue builds with the blob-removal chain: set ITEST_PIRI_BIN and ITEST_SPRUE_BIN (see test doc comment)")
39-
}
40-
4127
ctx := t.Context()
4228

43-
// The Curio-based piri requires a Postgres node (harmonydb) and, until
44-
// the published localdev image carries the mockrpc Ticket fix, an
45-
// overridable blockchain image.
46-
stackOpts := []stack.Option{
47-
stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}),
48-
stack.WithServiceBinary("piri", piriBin),
49-
stack.WithServiceBinary("upload", sprueBin),
50-
}
51-
if img := os.Getenv("ITEST_BLOCKCHAIN_IMAGE"); img != "" {
52-
stackOpts = append(stackOpts, stack.WithBlockchainImage(img))
53-
}
54-
s, ingotEndpoint := forgeStack(t, stackOpts...)
29+
s, ingotEndpoint := forgeStack(t)
5530
accessKey, secretKey := hiltProvisionTenant(t, ctx, s, "delete")
5631
cfg := forgeConfig(ingotEndpoint, accessKey, secretKey)
5732

itest/forge_multipart_deferred_test.go

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ package itest
55
import (
66
"bytes"
77
"context"
8-
"os"
98
"strings"
109
"testing"
1110
"time"
@@ -23,35 +22,14 @@ import (
2322
// (parked — outside the PDP pipeline), Complete must conclude (accept) every
2423
// part, and Abort must unwind parked blobs with /blob/abort → /blob/reject.
2524
//
26-
// Requires piri + sprue builds with the blob-removal chain (/blob/remove →
27-
// /blob/release, /blob/abort → /blob/reject); the published images predate
28-
// them, so inject working-tree binaries and skip otherwise (same gate as
29-
// TestForgeDeleteReleasesNetworkBlob):
30-
//
31-
// (cd ../../piri && CGO_ENABLED=0 GOOS=linux go build -o /tmp/piri ./cmd)
32-
// (cd ../../sprue && CGO_ENABLED=0 GOOS=linux go build -o /tmp/sprue ./cmd/main.go)
33-
// ITEST_PIRI_BIN=/tmp/piri ITEST_SPRUE_BIN=/tmp/sprue \
34-
// go test -tags itest ./itest -run TestForgeDeferredMultipart -v -timeout 1200s
25+
// Runs on the stock smelt-SDK images like every other itest. It needs
26+
// piri:main ≥ fil-forge/piri#30 (/blob/reject) and sprue:main ≥
27+
// fil-forge/sprue#33 (/blob/abort forwarding); until piri#30 publishes,
28+
// point INGOT_ITEST_PIRI_IMAGE at a branch image (the forgeStack escape
29+
// hatch).
3530
func TestForgeDeferredMultipart(t *testing.T) {
36-
piriBin := os.Getenv("ITEST_PIRI_BIN")
37-
sprueBin := os.Getenv("ITEST_SPRUE_BIN")
38-
if piriBin == "" || sprueBin == "" {
39-
t.Skip("requires piri+sprue builds with the /blob/abort chain: set ITEST_PIRI_BIN and ITEST_SPRUE_BIN (see test doc comment)")
40-
}
41-
4231
ctx := t.Context()
43-
// The Curio-based piri requires a Postgres node (harmonydb) and, until
44-
// the published localdev image carries the mockrpc Ticket fix, an
45-
// overridable blockchain image.
46-
stackOpts := []stack.Option{
47-
stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}),
48-
stack.WithServiceBinary("piri", piriBin),
49-
stack.WithServiceBinary("upload", sprueBin),
50-
}
51-
if img := os.Getenv("ITEST_BLOCKCHAIN_IMAGE"); img != "" {
52-
stackOpts = append(stackOpts, stack.WithBlockchainImage(img))
53-
}
54-
s, endpoint := forgeStack(t, stackOpts...)
32+
s, endpoint := forgeStack(t)
5533
accessKey, secretKey := hiltProvisionTenant(t, ctx, s, "mpdeferred")
5634
cl := sdkClient(forgeS3Conf(endpoint, accessKey, secretKey))
5735

itest/stack_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ func forgeStack(t *testing.T, extra ...stack.Option) (*stack.Stack, string) {
124124
t.Logf("using piri image override: %s", img)
125125
opts = append(opts, stack.WithPiriImage(img))
126126
}
127+
if img := os.Getenv("INGOT_ITEST_HILT_IMAGE"); img != "" {
128+
t.Logf("using hilt image override: %s", img)
129+
opts = append(opts, stack.WithHiltImage(img))
130+
}
127131
// Same idea one step earlier in the pipeline: mount a locally-built piri
128132
// binary (linux, static) over the image's /usr/bin/piri — validates an
129133
// unreleased piri/ucantone change with no image build at all.

itest/versity_multipart_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,9 @@ var completeMultipartXFail = []forgeCase{
160160
// The conditional matrix itself passes; the case then fails its bucket
161161
// teardown: upstream's teardown never aborts in-flight uploads, so
162162
// DeleteBucket implicitly aborts them (s3frontend/bucket.go), but the
163-
// /blob/abort goes out proofless — hilt's per-operation proof grants
164-
// carry blob.Abort for the S3 Abort operation only, not for
165-
// DeleteBucket/UploadPart. Promote once hilt's s3perm map grants
166-
// blob.Abort on bucket delete.
163+
// /blob/abort goes out proofless — hilt's s3perm map delegates no
164+
// blob.Abort today. fil-forge/hilt#36 adds it to the write set;
165+
// promote once a hilt image with it publishes.
167166
{name: "conditional_writes", fn: integration.CompleteMultipartUpload_conditional_writes},
168167
{name: "invalid_checksum_part", fn: integration.CompleteMultipartUpload_invalid_checksum_part},
169168
{name: "multiple_checksum_part", fn: integration.CompleteMultipartUpload_multiple_checksum_part},

s3frontend/multipart.go

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,12 @@ import (
2424

2525
msbucket "github.com/fil-forge/ingot/bucket"
2626
"github.com/fil-forge/ingot/bucketop"
27+
"github.com/fil-forge/ingot/internal/reqscope"
2728
"github.com/fil-forge/ingot/mst"
2829
"github.com/fil-forge/ingot/registry"
2930
"github.com/fil-forge/ingot/uploader"
3031
)
3132

32-
// minPartSize is S3's minimum size for every part except the last one,
33-
// enforced at CompleteMultipartUpload (EntityTooSmall).
34-
const minPartSize = 5 << 20
35-
3633
// defaultMaxListing is the S3 default and cap for max-parts / max-uploads.
3734
const defaultMaxListing = 1000
3835

@@ -295,10 +292,12 @@ func (b *Backend) CompleteMultipartUpload(ctx context.Context, input *s3.Complet
295292
requested = append(requested, sp)
296293
etagHasher.Write(sp.ETagMD5)
297294
}
298-
// Every part but the last must meet S3's 5 MiB minimum.
295+
// Every part but the last must meet S3's protocol-level 5 MiB minimum
296+
// (backend.MinPartSize — an S3 constant clients and SDKs assume, not an
297+
// operator knob).
299298
var total int64
300299
for i, sp := range requested {
301-
if i < len(requested)-1 && sp.Size < minPartSize {
300+
if i < len(requested)-1 && sp.Size < backend.MinPartSize {
302301
return s3response.CompleteMultipartUploadResult{}, "", s3err.GetAPIError(s3err.ErrEntityTooSmall)
303302
}
304303
total += sp.Size
@@ -390,7 +389,10 @@ func (b *Backend) CompleteMultipartUpload(ctx context.Context, input *s3.Complet
390389
// parts so a duplicate Complete is idempotent; the sweeper reaps it later.
391390
// Best-effort: a failed latch leaves the row in 'completing', which the
392391
// sweeper also treats as terminal after the TTL.
393-
_, _ = b.multipart.LatchSession(ctx, uploadID, registry.SessionCompleting, registry.SessionCompleted)
392+
if _, err := b.multipart.LatchSession(ctx, uploadID, registry.SessionCompleting, registry.SessionCompleted); err != nil {
393+
b.logger.Warn("latch session to completed failed; sweeper reaps the completing row after the TTL",
394+
zap.String("uploadID", uploadID), zap.Error(err))
395+
}
394396

395397
etagQ := `"` + etag + `"`
396398
return s3response.CompleteMultipartUploadResult{Bucket: &bucket, Key: &key, ETag: &etagQ}, "", nil
@@ -449,6 +451,13 @@ func (b *Backend) AbortMultipartUpload(ctx context.Context, input *s3.AbortMulti
449451
// drop the session, release its parts' now-unreferenced blobs. Used by
450452
// DeleteBucket's implicit abort of in-flight uploads.
451453
func (b *Backend) abortOpenSession(ctx context.Context, space did.DID, sess registry.MultipartSession) {
454+
// s3:DeleteBucket delegates no blob commands (hilt's s3perm maps it to
455+
// nil), so the surrounding request's proofs cannot authorize
456+
// /blob/abort; mask them so the uploader falls back to the blob
457+
// authority captured at UploadPart — the same resolution the
458+
// session-expiry sweeper uses. blob.Abort rides the write set as of
459+
// fil-forge/hilt#36.
460+
ctx = reqscope.WithoutProofStore(ctx)
452461
won, err := b.multipart.LatchSession(ctx, sess.UploadID, registry.SessionOpen, registry.SessionAborting)
453462
if err != nil || !won {
454463
return
@@ -522,11 +531,20 @@ func (b *Backend) cleanupPartBlobs(ctx context.Context, space did.DID, uploadID
522531
zap.String("digest", hex.EncodeToString(d)), zap.Error(aerr))
523532
}
524533
}
525-
_ = b.parks.DeletePark(ctx, d)
534+
if derr := b.parks.DeletePark(ctx, d); derr != nil {
535+
b.logger.Warn("delete park row failed",
536+
zap.String("digest", hex.EncodeToString(d)), zap.Error(derr))
537+
}
526538
}
527539
}
528-
_ = b.spool.Remove(mh.Multihash(d))
529-
_ = b.intents.DeleteIntent(ctx, d)
540+
if rerr := b.spool.Remove(mh.Multihash(d)); rerr != nil {
541+
b.logger.Warn("remove spooled blob failed",
542+
zap.String("digest", hex.EncodeToString(d)), zap.Error(rerr))
543+
}
544+
if derr := b.intents.DeleteIntent(ctx, d); derr != nil {
545+
b.logger.Warn("delete upload intent failed",
546+
zap.String("digest", hex.EncodeToString(d)), zap.Error(derr))
547+
}
530548
}
531549
}
532550

0 commit comments

Comments
 (0)