Skip to content

feat: transport-agnostic request pipeline for logging, auth and correlation - #3

Merged
Katyara11 merged 2 commits into
mainfrom
feat/request-pipeline
Nov 7, 2026
Merged

Katyara11 merged 2 commits into
mainfrom
feat/request-pipeline

Conversation

@Katyara11

Copy link
Copy Markdown
Member

Summary

Implements the roadmap item: logging, authentication and correlation as transport-agnostic
middleware. DelegatingHandler is where these would normally live and only exists in
HttpClient-land, so anything built on it cannot be reused for RestSharp — this sits above
IHttpTransport instead, for the same reason serialization, retries and the capability check do.

Each stage is independently opt-in, and a client with no stages pays nothing: the pipeline is the
transport call.

services.AddRunaxClient<IRepositoryClient>()
        .UseCorrelation()
        .UseLogging()
        .UseBearerToken(() => tokenCache.Current)
        .WithRetry(3);

Changes

  • IHttpMiddleware, RequestPipelineStep, RequestContext, HttpMiddlewareOrder and the
    internal composer, under src/Runax.Http/Pipeline/.
  • Three built-in stages — LoggingMiddleware, AuthenticationMiddleware (with
    IAccessTokenProvider and DelegateAccessTokenProvider), CorrelationMiddleware.
  • Builder surface — Use(...), UseLogging(...), UseBearerToken(...) ×3, UseCorrelation(...),
    backed by RunaxClientOptions.MiddlewareFactories.
  • Executor — composes the pipeline once per client and invokes it per attempt. +30/−10 lines,
    kept deliberately small.
  • Tests — 153 new (1272 total): ordering, per-attempt invocation, token refresh across a retry,
    correlation stability, Activity/traceparent propagation, every redaction rule, short-circuit.
  • Docs — new docs/pipeline.md, linked from the guides table; src/Runax.Http/README.md;
    CHANGELOG.md.

Type

  • Feature
  • Bug fix
  • Refactor
  • Chore (CI, deps, config)

Checklist

  • Code builds without errors
  • Tests added/updated

Notes for review

Where the pipeline sits relative to retries

This was the real design question, and neither obvious answer works:

Outside the retry loop Inside the retry loop
Token refreshed after a failure stale on every retry ✗ re-read per attempt ✓
Correlation id stable ✓ regenerated per attempt ✗
Log line per attempt one line for three tries ✗ one each ✓

So the pipeline runs inside the loop — once per attempt — and RequestContext is created
once per call and reused across attempts. Stages that must be fresh simply are; anything that
must stay constant lives on the context. CorrelationMiddleware is the whole pattern in one line:

context.CorrelationId ??= Activity.Current?.TraceId.ToHexString() ?? NewTraceId();

context.Attempt distinguishes the tries, so one call yields one id and a log line per attempt.

Logging redaction

Redaction is the default and, in two places, not optional:

  • Bodies are never logged. No option. A body is the likeliest place for a credential or personal
    data; logs ship to third-party aggregators and are kept for months; and a switch like that gets
    enabled during an incident and left on.
  • Header values are an allow-list, empty by default — a deny-list protects the headers someone
    thought of, and the one that leaks is always the one nobody did.
  • Authorization, Proxy-Authorization, Cookie, Set-Cookie render as *** even when
    explicitly allow-listed.
    Enough to answer "was auth attached?" without carrying the token, and
    deliberately not switchable: an option that can print a bearer token into a log aggregator would
    eventually be used.
  • The query string is logged by default, since a path without it rarely identifies the
    request. LogQueryString = false for APIs that take a credential as a query parameter — though
    RXH005 exists partly to stop that happening by accident.

A cancellation logs at request level, not as an error, so a cancelled shutdown does not fill the log
with errors nobody can act on.

Bug found along the way

ToTransportRequest() handed out the context's live header list. On a retry, the next attempt's
SetHeader therefore rewrote a request that had already been sent — and TransportRequest is
documented as immutable with transports entitled to hold one. It surfaced as the first attempt
appearing to carry the second attempt's token, caught by the test written to prove refresh worked.
Headers are snapshotted now, with a test pinning it.

Deliberate limitation

A stage may only change headers. RequiredCapabilities is derived from the body, proxy, timeout
and HTTP version, and the capability check runs before the pipeline — so a stage altering one of
those could introduce a requirement nothing had checked. Lifting this means moving the capability
check inside the loop, which also requires excluding TransportCapabilityException from the retry
catch (it currently derives from TransportException, which the loop catches). That is a design
change rather than a configuration one, and I left it out rather than half-doing it. Documented in
IHttpMiddleware and docs/pipeline.md.

Also left out

  • No span creation. Correlation propagates identity and reads an ambient Activity; creating
    spans, tracing and metrics belong with the separate observability roadmap item.
  • IHttpTransport, the transports and the generator are untouched.

Verification

1272 tests across net8.0/net9.0/net10.0; dotnet build --warnaserror clean, dotnet format --verify-no-changes clean, and the native AOT publish takes a client through all three built-in
stages plus a custom one with zero trim warnings, then runs.

Conflict note

Touches src/Runax.Http/HttpContractExecutor.cs (+30/−10) and CHANGELOG.md, both of which the
parallel [Retry] attribute PR also touches. The executor edits are concentrated in the constructor
and SendWithRetriesAsync's signature plus its send call.

Katyara11 and others added 2 commits November 7, 2026 11:05
…lation

DelegatingHandler is where these three would normally go, and it only exists
in HttpClient-land — so anything built on it cannot be reused for RestSharp or
any other transport. IHttpMiddleware is the replacement, above IHttpTransport
for the same reason serialization, retries and the capability check are.

The interesting decision is where the pipeline sits relative to retries, and
neither obvious answer works. Outside the retry loop, a token refreshed after
a failure never reaches the retry that needed it. Inside, with nowhere to keep
per-call state, a correlation id is regenerated on every attempt — and an id
that differs between the retries of one call cannot correlate them, which is
the only thing it is for.

So the pipeline runs inside the loop, once per attempt, and RequestContext is
created once per call and reused across attempts. Stages that must be fresh
simply are; anything that must stay constant lives on the context.
CorrelationMiddleware is the whole pattern in one line:

    context.CorrelationId ??= Activity.Current?.TraceId.ToHexString()
                              ?? NewTraceId();

Ordering comes from IHttpMiddleware.Order, not registration order, so moving a
Use…() call up or down a fluent chain cannot silently change behaviour.
Correlation outermost, logging next, authentication closest to the wire —
logging outside auth on purpose, so the elapsed time it reports includes
fetching a token, which is latency the caller really experiences.

The logging stage will not log bodies, and that is not configurable. A body is
the likeliest place for a credential or someone's personal data, logs are
routinely shipped to a third-party aggregator and kept for months, and a
switch like that gets turned on during an incident and left on. Header values
are an allow-list rather than a deny-list, empty by default, because a
deny-list protects the headers someone thought of and the one that leaks is
always the one nobody did. Authorization, Proxy-Authorization, Cookie and
Set-Cookie render as *** even when explicitly allow-listed — enough to answer
"was auth attached?" without carrying the token, and with no way to turn it
off, since an option that can print a bearer token into a log aggregator would
eventually be used.

A stage may only change headers. RequiredCapabilities is derived from the body,
proxy, timeout and HTTP version, and the capability check runs before the
pipeline — so a stage altering one of those could introduce a requirement
nothing had checked. Lifting that limit means moving the check, which is a
design change rather than a configuration one, and it is documented as such.

Also fixes a real aliasing bug the work surfaced. ToTransportRequest handed out
the context's live header list, so on a retry the next attempt's SetHeader
rewrote a request that had already been sent — TransportRequest is documented
as immutable and a transport may hold on to one. It appeared as the first
attempt seeming to carry the second attempt's token, found by the test written
to prove refresh worked. Headers are snapshotted now, and a test covers it.

1272 tests; build clean with warnings-as-errors across net8.0/net9.0/net10.0,
format clean, and the native AOT publish takes the full pipeline with zero trim
warnings and runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two features were built in parallel and each passes its own suite, which
says nothing about the pair. These four tests are the behaviour neither could
reach alone: the pipeline's "stages re-run per attempt" and [Retry]'s "a
declaration turns retrying on by itself" only meet here.

Asserts that a token refreshed between a failure and its retry reaches the
retry when the retry is driven by [Retry] alone with no WithRetry anywhere;
that the first attempt's header is not rewritten by the second, which is the
regression the pipeline work turned up and which is invisible unless something
looks back at an already-sent request; that a correlation id stays put across
an attribute-driven retry, since ids that differ between tries cannot
correlate them; and that WithoutRetries() still wins with middleware in the
way, because an operator reaching for it mid-incident will not stop to check
whether a pipeline changes the answer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Katyara11
Katyara11 force-pushed the feat/request-pipeline branch from 14bd4f9 to 372c0e4 Compare November 7, 2026 09:09
@Katyara11
Katyara11 merged commit eff6175 into main Nov 7, 2026
2 checks passed
@Katyara11
Katyara11 deleted the feat/request-pipeline branch November 7, 2026 09:11
@Katyara11 Katyara11 mentioned this pull request Nov 7, 2026
3 of 6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant