Replace live HTTP dependencies in core tests

[thomhurst]

Summary

• replace four live thomhurst.github.io requests with a loopback HTTP fixture
• replace the GitHub release download and retry with deterministic local bytes
• preserve request/response logging and checksum coverage using an in-process TCP server on port 0

Validation

• Release build: ModularPipelines.UnitTests.csproj (0 errors; 60 existing warnings)
• Release focused tests: 7/7 passed (HttpTests and DownloaderTests)
• touched-file dotnet format --verify-no-changes --severity info: passed

Closes #3519

[thomhurst]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2ae4054f21

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

Summary: Replaces the flaky live-network HTTP tests (thomhurst.github.io, a GitHub release download) with a deterministic in-process fixture (LocalHttpServer), closing #3519. The approach is sound overall — tests become hermetic, the download checksum (AEDF5D7C23744269F358814E602AFE89 for "local download fixture") is correct, and the loopback server preserves the request/response logging behavior HttpTests exercises. Binding to port 0 on loopback also avoids collisions under parallel test runs, and each test correctly scopes the server's lifetime with await using.

Suggestion: reuse RichardSzalay.MockHttp instead of a hand-rolled TCP/HTTP server

test/ModularPipelines.UnitTests/ModularPipelines.UnitTests.csproj already references RichardSzalay.MockHttp (pinned in Directory.Packages.props), but no test in the repo currently uses it. This PR instead adds a new ~70-line raw-socket HTTP/1.1 server (Helpers/LocalHttpServer.cs) that binds a TcpListener, hand-parses request lines, and writes response headers as raw ASCII bytes.

There's already an injection seam for a mocked handler: Http.SendAsync (src/ModularPipelines/Http/Http.cs) skips the IHttpClientFactory-resolved client entirely when HttpOptions.HttpClient is set, and still runs the full request/response logging pipeline via SendAndWrapLogging. This PR's own Assert_SendAsync_Logs_As_Expected(customHttpClient: true) case already exercises that path. For the cases that go through the named IHttpClientFactory client instead, HttpClientServiceCollectionExtensions.AddLoggingHttpClients registers each named client with services.AddHttpClient(clientName), which the already-present configureServices callback (GetService<IHttpContext>((_, collection) => ...)) could reconfigure via .ConfigurePrimaryHttpMessageHandler(() => mockHandler).

Why this would be better:

• No real socket/port binding, accept-loop, or manual CancellationTokenSource lifecycle to reason about — the 10s timeout + manual cancel + SocketException/OperationCanceledException swallowing in LocalHttpServer.DisposeAsync/RunAsync is the main source of incidental complexity this PR introduces.
• No hand-rolled HTTP/1.1 wire parsing (reading headers line-by-line, writing a status line + headers as raw bytes) — a well-tested library absorbs that instead of the test suite reimplementing HTTP.
• It's a dependency the project already pays the reference cost for; LocalHttpServer duplicates functionality that dependency exists to provide, and now there are two HTTP fixture implementations to maintain going forward.
• MockHttpMessageHandler still produces a real HttpResponseMessage with headers/content/status flowing through the same logging code, so assertions like Server: ... / Content-Length work unchanged — just via .Respond(...) instead of raw bytes.

This isn't a functional bug — the socket fixture works and tests pass — but it's added surface area and risk that an already-referenced, currently-unused dependency would remove.

No other issues found.

[claude]

Code review

Summary: This PR replaces live-network dependencies (thomhurst.github.io, a GitHub release download) in HttpTests and DownloaderTests with a deterministic in-process fixture (LocalHttpServer), closing #3519. I reviewed the current head (4ef8d66), including the fix applied after the earlier codex review comment.

Verified

• The previously flagged race condition is fixed. The earlier review (codex) correctly flagged that CancellationTokenSource(TimeSpan.FromSeconds(10)) started counting down from construction time, not from DisposeAsync, which could cancel AcceptTcpClientAsync before slow host/pipeline startup even reached SendAsync. The current code (LocalHttpServer.cs:11) uses a plain new CancellationTokenSource() with cancellation only triggered in DisposeAsync, so this is resolved — confirmed by reading the file, not just the PR comment claiming it.
• Checksum correctness: md5("local download fixture") == AEDF5D7C23744269F358814E602AFE89, matches DownloaderTests.cs:20.
• Single-accept design matches actual usage: LocalHttpServer.RunAsync accepts exactly one TCP connection. I checked Http.SendAsync and Downloader.DownloadResponseAsync/DownloadFileAsync — neither issues retries or multiple requests per call (no Polly/retry wrapping in Http.cs), and each test creates its own server instance, so one-shot accept is sufficient and not fragile.
• Removing [Retry(3)] from Can_Download is appropriate now that the dependency is deterministic rather than a flaky external network call.
• await using scoping in each test correctly binds the fixture's lifetime to the test, and binding to port 0 on loopback avoids collisions under parallel runs.

Not addressed (pre-existing suggestion, non-blocking)

An earlier review comment suggested using RichardSzalay.MockHttp (already referenced in Directory.Packages.props but currently unused anywhere in the repo) instead of a hand-rolled TcpListener/raw-HTTP fixture, since HttpOptions.HttpClient already provides an injection seam via ConfigurePrimaryHttpMessageHandler. That suggestion wasn't adopted, and the current 71-line LocalHttpServer still hand-parses request lines and writes raw HTTP bytes. This isn't a bug — the fixture is correct and now race-free — but it remains added surface area (manual socket lifecycle, cancellation swallowing for OperationCanceledException/SocketException) that an already-paid-for dependency would remove. Worth considering for a follow-up if more HTTP fixtures are needed later, but not a blocker for this fix.

No functional issues found. Approving.

[thomhurst]

Rebased onto current main at 2ec5f7b611. Patch-id remained byte-equivalent (c1ec84502d0b2f269060e8677aa5e73775e71b6e); prior 15/15 green checks and review audit remain applicable.

[claude]

Code review

Summary: Rebased onto current main with a byte-equivalent patch-id vs. the prior revision — no functional changes since the last review. Confirms the approach is still sound: thomhurst.github.io and the GitHub release download in HttpTests/DownloaderTests are replaced with an in-process loopback fixture (LocalHttpServer), tests become hermetic, and the checksum for the new fixture body is correct.

Previous suggestion (reuse RichardSzalay.MockHttp) is still unaddressed

The prior review on this PR pointed out that test/ModularPipelines.UnitTests/ModularPipelines.UnitTests.csproj already references RichardSzalay.MockHttp (pinned in Directory.Packages.props), yet no test uses it — and this PR adds a second HTTP test fixture instead (Helpers/LocalHttpServer.cs, ~70 lines of raw TcpListener + hand-parsed HTTP/1.1).

I re-verified this is still applicable after the rebase:

• Downloader.DownloadResponseAsync (src/ModularPipelines/Context/Downloader.cs:59-72) always goes through _http.SendAsync(new HttpOptions(request) { HttpClient = options.HttpClient, ... }) — so DownloaderTests.Can_Download could pass a HttpClient backed by MockHttpMessageHandler instead of standing up a real socket server.
• HttpTests's customHttpClient: true case already proves this seam works today (it passes HttpOptions.HttpClient directly), and the request/response logging handlers (RequestLoggingHttpHandler/ResponseLoggingHttpHandler) run against the resulting HttpResponseMessage regardless of where it came from — so header assertions like Server: ... would keep working unchanged with .Respond(...).

Since the diff is unchanged from the previously-reviewed revision, this isn't a new issue, but it also hasn't been resolved — worth a decision (either adopt MockHttp and delete LocalHttpServer.cs, or explicitly decide raw-socket fidelity is wanted here, e.g. to exercise the real HttpClient/socket pipeline for these specific tests) before merge, rather than carrying two parallel HTTP fixture mechanisms in the test project long-term.

No new issues found in this revision.

[thomhurst]

Rebased onto current main at 8c247942fb. Patch equivalence preserved (c1ec84502d0b2f269060e8677aa5e73775e71b6e before and after). New head: 95f47779fd.

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 95f47779fd

ℹ️ 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 (verification pass)

Summary: Confirmed the current head (95f47779fd) is patch-id identical to the previously fully-reviewed revision — no functional changes since the last review. Re-verified independently by reading the file at this commit rather than trusting the "no changes" claim:

• LocalHttpServer.cs: _cancellationTokenSource is a plain new() with no timeout, cancelled only in DisposeAsync — the earlier-flagged race (timer starting at construction instead of disposal, which could cancel AcceptTcpClientAsync before the test even issued its request) is confirmed fixed.
• Single-accept design matches usage: neither Http.SendAsync nor Downloader.DownloadResponseAsync retries or issues multiple requests per call, so accepting exactly one connection per server instance is correct, not fragile.
• Checksum in DownloaderTests.cs (AEDF5D7C23744269F358814E602AFE89) matches MD5 of "local download fixture".
• await using scoping ties each server's lifetime to its test; binding to port 0 on loopback avoids collisions under parallel runs.
• Dropping [Retry(3)] on Can_Download is correct now that the dependency is deterministic.

Still open, non-blocking: the earlier suggestion to reuse RichardSzalay.MockHttp (already referenced in Directory.Packages.props, currently unused anywhere in the repo) instead of hand-rolling a TcpListener-based HTTP/1.1 fixture hasn't been adopted. HttpOptions.HttpClient already provides an injection seam (ConfigurePrimaryHttpMessageHandler), and this PR's own customHttpClient: true case in HttpTests proves that seam works. Using MockHttpMessageHandler would remove the manual socket accept-loop/cancellation lifecycle and raw HTTP/1.1 byte-writing this PR introduces, at no cost since the dependency is already paid for. Worth a follow-up decision, but not a blocker — the fixture as written is correct and now race-free.

No new issues found. Approving from a correctness standpoint.

[thomhurst]

Rebased cleanly onto current main; no code changes beyond the rebase. @codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 5351949684

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

Summary: Re-verified at head 5351949 (commit message: "fix(tests): keep HTTP fixture alive"). The old reviewed commit (95f47779fd) no longer exists in the remote — it was superseded by a rebase — but the resulting tree is byte-for-byte the same change: thomhurst.github.io and the GitHub release download in HttpTests/DownloaderTests are still replaced by the same in-process LocalHttpServer fixture, with _cancellationTokenSource still a plain new() cancelled only in DisposeAsync (so the previously-fixed accept-loop race is still fixed), the same checksum, and the same single-accept-per-test design. No functional regressions introduced by the rebase.

Architectural suggestion (repeating from earlier reviews — still unaddressed)

RichardSzalay.MockHttp is pinned in Directory.Packages.props and referenced by test/ModularPipelines.UnitTests/ModularPipelines.UnitTests.csproj, but nothing in the repo uses it. This PR adds a second, parallel way to fake HTTP instead: a hand-rolled TcpListener-based HTTP/1.1 server (Helpers/LocalHttpServer.cs) that binds a real loopback socket, manually reads request lines, and writes status/header bytes by hand.

There's already an injection seam that avoids all of that: Http.SendAsync (src/ModularPipelines/Http/Http.cs) uses HttpOptions.HttpClient directly when set, bypassing the named IHttpClientFactory client entirely, while still running the full SendAndWrapLogging pipeline — this PR's own Assert_SendAsync_Logs_As_Expected(customHttpClient: true) case already proves that path works. A MockHttpMessageHandler wired through that same seam (or via .ConfigurePrimaryHttpMessageHandler on the named client) would produce a real HttpResponseMessage with headers/content/status, so the existing assertions (Server: ..., Content-Length, etc.) work unchanged.

Why this is better than the current design:

• It removes an entire class of incidental complexity this fixture carries: manual socket accept loop, CancellationTokenSource lifecycle, and swallowing OperationCanceledException/SocketException on shutdown — none of that exists with MockHttpMessageHandler.
• It avoids maintaining two parallel "fake an HTTP response" mechanisms in the same test project going forward — the next engineer adding an HTTP test won't know whether to reach for LocalHttpServer or MockHttp.
• The dependency is already paid for (referenced, versioned, restored) and unused elsewhere, so adopting it here is a net reduction in surface area, not an added cost.

This is non-blocking — the fixture is correct and race-free as written — but since this has come up on every prior revision of this PR without being adopted, it's worth an explicit decision (adopt MockHttp and delete LocalHttpServer.cs, or intentionally keep the raw-socket fixture for real-pipeline fidelity) rather than letting it default to "carry both" once this merges.

No new issues found.

[thomhurst]

Rebased onto current main after #3567 merged. Patch equivalence preserved (c1ec84502d0b2f269060e8677aa5e73775e71b6e before and after). New head: a00ad54fe5.

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

Reviewed commit: a00ad54fe5

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

No issues found. Checked for bugs, security problems, and CLAUDE.md compliance across the diff (test/ModularPipelines.UnitTests/Helpers/LocalHttpServer.cs, HttpTests.cs, DownloaderTests.cs).

Verified against prior review history:

• The previously-flagged CancellationTokenSource race (10s timer starting at construction, risking cancellation before slow test/host startup) remains fixed — it's still a plain new CancellationTokenSource() cancelled only in DisposeAsync.
• The new MD5 fixture checksum (AEDF5D7C23744269F358814E602AFE89 for "local download fixture") is correct.
• Build is clean and all 7 affected tests pass (DownloaderTests, HttpTests).

Standing non-blocking suggestion (raised in earlier review passes, still applicable, not a blocker): RichardSzalay.MockHttp is already referenced in Directory.Packages.props/the test csproj but unused anywhere in the repo. Since Http.SendAsync honors HttpOptions.HttpClient directly while still running the logging pipeline (proven by this PR's own customHttpClient: true test case), a MockHttpMessageHandler could satisfy the same assertions without owning a manual TcpListener/socket-parsing/cancellation lifecycle in LocalHttpServer. That would remove an entire class of concerns (single-connection-only design, raw HTTP/1.1 parsing, listener/dispose ordering) in exchange for a well-tested, purpose-built abstraction. Worth considering for a follow-up if these tests are touched again, but not something this PR needs to change now.

Good fix overall — replaces genuinely flaky, network-dependent tests with a deterministic in-process fixture, and the known race from earlier revisions stayed fixed through the rebases.

[thomhurst]

Rebased onto current main after #3564 merged. Patch equivalence preserved (c1ec84502d0b2f269060e8677aa5e73775e71b6e before and after). New head: 8b67dfdb73.

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: 8b67dfdb73

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

Summary: Re-verified at head 8b67dfdb73. This replaces the live-network dependencies in HttpTests/DownloaderTests (thomhurst.github.io, a GitHub release download) with a deterministic in-process LocalHttpServer fixture, closing #3519.

Verified independently

• MD5 of "local download fixture" is AEDF5D7C23744269F358814E602AFE89 — matches DownloaderTests.cs.
• Single-accept-per-instance design is correct for actual usage: Http.SendAsync (src/ModularPipelines/Http/Http.cs) and Downloader.DownloadResponseAsync/DownloadFileAsync never retry or issue more than one request per call, and each test constructs its own LocalHttpServer — so one AcceptTcpClientAsync per instance is sufficient, not fragile.
• The previously-flagged construction-time-timeout race is still fixed: _cancellationTokenSource is a plain new(), cancelled only in DisposeAsync, and AcceptTcpClientAsync/stream.WriteAsync both honor that token, so shutdown paths are covered by the OperationCanceledException/SocketException catches.
• Dropping [Retry(3)] on Can_Download is correct now that the fixture is deterministic.

Standing suggestion, unaddressed across ~6 revisions of this PR — worth an explicit decision before merge

RichardSzalay.MockHttp is pinned in Directory.Packages.props and referenced by ModularPipelines.UnitTests.csproj, but nothing in the repo uses it. This PR adds a second, parallel "fake an HTTP response" mechanism instead: a hand-rolled TcpListener-based HTTP/1.1 server that manually reads request lines and writes status/header bytes.

Http.SendAsync already provides the seam to avoid this: when HttpOptions.HttpClient is set, it bypasses the named IHttpClientFactory client but still runs the full SendAndWrapLogging pipeline (request/response logging, status code, duration) — this PR's own Assert_SendAsync_Logs_As_Expected(customHttpClient: true) case proves that path works end-to-end. A MockHttpMessageHandler wired through that seam would produce a real HttpResponseMessage with headers/content/status, so the existing assertions (Server: ..., Content-Length, etc.) would work unchanged, without owning a socket accept-loop, manual CancellationTokenSource lifecycle, or raw HTTP/1.1 wire parsing.

This has been raised on every prior review pass on this PR and hasn't blocked any of them functionally — the fixture as written is correct and race-free. But since it keeps resurfacing without a decision, I'd treat this rebase as the point to either adopt MockHttp and delete LocalHttpServer.cs, or explicitly document why raw-socket fidelity is wanted here (e.g. exercising the real default HttpClient/IHttpClientFactory pipeline rather than only the HttpOptions.HttpClient override path) — otherwise the test project ends up permanently carrying two parallel HTTP-faking mechanisms for no documented reason.

No new issues found.