diff --git a/shelfmark/release_sources/irc/cache.py b/shelfmark/release_sources/irc/cache.py index e0b4e844..4ba0aa47 100644 --- a/shelfmark/release_sources/irc/cache.py +++ b/shelfmark/release_sources/irc/cache.py @@ -13,7 +13,6 @@ from typing import Any from shelfmark.config import env from shelfmark.core.logger import setup_logger -from shelfmark.core.utils import is_audiobook as check_audiobook from shelfmark.release_sources import Release, ReleaseProtocol logger = setup_logger(__name__) @@ -56,12 +55,6 @@ def _coerce_timestamp(value: object) -> float: return 0.0 -def _generate_cache_key(provider: str, provider_id: str, content_type: str | None = None) -> str: - """Generate a cache key from provider, provider_id, and content type.""" - normalized_content_type = "audiobook" if check_audiobook(content_type) else "ebook" - return f"{provider}:{provider_id}:{normalized_content_type}" - - def _load_cache() -> dict[str, Any]: """Load cache from disk.""" try: @@ -103,17 +96,17 @@ def _dict_to_release(data: dict[str, Any]) -> Release: def get_cached_results( - provider: str, - provider_id: str, - content_type: str | None = None, + cache_key: str, ttl_seconds: int | None = None, ) -> dict[str, Any] | None: - """Get cached search results for a book. + """Get the cached IRC answer for a query identity (server:channel:query). + + The cache stores the whole answer (releases for all content types) under the query + identity, so it is not isolated by book or content type. Callers filter by content + type after reading. Args: - provider: Metadata provider name (e.g., "hardcover", "openlibrary") - provider_id: Book ID in the provider's system - content_type: Search content type for cache isolation + cache_key: Query identity (e.g. "server:channel:query") ttl_seconds: Cache TTL in seconds (from settings) Returns: @@ -127,8 +120,6 @@ def get_cached_results( ttl_value = config.get("IRC_CACHE_TTL", DEFAULT_CACHE_TTL) ttl_seconds = _coerce_cache_ttl(ttl_value, DEFAULT_CACHE_TTL) - cache_key = _generate_cache_key(provider, provider_id, content_type) - with _cache_lock: cache = _load_cache() entry = cache.get("entries", {}).get(cache_key) @@ -141,10 +132,9 @@ def get_cached_results( age = time.time() - cached_at if ttl_seconds != 0 and age > ttl_seconds: - title = entry.get("title", cache_key) logger.debug( "IRC cache expired for '%s' (age: %.0fs > TTL: %ss)", - title, + entry.get("title", cache_key), age, ttl_seconds, ) @@ -153,44 +143,36 @@ def get_cached_results( # Convert dicts back to Release objects releases = [_dict_to_release(r) for r in entry.get("releases", [])] - online_servers = entry.get("online_servers", []) - title = entry.get("title", "") logger.info( "IRC cache hit for '%s' (%s releases, age: %.0fs)", - title, + entry.get("title", ""), len(releases), age, ) return { "releases": releases, - "online_servers": online_servers, + "online_servers": entry.get("online_servers", []), "cached_at": cached_at, } def cache_results( - provider: str, - provider_id: str, + cache_key: str, title: str, releases: list[Release], - content_type: str | None = None, online_servers: list[str] | None = None, ) -> None: - """Cache search results for a book. + """Cache the whole IRC answer for a query identity. Args: - provider: Metadata provider name - provider_id: Book ID in the provider's system - title: Book title (for logging/display) - releases: List of Release objects from search - content_type: Search content type for cache isolation + cache_key: Query identity (e.g. "server:channel:query") + title: Query text (for logging/display) + releases: All Release objects from the search (every content type) online_servers: List of online server nicks (optional) """ - cache_key = _generate_cache_key(provider, provider_id, content_type) - with _cache_lock: cache = _load_cache() @@ -198,9 +180,6 @@ def cache_results( cache["entries"] = {} cache["entries"][cache_key] = { - "provider": provider, - "provider_id": provider_id, - "content_type": "audiobook" if check_audiobook(content_type) else "ebook", "title": title, "releases": [_release_to_dict(r) for r in releases], "online_servers": list(online_servers) if online_servers else [], @@ -211,27 +190,23 @@ def cache_results( logger.info("Cached %s IRC releases for '%s'", len(releases), title) -def invalidate_cache(provider: str, provider_id: str, content_type: str | None = None) -> bool: +def invalidate_cache(cache_key: str) -> bool: """Remove a specific entry from the cache. Args: - provider: Metadata provider name - provider_id: Book ID in the provider's system - content_type: Search content type for cache isolation + cache_key: Query identity to remove Returns: True if entry was found and removed """ - cache_key = _generate_cache_key(provider, provider_id, content_type) - with _cache_lock: cache = _load_cache() - entry = cache.get("entries", {}).get(cache_key) - title = entry.get("title", cache_key) if entry else cache_key + entries = cache.get("entries", {}) - if cache_key in cache.get("entries", {}): - del cache["entries"][cache_key] + if cache_key in entries: + title = entries[cache_key].get("title", cache_key) + del entries[cache_key] _save_cache(cache) logger.info("Invalidated IRC cache for '%s'", title) return True diff --git a/shelfmark/release_sources/irc/source.py b/shelfmark/release_sources/irc/source.py index d3b20575..63b011d3 100644 --- a/shelfmark/release_sources/irc/source.py +++ b/shelfmark/release_sources/irc/source.py @@ -15,6 +15,7 @@ if TYPE_CHECKING: from shelfmark.api.websocket import ws_manager from shelfmark.core.config import config from shelfmark.core.logger import setup_logger +from shelfmark.core.utils import is_audiobook from shelfmark.release_sources import ( ColumnColorHint, ColumnRenderType, @@ -88,12 +89,16 @@ def _emit_status(message: str, phase: str = "searching") -> None: MIN_SEARCH_INTERVAL = 15.0 _last_search_time: float = 0 -# Per-query cooldown: never re-post an identical search to the channel within this -# window, even when a user hits "Refresh" or retries a book that returned nothing. -# This stops retry loops from flooding the channel with the same message over and over. -# One day: results don't change minute-to-minute, so there's no reason to re-ask sooner. -QUERY_COOLDOWN_SECONDS = 24 * 60 * 60 # 1 day -_recent_query_times: dict[str, float] = {} +# Anti-spam budget: the exact same message may only be posted to the channel a limited +# number of times within a rolling window. This stops a retry/refresh loop from flooding +# the channel with the same line over and over, while still allowing a few genuine retries +# (a search that came back empty can be tried again, and Refresh works until the budget runs +# out). Normal use never hits this: successful searches are served from the result cache +# without re-posting at all. +MAX_IDENTICAL_SENDS = 3 +IDENTICAL_SEND_WINDOW_SECONDS = 24 * 60 * 60 # 24 hours +# message-send-key -> timestamps of recent posts of that exact message +_recent_message_sends: dict[str, list[float]] = {} def _enforce_rate_limit() -> None: @@ -109,29 +114,34 @@ def _enforce_rate_limit() -> None: _last_search_time = time.time() -def _query_cooldown_key(channel: str, query: str) -> str: - """Build a normalized key identifying a search posted to a channel.""" - return f"{channel.casefold()}:{query.casefold().strip()}" +def _query_identity(server: str, channel: str, query: str) -> str: + """Stable identity for a query on a given IRC server-channel. + + Used as BOTH the result-cache key and the per-query send-counter key, so the same + query shares one cached answer and one send budget regardless of which book or + content type triggered it. + """ + return f"{server.casefold()}:{channel.casefold()}:{query.strip().casefold()}" -def _query_on_cooldown(key: str) -> bool: - """Return True if an identical query was posted to the channel recently.""" - last = _recent_query_times.get(key) - if last is None: - return False - return (time.time() - last) < QUERY_COOLDOWN_SECONDS +def _recent_send_count(key: str) -> int: + """Number of times this exact message was posted within the rolling window.""" + cutoff = time.time() - IDENTICAL_SEND_WINDOW_SECONDS + timestamps = [ts for ts in _recent_message_sends.get(key, []) if ts > cutoff] + if timestamps: + _recent_message_sends[key] = timestamps + else: + _recent_message_sends.pop(key, None) + return len(timestamps) -def _record_query_sent(key: str) -> None: - """Record that a query was just posted to the channel (for cooldown).""" +def _record_message_sent(key: str) -> None: + """Record that an exact message was just posted to the channel.""" now = time.time() - _recent_query_times[key] = now - # Opportunistically prune stale entries so the dict can't grow unbounded. - stale = [ - k for k, sent_at in _recent_query_times.items() if now - sent_at > QUERY_COOLDOWN_SECONDS - ] - for k in stale: - _recent_query_times.pop(k, None) + cutoff = now - IDENTICAL_SEND_WINDOW_SECONDS + timestamps = [ts for ts in _recent_message_sends.get(key, []) if ts > cutoff] + timestamps.append(now) + _recent_message_sends[key] = timestamps @register_source("irc") @@ -216,14 +226,6 @@ class IRCReleaseSource(ReleaseSource): logger.debug("IRC source is disabled, skipping search") return [] - # Check cache first (unless expand_search/refresh is requested) - if not expand_search: - cached = get_cached_results(book.provider, book.provider_id, content_type=content_type) - if cached: - _emit_status("Using cached results", phase="complete") - self._online_servers = set(cached.get("online_servers", [])) - return cached["releases"] - # Build search query query = plan.primary_query or self._build_query(book) if not query: @@ -248,17 +250,37 @@ class IRCReleaseSource(ReleaseSource): _emit_status("IRC search bot not configured", phase="error") return [] - # Don't re-post an identical query to the channel within the cooldown window, - # even on refresh. This is what stops a frustrated user (or a retry loop) from - # spamming the same title over and over. Fall back to whatever is cached. - cooldown_key = _query_cooldown_key(channel, query) - if _query_on_cooldown(cooldown_key): - logger.info("IRC query on cooldown, not re-posting to channel: %s", query) - _emit_status("Search sent recently — showing latest results", phase="complete") - cached = get_cached_results(book.provider, book.provider_id, content_type=content_type) + # One identity per query on this server-channel. The result cache and the send + # counter are both keyed on it: the SAME query shares one cached answer and one + # send budget regardless of which book/content type triggered it, while different + # queries are independent (searching 100 different books posts 100 messages). + requested = "audiobook" if is_audiobook(content_type) else "ebook" + query_key = _query_identity(server, channel, query) + + # Serve the cached whole answer for an identical query (unless this is a refresh). + if not expand_search: + cached = get_cached_results(query_key) + if cached: + _emit_status("Using cached results", phase="complete") + self._online_servers = set(cached.get("online_servers", [])) + return self._filter_by_content_type(cached["releases"], requested) + + # Anti-spam cap: the exact same query may only be POSTED a limited number of times + # per window, even via refresh. Beyond that, serve whatever is cached rather than + # re-posting the identical message to the channel. + if _recent_send_count(query_key) >= MAX_IDENTICAL_SENDS: + logger.info( + "IRC query hit %s-send limit in window, not re-posting: %s", + MAX_IDENTICAL_SENDS, + query, + ) + _emit_status( + "Search limit reached for this query — showing latest results", phase="complete" + ) + cached = get_cached_results(query_key) if cached: self._online_servers = set(cached.get("online_servers", [])) - return cached["releases"] + return self._filter_by_content_type(cached["releases"], requested) return [] logger.info("IRC search: %s", query) @@ -282,28 +304,26 @@ class IRCReleaseSource(ReleaseSource): self._online_servers = client.online_servers # Send search request (always addressed to the search bot, never bare) - search_msg = f"@{search_bot} {query}" - client.send_message(f"#{channel}", search_msg) - _record_query_sent(cooldown_key) + client.send_message(f"#{channel}", f"@{search_bot} {query}") + _record_message_sent(query_key) - # Wait for results DCC - this is the long wait + # Wait for results DCC - this is the long wait. + # Don't restrict the sender to the trigger bot's nick: many channels answer an + # "@search" from a differently-named results bot. The DCC endpoint/filename are + # still validated, and wait_for_dcc falls back to the channel's server list. _emit_status(f"Connected to #{channel} - Waiting for results...", phase="searching") - wait_kwargs = {"expected_senders": {search_bot}} if search_bot else {} - offer = client.wait_for_dcc(timeout=60.0, result_type=True, **wait_kwargs) + offer = client.wait_for_dcc(timeout=60.0, result_type=True) + + online_servers = list(self._online_servers) if self._online_servers else None + if not offer: logger.info("No search results received") _emit_status("No results found", phase="complete") # Release connection for reuse (don't close it) connection_manager.release_connection(client) - # Cache empty result to avoid repeated failed searches - cache_results( - book.provider, - book.provider_id, - book.title, - [], - content_type=content_type, - online_servers=list(self._online_servers) if self._online_servers else None, - ) + # Cache the (empty) answer under the query identity so an identical query + # is served from cache instead of re-posting. + cache_results(query_key, query, [], online_servers=online_servers) return [] # Download results file @@ -321,19 +341,22 @@ class IRCReleaseSource(ReleaseSource): # Release connection for reuse (don't close it) connection_manager.release_connection(client) - # Convert to Release objects - results = parse_results_file(content, content_type=content_type) - releases = self._convert_to_releases(results, content_type=content_type) - - # Cache results - cache_results( - book.provider, - book.provider_id, - book.title, - releases, - content_type=content_type, - online_servers=list(self._online_servers) if self._online_servers else None, + # A single "@search" returns one file containing every format. Parse the whole + # answer (both ebooks and audiobooks) and cache it under the query identity, so + # requesting the other content type is served from cache without re-posting. + ebook_releases = self._convert_to_releases( + parse_results_file(content, content_type="ebook"), content_type="ebook" ) + audiobook_releases = self._convert_to_releases( + parse_results_file(content, content_type="audiobook"), content_type="audiobook" + ) + cache_results( + query_key, + query, + ebook_releases + audiobook_releases, + online_servers=online_servers, + ) + releases = audiobook_releases if requested == "audiobook" else ebook_releases except DCCError as e: logger.exception("DCC error during search") @@ -451,6 +474,15 @@ class IRCReleaseSource(ReleaseSource): return releases + @staticmethod + def _filter_by_content_type(releases: list[Release], requested: str) -> list[Release]: + """Pick the requested content type out of a cached whole answer. + + The cache stores releases for every content type under one query identity; each + release is tagged with its content type (defaulting to ebook when missing). + """ + return [release for release in releases if (release.content_type or "ebook") == requested] + @staticmethod def _parse_size(size_str: str) -> int | None: """Parse human-readable size (e.g., '1.2MB', '500K') to bytes.""" diff --git a/tests/irc/test_cache.py b/tests/irc/test_cache.py index ab901af2..efdfb1a4 100644 --- a/tests/irc/test_cache.py +++ b/tests/irc/test_cache.py @@ -2,26 +2,29 @@ from shelfmark.release_sources import Release from shelfmark.release_sources.irc import cache -def test_cache_results_isolated_by_content_type(monkeypatch): +def test_cache_results_round_trip_by_query_identity(monkeypatch): + """The whole answer (all content types) is cached under one query identity.""" state = {"entries": {}, "version": 1} monkeypatch.setattr(cache, "_load_cache", lambda: state) monkeypatch.setattr(cache, "_save_cache", lambda _cache: None) - ebook_release = Release(source="irc", source_id="ebook", title="Shared Title", format="epub") - audiobook_release = Release(source="irc", source_id="audio", title="Shared Title", format="zip") - - cache.cache_results("hardcover", "123", "Shared Title", [ebook_release], content_type="ebook") - cache.cache_results( - "hardcover", "123", "Shared Title", [audiobook_release], content_type="audiobook" + ebook_release = Release( + source="irc", source_id="ebook", title="Shared Title", format="epub", content_type="ebook" + ) + audiobook_release = Release( + source="irc", + source_id="audio", + title="Shared Title", + format="mp3", + content_type="audiobook", ) - ebook_cached = cache.get_cached_results( - "hardcover", "123", content_type="ebook", ttl_seconds=60 - ) - audiobook_cached = cache.get_cached_results( - "hardcover", "123", content_type="audiobook", ttl_seconds=60 - ) + key = "irc.example.net:ebooks:words of radiance" + cache.cache_results(key, "words of radiance", [ebook_release, audiobook_release]) - assert [release.source_id for release in ebook_cached["releases"]] == ["ebook"] - assert [release.source_id for release in audiobook_cached["releases"]] == ["audio"] + cached = cache.get_cached_results(key, ttl_seconds=60) + assert {release.source_id for release in cached["releases"]} == {"ebook", "audio"} + + # A different query identity is isolated. + assert cache.get_cached_results("irc.example.net:ebooks:other query", ttl_seconds=60) is None diff --git a/tests/irc/test_source.py b/tests/irc/test_source.py index 80fa1bf1..c0eb0927 100644 --- a/tests/irc/test_source.py +++ b/tests/irc/test_source.py @@ -1,3 +1,4 @@ +import time from types import SimpleNamespace from shelfmark.metadata_providers import BookMetadata @@ -46,9 +47,19 @@ def test_search_uses_cached_results_without_opening_a_connection(monkeypatch): ) monkeypatch.setattr(source, "is_available", lambda: True) + monkeypatch.setattr( + irc_source, + "_config_text", + lambda key: { + "IRC_SERVER": "irc.example.net", + "IRC_CHANNEL": "ebooks", + "IRC_NICK": "tester", + "IRC_SEARCH_BOT": "search", + }.get(key, ""), + ) monkeypatch.setattr( "shelfmark.release_sources.irc.cache.get_cached_results", - lambda provider, provider_id, *, content_type: { + lambda cache_key, *_args, **_kwargs: { "releases": [cached_release], "online_servers": ["AudioBot"], }, @@ -102,29 +113,25 @@ def test_search_no_dcc_offer_releases_connection_and_caches_empty_result(monkeyp "IRC_SEARCH_BOT": "search", }.get(key, ""), ) - # Ensure no leftover cooldown entry from a previous test blocks the send. - irc_source._recent_query_times.clear() + # Ensure no leftover send budget from a previous test blocks the send. + irc_source._recent_message_sends.clear() monkeypatch.setattr(source, "is_available", lambda: True) monkeypatch.setattr(irc_source, "_enforce_rate_limit", lambda: None) monkeypatch.setattr(irc_source, "_emit_status", lambda *_args, **_kwargs: None) monkeypatch.setattr( "shelfmark.release_sources.irc.cache.get_cached_results", - lambda provider, provider_id, *, content_type: None, + lambda cache_key, *_args, **_kwargs: None, ) monkeypatch.setattr( "shelfmark.release_sources.irc.cache.cache_results", - lambda provider, provider_id, title, releases, *, content_type, online_servers: ( - cache_calls.append( - { - "provider": provider, - "provider_id": provider_id, - "title": title, - "releases": releases, - "content_type": content_type, - "online_servers": online_servers, - } - ) + lambda cache_key, title, releases, *, online_servers=None: cache_calls.append( + { + "cache_key": cache_key, + "title": title, + "releases": releases, + "online_servers": online_servers, + } ), ) monkeypatch.setattr( @@ -143,13 +150,12 @@ def test_search_no_dcc_offer_releases_connection_and_caches_empty_result(monkeyp assert releases == [] assert released_clients == [client] + # One query maps to one cache entry (the whole, empty answer), keyed by server:channel:query. assert cache_calls == [ { - "provider": "hardcover", - "provider_id": "abc", + "cache_key": "irc.example.net:ebooks:missing result", "title": "Missing Result", "releases": [], - "content_type": "audiobook", "online_servers": ["AudioBot"], } ] @@ -167,7 +173,7 @@ def test_search_without_search_bot_never_posts_to_channel(monkeypatch): monkeypatch.setattr(irc_source, "_enforce_rate_limit", lambda: None) monkeypatch.setattr( "shelfmark.release_sources.irc.cache.get_cached_results", - lambda provider, provider_id, *, content_type: None, + lambda cache_key, *_args, **_kwargs: None, ) monkeypatch.setattr( irc_source, @@ -192,8 +198,30 @@ def test_search_without_search_bot_never_posts_to_channel(monkeypatch): assert source.search(book, plan) == [] -def test_search_on_cooldown_returns_cache_without_reposting(monkeypatch): - """An identical query within the cooldown window must not be re-posted to the channel.""" +def test_recent_send_count_caps_and_windows(): + """The send budget counts identical queries and prunes entries outside the window.""" + import shelfmark.release_sources.irc.source as irc_source + + irc_source._recent_message_sends.clear() + key = irc_source._query_identity("irc.example.net", "ebooks", "Dubliners") + other = irc_source._query_identity("irc.example.net", "ebooks", "Ulysses") + + assert irc_source._recent_send_count(key) == 0 + for expected in range(1, irc_source.MAX_IDENTICAL_SENDS + 1): + irc_source._record_message_sent(key) + assert irc_source._recent_send_count(key) == expected + + # A different query has its own independent budget. + assert irc_source._recent_send_count(other) == 0 + + # Timestamps older than the window are pruned and don't count. + stale = time.time() - irc_source.IDENTICAL_SEND_WINDOW_SECONDS - 10 + irc_source._recent_message_sends[key] = [stale, stale] + assert irc_source._recent_send_count(key) == 0 + + +def test_search_send_budget_blocks_repost_and_returns_cache(monkeypatch): + """Once the exact query hit its per-window send limit, don't re-post; serve cache.""" import shelfmark.release_sources.irc.source as irc_source source = IRCReleaseSource() @@ -214,7 +242,7 @@ def test_search_on_cooldown_returns_cache_without_reposting(monkeypatch): ) monkeypatch.setattr( "shelfmark.release_sources.irc.cache.get_cached_results", - lambda provider, provider_id, *, content_type: { + lambda cache_key, *_args, **_kwargs: { "releases": [cached_release], "online_servers": ["AudioBot"], }, @@ -222,18 +250,20 @@ def test_search_on_cooldown_returns_cache_without_reposting(monkeypatch): monkeypatch.setattr( "shelfmark.release_sources.irc.connection_manager.connection_manager.get_connection", lambda **_kwargs: (_ for _ in ()).throw( - AssertionError("cooldown should skip IRC connection") + AssertionError("send budget should skip IRC connection") ), ) - # Pretend the same query was just posted to the channel. - irc_source._recent_query_times.clear() - irc_source._record_query_sent(irc_source._query_cooldown_key("ebooks", "Cooldown Book")) + # Exhaust the budget for this exact query on this server-channel. + irc_source._recent_message_sends.clear() + send_key = irc_source._query_identity("irc.example.net", "ebooks", "Budget Book") + for _ in range(irc_source.MAX_IDENTICAL_SENDS): + irc_source._record_message_sent(send_key) - book = BookMetadata(provider="hardcover", provider_id="cd", title="Cooldown Book") - plan = SimpleNamespace(primary_query="Cooldown Book") + book = BookMetadata(provider="hardcover", provider_id="cd", title="Budget Book") + plan = SimpleNamespace(primary_query="Budget Book") - # expand_search=True bypasses the normal top-level cache, forcing the cooldown path. + # expand_search=True bypasses the top-level cache, forcing the budget path. releases = source.search(book, plan, expand_search=True) assert releases == [cached_release]