phase: 187_sync_peak_memory_line
**Phase 187 — final verification pass: all criteria met, repo 100% green.**
Verified (all four tasks already in `complete/`)
- `app/core/memory.py`: `rss_mb` / `rss_peak_mb` / `cgroup_peak_mb` / `fmt_mb`, pure stdlib, never-raise, **100%** coverage, 24 branch tests.
- Lifecycle lines append-only: `sync: started … rss_mb=`, all three terminals `… rss_mb= rss_peak_mb=` (failed = sanitized one-liner beside the surviving `logger.exception`), `import: summary … rss_mb=`; prefixes byte-identical.
- `GET /api/sync/status`: additive `rss_mb`/`rss_peak_mb` (`int|null`), keyed off the ONE `_sample_rss` (run start, phase transitions, forced terminals, 5 s-throttled hook), `peak >= current` invariant, anonymous 403.
- `sources.js` `fmtSyncPeakClause`: ` · peak <N> MB` trailing the counts line / "Sync cancelled", nothing when null, no new control/live region.
Gates (exact commands)
- `uv run pytest --cov=app --cov-report=term-missing` → **4570 passed**, TOTAL **99%**, `app/api/sync.py` **100%**, `app/core/memory.py` **100%**
- `uv run ruff check .` → clean · `uv run pyright` → **0 errors**
- `uv run pytest tests/e2e/test_sync_peak_memory_visible.py -v --no-cov` → **5 passed in isolation** (ran twice)
- All 27 touched E2E families re-run **in isolation**, all green (progress_box 8, cancel 3, status_freshness 4, summary_visibility 3, sync_button 3, session_reset 4, git_freeze 2, git_log_stream_dates 5, PDF/image suites, …)
Completion criteria: every checkbox ✅ except the commit + phase-dir move, which belong to the harness.
Documentation
- README reviewed in full (pages, commands, config tables cross-checked against `app/config.py` — 85 env rows, all defaults match; phase-187 ops section, greps, process-vs-cgroup caveat): consistent, no changes needed.
- `AGENTS.md`: **fixed one pre-existing inconsistency** — rule 6 claimed the no-CDN test covers "the index page"; it actually parametrizes 10 pages (all but `/doc-edit.html`).
Notable
- Two failures early in the pass were self-inflicted by running the E2E module concurrently with the full suite on the shared Postgres (TRUNCATE deadlock + one order-dependent `test_overview` flake); both vanish under the documented serial isolation — no code defect, no change made.
- **PLAN §9 still needs the owner's reconciliation** (phase 187 adds the sync memory fields + two status keys; PLAN is never agent-edited — AGENTS rule 10 carries the spec). `TODO.md` holds an unrelated owner note, left untouched.
Next pending phase: `188_chunks_hnsw_index`.
This commit is contained in:
+197
-5
@@ -239,6 +239,32 @@ it (a file with no images must not inherit the previous PDF's
|
||||
position). ASSUMPTION A5: no new ``phase`` value — the image spans
|
||||
live inside ``phase: "import"``.
|
||||
|
||||
Phase 187 (task 02) makes the run's MEMORY a logged number: every
|
||||
lifecycle line gains trailing RSS fields read through
|
||||
:mod:`app.core.memory` — ``sync: started … rss_mb=N`` and, in every
|
||||
terminal, ``… rss_mb=N rss_peak_mb=M`` (the ``import: summary …`` line
|
||||
carries ``rss_mb=`` too, on the importer side). APPEND-ONLY (LOCKED A2):
|
||||
no existing field is renamed, reordered or re-typed, so every pre-phase
|
||||
``grep`` keeps working; an unknowable reading renders ``rss_mb=?``
|
||||
through the ONE :func:`app.core.memory.fmt_mb` helper. The numbers are
|
||||
PROCESS numbers, not the host's cgroup figure — the
|
||||
:mod:`app.core.memory` module docstring owns that caveat. The ``failed``
|
||||
funnel keeps its ``logger.exception`` (the traceback) and gains ONE
|
||||
sanitized one-line summary beside the status field, so the raw exception
|
||||
string never reaches the log un-sanitized.
|
||||
|
||||
Phase 187 (task 03) puts the same numbers on the STATUS surface: the
|
||||
status object carries ``rss_mb`` / ``rss_peak_mb`` (``int | null``,
|
||||
sampled through the ONE :func:`_sample_rss` — at the run start, at every
|
||||
``phase`` transition, in every terminal, and at most once per
|
||||
``_RSS_SAMPLE_INTERVAL_S`` from the per-file progress hook, so a 27k-file
|
||||
walk costs ~600 ``/proc`` reads and not 27k — LOCKED A4), and
|
||||
``GET /api/sync/status`` reports both keys ADDITIVELY. The terminals keep
|
||||
the FINAL reading next to the final counts (the phase-64 convention), so
|
||||
the page that started the run answers the incident's question without a
|
||||
shell. A reading the platform cannot know stays ``null`` on the wire (the
|
||||
UI renders no clause for it) — never 0, never an invented number.
|
||||
|
||||
The ``failed`` state's ``error`` string is masked by the shared
|
||||
sanitizer — the ``user:pass@`` masker now lives in :mod:`app.core.errors`
|
||||
(imported here under the private name ``_sanitize_error``).
|
||||
@@ -247,6 +273,7 @@ from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import logging
|
||||
import time
|
||||
from dataclasses import dataclass, field
|
||||
from datetime import UTC, datetime
|
||||
from pathlib import Path
|
||||
@@ -257,6 +284,7 @@ from fastapi import APIRouter, Depends, HTTPException
|
||||
from app.config import get_settings
|
||||
from app.core.auth import require_admin
|
||||
from app.core.errors import sanitize_error as _sanitize_error
|
||||
from app.core.memory import fmt_mb, rss_mb, rss_peak_mb
|
||||
from app.db import SessionLocal
|
||||
from app.rag.folder_summaries import generate_folder_summaries, missing_folder_summaries
|
||||
from app.rag.git_sources import clone_url_for, effective_sources
|
||||
@@ -338,6 +366,21 @@ class SyncStatus:
|
||||
``progress`` hook resets it to 0/0 (a ``progress`` call names the
|
||||
file and clears the image position; a file with no images must
|
||||
not inherit the previous PDF's position).
|
||||
|
||||
Phase 187 (task 03) memory fields: ``rss_mb`` is the process's
|
||||
CURRENT resident set size in MB and ``rss_peak_mb`` its HIGH-WATER
|
||||
mark — both PROCESS readings from :mod:`app.core.memory`, never the
|
||||
host's cgroup figure (that one counts page cache; the caveat lives
|
||||
in that module). They are written by ONE sampler, :func:`_sample_rss`:
|
||||
forced at the run start, at every ``phase`` transition and in every
|
||||
terminal, and throttled to one read per ``_RSS_SAMPLE_INTERVAL_S``
|
||||
from the per-file ``progress`` hook (LOCKED A4 — never one ``/proc``
|
||||
read per file). Both reset to ``None`` with the run — an idle or
|
||||
never-run status reports null, and the Sources page renders no peak
|
||||
clause for null — and every terminal KEEPS the final reading next to
|
||||
the final counts (the phase-64 keep-final-values convention, the one
|
||||
that makes "how much RAM did that sync use?" answerable from the
|
||||
page after the run, not only from the log).
|
||||
"""
|
||||
|
||||
state: Literal["idle", "running", "success", "failed", "cancelled"] = "idle"
|
||||
@@ -372,6 +415,13 @@ class SyncStatus:
|
||||
# the prelude/overview/summaries phases, and in terminal states.
|
||||
images_done: int = 0
|
||||
images_total: int = 0
|
||||
# Phase 187 (task 03): the run's PROCESS memory in MB — current and
|
||||
# high-water (app.core.memory, never the host's cgroup figure). None
|
||||
# idle / never-run and reset with the run; kept as the FINAL reading
|
||||
# in every terminal. Written ONLY through _sample_rss, the one
|
||||
# sampling point (LOCKED A4).
|
||||
rss_mb: int | None = None
|
||||
rss_peak_mb: int | None = None
|
||||
|
||||
|
||||
_status = SyncStatus()
|
||||
@@ -383,6 +433,66 @@ _task: asyncio.Task[None] | None = None
|
||||
# starts, so a stale event can never bleed into the next run.
|
||||
_cancel_event = asyncio.Event()
|
||||
|
||||
#: Phase 187 (task 03, D3/A4): how often the PER-FILE progress hook may
|
||||
#: re-read the process memory, in seconds. That hook fires once per
|
||||
#: file, so without this guard a 27k-file walk would cost 27k
|
||||
#: ``/proc/self/status`` reads; with it the same walk costs ~600 and the
|
||||
#: operator watching the page still sees the number move (PLAN §7.4's
|
||||
#: never-stale contract). Every other sampling point (the run start, the
|
||||
#: ``phase`` transitions, the terminals) FORCES the read instead.
|
||||
_RSS_SAMPLE_INTERVAL_S = 5.0
|
||||
|
||||
#: ``time.monotonic()`` of the last RSS read; ``-inf`` means "never", so
|
||||
#: the first throttled call reads instead of waiting out the interval.
|
||||
#: Module-global like ``_status`` itself (the tests' fresh-state fixture
|
||||
#: re-seeds both).
|
||||
_rss_sampled_at: float = float("-inf")
|
||||
|
||||
|
||||
def _sample_rss(force: bool = False) -> None:
|
||||
"""Read the process memory onto ``_status`` — the ONE sampling point
|
||||
(phase 187, task 03).
|
||||
|
||||
``force=True`` reads NOW: the run start, each ``phase`` transition,
|
||||
every terminal — the points where the number is worth a syscall and
|
||||
where a stale reading would be a lie. ``force=False`` (the per-file
|
||||
progress hook) is a NO-OP inside :data:`_RSS_SAMPLE_INTERVAL_S` of
|
||||
the previous read, which is what makes the sampling throttled rather
|
||||
than per-file (LOCKED A4).
|
||||
|
||||
Both fields come off the SAME read, so the pair the status endpoint
|
||||
and the lifecycle line report is one instant, not two. Nothing here
|
||||
raises: :mod:`app.core.memory` degrades a platform that cannot know
|
||||
to ``None``, which the status renders as JSON null (no UI clause) and
|
||||
the log as ``?``.
|
||||
|
||||
**The pair is normalized to one truth: ``peak >= current``.** The two
|
||||
numbers come from two different kernel counters read at two instants
|
||||
(``VmRSS`` from ``/proc/self/status``, then ``ru_maxrss``), and the
|
||||
kernel refreshes its high-water LAZILY, so a growing process really
|
||||
can report a current reading ABOVE the reported high-water (measured
|
||||
on this repo's own suite: ``rss_mb=164`` against ``rss_peak_mb=162``).
|
||||
A "peak" below the run's own current footprint is not a smaller
|
||||
number, it is a FALSE one — the high-water is by definition at least
|
||||
the current set — so the ONE sampling point raises it to the current
|
||||
reading. That is a lower bound on the truth, never an invention, and
|
||||
it is why every consumer (both lifecycle lines, the status payload,
|
||||
the page's clause) can now take ``peak >= current`` as an invariant
|
||||
instead of carrying a slack. A ``None`` is still a ``None``: an
|
||||
unknowable peak is never fabricated from the current reading.
|
||||
"""
|
||||
global _rss_sampled_at
|
||||
now = time.monotonic()
|
||||
if not force and (now - _rss_sampled_at) < _RSS_SAMPLE_INTERVAL_S:
|
||||
return
|
||||
_rss_sampled_at = now
|
||||
current = rss_mb()
|
||||
peak = rss_peak_mb()
|
||||
if current is not None and peak is not None and peak < current:
|
||||
peak = current # a high-water below the current set is a stale one
|
||||
_status.rss_mb = current
|
||||
_status.rss_peak_mb = peak
|
||||
|
||||
|
||||
@router.get("/status")
|
||||
def sync_status() -> dict[str, Any]:
|
||||
@@ -415,8 +525,22 @@ def sync_status() -> dict[str, Any]:
|
||||
file share ONE continuous counter): 0/0 before the first image, in
|
||||
the prelude/overview/summaries phases, and in terminal states; the
|
||||
per-file ``progress`` hook clears it at every file boundary (a
|
||||
file with no images never shows a stale position). Additive only —
|
||||
every pre-existing key keeps its name, type, and meaning.
|
||||
file with no images never shows a stale position).
|
||||
|
||||
Phase 187 (task 03): ``rss_mb`` / ``rss_peak_mb`` are the run's
|
||||
PROCESS memory in MB — the current resident set and the high-water
|
||||
mark (:mod:`app.core.memory`, never the host's cgroup figure, which
|
||||
counts page cache). ``null`` means "no reading", which is the idle /
|
||||
never-run state AND any platform that cannot know — never 0, and
|
||||
never an invented number. The run samples them through
|
||||
:func:`_sample_rss`: at the run start, at every ``phase``
|
||||
transition, throttled to one read per 5 s through the import's
|
||||
per-file hook, and FORCED in every terminal — so a terminal reports
|
||||
the run's FINAL reading next to its final counts (the phase-64
|
||||
convention, same as ``files_done`` / ``files_total``), and a reload
|
||||
after the run still shows the number that ``sync: done …`` logged.
|
||||
Additive only — every pre-existing key keeps its name, type, and
|
||||
meaning.
|
||||
"""
|
||||
return {
|
||||
"state": _status.state,
|
||||
@@ -449,6 +573,14 @@ def sync_status() -> dict[str, Any]:
|
||||
"current_summary": _status.current_summary,
|
||||
"summaries_done": _status.summaries_done,
|
||||
"summaries_total": _status.summaries_total,
|
||||
# Phase 187 (task 03): the run's PROCESS memory in MB (current +
|
||||
# high-water), read through the ONE _sample_rss. null idle /
|
||||
# never-run; in a terminal it is the run's FINAL reading (the
|
||||
# phase-64 keep-final-values convention — the number the
|
||||
# 2026-10-15 incident had to reconstruct from systemd teardown
|
||||
# lines, now on the page that started the run).
|
||||
"rss_mb": _status.rss_mb,
|
||||
"rss_peak_mb": _status.rss_peak_mb,
|
||||
}
|
||||
|
||||
|
||||
@@ -567,6 +699,12 @@ async def _run_sync() -> None:
|
||||
# per-file progress hook re-clears them at every file boundary).
|
||||
_status.images_done = 0
|
||||
_status.images_total = 0
|
||||
# Phase 187 (task 03): the run-start memory sample — forced, so it
|
||||
# BOTH replaces the previous run's readings (a forced sample fully
|
||||
# determines the pair; None where the platform cannot know) and
|
||||
# re-arms the 5 s throttle. No stale peak ever bleeds into a new
|
||||
# run's status.
|
||||
_sample_rss(force=True)
|
||||
try:
|
||||
settings = get_settings()
|
||||
# Step 1 (phase 41): fail fast — verify both models the sync
|
||||
@@ -591,9 +729,15 @@ async def _run_sync() -> None:
|
||||
# legacy directories.
|
||||
raise GitSyncError("no sources configured (git or local)")
|
||||
git_count = sum(1 for row in rows if row.kind == "git")
|
||||
# Phase 187 (task 02): the trailing rss_mb= is APPEND-ONLY (LOCKED
|
||||
# A2) — every pre-phase grep of this line keeps matching. Task 03
|
||||
# logs the run-start SAMPLE (the reading the status reports) so
|
||||
# the line and the payload never disagree — the shape is
|
||||
# byte-identical to task 02's.
|
||||
logger.info(
|
||||
"sync: started repos=%d origin=%s git=%d local=%d",
|
||||
"sync: started repos=%d origin=%s git=%d local=%d rss_mb=%s",
|
||||
len(rows), origin, git_count, len(rows) - git_count,
|
||||
fmt_mb(_status.rss_mb),
|
||||
)
|
||||
sources_root = Path(settings.sources_dir).expanduser()
|
||||
sources: list[Path] = []
|
||||
@@ -690,6 +834,11 @@ async def _run_sync() -> None:
|
||||
# run-start reset alone cannot cover it).
|
||||
_status.images_done = 0
|
||||
_status.images_total = 0
|
||||
# Phase 187 (task 03): the run's memory, sampled THROTTLED
|
||||
# (unforced) — this hook fires once per file, so a 27k-file
|
||||
# walk reads /proc ~600 times instead of 27k (LOCKED A4)
|
||||
# while the number on the page still climbs (PLAN §7.4).
|
||||
_sample_rss()
|
||||
|
||||
# Phase 129 (task 03): the in-flight file's embed-chunk
|
||||
# position — the LLMClient.embed hook (task 01) fires before
|
||||
@@ -720,6 +869,10 @@ async def _run_sync() -> None:
|
||||
# starts NOW (the model-check + clone/pull prelude above
|
||||
# reported ``phase: null``).
|
||||
_status.phase = "import"
|
||||
# Phase 187 (task 03): the phase transitions are the forced
|
||||
# sampling points — each stage of the run begins with its own
|
||||
# reading on the status (the per-file hook throttles).
|
||||
_sample_rss(force=True)
|
||||
# Phase 134 (task 01): the import boundary — a cancel between
|
||||
# the source walk and the import (or parked in a blocking git
|
||||
# span that just finished) lands here, before any file is
|
||||
@@ -744,6 +897,7 @@ async def _run_sync() -> None:
|
||||
overview = False
|
||||
if summary.added + summary.updated > 0:
|
||||
_status.phase = "overview" # phase 98 (task 01)
|
||||
_sample_rss(force=True) # phase 187 (task 03)
|
||||
# Phase 129 (task 03): the import phase ends here — the
|
||||
# chunk position (a sub-position of a done file) clears
|
||||
# with the phase change; the file counts keep their final
|
||||
@@ -803,6 +957,7 @@ async def _run_sync() -> None:
|
||||
# position through the (long) span (its total is the
|
||||
# affected count).
|
||||
_status.phase = "summaries"
|
||||
_sample_rss(force=True) # phase 187 (task 03)
|
||||
# Phase 129 (task 03): the import phase ends here too
|
||||
# (the changed-KB branch skips no phase) — same
|
||||
# chunk-position clear as the overview transition.
|
||||
@@ -828,6 +983,7 @@ async def _run_sync() -> None:
|
||||
# phase + hook (the fill's total is the missing
|
||||
# count — the hook's loop-start ``total``).
|
||||
_status.phase = "summaries"
|
||||
_sample_rss(force=True) # phase 187 (task 03)
|
||||
# Phase 129 (task 03): the import phase ends here
|
||||
# too (the unchanged-walk gap-fill branch) — same
|
||||
# chunk-position clear.
|
||||
@@ -903,7 +1059,20 @@ async def _run_sync() -> None:
|
||||
"overview": overview,
|
||||
"sources_version": sources_version,
|
||||
}
|
||||
logger.info("sync: done detail=%s", _status.detail)
|
||||
# Phase 187 (task 02): the memory the run is at NOW and its
|
||||
# high-water mark, next to the final counts (the phase-64
|
||||
# keep-final-values convention, extended to the numbers the
|
||||
# 2026-10-15 incident had to reconstruct from systemd teardown
|
||||
# lines). Appended after ``detail=`` — the line stays greppable
|
||||
# as ``sync: done detail=``. Task 03 forces the sample FIRST and
|
||||
# logs it, so the payload's rss_mb / rss_peak_mb and this line
|
||||
# carry the SAME final pair (the phase-64 convention, kept
|
||||
# honest across both surfaces).
|
||||
_sample_rss(force=True)
|
||||
logger.info(
|
||||
"sync: done detail=%s rss_mb=%s rss_peak_mb=%s",
|
||||
_status.detail, fmt_mb(_status.rss_mb), fmt_mb(_status.rss_peak_mb),
|
||||
)
|
||||
except asyncio.CancelledError:
|
||||
# Phase 134 (task 01): the TWO conversion paths (the module
|
||||
# docstring's cancel paragraph). A user-requested cancel —
|
||||
@@ -933,9 +1102,17 @@ async def _run_sync() -> None:
|
||||
# counts above keep their final values).
|
||||
_status.images_done = 0
|
||||
_status.images_total = 0
|
||||
# Phase 187 (task 02): the same two trailing fields as every
|
||||
# other terminal — a cancelled run's memory is still data.
|
||||
# Task 03 forces the sample first, so the cancelled payload
|
||||
# keeps that same final pair (the status object stays
|
||||
# authoritative after a cancel, as everywhere).
|
||||
_sample_rss(force=True)
|
||||
logger.info(
|
||||
"sync: cancelled by user state=cancelled files_done=%d files_total=%d",
|
||||
"sync: cancelled by user state=cancelled files_done=%d files_total=%d"
|
||||
" rss_mb=%s rss_peak_mb=%s",
|
||||
_status.files_done, _status.files_total,
|
||||
fmt_mb(_status.rss_mb), fmt_mb(_status.rss_peak_mb),
|
||||
)
|
||||
raise
|
||||
except Exception as e: # noqa: BLE001 — a background task dies in state, see above
|
||||
@@ -943,6 +1120,21 @@ async def _run_sync() -> None:
|
||||
_status.state = "failed"
|
||||
_status.finished_at = datetime.now(UTC)
|
||||
_status.error = _sanitize_error(str(e))
|
||||
# Phase 187 (task 02): ONE sanitized one-line summary beside the
|
||||
# status field (the SAME masked string, so the log can never leak
|
||||
# credentials the API does not) — the ``logger.exception`` above
|
||||
# stays for the traceback. Task 03 forces the sample first, so
|
||||
# the failed payload carries the same final pair as this line —
|
||||
# a run that died mid-walk is exactly the run whose memory
|
||||
# matters. _sample_rss never raises (app.core.memory never
|
||||
# raises), so a memory read can never mask the failure.
|
||||
_sample_rss(force=True)
|
||||
logger.error(
|
||||
"sync: failed state=failed files_done=%d files_total=%d"
|
||||
" rss_mb=%s rss_peak_mb=%s error=%s",
|
||||
_status.files_done, _status.files_total,
|
||||
fmt_mb(_status.rss_mb), fmt_mb(_status.rss_peak_mb), _status.error,
|
||||
)
|
||||
_status.current_file = None # phase 64: keep the final counts
|
||||
_status.phase = None # phase 98: keep the final summary counts
|
||||
_status.current_summary = None
|
||||
|
||||
Reference in New Issue
Block a user