fix(http): stream response bodies

[thomhurst]

Closes #3467.

Streams HTTP response bodies by using ResponseHeadersRead, keeps request/default/HttpClient.Timeout cancellation alive through body consumption, interrupts synchronous and asynchronous reads, and preserves bounded response-body validation for non-success responses.

Cancellation failures from disposed body streams are normalized to OperationCanceledException for configured timeouts, caller cancellation, and per-read cancellation tokens.

Validation:

• HttpTests: 31/31 passed
• scoped whitespace formatting passed

Atomic download follow-up

• Stream downloads into a same-directory temporary file and atomically move into place only after the response body completes.
• Preserve existing destinations and remove temporary files when streaming fails.
• Legacy providers that lack atomic overwrite support now fail before touching an existing destination.n- DownloaderTests`: 8/8.

[claude]

Review: fix(http): stream response bodies (#3566)

Nice direction overall — switching to ResponseHeadersRead and replacing the old LoadIntoBufferAsync full-body buffering with a bounded preview + replay stream is exactly the right shape for fixing the download-memory problem. I found two issues in the new preview/replay path that undercut the goal, plus a couple of smaller notes.

1. (High / correctness) MaxBodySizeToLog <= 0 ("disable truncation") permanently drains the body stream — reintroduces the bug this PR is fixing

HttpContentPreviewReader.ReadAsync (src/ModularPipelines/Http/HttpContentPreviewReader.cs:59-63):

if (maxBytes <= 0 || maxBytes == int.MaxValue)
{
    var unboundedBody = await content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
    return (unboundedBody, false, content, content.Headers.ContentLength);
}

This branch reads the content fully via ReadAsStringAsync, then returns the same original content reference as ReplayContent. For live network-backed HttpContent (a real response, not something like StringContent), the underlying stream is single-pass — once ReadAsStringAsync drains it, there is nothing left to replay. Setting response.Content = replayContent is a no-op (same object), so the "replay" guarantee silently doesn't hold here.

MaxBodySizeToLog is documented as "Set to 0 or negative to disable truncation" (HttpLoggingOptions.cs:72) — i.e. "log the whole body, no size cap," not "skip preview/replay entirely." That's a realistic, reachable configuration (e.g. someone wants full-body logging for a JSON API call), and any code that reads response.Content afterwards (a module reading ReadFromJsonAsync, DownloadFileAsync, or EnsureSuccessStatusCodeWithContentAsync on a later failure) will get an empty/short body. This is the same class of bug as the original issue (#1610) this PR is closing, just reintroduced for the "no truncation" configuration.

Suggested fix: make the unbounded branch go through the same buffer-and-wrap strategy as the bounded branch (read into a growable buffer / MemoryStream, then wrap in the same kind of prefix-replay stream) instead of special-casing it to return the original content untouched.

2. (High / security) Secrets that straddle the preview boundary are no longer obfuscated

Previously (HttpResponseFormatter/HttpRequestFormatter, pre-PR): the full body was read, obfuscated, and only then truncated to maxBodySize characters for display — so a secret appearing anywhere in the displayed prefix was always matched against the complete string before slicing.

Now: HttpContentPreviewReader.ReadAsync reads only the first maxBytes + 1 raw bytes, and obfuscation runs afterwards on that truncated preview (AppendBodyAsync in both formatters: _secretObfuscator.Obfuscate(body, null) where body is already cut short). SecretObfuscator.Obfuscate (src/ModularPipelines/Engine/SecretObfuscator.cs) does exact substring matching against known secret values — if a secret value in the body starts inside the preview window but extends past maxBytes (e.g. an API key embedded in a JSON response, truncated mid-token by the default 4096-byte window), the exact match fails on the incomplete fragment and the partial secret is printed to logs unmasked.

Given "Automatic secret obfuscation in logs" is called out as a core guarantee of this library, this is a meaningful regression trade for the memory fix. Options to consider:

• Read a bit more than the display window (e.g. maxBytes + longestKnownSecretLength) before obfuscating, then truncate the obfuscated text down to maxBytes for display — restores the old guarantee for secrets that start within the display window, at a small bounded memory cost.
• At minimum, flag this trade-off in the MaxBodySizeToLog doc comment so users aren't surprised.

No test currently exercises this boundary case — the new HttpFormatterTests use a no-op obfuscator mock, so this wouldn't have been caught.

Minor notes

• HttpContentPreviewReader.ReadAsync allocates new byte[maxBytes + 1] up front — if MaxBodySizeToLog is misconfigured to a very large value this allocates that much memory eagerly per call. Not a blocker, just worth a sanity comment/cap given the whole point of this PR is bounding memory use.
• PrefixReplayStream/HttpContentPreviewReader disposal chain (disposing ownedContent cascades to the underlying stream) looks correct, and the binary-content short-circuit (skipping preview entirely for non-text content types) is a good call — it's what makes real file downloads with response-logging opted-in safe today.

What's good

• ResponseHeadersRead change in Http.cs is correct and covered by a real test (SendAsync_ReturnsAfterHeadersWithoutBufferingResponseBody) that blocks on the body stream to prove headers return first.
• Removing the blanket LoadIntoBufferAsync from ResponseLoggingHttpHandler is right given the new per-formatter replay mechanism.
• Flipping DownloadOptions's default LoggingType to drop Response is a sensible default-safety improvement for downloads.
• Bounded preview + replay design for the common (maxBytes > 0) path is solid and well tested (HttpFormatterTests).

[thomhurst]

Rebased onto current main at 8c247942fb.

Patch equivalence preserved (5e6efe50ee5ea03a2f23c4d92d4e354dc3ba9960 before and after). New head: 2ca4846896.

@codex review

[claude]

Code review

I compared the current commit (2ca4846) against the earlier review left on this PR (submitted against commit 1c2c6b8, which is no longer reachable — looks like a rebase/force-push happened without a code change). The two high-severity issues raised previously are still present in the current diff; nothing in HttpContentPreviewReader.cs, HttpRequestFormatter.cs, or HttpResponseFormatter.cs changed to address them.

1. (High / correctness) MaxBodySizeToLog <= 0 ("disable truncation") still drains the stream irreplayably

src/ModularPipelines/Http/HttpContentPreviewReader.cs (lines 59-63):

if (maxBytes <= 0 || maxBytes == int.MaxValue)
{
    var unboundedBody = await content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
    return (unboundedBody, false, content, content.Headers.ContentLength);
}

This branch fully drains content via ReadAsStringAsync and then returns the same content reference as ReplayContent. For a real network-backed HttpContent, the stream is single-pass, so response.Content = replayContent is a no-op — there is nothing left to read. MaxBodySizeToLog's doc comment says "Set to 0 or negative to disable truncation" (i.e., log the whole body, no cap) — a realistic, reachable setting — and any code that reads response.Content afterward (e.g. ReadFromJsonAsync, DownloadFileAsync) gets an empty body. This reintroduces the exact class of bug (#1610 / #3467) this PR sets out to fix, just for the "no truncation" configuration.

Suggested fix: route the unbounded branch through the same buffer-and-wrap approach as the bounded path (read into a growable buffer, then wrap in a replay stream) instead of returning content untouched.

2. (High / security) Secrets straddling the preview boundary are no longer obfuscated

Previously the full body was read, obfuscated, and then truncated for display, so a secret anywhere in the displayed prefix was matched against the complete string. Now HttpContentPreviewReader.ReadAsync reads only maxBytes + 1 raw bytes, and obfuscation (_secretObfuscator.Obfuscate(body, null) in both formatters) runs on that already-truncated preview. SecretObfuscator.Obfuscate does exact substring matching, so a secret that starts inside the preview window but extends past maxBytes (e.g. an API key in a JSON response cut mid-token by the default 4096-byte window) won't match and gets printed to logs unmasked. Given automatic secret obfuscation is a core guarantee of this library, this is a meaningful regression traded for the memory fix, and there's no test covering this boundary (the new HttpFormatterTests use a no-op obfuscator mock).

Suggested fix: read somewhat more than the display window before obfuscating (e.g. maxBytes + longest-known-secret-length), obfuscate first, then truncate the obfuscated text down to maxBytes for display.

What's good (unchanged from before)

• Switching to HttpCompletionOption.ResponseHeadersRead in Http.cs, with a real test (SendAsync_ReturnsAfterHeadersWithoutBufferingResponseBody) proving headers return before the body is read.
• Removing the blanket LoadIntoBufferAsync call in ResponseLoggingHttpHandler now that per-formatter replay exists.
• Flipping DownloadOptions's default LoggingType to drop Response is a sensible default for downloads, and is covered by a test.
• The bounded preview + replay design (maxBytes > 0) itself is solid and well tested.

Since the underlying code hasn't changed since the last review, these two issues still need to be addressed before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ca4846896

ℹ️ 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".

[thomhurst]

Addressed the current review findings in 9803a9456e and fcb99edaff:

• Configured HTTP timeouts now remain active through streamed response-body consumption.
• Request/response previews now redact registered secrets crossing MaxBodySizeToLog.
• For the older unbounded-replay concern: HttpContent.ReadAsStringAsync buffers the content, so returning the same instance remains replayable. Added explicit request and response regressions for MaxBodySizeToLog = 0 to lock this behavior down.

Validation: HttpFormatterTests 6/6, configured-timeout tests 2/2, scoped error analyzers clean, core Release build 0 errors.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcb99edaff

ℹ️ 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".

[claude]

Code review

I compared the current head (fcb99edaff) against the two high-severity issues raised in the earlier reviews on this PR (unbounded-replay draining the stream, and secrets straddling the preview truncation boundary).

Previously flagged issues — now resolved

1. Unbounded (MaxBodySizeToLog <= 0) replay — I verified this empirically: HttpContent.ReadAsStringAsync() does internally buffer the content (a minimal repro against a genuinely non-seekable, forward-only Stream-backed StreamContent showed a second ReadAsStreamAsync()/ReadAsStringAsync() after the first still returns the full original body). So returning the same content reference as ReplayContent in HttpContentPreviewReader.cs L21-L25 is correct — this concern doesn't reproduce.

2. Secrets straddling the preview boundary — now fixed via the new HttpBodySecretRedactor.RedactPartialSecretAtBoundary (HttpBodySecretRedactor.cs), which detects a known-secret suffix match at the truncation boundary and masks it before the normal obfuscator runs. Covered by new tests (ResponseFormatter_RedactsSecretCrossingPreviewBoundary, RequestFormatter_RedactsSecretCrossingPreviewBoundary). Looks correct.

Also worth calling out: the new TimeoutHttpContent/TimeoutReadStream wrapper (Http.cs) is a good catch on the author's part — switching to ResponseHeadersRead meant the timeout CancellationTokenSources would previously be disposed (via using) as soon as SendAsync returned, silently disabling the configured timeout for anything that reads the body later. The ownership hand-off in WrapResponse/the finally block is correct, and it's covered by SendAsync_ConfiguredTimeoutCancelsStreamedBody.

New issue found in this round

1. (High / correctness) Replacing request.Content with a non-seekable replay stream breaks HTTP redirects and auth-challenge retries — reachable with default settings

HttpRequestFormatter.cs L162-L165:

var (body, isTruncated, replayContent, totalLength) = await HttpContentPreviewReader
    .ReadAsync(content, maxBodySize, cancellationToken)
    .ConfigureAwait(false);
request.Content = replayContent;

replayContent is a StreamContent wrapping PrefixReplayStream, and PrefixReplayStream.CanSeek => false. I verified directly that StreamContent over a non-seekable stream throws InvalidOperationException: "The stream was already consumed. It cannot be read again." the second time it's serialized.

Before this PR, AppendBodyAsync never reassigned request.Content — it just read the body via ReadAsStringAsync() and left the original (often seekable/re-buffered) content in place. Now, for the default configuration (LogRequestBody = true, MaxBodySizeToLog = 4096 > 0, so the bounded path runs for any request with a body), request.Content is swapped for a stream that can only be serialized once.

Concretely: POST/PUT a body to an endpoint that responds 307/308 (which — unlike 301/302/303 — preserve the method and body and are auto-retried by SocketsHttpHandler when AllowAutoRedirect is on, the default), or one that issues an NTLM/Negotiate/Digest challenge requiring the body to be resent. HttpClient re-serializes the same HttpContent instance for the retry and throws instead of following the redirect/challenge. This is the exact same class of bug (single-pass stream where a replay is required) the PR is fixing on the response side, just newly introduced on the request side by applying the same replay-stream pattern there.

Suggested direction: for the request-formatter path specifically, prefer a seekable buffer (e.g. MemoryStream-backed replay content, or rewind-capable content) so the underlying HttpContent can seek back to the start on retry, rather than the forward-only PrefixReplayStream used for the response side.

Minor / worth a look

TimeoutHttpContent (TimeoutHttpContent.cs L26-L58) only overrides the async SerializeToStreamAsync/CreateContentReadStreamAsync overloads. I confirmed with a minimal repro that an HttpContent subclass that only overrides the async overloads throws NotSupportedException from the synchronous SerializeToStream/ReadAsStream/CopyTo APIs (the base class's fallback). So whenever a timeout is configured (HttpOptions.Timeout or PipelineOptions.DefaultHttpTimeout), any caller that reads response.Content synchronously instead of via the *Async APIs will get a NotSupportedException where it previously worked. Narrower than the issue above (needs a configured timeout + sync content access), but worth closing while this file is fresh.

---

No CLAUDE.md compliance issues found in the changed files.

[thomhurst]

Addressed the latest review findings in 99389806ea:

• propagate configured cancellation through factory and custom-client HTTP logging
• dispose retryable responses before the next attempt
• decode only complete characters at preview byte boundaries

Focused regression tests pass, scoped formatting/analyzers pass, and ModularPipelines.sln Release build passes with 0 errors (217 existing warnings).

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99389806ea

ℹ️ 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".

[claude]

Code review

Compared the current head (9938980, "fix(http): harden response streaming") against the previous review round on this PR (against fcb99eda), diffing the two commits directly to see what actually changed.

Fixed since the last review

• Cancellation now reaches body reads during logging — PrintRequest/PrintResponse gained CancellationToken overloads (HttpLogger.cs, IHttpLogger.cs via default interface methods) that flow into HttpContentPreviewReader.ReadAsync. Previously a configured HttpOptions.Timeout would stop cancelling once inside the logging/preview read, so a stalled body could hang past the timeout. Covered by the new SendAsync_ConfiguredTimeoutCancelsFactoryResponseLogging test.
• ResilienceHttpHandler disposes the discarded response before retrying (ResilienceHttpHandler.cs OnRetry) — previously every retried attempt's HttpResponseMessage/content leaked. Covered by ResilienceHandler_DisposesRetryableResponseBeforeNextAttempt.
• Preview truncation now decodes only complete characters (HttpContentPreviewReader.DecodeCompletePrefix) — fixes mangled/replacement characters when the byte-level cutoff split a multi-byte UTF-8 sequence. Covered by ResponseFormatter_RedactsSecretWhenPreviewSplitsMultibyteCharacter.

All three are well-targeted and test-covered.

Still open from the last two review rounds — not touched by this push

I diffed fcb99eda..9938980 and confirmed HttpRequestFormatter.cs and TimeoutHttpContent.cs have zero changes between them, so the two previously-flagged issues remain exactly as before, even though the round's commit message says it "addressed the latest review findings":

1. (High / correctness) request.Content is still replaced with a non-seekable stream, still breaking redirects/auth retries for any request with a body under default settings

HttpRequestFormatter.AppendBodyAsync still does request.Content = replayContent; where replayContent wraps PrefixReplayStream, whose CanSeek is hardcoded false. This runs by default (MaxBodySizeToLog = 4096 > 0 is the bounded/common path) for every request that has a body. AllowAutoRedirect isn't overridden anywhere in HttpClientServiceCollectionExtensions.cs, so it's the default true. A 307/308 redirect (or an NTLM/Negotiate/Digest challenge) that needs the same body resent will hit a forward-only stream that's already been drained, and throw.

This is worth calling out as more than a one-line fix: the request side and response side have different replay requirements, and this PR's HttpContentPreviewReader design only really fits the response side (a downloaded response body is legitimately single-pass, which is the whole point of the bounded prefix+tail stream). Before this PR, the request formatter never reassigned request.Content at all — it just read a copy and left the original in place — so this regression is a direct side effect of routing both directions through the same preview/replay utility. Request bodies logged here are already capped by MaxBodySizeToLog/small compared to a downloaded response, so there's no memory-pressure reason to avoid full buffering on this side the way there is for downloads. ResilienceHttpHandler.SendAsync a few files over already does exactly this (ReadAsByteArrayAsync into a buffer, then a fresh ByteArrayContent per attempt) — reusing that same buffer-to-bytes approach for the request-formatter's replacement content (instead of PrefixReplayStream) would give a genuinely seekable/rewindable body and remove this class of bug entirely, rather than patching PrefixReplayStream to pretend to be seekable.

2. (Minor, carried over) TimeoutHttpContent only overrides the async SerializeToStreamAsync/CreateContentReadStreamAsync overloads

Any caller that reads a timeout-wrapped response's content via the synchronous HttpContent APIs (CopyTo, ReadAsStream, etc.) will hit the base class's NotSupportedException fallback. Narrower blast radius than #1 (needs a configured timeout + synchronous content access), so not a blocker, but flagging again since the file was untouched this round.

Since both were already raised twice on this PR and the touched files are byte-for-byte identical to the last-reviewed commit, it's worth double-checking these weren't simply missed when triaging this round's fixes, since the PR description implies they were folded into this round's addressed items.

[thomhurst]

Addressed the latest timeout findings in 935e635d2e after rebasing onto current main:

• preserve finite HttpClient.Timeout across streamed body reads for custom and factory clients
• interrupt synchronous ReadExactly/CopyTo by disposing the underlying stream on timeout
• wrap the .NET 10 synchronous content-stream creation path

Focused timeout regressions pass, scoped formatting/analyzers pass, and the core Release build passes with 0 errors (227 existing warnings).

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 935e635d2e

ℹ️ 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".

[claude]

Code review

Reviewed the current head (935e635, "fix(http): preserve client body timeouts"). I diffed it against the previous reviewed commit (0ca19a6) to scope what's new this round, and re-checked the file state directly for issues raised in earlier rounds.

Still open — raised twice already, unaddressed for the third round in a row

(High / correctness) request.Content is still replaced with a non-seekable stream, still breaking redirects/auth retries for any request with a body under default settings

src/ModularPipelines/Http/HttpRequestFormatter.cs:165 still does request.Content = replayContent;, where replayContent wraps PrefixReplayStream (HttpContentPreviewReader.cs:113, CanSeek => false, hardcoded). I confirmed via git diff that this round's commit only touched Http.cs, TimeoutHttpContent.cs, and HttpTests.cs — HttpRequestFormatter.cs and HttpContentPreviewReader.cs are byte-for-byte unchanged since this was first flagged.

This runs by default (MaxBodySizeToLog = 4096 > 0 is the bounded/common path) for every request that has a body, and AllowAutoRedirect defaults to true. A 307/308 redirect or an NTLM/Negotiate/Digest challenge that needs to resend the same body will hit a forward-only stream that's already been drained, and throw — the same class of single-pass-stream bug this PR is fixing on the response side, reintroduced on the request side. As noted previously, the request side doesn't have the memory-pressure constraint that motivates the response side's bounded prefix+tail design (request bodies here are already capped by MaxBodySizeToLog), so a seekable buffer (e.g. read into a byte array / MemoryStream-backed content, the way ResilienceHttpHandler.SendAsync already does for retries) would fix this correctly instead of reusing the response-side replay stream for a use case it doesn't fit.

Given this has now survived three separate "addressed the latest review findings" rounds without the flagged files changing at all, it'd be worth confirming this is intentionally deferred rather than being missed during triage.

This round's changes (Http.cs, TimeoutHttpContent.cs) — solid, with one fragility worth flagging

• Http.cs: falling back to the HttpClient's own (finite) Timeout when neither HttpOptions.Timeout nor PipelineOptions.DefaultHttpTimeout is set is the right fix — since ResponseHeadersRead was introduced, HttpClient.Timeout only protects the headers phase internally, so without this the client's configured timeout would silently stop covering body reads. Priority ordering and disposal (ownership handoff to TimeoutHttpContent in WrapResponse) look correct.
• TimeoutHttpContent.CreateContentReadStream(CancellationToken) (the missing sync overload flagged in an earlier round) is now implemented — that gap is closed.
• The new "interrupt a blocked synchronous Read/CopyTo by disposing the stream on timeout" mechanism (TimeoutReadStream's _timeoutRegistration) is a reasonable approach for synchronous reads, which don't take a CancellationToken, but it leans on catching ObjectDisposedException specifically (TimeoutHttpContent.cs:144-148, 158-162) to turn the disposal into a clean OperationCanceledException. That's only reliable if disposing the real underlying transport stream mid-read reliably throws ObjectDisposedException. For actual SocketsHttpHandler response streams (chunked/HTTP2 read streams, SslStream, etc.), racing a Dispose() against an in-flight blocking Read() can just as plausibly surface an IOException (sometimes wrapping ObjectDisposedException, sometimes not) — in which case this catch doesn't trigger and a raw, less-friendly exception leaks out instead of the intended OperationCanceledException.
  The new test (SendAsync_ConfiguredTimeoutInterruptsSynchronousBodyRead) doesn't exercise this against a real network stream — it uses BlockingReadStream, a test double whose Dispose() sets a flag and whose blocked Read explicitly does if (_isDisposed) throw new ObjectDisposedException(...). That's asserting the mock behaves the way the mock was written to behave, not that a real HTTP body stream reacts to disposal the same way. Worth either widening the catch to also handle IOException (checking the timeout token in the filter, same as today) or, if real-stream behavior has actually been verified against SocketsHttpHandler, a comment saying so would help the next reader trust the narrow catch.

What's good (carried from earlier rounds, still holds)

• ResponseHeadersRead in Http.cs + secret-boundary redaction (HttpBodySecretRedactor) + bounded preview/replay design for the response side — solid and well tested.
• Retry-response disposal in ResilienceHttpHandler, cancellation propagation through logging, and complete-character preview decoding are all in place and covered.

Net: the timeout hardening in this specific commit is a good, mostly well-tested improvement, but the pre-existing High-severity request-body-replay issue is still the blocker for merge, and it hasn't been touched across three fix rounds now.

[thomhurst]

Addressed all fresh review findings in 036aecbc83:

• log custom-client raw response content before applying TimeoutHttpContent
• keep timeout wrapper outside formatter replay content and explicitly reject pre-cancelled async reads
• buffer logged request content into replayable ByteArrayContent for redirects/auth retries; dispose replaced content
• normalize timeout-triggered synchronous IOException and ObjectDisposedException

Validation: HttpTests 19/19, HttpFormatterTests 7/7, scoped warning-level analyzers/whitespace clean, core Release build 0 errors (227 existing warnings).

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 036aecbc83

ℹ️ 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".

[claude]

Code review

Reviewed the current head (036aecbc83, "fix(http): preserve replay and timeouts") against the prior review rounds on this PR, focusing on what changed since the last reviewed commit (935e635d2e) and re-verifying the three previously-flagged issues.

Previously flagged High-severity issues — all now resolved

1. Request body replaced with a non-seekable stream, breaking redirects/auth retries (raised 3 rounds in a row) — fixed. HttpRequestFormatter.AppendBodyAsync (HttpRequestFormatter.cs:162-166) now calls the new HttpContentPreviewReader.ReadReplayableAsync (HttpContentPreviewReader.cs:64-84), which fully buffers the request body into a byte[] and wraps it in a ByteArrayContent instead of the forward-only PrefixReplayStream. ByteArrayContent.SerializeToStreamAsync can be invoked repeatedly, so HttpClient can now resend the same body for a 307/308 redirect or an auth challenge. This is exactly the direction suggested in earlier rounds (mirroring what ResilienceHttpHandler already does for retries), and it correctly diverges from the response-side PrefixReplayStream approach, which still fits the response/download case since that stream is legitimately single-pass. Covered by RequestFormatter_PreservesBodyForRepeatedSends, which asserts the content can be copied twice and that the original stream is disposed.

2. Secrets straddling the preview truncation boundary going unmasked — fixed via HttpBodySecretRedactor.RedactPartialSecretAtBoundary, unchanged since the previous (confirmed-good) round.

3. Unbounded (MaxBodySizeToLog <= 0) replay draining the stream irreplayably — this was already re-verified as a non-issue in an earlier round (HttpContent.ReadAsStringAsync() buffers via the base class), and the code here is unchanged.

This round's other change — response wrapping order in Http.cs

response = wrapResponse(response) moved from immediately after SendAsync to after status/duration logging and PrintResponse (Http.cs:140-152 for the custom-client path; the factory-client path gets the same effect for free since WrapResponse now runs after httpClient.SendAsync returns from a pipeline that already includes ResponseLoggingHttpHandler). This means response-body logging always reads/replaces the raw content before TimeoutHttpContent wraps it, rather than logging through an extra timeout-wrapper layer. Sensible, and it's the kind of ordering bug that's easy to get wrong silently — good that it's covered by SendAsync_CustomClientLogsRawContentBeforeApplyingBodyTimeout (asserts the type seen by the logger) and SendAsync_CustomClientKeepsTimeoutOutsideLoggedReplayContent (asserts the timeout still cancels a stalled read on the final wrapped content).

TimeoutHttpContent's synchronous read paths now also catch IOException in addition to ObjectDisposedException when converting a timeout-triggered disposal into OperationCanceledException (the fragility flagged in the previous round), with a test parameterized over both exception types.

Minor, non-blocking observations

• The redirect fix is validated at the "can this content be serialized twice" level (RequestFormatter_PreservesBodyForRepeatedSends), not with an end-to-end HttpClient/AllowAutoRedirect test against a real 307 response. The unit test is a reasonable proxy since it exercises the exact mechanism HttpClient relies on for redirects, but a real redirect-following test would be stronger evidence for the scenario that was actually reported as broken, if it's easy to add later.
• With both ResilienceHttpHandler (retries) and the request formatter (logging) enabled on the same request, the request body is now fully buffered into a separate byte[] by each independently — not a bug, just a bit of duplicated memory/work for large request bodies in that combination. Not worth blocking on given request bodies are the smaller side of this PR's concerns (the response/download side is where the real memory risk lives, and that's handled correctly by the bounded preview).
• HttpRequestFormatter and HttpResponseFormatter still duplicate a fair amount of logic (AppendHeaders, IsSensitiveHeader, IsBinaryContent, FormatSize, AppendTruncationMessage) — pre-existing, not introduced by this PR, but worth calling out: this PR's multi-round back-and-forth (the two formatters needing genuinely different replay strategies) is a good illustration of how these two files are similar-but-not-identical in ways that are easy to lose track of when they're separate copy-pasted classes. Extracting the header/size/binary-detection helpers into a shared internal type wouldn't change behavior here, but would reduce the chance that the next similar fix gets applied to one formatter and missed in the other, the way this one initially was for several rounds.

Net: this round closes out the last remaining high-severity issue (request-body replay), and I didn't find any new correctness problems in the diff since the last review. Good to merge from my side, modulo the maintainer's own judgment on the minor points above.

[thomhurst]

Rebased onto current main and force-pushed head 41d6a40ca9f3705aba94f9518882b5a3f6a120d1.

Validation:

• Aggregate patch-id preserved: a47539b1e1f644acf709ff209f0150fadce39688
• HttpTests: 19/19 passed
• HttpFormatterTests: 7/7 passed
• Core Release build: 0 errors (227 pre-existing warnings)
• git diff --check: passed

@codex review

[claude]

Code review

Re-reviewed the current head after this round's push, which the PR description/comments indicate was a rebase-only update onto current main (aggregate patch-id preserved: a47539b1e1f644acf709ff209f0150fadce39688). I diffed the touched files directly against the last reviewed commit (036aecbc83, which received a "good to merge" verdict) rather than trusting the claim — Http.cs, HttpContentPreviewReader.cs, HttpBodySecretRedactor.cs, HttpRequestFormatter.cs, HttpResponseFormatter.cs, TimeoutHttpContent.cs, ResilienceHttpHandler.cs, and ResponseLoggingHttpHandler.cs are unchanged in substance, only rebased.

Status of previously-flagged issues (all resolved, confirmed still fixed)

1. Unbounded (MaxBodySizeToLog <= 0) replay draining the stream irreplayably — confirmed non-issue; HttpContent.ReadAsStringAsync() buffers internally so the same-instance replay in HttpContentPreviewReader.ReadAsync (lines 284-289) holds.
2. Secrets straddling the preview truncation boundary going unmasked — fixed via HttpBodySecretRedactor.RedactPartialSecretAtBoundary, which matches a secret's prefix against the tail of the truncated preview before the normal obfuscator runs, and is covered by dedicated boundary tests.
3. Request body replaced with a non-seekable stream, breaking redirects/auth retries — fixed by routing the request side through HttpContentPreviewReader.ReadReplayableAsync, which fully buffers into a ByteArrayContent (seekable/re-serializable) instead of reusing the response-side's forward-only PrefixReplayStream. This is the right design: the response side has a genuine memory-pressure reason for a bounded, single-pass prefix+tail stream (large downloads), while the request side doesn't (bodies are already capped by MaxBodySizeToLog), so giving the two sides different replay strategies instead of forcing one abstraction to fit both is the correct call here.
4. Cancellation propagation through logging, retry-response disposal in ResilienceHttpHandler, complete-character preview decoding at UTF-8 boundaries, and synchronous-read timeout normalization (IOException/ObjectDisposedException → OperationCanceledException) — all previously verified and unchanged.

Standing minor/non-blocking note (carried over, not a blocker)

HttpRequestFormatter and HttpResponseFormatter still duplicate a fair amount of logic (AppendHeaders, IsSensitiveHeader, IsBinaryContent, FormatSize, AppendTruncationMessage) beyond what HttpBodySecretRedactor now shares. This PR's multi-round history is itself good evidence for extracting these into a shared internal helper: several of the fixes here (secret-boundary redaction, truncation messaging) had to be threaded through both copies in lockstep, which is exactly the kind of drift a shared type would prevent for the next similar change. Not worth blocking on, since behavior is currently correct and identical between the two, but worth a follow-up if this file sees more churn.

Overall

Good shape to merge. The design cleanly separates concerns that looked like they could share one abstraction but didn't (response-side bounded streaming vs. request-side full-buffer replay), and the back-and-forth across review rounds is reflected in solid, targeted test coverage for each fix (HttpFormatterTests, HttpTests, the downloader logging-default regression). No new issues found in this round.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 41d6a40ca9

ℹ️ 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".

[thomhurst]

Rebased onto current main; HTTP, formatter, and downloader suites pass (46/46). @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 102e603926

ℹ️ 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".

[thomhurst]

Addressed four fresh review findings in 2bab96c and rebased onto current main. Focused tests pass 51/51; scoped formatting and core Release build pass. @codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2bab96cccb

ℹ️ 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".

[claude]

Code review

Reviewed commit 2bab96c ("fix(http): harden streamed body handling"), the newest push on this PR, against the four chatgpt-codex-connector findings it says it addresses. Three of the four are fully and correctly fixed (HttpContentPreviewReader's pooled-buffer preview read, Downloader's bounded sibling temp-file name). The fourth — the cancellation-after-read hardening in TimeoutHttpContent — and the IHttpLogger cancellation fix are both only half applied. Two independent Opus review passes converged on the same two gaps, and I verified both directly against the current file content (including confirming, via the real SocketsHttpHandler stream types, that the "disposed stream returns 0 instead of throwing" scenario this commit is designed to guard against is real, not hypothetical).

1. TimeoutReadStream's synchronous Read overloads still treat a disposed stream's 0 as clean EOF

ModularPipelines/src/ModularPipelines/Http/TimeoutHttpContent.cs

Lines 165 to 197 in 2bab96c

	}
	public override int Read(byte[] buffer, int offset, int count)
	{
	timeoutCancellationToken.ThrowIfCancellationRequested();
	try
	{
	return innerStream.Read(buffer, offset, count);
	}
	catch (Exception exception)
	when (timeoutCancellationToken.IsCancellationRequested &&
	exception is ObjectDisposedException or IOException)
	{
	throw CreateTimeoutException(exception);
	}
	}
	public override int Read(Span<byte> buffer)
	{
	timeoutCancellationToken.ThrowIfCancellationRequested();
	try
	{
	return innerStream.Read(buffer);
	}
	catch (Exception exception)
	when (timeoutCancellationToken.IsCancellationRequested &&
	exception is ObjectDisposedException or IOException)
	{
	throw CreateTimeoutException(exception);
	}
	}
	public override Task<int> ReadAsync(

This commit added a post-read effectiveCancellationToken.ThrowIfCancellationRequested() to CopyInnerContentToAsync (line 113) and to the async ReadAsync override (line 224), specifically so that when the timeout registration disposes innerStream mid-read and the inner stream reports that as a 0-byte EOF rather than throwing, the caller doesn't mistake a timed-out/truncated body for a successful, complete read. The two synchronous overloads, Read(byte[], int, int) (L167-180) and Read(Span<byte>) (L182-195), have the identical shape and the identical trigger (the same _timeoutRegistration disposes the same innerStream), but only check cancellation before the read — there's no recheck after innerStream.Read(...) returns. CreateContentReadStream(CancellationToken) (L48-56) hands out this same TimeoutReadStream, so response.Content.ReadAsStream() + Stream.CopyTo/Read is a live, reachable path, not dead code — and real SocketsHttpHandler response streams (ContentLengthReadStream, ChunkedEncodingReadStream) do return 0 rather than throw once their connection is disposed. Net effect: a synchronous consumer of a timed-out response can silently get a truncated body treated as success — the exact bug class this commit fixes for the async path, and the PR description's claim that reads are interrupted "synchronous[ly] and asynchronous[ly]" isn't quite true yet.

Fix is a two-line mirror of the async change — add timeoutCancellationToken.ThrowIfCancellationRequested(); after innerStream.Read(...) in both overloads before returning. Worth also noting the new test double, BlockingReadStream.WaitForSynchronousRead() in HttpTests.cs, ignores the returnEofWhenDisposed flag its ReadAsync honors, so the sync path currently can't even be exercised by the new regression test's scenario.

2. IHttpLogger's PrintRequest default overloads still drop the cancellation token (only PrintResponse was fixed)

ModularPipelines/src/ModularPipelines/Http/IHttpLogger.cs

Lines 27 to 108 in 2bab96c

	Task PrintRequest(
	HttpRequestMessage request,
	IModuleLogger logger,
	CancellationToken cancellationToken)
	{
	return PrintRequest(request, logger);
	}
	/// <summary>
	/// Prints the HTTP request with logging options.
	/// </summary>
	/// <param name="request">The HTTP request to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <param name="options">Options controlling what parts of the request to log.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintRequest(HttpRequestMessage request, IModuleLogger logger, HttpLoggingOptions options);
	/// <summary>
	/// Prints the HTTP request with logging options.
	/// </summary>
	/// <param name="request">The HTTP request to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <param name="options">Options controlling what parts of the request to log.</param>
	/// <param name="cancellationToken">A token to cancel request formatting.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintRequest(
	HttpRequestMessage request,
	IModuleLogger logger,
	HttpLoggingOptions options,
	CancellationToken cancellationToken)
	{
	return PrintRequest(request, logger, options);
	}
	/// <summary>
	/// Prints the HTTP response.
	/// </summary>
	/// <param name="response">The HTTP response to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintResponse(HttpResponseMessage response, IModuleLogger logger);
	/// <summary>
	/// Prints the HTTP response.
	/// </summary>
	/// <param name="response">The HTTP response to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <param name="cancellationToken">A token to cancel response formatting.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintResponse(
	HttpResponseMessage response,
	IModuleLogger logger,
	CancellationToken cancellationToken)
	{
	return PrintResponse(response, logger).WaitAsync(cancellationToken);
	}
	/// <summary>
	/// Prints the HTTP response with logging options.
	/// </summary>
	/// <param name="response">The HTTP response to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <param name="options">Options controlling what parts of the response to log.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintResponse(HttpResponseMessage response, IModuleLogger logger, HttpLoggingOptions options);
	/// <summary>
	/// Prints the HTTP response with logging options.
	/// </summary>
	/// <param name="response">The HTTP response to print.</param>
	/// <param name="logger">The current module logger.</param>
	/// <param name="options">Options controlling what parts of the response to log.</param>
	/// <param name="cancellationToken">A token to cancel response formatting.</param>
	/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
	Task PrintResponse(
	HttpResponseMessage response,
	IModuleLogger logger,
	HttpLoggingOptions options,
	CancellationToken cancellationToken)
	{
	return PrintResponse(response, logger, options).WaitAsync(cancellationToken);
	}

This commit wrapped both PrintResponse(..., cancellationToken) default methods with .WaitAsync(cancellationToken) (L81, L107) so a legacy IHttpLogger that only implements the tokenless overload still has the caller's timeout observed. The two PrintRequest(..., cancellationToken) defaults (L27-33, L52-59) still just do return PrintRequest(request, logger); with the token dropped. Both are live, awaited call sites, not dead code: RequestLoggingHttpHandler.cs:21 and Http.cs:126-132 await PrintRequest(..., cancellationToken) using the same configured-timeout token that PrintResponse now honors. HttpRequestFormatter.AppendBodyAsync itself reads request.Content while formatting, confirming that reading the request body during PrintRequest is normal, intended behavior for an IHttpLogger — so a third-party legacy logger blocking on a slow/streaming request body would hang past the configured timeout, symmetric to the response-side bug this commit just fixed.

Fix mirrors the PrintResponse change: return PrintRequest(request, logger).WaitAsync(cancellationToken); (and the options overload) at L32 and L58.

Everything else in this commit — the ArrayPool-backed preview buffer sizing/return safety, and the bounded same-directory temp filename for downloads — checked out correctly with no issues.

[thomhurst]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 7cf8f8b7d1

ℹ️ 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".