feat(server): worker-side dependency hashing and module import edges - #492
feat(server): worker-side dependency hashing and module import edges#49216bit-ykiko wants to merge 2 commits into
Conversation
Workers now hash every dependency from the buffers the compilation
actually consumed and report {path, hash} plus the compile start time;
the master adopts these snapshots instead of re-reading disk at result
arrival, so an artifact can never be blessed as built from content it
never saw. TUIndex ships the same hashes, and the index merge skips
files whose disk diverged from the consumed content, re-enqueueing the
TU instead of storing a corrupt pairing.
C++20 import edges now reach all three invalidation mechanisms: worker
dep reports, index shard dependencies, and a scan-built module-to-
importers reverse map that lets a saved interface recompile open
importers and reindex closed ones that never compiled this session.
Background indexing builds an importer's module PCMs before indexing
it, and a pre-existing stateless-worker crash on indexing any module
importer (unknown FileIDs handed to the include graph) is fixed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds hashed dependency metadata and build timestamps to compile, worker, and snapshot flows. Extends TU indexing and merge staleness checks with path hashes and imports. Adds module import tracking in the dependency graph, scanner, workspace rescans, and invalidation cascades. ChangesHashed Dependency Tracking and Module Import Propagation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Worker as StatelessWorker
participant Compiler
participant Workspace
participant Indexer
Worker-->>Compiler: BuildResult{deps: HashedDep[], build_at}
Compiler->>Workspace: capture_deps_snapshot(deps, build_at)
Workspace-->>Indexer: TUIndex / deps snapshot
Indexer->>Indexer: stale_against_worker(hash, disk content)
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 |
![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/indexer.cpp (1)
110-121: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMain-TU read failure never re-enqueues, unlike the header path.
When the main TU's on-disk content can't be read, the code returns before
stale_against_workerruns, soenqueue()is never called. The header branch below (line 149) reaches the same failure mode (empty content) but does fall through tostale_against_worker, which enqueues a redo. The comment here ("the staleness check re-enqueues it later") doesn't match what actually happens — nothing schedules a retry for this branch, so a transient read failure (lock contention, race with a build tool, NFS blip) leaves this TU's shard stuck on its old snapshot indefinitely.🔧 Proposed fix: re-enqueue on unreadable main file
if(!buf) { // Overwriting the shard's stored content with nothing would // silently break every position mapping for this TU; keep the // old snapshot, the staleness check re-enqueues it later. LOG_WARN("Skip merge for {}: cannot read content: {}", file_path, buf.getError().message()); touched.insert(global_path_id); + enqueue(file_ids_map[main_tu_path_id]); 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/compiler/indexer.cpp` around lines 110 - 121, The main-TU read failure path in indexer.cpp returns before stale_against_worker can run, so enqueue() is never triggered for unreadable content. Update the main-file branch around the file_path / llvm::MemoryBuffer::getFile handling so it follows the same retry behavior as the header path: preserve the old snapshot, but still route through the staleness check or explicitly call enqueue() when the file cannot be read. Keep the existing logging and touched.insert(global_path_id) behavior, but ensure a transient read error actually schedules a retry for this TU.
🧹 Nitpick comments (2)
src/compile/compilation_unit.cpp (2)
318-344: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
imported_module_sources()is invoked twice per compilation, doubling disk I/O.
deps()callsimported_module_sources()(line 305), andTUIndex::Builder's constructor (src/index/tu_index.cpp) independently callsunit.imported_module_sources()again for the sameCompilationUnitRef. Inhandle_build_pcm/handle_build_pch, bothunit.deps()andserialize_tu_index(unit, ...)run against the same unit, so every imported module interface source gets read from disk and re-hashed twice per build. Memoizing the result on the unit (e.g. caching inselfon first call) would avoid the redundant I/O.🤖 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/compile/compilation_unit.cpp` around lines 318 - 344, imported_module_sources() is doing the same disk reads and hashes twice per compilation because both deps() and TUIndex::Builder call it on the same CompilationUnitRef. Add memoization/caching inside CompilationUnitRef::imported_module_sources() (using self or another unit-owned cache) so the first call computes the vector and subsequent calls reuse it, and make sure handle_build_pcm/handle_build_pch benefits through deps() and serialize_tu_index() without re-reading module sources.
261-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird copy of the same content-hashing helper.
hash_file_contentduplicateshash_fileinsrc/index/merged_index.cpp(lines 93-101) and theworkspace.cpphash_filereferenced fromdeps_changed. All three implement byte-identicalxxh3_64bits(MemoryBuffer::getFile(path))logic. Since staleness correctness across worker/index/master layers depends on these staying in lockstep, consolidating into one shared helper (e.g. a smallsupportheader) removes the drift risk of one copy changing (algorithm, error handling) without the others.♻️ Suggested consolidation approach
// e.g. src/support/file_hash.h namespace clice { inline std::uint64_t hash_file_content(llvm::StringRef path) { auto buffer = llvm::MemoryBuffer::getFile(path); if(!buffer) return 0; return llvm::xxh3_64bits((*buffer)->getBuffer()); } }Then have
merged_index.cpp'shash_fileandworkspace.cpp'shash_filecall this shared helper instead of re-implementing it.🤖 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/compile/compilation_unit.cpp` around lines 261 - 267, The content-hashing logic is duplicated across `hash_file_content` in compilation_unit.cpp, `hash_file` in merged_index.cpp, and the workspace-side `hash_file` used by `deps_changed`, so consolidate this byte-identical `MemoryBuffer::getFile` + `xxh3_64bits` implementation into one shared helper (for example in a small support header) and update the existing callers to use it. Keep the shared helper as the single source of truth so any future change to hashing or error handling stays consistent across worker, index, and master paths.
🤖 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/indexer.cpp`:
- Around line 110-121: The main-TU read failure path in indexer.cpp returns
before stale_against_worker can run, so enqueue() is never triggered for
unreadable content. Update the main-file branch around the file_path /
llvm::MemoryBuffer::getFile handling so it follows the same retry behavior as
the header path: preserve the old snapshot, but still route through the
staleness check or explicitly call enqueue() when the file cannot be read. Keep
the existing logging and touched.insert(global_path_id) behavior, but ensure a
transient read error actually schedules a retry for this TU.
---
Nitpick comments:
In `@src/compile/compilation_unit.cpp`:
- Around line 318-344: imported_module_sources() is doing the same disk reads
and hashes twice per compilation because both deps() and TUIndex::Builder call
it on the same CompilationUnitRef. Add memoization/caching inside
CompilationUnitRef::imported_module_sources() (using self or another unit-owned
cache) so the first call computes the vector and subsequent calls reuse it, and
make sure handle_build_pcm/handle_build_pch benefits through deps() and
serialize_tu_index() without re-reading module sources.
- Around line 261-267: The content-hashing logic is duplicated across
`hash_file_content` in compilation_unit.cpp, `hash_file` in merged_index.cpp,
and the workspace-side `hash_file` used by `deps_changed`, so consolidate this
byte-identical `MemoryBuffer::getFile` + `xxh3_64bits` implementation into one
shared helper (for example in a small support header) and update the existing
callers to use it. Keep the shared helper as the single source of truth so any
future change to hashing or error handling stays consistent across worker,
index, and master paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f5313cb-afc1-4d3c-87ed-11dab6ec12b7
📒 Files selected for processing (34)
src/compile/compilation.cppsrc/compile/compilation.hsrc/compile/compilation_unit.cppsrc/compile/compilation_unit.hsrc/compile/hashed_dep.hsrc/index/include_graph.cppsrc/index/include_graph.hsrc/index/merged_index.cppsrc/index/merged_index.hsrc/index/schema.fbssrc/index/tu_index.cppsrc/index/tu_index.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/state/invalidator.cppsrc/server/state/workspace.cppsrc/server/state/workspace.hsrc/server/worker/stateful_worker.cppsrc/server/worker/stateless_worker.cppsrc/syntax/dependency_graph.cppsrc/syntax/dependency_graph.hsrc/syntax/scan.cpptests/integration/modules/test_module_reindex.pytests/unit/compile/compilation_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/server/deps_snapshot_tests.cpptests/unit/server/indexer_merge_tests.cpptests/unit/server/invalidator_tests.cpptests/unit/server/module_worker_tests.cpptests/unit/syntax/dependency_graph_tests.cpptests/unit/syntax/scan_tests.cpp
![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: 4c03f610ec
ℹ️ 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".
| if(stale_against_worker(tu_path_id, header_content, header_path)) { | ||
| touched.insert(global_path_id); | ||
| return; |
There was a problem hiding this comment.
When this detects that a header's disk content no longer matches what the worker indexed, return only exits merge_file_index; the outer loop continues and can still merge the main file or other headers from the same stale TU. Since any changed header can alter the preprocessor state for the entire compilation, publishing the remaining shards can serve rows built from old content until the re-enqueued round finishes; this should skip the whole TU result before merging any shard.
Useful? React with 👍 / 👎.
| auto module_it = workspace.path_to_module.find(seed); | ||
| if(module_it == workspace.path_to_module.end()) { | ||
| continue; |
There was a problem hiding this comment.
This lookup runs after rescan_after_save has already overwritten or erased path_to_module for the saved file, so it only walks importers of the new module name. If an interface changes from export module m; to another name, or removes the module declaration, closed TUs that import m are never reached by importers_of("m") and keep stale index/PCM state; preserve the pre-rescan module name (or otherwise traverse the stale provider entry) when cascading.
Useful? React with 👍 / 👎.
| if(tok.is(clang::tok::raw_identifier)) { | ||
| name += content.substr(tok.Offset, tok.Length); |
There was a problem hiding this comment.
For an import spelled through a macro, e.g. #define MY_IMPORT dep followed by import MY_IMPORT;, this records MY_IMPORT as the imported module name in the scan-built graph, while Clang and scan_precise resolve the actual dependency to dep. A later save to module dep calls importers_of("dep"), so closed TUs using macro-spelled imports are not reindexed; fall back to precise scanning or mark these imports as needing preprocessing instead of storing the raw token text.
Useful? React with 👍 / 👎.
| // within-batch cascades tolerate a stale reverse map by | ||
| // design (they union the pre/post snapshots). | ||
| workspace.dep_graph.clear_includes(path_id); | ||
| workspace.dep_graph.set_imports(path_id, {}); |
There was a problem hiding this comment.
The DiskRemoved path only updates the compile graph and clears this file's outgoing import edges; it never walks dep_graph.importers_of for the module the removed interface provided. If a module interface file is deleted or renamed, closed TUs that import that module but never compiled in this session are therefore not enqueued and keep serving stale index data; run the same importer cascade before erasing the provider state.
Useful? React with 👍 / 👎.
| ? file.OriginalSourceFileName | ||
| : file.ActualOriginalSourceFileName; | ||
| if(!source.empty()) { | ||
| sources.push_back({source, hash_file_content(source)}); |
There was a problem hiding this comment.
Imported module sources are consumed through already-built PCMs, but this records the source file's current disk hash after the importer finishes. If a module interface is saved while an importer is compiling against the old PCM, the importer result can store the post-save hash; the save enqueues a reindex, but need_update() then sees the stored hash matches disk and skips it, leaving rows built from the old PCM indefinitely. Use the PCM build's dependency snapshot/hash for imported modules, or otherwise avoid treating a post-compile disk read as the content the importer consumed.
Useful? React with 👍 / 👎.
| for(auto import_id: tu_index.imports) { | ||
| deps.push_back({tu_index.graph.paths[import_id], 0, 0, worker_hash(import_id)}); |
There was a problem hiding this comment.
Adding imported module sources to a shard's dependency list only affects newly written shards, but existing persisted MergedIndex blobs are still accepted with the same index format version and therefore lack these module deps. After an upgrade, a module save can enqueue an importer, then index_one filters it out as !need_update() because the old shard has no dependency on the module source; bump/discard the index cache or force these importer reindexes so stale pre-change shards cannot survive indefinitely.
Useful? React with 👍 / 👎.
- Consolidate the three per-layer file-hash helpers into fs::hash_file: workers and master now provably share one hashing scheme, which the cross-process hash comparisons depend on. - Re-enqueue a TU whose main file could not be read at merge time; the old comment promised a retry that nothing actually scheduled. - Memoize imported_module_sources: deps() and the TUIndex build both need it for the same unit, halving the disk reads per module import. - Make the interface-source snapshot comment precise about the window a save can hit between a PCM build and the importer's own hash. - Compare PCM dep paths separator-normalized in the chain test: the Windows driver records forward slashes while TempDir hands out native paths, and the consuming staleness checks are stat-based.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d0f5712e7
ℹ️ 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".
| } | ||
| for(auto importer: workspace.dep_graph.importers_of(module_it->second)) { | ||
| if(visited.insert(importer).second) { | ||
| split_dependents(importer); |
There was a problem hiding this comment.
When the importer returned by importers_of() is a header discovered during dependency scanning, this marks only that header dirty; it does not enqueue the root TUs that include it. For a closed TU like a.cpp that includes h.h, where h.h contains import M;, saving module M leaves a.cpp's index/PCM state stale because the changed module is consumed through the header but find_host_sources(h.h) is never cascaded here.
Useful? React with 👍 / 👎.
| if(!sr.modules.empty() || implicit_interface_dep) { | ||
| llvm::SmallVector<std::string> imports(sr.modules.begin(), sr.modules.end()); | ||
| if(implicit_interface_dep) { | ||
| imports.push_back(sr.module_name); | ||
| } | ||
| graph.set_imports(scan_result.path_id, imports); |
There was a problem hiding this comment.
For files where scan() sets need_preprocess on a conditional module declaration, it returns before collecting later import declarations; the fallback above only fills module_name and is_interface_unit, so sr.modules remains empty and this new import-edge block records nothing. A conditional module interface that imports B therefore will not be reached when B is saved, leaving closed importers stale; the fallback needs to populate imports too, e.g. via the precise scan result.
Useful? React with 👍 / 👎.
|
Closing for a redo: the module-importer reverse edges here are built from the lexical directive scanner parsing import names out of raw tokens, which is the wrong layer for imports — lexical include edges over-approximate harmlessly, but lexically-parsed import names can miss the real module (macro-spelled names, conditional declarations), which under-invalidates. The rework will source importer edges from precise, already-persisted data (worker-reported interface sources in shard dependencies) instead. The worker-side content hashing, shard dependency format, and version bump in this PR are sound and will be carried over. |
1 participant
Problem
The master validates compilation artifacts (AST, PCH, PCM, index shards)
against dependency snapshots it captures by re-reading files from disk when
a worker result arrives. That snapshot describes the disk at completion
time, not the content the worker actually compiled. If a file is saved while
a compilation is in flight, the master can bless the new disk content as
the baseline of an artifact built from the old content — the artifact then
looks permanently fresh and the pull-side staleness check can never recover.
For PCH and PCM blobs this even survives a restart, because the wrong
baseline is persisted in cache.json.
Separately, C++20
importedges were invisible to all three invalidationmechanisms (the include reverse-graph, worker-reported deps, and index shard
dependencies). Saving a module interface only reached importers that had
already compiled this session; a closed importer that never compiled kept
serving stale cross-file references indefinitely.
Changes
Consumed-content hashing (worker side)
CompileResult::deps,BuildResult::deps) now carry{path, xxh3 hash}entries plus the compilestart time. The worker hashes each dependency's SourceManager-resident
buffer at the end of the compilation — the bytes the compiler actually
consumed — instead of the master re-reading disk later. This is an internal
protocol: master and workers ship from the same build, so there is no
compatibility concern.
TUIndexships a consumed-content hash per path(
IncludeGraph::path_hashes), resolving the existing TODO inMergedIndex::mergeabout re-reading every dependency on the event loop.capture_deps_snapshotadopts the worker hashes verbatim and stamps thesnapshot with the compile start time. The previous arrival-time baseline
let the mtime fast layer wave through files saved mid-compile; with a
start-time baseline such saves fall through to the hash comparison and are
detected. The reported time additionally backs off one second, since file
mtimes compare at second granularity.
existed but nothing filled it); they now carry the module's own interface
source, every header it includes, and the interfaces of imported modules —
so pull-side PCM revalidation actually works.
Index merge: explicit redo instead of silently wrong stores
Indexer::mergestores file content read from disk next to index rows theworker built from the content it read. The two can diverge when a write
lands during the indexing run; storing either would corrupt position
mapping. The merge now compares the disk content's hash against the
worker-reported hash and, on mismatch, skips that file, keeps the previous
snapshot (now also protected from the stale-contribution sweep, which
previously deleted the very snapshot the read-failure path meant to keep),
and re-enqueues the TU. An indexing round that ends with such leftovers
reschedules itself.
Module import edges
CompilationUnitRef::deps()reports the interface sources of every loadedmodule file (direct and transitive), so AST, PCH/PCM, and index shard
dependencies all see
importedges.previously never populated
ScanResult::modules—importdeclarationswere skipped entirely), and
DependencyGraphkeeps a module → importersreverse map, updated on save rescans and file removal.
a module interface (or a header embedded in one) now recompiles open
importers and re-enqueues closed ones, transitively through importing
interfaces — including importers that never compiled this session.
(previously only module units themselves got this treatment, so an
importer's index run compiled against nothing after its PCMs were
invalidated, producing junk shards full of errors).
Pre-existing crash fix
Indexing any TU that imports a module crashed the stateless worker: index
rows can land in the module's own source files (via the loaded AST), whose
FileIDs the include graph does not know, and
IncludeGraph::path_iddereferenced a not-found iterator. Those rows belong to the module's own
index run and are now dropped, with a comment explaining why.
Small items in the same area
MergedIndex::need_updatedocuments why checkingcompilation_contexts.begin()is exhaustive (the map holds at most oneentry — the shard's own TU; header contributions live in a separate table).
Indexer::index_onere-checks the session table and shard freshness afterawaiting module PCM builds, which can suspend for a long time.
name, so the importer cascade recognizes the file as a provider.
Behavioral notes
artifact was provably built from exactly this content".
blessed with the new source hash (the source is consumed indirectly through
its PCM, so there is no resident buffer to hash); the save-side cascade
covers that window, and
imported_module_sourcesdocuments the contract.back to the conservative rebuild path, same as before.
not yet cascade to importers; that belongs with the close/removal semantics
work and is left for a follow-up.
Tests
capture_deps_snapshot/deps_changed(including the mid-compile-save recovery the arrival-time baseline could
not detect); merge mismatch skip/keep/re-enqueue; import edge storage,
replacement and importer lookup in
DependencyGraph; the lexer scan's newimport parsing (dotted names, export import, partitions, header units);
save cascades reaching open/closed/transitive importers (and not cascading
from implementation units); save-rescan updating import edges from disk;
PCH/PCM dep hashes against real compilations; module symbol identity across
the PCM boundary; TUIndex hash/import serialization round-trip; end-to-end
worker IPC assertions for compile, PCM build, and index results.
test_module_reindex.py— cross-file references from aclosed importer resolve, the interface is saved with a changed symbol, and
the closed importer's references update after the automatic reindex.
Local verification on top of the rebased main: format, RelWithDebInfo build,
873 unit tests, 261 integration tests, 3/3 smoke replays — all green.
Summary by CodeRabbit