Edwin van den Houdt
40dbcb2606
Correlation id becomes real ASP.NET Core middleware instead of a per-endpoint
read: every request gets one (client-supplied or generated), it's echoed as
an X-Correlation-Id response header, and pushed into the logging scope so
every log line for that request carries it — not just the Submit helper's,
verified against LogBrief which never threads it explicitly.
Idempotency-Key moves from per-HTTP-attempt (defeating its own purpose) to
per-logical-submit: runSubmit mints one key and threads it through a small
bridge (withIdempotencyKey/currentIdempotencyKey) since the NSwag-generated
client has no per-call header hook. Backend gains an IdempotencyStore that
short-circuits a replayed key to the first call's result instead of minting
a second reference — scoped to the Submit-helper endpoints per the WP's own
decision.
GET requests now retry transient failures (rxjs retry({count:2, delay:500}));
writes never auto-retry. Proven with a fake-HttpClient spec
(api-client.provider.spec.ts) rather than a manual network-tab check — the
WP's suggested `?scenario=error` check turned out not to exercise a real
network call at all (the interceptor throws before calling next()), so the
automated test is the actual proof.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
152 lines
8.6 KiB
Markdown
152 lines
8.6 KiB
Markdown
# WP-21 — Resilience seams (correlation-id, idempotency, retry)
|
|
|
|
Status: done (pending commit)
|
|
Phase: 5 — productie-volwassenheid
|
|
|
|
## Why
|
|
|
|
`api-client.provider.ts`'s own header comment lists four cross-cutting seams and
|
|
marks three "done" — but two of the three are only half-done, and the fourth
|
|
(retry/backoff) is an explicit unfilled seam:
|
|
|
|
- **Correlation id**: the FE generates a fresh `X-Correlation-Id` per request
|
|
(`api-client.provider.ts:29`), but the backend only _reads_ it opportunistically
|
|
inside the `Submit` helper (`Program.cs:344`) for log lines — there's no
|
|
middleware, so most endpoints never see or echo it, and it's never returned to
|
|
the caller for support/debugging correlation.
|
|
- **Idempotency key**: generated per-attempt (`api-client.provider.ts:31`), which
|
|
the same comment admits defeats its own purpose — "a real retry would thread a
|
|
STABLE key per logical submit so re-sends dedupe; here it's per-attempt." A retry
|
|
today would double-submit, not dedupe.
|
|
- **Retry/backoff**: not implemented at all — the comment names it as the one
|
|
remaining line to add, never added.
|
|
|
|
## Read first
|
|
|
|
- `src/app/shared/infrastructure/api-client.provider.ts` (the whole seam-comment
|
|
block at the top, lines 10-22, plus `httpClientFetch`'s header-building code)
|
|
- `backend/src/BigRegister.Api/Program.cs:344-368` (`Submit` helper — where
|
|
`X-Correlation-Id` is read today, and the only place)
|
|
- `backend/src/BigRegister.Api/Data/DocumentStore.cs:16` (`AuditEntry` — same
|
|
correlation id shape reused for audit `Actor` today, see `Program.cs:361` passing
|
|
`cid` as the audit actor for post-delivery)
|
|
|
|
## Decisions (pre-made, don't relitigate)
|
|
|
|
- **Correlation id becomes ASP.NET Core middleware**, not a per-endpoint read: every
|
|
request gets a correlation id (client-supplied `X-Correlation-Id` if present,
|
|
else server-generated), it's pushed into the logging scope for every log line in
|
|
that request (not just `Submit`'s), and echoed back as a response header so the
|
|
FE/caller can log it too.
|
|
- **Idempotency key becomes stable per logical operation**, generated once when a
|
|
submit/mutation _starts_ (e.g. once per wizard's submit action) and reused across
|
|
retries of that same logical attempt — not regenerated on every HTTP call. This
|
|
is a FE-side change (where the key is generated) plus a backend-side change
|
|
(actually deduping on it — see Files).
|
|
- **Retry/backoff applies only to idempotent GETs**, using rxjs `retry({ count,
|
|
delay })` in `httpClientFetch`'s pipe, per the existing header comment's own
|
|
suggestion. Writes are never auto-retried (the point of item above is making
|
|
retries _safe_, not making everything retry automatically — a POST retry policy
|
|
is a separate, larger decision about at-least-once semantics best left for when a
|
|
real backend needs it).
|
|
- Server-side idempotency _deduplication_ (actually short-circuiting a repeated key
|
|
to return the first result) is scoped to the submit endpoints only
|
|
(`Program.cs`'s `Submit` helper callers) — not every mutation — since that's
|
|
where the existing seam already concentrates correlation/idempotency handling.
|
|
|
|
## Files
|
|
|
|
- `backend/src/BigRegister.Api/Program.cs` — add correlation-id middleware
|
|
(`app.Use(async (ctx, next) => { … })` near the top of the pipeline, before route
|
|
registration): read-or-generate `X-Correlation-Id`, stash in
|
|
`ctx.Items`/`HttpContext`, push into `ILogger` scope
|
|
(`BeginScope(new Dictionary<string,object>{["CorrelationId"]=cid})`), set it on
|
|
`ctx.Response.Headers` before the response is written.
|
|
- `backend/src/BigRegister.Api/Program.cs` — simplify the `Submit` helper's own
|
|
`cid` read (now redundant with the middleware-populated value; read from
|
|
`HttpContext.Items` or inject via a lightweight accessor) so every log line in
|
|
`Submit` picks up the same id without re-parsing the header.
|
|
- New backend idempotency check: a small in-memory `IdempotencyStore` (same pattern
|
|
as `ApplicationStore`/`DocumentStore` — static dict + lock, ponytail-labeled with
|
|
the upgrade path to a real cache/store) keyed on `Idempotency-Key`, consulted by
|
|
the submit endpoints before calling `SubmissionRules.NewReference()`; returns the
|
|
cached response on a replayed key instead of minting a new reference.
|
|
- `src/app/shared/infrastructure/api-client.provider.ts` — generate the
|
|
`Idempotency-Key` once per logical submit rather than per HTTP attempt (thread it
|
|
in from the caller — likely means the submit commands in `application/submit-*.ts`
|
|
generate and pass the key, not the low-level fetch adapter); add
|
|
`retry({ count: 2, delay: 500 })` (or similar) to the GET-only path in the rxjs
|
|
pipe, gated on `method === 'GET'`.
|
|
- New backend test `backend/tests/BigRegister.Tests/IdempotencyTests.cs` — replay a
|
|
submit with the same `Idempotency-Key`, assert the same reference comes back and
|
|
`SubmissionRules.NewReference()` was not called twice (or assert the observable
|
|
effect: identical response body).
|
|
|
|
## Steps
|
|
|
|
1. Backend middleware for correlation id first (smallest, most mechanical change);
|
|
confirm every existing log line still works and now the id is consistent
|
|
end-to-end, not just inside `Submit`.
|
|
2. Backend `IdempotencyStore` + wiring into the submit endpoints; test the replay
|
|
behavior.
|
|
3. FE: move idempotency-key generation up to the command layer
|
|
(`submit-change-request.ts` and equivalents) so one logical submit = one key
|
|
even if `runSubmit`/the HTTP layer retries underneath.
|
|
4. FE: add GET retry/backoff in `httpClientFetch`; verify it doesn't retry writes
|
|
(assert via a spec on the adapter, or a targeted e2e/manual check with the
|
|
`?scenario=slow` toggle).
|
|
|
|
## Acceptance criteria
|
|
|
|
- [x] Every backend log line for a given request shares one correlation id (not
|
|
just lines inside `Submit`); the id is echoed in the response headers.
|
|
Verified manually: `LogBrief`'s log line — which never interpolates a `Cid`
|
|
itself — now prints `=> CorrelationId:scope-check-5` from the middleware's
|
|
`BeginScope`, and `EndpointTests.Correlation_id_supplied_by_the_caller_is_echoed_back`
|
|
/ `..._is_generated_when_the_caller_omits_it` cover the response header.
|
|
- [x] Replaying a submit with the same `Idempotency-Key` returns the same result
|
|
without minting a second reference (backend test proves this —
|
|
`IdempotencyTests`, 3 cases: same key twice, different keys, a replayed
|
|
rejection).
|
|
- [x] A logical wizard submit generates exactly one `Idempotency-Key`, reused across
|
|
any FE-side retry of that submit (not regenerated per HTTP attempt).
|
|
`runSubmit` mints it once and threads it via `withIdempotencyKey`; covered by
|
|
`api-client.provider.spec.ts`.
|
|
- [x] GET requests retry on transient failure (e.g. simulated via `?scenario=slow`
|
|
or a forced 5xx); POST/PUT/DELETE never auto-retry. Covered by
|
|
`api-client.provider.spec.ts` (3 retries on GET, 1 attempt on POST).
|
|
|
|
## Verification
|
|
|
|
GREEN + `cd backend && dotnet test` (84 passing) — done. Manual: confirmed via curl
|
|
against a locally running backend that `X-Correlation-Id` is echoed (client-supplied
|
|
and server-generated) and that replaying `/api/v1/change-requests` with the same
|
|
`Idempotency-Key` returns the identical `referentie` while a different key mints a
|
|
new one; tailed the console log to confirm the correlation id shows up on a brief
|
|
endpoint's log line that never explicitly threads it.
|
|
|
|
**Deviation**: the `?scenario=error` network-tab check this section originally
|
|
proposed doesn't actually work — `scenario.interceptor.ts`'s `error` case
|
|
`throwError`s before ever calling `next(req)`, so no real request reaches the
|
|
network stack and devtools shows nothing to retry. Automated tests
|
|
(`api-client.provider.spec.ts`, using a fake `HttpClient`-shaped `.request()` so no
|
|
TestBed/HttpClientTestingModule is needed) are the actual proof of the retry
|
|
mechanism instead — a more reliable check than a manual browser pass would have
|
|
been anyway.
|
|
|
|
## Out of scope
|
|
|
|
Retrying writes automatically (explicitly deferred, see Decisions); a durable
|
|
idempotency store surviving restart (in-memory is consistent with the rest of the
|
|
backend's persistence posture — see WP-22 if that changes); circuit breakers or
|
|
more advanced resilience patterns (Polly, etc.) — out of scope for a POC-scale
|
|
seam.
|
|
|
|
## Risks
|
|
|
|
Correlation-id middleware ordering matters — it must run before any endpoint that
|
|
logs, including error-handling middleware, or some log lines will still lack the
|
|
id. The idempotency store trades a small amount of memory for correctness under
|
|
replay; fine at demo scale, but the ponytail comment should name the real upgrade
|
|
(a TTL'd cache) so it isn't mistaken for a production-ready dedup mechanism.
|