diff --git a/backend/src/BigRegister.Api/Data/IdempotencyStore.cs b/backend/src/BigRegister.Api/Data/IdempotencyStore.cs new file mode 100644 index 0000000..2635391 --- /dev/null +++ b/backend/src/BigRegister.Api/Data/IdempotencyStore.cs @@ -0,0 +1,25 @@ +namespace BigRegister.Api.Data; + +/// +/// In-memory idempotency-key ledger for the submit endpoints (Program.cs's `Submit` +/// helper): a replayed `Idempotency-Key` returns the exact result computed the first +/// time, instead of re-running the submission and minting a new reference. +/// ponytail: one global lock + no TTL/eviction — fine for a single-process demo; +/// swap for a TTL'd cache before this ever serves real traffic (an unbounded +/// dictionary keyed on client-supplied strings is a memory leak at scale). +/// +public static class IdempotencyStore +{ + private static readonly Dictionary _results = new(); + private static readonly object _gate = new(); + + public static bool TryGet(string key, out IResult? result) + { + lock (_gate) return _results.TryGetValue(key, out result); + } + + public static void Set(string key, IResult result) + { + lock (_gate) _results[key] = result; + } +} diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index 12ecc1d..b67e323 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -8,6 +8,7 @@ using BigRegister.Domain.Documents; using BigRegister.Domain.Intake; using BigRegister.Domain.Registrations; using BigRegister.Domain.Submissions; +using Microsoft.Extensions.Logging.Console; var builder = WebApplication.CreateBuilder(args); @@ -20,6 +21,9 @@ builder.Services.ConfigureHttpJsonOptions(o => o.SerializerOptions.PropertyNamingPolicy = JsonNamingPolicy.CamelCase; o.SerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull; }); +// So the correlation-id scope pushed by the middleware below actually shows up in +// the console, not just in memory for a formatter that never renders it. +builder.Logging.AddSimpleConsole(o => o.IncludeScopes = true); const string SpaCors = "spa"; builder.Services.AddCors(o => o.AddPolicy(SpaCors, p => @@ -27,6 +31,21 @@ builder.Services.AddCors(o => o.AddPolicy(SpaCors, p => var app = builder.Build(); +// Every request gets a correlation id (client-supplied X-Correlation-Id if present, +// else generated), pushed into the logging scope for every log line the request +// produces (not just the Submit helper's) and echoed back as a response header for +// support/debugging correlation. Runs first so nothing downstream logs without it. +app.Use(async (ctx, next) => +{ + var cid = ctx.Request.Headers.TryGetValue("X-Correlation-Id", out var v) && !string.IsNullOrEmpty(v) + ? v.ToString() + : Guid.NewGuid().ToString(); + ctx.Items["CorrelationId"] = cid; + ctx.Response.Headers["X-Correlation-Id"] = cid; + using (app.Logger.BeginScope("CorrelationId:{CorrelationId}", cid)) + await next(ctx); +}); + app.UseSwagger(); app.UseSwaggerUI(); app.UseCors(SpaCors); @@ -345,33 +364,48 @@ void LogBrief(string action, (BriefStore.Outcome outcome, BriefEntity? entity) r // Audit + outcome for a submit, with NO personal data: only kind, outcome, // generated reference and the caller's correlation id (the observability seam — a -// real system ships this to structured logging / an audit store). +// real system ships this to structured logging / an audit store). A repeated +// Idempotency-Key short-circuits to the first call's result — see IdempotencyStore +// — so a retried submit dedupes instead of minting a second reference. IResult Submit(HttpContext ctx, string kind, string? reject, IReadOnlyList? documents = null) { - var cid = ctx.Request.Headers.TryGetValue("X-Correlation-Id", out var v) && !string.IsNullOrEmpty(v) - ? v.ToString() - : "none"; + var cid = ctx.Items.TryGetValue("CorrelationId", out var v) ? (string)v! : "none"; + var idemKey = ctx.Request.Headers.TryGetValue("Idempotency-Key", out var k) && !string.IsNullOrEmpty(k) + ? k.ToString() + : null; + if (idemKey is not null && IdempotencyStore.TryGet(idemKey, out var cached)) + { + app.Logger.LogInformation("submit kind={Kind} outcome=replayed correlationId={Cid}", kind, cid); + return cached!; + } + + IResult result; if (reject is not null) { app.Logger.LogInformation("submit kind={Kind} outcome=rejected correlationId={Cid}", kind, cid); - return Results.Problem(detail: reject, statusCode: StatusCodes.Status422UnprocessableEntity); + result = Results.Problem(detail: reject, statusCode: StatusCodes.Status422UnprocessableEntity); } - - if (documents is not null) + else { - // Link digital documents (blocks later user delete) and record post-delivery - // intent so a caseworker knows to expect the physical document. - DocumentStore.Link(documents.Where(d => d.Channel == "digital" && d.DocumentId is not null).Select(d => d.DocumentId!)); - foreach (var d in documents.Where(d => d.Channel == "post")) - DocumentStore.Audit("post-delivery", d.DocumentId ?? "-", d.CategoryId, cid); + if (documents is not null) + { + // Link digital documents (blocks later user delete) and record post-delivery + // intent so a caseworker knows to expect the physical document. + DocumentStore.Link(documents.Where(d => d.Channel == "digital" && d.DocumentId is not null).Select(d => d.DocumentId!)); + foreach (var d in documents.Where(d => d.Channel == "post")) + DocumentStore.Audit("post-delivery", d.DocumentId ?? "-", d.CategoryId, cid); + } + + var reference = SubmissionRules.NewReference(); + app.Logger.LogInformation( + "submit kind={Kind} outcome=accepted reference={Reference} correlationId={Cid} at={At:o}", + kind, reference, cid, DateTimeOffset.UtcNow); + result = Results.Ok(new ReferentieResponse(reference)); } - var reference = SubmissionRules.NewReference(); - app.Logger.LogInformation( - "submit kind={Kind} outcome=accepted reference={Reference} correlationId={Cid} at={At:o}", - kind, reference, cid, DateTimeOffset.UtcNow); - return Results.Ok(new ReferentieResponse(reference)); + if (idemKey is not null) IdempotencyStore.Set(idemKey, result); + return result; } // Exposed so the integration tests can spin up the app with WebApplicationFactory. diff --git a/backend/tests/BigRegister.Tests/EndpointTests.cs b/backend/tests/BigRegister.Tests/EndpointTests.cs index 1e85d0f..84cfc8d 100644 --- a/backend/tests/BigRegister.Tests/EndpointTests.cs +++ b/backend/tests/BigRegister.Tests/EndpointTests.cs @@ -125,6 +125,22 @@ public class EndpointTests(WebApplicationFactory factory) : IClassFixtu res.EnsureSuccessStatusCode(); } + [Fact] + public async Task Correlation_id_supplied_by_the_caller_is_echoed_back() + { + var req = new HttpRequestMessage(HttpMethod.Get, "/api/v1/notes"); + req.Headers.Add("X-Correlation-Id", "test-cid-123"); + var res = await _client.SendAsync(req); + Assert.Equal("test-cid-123", res.Headers.GetValues("X-Correlation-Id").Single()); + } + + [Fact] + public async Task Correlation_id_is_generated_when_the_caller_omits_it() + { + var res = await _client.GetAsync("/api/v1/notes"); + Assert.NotEmpty(res.Headers.GetValues("X-Correlation-Id").Single()); + } + // --- Document upload --- private static MultipartFormDataContent UploadForm(string localId, string categoryId, string wizardId, string fileName, string contentType) diff --git a/backend/tests/BigRegister.Tests/IdempotencyTests.cs b/backend/tests/BigRegister.Tests/IdempotencyTests.cs new file mode 100644 index 0000000..a800e5e --- /dev/null +++ b/backend/tests/BigRegister.Tests/IdempotencyTests.cs @@ -0,0 +1,69 @@ +using System.Net.Http.Json; +using BigRegister.Api.Contracts; +using Microsoft.AspNetCore.Mvc.Testing; + +namespace BigRegister.Tests; + +public class IdempotencyTests(WebApplicationFactory factory) : IClassFixture> +{ + private readonly HttpClient _client = factory.CreateClient(); + + private static HttpRequestMessage ChangeRequestWithKey(string key) + { + var req = new HttpRequestMessage(HttpMethod.Post, "/api/v1/change-requests") + { + Content = JsonContent.Create(new { straat = "Lange Voorhout 9", postcode = "2514 EA", woonplaats = "Den Haag" }), + }; + req.Headers.Add("Idempotency-Key", key); + return req; + } + + [Fact] + public async Task Replaying_the_same_idempotency_key_returns_the_same_reference_not_a_new_one() + { + var key = Guid.NewGuid().ToString(); + + var first = await _client.SendAsync(ChangeRequestWithKey(key)); + first.EnsureSuccessStatusCode(); + var firstBody = await first.Content.ReadFromJsonAsync(); + + var replay = await _client.SendAsync(ChangeRequestWithKey(key)); + replay.EnsureSuccessStatusCode(); + var replayBody = await replay.Content.ReadFromJsonAsync(); + + Assert.Equal(firstBody!.Referentie, replayBody!.Referentie); + } + + [Fact] + public async Task Different_idempotency_keys_are_independent_submissions() + { + var first = await _client.SendAsync(ChangeRequestWithKey(Guid.NewGuid().ToString())); + var second = await _client.SendAsync(ChangeRequestWithKey(Guid.NewGuid().ToString())); + var firstBody = await first.Content.ReadFromJsonAsync(); + var secondBody = await second.Content.ReadFromJsonAsync(); + + Assert.NotEqual(firstBody!.Referentie, secondBody!.Referentie); + } + + [Fact] + public async Task A_rejected_submission_replays_the_same_rejection_not_a_retry() + { + var key = Guid.NewGuid().ToString(); + var badRequest = new HttpRequestMessage(HttpMethod.Post, "/api/v1/change-requests") + { + Content = JsonContent.Create(new { straat = "Straat 1", postcode = "nope", woonplaats = "Den Haag" }), + }; + badRequest.Headers.Add("Idempotency-Key", key); + + var first = await _client.SendAsync(badRequest); + Assert.Equal(System.Net.HttpStatusCode.UnprocessableEntity, first.StatusCode); + + var replayRequest = new HttpRequestMessage(HttpMethod.Post, "/api/v1/change-requests") + { + Content = JsonContent.Create(new { straat = "Straat 1", postcode = "nope", woonplaats = "Den Haag" }), + }; + replayRequest.Headers.Add("Idempotency-Key", key); + var replay = await _client.SendAsync(replayRequest); + Assert.Equal(System.Net.HttpStatusCode.UnprocessableEntity, replay.StatusCode); + } +} diff --git a/docs/backlog/WP-21-resilience-seams.md b/docs/backlog/WP-21-resilience-seams.md index afb3af1..1fd00c3 100644 --- a/docs/backlog/WP-21-resilience-seams.md +++ b/docs/backlog/WP-21-resilience-seams.md @@ -1,6 +1,6 @@ # WP-21 — Resilience seams (correlation-id, idempotency, retry) -Status: todo +Status: done (pending commit) Phase: 5 — productie-volwassenheid ## Why @@ -98,22 +98,41 @@ delay })` in `httpClientFetch`'s pipe, per the existing header comment's own ## Acceptance criteria -- [ ] Every backend log line for a given request shares one correlation id (not +- [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. -- [ ] Replaying a submit with the same `Idempotency-Key` returns the same result - without minting a second reference (backend test proves this). -- [ ] A logical wizard submit generates exactly one `Idempotency-Key`, reused across + 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). -- [ ] GET requests retry on transient failure (e.g. simulated via `?scenario=slow` - or a forced 5xx); POST/PUT/DELETE never auto-retry. + `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`. Manual: `?scenario=error` on a GET-backed page, -confirm a retry attempt happens (network tab shows 2 requests) before the error -state renders; submit a wizard twice with a manually replayed idempotency key (e.g. -via curl against the backend directly) and confirm the second call doesn't create a -duplicate application. +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 diff --git a/src/app/shared/application/submit.ts b/src/app/shared/application/submit.ts index 0703d6e..4ad4acf 100644 --- a/src/app/shared/application/submit.ts +++ b/src/app/shared/application/submit.ts @@ -1,18 +1,23 @@ import { Result, ok, err } from '@shared/kernel/fp'; import { problemDetail } from '@shared/infrastructure/api-error'; +import { withIdempotencyKey } from '@shared/infrastructure/api-client.provider'; /** * Run a mutating API call and fold it into a `Result` — the one place the * try/catch + ProblemDetails-mapping lives, so every `submit-*` command is just * its own payload mapping. The backend re-validates and returns a 422 * ProblemDetails on rejection, surfaced here as the error string. + * + * Also the one place a logical submit's Idempotency-Key is minted — once per + * `runSubmit` call, not per HTTP attempt — so a retry of this same submit + * dedupes on the backend (see `withIdempotencyKey`). */ export async function runSubmit( fn: () => Promise, fallback: string, ): Promise> { try { - return ok(await fn()); + return ok(await withIdempotencyKey(crypto.randomUUID(), fn)); } catch (e) { return err(problemDetail(e, fallback)); } diff --git a/src/app/shared/infrastructure/api-client.provider.spec.ts b/src/app/shared/infrastructure/api-client.provider.spec.ts new file mode 100644 index 0000000..9b4b3e6 --- /dev/null +++ b/src/app/shared/infrastructure/api-client.provider.spec.ts @@ -0,0 +1,85 @@ +import { describe, it, expect } from 'vitest'; +import { of, throwError } from 'rxjs'; +import { HttpClient, HttpErrorResponse, HttpResponse } from '@angular/common/http'; +import { currentIdempotencyKey, httpClientFetch, withIdempotencyKey } from './api-client.provider'; + +/** Minimal stand-in for HttpClient — only `.request(...)` is ever called by the + * adapter under test, so no TestBed/HttpClientTestingModule needed. */ +function fakeHttpClient( + request: (method: string, url: string, options: { headers: Record }) => unknown, +): HttpClient { + return { request } as unknown as HttpClient; +} + +describe('withIdempotencyKey / currentIdempotencyKey', () => { + it('threads the key to every read made inside the wrapped fn', async () => { + const seen: string[] = []; + await withIdempotencyKey('fixed-key', async () => { + seen.push(currentIdempotencyKey()); + seen.push(currentIdempotencyKey()); + }); + expect(seen).toEqual(['fixed-key', 'fixed-key']); + }); + + it('clears the key once the wrapped fn settles', async () => { + await withIdempotencyKey('fixed-key', async () => undefined); + expect(currentIdempotencyKey()).not.toBe('fixed-key'); + }); + + it('falls back to a generated uuid-shaped key when none is pending', () => { + expect(currentIdempotencyKey()).toMatch( + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i, + ); + }); +}); + +describe('httpClientFetch', () => { + it('sends the pending idempotency key as a header for a write, not a fresh one per attempt', async () => { + let sentHeaders: Record | undefined; + const http = fakeHttpClient((_method, _url, opts) => { + sentHeaders = opts.headers; + return of(new HttpResponse({ status: 200, body: '' })); + }); + + await withIdempotencyKey('logical-submit-key', () => + httpClientFetch(http).fetch('/api/v1/change-requests', { method: 'POST' }), + ); + + expect(sentHeaders?.['Idempotency-Key']).toBe('logical-submit-key'); + }); + + // `http.request(...)` itself is only called once per `fetch()` — it returns a + // cold Observable, and `retry` resubscribes to *that*, not to `.request()` + // again (exactly how Angular's real HttpClient triggers a fresh network call + // per subscription). So attempts are counted where the resubscription lands: + // the `throwError` factory, not the outer mock call. + it('retries a failing GET twice before giving up', async () => { + let attempts = 0; + const http = fakeHttpClient(() => + throwError(() => { + attempts++; + return new HttpErrorResponse({ status: 500 }); + }), + ); + + const res = await httpClientFetch(http).fetch('/api/v1/notes', { method: 'GET' }); + + expect(attempts).toBe(3); // 1 original + 2 retries + expect(res.status).toBe(500); + }); + + it('never retries a failing write', async () => { + let attempts = 0; + const http = fakeHttpClient(() => + throwError(() => { + attempts++; + return new HttpErrorResponse({ status: 500 }); + }), + ); + + const res = await httpClientFetch(http).fetch('/api/v1/change-requests', { method: 'POST' }); + + expect(attempts).toBe(1); + expect(res.status).toBe(500); + }); +}); diff --git a/src/app/shared/infrastructure/api-client.provider.ts b/src/app/shared/infrastructure/api-client.provider.ts index cec9e5b..7f1df6a 100644 --- a/src/app/shared/infrastructure/api-client.provider.ts +++ b/src/app/shared/infrastructure/api-client.provider.ts @@ -1,12 +1,34 @@ import { Provider } from '@angular/core'; import { HttpClient, HttpErrorResponse } from '@angular/common/http'; -import { firstValueFrom, timeout, TimeoutError } from 'rxjs'; +import { firstValueFrom, retry, timeout, TimeoutError } from 'rxjs'; import { ApiClient, ProblemDetails } from './api-client'; import { environment } from '../../../environments/environment'; /** Single place every API call passes through: the seam for cross-cutting concerns. */ const REQUEST_TIMEOUT_MS = 10_000; +/** + * A stable Idempotency-Key threaded down from the command layer (one per logical + * submit — see `runSubmit`) rather than minted per HTTP attempt, so a retried + * submit dedupes on the backend instead of double-submitting. The NSwag-generated + * `ApiClient` has no per-call header hook, so `withIdempotencyKey` bridges it here: + * every non-GET call made synchronously inside `fn` picks up the same key. + * ponytail: a module-level variable, not a proper async-context primitive — holds + * up because every submit command calls its adapter synchronously (no await + * before reaching this file); swap for `AsyncLocal`-equivalent if concurrent + * submits ever become possible. + */ +let pendingIdempotencyKey: string | undefined; + +export function withIdempotencyKey(key: string, fn: () => Promise): Promise { + pendingIdempotencyKey = key; + return fn().finally(() => (pendingIdempotencyKey = undefined)); +} + +export function currentIdempotencyKey(): string { + return pendingIdempotencyKey ?? crypto.randomUUID(); +} + /** * Adapts Angular's HttpClient to the fetch-shaped interface the NSwag-generated * client expects, so every API call flows through HttpClient interceptors (the @@ -15,12 +37,14 @@ const REQUEST_TIMEOUT_MS = 10_000; * Angular's HTTP stack — i.e. the one seam to add: * - timeout (done — REQUEST_TIMEOUT_MS), * - correlation id (done — X-Correlation-Id, echoed in backend logs), - * - idempotency key for writes (done — Idempotency-Key; a real retry would thread - * a STABLE key per logical submit so re-sends dedupe; here it's per-attempt), + * - idempotency key for writes (done — Idempotency-Key, stable per logical + * submit via `withIdempotencyKey`/`runSubmit`, so a retry dedupes), * - auth: attach `Authorization: Bearer …` here (one line) when real DigiD lands, - * - retry/backoff: wrap the pipe with rxjs `retry({ count, delay })` here. + * - retry/backoff (done — GET only, `retry({ count: 2, delay: 500 })`; writes are + * never auto-retried, which is exactly what makes the idempotency key above + * matter only for a future/manual retry, not routine traffic). */ -function httpClientFetch(http: HttpClient) { +export function httpClientFetch(http: HttpClient) { return { async fetch(url: RequestInfo, init?: RequestInit): Promise { const method = (init?.method ?? 'GET').toUpperCase(); @@ -28,17 +52,18 @@ function httpClientFetch(http: HttpClient) { ...((init?.headers ?? {}) as Record), 'X-Correlation-Id': crypto.randomUUID(), }; - if (method !== 'GET') headers['Idempotency-Key'] = crypto.randomUUID(); + if (method !== 'GET') headers['Idempotency-Key'] = currentIdempotencyKey(); try { + const request$ = http + .request(method, url as string, { + body: init?.body as string | undefined, + headers, + observe: 'response', + responseType: 'text', + }) + .pipe(timeout(REQUEST_TIMEOUT_MS)); const res = await firstValueFrom( - http - .request(method, url as string, { - body: init?.body as string | undefined, - headers, - observe: 'response', - responseType: 'text', - }) - .pipe(timeout(REQUEST_TIMEOUT_MS)), + method === 'GET' ? request$.pipe(retry({ count: 2, delay: 500 })) : request$, ); // 204/205/304 are null-body statuses — new Response(body, …) throws for any non-null body. const nullBody = res.status === 204 || res.status === 205 || res.status === 304;