doctor integrity check reports "could not measure" as FAIL, and a real error as OK #209
Labels
No labels
code-review
correctness
dos
performance
security
severity/high
severity/low
severity/medium
tech-debt
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
h-dv/code-index#209
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found by dogfooding v0.27.0-rc (
caa62fe) immediately after reinstalling the MCP server.The repro, in two commands with no change to the database
While the daemon was mid-reconcile:
Same command, ~2 minutes later, once the resolve had committed:
Nothing about the database changed. The only difference was a write lock.
So
code-index doctortells an operator their index is corrupt precisely when they are most likely to run it — while indexing is in flight, which is exactly when something feels wrong.integrityis the check whose FAIL causes someone to delete their index and rebuild.Both directions are wrong
crates/cli/src/doctor.rs:548:Unmeasurable renders as measured-and-bad. Any row that is not the literal
"ok"becomesFail. SQLite's FTS5 arm emitsunable to validate the inverted index …: database is locked, which is a statement that the check DID NOT RUN. There is no third state.A real error renders as an all-clear.
while let Ok(Some(row))treatsErras end-of-iteration and falls through toSeverity::Ok, "PRAGMA integrity_check ok". AnSQLITE_CORRUPTsurfacing mid-iteration — the case this check exists for — reports OK.stmt.query([]).unwrap()panics rather than reporting anything.This is the tree's own three-state rule inverted on both sides: absent/unmeasured is not a measurement, and it is not the same as either verdict.
Why it survived
doctor.rs::check_integrityis ungraded. The onlyintegritystring incrates/cli/tests/is a doc comment inplugin_gc_doctor_cli.rs:361. The test that does exist,corrupt_db_is_detected_by_integrity_check, grades a DIFFERENT function —crates/indexer/src/db.rs::check_integrity— with the same name in another crate. The operator-facing one has never been driven by a test.What the fix has to distinguish
Three states, not two:
"ok".Fail.database is lockedis the marker). Must NOT beFail, must NOT beOk, and must say that it did not run and that retrying when the daemon is idle is the remedy.Severity::Warnwith wording that names the reason, or a dedicated state.An
Errduring row iteration is also the "not verified" state, not the OK it currently produces.Mutations the fix must run
Fail→ the not-verified assertion fails.Errarm fall through toOk(i.e. restore today's bug) → the error assertion fails.The locked case is drivable in a test: hold a write transaction on the DB from a second connection and run the check.
Related: the same "did not measure vs measured" rule is what
index_coverage'spending/neversplit,search_symbols'sempty_population, andproject_overview'savailabilityfields all implement correctly.doctoris the surface that did not get it.doctorindex freshness counts never-indexable files as staleness — 35/35 false positives on this repo #210changed_symbolsships a bareref_count: 0wheresearch_symbolssays the zero was never measured — same symbol, same index, opposite honesty #213evidence_gaps.semanticstells the reader to consultpartial_sources, and no tool ever emits that field #216plugin statusand the operator guide still call a predates-the-knobs verdict "another engine" — drift introduced by #211's fix #217Fixed in
c566f87— "doctorstops rendering 'could not measure' as a verdict" — onmaster, shipping in v0.27.0.Both directions were wrong and both are fixed:
PRAGMA integrity_checkreturning "unable to validate … : database is locked" said the check did not run and was reported as corruption — hardest exactly when an operator was most likely to run it, mid-reconcile — while an error raised mid-iteration reported an all-clear. The three states (did not run / ran clean / ran and found something) are now distinct in both the check and its rendering.Closing on merge; the release itself is held on an unrelated Windows archive question (#231).