Skip to content

test(timed_cache): de-flake test_expiry_removal (#1408) - #1409

Open
yashksaini-coder wants to merge 1 commit into
libp2p:mainfrom
yashksaini-coder:test/deflake-timed-cache-expiry
Open

test(timed_cache): de-flake test_expiry_removal (#1408)#1409
yashksaini-coder wants to merge 1 commit into
libp2p:mainfrom
yashksaini-coder:test/deflake-timed-cache-expiry

Conversation

@yashksaini-coder

Copy link
Copy Markdown
Contributor

Summary

Closes #1408.

test_expiry_removal fails intermittently on CI (seen on windows (3.11, core); green on all Linux runners and locally):

assert MSG_1 not in cache.cache
AssertionError: assert b'msg1' not in {b'msg1': <expiry-ts>}

Root cause

cache = LastSeenCache(ttl=2, sweep_interval=1)
cache.add(MSG_1)
await trio.sleep(2.1)  # Wait for sweeper to remove expired items
assert MSG_1 not in cache.cache

BaseTimedCache sweeps in a background thread (while not self._stop_event.wait(self.sweep_interval)) with an integer-second TTL. Removal therefore happens on a coarse ~1s cadence in a separate thread. The fixed trio.sleep(2.1) assumes the sweep tick lands within 2.1s; on a slow Windows runner the thread wakeup is delayed and the entry is still present when the assertion runs. A timing race, not a logic bug.

Fix

Poll until the sweeper removes the entry — the idiom test_simple_last_seen_cache already uses in this same file:

-await trio.sleep(2.1)  # Wait for sweeper to remove expired items
-assert MSG_1 not in cache.cache  # Should be removed
+with trio.fail_after(15):
+    while MSG_1 in cache.cache:
+        await trio.sleep(0.1)

Deterministic (exits as soon as the entry is swept) and non-hanging (fails after 15s if the sweeper is genuinely broken).

Scope

Limited to the test that actually failed in CI. Sibling tests in the file (test_simple_first_seen_cache, test_timed_cache_expiry, test_timed_cache_stress_test, test_readding_after_expiry) share the same fixed-sleep pattern and are latent flakes on the same root cause — notable as a follow-up, out of scope here.

Test plan

  • test_expiry_removal run 5× locally → stable pass
  • Full tests/core/tools/timed_cache/ → 8 passed
  • ruff check / ruff format --check clean
  • newsfragments/1408.internal.rst added

The test added an entry then waited a fixed trio.sleep(2.1) before
asserting the background sweeper had removed it. The sweeper runs in a
separate thread on a ~1s cadence (`_stop_event.wait(sweep_interval)`) and
the TTL is integer-second, so removal timing is coarse; on a slow CI
runner (seen on windows 3.11) the sweep tick landed after 2.1s and the
entry was still present -> 'assert MSG_1 not in cache.cache' failed.

Replace the fixed sleep + immediate assert with a bounded poll until the
entry is swept (the idiom test_simple_last_seen_cache already uses):

    with trio.fail_after(15):
        while MSG_1 in cache.cache:
            await trio.sleep(0.1)

Deterministic and non-hanging (15s timeout if the sweeper is broken).

Closes libp2p#1408.
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.

flaky: test_expiry_removal races the background sweeper thread (fixed trio.sleep)

1 participant