fix(store): keep records-by-key index in sync on prefix delete - #106
Open
cbenhagen wants to merge 1 commit into
Open
fix(store): keep records-by-key index in sync on prefix delete#106cbenhagen wants to merge 1 commit into
cbenhagen wants to merge 1 commit into
Conversation
`remove_prefix_filtered` deleted rows from the `records` table only and never removed the matching rows from the `records-by-key` secondary index. Since it is the production prefix-delete path (reached via `Replica::delete_prefix`), every prefix-deleted record leaked an orphan index row that was never reclaimed, causing unbounded store growth and dangling pointers in key-range queries. Remove the corresponding `records-by-key` row for each deleted record, keeping the index 1:1 with `records`. Add regression tests asserting `records_by_key.len() == records.len()` after a delete.
4 tasks
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.
Description
Fixes a secondary-index leak in the redb (
fs-store) replica store.The store keeps two tables that must stay 1:1:
records-1(namespace, author, key)records-by-key-1(namespace, key, author)()StoreInstance::remove_prefix_filtered(src/store/fs.rs) is the production prefix-delete path. It is called fromRanger::putwhenever an entry supersedes its prefix children, which is exactly whatReplica::delete_prefixtriggers. It removed rows fromrecordsonly and never removed the matching rows fromrecords_by_key. Every prefix-deleted record therefore leaked an orphan index row that was never reclaimed.The only code that removed from both tables was
entry_remove, which is#[cfg(test)]and never runs in production, so there was no live code path that kept the index in sync on delete.Consequences:
records-by-key-1are never reclaimed and accumulate on every delete-heavy namespace.RecordsByKeyRangewalksrecords-by-key-1and dereferences intorecords-1; orphans are dangling pointers that, at best, waste work and, at worst, surface as lookup misses.Changes
remove_prefix_filterednow drains theextract_from_ifiterator into the set of removed keys (which performs the deletion fromrecords) and then removes the corresponding row fromrecords_by_keyfor each one, keeping the index 1:1 withrecords. The collect-first step releases the mutable borrow oftables.recordsbeforetables.records_by_keyis mutated.records_by_key.len() == records.len()after a delete:test_remove_prefix_filtered_cleans_by_key_indexexercisesremove_prefix_filtereddirectly and confirms records under unrelated prefixes are untouched.test_delete_prefix_keeps_by_key_index_in_syncdrives the same path end-to-end throughReplica::delete_prefix.Breaking Changes
None. This is a behavioural bug fix internal to the
fs-storebackend; no public API changes.Notes & open questions
records-by-key-1is a pure derived index, an existing store can be repaired offline (with the scheduler stopped) by clearingrecords-by-key-1and re-inserting(namespace, key, author) -> ()for every row inrecords-1. A migration could be added if we want this to self-heal on open; left out of this PR to keep it focused.entry_removeremains#[cfg(test)]. The single-entry remove is only used by tests, and the prefix path is the one that runs in production. Worth considering whether both deletes should route through one shared helper so the index-cleanup can't drift out of the live path again.docs.redb,count(records-by-key-1) > count(records-1)means the leak is present and the delta is the orphan count.Change checklist