Skip to content

fix(store): keep records-by-key index in sync on prefix delete - #106

Open
cbenhagen wants to merge 1 commit into
n0-computer:mainfrom
cbenhagen:fix/records-by-key-index-leak
Open

fix(store): keep records-by-key index in sync on prefix delete#106
cbenhagen wants to merge 1 commit into
n0-computer:mainfrom
cbenhagen:fix/records-by-key-index-leak

Conversation

@cbenhagen

@cbenhagen cbenhagen commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes a secondary-index leak in the redb (fs-store) replica store.

The store keeps two tables that must stay 1:1:

Table Key Role
records-1 (namespace, author, key) authoritative records
records-by-key-1 (namespace, key, author) secondary index, value ()

StoreInstance::remove_prefix_filtered (src/store/fs.rs) is the production prefix-delete path. It is called from Ranger::put whenever an entry supersedes its prefix children, which is exactly what Replica::delete_prefix triggers. It removed rows from records only and never removed the matching rows from records_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:

  • Unbounded store growth: orphan index rows in records-by-key-1 are never reclaimed and accumulate on every delete-heavy namespace.
  • Degraded / incorrect key-range queries: RecordsByKeyRange walks records-by-key-1 and dereferences into records-1; orphans are dangling pointers that, at best, waste work and, at worst, surface as lookup misses.

Changes

  • remove_prefix_filtered now drains the extract_from_if iterator into the set of removed keys (which performs the deletion from records) and then removes the corresponding row from records_by_key for each one, keeping the index 1:1 with records. The collect-first step releases the mutable borrow of tables.records before tables.records_by_key is mutated.
  • Added two regression tests asserting records_by_key.len() == records.len() after a delete:
    • test_remove_prefix_filtered_cleans_by_key_index exercises remove_prefix_filtered directly and confirms records under unrelated prefixes are untouched.
    • test_delete_prefix_keeps_by_key_index_in_sync drives the same path end-to-end through Replica::delete_prefix.

Breaking Changes

None. This is a behavioural bug fix internal to the fs-store backend; no public API changes.

Notes & open questions

  • This fix prevents new orphans. It does not retroactively prune orphan rows that already accumulated in existing stores. Because records-by-key-1 is a pure derived index, an existing store can be repaired offline (with the scheduler stopped) by clearing records-by-key-1 and re-inserting (namespace, key, author) -> () for every row in records-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_remove remains #[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.
  • Detection for the fleet: on any docs.redb, count(records-by-key-1) > count(records-1) means the leak is present and the delta is the orphan count.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

1 participant