fix(server): layered freshness policy for index queries - #496
Conversation
Cursor resolution awaits the current file's compile like every other AST-backed request; cross-file results honor the reindex queue's pending reason (deps-only pending files keep serving, content-changed ones are skipped until reindexed); out-of-sync didChange ranges are dropped with an error log instead of silently. Replaces the rejected bounded-wait approach.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-file reindex reasons and tickets, splits invalidation into deps-only and content-changed paths, skips stale merged-index contributions in queries, requires compiled sessions for index-backed handlers, logs unmappable edits, and updates tests for the new freshness behavior. ChangesReindex reason and query freshness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FeatureRouter
participant Compiler
participant IndexQuery
participant Indexer
Client->>FeatureRouter: definition/references request
FeatureRouter->>Compiler: ensure_compiled(session)
FeatureRouter->>IndexQuery: query with session
IndexQuery->>Indexer: pending_reason(path_id)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
src/server/service/query.cpp (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated skip predicate.
is_path_open(file_id) || skip_stale_contribution(file_id)is duplicated across six call sites (query_relations, find_definition_location, collect_grouped_relations, collect_unique_targets, get_definition_text, collect_references). A small private helper centralizes the freshness-gating rule and reduces the risk of one site drifting from the others in a future edit.♻️ Proposed refactor
+ /// Whether a reference file's merged-index contribution should be + /// skipped for this query: either an open session already covers it, + /// or its own content changed and reindex hasn't landed. + bool skip_reference_file(std::uint32_t path_id) const { + return is_path_open(path_id) || skip_stale_contribution(path_id); + }Then replace each
is_path_open(file_id) || skip_stale_contribution(file_id)withskip_reference_file(file_id).Also applies to: 256-256, 310-310, 345-345, 431-431, 474-474
🤖 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/server/service/query.cpp` at line 158, The freshness gate `is_path_open(file_id) || skip_stale_contribution(file_id)` is repeated in several query paths, so extract it into a small private helper in `query.cpp` and use that helper everywhere. Add a single method such as `skip_reference_file` near the existing query helpers, then replace the duplicated predicate in `query_relations`, `find_definition_location`, `collect_grouped_relations`, `collect_unique_targets`, `get_definition_text`, and `collect_references` so they all share the same rule.
🤖 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/server/compiler/indexer.cpp`:
- Around line 555-560: The scheduling path in indexer.cpp needs to handle
entries added after the dispatch loop exits but before worker joins complete,
because schedule() can return with indexing_active still true and leave queued
work stranded. Update the queue-drain handling around index_queue_pos,
index_queue, and reindex_reasons so that any entries appended during worker
joining are rescheduled into a new round rather than only being cleared when the
queue is fully drained. Use the existing schedule()/enqueue() flow and the
pending_ids/reindex_reasons bookkeeping to ensure the next round starts whenever
index_queue_pos is still behind index_queue.size().
- Around line 453-466: Gate the merge in Indexer::run_index_task, not just the
reindex_reasons cleanup: an older in-flight task can still call index_one() and
merge stale TUIndex data after a newer ticket has taken over. Add the ticket
check around the index_one() result handling so only the current ticket is
allowed to commit the merge, and keep the existing reindex_reasons erase logic
tied to the same ticket in Indexer::run_index_task.
---
Nitpick comments:
In `@src/server/service/query.cpp`:
- Line 158: The freshness gate `is_path_open(file_id) ||
skip_stale_contribution(file_id)` is repeated in several query paths, so extract
it into a small private helper in `query.cpp` and use that helper everywhere.
Add a single method such as `skip_reference_file` near the existing query
helpers, then replace the duplicated predicate in `query_relations`,
`find_definition_location`, `collect_grouped_relations`,
`collect_unique_targets`, `get_definition_text`, and `collect_references` so
they all share the same rule.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2720760-8cce-4af2-93d6-1b8157d52e5f
📒 Files selected for processing (16)
src/server/compiler/indexer.cppsrc/server/compiler/indexer.hsrc/server/service/feature_router.cppsrc/server/service/feature_router.hsrc/server/service/query.cppsrc/server/service/query.hsrc/server/state/invalidator.cppsrc/server/state/invalidator.hsrc/server/state/session_store.cppsrc/server/state/session_store.hsrc/server/transport/master_server.cppsrc/server/transport/master_server.htests/integration/features/test_query_freshness.pytests/integration/lifecycle/test_protocol_edges.pytests/unit/server/invalidator_tests.cpptests/unit/server/query_freshness_tests.cpp
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ff09a6fec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeb191c1ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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/server/service/feature_router.cpp`:
- Around line 109-114: The compile-and-generation freshness check is duplicated
across the FeatureRouter handlers, so extract that repeated block into a shared
helper in FeatureRouter (for example the existing settle_cursor_file pattern or
a new settle_cursor_session method in feature_router.h/.cpp). Have each handler
call the helper and co_return serde_raw{"null"} when it reports the session is
stale or not compiled, so the policy lives in one place and all 11 call sites
stay consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6b31f0e-3903-41df-9168-5c7230cc32cf
📒 Files selected for processing (6)
src/server/compiler/indexer.hsrc/server/service/feature_router.cppsrc/server/state/invalidator.cppsrc/server/state/invalidator.hsrc/server/transport/master_server.cpptests/unit/server/invalidator_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/state/invalidator.h
- src/server/transport/master_server.cpp
- src/server/compiler/indexer.h
- src/server/state/invalidator.cpp
- tests/unit/server/invalidator_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ccd0b123e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ca224bce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be847fc215
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701e5d203c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ac86c12cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31334a8356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…, honest index gaps
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f8d9556e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lback for open files
1 participant
Supersedes #493 (bounded-wait approach, rejected). Instead of paying a fixed
delay for a still-incomplete freshness guarantee, index queries now follow a
layered freshness policy.
Cursor resolution waits for the file's compile
Requests that resolve a cursor position into a symbol (definition,
references, declaration, type definition, implementation, call/type
hierarchy) now await the current file's compile before querying the index —
the same await, with no timeout, that hover and every other AST-backed
request already uses. Previously most of these queried the index
immediately, so a query racing a
didChangecould resolve the cursoragainst pre-edit positions and name the wrong symbol.
Cross-file results honor the reindex queue's pending reason
When a query fans out to other files' index contributions, files sitting in
the background reindex queue split two ways, decided by why they were
enqueued:
cascade case): their own text did not move, so the existing rows keep
serving until the reindex lands.
command change): their rows describe text that no longer exists, so their
contribution is skipped until the reindex lands.
The invalidation engine knows the cause at enqueue time, so the
Indexerrecords a two-level pending reason (
DepsOnly | ContentChanged, upgradesare absorbing) and the query side checks it in O(1) with no I/O. A file
re-enqueued while its index task is in flight keeps its newer pending state
(ticket-guarded clear).
didCloseclassifies by comparing the disk contentagainst the shard's stored snapshot — a browse-and-close keeps serving its
rows, a close after saved edits does not. The startup sweep enqueues as
deps-only so a warm index cache keeps serving through the initial scan.
With indexing disabled the gate is off: serving last-known rows beats a
permanent hole.
Results may therefore be incomplete while the queue drains. That is now a
documented contract on
IndexQuery(replacing two FIXMEs), together withtwo recorded-not-implemented TODOs: a blocking "complete results" query
mode, and a dedicated "is the index ready?" request for agent consumers.
Fixed along the way: the background round used to spawn its per-file task
as an immediately-invoked capturing lambda. A lambda coroutine's captures
live in the lambda object, which dies at the end of the spawning statement,
so anything the task read after its first suspension was dangling. The task
is now a member coroutine taking its inputs as parameters, which are copied
into the coroutine frame.
Buffer desync is tolerated and logged
An incremental
didChangewhose range does not fit the buffer (client andserver views drifted) was silently discarded; it is now discarded with an
ERROR log. No desync flag, no refusal of service — a full-document change
or reopen resynchronizes.
Tests
serving rows while content-changed ones are skipped (real shards built
through the test compiler), including line-based symbol resolution;
didCloseclassification (no shard / current shard / divergent shard);the invalidator suite migrated to the split effect lists with
complementary-list assertions.
didChangeresolvecorrectly against the edited buffer; an out-of-range edit produces an
error log and later requests keep working. The pending-flag clear after
a reindex lands is exercised end-to-end by the existing file-tracker
tests (a stuck flag would time them out).
state when it is re-enqueued while its index task is in flight — forcing
that interleaving needs an injectable suspension point in the index
task, which does not exist today.
Summary by CodeRabbit