diff --git a/.github/api-surface-baseline.json b/.github/api-surface-baseline.json index 98e0dc0..5ffb1a7 100644 --- a/.github/api-surface-baseline.json +++ b/.github/api-surface-baseline.json @@ -13,6 +13,124 @@ }, "functions": {} }, + "mcp_server.file_transaction": { + "classes": { + "StagedTextFile": { + "bases": [], + "methods": { + "close": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, + "publish": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + } + } + } + }, + "functions": { + "replace_with_retry": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "source" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "destination" + } + ], + "is_async": false, + "returns_annotated": true + }, + "staged_text_file": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "destination" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "content" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, + "mcp_server.filesystem": { + "classes": {}, + "functions": { + "io_path": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "path" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "extend_directory" + } + ], + "is_async": false, + "returns_annotated": true + }, + "resolve_path": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "path" + } + ], + "is_async": false, + "returns_annotated": true + }, + "walk_document_paths": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "directory" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, "mcp_server.fts5_index": { "classes": { "Fts5CorruptError": { @@ -60,6 +178,18 @@ "is_async": false, "returns_annotated": true }, + "clear": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, "close": { "args": [ { @@ -72,6 +202,18 @@ "is_async": false, "returns_annotated": true }, + "count": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, "db_path": { "args": [ { @@ -133,6 +275,12 @@ "has_default": true, "kind": "positional", "name": "top_k" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "allowed_ids" } ], "is_async": false, @@ -169,6 +317,12 @@ "has_default": true, "kind": "keyword_only", "name": "on_progress" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "source_snapshot" } ], "is_async": false, @@ -186,6 +340,24 @@ "is_async": false, "returns_annotated": true }, + "stop_migration": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "timeout" + } + ], + "is_async": false, + "returns_annotated": true + }, "update_document": { "args": [ { @@ -308,6 +480,15 @@ } } }, + "mcp_server.health": { + "classes": { + "HealthMiddleware": { + "bases": [], + "methods": {} + } + }, + "functions": {} + }, "mcp_server.ingestion": { "classes": { "Chunk": { @@ -346,6 +527,24 @@ "DocumentParser": { "bases": [], "methods": { + "iter_files": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "directory" + } + ], + "is_async": false, + "returns_annotated": false + }, "parse_directory": { "args": [ { @@ -377,6 +576,12 @@ "has_default": false, "kind": "positional", "name": "filepath" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "logical_filename" } ], "is_async": false, @@ -422,6 +627,67 @@ } } }, + "mcp_server.logging_config": { + "classes": { + "JsonFormatter": { + "bases": [ + "logging.Formatter" + ], + "methods": { + "format": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "record" + } + ], + "is_async": false, + "returns_annotated": true + } + } + } + }, + "functions": { + "get_logger": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "name" + } + ], + "is_async": false, + "returns_annotated": true + }, + "setup_logging": { + "args": [ + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "fmt" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "level" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, "mcp_server.metrics": { "classes": { "MetricsCollector": { @@ -588,6 +854,41 @@ } } }, + "mcp_server.pdf_support": { + "classes": {}, + "functions": { + "read_extracted_pdf_text": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "filepath" + } + ], + "is_async": false, + "returns_annotated": true + }, + "release_exception_frames": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "error" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "stop_at" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, "mcp_server.preflight": { "classes": {}, "functions": { @@ -605,6 +906,65 @@ } } }, + "mcp_server.publication": { + "classes": { + "PublicationLock": { + "bases": [], + "methods": { + "read": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, + "write": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + } + } + } + }, + "functions": { + "collection_publication": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "orchestrator" + } + ], + "is_async": false, + "returns_annotated": true + }, + "collection_reader": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "method" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, "mcp_server.query_router": { "classes": { "QueryRouter": { @@ -674,6 +1034,117 @@ } } }, + "mcp_server.search_candidates": { + "classes": {}, + "functions": { + "select_rerank_candidates": { + "args": [ + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "candidates" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "limit" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "results" + } + ], + "is_async": false, + "returns_annotated": true + } + } + }, + "mcp_server.search_scope": { + "classes": { + "FolderScope": { + "bases": [], + "methods": { + "active": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, + "create": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "cls" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "root" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "include" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "exclude" + } + ], + "is_async": false, + "returns_annotated": true + }, + "document_ids": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + }, + { + "annotated": true, + "has_default": false, + "kind": "positional", + "name": "indexed" + } + ], + "is_async": false, + "returns_annotated": true + }, + "key": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + } + } + } + }, + "functions": {} + }, "mcp_server.security": { "classes": { "BearerAuthMiddleware": { @@ -924,6 +1395,12 @@ "has_default": true, "kind": "positional", "name": "top_k" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "allowed_ids" } ], "is_async": false, @@ -1363,6 +1840,18 @@ "has_default": true, "kind": "positional", "name": "search_method" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "include_folders" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "exclude_folders" } ], "is_async": false, @@ -1381,6 +1870,12 @@ "has_default": true, "kind": "positional", "name": "resume_state" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "force" } ], "is_async": false, @@ -1487,6 +1982,18 @@ "QueryCache": { "bases": [], "methods": { + "generation": { + "args": [ + { + "annotated": false, + "has_default": false, + "kind": "positional", + "name": "self" + } + ], + "is_async": false, + "returns_annotated": true + }, "get": { "args": [ { @@ -1524,6 +2031,12 @@ "has_default": true, "kind": "positional", "name": "search_method" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "folder_scope" } ], "is_async": false, @@ -1584,6 +2097,18 @@ "has_default": true, "kind": "positional", "name": "search_method" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "expected_generation" + }, + { + "annotated": true, + "has_default": true, + "kind": "keyword_only", + "name": "folder_scope" } ], "is_async": false, @@ -1799,6 +2324,18 @@ "has_default": true, "kind": "positional", "name": "search_method" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "include_folders" + }, + { + "annotated": true, + "has_default": true, + "kind": "positional", + "name": "exclude_folders" } ], "is_async": false, diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7bc38dc..67f047c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -108,6 +108,7 @@ jobs: packages: write steps: - uses: actions/checkout@v7 + - uses: docker/setup-qemu-action@v4 - uses: docker/setup-buildx-action@v4 - uses: docker/login-action@v4 with: @@ -125,6 +126,7 @@ jobs: - uses: docker/build-push-action@v7 with: context: . + platforms: linux/amd64,linux/arm64 push: true tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} diff --git a/CHANGELOG.md b/CHANGELOG.md index ac4ec62..613e4a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 --- +### v4.11.1 (2026-10-25) — Fresh-install startup fix and Windows-safe state writes + +Patch release. On v4.10.0 and v4.11.0, every fresh install and every new data directory, new Docker volumes included, failed to start with `Embedding function conflict … persisted: default`. Installs that already hit the error start normally after upgrading. No breaking changes and no migration steps. Thanks to @mmathoosh and @rshun for the reports, @franzgranlund for tracing it to the preflight probe, @cauger-dev for the detailed Windows analysis in #249, and @vishalhabib99 and @viniciussantos45 for #236 and #242. + +**Added:** + +- The Docker image is published for `linux/amd64` and `linux/arm64` (#242), so Apple Silicon and ARM servers run it natively instead of under emulation. + +**Fixed:** + +- `search_knowledge(category)`, `list_documents(category)` and `add_from_url(title)` now accept an explicit `null` (#236). They were annotated `str = None`, so the tool schema advertised `"default": null` on a string-only field and the MCP SDK rejected `{"category": null}` with a validation error before the tool ran. They are now `Optional[str]`, matching the other optional arguments. +- Fresh installs and new data directories start again (#248, #252). Since v4.10.0 they failed with `Embedding function conflict: new: fastembed-… vs persisted: default`: the startup preflight probe created the collection without an embedding function, so ChromaDB persisted its `default` one and refused FastEmbed afterwards. The probe now only reads the collection, and the empty collection the old probe left behind is recreated on the next start. A collection that holds data is never dropped. +- On Windows, saving the index metadata, the reindex checkpoint, the repair journal, the FTS5 state or a document no longer fails when an antivirus, indexer or concurrent reader briefly holds the file (#249). The atomic replace is retried for about 1.5 s on `WinError 5`/`32`, then the original error is raised. Other platforms are unchanged. +- `reindex_documents` answers `already_running` instead of `started` when a file-watcher pass or a document write holds the index, instead of starting a reindex that was refused at once (#249). `full_rebuild=True` still starts and waits for the lock. + +--- + ### v4.11.0 (2026-10-04) — Cloud-sync data_dir guard **Added:** diff --git a/docs/API.md b/docs/API.md index 83fa3fc..846c4b0 100644 --- a/docs/API.md +++ b/docs/API.md @@ -100,7 +100,7 @@ Index or reindex all documents in the knowledge base. **Runs in background** — | `full_rebuild` | bool | false | Re-embed all documents into a staging collection and replace the live collection after validation. Requires a running orchestrator and temporary storage for both collections. | | `resume` | bool | false | Resume a compatible smart-reindex checkpoint; missing, corrupt, or incompatible checkpoints start a fresh smart run. Cannot be combined with `full_rebuild=true`. | -**Returns:** `{"status": "started", "operation": "..."}` immediately. If already running, returns `{"status": "already_running", "progress": "1200/3734"}`. +**Returns:** `{"status": "started", "operation": "..."}` immediately. If a reindex, a file-watcher pass or a document write is already running, returns `{"status": "already_running", "progress": "1200/3734"}`; retry after it finishes. `full_rebuild=true` waits for a watcher pass or document write instead. Poll until `reindex.active` is false, then inspect `last_error` and `last_result.errors`. An idle worker is not proof of a successful rebuild. Smart reindexing writes a resumable checkpoint; see the [operations guide](reindex-operations.md). diff --git a/mcp_server/__init__.py b/mcp_server/__init__.py index ffe0e92..8bcbb18 100644 --- a/mcp_server/__init__.py +++ b/mcp_server/__init__.py @@ -1,6 +1,6 @@ """Knowledge RAG MCP Server - Local Retrieval-Augmented Generation System""" -__version__ = "4.11.0" +__version__ = "4.11.1" __author__ = "Ailton Rocha (Lyon.)" from .config import Config # noqa: E402 diff --git a/mcp_server/file_transaction.py b/mcp_server/file_transaction.py index 61d7570..ec7c697 100644 --- a/mcp_server/file_transaction.py +++ b/mcp_server/file_transaction.py @@ -8,9 +8,33 @@ from collections.abc import Iterator from contextlib import contextmanager from pathlib import Path +from time import sleep logger = logging.getLogger(__name__) +# ERROR_ACCESS_DENIED / ERROR_SHARING_VIOLATION: a scanner, indexer or reader holds +# the path without FILE_SHARE_DELETE, and Windows cannot replace over it until it lets go. +_WINDOWS_HELD_ERRORS = frozenset({5, 32}) +_REPLACE_RETRY_DELAYS = (0.05, 0.1, 0.2, 0.4, 0.8) + + +def replace_with_retry(source: str | os.PathLike[str], destination: str | os.PathLike[str]) -> None: + """``os.replace`` that waits out a transient Windows handle on either path (GH #249). + + Only PermissionError carrying winerror 5 or 32 is retried, for about 1.5 s; then + it is re-raised, so a real permission problem still surfaces. Elsewhere this is + a plain ``os.replace``. + """ + for delay in _REPLACE_RETRY_DELAYS: + try: + os.replace(source, destination) + return + except PermissionError as exc: + if getattr(exc, "winerror", None) not in _WINDOWS_HELD_ERRORS: + raise + sleep(delay) + os.replace(source, destination) + class StagedTextFile: """Prepare beside the destination so rename works on every supported OS.""" @@ -47,19 +71,19 @@ def publish(self) -> None: ) as stream: backup = Path(stream.name) try: - os.replace(self.destination, backup) + replace_with_retry(self.destination, backup) except BaseException: backup.unlink(missing_ok=True) raise self.backup = backup - os.replace(self.path, self.destination) + replace_with_retry(self.path, self.destination) self.published = True def close(self) -> None: """Restore old bytes after failure; never delete a backup whose restore failed.""" if not self.committed: if self.backup is not None: - os.replace(self.backup, self.destination) + replace_with_retry(self.backup, self.destination) elif self.published: self.destination.unlink(missing_ok=True) for temporary in (self.path, self.backup): diff --git a/mcp_server/fts5_index.py b/mcp_server/fts5_index.py index 0c0d7ce..4c928bf 100644 --- a/mcp_server/fts5_index.py +++ b/mcp_server/fts5_index.py @@ -27,6 +27,8 @@ from pathlib import Path from typing import Any, Callable, Iterable, List, Optional, Sequence, Tuple +from .file_transaction import replace_with_retry + ChunkRow = Tuple[str, str, str, str] ChunkIterFactory = Callable[[], Iterable[ChunkRow]] ProgressCallback = Callable[[int, int], None] @@ -137,7 +139,7 @@ def write(self, payload: dict[str, Any]) -> None: json.dump(payload, tmp) tmp.flush() os.fsync(tmp.fileno()) - os.replace(tmp_name, self._path) + replace_with_retry(tmp_name, self._path) except Exception: try: os.unlink(tmp_name) diff --git a/mcp_server/preflight.py b/mcp_server/preflight.py index d870134..8952309 100644 --- a/mcp_server/preflight.py +++ b/mcp_server/preflight.py @@ -160,6 +160,7 @@ def _probe_chroma(timeout_seconds: int = PROBE_TIMEOUT_SECONDS) -> subprocess.Co """Check Chroma in a child process so native crashes do not kill MCP startup.""" code = r""" import chromadb +from chromadb.errors import NotFoundError from mcp_server.config import config @@ -168,7 +169,13 @@ def _probe_chroma(timeout_seconds: int = PROBE_TIMEOUT_SECONDS) -> subprocess.Co raise SystemExit(0) client = chromadb.PersistentClient(path=str(config.chroma_dir)) -collection = client.get_or_create_collection(name=config.collection_name) +try: + # GH #248/#252: read only. Creating it here persisted Chroma's default + # embedding function, which the server then refused to open. + collection = client.get_collection(name=config.collection_name, embedding_function=None) +except NotFoundError: + print("missing") + raise SystemExit(0) print(collection.count()) """ env = os.environ.copy() diff --git a/mcp_server/server.py b/mcp_server/server.py index ec89ed7..2796f03 100644 --- a/mcp_server/server.py +++ b/mcp_server/server.py @@ -64,7 +64,7 @@ # Local imports from . import __version__ from .config import config -from .file_transaction import staged_text_file +from .file_transaction import replace_with_retry, staged_text_file from .filesystem import io_path from .fts5_index import Fts5LexicalIndex, Fts5MigrationError, Fts5NotReadyError from .ingestion import Document, DocumentParser @@ -1642,17 +1642,41 @@ def _init_chroma_client(self): def _safe_get_collection(self): """Open the collection without discarding data on initialization errors.""" try: - return self.chroma_client.get_or_create_collection( - name=config.collection_name, - embedding_function=self.embed_fn, - metadata={"description": "Knowledge base for RAG"}, - ) + return self._open_collection() except Exception as exc: - raise RuntimeError( - f"Could not open ChromaDB collection {config.collection_name!r} " - f"at {config.chroma_dir}: {exc}. Existing index preserved. " - "Check database access and embedding configuration before retrying." - ) from exc + if not self._drop_empty_default_ef_collection(): + raise RuntimeError( + f"Could not open ChromaDB collection {config.collection_name!r} " + f"at {config.chroma_dir}: {exc}. Existing index preserved. " + "Check database access and embedding configuration before retrying." + ) from exc + return self._open_collection() + + def _open_collection(self) -> Any: + """Get or create the production collection bound to the FastEmbed function.""" + return self.chroma_client.get_or_create_collection( + name=config.collection_name, + embedding_function=self.embed_fn, + metadata={"description": "Knowledge base for RAG"}, + ) + + def _drop_empty_default_ef_collection(self) -> bool: + """Drop the collection only when it is empty and bound to Chroma's default EF. + + GH #248/#252: the preflight probe used to create it without an embedding + function, so Chroma persisted ``default`` and refused FastEmbed on every + later open. An empty one holds nothing; one with data is never touched. + """ + try: + stale = self.chroma_client.get_collection(name=config.collection_name, embedding_function=None) + ef_name = (stale.configuration_json.get("embedding_function") or {}).get("name") + if ef_name != "default" or stale.count() != 0: + return False + self.chroma_client.delete_collection(name=config.collection_name) + except Exception: + return False + print("[RECOVERY] Recreating empty collection bound to Chroma's default embedding (GH #248)", file=sys.stderr) + return True def _check_dimension_mismatch(self) -> bool: """Check if stored embeddings have different dimension than current config. @@ -2387,6 +2411,17 @@ def start_reindex_background( if self._reindex_progress.get("active"): return {"status": "already_running", "progress": dict(self._reindex_progress)} + # GH #249: index_all refuses at once while a watcher pass or a document + # write holds the index lock, so "started" would hide the refusal. + # nuclear_rebuild waits for the lock instead. + # ponytail: a probe, not admission. A pass that takes the lock between this + # check and the worker's index_all is still refused, as before, and shows up + # as last_result.skipped_reason; move admission into the worker if it matters. + if mode != "nuclear_rebuild": + if not self._index_lock.acquire(blocking=False): + return {"status": "already_running", "progress": {**self._reindex_progress, "operation": None}} + self._index_lock.release() + self._reindex_progress = self._fresh_reindex_progress(mode, resume_state) # GH #163 (v4.8.3): propagate force through smart_reindex so @@ -4367,7 +4402,7 @@ def _save_document_repairs(self, repairs: Dict[str, List[str]]) -> None: temporary.unlink(missing_ok=True) raise try: - os.replace(temporary, path) + replace_with_retry(temporary, path) finally: temporary.unlink(missing_ok=True) @@ -4497,7 +4532,7 @@ def _save_metadata(self) -> None: temporary.unlink(missing_ok=True) raise try: - os.replace(temporary, self._metadata_file) + replace_with_retry(temporary, self._metadata_file) finally: temporary.unlink(missing_ok=True) @@ -4556,7 +4591,7 @@ def _write_checkpoint( json.dumps(payload, indent=2, ensure_ascii=False), encoding="utf-8", ) - os.replace(tmp_path, self._checkpoint_file) + replace_with_retry(tmp_path, self._checkpoint_file) # Reflect in progress dict for status polling. self._reindex_progress["checkpoint_saved_at"] = payload["checkpoint_at"] @@ -4688,7 +4723,7 @@ def _make_snippet(content: str, max_chars: int = 500) -> str: def search_knowledge( query: str, max_results: int = 5, - category: str = None, + category: Optional[str] = None, hybrid_alpha: float = 0.3, min_score: float = 0.0, snippet_mode: bool = True, @@ -4986,7 +5021,7 @@ def list_categories() -> str: @mcp.tool() @rate_limited @instrument("list_documents") -def list_documents(category: str = None) -> str: +def list_documents(category: Optional[str] = None) -> str: """ List all indexed documents, optionally filtered by category. @@ -5155,7 +5190,7 @@ def remove_document(filepath: str, delete_file: bool = False) -> str: @mcp.tool() @rate_limited @instrument("add_from_url") -def add_from_url(url: str, category: str = "general", title: str = None) -> str: +def add_from_url(url: str, category: str = "general", title: Optional[str] = None) -> str: """ Fetch content from a URL, convert to markdown, and add to the knowledge base. diff --git a/npm/package.json b/npm/package.json index 655e46f..0191abf 100644 --- a/npm/package.json +++ b/npm/package.json @@ -1,6 +1,6 @@ { "name": "knowledge-rag", - "version": "4.11.0", + "version": "4.11.1", "description": "Local RAG MCP server with hybrid search, cross-encoder reranking and local document indexing.", "bin": { "knowledge-rag": "./bin/cli.js" diff --git a/pyproject.toml b/pyproject.toml index 5202a2b..9aacf4a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "knowledge-rag" -version = "4.11.0" +version = "4.11.1" description = "Local RAG System for Claude Code — Hybrid search + Cross-encoder Reranking + 13 MCP Tools + 20 Format Parsers. Zero external servers." readme = "README.md" license = {text = "MIT"} diff --git a/requirements-dev.txt b/requirements-dev.txt index 40c4199..b8c159d 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -8,7 +8,7 @@ # ── Linting & formatting ──────────────────────────────────────────────────── ruff>=0.16.9,<1.0 -pre-commit>=4.6.2,<5.0 +pre-commit>=4.7.0,<5.0 # ── Tests ─────────────────────────────────────────────────────────────────── pytest>=9.1.1,<10.0 diff --git a/tests/test_file_transaction.py b/tests/test_file_transaction.py index ead3f46..1197a82 100644 --- a/tests/test_file_transaction.py +++ b/tests/test_file_transaction.py @@ -1,5 +1,7 @@ """File publication must preserve rollback bytes and report committed writes accurately.""" +import json +import os from pathlib import Path import pytest @@ -8,6 +10,108 @@ from mcp_server.file_transaction import StagedTextFile, staged_text_file +def _windows_denial(winerror): + """PermissionError as Windows raises it while another handle holds the path.""" + error = PermissionError(13, "Access is denied") + error.winerror = winerror + return error + + +def test_replace_waits_out_a_transient_windows_handle(tmp_path, monkeypatch): + """GH #249: scanners and readers briefly hold files without FILE_SHARE_DELETE.""" + source, target = tmp_path / "state.json.tmp", tmp_path / "state.json" + source.write_text("new", encoding="utf-8") + target.write_text("old", encoding="utf-8") + real_replace = os.replace + failures = [_windows_denial(5), _windows_denial(32)] + + def held_twice(src, dst): + if failures: + raise failures.pop(0) + real_replace(src, dst) + + monkeypatch.setattr(file_transaction.os, "replace", held_twice) + monkeypatch.setattr(file_transaction, "sleep", lambda _delay: None) + file_transaction.replace_with_retry(source, target) + + assert target.read_text(encoding="utf-8") == "new" + assert not source.exists() + + +@pytest.mark.parametrize("winerror", [5, None]) +def test_replace_reraises_persistent_or_non_windows_denial(tmp_path, monkeypatch, winerror): + denial = _windows_denial(winerror) + calls = [] + + def always_denied(src, dst): + calls.append(src) + raise denial + + monkeypatch.setattr(file_transaction.os, "replace", always_denied) + monkeypatch.setattr(file_transaction, "sleep", lambda _delay: None) + with pytest.raises(PermissionError) as caught: + file_transaction.replace_with_retry(tmp_path / "a", tmp_path / "b") + + assert caught.value is denial + assert len(calls) == (len(file_transaction._REPLACE_RETRY_DELAYS) + 1 if winerror else 1) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows sharing semantics") +def test_replace_succeeds_once_a_real_windows_handle_is_released(tmp_path, monkeypatch): + import ctypes + from ctypes import wintypes + + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.CreateFileW.restype = wintypes.HANDLE + kernel32.CloseHandle.argtypes = (wintypes.HANDLE,) + kernel32.CloseHandle.restype = wintypes.BOOL + source, target = tmp_path / "index_metadata.json.tmp", tmp_path / "index_metadata.json" + source.write_text("new", encoding="utf-8") + target.write_text("old", encoding="utf-8") + # GENERIC_READ shared for read/write but not delete, like a scanner or open(). + handle = kernel32.CreateFileW(str(target), 0x80000000, 0x1 | 0x2, None, 3, 0x80, None) + assert handle not in (None, wintypes.HANDLE(-1).value) + released = [] + + def release(_delay): + if not released: + assert kernel32.CloseHandle(handle), ctypes.get_last_error() + released.append(handle) + + try: + with pytest.raises(PermissionError): + os.replace(source, target) + monkeypatch.setattr(file_transaction, "sleep", release) + file_transaction.replace_with_retry(source, target) + finally: + release(0) + + assert target.read_text(encoding="utf-8") == "new" + + +def test_reindex_state_files_survive_a_transient_windows_handle(isolated_orchestrator, monkeypatch): + """GH #249: a held metadata or checkpoint file must not lose the write.""" + orch = isolated_orchestrator + orch._indexed_docs = {"doc": {"source": "a.md"}} + held = {orch._metadata_file.name, orch._checkpoint_file.name} + real_replace = os.replace + + def held_once(src, dst): + if Path(dst).name in held: + held.discard(Path(dst).name) + raise _windows_denial(5) + real_replace(src, dst) + + monkeypatch.setattr(file_transaction.os, "replace", held_once) + monkeypatch.setattr(file_transaction, "sleep", lambda _delay: None) + orch._save_metadata() + orch._write_checkpoint("smart_reindex", ["doc"], 3) + + assert not held + assert json.loads(orch._metadata_file.read_text(encoding="utf-8")) == orch._indexed_docs + assert json.loads(orch._checkpoint_file.read_text(encoding="utf-8"))["indexed_doc_ids"] == ["doc"] + + def test_committed_file_survives_temporary_backup_cleanup_failure(tmp_path, monkeypatch, caplog): destination = tmp_path / "document.md" destination.write_text("old content", encoding="utf-8") diff --git a/tests/test_lifecycle_concurrency.py b/tests/test_lifecycle_concurrency.py index 97f7cf1..0c484fc 100644 --- a/tests/test_lifecycle_concurrency.py +++ b/tests/test_lifecycle_concurrency.py @@ -142,6 +142,40 @@ def index(**kwargs): timer.start.assert_called_once() +@pytest.mark.parametrize( + ("mode", "status"), + [("incremental", "already_running"), ("smart_reindex", "already_running"), ("nuclear_rebuild", "started")], +) +def test_reindex_reports_already_running_while_the_index_lock_is_held(isolated_orchestrator, monkeypatch, mode, status): + """GH #249: index_all refuses while a watcher pass or document write holds the lock. + + Answering "started" hid the refusal; nuclear_rebuild waits for the lock, so it still starts. + """ + orch = isolated_orchestrator + held, release = threading.Event(), threading.Event() + + def watcher_pass(): + with orch._index_lock: + held.set() + release.wait(5) + + holder = threading.Thread(target=watcher_pass) + holder.start() + assert held.wait(5) + spawned = Mock() + monkeypatch.setattr(server.threading, "Thread", spawned) + try: + result = orch.start_reindex_background(mode) + finally: + release.set() + holder.join(5) + + assert result["status"] == status + assert spawned.called == (status == "started") + if status == "already_running": + assert orch._reindex_progress == {"active": False} + + def test_public_smart_reindex_routes_force_true(isolated_orchestrator, monkeypatch): orch = isolated_orchestrator reindex = Mock(return_value={}) diff --git a/tests/test_preflight.py b/tests/test_preflight.py index 702d111..e67c7e3 100644 --- a/tests/test_preflight.py +++ b/tests/test_preflight.py @@ -4,10 +4,60 @@ """ import subprocess +import sys +from pathlib import Path from unittest.mock import patch import pytest +ROOT = Path(__file__).resolve().parents[1] + + +def test_real_probe_reads_but_never_creates_the_collection(tmp_path, monkeypatch): + """GH #248/#252: the probe must read, never create, the collection. + + ``get_or_create_collection`` without an embedding function persisted + Chroma's ``default`` EF on every fresh data_dir, and the server then + refused to open it with FastEmbed ("Embedding function conflict ... + persisted: default"). Runs the real child so a broken probe script, + which would quarantine every index after 3 attempts, fails here first. + """ + from mcp_server import preflight + + base = tmp_path / "rag" + base.mkdir() + monkeypatch.setattr(preflight, "BASE_DIR", base) + monkeypatch.setenv("PYTHONPATH", str(ROOT)) + monkeypatch.setenv("HF_HUB_OFFLINE", "1") + + chroma_dir = str(base / "data" / "chroma_db") + seed = ( + "import sys, chromadb\n" + "class FastEmbedLike:\n" + " def __call__(self, input): return [[0.1, 0.2] for _ in input]\n" + " def name(self): return 'fastembed-test'\n" + "client = chromadb.PersistentClient(path=sys.argv[1])\n" + "if sys.argv[2] == 'production':\n" + " client.create_collection('knowledge_base', embedding_function=FastEmbedLike()).add(ids=['a'], documents=['a'])\n" + "else:\n" + " client.delete_collection('knowledge_base')\n" + " client.create_collection('knowledge_base')\n" + ) + + fresh = preflight._probe_chroma(timeout_seconds=120) + assert (fresh.returncode, fresh.stdout.strip()) == (0, "missing"), fresh.stderr + + # A healthy index as the server writes it; create_collection also fails if the + # probe above left a collection behind. + subprocess.run([sys.executable, "-c", seed, chroma_dir, "production"], check=True, timeout=120) + production = preflight._probe_chroma(timeout_seconds=120) + assert (production.returncode, production.stdout.strip()) == (0, "1"), production.stderr + + # What the old probe left on fresh installs: empty, bound to Chroma's default EF. + subprocess.run([sys.executable, "-c", seed, chroma_dir, "leftover"], check=True, timeout=120) + leftover = preflight._probe_chroma(timeout_seconds=120) + assert (leftover.returncode, leftover.stdout.strip()) == (0, "0"), leftover.stderr + def test_probe_uses_absolute_parent_base_even_with_relative_environment(tmp_path, monkeypatch): """The child cwd must not make a relative KNOWLEDGE_RAG_DIR resolve twice.""" diff --git a/tests/test_reindex_resume.py b/tests/test_reindex_resume.py index 05e7eef..b770375 100644 --- a/tests/test_reindex_resume.py +++ b/tests/test_reindex_resume.py @@ -21,6 +21,7 @@ from __future__ import annotations import json +import threading import time from datetime import datetime from pathlib import Path @@ -41,8 +42,10 @@ def _fresh_orchestrator(tmp_path: Path) -> KnowledgeOrchestrator: - _checkpoint_file pointing inside tmp_path - _reindex_progress dict - _indexed_docs empty (per-test setup can populate it) + - _index_lock, as __init__ always creates it """ orch = object.__new__(KnowledgeOrchestrator) + orch._index_lock = threading.RLock() orch._checkpoint_file = tmp_path / "reindex_checkpoint.json" orch._metadata_file = tmp_path / "index_metadata.json" orch._indexed_docs = {} diff --git a/tests/test_storage_safety.py b/tests/test_storage_safety.py index a0bda1f..26bcebd 100644 --- a/tests/test_storage_safety.py +++ b/tests/test_storage_safety.py @@ -2,6 +2,7 @@ import threading import time +import uuid from concurrent.futures import ThreadPoolExecutor from unittest.mock import Mock @@ -35,6 +36,54 @@ def test_collection_open_failure_preserves_database_and_collections(tmp_path, mo assert orchestrator.chroma_client.get_or_create_collection.call_count == 1 +class _FastEmbedLike: + """What Chroma sees of FastEmbedEmbeddings: callable plus name(), no config.""" + + def __call__(self, input): + return [[0.1, 0.2] for _ in input] + + @staticmethod + def name(): + return "fastembed-test" + + +@pytest.fixture +def default_ef_collection(monkeypatch): + """The collection as the pre-fix preflight probe left it: Chroma's default EF.""" + import chromadb + + monkeypatch.setattr(server.config, "collection_name", "test-" + uuid.uuid4().hex) + orchestrator = object.__new__(server.KnowledgeOrchestrator) + orchestrator.embed_fn = _FastEmbedLike() + orchestrator.chroma_client = chromadb.EphemeralClient() + stale = orchestrator.chroma_client.create_collection(server.config.collection_name) + yield orchestrator, stale + for collection in orchestrator.chroma_client.list_collections(): + if collection.name == server.config.collection_name: + orchestrator.chroma_client.delete_collection(collection.name) + + +def test_empty_default_ef_collection_is_recreated(default_ef_collection): + """GH #248/#252: installs that already hit the conflict start again after upgrading.""" + orchestrator, stale = default_ef_collection + + collection = orchestrator._safe_get_collection() + + assert collection.id != stale.id + assert orchestrator._safe_get_collection().id == collection.id + + +def test_default_ef_collection_with_data_is_never_dropped(default_ef_collection): + orchestrator, stale = default_ef_collection + stale.add(ids=["a"], embeddings=[[0.1, 0.2]]) + + with pytest.raises(RuntimeError, match="Existing index preserved"): + orchestrator._safe_get_collection() + + kept = orchestrator.chroma_client.get_collection(server.config.collection_name) + assert (kept.id, kept.count()) == (stale.id, 1) + + def test_bm25_retry_does_not_accumulate_duplicate_documents(monkeypatch): orchestrator = object.__new__(server.KnowledgeOrchestrator) orchestrator.collection = Mock() diff --git a/tests/test_tool_null_defaults.py b/tests/test_tool_null_defaults.py new file mode 100644 index 0000000..eb4128f --- /dev/null +++ b/tests/test_tool_null_defaults.py @@ -0,0 +1,33 @@ +"""Optional MCP tool arguments that default to None must accept an explicit null. + +Some MCP clients send ``"arg": null`` for an optional argument they leave unset. +A parameter annotated ``str = None`` advertises ``"default": null`` but its type +excludes null, so the SDK's argument model rejects the call before the tool runs. +""" + +import asyncio + + +def test_null_default_tool_args_accept_explicit_null(): + from mcp_server import server + + rejected = [] + checked = 0 + for tool in asyncio.run(server.mcp.list_tools()): + schema = tool.input_schema + props = schema.get("properties", {}) + required = schema.get("required", []) + arg_model = server.mcp._tool_manager.get_tool(tool.name).fn_metadata.arg_model + for name, prop in props.items(): + if "default" not in prop or prop["default"] is not None: + continue + checked += 1 + args = {r: "x" if props[r].get("type") == "string" else 1 for r in required} + args[name] = None + try: + arg_model.model_validate(args) + except Exception: + rejected.append(f"{tool.name}.{name}") + + assert checked >= 5 + assert rejected == []