Merge pull request #1545 from LynxBay/fix/integrity-check-quarantine

fix(store): distinguish transient SQLITE_BUSY from corruption before quarantine (#1206, #1037)

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;

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;

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);