fix(resilience): every HttpClient names its own pipeline instance — '-standard//' named nobody (#4528) - #5664
Conversation
…-standard//' named nobody (#4528) ConfigureHttpClientDefaults hands out a builder with no name, so AddStandardResilienceHandler named the defaults pipeline '-standard' for EVERY client that does not re-register itself, and the registry keyed it by an empty instance: one pipeline, one circuit breaker, one anonymous 'Source: -standard//…' for self-update-handover, deployment-report, the OCI listers, the web fetcher and every other named client. It was read as "some caller uses an unnamed HttpClient". Measured on memex-cloud (read-only Logs actions): every TimeoutRejectedException stack under those lines ends in SelfUpdateHandover.Post — the named self-update-handover client posting to the control inbox — and the bursts coincide with the control instance's replicas (re)starting. An outermost handler now stamps HttpMessageHandlerBuilder.Name onto the request and the defaults' standard handler selects its pipeline instance by it (SelectPipelineBy), so every line reads '-standard/<client>/…' and each client trips its own breaker. Keyed by client name, never by authority: the defaults serve user-supplied URLs and the registry never evicts. Refs #4528 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved compile errors and a broader-than-intended public helper contract must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Updates HTTP resilience so each client uses its own named pipeline, with regression tests and architecture documentation.
Changes:
- Adds per-client pipeline selection and request-name stamping.
- Adds pipeline identity tests.
- Documents the diagnosis, fix, and operational guidance.
| File | Reviewed changes |
|---|---|
test/Memex.Portal.Shared.Test/EveryClientNamesItsOwnResiliencePipelineTest.cs |
Adds pipeline identity coverage; contains compile and async-path issues requiring changes. |
src/MeshWeaver.Documentation/Data/Architecture/LogWatchTriage.md |
Updates timeout attribution guidance. |
src/MeshWeaver.Documentation/Data/Architecture/EveryHttpClientNamesItsPipeline.md |
Documents the incident and fix; contains link and wording nits. |
src/MeshWeaver.Documentation/Data/Architecture.md |
Adds the architecture page to the index. |
memex/aspire/Memex.Portal.ServiceDefaults/ServiceDefaults.cs |
Adds per-client resilience selection; contains a compile issue and broader helper side effects. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| http.AddServiceDiscovery(); | ||
| }) | ||
| // 🚨 ONE PIPELINE INSTANCE PER CLIENT NAME (#4528) — see ClientNameOf. | ||
| .SelectPipelineBy(static _ => ClientNameOf); |
There was a problem hiding this comment.
Not changed — this compiles and runs as written. The overload is HttpStandardResiliencePipelineBuilderExtensions.SelectPipelineBy(this IHttpStandardResiliencePipelineBuilder, Func<IServiceProvider, Func<HttpRequestMessage, string>> selectorFactory) (read from the Microsoft.Extensions.Http.Resilience 10.10.0 assembly, line 87 of the decompiled class): it takes a FACTORY that returns the selector. So static _ => ClientNameOf returning the method group is correct. Evidence: dotnet build -c Release -warnaserror on Memex.Portal.ServiceDefaults gives 0 warnings and 0 errors, and EveryClientNamesItsOwnResiliencePipelineTest passes 3/3. With the call removed, that test fails with the production form '-standard//Standard-Retry', which shows the selector really is in effect.
|
|
||
| Send(factory.CreateClient(Handover)); | ||
| Send(factory.CreateClient(Report)); | ||
| Send(factory.CreateClient()); |
There was a problem hiding this comment.
Not changed — this compiles and runs as written. The parameterless CreateClient() is the extension method HttpClientFactoryExtensions.CreateClient(this IHttpClientFactory) in Microsoft.Extensions.Http, and it calls CreateClient(Options.DefaultName). That is exactly the "unnamed client" case the assertion -standard/(unnamed)/ pins. Evidence: the test project builds with -c Release -warnaserror (0 errors) and the test passes 3/3.
|
⛔ Merge-queue steward: left dequeued — the group build failed on an assertion the flake catalogue does not know.
Not a catalogued flake. Fix the failure, or — with run URLs, an issue and an assertion-message pattern — add it to |
meshweaver-cloud
Bot
added
the
queue-rejected
|
Both of this PR's queue ejections are now fixed. It is ready for the maintainer to re-queue with
|
rbuergi
removed
the
queue-rejected
Refs #4528
What
Source: '-standard//…'actually wasNot "a caller resolving an unnamed
HttpClient".ConfigureHttpClientDefaultshands out a builder with no name, andAddStandardResilienceHandlernames its pipeline{builder.Name}-standard(PipelineNameHelper.GetName, Microsoft.Extensions.Http.Resilience 10.10.0). So the defaults pipeline is-standardfor every client that does not re-register itself — named or not — and with no selector the registry keys itHttpKey("-standard", ""): ONE pipeline, ONE circuit breaker, forself-update-handover,deployment-report,module-published-broadcast,OpenGraphPreview, the OCI tag listers and every Plugins client registered by name.What it named in production (read-only
Logsactions on memex.meshweaver.cloud)Ops/logs-memexcloud-20260924-timeout-stack2/…-2x9rq-stack: everyTimeoutRejectedExceptionunder the-standardlines ends inSelfUpdateHandover.<>c__DisplayClass37_0.<<Post…— the namedself-update-handoverclient posting tohttps://memex.systemorph.com/api/hooks/Hosting/PlatformBuilds.Ops/logs-memexcloud-20260924-totaltimeouts-1440(157 lines) againstOps/logs-memex-20260924-boots-600: the large bursts coincide with the control instance's replicas (re)starting (06:29, 07:07/07:22, 08:22, 09:21/09:32, 10:03–10:08, 12:23/12:47, 13:43 UTC); the 13:43 attempts answered 503 in 1.3 ms (ingress, no ready backend).The change
ServiceDefaults.AddHttpClientResilienceDefaults(new, called byAddServiceDefaults; behaviour otherwise unchanged):DelegatingHandleron every client stampsHttpMessageHandlerBuilder.Nameintorequest.Options;.SelectPipelineBy(_ => ClientNameOf)→ the pipeline instance is the client's name ((unnamed)for the default client).Lines now read
-standard/self-update-handover/Standard-AttemptTimeout; each client has its own breaker. Keyed by name, never by authority (the defaults serve user-supplied URLs; the registry never evicts). The two plugin-registry clients keep their own pipelines (pinned by a test).This does NOT change any budget and does not make the control inbox answer — that is the control instance restarting (documented with the evidence in the new doc page). Service discovery is still registered after resilience, so the handler order is unchanged.
Verification
EveryClientNamesItsOwnResiliencePipelineTest(Memex.Portal.Shared.Test) builds exactly what production builds and reads the identity off Polly's own log line: 3/3 green. Negative control: with theSelectPipelineByline removed it fails with the production formExecution attempt. Source: '-standard//Standard-Re….NonexistentHostIsNotRetriedTest(the Memex portal outbound calls fail DNS resolution for boss-software.ch hosts (Polly retries exhausted) #4613 carve-out on the same pipeline) still green.dotnet build -c Release -warnaserror: Memex.Portal.ServiceDefaults, Memex.Portal.Shared.Test, MeshWeaver.Documentation — 0 warnings, 0 errors each.Docs
New
Doc/Architecture/EveryHttpClientNamesItsPipeline(finding, evidence, fix, what it does not fix), linked from the Architecture topic map;LogWatchTriagecorrected ("empty client name" → the shared defaults pipeline).Pairs-with: none — nothing is removed; one public method is added.
Goes live on a roll of the portals; no recycle needed (the resilience registration is process-start DI).
🤖 Generated with Claude Code