Repository navigation
ci(verify): keep and print Windows crash dumps from the engine suites - #3403
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Comment on lines
+196
to
+197
| $dir = "$env:RUNNER_TEMP\verify-report\crash-dumps" | ||
| New-Item -ItemType Directory -Force $dir | Out-Null |
Contributor
There was a problem hiding this comment.
If prepare_recorded_fixtures crashes, Windows saves its dump here, but the failed preparation step skips Test. Upload report then skips too: it only accepts a skipped test when the macOS collector sets collected=true. The printed stack remains, but the dump disappears with the runner.
Let the Windows collector signal that dumps exist, and include that signal in the upload condition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/engine-verify-os.yml
Line: 196-197
Comment:
**Fixture crash dumps disappear**
If `prepare_recorded_fixtures` crashes, Windows saves its dump here, but the failed preparation step skips `Test`. `Upload report` then skips too: it only accepts a skipped test when the macOS collector sets `collected=true`. The printed stack remains, but the dump disappears with the runner.
Let the Windows collector signal that dumps exist, and include that signal in the upload condition.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Member
Author
|
@greptileai review |
This branch was successfully deployed
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
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.
Windows Engine verify sometimes kills an export test process with an access violation (
0xc0000005), on main and on unrelated branches:colour,gif,movandcamera_offsetin the last week, a different test each time. The job log shows only the exit code, so there is nothing to diagnose from.Two things hid the crash:
run.shruns in Git Bash, and MSYS starts its child processes with crash reporting off (SEM_NOGPFAULTERRORBOX). nextest and the test binaries inherit that, so Windows never writes a dump, even with Windows Error Reporting configured.This change applies to Windows test jobs only:
scripts/verify/native-launch.ps1(PowerShell clears the inherited mode) via Cargo's target runner, which nextest honors;cdband the build's own PDBs.Checked on
blacksmith-8vcpu-windows-2025, with nextest 0.9.148 and a test crate:cdbreads itmacOS, Linux and local runs are unchanged.


Confidence Score: 5/5
The PR appears safe to merge; the previous dump-upload issue is addressed.Summary
Keeps Windows crash dumps from engine tests and prints their stacks.
Reviews (2) · Last reviewed commit: "ci(verify): upload Windows crash dumps w..." · Reviewed by Greptile