registry: fix five silent cue/BM25 cache decay paths (#563) - #574
Conversation
Closes #563 - cue_vector no longer caches an empty embedding-provider response; it's treated as a resolve failure instead, and CueCache clears itself on a width change so a backend swap self-heals rather than scoring 0.0 forever. - refresh_bm25 only marks bm25_dirty when a batch actually changed the resident index, not whenever the batch was merely nonempty. - CueCache::insert now advances recency on a re-insert of an existing key, matching get's behavior. - Semaphore (embed_provider_slots) gained a real deadline, FIFO fairness, and observability (taguru_embed_slot_waiters/_waits_total/ _timeouts_total), replacing the original unbounded, non-FIFO wait. - TAGURU_EMBED_PARALLEL=0 is now rejected loudly (floored to 1 with a warning) at the env boundary and in boot_with, instead of silently rewritten in two places that could disagree. Claude-Session: https://claude.ai/code/session_01GXX8bsY4AwECtGBxGwj7nM
Both new tests sampled the "queued" gauge only after the blocked call had already returned, so waiting()/embed_slot_waiters() being hardcoded to 0 slipped past cargo mutants --in-diff. Both now run the blocking half on a separate thread and sample the live gauge while it is still queued. Claude-Session: https://claude.ai/code/session_01GXX8bsY4AwECtGBxGwj7nM
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughBM25の変更判定、CueCacheの整合性、埋め込みスロットの期限付きFIFO制御、並列数の正規化、待機状態のメトリクス出力を追加・変更しました。 ChangesRegistry correctness and embedding control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant 更新処理
participant セマフォ
participant 埋め込みプロバイダー
participant メトリクス
更新処理->>セマフォ: 期限付きスロット取得
セマフォ-->>更新処理: Acquisition
alt スロット取得成功
更新処理->>埋め込みプロバイダー: ベクトル生成
埋め込みプロバイダー-->>更新処理: ベクトル
else 待機期限切れ
セマフォ-->>更新処理: SlotDeadlineExceeded
更新処理->>メトリクス: 待機タイムアウトと更新失敗を記録
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/registry.rs`:
- Around line 2477-2496: Update the `sources` loop to have `index.upsert_source`
report whether it actually changed the index, and combine that result into
`changed` instead of unconditionally setting it to true. Ensure empty
`PassageRecord` values with no existing slots return false, while real tombstone
creation or slot additions return true; preserve the existing `remove_source`
dirty tracking.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56a42b44-04e6-41c0-bebc-a38e9ff9ed86
📒 Files selected for processing (14)
src/bm25.rssrc/env.rssrc/metrics.rssrc/metrics/prometheus.rssrc/metrics/record.rssrc/metrics/taxonomy.rssrc/registry.rssrc/registry/boot.rssrc/registry/concurrency.rssrc/registry/core_tests.rssrc/registry/embeddings.rssrc/registry/embeddings/gloss_tests.rssrc/registry/gauges.rssrc/registry/retrieval_cache.rs
upsert_source now reports whether it actually changed the index — an empty or whitespace-only PassageRecord upserted for a source with nothing live to tombstone either is index-inert, but refresh_bm25's Some(record) arm was unconditionally counting it as a change. The dirty gate trusts upsert_source's own return now, matching how it already trusted remove_source's. Claude-Session: https://claude.ai/code/session_01GXX8bsY4AwECtGBxGwj7nM
PR #574's diff (56 mutants, under the old 60 budget) blew the job's 60min ceiling and got cancelled mid-run — an infrastructure timeout, not a missed-mutant finding. At the observed non-incremental, --jobs 3 rate (~1.6min/mutant, from #572/#573's 21-22-mutant runs finishing in 32min), 25 mutants lands around 45min with headroom for a slower runner; 56 would need ~95min. Local verification (0 missed across the full diff, CARGO_INCREMENTAL=1 --jobs 4) already covered #574's actual mutants before this — this only tightens the automatic CI gate size for future PRs. CLAUDE.md's local pre-PR gate threshold updated to match, so it stays in sync with what CI will actually run. Claude-Session: https://claude.ai/code/session_01GXX8bsY4AwECtGBxGwj7nM
Summary
#532's audit found five paths where the registry's cue/BM25 caches degrade silently — no error, no warning, no metric, nothing to trigger on. All five are fixed here, one PR, matching #562's precedent of a compact fix set with test-per-item.
Closes #563
cue_vectornow treats a provider'sOk(vec![])as a resolve failure — logged, metered, and never written tocue_cache.CueCachealso tracks the resident vector width and clears itself on a width change, so a backend swap behind a stable model name (the same hazard the gloss refresh path already guards against) self-heals instead of leaving stale-width cues stuck at a silent 0.0 forever.refresh_bm25marked the sidecar dirty even when nothing changed. The dirty gate is nowchanged— whether any source in the batch actually altered the resident index — instead of!sources.is_empty().Bm25Index::remove_source/tombstonenow report whether they tombstoned anything live.CueCache::insertdidn't refresh recency on an existing key. A re-inserted cue now advances the LRU clock exactly like aget, so a cue that keeps getting resolved can no longer be evicted while genuinely hot.Semaphore::acquirehad no deadline and no fairness. Replaced withacquire_until(deadline): a FIFO ticket queue (fixing potential starvation underparking_lot::Condvar's non-FIFO wakeups) and a real timeout, both backed by three new metrics —taguru_embed_slot_waiters(gauge),taguru_embed_slot_waits_total/_timeouts_total(counters).TAGURU_EMBED_PARALLEL=0silently rewritten to 1 in two places that could disagree. Now floored loudly at the env boundary (resolve_embed_parallel, warns) and normalized once inboot_withbefore it reaches both theembed_parallelfield and the semaphore's construction, so the two ceilings can never read different numbers.Test plan
cargo fmtcargo clippy --all-targets -- -D warningscargo test— full suite, 0 failedcargo mutants --in-diff— 55 mutants, 0 missed, 0 timeouts (2 initially-missed mutants around the newwaiting()/embed_slot_waiters()gauges fixed by sampling them while a thread is still blocked, not only after it returns)embed_parallelno-hang)Summary by CodeRabbit
新機能
改善