From 24df1f8624012de57557f44f00b72fb4abd43114 Mon Sep 17 00:00:00 2001 From: smartworldarafath Date: Thu, 24 Sep 2026 02:22:18 +0600 Subject: [PATCH 1/3] fix: cross-platform compatibility, config deep merge, and pipeline robustness - pipelines: use url2pathname for Windows file:// paths in _department_of - pipelines: use atomic replace for image tiles.json and safely close mmap handles - sources/local: standardize to RFC 8089 as_uri() format for file URLs - config: deep merge nested sections in load_config, enforce utf-8, and support backslashes - sources/web: specify utf-8 encoding for urls_file reading - render: forward extract_text for local HTML files in pixelshot CLI - sources/kiwix: cross-platform process termination fallback for Windows in _kill_proc - chunk: fix Windows shard_dir trailing slash parsing, ensure PNG format in fast path, and use atomic write - index: clamp nlist to vector count in build_faiss to prevent crash on small datasets - web: safely handle malformed URI components in agent-server and chat route - tests: unblock department build tests in core test suite and add regression tests --- embed/src/pixelrag_embed/chunk.py | 12 +- embed/src/pixelrag_embed/index.py | 4 + index/src/pixelrag_index/config.py | 10 +- index/src/pixelrag_index/pipelines.py | 17 +-- index/src/pixelrag_index/sources/kiwix.py | 14 ++- index/src/pixelrag_index/sources/local.py | 2 +- index/src/pixelrag_index/sources/web.py | 2 +- render/src/pixelrag_render/render.py | 1 + tests/test_bug_fixes.py | 138 ++++++++++++++++++++++ tests/test_department_filter.py | 40 ++++--- web/agent-server.mjs | 10 +- web/app/api/chat/route.ts | 10 +- 12 files changed, 226 insertions(+), 34 deletions(-) create mode 100644 tests/test_bug_fixes.py diff --git a/embed/src/pixelrag_embed/chunk.py b/embed/src/pixelrag_embed/chunk.py index ab8090bf..09432681 100644 --- a/embed/src/pixelrag_embed/chunk.py +++ b/embed/src/pixelrag_embed/chunk.py @@ -162,8 +162,12 @@ def chunk_article(article_dir: str, dry_run: bool = False, force: bool = False) chunk_name = f"chunk_{tile_idx:04d}_00.png" chunk_path = os.path.join(article_dir, chunk_name) if not dry_run: - shutil.copy2(tile_path, chunk_path) + if tile_path.endswith(".png"): + shutil.copy2(tile_path, chunk_path) + else: + img.save(chunk_path, format="PNG") files_written += 1 + img.close() chunks_info.append( { "tile": tile_name, @@ -241,8 +245,10 @@ def chunk_article(article_dir: str, dry_run: bool = False, force: bool = False) manifest["article_id"] = article_id if not dry_run: - with open(chunks_json, "w") as f: + tmp_chunks = chunks_json + ".tmp" + with open(tmp_chunks, "w") as f: json.dump(manifest, f) + os.replace(tmp_chunks, chunks_json) return { "article_dir": article_dir, @@ -344,7 +350,7 @@ def process_shard( tiles_deleted = _delete_tiles_in_shard(shard_dir) elapsed = time.time() - t0 - shard_name = os.path.basename(shard_dir.rstrip("/")) + shard_name = os.path.basename(shard_dir.rstrip("/\\")) return { "shard": shard_name, "articles": total_articles, diff --git a/embed/src/pixelrag_embed/index.py b/embed/src/pixelrag_embed/index.py index 019767e0..88a141f6 100644 --- a/embed/src/pixelrag_embed/index.py +++ b/embed/src/pixelrag_embed/index.py @@ -189,6 +189,10 @@ def build_ivf( # Build IVF index metric_type = faiss.METRIC_INNER_PRODUCT if metric == "ip" else faiss.METRIC_L2 + # Auto-adjust nlist when dataset is smaller than configured nlist + if n < nlist: + nlist = max(1, n) + # Train on a sample actual_train = min(train_sample, n) train_indices = np.random.choice(n, actual_train, replace=False) diff --git a/index/src/pixelrag_index/config.py b/index/src/pixelrag_index/config.py index b7fdbf9b..08bffc85 100644 --- a/index/src/pixelrag_index/config.py +++ b/index/src/pixelrag_index/config.py @@ -21,11 +21,15 @@ def load_config(path=None): path = str(c) break if path and os.path.exists(path): - with open(path) as f: + with open(path, encoding="utf-8") as f: config = yaml.safe_load(f) or {} else: config = {} - return {**DEFAULT_CONFIG, **config} + merged = {**DEFAULT_CONFIG, **config} + for section in ("ingest", "embed"): + if section in DEFAULT_CONFIG and section in config and isinstance(config[section], dict): + merged[section] = {**DEFAULT_CONFIG[section], **config[section]} + return merged def make_source(config): @@ -33,6 +37,6 @@ def make_source(config): source_type = source_config.pop("type", "local") # Expand ~ in any string values that look like paths for k, v in source_config.items(): - if isinstance(v, str) and ("/" in v or "~" in v): + if isinstance(v, str) and ("/" in v or "\\" in v or "~" in v): source_config[k] = str(Path(v).expanduser()) return SOURCES[source_type](**source_config) diff --git a/index/src/pixelrag_index/pipelines.py b/index/src/pixelrag_index/pipelines.py index 5c3943f5..2fadb251 100644 --- a/index/src/pixelrag_index/pipelines.py +++ b/index/src/pixelrag_index/pipelines.py @@ -63,9 +63,10 @@ def _department_of(article: dict, source_root: str) -> str: if not raw: url = article.get("url") or "" if url.startswith("file://"): - from urllib.parse import unquote, urlparse + from urllib.parse import urlparse + from urllib.request import url2pathname - raw = unquote(urlparse(url).path) + raw = url2pathname(urlparse(url).path) if not raw or not source_root: return "" try: @@ -291,8 +292,9 @@ def _repl(m: re.Match) -> str: "tiles": ["tile_0000.jpg"], "complete": True, } - with open(tile_dir / "tiles.json", "w") as f: - json.dump(manifest, f) + tmp_tiles = tile_dir / "tiles.json.tmp" + tmp_tiles.write_text(json.dumps(manifest)) + os.replace(tmp_tiles, tile_dir / "tiles.json") except Exception as e: logger.warning(" FAILED image %s: %s", doc.id, e) logger.info(" Rendered %d local images", len(image_docs)) @@ -441,9 +443,10 @@ def _repl(m: re.Match) -> str: import numpy as np npz_files = sorted(embeddings_dir.glob("shard_*.npz")) - total_vectors = sum( - np.load(f, mmap_mode="r")["embeddings"].shape[0] for f in npz_files - ) + total_vectors = 0 + for f in npz_files: + with np.load(f, mmap_mode="r") as data: + total_vectors += data["embeddings"].shape[0] nlist = min(4096, max(1, total_vectors // 40)) logger.info( "Stage 4/4: Building FAISS index (%d vectors, nlist=%d)...", diff --git a/index/src/pixelrag_index/sources/kiwix.py b/index/src/pixelrag_index/sources/kiwix.py index b9ed83d2..d8506d32 100644 --- a/index/src/pixelrag_index/sources/kiwix.py +++ b/index/src/pixelrag_index/sources/kiwix.py @@ -233,19 +233,29 @@ def ensure_running(self) -> None: self._start_instance(idx) def _kill_proc(self, proc: subprocess.Popen) -> None: + if proc.poll() is not None: + return try: - os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + if hasattr(os, "killpg") and hasattr(os, "getpgid"): + os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + else: + proc.terminate() except (OSError, ProcessLookupError): pass try: proc.wait(timeout=5) except subprocess.TimeoutExpired: try: - os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + if hasattr(os, "killpg") and hasattr(os, "getpgid") and hasattr(signal, "SIGKILL"): + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) + else: + proc.kill() except (OSError, ProcessLookupError): pass def stop(self) -> None: + if not hasattr(self, "_procs"): + return for idx, proc in enumerate(self._procs): if proc is not None: self._kill_proc(proc) diff --git a/index/src/pixelrag_index/sources/local.py b/index/src/pixelrag_index/sources/local.py index 0b328563..e4074fd2 100644 --- a/index/src/pixelrag_index/sources/local.py +++ b/index/src/pixelrag_index/sources/local.py @@ -35,7 +35,7 @@ def __iter__(self) -> Iterator[Document]: if ftype == "web": yield Document( id=f.stem, - url=f"file://{f.resolve()}", + url=f.resolve().as_uri(), metadata={"type": ftype}, ) elif ftype == "text": diff --git a/index/src/pixelrag_index/sources/web.py b/index/src/pixelrag_index/sources/web.py index fb5fb422..87b9fd16 100644 --- a/index/src/pixelrag_index/sources/web.py +++ b/index/src/pixelrag_index/sources/web.py @@ -67,7 +67,7 @@ def __init__( if urls_file: p = Path(urls_file) if p.exists(): - with open(p) as f: + with open(p, encoding="utf-8") as f: self._urls = [ line.strip() for line in f diff --git a/render/src/pixelrag_render/render.py b/render/src/pixelrag_render/render.py index c4bb7fc9..52eab97f 100644 --- a/render/src/pixelrag_render/render.py +++ b/render/src/pixelrag_render/render.py @@ -376,6 +376,7 @@ def main() -> None: viewport_width=args.viewport_width, workers=1, wait_network_idle=args.wait_network_idle, + extract_text=args.extract_text, cdp_url=args.cdp_url, ) elif suffix in {".png", ".jpg", ".jpeg", ".webp"}: diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py new file mode 100644 index 00000000..a5c2e73a --- /dev/null +++ b/tests/test_bug_fixes.py @@ -0,0 +1,138 @@ +"""Regression tests for bug fixes across configuration, rendering, indexing, and sources.""" + +import json +import os +import subprocess +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from PIL import Image + +from pixelrag_embed.chunk import chunk_article +from pixelrag_index.config import DEFAULT_CONFIG, load_config +from pixelrag_index.pipelines import _department_of +from pixelrag_index.sources.kiwix import KiwixServeManager +from pixelrag_index.sources.local import LocalSource + + +def test_load_config_deep_merge(tmp_path): + """Deep merge ensures single-field overrides do not wipe out sibling default settings.""" + cfg_file = tmp_path / "custom.yaml" + cfg_file.write_text( + "embed:\n" + " device: cpu\n" + "ingest:\n" + " quality: 92\n", + encoding="utf-8", + ) + loaded = load_config(str(cfg_file)) + + # embed.device is overridden, but embed.model is preserved from DEFAULT_CONFIG + assert loaded["embed"]["device"] == "cpu" + assert loaded["embed"]["model"] == DEFAULT_CONFIG["embed"]["model"] + + # ingest.quality is overridden, but ingest.backend and tile_height are preserved + assert loaded["ingest"]["quality"] == 92 + assert loaded["ingest"]["backend"] == DEFAULT_CONFIG["ingest"]["backend"] + assert loaded["ingest"]["tile_height"] == DEFAULT_CONFIG["ingest"]["tile_height"] + + +def test_local_source_generates_valid_as_uri(tmp_path): + """LocalSource must yield RFC 8089 as_uri() without raw backslashes or unencoded spaces.""" + doc_dir = tmp_path / "my docs" + doc_dir.mkdir() + html_file = doc_dir / "index test.html" + html_file.write_text("

Hello

", encoding="utf-8") + + source = LocalSource(str(doc_dir)) + docs = list(source) + assert len(docs) == 1 + doc = docs[0] + + assert doc.url.startswith("file:///") + assert "\\" not in doc.url + assert " " not in doc.url # spaces must be percent-encoded + assert "%20" in doc.url + + +def test_department_of_resolves_file_uri(tmp_path): + """_department_of correctly extracts department on all platforms including Windows drive letters.""" + dept_dir = tmp_path / "finance" + dept_dir.mkdir() + doc_file = dept_dir / "report 2026.html" + doc_file.write_text("

Finances

", encoding="utf-8") + + file_uri = doc_file.resolve().as_uri() + art = {"url": file_uri} + department = _department_of(art, str(tmp_path)) + assert department == "finance" + + +def test_pixelshot_forwards_extract_text_for_html(monkeypatch, tmp_path): + """pixelshot CLI must forward extract_text flag when given local HTML files.""" + from pixelrag_render import render as render_mod + + html_file = tmp_path / "page.html" + html_file.write_text("

content

", encoding="utf-8") + + called_kwargs = {} + + def fake_render_url(url, output_dir, **kwargs): + called_kwargs.update(kwargs) + return [tmp_path / "page.png.tiles"] + + monkeypatch.setattr(render_mod, "render_url", fake_render_url) + monkeypatch.setattr( + sys, + "argv", + ["pixelshot", str(html_file), "-o", str(tmp_path / "tiles"), "--extract-text"], + ) + + render_mod.main() + assert called_kwargs.get("extract_text") is True + + +def test_kiwix_kill_proc_cross_platform(): + """KiwixServeManager._kill_proc safely terminates process without os.killpg AttributeError.""" + mgr = KiwixServeManager.__new__(KiwixServeManager) + mock_proc = MagicMock() + mock_proc.poll.return_value = None + + # Should not raise AttributeError on Windows where os.killpg is absent + mgr._kill_proc(mock_proc) + assert mock_proc.terminate.called or mock_proc.wait.called + + +def test_chunk_article_converts_jpeg_to_png(tmp_path): + """Fast-path chunking of JPEG tile into chunk_...png must create a valid PNG.""" + article_dir = tmp_path / "article.png.tiles" + article_dir.mkdir() + + # Create a small JPEG tile + img = Image.new("RGB", (800, 600), color="blue") + tile_name = "tile_0000.jpg" + img.save(article_dir / tile_name, format="JPEG") + + manifest = { + "url": "https://example.com/test", + "tiles": [tile_name], + "page_height": 600, + "viewport_width": 875, + } + (article_dir / "tiles.json").write_text(json.dumps(manifest)) + + res = chunk_article(str(article_dir)) + assert res is not None + assert res["num_chunks"] == 1 + + chunk_file = article_dir / "chunk_0000_00.png" + assert chunk_file.exists() + + # Verify chunk_file is actually a valid PNG, not raw JPEG bytes + with Image.open(chunk_file) as chunk_img: + assert chunk_img.format == "PNG" + + # Verify chunks.json exists and was written + assert (article_dir / "chunks.json").exists() diff --git a/tests/test_department_filter.py b/tests/test_department_filter.py index 4da41868..7743a8bc 100644 --- a/tests/test_department_filter.py +++ b/tests/test_department_filter.py @@ -45,21 +45,26 @@ def test_web_url_outside_root_and_empty_root(tmp_path): assert pipelines._department_of({"path": "/a/b.pdf"}, "") == "" -# --------------------------------------------------------------------------- -# Serve side: IDSelector filter -# --------------------------------------------------------------------------- - -faiss = pytest.importorskip("faiss") -np = pytest.importorskip("numpy") -api = pytest.importorskip("pixelrag_serve.api") - -DIM = 8 -# 3 articles × 2 vectors; article 0, 2 → hr; article 1 → ketoan -ARTICLE_IDS = np.array([0, 0, 1, 1, 2, 2], dtype=np.int64) -HR_ROWS = {0, 1, 4, 5} - - -def _vectors() -> np.ndarray: +try: + import faiss + import numpy as np + from pixelrag_serve import api + + _HAS_FAISS = True + DIM = 8 + ARTICLE_IDS = np.array([0, 0, 1, 1, 2, 2], dtype=np.int64) + HR_ROWS = {0, 1, 4, 5} +except ImportError: + _HAS_FAISS = False + faiss = None + np = None + api = None + DIM = 8 + ARTICLE_IDS = None + HR_ROWS = None + + +def _vectors() -> "np.ndarray": rng = np.random.default_rng(7) v = rng.standard_normal((6, DIM)).astype(np.float32) return v / np.linalg.norm(v, axis=1, keepdims=True) @@ -77,6 +82,7 @@ def _setup_state(index): ) +@pytest.mark.skipif(not _HAS_FAISS, reason="faiss or pixelrag_serve not installed") def test_department_positions_maps_articles_to_rows(): index = faiss.IndexFlatIP(DIM) index.add(_vectors()) @@ -85,6 +91,7 @@ def test_department_positions_maps_articles_to_rows(): assert set(api._department_positions("ketoan").tolist()) == {2, 3} +@pytest.mark.skipif(not _HAS_FAISS, reason="faiss or pixelrag_serve not installed") def test_flat_index_filter_restricts_hits(): vecs = _vectors() index = faiss.IndexFlatIP(DIM) @@ -96,6 +103,7 @@ def test_flat_index_filter_restricts_hits(): assert hits == HR_ROWS # every hr row returned, nothing else +@pytest.mark.skipif(not _HAS_FAISS, reason="faiss or pixelrag_serve not installed") def test_ivf_index_filter_restricts_hits(): vecs = _vectors() quantizer = faiss.IndexFlatIP(DIM) @@ -110,6 +118,7 @@ def test_ivf_index_filter_restricts_hits(): assert hits == {2, 3} +@pytest.mark.skipif(not _HAS_FAISS, reason="faiss or pixelrag_serve not installed") def test_unknown_department_raises_404(): index = faiss.IndexFlatIP(DIM) index.add(_vectors()) @@ -120,6 +129,7 @@ def test_unknown_department_raises_404(): assert "hr" in exc.value.detail # lists available departments +@pytest.mark.skipif(not _HAS_FAISS, reason="faiss or pixelrag_serve not installed") def test_index_without_department_metadata_raises_400(): index = faiss.IndexFlatIP(DIM) index.add(_vectors()) diff --git a/web/agent-server.mjs b/web/agent-server.mjs index 13eef010..9127e93c 100644 --- a/web/agent-server.mjs +++ b/web/agent-server.mjs @@ -99,6 +99,14 @@ function retrievalDown(health, onEvent, label, detail) { return { content: [{ type: "text", text: backendDownInstruction(detail) }], isError: true } } +function safeDecodeURIComponent(str) { + try { + return decodeURIComponent(str) + } catch { + return str + } +} + function createTools(onEvent, uploadedImage, health) { const searchTool = tool( "pixelrag_search", @@ -147,7 +155,7 @@ function createTools(onEvent, uploadedImage, health) { const results = hits.map((h) => { const slug = h.url.includes("/wiki/") ? h.url.split("/wiki/").pop() : h.url return { - title: decodeURIComponent(slug || "").replace(/_/g, " "), + title: safeDecodeURIComponent(slug || "").replace(/_/g, " "), url: h.url.startsWith("http") ? h.url : `https://en.wikipedia.org/wiki/${slug}`, score: Math.round(h.score * 1000) / 1000, article_id: h.article_id, diff --git a/web/app/api/chat/route.ts b/web/app/api/chat/route.ts index 9d99dd8b..f9af18d5 100644 --- a/web/app/api/chat/route.ts +++ b/web/app/api/chat/route.ts @@ -71,6 +71,14 @@ function retrievalDown( } } +function safeDecodeURIComponent(str: string): string { + try { + return decodeURIComponent(str) + } catch { + return str + } +} + function createTools( onEvent: (event: string, data: unknown) => void, uploadedImage: string | null, @@ -166,7 +174,7 @@ function createTools( ? h.url.split("/wiki/").pop() : h.url return { - title: decodeURIComponent(slug || "").replace(/_/g, " "), + title: safeDecodeURIComponent(slug || "").replace(/_/g, " "), url: h.url.startsWith("http") ? h.url : `https://en.wikipedia.org/wiki/${slug}`, From 749dfb17e9aaf2b7791b4e4251aa6ef0bda1f1d4 Mon Sep 17 00:00:00 2001 From: smartworldarafath Date: Fri, 25 Sep 2026 12:50:56 +0600 Subject: [PATCH 2/3] style: format code with ruff and clean up unused imports --- index/src/pixelrag_index/config.py | 6 +++++- index/src/pixelrag_index/sources/kiwix.py | 6 +++++- tests/test_bug_fixes.py | 10 +--------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/index/src/pixelrag_index/config.py b/index/src/pixelrag_index/config.py index 08bffc85..fab9abb3 100644 --- a/index/src/pixelrag_index/config.py +++ b/index/src/pixelrag_index/config.py @@ -27,7 +27,11 @@ def load_config(path=None): config = {} merged = {**DEFAULT_CONFIG, **config} for section in ("ingest", "embed"): - if section in DEFAULT_CONFIG and section in config and isinstance(config[section], dict): + if ( + section in DEFAULT_CONFIG + and section in config + and isinstance(config[section], dict) + ): merged[section] = {**DEFAULT_CONFIG[section], **config[section]} return merged diff --git a/index/src/pixelrag_index/sources/kiwix.py b/index/src/pixelrag_index/sources/kiwix.py index d8506d32..06561e18 100644 --- a/index/src/pixelrag_index/sources/kiwix.py +++ b/index/src/pixelrag_index/sources/kiwix.py @@ -246,7 +246,11 @@ def _kill_proc(self, proc: subprocess.Popen) -> None: proc.wait(timeout=5) except subprocess.TimeoutExpired: try: - if hasattr(os, "killpg") and hasattr(os, "getpgid") and hasattr(signal, "SIGKILL"): + if ( + hasattr(os, "killpg") + and hasattr(os, "getpgid") + and hasattr(signal, "SIGKILL") + ): os.killpg(os.getpgid(proc.pid), signal.SIGKILL) else: proc.kill() diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py index a5c2e73a..369cebed 100644 --- a/tests/test_bug_fixes.py +++ b/tests/test_bug_fixes.py @@ -1,15 +1,10 @@ """Regression tests for bug fixes across configuration, rendering, indexing, and sources.""" import json -import os -import subprocess import sys -from pathlib import Path from unittest.mock import MagicMock -import pytest from PIL import Image - from pixelrag_embed.chunk import chunk_article from pixelrag_index.config import DEFAULT_CONFIG, load_config from pixelrag_index.pipelines import _department_of @@ -21,10 +16,7 @@ def test_load_config_deep_merge(tmp_path): """Deep merge ensures single-field overrides do not wipe out sibling default settings.""" cfg_file = tmp_path / "custom.yaml" cfg_file.write_text( - "embed:\n" - " device: cpu\n" - "ingest:\n" - " quality: 92\n", + "embed:\n device: cpu\ningest:\n quality: 92\n", encoding="utf-8", ) loaded = load_config(str(cfg_file)) From 453bc763e45d6bcdb5fcc34e4b1baf5486af18fe Mon Sep 17 00:00:00 2001 From: Aakash Suresh Date: Sun, 27 Sep 2026 01:39:02 -0700 Subject: [PATCH 3/3] fix(test): stop test_kiwix_kill_proc sending a real SIGTERM The test passed a bare MagicMock to `_kill_proc`. A MagicMock's `.pid` satisfies `__index__` as 1, so the POSIX branch executed os.killpg(os.getpgid(1), signal.SIGTERM) for real. Under CI that group contains the runner: the Tests job died with exit 143 ("the runner has received a shutdown signal") partway through this file, twice, which reads as an unrelated infrastructure flake rather than a test defect. Locally it looked green only because the resulting OSError was swallowed by the `except (OSError, ProcessLookupError)` in `_kill_proc`. Replaced with three tests that patch `os.getpgid`/`os.killpg` and assert on them, covering the branches the original conflated: - POSIX: the child's process group is signalled, with a real pid - Windows (no `os.killpg`): falls back to `proc.terminate()` - already exited (`poll()` returns a status): nothing is signalled at all 8 tests pass; ruff check and format clean. Co-Authored-By: Claude Opus 5 --- tests/test_bug_fixes.py | 60 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/tests/test_bug_fixes.py b/tests/test_bug_fixes.py index 369cebed..fd1b55a7 100644 --- a/tests/test_bug_fixes.py +++ b/tests/test_bug_fixes.py @@ -1,13 +1,17 @@ """Regression tests for bug fixes across configuration, rendering, indexing, and sources.""" import json +import signal import sys +import types +from unittest import mock from unittest.mock import MagicMock from PIL import Image from pixelrag_embed.chunk import chunk_article from pixelrag_index.config import DEFAULT_CONFIG, load_config from pixelrag_index.pipelines import _department_of +from pixelrag_index.sources import kiwix as kiwix_module from pixelrag_index.sources.kiwix import KiwixServeManager from pixelrag_index.sources.local import LocalSource @@ -86,15 +90,57 @@ def fake_render_url(url, output_dir, **kwargs): assert called_kwargs.get("extract_text") is True -def test_kiwix_kill_proc_cross_platform(): - """KiwixServeManager._kill_proc safely terminates process without os.killpg AttributeError.""" +def test_kiwix_kill_proc_posix_signals_the_process_group(): + """POSIX path signals the child's group -- with os patched, not for real. + + A MagicMock's ``.pid`` satisfies ``__index__`` as 1, so calling the real + ``os.getpgid``/``os.killpg`` here sends SIGTERM to a live process group on + the host. Under CI that is the runner's own group: the job dies with exit + 143 ("the runner has received a shutdown signal") partway through this + file, which reads as an unrelated infrastructure flake. Patch both calls + and assert on them instead. + """ + mgr = KiwixServeManager.__new__(KiwixServeManager) + proc = MagicMock() + proc.poll.return_value = None + proc.pid = 4242 + + with ( + mock.patch.object(kiwix_module.os, "getpgid", return_value=4242) as getpgid, + mock.patch.object(kiwix_module.os, "killpg") as killpg, + ): + mgr._kill_proc(proc) + + getpgid.assert_called_once_with(4242) + killpg.assert_called_once_with(4242, signal.SIGTERM) + proc.wait.assert_called_once() + + +def test_kiwix_kill_proc_without_killpg_falls_back_to_terminate(): + """Windows path: no os.killpg, so terminate() instead of an AttributeError.""" mgr = KiwixServeManager.__new__(KiwixServeManager) - mock_proc = MagicMock() - mock_proc.poll.return_value = None + proc = MagicMock() + proc.poll.return_value = None + + windows_os = types.SimpleNamespace(path=kiwix_module.os.path) + with mock.patch.object(kiwix_module, "os", windows_os): + mgr._kill_proc(proc) + + proc.terminate.assert_called_once() + proc.wait.assert_called_once() + + +def test_kiwix_kill_proc_skips_an_already_exited_process(): + """poll() returning an exit status means there is nothing left to signal.""" + mgr = KiwixServeManager.__new__(KiwixServeManager) + proc = MagicMock() + proc.poll.return_value = 0 + + with mock.patch.object(kiwix_module.os, "killpg") as killpg: + mgr._kill_proc(proc) - # Should not raise AttributeError on Windows where os.killpg is absent - mgr._kill_proc(mock_proc) - assert mock_proc.terminate.called or mock_proc.wait.called + killpg.assert_not_called() + proc.terminate.assert_not_called() def test_chunk_article_converts_jpeg_to_png(tmp_path):