Surface Transfer Failures - #222
Conversation
Core reports the truth via per-request counts; the CLI applies policy. - NetPace.Core: aggregate per-request failures into RequestsAttempted/ Succeeded/Failed on SpeedTestResult instead of swallowing them; validate upload HTTP status (a rejected upload is a failed request, not throughput); never throw for network outcomes (only cancellation and operational faults); stream each failure reason live via SpeedTestProgress.FailedRequestReason; reverse the ISpeedTestService exception-propagation contract. - NetPace.Console: route human/operational output to a stderr console; make the exit code reflect only NetPace's health (network conditions exit 0); add --fail-on total|partial|none (fail-fast); surface counts in every format (token annotation, CSV columns, JSON fields); stream failure reasons at Debug. - Docs: README --help snapshot and a USER_GUIDE section on detecting failures. Public NetPace.Core API change ships as a MINOR bump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@claude Review this pull request. Analyse the code changes and provide feedback covering:
|
|
Claude finished @FrankRay78's task in 8m 17s —— View job Review: Surface Transfer Failures (#222)Todo list
Overall this is a well-scoped, thoroughly-tested implementation of the approved spec, and CI (Build and Test + CodeQL) is green. The core design — counts-not-exceptions in 1.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- CSV header no longer dropped when a leading --loop/--count iteration finds no server: key the header on the first actual data row, not the iteration index. - Scope the USER_GUIDE Debug live-streaming claim to normal/interactive output (machine formats never write failure reasons to stderr, including at Debug). - Rename the shadowing 'server' field in ScriptedSpeedTester (clears CodeQL). - Add tests: CSV header survives a leading no-server iteration; JSON+Debug writes nothing to stderr; --quiet still emits the all-failed stderr notice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@claude Please re-review, identify only showstoppers or important issues (P1/P2). |
|
Claude finished @FrankRay78's task in 1m 29s —— View job Re-review: Surface Transfer Failures (#222)Todo list
No P1/P2 (showstopper/important) issues found. Commit
The two remaining CodeQL notes ( Spot-re-checked the core failure-aggregation path ( Nothing blocking merge from this pass. |
Issue #206 is answered by the request counts, --fail-on and the exit code. The standard-error channel and the live per-request reason stream were implementation scope beyond that, and together they corrupted the progress display: writes to a second IAnsiConsole landed inside Spectre's live region, which repaints by moving the cursor, so each notice was partially overwritten and pushed the bars down. - Delete ErrorConsole and route notices back through the single console, as on main. The no-servers notice is now mode-gated like the failure notices so prose cannot corrupt CSV or JSON output. - Remove SpeedTestProgress.FailedRequestReason and its plumbing; SpeedTestProgress is again identical to main. A total failure at --profile Mega would have emitted 456 near-identical lines, because the byte-budget cap only advances on success and so never trips. - --quiet suppresses the notice along with the rest of the output (matching main); --fail-on is the detection path for quiet consumers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Discovery is not part of issue #206, which is answered by the request counts, --fail-on and the exit code. The branch had rewritten it anyway: ServerSelector swallowed every exception and returned null, collapsing DNS errors, TLS errors and HTTP statuses into one fixed sentence. main reports e.Message, so main was strictly more diagnosable on this path. - Restore ServerSelector verbatim from main (discovery throws again). - Restore WriteError and the per-iteration catch in SpeedTestCommand, and main's CSV header index; drop SpeedTestOutcome.ServersFound. - Restore main's no-servers and network-exception snapshots. One deliberate deviation: the per-iteration catch re-raises IOException and UnauthorizedAccessException instead of swallowing them, so a fault in NetPace itself still exits non-zero while network conditions exit 0. Covered by Operational_Fault_During_A_Run_Exits_One. Verified against origin/main by running both builds over the same no-servers scenarios (default, --csv, --json, Minimal, --quiet): output and exit codes are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The class holds extension methods on SpeedTestResult, the same shape as NetPace.Core's SpeedTestExtensions, but its name read like a service. Both are now named for what they are, so a reader meeting GetSpeedString and GetFailureAnnotation on the same line can see they are the same kind of thing - Core stays policy-free, the CLI applies policy. No behaviour change; call sites use extension syntax and are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| Timestamp,Latency,Download,DownloadSucceeded,DownloadFailed,Upload,UploadSucceeded,UploadFailed,IPAddress,Hostname | ||
| ``` | ||
| - **JSON** — each active dimension gains integer `…Succeeded` / `…Failed` fields; on a total | ||
| failure the speed field is omitted (there is no valid measurement) while the counts remain: |
There was a problem hiding this comment.
Lets consider if we should include the 0 bps measurement field, so the JSON schema is always consistent - also it would align to the Normal and CSV behaviour. Please investigate why it's been implemented like this, and if there was an underlying good reason.
| ``` | ||
| Timestamp,Latency,Download,DownloadSucceeded,DownloadFailed,Upload,UploadSucceeded,UploadFailed,IPAddress,Hostname | ||
| ``` | ||
| - **JSON** — each active dimension gains integer `…Succeeded` / `…Failed` fields; on a total |
There was a problem hiding this comment.
Is the use of the word "dimension" obvious to the user - maybe there is a better term? Please investigate
| operational failure (for example, being unable to write the `--file` output) exits non-zero. So a | ||
| `0 bps` measurement still exits `0` by default: inspect the counts (or use `--fail-on`) to detect it. | ||
|
|
||
| If you want a failed measurement to fail the process — for scripting or CI — opt in with |
There was a problem hiding this comment.
Make: "If you want a failed measurement to fail the process, opt in with"
| `1` at the first measurement that meets the threshold. | ||
|
|
||
| ```bash | ||
| # In CI: treat a totally-failed dimension as a build failure |
There was a problem hiding this comment.
Make: "# Treat a totally-failed dimension as a build failure"
| --file-mode Append Determines file output behavior. <Append, Overwrite> | ||
| -q, --quiet Suppress all normal console output (file output still works). | ||
| --fail-on None Exit with a non-zero code on a failed measurement. <None, Total, Partial> | ||
| None (default) never affects the exit code; Total triggers when a dimension is all-failed; Partial triggers on any failed request. Fail-fast across --count and --loop. |
There was a problem hiding this comment.
Make:
"None never affects the exit code; Total triggers when a dimension is all-failed; Partial triggers on any failed request."
| /// <see cref="RequestsSucceeded"/>; a value of zero with <see cref="RequestsAttempted"/> greater | ||
| /// than zero means every request failed and the reported speed is not a valid measurement. | ||
| /// </remarks> | ||
| public int RequestsAttempted { get; init; } |
There was a problem hiding this comment.
Why is this not computed from RequestsSucceeded/Failed - or left for the consumer to make this calc themself? I'm not sure I like this property. Please think and adivse.
| /// <param name="progress">A progress reporter that receives upload progress updates.</param> | ||
| /// <param name="cancellationToken">The token to allow the operation to be cancelled.</param> | ||
| /// <returns>The result including bytes processed and elapsed time in milliseconds.</returns> | ||
| /// <returns> |
There was a problem hiding this comment.
Revert this comment - I don't like including field level specifics here - the user should examine the return class for that. The existing comment was more robust.
| /// <param name="server">The server to measure upload speed from.</param> | ||
| /// <param name="cancellationToken">The token to allow the operation to be cancelled.</param> | ||
| /// <returns>The result including bytes processed and elapsed time in milliseconds.</returns> | ||
| /// <returns> |
There was a problem hiding this comment.
As per my comment on line R120.
| /// <param name="progress">A progress reporter that receives download progress updates.</param> | ||
| /// <param name="cancellationToken">The token to allow the operation to be cancelled.</param> | ||
| /// <returns>The result including bytes processed and elapsed time in milliseconds.</returns> | ||
| /// <returns> |
There was a problem hiding this comment.
As per my comment on line R120.
| /// <param name="server">The server to measure download speed from.</param> | ||
| /// <param name="cancellationToken">The token to allow the operation to be cancelled.</param> | ||
| /// <returns>The result including bytes processed and elapsed time in milliseconds.</returns> | ||
| /// <returns> |
There was a problem hiding this comment.
As per my comment on line R120.
| /// when every request fails. Callers detect an unusable measurement by inspecting the counts | ||
| /// (for example, <see cref="SpeedTestResult.RequestsSucceeded"/> is zero). Exceptions are reserved | ||
| /// for caller-requested cancellation and for genuine operational failures; they do not signal | ||
| /// network conditions. |
There was a problem hiding this comment.
I hate this updated comment - you have put all sorts of implementation details on an interface. Favour the existing comment - but consider any material improvements, if you think there are some. Otherwise this is not the place to note all this. Let the code do the talking.
| namespace NetPace.Core.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Per-request failure aggregation for download and upload tests (issue #206): failed requests are |
There was a problem hiding this comment.
I'm not a fan of including issue numbers in the code - please remove.
|
|
||
| var failOnOption = new Option<FailOn>("--fail-on") | ||
| { | ||
| Description = "Exit with a non-zero code on a failed measurement. <None, Total, Partial>\nNone (default) never affects the exit code; Total triggers when a dimension is all-failed; Partial triggers on any failed request. Fail-fast across --count and --loop.", |
There was a problem hiding this comment.
Align this with any changes made in the README.md changes I suggest below - make 100% sure they are aligned if either is updated
|
|
||
|
|
||
| // Display speed test result. | ||
| // Display speed test result. The token carries the count annotation when requests failed. |
There was a problem hiding this comment.
Revert this line please
|
|
||
|
|
||
| // Display speed test result. | ||
| // Display speed test result. The token carries the count annotation when requests failed. |
There was a problem hiding this comment.
Please revert this line.
|
|
||
|
|
||
| if ((settings.Verbosity & Verbosity.Debug) != 0) | ||
| if (debug) |
There was a problem hiding this comment.
Please explain why this change.
There was a problem hiding this comment.
Revert the annotated "Display speed test result." comments in the default and minimal writers, and drop the single-use `debug` local left behind by the reverted stderr work - one call site does not earn a named alias. Restore the original ISpeedTestService remarks and amend only the clause the behaviour change made false, rather than replacing the contract note with implementation detail. Filter the --fail-on sequence explicitly with OfType instead of a continue-guard, clearing the outstanding CodeQL finding, and drop the ScriptedSpeedTester doc line describing per-request failure reasons on the progress channel - that feature was reverted in fb0e632. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The writer-selection switch and ShouldEmitFailureNotice encoded the same mapping over settings in two places, so a fifth writer meant editing both and they could drift. Replace the settings sniffing with one property, AcceptsProseNotices, answered by the writer that owns the output shape. The predicate was never "is this machine readable" - MinimalConsoleWriter is human prose and still excludes the notice. It is whether an extra line of free text breaks what the writer promised: CSV promises delimited records, JSON parseable documents, Minimal exactly one line per run. Behaviour is unchanged; the existing failure tests cover both sides. Make the console-output snapshot convention constitutional (1.6.0 -> 1.7.0) and reconcile it with the "do not test Spectre.Console" rule, which read as a ban on the 125 snapshots the repo already relies on. CLAUDE.md reviewed and updated in lockstep, per Governance point 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restores ShouldEmitFailureNotice and the settings-based check in SpeedTestCommand, and removes AcceptsProseNotices from IConsoleWriter and its four implementations. The constitution and CLAUDE.md snapshot-testing changes from 080b28d are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| { | ||
| if (result is not null && result.IsAllFailed()) | ||
| { | ||
| console.WriteLine($"{testName} failed: all {result.RequestsFailed} requests to {serverUrl} failed."); |
There was a problem hiding this comment.
Why should this not be moved out of SpeetTestCommand and put into DefaultConsoleWriter.PerformSpeedTestAsync ? The direct call to console here looks really really dirty.
The notice was emitted from SpeedTestCommand, which had to sniff the
output format to decide whether prose was safe - a condition that was
already an exact restatement of the writer-selection switch a few lines
above. DefaultConsoleWriter is the only writer that emits it, so the
notice moves there and ShouldEmitFailureNotice disappears rather than
being abstracted. The command no longer writes to the console directly,
and no longer depends on the composite console assigned into the primary
constructor parameter mid-method.
SpeedTestOutcome.ServerUrl existed only to carry the URL back for that
notice; the writer has it in hand, so the property and its four
assignments are removed. ProcessOutcome collapses into FailOnTriggered.
Output is unchanged - the notice still trails the help footer, which is
worth revisiting separately.
Snapshot the three shape-asserting failure tests, per the new Testing
Standards. The CSV one replaces a Split('\n')[1] plus Contains(",0,32,")
with the row itself.
Make JsonResult.Latency/DownloadSpeed/UploadSpeed `required string?`:
all three were assigned null with `!` when their test was skipped, so
the declaration now matches reality. No output change.
Split the snapshot guidance: the constitution carries the rule and the
rationale, CLAUDE.md the mechanics. CLAUDE.md includes the constitution,
so restating it there was guaranteed-redundant context.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pins the current output ahead of moving the notice, so the reorder shows up as a reviewable diff rather than an invisible change. The snapshot records the notice trailing the help footer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The notice was emitted after the "Try 'NetPace --help'" footer, leaving it stranded below the sign-off and detached from the measurement it explains. It now follows the result line directly, and the footer closes the output as it does everywhere else. The snapshot committed in the previous commit shows the move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The record spelled "may be absent" two ways: three conditional properties were required string?, while their four count siblings were plain int?. Now one rule holds throughout - required means the property always appears, nullable means it is omitted when its test did not run - and the class documents it. Nothing is lost in practice: JsonResult has a single construction site, and a property left unset fails the JSON snapshots immediately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The notice was specified for stderr, where a human-facing line sits on its own channel and repeats nothing. That channel was reverted in fb0e632; the notice stayed on stdout, where every fact in it is already on screen - the URL four lines above in the server header, the count in the token annotation immediately before it. The counts were the point of this work. The annotation already spends them; the notice spent them a second time, in longer form, on the same stream. Six of the sixteen failure tests were organised around its presence or absence and are now simpler assertions about the annotation. The USER_GUIDE paragraph and two stale doc comments go with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@claude Review this pull request. Analyse the code changes and provide feedback covering:
|
|
Claude finished @FrankRay78's task in 4m 46s —— View job Re-review: Surface Transfer Failures (#222)Todo list
The branch has moved substantially since the last review pass (25 commits, several rounds of "address PR review feedback"). Almost everything raised in the two earlier reviews and Frank's inline thread comments is now resolved in the current HEAD ( Bugs / correctness1. Misplaced XML doc comment in /// <summary>
/// Executes the speed test command using the provided settings.
/// </summary>
/// <remarks>
/// Network and discovery outcomes are data, not errors: ...
/// </remarks>
/// <summary>
/// Writes an error message to the console.
/// </summary>
private static void WriteError(IAnsiConsole console, string message)Net effect: No other correctness issues found. Spot-checked:
Security / static analysisAll prior CodeQL findings are resolved:
CLAUDE.md / constitution adherence
Test coverageCoverage is thorough and now includes the gaps flagged in the first review: Spec/test-plan mismatchesThe one explicitly-flagged divergence from the PR description — JSON omitting the speed field on total failure instead of the spec's literal Nothing here blocks merge — item 1 is a doc-comment hygiene fix, not a behavioural bug. |
|
@claude Please review the last commit, which addresses previous review findings. I'm looking for any remaining showstoppers or important items in the codebase, that would stop merging. |
|
Claude finished @FrankRay78's task in 2m 41s —— View job Re-review: Surface Transfer Failures (#222)Todo list
No P1/P2 (showstopper/important) issues found. Commit What the last commit fixed
Checked and confirmed non-issues
Nothing here blocks merge. |
2 participants
Closes #206.
Why
Certain upload endpoints (e.g. the Deutsche Telekom servers in #206) reported 0 bps while working on speedtest.net. The cause: the parallel test loop caught and discarded every per-request exception, counting failed requests as 0 bytes transferred. When every request failed,
0 bytes ÷ elapsed = 0 bpswas reported as if it were a valid measurement — with no log, no message, and no non-zero exit code. This makes that failure visible.Implements the approved specification in #206 (comment). Governing principle: Core reports the truth via request counts; the CLI applies policy. The exit code reflects only whether NetPace itself functioned — network conditions are data, not errors.
What changes
NetPace.Core (public API — MINOR bump)
SpeedTestResultgainsRequestsAttempted/RequestsSucceeded/RequestsFailed. Per-request failures (transport errors, timeouts, non-success HTTP status) are aggregated into these counts instead of being swallowed; the method no longer throws for network outcomes (only caller cancellation and operational faults propagate).SpeedTestProgressgainsFailedRequestReasonto stream each failure live; nothing is retained on the result.ISpeedTestServicedoc reversed to describe the counts-not-exceptions contract.NetPace.Console
0; only operational failures (e.g. can't write--file) exit non-zero.--fail-on <total|partial|none>(defaultnone) to opt in to a failure exit code; fail-fast and uniform across single /--count/--loop.Upload: 0 bps (32 of 32 requests failed)), CSV columns (DownloadSucceeded/DownloadFailed/…), and JSON integer fields. Human notices and operational errors now go to stderr; at--verbosity Debugeach failure reason is streamed live.Docs: README
--helpsnapshot and a new USER_GUIDE "Detecting failed measurements" section (counts, exit-code model,--fail-on).Non-obvious things a reviewer should know
0 bpsun-ignorable: it always co-travels with the counts, and the CLI is obligated to surface them. Known accepted trade-off:netpace && nextproceeds on a total outage by default; use--fail-onor inspect the counts to detect it.null"; I omit the speed field instead (System.Text.JsonWhenWritingNull, consistent with how--no-downloadalready drops fields), letting"UploadSucceeded": 0carry validity. Per-instance explicitnullisn't achievable without also emittingnullfor skipped dimensions. Happy to switch to explicitnullif preferred.Directory.Build.propsis intentionally untouched — tagv1.1.0at release.How to verify
dotnet build srcanddotnet test srcare clean (633 tests, 0 warnings).netpace --jsonagainst a server whose uploads all fail → JSON has"UploadSucceeded":0/"UploadFailed":N, noUploadSpeed, nothing on stderr, exit0.--csv→ the data row shows…,0,N,…; with default output →Upload: 0 bps (N of N requests failed)plus a stderr notice.--fail-on totalon an all-failed dimension → exit1; default → exit0.--server <unreachable>(latency on) → exit0with the no-servers notice (regression: previously exit 1 only in this path).