fix(web): correct idempotent replay payload and serialize concurrent duplicates - #1378
Open
marcelo-maciel wants to merge 24 commits into
Open
marcelo-maciel wants to merge 24 commits into
marcelo-maciel wants to merge 24 commits into
Conversation
…duplicates
Two defects in IdempotencyEndpointFilter:
API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result
is the wrapped IResult (Ok<T>/Created<T>), so it stored {"value":...,"statusCode":200}
instead of the wire DTO, and it read Response.StatusCode before the IResult executed,
so a 201 Created replayed as 200. The handler result is now executed into a buffer to
capture the real wire body + status, which is what gets served and cached.
CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests
with the same key both missed the probe and both executed the handler. An atomic in-flight
reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is
registered (the multi-instance case — this stack already requires Redis there for the
shared Data Protection key ring), an in-process set otherwise (single instance). A
duplicate that arrives while the original is still running gets 409 Conflict.
Redis stays optional: without it the app falls back to the in-memory reservation, correct
for a single instance where a cross-container race cannot occur.
…ore) The write went through HybridCache.SetAsync while the probe read IDistributedCache by the raw key. HybridCache keys its L2 entries under its own scheme, so the probe never found the entry and replay silently never engaged — even in production. Proven by un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused, which now passes. Write to the same IDistributedCache, key and serializer the probe uses. Idempotency entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping HybridCache here loses nothing.
Address review on fullstackhero#1333: - Reservation used the 24h response TTL, so a crash between reserving and the finally-release stranded the Redis lock for a day (every retry 409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled from DefaultTtl. - Reserve now fails open on a transient Redis error instead of 500ing the request, matching the best-effort stance of the response write. - Guard the release KeyDeleteAsync so a Redis fault can't throw out of the finally. Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis on reserve/release proceeds without throwing (exercises the Redis NX branch the prior tests skipped).
Three defects surfaced by re-reading the whole filter rather than the delta. The response store was tied to the client's connection, so the retry that idempotency exists to serve re-executed the handler. Two paths caused it, not one: the body write to the client ran before the store, so a closed socket threw and skipped it entirely; and the capture itself ran under RequestAborted, where WriteAsJsonAsync swallows the cancellation and hands back an EMPTY body — which was then cached and replayed as a 200 for the full 24h TTL. The capture now runs with the abort token detached (it writes to an in-memory buffer, never the socket), the store runs before the client write and on CancellationToken.None, and only then does the body go out. Replay dropped every response header, so a replayed 201 arrived without Location: a client that follows the header worked on the first call and broke on the retry. The captured response now carries an allow-listed set (Location, ETag) and replays it. Transport and host-owned headers stay out — a stale Content-Length would corrupt the response. Non-2xx is no longer stored. Faithful status capture made the pre-existing behaviour bite: a transient downstream failure locked the caller out of that key for 24h. A failure is not a record of a committed side effect. CachedIdempotentResponse is no longer a HybridCache type, so its [ImmutableObject(true)] contract (and the CachedTypeContractTests entry asserting it) described a store this filter stopped using. Both dropped. The new Headers property defaults to empty so entries written before it deserialize. Tests: replayed 201 carries Location; a first call whose client disconnects still replays the real DTO body; a non-2xx first response lets the retry run. All three fail on the previous commit and pass here.
…t alone The entry was keyed on tenant + caller key, with nothing identifying the operation. One key reused against a second idempotent endpoint replayed the first endpoint's response and the second request silently never ran. Thirty-one endpoints across eight modules share that namespace, and one of them (self-registration) is anonymous: it resolves no tenant claim, so every caller of it lands in the same "global" bucket. This was latent only for as long as replay never engaged — the fix that makes replay work is what would have put it on the wire. The key now folds in the HTTP method and the route pattern. Two smaller things in the same area. The 409 for an in-flight duplicate and the 400 for an over-long key emitted a bare JSON string, where every other error these endpoints produce is RFC 9457 ProblemDetails; both now match. And an unreadable cache entry (written by another version, or another writer at the same key) let JsonException escape as a 500 — that path only became reachable once replay started engaging at all. It now degrades to a miss and logs. ReleaseReservationAsync also swallows cancellation now, not just faults: it runs in a finally after the response body has already gone to the client, so anything thrown there can only reset the connection on a request that succeeded. Tests: a key reused across two route patterns runs the second handler; an unreadable entry runs the handler (with a valid entry seeded at the same key first, so the assertion can't pass as a plain cache miss); a 204 replays without a fabricated content type. Each fails on a mutated implementation.
The reservation guarded the handler against concurrent duplicates but four holes let one through anyway, or locked a caller out of a key: - The entry was keyed on the caller's `tenant` claim. A root operator scoping requests to different tenants shares one "root" bucket, so one key reused across two targets replays the first tenant's body to the second. Key off the resolved tenant context instead — the one BaseDbContext scopes the side effect to — with the claim as the fallback for a JWT-only request (Finbuckle's claim strategy runs pre-authentication and resolves nothing for those). The raw `tenant` header is deliberately not a fallback: an unresolved header is one Finbuckle refused, and an unvalidated value has no business in a shared key. - The cache was probed once, before the reservation. The original request can store its response and release the lock inside that window; the duplicate then takes the free lock and executes the handler again. Probe once more with the lock held. - The lock was a `:inflight` suffix on the entry key, so a caller key ending in that suffix put its 24h entry exactly where another key's lock goes — every later request with that key 409s for the full response TTL. Give the lock its own prefix. - Release was an unconditional delete. A request that failed open on a Redis blip, or one whose reservation had already expired, freed a lock another request was holding. Release via compare-and-delete against the token the reservation was taken with; failing open carries no token and deletes nothing. The in-process fallback also gains the TTL takeover the Redis branch gets for free: without it a handler that never returns strands the key until the process restarts and every retry 409s forever. Each fix is pinned by a test that was verified to fail when the fix is reverted.
Follow-up from adversarial passes over the whole filter. Each item below is pinned by a test verified to fail when the fix is reverted. - The handler ran under the client's abort token. A disconnect after the side effect committed cancelled the next await inside the handler (an EF read, an outbox write, a Mediator behaviour), so the filter had nothing to store and the client's retry re-executed the side effect — the duplicate this filter exists to absorb. The handler now runs with the token detached; the trade is that a disconnect no longer aborts an idempotent handler. - The probe was the one link that hard-failed. Reserve and store both degrade to a warning when the cache is down, so a `RedisConnectionException` on the probe took every idempotent endpoint down for exactly the clients that send a key. It now fails open as a miss. - A handler that writes the response itself had already started it, so the buffer swap captured nothing and setting the captured status threw. That case now passes through untouched and stores nothing. - The key covered the route pattern but not its values, so `PUT /tickets/1` and `PUT /tickets/2` were one operation: the second replayed the first ticket's response and never ran. It now folds in the resolved route values. - The key was not scoped to the caller, so two users of one tenant reusing a low-entropy key on the same endpoint received each other's response bodies while their own request was silently suppressed. - The 409 said "retry shortly" with no `Retry-After`. It now sends 1 second: the original is normally about to store its response, and the reservation TTL is the worst case, not the hint. Also: options are validated at startup like every other block here (a zero TTL failed silently inside the best-effort write, so nothing was ever stored), `CacheKeys.Tags.Idempotency` no longer claims to be applied, and the cached headers dictionary documents that its comparer does not survive deserialization. Ceilings that stay: no size cap on the buffered response (do not put `.WithIdempotency()` on a streaming endpoint), no lease renewal, and a lock whose Redis may not be the cache's Redis — all three now carry `ponytail:` notes.
A test-quality pass over the suite found assertions that survive the mutation they exist to catch, and branches with no test at all. Each case below now fails when the behaviour it pins is reverted. Assertions that could not fail: - The Lua release script was matched with Arg.Any<string>() while the fake hardcoded compare-and-delete, so swapping the script for an unconditional `del` kept the suite green — the exact bug the script's comment warns about. The script text is asserted now. - `(result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(409)` skips the whole assertion for a result that isn't one, which is precisely the mutation it guards. Cast instead. - The concurrency test relied on the default one-minute ReservationTtl outliving the test; a CI freeze past it hands the key over and fails a correct filter. It pins the TTL explicitly. Branches with no coverage: a handler that throws (the release has to stay in the finally, or one exception strands the key until the TTL), the tenant-claim fallback (collapsing it to "global" puts every JWT-only caller in one bucket and replays across tenants), the refused duplicate's re-probe, the restrictive half of the header allow-list (Set-Cookie must not come back on a replay), the best-effort store, a faulting release, the no-header pass-through, the MaxKeyLength rejection, and an entry stored without the Headers member — the shape a previous version wrote, which has to keep replaying through a rolling deploy. Also drops a stale comment claiming body capture is out of reach; this PR is what made it possible, and the integration suite asserts it end to end.
…mapping The idempotency filter scopes its cache key by ClaimsPrincipal.GetUserId(), which reads ClaimTypes.NameIdentifier only. Until now nothing proved that claim type is present after JwtBearer validates a real issued token: every existing test built the principal by hand, so caller scoping could have been inert in production (every caller collapsing into one bucket) with a green suite. Round-trips a token from TokenService through JsonWebTokenHandler configured with JwtBearerOptions' own MapInboundClaims default, then asserts GetUserId() resolves. Verified with the claim removed from the token as well: the short-form `sub` maps to it, so both shapes IdentityService emits resolve.
…changes The rule described the reservation work from the previous round but not what landed after it, so an agent reading it would still believe the probe hard-fails and the key ignores route values and the caller. Adds the abort-token detachment together with the constraint it implies (no streaming endpoints), the HasStarted pass-through, Retry-After and the startup validation.
…lazily The startup-validation tests resolved IOptions<IdempotencyOptions>.Value, which validates on first access with or without .ValidateOnStart(). Deleting that call left all six green while moving the failure from boot to the first request that carries an Idempotency-Key — a suite that could not see the difference between "rejected at startup" and "rejected once, in production, per process". They now go through IStartupValidator, which is what .ValidateOnStart() registers and what the host runs before serving traffic. Verified: with .ValidateOnStart() removed, 6 of the 10 tests fail.
…sage Asserting only OptionsValidationException let a clause be deleted with every row still green: a zero DefaultTtl also trips "ReservationTtl must not exceed DefaultTtl", so the row aimed at DefaultTtl passed on the wrong clause. Each row now names the failure it expects. Per-clause mutation run: removing any one of the five clauses fails exactly the one row that targets it; removing .ValidateOnStart() fails all six cases, since IStartupValidator is then unregistered.
…ymous endpoints
The cache key scopes by caller, and ResolveCaller returns "anon" for every
unauthenticated request, so on an anonymous endpoint all callers share one bucket.
Two people registering on the same tenant with the same low-entropy key ("1",
"retry") built the identical key: the second replayed the first registrant's 201
with the first registrant's UserId, and their own account was silently never
created. /self-register was the only anonymous idempotent endpoint. It was
unreachable until replay started engaging.
A retry there is already safe without the filter — the unique-email constraint
rejects the duplicate — so the endpoint drops .WithIdempotency() rather than
gaining a body fingerprint.
Deleting one call would leave nothing stopping the next one, so WithIdempotency()
now attaches IdempotentEndpointMetadata: an endpoint filter is invisible in
metadata, and the marker makes the wiring inspectable. IdempotencyWiringTests
walks the endpoint map and fails when an AllowAnonymous() endpoint carries it,
plus a second test that fails if the marker stops being attached, so the first
cannot pass over an empty set.
…ertions UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen returning 401 instead of 204 twice on a loaded machine, then passed four runs in a row (including two with 14 of 16 cores saturated) and passes in isolation and in CI. A bare status assertion gives nothing to work with: the reason JwtBearer rejected the token is in the ProblemDetails body, which the test discarded. The assertions now report method, URL and body on mismatch. Exercised by expecting the wrong status on purpose: the failure message carries the body. This is diagnosis, not a fix. The cause is still unidentified, and this test can still go red.
The Testcontainers packages pull SSH.NET 2025.1.0 transitively, which carries GHSA-q939-rpr3-3284 (CVE-2026-48798, high): ScpClient recursive download writes files outside the target directory. Under TreatWarningsAsErrors that advisory is NU1903 as an error, so `dotnet restore src/FSH.Starter.slnx` fails for the whole solution — Backend CI, CodeQL and the template smoke build all die at restore. Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not clear it. 2026.0.0 is the first patched release, and transitive pinning is already enabled, so this entry alone bumps it — same shape as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins next to it.
CodeQL flagged the `Response.HasStarted` pass-through warning (alert 28, cs/log-forging): it logged `operation`, which folds in `Request.Method`, the resolved route values and the raw request path, so three caller-controlled sources reached a log line. Every other log in the filter already passes `HashKey(...)`, which is why this was the only one. The warning now logs the route pattern read off the endpoint's `RoutePattern`, a literal from the route table, which identifies the endpoint just as well. `operation` is unchanged for the cache key, where the route values have to stay: `PUT /tickets/1` and `PUT /tickets/2` are different operations.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c2dcbaf8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…stAborted Minimal-API parameter binding resolves a handler's CancellationToken from HttpContext.RequestAborted before endpoint filters run, so the handler already holds a copy of the original token. Reassigning the property inside the filter never reached it: a client disconnect after the side effect committed could still cancel an await inside the handler, leaving the filter with nothing to store and letting the retry execute the side effect a second time. Replace the bound argument as well. RequestAborted keeps being reassigned for code that reads it directly instead of taking it as a parameter. The regression test runs a real host over the real filter; a hand-built EndpointFilterInvocationContext skips binding entirely and cannot see this.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…advisories `dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on `main` and on every open PR alike. Advisory-database drift, not a regression from any change: a commit green on 2026-08-10 is red today with no edits. - `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903, GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already depends on the patched 2026.0.0, so the advisory clears with no transitive pin to remember to remove later. Same fix as fullstackhero#1369, so the two do not conflict. - `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902, GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the 8.x line has no patched release, so a transitive pin cannot fix it; the package itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401, past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is excluded from the template, so the scaffold never sees it. Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and `dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings and 0 errors.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:
pull access denied for minio/minio, repository does not exist or may
require 'docker login'
That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:
- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README
The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.
While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).
Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Sep 17, 2026
The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers 404), and it is what `minio-init` runs: without it `dotnet run --project src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull, and the `fsh` bucket is never created, so the first upload fails with NoSuchBucket. Same pinned tag as fullstackhero#1388, which owns the fix, so the copy stays byte-identical to it and can be dropped once that lands.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not help", but the branch also bumps Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two statements cannot both be true, and the bump is the one that is: with the pin removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903 and exits 0. It was carrying a transitive pin that no longer pins anything. The MessagePack pin above it stays: that one is still load-bearing (removing it brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…s one Every concurrency test ran with `multiplexer: null`, so the branch that actually refuses a duplicate in a multi-instance deployment — `StringSetAsync(..., When.NotExists)` coming back false — was never executed. The one test that does share a keyspace asserts the opposite case (a key that must NOT be blocked). Verified by mutation: flipping the reservation to `When.Always` turns the new test red and leaves the rest of the suite green, which is exactly the regression that used to be invisible.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
`RequestUploadUrl` returns a presigned URL good for `FilesOptions.UploadUrlTtlMinutes` (15 by default) and cached the response for `DefaultTtl`, 24 hours. A client retrying with the same key an hour later got a 200 carrying a URL that had expired 45 minutes earlier, with no way out except inventing a new key. Dropping `.WithIdempotency()` from the endpoint was the other option and is worse: the handler INSERTs a pending `FileAsset` and pre-checks quota, so the common case (a network retry seconds later) would start duplicating rows to fix the rare one. `WithIdempotency(TimeSpan)` puts the window on the endpoint that knows it. `IdempotentEndpointMetadata` carries it, the filter prefers it over the configured default, and the endpoint reads the value at map time from the same option the handler mints the URL with, so the two cannot drift. A non-positive TTL throws rather than expiring every entry on write, which would leave the endpoint advertising an idempotency it no longer has. Two more from the same review: - A handler returning a bare `string` was captured through `WriteAsJsonAsync`, which quotes it and sends `application/json`. Minimal APIs write `text/plain`, so the first response through this filter differed from the same handler's response without it, and the replay then repeated the difference. - The filter comment and `.agents/rules/security.md` both described anonymous endpoints as a live case of the shared bucket while `IdempotencyWiringTests` fails the build for exactly that. They now describe the floor the code has, not a configuration it forbids.
`IdempotentHandler_Should_RunToCompletion_When_ClientAbortIsSignalled` signalled nothing: the handler slept 20ms on its own token and the assertion passed whether or not the filter detached anything. Deleting `DetachBoundCancellationTokens` left it green. The abort is real now. A middleware ahead of the endpoint publishes a token the test controls as `RequestAborted` (TestServer cannot hang up a live request from the client side), and the handler cancels it mid-flight. The first response then never reaches the client, which is correct and is asserted rather than swallowed: the store is deliberately sequenced ahead of the body write for this case. What the test asserts is the point of the detach, that the retry replays instead of committing the side effect twice. With the detach reverted, both tests in the class go red.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the idempotency filter. It started as three defects (audit findings API-01 and CONC-01, plus a replay path that never engaged) and grew after review: re-reading the whole filter rather than the delta surfaced the rest, listed at the bottom.
API-03 (root): replay never engaged, even in production
The write went through
HybridCache.SetAsyncwhile the probe readIDistributedCacheby the raw key. HybridCache keys its L2 entries under its own scheme, so the raw-key probe never found the entry and idempotency replay silently never engaged, not just in tests. The repo'sChatSendMessageTestsreplay test was skipped with this exact symptom attributed to a "test-env cache split"; un-skipping it proves it was the filter.Fix: write to the same
IDistributedCache, key and serializer the probe uses. Idempotency entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping HybridCache here loses nothing.API-01: replay served the wrong shape and status
The filter cached
SerializeToUtf8Bytes(result)whereresultis the wrappedIResult(Ok<T>/Created<T>), storing{"value":{...},"statusCode":200}instead of the wire DTO, and readResponse.StatusCodebefore theIResultexecuted, so a 201 replayed as 200. The handler result is now executed into a buffer to capture the real wire body and status.CONC-01: concurrent duplicates both executed
probe -> execute -> writehad no atomic reservation, so two concurrent same-key requests both ran the handler. An atomic in-flight reservation now serializes duplicates: RedisSET NXwhen anIConnectionMultiplexeris registered (the multi-instance case, where this stack already requires Redis for the shared Data Protection key ring), an in-process set otherwise (single instance). A duplicate in flight gets 409 Conflict. Redis stays optional.What review added
CancellationToken.None, so the timeout-then-retry that idempotency exists to absorb replays instead of re-executing.LocationandETag, set while theIResultexecutes and previously dropped.lock:prefix instead of a suffix on the entry key, compare-and-delete release against the reservation's own token, and the in-process fallback expires onReservationTtl.HttpContext.RequestAborteddetached. A client hanging up after the side effect committed used to cancel the handler's next await, leaving nothing to store. The trade-off, that a disconnect no longer aborts an idempotent handler, is called out in the review thread and in the docs.Responseitself is passed through; the 409 carriesRetry-After: 1;IdempotencyOptionsis validated on start.Tests
IdempotencyEndpointFilterReplayTests(38 cases) covers each branch: replay preserves 201 and the plain DTO body, allow-listed headers only, concurrent duplicates execute once with the loser getting 409, probe and store failures fall through to execution, reservation release under a thrown handler.OptionsDefaultsTestspins the startup validation.ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reusedis un-skipped and passes — againstMemoryDistributedCache, not Redis.FshWebApplicationFactorysetsCachingOptions:Redisto an empty string and registers no Redis container, so the integration harness never touches one; an earlier version of this line said "against real Redis" and was wrong. The Redis branches are covered at the unit level instead, including the reservation refusal (Conflict_Should_BeRefusedByRedis_When_ADuplicateIsStillInFlight, added after review: every other concurrency test ran in-process, so flipping the reservation toWhen.Alwaysused to leave the suite green). Every fix was mutation-tested: 22 mutations, each reverting one fix, each required to turn its pinning test red, 22 of 22 red.Docs
fullstackhero/docs#240, including the changelog entry and the documented ceilings (buffered response has no size cap, so not for streaming endpoints; no lease renewal; cross-instance dedup depends on the cache being on Valkey).
Unrelated: the dependency bumps, so CI can run at all
The Testcontainers bump to 4.14.0 and the SourceLink bump are not part of this fix: without them
dotnet restore src/FSH.Starter.slnxfails underTreatWarningsAsErrorsonmainand on every open PR. This branch also carried an explicitSSH.NETpin; review showed it pinned nothing once Testcontainers moved to 4.14.0, and it has been removed (see the note at the end).Infra carve-outs, corrected after review. Two things in the out-of-topic hunks were wrong and
are fixed on the branch:
minio/minioto quay.io.minio/mcis gone from Docker Hub too(
hub.docker.com/v2/repositories/minio/mc/answers 404) and it is whatminio-initruns, so bothdotnet run --project src/Host/FSH.Starter.AppHostanddocker compose updied on the pull and thefshbucket was never created. Now pinned to the same quay tag #1388 uses.SSH.NETpin is gone: it pinned nothing. Its own comment claimed bumping Testcontainersdoes not help, but 4.14.0 — which this branch also carries — declares
SSH.NET >= 2026.0.0.Measured rather than argued: with the pin removed,
dotnet restore src/FSH.Starter.slnx --forcereports zero NU1902/NU1903 and exits 0. (The MessagePack pin next to it stays; removing that one
does bring its advisory straight back.)
With both applied,
deploy/docker/docker-compose.ymlandsrc/Directory.Packages.propsare nowgenuinely byte-identical to #1388 (
git diff --exit-code, checked today), which the earlier claimwas not.
Two things this PR changed without saying so, and a second review round.
Not declared until now:
SelfRegisterUserEndpointlost its.WithIdempotency()(430122aa).It is
AllowAnonymous, and the entry key scopes by caller, which is"anon"for everyunauthenticated request: two people picking the same low-entropy key ("1", "retry") collided, and
the second got the first's response instead of an account.
IdempotencyWiringTestsnow fails thebuild if any anonymous endpoint carries the filter, so this cannot come back quietly.
b2770010is a drive-by in
TenantThemeTests: the status assertions carry the response body now, so afailure says what the server answered instead of only which code it was not.
From the second round:
RequestUploadUrlreturns a presigned URL goodfor 15 minutes and the entry lived for the 24-hour default, so a retry an hour later got a 200
carrying a dead URL. Dropping idempotency from that endpoint would have been worse (the handler
INSERTs a pending
FileAsset, so the common case, a retry seconds later, would startduplicating rows).
WithIdempotency(TimeSpan)lets the endpoint cap its own window, read at maptime from the same option the handler mints the URL with so the two cannot drift. A non-positive
TTL throws.
DetachBoundCancellationTokensleft it green. A middleware now publishes a token the testcancels mid-handler as
RequestAborted, and what gets asserted is the point of the detach: theretry replays instead of committing the side effect twice. The first response never reaching the
client is asserted rather than swallowed, since the store is deliberately sequenced ahead of the
body write for exactly that case. With the detach reverted both tests in the class go red.
stringreturn was captured as JSON. Minimal APIs write it astext/plain, so the firstresponse through the filter differed from the same handler without it, quoted and with a
different content type, and the replay repeated the difference.
.agents/rules/security.mddescribed anonymous endpoints as a livecase of the shared bucket, while the gate above fails the build for exactly that. Both now
describe the floor the code has rather than a configuration it forbids.