SEP-1835: Keep the outcome the sync resolved when a run is stopped

[marcuscruz-percona]

Summary

• BaseExecutor.stop_task synced the task history against the backend and then unconditionally stamped STOPPED with a fresh finished_at, discarding the outcome the sync had just derived. It now persists what the sync resolved: a terminal status and the finish time that came with it stand as they are, STOPPED is stamped only for a run the sync left non-terminal, and finished_at is filled only when it is still unset.
• Exactly one terminal PMM annotation is emitted per stop, and it names the status that was actually persisted — previously a stop on a failed run emitted FAILED (from sync_task_history) and a contradicting STOPPED.
• Tests: base-executor coverage for every terminal status the sync can return, finish-time preservation, the terminal-without-finished_at case, backend-stop failure, an end-to-end Nomad reproduction (failed allocation + stop), and a Celery non-regression that the stop still terminates the row.

Why it was wrong

The stop route only accepts a RUNNING row (app/tasks/routes.py), so the reproduction window is exactly "the payload already exited, but the periodic sync has not moved the row out of RUNNING yet". In that window:

1. A failed run was persisted as STOPPED — history and UI reported an operator action while the PagerDuty incident (raised inside sync_task_history, before the overwrite) said the run failed.
2. The real finish time was lost. The Nomad executor stamps finished_at from the allocation's ModifyTime; the stop path replaced it with the stop-request wall clock, which also corrupts TaskHistory.duration.
3. PMM got two contradictory annotations for the same run: the derived event from the sync, then STOPPED from the stop path.

The Nomad executor already implements the opposite contract deliberately — _apply_terminal_status downgrades to STOPPED only when the derived status is not FAILED, "so the failure is not relabelled as an operator action". The base class threw that decision away one frame up: the guard existed and was overwritten by its own caller. Who wins between the stop and the run's own result stays owned by the executor's derivation, which is already correct; the base class now just records it.

The change

queue_item = await self.sync_task_history(queue_item)
sync_resolved_it = queue_item.status.is_terminal()
if not sync_resolved_it:
    queue_item.status = TaskHistoryStatusEnum.STOPPED
if queue_item.finished_at is None:
    queue_item.finished_at = utc_now()
event = _TERMINAL_STATUS_EVENT_MAP[queue_item.status]
saved = await TaskHistoryManager.save(session, queue_item)
# A run the sync found running and resolved is already annotated by it.
if not (was_running and sync_resolved_it):
    await session.refresh(saved, attribute_names=["execution_request"])
    schedule_annotation(saved, event)

Notes on the details:

• The rule is "do not override a terminal status", expressed with TaskHistoryStatusEnum.is_terminal() rather than a hand-listed set of statuses the stop path may keep. _TERMINAL_STATUS_EVENT_MAP is then indexed (not .get()), so a future terminal status added without an annotation event fails loudly instead of being silently relabelled STOPPED. A new test pins set(_TERMINAL_STATUS_EVENT_MAP) == {s for s in TaskHistoryStatusEnum if s.is_terminal()} so that drift is caught in CI rather than at runtime.
• finished_at is filled only where the sync did not establish one. This is load-bearing beyond preservation: _sync_task_history resolves LOST on JobNotFoundError without stamping a finish time, so leaving it None would produce a terminal row with no duration.
• The annotation guard widens from "the sync returned STOPPED" to "the sync already emitted a terminal event", and the emitted event is read before TaskHistoryManager.save so it does not depend on what the post-save refresh returns.
• Non-regression that carries the most weight: CeleryExecutor._sync_task_history returns the record unchanged, so under Celery the row is still RUNNING when stop_task runs and the removed assignment was the only thing making it terminal. The not sync_resolved_it branch is what keeps every Celery stop from wedging in RUNNING; test_stop_reaches_stopped_status covers it explicitly.

Test changes

tests/app/tasks/execution/test_models.py — TestStopTask now drives a StopStubExecutor whose backend stop and executor-specific sync report a configured state, so stop_task runs against real persistence (real TaskHistoryManager.save / session.refresh, real sync_task_history including its own annotation) with PMM as the only patched boundary. That replaces four near-identical fake_sync closures plus a three-patch harness repeated per test. New cases:

• every terminal status the sync can return (FAILED, SUCCESS, LOST, STALE) is preserved, verified after session.rollback() + refetch, with a single annotation naming the derived event and never STOPPED;
• the sync's finished_at survives instead of being replaced by the stop-request wall clock;
• a terminal status with no finish time still gets one (the LOST shape);
• when the row was not RUNNING and the sync returns terminal, the stop path owns the only emit and it names the persisted outcome;
• a backend that raises on stop propagates before anything is written — the sync is never reached, no annotation is sent, and the row is still RUNNING.

nomad/test_models.py adds the end-to-end reproduction (failed allocation with ModifyTime, job dead/Stop: true, Nomad as the only mocked boundary): the row persists FAILED, finished_at comes from ModifyTime, and PMM hears FAILED once. celery/test_models.py adds the wedge-in-RUNNING guard. TestStopTaskRegression (the MissingGreenlet guard on the explicit session.refresh before schedule_annotation) is untouched and still passes.

Out of scope

• Broadening the stop route's RUNNING-only precondition.
• Any change to how an executor derives status, including Nomad's stop-beats-success rule.
• Backfilling rows already persisted STOPPED by this defect — the derived status was never written anywhere, so it cannot be recovered after the fact. Called out in the changelog fragment.

No auth or authorization surface changes: the route keeps its dependency and its RUNNING-only precondition, so no new caller can reach the new branch. This is record-correctness only.

Tested

• make test — 9897 passed, 424 skipped
• make lint — clean
• Dispatch a task whose payload exits non-zero, POST /history/{id}/stop/ inside the window before the periodic sync, then read the row back: status is failed, finished_at is the payload exit time, duration is the real run length, and PMM shows one FAILED annotation for the run.
• Stop a still-running task: row reaches stopped with a finish time and a single STOPPED annotation.

Checklist

• New/modified functions have type hints and rST docstrings
• New tests added for new features or bug fixes
• All tests pass locally (make test)
• Pre-commit hooks pass (make run-pre-commit)
• Database migrations generated if models changed (make makemigrations) — N/A, no model changes
• User-facing changes documented (README, inline help, UI text) — N/A, no UI or config surface change
• Configuration changes documented with examples — N/A
• Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A

[Copilot]

Pull request overview

Corrects task stopping so executor-resolved terminal outcomes and finish times are preserved, with one matching PMM annotation.

Changes:

• Preserves terminal status and finished_at after synchronization.
• Adds Nomad, Celery, and base-executor regression coverage.
• Documents the user-visible correction.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
app/tasks/execution/models.py	Preserves synchronized terminal outcomes during stops.
tests/app/tasks/execution/test_models.py	Covers status, timestamp, annotation, and failure behavior.
tests/app/tasks/execution/executors/nomad/test_models.py	Reproduces the Nomad failure race.
tests/app/tasks/execution/executors/celery/test_models.py	Ensures Celery stops still terminate rows.
changelog.d/SEP-1835.fixed.md	Records the user-visible fix.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
app/sep
inventory.py
app/sep/sync/syncers
pmm.py
app/tasks/execution
models.py
Project Total

This report was generated by python-coverage-comment-action