From 7b5ea7c2ee153e4437fd508dfcb56c88422a0e12 Mon Sep 17 00:00:00 2001 From: ducoterra Date: Thu, 15 Oct 2026 16:00:28 -0400 Subject: [PATCH] phase: 183_sync_passive_delete_cascade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All gates green. Final state verified. **Implemented / verified** - Task 03's work was **never actually done** (its report was a stub, `tests/e2e/test_prune_cascade_cleanup.py` did not exist) → wrote it: 3 tests — 9-file local source synced from the Sources page (`9 added`), 6 files deleted upstream, re-sync (`0 added · 3 unchanged · 6 pruned`), documents vanished from the tree + `GET /api/documents/content` 404 + total 3, **0 orphan chunk rows** via the test's own SQL, survivor chunk total unchanged; partial-prune chat turn (grounded, non-deflected, `query_log` names the survivor and no pruned doc); total source removal (`DELETE /api/git-sources/{id}`) wipes every doc+chunk of the source with 0 orphans, sibling source untouched. Mock-only (`E2E_REAL_LLM=1` skipped), own port 8199, truncate before/after. - Tasks 01/02 re-verified: `passive_deletes=True` + mapper pin, probe flipped to `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, `_prune`/`delete_git_source` docstrings. - **Drift guard proven non-tautological by my own one-off run**: scratch DB (`alembic upgrade head`) → FK `confdeltype='c'`, guard PASSES, CASCADE delete leaves 0 chunks; FK recreated `ON DELETE NO ACTION` → guard FAILS verbatim and a `passive_deletes` delete raises `ForeignKeyViolation` (never silent orphans). Scratch DB dropped; shared test DB untouched (`'c'`). **Gates** - `uv run pytest --cov=app --cov-report=term-missing` → **4488 passed**, `app/` TOTAL **99%** (>90%) - `uv run pytest tests/integration/test_prune_cascade_statements.py -v` → 4 passed - `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` → **3 passed in isolation**; `test_source_removal_cleanup.py` 6 passed, `test_force_push_resync.py` 2 passed (both isolated) - `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors **Completion criteria**: passive_deletes+pin ✅ · probe post-fix ✅ · drift guard green + proven-failing variant ✅ · full suite/coverage/lint ✅ · E2E in isolation ✅ · docs consistent ✅ · commit = harness-only (per pipeline rule I ran no git write commands). **Docs**: fixed 4 pre-existing README inconsistencies — per-module E2E ports (claim of "one pair of fixed ports" predates phase 79), debugpy `launch.json` snippet vs the shipped file (`name`/`remoteRoot`), and the page-naming wobble (navbar labels `/sources.html` **RAG**, `/git-sources.html` **Sources** — the Sync button's home is now named correctly in 4 places); added the phase-183 E2E pointer. AGENTS.md verified accurate (rule 12 matches the delete-site audit). **Deviation**: none from the locked decisions; the E2E is a behaviour pin (byte-identical owner-visible behaviour), the statement-count probe remains the memory teeth, as phase 183 D1/A4 specify. **Next pending phase**: `184_sync_bounded_session`. --- .../00_phase.md | 57 ++ .../01_prune_cascade_probe.md | 29 + .../02_passive_deletes.md | 52 ++ .../03_e2e_prune_cascade.md | 34 + .../todo/184_sync_bounded_session/00_phase.md | 59 ++ .../01_identity_map_probe.md | 33 + .../02_session_reset_cadence.md | 45 ++ .../03_batched_prune.md | 51 ++ .../04_e2e_bounded_sync.md | 35 + .../00_phase.md | 55 ++ .../01_fast_path_statement_probe.md | 27 + .../02_defer_content.md | 53 ++ .../03_e2e_deferred_content.md | 32 + .../186_git_log_streaming_dates/00_phase.md | 68 ++ .../01_parser_accepts_lines.md | 26 + .../02_stream_git_log.md | 37 + .../03_e2e_streamed_dates.md | 34 + .../187_sync_peak_memory_line/00_phase.md | 54 ++ .../01_memory_helper.md | 26 + .../02_sync_log_lines.md | 28 + .../03_status_key_and_ui.md | 30 + .../04_e2e_peak_memory.md | 34 + .../todo/188_chunks_hnsw_index/00_phase.md | 62 ++ .../01_hnsw_migration.md | 44 ++ .../02_ef_search_setting.md | 37 + .../03_plan_and_recall_pins.md | 27 + .../04_recall_battery_gate.md | 35 + .../05_e2e_index_chat_and_runbook.md | 36 + ...nc_passive_delete_cascade__00_phase.a1.err | 0 ...ync_passive_delete_cascade__00_phase.a1.md | 20 + ...sive_delete_cascade__00_phase.a1.md.status | 1 + ...ssive_delete_cascade__00_phase.a1.validate | 143 ++++ ...ete_cascade__01_prune_cascade_probe.a1.err | 0 ...lete_cascade__01_prune_cascade_probe.a1.md | 21 + ...scade__01_prune_cascade_probe.a1.md.status | 1 + ...ascade__01_prune_cascade_probe.a1.validate | 143 ++++ ..._delete_cascade__02_passive_deletes.a1.err | 0 ...e_delete_cascade__02_passive_deletes.a1.md | 16 + ...e_cascade__02_passive_deletes.a1.md.status | 1 + ...te_cascade__02_passive_deletes.a1.validate | 143 ++++ ...elete_cascade__03_e2e_prune_cascade.a1.err | 0 ...delete_cascade__03_e2e_prune_cascade.a1.md | 1 + ...cascade__03_e2e_prune_cascade.a1.md.status | 1 + ..._cascade__03_e2e_prune_cascade.a1.validate | 143 ++++ AGENTS.md | 17 + README.md | 68 +- app/api/git_sources.py | 7 +- app/models.py | 12 +- app/rag/importer.py | 17 + tests/e2e/test_prune_cascade_cleanup.py | 667 ++++++++++++++++++ .../test_prune_cascade_statements.py | 477 +++++++++++++ tests/unit/test_importer.py | 24 + 52 files changed, 3044 insertions(+), 19 deletions(-) create mode 100644 .agents/phases/complete/183_sync_passive_delete_cascade/00_phase.md create mode 100644 .agents/phases/complete/183_sync_passive_delete_cascade/01_prune_cascade_probe.md create mode 100644 .agents/phases/complete/183_sync_passive_delete_cascade/02_passive_deletes.md create mode 100644 .agents/phases/complete/183_sync_passive_delete_cascade/03_e2e_prune_cascade.md create mode 100644 .agents/phases/todo/184_sync_bounded_session/00_phase.md create mode 100644 .agents/phases/todo/184_sync_bounded_session/01_identity_map_probe.md create mode 100644 .agents/phases/todo/184_sync_bounded_session/02_session_reset_cadence.md create mode 100644 .agents/phases/todo/184_sync_bounded_session/03_batched_prune.md create mode 100644 .agents/phases/todo/184_sync_bounded_session/04_e2e_bounded_sync.md create mode 100644 .agents/phases/todo/185_sync_defer_content_fast_path/00_phase.md create mode 100644 .agents/phases/todo/185_sync_defer_content_fast_path/01_fast_path_statement_probe.md create mode 100644 .agents/phases/todo/185_sync_defer_content_fast_path/02_defer_content.md create mode 100644 .agents/phases/todo/185_sync_defer_content_fast_path/03_e2e_deferred_content.md create mode 100644 .agents/phases/todo/186_git_log_streaming_dates/00_phase.md create mode 100644 .agents/phases/todo/186_git_log_streaming_dates/01_parser_accepts_lines.md create mode 100644 .agents/phases/todo/186_git_log_streaming_dates/02_stream_git_log.md create mode 100644 .agents/phases/todo/186_git_log_streaming_dates/03_e2e_streamed_dates.md create mode 100644 .agents/phases/todo/187_sync_peak_memory_line/00_phase.md create mode 100644 .agents/phases/todo/187_sync_peak_memory_line/01_memory_helper.md create mode 100644 .agents/phases/todo/187_sync_peak_memory_line/02_sync_log_lines.md create mode 100644 .agents/phases/todo/187_sync_peak_memory_line/03_status_key_and_ui.md create mode 100644 .agents/phases/todo/187_sync_peak_memory_line/04_e2e_peak_memory.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/00_phase.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/01_hnsw_migration.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/02_ef_search_setting.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/03_plan_and_recall_pins.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/04_recall_battery_gate.md create mode 100644 .agents/phases/todo/188_chunks_hnsw_index/05_e2e_index_chat_and_runbook.md create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.err create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md.status create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.validate create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.err create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md.status create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.validate create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.err create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md.status create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.validate create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.err create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md.status create mode 100644 .agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.validate create mode 100644 tests/e2e/test_prune_cascade_cleanup.py create mode 100644 tests/integration/test_prune_cascade_statements.py diff --git a/.agents/phases/complete/183_sync_passive_delete_cascade/00_phase.md b/.agents/phases/complete/183_sync_passive_delete_cascade/00_phase.md new file mode 100644 index 0000000..c96f263 --- /dev/null +++ b/.agents/phases/complete/183_sync_passive_delete_cascade/00_phase.md @@ -0,0 +1,57 @@ +# Phase 183 — Mass prune must not load the embeddings it is deleting + +**Source:** owner incident 2026-10-15 — "brain-of-code hit 4 GB of RAM while running the sync" (the sync `started repos=4` at 11:19:23, the run reached `_prune` at 12:14:14, pruned **3,008 litellm docs**, and the VM locked up hard enough that boot −1 has no shutdown record at all). +**Story:** n/a (incident-derived, owner directive 2026-10-15) +**Context:** `app/models.py` — `Document.chunks` (**L227–229**, `relationship(back_populates="document", cascade="all, delete-orphan")` — **no `passive_deletes`**), `Chunk` (L232), `Chunk.embedding` (L241, `Vector(EMBEDDING_DIM)` = 768), `Chunk.document` (L247); `app/rag/importer.py` — `_prune` (**L3860**, the one-shot `docs = session.scalars(select(Document).where(Document.source.in_(source_names))).all()` at **L3909**, the per-doc `session.delete(doc)` at **L3931**, the single `if pruned: session.commit()` at **L3934–3935**), `_replace_content_chunks` (L992, the `doc.chunks = [...]` ORPHAN-removal path at L1033), `_store_summary` (L1595, the `for old in [c for c in doc.chunks if c.is_summary]` loop at L1648); `app/api/git_sources.py` — `delete_git_source` (L855, the `db.delete(doc)` loop at **L926**, the docstring's "chunks + embeddings via the `all, delete-orphan` cascade" line at **L863**, `docs_pruned` at L919); `app/api/docs.py` — the document-delete endpoint's `db.delete(row)` (L700); `alembic/versions/0001_initial_schema.py` — **L54 `sa.ForeignKey("documents.id", ondelete="CASCADE")`** (the DB already cascades — production confirms: `chunks_document_id_fkey`, `confdeltype='c'`); `tests/unit/test_importer.py` + `tests/integration/test_importer_e2e.py` (the importer families), `tests/integration/test_source_removal_api.py` + `tests/e2e/test_source_removal_cleanup.py` (the removal families), `tests/e2e/test_force_push_resync.py` (the "upstream deleted the file, re-sync prunes it" shape). + +## Measured baseline (the numbers this phase is written against; recorded 2026-10-15) +- A loaded 768-dim `Chunk.embedding` costs **24.8 KiB of Python per chunk** (list header + 768 float objects) against **3 KiB on disk** — the ORM round-trip is an 8× expansion, and pgvector hands back a `list[float]`, never a buffer. +- `session.delete(doc)` on a relationship **without** `passive_deletes` makes SQLAlchemy load the child collection first (it must know which children to orphan/delete). Measured against the dev DB with production-shaped rows (400 docs × 25 chunks, ~745-char chunk content): **RSS grows linearly at ~0.24 MB per document deleted** = 25 chunks × ~9.7 KiB resident, +241 MB over 400 deletes, and NOTHING is released before the single commit because `app/db.py:30` builds sessions with `expire_on_commit=False`. +- Applied to the incident run: 3,008 docs × ~24 chunks ≈ **720 MB of float lists materialised to delete 72k rows**, on top of the ~300 MB the same walk already retained — on a 3,904 MB VM with **no swap**, sharing RAM with a 3.1 GB Postgres. + +## Objective +Make a mass prune cost O(documents) instead of O(chunks × 24.8 KiB): `Document.chunks` declares `passive_deletes=True` so the DB's already-present `ON DELETE CASCADE` removes the chunk rows without the ORM ever hydrating them, on both mass-delete surfaces (the importer's prune and the admin's source removal), with every user-visible behaviour (counts, log lines, image/page-copy cleanup, the phase-122/126 prune guards, the `changed_paths` feed into the folder summaries) byte-identical. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — the pin is statement count, not RSS (task 01).** Memory assertions are flaky; the SQLAlchemy `before_cursor_execute` event stream is not. A new `tests/integration/test_prune_cascade_statements.py` counts, for one prune, every SELECT whose SQL targets the `chunks` table. PRE-fix the count is **exactly one per pruned document** (the lazy child collection load) and that test is GREEN before any code change — it IS the demonstration, exactly as phase 179 task 01 pinned the freeze with a latency probe. The measured RSS delta (the ~0.24 MB/doc figure above) is recorded in the module docstring as the corroborating number, asserted only as a loose ceiling (`< 40 MB` for a 200-doc fixture, where pre-fix measures ≈ 124 MB) so a noisy CI box cannot fail the suite on allocator behaviour. +- **D2 — the fix is one keyword, and it is a semantic claim about the schema (task 02).** `app/models.py:227–229` becomes `relationship(back_populates="document", cascade="all, delete-orphan", passive_deletes=True)`. That is only correct because the FK really cascades — so task 02 also adds a **schema-drift guard test** that reads the live constraint (`pg_constraint` where `conrelid='chunks'::regclass and contype='f'` → `confdeltype == 'c'`) and fails loudly if a future migration ever drops `ON DELETE CASCADE`, because at that point `passive_deletes=True` would silently orphan chunk rows. **No new alembic revision** — 0001 L54 already declares it, and the incident box's live schema matches (A13 stays satisfied because nothing changes). +- **D3 — both mass-delete surfaces, one keyword's blast radius (task 02).** With `passive_deletes=True` the ORM stops hydrating children for EVERY `session.delete(document)`: the importer's `_prune` (L3931), `delete_git_source`'s `db.delete(doc)` loop (`app/api/git_sources.py:926` — the endpoint that took 47 s for 2,303 immich docs on 2026-10-15 11:18:30), and `app/api/docs.py:700`. Audit-and-note, not change: none of the three reads `doc.chunks` AFTER deleting it (verified: `_prune` only touches `image_path`/`page_count`/`source`/`path`; the git-sources endpoint only counts; the docs endpoint only logs). The `doc.chunks = [...]` reassignment inside `_replace_content_chunks` (L1033) is the **orphan-removal** path, which `passive_deletes` does NOT short-circuit — the collection still loads there (a re-embed needs the old rows gone and the new ones built); it runs only for CHANGED files (217 in the incident run), and it is explicitly OUT OF SCOPE (A2) — the docstring says so in one line so the next reader does not "discover" it. +- **D4 — the cascade contract is verified at the row level, not assumed (tasks 01 + 03).** Every prune test asserts `select(count()).select_from(Chunk).where(Chunk.document_id.in_(deleted ids)) == 0` — no orphan chunk rows, before and after the change — plus the existing `documents`-level assertions. This is the phase's non-negotiable: a delete that stops hydrating children must still delete the children. +- **D5 — the E2E pins the owner-visible prune (task 03).** `tests/e2e/test_prune_cascade_cleanup.py` (this phase's dedicated Playwright file, AGENTS rule 4), the `test_force_push_resync.py` + `test_source_removal_cleanup.py` house shapes (dedicated module app, mock LLM, `BOR_SOURCES_DIR`/`BOR_PDF_PAGE_DIR` under `tmp_path_factory`, the shared E2E Postgres truncated before AND after each test): a fixture source of N files is synced to `success` from the Sources page, a subset is deleted from disk upstream, the re-sync's counts line reports `pruned=N-subset` and those documents vanish from the KB (the ls/tree row is gone, `GET /api/docs` 404s the path, the Documents list total drops), while the chunk rows are gone too (the test's own DB query — the same D4 assertion, end to end) and the surviving documents still answer a chat question (a partial prune never damages the RAG). `E2E_REAL_LLM=1` skipped. +- **NOT touched:** the `_prune` batching (phase 184), the fast-path column set (phase 185), `expire_on_commit` (deliberately left `False` — every request path relies on reading attributes after commit), `Chunk.embedding`'s type, the retrieval query, the frontend, and any LLM/vision path. + +## Dependencies +— (none: the app is fully functional at the head of `complete/`; phases 184 and 185 build on this one, in that order) + +## Tasks +1. `01_prune_cascade_probe.md` — the deterministic statement-count probe (one `chunks` SELECT per pruned doc, PRE-fix, green before the fix) + the row-level orphan assertions + the measured RSS delta recorded in the module docstring. +2. `02_passive_deletes.md` — the fix (`passive_deletes=True`), the live-FK drift guard, the three delete-site audit, the probe flipped to the post-fix assertion (`0` chunk SELECTs), the git-sources endpoint docstring correction. +3. `03_e2e_prune_cascade.md` — `tests/e2e/test_prune_cascade_cleanup.py` (the phase's dedicated Playwright suite), the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Integration:** `tests/integration/test_prune_cascade_statements.py` (the statement-count probe, the orphan-row assertions, the loose RSS ceiling + the recorded measurement). +- **Unit:** the existing importer prune pins stay green as written — the phase-122 images-off prune guard, the phase-126 parent-PDF rule, `changed_paths` population, the `summary.pruned` count (`tests/unit/test_importer.py`, `tests/unit/test_importer_ignore.py`); add a unit pin that `Document.__mapper__.relationships["chunks"].passive_deletes is True`. +- **Regression:** `tests/integration/test_importer.py`, `test_importer_e2e.py`, `test_source_removal_api.py`, `test_sync_api.py`, `test_git_sources_api.py` — all green, byte-identical counts and log lines. +- **E2E (AGENTS rule 4, task 03):** `tests/e2e/test_prune_cascade_cleanup.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `README.md`: the operations note that a prune deletes chunk rows in the DATABASE (FK cascade) and that the operator-visible cost of a mass prune is bounded by documents, not chunks (the number the incident was missing). +- The `_prune` docstring (`app/rag/importer.py:3860`) gains the one-paragraph "why `passive_deletes` + the FK cascade, and why the re-embed path is different" note; `app/models.py:227` gains the one-line "do not remove without re-checking the FK's ON DELETE CASCADE (pinned by tests/integration/test_prune_cascade_statements.py)" warning. + +## Completion Criteria +- [ ] `app/models.py::Document.chunks` declares `passive_deletes=True`; a unit pin asserts it. +- [ ] `uv run pytest tests/integration/test_prune_cascade_statements.py -v` green with the POST-fix assertion (zero `chunks` SELECTs during a prune of N docs, `0` orphan chunk rows, RSS delta under the pinned ceiling) and the pre/post measurements + the incident reference in the module docstring. +- [ ] The live-FK drift guard is green and would fail if `ON DELETE CASCADE` were absent (proven by the test's own negative fixture or a documented, executed one-off check). +- [ ] `uv run pytest` green, `app/` coverage >90%, `uv run ruff check . && uv run pyright` clean. +- [ ] `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` green **in isolation** (the counts line, the vanished documents, zero orphan chunks, the surviving KB still answers). +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (verified in the final pass). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `fix(models): prune with passive_deletes so deleting documents never hydrates their embeddings`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — mechanism: `passive_deletes=True`, not a bulk-`delete()` rewrite.** The relationship keyword keeps `session.delete(doc)` as the one delete shape every caller already uses (importer prune, source removal, doc delete), so there is no new control flow to get wrong; the bulk-rewrite variant belongs to phase 184's batching, which builds on this. +- **A2 — out of scope: the `doc.chunks = [...]` orphan path** (`_replace_content_chunks` L1033). `passive_deletes` does not cover collection replacement, and a re-embed genuinely needs the loaded collection; it runs for changed files only. Documented as a known, bounded, deliberate non-fix. +- **A3 — no migration.** The FK's `ON DELETE CASCADE` ships in 0001 L54 and is verified live; this phase adds a drift guard instead of a schema change. +- **A4 — the probe is statement-count based.** Deterministic, CI-safe, and it names the exact waste; the RSS figure is documentation plus a loose ceiling, never the assertion that can flake a build. diff --git a/.agents/phases/complete/183_sync_passive_delete_cascade/01_prune_cascade_probe.md b/.agents/phases/complete/183_sync_passive_delete_cascade/01_prune_cascade_probe.md new file mode 100644 index 0000000..ab92e31 --- /dev/null +++ b/.agents/phases/complete/183_sync_passive_delete_cascade/01_prune_cascade_probe.md @@ -0,0 +1,29 @@ +# Task 01 — Pin what a prune actually loads (the probe, green PRE-fix) + +**Phase:** `183_sync_passive_delete_cascade` · **Source:** owner incident 2026-10-15 — the sync hit 4 GB of RAM while pruning 3,008 documents +**Story:** n/a + +## Objective +A deterministic integration probe that shows — before any code changes — that deleting documents hydrates their chunk rows (and therefore every 768-dim embedding) into Python, one SELECT per document. The probe is GREEN pre-fix; task 02 flips it. The measured cost is recorded in the module docstring so the next reader never has to re-derive it. + +## Work +1. `tests/integration/test_prune_cascade_statements.py` — a NEW file, house patterns: + - Session/DB fixtures per `tests/integration/test_importer.py` (the shared test DB, the per-test truncate, `SessionLocal` from `app.db`). + - A `seed_docs` helper: **200** `Document` rows, `source="cascade-probe"`, each with **25** `Chunk` rows whose `content` is ~745 chars of fixed text and whose `embedding` is a deterministic 768-float vector (`[i / 768 for i in range(768)]` rotated by the doc index — deterministic, and L2-normalised enough not to matter here). Commit. + - **The statement capture:** a `sqlalchemy.event` listener on the engine's `before_cursor_execute` that appends `(statement, params)` tuples for the duration of the test; a helper `chunk_selects(captured)` returns the statements that are `SELECT` and reference `"FROM chunks"` (case-insensitive, `chunks` word-boundary so `documents` never matches). Install in the fixture, remove in teardown (`event.remove`) so no other test in the session sees the listener. + - **The probe test** `test_prune_loads_every_chunk_collection`: build the prune inputs the way `_prune` receives them (`source_names={"cascade-probe"}`, `seen=set()` → everything prunes), call `app.rag.importer._prune(session, source_names, seen, images=False, summary=ImportSummary(), page_dir=None)` (the real signature, `app/rag/importer.py:3860`), then assert: + - `chunk_selects(captured) == 200` — **exactly one child-collection SELECT per deleted document.** This is the PRE-fix assertion and it is GREEN now. Mark it so the flip is unmissable: a module-level `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC` constant with the comment `# 1 pre-phase-183 (the lazy child load); 0 post-phase-183 — task 02 flips this`. + - every `Chunk` row of the 200 documents is gone (`select(count()).select_from(Chunk).where(Chunk.document_id.in_(ids)) == 0`) — the D4 no-orphan contract, asserted before the fix so post-fix it means "unchanged, and not by accident". + - the `documents` rows are gone, `_prune`'s return value is 200, and `ImportSummary.changed_paths` holds the 200 `(source, path)` pairs. + - **The loose memory ceiling** `test_prune_memory_delta_has_a_ceiling`: the same 200-doc prune with `VmRSS` read from `/proc/self/status` before and after (a tiny local helper inside the test module — no new `app/` code in this task), asserting the delta is **< 40 MB**, with the MEASURED value recorded in the docstring. Pre-fix this measures ≈ **48 MB** for 200 docs (200 × 25 × 24.8 KiB ≈ 124 MB of float lists, of which the allocator keeps what it can reuse — the point is the order, not the exact figure), so the pinned ceiling must be picked from the ACTUAL measured pre-fix number: set it to roughly **one third** of what the pre-fix run measures, i.e. a ceiling the pre-fix run VIOLATES if it can be made to (if the pre-fix delta comes in under 40 MB on this box, LOWER the ceiling to a value the post-fix run clears with room and document both numbers — the assertion's job is to catch the hydrate-the-embeddings behaviour, never to fail on allocator noise). If the delta cannot be made discriminating on this hardware, keep the test but assert only `< 40 MB` and state in the docstring that the statement count is the load-bearing pin. +2. **The module docstring (the measurement record):** the incident in one paragraph (the 2026-10-15 sync pruned 3,008 litellm docs; the app cgroup peaked at 3.3 GB twice that week; the VM has 3,904 MB, no swap, and shares RAM with a 3.1 GB Postgres); the mechanism (`app/models.py:227–229` has no `passive_deletes`, so `session.delete(doc)` must load the children; `app/db.py:30` sets `expire_on_commit=False`, so nothing is released before the single commit at `importer.py:3934`); the measured unit costs (**24.8 KiB** of Python per 768-dim embedding vs 3 KiB on disk; **~0.24 MB resident per document deleted** at 25 chunks/doc, measured 2026-10-15 against the dev DB with production-shaped rows); this test's own measured numbers (fill them in from the run); and the verdict line **CONFIRMED — one chunk SELECT per pruned document** (if the probe comes back with 0 selects, STOP and flag it in the docstring and the phase report instead of proceeding). +3. Verify: `uv run pytest tests/integration/test_prune_cascade_statements.py -v` green; `uv run pytest` green (the event listener is scoped to the fixture, nothing else sees it). + +## Testing & Quality +- Integration: the probe family above (statement count, orphan-row check, return value, `changed_paths`, the loose RSS ceiling). +- Coverage: **>90%** on `app/` — test-only task, `app/` untouched, so the gate must simply stay green. + +## Completion Criteria +- [ ] `uv run pytest tests/integration/test_prune_cascade_statements.py -v` green with `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 1` (the pre-fix assertion) and the 0-orphan-chunks assertion. +- [ ] The module docstring records the incident, the mechanism, the 24.8 KiB / 0.24 MB-per-doc unit costs, and this run's measured statement count + RSS delta, plus the **CONFIRMED** verdict. +- [ ] `uv run pytest` green; no `app/` change in this task. diff --git a/.agents/phases/complete/183_sync_passive_delete_cascade/02_passive_deletes.md b/.agents/phases/complete/183_sync_passive_delete_cascade/02_passive_deletes.md new file mode 100644 index 0000000..bfa2e47 --- /dev/null +++ b/.agents/phases/complete/183_sync_passive_delete_cascade/02_passive_deletes.md @@ -0,0 +1,52 @@ +# Task 02 — The fix: `passive_deletes=True`, with the FK contract pinned + +**Phase:** `183_sync_passive_delete_cascade` · **Source:** owner incident 2026-10-15 — the sync hit 4 GB of RAM while pruning 3,008 documents +**Story:** n/a + +## Objective +Stop the ORM from hydrating chunk rows (and their 768-dim embeddings) when a `Document` is deleted: declare `passive_deletes=True` on `Document.chunks` so the database's existing `ON DELETE CASCADE` does the work, prove the schema really cascades with a drift guard, and flip task 01's probe to the post-fix assertion. + +## Work +1. `app/models.py` — `Document.chunks` (**L227–229**): + ```python + chunks: Mapped[list[Chunk]] = relationship( + back_populates="document", + cascade="all, delete-orphan", + # Phase 183: deleting a Document removes its chunks IN THE DATABASE + # (chunks_document_id_fkey is ON DELETE CASCADE — alembic 0001 L54, + # pinned live by tests/integration/test_prune_cascade_statements.py). + # WITHOUT this keyword the ORM loads every child before deleting it, + # which hydrates every 768-dim embedding (~24.8 KiB of Python each): + # a 3,008-document prune cost ~720 MB and took the 2026-10-15 VM + # (3,904 MB, no swap) down. Do not remove this without re-checking + # the FK — the drift guard fails the suite if the cascade is gone. + passive_deletes=True, + ) + ``` + Nothing else in `models.py` changes (`Chunk.document` L247, `Chunk.embedding` L241 untouched). +2. `app/rag/importer.py` — the `_prune` docstring (L3860): append the phase-183 paragraph — the children go through the FK cascade, so no chunk row is ever SELECTed here; the `changed_paths` / image-copy / page-copy cleanup is unchanged; and the **A2 note**: the re-embed path `_replace_content_chunks` (L992, `doc.chunks = [...]` at L1033) is collection REPLACEMENT, which `passive_deletes` does not cover and which legitimately needs the loaded collection — a known bounded non-fix. +3. `app/api/git_sources.py` — `delete_git_source`'s docstring (**L863**, "documents (chunks + embeddings via the `all, delete-orphan` cascade)") → correct it to name the FK cascade (phase 183): the rows go in the database, the app never loads them. No logic change in the endpoint (verified: the loop at L919–927 only counts `docs_pruned`, it never reads `doc.chunks`). +4. `tests/integration/test_prune_cascade_statements.py` — flip the probe: `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, comment updated (`0 post-phase-183 — the FK cascade; 1 pre-phase-183 was the lazy child load`). Keep every other assertion (0 orphan chunks, 0 documents, the return value, `changed_paths`, the RSS ceiling — now clearing it by a wide margin) and extend the docstring: `CONFIRMED → FIXED (phase 183, passive_deletes=True)` with the post-fix measured statement count and RSS delta. +5. **The drift guard** — same file, `test_chunks_fk_cascades_at_database_level`: query the live schema and assert the cascade exists, so `passive_deletes=True` can never silently orphan rows. + ```python + row = session.execute( + text("select confdeltype from pg_constraint " + "where conrelid = 'chunks'::regclass and contype = 'f'") + ).all() + assert row and all(r[0] == "c" for r in row) # 'c' = ON DELETE CASCADE + ``` + Prove the guard is real (not a tautology) in the task's own verification: run it once against a scratch DB where the constraint is recreated `ON DELETE NO ACTION` (a throwaway database the task creates and drops — never the shared test DB) and record in the test's comment that the guard fails there, plus a unit pin `Document.__mapper__.relationships["chunks"].passive_deletes is True` in `tests/unit/test_importer.py`. +6. **Audit note (no code change):** grep `session.delete` / `db.delete` for the `Document` deletes (`app/rag/importer.py:3931`, `app/api/git_sources.py:926`, `app/api/docs.py:700`) and confirm none reads `doc.chunks` after the delete; record the confirmation in the task's completion note. If any site DOES read the collection after deleting, stop and report it rather than papering over it. +7. Verify: `uv run pytest tests/integration/test_prune_cascade_statements.py -v` green post-fix; then the families the keyword touches — `uv run pytest tests/integration/test_importer.py tests/integration/test_importer_e2e.py tests/integration/test_source_removal_api.py tests/integration/test_sync_api.py tests/integration/test_git_sources_api.py -v`; then `uv run pytest`. + +## Testing & Quality +- Integration: the flipped probe + the live-schema drift guard + the five families above, byte-identical counts and log lines. +- Unit: the `passive_deletes` mapper pin in `tests/unit/test_importer.py`. +- Coverage: **>90%** on `app/` (the `app/` change is one keyword + docstrings). + +## Completion Criteria +- [ ] `app/models.py::Document.chunks` has `passive_deletes=True` with the explanatory comment; the unit mapper pin is green. +- [ ] `uv run pytest tests/integration/test_prune_cascade_statements.py -v` green with `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, zero orphan chunks, and the docstring's CONFIRMED → FIXED record with both measured numbers. +- [ ] The drift guard is green on the test DB and documented as failing on a scratch DB whose FK lacks `ON DELETE CASCADE`. +- [ ] `_prune`'s and `delete_git_source`'s docstrings match the new behaviour (the A2 re-embed note included); the three-delete-site audit is recorded. +- [ ] `uv run pytest` green, `app/` coverage >90%, `uv run ruff check . && uv run pyright` clean. diff --git a/.agents/phases/complete/183_sync_passive_delete_cascade/03_e2e_prune_cascade.md b/.agents/phases/complete/183_sync_passive_delete_cascade/03_e2e_prune_cascade.md new file mode 100644 index 0000000..6e107c0 --- /dev/null +++ b/.agents/phases/complete/183_sync_passive_delete_cascade/03_e2e_prune_cascade.md @@ -0,0 +1,34 @@ +# Task 03 — E2E: a mass prune leaves nothing behind (the phase's dedicated Playwright suite) + +**Phase:** `183_sync_passive_delete_cascade` · **Source:** owner incident 2026-10-15 — the sync hit 4 GB of RAM while pruning 3,008 documents +**Story:** n/a + +## Objective +Pin the fix against the real pipeline the owner actually drives: upstream deletes files from a synced source, the admin re-syncs from the Sources page, and the prune removes exactly the vanished documents, leaves **zero orphan chunk rows**, keeps the surviving KB answerable, and reports the same counts line it always did. Then run the full gates, do the docs pass, and commit the phase. + +## Work +1. `tests/e2e/test_prune_cascade_cleanup.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped), following the `tests/e2e/test_force_push_resync.py` + `tests/e2e/test_source_removal_cleanup.py` house shapes point for point: + - **Isolation:** a per-module app on a dedicated port (the conftest session app is never started in the isolated run; `tests/e2e/conftest.py` — `APP_PORT` L32, `app_server` L106, `page` L219), the mock LLM on its conftest port, a dedicated `BOR_SOURCES_DIR` and `BOR_PDF_PAGE_DIR` under `tmp_path_factory`, the shared E2E Postgres truncated **before AND after** every test, admin login via `tests/e2e/auth_helpers.py`. + - **Fixture:** a `prune_source` fixture dir with **9** importable `.md` files across two folders (`notes/keep-1..3.md`, `notes/gone-1..6.md`), one of the `keep-*` files carrying the sentinel phrase the chat test asks about. + - **Test 1 `test_deleted_files_prune_without_orphans`:** sync → assert the run reaches `success` and the Sources counts line reports `added=9` / `pruned=0` → delete the six `gone-*.md` files from disk → re-sync → assert the counts line reports `pruned=6` and `added=0` (the unchanged 3 stay unchanged) → the six documents are gone from the KB (the ls/tree row for `notes/gone-1.md` is absent, `GET /api/docs` for the path 404s via `page.request`, the Documents total is 3) → **zero orphan chunk rows** for the deleted document ids (the test's own `psycopg`/SQLAlchemy query against the E2E DB, the D4 assertion) and `select(count()).select_from(Chunk)` equals the sum of the 3 survivors' chunk counts → the run leaves no `documents` row whose `source` lost its file. + - **Test 2 `test_surviving_kb_still_answers_after_a_partial_prune`:** with the same pruned state, ask the chat the sentinel question (the `test_chat_rag.py` shape) and assert the answer cites the surviving sentinel document and the turn completes (no error state, no empty answer) — the prune did not damage retrieval. + - **Test 3 `test_source_removal_prunes_every_document_of_the_source`:** the admin path (`delete_git_source`, `app/api/git_sources.py:855`) through the Git sources UI or `page.request` `DELETE /api/git-sources/{id}` → every document AND every chunk row of that source is gone (the same orphan assertion), `docs_pruned` matches the seeded count via the log/response contract the endpoint already exposes, and the remaining source is intact. + - The module docstring: the incident reference, the fix (`passive_deletes=True` + the FK cascade), the test→observable mapping, the run-in-isolation command, the prerequisites (DB up: `podman compose up -d db`). +2. **Docs pass (the phase's final documentation review):** `README.md` + `AGENTS.md` reviewed in full against the post-phase app and every inconsistency fixed (pre-existing included). Add to `README.md`'s operations/sync section: a prune deletes chunk rows through the database cascade, so the cost of a mass prune scales with documents, not embeddings — and the `chunks` FK's `ON DELETE CASCADE` is load-bearing (pinned by `tests/integration/test_prune_cascade_statements.py`), so it must never be dropped. +3. **The full gates:** + - `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_source_removal_cleanup.py -v --no-cov` and `uv run pytest tests/e2e/test_force_push_resync.py -v --no-cov` — green in isolation (the two prune/removal E2E families this phase must not have broken; run them AFTER the new suite so the DB truncate ordering stays clean). + - `uv run pytest --cov=app --cov-report=term-missing` — the whole suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. +4. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit covering `app/models.py`, the two docstring updates, the new integration + E2E files, the unit pin and the `.agents/` tree — e.g. `fix(models): prune with passive_deletes so deleting documents never hydrates their embeddings`, body naming the 2026-10-15 incident and the measured 24.8 KiB/embedding, ~0.24 MB/doc. Move the phase dir to `.agents/phases/complete/183_sync_passive_delete_cascade/`. + +## Testing & Quality +- E2E: the dedicated suite above + the two regression E2E families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` green **in isolation** (the counts line, the vanished documents, zero orphan chunks, the surviving KB answers, the source-removal variant clean). +- [ ] `tests/e2e/test_source_removal_cleanup.py` and `tests/e2e/test_force_push_resync.py` green in isolation. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (the cascade-is-load-bearing note included). +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/phases/todo/184_sync_bounded_session/00_phase.md b/.agents/phases/todo/184_sync_bounded_session/00_phase.md new file mode 100644 index 0000000..76e2b50 --- /dev/null +++ b/.agents/phases/todo/184_sync_bounded_session/00_phase.md @@ -0,0 +1,59 @@ +# Phase 184 — The sync's session stops growing with the knowledge base + +**Source:** owner incident 2026-10-15 (as phase 183) — the import run held ONE session (`app/db.py:30` sets `expire_on_commit=False`) for the whole walk + prune, so every row it touched stayed resident until the run ended. +**Story:** n/a (incident-derived, owner directive 2026-10-15) +**Depends on:** `183_sync_passive_delete_cascade` (the batched prune deletes documents with the FK cascade doing the child work — it is only cheap because of `passive_deletes=True`). + +**Context:** `app/db.py` — **L30 `SessionLocal = sessionmaker(bind=engine, autoflush=False, expire_on_commit=False, future=True)`** (left EXACTLY as it is — every request path reads attributes after commit); `app/rag/importer.py` — `import_sources` (**L708**, `owns_session = session is None` at **L878**, `session = SessionLocal()` at **L880**, the `total` pre-walk at L884–899, the per-source loop at L901, the per-file loop at **L916** with `progress(source, rel, summary.files, total)` at **L943** and `await _index_file(...)` at **L945**, the `EmbeddingError` → `session.rollback()` handler at L951–958, the per-source post-passes `_enrich_pending_images` (**L965**) and `_backfill_image_contexts` (**L972**), `_prune(...)` (**L979**), `session.close()` in the `finally` at L984–985), `_prune` (**L3860**, `docs = session.scalars(select(Document).where(Document.source.in_(source_names))).all()` at **L3909**, `session.delete(doc)` at **L3931**, the single commit at **L3934–3935**), `_delete_image_copy` (takes `image_path: str | None`), `_delete_page_copies(doc, page_dir)` (needs only `doc.id` + `doc.page_count`); `app/api/sync.py` — `import_sources(...)` called with **no `session=`** at **L725** (the importer owns it — this phase never changes that contract); `app/config.py` (the `BOR_`-prefixed `Settings` pattern, `chunk_target_chars` at L228 as the shape), `app/core/settings_catalog.py` — `GROUPS` at **L72** (the `"Import & sources"` group) and the `CATALOG` entry shape at L226, pinned complete by `tests/unit/test_settings_catalog.py::test_catalog_covers_exactly_model_fields` (**`set(CATALOG) == set(Settings.model_fields)`** — a new `Settings` field without a catalogue row FAILS the build); `tests/unit/test_importer.py`, `tests/integration/test_importer_e2e.py`, `tests/integration/test_sync_api.py`, `tests/e2e/test_sync_changed_only.py`, `tests/e2e/test_sync_summary_visibility.py`. + +## Measured baseline (2026-10-15, this repo's dev DB, production-shaped rows) +- A `Document`'s `content` averages **15.2 KB** (17,407 docs, 252.6 MB of markdown in the incident DB), and the identity map keeps every one the walk touched — see phase 185 for the column-level fix. +- Objects are released by NOTHING during the run: `expire_on_commit=False` means a commit does not drop attributes, and the importer never expunges. The identity map is therefore the run's memory profile, and it is unbounded: it scales with (files walked) + (chunks created) + (chunks hydrateable per prune — phase 183). + +## Objective +Bound the import run's resident set independently of the size of the knowledge base: `session.expunge_all()` on a fixed cadence at the one point where no previously-loaded object is reachable, and `_prune` released in committed batches instead of one transaction at the end of the whole import — with every count, log line, guard, cancel boundary and summary number byte-identical. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — the pin is the identity map, not RSS (task 01).** `len(session.identity_map)` is exact, deterministic and it IS the retention surface. `tests/integration/test_import_session_retention.py` runs an import of N files through `import_sources` (the real entry point, the `test_importer_e2e.py` fixture shapes + the mock-embedder pattern from `tests/unit/test_importer.py`) and asserts the map size **at the end of the run**: PRE-fix it is `≈ docs + chunks + documents-touched` (record the measured number), and the test pins `>= N` to prove accumulation. The post-fix assertion (task 02) is `<= reset_files + slack`. RSS is recorded in the docstring only, never asserted (phase 183 A4's rule, inherited). +- **D2 — one new setting per new cadence, catalogue-compliant (tasks 02 + 03).** `sync_session_reset_files: int = 200` (`BOR_SYNC_SESSION_RESET_FILES`) and `sync_prune_batch: int = 500` (`BOR_SYNC_PRUNE_BATCH`). **`0` means "never reset / one transaction" = the exact pre-phase behaviour**, the operator's rollback knob without a re-deploy (the `BOR_LOG_LARGE_RESPONSE_BYTES` precedent). Both get `SettingMeta` entries in the `"Import & sources"` group (the completeness pin makes this mandatory, not optional), both documented in `.env.example` and `README.md`, neither `restart_required` (the importer reads `llm.settings` per run) and neither `secret`. +- **D3 — the reset point is the top of the per-file loop, and nowhere else (task 02).** Inside `import_sources`, at the top of the L916 loop body — after the PREVIOUS file's `_index_file` has returned, before this file's `progress(...)` hook at L943 — call `session.expunge_all()` every `sync_session_reset_files` files (`summary.files % reset == 0`), plus once at each source boundary before the two post-passes (L965/L972) so a source's walk residue never crosses into the next source. Proof of safety that the task must state in the comment: no object from a finished file is reachable afterwards (`_index_file` returns `None`, the progress/chunk/image hooks only assign fields on `_status`, `dates_map`/`ignore` are plain data, and `total` was computed before the loop), and the `EmbeddingError` handler's `session.rollback()` (L951–958) is safe with detached objects because it touches none. The two post-passes select their own rows fresh (`L3241`, `L3391`), so they are unaffected. +- **D4 — the prune is batched on light rows, not streamed cursors (task 03).** `_prune` replaces `select(Document).where(...).all()` with `select(Document).where(...).options(load_only(Document.id, Document.source, Document.path, Document.is_image, Document.image_path, Document.page_count)).order_by(Document.id)` — the exact column set the guards and the copy cleanup need (`is_image` + `path` for the phase-122 toggle guard and the phase-126 `_EMBEDDED_IMAGE_RE` parent rule, `image_path` for `_delete_image_copy`, `id` + `page_count` for `_delete_page_copies`) — and commits every `sync_prune_batch` deletes instead of once at the end (`sync_session_reset_files`'s sibling knob, `0` = the pre-phase single commit). Deletes stay `session.delete(doc)` (the phase-183 `passive_deletes` makes them child-load-free; a bulk `delete()` is NOT used, so `changed_paths`, the log line and the guards keep their exact per-document shape). `yield_per`/server-side cursors are explicitly rejected: a flush inside an open streaming result is a trap, and the light rows are ~1 KB each. +- **D5 — batch commits change the prune's atomicity, deliberately (A2).** Pre-phase a crash mid-prune rolled the whole prune back; post-phase a crash leaves a PREFIX pruned. That is safe here and must be stated in the docstring: `seen` is recomputed from the filesystem on every run, so the next sync converges; the import already commits per file throughout the walk (L1176–1190 and friends), so the run was never one transaction. The `summary.pruned` count is the count of documents actually deleted, unchanged in meaning. +- **D6 — the E2E pins "nothing changed, at scale" (task 04).** `tests/e2e/test_sync_session_reset.py`: a fixture source of **more files than the reset cadence** (the module pins `BOR_SYNC_SESSION_RESET_FILES=3` with 10 files, so the cadence fires repeatedly mid-run), synced from the Sources page → every document present exactly once, the counts line identical to a `BOR_SYNC_SESSION_RESET_FILES=0` run of the same fixture (the byte-identical assertion the phase lives on), a second sync reports all-`unchanged` with `pruned=0`, no 500s and no network-error state on the page, and the KB totals in the Sources stats match the DB. Plus the prune path with the batch pin at 2 (so a multi-batch prune is exercised through the UI). +- **NOT touched:** `app/db.py:30` (the global `expire_on_commit=False` — changing it would re-fetch rows across every request path for no benefit here), the importer's `owns_session` contract, the per-file index/summary/vision logic, the walk's column set (phase 185), `scripts/git_sync.py` (phase 186), the retrieval path, the frontend. + +## Dependencies +- `183_sync_passive_delete_cascade` — required: batched `session.delete(doc)` without child hydration is the whole point of the batch cadence. + +## Tasks +1. `01_identity_map_probe.md` — the deterministic identity-map probe (an `import_sources` run pinned to accumulate; the measurement recorded) plus the `sync_session_reset_files` / `sync_prune_batch` settings + catalogue + `.env.example` groundwork with the completeness pin green. +2. `02_session_reset_cadence.md` — the `expunge_all` cadence at the per-file loop top + source boundaries, the probe flipped to the bounded assertion, the safety-argument comment. +3. `03_batched_prune.md` — `_prune`'s light `load_only` select + per-batch commits + the atomicity note, the prune-side retention and statement-count pins. +4. `04_e2e_bounded_sync.md` — `tests/e2e/test_sync_session_reset.py` (the phase's dedicated Playwright suite), the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Unit:** the settings defaults + `0`-means-disabled semantics; `tests/unit/test_settings_catalog.py` completeness pin green with the two new fields in `"Import & sources"`; the existing prune guard pins (`images`-off skip, phase-126 parent rule, `changed_paths`) green as written. +- **Integration:** `tests/integration/test_import_session_retention.py` (the identity-map probe, both cadences), `tests/integration/test_importer*.py`, `test_sync_api.py`, `test_source_removal_api.py` — byte-identical counts and log lines. +- **E2E (AGENTS rule 4, task 04):** `tests/e2e/test_sync_session_reset.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `README.md` + `.env.example`: the two new settings (default, `0` = pre-phase behaviour, what each bounds); one operations paragraph stating that the sync's memory is now bounded per-run rather than per-KB, and where the operator sets the ceiling. + +## Completion Criteria +- [ ] `sync_session_reset_files=200` and `sync_prune_batch=500` exist on `Settings`, appear in `CATALOG` under `"Import & sources"`, in `.env.example`, in `README.md`; `uv run pytest tests/unit/test_settings_catalog.py -v` green. +- [ ] `uv run pytest tests/integration/test_import_session_retention.py -v` green with the POST-fix bound (identity-map size ≤ `reset_files + slack` with the cadence on, unbounded with it `0`) and both measured numbers in the module docstring. +- [ ] `import_sources` expunges only at the two proven-safe points (documented in the code comment); `_prune` deletes light rows in `sync_prune_batch` commits with the phase-122/126 guards and `changed_paths` intact. +- [ ] Every existing importer/sync/removal test is green with unchanged expectations (the byte-identical contract). +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. +- [ ] `uv run pytest tests/e2e/test_sync_session_reset.py -v --no-cov` green **in isolation**. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (verified in the final pass). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `perf(import): bound the sync session with a reset cadence and batched prune commits`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — mechanism: periodic `expunge_all` + batched prune commits.** Chosen over "new session per source" (would re-derive the importer's `owns_session`/rollback semantics for a smaller win) and over "flip `expire_on_commit=True` for the sync session" (re-reads rows on every commit across a 27k-file walk and relies on implicit expiry instead of an explicit release). +- **A2 — the prune's atomicity changes to per-batch, accepted.** `seen` is recomputed from disk each run so a partial prune converges on the next sync; the import already commits per file, so no run-level transaction existed to lose. +- **A3 — both knobs default ON, `0` restores pre-phase behaviour exactly.** The operator keeps a no-redeploy rollback (the phase-167 `BOR_LOG_LARGE_RESPONSE_BYTES` precedent). +- **A4 — no server-side cursors / `yield_per`.** A flush inside an open streaming result is a correctness trap; the light prune rows cost ~1 KB each, so materialising them is not the problem this phase is fixing. diff --git a/.agents/phases/todo/184_sync_bounded_session/01_identity_map_probe.md b/.agents/phases/todo/184_sync_bounded_session/01_identity_map_probe.md new file mode 100644 index 0000000..dbeec10 --- /dev/null +++ b/.agents/phases/todo/184_sync_bounded_session/01_identity_map_probe.md @@ -0,0 +1,33 @@ +# Task 01 — Pin the run's retention (identity map) and lay the two knobs + +**Phase:** `184_sync_bounded_session` · **Source:** owner incident 2026-10-15 — the sync's memory grew with the KB, not the work +**Story:** n/a + +## Objective +A deterministic integration probe on the real `import_sources` entry point showing that the run's SQLAlchemy identity map grows with every file touched and is never released (PRE-fix, green before the fix), plus the two new settings (`sync_session_reset_files`, `sync_prune_batch`) with their catalogue rows so the cadence tasks have somewhere to read their numbers from. + +## Work +1. `app/config.py` — two new `Settings` fields, the `chunk_target_chars` (L228) shape, `BOR_` prefix from `env_prefix` (L38): + - `sync_session_reset_files: int = 200` — the number of files between identity-map releases inside `import_sources`; `0` = never (the pre-phase behaviour). Docstring names the phase and the `0` semantics. + - `sync_prune_batch: int = 500` — the number of pruned documents per commit inside `_prune`; `0` = one commit at the end (the pre-phase behaviour). + Both `int`, both validated to reject negatives (the `show_text_max_*` "negative refuses startup" precedent — follow whatever those fields do; if they validate in a `model_validator`, add these to the same validator). +2. `app/core/settings_catalog.py` — `CATALOG` entries in the **`"Import & sources"`** group (`GROUPS` L72, the group `chunk_target_chars` lives in is `"Chunking & embedding"` — use `"Import & sources"`, these are sync-run knobs): `"Sync session reset files"`, `"Sync prune batch"`. Neither `secret`, neither `restart_required`, both `overridable`. Verify `uv run pytest tests/unit/test_settings_catalog.py -v` — the `set(CATALOG) == set(Settings.model_fields)` pin fails until both entries exist, so this step is proven by that test going green. +3. `.env.example` — both keys with their defaults and a one-line comment each (`0` = pre-phase behaviour). +4. `tests/integration/test_import_session_retention.py` — a NEW file, house patterns (`tests/integration/test_importer_e2e.py` for the fixture-KB + mock-embedder shape, `tests/unit/test_importer.py` for the fake `LLMClient`): + - Fixture: a `sources` tmp dir with **12** `.md` files (each ~4 KB so each produces ≥2 chunks), a `Settings` with `sync_session_reset_files=0` and `sync_prune_batch=0` (the PRE-phase behaviour, explicitly), an isolated truncate of `documents`/`chunks` before and after. + - `test_import_accumulates_everything_it_touched`: run `await import_sources([root], llm, prune=False, session=session)` on a **fresh** session, then assert `len(session.identity_map) >= 12 * 2` (the 12 documents plus their chunks — the assertion that proves nothing is released; pin the exact measured figure in a module constant with the measured value in a comment). Record in the docstring: the map size, the chunk count, and `sum(1 for s in session.identity_map.all_states() if s.class_ is Document)`. + - `test_reset_cadence_is_configurable_off`: with `sync_session_reset_files=0` the map is exactly as the previous test measured (the `0` = pre-phase pin, which stays true after task 02 ships — it is the rollback guarantee, not a pre-fix only assertion). + - A module-level `_map_size(session)` helper (`len(session.identity_map)`) so tasks 02/03 assert through one door. + - Docstring: the incident paragraph, `app/db.py:30`'s `expire_on_commit=False` (which every request path depends on and this phase does NOT change), the measured numbers, the verdict **CONFIRMED — the run's identity map grows monotonically and nothing releases it**. +5. Verify: `uv run pytest tests/integration/test_import_session_retention.py tests/unit/test_settings_catalog.py -v` green; `uv run pytest` green. + +## Testing & Quality +- Unit: the two settings' defaults, negative rejection, and the catalogue completeness pin. +- Integration: the retention probe (both cadence-off assertions). +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] Both settings exist with the pinned defaults, `0`-means-disabled semantics, catalogue rows in `"Import & sources"`, `.env.example` entries. +- [ ] `uv run pytest tests/unit/test_settings_catalog.py -v` green (the completeness pin proves the catalogue work). +- [ ] `uv run pytest tests/integration/test_import_session_retention.py -v` green with the accumulation assertion and the docstring's measured numbers + **CONFIRMED** verdict. +- [ ] `uv run pytest` green; no behaviour change yet (the settings are read by nothing until tasks 02/03). diff --git a/.agents/phases/todo/184_sync_bounded_session/02_session_reset_cadence.md b/.agents/phases/todo/184_sync_bounded_session/02_session_reset_cadence.md new file mode 100644 index 0000000..1483d8d --- /dev/null +++ b/.agents/phases/todo/184_sync_bounded_session/02_session_reset_cadence.md @@ -0,0 +1,45 @@ +# Task 02 — The fix: release the identity map on a cadence at the safe point + +**Phase:** `184_sync_bounded_session` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Bound the import run's resident set: `session.expunge_all()` every `sync_session_reset_files` files at the one point in `import_sources` where no previously-loaded object is reachable, plus once at each source boundary — with every count, log line and error path byte-identical, and task 01's probe flipped to the bounded assertion. + +## Work +1. `app/rag/importer.py` — `import_sources` (L708): + - Read the knob once before the source loop: `reset_every = llm.settings.sync_session_reset_files` (`0` → disabled; resolve it ONCE so a mid-run settings overlay cannot change cadence between files). + - **The per-file reset** — at the TOP of the L916 loop body, before `summary.files += 1` / the `progress(...)` call at L943: + ```python + # Phase 184 (task 02): the identity-map release. expire_on_commit=False + # (app/db.py:30) means a commit drops NOTHING, so without this the run + # holds every Document and Chunk it touched until the END of the import + # — the 2026-10-15 incident's second driver. This is the ONLY safe + # point: the previous file's _index_file has already returned (it + # returns None), the progress/chunk/image hooks only assign fields on + # the sync's own status object, dates_map/ignore are plain data, and + # `total` was computed before the loop. The EmbeddingError handler + # below calls session.rollback() and touches no object of its own, so + # detached state is irrelevant to it. reset_every == 0 → never + # (the exact pre-phase behaviour, the operator's rollback knob). + if reset_every and summary.files and summary.files % reset_every == 0: + session.expunge_all() + ``` + - **The source-boundary reset** — immediately before `await _enrich_pending_images(...)` (L965), the same guard (`if reset_every: session.expunge_all()`), with a one-line comment: the two post-passes (L965, L972) select their own rows fresh (L3241, L3391), so no object survives the reset. + - Do NOT reset inside `_index_file`, `_unchanged_doc`, `_store_summary`, or any post-pass — their locals are live across statements. +2. `tests/integration/test_import_session_retention.py` — extend, then flip: + - `test_reset_cadence_bounds_the_map`: the same 12-file fixture with `sync_session_reset_files=3` → after the run `assert _map_size(session) <= 3 + SLACK` (pick `SLACK` from the measurement — it must cover the last partial batch plus whatever a single file's own chunks contribute; document the arithmetic in the comment) and the import still reports `added=12`, the same chunk total, and the same documents as the cadence-`0` run (the byte-identical comparison the phase lives on — compare the two runs' `ImportSummary` fields and the DB's per-path chunk counts). + - Keep `test_import_accumulates_everything_it_touched` and `test_reset_cadence_is_configurable_off` (cadence `0` still accumulates → the rollback guarantee). + - `test_reset_cadence_survives_an_embedding_error`: a stub embedder that raises `EmbeddingError` on file #4 (with `sync_session_reset_files=2`, so a reset has already happened) → the run records `errors=1`, keeps going, and finishes with the other 11 files indexed; asserts nothing blows up on detached state (the handler's `session.rollback()` path). + - Docstring: `CONFIRMED → FIXED (phase 184)`, both measured map sizes (cadence 0 vs cadence 3), and the post-fix bound. +3. Verify: `uv run pytest tests/integration/test_import_session_retention.py -v`; then the families — `uv run pytest tests/integration/test_importer.py tests/integration/test_importer_e2e.py tests/integration/test_importer_dates.py tests/integration/test_sync_api.py tests/unit/test_importer.py -v`; then `uv run pytest`. + +## Testing & Quality +- Integration: the bounded-map test, the cadence-`0` guarantee, the error-path test, the byte-identical summary comparison. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `import_sources` calls `session.expunge_all()` at exactly the two documented points, guarded by `reset_every`, with the safety argument in the comment. +- [ ] `uv run pytest tests/integration/test_import_session_retention.py -v` green (bounded with the cadence on, unbounded with it `0`, error path clean, identical summaries). +- [ ] No existing importer/sync test needed an expectation change. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/184_sync_bounded_session/03_batched_prune.md b/.agents/phases/todo/184_sync_bounded_session/03_batched_prune.md new file mode 100644 index 0000000..cf76ff5 --- /dev/null +++ b/.agents/phases/todo/184_sync_bounded_session/03_batched_prune.md @@ -0,0 +1,51 @@ +# Task 03 — The prune stops holding 17k full documents and releases in batches + +**Phase:** `184_sync_bounded_session` · **Source:** owner incident 2026-10-15 (the prune phase was where the box died) +**Story:** n/a + +## Objective +Make `_prune`'s resident set independent of the size of the knowledge base: select only the columns its guards and cleanup need (not `content`, avg 15.2 KB × 17,407 rows), and commit every `sync_prune_batch` deletes instead of one transaction after the whole loop. The per-document semantics — the phase-122 images-off guard, the phase-126 parent-PDF rule, `changed_paths`, the `import: pruned …` log line, the returned count — stay byte-identical. + +## Work +1. `app/rag/importer.py` — `_prune` (L3860): + - Replace **L3909** `docs = session.scalars(select(Document).where(Document.source.in_(source_names))).all()` with a light-column, deterministically ordered select: + ```python + docs = session.scalars( + select(Document) + .options( + load_only( + Document.id, Document.source, Document.path, + Document.is_image, Document.image_path, Document.page_count, + ) + ) + .where(Document.source.in_(source_names)) + .order_by(Document.id) + ).all() + ``` + with `from sqlalchemy.orm import load_only` added to the module's SQLAlchemy imports, and a comment naming exactly why those six columns (the phase-122 toggle guard reads `is_image`; the phase-126 `_EMBEDDED_IMAGE_RE` parent rule reads `path`; `_delete_image_copy` takes `image_path`; `_delete_page_copies` needs `id` + `page_count`; the `seen` tuple and `changed_paths` need `source` + `path`) and why `content` is not among them (252 MB of markdown per full KB walk; phase 185 does the same job for the fast path). + - Add the batch commit. After `session.delete(doc)` / `pruned += 1` / the `logger.info("import: pruned …")` line, commit on the batch boundary and once at the end: + ```python + if prune_batch and pruned and pruned % prune_batch == 0: + session.commit() # phase 184: a long prune no longer holds a huge transaction + ... + if pruned: + session.commit() + ``` + `prune_batch = llm-free`: `_prune` has no `llm` argument, so pass the cadence in as a keyword — `prune_batch: int = 500` on the signature, and the caller at L979 passes `prune_batch=llm.settings.sync_prune_batch`. Keep `prune_batch=0` meaning "single commit at the end" (the pre-phase behaviour, and the default for the standalone `scripts/import_docs.py` path if it calls `_prune` directly — check and keep its call byte-identical by leaving the keyword default at the value that preserves its behaviour). + - Update the `_prune` docstring: the light-row contract, the batch-commit contract, and the **A2 atomicity note** (a crash mid-prune leaves a prefix pruned and the next sync converges, because `seen` is recomputed from the filesystem; the import already commits per file, so no run-level transaction existed). +2. `tests/integration/test_import_session_retention.py` — add the prune side: + - `test_prune_holds_no_content`: seed 30 docs × 5 chunks, prune them all, and assert (a) via a `before_cursor_execute` capture that the documents SELECT issued by `_prune` does NOT list `documents.content` in its column list (the same deterministic technique phase 183 task 01 uses for chunks — assert on the captured statement text), and (b) 0 orphan chunks afterwards. + - `test_prune_batch_commits`: `sync_prune_batch=10` over 30 prunable docs → assert the commit count (a `after_commit`/`after_transaction_end` event counter, or spy `Session.commit` with `monkeypatch`) is 3 (+ whatever the caller adds), that all 30 documents and 150 chunks are gone, and `summary.pruned == 30`; with `sync_prune_batch=0` the commit count is 1 and every other number is identical. + - `test_prune_guards_unaffected_with_light_rows`: the phase-122 images-off guard and the phase-126 parent-PDF rule keep working with `load_only` rows (an `is_image` doc with `images=False` survives; an embedded-image doc whose parent PDF rel is in `seen` survives) — this is the assertion that the six columns really are enough. +3. **Check the deferred-attribute trap explicitly:** anything inside the prune loop that touches a non-loaded attribute would emit a per-row lazy SELECT — the statement capture in `test_prune_holds_no_content` must assert **no per-document follow-up SELECT** (the captured count of `FROM documents` statements stays at 1 for the initial load). If a lazy load appears, the missing column goes into `load_only` (and the comment updates), it does not get papered over with `raiseload`. +4. Verify: `uv run pytest tests/integration/test_import_session_retention.py -v`; then `uv run pytest tests/integration/test_importer.py tests/integration/test_importer_e2e.py tests/integration/test_source_removal_api.py tests/unit/test_importer.py tests/unit/test_importer_ignore.py -v`; then `uv run pytest`. + +## Testing & Quality +- Integration: the no-`content` statement pin, the batch-commit pins (both cadences), the guard pins with light rows, the orphan-row pins. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `_prune` selects through `load_only(id, source, path, is_image, image_path, page_count)` ordered by `Document.id`, commits every `sync_prune_batch` (default 500, `0` = single commit), and the docstring records the column rationale + the A2 atomicity note. +- [ ] `uv run pytest tests/integration/test_import_session_retention.py -v` green (no `content` in the prune SELECT, no per-row lazy load, the batch counts, the guards, zero orphans). +- [ ] `scripts/import_docs.py`'s prune path (if any) keeps its pre-phase behaviour. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/184_sync_bounded_session/04_e2e_bounded_sync.md b/.agents/phases/todo/184_sync_bounded_session/04_e2e_bounded_sync.md new file mode 100644 index 0000000..27121f0 --- /dev/null +++ b/.agents/phases/todo/184_sync_bounded_session/04_e2e_bounded_sync.md @@ -0,0 +1,35 @@ +# Task 04 — E2E: the bounded sync still syncs everything (the phase's dedicated Playwright suite) + +**Phase:** `184_sync_bounded_session` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Prove through the UI that the phase's two cadences change the run's memory profile and NOTHING else: the same fixture produces the same counts line with the cadence on and off, every document lands exactly once, a re-sync is all-`unchanged`, a multi-batch prune behaves, and the Sources page never stalls or errors. Then the full gates, the docs pass, and the atomic commit. + +## Work +1. `tests/e2e/test_sync_session_reset.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped), the `tests/e2e/test_sync_changed_only.py` + `test_sync_summary_visibility.py` house shapes: + - **Isolation:** a dedicated module app + port (`tests/e2e/conftest.py` — `APP_PORT` L32, `app_server` L106, `page` L219), the mock LLM, dedicated `BOR_SOURCES_DIR` / `BOR_PDF_PAGE_DIR` under `tmp_path_factory`, the shared E2E Postgres truncated before AND after every test, admin login via `tests/e2e/auth_helpers.py`. + - **Fixture:** 10 `.md` files across two folders (`a/1..5.md`, `b/1..5.md`), one carrying the sentinel phrase. + - **Test 1 `test_reset_cadence_produces_the_identical_run`:** module sets `BOR_SYNC_SESSION_RESET_FILES=3` (so the cadence fires 3× through a 10-file walk). Sync from the Sources page → `success`; the counts line reports the 10 added (the phase-98 counts-line contract, `test_sync_summary_visibility.py` shape); the KB totals match `select(count())` on `documents` (10) and on `chunks` (>10); no network-error state and no 500 during the run's polls. + - **Test 2 `test_resync_is_unchanged_with_the_cadence_on`:** a second sync from the same fixture (nothing changed on disk) → `unchanged=10`, `added=0`, `pruned=0`, `success`, and the DB's chunk total is unchanged (no silent re-embed — the cadence must not be losing rows and re-adding them). + - **Test 3 `test_multi_batch_prune_from_the_ui`:** `BOR_SYNC_PRUNE_BATCH=2` (module-level env), delete 5 of the 10 files from disk, re-sync → the counts line reports `pruned=5`, the 5 documents are gone from the KB and from the ls/tree rows, the survivors' chunk total is unchanged, and 0 orphan chunk rows exist (the test's own DB query). The batch cadence therefore commits 3 times through the UI's own run. + - **Test 4 `test_cadence_zero_matches_cadence_three` (the byte-identical cross-check):** re-seed the fixture, run with `BOR_SYNC_SESSION_RESET_FILES=0` and compare the resulting `documents`/`chunks` per-path counts against test 1's recorded numbers (the test records its numbers into a module-level fixture or asserts against a fixed expected table — whichever is deterministic in this suite's house style; the assertion must be a real comparison, not a tautology). The two runs must agree on every path, every chunk count, and the counts line. + - Module docstring: the incident reference, the two knobs + their defaults + `0` semantics, the test→observable mapping, the run-in-isolation command, the prerequisites (`podman compose up -d db`). +2. **Docs pass:** `README.md` + `AGENTS.md` reviewed in full against the post-phase app; every inconsistency fixed, pre-existing included. `README.md` gains the operations paragraph (the sync's memory is now bounded per run; the two knobs; `0` restores the old behaviour) and `.env.example` matches the shipped defaults. +3. **The full gates:** + - `uv run pytest tests/e2e/test_sync_session_reset.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_sync_changed_only.py -v --no-cov` and `uv run pytest tests/e2e/test_sync_git_freeze.py -v --no-cov` — green in isolation (the sync-suite neighbours this phase must not disturb; run after the new suite). + - `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. +4. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit covering `app/config.py`, `app/core/settings_catalog.py`, `app/rag/importer.py`, the integration + E2E files, `.env.example`, the docs and the `.agents/` tree — e.g. `perf(import): bound the sync session with a reset cadence and batched prune commits`. Move the phase dir to `.agents/phases/complete/184_sync_bounded_session/`. + +## Testing & Quality +- E2E: the dedicated suite above + the two regression E2E families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_sync_session_reset.py -v --no-cov` green **in isolation** (identical run with the cadence on, unchanged re-sync, multi-batch prune, the cadence-0 vs cadence-3 cross-check). +- [ ] `tests/e2e/test_sync_changed_only.py` and `tests/e2e/test_sync_git_freeze.py` green in isolation. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md`, `AGENTS.md` and `.env.example` accurate and consistent with the whole project. +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/phases/todo/185_sync_defer_content_fast_path/00_phase.md b/.agents/phases/todo/185_sync_defer_content_fast_path/00_phase.md new file mode 100644 index 0000000..d38d0a1 --- /dev/null +++ b/.agents/phases/todo/185_sync_defer_content_fast_path/00_phase.md @@ -0,0 +1,55 @@ +# Phase 185 — The unchanged fast path stops fetching 15 KB of markdown per file + +**Source:** owner incident 2026-10-15 (as phases 183/184) — every one of the ~27k files the walk inspected hydrated its `Document` row, `content` included, and `expire_on_commit=False` kept it resident for the whole run. +**Story:** n/a (incident-derived, owner directive 2026-10-15) +**Depends on:** none functionally; sequenced after `183`/`184` because it touches the same walk and reuses their statement-capture technique. + +**Context:** `app/rag/importer.py` — `_index_file` (**L1384**; the row load **`doc = session.scalar(select(Document).where(Document.source == source, Document.path == rel))` at L1452** — the ONLY place the fast path gets its row, `_file_hash` streamed digest at L1451, the fast-path guard at L1460–1469, the three sibling loads at **L1914** (PDF), **L2675** (embedded image), **L3674** (image) which the phase-147 fast path shares), `_row_kind_mismatch` (**L1352** — reads `doc.is_image` only), `_unchanged_doc` (**L1061** — `doc.file_hash = file_digest` at L1137, the **phase-118 summary-NULL backfill `if doc.summary is None: await _store_summary(..., content=doc.content ...)` at L1139–1142**, `_regate_strip` at L1145–1157 which reads `doc.page_count` + `doc.path` + `doc.full_path` for a `.pdf` with a strip, the D1 manual-date lock at L1163, the D4 date refresh at L1172), `_store_summary` (L1595), `ImportSummary.log()` (L520); `app/db.py:30` (`expire_on_commit=False`, unchanged); `tests/unit/test_importer_unchanged_fast_path.py` (the phase-147 matrix, whose docstring already pins "the phase-118 backfill on the fast path runs from the STORED `doc.content`"), `tests/unit/test_importer.py`, `tests/integration/test_importer.py`, `tests/integration/test_importer_e2e.py`, `tests/e2e/test_document_viewer.py` + `tests/e2e/test_document_summaries.py` (the surfaces that render stored `content` and summaries), `tests/e2e/test_sync_changed_only.py`. + +## Measured baseline (2026-10-15, incident box) +`documents`: 17,407 rows, `avg(length(content)) = 15,217` chars, **252.6 MB of markdown total**. `_index_file` loads the whole row for every file it inspects — the incident run inspected ~27k files and re-indexed **217**, so the run paid ~250 MB of markdown resident (`expire_on_commit=False`) to discover that 99% of the KB had not changed. + +## Objective +Keep the phase-147 unchanged fast path's behaviour exactly as it is and make it stop hydrating `Document.content`: the row load defers the one column the fast path does not read, so a full walk's memory scales with row metadata instead of markdown — while the phase-118 summary-NULL backfill (the one fast-path branch that genuinely needs the stored content) keeps working from the stored text. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — `defer(Document.content)`, not a `load_only` allowlist (task 02).** The surgical form: `select(Document).options(defer(Document.content)).where(...)` at the row loads. A `load_only` allowlist would defer every column not named — including columns added by a future phase, which is how a "memory fix" turns into a mysterious N+1 six phases later. `defer()` names exactly what is dropped, so everything else keeps today's semantics, and pyright/the suite stay honest. Applies to the shared row load at **L1452** and the three sibling loads that share the phase-147 fast path — **L1914** (PDF), **L2675** (PDF-embedded image), **L3674** (image) — via ONE module-level helper so the four sites cannot drift: `_DOC_ROW_OPTS: tuple[Callable[[str], Any], ...] = (defer(Document.content),)` (or a tiny `_doc_row_stmt(source, rel)` helper returning the prepared `select` — the executor picks one, and a unit pin asserts every fast-path load uses it). +- **D2 — NO `raiseload`.** `noload`/`raiseload` would break the phase-118 backfill (L1139–1142) and any future legitimate reader. With plain `defer`, reading `doc.content` emits one lazy SELECT for that row and behaves exactly as today. The trade is explicit: unchanged files with a stored summary pay ZERO extra queries; a summary-NULL backfill pays ONE extra SELECT, which is a per-row, one-time, already-rare cost. The docstring says so. +- **D3 — the pin is the statement's column list, deterministic (task 01).** `tests/integration/test_import_fast_path_columns.py` captures SQL through `before_cursor_execute` (the phase-183 task 01 technique) and asserts, for an unchanged re-sync of a seeded KB: (a) the documents SELECT issued by `_index_file` does **not** mention `documents.content` in its select list — PRE-fix that assertion FAILS, so task 01 writes it as the *inverse* assertion (content IS present, green pre-fix) and task 02 flips it, exactly the 179 pattern; (b) exactly ONE documents SELECT per file (no lazy follow-up) for a doc WITH a summary; (c) for a doc with `summary IS NULL`, the backfill still runs and emits exactly ONE extra documents SELECT (the lazy `content` load) and produces the same summary chunk as today. +- **D4 — observable parity is non-negotiable.** Every count (`files/added/updated/unchanged/dates_updated`), every log word (`import: unchanged …`, `import: date-refreshed …`, `import: summary …`), the D1 manual-date lock, the phase-163 PDF strip re-gate and the phase-118 backfill result stay byte-identical — this phase changes WHICH COLUMNS travel, never what the run does. `tests/unit/test_importer_unchanged_fast_path.py` must go green with **no expectation edits** (if an expectation must change, the design is wrong — stop and flag). +- **D5 — the E2E pins the two surfaces `content` feeds (task 03).** `tests/e2e/test_fast_path_deferred_content.py`: (1) a doc whose `summary` is NULL in the DB (the test nulls it directly, the way phase 118's tests do) is re-synced from the Sources page and its summary comes back from the STORED content (the document page's summary renders + the summary chunk exists + `dates_updated`/counts stay honest); (2) a normal unchanged doc's viewer still shows the FULL stored text (no truncation, no blank) after a sync that ran with the deferral; (3) a changed file re-indexes and its viewer shows the NEW text. `E2E_REAL_LLM=1` skipped. +- **NOT touched:** `app/db.py:30`, the walk itself (`iter_importable_files`, L624 — its `sorted(root.rglob("*"))` materialisation is a separate, much smaller cost and stays), `_unchanged_doc`'s logic, `_store_summary`, the index/summary/vision pipelines, the retrieval path, the frontend, phases 183/184's files beyond the shared row-load line. + +## Dependencies +— (none functionally; run after `183_sync_passive_delete_cascade` and `184_sync_bounded_session` so the walk's three memory fixes land in one coherent story) + +## Tasks +1. `01_fast_path_statement_probe.md` — the statement-capture probe pinning that the fast-path row load hydrates `content` (PRE-fix, green now), the zero-extra-SELECT pin for a doc with a summary, and the backfill's lazy-load pin. +2. `02_defer_content.md` — the fix: the shared `_doc_row_stmt` / `defer(Document.content)` helper wired into the four fast-path row loads, the docstrings, the probe flipped. +3. `03_e2e_deferred_content.md` — `tests/e2e/test_fast_path_deferred_content.py` (the phase's dedicated Playwright suite), the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Unit:** `tests/unit/test_importer_unchanged_fast_path.py` green with **no expectation edits** (the phase-147 matrix: zero-LLM-call, D1/D4 date rules, the phase-118 backfill from stored content); `tests/unit/test_importer.py` green. +- **Integration:** `tests/integration/test_import_fast_path_columns.py` (the column-list + statement-count pins), `tests/integration/test_importer.py`, `test_importer_e2e.py`, `test_importer_dates.py`, `test_sync_api.py`. +- **E2E (AGENTS rule 4, task 03):** `tests/e2e/test_fast_path_deferred_content.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `README.md` (sync/import operations): one sentence — the unchanged fast path reads row metadata only, so a full-resync walk's cost is metadata-bound; the rare summary-NULL backfill re-reads the stored content (one query) and that is intentional. +- `app/rag/importer.py`: the module docstring's phase-147 fast-path paragraph and `_index_file`'s docstring gain the deferral note (with the D2 no-`raiseload` rationale), so nobody "optimises" it into a lazy-load landmine. + +## Completion Criteria +- [ ] The four fast-path row loads share ONE deferral helper; `Document.content` is deferred and nothing else is. +- [ ] `uv run pytest tests/integration/test_import_fast_path_columns.py -v` green with the POST-fix column-list assertion (no `documents.content` in the fast-path SELECT), the one-SELECT-per-file pin, and the backfill's single lazy-load pin, all measured numbers in the module docstring. +- [ ] `uv run pytest tests/unit/test_importer_unchanged_fast_path.py -v` green with zero edited expectations. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. +- [ ] `uv run pytest tests/e2e/test_fast_path_deferred_content.py -v --no-cov` green **in isolation** (backfill from stored content, full text in the viewer, changed file re-indexes). +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (verified in the final pass). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `perf(import): defer Document.content on the unchanged fast path so a full walk no longer hydrates the KB`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — `defer(Document.content)` only.** Not a `load_only` allowlist (future columns would silently become lazy), not `undefer_group`s, not a DTO/read-model rewrite of the importer (too large a change to keep phase-147 parity provable). +- **A2 — no `raiseload`/`noload`.** The phase-118 backfill must keep reading the stored content; failing loudly there would be a behaviour regression, and the extra SELECT is per-row and one-time. +- **A3 — the walk itself (`iter_importable_files`) is out of scope** this phase — its `sorted(root.rglob("*"))` list is a one-off per source (tens of thousands of `Path`s, ~10 MB), orders of magnitude below the markdown this phase removes. diff --git a/.agents/phases/todo/185_sync_defer_content_fast_path/01_fast_path_statement_probe.md b/.agents/phases/todo/185_sync_defer_content_fast_path/01_fast_path_statement_probe.md new file mode 100644 index 0000000..496b648 --- /dev/null +++ b/.agents/phases/todo/185_sync_defer_content_fast_path/01_fast_path_statement_probe.md @@ -0,0 +1,27 @@ +# Task 01 — Pin that the fast path hydrates the markdown it never reads + +**Phase:** `185_sync_defer_content_fast_path` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +A deterministic, SQL-level probe proving that the phase-147 unchanged fast path SELECTs `documents.content` for every file it inspects (green PRE-fix, task 02 flips it), that it issues exactly one documents SELECT per unchanged file, and that the phase-118 summary-NULL backfill path behaves as documented. + +## Work +1. `tests/integration/test_import_fast_path_columns.py` — a NEW file, house patterns (`tests/integration/test_importer_e2e.py`'s fixture-KB + fake-embedder shape; the statement capture is phase 183 task 01's: a `sqlalchemy.event` `before_cursor_execute` listener installed and removed in a fixture). + - Fixture: 4 `.md` files (each ~8 KB so `content` is unmistakably large), seeded through the real `import_sources` so rows look production-shaped, then a SECOND `import_sources` run over the same tree (the unchanged run) with the capture active. One of the four rows gets `documents.summary = NULL` committed before the second run (the phase-118 backfill case). + - Helpers: `doc_selects(captured)` → statements that are SELECTs `FROM documents`; `loads_content(stmt_text)` → True when the statement's select list names `content` (match on the aliased/plain column token, and state the matcher in a comment so it cannot match `content_hash`). + - `test_unchanged_run_hydrates_content_pre_fix`: assert `len(doc_selects(captured)) >= 4` and `all(loads_content(s) for s in doc_selects(captured))` — **the PRE-fix assertion**, with the marker comment `# task 02 flips this: no content in the fast-path SELECT`. Record the captured statement text for ONE file in the docstring (the evidence). + - `test_unchanged_file_is_one_select`: for a doc WITH a summary, the per-file documents statement count is exactly **1** (this pin must hold before AND after the fix — post-fix it is the guard against the deferral turning into a per-row lazy-load storm). + - `test_summary_null_backfill_reads_stored_content`: for the summary-NULL row, assert the second run produced a summary chunk (`is_summary` chunk exists, its content matches the stored markdown) and, before the fix, no extra documents SELECT is needed; structure the assertion so task 02's post-fix expectation is a documented constant: `EXPECTED_EXTRA_DOC_SELECTS_FOR_BACKFILL = 0` → task 02 sets it to `1` (the single lazy `content` load) with the arithmetic in the comment. +2. Docstring: the measured baseline (17,407 rows, avg 15,217 chars, 252.6 MB), the mechanism (`_index_file` L1452 loads the whole row; `app/db.py:30` `expire_on_commit=False` keeps it), the captured statement evidence, and the verdict **CONFIRMED — the fast path SELECTs `content` for every file inspected**. +3. Verify: `uv run pytest tests/integration/test_import_fast_path_columns.py -v` green; `uv run pytest` green. + +## Testing & Quality +- Integration: the three pins above. +- Coverage: **>90%** on `app/` (test-only task). + +## Completion Criteria +- [ ] `uv run pytest tests/integration/test_import_fast_path_columns.py -v` green with the PRE-fix content-in-SELECT assertion and `EXPECTED_EXTRA_DOC_SELECTS_FOR_BACKFILL = 0`. +- [ ] The one-SELECT-per-file pin is green and marked as must-stay-green post-fix. +- [ ] The docstring carries the baseline numbers, the captured statement, and the **CONFIRMED** verdict. +- [ ] `uv run pytest` green; no `app/` change in this task. diff --git a/.agents/phases/todo/185_sync_defer_content_fast_path/02_defer_content.md b/.agents/phases/todo/185_sync_defer_content_fast_path/02_defer_content.md new file mode 100644 index 0000000..038f605 --- /dev/null +++ b/.agents/phases/todo/185_sync_defer_content_fast_path/02_defer_content.md @@ -0,0 +1,53 @@ +# Task 02 — The fix: defer `Document.content` on the fast-path row loads + +**Phase:** `185_sync_defer_content_fast_path` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Make the unchanged fast path read row metadata instead of markdown: one shared statement helper that defers `Document.content` (and nothing else), wired into the four row loads that share the phase-147 fast path, with task 01's probe flipped and zero behaviour change. + +## Work +1. `app/rag/importer.py` — add ONE module-level helper next to `_file_hash` (L687): + ```python + def _doc_row_stmt(source: str, rel: str) -> Select[tuple[Document]]: + """The importer's single Document-row statement (phase 185). + + ``defer(Document.content)`` — the unchanged fast path (phase 147) reads + file_hash/is_image/page_count/summary/dates, never the stored markdown, + and app/db.py builds sessions with expire_on_commit=False, so hydrating + content for EVERY inspected file is what took the 2026-10-15 run to + ~250 MB on a 17k-document KB (avg 15,217 chars/row). ONE column is + deferred on purpose: a load_only allowlist would silently defer every + column a later phase adds, and the deferred read would then surface as + an unexplained N+1. No raiseload/noload: the phase-118 summary-NULL + backfill (``_unchanged_doc``) genuinely reads ``doc.content`` and must + keep working — it costs one extra SELECT for that one row. + """ + return ( + select(Document) + .options(defer(Document.content)) + .where(Document.source == source, Document.path == rel) + ) + ``` + with `from sqlalchemy import select` already present and `from sqlalchemy.orm import defer` added (import style: match the module's existing import block). +2. Replace the four identical fast-path row loads with `session.scalar(_doc_row_stmt(source, rel))`: + - `_index_file` **L1452** (the text/branch dispatch), + - `_index_pdf_file` **L1914**, + - `_index_one_embedded_image` **L2675** (the `image_doc` variant — same statement shape, different variable name), + - `_index_image_file` **L3674**. + Keep every surrounding line untouched (`file_digest` computed first, the guard, the branch bodies). Grep for any OTHER `select(Document).where(Document.source ==` in the importer to confirm no fifth site exists; if one does (e.g. the post-passes at L3241/L3391 select differently shaped rows), leave it and say so in the task note — those are NOT fast-path loads. +3. Docstrings: the module docstring's phase-147 paragraph and `_index_file`'s docstring (L1440–1470 region) gain the deferral sentence; `_unchanged_doc`'s step-3 comment (the phase-118 backfill) gains "…which is the one fast-path read that pulls the deferred `content` back — one SELECT, one time, by design". +4. `tests/integration/test_import_fast_path_columns.py` — flip: `EXPECTED_CONTENT_IN_FAST_PATH_SELECT = False` (assert NO `content` token in the fast-path documents SELECT list) and `EXPECTED_EXTRA_DOC_SELECTS_FOR_BACKFILL = 1`; keep the one-SELECT-per-file pin green; docstring `CONFIRMED → FIXED (phase 185, defer(Document.content))` with the post-fix captured statement text (the evidence that markdown no longer travels). +5. `tests/unit/test_importer_unchanged_fast_path.py` — must go green **without editing an expectation**. If a unit test stubs the session (no real lazy load possible) and fails on the deferral, fix the TEST HARNESS honestly (e.g. the fake returns a real `Document` instance, which already has `content` set) rather than weakening a pin — and say in the task note which harness detail was adjusted. +6. Verify: `uv run pytest tests/integration/test_import_fast_path_columns.py tests/unit/test_importer_unchanged_fast_path.py -v`; then `uv run pytest tests/integration/test_importer.py tests/integration/test_importer_e2e.py tests/integration/test_importer_dates.py tests/integration/test_import_extensions_env.py tests/integration/test_sync_api.py -v`; then `uv run pytest`. + +## Testing & Quality +- Integration: the flipped column pin, the one-SELECT pin, the single-lazy-load backfill pin. +- Unit: the phase-147 matrix green, zero edited expectations. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `_doc_row_stmt` exists and all four fast-path loads use it (grep evidence in the task note); only `Document.content` is deferred. +- [ ] `uv run pytest tests/integration/test_import_fast_path_columns.py -v` green post-fix with both flipped constants and the post-fix captured statement in the docstring. +- [ ] `uv run pytest tests/unit/test_importer_unchanged_fast_path.py -v` green with zero edited expectations (any harness adjustment documented). +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/185_sync_defer_content_fast_path/03_e2e_deferred_content.md b/.agents/phases/todo/185_sync_defer_content_fast_path/03_e2e_deferred_content.md new file mode 100644 index 0000000..888345e --- /dev/null +++ b/.agents/phases/todo/185_sync_defer_content_fast_path/03_e2e_deferred_content.md @@ -0,0 +1,32 @@ +# Task 03 — E2E: the deferred column still shows up where the owner reads it (the phase's dedicated Playwright suite) + +**Phase:** `185_sync_defer_content_fast_path` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Prove through the UI that the deferral is invisible to the owner: the phase-118 summary-NULL backfill still produces a summary from the STORED content, an unchanged document's viewer still renders its full stored text, and a changed file re-indexes to the new text. Then the full gates, the docs pass, the atomic commit. + +## Work +1. `tests/e2e/test_fast_path_deferred_content.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped), the `tests/e2e/test_document_summaries.py` / `test_document_viewer.py` / `test_sync_changed_only.py` house shapes (dedicated module app + port, mock LLM, dedicated `BOR_SOURCES_DIR`, the shared E2E Postgres truncated before AND after each test, admin login via `tests/e2e/auth_helpers.py`). + - **Test 1 `test_summary_null_backfill_survives_the_deferral`:** seed+sync a fixture with 2 files, then set `documents.summary = NULL` for one row directly (the way phase 118's tests do), re-sync from the Sources page → the run reports `unchanged` (not `added`/`updated`) and the document page shows a non-empty summary; the `is_summary` chunk exists for that document; exactly one summary chunk (no duplicate). + - **Test 2 `test_unchanged_document_viewer_shows_full_stored_text`:** re-sync without touching the files → open the document view and assert the rendered text equals the stored `content` (the sentinel phrase at the START and the END of the file both present, no truncation, no empty body) — the deferral must not have made the viewer's source row thinner. + - **Test 3 `test_changed_file_reindexes_under_the_deferral`:** rewrite one file on disk (new sentinel + changed length), re-sync → the run reports `updated=1`, the viewer shows the NEW text and no stale fragment, the chunk count changes, and a third sync reports all `unchanged` (the fast path still fires after a deferral-mediated write). + - Module docstring: the incident reference, the deferral (`defer(Document.content)`, one column, no `raiseload` — A1/A2), the test→observable mapping, the run-in-isolation command, prerequisites (`podman compose up -d db`). +2. **Docs pass:** `README.md` + `AGENTS.md` reviewed in full against the post-phase app; every inconsistency fixed, pre-existing included. `README.md` gains the one-sentence operations note (a full re-sync walk is metadata-bound; the summary-NULL backfill intentionally re-reads the stored content once). +3. **The full gates:** + - `uv run pytest tests/e2e/test_fast_path_deferred_content.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_document_dates.py -v --no-cov` and `uv run pytest tests/e2e/test_sync_changed_only.py -v --no-cov` — green in isolation (the fast path's UI neighbours). + - `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. +4. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit — e.g. `perf(import): defer Document.content on the unchanged fast path so a full walk no longer hydrates the KB` — covering `app/rag/importer.py`, the integration + E2E files, the docs and `.agents/`. Move the phase dir to `.agents/phases/complete/185_sync_defer_content_fast_path/`. + +## Testing & Quality +- E2E: the dedicated suite + the two regression E2E families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_fast_path_deferred_content.py -v --no-cov` green **in isolation** (backfill, full text, changed-file re-index). +- [ ] `tests/e2e/test_document_dates.py` and `tests/e2e/test_sync_changed_only.py` green in isolation. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project. +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/phases/todo/186_git_log_streaming_dates/00_phase.md b/.agents/phases/todo/186_git_log_streaming_dates/00_phase.md new file mode 100644 index 0000000..ee3401d --- /dev/null +++ b/.agents/phases/todo/186_git_log_streaming_dates/00_phase.md @@ -0,0 +1,68 @@ +# Phase 186 — The commit-date walk streams instead of buffering the whole `git log` + +**Source:** owner incident 2026-10-15 (as phases 183–185) — `_run_sync` builds `doc_dates_by_root` from a full-history `git log --name-only` captured ENTIRELY in memory, per source, and keeps the result for the whole run. +**Story:** n/a (incident-derived, owner directive 2026-10-15) +**Depends on:** none functionally; sequenced after 183–185 so the sync's memory work lands together. + +**Context:** `scripts/git_sync.py` — the module docstring's **A11 "every git command goes through `run_git`" claim at L31**, `clone_or_pull` (L96), `_unshallow_if_shallow` (L221), **`_parse_commit_dates(output: str)` (L240)** — `for line in output.splitlines()` at L245-ish, the first-sighting-wins `dates.setdefault(path, commit)`, the `ValueError` paths; **`file_commit_dates` (L270)** — `output = run_git(["git", "log", "--name-only", "--format=@@%cI"], cwd=dest)` (L298), the two fail-soft `except` arms returning `{}` with the `file_commit_dates: …` warnings, `run_git` (**L320**, `subprocess.run(argv, cwd=cwd, capture_output=True, text=True, env=env)` at **L342**, `GitSyncError` with `proc.stderr` on non-zero); `app/api/sync.py` — `doc_dates_by_root[str(root)] = await asyncio.to_thread(file_commit_dates, root)` (**L646**, phase 179), the dict's consumer `dates_map = (doc_dates_by_root or {}).get(str(root), {})` in `app/rag/importer.py` (**L913**), `raw_date=dates_map.get(rel)` passed to `_index_file` (**L945–948**), the mtime fallback `file_mtime_datetime` (`app/rag/doc_dates.py` L57) and `normalize_doc_date`; `app/rag/doc_dates.py` (pure, unchanged); `tests/unit/test_git_sync.py` (the `_parse_commit_dates` + argv pins, which call it with **strings**), `tests/integration/test_git_file_dates.py`, `tests/integration/test_importer_dates.py`, `tests/e2e/test_git_source_dates.py`, `tests/e2e/test_document_dates.py`. + +## Measured baseline (2026-10-15, the incident box's own checkouts, read-only `git` inside the app container) +| checkout | commits | `git log --name-only --format=@@%cI` lines | bytes | +|---|---|---|---| +| litellm | 54,502 | 346,829 | 15,077,351 | +| llama.cpp | 11,659 | 69,922 | 1,867,046 | +| vllm-radiance | 192 | 4,855 | 387,672 | +| gufo | 589 | 9,155 | 327,032 | + +`run_git` returns that as ONE `str`; `_parse_commit_dates` then calls `.splitlines()`, materialising a second copy as a list of 346,829 `str` objects — tens of MB of transient peak per source on a 3,904 MB box, before the retained dict. The source removed that morning (`server`, nextcloud) is the case that shows how unbounded this is: the walk scales with the repository's ENTIRE history, not with its working tree. + +## Objective +Stream the walk: parse the `git log` output line by line from the subprocess pipe so the run never holds the whole output, keep the phase-106 D2 contract byte-for-byte (first-sighting-wins last-commit dates, the same fail-soft arms, the same warning lines, mtime fallback), and amend the A11 single-site claim so the streaming variant is a documented, tested sibling rather than an exception hiding in the module. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — `stream_git_lines(argv, cwd)` is the second git site, and A11 says so (task 02).** A generator in `scripts/git_sync.py`: + ```python + def stream_git_lines(argv: list[str], cwd: Path) -> Iterator[str]: + """Stream one git command's stdout, line by line (phase 186, A11 sibling).""" + ``` + `subprocess.Popen(argv, cwd=cwd, stdout=PIPE, stderr=PIPE, text=True)`; stdout is iterated directly (`for line in proc.stdout`); **stderr is drained by a daemon thread into a list** so a chatty stderr can never deadlock the pipe while stdout is being consumed (the classic `Popen` + two-pipes trap — this is the reason not to `communicate()` at the end and not to `stderr=STDOUT`, which would pollute the parse). On `proc.wait() != 0` it raises `GitSyncError(f"git {' '.join(argv[1:])} failed (exit …): {stderr}")` — the SAME message shape as `run_git` (L344–347), so every caller's fail-soft handling is unchanged. The module docstring's L31 claim becomes: `run_git` is the buffered site, `stream_git_lines` the streaming one, and **both** are the only places a `subprocess` is spawned (a unit pin greps `subprocess.` occurrences in the file — the phase-179 AGENTS rule 11 "one git-invocation site" intent survives as "one for each shape, both owned here"). +- **D2 — the parser takes any iterable; strings keep working (task 01 first).** `_parse_commit_dates(lines: str | Iterable[str]) -> dict[str, datetime]`: `if isinstance(lines, str): lines = lines.splitlines()` and everything else — the `@@%cI` commit-header rule, the whitespace path split, the `\`→`/` normalisation, the leading-`/` strip, `setdefault` first-sighting-wins, both `ValueError` arms — stays byte-identical, so `tests/unit/test_git_sync.py`'s existing string pins need no edits. +- **D3 — `file_commit_dates` consumes the stream, fail-soft unchanged (task 02).** `return _parse_commit_dates(stream_git_lines([...], cwd=dest))` inside the existing `try`, the existing `except GitSyncError` → the same warning + `{}`, the existing `except ValueError` → the same warning + `{}`. Two consequences documented in the docstring, not hidden: a mid-stream failure now surfaces AFTER some lines were parsed, and the method still returns `{}` because the caller falls back to mtimes (the phase-106 D2 fail-soft contract is about the RESULT, and the result is unchanged); and the returned dict — one entry per path in the repository's history — remains retained by `_run_sync` in `doc_dates_by_root` for the run (A3: out of scope; the dict is ~10% of the buffered cost this phase removes, and bounding it changes dates, not just memory). +- **D4 — the pins are comparative and deterministic (tasks 01 + 02).** Unit: a one-shot generator input proves single-pass consumption (a generator that raises on a second `iter()`), and the lazy-vs-buffered assertion measures `tracemalloc` peak for the SAME synthetic corpus (≈300k lines) through (a) one big `str` + `splitlines()` and (b) a generator, asserting `streamed < buffered * 0.6` with both numbers printed in the failure message. Integration: `tests/integration/test_git_file_dates.py` extended with a generated two-commit repo proving the post-fix dates equal the pre-fix dates for (i) a file only in commit 1, (ii) a file modified in commit 2 (its date is commit 2's), (iii) a path in a subdirectory, (iv) a file absent from history entirely (mtime fallback). +- **D5 — the E2E pins the owner-visible date (task 03).** `tests/e2e/test_git_log_stream_dates.py`: a local `file://` fixture repo with two dated commits (the `test_git_source_dates.py` `_git` helper shape, `GIT_AUTHOR_DATE`/`GIT_COMMITTER_DATE` pinned so the dates are exact), synced from the Sources page; the document date shown for the modified file equals commit 2's date and the untouched file shows commit 1's date (the phase-106 contract, now through the stream), a file git never committed falls back to its mtime, and the D1 manual-date lock still survives a re-sync. Whatever the walk currently does with a quoted non-ASCII path is recorded in the module docstring as-is and pinned as UNCHANGED behaviour (this phase does not change the parser's quoting semantics — it only changes how the bytes arrive). +- **NOT touched:** the `git log` ARGV (`--name-only --format=@@%cI` stays, pinned by the existing argv unit tests), `clone_or_pull` / `_unshallow_if_shallow` (they stay on `run_git`), `app/rag/doc_dates.py` (pure), the importer's `dates_map.get(rel)` lookup, the mtime fallback, the sync's cancel semantics (phase 134/179), the frontend. + +## Dependencies +— (none functionally; run after `183`/`184`/`185`) + +## Tasks +1. `01_parser_accepts_lines.md` — `_parse_commit_dates` takes `str | Iterable[str]` with byte-identical semantics, the one-shot-generator + comparative-memory unit pins, the existing string pins untouched. +2. `02_stream_git_log.md` — `stream_git_lines` (Popen + stderr thread + the `run_git`-shaped `GitSyncError`), `file_commit_dates` on the stream, the A11 docstring amendment, the grep pin, the integration date-parity tests. +3. `03_e2e_streamed_dates.md` — `tests/e2e/test_git_log_stream_dates.py` (the phase's dedicated Playwright suite), the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Unit:** `tests/unit/test_git_sync.py` — existing string pins green unedited; the new iterable/one-shot/lazy-memory pins; the `subprocess.` occurrence grep pin. +- **Integration:** `tests/integration/test_git_file_dates.py` (date parity on a generated repo, the fail-soft arms still returning `{}` with the same warnings), `tests/integration/test_importer_dates.py`, `test_import_docs_git.py`, `test_sync_api.py` (the `file_commit_dates` monkeypatch points keep working — `app.api.sync` imports the name, phase 179's probe still patches it). +- **E2E (AGENTS rule 4, task 03):** `tests/e2e/test_git_log_stream_dates.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` plus full coverage of `scripts/git_sync.py`'s new function (the existing `tests/unit/test_git_sync.py` family covers this module today — keep it covered, not just `app/`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `AGENTS.md` rule 11 (the "never block the event loop" rule already cites `run_git`): one clause noting the streaming sibling exists and that BOTH are off-loop via `asyncio.to_thread` (phase 179) — do not rewrite the rule, extend the reference. `README.md`: nothing user-facing changes; add the ops note that the commit-date walk streams (so a huge repo no longer needs the whole `git log` in RAM) and that the per-source date dict is still held for the run (A3). + +## Completion Criteria +- [ ] `_parse_commit_dates` accepts `str | Iterable[str]` with byte-identical parse semantics; the existing string-based unit pins are green with no edits. +- [ ] `stream_git_lines` exists, drains stderr on a thread, raises the `run_git`-shaped `GitSyncError`; `file_commit_dates` streams; the module docstring's A11 claim names both sites and the grep pin enforces it. +- [ ] The lazy-vs-buffered memory comparison and the one-shot generator pins are green with their measured numbers in the test docstrings. +- [ ] `uv run pytest tests/integration/test_git_file_dates.py tests/unit/test_git_sync.py -v` green (date parity across all four cases, both fail-soft arms unchanged). +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. +- [ ] `uv run pytest tests/e2e/test_git_log_stream_dates.py -v --no-cov` green **in isolation**. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (verified in the final pass). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `perf(sync): stream the git commit-date walk instead of buffering git log in RAM`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — stream, do not shrink the argv.** No `--since`, no `--max-count`, no per-file `git log -1`: every one of those changes WHICH dates the KB stores, and the phase-106 D2 last-commit-date contract is a data contract, not a performance knob. +- **A2 — stderr on a worker thread.** The deadlock-free way to stream stdout while still reporting git's stderr; `stderr=STDOUT` would corrupt the parse, `stderr=DEVNULL` would lose the error text `GitSyncError` carries today. +- **A3 — the retained `doc_dates_by_root` dict is out of scope.** Bounding it (by age, by working-tree membership, or by streaming it to a temp file) changes document dates or adds a new subsystem; this phase removes the transient whole-`git log` copy, which is the unbounded piece. +- **A4 — `run_git` keeps its shape.** Every other caller (`clone_or_pull`, `_unshallow_if_shallow`, `app/core/docs_push.py`) keeps the buffered site; migrating them is churn without a measured cost. diff --git a/.agents/phases/todo/186_git_log_streaming_dates/01_parser_accepts_lines.md b/.agents/phases/todo/186_git_log_streaming_dates/01_parser_accepts_lines.md new file mode 100644 index 0000000..5288f85 --- /dev/null +++ b/.agents/phases/todo/186_git_log_streaming_dates/01_parser_accepts_lines.md @@ -0,0 +1,26 @@ +# Task 01 — The parser takes a stream (semantics untouched) + +**Phase:** `186_git_log_streaming_dates` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Widen `_parse_commit_dates` to accept any iterable of lines (keeping `str` working so every existing pin is untouched), and pin in tests that it consumes its input exactly once and that streaming allocates materially less than the buffered path for the same corpus. + +## Work +1. `scripts/git_sync.py` — `_parse_commit_dates` (**L240**): signature `def _parse_commit_dates(lines: str | Iterable[str]) -> dict[str, datetime]:` with the single line `if isinstance(lines, str): lines = lines.splitlines()` at the top; rename the loop variable to match; `from collections.abc import Iterable` added. Every parsing rule stays byte-identical: the `@@`-prefixed `%cI` header (`datetime.fromisoformat(line[2:])`), the blank-line skip, `line.split()` for multiple paths per line, `raw_path.replace("\\", "/").lstrip("/")`, `dates.setdefault(path, commit)` (newest-first, first sighting wins), and BOTH `ValueError` arms (a malformed date, a path line before any commit header). The docstring gains: "*lines* may be a string or any iterable of lines (phase 186 — `stream_git_lines` feeds it a live pipe); a string input behaves exactly as before." +2. `tests/unit/test_git_sync.py` — extend (existing string-based tests untouched): + - `test_parser_accepts_a_line_iterable`: the same corpus the existing tests use, fed as `iter([...])`, produces the identical dict. + - `test_parser_consumes_exactly_once`: a one-shot generator wrapper that raises `AssertionError("re-iterated")` if `iter()` is called twice on it; parse succeeds → proves the parser does not materialise-then-rescan (and rules out an accidental `list(lines)` inside the loop). + - `test_streamed_parse_alloculates_less_than_buffered`: build ONE synthetic corpus of ~300,000 lines in the exact real shape (3,000 commits × 100 paths, `@@2026-01-01T00:00:00+00:00` headers), then measure `tracemalloc` peak over (a) `_parse_commit_dates(one_big_str)` (the buffered path's own `splitlines()`) and (b) `_parse_commit_dates(iter_of_lines_from_a_generator)` (each line built lazily from a fixed seed), asserting `streamed_peak < buffered_peak * 0.6` with both numbers in the assertion message. Record both numbers + the machine in the docstring; if the real ratio on this box differs, keep the 0.6 factor only if the pre/post split genuinely clears it and document the measured ratio (never weaken to `<= buffered_peak`, which is a tautology). + - Corpus justification in the module docstring (not a test): the incident box's measured sizes — litellm 54,502 commits / 346,829 lines / 15 MB; llama.cpp 69,922 lines; vllm-radiance 4,855; gufo 9,155 — which is why the synthetic corpus is ~300k lines. +3. Verify: `uv run pytest tests/unit/test_git_sync.py -v`; `uv run pytest` green. + +## Testing & Quality +- Unit: the three new pins + every existing pin green unedited. +- Coverage: **>90%** on `app/` and `scripts/git_sync.py`'s parser stays fully covered. + +## Completion Criteria +- [ ] `_parse_commit_dates` accepts `str | Iterable[str]`; no parsing rule changed; the docstring says so. +- [ ] The one-shot-generator pin is green (no re-iteration, no hidden `list()`). +- [ ] The streamed-vs-buffered memory comparison is green with both measured peaks recorded in the docstring. +- [ ] `uv run pytest tests/unit/test_git_sync.py -v` and `uv run pytest` green. diff --git a/.agents/phases/todo/186_git_log_streaming_dates/02_stream_git_log.md b/.agents/phases/todo/186_git_log_streaming_dates/02_stream_git_log.md new file mode 100644 index 0000000..e05d391 --- /dev/null +++ b/.agents/phases/todo/186_git_log_streaming_dates/02_stream_git_log.md @@ -0,0 +1,37 @@ +# Task 02 — The fix: `stream_git_lines` + `file_commit_dates` off the buffer + +**Phase:** `186_git_log_streaming_dates` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Add the streaming git site and put the commit-date walk on it, so no `git log` output is ever held as one string — with `run_git`'s error contract, the phase-106 D2 fail-soft result, and the A11 single-site rule kept intact (as two documented siblings). + +## Work +1. `scripts/git_sync.py` — new function next to `run_git` (L320): + ```python + def stream_git_lines(argv: list[str], cwd: Path) -> Iterator[str]: + ``` + - `proc = subprocess.Popen(argv, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, env=None)` (the same env semantics as `run_git`: `None` inherits — do NOT introduce the env-replacement trap the module docstring warns about). + - stderr drained by a daemon thread appending to a list, started before stdout is consumed (A2: the deadlock-free shape — never `communicate()` after iterating stdout, never `stderr=STDOUT`, never `stderr=DEVNULL`). + - `for line in proc.stdout or (): yield line` (guard the `text=True` optional type for pyright), then `proc.stdout.close()`, `err_thread.join()`, `rc = proc.wait()`; `if rc != 0: raise GitSyncError(f"git {' '.join(argv[1:])} failed (exit {rc}): " + "".join(stderr_lines).strip())` — the message shape copied from `run_git` L344–347 so every caller's fail-soft text is unchanged. + - `finally`-safe: if the generator is closed early (`GeneratorExit`), terminate the process (`proc.terminate()` guarded by try/except) so a partial consume never leaks a `git` child. + - Module docstring: amend the **L31** claim → `run_git` (buffered) and `stream_git_lines` (streaming, phase 186) are the ONLY two places a git subprocess is spawned; both own the argv→`GitSyncError` contract; both are reached from async code only through `asyncio.to_thread` (phase 179 / AGENTS rule 11). +2. `file_commit_dates` (**L270**): `output = run_git([...])` (L298) → `return _parse_commit_dates(stream_git_lines(["git", "log", "--name-only", "--format=@@%cI"], cwd=dest))` inside the existing `try`, keeping BOTH `except` arms and their exact warning strings. Add the docstring notes: the walk streams (no whole-`git log` in RAM — the incident's transient peak); a mid-stream failure still yields `{}` + the mtime fallback (the D2 contract is about the RESULT); and the returned per-source dict stays retained by `_run_sync` for the whole run — **A3, out of scope, named**. +3. `tests/unit/test_git_sync.py` — add: + - `test_stream_git_lines_yields_lines_and_raises_run_git_shaped_error` — run it against a real throwaway git repo (the house `_git` helper): success path yields the expected lines; `git log` in a non-repo dir raises `GitSyncError` whose message matches `^git log failed \(exit \d+\): ` (the `run_git` shape). + - `test_stream_git_lines_reports_stderr_without_deadlocking` — a command that writes >100 KB to stderr (e.g. `git -c core.quotepath=bogus …` is contrived; use `git log` in a repo with a corrupt arg that spews usage, or `git` with `--help` on stderr via a bad flag) and assert the call raises with a non-empty stderr excerpt and completes in a bounded time (a wall-clock guard, e.g. `< 10 s`). + - `test_only_two_git_spawn_sites` — read `scripts/git_sync.py` and assert `subprocess.` occurrences are exactly inside `run_git` and `stream_git_lines` (the A11 grep pin), and that no other module in `app/` or `scripts/` spawns `subprocess` for git (`grep`-style assertion over the repo; if the docs-push path legitimately uses `run_git`, it passes by construction). + - Keep every existing `run_git`/argv pin green (the `git log` argv is unchanged — A1). +4. `tests/integration/test_git_file_dates.py` — extend with the date-parity fixture (a generated repo, `GIT_AUTHOR_DATE`/`GIT_COMMITTER_DATE` pinned per commit): file only in commit 1 → commit 1's date; file modified in commit 2 → commit 2's date; a file in a subdirectory → its own last-commit date; a file that exists but was never committed → `{}` entry, so the importer's mtime fallback applies (assert the empty lookup, not the resulting date, to keep this test about `file_commit_dates`). Also `test_file_commit_dates_fail_soft_unchanged`: a non-directory and a non-repo directory both return `{}` with the existing warning lines (wording pinned). +5. Verify: `uv run pytest tests/unit/test_git_sync.py tests/integration/test_git_file_dates.py tests/integration/test_import_docs_git.py tests/integration/test_sync_api.py -v`; then `uv run pytest`. + +## Testing & Quality +- Unit: the streaming site (success, error shape, stderr safety, spawn-site grep). +- Integration: date parity + both fail-soft arms + the phase-179 `app.api.sync.file_commit_dates` monkeypatch point still exercised by `test_sync_git_freeze.py`. +- Coverage: **>90%** on `app/`; `scripts/git_sync.py` fully covered by the unit family. + +## Completion Criteria +- [ ] `stream_git_lines` exists with the stderr-thread + terminate-on-close shape and the `run_git`-shaped `GitSyncError`; the module docstring names both spawn sites and the grep pin enforces it. +- [ ] `file_commit_dates` streams, returns byte-identical dicts for the parity fixture, and keeps both fail-soft arms and warnings. +- [ ] `uv run pytest tests/unit/test_git_sync.py tests/integration/test_git_file_dates.py -v` green. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/186_git_log_streaming_dates/03_e2e_streamed_dates.md b/.agents/phases/todo/186_git_log_streaming_dates/03_e2e_streamed_dates.md new file mode 100644 index 0000000..ba1c342 --- /dev/null +++ b/.agents/phases/todo/186_git_log_streaming_dates/03_e2e_streamed_dates.md @@ -0,0 +1,34 @@ +# Task 03 — E2E: the dates the owner reads come through the stream (the phase's dedicated Playwright suite) + +**Phase:** `186_git_log_streaming_dates` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Pin the phase-106 D2 promise through the browser with the streaming walk: after syncing a real two-commit `file://` repo, the dates shown for each document are that file's LAST-COMMIT dates (not the sync date, not the clone date), uncommitted files fall back to mtime, and a manual date correction survives a re-sync. Then the full gates, the docs pass, the atomic commit. + +## Work +1. `tests/e2e/test_git_log_stream_dates.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped), the `tests/e2e/test_git_source_dates.py` + `test_document_dates.py` shapes (dedicated module app + port, mock LLM, dedicated `BOR_SOURCES_DIR`, `BOR_GIT_SOURCES=file://`, the shared E2E Postgres truncated before AND after each test, admin login via `tests/e2e/auth_helpers.py`, the `_git` helper with `GIT_AUTHOR_DATE`/`GIT_COMMITTER_DATE` set per commit so dates are exact and stable). + - Fixture repo: commit A (dated 2026-01-05) adds `notes/old.md` + `notes/changed.md`; commit B (dated 2026-03-20) modifies `notes/changed.md` only; plus an untracked `notes/untracked.md` written directly into the CHECKOUT after cloning (or, if the sync re-clones, into the source dir the run walks — whichever the house fixture model uses; state which in the docstring). + - **Test 1 `test_document_dates_are_the_last_commit_dates`:** sync from the Sources page → `notes/old.md`'s document date is 2026-01-05 and `notes/changed.md`'s is 2026-03-20 (assert via `GET /api/docs/{id}` through `page.request` and the document page's date surface), neither equal to the run's own timestamp. + - **Test 2 `test_uncommitted_file_falls_back_to_mtime`:** the untracked file's document date equals its file mtime (the fail-soft contract through the streaming path) and the run reaches `success` with no error banner. + - **Test 3 `test_manual_date_survives_a_resync` (the D1 lock):** set a document's date through the existing date-edit surface, re-sync, and the manual date is unchanged while the other documents refresh from the stream. + - **Test 4 `test_resync_is_unchanged_and_dates_hold`:** a second sync with nothing changed reports all `unchanged` and identical dates (the stream re-parses, it does not drift). + - Non-ASCII/quoted path behaviour: record what the walk does TODAY with a `core.quotepath`-quoted path in the module docstring and pin it as UNCHANGED behaviour (whatever the run shows — this phase changes transport, not parsing; if the behaviour is wrong, note it for a future phase rather than fixing it here). +2. **Docs pass:** `README.md` + `AGENTS.md` in full, every inconsistency fixed (pre-existing included). `AGENTS.md` rule 11 gains the one-clause reference to `stream_git_lines` (both sites off-loop via `asyncio.to_thread`); `README.md` gains the ops note (the commit-date walk streams; the per-source date dict is still held for the run — phase 186 A3). +3. **The full gates:** + - `uv run pytest tests/e2e/test_git_log_stream_dates.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_git_source_dates.py -v --no-cov` and `uv run pytest tests/e2e/test_document_dates.py -v --no-cov` — green in isolation (the date families this phase must not disturb). + - `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. +4. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit — e.g. `perf(sync): stream the git commit-date walk instead of buffering git log in RAM` — covering `scripts/git_sync.py`, the unit + integration + E2E files, the docs and `.agents/`. Move the phase dir to `.agents/phases/complete/186_git_log_streaming_dates/`. + +## Testing & Quality +- E2E: the dedicated suite + the two date E2E families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_git_log_stream_dates.py -v --no-cov` green **in isolation** (last-commit dates, mtime fallback, the D1 manual lock, the unchanged re-sync). +- [ ] `tests/e2e/test_git_source_dates.py` and `tests/e2e/test_document_dates.py` green in isolation. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (the rule-11 clause included). +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/phases/todo/187_sync_peak_memory_line/00_phase.md b/.agents/phases/todo/187_sync_peak_memory_line/00_phase.md new file mode 100644 index 0000000..14bea35 --- /dev/null +++ b/.agents/phases/todo/187_sync_peak_memory_line/00_phase.md @@ -0,0 +1,54 @@ +# Phase 187 — Make the sync's memory a logged, visible number + +**Source:** owner incident 2026-10-15 — reconstructing "how much RAM did the sync actually use?" meant reading `systemd` unit-teardown lines from three boots (`brain-of-code-app.service: … 3.3G memory peak`), and boot −1 (the run that killed the box) had NO teardown record at all because the VM never shut down cleanly. +**Story:** n/a (incident-derived, owner directive 2026-10-15) +**Depends on:** none functionally; lands after 183–186 so the numbers it reports describe the fixed sync. + +**Context:** `.agents/PLAN.md` §9 Observability (the log-format authority — PLAN is never edited by an agent, so the new fields are documented in `README.md` + `AGENTS.md` rule 10 and the owner reconciles PLAN); `AGENTS.md` rule 10 ("Amply log, visibly feed back"); `app/core/response_budget.py` (the house precedent this phase mirrors: a pure-stdlib `app/core/` module, one greppable logger name — `grep ' :: api '`, `BOR_LOG_LARGE_RESPONSE_BYTES`, and `scripts/api_payload_audit.py` as the offline audit); `app/api/sync.py` — the `SyncStatus` dataclass (**L276**, phase-64 "keep the final counts in the terminal" convention), `sync_status()` (**L385**, its additive-keys contract "Additive only — every pre-existing key keeps its name, type, and meaning"), the `sync: started repos=%d origin=%s git=%d local=%d` line (**L591–594**), the phase transitions (`_status.phase = "import"` **L718**, `"overview"` **L743**, `"summaries"`), `sync: done detail=%s` (**L903**), the cancel terminal's `sync: cancelled by user state=cancelled files_done=%d files_total=%d` (**L933–936**), the failed terminal (`logger.exception("sync: failed")` **L938**); `app/rag/importer.py` — `ImportSummary.format_counts` (**L513**) + `ImportSummary.log()` (**L520**, the cron-safe greppable summary line PLAN §9 names); `frontend/assets/sources.js` — the sync progress box + result line (`setSyncProgress(…)` around **L1325**, the label/counts helper at **L1080–1100** and **L1378**, the aria-live announcements), `frontend/index.html` (the progress box markup), `tests/unit/test_sync_api*.py` / `tests/integration/test_sync_api.py` (the status-shape pins), `tests/e2e/test_sync_summary_visibility.py`, `tests/e2e/test_sync_progress_box.py`, `tests/e2e/test_sync_status_freshness.py`, `tests/e2e/test_sync_cancel.py`. + +## Objective +Make "how much memory did that sync use?" answerable from one log line and visible on the page that started it: a small `app/core/memory.py` reading current + peak RSS, appended to every sync lifecycle line and the importer's summary line, plus an additive `rss_peak_mb` key on `GET /api/sync/status` and a `peak MB` clause on the Sources page's sync result line. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — `app/core/memory.py` never raises and never lies.** Three pure helpers: `rss_mb() -> int | None` (Linux: parse `VmRSS` from `/proc/self/status`; non-Linux/failure: `resource.getrusage(RUSAGE_SELF).ru_maxrss // 1024` as the best available proxy), `rss_peak_mb() -> int | None` (`ru_maxrss // 1024` — the process high-water mark), and `cgroup_peak_mb() -> int | None` (read `/sys/fs/cgroup/memory.peak` when present, else `None`). Docstring states the honest nuance the incident turned on: **inside a container the process high-water and the host's cgroup `memory.peak` are different numbers, and the cgroup number includes page cache** (the incident box's app cgroup showed 2.16 GB of `file` against 211 MB of `anon` while idle) — so the app logs the PROCESS number and the operator's dashboard may show a larger one; neither is wrong, and the log says which it is. +- **D2 — the format is append-only, in the §9 style (task 02).** `sync: started … rss_mb=N`, and every terminal (`sync: done detail=… rss_mb=N rss_peak_mb=M`, `sync: cancelled by user … rss_mb=N rss_peak_mb=M`, and the failed funnel) gains the same two trailing fields; `ImportSummary.log()` gains a trailing ` rss_mb=N`. A `None` renders as `rss_mb=?` — a helper `fmt_mb(v: int | None) -> str` keeps that in one place. No existing field is renamed, reordered or re-typed (the phase-167/182 append-only convention), so every existing `grep` keeps working. +- **D3 — sampled, not polled per file (task 03).** `_status` gains `rss_mb: int | None` and `rss_peak_mb: int | None`, reset at run start; sampled at the run start, at each `_status.phase` transition (`import` L718, `overview` L743, `summaries`), and in EVERY terminal (which keep the final value next to the counts — the phase-64 convention). The per-file progress hook samples at most once per 5 s via a `time.monotonic()` guard, so a 27k-file walk costs ~600 `/proc` reads, not 27k, and the operator watching the page sees the number move (PLAN §7.4's never-stale contract). +- **D4 — the status key and the UI clause are additive and admin-only (task 03).** `sync_status()` returns two new keys (`rss_mb`, `rss_peak_mb`, `int | null`); `sources.js` appends ` · peak 512 MB` to the existing result/counts line — a clause on text that already exists, never a new control, never replacing a state string, and the aria-live announcement keeps its current wording (a screen-reader user gets the same announcement plus the clause; the peak is supplementary, so it must not become the announcement's only content). The Sources view is admin-only (PLAN B5), which is the exposure guard; no memory number appears on any anonymous surface (chat, shared chat, health). +- **D5 — an offline reader for the log (task 02, small).** Document `grep ' :: ' | grep ' rss_peak_mb='` in `README.md`'s operations section. NO new audit script: `scripts/api_payload_audit.py` exists for bytes; a memory equivalent would be a second parser for one number. Explicitly out of scope (A3). +- **NOT touched:** `PLAN.md` (never), the sync pipeline's sequence/errors/cancel semantics, the response-budget logger, the chat's per-turn line (this phase is about the sync), the container/quadlet memory limits (owner's own task), the retrieval path. + +## Dependencies +— (none; after 183–186 in the roadmap so the reported numbers describe the bounded sync) + +## Tasks +1. `01_memory_helper.md` — `app/core/memory.py` (the three helpers + `fmt_mb`), unit-tested with `/proc` and `resource` stubbed (Linux path, missing-file path, garbage-file path, non-Linux fallback). +2. `02_sync_log_lines.md` — the four sync lifecycle lines + `ImportSummary.log()` gain `rss_mb=` / `rss_peak_mb=`, the `fmt_mb` single-sourcing, the log-format assertions in the integration tests, the README operations note. +3. `03_status_key_and_ui.md` — the `SyncStatus` fields + sampling (start, phase transitions, terminals, the 5 s-throttled progress sample), the two additive `sync_status()` keys, the `sources.js` clause + aria-live behaviour, the shape pins. +4. `04_e2e_peak_memory.md` — `tests/e2e/test_sync_peak_memory_visible.py` (the phase's dedicated Playwright suite), the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Unit:** `tests/unit/test_core_memory.py` — every helper branch (real `/proc` on Linux + stubbed failure modes, the `None` rendering, the kB→MB arithmetic), `tests/unit/test_settings*` untouched. +- **Integration:** `tests/integration/test_sync_api.py` — the lifecycle lines carry the fields (caplog assertions on the `sync: started` / `sync: done` / `sync: cancelled` / `sync: failed` shapes with `rss_mb=?` accepted on a platform without `/proc`), the status payload's two new keys with their types and the terminal-keeps-final-value convention, every pre-existing key byte-identical. +- **E2E (AGENTS rule 4, task 04):** `tests/e2e/test_sync_peak_memory_visible.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `AGENTS.md` rule 10: the required sync lines now carry `rss_mb=` / `rss_peak_mb=` (extend rule 10's parenthetical, do not rewrite it). `README.md`: the operations grep, what each number means, and the container-vs-cgroup caveat (page cache included in the host's number) written in one short paragraph. PLAN §9 is the owner's to reconcile — flag it in the phase report, do not edit it. + +## Completion Criteria +- [ ] `app/core/memory.py` exists with the four documented functions, every branch unit-tested, and never raises. +- [ ] All four sync lifecycle lines + `import: summary …` carry `rss_mb=` (and the terminals `rss_peak_mb=`), appended after the existing fields; no existing field changed. +- [ ] `GET /api/sync/status` exposes `rss_mb` / `rss_peak_mb` (`int | null`), keeps the final value in every terminal, and every pre-existing key is unchanged. +- [ ] The Sources page's sync result line shows the ` · peak MB` clause with no new control and no regression in the existing state strings. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. +- [ ] `uv run pytest tests/e2e/test_sync_peak_memory_visible.py -v --no-cov` green **in isolation**. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (the PLAN §9 reconciliation flagged, not performed). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `feat(observability): log and show the sync's RSS so memory is a number, not a forensic exercise`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — the app logs PROCESS RSS, not the cgroup number**, with the caveat documented (`anon` vs `file` differ by an order of magnitude and the cgroup figure includes page cache — demonstrated during the incident investigation). +- **A2 — append-only to the existing lines and keys.** No new log line type, no renamed field, no separate metrics endpoint (A12: no aux services), no Prometheus. +- **A3 — no memory audit script** this phase; the grep is documented instead. +- **A4 — sampling is throttled (5 s) and never per-file**, and the UI clause is text on a line that already exists on an admin-only view. diff --git a/.agents/phases/todo/187_sync_peak_memory_line/01_memory_helper.md b/.agents/phases/todo/187_sync_peak_memory_line/01_memory_helper.md new file mode 100644 index 0000000..d736b0c --- /dev/null +++ b/.agents/phases/todo/187_sync_peak_memory_line/01_memory_helper.md @@ -0,0 +1,26 @@ +# Task 01 — `app/core/memory.py` + +**Phase:** `187_sync_peak_memory_line` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +One pure-stdlib module that answers "how much RAM is this process using, and what was its high-water mark" without ever raising, mirroring `app/core/response_budget.py`'s shape (no FastAPI, no imports from `app.api`, logger only if needed). + +## Work +1. `app/core/memory.py`: + - `rss_mb() -> int | None` — parse `VmRSS` from `/proc/self/status` (kB → MB, integer, floor); any `OSError`/`KeyError`/parse failure returns `None`. Non-Linux fallback: `resource.getrusage(resource.RUSAGE_SELF).ru_maxrss // 1024` (documented as a high-water proxy, not a current value). + - `rss_peak_mb() -> int | None` — `resource.getrusage(RUSAGE_SELF).ru_maxrss // 1024`, `None` only if `resource` is unavailable (wrap in a guarded import for platforms without it). + - `cgroup_peak_mb() -> int | None` — read `/sys/fs/cgroup/memory.peak` (bytes → MB) when it exists and parses, else `None`. Docstring: inside a container this file is usually absent (the host reads it), and the host's number counts PAGE CACHE too — the incident box's app cgroup showed ~2.16 GB `file` against ~211 MB `anon` while idle, and `systemd` reported `3.3G memory peak` for the unit while the uvicorn process was a fraction of that. This module therefore reports PROCESS numbers and this distinction is written here once, so no future reader conflates them. + - `fmt_mb(v: int | None) -> str` — `"?"` for `None`, else `str(v)` (so log lines never print `rss_mb=None`). + - Module docstring: the incident (one paragraph), the three helpers, the never-raise contract, and the container/cgroup caveat above. +2. `tests/unit/test_core_memory.py` — every branch, with the filesystem and `resource` stubbed: a real read (Linux runner: `/proc/self/status` has `VmRSS`, so `rss_mb()` is a positive int), a missing file, an unparseable file, a file without a `VmRSS` line, the non-Linux `resource` fallback path (patch the `/proc` open to raise + patch `getrusage`), `rss_peak_mb()`'s arithmetic, `cgroup_peak_mb()` present/absent/garbage, `fmt_mb(None) == "?"` / `fmt_mb(0) == "0"` (0 is a value, not missing). +3. Verify: `uv run pytest tests/unit/test_core_memory.py -v`; `uv run ruff check . && uv run pyright` clean; `uv run pytest` green. + +## Testing & Quality +- Unit: 100% branch coverage of the new module (well above the >90% gate). +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `app/core/memory.py` with the four functions, the never-raise contract, and the container/cgroup caveat documented. +- [ ] `uv run pytest tests/unit/test_core_memory.py -v` green, every branch covered. +- [ ] No import cycle (`app.core.memory` imports stdlib only — prove it in the module docstring's "pure stdlib" line and by the absence of `app.` imports). diff --git a/.agents/phases/todo/187_sync_peak_memory_line/02_sync_log_lines.md b/.agents/phases/todo/187_sync_peak_memory_line/02_sync_log_lines.md new file mode 100644 index 0000000..74195d8 --- /dev/null +++ b/.agents/phases/todo/187_sync_peak_memory_line/02_sync_log_lines.md @@ -0,0 +1,28 @@ +# Task 02 — The lifecycle lines carry the numbers + +**Phase:** `187_sync_peak_memory_line` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Append `rss_mb=` to the sync's start line, `rss_mb=`/`rss_peak_mb=` to all three terminals, and `rss_mb=` to the importer's cron-safe summary line — append-only, so every existing grep and every existing field keeps working. + +## Work +1. `app/api/sync.py` — the four lifecycle log sites, using `app.core.memory.rss_mb/rss_peak_mb/fmt_mb` (imported at module level, matching the file's import style): + - **L591–594** `sync: started repos=%d origin=%s git=%d local=%d` → append ` rss_mb=%s` (`fmt_mb(rss_mb())`). + - **L903** `sync: done detail=%s` → append ` rss_mb=%s rss_peak_mb=%s`. + - **L933–936** `sync: cancelled by user state=cancelled files_done=%d files_total=%d` → append the same two fields. + - **L938** the failed funnel: `logger.exception("sync: failed")` stays as-is (the traceback), and the FAILED line gains a dedicated `logger.error("sync: failed state=failed files_done=%d files_total=%d rss_mb=%s rss_peak_mb=%s error=%s", …)` immediately after the `_status.error = _sanitize_error(str(e))` assignment (sanitized exactly like the status field — AGENTS/security: the raw exception string never reaches the log un-sanitized). Keep `logger.exception` so the traceback survives. +2. `app/rag/importer.py` — `ImportSummary.log()` (**L520**): append ` rss_mb=%s` to the existing `import: summary …` line (the `format_counts()` helper at L513 stays untouched — the memory field belongs to the LOG line, not the counts formatter, so the CLI's other consumers of `format_counts()` are byte-identical). +3. `tests/integration/test_sync_api.py` — caplog assertions (the house caplog pattern): after a run to `success`, the captured records include a `sync: started … rss_mb=` line and a `sync: done … rss_mb=… rss_peak_mb=…` line whose values are `?` or a positive integer; the same for the cancelled terminal (reuse the existing cancel test's fixture) and the failed terminal (reuse the existing failure test — `sync: model check failed` or a stubbed raise). Assert the PRE-existing prefixes are unchanged (`line.startswith("sync: started repos=")` etc.) — the append-only pin. +4. `tests/integration/test_importer_e2e.py` (or the importer family's log-pin home): the `import: summary …` line ends with ` rss_mb=` and its earlier fields are unchanged. +5. `README.md` — the operations grep (`grep ' :: ' … | grep ' rss_peak_mb='`), the field meanings, and the process-vs-cgroup caveat in one short paragraph (D1/A1). `AGENTS.md` rule 10: extend the parenthetical to mention the sync's new fields (do not rewrite the rule). +6. Verify: `uv run pytest tests/integration/test_sync_api.py tests/integration/test_importer_e2e.py -v`; `uv run pytest`; ruff + pyright. + +## Testing & Quality +- Integration: the four lifecycle terminals + the importer summary line pin the fields and the unchanged prefixes. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] All four sync lifecycle lines and `import: summary …` carry the new fields, appended after the existing ones; the failed terminal gains the sanitized one-line summary while `logger.exception` still writes the traceback. +- [ ] `uv run pytest tests/integration/test_sync_api.py -v` green (fields present, pre-existing prefixes byte-identical). +- [ ] `README.md` + `AGENTS.md` rule 10 updated; `uv run pytest` green, coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/187_sync_peak_memory_line/03_status_key_and_ui.md b/.agents/phases/todo/187_sync_peak_memory_line/03_status_key_and_ui.md new file mode 100644 index 0000000..081f73c --- /dev/null +++ b/.agents/phases/todo/187_sync_peak_memory_line/03_status_key_and_ui.md @@ -0,0 +1,30 @@ +# Task 03 — The status key and the Sources-page clause + +**Phase:** `187_sync_peak_memory_line` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Put the number where the operator is: two additive keys on `GET /api/sync/status` (sampled at the run start, each phase transition, every terminal, and at most every 5 s during the walk) and a ` · peak MB` clause on the Sources page's existing sync result line. + +## Work +1. `app/api/sync.py`: + - `SyncStatus` (**L276**): two new fields `rss_mb: int | None = None`, `rss_peak_mb: int | None = None`, documented as phase-187 additive state with the phase-64 "terminals keep the final value" convention and the run-start reset. + - A single private sampler on the module: `_sample_rss(force: bool = False) -> None` — reads `app.core.memory.rss_mb()` / `rss_peak_mb()`, stores both, keeps a `time.monotonic()` guard so a non-forced call is a no-op within 5 s (D3/A4); forced at the run start, each `_status.phase = "import"|"overview"|"summaries"` transition (**L718** and friends), and every terminal (success L903 path, cancelled L933 path, failed L938 path) so the final value is exact. + - `sync_status()` (**L385**): append `"rss_mb"` and `"rss_peak_mb"` to the dict (the file's own words: "Additive only — every pre-existing key keeps its name, type, and meaning") and document the two keys + the terminal convention in its docstring. + - The per-file `_hook` (L677) calls `_sample_rss()` (unforced → throttled). +2. `frontend/assets/sources.js`: on the terminal render path, append the clause to the EXISTING result/counts text — ` · peak 512 MB` — sourced from `status.rss_peak_mb ?? status.rss_mb`, and render NOTHING when both are null (never `peak ? MB`). Touch the result-line helper used at **L1080–1100** / **L1378** and the terminal/announce path around **L1314–1330**; the existing state strings ("Sync cancelled", the counts line, the error copy) keep their exact wording with the clause appended, and the aria-live announcement keeps announcing what it announces today plus the clause (no new announcement). No new DOM control, no layout shift on the 375px viewport (the clause wraps inside the existing line's `.cell-clip`-free text; verify visually in the E2E at mobile width). +3. Pins: + - `tests/integration/test_sync_api.py`: the payload carries both keys with `int | None`; after a run the terminal payload's `rss_peak_mb` is non-null on Linux and `>= rss_mb` when both are set; idle (never-run) reports `null`/`null`; every pre-existing key/value shape asserted unchanged (the additive pin); anonymous `GET /api/sync/status` still 403 (no new anonymous surface — A4). + - `tests/unit/test_sync_status_shape.py` (or this file's existing home for the status-shape matrix): the key set is exactly the old set ∪ {`rss_mb`, `rss_peak_mb`}. + - `frontend`: the clause appears in the terminal line and NOT in `running`; a null peak renders no clause. +4. Verify: `uv run pytest tests/integration/test_sync_api.py -v`; the three sync E2E neighbours in isolation (`test_sync_summary_visibility.py`, `test_sync_progress_box.py`, `test_sync_status_freshness.py`) still green (they assert the existing strings — the clause must not break them; if one asserts an exact full-line equality, update it to the clause-appended form and say so in the task note); `uv run pytest`; ruff + pyright. + +## Testing & Quality +- Integration: the shape + additive + admin-gating pins above. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `SyncStatus` + `sync_status()` carry the two additive keys; sampling happens at start / phase transitions / every terminal and is throttled to 5 s in the per-file hook. +- [ ] The Sources page shows ` · peak MB` on the terminal line, renders nothing when the value is null, and adds no new control. +- [ ] The three existing sync E2E families are green (any exact-string update documented). +- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/187_sync_peak_memory_line/04_e2e_peak_memory.md b/.agents/phases/todo/187_sync_peak_memory_line/04_e2e_peak_memory.md new file mode 100644 index 0000000..a42999a --- /dev/null +++ b/.agents/phases/todo/187_sync_peak_memory_line/04_e2e_peak_memory.md @@ -0,0 +1,34 @@ +# Task 04 — E2E: the operator sees the peak (the phase's dedicated Playwright suite) + +**Phase:** `187_sync_peak_memory_line` · **Source:** owner incident 2026-10-15 +**Story:** n/a + +## Objective +Prove through the browser that the number is there and honest: a real sync from the Sources page ends with a `peak MB` clause on the result line, the status payload's `rss_peak_mb` is a positive integer that survives the terminal, the running state shows no clause, and nothing anonymous can read it. Then the full gates, the docs pass, the atomic commit. + +## Work +1. `tests/e2e/test_sync_peak_memory_visible.py` — a NEW module, mock-only, the `tests/e2e/test_sync_progress_box.py` / `test_sync_summary_visibility.py` house shapes (dedicated module app + port, mock LLM, dedicated `BOR_SOURCES_DIR`, shared E2E Postgres truncated before AND after each test, admin login via `tests/e2e/auth_helpers.py`). + - **Test 1 `test_result_line_shows_the_peak`:** sync a small fixture → after the `success` terminal, the progress box's result line matches `/peak \d+ MB/`, the existing wording ("Sync complete"/the counts line) is still present, and the aria-live region contains the same clause. + - **Test 2 `test_status_payload_reports_rss`:** during `running` and after the terminal, `GET /api/sync/status` via `page.request` → `rss_peak_mb` is a positive int in the terminal, `>= rss_mb` when both are set, and pre-existing keys are unchanged (the additive pin end to end). During `running` the result line shows NO `peak` clause (the clause is terminal-only in the UI). + - **Test 3 `test_anonymous_cannot_read_memory`:** logged-out `GET /api/sync/status` → 403 (the A4 exposure guard, unchanged). + - **Test 4 `test_cancelled_terminal_keeps_the_peak`:** cancel a run (the `test_sync_cancel.py` shape) → the neutral "Sync cancelled" state still renders, with the peak clause present (terminals keep the value). + - **Mobile width:** assert the clause at 375 px does not overflow the progress box (the PLAN §7 container rule). + - Module docstring: the incident reference, the fields + the process-vs-cgroup caveat pointer to `README.md`, the test→observable mapping, the run-in-isolation command, prerequisites (`podman compose up -d db`). +2. **Docs pass:** `README.md` + `AGENTS.md` in full against the post-phase app, every inconsistency fixed (pre-existing included); confirm rule 10's extension and the operations grep landed in task 02; flag the PLAN §9 reconciliation in the phase report (never edit PLAN.md). +3. **The full gates:** + - `uv run pytest tests/e2e/test_sync_peak_memory_visible.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_sync_progress_box.py -v --no-cov`, `tests/e2e/test_sync_status_freshness.py -v --no-cov`, `tests/e2e/test_sync_cancel.py -v --no-cov` — green in isolation (the surfaces the clause touches). + - `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. +4. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit — e.g. `feat(observability): log and show the sync's RSS so memory is a number, not a forensic exercise` — covering `app/core/memory.py`, `app/api/sync.py`, `app/rag/importer.py`, `frontend/assets/sources.js`, the tests, the docs and `.agents/`. Move the phase dir to `.agents/phases/complete/187_sync_peak_memory_line/`. + +## Testing & Quality +- E2E: the dedicated suite + the three sync UI families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_sync_peak_memory_visible.py -v --no-cov` green **in isolation** (clause on both terminals, absent while running, payload types, anonymous 403, mobile width clean). +- [ ] The three regression E2E families green in isolation. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project; the PLAN §9 reconciliation flagged for the owner. +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/00_phase.md b/.agents/phases/todo/188_chunks_hnsw_index/00_phase.md new file mode 100644 index 0000000..10b3ed0 --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/00_phase.md @@ -0,0 +1,62 @@ +# Phase 188 — The vector search stops full-scanning 414,220 embeddings + +**Source:** found while investigating the 2026-10-15 memory incident — `pg_indexes` for `chunks` lists `chunks_pkey`, `ix_chunks_document_id`, `ix_chunks_tsv` (GIN) and **no index on `embedding`**, so every chat turn runs `ORDER BY embedding <=> :vec LIMIT 100` (A7's cosine tier) as a sequential scan of a 3 GB relation: per-turn CPU on the interactive path, plus 3 GB of page-cache churn on a 3,904 MB box that the sync also needs. +**Story:** n/a (incident-adjacent, owner directive 2026-10-15) +**Depends on:** none functionally; LAST of the six incident phases — it is the only one that touches the schema and the retrieval quality contract, so it lands after the sync's memory work is in. + +**Context:** `.agents/PLAN.md` §2 — **A3** (PostgreSQL 17 + pgvector, cosine `<=>`, one system), **A6** (dim **768**, fixed at table creation, a mismatch fails loudly), **A7** (hybrid: **cosine top-100** ∪ FTS top-30 → RRF k=60 → full text of the top-N=2 documents), **A8** (the honesty gate: deflect only when best cosine < `BOR_RELEVANCE_THRESHOLD` 0.62 AND (zero FTS hits OR best cosine < `BOR_LEXICAL_SUPPORT_FLOOR` 0.35)), **A13** (every migration ships a TESTED downgrade), **A16** (per-phase unit + integration + >90% + one dedicated E2E file), **A21** (LLM calls are free; the interactive path's latency is what matters); `app/models.py` — `Chunk.embedding: Mapped[list[float] | None] = mapped_column(Vector(EMBEDDING_DIM))` (**L241**); `app/rag/retriever.py` — **`_vector_candidates(db, question_embedding, limit)` (L431)** building `distance = Chunk.embedding.cosine_distance(question_embedding)` (**L440**) `.where(Chunk.embedding.is_not(None))` (**L444**) `.order_by(distance).limit(limit)` (**L446**), the fusion caller at **L690–711** (`vector = _vector_candidates(db, question_embedding, v_n)`), `_lexical_candidates` (L632); `app/config.py` (the `BOR_` `Settings` pattern, `relevance_threshold` / `lexical_support_floor` / `suggested_docs` as the retrieval-knob neighbours); `app/core/settings_catalog.py` — `GROUPS` (**L72**, the `"Retrieval & honesty"` group) + the `set(CATALOG) == set(Settings.model_fields)` completeness pin; `alembic/versions/` — head is **`0028`** (`uv run alembic heads`), so this phase adds **`0029`**; `db/Containerfile` — **`ARG PGVECTOR_VERSION=v0.8.0`** (HNSW + `halfvec` available; verified 0.8.0 on both the dev and the production instance); `scripts/eval_retrieval.py` (+ `tests/fixtures/retrieval_battery.txt`, the phase-119/162 regression battery — `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt`, `AIPI_KEY` required); `benchmarks/model_benchmarks.csv` (the recorded-measurement home); `tests/e2e/test_retrieval_quality.py`, `tests/e2e/test_search_recall_small_sources.py`, `tests/e2e/test_chat_rag.py`, `tests/unit/test_retriever.py`, `tests/unit/test_retriever_recency.py`. + +## Production numbers (read 2026-10-15, read-only) +414,220 chunks, **all 414,220 with a non-null `embedding`**, `chunks` relation ≈ **3,000 MB**, database 3,147 MB, `shared_buffers=128MB`, `work_mem=4MB`, host 3,904 MB with **no swap**. pgvector **0.8.0**. + +## Objective +Add an HNSW cosine index on `chunks.embedding` through a reversible migration built `CONCURRENTLY` (A3's one-system rule respected — no new service, no new dependency), give the retrieval tier an explicit `hnsw.ef_search` knob that can never be set below the A7 top-100 candidate count, and prove with a deterministic recall pin plus the live retrieval battery that the approximate index does not move the top hits, the `top_score`, or the honesty-gate verdicts. + +## Design (shared by all tasks — the executor reads this, not the chat) +- **D1 — the index is on the existing `vector(768)` column, `vector_cosine_ops` (task 01).** `CREATE INDEX CONCURRENTLY … USING hnsw (embedding vector_cosine_ops) WITH (m = 16, ef_construction = 64)` — the pgvector defaults' neighbourhood, which keeps the graph buildable on a 4 GB box. `halfvec(768)` (half the index size) was considered and REJECTED for this phase (A2): it needs `embedding::halfvec(768)` in `_vector_candidates` AND a quantisation-recall study, i.e. two quality risks in one phase. The index is smaller-with-halfvec; if the prod build does not fit, that is a follow-up phase, not a silent compromise here. +- **D2 — `CONCURRENTLY` needs autocommit, and an interrupted build leaves an INVALID index (task 01).** Migration `alembic/versions/0029_chunks_embedding_hnsw.py` runs its DDL inside `with op.get_context().autocommit_block():` and, before creating, drops any existing INVALID index of that name (`select indisvalid from pg_index join pg_class …` → `DROP INDEX CONCURRENTLY IF EXISTS ix_chunks_embedding_hnsw` when `indisvalid` is false), then re-checks `indisvalid` afterwards and raises if the build did not validate — because a `CONCURRENTLY` build that dies (a 3,904 MB box is exactly where it dies) leaves an index that costs insert time and buys nothing, silently. Upgrade and downgrade both run in the autocommit block; the downgrade is `DROP INDEX CONCURRENTLY IF EXISTS …` and is TESTED (A13) by `alembic downgrade -1` / `upgrade head` in the task's verification. +- **D3 — `ef_search` is explicit and clamped (task 02).** `retrieval_ef_search: int = 100` (`BOR_RETRIEVAL_EF_SEARCH`, `"Retrieval & honesty"` catalogue group): `_vector_candidates` issues `SET LOCAL hnsw.ef_search = :ef` (per-transaction — no leakage between requests, no global GUC change; A12 keeps state where it belongs) immediately before the SELECT, and a value below the A7 candidate count is CLAMPED UP to it with one WARNING line (`retrieval: ef_search=%d raised to %d (A7 candidate count)`) rather than silently degrading recall. Negative values refuse startup the way the phase-182 `show_text_max_*` knobs do. +- **D4 — recall is pinned twice: deterministic in CI, live against the battery (tasks 03 + 04).** + - **CI (deterministic, no LLM):** seed ~5,000 chunks with fixed-seed 768-dim vectors, compute the EXACT cosine top-100 in Python (`numpy`-free is fine — a pure-Python dot product over the seeded corpus, or reuse the existing helper if `tests/` already has one), then assert the index-backed query's top-100 set overlaps the exact set by **≥ 0.99** at `ef_search=100` and that `1 - distance` (`top_score`) matches the exact value to 6 decimals for the returned rows. Plus the plan pin: `EXPLAIN (FORMAT JSON)` for the real `_vector_candidates` statement names `ix_chunks_embedding_hnsw` (and does NOT show `Seq Scan on chunks`). + - **Live (A8's honesty contract):** the retrieval battery, index OFF vs ON, on the 1,000-document live-replica KB (the `restore-test-db` skill) with `AIPI_KEY` set — every question's `top_score`, top-5 document set and deflection verdict must be identical (a difference means RAISING `ef_search`, never accepting it). The before/after numbers are recorded in `benchmarks/` and the phase report; the build's wall time and the box's free RAM before/after are recorded too (they are the deploy-runbook numbers). +- **D5 — the build is an operator step with a runbook (task 01 + README).** The migration is correct, but building HNSW over 414k × 768 on the production 3,904 MB box needs a documented pre-flight: free-RAM check, `maintenance_work_mem` set explicitly for the session (e.g. 512 MB, never left at the 4 MB `work_mem` default), the build run against the postgres container directly (`podman exec … psql`) if the app container is memory-limited (the owner's separate task), `autovacuum` left running afterwards, and the INVALID-index recovery path. `README.md` gains that runbook section; the phase's own CI build runs against the small test corpus, not 414k rows. +- **NOT touched:** the embedding dim (A6), the `<=>` operator and the hybrid fusion (A7), the thresholds themselves (A8 — unchanged unless the battery forces an `ef_search` raise), the `tsv` GIN index, `chunks`/`documents` columns, the importer/sync (phases 183–186), the frontend, any new dependency or service (A3/A12). + +## Dependencies +— (none; deliberately the LAST of the six incident phases — schema + retrieval-quality risk after the sync's memory work) + +## Tasks +1. `01_hnsw_migration.md` — migration `0029` (autocommit block, `CONCURRENTLY`, the INVALID-index pre-drop + post-validate), the tested downgrade, the migration tests. +2. `02_ef_search_setting.md` — `retrieval_ef_search` on `Settings` + the catalogue group + `.env.example`, `SET LOCAL` in `_vector_candidates`, the clamp-and-warn rule, the negative-refuses-startup pin. +3. `03_plan_and_recall_pins.md` — the `EXPLAIN` plan pin (index used, no seq scan) and the deterministic ≥0.99 overlap / exact-`top_score` recall pin on a fixed-seed corpus. +4. `04_recall_battery_gate.md` — the live battery index-off vs index-on on the 1,000-doc replica KB; the recorded numbers in `benchmarks/` + the phase report; the `ef_search` raise loop if anything moved. +5. `05_e2e_index_chat_and_runbook.md` — `tests/e2e/test_vector_index_recall.py` (the phase's dedicated Playwright suite), the `README.md` build runbook, the full gates, the docs pass, the atomic commit. + +## Testing & Quality +- **Unit:** the `retrieval_ef_search` default/clamp/negative rules; the migration module's helpers (the INVALID-index decision function is a pure function over a query result, so it unit-tests without a DB). +- **Integration:** `tests/integration/test_chunks_hnsw_index.py` — upgrade/downgrade round-trip on the test DB, `pg_index.indisvalid` asserted, the INVALID-index recovery path exercised by creating an INVALID index deliberately (`… WHERE false` won't produce INVALID; use the documented `UPDATE pg_index SET indisvalid=false` superuser trick or a deliberately interrupted build — pick the one the test DB allows and document it), the `EXPLAIN` plan pin, the deterministic recall pin. `tests/unit/test_retriever.py` + `test_retriever_recency.py` + `tests/integration/test_chat_db_sessions.py` green with `SET LOCAL` in play. +- **Live gate:** the retrieval battery off/on, identical verdicts, recorded. +- **E2E (AGENTS rule 4, task 05):** `tests/e2e/test_vector_index_recall.py`, run **in isolation**. +- **Coverage:** **>90%** on `app/` (`uv run pytest --cov=app --cov-report=term-missing`). +- **Lint/types:** `uv run ruff check . && uv run pyright`. + +## Documentation & Consistency +- Final pass reviews `README.md` and `AGENTS.md` in their entirety against the post-phase app and fixes every inconsistency found, pre-existing ones included. +- `README.md`: the `0029` deploy section (the D5 build runbook: pre-flight free RAM, `maintenance_work_mem`, `CONCURRENTLY` semantics, the INVALID-index recovery, expected build time from task 04's measurement) and the `BOR_RETRIEVAL_EF_SEARCH` knob (default 100, the clamp, "raise it if recall matters"). `AGENTS.md`: the quick-reference gains nothing new unless a command changes — the retrieval battery line already exists; keep it accurate. + +## Completion Criteria +- [ ] `alembic/versions/0029_…` creates `ix_chunks_embedding_hnsw` `CONCURRENTLY` inside an autocommit block, pre-drops an INVALID index of that name, post-validates `indisvalid`, and its downgrade is tested (`upgrade head` → `downgrade -1` → `upgrade head` green on the test DB, A13). +- [ ] `retrieval_ef_search` (default 100) is applied with `SET LOCAL` in `_vector_candidates`, clamped up with a WARNING below the A7 candidate count, negative refuses startup, and it appears in `CATALOG` (`"Retrieval & honesty"`) + `.env.example` + `README.md`. +- [ ] The `EXPLAIN` pin shows the index used and no seq scan; the deterministic recall pin shows ≥0.99 top-100 overlap and exact `top_score`s. +- [ ] The live battery ran index-off and index-on over the 1,000-doc replica with IDENTICAL top scores, top-5 sets and deflection verdicts, and the numbers (plus build time + free-RAM readings) are recorded in `benchmarks/` and the phase report. +- [ ] `uv run pytest` green, `app/` coverage >90%, ruff + pyright clean. +- [ ] `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` green **in isolation**. +- [ ] `README.md` and `AGENTS.md` accurate and consistent with the whole project (the runbook included). +- [ ] One atomic `--no-gpg-sign` Conventional Commit (e.g. `perf(retrieval): index chunks.embedding with HNSW and pin recall so cosine stops full-scanning 414k vectors`); the phase dir moves to `.agents/phases/complete/`. + +## Locked decisions (owner-confirmed 2026-10-15) +- **A1 — HNSW on `vector(768)` with `vector_cosine_ops`, built `CONCURRENTLY`.** No query rewrite, no new dependency, `<=>` uses it directly (A3). +- **A2 — `halfvec` quantisation is NOT part of this phase** (it needs a statement cast + its own recall study); revisit only if the production build does not fit in RAM. +- **A3 — recall is a gate, not a hope.** The honesty gate (A8) reads `top_score`; an approximate index that changes a top hit can flip a deflect. The deterministic ≥0.99 pin AND the live battery must pass, and the remedy is a higher `ef_search`, never a relaxed assertion. +- **A4 — `SET LOCAL`, per transaction.** No global GUC mutation, no connection-pool-wide state (A12: no aux services, restart clears state). +- **A5 — the production build is an operator action with a runbook**, executed after the owner's container-memory work lands; the phase's CI never builds a 414k-row graph. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/01_hnsw_migration.md b/.agents/phases/todo/188_chunks_hnsw_index/01_hnsw_migration.md new file mode 100644 index 0000000..ba0400a --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/01_hnsw_migration.md @@ -0,0 +1,44 @@ +# Task 01 — Migration `0029`: the HNSW index, built concurrently, reversible + +**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation (`pg_indexes` shows no index on `chunks.embedding`) +**Story:** n/a + +## Objective +Add `ix_chunks_embedding_hnsw` on `chunks.embedding` with `vector_cosine_ops` through alembic `0029`, built `CONCURRENTLY` inside an autocommit block, self-healing an interrupted build's INVALID index, with a tested downgrade (A13). + +## Work +1. `alembic/versions/0029_chunks_embedding_hnsw.py` — `revision = "0029"`, `down_revision = "0028"` (verify with `uv run alembic heads` before writing), the house docstring shape (why / what / reversibility): + - Pure helpers (unit-testable, no I/O): `INDEX_NAME = "ix_chunks_embedding_hnsw"`, and `def should_drop_invalid(indisvalid: bool | None) -> bool` (True when the row exists and `indisvalid is False`), plus `def is_valid(indisvalid: bool | None) -> bool`. + - `upgrade()`: + ```python + with op.get_context().autocommit_block(): + if should_drop_invalid(_lookup_index_validity(op.get_bind())): + op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}") + op.execute( + f"CREATE INDEX CONCURRENTLY {INDEX_NAME} ON chunks " + "USING hnsw (embedding vector_cosine_ops) WITH (m = 16, ef_construction = 64)" + ) + if not is_valid(_lookup_index_validity(op.get_bind())): + raise RuntimeError(f"{INDEX_NAME} did not validate — drop it and re-run") + ``` + `_lookup_index_validity(bind)` runs `select i.indisvalid from pg_index i join pg_class c on c.oid = i.indexrelid where c.relname = :name` and returns `None` when absent. The docstring names the failure mode this guards (a `CONCURRENTLY` build killed mid-way leaves an INVALID index that costs inserts and serves nothing — on a 3,904 MB production box that is the expected failure, not the exotic one). + - `downgrade()`: the same autocommit block with `DROP INDEX CONCURRENTLY IF EXISTS ix_chunks_embedding_hnsw`. + - No `chunk`/`document` column change, no data rewrite (so the migration is cheap on a healthy box and interruptible). +2. `tests/integration/test_chunks_hnsw_index.py` — the migration family: + - `test_upgrade_creates_a_valid_hnsw_index`: run the migration's `upgrade()` against the test DB (the house alembic-in-test pattern — find the existing migration test's approach first and reuse it), then assert `pg_indexes` has the index, `pg_index.indisvalid` is true, and `pg_get_indexdef` contains `USING hnsw` + `vector_cosine_ops` + `ef_construction=64`. + - `test_downgrade_removes_it` then `test_upgrade_head_again` — the A13 round-trip. + - `test_invalid_index_is_dropped_and_rebuilt`: force an INVALID state (either `UPDATE pg_index SET indisvalid = false WHERE indexrelid = '…'::regclass` as the test DB's superuser, or a documented alternative) → run `upgrade()` → the index is valid again (no `relation already exists` error, no leftover INVALID index). + - `test_should_drop_invalid_matrix` (pure): `None` → False, `True` → False, `False` → True. + - Skip-guard: these tests need the `vector` extension and DDL rights — follow the existing migration tests' skip/fixture conventions (do NOT invent a new one). +3. Verify: `uv run alembic upgrade head && uv run alembic downgrade -1 && uv run alembic upgrade head` on the dev DB (record the output in the task note); `uv run pytest tests/integration/test_chunks_hnsw_index.py -v`; `uv run pytest`. + +## Testing & Quality +- Integration: the four migration pins above (create / downgrade / re-upgrade / INVALID self-heal). +- Unit: the two pure helpers' matrices. +- Coverage: **>90%** on `app/` (the migration module is covered by its own tests). + +## Completion Criteria +- [ ] `alembic/versions/0029_chunks_embedding_hnsw.py` exists with the autocommit block, `CONCURRENTLY`, the INVALID self-heal, the post-validate raise, and the `CONCURRENTLY` downgrade. +- [ ] The `upgrade head → downgrade -1 → upgrade head` round-trip is green on the dev DB (output recorded) and in the integration tests. +- [ ] The forced-INVALID test is green and documents the trick it used. +- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/02_ef_search_setting.md b/.agents/phases/todo/188_chunks_hnsw_index/02_ef_search_setting.md new file mode 100644 index 0000000..c87658a --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/02_ef_search_setting.md @@ -0,0 +1,37 @@ +# Task 02 — `BOR_RETRIEVAL_EF_SEARCH`: explicit, per-transaction, clamped + +**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation +**Story:** n/a + +## Objective +Give the HNSW tier an operator-visible `ef_search` that defaults to the A7 candidate count and can never be set below it, applied per transaction with `SET LOCAL` so no request leaks state into another. + +## Work +1. `app/config.py` — `retrieval_ef_search: int = 100`, the `hybrid_vector_candidates` (L351) docstring style, `BOR_RETRIEVAL_EF_SEARCH` via the `env_prefix`. A negative value refuses startup exactly like the phase-182 `show_text_max_*` knobs (follow their validator, don't invent one); `0` is rejected the same way (there is no sensible `ef_search=0` — the clamp would silently rewrite it, which is exactly what a `0`-means-off operator would not expect). +2. `app/core/settings_catalog.py` — `SettingMeta("retrieval_ef_search", "Retrieval & honesty", "Vector ef_search")`; `uv run pytest tests/unit/test_settings_catalog.py -v` proves the completeness pin. +3. `app/rag/retriever.py` — `_vector_candidates` (**L430–446**): before the SELECT, + ```python + ef = settings.retrieval_ef_search # settings passed in / read the way this module already reads them + if ef < limit: + logger.warning("retrieval: ef_search=%d raised to %d (A7 candidate count)", ef, limit) + ef = limit + db.execute(text(f"SET LOCAL hnsw.ef_search = {int(ef)}")) # int()-coerced: no user string reaches SQL + ``` + Notes the code must carry: `SET LOCAL` is transaction-scoped (A4 — the session's transaction commits/rolls back with the request; no global GUC mutation, nothing surviving a pool recycle); the value is `int()`-coerced so nothing but a number is interpolated; the clamp exists because `ef_search < LIMIT` returns FEWER candidates than the A7 tier promises, i.e. a silent recall regression. + Threading: `_vector_candidates` currently takes `(db, question_embedding, limit)` — read `Settings` the same way the surrounding functions do (find the module's existing `get_settings()`/passed-settings convention and match it; no new plumbing if one exists). +4. Pins: + - `tests/unit/test_retriever.py`: the clamp (ef 10 with limit 100 → warning + 100 used), the pass-through (ef 200 → 200), the default (unset → 100), the negative-refusal at settings construction; assert the emitted statement text through a stub session that records `db.execute` calls (no DB needed). + - `tests/integration/test_chunks_hnsw_index.py`: after a real `SET LOCAL`, `show hnsw.ef_search` in the same transaction reports the clamped value and a NEW transaction reports the server default again (the scoping pin). + - `tests/integration/test_chat_db_sessions.py` + `tests/unit/test_retriever_recency.py` green (the extra statement in the turn's transaction breaks nothing). +5. Verify: `uv run pytest tests/unit/test_retriever.py tests/unit/test_settings_catalog.py tests/integration/test_chunks_hnsw_index.py tests/integration/test_chat_db_sessions.py -v`; `uv run pytest`. + +## Testing & Quality +- Unit: clamp / pass-through / default / negative refusal / statement text. +- Integration: `SET LOCAL` scoping (same-transaction vs new). +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `retrieval_ef_search` exists (default 100, negative refused), is catalogued under `"Retrieval & honesty"`, and is in `.env.example` + `README.md`. +- [ ] `_vector_candidates` issues `SET LOCAL hnsw.ef_search` with the clamp-and-warn rule, int-coerced. +- [ ] The scoping pin proves a new transaction sees the server default again. +- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/03_plan_and_recall_pins.md b/.agents/phases/todo/188_chunks_hnsw_index/03_plan_and_recall_pins.md new file mode 100644 index 0000000..0fec4bc --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/03_plan_and_recall_pins.md @@ -0,0 +1,27 @@ +# Task 03 — Prove the planner uses it and that recall did not move (deterministic) + +**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation +**Story:** n/a + +## Objective +Two CI pins: the cosine tier's statement uses `ix_chunks_embedding_hnsw` (no sequential scan), and the index-backed top-100 matches the exact top-100 on a fixed-seed corpus, with `top_score` values exact to 6 decimals. + +## Work +1. `tests/integration/test_chunks_hnsw_recall.py` — a NEW file: + - **The corpus fixture:** ~5,000 `Chunk` rows with 768-dim vectors from a FIXED seed (a documented `random.Random(20261015)` scheme: 40 cluster centroids + noise around them, so nearest neighbours are meaningful rather than uniformly random), all L2-normalised (cosine == dot product, so the exact answer is trivial to compute), committed, then the HNSW index built for this corpus via the `0029` migration's `upgrade()` (task 01) — never `CREATE INDEX` inline (the migration is the single definition). + - `test_plan_uses_the_hnsw_index`: `EXPLAIN (FORMAT JSON)` for the exact statement `_vector_candidates` builds (the same SQLAlchemy statement compiled with literal binds, so the planner sees a real plan), assert the node names `ix_chunks_embedding_hnsw` / an index scan node and that no `Seq Scan on chunks` node appears. Assert the same statement's plan WITHOUT the index (a `DROP INDEX`/re-create around a sub-fixture, or a `SET enable_indexscan = off`-style comparison — pick one, document it) DOES seq-scan, so the pin cannot pass vacuously. + - `test_top100_recall_at_ef_search_100`: for 30 fixed-seed query vectors, compute the EXACT cosine top-100 (pure Python over the fixture corpus) and compare against the index-backed `_vector_candidates(db, q, 100)` result: assert `|overlap| / 100 >= 0.99` for every query (mean reported in the docstring) and that each returned row's `1 - distance` equals the exact cosine within 1e-6. Print the per-query overlap in the failure message (so a future raise of `ef_search` has evidence). + - `test_recall_never_below_the_pinned_floor_when_ef_search_raised`: the same assertion at `ef_search = 200` must be ≥ the `ef_search = 100` mean overlap (monotonicity sanity: raising `ef` cannot make recall worse). + - Module docstring: the production numbers (414,220 chunks / 3,000 MB / no index found / pgvector 0.8.0), why 5,000 rows is enough for a plan pin but not a proxy for prod (the live battery in task 04 is), the measured overlaps, and the `ef_search` values used. +2. Runtime guard: the fixture corpus build + index creation must stay fast (target < ~60 s); if it drifts, reduce the corpus (never the assertions) and record the new size — the phase-179 A4 discipline ("cut the fixture, never the guard"). +3. Verify: `uv run pytest tests/integration/test_chunks_hnsw_recall.py -v`; `uv run pytest`. + +## Testing & Quality +- Integration: the plan pin (with its vacuity guard) + the two recall pins. +- Coverage: **>90%** on `app/`. + +## Completion Criteria +- [ ] `uv run pytest tests/integration/test_chunks_hnsw_recall.py -v` green: index used, no seq scan, mean top-100 overlap ≥ 0.99, `top_score`s exact to 1e-6, monotonic at `ef_search=200`. +- [ ] The vacuity guard (seq scan when the index is unavailable) is present and green. +- [ ] The docstring records the measured overlaps and the prod context numbers. +- [ ] `uv run pytest` green, coverage >90%, ruff + pyright clean. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/04_recall_battery_gate.md b/.agents/phases/todo/188_chunks_hnsw_index/04_recall_battery_gate.md new file mode 100644 index 0000000..57b91e7 --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/04_recall_battery_gate.md @@ -0,0 +1,35 @@ +# Task 04 — The live gate: the retrieval battery, index off vs on + +**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation; PLAN A8 +**Story:** n/a + +## Objective +Prove on the real embedding endpoint and the 1,000-document live-replica KB that the approximate index does not move a single verdict — same `top_score`, same top-5 documents, same deflection decisions — and record the numbers plus the production build cost. + +## Why this is a gate and not a note +PLAN **A8** derives the honesty decision from `best_cosine` against `BOR_RELEVANCE_THRESHOLD` (0.62) and `BOR_LEXICAL_SUPPORT_FLOOR` (0.35). An approximate index that swaps which chunk is nearest can flip a HIGH into a deflect, or cite a different document — the exact class of silent regression the phase-119/162 battery exists to catch. + +## Prerequisites (document them in the phase report before starting) +`AIPI_KEY` in the environment (the same convention as `scripts/llm_probe.py` / `scripts/eval_retrieval.py`), the 1,000-document live-replica KB restored (the `restore-test-db` skill), and `uv run alembic upgrade head` applied. If `AIPI_KEY` or the replica KB is unavailable, STOP and report it — do not commit the migration as phase-complete on the strength of task 03's synthetic pin alone. + +## Work +1. **Baseline (index OFF):** drop the index (`DROP INDEX CONCURRENTLY ix_chunks_embedding_hnsw` — the migration's downgrade), then + `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt > /tmp/battery_off.txt`. + Capture per question: the effective/top score, the top-5 document list, the honesty-gate verdict column. +2. **Candidate (index ON):** re-apply (`uv run alembic upgrade head`), same command to `/tmp/battery_on.txt`. +3. **The diff:** compare question by question. Required: identical deflection verdicts, identical top-5 document sets, and `top_score` deltas ≤ 1e-4. Any difference → raise `BOR_RETRIEVAL_EF_SEARCH` (150 → 200 → 400) and re-run BOTH sides until the verdicts match; the `ef_search` that restores parity becomes the shipped default (update `Settings` + `.env.example` + `README.md` + the task-02 tests, and say so in the commit body). Never widen the tolerance. +4. **Record** (the repo's measurement homes, not chat): + - `benchmarks/` — a row/section per run: pgvector version, index definition (`m`, `ef_construction`), `ef_search`, corpus size, the battery's question count, verdict-agreement counts, the max `top_score` delta, the battery's wall time off vs on, and the per-question cosine-tier latency if the script reports it. + - the phase report (`.agents/reports/`, house location) — the production BUILD numbers captured while building on a box of the deployment's size class where possible: build wall time, `maintenance_work_mem` used, free RAM before/after (`free -m`), index size (`select pg_size_pretty(pg_relation_size('ix_chunks_embedding_hnsw'))`), and the seq-scan → index-scan latency change on a real chat turn. +5. **Timing sanity:** the interactive path got faster, not slower (A21's "latency binds the interactive path"). Record the before/after; if the index makes a turn slower at the shipped `ef_search`, that is a finding for the phase report, not a silent acceptance. +6. Re-run the app's own retrieval regression expectations if the battery script's output is asserted anywhere in the suite (`grep -rn "retrieval_battery" tests/ scripts/`), and keep those green. + +## Testing & Quality +- Live gate: the off/on battery comparison with identical verdicts (the remedy is `ef_search`, never the tolerance). +- Recorded artifacts: `benchmarks/` entry + the phase report numbers (both referenced by path in the commit body). + +## Completion Criteria +- [ ] `/tmp/battery_off.txt` and `/tmp/battery_on.txt` (or their committed equivalents) exist, and the comparison shows identical deflection verdicts, identical top-5 sets, max score delta ≤ 1e-4. +- [ ] The shipped `retrieval_ef_search` default is the value that achieved parity (documented whether it stayed 100 or was raised). +- [ ] `benchmarks/` records the index definition, sizes, build time, free-RAM readings and the off/on timings; the phase report repeats the production build numbers. +- [ ] Any test that reads the battery fixture stays green. diff --git a/.agents/phases/todo/188_chunks_hnsw_index/05_e2e_index_chat_and_runbook.md b/.agents/phases/todo/188_chunks_hnsw_index/05_e2e_index_chat_and_runbook.md new file mode 100644 index 0000000..57cf2e7 --- /dev/null +++ b/.agents/phases/todo/188_chunks_hnsw_index/05_e2e_index_chat_and_runbook.md @@ -0,0 +1,36 @@ +# Task 05 — E2E: chat over the indexed KB (the phase's dedicated Playwright suite) + +**Phase:** `188_chunks_hnsw_index` · **Source:** the 2026-10-15 incident investigation +**Story:** n/a + +## Objective +Prove through the browser that retrieval behaves identically over the indexed KB — the same document is cited, the answer is not deflected, and the turn completes — plus the `README.md` build runbook, the full gates, the docs pass, and the atomic commit. + +## Work +1. `tests/e2e/test_vector_index_recall.py` — a NEW module, mock-only (`E2E_REAL_LLM=1` skipped; the E2E mock's deterministic L2-normalised token-overlap embeddings make the cosine tier reproducible, PLAN A16), the `tests/e2e/test_retrieval_quality.py` / `test_search_recall_small_sources.py` / `test_chat_rag.py` shapes (dedicated module app + port, mock LLM, dedicated `BOR_SOURCES_DIR`, shared E2E Postgres truncated before AND after each test, admin login via `tests/e2e/auth_helpers.py`). + - Fixture: a small KB (8–12 docs) imported through the real importer so chunks + embeddings exist, with the `0029` migration's index present in the test DB (import-time inserts into an HNSW-indexed table are exercised by this fixture too — inserts are the phase's insert-cost surface, so keep them in the path rather than importing then indexing). + - **Test 1 `test_answered_question_cites_the_same_document_indexed_or_not`:** the sentinel question is asked twice — once with the index dropped (test-local DDL, restored after) and once with it present — and the cited source chip / cited document is the SAME in both, with no deflection either way. This is the A7/A8 contract expressed in the UI, and it is a real before/after, not a tautology. + - **Test 2 `test_index_exists_and_is_valid_after_import`:** after the fixture import, the test's own DB query asserts `pg_index.indisvalid` is true for `ix_chunks_embedding_hnsw` (an index that died during the fixture's inserts would be caught here — the silent-INVALID failure mode from task 01, at the app layer). + - **Test 3 `test_near_miss_question_still_deflects`:** a question with no KB support is deflected with the index present (A8's LOW path is unchanged — the index must not manufacture a high enough `top_score` to flip a deflect). + - **Test 4 `test_ef_search_setting_is_honoured`:** with `BOR_RETRIEVAL_EF_SEARCH=150` on the module app, the turn still answers normally (a knob change cannot break a chat turn), and the clamp warning appears when set to a value below the A7 tier (caplog or the status/log surface the test harness can read). + - Module docstring: the incident/investigation reference, the index definition + `ef_search` default, the test→observable mapping, the run-in-isolation command, prerequisites (`podman compose up -d db`). +2. **README runbook (D5):** a `Deploying migration 0029 (HNSW)` section — the pre-flight (`free -m`, `shared_buffers`, expected peak), building with an explicit `maintenance_work_mem` (never the 4 MB default), `CREATE INDEX CONCURRENTLY` semantics (no lock, longer build, may leave an INVALID index if killed), the recovery path (drop and re-run the migration), the measured numbers from task 04 (build time, index size), and the pointer that the owner's container-memory work should land before building on a 4 GB-class box. +3. **Docs pass:** `README.md` + `AGENTS.md` reviewed in full against the post-phase app; every inconsistency fixed, pre-existing included; `AGENTS.md`'s quick-reference stays accurate (the retrieval-battery line already exists and is now load-bearing for this phase — confirm its command line is still correct). +4. **The full gates:** + - `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` — green **in isolation**. + - `uv run pytest tests/e2e/test_retrieval_quality.py -v --no-cov`, `tests/e2e/test_search_recall_small_sources.py -v --no-cov`, `tests/e2e/test_chat_rag.py -v --no-cov` — green in isolation (the retrieval/chat families the index touches; run after the new suite). + - `uv run pytest --cov=app --cov-report=term-missing` — full suite green, `app/` coverage **>90%**. + - `uv run ruff check . && uv run pyright` — clean. + - `uv run python -m scripts.eval_retrieval --from-file tests/fixtures/retrieval_battery.txt` — task 04's parity verdict still holds at the shipped `ef_search`. +5. **Commit + move:** one atomic `--no-gpg-sign` Conventional Commit — e.g. `perf(retrieval): index chunks.embedding with HNSW and pin recall so cosine stops full-scanning 414k vectors` — covering `alembic/versions/0029_…`, `app/config.py`, `app/core/settings_catalog.py`, `app/rag/retriever.py`, the tests, `benchmarks/`, `README.md`, `.env.example` and `.agents/`. Move the phase dir to `.agents/phases/complete/188_chunks_hnsw_index/`. + +## Testing & Quality +- E2E: the dedicated suite + the three retrieval/chat E2E families in isolation. +- Coverage: **>90%** on `app/`; ruff + pyright clean. + +## Completion Criteria +- [ ] `uv run pytest tests/e2e/test_vector_index_recall.py -v --no-cov` green **in isolation** (same citation indexed vs not, index valid after import, near-miss still deflects, `ef_search` change harmless). +- [ ] The three regression E2E families green in isolation; the battery parity verdict still holds. +- [ ] `uv run pytest --cov=app --cov-report=term-missing` green with `app/` coverage >90%; ruff + pyright clean. +- [ ] `README.md` (runbook + knob) and `AGENTS.md` accurate and consistent with the whole project. +- [ ] One atomic `--no-gpg-sign` Conventional Commit; the phase dir moved to `.agents/phases/complete/`. diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.err b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.err new file mode 100644 index 0000000..e69de29 diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md new file mode 100644 index 0000000..6f855c7 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md @@ -0,0 +1,20 @@ +All gates green. Final state verified. + +**Implemented / verified** +- Task 03's work was **never actually done** (its report was a stub, `tests/e2e/test_prune_cascade_cleanup.py` did not exist) → wrote it: 3 tests — 9-file local source synced from the Sources page (`9 added`), 6 files deleted upstream, re-sync (`0 added · 3 unchanged · 6 pruned`), documents vanished from the tree + `GET /api/documents/content` 404 + total 3, **0 orphan chunk rows** via the test's own SQL, survivor chunk total unchanged; partial-prune chat turn (grounded, non-deflected, `query_log` names the survivor and no pruned doc); total source removal (`DELETE /api/git-sources/{id}`) wipes every doc+chunk of the source with 0 orphans, sibling source untouched. Mock-only (`E2E_REAL_LLM=1` skipped), own port 8199, truncate before/after. +- Tasks 01/02 re-verified: `passive_deletes=True` + mapper pin, probe flipped to `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, `_prune`/`delete_git_source` docstrings. +- **Drift guard proven non-tautological by my own one-off run**: scratch DB (`alembic upgrade head`) → FK `confdeltype='c'`, guard PASSES, CASCADE delete leaves 0 chunks; FK recreated `ON DELETE NO ACTION` → guard FAILS verbatim and a `passive_deletes` delete raises `ForeignKeyViolation` (never silent orphans). Scratch DB dropped; shared test DB untouched (`'c'`). + +**Gates** +- `uv run pytest --cov=app --cov-report=term-missing` → **4488 passed**, `app/` TOTAL **99%** (>90%) +- `uv run pytest tests/integration/test_prune_cascade_statements.py -v` → 4 passed +- `uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov` → **3 passed in isolation**; `test_source_removal_cleanup.py` 6 passed, `test_force_push_resync.py` 2 passed (both isolated) +- `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors + +**Completion criteria**: passive_deletes+pin ✅ · probe post-fix ✅ · drift guard green + proven-failing variant ✅ · full suite/coverage/lint ✅ · E2E in isolation ✅ · docs consistent ✅ · commit = harness-only (per pipeline rule I ran no git write commands). + +**Docs**: fixed 4 pre-existing README inconsistencies — per-module E2E ports (claim of "one pair of fixed ports" predates phase 79), debugpy `launch.json` snippet vs the shipped file (`name`/`remoteRoot`), and the page-naming wobble (navbar labels `/sources.html` **RAG**, `/git-sources.html` **Sources** — the Sync button's home is now named correctly in 4 places); added the phase-183 E2E pointer. AGENTS.md verified accurate (rule 12 matches the delete-site audit). + +**Deviation**: none from the locked decisions; the E2E is a behaviour pin (byte-identical owner-visible behaviour), the statement-count probe remains the memory teeth, as phase 183 D1/A4 specify. + +**Next pending phase**: `184_sync_bounded_session`. diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md.status b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md.status new file mode 100644 index 0000000..9766475 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.md.status @@ -0,0 +1 @@ +ok diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.validate b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.validate new file mode 100644 index 0000000..1e5a494 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__00_phase.a1.validate @@ -0,0 +1,143 @@ +........................................................................ [ 1%] +........................................................................ [ 3%] +........................................................................ [ 4%] +........................................................................ [ 6%] +........................................................................ [ 8%] +........................................................................ [ 9%] +........................................................................ [ 11%] +........................................................................ [ 12%] +........................................................................ [ 14%] +........................................................................ [ 16%] +........................................................................ [ 17%] +........................................................................ [ 19%] +........................................................................ [ 20%] +........................................................................ [ 22%] +........................................................................ [ 24%] +........................................................................ [ 25%] +........................................................................ [ 27%] +........................................................................ [ 28%] +........................................................................ [ 30%] +........................................................................ [ 32%] +........................................................................ [ 33%] +........................................................................ [ 35%] +........................................................................ [ 36%] +........................................................................ [ 38%] +........................................................................ [ 40%] +........................................................................ [ 41%] +........................................................................ [ 43%] +........................................................................ [ 44%] +........................................................................ [ 46%] +........................................................................ [ 48%] +........................................................................ [ 49%] +........................................................................ [ 51%] +........................................................................ [ 52%] +........................................................................ [ 54%] +........................................................................ [ 56%] +........................................................................ [ 57%] +........................................................................ [ 59%] +........................................................................ [ 60%] +........................................................................ [ 62%] +........................................................................ [ 64%] +........................................................................ [ 65%] +........................................................................ [ 67%] +........................................................................ [ 68%] +........................................................................ [ 70%] +........................................................................ [ 72%] +........................................................................ [ 73%] +........................................................................ [ 75%] +........................................................................ [ 77%] +........................................................................ [ 78%] +........................................................................ [ 80%] +........................................................................ [ 81%] +........................................................................ [ 83%] +........................................................................ [ 85%] +........................................................................ [ 86%] +........................................................................ [ 88%] +........................................................................ [ 89%] +........................................................................ [ 91%] +........................................................................ [ 93%] +........................................................................ [ 94%] +........................................................................ [ 96%] +........................................................................ [ 97%] +........................................................................ [ 99%] +........................ [100%] +=============================== warnings summary =============================== +.venv/lib/python3.13/site-packages/fastapi/testclient.py:1 + /var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead. + from starlette.testclient import TestClient as TestClient # noqa + +-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html +================================ tests coverage ================================ +_______________ coverage: platform linux, python 3.13.13-final-0 _______________ + +Name Stmts Miss Cover +-------------------------------------------------- +app/__init__.py 1 0 100% +app/api/__init__.py 0 0 100% +app/api/admin_settings.py 114 0 100% +app/api/auth.py 52 0 100% +app/api/chat.py 292 1 99% +app/api/chat_images.py 52 0 100% +app/api/chats.py 111 0 100% +app/api/config.py 13 0 100% +app/api/doc_drafts.py 99 0 100% +app/api/docs.py 239 1 99% +app/api/git_sources.py 259 0 100% +app/api/health.py 10 0 100% +app/api/steering.py 42 0 100% +app/api/suggestions.py 33 0 100% +app/api/sync.py 204 0 100% +app/api/tokens.py 40 0 100% +app/api/ui_settings.py 55 0 100% +app/config.py 304 0 100% +app/core/__init__.py 0 0 100% +app/core/auth.py 45 0 100% +app/core/caching.py 124 0 100% +app/core/compression.py 59 0 100% +app/core/debugging.py 29 2 93% +app/core/docs_push.py 43 0 100% +app/core/errors.py 5 0 100% +app/core/http_cache.py 22 0 100% +app/core/logging.py 13 0 100% +app/core/rate_limit.py 44 0 100% +app/core/response_budget.py 63 0 100% +app/core/security_headers.py 20 0 100% +app/core/settings_catalog.py 18 0 100% +app/core/settings_migrate.py 32 0 100% +app/core/settings_overlay.py 111 0 100% +app/core/theming.py 40 0 100% +app/core/tokens.py 44 0 100% +app/db.py 22 0 100% +app/main.py 105 0 100% +app/models.py 141 0 100% +app/rag/__init__.py 0 0 100% +app/rag/agent.py 494 1 99% +app/rag/archive_upload.py 134 0 100% +app/rag/chunker.py 220 4 98% +app/rag/doc_dates.py 18 0 100% +app/rag/folder_summaries.py 134 0 100% +app/rag/git_sources.py 77 0 100% +app/rag/importer.py 855 4 99% +app/rag/llm.py 253 1 99% +app/rag/md_images.py 63 0 100% +app/rag/overview.py 78 0 100% +app/rag/pdf.py 192 0 100% +app/rag/prompts.py 162 0 100% +app/rag/retriever.py 252 3 99% +app/rag/scaffolding.py 55 0 100% +app/rag/source_removal.py 41 0 100% +app/rag/sources_meta.py 16 0 100% +app/rag/suggestions.py 27 0 100% +app/rag/summarizer.py 58 0 100% +app/rag/svg_text.py 111 0 100% +app/rag/verbatim.py 160 0 100% +app/schemas.py 549 0 100% +-------------------------------------------------- +TOTAL 6849 17 99% +coverage gate: app/ 99% (>90%) OK +All checks passed! +0 errors, 0 warnings, 0 informations +WARNING: there is a new pyright version available (v1.1.411 -> v1.1.414). +Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest` + +validation OK diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.err b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.err new file mode 100644 index 0000000..e69de29 diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md new file mode 100644 index 0000000..850ac06 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md @@ -0,0 +1,21 @@ +Task complete. Report: + +**Implemented** (`tests/integration/test_prune_cascade_statements.py`, new; no `app/` change) +- Statement-capture fixture: `before_cursor_execute` listener on `engine`, installed per-test and `event.remove`d in teardown; `chunk_selects()` matches `SELECT … \bfrom chunks\b` (word-boundary, so `documents` never matches). +- `seed_docs`: 200 `Document` rows (`source="cascade-probe"`) × 25 `Chunk` rows, 745-char content, deterministic 768-float vector rotated by doc index, chunks added by `document_id` so the relationship stays unloaded. +- `test_prune_loads_every_chunk_collection`: real `_prune(session, {"cascade-probe"}, set(), images=False, summary=…, page_dir=None)` → `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 1` × 200 chunk SELECTs, `_prune` returns 200, 0 orphan `Chunk` rows (D4), 0 documents left, 200 `changed_paths` pairs. +- `test_prune_memory_delta_has_a_ceiling`: `VmRSS` delta with `gc.collect()` on both sides; branches on the constant — post-fix (task 02, `0` selects) asserts `< 40 MB`, while pre-fix asserts the delta lands **above** 40 MB and under a 400 MB sanity bound, so the ceiling is proven to have teeth before it becomes the contract (D1/LOCKED A4: statement count is the load-bearing pin; RSS is corroboration, never the flake). +- Module docstring = the measurement record: the 2026-10-15 incident (3,008 litellm docs, 3,904 MB VM, no swap, 3.1 GB Postgres, 3.3 GB cgroup peak), the mechanism (`models.py:227–229` no `passive_deletes`, `db.py:30` `expire_on_commit=False`, single commit at `importer.py:3934`), the 24.8 KiB-vs-3 KiB and ~0.24 MB/doc unit costs, this run's numbers, and the **CONFIRMED — one chunk SELECT per pruned document** verdict. + +**Gates (all green)** +- `uv run pytest tests/integration/test_prune_cascade_statements.py -v` → 2 passed (run 3×, stable). +- `uv run pytest --cov=app --cov-report=term-missing` → **4485 passed**, `app/` TOTAL 6849 stmts / 17 missed = **99%**. +- `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors. + +**Measured here**: 203 statements per prune (200 `SELECT … FROM chunks` + 1 `SELECT documents` + 2 executemany `DELETE`s), RSS delta **+165.6 MB**; the FK-cascade path measured as a one-off proxy (raw `DELETE FROM documents`) = **0 chunk SELECTs, +0.0 MB, 0 orphans** — hence the 40 MB ceiling (~¼ of the pre-fix delta). + +**Docs**: no doc impact — test-only task; the README prune/FK-cascade note and the `_prune`/`models.py` docstrings belong to task 02 (documenting `passive_deletes` now would describe code that does not exist yet). + +**Deviations**: the RSS ceiling test branches on the flip constant (see above) so the file is green both pre- and post-fix while still discriminating; pre-fix RSS measures 165.6 MB, not the phase's ≈48 MB estimate (fully hydrated vectors + `expire_on_commit=False`) — recorded in the docstring. + +**Next pending task**: `183_sync_passive_delete_cascade/02_passive_deletes.md` (the `passive_deletes=True` fix + live-FK drift guard + flipping `EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC` to `0`). diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md.status b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md.status new file mode 100644 index 0000000..9766475 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.md.status @@ -0,0 +1 @@ +ok diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.validate b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.validate new file mode 100644 index 0000000..7a8a85e --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__01_prune_cascade_probe.a1.validate @@ -0,0 +1,143 @@ +........................................................................ [ 1%] +........................................................................ [ 3%] +........................................................................ [ 4%] +........................................................................ [ 6%] +........................................................................ [ 8%] +........................................................................ [ 9%] +........................................................................ [ 11%] +........................................................................ [ 12%] +........................................................................ [ 14%] +........................................................................ [ 16%] +........................................................................ [ 17%] +........................................................................ [ 19%] +........................................................................ [ 20%] +........................................................................ [ 22%] +........................................................................ [ 24%] +........................................................................ [ 25%] +........................................................................ [ 27%] +........................................................................ [ 28%] +........................................................................ [ 30%] +........................................................................ [ 32%] +........................................................................ [ 33%] +........................................................................ [ 35%] +........................................................................ [ 36%] +........................................................................ [ 38%] +........................................................................ [ 40%] +........................................................................ [ 41%] +........................................................................ [ 43%] +........................................................................ [ 44%] +........................................................................ [ 46%] +........................................................................ [ 48%] +........................................................................ [ 49%] +........................................................................ [ 51%] +........................................................................ [ 52%] +........................................................................ [ 54%] +........................................................................ [ 56%] +........................................................................ [ 57%] +........................................................................ [ 59%] +........................................................................ [ 61%] +........................................................................ [ 62%] +........................................................................ [ 64%] +........................................................................ [ 65%] +........................................................................ [ 67%] +........................................................................ [ 69%] +........................................................................ [ 70%] +........................................................................ [ 72%] +........................................................................ [ 73%] +........................................................................ [ 75%] +........................................................................ [ 77%] +........................................................................ [ 78%] +........................................................................ [ 80%] +........................................................................ [ 81%] +........................................................................ [ 83%] +........................................................................ [ 85%] +........................................................................ [ 86%] +........................................................................ [ 88%] +........................................................................ [ 89%] +........................................................................ [ 91%] +........................................................................ [ 93%] +........................................................................ [ 94%] +........................................................................ [ 96%] +........................................................................ [ 97%] +........................................................................ [ 99%] +..................... [100%] +=============================== warnings summary =============================== +.venv/lib/python3.13/site-packages/fastapi/testclient.py:1 + /var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead. + from starlette.testclient import TestClient as TestClient # noqa + +-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html +================================ tests coverage ================================ +_______________ coverage: platform linux, python 3.13.13-final-0 _______________ + +Name Stmts Miss Cover +-------------------------------------------------- +app/__init__.py 1 0 100% +app/api/__init__.py 0 0 100% +app/api/admin_settings.py 114 0 100% +app/api/auth.py 52 0 100% +app/api/chat.py 292 1 99% +app/api/chat_images.py 52 0 100% +app/api/chats.py 111 0 100% +app/api/config.py 13 0 100% +app/api/doc_drafts.py 99 0 100% +app/api/docs.py 239 1 99% +app/api/git_sources.py 259 0 100% +app/api/health.py 10 0 100% +app/api/steering.py 42 0 100% +app/api/suggestions.py 33 0 100% +app/api/sync.py 204 0 100% +app/api/tokens.py 40 0 100% +app/api/ui_settings.py 55 0 100% +app/config.py 304 0 100% +app/core/__init__.py 0 0 100% +app/core/auth.py 45 0 100% +app/core/caching.py 124 0 100% +app/core/compression.py 59 0 100% +app/core/debugging.py 29 2 93% +app/core/docs_push.py 43 0 100% +app/core/errors.py 5 0 100% +app/core/http_cache.py 22 0 100% +app/core/logging.py 13 0 100% +app/core/rate_limit.py 44 0 100% +app/core/response_budget.py 63 0 100% +app/core/security_headers.py 20 0 100% +app/core/settings_catalog.py 18 0 100% +app/core/settings_migrate.py 32 0 100% +app/core/settings_overlay.py 111 0 100% +app/core/theming.py 40 0 100% +app/core/tokens.py 44 0 100% +app/db.py 22 0 100% +app/main.py 105 0 100% +app/models.py 141 0 100% +app/rag/__init__.py 0 0 100% +app/rag/agent.py 494 1 99% +app/rag/archive_upload.py 134 0 100% +app/rag/chunker.py 220 4 98% +app/rag/doc_dates.py 18 0 100% +app/rag/folder_summaries.py 134 0 100% +app/rag/git_sources.py 77 0 100% +app/rag/importer.py 855 4 99% +app/rag/llm.py 253 1 99% +app/rag/md_images.py 63 0 100% +app/rag/overview.py 78 0 100% +app/rag/pdf.py 192 0 100% +app/rag/prompts.py 162 0 100% +app/rag/retriever.py 252 3 99% +app/rag/scaffolding.py 55 0 100% +app/rag/source_removal.py 41 0 100% +app/rag/sources_meta.py 16 0 100% +app/rag/suggestions.py 27 0 100% +app/rag/summarizer.py 58 0 100% +app/rag/svg_text.py 111 0 100% +app/rag/verbatim.py 160 0 100% +app/schemas.py 549 0 100% +-------------------------------------------------- +TOTAL 6849 17 99% +coverage gate: app/ 99% (>90%) OK +All checks passed! +0 errors, 0 warnings, 0 informations +WARNING: there is a new pyright version available (v1.1.411 -> v1.1.414). +Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest` + +validation OK diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.err b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.err new file mode 100644 index 0000000..e69de29 diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md new file mode 100644 index 0000000..080e2e6 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md @@ -0,0 +1,16 @@ +**Task 02 complete — `passive_deletes=True` landed, all gates green.** + +- `app/models.py`: `Document.chunks` now `passive_deletes=True` + the "don't remove without re-checking the FK" comment (nothing else changed). +- `app/rag/importer.py`: `_prune` docstring gained the phase-183 paragraph (FK cascade, no chunk SELECT, unchanged `changed_paths`/copy cleanup, LOCKED A2 re-embed non-fix). +- `app/api/git_sources.py`: `delete_git_source` docstring corrected — rows drop in the DB via `chunks_document_id_fkey` `ON DELETE CASCADE`, never loaded. +- `tests/integration/test_prune_cascade_statements.py`: probe flipped (`EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0`, test renamed `test_prune_never_selects_a_chunk_row`), docstring now CONFIRMED → FIXED with both measurements; new `test_chunks_fk_cascades_at_database_level` drift guard + new `test_prune_with_a_loaded_collection_still_deletes_the_chunks`. +- `tests/unit/test_importer.py`: mapper pin `passive_deletes is True` (+ `delete-orphan` retained). + +Measurements/gates: pre-fix 203 stmts (200 chunk SELECTs, +165.6 MB RSS) → post-fix **2 stmts, 0 chunk SELECTs, +0.2 MB, 0 orphan chunks**. Drift guard proven non-tautological on a throwaway DB (`bor_183_fk_scratch`: alembic head → FK recreated `ON DELETE NO ACTION` → guard FAILED verbatim, prune then hit `ForeignKeyViolation`; DB dropped, shared test DB untouched). +`uv run pytest` → **4488 passed**; `uv run pytest --cov=app --cov-report=term-missing` → **99% TOTAL (>90%)**; `uv run ruff check .` → All checks passed; `uv run pyright` → 0 errors. E2E spot-checks in isolation: `test_source_removal_cleanup.py` 6 passed, `test_force_push_resync.py` 2 passed, `test_sync_button.py` 3 passed. + +Docs: README — new "A mass prune costs documents, not chunks (phase 183)" bullet under *Sync from the UI* + a `--prune` pointer bullet; AGENTS.md — new rule 12 "Never hydrate what you delete". + +Deviations/audit: delete-site audit confirmed no `doc.chunks` read after any Document delete (`importer._prune` reads only `image_path`/`page_count`/`source`/`path`; `git_sources.py:926` only counts). **Phase-file correction:** `app/api/docs.py:700` is *not* a document-delete endpoint — it deletes a `FolderSummary` row in `PATCH /api/folders/summary` (docs.py's only other delete is a single `Chunk` at L549); there is no Document-delete endpoint there, so the keyword has no effect on that file. Added one test beyond the task list (loaded-collection delete) because the keyword changes ORM behaviour in that state — result: still 0 orphans, ORM deletes loaded children itself. + +Next pending task: `183_sync_passive_delete_cascade/03_e2e_prune_cascade.md`. diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md.status b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md.status new file mode 100644 index 0000000..9766475 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.md.status @@ -0,0 +1 @@ +ok diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.validate b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.validate new file mode 100644 index 0000000..1e5a494 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__02_passive_deletes.a1.validate @@ -0,0 +1,143 @@ +........................................................................ [ 1%] +........................................................................ [ 3%] +........................................................................ [ 4%] +........................................................................ [ 6%] +........................................................................ [ 8%] +........................................................................ [ 9%] +........................................................................ [ 11%] +........................................................................ [ 12%] +........................................................................ [ 14%] +........................................................................ [ 16%] +........................................................................ [ 17%] +........................................................................ [ 19%] +........................................................................ [ 20%] +........................................................................ [ 22%] +........................................................................ [ 24%] +........................................................................ [ 25%] +........................................................................ [ 27%] +........................................................................ [ 28%] +........................................................................ [ 30%] +........................................................................ [ 32%] +........................................................................ [ 33%] +........................................................................ [ 35%] +........................................................................ [ 36%] +........................................................................ [ 38%] +........................................................................ [ 40%] +........................................................................ [ 41%] +........................................................................ [ 43%] +........................................................................ [ 44%] +........................................................................ [ 46%] +........................................................................ [ 48%] +........................................................................ [ 49%] +........................................................................ [ 51%] +........................................................................ [ 52%] +........................................................................ [ 54%] +........................................................................ [ 56%] +........................................................................ [ 57%] +........................................................................ [ 59%] +........................................................................ [ 60%] +........................................................................ [ 62%] +........................................................................ [ 64%] +........................................................................ [ 65%] +........................................................................ [ 67%] +........................................................................ [ 68%] +........................................................................ [ 70%] +........................................................................ [ 72%] +........................................................................ [ 73%] +........................................................................ [ 75%] +........................................................................ [ 77%] +........................................................................ [ 78%] +........................................................................ [ 80%] +........................................................................ [ 81%] +........................................................................ [ 83%] +........................................................................ [ 85%] +........................................................................ [ 86%] +........................................................................ [ 88%] +........................................................................ [ 89%] +........................................................................ [ 91%] +........................................................................ [ 93%] +........................................................................ [ 94%] +........................................................................ [ 96%] +........................................................................ [ 97%] +........................................................................ [ 99%] +........................ [100%] +=============================== warnings summary =============================== +.venv/lib/python3.13/site-packages/fastapi/testclient.py:1 + /var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead. + from starlette.testclient import TestClient as TestClient # noqa + +-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html +================================ tests coverage ================================ +_______________ coverage: platform linux, python 3.13.13-final-0 _______________ + +Name Stmts Miss Cover +-------------------------------------------------- +app/__init__.py 1 0 100% +app/api/__init__.py 0 0 100% +app/api/admin_settings.py 114 0 100% +app/api/auth.py 52 0 100% +app/api/chat.py 292 1 99% +app/api/chat_images.py 52 0 100% +app/api/chats.py 111 0 100% +app/api/config.py 13 0 100% +app/api/doc_drafts.py 99 0 100% +app/api/docs.py 239 1 99% +app/api/git_sources.py 259 0 100% +app/api/health.py 10 0 100% +app/api/steering.py 42 0 100% +app/api/suggestions.py 33 0 100% +app/api/sync.py 204 0 100% +app/api/tokens.py 40 0 100% +app/api/ui_settings.py 55 0 100% +app/config.py 304 0 100% +app/core/__init__.py 0 0 100% +app/core/auth.py 45 0 100% +app/core/caching.py 124 0 100% +app/core/compression.py 59 0 100% +app/core/debugging.py 29 2 93% +app/core/docs_push.py 43 0 100% +app/core/errors.py 5 0 100% +app/core/http_cache.py 22 0 100% +app/core/logging.py 13 0 100% +app/core/rate_limit.py 44 0 100% +app/core/response_budget.py 63 0 100% +app/core/security_headers.py 20 0 100% +app/core/settings_catalog.py 18 0 100% +app/core/settings_migrate.py 32 0 100% +app/core/settings_overlay.py 111 0 100% +app/core/theming.py 40 0 100% +app/core/tokens.py 44 0 100% +app/db.py 22 0 100% +app/main.py 105 0 100% +app/models.py 141 0 100% +app/rag/__init__.py 0 0 100% +app/rag/agent.py 494 1 99% +app/rag/archive_upload.py 134 0 100% +app/rag/chunker.py 220 4 98% +app/rag/doc_dates.py 18 0 100% +app/rag/folder_summaries.py 134 0 100% +app/rag/git_sources.py 77 0 100% +app/rag/importer.py 855 4 99% +app/rag/llm.py 253 1 99% +app/rag/md_images.py 63 0 100% +app/rag/overview.py 78 0 100% +app/rag/pdf.py 192 0 100% +app/rag/prompts.py 162 0 100% +app/rag/retriever.py 252 3 99% +app/rag/scaffolding.py 55 0 100% +app/rag/source_removal.py 41 0 100% +app/rag/sources_meta.py 16 0 100% +app/rag/suggestions.py 27 0 100% +app/rag/summarizer.py 58 0 100% +app/rag/svg_text.py 111 0 100% +app/rag/verbatim.py 160 0 100% +app/schemas.py 549 0 100% +-------------------------------------------------- +TOTAL 6849 17 99% +coverage gate: app/ 99% (>90%) OK +All checks passed! +0 errors, 0 warnings, 0 informations +WARNING: there is a new pyright version available (v1.1.411 -> v1.1.414). +Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest` + +validation OK diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.err b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.err new file mode 100644 index 0000000..e69de29 diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md new file mode 100644 index 0000000..59d0649 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md @@ -0,0 +1 @@ +I'll start by reading the project context files. diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md.status b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md.status new file mode 100644 index 0000000..9766475 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.md.status @@ -0,0 +1 @@ +ok diff --git a/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.validate b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.validate new file mode 100644 index 0000000..1e5a494 --- /dev/null +++ b/.agents/reports/183_sync_passive_delete_cascade/183_sync_passive_delete_cascade__03_e2e_prune_cascade.a1.validate @@ -0,0 +1,143 @@ +........................................................................ [ 1%] +........................................................................ [ 3%] +........................................................................ [ 4%] +........................................................................ [ 6%] +........................................................................ [ 8%] +........................................................................ [ 9%] +........................................................................ [ 11%] +........................................................................ [ 12%] +........................................................................ [ 14%] +........................................................................ [ 16%] +........................................................................ [ 17%] +........................................................................ [ 19%] +........................................................................ [ 20%] +........................................................................ [ 22%] +........................................................................ [ 24%] +........................................................................ [ 25%] +........................................................................ [ 27%] +........................................................................ [ 28%] +........................................................................ [ 30%] +........................................................................ [ 32%] +........................................................................ [ 33%] +........................................................................ [ 35%] +........................................................................ [ 36%] +........................................................................ [ 38%] +........................................................................ [ 40%] +........................................................................ [ 41%] +........................................................................ [ 43%] +........................................................................ [ 44%] +........................................................................ [ 46%] +........................................................................ [ 48%] +........................................................................ [ 49%] +........................................................................ [ 51%] +........................................................................ [ 52%] +........................................................................ [ 54%] +........................................................................ [ 56%] +........................................................................ [ 57%] +........................................................................ [ 59%] +........................................................................ [ 60%] +........................................................................ [ 62%] +........................................................................ [ 64%] +........................................................................ [ 65%] +........................................................................ [ 67%] +........................................................................ [ 68%] +........................................................................ [ 70%] +........................................................................ [ 72%] +........................................................................ [ 73%] +........................................................................ [ 75%] +........................................................................ [ 77%] +........................................................................ [ 78%] +........................................................................ [ 80%] +........................................................................ [ 81%] +........................................................................ [ 83%] +........................................................................ [ 85%] +........................................................................ [ 86%] +........................................................................ [ 88%] +........................................................................ [ 89%] +........................................................................ [ 91%] +........................................................................ [ 93%] +........................................................................ [ 94%] +........................................................................ [ 96%] +........................................................................ [ 97%] +........................................................................ [ 99%] +........................ [100%] +=============================== warnings summary =============================== +.venv/lib/python3.13/site-packages/fastapi/testclient.py:1 + /var/home/ducoterra/Projects/Personal/brain_of_reese/.venv/lib/python3.13/site-packages/fastapi/testclient.py:1: StarletteDeprecationWarning: Using `httpx` with `starlette.testclient` is deprecated; install `httpx2` instead. + from starlette.testclient import TestClient as TestClient # noqa + +-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html +================================ tests coverage ================================ +_______________ coverage: platform linux, python 3.13.13-final-0 _______________ + +Name Stmts Miss Cover +-------------------------------------------------- +app/__init__.py 1 0 100% +app/api/__init__.py 0 0 100% +app/api/admin_settings.py 114 0 100% +app/api/auth.py 52 0 100% +app/api/chat.py 292 1 99% +app/api/chat_images.py 52 0 100% +app/api/chats.py 111 0 100% +app/api/config.py 13 0 100% +app/api/doc_drafts.py 99 0 100% +app/api/docs.py 239 1 99% +app/api/git_sources.py 259 0 100% +app/api/health.py 10 0 100% +app/api/steering.py 42 0 100% +app/api/suggestions.py 33 0 100% +app/api/sync.py 204 0 100% +app/api/tokens.py 40 0 100% +app/api/ui_settings.py 55 0 100% +app/config.py 304 0 100% +app/core/__init__.py 0 0 100% +app/core/auth.py 45 0 100% +app/core/caching.py 124 0 100% +app/core/compression.py 59 0 100% +app/core/debugging.py 29 2 93% +app/core/docs_push.py 43 0 100% +app/core/errors.py 5 0 100% +app/core/http_cache.py 22 0 100% +app/core/logging.py 13 0 100% +app/core/rate_limit.py 44 0 100% +app/core/response_budget.py 63 0 100% +app/core/security_headers.py 20 0 100% +app/core/settings_catalog.py 18 0 100% +app/core/settings_migrate.py 32 0 100% +app/core/settings_overlay.py 111 0 100% +app/core/theming.py 40 0 100% +app/core/tokens.py 44 0 100% +app/db.py 22 0 100% +app/main.py 105 0 100% +app/models.py 141 0 100% +app/rag/__init__.py 0 0 100% +app/rag/agent.py 494 1 99% +app/rag/archive_upload.py 134 0 100% +app/rag/chunker.py 220 4 98% +app/rag/doc_dates.py 18 0 100% +app/rag/folder_summaries.py 134 0 100% +app/rag/git_sources.py 77 0 100% +app/rag/importer.py 855 4 99% +app/rag/llm.py 253 1 99% +app/rag/md_images.py 63 0 100% +app/rag/overview.py 78 0 100% +app/rag/pdf.py 192 0 100% +app/rag/prompts.py 162 0 100% +app/rag/retriever.py 252 3 99% +app/rag/scaffolding.py 55 0 100% +app/rag/source_removal.py 41 0 100% +app/rag/sources_meta.py 16 0 100% +app/rag/suggestions.py 27 0 100% +app/rag/summarizer.py 58 0 100% +app/rag/svg_text.py 111 0 100% +app/rag/verbatim.py 160 0 100% +app/schemas.py 549 0 100% +-------------------------------------------------- +TOTAL 6849 17 99% +coverage gate: app/ 99% (>90%) OK +All checks passed! +0 errors, 0 warnings, 0 informations +WARNING: there is a new pyright version available (v1.1.411 -> v1.1.414). +Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest` + +validation OK diff --git a/AGENTS.md b/AGENTS.md index 0ffeb2f..29c3105 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,6 +116,23 @@ await; the worker thread itself runs to completion (threads are not interruptible) while the loop keeps serving — pinned by the same E2E module's cancel-during-clone test. +12. **Never hydrate what you delete:** `Document.chunks` is + `passive_deletes=True` (phase 183), so deleting a document removes its + chunk rows **in the database** (`chunks_document_id_fkey` is + `ON DELETE CASCADE`, alembic 0001) and a mass prune costs + O(documents). The importer's prune and the source-removal endpoint both + rely on it. Do not remove the keyword, and do not add a delete path + that SELECTs `chunks` (or a `Chunk.embedding`) on its way to deleting + them — one hydrated 768-dim embedding is ~24.8 KiB of Python, which is + how the 2026-10-15 sync's 3,008-document prune reached 4 GB and locked + up the VM. Pinned by + `tests/integration/test_prune_cascade_statements.py` (statement count, + zero orphan chunk rows, and a live-FK drift guard that fails the suite + if the cascade clause ever disappears — because then + `passive_deletes=True` would silently orphan rows). The + `doc.chunks = [...]` **re-embed** path is the deliberate, documented + exception (collection replacement legitimately needs the loaded + collection; changed files only). ## Quick reference ```bash diff --git a/README.md b/README.md index 177365e..0a46e1d 100644 --- a/README.md +++ b/README.md @@ -272,7 +272,8 @@ just looked at costs a bodiless 304 (see *Caching & deploys*). The admin-managed source registry: add or remove git repositories, upload source archives (`.tar`, `.tar.gz`, `.tgz`, `.zip`), and register local directories — all in one table with a `kind` discriminator. No `.env` -editing, no restart. The **Sync sources** button clones/pulls/walks every +editing, no restart. The **Sync sources** button — which lives on the +**RAG** page (`/sources.html`), not here — clones/pulls/walks every source and re-imports in one click. ### Tuning (`/tuning.html`) @@ -446,10 +447,17 @@ uv run python -m scripts.import_docs --source ~/SomeOtherDocs With **git-based sources** each run first pulls the latest commits of your repos, so this same command is the whole update loop: *commit → re-run*. -Then check the **Sources** page (`http://localhost:8000/sources.html`): +Then check the **RAG** page — the knowledge-base catalog at +`http://localhost:8000/sources.html` (the navbar labels it **RAG**; the +separate **Sources** page, `/git-sources.html`, is the source registry): the *documents* / *chunks* counters and *last indexed* timestamp should reflect the new files. +- `--prune` removes a pruned file **completely** — its `documents` row and + its `chunks` rows (the embeddings) go together, the latter through the + database's own `ON DELETE CASCADE`, so the cost of a mass prune is + bounded by documents, not chunks (see *A mass prune costs documents, + not chunks* under [Sync from the UI](#sync-from-the-ui)). - The import prints one line per file and ends with a greppable summary, so it is safe to run from cron or after every commit. - Indexed formats: **md, markdown, txt, yaml, yml, json, py**, the Podman @@ -539,9 +547,9 @@ BOR_SOURCES_DIR=~/bor-sources # default; each repo lands in // Upload a `.tar`, `.tar.gz`, `.tgz`, or `.zip` archive to make it a source. The form on the Git sources page — and the `POST /api/git-sources/upload` route behind it — accepts archives and unpacks + registers them (no scan — -the upload indexes nothing; the **Sync sources** button on the RAG page -performs the scan, which leaves room to edit the new source's ignore list -first). +the upload indexes nothing; the **Sync sources** button on the **RAG** +page (`/sources.html`) performs the scan, which leaves room to edit the +new source's ignore list first). - Re-uploading the same filename replaces the source **in place** (no second folder, no duplicate row). @@ -572,8 +580,8 @@ discriminator: `git` | `local`), **one admin page**, and **one Sync button**. ### Sync from the UI -The **Sync sources** button on the **Sources** page (admin only) runs the -whole git-source refresh in one click: +The **Sync sources** button on the **RAG** page (`/sources.html`, admin +only) runs the whole source refresh in one click: 1. **clone/pull + walk** every configured source — git repos and local directories, including uploaded archives. @@ -625,6 +633,24 @@ whole git-source refresh in one click: above all the 2 s `GET /api/sync/status` poll that watches the run. Before this a big first clone stalled the loop thread and the entire app looked dead until git returned. +- **A mass prune costs documents, not chunks** (phase 183). A pruned file + leaves the index completely — its `documents` row AND its `chunks` rows + (every embedding) — but the chunk rows go **in the database**, through + `chunks_document_id_fkey`'s `ON DELETE CASCADE` (alembic 0001), because + `Document.chunks` is declared `passive_deletes=True`. So a prune of 3,000 + documents issues batched `DELETE FROM documents` statements and **zero** + `SELECT`s of the 768-dim vectors it is about to remove: the memory a + prune costs is bounded by the *document* count, not by chunks × + ~24.8 KiB of hydrated embedding. Before this, `session.delete(doc)` + loaded every child collection first, and the 2026-10-15 sync's + 3,008-document prune materialised ~72k embeddings (~720 MB) on a 3,904 + MB VM with no swap. The same holds for removing a source on the + **Git sources** page (the other mass-delete surface). Pinned by + `tests/integration/test_prune_cascade_statements.py` — the statement + count, the zero-orphan-chunk-rows contract, and a live-FK drift guard + that fails the suite if the cascade clause ever disappears; the owner + flow itself (delete upstream → re-sync → exactly those documents and + chunks gone) is pinned by `tests/e2e/test_prune_cascade_cleanup.py`. - **Cancel**: while a run is in flight, a **Cancel** button appears next to the sync button; clicking it posts `POST /api/sync/cancel` (admin only) and registers a cooperative cancel (`202`) — the run @@ -1610,18 +1636,22 @@ DEBUGPY=1 uv run uvicorn app.main:app # → log line: debugpy: remote debugging ENABLED, listening on 0.0.0.0:5678 ``` -Attach from VS Code (`.vscode/launch.json`): +Attach from VS Code — the repo ships `.vscode/launch.json` with the +matching configuration (`"Python Debugger: Remote Attach`), mapped for a +**local** dev run: ```json { - "name": "Attach to Brain of Reese", + "name": "Python Debugger: Remote Attach", "type": "debugpy", "request": "attach", "connect": { "host": "localhost", "port": 5678 }, "pathMappings": [ - { "localRoot": "${workspaceFolder}", "remoteRoot": "/app" } + { "localRoot": "${workspaceFolder}", "remoteRoot": "." } ] } ``` +Attaching to a run **inside the container** instead needs the container's +own root: `"remoteRoot": "/app"` (that is where the image puts the code). The port is non-blocking and attach-on-demand: the app keeps running normally until you attach. Override the port with `DEBUGPY_PORT`. @@ -1654,12 +1684,18 @@ uv run pytest tests/e2e/test_chat_rag.py -v --no-cov # ...one file per phase in tests/e2e/ (the stories live in # .agents/user_stories/, see .agents/phases/todo/). # -# Isolation is REQUIRED, not a style preference: every suite shares one -# Postgres and one pair of fixed ports (the app on 8123, the mock aipi on -# 8901 — overridable with E2E_APP_PORT / E2E_MOCK_PORT), and each file -# truncates + reseeds the KB for its own fixture. Running the whole -# `tests/e2e/` directory in a single pytest session interleaves those -# fixtures and fails tests that pass file by file. +# Isolation is REQUIRED, not a style preference: every suite shares ONE +# Postgres, one mock aipi port (8901 — E2E_MOCK_PORT) and the KB it seeds, +# and each file truncates + reseeds the KB for its own fixture. Running +# the whole `tests/e2e/` directory in a single pytest session interleaves +# those fixtures and fails tests that pass file by file. +# +# Ports: the shared session app (tests/e2e/conftest.py) binds 8123 +# (E2E_APP_PORT). A suite that needs its OWN app env boots a per-module +# uvicorn on its own port (phase 79) — e.g. E2E_APP_PORT_PRUNECASCADE for +# tests/e2e/test_prune_cascade_cleanup.py — because a second uvicorn on +# the session port dies on bind and the suite would drive the wrong +# server. So "isolation" is about the shared DATABASE, not about ports. # To sweep every suite, loop over the files (one pytest process each): for f in tests/e2e/test_*.py; do uv run pytest "$f" --no-cov -q || echo "FAIL $f"; done diff --git a/app/api/git_sources.py b/app/api/git_sources.py index 157ca5f..faf33db 100644 --- a/app/api/git_sources.py +++ b/app/api/git_sources.py @@ -860,8 +860,11 @@ async def delete_git_source( """Remove a stored source — **totally** (phase 69); 404 unknown id. Total removal, one action: the row, every indexed document of the - source (chunks + embeddings via the ``all, delete-orphan`` cascade - — ``app.models``), and, for app-managed sources, the on-disk dir + source (chunks + embeddings dropped **in the database** by the + ``chunks_document_id_fkey`` ``ON DELETE CASCADE`` — phase 183, + ``Document.chunks`` is ``passive_deletes=True``, so the endpoint + never loads a chunk row or its vector), and, for app-managed + sources, the on-disk dir (the git checkout or the unpacked upload folder). Foreign local directories (the owner's own) are never touched on disk — their row + index entries still go. diff --git a/app/models.py b/app/models.py index c37917c..a08092b 100644 --- a/app/models.py +++ b/app/models.py @@ -225,7 +225,17 @@ class Document(Base): page_strip_mode: Mapped[str | None] = mapped_column(String(16), default=None) chunks: Mapped[list[Chunk]] = relationship( - back_populates="document", cascade="all, delete-orphan" + back_populates="document", + cascade="all, delete-orphan", + # Phase 183: deleting a Document removes its chunks IN THE DATABASE + # (chunks_document_id_fkey is ON DELETE CASCADE — alembic 0001 L54, + # pinned live by tests/integration/test_prune_cascade_statements.py). + # WITHOUT this keyword the ORM loads every child before deleting it, + # which hydrates every 768-dim embedding (~24.8 KiB of Python each): + # a 3,008-document prune cost ~720 MB and took the 2026-10-15 VM + # (3,904 MB, no swap) down. Do not remove this without re-checking + # the FK — the drift guard fails the suite if the cascade is gone. + passive_deletes=True, ) diff --git a/app/rag/importer.py b/app/rag/importer.py index 6c6c5dd..59675c4 100644 --- a/app/rag/importer.py +++ b/app/rag/importer.py @@ -3902,6 +3902,23 @@ def _prune( image copy — no orphans when a page-qualifying PDF (its parent or a standalone doc) leaves the source. Callers passing nothing keep the pre-phase copy-cleanup contract. + + Phase 183 — why the ``session.delete`` below costs O(documents). + ``Document.chunks`` declares ``passive_deletes=True``, so the chunk + rows go **in the database** (``chunks_document_id_fkey`` is + ``ON DELETE CASCADE`` — alembic 0001 L54): this walk never SELECTs a + chunk row, never hydrates a 768-dim embedding, and a mass prune costs + documents instead of chunks × 24.8 KiB (the 2026-10-15 incident: a + 3,008-doc prune materialised ~720 MB to delete 72k rows and locked up + a 3,904 MB VM with no swap). The ``changed_paths`` feed and the + image/page copy cleanup above are unchanged, and they only ever read + ``doc.image_path`` / ``doc.page_count`` / ``doc.source`` / ``doc.path`` + — never ``doc.chunks``. The **re-embed** path is the deliberate + exception (LOCKED A2): :func:`_replace_content_chunks` reassigns + ``doc.chunks = [...]``, which is collection REPLACEMENT (the ORM must + know which old rows to orphan) and legitimately needs the loaded + collection — ``passive_deletes`` does not cover it, and CHANGED files + are the only ones that walk it. """ if not source_names: return 0 diff --git a/tests/e2e/test_prune_cascade_cleanup.py b/tests/e2e/test_prune_cascade_cleanup.py new file mode 100644 index 0000000..69d3d8f --- /dev/null +++ b/tests/e2e/test_prune_cascade_cleanup.py @@ -0,0 +1,667 @@ +"""Phase 183 E2E (Playwright, mock-only): a mass prune leaves nothing behind. + +Source: owner incident 2026-09-15 → 2026-10-15 — the brain-of-code sync +pruned **3,008 litellm docs** and the app hit 4 GB of RAM, locking up a +3,904 MB VM (no swap, sharing RAM with a 3.1 GB Postgres) hard enough that +boot −1 has no shutdown record at all. The cause was NOT the delete itself: +``Document.chunks`` carried ``cascade="all, delete-orphan"`` with **no +``passive_deletes``**, so every ``session.delete(doc)`` first SELECTed the +document's whole chunk collection — hydrating every 768-dim +``chunks.embedding`` (~24.8 KiB of Python each vs 3 KiB on disk) purely to +be thrown away, and nothing was released before the single commit +(``app/db.py`` builds sessions with ``expire_on_commit=False``). + +The fix under test: ``Document.chunks`` is now ``passive_deletes=True``, so +the rows go **in the database** through ``chunks_document_id_fkey``'s +``ON DELETE CASCADE`` (alembic 0001 L54) and Python never sees them. That +is a *display-of-work* change with exactly one owner-visible contract: the +prune must still remove **every chunk row of every pruned document** — zero +orphans — while changing nothing else (the counts line, the KB tree, the +surviving RAG). This module pins that contract end to end, through the +pipeline the owner actually drives (the Sources page Sync button) and +through the admin's total source removal. + +Run in isolation (DB must be up: ``podman compose up -d db``): + + uv run pytest tests/e2e/test_prune_cascade_cleanup.py -v --no-cov + +MOCK-ONLY suite: ``E2E_REAL_LLM=1`` is skipped — the gate is the deterministic +prune count + the exact orphan-row query, and test 2's grounding needs the +mock-calibrated relevance thresholds (the conftest convention). + +Per-module app env (the ``test_local_directory_sources.py`` / +``test_force_push_resync.py`` module-app pattern): this module's app boots +on its OWN port (the conftest session app owns 8123 in a combined run; a +same-port second uvicorn dies on bind and would drive the wrong server), +against the mock LLM, with a dedicated ``BOR_SOURCES_DIR`` / +``BOR_PDF_PAGE_DIR`` under ``tmp_path_factory`` and ``BOR_GIT_SOURCES`` +forced empty (the owner's local gitignored ``.env`` must never leak a real +source into the run — the registry rows this suite creates through the +authenticated API are the only sources). The fixture KB is a plain +(non-git) host directory per test under ``tmp_path`` — the app server runs +on the same host, so the path is visible to it — with the stable basename +:data:`SOURCE_NAME` (``kind='local'`` rows index under +``Path(path).name``, ``app/rag/source_removal.py``) so the SQL assertions +name one source whatever the pytest temp root is. + +Isolation: the shared E2E Postgres is truncated BEFORE **and** AFTER every +test (``chunks, documents, query_log, kb_overview, git_sources, +sources_meta, folder_summaries``) — a leftover registry row would flip a +sync onto another suite's source, and a leftover document under this +suite's deterministic source name would survive the prune and falsify the +count assertions. + +Test → observable mapping (Playwright Mapping Rule): +1. ``test_deleted_files_prune_without_orphans`` — sync 9 files (the counts + line reports "9 added"), delete 6 of them upstream, re-sync: the counts + line reports "0 added · 3 unchanged · 6 pruned", the 6 documents are + gone from the KB (the tree row is absent, ``GET /api/documents/content`` + 404s the pair, ``GET /api/docs`` holds exactly the 3 survivors), the + pruned documents' **chunk rows are all gone** (the test's own SQL — the + no-orphan half of the phase-183 D4 contract) and the surviving chunk + total is exactly the sum of the 3 survivors' chunks. +2. ``test_surviving_kb_still_answers_after_a_partial_prune`` — the same + pruned state answers a chat question about a SURVIVING document: the + turn settles with a real answer (no error state, no empty bubble, the + composer recovered) and the durable ``query_log`` row is NOT deflected + and records the surviving document — a partial prune never damages the + RAG. +3. ``test_source_removal_prunes_every_document_of_the_source`` — the admin + path (``DELETE /api/git-sources/{id}``, ``app/api/git_sources.py``): the + second mass-delete surface ``passive_deletes=True`` covers. Every + document AND every chunk row of the removed source is gone (the same + orphan query), the count matches what was indexed there, the row is + gone, and the OTHER registered source's documents and chunks are + untouched. +""" +from __future__ import annotations + +import os +import re +import subprocess +import sys +import time +import uuid +from collections.abc import Iterator +from datetime import datetime +from pathlib import Path +from typing import Any + +import pytest +from playwright.sync_api import Page, expect +from sqlalchemy import func, select, text +from sqlalchemy.orm import Session + +from app.db import SessionLocal +from app.models import Chunk, Document, QueryLog +from e2e.auth_helpers import login +from e2e.conftest import ( + ADMIN_PASSWORD, + SESSION_SECRET, + USE_REAL_LLM, + _wait_http, +) + +REPO = Path(__file__).resolve().parents[2] + +# Phase 79 (task 04, full inventory): the conftest session app owns its +# port in a combined run — this module app binds its own port instead. +# Env-overridable. +APP_PORT = int(os.environ.get("E2E_APP_PORT_PRUNECASCADE", "8199")) +APP_URL = f"http://127.0.0.1:{APP_PORT}" + +SOURCES_URL = "/sources.html" + +#: The fixture directory's basename — the ``source`` the importer records +#: for a ``kind='local'`` row (``Path(path).name``), so it is the same +#: string in the DB whatever ``tmp_path`` this test got. +SOURCE_NAME = "prune-cascade" +KEEPER_NAME = "prune-keeper" + +#: The fixture layout: 9 importable files across one folder, 6 of which +#: the test deletes upstream (the incident's shape — a bulk deletion in a +#: synced source — scaled to what a test sync walks in seconds). +KEEP_PATHS = [f"notes/keep-{n}.md" for n in (1, 2, 3)] +GONE_PATHS = [f"notes/gone-{n}.md" for n in range(1, 7)] +ALL_PATHS = KEEP_PATHS + GONE_PATHS + +#: The sentinel inside the FIRST survivor — test 2's question is answerable +#: ONLY from that document, so a grounded, non-deflected turn proves the +#: surviving chunk rows are still retrievable after the prune. +SENTINEL_DOC = KEEP_PATHS[0] +SENTINEL = "warden-lighthouse-RESE-PRUNECASCADE-7f31" +QUESTION = "Which keeper tends the warden lighthouse beacon log?" + +#: The mock LLM's byte-stable grounded-answer marker (``mock_llm. +#: compose_answer``'s fallback branch) — its presence proves a real, +#: grounded answer streamed (the DEFLECT_MODE branch answers with +#: different, deterministic text). +MOCK_ANSWER_MARKER = "Deterministic mock answer for E2E" + +#: "Synced HH:MM" — the local-time last-result label (sources.js's +#: fmtSyncTime), any hour/minute. +SYNCED_LABEL = re.compile(r"Synced \d{1,2}:\d{2}") + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def app_server(mock_llm: int, tmp_path_factory: pytest.TempPathFactory) -> Iterator[str]: + """The real app under test — per-module env: the mock LLM, the + mock-calibrated gate (so test 2's grounding is deterministic), an + empty ``BOR_GIT_SOURCES`` fallback, and scratch + ``BOR_SOURCES_DIR`` / ``BOR_PDF_PAGE_DIR`` under + ``tmp_path_factory`` (nothing in this suite clones a repo or renders a + PDF — the dirs are set so the run never writes into the operator's + own). The session app is never started in this isolated run, so no + port clash.""" + if USE_REAL_LLM: + pytest.skip( + "mock-only suite — the grounding pin is the mock's byte-stable " + "answer on the mock-calibrated gate (E2E_REAL_LLM unsupported)" + ) + scratch = tmp_path_factory.mktemp("bor_prune_cascade") + env = dict(os.environ) + env.pop("DEBUGPY", None) + env["BOR_ENVIRONMENT"] = "e2e" + env["BOR_STATIC_DIR"] = str(REPO / "frontend") + env["BOR_LLM_BASE_URL"] = ( + "https://aipi.reeseapps.com/v1" + if USE_REAL_LLM + else f"http://127.0.0.1:{mock_llm}/v1" + ) + # Mock-calibrated gate (the conftest pattern, phase 112/113): the + # token-overlap mock embeddings score far below the production 0.62, + # and BOTH floors must stay <= the threshold or the app refuses to + # boot (app/config.py's typo guards). + env["BOR_RELEVANCE_THRESHOLD"] = "0.30" + env["BOR_LEXICAL_SUPPORT_FLOOR"] = "0.15" + env["BOR_SOURCE_USEFULNESS_FLOOR"] = "0.15" + env.setdefault( + "BOR_DATABASE_URL", + "postgresql+psycopg://reese:reese@localhost:5432/brain_of_reese", + ) + # Phase 16: admin auth must be set or create_app() refuses to boot. + env["BOR_ADMIN_PASSWORD"] = ADMIN_PASSWORD + env["BOR_SESSION_SECRET"] = SESSION_SECRET + # No env fallback rows: the registry this suite writes through the + # authenticated API is the only source list, so every sync's counts + # are exactly this test's doing. + env["BOR_GIT_SOURCES"] = "" + env["BOR_SOURCES_DIR"] = str(scratch / "checkouts") + env["BOR_PDF_PAGE_DIR"] = str(scratch / "pdf-pages") + proc = subprocess.Popen( + [ + sys.executable, "-m", "uvicorn", "app.main:app", + "--host", "127.0.0.1", "--port", str(APP_PORT), "--log-level", "warning", + ], + cwd=REPO, + env=env, + ) + try: + _wait_http(f"{APP_URL}/api/health") + yield APP_URL + finally: + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + + +@pytest.fixture(scope="module") +def app_url(app_server: str) -> str: + return app_server + + +def _truncate_all() -> None: + """Fresh registry + KB per test (the E2E isolation pattern, + ``test_source_removal_cleanup.py`` / ``test_force_push_resync.py``). + ``sources_meta`` (the KB generation counter) and + ``folder_summaries`` reset with the KB — the sync's overview/bump is an + upsert, and a leftover folder summary would ride the very + ``changed_paths`` feed the prune writes.""" + with SessionLocal() as db: + db.execute( + text( + "TRUNCATE chunks, documents, query_log, kb_overview, " + "git_sources, sources_meta, folder_summaries" + ) + ) + db.commit() + + +@pytest.fixture(autouse=True) +def _clean(db_ready: None) -> Iterator[None]: + _truncate_all() + yield + _truncate_all() + + +# --------------------------------------------------------------------------- +# The fixture source +# --------------------------------------------------------------------------- + + +def _doc_body(path: str) -> str: + """One small markdown file per fixture path. The FIRST survivor is the + ONLY file carrying the sentinel's words, so test 2's question resolves + to exactly one document (and the mock's token-overlap cosine — + ``sqrt(shared/total)`` — clears the mock-calibrated 0.30 gate only for + that one).""" + if path == SENTINEL_DOC: + return ( + "# Keeper log\n" + "\n" + "The keeper tends the warden lighthouse beacon log.\n" + f"Marker: {SENTINEL}\n" + ) + return ( + f"# {path}\n" + "\n" + f"One unrelated note from the {SOURCE_NAME} fixture source.\n" + ) + + +def _make_source_dir(root: Path, name: str, paths: list[str]) -> Path: + """A plain (non-git) directory of importable files — the shape the + owner's local sources have. Returns the registered root.""" + base = root / name + for rel in paths: + target = base / rel + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(_doc_body(rel), encoding="utf-8") + assert not (base / ".git").exists() + return base + + +# --------------------------------------------------------------------------- +# Helpers: the DB side of the contract (the phase-183 D4 assertions) +# --------------------------------------------------------------------------- + + +def _doc_ids(db: Session, source: str, paths: list[str]) -> list[str]: + """The indexed document ids of ``(source, path)``, sorted.""" + return sorted( + str(doc_id) + for (doc_id,) in db.execute( + select(Document.id).where( + Document.source == source, Document.path.in_(paths) + ) + ).all() + ) + + +def _chunk_count(db: Session, document_ids: list[str]) -> int: + """The chunk rows still belonging to ``document_ids`` — THE orphan + probe. Non-zero means the documents are gone and their embeddings (and + their FTS rows) stayed behind.""" + if not document_ids: + return 0 + total = db.scalar( + select(func.count()) + .select_from(Chunk) + .where(Chunk.document_id.in_([uuid.UUID(x) for x in document_ids])) + ) + return int(total or 0) + + +def _total_chunks(db: Session) -> int: + return int(db.scalar(select(func.count()).select_from(Chunk)) or 0) + + +def _chunk_counts_by_doc( + db: Session, source: str, paths: list[str] | None = None +) -> dict[str, int]: + """``path`` → chunk count, for the surviving-total assertion.""" + query = ( + select(Document.path, func.count(Chunk.id)) + .outerjoin(Chunk, Chunk.document_id == Document.id) + .where(Document.source == source) + .group_by(Document.path) + ) + if paths is not None: + query = query.where(Document.path.in_(paths)) + rows = db.execute(query).all() + return {str(path): int(count) for path, count in rows} + + +def _sources_in_db(db: Session) -> list[str]: + return sorted( + { + str(name) + for (name,) in db.execute( + select(Document.source).distinct() + ).all() + } + ) + + +# --------------------------------------------------------------------------- +# Helpers: the UI / API side +# --------------------------------------------------------------------------- + + +def _started_at(body: dict[str, Any]) -> float: + """The status body's ``started_at`` as a wall-clock epoch (the server + stamps it in UTC, this process runs on the same host — the + ``test_git_source_branch`` idiom).""" + started = body.get("started_at") + if not started: + return 0.0 + try: + return datetime.fromisoformat(str(started)).timestamp() + except ValueError: + return 0.0 + + +def _sync_from_sources_page(page: Page, app_url: str) -> dict[str, Any]: + """Click the Sources page **Sync sources** button and wait for the + run's TERMINAL state (the ``test_force_push_resync._wait_sync_state`` + pattern: the module app is shared across a test's two runs, so a + terminal is accepted only after a ``running`` tick was seen or its + ``started_at`` post-dates the click). Settles the UI on success + (``#sync-label`` → "Synced HH:MM" — the never-stale contract, §7.4).""" + login(page, app_url, next=SOURCES_URL) + expect(page.locator("#sign-out-btn")).to_be_visible(timeout=15_000) + btn = page.locator("#sync-btn") + expect(btn).to_be_visible() + clicked_at = time.time() + btn.click() + expect(btn).to_be_disabled() + expect(page.locator("#sync-label")).to_have_text("Syncing…") + + deadline = time.monotonic() + 90.0 + saw_running = False + body: dict[str, Any] = {} + while time.monotonic() < deadline: + r = page.request.get(f"{app_url}/api/sync/status") + assert r.status == 200, r.text + body = r.json() + state = body["state"] + if state == "running": + saw_running = True + elif state in ("success", "failed") and ( + saw_running or _started_at(body) >= clicked_at - 1.0 + ): + break + time.sleep(0.2) + else: + raise AssertionError(f"sync never reached a terminal state: {body}") + + assert body["state"] == "success", f"sync failed: {body}" + expect(btn).to_be_enabled(timeout=15_000) + expect(page.locator("#sync-label")).to_have_text(SYNCED_LABEL, timeout=15_000) + # The counts line is the owner-visible prune report (phase 150 — the + # single sync text surface, sources.js fmtSyncResult). + return body + + +def _expect_counts_line(page: Page, expected: str) -> None: + """The Sources page's counts line reads exactly ``expected`` — + "9 added" / "0 added · 3 unchanged · 6 pruned".""" + expect(page.locator("#sync-progress-text")).to_have_text(expected, timeout=15_000) + + +def _add_local_source(page: Page, app_url: str, path: Path) -> str: + """Register a local directory through the authenticated API (the + phase-49 shape — the page's own form for this is gone; the admin + cookie rides ``page.request``). Returns the row id.""" + r = page.request.post( + f"{app_url}/api/git-sources", + data={"kind": "local", "path": str(path)}, + ) + assert r.status == 201, f"expected 201 for {path}: {r.status} {r.text}" + row_id = str(r.json()["id"]) + assert row_id + return row_id + + +def _api_docs(page: Page, app_url: str) -> list[dict[str, Any]]: + """``GET /api/docs`` as the signed-in page (admin cookie rides along).""" + r = page.request.get(f"{app_url}/api/docs") + assert r.status == 200, r.text + return list(r.json()["documents"]) + + +def _api_doc_pairs(page: Page, app_url: str) -> list[tuple[str, str]]: + return sorted((d["source"], d["path"]) for d in _api_docs(page, app_url)) + + +def _api_sources(page: Page, app_url: str) -> list[str]: + """The distinct ``source`` labels the KB holds, read through the + signed-in API (the same view the owner gets on the Sources page).""" + return sorted({d["source"] for d in _api_docs(page, app_url)}) + + +def _drill_to_fixture_folder(page: Page) -> None: + """Open the KB catalogue down to the fixture's ``notes`` folder (the + drill-down tree is the phase-97 shape, the + ``test_force_push_resync._expect_fixture_row`` idiom).""" + crumb = page.locator("#kb-crumb") + if not crumb.is_hidden(): + page.click("#kb-crumb a.kb-crumb-link") + page.click(f'#folders-tbody a.folder-link:text-is("{SOURCE_NAME}")') + page.click('#folders-tbody a.folder-link:text-is("notes")') + + +# --------------------------------------------------------------------------- +# 1. Upstream deletion + re-sync: the prune is exact and leaves no orphans +# --------------------------------------------------------------------------- + + +def test_deleted_files_prune_without_orphans( + page: Page, app_url: str, db_ready: None, tmp_path: Path +) -> None: + """The incident's own shape, end to end: 9 files sync, 6 are deleted + upstream, the re-sync prunes exactly those 6 — through the Sources page + Sync button, with the counts line as the owner-visible report — and the + pruned documents' chunk rows are gone FROM THE DATABASE (the phase-183 + D4 no-orphan contract, asserted by this test's own SQL).""" + page.set_default_timeout(30_000) + source = _make_source_dir(tmp_path, SOURCE_NAME, ALL_PATHS) + + login(page, app_url, next=SOURCES_URL) + _add_local_source(page, app_url, source) + + # --- sync 1: everything is new ------------------------------------ + body = _sync_from_sources_page(page, app_url) + assert body["detail"]["added"] == len(ALL_PATHS), body + assert body["detail"]["pruned"] == 0, body + _expect_counts_line(page, f"{len(ALL_PATHS)} added") + assert _api_doc_pairs(page, app_url) == [ + (SOURCE_NAME, path) for path in sorted(ALL_PATHS) + ] + + # The subject of the prune, captured BEFORE it happens: the 6 doomed + # document ids + every survivor's chunk count. The orphan query below + # needs ids that no longer exist by the time it runs. + with SessionLocal() as db: + gone_ids = _doc_ids(db, SOURCE_NAME, GONE_PATHS) + keep_counts = _chunk_counts_by_doc(db, SOURCE_NAME, KEEP_PATHS) + assert _chunk_count(db, gone_ids) > 0, "the doomed docs must be embedded" + assert len(gone_ids) == len(GONE_PATHS), "the doomed documents must exist first" + assert all(keep_counts[p] > 0 for p in KEEP_PATHS), keep_counts + assert sum(keep_counts.values()) > 0 + + # --- the upstream deletion (the test owns the source dir) ---------- + for rel in GONE_PATHS: + (source / rel).unlink() + assert not any((source / rel).exists() for rel in GONE_PATHS) + + # --- sync 2: the prune --------------------------------------------- + body = _sync_from_sources_page(page, app_url) + assert body["detail"]["pruned"] == len(GONE_PATHS), body + assert body["detail"]["added"] == 0, body + assert body["detail"]["updated"] == 0, body + assert body["detail"]["unchanged"] == len(KEEP_PATHS), body + _expect_counts_line( + page, + f"0 added · {len(KEEP_PATHS)} unchanged · {len(GONE_PATHS)} pruned", + ) + + # The KB no longer serves the deleted files: the catalogue row is gone, + # the content route 404s the pair, and the total is exactly 3. + _drill_to_fixture_folder(page) + expect(page.locator("#docs-tbody tr", has_text=KEEP_PATHS[0])).to_have_count(1) + expect(page.locator("#docs-tbody tr", has_text=GONE_PATHS[0])).to_have_count(0) + r = page.request.get( + f"{app_url}/api/documents/content" + f"?source={SOURCE_NAME}&path={GONE_PATHS[0]}" + ) + assert r.status == 404, f"a pruned document must 404, got {r.status}: {r.text}" + docs = _api_docs(page, app_url) + assert len(docs) == len(KEEP_PATHS), docs + assert {d["path"] for d in docs} == set(KEEP_PATHS), docs + + # THE phase-183 assertion: the pruned documents left NO chunk rows + # behind (the FK cascade, not the ORM, removed them) and the surviving + # chunk total is exactly the survivors' own chunks — nothing orphaned, + # nothing collateral-damaged. + with SessionLocal() as db: + assert _chunk_count(db, gone_ids) == 0, ( + "pruned documents left orphan chunk rows — the FK cascade did " + "not run (Document.chunks' passive_deletes without ON DELETE " + "CASCADE)" + ) + assert _doc_ids(db, SOURCE_NAME, GONE_PATHS) == [] + after = _chunk_counts_by_doc(db, SOURCE_NAME, KEEP_PATHS) + assert after == keep_counts, ( + f"the surviving documents' chunks changed across the prune: {after}" + ) + assert _total_chunks(db) == sum(keep_counts.values()) + assert _sources_in_db(db) == [SOURCE_NAME] + + +# --------------------------------------------------------------------------- +# 2. The surviving KB still answers after a partial prune +# --------------------------------------------------------------------------- + + +def test_surviving_kb_still_answers_after_a_partial_prune( + page: Page, app_url: str, db_ready: None, tmp_path: Path +) -> None: + """The prune's other half: what stays must still work. With the 6 files + pruned, the chat answers a question that only the surviving + :data:`SENTINEL_DOC` can answer — a real streamed answer (no empty + bubble, no error state, the composer recovered) whose durable + ``query_log`` row is NOT deflected and names the surviving document.""" + page.set_default_timeout(30_000) + source = _make_source_dir(tmp_path, SOURCE_NAME, ALL_PATHS) + + login(page, app_url, next=SOURCES_URL) + _add_local_source(page, app_url, source) + _sync_from_sources_page(page, app_url) + for rel in GONE_PATHS: + (source / rel).unlink() + body = _sync_from_sources_page(page, app_url) + assert body["detail"]["pruned"] == len(GONE_PATHS), body + + # The chat turn, through the real shell. + page.goto(f"{app_url}/") + expect(page.locator("#message-input")).to_be_visible(timeout=30_000) + # A healthy KB: the offline banner stays hidden (the prune did not + # empty or break the index the page reports on). + expect(page.locator("#kb-banner")).to_be_hidden() + page.fill("#message-input", QUESTION) + page.click("#send-btn") + expect(page.locator(".msg.user .bubble").last).to_contain_text(QUESTION) + bubble = page.locator(".msg.brain .bubble").last + # Never an empty answer bubble, and the grounded (not deflected) mock + # answer streamed. + expect(bubble).not_to_have_text("", timeout=30_000) + expect(bubble).to_contain_text(MOCK_ANSWER_MARKER, timeout=30_000) + # Never stale: the composer recovers (§7.4), and no error state landed. + expect(page.locator("#send-btn")).to_be_enabled(timeout=30_000) + expect(page.locator("#send-label")).to_have_text("Send", timeout=30_000) + + # The durable record: grounded on the SURVIVING document. + with SessionLocal() as db: + rows = db.scalars(select(QueryLog)).all() + assert len(rows) == 1, [r.question for r in rows] + row = rows[0] + assert row.question == QUESTION + assert row.deflected is False, ( + f"a partial prune broke retrieval — the turn deflected " + f"(top_score={row.top_score}, fts_hits={row.fts_hits})" + ) + assert f"{SOURCE_NAME}/{SENTINEL_DOC}" in row.sources, row.sources + assert all(GONE_PATHS[i] not in row.sources for i in range(len(GONE_PATHS))), ( + f"a pruned document still grounds a turn: {row.sources}" + ) + + +# --------------------------------------------------------------------------- +# 3. Total source removal: every document AND chunk row of the source +# --------------------------------------------------------------------------- + + +def test_source_removal_prunes_every_document_of_the_source( + page: Page, app_url: str, db_ready: None, tmp_path: Path +) -> None: + """The second mass-delete surface the keyword covers + (``delete_git_source``, ``app/api/git_sources.py`` — the endpoint that + took 47 s for 2,303 immich docs on the incident day): removing a + registered source removes every one of its documents and chunks with + zero orphans, and the OTHER registered source stays byte-identical. + + The endpoint's contract is ``204`` with **no body** (phase 69), so the + prune count is pinned against what the KB held immediately before the + DELETE rather than a response field.""" + page.set_default_timeout(30_000) + doomed = _make_source_dir(tmp_path, SOURCE_NAME, ALL_PATHS) + keeper = _make_source_dir( + tmp_path, KEEPER_NAME, ["notes/keeper-1.md", "notes/keeper-2.md"] + ) + + login(page, app_url, next=SOURCES_URL) + doomed_id = _add_local_source(page, app_url, doomed) + _add_local_source(page, app_url, keeper) + _sync_from_sources_page(page, app_url) + assert _api_sources(page, app_url) == [SOURCE_NAME, KEEPER_NAME] + + with SessionLocal() as db: + doomed_doc_ids = _doc_ids(db, SOURCE_NAME, ALL_PATHS) + doomed_chunks = _chunk_count(db, doomed_doc_ids) + keeper_before = _chunk_counts_by_doc(db, KEEPER_NAME) + assert len(doomed_doc_ids) == len(ALL_PATHS) + assert doomed_chunks > 0, "the fixture must embed chunks to prune" + + r = page.request.delete(f"{app_url}/api/git-sources/{doomed_id}") + assert r.status == 204, f"expected 204, got {r.status}: {r.text}" + + # Every document of the source is gone and left NO chunk rows; the + # remaining source is untouched, chunks included. + with SessionLocal() as db: + assert _chunk_count(db, doomed_doc_ids) == 0, ( + "source removal left orphan chunk rows — the FK cascade did " + "not run (Document.chunks' passive_deletes without ON DELETE " + "CASCADE)" + ) + assert ( + db.scalar( + select(func.count()) + .select_from(Document) + .where(Document.source == SOURCE_NAME) + ) + or 0 + ) == 0 + assert _sources_in_db(db) == [KEEPER_NAME] + assert _chunk_counts_by_doc(db, KEEPER_NAME) == keeper_before + assert _total_chunks(db) == sum(keeper_before.values()) + + # The registry row is gone too, and the API agrees with the DB. + r = page.request.get(f"{app_url}/api/git-sources") + assert r.status == 200, r.text + rows = r.json()["sources"] + assert doomed_id not in [row["id"] for row in rows], rows + assert len(rows) == 1 and rows[0]["path"] == str(keeper), rows + assert _api_doc_pairs(page, app_url) == [ + (KEEPER_NAME, f"notes/keeper-{n}.md") for n in (1, 2) + ] + # The doomed files stay on disk — a foreign local directory is never + # touched (the phase-69 locked decision, unaffected by phase 183). + assert doomed.is_dir() and all((doomed / p).is_file() for p in KEEP_PATHS) diff --git a/tests/integration/test_prune_cascade_statements.py b/tests/integration/test_prune_cascade_statements.py new file mode 100644 index 0000000..7903225 --- /dev/null +++ b/tests/integration/test_prune_cascade_statements.py @@ -0,0 +1,477 @@ +"""Integration: phase 183 — what a mass prune ACTUALLY loads (the probe). + +**VERDICT (task 01): CONFIRMED — one chunk SELECT per pruned document → +FIXED (task 02, ``passive_deletes=True``).** Pruning 200 documents (25 +chunks each, a real 768-dim ``chunks.embedding`` per chunk) through the +importer's own ``_prune`` used to issue **exactly 200** ``SELECT … FROM +chunks`` statements — one lazy child-collection load per +``session.delete(doc)`` — materialising every 768-dim embedding of every +document into Python on its way to the delete. Task 01 pinned that with +``EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 1`` GREEN before any code change +(the phase-179 task-01 "pin the defect with a probe first" precedent); +task 02 declared ``passive_deletes=True`` on +``Document.chunks`` and flipped the constant to ``0``. The same fixture +now prunes in **2 statements** (the prune's own ``SELECT documents`` plus +one batched ``DELETE FROM documents``), **0 chunk SELECTs**, **0 orphan +chunk rows** — the ``chunks_document_id_fkey`` ``ON DELETE CASCADE`` +drops the rows in the database, so Python never sees them. + +:func:`test_chunks_fk_cascades_at_database_level` is the other half of the +contract: ``passive_deletes`` is only correct while the FK really +cascades, so the live ``pg_constraint`` row is asserted here (a future +migration dropping ``ON DELETE CASCADE`` would otherwise let every prune +silently orphan chunk rows). + +**The incident (owner, 2026-10-15).** The brain-of-code sync ``started +repos=4`` at 11:19:23, reached ``_prune`` at 12:14:14, pruned **3,008 +litellm docs** — and the VM locked up so hard that boot −1 has no shutdown +record at all (4 GB of RAM). The app cgroup had already peaked at 3.3 GB +twice that same week. The box has 3,904 MB and **no swap**, and shares that +RAM with a 3.1 GB Postgres. 3,008 docs × ~24 chunks ≈ 72k rows whose +embeddings the prune pulled into Python to delete them. + +**Mechanism (pre-phase-183).** ``Document.chunks`` carried +``cascade="all, delete-orphan"`` and **no ``passive_deletes``** — so +``session.delete(doc)`` had to LOAD the child collection first (the ORM has +to know which children to orphan/delete), SELECTing every chunk row +*including* its ``Vector(768)`` column. Nothing was released afterwards: +``app/db.py:30`` builds sessions with ``expire_on_commit=False``, so the 72k +hydrated ``Chunk`` objects (and their float lists) stayed resident until the +single ``session.commit()`` at ``app/rag/importer.py`` returned — the +``passive_deletes=True`` of task 02 is what takes that walk out. + +**Measured unit costs.** + +* A loaded 768-dim ``Chunk.embedding`` costs **24.8 KiB of Python** (list + header + 768 float objects — pgvector hands back a ``list[float]``, never + a buffer) against **3 KiB on disk**: an 8× expansion per embedding. +* **~0.24 MB resident per document deleted** at 25 chunks/doc — measured + 2026-10-15 against the dev DB with production-shaped rows (400 docs × + 25 chunks, ~745-char chunk content): RSS grew linearly, +241 MB over + 400 deletes. + +**Measured BY THIS FILE (2026-10-15, the fixture below: 200 docs × 25 +chunks × 745 chars × a 768-float vector, the dev Postgres).** + +* PRE-fix (no ``passive_deletes``) statement capture + (``before_cursor_execute``, the fixture below): **203** statements for + the whole prune — **200 of them ``SELECT … FROM chunks``** (one per + pruned document, the load-bearing pin), plus 1 ``SELECT documents`` (the + prune's own walk) and the 2 ``executemany`` ``DELETE``s (``chunks``, then + ``documents``). +* PRE-fix ``VmRSS`` delta: **+165.6 MB for the 200-doc prune** + (≈ 0.83 MB/doc). Higher than the 0.24 MB/doc above because this fixture's + chunk content and vector are both fully hydrated and the session keeps + everything (``expire_on_commit=False``); the ORDER is the point, not the + figure. +* the same 200 deletes issued as plain ``DELETE FROM documents`` (i.e. what + the DB's own ``ON DELETE CASCADE`` does — the shape task 02 lands, + measured as a one-off while writing task 01): **0 chunk SELECTs, + +0.0 MB RSS**, and still **0 orphan chunk rows** — the ceiling below is + picked from that contrast, roughly one quarter of the pre-fix delta. +* POST-fix (``passive_deletes=True``, task 02, the same fixture and the + same capture): **2** statements — 1 ``SELECT documents`` + 1 batched + ``executemany`` ``DELETE FROM documents`` — **0 chunk SELECTs**, and the + ``VmRSS`` delta is **+0.2 MB** (run-to-run +0.1…0.6 MB — allocator noise, + two orders of magnitude under the 165.6 MB the hydrating walk cost, and + still 200× inside the 40 MB ceiling). Still **0 + orphan chunk rows**, which is what the D4 assertions inside + :func:`test_prune_never_selects_a_chunk_row` and the drift guard + :func:`test_chunks_fk_cascades_at_database_level` exist to keep true. + +**Why the memory test branches on the constant.** D1/LOCKED A4: the +statement count is the load-bearing, CI-safe pin; the RSS figure is +corroboration and must never flake a build on allocator noise. So +``test_prune_memory_delta_has_a_ceiling`` asserts the post-fix ceiling +(``< 40 MB``); its ``else`` branch, kept for the pre-fix state the constant +can be flipped back to, asserts the opposite (the delta lands WELL ABOVE +that ceiling) — which is how the ceiling was proven to have teeth before it +became the contract. + +**What this file does NOT touch:** the prune's batching (phase 184), the +fast-path column set (phase 185), +``expire_on_commit``, and the ``doc.chunks = [...]`` orphan-replacement +path inside ``_replace_content_chunks`` (``app/rag/importer.py:1033``) — +``passive_deletes`` does not cover collection reassignment and a re-embed +genuinely needs the loaded collection (phase 183 LOCKED A2). +""" +from __future__ import annotations + +import gc +import re +from collections.abc import Iterator +from pathlib import Path +from uuid import UUID + +import pytest +from sqlalchemy import event, func, select, text +from sqlalchemy.orm import Session + +from app.db import engine +from app.models import Chunk, Document +from app.rag.importer import ImportSummary, _prune + +#: The probe's own source name — nothing else in the KB uses it. +PROBE_SOURCE = "cascade-probe" + +#: The production-shaped fixture: 200 documents, 25 chunks each (the +#: incident pruned 3,008 docs × ~24 chunks; this is that walk scaled down +#: to what a test run can seed in seconds). +DOC_COUNT = 200 +CHUNKS_PER_DOC = 25 + +#: ~745 chars, the measured production chunk size (phase 183 baseline). +CHUNK_CONTENT = ("The homelab runs podman quadlets on the hnb host. " * 18)[:745] + +#: **0 post-phase-183 — the FK cascade does the work in the database; +#: 1 pre-phase-183 was the lazy child load** (one ``SELECT … FROM chunks`` +#: per deleted document, the waste this phase removed: 5,000 chunk rows and +#: 5,000 768-dim embeddings hydrated into Python purely to be deleted). +#: The DB's ``ON DELETE CASCADE`` (alembic 0001 L54, +#: ``chunks_document_id_fkey`` confdeltype='c' — pinned live by +#: :func:`test_chunks_fk_cascades_at_database_level`) deletes the rows +#: without Python ever seeing them. +EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC = 0 + +#: The POST-phase-183 memory ceiling for a 200-doc prune (~1/4 of the +#: measured pre-fix delta of 165.6 MB; the FK-cascade path measures +#: +0.2 MB, module docstring). +RSS_CEILING_MB = 40.0 + +#: The loose upper bound that still applies while the hydrating behaviour +#: is the expected one — a runaway allocator must never go unnoticed. +PRE_FIX_RSS_SANITY_CEILING_MB = 400.0 + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def clean_state(db: Session) -> Iterator[None]: + """``documents`` / ``chunks`` are global state: reset around every + test (the ``test_sync_api.py`` truncate pattern).""" + db.execute(text("TRUNCATE chunks, documents")) + db.commit() + yield + db.execute(text("TRUNCATE chunks, documents")) + db.commit() + + +@pytest.fixture(autouse=True) +def statement_capture() -> Iterator[list[str]]: + """Every SQL the engine executes for the duration of ONE test. + + Installed on the engine (the session under test binds to it) and + REMOVED in teardown, so no other test of the session ever sees the + listener (AGENTS: the suite stays isolated — verified by the full + ``uv run pytest`` run being green with this file in it). + """ + captured: list[str] = [] + + def _listener( + conn: object, # noqa: ARG001 + cursor: object, # noqa: ARG001 + statement: str, + parameters: object, # noqa: ARG001 + context: object, # noqa: ARG001 + executemany: bool, # noqa: ARG001 + ) -> None: + captured.append(statement) + + event.listen(engine, "before_cursor_execute", _listener) + try: + yield captured + finally: + event.remove(engine, "before_cursor_execute", _listener) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +#: ``SELECT … FROM chunks`` — the child-collection load. The word boundary +#: keeps ``documents`` (and its ``…_id`` aliases) out of the count while +#: still matching the real lazy load, whose SQL starts +#: ``SELECT chunks.id AS chunks_id, … FROM chunks WHERE chunks.document_id``. +_CHUNK_SELECT_RE = re.compile(r"\bfrom\s+chunks\b", re.IGNORECASE) + + +def chunk_selects(captured: list[str]) -> list[str]: + """The captured statements that SELECT the ``chunks`` table.""" + return [ + stmt + for stmt in captured + if stmt.lstrip().lower().startswith("select") and _CHUNK_SELECT_RE.search(stmt) + ] + + +def _seed_docs(session: Session) -> list[UUID]: + """``DOC_COUNT`` indexed documents of :data:`PROBE_SOURCE`, each with + ``CHUNKS_PER_DOC`` embedded chunks. Committed; returns the doc ids. + + The chunks are added BY ``document_id`` (never through + ``doc.chunks``) so the relationship stays UNLOADED — the state a real + prune starts from, and the only state in which the lazy load is + observable. + """ + ids: list[UUID] = [] + for n in range(DOC_COUNT): + doc = Document( + source=PROBE_SOURCE, + path=f"docs/doc-{n:04d}.md", + full_path=f"/srv/brain/{PROBE_SOURCE}/docs/doc-{n:04d}.md", + title=f"Cascade probe doc {n}", + content=CHUNK_CONTENT, + content_hash=f"{n:064x}", + ) + session.add(doc) + session.flush() + ids.append(doc.id) + # Deterministic 768-float vector, rotated by the doc index (a real + # pgvector payload — the point is the HYDRATION cost, so the values + # only need to be stable). + rot = n % 768 + embedding = [((i + rot) % 768) / 768 for i in range(768)] + session.add_all( + [ + Chunk( + document_id=doc.id, + position=position, + content=CHUNK_CONTENT, + embedding=embedding, + ) + for position in range(CHUNKS_PER_DOC) + ] + ) + session.commit() + return ids + + +def _run_prune(session: Session) -> tuple[int, ImportSummary]: + """The prune exactly as ``import_sources`` calls it with a walk that saw + NOTHING (``seen`` empty → every doc of the source prunes) and the + images toggle OFF (``app/rag/importer.py:3860``).""" + summary = ImportSummary() + pruned = _prune( + session, + {PROBE_SOURCE}, + set(), + images=False, + summary=summary, + page_dir=None, + ) + return pruned, summary + + +def _vm_rss_mb() -> float | None: + """``VmRSS`` from ``/proc/self/status`` in MB, ``None`` when unreadable + (a non-Linux box skips the memory test rather than pinning noise).""" + try: + for line in Path("/proc/self/status").read_text().splitlines(): + if line.startswith("VmRSS:"): + return int(line.split()[1]) / 1024.0 + except OSError: + return None + return None + + +# --------------------------------------------------------------------------- +# The probe +# --------------------------------------------------------------------------- + + +def test_prune_never_selects_a_chunk_row(db: Session, statement_capture: list[str]) -> None: + """Zero ``SELECT … FROM chunks`` for 200 pruned documents — the fix. + + POST-phase-183 assertion (``EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC`` is + ``0``): ``passive_deletes=True`` means the prune's 200 + ``session.delete(doc)`` calls never hydrate a child collection, so no + chunk row and no 768-dim embedding reaches Python — the whole walk is 2 + statements. PRE-phase-183 this same fixture measured 200 chunk SELECTs + and 203 statements (module docstring). + + Every other assertion here (the counts, the ``changed_paths`` feed, and + above all the ZERO orphan chunk rows) held byte-identically across the + flip — they exist so the fix cannot pass by quietly stopping to delete + children (phase 183 D4). + """ + ids = _seed_docs(db) + statement_capture.clear() # the seed's own SQL is not part of the probe + + pruned, summary = _run_prune(db) + + assert pruned == DOC_COUNT + expected_selects = DOC_COUNT * EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC + assert len(chunk_selects(statement_capture)) == expected_selects + + # D4 — the no-orphan contract, asserted BEFORE the fix so that after it + # "0 orphans" means "unchanged", never "never checked". + 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 == PROBE_SOURCE) + ) == 0 + + # The phase-127 changed_paths feed: one (source, path) per pruned doc, + # appended BEFORE the delete (the folder-summary regeneration input). + assert sorted(summary.changed_paths) == sorted( + (PROBE_SOURCE, f"docs/doc-{n:04d}.md") for n in range(DOC_COUNT) + ) + + +def test_prune_memory_delta_has_a_ceiling(db: Session) -> None: + """The corroborating memory figure — a ceiling, never the flaky pin. + + Same 200-doc prune, ``VmRSS`` read before and after (``gc.collect()`` + on both sides so the allocator gets its fair say). See the module + docstring for the measured numbers: **+165.6 MB** while the ORM hydrates + the child collections (PRE-phase-183), **+0.2 MB** on the FK-cascade + path (POST, task 02) against the 40 MB ceiling pinned below. + """ + before = _vm_rss_mb() + if before is None: + pytest.skip("/proc/self/status unreadable — the RSS ceiling is Linux-only") + + ids = _seed_docs(db) + gc.collect() + baseline = _vm_rss_mb() + assert baseline is not None + + pruned, _ = _run_prune(db) + gc.collect() + after = _vm_rss_mb() + assert after is not None + delta = after - baseline + + assert pruned == DOC_COUNT + assert db.scalar( + select(func.count()).select_from(Chunk).where(Chunk.document_id.in_(ids)) + ) == 0 + + if EXPECTED_CHUNK_SELECTS_PER_PRUNED_DOC == 0: + # POST-phase-183: no embedding is ever hydrated, so the whole prune + # must cost well under the ceiling (measured +0.2 MB). + assert delta < RSS_CEILING_MB, ( + f"VmRSS grew {delta:.1f} MB during a cascade prune — chunk rows" + " are being hydrated again (passive_deletes removed?)" + ) + else: + # PRE-phase-183 (the state the constant can be flipped back to): the + # hydration is the behaviour under test, so this fixture must land + # WELL ABOVE that ceiling — a ceiling nothing ever crossed would be + # a pin in name only. + assert delta > RSS_CEILING_MB + assert delta < PRE_FIX_RSS_SANITY_CEILING_MB + + +def test_chunks_fk_cascades_at_database_level(db: Session) -> None: + """The drift guard: every ``chunks`` FK really is ``ON DELETE CASCADE``. + + ``passive_deletes=True`` tells the ORM to trust the database with the + child rows. That trust is only sound while ``chunks_document_id_fkey`` + cascades (alembic 0001 L54; production confirmed + ``confdeltype='c'``); a future migration that recreates the FK without + the clause would turn every prune into a silent orphan-generator — the + documents would vanish, the chunk rows (and their vectors) would stay, + and nothing in the app would notice. So read the LIVE catalog and fail + loudly if the cascade is gone (phase 183 A3 — a guard instead of a new + migration). + + **The guard is not a tautology — proven 2026-10-15 against a scratch + database** (``bor_183_fk_scratch``, created + dropped for the check; + the shared test DB was never touched): ``alembic upgrade head`` + (``confdeltype='c'``, this test GREEN) → ``ALTER TABLE chunks DROP + CONSTRAINT chunks_document_id_fkey`` + re-add it ``ON DELETE NO + ACTION`` (``confdeltype='a'``) → this exact test FAILED, verbatim: + + ``` + AssertionError: chunks FKs not ON DELETE CASCADE: + chunks_document_id_fkey confdeltype='a' (NO ACTION) — Document.chunks' + passive_deletes=True would silently orphan chunk rows; restore the + clause (alembic 0001 L54) + ``` + + and with the clause still present-but-NO-ACTION the same fixture's + prune died on ``psycopg.errors.ForeignKeyViolation`` rather than + orphaning rows unnoticed — the ORM really does delegate the child rows + to the database now (which is the point, and exactly why the clause is + load-bearing). + """ + # ``pg_constraint.confdeltype`` codes → the DDL clause, so the failure + # message names whatever the FK actually does (not always NO ACTION). + deltype = { + "a": "NO ACTION", + "r": "RESTRICT", + "c": "CASCADE", + "n": "SET NULL", + "d": "SET DEFAULT", + } + rows = db.execute( + text( + "select conname, confdeltype from pg_constraint" + " where conrelid = 'chunks'::regclass and contype = 'f'" + " order by conname" + ) + ).all() + # 'c' = ON DELETE CASCADE (pg_constraint.confdeltype). + assert rows, "no foreign key on chunks at all — the cascade cannot hold" + assert all(r[1] == "c" for r in rows), ( + "chunks FKs not ON DELETE CASCADE: " + + ", ".join( + f"{r[0]} confdeltype={r[1]!r} ({deltype.get(r[1], 'UNKNOWN')})" for r in rows + ) + + " — Document.chunks' passive_deletes=True would silently orphan" + " chunk rows; restore the clause (alembic 0001 L54)" + ) + + +def test_prune_with_a_loaded_collection_still_deletes_the_chunks(db: Session) -> None: + """The other caller state: the collection IS loaded (phase 183 D4). + + ``passive_deletes=True`` only promises not to LOAD the children — a + caller that already holds them (what ``_replace_content_chunks`` leaves + behind in the same sync session, or any future caller that queries + eagerly) must still lose the rows. Verified 2026-10-15: the ORM deletes + the LOADED children with its own ``DELETE FROM chunks`` and the FK + cascade covers whatever is not loaded — no ``SELECT`` of ``chunks`` + either way, and zero orphan rows in BOTH states. The keyword changes + what Python loads, never what the database deletes. + """ + doc = Document( + source=PROBE_SOURCE, + path="loaded.md", + full_path=f"/srv/brain/{PROBE_SOURCE}/loaded.md", + title="Loaded collection", + content=CHUNK_CONTENT, + content_hash="f" * 64, + ) + db.add(doc) + db.flush() + db.add_all( + [ + Chunk( + document_id=doc.id, + position=position, + content=CHUNK_CONTENT, + embedding=[(i + position) % 768 / 768 for i in range(768)], + ) + for position in range(5) + ] + ) + db.commit() + + row = db.scalar(select(Document).where(Document.source == PROBE_SOURCE)) + assert row is not None + assert len(row.chunks) == 5 # the collection is now IN the identity map + + db.delete(row) + db.commit() + + assert db.scalar( + select(func.count()).select_from(Document).where(Document.id == doc.id) + ) == 0 + assert db.scalar( + select(func.count()).select_from(Chunk).where(Chunk.document_id == doc.id) + ) == 0 diff --git a/tests/unit/test_importer.py b/tests/unit/test_importer.py index a265d4d..688168d 100644 --- a/tests/unit/test_importer.py +++ b/tests/unit/test_importer.py @@ -1469,6 +1469,30 @@ def test_prune_records_changed_paths_only_when_summary_given(db, tmp_path: Path) _cleanup_source(db, root.name) +# ---------- phase 183 (task 02): the prune deletes IN the database ---------- + + +def test_document_chunks_relationship_is_passive_deletes() -> None: + """``Document.chunks`` is ``passive_deletes=True`` (phase 183, LOCKED A1). + + The mapper-level pin is the cheapest place to catch the keyword being + dropped: without it ``session.delete(doc)`` SELECTs every chunk of the + document before deleting it, hydrating every 768-dim + ``Chunk.embedding`` (~24.8 KiB of Python each) — the shape that made a + 3,008-document prune cost ~720 MB and locked up the 2026-10-15 VM + (3,904 MB, no swap). The other half of the contract — the FK really is + ``ON DELETE CASCADE`` — is pinned live by + ``tests/integration/test_prune_cascade_statements.py`` + (``test_chunks_fk_cascades_at_database_level``), so these two tests + fail together the moment either side drifts. + """ + rel = Document.__mapper__.relationships["chunks"] + assert rel.passive_deletes is True + # The ORPHAN semantics ``_replace_content_chunks``' re-embed relies on + # (``doc.chunks = [...]`` drops the old rows) stay intact (A2). + assert "delete-orphan" in str(rel.cascade) + + # ---------- phase 129 (task 02): on_chunk_progress plumbing ----------