Skip to content

fix: only treat a radix node that holds a value as a duplicate (closes #36) - #37

Merged
danielhuici merged 2 commits into
reverseame:apotheosis2from
Dani-giron:n1-radix-ghost-duplicate
Sep 1, 2026
Merged

danielhuici merged 2 commits into
reverseame:apotheosis2from
Dani-giron:n1-radix-ghost-duplicate

Conversation

@Dani-giron

Copy link
Copy Markdown

Summary

insert() decided whether a key was already present by checking only that RadixNode::find() returned a node. find() also returns internal nodes that hold no value: splitting the tree on a shared prefix moves the existing value down into a child and leaves the parent as a branch point with data: None. A record whose key matched one of those split points was reported as an existing key and silently discarded, even though nothing had ever been stored there.

search() already got this right, checking if let Some(Some(node_index)) = radix_node.data. Only insert() conflated "a node exists at this path" with "a record is stored at this path".

This affects variable-length keys, so the shipped RadixKeyMapping for u32 (integers encoded as decimal text) is vulnerable. TLSH keys are all the same length and an internal node path is always strictly shorter than a complete key, so the TLSH path was never affected.

Changes

  • src/controllers/apotheosis.rs: the duplicate check becomes self.radix.find(key).is_some_and(|node| node.data.is_some()), so only a node actually holding an index counts as a duplicate. One line plus a comment explaining why the node alone is not enough.

Test plan

Verified locally with the same commands CI runs: cargo clippy --all-targets --all-features -- -D warnings clean, cargo fmt --check clean, and all 44 tests passing.

Beyond that, the behavior was exercised before and after the change with the scenarios recorded in #36: the RadixNode-level mechanism, the minimal 12/13/1 case, insertion-order dependence, genuine duplicates, the TLSH path, and a 20,000-insert workload.

Before the fix, 588 of 18175 unique records were dropped (3.24%), with the accounting closing exactly at 17587 indexed plus 588 falsely rejected. After the fix, all 18175 unique records are indexed and none are falsely rejected, while the same 1825 genuine duplicates are still rejected and 12/13/1 yields 3 records in either insertion order.

@Dani-giron

Copy link
Copy Markdown
Author

Heads up: this PR will conflict with #27, same block in insert(), see the comment there. Both changes should be kept, not one over the other, whoever merges second needs to combine them by hand.

@danielhuici
danielhuici merged commit f2817ca into reverseame:apotheosis2 Sep 1, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants