Fix stale FTS cache detection#100
Conversation
Summary by CodeRabbit
WalkthroughThis PR adds a Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (archive create)"
participant ArgsParser as "args.ts"
participant FS as "Filesystem"
participant Archive as "archive.ts"
CLI->>ArgsParser: parse create --replace
ArgsParser->>ArgsParser: rejectNonCreateReplaceFlag()
ArgsParser-->>CLI: parsed args (replace: true/false)
CLI->>Archive: createArchive(args)
Archive->>FS: prepareCreateArchiveOutputPath()
alt archive exists and no --replace
Archive-->>CLI: throw "already exists" error
else safe to write
Archive->>FS: writeCreatedArchiveFile(outputPath)
Archive->>FS: rename(tempPath, archivePath)
Archive->>FS: rm(tempPath) [finally, if temp remains]
Archive-->>CLI: success
end
sequenceDiagram
participant Caller as "findArchiveObjectsIndexed / rebuildArchiveSearchIndex"
participant ArchiveView as "archive-view.ts"
participant SearchIndex as "search-index.ts"
participant DB as "search_index_state (DB)"
Caller->>ArchiveView: isArchiveSearchIndexCurrent(document)
ArchiveView->>SearchIndex: readSearchIndexStatus(document, input)
SearchIndex->>SearchIndex: createSearchIndexFingerprint(input)
SearchIndex->>DB: readSearchIndexFingerprintFromDatabase()
DB-->>SearchIndex: stored fingerprint or undefined
SearchIndex-->>ArchiveView: "current" | "dirty" | "missing"
ArchiveView-->>Caller: boolean / status
alt status is "dirty"
Caller->>ArchiveView: deleteSearchIndexDatabase()
Caller->>SearchIndex: ensureSearchIndex(input)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/wikg/wikg-coordinator.ts (1)
489-522: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftDirty-check extracts the full archive on every GC pass for each
fts.dboverlay.
isSqliteCacheOverlayDirtyis evaluated before the TTL/force gates incanRemoveSqliteCacheOverlay(Line 489), socreateCurrentArchiveSearchIndexFingerprintunzips the entire archive into a temp dir and rebuilds the search-index records (Line 567-583) even for freshly-updated, within-TTL overlays that would otherwise be retained cheaply. On routine/opportunistic GC over many or large archives this becomes a significant, repeated I/O and CPU cost.Consider gating the expensive fingerprint computation using the already-persisted
archive_signature(compare current archive signature vs the stored overlay signature, and only extract+fingerprint when the signature differs), preserving the "remove dirty regardless of TTL" semantics while avoiding extraction on unchanged archives.Also applies to: 567-583
🤖 Prompt for 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. In `@src/wikg/wikg-coordinator.ts` around lines 489 - 522, The dirty-check in canRemoveSqliteCacheOverlay is doing an expensive archive fingerprint before the TTL/force checks, so move the costly work behind a quick archive-signature comparison. In isSqliteCacheOverlayDirty, use the persisted archive_signature on the EntryOverlay to compare the current archive signature first, and only call createCurrentArchiveSearchIndexFingerprint/readSearchIndexCacheStatus when the signature has changed. Keep the existing semantics that dirty overlays are removable regardless of TTL, but avoid extracting and rebuilding search-index data for unchanged fts.db overlays.src/cli/queue.ts (1)
1048-1062: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPerformance-hint rendering diverges from
archive.ts's equivalent formatter.
formatQueuePerformanceHintLinesrenders onlymessageandCommandper hint, whilesrc/cli/archive.ts'sformatInspectPerformanceHintsalso renders aCurrent {kind}: {current}; suggested: {recommended}.line for the sameGenerationPerformanceHintshape. Users get less context inqueue addoutput than inarchive inspectoutput for identical hint data. Consider extracting a single shared line-formatter intogeneration-planning.tsto keep both surfaces consistent.♻️ Suggested consolidation
-function formatQueuePerformanceHintLines( - hints: readonly GenerationPerformanceHint[], -): readonly string[] { - if (hints.length === 0) { - return []; - } - - return [ - "Performance hints:", - ...hints.flatMap((hint) => [ - ` ${hint.message}`, - ` Command: ${hint.command}`, - ]), - ]; -} +function formatQueuePerformanceHintLines( + hints: readonly GenerationPerformanceHint[], +): readonly string[] { + if (hints.length === 0) { + return []; + } + + return [ + "Performance hints:", + ...hints.flatMap((hint) => [ + ` ${hint.message}`, + ` Current ${hint.kind}: ${hint.current}; suggested: ${hint.recommended}.`, + ` Command: ${hint.command}`, + ]), + ]; +}🤖 Prompt for 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. In `@src/cli/queue.ts` around lines 1048 - 1062, The performance-hint rendering in formatQueuePerformanceHintLines is missing the same detail shown by archive.ts’s formatInspectPerformanceHints, so queue add output is inconsistent for identical GenerationPerformanceHint data. Update the queue formatter to include the current-versus-suggested line for each hint, or better, extract a shared helper in generation-planning.ts and have both formatQueuePerformanceHintLines and formatInspectPerformanceHints use it so the rendered hint lines stay consistent.src/cli/generation-planning.ts (1)
29-57: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
concurrent.jobis accepted but never used in the time estimate.
planGenerationTaskonly factorsconcurrent.requestintocallBatches;concurrent.jobis unused. Since callers (archive.ts inspect improvements, queue.tscreateQueueAddEstimate) queue multiple missing chapters as separate per-chapter jobs governed by job concurrency, the "Wait: min-max" estimate may significantly overstate wall-clock time for multi-chapter work when job concurrency lets several chapter-jobs run in parallel. Conversely, the "raise job concurrency" hint (createGenerationPerformanceHints) implies job concurrency does matter for these workloads.Worth confirming whether the time estimate should also factor in job-level parallelism (e.g. divide
callBatchesfurther byconcurrent.jobwhen multiple chapters are involved), or whether the current single-request-concurrency model is an intentional simplification.🤖 Prompt for 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. In `@src/cli/generation-planning.ts` around lines 29 - 57, `planGenerationTask` is ignoring `concurrent.job` in the wall-clock estimate, so multi-chapter workloads can be overestimated. Update the time calculation in `planGenerationTask` to account for job-level parallelism when chapters are processed as separate jobs, likely by folding `concurrent.job` into the batching logic alongside `concurrent.request`. Keep the existing `getGenerationPlanningProfile` and `createQueueAddEstimate`/`createGenerationPerformanceHints` behavior aligned with the new estimate.
🤖 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/archive/search-index/search-index.ts`:
- Around line 204-207: The freshness check in readSearchIndexStatus and
ensureSearchIndex only compares the stored fingerprint, so a
SEARCH_INDEX_VERSION bump can be missed when content is unchanged. Update
createSearchIndexFingerprint to incorporate SEARCH_INDEX_VERSION, or add an
explicit version comparison in ensureSearchIndex before the early return that
uses readSearchIndexFingerprintFromDatabase. Keep the logic centered around
createSearchIndexFingerprint, readSearchIndexStatus, and ensureSearchIndex so
old indexes are invalidated when the schema/tokenizer version changes.
---
Nitpick comments:
In `@src/cli/generation-planning.ts`:
- Around line 29-57: `planGenerationTask` is ignoring `concurrent.job` in the
wall-clock estimate, so multi-chapter workloads can be overestimated. Update the
time calculation in `planGenerationTask` to account for job-level parallelism
when chapters are processed as separate jobs, likely by folding `concurrent.job`
into the batching logic alongside `concurrent.request`. Keep the existing
`getGenerationPlanningProfile` and
`createQueueAddEstimate`/`createGenerationPerformanceHints` behavior aligned
with the new estimate.
In `@src/cli/queue.ts`:
- Around line 1048-1062: The performance-hint rendering in
formatQueuePerformanceHintLines is missing the same detail shown by archive.ts’s
formatInspectPerformanceHints, so queue add output is inconsistent for identical
GenerationPerformanceHint data. Update the queue formatter to include the
current-versus-suggested line for each hint, or better, extract a shared helper
in generation-planning.ts and have both formatQueuePerformanceHintLines and
formatInspectPerformanceHints use it so the rendered hint lines stay consistent.
In `@src/wikg/wikg-coordinator.ts`:
- Around line 489-522: The dirty-check in canRemoveSqliteCacheOverlay is doing
an expensive archive fingerprint before the TTL/force checks, so move the costly
work behind a quick archive-signature comparison. In isSqliteCacheOverlayDirty,
use the persisted archive_signature on the EntryOverlay to compare the current
archive signature first, and only call
createCurrentArchiveSearchIndexFingerprint/readSearchIndexCacheStatus when the
signature has changed. Keep the existing semantics that dirty overlays are
removable regardless of TTL, but avoid extracting and rebuilding search-index
data for unchanged fts.db overlays.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 584d38ba-ff01-4b3f-b0a5-90730a24ab2c
📒 Files selected for processing (24)
data/help/commands/predicate.jinjadata/help/commands/root.jinjadata/help/commands/uri.jinjadata/help/topics/config.jinjadata/help/topics/format.jinjadata/help/topics/recipe.jinjadata/help/topics/uri.jinjasrc/archive/query/archive-view.tssrc/archive/query/index.tssrc/archive/search-index/index.tssrc/archive/search-index/search-index.tssrc/cli/archive-index.tssrc/cli/archive.tssrc/cli/args.tssrc/cli/generation-planning.tssrc/cli/queue.tssrc/document/document.tssrc/wikg/wikg-coordinator.tstest/archive/query/archive-view.test.tstest/cli/archive.test.tstest/cli/args.test.tstest/cli/queue.test.tstest/gc/gc.test.tstest/wikg/spine-digest-file.test.ts
| const indexedFingerprint = | ||
| await readSearchIndexFingerprintFromDatabase(database); | ||
|
|
||
| if ( | ||
| state.get("version") === SEARCH_INDEX_VERSION && | ||
| state.get("fingerprint") === fingerprint && | ||
| state.get("chaptersRevision") === String(chaptersRevision) | ||
| ) { | ||
| if (indexedFingerprint === fingerprint) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant sections.
wc -l src/archive/search-index/search-index.ts
sed -n '140,230p' src/archive/search-index/search-index.ts
printf '\n---\n'
sed -n '620,710p' src/archive/search-index/search-index.ts
printf '\n=== search for version usage ===\n'
rg -n "SEARCH_INDEX_VERSION|readSearchIndexStatus|createSearchIndexFingerprint|readSearchIndexFingerprintFromDatabase|ensureSearchIndex" src/archive/search-index/search-index.ts src -SRepository: oomol-lab/wiki-graph
Length of output: 8559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the version write path and any other consumers of the stored search index state.
sed -n '230,280p' src/archive/search-index/search-index.ts
printf '\n---\n'
sed -n '540,590p' src/wikg/wikg-coordinator.ts
printf '\n---\n'
sed -n '1030,1105p' src/archive/query/archive-view.ts
printf '\n=== additional searches ===\n'
rg -n "archive_index_settings|search_index_state|SEARCH_INDEX_VERSION|fingerprint" src -SRepository: oomol-lab/wiki-graph
Length of output: 8965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,150p' src/cli/archive-index.ts
printf '\n---\n'
sed -n '280,330p' src/document/schema.ts
printf '\n---\n'
rg -n "SET value = value WHERE key = 'version'|WHERE key = 'version'|search_index_state" src -SRepository: oomol-lab/wiki-graph
Length of output: 3526
Freshness should include SEARCH_INDEX_VERSION
readSearchIndexStatus and ensureSearchIndex only compare the stored fingerprint, and createSearchIndexFingerprint does not include SEARCH_INDEX_VERSION. A tokenizer/schema version bump can therefore leave an old index marked current if the content is unchanged. Fold the version into the fingerprint, or compare it before the early return.
🤖 Prompt for 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.
In `@src/archive/search-index/search-index.ts` around lines 204 - 207, The
freshness check in readSearchIndexStatus and ensureSearchIndex only compares the
stored fingerprint, so a SEARCH_INDEX_VERSION bump can be missed when content is
unchanged. Update createSearchIndexFingerprint to incorporate
SEARCH_INDEX_VERSION, or add an explicit version comparison in ensureSearchIndex
before the early return that uses readSearchIndexFingerprintFromDatabase. Keep
the logic centered around createSearchIndexFingerprint, readSearchIndexStatus,
and ensureSearchIndex so old indexes are invalidated when the schema/tokenizer
version changes.
1 participant
Summary
Tests