Skip to content

Commit a completed events export even if Cancel is clicked late#669

Open
jschick04 wants to merge 1 commit into
mainfrom
jschick/export-cancel-race
Open

Commit a completed events export even if Cancel is clicked late#669
jschick04 wants to merge 1 commit into
mainfrom
jschick/export-cancel-race

Conversation

@jschick04

Copy link
Copy Markdown
Collaborator

Summary

MauiMenuActionService.ExportEventsAsync passed the export's cancellation token to SaveStreamingAsync, so AtomicFileWriter's post-write cancellation gate (a ThrowIfCancellationRequested immediately before the atomic commit) could throw in the tiny window between the streaming write finishing and the commit. A Cancel click landing there discarded a fully written export and reported it canceled. A finished flag tried to disarm the banner Cancel after the write, but it was set a few instructions too late to close the race.

Fix

Scope cancellation to the write phase only:

  • Pass the CTS token directly to EventTableExporter.ExportAsync — it throws per row on Cancel, so a mid-write Cancel discards the temp file (unchanged behavior).
  • Pass CancellationToken.None to SaveStreamingAsync, so once the write completes the atomic commit is unconditional — a late Cancel is now a no-op and the completed export is saved.

This matches Windows Event Viewer (a finished export shouldn't be thrown away) and the two other SaveStreamingAsync callers (DebugLogModal, DatabaseToolsLogView), which already pass None. The banner Cancel callback now swallows ObjectDisposedException (only) to stay safe against the CTS being disposed during teardown, replacing the racy finished flag; any other throw still reaches BannerActionGuard.

Behavior

  • Cancel during the write -> ExportAsync throws -> temp discarded, "Export canceled". (unchanged)
  • Cancel after the write completes -> ignored -> the finished export is saved. (the fix)
  • AtomicFileWriter (shared, safety-critical) is unchanged; the other callers are unaffected.

Testing

The MAUI action has no unit-test harness, so the two guarantees are locked at the Runtime layer:

  • EventTableExporterTests.ExportAsync_CancelledToken_ThrowsAndWritesNothing — a canceled token aborts the export and writes nothing (so AtomicFileWriter discards the temp).
  • AtomicFileWriterTests.WriteAsync_NoneToken_CommitsCompletedWriteEvenWhenASeparateSourceIsCancelled — a completed write commits under CancellationToken.None even when a separate source is canceled (complements the existing WriteAsync_CancelledAfterWrite, which proves the gate fires for a real token).

Full solution builds; Runtime.Tests 2032/2032.

Copilot AI review requested due to automatic review settings July 24, 2026 18:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a race in the MAUI “Export events” flow where a late Cancel click could discard a fully-written export by triggering AtomicFileWriter’s post-write cancellation gate. The change scopes cancellation to the export/write phase only, ensuring that once streaming finishes the atomic commit always proceeds.

Changes:

  • In MauiMenuActionService.ExportEventsAsync, pass CancellationToken.None to SaveStreamingAsync so AtomicFileWriter commits unconditionally after the write completes, while still canceling the export write via a separate CTS token.
  • Replace the racy finished flag with an ObjectDisposedException guard around the Cancel callback to handle CTS disposal during teardown safely.
  • Add unit tests covering (1) exporter behavior with an already-canceled token and (2) AtomicFileWriter committing under CancellationToken.None even if a separate cancellation source is triggered.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/Unit/EventLogExpert.Runtime.Tests/Export/EventTableExporterTests.cs Adds coverage ensuring a canceled token aborts export and writes nothing.
tests/Unit/EventLogExpert.Runtime.Tests/Common/Files/AtomicFileWriterTests.cs Adds coverage confirming CancellationToken.None allows commit even if an unrelated CTS is canceled.
src/EventLogExpert/Adapters/Menu/MauiMenuActionService.cs Scopes cancellation to the export/write phase and prevents late Cancel from aborting the atomic commit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/EventLogExpert/Adapters/Menu/MauiMenuActionService.cs Outdated
MauiMenuActionService.ExportEventsAsync passed the export's cancellation token
to SaveStreamingAsync, so AtomicFileWriter's post-write cancellation gate could
throw between the streaming write finishing and the atomic commit. A Cancel
click landing in that window discarded a fully written export and reported it
canceled. A `finished` flag tried to disarm the banner Cancel after the write,
but it was set a few instructions too late to close the race.

Scope cancellation to the write phase instead: pass the CTS token directly to
EventTableExporter.ExportAsync (which throws per row on Cancel, so the temp is
discarded mid-write) and pass CancellationToken.None to SaveStreamingAsync so
the atomic commit is unconditional once the write completes. A late Cancel is
now a no-op and the completed export is saved, matching Windows Event Viewer
and the two other SaveStreamingAsync callers that already pass None. The banner
Cancel callback swallows ObjectDisposedException to stay safe against the CTS
being disposed during teardown, replacing the racy `finished` flag.

Lock the two guarantees at the Runtime layer (the MAUI action has no test
harness): EventTableExporter aborts and writes nothing on a canceled token,
and AtomicFileWriter commits a completed write under CancellationToken.None
even when a separate source was canceled.
@jschick04
jschick04 force-pushed the jschick/export-cancel-race branch from 1baab55 to 7c9069d Compare July 24, 2026 19:26
Copilot AI review requested due to automatic review settings July 24, 2026 19:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@jschick04
jschick04 marked this pull request as ready for review July 24, 2026 19:31
@jschick04
jschick04 requested a review from a team as a code owner July 24, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants