refactor(fqdn-store): bound the store with pingora-memory-cache - #454
Merged
Merged
Conversation
The DNS-snoop `ip -> fqdn` store was an unbounded DashMap kept in check by a dedicated thread doing a full `retain` sweep every 60s. Because the TTL floor is 60s, every binding lives at least a minute, so a host resolving at a high rate — or anything spraying lookups — grew the map freely between sweeps with no ceiling. Back it with pingora-memory-cache (S3-FIFO + TinyLFU), already a workspace dependency. That caps live bindings at a fixed capacity and expires lazily on read, so the sweeper thread and its periodic O(n) scan both go away. Lookups stay TTL-respecting (`get`, not `get_stale`). A binding past its TTL is still treated as absent: the address may have been reassigned since, and serving the old name would block whoever holds it now — the reason the TTL is capped in the first place. Behaviour on expiry is therefore unchanged. The one new failure mode is eviction under capacity pressure, which turns a live binding into a miss and silently leaves a `dns.fqdn` rule unmatched. Capacity is sized well above a full TTL window of answers, and lookups are now counted by outcome (hit / expired / miss) so the pressure is visible rather than silent. Entries are keyed by the hash of the address rather than the address itself, so a 64-bit collision would mis-attribute a binding — negligible at this capacity, and the same tradeoff the other pingora-backed caches here already accept. Tests covering the counters and behaviour under capacity pressure share a lock, since the store and its counters are process-global and would otherwise race.
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.
Stacked on #453 — review/merge that first; this PR targets its branch so the diff stays to the store swap alone.
Problem
The DNS-snoop
ip -> fqdnstore is an unboundedDashMapkept in check by a dedicated thread doing a fullretainsweep every 60s. Since the TTL floor is 60s, every binding lives at least a minute, so a host resolving at a high rate — or anything spraying lookups — grows the map freely between sweeps, with no ceiling. The sweep itself is an O(n) pass taking write locks across shards.Change
Back it with
pingora-memory-cache(S3-FIFO + TinyLFU), already a workspace dependency used by synapse-proxy / -core / -worker / -waf. That gives:Lookups stay TTL-respecting — deliberately
get, notget_staleWorth stating explicitly, since "prefer stale over a miss" looks appealing for a security lookup and is the wrong call here.
lookup()has always failed open on expiry, by design: the TTL is capped precisely "so stale bindings expire", because an address may be reassigned. Serving the old name after expiry would block whoever holds that address now — trading a known, bounded failure mode for collateral damage against an innocent host.So expiry behaviour is unchanged. The only genuinely new failure mode is eviction under capacity pressure, which turns a live binding into a miss and silently leaves a
dns.fqdnrule unmatched.Mitigated two ways rather than by resurrecting expired data:
lookup_hits/lookup_expired/lookup_misses— so misses climbing against a steady record rate makes the pressure visible instead of silentTradeoff worth a reviewer's eye
MemoryCachekeys by the hash of the address, not the address itself, so a 64-bit collision would mis-attribute one binding to another. Negligible at this capacity, and the same tradeoff the other pingora-backed caches in this repo already accept — but it is a real semantic difference from theDashMapit replaces.Tests
The store and its counters are process-global, so the tests that record or look up share a lock — without it the counter deltas race against other tests in the same binary (caught while writing them).
Verification
cargo +nightly fmt -- --check— cleancargo clippy --locked --workspace --all-targets -- --deny warnings— cleancargo clippy --locked --workspace --features classifier --all-targets -- --deny warnings— cleancargo test --locked --workspace --lib— all 19 binaries greenfqdn_storesuite run repeatedly to confirm no flakiness; feat(dns): decide dns.fqdn rules when the answer is snooped #453'sdns_preblock+dns_fqdn_reprostill pass on the new backendCargo.lockmoves by exactly one line (the new dependency edge)Unrelated, noted in passing:
synapse-core'sservice_graph::tests::buffer_behaviourfailed once under parallel execution and then passed 5/5 in isolation and 4/4 in full runs. It is independent of this change (synapse-core depends on none of the touched crates), but it is an intermittent flake sitting in the wellness gate.