feat(sync): snapshot + retention wire format and hub snapshot plane (P4-SYNC-02 part 1, P6-HUB-04 format)#65
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds snapshot sealing, retention-manifest CAS, snapshot-object storage, floor-based pull and compaction behavior, plus supporting S3/R2, encrypted-hub, test, and spec updates. ChangesSnapshot Exchange and Retention Plane
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/hub/r2.go`:
- Around line 548-552: The PutSnapshotObject path in R2Hub currently only
validates the key format and then stores the bytes, so it can accept content
that does not match sha256Hex. Update PutSnapshotObject to compute the SHA-256
of body and compare its hex encoding against sha256Hex before calling
h.retry().do / h.S3.PutObject, returning dssync.ErrInvalidBlobKey on mismatch.
Use the existing R2Hub, PutSnapshotObject, and snapshotKey symbols to locate the
change, and add crypto/sha256 plus encoding/hex imports if needed.
- Around line 594-598: ListSnapshotObjects is returning keys that may not
actually include the required .json suffix because it strips the suffix before
validating the key. Update the object filtering in internal/hub/r2.go so only
objects whose key matches the snapshot JSON filename format are returned, and
keep using the existing symbols ListSnapshotObjects, isValidHexKey, and
dssync.BlobInfo to locate the logic. Ensure bare snapshots/<sha> objects are
excluded and only snapshots/<sha>.json entries are advertised.
In `@internal/sync/hub.go`:
- Around line 371-391: The FileHub retention update in PutRetention is using a
non-atomic read/check/write flow, so concurrent writers can both pass the
etag/absence check and overwrite each other. Update PutRetention to perform the
compare-and-swap under an exclusive lock or equivalent atomic file-CAS
mechanism, using the retentionPath/metaDir handling in FileHub, and keep the
write itself atomic by writing to a temp file and renaming it after the
validation step.
- Around line 349-351: The retention override merge in h.RetentionSeqs is
currently replacing each device floor directly, which can lower an existing
manifest floor and allow incremental pulls across compacted gaps. Update the
merge logic in the retention/floor handling path so the floor for each device is
set to the maximum of the existing value in floors and the override seq,
preserving the stricter floor while still allowing tests to tighten it. Use the
h.RetentionSeqs loop and the floors map as the main reference points when
applying the change.
- Around line 399-410: PutSnapshotObject currently accepts any body for a valid
key and treats any existing file as deduped without checking content, so add a
sha256(body) verification against sha256Hex before writing in
FileHub.PutSnapshotObject. Also, when os.Stat(dst) finds an existing snapshot,
read/verify the stored bytes match the requested hash before returning nil;
otherwise return an error so mismatched content cannot be silently reused.
In `@internal/sync/snapshot.go`:
- Around line 285-292: The UnsealSnapshot flow currently decrypts and unmarshals
the snapshot but does not verify that the plaintext document matches the
authenticated carrier fields. Update UnsealSnapshot, after json.Unmarshal into
Snapshot and before returning, to compare Snapshot.WorkspaceID, ProducedBy, HLC,
Epoch, and the candidate-key KID against the authenticated envelope/carrier
values, and return ErrSnapshotVerification on any mismatch.
- Around line 421-426: ParseRetentionFloors currently returns m.Floors even when
the manifest omits floors, which lets ParseRetentionManifest accept empty or
partial manifests as valid. Update ParseRetentionFloors to fail closed by
rejecting syntactically valid manifests that do not include floors, and make
sure the check is done after ParseRetentionManifest so Pull cannot treat a
malformed retention marker as “no floor.” Use the ParseRetentionFloors and
ParseRetentionManifest symbols to locate the validation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8ab8aa2-deaf-49f0-9535-c64e3d84eeda
📒 Files selected for processing (15)
internal/hub/mems3_test.gointernal/hub/r2.gointernal/hub/r2_snapshot_test.gointernal/hub/s3client_awssdk.gointernal/sync/encryptedhub.gointernal/sync/encryptedhub_test.gointernal/sync/hub.gointernal/sync/snapshot.gointernal/sync/snapshot_hub_test.gointernal/sync/snapshot_test.gospec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/15_SECURITY_THREAT_MODEL.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.mdspec/19_CLOUD_PROVISIONING_GUIDE.md
|
Codex review findings addressed in the follow-up commit: (P1) structural fail-closed manifest parsing — |
|
Opus review verdict: SHIP — its four findings map 1:1 onto the Codex trio already fixed on the branch. Its remaining nit (AAD domain separation between the snapshot and enc.v2 planes, which share the per-epoch WCK) is now also fixed in the latest commit: |
…P4-SYNC-02 part 1, P6-HUB-04 format) - snapshot.v1 sealed document + signed retention manifest (devstrap:retention:v1) - Hub interface retention/snapshot plane: CAS PutRetention, content-addressed snapshot objects, CompactEventsBelow (FileHub + R2Hub + EncryptedHub) - S3Client GetObjectWithETag/PutObjectIfMatch (If-Match CAS) - Pull reads manifest floors unverified (DoS-only); garbled manifest fails closed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rse, CAS self-race read-back, FileHub lock-file CAS
Codex review P1: {} / null-floors / wrong-version manifests are now hard
errors, never "no floor". P2s: a retried conditional PutRetention that 412s
against its own commit classifies as success by byte comparison; FileHub's
read-check-write runs under an O_EXCL lock file with atomic install.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Snapshots and enc.v2 events share the per-epoch WCK; a leading devstrap:snapshot-aead:v1 tag makes the AAD separation unconditional rather than structural. Free now — no sealed snapshot exists yet. envelopeAAD deliberately untouched (enc.v2 ciphertext is live). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2a13ee0 to
4cbfb71
Compare
…uffix listing, monotonic test override, UnsealSnapshot carrier cross-check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All five CodeRabbit findings applied in the latest commit: |
PR 1 of the compaction + snapshot-exchange wave (
P4-SYNC-02/P4-HUB-11). Library + backends only — no CLI behavior change, no migration.What
internal/sync/snapshot.go(new): thesnapshot.v1wire format — snapshot document (namespace entries with source-event coords, draft pointers, tombstones, per-device chain anchors, floor map) sealed under the current-epoch WCK with an enc.v2-style XChaCha20-Poly1305 AEAD (AAD binds workspace/producer/kid/HLC/epoch; kid field stays an unauthenticated routing hint). Content-addressed by sha256 of the sealed bytes. Signed retention manifest (per-device floors + snapshot ref + prev_sha256 chain) under the newdevstrap:retention:v1domain.Hubinterface grows the retention/snapshot plane:GetRetention/PutRetention(CAS: create-only or If-Match; lost race =ErrRetentionConflict), content-addressed snapshot objects,CompactEventsBelow(strictly below floor; never Seq<=0). Implemented by FileHub, R2Hub, EncryptedHub (pass-through).S3ClientgrowsGetObjectWithETag+PutObjectIfMatch(If-Match CAS — R2-supported S3 extension; sdk v1.104.1 modelsPutObjectInput.IfMatch); memS3 simulates etags/CAS.Trust model (spec/07 + spec/15 updated)
One Ed25519 signature on the manifest covers both objects (manifest names the snapshot by sha256; AEAD binds the plaintext to its carrier). Import verification is fail-closed with no pre-enrollment window — lands in PR 2. A malicious hub can only DoS, never inject state.
Tests
Seal/unseal + per-field AAD tamper matrix (kid-relabel-harmless mirrors enc.v2), manifest sign/verify + tamper matrix + canonical re-parse pin, FileHub/memS3 CAS conflict matrices, floor gates (exact at-floor boundary; fresh device forced to snapshot), both-layouts compaction with unparseable-legacy-keys-KEPT.
Validation
gofmt;golangci-lint run(0 issues);spec-drift(20 specs, 15 files);go test -race ./...green (one pre-existinginternal/gitflake, passes on rerun).Follow-up PRs in this wave: import/recovery + migration 00020 (PR 2),
hub compact(PR 3), sync acks + tombstone GC (PR 4),hub migrate-events+ sweep lock (PR 5).🤖 Generated with Claude Code
Summary by CodeRabbit