Quality delta review 2026-07-14: moments N+1 batch, token-formula drift, DB atomicity (D1-D5) - #9
Merged
Merged
Conversation
Delta-review (2026-07-14) value-object cluster: - D1: ObservationStats#source_tokens_for re-derived the token formula with .round while the observation token_count denominator uses TokenEstimator.from_chars (.ceil). Compression ratio now compares like with like — call from_chars so the 4-chars/token constant lives in one place. - TokenEstimator#estimate now defers to from_chars (arithmetic in one place); class -> module (only self. methods, never instantiated). - TokenBudget#min/#max return 0 (not nil) on an empty budget, matching avg/ p50/p95; the value object now freezes itself. - Jaccard.score: removed the unreachable union.zero? guard (the empty-set guard already covers both-empty).
The prior N+1 fix batched the extraction/ingest content+facts path, but recall_hit/recall_empty/context_injection moments still resolved their top-facts per row: each build_moment called ScopedFactResolver.resolve, one facts.where(id:) query PLUS one FactPresenter entity load per moment. A 50-moment recall page fired ~100 queries; up to 2xlimit events are enriched before the trim, so worst case ~400 queries per /api/moments request. - Add ScopedFactResolver.merge_scoped_ids + build_fact_index (one facts query and one entity load per scope for the whole page) + pure resolve_from_index. - Moments#list builds the index once and threads it through build_moment/ enrich; the per-row resolve_scoped_facts is gone. - ScopedFactResolver.resolve stays (still used by api.rb's single-event path). Batch behavior pinned by new specs (per-scope query, pure lookup, order preserved); existing moments top_facts specs unchanged and green. Refs: docs/quality_review.md D2 (2026-07-14)
truncated_source_content loaded every content item's raw_text (the largest column, up to the ingest cap per row) from both DBs into memory at once to regex-scan. On a mature DB that's an unbounded full-table load. Stream a page at a time (rows_per_fetch: 500), accumulating flagged ids — the same pattern LexicalFTS#rebuild! uses. Behavior unchanged; runner_spec C015 green. Refs: docs/quality_review.md D3 (2026-07-14)
- D4: VectorIndex#backfill_batch! INSERTed vec0 rows (each auto-committing) and set vec_indexed_at in a batch update AFTER the loop. A crash mid-loop left vec0 rows with vec_indexed_at nil, so the next backfill re-INSERTed them without a DELETE -> duplicate embeddings that never self-heal. Wrap the loop + flag update in one @db.transaction, mirroring insert_embedding; the 'no DELETE needed' invariant now genuinely holds. - D5: Reflector#reflect! used a raw @db.transaction, not transaction_with_retry. Reflection runs in the sweep hook racing ingest for the WAL writer; retry the whole transaction on SQLITE_BUSY. The individual mutators stay un-wrapped by design (they run inside this transaction; retry belongs at the boundary). Refs: docs/quality_review.md D4/D5 (2026-07-14)
The transaction wrap on rebuild! (DROP + recreate + reinsert) had only happy-path coverage; the load-bearing branch — a mid-rebuild failure rolls back the DROP and leaves the old content_fts intact — was untested. Add a spec that raises after the DROP and asserts search still answers, so the atomicity guarantee can't silently regress. Refs: docs/quality_review.md (2026-07-14, Beck)
… C015 var - resolve_from_index now uniqs each event's id list so a repeated id emits its fact once, matching the row-set dedup the query-based ScopedFactResolver.resolve gets for free. Regression spec added. - Drop the unused 'total' accumulator in the C015 check (dead before and after the paged_each change; the Finding only uses flagged).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the findings from the 2026-07-14 delta quality review (
docs/quality_review.md). A two-agent expert-lens pass (Metz/Grimm/Bernhardt + Evans/Beck) over the 32-commit delta since 2026-07-01, every finding re-verified against source. The prior review's structural fixes all held (SQLiteStore600,api.rb622,maintenance.rb296; bare rescues 6,rescue Exception0); these are the new items it surfaced.Fixes
enrichresolved top-facts per row forrecall_hit/recall_empty/context_injectionmoments — onefacts.where(id:)+ oneFactPresenterentity load each. A 50-moment recall page fired ~100 queries; worst case ~400/request. AddedScopedFactResolver.merge_scoped_ids+build_fact_index(one facts query + one entity load per scope for the whole page) + pureresolve_from_index;Moments#listbuilds the index once. This closes a second N+1 the prior review's Q7 batching left open (it only covered the extraction/ingest path).~400 → ~3queries.ObservationStats#source_tokens_forre-derived the token estimate with.roundwhile the observationtoken_countdenominator usesTokenEstimator.from_chars(.ceil) — the compression ratio compared ceil-vs-round. Now callsfrom_chars. Bundled with value-object nits:TokenBudget#min/maxreturn 0 (not nil) on empty +freeze;Jaccarddead-branch removal;TokenEstimator#estimatedefers tofrom_chars,class→module.paged_each. The truncation audit loaded every content item'sraw_text(largest column, both DBs) into memory at once. Now streamsrows_per_fetch: 500, the patternLexicalFTS#rebuild!already uses.VectorIndex#backfill_batch!atomicity. vec0 rows were INSERTed (auto-committing) withvec_indexed_atset in a batch update after the loop; a mid-loop crash left vec0 rows flagged nil, so the next backfill re-INSERTed them without a DELETE → duplicate embeddings that never self-heal. Wrapped the loop + flag update in one@db.transaction, mirroringinsert_embedding.Reflector#reflect!retryable. Used a raw@db.transaction, nottransaction_with_retry; reflection runs in the sweep hook racing ingest for the WAL writer. Now retries the whole transaction onSQLITE_BUSY. The individual mutators stay un-wrapped by design — they run inside this transaction, so retry belongs at the boundary, not the statement.LexicalFTS#rebuild!rollback contract. The transaction wrap had happy-path-only coverage; added a spec that raises after the DROP and asserts the old index still answers.Verification
rebuild!rollback test).rake audit:error_handlinggreen (0 blocking).Deferred
HistoricalCleanup#decide_restoreextraction (Low) — readability polish on one-shot historical-cleanup code; the gain doesn't justify the refactor risk. Noted indocs/quality_review.md.