fix(jobs): report a timed-out source as Failed, keep a deadline as Cancelled - #240
Merged
Conversation
…ncelled
The worker's `catch (OperationCanceledException)` was unfiltered, so it also
swallowed an OCE raised by something other than the run's own token — most
commonly HttpClient.Timeout, whose TaskCanceledException carries an internal
token and which the runner deliberately lets past its batch handlers (they
filter on `ex is not OperationCanceledException`). A genuine failure was
recorded as "Cancelled." with its error detail discarded and, because that path
does not rethrow, Hangfire recorded the background job as succeeded.
Filtering on `cancellationToken.IsCancellationRequested` alone would have broken
the documented Deadline contract: a deadline cancels through a LINKED token, so
the caller's token is not cancelled for it either, and the job would have been
downgraded to Failed — which rethrows, handing it to Hangfire's automatic retry
to re-run the whole report up to ten more times. The runner therefore now reports
a deadline as ReportDeadlineExceededException (an OperationCanceledException
subclass, so every existing catch site is unaffected) and the worker treats that,
plus its own token, as cancellation — recording a reason that says which.
Also fixes the same fixed-temp-name collision in both file stores: concurrent
saves of one report's schedule override (or config) shared `{name}.json.tmp` and
failed with a sharing violation or a FileNotFound. Both now go through the shared
AtomicFileWrite, whose temp name is unique per write and is cleaned up on failure.
Both regressions are covered by tests that were each verified to FAIL without
their fix (timeout → Cancelled; deadline → Failed).
The Sonar gate flagged new_coverage (60.7%): AtomicFileWrite's failure branch — the one that deletes the unique temp file so a failed save leaves no orphan — had no test. Exercise it through the public store API: a directory planted where the store wants its file makes the final move fail after the temp is written. Also assert a successful save leaves no temp behind.
S6667 (log in a catch should pass the caught exception) fired on the two cancellation logs — the deadline warning now has the exception in scope, so pass it; it also records where the run was when it was cut off. CA1861 (constant array argument) on the new store assertion. The remaining Sonar issues on ReportRunner.cs (S107/S125/IDE0008) sit on pre-existing lines this PR never touched and are left alone.
|
|
thiagoluga
added a commit
that referenced
this pull request
Aug 5, 2026
…load a destination error (ADR D78) (#275) * fix(core): give FailureRate a sample, and stop calling a cancelled upload 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. * test(local): cover the destination's cancellation path 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.
What
ReportJobWorker'scatch (OperationCanceledException)was unfiltered, so it also swallowed an OCE raised by something other than the run's own token — most commonlyHttpClient.Timeout, whoseTaskCanceledExceptioncarries an internal token, and which the runner deliberately lets past its batch handlers (they filter onex is not OperationCanceledException).Consequences for any of the 7 shipped HTTP-family sources:
Cancelledwith the message"Cancelled."— indistinguishable from an operator-initiated cancel, real error detail discarded;The subtlety this PR had to solve
Filtering on
cancellationToken.IsCancellationRequestedalone — the obvious fix — would have broken the documentedDeadlinecontract. A deadline cancels through a linked token (ReportRunner.RunAsync), so the caller's own token isn't cancelled for it either. A deadline would have been downgraded toFailed, which rethrows, handing it to Hangfire's default 10-attemptAutomaticRetry— re-running the whole report up to ten more times, each burning a full deadline. Three shipped doc comments state a deadline "surfaces as a cancelled run".Widening the filter to
ex.CancellationToken.IsCancellationRequesteddoes not work either:HttpClientbuilds its timeout exception with its own already-cancelled token, which would readmit the original bug.So the runner now reports a deadline as
ReportDeadlineExceededException— anOperationCanceledExceptionsubclass, so every existing catch site andDeadlineTestsare unaffected — and the worker treats that plus its own token as cancellation, recording a reason that says which (a bare"Cancelled."gave an operator no way to tell a cancel from a run that ran out of time).Also fixed
Both file stores (
FileScheduleOverrideStore,FileReportConfigStore) staged every write through a temp path derived only from the name, so two concurrent saves of the same document shared{name}.json.tmp→ sharing violation, or aFileNotFoundwhen the second move found the first had taken it. Both now use a sharedAtomicFileWritewith a unique temp name, deleted if the save fails. (ListAsyncenumerates*.json, so.tmpfiles are never picked up.)Verification
Jobs.UnitTests34 passed,Core.UnitTests296 passed, full solution builds clean.A_source_timeout_is_reported_as_failed_not_cancelledyieldsCancelledwithout the filter;An_expired_deadline_is_still_recorded_as_cancelledyieldsFailedwithout the discriminator. (The deadline path had no worker-level test at all before — which is exactly why the naive fix would have shipped silently.)Found by a bug hunt over the job/scheduling layer; the remaining findings (Hangfire's default 10× retry vs. the atomic-job model, the in-memory recurring-registration race, and the S3 key template not guarding caller-controlled parameters — the highest open security item) are recorded in
docs/STATUS-AND-BACKLOG.md§5 as they need a decision.