chore: PR #235 회차 2 리뷰 반영 — doctor 점검의 오탐·부작용 제거
2회차 리뷰가 1회차 지적 6건은 반영됐다고 확인했고, 대신 **1회차 대응으로
새로 넣은 `fts_shadow` 점검 자체에** HIGH 1건 + MEDIUM 2건을 찾았다.
1) 진단 명령이 없던 스토어를 만들었다 (HIGH)
`SqliteStore::open` 은 마이그레이션은 안 돌리지만 `Connection::open` 이
파일을 만든다. KB 없는 머신에서 `kebab doctor` 한 번이
`<data_dir>/kebab.sqlite` 를 남겼고, 그러면 이후 `open_existing` 이
성공해 버려 `not_indexed` 로 갈렸어야 할 경로가 일반 오류로 바뀐다.
`--readonly` 규약도 진단 명령이 깬다.
`SQLITE_FILE` 을 공개하고 파일 존재를 먼저 확인한 뒤에만 연다.
`doctor_does_not_create_a_store_where_none_exists` 로 고정.
2) V016 미적용 스토어를 고장 났다고 하고, 파괴적 조치를 권했다 (MEDIUM)
doctor 는 마이그레이션을 돌리지 않으므로 V015 스토어를 그대로 읽는다.
그런데 V009 백필이 rowid 를 명시하지 않아서, 그 시점 `chunks.rowid` 에
구멍이 있던 스토어는 V015 에서 **이미 어긋나 있다**. 거기서는 삭제가
`chunk_id` 기준이라 무해한데, 새 점검이 `ok: false` → exit 3 + "`kebab
reset` 후 재색인" 을 냈다. 바이너리를 올리고 doctor 부터 돌리는 자연스러운
순서에서 멀쩡한 KB 를 날리라고 안내한 셈이다.
`migration_version()` 을 보고 V016 미만이면 `ok: true` + "마이그레이션
후 점검된다"로 간다. 실제 해법이 그것이다 — V016 의 명시 rowid
repopulate 가 이 어긋남을 고쳐 준다.
`doctor_does_not_call_a_pre_v016_store_broken` 으로 고정.
3) env override 를 무시해 엉뚱한 DB 를 검사했다 (MEDIUM)
바로 위 `data_dir_writable` 은 "Config::load 와 같은 precedence 유지"를
이유로 env 를 다시 얹는데 이 블록은 안 했다. `KEBAB_STORAGE_DATA_DIR` 을
쓰면 data_dir 은 A 로 보고하면서 인덱스는 B 를 검사한다. 같은 방식으로
맞췄다.
4) 잔가지 (LOW)
- `None` 분기가 "스토어 없음 / 열기 실패 / 읽기 실패"를 전부 "KB 없음"
으로 뭉갰다. 조용한 실패를 드러내려는 점검이 자기 실패를 삼키면 안
되므로 "점검하지 못했다" 를 따로 뒀다 (hint 가 있으므로 CLI 가 `!`
로 찍는다).
- "표본 400행" 이 chunk 400개 미만인 스토어에서 거짓이었다. ASC/DESC
두 창이 겹치면 분자와 분모를 둘 다 두 번 셌다. UNION 으로 바꾸고
실제 표본 수를 함께 돌려준다 — 반환형이 `(checked, misaligned)` 다.
- `pub const SQLITE_FILE` 위에 "Kept private" 이라는 옛 독 주석이
남아 있었다.
- README 의 doctor 행에 `fts_shadow` 가 exit 3 을 낼 수 있다고 적었다.
새 플래그도 config 키도 없지만 **doctor 가 실패하는 새 사유**는
스크립트와 에이전트가 분기하는 사용자 표면이다.
- `tasks/phase-2-lexical-search.md` 가 `kebab index --rebuild-fts` 를
산출물로 나열하고 있었다. 배선된 적 없는 명령이고 이 PR 이 "CLI
경로 없음"이라고 못박은 것과 어긋나서 정정했다.
실측 확인: env override 를 준 doctor 가 지정한 KB 를 검사하고("양끝 400행
표본"), V015 실제 KB(28,427 문서)는 "V016 적용 전 (현재 V015)" 로 나오며,
KB 없는 경로에서는 파일을 남기지 않는다.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017c9JwQq8ZkGvYjpKXMiDhF
This commit is contained in:
@@ -35,4 +35,4 @@ pub use error::StoreError;
|
||||
pub use eval::{EvalQueryResultRecord, EvalRunRecord, EvalRunRow};
|
||||
pub use fts::rebuild_chunks_fts;
|
||||
pub use jobs::IngestRunRow;
|
||||
pub use store::{CountSummary, NotIndexed, SqliteStore, purge_deleted_workspace_path};
|
||||
pub use store::{CountSummary, NotIndexed, SQLITE_FILE, SqliteStore, purge_deleted_workspace_path};
|
||||
|
||||
@@ -41,10 +41,10 @@ static TEMP_SUFFIX_COUNTER: AtomicU64 = AtomicU64::new(0);
|
||||
/// truncated, mirrored from `kb-core`'s newtype invariant.
|
||||
const ASSET_ID_HEX_LEN: usize = 32;
|
||||
|
||||
/// Default file name under `config.storage.data_dir`. Kept private — the
|
||||
/// path layout is a §6.3 design decision, not part of the store's public
|
||||
/// surface.
|
||||
const SQLITE_FILE: &str = "kebab.sqlite";
|
||||
/// Filename of the main SQLite database under `storage.data_dir`.
|
||||
/// Public so read-only probes (e.g. `kebab doctor`) can test for the
|
||||
/// file before calling [`SqliteStore::open`], which creates it.
|
||||
pub const SQLITE_FILE: &str = "kebab.sqlite";
|
||||
|
||||
/// Subdirectory under `data_dir` holding shard-prefixed asset bytes
|
||||
/// (`<aa>/<asset_id>`). Mirrors design §6.3.
|
||||
@@ -351,9 +351,10 @@ fn temp_path_for(dest: &Path) -> PathBuf {
|
||||
}
|
||||
|
||||
impl SqliteStore {
|
||||
/// Count rows in a bounded sample where `chunks_fts` and `chunks`
|
||||
/// disagree about which chunk lives at a given rowid (issue #229 /
|
||||
/// V016). `0` means the sample is aligned.
|
||||
/// Sample rows from both ends of the rowid range and report
|
||||
/// `(checked, misaligned)` — how many were compared and how many
|
||||
/// disagree with `chunks` about which chunk lives at that rowid
|
||||
/// (issue #229 / V016). A `misaligned` of 0 means the sample agrees.
|
||||
///
|
||||
/// V016 pointed the FTS delete trigger at `rowid`, so this alignment
|
||||
/// is what keeps a delete from removing some other document's shadow
|
||||
@@ -362,27 +363,51 @@ impl SqliteStore {
|
||||
/// 10ms, and enough to catch the wholesale renumbering that any
|
||||
/// realistic drift would produce. It is a health probe, not a proof:
|
||||
/// a drift confined to the middle of the range passes.
|
||||
pub fn fts_shadow_misaligned_sample(&self, sample: usize) -> Result<usize> {
|
||||
pub fn fts_shadow_misaligned_sample(&self, sample: usize) -> Result<(usize, usize)> {
|
||||
let conn = self.read_conn();
|
||||
let mut total = 0usize;
|
||||
for order in ["ASC", "DESC"] {
|
||||
let n: i64 = conn
|
||||
.query_row(
|
||||
&format!(
|
||||
"SELECT COUNT(*) FROM (
|
||||
SELECT rowid AS r, chunk_id AS cid FROM chunks
|
||||
ORDER BY rowid {order} LIMIT ?1
|
||||
) c
|
||||
LEFT JOIN chunks_fts f ON f.rowid = c.r
|
||||
WHERE f.rowid IS NULL OR f.chunk_id != c.cid"
|
||||
),
|
||||
params![sample as i64],
|
||||
|r| r.get(0),
|
||||
)
|
||||
.map_err(StoreError::from)?;
|
||||
total += usize::try_from(n).unwrap_or(0);
|
||||
}
|
||||
Ok(total)
|
||||
// UNION of the two ends rather than two separate counts: on a
|
||||
// store with fewer rows than 2×sample the windows overlap, and
|
||||
// counting them separately would double both the numerator and
|
||||
// the denominator the caller reports.
|
||||
let (checked, bad): (i64, i64) = conn
|
||||
.query_row(
|
||||
"WITH ends AS (
|
||||
SELECT rowid AS r, chunk_id AS cid FROM
|
||||
(SELECT rowid, chunk_id FROM chunks ORDER BY rowid ASC LIMIT ?1)
|
||||
UNION
|
||||
SELECT rowid AS r, chunk_id AS cid FROM
|
||||
(SELECT rowid, chunk_id FROM chunks ORDER BY rowid DESC LIMIT ?1)
|
||||
)
|
||||
SELECT COUNT(*),
|
||||
COALESCE(SUM(f.rowid IS NULL OR f.chunk_id != c.cid), 0)
|
||||
FROM ends c LEFT JOIN chunks_fts f ON f.rowid = c.r",
|
||||
params![sample as i64],
|
||||
|r| Ok((r.get(0)?, r.get(1)?)),
|
||||
)
|
||||
.map_err(StoreError::from)?;
|
||||
Ok((
|
||||
usize::try_from(checked).unwrap_or(0),
|
||||
usize::try_from(bad).unwrap_or(0),
|
||||
))
|
||||
}
|
||||
|
||||
/// Highest applied refinery migration version, or `None` if the
|
||||
/// history table is missing (a file that is not a kebab store, or
|
||||
/// one opened before `run_migrations`).
|
||||
///
|
||||
/// Read-only callers use this to tell "this store predates the
|
||||
/// migration I care about" from "this store is broken" — the two
|
||||
/// need opposite advice.
|
||||
pub fn migration_version(&self) -> Option<u32> {
|
||||
self.read_conn()
|
||||
.query_row(
|
||||
"SELECT MAX(version) FROM refinery_schema_history",
|
||||
[],
|
||||
|r| r.get::<_, Option<i64>>(0),
|
||||
)
|
||||
.ok()
|
||||
.flatten()
|
||||
.and_then(|v| u32::try_from(v).ok())
|
||||
}
|
||||
|
||||
/// p9-fb-19: read the persisted `corpus_revision` from the `kv`
|
||||
|
||||
@@ -852,8 +852,9 @@ fn fts_v016_shadow_probe_detects_forced_drift() {
|
||||
}
|
||||
assert_eq!(
|
||||
store.fts_shadow_misaligned_sample(200).unwrap(),
|
||||
0,
|
||||
"a freshly written shadow must be aligned"
|
||||
(3, 0),
|
||||
"a freshly written shadow must be aligned, and the two rowid \
|
||||
windows must not double-count a store smaller than the sample"
|
||||
);
|
||||
|
||||
// Shift one shadow row off its source. `chunks_fts` rows are not
|
||||
@@ -868,8 +869,15 @@ fn fts_v016_shadow_probe_detects_forced_drift() {
|
||||
)
|
||||
.expect("reinsert at a drifted rowid");
|
||||
|
||||
assert!(
|
||||
store.fts_shadow_misaligned_sample(200).unwrap() > 0,
|
||||
"the probe must report the drifted row"
|
||||
assert_eq!(
|
||||
store.fts_shadow_misaligned_sample(200).unwrap(),
|
||||
(3, 1),
|
||||
"the probe must report the one drifted row out of three checked"
|
||||
);
|
||||
assert_eq!(
|
||||
store.migration_version(),
|
||||
Some(16),
|
||||
"a migrated store must report V016 so doctor can tell it apart \
|
||||
from a pre-V016 one, where the same drift is harmless"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user