Index-backed reads: Find/Visible/Prime/Status query index.sqlite instead of scanning entries#41
Closed
quad341 wants to merge 11 commits into
Closed
Index-backed reads: Find/Visible/Prime/Status query index.sqlite instead of scanning entries#41quad341 wants to merge 11 commits into
quad341 wants to merge 11 commits into
Conversation
…psert (crn-6az.6.1.1) Reindex used to unconditionally DROP+recreate entries/entry_tags then INSERT OR REPLACE every parsed entry -- wiping any index-only state (like a future hit-counter) on every rebuild, and never persisting anchor_repo/ anchor_paths/anchor_spec/created_at even though the Entry/Anchor Go structs already carry them. - Split the schema: entries + new index_meta table use CREATE TABLE IF NOT EXISTS (persist across reindexes); entry_tags carries no index-only state and is still dropped/recreated wholesale each time. - addColumnIfMissing forward-migrates a pre-existing index.sqlite built by an older binary (ALTER TABLE ADD COLUMN, tolerating "duplicate column name" for columns already present). - Reindex's per-entry write is now INSERT ... ON CONFLICT(id) DO UPDATE SET <every column except hit_count> -- hit_count is deliberately left out of the UPDATE SET so a reindex can't stamp a surviving row back to the body's implicit zero. - anchor_paths is JSON-encoded (encoding/json) into a TEXT column. - index_meta.indexed_at_commit is stamped via the existing git() helper (freshness.go), which already degrades to "" on a non-git store -- matching TestReindex's non-git t.TempDir() fixture. - Added a same-transaction orphan-row sweep (TEMP TABLE current_ids + DELETE ... WHERE id NOT IN) for entries whose source file is gone. Not explicitly called out in the bead's acceptance criteria, but required to avoid reintroducing a real regression: the old DROP+recreate-every-time approach incidentally already dropped deleted entries, and the new upsert-based approach would otherwise let them linger forever. Six new tests in index_test.go (new columns + anchor_paths JSON round-trip, index_meta watermark, hit_count preserved across a second reindex, orphaned rows removed, legacy pre-migration schema forward- migrates cleanly); the existing TestReindex is unmodified and still green. Full gate green: gofmt, go vet, go build, golangci-lint (0 issues), go test ./... -race -count=1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…az.6.1.2) ensureFresh(ctx, store) compares the store's git HEAD against index_meta.indexed_at_commit and synchronously reindexes on any mismatch or unreadable watermark, so a body edit committed outside cairn's own write path is never served from a stale index. On an already-fresh index it is a no-op (verified by call count, not just absence of error). Non-git stores skip the HEAD comparison once an index exists, avoiding a reindex on every call. Not yet wired into any CLI read path: get/freshness/verify/status/map/ prime all still read bodies directly via Find/IterEntries/Visible (crn-6az.6.1.3/.4/.5, still blocked on this bead). This lands the helper those follow-ons will call.
….1.3)
Find(ctx, store, id) now calls ensureFresh, then a single
SELECT body_path FROM entries WHERE id=? (factored into findBodyPath),
then exactly one ParseEntry on the resolved path for Body + full
Anchor{Repo,Paths,Spec} (not index columns). Replaces the old
IterEntries-plus-linear-scan, so a lookup costs one SQL query and one
file read regardless of store size. On a hit, hit_count is incremented
via UPDATE ... RETURNING and scanned directly into the returned
Entry.HitCount, overwriting whatever stale value ParseEntry populated
from the body's own frontmatter (FR-4). get/freshness/verify are the
only callers touched -- map/prime still go through Visible/IterEntries
and gain no hit_count side effect.
A SELECT miss retries once after a forced Reindex before returning
ErrNotFound. This closes a false-negative found while implementing
this bead: Entry.Create is a pure filesystem write with no git commit
and no reindex, so an entry created into a non-git store after that
store's index was already built was reported not-found -- ensureFresh
intentionally treats an already-built non-git-store index as fresh
forever (crn-6az.6.1.2), since there's no git HEAD to compare against.
The retry only costs extra work on the miss path (already O(store
size) under the old linear scan); the hit path stays one query + one
read, which is this bead's actual benchmark-relevant guarantee.
Four new tests in entry_test.go: unknown id still returns ErrNotFound
unchanged; hit_count increments by exactly 1 per call and the returned
Entry reflects it; a malformed sibling file written after indexing
proves Find resolves via the index rather than re-walking (a walk
would error on it, Find does not); two sequential Create+Find calls
against the same non-git store both succeed (the regression above).
Full gate green: gofmt, go vet, go build, golangci-lint (0 issues),
go test ./... -race -count=1.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s (crn-6az.6.1.4) Visible and Prime now query the SQL index (via ensureFresh's self-heal) instead of walking entry bodies: Visible replaces IterEntries+filter with SELECT id, topic_key, verified_at, created_at FROM entries plus a new scopeTags helper over entry_tags. map/prime ctx-threaded end to end (cmd/commands.go, cmd/prime.go, internal/cairn/prime.go). Also fixes the critic package's fixture/perf/recall/scope_precedence helpers to thread context and adds race_on.go/race_off.go build-tag constants so RunPerfScenario can widen its latency thresholds under `go test -race` without the instrumentation overhead reading as a regression.
….6.1.5) Status(ctx, store) sources every field cairn status actually consumes (ID, TopicKey, Scope, VerifiedAt, CreatedAt, Anchor) from the SQLite index via ensureFresh + a single SELECT, instead of IterEntries's walk-and-parse-every-body. Anchor fingerprint checks and shadow computation are untouched -- Check still reads only e.Anchor, and ShadowMap only reads the fields Status now populates. scopeTags duplicates crn-6az.6.1.4's helper of the same name rather than merging its unmerged branch, per the crn-0yv precedent of independent duplication now, reconciliation later.
# Conflicts: # internal/cairn/entry_test.go
…anch Resolves conflicts with the prior .1.3+.1.4 merge: dedupes the scopeTags helper (added independently by both .1.4 and .1.5) and splits the interleaved Visible/Status test pairs into standalone functions. Integration point only, for crn-6az.6.1.6 adversarial test/benchmark pass. No production behavior changed beyond what .1.1-.1.5 already introduced.
openDB() opened the sqlite connection with no busy_timeout or WAL pragma. ensureFresh() synchronously triggers Reindex() on every read path (Find, Get, Verify, Visible, map, prime, status), and CAIRN_STORE defaults to one shared path across the whole agent fleet, so concurrent CLI invocations routinely raced on Reindex and produced a spurious hard "database is locked" failure for the losing caller instead of waiting. Fix: set busy_timeout(5000) and WAL journal mode in openDB()'s DSN. Separately, indexStale() couldn't distinguish a confirmed non-git store from a git invocation that failed for an unrelated/transient reason -- both silently returned (false, nil), risking silent under-detection of staleness. Fix: git() now returns a three-way result (output, found, err) that distinguishes a confirmed negative verdict from git itself (not a repo, or zero commits -- exec.ExitError) from a genuine invocation failure (context cancellation, PATH issue, etc.), and indexStale propagates the latter as an error instead of folding it into "not stale". Adds regression coverage: busy_timeout/WAL pragma application, git() invocation-failure propagation via freshness.go and indexStale, and a concurrent Find/Reindex stress test that reliably reproduces the original "database is locked" failure against the pre-fix code (134-138/140 calls) and passes reliably (30/30 runs) with the fix. Refs crn-t250, crn-gjmy AC(a). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…crn-j3k4) Reindex() dropped and recreated entry_tags via two independent autocommit statements, outside any transaction and before its own per-entry-upsert tx began. Concurrent Reindex() calls could interleave those statements (both DROPs succeed via IF EXISTS, then both CREATEs race) and the loser hit a hard "table entry_tags already exists" SQL logic error. Move both statements inside the same transaction as the rest of the rebuild, so concurrent Reindex() calls fully serialize on SQLite's single-writer lock instead of interleaving. Also add _txlock=immediate to openDB's DSN: the only db.BeginTx in this package is this write transaction, which never reads before its first write, so the driver's default "deferred" lock timing (acquired lazily at the first write statement rather than at BEGIN) could itself surface SQLITE_BUSY without honoring busy_timeout's retry loop. "immediate" acquires the write lock at BEGIN, where busy_timeout's retry reliably applies. A third, narrower issue was found and characterized while stress testing this fix: concurrent Reindex() calls racing to initialize index.sqlite from a store where it doesn't exist yet (rather than steady-state contention on an already-initialized file) still hit an intermittent SQLITE_BUSY at a low rate. Filed separately as crn-t42e per this fleet's established norm rather than expanding this bead's scope after the fact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reconciles crn-ln1's scopeMismatchWarnings diagnostic (PR #23, merged to main) with this stack's index-backed Visible/Prime rewrite (crn-6az.6.1.x), which independently gave both functions a leading context.Context param and dropped the diagnostic entirely. Design: Visible and Prime's scope-mismatch diagnostic both need a bulk, unfiltered entry read -- Visible to filter+shadow it, scopeMismatchWarnings to compare against the filtered result. Rather than add a second bulk-query path, both now source that read from this stack's own Status(ctx, store): Visible becomes Status + visibleFrom (ported from origin/main's IterEntries + visibleFrom split), and Prime calls Status once and derives both the visible set and the diagnostic from it. Also fixed a silent (non-conflict-marked) breakage in prime_test.go: git's line-based merge auto-applied two non-overlapping edits to the same file, leaving 4 of its Prime() calls on the old two-arg signature while others already used the new ctx-based one -- caught by grepping every Visible(/ Prime( call site tree-wide, not just the files git flagged as conflicted. Kept origin/main's Prime footer text ("cairn remember <body> ...") over this stack's ("no `remember` command yet"): cmd/remember.go already exists on origin/main (crn-6az.2/crn-ilu8 reconciled this footer separately, earlier), so the stack's own text was stale, not a live alternative. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Blocked: CI is red on this branch because origin/main's PR #40 (review CLI verb) landed test files ( Routed to builder for a fix; do not merge this PR as-is. Will be closed as superseded once a fixed, re-reviewed replacement is opened. |
Owner
Author
|
Superseded: the underlying branch has been squash-rebased onto current |
quad341
added a commit
that referenced
this pull request
Jul 23, 2026
…ead of scanning entries (#42) * fix(cairn): index-backed reads + entry_tags DDL race fix, squashed for rebase (crn-2xpm) Squashes builder/crn-2xpm-reconcile's commits since merge-base f6970f5 (net diff only, unchanged) into one commit re-applied cleanly on current origin/main, so a linear rebase no longer replays this branch's internal merge commits' pre-resolution states as independent patches. Deploy gate's attempt_bounded_self_rebase found a real conflict (rc=12) rebasing this branch's history onto origin/main, even though a 3-way git merge-tree of the same two trees was clean. Root cause: this branch contains merge commits (9168234, 3479d32, cc6ceb2, 8d913c5) that already 3-way-resolved conflicts between parallel feature branches and against origin/main. A linear rebase instead replays each original commit individually -- including pre-resolution commits like 9724ae9, whose lone-patch content still conflicts with what origin/main looks like now, even though that conflict was already resolved once, downstream, via those merge commits. Merging origin/main again (a third time) would not fix this: it resolves the conflict at the merge point but leaves the same problematic commit replayable-and-conflicting on any subsequent rebase. Flattening the branch's own history is the only durable fix. Net change (unchanged from e9cdcd2, verified identical via `git diff --cached --stat` vs `git diff f6970f5 e9cdcd2 --stat`): the index-backed reads stack (Status/Visible/Prime/Find all index-backed, ctx-threaded, zero full-body TOML decode or hit_count writes on read paths), the entry_tags DROP+CREATE DDL race fix + txlock=immediate (crn-j3k4), the busy_timeout/WAL locking fix (crn-t250), crn-ln1's scopeMismatchWarnings diagnostic reconciled onto the index-backed Status read, and the 4 review_test.go/cmd/review_test.go call sites updated to the new Find(ctx, store, id) signature after PR #40 landed with the old one. New SHA per crn-fie5 SHA-pinning mandate -- requires fresh reviewer verdict; e9cdcd2's PASS does not carry over. Old branch builder/crn-2xpm-reconcile (tip e9cdcd2, already-open PR #41) left untouched as historical reference, same treatment as the PR it superseded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: release gate PASS for cairn-index-backed-reads-squashed --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
What this changes
cairn find,cairn prime, and the store'sVisible/Statuschecks nowanswer from the SQL index (
index.sqlite) instead of scanning and decodingevery entry's TOML body on every call. Behavior is unchanged from the caller's
side — same results, same CLI output — but lookups no longer pay the cost of
a full-store body decode, and
Reindexgained two concurrency fixes:entry_tags' schema (re)creation now runs inside the same writetransaction as the per-entry upsert work, instead of as separate
autocommit statements — eliminating a race where two concurrent
Reindexcalls could hit a hardtable entry_tags already existserror._txlock=immediate, soReindex's write lock isacquired at
BEGIN(wherebusy_timeout's retry logic applies) ratherthan lazily at the first write statement.
cairn prime's scope-mismatch diagnostic (warns when a topic's tags don'tmatch any configured scope dimension) is preserved on top of the new
index-backed read path — see Review notes.
Why one PR
This bundles 7 beads because they're one continuous refactor of the same
read path (
Find,Visible,Prime,Status,Reindex) — later beadsbuild directly on earlier ones' schema and query changes, and none of them
is independently shippable without the others.
Review notes
internal/cairn/entry.go'sVisibleandinternal/cairn/prime.go'sPrimeboth gained a leadingcontext.Contextparameter and now sourcetheir data from a single shared
Status(ctx, store)call rather thanIterEntries. This is a reconciliation againstorigin/main'sindependently-landed scope-mismatch diagnostic (PR cairn prime: warn on scope-dimension silent-miss #23) — the two features
were built in parallel against incompatible
Visible/Primesignatures,and this PR carries the merged result.
mainmid-flight(PR Fix WriteBack corrupting anchor table when patching verification fields #24, feat(cairn): stale review-branch detection + reactive re-notify (crn-0yv.1) #39) and are resolved by rename, not merge, since the contracts
differ:
splitFrontmatterForPatch([]byte, keeps closing fence) staysdistinct from
entry.go'ssplitFrontmatter(string, no fence);ReviewMergeBranch/ListReviewMergeBranches(interactive review flow)stay distinct from
branches.go'sReviewBranch/ListReviewBranches(librarian stale-branch sweep).
operators —
index.sqliteself-heals/rebuilds via the existingensureFreshpath.Test plan
🤖 Deployed by actual-factory