feat: make mobile issue capture idempotent (#177)
This commit is contained in:
parent
9293dde1af
commit
6f54d5764b
|
|
@ -20,7 +20,10 @@ create issue comments, close assigned issues, inspect/comment on assigned pull
|
|||
requests, merge assigned pull requests, and submit pull-request reviews.
|
||||
Pull-request replies and mobile My Work issue and PR comments use Gitea's
|
||||
issue-comment API; mobile issue capture requires issue
|
||||
creation and assignment permission. Closing an assigned issue, native Comment,
|
||||
creation and assignment permission. Issue capture persists a per-draft idempotency key,
|
||||
so retrying after a timeout or reload replays a confirmed creation instead of posting a
|
||||
duplicate; callers integrating directly should preserve the `Idempotency-Key` header
|
||||
with the unchanged payload until a `201` response is confirmed. Closing an assigned issue, native Comment,
|
||||
Approve, and Request changes reviews, and assigned-PR merge require repository
|
||||
write permission. Native Comment, Approve, and Request changes reviews support
|
||||
head-scoped draft comments anchored to changed lines; the dashboard validates each
|
||||
|
|
|
|||
|
|
@ -1,4 +1,14 @@
|
|||
function createIssueCapture({ fetchJson, storage }) {
|
||||
function newIssueOperationId() {
|
||||
if (globalThis.crypto?.randomUUID) return globalThis.crypto.randomUUID();
|
||||
if (globalThis.crypto?.getRandomValues) {
|
||||
const bytes = new Uint8Array(16);
|
||||
globalThis.crypto.getRandomValues(bytes);
|
||||
return Array.from(bytes, byte => byte.toString(16).padStart(2, '0')).join('');
|
||||
}
|
||||
return String(Date.now()) + '-' + Math.random().toString(16).slice(2);
|
||||
}
|
||||
|
||||
function createIssueCapture({ fetchJson, storage, createOperationId = newIssueOperationId }) {
|
||||
const storageKey = 'stackchain.issue-capture.v1';
|
||||
let pending = null;
|
||||
const safeLabelIds = value => Array.from(new Set(
|
||||
|
|
@ -6,30 +16,44 @@ function createIssueCapture({ fetchJson, storage }) {
|
|||
)).slice(0, 20);
|
||||
const emptyDraft = () => ({ repository: '', title: '', body: '', labelIds: [] });
|
||||
|
||||
function loadStored() {
|
||||
try {
|
||||
const parsed = JSON.parse(storage.getItem(storageKey) || 'null');
|
||||
if (!parsed || typeof parsed !== 'object') return {...emptyDraft(), operationId: ''};
|
||||
return {
|
||||
repository: String(parsed.repository || ''),
|
||||
title: String(parsed.title || ''),
|
||||
body: String(parsed.body || ''),
|
||||
labelIds: safeLabelIds(parsed.labelIds),
|
||||
operationId: String(parsed.operationId || '').slice(0, 128),
|
||||
};
|
||||
} catch (_error) {
|
||||
return {...emptyDraft(), operationId: ''};
|
||||
}
|
||||
}
|
||||
|
||||
function writeStored(record) {
|
||||
try { storage.setItem(storageKey, JSON.stringify(record)); }
|
||||
catch (_error) { /* Keep the form as the in-memory fallback. */ }
|
||||
}
|
||||
|
||||
function saveDraft(draft) {
|
||||
const previous = loadStored();
|
||||
const safe = {
|
||||
repository: String(draft?.repository || ''),
|
||||
title: String(draft?.title || ''),
|
||||
body: String(draft?.body || ''),
|
||||
labelIds: safeLabelIds(draft?.labelIds),
|
||||
};
|
||||
try { storage.setItem(storageKey, JSON.stringify(safe)); }
|
||||
catch (_error) { /* Keep the form as the in-memory fallback. */ }
|
||||
const unchanged = ['repository', 'title', 'body'].every(key => previous[key] === safe[key]) &&
|
||||
JSON.stringify(previous.labelIds) === JSON.stringify(safe.labelIds);
|
||||
writeStored({...safe, operationId: unchanged ? previous.operationId : ''});
|
||||
return safe;
|
||||
}
|
||||
|
||||
function loadDraft() {
|
||||
try {
|
||||
const parsed = JSON.parse(storage.getItem(storageKey) || 'null');
|
||||
return parsed && typeof parsed === 'object' ? {
|
||||
repository: String(parsed.repository || ''),
|
||||
title: String(parsed.title || ''),
|
||||
body: String(parsed.body || ''),
|
||||
labelIds: safeLabelIds(parsed.labelIds),
|
||||
} : emptyDraft();
|
||||
} catch (_error) {
|
||||
return emptyDraft();
|
||||
}
|
||||
const {operationId: _operationId, ...draft} = loadStored();
|
||||
return draft;
|
||||
}
|
||||
|
||||
function clearDraft() {
|
||||
|
|
@ -52,10 +76,16 @@ function createIssueCapture({ fetchJson, storage }) {
|
|||
function submit(draft) {
|
||||
if (pending) return pending;
|
||||
const saved = saveDraft(draft);
|
||||
const stored = loadStored();
|
||||
const operationId = stored.operationId || String(createOperationId()).slice(0, 128);
|
||||
writeStored({...saved, operationId});
|
||||
const repository = saved.repository.split('/').map(encodeURIComponent).join('/');
|
||||
pending = fetchJson('api/v1/repos/' + repository + '/issues', {
|
||||
method: 'POST',
|
||||
headers: { Accept: 'application/json', 'Content-Type': 'application/json' },
|
||||
headers: {
|
||||
Accept: 'application/json', 'Content-Type': 'application/json',
|
||||
'Idempotency-Key': operationId,
|
||||
},
|
||||
body: JSON.stringify({
|
||||
title: saved.title, body: saved.body, label_ids: saved.labelIds,
|
||||
}),
|
||||
|
|
|
|||
56
src/main.py
56
src/main.py
|
|
@ -6,7 +6,7 @@ from contextlib import asynccontextmanager
|
|||
from pathlib import Path
|
||||
from typing import Any, Literal
|
||||
|
||||
from fastapi import FastAPI, HTTPException, Path as PathParam, Query
|
||||
from fastapi import FastAPI, Header, HTTPException, Path as PathParam, Query
|
||||
from fastapi.responses import JSONResponse
|
||||
from fastapi.staticfiles import StaticFiles
|
||||
from pydantic import BaseModel, Field, PositiveInt, field_validator, model_validator
|
||||
|
|
@ -55,6 +55,8 @@ EVENT_STREAM_TIMEOUT_SECONDS = 5.0
|
|||
READINESS_TIMEOUT_SECONDS = 5.0
|
||||
REVIEW_DETAIL_TIMEOUT_SECONDS = 5.0
|
||||
ISSUE_ACTION_TIMEOUT_SECONDS = 5.0
|
||||
ISSUE_CREATION_IDEMPOTENCY_TTL_SECONDS = 600.0
|
||||
ISSUE_CREATION_IDEMPOTENCY_MAX_ENTRIES = 256
|
||||
NOTIFICATION_MUTATION_TIMEOUT_SECONDS = 5.0
|
||||
NOTIFICATION_PAGE_TIMEOUT_SECONDS = 5.0
|
||||
WORK_PAGE_TIMEOUT_SECONDS = 5.0
|
||||
|
|
@ -80,6 +82,9 @@ _live_section_retry_at: dict[str, float | None] = {
|
|||
}
|
||||
_live_snapshot_refreshing_sections: set[str] = set()
|
||||
_read_notification_ids: set[int] = set()
|
||||
_issue_creation_operations: dict[
|
||||
str, tuple[tuple[Any, ...], asyncio.Task, float]
|
||||
] = {}
|
||||
|
||||
|
||||
class ContextPayloadError(ValueError):
|
||||
|
|
@ -952,7 +957,12 @@ async def repository_labels(owner: str, repo: str):
|
|||
|
||||
|
||||
@app.post("/api/v1/repos/{owner}/{repo}/issues", status_code=201)
|
||||
async def create_assigned_issue(creation: IssueCreation, owner: str, repo: str):
|
||||
async def create_assigned_issue(
|
||||
creation: IssueCreation,
|
||||
owner: str,
|
||||
repo: str,
|
||||
idempotency_key: str | None = Header(default=None, max_length=128),
|
||||
):
|
||||
repository = f"{owner}/{repo}"
|
||||
|
||||
async def create_issue():
|
||||
|
|
@ -980,8 +990,48 @@ async def create_assigned_issue(creation: IssueCreation, owner: str, repo: str):
|
|||
repository, creation.title, creation.body, login, creation.label_ids
|
||||
)
|
||||
|
||||
operation = create_issue()
|
||||
if idempotency_key:
|
||||
now = time.monotonic()
|
||||
expired = [
|
||||
key for key, (_, task, created_at) in _issue_creation_operations.items()
|
||||
if task.done()
|
||||
and now - created_at >= ISSUE_CREATION_IDEMPOTENCY_TTL_SECONDS
|
||||
]
|
||||
for key in expired:
|
||||
_issue_creation_operations.pop(key, None)
|
||||
fingerprint = (
|
||||
repository, creation.title, creation.body, tuple(creation.label_ids)
|
||||
)
|
||||
existing = _issue_creation_operations.get(idempotency_key)
|
||||
if existing is not None:
|
||||
operation.close()
|
||||
if existing[0] != fingerprint:
|
||||
raise HTTPException(status_code=409, detail="Idempotency key already used")
|
||||
task = existing[1]
|
||||
else:
|
||||
while len(_issue_creation_operations) >= ISSUE_CREATION_IDEMPOTENCY_MAX_ENTRIES:
|
||||
completed = [
|
||||
key for key, (_, task, _) in _issue_creation_operations.items()
|
||||
if task.done()
|
||||
]
|
||||
if not completed:
|
||||
operation.close()
|
||||
raise HTTPException(
|
||||
status_code=503,
|
||||
detail="Issue creation is busy; please retry",
|
||||
headers={"Retry-After": "1"},
|
||||
)
|
||||
oldest = min(
|
||||
completed, key=lambda key: _issue_creation_operations[key][2]
|
||||
)
|
||||
_issue_creation_operations.pop(oldest)
|
||||
task = asyncio.create_task(operation)
|
||||
_issue_creation_operations[idempotency_key] = (fingerprint, task, now)
|
||||
operation = asyncio.shield(task)
|
||||
|
||||
try:
|
||||
result = await asyncio.wait_for(create_issue(), timeout=ISSUE_ACTION_TIMEOUT_SECONDS)
|
||||
result = await asyncio.wait_for(operation, timeout=ISSUE_ACTION_TIMEOUT_SECONDS)
|
||||
except HTTPException:
|
||||
raise
|
||||
except Exception:
|
||||
|
|
|
|||
|
|
@ -6,6 +6,13 @@ import pytest
|
|||
from src import gitea_proxy, main
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def clear_issue_creation_operations():
|
||||
main._issue_creation_operations.clear()
|
||||
yield
|
||||
main._issue_creation_operations.clear()
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_create_issue_endpoint_derives_self_assignment_and_returns_confirmed_issue(monkeypatch):
|
||||
calls = []
|
||||
|
|
@ -60,6 +67,166 @@ async def test_create_issue_endpoint_derives_self_assignment_and_returns_confirm
|
|||
assert calls == [("stackchain/api", "Capture mobile work", "Context", "timmy", [3])]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_create_issue_replays_one_upstream_result_for_concurrent_idempotent_requests(monkeypatch):
|
||||
calls = 0
|
||||
started = asyncio.Event()
|
||||
release = asyncio.Event()
|
||||
|
||||
async def user():
|
||||
return {"login": "timmy"}
|
||||
|
||||
async def available_repos():
|
||||
return [{"full_name": "stackchain/api"}]
|
||||
|
||||
async def create(repository, title, body, assignee, label_ids):
|
||||
nonlocal calls
|
||||
calls += 1
|
||||
started.set()
|
||||
await release.wait()
|
||||
return {
|
||||
"id": 81, "number": 17, "title": title, "state": "open",
|
||||
"repository": repository, "labels": [], "assignees": [assignee],
|
||||
"updated_at": "2026-08-07T03:00:00Z",
|
||||
"url": "https://forge.example/stackchain/api/issues/17",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(main.gitea_proxy, "current_user", user)
|
||||
monkeypatch.setattr(main.gitea_proxy, "repos", available_repos)
|
||||
monkeypatch.setattr(main.gitea_proxy, "create_issue", create)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
headers = {"Idempotency-Key": "capture-177-concurrent"}
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
first = asyncio.create_task(client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "Capture work"}, headers=headers
|
||||
))
|
||||
await started.wait()
|
||||
second = asyncio.create_task(client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "Capture work"}, 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 [response.json()["number"] for response in responses] == [17, 17]
|
||||
assert calls == 1
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_create_issue_rejects_changed_payload_for_an_existing_idempotency_key(monkeypatch):
|
||||
calls = []
|
||||
|
||||
async def user():
|
||||
return {"login": "timmy"}
|
||||
|
||||
async def available_repos():
|
||||
return [{"full_name": "stackchain/api"}]
|
||||
|
||||
async def create(repository, title, body, assignee, label_ids):
|
||||
calls.append(title)
|
||||
return {
|
||||
"id": 81, "number": 17, "title": title, "state": "open",
|
||||
"repository": repository, "labels": [], "assignees": [assignee],
|
||||
"updated_at": "2026-08-07T03:00:00Z",
|
||||
"url": "https://forge.example/stackchain/api/issues/17",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(main.gitea_proxy, "current_user", user)
|
||||
monkeypatch.setattr(main.gitea_proxy, "repos", available_repos)
|
||||
monkeypatch.setattr(main.gitea_proxy, "create_issue", create)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
headers = {"Idempotency-Key": "capture-177-payload-conflict"}
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
created = await client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "First title"}, headers=headers
|
||||
)
|
||||
conflict = await client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "Changed title"}, headers=headers
|
||||
)
|
||||
|
||||
assert created.status_code == 201
|
||||
assert conflict.status_code == 409
|
||||
assert conflict.json()["detail"] == "Idempotency key already used"
|
||||
assert calls == ["First title"]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_create_issue_retry_recovers_result_after_the_first_request_times_out(monkeypatch):
|
||||
calls = 0
|
||||
|
||||
async def user():
|
||||
return {"login": "timmy"}
|
||||
|
||||
async def available_repos():
|
||||
return [{"full_name": "stackchain/api"}]
|
||||
|
||||
async def create(repository, title, body, assignee, label_ids):
|
||||
nonlocal calls
|
||||
calls += 1
|
||||
await asyncio.sleep(0.03)
|
||||
return {
|
||||
"id": 81, "number": 17, "title": title, "state": "open",
|
||||
"repository": repository, "labels": [], "assignees": [assignee],
|
||||
"updated_at": "2026-08-07T03:00:00Z",
|
||||
"url": "https://forge.example/stackchain/api/issues/17",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(main, "ISSUE_ACTION_TIMEOUT_SECONDS", 0.01)
|
||||
monkeypatch.setattr(main.gitea_proxy, "current_user", user)
|
||||
monkeypatch.setattr(main.gitea_proxy, "repos", available_repos)
|
||||
monkeypatch.setattr(main.gitea_proxy, "create_issue", create)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
headers = {"Idempotency-Key": "capture-177-timeout-recovery"}
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
timed_out = await client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "Slow capture"}, headers=headers
|
||||
)
|
||||
await asyncio.sleep(0.03)
|
||||
recovered = await client.post(
|
||||
"/api/v1/repos/stackchain/api/issues", json={"title": "Slow capture"}, headers=headers
|
||||
)
|
||||
|
||||
assert timed_out.status_code == 503
|
||||
assert recovered.status_code == 201
|
||||
assert recovered.json()["number"] == 17
|
||||
assert calls == 1
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_create_issue_idempotency_registry_evicts_oldest_entry_at_size_limit(monkeypatch):
|
||||
async def user():
|
||||
return {"login": "timmy"}
|
||||
|
||||
async def available_repos():
|
||||
return [{"full_name": "stackchain/api"}]
|
||||
|
||||
async def create(repository, title, body, assignee, label_ids):
|
||||
return {
|
||||
"id": 81, "number": 17, "title": title, "state": "open",
|
||||
"repository": repository, "labels": [], "assignees": [assignee],
|
||||
"updated_at": "2026-08-07T03:00:00Z",
|
||||
"url": "https://forge.example/stackchain/api/issues/17",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(main, "ISSUE_CREATION_IDEMPOTENCY_MAX_ENTRIES", 2)
|
||||
monkeypatch.setattr(main.gitea_proxy, "current_user", user)
|
||||
monkeypatch.setattr(main.gitea_proxy, "repos", available_repos)
|
||||
monkeypatch.setattr(main.gitea_proxy, "create_issue", create)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
|
||||
for index in range(3):
|
||||
response = await client.post(
|
||||
"/api/v1/repos/stackchain/api/issues",
|
||||
json={"title": f"Capture {index}"},
|
||||
headers={"Idempotency-Key": f"capture-177-bounded-{index}"},
|
||||
)
|
||||
assert response.status_code == 201
|
||||
|
||||
assert len(main._issue_creation_operations) == 2
|
||||
assert "capture-177-bounded-0" not in main._issue_creation_operations
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_gitea_create_issue_posts_self_assignment_and_normalizes_confirmation():
|
||||
requests = []
|
||||
|
|
|
|||
|
|
@ -395,6 +395,53 @@ Promise.all([first, duplicate]).then(results => process.stdout.write(JSON.string
|
|||
assert output["results"][1]["number"] == 17
|
||||
|
||||
|
||||
def test_issue_capture_reuses_its_persisted_idempotency_key_after_reload():
|
||||
script = f"""
|
||||
const createIssueCapture = require({json.dumps(str(CREATE_ISSUE_SHEET))});
|
||||
const values = new Map();
|
||||
const storage = {{
|
||||
getItem:key => values.get(key) || null,
|
||||
setItem:(key,value) => values.set(key,value),
|
||||
removeItem:key => values.delete(key),
|
||||
}};
|
||||
const calls = [];
|
||||
const draft = {{repository:'stackchain/api', title:'Capture work', body:'Context', labelIds:[3]}};
|
||||
const first = createIssueCapture({{
|
||||
storage,
|
||||
createOperationId: () => 'operation-177',
|
||||
fetchJson: (_url, options) => {{ calls.push(options.headers['Idempotency-Key']); return Promise.reject(new Error('timeout')); }},
|
||||
}});
|
||||
first.submit(draft).catch(() => {{
|
||||
const restored = createIssueCapture({{
|
||||
storage,
|
||||
createOperationId: () => 'must-not-replace-operation-177',
|
||||
fetchJson: (_url, options) => {{
|
||||
calls.push(options.headers['Idempotency-Key']);
|
||||
return Promise.resolve({{number:17, title:'Capture work'}});
|
||||
}},
|
||||
}});
|
||||
const before = restored.loadDraft();
|
||||
restored.submit(before).then(issue => process.stdout.write(JSON.stringify({{
|
||||
calls, before, after:restored.loadDraft(), number:issue.number
|
||||
}})));
|
||||
}});
|
||||
"""
|
||||
result = subprocess.run(
|
||||
["node", "-e", script], check=True, capture_output=True, text=True
|
||||
)
|
||||
output = json.loads(result.stdout)
|
||||
|
||||
assert output["calls"] == ["operation-177", "operation-177"]
|
||||
assert output["before"] == {
|
||||
"repository": "stackchain/api", "title": "Capture work", "body": "Context",
|
||||
"labelIds": [3],
|
||||
}
|
||||
assert output["after"] == {
|
||||
"repository": "", "title": "", "body": "", "labelIds": []
|
||||
}
|
||||
assert output["number"] == 17
|
||||
|
||||
|
||||
def test_issue_capture_loads_repository_labels_with_priorities_first():
|
||||
script = f"""
|
||||
const createIssueCapture = require({json.dumps(str(CREATE_ISSUE_SHEET))});
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user