Skip to content

Commit eef798b

Browse files
committed
fix(store): distinguish transient SQLITE_BUSY from corruption before quarantine (DeusData#1206, DeusData#1037)
The quarantine path in resolve_store_internal() used the shallow cbm_store_check_integrity() to decide whether to rename a project DB to .corrupt and rebuild it. That check has two flaws that together cause healthy databases to be destroyed under concurrent access: 1. It treats ANY sqlite3_prepare_v2 failure as corruption. Under concurrent MCP instances (DeusData#1206), a writer lock held by another instance makes the schema lookup return SQLITE_BUSY/LOCKED — a transient condition, not damage. The losing instance quarantines a perfectly healthy DB and rebuilds a sparse replacement. 2. It never runs PRAGMA quick_check, so it cannot catch real page-level corruption either. A DB with a torn node/edge btree but an intact projects table passes the check and keeps being served (DeusData#1037) — the inverse failure. This change introduces cbm_store_check_integrity_verdict(), which returns a tri-state (OK / CORRUPT / TRANSIENT) and runs the shallow check plus PRAGMA quick_check, classifying BUSY/LOCKED/IO-lock codes as TRANSIENT. The quarantine decision in mcp.c now quarantines ONLY on a confirmed CORRUPT verdict; TRANSIENT releases the mutation lease and retries on the next resolve, never touching the DB file. The existing cbm_store_check_integrity() and _deep() are unchanged, so all other call sites (hot opens, artifact import, maintenance) keep their current behavior. Verified: builds clean with -Wall -Wextra -Werror; cli list_projects and index_status run correctly against the patched binary. Signed-off-by: LynxBay <187708458+LynxBay@users.noreply.github.com>
1 parent 58d6100 commit eef798b

3 files changed

Lines changed: 150 additions & 2 deletions

File tree

‎src/mcp/mcp.c‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2156,9 +2156,34 @@ static cbm_store_t *resolve_store_internal(cbm_mcp_server_t *srv, const char *pr
21562156
}
21572157

21582158
/* The lease may have waited behind a publisher. Re-open and trust
2159-
* only the current generation, never the stale pre-wait verdict. */
2159+
* only the current generation, never the stale pre-wait verdict.
2160+
* Use the verdict API here — this is the point that decides whether
2161+
* a healthy DB gets quarantined. The plain bool check cannot tell
2162+
* corruption from a transient SQLITE_BUSY race (#1206: concurrent
2163+
* instances quarantining each other's DBs) and does not run
2164+
* quick_check, so page-torn DBs with an intact projects table sail
2165+
* through (#1037). Only a confirmed CORRUPT verdict is quarantined;
2166+
* TRANSIENT (lock/IO) falls through and retries on next access. */
21602167
srv->store = cbm_store_open_path_query(path);
2161-
bool current_valid = srv->store && cbm_store_check_integrity(srv->store);
2168+
cbm_integrity_verdict_t verdict = srv->store
2169+
? cbm_store_check_integrity_verdict(srv->store)
2170+
: CBM_INTEGRITY_TRANSIENT;
2171+
bool current_valid = (verdict == CBM_INTEGRITY_OK);
2172+
if (verdict == CBM_INTEGRITY_TRANSIENT) {
2173+
/* The DB could not be conclusively evaluated (lock contention,
2174+
* busy writer, IO hiccup). Do NOT quarantine — close and let
2175+
* the next resolve retry. A spurious quarantine here is exactly
2176+
* what destroys healthy DBs under concurrent access. */
2177+
cbm_store_close(srv->store);
2178+
srv->store = NULL;
2179+
if (recovery_status) {
2180+
*recovery_status = STORE_RECOVERY_BUSY;
2181+
}
2182+
if (!mutation_already_held) {
2183+
mcp_project_mutation_end(srv, project);
2184+
}
2185+
return NULL;
2186+
}
21622187
if (!current_valid) {
21632188
cbm_store_close(srv->store);
21642189
srv->store = NULL;

‎src/store/store.c‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,6 +948,107 @@ bool cbm_store_check_integrity_deep(cbm_store_t *s) {
948948
return ok;
949949
}
950950

951+
/* Classify a SQLite result code as a transient (retryable) condition vs. a
952+
* hard error. SQLITE_BUSY / SQLITE_LOCKED happen when another connection holds
953+
* the writer lock — they are NOT evidence of corruption, yet the bare
954+
* cbm_store_check_integrity() treats any prepare failure as "corrupt". This
955+
* helper backs the verdict API so the quarantine path stops destroying healthy
956+
* DBs that merely lost a lock race (#1206, #1037). */
957+
static bool st_rc_is_transient(int rc) {
958+
switch (rc) {
959+
case SQLITE_BUSY:
960+
case SQLITE_LOCKED:
961+
case SQLITE_IOERR_LOCK:
962+
case SQLITE_IOERR_BLOCKED:
963+
return true;
964+
default:
965+
return false;
966+
}
967+
}
968+
969+
cbm_integrity_verdict_t cbm_store_check_integrity_verdict(cbm_store_t *s) {
970+
if (!s || !s->db) {
971+
/* No handle at all — the caller could not even open the file. That is
972+
* an open problem, not a corruption verdict, so the DB must not be
973+
* quarantined (renaming it would discard a file we never read). */
974+
return CBM_INTEGRITY_TRANSIENT;
975+
}
976+
977+
/* ── Shallow check (projects table shape + root_path sanity) ── */
978+
sqlite3_stmt *stmt = NULL;
979+
int rc =
980+
sqlite3_prepare_v2(s->db, "SELECT count(*) FROM projects;", CBM_NOT_FOUND, &stmt, NULL);
981+
if (rc != SQLITE_OK) {
982+
/* A prepare failure here is the #1206 trigger: under concurrent access
983+
* the schema lookup can return BUSY/LOCKED. Treat transient codes as
984+
* "do not quarantine"; only a hard error is corruption evidence. */
985+
return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT;
986+
}
987+
cbm_integrity_verdict_t verdict = CBM_INTEGRITY_OK;
988+
if (sqlite3_step(stmt) == SQLITE_ROW) {
989+
int row_count = sqlite3_column_int(stmt, 0);
990+
if (row_count > ST_MAX_ROW_CHECK) {
991+
(void)fprintf(stderr, "ERROR store.corrupt table=projects rows=%d (expected 1)\n",
992+
row_count);
993+
verdict = CBM_INTEGRITY_CORRUPT;
994+
}
995+
} else {
996+
/* step returned DONE/ERROR — for a SELECT count(*) this means the table
997+
* is unreadable. BUSY-family => transient; otherwise corruption. */
998+
int step_rc = sqlite3_errcode(s->db);
999+
verdict = st_rc_is_transient(step_rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT;
1000+
}
1001+
sqlite3_finalize(stmt);
1002+
if (verdict != CBM_INTEGRITY_OK) {
1003+
return verdict;
1004+
}
1005+
1006+
/* root_path sanity — same logic as cbm_store_check_integrity, but classify
1007+
* prepare failures as transient vs. corrupt rather than a blanket false. */
1008+
rc = sqlite3_prepare_v2(s->db,
1009+
"SELECT root_path FROM projects WHERE root_path != '' "
1010+
"AND NOT (substr(root_path, 1, 1) = '/' "
1011+
"OR (substr(root_path, 1, 1) BETWEEN 'A' AND 'Z') "
1012+
"OR (substr(root_path, 1, 1) BETWEEN 'a' AND 'z')) LIMIT 1;",
1013+
CBM_NOT_FOUND, &stmt, NULL);
1014+
if (rc != SQLITE_OK) {
1015+
return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT;
1016+
}
1017+
if (sqlite3_step(stmt) == SQLITE_ROW) {
1018+
const char *bad_path = (const char *)sqlite3_column_text(stmt, 0);
1019+
(void)fprintf(stderr, "ERROR store.corrupt table=projects bad_root_path=%s\n",
1020+
bad_path ? bad_path : "(null)");
1021+
verdict = CBM_INTEGRITY_CORRUPT;
1022+
}
1023+
sqlite3_finalize(stmt);
1024+
if (verdict != CBM_INTEGRITY_OK) {
1025+
return verdict;
1026+
}
1027+
1028+
/* ── Deep check: walk the btrees for page-level damage (#1037) ──
1029+
* The shallow check above passes for a DB whose projects table is intact
1030+
* but whose node/edge btrees are torn (observed: 185k nodes / 6k edges with
1031+
* PRAGMA integrity_check reporting btreeInitPage errors). Only quick_check
1032+
* catches that. quick_check is O(db size); this function runs only on the
1033+
* quarantine decision path, never on hot opens. */
1034+
rc = sqlite3_prepare_v2(s->db, "PRAGMA quick_check(1);", CBM_NOT_FOUND, &stmt, NULL);
1035+
if (rc != SQLITE_OK) {
1036+
return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT;
1037+
}
1038+
if (sqlite3_step(stmt) == SQLITE_ROW) {
1039+
const char *res = (const char *)sqlite3_column_text(stmt, 0);
1040+
if (!res || strcmp(res, "ok") != 0) {
1041+
(void)fprintf(stderr, "ERROR store.corrupt quick_check=%s\n", res ? res : "(null)");
1042+
verdict = CBM_INTEGRITY_CORRUPT;
1043+
}
1044+
} else {
1045+
int step_rc = sqlite3_errcode(s->db);
1046+
verdict = st_rc_is_transient(step_rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT;
1047+
}
1048+
sqlite3_finalize(stmt);
1049+
return verdict;
1050+
}
1051+
9511052
bool cbm_store_check_integrity(cbm_store_t *s) {
9521053
if (!s || !s->db) {
9531054
return false;

‎src/store/store.h‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,28 @@ bool cbm_store_check_integrity(cbm_store_t *s);
282282
* O(db size); use on rare paths (artifact import), not hot opens. */
283283
bool cbm_store_check_integrity_deep(cbm_store_t *s);
284284

285+
/* Outcome of a quarantine-grade integrity check. Used to decide whether a DB
286+
* that failed the cheap open-time check should be quarantined (renamed to
287+
* .corrupt and rebuilt) or left alone. See cbm_store_check_integrity_verdict. */
288+
typedef enum {
289+
CBM_INTEGRITY_OK = 0, /* DB is healthy */
290+
CBM_INTEGRITY_CORRUPT = 1, /* DB is structurally damaged — safe to quarantine */
291+
CBM_INTEGRITY_TRANSIENT = 2, /* SQL/busy/IO error — NOT corruption, do NOT quarantine */
292+
} cbm_integrity_verdict_t;
293+
294+
/* Full integrity verdict for the quarantine decision path.
295+
*
296+
* The plain cbm_store_check_integrity() returns a single bool and cannot
297+
* distinguish "the projects table has 99 rows" (real corruption) from
298+
* "sqlite3_prepare_v2 returned SQLITE_BUSY because another instance held the
299+
* writer lock" (a transient lock contention, #1206). Quarantining on the latter
300+
* is what makes concurrent MCP instances destroy each other's healthy DBs.
301+
*
302+
* This function runs the shallow check, then PRAGMA quick_check, and classifies
303+
* the failure mode so the caller can quarantine ONLY on confirmed corruption.
304+
* O(db size); use only on the recovery/quarantine path, not hot opens. */
305+
cbm_integrity_verdict_t cbm_store_check_integrity_verdict(cbm_store_t *s);
306+
285307
/* Open database for a named project in the default cache dir. */
286308
cbm_store_t *cbm_store_open(const char *project);
287309

0 commit comments

Comments
 (0)