Replace the PID file atomically and treat contention as another instance [patch] - #186
Merged
Merged
Conversation
…nce [patch] WritePidFile wrote the file in place, so two instances starting together could tear it, and a torn file read as "no instance", letting both launch. Reading or writing the file while another instance held it threw an IOException out of ShouldLaunch instead of returning a decision. - WritePidFile writes a temporary file beside the PID file and moves it over, retrying briefly while another instance holds the file - Reads retry briefly on IOException/UnauthorizedAccessException; a file that stays inaccessible reads as another instance starting - Only the leading JSON record is parsed, so a file torn by older versions still names the instance that wrote it - The post-sleep recheck treats unparseable content as a racing instance rather than as no instance, and ShouldLaunch returns false if the PID file cannot be written Fixes #181 Fixes #182 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018x3sGQTDEmvXP1cRdbmmkx
On Windows a freshly started process reports no main module until the loader has finished, so the tests recorded a null module path and then compared it against the loaded one. The helper now waits until its main module is readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018x3sGQTDEmvXP1cRdbmmkx
A directory where the PID file belongs can never be replaced, which drives WritePidFile through every retry to its exception and checks that the temporary file is removed. ShouldLaunch now goes through TryWritePidFile, so the not-claimed outcome is tested directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018x3sGQTDEmvXP1cRdbmmkx
The replace loop had no stop condition of its own (Sonar S1994); it now retries a bounded number of times and makes the final attempt outside the loop, so a persistent failure still reaches the caller. The race tests pass the test's cancellation token and wait with Task.Delay instead of Thread.Sleep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018x3sGQTDEmvXP1cRdbmmkx
|
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.



Fixes #181
Fixes #182
Both issues come from the same concurrent PID-file race, and both triage comments recommend fixing them together. They also rewrite the same two methods (
WritePidFileand the read inIsAlreadyRunning), so separate PRs would conflict with each other.What was wrong
WritePidFilewrote in place withFile.WriteAllText. Two instances starting together could tear the file: the shorter record, then the tail of the longer one. The torn file failed to parse, fell through to the legacy integer path, and read as "no instance". Every racing instance launched, and so did the next launch after them.IsAlreadyRunningcaught only not-found and format errors, andWritePidFilecaught nothing. A sharing violation from a concurrent writer, or a locked or unreadable file, went throughShouldLaunch()as an unhandledIOExceptionand crashed the app at startup.Change
WritePidFilewrites to<pid>.<guid>.tmpbeside the PID file and moves it over the PID file:File.Move(…, overwrite: true)on .NET Core 3.0 and later,File.Replace/File.Moveon netstandard. Readers see either the old file or the new one. The replace is retried up to 5 times with a short linear backoff while another instance holds the file. The temp file is always cleaned up.IOException/UnauthorizedAccessException. If the file stays inaccessible, it reads asAnotherInstance. IfWritePidFilestill can't replace the file,ShouldLaunch()returnsfalserather than throwing.Utf8JsonReader+Deserialize(ref reader)). A record followed by garbage still names the instance that wrote it, and the full identity check still applies to it. Legacy plain-integer files and all the existing malformed-content cases behave as before.PidFileState(NoInstance/AnotherInstance/Unreadable) lets the post-sleep recheck inShouldLaunch()tell the cases apart. Content that can't be parsed right after we wrote a whole file came from a racing instance, so it no longer counts as "no instance". The first check still treats unparseable content as stale, so a leftover corrupt file doesn't block launch forever.IsAlreadyRunning()keeps its signature and existing behaviour.Tests
Five new tests. All five fail with the library change reverted and pass with it:
IsAlreadyRunning_WithGarbageSuffixedPidFileForRunningInstance_ShouldReturnTrueShouldLaunch_WithGarbageSuffixedPidFileForRunningInstance_ShouldReturnFalseShouldLaunch_WhenPidFileBecomesUnreadableDuringRaceWindow_ShouldReturnFalseWritePidFile_WhileBeingRead_ReaderNeverSeesAPartialFileShouldLaunch_WhenPidFileIsHeldExclusively_ShouldReturnFalseWithoutThrowingdotnet test: 31/31 passed on Linux (net10.0), three runs in a row with no flakes. The build is clean on every target framework with the repo's analyzers, including a CA1508 finding fixed along the way.MoveFileExreplace failing while a reader has the file open) is covered by the retry logic, but I have only run it on Linux. The race tests tolerate sharing violations and don't count them as partial reads, so they should hold on the Windows and macOS CI legs.This PR merges cleanly with #185 (for #180) in either order. I checked this with a local
git merge-tree.🤖 Generated with Claude Code
https://claude.ai/code/session_018x3sGQTDEmvXP1cRdbmmkx
Generated by Claude Code