From 89dcf5f48ebab1854e162ae4a2df7dec7c7c67ed Mon Sep 17 00:00:00 2001 From: IK Date: Tue, 22 Sep 2026 08:17:47 +0700 Subject: [PATCH] fix: make screenshot retry idempotent --- apps/api/app/routers/submissions.py | 20 +++++++++++++++----- apps/api/tests/test_api.py | 11 +++++++++-- apps/web/src/pages/api/report-screenshot.ts | 5 ++++- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/apps/api/app/routers/submissions.py b/apps/api/app/routers/submissions.py index f0a1aff..099f107 100644 --- a/apps/api/app/routers/submissions.py +++ b/apps/api/app/routers/submissions.py @@ -19,7 +19,7 @@ from ..dependencies import Db from ..importer import normalize from ..models import Bait, BaitKind, CatchReport, Fish, ModerationStatus, SourceType, Spot, SubmissionAttempt, Waterbody from ..schemas import CatchReportAccepted, CatchReportCreate -from ..storage import ScreenshotError, upload_screenshot +from ..storage import ScreenshotError, delete_screenshot, upload_screenshot from ..submission_security import check_rate_limit from ..tackle_components import replace_tackle_components from rf4_research.gear_components import from_catch_fields @@ -77,21 +77,31 @@ def create_catch_report(payload: CatchReportCreate, request: Request, db: Db, id @router.post("/api/v1/catch-reports/{report_id}/screenshot", status_code=204, response_class=Response) def add_screenshot(report_id: UUID, db: Db, screenshot: UploadFile = File(), upload_token: Annotated[str | None, Header(alias="X-Upload-Token")] = None) -> Response: - report = db.get(CatchReport, report_id) + report_query = select(CatchReport).where(CatchReport.id == report_id) + if db.bind is not None and db.bind.dialect.name == "postgresql": + report_query = report_query.with_for_update() + report = db.scalar(report_query) if report is None or report.source_type != SourceType.user or report.moderation_status != ModerationStatus.pending: raise HTTPException(status_code=404, detail="pending catch report not found") supplied_hash = hashlib.sha256((upload_token or "").encode()).hexdigest() if not report.screenshot_upload_token_hash or not hmac.compare_digest(report.screenshot_upload_token_hash, supplied_hash): raise HTTPException(status_code=401, detail="invalid screenshot upload token") if report.screenshot_key: - raise HTTPException(status_code=409, detail="screenshot already uploaded") + return Response(status_code=204) raw = screenshot.file.read(settings.screenshot_max_bytes + 1) try: report.screenshot_key = upload_screenshot(raw, filename=screenshot.filename, content_type=screenshot.content_type) except ScreenshotError as exc: raise HTTPException(status_code=422, detail=str(exc)) from exc - report.screenshot_upload_token_hash = None - db.commit() + try: + db.commit() + except Exception as exc: + db.rollback() + try: + delete_screenshot(report.screenshot_key) + except Exception: + logger.exception("failed to remove orphaned screenshot", extra={"screenshot_key": report.screenshot_key}) + raise HTTPException(status_code=503, detail="screenshot could not be saved; please retry") from exc return Response(status_code=204) diff --git a/apps/api/tests/test_api.py b/apps/api/tests/test_api.py index e5034a7..f47db47 100644 --- a/apps/api/tests/test_api.py +++ b/apps/api/tests/test_api.py @@ -528,14 +528,21 @@ def test_admin_can_start_and_list_official_import(monkeypatch) -> None: def test_pending_report_accepts_one_validated_screenshot(monkeypatch) -> None: created = client.post("/api/v1/catch-reports", json={"fish_slug": "pike", "waterbody_slug": "test-lake", "x": 91, "y": 92, "weight_g": 4200}).json() - monkeypatch.setattr("app.routers.submissions.upload_screenshot", lambda raw, **metadata: "reports/test.jpg" if raw == b"image-bytes" and metadata == {"filename": "catch.jpg", "content_type": "image/jpeg"} else "unexpected") + uploaded: list[bytes] = [] + def fake_upload(raw, **metadata): + if raw == b"image-bytes" and metadata == {"filename": "catch.jpg", "content_type": "image/jpeg"}: + uploaded.append(raw) + return "reports/test.jpg" + return "unexpected" + monkeypatch.setattr("app.routers.submissions.upload_screenshot", fake_upload) upload_url = f"/api/v1/catch-reports/{created['id']}/screenshot" assert client.post(upload_url, files={"screenshot": ("catch.jpg", b"image-bytes", "image/jpeg")}).status_code == 401 assert client.post(upload_url, headers={"X-Upload-Token": "wrong"}, files={"screenshot": ("catch.jpg", b"image-bytes", "image/jpeg")}).status_code == 401 response = client.post(upload_url, headers={"X-Upload-Token": created["screenshot_upload_token"]}, files={"screenshot": ("catch.jpg", b"image-bytes", "image/jpeg")}) assert response.status_code == 204 reused = client.post(upload_url, headers={"X-Upload-Token": created["screenshot_upload_token"]}, files={"screenshot": ("catch.jpg", b"image-bytes", "image/jpeg")}) - assert reused.status_code == 401 + assert reused.status_code == 204 + assert uploaded == [b"image-bytes"] def test_idempotency_replay_survives_completed_screenshot(monkeypatch) -> None: diff --git a/apps/web/src/pages/api/report-screenshot.ts b/apps/web/src/pages/api/report-screenshot.ts index 30b95c2..b31ef14 100644 --- a/apps/web/src/pages/api/report-screenshot.ts +++ b/apps/web/src/pages/api/report-screenshot.ts @@ -12,7 +12,10 @@ export const POST: APIRoute = async ({ request, redirect, cookies }) => { try { const upload = new FormData(); upload.set("screenshot", screenshot); const response = await fetch(`${base}/api/v1/catch-reports/${reportId}/screenshot`, { method: "POST", headers:{"X-Upload-Token":uploadToken}, body: upload, signal: AbortSignal.timeout(60_000) }); - if (response.ok) cookies.delete(cookieName, {path:"/"}); + if (response.ok) { + cookies.delete(cookieName, {path:"/"}); + cookies.delete("rf4-idempotency-key", {path:"/"}); + } return redirect(response.ok ? "/report?state=screenshot_sent" : `/report?state=screenshot_error&report_id=${encodeURIComponent(reportId)}`, 303); } catch (err) { const isTimeout = err instanceof DOMException && err.name === "TimeoutError" ||