Repository navigation
fix(compile): include embedded files in unit deps - #506
Merged
Merged
Conversation
`deps()` only collected includes and __has_include hits, so a change to an `#embed`ed file never invalidated the products built from it. The directive collector already records embeds with their file entries; feed them into the dependency set. Embedded files have no FileID, which surfaced that path resolution was duplicated and VFS-blind. `file_path` gains a FileEntryRef overload as the single canonicalization point: absolutize against the compile's working directory, resolve through the compiler's VFS (real_path used to bypass it and silently fall back to the raw — possibly relative — spelling for VFS-only files), and cache keyed by the inode-unique FileEntry so every symlinked spelling of a file collapses to one path.
📝 WalkthroughWalkthroughCompilation units now resolve and cache canonical file paths by ChangesCompilation dependency tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CompilationUnitRef
participant FileEntryRef
participant VFS
CompilationUnitRef->>FileEntryRef: resolve file entry name
CompilationUnitRef->>VFS: resolve real path
VFS-->>CompilationUnitRef: return canonical path or failure
CompilationUnitRef-->>CompilationUnitRef: cache resolved path
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 075c605a92
ℹ️ 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".
file_path returned an empty StringRef for invalid or entry-less fids, but no caller ever checked for it — the error channel only spread empty paths silently. Make the contract explicit: assert in debug, degrade in release. The two places that actually leaned on the empty return now handle their cases themselves: deps() skips failed includes (invalid fid) explicitly, and diagnostics' related information skips synthetic buffers instead of emitting an empty URI.
Each guard added with the file_path assert contract gets a test that was verified to abort a build with the guard removed: deps() skipping a failed include (invalid fid), and diagnostics' related information skipping a note into <command line> (macro redefinition) instead of emitting an empty URI.
Keying by FileEntry* collapsed hardlinked spellings (which real_path does not fold) to whichever path was canonicalized first, dropping the other spellings from the dependency set. Key by FileEntryRef instead: symlinked spellings still resolve to one path, hardlinked ones each keep their own — deps must cover every spelling the compile read. Also documents that not-found __has_include/__has_embed probes leave no dependency trace (tracking them needs the failed lookup's candidate paths, clang's MissingFiles mechanism).
16bit-ykiko
added a commit
that referenced
this pull request
Jul 15, 2026
## Worker pool refactor Replaces the worker pool's ad-hoc bookkeeping with an explicit design: - **Slot state machine** (`Alive → Dying → Respawning`, terminal `Retired`/`Dead`) with a per-slot generation bumped on every death; all occupancy counts are derived from slot state instead of hand-maintained counters. - **No in-pool request retry.** A transport failure marks the slot dead immediately (plus a safety SIGKILL) and surfaces the new `dispatch_errc::worker_crashed`; callers own the policy — the compiler resends idempotent builds once, the indexer requeues the file (bounded at 3 attempts, so a poison file cannot drain the pool's crash budget; `worker_unavailable` is never requeued to avoid a dead-pool spin). - **Real memory preemption.** Under severe pressure the pool kills low-priority workers (immediately reclaiming their RSS), floors the low allowance, and respawns without crash accounting; the sender observes `cancelled`. Master-side accounting no longer diverges from worker reality. - **Crash budget with recovery.** Consecutive crash streak, reset after 30s healthy uptime, exponential respawn backoff (immediate/500ms/1s/... capped); spawn failures count into the streak and retry. Replaces the lifetime `max_restarts=2` that permanently killed slots and could be burned in milliseconds. - **Peer lifetime via `shared_ptr`** (senders hold their own reference), removing the unbounded `retired_peers` retention. - **Controllers split** into `tick_memory(ratio)` / `tick_scaling(ratio)` (unit-testable with injected ratios); scale-up grows `low_limit` by exactly one instead of resetting pressure/backoff state. - Stateful side: least-loaded assignment can no longer pin a document to a dead worker; crash-window sends fail fast with `worker_crashed`. Fixes found during the investigation and covered by new tests: retry-after-failure could land on the same dead worker (the `exclude` parameter was never forwarded), cancelled index requests were silently dropped, `scale_up` wiped AIMD state, dispatch was not triggered when a death freed a busy claim. ## Staleness fix (pre-existing flake) `test_staleness.py` flaked on main (2/4 local runs on RelWithDebInfo): `DepsSnapshot`'s fast path compared dep mtimes against a wall-clock `build_at`, so an edit made after the system clock stepped backwards (NTP sync, VM resume) got an "older" mtime and was silently skipped — stale ASTs/diagnostics until an unrelated invalidation. Now each dep records its nanosecond mtime at capture and is compared by equality, falling through to the content-hash check on any mismatch (the file tracker's existing scheme). Old `cache.json` files load unchanged and re-validate by hash once. 10 consecutive local runs of the suite pass. Note: the index-side staleness gate (`merged_index.cpp`, `mtime <= build_at`) has the same wall-clock hazard; fixing it needs an index schema change and is left as a follow-up. ## Testing - Unit: 884 pass (Debug + RelWithDebInfo); new coverage for backoff schedule, crash budget boundaries, healthy-uptime reset, preemption accounting, FIFO fairness, generation guards, transport-error classification, clock-rollback staleness. - Integration: 263 pass on both configs, including new `test_crash_during_indexing` (SIGKILL a stateless worker mid-round, assert full index convergence). - Smoke: 3/3 on both configs. ## Follow-up (2026-07-13) - Rebased onto current main (#501 preamble index, #505 TUIndex fid fix, #506 embed deps). The one conflict (`workspace.cpp` cache load/save) resolved in favor of #501's `.pch.idx` fields plus this branch's per-dep mtime scheme; legacy `cache.json` compatibility re-verified against the JSON codec (unknown fields ignored, absent `mtime` re-validates by hash once). - Addressed both review comments: - `preempt_low_priority` now cancels the preempt source **before** killing the worker. Investigation note: the old order was not an observable bug (kotatsu's `event.set()` only queues waiters and there is no suspension point between the calls), so this is a defensive ordering, documented as such. - The index requeue cap now counts only `worker_crashed`; a memory-pressure cancellation requeues without spending the poison-file budget. The policy moved into `Indexer::note_dispatch_failure` with a friend fixture and three new unit tests pinning it. - Cleanup: refreshed comments that still described the removed `DepsSnapshot::build_at`, and the pool class doc's off-by-one crash-budget wording. - Updated totals: unit 927 (Debug + RelWithDebInfo), integration 271, smoke 3/3 — all green locally. ## Document quarantine (F19) — 2026-07-14/15 The pool's per-slot crash budget cannot contain a poison *document*: the budget lives on slots, the poison lives in content, and one file could burn slot after slot until the whole pool was dead (bughunt F19). This branch adds content-side crash containment, hardened over ten codex review rounds (24 findings — 23 fixed, 1 documented acceptance) and consolidated into a single-writer architecture whose invariants are stated in `state/quarantine.h`: - **`Quarantine`**: per-document state machine with private fields — every transition is a method. Two ledger families with distinct disproof rules: the **compile streak** (cleared only by a successful compile of the same content, via a `Flight` token that pins "a landing clears exactly the evidence it inherited at takeoff") and **per-kind ledgers** (queries by `QueryKind`, stateless builds by `BuildKind`, document links) cleared only when *that kind* answers — a hover cannot launder semantic-tokens evidence, a compile cannot launder format strikes. Evidence is counted **per worker killed** (the retry's death is a second strike) and deduplicated by the dead incarnation's identity carried in `Error::data`, so one process death failing several in-flight requests is blamed at most once per document. - **Recovery licensing**: a real content change arms one probe; only the dispatch that can disprove the evidence (`recovery_compile` / `recovery_kind`) may spend it, concurrent racers see the kind as plainly blocked, and `ProbeGuard` hands the license back if the attempt provably never dispatched (cancellation unwind, capacity window) while a recorded strike keeps it spent. While quarantined, everything except the recovery dispatch is refused. - **Visibility**: `publish_quarantined` is the single materialization point for the quarantine diagnostic (announce-once per spell, including quarantines reached via completion/PCH paths that never run a compile); `publish_recovered` clears it when a stateless/query recovery lifts the quarantine without a compile. - **`CrashBudget`**: content-keyed budget for shared artifacts (PCH/PCM) that document quarantine cannot reach — every dependent stops re-triggering a build that keeps killing workers. Full lifecycle: blocks are five-minute cooldowns rather than verdicts (the poison may live in a header the key cannot see), a successful build clears the key's strikes, and the PCM budget key embeds the module's content hash so fixing the module unlocks immediately. - **Pool-side isolation** (`Suspect`): probes run `Isolated` on document-free workers (single-worker pools excepted) and never spend slot budget; recovery queries run `InPlace` — owner routing, budget-exempt, avoided by new-document assignment while in flight, with the counter unwound by RAII across cancellations. Dead slots revive after a cooldown and are preferred by scale-up; the scale-up ceiling counts every live process including retiring ones; the configured `min_stateless` floor survives above the startup count. The pool's responsibility contract (mechanism vs. policy, the `dispatch_errc` taxonomy, `worker_restarting` for blameless restart windows) is documented on the class. - Indexer debt hardening: failures attribute by launch ticket (stale crashes never spend fresh content's budget), giving up clears the pending slot even past a mid-flight deps-only downgrade, `worker_unavailable` requeues while slot revival is pending, and a failed content pass keeps its `ContentChanged` debt. - One documented acceptance: the same-generation eviction ABA (a live worker's stale-copy eviction draining after the path was assigned back to it) costs one spurious recompile in a one-notification-drain window; fixing it needs an ownership epoch in the worker protocol. Verified end to end by the F19 stress scenario (healthy files answer throughout, quarantined streak freezes, fixed file recovers via probe within ms, worker count never reaches zero), poison / poison-preamble integration tests, and 30+ type-level state-machine unit tests (several validated by reverting the fix and watching them fail). - Final totals: unit 976 (Debug + RelWithDebInfo), integration 279 (both configs), smoke 3/3 — all green locally and on CI (17 checks).
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.
Problem
deps()only collected#includeand__has_includehits, so a change to an#embeded file never invalidated the products built from it — stale PCH/AST/index served indefinitely until an unrelated dependency changed. The directive collector already records embeds with their file entries; they just never reached the dependency set.Changes
1.
#embed/__has_embedfiles enter the dependency set (split out of #505 as promised there).2.
file_pathgains aFileEntryRefoverload as the single canonicalization point. Embedded files have no FileID (clang delivers their contents as one annotation token), which surfaced that path resolution was about to be duplicated — and that the existing implementation was VFS-blind:FileManager::makeAbsolutePath, i.e. against the compile's working directory (the oldllvm::sys::fs::real_pathresolved relative spellings against the server process CWD);getVirtualFileSystem().getRealPath()); the old call bypassed the VFS entirely and silently fell back to the raw — possibly relative — spelling for VFS-only files;The cache is keyed by the spelling-level
FileEntryRef(review finding): symlinked spellings still collapse into one path throughgetRealPath, while hardlinked spellings — whichreal_pathdoes not fold — each keep their own, so the dependency set covers every spelling the compile read. For on-disk, symlink-free absolute paths (the production common case) the produced bytes are identical to before, so persistedcache.jsonsnapshots validate without spurious rebuilds.3.
file_pathinvalid input is now an asserted contract, not a silent empty return. The old empty-StringReferror channel had no checking callers — it only spread empty paths silently. Auditing callers under the new contract surfaced two spots that were leaning on it:deps()let failed includes (file not found → invalid fid) flow intofile_pathand relied on the empty return to drop them; it now skips them explicitly;-DFOO=1redefinition notes point into<command line>) and emitted empty-URI locations to the client; it now skips them.Debug builds assert on contract violations (the full Debug test matrix acts as caller verification); release degrades gracefully — the worker never crashes on it.
Explicitly out of scope — not-found probe tracking (review finding): a
__has_include/__has_embedmiss leaves no dependency trace, so creating the probed file later cannot invalidate products built while it was missing. Investigation showedclang -MDdrops misses the same way (the build ecosystem accepts this hole), and even clang's preamble simulates the failed lookup's candidate paths only for#includemisses. Rather than stat'ing candidate sets on every freshness check, the right home is the invalidation pipeline — persist unresolved lookups and match them against file-creation events from the workspace watcher. Documented in a FIXME; belongs with the unresolved-include invisibility work.Testing
Directive.EmbedInDeps:#embed,__has_embed, and#includefiles all land indeps()with exact path equality, and every dep is absolute (pins the relative-fallback fix; the in-memory test VFS exercises the VFS resolution branch the old implementation couldn't handle).Directive.FailedIncludeDepsandDiagnostic.CommandLineNotepin the two contract guards; each was verified to abort a Debug build with its guard removed, and both also fail against the old empty-return behavior (the empty-URI related-info was real, observable bad output).EXPECT_INCLUDE/EXPECT_HAS_INLfull-path assertions double as regression guards for the resolution change.