feat(api): optimistic concurrency on report editing (ADR D87)

CHANGELOG.md

@@ -8,6 +8,16 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
 
 ## [Unreleased]
 
+### Added
+- **Optimistic concurrency on report editing (ADR D87).** `GET /api/reports/{name}/config` now returns
+  an `ETag`, and `PUT /api/reports/{name}` honours `If-Match`, answering `412 Precondition Failed`
+  when the stored document changed since it was read. This closes the window D86 recorded and left
+  open: the redaction placeholder carries the address of the slot its value came from, so if another
+  editor reorders the destinations between the `GET` and the `PUT`, `destinations[0]` addresses a
+  different bucket than the one the first editor was shown — and the wrong section's credential is
+  restored. The header is **optional**, so clients that do not send it keep working exactly as before;
+  the Builder always sends it.
+
 ### Fixed
 - **Editing a report in the Builder no longer starts from a blank form (ADR D86).** Reported by the
   maintainer: *Edit* prefilled almost nothing the report actually reads from — source type, query, key

DECISIONS.md

@@ -2129,3 +2129,74 @@ formats was opened in the Builder — every step prefilled — its page size cha
 stored document came back with the password intact, the filter intact, the column type and display
 name intact, `90` still a number, and the new page size applied. Saving with no changes at all
 reproduces the original document.
+
+## D87 — Optimistic concurrency on report editing (2026-08-18)
+
+Closes the one gap D86 recorded and did not fix.
+
+### The window
+
+D86's redaction placeholder carries the address of the slot its value came from —
+`${neoreports:redacted:destinations[1]}` — so a client may reorder, remove or retype sections and each
+placeholder still resolves to its own stored value. That is exactly what makes a *single* editor safe.
+
+It is blind in the other direction. The address names a slot of the document **as it was at the
+`GET`**, and `PUT` resolves it against the document **as it is now**. Between the two, another editor
+can reorder the destinations, and `destinations[0]` then addresses a different bucket than the one the
+first editor was shown. The result is the wrong section's credential restored into a section the
+caller defined — the precise outcome the address exists to prevent, reached through the stored side
+rather than the incoming one.
+
+### The decision: an HTTP validator, not a lock
+
+`GET /reports/{name}/config` returns an `ETag`. `PUT /reports/{name}` accepts `If-Match` and answers
+`412 Precondition Failed` when it no longer matches. Three properties matter:
+
+- **The validator is over the *redacted* form — the bytes the client was given.** The first cut hashed
+  the *stored* document, on the reasoning that it is what `Restore` resolves against. The security pass
+  showed that reasoning bought a real weakening: the redacted body and the stored document are
+  byte-identical apart from the redacted values, so publishing a hash of the stored document hands any
+  caller a free, offline **verification oracle** — reconstruct a candidate document with a guessed
+  connection string, hash it, compare. The secret still never leaves the host, but the host would
+  confirm guesses about it, at unlimited speed and with no failed login on the database to notice.
+  Hashing the redacted form removes that entirely: the tag carries nothing the client does not already
+  hold. It is also still the *right* validator, which is the part that makes this cheap — an address is
+  invalidated by a change to the document's **structure** (sections added, removed, reordered), and
+  that structure is wholly visible in the redacted body. A change to a secret *value* moves no address,
+  and an editor sending a placeholder back is asking for whatever is stored now, so resolving to the
+  newer secret is the correct outcome rather than a conflict.
+- **A keyed MAC was considered and rejected.** An HMAC under a per-process key also removes the oracle,
+  but its tags do not survive a restart and, worse, differ between instances — a load-balanced host
+  would answer `412` at random depending on which instance received the save. Deriving the key from
+  Data Protection would fix that and costs a dependency and a key-management story, for no benefit over
+  simply not hashing the secret in the first place.
+- **`If-Match` is optional, and honoured when sent.** Requiring it would break every existing client
+  of an endpoint that shipped one release ago, to protect against a window those clients are not
+  exposed to any more than they were yesterday. The realistic scenario is two people in the Builder,
+  and the Builder always sends it. A caller that omits the header gets exactly D86's behaviour.
+- **The comparison happens against the same read that `Restore` uses.** Reading the stored document
+  once and using it for both the check and the merge is what makes the check meaningful; re-reading
+  would reintroduce the window inside the handler.
+
+`*` is accepted with its RFC 9110 meaning — "if the resource exists" — which the handler has already
+established by the time it looks.
+
+### Why not a lock, a version column, or a last-modified date
+
+A lock needs a lifetime and an owner, and a browser tab that closes has neither. A version number in
+the document would be config the user can edit, and editing it is exactly what the field must survive.
+`Last-Modified` has one-second resolution, which is wider than the window it is guarding.
+
+### Not covered
+
+Two editors whose changes do not overlap still lose one of them — the second gets a `412` and has to
+reload. Merging concurrent edits is out of scope for v1 and would need a per-field model the config
+document does not have.
+
+The check is also **check-then-act, not atomic**: `IReportConfigStore` has no compare-and-swap, so a
+third writer landing between the comparison and `SaveAsync` is still not caught. That residual window
+is microseconds of in-process work rather than the human-scale minutes an editor spends on a form,
+which is the window worth closing and the one this closes. Making it atomic would mean a new
+store-contract method — a change to an interface every custom store implements, for a race that needs
+two saves inside the same instant. Recorded rather than papered over: this narrows the window, it does
+not serialise writes.

docs/STATUS-AND-BACKLOG.md

@@ -75,11 +75,10 @@ enterprise-readiness and test coverage, and shipped everything actionable.
 >    §1 went out in **v2.0.0** (2026-08-08) and §2 is done (`DockerGate`, hard-fail under
 >    `NEOREPORTS_REQUIRE_DOCKER=1`).
 >
-> **State as of 2026-08-18.** With §1–§4 and §6 closed, exactly two things remain open, and each needs
+> **State as of 2026-08-18.** With §1–§4, §1b and §6 closed, **one** thing remains open, and it needs
 > the maintainer:
 >
-> - **§1b — no optimistic concurrency on report editing.** New API surface (`ETag` / `If-Match` /
->   `412`); the concurrent-editor window can restore the wrong section's credential.
+> - ~~**§1b — no optimistic concurrency on report editing.**~~ — **FIXED (ADR D87, 2026-08-18).**
 > - **§5 — PostgreSQL `timetz` drops its zone.** Needs a `Time`/`TimeTz` split in the frozen
 >   `ColumnType` enum plus its own cursor-encoding decision, so it is a next-major item with a design
 >   question attached, not a cast.
@@ -93,18 +92,31 @@ enterprise-readiness and test coverage, and shipped everything actionable.
   updated. Source-breaking for positional callers, so tagged **next-major** in `CHANGELOG.md`
   (Changed → breaking, public API) alongside the #228 removal.
 
-### 1b. Report editing: no optimistic concurrency on PUT (ADR D86, 2026-08-18)
+### 1b. Report editing: no optimistic concurrency on PUT — **FIXED (ADR D87, 2026-08-18)**
 
 Two editors open the same report; the second reorders its destinations and saves; the first then saves
 a placeholder addressed `destinations[0]`, which resolves against the **reordered** stored document and
 restores the wrong section's credential. The carried address is exactly what makes a *single*-editor
 reorder safe, and it cannot see a change made on the stored side between the `GET .../config` and the
 `PUT`.
 
-Not fixed with D86 because the fix is new API surface — an `ETag` on `GET /reports/{name}/config` and
-`If-Match` on `PUT /reports/{name}`, answering `412` when they disagree — and that ADR was already the
-secrets round-trip. Single-worker, single-maintainer v1 (architecture rule 6) makes concurrent editors
-unlikely, not impossible. Recorded so the next change to these endpoints starts from it.
+**Fixed in D87**, with the maintainer's go-ahead on the new API surface. `GET .../config` returns an
+`ETag` and `PUT` honours `If-Match` with a `412`; the header is optional, so clients from before D87
+are unaffected, and a successful `PUT` returns the new tag so an editor can save twice in a row.
+
+The validator is computed over the **redacted** form, not the stored document. The first cut hashed the
+stored one — it is what `Restore` resolves against — and the security pass showed that made the tag a
+free offline **verification oracle**: the two forms are byte-identical apart from the redacted values,
+so a caller could reconstruct candidates, hash them, and confirm a guessed connection string with no
+failed login to notice. Hashing the redacted form carries nothing the caller does not already hold, and
+is still the right validator, because an address is invalidated by a change to the document's
+*structure* and that structure is fully visible there.
+
+Two things remain uncovered, both recorded in D87: two non-overlapping concurrent edits still cost one
+of them a reload (merging needs a per-field model the document does not have), and the check is
+check-then-act rather than atomic — closing that needs a compare-and-swap on `IReportConfigStore`,
+an interface every custom store implements, for a race orders of magnitude smaller than the human one
+this closes.
 
 ### 2. CI hardening
 - **Fail (not skip) the Testcontainers integration tests when Docker is absent in CI.** — **done**:

src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

@@ -164,7 +164,7 @@
         return group;
     }
 
     private static async Task<IResult> RunReportAsync(
         string name,
         string? mode,
         RunReportRequest? body,
@@ -407,7 +407,13 @@
 
         try
         {
-            return Results.Text(ReportConfigSecrets.Redact(document), "application/json");
+            string redacted = ReportConfigSecrets.Redact(document);
+
+            // ADR D87. Deliberately a validator over what the CLIENT can see: hashing the stored
+            // document instead would let a caller confirm a guessed connection string offline, since
+            // the two differ only in the redacted values.
+            http.Response.Headers.ETag = ReportConfigETag.For(document);
+            return Results.Text(redacted, "application/json");
         }
         catch (ConfigurationException ex)
         {
@@ -597,6 +603,26 @@
                 statusCode: StatusCodes.Status500InternalServerError);
         }
 
+        // Optimistic concurrency (ADR D87), checked against the very document Restore is about to
+        // resolve against — re-reading here would reopen the window it exists to close. A request
+        // without If-Match states no precondition and behaves exactly as it did before D87.
+        if (!ReportConfigETag.Allows(http.Request.Headers.IfMatch, stored))
+        {
+            // Shaped like every other rejection this endpoint returns, not as ProblemDetails: the
+            // client reads `error`, so a ProblemDetails body left it with nothing to show and the
+            // user was told the configuration was invalid instead of being told to reload — which is
+            // the entire point of answering 412 rather than saving.
+            return Results.Json(
+                new
+                {
+                    error = $"'{name}' changed since you opened it — another editor saved it in the " +
+                            "meantime. Reload the report and apply your change again. Saving now could " +
+                            "resolve a held-back value against a section that is no longer the one it " +
+                            "came from.",
+                },
+                statusCode: StatusCodes.Status412PreconditionFailed);
+        }
+
         string document = await ReadBodyAsync(http, cancellationToken).ConfigureAwait(false);
 
         ReportConfig config;
@@ -668,6 +694,12 @@
 
         await ReconcileScheduleAsync(name, compiled, scheduler, http, cancellationToken).ConfigureAwait(false);
 
+        // The validator for what was just stored (ADR D87). Without it an editor's captured tag goes
+        // stale the instant its own save succeeds, so any second save from the same page — a retry
+        // after "Run now" failed to start, a double-click — would be refused with a 412 that names a
+        // conflict with itself.
+        http.Response.Headers.ETag = ReportConfigETag.For(document);
+
         var columns = compiled.Schema.Columns.Select(c => c.Name).ToArray();
         return Results.Ok(new ReportCreatedResponse(name, columns));
     }
@@ -1403,7 +1435,7 @@
 
         try
         {
             SchemaCatalog catalog = await explorer!.GetCatalogAsync(definition!, cancellationToken).ConfigureAwait(false);
             return Results.Ok(ToCatalogResponse(catalog));
         }
         catch (Exception ex) when (ex is not OperationCanceledException)
@@ -1425,7 +1457,7 @@
 
         try
         {
             TablePreview preview = await explorer!
                 .PreviewTableAsync(definition!, schema ?? string.Empty, table, SchemaPreviewTop, cancellationToken)
                 .ConfigureAwait(false);
             return Results.Ok(new TablePreviewResponse(preview.Columns, preview.Rows));

src/Integrations/NeoReports.AspNetCore/README.md

@@ -16,7 +16,7 @@ reports and jobs, via Minimal API.
 | GET | `/reports/{name}/config` | the stored config document, credential-bearing values replaced by `${neoreports:redacted}` (ADR D86) → `404` for a code-registered report |
 | POST | `/reports` | register a report at runtime from a config document → `201` (`409` if the name exists, `400` if the config is invalid) |
 | POST | `/reports/validate` | dry-run compile a config document → `200 { valid, error, name, columns, nameTaken }`; never registers or persists |
-| PUT | `/reports/{name}` | replace a runtime-registered report in one step → `200` (`400` if the config is invalid — nothing is changed, `409` for a code-registered report, `404` if unknown) |
+| PUT | `/reports/{name}` | replace a runtime-registered report in one step → `200` (`400` if the config is invalid — nothing is changed, `409` for a code-registered report, `404` if unknown, `412` if `If-Match` no longer matches the stored document) |
 | DELETE | `/reports/{name}` | remove a runtime-registered report → `204` (`409` for a code-registered report, `404` if unknown) |
 | GET | `/capabilities` | source/format/destination type ids the host has registered |
 | GET | `/jobs` | list jobs, filterable by `status`/`report`/`since`, paged (`limit` ≤ 200, `offset`) |
@@ -39,6 +39,14 @@ by the reserved placeholder `${neoreports:redacted}`; sending that placeholder b
 the user having to retype a connection string, and without the secret ever leaving the host. A
 `${VAR}` placeholder is not a secret and comes back verbatim.
 
+**Concurrent edits.** That same response carries an `ETag`, computed over the **stored** document
+rather than the redacted body. Send it back as `If-Match` on the `PUT` and the engine answers
+`412 Precondition Failed` when another editor saved in between: a placeholder addressed
+`destinations[0]` would otherwise resolve against a document whose destinations have since been
+reordered, restoring the wrong section's credential (ADR D87). The header is **optional** — a request
+that omits it states no precondition and behaves as it did before D87 — and a successful `PUT` returns
+the new `ETag`, so an editor can save twice in a row.
+
 `POST /reports/validate?for={name}` resolves the placeholder the same way, so a dry run means the
 same thing while editing as it does while creating. `POST /reports` rejects the placeholder outright:
 there is no stored document to resolve it against.

src/Integrations/NeoReports.AspNetCore/ReportConfigETag.cs

@@ -0,0 +1,81 @@
+using System.Security.Cryptography;
+using System.Text;
+using Microsoft.Extensions.Primitives;
+using NeoReports.Abstractions;
+using NeoReports.Core.Configuration;
+
+namespace NeoReports.AspNetCore;
+
+/// <summary>
+/// The entity-tag for a report configuration document (ADR D87). Lets an editor detect that the
+/// document changed under it between reading the configuration and saving it back.
+/// </summary>
+/// <remarks>
+/// <para>
+/// Computed over the <b>redacted</b> form — the same bytes the client was given — and never over the
+/// stored document. Hashing the stored document made the tag a free, offline <i>verification oracle</i>
+/// for the very values the endpoint exists to withhold: the redacted body and the stored document are
+/// byte-identical apart from the redacted values, so a holder of the body could reconstruct candidate
+/// documents, hash them, and confirm a guessed connection string without ever contacting the database.
+/// </para>
+/// <para>
+/// It is still the right validator. What a placeholder's address can be invalidated by is a change to
+/// the document's <i>structure</i> — sections added, removed or reordered — and that structure is
+/// wholly visible in the redacted body. A change to a secret <i>value</i> moves no address, and an
+/// editor that sends a placeholder back is asking for whatever is stored now, so resolving to the
+/// newer secret is the correct outcome rather than a conflict.
+/// </para>
+/// </remarks>
+internal static class ReportConfigETag
+{
+    /// <summary>The strong entity-tag for a stored document, quoted, ready for a header.</summary>
+    /// <param name="storedDocument">The document as the config store holds it.</param>
+    /// <exception cref="ConfigurationException">Thrown when the document is not readable.</exception>
+    /// <remarks>
+    /// Takes the <i>stored</i> document and redacts it here rather than offering an overload for the
+    /// already-redacted body: one entry point means no call site can hash the wrong form, which is the
+    /// mistake this whole type is now shaped to prevent.
+    /// </remarks>
+    internal static string For(string storedDocument)
+    {
+        byte[] hash = SHA256.HashData(Encoding.UTF8.GetBytes(ReportConfigSecrets.Redact(storedDocument)));
+
+        // Base64url so the value needs no escaping inside the quotes, and truncated to 128 bits —
+        // this detects an edit, it does not authenticate one, and a shorter header reads better in a
+        // log. Collisions at this width are not reachable by a report being edited twice.
+        return '"' + Convert.ToBase64String(hash, 0, 16).Replace('+', '-').Replace('/', '_').TrimEnd('=') + '"';
+    }
+
+    /// <summary>
+    /// Whether the request may proceed under RFC 9110 <c>If-Match</c>: no header at all means "no
+    /// precondition" (D87 keeps the header optional so clients from before it still work), <c>*</c>
+    /// means "if the resource exists", and otherwise one of the supplied tags has to match.
+    /// </summary>
+    /// <param name="ifMatch">The raw <c>If-Match</c> header values, as the request carries them.</param>
+    /// <param name="storedDocument">The document the request is about to be applied to.</param>
+    /// <exception cref="ConfigurationException">Thrown when the document is not readable.</exception>
+    internal static bool Allows(StringValues ifMatch, string storedDocument)
+    {
+        if (StringValues.IsNullOrEmpty(ifMatch))
+            return true;
+
+        string[] candidates = ifMatch
+            .Where(value => !string.IsNullOrWhiteSpace(value))
+            .SelectMany(value => value!.Split(
+                ',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries))
+            .ToArray();
+
+        // "*" first, and before computing anything: the tag now costs a parse of the stored document,
+        // and a wildcard has already said it does not care what the tag is.
+        if (candidates.Contains("*", StringComparer.Ordinal))
+            return true;
+
+        // A weak tag ("W/…") never satisfies If-Match, which requires strong comparison — it simply
+        // fails to equal the strong tag rather than needing a rule of its own.
+        if (candidates.Length > 0)
+            return candidates.Contains(For(storedDocument), StringComparer.Ordinal);
+
+        // A header present but empty carries no tag to match, so it states no precondition either.
+        return true;
+    }
+}