fix(core): query cache concurrency and hybrid bucket ranking - #235
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
Summary by CodeRabbit
WalkthroughThe change isolates internal child processes from dangerous runtime overrides. It updates archive search cache population to include bounded text hits and sentence evidence. SQLite lease acquisition now checks conflicts transactionally and retries until compatible. Archive file deletion and database resolution coordinate lease ownership. CLI help and tests document cache readiness, retrieval grouping, summary guarantees, index jobs, and JSONL output rules. Sequence Diagram(s)sequenceDiagram
participant ArchiveQuery
participant ObjectBucketPopulation
participant SentenceEvidenceSearchCache
participant SessionObjectCaches
ArchiveQuery->>ObjectBucketPopulation: request bounded bucket results
ObjectBucketPopulation->>SentenceEvidenceSearchCache: build sentence evidence cache inputs
SentenceEvidenceSearchCache-->>ObjectBucketPopulation: return evidence-backed hits
ObjectBucketPopulation->>SessionObjectCaches: merge and populate object caches
sequenceDiagram
participant CacheWriteSession
participant acquireSqliteLease
participant SQLiteLeaseTable
participant ArchiveReadSession
CacheWriteSession->>acquireSqliteLease: request write lease
acquireSqliteLease->>SQLiteLeaseTable: check locks and leases transactionally
SQLiteLeaseTable-->>acquireSqliteLease: retry while the read lease conflicts
ArchiveReadSession-->>SQLiteLeaseTable: release read lease
acquireSqliteLease->>SQLiteLeaseTable: insert write lease
SQLiteLeaseTable-->>CacheWriteSession: allow database access
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 6
🧹 Nitpick comments (1)
packages/core/src/storage/wikg/wikg-coordinator/locks.ts (1)
317-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
getStringfor themodecolumn to match the other row mappers.
String(row.mode)converts a missing or non-string column into a value such as"undefined", and the thrown message then reports that value as an unsupported lease mode.mapEntryLockinpackages/core/src/storage/wikg/wikg-coordinator/state.tsusesgetStringfor the same purpose, andgetStringis already imported at line 6.♻️ Proposed refactor
function getSqliteLeaseMode(row: Record<string, unknown>): SqliteLeaseMode { - const mode = String(row.mode); + const mode = getString(row, "mode"); if (mode === "read" || mode === "write") { return mode; }🤖 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 `@packages/core/src/storage/wikg/wikg-coordinator/locks.ts` around lines 317 - 325, Update getSqliteLeaseMode to read row.mode with the already imported getString helper, matching mapEntryLock’s row-mapping behavior, while preserving the existing read/write validation and unsupported-mode error handling.
🤖 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 `@packages/core/data/help/topics/readiness.jinja`:
- Line 149: Update the Hybrid workflow guidance near the retrieval instructions
to include running index sync when an existing archive cache is outdated, while
noting the exception that a missing cache may be created lazily. Preserve the
existing requirement to build both FTS and source embedding artifacts before
querying.
In `@packages/core/data/help/topics/uri.jinja`:
- Around line 80-81: Update the broad-query guidance in the URI help topic to
stop presenting `/source` and `/summary` as query lenses, since they are
readable text streams rather than collection scopes. Keep only the valid
object-family lenses, or replace them with a confirmed valid source/summary
query scope without changing the surrounding search guidance.
In `@packages/core/src/retrieval/query/archive-view/search/buckets.ts`:
- Line 248: Update the cache population flow around populateObjectBucketCaches
so the session-wide evidence window is independent of the first response page’s
limit, preventing the cache from being marked complete with only the initial
text hits. Use the established fixed session-wide window or transactionally
extend the cache for later cursor pages, and add a regression test that compares
a small-limit initial query with later evidence-backed object results.
In `@packages/core/src/storage/wikg/wikg-coordinator/locks.ts`:
- Around line 173-242: The acquireSqliteLease function at locks.ts contains an
unbounded while(true) polling loop that acquires locks while its caller
resolveSqliteDatabasePath already holds the "state" lock, creating circular
lock-acquisition order with deleteFile which holds the "write" lock before
acquiring "state". Fix the lock-acquisition order by restructuring
acquireSqliteLease to acquire the lease before the "state" lock is held
(requiring coordination with its caller), and add a deadline to the polling loop
that throws a lock-timeout error instead of looping forever. At file-store.ts in
deleteFile, align the lock-acquisition order to match the corrected pattern used
by resolveSqliteDatabasePath, and bound the waitForSqliteLeasesToDrain call with
the same deadline mechanism so both paths respect the same timeout and neither
hangs indefinitely.
In `@test/core/retrieval/query/archive-view/graph-search.test.ts`:
- Around line 134-142: Update the mention-gamma fixture in the
openedDocument.mentions.save call so its rangeStart is 29 and rangeEnd is 43,
matching the exclusive source span of “unnamed person.”
In `@test/core/storage/wikg/wiki-graph-archive-file.test.ts`:
- Around line 208-231: Update the catch block in the test around the writer and
reader promises to settle writer during failure cleanup, alongside releasing the
reader and awaiting reader. Ensure writer is awaited or caught so pending
polling is completed and any rejection is observed before rethrowing the
original error.
---
Nitpick comments:
In `@packages/core/src/storage/wikg/wikg-coordinator/locks.ts`:
- Around line 317-325: Update getSqliteLeaseMode to read row.mode with the
already imported getString helper, matching mapEntryLock’s row-mapping behavior,
while preserving the existing read/write validation and unsupported-mode error
handling.
🪄 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: ce305792-4865-4dee-9ee7-16641ac83ff1
📒 Files selected for processing (13)
packages/cli/src/args/help.test.tspackages/cli/src/runtime/entry-context.tspackages/cli/src/runtime/internal-child.test.tspackages/cli/src/runtime/internal-child.tspackages/core/data/help/commands/predicate.jinjapackages/core/data/help/commands/uri.jinjapackages/core/data/help/topics/readiness.jinjapackages/core/data/help/topics/uri.jinjapackages/core/src/retrieval/query/archive-view/search/buckets.tspackages/core/src/storage/wikg/wikg-coordinator/file-store.tspackages/core/src/storage/wikg/wikg-coordinator/locks.tstest/core/retrieval/query/archive-view/graph-search.test.tstest/core/storage/wikg/wiki-graph-archive-file.test.ts
Comment on lines
+134
to
+142
| await openedDocument.mentions.save({ | ||
| chapterId: 1, | ||
| id: "mention-gamma", | ||
| qid: "QGamma", | ||
| rangeEnd: 31, | ||
| rangeStart: 17, | ||
| sentenceIndex: 0, | ||
| surface: "unnamed person", | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the mention offsets in the fixture.
"unnamed person" starts at offset 29 and ends at exclusive offset 43. The current range 17..31 selects part of "appears beside an un". This creates an invalid mention record and can let the test pass without valid source-span metadata.
Proposed fix
await openedDocument.mentions.save({
chapterId: 1,
id: "mention-gamma",
qid: "QGamma",
- rangeEnd: 31,
- rangeStart: 17,
+ rangeEnd: 43,
+ rangeStart: 29,
sentenceIndex: 0,
surface: "unnamed person",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await openedDocument.mentions.save({ | |
| chapterId: 1, | |
| id: "mention-gamma", | |
| qid: "QGamma", | |
| rangeEnd: 31, | |
| rangeStart: 17, | |
| sentenceIndex: 0, | |
| surface: "unnamed person", | |
| }); | |
| await openedDocument.mentions.save({ | |
| chapterId: 1, | |
| id: "mention-gamma", | |
| qid: "QGamma", | |
| rangeEnd: 43, | |
| rangeStart: 29, | |
| sentenceIndex: 0, | |
| surface: "unnamed person", | |
| }); |
🤖 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 `@test/core/retrieval/query/archive-view/graph-search.test.ts` around lines 134
- 142, Update the mention-gamma fixture in the openedDocument.mentions.save call
so its rangeStart is 29 and rangeEnd is 43, matching the exclusive source span
of “unnamed person.”
Moskize91
changed the title
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.
Summary
Validation