feat(s3): multipart checksums (FIL-620) - #68
Conversation
20f235f to
bc718e0
Compare
bc718e0 to
ffd2000
Compare
| // (each sealed CAR plus its sharded-dag-index blob), and hilt | ||
| // refuses to delete a space that still holds registrations — so | ||
| // release them first. The release is idempotent (removing an | ||
| // unregistered blob is a no-op), so a retried DeleteBucket is safe. |
There was a problem hiding this comment.
Wow 👌 I did not anticipate this being dealt with yet but awesome...
| // that still holds registrations). Segments whose ship registered nothing — | ||
| // unshipped, header-only, or shipped through a non-publishing uploader — | ||
| // carry no IndexDigest and contribute nothing. | ||
| func (m *Manager) ShippedSegmentDigests(ctx context.Context, bucket string) ([][]byte, error) { |
There was a problem hiding this comment.
Should this be an iterator? It could be big in buckets that have lived for a long time right? We'll probably find we need a bulk /blob/remove or GC beforehand somehow to allow this to complete in a reasonable time.
There was a problem hiding this comment.
It stays small by construction, so an iterator would not buy anything: retention prunes segment rows past the Retain window (default 6), so this enumerates roughly Retain shipped segments plus a handful of unshipped tails, regardless of the bucket's age.
The pruning is also the sharp edge you are gesturing at. A retired segment loses its row here, but its CAR and index stay registered in the space — the network copy is the durable read tier, so releasing at retirement would be wrong. That means this release pass only covers registrations the rows still describe: a long-lived bucket with more than Retain shipped segments still cannot be fully released from ingot's side, and the per-blob /blob/remove loop would not scale to that count anyway.
So agreed on where this lands: the durable fix is a space-scoped bulk release (or the tenant service tearing down a space it knows is being deleted) rather than ingot enumerating per blob — that would also sweep any registrations leaked by past best-effort release failures. Filed as #76. This pass keeps bucket deletion correct for the window the rows cover, which is also everything the conformance suite exercises.
| return nil | ||
| } | ||
| m.ShippedAt = shippedAt | ||
| m.IndexDigest = append([]byte(nil), indexDigest...) |
There was a problem hiding this comment.
I find slices.Clone(...) more readable for this.
There was a problem hiding this comment.
Done in a218882 — here and the matching site in the inmem fake.
0333289 to
fc29674
Compare
d926307 to
8a5c3d3
Compare
Per-part checksums at UploadPart, composite / full-object / default final
checksums at Complete, and the ListParts echo, mirroring the upstream posix
backend over versitygw's utils (composite checksum-of-checksums reader, CRC
combine) and s3err constructors:
- multipart_parts gains a checksum column (00009): the session algorithm's
base64 value, or the internal full-object CRC64NVME of an undeclared
session (never echoed; derives Complete's default final checksum).
- UploadPart negotiates the part algorithm against the session declaration
(mismatch / composite-without-checksum rejections), computes + validates
through a HashReader stack over the ingest pass, persists, and echoes.
- Complete verifies the request's checksum type and every part entry
(multiple / malformed / missing / wrong-algo / wrong-value), folds the
stored part checksums into the final value (COMPOSITE "-N" suffix or
FULL_OBJECT CRC combine), verifies a client-supplied final checksum, and
persists algorithm/value/type on the manifest; the idempotent re-Complete
returns the same checksum fields.
- ObjectManifest gains ChecksumType ("cy"); checksumFields stops hardcoding
FULL_OBJECT so GET/HEAD/ListObjectVersions echo the stored type (empty =
legacy full-object).
- ListParts echoes per-part checksums + the session algorithm/type, or the
literal "null" pair for an undeclared session.
- itest: the 19 checksum rows promote from the UploadPart / ListParts /
CompleteMultipartUpload XFail tables to pass; racey_data_integrity stays
xfail (load-sensitive concurrency, not checksums).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A copy without a checksum request carries the source's checksum triple (algorithm/value/type) to the destination; a request naming a different x-amz-checksum-algorithm replaces it, streaming the shared body through the new algorithm once for a full-object value. The CopyObjectResult now reports the destination checksum fields. Promotes the four checksum rows of the CopyObject XFail table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
Code comment on relevant code section Co-authored-by: ash <alan138@gmail.com>
8a5c3d3 to
c3deaf3
Compare
2 participants
Closes FIL-620: multipart checksum support, plus the CopyObject checksum rows and a
pre-existing DeleteBucket bug the promoted cases surfaced. Stacked on #66 (the S3
versioning implementation): review that PR first, this one adds three commits on top.
Multipart checksums (mirrors the upstream posix backend over versitygw's utils/s3err)
multipart_partsgains achecksumcolumn (migration 00009) —the session algorithm's base64 value, or an internal full-object CRC64NVME when the
session declared none (it derives Complete's default final checksum and is never echoed).
declaration (mismatch and composite-without-checksum rejections), computes and validates
through a HashReader stack in the single ingest pass, persists, and echoes the part
checksum.
validates every part entry (multiple / malformed / missing / wrong-algorithm /
wrong-value, with the upstream error taxonomy), folds the stored part checksums into the
final value — COMPOSITE checksum-of-checksums with the
-Nsuffix, or FULL_OBJECT CRCcombining without re-reading bytes — verifies a client-supplied final checksum, persists
algorithm/value/type on the manifest, and reports the checksum fields (the idempotent
re-Complete included).
"null"pair for a session that declared none.ObjectManifestgainsChecksumType(cborcy);GET/HEAD/ListObjectVersions echo the stored type instead of hardcoding FULL_OBJECT
(empty = legacy full-object).
CopyObject checksums
A copy carries the source's checksum triple verbatim; a request naming a different
x-amz-checksum-algorithmreplaces it, streaming the shared body through the newalgorithm once (full-object).
CopyObjectResultnow reports the destination checksum.DeleteBucket fix (pre-existing, surfaced by promotion)
A shipped catalog segment registers two blobs in the bucket's space — the sealed CAR and
its sharded-dag-index — and hilt refuses to delete a space holding registrations, so any
bucket that lived past the catalog seal age (5s) with a successful ship was undeletable.
CompleteMultipartUpload_should_verify_final_composite_checksum(~11s of commits) hitthis deterministically at upstream teardown. The ship path now persists the index blob's
digest (
ingot.segments.index_digest, migration 00010, threadedSubmitShard→FlushFunc→MarkSegmentShipped), and DeleteBucket releases both blobs per shippedsegment before asking hilt to delete the space (idempotent, so retries are safe).
Conformance ratchet
23 rows promoted from XFail to pass:
null_checksums(table now empty)racey_data_integrityremains XFail)Verification
make build,go vet ./...+-tags itest,gofmt -lclean,go mod tidy -diffclean,
make genno-op on the committedcbor_gen.go.GOWORK=off go test ./... -count=1green — new coverage for the FULL_OBJECT /COMPOSITE / default-CRC64NVME flows (cross-checked against a single PUT of the same
bytes), UploadPart negotiation and BadDigest, Complete's per-part validation taxonomy,
ListParts echo incl. the
"null"literals, idempotent re-Complete, part re-uploadsuperseding, CopyObject propagate-vs-replace, and the manager's shipped-segment digest
listing.
failing composite case passes on a fresh stack, and the full
TestForgeVersity+TestForgeDeferredMultipartrun is green — 304 passes, 0 failures, 59 expected skips,227s.
Follow-ups (noted, out of scope)
?partNumber=Nreturns the object checksum, not the part's — part checksumsare not retained past the session (no conformance row pins this).
🤖 Generated with Claude Code