fix: address PR #9 review feedback

ayhammouda · claude · ayhammouda · commit 928fc183a77d · 2026-05-09T08:54:13.000+02:00
- package_docs: catch UnicodeDecodeError on non-UTF-8 PyPI
  responses so they surface as the controlled empty-result
  path instead of leaking a tool-level exception
- ranker/SymbolHit: return None for page-only symbol anchors
  so get_docs() retrieves the whole page; SymbolHit.anchor is
  now str | None
- persistent_cache: serialize execute()/commit()/stats updates
  with threading.Lock — per the Python sqlite3 docs,
  check_same_thread=False alone does not make a connection
  safe for concurrent writes; without serialization concurrent
  put() raised SystemError under load

Minor follow-ups picked up along the way: _empty_result helper
in package_docs, drop unused PackageDocsInput model, use
storage.db.get_cache_dir/get_index_path helpers from server.py,
import _NO_ANCHOR_KEY constant in smoke test.

Tests: UTF-8 decode path, page-only anchor=None contract,
concurrent put() across 20 threads. 261/261 pass.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/src/mcp_server_python_docs/models.py b/src/mcp_server_python_docs/models.py
@@ -48,7 +48,10 @@ class SymbolHit(BaseModel):
     score: float = Field(default=0.0, description="Relevance score")
     version: str = Field(description="Python version this hit belongs to")
     slug: str = Field(default="", description="Page slug for get_docs follow-up")
-    anchor: str = Field(default="", description="Section anchor for get_docs follow-up")
+    anchor: str | None = Field(
+        default=None,
+        description="Section anchor for get_docs follow-up; None for page-level hits",
+    )
 
 
 class SearchDocsResult(BaseModel):
@@ -165,16 +168,6 @@ class DetectPythonVersionResult(BaseModel):
 # --- lookup_package_docs models ---
 
 
-class PackageDocsInput(BaseModel):
-    """Input parameters for lookup_package_docs tool."""
-
-    package: str = Field(
-        min_length=1,
-        max_length=214,
-        description="PyPI package/project name (e.g. 'requests').",
-    )
-
-
 class PackageDocsSource(BaseModel):
     """A package-declared documentation or project source URL."""
 
diff --git a/src/mcp_server_python_docs/retrieval/ranker.py b/src/mcp_server_python_docs/retrieval/ranker.py
@@ -37,8 +37,12 @@ def _document_candidates(uri: str) -> tuple[str, ...]:
 def _resolve_symbol_location(
     conn: sqlite3.Connection,
     row: sqlite3.Row,
-) -> tuple[str, str]:
-    """Resolve a symbol row to a get_docs-compatible slug and anchor."""
+) -> tuple[str, str | None]:
+    """Resolve a symbol row to a get_docs-compatible slug and anchor.
+
+    Returns ``(slug, None)`` when the hit is page-level. ``get_docs()`` treats
+    any non-None anchor as a section lookup, so blank anchors must never leak.
+    """
     uri = str(row["uri"])
     fallback_slug = _page_uri(uri)
     fallback_anchor = str(row["anchor"] or "")
@@ -56,7 +60,7 @@ def _resolve_symbol_location(
             (section_id,),
         ).fetchone()
         if section_row is not None:
-            return section_row["slug"], section_row["anchor"] or ""
+            return section_row["slug"], (section_row["anchor"] or None)
 
     doc_row = None
     document_id = row["document_id"]
@@ -81,22 +85,22 @@ def _resolve_symbol_location(
                 break
 
     if doc_row is None:
-        return fallback_slug, fallback_anchor
+        return fallback_slug, (fallback_anchor or None)
 
     if fallback_anchor:
         section_row = conn.execute(
             """
-            SELECT anchor
+            SELECT 1
             FROM sections
             WHERE document_id = ? AND anchor = ?
             LIMIT 1
             """,
             (doc_row["id"], fallback_anchor),
         ).fetchone()
         if section_row is not None:
-            return doc_row["slug"], section_row["anchor"] or ""
+            return doc_row["slug"], fallback_anchor
 
-    return doc_row["slug"], ""
+    return doc_row["slug"], None
 
 
 def _normalize_scores(hits: list[SymbolHit]) -> list[SymbolHit]:
@@ -182,7 +186,7 @@ def search_sections(
             score=row["score"],
             version=row["version"],
             slug=row["slug"],
-            anchor=row["anchor"],
+            anchor=row["anchor"] or None,
         )
         for row in rows
     ]
@@ -300,7 +304,7 @@ def search_examples(
             score=row["score"],
             version=row["version"],
             slug=row["slug"],
-            anchor=row["anchor"] or "",
+            anchor=row["anchor"] or None,
         )
         for row in rows
     ]
diff --git a/src/mcp_server_python_docs/server.py b/src/mcp_server_python_docs/server.py
@@ -14,10 +14,8 @@
 import traceback
 from collections.abc import AsyncIterator
 from contextlib import asynccontextmanager
-from pathlib import Path
 from typing import Annotated, Literal
 
-import platformdirs
 import yaml
 from mcp.server.fastmcp import Context, FastMCP
 from mcp.server.fastmcp.exceptions import ToolError
@@ -39,7 +37,11 @@
 from mcp_server_python_docs.services.persistent_cache import PersistentDocsCache
 from mcp_server_python_docs.services.search import SearchService
 from mcp_server_python_docs.services.version import VersionService
-from mcp_server_python_docs.storage.db import get_readonly_connection
+from mcp_server_python_docs.storage.db import (
+    get_cache_dir,
+    get_index_path,
+    get_readonly_connection,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -67,8 +69,8 @@ async def app_lifespan(server: FastMCP) -> AsyncIterator[AppContext]:
     constructs service instances, and fails fast on missing index or
     unavailable FTS5.
     """
-    cache_dir = Path(platformdirs.user_cache_dir("mcp-python-docs"))
-    index_path = cache_dir / "index.db"
+    cache_dir = get_cache_dir()
+    index_path = get_index_path()
 
     # Fail fast on missing index (SRVR-10)
     if not index_path.exists():
diff --git a/src/mcp_server_python_docs/services/package_docs.py b/src/mcp_server_python_docs/services/package_docs.py
@@ -71,6 +71,16 @@ def _read_limited(response: _HTTPResponse) -> bytes | None:
     return data
 
 
+def _empty_result(package: str, metadata_source: str, note: str) -> PackageDocsResult:
+    return PackageDocsResult(
+        package=package,
+        version="",
+        metadata_source=metadata_source,
+        sources=[],
+        note=note,
+    )
+
+
 class PackageDocsService:
     """Return package-declared docs/homepage/source URLs from PyPI metadata only."""
 
@@ -86,32 +96,20 @@ def lookup(self, package: str) -> PackageDocsResult:
             with self._fetcher(metadata_source, self._timeout) as response:
                 data = _read_limited(response)
                 if data is None:
-                    return PackageDocsResult(
-                        package=package,
-                        version="",
-                        metadata_source=metadata_source,
-                        sources=[],
-                        note="PyPI metadata exceeded size limit.",
+                    return _empty_result(
+                        package, metadata_source, "PyPI metadata exceeded size limit."
                     )
                 payload = json.loads(data.decode("utf-8"))
         except HTTPError as e:
             note = (
                 "Package not found on PyPI." if e.code == 404 else f"PyPI returned HTTP {e.code}."
             )
-            return PackageDocsResult(
-                package=package,
-                version="",
-                metadata_source=metadata_source,
-                sources=[],
-                note=note,
-            )
-        except (URLError, TimeoutError, json.JSONDecodeError) as e:
-            return PackageDocsResult(
-                package=package,
-                version="",
-                metadata_source=metadata_source,
-                sources=[],
-                note=f"Unable to retrieve PyPI metadata: {type(e).__name__}.",
+            return _empty_result(package, metadata_source, note)
+        except (URLError, TimeoutError, json.JSONDecodeError, UnicodeDecodeError) as e:
+            return _empty_result(
+                package,
+                metadata_source,
+                f"Unable to retrieve PyPI metadata: {type(e).__name__}.",
             )
 
         info = payload.get("info") if isinstance(payload, dict) else {}
diff --git a/src/mcp_server_python_docs/services/persistent_cache.py b/src/mcp_server_python_docs/services/persistent_cache.py
@@ -4,6 +4,7 @@
 
 import logging
 import sqlite3
+import threading
 from pathlib import Path
 from typing import NamedTuple
 
@@ -28,10 +29,16 @@ def __init__(self, cache_path: Path, index_path: Path) -> None:
         self._cache_path = Path(cache_path)
         self._fingerprint = self._fingerprint_index(Path(index_path))
         self._hits = self._misses = self._writes = 0
+        # ``check_same_thread=False`` lets multiple threads share the connection,
+        # but per the Python sqlite3 docs writes must still be serialized by the
+        # application — this lock guards every execute()/commit() and the stats
+        # counters they update.
+        self._lock = threading.Lock()
         self._conn: sqlite3.Connection | None = None
         try:
             self._cache_path.parent.mkdir(parents=True, exist_ok=True)
             self._conn = sqlite3.connect(str(self._cache_path), check_same_thread=False)
+            self._conn.execute("PRAGMA journal_mode = WAL")
             self._conn.execute("PRAGMA synchronous = NORMAL")
             self._conn.execute(
                 "CREATE TABLE IF NOT EXISTS retrieved_docs_cache ("
@@ -40,6 +47,10 @@ def __init__(self, cache_path: Path, index_path: Path) -> None:
                 "result_json TEXT NOT NULL, created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, "
                 "PRIMARY KEY (index_fingerprint, version, slug, anchor, max_chars, start_index))"
             )
+            self._conn.execute(
+                "DELETE FROM retrieved_docs_cache WHERE index_fingerprint != ?",
+                (self._fingerprint,),
+            )
             self._conn.commit()
         except (OSError, sqlite3.Error) as e:
             if self._conn is not None:
@@ -67,62 +78,67 @@ def get(
         self, *, version: str, slug: str, anchor: str | None, max_chars: int, start_index: int
     ) -> GetDocsResult | None:
         if self._conn is None:
-            self._misses += 1
-            return None
-        try:
-            row = self._conn.execute(
-                "SELECT result_json FROM retrieved_docs_cache WHERE index_fingerprint = ? "
-                "AND version = ? AND slug = ? AND anchor = ? AND max_chars = ? AND start_index = ?",
-                (
-                    self._fingerprint,
-                    version,
-                    slug,
-                    self._anchor_key(anchor),
-                    max_chars,
-                    start_index,
-                ),
-            ).fetchone()
-        except sqlite3.Error as e:
-            self._misses += 1
-            logger.warning("Persistent docs cache read skipped: %s", e)
+            with self._lock:
+                self._misses += 1
             return None
-        if row is None:
-            self._misses += 1
-            return None
-        try:
-            result = GetDocsResult.model_validate_json(row[0])
-        except (ValidationError, ValueError) as e:
-            self._misses += 1
-            logger.warning("Persistent docs cache entry ignored: %s", e)
-            return None
-        self._hits += 1
-        return result
+        with self._lock:
+            try:
+                row = self._conn.execute(
+                    "SELECT result_json FROM retrieved_docs_cache WHERE index_fingerprint = ? "
+                    "AND version = ? AND slug = ? AND anchor = ? AND max_chars = ? "
+                    "AND start_index = ?",
+                    (
+                        self._fingerprint,
+                        version,
+                        slug,
+                        self._anchor_key(anchor),
+                        max_chars,
+                        start_index,
+                    ),
+                ).fetchone()
+            except sqlite3.Error as e:
+                self._misses += 1
+                logger.warning("Persistent docs cache read skipped: %s", e)
+                return None
+            if row is None:
+                self._misses += 1
+                return None
+            try:
+                result = GetDocsResult.model_validate_json(row[0])
+            except (ValidationError, ValueError) as e:
+                self._misses += 1
+                logger.warning("Persistent docs cache entry ignored: %s", e)
+                return None
+            self._hits += 1
+            return result
 
     def put(self, *, result: GetDocsResult, max_chars: int, start_index: int) -> None:
         if self._conn is None:
             return
-        try:
-            self._conn.execute(
-                "INSERT OR REPLACE INTO retrieved_docs_cache "
-                "(index_fingerprint, version, slug, anchor, max_chars, start_index, result_json) "
-                "VALUES (?, ?, ?, ?, ?, ?, ?)",
-                (
-                    self._fingerprint,
-                    result.version,
-                    result.slug,
-                    self._anchor_key(result.anchor),
-                    max_chars,
-                    start_index,
-                    result.model_dump_json(),
-                ),
-            )
-            self._conn.commit()
-        except (sqlite3.Error, ValueError) as e:
-            logger.warning("Persistent docs cache write skipped: %s", e)
-            return
-        self._writes += 1
+        with self._lock:
+            try:
+                self._conn.execute(
+                    "INSERT OR REPLACE INTO retrieved_docs_cache "
+                    "(index_fingerprint, version, slug, anchor, max_chars, start_index, "
+                    "result_json) VALUES (?, ?, ?, ?, ?, ?, ?)",
+                    (
+                        self._fingerprint,
+                        result.version,
+                        result.slug,
+                        self._anchor_key(result.anchor),
+                        max_chars,
+                        start_index,
+                        result.model_dump_json(),
+                    ),
+                )
+                self._conn.commit()
+            except (sqlite3.Error, ValueError) as e:
+                logger.warning("Persistent docs cache write skipped: %s", e)
+                return
+            self._writes += 1
 
     def close(self) -> None:
-        if self._conn is not None:
-            self._conn.close()
-            self._conn = None
+        with self._lock:
+            if self._conn is not None:
+                self._conn.close()
+                self._conn = None
diff --git a/tests/fixtures/schema-search_docs-output.json b/tests/fixtures/schema-search_docs-output.json
@@ -42,10 +42,17 @@
           "type": "string"
         },
         "anchor": {
-          "default": "",
-          "description": "Section anchor for get_docs follow-up",
-          "title": "Anchor",
-          "type": "string"
+          "anyOf": [
+            {
+              "type": "string"
+            },
+            {
+              "type": "null"
+            }
+          ],
+          "default": null,
+          "description": "Section anchor for get_docs follow-up; None for page-level hits",
+          "title": "Anchor"
         }
       },
       "required": [
diff --git a/tests/test_mcp_get_docs_cache_smoke.py b/tests/test_mcp_get_docs_cache_smoke.py
@@ -6,6 +6,7 @@
 import sys
 from pathlib import Path
 
+from mcp_server_python_docs.services.persistent_cache import _NO_ANCHOR_KEY
 from mcp_server_python_docs.storage.db import bootstrap_schema, get_readwrite_connection
 from tests.test_stdio_smoke import (
     _assert_protocol_on_stdout_only,
@@ -150,7 +151,7 @@ def test_get_docs_cache_restart_and_corrupt_cache_fallback(tmp_path: Path):
     assert (version, slug, anchor, max_chars, start_index) == (
         "3.13",
         "library/json.html",
-        "\x00mcp-python-docs:no-anchor\x00",
+        _NO_ANCHOR_KEY,
         8000,
         0,
     )
diff --git a/tests/test_package_docs.py b/tests/test_package_docs.py
@@ -128,3 +128,10 @@ def invalid_json(url: str, timeout: float):
     json_result = PackageDocsService(fetcher=invalid_json).lookup("demo")
     assert json_result.sources == []
     assert json_result.note == "Unable to retrieve PyPI metadata: JSONDecodeError."
+
+    def invalid_utf8(url: str, timeout: float):
+        return _Resp(b"\xff\xfe\xfd")
+
+    utf8_result = PackageDocsService(fetcher=invalid_utf8).lookup("demo")
+    assert utf8_result.sources == []
+    assert utf8_result.note == "Unable to retrieve PyPI metadata: UnicodeDecodeError."
diff --git a/tests/test_persistent_docs_cache.py b/tests/test_persistent_docs_cache.py
diff --git a/tests/test_retrieval.py b/tests/test_retrieval.py
diff --git a/tests/test_services.py b/tests/test_services.py
