Skip to content

Fix CuckooFilter losing elements when an insert or expansion fails - #149

Merged
barrust merged 4 commits into
barrust:masterfrom
mayur-dhavan:cuckoo-preserve-elements-on-failure
Oct 27, 2026
Merged

barrust merged 4 commits into
barrust:masterfrom
mayur-dhavan:cuckoo-preserve-elements-on-failure

Conversation

@mayur-dhavan

Copy link
Copy Markdown
Contributor

Fix: CuckooFilter loses elements when an insert or expansion fails

When CuckooFilter.add() raises CuckooFilterFullError, 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 to CountingCuckooFilter.

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 exhausts max_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):

import random
from probables import CuckooFilter, CuckooFilterFullError

random.seed(0)
cko = CuckooFilter(capacity=4, bucket_size=1, max_swaps=1, auto_expand=False)
for k in ("k24", "k26", "k2", "k16"):
    cko.add(k)
try:
    cko.add("k32")
except CuckooFilterFullError:
    pass
print(cko.check("k16"))   # False on master: k16 was added, never removed

2. Failed auto-expansion: the table is left partially repopulated.
_expand_logic() resets self._cuckoo_capacity and self._buckets, then reinserts every fingerprint into the larger table. If one of those reinserts exhausts its cascade, it raises CuckooFilterFullError("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.

CountingCuckooFilter has 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.

Copilot AI balanced review requested due to automatic review settings October 26, 2026 09:49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@barrust
barrust merged commit cf7d471 into barrust:master Oct 27, 2026
9 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.

3 participants