encryption: add the §5.2 writes-per-DEK rotation budget by bootjp · Pull Request #1233 · bootjp/elastickv

bootjp · 2026-09-10T15:13:54Z

Summary

Add the process-local, per-DEK write-budget primitive specified by the data-at-rest encryption design.

reserve write slots atomically up to the 90% rotation threshold
keep refused writes from consuming the budget
isolate counters by DEK and support retired-key cleanup
define consistent nil and zero-value behavior
retain the remaining admission-control, auto-propose, and lifecycle wiring as open design work

Behavior and risk

This PR adds an isolated primitive and does not wire it into production write admission yet, so it does not change runtime request handling. The main risk is incorrect boundary or concurrency accounting; table, boundary, zero-value, and sustained-contention tests cover those cases.

Main synchronization

Signed merge commits a186116 and 9f83eb6 incorporate main through 9a20fbb. The only manual conflict was the encryption design status table: retain merged 9C-3 startup unwrap caching (#1226), merged 9C-6 rewrite decisions (#1235), this PR's 9C-4 write budget, and the still-open runtime wiring. The subsequent #1287 document update is preserved. No Go implementation or test from the original PR was modified, and no other main content was discarded.

Caller audit with rg finds no production users of the budget; existing callers remain its tests. This synchronization changes no runtime semantics.

Verification

go test -race ./internal/encryption/... -count=1: passed (encryption 86.692s, fsmwire 1.296s, kek 1.540s)
go vet ./internal/encryption/...: passed
scoped golangci-lint with Go 1.26.6: zero issues
git diff --check: passed
ancestor check confirms main 9a20fbb is retained; the delta against that main is only the original two budget files and the reconciled encryption design status

The locally incompatible default full-tree commit hook was bypassed per commit after explicit scoped checks; no hook/configuration was changed. Current-HEAD Linux CI and the review requested after the final push remain the merge gates.

Author

bootjp

bootjp · 2026-09-10T15:14:01Z

@codex review

bootjp · 2026-09-10T15:14:02Z

@claude review

chatgpt-codex-connector · 2026-09-10T15:14:17Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-22T19:27:06.357244Z	`0b26f97`	Manual request
🔒 Security Review	✅ Completed 2026-10-22T19:28:13.201562Z	`0b26f97`	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

coderabbitai · 2026-09-10T15:14:19Z

Warning

Review limit reached

You'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 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 4b3333f5-c03d-4236-be8c-a1da1376e826

📥 Commits

Reviewing files that changed from the base of the PR and between 14c06c6 and 0b26f97.

📒 Files selected for processing: 3

docs/design/2026_04_29_partial_data_at_rest_encryption.md
internal/encryption/write_budget.go
internal/encryption/write_budget_test.go

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

claude · 2026-09-10T15:14:32Z

Claude finished @bootjp's task in 4m 36s —— View job

Review: §5.2 writes-per-DEK rotation budget

Read repo guidelines (CLAUDE.md)
Read changed files (write_budget.go, write_budget_test.go, design doc diff)
Review for correctness, concurrency, and test coverage per repo self-review lenses
Post review feedback

Could not execute go test/golangci-lint myself in this sandboxed review session (Bash requires interactive approval that wasn't granted here), so this is a static read of the diff against origin/main, not a re-run of the reported test evidence.

The design is sound and the write path (Record, counterFor) is genuinely lock-light and race-safe as documented. Two things worth fixing before this gets built on in the admission-control PR:

1. WriteBudgetExhausted is unreachable through Record() as written.
internal/encryption/write_budget.go:117-134 — once counter.Load() >= b.threshold, every call takes the case used >= b.threshold branch and returns WriteBudgetRotate without touching the counter (that's the "refused writes aren't counted" property, working as intended). But that means the counter freezes at ~90% forever and can never climb the remaining 10% of the ceiling to satisfy case used >= b.ceiling. The godoc on WriteBudgetExhausted (lines 48-52) says it's "Reachable only if a caller ignored WriteBudgetRotate" — but an ignorant caller that keeps calling Record() gets the exact same frozen-counter behavior, so it still never fires. This isn't just theoretical: TestWriteBudgetFailsClosedPastTheCeiling (write_budget_test.go:63-76) is the only test aimed at that verdict, and it never actually produces one — it only checks Allowed() == false, which WriteBudgetRotate alone already satisfies. Grepping the test file confirms WriteBudgetExhausted is asserted only via its .String() (line 167), never via an actual Record() return value. Worth deciding now, before admission control is wired: either let Record keep advancing past the threshold so Exhausted is a real, reachable state, or drop/re-scope the verdict and stop documenting it as reachable.
Fix this →

2. Threshold truncation in NewWriteBudget.
internal/encryption/write_budget.go:94-98 computes threshold: ceiling / 100 * writeBudgetRefusePercent, with a comment claiming this order "cannot round to zero for a small one." That's backwards: ceiling/100 truncates first, so any ceiling < 100 collapses the threshold to exactly 0, and any ceiling that isn't a clean multiple of 100 loses up to ~90 units of precision (e.g. ceiling=150 → threshold 90, not the correct 135). At the real DefaultWriteBudgetCeiling (2³²) this is negligible, but write_budget_test.go:66-70 (TestWriteBudgetFailsClosedPastTheCeiling) calls NewWriteBudget(10) with a comment stating "Threshold is 9" — the actual computed threshold is 0, which is also why finding #1's test never reaches Exhausted (every call is refused from the first one, counter never moves). Worth fixing the comment/test and, if this constructor is ever exercised with a small ceiling in production config or the admission-control tests, tightening the rounding.
Fix this →

Minor / non-blocking:

docs/design/2026_04_29_partial_data_at_rest_encryption.md:37 — the new 9C-4 row's link column is — while every other "shipped" row cites either an implemented doc or a PR reference (e.g. row 5E says this PR). Worth filling in once merged, for consistency with the table's own convention.
Record's post-increment re-check (write_budget.go:126-131) can, in the narrow race window right at the threshold, increment the counter for a call that still returns WriteBudgetRotate — a small, already-documented and bounded exception to the unconditional "a refused write is not counted" claim in the exported doc comment (lines 106-108) and the PR description. It's safe (only ever pushes rotation earlier, never reaches the ceiling), just slightly overclaimed in the docs.
I couldn't find elastickv_encryption_writes_per_dek (or any promauto/prometheus registration) anywhere under internal/encryption/ in this tree — worth double-checking the PR's claim that it "shipped in 9C-1 (encryption: add storage-envelope metrics (Stage 9C-1, §9.2) #1221)" still holds, though this sandbox's shallow clone couldn't confirm or refute that history either way.

Otherwise: key_id typing matches uint32 used elsewhere in the package (envelope.go:72), the nil-receiver/Forget/Used/Remaining behavior all read correctly, and the double-checked-locking counterFor is a standard, correct pattern. Scope (per-process-load, no wiring yet) matches what the PR claims.
· branch design/encryption-9c4-write-budget

chatgpt-codex-connector

💡 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".

chatgpt-codex-connector · 2026-09-10T15:19:57Z

+		// 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,


Compute the 90% threshold without discarding the remainder

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 👍 / 👎.

Resolved in the current implementation. The refusal threshold preserves the remainder with overflow-safe quotient/remainder arithmetic, including non-multiples of 100 and small ceilings. Regression coverage includes 199, 99, 10, 2, 1, and the 2^32 default.

chatgpt-codex-connector · 2026-09-10T15:19:57Z

+		if counter.Add(1) > b.threshold {
+			return WriteBudgetRotate


Reserve budget atomically before allowing a write

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 👍 / 👎.

Resolved in the current implementation. Record reserves a slot with CompareAndSwap, so only allowed writes advance the counter. Sustained concurrent boundary tests verify allowed count, Used, and threshold remain equal.

chatgpt-codex-connector · 2026-09-10T15:19:57Z

 | 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 | — |


Keep the write-counter metric in the open milestone

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 👍 / 👎.

Resolved in the current design table. The 9C-1 writes_per_dek metric remains recorded as shipped, while the open 9C+ row explicitly retains wiring this budget to that accounting and the remaining observability work.

chatgpt-codex-connector · 2026-09-11T11:40:17Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

bootjp · 2026-09-11T11:40:32Z

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 Add, results 90…100. The one that lands on 90 returns Allow; the other ten return Rotate having already incremented. The counter reaches 100 — the ceiling — for a DEK that issued one write, and the next call reports Exhausted. This contradicted both the WriteBudgetRotate doc ("The write is NOT counted, so a caller that retries cannot drive the counter past the ceiling") and my own inline claim that the overshoot "never reaches the ceiling from below the threshold". Record now reserves the slot with CompareAndSwap.

Worth flagging: TestWriteBudgetDoesNotCountRefusedWrites already existed and passed, because it is sequential — the Load sees the threshold and returns before incrementing, so the bug is unreachable without contention. My first replacement test (a staged burst of 16 racers released by closing a channel) also passed against the buggy code — the channel wake-ups serialise enough that late racers observe the incremented count. It only became a real detector once the racers hammer Record until refused, giving sustained contention at the boundary; it then fails on trial 1.

P2 — Compute the 90% threshold without discarding the remainder. Confirmed, and worse than a rounding issue at small ceilings: NewWriteBudget(199) refused at 90 (45%), and every ceiling below 100 refused at 0 — the first write refused, rotation proposed forever, no write ever issued under the new DEK either. Now computed as ceiling/100*90 + ceiling%100*90/100, the exact floor (with ceiling = 100q + r this is 90q + floor(9r/10), and r < 100 bounds the remainder term at 8910, so neither term overflows). A ceiling whose 90% point floors to zero allows exactly one write instead of none.

Two existing tests were complicit:

TestWriteBudgetDefaultCeilingMatchesTheDesign asserted uint64(1)<<32/100*90 — the implementation's own expression — so it could never catch a division-order error. It asserted the lossy 3865470480; the exact value is 3865470566. Now stated as the specification (1<<32*9/10, plus the literal).
TestWriteBudgetFailsClosedPastTheCeiling carried the comment "Threshold is 9" for NewWriteBudget(10). Under the old arithmetic that threshold was 0, so every write in that test was refused and the assertion !Allowed() held vacuously. The fix makes the comment true; the test now also asserts Used == 9.

P2 — Keep the write-counter metric in the open milestone. Confirmed. elastickv_encryption_writes_per_dek{key_id} appears only in this design document (lines 1185 and 2534) — no Go file references it, and WriteBudget registers no collector. The 9C+ row now names the metrics and encrypted Jepsen scope again, and calls out that this budget does not export the counter yet.

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

go test ./internal/encryption/ -race -count=1 — ok, 25.3s; golangci-lint — 0 issues
Revert-checks, each confirming the named test fails with the fix reverted and the file restores byte-exact:
- Load-then-Add → TestWriteBudgetRefusedWritesDoNotConsumeBudgetUnderConcurrency FAILS ("the counter passed the refusal threshold, so refused writes consumed budget")
- divide-before-multiply → TestWriteBudgetThresholdIsNinetyPercent, TestWriteBudgetDefaultCeilingMatchesTheDesign, TestWriteBudgetFailsClosedPastTheCeiling FAIL
- drop the anti-wedge guard → TestWriteBudgetTinyCeilingDoesNotWedgeTheDEK, TestWriteBudgetReachesExhaustedAtADegenerateCeiling, TestWriteBudgetThresholdIsNinetyPercent FAIL

One consequence to note: with atomic reservation the counter stops at the threshold, so WriteBudgetExhausted is unreachable through Record for any ceiling above its threshold. I kept it as the fail-closed guard and documented that precisely, rather than leaving a doc comment that overstates when it fires. TestWriteBudgetReachesExhaustedAtADegenerateCeiling covers the one configuration that does reach it.

@codex review
@claude review

chatgpt-codex-connector · 2026-09-11T11:40:41Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

claude · 2026-09-11T11:40:48Z

Claude finished @bootjp's task in 1m 49s —— View job

Review: fixes in `372c052`

Read repo guidelines (CLAUDE.md)
Review the fixes in 372c052 against the three findings raised by Codex/CodeRabbit
Verify the CAS-based reservation fix in write_budget.go
Verify the threshold rounding fix
Verify the design doc scope-note fix
Run tests / lint if possible
Post final review

Could not execute go test / golangci-lint in this sandboxed review session — every go/shell invocation here (even go version) required interactive approval that wasn't available — so this is a static verification of the diff, not a re-run of the reported evidence. Same limitation the earlier review hit.

All three fixes check out.

1. CAS reservation (internal/encryption/write_budget.go:141-166). The new loop reserves the slot before allowing:

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: Allow only ever fires on a winning CAS from used < threshold, so the counter can never advance past threshold through Record. That in turn makes WriteBudgetExhausted unreachable via Record except when threshold == ceiling (the degenerate small-ceiling case) — and the doc comment on WriteBudgetExhausted (lines 52-62) and TestWriteBudgetReachesExhaustedAtADegenerateCeiling both say so honestly now, instead of the earlier comment claiming a reachability that never actually happened. TestWriteBudgetRefusedWritesDoNotConsumeBudgetUnderConcurrency is a real detector this time — sustained contention (racers loop until refused, not one staged burst) rather than the original one-shot burst that the PR notes "also passed against the buggy code."

2. Threshold arithmetic (refusalThreshold, lines 122-133). Checked the decomposition by hand: for ceiling = 100q + r, 90q + floor(9r/10) == floor(9·ceiling/10) exactly (no approximation), and r < 100 bounds the remainder term at 8910, so there's no overflow at any uint64 ceiling — I verified the large-ceiling side too: ceiling/100*90 is at most 90% of ceiling, which always fits in uint64 since it's less than the input. Spot-checked the table cases by hand:

199 → 179 (matches floor(199*9/10)=179.1)
2^32 → 3865470566 (matches floor(2^32*9/10))
degenerate ceiling=1 → exact=0 → clamped to 1, so the DEK gets one write instead of being wedged forever.

All correct.

3. Design doc (docs/design/2026_04_29_partial_data_at_rest_encryption.md:37). The open 9C+ row now explicitly re-lists the §9.2 metrics (naming elastickv_encryption_writes_per_dek{key_id} and stating it isn't exported yet) and encrypted Jepsen, addressing CodeRabbit's point that narrowing the row had made that gap invisible.

Minor, non-blocking: the CAS loop is unbounded under pathological contention (a Record call can in principle retry indefinitely if it keeps losing the race right at the threshold boundary), but that's inherent to lock-free reservation and bounded in practice by the number of concurrent writers on one key — not worth a backoff for this hot path.

Nothing else to flag. Scope, doc comments, and tests are now mutually consistent, and the two new regression tests (TestWriteBudgetRefusedWritesDoNotConsumeBudgetUnderConcurrency, TestWriteBudgetThresholdIsNinetyPercent) are the kind that would have caught these on the first pass — good revert-check discipline per the PR description.
· branch design/encryption-9c4-write-budget

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

bootjp · 2026-09-12T10:31:02Z

Correction to my earlier comment on this PR.

I wrote that elastickv_encryption_writes_per_dek{key_id} "appears only in this design document — no Go file references it". That was true of this branch's base, but main has since merged #1221 (Stage 9C-1), which does export it from the storage-envelope path (monitoring/encryption.go:58). So the claim is now wrong, and the reviewer's original point — keep the metric visible in the open scope — is satisfied differently than I described: the counter exists; what remains is wiring this budget's accounting to it.

Fixed while merging main to clear the conflict. The milestone table now carries main's 9C-1 row alongside this stage's, and the open 9C+ row lists the metrics that genuinely remain unexported (active_dek_id, last_proposed_index_per_raft_dek, kek_unwrap_seconds, sidecar_raft_index) rather than one that has shipped.

No code change: the three fixes in this PR and their revert-checks are unaffected.

chatgpt-codex-connector

💡 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".

bootjp · 2026-09-13T05:36:46Z

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: Record on a nil budget returns Allow, so the write path said "proceed" while Remaining said "rotation due" about the same receiver — and any admission check or dashboard keyed on Remaining(k) == 0 would demand rotation on every unconfigured node forever.

Remaining now returns RemainingUnlimited (^uint64(0)) for the unwired state. I chose an explicit sentinel over, say, returning the ceiling, so that zero keeps exactly one meaning — rotation is due — and a caller that cares can distinguish "no budget" without inferring it.

P2 — Initialize the counter map for zero-value budgets. Confirmed; the revert-check reproduces the exact panic: assignment to entry in nil map. Your framing of why it matters is the right one: this type tolerates a nil receiver and its other methods tolerate a zero value, so panicking on a construction detail is the inconsistency.

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 refusalThreshold(0) refuses the very first write, so the panic would have become a wedged DEK. A test asserts the zero value agrees with NewWriteBudget(0).

Verification

go test ./internal/encryption/ -race -count=1 — ok 28.8s; golangci-lint — 0 issues
Revert-checks, each confirming the named test fails with the fix reverted and the file restores byte-exact:
- nil Remaining returns 0 → TestWriteBudgetNilReceiverAgreesAcrossItsAPIs FAILS
- no lazy map init → TestWriteBudgetZeroValueIsUsable panics with assignment to entry in nil map
- zero value keeps a zero ceiling → TestWriteBudgetZeroValueIsUsable FAILS on the default-ceiling and embedded cases

@codex review
@claude review

chatgpt-codex-connector · 2026-09-13T05:36:53Z

To use Codex here, create a Codex account and connect to github.

claude · 2026-09-13T05:36:59Z

Claude finished @bootjp's task in 2m 16s —— View job

Review: second-round fixes in `0992fc1`

Read repo guidelines (CLAUDE.md)
Read current internal/encryption/write_budget.go and write_budget_test.go
Verify the two fixes claimed in the trigger comment (nil Remaining sentinel, zero-value map init)
Check for regressions or new issues introduced by these fixes
Post final review

Could not execute go test / golangci-lint in this sandboxed review session — every go/shell invocation here required interactive approval that wasn't available, same limitation the two earlier reviews on this PR hit. This is a static verification of the diff (c49cca6..0992fc1) against the two findings from the 2026-09-12 Codex review, not a re-run of the reported test evidence.

Both fixes check out.

1. Nil-budget Remaining sentinel (write_budget.go:184-207). RemainingUnlimited = ^uint64(0) is exported with a doc comment explaining why zero was unsafe to overload, and Remaining now returns it only on a nil receiver. I checked whether the sentinel could collide with a legitimate value: Remaining can never exceed threshold, and threshold is capped at refusalThreshold(ceiling) ≤ 0.9·ceiling, so even at ceiling = math.MaxUint64 the largest real Remaining is ~10% below MaxUint64 — the sentinel is unreachable via normal computation, so there's no ambiguity between "real remaining count" and "unwired." ^uint64(0) also matches existing usage elsewhere in this repo (main_raft_envelope_wiring.go:16, store/mvcc_store.go:284), so it's idiomatic here, not a one-off.

2. Zero-value map init (counterFor, write_budget.go:260-270). The lazy make(map[uint32]*atomic.Uint64, 1) sits inside the existing write-lock double-check, after the nil-map-safe existence probe (b.counters[keyID] on a nil map is legal and returns ok=false), so the fix doesn't disturb the double-checked-locking shape reviewed earlier — it just closes the one path (first insert on an unconstructed budget) that used to panic.

Consistency check I specifically went looking for a hole in: ceilingOrDefault/refusalThresholdOrDefault treat a field as "unconfigured" via == 0. That's only sound if a properly-constructed budget can never legitimately end up with ceiling == 0 or threshold == 0. Confirmed: NewWriteBudget always substitutes the §5.2 default before storing ceiling (never stores 0), and refusalThreshold always clamps to at least 1 (never returns 0) — so the two fields are either both zero (true zero-value struct) or both non-zero (constructed), with no partial state. Record and Remaining both go through the same two helper methods, so they can't disagree with each other the way Record/Remaining disagreed before this fix.

The two new tests (TestWriteBudgetNilReceiverAgreesAcrossItsAPIs, TestWriteBudgetZeroValueIsUsable, including the embedded-struct case) are the right shape to pin this — they check cross-API agreement rather than just "doesn't panic," which is exactly what broke last time.

Nothing else to flag. No other call sites exist yet (WriteBudget/RemainingUnlimited are referenced only from the package's own files, confirming the PR's "nothing constructs it yet" scope claim still holds), and the design-doc row from the first round is untouched by this commit.
· branch design/encryption-9c4-write-budget

Copilot

🟢 Approval recommended

The implementation is isolated and well tested; only a non-blocking milestone-numbering ambiguity remains.

1 open finding

Milestone sequence skips undefined 9C-3 · New

What changed in this PR

Adds a process-local, per-DEK write-budget primitive for future encryption admission control.

Changes:

Enforces the 90% rotation threshold using atomic reservations.
Adds concurrency, boundary, nil-receiver, and per-DEK tests.
Updates the encryption roadmap.

File	Description
`internal/encryption/write_budget.go`	Implements write-budget accounting.
`internal/encryption/write_budget_test.go`	Tests budget invariants and concurrency.
`docs/design/2026_04_29_partial_data_at_rest_encryption.md`	Records milestone status.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

bootjp · 2026-10-22T09:21:05Z

Review fixes are complete on current HEAD 1df394bd.

rebuilt the branch from current main to clear conflicts and remove the unsigned historical commit
retained the 9C-3 startup unwrap-cache milestone and recorded this stage as 9C-4
preserved the CAS-based reservation, exact 90% threshold, nil-budget sentinel, and zero-value initialization fixes
audited production callers: none; this remains an unwired primitive
updated the PR description and removed prohibited attribution
verified go test -race ./internal/encryption/... -count=1, go vet ./internal/encryption/..., and git diff --check
local lint could not run because the binary was built with Go 1.26 while dependencies require Go 1.27; CI is authoritative
commit is signed by bootjp <contact@bootjp.me>

@codex review

chatgpt-codex-connector · 2026-10-22T09:23:25Z

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 1df394bd4c

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

bootjp · 2026-10-22T09:25:20Z

Review fixes are complete on current HEAD .\n\n- rebuilt the branch from current to clear conflicts and remove the unsigned historical commit\n- retained the 9C-3 startup unwrap-cache milestone and recorded this stage as 9C-4\n- preserved the CAS-based reservation, exact 90% threshold, nil-budget sentinel, and zero-value initialization fixes\n- audited production callers: none; this remains an unwired primitive\n- updated the PR description and removed prohibited attribution\n- verified ok github.com/bootjp/elastickv/internal/encryption 138.749s
ok github.com/bootjp/elastickv/internal/encryption/fsmwire 1.298s
ok github.com/bootjp/elastickv/internal/encryption/kek 1.980s, , and \n- local lint could not run because the binary was built with Go 1.26 while dependencies require Go 1.27; CI is authoritative\n- commit is signed by \n\n@codex review

chatgpt-codex-connector · 2026-10-22T09:27:12Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 1df394bd4c

ℹ️ 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".

chatgpt-codex-connector · 2026-10-22T09:27:41Z

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 1df394bd4c

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

bootjp · 2026-10-22T17:30:35Z

Resolved the main conflict in signed bootjp/contact commits a186116 and 9f83eb6 (current HEAD9f83eb62). Preserved merged #1226 startup cache, #1235 rewrite classifier and #1287 document update; main9a20fbb7 is an ancestor. Only the encryption status table needed manual resolution; original write_budget.go and its tests are byte-identical. No runtime semantic change; rg caller audit still finds no production budget callers. Encryption package race suite passed (86.692s plus fsmwire/kek), vet passed, scoped Go1.26.6 lint0 issues, diff check passed. Please review this final pushed HEAD. @codex review

chatgpt-codex-connector · 2026-10-22T17:32:51Z

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 9f83eb62de

ℹ️ 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".

chatgpt-codex-connector · 2026-10-22T17:33:34Z

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 9f83eb62de

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

bootjp · 2026-10-22T19:25:28Z

Integrated current main14c06c64 after #1295/#1296/#1293 merged, preserving all other main PRs. Final pushed HEAD: 0b26f97; signed bootjp/contact commits, GitHub Verified. Integration was conflict-free; write_budget.go and write_budget_test.go are byte-identical to the previous reviewed implementation1df394bd. Diff against main remains3 files: budget implementation/test and encryption status document, which still marks admission/auto-propose wiring and remaining Stage9 work open. rg caller audit finds no production budget callers; no runtime semantics were changed by this integration. Race suites passed for encryption (49.392s, plus all subpackages), backup3.264s and keyviz1.689s; encryption vet passed, CI-equivalent v2.14.0 diff lint0 issues, diff check passed. No duplicate fixture/reaper/KeyViz fixes were added: those arrive only through main. Earlier9f83 receipts are stale. Please review this final pushed HEAD. @codex review

chatgpt-codex-connector · 2026-10-22T19:27:05Z

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 0b26f970fe

ℹ️ 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".

chatgpt-codex-connector · 2026-10-22T19:28:11Z

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 0b26f970fe

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

chatgpt-codex-connector Bot reviewed Sep 10, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 12, 2026

View reviewed changes

Comment thread internal/encryption/write_budget.go

Comment thread internal/encryption/write_budget.go

Copilot AI balanced review requested due to automatic review settings October 11, 2026 10:44

Copilot started reviewing on behalf of bootjp October 11, 2026 10:48

View session

Copilot AI reviewed Oct 11, 2026

View reviewed changes

Comment thread docs/design/2026_04_29_partial_data_at_rest_encryption.md Outdated

encryption: add writes-per-DEK rotation budget

1df394b

bootjp force-pushed the design/encryption-9c4-write-budget branch from 83eeff3 to 1df394b Compare October 22, 2026 09:19

bootjp added 2 commits October 23, 2026 02:29

Merge main into encryption write budget

a186116

Merge latest design status updates

9f83eb6

bootjp added 2 commits October 23, 2026 02:44

Merge current main into encryption write budget

982c77d

Merge main after encrypted restart fixes

0b26f97

bootjp merged commit b086203 into main Oct 22, 2026
11 checks passed

bootjp deleted the design/encryption-9c4-write-budget branch October 22, 2026 19:37

This was referenced Oct 22, 2026

encryption: add the §5.4 DEK retirement eligibility classifier #1234

Merged

test: verify DynamoDB snapshot seals with real Raft #1290

Merged

main: transfer Raft leadership before shutdown #1291

Merged

Conversation

bootjp commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Behavior and risk

Main synchronization

Verification

Author

Uh oh!

bootjp commented Sep 10, 2026

Uh oh!

bootjp commented Sep 10, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

claude Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: §5.2 writes-per-DEK rotation budget

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

bootjp Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

bootjp Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

bootjp Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented Sep 11, 2026

Uh oh!

bootjp commented Sep 11, 2026

Verification

Uh oh!

chatgpt-codex-connector Bot commented Sep 11, 2026

Uh oh!

claude Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: fixes in 372c052

Uh oh!

bootjp commented Sep 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

bootjp commented Sep 13, 2026

Verification

Uh oh!

chatgpt-codex-connector Bot commented Sep 13, 2026

Uh oh!

claude Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: second-round fixes in 0992fc1

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

bootjp commented Sep 10, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 10, 2026 •

edited

Loading

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading

claude Bot commented Sep 10, 2026 •

edited

Loading

claude Bot commented Sep 11, 2026 •

edited

Loading

Review: fixes in `372c052`

claude Bot commented Sep 13, 2026 •

edited

Loading

Review: second-round fixes in `0992fc1`