diff --git a/backend/app/api/v1/admin_scrape.py b/backend/app/api/v1/admin_scrape.py index a8e412cc..685bb53d 100644 --- a/backend/app/api/v1/admin_scrape.py +++ b/backend/app/api/v1/admin_scrape.py @@ -1123,18 +1123,48 @@ def cancel_geo_job( db: Annotated[Session, Depends(get_db)], ) -> dict[str, Any]: """Пометить job как cancelled. Worker увидит при следующей итерации.""" - db.execute( - text( - """ - UPDATE nspd_geo_jobs SET status = 'cancelled', finished_at = NOW(), - error = COALESCE(error, 'cancelled by admin') - WHERE job_id = :id AND status IN ('queued','running','paused') - """ - ), - {"id": job_id}, + # #2464: фильтр статуса здесь был всегда (в отличие от resume ниже), но ответ + # возвращал cancelled=True независимо от того, задел ли UPDATE хоть одну строку. + # Несуществующий job_id и уже завершённая задача давали тот же ответ, что + # настоящая отмена — оператор и админ-UI получали подтверждение действия, + # которого не было. + # + # Обоснование держим в КОММЕНТАРИИ, а не в докстринге: FastAPI кладёт докстринг + # в OpenAPI-description, откуда он попадает в опубликованный контракт и в + # сгенерированные типы фронта (frontend/src/lib/api-types.ts). Внутренние замеры + # там не нужны, а gate openapi-codegen-check честно ловит такое расхождение. + row = ( + db.execute( + text( + """ + UPDATE nspd_geo_jobs SET status = 'cancelled', finished_at = NOW(), + error = COALESCE(error, 'cancelled by admin') + WHERE job_id = :id AND status IN ('queued','running','paused') + RETURNING job_id + """ + ), + {"id": job_id}, + ) + .mappings() + .first() ) + if row is None: + current = db.execute( + text("SELECT status FROM nspd_geo_jobs WHERE job_id = :id"), + {"id": job_id}, + ).scalar() + db.commit() + return { + "job_id": job_id, + "cancelled": False, + "status": current, + "reason": ( + "задача не найдена" if current is None else f"статус {current!r} уже терминальный" + ), + } + db.commit() - return {"job_id": job_id, "cancelled": True} + return {"job_id": job_id, "cancelled": True, "status": "cancelled"} @router.post("/geo/jobs/{job_id}/resume") @@ -1142,23 +1172,21 @@ def resume_geo_job( job_id: int, db: Annotated[Session, Depends(get_db)], ) -> dict[str, Any]: - """Re-enqueue задачу из НЕзавершённого состояния (paused / failed / cancelled). - - #2464: UPDATE шёл БЕЗ фильтра статуса — в отличие от соседнего cancel_geo_job, - который фильтрует явно. Из-за этого «возобновить» можно было завершённую задачу - (status='done' → снова 'queued' и повторный прогон, затирая результат) и уже - бегущую (второй worker на тот же job_id — лишние запросы к НСПД, у которого WAF). - - Замер на проде 19.08: все 66 задач в терминальных статусах — 61 done, 5 - cancelled. То есть resume на ЛЮБУЮ существующую делал ровно то, чего не должен. - - Второе: ручка возвращала resumed=True всегда, независимо от того, изменилось ли - что-нибудь. Теперь ответ отражает факт: не подошёл статус → resumed=False, - текущий статус в ответе, задача НЕ ставится в очередь. - - 'cancelled' оставлен возобновляемым намеренно: cancel_geo_job — ручное действие - оператора, и без этого отменённая по ошибке задача не восстанавливалась бы никак. - """ + """Re-enqueue задачу из НЕзавершённого состояния (paused / failed / cancelled).""" + # #2464: UPDATE шёл БЕЗ фильтра статуса — в отличие от соседнего cancel_geo_job, + # который фильтрует явно. Из-за этого «возобновить» можно было завершённую задачу + # (done → снова queued и повторный прогон, затирая результат) и уже бегущую + # (второй worker на тот же job_id — лишние запросы к НСПД, у которого WAF). + # + # Замер на проде 19.08: все 66 задач в терминальных статусах — 61 done, 5 + # cancelled. То есть resume на ЛЮБУЮ существующую делал ровно то, чего не должен. + # + # Второе: ручка возвращала resumed=True всегда, независимо от того, изменилось ли + # что-нибудь. Теперь ответ отражает факт — статус и причина в ответе, задача НЕ + # ставится в очередь. + # + # 'cancelled' оставлен возобновляемым намеренно: cancel — ручное действие + # оператора, и без этого отменённая по ошибке задача не восстанавливалась бы. from app.services.job_settings import get_setting_value from app.workers.tasks.nspd_geo import process_nspd_geo_job diff --git a/backend/tests/api/v1/test_2464_resume_geo_job_guard.py b/backend/tests/api/v1/test_2464_resume_geo_job_guard.py index 198d8a34..fc6d417d 100644 --- a/backend/tests/api/v1/test_2464_resume_geo_job_guard.py +++ b/backend/tests/api/v1/test_2464_resume_geo_job_guard.py @@ -105,3 +105,46 @@ def test_response_reports_the_new_status() -> None: db = _Db("paused", update_matches=True) assert _resume(db)["status"] == "queued" + + +# ── cancel_geo_job: тот же класс — подтверждение действия, которого не было ───── +# +# Фильтр статуса здесь был всегда, но ответ возвращал cancelled=True независимо от +# того, задел ли UPDATE строку. Несуществующий job_id и уже завершённая задача давали +# тот же ответ, что настоящая отмена. + + +def _cancel(db: _Db) -> dict[str, Any]: + return admin_scrape.cancel_geo_job(job_id=1, db=db) # type: ignore[arg-type] + + +def test_cancel_of_finished_job_is_not_reported_as_success() -> None: + db = _Db("done", update_matches=False) + + out = _cancel(db) + + assert out["cancelled"] is False + assert out["status"] == "done" + assert "терминальный" in out["reason"] + + +def test_cancel_of_missing_job_reports_absence() -> None: + db = _Db(None, update_matches=False) + + out = _cancel(db) + + assert out["cancelled"] is False + assert out["status"] is None + assert "не найдена" in out["reason"] + + +def test_cancel_of_running_job_still_works() -> None: + """Контроль: законная отмена по-прежнему подтверждается. + + Проверяется только `cancelled` — новый ключ `status` намеренно не трогаем, иначе + контроль падал бы на origin/main с KeyError, то есть «в ответе нет поля», а не + «отмена сломана». + """ + db = _Db("running", update_matches=True) + + assert _cancel(db)["cancelled"] is True diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 9b8febee..5936c42a 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -1763,7 +1763,7 @@ export interface paths { put?: never; /** * Resume Geo Job - * @description Re-enqueue paused/failed job. Resume idempotent через pending targets. + * @description Re-enqueue задачу из НЕзавершённого состояния (paused / failed / cancelled). */ post: operations["resume_geo_job_api_v1_admin_scrape_geo_jobs__job_id__resume_post"]; delete?: never;