backup: S3 encoder for buckets, objects, and blob reassembly (Phase 0a)#718
backup: S3 encoder for buckets, objects, and blob reassembly (Phase 0a)#718bootjp wants to merge 35 commits intofeat/backup-phase0a-dynamodbfrom
Conversation
The Phase 0a logical-backup decoder (internal/backup) needs to route each !s3|blob| record to its assembled object body without holding a live cluster. Today the package only exports parsers for object manifests and upload parts; blob keys are constructable via BlobKey / VersionedBlobKey but not parseable. ParseBlobKey decodes the 6-component form (BlobKey output) and the 7-component form (VersionedBlobKey output with partVersion != 0) into all components: bucket, generation, object, uploadID, partNo, chunkNo, partVersion. Truncated keys, malformed segments, and trailers that aren't either zero bytes or exactly one u64 are rejected with ok=false. Implementation is split into parseBlobKeyHead (the 6-component head) and parseOptionalPartVersion (the trailer) so cyclomatic complexity stays under the package cap without a //nolint marker. Tests cover the un-versioned round-trip, versioned round-trip, partVersion=0 fallback to un-versioned shape (matching VersionedBlobKey's documented behaviour), rejection of non-blob keys (bucket meta, object manifest, upload part, junk), and rejection of trailing-garbage keys.
Builds on PR #716 (DynamoDB) and PR #717 (s3keys.ParseBlobKey). Adds the S3 encoder for the Phase 0 logical-backup decoder. Snapshot prefixes handled: - !s3|bucket|meta|<bucket> -> s3/<bucket>/_bucket.json (live s3BucketMeta projected into the dump-format s3PublicBucket; cluster- internal fields like CreatedAtHLC and Generation stripped). - !s3|obj|head|<bucket><gen><object> -> s3/<bucket>/<object>.elastickv-meta.json (live s3ObjectManifest projected into the dump-format s3PublicManifest; UploadID and per-part chunk arrays stripped). - !s3|blob|<bucket><gen><object><uploadID><partNo><chunkNo>[<partVersion>]: spilled to a per-(bucket, object) scratch directory as it arrives; assembled at Finalize by walking sortChunkKeys output (partNo, partVersion, chunkNo) and concatenating into the final body file via writeFileAtomic-style tmp+rename. Memory: O(num_objects); body bytes never held in memory. - !s3|upload|meta|, !s3|upload|part|: excluded by default; --include-incomplete-uploads emits them as opaque {prefix, key, value} JSONL records under s3/<bucket>/_incomplete_uploads/. - !s3|bucket|gen|, !s3|gc|upload|, !s3route|: HandleIgnored no-op so the master pipeline can dispatch all !s3|* prefixes uniformly. Reserved-suffix collision: - A user object key ending in .elastickv-meta.json is rejected with ErrS3MetaSuffixCollision by default; --rename-collisions appends .user-data and records the rename in s3/<bucket>/KEYMAP.jsonl with KindMetaCollision so the on-disk reverse map is sound. - Per-bucket KEYMAP.jsonl is opened lazily and dropped if Finalize finds zero records (the spec's "omit empty file" rule). Body assembly: - Each blob chunk is written atomically to scratch as it arrives; the chunk path is registered in the object's chunkPaths map keyed by (uploadID, partNo, chunkNo, partVersion). - Finalize sorts the chunk keys by (partNo, partVersion, chunkNo) and concatenates the matching scratch files into a single <bodyPath> via tmp+rename. - The scratch tree is rm -rf'd on Finalize via deferred RemoveAll, so a successful run leaves nothing behind. Tests: bucket meta + single-chunk object round-trip, multipart ordering verification (chunks emitted out of order, body assembled in part/chunk order), orphan-chunks warning, .elastickv-meta.json collision rejection (default) and rename (with flag) including KEYMAP entry, malformed JSON rejection on both manifest and bucket paths, ignored-prefix no-op, --include-incomplete-uploads round- trip, default no _incomplete_uploads dir, versioned blob assembly.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an S3Encoder to process S3 backup records, including bucket metadata, object manifests, and blob chunks, with final assembly performed during the Finalize step. It also adds a ParseBlobKey utility for decoding S3 blob keys. Feedback focuses on critical improvements for data integrity and performance, such as filtering blobs by uploadID during assembly to avoid corruption from multiple upload attempts, optimizing file system operations by caching directory creation status and file handles, and correctly implementing the includeOrphans flag. Additionally, the reviewer advised against ignoring errors from resource cleanup methods.
| type s3ObjectState struct { | ||
| bucket string | ||
| generation uint64 | ||
| object string | ||
| manifest *s3PublicManifest | ||
| // chunkPaths maps (uploadID, partNo, chunkNo) -> scratch path. | ||
| chunkPaths map[s3ChunkKey]string | ||
| } |
There was a problem hiding this comment.
The s3ObjectState should store the uploadID from the object manifest to ensure that only blobs belonging to the active upload are assembled. Additionally, adding a dirCreated flag can optimize directory creation in HandleBlob.
type s3ObjectState struct {
bucket string
generation uint64
object string
uploadID string
dirCreated bool
manifest *s3PublicManifest
// chunkPaths maps (uploadID, partNo, chunkNo) -> scratch path.
chunkPaths map[s3ChunkKey]string
}| jl, err := openJSONL(filepath.Join(dir, "records.jsonl")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = closeJSONL(jl) }() |
There was a problem hiding this comment.
Opening and closing the records.jsonl file for every incomplete upload record is inefficient. Consider lazily opening the file and caching the writer in s3BucketState to amortize the I/O cost. Furthermore, if openJSONL uses os.Create, this logic will incorrectly truncate the file on every call, preserving only the last record. Avoid silently dropping entries during serialization to ensure state consistency.
References
- Avoid silently dropping entries during serialization or normalization. If placeholder or incomplete entries are valid in some contexts, provide a specific, more lenient handling path for them to ensure they are preserved on disk and avoid state inconsistencies.
| func (st *s3ObjectState) attachManifestParts(_ string, _ []s3LivePart) { | ||
| // Intentionally empty: assembleObjectBody consumes the manifest | ||
| // directly via st.manifest at Finalize. Kept as a hook so the | ||
| // callsite reads symmetrically with HandleBlob. | ||
| } |
There was a problem hiding this comment.
Capture the uploadID from the manifest so it can be used to filter blobs during assembly.
func (st *s3ObjectState) attachManifestParts(uploadID string, _ []s3LivePart) {
st.uploadID = uploadID
}| chunks := sortChunkKeys(obj.chunkPaths) | ||
| for _, k := range chunks { | ||
| path := obj.chunkPaths[k] | ||
| if err := appendFile(tmp, path); err != nil { | ||
| _ = tmp.Close() | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Filter the chunks by the uploadID specified in the manifest. This prevents data corruption if the snapshot contains blobs from multiple upload attempts for the same object key. Additionally, ensure that errors from tmp.Close() are not silently ignored.
chunks := sortChunkKeys(obj.chunkPaths)
for _, k := range chunks {
if k.uploadID != obj.uploadID {
continue
}
path := obj.chunkPaths[k]
if err := appendFile(tmp, path); err != nil {
if closeErr := tmp.Close(); closeErr != nil {
// Log the error as per repository rules
}
return err
}
}References
- Do not silently ignore errors from Close() methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
| dir := filepath.Join(s.scratchRoot, EncodeSegment([]byte(bucket)), EncodeSegment([]byte(object))) | ||
| if err := os.MkdirAll(dir, 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode | ||
| return errors.WithStack(err) | ||
| } |
There was a problem hiding this comment.
Calling os.MkdirAll for every blob chunk is inefficient. Use the dirCreated flag in s3ObjectState to perform this operation only once per object.
if !st.dirCreated {
dir := filepath.Join(s.scratchRoot, EncodeSegment([]byte(bucket)), EncodeSegment([]byte(object)))
if err := os.MkdirAll(dir, 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode
return errors.WithStack(err)
}
st.dirCreated = true
}| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = closeJSONL(jl) }() |
There was a problem hiding this comment.
The error from closeJSONL is silently ignored. For a writable resource, failing to close properly might mean data was not flushed to disk. Per general rules, errors from Close() methods should not be silently ignored.
References
- Do not silently ignore errors from Close() methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
| if obj.manifest == nil { | ||
| s.emitWarn("s3_orphan_chunks", | ||
| "bucket", b.name, | ||
| "object", obj.object, | ||
| "chunks", len(obj.chunkPaths), | ||
| "hint", "blob chunks present but no !s3|obj|head record matched") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The includeOrphans flag is currently ignored. The implementation only emits a warning but does not surface the orphan chunks under _orphans/ as described in the documentation and the WithIncludeOrphans method.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 554dd42276
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // vs `path/to/sub`) are deferred until the master pipeline | ||
| // detects them across multiple manifests; this PR's per-object | ||
| // flush trusts the caller's collision detection. | ||
| return obj.object, "", nil |
There was a problem hiding this comment.
resolveObjectFilename returns the raw S3 object key and flushObject then does filepath.Join(bucketDir, objectName), so object names like ../x or /tmp/x will escape the bucket directory and write outside the dump root. Because S3 object keys are user-controlled, this enables path traversal/arbitrary file overwrite during backup decode and can corrupt host files rather than just the export tree.
Useful? React with 👍 / 👎.
| chunks := sortChunkKeys(obj.chunkPaths) | ||
| for _, k := range chunks { | ||
| path := obj.chunkPaths[k] | ||
| if err := appendFile(tmp, path); err != nil { |
There was a problem hiding this comment.
assembleObjectBody concatenates every entry in obj.chunkPaths after sorting, but never filters by the manifest's upload_id/declared parts, so stale chunks from a different upload attempt for the same (bucket,generation,object) can be merged into the output body. This produces corrupted object bytes whenever old or concurrent multipart chunks are still present in the blob keyspace.
Useful? React with 👍 / 👎.
| jl, err := openJSONL(filepath.Join(dir, "records.jsonl")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { _ = closeJSONL(jl) }() |
There was a problem hiding this comment.
HandleIncompleteUpload reopens records.jsonl on every record via openJSONL(...), and that helper uses create/truncate semantics, so each call overwrites prior lines. With --include-incomplete-uploads, only the last upload-meta/part record survives per bucket, defeating the stated “one JSON line per record” behavior and losing forensic data.
Useful? React with 👍 / 👎.
…to feat/backup-phase0a-s3
Three correctness bugs and several quality issues, all valid. CRITICAL: path traversal (Codex P1 #425). S3 object keys are user-controlled. A key like "../../../etc/passwd" would, under the prior filepath.Join-and-write code, escape the bucket directory and overwrite host files. Added safeJoinUnderRoot which Cleans the joined path and asserts it stays rooted under the bucket dir; ".." traversal is rejected with ErrS3MalformedKey. Absolute paths (leading "/") are normalised under the bucket dir by filepath.Join, which is the safest outcome. Tests: TestS3_PathTraversalAttemptRejected, TestS3_AbsolutePathObjectKeyConfinedUnderBucket. CRITICAL: stale upload-id chunks merged into body (Codex P1 #500, Gemini HIGH #106/#476/#504). A snapshot mid-delete-and-recreate or mid-retry can carry blob chunks for multiple upload attempts under the same (bucket, gen, object). The prior assembleObjectBody concatenated every chunk regardless of upload_id, producing corrupted bytes. Now: - s3ObjectState gains uploadID; HandleObjectManifest sets it. - New filterChunksForManifest takes the chunkPaths map and the manifest's uploadID, returns only matching chunks sorted by (partNo, partVersion, chunkNo). Stale-uploadID chunks never enter the assembled body. Test: TestS3_StaleUploadIDChunksFilteredFromAssembledBody. CRITICAL: incomplete-uploads file truncated per record (Codex P2 #318, Gemini HIGH+MEDIUM #318). HandleIncompleteUpload re-opened records.jsonl on every call; openJSONL uses os.Create which truncates. Only the last record survived per bucket. Now: - s3BucketState carries an incompleteUploadsJL *jsonlFile lazily opened on the first record and cached. - flushBucket closes it and surfaces the error (was silently ignored). Test: TestS3_IncompleteUploadsAppendsAcrossCalls (3 records, 3 lines on disk). QUALITY: - Gemini MEDIUM #285 (MkdirAll per blob): s3ObjectState gains a scratchDirCreated bool; HandleBlob runs MkdirAll once. - Gemini MEDIUM #318 (closeJSONL error ignored): keymap and incomplete-uploads writers now surface close errors via closeBucketKeymap / explicit closeJSONL return-check. - Gemini MEDIUM #386 (includeOrphans flag ignored): orphan chunks for objects without manifests now write to <bucket>/_orphans/<encoded-object>/<chunk>.bin under WithIncludeOrphans(true). Test: TestS3_OrphanChunksWrittenWhenIncludeOrphans. Also removed the now-unused attachManifestParts placeholder and sortChunkKeys helper; their logic moved into filterChunksForManifest which combines uploadID filtering with the sort.
|
3 critical correctness bugs + 3 quality issues addressed in Critical Codex P1 #425 — Path traversal. A user-supplied key like Codex P1 #500 + Gemini HIGH #106/#476/#504 — Stale upload-id chunks. A snapshot mid-delete-and-recreate or mid-retry can carry blob chunks for multiple upload attempts under the same Codex P2 #318 + Gemini HIGH+MEDIUM #318 — Truncated Quality Gemini MEDIUM #285 — Gemini MEDIUM #318 — closeJSONL error. Gemini MEDIUM #386 — Removed the now-unused @claude review |
|
Claude finished @bootjp's task in 4m 18s —— View job Review —
|
| Item | Severity | Location |
|---|---|---|
"." object key hits EISDIR instead of ErrS3MalformedKey |
Low | s3.go:safeJoinUnderRoot |
Silent bypass when manifest upload_id is empty |
Low | s3.go:HandleObjectManifest |
Scratch dir recomputed on every blob after gate set |
Negligible | s3.go:HandleBlob |
Orphan chunks loaded into memory via os.ReadFile |
Low | s3.go:flushOrphanObject |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ae3289ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if manifestUploadID != "" && k.uploadID != manifestUploadID { | ||
| continue | ||
| } | ||
| keys = append(keys, k) |
There was a problem hiding this comment.
Fresh evidence: this revision now filters by upload_id, but filterChunksForManifest still appends every (partNo, partVersion, chunkNo) for that upload without checking the manifest’s declared parts. In S3, overwritten parts can leave older blob versions present until async cleanup runs, so this logic can concatenate stale part versions (or silently miss required chunks) and produce corrupted object bodies even when the manifest points to a single canonical part version.
Useful? React with 👍 / 👎.
| joined := filepath.Clean(filepath.Join(cleanRoot, rel)) | ||
| rootSep := cleanRoot + string(filepath.Separator) | ||
| if joined != cleanRoot && !strings.HasPrefix(joined, rootSep) { |
There was a problem hiding this comment.
safeJoinUnderRoot uses filepath.Clean(filepath.Join(...)), which collapses keys like a/../b to b and . to the bucket root. Because S3 object keys treat these bytes literally, this can make distinct keys collide/overwrite each other, and keys that clean to the root can cause finalize-time write errors instead of emitting the object data.
Useful? React with 👍 / 👎.
| // flush trusts the caller's collision detection. Path-traversal | ||
| // sanitisation runs in safeJoinUnderRoot, downstream of this | ||
| // function, where the bucket-directory root is in scope. | ||
| return obj.object, "", nil |
There was a problem hiding this comment.
Object state is tracked per (object,generation), but resolveObjectFilename drops generation and returns only obj.object, so two generations of the same bucket/object can write to the same output path and whichever map iteration runs last wins. Since old-generation keys are intentionally isolated rather than guaranteed absent, this creates nondeterministic exports when stale generations coexist in the snapshot.
Useful? React with 👍 / 👎.
…to feat/backup-phase0a-s3
Three follow-up issues from Codex on top of round 1. #619 Codex P1 -- assemble only manifest-declared parts. filterChunksForManifest previously matched on uploadID only; the manifest declares specific (partNo, partVersion) tuples, and S3's overwrite-then-async-cleanup window can leave older partVersion chunks present under the same (bucket, generation, object, uploadID). Mixing them produces corrupted bytes. s3ObjectState gains declaredParts map[s3PartKey]struct{}, populated by HandleObjectManifest. filterChunksForManifest takes the set and restricts emission to declared (partNo, partVersion) tuples. Test TestS3_StalePartVersionExcludedFromAssembledBody asserts a stale partVersion=7 cannot leak when the manifest declares partVersion=9. #497 Codex P2 -- preserve dot segments. safeJoinUnderRoot used filepath.Clean which collapses "a/../b" to "b". S3 treats those bytes literally; "a/../b" and "b" are distinct keys that would have silently merged into one output file. Now explicitly rejects any object key whose segments are "." or ".." with ErrS3MalformedKey. NUL bytes also rejected. Test TestS3_DotSegmentObjectKeyRejected covers the four forms ("a/../b", "a/./b", "..", "."). #521 Codex P2 -- cross-generation collision. s3BucketState gains activeGen captured from the bucket-meta record. flushBucket suppresses objects whose generation differs from activeGen (under --include-orphans, those flow to _orphans/; by default they're dropped with an s3_stale_generation_objects warning). Tests TestS3_StaleGenerationObjectExcluded. flushBucket cyclomatic complexity stayed under the cap by extracting flushBucketObjects.
|
3 Codex follow-ups addressed in P1 #619 — Assemble only manifest-declared parts. P2 #497 — Preserve dot segments. P2 #521 — Cross-generation collision.
@claude review |
|
Claude finished @bootjp's task in 3m 49s —— View job Claude finished @bootjp's task in 4m 18s — Review —
|
| Item | Status |
|---|---|
Silent bypass when manifest.upload_id == "" |
Still present (by design — documented in comment) |
Scratch dir recomputed after scratchDirCreated gate set |
Still present (negligible) |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Still present (low priority) |
Summary
All three Codex P1/P2 findings from round 2 (declaredParts allow-list, dot-segment rejection, generation guard) are correctly implemented and tested. No new blocking issues found. The PR is ready to merge once the stacked bases land.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ee22efad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // flush trusts the caller's collision detection. Path-traversal | ||
| // sanitisation runs in safeJoinUnderRoot, downstream of this | ||
| // function, where the bucket-directory root is in scope. | ||
| return obj.object, "", nil |
There was a problem hiding this comment.
When a bucket contains both path/to and path/to/sub, resolveObjectFilename returns the raw object key for both objects, so flushObject later tries to materialize mutually incompatible filesystem paths (one as a file, one as a parent directory). On POSIX this causes MkdirAll/Rename failures during finalize, so valid S3 keysets can make the backup fail instead of applying the documented leaf-data rename + KEYMAP record strategy.
Useful? React with 👍 / 👎.
…to feat/backup-phase0a-s3
Codex P1 #615: when a bucket contains both `path/to` and `path/to/sub`, the natural-path mapping puts the shorter key at a file path and the longer key requires that path to be a parent directory. POSIX cannot represent both; the prior code would fail with EEXIST/ENOTDIR at MkdirAll/Rename during finalize, breaking the documented leaf-data-suffix rename strategy. Pre-flush computeDirPrefixes() walks every active-gen object key and accumulates each parent prefix (a key "a/b/c" contributes "a" and "a/b"). flushBucketObjects checks whether each object's key is itself a prefix consumed by another active key; if so, flushObjectWithCollision is called with needsLeafDataRename=true, which routes resolveObjectFilename onto the existing S3LeafDataSuffix path. KEYMAP.jsonl records the rename with KindS3LeafData so restore can reverse it. flushObject (the no-collision wrapper) was inlined into flushObjectWithCollision since it was unused after the call sites moved to the explicit-flag form. Test TestS3_FileVsDirectoryKeyCollisionGetsLeafDataRename emits "path/to" and "path/to/sub", asserts: - "path/to/sub" lands at its natural path with body "CHILD" - "path/to" is renamed to "path/to.elastickv-leaf-data" with body "LEAF" - KEYMAP.jsonl carries one record with KindS3LeafData and original "path/to"
|
Codex P1 #615 addressed in
@claude review |
|
Claude finished @bootjp's task in 3m 30s —— View job Claude finished @bootjp's task —
Review —
|
| Severity | Location | |
|---|---|---|
Renamed path could itself be a directory prefix for another user object (e.g., path/to, path/to/sub, path/to.elastickv-leaf-data/x): the renamed path/to writes path/to.elastickv-leaf-data as a file, then path/to.elastickv-leaf-data/x needs it as a directory → ENOTDIR. Error surfaces correctly but is opaque (no ErrS3MalformedKey mention). |
Low | s3.go:flushObjectWithCollision |
No test for 3-object multi-rename ("a", "a/b", "a/b/c") — the algorithm handles it but the test suite doesn't verify both KEYMAP records are emitted. |
Low | s3_test.go |
emitObjectAtGen fixture emits part_version as 0 (default); the new declaredParts filter for {partNo:1, partVersion:0} passes correctly via ParseBlobKey → unversioned branch (partVersion=0). The filter is exercised but at the same version as the "no filter" escape hatch. |
Negligible | s3_test.go:emitObjectAtGen |
Carry-overs from prior rounds
| Item | Status |
|---|---|
Silent bypass when manifest.upload_id == "" |
Still present (by design, documented) |
Scratch dir recomputed after scratchDirCreated gate set |
Still present (negligible) |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Still present (low priority) |
The fix is correct and ready to merge once stacked bases land. None of the remaining observations are blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c44292fba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| chunks := filterChunksForManifest(obj.chunkPaths, obj.uploadID, obj.declaredParts) | ||
| for _, k := range chunks { | ||
| path := obj.chunkPaths[k] | ||
| if err := appendFile(tmp, path); err != nil { |
There was a problem hiding this comment.
assembleObjectBody filters/sorts whatever chunk files happen to exist and writes them, but it never verifies that every manifest-declared chunk is present (e.g., via parts[].chunk_count/chunk_sizes). If one or more blob keys are missing in the snapshot (partial data, race, or corruption), finalize still succeeds and emits a truncated object body without any error signal, which silently corrupts the backup output.
Useful? React with 👍 / 👎.
| for _, seg := range strings.Split(rel, "/") { | ||
| switch seg { | ||
| case ".", "..": | ||
| return "", errors.Wrapf(ErrS3MalformedKey, |
There was a problem hiding this comment.
The path guard rejects . and .. segments, but it allows empty segments, and filepath.Join then normalizes keys like a/, a, and a//b onto the same filesystem path. Since S3 treats these keys as distinct, this causes collisions/last-write-wins overwrites during finalize for valid object names containing repeated or trailing / characters.
Useful? React with 👍 / 👎.
…est' into feat/backup-phase0a-s3
…718, round 4) Two Codex P1 follow-ups. #729 -- chunk completeness check. assembleObjectBody filtered + sorted whatever chunk files happened to exist and wrote them, but never verified the manifest's declared chunk_count was actually present. A partial / racy / corrupted snapshot would silently emit a truncated body. Added verifyChunkCompleteness which counts chunks per (partNo, partVersion) and asserts the count matches manifest.parts[].chunk_count AND the highest chunkNo equals chunk_count-1. Mismatch surfaces as ErrS3IncompleteBlobChunks before any bytes are written. The declaredParts map's value type changed from struct{} to s3DeclaredPart{chunkCount} to carry the contract. #614 -- empty slash segments. safeJoinUnderRoot rejected `.` and `..` but allowed empty segments ("a//b", "a/", trailing "/"). filepath.Join collapses these, so distinct S3 keys would silently overwrite each other at finalize. The dot-segment guard now also rejects "" segments anywhere except the leading position (where a leading "/" produces an initial empty segment that filepath.Join handles safely under the already-tested absolute-path-confined-under-bucket behaviour). Tests: TestS3_IncompleteChunksRejected, TestS3_EmptySlashSegmentsRejected (covers a//b, a/, /a//, x/).
|
2 Codex P1 follow-ups addressed in #729 — Chunk completeness. #614 — Empty slash segments. @claude review |
|
Claude finished @bootjp's task in 3m 43s —— View job Review —
|
| chunks for part | declared count | count check | maxIndex check | outcome |
|---|---|---|---|---|
| chunkNo=0, chunkNo=2 (missing 1) | 3 | 2 ≠ 3 → fail | — | error ✅ |
| chunkNo=0 × 3 (dup of 0) | 3 | 3 = 3 | maxIndex+1=1 ≠ 3 → fail | error ✅ |
| chunkNo=0, chunkNo=1, chunkNo=2 | 3 | 3 = 3 | maxIndex+1=3 = 3 | pass ✅ |
| no chunks | N > 0 | 0 ≠ N → fail | — | error ✅ |
The nil escape hatch is safe and correctly limits backward-compat scope to tests that pre-date the manifest-parts feature.
Minor — want.chunkCount == 0 bypass. The guard want.chunkCount > 0 skips validation for zero-chunk declared parts. If a snapshot contains stale chunks for a part declared with chunk_count=0, filterChunksForManifest passes them through (key is in declaredParts), but verifyChunkCompleteness skips the check — those bytes are silently assembled into the output. A chunk_count=0 part is pathological in the first place (an S3 part with no data) so this is an extreme edge case, not a regression.
Fix 2 — safeJoinUnderRoot empty segment rejection ✅
The guard now rejects "" at i != 0 (s3.go:688). The leading-"" carve-out for rel starting with / is correct: filepath.Join(cleanRoot, "/a/b") = filepath.Clean(cleanRoot + "/" + "/a/b") = cleanRoot/a/b on POSIX — the absolute-path is absorbed under the bucket dir, which is covered by TestS3_AbsolutePathObjectKeyConfinedUnderBucket from round 1. The residual boundary check (strings.HasPrefix(joined, rootSep)) still guards against any future join anomaly.
All four test cases traced:
| key | segs |
first "" at i |
result |
|---|---|---|---|
"a//b" |
["a","","b"] |
i=1 | ErrS3MalformedKey ✅ |
"a/" |
["a",""] |
i=1 | ErrS3MalformedKey ✅ |
"/a//" |
["","a","",""] |
i=2 | ErrS3MalformedKey ✅ |
"x/" |
["x",""] |
i=1 | ErrS3MalformedKey ✅ |
The flushOrphanObject path is exempt (it uses EncodeSegment([]byte(obj.object)) for the directory name, not the raw key), so no gap there.
The error propagates cleanly: safeJoinUnderRoot → flushObjectWithCollision → flushBucketObjects → Finalize with no swallowing — TestS3_EmptySlashSegmentsRejected asserts errors.Is(err, ErrS3MalformedKey) on the Finalize call. ✓
Carry-overs from prior rounds
| Item | Status |
|---|---|
Silent bypass when manifest.upload_id == "" |
Still present (by design, documented) |
Scratch dir recomputed after scratchDirCreated gate set |
Still present (negligible) |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Still present (low priority) |
No test for --include-orphans + stale-gen routing |
Still present (low priority) |
Both P1 findings are correctly fixed and tested. No new blocking issues. The PR is ready to merge once the stacked bases (feat/backup-phase0a-dynamodb, feat/s3keys-parse-blob-key) land.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba33df8e5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if i != 0 { | ||
| return "", errors.Wrapf(ErrS3MalformedKey, | ||
| "object name has empty path segment %q", rel) |
There was a problem hiding this comment.
safeJoinUnderRoot intentionally permits an empty first segment, so object keys like /a are accepted and then normalized by filepath.Join to the same output path as a. Because S3 treats /a and a as distinct keys, a bucket containing both will silently collide and whichever object flushes last wins, corrupting backup output without any keymap record. This is triggered whenever a bucket has leading-slash keys alongside equivalent non-leading-slash keys.
Useful? React with 👍 / 👎.
…est' into feat/backup-phase0a-s3
Codex P1 round 5 (commit 2f87b84): `safeJoinUnderRoot` permitted an empty first segment so leading-slash keys like `/a` were accepted and then normalised by `filepath.Join` to the same output path as `a`. S3 treats `/a` and `a` as two distinct keys (the literal byte '/' is part of the key), so a bucket containing both produced last-flush-wins corruption with no KEYMAP record. The "absolute path collapses safely under bucket dir" comfort the previous behaviour leaned on was false comfort: the collapse silently merged distinct user data. Now empty segments are refused everywhere — leading, mid-path, and trailing — with ErrS3MalformedKey. Operators with leading-slash keys must rename them in S3 first. The previous test `TestS3_AbsolutePathObjectKeyConfinedUnderBucket` (which asserted the wrong behaviour) is replaced by `TestS3_LeadingSlashObjectKeyRejected`.
|
@claude review Round 5 fix for Codex P1 on commit 2f87b84:
Now empty segments are rejected everywhere — leading, mid-path, and trailing — with
|
|
Claude finished @bootjp's task in 2m 5s —— View job Review —
|
| Item | Status |
|---|---|
Silent bypass when manifest.upload_id == "" |
Still present (by design, documented) |
Scratch dir recomputed after scratchDirCreated gate |
Still present (negligible) |
Orphan chunks loaded via os.ReadFile |
Still present (low priority) |
No test for --include-orphans + stale-gen routing |
Still present (low priority) |
The Codex P1 finding (leading-slash collision) is correctly fixed and tested. One stale comment worth cleaning up but nothing blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2febd423ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| segs := strings.Split(rel, "/") | ||
| for _, seg := range segs { | ||
| switch seg { | ||
| case ".", "..": |
There was a problem hiding this comment.
In safeJoinUnderRoot, validation only splits object keys on /, but filepath.Join treats \ as a path separator on Windows. That means keys like a\\..\\b bypass the dot-segment check and normalize to the same output path as b, causing distinct S3 keys to collide/overwrite silently when the decoder runs on Windows. This should validate path segments using OS-aware separators (or reject \ in keys) before joining.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d33a6511
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f, err := os.Create(path) //nolint:gosec // path composed from output root | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| b.keymap = NewKeymapWriter(f) |
There was a problem hiding this comment.
recordKeymap opens KEYMAP.jsonl with os.Create and only stores a KeymapWriter; later closeBucketKeymap calls KeymapWriter.Close(), which flushes buffers but does not close the underlying file descriptor. In dumps that produce keymaps for many buckets (e.g., repeated collision renames), descriptors accumulate until EMFILE and subsequent bucket flushes fail, leaving backup output incomplete.
Useful? React with 👍 / 👎.
| func (s *S3Encoder) recordKeymap(b *s3BucketState, bucketDir, encodedFilename string, original []byte, kind string) error { | ||
| if b.keymap == nil { | ||
| path := filepath.Join(bucketDir, "KEYMAP.jsonl") | ||
| f, err := os.Create(path) //nolint:gosec // path composed from output root |
There was a problem hiding this comment.
This path uses os.Create directly for KEYMAP.jsonl, which follows symlinks. If an attacker (or stale prior run) places a symlink at that location, finalize can truncate/overwrite an arbitrary target file when the first keymap record is written. Other backup sidecars already use openSidecarFile to prevent this class of overwrite; KEYMAP should use the same guarded open path.
Useful? React with 👍 / 👎.
…' into feat/backup-phase0a-s3
Codex round 9 raised two issues on commit ab38eb0: 1. P1: closeBucketKeymap leaked file descriptors. recordKeymap stored only the *KeymapWriter; closeBucketKeymap called KeymapWriter.Close() which flushes the bufio buffer but does NOT close the underlying *os.File. A dump producing keymaps for many buckets accumulated descriptors until EMFILE, after which subsequent bucket flushes failed and the dump output was incomplete. Track the *os.File on s3BucketState and close it from closeBucketKeymap alongside the KeymapWriter flush. 2. P2: recordKeymap used os.Create for KEYMAP.jsonl, which follows symlinks and clobbers hard links. The redis encoder already routes through openSidecarFile for the same kind of sidecar; mirror that path so a stale prior run (or local adversary) cannot turn a missing KEYMAP into an arbitrary-write primitive against /etc/passwd or similar. Test: TestS3_KeymapRefusesSymlinkAtFinalize pre-creates KEYMAP.jsonl as a symlink to a bait file, drives a meta-suffix rename (so recordKeymap fires), and asserts both that the finalize returns the symlink-refusal error and that the bait file is untouched.
…' into feat/backup-phase0a-s3
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@claude review Round 7 fixes for Codex round 9 on commit ab38eb0 (commit
|
|
Claude finished @bootjp's task in 2m 29s —— View job Review —
|
| File | Opener |
|---|---|
KEYMAP.jsonl |
openSidecarFile ← this fix |
_incomplete_uploads/records.jsonl |
openSidecarFile (via openJSONL) |
The Unix path uses O_NOFOLLOW atomically — no TOCTOU race. The Windows path uses Lstat-then-OpenFile with the documented acceptable residual window. Both produce the "refusing to overwrite symlink at %s" error string the test checks. ✓
return err (not errors.WithStack(err)) at s3.go:769 is correct: openSidecarFile already wraps the error via cockroachdberr.WithStack. ✓
Test: TestS3_KeymapRefusesSymlinkAtFinalize ✅
The test (s3_test.go:292) correctly:
- Pre-creates
bucketDir/KEYMAP.jsonlas a symlink to a bait file - Triggers
recordKeymapviaWithRenameCollisions(true)+ a.elastickv-meta.jsonkey - Asserts
Finalizereturns an error containing"refusing to overwrite symlink" - Asserts
os.ReadFile(bait)still contains"stay-out"— the bait was not clobbered ✓
The propagation path is clean: Finalize → flushBucket → closeBucketKeymap is called first (line 496), but recordKeymap fires earlier via flushObjectWithCollision → the error surfaces there and bubbles through flushBucketObjects → flushBucket → Finalize. ✓
Stale comment (carry-over from rounds 5 and 6)
s3.go:706–709 still reads:
// Use filepath.Join here — its only behavioural change vs. raw
// concatenation after the dot-segment guard above is normalising
// a leading "/" off `rel` (which is what we want: absolute-path
// keys collapse safely under bucketDir).Since round 5 (leading-slash rejection), no leading / can reach filepath.Join — it is caught as an empty segment by the guard. The phrase "which is what we want: absolute-path keys collapse safely" actively contradicts the current design. Still low priority, still not blocking.
Carry-overs from prior rounds
| Item | Status |
|---|---|
Silent bypass when manifest.upload_id == "" |
Still present (by design, documented) |
Scratch dir recomputed after scratchDirCreated gate |
Still present (negligible) |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Still present (low priority) |
No test for --include-orphans + stale-gen routing |
Still present (low priority) |
Stale comment at safeJoinUnderRoot re: absolute-path confinement |
Still present (low priority) |
Both P1 and P2 findings are correctly fixed and tested. No new blocking issues. The PR is ready to merge once the stacked bases (feat/backup-phase0a-dynamodb, feat/s3keys-parse-blob-key) land.
…at/backup-phase0a-s3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dc688429b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if needsLeafDataRename { | ||
| return obj.object + S3LeafDataSuffix, KindS3LeafData, nil | ||
| } |
There was a problem hiding this comment.
When needsLeafDataRename is true, this always rewrites "<object>" to "<object>.elastickv-leaf-data" without checking whether that target is already a real S3 object key in the same bucket. A valid bucket containing both path/to + path/to/sub and an existing path/to.elastickv-leaf-data will have two distinct objects mapped to one output path, so finalize becomes last-write-wins and backup data is silently lost.
Useful? React with 👍 / 👎.
| if strings.HasSuffix(obj.object, S3MetaSuffixReserved) { | ||
| if !s.renameCollisions { | ||
| return "", "", errors.Wrapf(ErrS3MetaSuffixCollision, | ||
| "bucket %q object %q", b.name, obj.object) | ||
| } | ||
| return obj.object + ".user-data", KindMetaCollision, nil | ||
| } |
There was a problem hiding this comment.
In rename-collision mode, objects ending with .elastickv-meta.json are rewritten to "<object>.user-data" unconditionally. If the bucket already contains a real key with that .user-data suffix, both objects are emitted to the same filesystem path and one overwrites the other, and KEYMAP cannot represent both originals, so restore cannot reconstruct the original keyset.
Useful? React with 👍 / 👎.
| st.manifest = &s3PublicManifest{ | ||
| FormatVersion: 1, | ||
| ETag: live.ETag, | ||
| SizeBytes: live.SizeBytes, | ||
| ContentType: live.ContentType, | ||
| ContentEncoding: live.ContentEncoding, | ||
| CacheControl: live.CacheControl, | ||
| ContentDisposition: live.ContentDisposition, | ||
| UserMetadata: live.UserMetadata, | ||
| } |
There was a problem hiding this comment.
The decoded live manifest includes last_modified_hlc, and s3PublicManifest exposes a last_modified field, but this struct literal never sets it. As a result every exported .elastickv-meta.json drops object modification time metadata, reducing backup fidelity for tooling that relies on the sidecar to preserve HEAD-visible timestamps.
Useful? React with 👍 / 👎.
…at/backup-phase0a-s3
…to feat/backup-phase0a-s3
…, round 8) Codex round 9 raised three issues on commit 1dc6884: 1. P1: Leaf-data rename collided with real user keys. `needsLeafDataRename` rewrote `<object>` to `<object>.elastickv-leaf-data` without checking whether a real key with that suffix already existed in the same bucket. Two distinct objects could be mapped to one filesystem path and finalize was last-flush-wins. resolveObjectFilename now consults a per-bucket set of active-gen object keys via computeActiveGenObjectKeys; if the rename target is a real key, surface ErrS3MetaSuffixCollision. 2. P1: Meta-suffix rename collided too. Same root cause: rename-collisions mode rewrote `<object>.elastickv-meta.json` to `<object>.elastickv-meta.json.user-data`. If that suffixed key was itself a real object, the rename target collided. The same set lookup now refuses the rename and surfaces ErrS3MetaSuffixCollision. 3. P2: last_modified was never populated. The decoded s3LiveManifest carried last_modified_hlc, but the s3PublicManifest projection silently dropped it, so every exported sidecar lost the HEAD-visible Last-Modified timestamp. formatHLCAsRFC3339Nano extracts the millisecond half (HLC >> 16, see kv/hlc.go) and renders RFC3339Nano UTC, matching S3 HEAD semantics. HLC zero maps to "" so omitempty drops the field rather than emitting "1970-01-01T00:00:00Z". Tests: - TestS3_LeafDataRenameRejectsCollidingUserKey - TestS3_MetaSuffixRenameRejectsCollidingUserKey - TestS3_LastModifiedSidecarPopulated
…to feat/backup-phase0a-s3
|
@claude review Round 8 fixes for Codex round 9 on commit 1dc6884 (commit
|
|
Claude finished @bootjp's task in 2m 41s —— View job Review —
|
| Item | Status |
|---|---|
Stale comment at safeJoinUnderRoot:763 re: "absolute-path keys collapse safely" |
Still present (low priority) |
joined == cleanRoot dead branch at safeJoinUnderRoot:769 |
Still present (low priority) |
Silent bypass when manifest.upload_id == "" |
By design (documented) |
Scratch dir recomputed per-blob after scratchDirCreated gate |
Negligible |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Low priority |
No test for --include-orphans + stale-gen routing |
Low priority |
All three Codex round-9 findings are correctly fixed and tested. No new blocking issues. The PR is ready to merge once the stacked bases (feat/backup-phase0a-dynamodb, feat/s3keys-parse-blob-key) land.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63959370d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return errors.Wrapf(ErrS3MalformedKey, "blob key: %q", key) | ||
| } | ||
| st := s.objectState(bucket, gen, object) | ||
| dir := filepath.Join(s.scratchRoot, EncodeSegment([]byte(bucket)), EncodeSegment([]byte(object))) |
There was a problem hiding this comment.
HandleBlob writes chunk files under filepath.Join(s.scratchRoot, ..., EncodeSegment(object)), but EncodeSegment intentionally preserves / and .. for S3 keys, so keys like ../x or ../../tmp/pwn can escape the scratch subtree and write outside the decoder’s controlled directory before Finalize runs. The later safeJoinUnderRoot check only protects final object output paths, not this early spill-to-disk step, so a crafted snapshot/object key can still trigger arbitrary file writes on the host during decode.
Useful? React with 👍 / 👎.
…at/backup-phase0a-s3
…d 9) Codex P1 round 11 (commit 9a63e32): `HandleBlob` composed scratch paths with `filepath.Join(s.scratchRoot, EncodeSegment([]byte(bucket)), EncodeSegment([]byte(object)))`. EncodeSegment uses the RFC3986 unreserved set (ALPHA/DIGIT/-/./_) — `/` is percent-encoded but `.` is preserved, so the literal segment `..` survives unchanged. A crafted `bucket=".."` and/or `object=".."` would resolve to `<scratchRoot>/../...`, letting `writeFileAtomic` land outside the decoder's controlled directory before `safeJoinUnderRoot` runs at output time. Add `scratchDirForBlob` which rejects `.` / `..` / "" bucket and object literals at the encoder boundary so the spill-to-disk step inherits the same containment invariant the final output path enforces. Apply the same guard to `flushOrphanObject` which shared the failure mode under `--include-orphans`. (Multi-segment dot keys like `a/../b` continue to be caught at Finalize via `safeJoinUnderRoot` because EncodeSegment keeps the whole key in one filename segment that splits cleanly there.) Tests: - TestS3_HandleBlobRejectsScratchEscape: 5 sub-cases covering bucket/object/both variants of `.`/`..` literals. - TestS3_DotSegmentObjectKeyRejected updated to allow either HandleBlob or Finalize to surface ErrS3MalformedKey, since sole-dot keys are now caught earlier.
|
@claude review Round 9 fix for Codex P1 round 11 on commit 9a63e32 (commit
Added Tests: |
|
Claude finished @bootjp's task in 2m 53s —— View job Review —
|
| case | bucket | object | scratchDirForBlob check |
|---|---|---|---|
object_dotdot |
"b" |
".." |
object == ".." → reject |
object_dot |
"b" |
"." |
object == "." → reject |
bucket_dotdot |
".." |
"x" |
bucket == ".." → reject |
bucket_dot |
"." |
"x" |
bucket == "." → reject |
both_dotdot |
".." |
".." |
bucket == ".." → reject (loop exits on first match) |
All assert errors.Is(err, ErrS3MalformedKey) directly on the HandleBlob return — no Finalize required. ✓
Test: Updated TestS3_DotSegmentObjectKeyRejected ✅
The updated test (s3_test.go:621) correctly handles the split-rejection design: for ".." and "." object keys, HandleBlob fires the guard and the test returns early; for "a/../b" and "a/./b", HandleBlob succeeds (object is multi-segment, safely encoded by EncodeSegment in scratch path) and safeJoinUnderRoot catches them at Finalize. The added t.Parallel() is a minor improvement.
The logic paths for each key:
| key | HandleBlob result | rejection point |
|---|---|---|
".." |
ErrS3MalformedKey |
scratchDirForBlob (round 11) |
"." |
ErrS3MalformedKey |
scratchDirForBlob (round 11) |
"a/../b" |
success (object is multi-segment) | safeJoinUnderRoot at Finalize |
"a/./b" |
success | safeJoinUnderRoot at Finalize |
✓
Stale comment (carry-over from rounds 5 and 6 — still present)
safeJoinUnderRoot at s3.go:799–802:
// Use filepath.Join here — its only behavioural change vs. raw
// concatenation after the dot-segment guard above is normalising
// a leading "/" off `rel` (which is what we want: absolute-path
// keys collapse safely under bucketDir).Since round-5 leading-slash keys are rejected (empty leading segment). The phrase "absolute-path keys collapse safely" contradicts the current design. Still low priority, still not blocking.
Carry-overs from prior rounds
| Item | Status |
|---|---|
Stale comment at safeJoinUnderRoot re: "absolute-path keys collapse safely" |
Still present (low priority) |
joined == cleanRoot dead branch |
Still present (low priority) |
Silent bypass when manifest.upload_id == "" |
By design (documented) |
Scratch dir recomputed per-blob after scratchDirCreated gate |
Negligible |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Low priority |
No test for --include-orphans + stale-gen routing |
Low priority |
The Codex P1 round-11 finding (scratch-path escape via literal ./.. bucket or object names) is correctly fixed at the right enforcement boundary and tested. No new blocking issues. The PR is ready to merge once the stacked bases (feat/backup-phase0a-dynamodb, feat/s3keys-parse-blob-key) land.
…' into feat/backup-phase0a-s3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8881bf3fa7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if want.chunkCount > 0 && (o.count != want.chunkCount || o.maxIndex+1 != want.chunkCount) { | ||
| return errors.Wrapf(ErrS3IncompleteBlobChunks, |
There was a problem hiding this comment.
Fresh evidence: verifyChunkCompleteness can still accept missing manifest chunks because it only checks o.count and o.maxIndex. For a declared chunk_count=3, observed chunk numbers {0,2,2} satisfy count==3 and maxIndex+1==3, so this branch does not raise ErrS3IncompleteBlobChunks even though chunk 1 is absent; finalize then assembles corrupted object bytes silently.
Useful? React with 👍 / 👎.
…at/backup-phase0a-s3
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codex P1 round 12 (commit e91f086): `verifyChunkCompleteness` only checked `count` and `maxIndex` per (partNo, partVersion). For declared `chunk_count=3`, observed chunk numbers `{0, 2, 2}` satisfy `count == 3` and `maxIndex+1 == 3` while chunk_no=1 is absent — assembleObjectBody would then emit a corrupted body silently. Replace the dual-threshold heuristic with a set-membership check: track every observed chunk index per (partNo, partVersion) and verify the set is exactly `{0, 1, …, chunk_count-1}`. Both the unique-count guard and a per-index lookup surface ErrS3IncompleteBlobChunks before the assembler runs. Test: - TestS3_DuplicateChunksWithMissingIndexRejected: drives the `{0, 2, 2}` shape, asserts the new validator rejects it. - TestS3_IncompleteChunksRejected (existing) still passes against the new code path.
|
@claude review Round 10 fix for Codex P1 round 12 on commit e91f086 (commit
Replaced the dual-threshold heuristic with a set-membership check: track every observed chunk index per (partNo, partVersion) and verify the set is exactly Test: |
|
Claude finished @bootjp's task in 4m 10s —— View job Review —
|
| Item | Status |
|---|---|
Stale comment at safeJoinUnderRoot re: "absolute-path keys collapse safely" |
Still present (low priority) |
joined == cleanRoot dead branch |
Still present (low priority) |
Silent bypass when manifest.upload_id == "" |
By design (documented) |
Scratch dir recomputed per-blob after scratchDirCreated gate |
Negligible |
Orphan chunks loaded via os.ReadFile in flushOrphanObject |
Low priority |
No test for --include-orphans + stale-gen routing |
Low priority |
The set-based replacement is correct and future-proof. No new blocking issues. The PR is ready to merge once the stacked bases (feat/backup-phase0a-dynamodb, feat/s3keys-parse-blob-key) land.
…to feat/backup-phase0a-s3
1 participant
Summary
Stacked on top of #716 (DynamoDB) and #717 (s3keys.ParseBlobKey). Adds the S3 encoder for the Phase 0 logical-backup decoder — the most complex per-adapter piece because it must reassemble multipart object bodies from independent blob chunks.
Snapshot prefixes handled:
!s3|bucket|meta|<bucket>→s3/<bucket>/_bucket.json!s3|obj|head|<bucket><gen><object>→s3/<bucket>/<object>.elastickv-meta.json(sidecar)!s3|blob|<bucket><gen><object><uploadID><partNo><chunkNo>[<partVersion>]→ spilled to scratch on arrival; concatenated intos3/<bucket>/<object>at Finalize in(partNo, partVersion, chunkNo)order!s3|upload|meta|/!s3|upload|part|: excluded by default; opt in via--include-incomplete-uploadsto emit under_incomplete_uploads/records.jsonl!s3|bucket|gen|,!s3|gc|upload|,!s3route|: ignoredHow body assembly works
Each blob chunk is written atomically to a per-(bucket, object) scratch directory as it arrives, registered under the
(uploadID, partNo, chunkNo, partVersion)routing key. At Finalize, chunks for each object are sorted and concatenated into a single body file via tmp+rename. Body bytes are never held in memory — only the scratch path map. The scratch tree is removed on Finalize.Reserved-suffix collision handling
A user object key ending in
.elastickv-meta.jsonis rejected withErrS3MetaSuffixCollisionby default.WithRenameCollisions(true)appends.user-datato the body file and records the rename ins3/<bucket>/KEYMAP.jsonlwithKindMetaCollisionso the dump remains reversible. Per-bucketKEYMAP.jsonlis opened lazily and dropped on Finalize if no records were written.Test plan
go test -race ./internal/backup/...— pass.golangci-lint run ./internal/backup/...— clean.--include-incomplete-uploadsround-trip + default skip, versioned blob assembly.Stacking
Base:
feat/backup-phase0a-dynamodb(PR #716). Also depends onfeat/s3keys-parse-blob-key(PR #717).