phase: 134_sync_cancel
**Phase 134 final verification pass — all green** (all 3 task files already in `complete/`; no implementation was missing) - Verified: 5-state `SyncStatus` + `cancelled` terminal (error None, final counts kept), `POST /api/sync/cancel` (202/409/403), cooperative `_check_cancelled` boundaries, shutdown `CancelledError` contract preserved, `#sync-cancel-btn` show/hide + neutral "Sync cancelled" settle, 8-branch poll tree, re-attach — code, unit, integration, and E2E layers all present - `uv run pytest --cov=app --cov-report=term-missing` → **3150 passed**, `app/` coverage **99%** (gate: >90%) - `uv run pytest tests/e2e/test_sync_cancel.py -v --no-cov` (isolated) → **3 passed** (47.6 s) - `uv run ruff check .` → clean; `uv run pyright` → 0 errors, 0 warnings **Completion criteria:** ① cancel endpoint 202/409/403 ✅ (unit + E2E) ② `cancelled` terminal reported by status, error null, counts kept ✅ (integration `test_cancel_mid_run_lands_in_cancelled_then_resyncs_clean`) ③ unrequested-cancel pin ✅ (`test_unrequested_cancel_is_never_swallowed_and_never_converts`) ④ UI lifecycle + clean re-sync (sentinel row, full 40-file import) ✅ (E2E) ⑤ suite/coverage/E2E gates ✅ ⑥ uncancelled runs byte-identical ✅ (all 3150 pre-existing tests green untouched) - Docs: README.md reviewed in full against code (pages, auth endpoints, every config default vs `app/config.py`, palette 9+8, fail-loud, cancel section) — consistent, no changes needed. AGENTS.md: fixed one pre-existing inaccuracy (rule 2 "move its file" → "move its directory" — phases are directories); everything else verified accurate - No live infrastructure touched; no deviations **Next pending phase:** `135_git_source_branch`
This commit is contained in:
+182
-14
@@ -10,7 +10,13 @@ untouched — no queue, no new services): one ``asyncio`` background task
|
||||
plus a module-level :class:`SyncStatus` that the UI polls every 2 s
|
||||
(task 02). One sync at a time — ``POST`` while a run is in flight is
|
||||
409; the status object is authoritative, so the UI can never sit on a
|
||||
stale button state (§7.4 adaptation, phase locked decisions).
|
||||
stale button state (§7.4 adaptation, phase locked decisions). Phase
|
||||
134 (task 01) adds the cancel: ``POST /api/sync/cancel`` (admin-only,
|
||||
same router) registers a COOPERATIVE cancel while a run is in flight
|
||||
(202, 409 ``no sync is running`` otherwise), and the run lands in the
|
||||
new ``cancelled`` TERMINAL state — a cancel is intent, not failure
|
||||
(``error = None``, ``detail = {}``, the final counts kept; the cancel
|
||||
paragraph below).
|
||||
|
||||
Pipeline (the canonical "mirror the sources" action — phase locked
|
||||
decisions):
|
||||
@@ -92,6 +98,53 @@ carries the phase-64 per-file progress — ``current_file`` (the
|
||||
(clone/pull reports no file yet) and in terminal states, which clear
|
||||
``current_file`` but keep the run's final counts.
|
||||
|
||||
Phase 134 (task 01) makes the machine FIVE-state: ``cancelled`` joins
|
||||
``success`` / ``failed`` as a TERMINAL state — it carries
|
||||
``finished_at``, clears ``current_file`` / ``phase`` /
|
||||
``current_summary`` and the chunk position, and keeps the run's final
|
||||
``files_done`` / ``files_total`` + summary counts (the phase-64/98
|
||||
keep-final-counts convention) — but with ``error = None`` and
|
||||
``detail = {}``: a cancel is intent, not failure, so the UI never
|
||||
treats it as an error (no banner, no modal — a neutral result line).
|
||||
``GET /api/sync/status`` gains nothing structurally — the ``state``
|
||||
key simply reports ``"cancelled"``.
|
||||
|
||||
The cancel is COOPERATIVE, best-effort (phase 134 design): while a
|
||||
run is in flight, ``POST /api/sync/cancel`` (the router's
|
||||
``require_admin`` dependency applies — 403 anonymous) sets the
|
||||
module-level ``_cancel_event`` AND ``_task.cancel()`` (unblocking
|
||||
whatever ``await`` the pipeline is parked on — e.g. a per-file LLM
|
||||
call inside the import), answering 202 the moment the request is
|
||||
REGISTERED (the run may already be terminal by the next poll — the
|
||||
status object, not the task, is authoritative). ``start_sync`` clears
|
||||
the event when it starts a run, so a stale event from a previously
|
||||
cancelled run can never bleed into the next one. ``_run_sync``
|
||||
checks the event at the pipeline boundaries via
|
||||
:func:`_check_cancelled` (raises ``asyncio.CancelledError`` when
|
||||
set): the top of the per-source loop (before each clone/pull or
|
||||
local-dir check), immediately before ``import_sources``, and
|
||||
immediately before the overview + summaries span (the
|
||||
``added + updated > 0`` block). Consequence a future reader must
|
||||
know: a git subprocess already running at the cancel moment runs to
|
||||
completion (the blocking ``subprocess.run`` has no await inside) —
|
||||
the cancel lands at the NEXT boundary (after that source's git ops,
|
||||
between files, or before overview/summaries); during the import
|
||||
span, the ``_task.cancel()`` from the endpoint interrupts at the
|
||||
next per-file await.
|
||||
|
||||
The ``CancelledError`` contract has TWO paths (the former
|
||||
"deliberately not caught" rule refined, not broken): in
|
||||
``_run_sync``'s ``except asyncio.CancelledError`` clause (placed
|
||||
before the ``except Exception``), a USER-requested cancel
|
||||
(``_cancel_event`` set) lands the run in the ``cancelled`` terminal
|
||||
+ one INFO line (PLAN §9: ``sync: cancelled by user state=cancelled
|
||||
files_done=… files_total=…``) and then RE-RAISES (the task ends
|
||||
cancelled — ``_task.done()`` is True, the status object stays
|
||||
authoritative); an UNREQUESTED cancel (the app-shutdown path — the
|
||||
event unset) re-raises UNCHANGED (the shutdown contract holds: a
|
||||
background task must die in state, never as an unobserved exception
|
||||
— swallowing an unrequested stop would mask a real one).
|
||||
|
||||
Phase 98 (task 01) adds the run's PHASE machine — ``phase`` is the
|
||||
pipeline stage the run is in: the model-check + clone/pull prelude
|
||||
reports ``null`` (the bare "Syncing…" label stays), then ``"import"``
|
||||
@@ -108,9 +161,10 @@ summarized — the bare source name for the source-root row) plus
|
||||
through the long summary span where the file count sits still (the
|
||||
hook's ``total`` is the run's key count — the AFFECTED count on a
|
||||
changed KB, the missing count on a gap-fill).
|
||||
Terminal states (success AND failed) clear ``phase`` +
|
||||
``current_summary`` but keep the run's final ``summaries_done`` /
|
||||
``summaries_total`` (the phase-64 keep-final-counts convention).
|
||||
Terminal states (success, failed, and — phase 134 — cancelled) clear
|
||||
``phase`` + ``current_summary`` but keep the run's final
|
||||
``summaries_done`` / ``summaries_total`` (the phase-64
|
||||
keep-final-counts convention).
|
||||
|
||||
Phase 129 (task 03) adds the in-flight file's EMBED-chunk position —
|
||||
``current_chunk`` (the 1-based first chunk of the in-flight
|
||||
@@ -120,7 +174,7 @@ chunks), batch granularity via the :meth:`app.rag.llm.LLMClient.embed`
|
||||
batch (the prelude), during the ``"overview"`` / ``"summaries"``
|
||||
phases, and in terminal states — the import phase clearing them at
|
||||
EVERY point it ends (the ``phase = "overview"`` and
|
||||
``phase = "summaries"`` transitions and both terminals, the same spots
|
||||
``phase = "summaries"`` transitions and every terminal, the same spots
|
||||
``current_file`` clears). Unlike the phase-64 FILE counts, the chunk
|
||||
position is a SUB-position of a file that's done — the keep-final-
|
||||
counts convention does NOT extend to it: the file counts survive the
|
||||
@@ -168,10 +222,15 @@ router = APIRouter(
|
||||
class SyncStatus:
|
||||
"""In-memory state of the (at most one) in-flight sync run.
|
||||
|
||||
``state`` is a four-state machine: ``idle`` (never run / reset),
|
||||
``running``, ``success``, ``failed``. Terminal states carry the run's
|
||||
``detail`` (success) or ``error`` (failure) so the UI can render the
|
||||
last result after a page reload (task 02's re-attach behavior).
|
||||
``state`` is a five-state machine: ``idle`` (never run / reset),
|
||||
``running``, ``success``, ``failed``, and ``cancelled`` (phase 134,
|
||||
task 01 — terminal like ``success`` / ``failed``: ``finished_at``
|
||||
set, ``current_file`` / ``phase`` / ``current_summary`` cleared, the
|
||||
final file + summary counts kept, but ``error = None`` and
|
||||
``detail = {}`` — a cancel is intent, not failure). Terminal states
|
||||
carry the run's ``detail`` (success) or ``error`` (failure) so the UI
|
||||
can render the last result after a page reload (task 02's re-attach
|
||||
behavior).
|
||||
|
||||
Phase 64 (task 02) progress fields: ``current_file`` is the
|
||||
``source/relative/path`` the import is processing right now (null
|
||||
@@ -201,14 +260,14 @@ class SyncStatus:
|
||||
contract). They are 0/0 before the first batch, during the
|
||||
``"overview"`` / ``"summaries"`` phases, and in terminal states —
|
||||
the import phase clears them at every point it ends (the
|
||||
overview/summaries transitions + both terminals, where
|
||||
overview/summaries transitions + every terminal, where
|
||||
``current_file`` also clears). The phase-64 keep-final-counts
|
||||
convention covers the FILE counts only — the chunk position is a
|
||||
sub-position of a file that's done, so it clears in the terminal
|
||||
while ``files_done`` / ``files_total`` keep their final values.
|
||||
"""
|
||||
|
||||
state: Literal["idle", "running", "success", "failed"] = "idle"
|
||||
state: Literal["idle", "running", "success", "failed", "cancelled"] = "idle"
|
||||
started_at: datetime | None = None
|
||||
finished_at: datetime | None = None
|
||||
detail: dict[str, Any] = field(default_factory=dict)
|
||||
@@ -236,6 +295,12 @@ class SyncStatus:
|
||||
|
||||
_status = SyncStatus()
|
||||
_task: asyncio.Task[None] | None = None
|
||||
# Phase 134 (task 01): the cancel request — set by POST
|
||||
# /api/sync/cancel while a run is in flight; _run_sync's boundaries
|
||||
# + its ``except CancelledError`` clause read it (the module
|
||||
# docstring's cancel paragraph). Cleared by start_sync when a run
|
||||
# starts, so a stale event can never bleed into the next run.
|
||||
_cancel_event = asyncio.Event()
|
||||
|
||||
|
||||
@router.get("/status")
|
||||
@@ -257,6 +322,11 @@ def sync_status() -> dict[str, Any]:
|
||||
terminal states; while in ``"summaries"``, ``current_summary`` +
|
||||
``summaries_done`` / ``summaries_total`` carry the generator's
|
||||
position (the terminal keeps the final summary counts).
|
||||
|
||||
Phase 134 (task 01): ``state`` also reports ``"cancelled"`` for a
|
||||
user-cancelled run — a terminal state with ``error`` null and
|
||||
``detail`` empty; the run's final file + summary counts are kept,
|
||||
the chunk position cleared.
|
||||
"""
|
||||
return {
|
||||
"state": _status.state,
|
||||
@@ -270,7 +340,7 @@ def sync_status() -> dict[str, Any]:
|
||||
# Phase 129 (task 03): the in-flight file's embed-chunk
|
||||
# position — the dataclass defaults make idle 0/0, and the
|
||||
# runner clears it at every point the import phase ends
|
||||
# (overview/summaries transitions + both terminals).
|
||||
# (overview/summaries transitions + every terminal).
|
||||
"current_chunk": _status.current_chunk,
|
||||
"chunks_total": _status.chunks_total,
|
||||
# Phase 98 (task 01): the phase machine — null/0/0 idle (the
|
||||
@@ -291,22 +361,77 @@ async def start_sync() -> dict[str, str]:
|
||||
loop. 409 when a run is already in flight (one sync at a time — the
|
||||
status endpoint is the single source of truth for the run, and the
|
||||
UI re-attaches to it rather than starting a second one).
|
||||
|
||||
Phase 134 (task 01): the run's cancel request resets with the run —
|
||||
``_cancel_event.clear()`` before the task is created, so a stale
|
||||
event from a previously cancelled run can never bleed into this one.
|
||||
"""
|
||||
global _task
|
||||
if _task is not None and not _task.done():
|
||||
raise HTTPException(status_code=409, detail="a sync is already running")
|
||||
_cancel_event.clear()
|
||||
_task = asyncio.create_task(_run_sync())
|
||||
return {"detail": "sync started"}
|
||||
|
||||
|
||||
@router.post("/cancel", status_code=202)
|
||||
async def cancel_sync() -> dict[str, str]:
|
||||
"""Request the cancel of the in-flight run (phase 134, task 01).
|
||||
|
||||
202 + ``cancel requested`` is answered the moment the request is
|
||||
REGISTERED: the module ``_cancel_event`` is set (the runner's
|
||||
``_check_cancelled`` boundaries + its ``except CancelledError``
|
||||
conversion see it) AND ``_task.cancel()`` unblocks whatever
|
||||
``await`` the pipeline is parked on (e.g. a per-file LLM call
|
||||
inside the import). The run may already be terminal by the next
|
||||
status poll — the status object, not the task, is authoritative,
|
||||
so the UI's poll settles on the terminal (``cancelled``) state.
|
||||
409 ``no sync is running`` when there is no run in flight (the
|
||||
``_task`` is None or done — the harmless race the UI's poll
|
||||
settles; no error copy, no state change).
|
||||
"""
|
||||
if _task is None or _task.done():
|
||||
raise HTTPException(status_code=409, detail="no sync is running")
|
||||
_cancel_event.set()
|
||||
_task.cancel()
|
||||
return {"detail": "cancel requested"}
|
||||
|
||||
|
||||
def _check_cancelled() -> None:
|
||||
"""Phase 134 (task 01): the cooperative cancel boundary.
|
||||
|
||||
Raises ``asyncio.CancelledError`` when the module ``_cancel_event``
|
||||
is set (the user requested a stop — ``POST /api/sync/cancel``);
|
||||
:func:`_run_sync`'s ``except asyncio.CancelledError`` clause
|
||||
converts it into the ``cancelled`` terminal state. Called at the
|
||||
pipeline boundaries (the module docstring's cancel paragraph) —
|
||||
the top of the per-source loop, before ``import_sources``, and
|
||||
before the overview + summaries span. A git subprocess in flight
|
||||
at the cancel moment runs to completion (no await inside
|
||||
``subprocess.run``); the cancel lands at the NEXT boundary.
|
||||
"""
|
||||
if _cancel_event.is_set():
|
||||
raise asyncio.CancelledError()
|
||||
|
||||
|
||||
async def _run_sync() -> None:
|
||||
"""The full sync pipeline, one in-process background task.
|
||||
|
||||
Every failure mode (git, embeddings, anything else) lands in the
|
||||
``failed`` state with a sanitized ``error`` string — a background
|
||||
task must die in state, never as an unobserved exception.
|
||||
``CancelledError`` is deliberately *not* caught: app shutdown
|
||||
cancels the task, and swallowing that would mask a real stop.
|
||||
|
||||
Cancellation (phase 134, task 01 — the module docstring's cancel
|
||||
paragraph): the cooperative boundaries (:func:`_check_cancelled`
|
||||
at the top of the per-source loop, before ``import_sources``, and
|
||||
before the overview + summaries span) plus the ``except
|
||||
asyncio.CancelledError`` clause convert a USER-requested cancel
|
||||
(``_cancel_event`` set by ``POST /api/sync/cancel``) into the
|
||||
``cancelled`` terminal state + one INFO line, then re-raise (the
|
||||
task ends cancelled — the status object stays authoritative). An
|
||||
UNREQUESTED cancel (the app-shutdown path — the event unset)
|
||||
re-raises unchanged: a background task is never swallowed, and
|
||||
swallowing a real stop would mask it.
|
||||
"""
|
||||
_status.state = "running"
|
||||
_status.started_at = datetime.now(UTC)
|
||||
@@ -363,6 +488,11 @@ async def _run_sync() -> None:
|
||||
include_hidden_by_root: dict[str, bool] = {}
|
||||
doc_dates_by_root: dict[str, dict[str, datetime]] = {}
|
||||
for row in rows:
|
||||
# Phase 134 (task 01): the per-source boundary — a cancel
|
||||
# registered while the previous source's git ops run (a
|
||||
# blocking subprocess runs to completion) lands here,
|
||||
# before this source's clone/pull or local-dir check.
|
||||
_check_cancelled()
|
||||
if row.kind == "git":
|
||||
# Phase 121: the token column is injected into the clone
|
||||
# URL ONLY here (clone_url_for — NULL token → the bare
|
||||
@@ -424,12 +554,22 @@ async def _run_sync() -> None:
|
||||
# starts NOW (the model-check + clone/pull prelude above
|
||||
# reported ``phase: null``).
|
||||
_status.phase = "import"
|
||||
# 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
|
||||
# imported.
|
||||
_check_cancelled()
|
||||
summary: ImportSummary = await import_sources(
|
||||
sources, llm, prune=True, progress=_hook, on_chunk_progress=_chunk_hook,
|
||||
ignore_by_root=ignore_by_root,
|
||||
include_hidden_by_root=include_hidden_by_root,
|
||||
doc_dates_by_root=doc_dates_by_root,
|
||||
)
|
||||
# Phase 134 (task 01): the overview + summaries boundary — a
|
||||
# cancel during the import span (the endpoint's _task.cancel()
|
||||
# interrupts at the next per-file await) or registered anywhere
|
||||
# before this point lands here, before the lite spans start.
|
||||
_check_cancelled()
|
||||
overview = False
|
||||
if summary.added + summary.updated > 0:
|
||||
_status.phase = "overview" # phase 98 (task 01)
|
||||
@@ -574,6 +714,34 @@ async def _run_sync() -> None:
|
||||
"sources_version": sources_version,
|
||||
}
|
||||
logger.info("sync: done detail=%s", _status.detail)
|
||||
except asyncio.CancelledError:
|
||||
# Phase 134 (task 01): the TWO conversion paths (the module
|
||||
# docstring's cancel paragraph). A user-requested cancel —
|
||||
# ``_cancel_event`` set by POST /api/sync/cancel, whether the
|
||||
# endpoint's ``_task.cancel()`` interrupted a parked await or a
|
||||
# ``_check_cancelled`` boundary raised — lands the run in the
|
||||
# ``cancelled`` terminal and re-raises (the task ends
|
||||
# cancelled — ``_task.done()`` is True, the status object stays
|
||||
# authoritative). An unrequested cancel (the app-shutdown path
|
||||
# — the event unset) re-raises unchanged: never swallow a real
|
||||
# stop.
|
||||
if _cancel_event.is_set():
|
||||
_status.state = "cancelled"
|
||||
_status.finished_at = datetime.now(UTC)
|
||||
_status.error = None # a cancel is intent, not failure
|
||||
_status.current_file = None # keep the final counts
|
||||
_status.phase = None # keep the final summary counts
|
||||
_status.current_summary = None
|
||||
# Phase 129 (task 03): the chunk position is a sub-position
|
||||
# of a file that's done — it clears in EVERY terminal, like
|
||||
# success and failed (the convention split, pinned).
|
||||
_status.current_chunk = 0
|
||||
_status.chunks_total = 0
|
||||
logger.info(
|
||||
"sync: cancelled by user state=cancelled files_done=%d files_total=%d",
|
||||
_status.files_done, _status.files_total,
|
||||
)
|
||||
raise
|
||||
except Exception as e: # noqa: BLE001 — a background task dies in state, see above
|
||||
logger.exception("sync: failed")
|
||||
_status.state = "failed"
|
||||
|
||||
Reference in New Issue
Block a user