fix: replace concorekill.bat single-PID overwrite with safe PID registry (#391)#489
Open
GaneshPatil7517 wants to merge 2 commits intoControlCore-Project:devfrom
Open
Conversation
…try (ControlCore-Project#391) On Windows, concore.py overwrote concorekill.bat at import time with a single PID, creating race conditions when multiple nodes launched simultaneously and leaving stale PIDs after crashes. Replace with append-based PID registry (concorekill_pids.txt): - _register_pid(): appends current PID (append mode, no overwrite) - _cleanup_pid(): removes current PID on exit with file locking - _write_kill_script(): generates concorekill.bat that validates each PID via tasklist before issuing taskkill Users still run concorekill.bat as before (backward compatible).
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows race conditions and stale-PID hazards in concorekill.bat by replacing the single-PID overwrite behavior with a shared PID registry and a generated kill script that validates PIDs before terminating processes.
Changes:
- Add a PID registry (
concorekill_pids.txt) plus_register_pid(),_cleanup_pid(), and_write_kill_script()to manage multi-node termination on Windows. - Generate
concorekill.batto iterate over registered PIDs and validate each PID before attemptingtaskkill. - Add a
TestPidRegistrytest suite covering PID registry and script generation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
concore.py |
Implements PID registry + cleanup + generated Windows kill script; hooks cleanup via atexit. |
tests/test_concore.py |
Adds tests for PID registry creation/appending, cleanup semantics, and Windows import-time registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lock range with finally unlock, absolute paths - Kill script uses wmic+concore instead of tasklist+python for precise PID validation - _register_pid() now acquires file lock (same as _cleanup_pid) - Lock range changed from 1 byte to 0x7FFFFFFF with LK_UNLCK in finally block - _PID_REGISTRY_FILE and _KILL_SCRIPT_FILE use os.path.abspath at module init - Tests updated: monkeypatch module-level paths to temp_dir, assert wmic/concore
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.pyoverwroteconcorekill.batat import time with a singlehardcoded PID. When multiple nodes launched simultaneously, each overwrote the file
only the last node's PID survived. Stale PIDs from crashed studies could also kill
unrelated processes.
Solution
Replace the 3-line overwrite block with an append-based PID registry:
_register_pid()- appends current PID toconcorekill_pids.txt(append mode)_cleanup_pid()- removes current PID on exit withmsvcrtfile locking_write_kill_script()- generatesconcorekill.batthat validates each PIDvia
tasklistbefore issuingtaskkillUsers still run
concorekill.batas before - no workflow change.Changes
concore.py: replaced lines 24–30 (theif hasattr(sys, 'getwindowsversion')block)tests/test_concore.py: addedTestPidRegistryclass (9 tests) at end of fileNo formatting changes, no unrelated edits.
Closes #391