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
283 lines
10 KiB
Rust
283 lines
10 KiB
Rust
//! Issue #228: the deleted-file sweep runs between the scan and the asset
|
|
//! loop and used to emit nothing at all — no progress events, no ndjson
|
|
//! log lines. A sweep that took 32 hours in dogfooding was externally
|
|
//! indistinguishable from a hang, and users killed the run three times in
|
|
//! a row. These tests pin the observability, not the purging (which
|
|
//! `file_deletion_auto_purge.rs` already covers).
|
|
|
|
mod common;
|
|
|
|
use std::sync::mpsc;
|
|
|
|
use common::TestEnv;
|
|
use kebab_app::{IngestEvent, IngestOpts, ingest_with_config};
|
|
use kebab_core::SourceScope;
|
|
|
|
/// Ingest whatever is in the workspace, collecting progress events.
|
|
fn ingest_collecting(env: &TestEnv) -> Vec<IngestEvent> {
|
|
let (tx, rx) = mpsc::channel::<IngestEvent>();
|
|
ingest_with_config(
|
|
env.config.clone(),
|
|
env.scope(),
|
|
IngestOpts {
|
|
progress: Some(tx),
|
|
..Default::default()
|
|
},
|
|
)
|
|
.expect("ingest must succeed");
|
|
let mut events = Vec::new();
|
|
while let Ok(ev) = rx.recv() {
|
|
events.push(ev);
|
|
}
|
|
events
|
|
}
|
|
|
|
fn sweep_bounds(events: &[IngestEvent]) -> (Option<u32>, Option<(u32, u32)>) {
|
|
let started = events.iter().find_map(|e| match e {
|
|
IngestEvent::SweepStarted { total } => Some(*total),
|
|
_ => None,
|
|
});
|
|
let completed = events.iter().find_map(|e| match e {
|
|
IngestEvent::SweepCompleted {
|
|
checked, purged, ..
|
|
} => Some((*checked, *purged)),
|
|
_ => None,
|
|
});
|
|
(started, completed)
|
|
}
|
|
|
|
#[test]
|
|
fn sweep_emits_a_bounded_phase_with_one_event_per_candidate() {
|
|
let env = TestEnv::lexical_only();
|
|
for i in 0..4 {
|
|
std::fs::write(
|
|
env.workspace_root.join(format!("gone{i}.rs")),
|
|
format!("// file {i}\nfn f{i}() {{}}\n"),
|
|
)
|
|
.unwrap();
|
|
}
|
|
let first = ingest_collecting(&env);
|
|
assert!(
|
|
matches!(sweep_bounds(&first), (None, None)),
|
|
"a first ingest has no stored paths outside its own scan, so there \
|
|
is no sweep phase to announce: {first:?}"
|
|
);
|
|
|
|
for i in 0..4 {
|
|
std::fs::remove_file(env.workspace_root.join(format!("gone{i}.rs"))).unwrap();
|
|
}
|
|
let second = ingest_collecting(&env);
|
|
|
|
let (total, done) = sweep_bounds(&second);
|
|
let total = total.expect("second ingest must announce the sweep phase");
|
|
let (checked, purged) = done.expect("and must announce its end");
|
|
assert_eq!(total, 4, "the four deleted files are the candidates");
|
|
assert_eq!(checked, total, "every announced candidate is examined");
|
|
assert_eq!(purged, 4, "all four are truly gone");
|
|
|
|
// A denominator is only useful if the numerator actually walks it.
|
|
let mut indices: Vec<u32> = second
|
|
.iter()
|
|
.filter_map(|e| match e {
|
|
IngestEvent::SweepProgress { idx, total: t, .. } => {
|
|
assert_eq!(*t, total, "every progress event carries the same total");
|
|
Some(*idx)
|
|
}
|
|
_ => None,
|
|
})
|
|
.collect();
|
|
indices.sort_unstable();
|
|
assert_eq!(
|
|
indices,
|
|
(1..=total).collect::<Vec<_>>(),
|
|
"one event per candidate, 1-based and contiguous: {second:?}"
|
|
);
|
|
|
|
// Ordering is what makes the phase legible: the bar has to be told the
|
|
// length before it is told a position.
|
|
let start_at = second
|
|
.iter()
|
|
.position(|e| matches!(e, IngestEvent::SweepStarted { .. }))
|
|
.unwrap();
|
|
let end_at = second
|
|
.iter()
|
|
.position(|e| matches!(e, IngestEvent::SweepCompleted { .. }))
|
|
.unwrap();
|
|
let first_progress = second
|
|
.iter()
|
|
.position(|e| matches!(e, IngestEvent::SweepProgress { .. }))
|
|
.unwrap();
|
|
assert!(
|
|
start_at < first_progress && first_progress < end_at,
|
|
"SweepStarted < SweepProgress* < SweepCompleted: {second:?}"
|
|
);
|
|
}
|
|
|
|
/// The reported case was a sweep that purged thousands of documents, but
|
|
/// the same silence happens when a narrowed `include` glob leaves stored
|
|
/// paths out of scope: the sweep still stats every one of them and purges
|
|
/// none. "checked 12115, purged 0" is precisely the answer the user was
|
|
/// missing, so a zero-purge sweep must still announce itself.
|
|
#[test]
|
|
fn a_sweep_that_purges_nothing_still_announces_itself() {
|
|
let env = TestEnv::lexical_only();
|
|
std::fs::write(
|
|
env.workspace_root.join("kept.rs"),
|
|
"// still here\nfn kept() {}\n",
|
|
)
|
|
.unwrap();
|
|
ingest_collecting(&env);
|
|
|
|
// Narrow the scan so the fixtures are stored but out of scope. They
|
|
// are still on disk, so the sweep must examine each and leave it alone.
|
|
let narrowed = SourceScope {
|
|
root: env.workspace_root.clone(),
|
|
include: vec!["kept.rs".to_string()],
|
|
exclude: env.config.workspace.exclude.clone(),
|
|
};
|
|
let (tx, rx) = mpsc::channel::<IngestEvent>();
|
|
ingest_with_config(
|
|
env.config.clone(),
|
|
narrowed,
|
|
IngestOpts {
|
|
progress: Some(tx),
|
|
..Default::default()
|
|
},
|
|
)
|
|
.expect("narrowed ingest must succeed");
|
|
let events: Vec<IngestEvent> = rx.into_iter().collect();
|
|
|
|
let (total, done) = sweep_bounds(&events);
|
|
let total = total.expect("the phase is announced even when it purges nothing");
|
|
let (checked, purged) = done.expect("and it reports its end");
|
|
assert!(total > 0, "the out-of-scope fixtures are candidates");
|
|
assert_eq!(checked, total);
|
|
assert_eq!(purged, 0, "nothing was deleted from disk: {events:?}");
|
|
assert!(
|
|
events
|
|
.iter()
|
|
.any(|e| matches!(e, IngestEvent::SweepProgress { removed: false, .. })),
|
|
"and the candidates it left alone are still reported: {events:?}"
|
|
);
|
|
}
|
|
|
|
/// The CLI's first Ctrl-C prints "aborting after current asset" and flips
|
|
/// the cancel flag. The sweep did not look at it, so during a long sweep
|
|
/// that message was simply untrue — and the user's only recourse was a
|
|
/// second Ctrl-C, which is `exit(130)` and strands whatever vector deletes
|
|
/// are buffered. Issue #228 is a report of someone pressing Ctrl-C three
|
|
/// times in exactly this phase.
|
|
#[test]
|
|
fn a_cancelled_sweep_stops_and_reports_only_what_it_examined() {
|
|
let env = TestEnv::lexical_only();
|
|
for i in 0..6 {
|
|
std::fs::write(
|
|
env.workspace_root.join(format!("bye{i}.rs")),
|
|
format!("// file {i}\nfn g{i}() {{}}\n"),
|
|
)
|
|
.unwrap();
|
|
}
|
|
ingest_collecting(&env);
|
|
for i in 0..6 {
|
|
std::fs::remove_file(env.workspace_root.join(format!("bye{i}.rs"))).unwrap();
|
|
}
|
|
|
|
// Pre-cancelled: the sweep must stop at the top of its first
|
|
// iteration rather than walk every candidate.
|
|
let cancel = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(true));
|
|
let (tx, rx) = mpsc::channel::<IngestEvent>();
|
|
let report = 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_eq!(
|
|
report.purged_deleted_files, 0,
|
|
"nothing is purged after the flag is already set: {report:?}"
|
|
);
|
|
let (total, done) = sweep_bounds(&events);
|
|
assert!(
|
|
total.expect("the phase is still announced") > 0,
|
|
"the candidates were counted before the check: {events:?}"
|
|
);
|
|
let (checked, purged) = done.expect("and it still reports its end");
|
|
assert_eq!(
|
|
(checked, purged),
|
|
(0, 0),
|
|
"a cancelled sweep must not claim work it did not do — reporting \
|
|
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()
|
|
);
|
|
}
|