fix(mcp): report an absent integrity verdict instead of a clean one (#2290)

sqlite_integrity_errors answers [] when chroma.sqlite3 is not there, and the
MCP startup gate published that as checked: true, ok: true. A clean bill of
health and an unopened database were the same answer, and /statusz built its
health flag from it.

os.path.exists cannot separate the two: it is genericpath.exists, which calls
os.stat inside a try whose except folds every OSError and ValueError into
False, and os.stat follows symlinks. ENOENT from lstat is now the only answer
accepted as proof of absence; everything else falls through to the open
attempt, where SQLite reports it. An absent database gets the not-applicable
shape #1931 introduced, checked: false / ok: null plus a reason.

ENOENT does not say which path component was missing, so it proves something
about the path and not about the palace: a palace directory that is itself a
dangling symlink, and an unmounted mount point, both report no verdict rather
than a failed probe. That is the answer develop gives too, without develop's
claim that the check passed.

Every state that is not proven absent reaches the probe. Measured against
develop, six palace states out of nine answered checked: true, ok: true: an
absent database, a palace directory that does not exist, an intact database
under a mode-000 directory, a dangling symlink, a symlink loop, and a path
whose parent is a file. The last four now report PRAGMA quick_check failed and
so trip the existing -32002 refusal, as do an over-long name and an embedded
NUL. A database file at mode 000 already reported ok: false, because
os.path.exists is true there and the probe ran.

The absence question is asked once per call. Reaching the probe through
sqlite_integrity_errors would ask it again, and a database unlinked between
the two would answer [] the second time, which is the clean verdict this
change exists to withhold. The quick_check body moved to _quick_check_errors,
which has no absence gate, and both callers use it.

/statusz reads an absent verdict as healthy rather than unhealthy. develop
already answered ok: true for a palace with no database, through the clean
verdict this removes, so the change is what keeps the new ok: null from
turning a fresh install red; the visible flip is the non-chroma backends,
unhealthy since the #1931 fix. The default in integrity.get("ok", False) keeps
a payload with no ok key failing closed.

SqliteIntegrityStatus carries its errors as a tuple. With a list field the
generated __hash__ raised TypeError on every call and a caller could append to
a verdict it had been handed, so frozen=True described nothing.

Windows raises ENOENT where POSIX raises ENOTDIR for a file used as a
directory component, so there the palace path is proven absent and reported as
no verdict rather than as a failed probe. Both answers are safe; the
difference belongs to the platform, not to the gate. The ENOTDIR test is
scoped to POSIX for that reason.

The refresh clears the no-verdict reason on every one of its five exits,
including the two that return before any probe runs, so a server pointed at a
new palace cannot keep naming the old one's database.

The payload still reads those globals one at a time without the lock, and the
comments there no longer claim otherwise: a refresh landing between two reads
can publish ok: true for a palace with no database, and snapshotting into
locals moves that window rather than closing it. Closing it means publishing
the verdict as one value, which is a change to what the gate stores.

Author: mvalentsev

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### Bug Fixes
 
+- **A palace with no database is no longer reported as one that passed its integrity check.** `sqlite_integrity_errors` answers `[]` when `chroma.sqlite3` is absent, and the MCP gate published that as `checked: true, ok: true`. Absence is now decided by `ENOENT` alone, which proves that nothing resolves under the path, and reported as the not-applicable shape #1931 introduced, `checked: false`/`ok: null` plus a reason. Every state that is not proven absent reaches the probe, and a probe that cannot open the file reports `PRAGMA quick_check failed`, which trips the existing `-32002` refusal: a dangling symlink, a database under an unreadable directory, a symlink loop, a name the filesystem rejects, an embedded NUL in the path, and, on POSIX, a palace path whose parent is a file. `/statusz` reads an absent verdict as healthy, so the new `ok: null` does not turn a fresh install red, and non-chroma backends stop reporting themselves unhealthy, which they had done since the #1931 fix. The size-limited startup skip still publishes a clean verdict; the only change there is that it no longer inherits the previous probe's absence reason. (#2290)
 - **`sweep` books a failed `stat` as a failure again.** The non-regular-file gate added in 3.7.1 reads `stat.S_ISREG(f.stat().st_mode)` inside a `try`, and its `except OSError` printed `SKIP` and moved on. A dangling symlink, a symlink loop and a file unlinked between `rglob` and the gate all raise there, and before the gate existed every one of them reached `sweep()` and was booked in `failures` — so `sweep` went from reporting a transcript it could not read to reporting success. A failed probe is now an error, not a benign file type: it is logged, printed as `WARNING`, and appended to `failures`, while a probe that succeeds and says "not regular" still skips silently. (#2221)
 - **`mempalace init` no longer tracebacks on a directory it cannot enter.** `_parse_gradle`'s `is_file()` gate sat in front of the `try` that the parser's own `except OSError` provides, so a manifest under a directory with `r` but no `x` raised `PermissionError` out of a call that used to answer "no manifest name". The gate moved inside that `try`, and `_collect_manifest_names` stats through `os.path.isfile`, which reports rather than raises. (#2221)
 - **`split` no longer blocks on a FIFO at its own output name, nor writes through a broken link.** The type gate in `main` covers the files the glob listed; `split_file` builds its output names itself, so a pre-existing named pipe at one of them wedged `write_text` in the kernel waiting for a reader. The check asks about the link itself rather than its target, because a dangling symlink at an output name reads as "nothing there" and `write_text` would create the target — landing a chunk outside the output directory. Output names that are anything but a regular file are now skipped with a `SKIP` line. (#2221)

mempalace/mcp_server.py

@@ -352,6 +352,13 @@ def _parse_args():
 _sqlite_integrity_checked = False
 _sqlite_integrity_errors: list[str] = []
 _sqlite_integrity_check_error = ""
+# Why no verdict exists, when none does. An empty _sqlite_integrity_errors is
+# ambiguous on its own: quick_check found nothing wrong, or it never ran. Four
+# exits in the refresh leave the list empty and only one of them means the
+# database came back clean. This names one of the others, the palace with no
+# chroma.sqlite3, so the status payload can report an absence rather than a
+# clean bill of health.
+_sqlite_integrity_no_verdict_reason = ""
 # Serializes quick_check runs between the async startup preflight thread and
 # lazy consumers on the protocol thread (double-checked in
 # _ensure_sqlite_integrity_status) so the O(database size) probe never runs
@@ -838,10 +845,11 @@ def _startup_integrity_size_limit_bytes() -> int:
 def _refresh_sqlite_integrity_status() -> None:
     """Refresh the MCP startup SQLite/FTS5 integrity gate.
 
-    Uses repair.sqlite_integrity_errors(), which is read-only and already backs
-    repair preflight. A failure here is treated as an integrity failure so the
-    server does not proceed silently after a malformed FTS5 index or other
-    SQLite-layer corruption (#1818).
+    Uses repair.sqlite_integrity_status(), which wraps the read-only
+    quick_check backing repair preflight and adds whether a verdict exists at
+    all. A failure here is treated as an integrity failure so the server does
+    not proceed silently after a malformed FTS5 index or other SQLite-layer
+    corruption (#1818).
     """
 
     with _sqlite_integrity_refresh_lock:
@@ -853,11 +861,13 @@ def _refresh_sqlite_integrity_status_locked() -> None:
     global _sqlite_integrity_checked
     global _sqlite_integrity_errors
     global _sqlite_integrity_check_error
+    global _sqlite_integrity_no_verdict_reason
 
     if not _config.palace_path or not _is_chroma_backend():
         _sqlite_integrity_checked = True
         _sqlite_integrity_errors = []
         _sqlite_integrity_check_error = ""
+        _sqlite_integrity_no_verdict_reason = ""
         return
 
     max_bytes = _startup_integrity_size_limit_bytes()
@@ -871,6 +881,12 @@ def _refresh_sqlite_integrity_status_locked() -> None:
             _sqlite_integrity_checked = True
             _sqlite_integrity_errors = []
             _sqlite_integrity_check_error = ""
+            # This exit is its own kind of "no verdict" and does not yet name
+            # itself (#2240). It must at least not inherit the previous
+            # probe's reason: a palace that had no database when the server
+            # started, and has an oversized one now, would otherwise be
+            # described as having no database at all.
+            _sqlite_integrity_no_verdict_reason = ""
             logger.warning(
                 "SQLite startup integrity check skipped: %s is %.0f MB "
                 "(> %.0f MB limit); PRAGMA quick_check would block MCP "
@@ -884,16 +900,35 @@ def _refresh_sqlite_integrity_status_locked() -> None:
             return
 
     try:
-        from .repair import sqlite_integrity_errors
+        from .repair import sqlite_integrity_status
 
-        errors = sqlite_integrity_errors(_config.palace_path)
+        status = sqlite_integrity_status(_config.palace_path)
     except Exception as exc:
         _sqlite_integrity_check_error = (
             f"sqlite integrity probe failed: {type(exc).__name__}: {exc}"
         )
         _sqlite_integrity_errors = [_sqlite_integrity_check_error]
+        _sqlite_integrity_no_verdict_reason = ""
     else:
-        _sqlite_integrity_errors = [str(error) for error in errors if str(error)]
+        fresh_errors = [str(error) for error in status.errors if str(error)]
+        # _sqlite_integrity_payload reads these globals without the lock, so
+        # the order within each branch is chosen to keep that branch's own
+        # window on the safer side: entering "no verdict" records the reason
+        # before the list it explains, and leaving it clears the reason only
+        # once the fresh errors are in place.
+        #
+        # No write order makes the payload safe, and this one does not claim
+        # to. That reader loads the error list more than once, so a refresh
+        # landing between two of its loads can still publish `ok: true` for a
+        # palace with no database. Closing that means publishing the verdict as
+        # one value, which is a change to what this gate stores rather than to
+        # the order it stores it in.
+        if status.checked:
+            _sqlite_integrity_errors = fresh_errors
+            _sqlite_integrity_no_verdict_reason = ""
+        else:
+            _sqlite_integrity_no_verdict_reason = status.reason
+            _sqlite_integrity_errors = fresh_errors
         _sqlite_integrity_check_error = ""
 
     _sqlite_integrity_checked = True
@@ -920,6 +955,17 @@ def _ensure_sqlite_integrity_status() -> None:
 def _sqlite_integrity_payload() -> dict:
     _ensure_sqlite_integrity_status()
 
+    # These globals are read one at a time, without the refresh lock, as they
+    # have always been, and a refresh landing between two of those reads can
+    # make this function publish a verdict the gate never held. Measured, both
+    # directions produce one: reading through, as here, can answer `ok: true`
+    # for a palace with no database; snapshotting into locals first can answer
+    # `ok: true` for a palace whose corruption the refresh had just found,
+    # because separate loads stay separate moments either way. A snapshot
+    # therefore trades one window for another of the same class rather than
+    # closing anything, which is why it is not taken here. Closing it means
+    # publishing the verdict as one value, a change to what the gate stores.
+    #
     # The integrity gate only knows how to check chroma.sqlite3, and
     # _refresh_sqlite_integrity_status short-circuits for non-chroma backends,
     # so on a non-chroma backend no quick_check runs. Reporting checked/ok true
@@ -947,6 +993,24 @@ def _sqlite_integrity_payload() -> dict:
                     f"{backend_name or 'unknown'!r}"
                 ),
             }
+        # Same shape, second way this payload reports an absent verdict: the
+        # backend is chroma, but there was no database to open. Reporting
+        # checked/ok true here would claim a quick_check that never ran. A
+        # palace whose file is unreachable rather than absent does not arrive
+        # here at all: its probe recorded an error, so it falls through to the
+        # verdict payload below with `ok: false`.
+        if _sqlite_integrity_no_verdict_reason:
+            return {
+                "checked": False,
+                "ok": None,
+                "palace": _config.palace_path or "",
+                "sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3")
+                if _config.palace_path
+                else "",
+                "error_count": 0,
+                "errors": [],
+                "reason": _sqlite_integrity_no_verdict_reason,
+            }
 
     payload = {
         "checked": _sqlite_integrity_checked,
@@ -6979,7 +7043,12 @@ def _http_status_payload(httpd) -> dict:
         os.path.abspath(os.path.expanduser(_config.palace_path)) if _config.palace_path else ""
     )
     return {
-        "ok": bool(integrity.get("ok")),
+        # `ok` is None in the two cases the payload reports as an absent
+        # verdict: a non-chroma backend (#1931), and a chroma palace with no
+        # database file yet. That is an absence, not a failure, and collapsing
+        # it with bool() would report a freshly installed server as unhealthy.
+        # A missing key is not one of those cases and still fails closed.
+        "ok": integrity.get("ok", False) is not False,
         "server": {
             "name": "mempalace",
             "version": __version__,

mempalace/repair.py

@@ -38,6 +38,7 @@
 import time
 from collections import defaultdict
 from contextlib import closing
+from dataclasses import dataclass
 from datetime import datetime
 import re
 from typing import Callable, Iterator, Optional
@@ -732,23 +733,71 @@ def sqlite_drawer_count(palace_path: str, collection_name: Optional[str] = None)
 _SQLITE_INTEGRITY_BUSY_TIMEOUT_SECONDS = 15.0
 
 
-def sqlite_integrity_errors(palace_path: str) -> list[str]:
-    """Return SQLite quick_check errors for chroma.sqlite3.
+@dataclass(frozen=True)
+class SqliteIntegrityStatus:
+    """Whether a quick_check verdict exists for a palace, and what it says.
 
-    The repair rebuild path eventually calls Chroma's delete_collection().
-    If the SQLite layer has corrupt secondary indexes or FTS5 shadow pages,
-    Chroma can raise an opaque SQLITE_CORRUPT_INDEX / code 779 error before
-    repair reaches the HNSW rebuild.
+    ``checked`` False means no probe ran, so an empty ``errors`` says nothing
+    about the database. That is the distinction :func:`sqlite_integrity_errors`
+    cannot express: it answers ``[]`` both for a database quick_check found
+    intact and for a palace that has none to open.
 
-    Run a direct SQLite quick_check first so repair can fail with a clear,
-    actionable message before invoking Chroma's destructive collection-delete
-    path.
+    ``errors`` is a tuple rather than a list so ``frozen=True`` means what it
+    says: a list field would leave the generated ``__hash__`` raising
+    ``TypeError`` on every call, and would let a caller append to a verdict it
+    was handed.
     """
 
-    sqlite_path = os.path.join(palace_path, "chroma.sqlite3")
-    if not os.path.exists(sqlite_path):
-        return []
-
+    checked: bool
+    errors: tuple[str, ...]
+    reason: str
+
+
+def _integrity_target_is_absent(sqlite_path: str) -> bool:
+    """Return True only when nothing resolves under ``sqlite_path``.
+
+    ``ENOENT`` is the one errno this accepts as proof, because it cannot mean
+    anything else: no file answered to that path. It does not say which
+    component was missing, so it is proof about the path and not about the
+    palace. A palace directory that is itself a dangling symlink, and a mount
+    point with nothing mounted on it, both reach here as ``ENOENT`` and are
+    reported as no verdict. That is the same answer ``develop`` gives, only
+    without ``develop``'s claim that the check passed.
+
+    ``ENOTDIR`` would be proof too and is deliberately left to the open
+    attempt, which reports it rather than silently treating it as nothing to
+    do. Windows raises ``ENOENT`` for that case, so it is proven absent there;
+    both answers are safe. Every other failure, a parent directory this process
+    may not enter for one, leaves the question open and takes the same route.
+    False therefore means "not proven absent", not "the file is there".
+
+    ``os.path.exists`` cannot draw that line: it follows symlinks and folds
+    every ``OSError`` into False, so a dangling link and an unreadable
+    directory both read as "no database here". ``ValueError`` (an embedded NUL
+    in the path) is caught for the same reason as any ``OSError``: it does not
+    prove absence either, and callers that never handled it still never see
+    it. What they do see changes, as it does for the other unreadable paths:
+    the open attempt reports the failure instead of returning nothing.
+    """
+    try:
+        os.lstat(sqlite_path)
+    except FileNotFoundError:
+        return True
+    except (OSError, ValueError):
+        return False
+    return False
+
+
+def _quick_check_errors(sqlite_path: str) -> list[str]:
+    """Run ``PRAGMA quick_check`` against ``sqlite_path`` and report what it says.
+
+    There is no absence gate here on purpose. A caller that has already
+    established the file's state passes the path straight through, so the answer
+    always comes from an open attempt. Re-testing for absence would reopen the
+    hole this module exists to close: a file that vanishes between the two tests
+    would come back as an empty error list, which reads as a clean verdict for a
+    database nobody opened.
+    """
     try:
         # A writer holding SQLite's lock is contention, not corruption. The
         # sqlite3 module defaults to five seconds, which is shorter than
@@ -775,6 +824,65 @@ def sqlite_integrity_errors(palace_path: str) -> list[str]:
     return errors
 
 
+def sqlite_integrity_status(palace_path: str) -> SqliteIntegrityStatus:
+    """Run the quick_check probe and report whether it produced a verdict.
+
+    Callers that state an integrity result to an operator want this rather
+    than :func:`sqlite_integrity_errors`, whose empty list cannot separate a
+    clean database from one that was never opened.
+    """
+    sqlite_path = os.path.join(palace_path, "chroma.sqlite3")
+    if _integrity_target_is_absent(sqlite_path):
+        return SqliteIntegrityStatus(
+            checked=False,
+            errors=(),
+            reason=(
+                f"no quick_check ran: {sqlite_path} does not exist, so there "
+                "was no SQLite database to open"
+            ),
+        )
+    # _quick_check_errors, not sqlite_integrity_errors: that one gates on
+    # absence again, and a file unlinked between the two gates would come back
+    # as an empty list, which is the clean verdict this function exists to
+    # withhold. Past the gate above, only an open attempt may answer.
+    return SqliteIntegrityStatus(
+        checked=True,
+        errors=tuple(_quick_check_errors(sqlite_path)),
+        reason="",
+    )
+
+
+def sqlite_integrity_errors(palace_path: str) -> list[str]:
+    """Return SQLite quick_check errors for chroma.sqlite3.
+
+    The repair rebuild path eventually calls Chroma's delete_collection().
+    If the SQLite layer has corrupt secondary indexes or FTS5 shadow pages,
+    Chroma can raise an opaque SQLITE_CORRUPT_INDEX / code 779 error before
+    repair reaches the HNSW rebuild.
+
+    Run a direct SQLite quick_check first so repair can fail with a clear,
+    actionable message before invoking Chroma's destructive collection-delete
+    path.
+
+    An empty list means one of two things and cannot tell them apart: the
+    check ran and found nothing, or the palace provably has no database to
+    check. Callers stating a result to an operator want
+    :func:`sqlite_integrity_status` instead. A path that resolves to a
+    directory entry but cannot be opened — a dangling symlink, a database
+    under a directory this process may not enter — is reported here as an
+    error, since the probe did fail.
+    """
+
+    sqlite_path = os.path.join(palace_path, "chroma.sqlite3")
+    # Absence is the one state with nothing to report; anything else that
+    # cannot be read is reported by the open attempt below. Callers that need
+    # to tell an absent database from a clean one use sqlite_integrity_status.
+    if _integrity_target_is_absent(sqlite_path):
+        return []
+
+    return _quick_check_errors(sqlite_path)
+
+
 def print_sqlite_integrity_abort(palace_path: str, errors: list[str]) -> None:
     """Print a clear repair abort message for SQLite-layer corruption."""