fix(index): flip shards back to disk blobs after save - #520
Merged
Conversation
Any merge materialized a shard's heap Impl and dropped its mmap buffer permanently: memory grew one-way until shutdown (~100KB per synthetic file, MB-level on llvm-scale shards), and need_rewrite() kept claiming every touched shard dirty forever, so each save rewrote the whole namespace and shutdown paid a full serialize. - MergedIndex carries a mutation stamp drawn from a process-wide monotonic source (values never repeat across objects, so an entry erased and re-created at the same key cannot alias a snapshot taken before the swap). Every merge/remove reassigns it. - Indexer::save() reopens each committed blob inside the commit's thread-pool job (mmap + flatbuffer verification stay off the event loop) and flips the shard back to the buffer-backed reload — only when the shard still exists and its stamp is unchanged across the commit await; a merge that landed meanwhile keeps the shard dirty for the next save instead of being silently dropped. - need_rewrite() is now a true dirty set: an incremental round saves only the shards its merges touched, and a round with nothing to write commits zero blobs. - New TEST-ONLY clice/internal/stats endpoint exposes ownership gauges (loaded preamble states/bytes, in-memory shards/bytes, last save's committed shard count, in-flight tmp blobs, trend gauges) so memory-lifecycle regressions are pinned by deterministic counters instead of RSS assertions. Tests: unit RevisionAndFlipBack + SaveFlipsShards + MidSaveMergeKept (the mid-save merge interleaving pins the data-loss guard); integration flip-back, dirty-set-only save, no-op save commits zero, cancel-storm leaves no tmp blobs.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe change adds mutation revisions to Index persistence and lifecycle observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Indexer
participant MergedIndex
participant CacheStore
Indexer->>MergedIndex: serialize dirty shard and record revision
Indexer->>CacheStore: commit pending shard blob
CacheStore-->>Indexer: return committed shard path
Indexer->>MergedIndex: reload committed shard
Indexer->>MergedIndex: flip back only when revision matches
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 |
![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 (1)
src/server/transport/lsp_client.cpp (1)
625-631: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGate the test-only stats endpoint behind
project.test_hooks.This endpoint is registered in production despite being test-only and performs synchronous workspace/directory scans. Apply the same guard used by
clice/internal/logFlood, then enabletest_hooksin the integration-test initialization.Proposed guard
"clice/internal/stats", [this](RequestContext& ctx, const ext::StatsParams&) -> RawResult { + if(!this->server.workspace.config.project.test_hooks.value_or(false)) { + co_return kota::outcome_error( + kota::ipc::Error{protocol::ErrorCode::InvalidRequest, + "test hooks are not enabled"}); + } auto& srv = this->server;🤖 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/transport/lsp_client.cpp` around lines 625 - 631, Guard registration of the clice/internal/stats request in the same project.test_hooks condition used by clice/internal/logFlood, so production projects do not expose or execute this test-only endpoint. Update integration-test initialization to enable project.test_hooks, preserving stats endpoint availability in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/compiler/indexer.cpp`:
- Around line 269-272: Move the saved_shards reset from the Phase 2 commit
section to the beginning of save(), before any serialization or other
early-return paths. Preserve the existing counter behavior while ensuring failed
ProjectIndex serialization cannot expose the previous successful save count.
---
Nitpick comments:
In `@src/server/transport/lsp_client.cpp`:
- Around line 625-631: Guard registration of the clice/internal/stats request in
the same project.test_hooks condition used by clice/internal/logFlood, so
production projects do not expose or execute this test-only endpoint. Update
integration-test initialization to enable project.test_hooks, preserving stats
endpoint availability in tests.
🪄 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: ccac8f6b-2f80-4d3d-98be-7ab046cb680f
📒 Files selected for processing (13)
src/index/merged_index.cppsrc/index/merged_index.hsrc/index/preamble_state.hsrc/server/compiler/indexer.cppsrc/server/compiler/indexer.hsrc/server/protocol/extension.hsrc/server/transport/lsp_client.cppsrc/support/cache_store.cppsrc/support/cache_store.htests/integration/server/test_memory_ownership.pytests/tools/client.pytests/unit/index/merged_index_tests.cpptests/unit/server/indexer_tests.cpp
A ProjectIndex serialization or store failure left the previous round's committed count exposed as current; a failed save committed nothing and must read back as zero.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Index shards are served from memory-mapped blobs, but any merge materializes a shard's in-memory form and drops the mapped buffer permanently. Two consequences:
Changes
MergedIndexnow carries a mutation stamp drawn from a process-wide monotonic source. Values never repeat across objects, so an entry erased and re-created at the same key while a save is mid-flight cannot alias a snapshot taken before the swap (a per-object counter restarting from zero could).Indexer::save()reopens each committed blob inside the same thread-pool job as the commit — the mmap and the flatbuffer verification walk the whole file and stay off the event loop — and flips the shard back to the buffer-backed reload, but only when the shard still exists and its stamp is unchanged across the commit await. A merge that landed meanwhile keeps the shard dirty for the next save instead of being silently overwritten by the stale blob.clice/internal/statsrequest (absent from capabilities, same test-hook family asclice/internal/poll) exposes ownership gauges: loaded preamble states and their mapped bytes, in-memory shards and their content bytes, the last save's committed shard count, in-flight cache tmp blobs, and trend counters. Memory-lifecycle regressions are pinned by deterministic counters instead of brittle RSS assertions.Testing
MergedIndex.RevisionAndFlipBack(stamp semantics, serialized twin is clean and equal),IndexerMerge.SaveFlipsShards(save flips a committed shard back and it still answers),IndexerMerge.MidSaveMergeKept(a merge interleaved into save's commit await is not lost: the shard keeps the newer content, stays dirty, and the next save writes it — the data-loss guard, exercised deterministically on the event loop).tests/integration/server/test_memory_ownership.py): shards flip back after a settled round; an incremental change saves exactly one shard; a round with nothing to write commits zero; a 15-edit preamble supersession storm leaves zero in-flight tmp blobs; plus shape smoke for the gauges.Notes
pending_tmp_filesscans the store's tmp directory under the store mutex, which is fine for a test hook but not meant for hot paths.Summary by CodeRabbit
New Features
Bug Fixes
Performance
Tests