Upload Redirect 307

[FrankRay78]

Why

Ookla is migrating its speed test fleet to HTTPS. A migrated server answers a plain-HTTP upload POST with a 307 to its HTTPS endpoint — and it sends that redirect after roughly 64 KB, closing the request stream while the body is still being written.

Since 0.14.0 (#69) the upload body has been streamed in 8 KB chunks instead of handed over as one buffered ByteArrayContent. A streaming body cannot survive that mid-write teardown: it fails with IOException: Unable to write data to the transport connection, so .NET never reads the 307 and never follows it. Every upload request failed and the run reported 0 bps.

Roughly ten months of releases (0.14.0 → 0.24.0) have reported 0 bps against any migrated server — silently, until #222 started surfacing the failure count.

Downloads were never affected, which is what made this look server-specific rather than transport-shaped: a GET carries no body, so it replays across the redirect for free. The same server would report 291 Mbps down and 0 bps up.

What changes

• The upload endpoint is resolved once, before the measured uploads begin, and uploads go straight to wherever the server actually wants them. Redirects are followed explicitly (up to 5 hops) or honoured by the handler, whichever happens first.
• Upload throughput is now reported correctly against HTTPS-migrated servers.

Non-obvious things a reviewer should know

• The probe deliberately sends no body. A redirect is decided by scheme and host, not payload, so an empty POST draws the same answer with nothing to write and nothing to tear down mid-write. It also keeps the probe out of the measurement — an earlier 1 KB probe was caught by GetUploadSpeedAsync_ShouldReturnSpeedTestResult_WhenSuccessful, which tallies every byte the server receives against reported throughput. That test passes unmodified here, and is worth keeping that way.
• Fix: Ram requirements continue to increase significantly with each loop #69's memory fix is retained. Per-chunk streaming is untouched; this only changes where the chunks are sent. Buffering the body again would have fixed the bug but re-opened the RAM growth Fix: Ram requirements continue to increase significantly with each loop #69 closed.
• Probing is best-effort. If it cannot complete, the configured URL is used and any real fault surfaces through the upload requests as before — a broken probe never fails a test run on its own.
• The regression test pins the outcome, not the mechanism (Constitution IX): a redirecting upload endpoint must still yield throughput. It holds for any of the redirect-handling approaches we considered.
• The hermetic seam has a known limit. MockHttpMessageHandler replaces the transport, so no test can reproduce the actual socket teardown mid-body-write. The test covers the redirect contract; the teardown itself was verified manually against a live migrated server.

How to verify

• dotnet build src — clean, zero warnings
• dotnet test src — 647 pass, 0 skipped
• New test fails without the fix: stash the OoklaSpeedtest.cs change and run dotnet test src --filter "FullyQualifiedName~WhenServerRedirectsUploadEndpoint"
• Against a live migrated server, upload reports real throughput rather than 0 bps:
  netpace --no-download --no-latency --server "http://speedtest-bracknell.vodafone.co.uk:8080/speedtest/upload.php"
  (measured 176.44 Mbps here, against 0 bps / 30 of 30 failed before)
• Against a non-migrated server (e.g. Aquiss), upload is unchanged

Related

Regression introduced by #69. Failure counts that made it visible came from #222.

Three follow-ups found while diagnosing this, deliberately left out of this branch:

1. Per-request failure reasons are discarded — only the count survives, which is why this took four releases and a dozen screenshots to pin down. Surfacing them means adding failure detail to SpeedTestResult, a public NetPace.Core API change.
2. Downloads still pay a redirect per request against migrated servers. Not broken, but silent overhead on every GET.
3. --no-latency selects servers.First() unvalidated. It is what made this bug reproducible, and the server it lands on shifts between releases.

[FrankRay78]

@claude Review this pull request. Analyse the code changes and provide feedback covering:

• Bugs or correctness issues
• Security concerns (including any flagged by static analysis)
• Adherence to the project conventions in CLAUDE.md
• Test coverage — do the tests adequately cover the new behaviour?
• Any spec/test-plan mismatches

[claude]

Claude finished @FrankRay78's task in 5m 7s —— View job

---

Review of PR #245 — Upload Redirect 307

• Gather context (PR body, diff, changed files)
• Read OoklaSpeedtest.cs changes in detail
• Read new test file OoklaSpeedtestTests.Redirects.cs
• Check for bugs/correctness issues
• Check security concerns
• Check CLAUDE.md / constitution adherence
• Check test coverage adequacy
• Check spec/test-plan mismatches
• Post final review

Note: dotnet build/dotnet test are not in my allowed tools in this sandbox (the command is blocked pending approval I can't grant non-interactively), so this review is code-reading only — the diagnosis below wasn't verified by actually running the suite. If you want me to run the checklist in the PR description, --allowedTools needs Bash(dotnet build:*)/Bash(dotnet test:*).

Correctness

The core fix is sound. HttpClientHandler.AllowAutoRedirect (default true) does try to follow 307s automatically, but because the streaming upload body throws IOException while still writing when the server tears down the connection mid-body, the client never lives long enough to see the redirect. Resolving the endpoint first with a zero-byte probe, then uploading straight to that resolved URL, correctly sidesteps this. ResolveUploadUrlAsync (src/NetPace.Core/Clients/Ookla/OoklaSpeedtest.cs:539-571) handles both cases sensibly: if the transport already followed the redirect (real HttpClientHandler), response.RequestMessage.RequestUri gives the final URL; if not (e.g. MockHttpMessageHandler in tests, which has no auto-redirect), the explicit Location-header loop takes over. Good defensive design.

One correctness nit: IsRedirect (line 576) also matches 301/302/303, but per redirect semantics HttpClientHandler auto-rewrites those to a GET on redirect — irrelevant here since this is a resolve-the-URL probe, not a real transfer, but worth a one-line comment so a future reader doesn't wonder why POST-vs-GET semantics aren't handled.

Style / convention (CLAUDE.md, docs/conventions/csharp-style.md)

• ResolveUploadUrlAsync is an instance method (uses httpClient) but is placed inside #region Static Functions (OoklaSpeedtest.cs:444-581), sandwiched between GenerateDownloadUrls and GenerateUploadDataLengths, which are genuinely static. This breaks the file's own static/instance separation that the region name implies. Consider moving it (and keeping IsRedirect, which is static) out of that region, e.g. adjacent to GenericTestSpeedAsync.
• The style guide states "catch specific exception types, not System.Exception" (docs/conventions/csharp-style.md:68). The new catch (Exception) when (!cancellationToken.IsCancellationRequested) (line 564) is a broad catch, though it mirrors an existing, already-justified pattern in this same file (GenericTestSpeedAsync's catch, and ReportProgress's bare catch). Given the doc comment explaining the best-effort intent, this reads as a deliberate, consistent exception to the rule rather than an oversight — flagging for awareness, not blocking.

Test coverage

The new regression test (OoklaSpeedtestTests.Redirects.cs:23-64) correctly pins the described bug: a 307 to a migrated HTTPS endpoint now yields real throughput via MockHttpMessageHandler, which (unlike a real HttpClientHandler) doesn't auto-follow redirects — so this test actually exercises the new Location-header-following branch, not just the "transport already redirected" branch. Good choice of seam.

I verified the PR's claim that the pre-existing GetUploadSpeedAsync_ShouldReturnSpeedTestResult_WhenSuccessful test (OoklaSpeedtestTests.cs:1189) still passes unmodified: it tallies bytes from every matched request including the probe, but since the probe body is 0 bytes (RandomStreamContent(0)), it doesn't skew the tally. This checks out.

Two behaviors called out explicitly in the PR description as important design decisions have no test coverage:

1. Best-effort fallback — "if it cannot complete, the configured URL is used and any real fault surfaces through the upload requests as before." Nothing exercises the catch branch at OoklaSpeedtest.cs:564-568 (e.g., probe throws HttpRequestException) to confirm the upload test still proceeds using the original URL rather than failing outright.
2. Redirect-hop cap — maximumHops = 5 (line 541) has no test for a redirect loop (or a chain that never resolves) to confirm the method returns the last-seen URL rather than hanging or throwing.

Given these are both explicitly documented as intentional contracts in the PR body, I'd suggest adding at least the fallback-on-probe-failure case — it's the one most likely to regress silently and matches this bug's own history (a silent 0 bps failure that went unnoticed for ~10 months).

Security

No concerns — no new external input parsing beyond Uri construction from a Location header, which throws (caught by the best-effort fallback) rather than misbehaving on malformed input. No secrets, no injection surface.

Spec/test-plan

No specs/ entry for this fix (consistent with this being a direct bug-fix branch rather than a spec-kit feature flow) — no mismatch to flag. Constitution Principle IX (behavioural specification) is respected: the test asserts throughput/failure counts, not the redirect mechanism.

Summary

Solid, well-reasoned fix with a clear regression test for the primary bug. Two suggestions before merge: relocate ResolveUploadUrlAsync out of the Static Functions region, and consider a test for the best-effort-fallback-on-probe-failure path since it's a explicitly-documented safety property with zero coverage today.

Fix: move ResolveUploadUrlAsync out of #region Static Functions, and add a test for probe-failure fallback →

[FrankRay78]

@claude Review the newly pushed commit d0a2b30, which addresses your earlier review. Please check:

• ResolveUploadUrlAsync relocated out of #region Static Functions
• New test GetUploadSpeedAsync_ShouldMeasureThroughputAtTheConfiguredUrl_WhenResolvingTheEndpointFails covering the best-effort fallback — is the seam right, and does it actually pin the contract?
• The new IsRedirect remarks explaining why 301/302/303 are treated alike

Two of your suggestions were deliberately not actioned, and I'd welcome a challenge if you disagree:

• No test for maximumHops — it is a counted for loop, so a test asserting it terminates would be testing the language rather than NetPace.
• Broad catch (Exception) left as-is — a best-effort probe must not fail the run for any reason, matching the existing justified pattern in GenericTestSpeedAsync.

Note: the full suite was run locally — 648 pass, 0 fail, 0 skipped, 0 warnings. The fallback test was also confirmed to fail when the fallback is removed.

[claude]

Claude finished @FrankRay78's task in 1m 20s —— View job

---

Review of commit d0a2b30

• Gather diff for d0a2b30 vs baa5e0a
• Verify ResolveUploadUrlAsync relocation out of #region Static Functions
• Review new fallback test — seam and contract
• Review new IsRedirect remarks (301/302/303 rationale)
• Respond to the two deliberately-not-actioned suggestions
• Post final review

Note: dotnet build/dotnet test are still blocked pending approval in this sandbox, so this is code-reading only again — I did not independently re-verify the "648 pass, 0 fail" claim.

ResolveUploadUrlAsync relocation

Confirmed. It now sits between GenericTestSpeedAsync and #region Static Functions (OoklaSpeedtest.cs:456-488) — correctly grouped with the other instance methods, and #region Static Functions (:490 onward) now starts clean with ReportProgress. IsRedirect stayed in the static region, which is right since it's still a pure static helper. Good fix.

New fallback test — seam and contract

GetUploadSpeedAsync_ShouldMeasureThroughputAtTheConfiguredUrl_WhenResolvingTheEndpointFails (OoklaSpeedtestTests.Redirects.cs:67-112) does pin the contract, and the seam is right:

• ResolveUploadUrlAsync is called at OoklaSpeedtest.cs:277, before any parallel upload task starts (GenericTestSpeedAsync is only invoked at :295) — so the probe is deterministically the first HTTP call the test makes. The Interlocked.Increment(ref requestCount) == 1 check reliably targets the probe with no race against the later parallel uploads, which only fire once the probe has already been awaited.
• The mock only registers uploadUrl — nothing is registered for any other address. If the fallback path didn't actually return the original URL (e.g. it returned null, an empty string, or swallowed the failure but still mutated currentUrl), the subsequent measured uploads would hit an unmocked route, MockHttpMessageHandler would fault them, and they'd surface as requestFailed = true in GenericTestSpeedAsync (OoklaSpeedtest.cs:341-358) — so RequestsFailed.ShouldBe(0) genuinely exercises the "falls back to the configured URL" contract, not just "doesn't crash."
• The thrown HttpRequestException inside the Respond callback is caught by the broad catch (Exception) when (!cancellationToken.IsCancellationRequested) in ResolveUploadUrlAsync regardless of whatever concrete exception type MockHttpMessageHandler propagates it as — the seam doesn't depend on exception-type fidelity between the mock and a real transport failure.

This is a solid regression test for the exact property called out in the PR body ("if it cannot complete, the configured URL is used").

IsRedirect remarks (301/302/303)

The new remarks at OoklaSpeedtest.cs:576-581 directly address my earlier nit — explaining that method-rewriting semantics (303/302 forcing GET on replay) don't apply here because the probe is discarded once its address is known, and that 301 is just as plausible an HTTPS-migration signal as 307. That's a complete and correct answer; no further concern here.

The two deliberately-unactioned suggestions

• maximumHops test — agreed, don't add a test whose only job is proving a bounded for loop terminates. If there's a version of this worth having later, it isn't "does it terminate" but "does exhausting the hop cap return currentUrl (the last-seen redirect target) rather than the original url" (OoklaSpeedtest.cs:487) — a NetPace choice, not a language guarantee. That's a real but low-value edge case (a 5-hop redirect chain on a speedtest upload endpoint is exceptionally unlikely), so I agree it's fine to leave uncovered rather than add a test for its own sake.
• Broad catch (Exception) — agreed. A best-effort probe that must never fail the run needs to catch everything, and this now matches the existing, already-justified pattern in ReportProgress (OoklaSpeedtest.cs:497-509) and GenericTestSpeedAsync. No objection.

Summary

Both requested fixes are correctly applied, and the new test's seam genuinely exercises the fallback contract rather than getting a false pass. No further blocking feedback — the two intentionally-skipped suggestions are reasonable calls I agree with. Nothing else stood out in the diff.
· branch fix/upload-redirect-307