Repository navigation
fix(server): eviction recovery and conditional dirty-flag clearing - #490
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesEviction cap, epoch-based staleness, and centralized session reset
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/server/compiler/compiler.cpp (1)
823-835: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
on_stalebefore invoking it.Compiler::Compiler(...)doesn’t accept this callback, and there’s no wiring in this tree, soon_stale(path_id)can throwstd::bad_function_callon the stale path. Either guard it likeon_indexing_neededor make it a required constructor dependency.🤖 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/compiler/compiler.cpp` around lines 823 - 835, The stale-path callback in Compiler::needs_refresh (or the surrounding stale-check block) can throw because on_stale is invoked without verifying it is set. Add the same kind of guard used for on_indexing_needed before calling on_stale(path_id), or make on_stale an обязательный constructor dependency in Compiler::Compiler(...) and wire it through the class so the callback is always valid.
🧹 Nitpick comments (1)
tests/integration/stress/test_eviction.py (1)
1-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest implicitly coupled to
default_max_documents.
FILE_COUNT = 18and the "16" in the comment hardcode the worker's default cap. Consider explicitly settingstateful_worker_count's companion--max-documents(if exposed via config) or documenting the coupling more strongly, so a future change todefault_max_documentsdoesn't produce a confusing failure here.🤖 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 `@tests/integration/stress/test_eviction.py` around lines 1 - 7, The stress test is implicitly tied to the worker document cap, so update test_eviction to avoid hardcoding the default limit. In the test setup around FILE_COUNT and the LRU-cap comment, either pass an explicit max-documents setting alongside stateful_worker_count or strengthen the test description to clearly state the dependency on default_max_documents. Use the FILE_COUNT constant and the worker configuration used by this test to keep the expectation stable if the default changes.
🤖 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/compiler/compiler.cpp`:
- Around line 823-835: The stale-path callback in Compiler::needs_refresh (or
the surrounding stale-check block) can throw because on_stale is invoked without
verifying it is set. Add the same kind of guard used for on_indexing_needed
before calling on_stale(path_id), or make on_stale an обязательный constructor
dependency in Compiler::Compiler(...) and wire it through the class so the
callback is always valid.
---
Nitpick comments:
In `@tests/integration/stress/test_eviction.py`:
- Around line 1-7: The stress test is implicitly tied to the worker document
cap, so update test_eviction to avoid hardcoding the default limit. In the test
setup around FILE_COUNT and the LRU-cap comment, either pass an explicit
max-documents setting alongside stateful_worker_count or strengthen the test
description to clearly state the dependency on default_max_documents. Use the
FILE_COUNT constant and the worker configuration used by this test to keep the
expectation stable if the default changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08667415-ef3d-4e5a-90be-40c05c5ad237
📒 Files selected for processing (20)
src/clice.ccsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/context_resolver.cppsrc/server/state/invalidator.cppsrc/server/state/invalidator.hsrc/server/state/session.hsrc/server/state/session_store.cppsrc/server/state/session_store.hsrc/server/transport/lsp_client.cppsrc/server/transport/master_server.cppsrc/server/transport/master_server.hsrc/server/worker/stateful_worker.cppsrc/server/worker/stateful_worker.htests/integration/features/test_server.pytests/integration/stress/test_eviction.pytests/unit/server/invalidator_tests.cpptests/unit/server/session_store_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/worker_test_helpers.h
![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: a024822655
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd666390d7
ℹ️ 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".
bfdba30 to
6998113
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6998113bb8
ℹ️ 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 (1)
src/server/compiler/compiler.cpp (1)
869-876: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
on_stalebefore invoking it.Compiler::on_staleis a plainstd::function, so a directCompilerinstance can leave it empty; calling it here will fail withstd::bad_function_callif the stale path is hit. Add a no-op default or anif(on_stale)check first.🤖 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/compiler/compiler.cpp` around lines 869 - 876, The stale-path handler in Compiler::dispatch is invoked unconditionally even though on_stale is a plain std::function and may be empty on a direct Compiler instance. Add a guard before calling on_stale(path_id) or initialize on_stale with a no-op default so the stale-path flow in Compiler::on_stale cannot throw std::bad_function_call when unset.
🧹 Nitpick comments (1)
tests/unit/server/compiler_tests.cpp (1)
26-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for a post-suspension epoch guard.
This test bumps
dirty_epochbeforeensure_pch()starts, so it only exercises the entry guard. Please add a case wheredirty_epochchanges while waiting on an in-flight PCH or after BuildPCH returns, beforepch_refassignment.🤖 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 `@tests/unit/server/compiler_tests.cpp` around lines 26 - 64, This test only covers the early dirty_epoch check in CompilerFixture::ensure_pch, so extend it to verify the post-suspension epoch guard as well. Add a scenario where Session::dirty_epoch changes after the coroutine has already started waiting on the in-flight PCH or immediately after BuildPCH returns but before pch_ref is written, and assert the continuation exits without updating session.pch_ref. Use the existing ensure_pch/CompilerFixture path and keep the stale-session expectations explicit.
🤖 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/compiler/compiler.cpp`:
- Around line 869-876: The stale-path handler in Compiler::dispatch is invoked
unconditionally even though on_stale is a plain std::function and may be empty
on a direct Compiler instance. Add a guard before calling on_stale(path_id) or
initialize on_stale with a no-op default so the stale-path flow in
Compiler::on_stale cannot throw std::bad_function_call when unset.
---
Nitpick comments:
In `@tests/unit/server/compiler_tests.cpp`:
- Around line 26-64: This test only covers the early dirty_epoch check in
CompilerFixture::ensure_pch, so extend it to verify the post-suspension epoch
guard as well. Add a scenario where Session::dirty_epoch changes after the
coroutine has already started waiting on the in-flight PCH or immediately after
BuildPCH returns but before pch_ref is written, and assert the continuation
exits without updating session.pch_ref. Use the existing
ensure_pch/CompilerFixture path and keep the stale-session expectations
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 229bbe12-e43c-4e4b-b5dc-8136f1aa321f
📒 Files selected for processing (3)
src/server/compiler/compiler.cppsrc/server/compiler/compiler.htests/unit/server/compiler_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/compiler/compiler.h
1 participant
Motivation
Three related state-consistency holes around the same question — when may a compile result claim freshness?
generationon some of these paths, which conflated "the buffer changed" with "the world got dirtier" and made still-valid in-flight results get discarded.Changes
dirty_epochtoken. Every AST-invalidating dispatch effect bumps it; a compile snapshots it at takeoff and clearsast_dirtyon landing only if it is unchanged (Session::settle_compile— now the only way to clear the flag).generationreturns to pure buffer identity; the two dispatch-sidegenerationbumps are reverted. Results that still match the buffer are published (bounded staleness) while the flag stays dirty, so the next request recompiles.DocumentEvictedevent whose effect is the same "AST lost, recompile" treatment as a worker crash. The worker's document cap is injectable via--max-documents(default 16, unchanged).SessionStore::reset_compile_state(Session&, ResetDepth)replaces three hand-copied reset blocks (context switch, orphaned context choices, dispatch effect loops):Supersededbumps generation and drops PCH ref / deps snapshot / trial verdict;Lostbumpsdirty_epochonly.DiskChangedevent (synchronously) instead of hand-rolling the same session reset, so it shares the file tracker's invalidation path. Behavior for open files is unchanged.ContextChangedremoved. The event had an empty handler and existed only as ceremony; the criterion for when logic may bypass the event pipeline is now documented in the invalidator header.close_sessionno longer takes a peer. Diagnostics retraction goes through the session's output plus theon_outputsignal, like every other publish; the server core stays transport-free. Side effect: closing a file that was never opened no longer pushes an empty-diagnostics notification.ensure_pchre-checks its generation snapshot before writingsession.pch_refafter each suspension point, so a superseded round cannot write back a stale reference.Tests
--max-documents 2.Local verification: format, RelWithDebInfo build, full unit suite (848 passed), targeted integration subset (47 passed incl. file tracker, context switching, rapid edit, eviction), smoke tests 3/3.
Summary by CodeRabbit
--max-documentsto configure the compiled-document eviction cap for stateful workers (default included).