phase: 184_sync_bounded_session
All gates green; no code changes were needed — the prior failure was a missing final report, not a test failure. **Verified / implemented (phase 184 final pass)** - All 4 tasks present in `.agents/phases/complete/184_sync_bounded_session/`; confirmed their output in the worktree: `sync_session_reset_files=200` / `sync_prune_batch=500` on `Settings` (+ non-negative validators), both `SettingMeta` rows under `"Import & sources"`, `.env.example` + `README.md` entries. - `import_sources` expunges at **exactly two** sites (per-file loop top L972, source boundary L1012) — grep proves no third `expunge`/`expire_all` anywhere in `app/`; `app/db.py:30` untouched. - `_prune` uses `load_only(id, source, path, is_image, image_path, page_count).order_by(Document.id)` + per-batch commits, with the phase-122/126 guards and `changed_paths` intact. **Tests / lint / coverage (all 100% green)** - `uv run pytest --cov=app --cov-report=term-missing` → **4506 passed**, `app/` TOTAL **99%** (>90%). - `uv run pytest tests/integration/test_import_session_retention.py -v --no-cov` → **12 passed**. - `uv run pytest tests/e2e/test_sync_session_reset.py -v --no-cov` → **4 passed in isolation**. - Regression sweep of neighbouring sync suites (each isolated): `test_sync_changed_only` 3, `test_sync_summary_visibility` 3, `test_prune_cascade_cleanup` 3, `test_sync_git_freeze` 2 — all passed. - `uv run ruff check .` → All checks passed. `uv run pyright` → 0 errors, 0 warnings. **Completion criteria** 1. Settings + CATALOG + `.env.example` + README; `test_settings_catalog.py` → ✅ green 2. Retention test green with the post-fix bound and both measured numbers in the docstring (pre-fix 12-row monotone staircase / ≈265 MB extrapolated; post-fix ends at 0, add-path peak 14 vs 21→49) → ✅ 3. Expunge only at the two documented-safe points; light-row batched prune with guards intact → ✅ 4. Every existing importer/sync/removal test green with unchanged expectations → ✅ 5. Full suite green, coverage >90%, ruff + pyright clean → ✅ 6. Phase E2E green in isolation → ✅ 7. README + AGENTS accurate/consistent → ✅ 8. Atomic commit — left to the harness (I ran no git write commands). **Documentation** — Full README (1886 lines) + AGENTS.md (189 lines) review: **verified consistent — no changes needed.** Programmatic cross-checks: every `Settings` field appears in `.env.example` and in the README config reference; every `.env.example` var is documented in that reference; every `tests/…` and `app/…` path cited in README/AGENTS exists; all 10 README page URLs are registered in `app/main.py`; documented E2E port vars (`E2E_APP_PORT_SYNCRESET[_ZERO]`) exist in the test. The only reference-var without a `Settings` field is `BOR_THEME`, which the README explicitly documents as retired (correct as written). **Notable decisions/deviations** — None. No code, test, or doc edits were made this pass; changes remain entirely in the working tree for the harness to stage. `00_phase.md` still sits in `todo/` while tasks 01–04 are in `complete/` — harness-owned, so I left it untouched. **Next pending phase:** `185_sync_defer_content_fast_path`.
This commit is contained in:
@@ -1,59 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,33 +0,0 @@
|
||||
# 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).
|
||||
@@ -1,45 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,51 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,35 +0,0 @@
|
||||
# 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/`.
|
||||
Reference in New Issue
Block a user