Conversation
The second of §5.2's two rotation triggers: a hard ceiling of 2^32
writes per (DEK, process-load) pair, per NIST SP 800-38D §8.3, with
admission control refusing new writes at 90% and the cluster
auto-proposing a rotate-dek entry at that point.
I had been reporting this whole stage as blocked on open key-lifecycle
questions. That was wrong — §5.2 specifies the budget trigger
completely, and it builds directly on the
elastickv_encryption_writes_per_dek counter shipped in 9C-1. Only the
rewrap/retire/rewrite half carries open questions.
Three properties the tests pin, each revert-checked:
- a refused write is NOT counted, so a caller that retries on refusal
cannot walk the counter past the ceiling — the one thing the
ceiling exists to prevent;
- refusal happens at 90%, not at the ceiling: rotation needs a Raft
round trip, so no headroom would mean either blocking writes while
it commits or issuing writes past the ceiling;
- the budget is per DEK, so a freshly rotated key starts full. A
shared counter would have the new key inherit the old key's spent
budget and wedge the cluster the moment it rotated.
Scope is per-process-load, matching §5.2's "(DEK, process-load)" pair
and the §4.1 nonce construction whose local_epoch bumps on every
process start: a restart begins a fresh budget because it also begins a
fresh nonce epoch, so the (key, nonce) space the ceiling protects is
itself fresh.
Pure and injectable — no store, no clock — so the accounting is
testable without a cluster. Wiring it into admission control and the
auto-propose path is the execution half and stays open.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
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 reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
|
Claude finished @bootjp's task in 4m 36s —— View job Review: §5.2 writes-per-DEK rotation budget
Could not execute The design is sound and the write path ( 1. 2. Threshold truncation in Minor / non-blocking:
Otherwise: |
![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: 146e42e785
ℹ️ 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".
| // Integer arithmetic in this order so the threshold cannot | ||
| // overflow for a large ceiling and cannot round to zero for a | ||
| // small one. | ||
| threshold: ceiling / 100 * writeBudgetRefusePercent, |
There was a problem hiding this comment.
For ceilings that are not multiples of 100, dividing before multiplying can make the refusal point far lower than 90%; for example, NewWriteBudget(199) sets the threshold to 90 (about 45%), and any ceiling below 100 sets it to zero and refuses the first write. Preserve the remainder while using overflow-safe arithmetic so supported custom ceilings receive an actual 90% budget.
Useful? React with 👍 / 👎.
| if counter.Add(1) > b.threshold { | ||
| return WriteBudgetRotate |
There was a problem hiding this comment.
When several writers load a count just below the threshold concurrently, they can all enter this branch; only the writer whose add lands on the threshold is allowed, while later writers return WriteBudgetRotate but leave their increments recorded. With a ceiling of 100 and the count at 89, eleven simultaneous calls can therefore raise Used to 100 even though ten writes were refused, causing subsequent calls to report WriteBudgetExhausted. Use a CAS loop so calls returning a refusal do not consume budget.
Useful? React with 👍 / 👎.
| | 9B | AWS KMS, GCP KMS, Vault Transit, and test/CI env KEK providers; mutually-exclusive source loader and loaded-provider mutator gate (§5.1, §6.1, §6.5) | shipped | `2026_07_18_implemented_9b_kek_providers.md` | | ||
| | 9C+ | Rotation budget/rewrap/retire/rewrite, metrics, remaining benchmarks and encrypted Jepsen (§5.2, §5.4, §6.5, §8, §9.2) | open | — | | ||
| | 9C-4 | §5.2 writes-per-DEK rotation budget: the 2^32 per-(DEK, process-load) ceiling, the 90% refuse-and-rotate threshold, and per-DEK accounting | shipped | — | | ||
| | 9C+ | Rotation rewrap/retire/rewrite, admission-control and auto-propose wiring for the budget above, remaining benchmarks (§5.2, §5.4, §6.5, §8) | open | — | |
There was a problem hiding this comment.
Removing metrics from this open row makes the required elastickv_encryption_writes_per_dek{key_id} observable disappear from the implementation plan even though it has not shipped: a repository-wide search for that exact metric finds only the design document, and the new WriteBudget neither registers nor updates a Prometheus collector. Until the metric is implemented, retain it in the open scope so deployments do not silently miss the §9.2 counter.
Useful? React with 👍 / 👎.
Three review findings on the §5.2 budget.
1. Refused writes consumed budget. Record loaded the counter, decided,
then incremented — so every writer that read a count below the
threshold incremented it, including the ones then refused. With a
ceiling of 100 and the count at 89, concurrent callers left the
counter at 100 having permitted one write, and the next call
reported Exhausted for a DEK that had issued 90 writes. That
contradicted this type's own documented contract ("A refused write
is not counted") and the comment claiming the overshoot "never
reaches the ceiling from below the threshold". Record now reserves
the slot with CompareAndSwap, so the counter records permitted
writes and nothing else.
The existing TestWriteBudgetDoesNotCountRefusedWrites could not
catch this: sequentially the Load sees the threshold and returns
before incrementing, so the bug is only reachable under contention.
2. The threshold discarded the remainder. ceiling/100*90 loses up to 99
ceiling-units: a ceiling of 199 refused at 90 (about 45%) and any
ceiling below 100 refused at 0, i.e. refused the first write and
wedged the DEK. Splitting quotient and remainder keeps the exact
floor without overflow, and a ceiling whose 90% point floors to zero
now allows one write rather than none.
TestWriteBudgetDefaultCeilingMatchesTheDesign asserted
`1<<32/100*90` — the same expression as the implementation — so it
could not catch a division-order error either. It now states the
specification: 90% of 2^32 is 3865470566, not 3865470480.
3. The open 9C+ milestone row had dropped the §9.2 metrics and
encrypted Jepsen. elastickv_encryption_writes_per_dek{key_id} exists
only in this design document; this budget does not export it. Put
the metrics and Jepsen scope back in the open row, naming that gap.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
All three findings are correct and fixed in 372c052. Details, including two cases where the existing tests could not have caught the defect: P2 — Reserve budget atomically before allowing a write. Confirmed. Traced with ceiling 100, threshold 90, count 89, 11 concurrent callers: all load 89, all pass the check, all Worth flagging: P2 — Compute the 90% threshold without discarding the remainder. Confirmed, and worse than a rounding issue at small ceilings: Two existing tests were complicit:
P2 — Keep the write-counter metric in the open milestone. Confirmed. I applied the same correction to the 9C-5 row on #1234 and the 9C-6 row on #1235, which had narrowed the open row the same way. Verification
One consequence to note: with atomic reservation the counter stops at the threshold, so |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @bootjp's task in 1m 49s —— View job Review: fixes in 372c052
Could not execute All three fixes check out. 1. CAS reservation ( for {
used := counter.Load()
if used >= b.ceiling { return WriteBudgetExhausted }
if used >= b.threshold { return WriteBudgetRotate }
if counter.CompareAndSwap(used, used+1) { return WriteBudgetAllow }
}This is correct: 2. Threshold arithmetic (
All correct. 3. Design doc ( Minor, non-blocking: the CAS loop is unbounded under pathological contention (a Nothing else to flag. Scope, doc comments, and tests are now mutually consistent, and the two new regression tests ( |
The 9C-6 row had narrowed the open 9C+ scope to "remaining benchmarks
and encrypted Jepsen", dropping the explicit mention of the §9.2
metrics. Neither elastickv_encryption_writes_per_dek{key_id} nor
elastickv_encryption_last_proposed_index_per_raft_dek{key_id} exists
outside this design document, so naming them keeps the gap visible
rather than leaving it implied by the section reference alone.
Same correction applied on #1233 and #1234, which narrowed the row the
same way.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
…-write-budget # Conflicts: # docs/design/2026_04_29_partial_data_at_rest_encryption.md
|
Correction to my earlier comment on this PR. I wrote that Fixed while merging No code change: the three fixes in this PR and their revert-checks are unaffected. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c49cca66a0
ℹ️ 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 b == nil { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
When the budget is intentionally unwired, Record treats a nil receiver as unlimited and allows writes, but Remaining returns zero, which its contract defines as “rotation is due.” Any admission or observability code that checks Remaining(keyID) == 0 will therefore trigger rotation immediately on an unconfigured budget even though the corresponding write verdict is Allow. Return a value representing an unlimited/disabled budget, or make the nil state explicit so these APIs agree.
Useful? React with 👍 / 👎.
| counter = &atomic.Uint64{} | ||
| b.counters[keyID] = counter |
There was a problem hiding this comment.
When WriteBudget is embedded or declared without calling NewWriteBudget, its zero value reaches this assignment with a nil counters map and Record panics. This is particularly surprising because the exported type otherwise handles nil receivers and its other methods tolerate a zero value. Lazily initialize the map and apply the default ceiling before recording, or explicitly prevent zero-value use.
Useful? React with 👍 / 👎.
…stently Two findings from the second review round, both about a budget that was not built by NewWriteBudget. P2 — the nil receiver disagreed with itself. Record returns Allow on a nil budget, but Remaining returned 0, which this type documents as "rotation is due". Any admission check or dashboard keyed on Remaining(k) == 0 would therefore demand rotation on every unconfigured node forever while the write verdict said Allow. Remaining now returns RemainingUnlimited for that state, so the two APIs agree about the same receiver, and zero keeps its single meaning. P2 — the zero value panicked. A WriteBudget declared or embedded rather than constructed reached counterFor with a nil map, and assigning into one panics -- surprising precisely because this type tolerates a nil receiver and its other methods tolerate a zero value. The map is initialised on first use, and the ceiling and threshold fall back to the §5.2 default rather than staying zero, which would have refused the very first write. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Second round: both fixed in the commit above. P2 — Keep nil budgets from reporting rotation due. Confirmed, and the disagreement was in the dangerous direction:
P2 — Initialize the counter map for zero-value budgets. Confirmed; the revert-check reproduces the exact The map is now initialised on first use, and — the part worth calling out — the ceiling and threshold fall back to the §5.2 default rather than staying zero. Lazily initialising only the map would have left a zero ceiling, and Verification
|
|
To use Codex here, create a Codex account and connect to github. |
|
Claude finished @bootjp's task in 2m 16s —— View job Review: second-round fixes in 0992fc1
Could not execute Both fixes check out. 1. Nil-budget 2. Zero-value map init ( Consistency check I specifically went looking for a hole in: The two new tests ( Nothing else to flag. No other call sites exist yet ( |
…-write-budget # Conflicts: # docs/design/2026_04_29_partial_data_at_rest_encryption.md
1 participant
I was wrong that this was blocked
I have been reporting encryption 9C+ as blocked on open key-lifecycle questions. Re-reading §5.2, that is only true of the rewrap/retire/rewrite half. The rotation budget is specified completely:
And it builds directly on
elastickv_encryption_writes_per_dek, which shipped in 9C-1 (#1221). Nothing about it needed a decision from you.Three properties, each revert-checked
A refused write is not counted. This is the one that keeps the ceiling meaningful — if refusals consumed budget, a caller retrying on refusal would walk the counter straight past the ceiling, which is precisely what it exists to prevent.
Refusal is at 90%, not at the ceiling. Rotation takes a Raft round trip. With no headroom you get one of two bad outcomes: block writes while it commits, or keep issuing writes past the ceiling.
The budget is per DEK. A freshly rotated key must start full — a shared counter would have the new key inherit the old key's spent budget and wedge the cluster the instant it rotated.
Scope note
Per-process-load, matching §5.2's
(DEK, process-load)pair and the §4.1 nonce construction whoselocal_epochbumps on every process start. A restart begins a fresh budget because it also begins a fresh nonce epoch, so the(key, nonce)space the ceiling protects is itself fresh. That is the reasoning for what would otherwise look like a counter that forgets too easily.Behavior change / risk
New type; nothing constructs it yet, so there is no runtime change. Wiring it into admission control and the auto-propose path is the execution half and stays open — deliberately, since refusing writes is a change I would not fold into the same PR as the accounting.
Hot-path shape: steady state is an uncontended
RLockplus one atomic add. A nil budget allows everything, so an unwired node is unaffected.Test evidence
go test ./internal/encryption/... -race -count=1— passgolangci-lint run(full repo) — 0 issues, no//nolint10 tests including the concurrency case (this sits on every encrypted write, so a torn counter would be a live production race) and nil-receiver.
Self-review (five passes)
https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE