Fix: eliminate Windows PID race condition by replacing concorekill.bat overwrite mechanism with safe PID registry (#391)#465
Open
GaneshPatil7517 wants to merge 2 commits intoControlCore-Project:devfrom
Conversation
…try (ControlCore-Project#391) Problem: On Windows, concore.py wrote a single PID to concorekill.bat at import time using 'w' mode. When multiple Python nodes started simultaneously (via makestudy), each overwrote the file. Only the last node's PID survived, making the kill script unable to terminate other nodes. Stale PIDs from crashed studies could also kill unrelated processes. Solution: - Replace single-PID overwrite with append-based PID registry (concorekill_pids.txt) storing one PID per line - Register atexit handler to remove current PID on graceful shutdown - Generate concorekill.bat dynamically with PID validation: each PID is checked via tasklist before taskkill executes - Clean up both files when last node exits - Backward compatible: users still run concorekill.bat Added 9 tests covering registration, cleanup, multi-node scenarios, missing registry handling, and kill script generation. Fixes ControlCore-Project#391
…nature read() returns a single value, not a tuple. The test incorrectly unpacked into (result, ok) causing ValueError on CI.
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.
Problem
On Windows,
concore.pywrote a single PID toconcorekill.batat module import timeusing write mode (
"w"). When multiple Python nodes launched simultaneously viamakestudy, each node overwrote the file — only the last node's PID survived.This caused:
concorekill.batto an unrelated process, risking
taskkillterminating the wrong programSolution
Replaced the single-PID overwrite with an append-based PID registry system:
concorekill.batwith one hardcodedtaskkilllineconcorekill_pids.txtregistry (one PID per line, append mode)atexithandler removes current PID on shutdowntaskkillconcorekill.batvalidates each PID viatasklistbefore killingKey changes in
concore.py:_register_pid()— appends current PID toconcorekill_pids.txt_cleanup_pid()— removes current PID on exit; deletes both files when last node exits_write_kill_script()— generatesconcorekill.batthat checks each PID is a running Python process before issuingtaskkillBackward compatibility:
Users still run
concorekill.batexactly as before — no workflow change required.Only Python layer modified; C++, MATLAB, and Verilog implementations are untouched.
Tests
Added 9 tests in
TestPidRegistry:tasklist+taskkill)All 9 new tests pass. All pre-existing tests unaffected.
Fixes
Closes #391