#52 fix: fts pack lands on owning target only (no cross-target leak)
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.
This commit is contained in:
parent
45960d3909
commit
db18172f9e
2 changed files with 153 additions and 40 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue