Repository navigation
Fix CuckooFilter losing elements when an insert or expansion fails - #149
Merged
barrust merged 4 commits intoOct 27, 2026
Merged
Conversation
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.
Fix: CuckooFilter loses elements when an insert or expansion fails
When
CuckooFilter.add()raisesCuckooFilterFullError, the filter should contain exactly the same elements as before the call:check()promises "True if the element is likely present, False if it is definitely not present", so a failed insert silently dropping a previously added element breaks that contract. The same applies toCountingCuckooFilter.There are two failure paths, each losing elements in its own way:
1. Failed insert with
auto_expand=False: the carried fingerprint is dropped._insert_fingerprint()relocates fingerprints along the kick-out path; every swap keeps each fingerprint in one of its two valid buckets, so the buckets stay consistent. But if the cascade exhaustsmax_swaps, the fingerprint it is still carrying is returned and discarded, and the element it belonged to is no longer in the filter, even though it was added successfully earlier and never removed.Reproducer (master):
2. Failed auto-expansion: the table is left partially repopulated.
_expand_logic()resetsself._cuckoo_capacityandself._buckets, then reinserts every fingerprint into the larger table. If one of those reinserts exhausts its cascade, it raisesCuckooFilterFullError("The CuckooFilter failed to expand")at a point where the filter has the new capacity but only some of its elements: everything not yet reinserted is lost.CountingCuckooFilterhas the same two problems in_insert_fingerprint_alt()and its own_expand_logic().Fix
_insert_fingerprint()/_insert_fingerprint_alt(): record the swap sequence; if the cascade is exhausted, replay it in reverse so every displaced fingerprint returns to its original bucket, then return the original (uninserted) fingerprint. The exception is raised against a filter whose contents are unchanged._expand_logic()(both classes): snapshot_cuckoo_capacity,_buckets, and the inserted-elements count before mutating (the snapshot is by reference, so O(1)); restore all of them on any exception, then re-raise.Cost: the insert path now allocates one journal list and appends one tuple per swap on every cascade (bounded by
max_swaps), and pays the reverse replay only when the cascade actually fails; the expansion path pays only the O(1) reference snapshot on top of the reinsertion work it already did.Scope: this fixes rollback after an exhausted insertion cascade or a failed expansion. Rolling back the initial cascade also changes the reinsertion order used during auto-expansion, so some operations that failed before may succeed. The public API and defaults are unchanged. Unrelated pre-existing issues in the counting filter's element accounting are outside this change.
Tests
Four new regression tests, each of which fails on master and passes with the fix:
test_cuckoo_filter_full_preserves_elements(failed insert,auto_expand=False, base filter)test_cuckoo_filter_failed_expand_preserves_elements(failed expansion,auto_expand=True)test_c_cuckoo_filter_full_preserves_elements(failed insert, counting filter)test_c_cuckoo_filter_failed_expand_preserves_elements(failed expansion, counting filter)Full test suite passes under pytest (the repo's CI runner): 314 tests on master, 318 with this patch. I also ran a randomized property fuzz (add/remove/check against a reference set, including forced-full and forced-expansion-failure configurations) which reproduces the element loss on master and is clean with the fix; the harness is available on request.