From 96898e9bc63ec9ec5070ee9c83c65599f956de82 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 6 Mar 2026 10:20:20 +0100 Subject: [PATCH 01/13] [FEATURE] Conditional HTTP GET for Blacklist Downloads (ETag / Last-Modified) #3282 [FEATURE] Conditional HTTP GET for Blacklist Downloads (ETag / Last-Modified) #3282 --- .../core/blacklist/jobs/blacklist-download.py | 174 +++++++++++++----- 1 file changed, 125 insertions(+), 49 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index ce72e7125b..2f34a8527d 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -15,7 +15,7 @@ if deps_path not in sys_path: sys_path.append(deps_path) -from requests import get +from requests import get, head from requests.exceptions import ConnectionError from common_utils import bytes_hash # type: ignore @@ -53,6 +53,33 @@ def check_line(kind: str, line: bytes) -> Tuple[bool, bytes]: return False, b"" +def parse_http_metadata(cached_data: bytes) -> dict: + """Extract ETag, Last-Modified, and Content-Length stored in cache file comment headers.""" + meta = {"etag": "", "last_modified": "", "content_length": ""} + for line in cached_data.split(b"\n"): + if not line.startswith(b"# "): + break + if line.startswith(b"# ETag: "): + meta["etag"] = line[8:].decode("utf-8", errors="replace").strip() + elif line.startswith(b"# Last-Modified: "): + meta["last_modified"] = line[17:].decode("utf-8", errors="replace").strip() + elif line.startswith(b"# Content-Length: "): + meta["content_length"] = line[18:].decode("utf-8", errors="replace").strip() + return meta + + +def build_cache_header(url: str, etag: str = "", last_modified: str = "", content_length: str = "") -> bytes: + """Build cache file header bytes with URL and optional HTTP metadata.""" + header = b"# Downloaded from " + url.encode("utf-8") + b"\n" + if etag: + header += b"# ETag: " + etag.encode("utf-8") + b"\n" + if last_modified: + header += b"# Last-Modified: " + last_modified.encode("utf-8") + b"\n" + if content_length: + header += b"# Content-Length: " + content_length.encode("utf-8") + b"\n" + return header + + LOGGER = getLogger("BLACKLIST") status = 0 @@ -226,13 +253,19 @@ def check_line(kind: str, line: bytes) -> Tuple[bool, bytes]: # Process cached data and add to unique_entries cached_data = cached_url.get("data", b"") if cached_data: - # Skip first line (URL comment) and process entries - for line in cached_data.split(b"\n")[1:]: + for line in cached_data.split(b"\n"): line = line.strip() - if line: - unique_entries.add(line) + if not line or line.startswith(b"#"): + continue + unique_entries.add(line) else: failed = False + handle_304 = False + old_cached_data = cached_url.get("data", b"") if isinstance(cached_url, dict) else b"" + etag = "" + last_modified = "" + content_length = "" + iterable = [] LOGGER.info(f"Downloading blacklist data from {url} ...") if url.startswith("file://"): try: @@ -247,52 +280,95 @@ def check_line(kind: str, line: bytes) -> Tuple[bool, bytes]: aggregated_recap[kind]["failed_count"] += 1 failed = True else: - max_retries = 3 - retry_count = 0 - while retry_count < max_retries: - try: - resp = get(url, stream=True, timeout=10) - break - except ConnectionError as e: - retry_count += 1 - if retry_count == max_retries: - raise e - LOGGER.warning(f"Connection refused, retrying in 3 seconds... ({retry_count}/{max_retries})") - sleep(3) - - if resp.status_code != 200: - status = 2 - LOGGER.warning(f"Got status code {resp.status_code}, skipping...") - failed_urls.add(url) - if url_file not in urls: - aggregated_recap[kind]["failed_count"] += 1 - failed = True - else: - iterable = resp.iter_lines() + # Build conditional request headers from stored metadata + req_headers = {} + if old_cached_data: + meta = parse_http_metadata(old_cached_data) + if meta["etag"]: + req_headers["If-None-Match"] = meta["etag"] + elif meta["last_modified"]: + req_headers["If-Modified-Since"] = meta["last_modified"] + elif meta["content_length"]: + # No ETag or Last-Modified: use a HEAD request to check size + try: + head_resp = head(url, timeout=10) + head_resp.close() + if head_resp.status_code == 200 and head_resp.headers.get("Content-Length", "") == meta["content_length"]: + LOGGER.debug(f"URL {url} Content-Length unchanged ({meta['content_length']} bytes), skipping download.") + handle_304 = True + except Exception as head_exc: + LOGGER.debug(f"HEAD request for {url} failed, falling back to full GET: {head_exc}") + + if not handle_304: + max_retries = 3 + retry_count = 0 + resp = None + while retry_count < max_retries: + try: + resp = get(url, stream=True, timeout=10, headers=req_headers or None) + break + except ConnectionError as e: + retry_count += 1 + if retry_count == max_retries: + raise e + LOGGER.warning(f"Connection refused, retrying in 3 seconds... ({retry_count}/{max_retries})") + sleep(3) + + if resp is None: + failed = True + elif resp.status_code == 304: + handle_304 = True + elif resp.status_code != 200: + status = 2 + LOGGER.warning(f"Got status code {resp.status_code}, skipping...") + failed_urls.add(url) + if url_file not in urls: + aggregated_recap[kind]["failed_count"] += 1 + failed = True + else: + etag = resp.headers.get("ETag", "") + last_modified = resp.headers.get("Last-Modified", "") + content_length = resp.headers.get("Content-Length", "") + iterable = resp.iter_lines() if not failed: - if url not in processed_urls: - aggregated_recap[kind]["downloaded_urls"] += 1 - - url_content = b"" - count_lines = 0 - for line in iterable: - line = line.strip() - if not line or line.startswith((b"#", b";")): - continue - elif kind != "USER_AGENT": - line = line.split(b" ")[0] - ok, data = check_line(kind, line) - if ok: - unique_entries.add(data) - url_content += data + b"\n" - count_lines += 1 - if url not in processed_urls: - aggregated_recap[kind]["total_lines"] += count_lines - - cached, err = JOB.cache_file(url_file, b"# Downloaded from " + url.encode("utf-8") + b"\n" + url_content) - if not cached: - LOGGER.error(f"Error while caching url content for {url}: {err}") + if handle_304: + LOGGER.debug(f"URL {url} returned 304 Not Modified, using cached data.") + if url not in processed_urls: + aggregated_recap[kind]["skipped_urls"] += 1 + if old_cached_data: + # Re-save to bump last_update timestamp so the 1-hour check stays fresh + cached, err = JOB.cache_file(url_file, old_cached_data) + if not cached: + LOGGER.warning(f"Error while refreshing cache for {url}: {err}") + for line in old_cached_data.split(b"\n"): + line = line.strip() + if not line or line.startswith(b"#"): + continue + unique_entries.add(line) + else: + if url not in processed_urls: + aggregated_recap[kind]["downloaded_urls"] += 1 + + url_content = b"" + count_lines = 0 + for line in iterable: + line = line.strip() + if not line or line.startswith((b"#", b";")): + continue + elif kind != "USER_AGENT": + line = line.split(b" ")[0] + ok, data = check_line(kind, line) + if ok: + unique_entries.add(data) + url_content += data + b"\n" + count_lines += 1 + if url not in processed_urls: + aggregated_recap[kind]["total_lines"] += count_lines + + cached, err = JOB.cache_file(url_file, build_cache_header(url, etag, last_modified, content_length) + url_content) + if not cached: + LOGGER.error(f"Error while caching url content for {url}: {err}") except BaseException as e: status = 2 LOGGER.debug(format_exc()) From 463e94499fe8ee20658a856d95f39153bfdce470 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 6 Mar 2026 10:37:33 +0100 Subject: [PATCH 02/13] change error messages to type error so the error icon will be visible --- src/common/core/blacklist/jobs/blacklist-download.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 2f34a8527d..3b337d1f34 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -194,7 +194,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten deleted, err = JOB.del_cache(file, service_id=file.parent.name) if not deleted: - LOGGER.warning(f"Couldn't delete file {file} from cache : {err}") + LOGGER.error(f"Couldn't delete file {file} from cache : {err}") sys_exit(0) urls = set() @@ -221,7 +221,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten LOGGER.warning(f"{service} blacklist for {kind} is cached but no URL is configured, removing from cache...") deleted, err = JOB.del_cache(f"{kind}.list", service_id=service) if not deleted: - LOGGER.warning(f"Couldn't delete {service} {kind}.list from cache : {err}") + LOGGER.error(f"Couldn't delete {service} {kind}.list from cache : {err}") continue # Track that this service provided URLs for the current kind @@ -320,7 +320,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten handle_304 = True elif resp.status_code != 200: status = 2 - LOGGER.warning(f"Got status code {resp.status_code}, skipping...") + LOGGER.error(f"Got status code {resp.status_code}, skipping...") failed_urls.add(url) if url_file not in urls: aggregated_recap[kind]["failed_count"] += 1 @@ -340,7 +340,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten # Re-save to bump last_update timestamp so the 1-hour check stays fresh cached, err = JOB.cache_file(url_file, old_cached_data) if not cached: - LOGGER.warning(f"Error while refreshing cache for {url}: {err}") + LOGGER.error(f"Error while refreshing cache for {url}: {err}") for line in old_cached_data.split(b"\n"): line = line.strip() if not line or line.startswith(b"#"): @@ -428,7 +428,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten LOGGER.warning(f"Removing no longer used url file {url_file} ...") deleted, err = JOB.del_cache(url_file) if not deleted: - LOGGER.warning(f"Couldn't delete url file {url_file} from cache : {err}") + LOGGER.error(f"Couldn't delete url file {url_file} from cache : {err}") except SystemExit as e: status = e.code except BaseException as e: From c7c1718e112cafe2905e28fabdb7f3902ea941cf Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 6 Mar 2026 16:11:04 +0100 Subject: [PATCH 03/13] Copilot fixes --- .../core/blacklist/jobs/blacklist-download.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 3b337d1f34..dbbeaa57b2 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -15,7 +15,7 @@ if deps_path not in sys_path: sys_path.append(deps_path) -from requests import get, head +from requests import get from requests.exceptions import ConnectionError from common_utils import bytes_hash # type: ignore @@ -288,16 +288,8 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten req_headers["If-None-Match"] = meta["etag"] elif meta["last_modified"]: req_headers["If-Modified-Since"] = meta["last_modified"] - elif meta["content_length"]: - # No ETag or Last-Modified: use a HEAD request to check size - try: - head_resp = head(url, timeout=10) - head_resp.close() - if head_resp.status_code == 200 and head_resp.headers.get("Content-Length", "") == meta["content_length"]: - LOGGER.debug(f"URL {url} Content-Length unchanged ({meta['content_length']} bytes), skipping download.") - handle_304 = True - except Exception as head_exc: - LOGGER.debug(f"HEAD request for {url} failed, falling back to full GET: {head_exc}") + # No ETag or Last-Modified available: Content-Length alone is not + # a reliable cache validator, so always perform a full GET. if not handle_304: max_retries = 3 @@ -317,6 +309,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten if resp is None: failed = True elif resp.status_code == 304: + resp.close() handle_304 = True elif resp.status_code != 200: status = 2 From a23c1301a6dbdaded5aea414a9e9aeea38643a37 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:09:51 +0200 Subject: [PATCH 04/13] =?UTF-8?q?after=20HEAD=20removal,=20nothing=20does?= =?UTF-8?q?=20meta["content=5Flength"]=20for=20decisions=20=E2=80=94=20onl?= =?UTF-8?q?y=20etag=20/=20last=5Fmodified=20feed=20If-None-Match=20/=20If-?= =?UTF-8?q?Modified-Since.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove leftover "Content-Length" --- .../core/blacklist/jobs/blacklist-download.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index dbbeaa57b2..1087b22754 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -54,8 +54,8 @@ def check_line(kind: str, line: bytes) -> Tuple[bool, bytes]: def parse_http_metadata(cached_data: bytes) -> dict: - """Extract ETag, Last-Modified, and Content-Length stored in cache file comment headers.""" - meta = {"etag": "", "last_modified": "", "content_length": ""} + """Extract ETag and Last-Modified stored in cache file comment headers.""" + meta = {"etag": "", "last_modified": ""} for line in cached_data.split(b"\n"): if not line.startswith(b"# "): break @@ -63,20 +63,16 @@ def parse_http_metadata(cached_data: bytes) -> dict: meta["etag"] = line[8:].decode("utf-8", errors="replace").strip() elif line.startswith(b"# Last-Modified: "): meta["last_modified"] = line[17:].decode("utf-8", errors="replace").strip() - elif line.startswith(b"# Content-Length: "): - meta["content_length"] = line[18:].decode("utf-8", errors="replace").strip() return meta -def build_cache_header(url: str, etag: str = "", last_modified: str = "", content_length: str = "") -> bytes: +def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> bytes: """Build cache file header bytes with URL and optional HTTP metadata.""" header = b"# Downloaded from " + url.encode("utf-8") + b"\n" if etag: header += b"# ETag: " + etag.encode("utf-8") + b"\n" if last_modified: header += b"# Last-Modified: " + last_modified.encode("utf-8") + b"\n" - if content_length: - header += b"# Content-Length: " + content_length.encode("utf-8") + b"\n" return header @@ -264,7 +260,6 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten old_cached_data = cached_url.get("data", b"") if isinstance(cached_url, dict) else b"" etag = "" last_modified = "" - content_length = "" iterable = [] LOGGER.info(f"Downloading blacklist data from {url} ...") if url.startswith("file://"): @@ -288,8 +283,6 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten req_headers["If-None-Match"] = meta["etag"] elif meta["last_modified"]: req_headers["If-Modified-Since"] = meta["last_modified"] - # No ETag or Last-Modified available: Content-Length alone is not - # a reliable cache validator, so always perform a full GET. if not handle_304: max_retries = 3 @@ -321,7 +314,6 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten else: etag = resp.headers.get("ETag", "") last_modified = resp.headers.get("Last-Modified", "") - content_length = resp.headers.get("Content-Length", "") iterable = resp.iter_lines() if not failed: @@ -359,7 +351,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "", conten if url not in processed_urls: aggregated_recap[kind]["total_lines"] += count_lines - cached, err = JOB.cache_file(url_file, build_cache_header(url, etag, last_modified, content_length) + url_content) + cached, err = JOB.cache_file(url_file, build_cache_header(url, etag, last_modified) + url_content) if not cached: LOGGER.error(f"Error while caching url content for {url}: {err}") except BaseException as e: From 451f117faac50ae36f70deda5f8e5c00fbb063aa Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:11:31 +0200 Subject: [PATCH 05/13] =?UTF-8?q?after=20HEAD=20removal,=20nothing=20does?= =?UTF-8?q?=20meta["content=5Flength"]=20for=20decisions=20=E2=80=94=20onl?= =?UTF-8?q?y=20etag=20/=20last=5Fmodified=20feed=20If-None-Match=20/=20If-?= =?UTF-8?q?Modified-Since.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove leftover "Content-Length" --- .../core/blacklist/jobs/blacklist-download.py | 61 +++++++++---------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 1087b22754..7708d00657 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -284,37 +284,36 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt elif meta["last_modified"]: req_headers["If-Modified-Since"] = meta["last_modified"] - if not handle_304: - max_retries = 3 - retry_count = 0 - resp = None - while retry_count < max_retries: - try: - resp = get(url, stream=True, timeout=10, headers=req_headers or None) - break - except ConnectionError as e: - retry_count += 1 - if retry_count == max_retries: - raise e - LOGGER.warning(f"Connection refused, retrying in 3 seconds... ({retry_count}/{max_retries})") - sleep(3) - - if resp is None: - failed = True - elif resp.status_code == 304: - resp.close() - handle_304 = True - elif resp.status_code != 200: - status = 2 - LOGGER.error(f"Got status code {resp.status_code}, skipping...") - failed_urls.add(url) - if url_file not in urls: - aggregated_recap[kind]["failed_count"] += 1 - failed = True - else: - etag = resp.headers.get("ETag", "") - last_modified = resp.headers.get("Last-Modified", "") - iterable = resp.iter_lines() + max_retries = 3 + retry_count = 0 + resp = None + while retry_count < max_retries: + try: + resp = get(url, stream=True, timeout=10, headers=req_headers or None) + break + except ConnectionError as e: + retry_count += 1 + if retry_count == max_retries: + raise e + LOGGER.warning(f"Connection refused, retrying in 3 seconds... ({retry_count}/{max_retries})") + sleep(3) + + if resp is None: + failed = True + elif resp.status_code == 304: + resp.close() + handle_304 = True + elif resp.status_code != 200: + status = 2 + LOGGER.error(f"Got status code {resp.status_code}, skipping...") + failed_urls.add(url) + if url_file not in urls: + aggregated_recap[kind]["failed_count"] += 1 + failed = True + else: + etag = resp.headers.get("ETag", "") + last_modified = resp.headers.get("Last-Modified", "") + iterable = resp.iter_lines() if not failed: if handle_304: From 1e4061c8662d46f237ce02a85399f12650dab0e3 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:14:44 +0200 Subject: [PATCH 06/13] RFC 7232 prefers both: ETag XOR Last-Modified (elif) --- src/common/core/blacklist/jobs/blacklist-download.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 7708d00657..23bd0c7ee2 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -281,7 +281,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt meta = parse_http_metadata(old_cached_data) if meta["etag"]: req_headers["If-None-Match"] = meta["etag"] - elif meta["last_modified"]: + if meta["last_modified"]: req_headers["If-Modified-Since"] = meta["last_modified"] max_retries = 3 From 8403813984c8b506f880c10fecd93452bf4a5027 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:16:23 +0200 Subject: [PATCH 07/13] Non-200 responses never resp.close() with stream=True With stream=True, requests opens the HTTP connection and does not download the body until you read it. The connection stays checked out until the body is consumed or you call resp.close(). --- src/common/core/blacklist/jobs/blacklist-download.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 23bd0c7ee2..a8c0ada7f7 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -306,6 +306,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt elif resp.status_code != 200: status = 2 LOGGER.error(f"Got status code {resp.status_code}, skipping...") + resp.close() failed_urls.add(url) if url_file not in urls: aggregated_recap[kind]["failed_count"] += 1 From b7c15c5c1f150673aab40fc17f15c122a0b93b5c Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:22:45 +0200 Subject: [PATCH 08/13] Close the 200 response if parsing fails. 304 and other non-200 statuses call resp.close(). A 200 assigns resp.iter_lines() and reads it later. If check_line or the loop raises, that exception is caught above and the streamed connection is never closed. Wrap the read in try/finally and close resp there. --- .../core/blacklist/jobs/blacklist-download.py | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index a8c0ada7f7..7d0625dfeb 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -261,6 +261,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt etag = "" last_modified = "" iterable = [] + resp = None LOGGER.info(f"Downloading blacklist data from {url} ...") if url.startswith("file://"): try: @@ -286,7 +287,6 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt max_retries = 3 retry_count = 0 - resp = None while retry_count < max_retries: try: resp = get(url, stream=True, timeout=10, headers=req_headers or None) @@ -337,17 +337,21 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt url_content = b"" count_lines = 0 - for line in iterable: - line = line.strip() - if not line or line.startswith((b"#", b";")): - continue - elif kind != "USER_AGENT": - line = line.split(b" ")[0] - ok, data = check_line(kind, line) - if ok: - unique_entries.add(data) - url_content += data + b"\n" - count_lines += 1 + try: + for line in iterable: + line = line.strip() + if not line or line.startswith((b"#", b";")): + continue + elif kind != "USER_AGENT": + line = line.split(b" ")[0] + ok, data = check_line(kind, line) + if ok: + unique_entries.add(data) + url_content += data + b"\n" + count_lines += 1 + finally: + if resp is not None: + resp.close() if url not in processed_urls: aggregated_recap[kind]["total_lines"] += count_lines From bea1ee868cd90a5715cffe15b0f3f1320d7542f8 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:24:33 +0200 Subject: [PATCH 09/13] Reject validators that contain newlines. The cache file treats everything before the first non-comment line as HTTP metadata. build_cache_header writes each validator as one comment line, and parse_http_metadata stops at the first line that does not start with # . --- .../core/blacklist/jobs/blacklist-download.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 7d0625dfeb..4b96fb562a 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -53,6 +53,14 @@ def check_line(kind: str, line: bytes) -> Tuple[bool, bytes]: return False, b"" +def sanitize_validator(value: str) -> str: + """Keep a validator only when it is a single line.""" + value = value.strip() + if "\n" in value or "\r" in value: + return "" + return value + + def parse_http_metadata(cached_data: bytes) -> dict: """Extract ETag and Last-Modified stored in cache file comment headers.""" meta = {"etag": "", "last_modified": ""} @@ -60,15 +68,17 @@ def parse_http_metadata(cached_data: bytes) -> dict: if not line.startswith(b"# "): break if line.startswith(b"# ETag: "): - meta["etag"] = line[8:].decode("utf-8", errors="replace").strip() + meta["etag"] = sanitize_validator(line[8:].decode("utf-8", errors="replace")) elif line.startswith(b"# Last-Modified: "): - meta["last_modified"] = line[17:].decode("utf-8", errors="replace").strip() + meta["last_modified"] = sanitize_validator(line[17:].decode("utf-8", errors="replace")) return meta def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> bytes: """Build cache file header bytes with URL and optional HTTP metadata.""" header = b"# Downloaded from " + url.encode("utf-8") + b"\n" + etag = sanitize_validator(etag) + last_modified = sanitize_validator(last_modified) if etag: header += b"# ETag: " + etag.encode("utf-8") + b"\n" if last_modified: From 921b4df475628d305e454c1497a698f799ec201a Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Sun, 27 Sep 2026 16:26:00 +0200 Subject: [PATCH 10/13] Log 304 at info, and count it separately. A 304 and a one-hour cache hit are recorded as the same event, and the 304 itself is logged only at debug level. At the default log level the conditional GET is invisible. --- src/common/core/blacklist/jobs/blacklist-download.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 4b96fb562a..88edc5fa5a 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -213,6 +213,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt "total_urls": 0, "downloaded_urls": 0, "skipped_urls": 0, + "not_modified": 0, "failed_count": 0, "total_lines": 0, } @@ -328,9 +329,9 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt if not failed: if handle_304: - LOGGER.debug(f"URL {url} returned 304 Not Modified, using cached data.") + LOGGER.info(f"URL {url} returned 304 Not Modified, using cached data.") if url not in processed_urls: - aggregated_recap[kind]["skipped_urls"] += 1 + aggregated_recap[kind]["not_modified"] += 1 if old_cached_data: # Re-save to bump last_update timestamp so the 1-hour check stays fresh cached, err = JOB.cache_file(url_file, old_cached_data) @@ -413,11 +414,12 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt continue successful = recap["downloaded_urls"] skipped = recap["skipped_urls"] + not_modified = recap["not_modified"] failed = recap["failed_count"] total_lines = recap["total_lines"] LOGGER.info( f"Recap for {kind} urls: Total Services: {service_count}, Successful: {successful}, " - f"Skipped (cached): {skipped}, Failed: {failed}, Total Lines: {total_lines}" + f"Skipped (cached): {skipped}, Not modified: {not_modified}, Failed: {failed}, Total Lines: {total_lines}" ) # Remove old files From 99bec92d5f70a4dfce0241f567e3ff89082513fe Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 30 Oct 2026 20:37:48 +0100 Subject: [PATCH 11/13] fix open code reviews - Include the blacklist kind in the cache key - if resp is None: can never run (line 313) - No tests for the helpers or the 304 path - fix linter errors --- .../core/blacklist/jobs/blacklist-download.py | 14 +- .../test_blacklist_conditional_download.py | 205 ++++++++++++++++++ 2 files changed, 213 insertions(+), 6 deletions(-) create mode 100644 tests/unit/jobs/test_blacklist_conditional_download.py diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 66f98f9de3..6b9adee698 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -239,7 +239,10 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt unique_entries = set() content = b"" for url in urls_list: - url_file = f"{bytes_hash(url, algorithm='sha1')}.list" + # Key by kind too: cached entries are already transformed by check_line for that kind + # SHA-256 instead of SHA-1; TODO: switch the other list downloaders (greylist, whitelist, + # realip, dnsbl, robotstxt) to the same SHA-256 url_file key, they still use SHA-1 + url_file = f"{bytes_hash(f'{kind}:{url}', algorithm='sha256')}.list" cached_url = JOB.get_cache(url_file, with_info=True, with_data=True) try: # Only count URLs that haven't been processed globally @@ -272,10 +275,10 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt old_cached_data = cached_url.get("data", b"") if isinstance(cached_url, dict) else b"" etag = "" last_modified = "" - resp = None LOGGER.info(f"Downloading blacklist data from {url} ...") download_url, url_filter = split_list_url(url) content_type = "" + body = b"" if download_url.startswith("file://"): try: body = Path(normpath(download_url[7:])).read_bytes() @@ -299,7 +302,8 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt max_retries = 3 retry_count = 0 - while retry_count < max_retries: + # Leaves only by break (resp assigned) or by re-raising on the last attempt + while True: try: resp = get(download_url, stream=True, timeout=10, headers=req_headers or None) break @@ -310,9 +314,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt LOGGER.warning(f"Connection refused, retrying in 3 seconds... ({retry_count}/{max_retries})") sleep(3) - if resp is None: - failed = True - elif resp.status_code == 304 and old_cached_data: + if resp.status_code == 304 and old_cached_data: resp.close() handle_304 = True elif resp.status_code != 200: diff --git a/tests/unit/jobs/test_blacklist_conditional_download.py b/tests/unit/jobs/test_blacklist_conditional_download.py new file mode 100644 index 0000000000..346d40164a --- /dev/null +++ b/tests/unit/jobs/test_blacklist_conditional_download.py @@ -0,0 +1,205 @@ +"""Conditional GET (ETag / Last-Modified) in the blacklist download job, against a local server and a cache that can age.""" + +import logging +import os +import runpy +import sys +from hashlib import sha1, sha256 +from pathlib import Path +from time import time +from types import SimpleNamespace + +import pytest +import requests + +from common_utils import bytes_hash + +ROOT = Path(os.environ.get("BW_BLACKLIST_JOB_ROOT", Path(__file__).resolve().parents[3])) +JOB_SCRIPT = ROOT / "src/common/core/blacklist/jobs/blacklist-download.py" +ETAG = '"v1"' +LAST_MODIFIED = "Wed, 21 Oct 2026 07:28:00 GMT" +ENTRIES = b"192.0.2.1\n198.51.100.0/24\n" + + +class Cache: + """Job cache stand-in that keeps last_update per file, so a test can push it past the 1-hour skip.""" + + def __init__(self, path): + self.job_path = path + self.data = {} + self.updated = {} + + def get_cache(self, name, service_id="", **kwargs): + data = self.data.get((service_id, name)) + return {"last_update": self.updated[service_id, name], "data": data} if data is not None else None + + def cache_file(self, name, data, service_id="", **kwargs): + self.data[service_id, name] = data + self.updated[service_id, name] = time() + path = self.job_path / service_id / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(data) + return True, "" + + def cache_hash(self, name, service_id=""): + data = self.data.get((service_id, name)) + return bytes_hash(data) if data is not None else None + + def del_cache(self, name, service_id=""): + path = name if isinstance(name, Path) else self.job_path / service_id / name + self.data.pop((service_id, path.name), None) + self.updated.pop((service_id, path.name), None) + path.unlink(missing_ok=True) + return True, "" + + def url_files(self): + return {name: data for (service_id, name), data in self.data.items() if not service_id} + + def age(self): + for key in self.updated: + self.updated[key] -= 7200 + + +def run_job(cache, monkeypatch, **urls): + for key in list(os.environ): + if key.startswith("BLACKLIST_"): + monkeypatch.delenv(key) + for key, value in {"SERVER_NAME": "lists.example", "MULTISITE": "no", "USE_BLACKLIST": "yes", "BLACKLIST_COMMUNITY_LISTS": "", **urls}.items(): + monkeypatch.setenv(key, value) + monkeypatch.setitem(sys.modules, "jobs", SimpleNamespace(Job=lambda *args: cache)) + monkeypatch.setitem(sys.modules, "logger", SimpleNamespace(getLogger=logging.getLogger)) + with pytest.raises(SystemExit) as result: + runpy.run_path(str(JOB_SCRIPT)) + return result.value.code + + +def conditional_route(seen, etag=ETAG, body=ENTRIES): + """Answer 304 when the client replays ``etag``, else 200 with validators; record what was sent.""" + + def route(headers): + seen.append((headers.get("If-None-Match"), headers.get("If-Modified-Since"))) + if headers.get("If-None-Match") == etag: + return 304, b"", {"ETag": etag} + return 200, body, {"ETag": etag, "Last-Modified": LAST_MODIFIED} + + return route + + +def test_validators_stored_then_replayed_and_304_reuses_entries(url_server, tmp_path, monkeypatch): + seen = [] + url_server.routes["/ips"] = conditional_route(seen) + url = url_server.http("/ips") + cache = Cache(tmp_path) + + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + assert seen == [(None, None)] + assert list(cache.url_files().values()) == [f"# Downloaded from {url}\n# ETag: {ETAG}\n# Last-Modified: {LAST_MODIFIED}\n".encode() + ENTRIES] + + # Within the hour: no request, and the header lines are not read back as entries + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 + assert len(seen) == 1 + assert cache.data["lists.example", "IP.list"] == ENTRIES + + # Past the hour: both validators replayed, 304 keeps the entries and restarts the hour + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 + assert seen[-1] == (ETAG, LAST_MODIFIED) + assert cache.data["lists.example", "IP.list"] == ENTRIES + assert all(time() - updated < 60 for (service_id, _), updated in cache.updated.items() if not service_id) + + +def test_changed_validator_downloads_new_entries(url_server, tmp_path, monkeypatch): + seen = [] + url_server.routes["/ips"] = conditional_route(seen) + url = url_server.http("/ips") + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + + url_server.routes["/ips"] = conditional_route(seen, etag='"v2"', body=b"203.0.113.7\n") + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + assert seen[-1] == (ETAG, LAST_MODIFIED) + assert cache.data["lists.example", "IP.list"] == b"203.0.113.7\n" + assert b'# ETag: "v2"\n' in next(iter(cache.url_files().values())) + + +def test_unsolicited_304_without_cache_is_a_failure(url_server, tmp_path, monkeypatch): + url_server.routes["/ips"] = (304, b"", {}) + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url_server.http("/ips")) == 2 + assert ("lists.example", "IP.list") not in cache.data + assert not cache.url_files() + + +def test_connection_refused_retries_three_times_then_fails(tmp_path, monkeypatch): + attempts, waits = [], [] + + def refuse(*args, **kwargs): + attempts.append(args[0]) + raise requests.exceptions.ConnectionError("refused") + + # The job binds both names at import time, so patch them before runpy executes it + monkeypatch.setattr("requests.get", refuse) + monkeypatch.setattr("time.sleep", waits.append) + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS="http://127.0.0.1:9/ips") == 2 + assert len(attempts) == 3 + assert waits == [3, 3] + assert not cache.url_files() + + +@pytest.mark.parametrize( + "header", + ( + pytest.param(b"", id="legacy-cache-without-validators"), + pytest.param(b'# ETag: "a\rb"\n', id="validator-with-carriage-return"), + ), +) +def test_cache_without_usable_validator_gets_plain_download(header, url_server, tmp_path, monkeypatch): + seen = [] + url_server.routes["/ips"] = conditional_route(seen) + url = url_server.http("/ips") + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + + [url_file] = cache.url_files() + cache.cache_file(url_file, f"# Downloaded from {url}\n".encode() + header + ENTRIES) + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 + assert seen[-1] == (None, None) + assert f"# ETag: {ETAG}\n".encode() in cache.url_files()[url_file] + + +def test_url_cache_file_named_by_sha256_of_kind_and_url(url_server, tmp_path, monkeypatch): + url_server.routes["/ips"] = (200, ENTRIES, {}) + url = url_server.http("/ips") + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + assert list(cache.url_files()) == [f"{sha256(f'IP:{url}'.encode()).hexdigest()}.list"] + + +def test_legacy_sha1_url_file_is_removed(url_server, tmp_path, monkeypatch): + url_server.routes["/ips"] = (200, ENTRIES, {}) + url = url_server.http("/ips") + cache = Cache(tmp_path) + legacy = f"{sha1(url.encode()).hexdigest()}.list" + cache.cache_file(legacy, f"# Downloaded from {url}\n".encode() + ENTRIES) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 + assert legacy not in cache.url_files() + assert not (tmp_path / legacy).exists() + + +def test_same_url_under_two_kinds_keeps_entries_apart(url_server, tmp_path, monkeypatch): + seen = [] + url_server.routes["/mixed"] = conditional_route(seen, body=b"/admin\n") + url = url_server.http("/mixed") + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_USER_AGENT_URLS=url, BLACKLIST_URI_URLS=url) == 1 + assert cache.data["lists.example", "USER_AGENT.list"] == b"(?:\\b)/admin(?:\\b)\n" + assert cache.data["lists.example", "URI.list"] == b"/admin\n" + + # The 304 path must keep them apart too + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_USER_AGENT_URLS=url, BLACKLIST_URI_URLS=url) == 0 + assert seen[-2:] == [(ETAG, LAST_MODIFIED)] * 2 + assert cache.data["lists.example", "URI.list"] == b"/admin\n" From c21aeded9271726ac78ac438356b1b8642d62ee5 Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 30 Oct 2026 20:57:43 +0100 Subject: [PATCH 12/13] fix(blacklist): keep a failed URL's last cached entries, extend conditional GET tests fix(blacklist): keep a failed URL's last cached entries, extend conditional GET tests --- .../core/blacklist/jobs/blacklist-download.py | 27 +++++++----- .../test_blacklist_conditional_download.py | 44 ++++++++++++++++--- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index 6b9adee698..c6f38d7136 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -75,6 +75,14 @@ def parse_http_metadata(cached_data: bytes) -> dict: return meta +def iter_cached_entries(cached_data: bytes): + """Yield the entries of a per-URL cache file, skipping its comment header lines.""" + for line in cached_data.split(b"\n"): + line = line.strip() + if line and not line.startswith(b"#"): + yield line + + def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> bytes: """Build cache file header bytes with URL and optional HTTP metadata.""" header = b"# Downloaded from " + url.encode("utf-8") + b"\n" @@ -262,13 +270,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt if url not in processed_urls: aggregated_recap[kind]["skipped_urls"] += 1 # Process cached data and add to unique_entries - cached_data = cached_url.get("data", b"") - if cached_data: - for line in cached_data.split(b"\n"): - line = line.strip() - if not line or line.startswith(b"#"): - continue - unique_entries.add(line) + unique_entries.update(iter_cached_entries(cached_url.get("data") or b"")) else: failed = False handle_304 = False @@ -344,11 +346,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt cached, err = JOB.cache_file(url_file, old_cached_data) if not cached: LOGGER.error(f"Error while refreshing cache for {url}: {err}") - for line in old_cached_data.split(b"\n"): - line = line.strip() - if not line or line.startswith(b"#"): - continue - unique_entries.add(line) + unique_entries.update(iter_cached_entries(old_cached_data)) else: if url not in processed_urls: aggregated_recap[kind]["downloaded_urls"] += 1 @@ -388,6 +386,11 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt processed_urls.add(url) urls.add(url_file) + # Keep a failed URL's last downloaded entries, so the list is not rebuilt without them + if url in failed_urls and isinstance(cached_url, dict) and cached_url.get("data"): + LOGGER.warning(f"Using the last cached {kind} entries from {url} until it can be downloaded again") + unique_entries.update(iter_cached_entries(cached_url["data"])) + # Build final content from unique entries, sorted for consistency content = b"\n".join(sorted(unique_entries)) + b"\n" if unique_entries else b"" diff --git a/tests/unit/jobs/test_blacklist_conditional_download.py b/tests/unit/jobs/test_blacklist_conditional_download.py index 346d40164a..3f1557636c 100644 --- a/tests/unit/jobs/test_blacklist_conditional_download.py +++ b/tests/unit/jobs/test_blacklist_conditional_download.py @@ -85,27 +85,61 @@ def route(headers): return route -def test_validators_stored_then_replayed_and_304_reuses_entries(url_server, tmp_path, monkeypatch): +@pytest.mark.parametrize("etag", (pytest.param(ETAG, id="strong-etag"), pytest.param('W/"v1"', id="weak-etag"))) +def test_validators_stored_then_replayed_and_304_reuses_entries(etag, url_server, tmp_path, monkeypatch, caplog): + caplog.set_level(logging.INFO) seen = [] - url_server.routes["/ips"] = conditional_route(seen) + url_server.routes["/ips"] = conditional_route(seen, etag=etag) url = url_server.http("/ips") cache = Cache(tmp_path) assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 1 assert seen == [(None, None)] - assert list(cache.url_files().values()) == [f"# Downloaded from {url}\n# ETag: {ETAG}\n# Last-Modified: {LAST_MODIFIED}\n".encode() + ENTRIES] + assert list(cache.url_files().values()) == [f"# Downloaded from {url}\n# ETag: {etag}\n# Last-Modified: {LAST_MODIFIED}\n".encode() + ENTRIES] # Within the hour: no request, and the header lines are not read back as entries assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 assert len(seen) == 1 assert cache.data["lists.example", "IP.list"] == ENTRIES - # Past the hour: both validators replayed, 304 keeps the entries and restarts the hour + # Past the hour: both validators replayed as stored, 304 keeps the entries and restarts the hour cache.age() + caplog.clear() assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 - assert seen[-1] == (ETAG, LAST_MODIFIED) + assert seen[-1] == (etag, LAST_MODIFIED) assert cache.data["lists.example", "IP.list"] == ENTRIES assert all(time() - updated < 60 for (service_id, _), updated in cache.updated.items() if not service_id) + assert "Not modified: 1" in caplog.text + + +def test_304_on_header_only_cache_writes_no_list(url_server, tmp_path, monkeypatch): + seen = [] + url_server.routes["/ips"] = conditional_route(seen, body=b"# no entries yet\n") + url = url_server.http("/ips") + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 + assert list(cache.url_files().values()) == [f"# Downloaded from {url}\n# ETag: {ETAG}\n# Last-Modified: {LAST_MODIFIED}\n".encode()] + + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url) == 0 + assert seen[-1] == (ETAG, LAST_MODIFIED) + assert ("lists.example", "IP.list") not in cache.data + + +def test_failed_url_keeps_its_last_cached_entries(url_server, tmp_path, monkeypatch, caplog): + url_server.routes["/a"] = (200, b"192.0.2.1\n", {}) + url_server.routes["/b"] = (200, b"198.51.100.1\n", {}) + urls = f"{url_server.http('/a')} {url_server.http('/b')}" + cache = Cache(tmp_path) + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=urls) == 1 + + # /b goes down and /a changes: /a's update lands, /b's last good entries stay + url_server.routes["/a"] = (200, b"192.0.2.2\n", {}) + url_server.routes["/b"] = (503, b"down", {}) + cache.age() + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=urls) == 2 + assert cache.data["lists.example", "IP.list"] == b"192.0.2.2\n198.51.100.1\n" + assert "Using the last cached IP entries" in caplog.text def test_changed_validator_downloads_new_entries(url_server, tmp_path, monkeypatch): From b2b19b483baefce7f6cd2995dc726ac93cfc9c0a Mon Sep 17 00:00:00 2001 From: sysangels | Michal Koeckeis-Fresel Date: Fri, 30 Oct 2026 21:08:07 +0100 Subject: [PATCH 13/13] failed_urls stores only the URL and is shared across kinds. After one kind fails, the same URL enters the if url in failed_urls branch for every later kind, so those kinds skip their own downloads and can retain stale cached entries. Store (kind, url) instead. --- .../core/blacklist/jobs/blacklist-download.py | 12 ++++++------ .../jobs/test_blacklist_conditional_download.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/common/core/blacklist/jobs/blacklist-download.py b/src/common/core/blacklist/jobs/blacklist-download.py index c6f38d7136..c9d2068560 100644 --- a/src/common/core/blacklist/jobs/blacklist-download.py +++ b/src/common/core/blacklist/jobs/blacklist-download.py @@ -213,7 +213,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt sys_exit(0) urls = set() - failed_urls = set() + failed_urls = set() # (kind, url) pairs: a failure while handling one kind must not skip another kind processed_urls = set() # Track which URLs have been processed globally # Initialize aggregation per kind with service tracking aggregated_recap = { @@ -258,7 +258,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt aggregated_recap[kind]["total_urls"] += 1 # If the URL has recently been downloaded, use cache - if url in failed_urls: + if (kind, url) in failed_urls: if url not in processed_urls: aggregated_recap[kind]["failed_count"] += 1 elif ( @@ -288,7 +288,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt status = 2 LOGGER.debug(format_exc()) LOGGER.error(f"Error while opening file {download_url[7:]} : {e}") - failed_urls.add(url) + failed_urls.add((kind, url)) if url_file not in urls: aggregated_recap[kind]["failed_count"] += 1 failed = True @@ -324,7 +324,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt status = 2 LOGGER.error(f"Got status code {resp.status_code}, skipping...") resp.close() - failed_urls.add(url) + failed_urls.add((kind, url)) if url_file not in urls: aggregated_recap[kind]["failed_count"] += 1 failed = True @@ -378,7 +378,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt status = 2 LOGGER.debug(format_exc()) LOGGER.error(f"Exception while getting {service} blacklist from {url} :\n{e}") - failed_urls.add(url) + failed_urls.add((kind, url)) if url not in processed_urls: aggregated_recap[kind]["failed_count"] += 1 finally: @@ -387,7 +387,7 @@ def build_cache_header(url: str, etag: str = "", last_modified: str = "") -> byt urls.add(url_file) # Keep a failed URL's last downloaded entries, so the list is not rebuilt without them - if url in failed_urls and isinstance(cached_url, dict) and cached_url.get("data"): + if (kind, url) in failed_urls and isinstance(cached_url, dict) and cached_url.get("data"): LOGGER.warning(f"Using the last cached {kind} entries from {url} until it can be downloaded again") unique_entries.update(iter_cached_entries(cached_url["data"])) diff --git a/tests/unit/jobs/test_blacklist_conditional_download.py b/tests/unit/jobs/test_blacklist_conditional_download.py index 3f1557636c..65275beb86 100644 --- a/tests/unit/jobs/test_blacklist_conditional_download.py +++ b/tests/unit/jobs/test_blacklist_conditional_download.py @@ -223,6 +223,23 @@ def test_legacy_sha1_url_file_is_removed(url_server, tmp_path, monkeypatch): assert not (tmp_path / legacy).exists() +def test_failure_for_one_kind_does_not_skip_another_kind(url_server, tmp_path, monkeypatch): + url_server.routes["/mixed"] = (200, b"/admin\n", {}) + url = url_server.http("/mixed") + ip_url_file = f"{sha256(f'IP:{url}'.encode()).hexdigest()}.list" + + class FailingIpCache(Cache): + def cache_file(self, name, data, service_id="", **kwargs): + if name == ip_url_file: + raise OSError("disk full") + return super().cache_file(name, data, service_id, **kwargs) + + cache = FailingIpCache(tmp_path) + # IP runs first (KINDS order): its cache write fails, URI must still download and publish its list + assert run_job(cache, monkeypatch, BLACKLIST_IP_URLS=url, BLACKLIST_URI_URLS=url) == 2 + assert cache.data["lists.example", "URI.list"] == b"/admin\n" + + def test_same_url_under_two_kinds_keeps_entries_apart(url_server, tmp_path, monkeypatch): seen = [] url_server.routes["/mixed"] = conditional_route(seen, body=b"/admin\n")