Repository navigation
fix: missing params fails decryption + multipart completion cleanup - #119
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated sweeper behavior can incorrectly crypto-shred/delete locations for blobs of a committed object if a crash occurs after the catalog commit but before post-commit claim reconciliation completes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the encryption read/complete paths to fail closed when encryption metadata is missing (instead of accidentally serving ciphertext as if it were plaintext), and improves multipart hygiene by cleaning up orphaned multipart part blobs so they don’t leak spool/encryption/park state after completion/abort/sweep.
Changes:
- Make decryption and multipart span reconstruction error on missing
blob_encryption_paramsrows (fail closed). - Add multipart orphan-part cleanup at
CompleteMultipartUploadand broaden/solidify abort/sweeper cleanup logic. - Add focused unit + integration coverage for multipart orphan cleanup and fail-closed encryption behavior.
File summaries
| File | Description |
|---|---|
| s3frontend/multipart.go | Adds orphan-part cleanup, strengthens abort/sweep cleanup, and extends cleanupPartBlobs to handle accepted-orphan release. |
| s3frontend/multipart_hygiene_test.go | New unit tests asserting multipart cleanup fully releases orphaned blobs (enc-params/intents/parks/network release). |
| s3frontend/decrypt.go | Fail-closed behavior when encryption params are missing; removes plaintext fallback for missing rows. |
| s3frontend/decrypt_test.go | Updates tests to validate missing encryption params fail closed rather than mixing plaintext/ciphertext handling. |
| migrations/sql/00014_blob_encryption.sql | Updates schema commentary to reflect “all blobs encrypted” invariant and fail-closed read semantics. |
| itest/forge_encryption_test.go | Adds integration coverage for orphan-part shredding after multipart completion. |
| docs/architecture.md | Clarifies that blob_refs updates occur post-commit (best-effort) rather than inside the commit transaction. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Stale open sessions and crash-stranded 'completing' rows: latch into | ||
| // 'aborting' (losing gracefully to a concurrent Complete/Abort) and clean | ||
| // up like an abort. A 'completing' row whose commit actually landed is | ||
| // safe here: its winners hold reference claims, which the cleanup skips. | ||
| for _, state := range []string{registry.SessionOpen, registry.SessionCompleting} { |
276c615 to
f7b74e6
Compare
cb4869a to
c960cc6
Compare
c960cc6 to
7b10869
Compare
closes #90