test: verify automatic flush on exit without Stop-Sentry - #138
test: verify automatic flush on exit without Stop-Sentry#138jamescrosswell wants to merge 3 commits into
Conversation
Add a regression test for #38 that runs a child process which starts Sentry, captures a message, and exits WITHOUT calling Stop-Sentry. It asserts the process exits cleanly within a timeout (no hang, as in sentry-dotnet#3141) and that the captured event is still delivered. A file-writing transport (FileTransport in utils.ps1) is used so delivery can be observed from the parent after the child exits, without networking/ports in CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When powershell.exe (Windows PowerShell) is spawned from a pwsh host, the inherited PSModulePath points only at PowerShell Core's module directories, preventing autoload of Microsoft.PowerShell.Utility (Import-PowerShellDataFile), which the module's psm1 uses during import. Reset to the machine default under Desktop edition so built-in modules are discoverable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Windows PowerShell 5.1 does not reliably populate .ExitCode on the object returned by `Start-Process -PassThru` after WaitForExit(timeout). Start the child via System.Diagnostics.Process instead, reading stdout/stderr async to avoid pipe-buffer deadlocks, so the exit code is available on both editions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| try { | ||
| $exited = $proc.WaitForExit($TimeoutSeconds * 1000) | ||
| if (-not $exited) { | ||
| try { $proc.Kill() } catch {} |
|
@vaind this was mostly done by a clunker months ago... looks valuable though so I figured rather than closing, we may as well merge. |
vaind
left a comment
There was a problem hiding this comment.
SynchronousWorker.EnqueueEnvelope sends inline and Wait()s, and FileTransport returns Task.CompletedTask, so the envelope is on disk before the process starts exiting — 7793 bytes right after CaptureMessage, still 7793 after exit. There is no exit-time flush for a child process to observe, so this doesn't yet guard the #85 regression (details inline).
Suggested split, each at the level where it's deterministic:
- Keep the delivery test, renamed to what it proves.
- Hang guard out-of-process, with the real
SynchronousTransportagainst a closed port. - Deferred-flush path as an in-process unit test.
Harness nits:
- Move
$psi.EnvironmentVariables.Remove('PSModulePath')intoInvoke-ExitFlushChildand drop the child'sPSEdition -eq 'Desktop'branch — CI runs Pester undershell: powershell, sopwshis the cross-edition case and that branch is skipped there. - Add
-NonInteractive, or a binding failure on mandatory$OutputFileprompts and reads as a hang. - After
Kill(),$stdoutTask.Wait(5000)rather than.Result— otherwise hang detection can itself hang on an open pipe. - Surface the
Kill()exception instead ofcatch {}. [Process]::Start($psi)inside thetry, so a throw in the stream setup doesn't leak the process.- Minor: GUID temp name instead of
GetTempFileName()+ delete;-Encoding UTF8onGet-Content -Raw(WinPS 5.1 decodes as ANSI);StdErrin the third-Because; assertCHILD_DONEor drop it.
| $result = Invoke-ExitFlushChild -Executable 'powershell.exe' | ||
| $result.Exited | Should -BeTrue -Because "the process must not hang on exit. StdErr: $($result.StdErr)" | ||
| $result.ExitCode | Should -Be 0 -Because "the process must exit cleanly. StdErr: $($result.StdErr)" | ||
| $result.Envelope | Should -Match 'hello-from-exit-flush-test' -Because 'the captured event must be delivered even without Stop-Sentry' |
There was a problem hiding this comment.
This assertion (and line 83) can't fail for the reason the test exists. Adding $_.DisableAppDomainProcessExitFlush() to the child's Start-Sentry block — reintroducing exactly what #85 removed — still gives exit code 0 and an envelope containing hello-from-exit-flush-test, because SynchronousWorker + FileTransport finish delivery at capture time.
What it does prove is worth keeping: an event is delivered without Stop-Sentry. Suggest renaming the Describe to that and rewording the header — the guarantee comes from the synchronous worker, not the exit hook.
|
|
||
| Start-Sentry { | ||
| $_.Dsn = 'https://key@127.0.0.1/1' | ||
| $_.Transport = [FileTransport]::new($OutputFile) |
There was a problem hiding this comment.
This also displaces SynchronousTransport, the component that actually hung in getsentry/sentry-dotnet#3141, so $result.Exited can't reproduce that failure either: FileTransport returns Task.CompletedTask and FlushAsync then waits on an empty task list.
Worth a separate case that keeps the real transport and just points the DSN at a closed port:
Start-Sentry {
# Port 1 is closed, so the real SynchronousTransport runs and fails fast. The point isn't
# delivery - it's that AppDomain.ProcessExit invokes PowerShell code (Invoke-WebRequest)
# during shutdown, the shape of sentry-dotnet#3141, and that the process still exits cleanly.
$_.Dsn = 'http://key@127.0.0.1:1/1'
}Confirmed that path is live: with $_.Debug = $true the child logs Registering integration: 'AppDomainProcessExitIntegration' and the shutdown stack is AppDomainAdapter.OnProcessExit → HandleProcessExit → ScriptBlock.InvokeWithPipe. Exits 0 in ~0.8s. Needs a small -Transport File|Http parameter here.
| } | ||
| } | ||
|
|
||
| class FileTransport:Sentry.Extensibility.ITransport { |
There was a problem hiding this comment.
The $unfinishedTasks path is the only case where the exit-time flush has anything to drain, and in-process it's exact — out-of-process it means racing the child's exit. A transport that completes only when the test says so:
class BlockingTransport:Sentry.Extensibility.ITransport {
[System.Threading.Tasks.TaskCompletionSource[bool]] $tcs = [System.Threading.Tasks.TaskCompletionSource[bool]]::new()
# Never completes on its own; the test decides when the send finishes.
[System.Threading.Tasks.Task]SendEnvelopeAsync([Sentry.Protocol.Envelopes.Envelope] $envelope, [System.Threading.CancellationToken] $cancellationToken) {
return $this.tcs.Task
}
}and the test (this passes):
$options.FlushTimeout = [TimeSpan]::FromMilliseconds(1)
$options.Transport = $transport = [BlockingTransport]::new()
$worker = [SynchronousWorker]::new($options)
$worker.EnqueueEnvelope($envelope) | Should -BeTrue
$worker.QueuedItems | Should -Be 1 # send didn't finish within FlushTimeout -> deferred
$transport.tcs.SetResult($true)
$null = $worker.FlushAsync([TimeSpan]::FromSeconds(5))
$worker.QueuedItems | Should -Be 0 # flush drained itReach the private class via InModuleScope Sentry or by dot-sourcing private/SynchronousWorker.ps1. Either way the class has to live in a file dot-sourced after Import-Module (like RecordingTransport here), or it fails at parse time with Unable to find type [Sentry.Extensibility.ITransport].
Summary
Adds a regression test for automatic flush on process exit. After #85 removed the workaround that disabled the .NET SDK's
AppDomain.ProcessExitflush hook (the workaround existed because that hook used to hang/crash — getsentry/sentry-dotnet#3141, fixed in getsentry/sentry-dotnet#4323), a script should no longer need to callStop-Sentryfor events to be delivered.The test launches a child process that:
Stop-Sentry.The parent then asserts the child:
A
FileTransport(added totests/utils.ps1) serializes envelopes to disk so delivery is observable from the parent after the child exits, avoiding networking/open ports in CI.The main goal is to confirm this holds on the Windows CI matrices (Windows PowerShell 5.1 / net462 — the platform the synchronous transport design targets), which we couldn't validate locally.
Part of #38 (validates that the
AppDomain.ProcessExitflush hook re-enabled in #85 works and doesn't hang — the remaining work is making the background worker actually asynchronous, so this doesn't close the issue).#skip-changelog