From de596f397f039f297d5fce2512308553340c6e9a Mon Sep 17 00:00:00 2001 From: timmy Date: Mon, 10 Aug 2026 09:54:56 +0000 Subject: [PATCH] fix: make screenshot uploads retry-safe (#471) --- frontend/dashboard.js | 6 +- frontend/issue-attachment.js | 10 +++ src/main.py | 33 +++++--- tests/test_issue_attachment_ui.py | 29 +++++++ tests/test_issue_attachments.py | 122 ++++++++++++++++++++++++++++++ 5 files changed, 190 insertions(+), 10 deletions(-) diff --git a/frontend/dashboard.js b/frontend/dashboard.js index 5aedcb9..eef37ae 100644 --- a/frontend/dashboard.js +++ b/frontend/dashboard.js @@ -293,7 +293,11 @@ 'api/v1/repos/' + repository + '/issues/' + encodeURIComponent(payload.number) + '/attachments', { method: 'POST', - headers: { Accept: 'application/json', 'Content-Type': 'application/json' }, + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + 'Idempotency-Key': payload.operation_id, + }, body: JSON.stringify({ filename: payload.filename, content_type: payload.content_type, diff --git a/frontend/issue-attachment.js b/frontend/issue-attachment.js index 66d8657..2837edd 100644 --- a/frontend/issue-attachment.js +++ b/frontend/issue-attachment.js @@ -11,9 +11,16 @@ function create(options) { const readDataUrl = options.readDataUrl; const upload = options.upload; + const createOperationId = options.createOperationId || (() => { + if (typeof globalThis !== 'undefined' && globalThis.crypto?.randomUUID) { + return globalThis.crypto.randomUUID(); + } + return Date.now().toString(36) + '-' + Math.random().toString(36).slice(2); + }); let selected = null; let confirmed = null; let serialized = null; + let operationId = null; function select(file) { if (!file || !IMAGE_TYPES.has(file.type)) { @@ -25,6 +32,7 @@ selected = file; confirmed = null; serialized = null; + operationId = createOperationId(); return state(); } @@ -32,6 +40,7 @@ selected = null; confirmed = null; serialized = null; + operationId = null; } function restore(value) { @@ -80,6 +89,7 @@ filename: attachment.filename, content_type: attachment.contentType, data: attachment.data, + operation_id: operationId, }); if (!confirmed || typeof confirmed.markdown !== 'string' || !confirmed.markdown) { confirmed = null; diff --git a/src/main.py b/src/main.py index a5991fa..d86bccd 100644 --- a/src/main.py +++ b/src/main.py @@ -1,6 +1,7 @@ import asyncio import base64 import binascii +import hashlib import hmac import math import os @@ -2776,20 +2777,40 @@ async def attach_to_assigned_issue( owner: str, repo: str, number: int = PathParam(gt=0), + idempotency_key: str | None = Header(default=None, max_length=128), ): repository = f"{owner}/{repo}" try: content = attachment.content() except ValueError as exc: raise HTTPException(status_code=422, detail=str(exc)) from exc + async def upload_attachment(): + result = await gitea_proxy.upload_assigned_issue_attachment( + repository, + number, + attachment.filename, + attachment.content_type, + content, + ) + safe_name = ( + result["name"].replace("\\", "\\\\").replace("[", "\\[").replace("]", "\\]") + .replace("\r", " ").replace("\n", " ") + ) + safe_url = result["url"].replace("<", "%3C").replace(">", "%3E") + result["markdown"] = f"![{safe_name}](<{safe_url}>)" + return result + try: - result = await asyncio.wait_for( - gitea_proxy.upload_assigned_issue_attachment( + result = await _run_idempotent_authored_action( + upload_attachment(), + idempotency_key=idempotency_key, + fingerprint=( + "issue-attachment", repository, number, attachment.filename, attachment.content_type, - content, + hashlib.sha256(content).hexdigest(), ), timeout=ISSUE_ACTION_TIMEOUT_SECONDS, ) @@ -2803,12 +2824,6 @@ async def attach_to_assigned_issue( status_code=503, headers={"Retry-After": "1"}, ) - safe_name = ( - result["name"].replace("\\", "\\\\").replace("[", "\\[").replace("]", "\\]") - .replace("\r", " ").replace("\n", " ") - ) - safe_url = result["url"].replace("<", "%3C").replace(">", "%3E") - result["markdown"] = f"![{safe_name}](<{safe_url}>)" return JSONResponse(result, status_code=201) diff --git a/tests/test_issue_attachment_ui.py b/tests/test_issue_attachment_ui.py index e2c51e6..b28bbc4 100644 --- a/tests/test_issue_attachment_ui.py +++ b/tests/test_issue_attachment_ui.py @@ -24,6 +24,7 @@ const attachment = require({json.dumps(str(ATTACHMENT))}); const calls = []; const file = {{name:'checkout.png', type:'image/png', size:8}}; const controller = attachment.create({{ + createOperationId: () => 'attachment-comment-471', readDataUrl: async selected => {{ calls.push('read:' + selected.name); return 'data:image/png;base64,iVBORw0KGgo='; }}, upload: async payload => {{ calls.push(payload); return {{markdown:'![checkout.png](https://forge.example/a.png)'}}; }}, }}); @@ -47,6 +48,7 @@ controller.select(file); "filename": "checkout.png", "content_type": "image/png", "data": "iVBORw0KGgo=", + "operation_id": "attachment-comment-471", }, ] assert output["state"] == { @@ -54,6 +56,32 @@ controller.select(file); } +def test_mobile_attachment_retry_reuses_operation_key_until_file_changes(): + script = f""" +const attachment = require({json.dumps(str(ATTACHMENT))}); +const keys=[]; +let sequence=0; +let fail=true; +const controller=attachment.create({{ + createOperationId:()=> 'attachment-' + (++sequence), + readDataUrl:async file=>'data:'+file.type+';base64,iVBORw0KGgo=', + upload:async payload=>{{keys.push(payload.operation_id);if(fail){{fail=false;throw new Error('offline');}}return {{markdown:'![ok](https://forge.example/a)'}};}}, +}}); +controller.select({{name:'first.png',type:'image/png',size:8}}); +(async()=>{{ + try{{await controller.prepareComment({{repository:'stackchain/api',number:17}},'Evidence');}}catch(_error){{}} + await controller.prepareComment({{repository:'stackchain/api',number:17}},'Evidence'); + controller.select({{name:'replacement.png',type:'image/png',size:8}}); + await controller.prepareComment({{repository:'stackchain/api',number:17}},'Replacement'); + process.stdout.write(JSON.stringify(keys)); +}})().catch(error=>{{console.error(error);process.exit(1);}}); +""" + + assert json.loads(run_node(script)) == [ + "attachment-1", "attachment-1", "attachment-2" + ] + + def test_selected_screenshot_serializes_for_durable_issue_capture_without_uploading(): script = f""" const attachment = require({json.dumps(str(ATTACHMENT))}); @@ -165,6 +193,7 @@ def test_issue_comment_actions_upload_before_posting_and_clear_after_acceptance( assert "issueAttachmentController.prepareComment(item, body)" in source assert "issueAttachmentController.prepareComment(selectedIssue, body)" in source assert source.count("issueAttachmentController.clear();") >= 2 + assert "'Idempotency-Key': payload.operation_id" in source def test_closing_issue_sheet_cannot_carry_a_screenshot_to_another_issue(): diff --git a/tests/test_issue_attachments.py b/tests/test_issue_attachments.py index b661e2c..10ff6f4 100644 --- a/tests/test_issue_attachments.py +++ b/tests/test_issue_attachments.py @@ -1,3 +1,4 @@ +import asyncio import base64 import httpx @@ -9,6 +10,15 @@ from src import gitea_proxy, main PNG_BYTES = b"\x89PNG\r\n\x1a\n" + b"mobile screenshot" +@pytest.fixture(autouse=True) +def clear_attachment_idempotency(): + main._authored_action_operations.clear() + main._idempotency_ledger.clear() + yield + main._authored_action_operations.clear() + main._idempotency_ledger.clear() + + @pytest.mark.anyio async def test_attachment_endpoint_uploads_valid_screenshot_to_assigned_issue(monkeypatch): calls = [] @@ -48,6 +58,118 @@ async def test_attachment_endpoint_uploads_valid_screenshot_to_assigned_issue(mo ] +@pytest.mark.anyio +async def test_attachment_endpoint_replays_confirmed_upload_for_same_key(monkeypatch): + calls = [] + + async def upload(repository, number, filename, content_type, content): + calls.append((repository, number, filename, content_type, content)) + return { + "name": filename, + "url": "https://forge.example/attachments/checkout.png", + "size": len(content), + } + + monkeypatch.setattr(main.gitea_proxy, "upload_assigned_issue_attachment", upload) + payload = { + "filename": "checkout.png", + "content_type": "image/png", + "data": base64.b64encode(PNG_BYTES).decode("ascii"), + } + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + first = await client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + json=payload, + headers={"Idempotency-Key": "attachment-retry-471"}, + ) + replay = await client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + json=payload, + headers={"Idempotency-Key": "attachment-retry-471"}, + ) + + assert first.status_code == replay.status_code == 201 + assert replay.json() == first.json() + assert len(calls) == 1 + + +@pytest.mark.anyio +async def test_attachment_endpoint_coalesces_concurrent_upload_retries(monkeypatch): + calls = 0 + + async def upload(_repository, _number, filename, _content_type, content): + nonlocal calls + calls += 1 + await asyncio.sleep(0.05) + return { + "name": filename, + "url": "https://forge.example/attachments/checkout.png", + "size": len(content), + } + + monkeypatch.setattr(main.gitea_proxy, "upload_assigned_issue_attachment", upload) + payload = { + "filename": "checkout.png", + "content_type": "image/png", + "data": base64.b64encode(PNG_BYTES).decode("ascii"), + } + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + first, retry = await asyncio.gather(*[ + client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + json=payload, + headers={"Idempotency-Key": "attachment-concurrent-471"}, + ) + for _ in range(2) + ]) + + assert first.status_code == retry.status_code == 201 + assert first.json() == retry.json() + assert calls == 1 + + +@pytest.mark.anyio +async def test_attachment_endpoint_rejects_changed_upload_for_used_key(monkeypatch): + calls = 0 + + async def upload(_repository, _number, filename, _content_type, content): + nonlocal calls + calls += 1 + return { + "name": filename, + "url": "https://forge.example/attachments/checkout.png", + "size": len(content), + } + + monkeypatch.setattr(main.gitea_proxy, "upload_assigned_issue_attachment", upload) + transport = httpx.ASGITransport(app=main.app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + accepted = await client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + json={ + "filename": "checkout.png", + "content_type": "image/png", + "data": base64.b64encode(PNG_BYTES).decode("ascii"), + }, + headers={"Idempotency-Key": "attachment-conflict-471"}, + ) + conflict = await client.post( + "/api/v1/repos/stackchain/api/issues/17/attachments", + json={ + "filename": "checkout.png", + "content_type": "image/png", + "data": base64.b64encode(PNG_BYTES + b" changed").decode("ascii"), + }, + headers={"Idempotency-Key": "attachment-conflict-471"}, + ) + + assert accepted.status_code == 201 + assert conflict.status_code == 409 + assert calls == 1 + + @pytest.mark.anyio async def test_attachment_endpoint_admits_a_normal_phone_screenshot(monkeypatch): screenshot = b"\x89PNG\r\n\x1a\n" + (b"x" * (100 * 1024)) -- 2.43.0