chore: PR #236 회차 2 리뷰 반영 — sweep 취소 협조 + 바 복구 회귀 가드
2회차 리뷰가 1회차 지적 6건 모두 실질 해결됐음을 확인하고 머지 가능으로 결론냈다. 새로 나온 것 중 값이 있는 것을 반영한다. 1) sweep 이 취소 플래그를 보지 않았다 (MEDIUM) CLI 의 첫 Ctrl-C 는 "aborting after current asset" 을 찍고 AtomicBool 을 세운다. 그런데 `sweep_deleted_files` 는 그걸 인자로 받지도 검사하지도 않았다. 즉 긴 sweep 중에는 그 안내가 사실이 아니었고, 사용자에게 남은 수단은 두 번째 Ctrl-C 뿐인데 그건 `exit(130)` 이라 버퍼에 쌓인 벡터 삭제(최대 5,000)가 고아로 남는다. 이슈 #228 자체가 "sweep 중 Ctrl-C 를 세 번 눌러 죽였다" 는 보고다. 눌렀을 때의 동작을 그대로 둔 채 표시만 고치는 건 절반만 고친 것이다. 루프 상단에서 검사하고 break 한다. `sweep_completed.checked` 는 예고한 `total` 이 아니라 실제 검사한 수로 나간다 — total 을 그대로 쓰면 하지 않은 일을 했다고 보고하는 셈이다. `a_cancelled_sweep_stops_and_reports_ only_what_it_examined` 로 고정. 2) 1회차 HIGH 가 테스트로 고정되지 않았다 (MEDIUM) 바 상태 오염은 "phase 를 하나 더 추가하면 또 밟는" 유형인데 회귀 가드가 없었다. 실제로 한 번 배포될 뻔했다. `sweep_hands_the_bar_back_to_the_ asset_loop` 이 sweep 중에는 후보 수를, 끝난 뒤에는 asset 수를 세는지 본다. `ProgressMode::Human { tty: false, quiet: true }` 면 바가 hidden draw target 으로 살아 있어 터미널 없이도 상태를 볼 수 있다. 3) 잔가지 (LOW) - `SweepProgress` 독 주석이 `removed` 를 두 경우로만 설명했다. 이번 PR 이 만든 세 번째 경우(purge 실패)가 빠져 있었다. 스키마 쪽은 이미 맞게 적혀 있었다. - `sweep_deleted_files` 의 기존 독 주석이 "purge 실패가 per-file 레벨 에서 error 로 집계된다" 고 적었는데 사실이 아니다. 어떤 카운터에도 안 들어가고 `IngestReport.errors` 에도 반영되지 않는다. 실패 경로를 손댄 김에 고쳤다. - HOTFIXES 와 DOGFOOD 가 `purge_failed` 와 취소 동작을 언급하지 않았다. HOTFIXES 가 live SoT 인데 실제 표면보다 좁게 적힌 상태였다. 미반영: `SweepCompleted` 가 TTY 에서 `bar.println` 대신 stderr 로 직접 쓰는 것(기존 `AssetTimings`/`PdfOcr*` 과 같은 패턴이라 신규 회귀가 아니고, 바꾸려면 셋을 같이 옮겨야 한다). `ScanCompleted` 에서 스타일을 길이보다 먼저 세팅하는 순서가 이론상 `0/u64::MAX` 프레임을 허용한다는 지적 — 창이 마이크로초이고 `SweepCompleted` 쪽에서는 새 순서가 더 낫다. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017c9JwQq8ZkGvYjpKXMiDhF
This commit is contained in:
@@ -300,6 +300,7 @@ pub fn ingest_with_config(
|
||||
vector_store.as_ref().map(std::convert::AsRef::as_ref),
|
||||
progress,
|
||||
log_writer.as_ref(),
|
||||
&cancelled,
|
||||
)?;
|
||||
|
||||
let started_at = time::OffsetDateTime::now_utc();
|
||||
@@ -2123,16 +2124,18 @@ fn store_document_records(
|
||||
///
|
||||
/// Returns the number of documents purged.
|
||||
///
|
||||
/// Non-fatal design: individual purge failures are logged and counted
|
||||
/// as errors on the per-file level but do NOT abort the sweep — a
|
||||
/// partial failure is preferable to blocking the rest of ingest. The
|
||||
/// return value only counts successful purges.
|
||||
/// Non-fatal design: an individual purge failure is logged (tracing plus
|
||||
/// a `purge_failed` ndjson line) and skipped, and does NOT abort the
|
||||
/// sweep — a partial failure is preferable to blocking the rest of
|
||||
/// ingest. It is not counted in `IngestReport.errors`, which tracks the
|
||||
/// per-asset loop; the return value counts successful purges only.
|
||||
fn sweep_deleted_files(
|
||||
app: &App,
|
||||
scanned_paths: &std::collections::HashSet<kebab_core::WorkspacePath>,
|
||||
vector_store: Option<&kebab_store_vector::LanceVectorStore>,
|
||||
progress: Option<&std::sync::mpsc::Sender<crate::ingest_progress::IngestEvent>>,
|
||||
log_writer: Option<&Arc<Mutex<crate::ingest_log::IngestLogWriter>>>,
|
||||
cancelled: &dyn Fn() -> bool,
|
||||
) -> anyhow::Result<u32> {
|
||||
use kebab_core::DocumentStore as _;
|
||||
|
||||
@@ -2181,8 +2184,19 @@ fn sweep_deleted_files(
|
||||
// magnitude.
|
||||
let mut doomed_chunk_ids: Vec<kebab_core::ChunkId> = Vec::new();
|
||||
|
||||
let mut examined: u32 = 0;
|
||||
for (i, stored_path) in candidates.into_iter().enumerate() {
|
||||
// The CLI's first Ctrl-C prints "aborting after current asset"
|
||||
// and flips this flag. Before this check the sweep ignored 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 is buffered below. Issue #228 is literally
|
||||
// a report of someone pressing Ctrl-C three times here.
|
||||
if cancelled() {
|
||||
break;
|
||||
}
|
||||
let idx = u32::try_from(i + 1).unwrap_or(u32::MAX);
|
||||
examined = idx;
|
||||
// Resolve to an absolute path and check existence on disk.
|
||||
// Use `try_exists` + `unwrap_or(true)` so transient FS errors
|
||||
// (EACCES on a path we lack read on, NFS hiccups, ownership
|
||||
@@ -2288,7 +2302,7 @@ fn sweep_deleted_files(
|
||||
{
|
||||
let _ = w.write_event(&crate::ingest_log::LogEvent::SweepSummary {
|
||||
ts: crate::ingest_log::now_ts(),
|
||||
checked: total,
|
||||
checked: examined,
|
||||
purged,
|
||||
ms,
|
||||
});
|
||||
@@ -2296,7 +2310,10 @@ fn sweep_deleted_files(
|
||||
crate::ingest_progress::emit(
|
||||
progress,
|
||||
crate::ingest_progress::IngestEvent::SweepCompleted {
|
||||
checked: total,
|
||||
// The candidates actually examined, which is short of `total`
|
||||
// when the run was cancelled mid-sweep. Reporting `total`
|
||||
// there would claim work that did not happen.
|
||||
checked: examined,
|
||||
purged,
|
||||
ms,
|
||||
},
|
||||
|
||||
@@ -148,8 +148,9 @@ pub enum IngestEvent {
|
||||
/// whole sweep and users read it as a hang and killed the run.
|
||||
SweepStarted { total: u32 },
|
||||
/// v0.32.1 (additive): the `idx`-th sweep candidate (1-based) has been
|
||||
/// examined. `removed` distinguishes "file really is gone, its document
|
||||
/// was purged" from "still on disk, left alone" — a sweep can walk
|
||||
/// examined. `removed` is true only when the file really was gone and
|
||||
/// its document was purged; it is false both for a file still on disk
|
||||
/// (left alone) and for a purge that failed. A sweep can walk
|
||||
/// thousands of candidates and purge none, and a bar that only moved
|
||||
/// on purges would look frozen in exactly that case. (Named `removed`
|
||||
/// rather than `purged` so the wire key keeps one type: `purged` is
|
||||
@@ -161,7 +162,8 @@ pub enum IngestEvent {
|
||||
removed: bool,
|
||||
},
|
||||
/// v0.32.1 (additive): sweep finished. `checked` candidates examined,
|
||||
/// `purged` documents removed, `ms` wall-clock.
|
||||
/// `purged` documents removed, `ms` wall-clock. `checked` falls short
|
||||
/// of the announced `total` when the run was cancelled mid-sweep.
|
||||
SweepCompleted { checked: u32, purged: u32, ms: u64 },
|
||||
/// Run finished normally. `counts` is the final aggregate.
|
||||
Completed { counts: AggregateCounts },
|
||||
|
||||
@@ -160,3 +160,58 @@ fn a_sweep_that_purges_nothing_still_announces_itself() {
|
||||
"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:?}"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -547,6 +547,71 @@ pub(crate) fn now_rfc3339() -> anyhow::Result<String> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn feed(events: &[IngestEvent]) -> ProgressDisplay {
|
||||
// quiet + non-tty: the bar exists with a hidden draw target, so
|
||||
// its state is observable without a terminal and nothing is
|
||||
// printed to the test's stderr.
|
||||
let mut d = ProgressDisplay::new(ProgressMode::Human {
|
||||
tty: false,
|
||||
quiet: true,
|
||||
});
|
||||
for e in events {
|
||||
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 {
|
||||
root: "/ws".to_string(),
|
||||
},
|
||||
IngestEvent::ScanCompleted { total: 17 },
|
||||
IngestEvent::SweepStarted { total: 3 },
|
||||
IngestEvent::SweepProgress {
|
||||
idx: 1,
|
||||
total: 3,
|
||||
path: "gone.md".to_string(),
|
||||
removed: true,
|
||||
},
|
||||
]);
|
||||
assert_eq!(
|
||||
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
||||
Some(3),
|
||||
"during the sweep the bar counts sweep candidates"
|
||||
);
|
||||
|
||||
let d = feed(&[
|
||||
IngestEvent::ScanStarted {
|
||||
root: "/ws".to_string(),
|
||||
},
|
||||
IngestEvent::ScanCompleted { total: 17 },
|
||||
IngestEvent::SweepStarted { total: 3 },
|
||||
IngestEvent::SweepCompleted {
|
||||
checked: 3,
|
||||
purged: 3,
|
||||
ms: 12,
|
||||
},
|
||||
]);
|
||||
assert_eq!(
|
||||
d.bar.as_ref().and_then(indicatif::ProgressBar::length),
|
||||
Some(17),
|
||||
"and once it ends the bar counts assets again"
|
||||
);
|
||||
assert_eq!(
|
||||
d.bar.as_ref().map(indicatif::ProgressBar::position),
|
||||
Some(0),
|
||||
"from the start, not from wherever the sweep left off"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn from_flags_json_takes_priority_over_tty() {
|
||||
assert_eq!(
|
||||
|
||||
@@ -276,7 +276,8 @@ echo "# stdin content" | "$RELEASE_BIN" ingest-stdin --title "from stdin" --conf
|
||||
- aggregate `counts` on `completed` / `aborted`.
|
||||
- sweep 구간: `sweep_started.total` 이 후보 수와 맞고, `sweep_progress.idx` 가 1..=total 로 연속이며, `sweep_completed.checked == total`.
|
||||
- sweep 이 끝난 뒤 진행바가 asset 분모·라벨로 돌아오는가 (TTY). sweep 이 같은 바를 빌려 쓰므로 복구가 빠지면 색인 구간 내내 `sweep [..] 4213/21` 로 그려진다.
|
||||
- ndjson 로그에 `purge` 줄과 `sweep_summary` 가 남는가 (이슈 #228 이전에는 이 구간이 0바이트였다).
|
||||
- ndjson 로그에 `purge` 줄과 `sweep_summary` 가 남는가 (이슈 #228 이전에는 이 구간이 0바이트였다). purge 실패 시 `purge_failed`.
|
||||
- sweep 중 Ctrl-C 한 번에 실제로 멈추는가. `sweep_completed.checked` 가 예고한 `total` 이 아니라 실제 검사한 수로 나와야 한다.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -32,7 +32,9 @@ git history.
|
||||
|
||||
`purged` 를 두 이벤트에서 다른 타입으로 쓰지 않으려고 `sweep_progress` 쪽은 `removed`(bool) 로, `sweep_completed` 쪽은 `purged`(정수) 로 이름을 나눴다. 같은 wire 키가 두 타입을 갖는 건 소비자 입장에서 함정이다.
|
||||
|
||||
ndjson 로그에는 `purge { ts, doc_path }` 와 `sweep_summary { ts, checked, purged, ms }` 를 추가했다. 이슈 제안 2 가 요청한 형태 그대로다. 로그가 유일한 사후 기록이다 — tracing 은 stderr 로 흘러가고 남지 않는다.
|
||||
ndjson 로그에는 `purge { ts, doc_path }` 와 `sweep_summary { ts, checked, purged, ms }` 를 추가했다. 이슈 제안 2 가 요청한 형태 그대로다. 로그가 유일한 사후 기록이다 — tracing 은 stderr 로 흘러가고 남지 않는다. 리뷰 지적을 받아 `purge_failed { ts, doc_path, message }` 도 넣었다. 실패는 진행 이벤트에서 "디스크에 남아 있어 그냥 뒀다" 와 똑같이 `removed: false` 로 나가고, `sweep_summary` 의 `checked - purged` 차이로도 못 가르기 때문이다.
|
||||
|
||||
sweep 루프가 취소 플래그도 본다. CLI 의 첫 Ctrl-C 는 "aborting after current asset" 을 찍고 플래그를 세우는데, sweep 은 그걸 보지 않아서 **긴 sweep 중에는 그 안내가 사실이 아니었다**. 사용자에게 남은 수단은 두 번째 Ctrl-C 뿐이고 그건 `exit(130)` 이라 버퍼에 쌓인 벡터 삭제가 고아로 남는다. 이 이슈 자체가 "sweep 중 Ctrl-C 를 세 번 눌러 죽였다" 는 보고다. 취소로 중단하면 `checked` 는 실제로 검사한 수로 나간다 — 예고한 `total` 을 그대로 쓰면 하지 않은 일을 했다고 보고하는 셈이다.
|
||||
|
||||
CLI 는 sweep 을 asset 진행바와 **별 phase** 로 그린다. 후보 12k 를 훑는 일과 asset 12k 를 색인하는 일은 분모가 다른 별개의 작업이라, 카운터를 공유하면 두 번째 구간이 처음부터 다시 시작하는 것처럼 보인다. 비-TTY 는 실제로 지운 것만 줄로 찍는다 — 검사한 후보마다 한 줄이면 그대로 둔 경로들이 run 의 진짜 출력을 덮는다.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user