fix(jobs): report a timed-out source as Failed, keep a deadline as Cancelled (#240)

* fix(jobs): report a timed-out source as Failed, keep a deadline as Cancelled

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).

* test(core): cover the atomic-write cleanup path

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.

* style(jobs): address the analyzer findings this PR introduced

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.

Author: thiagoluga

CHANGELOG.md

@@ -36,6 +36,11 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   dropped column auto-fit. (`AdjustToContents` can't stream.)
 - **Opt-in `AddNeoReportsStartupValidation()`** compiles config-driven reports at host startup so a
   malformed document fails fast at boot rather than on the first request.
+- **`ReportDeadlineExceededException`** (in `NeoReports.Core.Pipeline`), thrown when a run exceeds its
+  configured `Deadline`. It derives from `OperationCanceledException`, so every existing catch site is
+  unaffected and a deadline still surfaces as a cancelled run — the distinct type exists because the
+  caller's own token is *not* cancelled on a deadline, which left it indistinguishable from an
+  unrelated `OperationCanceledException` (an `HttpClient.Timeout`, say — a genuine failure).
 - **Parameterless `ReportBuilder<T>.Retry()`** enabling a sensible production default (3 attempts,
   exponential backoff from 1s, jitter). Retries remain **off by default** — this lowers the barrier
   to turning them on and the docs now flag the recommendation for production network sources.
@@ -46,6 +51,16 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   the host — D20 — this is a nudge, not a behaviour change).
 
 ### Fixed
+- A job whose source times out is now recorded as **Failed**, not "Cancelled". The worker caught
+  every `OperationCanceledException`, including the `TaskCanceledException` an `HttpClient.Timeout`
+  raises (its token is not the run's), so a real failure was labelled as an operator-initiated
+  cancellation with its error detail discarded — and, because that path does not rethrow, Hangfire
+  recorded the run as succeeded. Only a cancellation from the run's own token takes that path now.
+- Concurrent saves of the same report's schedule override — or of the same report config — no longer
+  collide: both file stores staged every write through a temp path derived only from the report name,
+  so two overlapping saves shared it (sharing violation, or a `FileNotFound` when the second move
+  found the first had taken it). Both now share `AtomicFileWrite`, whose temp name is unique per
+  write and cleaned up if the save fails.
 - XLSX output no longer corrupts on edge-case cell values: an XML-illegal control character in a
   string is stripped (previously it threw and aborted the whole file), `NaN`/`Infinity` are written
   as text (previously an un-openable number cell), `byte[]` is Base64 (previously `"System.Byte[]"`),

docs/STATUS-AND-BACKLOG.md

@@ -136,6 +136,56 @@ ones are **fixed** (PR pending/merged); two representation tradeoffs are recorde
   before 1899-12-30. Both are inherent to the OADate/no-tz cell model; revisit only if a real report
   needs sub-day-offset fidelity or pre-1900 dates.
 
+A third hunt covered the destination (upload) and job/scheduling layers. Two fixes shipped; the rest
+need a decision.
+- **Job/schedule robustness — FIXED.** (a) The worker's unfiltered `catch (OperationCanceledException)`
+  recorded an `HttpClient.Timeout` (`TaskCanceledException`, foreign token) as "Cancelled." and did
+  not rethrow, so a genuine failure looked operator-initiated and Hangfire saw success — now filtered
+  on the run's own token. Because a **deadline** also cancels through a linked token (the run's own
+  token is not cancelled either), the runner now reports it as `ReportDeadlineExceededException` — an
+  `OperationCanceledException` subclass — so the worker keeps recording a deadline as `Cancelled`
+  (now with a reason saying so) while everything else becomes `Failed`. Both directions are covered by
+  regression tests, each verified to fail without its fix. (b) `FileScheduleOverrideStore` **and**
+  `FileReportConfigStore` staged every save through a fixed `{name}.json.tmp`, so concurrent saves for
+  one name collided — both now use the shared `AtomicFileWrite` (unique temp name, deleted if the save
+  fails). Verified correct in the same pass: `InMemoryJobStore` thread-safety,
+  `EffectiveSchedule.Resolve` (override/tombstone/fallback), `ScheduleReconciliationHostedService`
+  (add/update/remove, no duplicate), `CronValidation` (UTC, Cronos, no off-by-one), and the whole
+  Local/S3 upload path for stream position, disposal, failure mapping and atomicity.
+- **⚠️ S3 key templating does not guard caller-controlled parameters (highest open security item).**
+  `LocalDestination` passes `LocalPathSegment.EnsureSafe` to `PathTemplate.Expand` (the WP2 guard);
+  `S3Destination` passes **none** — deliberately, since `/` is a legitimate key separator. But that
+  reasoning covers the author's template, not `{param}` values, which come from the run request body.
+  With a key template like `reports/{tenant}/{name}.{ext}`, a caller posting `tenant = "other"` (or a
+  value containing `/`) steers the object into another prefix — a **cross-tenant write** where a
+  shared bucket relies on prefix isolation. Not an OS traversal (S3 keys are literal, `..` is not
+  collapsed) and harmless in a single-tenant bucket. The fix is a decision because the safe version
+  (reject `/` in substituted **values** while keeping it in template literals) would break anyone
+  intentionally passing a hierarchy fragment as a parameter. **Recommended:** adopt that guard and
+  note it as breaking, or document that S3 key templates must not interpolate untrusted parameters.
+- **Upload swallows `OperationCanceledException` into a `Fail` result (deferred — semantics).** Both
+  destinations' `catch (Exception)` also catch a cancellation, so a deadline firing mid-upload is
+  reported as a destination error rather than a cancellation. The run still ends Failed, so this is
+  attribution accuracy; rethrowing would also change multi-destination behaviour (today the loop
+  continues and reports per-destination results).
+- **Hangfire applies its default 10-attempt `AutomaticRetry` (deferred — decision).** The invoker
+  carries no `[AutomaticRetry(Attempts = 0)]` and nothing configures `GlobalJobFilters`, so a
+  deterministically failing job (bad credentials, unreachable source) is re-run up to 10× — re-reading
+  the whole dataset each time and flapping the stored status Failed→Running→Failed. Output integrity
+  holds (temp-dir staging is idempotent), but it contradicts the "a job is atomic, one attempt"
+  model (rule 6). Decide whether NeoReports should pin `Attempts = 0` or leave retries to the host.
+- **`InMemoryJobScheduler.RegisterRecurringAsync` remove-then-add isn't atomic (deferred — narrow).**
+  Two concurrent registrations for one report can both start a loop; the loser is overwritten in the
+  dictionary without its CTS being cancelled, so it keeps firing untracked for the process lifetime.
+  Reachable only by racing two schedule updates (or one against startup reconciliation); the Hangfire
+  path is safe (`AddOrUpdate` is idempotent). A lock around register/remove would fix it.
+- **The in-memory recurring loop has no catch-all (deferred — narrow).** Any non-cancellation throw
+  faults the fire-and-forget loop and the schedule silently stops for the process lifetime, unlogged.
+- **`CompletedPartial` surfaces as a `Completed` job (by design, flagged).** A run that skipped
+  batches maps to `ReportJobStatus.Completed`; the skip is visible only in `Stats.SkippedBatches`.
+  There is no `Partial` job status. Worth confirming this is still the intent, since silent partial
+  data reads as a green job.
+
 ---
 
 ## Where the fuller context lives

src/Jobs/NeoReports.Jobs/ReportJobWorker.cs

@@ -69,17 +69,31 @@ public async Task RunAsync(
                 "Job {JobId} for report {Report} finished with status {Status} (read={Read}, written={Written}).",
                 jobId, reportName, status, result.Stats.RecordsRead, result.Stats.RecordsWritten);
         }
-        catch (OperationCanceledException)
+        // A cancellation is either one caused by OUR token (a caller-requested cancel) or a run that
+        // exceeded its configured deadline — the runner reports the latter as
+        // ReportDeadlineExceededException precisely because the caller's token is not cancelled for
+        // it. An unfiltered catch also swallowed an OperationCanceledException raised by anything
+        // else — most commonly HttpClient.Timeout, which throws TaskCanceledException carrying its
+        // own internal token, and which the runner deliberately lets through (its batch handlers
+        // filter on `ex is not OperationCanceledException`). Those are genuine failures: labelling
+        // them "Cancelled." discarded the real error and, because this path does not rethrow, made
+        // Hangfire record the run as succeeded.
+        catch (OperationCanceledException ex)
+            when (cancellationToken.IsCancellationRequested || ex is ReportDeadlineExceededException)
         {
             // Record the event before flipping the store status, so any observer that polls the
             // store and sees Cancelled is guaranteed to already find the event on lookup — not a
             // race where the status update wins and the event append hasn't landed yet.
             await TryEmitCancelledEventAsync(jobId).ConfigureAwait(false);
+            // A deadline expiry is still a cancellation, but say so: "Cancelled." alone gives an
+            // operator no way to tell a caller-requested cancel from a run that ran out of time. The
+            // message is ours (curated, secret-free), so it is safe to persist verbatim.
+            var reason = ex is ReportDeadlineExceededException deadline ? deadline.Message : "Cancelled.";
             // Use CancellationToken.None for the store update — the cancelling token is already
             // tripped and we still need to record the terminal state.
-            await _store.UpdateStatusAsync(jobId, ReportJobStatus.Cancelled, "Cancelled.", CancellationToken.None)
+            await _store.UpdateStatusAsync(jobId, ReportJobStatus.Cancelled, reason, CancellationToken.None)
                 .ConfigureAwait(false);
-            _logger.LogInformation("Job {JobId} for report {Report} was cancelled.", jobId, reportName);
+            _logger.LogInformation(ex, "Job {JobId} for report {Report} was cancelled ({Reason}).", jobId, reportName, reason);
         }
         catch (Exception ex)
         {

src/NeoReports.Core/AtomicFileWrite.cs

@@ -0,0 +1,54 @@
+namespace NeoReports.Core;
+
+/// <summary>
+/// Writes a file atomically: the content goes to a temp file first and is then moved over the final
+/// path, so a reader never observes a half-written document and a crash mid-write leaves the previous
+/// version intact.
+/// </summary>
+internal static class AtomicFileWrite
+{
+    /// <summary>
+    /// Writes <paramref name="content"/> to <paramref name="finalPath"/> through a unique temp file
+    /// in the same directory (so the move stays on one volume and therefore atomic).
+    /// </summary>
+    /// <param name="finalPath">The destination path.</param>
+    /// <param name="content">The text to write.</param>
+    /// <param name="cancellationToken">Cancellation token.</param>
+    public static async Task WriteAsync(string finalPath, string content, CancellationToken cancellationToken)
+    {
+        // The temp name must be unique per write, not derived from the destination alone: two
+        // concurrent saves of the same document would otherwise open and move the SAME temp file,
+        // failing with a sharing violation or a FileNotFound when the second move finds the first
+        // already took it. The ".tmp" suffix also keeps it out of the "*.json" listings these stores do.
+        string tempPath = $"{finalPath}.{Guid.NewGuid():N}.tmp";
+        try
+        {
+            await File.WriteAllTextAsync(tempPath, content, cancellationToken).ConfigureAwait(false);
+            File.Move(tempPath, finalPath, overwrite: true);
+        }
+        catch
+        {
+            // A unique temp name can't be reclaimed by the next attempt the way a fixed one was, so
+            // clean it up here rather than leaving an orphan behind on every failed save.
+            TryDelete(tempPath);
+            throw;
+        }
+    }
+
+    private static void TryDelete(string path)
+    {
+        try
+        {
+            if (File.Exists(path))
+                File.Delete(path);
+        }
+        catch (IOException)
+        {
+            // Best-effort cleanup: a leftover temp file must never mask the original failure.
+        }
+        catch (UnauthorizedAccessException)
+        {
+            // Same as above.
+        }
+    }
+}

src/NeoReports.Core/Configuration/FileReportConfigStore.cs

@@ -25,10 +25,7 @@ public async Task SaveAsync(string name, string configDocument, CancellationToke
         ArgumentNullException.ThrowIfNull(configDocument);
 
         Directory.CreateDirectory(_directory);
-        string finalPath = GetPath(name);
-        string tempPath = finalPath + ".tmp";
-        await File.WriteAllTextAsync(tempPath, configDocument, cancellationToken).ConfigureAwait(false);
-        File.Move(tempPath, finalPath, overwrite: true);
+        await AtomicFileWrite.WriteAsync(GetPath(name), configDocument, cancellationToken).ConfigureAwait(false);
     }
 
     /// <inheritdoc />

src/NeoReports.Core/Pipeline/ReportDeadlineExceededException.cs

@@ -0,0 +1,34 @@
+namespace NeoReports.Core.Pipeline;
+
+/// <summary>
+/// Thrown when a run exceeds the whole-job deadline configured with
+/// <c>ReportBuilder&lt;T&gt;.Deadline(TimeSpan)</c>.
+/// <para>
+/// It derives from <see cref="OperationCanceledException"/> because a deadline <b>is</b> a
+/// cooperative cancellation — every existing <c>catch (OperationCanceledException)</c> keeps working
+/// and the run still surfaces as cancelled. The distinct type exists so a caller can tell a deadline
+/// apart from an <see cref="OperationCanceledException"/> raised by something else: the run's own
+/// token is not cancelled in either case, so the token alone cannot discriminate (an
+/// <c>HttpClient.Timeout</c>, for instance, throws a <see cref="TaskCanceledException"/> carrying its
+/// own already-cancelled internal token, which is a genuine failure rather than a cancellation).
+/// </para>
+/// </summary>
+public sealed class ReportDeadlineExceededException : OperationCanceledException
+{
+    /// <summary>Creates the exception.</summary>
+    /// <param name="reportName">The report whose deadline elapsed.</param>
+    /// <param name="deadline">The configured deadline.</param>
+    /// <param name="innerException">The cancellation that unwound the run.</param>
+    public ReportDeadlineExceededException(string reportName, TimeSpan deadline, Exception? innerException = null)
+        : base($"Report '{reportName}' exceeded its {deadline} deadline and was cancelled.", innerException)
+    {
+        ReportName = reportName;
+        Deadline = deadline;
+    }
+
+    /// <summary>The report whose deadline elapsed.</summary>
+    public string ReportName { get; }
+
+    /// <summary>The configured deadline.</summary>
+    public TimeSpan Deadline { get; }
+}

src/NeoReports.Core/Pipeline/ReportRunner.cs

@@ -67,12 +67,18 @@ public async Task<ReportRunResult> RunAsync(
         {
             return await ExecuteAsync(report, execution, _services, runToken).ConfigureAwait(false);
         }
-        catch (OperationCanceledException) when (deadlineCts is { IsCancellationRequested: true } && !cancellationToken.IsCancellationRequested)
+        catch (OperationCanceledException ex) when (deadlineCts is { IsCancellationRequested: true } && !cancellationToken.IsCancellationRequested)
         {
             logger.LogWarning(
+                ex,
                 "Report {Report} (job {JobId}) exceeded its {Deadline} deadline and was cancelled.",
                 report.Name, jobId, report.Deadline);
-            throw;
+            // Rethrown as a deadline-specific OperationCanceledException: the caller's own token is
+            // NOT cancelled here, so a plain OCE is indistinguishable from one raised by something
+            // else (an HttpClient.Timeout, say — a genuine failure). The job worker needs that
+            // distinction to keep recording a deadline as Cancelled while reporting the rest as
+            // Failed. Still an OperationCanceledException, so existing catch sites are unaffected.
+            throw new ReportDeadlineExceededException(report.Name, report.Deadline!.Value, ex);
         }
     }
 

src/NeoReports.Core/Scheduling/FileScheduleOverrideStore.cs

@@ -6,8 +6,8 @@ namespace NeoReports.Core.Scheduling;
 /// <summary>
 /// File-backed <see cref="IScheduleOverrideStore"/>: one <c>{reportName}.json</c> file per override
 /// in a configured directory, containing the serialized <see cref="ScheduleOverrideEntry"/> (a null
-/// <c>cron</c> is the tombstone). Writes go through a temp file plus an atomic move, the same
-/// pattern <see cref="FileReportConfigStore"/> uses.
+/// <c>cron</c> is the tombstone). Writes go through <see cref="AtomicFileWrite"/> (unique temp file
+/// plus an atomic move), shared with <see cref="FileReportConfigStore"/>.
 /// </summary>
 public sealed class FileScheduleOverrideStore : IScheduleOverrideStore
 {
@@ -30,11 +30,8 @@ public async Task SaveAsync(string reportName, ScheduleOverrideEntry entry, Canc
         ArgumentNullException.ThrowIfNull(entry);
 
         Directory.CreateDirectory(_directory);
-        string finalPath = GetPath(reportName);
-        string tempPath = finalPath + ".tmp";
         string document = JsonSerializer.Serialize(entry, Json);
-        await File.WriteAllTextAsync(tempPath, document, cancellationToken).ConfigureAwait(false);
-        File.Move(tempPath, finalPath, overwrite: true);
+        await AtomicFileWrite.WriteAsync(GetPath(reportName), document, cancellationToken).ConfigureAwait(false);
     }
 
     /// <inheritdoc />

tests/NeoReports.Core.UnitTests/Scheduling/FileScheduleOverrideStoreTests.cs

@@ -99,9 +99,41 @@ await Should.ThrowAsync<ArgumentException>(
             () => store.SaveAsync(invalidName, new ScheduleOverrideEntry("0 6 * * 1"), CancellationToken.None));
     }
 
+    [Fact]
+    public async Task A_successful_save_leaves_no_temp_file_behind()
+    {
+        var store = new FileScheduleOverrideStore(_directory);
+
+        await store.SaveAsync("alpha", new ScheduleOverrideEntry("0 6 * * 1"), CancellationToken.None);
+
+        // The write is staged through a unique temp file and moved into place; nothing may linger
+        // (a stray temp would also have to stay out of ListAsync's "*.json" enumeration).
+        Directory.EnumerateFiles(_directory, "*.tmp").ShouldBeEmpty();
+        (await store.ListAsync(CancellationToken.None)).ShouldHaveSingleItem().ReportName.ShouldBe("alpha");
+    }
+
+    [Fact]
+    public async Task A_failed_save_cleans_up_its_temp_file_and_surfaces_the_error()
+    {
+        var store = new FileScheduleOverrideStore(_directory);
+        Directory.CreateDirectory(_directory);
+        // A directory sitting where the store wants its file makes the final move fail *after* the
+        // temp file has been written — the one path that must not leave an orphan behind.
+        Directory.CreateDirectory(Path.Join(_directory, "alpha.json"));
+
+        // The exact type is the OS's business (Windows raises UnauthorizedAccessException here, other
+        // platforms an IOException); what matters is that the failure surfaces rather than being
+        // swallowed by the cleanup.
+        await Should.ThrowAsync<Exception>(
+            () => store.SaveAsync("alpha", new ScheduleOverrideEntry("0 6 * * 1"), CancellationToken.None));
+
+        Directory.EnumerateFiles(_directory, "*.tmp").ShouldBeEmpty();
+    }
+
     public void Dispose()
     {
         if (Directory.Exists(_directory))
             Directory.Delete(_directory, recursive: true);
+        GC.SuppressFinalize(this);
     }
 }