Skip to content

Commit e00d503

Browse files
committed
fix(repair): anchor the FTS5 classifier, attribute integrity verdicts (#2240)
maybe_autoheal_fts5_index rebuilds a corrupt FTS5 index in place when PRAGMA quick_check's errors look like they came from that index alone. On develop it has six exits, two of them take that decision silently, and no verdict anywhere says which SQLite produced it -- or whether one was produced at all. Anchor both known phrasings to the start of the quick_check row. sqlite_integrity_errors reports a failed probe as `PRAGMA quick_check failed: <error>`, and an unanchored match reads such a row as an isolated FTS5 fault whenever the wrapped text carries one of the phrasings, authorizing a write on a database whose state was never established. That is not a cosmetic misread: on develop, handing such a row to maybe_autoheal_fts5_index takes the mine lock and changes chroma.sqlite3 on disk. Producing a matching wrapped row needs a table named after a phrasing, which a chroma palace never has, so this is hardening rather than a reachable fix. Across 3,200 deliberately damaged databases per build on 3.45.1, 3.47.1 and 3.51.2, 2,975 dirty on each, the anchored pattern refused no row the old one accepted and accepted none it refused; FTS5 faults arrived as whole rows, and the `*** in database <db> ***` header appeared on b-tree rows only. Explain a declined heal instead of returning silently, on both quiet exits and in --dry-run, and name the SQLite that decided. The message reports what the classifier concluded rather than guessing a cause, since quick_check may have failed to run at all, reported damage outside the index, or reported an FTS5 fault in an unrecognized wording. The --dry-run copy is marked as a preview: without the marker it was byte-for-byte the run it was predicting, which is the one thing a preview must not be. The shared text no longer opens with a blank line either, because the post-mine path passes logger.warning as progress, where a leading newline renders as a bare WARNING header above an empty line. Carry sqlite3.sqlite_version wherever a quick_check result is stated: the abort banner, both --dry-run branches, the heal's outcome line, the post-recovery check, all three shapes of the MCP integrity payload, the tool-refusal payload, the integrity log line and MineValidationError's message. A clean result from a build that cannot detect a given FTS5 fault reads exactly like one from a build that can. The exception message matters because two callers render it rather than the banner: the MCP mine tool and the daemon's job runner. Stop the MCP integrity payload reporting a verdict nobody produced. Above MEMPALACE_STARTUP_INTEGRITY_MAX_MB, 512 MB by default, the startup probe does not run, yet the payload answered checked: true, ok: true. It now returns the not-applicable shape #1931 introduced for the same reason, carrying the skip as its reason. This matters here specifically: the palace in #2240 is about four times that limit, so on that install no quick_check runs at all. The reason is recorded before the rest of the state, so a reader taking the payload without the refresh lock cannot catch a half-updated skip and read it as clean. That reason also states sizes precisely enough to show the comparison it reports: rounded to whole megabytes, a 1.4 MB database against a 1 MB limit read as "is 1 MB, over the 1 MB limit". /statusz no longer collapses a missing verdict into a failure either: false there now means a check came back dirty, which also stops a non-chroma backend from reporting itself unhealthy. Report the post-mine validator at warning rather than info. Nothing configures logging on the mempalace mine path, so its info records were dropped before reaching a handler there; the MCP server, which configures logging at info on import, was already showing them. `fts5: checksum mismatch`, which SQLite 3.51.2 emits, stays unmatched on purpose. It says the index and the content table disagree without naming the damaged side, and a rebuild reads from content: edited bytes inside %_content produce it while the index still holds the original token, and single-byte damage to an index leaf produces it too, in 75 of the 115 flips that registered at all in a 120-trial run. A rebuild then leaves quick_check clean whichever side was damaged, on all three builds. A comment and a test record the counterexample so the pattern is not widened again. The subject of #2240 -- one SQLite calling a file clean while another calls it malformed -- is not addressed here, and the issue stays open. That asymmetry did not reproduce: 3.45.1, 3.47.1 and 3.51.2 all called the same 2,975 shapes dirty, differing only in wording, and quick_check agreed with FTS5's own integrity-check 24 times out of 24 on each build. Reported by @darkrhodes7, whose suggestion to log the SQLite version alongside integrity verdicts is implemented here.
1 parent 3e56979 commit e00d503

8 files changed

Lines changed: 1017 additions & 36 deletions

File tree

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
88

99
## [Unreleased]
1010

11+
### Improvements
12+
13+
- **`repair` says when it declines an in-place FTS5 heal, and names the SQLite that decided.** Declining was a silent `return` on two paths, so the operator saw the ABORT banner with no sign that a rebuild had been considered; `--dry-run` withheld the same explanation. All of them now print it, and the `--dry-run` copy is marked as a preview so it cannot be read as the run it is predicting. `sqlite3.sqlite_version` accompanies every verdict these paths state — the abort banner, both `--dry-run` branches, the heal's outcome line, the post-recovery check, all three shapes of the MCP integrity payload, the tool-refusal payload, the integrity log line and `MineValidationError`'s message — because a clean result from a build that cannot detect a given FTS5 fault reads exactly like one from a build that can. The post-mine validator reports at `warning` rather than `info`, the level that survives on the `mempalace mine` path, which configures no logging. (#2240)
14+
- **A skipped startup integrity probe is no longer reported as a clean verdict.** Above `MEMPALACE_STARTUP_INTEGRITY_MAX_MB` no `quick_check` runs, yet the MCP payload answered `checked: true, ok: true`. It now returns the not-applicable shape with the skip as its `reason`, and that reason states sizes precisely enough to show the comparison it reports — a 1.4 MB database against a 1 MB limit used to read as "is 1 MB, over the 1 MB limit". Write tools gate on recorded errors rather than on `ok`, so nothing is newly refused, and `/statusz` no longer collapses a missing verdict into a failure — `ok` is false only when a check came back dirty, which also stops a non-chroma backend from reporting itself unhealthy (#1931). (#2240)
15+
- **The FTS5 classifier is anchored to the start of the `quick_check` row.** `sqlite_integrity_errors` reports a failed probe as `PRAGMA quick_check failed: <error>`, and an unanchored match read such a row as an isolated FTS5 fault — authorizing a rebuild on a database whose state was never established. `fts5: checksum mismatch`, which SQLite 3.51.2 emits, stays unmatched deliberately: it does not name which side is damaged, and a rebuild reads from the content table. (#2240)
16+
1117
---
1218

1319
## [3.8.0] — 2026-08-20

mempalace/mcp_server.py

Lines changed: 99 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,9 @@ def _parse_args():
352352
_sqlite_integrity_checked = False
353353
_sqlite_integrity_errors: list[str] = []
354354
_sqlite_integrity_check_error = ""
355+
# Set when the startup probe was deliberately not run, so the payload can say a
356+
# verdict is absent rather than report a clean one nobody produced (#2240).
357+
_sqlite_integrity_skipped_reason = ""
355358
# Serializes quick_check runs between the async startup preflight thread and
356359
# lazy consumers on the protocol thread (double-checked in
357360
# _ensure_sqlite_integrity_status) so the O(database size) probe never runs
@@ -834,6 +837,31 @@ def _startup_integrity_size_limit_bytes() -> int:
834837
return int(mb * 1024 * 1024)
835838

836839

840+
def _rendered_mb(*byte_counts: float) -> tuple[str, ...]:
841+
"""Render byte counts as MB strings that stay distinguishable from each other.
842+
843+
Whole megabytes turned a 1.4 MB database against a 1 MB limit into "is 1 MB,
844+
over the 1 MB limit", and a sub-megabyte limit into "0 MB, over the 0 MB
845+
limit" — a sentence that reads as a contradiction and hides the very
846+
comparison it is reporting. The limit is read from the environment as a
847+
float, so neither end is guaranteed to be a round number. Widening the
848+
precision until the rendered values differ keeps the common case ("1954 MB,
849+
over the 512 MB limit") free of noise digits.
850+
"""
851+
values = [count / (1024 * 1024) for count in byte_counts]
852+
for digits in range(4):
853+
rendered = tuple(f"{value:.{digits}f}" for value in values)
854+
# Distinct is not enough on its own: 0.6 MB over a 0.5 MB limit renders
855+
# as "1" over "0" at zero digits, which is distinct and wrong in both
856+
# numbers. A value that survives rounding to nothing has been rounded
857+
# past the point of describing itself.
858+
if len(set(rendered)) == len(rendered) and not any(
859+
float(text) == 0 and value > 0 for text, value in zip(rendered, values)
860+
):
861+
return rendered
862+
return tuple(f"{value:.3f}" for value in values)
863+
864+
837865
def _refresh_sqlite_integrity_status() -> None:
838866
"""Refresh the MCP startup SQLite/FTS5 integrity gate.
839867
@@ -852,11 +880,25 @@ def _refresh_sqlite_integrity_status_locked() -> None:
852880
global _sqlite_integrity_checked
853881
global _sqlite_integrity_errors
854882
global _sqlite_integrity_check_error
855-
883+
global _sqlite_integrity_skipped_reason
884+
885+
# Deliberately not cleared up front. _sqlite_integrity_payload reads these
886+
# globals without the lock, and _ensure_sqlite_integrity_status short-
887+
# circuits on _sqlite_integrity_checked, which a previous skip already set
888+
# to True. Clearing here and re-populating after os.path.getsize would open
889+
# a window in which a reader sees checked=True, errors=[] and no reason —
890+
# the clean verdict nobody produced that this whole path exists to prevent.
891+
# Each exit below sets it instead, and the order within an exit is chosen
892+
# so every window falls on the safe side. Entering the skip records the
893+
# reason before the other three, so a reader cannot catch an empty error
894+
# list that nothing explains. Leaving it clears the reason only once the
895+
# fresh errors are in place, so the reader catching that half sees a stale
896+
# "no verdict" rather than a clean one it was never entitled to.
856897
if not _config.palace_path or not _is_chroma_backend():
857898
_sqlite_integrity_checked = True
858899
_sqlite_integrity_errors = []
859900
_sqlite_integrity_check_error = ""
901+
_sqlite_integrity_skipped_reason = ""
860902
return
861903

862904
max_bytes = _startup_integrity_size_limit_bytes()
@@ -867,19 +909,29 @@ def _refresh_sqlite_integrity_status_locked() -> None:
867909
except OSError:
868910
db_bytes = 0
869911
if db_bytes > max_bytes:
870-
_sqlite_integrity_checked = True
871-
_sqlite_integrity_errors = []
872-
_sqlite_integrity_check_error = ""
912+
# Reason first: it is the field that distinguishes this state from a
913+
# clean verdict, so a lock-free reader must never catch the other
914+
# three updated while it still says nothing was skipped.
915+
db_mb, limit_mb = _rendered_mb(db_bytes, max_bytes)
916+
_sqlite_integrity_skipped_reason = (
917+
f"startup integrity check skipped: {sqlite_path} is "
918+
f"{db_mb} MB, over the {limit_mb} MB limit "
919+
f"({_STARTUP_INTEGRITY_MAX_MB_ENV}); no quick_check has run "
920+
"against this palace. Run `mempalace repair` for a full check."
921+
)
873922
logger.warning(
874-
"SQLite startup integrity check skipped: %s is %.0f MB "
875-
"(> %.0f MB limit); PRAGMA quick_check would block MCP "
923+
"SQLite startup integrity check skipped: %s is %s MB "
924+
"(> %s MB limit); PRAGMA quick_check would block MCP "
876925
"startup. Run `mempalace repair` for a full check, or set "
877926
"%s (MB; 0 disables the limit).",
878927
sqlite_path,
879-
db_bytes / (1024 * 1024),
880-
max_bytes / (1024 * 1024),
928+
db_mb,
929+
limit_mb,
881930
_STARTUP_INTEGRITY_MAX_MB_ENV,
882931
)
932+
_sqlite_integrity_checked = True
933+
_sqlite_integrity_errors = []
934+
_sqlite_integrity_check_error = ""
883935
return
884936

885937
try:
@@ -895,12 +947,16 @@ def _refresh_sqlite_integrity_status_locked() -> None:
895947
_sqlite_integrity_errors = [str(error) for error in errors if str(error)]
896948
_sqlite_integrity_check_error = ""
897949

950+
# A probe ran, so whatever an earlier skip recorded no longer describes
951+
# this palace.
952+
_sqlite_integrity_skipped_reason = ""
898953
_sqlite_integrity_checked = True
899954

900955
if _sqlite_integrity_errors:
901956
logger.error(
902-
"SQLite integrity check failed for palace=%s: %s",
957+
"SQLite integrity check failed for palace=%s (SQLite %s): %s",
903958
_config.palace_path,
959+
sqlite3.sqlite_version,
904960
"; ".join(_sqlite_integrity_errors[:3]),
905961
)
906962

@@ -928,6 +984,24 @@ def _sqlite_integrity_payload() -> dict:
928984
# backend lookup (which may itself fail); only the clean case is
929985
# reclassified as not-applicable.
930986
if not _sqlite_integrity_errors:
987+
# Same rule, second way to have no verdict: the startup probe is
988+
# skipped above a size limit, and the palace in #2240 is roughly four
989+
# times the default. Reporting checked/ok true there says a build
990+
# examined the database and found it clean when none ever opened it.
991+
if _sqlite_integrity_skipped_reason:
992+
return {
993+
"checked": False,
994+
"ok": None,
995+
"sqlite_version": sqlite3.sqlite_version,
996+
"palace": _config.palace_path or "",
997+
"sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3")
998+
if _config.palace_path
999+
else "",
1000+
"error_count": 0,
1001+
"errors": [],
1002+
"reason": _sqlite_integrity_skipped_reason,
1003+
}
1004+
9311005
try:
9321006
backend_name = _selected_backend_name()
9331007
except Exception:
@@ -937,6 +1011,9 @@ def _sqlite_integrity_payload() -> dict:
9371011
return {
9381012
"checked": False,
9391013
"ok": None,
1014+
# A property of this process, not of the backend, and every
1015+
# payload shape is deliberately kept parallel on it.
1016+
"sqlite_version": sqlite3.sqlite_version,
9401017
"palace": _config.palace_path or "",
9411018
"sqlite_path": "",
9421019
"error_count": 0,
@@ -950,6 +1027,10 @@ def _sqlite_integrity_payload() -> dict:
9501027
payload = {
9511028
"checked": _sqlite_integrity_checked,
9521029
"ok": not _sqlite_integrity_errors,
1030+
# Which SQLite produced this verdict. An "ok" from a build that cannot
1031+
# detect a given FTS5 fault is not the same claim as an "ok" from one
1032+
# that can, and the two are otherwise indistinguishable here (#2240).
1033+
"sqlite_version": sqlite3.sqlite_version,
9531034
"palace": _config.palace_path,
9541035
"sqlite_path": os.path.join(_config.palace_path, "chroma.sqlite3")
9551036
if _config.palace_path
@@ -995,6 +1076,9 @@ def _mcp_sqlite_integrity_refusal(req_id, tool_name: str):
9951076
),
9961077
"errors": _sqlite_integrity_errors[:10],
9971078
"error_count": len(_sqlite_integrity_errors),
1079+
# This is the payload an operator is most likely to paste into
1080+
# a report, so it carries the build behind the verdict too.
1081+
"sqlite_version": sqlite3.sqlite_version,
9981082
"hint": (
9991083
"Stop all MemPalace MCP clients/writers, back up the palace, "
10001084
"repair the SQLite/FTS5 corruption offline, then run "
@@ -7089,7 +7173,12 @@ def _http_status_payload(httpd) -> dict:
70897173
os.path.abspath(os.path.expanduser(_config.palace_path)) if _config.palace_path else ""
70907174
)
70917175
return {
7092-
"ok": bool(integrity.get("ok")),
7176+
# `ok` is None when no integrity verdict exists — a non-chroma backend
7177+
# (#1931) or a palace above the startup-probe size limit. That is an
7178+
# absence, not a failure, and collapsing it with bool() reported every
7179+
# such server as unhealthy. Only a verdict that actually came back
7180+
# dirty belongs here (#2240).
7181+
"ok": integrity.get("ok") is not False,
70937182
"server": {
70947183
"name": "mempalace",
70957184
"version": __version__,

mempalace/palace.py

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import logging
1010
import os
1111
import re
12+
import sqlite3
1213
import sys
1314
import threading
1415
import time
@@ -1107,7 +1108,16 @@ def __init__(self, palace_path: str, errors: list[str]) -> None:
11071108
raise ValueError("MineValidationError requires at least one error string")
11081109
if not palace_path:
11091110
raise ValueError("MineValidationError requires a non-empty palace_path")
1110-
super().__init__(f"FTS5/SQLite quick_check failed: {len(errors)} issue(s)")
1111+
# Name the SQLite that produced the verdict. #2240 points at this
1112+
# post-mine check by name: a build that cannot detect a given FTS5
1113+
# fault reports the same "clean" as one that can. The CLI handler
1114+
# renders the abort banner, which carries the version; the MCP `mine`
1115+
# tool and the daemon's job runner render this message instead, so it
1116+
# belongs in the message.
1117+
super().__init__(
1118+
f"FTS5/SQLite quick_check failed: {len(errors)} issue(s) "
1119+
f"(SQLite {sqlite3.sqlite_version})"
1120+
)
11111121
self.palace_path = palace_path
11121122
# Freeze the forensic snapshot so handlers cannot mutate it.
11131123
self.errors: tuple[str, ...] = tuple(errors)
@@ -1144,11 +1154,19 @@ def _validate_palace_fts5_after_mine(palace_path: str) -> None:
11441154

11451155
errors = sqlite_integrity_errors(palace_path)
11461156
if errors:
1147-
# progress=logger.info, not the default print: this runs inside the
1148-
# MCP server process too (mcp_server.tool_mine -> miner.mine), where
1149-
# stdout is the JSON-RPC transport -- a stray print() here would
1150-
# corrupt the protocol stream and crash the connection.
1151-
errors = maybe_autoheal_fts5_index(palace_path, errors, progress=logger.info)
1157+
# Not the default print: this runs inside the MCP server process too
1158+
# (mcp_server.tool_mine -> miner.mine), where stdout is the JSON-RPC
1159+
# transport -- a stray print() here would corrupt the protocol stream
1160+
# and crash the connection.
1161+
#
1162+
# warning, not info: this module logs through the `mempalace_mcp`
1163+
# logger, which sets no level of its own, and nothing configures logging
1164+
# on the `mempalace mine` path -- so root keeps its default and info
1165+
# records are dropped. Every message this call can emit describes a palace whose
1166+
# quick_check already failed -- a rebuild being attempted, refused, or
1167+
# completed against the operator's data -- so warning is both the level
1168+
# that survives and the level that fits.
1169+
errors = maybe_autoheal_fts5_index(palace_path, errors, progress=logger.warning)
11521170
if errors:
11531171
raise MineValidationError(palace_path, errors)
11541172

0 commit comments

Comments
 (0)