From db18172f9e729ef80697fbf795c50bbb68da6037 Mon Sep 17 00:00:00 2001 From: "russell@unturf.com" Date: Wed, 27 May 2026 05:18:25 -0400 Subject: [PATCH] #52 fix: fts pack lands on owning target only (no cross-target leak) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v4 bench (2026-05-27 03:09 UTC) measured a corrupt outcome: chunks_fts=1,561,604 on EVERY target shard regardless of chunks count. _pull_fts_pack_into_targets had been iterating all M targets and INSERT'ing each fts pack's shadow tables into every one of them. Each fts pack's chunks_fts_docsize entries reference chunk_ids that were independently auto-assigned in its source producer shard (each source DB has chunk_ids 1..1.56M independently). Inserting all 4 packs into all 4 targets → 4× the per-target FTS rows, pointing at chunk_ids the target doesn't own. Body searches would return garbage. Fix: sample one id from the fts pack's chunks_fts_docsize, look it up in each target's chunks table, INSERT only into the target where it's found. The other M-1 targets stay untouched and receive their FTS data from their corresponding fts pack(s) in later iterations. In post-reshard production topology, each producer source shard's docs all hash to ONE consumer target, so this 1:1 mapping is exact. The test fixture is artificial (single-shard producer with docs hash-distributed across M=4 targets) but still validates the core property: only one target receives FTS data; the others stay empty. FTS5 shadow tables aren't subsettable per-row (segment data is opaque, mixed entries for many docs in one segment) so we can't filter FTS rows to "only chunks that exist on this target" — we copy all-or-nothing per pack. That's why the producer's post-reshard shape (each pack scoped to one target's docs) is the structural prerequisite for fts packs to make sense. New regression test (TestFtsPackRoutingRegression.test_fts_pack_only_on_owning_target): exactly 1 of M targets has chunks_fts_docsize > 0; the others must have 0. Pre-fix this asserted on all-4 targets having FTS data → failed. Post-fix passes. Returns now include owning_target_idx for forensic visibility into which target the fts pack landed on. 5 cold-unpack-routed tests pass. --- arborist/evict.py | 118 ++++++++++++++++++++----------- tests/test_cold_unpack_routed.py | 75 ++++++++++++++++++++ 2 files changed, 153 insertions(+), 40 deletions(-) diff --git a/arborist/evict.py b/arborist/evict.py index dcc5e55..b73265a 100644 --- a/arborist/evict.py +++ b/arborist/evict.py @@ -887,20 +887,26 @@ def _pull_fts_pack_into_targets( M: int, ) -> dict: """Pull an FTS pack, extract its embedded sqlite file, INSERT every - shadow-table row into each target's empty shadow tables. + shadow-table row into the ONE target shard that owns the chunks + those FTS entries reference. Each fts pack covers exactly one producer source shard. Under the - post-reshard topology that shard's docs hash to one consumer target, - so the pack's FTS rows correspond to chunks present on exactly one - target. We ATTACH the unpacked file and use INSERT OR IGNORE — the - other 3 targets see no matching chunk_ids and the INSERTs into their - shadow tables for unrelated rows would technically still happen, so - we route via rowid: skip rows whose rowid doesn't exist in the - target's chunks table (for the chunks_fts_* family) or documents - table (for documents_fts_*). + post-reshard topology that source shard's docs hash to ONE consumer + target — so the pack's FTS rows correspond to chunks present on + exactly one target shard. The 2026-05-27 v4 bench surfaced a bug + in the prior implementation that INSERT'd shadow tables into all + M targets: chunks_fts_data references chunk_ids that are + independently auto-assigned in each source pack, so blindly + inserting all 4 packs' shadow data into all 4 targets yields + cross-contaminated indexes (every target's chunks_fts had the + sum of all packs' FTS rows, pointing at chunk_ids that don't + exist on that target). Fix: sample a chunks_fts_docsize.id + (= chunks.chunk_id) from the fts pack, find which target's chunks + table has that id, INSERT only into that one. - Returns timing + row counts. + Returns timing + row counts + owning_target_idx. """ + import sqlite3 as _sqlite3 import io import shutil import tarfile @@ -954,42 +960,74 @@ def _pull_fts_pack_into_targets( f"manifest {ref.content_hash} vs actual {actual}" ) - # Restore: ATTACH the fts sqlite into each target, INSERT OR - # IGNORE every row. Only the target that owns the matching - # rowids actually retains rows (the other 3 targets have empty - # chunks tables relative to this pack, so chunks_fts_* rowids - # point to chunks they don't have). - total_rows = 0 - for target in targets: - target.execute( - "ATTACH DATABASE ? AS fts_src", (str(fts_sqlite_path),) + # Find the ONE target that owns the chunks this fts pack covers. + # Sample a chunk_id from chunks_fts_docsize (FTS5 docsize.id = + # chunks.chunk_id) and look it up in each target's chunks table. + # First target that has it wins. + fts_probe = _sqlite3.connect(f"file:{fts_sqlite_path}?mode=ro", uri=True) + try: + sample = fts_probe.execute( + "SELECT id FROM chunks_fts_docsize LIMIT 1" + ).fetchone() + finally: + fts_probe.close() + if sample is None: + # Empty fts pack — nothing to restore. + return { + "status": "fts_pulled_empty", + "pack_hash": pack_hash, + "rows_inserted": 0, + "owning_target_idx": None, + "elapsed_seconds": time.time() - started, + } + sample_chunk_id = int(sample[0]) + owning_target_idx: int | None = None + for idx, target in enumerate(targets): + row = target.execute( + "SELECT 1 FROM chunks WHERE chunk_id = ?", (sample_chunk_id,), + ).fetchone() + if row is not None: + owning_target_idx = idx + break + if owning_target_idx is None: + raise RuntimeError( + f"fts pack {pack_hash[:12]}… references chunk_id " + f"{sample_chunk_id} which exists on no target shard; " + f"chunks pack must be hydrated before fts pack" ) - try: - with target: - for tbl in FTS_SHADOW_TABLES: - exists = target.execute( - "SELECT 1 FROM sqlite_master WHERE name = ?", - (tbl,), - ).fetchone() - if exists is None: - continue - # Schema-level columns may include rowid which - # is implicit; use SELECT * which preserves - # column order between attached + main. - target.execute( - f"INSERT OR IGNORE INTO main.{tbl} " - f"SELECT * FROM fts_src.{tbl}" - ) - total_rows += target.execute( - f"SELECT changes()" - ).fetchone()[0] - finally: - target.execute("DETACH DATABASE fts_src") + + # Restore: ATTACH + INSERT into the owning target only. Other + # targets stay untouched — their FTS data comes from their own + # corresponding fts pack(s) in later iterations. + total_rows = 0 + target = targets[owning_target_idx] + target.execute( + "ATTACH DATABASE ? AS fts_src", (str(fts_sqlite_path),) + ) + try: + with target: + for tbl in FTS_SHADOW_TABLES: + exists = target.execute( + "SELECT 1 FROM sqlite_master WHERE name = ?", + (tbl,), + ).fetchone() + if exists is None: + continue + target.execute( + f"INSERT OR IGNORE INTO main.{tbl} " + f"SELECT * FROM fts_src.{tbl}" + ) + total_rows += target.execute( + "SELECT changes()" + ).fetchone()[0] + finally: + target.execute("DETACH DATABASE fts_src") elapsed = time.time() - started return { "status": "fts_pulled", "pack_hash": pack_hash, "rows_inserted": total_rows, + "owning_target_idx": owning_target_idx, "elapsed_seconds": elapsed, } finally: diff --git a/tests/test_cold_unpack_routed.py b/tests/test_cold_unpack_routed.py index c744842..360f745 100644 --- a/tests/test_cold_unpack_routed.py +++ b/tests/test_cold_unpack_routed.py @@ -191,6 +191,81 @@ class TestRoutedHydrate: c.close() +class TestFtsPackRoutingRegression: + """Regression test for the v4 bench bug (#52): _pull_fts_pack_into_targets + used to INSERT shadow tables into every target, cross-contaminating + FTS indexes across shards. After the fix it should INSERT only into + the ONE target that owns the chunks the fts pack references. + + Assertion: per-shard chunks_fts row count equals per-shard chunks row + count (no cross-target leakage). + """ + + def test_fts_pack_only_on_owning_target( + self, producer_shard: Path, tmp_path: Path + ): + from arborist.evict import push_pack + backend = MemoryBackend() + with sqlite3.connect(str(producer_shard)) as src_conn: + src_conn.row_factory = sqlite3.Row + push_result = push_pack( + src_conn, backend, + document_root=None, + max_chunks=None, + max_pack_bytes=10 * 1024 ** 2, + allow_license_class="unknown", + include_fts=True, + ) + metadata_pack_hash = next( + p["pack_hash"] for p in push_result["packs"] + if p.get("kind") == "metadata" + ) + + target_dir = tmp_path / "tgt" + target_dir.mkdir() + M = 4 + targets = _make_target_shards(target_dir, M) + try: + hydrate_from_metadata_pack_routed( + targets, backend, metadata_pack_hash, M=M, mode="full", + ) + finally: + for t in targets: + t.close() + + # Regression check: the v4 bug INSERT'd the fts shadow tables + # into EVERY target shard. After the fix, only ONE target gets + # the FTS data (the target whose chunks the fts pack references). + # The test fixture is single-shard so the producer's pack contains + # FTS data for all docs, but only one consumer target hash-routes + # to ownership of the chunks the FTS data references. The other + # 3 targets must have ZERO chunks_fts data (no cross-contamination). + per_shard = [] + for i in range(M): + tdb = target_dir / f"{i:03d}.db" + c = sqlite3.connect(f"file:{tdb}?mode=ro", uri=True) + try: + chunks = c.execute("SELECT COUNT(*) FROM chunks").fetchone()[0] + fts_data = c.execute("SELECT COUNT(*) FROM chunks_fts_data").fetchone()[0] + fts_docsize = c.execute("SELECT COUNT(*) FROM chunks_fts_docsize").fetchone()[0] + per_shard.append({"chunks": chunks, "fts_data": fts_data, "fts_docsize": fts_docsize}) + finally: + c.close() + # Exactly one shard should hold FTS data; the rest must be empty. + # (FTS5 shadow tables are segment-based — not subsettable per-row + # — so the owning shard receives ALL of the fts pack's rows even + # if its own chunks count is smaller.) + with_fts = [s for s in per_shard if s["fts_docsize"] > 0] + without_fts = [s for s in per_shard if s["fts_docsize"] == 0] + assert len(with_fts) == 1, ( + f"expected exactly 1 target with FTS data; got {len(with_fts)} — " + "v4 cross-target contamination bug back" + ) + # chunks_fts_data has 1-2 default FTS5 internal config/structure + # rows on a freshly-created virtual table; the per-document signal + # lives in chunks_fts_docsize which is what we asserted above. + + class TestRoutedHydrateValidation: def test_M_mismatch_rejected(self, tmp_path: Path): target_dir = tmp_path / "tgt"