chore: PR #235 회차 1 리뷰 반영 — 실패 양상 탐지 + 사실관계 정정
리뷰 두 건에서 나온 지적을 반영한다. 1) 실패 양상이 바뀐 것을 다루지 않았다 (MEDIUM) `chunk_id` 로 행을 찾던 때는 shadow 정렬이 어긋나도 느릴 뿐 정확했다. rowid 로 찾으면 어긋난 순간 `chunks_ad` 가 남의 문서 shadow 행을 지우고 아무 오류도 내지 않는다. 즉 이 PR 은 실패 양상을 "느림"에서 "조용한 오삭제"로 바꿨는데, 그 불변식이 눈에 안 보이는 상태였다. `kebab doctor` 에 `fts_shadow` 점검을 넣었다. 전수 대조는 60만 chunk 에서 33초라 doctor 앞에 둘 수 없어 rowid 범위 앞뒤 200행씩만 본다 — 실측 10 ms 이고, 현실적인 드리프트가 취하는 전면 재번호는 잡는다. 표본이라는 사실을 detail 에 적어 정렬 증명으로 읽히지 않게 했다. `SqliteStore::fts_shadow_misaligned_sample` 이 질의를 들고 있다. 2) VACUUM 위험을 과장했다 (정정) 초안이 "VACUUM 이 rowid 를 다시 매길 수 있고 그러면 정렬이 깨진다"고 단정했다. 실제로 재보니 다시 매기지 않았다 — 실제 KB 사본(60만 chunk, 문서 3,000건을 지워 rowid 에 구멍을 낸 뒤)과 소형 합성 DB 양쪽에서 VACUUM 후 전수 대조 불일치가 0 이었다 (sqlite 3.53.4). SQLite 문서가 "다시 매길 수 있다"고 적은 것은 보장이 없다는 뜻이지 실제로 그렇게 한다는 뜻이 아니다. 문구를 실측대로 고쳤다. 남는 실제 경로는 앞으로 `chunks` 를 테이블 재작성 방식으로 바꾸는 마이그레이션이다. V016 주석에 "그런 마이그레이션은 repopulate 를 같이 돌려야 한다"는 울타리를 박았다. 3) 같은 실측치를 파일마다 다르게 적었다 (MEDIUM) 삭제 시간이 커밋 메시지·HOTFIXES 는 2.0초, 마이그레이션 주석·테스트 독스트링·설계 문서는 0.73초였다. 0.73초는 손으로 마이그레이션한 사본을 따뜻한 캐시에서 잰 값이고 2.0초는 릴리스 바이너리가 마이그레이션한 새 사본에서 잰 값이다. 보수적인 2.0초로 통일했다. '한국' hit 수도 15,837(문서 200건 삭제 후) 과 15,977(전체 코퍼스) 이 섞여 있어 15,977 로 통일했다. 4) docs/ARCHITECTURE.md 디렉토리 트리가 V001..V015 로 멈춰 있었다 (MEDIUM) V016 까지로 갱신. README 는 손대지 않는다 — 새 서브커맨드·플래그·config 키·`--json` 필드가 없다. 5) 잔가지 (LOW) `:=` 검사가 번들 SQLite 의 FTS5 idxStr 인코딩에 기대는 것을 assert 메시지에 적었다 (rusqlite 를 올린 직후 실패하면 거기부터 보라는 뜻). 가상 테이블은 항상 `SCAN` 으로 찍히므로 `SEARCH` 로 대체 검사할 방법이 없다는 것도 독스트링에 남겼다. `kb index --rebuild-fts` 라는 옛 이름 + 존재하지 않는 명령 참조 두 곳을 지웠다. `fts_v016_shadow_probe_detects_forced_drift` 로 탐지 자체를 시험한다 — 어긋난 shadow 행을 억지로 만들어 점검이 잡는지 본다. 잡지 못하는 점검은 없느니만 못하다. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017c9JwQq8ZkGvYjpKXMiDhF
This commit is contained in:
@@ -10,9 +10,11 @@
|
||||
//!
|
||||
//! Normal operation needs nothing from this module — every mutation on
|
||||
//! `chunks` propagates automatically inside the host transaction. The
|
||||
//! only entry point exposed here is [`rebuild_chunks_fts`], used as the
|
||||
//! escape hatch for `kb index --rebuild-fts` (wired by `kb-cli` later;
|
||||
//! out of scope for P2-1).
|
||||
//! only entry point exposed here is [`rebuild_chunks_fts`], the escape
|
||||
//! hatch for a shadow that has drifted from `chunks`. It is a library
|
||||
//! API with no CLI wiring — `kebab doctor`'s `fts_shadow` check detects
|
||||
//! drift, but recovery through the CLI is `kebab reset` plus a
|
||||
//! re-ingest.
|
||||
|
||||
use anyhow::{Context, Result};
|
||||
use rusqlite::Connection;
|
||||
|
||||
@@ -351,6 +351,40 @@ 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.
|
||||
///
|
||||
/// V016 pointed the FTS delete trigger at `rowid`, so this alignment
|
||||
/// is what keeps a delete from removing some other document's shadow
|
||||
/// row. The full anti-join is a 33s scan at 600k chunks, so this
|
||||
/// takes `sample` rows from each end of the rowid range instead —
|
||||
/// 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> {
|
||||
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)
|
||||
}
|
||||
|
||||
/// p9-fb-19: read the persisted `corpus_revision` from the `kv`
|
||||
/// table. Returns `0` if the row is missing (not migrated yet) or
|
||||
/// unparseable — defensive: callers use the value as a cache-key
|
||||
|
||||
@@ -282,7 +282,7 @@ fn fts_rebuild_chunks_fts_recovers_from_drift() {
|
||||
insert_chunk(&conn, &cid, &"d".repeat(32), "[]", "recovered");
|
||||
|
||||
// Manually wipe chunks_fts to simulate drift; this is the failure
|
||||
// mode `kb index --rebuild-fts` exists to recover from.
|
||||
// mode `rebuild_chunks_fts` exists to recover from.
|
||||
conn.execute("DELETE FROM chunks_fts", []).unwrap();
|
||||
assert_eq!(count(&conn, "chunks_fts"), 0);
|
||||
assert_eq!(count(&conn, "chunks"), 1);
|
||||
@@ -742,7 +742,10 @@ fn fts_v016_delete_removes_only_the_deleted_row() {
|
||||
/// equality itself. Addressing by `chunk_id` leaves that field empty
|
||||
/// because `chunk_id` is UNINDEXED, and the delete degrades to a full
|
||||
/// index scan (measured at 1590s for 200 documents over 600k chunks,
|
||||
/// against 0.73s for this plan).
|
||||
/// against 2.0s for this plan).
|
||||
///
|
||||
/// A virtual table is always reported as `SCAN`, so the idxStr field is
|
||||
/// the only signal available here — there is no `SEARCH` to assert on.
|
||||
#[test]
|
||||
fn fts_v016_delete_plan_uses_rowid_not_a_scan() {
|
||||
let env = common::TestEnv::new();
|
||||
@@ -766,7 +769,9 @@ fn fts_v016_delete_plan_uses_rowid_not_a_scan() {
|
||||
|
||||
assert!(
|
||||
by_rowid.contains(":="),
|
||||
"rowid delete must hand FTS5 the equality constraint; got {by_rowid:?}"
|
||||
"rowid delete must hand FTS5 the equality constraint; got {by_rowid:?}. \
|
||||
This reads the bundled SQLite's FTS5 idxStr encoding — if it fails right \
|
||||
after a rusqlite bump, check that before suspecting the migration."
|
||||
);
|
||||
assert!(
|
||||
!by_chunk_id.contains(":="),
|
||||
@@ -824,3 +829,47 @@ fn fts_v016_rebuild_preserves_rowid_and_korean_morphemes() {
|
||||
"deletes after a rebuild must still find their shadow rows"
|
||||
);
|
||||
}
|
||||
|
||||
/// The `fts_shadow` doctor probe has to actually notice drift, or it is
|
||||
/// worse than no check — it would report health while deletes silently
|
||||
/// remove other documents' rows. Forcing a misaligned shadow row is the
|
||||
/// only way to test that, since nothing in the codebase produces one.
|
||||
#[test]
|
||||
fn fts_v016_shadow_probe_detects_forced_drift() {
|
||||
let env = common::TestEnv::new();
|
||||
let store = SqliteStore::open(&env.config().storage).unwrap();
|
||||
store.run_migrations().unwrap();
|
||||
|
||||
let conn = raw_conn_no_fk(&env);
|
||||
for i in 0..3u8 {
|
||||
insert_chunk(
|
||||
&conn,
|
||||
&format!("{i:032}"),
|
||||
&"d".repeat(32),
|
||||
"[]",
|
||||
&format!("body {i}"),
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
store.fts_shadow_misaligned_sample(200).unwrap(),
|
||||
0,
|
||||
"a freshly written shadow must be aligned"
|
||||
);
|
||||
|
||||
// Shift one shadow row off its source. `chunks_fts` rows are not
|
||||
// UPDATE-able in place, so delete and reinsert at a rowid that
|
||||
// belongs to nothing.
|
||||
conn.execute("DELETE FROM chunks_fts WHERE rowid = 2", [])
|
||||
.expect("drop one shadow row");
|
||||
conn.execute(
|
||||
"INSERT INTO chunks_fts(rowid, chunk_id, doc_id, heading_path, text)
|
||||
VALUES (9999, ?, ?, '[]', 'body 1')",
|
||||
rusqlite::params![format!("{:032}", 1u8), "d".repeat(32)],
|
||||
)
|
||||
.expect("reinsert at a drifted rowid");
|
||||
|
||||
assert!(
|
||||
store.fts_shadow_misaligned_sample(200).unwrap() > 0,
|
||||
"the probe must report the drifted row"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user