fix(rekey): a reader leaving mid-deferral must not strand the retirement - #30
Open
presempathy-awb wants to merge 4 commits into
Open
fix(rekey): a reader leaving mid-deferral must not strand the retirement#30presempathy-awb wants to merge 4 commits into
presempathy-awb wants to merge 4 commits into
Conversation
retire_recorded previously stopped draining at the first failed retirement, leaving every obligation behind it stuck queued as well. Retirements are independent epochs, so it now retains only the entries that actually fail and keeps retiring the rest, reporting the first error once the pass is done.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29.
The defect
retire_rekey_source_when_safetook the decision to defer undertracked_readersand recorded the obligation underpending_key_retirements— two locks with a gap between them, and nothing holding the reader set still
across it.
So the last reader could leave inside that gap. Its
unregister_readis the onlycaller of
drain_pending_key_retirements, and it only drains when the reader vecbecomes empty, so it ran, read a still-empty obligation list, and returned. The
push landed a moment later in a list whose only reader had gone, and nothing
visits that list again unless another reader happens to register and drop.
The superseded master key then stayed leasable for the life of the handle while
rekey_dbreturnedOk(())— no error, no log line, no completion state toquery.
EpochKeyring::remove's own doc names what breaks: "without it thesuperseded master key stays leasable for the life of the handle, and the store
keeps answering under the key the rotation was performed to stop honouring."
The fix
Hold
pending_key_retirementsacross the reader check, so deciding to defer andrecording what was deferred are one step.
drain_pending_key_retirementstakesthe same two locks in the same order.
No call path nests them the other way:
unregister_readreleasestracked_readersbefore it drains, and nothing else in the tree touchespending_key_retirements. Whichever side gets there first, the outcome is thesame — if the reader is still registered the obligation is recorded before its
drain can run; if it has already left, the retirement happens inline.
Also stopped the drain discarding work it never attempted: it
mem::take-d thewhole list up front, so an error mid-loop dropped every remaining obligation, and
unregister_readonly logs the error. Entries now leave the list once theirretirement has succeeded. Latent today — the one reachable error is the
active-epoch refusal, which strictly increasing epochs already prevent — but it
is the same shape as the bug above.
How to test it
a_reader_leaving_mid_deferral_still_retires_the_source_epochdrives a readerout at exactly the instant the old code pushed into, then requires the epoch to
retire anyway. The interleaving is made deterministic by a
#[cfg(test)]hookinvoked inside the critical section, in the style of the existing
RekeyTestFault/VisibilityTestHookseams, rather than by a sleep or a race.It discriminates rather than merely passing: revert only the lock order in
retire_rekey_source_when_safeand it fails with "an obligation no reader iswaiting on must not survive the deferral".
The runtime is
current_threadon purpose — the interleaving comes from a realOS thread via
std::thread::scope, not from the executor, so it needs nort-multi-thread.Against
c6dbba8:cargo test --lib— 515 passedcargo clippy --lib --all-features— cleancargo fmt --check— cleanTradeoffs
The test seam adds one
#[cfg(test)]field toDbplus its two constructorsites. That is the cost of pinning this deterministically; the alternative is a
stress loop that passes both before and after the fix.
No format change, no public API change, no durability ordering change — the two
locks are in-process only, so there is nothing for an existing store to do on
upgrade.
Scope
Crash-recovery, resume and abort paths were already sound:
open/recovery.rsalways reaches
resume_rekey_intent→ retirement, refuses non-standalone openswith an intent in flight, and a crash before retirement achieves it implicitly
because a reopen installs only the header's epoch. This deferral gap was the only
leak I found.
Worth stating since it bounds what the fix buys: retirement drops the key from
the in-memory
EpochKeyring; it does not make the source epoch underivable,since
derive_mk(kek, kek_salt, epoch)reproduces it from the KEK at will.