Repository navigation
feat(index): automatic flatbuffers serialization via kotatsu codec::fbs, drop flatc - #598
Conversation
…bs, drop flatc Index blobs are now serialized by reflecting the index types directly (kota::codec::fbs::to_bytes) and read back either eagerly (verified from_bytes: TUIndex, ProjectIndex) or through zero-copy views verified once at load (table_view: MergedIndex, PreambleState). schema.fbs, the flatc toolchain and all hand-written builder/GetRoot code are gone. - Relation::kind stores the raw RelationKind::Kind enum so Relation keeps its reflected memcpy image (struct vectors, map keys) - build-time-only fields (FileID-keyed maps) carry skip annotations and never reach the schema - repr specializations map Bitmap to its roaring image, SymbolKind to its underlying value, milliseconds to its count - TUIndex::from/ProjectIndex::from take sized buffers and verify before reading (closes the unverified GetRoot holes) - wire format break: cache_format_version 4 -> 5, index_format_version 1 -> 2, preamble_format_version 3 -> 4; old cache directories are swept and rebuilt
Code folds: TUIndex::from normalizes path_hashes to the path table's length (the verifier checks structure, not cross-field invariants); serialize() no longer wipes a deserialized index's path-keyed rows; shared deserialize_blob/scan_occurrences_at helpers replace triplicated decode calls and duplicated binary searches; integration tooling's cache root caught up to v5; stale flatc-era comments trimmed. Tests: TUIndex direct round-trip (multi-file, built_at, symbols, path_file_indices), hostile-input rejection for every loader (garbage, truncation, identifier clobber), version-gate positive controls pinning slot alignment, buffer-path relation lookup parity with the impl path, multi-occurrence binary search, ProjectIndex out-of-range local-id guard, Indexer::merge garbage rejection.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR removes FlatBuffers generation and migrates index, preamble, project, and translation-unit persistence to Kotatsu codec serialization with verified views. It updates loading APIs, cache versions, relation handling, and corruption tests. ChangesCodec-backed index persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to This change replaces the index persistence format and verification paths, but malformed or incompatible cache data can still trigger out-of-bounds writes, excessive allocations, unsafe decoding, missed occurrence results, or invalid path handling. These current-head risks can cause crashes, corrupted index state, or incorrect queries, so the PR should not merge until the affected loaders and lookup paths are hardened. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 4
🧹 Nitpick comments (3)
tests/unit/index/preamble_state_tests.cpp (2)
244-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExercise the query APIs on the minimal blob.
AcceptCurrentVersionBlobproves that a blob carrying onlyformat_versionloads. Production code therefore hands out aPreambleStatewhosepaths,files,preamble,symbols,links,inactive_regionsandopen_conditionalsfields are all absent. No test queries that state.
preamble_content()and bothlookup_preambleoverloads readroot[&PreambleStateRepr::preamble]and then index into a nested table that was never written. Add calls to every accessor on this state, so the "absent field reads back empty" assumption is asserted rather than assumed.🧪 Proposed additional assertions
dir.touch("current.pch.idx", llvm::StringRef(reinterpret_cast<const char*>(blob->data()), blob->size())); - EXPECT_TRUE(index::PreambleState::load(dir.path("current.pch.idx")) != nullptr); + auto minimal = index::PreambleState::load(dir.path("current.pch.idx")); + ASSERT_TRUE(minimal != nullptr); + + // Every field is absent: each accessor must answer empty, not crash. + EXPECT_TRUE(minimal->source_path().empty()); + EXPECT_TRUE(minimal->preamble_content().empty()); + EXPECT_TRUE(minimal->links().empty()); + EXPECT_TRUE(minimal->inactive_regions().empty()); + EXPECT_TRUE(minimal->open_conditionals().empty()); + + std::string name; + SymbolKind kind; + EXPECT_FALSE(minimal->find_symbol(42, name, kind)); + + minimal->lookup(42, RelationKind::Reference, [](auto&, auto&) { return true; }); + minimal->lookup_preamble(0, [](const index::Occurrence&) { return true; }); + minimal->lookup_preamble(42, RelationKind::Reference, [](const index::Relation&) { + return true; + });🤖 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 `@tests/unit/index/preamble_state_tests.cpp` around lines 244 - 258, Extend AcceptCurrentVersionBlob to exercise every accessor on the minimally encoded PreambleState, including paths, files, preamble_content(), symbols, links, inactive_regions, open_conditionals, and both lookup_preamble overloads. Assert that each absent field or lookup result returns its documented empty/null result without crashing, using the existing state returned by PreambleState::load.
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the duplicated
VersionOnlyfixture to the enclosing test-suite scope and reuse it in both cases. The same cleanup applies to the duplicate fixture intests/unit/index/merged_index_tests.cpp, keeping the positive-control blob definitions consistent and easier to maintain.🤖 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 `@tests/unit/index/preamble_state_tests.cpp` around lines 231 - 233, Move the duplicated VersionOnly struct declarations out of the individual test cases and define one shared VersionOnly type at the test suite scope. Update both test cases to reuse this shared declaration without changing their behavior. Apply the same fix in `@tests/unit/index/merged_index_tests.cpp` around lines 681 - 718: The same duplicated fixture and remediation are present in this test file.src/index/preamble_state.cpp (1)
48-59: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMove the
path_idbound check intofile_of.
file_ofindexespaths[entry[&PreambleFileEntryRepr::path_id]]without a bound check. The only caller checks the bound at Line 155. A future caller (for example, one that materializes thepreambleentry) would read out of range on a corrupt blob. Put the check insidefile_ofand return an optional, so the invariant travels with the code that depends on it.Note that
serializewritesrepr.preamble.path_idaspaths.size() - 1(Line 99), which underflows whenindex.graph.pathsis empty. That value is not read today, which keeps the current impact at zero.🤖 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/index/preamble_state.cpp` around lines 48 - 59, Update file_of to validate the entry path_id against paths.size() before indexing, and return an optional PreambleState::File that is empty for invalid IDs. Adjust its caller(s), including the path around the existing Line 155 check, to consume the optional and preserve current behavior without duplicating the bound check.
🤖 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/index/merged_index.cpp`:
- Around line 343-380: In the merged-index load path, validate every
include.canonical_id and context.canonical_id against
canonical_ref_counts.size() before incrementing it, rejecting the blob through
the existing load/error mechanism rather than indexing invalid values. Replace
the assert-only handling of deserialize_blob in deserialize/load flow with an
explicit failure check that prevents partially decoded repr data from being
moved into Impl in release builds.
In `@src/index/serialization.h`:
- Around line 110-124: Update the binary search in the surrounding serialization
lookup method to order candidates by range.begin, finding the upper bound where
range.begin is less than or equal to offset instead of comparing range.end. Then
inspect all preceding candidates so containing ranges are not skipped, while
preserving the existing range.contains(offset) matching behavior; add a
regression test covering nested ranges such as [0,100] and [1,2].
In `@src/index/tu_index.cpp`:
- Around line 654-662: Update TUIndex and its serialization/deserialization flow
to persist and validate index_format_version, matching the gate used by
ProjectIndex::from and the contract in serialization.h. Write the current
version when serializing, reject mismatched versions by returning std::nullopt
from TUIndex::from before normalization, and add coverage for a stale but
structurally valid blob.
- Around line 654-662: Update TUIndex::from to validate decoded
path_file_indices keys against the graph path table bounds before returning the
index, rejecting or removing rows with out-of-range IDs so Indexer::merge cannot
perform unchecked access. Add a hostile-input test covering an invalid decoded
path ID and verify deserialization fails or safely excludes the invalid row.
---
Nitpick comments:
In `@src/index/preamble_state.cpp`:
- Around line 48-59: Update file_of to validate the entry path_id against
paths.size() before indexing, and return an optional PreambleState::File that is
empty for invalid IDs. Adjust its caller(s), including the path around the
existing Line 155 check, to consume the optional and preserve current behavior
without duplicating the bound check.
In `@tests/unit/index/preamble_state_tests.cpp`:
- Around line 244-258: Extend AcceptCurrentVersionBlob to exercise every
accessor on the minimally encoded PreambleState, including paths, files,
preamble_content(), symbols, links, inactive_regions, open_conditionals, and
both lookup_preamble overloads. Assert that each absent field or lookup result
returns its documented empty/null result without crashing, using the existing
state returned by PreambleState::load.
- Around line 231-233: Move the duplicated VersionOnly struct declarations out
of the individual test cases and define one shared VersionOnly type at the test
suite scope. Update both test cases to reuse this shared declaration without
changing their behavior.
Apply the same fix in `@tests/unit/index/merged_index_tests.cpp` around lines 681
- 718: The same duplicated fixture and remediation are present in this test
file.
🪄 Autofix
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 Plus
Run ID: af5e08bc-df44-4197-b4e3-f5ce6f9ee517
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpixi.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
CMakeLists.txtcmake/package.cmakepixi.tomlsrc/index/include_graph.hsrc/index/merged_index.cppsrc/index/preamble_state.cppsrc/index/preamble_state.hsrc/index/project_index.cppsrc/index/project_index.hsrc/index/schema.fbssrc/index/serialization.hsrc/index/tu_index.cppsrc/index/tu_index.hsrc/server/compiler/compiler.cppsrc/server/compiler/indexer.cppsrc/server/service/query.cppsrc/server/state/workspace.htests/unit/index/merged_index_tests.cpptests/unit/index/persisted_index_tests.cpptests/unit/index/preamble_state_tests.cpptests/unit/index/project_index_tests.cpptests/unit/index/tu_index_tests.cpptests/unit/server/indexer_tests.cpptools/client/workspace.ts
💤 Files with no reviewable changes (2)
- src/index/schema.fbs
- pixi.toml
TUIndex now stamps and gates index_format_version: the blob is IPC-only, but a worker respawned after an on-disk binary upgrade can run one build ahead of the server, and a layout change need not be structurally detectable. from() also rejects blobs whose location path ids, file index keys or symbol bitmap values fall outside the blob's own path table, and MergedIndex::load_in_memory range-validates every decoded canonical id (cache entries, header includes, compilation contexts) instead of assert-only decode checking — the verifier checks structure, not cross-field invariants, and consumers index ref-count vectors with these values unchecked. Binary-searching occurrences on range.end is documented sound: name-token spans are pairwise disjoint or identical.
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 (2)
src/index/merged_index.cpp (2)
381-393: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound
max_canonical_idbefore resizingcanonical_ref_counts.A structurally valid blob can set
max_canonical_idtoUINT32_MAXwhile leavingcanonical_cache, header contexts, and compilation contexts empty. It passesusable(), then Line 393 attempts to allocate about 16 GiB forstd::uint32_tentries. An uncaught allocation failure can terminate the server when it loads a corrupt shard.Reject values above an explicit resource limit before
resize, or compact and remap canonical IDs during serialization so the persisted maximum remains bounded.🤖 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/index/merged_index.cpp` around lines 381 - 393, Validate index.max_canonical_id against an explicit resource limit immediately after assigning it from repr.max_canonical_id and before canonical_ref_counts.resize. Reject or otherwise fail safely when the bound is exceeded, while preserving normal loading for valid blobs and avoiding the unbounded allocation path.
301-304: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftValidate
MergedIndex(llvm::StringRef)before storing the buffer.The public constructor stores arbitrary bytes, but buffer-backed queries use
ShardView::from_verified_bytes. Validate the bytes withShardView::from_bytesand requireindex_format_version, or make the constructor private.🤖 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/index/merged_index.cpp` around lines 301 - 304, Update the public MergedIndex(llvm::StringRef) constructor to validate the supplied bytes with ShardView::from_bytes and require index_format_version before storing the buffer, ensuring root_of uses only validated data; alternatively, make the constructor private if callers must use an already-validated construction path.
🤖 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/index/merged_index.cpp`:
- Around line 381-393: Validate index.max_canonical_id against an explicit
resource limit immediately after assigning it from repr.max_canonical_id and
before canonical_ref_counts.resize. Reject or otherwise fail safely when the
bound is exceeded, while preserving normal loading for valid blobs and avoiding
the unbounded allocation path.
- Around line 301-304: Update the public MergedIndex(llvm::StringRef)
constructor to validate the supplied bytes with ShardView::from_bytes and
require index_format_version before storing the buffer, ensuring root_of uses
only validated data; alternatively, make the constructor private if callers must
use an already-validated construction path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b37a9a61-b3be-4682-9bf2-d62b85038c66
📒 Files selected for processing (6)
src/index/merged_index.cppsrc/index/serialization.hsrc/index/tu_index.cppsrc/index/tu_index.htests/unit/index/merged_index_tests.cpptests/unit/index/tu_index_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index/tu_index.cpp
- tests/unit/index/merged_index_tests.cpp
The fbs blob schemas are now the native structs themselves instead of field-by-field mirror copies: - MergedIndex::Impl doubles as the shard schema: runtime-only fields are skip-annotated, serialize() compacts masked rows in place and reflects the impl straight onto the wire, load decodes into it directly. - PreambleState::serialize takes the TUIndex by value and assembles the blob by moving its rows; file contents and feature arrays are encoded as StringRef/ArrayRef borrows. Symbols persist as the full SymbolTable. - ProjectIndex persists its symbol bitmaps with raw pool ids plus an id-to-path table (still garbage-collected), remapped at load; the per-symbol bitmap rebuild on the encode side is gone. - PathPool repr drives the visitor imperatively (zero-copy encode, interning decode); new StringMap repr for the canonical cache. index_format_version 2 -> 3, preamble_format_version 4 -> 5.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index/serialization.h (1)
39-41: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse
roaring_bitmap_deserialize_safefor the non-portable image.
Bitmap::readperforms unbounded reads, and FBS verification does not validate the embedded bitmap. Callroaring_bitmap_deserialize_safewithbuffer.size(), check fornullptr, and reject invalid images before constructing the index. Do not use C++readSafedirectly because it only supports the portable format. Add a corrupt-bitmap FBS fixture.🤖 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/index/serialization.h` around lines 39 - 41, Update Bitmap::from to validate the non-portable serialized image with roaring_bitmap_deserialize_safe, passing buffer.data() and buffer.size(), and reject a nullptr result before constructing the clice::Bitmap. Keep the existing non-portable format handling and avoid the C++ readSafe API; add a corrupt-bitmap FBS fixture covering the rejection path.
🤖 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/index/project_index.cpp`:
- Around line 69-73: Validate each entry in the project-index path remapping
loop before calling pool.intern, rejecting an empty path by returning nullopt
through the existing cache-loading failure path. Update the project-index
corruption tests to cover a codec-valid paths table containing an empty entry
and assert loading fails cleanly.
---
Outside diff comments:
In `@src/index/serialization.h`:
- Around line 39-41: Update Bitmap::from to validate the non-portable serialized
image with roaring_bitmap_deserialize_safe, passing buffer.data() and
buffer.size(), and reject a nullptr result before constructing the
clice::Bitmap. Keep the existing non-portable format handling and avoid the C++
readSafe API; add a corrupt-bitmap FBS fixture covering the rejection path.
🪄 Autofix
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 Plus
Run ID: 90723869-7556-481e-a681-4f9669ae07ef
📒 Files selected for processing (11)
src/index/merged_index.cppsrc/index/merged_index.hsrc/index/preamble_state.cppsrc/index/preamble_state.hsrc/index/project_index.cppsrc/index/project_index.hsrc/index/serialization.hsrc/server/worker/stateless_worker.cpptests/unit/index/merged_index_tests.cpptests/unit/index/persisted_index_tests.cpptests/unit/index/preamble_state_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/index/preamble_state.cpp
- tests/unit/index/persisted_index_tests.cpp
- src/index/merged_index.cpp
- tests/unit/index/merged_index_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: 0c6f2814a7
ℹ️ 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".
Review round: the preamble blob stores a borrowed {name, kind} entry per
symbol instead of reflecting the full Symbol (scope and reference bitmaps
were dead weight on the wire), and ProjectIndex::from rejects a corrupt
blob whose path table carries an empty entry instead of interning it.
1 participant
What
Index blobs are now serialized by reflecting the index types directly through kotatsu's
codec::fbsbackend.schema.fbs, the flatc toolchain (CMake custom command + pixiflatbuffersdependency) and all hand-written builder/GetRootcode are gone.Per blob:
from_bytes;TUIndex::fromnow takes a sized buffer and verifies before reading — closing the unverified-GetRoot-without-length holes on the worker→master path.table_view::from_bytes) and queried through unchecked per-query views (from_verified_bytes). Hand-rolled sorted-vector binary searches becamemap_viewlookups — the encoder sorts entries by the same canonical ordering the view searches with, so relations index asDenseMap<SymbolHash, DenseMap<Relation, Bitmap>>and occurrences asDenseMap<Occurrence, Bitmap>(struct keys) directly.No mirror structs. The persisted schema of every blob is the native type itself — there is no parallel
…Reprstruct copied field by field at serialize time:MergedIndex::Impldoubles as the shard schema: runtime-only fields (canonical_ref_counts,removed,occurrences_cache) are skip-annotated and occupy no slot,serialize()compacts removal-masked rows in place and reflects the impl straight onto the wire, and loading decodes into the impl directly. The zero-copy query views readtable_view<MergedIndex::Impl>.PreambleState::serializetakes itsTUIndexby value and assembles the blob by moving the rows out of it; per-file contents and the feature arrays (links, inactive regions, open conditionals) are encoded asStringRef/ArrayRefborrows of the compilation's buffers and the caller's arrays. Symbols persist as borrowed{name, kind}entries —find_symbolserves nothing else, and reflecting the fullSymbolwould drag every symbol's scope and reference bitmap into large preamble blobs.ProjectIndexreflects itself: symbol bitmaps persist their raw pool ids, and the blob stays self-contained through a pool-id→path table (still written garbage-collected: only ids referenced by a symbol or shard). Loading interns the table and remaps every id in place — the per-symbol bitmap rebuild on the encode side is gone.Type-level changes:
Relation::kindstores the rawRelationKind::Kindenum — the wrapper's constructors hide it from reflection, and reflection is what keeps relation vectors as contiguous struct vectors andRelationvalid as a map key.IncludeGraph::file_table,TUIndex::file_indices— both keyed byclang::FileID, meaningless outside the compilation) carryKOTATSU_ANNOTATE(skip = true)and never reach the schema; the annotation wrapper inherits the map type, so call sites are untouched.Bitmapto its roaring image,SymbolKindto its underlying value,std::chrono::millisecondsto its count.PathPooluses the imperative repr form, driving the visitor directly: encode writes the internedStringRefs straight to the wire, decode interns one path at a time (rejecting empty paths). A newllvm::StringMap<uint32_t>repr persists the canonical cache as id-sorted key/value pairs — the repr is format-agnostic because the schema layer classifies fields without a format tag, andStringMapEntryis unclassifiable.Wire format break (sanctioned):
cache_format_version4→5 (old cache directories are swept wholesale),index_format_version1→3,preamble_format_version3→5. kotatsu-encoded blobs carry the codec's file identifier, so pre-migration blobs are rejected at verification regardless of version fields.Hardening folded from self-review
TUIndex::fromnormalizespath_hashesto the path table's length (the verifier checks structure, not cross-field invariants; consumers index it by path id).TUIndex::serializeno longer wipes a deserialized index's path-keyed rows on re-serialization.Indexer::mergeignores unverifiable payloads; a compile result whose index fails verification drops the session index (honest gap) instead of UB.All suites green: unit 1186, integration 341 (cache-root-sensitive tests explicitly verified against the v5 directory), smoke 3, snap 395.
Requires kotatsu ≥ 5232e67 (#201: reflected struct map keys, trivially-copyable inline structs,
from_verified_bytes); the pin is bumped accordingly.Fixes #594 — every loader now verifies through kotatsu's verifier, whose table budget scales with blob size by construction (
max_tables = size/4 + 16; a wire table occupies at least 4 bytes, so no valid blob can exceed it). No fixed-budget verifier remains, including the hand-tuned1<<26the preamble loader carried.