fix(execution): the CAS-won terminal owns the paired activity close (#1804)

[AndriiPasternak31]

Fixes #1804

Problem

Every terminal writer outside the dispatching coroutine wrote the terminal to schedule_executions and walked away. The paired agent_activities row stayed activity_state='started' — the Dashboard Timeline rendered the agent as still working (ReplayTimeline.vue) until the generic 120-minute sweep closed it with a fabricated duration_ms = now − started_at that nothing ever recomputes. A 15-minute run was permanently recorded as a ~120-minute failure.

The close was gated on holding the activity_id local and winning the CAS in the same coroutine, so it failed two independent ways: process death (no finally, the id dies with the coroutine) and CAS loss (_write_terminal_and_gate had no lost-CAS branch, unlike the SUCCESS applier).

Third appearance of the class after #45 (tool-call activities) and #767 (CB probes) — both patched a single producer and left the ownership model alone.

Fix — move the responsibility to whoever wins the CAS

One owner: activity_service.close_execution_activity(...) (+ a sync spawn_close_execution_activity wrapper for the pull sink), structurally the twin of event_dispatch_service.spawn_task_terminal_event. It maps the terminal via the shared models.activity_state_for_terminal (#1332 — never a second mapping), delegates to complete_activity so the agent_activity WS broadcast survives, and is fail-open: it runs after a committed terminal and can never affect it.

The close is itself a CAS. db.complete_activity now returns ActivityCloseOutcome (UPDATED/ALREADY_CLOSED/NOT_FOUND) — one bool cannot answer both "did this row exist" (routers/internal.py 404s on it) and "did anything change" (the broadcast gate), and once idempotent no-op closes are designed behaviour the two answers diverge routinely. "The CAS winner owns it" only holds if a second closer trying is safe.

The lattice is the authority ordering, stated literally (started < failed < {completed, cancelled}): an authoritative COMPLETED accepts started|failed (so the #1083 late-SUCCESS-after-lease-expiry path may upgrade a provisional FAILED); CANCELLED/FAILED accept started only. The COMPLETED arm is deliberately tighter than the execution row's own != CANCELLED — see the review note below. The lookup is widened to agree (include_failed=True for authoritative terminals); a lookup narrower than the write would make the whole fix inert.

Split by cardinality: single-row recovery paths use the per-row helper (WS broadcast preserved); the two bulk sweeps use db.close_open_activities_for_executions (set-wise, one transaction, chunked). _close_bulk_swept_activities is a sibling of _emit_bulk_terminal_events, never folded into it — that method short-circuits on has_task_terminal_subscribers() is False, which would skip the close on every install with no event subscribers.

Wired writers: _write_terminal_and_gate (won and lost), apply_result (both branches), both backend-shutdown CancelledError handlers, watchdog + startup _recover_execution (the latter's CAS bool was previously discarded), the two bulk sweeps, the lease reaper (park → FAILED, re-queue → CANCELLED: a superseded attempt is not a failure), the pull sink, and terminate_execution.

The parity guard (test_1804_terminal_activity_parity.py) is anchored on terminal writes, not on #1578 event emission — the two backend-shutdown handlers write a terminal and emit nothing, which is exactly how they hid.

Review findings fixed in-branch

1. The lattice inherited the upstream's permissive edge — the bug it exists to prevent. The predicate was specified as an ordering but implemented as an exclusion copied literally from the execution CAS (!= 'cancelled'), and those disagree exactly on the reflexive case: != 'cancelled' matches 'completed'. A second COMPLETED close re-dated completed_at/duration_ms/error — a 15-minute activity became ~9h, i.e. the bug: recovery paths write execution terminals without closing the paired activity — agent shows as running for up to 2h, then logs a fabricated 120-min failure #1804 symptom arriving through bug: recovery paths write execution terminals without closing the paired activity — agent shows as running for up to 2h, then logs a fabricated 120-min failure #1804's own fix, reachable from the lost-CAS branch (which passes an explicit activity_id, so the narrower lookup cannot shield it). The literal copy was faithful; the execution row survives that edge only because the refactor: fire-and-forget dispatch — a hung turn holds zero backend resource #1083 callback's replay short-circuit stops the duplicate upstream — a protection the activity's call sites don't have.
2. A CAS loss to a successful row stamped "superseded by ..." onto a clean success and rendered the enum repr (str, Enum, the feat: system-emitted agent.task.completed/failed events at execution terminal (async caller report-back) #1578 footgun). Now error=None on an authoritative SUCCESS close, .value for the label.
3. _recover_execution returning the CAS bool changed what a False means to recover_orphaned_executions, which counted every False as errors. A lost CAS is RELIABILITY-005 working as designed, so it's now partitioned into cas_lost; genuine exceptions self-count.

Observability

CleanupReport.activities_closed_on_recovery. stale_activities should trend to ~0 while this picks up the volume — a non-zero stale_activities now means a producer is still unowned. The 120-minute backstop is demoted to a backstop for the unclaimed and moved to run after _sweep_stale_slots (it used to run one line before the reaper that legitimately closes activities, so within a single cycle the duration fabricator could beat a real closer). #429 deletes the backstop entirely — a contract survives that, per-site patches would not.

Scope note

db/schedules/cleanup.py::mark_no_session_executions_failed now returns the CAS-won count rather than the candidate count (it previously over-reported report.no_session_executions by counting rows whose guarded UPDATE lost). Its sibling mark_stale_executions_failed already did this — a deliberate, visible value change.

Verification

• tests/unit/ full suite, 5143 passed / 14 skipped (alphabetical order and --randomly-seed=12345); 58 new tests across 3 files.
• tests/lint_sys_modules.py: no new violations (test_reset_preserve_state_guardrails.py 9 → 0).
• Requirements-structure guard: clean. No schema change, no migration, no feature flag.

Pre-existing flake, not from this branch: under some orderings test_1081_physical_meter.py::{test_max_respects_506_clamp,test_available_floors_at_zero_over_ceiling}[sqlite] fail with assert 5 == 2 — its set_ceiling monkeypatch doesn't reach clamp_to_ceiling. Attribution was done by replaying the literal failing order on pristine origin/dev (seeds aren't comparable across trees once the collected set changes): dev fails identically. This branch only shifts which seeds surface it. Worth a separate issue.

🤖 Generated with Claude Code

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[vybe]

Validated via /validate-pr. Fixes #1804 with a closing keyword. The close-lattice CAS (db/activities.py::_close_predicate) mirrors the execution-row CAS with a correctly-argued tightening (no COMPLETED->COMPLETED re-date), close_execution_activity is the single owner with tri-state outcome semantics (ALREADY_CLOSED as designed no-op), and include_failed=True is applied exactly on the late-SUCCESS callback path. Named #1804 regression suites (R1-R5, chain, parity) present; docs complete; full pytest matrix + pg-migrations green; security scan clean. Will resolve the expected docs/routers conflicts against dev after #1867 lands, then merge.