feat: make authored actions idempotent (#185)

tests/test_authored_idempotency.py

@@ -0,0 +1,142 @@
+import asyncio
+
+import httpx
+import pytest
+
+from src import main
+
+
+@pytest.fixture(autouse=True)
+def clear_authored_operations():
+    main._authored_action_operations.clear()
+    yield
+    main._authored_action_operations.clear()
+
+
+@pytest.mark.anyio
+async def test_issue_comment_replays_one_upstream_result_for_concurrent_requests(monkeypatch):
+    calls = 0
+    started = asyncio.Event()
+    release = asyncio.Event()
+
+    async def assigned(_repository, _number):
+        return True
+
+    async def comment(_repository, _number, body):
+        nonlocal calls
+        calls += 1
+        started.set()
+        await release.wait()
+        return {"id": 82, "body": body}
+
+    monkeypatch.setattr(main.gitea_proxy, "is_assigned_issue", assigned)
+    monkeypatch.setattr(main.gitea_proxy, "comment_on_issue", comment)
+    transport = httpx.ASGITransport(app=main.app)
+    headers = {"Idempotency-Key": "issue-comment-185"}
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        first = asyncio.create_task(client.post(
+            "/api/v1/repos/stackchain/api/issues/7/comments",
+            json={"body": "Ship it"}, headers=headers,
+        ))
+        await started.wait()
+        second = asyncio.create_task(client.post(
+            "/api/v1/repos/stackchain/api/issues/7/comments",
+            json={"body": "Ship it"}, headers=headers,
+        ))
+        await asyncio.sleep(0)
+        release.set()
+        responses = await asyncio.gather(first, second)
+
+    assert [response.status_code for response in responses] == [201, 201]
+    assert calls == 1
+
+
+@pytest.mark.anyio
+async def test_pull_comment_rejects_changed_payload_for_same_key(monkeypatch):
+    calls = []
+
+    async def assigned(_repository, _number):
+        return True
+
+    async def comment(_repository, _number, body):
+        calls.append(body)
+        return {"id": len(calls), "body": body}
+
+    monkeypatch.setattr(main.gitea_proxy, "is_assigned_pull", assigned)
+    monkeypatch.setattr(main.gitea_proxy, "comment_on_issue", comment)
+    transport = httpx.ASGITransport(app=main.app)
+    headers = {"Idempotency-Key": "pull-comment-185"}
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        created = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/comments",
+            json={"body": "Ship it"}, headers=headers,
+        )
+        conflict = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/comments",
+            json={"body": "Changed"}, headers=headers,
+        )
+
+    assert created.status_code == 201
+    assert conflict.status_code == 409
+    assert calls == ["Ship it"]
+
+
+@pytest.mark.anyio
+async def test_notification_reply_retry_recovers_after_caller_timeout(monkeypatch):
+    calls = 0
+
+    async def reply(_thread_id, body):
+        nonlocal calls
+        calls += 1
+        await asyncio.sleep(0.03)
+        return {"id": 91, "body": body}
+
+    monkeypatch.setattr(main, "NOTIFICATION_MUTATION_TIMEOUT_SECONDS", 0.01)
+    monkeypatch.setattr(main.gitea_proxy, "reply_to_notification", reply)
+    transport = httpx.ASGITransport(app=main.app)
+    headers = {"Idempotency-Key": "notification-reply-185"}
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        timed_out = await client.post(
+            "/api/v1/notifications/42/reply", json={"body": "Retry"}, headers=headers,
+        )
+        await asyncio.sleep(0.03)
+        recovered = await client.post(
+            "/api/v1/notifications/42/reply", json={"body": "Retry"}, headers=headers,
+        )
+
+    assert timed_out.status_code == 503
+    assert recovered.status_code == 201
+    assert recovered.json()["id"] == 91
+    assert calls == 1
+
+
+@pytest.mark.anyio
+async def test_review_replays_same_key_and_rejects_changed_head(monkeypatch):
+    calls = []
+
+    async def requested(_repository, _number):
+        return True
+
+    async def submit(_repository, _number, head, decision, body):
+        calls.append((head, decision, body))
+        return {"id": 93, "state": "APPROVED"}
+
+    monkeypatch.setattr(main, "is_requested_review", requested)
+    monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
+    transport = httpx.ASGITransport(app=main.app)
+    headers = {"Idempotency-Key": "review-185"}
+    payload = {"expected_head_sha": "abc", "decision": "approve", "body": "Good"}
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        created = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/review", json=payload, headers=headers,
+        )
+        replayed = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/review", json=payload, headers=headers,
+        )
+        conflict = await client.post(
+            "/api/v1/repos/stackchain/api/pulls/7/review",
+            json={**payload, "expected_head_sha": "def"}, headers=headers,
+        )
+
+    assert [created.status_code, replayed.status_code, conflict.status_code] == [201, 201, 409]
+    assert calls == [("abc", "approve", "Good")]

tests/test_my_work.py

@@ -1888,7 +1888,7 @@ def test_review_controller_submits_once_while_request_is_in_flight():
 const createReviewController = require({json.dumps(str(REVIEW_SHEET))});
 let resolveRequest;
 const calls = [];
-const controller = createReviewController({{ fetchJson: (url, options) => {{
+const controller = createReviewController({{ createOperationId: () => 'review-op-test', fetchJson: (url, options) => {{
   calls.push({{url, options}});
   return new Promise(resolve => {{ resolveRequest = resolve; }});
 }} }});
@@ -1914,6 +1914,7 @@ Promise.all([first, second]).then(results => process.stdout.write(JSON.stringify
             "headers": {
                 "Accept": "application/json",
                 "Content-Type": "application/json",
+                "Idempotency-Key": "review-op-test",
             },
             "body": json.dumps(
                 {
@@ -1975,3 +1976,72 @@ async def test_mobile_review_failure_offers_an_in_place_retry_for_the_same_item(
     assert "qs('#retry-review-load').hidden = true" in html
     assert "openReviewSheet(selectedReview, reviewTrigger)" in html
     assert "qs('#retry-review-load').focus()" in html
+
+
+def test_issue_comment_reuses_operation_key_after_reload_until_success():
+    script = f"""
+const createIssueSheet = require({json.dumps(str(ISSUE_SHEET))});
+const values = new Map();
+const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
+const calls = [];
+const item = {{repository:'stackchain/api', number:7}};
+const first = createIssueSheet({{storage, createOperationId:() => 'issue-op-185', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.reject(new Error('timeout'));}}}});
+first.comment(item, 'Ship it').catch(() => {{
+  const second = createIssueSheet({{storage, createOperationId:() => 'wrong-key', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.resolve({{id:82}});}}}});
+  second.comment(item, 'Ship it').then(() => process.stdout.write(JSON.stringify({{calls, keys:[...values.keys()]}})));
+}});
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    assert json.loads(result.stdout) == {"calls": ["issue-op-185", "issue-op-185"], "keys": []}
+
+
+def test_pull_comment_reuses_operation_key_after_reload_until_success():
+    script = f"""
+const createPullSheet = require({json.dumps(str(PULL_SHEET))});
+const values = new Map();
+const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
+const calls = [];
+const item = {{repository:'stackchain/api', number:7}};
+const first = createPullSheet({{storage, createOperationId:() => 'pull-op-185', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.reject(new Error('timeout'));}}}});
+first.comment(item, 'Ship it').catch(() => {{
+  createPullSheet({{storage, createOperationId:() => 'wrong-key', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.resolve({{id:91}});}}}}).comment(item, 'Ship it')
+    .then(() => process.stdout.write(JSON.stringify({{calls, keys:[...values.keys()]}})));
+}});
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    assert json.loads(result.stdout) == {"calls": ["pull-op-185", "pull-op-185"], "keys": []}
+
+
+def test_notification_reply_reuses_persisted_operation_key():
+    script = f"""
+const build = require({json.dumps(str(MY_WORK))});
+const values = new Map();
+const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
+const calls = [];
+const item = {{notification_id:42}};
+const first = build.createNotificationReplier({{storage, onStatus:()=>{{}}, createOperationId:() => 'reply-op-185', post:(_id,_body,key) => {{calls.push(key); return Promise.reject(new Error('timeout'));}}}});
+first.submit(item, 'Retry').then(() => {{
+  const second = build.createNotificationReplier({{storage, onStatus:()=>{{}}, createOperationId:() => 'wrong-key', post:(_id,_body,key) => {{calls.push(key); return Promise.resolve({{id:91}});}}}});
+  second.submit(item, 'Retry').then(() => process.stdout.write(JSON.stringify({{calls, keys:[...values.keys()]}})));
+}});
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    assert json.loads(result.stdout) == {"calls": ["reply-op-185", "reply-op-185"], "keys": []}
+
+
+def test_review_submission_reuses_persisted_operation_key():
+    script = f"""
+const createReviewController = require({json.dumps(str(REVIEW_SHEET))});
+const values = new Map();
+const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
+const calls = [];
+const item = {{repository:'stackchain/api', number:7}};
+const payload = {{decision:'approve', body:'Good', expected_head_sha:'abc', comments:[]}};
+const first = createReviewController({{storage, createOperationId:() => 'review-op-185', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.reject(new Error('timeout'));}}}});
+first.submit(item, payload).catch(() => {{
+  createReviewController({{storage, createOperationId:() => 'wrong-key', fetchJson:(_u,o) => {{calls.push(o.headers['Idempotency-Key']); return Promise.resolve({{id:93}});}}}}).submit(item, payload)
+    .then(() => process.stdout.write(JSON.stringify({{calls, keys:[...values.keys()]}})));
+}});
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    assert json.loads(result.stdout) == {"calls": ["review-op-185", "review-op-185"], "keys": []}