diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 305e94db7..2bbbec52d 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -2156,9 +2156,34 @@ static cbm_store_t *resolve_store_internal(cbm_mcp_server_t *srv, const char *pr } /* The lease may have waited behind a publisher. Re-open and trust - * only the current generation, never the stale pre-wait verdict. */ + * only the current generation, never the stale pre-wait verdict. + * Use the verdict API here — this is the point that decides whether + * a healthy DB gets quarantined. The plain bool check cannot tell + * corruption from a transient SQLITE_BUSY race (#1206: concurrent + * instances quarantining each other's DBs) and does not run + * quick_check, so page-torn DBs with an intact projects table sail + * through (#1037). Only a confirmed CORRUPT verdict is quarantined; + * TRANSIENT (lock/IO) falls through and retries on next access. */ srv->store = cbm_store_open_path_query(path); - bool current_valid = srv->store && cbm_store_check_integrity(srv->store); + cbm_integrity_verdict_t verdict = srv->store + ? cbm_store_check_integrity_verdict(srv->store) + : CBM_INTEGRITY_TRANSIENT; + bool current_valid = (verdict == CBM_INTEGRITY_OK); + if (verdict == CBM_INTEGRITY_TRANSIENT) { + /* The DB could not be conclusively evaluated (lock contention, + * busy writer, IO hiccup). Do NOT quarantine — close and let + * the next resolve retry. A spurious quarantine here is exactly + * what destroys healthy DBs under concurrent access. */ + cbm_store_close(srv->store); + srv->store = NULL; + if (recovery_status) { + *recovery_status = STORE_RECOVERY_BUSY; + } + if (!mutation_already_held) { + mcp_project_mutation_end(srv, project); + } + return NULL; + } if (!current_valid) { cbm_store_close(srv->store); srv->store = NULL; diff --git a/src/store/store.c b/src/store/store.c index fab6ade61..d91346e67 100644 --- a/src/store/store.c +++ b/src/store/store.c @@ -948,6 +948,107 @@ bool cbm_store_check_integrity_deep(cbm_store_t *s) { return ok; } +/* Classify a SQLite result code as a transient (retryable) condition vs. a + * hard error. SQLITE_BUSY / SQLITE_LOCKED happen when another connection holds + * the writer lock — they are NOT evidence of corruption, yet the bare + * cbm_store_check_integrity() treats any prepare failure as "corrupt". This + * helper backs the verdict API so the quarantine path stops destroying healthy + * DBs that merely lost a lock race (#1206, #1037). */ +static bool st_rc_is_transient(int rc) { + switch (rc) { + case SQLITE_BUSY: + case SQLITE_LOCKED: + case SQLITE_IOERR_LOCK: + case SQLITE_IOERR_BLOCKED: + return true; + default: + return false; + } +} + +cbm_integrity_verdict_t cbm_store_check_integrity_verdict(cbm_store_t *s) { + if (!s || !s->db) { + /* No handle at all — the caller could not even open the file. That is + * an open problem, not a corruption verdict, so the DB must not be + * quarantined (renaming it would discard a file we never read). */ + return CBM_INTEGRITY_TRANSIENT; + } + + /* ── Shallow check (projects table shape + root_path sanity) ── */ + sqlite3_stmt *stmt = NULL; + int rc = + sqlite3_prepare_v2(s->db, "SELECT count(*) FROM projects;", CBM_NOT_FOUND, &stmt, NULL); + if (rc != SQLITE_OK) { + /* A prepare failure here is the #1206 trigger: under concurrent access + * the schema lookup can return BUSY/LOCKED. Treat transient codes as + * "do not quarantine"; only a hard error is corruption evidence. */ + return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT; + } + cbm_integrity_verdict_t verdict = CBM_INTEGRITY_OK; + if (sqlite3_step(stmt) == SQLITE_ROW) { + int row_count = sqlite3_column_int(stmt, 0); + if (row_count > ST_MAX_ROW_CHECK) { + (void)fprintf(stderr, "ERROR store.corrupt table=projects rows=%d (expected 1)\n", + row_count); + verdict = CBM_INTEGRITY_CORRUPT; + } + } else { + /* step returned DONE/ERROR — for a SELECT count(*) this means the table + * is unreadable. BUSY-family => transient; otherwise corruption. */ + int step_rc = sqlite3_errcode(s->db); + verdict = st_rc_is_transient(step_rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT; + } + sqlite3_finalize(stmt); + if (verdict != CBM_INTEGRITY_OK) { + return verdict; + } + + /* root_path sanity — same logic as cbm_store_check_integrity, but classify + * prepare failures as transient vs. corrupt rather than a blanket false. */ + rc = sqlite3_prepare_v2(s->db, + "SELECT root_path FROM projects WHERE root_path != '' " + "AND NOT (substr(root_path, 1, 1) = '/' " + "OR (substr(root_path, 1, 1) BETWEEN 'A' AND 'Z') " + "OR (substr(root_path, 1, 1) BETWEEN 'a' AND 'z')) LIMIT 1;", + CBM_NOT_FOUND, &stmt, NULL); + if (rc != SQLITE_OK) { + return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT; + } + if (sqlite3_step(stmt) == SQLITE_ROW) { + const char *bad_path = (const char *)sqlite3_column_text(stmt, 0); + (void)fprintf(stderr, "ERROR store.corrupt table=projects bad_root_path=%s\n", + bad_path ? bad_path : "(null)"); + verdict = CBM_INTEGRITY_CORRUPT; + } + sqlite3_finalize(stmt); + if (verdict != CBM_INTEGRITY_OK) { + return verdict; + } + + /* ── Deep check: walk the btrees for page-level damage (#1037) ── + * The shallow check above passes for a DB whose projects table is intact + * but whose node/edge btrees are torn (observed: 185k nodes / 6k edges with + * PRAGMA integrity_check reporting btreeInitPage errors). Only quick_check + * catches that. quick_check is O(db size); this function runs only on the + * quarantine decision path, never on hot opens. */ + rc = sqlite3_prepare_v2(s->db, "PRAGMA quick_check(1);", CBM_NOT_FOUND, &stmt, NULL); + if (rc != SQLITE_OK) { + return st_rc_is_transient(rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT; + } + if (sqlite3_step(stmt) == SQLITE_ROW) { + const char *res = (const char *)sqlite3_column_text(stmt, 0); + if (!res || strcmp(res, "ok") != 0) { + (void)fprintf(stderr, "ERROR store.corrupt quick_check=%s\n", res ? res : "(null)"); + verdict = CBM_INTEGRITY_CORRUPT; + } + } else { + int step_rc = sqlite3_errcode(s->db); + verdict = st_rc_is_transient(step_rc) ? CBM_INTEGRITY_TRANSIENT : CBM_INTEGRITY_CORRUPT; + } + sqlite3_finalize(stmt); + return verdict; +} + bool cbm_store_check_integrity(cbm_store_t *s) { if (!s || !s->db) { return false; diff --git a/src/store/store.h b/src/store/store.h index ecabd5b72..65f4971f8 100644 --- a/src/store/store.h +++ b/src/store/store.h @@ -282,6 +282,28 @@ bool cbm_store_check_integrity(cbm_store_t *s); * O(db size); use on rare paths (artifact import), not hot opens. */ bool cbm_store_check_integrity_deep(cbm_store_t *s); +/* Outcome of a quarantine-grade integrity check. Used to decide whether a DB + * that failed the cheap open-time check should be quarantined (renamed to + * .corrupt and rebuilt) or left alone. See cbm_store_check_integrity_verdict. */ +typedef enum { + CBM_INTEGRITY_OK = 0, /* DB is healthy */ + CBM_INTEGRITY_CORRUPT = 1, /* DB is structurally damaged — safe to quarantine */ + CBM_INTEGRITY_TRANSIENT = 2, /* SQL/busy/IO error — NOT corruption, do NOT quarantine */ +} cbm_integrity_verdict_t; + +/* Full integrity verdict for the quarantine decision path. + * + * The plain cbm_store_check_integrity() returns a single bool and cannot + * distinguish "the projects table has 99 rows" (real corruption) from + * "sqlite3_prepare_v2 returned SQLITE_BUSY because another instance held the + * writer lock" (a transient lock contention, #1206). Quarantining on the latter + * is what makes concurrent MCP instances destroy each other's healthy DBs. + * + * This function runs the shallow check, then PRAGMA quick_check, and classifies + * the failure mode so the caller can quarantine ONLY on confirmed corruption. + * O(db size); use only on the recovery/quarantine path, not hot opens. */ +cbm_integrity_verdict_t cbm_store_check_integrity_verdict(cbm_store_t *s); + /* Open database for a named project in the default cache dir. */ cbm_store_t *cbm_store_open(const char *project);