Skip to content

fix(server): bound loaded preamble states with an LRU - #521

Merged
16bit-ykiko merged 1 commit into
mainfrom
fix/preamble-state-lru
Jul 17, 2026
Merged

16bit-ykiko merged 1 commit into
mainfrom
fix/preamble-state-lru

Conversation

@16bit-ykiko

@16bit-ykiko 16bit-ykiko commented Jul 17, 2026 •

Copy link
Copy Markdown
Member

Background

The PCH cache pairs each preamble with an index blob holding the preamble's symbols, links and inactive regions. The blob is memory-mapped on first use — and then retained for the server's lifetime: closing the document, going idle, even the store evicting the pair from disk, none of it releases the mapping. Every distinct preamble key ever touched keeps tens of MB mapped (the mappings are file-backed and reclaimable under pressure, but the address space and the working set grow without bound, and evicted blobs cannot even free their disk space while a mapping pins them).

Changes

  • The workspace keeps a loaded-state LRU budgeted at open documents + 2 (the count provider is wired by the master; the two slots of slack keep a just-closed state warm for a quick reopen and a shared key warm across consumer churn). Opening or rebuilding a state touches the LRU; didClose shrinks the budget and enforces it. Unloading only drops the cache's reference — consumers hold shared_ptr copies and finish safely — and the next use reopens the blob from disk lazily.
  • The cache store now records the blobs its size budget evicts (eviction runs inside commit, possibly on a worker thread, so the store records instead of calling back) and the master drains the records on its periodic checkpoint task, dropping orphaned in-memory PCH metadata. A record is acted on only when the store still lacks the blob, so a key rebuilt after the eviction was recorded keeps its live entry, and an entry mid-rebuild is never touched.

Testing

  • Integration (tests/integration/server/test_memory_ownership.py): three documents with distinct preambles load three states and converge to the budget once all are closed (verified red before the fix — the count stayed at three forever); after the unload, reopening a file reloads the blob and queries keep working; four documents sharing one preamble keep sharing a single loaded state (regression guard).
  • Unit: the store's eviction reporting is pinned as exact and drain-once (extending the existing LRU eviction test).
  • Full suites green in RelWithDebInfo and Debug (1019 unit, 297 integration, 3/3 smoke). Debug's ASan caught — and the fix removed — a use-after-free in an early draft of the eviction recording (the key was copied after the entry erase).

Notes

  • A state unloaded by the budget pays its reload (mmap + flatbuffer verification) on the event loop at the next query. This only happens for keys beyond the open-document working set — open documents always fit the budget — and is the accepted cost of bounding the mappings; noted in the code.
  • The stats endpoint's loaded-state gauges (added with the flip-back PR) are what the new tests assert against.

Summary by CodeRabbit

  • Bug Fixes

    • Improved memory management for preamble data by automatically unloading least-recently-used entries when limits are reached.
    • Released cached preamble data promptly when documents are closed.
    • Kept cache metadata synchronized after underlying cache entries are evicted.
    • Preserved sharing of identical preamble data across multiple documents.
  • Tests

    • Added coverage for preamble release, reload behavior, shared state, and cache eviction tracking.

pch_cache's PreambleState blobs (mmap of the paired index, tens of
MB per distinct preamble key on real projects) were opened on first
use and retained for the server's lifetime: didClose, idle time and
store eviction all failed to release them, so memory grew with every
preamble key ever touched.

- Workspace keeps a loaded-state LRU, budgeted at open documents + 2
  (provider wired by the master; the slack keeps a just-closed state
  warm for quick reopen). Loads and rebuilds touch the LRU; didClose
  shrinks the budget and enforces it. Unloading only drops the cache's
  reference — consumers hold shared_ptr copies and finish safely, and
  the next use reopens the blob from disk.
- CacheStore records blobs its LRU evicts (copied before the entry
  erase — ASan caught a use-after-free in an earlier draft) and the
  master drains the records on its checkpoint task, dropping orphaned
  pch_cache metadata only when the store still lacks the blob, so a
  key rebuilt after the eviction keeps its live entry.

Tests: loaded states converge to the budget after closing documents
and queries survive the unload/reload cycle; identical preambles keep
sharing one loaded state; the store's eviction reporting is pinned
drain-once at unit level.
@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 74bb1510-ed7b-48e8-be8c-2cc7913d9c01

📥 Commits

Reviewing files that changed from the base of the PR and between ecd3043 and 03673ea.

📒 Files selected for processing (9)
  • src/server/compiler/compiler.cpp
  • src/server/state/workspace.cpp
  • src/server/state/workspace.h
  • src/server/transport/master_server.cpp
  • src/server/transport/master_server.h
  • src/support/cache_store.cpp
  • src/support/cache_store.h
  • tests/integration/server/test_memory_ownership.py
  • tests/unit/support/cache_store_tests.cpp

📝 Walkthrough

Walkthrough

The PR adds LRU tracking and budget enforcement for loaded preamble states, wires the budget to open sessions, and unloads excess states. CacheStore now reports evictions, allowing MasterServer to remove stale PCH metadata. Integration and unit tests cover release, sharing, and eviction draining.

Changes

Preamble lifecycle management

Layer / File(s) Summary
Loaded preamble state budget
src/server/state/workspace.*, src/server/compiler/compiler.cpp, src/server/transport/master_server.cpp, tests/integration/server/test_memory_ownership.py
Workspace tracks loaded preamble-state usage, enforces a budget based on open documents, and unloads least-recently-used states during access, PCH creation, and document closure. Integration tests cover unloading, reloading, and shared preambles.
PCH cache eviction reconciliation
src/support/cache_store.*, src/server/transport/master_server.*, tests/unit/support/cache_store_tests.cpp
CacheStore records and drains LRU eviction records, while MasterServer removes missing non-building PCH cache entries during checkpoint processing. Unit tests verify namespace/key reporting and single-use draining.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant MasterServer
  participant Workspace
  participant CacheStore
  Client->>MasterServer: open or close document
  MasterServer->>Workspace: update open-document budget
  Workspace->>Workspace: touch and enforce loaded-state LRU
  Workspace->>Workspace: unload excess PreambleState entries
  MasterServer->>CacheStore: checkpoint and take evictions
  CacheStore-->>MasterServer: evicted namespace/key records
  MasterServer->>Workspace: erase missing PCH cache metadata
Loading

Possibly related PRs

  • clice-io/clice#479: Related PCH compilation and workspace cache-state management changes.
  • clice-io/clice#501: Related loaded preamble-state handling for PCH-backed overlay queries.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: bounding loaded preamble states with an LRU in server code.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/preamble-state-lru

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@16bit-ykiko
16bit-ykiko merged commit dcf9288 into main Jul 17, 2026
22 checks passed
@16bit-ykiko
16bit-ykiko deleted the fix/preamble-state-lru branch July 17, 2026 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant