fix(release): v4.8.3 — nuclear-rebuild + smart-reindex hardening (closes #161, #162, #163) - #169
Merged
Merged
Conversation
Fixes 3 GitHub issues + 5 additional latent bugs found while triaging production reproductions on a 5889-doc / 75016-chunk corpus. GitHub issues addressed: - Fixes #161 (grishkovei) - Zero-downtime rebuild no longer rebinds self.collection to empty staging during populate. Writes route through new _write_collection property (returns _staging_target when set, else production); reads on self.collection stay production. Nuclear rebuild now truly zero-downtime. - Fixes #162 (grishkovei) - Smart-reindex checkpoint serializes only doc-ids the current run committed (new tracking['committed_this_run'] set) rather than list(self._indexed_docs.keys()). Changed-but-not-yet- reached docs are no longer silently skipped on resume. - Fixes #163 (grishkovei) - reindex_documents(force=True) now propagates to index_all(force=True). The migration guide's force=True path was silently a no-op because reindex_all() hardcoded force=False. Internal fixes: - _index_lock moved from class-level to instance-level. Class singleton was silently shared across nuclear_rebuild staging orchestrators; if a staging orch was GC'd while holding the lock, every subsequent reindex returned reindex_already_running forever until daemon restart. - _ensure_bm25_initialized and _iter_chroma_chunks_for_fts5 batch-read Chroma via offset+limit=500. Previous collection.get(limit=count) blew past SQLite max_variable_number=999 with 'Internal error: too many SQL variables' for any user with >999 chunks running rebuild. - Fts5LexicalIndex.is_ready() re-reads marker when cached _ready is False. Post-swap orchestrators cached _ready=False forever and never re-checked the marker, permanently silencing the fast-path. - _maybe_start_fts5_migration cross-checks FTS5 count vs Chroma count (new _fts5_marker_matches_reality helper). Stale complete marker on a near-empty FTS5 no longer leaves fast-path silent forever. - _format_fts5_results skips orphan hits where FTS5 has chunk_id but Chroma doesn't. Previously emitted empty result items with source='' that broke reranking and polluted result lists. Also adds Fts5LexicalIndex.count() (used by the new sanity check). Test count baseline: 314 -> 322 (+8, one regression test per fix in tests/test_v483_hotfix.py). Each test literally reproduces the pre-fix bug so post-fix passes assert the correction. Version bump: 4.8.2 -> 4.8.3 (atomic across pyproject.toml, mcp_server/__init__.py, npm/package.json). All fixes verified end-to-end against a 75016-chunk corpus with the newly-added 2052 LOL* catalog documents (GTFOBins, LOLDrivers, HijackLibs, LOOBins, LOLESXi, LOFL, LOLC2, LOTP, Bootloaders.io). Nuclear rebuild now populates FTS5 with 75016 rows on first attempt.
📝 WalkthroughWalkthroughVersion 4.8.3 hardens zero-downtime rebuilds, reindex checkpointing, forced re-embedding, ChromaDB batching, FTS5 migration validation, and orphan-result filtering. It adds regression coverage and release documentation. ChangesIndexing and FTS5 hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KnowledgeOrchestrator
participant ChromaDB
participant ProductionCollection
participant StagingCollection
KnowledgeOrchestrator->>StagingCollection: route rebuild writes
KnowledgeOrchestrator->>ChromaDB: populate staging
KnowledgeOrchestrator->>ProductionCollection: serve production reads
KnowledgeOrchestrator->>StagingCollection: swap after validation
sequenceDiagram
participant KnowledgeOrchestrator
participant Fts5LexicalIndex
participant ChromaDB
KnowledgeOrchestrator->>Fts5LexicalIndex: validate migration marker
Fts5LexicalIndex->>Fts5LexicalIndex: count indexed rows
KnowledgeOrchestrator->>ChromaDB: retrieve 500-row corpus batches
KnowledgeOrchestrator->>Fts5LexicalIndex: populate validated batches
KnowledgeOrchestrator->>ChromaDB: hydrate result chunk IDs
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+2581
to
+2590
| order = sorted(range(len(ids)), key=lambda i: ids[i]) | ||
| for i in order: | ||
| meta = metas[i] or {} | ||
| yield ( | ||
| str(ids[i]), | ||
| str(docs[i] or ""), | ||
| str(meta.get("filename", "") or ""), | ||
| str(meta.get("category", "") or ""), | ||
| ) | ||
| offset += len(ids) |
There was a problem hiding this comment.
If the Chroma corpus changes during an interrupted or resumed migration, sorting only within each offset page does not preserve the global prefix that ordinal resume skips, causing chunks to be omitted or inserted twice before the FTS5 migration is marked complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/server.py
Line: 2581-2590
Comment:
**Resume loses stable migration order**
If the Chroma corpus changes during an interrupted or resumed migration, sorting only within each offset page does not preserve the global prefix that ordinal resume skips, causing chunks to be omitted or inserted twice before the FTS5 migration is marked complete.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/test_v483_hotfix.py (1)
50-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for BM25 batching.
This test only exercises
_iter_chroma_chunks_for_fts5. Add a test for_ensure_bm25_indexwith more than 500 chunks. Assert that everycollection.get()call uses the bounded limit and that BM25 receives all rows.🤖 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/test_v483_hotfix.py` around lines 50 - 79, Add a test covering _ensure_bm25_index with a simulated collection containing more than 500 chunks. Track collection.get calls, assert every call uses the bounded batch limit, and verify the BM25 index receives all corpus rows; reuse the existing batching-test setup patterns without changing the FTS5 test.
🤖 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 `@mcp_server/fts5_index.py`:
- Around line 184-188: Move the self._conn is None check in count() inside the
_fts5_lock context, before executing the count query. Ensure both reading the
connection and using it are synchronized with close(), preserving the existing
zero return for a closed connection.
- Line 188: Update the count query in the method containing the fts5_documents
lookup to count DISTINCT chunk_id values instead of rows, so duplicate chunks
from resumed insertion cannot inflate marker validation. Preserve the existing
integer return behavior and query execution flow.
In `@mcp_server/server.py`:
- Around line 2120-2142: The _populate_staging method exposes partial staging
BM25 and metadata state through shared query fields; build those values in
separate local staging state and publish them only after the collection swap
succeeds, preserving production query behavior throughout population. Update
tests/test_v483_hotfix.py lines 110-131 to pause staging population and verify
concurrent queries use the production BM25 index and production source lookup.
In `@README.md`:
- Line 1457: Update the production-validation sentence in the README to
accurately match the documented fixes: either explicitly identify the six fixes
included in validation or change “All 6 core fixes” to the correct count based
on the three linked and five additional fixes.
In `@tests/test_v483_hotfix.py`:
- Around line 37-42: Update the test around KnowledgeOrchestrator construction
to avoid patching out __init__ or manually assigning _index_lock; mock only the
constructor’s external initialization dependencies, create two real instances,
and assert their instance-owned _index_lock objects are distinct.
---
Nitpick comments:
In `@tests/test_v483_hotfix.py`:
- Around line 50-79: Add a test covering _ensure_bm25_index with a simulated
collection containing more than 500 chunks. Track collection.get calls, assert
every call uses the bounded batch limit, and verify the BM25 index receives all
corpus rows; reuse the existing batching-test setup patterns without changing
the FTS5 test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 983d991c-c409-444e-a334-90869b6a3df6
📒 Files selected for processing (8)
.github/test-count-baseline.txtREADME.mdmcp_server/__init__.pymcp_server/fts5_index.pymcp_server/server.pynpm/package.jsonpyproject.tomltests/test_v483_hotfix.py
Comment on lines
+184
to
+188
| if self._conn is None: | ||
| return 0 | ||
| with self._fts5_lock: | ||
| try: | ||
| return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Move the closed-connection check inside _fts5_lock.
close() sets self._conn to None while holding _fts5_lock. If count() passes Line 184 before close() acquires the lock, count() later calls execute() on None and raises AttributeError. Acquire the lock before reading self._conn.
Proposed fix
- if self._conn is None:
- return 0
with self._fts5_lock:
+ conn = self._conn
+ if conn is None:
+ return 0
try:
- return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0])
+ return int(conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self._conn is None: | |
| return 0 | |
| with self._fts5_lock: | |
| try: | |
| return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0]) | |
| with self._fts5_lock: | |
| conn = self._conn | |
| if conn is None: | |
| return 0 | |
| try: | |
| return int(conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0]) |
🤖 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 `@mcp_server/fts5_index.py` around lines 184 - 188, Move the self._conn is None
check in count() inside the _fts5_lock context, before executing the count
query. Ensure both reading the connection and using it are synchronized with
close(), preserving the existing zero return for a closed connection.
| return 0 | ||
| with self._fts5_lock: | ||
| try: | ||
| return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Count distinct chunks for marker validation.
tests/test_v483_hotfix.py Lines 206-224 compare this result with collection.count(). The migration commits a batch before writing its checkpoint at Lines 407-411. If the process stops in that gap, resume can insert the committed batch again. COUNT(*) then overstates coverage and can mark an incomplete index as credible. Count DISTINCT chunk_id values, or make resumed insertion idempotent.
Proposed query change
- return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0])
+ return int(
+ self._conn.execute(
+ "SELECT count(DISTINCT chunk_id) FROM fts5_documents"
+ ).fetchone()[0]
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return int(self._conn.execute("SELECT count(*) FROM fts5_documents").fetchone()[0]) | |
| return int( | |
| self._conn.execute( | |
| "SELECT count(DISTINCT chunk_id) FROM fts5_documents" | |
| ).fetchone()[0] | |
| ) |
🤖 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 `@mcp_server/fts5_index.py` at line 188, Update the count query in the method
containing the fts5_documents lookup to count DISTINCT chunk_id values instead
of rows, so duplicate chunks from resumed insertion cannot inflate marker
validation. Preserve the existing integer return behavior and query execution
flow.
Comment on lines
+2120
to
+2142
| _saved = { | ||
| "collection": self.collection, | ||
| "bm25_index": self.bm25_index, | ||
| "_bm25_initialized": self._bm25_initialized, | ||
| "_indexed_docs": dict(self._indexed_docs), | ||
| "_source_to_docid": dict(self._source_to_docid), | ||
| } | ||
| self.collection = staging | ||
| self._staging_target = staging # write dispatch redirects here | ||
| self.bm25_index = BM25Index() | ||
| self._bm25_initialized = True # suppress lazy rebuild on staging | ||
| self._indexed_docs = {} | ||
| self._source_to_docid = {} | ||
| try: | ||
| return self.index_all(force=True) | ||
| except Exception: | ||
| self._staging_target = None | ||
| for k, v in _saved.items(): | ||
| setattr(self, k, v) | ||
| raise | ||
|
|
||
| @property | ||
| def _write_collection(self): | ||
| """Return the collection writes should hit — staging if active, else prod.""" | ||
| return self._staging_target if self._staging_target is not None else self.collection |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep all query-visible state on production until cutover.
_populate_staging keeps self.collection on production, but it replaces self.bm25_index, _bm25_initialized, _indexed_docs, and _source_to_docid with staging state before population completes. Concurrent queries therefore combine production Chroma data with an unbuilt or partial staging BM25 index. Keyword-only queries can return no results during the rebuild. Preserve the production in-memory state until the final swap, and build staging state separately.
mcp_server/server.py#L2120-L2142: do not publish staging BM25 and metadata state through shared query fields until the collection swap succeeds.tests/test_v483_hotfix.py#L110-L131: pause a staging populate and verify that a concurrent query still uses the production BM25 index and production source lookup.
📍 Affects 2 files
mcp_server/server.py#L2120-L2142(this comment)tests/test_v483_hotfix.py#L110-L131
🤖 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 `@mcp_server/server.py` around lines 2120 - 2142, The _populate_staging method
exposes partial staging BM25 and metadata state through shared query fields;
build those values in separate local staging state and publish them only after
the collection swap succeeds, preserving production query behavior throughout
population. Update tests/test_v483_hotfix.py lines 110-131 to pause staging
population and verify concurrent queries use the production BM25 index and
production source lookup.
| - **fix(fts5)** — `_format_fts5_results` skips orphan hits (FTS5 has the `chunk_id` but Chroma doesn't — nuclear-rebuild residue or manual-delete race). Previously emitted empty result items with `source=""` that broke reranking and polluted result lists. | ||
| - **fix(indexing, config)** — Added `Fts5LexicalIndex.count()` for the new sanity check. | ||
|
|
||
| All 6 core fixes were confirmed via production reproduction on a 75016-chunk corpus + 2052 newly-added catalog documents; the `nuclear_rebuild(full_rebuild=True)` path now completes cleanly and populates FTS5 with 75016 rows on first attempt. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the production-validation fix count.
The preceding entry lists three linked fixes plus five additional fixes, but this sentence states that “All 6 core fixes” were confirmed without defining the subset. Name the six fixes or update the number so the release note matches the documented fix list.
🤖 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 `@README.md` at line 1457, Update the production-validation sentence in the
README to accurately match the documented fixes: either explicitly identify the
six fixes included in validation or change “All 6 core fixes” to the correct
count based on the three linked and five additional fixes.
Comment on lines
+37
to
+42
| with patch.object(KnowledgeOrchestrator, "__init__", lambda self: None): | ||
| a = KnowledgeOrchestrator() | ||
| a._index_lock = threading.Lock() | ||
| b = KnowledgeOrchestrator() | ||
| b._index_lock = threading.Lock() | ||
| assert a._index_lock is not b._index_lock |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise constructor-created lock ownership.
This test patches out __init__ and assigns both locks manually. It also passes when _index_lock remains class-scoped. Construct two real instances with mocked initialization dependencies, then assert that each instance owns a distinct _index_lock.
🤖 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/test_v483_hotfix.py` around lines 37 - 42, Update the test around
KnowledgeOrchestrator construction to avoid patching out __init__ or manually
assigning _index_lock; mock only the constructor’s external initialization
dependencies, create two real instances, and assert their instance-owned
_index_lock objects are distinct.
CI on PR #169 caught 6 test_batch_parallel tests failing with AttributeError: 'KnowledgeOrchestrator' object has no attribute '_staging_target'. Those tests use patch.object(KnowledgeOrchestrator, '__init__', ...) so the new attribute set in __init__ never runs. Fix: getattr-defensive read so legacy mock instances continue working while real orchestrator instances still get the None default from __init__ semantics.
This was referenced Aug 11, 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
Critical hotfix for nuclear-rebuild + smart-reindex state semantics. Fixes 3 GitHub issues from @grishkovei against v4.8.1 plus 5 additional latent bugs discovered while triaging a real production reproduction on a 5889-doc / 75016-chunk corpus (hacktricks + LOL* catalogs).
Recommended upgrade for any corpus >1000 chunks or any user relying on
reindex_documents(force=True)after a config change.GitHub issues fixed
_populate_staging()no longer rebindsself.collectionto staging during populate. Writes route through new_write_collectionproperty (returns_staging_targetwhen set, else production); the query path continues readingself.collectionunchanged. Zero-downtime is now real, not just a docs promise.tracking["committed_this_run"]set) instead oflist(self._indexed_docs.keys()). Changed-but-not-yet-reached docs are properly detected on resume.reindex_documents(force=True)does not force re-embedding.forcenow propagates throughreindex_all(force=True)toindex_all(force=True). The v4.8 migration guide's documentedforce=Truepath was silently a no-op becausereindex_all()hardcodedforce=False.Additional fixes (discovered in production triage)
_index_lockmoved from class-level to instance-level (__init__). The classthreading.Lockwas silently shared acrossKnowledgeOrchestratorinstances, so a staging orchestrator from anuclear_rebuildswap could be GC'd while holding the lock, leaving every subsequentreindex_documentsreturningreindex_already_runningdespiteactive: false. Only recovery pre-fix was daemon restart._ensure_bm25_initializedand_iter_chroma_chunks_for_fts5batch-read viaoffset+limit=500. chromadb 1.x rebuildsIN (?, ?, ...)per returned row, so singlecollection.get(limit=count)on 48k+ chunks blows past SQLitemax_variable_number=999withInternal error: too many SQL variables. Any user with >999 chunks running nuclear rebuild or FTS5 lazy migration got a silently failed rebuild.Fts5LexicalIndex.is_ready()re-reads the marker when cached_readyis False. Post-swap orchestrators cached_ready=Falseforever and never re-checked, permanently silencing the fast-path._maybe_start_fts5_migrationcross-checks FTS5 row count vs Chroma count via new_fts5_marker_matches_realityhelper. Stalecompletemarker on a near-empty FTS5 (post-swap orphan cleanup, manual truncate, disk corruption) no longer leaves fast-path silent forever._format_fts5_resultsskips orphan hits where FTS5 has thechunk_idbut Chroma doesn't. Previously emitted empty result items withsource=""that broke reranking + polluted result lists.Fts5LexicalIndex.count()used by the sanity check.Verification
tests/test_v483_hotfix.py, one per bug. Each test literally reproduces the pre-fix behavior — passes post-fix, fails pre-fix.nuclear_rebuild(full_rebuild=True)now populates FTS5 with 75016 rows on first attempt with no orphans, no wedged locks, no silent failures.LEI 1 (frozen public API) preserved
Zero change to the 13 frozen MCP tool signatures. All fixes are internal state semantics + one new helper (
Fts5LexicalIndex.count()) that is purely additive.Rollback
git revert <sha>— the fixes are additive/corrective and revert cleanly. If FTS5 index gets corrupted post-upgrade (marker sayscompletebut queries return empty), deletedata/fts5_migration.stateand restart the daemon — the new sanity check will detect and rebuild automatically.Migration path
pip install -U knowledge-rag==4.8.3(ornpx -y knowledge-rag@4.8.3/docker pull ghcr.io/lyonzin/knowledge-rag:4.8.3)Test plan
tests/test_v483_hotfix.py— 8/8 pass localscripts/check_version_sync.py)scripts/check_api_surface.py --check)ruff check + ruff format --check)Closes #161. Closes #162. Closes #163.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
The PR hardens nuclear rebuild, smart reindex checkpointing, and FTS5 migration behavior while releasing version 4.8.3.
Confidence Score: 2/5
The PR does not appear safe to merge until forced reindex replacement, staging query-state isolation, and stable FTS5 resume semantics are fixed.
Forced reindex still reaches duplicate-ID adds without evicting old chunks, staging population still exposes partial in-memory query state, and FTS5 resume still skips by ordinal position over page-local ordering that can change with the corpus.
Files Needing Attention: mcp_server/server.py and mcp_server/fts5_index.py
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Orchestrator participant Production as Production Chroma participant Staging as Staging Chroma Client->>Orchestrator: nuclear_rebuild() Orchestrator->>Staging: populate through _write_collection Client->>Orchestrator: query() Orchestrator->>Production: semantic reads during population Orchestrator->>Orchestrator: validate staging Orchestrator->>Production: atomic collection swap Orchestrator->>Orchestrator: rebuild BM25 and start FTS5 migrationReviews (2): Last reviewed commit: "fix(server): _write_collection tolerates..." | Re-trigger Greptile