Server idempotency middleware for console writes

Task 12465 · Done · Coder · created · updated

## Context (BA Task 12447 → Architecture Task 12448 → Plan Task 12449) Original request (verbatim): "Regarding task <https://app.jaah.ai/tasks/12393> This is a repeat situation with the same user apparently behind a broken corporate firewall. He says that previous fix helped and his task list is no longer causing trouble, but he was now struggling with Projects edit. Go over all UI surfaces and ensure that the same code is reused everywhere - one choke point - that works identically and uses the approach and protocols that are now proven working. If there will be other similar issues we would only have to fix it in one place" Root cause (Architecture §0, AC-0): the hang was a WRITE — `PUT /api/projects/54` sat on a dead direct HTTP/2 connection (prod `activity_events` 1661992/3; every page read was relay-rescued, `client_events` 1335-1345). Writes are bare `fetch` with no relay/timeout (`apps/web/src/api.ts:68`); the relay is GET-only (`apps/api/app/routes/read_relay.py:13`). Operator decisions (verbatim, PRD §3 + Architecture §1): D2 "**Relay once + idempotency key.** Every write carries a unique key. When the direct connection looks dead, the write also goes over the WebSocket relay, and the server applies a given key at most once." D3 "**All four:** all `api()` reads + writes; the raw-fetch stragglers (`useVersionCheck`, live `config.ts`, live rewriter, S3 upload PUT); streams + terminal (EventSource `/api/stream`, terminal WebSocket); browser-fetched URLs (`apiHref`: img, iframe, downloads)." D4 "**Yes, also unify edit pages:** load via `useEditRecord`, save via `useSubmit`". A1 one epic, parallel where no dependency. A2 **At most once** (claim before run). A3 **All console writes, capped** (body ≤ 1 MiB, per-session write budget, key required). A4 **Public relay, live paths only** (cookieless socket, `GET /api/public/live/*`, per-IP budget). Full documents (read your section in full before building): the PRD and Architecture are the settled `prd` / `architecture` artifacts of Tasks 12447 / 12448 (read with `task_read` / `artifact_read`). Your section below is copied from them; the pinned seams are binding contracts other children build against — do not change them without updating the sibling that consumes them. ## Your scope: C1 — Server idempotency Build `apps/api/app/idempotency.py` (pure-ASGI `IdempotencyMiddleware`), the additive `request_idempotency` migration (+ `slow_write` client-event kind in the same migration), tenancy-plane registration (table + middleware, `test_tenancy_plane_sites.py`), and the 24 h prune cron. Exactly per §3.1 below. Note: `Idempotency-Key` already appears in `apps/api/app/trigger_intake.py` for inbound GitLab webhooks — unrelated machine traffic, leave it alone (the middleware applies only to console-session writes). ### Architecture §3.1 (verbatim) ### 3.1 Idempotency (server) — `apps/api/app/idempotency.py` [child C1] **Wire contract (pinned; C3 builds against it):** - Request header `Idempotency-Key: <22–64 chars [A-Za-z0-9_-]>`. The console sends `crypto.randomUUID()` without dashes, or a base64url value. Any other shape → `400 {"detail":"idempotency_key_invalid"}`. - It applies only to `POST|PUT|PATCH|DELETE` on `/api/*` where the request carries a console session cookie. Machine callers (node, agent, CLI, `/api/agent/*`, `/api/node-bus*`) are untouched. With no header there is no behaviour change, so old tabs and API clients keep working. - **Scope** = `(admin_id from session, key)`. The stored fingerprint is `method`, `path` with query, and `sha256(body)`. - Answers: - First sight → a claim row (`state='running'`) is inserted and **committed in its own transaction before the route runs**. The route runs. Then status, the headers in `{content-type, location, content-disposition}` and the body (≤ 1 MiB) are stored, `state='done'`. - Same key + same fingerprint + `done` → the stored answer is replayed, with response header `Idempotent-Replayed: true`. The route does not run. - Same key + same fingerprint + `running` → the request waits, polling the row every 250 ms for up to 25 s. On `done` it replays. Past 25 s → `409 {"detail":"idempotency_in_progress"}`. - Same key + `running` with a stale heartbeat (its process died; see "Stale threshold" below) → `409 {"detail":"idempotency_outcome_unknown"}` (A2). Never re-run. - Same key + **different** fingerprint → `422 {"detail":"idempotency_key_reused"}`. Nothing applied. - A stored answer over 1 MiB → stored as status only. A replay then answers `409 {"detail":"idempotency_replay_unavailable"}`. - Every completed status is stored, including 4xx and 5xx. A retry with the same key never re-runs a failed write into a duplicate (AC-3 5xx row). The client mints a NEW key per user action, so a user's explicit retry is a new write. - **Table** `request_idempotency`: `admin_id uuid`, `key text`, `method`, `path`, `body_sha256`, `state`, `status smallint null`, `headers jsonb`, `body bytea null`, `created_at`, `heartbeat_at`, `completed_at`. PK `(admin_id, key)`, index `created_at`. A new migration, additive. It is registered in the tenancy plane as a control-plane table keyed by principal, the same treatment as `rate_limit_hits` (`models/control_plane.py:120`). - **Retention** 24 h. Pruned by a registered cron activity (`cron_activities.py` register, constant beside its peers in `cron_system_activities.py`). - **Unhandled route exception**: the middleware catches it around `await app(...)`. It stores `status=500` (body `{"detail":"Internal Server Error"}`), sets `state='done'` and re-raises. A copy arriving later replays that 500 and never re-runs the route. - **Stale threshold (one rule)**: the claim sets `heartbeat_at = created_at`. While the route runs, the claiming request refreshes `heartbeat_at` every 10 s. A `running` claim is stale when `heartbeat_at < now() − 30 s`. So a legitimately long route is never declared "unknown", and only a dead process is. - **Expiry (AC-2 "key expired" state)**: a row older than 24 h is ignored as if absent, even before the prune cron removes it. That key is then a first sighting and applies again. The 24 h window is far beyond any relay or retry the console makes (30 s bound). - **Body cap**: request bodies over 1 MiB (only direct multipart uploads reach that size) are **not** given idempotency. With a `content-length` above 1 MiB, the middleware passes the request through untouched without buffering. Without a `content-length` (chunked), it buffers up to 1 MiB. Past that, it makes no claim, replays the buffered first 1 MiB to the app, then streams the rest from the original `receive`. Those requests are never relayed (§3.3 rule 6), so only one copy can exist. - **Placement (exact)**: `app.add_middleware(IdempotencyMiddleware)` goes on the line **directly before** `app.add_middleware(StaleTabGuardMiddleware)` (`main.py:424`). Starlette's last-added-is-outermost order (`user_activity.py:132`) makes the stack: SessionAuth → StaleTabGuard → **Idempotency** → SessionRefresh → UserActivity → routes. A replay therefore re-mints no session cookie (SessionRefresh is not reached) and records no second user-activity row, so AC-2 gets one audit entry. - **Placement**: pure ASGI, not `BaseHTTPMiddleware`, which breaks body and context binding (`user_activity.py:923`). It buffers the request body once and replays `receive`. It sits **inside** `StaleTabGuardMiddleware` and `SessionAuthMiddleware` (`main.py:424-425`), so an org-switched or unauthenticated request is refused before a key is claimed and never consumes one. It sits **outside** the route's authz dependency, so a replay serves exactly the answer the authorized first copy got. Scope by `admin_id` keeps one user from replaying another's answer. - Also in C1: add `slow_write` to `CLIENT_EVENT_KINDS` (`models/audit.py:165`) and to the `ck_client_events_kind` check, in the same migration. C3 posts this kind. ### PRD assumptions that bind this child (verbatim) - AS2. **Idempotency covers every console write.** Assumed every `POST/PUT/PATCH/DELETE` the console sends to `/api` carries the key and is deduplicated by the server, not only Projects. Why: D3 "all api() reads+writes" and A5 "fix it in one place". ### Requirement constraints (PRD §7, verbatim) - Writes are never applied twice (D2, AC-2). Idempotency touches every mutating route, so it is a security-surface change (data integrity). Project instructions grant merge permission. The review lane still applies. - Act-as headers (`lib/actAs.ts`) and session headers (`lib/activeOrg.ts`) must ride relayed writes exactly as direct ones. A relayed write must be authorized as the same session. A relayed 401/403 never wins over the direct answer, the same rule as reads. - Single-home: the relay rules live in one module. `docs/standards/coding-standards.md` and the context policy apply. ### Risks that bind this child (Architecture §5, verbatim) 1. **Double-apply through a crash window.** This is closed by claim-before-run (A2). The trap: storing the key only after the route runs would pass AC-2's happy path and still double-apply on a slow route, because the second copy arrives while the first is running. The `running` state and its wait are mandatory. 5. **Key reuse across user retries.** Re-using a key on "retry" after a 5xx would replay the 5xx forever. Keys are minted per `send` call, never stored on the form. 7. **Tenancy tests.** These enumerate tables and middlewares (`test_tenancy_plane_sites.py:172-180`). C1 must register the new table and middleware there, or the full gate fails late. ### Acceptance criteria (PRD §5, verbatim) **AC-2 — A write is applied exactly once.** A write sent both direct and over the relay (or retried) with one idempotency key changes server state once and returns the same answer to both copies. - verify (API test): send `PUT /api/projects/{id}` twice with the same key → one `updated_at` change, one audit row, identical bodies. Send a create (`POST`, e.g. env var) twice with one key → exactly one row. Send two writes with different keys → both applied. Reuse a key with a different body → refused (4xx), nothing applied. - verify (dev): AC-1's flow while the direct request is released late (it lands after the relay). Observe one audit entry for the save. - States: direct only; relay only; both arrive (either order); key reused with different payload; key expired (the retention window the architect sets). Credentials & reach: (Architecture §6, verbatim rows) | Oracle | Credential / identity / environment | Status | |---|---|---| | AC-2 API tests, AC-4/AC-8 gate | the worktree's `./dc` stack only | **verified**: needs nothing beyond the worktree | | AC-10 deploy run | `jaah-forge pr checks` / `ci log` (pre-authenticated forge) | **verified**: no credential to fetch | Out of scope: PRD §6 (verbatim) — - Do NOT add a hostname, port, certificate, or change cookies or auth to carry writes. The WebSocket relay carrier stays (decision of Task 11936). - Do NOT change the read relay's existing read semantics (race timing, probe, suspect and dropping memory) beyond what writes, streams and uploads need. - Do NOT change any edit page's visible UI: layout, fields, tabs, copy, URLs, routes (AC-9 quality). - Do NOT change React Query's global defaults (`apps/web/src/queryClient.ts`, `retry: false` for queries). - Do NOT change server route business logic. Server changes are limited to (a) idempotency for console writes and (b) the relay accepting writes under that key. - Do NOT touch `apps/site-widget`, `apps/docs`, `apps/jaahd`, `packages/` (AS9). - Do NOT refactor non-edit-page mutations (compliance, billing, playbook list, template cards) beyond what routing them through the choke point needs. - Do NOT add a feature flag (AS8). - Do NOT treat this as Nick-only: no per-user or per-org switch. - Do NOT touch the relay (`routes/read_relay.py`, child C2) or any web code (C3+). Done means: - AC-2 — oracle verbatim above (PRD §5 AC-2 `verify:` lines) - AC-2 API tests cover EVERY §3.1 state: first sight, done-replay (`Idempotent-Replayed: true`, route not re-run), running-wait→replay, running past 25 s → 409 in_progress, stale heartbeat → 409 outcome_unknown, different fingerprint → 422 key_reused (nothing applied), >1 MiB answer → 409 replay_unavailable, 4xx/5xx and unhandled-exception answers stored and replayed (never re-run), expired (>24 h) key applies again, invalid key shape → 400, no header → unchanged behaviour, machine callers untouched, a replay records no second user-activity/audit row. A happy-path-only test suite (store-after-run) FAILS this child (Architecture §5 risk 1). - (dev part of AC-2 — direct released late → one audit entry — is owned by C3's dev run; this child's AC-2 oracle is the API tests.) - A route cancelled mid-run (e.g. the relay's 30 s `wait_for` raising `CancelledError`, which is not an `Exception`) leaves a defined state, pinned by a test: the claim is finalized or goes stale into `idempotency_outcome_unknown` — never re-applied. - `./dc check quick` green; PR merged via the merge lane; Jaah Deploy to dev green. This child owns no dev-browser oracle. Watch that deploy; if it fails, fix and merge again.

Outcome

Built server idempotency middleware for console writes (claim-before-run, replay, sealed answers, prune cron, slow_write kind). Quick gate green, 51 tests; review lanes passed. PR #7686 merged as ddad1818; its dev deploy was superseded by a newer develop deploy. Deferred: per-principal stored-row cap.

Pull request

Artifacts

Activity

  • Draft
  • Ready
  • Runner started
  • Runner ended (done)