phase: 185_sync_defer_content_fast_path
Build and Push Containers / build-and-push-app (push) Successful in 1m41s
Build and Push Containers / build-and-push-db (push) Successful in 11s

All gates green. Report:

**Implemented / verified (phase 185)**
- `_doc_row_stmt` is the ONE deferral helper for all four fast-path row loads; only `Document.content` is deferred, no `raiseload`/`noload` (LOCKED A1/A2) — pinned by `tests/unit/test_importer_defer_content.py`.
- `tests/integration/test_import_fast_path_columns.py` (5), `tests/e2e/test_fast_path_deferred_content.py` (3, isolation) green; added 2 constraint pins to `test_import_session_retention.py` (summary-NULL backfill survives the cadence; both prune guards cost the same statement budget).
- **Fixed 6 pre-existing E2E failures** (all reproduce at HEAD with this phase's importer reverted): stale pins from phase 168 `Settings` nav link (`test_nav_rename_sources`), phase 182 `verbatim.js` script count (`test_steering`), phase 175 `/api/chats?limit=…` pagination breaking `endswith` counting (`test_navbar_refresh`), phase-167 lazy-drill race in the breadcrumb read (`test_modal_back_button`), nav-length tab budget (`test_suggestion_chips`).
- **Fixed a genuine flake** in `test_source_chip_quality` (passed/failed across identical runs): `ORDER BY embedding <=> q` is not a total order, so equal-cosine fixtures get a different RRF rank each run. Pinned it as an explicit tie-group (`_assert_ranked` / `_assert_related_pair`) instead of an exact order.

**Tests / lint (all 100% green)**
- `uv run pytest --cov=app --cov-report=term-missing` → **4519 passed**, `app/rag/importer.py` **99%**, `TOTAL` **99%** (>90%).
- `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors.
- Full E2E sweep, one pytest process per file (the README's sanctioned loop) → **167/167 files OK, 0 FAIL** (re-run after the fixes).

**Completion criteria**: helper shared by 4 loads ✅ · column-list + 1-SELECT + backfill pins ✅ · `test_importer_unchanged_fast_path.py` 24 passed, zero expectation edits ✅ · full suite + coverage + lint/types ✅ · phase E2E in isolation ✅ · docs accurate ✅ · commit left to the harness (no git commands run).

**Documentation**: README + AGENTS.md already carry the phase-185 rule; full pass found and fixed no doc inconsistencies (env vars: all 85 `Settings` fields documented in `.env.example` + README, no ghosts; every `app/… tests/… scripts/…` path referenced in both files exists).

**Deviations**: (1) the ranking non-determinism behind the chip-quality flake is real in `app/rag/retriever.py` — a SQL tie-break would cost pgvector index usage, so I kept the fix test-side; a follow-up phase should make vector-candidate ranking a total order. (2) Two tracked E2E screenshots were regenerated by the sweep (artifacts, left modified).

**Next pending phase**: `186_git_log_streaming_dates`.
This commit is contained in:
2026-10-16 04:54:01 -04:00
parent 065304b52f
commit 632fb88e43
35 changed files with 2862 additions and 138 deletions
@@ -136,6 +136,7 @@ lazy SELECT per row and fail that test, not get papered over.
``scripts/git_sync.py`` (phase 186), ``app/db.py:30``, and the
``doc.chunks = [...]`` re-embed path (phase 183 LOCKED A2).
"""
from __future__ import annotations
import asyncio
@@ -189,9 +190,7 @@ EXPECTED_SUMMARY_CHUNKS = 12
#: Every ORM object a full ADD run creates: the 12 documents, their content
#: chunks and their summary chunks. What the identity map would have to
#: hold if the ADDED path retained what the phase-184 baseline describes.
OBJECTS_CREATED_BY_ADD_RUN = (
FILE_COUNT + EXPECTED_CONTENT_CHUNKS + EXPECTED_SUMMARY_CHUNKS
)
OBJECTS_CREATED_BY_ADD_RUN = FILE_COUNT + EXPECTED_CONTENT_CHUNKS + EXPECTED_SUMMARY_CHUNKS
#: The measured number of identity-map entries at the end of an UNCHANGED
#: run with the cadence OFF — one per file walked, each a full ``Document``
@@ -322,9 +321,7 @@ def _kb_snapshot(db: Session) -> dict[str, tuple[str, str | None, str, int]]:
}
snapshot: dict[str, tuple[str, str | None, str, int]] = {}
for doc in db.scalars(
select(Document)
.where(Document.source == PROBE_SOURCE)
.order_by(Document.path)
select(Document).where(Document.source == PROBE_SOURCE).order_by(Document.path)
):
assert doc.file_hash is not None # the phase-147 stamp is always set
snapshot[doc.path] = (
@@ -373,9 +370,7 @@ def test_the_added_run_holds_its_objects_only_until_a_collection(
root = _seed_files(tmp_path)
session = SessionLocal()
try:
summary = asyncio.run(
import_sources([root], _probe_llm(), prune=False, session=session)
)
summary = asyncio.run(import_sources([root], _probe_llm(), prune=False, session=session))
assert (summary.files, summary.added, summary.unchanged) == (
FILE_COUNT,
FILE_COUNT,
@@ -405,9 +400,7 @@ def test_the_added_run_holds_its_objects_only_until_a_collection(
session.close()
def test_the_unchanged_run_keeps_every_document_it_walks(
db: Session, tmp_path: Path
) -> None:
def test_the_unchanged_run_keeps_every_document_it_walks(db: Session, tmp_path: Path) -> None:
"""THE PROBE: a repeat sync holds one full ``Document`` per file walked.
The production-shaped case — the second sync of the same source, every
@@ -446,9 +439,7 @@ def test_the_unchanged_run_keeps_every_document_it_walks(
staircase.append(_map_size(session))
summary = asyncio.run(
import_sources(
[root], _probe_llm(), prune=False, session=session, progress=progress
)
import_sources([root], _probe_llm(), prune=False, session=session, progress=progress)
)
assert (summary.files, summary.added, summary.unchanged) == (
FILE_COUNT,
@@ -459,8 +450,7 @@ def test_the_unchanged_run_keeps_every_document_it_walks(
# One row resident per file already walked — a staircase, never a
# plateau and never a release.
assert staircase == list(range(FILE_COUNT)), (
"expected exactly one document retained per file already walked,"
f" got {staircase}"
f"expected exactly one document retained per file already walked, got {staircase}"
)
assert _map_size(session) == MEASURED_UNCHANGED_MAP_SIZE
@@ -643,8 +633,7 @@ def test_reset_cadence_bounds_the_unchanged_walk(db: Session, tmp_path: Path) ->
FILE_COUNT,
)
assert staircase == [n % RESET_FILES for n in range(FILE_COUNT)], (
f"the bounded walk sampled {staircase} — expected a release every"
f" {RESET_FILES} files"
f"the bounded walk sampled {staircase} — expected a release every {RESET_FILES} files"
)
assert max(staircase) < MEASURED_UNCHANGED_MAP_SIZE
assert _map_size(session) == 0, (
@@ -682,14 +671,10 @@ def test_reset_cadence_survives_an_embedding_error(db: Session, tmp_path: Path)
session = SessionLocal()
try:
summary = asyncio.run(
import_sources([root], llm, prune=False, session=session)
)
summary = asyncio.run(import_sources([root], llm, prune=False, session=session))
assert (summary.files, summary.errors) == (FILE_COUNT, 1)
assert summary.added == FILE_COUNT - 1
assert summary.chunks == EXPECTED_CONTENT_CHUNKS - (
EXPECTED_CONTENT_CHUNKS // FILE_COUNT
)
assert summary.chunks == EXPECTED_CONTENT_CHUNKS - (EXPECTED_CONTENT_CHUNKS // FILE_COUNT)
assert summary.summaries == EXPECTED_SUMMARY_CHUNKS - 1
assert _map_size(session) <= MAP_BOUND
finally:
@@ -743,9 +728,7 @@ def test_a_clean_row_leaves_the_map_and_a_dirty_one_never_does(db: Session) -> N
assert _map_size(session) == 0, "a clean committed row must be weakly held"
row = session.scalar(
select(Document).where(
Document.source == PROBE_SOURCE, Document.path == "mechanism.md"
)
select(Document).where(Document.source == PROBE_SOURCE, Document.path == "mechanism.md")
)
assert row is not None
row.file_hash = row.file_hash # the stamp: same value, dirty state
@@ -765,9 +748,7 @@ def test_a_clean_row_leaves_the_map_and_a_dirty_one_never_does(db: Session) -> N
assert _map_size(session) == 0
assert (
db.scalar(
select(func.count())
.select_from(Document)
.where(Document.source == PROBE_SOURCE)
select(func.count()).select_from(Document).where(Document.source == PROBE_SOURCE)
)
== 1
)
@@ -801,9 +782,7 @@ PRUNE_BATCH = 10
#: The SIX columns the prune loads — the ``load_only`` set of ``_prune``
#: (docstring: the light-row contract). An eighth name here means the walk
#: got heavier; one missing means a guard lost the column it reads.
PRUNE_COLUMNS = frozenset(
{"id", "source", "path", "is_image", "image_path", "page_count"}
)
PRUNE_COLUMNS = frozenset({"id", "source", "path", "is_image", "image_path", "page_count"})
#: ``… FROM documents`` — the prune's own walk AND (that is the pin) any
#: per-document follow-up lazy load of a deferred column. The word boundary
@@ -862,9 +841,7 @@ def _projected_columns(statement: str) -> frozenset[str]:
return frozenset(re.findall(r"documents\.\w+", head))
def _seed_prune_docs(
session: Session, *, page_dir: Path | None = None
) -> list[UUID]:
def _seed_prune_docs(session: Session, *, page_dir: Path | None = None) -> list[UUID]:
"""``PRUNE_DOC_COUNT`` indexed docs of :data:`PRUNE_SOURCE`, each with
``PRUNE_CHUNKS_PER_DOC`` embedded chunks and a full-fat ``content``.
@@ -924,9 +901,7 @@ def _arm_commit_counter(session: Session) -> list[int]:
return commits
def test_prune_holds_no_content(
db: Session, tmp_path: Path, statement_capture: list[str]
) -> None:
def test_prune_holds_no_content(db: Session, tmp_path: Path, statement_capture: list[str]) -> None:
"""THE PRUNE PROBE: the walk loads SIX columns and never ``content``.
30 documents × 5 chunks are pruned through the importer's own ``_prune``
@@ -971,10 +946,7 @@ def test_prune_holds_no_content(
"prune — a deferred column is being read per row, so the light row "
"is incomplete (add the column to load_only)"
)
projected = {
col.removeprefix("documents.")
for col in _projected_columns(selects[0])
}
projected = {col.removeprefix("documents.") for col in _projected_columns(selects[0])}
assert projected == PRUNE_COLUMNS, (
f"the prune's row is not the six-column light row: {sorted(projected)}"
)
@@ -985,12 +957,11 @@ def test_prune_holds_no_content(
# Everything really went, children included (no orphans — phase 183 D4,
# asserted here too so a light row cannot quietly change it).
assert db.scalar(
select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE)
) == 0
assert db.scalar(
select(func.count()).select_from(Chunk).where(Chunk.document_id.in_(ids))
) == 0
assert (
db.scalar(select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE))
== 0
)
assert db.scalar(select(func.count()).select_from(Chunk).where(Chunk.document_id.in_(ids))) == 0
assert sorted(summary.changed_paths) == sorted(
(PRUNE_SOURCE, f"docs/doc-{n:04d}.md") for n in range(PRUNE_DOC_COUNT)
)
@@ -1001,9 +972,7 @@ def test_prune_holds_no_content(
assert _map_classes(db) == {}
def test_prune_without_sources_touches_nothing(
db: Session, statement_capture: list[str]
) -> None:
def test_prune_without_sources_touches_nothing(db: Session, statement_capture: list[str]) -> None:
"""The empty-source guard is a real short-circuit, not a walk for naught.
``_prune`` with no source names returns ``0`` before the SELECT — so a
@@ -1017,9 +986,10 @@ def test_prune_without_sources_touches_nothing(
assert _prune(db, set(), set(), summary=summary, prune_batch=10) == 0
assert statement_capture == []
assert summary.changed_paths == []
assert db.scalar(
select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE)
) == PRUNE_DOC_COUNT
assert (
db.scalar(select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE))
== PRUNE_DOC_COUNT
)
def test_prune_batch_commits(db: Session) -> None:
@@ -1059,11 +1029,10 @@ def test_prune_batch_commits(db: Session) -> None:
session.close()
# The FK cascade did the child work in EVERY transaction the
# prune opened, not just the last one.
assert db.scalar(
select(func.count())
.select_from(Chunk)
.where(Chunk.document_id.in_(ids))
) == 0
assert (
db.scalar(select(func.count()).select_from(Chunk).where(Chunk.document_id.in_(ids)))
== 0
)
pruned, summary, commits = run(PRUNE_BATCH)
assert pruned == PRUNE_DOC_COUNT
@@ -1074,9 +1043,10 @@ def test_prune_batch_commits(db: Session) -> None:
assert sorted(summary.changed_paths) == sorted(
(PRUNE_SOURCE, f"docs/doc-{n:04d}.md") for n in range(PRUNE_DOC_COUNT)
)
assert db.scalar(
select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE)
) == 0
assert (
db.scalar(select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE))
== 0
)
# …and the pre-phase run does the SAME work in ONE transaction.
_truncate(db)
@@ -1087,9 +1057,10 @@ def test_prune_batch_commits(db: Session) -> None:
"reproduce the pre-phase single transaction exactly"
)
assert sorted(summary_0.changed_paths) == sorted(summary.changed_paths)
assert db.scalar(
select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE)
) == 0
assert (
db.scalar(select(func.count()).select_from(Document).where(Document.source == PRUNE_SOURCE))
== 0
)
def test_prune_batch_setting_reaches_the_prune(
@@ -1137,9 +1108,7 @@ def test_prune_batch_setting_reaches_the_prune(
monkeypatch.setattr(importer, "_prune", spy)
second = asyncio.run(import_sources([root], llm, prune=True, session=db))
assert seen_kwargs == [3], (
"import_sources did not pass Settings.sync_prune_batch to _prune"
)
assert seen_kwargs == [3], "import_sources did not pass Settings.sync_prune_batch to _prune"
assert (second.files, second.pruned, second.errors) == (1, 3, 0)
assert sorted(path for (_src, path) in second.changed_paths) == [
"keep-0.md",
@@ -1152,16 +1121,99 @@ def test_prune_batch_setting_reaches_the_prune(
select(func.count()).select_from(Chunk).where(Chunk.embedding.is_not(None))
)
assert embedded is not None and embedded > 0
assert db.scalar(
select(func.count())
.select_from(Chunk)
.where(Chunk.document_id.not_in(select(Document.id)))
) == 0
assert (
db.scalar(
select(func.count())
.select_from(Chunk)
.where(Chunk.document_id.not_in(select(Document.id)))
)
== 0
)
def test_prune_guards_unaffected_with_light_rows(
db: Session, tmp_path: Path
) -> None:
def test_the_summary_backfill_survives_the_reset_cadence(db: Session, tmp_path: Path) -> None:
"""The constraint that fixes WHERE the cadence may sit, in ONE file.
``_unchanged_doc`` (phase 118) backfills a NULL summary by calling
:func:`app.rag.summarizer.summarize_document`, which reads
``doc.content`` AFTER the row was loaded and its ``file_hash`` stamped.
That local is live across statements: a
``session.expunge_all()`` moved into ``_unchanged_doc`` — or a
``noload``/``raiseload`` added to the row it walks — would discard or
break the very row the backfill reads. Phase 185 task 01 chose
``defer`` over ``noload``/``raiseload`` (LOCKED A2) for the same reason,
and task 05 of THIS phase pinned the choice on the statement itself
(``tests/unit/test_importer_defer_content.py``); this pins the SAME
constraint on the run.
The fixture is a document whose ``is_summary`` chunk is missing while
its ``file_hash`` matches (a re-sync with a NULL summary — exactly the
row the backfill exists for), imported at the tightest cadence the
setting allows that still resets inside a 12-file walk. Three
observables, none of them a memory proxy:
* the backfill RUNS — one and only one ``is_summary`` chunk appears,
carrying the fake-embedder's summary text;
* every other file is still ``unchanged`` and the summary count is
exactly the file count afterwards, i.e. the reset cadence destroyed
no summary on the way through;
* the doc's own text is still intact afterwards (``content`` was never
rewritten by a re-index).
"""
root = _seed_files(tmp_path)
seed = SessionLocal()
try:
first = asyncio.run(import_sources([root], _probe_llm(), session=seed))
assert first.added == FILE_COUNT
finally:
seed.close()
# Hand the next sync ONE document with a NULL summary: it takes
# ``_unchanged_doc``'s backfill branch instead of the early return.
probe_rel = "probe-00.md"
before = _kb_snapshot(db)[probe_rel]
target = db.execute(
select(Document).where(Document.source == PROBE_SOURCE, Document.path == probe_rel)
).scalar_one()
db.query(Chunk).filter(Chunk.document_id == target.id, Chunk.is_summary.is_(True)).delete()
target.summary = None
db.commit()
db.expunge_all()
assert _kb_snapshot(db)[probe_rel][1] is None
session = SessionLocal()
try:
summary = asyncio.run(
import_sources(
[root], _probe_llm(reset_files=RESET_FILES), prune=False, session=session
)
)
finally:
session.close()
# Everything stayed unchanged — the backfill never re-indexed.
assert (summary.files, summary.added, summary.updated, summary.unchanged) == (
FILE_COUNT,
0,
0,
FILE_COUNT,
)
assert (summary.errors, summary.summary_backfilled) == (0, 1)
snap = _kb_snapshot(db)
backfilled = snap[probe_rel]
assert backfilled[1] is not None, "the summary-NULL backfill did not run"
# The one doc that needed the deferred read still has its OWN text and
# its own hash: a summary was added, the document was not rewritten.
assert backfilled[0] == before[0]
assert backfilled[2] == before[2]
# And the cadence destroyed nothing on its way through: every file is
# back to 5 content chunks + 1 summary chunk.
assert {p: c for p, (_, _, _, c) in snap.items()} == {
f"probe-{n:02d}.md": 6 for n in range(FILE_COUNT)
}
def test_prune_guards_unaffected_with_light_rows(db: Session, tmp_path: Path) -> None:
"""The guards run off SIX columns — the light row is COMPLETE.
A column missing from ``load_only`` does not announce itself: the guard
@@ -1216,9 +1268,7 @@ def test_prune_guards_unaffected_with_light_rows(
seen = {(PRUNE_SOURCE, "notes/kept.md"), (PRUNE_SOURCE, "deck.pdf")}
session = SessionLocal()
try:
pruned_toggle_off = _prune(
session, {PRUNE_SOURCE}, seen, images=False, prune_batch=0
)
pruned_toggle_off = _prune(session, {PRUNE_SOURCE}, seen, images=False, prune_batch=0)
finally:
session.close()
# Toggle OFF: BOTH image docs are invisible to the walk, not deleted —
@@ -1228,9 +1278,7 @@ def test_prune_guards_unaffected_with_light_rows(
session = SessionLocal()
try:
pruned_toggle_on = _prune(
session, {PRUNE_SOURCE}, seen, images=True, prune_batch=0
)
pruned_toggle_on = _prune(session, {PRUNE_SOURCE}, seen, images=True, prune_batch=0)
finally:
session.close()
# Toggle ON: the deleted image file prunes its row AND its copy (the
@@ -1240,8 +1288,82 @@ def test_prune_guards_unaffected_with_light_rows(
assert not gone_copy.exists()
assert kept_copy.exists()
assert sorted(
path
for (path,) in db.execute(
select(Document.path).where(Document.source == PRUNE_SOURCE)
)
path for (path,) in db.execute(select(Document.path).where(Document.source == PRUNE_SOURCE))
) == ["deck.pdf", "deck.pdf#page=1#img=0.png", "notes/kept.md"]
def test_both_prune_guards_cost_the_same_statement_budget(
db: Session, tmp_path: Path, statement_capture: list[str]
) -> None:
"""The guards add NO hidden read — proved by statement count, both ways.
:func:`test_prune_guards_unaffected_with_light_rows` above proves the
guards DECIDE correctly off the six light columns. This pins the other
half of the same contract, the half that is invisible in behaviour: a
guard reaching for a deferred attribute would still decide correctly and
only show up as ONE MORE SELECT PER ROW. So the assertion here is a
statement count, not an outcome:
* the SAME projection runs under both toggles — one candidate walk
apiece, naming the same six columns (images OFF only skips rows
through ``is_image``, a column the walk already reads);
* neither walk projects ``documents.content``;
* deleting an image row + copying its file costs no extra document
SELECT (the ``image_path`` the copy cleanup uses is in the projection).
Total: exactly TWO document SELECTs for the two syncs, whatever the
guards do.
"""
image_dir = tmp_path / "image_dir"
image_dir.mkdir()
gone_copy = image_dir / "gone.png"
gone_copy.write_bytes(b"\x89PNG\r\n\x1a\n gone")
def add(path: str, *, is_image: bool, copy: Path | None = None) -> None:
db.add(
Document(
source=PRUNE_SOURCE,
path=path,
full_path=f"/srv/brain/{PRUNE_SOURCE}/{path}",
title=f"Budget probe {path}",
content=PRUNE_CONTENT,
content_hash=f"{abs(hash(path)):064x}"[:64],
is_image=is_image,
image_path=str(copy) if copy else None,
)
)
add("notes/kept.md", is_image=False)
add("notes/gone.md", is_image=False)
add("img/gone.png", is_image=True, copy=gone_copy)
add("deck.pdf#page=1#img=0.png", is_image=True)
db.commit()
seen = {(PRUNE_SOURCE, "notes/kept.md")}
projections: list[frozenset[str]] = []
for toggle in (False, True):
since = len(statement_capture)
session = SessionLocal()
try:
_prune(session, {PRUNE_SOURCE}, seen, images=toggle, prune_batch=0)
finally:
session.close()
# Only what THIS sync emitted — the capture is cumulative.
walks = [
s for s in _doc_selects(statement_capture[since:]) if "WHERE documents.source" in s
]
assert len(walks) == 1, (
f"the images={toggle} prune ran {len(walks)} candidate walks; "
"one per sync is the budget (a second is a per-row lazy load)"
)
projections.append(_projected_columns(walks[0]))
assert projections[0] == projections[1], (
"the two toggles read DIFFERENT columns — a guard grew a deferred "
f"read:\n off: {sorted(projections[0])}\n on: {sorted(projections[1])}"
)
assert "documents.content" not in projections[0]
assert len(_doc_selects(statement_capture)) == 2, (
"two syncs must cost exactly two document SELECTs in total, guards "
"included — any extra is a per-row read the light row must cover"
)