feat: persistent PCH/PCM cache across sessions - #391
Merged
Conversation
Write PCH/PCM artifacts to .clice/pch/ and .clice/pcm/ with deterministic content-addressed filenames so they survive across sessions. PCH files are named by preamble content hash; PCM files by module name + source path hash. Key changes: - Add output_path field to BuildPCHParams/BuildPCMParams so master can specify where workers write artifacts - Workers use atomic temp+rename when output_path is provided - ensure_pch() computes deterministic path, checks disk for cached files from prior sessions, sets output_path for workers - CompileGraph dispatch checks pcm_states before rebuilding modules - cache.json persists deps snapshots (paths + content hashes) for cross-session staleness validation - load_cache()/save_cache()/cleanup_cache() manage the persistent cache lifecycle; stale files (>7 days) are cleaned on startup - didClose only clears in-memory state; cached files remain on disk - Add fs::rename() helper; update ThreadSafeFS to match .pch extension instead of preamble- prefix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds persistent on-disk JSON-backed caching for PCH and PCM with deterministic content-addressed artifact filenames, workspace cache directory setup and cleanup, dependency-snapshot staleness checks, worker-controlled output paths, and cache persistence on shutdown and didSave invalidation. Changes
Sequence DiagramsequenceDiagram
participant MS as MasterServer
participant Disk as DiskCache
participant Worker as StatelessWorker
participant Comp as Compiler
rect rgba(100,150,200,0.5)
Note over MS,Disk: Startup
MS->>Disk: load_cache() (read cache/cache.json)
Disk-->>MS: pch/pcm metadata
MS->>Disk: cleanup_cache(max_age_days)
end
rect rgba(100,200,150,0.5)
Note over MS,Worker: Build request with caching
MS->>MS: check pcm_states / pch_states & deps_changed()
alt cache hit & deps valid
MS-->>MS: return cached artifact path
else cache miss or stale
MS->>Worker: BuildPCM/BuildPCH(params.output_path)
Worker->>Comp: compile -> params.output_path
Comp-->>Worker: compile result
Worker->>Disk: place/rename artifact into cache dir
Worker-->>MS: Build result with final_path
MS->>MS: update pcm_states/pch_states
MS->>Disk: save_cache() write cache/cache.json
end
end
rect rgba(200,150,100,0.5)
Note over MS,Disk: Shutdown / didSave
MS->>Disk: save_cache() persist metadata
Disk-->>MS: persisted
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/server/master_server.cpp (2)
565-568: Consider deferring cache saves for better performance in large projects.
save_cache()is called after every successful PCH/PCM build (lines 567, 689), which serializes the entire cache state to disk. For projects with many modules, this O(n) operation on each build could add noticeable latency.Consider either:
- Batching saves (e.g., save every N builds or after a timeout)
- Using a dirty flag and saving only on shutdown/periodic intervals
The current approach prioritizes durability, which is reasonable. This is just a performance consideration for future optimization if needed.
Also applies to: 688-690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 565 - 568, The current code calls save_cache() after every successful PCH/PCM build (e.g., where save_cache() is invoked following build completion), which causes an O(n) disk serialization on each build; change this to defer frequent saves by introducing a dirty flag (e.g., cache_dirty) set by the build completion paths and replace immediate save_cache() calls with only marking dirty, then persist the cache either on shutdown or via a periodic flush task (timer-based) or batch counter (save after N builds) to reduce synchronous save frequency; ensure the same update is applied to both locations where save_cache() is currently invoked so durability is preserved on shutdown or at configured intervals.
619-643: Minor inefficiency: double map lookup.Line 620 performs
pch_states.find(path_id)and then line 622 usespch_states[path_id], resulting in two separate lookups. Consider reusing the iterator from the first lookup.🔧 Proposed fix to reuse iterator
- // Check if the file exists on disk (e.g. from a previous session) but not in memory. - if(pch_states.find(path_id) == pch_states.end() || pch_states[path_id].path != pch_path) { + // Check if the file exists on disk (e.g. from a previous session) but not in memory. + auto disk_it = pch_states.find(path_id); + bool no_memory_entry = (disk_it == pch_states.end()); + bool path_mismatch = !no_memory_entry && (disk_it->second.path != pch_path); + if(no_memory_entry || path_mismatch) { if(llvm::sys::fs::exists(pch_path)) { - auto& st = pch_states[path_id]; + auto& st = no_memory_entry ? pch_states[path_id] : disk_it->second;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 619 - 643, The code does a double lookup into pch_states (pch_states.find(path_id) then pch_states[path_id]); change to use the iterator returned by pch_states.find(path_id) to avoid the second lookup: store the iterator result (e.g., auto it = pch_states.find(path_id)), check it == end(), and then use it->second (st) instead of pch_states[path_id] throughout this block (keep using pch_path, preamble_hash, st.deps, st.path, st.hash, st.bound, and deps_changed(path_pool, st.deps) as before).src/server/stateless_worker.cpp (1)
144-185: Minor inconsistency in error return initialization.The BuildPCM flow correctly implements the atomic write pattern. However, line 153 returns
{false, "Failed to create temporary PCM file"}with only 2 initializers, while other error returns (lines 174, 184) provide 3 initializers. This works due to aggregate initialization defaultingpcm_pathto empty string, but is inconsistent.🔧 Proposed fix for consistency
if(!tmp) { LOG_ERROR("BuildPCM: failed to create temp file"); - return {false, "Failed to create temporary PCM file"}; + return {false, "Failed to create temporary PCM file", ""}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/stateless_worker.cpp` around lines 144 - 185, The return when failing to create the temporary PCM file is using only two initializers ({false, "Failed to create temporary PCM file"}) which is inconsistent with other error returns that provide three fields; update that return to supply the missing pcm_path field (e.g. include an empty string) so it matches the other error returns and the worker::BuildPCMResult aggregate initialization used elsewhere (reference: tmp_path, params.output_path, worker::BuildPCMResult).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 341-366: The cleanup_cache() call can remove files that were just
loaded into pch_states, leaving dangling entries; either move the
cleanup_cache() invocation to run before load_cache() in MasterServer
initialization, or (preferred) add an existence check where pch_states entries
are reused (the PCH reuse path that reads from pch_states) — call
llvm::sys::fs::exists(or llvm::sys::fs::status) on the file path stored in the
pch_states entry before using it, and if the file no longer exists remove that
entry from pch_states and fall back to a cache miss; reference
MasterServer::cleanup_cache(), load_cache(), and the pch_states reuse site when
applying the change.
---
Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 565-568: The current code calls save_cache() after every
successful PCH/PCM build (e.g., where save_cache() is invoked following build
completion), which causes an O(n) disk serialization on each build; change this
to defer frequent saves by introducing a dirty flag (e.g., cache_dirty) set by
the build completion paths and replace immediate save_cache() calls with only
marking dirty, then persist the cache either on shutdown or via a periodic flush
task (timer-based) or batch counter (save after N builds) to reduce synchronous
save frequency; ensure the same update is applied to both locations where
save_cache() is currently invoked so durability is preserved on shutdown or at
configured intervals.
- Around line 619-643: The code does a double lookup into pch_states
(pch_states.find(path_id) then pch_states[path_id]); change to use the iterator
returned by pch_states.find(path_id) to avoid the second lookup: store the
iterator result (e.g., auto it = pch_states.find(path_id)), check it == end(),
and then use it->second (st) instead of pch_states[path_id] throughout this
block (keep using pch_path, preamble_hash, st.deps, st.path, st.hash, st.bound,
and deps_changed(path_pool, st.deps) as before).
In `@src/server/stateless_worker.cpp`:
- Around line 144-185: The return when failing to create the temporary PCM file
is using only two initializers ({false, "Failed to create temporary PCM file"})
which is inconsistent with other error returns that provide three fields; update
that return to supply the missing pcm_path field (e.g. include an empty string)
so it matches the other error returns and the worker::BuildPCMResult aggregate
initialization used elsewhere (reference: tmp_path, params.output_path,
worker::BuildPCMResult).
🪄 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: 53ad509e-3bd8-463b-b51e-5625ee8e0ffc
📒 Files selected for processing (5)
src/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/stateless_worker.cppsrc/support/filesystem.h
9 integration tests covering: - PCH written to .clice/pch/ with hex-hash filename - cache.json persisted with deps snapshot after build - PCH reused on close+reopen within same session - PCH survives full server restart (cache.json loaded) - Same preamble across files shares one PCH (content-addressed) - Different preambles produce different PCH files - Header changes trigger PCH rebuild - No .tmp files left after successful builds - Cache directories created on workspace initialization Also simplify worker output path handling: write directly to output_path instead of temp+rename, avoiding an LLVM fs::rename issue where rename fails with ENOENT on compiled PCH files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use eventide's compile-time reflection serde instead of manual llvm::json::Object/Array construction. Define CacheData/CachePCHEntry/ CachePCMEntry structs and let et::serde::json::to_json/from_json handle serialization automatically. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/server/master_server.cpp (3)
174-175: Consider logging parse failures for hash strings.
llvm::StringRef::getAsInteger()returnstrueon parse failure, but the return value is ignored. While this gracefully degrades (hash stays 0, causing cache miss), silently ignoring malformedcache.jsondata could mask corruption.Optional: Add logging for parse failures
std::uint64_t hash = 0; - llvm::StringRef(hash_str).getAsInteger(10, hash); + if(llvm::StringRef(hash_str).getAsInteger(10, hash)) { + LOG_WARN("Failed to parse PCH hash '{}' in cache.json", hash_str); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 174 - 175, The code currently calls llvm::StringRef(hash_str).getAsInteger(10, hash) but ignores its boolean return (true on failure); update the call site to capture the return value and, on failure, emit a diagnostic via the existing logging facility (include the offending hash_str and context such as the cache key or filename) so malformed cache.json entries are visible; keep the fallback behavior (leave hash == 0) but ensure the parse failure is logged using the same logger used elsewhere in this module so it’s easy to trace.
571-573: Consider batching cache saves for performance.
save_cache()is called after every successful PCM build (line 573) and PCH build (line 695). For projects with many modules, this could cause significant I/O overhead during initial builds. Consider deferring persistence to idle periods or shutdown, or implementing a dirty flag with periodic flush.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 571 - 573, Currently save_cache() is invoked immediately after each successful PCM and PCH build; change this to mark the in-memory cache as dirty (e.g., set a cache_dirty flag) instead of persisting synchronously, remove the immediate save_cache() calls in the PCM/PCH build success paths, and add a single flushing mechanism that writes the cache to disk only on idle/periodic intervals or at shutdown (invoke save_cache() if cache_dirty), ensuring you handle concurrency by protecting cache_dirty/save_cache() with the existing cache mutex or a new lock.
625-649: On-disk PCH reuse logic is functional but could be clearer.The logic at lines 626-649 handles reusing PCH files from disk when not in memory. However,
pch_states[path_id]at line 628 implicitly inserts a default entry if the key doesn't exist (since line 626's condition includesfind() == end()). While functional, this implicit insertion makes the intent less clear.Optional: Use explicit insertion for clarity
// Check if the file exists on disk (e.g. from a previous session) but not in memory. - if(pch_states.find(path_id) == pch_states.end() || pch_states[path_id].path != pch_path) { + auto [it, inserted] = pch_states.try_emplace(path_id); + auto& st = it->second; + if(inserted || st.path != pch_path) { if(llvm::sys::fs::exists(pch_path)) { - auto& st = pch_states[path_id]; if(st.path.empty() || st.hash != preamble_hash) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 625 - 649, The current code implicitly inserts a default entry via pch_states[path_id] when checking/using it; change the logic to obtain an iterator/result from pch_states.find(path_id) once, and only create or emplace a new entry explicitly when needed (e.g., use the found iterator to read existing entry, and call pch_states.emplace/try_emplace to construct st only when you intend to insert). Update uses of st to reference the iterator->second or the newly emplaced element so the intent around creation vs lookup for pch_states, path_id, and subsequent fields (st.path, st.hash, st.deps, st.bound, st.deps.build_at) is explicit.src/server/stateless_worker.cpp (1)
134-136: Missingpcm_pathfield in error return.The return statement on line 135 only initializes 2 fields (
success,error), butBuildPCMResulthas 4 fields. While the compiler will default-initializepcm_pathto an empty string, this is inconsistent with the similar return at line 152 which explicitly provides all 3 positional fields. For consistency and clarity:Proposed fix
if(!tmp) { LOG_ERROR("BuildPCM: failed to create temp file"); - return {false, "Failed to create temporary PCM file"}; + return {false, "Failed to create temporary PCM file", ""}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/stateless_worker.cpp` around lines 134 - 136, The error return from BuildPCM (where LOG_ERROR("BuildPCM: failed to create temp file") is called) constructs a BuildPCMResult with only success and error fields; update that return to explicitly include the pcm_path field (e.g., provide an empty string or the temp path variable) so it matches the other return site that supplies all positional fields and keeps BuildPCMResult construction consistent.tests/integration/test_persistent_cache.py (2)
77-96: Consider logging exceptions in shutdown cleanup.The
try-except-passpatterns swallow all exceptions during client shutdown, which can make debugging test failures harder. While acceptable for cleanup code that shouldn't fail tests, adding debug logging would help diagnose issues.Optional: Add logging for swallowed exceptions
+import logging + +logger = logging.getLogger(__name__) + async def _shutdown_client(c: CliceClient) -> None: """Gracefully shut down a client.""" try: await asyncio.wait_for(c.shutdown_async(None), timeout=5.0) - except Exception: - pass + except Exception as e: + logger.debug("shutdown_async failed: %s", e) try: c.exit(None) - except Exception: - pass + except Exception as e: + logger.debug("exit failed: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 77 - 96, The shutdown helper _shutdown_client currently swallows all exceptions with bare excepts; update each except block to catch Exception as e and log the exception (including traceback) via the test logger or standard logging before continuing so cleanup still proceeds; specifically wrap and log errors from await asyncio.wait_for(c.shutdown_async(None), ...), c.exit(None), c._server.kill(), and the block that sets c._stop_event and cancels c._async_tasks, using the logger name used in tests or import logging and call logging.debug/error with the exception information to aid debugging.
309-316: Consider strengthening the header-change rebuild assertion.The test correctly explains in the comment (lines 310-315) that the PCH is overwritten because deps changed, not because the preamble hash changed. However, the assertion only verifies a PCH file exists, not that it was actually rebuilt. Checking that the mtime changed would strengthen this test.
Optional: Add mtime change verification
pch_before = _list_pch_files(tmp_path) assert len(pch_before) >= 1 + pch_mtime_before = pch_before[0].stat().st_mtime # Modify header — changes preamble content hash. await asyncio.sleep(1.1) # ... existing code ... pch_after = _list_pch_files(tmp_path) - # ... existing comment ... assert len(pch_after) >= 1 + # Verify the PCH was rebuilt (mtime should be different). + pch_mtime_after = pch_after[0].stat().st_mtime + assert pch_mtime_after > pch_mtime_before, ( + "PCH should have been rebuilt due to header change" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 309 - 316, Record PCH file mtimes before the change and compare them after to ensure rebuild occurred: use the helper _list_pch_files (and the prior pch_before variable) to map filenames to their os.stat(...).st_mtime before the edit, then after running the rebuild get pch_after and assert that for the matching PCH path(s) the mtime increased (or at least differs), e.g., find the common filename in pch_before/pch_after and assert old_mtime != new_mtime to prove the file was overwritten/rebuilt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 174-175: The code currently calls
llvm::StringRef(hash_str).getAsInteger(10, hash) but ignores its boolean return
(true on failure); update the call site to capture the return value and, on
failure, emit a diagnostic via the existing logging facility (include the
offending hash_str and context such as the cache key or filename) so malformed
cache.json entries are visible; keep the fallback behavior (leave hash == 0) but
ensure the parse failure is logged using the same logger used elsewhere in this
module so it’s easy to trace.
- Around line 571-573: Currently save_cache() is invoked immediately after each
successful PCM and PCH build; change this to mark the in-memory cache as dirty
(e.g., set a cache_dirty flag) instead of persisting synchronously, remove the
immediate save_cache() calls in the PCM/PCH build success paths, and add a
single flushing mechanism that writes the cache to disk only on idle/periodic
intervals or at shutdown (invoke save_cache() if cache_dirty), ensuring you
handle concurrency by protecting cache_dirty/save_cache() with the existing
cache mutex or a new lock.
- Around line 625-649: The current code implicitly inserts a default entry via
pch_states[path_id] when checking/using it; change the logic to obtain an
iterator/result from pch_states.find(path_id) once, and only create or emplace a
new entry explicitly when needed (e.g., use the found iterator to read existing
entry, and call pch_states.emplace/try_emplace to construct st only when you
intend to insert). Update uses of st to reference the iterator->second or the
newly emplaced element so the intent around creation vs lookup for pch_states,
path_id, and subsequent fields (st.path, st.hash, st.deps, st.bound,
st.deps.build_at) is explicit.
In `@src/server/stateless_worker.cpp`:
- Around line 134-136: The error return from BuildPCM (where
LOG_ERROR("BuildPCM: failed to create temp file") is called) constructs a
BuildPCMResult with only success and error fields; update that return to
explicitly include the pcm_path field (e.g., provide an empty string or the temp
path variable) so it matches the other return site that supplies all positional
fields and keeps BuildPCMResult construction consistent.
In `@tests/integration/test_persistent_cache.py`:
- Around line 77-96: The shutdown helper _shutdown_client currently swallows all
exceptions with bare excepts; update each except block to catch Exception as e
and log the exception (including traceback) via the test logger or standard
logging before continuing so cleanup still proceeds; specifically wrap and log
errors from await asyncio.wait_for(c.shutdown_async(None), ...), c.exit(None),
c._server.kill(), and the block that sets c._stop_event and cancels
c._async_tasks, using the logger name used in tests or import logging and call
logging.debug/error with the exception information to aid debugging.
- Around line 309-316: Record PCH file mtimes before the change and compare them
after to ensure rebuild occurred: use the helper _list_pch_files (and the prior
pch_before variable) to map filenames to their os.stat(...).st_mtime before the
edit, then after running the rebuild get pch_after and assert that for the
matching PCH path(s) the mtime increased (or at least differs), e.g., find the
common filename in pch_before/pch_after and assert old_mtime != new_mtime to
prove the file was overwritten/rebuilt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e8b203f-470b-410c-a78e-9720427193e4
📒 Files selected for processing (3)
src/server/master_server.cppsrc/server/stateless_worker.cpptests/integration/test_persistent_cache.py
- Worker always uses output_path from master, remove
createTemporaryFile fallback (dead code)
- Cache directory layout: .clice/cache/{pch/, pcm/, cache.json}
instead of .clice/{pch/, pcm/, cache.json}
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/master_server.cpp (1)
559-565:⚠️ Potential issue | 🔴 CriticalAdd existence check before reusing cached PCH path.
The fast-path PCH reuse at line 562 reuses
st.pathwithout verifying the file still exists on disk. During workspace initialization,load_cache()loads cache entries whose files exist, thencleanup_cache()immediately runs and deletes files older than 7 days based on file modification time. A PCH file that's 7+ days old could be deleted bycleanup_cache()after being loaded intopch_states, leaving a stale path reference.The code at lines 569-570 shows an existence check was added for the disk-only reuse path but not for the cached entry path. Add
llvm::sys::fs::exists(st.path)to the condition at line 562 to ensure the file hasn't been cleaned up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 559 - 565, The cached-PCH fast-path in the pch_states lookup reuses st.path without verifying the file still exists; update the condition in the block that checks pch_states.find(path_id) (the branch that compares st.hash == preamble_hash && !st.path.empty() && !deps_changed(path_pool, st.deps)) to also call llvm::sys::fs::exists(st.path) before reusing it and setting st.bound = bound so you don't return a stale deleted PCH; this mirrors the existence check used in the disk-only reuse path and prevents using files removed by cleanup_cache after load_cache populated pch_states.
🧹 Nitpick comments (3)
tests/integration/test_persistent_cache.py (1)
77-96: Consider logging exceptions in shutdown helper.The
_shutdown_clienthelper silently swallows all exceptions with bareexcept Exception: pass. While graceful shutdown needs to be resilient, completely silencing errors can mask real issues during test debugging.Static analysis flagged this pattern (Ruff S110, BLE001).
♻️ Proposed fix: Log exceptions at debug level
+import logging + +logger = logging.getLogger(__name__) + async def _shutdown_client(c: CliceClient) -> None: """Gracefully shut down a client.""" try: await asyncio.wait_for(c.shutdown_async(None), timeout=5.0) - except Exception: - pass + except Exception as e: + logger.debug("Shutdown warning: %s", e) try: c.exit(None) - except Exception: - pass + except Exception as e: + logger.debug("Exit warning: %s", e) await asyncio.sleep(0.3) if hasattr(c, "_server") and c._server is not None and c._server.returncode is None: c._server.kill() try: c._stop_event.set() for task in c._async_tasks: task.cancel() await asyncio.sleep(0.1) - except Exception: - pass + except Exception as e: + logger.debug("Cleanup warning: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 77 - 96, The helper _shutdown_client currently swallows all exceptions with bare except blocks; change each except Exception: pass to catch the exception and log it at debug (or warning) level so failures during teardown are visible: wrap the await asyncio.wait_for(c.shutdown_async(None), ...) in try/except Exception as e and call the test logger or logging.getLogger(__name__).debug("shutdown_async failed for %s: %s", c, e, exc_info=True); do the same for c.exit(None), the c._stop_event and task cancellation block, and optionally when killing c._server — reference the function name _shutdown_client and members c.shutdown_async, c.exit, c._server, c._stop_event, and c._async_tasks to locate the spots to replace the silent excepts with logged exceptions.src/server/master_server.cpp (2)
513-515: Consider batching or debouncingsave_cache()calls.
save_cache()is called after every successful PCM build (line 515) and every successful PCH build (line 637). For projects with many modules, this could result in frequent disk I/O as each module triggers a full JSON serialization and write.Consider debouncing these writes (e.g., coalescing multiple saves within a time window) or deferring to shutdown/periodic intervals for better performance during heavy compilation activity.
Also applies to: 636-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 513 - 515, Multiple calls to save_cache() after each successful PCM/PCH build cause excessive disk I/O; change this by coalescing saves: replace direct calls to save_cache() in the PCM/PCH build-success paths with a scheduler that sets a dirty flag and starts/refreshes a debounce timer (e.g., schedule_cache_save()/enqueue_cache_save()) which performs the actual save after a configurable delay or at periodic intervals, and ensure a flush_save_cache_now() is invoked during shutdown to persist remaining changes; reference save_cache(), the PCM/PCH build-success call sites, the new schedule_cache_save()/flush_save_cache_now() symbols you add, and make the debounce interval configurable.
567-591: Complex conditional logic may benefit from restructuring.The nested conditions here are hard to follow:
- Outer check: entry doesn't exist OR path differs from computed pch_path
- Inner check:
st.path.empty() || st.hash != preamble_hash- Innermost check:
!st.deps.path_ids.empty() && st.hash == preamble_hashIf we enter the inner block via
st.hash != preamble_hash, the innermost conditionst.hash == preamble_hashwill always be false, making that branch unreachable in that case.The logic appears correct but could be clearer with early returns or separate functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 567 - 591, The nested conditional is confusing and contains an unreachable branch when entering via st.hash != preamble_hash; refactor the checks in master_server.cpp around pch_states/path_id to use clear early-exit branches: first obtain or create a reference to pch_states[path_id] (st) after confirming llvm::sys::fs::exists(pch_path), then if st.path.empty() or st.hash != preamble_hash treat it as the "mismatch" case (set st.path = pch_path, st.bound = bound, st.hash = preamble_hash and if st.deps.path_ids.empty() set st.deps.build_at = 0) and co_return true; otherwise handle the "hash matches" case separately: if st.hash == preamble_hash and !st.deps.path_ids.empty() and !deps_changed(path_pool, st.deps) then set st.path and st.bound and co_return true; this removes the nested contradiction and makes the roles of st.hash, preamble_hash, and st.deps explicit (referencing pch_states, path_id, pch_path, st.hash, preamble_hash, st.deps, deps_changed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 559-565: The cached-PCH fast-path in the pch_states lookup reuses
st.path without verifying the file still exists; update the condition in the
block that checks pch_states.find(path_id) (the branch that compares st.hash ==
preamble_hash && !st.path.empty() && !deps_changed(path_pool, st.deps)) to also
call llvm::sys::fs::exists(st.path) before reusing it and setting st.bound =
bound so you don't return a stale deleted PCH; this mirrors the existence check
used in the disk-only reuse path and prevents using files removed by
cleanup_cache after load_cache populated pch_states.
---
Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 513-515: Multiple calls to save_cache() after each successful
PCM/PCH build cause excessive disk I/O; change this by coalescing saves: replace
direct calls to save_cache() in the PCM/PCH build-success paths with a scheduler
that sets a dirty flag and starts/refreshes a debounce timer (e.g.,
schedule_cache_save()/enqueue_cache_save()) which performs the actual save after
a configurable delay or at periodic intervals, and ensure a
flush_save_cache_now() is invoked during shutdown to persist remaining changes;
reference save_cache(), the PCM/PCH build-success call sites, the new
schedule_cache_save()/flush_save_cache_now() symbols you add, and make the
debounce interval configurable.
- Around line 567-591: The nested conditional is confusing and contains an
unreachable branch when entering via st.hash != preamble_hash; refactor the
checks in master_server.cpp around pch_states/path_id to use clear early-exit
branches: first obtain or create a reference to pch_states[path_id] (st) after
confirming llvm::sys::fs::exists(pch_path), then if st.path.empty() or st.hash
!= preamble_hash treat it as the "mismatch" case (set st.path = pch_path,
st.bound = bound, st.hash = preamble_hash and if st.deps.path_ids.empty() set
st.deps.build_at = 0) and co_return true; otherwise handle the "hash matches"
case separately: if st.hash == preamble_hash and !st.deps.path_ids.empty() and
!deps_changed(path_pool, st.deps) then set st.path and st.bound and co_return
true; this removes the nested contradiction and makes the roles of st.hash,
preamble_hash, and st.deps explicit (referencing pch_states, path_id, pch_path,
st.hash, preamble_hash, st.deps, deps_changed).
In `@tests/integration/test_persistent_cache.py`:
- Around line 77-96: The helper _shutdown_client currently swallows all
exceptions with bare except blocks; change each except Exception: pass to catch
the exception and log it at debug (or warning) level so failures during teardown
are visible: wrap the await asyncio.wait_for(c.shutdown_async(None), ...) in
try/except Exception as e and call the test logger or
logging.getLogger(__name__).debug("shutdown_async failed for %s: %s", c, e,
exc_info=True); do the same for c.exit(None), the c._stop_event and task
cancellation block, and optionally when killing c._server — reference the
function name _shutdown_client and members c.shutdown_async, c.exit, c._server,
c._stop_event, and c._async_tasks to locate the spots to replace the silent
excepts with logged exceptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b69959e-62be-4af2-8c5c-6283090c1f83
📒 Files selected for processing (2)
src/server/master_server.cpptests/integration/test_persistent_cache.py
load_cache() already populates pch_states at startup, so the block that checked for PCH files on disk but not in memory was redundant. Worse, it could reuse a stale PCH without proper deps validation when no cache.json entry existed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Store all file paths in a shared paths[] array and reference them by index from deps and source_file fields. This avoids redundant storage when many PCH/PCM entries share the same header dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/integration/test_persistent_cache.py (2)
69-74: Keep the restart helper aligned with the shared test harness.
tests/conftest.py:273-304already owns transport selection and teardown, but_make_client()hardcodes--mode pipeand_shutdown_client()forks a second cleanup path. That makestest_pch_survives_server_restart()drift from the mode the rest of the suite is actually running under. Please thread the configured launch parameters through here or extract a reusable start/stop helper fromtests.conftest.Also applies to: 77-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 69 - 74, The helper _make_client (and its companion _shutdown_client) must stop hardcoding "--mode pipe" and instead reuse the shared transport/launch configuration from the test harness; either accept and forward the configured launch args (e.g. a parameter like launch_args or transport_args) into CliceClient.start_io inside _make_client, or call the existing start/stop helper in the test harness (from tests/conftest.py) so transport selection and teardown remain centralized; update test_pch_survives_server_restart (and the similar block at 77-96) to use the new parameterized/reusable start/stop helper so the test follows the suite-wide mode.
53-58: Add one real PCM cache scenario.This file introduces PCM cache helpers, but every scenario below still opens ordinary
#include-based.cppfiles. As written, the newBuildPCMpath,.clice/cache/pcm/reuse, and persistedcache.json["pcm"]metadata are never exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 53 - 58, The tests never exercise the PCM cache path; add a real PCM scenario that writes an actual .pcm file into the workspace PCM directory and populates the persisted cache metadata so BuildPCM logic runs: use the helper _list_pcm_files to assert presence, create the directory ".clice/cache/pcm/" under the test workspace, write a representative .pcm file there and update cache.json["pcm"] (or the test fixture that writes cache.json) to reference that file, then add an integration test that triggers the code paths that read cache.json and call BuildPCM to validate reuse/persistence of the PCM entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/stateless_worker.cpp`:
- Around line 79-95: The code writes PCH output directly to params.output_path
(via cp.output_file) and then removes it on failure, which can corrupt or delete
another process's artifact; change the flow in the compile/BuildPCH path (the
block using compile(cp, pch_info), LOG_INFO/LOG_WARN, BuildPCHResult,
pch_info.deps, and fs::remove(params.output_path)) to write to a unique
per-build temporary file (e.g., output_path + process/uuid + .tmp), set
cp.output_file to that temp, and after unit.completed() perform an atomic
fs::rename(temp, params.output_path) and only return success then; on failure
delete the temp only (not params.output_path) and apply the same staging+rename
fix to the other similar block (lines ~117-129) that handles PCH/PCM outputs.
In `@tests/integration/test_persistent_cache.py`:
- Around line 332-340: The test currently checks for leaked .tmp files in the
old directories by setting pch_dir and pcm_dir to tmp_path / ".clice" / "pch"
and "/pcm"; update those to the new layout by pointing pch_dir to tmp_path /
".clice" / "cache" / "pch" and pcm_dir to tmp_path / ".clice" / "cache" / "pcm"
(or check both old and new locations for backward compatibility) and then glob
for "*.tmp" as before to assert no stale temp files remain; use the existing
variable names pch_dir and pcm_dir so minimal edits are needed.
---
Nitpick comments:
In `@tests/integration/test_persistent_cache.py`:
- Around line 69-74: The helper _make_client (and its companion
_shutdown_client) must stop hardcoding "--mode pipe" and instead reuse the
shared transport/launch configuration from the test harness; either accept and
forward the configured launch args (e.g. a parameter like launch_args or
transport_args) into CliceClient.start_io inside _make_client, or call the
existing start/stop helper in the test harness (from tests/conftest.py) so
transport selection and teardown remain centralized; update
test_pch_survives_server_restart (and the similar block at 77-96) to use the new
parameterized/reusable start/stop helper so the test follows the suite-wide
mode.
- Around line 53-58: The tests never exercise the PCM cache path; add a real PCM
scenario that writes an actual .pcm file into the workspace PCM directory and
populates the persisted cache metadata so BuildPCM logic runs: use the helper
_list_pcm_files to assert presence, create the directory ".clice/cache/pcm/"
under the test workspace, write a representative .pcm file there and update
cache.json["pcm"] (or the test fixture that writes cache.json) to reference that
file, then add an integration test that triggers the code paths that read
cache.json and call BuildPCM to validate reuse/persistence of the PCM entry.
🪄 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: a507dc3c-2c15-463b-b682-11d7d9d89df9
📒 Files selected for processing (3)
src/server/master_server.cppsrc/server/stateless_worker.cpptests/integration/test_persistent_cache.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/master_server.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t tests - save_cache() now writes to .tmp then renames for crash safety - PCM filename hash incorporates compile arguments, not just file path - Remove per-build save_cache() calls; only save on exit - Fix test_no_tmp_files checking wrong cache directory paths - Add output_path to all unit tests that build PCH/PCM Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t tests - Remove CacheData member function/annotation (breaks serde aggregate requirement); use local index_map + lambda in save_cache() instead - Reorder startup: cleanup_cache() before load_cache() so stale files are removed before entries are loaded - Add save_cache() calls after PCH and PCM builds to persist immediately - Fix test_didchange_preamble_edit_recompiles: use incremental change with full-file range to work around eventide serde variant deserialization issue (whole-document changes parsed as partial) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/test_persistent_cache.py (2)
77-96: Consider logging exceptions in cleanup helper for debuggability.The
try-except-passblocks silence all exceptions during shutdown, which is acceptable for test cleanup but can make debugging difficult when shutdown actually fails unexpectedly.💡 Optional: Add minimal logging for debugging
async def _shutdown_client(c: CliceClient) -> None: """Gracefully shut down a client.""" try: await asyncio.wait_for(c.shutdown_async(None), timeout=5.0) - except Exception: - pass + except Exception as e: + # Swallow but optionally log for debugging + pass # or: print(f"shutdown_async failed: {e}", file=sys.stderr) try: c.exit(None) - except Exception: - pass + except Exception as e: + pass # or: print(f"exit failed: {e}", file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 77 - 96, The helper _shutdown_client currently swallows all exceptions with bare try/except-pass which hides failures; update _shutdown_client to catch exceptions around each operation (calls to c.shutdown_async, c.exit, c._server.kill, c._stop_event.set and cancelling c._async_tasks) and log the exception details using the module logger (e.g., logger = logging.getLogger(__name__)) or test logger before continuing, including context like which operation failed and the CliceClient identity so failures during shutdown are visible without changing control flow.
277-316: Clarify test expectation in comment.The comment on lines 310-315 explains that the preamble hash is based on source text (not header content), so the PCH filename stays the same but gets rebuilt due to deps change. This is accurate behavior — consider making the assertion more explicit.
💡 Optional: Make assertion intent clearer
pch_after = _list_pch_files(tmp_path) # The preamble content changed (`#include` "header.h" is the same text, # but the preamble hash is computed from the preamble TEXT in the source file, # not from the header content). Since the `#include` line is identical, # the preamble hash is the same → same PCH filename, but deps changed # so PCH gets rebuilt (overwritten at the same path). - # Either way, compilation should succeed. - assert len(pch_after) >= 1 + assert len(pch_after) >= 1, "PCH should still exist after rebuild" + # Verify compilation succeeded (no diagnostics from V2 usage)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_persistent_cache.py` around lines 277 - 316, The test comment is ambiguous about whether the PCH filename should remain the same; update test_pch_rebuilt_on_header_change to assert the intended behavior explicitly: after capturing pch_before via _list_pch_files(tmp_path) and pch_after via _list_pch_files(tmp_path) assert either that the filename sets are equal (if you want to enforce same-name overwrite) using the filenames from _list_pch_files, or assert that at least one file’s modification time changed (stat().st_mtime) to prove rebuild; keep the existing diagnostic checks using client.open_and_wait and reference the same uri/uri2, and add a brief clarifying comment above the new assertion explaining which of the two behaviors (same filename overwritten vs new filename created) the test expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/test_persistent_cache.py`:
- Around line 77-96: The helper _shutdown_client currently swallows all
exceptions with bare try/except-pass which hides failures; update
_shutdown_client to catch exceptions around each operation (calls to
c.shutdown_async, c.exit, c._server.kill, c._stop_event.set and cancelling
c._async_tasks) and log the exception details using the module logger (e.g.,
logger = logging.getLogger(__name__)) or test logger before continuing,
including context like which operation failed and the CliceClient identity so
failures during shutdown are visible without changing control flow.
- Around line 277-316: The test comment is ambiguous about whether the PCH
filename should remain the same; update test_pch_rebuilt_on_header_change to
assert the intended behavior explicitly: after capturing pch_before via
_list_pch_files(tmp_path) and pch_after via _list_pch_files(tmp_path) assert
either that the filename sets are equal (if you want to enforce same-name
overwrite) using the filenames from _list_pch_files, or assert that at least one
file’s modification time changed (stat().st_mtime) to prove rebuild; keep the
existing diagnostic checks using client.open_and_wait and reference the same
uri/uri2, and add a brief clarifying comment above the new assertion explaining
which of the two behaviors (same filename overwritten vs new filename created)
the test expects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 717ee68b-7c7b-409b-8f57-7af17d250d12
📒 Files selected for processing (6)
src/server/master_server.cpptests/integration/test_persistent_cache.pytests/integration/test_staleness.pytests/unit/server/module_worker_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/stateless_worker_tests.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/unit/server/pch_worker_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/master_server.cpp
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two bugs in stateless_worker.cpp: 1. CompilationUnit destructor calls EndSourceFile which serializes PCH/PCM to disk. Rename was attempted before destruction, so the .tmp file didn't exist yet. Fix: destroy unit before rename. 2. fs::rename returns std::expected<void, error_code> where operator bool() is true on success. Use it consistently via the if(ec)/else pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
changed the title
This was referenced Apr 5, 2026
Merged
This was referenced Apr 22, 2026
This was referenced Jun 29, 2026
This was referenced Jul 7, 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.
Summary
PCH and PCM artifacts are now cached to disk at
.clice/cache/{pch/,pcm/}with content-addressed filenames, so they survive server restarts. Dependency metadata is persisted incache.json(using eventide serde) with a shared path table for deduplication.Key changes
output_pathfield onBuildPCHParams/BuildPCMParamsso master specifies where workers write.tmp+fs::rename;CompilationUnitdestroyed before rename to flush the file to disk; fallback to temp file whenoutput_pathis empty (unit tests)PCMStatestruct,pcm_statesmap,load_cache()/save_cache()/cleanup_cache()methodscache.jsonon startup, save after each PCH/PCM build and on exit; deterministic path computation (xxh3preamble hash for PCH, module name + source path hash for PCM); stale files (>7 days) cleaned on startup;cache.jsonuses shared path table to avoid redundant storage of header paths across entriesfs::rename()helper;ThreadSafeFSbroadened to match.pchextension instead ofpreamble-prefixoutput_pathNaming scheme
.clice/cache/pch/<016x(xxh3(preamble))>.pch.clice/cache/pcm/<module_name>-<016x(xxh3(source_path))>.pcmTest plan
🤖 Generated with Claude Code