Skip to content

feat(corpus): --verbose to trace phases and stream way-embed progress#354

Merged
aaronsb merged 3 commits into
mainfrom
fix/corpus-verbose
Jul 10, 2026
Merged

feat(corpus): --verbose to trace phases and stream way-embed progress#354
aaronsb merged 3 commits into
mainfrom
fix/corpus-verbose

Conversation

@aaronsb

@aaronsb aaronsb commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • A quiet ways corpus build is silent through every project scan and all three way-embed passes. way-embed generate already prints [n/total] <way-id> per way, but corpus.rs routed the child's stderr to /dev/null on every non-Windows platform — so a slow pass and a wedged pass looked identical. That is how a macOS build came to be reported as "hangs building the combined corpus" with nothing to go on.
  • --verbose stamps each phase with elapsed-since-start and prints it before the step it announces (stderr is unbuffered), so when the process wedges, the last line on screen names the step that wedged it. It inherits the child's stderr — pinning a stall to a single way id — and echoes each child's argv so that pass can be rerun standalone. Per-project resolution is traced too, since resolve_project_path probes is_dir() across candidate splits and an unreachable mount would stall there.
  • Overrides --quiet. Default output is unchanged.

Notes for the reviewer

The macOS hang itself is not fixed here, and not yet diagnosed. I ruled out the three mechanisms I could check by reading, so this ships the instrument to get evidence off the affected box:

  • stdin/stdout pipe deadlock in batch_similarity — ruled out: cmd_similarity calls fflush(stdout) per line and fgets-drains stdin, and the probe corpus emits ~1KB, far under any pipe buffer.
  • Metal backend init — ruled out: CMakeLists.txt forces GGML_METAL OFF, so macOS is a plain CPU build.
  • Long-line overflow in load_corpus — ruled out: MAX_LINE is 65536, longest observed corpus line is 4527 bytes.

Standing suspicion, now traceable: the combined pass re-embeds every entry the en and multi passes already embedded (~2x the work), and it runs last — so it is the longest silence, right where the hang was reported.

Test plan

  • cargo test -p ways — 346 pass, 0 fail
  • cargo clippy -p ways --all-targets — exits 0 (one pre-existing warning in an unrelated settings provenance test)
  • ways corpus --verbose --ways-dir ~/.claude/hooks/ways --output <scratch> — phase markers appear, [n/total] streams, both calibration lanes named, timings print
  • Plain ways corpus — grepped for trace lines and timing suffixes: zero leaks, output identical to before
  • -q alone — silent (0 lines); -q -v together — verbose wins (192 trace lines)
  • Canonical corpus at ~/.cache/agent-ways/user never touched (all runs used --output)

…ress

A quiet corpus build is silent through every project scan and all three
way-embed passes. `way-embed generate` already prints `[n/total] <way-id>`
per way, but corpus.rs routed the child's stderr to /dev/null on every
non-Windows platform, so a slow pass and a wedged pass looked identical.
That is how a macOS build came to be reported as "hangs at the combined
corpus" with nothing to go on.

--verbose stamps each phase with elapsed-since-start and prints it *before*
the step it announces, so when the process wedges the last line on screen
names the step that wedged it. It inherits the child's stderr, pinning a
stall to a single way id, and echoes each child's argv so that pass can be
rerun standalone. Per-project resolution is traced too, since that is where
an unreachable mount would stall.

Overrides --quiet: a diagnostic you must un-silence twice is not a
diagnostic. Default (non-verbose) output is byte-for-byte unchanged.

The three generate passes are consolidated into one run_generate helper,
and the Windows NUL-device workaround moves into embed_stderr().
@aaronsb

aaronsb commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Review: --verbose corpus tracing

Reviewed against the four focus areas. No blocking issues. The refactor is behavior-preserving on the target platforms and the error-handling collapse was un-collapsed correctly. One Windows-only behavior change is worth a note. Details below.

1. Subprocess stdio — Windows cfg semantics: preserved for generate, subtly changed for batch_similarity

embed_stderr(verbose) reproduces the old inline closure exactly for the non-verbose generate path:

  • verbose -> inherit() (both platforms)
  • non-verbose + windows -> inherit() (was inherit())
  • non-verbose + non-windows -> null() (was null())

So the three run_generate call sites are byte-for-byte equivalent to the originals when not verbose. Good.

One real change, in batch_similarity: on main that function used an unconditional Stdio::null() on all platforms — it never saw the embed_stderr closure, which was local to auto_embed. Now it routes through embed_stderr(verbose), so on Windows, non-verbose it flips null() -> inherit(). On macOS/Linux (the platforms in play) non-verbose stays null(), identical. The Windows flip is arguably a latent-bug fix — it's exactly the MSVC-C-runtime-aborts-on-NUL case the code comments warn about — but it is a behavior change on a platform you can't test from the affected box. Worth a one-line mention in the PR body so it's a deliberate decision on record, not an incidental side effect.

Deadlock: still safe. The change cannot introduce one. batch_similarity uses wait_with_output(), and when stderr is inherit()/null() (not piped) child.stderr is None, so wait_with_output only drains stdout — inherited stderr flows straight to the parent fd and never participates in the read loop. The only real hazard here is the pre-existing "write all of stdin, then read stdout" ordering, which can wedge if the child fills the stdout pipe buffer before draining stdin — but that's unchanged by this PR and, per your own note, the ~1KB probe output is far under any pipe buffer. So: no regression, and no new deadlock surface.

2. Behavior preservation — non-verbose output is byte-identical

elapsed_suffix(t, false) returns String::new(), so format!(" EN embeddings: {}{}", path, "") collapses to the original format!(" EN embeddings: {}", path). Same for the Multi and Combined lines. let quiet = quiet && !verbose; leaves quiet untouched when verbose is false, so the log gate is unchanged. Confirmed clean — no timing suffix or trace line leaks into a plain build.

3. Error-handling — the collapsed _ arm was split correctly, nothing goes silent

Original: _ => eprintln!("WARNING: ... failed") caught both Ok(non-success) and Err(spawn). run_generate now splits them:

  • Ok(s) if s.success() -> Some(t) -> caller logs success (unchanged)
  • Ok(s) -> WARNING: {what} embedding generation failed ({s}) -> None
  • Err(e) -> WARNING: {what} embedding generation could not start: {e} -> None

Both former _ cases still warn (now with more detail — exit status / spawn error text), and success still logs. Nothing that previously warned now silently succeeds, and nothing that previously succeeded now warns. what values ("EN", "multilingual", "combined") reproduce the original message stems. No regression — this is a strict improvement in diagnosability.

4. Closure captures / &dyn Fn threading — correct

vlog closes over started: Instant (Copy) and verbose by shared borrow, self-gates on verbose, and coerces cleanly to &dyn Fn(&str) at the &vlog call sites. auto_embed / fit_calibration / run_generate receive it as &dyn Fn(&str) and forward the same reference through — no re-borrow or lifetime issue. elapsed_suffix takes Instant by value (Copy), fine.

Minor (non-blocking)

  • verbose is threaded two ways at once: captured inside the vlog closure and passed as an explicit bool to auto_embed/fit_calibration/run_generate/elapsed_suffix/embed_stderr/batch_similarity. The explicit param is genuinely needed (for embed_stderr and elapsed_suffix, which aren't the log closure), so this isn't removable without more plumbing — just flagging the slight redundancy for anyone who later wonders why both exist.
  • Cosmetic: the en-model probe logs MISSING while the multi-model logs absent for the same is_file() == false condition. Intentional (en absence is fatal-ish, multi is expected-optional), but a reader skimming trace output might read them as different states.

Assessment

Tight, well-motivated instrumentation PR. The refactor removes three duplicated spawn blocks in favor of one run_generate, and the dedup is faithful — I checked each collapsed branch against main and the non-verbose paths are equivalent. The only thing I'd surface explicitly is the Windows batch_similarity stderr flip (item 1); everything else is either identical or a clean improvement. Ship it once you've decided the Windows change is intentional.

…atforms

Review of #354 caught that batch_similarity never used auto_embed's inline
Windows NUL workaround — on main its stderr was an unconditional Stdio::null().
Routing it through the shared embed_stderr() would have flipped Windows
non-verbose from null() to inherit(), an unverifiable behavior change on a
platform this PR cannot test.

Quiet stays null() everywhere, exactly as before. --verbose only adds the
inherit case. Whether Windows should use inherit() here (the MSVC NUL-abort
the auto_embed comment warns about) is a real question, tracked separately.
@aaronsb

aaronsb commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Remediated in 3b53410.

Finding 1 (Windows stderr flip) — fixed, not documented-as-deliberate. Good catch, and I disagree with the "note it in the PR body" disposition: an unverifiable behavior change on an untestable platform doesn't belong in a diagnostic PR at all, whether or not it's disclosed. batch_similarity no longer routes through embed_stderr(). Quiet stays Stdio::null() on every platform, exactly as main had it; --verbose only adds the inherit() case.

The underlying question you surfaced — if the MSVC NUL-abort comment is accurate, then calibration has been silently never fitting on Windows — is real and worth answering on actual hardware. Filed as #355 rather than guessed at here.

Findings 2, 3, 4 — agreed, no action needed.

Minor: MISSING vs absent. Keeping the asymmetry; it's load-bearing rather than cosmetic. The EN model is required (its absence is why auto_embed bails with the ADR-125 make setup error), while the multilingual model is optional and fetched on demand per ADR-139. Same is_file() == false, genuinely different states — one is a broken install, the other is the default install.

Minor: verbose threaded twice. Confirmed not removable, as you note — embed_stderr() and elapsed_suffix() need the bare bool. Leaving it.

Re-verified after the fix: 346 tests pass, clippy clean on corpus.rs, both calibration lanes still fit (AUC=0.941 en / 0.943 multi), and non-verbose output has zero trace/timing/progress leakage.

@aaronsb aaronsb merged commit 6b05583 into main Jul 10, 2026
11 checks passed
@aaronsb aaronsb deleted the fix/corpus-verbose branch July 10, 2026 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant