Repository navigation
refactor(server): interest-counted module compile cancellation - #454
Merged
Merged
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:
📝 WalkthroughWalkthroughCompileGraph replaces recursive compile_impl with per-unit Round objects, RAII guards, and refcounted interest; Compiler/Session add generation-scoped dependency cancellation and wiring; tests migrate to shared event-loop harness and gain extensive deterministic cancellation/concurrency coverage. ChangesCompilation Engine Refactoring
Compiler Integration and Generation Tracking
Test Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Graph as CompileGraph
participant Task as unit_task/unit_body
participant Dispatch as dispatch_fn
Client->>Graph: compile(path_id) / compile_deps(path_id)
Graph->>Graph: acquire refs, spawn_unit
Graph->>Task: schedule on event loop
Task->>Dispatch: dispatch_fn(path_id)
Dispatch-->>Task: compile result / error / cancellation
Task->>Graph: publish Outcome, release refs, signal completion
Graph-->>Client: await_unit returns (Success/Failure/Stale)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/server/compile_graph_tests.cpp`:
- Around line 300-303: Tests CancelAll, UpdateUnknownPathId, and
EmptyGraphNoCompile bypass the harness shutdown contract by calling
graph->cancel_all() (and similar) directly instead of exercising execute(...),
so they skip the required shutdown()+idle() teardown; update these tests to
follow the suggested pattern: after creating the graph with make_graph(...),
invoke the graph operation (e.g., graph->cancel_all()), then call
graph->shutdown() and wait for idle() (or use the harness-provided teardown
helper) to validate the proper shutdown sequence, ensuring the test hits the
shutdown()+idle() verification instead of exiting early; apply the same change
to the other occurrences referenced around the file (lines ~625-639) for
consistency.
- Around line 112-115: In make_graph, avoid destroying the old loop while the
old CompileGraph still holds it: first clear the existing graph (reset or clear
graph) before touching the loop, then reset the loop, then emplace the loop and
finally emplace graph using *loop and std::move(dispatch)/std::move(resolve);
reference make_graph, loop, graph, and CompileGraph to locate and change the
order to graph.reset() -> loop.reset() -> loop.emplace() -> graph.emplace(*loop,
std::move(dispatch), std::move(resolve)).
🪄 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: 2dec855f-ffc0-4cb7-be23-6f51f1a9cad6
📒 Files selected for processing (8)
src/server/compiler/compile_graph.cppsrc/server/compiler/compile_graph.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/service/session.htests/unit/server/compile_graph_integration_tests.cpptests/unit/server/compile_graph_tests.cpp
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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: c4b6febfda
ℹ️ 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: 381521e88b
ℹ️ 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".
Replace token-tree cancellation in CompileGraph with edge reference counting. Each dirty unit now compiles in an independent task owned by the graph's task_group, cancellable only through its own round token; requesters and dependent units wait on a per-round completion event. - CompileUnit grows an in-flight interest count: requests hold root references, running unit tasks hold edge references on their direct dependencies. A unit whose interest drops to zero mid-compile is cancelled; shared dependencies survive as long as any consumer holds interest. - update() (staleness) is now separate from request cancellation: it dirties the transitive dependents and cancels their rounds without touching interest; waiters observe the stale round and retry. - Rounds publish a three-state outcome (success / failed / stale): failures (compile errors, dependency cycles) propagate to waiters without retry, stale rounds are respawned by surviving waiters. - All unit bookkeeping moves into a scope guard established before the first suspension point, since kotatsu cancellation unwinds frames without resuming them. - Cycle detection degenerates to per-wait-edge checks (BFS through compiling units back to the waiting unit) plus a self-dependency check after resolve. - Compiler::ensure_compiled supersedes a stale in-flight compile: the replacement is spawned before the old request scope is cancelled, so shared module dependencies never lose all interest across an import switch. - CompileGraph::shutdown() implements the structured two-step teardown (cancel + join), wired into Compiler::stop().
- run_compile: re-check the session generation before send_stateful so a superseded compile never sends stale text after its replacement (the worker applies compiles in arrival order without a version check). - ensure_deps: bail out after the buffer-scan waits when the request scope was cancelled, instead of marching on to the PCH build. - Tests: cover compile_deps request cancellation (root refs on direct deps released, shared dep survives), cancel_all respawn behavior, an asymmetric-depth shared dependency, and strengthen the cascade-cancel assertions in UpdateDepCascadesCancel. - Style: rename SharedDepFailureFailsBoth to SharedDepFailsBoth, drop a dead counter in RandomizedStress, replace decorative banners with one-line comments, document the task_group frame-retention trade-off.
The supersede path spawned a replacement run_compile for every feature request that observed a stale in-flight compile. For sessions without module dependencies the old compile had already sent its text to the worker (the send is not cancellable), so rapid edits piled up one queued worker compile per edit instead of coalescing into a single follow-up at the latest generation — slow CI runners (macOS Debug) timed out draining the backlog in the rapid_edit smoke replay. Track deps_done on PendingCompile and supersede only while the stale compile still holds interest in the module graph; past that point waiting is strictly better and restores the previous coalescing.
Move ModuleTestEnv, the event loop and the graph into suite members (zest builds a fresh suite object per case), removing the per-case construction boilerplate from all 26 integration cases. The custom resolver/dispatch cases now delegate to the suite defaults instead of duplicating their logic. New cases: SharedDepSequentialCancel verifies both cancellation directions stepwise (refcount 2 -> 1 -> 0, the shared unit survives the first cancel and dies on the last); UpdateSwapsDeps changes a unit's import set while its round is in flight and verifies the retry re-resolves, the orphaned dependency is released without restart and its back-edge is fully detached.
When a unit's import set changes mid-compile (deps {B,C} -> {B,E}),
the stale round's exit released its edge references before the retry
could re-acquire them, so the retained dependency B transited through
zero interest and was cancelled and restarted from scratch — wasted
work for a result that was never stale.
Defer the zero-interest decision by one event-loop tick instead of
cancelling on the spot: synchronous re-acquisition (the retry
respawning after update, the supersede handoff) happens within the
current drain cycle, strictly before the deferred check fires, so
still-wanted in-flight compilations are handed over at any cascade
depth; only a sustained zero cancels. Orphaned dependencies (C) are
still cancelled, one tick later.
Tests: UpdateKeepsRetainedDep covers the mixed retained/orphaned case;
UpdateWhileWaitingDeps now asserts the handover (single dispatch)
instead of documenting the restart; cancellation-completion assertions
poll via a bounded settle() helper since cascades now span one tick
per level. Also covers compile_deps root release with a shared dep.
Reorganize both compile graph test files into semantic sections (basic compilation, compile_deps, staleness marking, update vs in-flight rounds, failure, cycles, shared dependencies, lifecycle, stress), each case opening with a comment stating the scenario and the guarantee it verifies. Rename every case from CamelCase to snake_case, replace the decorative banner separators with plain one-line comments, and fold two redundant cases into their supersets (dispatch_failure into failure_leaves_dirty, cancel_all_idle into empty_graph).
Use the project's /// banner style for the section headers of both graph test files. New cases close review-identified gaps: - concurrent_requests_share_round / duplicate_requests_cancel_one: multiple requests on one unit share a single round, and cancelling one of them does not disturb the round the other waits on. - rerequest_within_grace: release-then-reacquire within one drain cycle — the strictest transient-zero ordering — keeps the in-flight round alive via the deferred zero-interest check. - shared_dep_failure_propagates: one failing round fails every consumer chain, dispatched once. - shared_dep_update_retries: updating a shared dependency makes both consumer chains retry onto one shared fresh round (two dispatches total, not three). The drivers of the two new multi-request cases suspend once before finishing: a when_all child completing synchronously during the arm phase trips a when_any bookkeeping assert downstream — a kotatsu bug to be fixed upstream, documented at the workaround sites.
16bit-ykiko
force-pushed
the
refactor/module-cancellation
branch
from
June 13, 2026 15:12
2509342 to
d46920f
Compare
3 tasks done
This was referenced Jul 2, 2026
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
clice compiles C++20 module dependencies through
CompileGraph: each module interface unit is a node,resolvelazily discovers its imports, anddispatchbuilds the PCM. Overlapping dependency closures are the normal case — two open files importing different modules that share a common dependency.The previous cancellation model propagated cancellation through a token tree: a requester wrapped every dependency task in
with_token(...)of its own source, recursively, and dependency compilations ran nested inside the requester's coroutine frames. That model has a structural mismatch with the problem:A → B → EandC → D → E, E's compilation runs inside whichever request got there first — say A's. Cancelling the A request tears down its whole coroutine tree and kills E's in-flight build, even though the C request still needs it. There was no notion of "who else is interested in E".source->cancel()insideupdate(), entangled with dirty-marking.Design
The core shift: execution is decoupled from interest.
Execution — each dirty unit compiles in an independent task spawned into a graph-owned
task_group, cancellable only through its own per-round token. Requesters and dependent units no longer own child coroutine frames; they wait on a per-round completion event. A requester dying implies nothing, by itself, about the units it was waiting on.Interest — an explicit per-unit count of in-flight demand: a request holds a root reference on the unit(s) it asked for; a running unit task holds an edge reference on each direct dependency. A unit whose count stays at zero for one event-loop tick while compiling has its round cancelled. The one-tick deferral matters: an interest drop is often transient — a stale round's edges being re-acquired by the retry that re-resolves it — and synchronous re-acquisition always lands within the same drain cycle, strictly before the deferred check fires. So when a unit's import set changes from
{B, C}to{B, E}mid-compile, the retained B is handed over to the new round (neither cancelled nor restarted, at any cascade depth), while the orphaned C is cancelled one tick later. Cancellation cascades structurally — the dying task's guard releases its edge references, which may zero out further dependencies — and stops exactly where shared interest remains: cancel the A request and A, B unwind while E drops 2 → 1 and keeps compiling.Edge references were chosen over counting each request's transitive closure deliberately: the closure is lazily discovered (
resolveruns on first compile), so closure-counting would need per-request acquired-sets with incremental backfill as resolution proceeds. Edge counting needs zero global bookkeeping and yields the same outcomes.The two cancellation reasons are now distinct operations:
update()): mark the unit and its transitive dependents dirty, bump their generation, cancel their in-flight rounds unconditionally — interest untouched. "The source changed; the results are garbage, but everybody still wants them."What a waiter does next is decided by a three-state round outcome rather than a boolean:
update(), no further retry.Cancellation safety
kotatsu cancels by frame destruction without resumption: a cancelled coroutine never executes another statement past its suspension point — only destructors of already-constructed locals run. All per-round bookkeeping therefore lives in a single scope guard constructed before the unit task's first suspension: publish the outcome (default stale — the cancel path can't reach an explicit assignment), clear the compiling flag, release acquired edge references (possibly cancelling zero-interest dependencies — a synchronous primitive, safe in a destructor), fire the completion event. Edge acquisition is likewise fully synchronous, so no suspension can strand a half-registered reference.
Two kotatsu behaviors shaped the implementation and are worth knowing for review:
task_groupis fail-fast structured concurrency: one child finishingCancelledaborts every sibling and permanently blocks new spawns. Since a cancelled round is the normal case here, rounds are spawned through a thin wrapper that converts the cancellation into a value, so every child finishesFinished. (Found empirically by the shared-dependency tests: "cancelling A killed C's entire chain".)task_groupreclaims child frames only at destruction, so frames accumulate one per round until shutdown — the same trade-off as the compiler's existing AST-compile group; documented in code as a candidate kotatsu-side improvement.The import-switch handoff
The headline scenario: a file says
import A, the build is in flight, the user switches toimport C, and both chains share E. The old request's interest must go away — but if it disappears before the new request registers its own, E transits through zero and is killed and restarted anyway.ensure_compiledtherefore supersedes a stale in-flight compile in a fixed order: spawn the replacement first — it descends the new dependency chain and acquires interest synchronously (spawned tasks run to their first suspension immediately, while cancellation only lands on the next event-loop tick) — then cancel the superseded request's dependency scope. Interest on shared units overlaps across the swap; E never sees zero.Reviewing this path surfaced two ordering hazards, both fixed here: a superseded compile re-checks the session generation before sending text to the stateful worker (the worker applies compiles in arrival order with no version check, so a stale send landing after the replacement would leave the worker on old text), and a superseded round abandons its remaining preparation instead of continuing into the PCH build.
Cycle handling
The old ancestor-set threading cannot survive the move to independent tasks — there is no call chain to thread it through. Cycles are caught where they manifest: a self-import directly after resolve, and a would-deadlock wait — before blocking on a dependency, the waiter walks currently-compiling units' dependency edges to check whether the chain leads back to itself. Detected cycles fail the unit (no retry), including cycles that only appear after
update()re-resolves a changed import set. Surfacing them as file-level diagnostics is follow-up work.Testing
The strategy is deterministic cancellation injection: a manually gated mock dispatch parks any unit mid-dispatch, the test observes interest counts at that instant, injects a request cancellation / update / shutdown, and asserts which compilations survived — by liveness, refcount, and dispatch-call counts, not just final success. Every test tears down through
shutdown()and asserts the graph is fully quiesced (zero refcounts, no compiling residue, every completion fired); a fixed-seed randomized stress run interleaves compiles, cancellations, updates and completions with per-step structural checks.Two integration cases run the headline scenarios against real clang module builds: switching the import target neither kills nor restarts the shared dependency (one dispatch, one PCM), and a shared dependency failing fails both consumers from a single round. Unit cases additionally pin down both cancellation directions of a shared dependency (stepwise refcount 2 → 1 → 0) and the in-flight import-set change (retained dependency handed over with a single dispatch, orphan cancelled and fully detached).
All pre-existing cases are preserved. One changes meaning intentionally: a file update during compilation used to fail the waiting request; it now retries once and succeeds, asserted by exact dispatch counts.
Verified locally on Debug and RelWithDebInfo: 615 unit / 172 integration / 2 smoke tests passing.
Summary by CodeRabbit
Refactor
New Features
Tests