Skip to content

feat(api): accept a client-supplied idempotency key on the eight non-convergent writes - #241

Open
LKSNDRTMLKV wants to merge 2 commits into
mainfrom
feat/idempotency-keys
Open

feat(api): accept a client-supplied idempotency key on the eight non-convergent writes#241
LKSNDRTMLKV wants to merge 2 commits into
mainfrom
feat/idempotency-keys

Conversation

@LKSNDRTMLKV

Copy link
Copy Markdown
Member

Closes #145.

The premise in #145 needed correcting before it could be built against

The issue motivates itself with "a client that retries a POST after a
timeout". That client does not exist. The bulk importer retries only on
429
— a rejection, so no duplicate is possible — and the CLI does not retry
writes at all. POST /dpp, the issue's own example, has no retrying caller.

Its actual argument survives intact, and is why this is worth doing now:
retrofitting idempotency once clients exist is either a breaking change to
their retry behaviour or a silent one. This is therefore API-surface design
with a small enabling implementation
, not a defect fix.

One real defect is hiding under the same heading — the resolver's scan flush
folds a failed window back in and re-sends it against an additive ingest, so a
read timeout on a committed request double-counts. That is a separate PR, and
it will use this mechanism rather than a private one.

Scope: eight routes, chosen by effect not verb

The test is does a replay create a second thing, or spend something that
cannot be un-spent
. POST /dpp, evidence generation, plugin install, and the
creates behind api-keys, webhooks, facilities, operator-identifiers and bulk
import. Facilities and operator-identifiers are on the list because
retire-not-delete (0013/0014 revoked those DELETE grants) makes a duplicate
permanent.

PUT and every lifecycle transition are excluded — they converge. The honest
qualification is in the code: they converge in state, not in effects; a
second publish appends an audit row and re-signs. The seal outbox already
handles its half correctly (0028 keys on (passport_id, payload_hash)).

Eight, not the ten the design note first counted. POST /credentials and
POST /unsold-goods do not exist on main — they are on an unmerged branch. A
policy entry naming a route nobody serves is worse than a missing one, because
policy_for simply never matches and the table reads as protection that is not
there. every_keyed_route_is_a_route_the_node_serves now fails the build on
exactly that, and I verified it goes red before trusting it.

A key on an unkeyed route is a 400, not a no-op. Silently accepting it
would advertise a protection that is not being recorded.

The four questions

  1. Scope — above.
  2. Storage — one table (0036), unique on (principal, method, path, key),
    24h, swept hourly. path is the matched route template, so a caller
    cannot mint unbounded rows by varying a path parameter. This makes it the
    fourth entry in ops/pg/README.md's DELETE set; grants-check failed
    until that list was edited, which is the gate working.
  3. Same key, different body — SHA-256 over the raw bytes, not
    canonicalised JSON. Canonicalising would invent a normalisation this API
    does not otherwise have. 422 with its own problem type, built from
    Problem directly: api_error derives the URI from
    status.canonical_reason(), so routing it through there would have emitted
    the generic unprocessable-entity and made it indistinguishable from every
    other validation failure. A test asserts all seven refusals have distinct
    type URIs.
  4. What is replayed — the stored response, with Idempotency-Replayed: true
    so a client can tell a working retry from a duplicating one. One carve-out:
    POST /api-keys and POST /webhooks return a secret once, so the secret is
    never stored and the replay carries "secretAlreadyDelivered": true in its
    place. A pointer design would have diverged for every route and could not
    have worked for these two at all. The alternative was parking a live
    credential in a table for a day.

The fifth question, which decides the implementation

A middleware cannot commit its row in the handler's transaction — the
repository ports beneath are per-operation. Hence in_flightcompleted,
with a 60s lease so a crash cannot wedge a key. Three rules: an unexpired claim
answers 409 + Retry-After; a 5xx releases the claim (the client must be able
to retry); a 4xx is recorded (deterministic, cheap to replay).

The residual hole — handler committed, record did not — is unavoidable without
threading the key through nine handlers' transactions, which has a worse
forgetting-failure mode. Its blast radius is today's behaviour, so this is never
a regression. It is a warn!, a CHANGELOG paragraph, and a comment at the exact
line, rather than something to discover.

An unavailable store refuses the write rather than running it: executing
with no record is precisely the outcome the caller asked to be protected from.

Placement

Port and middleware in dpp-common. Two crates mount keyed routes and neither
depends on the other; dpp-common is the only one both reach that has axum,
and nothing here carries a domain type. Putting the port in dpp-types with the
others would have meant dpp-common → dpp-types, dragging dpp-types and
dpp-rules into dpp-resolver's build for nothing. dpp-dal gains a
dpp-common edge instead, which keeps every Postgres repo in the DAL.

The two services resolve the principal differently, and had to: the vault reads
the AuthContext its middleware inserted; the integrator authenticates inside
its handlers, so it derives a truncated SHA-256 of the bearer token — never the
token, since the value becomes a primary-key column.

Verification

  • matched_path_inside_a_nested_route_layer_is_the_full_template pins the
    assumption the entire policy table rests on. Had axum reported only the inner
    segment, every lookup would miss and every keyed route would silently stop
    being keyed, with nothing else noticing.
  • crates/dpp-common/tests/idempotency_flow.rs — 7 end-to-end tests through a
    real router: replay, handler-runs-once, mismatch, unkeyed-route refusal,
    no-key passthrough, fail-closed, in-flight, per-principal scoping.
  • crates/dpp-dal/tests/pg_idempotency.rs — 8 tests against real Postgres for
    what only SQL can answer, including 8 concurrent claimants where exactly one
    may win, and the CHECK that refuses a completed row with no response.
  • fmt-check, clippy -D warnings (workspace and the feature-gated
    integration suites), 87 test binaries, openapi-check (rebundled, Redocly
    clean), and all nine script gates.

Not in this PR

The scan-telemetry double-count, and POST /credentials / POST /unsold-goods
when their branch lands. Both are noted in the code where the next person will
look.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 124 complexity · 18 duplication

Metric Results
Complexity 124
Duplication 18

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@LKSNDRTMLKV

Copy link
Copy Markdown
Member Author

Pushed 258e8e8: migrations-check and grants-check were in direct contradiction, and this PR is the first change to trip it.

grants-check requires ops/pg/README.md to be edited whenever a table gains a DELETE grant — that list lives in the README precisely because a migration cannot be edited. migrations-check watched all of ops/pg/, so it then refused the edit. Any change adding a DELETE-granted table could satisfy one gate or the other and never both.

It only surfaced late because migrations-check diffs base...HEAD, so it sees committed changes only — running it against an uncommitted README edit passes, and it goes red the moment you commit.

Scoped to ops/pg/*.sql, which is what sqlx::migrate! actually checksums; prose beside the migrations cannot stop a node booting, and that is the only failure the gate exists to prevent. Verified both directions on a throwaway branch: still fails on a touched 0028_seal_outbox.sql, no longer flags the README.

@LKSNDRTMLKV LKSNDRTMLKV changed the title Accept a client-supplied idempotency key on the eight non-convergent writes feat(api): accept a client-supplied idempotency key on the eight non-convergent writes Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No client-supplied idempotency key on write endpoints

1 participant