fix(core): give FailureRate a sample, and stop calling a cancelled upload a destination error (ADR D78) - #275
Merged
Conversation
…load a destination error (ADR D78) Two §5 items filed as "semantics choices". Reading the code closely made the first look less like a choice than a threshold that never worked. ReportRunner increments batches AND totalFailures before computing totalFailures/batches, so a failure in the very first batch always yields exactly 1.0 — tripping every FailureRate below 1, whatever it was configured to. The second batch failing gives 0.5. Three batches is not a rate. FailureRate now evaluates only after FailureRateMinimumBatches (default 10) batches have been seen; aborting earlier is what ConsecutiveFailures and TotalFailures are for, and both are untouched. FailureRateMinimumBatches is an init-only PROPERTY, not a fourth positional parameter on AbortThresholdConfig: that record is in the frozen Abstractions ABI (rule 7) and adding a parameter would change its primary constructor signature, while a new property is additive. ThresholdContext gained the batch count the same way, sourced from BatchFailureContext.PageNumber, which the runner increments in lockstep with batches. Separately, both destinations' catch (Exception) swallowed OperationCanceledException into UploadResult.Fail, so a deadline firing mid-upload was attributed to S3 or the filesystem with the real reason replaced by a provider-shaped message. Cancellation now rethrows, filtered on the caller's own token exactly as ReportJobWorker has done since #240 — an OCE carrying someone else's token (an SDK timeout) stays a genuine transport failure, which is also what leaves the multi-destination loop free to keep collecting per-destination results. One existing test encoded the old FailureRate behaviour on a three-batch fixture; it now sets the minimum to 3 explicitly so it keeps measuring the ratio arithmetic it was written for rather than the guard in front of it. That is the third test this sweep found asserting a defect as if it were the spec. Five tests. The S3 ones fail against the unfixed code; the Core ones cannot even compile without the new property, which is its own proof they are not vacuous. Core 306, S3 8, Local 14, Jobs 44.
I wrote the cancellation tests for S3 and forgot the identical branch in LocalDestination — Sonar's new-code coverage caught it at 72.7%. That is the same lapse I named two PRs ago: when a guard is added for an edge case, its test belongs in the same pass, not after a gate points at it. Two tests, mirroring the S3 pair: a cancelled write rethrows rather than becoming a destination error, and an OperationCanceledException carrying someone else's token stays a transport failure. The first also asserts no file survives. The target DIRECTORY is created up front and legitimately remains — my first assertion demanded it be gone and failed for the wrong reason, which is worth noting because an over-specified assertion looks exactly like a real defect until you read it. Local 16/16, verified to fail with the cancellation catch removed.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two §5 items filed as "semantics choices". Reading the code closely made the first look less like a choice than a threshold that never worked.
FailureRatewas effectively "abort on the first failure"ReportRunnerincrementsbatchesandtotalFailuresbefore computingtotalFailures / (double)batches. So a failure in the very first batch always yields exactly 1.0 — which trips everyFailureRatebelow 1, whatever it was configured to. The second batch failing gives 0.5. Three batches is not a rate.FailureRatenow evaluates only onceFailureRateMinimumBatches(default 10) batches have been seen. Aborting earlier is whatConsecutiveFailuresandTotalFailuresare for, and both are untouched — a run that should stop immediately still does, through the threshold that actually means that.ABI care (rule 7)
FailureRateMinimumBatchesis an init-only property, not a fourth positional parameter onAbortThresholdConfig. That record is in the frozenAbstractionsABI, and adding a parameter changes its primary constructor signature — a binary break. A new property is additive. Same shape as D75's enum append: the readable placement is the unsafe one.ThresholdContextgained the batch count the same way, sourced fromBatchFailureContext.PageNumber, which the runner increments in lockstep withbatches— so no new field had to cross into Abstractions at all.A cancelled upload was blamed on the destination
Both destinations'
catch (Exception)swallowedOperationCanceledExceptionintoUploadResult.Fail, so a deadline firing mid-upload was attributed to S3 or the filesystem, with the real reason replaced by a provider-shaped message. The run endedFailedeither way — this is attribution accuracy, but attribution is the first thing an operator reads.Cancellation now rethrows, filtered on the caller's own token, exactly as
ReportJobWorkerhas done since #240. AnOperationCanceledExceptioncarrying someone else's token — an SDK's internal timeout, most commonly — stays a genuine transport failure and is still reported as one. Filtering rather than rethrowing every OCE is also what leaves the runner's multi-destination loop free to carry on collecting per-destination results.A note on the existing test suite
FailureRate_threshold_aborts_when_the_ratio_is_reachedencoded the old behaviour on a three-batch fixture. It now sets the minimum to 3 explicitly, so it keeps measuring the ratio arithmetic it was written for rather than the guard in front of it, which has its own tests.That is the third test this sweep has found asserting a defect as if it were the spec — after the two pagination ones in #264. Worth saying out loud: a green suite is evidence the code does what the tests say, not that the tests say the right thing.
Tests
Five. The S3 pair fails against the unfixed code. The Core ones cannot compile without the new property — which is its own proof they are not vacuous, and the reason the usual stash-and-revert check reports a build error rather than a red test there.
Core 306, S3 8, Local 14, Jobs 44 — 0 failures.
Last of the batch: the multi-output skip guard (D11) and opt-in row-count reconciliation.