chore: PR #236 회차 3 리뷰 반영 — 빈 스캔 취소 오보고 + 테스트 탐지력
3회차 리뷰가 취소 처리에 CRITICAL/HIGH 가 없음을 확인하고(특히 break 후에도
`flush_vector_deletes` 가 돌아 고아 벡터가 안 생기는 것, `examined` 가 네
경계에서 모두 맞는 것) 머지 가능으로 결론냈다. 남은 지적 다섯을 반영한다.
1) 빈 스캔 + sweep 취소가 Aborted 가 아니라 Completed 로 보고됐다 (MEDIUM)
`was_cancelled` 는 asset 루프 **본문**에서만 세팅되는데, 스캔이 0건이면
본문이 한 번도 안 돈다. 하필 그게 sweep 이 가장 커지는 경우다 — 스캔이
0건이면 저장된 모든 경로가 후보이므로, "색인된 디렉토리를 통째로 지우고
재색인 → 긴 sweep → Ctrl-C" 가 정확히 이 구멍에 들어간다. 사용자는
취소했는데 "완료"를 본다.
플래그로 시드한다. `cancelling_a_sweep_with_nothing_left_to_scan_still_
reports_aborted` 로 고정했는데, 처음 쓴 버전은 워크스페이스 상위만
지워서 픽스처가 하위 디렉토리에 남았고 asset 루프가 한 번 돌아 **되돌려도
통과했다**. 재귀 삭제로 고치고, 전제(`ScanCompleted { total: 0 }`)를
어서션으로 박았다. 시드를 되돌리면 실패하는 것을 확인했다.
2) CLI 테스트의 position 어서션에 탐지력이 없었다 (LOW)
두 시나리오를 따로 돌려서 두 번째에 `SweepProgress` 가 없었고, position 은
`SweepStarted` 의 `set_position(0)` 이후 계속 0 이었다. 즉
`dress_bar_for_assets` 안의 `set_position(0)` 을 지워도 통과했다.
하나로 합쳐 sweep 이 position 을 3 까지 올린 뒤 복구를 본다.
3) 그 테스트 주석이 실제 범위보다 넓게 주장했다 (LOW)
"하트비트 키까지 고정한다" 고 적었는데 실제로는 length/position 만 본다.
indicatif 가 스타일을 되읽을 방법을 주지 않으므로, 그 절반은
`dress_bar_for_assets` 가 양쪽 phase 의 유일한 옷 입히는 자리라는 사실에
기댄다 — 주석을 그렇게 고쳐 적었다.
4) 취소 계약 독 주석이 stale 했다 (LOW)
`ingest_with_config` 의 §10 계약에 "in-flight asset 이 끝나고 이후는
스킵" 만 있고 sweep 이 이제 취소를 본다는 사실이 없었다.
5) DOGFOOD §1.8 이 자기 모순이었다 (LOW)
`sweep_completed.checked == total` 을 단정하고 바로 다음 줄에서 취소 시엔
다르다고 했다. 앞줄에 "취소 없이 완주하면" 을 붙였다.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017c9JwQq8ZkGvYjpKXMiDhF
This commit is contained in:
@@ -84,6 +84,10 @@ pub fn ingest(scope: SourceScope, opts: IngestOpts) -> anyhow::Result<IngestRepo
|
|||||||
///
|
///
|
||||||
/// - The current in-flight asset finishes (rollback would break
|
/// - The current in-flight asset finishes (rollback would break
|
||||||
/// idempotent re-run). Subsequent assets are skipped.
|
/// idempotent re-run). Subsequent assets are skipped.
|
||||||
|
/// - The deleted-file sweep, which runs before the asset loop, checks
|
||||||
|
/// the same flag between candidates and stops at the next one
|
||||||
|
/// (issue #228). Documents it already purged stay purged, and the
|
||||||
|
/// buffered vector deletes are still flushed on the way out.
|
||||||
/// - Cancellation is a normal exit, not an error — `Result::Err` is
|
/// - Cancellation is a normal exit, not an error — `Result::Err` is
|
||||||
/// reserved for actual failures.
|
/// reserved for actual failures.
|
||||||
/// - Partial commits in SQLite are kept; the next `kebab ingest` run
|
/// - Partial commits in SQLite are kept; the next `kebab ingest` run
|
||||||
@@ -328,7 +332,14 @@ pub fn ingest_with_config(
|
|||||||
// p9-fb-04: track whether the loop exited via cancellation (vs
|
// p9-fb-04: track whether the loop exited via cancellation (vs
|
||||||
// running to completion) so we can emit `Aborted` rather than
|
// running to completion) so we can emit `Aborted` rather than
|
||||||
// `Completed` and surface the right summary.
|
// `Completed` and surface the right summary.
|
||||||
let mut was_cancelled = false;
|
// Seeded from the flag rather than only from the asset loop: the loop
|
||||||
|
// body is what normally sets this, and it never runs when the scan
|
||||||
|
// found nothing. That is exactly the case where the sweep is largest
|
||||||
|
// — a scan of zero assets means every stored path is a sweep
|
||||||
|
// candidate — so "wipe the indexed directory, re-ingest, Ctrl-C
|
||||||
|
// during the long sweep" would otherwise end by reporting Completed
|
||||||
|
// to a user who cancelled.
|
||||||
|
let mut was_cancelled = cancelled();
|
||||||
|
|
||||||
for (zero_idx, asset) in assets.into_iter().enumerate() {
|
for (zero_idx, asset) in assets.into_iter().enumerate() {
|
||||||
// Step boundary check (p9-fb-04). Designed §10 invariant: the
|
// Step boundary check (p9-fb-04). Designed §10 invariant: the
|
||||||
|
|||||||
@@ -215,3 +215,68 @@ fn a_cancelled_sweep_stops_and_reports_only_what_it_examined() {
|
|||||||
the announced total here would be a lie: {events:?}"
|
the announced total here would be a lie: {events:?}"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The scan finding nothing is the case where the sweep is largest —
|
||||||
|
/// zero assets means every stored path is a candidate. It is also the
|
||||||
|
/// case where the asset loop body never runs, and that body is what
|
||||||
|
/// normally records "this run was cancelled". So "wipe the indexed
|
||||||
|
/// directory, re-ingest, Ctrl-C during the long sweep" used to end by
|
||||||
|
/// telling the user the run had Completed.
|
||||||
|
#[test]
|
||||||
|
fn cancelling_a_sweep_with_nothing_left_to_scan_still_reports_aborted() {
|
||||||
|
let env = TestEnv::lexical_only();
|
||||||
|
std::fs::write(
|
||||||
|
env.workspace_root.join("temporary.rs"),
|
||||||
|
"// here for one ingest\nfn t() {}\n",
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
ingest_collecting(&env);
|
||||||
|
|
||||||
|
// Empty the workspace, so the next scan finds no assets at all and
|
||||||
|
// every stored path becomes a sweep candidate. Recursive: the test
|
||||||
|
// fixtures sit in subdirectories, and leaving one behind gives the
|
||||||
|
// asset loop an iteration to run — which is precisely the iteration
|
||||||
|
// this test needs to not happen.
|
||||||
|
fn clear(dir: &std::path::Path) {
|
||||||
|
for entry in std::fs::read_dir(dir).unwrap() {
|
||||||
|
let p = entry.unwrap().path();
|
||||||
|
if p.is_dir() {
|
||||||
|
clear(&p);
|
||||||
|
} else {
|
||||||
|
std::fs::remove_file(p).unwrap();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
clear(&env.workspace_root);
|
||||||
|
|
||||||
|
let cancel = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(true));
|
||||||
|
let (tx, rx) = mpsc::channel::<IngestEvent>();
|
||||||
|
ingest_with_config(
|
||||||
|
env.config.clone(),
|
||||||
|
env.scope(),
|
||||||
|
IngestOpts {
|
||||||
|
progress: Some(tx),
|
||||||
|
cancel: Some(cancel),
|
||||||
|
..Default::default()
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.expect("a cancelled ingest still returns a report");
|
||||||
|
let events: Vec<IngestEvent> = rx.into_iter().collect();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
matches!(
|
||||||
|
events
|
||||||
|
.iter()
|
||||||
|
.find(|e| matches!(e, IngestEvent::ScanCompleted { .. })),
|
||||||
|
Some(IngestEvent::ScanCompleted { total: 0 })
|
||||||
|
),
|
||||||
|
"the premise: this run must find nothing to index, or the asset \
|
||||||
|
loop runs an iteration and records the cancel by itself: {events:?}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
matches!(events.last(), Some(IngestEvent::Aborted { .. })),
|
||||||
|
"a cancelled run must end in Aborted even when the asset loop \
|
||||||
|
never ran a single iteration: {:?}",
|
||||||
|
events.last()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
@@ -547,68 +547,64 @@ pub(crate) fn now_rfc3339() -> anyhow::Result<String> {
|
|||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
fn feed(events: &[IngestEvent]) -> ProgressDisplay {
|
/// The sweep phase (issue #228) borrows the asset bar and puts its
|
||||||
// quiet + non-tty: the bar exists with a hidden draw target, so
|
/// own, smaller total on it. If it does not hand the bar back, every
|
||||||
// its state is observable without a terminal and nothing is
|
/// asset drawn afterwards carries the sweep's denominator — that
|
||||||
// printed to the test's stderr.
|
/// shipped once already, so it is pinned here.
|
||||||
|
///
|
||||||
|
/// Scope: length and position only. The same bug also dropped the
|
||||||
|
/// `{asset_elapsed}` heartbeat key along with the style, and
|
||||||
|
/// indicatif exposes no way to read a bar's style back, so that half
|
||||||
|
/// rests on `dress_bar_for_assets` being the single place either
|
||||||
|
/// phase dresses the bar.
|
||||||
|
#[test]
|
||||||
|
fn sweep_hands_the_bar_back_to_the_asset_loop() {
|
||||||
let mut d = ProgressDisplay::new(ProgressMode::Human {
|
let mut d = ProgressDisplay::new(ProgressMode::Human {
|
||||||
tty: false,
|
tty: false,
|
||||||
quiet: true,
|
quiet: true,
|
||||||
});
|
});
|
||||||
for e in events {
|
for e in [
|
||||||
d.handle(e).expect("handle");
|
|
||||||
}
|
|
||||||
d
|
|
||||||
}
|
|
||||||
|
|
||||||
/// The sweep phase (issue #228) borrows the asset bar and puts its
|
|
||||||
/// own, smaller total on it. If it does not hand the bar back, every
|
|
||||||
/// asset drawn afterwards carries the sweep's label and denominator —
|
|
||||||
/// and the style swap also drops the `{asset_elapsed}` heartbeat that
|
|
||||||
/// is the only sign a slow asset is alive. That shipped once already;
|
|
||||||
/// this pins it so the next phase added here cannot repeat it.
|
|
||||||
#[test]
|
|
||||||
fn sweep_hands_the_bar_back_to_the_asset_loop() {
|
|
||||||
let d = feed(&[
|
|
||||||
IngestEvent::ScanStarted {
|
IngestEvent::ScanStarted {
|
||||||
root: "/ws".to_string(),
|
root: "/ws".to_string(),
|
||||||
},
|
},
|
||||||
IngestEvent::ScanCompleted { total: 17 },
|
IngestEvent::ScanCompleted { total: 17 },
|
||||||
IngestEvent::SweepStarted { total: 3 },
|
IngestEvent::SweepStarted { total: 3 },
|
||||||
IngestEvent::SweepProgress {
|
IngestEvent::SweepProgress {
|
||||||
idx: 1,
|
idx: 3,
|
||||||
total: 3,
|
total: 3,
|
||||||
path: "gone.md".to_string(),
|
path: "gone.md".to_string(),
|
||||||
removed: true,
|
removed: true,
|
||||||
},
|
},
|
||||||
]);
|
] {
|
||||||
|
d.handle(&e).expect("handle");
|
||||||
|
}
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
||||||
Some(3),
|
Some(3),
|
||||||
"during the sweep the bar counts sweep candidates"
|
"during the sweep the bar counts sweep candidates"
|
||||||
);
|
);
|
||||||
|
assert_eq!(
|
||||||
|
d.bar.as_ref().map(indicatif::ProgressBar::position),
|
||||||
|
Some(3),
|
||||||
|
"and tracks them"
|
||||||
|
);
|
||||||
|
|
||||||
let d = feed(&[
|
d.handle(&IngestEvent::SweepCompleted {
|
||||||
IngestEvent::ScanStarted {
|
checked: 3,
|
||||||
root: "/ws".to_string(),
|
purged: 3,
|
||||||
},
|
ms: 12,
|
||||||
IngestEvent::ScanCompleted { total: 17 },
|
})
|
||||||
IngestEvent::SweepStarted { total: 3 },
|
.expect("handle");
|
||||||
IngestEvent::SweepCompleted {
|
|
||||||
checked: 3,
|
|
||||||
purged: 3,
|
|
||||||
ms: 12,
|
|
||||||
},
|
|
||||||
]);
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
||||||
Some(17),
|
Some(17),
|
||||||
"and once it ends the bar counts assets again"
|
"once the sweep ends the bar counts assets again"
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
d.bar.as_ref().map(indicatif::ProgressBar::position),
|
d.bar.as_ref().map(indicatif::ProgressBar::position),
|
||||||
Some(0),
|
Some(0),
|
||||||
"from the start, not from wherever the sweep left off"
|
"from the start — leaving the sweep's position would make the \
|
||||||
|
asset loop look part-done before it began"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -274,7 +274,7 @@ echo "# stdin content" | "$RELEASE_BIN" ingest-stdin --title "from stdin" --conf
|
|||||||
- ordering invariant (design §2.4a).
|
- ordering invariant (design §2.4a).
|
||||||
- per-asset `idx/total/path/media/result/chunks`.
|
- per-asset `idx/total/path/media/result/chunks`.
|
||||||
- aggregate `counts` on `completed` / `aborted`.
|
- aggregate `counts` on `completed` / `aborted`.
|
||||||
- sweep 구간: `sweep_started.total` 이 후보 수와 맞고, `sweep_progress.idx` 가 1..=total 로 연속이며, `sweep_completed.checked == total`.
|
- sweep 구간: `sweep_started.total` 이 후보 수와 맞고, `sweep_progress.idx` 가 1..=total 로 연속이며, 취소 없이 완주하면 `sweep_completed.checked == total`.
|
||||||
- sweep 이 끝난 뒤 진행바가 asset 분모·라벨로 돌아오는가 (TTY). sweep 이 같은 바를 빌려 쓰므로 복구가 빠지면 색인 구간 내내 `sweep [..] 4213/21` 로 그려진다.
|
- sweep 이 끝난 뒤 진행바가 asset 분모·라벨로 돌아오는가 (TTY). sweep 이 같은 바를 빌려 쓰므로 복구가 빠지면 색인 구간 내내 `sweep [..] 4213/21` 로 그려진다.
|
||||||
- ndjson 로그에 `purge` 줄과 `sweep_summary` 가 남는가 (이슈 #228 이전에는 이 구간이 0바이트였다). purge 실패 시 `purge_failed`.
|
- ndjson 로그에 `purge` 줄과 `sweep_summary` 가 남는가 (이슈 #228 이전에는 이 구간이 0바이트였다). purge 실패 시 `purge_failed`.
|
||||||
- sweep 중 Ctrl-C 한 번에 실제로 멈추는가. `sweep_completed.checked` 가 예고한 `total` 이 아니라 실제 검사한 수로 나와야 한다.
|
- sweep 중 Ctrl-C 한 번에 실제로 멈추는가. `sweep_completed.checked` 가 예고한 `total` 이 아니라 실제 검사한 수로 나와야 한다.
|
||||||
|
|||||||
Reference in New Issue
Block a user