diff --git a/src/clice.cc b/src/clice.cc index 13b14ce07..fc789472e 100644 --- a/src/clice.cc +++ b/src/clice.cc @@ -42,6 +42,12 @@ struct WorkerOptions { required = false) memory_limit; + DecoKV(style = KVStyle::JoinedOrSeparate, + names = {"--max-documents", "--max-documents="}, + help = "Max compiled documents kept before LRU eviction (stateful worker only)", + required = false) + max_documents; + DecoKV(style = KVStyle::JoinedOrSeparate, names = {"--worker-name", "--worker-name="}, required = false) @@ -129,7 +135,8 @@ int main(int argc, const char** argv) { auto log_dir = opts.log_dir.value_or(""); if(opts.stateful) { auto limit = opts.memory_limit.value_or(4ULL * 1024 * 1024 * 1024); - exit_code = clice::run_stateful_worker_mode(limit, name, log_dir); + auto max_docs = opts.max_documents.value_or(clice::default_max_documents); + exit_code = clice::run_stateful_worker_mode(limit, name, log_dir, max_docs); } else { exit_code = clice::run_stateless_worker_mode(name, log_dir); } diff --git a/src/server/compiler/compiler.cpp b/src/server/compiler/compiler.cpp index 72bd8888a..365e0b4b3 100644 --- a/src/server/compiler/compiler.cpp +++ b/src/server/compiler/compiler.cpp @@ -283,9 +283,31 @@ std::string uri_to_path(const std::string& uri) { return uri; } +/// The pch_ref write license: a round may (re)write the session's PCH +/// reference only while BOTH staleness tokens still hold their takeoff +/// values. A supersede bumps generation; a Lost-type invalidation (disk or +/// CDB change behind an in-flight round) bumps only dirty_epoch — either +/// way the round's resolved directory/arguments may describe a command +/// that no longer exists, and writing its PCH key back would hand later +/// incomplete-preamble edits a stale-flag PCH. +static bool may_write_pch_ref(const Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch) { + return session.generation == launch_generation && session.dirty_epoch == launch_epoch; +} + kota::task Compiler::ensure_pch(Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch, const std::string& directory, const std::vector& arguments) { + // A round invalidated during the caller's earlier awaits (module + // dependencies) must not touch pch_ref at all: the reset and cache-hit + // branches below write it before the first suspension point. + if(!may_write_pch_ref(session, launch_generation, launch_epoch)) { + co_return false; + } + auto path_id = session.path_id; auto path = workspace.path_pool.resolve(path_id); auto& text = session.text; @@ -364,6 +386,12 @@ kota::task Compiler::ensure_pch(Session& session, if(auto it = workspace.pch_cache.find(pch_key); it != workspace.pch_cache.end() && it->second.building) { co_await it->second.building->wait(); + // Guard the pch_ref write below against an invalidated round's + // continuation: a newer round (or a context switch) may have + // established the session's PCH identity while we waited. + if(!may_write_pch_ref(session, launch_generation, launch_epoch)) { + co_return false; + } if(auto it2 = workspace.pch_cache.find(pch_key); it2 != workspace.pch_cache.end() && !it2->second.path.empty()) { session.pch_ref = Session::PCHRef{pch_key, it2->second.bound}; @@ -430,13 +458,18 @@ kota::task Compiler::ensure_pch(Session& session, st.inactive_regions = std::move(result.value().inactive_regions); st.open_conditionals = std::move(result.value().open_conditionals); - session.pch_ref = Session::PCHRef{pch_key, bound}; - LOG_INFO("PCH built for {}: {}", path, st.path); // Persist cache metadata after successful build. workspace.save_cache(contexts); + // The cache entry above is content-keyed and correct regardless; only + // the session pointer must not be written by an invalidated round. + if(!may_write_pch_ref(session, launch_generation, launch_epoch)) { + co_return false; + } + session.pch_ref = Session::PCHRef{pch_key, bound}; + co_return true; } @@ -444,6 +477,8 @@ kota::task Compiler::ensure_pch(Session& session, /// Shared preparation step used by both ensure_compiled() (stateful path) /// and forward_stateless() (completion/signatureHelp path). kota::task Compiler::ensure_deps(Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch, const std::string& directory, const std::vector& arguments, std::pair& pch, @@ -540,7 +575,8 @@ kota::task Compiler::ensure_deps(Session& session, } // Build or reuse PCH. - auto pch_ok = co_await ensure_pch(session, directory, arguments); + auto pch_ok = + co_await ensure_pch(session, launch_generation, launch_epoch, directory, arguments); if(pch_ok && session.pch_ref.has_value()) { if(auto pch_it = workspace.pch_cache.find(session.pch_ref->key); pch_it != workspace.pch_cache.end()) { @@ -589,6 +625,11 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { auto pc = session->compiling; auto pid = session->path_id; auto gen = session->generation; + // Takeoff snapshot for the conditional dirty-flag clear on landing + // (see Session::settle_compile). The generation checks below answer + // "is the buffer still the same buffer"; this answers "did the world + // get dirty again while we were flying". + auto epoch = session->dirty_epoch; auto finish_compile = [&]() { if(session->compiling == pc) { @@ -628,7 +669,17 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { } contexts.append_suffix_include(*session, params.text); + // Whether this round is the self-containment probe: a header + // deliberately compiled without its includer prefix to see if it + // stands alone. Decided here, where resolve_command chose to omit + // the prefix; the landing gates what the probe may write. + bool trial_round = attempt == 0 && !session->trial_done && header_context && + header_context->preamble_path.empty() && + contexts.header_mode(file_path, pid) == HeaderMode::Unknown; + bool deps_ok = co_await ensure_deps(*session, + gen, + epoch, params.directory, params.arguments, params.pch, @@ -700,6 +751,18 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { co_return; } + // A probe invalidated mid-flight is discarded whole: its verdict is + // a conditional write like the dirty flag (dispatch reset trial_done + // and the header mode for the recompile to re-earn), and its + // diagnostics come from a compile deliberately run without includer + // context — they are never published, including on this path. + // ast_dirty is still set, so the next request re-runs the trial. + if(trial_round && session->dirty_epoch != epoch) { + LOG_INFO("Discarding invalidated self-containment probe for {}", uri_str); + finish_compile(); + co_return; + } + // Self-containment trial verdict. Scored once per settled input // state: trial_done is reset whenever compile inputs change for // reasons other than buffer edits, so a dependency change re-runs @@ -707,10 +770,7 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { // persisted — SelfContained is recorded in memory alone (dependency // changes erase it) so queryContext can dedup identical-flag hosts // once the verdict is actually earned, never on a guess. - auto* trial_context = contexts.header_context(pid); - if(attempt == 0 && !session->trial_done && trial_context && - trial_context->preamble_path.empty() && - contexts.header_mode(file_path, pid) == HeaderMode::Unknown) { + if(trial_round) { std::vector diagnostics; if(!result.value().diagnostics.empty()) { [[maybe_unused]] auto status = @@ -729,7 +789,11 @@ kota::task<> Compiler::run_compile(std::shared_ptr session) { } } - session->ast_dirty = false; + // Conditional write: if an invalidation landed mid-flight (a header + // was saved, the document was evicted, ...) this product describes + // a stale world — record it, publish it (bounded staleness), but do + // not declare it fresh; the next request recompiles. + session->settle_compile(epoch); pc->succeeded = true; record_deps(*session, result.value().deps); @@ -802,10 +866,14 @@ kota::task Compiler::ensure_compiled(std::shared_ptr session) { if(!is_stale(*session)) { co_return true; } - // Dependency change, not a buffer edit: re-run the trial too. - session->ast_dirty = true; - session->trial_done = false; - contexts.forget_self_contained(path_id); + // A dependency changed on disk behind this session's back — the + // lazy twin of the file tracker's DiskChanged. Route it through + // the event pipeline (synchronous) so both share one cascade; for + // an open file that dispatch marks the AST dirty, resets the trial + // and bumps dirty_epoch. The dispatch re-resolves the session by + // path_id; no suspension separates it from this frame, so it finds + // the same open session this coroutine holds. + on_stale(path_id); } // If an up-to-date compile is already in flight, wait for it. @@ -947,6 +1015,11 @@ Compiler::RawResult Compiler::forward_build(worker::BuildKind kind, auto path_id = session->path_id; auto path = std::string(workspace.path_pool.resolve(path_id)); auto gen = session->generation; + // Takeoff snapshot for the pch_ref write license (see + // may_write_pch_ref): this request runs concurrently with compiles and + // holds no compiling token, so it is the easiest continuation to come + // back stale after a disk/CDB change. + auto epoch = session->dirty_epoch; worker::BuildParams wp; wp.priority = worker::Priority::High; @@ -958,7 +1031,7 @@ Compiler::RawResult Compiler::forward_build(worker::BuildKind kind, contexts.append_suffix_include(*session, wp.text); ScopedTimer timer; - if(!co_await ensure_deps(*session, wp.directory, wp.arguments, wp.pch, wp.pcms)) { + if(!co_await ensure_deps(*session, gen, epoch, wp.directory, wp.arguments, wp.pch, wp.pcms)) { LOG_WARN("forward_build: dependency preparation failed for {}", path); co_return kota::outcome_error(kota::ipc::Error{"Dependency preparation failed"}); } diff --git a/src/server/compiler/compiler.h b/src/server/compiler/compiler.h index 3b585f616..eb105afc3 100644 --- a/src/server/compiler/compiler.h +++ b/src/server/compiler/compiler.h @@ -23,6 +23,12 @@ namespace clice { +namespace testing { + +struct CompilerFixture; + +} + namespace protocol = kota::ipc::protocol; class ContextResolver; @@ -98,15 +104,34 @@ class Compiler { /// Callback invoked when indexing should be scheduled. std::function on_indexing_needed; + /// Invoked from ensure_compiled's fast path when the pull-side + /// staleness check finds a dependency changed on disk. The owner routes + /// it into the event pipeline as a DiskChanged (synchronously), so lazy + /// detection and the file tracker's polling share one invalidation + /// cascade instead of maintaining two. + std::function on_stale; + /// Cancel in-flight compile tasks and wait for them to finish. kota::task<> stop(); private: kota::task<> run_compile(std::shared_ptr session); + /// @param launch_generation, launch_epoch The caller's staleness-token + /// snapshots from the moment its round took off, NOT ones + /// taken on entry: a round invalidated during the + /// dependency phase would otherwise re-snapshot the new + /// values here and slip a stale pch_ref past the write + /// guards. Both tokens are needed — a supersede bumps + /// generation, but a Lost-type invalidation (disk or CDB + /// change behind an in-flight round) bumps only + /// dirty_epoch, and a round that resolved its command + /// before the event must not write pch_ref back either. /// @param scope When set, cancels the module-dependency wait if this /// compile round is superseded by a newer one. kota::task ensure_deps(Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch, const std::string& directory, const std::vector& arguments, std::pair& pch, @@ -114,6 +139,8 @@ class Compiler { std::optional scope = {}); kota::task ensure_pch(Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch, const std::string& directory, const std::vector& arguments); @@ -125,6 +152,8 @@ class Compiler { ContextResolver& contexts; WorkerPool& pool; kota::task_group<> compile_tasks{loop}; + + friend struct testing::CompilerFixture; }; } // namespace clice diff --git a/src/server/compiler/context_resolver.cpp b/src/server/compiler/context_resolver.cpp index 3de2f45dc..911185784 100644 --- a/src/server/compiler/context_resolver.cpp +++ b/src/server/compiler/context_resolver.cpp @@ -751,12 +751,7 @@ bool ContextResolver::drop_orphaned_choices(SessionStore& sessions) { LOG_INFO("Dropping orphaned context choice for {}: its basis no longer exists", workspace.path_pool.resolve(session_id)); drop_header_context(session_id); - session->pch_ref.reset(); - session->ast_dirty = true; - session->trial_done = false; - // Invalidate in-flight compiles so they cannot clobber the - // reset state when they finish (same as switchContext). - session->generation += 1; + SessionStore::reset_compile_state(*session, ResetDepth::Superseded); saved_contexts.erase(it); dropped_saved = true; } @@ -984,17 +979,12 @@ ext::SwitchContextResult ContextResolver::switch_context(llvm::StringRef path, } drop_header_context(path_id); - session->pch_ref.reset(); - session->ast_deps.reset(); - session->ast_dirty = true; - // The new context needs its own self-containment trial — a - // different host can change the macro environment. - session->trial_done = false; + // The new context is a different compilation identity: supersede any + // in-flight compile and drop the state earned under the old one. It + // also needs its own self-containment trial — a different host can + // change the macro environment. + SessionStore::reset_compile_state(*session, ResetDepth::Superseded); forget_self_contained(path_id); - // Invalidate any in-flight compile: without the bump it would - // clobber ast_dirty on completion and publish results for the - // old context, with nothing left for is_stale() to detect. - session->generation++; // The table entry is the active choice; persist it across sessions. saved_contexts[path_id] = std::move(saved); diff --git a/src/server/state/invalidator.cpp b/src/server/state/invalidator.cpp index 62c76cc5e..fbafef1a3 100644 --- a/src/server/state/invalidator.cpp +++ b/src/server/state/invalidator.cpp @@ -335,13 +335,6 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { dirty.reschedule_indexing = true; break; } - case FileEvent::Kind::ContextChanged: { - // Context validation, persistence and session reset happen in - // ContextResolver::switch_context, which already lives in the - // right module; this case is a hook for future cross-file - // policy. - break; - } case FileEvent::Kind::WorkerCrashed: { // The worker's ASTs are gone; every document it owned must // recompile. Compile inputs did not change, so trial state @@ -351,6 +344,13 @@ DirtySet Invalidator::apply(llvm::ArrayRef events) { } break; } + case FileEvent::Kind::DocumentEvicted: { + // Same loss as a crash, scoped to one document: without the + // recompile, feature requests re-route to a worker that no + // longer holds the AST and silently return null. + dirty.mark_lost.push_back(event.path_id); + break; + } } } diff --git a/src/server/state/invalidator.h b/src/server/state/invalidator.h index 194ad0b88..b5e7a4e23 100644 --- a/src/server/state/invalidator.h +++ b/src/server/state/invalidator.h @@ -36,10 +36,12 @@ struct FileEvent { /// The compilation database was reloaded; `cdb` lists the files /// whose entries were added, removed or changed. CDBChanged, - /// clice/switchContext changed the file's active header context. - ContextChanged, /// A stateful worker crashed; `paths` lists the documents it owned. WorkerCrashed, + /// A stateful worker evicted the document from its LRU cache: the + /// built AST is gone, but unlike a crash only this one document is + /// affected. + DocumentEvicted, }; /// CDBChanged payload: the reload's per-file delta, as master path-pool @@ -91,20 +93,30 @@ struct FileEvent { return event; } - static FileEvent context_changed(std::uint32_t path_id) { - return {Kind::ContextChanged, path_id}; - } - static FileEvent worker_crashed(llvm::ArrayRef lost_documents) { FileEvent event{Kind::WorkerCrashed}; event.paths.assign(lost_documents.begin(), lost_documents.end()); return event; } + + static FileEvent document_evicted(std::uint32_t path_id) { + return {Kind::DocumentEvicted, path_id}; + } }; /// The effects an event batch demands, deduplicated. The engine computes /// these; MasterServer::dispatch() executes them against the mutable /// services (sessions, context resolver, background indexer). +/// +/// Effect algebra: the sets are not disjoint, and stronger effects subsume +/// weaker ones on the same file — mark_ast_dirty implies the trial reset +/// that reset_trial asks for, force_revalidate implies mark_ast_dirty's +/// session treatment, and one event may push a file into several sets +/// (BufferSaved emits both reset_trial and reset_header_mode for the saved +/// file). Execution is idempotent per effect, so the overlap is harmless; +/// what matters is that each set can also occur ALONE (reset_trial without +/// mark_ast_dirty re-runs the trial on a clean AST), which is why they are +/// separate vocabulary rather than severity levels of one list. struct DirtySet { /// Compile inputs changed: ast_dirty + trial_done=false + forget the /// cached self-containment verdict. @@ -165,10 +177,15 @@ struct DirtySet { /// as a DirtySet effect and executed by the dispatcher, so the engine /// stays testable with plain data structures. /// -/// The engine is told about every event, but the buffer-change mechanics of -/// BufferOpened/BufferEdited (text, version, ast_dirty, generation) are the -/// SessionStore's charter and stay in apply_open/apply_change; those cases -/// exist as hooks for future cross-file policy, not as the sync path. +/// Exemption criterion — invalidation logic may bypass apply() if and only +/// if it (1) has no cross-file cascade, (2) touches only a single owner's +/// state, and (3) completes within one synchronous section. SessionStore's +/// buffer mechanics qualify (apply_open/apply_change own text, version, +/// ast_dirty, generation; the BufferOpened/BufferEdited cases below exist +/// as hooks for future cross-file policy, not as the sync path), and so +/// does clice/switchContext's session reset (single owner, synchronous, +/// no cascade). Anything failing a clause goes through the pipeline — do +/// not add ceremonial event kinds for exempt logic. class Invalidator { public: /// Read a file's current on-disk content, or nullopt if unreadable. diff --git a/src/server/state/session.h b/src/server/state/session.h index c7eb3a02c..f8d56d327 100644 --- a/src/server/state/session.h +++ b/src/server/state/session.h @@ -81,6 +81,28 @@ struct Session { /// Whether the AST needs to be rebuilt before serving queries. bool ast_dirty = true; + /// Invalidation epoch: bumped every time an event dispatch applies an + /// AST-invalidating effect to this session (dependency changed on disk, + /// worker crash, document eviction, ...). A compile snapshots it at + /// takeoff and may clear ast_dirty on landing only if it is unchanged — + /// see settle_compile(). Division of labor with generation: generation + /// answers "is the buffer still the same buffer?", dirty_epoch answers + /// "did the world get dirty again after I took off?". + std::uint64_t dirty_epoch = 0; + + /// Clearing ast_dirty is a conditional write — the only sanctioned way + /// for a compile to declare its product fresh. `launch_epoch` is the + /// dirty_epoch snapshotted when the compile took off; if any + /// invalidation landed while it was in flight, the flag stays set and + /// the next request recompiles. The compile's artifacts (deps snapshot, + /// file index, diagnostics) may still be recorded — they are not wrong, + /// only not-current. + void settle_compile(std::uint64_t launch_epoch) { + if(dirty_epoch == launch_epoch) { + ast_dirty = false; + } + } + /// Non-null while a compilation is in flight for this file. /// Other queries wait on the event; the compilation task itself /// runs independently and cannot be cancelled by LSP $/cancelRequest. diff --git a/src/server/state/session_store.cpp b/src/server/state/session_store.cpp index 99dedc5d6..4a2ef5dc7 100644 --- a/src/server/state/session_store.cpp +++ b/src/server/state/session_store.cpp @@ -79,4 +79,26 @@ void SessionStore::apply_change(Session& session, session.ast_dirty = true; } +void SessionStore::reset_compile_state(Session& session, ResetDepth depth) { + session.ast_dirty = true; + switch(depth) { + case ResetDepth::Superseded: { + session.pch_ref.reset(); + session.ast_deps.reset(); + session.trial_done = false; + // Invalidate any in-flight compile: without the bump it would + // pass its generation check on completion and publish results + // for the superseded identity. + session.generation++; + break; + } + case ResetDepth::Lost: { + // An in-flight compile consumed the pre-loss world; it must not + // clear ast_dirty when it lands (see Session::settle_compile). + session.dirty_epoch++; + break; + } + } +} + } // namespace clice diff --git a/src/server/state/session_store.h b/src/server/state/session_store.h index aa0790c9c..45fe76b9f 100644 --- a/src/server/state/session_store.h +++ b/src/server/state/session_store.h @@ -15,6 +15,23 @@ namespace clice { namespace protocol = kota::ipc::protocol; +/// How much of a session's compile state a reset invalidates. Both depths +/// mark the AST dirty; they differ in which staleness token they bump. +enum class ResetDepth : std::uint8_t { + /// The buffer's identity changed for compilation purposes (context + /// switch, orphaned context choice): in-flight compile results no + /// longer describe this document. Bumps generation, drops the PCH + /// reference and dependency snapshot earned under the old identity, + /// and re-arms the self-containment trial. + Superseded, + /// The built AST is gone (worker eviction/crash) or its inputs + /// changed, but the buffer is still the same buffer. Bumps + /// dirty_epoch so an in-flight compile cannot declare its product + /// fresh; master-side caches (PCH, deps snapshot) stay — pull-side + /// validation decides their fate. + Lost, +}; + /// The table of open documents plus the buffer-synchronization logic: the /// single owner of editor buffer truth. Every didOpen/didChange edit lands /// here, and every reader of an open file's text goes through the sessions @@ -54,6 +71,16 @@ struct SessionStore { void apply_change(Session& session, llvm::ArrayRef changes, int version); + + /// Invalidate a session's compile state to the given depth (see + /// ResetDepth). The single reset vocabulary for every "this session + /// must recompile" site — context switches, orphaned choices, event + /// dispatch effects — so the token discipline lives in one place. + /// Static so call sites that hold a Session* but no store reference + /// (ContextResolver::switch_context) can use it; it lives here rather + /// than on Session because state mutation vocabulary is this store's + /// charter. + static void reset_compile_state(Session& session, ResetDepth depth); }; } // namespace clice diff --git a/src/server/transport/lsp_client.cpp b/src/server/transport/lsp_client.cpp index 9cfd2f3f3..e7358d62e 100644 --- a/src/server/transport/lsp_client.cpp +++ b/src/server/transport/lsp_client.cpp @@ -293,9 +293,10 @@ void LSPClient::register_document_sync() { // is: a session opened during the handshake window must not stay // open forever when the editor already closed it. The diagnostics // clear is suppressed until the handshake completes — nothing was - // pushed, and publishDiagnostics may not flow yet. + // pushed, and publishDiagnostics may not flow yet (push_output + // drops the clear while !client_ready). auto [path, path_id, session] = resolve_uri(params.text_document.uri); - srv.close_session(path_id, client_ready ? &this->peer : nullptr); + srv.close_session(path_id); }); peer.on_notification([this](const protocol::DidSaveTextDocumentParams& params) { @@ -544,13 +545,15 @@ void LSPClient::register_extensions() { [this](RequestContext& ctx, const ext::SwitchContextParams& params) -> RawResult { auto [path, path_id, session] = resolve_uri(params.uri); auto [context_path, context_path_id, context_session] = resolve_uri(params.context_uri); + // The session reset lives inside switch_context (single owner, + // synchronous, no cross-file cascade — exempt from the event + // pipeline; see the Invalidator charter). auto result = this->server.contexts.switch_context(path, path_id, session.get(), context_path, context_path_id, params); - this->server.dispatch(FileEvent::context_changed(path_id)); co_return to_raw(result); }); diff --git a/src/server/transport/master_server.cpp b/src/server/transport/master_server.cpp index b00309fe2..330bcd713 100644 --- a/src/server/transport/master_server.cpp +++ b/src/server/transport/master_server.cpp @@ -17,8 +17,6 @@ #include "kota/async/async.h" #include "kota/codec/json/json.h" #include "kota/ipc/codec/json.h" -#include "kota/ipc/lsp/protocol.h" -#include "kota/ipc/lsp/uri.h" #include "kota/ipc/recording_transport.h" #include "kota/ipc/transport.h" #include "llvm/ADT/STLExtras.h" @@ -28,9 +26,6 @@ namespace clice { -namespace lsp = kota::ipc::lsp; -namespace protocol = kota::ipc::protocol; - /// Retention bound of the notify log. Subscribers drain promptly, so only /// messages that fire before any client attaches accumulate (a handful of /// startup guidance reports in practice); the cap is a safety net, not a @@ -163,6 +158,10 @@ kota::task<> MasterServer::workspace_poll_task() { void MasterServer::wire() { pool.on_crash = [this](const WorkerCrashInfo& info) { + // A stateless crash loses only in-flight requests, which fail back + // to their callers (send_stateless already retries once). No state + // outlives the request, so there is nothing to invalidate and no + // event to dispatch. if(!info.stateful) return; dispatch(FileEvent::worker_crashed(info.lost_documents)); @@ -174,12 +173,24 @@ void MasterServer::wire() { LOG_WARN("Evicted path not in pool: {}", path); return; } + // Owner-table upkeep is pool-domain state and stays here; the + // session-side consequence (the worker's AST is gone, same as a + // crash) goes through the event pipeline like any invalidation. pool.remove_owner(it->second); + dispatch(FileEvent::document_evicted(it->second)); }; compiler.on_indexing_needed = [this]() { indexer.schedule(); }; + + // The compiler's pull-side staleness check found a dependency changed + // on disk: route it through the same DiskChanged path the file + // tracker's polling uses, so lazy detection and polling share one + // invalidation cascade. + compiler.on_stale = [this](std::uint32_t path_id) { + dispatch(FileEvent::disk_changed(path_id)); + }; } void MasterServer::initialize(llvm::StringRef root) { @@ -195,25 +206,29 @@ std::shared_ptr MasterServer::open_session(std::uint32_t path_id) { return sessions.open(path_id); } -void MasterServer::close_session(std::uint32_t path_id, kota::ipc::JsonPeer* peer) { - namespace protocol = kota::ipc::protocol; - +void MasterServer::close_session(std::uint32_t path_id) { auto path = workspace.path_pool.resolve(path_id); // Route the eviction notification before dropping ownership: // notify_stateful uses the owner table to find the worker. pool.notify_stateful(path_id, worker::EvictParams{std::string(path)}); pool.remove_owner(path_id); - // Null before the client's handshake completed: nothing was ever - // pushed, so there are no diagnostics to clear (and the LSP spec - // forbids publishDiagnostics before the initialize response). - if(peer) { - protocol::PublishDiagnosticsParams diag_params; - auto uri = lsp::URI::from_file_path(std::string(path)); - if(uri) - diag_params.uri = uri->str(); - diag_params.diagnostics = {}; - peer->send_notification(diag_params); + // Retract the document's published diagnostics through the standard + // output path: materialize an empty output and signal the transports + // (MasterServer holds no peer — see the class charter). A transport + // whose client has not completed the handshake drops the push: nothing + // was ever published for it to clear, and publishDiagnostics may not + // flow before the initialize response. CDBExact keeps + // format_diagnostics from decorating the empty set with guidance. + if(auto session = sessions.find(path_id)) { + session->output = CompileOutput{ + .version = std::nullopt, + .source = CommandSource::CDBExact, + .diagnostics = {}, + .line_limit = std::nullopt, + .inactive_regions = std::nullopt, + }; + compiler.on_output.emit(session); } sessions.close(path_id); @@ -236,20 +251,22 @@ void MasterServer::dispatch(llvm::ArrayRef events) { contexts.reset_header_mode(path_id); } + // The Lost reset bumps dirty_epoch so an in-flight compile that + // consumed the pre-event world cannot clear ast_dirty when it lands; + // generation stays put — the buffer is still the same buffer, and + // results that pass their generation check may still be published + // (bounded staleness: the flag stays dirty, the next request recompiles). for(auto path_id: dirty.mark_ast_dirty) { if(auto session = sessions.find(path_id)) { - session->ast_dirty = true; + SessionStore::reset_compile_state(*session, ResetDepth::Lost); session->trial_done = false; - // Invalidate in-flight compiles so they cannot clobber the - // reset state when they finish (same as switchContext). - session->generation += 1; } contexts.forget_self_contained(path_id); } for(auto path_id: dirty.mark_lost) { if(auto session = sessions.find(path_id)) { - session->ast_dirty = true; + SessionStore::reset_compile_state(*session, ResetDepth::Lost); } } @@ -259,9 +276,8 @@ void MasterServer::dispatch(llvm::ArrayRef events) { for(auto path_id: dirty.force_revalidate) { contexts.invalidate_header_deps(path_id); if(auto session = sessions.find(path_id)) { - session->ast_dirty = true; + SessionStore::reset_compile_state(*session, ResetDepth::Lost); session->trial_done = false; - session->generation += 1; } } diff --git a/src/server/transport/master_server.h b/src/server/transport/master_server.h index 9bdabd12f..661d1a06c 100644 --- a/src/server/transport/master_server.h +++ b/src/server/transport/master_server.h @@ -104,10 +104,11 @@ class MasterServer { std::shared_ptr find_session(std::uint32_t path_id); std::shared_ptr open_session(std::uint32_t path_id); - /// Close the session and clear its published diagnostics on `peer`. - /// Pass null when no handshake-complete client is attached: nothing was - /// ever pushed, so there is nothing to clear. - void close_session(std::uint32_t path_id, kota::ipc::JsonPeer* peer); + /// Close the session. The diagnostics clear travels through the + /// session's output + on_output signal; a transport whose client has + /// not completed the handshake drops it (nothing was ever pushed, so + /// there is nothing to clear). + void close_session(std::uint32_t path_id); /// The single entry point for file events: fold the batch through the /// Invalidator, then execute the resulting effects against the mutable diff --git a/src/server/worker/stateful_worker.cpp b/src/server/worker/stateful_worker.cpp index 846918ccd..8bf2092a7 100644 --- a/src/server/worker/stateful_worker.cpp +++ b/src/server/worker/stateful_worker.cpp @@ -49,6 +49,7 @@ struct DocumentEntry { class StatefulWorker { kota::ipc::BincodePeer& peer; std::uint64_t memory_limit; + std::size_t max_documents; llvm::StringMap> documents; @@ -68,7 +69,7 @@ class StatefulWorker { void shrink_if_over_limit() { // TODO: Implement memory-based eviction using memory_limit. // For now, cap at a fixed number of documents. - while(documents.size() > 16 && !lru.empty()) { + while(documents.size() > max_documents && !lru.empty()) { auto path = lru.back(); lru.pop_back(); lru_index.erase(path); @@ -120,8 +121,10 @@ class StatefulWorker { } public: - StatefulWorker(kota::ipc::BincodePeer& peer, std::uint64_t memory_limit) : - peer(peer), memory_limit(memory_limit) {} + StatefulWorker(kota::ipc::BincodePeer& peer, + std::uint64_t memory_limit, + std::size_t max_documents) : + peer(peer), memory_limit(memory_limit), max_documents(max_documents) {} void register_handlers(); }; @@ -280,7 +283,8 @@ void StatefulWorker::register_handlers() { int run_stateful_worker_mode(std::uint64_t memory_limit, const std::string& worker_name, - const std::string& log_dir) { + const std::string& log_dir, + std::size_t max_documents) { logging::stderr_logger(worker_name, logging::options); if(!log_dir.empty()) { // File only: worker stderr is reserved for crash/unexpected output, @@ -300,7 +304,7 @@ int run_stateful_worker_mode(std::uint64_t memory_limit, kota::ipc::BincodePeer peer(loop, std::move(*transport_result)); - StatefulWorker worker(peer, memory_limit); + StatefulWorker worker(peer, memory_limit, max_documents); worker.register_handlers(); LOG_INFO("Stateful worker ready, waiting for requests"); diff --git a/src/server/worker/stateful_worker.h b/src/server/worker/stateful_worker.h index c3c8c36c7..40e3084e3 100644 --- a/src/server/worker/stateful_worker.h +++ b/src/server/worker/stateful_worker.h @@ -1,15 +1,23 @@ #pragma once +#include #include #include namespace clice { +/// Default cap on documents a stateful worker keeps compiled at once; +/// the least recently used document past it is evicted (the master learns +/// through an EvictedParams notification). Overridable per process via +/// `--max-documents`, which tests use to drive eviction cheaply. +constexpr inline std::size_t default_max_documents = 16; + /// Run the stateful worker process mode. /// The worker holds compiled ASTs and handles feature requests /// (hover, semantic tokens, etc.) alongside compile requests. int run_stateful_worker_mode(std::uint64_t memory_limit, const std::string& worker_name, - const std::string& log_dir); + const std::string& log_dir, + std::size_t max_documents = default_max_documents); } // namespace clice diff --git a/tests/integration/features/test_server.py b/tests/integration/features/test_server.py index 2af2ea778..340703d4f 100644 --- a/tests/integration/features/test_server.py +++ b/tests/integration/features/test_server.py @@ -114,6 +114,15 @@ async def test_diagnostics_received(client, workspace): client.close(uri) +@pytest.mark.workspace("hello_world") +async def test_close_clears_diagnostics(client, workspace): + uri, _ = await client.open_and_wait(workspace / "main.cpp") + event = client.wait_for_diagnostics(uri) + client.close(uri) + await asyncio.wait_for(event.wait(), timeout=10.0) + assert client.diagnostics[uri] == [] + + @pytest.mark.workspace("hello_world") async def test_hover_before_compile(client, workspace): uri, _ = client.open(workspace / "main.cpp") diff --git a/tests/integration/stress/test_eviction.py b/tests/integration/stress/test_eviction.py new file mode 100644 index 000000000..35b14593f --- /dev/null +++ b/tests/integration/stress/test_eviction.py @@ -0,0 +1,29 @@ +"""Worker document eviction: opening more files than a stateful worker's +LRU cap must not silently break features on the evicted documents.""" + +from tests.integration.utils import write_cdb + +FILE_COUNT = 18 # one stateful worker holds at most 16 compiled documents + + +async def test_evicted_document_recovers(client, tmp_path): + names = [] + for i in range(FILE_COUNT): + name = f"file_{i:02}.cpp" + (tmp_path / name).write_text(f"int value_{i} = {i};\n", newline="\n") + names.append(name) + write_cdb(tmp_path, names) + # A single stateful worker so all opens land in one document cache. + await client.initialize( + tmp_path, + initialization_options={"project": {"stateful_worker_count": 1}}, + ) + + first_uri, _ = await client.open_and_wait(tmp_path / names[0]) + for name in names[1:]: + await client.open_and_wait(tmp_path / name) + + # The first file was LRU-evicted from the worker; hover must trigger a + # recompile instead of silently returning null against the lost AST. + hover = await client.hover_at(first_uri, 0, 4) + assert hover is not None, "hover on the evicted document must recover" diff --git a/tests/unit/server/compiler_tests.cpp b/tests/unit/server/compiler_tests.cpp new file mode 100644 index 000000000..5b87ec7fa --- /dev/null +++ b/tests/unit/server/compiler_tests.cpp @@ -0,0 +1,71 @@ +#include +#include + +#include "test/test.h" +#include "server/compiler/compiler.h" +#include "server/compiler/context_resolver.h" + +namespace clice::testing { + +/// Reaches Compiler's private compile-preparation steps for guard tests. +struct CompilerFixture { + static kota::task ensure_pch(Compiler& compiler, + Session& session, + std::uint64_t launch_generation, + std::uint64_t launch_epoch, + const std::string& directory, + const std::vector& arguments) { + return compiler.ensure_pch(session, launch_generation, launch_epoch, directory, arguments); + } +}; + +namespace { + +TEST_SUITE(CompilerGuards) { + +TEST_CASE(EpochGuardsPchWrite) { + kota::event_loop loop; + Workspace workspace; + ContextResolver contexts(workspace); + WorkerPool pool(loop); + Compiler compiler(loop, workspace, contexts, pool); + + Session session; + session.path_id = workspace.path_pool.intern("/proj/a.cpp"); + // No preamble directives: a current round would take the pch_ref reset + // branch; an invalidated continuation must not touch it. + session.text = "int x;"; + session.pch_ref = Session::PCHRef{"key", 0}; + + auto gen = session.generation; + auto epoch = session.dirty_epoch; + // A Lost-type invalidation (disk/CDB change behind the in-flight + // round) lands after takeoff: dirty_epoch bumps, generation stays. + session.dirty_epoch += 1; + + std::string directory = "/proj"; + std::vector arguments = {"clang++", "-fsyntax-only", "/proj/a.cpp"}; + bool wrote = true; + auto body = [&]() -> kota::task<> { + wrote = co_await CompilerFixture::ensure_pch(compiler, + session, + gen, + epoch, + directory, + arguments); + }; + auto task = body(); + loop.schedule(task); + loop.run(); + + EXPECT_FALSE(wrote); + // The stale continuation left the session's PCH reference untouched. + ASSERT_TRUE(session.pch_ref.has_value()); + EXPECT_EQ(session.pch_ref->key, std::string("key")); +} + +}; // TEST_SUITE(CompilerGuards) + +} // namespace + +} // namespace clice::testing diff --git a/tests/unit/server/invalidator_tests.cpp b/tests/unit/server/invalidator_tests.cpp index 3219462ce..d53ecc80f 100644 --- a/tests/unit/server/invalidator_tests.cpp +++ b/tests/unit/server/invalidator_tests.cpp @@ -29,11 +29,9 @@ TEST_CASE(NoOpEventsNoEffects) { ContextResolver resolver(workspace); Invalidator invalidator(workspace, store, resolver); - // Buffer sync stays in SessionStore and context switching in - // ContextResolver; these events must produce no effects of their own. - FileEvent events[] = {FileEvent::buffer_opened(file), - FileEvent::buffer_edited(file), - FileEvent::context_changed(file)}; + // Buffer sync stays in SessionStore (exempt from the pipeline); these + // events must produce no effects of their own. + FileEvent events[] = {FileEvent::buffer_opened(file), FileEvent::buffer_edited(file)}; auto dirty = invalidator.apply(events); ASSERT_TRUE(dirty.empty()); @@ -233,6 +231,22 @@ TEST_CASE(CrashMarksLostDirty) { ASSERT_TRUE(dirty.reset_trial.empty()); } +TEST_CASE(EvictionMarksLost) { + Workspace workspace; + SessionStore store; + auto file = workspace.path_pool.intern("/proj/a.cpp"); + store.open(file); + + ContextResolver resolver(workspace); + Invalidator invalidator(workspace, store, resolver); + auto dirty = invalidator.apply(FileEvent::document_evicted(file)); + + // Same loss as a crash, scoped to one document. + ASSERT_EQ(dirty.mark_lost, llvm::SmallVector{file}); + ASSERT_TRUE(dirty.mark_ast_dirty.empty()); + ASSERT_TRUE(dirty.reset_trial.empty()); +} + TEST_CASE(BatchSavesDeduplicate) { Workspace workspace; SessionStore store; diff --git a/tests/unit/server/session_store_tests.cpp b/tests/unit/server/session_store_tests.cpp index c9a8adbc5..e247ba81a 100644 --- a/tests/unit/server/session_store_tests.cpp +++ b/tests/unit/server/session_store_tests.cpp @@ -120,6 +120,63 @@ TEST_CASE(CloseBumpsGeneration) { ASSERT_EQ(store.find(7), nullptr); } +TEST_CASE(ResetSupersededBumpsGeneration) { + SessionStore store; + auto session = store.open(1); + store.apply_open(*session, "int x;", 1); + session->ast_dirty = false; + session->trial_done = true; + session->pch_ref = Session::PCHRef{"key", 4}; + session->ast_deps.emplace(); + auto gen = session->generation; + auto epoch = session->dirty_epoch; + + SessionStore::reset_compile_state(*session, ResetDepth::Superseded); + + ASSERT_TRUE(session->ast_dirty); + ASSERT_FALSE(session->trial_done); + ASSERT_FALSE(session->pch_ref.has_value()); + ASSERT_FALSE(session->ast_deps.has_value()); + ASSERT_EQ(session->generation, gen + 1); + ASSERT_EQ(session->dirty_epoch, epoch); +} + +TEST_CASE(ResetLostBumpsEpoch) { + SessionStore store; + auto session = store.open(1); + store.apply_open(*session, "int x;", 1); + session->ast_dirty = false; + session->trial_done = true; + session->pch_ref = Session::PCHRef{"key", 4}; + auto gen = session->generation; + auto epoch = session->dirty_epoch; + + SessionStore::reset_compile_state(*session, ResetDepth::Lost); + + // The buffer is still the same buffer and its inputs did not change: + // only the freshness claim is revoked. + ASSERT_TRUE(session->ast_dirty); + ASSERT_TRUE(session->trial_done); + ASSERT_TRUE(session->pch_ref.has_value()); + ASSERT_EQ(session->generation, gen); + ASSERT_EQ(session->dirty_epoch, epoch + 1); +} + +TEST_CASE(SettleCompileConditional) { + Session session; + session.ast_dirty = true; + auto launch_epoch = session.dirty_epoch; + + // Invalidation landed mid-flight: the product must not claim freshness. + session.dirty_epoch += 1; + session.settle_compile(launch_epoch); + ASSERT_TRUE(session.ast_dirty); + + // Quiet flight: the clear goes through. + session.settle_compile(session.dirty_epoch); + ASSERT_FALSE(session.ast_dirty); +} + TEST_CASE(ForEachVisitsAll) { SessionStore store; store.open(1); diff --git a/tests/unit/server/stateful_worker_tests.cpp b/tests/unit/server/stateful_worker_tests.cpp index b259c87a0..4c581b90e 100644 --- a/tests/unit/server/stateful_worker_tests.cpp +++ b/tests/unit/server/stateful_worker_tests.cpp @@ -416,6 +416,59 @@ TEST_CASE(EvictNotification) { ASSERT_TRUE(test_done); } +TEST_CASE(LowLimitDrivesEviction) { + TempDir tmp; + std::vector paths; + std::vector texts; + for(int i = 0; i < 3; i++) { + auto name = "evict_" + std::to_string(i) + ".cpp"; + auto text = "int var_" + std::to_string(i) + " = " + std::to_string(i) + ";\n"; + tmp.touch(name, text); + paths.push_back(tmp.path(name)); + texts.push_back(text); + } + + WorkerHandle w; + ASSERT_TRUE(w.spawn(4ULL * 1024 * 1024 * 1024, /*max_documents=*/2)); + + std::vector evicted; + w.peer->on_notification( + [&](const worker::EvictedParams& params) { evicted.push_back(params.path); }); + + bool test_done = false; + + w.run([&]() -> kota::task<> { + for(int i = 0; i < 3; i++) { + worker::CompileParams cp; + cp.path = paths[i]; + cp.version = 1; + cp.text = texts[i]; + cp.directory = "/tmp"; + cp.arguments = make_args(paths[i]); + + auto result = co_await w.peer->send_request(cp); + EXPECT_TRUE(result.has_value()); + } + + // The third compile overflowed the 2-document cap: the least + // recently used document was evicted and announced to the master. + worker::QueryParams hp; + hp.kind = worker::QueryKind::Hover; + hp.path = paths[0]; + hp.offset = 4; // 'var_0' + + auto result = co_await w.peer->send_request(hp); + CO_ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result.value().data, std::string("null")); + + test_done = true; + w.peer->close_output(); + }); + + ASSERT_TRUE(test_done); + ASSERT_EQ(evicted, std::vector{paths[0]}); +} + TEST_CASE(SpawnWithMemoryLimit) { TempDir tmp; tmp.touch("memlimit_test.cpp", "int memlimit_var = 42;\n"); diff --git a/tests/unit/server/worker_test_helpers.h b/tests/unit/server/worker_test_helpers.h index 02763e0e7..e36d20d5f 100644 --- a/tests/unit/server/worker_test_helpers.h +++ b/tests/unit/server/worker_test_helpers.h @@ -65,7 +65,7 @@ struct WorkerHandle { std::unique_ptr peer; int stderr_fd = -1; - bool spawn(std::uint64_t memory_limit = 0) { + bool spawn(std::uint64_t memory_limit = 0, std::size_t max_documents = 0) { auto binary = clice_binary(); auto label = memory_limit > 0 ? "stateful" : "stateless"; @@ -82,6 +82,10 @@ struct WorkerHandle { opts.args.push_back("--memory-limit"); opts.args.push_back(std::to_string(memory_limit)); } + if(max_documents > 0) { + opts.args.push_back("--max-documents"); + opts.args.push_back(std::to_string(max_documents)); + } opts.streams = { kota::process::stdio::pipe(true, false), // stdin: child reads kota::process::stdio::pipe(false, true), // stdout: child writes