Repository navigation
encryption: add the §5.4 rewrite-job decision layer (Stage 9C-6) - #1235
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
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 |
![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: cbb6e954f9
ℹ️ 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".
|
Correction to my earlier doc-row commit on this PR. That commit said the §9.2 metrics needed naming in the open row because Fixed while merging The 9C-6 decision layer and its tests are unchanged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cff9662207
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The retention boundary, anti-burst throttling, and DEK reason classification contracts are currently incorrect.
3 open findings
What changed in this PR
Adds the unwired Stage 9C-6 decision layer for MVCC encryption rewrites and rate throttling.
Changes:
- Adds per-version rewrite classification.
- Adds rewrite throughput throttling.
- Documents the shipped decision layer with unit tests.
| File | Description |
|---|---|
internal/encryption/rewrite_plan.go |
Implements rewrite decisions and throttling. |
internal/encryption/rewrite_plan_test.go |
Tests classification and rate behavior. |
docs/design/2026_04_29_partial_data_at_rest_encryption.md |
Records Stage 9C-6 status. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if version.CommitTS <= minRetainedTS { | ||
| // Below the retention floor: no reader can reach it, and the | ||
| // retirement criterion (§5.4 item 4) is stated against | ||
| // minRetainedTS for exactly this reason. | ||
| return RewriteDecision{Verdict: RewriteSkip, Reason: RewriteReasonUnretained} |
There was a problem hiding this comment.
Verified in current HEAD: ClassifyMVCCRewrite processes all physically retained versions without a timestamp-only exclusion. Tests cover both an exact-floor version and an anchor below the floor, so a cold key remains rewritable. All callers were audited with rg; only tests call this unwired API. The encryption package race tests pass.
| if version.KeyID == retiringKeyID { | ||
| return RewriteDecision{Verdict: RewriteReencrypt, Reason: RewriteReasonRetiringDEK} | ||
| } | ||
| return RewriteDecision{Verdict: RewriteSkip, Reason: RewriteReasonAlreadyFresh} |
There was a problem hiding this comment.
Verified in current HEAD: the reason is now different_dek, without asserting that an untargeted DEK is active. The table-driven test separately covers the active key and another old key. All callers were audited; only tests consume this unwired contract. Race tests pass.
| required := time.Duration(float64(t.written) / t.bytesPerSecond * float64(time.Second)) | ||
| elapsed := now.Sub(t.started) | ||
| if elapsed >= required { | ||
| return 0 | ||
| } | ||
| return required - elapsed |
There was a problem hiding this comment.
Verified in current HEAD: Record advances the virtual deadline from max(previousDeadline, now) for every positive batch. TestRewriteThrottleDoesNotBankIdleTime covers a minute-long stall followed by back-to-back batches; the debt grows by one second per MiB at 1 MiB/s. The design row and PR description now describe the virtual-deadline rule. Race tests pass.
|
Updated HEAD: 4316e56. Merged current main and resolved the design-table conflict while preserving the encrypted Jepsen milestone. Rechecked all eight historical inline findings against the current implementation and replied with their regression coverage. Current-head root findings before this push: zero. Validation: encryption package race tests (all three packages), go vet, caller audit, and git diff --check passed. Full local testing is limited by the existing macOS FUSE Statfs_t.Namelen compile failure; local lint is limited by its Go 1.26-built binary encountering Go 1.27 source. Full Linux tests and lint will be checked in CI. Commit signature verified locally; Author and Committer are bootjp contact@bootjp.me. @codex review |
4316e56 to
9f8b749
Compare
|
Updated HEAD: 9f8b749. Replaced historical commits containing session attribution with one signed commit by bootjp contact@bootjp.me, based on current main. The complete Git tree is byte-identical to the previously tested 4316e56 commit (verified with git diff --exit-code). All inline replies and test results remain applicable. No implementation change was introduced by this history cleanup. @codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Adds the Stage 9C-6 decision layer for re-encrypting retained MVCC history during DEK rotation, plus an inter-batch rewrite rate budget.
Behavior
(user_key, version_ts), including the retained anchor below or at the retention floor. Tombstones are skipped; cleartext is rewritten only during an explicit migration sweep.different_dekfor encrypted versions outside the targeted retiring DEK, including intermediate old keys.max(previousDeadline, now)for each batch, preventing idle time from becoming burst credit. Preserve byte accounting in unthrottled mode and clamp unrepresentable delays.Validation
go test ./internal/encryption/... -race -count=1: all three packages passed.go vet ./internal/encryption/...: passed.git diff --check: passed.rg: classifier and throttle callers are confined to their tests.go test -race ./..., including required kernel FUSE tests: passed.syscall.Statfs_t.Namelencompile failure; the local lint binary was built with Go 1.26 and could not parse Go 1.27 source. The successful Linux CI checks cover both validations.Risk
Additive, unwired API contract only. Tests cover retained history, retention anchors, explicit cleartext migration, third-key classification, post-idle bursts, cumulative byte accounting, and duration overflow. The merge commit bypassed the hook because of the known unrelated macOS FUSE compile failure.