fix: confirm process identity for legacy PID files [patch] - #168
Merged
Merged
Conversation
The legacy plain-integer PID path treated any process holding the stored PID as a live instance, so a stale legacy PID file left behind by an abnormal exit made the application refuse to start once the operating system recycled that PID onto an unrelated process. This is the same PID-recycling failure the JSON format already guards against. The legacy path now resolves the PID and compares the running process's name against the current process's name before reporting another instance, and treats the PID as stale when identity cannot be confirmed. The existing legacy test pinned the weaker behaviour, so it now asserts that an unrelated running process is not mistaken for an instance, with a new test covering a running process that does carry this application's name. Fixes #167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EVRMVaSnFGJQBKLpYbMZ9
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Addresses the CodeQL "missed using opportunity" review comment on the manual try/finally disposal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EVRMVaSnFGJQBKLpYbMZ9
Windows CI showed ShouldLaunch_WhenAlreadyRunning_ShouldReturnFalse failing: it described a helper process in the JSON format, which depends on MainModule resolving identically for a freshly started child. That arrangement now uses the same legacy lookalike process the other legacy test uses, which passes on all three platforms. Running the tests in Release then exposed a real gap in the fix: Linux reports another process's name truncated to 15 characters while the current process reports its full name, so a genuine second instance of an application with a longer name would no longer be detected. The legacy check now accepts a name truncated at exactly that limit when it prefixes the current process's name, and the test that looks for an unrelated process skips any candidate whose name prefixes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012EVRMVaSnFGJQBKLpYbMZ9
|
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
2 participants
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 #167
What was wrong
IsAlreadyRunning()has two code paths. The JSON path (CheckPidFileContents→IsStoredProcessRunning) verifies PID and process name and main module before concluding another instance is running — the fix shipped for #10 / #11. The legacy plain-integer path (HandleLegacyPidFile→IsProcessRunning) only checked whether any process held that PID.So a stale legacy PID file (left by a crash or
kill -9, or a JSON file truncated to digits by a partial write) made the application refuse to start as soon as the OS recycled that PID onto an unrelated process — the exact PID-recycling failure #10 reported, reached through the legacy fallback instead.The change
HandleLegacyPidFilenow callsIsLegacyProcessRunning, which resolves the PID and compares the running process's name against the current process's name before reporting another instance. The legacy format carries no name of its own, so the current process's name is the only identity available to compare against. When identity cannot be confirmed (process gone, exited, or details inaccessible) the PID is treated as stale and the application launches, matching the conservative shape ofIsStoredProcessRunningByName. The now-unusedIsProcessRunningwas removed.The comparison goes through
IsSameApplicationName, which also accepts a name truncated at exactly 15 characters when it prefixes the current process's name. Linux stores at most 15 characters of a process name, and .NET reports another process's name from that truncated value while the current process reports its full name — without this, a genuine second instance of an app whose name is longer than 15 characters would no longer be detected on Linux. Running the suite in Release surfaced exactly that case (ktsu.SingleAppInstance.Testvsktsu.SingleAppI).No public API change; the JSON path is untouched.
Tests
IsAlreadyRunning_WithLegacyPidFile_RunningProcess_ShouldReturnTruepinned the buggy behaviour (it assertedtruefor any other running process's PID in legacy format). It is replaced byIsAlreadyRunning_WithLegacyPidFile_RunningProcessWithDifferentName_ShouldReturnFalse, mirroringIsAlreadyRunning_WithRunningProcessButDifferentName_ShouldReturnFalseon the JSON path. This test fails onmainand passes with the fix — verified by restoringmain'sSingleAppInstance.csand re-running:Assertion failed ... Expected condition to be false.IsAlreadyRunning_WithLegacyPidFile_RunningProcessWithSameName_ShouldReturnTruecovers the positive legacy path, so the fix cannot degrade into "always return false". It launches a long-lived helper process named after the current process (a copy ofsleep/ping.exeunder a temp name) and asserts detection still happens, including through the truncated-name case on Linux.ShouldLaunch_WhenAlreadyRunning_ShouldReturnFalserelied on the old legacy behaviour to force a detection, so it now uses that same lookalike helper. Its first arrangement described the helper in JSON format instead and failed on Windows CI, whereMainModulefor a freshly started child does not resolve to a matching path; the legacy lookalike is green on ubuntu, windows and macos.Local:
dotnet test26/26 in both Debug and Release,dotnet build -c Release0 warnings / 0 errors.Note for a follow-up (not changed here)
The JSON path compares the stored
ProcessName(written full, byWritePidFile) against the running process's name (read truncated on Linux), so on Linux it has the same blind spot for applications whose process name exceeds 15 characters. That predates this PR and is worth its own issue rather than widening this one.README's backward-compatibility bullet was updated to say the legacy path confirms the process name.
🤖 Generated with Claude Code
https://claude.ai/code/session_012EVRMVaSnFGJQBKLpYbMZ9