From 9ff09a6fece51601be2f54edae11837d2d42363d Mon Sep 17 00:00:00 2001 From: ykiko Date: Tue, 7 Jul 2026 23:47:11 +0800 Subject: [PATCH 01/11] fix(server): layered freshness policy for index queries 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. --- src/server/compiler/indexer.cpp | 64 ++++++- src/server/compiler/indexer.h | 60 ++++++- src/server/service/feature_router.cpp | 35 +++- src/server/service/feature_router.h | 8 + src/server/service/query.cpp | 41 +++-- src/server/service/query.h | 42 ++++- src/server/state/invalidator.cpp | 45 +++-- src/server/state/invalidator.h | 16 +- src/server/state/session_store.cpp | 14 ++ src/server/state/session_store.h | 16 +- src/server/transport/master_server.cpp | 15 +- src/server/transport/master_server.h | 2 +- .../features/test_query_freshness.py | 31 ++++ .../lifecycle/test_protocol_edges.py | 48 ++++- tests/unit/server/invalidator_tests.cpp | 105 ++++++++--- tests/unit/server/query_freshness_tests.cpp | 166 ++++++++++++++++++ 16 files changed, 630 insertions(+), 78 deletions(-) create mode 100644 tests/integration/features/test_query_freshness.py create mode 100644 tests/unit/server/query_freshness_tests.cpp diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 49ac81949..6b653f74a 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -288,9 +288,12 @@ void Indexer::load() { workspace.merged_indices[path_id] = std::move(shard); expected_keys.insert(key); } else { + // No shard survives, so there is nothing stale to keep + // serving; ContentChanged states the truth ("the index + // does not describe this file") without effect. LOG_INFO("Discarding unreadable shard for {}", workspace.path_pool.resolve(path_id)); - enqueue(path_id); + enqueue(path_id, ReindexReason::ContentChanged); } } } else { @@ -332,7 +335,19 @@ bool Indexer::need_update(llvm::StringRef file_path) { return merged_it->second.need_update(); } -void Indexer::enqueue(std::uint32_t server_path_id) { +void Indexer::enqueue(std::uint32_t server_path_id, ReindexReason reason) { + // Record (or refresh) why the file is pending. ContentChanged is + // absorbing: a deps-only cascade cannot downgrade a file whose own + // content already changed. The fresh ticket invalidates the clear of + // any index task already in flight for this file. + auto [it, inserted] = reindex_reasons.try_emplace(server_path_id, reason, ++reindex_ticket); + if(!inserted) { + if(reason == ReindexReason::ContentChanged) { + it->second.reason = ReindexReason::ContentChanged; + } + it->second.ticket = reindex_ticket; + } + // Already queued and not yet consumed — a second entry would only be // skipped by need_update later; drop it here. if(!pending_ids.insert(server_path_id).second) @@ -435,6 +450,26 @@ kota::task<> Indexer::index_one(std::uint32_t server_path_id, } } +kota::task<> Indexer::run_index_task(std::uint32_t server_path_id, + std::uint64_t ticket, + std::size_t index, + std::size_t total, + std::size_t& completed) { + co_await index_one(server_path_id, index, total); + // The pending window ends with the index attempt, success or not (a + // failed attempt is re-detected by the hash gate on the next round). + // A re-enqueue during the flight bumped the ticket: that newer pending + // state must survive this clear. + if(auto it = reindex_reasons.find(server_path_id); + it != reindex_reasons.end() && it->second.ticket == ticket) { + reindex_reasons.erase(it); + } + ++completed; + progress_data.stage = Progress::Stage::Report; + progress_data.completed = completed; + on_progress_changed.emit(); +} + kota::task<> Indexer::run_background_indexing() { if(index_idle_timer) { co_await index_idle_timer->wait(); @@ -478,18 +513,28 @@ kota::task<> Indexer::run_background_indexing() { pending_ids.erase(server_path_id); auto file_path = std::string(workspace.path_pool.resolve(server_path_id)); if(sessions.find(server_path_id) != nullptr || !need_update(file_path)) { + // Not pending anymore: an open session's buffer index is + // authoritative, and a hash-fresh shard already describes the + // current content. + reindex_reasons.erase(server_path_id); ++completed; continue; } ++dispatched; - workers.spawn([&, server_path_id, n = dispatched]() -> kota::task<> { - co_await index_one(server_path_id, n, total); - ++completed; - progress_data.stage = Progress::Stage::Report; - progress_data.completed = completed; - on_progress_changed.emit(); - }()); + // Invariant: a queued file always has a pending entry (enqueue + // writes it before the queue push, and nothing erases it while it + // sits in the un-consumed tail). Ticket 0 is never issued, so if + // the invariant ever broke in a release build the task would run + // normally and simply leave no entry to clear. + auto pending_it = reindex_reasons.find(server_path_id); + assert(pending_it != reindex_reasons.end() && "queued file must have a pending reason"); + auto ticket = pending_it != reindex_reasons.end() ? pending_it->second.ticket : 0; + // A member coroutine, not an immediately-invoked capturing lambda: + // a lambda's captures live in the lambda object, which dies at the + // end of this statement — anything read after the first suspension + // would dangle. Coroutine parameters are copied into the frame. + workers.spawn(run_index_task(server_path_id, ticket, dispatched, total, completed)); } LOG_DEBUG("Background indexing: all {} tasks spawned, waiting for completion", dispatched); @@ -509,6 +554,7 @@ kota::task<> Indexer::run_background_indexing() { // the next scheduled round. if(index_queue_pos >= index_queue.size()) { assert(pending_ids.empty() && "drained queue must have no pending ids"); + assert(reindex_reasons.empty() && "drained queue must have no pending reasons"); index_queue.clear(); index_queue_pos = 0; } diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index 0a6db40f0..a4fa0dae3 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -3,12 +3,14 @@ #include #include #include +#include #include #include "server/state/workspace.h" #include "support/signal.h" #include "kota/async/async.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/StringRef.h" @@ -18,6 +20,22 @@ class ContextResolver; class WorkerPool; struct SessionStore; +/// Why a file awaits re-indexing. The invalidation engine knows the cause +/// at enqueue time, so queries can decide in O(1) whether a pending file's +/// existing index rows are still trustworthy (see IndexQuery's freshness +/// contract). +enum class ReindexReason : std::uint8_t { + /// Enqueued by a dependency cascade (or a bulk sweep of unknown + /// staleness): the file's own content is not known to have changed, so + /// its index rows are positionally intact — at worst semantically + /// behind — and keep serving until the reindex lands. + DepsOnly, + /// The file's own content changed: its index rows describe text that + /// no longer exists, so queries skip this file's contribution until + /// the reindex lands. + ContentChanged, +}; + /// Background indexing scheduler. /// /// Indexer owns the indexing queue and drives disk files through @@ -71,8 +89,22 @@ class Indexer { return ScopedPause{*this}; } - /// Add a file to the background indexing queue. - void enqueue(std::uint32_t server_path_id); + /// Add a file to the background indexing queue. A file enqueued twice + /// keeps a single queue entry; its reason is upgraded to ContentChanged + /// if either enqueue says so (a file both cascaded onto and edited is + /// as stale as the edit makes it). + void enqueue(std::uint32_t server_path_id, ReindexReason reason); + + /// Why the file awaits re-indexing (queued or currently being indexed), + /// or nullopt when its index is not pending an update. O(1), no I/O — + /// the query path calls this per candidate file. + std::optional pending_reason(std::uint32_t server_path_id) const { + auto it = reindex_reasons.find(server_path_id); + if(it == reindex_reasons.end()) { + return std::nullopt; + } + return it->second.reason; + } /// Schedule background indexing (respects idle timeout and dedup). void schedule(); @@ -147,6 +179,20 @@ class Indexer { std::vector index_queue; llvm::DenseSet pending_ids; std::size_t index_queue_pos = 0; + + /// The reason each file awaits re-indexing, kept from enqueue until the + /// file is skipped (open session, fresh shard) or its index task ends — + /// unlike pending_ids, which only covers the un-consumed queue tail. + /// The ticket guards the clear: a file re-enqueued while its previous + /// index task is still in flight bumps the ticket, so the older task's + /// completion must not erase the newer pending state. + struct PendingReindex { + ReindexReason reason; + std::uint64_t ticket; + }; + + llvm::DenseMap reindex_reasons; + std::uint64_t reindex_ticket = 0; bool indexing_active = false; bool indexing_scheduled = false; std::shared_ptr index_idle_timer; @@ -160,6 +206,16 @@ class Indexer { kota::task<> run_background_indexing(); kota::task<> index_one(std::uint32_t server_path_id, std::size_t index, std::size_t total); + + /// One dispatched unit of a background round: index the file, then end + /// its pending window (ticket-guarded) and report progress. `completed` + /// refers into run_background_indexing's frame, which outlives every + /// spawned task (it joins them before returning). + kota::task<> run_index_task(std::uint32_t server_path_id, + std::uint64_t ticket, + std::size_t index, + std::size_t total, + std::size_t& completed); }; } // namespace clice diff --git a/src/server/service/feature_router.cpp b/src/server/service/feature_router.cpp index ffdf10bfa..b37646de9 100644 --- a/src/server/service/feature_router.cpp +++ b/src/server/service/feature_router.cpp @@ -37,6 +37,12 @@ static kota::ipc::Error item_not_resolved(llvm::StringRef kind) { std::format("Failed to resolve {} item", kind)}; } +kota::task<> FeatureRouter::settle_cursor_file(std::shared_ptr session) { + if(session) { + co_await compiler.ensure_compiled(std::move(session)); + } +} + const std::vector* FeatureRouter::find_preamble_links(const Session& session) { if(!session.pch_ref) @@ -101,10 +107,13 @@ kota::task FeatureRouter::definition(std::shared_ptr session, llvm::StringRef path, const protocol::Position& pos) { + co_await settle_cursor_file(session); + // Preamble include lines first: they have no symbol occurrence in - // the index and are invisible to the worker's AST. Dirty sessions - // skip this — the cached links may describe the pre-edit preamble — - // and retry below once the worker compile refreshed the PCH. + // the index and are invisible to the worker's AST. A session still + // dirty after the settle (failed or superseded compile) skips this — + // the cached links may describe the pre-edit preamble — and retries + // below once the worker compile refreshed the PCH. if(session && !session->ast_dirty) { if(auto directive = resolve_directive_definition(*session, pos); !directive.empty()) { co_return to_raw(directive); @@ -254,6 +263,8 @@ FeatureRouter::RawResult FeatureRouter::references(std::shared_ptr sess llvm::StringRef path, const protocol::Position& position, bool include_declaration) { + co_await settle_cursor_file(session); + auto locations = index_query.query_relations(path, position, RelationKind::Reference, session.get()); @@ -275,6 +286,8 @@ FeatureRouter::RawResult FeatureRouter::references(std::shared_ptr sess FeatureRouter::RawResult FeatureRouter::declaration(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { + co_await settle_cursor_file(session); + auto locations = index_query.query_relations(path, position, RelationKind::Declaration, session.get()); auto defs = @@ -288,6 +301,8 @@ FeatureRouter::RawResult FeatureRouter::declaration(std::shared_ptr ses FeatureRouter::RawResult FeatureRouter::type_definition(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { + co_await settle_cursor_file(session); + co_return to_raw(index_query.query_symbol_targets(path, position, RelationKind::TypeDefinition, @@ -297,6 +312,8 @@ FeatureRouter::RawResult FeatureRouter::type_definition(std::shared_ptr FeatureRouter::RawResult FeatureRouter::implementation(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { + co_await settle_cursor_file(session); + co_return to_raw(index_query.query_symbol_targets(path, position, RelationKind::Implementation, @@ -307,6 +324,8 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_prepare(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { + co_await settle_cursor_file(session); + auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); if(!info) @@ -334,6 +355,8 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_outgoing(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { + co_await settle_cursor_file(session); + auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); if(!info) @@ -346,6 +369,8 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_prepare(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { + co_await settle_cursor_file(session); + auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); if(!info) @@ -374,6 +401,8 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_subtypes(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { + co_await settle_cursor_file(session); + auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); if(!info) diff --git a/src/server/service/feature_router.h b/src/server/service/feature_router.h index 9b713565e..3561a3767 100644 --- a/src/server/service/feature_router.h +++ b/src/server/service/feature_router.h @@ -139,6 +139,14 @@ class FeatureRouter { RawResult workspace_symbol(llvm::StringRef query); private: + /// Cursor-resolution freshness (IndexQuery's contract, clause 1): an + /// index query that resolves a cursor in an open file waits for the + /// file's compile first, exactly like every AST-backed request does — + /// same await, no timeout. A failed or superseded compile is not an + /// error here: the query layer falls back per its contract. No-op for + /// closed documents (null session). + kota::task<> settle_cursor_file(std::shared_ptr session); + /// The preamble include links of a session's active PCH, or nullptr. const std::vector* find_preamble_links(const Session& session); diff --git a/src/server/service/query.cpp b/src/server/service/query.cpp index 9405ba2e2..3fe35ea11 100644 --- a/src/server/service/query.cpp +++ b/src/server/service/query.cpp @@ -8,6 +8,7 @@ #include "index/tu_index.h" #include "server/compiler/compiler.h" +#include "server/compiler/indexer.h" #include "server/state/session.h" #include "server/state/session_store.h" #include "support/filesystem.h" @@ -24,8 +25,8 @@ namespace lsp = kota::ipc::lsp; void IndexQuery::visit_sessions(SessionVisitor visitor) const { sessions.for_each([&](std::uint32_t path_id, const Session& session) -> bool { - // FIXME: when ast_dirty, consider awaiting recompilation - // instead of silently falling back to MergedIndex. + // Freshness contract, clause 3: a dirty session's file index may + // describe a buffer that no longer exists — skip it. if(session.file_index && session.symbols && !session.ast_dirty) { return visitor(path_id, session); } @@ -37,6 +38,15 @@ bool IndexQuery::is_path_open(std::uint32_t path_id) const { return sessions.find(path_id) != nullptr; } +bool IndexQuery::skip_stale_contribution(std::uint32_t path_id) const { + // With background indexing disabled nothing ever catches up: serving + // the last-known rows beats a permanent hole. + if(!*workspace.config.project.enable_indexing) { + return false; + } + return indexer.pending_reason(path_id) == ReindexReason::ContentChanged; +} + bool IndexQuery::find_symbol_info(index::SymbolHash hash, std::string& name, SymbolKind& kind) const { @@ -78,8 +88,10 @@ bool IndexQuery::find_symbol_info(index::SymbolHash hash, IndexQuery::CursorHit IndexQuery::resolve_cursor(llvm::StringRef path, const protocol::Position& position, Session* session) { - // FIXME: when ast_dirty, we fall back to MergedIndex which may be staler. - // Consider awaiting the pending recompilation to serve fresher results. + // Freshness contract, clause 1: callers awaited the session's compile, + // so a session that is still dirty here had its compile fail or be + // superseded — fall through to the merged shard, which resolves + // against its own stored snapshot. if(session && session->file_index && !session->ast_dirty) { auto map = session->line_map(); auto offset = map.to_offset(position); @@ -103,6 +115,10 @@ IndexQuery::CursorHit IndexQuery::resolve_cursor(llvm::StringRef path, auto path_id = workspace.path_pool.find(path); if(!path_id) return {}; + // A content-changed pending file's rows describe stale text: a cursor + // resolved against them would name the wrong symbol. + if(skip_stale_contribution(*path_id)) + return {}; auto shard_it = workspace.merged_indices.find(*path_id); if(shard_it == workspace.merged_indices.end()) return {}; @@ -139,7 +155,7 @@ std::vector IndexQuery::query_relations(llvm::StringRef path auto sym_it = workspace.project_index.symbols.find(hit.hash); if(sym_it != workspace.project_index.symbols.end()) { for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -237,7 +253,7 @@ std::optional IndexQuery::find_definition_location(index::Sy return std::nullopt; for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -291,7 +307,7 @@ void IndexQuery::collect_grouped_relations( auto sym_it = workspace.project_index.symbols.find(hash); if(sym_it != workspace.project_index.symbols.end()) { for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -326,7 +342,7 @@ void IndexQuery::collect_unique_targets(index::SymbolHash hash, auto sym_it = workspace.project_index.symbols.find(hash); if(sym_it != workspace.project_index.symbols.end()) { for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -412,7 +428,7 @@ std::optional IndexQuery::get_definition_text(index: return std::nullopt; for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -455,7 +471,7 @@ std::vector IndexQuery::collect_references(ind auto sym_it = workspace.project_index.symbols.find(hash); if(sym_it != workspace.project_index.symbols.end()) { for(auto file_id: sym_it->second.reference_files) { - if(is_path_open(file_id)) + if(is_path_open(file_id) || skip_stale_contribution(file_id)) continue; auto shard_it = workspace.merged_indices.find(file_id); if(shard_it == workspace.merged_indices.end()) @@ -738,6 +754,11 @@ std::vector IndexQuery::locate_symbols(const agentic::ReadSymbol if(!path_id) return {}; + // The shard's line numbers describe stale text; resolving the + // requested line against them would name the wrong symbol. + if(skip_stale_contribution(*path_id)) + return {}; + auto shard_it = workspace.merged_indices.find(*path_id); if(shard_it == workspace.merged_indices.end()) return {}; diff --git a/src/server/service/query.h b/src/server/service/query.h index acaddcc08..68b4f6e6c 100644 --- a/src/server/service/query.h +++ b/src/server/service/query.h @@ -23,6 +23,7 @@ namespace clice { namespace protocol = kota::ipc::protocol; namespace lsp = kota::ipc::lsp; +class Indexer; struct Session; struct SessionStore; @@ -58,14 +59,45 @@ struct ResolvedSymbol { /// - Compilation — handled by Compiler /// - Background indexing — handled by Indexer /// - Document lifecycle — handled by MasterServer +/// +/// Freshness contract — results may be incomplete, by design: +/// +/// 1. Cursor resolution (turning an offset in the request's file into a +/// symbol) is accurate: callers that hold an open session await its +/// compile first (FeatureRouter awaits ensure_compiled with the same +/// no-timeout posture as every AST-backed request), so the session's +/// file index describes the buffer being pointed at. For closed files +/// the merged shard resolves against its own stored content snapshot +/// — unless the file's own content changed and its reindex is still +/// pending, in which case the cursor is unresolvable (clause 2). +/// 2. Cross-file contributions honor the indexer's pending state: a file +/// awaiting reindex only because a dependency changed keeps serving +/// its previous rows (its own text did not move), while a file whose +/// own content changed has its contribution skipped until the reindex +/// lands — stale rows would point at text that no longer exists. +/// 3. Open sessions whose compile has not (re)finished are skipped +/// entirely (see visit_sessions): their buffer may have diverged from +/// the last file index, and unlike closed files their reindex is the +/// next compile, which the current file's request already awaits. +/// +/// Symbol identity lookups (find_symbol_info: hash → name/kind) are not +/// gated: a hash identifies one symbol, so even a stale shard answers +/// them correctly. +/// +/// TODO: a blocking query mode (await the pending reindexes instead of +/// skipping) for consumers that need completeness over latency. Not +/// implemented — no current caller wants to stall on a full queue. +/// TODO: a dedicated "is the index ready?" request so agent consumers +/// can distinguish "no references" from "not indexed yet". Not +/// implemented — needs protocol design. class IndexQuery { public: /// Visitor for iterating open Sessions. Returns false to stop early. using SessionVisitor = std::function; - IndexQuery(Workspace& workspace, const SessionStore& sessions) : - workspace(workspace), sessions(sessions) {} + IndexQuery(Workspace& workspace, const SessionStore& sessions, const Indexer& indexer) : + workspace(workspace), sessions(sessions), indexer(indexer) {} /// Query relations (Definition, Reference, etc.) for a symbol at cursor. /// @param session Active Session for this file, or nullptr to use MergedIndex only. @@ -198,8 +230,14 @@ class IndexQuery { /// Check whether a path_id has an active Session. bool is_path_open(std::uint32_t path_id) const; + /// Freshness contract, clause 2: whether a closed file's contribution + /// must be skipped because its own content changed and the reindex has + /// not landed yet. O(1) per candidate file, no I/O. + bool skip_stale_contribution(std::uint32_t path_id) const; + Workspace& workspace; const SessionStore& sessions; + const Indexer& indexer; }; } // namespace clice diff --git a/src/server/state/invalidator.cpp b/src/server/state/invalidator.cpp index fbafef1a3..ea91abfc8 100644 --- a/src/server/state/invalidator.cpp +++ b/src/server/state/invalidator.cpp @@ -40,7 +40,7 @@ void Invalidator::cascade_compile_graph(std::uint32_t path_id, DirtySet& dirty) if(store.find(dirty_id)) { dirty.mark_ast_dirty.push_back(dirty_id); } else { - dirty.enqueue_reindex.push_back(dirty_id); + dirty.reindex_deps_only.push_back(dirty_id); } } } @@ -66,7 +66,7 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d if(store.find(dirty_id)) { dirty.mark_ast_dirty.push_back(dirty_id); } else { - dirty.enqueue_reindex.push_back(dirty_id); + dirty.reindex_deps_only.push_back(dirty_id); } } @@ -82,7 +82,7 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d if(store.find(root)) { dirty.mark_ast_dirty.push_back(root); } else { - dirty.enqueue_reindex.push_back(root); + dirty.reindex_deps_only.push_back(root); } } }; @@ -101,9 +101,10 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d dirty.reset_header_mode.push_back(header_id); // Contexts outlive their sessions: a closed header's shard rows // were indexed under the old chain and only a background reindex - // can refresh them. + // can refresh them. The header's own content did not change, so + // its rows keep serving meanwhile. if(!store.find(header_id)) { - dirty.enqueue_reindex.push_back(header_id); + dirty.reindex_deps_only.push_back(header_id); } } @@ -163,7 +164,23 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // the file back to the background indexer, whose shard now // supersedes the dropped session's index. workspace.on_file_closed(event.path_id); - dirty.enqueue_reindex.push_back(event.path_id); + // Whether the shard's rows still describe the disk decides + // how queries treat the file until the reindex lands: a + // browse-and-close must not blank the file's references for + // the queue's latency, while a close after saved edits must + // not serve rows for text that no longer exists. One disk + // read settles it; an unreadable file counts as changed. + auto shard_it = workspace.merged_indices.find(event.path_id); + bool shard_current = false; + if(shard_it != workspace.merged_indices.end()) { + auto disk = read_file(workspace.path_pool.resolve(event.path_id)); + shard_current = disk && *disk == shard_it->second.content(); + } + if(shard_current) { + dirty.reindex_deps_only.push_back(event.path_id); + } else { + dirty.reindex_content_changed.push_back(event.path_id); + } dirty.reschedule_indexing = true; break; } @@ -180,7 +197,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // Closed file: disk is the truth. Run the same cascade a // save does, and refresh the file's own now-stale shard. cascade_disk_content_change(path_id, dirty); - dirty.enqueue_reindex.push_back(path_id); + dirty.reindex_content_changed.push_back(path_id); break; } case FileEvent::Kind::DiskRemoved: { @@ -193,7 +210,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { if(store.find(root)) { dirty.mark_ast_dirty.push_back(root); } else { - dirty.enqueue_reindex.push_back(root); + dirty.reindex_deps_only.push_back(root); } } // A removed module unit takes its PCM with it: importers' @@ -269,13 +286,16 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // The shard was indexed under the old command, and // the indexer's freshness gate validates content // only: evict the shard so the queued reindex is - // not filtered out as fresh. + // not filtered out as fresh. ContentChanged: a new + // command can rewrite the rows (macros, includes) + // as thoroughly as an edit — and the shard is gone + // anyway. // TODO: a background index task already in flight // can merge its old-command result back after this // eviction; closing that window needs an index // generation guard in the indexer. workspace.merged_indices.erase(path_id); - dirty.enqueue_reindex.push_back(path_id); + dirty.reindex_content_changed.push_back(path_id); } // A module unit's command change invalidates importers' @@ -299,7 +319,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dirty.mark_ast_dirty.push_back(header_id); } else { workspace.merged_indices.erase(header_id); - dirty.enqueue_reindex.push_back(header_id); + dirty.reindex_content_changed.push_back(header_id); } } }; @@ -363,7 +383,8 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dedup(dirty.reset_trial); dedup(dirty.reset_header_mode); dedup(dirty.force_revalidate); - dedup(dirty.enqueue_reindex); + dedup(dirty.reindex_content_changed); + dedup(dirty.reindex_deps_only); dedup(dirty.drop_context); return dirty; } diff --git a/src/server/state/invalidator.h b/src/server/state/invalidator.h index b5e7a4e23..95bb387b0 100644 --- a/src/server/state/invalidator.h +++ b/src/server/state/invalidator.h @@ -136,9 +136,16 @@ struct DirtySet { /// zero deps.build_at so every chain file is re-validated by hash, plus /// the mark_ast_dirty treatment. llvm::SmallVector force_revalidate; - /// Closed files whose index entries went stale: enqueue for background - /// reindexing. - llvm::SmallVector enqueue_reindex; + /// Closed files whose own content changed: their index rows describe + /// text that no longer exists. Enqueue for background reindexing as + /// ReindexReason::ContentChanged — queries skip these files' + /// contributions until the reindex lands. + llvm::SmallVector reindex_content_changed; + /// Closed files enqueued only because a dependency changed: their own + /// rows are positionally intact. Enqueue as ReindexReason::DepsOnly — + /// queries keep serving the previous rows. A file in both lists is + /// ContentChanged (the indexer's reason upgrade is absorbing). + llvm::SmallVector reindex_deps_only; /// Headers whose resolved context borrows a compile command that no /// longer exists in that form (the host's CDB entry changed): drop the /// context so the next use re-resolves. Content validation cannot see @@ -159,7 +166,8 @@ struct DirtySet { bool empty() const { return mark_ast_dirty.empty() && mark_lost.empty() && reset_trial.empty() && - reset_header_mode.empty() && force_revalidate.empty() && enqueue_reindex.empty() && + reset_header_mode.empty() && force_revalidate.empty() && + reindex_content_changed.empty() && reindex_deps_only.empty() && drop_context.empty() && !recheck_contexts && !save_cache && !reschedule_indexing && !ensure_compile_graph; } diff --git a/src/server/state/session_store.cpp b/src/server/state/session_store.cpp index 4a2ef5dc7..cf090af93 100644 --- a/src/server/state/session_store.cpp +++ b/src/server/state/session_store.cpp @@ -4,6 +4,8 @@ #include #include +#include "support/logging.h" + #include "kota/ipc/lsp/position.h" namespace clice { @@ -68,6 +70,18 @@ void SessionStore::apply_change(Session& session, auto end = map.to_offset(range.end); if(start && end && *start <= *end) { session.text.replace(*start, *end - *start, c.text); + } else { + // The client's view has drifted from ours (or the + // client is buggy). Drop the edit but keep serving: + // a full-document change or reopen resynchronizes. + LOG_ERROR( + "didChange range {}:{}-{}:{} does not fit the buffer " "(path_id={} version={}); edit dropped", + range.start.line, + range.start.character, + range.end.line, + range.end.character, + session.path_id, + version); } } session.line_starts = lsp::build_line_starts(session.text); diff --git a/src/server/state/session_store.h b/src/server/state/session_store.h index 45fe76b9f..f5a3a07e8 100644 --- a/src/server/state/session_store.h +++ b/src/server/state/session_store.h @@ -37,11 +37,14 @@ enum class ResetDepth : std::uint8_t { /// here, and every reader of an open file's text goes through the sessions /// this store hands out. /// -/// Future work: this store does not yet detect buffer desync (client and -/// server drifting out of sync) or bound the number of concurrently open -/// sessions. Those safeguards are left for later. Non-monotonic document -/// versions are warned about at the transport edge, where the protocol -/// context lives. +/// Buffer desync (client and server drifting out of sync) is tolerated: an +/// incremental edit whose range does not fit the buffer is dropped with an +/// ERROR log, and requests keep being served — a full-document change or a +/// reopen resynchronizes. Non-monotonic document versions are warned about +/// at the transport edge, where the protocol context lives. +/// +/// Future work: this store does not yet bound the number of concurrently +/// open sessions. struct SessionStore { llvm::DenseMap> sessions; @@ -67,7 +70,8 @@ struct SessionStore { /// Apply a didChange: fold the content changes into the buffer (range → /// offset mapping, in-place text replacement, line-start rebuild), then /// bump version, generation and mark the AST dirty. Changes whose range - /// cannot be mapped to a valid offset span are silently dropped. + /// cannot be mapped to a valid offset span are dropped with an ERROR + /// log (see the struct comment on desync tolerance). void apply_change(Session& session, llvm::ArrayRef changes, int version); diff --git a/src/server/transport/master_server.cpp b/src/server/transport/master_server.cpp index 330bcd713..3182ee051 100644 --- a/src/server/transport/master_server.cpp +++ b/src/server/transport/master_server.cpp @@ -34,7 +34,7 @@ constexpr static std::size_t notify_log_limit = 128; MasterServer::MasterServer(kota::event_loop& loop, std::string self_path) : loop(loop), pool(loop), contexts(workspace), compiler(loop, workspace, contexts, pool), - index_query(workspace, sessions), indexer(loop, workspace, pool, contexts, sessions), + indexer(loop, workspace, pool, contexts, sessions), index_query(workspace, sessions, indexer), features(compiler, index_query, workspace, contexts, indexer), invalidator(workspace, sessions, contexts), bg_tasks(loop), self_path(std::move(self_path)) { // The notify hook is process-wide because the logging layer cannot @@ -289,8 +289,11 @@ void MasterServer::dispatch(llvm::ArrayRef events) { contexts.drop_header_context(path_id); } - for(auto path_id: dirty.enqueue_reindex) { - indexer.enqueue(path_id); + for(auto path_id: dirty.reindex_content_changed) { + indexer.enqueue(path_id, ReindexReason::ContentChanged); + } + for(auto path_id: dirty.reindex_deps_only) { + indexer.enqueue(path_id, ReindexReason::DepsOnly); } if(dirty.ensure_compile_graph && !workspace.compile_graph) { @@ -447,7 +450,11 @@ void MasterServer::load_workspace() { for(auto& entry: workspace.cdb.get_entries()) { auto file = workspace.cdb.resolve_path(entry.file); auto server_id = workspace.path_pool.intern(file); - indexer.enqueue(server_id); + // Bulk sweep of unknown staleness: the hash gate decides per + // file. DepsOnly — a cold start with a warm index cache must + // keep serving the loaded shards, not blank every query until + // the sweep drains. + indexer.enqueue(server_id, ReindexReason::DepsOnly); } indexer.schedule(); } diff --git a/src/server/transport/master_server.h b/src/server/transport/master_server.h index 661d1a06c..a785129fb 100644 --- a/src/server/transport/master_server.h +++ b/src/server/transport/master_server.h @@ -135,8 +135,8 @@ class MasterServer { WorkerPool pool; ContextResolver contexts; Compiler compiler; - IndexQuery index_query; Indexer indexer; + IndexQuery index_query; FeatureRouter features; Invalidator invalidator; diff --git a/tests/integration/features/test_query_freshness.py b/tests/integration/features/test_query_freshness.py new file mode 100644 index 000000000..81b1d3d9b --- /dev/null +++ b/tests/integration/features/test_query_freshness.py @@ -0,0 +1,31 @@ +"""Navigation right after an edit must resolve against the edited buffer: +the server settles the file's compile before answering, with no timeout.""" + +from tests.integration.utils import write_cdb +from tests.integration.utils.workspace import did_change + +SOURCE_V1 = "int foo() { return 1; }\nint main() { return foo(); }\n" + +# Inserts a line at the top: every position shifts, so a query resolved +# against the pre-edit index would name the wrong symbol or nothing. +SOURCE_V2 = "// shift\nint foo() { return 1; }\nint main() { return foo(); }\n" + + +async def test_navigation_after_change(client, tmp_path): + (tmp_path / "main.cpp").write_text(SOURCE_V1, newline="\n") + write_cdb(tmp_path, ["main.cpp"]) + await client.initialize(tmp_path) + + uri, _ = await client.open_and_wait(tmp_path / "main.cpp") + + # No wait after the edit: the requests below must settle the compile + # themselves before resolving the cursor. + did_change(client, uri, 2, SOURCE_V2) + + defs = await client.definition_at(uri, 2, 20) + assert defs, "definition right after an edit returned nothing" + assert defs[0].range.start.line == 1 + + refs = await client.references_at(uri, 2, 20, include_declaration=False) + assert refs, "references right after an edit returned nothing" + assert {loc.range.start.line for loc in refs} == {2} diff --git a/tests/integration/lifecycle/test_protocol_edges.py b/tests/integration/lifecycle/test_protocol_edges.py index bd09ac2b3..1f2c31e53 100644 --- a/tests/integration/lifecycle/test_protocol_edges.py +++ b/tests/integration/lifecycle/test_protocol_edges.py @@ -5,7 +5,16 @@ import asyncio import pytest -from lsprotocol.types import ClientCapabilities, InitializedParams, InitializeParams +from lsprotocol.types import ( + ClientCapabilities, + DidChangeTextDocumentParams, + InitializedParams, + InitializeParams, + Position, + Range, + TextDocumentContentChangePartial, + VersionedTextDocumentIdentifier, +) from tests.conftest import check_no_anomaly, shutdown_client from tests.integration.utils.assertions import get_errors, guidance_messages @@ -75,6 +84,43 @@ async def test_change_without_open(client, workspace): assert get_errors(client.diagnostics[uri]) == [] +async def test_desync_range_tolerated(client, tmp_path): + write_source(tmp_path, "main.cpp", "int foo() { return 1; }\n") + write_cdb(tmp_path, ["main.cpp"]) + await client.initialize(tmp_path) + uri, _ = await client.open_and_wait(tmp_path / "main.cpp") + + # An incremental edit whose range lies outside the buffer: the views + # have drifted. The edit is dropped with an ERROR log; no refusal. + client.text_document_did_change( + DidChangeTextDocumentParams( + text_document=VersionedTextDocumentIdentifier(uri=uri, version=2), + content_changes=[ + TextDocumentContentChangePartial( + range=Range( + start=Position(line=999, character=0), + end=Position(line=999, character=5), + ), + text="oops", + ) + ], + ) + ) + + # Requests keep being served from the retained buffer. + hover = await client.hover_at(uri, 0, 4) + assert hover is not None + + logs_dir = tmp_path / ".clice" / "logs" + for _ in range(50): + logs = "".join(f.read_text(errors="replace") for f in logs_dir.rglob("*.log")) + if "didChange range" in logs: + break + await asyncio.sleep(0.1) + else: + pytest.fail("dropped out-of-sync edit never produced an error log") + + @pytest.mark.workspace("hello_world") async def test_version_regression_tolerated(client, workspace): uri, content = client.open(workspace / "main.cpp", version=5) diff --git a/tests/unit/server/invalidator_tests.cpp b/tests/unit/server/invalidator_tests.cpp index d53ecc80f..2d9bff268 100644 --- a/tests/unit/server/invalidator_tests.cpp +++ b/tests/unit/server/invalidator_tests.cpp @@ -95,7 +95,8 @@ TEST_CASE(CascadeSplitsOpenClosed) { EXPECT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_user}); llvm::SmallVector reindexed{mod, closed_user}; llvm::sort(reindexed); - EXPECT_EQ(dirty.enqueue_reindex, reindexed); + EXPECT_EQ(dirty.reindex_deps_only, reindexed); + EXPECT_TRUE(dirty.reindex_content_changed.empty()); co_await workspace.compile_graph->shutdown(); }; @@ -132,7 +133,9 @@ TEST_CASE(ChainHitAndMiss) { llvm::SmallVector reset{saved, hit, closed}; llvm::sort(reset); ASSERT_EQ(dirty.reset_header_mode, reset); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{closed}); + // The closed header's own content did not change — only its chain did. + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{closed}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); } TEST_CASE(SaveMarksDependents) { @@ -151,9 +154,11 @@ TEST_CASE(SaveMarksDependents) { auto dirty = invalidator.apply(FileEvent::buffer_saved(header)); // Open dependents recompile, closed ones reindex; the old/new dependent - // snapshots overlap fully here, so this also proves the dedup. + // snapshots overlap fully here, so this also proves the dedup. A + // dependent's own content did not change: deps-only. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_tu}); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{closed_tu}); + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{closed_tu}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); } TEST_CASE(TransitiveDependentsEnqueue) { @@ -171,7 +176,8 @@ TEST_CASE(TransitiveDependentsEnqueue) { auto dirty = invalidator.apply(FileEvent::buffer_saved(header)); // Only root TUs own index shards; the intermediate header is not one. - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{root}); + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{root}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); ASSERT_TRUE(dirty.mark_ast_dirty.empty()); } @@ -193,10 +199,11 @@ TEST_CASE(StaleReverseMapUnion) { llvm::SmallVector expected{known, unmapped}; llvm::sort(expected); - ASSERT_EQ(dirty.enqueue_reindex, expected); + ASSERT_EQ(dirty.reindex_deps_only, expected); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); } -TEST_CASE(CloseEnqueuesReindex) { +TEST_CASE(CloseWithoutShardReindexes) { Workspace workspace; SessionStore store; auto closed = workspace.path_pool.intern("/proj/a.cpp"); @@ -205,11 +212,49 @@ TEST_CASE(CloseEnqueuesReindex) { Invalidator invalidator(workspace, store, resolver); auto dirty = invalidator.apply(FileEvent::buffer_closed(closed)); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{closed}); + // No shard to compare against: nothing serves this file's rows anyway. + ASSERT_EQ(dirty.reindex_content_changed, llvm::SmallVector{closed}); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.reschedule_indexing); ASSERT_TRUE(dirty.mark_ast_dirty.empty()); } +TEST_CASE(CloseCurrentShardDepsOnly) { + Workspace workspace; + SessionStore store; + auto closed = workspace.path_pool.intern("/proj/a.cpp"); + workspace.merged_indices[closed]; + + ContextResolver resolver(workspace); + // Disk matches the shard's stored content: a browse-and-close must not + // blank the file's rows for the reindex queue's latency. + Invalidator invalidator(workspace, store, resolver, [](llvm::StringRef) { + return std::optional{""}; + }); + auto dirty = invalidator.apply(FileEvent::buffer_closed(closed)); + + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{closed}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); +} + +TEST_CASE(CloseDivergentShardContentChanged) { + Workspace workspace; + SessionStore store; + auto closed = workspace.path_pool.intern("/proj/a.cpp"); + workspace.merged_indices[closed]; + + ContextResolver resolver(workspace); + // Disk holds edits the shard never saw (saved while open): the shard's + // rows describe text that no longer exists. + Invalidator invalidator(workspace, store, resolver, [](llvm::StringRef) { + return std::optional{"int edited;"}; + }); + auto dirty = invalidator.apply(FileEvent::buffer_closed(closed)); + + ASSERT_EQ(dirty.reindex_content_changed, llvm::SmallVector{closed}); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); +} + TEST_CASE(CrashMarksLostDirty) { Workspace workspace; SessionStore store; @@ -312,7 +357,8 @@ TEST_CASE(DiskChangeOpenMarksDirty) { // compile's deps validation judges the disk change, but no rescan and // no cascade. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_file}); - ASSERT_TRUE(dirty.enqueue_reindex.empty()); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.reset_trial.empty()); ASSERT_FALSE(dirty.recheck_contexts); } @@ -333,11 +379,11 @@ TEST_CASE(DiskChangeClosedCascades) { auto dirty = invalidator.apply(FileEvent::disk_changed(header)); // A closed file's disk change cascades exactly like a save, plus the - // file's own stale shard is refreshed. + // file's own stale shard is refreshed. The changed file's own rows are + // untrustworthy; its dependent only rebuilds semantics. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_tu}); - llvm::SmallVector reindexed{header, closed_tu}; - llvm::sort(reindexed); - ASSERT_EQ(dirty.enqueue_reindex, reindexed); + ASSERT_EQ(dirty.reindex_content_changed, llvm::SmallVector{header}); + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{closed_tu}); ASSERT_EQ(dirty.reset_trial, llvm::SmallVector{header}); ASSERT_TRUE(dirty.recheck_contexts); ASSERT_TRUE(dirty.reschedule_indexing); @@ -363,7 +409,8 @@ TEST_CASE(DiskRemovedScrubsSourceRole) { ASSERT_EQ(workspace.dep_graph.get_includers(header), llvm::ArrayRef{other_tu}); ASSERT_TRUE(workspace.dep_graph.get_all_includes(removed_tu).empty()); ASSERT_TRUE(dirty.recheck_contexts); - ASSERT_TRUE(dirty.enqueue_reindex.empty()); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.mark_ast_dirty.empty()); ASSERT_EQ(workspace.context_epoch, epoch + 1); } @@ -389,8 +436,10 @@ TEST_CASE(CDBAddedScansAndEnqueues) { auto dirty = invalidator.apply(FileEvent::cdb_changed(std::move(delta))); // The rescan resolved the new entry's includes; the new file reindexes. + // A command change rewrites rows as thoroughly as an edit. ASSERT_EQ(workspace.dep_graph.get_includers(header_id), llvm::ArrayRef{main_id}); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{main_id}); + ASSERT_EQ(dirty.reindex_content_changed, llvm::SmallVector{main_id}); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.recheck_contexts); ASSERT_TRUE(dirty.ensure_compile_graph); } @@ -422,7 +471,8 @@ TEST_CASE(CDBChangedSplitsOpenClosed) { // Flag changes recompile open files and reindex closed ones; the // pull-side cache keys (canonical flags) miss on their own. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_id}); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{closed_id}); + ASSERT_EQ(dirty.reindex_content_changed, llvm::SmallVector{closed_id}); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.recheck_contexts); // The closed file's shard was built under the old command and looks @@ -448,7 +498,8 @@ TEST_CASE(CDBAddedOpenMarksDirty) { // The open file gained its first real entry: drop the guessed command // it was compiled with instead of queueing a background reindex. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{file}); - ASSERT_TRUE(dirty.enqueue_reindex.empty()); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); } TEST_CASE(CDBChangedDropsHostedContext) { @@ -477,7 +528,7 @@ TEST_CASE(CDBChangedDropsHostedContext) { llvm::sort(dropped); ASSERT_EQ(dirty.drop_context, dropped); ASSERT_TRUE(llvm::is_contained(dirty.mark_ast_dirty, open_header)); - ASSERT_TRUE(llvm::is_contained(dirty.enqueue_reindex, closed_header)); + ASSERT_TRUE(llvm::is_contained(dirty.reindex_content_changed, closed_header)); ASSERT_EQ(workspace.merged_indices.count(closed_header), 0u); } @@ -513,11 +564,15 @@ TEST_CASE(CDBChangedCascadesModule) { auto dirty = invalidator.apply(FileEvent::cdb_changed(std::move(delta))); // A module unit's flag change cascades through the compile graph - // exactly like a content change: importers' PCMs went stale. + // exactly like a content change: importers' PCMs went stale. The + // unit itself lands in both lists (its own entry changed AND the + // cascade dirtied its PCM); the indexer's absorbing upgrade + // resolves the overlap to ContentChanged. EXPECT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_user}); - llvm::SmallVector reindexed{mod, closed_user}; - llvm::sort(reindexed); - EXPECT_EQ(dirty.enqueue_reindex, reindexed); + EXPECT_EQ(dirty.reindex_content_changed, llvm::SmallVector{mod}); + llvm::SmallVector deps{mod, closed_user}; + llvm::sort(deps); + EXPECT_EQ(dirty.reindex_deps_only, deps); co_await workspace.compile_graph->shutdown(); }; @@ -544,7 +599,8 @@ TEST_CASE(DiskRemovedReindexesIncluders) { // Dependents now compile against a missing include: open ones // recompile, closed ones reindex. ASSERT_EQ(dirty.mark_ast_dirty, llvm::SmallVector{open_tu}); - ASSERT_EQ(dirty.enqueue_reindex, llvm::SmallVector{closed_tu}); + ASSERT_EQ(dirty.reindex_deps_only, llvm::SmallVector{closed_tu}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); ASSERT_TRUE(dirty.recheck_contexts); } @@ -605,7 +661,8 @@ TEST_CASE(BatchDiskEventsDeduplicate) { llvm::SmallVector expected{first, second}; llvm::sort(expected); - ASSERT_EQ(dirty.enqueue_reindex, expected); + ASSERT_EQ(dirty.reindex_content_changed, expected); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); } }; // TEST_SUITE(Invalidator) diff --git a/tests/unit/server/query_freshness_tests.cpp b/tests/unit/server/query_freshness_tests.cpp new file mode 100644 index 000000000..0c8ec1604 --- /dev/null +++ b/tests/unit/server/query_freshness_tests.cpp @@ -0,0 +1,166 @@ +#include +#include +#include +#include + +#include "test/test.h" +#include "test/tester.h" +#include "index/tu_index.h" +#include "server/compiler/context_resolver.h" +#include "server/compiler/indexer.h" +#include "server/service/query.h" +#include "server/state/session_store.h" +#include "server/worker/worker_pool.h" + +#include "llvm/Support/Path.h" + +namespace clice::testing { +namespace { + +TEST_SUITE(QueryFreshness, Tester) { + +kota::event_loop loop; +Workspace workspace; +SessionStore store; +WorkerPool pool{loop}; +ContextResolver resolver{workspace}; +Indexer indexer{loop, workspace, pool, resolver, store}; +IndexQuery index_query{workspace, store, indexer}; + +std::uint32_t main_id = 0; +std::uint32_t header_id = 0; + +/// Build a TUIndex from the added sources and merge it into the workspace +/// with real contents, so shards can map their rows to positions. +void merge_into_workspace() { + auto tu_index = index::TUIndex::build(*unit); + auto file_ids_map = workspace.project_index.merge(tu_index, workspace.path_pool); + + auto content_of = [&](llvm::StringRef path) -> llvm::StringRef { + auto it = sources.all_files.find(llvm::sys::path::filename(path)); + return it != sources.all_files.end() ? llvm::StringRef(it->second.content) + : llvm::StringRef(); + }; + + auto main_tu_path_id = static_cast(tu_index.graph.paths.size() - 1); + llvm::StringRef main_tu_path = tu_index.graph.paths[main_tu_path_id]; + main_id = file_ids_map[main_tu_path_id]; + + llvm::SmallVector deps; + for(auto& loc: tu_index.graph.locations) { + deps.push_back({tu_index.graph.paths[loc.path_id], loc.line, loc.include}); + } + workspace.merged_indices[main_id].merge(main_tu_path, + tu_index.built_at, + deps, + tu_index.main_file_index, + content_of(main_tu_path)); + + for(auto& [fid, file_idx]: tu_index.file_indices) { + auto tu_pid = tu_index.graph.path_id(fid); + auto global_pid = file_ids_map[tu_pid]; + auto include_id = tu_index.graph.include_location_id(fid); + workspace.merged_indices[global_pid].merge(main_tu_path, + include_id, + file_idx, + content_of(tu_index.graph.paths[tu_pid])); + if(llvm::sys::path::filename(tu_index.graph.paths[tu_pid]) == "header.h") { + header_id = global_pid; + } + } +} + +/// The symbol hash at an offset in a file's merged shard. +index::SymbolHash symbol_at(std::uint32_t path_id, std::uint32_t offset) { + index::SymbolHash result = 0; + workspace.merged_indices[path_id].lookup(offset, [&](const index::Occurrence& o) { + result = o.target; + return false; + }); + return result; +} + +/// Files contributing reference rows for a symbol, by basename. +std::vector reference_files(index::SymbolHash hash) { + std::vector files; + for(auto& ref: index_query.collect_references(hash, RelationKind::Reference)) { + files.push_back(llvm::sys::path::filename(ref.file).str()); + } + return files; +} + +TEST_CASE(PendingReasonUpgrade) { + auto file = workspace.path_pool.intern("/proj/upgrade.cpp"); + ASSERT_FALSE(indexer.pending_reason(file).has_value()); + + indexer.enqueue(file, ReindexReason::DepsOnly); + ASSERT_TRUE(indexer.pending_reason(file) == ReindexReason::DepsOnly); + ASSERT_EQ(indexer.pending_files(), 1u); + + // ContentChanged absorbs a queued DepsOnly without a second queue entry. + indexer.enqueue(file, ReindexReason::ContentChanged); + ASSERT_TRUE(indexer.pending_reason(file) == ReindexReason::ContentChanged); + ASSERT_EQ(indexer.pending_files(), 1u); + + // A later deps-only cascade never downgrades it. + indexer.enqueue(file, ReindexReason::DepsOnly); + ASSERT_TRUE(indexer.pending_reason(file) == ReindexReason::ContentChanged); +} + +TEST_CASE(PendingGateSplitsRows) { + workspace.config.project.enable_indexing = true; + + add_file("header.h", R"( + int helper() { return 1; } + )"); + add_main("main.cpp", R"( + #include "header.h" + int main() { + return $(use)helper(); + } + )"); + ASSERT_TRUE(compile()); + merge_into_workspace(); + + auto hash = symbol_at(main_id, point("use")); + ASSERT_NE(hash, 0UL); + + // Baseline: the main TU contributes its reference row, and the + // definition resolves into the header shard. + ASSERT_TRUE(std::ranges::contains(reference_files(hash), "main.cpp")); + ASSERT_TRUE(index_query.find_definition_location(hash).has_value()); + + // Pending for a dependency change only: the previous rows keep serving. + indexer.enqueue(main_id, ReindexReason::DepsOnly); + ASSERT_TRUE(std::ranges::contains(reference_files(hash), "main.cpp")); + + // Line-based resolution in the file works while its rows are current. + agentic::ReadSymbolParams by_line; + by_line.path = std::string(workspace.path_pool.resolve(main_id)); + by_line.line = 3; + ASSERT_FALSE(index_query.locate_symbols(by_line).empty()); + + // The file's own content changed: its contribution is skipped until the + // reindex lands; other files' rows are unaffected. + indexer.enqueue(main_id, ReindexReason::ContentChanged); + ASSERT_FALSE(std::ranges::contains(reference_files(hash), "main.cpp")); + ASSERT_TRUE(index_query.find_definition_location(hash).has_value()); + + // Cursor-style resolution against the stale rows is unresolvable: the + // line numbers describe text that no longer exists. + ASSERT_TRUE(index_query.locate_symbols(by_line).empty()); + + // A content-changed definition file drops out of definition lookups. + indexer.enqueue(header_id, ReindexReason::ContentChanged); + ASSERT_FALSE(index_query.find_definition_location(hash).has_value()); + + // With background indexing disabled nothing would ever catch up: + // last-known rows keep serving instead of leaving a permanent hole. + workspace.config.project.enable_indexing = false; + ASSERT_TRUE(std::ranges::contains(reference_files(hash), "main.cpp")); +} + +}; // TEST_SUITE(QueryFreshness) + +} // namespace +} // namespace clice::testing From 3c88b0d93d8484ff243a3ee52e6682477d29761b Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 00:17:04 +0800 Subject: [PATCH 02/11] fix(server): kick a follow-up round for files queued during join --- src/server/compiler/indexer.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 6b653f74a..bf6aaa19b 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -566,6 +566,14 @@ kota::task<> Indexer::run_background_indexing() { total, timer.ms()); co_await save(); + + // Files enqueued while the round was joining its workers saw their + // schedule() no-op against indexing_active; without this kick they + // would wait for the next external event — and a content-changed + // pending file's rows stay skipped for that whole wait. + if(index_queue_pos < index_queue.size()) { + schedule(); + } } } // namespace clice From aeb191c1ee7b17a053c8a724585f2a37fcf614dd Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 00:20:51 +0800 Subject: [PATCH 03/11] docs(server): state the failed-reindex gate tradeoff precisely --- src/server/compiler/indexer.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index bf6aaa19b..79c6774c4 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -456,10 +456,13 @@ kota::task<> Indexer::run_index_task(std::uint32_t server_path_id, std::size_t total, std::size_t& completed) { co_await index_one(server_path_id, index, total); - // The pending window ends with the index attempt, success or not (a - // failed attempt is re-detected by the hash gate on the next round). - // A re-enqueue during the flight bumped the ticket: that newer pending - // state must survive this clear. + // The pending window ends with the index attempt, success or not. On + // failure the last-known rows resume serving — deliberately: keeping + // the gate would hide a file that fails to index (broken compile, + // missing command) from every cross-file query with no recovery path, + // since only a future event re-enqueues it. Any such event re-judges + // staleness by content hash. A re-enqueue during the flight bumped + // the ticket: that newer pending state must survive this clear. if(auto it = reindex_reasons.find(server_path_id); it != reindex_reasons.end() && it->second.ticket == ticket) { reindex_reasons.erase(it); From 1ccd0b123ef79624078cb634910f6c95dd912582 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 01:11:57 +0800 Subject: [PATCH 04/11] fix(server): propagate failed cursor compiles, clear pending reindex on removal --- src/server/compiler/indexer.h | 10 ++ src/server/service/feature_router.cpp | 147 ++++++++++++++++++++---- src/server/service/feature_router.h | 8 -- src/server/state/invalidator.cpp | 7 ++ src/server/state/invalidator.h | 9 +- src/server/transport/master_server.cpp | 6 + tests/unit/server/invalidator_tests.cpp | 4 + 7 files changed, 160 insertions(+), 31 deletions(-) diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index a4fa0dae3..59ddd26c6 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -106,6 +106,16 @@ class Indexer { return it->second.reason; } + /// Forget a file's pending-reindex state (reason and queue membership): + /// used when the file is removed from disk — nothing is left to reindex, + /// and a lingering ContentChanged reason would suppress its deliberately + /// still-serving shard forever. A queue slot already consumed stays + /// consumed; one not yet consumed will run and skip gracefully. + void clear_pending(std::uint32_t server_path_id) { + reindex_reasons.erase(server_path_id); + pending_ids.erase(server_path_id); + } + /// Schedule background indexing (respects idle timeout and dedup). void schedule(); diff --git a/src/server/service/feature_router.cpp b/src/server/service/feature_router.cpp index b37646de9..2738159ad 100644 --- a/src/server/service/feature_router.cpp +++ b/src/server/service/feature_router.cpp @@ -37,12 +37,6 @@ static kota::ipc::Error item_not_resolved(llvm::StringRef kind) { std::format("Failed to resolve {} item", kind)}; } -kota::task<> FeatureRouter::settle_cursor_file(std::shared_ptr session) { - if(session) { - co_await compiler.ensure_compiled(std::move(session)); - } -} - const std::vector* FeatureRouter::find_preamble_links(const Session& session) { if(!session.pch_ref) @@ -107,13 +101,24 @@ kota::task FeatureRouter::definition(std::shared_ptr session, llvm::StringRef path, const protocol::Position& pos) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } // Preamble include lines first: they have no symbol occurrence in - // the index and are invisible to the worker's AST. A session still - // dirty after the settle (failed or superseded compile) skips this — - // the cached links may describe the pre-edit preamble — and retries - // below once the worker compile refreshed the PCH. + // the index and are invisible to the worker's AST. A session dirty even + // after the awaited compile means the world was re-dirtied mid-flight + // (dirty_epoch moved, so the settle did not clear the flag): the cached + // links may describe a pre-edit preamble — skip, and let the index and + // worker paths below answer. if(session && !session->ast_dirty) { if(auto directive = resolve_directive_definition(*session, pos); !directive.empty()) { co_return to_raw(directive); @@ -263,7 +268,17 @@ FeatureRouter::RawResult FeatureRouter::references(std::shared_ptr sess llvm::StringRef path, const protocol::Position& position, bool include_declaration) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto locations = index_query.query_relations(path, position, RelationKind::Reference, session.get()); @@ -286,7 +301,17 @@ FeatureRouter::RawResult FeatureRouter::references(std::shared_ptr sess FeatureRouter::RawResult FeatureRouter::declaration(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto locations = index_query.query_relations(path, position, RelationKind::Declaration, session.get()); @@ -301,7 +326,17 @@ FeatureRouter::RawResult FeatureRouter::declaration(std::shared_ptr ses FeatureRouter::RawResult FeatureRouter::type_definition(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } co_return to_raw(index_query.query_symbol_targets(path, position, @@ -312,7 +347,17 @@ FeatureRouter::RawResult FeatureRouter::type_definition(std::shared_ptr FeatureRouter::RawResult FeatureRouter::implementation(std::shared_ptr session, llvm::StringRef path, const protocol::Position& position) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } co_return to_raw(index_query.query_symbol_targets(path, position, @@ -324,7 +369,17 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_prepare(std::shared_ptrgeneration; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.lookup_symbol(uri, path, position, session.get()); if(!info) @@ -341,7 +396,17 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_incoming(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -355,7 +420,17 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_outgoing(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -369,7 +444,17 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_prepare(std::shared_ptrgeneration; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.lookup_symbol(uri, path, position, session.get()); if(!info) @@ -387,7 +472,17 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_supertypes(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -401,7 +496,17 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_subtypes(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { - co_await settle_cursor_file(session); + // Same posture as every AST-backed request: the session's file index + // is produced by the very compile awaited here, so once this settles + // the index describes the buffer. A failed or superseded compile + // (buffer changed while awaiting) yields null rather than a lookup + // against positions the buffer no longer has. + if(session) { + auto gen = session->generation; + if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { + co_return serde_raw{"null"}; + } + } auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); diff --git a/src/server/service/feature_router.h b/src/server/service/feature_router.h index 3561a3767..9b713565e 100644 --- a/src/server/service/feature_router.h +++ b/src/server/service/feature_router.h @@ -139,14 +139,6 @@ class FeatureRouter { RawResult workspace_symbol(llvm::StringRef query); private: - /// Cursor-resolution freshness (IndexQuery's contract, clause 1): an - /// index query that resolves a cursor in an open file waits for the - /// file's compile first, exactly like every AST-backed request does — - /// same await, no timeout. A failed or superseded compile is not an - /// error here: the query layer falls back per its contract. No-op for - /// closed documents (null session). - kota::task<> settle_cursor_file(std::shared_ptr session); - /// The preamble include links of a session's active PCH, or nullptr. const std::vector* find_preamble_links(const Session& session); diff --git a/src/server/state/invalidator.cpp b/src/server/state/invalidator.cpp index ea91abfc8..9fdb6d43a 100644 --- a/src/server/state/invalidator.cpp +++ b/src/server/state/invalidator.cpp @@ -213,6 +213,13 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dirty.reindex_deps_only.push_back(root); } } + // The file's shard deliberately keeps serving navigation + // (its content snapshot is the only remaining truth), so any + // pending reindex reason recorded before the removal — e.g. + // a DiskChanged observed moments earlier — must be dropped: + // there is nothing to reindex any more, and a lingering + // ContentChanged would suppress the shard forever. + dirty.clear_reindex.push_back(path_id); // A removed module unit takes its PCM with it: importers' // build products went stale, and it stops providing its // module name. diff --git a/src/server/state/invalidator.h b/src/server/state/invalidator.h index 95bb387b0..a53c0dd71 100644 --- a/src/server/state/invalidator.h +++ b/src/server/state/invalidator.h @@ -146,6 +146,11 @@ struct DirtySet { /// queries keep serving the previous rows. A file in both lists is /// ContentChanged (the indexer's reason upgrade is absorbing). llvm::SmallVector reindex_deps_only; + + /// Files whose pending-reindex state must be discarded: a removed file + /// has nothing left to reindex, and a stale ContentChanged reason would + /// otherwise suppress its (deliberately still-serving) shard forever. + llvm::SmallVector clear_reindex; /// Headers whose resolved context borrows a compile command that no /// longer exists in that form (the host's CDB entry changed): drop the /// context so the next use re-resolves. Content validation cannot see @@ -168,8 +173,8 @@ struct DirtySet { return mark_ast_dirty.empty() && mark_lost.empty() && reset_trial.empty() && reset_header_mode.empty() && force_revalidate.empty() && reindex_content_changed.empty() && reindex_deps_only.empty() && - drop_context.empty() && !recheck_contexts && !save_cache && !reschedule_indexing && - !ensure_compile_graph; + clear_reindex.empty() && drop_context.empty() && !recheck_contexts && !save_cache && + !reschedule_indexing && !ensure_compile_graph; } }; diff --git a/src/server/transport/master_server.cpp b/src/server/transport/master_server.cpp index 3182ee051..3590a3536 100644 --- a/src/server/transport/master_server.cpp +++ b/src/server/transport/master_server.cpp @@ -295,6 +295,12 @@ void MasterServer::dispatch(llvm::ArrayRef events) { for(auto path_id: dirty.reindex_deps_only) { indexer.enqueue(path_id, ReindexReason::DepsOnly); } + // After the enqueues: when one batch both dirties and removes a file + // (DiskChanged then DiskRemoved), the removal is the later fact and + // its clear must win. + for(auto path_id: dirty.clear_reindex) { + indexer.clear_pending(path_id); + } if(dirty.ensure_compile_graph && !workspace.compile_graph) { compiler.init_compile_graph(); diff --git a/tests/unit/server/invalidator_tests.cpp b/tests/unit/server/invalidator_tests.cpp index 2d9bff268..bcd250474 100644 --- a/tests/unit/server/invalidator_tests.cpp +++ b/tests/unit/server/invalidator_tests.cpp @@ -413,6 +413,10 @@ TEST_CASE(DiskRemovedScrubsSourceRole) { ASSERT_TRUE(dirty.reindex_deps_only.empty()); ASSERT_TRUE(dirty.mark_ast_dirty.empty()); ASSERT_EQ(workspace.context_epoch, epoch + 1); + // The removal clears any pending-reindex state recorded earlier (e.g. a + // DiskChanged observed just before deletion): the shard keeps serving + // and nothing is left to reindex. + ASSERT_EQ(dirty.clear_reindex, llvm::SmallVector{removed_tu}); } TEST_CASE(CDBAddedScansAndEnqueues) { From 69ca224bce4281f4e892ba22580757b36f9f5274 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 01:50:18 +0800 Subject: [PATCH 05/11] fix(server): skip queue slots cleared by file removal --- src/server/compiler/indexer.cpp | 18 ++++++++++-------- src/server/compiler/indexer.h | 3 ++- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 79c6774c4..9c26f1066 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -524,15 +524,17 @@ kota::task<> Indexer::run_background_indexing() { continue; } - ++dispatched; - // Invariant: a queued file always has a pending entry (enqueue - // writes it before the queue push, and nothing erases it while it - // sits in the un-consumed tail). Ticket 0 is never issued, so if - // the invariant ever broke in a release build the task would run - // normally and simply leave no entry to clear. + // A queued slot with no pending entry was cleared mid-batch: the + // file was removed from disk after being enqueued (clear_pending), + // so there is nothing to index — skip the slot. Every other slot + // has an entry, because enqueue writes it before the queue push. auto pending_it = reindex_reasons.find(server_path_id); - assert(pending_it != reindex_reasons.end() && "queued file must have a pending reason"); - auto ticket = pending_it != reindex_reasons.end() ? pending_it->second.ticket : 0; + if(pending_it == reindex_reasons.end()) { + continue; + } + + ++dispatched; + auto ticket = pending_it->second.ticket; // A member coroutine, not an immediately-invoked capturing lambda: // a lambda's captures live in the lambda object, which dies at the // end of this statement — anything read after the first suspension diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index 59ddd26c6..4f78b9452 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -110,7 +110,8 @@ class Indexer { /// used when the file is removed from disk — nothing is left to reindex, /// and a lingering ContentChanged reason would suppress its deliberately /// still-serving shard forever. A queue slot already consumed stays - /// consumed; one not yet consumed will run and skip gracefully. + /// consumed; one not yet consumed is skipped at dispatch time (the + /// consume loop treats a missing pending entry as a cleared slot). void clear_pending(std::uint32_t server_path_id) { reindex_reasons.erase(server_path_id); pending_ids.erase(server_path_id); From be847fc215607722d3f477648223bc448b3d5b34 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 02:25:09 +0800 Subject: [PATCH 06/11] fix(server): keep reindex effects per-file order consistent in batches --- src/server/state/invalidator.cpp | 22 ++++++++--------- src/server/state/invalidator.h | 33 +++++++++++++++++++++++++ src/server/transport/master_server.cpp | 6 ++--- tests/unit/server/invalidator_tests.cpp | 31 +++++++++++++++++++++++ 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/src/server/state/invalidator.cpp b/src/server/state/invalidator.cpp index 9fdb6d43a..5758749bf 100644 --- a/src/server/state/invalidator.cpp +++ b/src/server/state/invalidator.cpp @@ -40,7 +40,7 @@ void Invalidator::cascade_compile_graph(std::uint32_t path_id, DirtySet& dirty) if(store.find(dirty_id)) { dirty.mark_ast_dirty.push_back(dirty_id); } else { - dirty.reindex_deps_only.push_back(dirty_id); + dirty.add_reindex_deps_only(dirty_id); } } } @@ -66,7 +66,7 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d if(store.find(dirty_id)) { dirty.mark_ast_dirty.push_back(dirty_id); } else { - dirty.reindex_deps_only.push_back(dirty_id); + dirty.add_reindex_deps_only(dirty_id); } } @@ -82,7 +82,7 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d if(store.find(root)) { dirty.mark_ast_dirty.push_back(root); } else { - dirty.reindex_deps_only.push_back(root); + dirty.add_reindex_deps_only(root); } } }; @@ -104,7 +104,7 @@ void Invalidator::cascade_disk_content_change(std::uint32_t path_id, DirtySet& d // can refresh them. The header's own content did not change, so // its rows keep serving meanwhile. if(!store.find(header_id)) { - dirty.reindex_deps_only.push_back(header_id); + dirty.add_reindex_deps_only(header_id); } } @@ -177,9 +177,9 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { shard_current = disk && *disk == shard_it->second.content(); } if(shard_current) { - dirty.reindex_deps_only.push_back(event.path_id); + dirty.add_reindex_deps_only(event.path_id); } else { - dirty.reindex_content_changed.push_back(event.path_id); + dirty.add_reindex_content_changed(event.path_id); } dirty.reschedule_indexing = true; break; @@ -197,7 +197,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // Closed file: disk is the truth. Run the same cascade a // save does, and refresh the file's own now-stale shard. cascade_disk_content_change(path_id, dirty); - dirty.reindex_content_changed.push_back(path_id); + dirty.add_reindex_content_changed(path_id); break; } case FileEvent::Kind::DiskRemoved: { @@ -210,7 +210,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { if(store.find(root)) { dirty.mark_ast_dirty.push_back(root); } else { - dirty.reindex_deps_only.push_back(root); + dirty.add_reindex_deps_only(root); } } // The file's shard deliberately keeps serving navigation @@ -219,7 +219,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // a DiskChanged observed moments earlier — must be dropped: // there is nothing to reindex any more, and a lingering // ContentChanged would suppress the shard forever. - dirty.clear_reindex.push_back(path_id); + dirty.add_clear_reindex(path_id); // A removed module unit takes its PCM with it: importers' // build products went stale, and it stops providing its // module name. @@ -302,7 +302,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // eviction; closing that window needs an index // generation guard in the indexer. workspace.merged_indices.erase(path_id); - dirty.reindex_content_changed.push_back(path_id); + dirty.add_reindex_content_changed(path_id); } // A module unit's command change invalidates importers' @@ -326,7 +326,7 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dirty.mark_ast_dirty.push_back(header_id); } else { workspace.merged_indices.erase(header_id); - dirty.reindex_content_changed.push_back(header_id); + dirty.add_reindex_content_changed(header_id); } } }; diff --git a/src/server/state/invalidator.h b/src/server/state/invalidator.h index a53c0dd71..cafc3e48b 100644 --- a/src/server/state/invalidator.h +++ b/src/server/state/invalidator.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -10,6 +11,7 @@ #include "server/state/workspace.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" namespace clice { @@ -151,6 +153,37 @@ struct DirtySet { /// has nothing left to reindex, and a stale ContentChanged reason would /// otherwise suppress its (deliberately still-serving) shard forever. llvm::SmallVector clear_reindex; + + /// The three reindex effect lists are kept disjoint per file, in event + /// order: a batch can hold delete-then-recreate (atomic saves) as well + /// as change-then-delete, so neither "clear wins" nor "enqueue wins" is + /// right as a fixed rule — the later event for a given file wins. All + /// emission goes through these adders to keep that true by construction, + /// letting the executor apply the lists in any order. + void add_reindex_content_changed(std::uint32_t path_id) { + erase_id(clear_reindex, path_id); + reindex_content_changed.push_back(path_id); + } + + void add_reindex_deps_only(std::uint32_t path_id) { + erase_id(clear_reindex, path_id); + reindex_deps_only.push_back(path_id); + } + + void add_clear_reindex(std::uint32_t path_id) { + erase_id(reindex_content_changed, path_id); + erase_id(reindex_deps_only, path_id); + if(llvm::find(clear_reindex, path_id) == clear_reindex.end()) { + clear_reindex.push_back(path_id); + } + } + +private: + static void erase_id(llvm::SmallVector& ids, std::uint32_t path_id) { + ids.erase(std::remove(ids.begin(), ids.end(), path_id), ids.end()); + } + +public: /// Headers whose resolved context borrows a compile command that no /// longer exists in that form (the host's CDB entry changed): drop the /// context so the next use re-resolves. Content validation cannot see diff --git a/src/server/transport/master_server.cpp b/src/server/transport/master_server.cpp index 3590a3536..390ee01a7 100644 --- a/src/server/transport/master_server.cpp +++ b/src/server/transport/master_server.cpp @@ -295,9 +295,9 @@ void MasterServer::dispatch(llvm::ArrayRef events) { for(auto path_id: dirty.reindex_deps_only) { indexer.enqueue(path_id, ReindexReason::DepsOnly); } - // After the enqueues: when one batch both dirties and removes a file - // (DiskChanged then DiskRemoved), the removal is the later fact and - // its clear must win. + // The engine keeps the reindex lists disjoint per file in event order + // (see DirtySet's adders), so the clears may run in any order relative + // to the enqueues above. for(auto path_id: dirty.clear_reindex) { indexer.clear_pending(path_id); } diff --git a/tests/unit/server/invalidator_tests.cpp b/tests/unit/server/invalidator_tests.cpp index bcd250474..594067224 100644 --- a/tests/unit/server/invalidator_tests.cpp +++ b/tests/unit/server/invalidator_tests.cpp @@ -419,6 +419,37 @@ TEST_CASE(DiskRemovedScrubsSourceRole) { ASSERT_EQ(dirty.clear_reindex, llvm::SmallVector{removed_tu}); } +TEST_CASE(RemoveRecreateBatchOrder) { + Workspace workspace; + SessionStore store; + auto file = workspace.path_pool.intern("/proj/a.cpp"); + workspace.dep_graph.set_includes(file, 0, {}); + workspace.dep_graph.build_reverse_map(); + ContextResolver resolver(workspace); + Invalidator invalidator(workspace, store, resolver); + + // Change then delete: the removal is the later fact, the clear wins. + { + FileEvent events[] = {FileEvent::disk_changed(file), FileEvent::disk_removed(file)}; + auto dirty = invalidator.apply(events); + ASSERT_TRUE(llvm::find(dirty.reindex_content_changed, file) == + dirty.reindex_content_changed.end()); + ASSERT_EQ(dirty.clear_reindex, llvm::SmallVector{file}); + } + + // Delete then recreate (an editor's atomic save): the later change must + // survive — the recreated file needs its reindex. + { + workspace.dep_graph.set_includes(file, 0, {}); + workspace.dep_graph.build_reverse_map(); + FileEvent events[] = {FileEvent::disk_removed(file), FileEvent::disk_changed(file)}; + auto dirty = invalidator.apply(events); + ASSERT_TRUE(dirty.clear_reindex.empty()); + ASSERT_TRUE(llvm::find(dirty.reindex_content_changed, file) != + dirty.reindex_content_changed.end()); + } +} + TEST_CASE(CDBAddedScansAndEnqueues) { TempDir tmp; tmp.touch("inc/header.h", R"(int x = 1;)"); From 701e5d203c1d937a9df05b0b94c4b81f69123b27 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 03:00:28 +0800 Subject: [PATCH 07/11] fix(server): do not gate hierarchy expansion on current compiles --- src/server/service/feature_router.cpp | 64 +++++++++------------------ 1 file changed, 20 insertions(+), 44 deletions(-) diff --git a/src/server/service/feature_router.cpp b/src/server/service/feature_router.cpp index 2738159ad..6cf521b37 100644 --- a/src/server/service/feature_router.cpp +++ b/src/server/service/feature_router.cpp @@ -396,17 +396,11 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_incoming(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { - // Same posture as every AST-backed request: the session's file index - // is produced by the very compile awaited here, so once this settles - // the index describes the buffer. A failed or superseded compile - // (buffer changed while awaiting) yields null rather than a lookup - // against positions the buffer no longer has. - if(session) { - auto gen = session->generation; - if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { - co_return serde_raw{"null"}; - } - } + // No compile gate here: expansion resolves the previously prepared + // item through its stored symbol handle (item.data, with the recorded + // range as fallback), not the current cursor — the buffer's present + // compile state is irrelevant, and gating would blank expansions the + // moment the user edits the file again. auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -420,17 +414,11 @@ FeatureRouter::RawResult FeatureRouter::call_hierarchy_outgoing(std::shared_ptr session, llvm::StringRef path, const protocol::CallHierarchyItem& item) { - // Same posture as every AST-backed request: the session's file index - // is produced by the very compile awaited here, so once this settles - // the index describes the buffer. A failed or superseded compile - // (buffer changed while awaiting) yields null rather than a lookup - // against positions the buffer no longer has. - if(session) { - auto gen = session->generation; - if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { - co_return serde_raw{"null"}; - } - } + // No compile gate here: expansion resolves the previously prepared + // item through its stored symbol handle (item.data, with the recorded + // range as fallback), not the current cursor — the buffer's present + // compile state is irrelevant, and gating would blank expansions the + // moment the user edits the file again. auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -472,17 +460,11 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_supertypes(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { - // Same posture as every AST-backed request: the session's file index - // is produced by the very compile awaited here, so once this settles - // the index describes the buffer. A failed or superseded compile - // (buffer changed while awaiting) yields null rather than a lookup - // against positions the buffer no longer has. - if(session) { - auto gen = session->generation; - if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { - co_return serde_raw{"null"}; - } - } + // No compile gate here: expansion resolves the previously prepared + // item through its stored symbol handle (item.data, with the recorded + // range as fallback), not the current cursor — the buffer's present + // compile state is irrelevant, and gating would blank expansions the + // moment the user edits the file again. auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); @@ -496,17 +478,11 @@ FeatureRouter::RawResult FeatureRouter::type_hierarchy_subtypes(std::shared_ptr session, llvm::StringRef path, const protocol::TypeHierarchyItem& item) { - // Same posture as every AST-backed request: the session's file index - // is produced by the very compile awaited here, so once this settles - // the index describes the buffer. A failed or superseded compile - // (buffer changed while awaiting) yields null rather than a lookup - // against positions the buffer no longer has. - if(session) { - auto gen = session->generation; - if(!co_await compiler.ensure_compiled(session) || session->generation != gen) { - co_return serde_raw{"null"}; - } - } + // No compile gate here: expansion resolves the previously prepared + // item through its stored symbol handle (item.data, with the recorded + // range as fallback), not the current cursor — the buffer's present + // compile state is irrelevant, and gating would blank expansions the + // moment the user edits the file again. auto info = index_query.resolve_hierarchy_item(item.uri, path, item.range, item.data, session.get()); From 6ac86c12cccaf15bcbb21ba2061f492de06618fa Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 03:55:20 +0800 Subject: [PATCH 08/11] fix(server): removal clears survive cascades, requeues carry their own reason --- src/server/compiler/indexer.cpp | 20 +++++++++++----- src/server/state/invalidator.cpp | 32 +++++++++++++++++-------- tests/unit/server/invalidator_tests.cpp | 25 ++++++++++++++++++- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 9c26f1066..103f8616b 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -336,21 +336,29 @@ bool Indexer::need_update(llvm::StringRef file_path) { } void Indexer::enqueue(std::uint32_t server_path_id, ReindexReason reason) { - // Record (or refresh) why the file is pending. ContentChanged is - // absorbing: a deps-only cascade cannot downgrade a file whose own - // content already changed. The fresh ticket invalidates the clear of + // A fresh slot means any prior slot was already consumed (or none + // existed); a queued-and-unconsumed slot makes this call a duplicate. + bool fresh_slot = pending_ids.insert(server_path_id).second; + + // Record (or refresh) why the file is pending. Within one queued slot + // ContentChanged is absorbing: a deps-only cascade cannot downgrade a + // file whose own content already changed. Across slots it is not: a + // deps-only requeue after the previous slot was consumed is new debt of + // its own kind — the in-flight (or finished) pass already covers the + // earlier content change, and keeping ContentChanged would suppress the + // file's rows past that pass. The fresh ticket invalidates the clear of // any index task already in flight for this file. auto [it, inserted] = reindex_reasons.try_emplace(server_path_id, reason, ++reindex_ticket); if(!inserted) { if(reason == ReindexReason::ContentChanged) { it->second.reason = ReindexReason::ContentChanged; + } else if(fresh_slot) { + it->second.reason = ReindexReason::DepsOnly; } it->second.ticket = reindex_ticket; } - // Already queued and not yet consumed — a second entry would only be - // skipped by need_update later; drop it here. - if(!pending_ids.insert(server_path_id).second) + if(!fresh_slot) return; index_queue.push_back(server_path_id); } diff --git a/src/server/state/invalidator.cpp b/src/server/state/invalidator.cpp index 5758749bf..5eb367065 100644 --- a/src/server/state/invalidator.cpp +++ b/src/server/state/invalidator.cpp @@ -170,12 +170,21 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { // the queue's latency, while a close after saved edits must // not serve rows for text that no longer exists. One disk // read settles it; an unreadable file counts as changed. - auto shard_it = workspace.merged_indices.find(event.path_id); - bool shard_current = false; - if(shard_it != workspace.merged_indices.end()) { - auto disk = read_file(workspace.path_pool.resolve(event.path_id)); - shard_current = disk && *disk == shard_it->second.content(); + auto disk = read_file(workspace.path_pool.resolve(event.path_id)); + if(!disk) { + // Deleted while it was open: the tracker skips open + // files, so this close is the first observation of the + // missing file. Keep any shard serving (same deliberate + // choice as DiskRemoved) instead of recording a + // ContentChanged that would suppress it forever; the + // tracker's next sweep observes the removal and delivers + // the full DiskRemoved cascade. + dirty.add_clear_reindex(event.path_id); + break; } + auto shard_it = workspace.merged_indices.find(event.path_id); + bool shard_current = shard_it != workspace.merged_indices.end() && + *disk == shard_it->second.content(); if(shard_current) { dirty.add_reindex_deps_only(event.path_id); } else { @@ -213,17 +222,20 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dirty.add_reindex_deps_only(root); } } + // A removed module unit takes its PCM with it: importers' + // build products went stale, and it stops providing its + // module name. + cascade_compile_graph(path_id, dirty); // The file's shard deliberately keeps serving navigation // (its content snapshot is the only remaining truth), so any // pending reindex reason recorded before the removal — e.g. // a DiskChanged observed moments earlier — must be dropped: // there is nothing to reindex any more, and a lingering - // ContentChanged would suppress the shard forever. + // ContentChanged would suppress the shard forever. Emitted + // after the compile-graph cascade, which lists the removed + // module itself among its dirtied units: the removal is this + // event's final word for the file itself. dirty.add_clear_reindex(path_id); - // A removed module unit takes its PCM with it: importers' - // build products went stale, and it stops providing its - // module name. - cascade_compile_graph(path_id, dirty); workspace.path_to_module.erase(path_id); // Scrub the includer role: the file's outgoing edges vanished // with it, so it stops being a host-source candidate. diff --git a/tests/unit/server/invalidator_tests.cpp b/tests/unit/server/invalidator_tests.cpp index 594067224..a0cdf6170 100644 --- a/tests/unit/server/invalidator_tests.cpp +++ b/tests/unit/server/invalidator_tests.cpp @@ -209,7 +209,10 @@ TEST_CASE(CloseWithoutShardReindexes) { auto closed = workspace.path_pool.intern("/proj/a.cpp"); ContextResolver resolver(workspace); - Invalidator invalidator(workspace, store, resolver); + // The file exists on disk (injected read), it just was never indexed. + Invalidator invalidator(workspace, store, resolver, [](llvm::StringRef) { + return std::optional("int x;"); + }); auto dirty = invalidator.apply(FileEvent::buffer_closed(closed)); // No shard to compare against: nothing serves this file's rows anyway. @@ -450,6 +453,26 @@ TEST_CASE(RemoveRecreateBatchOrder) { } } +TEST_CASE(CloseOfDeletedFile) { + Workspace workspace; + SessionStore store; + auto file = workspace.path_pool.intern("/proj/gone.cpp"); + ContextResolver resolver(workspace); + // Disk read fails: the file vanished while it was open. + Invalidator invalidator(workspace, store, resolver, [](llvm::StringRef) { + return std::optional{}; + }); + + auto dirty = invalidator.apply(FileEvent::buffer_closed(file)); + + // The close is the first observation of the removal (the tracker skips + // open files): keep any shard serving, do not record ContentChanged, + // do not enqueue a nonexistent file. + ASSERT_EQ(dirty.clear_reindex, llvm::SmallVector{file}); + ASSERT_TRUE(dirty.reindex_content_changed.empty()); + ASSERT_TRUE(dirty.reindex_deps_only.empty()); +} + TEST_CASE(CDBAddedScansAndEnqueues) { TempDir tmp; tmp.touch("inc/header.h", R"(int x = 1;)"); From 31334a8356c261c064ba80d7b6a637fa020fe25a Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 04:41:21 +0800 Subject: [PATCH 09/11] fix(server): gate index merges on the launch ticket --- src/server/compiler/indexer.cpp | 14 +++++++++++++- src/server/compiler/indexer.h | 5 ++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 103f8616b..846f54d38 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -402,6 +402,7 @@ void Indexer::schedule() { } kota::task<> Indexer::index_one(std::uint32_t server_path_id, + std::uint64_t ticket, std::size_t index, std::size_t total) { auto file_path = std::string(workspace.path_pool.resolve(server_path_id)); @@ -435,6 +436,17 @@ kota::task<> Indexer::index_one(std::uint32_t server_path_id, auto result = co_await pool.send_stateless(params); if(result.has_value() && result.value().success && !result.value().tu_index_data.empty()) { auto index_ms = timer.ms(); + // Merge guard, same token the completion clear uses: a newer + // invalidation during this build bumped the ticket (or a removal + // cleared the entry), so this result describes a superseded world — + // e.g. a compile-command change whose erase+re-enqueue must not be + // undone by an in-flight merge of the old-command rows. Drop the + // merge; the follow-up queue slot redoes the work. + if(auto it = reindex_reasons.find(server_path_id); + it == reindex_reasons.end() || it->second.ticket != ticket) { + LOG_INFO("Discarding superseded index result for {}", file_path); + co_return; + } ScopedTimer merge_timer; merge(result.value().tu_index_data.data(), result.value().tu_index_data.size()); LOG_PERF("index", @@ -463,7 +475,7 @@ kota::task<> Indexer::run_index_task(std::uint32_t server_path_id, std::size_t index, std::size_t total, std::size_t& completed) { - co_await index_one(server_path_id, index, total); + co_await index_one(server_path_id, ticket, index, total); // The pending window ends with the index attempt, success or not. On // failure the last-known rows resume serving — deliberately: keeping // the gate would hide a file that fails to index (broken compile, diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index 4f78b9452..dc59105f3 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -216,7 +216,10 @@ class Indexer { Progress progress_data; kota::task<> run_background_indexing(); - kota::task<> index_one(std::uint32_t server_path_id, std::size_t index, std::size_t total); + kota::task<> index_one(std::uint32_t server_path_id, + std::uint64_t ticket, + std::size_t index, + std::size_t total); /// One dispatched unit of a background round: index the file, then end /// its pending window (ticket-guarded) and report progress. `completed` From 74f8d9556e8f627373aa6bdf1b88ae63509e2b83 Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 05:21:50 +0800 Subject: [PATCH 10/11] fix(server): content-aware merge guard, authoritative reindex reasons, honest index gaps --- src/server/compiler/compiler.cpp | 8 ++++++++ src/server/compiler/indexer.cpp | 34 +++++++++++++++++++++++--------- src/server/compiler/indexer.h | 6 ++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/server/compiler/compiler.cpp b/src/server/compiler/compiler.cpp index 365e0b4b3..c9b43923c 100644 --- a/src/server/compiler/compiler.cpp +++ b/src/server/compiler/compiler.cpp @@ -801,6 +801,14 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { auto tu_index = index::TUIndex::from(result.value().tu_index_data.data()); session->file_index = std::move(tu_index.main_file_index); session->symbols = std::move(tu_index.symbols); + } else { + // The AST and the file index settle together — that pairing is + // what lets navigation trust the index after ensure_compiled. A + // compile that produced no index data (fatal error, no AST) must + // therefore drop the previous buffer's index rather than leave + // it posing as current: an honest gap over yesterday's offsets. + session->file_index.reset(); + session->symbols.reset(); } auto version = session->version; diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index 846f54d38..e11fe3e5c 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -348,10 +348,16 @@ void Indexer::enqueue(std::uint32_t server_path_id, ReindexReason reason) { // earlier content change, and keeping ContentChanged would suppress the // file's rows past that pass. The fresh ticket invalidates the clear of // any index task already in flight for this file. - auto [it, inserted] = reindex_reasons.try_emplace(server_path_id, reason, ++reindex_ticket); + ++reindex_ticket; + auto [it, inserted] = + reindex_reasons.try_emplace(server_path_id, + reason, + reindex_ticket, + reason == ReindexReason::ContentChanged ? reindex_ticket : 0); if(!inserted) { if(reason == ReindexReason::ContentChanged) { it->second.reason = ReindexReason::ContentChanged; + it->second.content_ticket = reindex_ticket; } else if(fresh_slot) { it->second.reason = ReindexReason::DepsOnly; } @@ -410,8 +416,16 @@ kota::task<> Indexer::index_one(std::uint32_t server_path_id, if(sessions.find(server_path_id) != nullptr) co_return; - if(!need_update(file_path)) + // The engine's own observation is authoritative for content changes: + // it saw the event. The dep-hash check below cannot be trusted to see + // a file's own edit (it validates the recorded dependencies), so only + // deps-only slots — where it exists to deduplicate cascade storms — + // may take the shortcut. + if(auto it = reindex_reasons.find(server_path_id); + (it == reindex_reasons.end() || it->second.reason != ReindexReason::ContentChanged) && + !need_update(file_path)) { co_return; + } // For module interface units, compile their PCM (and transitive deps) // first so the stateless worker has the artifacts it needs. @@ -436,14 +450,16 @@ kota::task<> Indexer::index_one(std::uint32_t server_path_id, auto result = co_await pool.send_stateless(params); if(result.has_value() && result.value().success && !result.value().tu_index_data.empty()) { auto index_ms = timer.ms(); - // Merge guard, same token the completion clear uses: a newer - // invalidation during this build bumped the ticket (or a removal - // cleared the entry), so this result describes a superseded world — - // e.g. a compile-command change whose erase+re-enqueue must not be - // undone by an in-flight merge of the old-command rows. Drop the - // merge; the follow-up queue slot redoes the work. + // Merge guard: a newer content-level invalidation during this build + // (or a removal clearing the entry) means this result describes text + // that no longer exists — e.g. a compile-command change whose + // erase+re-enqueue must not be undone by an in-flight merge of the + // old-command rows. Drop the merge; the follow-up slot redoes it. + // A deps-only requeue is deliberately NOT superseding: the in-flight + // rows are positionally right, and suppressing them would trade a + // tolerated semantic drift for a coverage hole. if(auto it = reindex_reasons.find(server_path_id); - it == reindex_reasons.end() || it->second.ticket != ticket) { + it == reindex_reasons.end() || it->second.content_ticket > ticket) { LOG_INFO("Discarding superseded index result for {}", file_path); co_return; } diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index dc59105f3..53a055c73 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -200,6 +200,12 @@ class Indexer { struct PendingReindex { ReindexReason reason; std::uint64_t ticket; + /// Ticket of the newest ContentChanged enqueue. The merge guard + /// compares against this, not `ticket`: a deps-only requeue during a + /// flight bumps `ticket` (to survive the completion clear) but must + /// not discard an in-flight content pass — its rows are positionally + /// right and the follow-up slot redoes the semantic drift anyway. + std::uint64_t content_ticket; }; llvm::DenseMap reindex_reasons; From 975b62d7328c266a632fded23aaee7341c83287a Mon Sep 17 00:00:00 2001 From: ykiko Date: Wed, 8 Jul 2026 06:00:05 +0800 Subject: [PATCH 11/11] refactor(server): single decision point for index skips, no shard fallback for open files --- src/server/compiler/indexer.cpp | 16 +++++++--------- src/server/compiler/indexer.h | 32 ++++++++++++++++++++++++++------ src/server/service/query.cpp | 13 ++++++++++--- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/server/compiler/indexer.cpp b/src/server/compiler/indexer.cpp index e11fe3e5c..f2488dcba 100644 --- a/src/server/compiler/indexer.cpp +++ b/src/server/compiler/indexer.cpp @@ -550,15 +550,13 @@ kota::task<> Indexer::run_background_indexing() { auto server_path_id = index_queue[index_queue_pos++]; pending_ids.erase(server_path_id); - auto file_path = std::string(workspace.path_pool.resolve(server_path_id)); - if(sessions.find(server_path_id) != nullptr || !need_update(file_path)) { - // Not pending anymore: an open session's buffer index is - // authoritative, and a hash-fresh shard already describes the - // current content. - reindex_reasons.erase(server_path_id); - ++completed; - continue; - } + // No open-session or hash-freshness shortcut here: index_one is the + // single decision point for skipping (it knows the pending reason; + // a hash check alone cannot see a file's own edit), and the + // completion clear in run_index_task retires the pending state with + // the ticket honored. A second, reason-blind copy of these checks + // here is exactly what once erased ContentChanged state early and + // let a stale shard keep serving. // A queued slot with no pending entry was cleared mid-batch: the // file was removed from disk after being enqueued (clear_pending), diff --git a/src/server/compiler/indexer.h b/src/server/compiler/indexer.h index 53a055c73..f16a3425e 100644 --- a/src/server/compiler/indexer.h +++ b/src/server/compiler/indexer.h @@ -191,12 +191,32 @@ class Indexer { llvm::DenseSet pending_ids; std::size_t index_queue_pos = 0; - /// The reason each file awaits re-indexing, kept from enqueue until the - /// file is skipped (open session, fresh shard) or its index task ends — - /// unlike pending_ids, which only covers the un-consumed queue tail. - /// The ticket guards the clear: a file re-enqueued while its previous - /// index task is still in flight bumps the ticket, so the older task's - /// completion must not erase the newer pending state. + /// The pending-reindex state machine, per file. This block is the + /// authoritative description; every rule below exists because its + /// absence was a concrete bug. + /// + /// States: absent → queued (slot in index_queue + entry here) → + /// in-flight (slot consumed, entry alive) → absent again. + /// + /// Invariants: + /// 1. index_one is the ONLY place that decides to skip work (open + /// session, or hash-fresh shard for deps-only slots). Duplicating + /// those checks elsewhere reintroduces reason-blind skips. + /// 2. need_update() may shortcut deps-only slots ONLY: the engine + /// observed content changes itself, and the dep-hash check cannot + /// see a file's own edit. + /// 3. The merge lands iff the entry is alive and no ContentChanged + /// enqueue happened after launch (content_ticket <= launch ticket). + /// Deps-only requeues do not discard an in-flight pass. + /// 4. Completion erases the entry iff ticket == launch ticket: a + /// requeue during the flight must survive the older task's clear. + /// 5. Within a queued slot, ContentChanged absorbs; a fresh slot after + /// consumption carries its own reason (the consumed pass owns the + /// earlier debt). + /// 6. clear_pending (file removal) drops entry and queue membership; + /// the orphaned slot is skipped at dispatch. + /// 7. Queries suppress a file's contributions iff its entry's reason + /// is ContentChanged (see pending_reason). struct PendingReindex { ReindexReason reason; std::uint64_t ticket; diff --git a/src/server/service/query.cpp b/src/server/service/query.cpp index 3fe35ea11..29a18fe44 100644 --- a/src/server/service/query.cpp +++ b/src/server/service/query.cpp @@ -89,9 +89,16 @@ IndexQuery::CursorHit IndexQuery::resolve_cursor(llvm::StringRef path, const protocol::Position& position, Session* session) { // Freshness contract, clause 1: callers awaited the session's compile, - // so a session that is still dirty here had its compile fail or be - // superseded — fall through to the merged shard, which resolves - // against its own stored snapshot. + // and the file index settles together with it. An open document + // resolves against that index or not at all — its shard describes a + // disk snapshot, and mapping live buffer offsets onto it is exactly + // the mixed-view lookup the contract exists to prevent (the + // cross-file visit already skips shards of open files for the same + // reason). A dirty-after-await session (failed or superseded compile) + // or an index-less one therefore reports no hit. + if(session && (!session->file_index || session->ast_dirty)) { + return {}; + } if(session && session->file_index && !session->ast_dirty) { auto map = session->line_map(); auto offset = map.to_offset(position);