Add --crash-sequence option to MTP CrashDump extension (#7262)#8526
Conversation
Adds a testhost-side sequence file that records test state transitions (STARTED/ENDED) so the controller can list the tests that were running at the time of a crash without requiring dump analysis. Mirrors HangDump's UX but uses file-based persistence because the testhost is dead by the time the controller inspects it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds crash-time test “sequence file” journaling to the Microsoft.Testing.Platform (MTP) CrashDump extension, controlled by a new --crash-sequence on|off option, so users can see which tests were still running when a testhost crashes (and the file is also published as an artifact).
Changes:
- Introduces testhost-side
CrashDumpSequenceLoggerto append STARTED/ENDED records to a controller-provided sequence file path. - Adds controller-side parsing/publishing/cleanup of the sequence file on crash vs. graceful exit, and wires the path via
TESTINGPLATFORM_CRASHDUMP_SEQUENCE_FILE. - Updates command-line validation, help/info expectations, resources/XLFs, and adds unit + acceptance coverage for the new option.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CrashDumpTests.cs | Adds unit tests covering --crash-sequence validation and the “missing main option” rule. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs | Updates --help / --info expectations to include --crash-sequence. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/CrashDumpTests.cs | Adds acceptance tests for sequence generation, disabling, cleanup on graceful exit, and invalid usage; updates the CrashDump test asset to publish fake in-progress nodes. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/CrashDumpResources.resx | Adds new localized strings for the option, banner text, and artifact metadata. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.cs.xlf | Regenerates localization entries for new CrashDump sequence resources (Czech). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.de.xlf | Regenerates localization entries for new CrashDump sequence resources (German). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.es.xlf | Regenerates localization entries for new CrashDump sequence resources (Spanish). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.fr.xlf | Regenerates localization entries for new CrashDump sequence resources (French). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.it.xlf | Regenerates localization entries for new CrashDump sequence resources (Italian). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ja.xlf | Regenerates localization entries for new CrashDump sequence resources (Japanese). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ko.xlf | Regenerates localization entries for new CrashDump sequence resources (Korean). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pl.xlf | Regenerates localization entries for new CrashDump sequence resources (Polish). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.pt-BR.xlf | Regenerates localization entries for new CrashDump sequence resources (Portuguese - Brazil). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.ru.xlf | Regenerates localization entries for new CrashDump sequence resources (Russian). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.tr.xlf | Regenerates localization entries for new CrashDump sequence resources (Turkish). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hans.xlf | Regenerates localization entries for new CrashDump sequence resources (Chinese Simplified). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Resources/xlf/CrashDumpResources.zh-Hant.xlf | Regenerates localization entries for new CrashDump sequence resources (Chinese Traditional). |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/Microsoft.Testing.Extensions.CrashDump.csproj | Links in RoslynString helper from MTP to support shared helper usage in this extension. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpSequenceLogger.cs | New testhost-side consumer/lifetime handler that writes the sequence journal file. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpProcessLifetimeHandler.cs | Adds crash-time parsing/banner output and artifact publication; deletes sequence file on graceful exit/hangdump kill. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpExtensions.cs | Wires the new composite CrashDumpSequenceLogger into the testhost extension pipeline. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpEnvironmentVariableProvider.cs | Computes and exports the sequence file path env var; persists it into CrashDump configuration for controller-side handling. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpConfiguration.cs | Adds SequenceFileName to share computed path with the process lifetime handler. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpCommandLineProvider.cs | Adds --crash-sequence option and validates boolean-like arguments; treats it as requiring --crashdump/--crash-report. |
| src/Platform/Microsoft.Testing.Extensions.CrashDump/CrashDumpCommandLineOptions.cs | Adds the crash-sequence option name constant. |
Copilot's findings
- Files reviewed: 25/25 changed files
- Comments generated: 3
| catch (IOException ex) | ||
| { | ||
| // Best-effort diagnostic. If we cannot read the sequence file we still publish it so the | ||
| // user can inspect it manually, but skip the friendly summary. | ||
| await _outputDisplay.DisplayAsync(this, new ErrorMessageOutputDeviceData(string.Format(CultureInfo.InvariantCulture, CrashDumpResources.CrashDumpSequenceFileReadError, sequenceFilePath, ex.Message)), cancellationToken).ConfigureAwait(false); | ||
| await _messageBus.PublishAsync(this, new FileArtifact(new FileInfo(sequenceFilePath), CrashDumpResources.CrashDumpSequenceArtifactDisplayName, CrashDumpResources.CrashDumpSequenceArtifactDescription)).ConfigureAwait(false); | ||
| return; |
There was a problem hiding this comment.
Good catch — done in 906fe9a. The exception filter on TryPublishSequenceFileAsync now also covers UnauthorizedAccessException, SecurityException, ArgumentException and NotSupportedException (the documented throwers of File.ReadLines/iteration), and the file is still published as a FileArtifact so the user can inspect it manually.
There was a problem hiding this comment.
Addressed in 906fe9a. TryPublishSequenceFileAsync now catches the broader set IOException or UnauthorizedAccessException or System.Security.SecurityException or ArgumentException or NotSupportedException (filtered catch (Exception ex) when ...) and still publishes the file artifact in that branch so the user can inspect the raw file manually. Failing the crash-handling path because of a diagnostic file read would be strictly worse than missing the summary.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🔍 Build Failure AnalysisSummary — The build failed due to analyzer violations in the newly added Root cause 1: Synchronous I/O operations in async context (VSTHRD103)The new Design consideration: The PR description explicitly documents that the sequence file uses Affected files / errors All errors occur in
Proposed fix Two options:
Recommendation: Option 1 (suppress) aligns with the PR's stated performance design goals. The synchronous writes are load-bearing for the "survives Root cause 2: Legacy lock object (IDE0330)On .NET 9.0 only, line 42 uses Affected files / errors
Proposed fix Use TFM-conditional compilation to choose the appropriate lock type: #if NET9_0_OR_GREATER
private readonly System.Threading.Lock _writeLock = new();
#else
private readonly object _writeLock = new();
#endifBuild overview
All MSBuild errors (14)
🤖 Generated by the Build Failure Analysis workflow using binlog analysis · commit 74e7850
|
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Generated by Build Failure Analysis for issue #8526 · ● 1.2M
- Convert sync StreamWriter calls to async (WriteLineAsync/FlushAsync/DisposeAsync) to satisfy VSTHRD103. - Replace plain `lock` with SemaphoreSlim so we can await inside the critical section (also avoids IDE0330 push to System.Threading.Lock which would require multi-TFM gymnastics). - Suppress CA1416 narrowly on the sync Dispose path's SemaphoreSlim.Wait(), explaining that the path is unreachable on browser because the controller-side env var provider requires NETCOREAPP. - Fix CrashDump test asset to use the netfx-compatible char[] overload of String.Split. End-to-end validation: full `build.cmd -pack` now succeeds on origin/main; 19 CrashDumpTests acceptance tests pass (3 Windows-only skips for --crash-report); 9 HelpInfoAllExtensionsTests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| #pragma warning restore CA1416 | ||
| try | ||
| { | ||
| if (_disposed) |
There was a problem hiding this comment.
Same pattern as the ConsumeAsync re-check: the outer if (_disposed) return; is the fast path; the inner check under the semaphore is the canonical double-checked dispose pattern that prevents a second caller from racing through the dispose body after the first caller already flipped _disposed = true. CodeQL's single-method dataflow doesn't see that the field is mutated under the lock. Keeping as-is.
There was a problem hiding this comment.
Keeping the inner if (_disposed) recheck. The outer fast-path check is before _writeSemaphore.Wait(): two threads can pass it concurrently before either acquires the semaphore. Without the inner recheck the second thread would re-dispose _writer and re-call _writeSemaphore.Dispose() in the finally, the latter throwing SemaphoreFullException (or NRE on _writer?.Dispose() is fine, but the double-Dispose() of the semaphore is not). The recheck is the standard double-checked-locking idiom and is not constant — CodeQL is missing the cross-thread mutation.
…delay - CrashDumpProcessLifetimeHandler.TryPublishSequenceFileAsync now catches UnauthorizedAccessException/SecurityException/ArgumentException/NotSupportedException in addition to IOException so the controller crash path never fails on a best-effort diagnostic read. - CrashDumpCommandLineProvider.ValidateOptionArgumentsAsync collapses the nested 'if' inside each option branch into a single '&&' condition, and the orphan TODO is rewritten as a rationale comment (no .dmp extension enforcement on --crashdump-filename). - CrashDump test asset replaces the fixed 2000ms Task.Delay with a bounded polling loop (50ms tick, 30s deadline) on the sequence file via the TESTINGPLATFORM_CRASHDUMP_SEQUENCE_FILE env var, so the test is typically fast and still deterministic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| catch (IOException) | ||
| { | ||
| // Best-effort cleanup; a leftover sequence file is harmless beyond cluttering the results | ||
| // directory. Avoid surfacing this as a user-visible error because the run itself succeeded. | ||
| } | ||
| catch (UnauthorizedAccessException) | ||
| { | ||
| // Same rationale as IOException. | ||
| } |
Closes #7262.
What
Adds a
--crash-sequence on|offoption (defaulton) toMicrosoft.Testing.Extensions.CrashDumpthat generates a sequence file recording the start and end of each test as the run progresses. On a crash, the controller parses the file, prints a friendly "tests still running at the time of the crash" banner, and publishes the file as aFileArtifact.This addresses the long-standing pain point from the issue: today, on
--crashdump/--crash-report, users have to open the.dmpto know which test was running at crash time.--hangdumpalready produces an equivalent.logover IPC, but that path is unavailable for crashes because the testhost is dead.How
CrashDumpSequenceLogger(new testhostIDataConsumer + ITestSessionLifetimeHandler) appendsSTARTED\t<iso>\t<uid>\t<displayName>/ENDED\t<iso>\t<uid>\t<state>lines perTestNodeUpdateMessageto a sequence file. UsesStreamWriter.AutoFlush=true(nofsync) so records reach the OS cache and surviveEnvironment.FailFastat sub-microsecond cost per write.CrashDumpEnvironmentVariableProvidercomputes a path with a per-controller 8-char GUID token (avoids cross-host collisions for parallel testhost launches into the same results directory) and exposes it viaTESTINGPLATFORM_CRASHDUMP_SEQUENCE_FILE.CrashDumpProcessLifetimeHandleron crash: parses the file, computesstarted \ endedset, prints the friendly banner, publishes the file. On graceful exit / hangdump kill: deletes the file (no diagnostic value).--crash-sequencefollows the--ansistyle (on/off/true/false/enable/disable/1/0), validated by the existingCommandLineOptionArgumentValidator.Performance
The cost is two small writes per test (~10 µs each, no fsync). For 10 000 tests this is ~200 ms total — well under 1 % on huge suites. The user discussed and confirmed this in the design conversation; the default-on policy is justified by the very low cost.
Limitations (documented in source)
FailFasts within a few ms may not be recorded because the MTP message bus is asynchronous by design. This matches the same trade-off described in the issue ("mtp does not send the currently started test to testhost controller because of possible overhead"). The added value is for the much more common "test runs for some time, then crashes" pattern.fsync).Tests
--crash-sequencewithout--crashdump/--crash-report.FailFast(with a small sync delay to give the async consumer time to drain, since this is a deterministic test setup).UpdateXlf.Out of scope
This PR does not change
--hangdumpbehavior: it already generates an equivalent in-progress test list over IPC.