feat(server): stat-polling file tracker - #487
Conversation
📝 WalkthroughWalkthroughThis PR adds CDB diffing and reload preservation, a ChangesFile Tracker and CDB Diff Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3011e3bc24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/server/workspace/workspace.cpp (1)
213-221: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSubdirectory scan order is filesystem-dependent, making CDB discovery non-deterministic.
When more than one direct subdirectory contains a
compile_commands.json(e.g.build/andbuild-debug/),directory_iteratororder isn't guaranteed, so the discovered path can differ across OSes/filesystems or even between runs. Consider collecting candidates and picking deterministically (e.g., lexicographic sort, or preferring a directory literally namedbuild).♻️ Proposed fix for deterministic ordering
- std::error_code ec; - for(llvm::sys::fs::directory_iterator it(workspace_root, ec), end; it != end && !ec; - it.increment(ec)) { - if(it->type() == llvm::sys::fs::file_type::directory_file) { - if(auto found = try_candidate(it->path()); !found.empty()) { - return found; - } - } - } - return {}; + std::error_code ec; + std::vector<std::string> subdirs; + for(llvm::sys::fs::directory_iterator it(workspace_root, ec), end; it != end && !ec; + it.increment(ec)) { + if(it->type() == llvm::sys::fs::file_type::directory_file) { + subdirs.push_back(it->path()); + } + } + llvm::sort(subdirs); + for(auto& dir: subdirs) { + if(auto found = try_candidate(dir); !found.empty()) { + return found; + } + } + return {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/workspace/workspace.cpp` around lines 213 - 221, The subdirectory scan in the workspace CDB lookup is non-deterministic because llvm::sys::fs::directory_iterator does not guarantee a stable order. Update the logic around the workspace_root scan and try_candidate to first collect all direct subdirectory candidates, then choose one deterministically before returning a compile_commands.json path. Prefer a stable rule such as lexicographic ordering or an explicit preference for a directory named build so workspace discovery behaves consistently across runs and filesystems.src/server/service/lsp_client.cpp (2)
498-522: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueTest-only endpoint is unconditionally registered/reachable.
clice/internal/pollis wired intoregister_extensions()with no build-time or runtime gate, so any connected client can invoke it in production, despite the comment marking it "TEST-ONLY... not a stable API". LSP method dispatch isn't restricted to advertised capabilities, so this is reachable outside test harnesses.Consider gating registration behind a debug/test-only build flag or an opt-in server option so it isn't part of the shipped surface.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/service/lsp_client.cpp` around lines 498 - 522, The test-only `clice/internal/poll` request is being registered unconditionally in `register_extensions()`, which exposes it to production clients. Update the registration around `peer.on_request("clice/internal/poll", ...)` so it only happens behind a debug/test build flag or an explicit opt-in server option. Keep the handler itself unchanged, but ensure `LspClient`/extension setup does not advertise or accept this endpoint unless the test-only gate is enabled.
498-522: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPotential reentrancy hazard: internal poll hook vs. live background poll loop.
tick_workspace()yields internally between stat batches, andFileTracker(per its header) keeps no reentrancy guard around itsbaselinemap. If a client invokesclice/internal/pollwithloop="workspace"whileworkspace_poll_task()is concurrently mid-sweep (e.g. background polling is enabled, as intest_cdb_polling_loop_live), the two coroutines could interleave and corrupt/duplicate tracker state. The integration test suite avoids this by disabling polling intervals whenever the poll hook is used, but nothing in the code itself prevents concurrent invocation.Consider adding a simple in-flight guard in
FileTracker(e.g., a bool flag rejecting/serializing overlappingtick_workspace()/tick_cdb()calls) for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/service/lsp_client.cpp` around lines 498 - 522, The internal poll handler in lsp_client.cpp can overlap with the background workspace polling loop and reenter FileTracker state updates. Add a simple in-flight guard in FileTracker around tick_workspace() and tick_cdb() so overlapping calls are rejected or serialized, and make the clice/internal/poll request path honor that guard when dispatching on loop="workspace" or loop="cdb". Use the existing tick_workspace, tick_cdb, and workspace_poll_task flow to locate the call sites and keep the baseline/tracker state safe from concurrent access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/workspace/file_tracker.cpp`:
- Around line 111-149: FileTracker::tick_workspace currently yields between
batches, so concurrent invocations can interleave and mutate the shared baseline
twice during the same sweep. Add an in-flight guard in tick_workspace() (or
equivalent serialization around workspace_poll_task callers) so only one sweep
runs at a time; if a second call arrives while one is active, skip or queue it
rather than touching baseline concurrently.
In `@src/server/workspace/invalidator.cpp`:
- Around line 199-227: The CDBChanged handling in invalidator.cpp performs
scan_dependency_graph() synchronously inside MasterServer::dispatch(), which
blocks the event loop and stalls request handling. Move the workspace rescan
work out of the dispatch path for the CDBChanged case—e.g. schedule it on a
separate worker/task queue or another serialized background path—then apply the
dep_graph, path_to_module, and context_epoch updates only after the scan
completes.
- Around line 182-198: The DiskRemoved handling in invalidator::apply is
rebuilding the entire dependency reverse map on every single removal, which
makes batched deletions too expensive. Update the FileEvent::Kind::DiskRemoved
case to only call workspace.dep_graph.clear_includes(path_id) and mark the
reverse map dirty, then trigger workspace.dep_graph.build_reverse_map() once
after the event loop in apply() when any removals occurred. Use the existing
workspace.dep_graph and dirty handling in apply to preserve correctness while
avoiding repeated full rebuilds.
In `@tests/unit/server/file_tracker_tests.cpp`:
- Around line 88-144: In WorkspaceTickStateMachine, the rewrite cases still use
same-size content, so the FileTracker fast path may miss the change when mtime
does not advance enough. Update the test around tick_workspace() to force a
detectable rewrite by using different-length payloads for the
changed/touched/reborn writes, or by waiting at least MTIME_GRANULARITY before
rewriting, so the DiskChanged and DiskRemoved behavior is exercised reliably.
---
Nitpick comments:
In `@src/server/service/lsp_client.cpp`:
- Around line 498-522: The test-only `clice/internal/poll` request is being
registered unconditionally in `register_extensions()`, which exposes it to
production clients. Update the registration around
`peer.on_request("clice/internal/poll", ...)` so it only happens behind a
debug/test build flag or an explicit opt-in server option. Keep the handler
itself unchanged, but ensure `LspClient`/extension setup does not advertise or
accept this endpoint unless the test-only gate is enabled.
- Around line 498-522: The internal poll handler in lsp_client.cpp can overlap
with the background workspace polling loop and reenter FileTracker state
updates. Add a simple in-flight guard in FileTracker around tick_workspace() and
tick_cdb() so overlapping calls are rejected or serialized, and make the
clice/internal/poll request path honor that guard when dispatching on
loop="workspace" or loop="cdb". Use the existing tick_workspace, tick_cdb, and
workspace_poll_task flow to locate the call sites and keep the baseline/tracker
state safe from concurrent access.
In `@src/server/workspace/workspace.cpp`:
- Around line 213-221: The subdirectory scan in the workspace CDB lookup is
non-deterministic because llvm::sys::fs::directory_iterator does not guarantee a
stable order. Update the logic around the workspace_root scan and try_candidate
to first collect all direct subdirectory candidates, then choose one
deterministically before returning a compile_commands.json path. Prefer a stable
rule such as lexicographic ordering or an explicit preference for a directory
named build so workspace discovery behaves consistently across runs and
filesystems.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 075024d0-9f87-4263-9efd-7a75b56a0dab
📒 Files selected for processing (23)
docs/clice.tomlsrc/command/command.cppsrc/command/command.hsrc/server/protocol/extension.hsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/service/master_server.hsrc/server/workspace/config.cppsrc/server/workspace/config.hsrc/server/workspace/file_tracker.cppsrc/server/workspace/file_tracker.hsrc/server/workspace/invalidator.cppsrc/server/workspace/invalidator.hsrc/server/workspace/workspace.cppsrc/server/workspace/workspace.hsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.htests/integration/features/test_file_tracker.pytests/integration/features/test_header_reindex.pytests/integration/utils/client.pytests/unit/command/cdb_diff_tests.cpptests/unit/server/file_tracker_tests.cpptests/unit/server/invalidator_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b22e1d433
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/context/context_resolver.cpp (2)
941-953: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate hash-validation lambda; not migrated to the new shared helper.
has_commandhere reimplementsContextResolver::entry_has_hashverbatim. Since this PR's stated purpose is to introduce shared command-hash validation, this call site should use the new member instead of its own local copy to avoid the two implementations drifting apart.Proposed fix
- // Validate that `hash` names a real CDB entry of `entry_path`. - auto has_command = [&](llvm::StringRef entry_path, llvm::StringRef hash) { - if(!ws.cdb.has_entry(entry_path)) { - return false; - } - std::vector<std::string> rule_append, rule_remove; - ws.config.match_rules(entry_path, rule_append, rule_remove); - for(auto& cmd: ws.cdb.lookup(entry_path, {.remove = rule_remove, .append = rule_append})) { - if(canonical_command_hash(cmd.to_string_argv(), cmd.resolved.directory) == hash) { - return true; - } - } - return false; - }; + // Validate that `hash` names a real CDB entry of `entry_path`. + auto has_command = [&](llvm::StringRef entry_path, llvm::StringRef hash) { + return ws.cdb.has_entry(entry_path) && entry_has_hash(entry_path, hash); + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/context/context_resolver.cpp` around lines 941 - 953, The local hash-checking lambda in ContextResolver is duplicating the new shared command-hash logic instead of using it directly. Replace the inline has_command implementation with a call to ContextResolver::entry_has_hash so the lookup and canonical hash comparison stay centralized, and remove the duplicated rule_append/rule_remove and lookup logic from this call site.
721-750: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMirror the CDB-entry guard for host-pinned choices.
drop_orphaned_choices()can keep a host pin alive when the host has been removed from the CDB but is still reachable through some include chain, especially in the common emptycommand_hashcase. Add the samews.cdb.has_entry(host_path)check used byvalidate_saved_context()before accepting the host branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/context/context_resolver.cpp` around lines 721 - 750, The host-pinned branch in ContextResolver::drop_orphaned_choices should mirror the CDB-entry validation used in validate_saved_context(). Add the same workspace.cdb.has_entry(host_path) guard before treating a host-pinned saved context as valid, especially when saved.command_hash is empty, so host-pinned choices are dropped if the host entry no longer exists even if an include chain still reaches it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/context/context_resolver.cpp`:
- Around line 941-953: The local hash-checking lambda in ContextResolver is
duplicating the new shared command-hash logic instead of using it directly.
Replace the inline has_command implementation with a call to
ContextResolver::entry_has_hash so the lookup and canonical hash comparison stay
centralized, and remove the duplicated rule_append/rule_remove and lookup logic
from this call site.
- Around line 721-750: The host-pinned branch in
ContextResolver::drop_orphaned_choices should mirror the CDB-entry validation
used in validate_saved_context(). Add the same
workspace.cdb.has_entry(host_path) guard before treating a host-pinned saved
context as valid, especially when saved.command_hash is empty, so host-pinned
choices are dropped if the host entry no longer exists even if an include chain
still reaches it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0abf54a4-8a70-44db-9926-f6b48635b3af
📒 Files selected for processing (9)
src/command/command.cppsrc/command/command.hsrc/server/context/context_resolver.cppsrc/server/context/context_resolver.hsrc/server/service/master_server.cppsrc/server/workspace/file_tracker.cppsrc/server/workspace/invalidator.cpptests/unit/command/cdb_diff_tests.cpptests/unit/command/command_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- src/server/context/context_resolver.h
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/command/cdb_diff_tests.cpp
- src/server/workspace/file_tracker.cpp
- src/command/command.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b19e3fb3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
clice now notices changes the client never tells it about. A new
FileTrackerpolls the filesystem with plainstat(portable, no fd limits, no event storms — the same trade-off clangd makes) and feeds the existing event-driven invalidation engine:compile_commands.json— including the case where none exists yet at startup and the build system generates one later. A change must stay stable for two consecutive ticks before the database is reloaded (half-written-file guard). The reload diffs entries by canonical command hash and emits oneCDBChangedevent with the added/removed/changed file sets.DiskChangedis emitted, sotouchstays silent; a stat failure on a known file emitsDiskRemovedonce. The first sweep only seeds the baseline (no startup storm).Polling only marks dirty and emits events — it never needs to be complete. A missed change means derived state stays stale for one more poll period at worst; correctness is anchored by the pull side's two-layer deps validation at compile/index time.
User-visible behavior
compile_commands.jsonwhile the server runs (add/remove entries, change flags) takes effect without a restart; a database generated after startup is discovered automatically ("opened the editor before running cmake" now works).git checkout, code generators) are picked up: open files get correct diagnostics on their next request, closed files are reindexed.touch(mtime-only change) does not trigger reindexing.didSave), the file is additionally treated as a disk change, removing the "I see my old buffer, my dependents see the new disk" split.Configuration
New
[tracker]section inclice.toml:Notes
CDBChangedwith a uniform full dependency-graph rescan rather than per-entry incremental surgery: entry additions, removals and flag changes all funnel into one code path, and since CDB changes are the only rescan trigger, a persistent scan cache would never be warm anyway.-g,-fPIC, ...) deliberately do not count as changes;-O*does (it defines__OPTIMIZE__).DiskRemovedis conservative: the file loses its includer role and context choices through it are re-checked, but index shards are kept so last-known content still serves navigation.didChangeWatchedFiles) can later be added as just another event producer; deliberately out of scope here, as are directory-level new-file discovery and moving stats off the event loop (perf log line records sweep duration to decide if/when that is needed).