Skip to content

Commit 2d0eec9

Browse files
16bit-ykikoclaude
andcommitted
fix: address CodeRabbit review findings
- compile_graph: clean stale reverse edges on re-registration - master_server: re-lookup document after co_await to avoid dangling ref - master_server: don't set Ready until worker pool starts successfully - master_server: pass worker_memory_limit to pool options - master_server: send DocumentUpdateParams on didChange - stateful_worker: use std::string in LRU list to avoid dangling StringRef - tests: filter publishDiagnostics by target URI to prevent false matches - test_file_operation: clear diagnostics latch before triggering rebuild Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 4c6751d commit 2d0eec9

File tree

5 files changed

+53
-25
lines changed

5 files changed

+53
-25
lines changed

src/server/compile_graph.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,16 @@ namespace clice {
77
void CompileGraph::register_unit(std::uint32_t path_id, llvm::ArrayRef<std::uint32_t> deps) {
88
auto& unit = units[path_id];
99
unit.path_id = path_id;
10+
11+
// Remove this unit from old dependencies' dependents lists
12+
for(auto old_dep: unit.dependencies) {
13+
auto it = units.find(old_dep);
14+
if(it != units.end()) {
15+
auto& dep_list = it->second.dependents;
16+
dep_list.erase(std::remove(dep_list.begin(), dep_list.end(), path_id), dep_list.end());
17+
}
18+
}
19+
1020
unit.dependencies.assign(deps.begin(), deps.end());
1121
for(auto dep_id: deps) {
1222
units[dep_id].dependents.push_back(path_id);

src/server/master_server.cpp

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -95,23 +95,24 @@ et::task<> MasterServer::run_build_drain(std::uint32_t path_id, std::string uri)
9595
if(doc_it == documents.end())
9696
co_return;
9797

98-
auto& doc = doc_it->second;
99-
doc.build_running = true;
100-
doc.build_requested = false;
101-
auto gen = doc.generation;
98+
doc_it->second.build_running = true;
99+
doc_it->second.build_requested = false;
100+
auto gen = doc_it->second.generation;
102101

103102
// Ensure PCM/PCH dependencies are ready
104103
auto deps_ok = co_await compile_graph.compile_deps(path_id, loop);
104+
105+
// Re-lookup after co_await (map may have changed)
106+
doc_it = documents.find(path_id);
107+
if(doc_it == documents.end())
108+
co_return;
109+
105110
if(!deps_ok) {
106111
LOG_WARN("Dependency compilation failed for {}, skipping build", uri);
107112
clear_diagnostics(uri);
108-
doc_it = documents.find(path_id);
109-
if(doc_it == documents.end())
110-
co_return;
111-
auto& doc_after_deps = doc_it->second;
112-
if(!doc_after_deps.build_requested) {
113-
doc_after_deps.build_running = false;
114-
doc_after_deps.drain_scheduled = false;
113+
if(!doc_it->second.build_requested) {
114+
doc_it->second.build_running = false;
115+
doc_it->second.drain_scheduled = false;
115116
co_return;
116117
}
117118
continue;
@@ -120,8 +121,8 @@ et::task<> MasterServer::run_build_drain(std::uint32_t path_id, std::string uri)
120121
// Send compile request to stateful worker
121122
worker::CompileParams params;
122123
params.uri = uri;
123-
params.version = doc.version;
124-
params.text = doc.text;
124+
params.version = doc_it->second.version;
125+
params.text = doc_it->second.text;
125126
fill_compile_args(path_pool.resolve(path_id), params.directory, params.arguments);
126127

127128
auto result = co_await pool.send_stateful<worker::CompileParams>(path_id, params);
@@ -428,8 +429,6 @@ void MasterServer::register_handlers() {
428429

429430
// === initialized ===
430431
peer.on_notification([this](const protocol::InitializedParams& params) {
431-
lifecycle = ServerLifecycle::Ready;
432-
433432
// Load configuration from workspace
434433
config = CliceConfig::load_from_workspace(workspace_root);
435434

@@ -444,10 +443,14 @@ void MasterServer::register_handlers() {
444443
pool_opts.self_path = self_path;
445444
pool_opts.stateful_count = config.stateful_worker_count;
446445
pool_opts.stateless_count = config.stateless_worker_count;
446+
pool_opts.worker_memory_limit = config.worker_memory_limit;
447447
if(!pool.start(pool_opts)) {
448448
LOG_ERROR("Failed to start worker pool");
449+
return;
449450
}
450451

452+
lifecycle = ServerLifecycle::Ready;
453+
451454
// Load CDB and build include graph in background
452455
loop.schedule(load_workspace());
453456

@@ -567,6 +570,13 @@ void MasterServer::register_handlers() {
567570

568571
doc.generation++;
569572

573+
// Notify the owning stateful worker so it marks the document dirty
574+
worker::DocumentUpdateParams update;
575+
update.uri = params.text_document.uri;
576+
update.version = doc.version;
577+
update.text = doc.text;
578+
pool.notify_stateful(path_id, update);
579+
570580
// Reset idle timer on user activity
571581
reset_idle_timer();
572582

src/server/stateful_worker.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,16 @@ class StatefulWorker {
4848

4949
llvm::StringMap<std::unique_ptr<DocumentEntry>> documents;
5050

51-
// LRU tracking
52-
std::list<llvm::StringRef> lru;
53-
llvm::StringMap<std::list<llvm::StringRef>::iterator> lru_index;
51+
// LRU tracking — owns keys so they don't dangle after request handler returns
52+
std::list<std::string> lru;
53+
llvm::StringMap<std::list<std::string>::iterator> lru_index;
5454

5555
void touch_lru(llvm::StringRef uri) {
5656
auto it = lru_index.find(uri);
5757
if(it != lru_index.end()) {
5858
lru.erase(it->second);
5959
}
60-
lru.push_front(uri);
60+
lru.emplace_front(uri.str());
6161
lru_index[uri] = lru.begin();
6262
}
6363

tests/integration/test_file_operation.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,12 @@ async def test_clang_tidy(client: LSPClient, test_data_dir):
3636
@pytest.mark.asyncio
3737
async def test_hover_save_close(client: LSPClient, test_data_dir):
3838
workspace = test_data_dir / "hello_world"
39+
target_uri = (workspace / "main.cpp").as_uri()
3940
diagnostics_received = asyncio.Event()
4041

4142
def on_diagnostics(params):
42-
diagnostics_received.set()
43+
if params.get("uri") == target_uri:
44+
diagnostics_received.set()
4345

4446
client.register_notification_handler(
4547
"textDocument/publishDiagnostics", on_diagnostics
@@ -51,12 +53,12 @@ def on_diagnostics(params):
5153
# Wait for initial compilation to finish
5254
await asyncio.wait_for(diagnostics_received.wait(), timeout=15.0)
5355

56+
diagnostics_received.clear()
5457
content = client.get_file("main.cpp").content + "\nint saved = 1;\n"
5558
await client.did_change("main.cpp", content)
5659
await client.did_save("main.cpp", include_text=True)
5760

5861
# Wait for recompilation after change
59-
diagnostics_received.clear()
6062
await asyncio.wait_for(diagnostics_received.wait(), timeout=15.0)
6163

6264
hover = await client.hover("main.cpp", 2, 4) # hover on 'main'

tests/integration/test_server.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,12 +79,14 @@ async def test_incremental_change(client: LSPClient, test_data_dir):
7979
async def test_diagnostics_received(client: LSPClient, test_data_dir):
8080
"""Opening a file should produce diagnostics."""
8181
workspace = test_data_dir / "hello_world"
82+
target_uri = (workspace / "main.cpp").as_uri()
8283
diagnostics_received = asyncio.Event()
8384
received_diagnostics = []
8485

8586
def on_diagnostics(params):
86-
received_diagnostics.append(params)
87-
diagnostics_received.set()
87+
if params.get("uri") == target_uri:
88+
received_diagnostics.append(params)
89+
diagnostics_received.set()
8890

8991
client.register_notification_handler(
9092
"textDocument/publishDiagnostics", on_diagnostics
@@ -163,10 +165,12 @@ async def test_multiple_files(client: LSPClient, test_data_dir):
163165

164166
async def _wait_for_compilation(client, workspace):
165167
"""Helper: wait for diagnostics to confirm compilation finished."""
168+
target_uri = (workspace / "main.cpp").as_uri()
166169
diagnostics_received = asyncio.Event()
167170

168171
def on_diagnostics(params):
169-
diagnostics_received.set()
172+
if params.get("uri") == target_uri:
173+
diagnostics_received.set()
170174

171175
client.register_notification_handler(
172176
"textDocument/publishDiagnostics", on_diagnostics
@@ -387,10 +391,12 @@ async def test_hover_on_unknown_file(client: LSPClient, test_data_dir):
387391
async def test_all_features_after_compile_wait(client: LSPClient, test_data_dir):
388392
"""After waiting for compilation, exercise all feature requests and verify responses."""
389393
workspace = test_data_dir / "hello_world"
394+
target_uri = (workspace / "main.cpp").as_uri()
390395
diagnostics_received = asyncio.Event()
391396

392397
def on_diagnostics(params):
393-
diagnostics_received.set()
398+
if params.get("uri") == target_uri:
399+
diagnostics_received.set()
394400

395401
client.register_notification_handler(
396402
"textDocument/publishDiagnostics", on_diagnostics

0 commit comments

Comments
 (0)