diff --git a/crates/travsr-cli/src/embed.rs b/crates/travsr-cli/src/embed.rs index d0e4c81f..9671383e 100644 --- a/crates/travsr-cli/src/embed.rs +++ b/crates/travsr-cli/src/embed.rs @@ -120,6 +120,17 @@ pub enum EmbedCommand { /// nearest git root). #[arg(long)] db: Option, + /// Rebuild every embedding from scratch, replacing the whole index. + /// + /// Use this after switching the embedding model, when older embeddings + /// may be inconsistent with newer ones. Slower than a normal reindex, + /// which only fills in what is missing. + /// + /// Cannot be combined with `--phase1`: the sidecar clears the whole + /// model before re-embedding, so a phase-restricted rebuild would delete + /// the phase 2 tier and never refill it. + #[arg(long, conflicts_with = "phase1")] + rebuild: bool, /// Only embed symbol nodes with shell_number >= N (Phase 1 high-centrality pass). /// Omit to embed all pending nodes. #[arg(long)] @@ -230,6 +241,7 @@ pub fn run(cmd: EmbedCommand) -> Result<()> { } EmbedCommand::Reindex { db, + rebuild, phase1, capacity, jobs, @@ -240,7 +252,7 @@ pub fn run(cmd: EmbedCommand) -> Result<()> { max_workers: jobs, priority: priority.map(Into::into), }; - cmd_reindex(db, phase1, overrides) + cmd_reindex(db, phase1, rebuild, overrides) } EmbedCommand::Reconfigure { db, @@ -595,7 +607,6 @@ fn cmd_init( /// "Full" default). Only called on an interactive terminal. fn prompt_cpu_budget() -> Result> { use std::io::Write as _; - use travsr_plugin_host::Capacity; let cores = std::thread::available_parallelism() .map(|n| n.get()) @@ -617,28 +628,52 @@ fn prompt_cpu_budget() -> Result> { let mut input = String::new(); std::io::stdin().read_line(&mut input)?; - match input.trim() { - "" | "1" => Ok(None), // Full → defer to config/default (100%) - "2" => Ok(Some(Capacity::Percent(50))), - "3" => Ok(Some(Capacity::Percent(25))), - "4" => Ok(Some(Capacity::Auto)), - "5" => { - print!(" Percent (1-100): "); - std::io::stdout().flush()?; - let mut pct = String::new(); - std::io::stdin().read_line(&mut pct)?; - match Capacity::parse(pct.trim()) { - Some(c) => Ok(Some(c)), - None => { - println!(" (not a valid percent, using Full)"); - Ok(None) - } - } - } - _ => { - println!(" (unrecognised, using Full)"); - Ok(None) - } + let choice = input.trim().to_string(); + + // The custom (percent) sub-prompt is the only branch that needs a second + // line of input; read it here so the parse itself stays pure/testable. + let custom = if choice == "5" { + print!(" Percent (1-100): "); + std::io::stdout().flush()?; + let mut pct = String::new(); + std::io::stdin().read_line(&mut pct)?; + pct.trim().to_string() + } else { + String::new() + }; + + let parsed = parse_cpu_choice(&choice, &custom); + match choice.as_str() { + "" | "1" | "2" | "3" | "4" => {} + // parsed is None on these two arms, which means "no explicit choice, + // fall through to config/env/default" (F1) rather than Full, so the + // message must not claim Full or it repeats the very bug F1 fixed. + "5" if parsed.is_none() => println!(" (not a valid percent, using the configured budget)"), + "5" => {} + _ => println!(" (unrecognised, using the configured budget)"), + } + Ok(parsed) +} + +/// Pure parse of the CPU-budget menu selection, split out of +/// [`prompt_cpu_budget`] so it is testable without a TTY. `choice` is the +/// top-level answer; `custom` is the percent entered for the "5" (Custom) +/// path and is ignored otherwise. +/// +/// `None` means "user made no explicit choice" — fall through to +/// config/env/default. An explicit Full ("1"/Enter) returns `Percent(100)` so +/// the just-made interactive selection overrides ambient global config (F1); +/// returning `None` there is the bug, because `embed.capacity = auto` in config +/// would then silently win over the user's Full choice. +fn parse_cpu_choice(choice: &str, custom: &str) -> Option { + use travsr_plugin_host::Capacity; + match choice { + "" | "1" => Some(Capacity::Percent(100)), + "2" => Some(Capacity::Percent(50)), + "3" => Some(Capacity::Percent(25)), + "4" => Some(Capacity::Auto), + "5" => Capacity::parse(custom), + _ => None, } } @@ -1020,7 +1055,7 @@ fn reindex_after_init( let gov = travsr_plugin_host::resolve_governance_for_db(db_path, overrides); println!(" {}", reindex_banner(workers, &gov)); - run_reindex_with_progress(db_path, None, overrides)?; + run_reindex_with_progress(db_path, None, false, overrides)?; let embedded = query_embed_stats(db_path, &backend.id) .map(|s| s.stats.embedded) @@ -1460,10 +1495,11 @@ fn reindex_banner(workers: usize, gov: &travsr_plugin_host::EmbedGovernance) -> fn cmd_reindex( db_override: Option, phase1: Option, + rebuild: bool, overrides: travsr_plugin_host::EmbedOverrides, ) -> Result<()> { let db_path = resolve_graph_db(db_override)?; - run_reindex_locked(&db_path, phase1, &overrides) + run_reindex_locked(&db_path, phase1, rebuild, &overrides) } /// The locked reindex core: `embed.lock` guard, embed-text regen, resolved-budget @@ -1473,6 +1509,9 @@ fn cmd_reindex( fn run_reindex_locked( db_path: &Path, phase1: Option, + // `--rebuild`: clear + re-embed every node (a full rebuild), not just fill + // in what is missing. Always false on the reconfigure (WS4) path. + reembed: bool, overrides: &travsr_plugin_host::EmbedOverrides, ) -> Result<()> { // UX-015: fail fast when there is no backend to reindex with, before any @@ -1518,7 +1557,7 @@ fn run_reindex_locked( install_reindex_cancel_handler(db_path); // RFC-020: delegate to the parallel orchestrator with a live progress bar. - run_reindex_with_progress(db_path, phase1, overrides)?; + run_reindex_with_progress(db_path, phase1, reembed, overrides)?; if REINDEX_CANCELLED.load(Ordering::SeqCst) { println!( @@ -1653,7 +1692,7 @@ pub(crate) fn trigger_reindex_now( // A fresh reindex with the new budget. Blocks on embed.lock until the // cancelled run releases; resumes incrementally (no re-embed). - let result = run_reindex_locked(db_path, None, overrides); + let result = run_reindex_locked(db_path, None, false, overrides); if paused_daemon { if let Err(e) = crate::daemon_client::send_daemon_command( @@ -1678,6 +1717,7 @@ pub(crate) fn trigger_reindex_now( fn run_reindex_with_progress( db_path: &Path, phase1: Option, + reembed: bool, overrides: &travsr_plugin_host::EmbedOverrides, ) -> Result<()> { // Same fallback as `resolve_backend` (embed_catalog.rs): a repo indexed @@ -1715,7 +1755,8 @@ fn run_reindex_with_progress( }) }); - let result = travsr_plugin_host::run_parallel_reindex_blocking(db_path, phase1, overrides); + let result = + travsr_plugin_host::run_parallel_reindex_blocking(db_path, phase1, reembed, overrides); done_flag.store(true, Ordering::Relaxed); if let Some(m) = monitor { @@ -1921,18 +1962,16 @@ fn query_embed_stats(db_path: &std::path::Path, model_id: &str) -> Result String { - if nodes_per_sec < 1.0 || remaining == 0 { - return String::new(); - } - let secs = (remaining as f64 / nodes_per_sec).round() as u64; - if secs < 60 { - format!("~{secs}s remaining") - } else if secs < 3600 { - format!("~{}m remaining", secs / 60) - } else { - format!("~{}h {}m remaining", secs / 3600, (secs % 3600) / 60) +/// Display-only completion percentage, clamped to 100 (F4). The denominator +/// (`total`) is a tier snapshot that can lag the live `done` count as nodes +/// shift tiers, so `done > total` is possible; the raw `done/total` counts stay +/// honest, but the percentage must never read above 100%. An empty tier +/// (`total == 0`) reports 100% (nothing left to do). +fn pct_display(done: u64, total: u64) -> f64 { + if total == 0 { + return 100.0; } + ((done as f64 / total as f64) * 100.0).min(100.0) } fn fmt_count(n: u64) -> String { @@ -2144,16 +2183,7 @@ fn cmd_status() -> Result<()> { return Ok(()); } - let pct = stats.embedded as f64 / stats.total_symbols as f64 * 100.0; - // Phase 1 throughput ~400 nodes/sec (k8s: 109k nodes / 4.5 min). - // Phase 2 is background and ~10× slower on a loaded machine. - let nodes_per_sec: f64 = if stats.phase1_done < stats.phase1_total { - 400.0 - } else { - 40.0 - }; - let remaining = stats.total_symbols.saturating_sub(stats.embedded); - let eta = fmt_eta(remaining, nodes_per_sec); + let pct = pct_display(stats.embedded, stats.total_symbols); let pal = Palette::for_stream(std::io::stdout().is_terminal()); let bar = crate::progress::bar_of_width(pal, stats.embedded, stats.total_symbols, 36); @@ -2163,52 +2193,44 @@ fn cmd_status() -> Result<()> { fmt_count(stats.embedded), pct ); - if eta.is_empty() { + if stats.embedded >= stats.total_symbols { println!("{bar} done"); } else { - println!("{bar} {eta}"); + println!("{bar}"); } // ── per-phase breakdown ─────────────────────────────────────────────────── println!(); - let p1_pct = if stats.phase1_total > 0 { - stats.phase1_done as f64 / stats.phase1_total as f64 * 100.0 + let p1_pct = pct_display(stats.phase1_done, stats.phase1_total); + let p1_bar = crate::progress::bar_of_width(pal, stats.phase1_done, stats.phase1_total, 24); + let p1_done_marker = if stats.phase1_done >= stats.phase1_total { + " \u{2713} complete" } else { - 100.0 + "" }; - let p1_bar = crate::progress::bar_of_width(pal, stats.phase1_done, stats.phase1_total, 24); - let p1_eta = fmt_eta(stats.phase1_total.saturating_sub(stats.phase1_done), 400.0); println!( - "core symbols (centrality \u{2265}{threshold}) {} {}/{} ({:.0}%) {}", + "core symbols (centrality \u{2265}{threshold}) {} {}/{} ({:.0}%){}", p1_bar, fmt_count(stats.phase1_done), fmt_count(stats.phase1_total), p1_pct, - if p1_eta.is_empty() { - "\u{2713} complete".to_string() - } else { - p1_eta - }, + p1_done_marker, ); - let p2_pct = if stats.phase2_total > 0 { - stats.phase2_done as f64 / stats.phase2_total as f64 * 100.0 + let p2_pct = pct_display(stats.phase2_done, stats.phase2_total); + let p2_bar = crate::progress::bar_of_width(pal, stats.phase2_done, stats.phase2_total, 24); + let p2_done_marker = if stats.phase2_done >= stats.phase2_total { + " \u{2713} complete" } else { - 100.0 + "" }; - let p2_bar = crate::progress::bar_of_width(pal, stats.phase2_done, stats.phase2_total, 24); - let p2_eta = fmt_eta(stats.phase2_total.saturating_sub(stats.phase2_done), 40.0); println!( - "other symbols (centrality <{threshold}) {} {}/{} ({:.0}%) {}", + "other symbols (centrality <{threshold}) {} {}/{} ({:.0}%){}", p2_bar, fmt_count(stats.phase2_done), fmt_count(stats.phase2_total), p2_pct, - if p2_eta.is_empty() { - "\u{2713} complete".to_string() - } else { - p2_eta - }, + p2_done_marker, ); // ── HNSW index ──────────────────────────────────────────────────────────── @@ -2235,7 +2257,7 @@ fn cmd_status() -> Result<()> { "hint: no symbols embedded yet, the daemon starts embedding after semantic indexing." ); println!(" If the daemon is not running: travsr daemon start"); - } else if remaining > 0 { + } else if stats.embedded < stats.total_symbols { println!(); println!("hint: embedding is running in the background via the daemon."); println!(" Run `travsr embed status` again in a few minutes to see progress."); @@ -2775,3 +2797,72 @@ mod issue_755_tests { ); } } + +/// Track A: the CPU-budget menu parse (F1) and the clamped status percentage +/// (F4). Both are pure so they are tested without a TTY. +#[cfg(test)] +mod embed_ux_tests { + use super::*; + use travsr_plugin_host::Capacity; + + /// F1: an explicit Full ("1"/Enter) must resolve to `Percent(100)`, not + /// `None`. `None` defers to config, so a `None` here lets `embed.capacity = + /// auto` in global config silently override the just-made interactive choice + /// — the exact bug F1 fixes. + #[test] + fn full_choice_is_explicit_hundred_percent() { + assert_eq!(parse_cpu_choice("", ""), Some(Capacity::Percent(100))); + assert_eq!(parse_cpu_choice("1", ""), Some(Capacity::Percent(100))); + } + + /// `--rebuild` and `--phase1` must be mutually exclusive: the sidecar clears + /// the whole model before re-embedding, so a phase-restricted rebuild would + /// delete the phase 2 tier and never refill it. clap must reject the combo. + #[test] + fn rebuild_and_phase1_conflict() { + use clap::Parser; + #[derive(Parser)] + struct Wrap { + #[command(subcommand)] + cmd: EmbedCommand, + } + // Each alone parses. + assert!(Wrap::try_parse_from(["x", "reindex", "--rebuild"]).is_ok()); + assert!(Wrap::try_parse_from(["x", "reindex", "--phase1", "5"]).is_ok()); + // Together they are rejected. + let err = Wrap::try_parse_from(["x", "reindex", "--rebuild", "--phase1", "5"]) + .err() + .expect("--rebuild --phase1 must conflict"); + assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); + } + + #[test] + fn preset_choices_map_to_their_budgets() { + assert_eq!(parse_cpu_choice("2", ""), Some(Capacity::Percent(50))); + assert_eq!(parse_cpu_choice("3", ""), Some(Capacity::Percent(25))); + assert_eq!(parse_cpu_choice("4", ""), Some(Capacity::Auto)); + } + + #[test] + fn custom_choice_parses_its_percent() { + assert_eq!(parse_cpu_choice("5", "73"), Some(Capacity::Percent(73))); + // An unparseable custom percent yields no override (falls through to config). + assert_eq!(parse_cpu_choice("5", "abc"), None); + } + + #[test] + fn junk_makes_no_explicit_choice() { + assert_eq!(parse_cpu_choice("9", ""), None); + assert_eq!(parse_cpu_choice("xyz", ""), None); + } + + /// F4: a lagging tier denominator can make `done > total`; the displayed + /// percentage must never exceed 100%. Raw counts stay honest elsewhere. + #[test] + fn pct_display_clamps_and_handles_empty_tier() { + assert_eq!(pct_display(6943, 6716), 100.0); // overshoot clamps + assert_eq!(pct_display(0, 0), 100.0); // empty tier => nothing to do + assert_eq!(pct_display(50, 100), 50.0); // normal case + assert_eq!(pct_display(0, 100), 0.0); + } +} diff --git a/crates/travsr-cli/src/progress.rs b/crates/travsr-cli/src/progress.rs index b4b41517..7d8ea092 100644 --- a/crates/travsr-cli/src/progress.rs +++ b/crates/travsr-cli/src/progress.rs @@ -5,7 +5,7 @@ //! stays clean for the final summary), adapting to context: //! //! - **TTY**: a single self-updating line — a pulsing graph-node spinner, an -//! eighth-precision bar, `done/total`, percent, elapsed, and a rough ETA. +//! eighth-precision bar, `done/total`, percent, and elapsed time. //! Brand orange while working; the final summary node flips to fresh green. //! - **Non-TTY** (pipe/CI): occasional newline-terminated lines, no control //! chars or color. @@ -80,7 +80,7 @@ impl Palette { fn track(self, s: &str) -> String { self.paint("38;2;77;77;77", s) } - /// Muted secondary text (elapsed/eta/hints). + /// Muted secondary text (elapsed/hints). pub fn dim(self, s: &str) -> String { self.paint("2", s) } @@ -240,16 +240,12 @@ impl ProgressReporter { } InitProgress::Indexing { done, total, .. } => { let pct = (done * 100).checked_div(total).unwrap_or(0); - let tail = match eta(self.start, done, total) { - Some(e) => format!("{elapsed} · eta {}", fmt_dur(e)), - None => elapsed, - }; format!( " {spinner} indexing {} {}/{} {pct}% {}", bar(pal, pct), commas(done), commas(total), - pal.dim(&tail) + pal.dim(&elapsed) ) } InitProgress::Finalizing => { @@ -288,11 +284,8 @@ impl ProgressReporter { } InitProgress::Indexing { done, total, .. } => { let pct = (done * 100).checked_div(total).unwrap_or(0); - let eta = eta(self.start, done, total) - .map(|e| format!(" eta {}", fmt_dur(e))) - .unwrap_or_default(); format!( - "indexing {}/{} ({pct}%) {elapsed}{eta}", + "indexing {}/{} ({pct}%) {elapsed}", commas(done), commas(total) ) @@ -790,32 +783,6 @@ pub fn fmt_dur(d: Duration) -> String { } } -/// Minimum samples + elapsed window before an ETA is trustworthy. UX-005: -/// extrapolating from the first tick (1 file in 4 s) produced a 46-minute ETA on -/// a job that finished in 13 s, inviting a premature Ctrl-C. Withhold the -/// estimate until throughput has stabilised. -const ETA_WARMUP_FILES: u64 = 8; -const ETA_WARMUP_SECS: f64 = 2.0; - -/// Rough ETA from average throughput so far. `None` once done, at start, or -/// still inside the warm-up window (see [`ETA_WARMUP_FILES`]). -fn eta(start: Instant, done: u64, total: u64) -> Option { - if done == 0 || done >= total { - return None; - } - let secs = start.elapsed().as_secs_f64(); - // Warm-up floor: too few samples or too short a window still gives a wild - // extrapolation. Hold the ETA back and show only elapsed until then. - if done < ETA_WARMUP_FILES || secs < ETA_WARMUP_SECS { - return None; - } - let rate = done as f64 / secs; // files/sec - if rate <= 0.0 { - return None; - } - Some(Duration::from_secs_f64((total - done) as f64 / rate)) -} - /// #724 Finding 4: scip-java generates a `javac` wrapper that expands empty /// arrays under `set -u`; that is an "unbound variable" error in bash 3.2 (the /// default `/bin/bash` on macOS) but legal in bash 4.4+. When the `bash` @@ -881,29 +848,6 @@ mod tests { assert_eq!(fmt_dur(Duration::from_secs(3720)), "1h02m"); } - #[test] - fn eta_none_at_edges() { - let start = Instant::now(); - assert!(eta(start, 0, 100).is_none()); - assert!(eta(start, 100, 100).is_none()); - assert!(eta(start, 150, 100).is_none()); - } - - #[test] - fn eta_withheld_during_warmup() { - // UX-005: a fresh start with only a handful of files done must not emit an - // ETA — the sample count is below the warm-up floor. - let start = Instant::now(); - assert!( - eta(start, 1, 566).is_none(), - "one file in must be inside the warm-up window" - ); - assert!( - eta(start, ETA_WARMUP_FILES - 1, 566).is_none(), - "still under the file floor => no ETA" - ); - } - #[test] fn bar_width_is_constant_and_clamped() { // No color so we can measure visible cells directly. @@ -953,6 +897,29 @@ mod tests { ); assert_eq!(parse_bash_version("not a version banner"), None); } + + #[test] + fn indexing_frame_drops_eta_keeps_elapsed() { + // D1: the projected ETA is gone from the live bar, but measured elapsed + // (a fact, not a projection) stays. describe_plain ignores the mode, so + // constructing in any mode is fine. + let r = ProgressReporter::new(true, false); + let line = r.describe_plain(InitProgress::Indexing { + done: 283, + total: 566, + workers: 4, + }); + assert!(line.contains("283/566"), "counts must remain: {line}"); + assert!(line.contains("(50%)"), "percent must remain: {line}"); + assert!( + !line.to_ascii_lowercase().contains("eta"), + "projected ETA must be gone: {line}" + ); + assert!( + line.trim_end().ends_with('s'), + "measured elapsed must remain: {line}" + ); + } } /// #755 item 3: the semantic heartbeat line — the signal that stops a diff --git a/crates/travsr-plugin-host/src/embed_catalog.rs b/crates/travsr-plugin-host/src/embed_catalog.rs index 01a0d1a5..35b547ec 100644 --- a/crates/travsr-plugin-host/src/embed_catalog.rs +++ b/crates/travsr-plugin-host/src/embed_catalog.rs @@ -794,16 +794,18 @@ pub struct EmbedBackend { } /// RFC-025: the behavioral floor the host requires of the `travsr-embed` -/// sidecar. v1.2.0 is the first release whose content-hash CDC invalidation -/// (travsr-embed #376) replaced the blanket tombstone-delete path; a sidecar -/// below it runs a code path the current host no longer expects and fails deep -/// with a cryptic SQLite error (issue #701). Every embed catalog entry uses the -/// same `travsr-embed` binary, so the floor is a single host constant rather -/// than per-model catalog data. Bump this in the same commit that first relies -/// on a newer sidecar behavior (RFC-025 decision 3), and keep every -/// `version_fallback` in `embed_catalog.toml` at or above it (honesty test). +/// sidecar. v1.6.0 is the first release with the `--reembed` full-rebuild mode +/// that `travsr embed reindex --rebuild` depends on; a sidecar below it does not +/// understand the flag and would exit on the unknown argument instead of +/// rebuilding. (The prior floor, v1.2.0, guarded travsr-embed #376's content- +/// hash CDC path that replaced the blanket tombstone-delete; issue #701.) Every +/// embed catalog entry uses the same `travsr-embed` binary, so the floor is a +/// single host constant rather than per-model catalog data. Bump this in the +/// same commit that first relies on a newer sidecar behavior (RFC-025 +/// decision 3), and keep every `version_fallback` in `embed_catalog.toml` at or +/// above it (honesty test). pub const EMBED_MIN_VERSION: crate::sidecar_version::Semver = - crate::sidecar_version::Semver::new(1, 2, 0); + crate::sidecar_version::Semver::new(1, 6, 0); impl crate::sidecar_version::SidecarSpec for EmbedBackend { fn install_name(&self) -> &str { @@ -928,7 +930,33 @@ pub fn write_model_descriptor(model_dir: &Path, b: &EmbedBackend) -> anyhow::Res #[serde(skip_serializing_if = "str::is_empty")] family: &'a str, } - let content = toml::to_string(&Descriptor { + let path = model_dir.join("model.toml"); + + // The fields `travsr embed init` owns and rewrites on every run. Any other + // key already present in the file (for example a hand-set `macos_engine`, + // which is the sidecar's to own) is carried forward untouched, so re-running + // init no longer silently reverts a user's choice (F6). Merging onto the + // existing file also future-proofs against the next sidecar-owned key. + const INIT_OWNED_KEYS: &[&str] = &[ + "dim", + "pooling", + "query_prefix", + "n_inputs", + "truncate_dim", + "family", + ]; + + let mut table: toml::Table = std::fs::read_to_string(&path) + .ok() + .and_then(|s| toml::from_str(&s).ok()) + .unwrap_or_default(); + // Drop init-owned keys first so a value init no longer emits (e.g. `family` + // when the catalog entry has no arch) does not linger from a prior write. + for k in INIT_OWNED_KEYS { + table.remove(*k); + } + + let owned = toml::Value::try_from(Descriptor { dim: b.dim, pooling: &b.pooling, query_prefix: &b.query_prefix, @@ -937,7 +965,11 @@ pub fn write_model_descriptor(model_dir: &Path, b: &EmbedBackend) -> anyhow::Res family: &b.arch, }) .context("serialize model descriptor")?; - let path = model_dir.join("model.toml"); + if let toml::Value::Table(owned) = owned { + table.extend(owned); + } + + let content = toml::to_string(&table).context("serialize model descriptor")?; // Atomic write: the sidecar reads `model.toml` at startup and hard-errors on a // malformed descriptor, so a concurrent spawn (or the `ensure_model_descriptor` @@ -1334,6 +1366,7 @@ fn derive_phase1_threshold(db_path: &Path, fraction: f64) -> Option { /// /// RFC-021: one sidecar process loads the model once; N reader threads inside /// the sidecar feed a single inference loop. No temp-db management, no merge. +#[allow(clippy::too_many_arguments)] fn run_parallel_reindex( bin_path: &Path, db_path: &Path, @@ -1341,6 +1374,11 @@ fn run_parallel_reindex( model_id: &str, phase: PhaseFilter, quiet: bool, + // When true, the sidecar clears every stored vector for this model first and + // re-embeds all of them (`travsr embed reindex --rebuild`), instead of only + // filling in what is missing. CLI-only; the daemon's automatic spawns always + // pass false. + reembed: bool, overrides: &EmbedOverrides, ) -> anyhow::Result<()> { let model_ram_mb = lookup(model_id).map(|b| b.ram_mb as u64).unwrap_or(0); @@ -1398,6 +1436,10 @@ fn run_parallel_reindex( cmd.arg("--cancel-sentinel").arg(&sentinel_path); } + if reembed { + cmd.arg("--reembed"); + } + if let Some((flag, val)) = phase.sidecar_flag() { cmd.arg(flag).arg(val.to_string()); } @@ -1576,6 +1618,7 @@ pub fn spawn_background_reindex_phase1(db_path: &Path) -> bool { &model_id, PhaseFilter::Phase1 { threshold }, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed Phase 1 failed: {e:#}"); @@ -1625,6 +1668,7 @@ pub fn spawn_background_reindex_phase2(db_path: &Path) -> bool { &model_id, phase, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed Phase 2 failed: {e:#}"); @@ -1659,6 +1703,7 @@ pub fn spawn_background_reindex_all(db_path: &Path) -> bool { &model_id, PhaseFilter::All, false, + false, &EmbedOverrides::default(), ) { tracing::warn!("embed reindex-all failed: {e:#}"); @@ -1704,6 +1749,9 @@ pub fn ensure_reindex_backend_ready(db_path: &Path) -> anyhow::Result<()> { pub fn run_parallel_reindex_blocking( db_path: &Path, phase1_threshold: Option, + // `travsr embed reindex --rebuild`: clear + re-embed every node for the + // active model instead of only filling in what is missing. + reembed: bool, overrides: &EmbedOverrides, ) -> anyhow::Result<()> { let (bin_path, embed_db_path, model_id) = resolve_backend_for_reindex(db_path)?; @@ -1720,6 +1768,7 @@ pub fn run_parallel_reindex_blocking( &model_id, phase, false, + reembed, overrides, ) } @@ -1735,6 +1784,7 @@ pub fn run_parallel_reindex_blocking_quiet(db_path: &Path) -> anyhow::Result<()> &model_id, PhaseFilter::All, true, + false, &EmbedOverrides::default(), ) } @@ -2260,6 +2310,33 @@ mod tests { ); } + /// F6: re-running `embed init` must not revert a hand-set `macos_engine`. + /// The writer owns dim/pooling/etc. but preserves keys it does not own. + #[test] + fn write_descriptor_preserves_hand_set_keys() { + let dir = tempfile::tempdir().expect("tempdir"); + let b = lookup("bge-small-en-v1.5").expect("bge in catalog"); + let toml_path = dir.path().join("model.toml"); + // A prior descriptor with a hand-set sidecar-owned key plus a stale + // init-owned value that must be refreshed. + std::fs::write(&toml_path, b"dim = 1\nmacos_engine = \"tract\"\n").unwrap(); + + write_model_descriptor(dir.path(), b).expect("write descriptor"); + + let parsed: toml::Table = toml::from_str(&std::fs::read_to_string(&toml_path).unwrap()) + .expect("descriptor must parse"); + assert_eq!( + parsed.get("macos_engine").and_then(|v| v.as_str()), + Some("tract"), + "hand-set macos_engine must survive re-init" + ); + assert_eq!( + parsed.get("dim").and_then(|v| v.as_integer()), + Some(b.dim as i64), + "init-owned dim must be rewritten to the catalog value" + ); + } + #[test] fn lookup_finds_bge() { let b = lookup("bge-small-en-v1.5").expect("bge backend must be in catalog"); diff --git a/crates/travsr-plugin-host/src/embed_catalog.toml b/crates/travsr-plugin-host/src/embed_catalog.toml index 2a8a70e8..73bd1138 100644 --- a/crates/travsr-plugin-host/src/embed_catalog.toml +++ b/crates/travsr-plugin-host/src/embed_catalog.toml @@ -13,7 +13,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence for searching relevant passages: " n_inputs = 2 @@ -40,7 +40,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence for searching relevant passages: " n_inputs = 2 @@ -66,7 +66,7 @@ ram_mb = 200 init_secs = 11 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 @@ -92,7 +92,7 @@ ram_mb = 450 init_secs = 47 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 @@ -118,7 +118,7 @@ ram_mb = 1400 init_secs = 150 binary_name = "travsr-embed" github_repo = "Travsr-com/travsr-embed" -version_fallback = "v1.2.0" +version_fallback = "v1.6.0" pooling = "cls" query_prefix = "Represent this sentence: " n_inputs = 3 diff --git a/docs/adrs/ADR-019-embed-reembed-contract.md b/docs/adrs/ADR-019-embed-reembed-contract.md new file mode 100644 index 00000000..b3717ad8 --- /dev/null +++ b/docs/adrs/ADR-019-embed-reembed-contract.md @@ -0,0 +1,118 @@ +# ADR-019: Embed Re-embed Contract (`--reembed` + version floor) + +**Date:** 2026-08-23 +**Status:** Accepted +**Phase:** N/A (cross-repo contract change) +**Author:** Solution Architect +**Related:** RFC-025 (sidecar version floor / honesty tests), travsr-embed #6 (engine provenance), EMBED_UX_AUDIT.md findings F3 + F8 + +--- + +## Context + +A `travsr embed` index stores one vector per node in `embed.db`, tagged with the +engine that produced it (`meta.embed_backend`). GPU/ORT fp32 and tract fp32 +matmul are not bit-identical, so when the resolved engine changes between runs +(for example travsr-embed v1.5.0 reversed the default macOS engine to tract), +an incremental reindex leaves `embed.db` with vectors from two engines. The +sidecar detected this and printed: + +> WARNING: embedding backend changed (...). Existing vectors were produced by a +> different engine; run `travsr embed reindex --rebuild` for consistent +> embeddings. + +Two defects made that remedy dishonest (EMBED_UX_AUDIT.md F3): + +1. **`travsr embed reindex --rebuild` did not exist** — the flag errored with + "unexpected argument". +2. **No re-embed-all path existed anywhere.** `reindex` skips nodes that already + have a vector (`NOT EXISTS`); `switch` only rewrites config; `calibrate` is + explicitly "without re-embedding"; `gc` reclaims. A user could not get a + consistent index without manually deleting `embed.db`. + +The warning also fired **mid-reindex** (F8), so it read as "reindex while +reindexing". + +## Decision + +**Add a first-class full-rebuild path, spanning the sidecar and the CLI, gated by +the RFC-025 version floor. The three pieces move together and the sidecar +release ships first.** + +### 1. Sidecar: `--reindex --reembed` (travsr-embed v1.6.0) + +`--reembed` clears every stored vector for the active model **and** the recorded +engine, then the ordinary reindex re-embeds every node with the current engine +and rebuilds the HNSW index. Because the clear happens once, up front, the +existing chunked-streaming reindex loop (which drains via `NOT EXISTS`) is +reused unchanged and terminates normally. + +**Why clear-then-reindex rather than "bypass `NOT EXISTS`".** The reindex loop +depends on committed chunks dropping out of the pending set to make progress; +removing the filter would loop forever. Deleting the model's rows up front makes +every node pending again with no other change to the loop. + +**Crash-safety.** The old vectors are gone before any new one is written, so at +every moment the rows present in `embed.db` are all from the current engine +(never a torn or mixed file). The stale HNSW index files are removed in the same +up-front step, because search reads the index directly and `rebuild_index` only +rewrites it after the full re-embed; leaving them would let a killed run keep +serving old-engine vectors for rows that no longer exist. A killed run therefore +degrades to "no index" (visible, recoverable by a plain `--reindex`) and fewer +rows, all current-engine, rather than a stale index that looks healthy. +Forgetting the recorded engine up front means a mid-run crash leaves provenance +honest (single engine), and a completed run records the current engine with no +mixed-engine warning. + +### 2. Sidecar: honest, end-of-run notice (F8) + +The mixed-engine notice is emitted **once at the end of a run** instead of +mid-progress, and points at the command that now exists. It is plain-language: +no engine names, no internal terms. + +### 3. CLI: `travsr embed reindex --rebuild` + version floor (RFC-025) + +`--rebuild` invokes the sidecar in `--reembed` mode through the existing +banner/progress/cancel plumbing. Because `--reembed` is a **required** sidecar +behavior the moment the CLI depends on it, the same change bumps +`EMBED_MIN_VERSION` to **v1.6.0** and every embed `version_fallback` to v1.6.0 +(RFC-025 decision 3). A pre-v1.6.0 sidecar then hits the actionable floor +refusal at the reindex entry point instead of failing on an unknown argument. + +## Release ordering (load-bearing) + +The RFC-025 honesty test `declared_floor <= latest_released` (network) refuses a +floor above what users can install. Therefore: + +1. Publish **travsr-embed v1.6.0** (carrying `--reembed`) first. +2. Once published, `latest_released >= min_version`, so honesty test (b) passes. +3. Merge the main CLI change (`--rebuild` + the v1.6.0 floor bump) after. + +Until step 1 lands, honesty test (b) is red **by design** on a networked run +(offline it skips); the non-network honesty test (a), +`version_fallback >= min_version`, stays green because both are v1.6.0. + +## Consequences + +- The engine-change remedy is now reachable and consistent end-to-end. +- The daemon's automatic reindex paths are untouched: `--reembed` is CLI-only + (every daemon spawn passes `reembed = false`). +- A future sidecar-behavior dependency repeats this pattern: add the behavior, + release the sidecar, floor to it in the same commit that first relies on it. +- **Upgrade wall (accepted trade-off).** `EMBED_MIN_VERSION` is a single host + constant consumed by `resolve_backend`, the chokepoint every embed spawn funnels + through (foreground `--rebuild`, the daemon's background phase-1/phase-2/all + passes, and `run_parallel_reindex_blocking`). Raising it to v1.6.0 therefore + refuses **all** embedding for any user still on a sidecar at or below v1.5.0 - + including ordinary background reindexing - until they run `travsr embed init + --reinstall`, even though `--reembed` is strictly opt-in. This differs from the + v1.2.0 floor, which guarded the #376 content-hash CDC path the host relied on + unconditionally, so a blunt floor was the only correct answer there. Here a + narrower gate (refuse only on the `--rebuild` path, leave `PhaseFilter`-only + spawns on the v1.2.0 floor) would protect the same case without the wall. We + keep the single blunt floor deliberately, for RFC-025 consistency: one + honesty-tested constant, one refusal message, no per-operation floor threaded + through the trust-boundary chokepoint. The cost is that the reinstall prompt + reaches the whole installed base on first upgrade rather than only users of the + new flag. If that cost proves too high in practice, splitting the floor by + operation is the escape hatch and does not change the contract above. diff --git a/plugin-hashes.lock b/plugin-hashes.lock index e4596079..24971455 100644 --- a/plugin-hashes.lock +++ b/plugin-hashes.lock @@ -1,6 +1,6 @@ # Auto-generated by update-plugin-hashes.sh — do not edit manually # Format: crate_name = sha256_of_src_tree -travsr-plugin-host = a780c268d22c4c4a38bb0bf586c4d128bdc1902153cc50994b011008a41a8b84 +travsr-plugin-host = 1e19ef90398f1e8f73d0cd3431e0c0fae2b56c5bf6debc5fd22f75ef275b6597 travsr-plugin-protocol = d6f8d6949b3c684aa21d4742a8c9ba04ccec793bea9551d8264f4c7be901d427 travsr-plugin-sdk = 6f20b7313d11d8fa0dfafd5203ef5870be443ee30b2141687c4624b7cc5f9a2b