Repository navigation
fix(server): bounded navigation wait and buffer desync detection - #493
16bit-ykiko wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds ChangesBounded compile wait and desync handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FeatureRouter
participant Compiler
participant SessionStore
participant IndexQuery
Client->>SessionStore: did_change (incremental edit)
alt range unmappable
SessionStore->>SessionStore: mark session.desynced = true
else whole-document or valid range
SessionStore->>SessionStore: clear session.desynced
end
Client->>FeatureRouter: definition(session)
alt session.desynced
FeatureRouter-->>Client: document_out_of_sync error
else
FeatureRouter->>Compiler: ensure_compiled_bounded(session, timeout)
Compiler-->>FeatureRouter: fresh (bool)
FeatureRouter->>IndexQuery: query relations
IndexQuery-->>FeatureRouter: locations (fresh or stale)
FeatureRouter-->>Client: definition response
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 02fe375617
ℹ️ 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".
| bool include_declaration) { | ||
| if(session && session->desynced) | ||
| co_return kota::outcome_error(document_out_of_sync()); | ||
| co_await await_index_freshness(session); |
There was a problem hiding this comment.
When a desyncing didChange arrives while references/declaration/type-definition/etc. are suspended in await_index_freshness, the initial guard has already run. These handlers then call IndexQuery with the now-desynced session; resolve_cursor skips the session index but still maps the cursor through session->line_map() before falling back to shards, so the request returns [] or stale locations instead of the ContentModified error. definition has the needed post-wait guard; the other index-backed handlers need the same re-check after this await.
Useful? React with 👍 / 👎.
| if(session->desynced) | ||
| co_return kota::outcome_error(document_out_of_sync()); | ||
| co_return co_await compiler.forward_query(worker::QueryKind::Hover, session, position); |
There was a problem hiding this comment.
When an invalid didChange arrives after this preflight check but while the forwarded worker request is awaiting ensure_compiled or pool.send, the guard no longer applies. forward_query can return null on the generation mismatch before the router sees session->desynced, or return the old worker result if the edit lands after the pre-send generation check, so hover/semantic tokens/etc. don't consistently report the intended ContentModified error. Re-check session->desynced after the await (or have the compiler forward helpers surface it) for these worker-backed paths.
Useful? React with 👍 / 👎.
| if(!fresh) { | ||
| co_return to_raw( | ||
| index_query.query_relations(path, pos, RelationKind::Definition, session.get())); |
There was a problem hiding this comment.
When this timeout branch runs after an edit inserted or deleted text before the cursor, query_relations receives the dirty session and resolve_cursor converts the current LSP position through session->line_map() before looking in the pre-edit merged shard. That mixes offsets from the edited buffer with occurrences from the old file contents, so a timed-out definition request can jump to an unrelated symbol instead of merely returning a bounded-stale/empty shard result; use the shard's own map (or avoid the shard fallback for shifted dirty buffers) on this path.
Useful? React with 👍 / 👎.
|
Superseded: the approach here (bounded timeout wait + desync refusal) was rejected by the maintainer. A replacement PR will implement a layered freshness policy instead: cursor resolution waits for the current file's compile unconditionally (same as every other request), and cross-TU index queries serve existing results for files whose own content is unchanged while skipping files whose content changed but whose index has not caught up. |
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 `didChange` could resolve the cursor against 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: - **Dependency-only staleness** (a header they include changed — the common cascade case): their own text did not move, so the existing rows keep serving until the reindex lands. - **Own content changed** (disk edit, close after saved edits, compile 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 `Indexer` records a two-level pending reason (`DepsOnly | ContentChanged`, upgrades are 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). `didClose` classifies by comparing the disk content against 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 with two 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 `didChange` whose range does not fit the buffer (client and server 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 - Unit: pending-reason upgrade semantics; deps-only pending files keep serving rows while content-changed ones are skipped (real shards built through the test compiler), including line-based symbol resolution; `didClose` classification (no shard / current shard / divergent shard); the invalidator suite migrated to the split effect lists with complementary-list assertions. - Integration: definition/references immediately after `didChange` resolve correctly 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). - Not covered deterministically: the guard that keeps a file's pending 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Navigation and symbol lookups stay consistent immediately after edits, even while background indexing is still running. * Queries apply a freshness contract, skipping index contributions that are known stale until reindex completes. * **Bug Fixes** * Definition, references, and hierarchy views now await compilation before using index-backed results, avoiding stale/superseded session state. * Invalid incremental edits are logged with details and dropped without breaking subsequent requests. * Reindex behavior is refined for dependency-only vs content-changed cases to improve correctness. * **Tests** * Added integration coverage for post-edit navigation accuracy and desync range tolerance, plus new unit tests for query freshness gating. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Motivation
Two gaps in how feature requests relate to the editor buffer:
Changes
Compiler::ensure_compiled_boundedraces the compile against a timeout;FeatureRouter::await_index_freshnessapplies it with a 1000 ms budget (internal constant — deliberately not configuration) before every position-resolving index query. If the compile lands in time, the query sees the fresh file index; on timeout the query degrades to the shards exactly as before, now with a debug log. The wait is bounded-staleness by design: it waits for any compile round to settle, not necessarily the newest (an invalidation landing mid-flight keeps the dirty flag set), and a timeout abandons only the wait — the detached compile keeps running for the next request.definitionno longer double-waits. Its flow used to forward dirty sessions to the worker, which waits on the same compile without a bound. After the bounded wait expires it now returns the shard answer instead of queueing again.SessionStore::apply_changemarks the sessiondesyncedwhen a change's range cannot be mapped; a whole-document change or didOpen restores authoritative content (a later valid incremental change does not — the buffer it edits is already wrong). Every feature entry refuses a desynced session with error-32801("Document out of sync", LSP's ContentModified code), and the index layer excludes desynced sessions from cross-file queries.definitionre-checks after its bounded wait, since a desyncing didChange can land mid-wait.Tests
definitiondegrades to the shard answer on timeout instead of hanging (uses the real 1 s budget); invalid range marks desync; recovery matrix (valid incremental does not recover, whole-document and didOpen do); index session filter skips dirty and desynced sessions.resolve_cursorfor desynced sessions would need a merged-shard fixture — both paths are covered at unit level or by construction.Local verification: format, RelWithDebInfo build, unit suite 855 passed, integration subset (navigation freshness, protocol edges, index, server basics, rapid edit) passed, smoke tests 3/3.
Summary by CodeRabbit
New Features
Bug Fixes