Fix 14 process lifecycle bugs: orphans, races, leaks, cancellation#25
Merged
Fix 14 process lifecycle bugs: orphans, races, leaks, cancellation#25
Conversation
Critical/High: - Fix infinite recursion in StartRedirected(IConsoleLineHandler, string, params string[]) - ExecAsync: replace sync WaitForExit(int) + ignored CancellationToken with linked CancellationTokenSource; kill process on timeout and on caller cancellation; remove broken HardWaitForExitAsync - Exec: kill process before HardWaitForExit on timeout (was leaving orphan processes) Medium: - BufferedObservableProcess: fix TOCTOU race where process exit between HasExited check and Exited+= registration caused WaitForCompletion to hang; register handler first, re-check HasExited after - BufferedObservableProcess: dispose old CancellationTokenSource in CancelAsyncReads instead of leaking it - ObservableProcessBase: make _disposing volatile and never reset it; wrap Dispose() in _exitLock so concurrent Exited event + Dispose() cannot both enter Stop() - StartLongRunning: dispose WaitHandle (ManualResetEvent) in Dispose(); expose it as public so callers can also wait on it Low: - ObservableProcessBase: replace _sentControlC bool with int + Interlocked.CompareExchange for thread-safe single-fire guarantee - Add CancellationToken support to WaitForCompletion, Proc.Start, Proc.StartRedirected, and Proc.StartLongRunning (backward-compatible optional parameter) - Extract shared HardWaitForExit into ProcessWaitExtensions to remove three near-identical implementations Also add regression tests for each fix (one test file per source file). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CI nupkg-validator tool (0.10.1) bundles FSharp.Core 10.0.100. The updated .NET SDK on the CI runner now compiles Proc.Fs.dll against FSharp.Core 10.1.x, causing the validator to fail when loading the DLL. Pinning to 10.0.100 keeps the compiled DLL compatible with the validator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
Full review of all public
Procmethods surfaced 14 bugs ranging from a critical infinite recursion to medium thread-safety gaps. This PR fixes all of them and adds a regression test per source file.Critical / High
StartRedirected(IConsoleLineHandler, string, params string[])— fixed infinite recursion (params string[]coercion made it call itself, causingStackOverflowException)ExecAsync— replaced synchronousWaitForExit(int)(which blocked the thread and ignoredCancellationToken) with a linkedCancellationTokenSource; process is now killed on both timeout and caller cancellation (previously became an orphan); removedHardWaitForExitAsyncwhich unnecessarily wrapped an already-async method inTask.RunExec— process is now killed beforeHardWaitForExiton timeout (previously an orphan was left running)Medium
BufferedObservableProcess.KickOff— TOCTOU race: process could exit betweenHasExitedcheck andExited +=registration, causingWaitForCompletionto hang indefinitely; fixed by registering handler first, re-checkingHasExitedafterBufferedObservableProcess.CancelAsyncReads— cancelledCancellationTokenSourcewas replaced but never disposed (OS handle leak on every cancel/start cycle)ObservableProcessBase.Dispose—_isDisposingwas notvolatileand was reset tofalseafterStop(), creating a window for lateExitStopcalls to race in; nowvolatile bool _disposingthat is never reset;Dispose()also acquires_exitLockto prevent concurrentStop()from bothExitStopandDispose()LongRunningApplicationSubscription—WaitHandle(ManualResetEvent) was never disposed; also changed frominternaltopublicso callers can wait on it directlyLow
_sentControlC— replacedboolwithint+Interlocked.CompareExchangefor a thread-safe single-fire guaranteeCancellationTokensupport —WaitForCompletion,Proc.Start,Proc.StartRedirected, andProc.StartLongRunningall now accept an optionalCancellationToken; all are backward-compatible additionsHardWaitForExit— three near-identical implementations consolidated intoProcessWaitExtensions.HardWaitForExit(this Process, TimeSpan)Test plan
dotnet testpasses (48 passed, 4 skipped — Windows-only ControlC tests)StartRedirectedBugTests— proves recursion fix (previously crashed the test runner)ExecAsyncBugTests— provesCancellationTokenis respected within 500ms, and child process is dead after cancellation (PID file check)ExecBugTests— proves child process is dead after timeout (PID file check)BufferedObservableProcessBugTests— 30 fast-exiting processes all complete their observable (TOCTOU stress test)StartLongRunningBugTests—WaitHandle.WaitOne()throwsObjectDisposedExceptionafter disposeObservableProcessBaseTests— 30 concurrent exit+dispose iterations without exceptionsCancellationTokenTests—Start,StartRedirected,StartLongRunning, andWaitForCompletionall abort promptly on cancellation🤖 Generated with Claude Code