fix: address PR review - add file locking, fix path quoting, fix line endings, format with ruff

- Add msvcrt.locking in _cleanup_pid() to prevent race conditions
  when multiple nodes exit concurrently (Windows file lock)
- Use 'usebackq' and quote %~dp0 path in for /f loop so kill script
  works in directories with spaces/parentheses
- Open kill script with newline='' to prevent double-CR (\r\r\n)
- Improve test_import_registers_pid_on_windows to actually test
  import-time side effect by forcing module reimport
- Run ruff format on test_concore.py and test_read_status.py

Author: GaneshPatil7517

concore.py

@@ -11,14 +11,14 @@
 
 import concore_base
 
-logger = logging.getLogger('concore')
+logger = logging.getLogger("concore")
 logger.addHandler(logging.NullHandler())
 
-#these lines mute the noisy library
-logging.getLogger('matplotlib').setLevel(logging.WARNING)
-logging.getLogger('PIL').setLevel(logging.WARNING)
-logging.getLogger('urllib3').setLevel(logging.WARNING) 
-logging.getLogger('requests').setLevel(logging.WARNING) 
+# these lines mute the noisy library
+logging.getLogger("matplotlib").setLevel(logging.WARNING)
+logging.getLogger("PIL").setLevel(logging.WARNING)
+logging.getLogger("urllib3").setLevel(logging.WARNING)
+logging.getLogger("requests").setLevel(logging.WARNING)
 
 
 # ===================================================================
@@ -51,28 +51,40 @@ def _register_pid():
 
 
 def _cleanup_pid():
-    """Remove the current process PID from the registry on exit."""
+    """Remove the current process PID from the registry on exit.
+
+    Uses file locking on Windows (msvcrt.locking) to prevent race
+    conditions when multiple nodes exit concurrently.
+    """
     pid = str(os.getpid())
     try:
         if not os.path.exists(_PID_REGISTRY_FILE):
             return
-        with open(_PID_REGISTRY_FILE, "r") as f:
+        with open(_PID_REGISTRY_FILE, "r+") as f:
+            # Acquire an exclusive lock on Windows to prevent concurrent
+            # read-modify-write races between exiting nodes.
+            if hasattr(sys, "getwindowsversion"):
+                import msvcrt
+
+                msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1)
             pids = [line.strip() for line in f if line.strip()]
-        remaining = [p for p in pids if p != pid]
-        if remaining:
-            with open(_PID_REGISTRY_FILE, "w") as f:
+            remaining = [p for p in pids if p != pid]
+            if remaining:
+                f.seek(0)
+                f.truncate()
                 for p in remaining:
                     f.write(p + "\n")
-        else:
-            # No PIDs left — clean up both files
-            try:
-                os.remove(_PID_REGISTRY_FILE)
-            except OSError:
-                pass
-            try:
-                os.remove(_KILL_SCRIPT_FILE)
-            except OSError:
-                pass
+            else:
+                f.close()
+                # No PIDs left — clean up both files
+                try:
+                    os.remove(_PID_REGISTRY_FILE)
+                except OSError:
+                    pass
+                try:
+                    os.remove(_KILL_SCRIPT_FILE)
+                except OSError:
+                    pass
     except OSError:
         pass  # Non-fatal: best-effort cleanup
 
@@ -88,28 +100,34 @@ def _write_kill_script():
     """
     try:
         script = "@echo off\r\n"
-        script += "if not exist \"%~dp0" + _PID_REGISTRY_FILE + "\" (\r\n"
+        script += 'if not exist "%~dp0' + _PID_REGISTRY_FILE + '" (\r\n'
         script += "    echo No PID registry found. Nothing to kill.\r\n"
         script += "    exit /b 0\r\n"
         script += ")\r\n"
-        script += "for /f \"tokens=*\" %%p in (%~dp0" + _PID_REGISTRY_FILE + ") do (\r\n"
-        script += "    tasklist /FI \"PID eq %%p\" 2>nul | find /i \"python\" >nul\r\n"
+        script += (
+            'for /f "usebackq tokens=*" %%p in ("%~dp0'
+            + _PID_REGISTRY_FILE
+            + '") do (\r\n'
+        )
+        script += '    tasklist /FI "PID eq %%p" 2>nul | find /i "python" >nul\r\n'
         script += "    if not errorlevel 1 (\r\n"
         script += "        echo Killing Python process %%p\r\n"
         script += "        taskkill /F /PID %%p >nul 2>&1\r\n"
         script += "    ) else (\r\n"
-        script += "        echo Skipping PID %%p - not a Python process or not running\r\n"
+        script += (
+            "        echo Skipping PID %%p - not a Python process or not running\r\n"
+        )
         script += "    )\r\n"
         script += ")\r\n"
-        script += "del /q \"%~dp0" + _PID_REGISTRY_FILE + "\" 2>nul\r\n"
-        script += "del /q \"%~dp0" + _KILL_SCRIPT_FILE + "\" 2>nul\r\n"
-        with open(_KILL_SCRIPT_FILE, "w") as f:
+        script += 'del /q "%~dp0' + _PID_REGISTRY_FILE + '" 2>nul\r\n'
+        script += 'del /q "%~dp0' + _KILL_SCRIPT_FILE + '" 2>nul\r\n'
+        with open(_KILL_SCRIPT_FILE, "w", newline="") as f:
             f.write(script)
     except OSError:
         pass  # Non-fatal: best-effort script generation
 
 
-if hasattr(sys, 'getwindowsversion'):
+if hasattr(sys, "getwindowsversion"):
     _register_pid()
     _write_kill_script()
     atexit.register(_cleanup_pid)
@@ -125,17 +143,19 @@ def _write_kill_script():
 
 last_read_status = "SUCCESS"
 
-s = ''
-olds = ''
+s = ""
+olds = ""
 delay = 1
 retrycount = 0
-inpath = "./in" #must be rel path for local
+inpath = "./in"  # must be rel path for local
 outpath = "./out"
 simtime = 0
 
+
 def _port_path(base, port_num):
     return base + str(port_num)
 
+
 concore_params_file = os.path.join(_port_path(inpath, 1), "concore.params")
 concore_maxtime_file = os.path.join(_port_path(inpath, 1), "concore.maxtime")
 
@@ -145,16 +165,19 @@ def _port_path(base, port_num):
 
 _mod = sys.modules[__name__]
 
+
 # ===================================================================
 # ZeroMQ Communication Wrapper
 # ===================================================================
 def init_zmq_port(port_name, port_type, address, socket_type_str):
     concore_base.init_zmq_port(_mod, port_name, port_type, address, socket_type_str)
 
+
 def terminate_zmq():
     """Clean up all ZMQ sockets and contexts before exit."""
     concore_base.terminate_zmq(_mod)
 
+
 def signal_handler(sig, frame):
     """Handle interrupt signals gracefully."""
     print(f"\nReceived signal {sig}, shutting down gracefully...")
@@ -165,20 +188,23 @@ def signal_handler(sig, frame):
     concore_base.terminate_zmq(_mod)
     sys.exit(0)
 
+
 # Register cleanup handlers
 atexit.register(terminate_zmq)
-signal.signal(signal.SIGINT, signal_handler)   # Handle Ctrl+C
-if not hasattr(sys, 'getwindowsversion'):
+signal.signal(signal.SIGINT, signal_handler)  # Handle Ctrl+C
+if not hasattr(sys, "getwindowsversion"):
     signal.signal(signal.SIGTERM, signal_handler)  # Handle termination (Unix only)
 
 params = concore_base.load_params(concore_params_file)
 
-#9/30/22
+
+# 9/30/22
 def tryparam(n, i):
     """Return parameter `n` from params dict, else default `i`."""
     return params.get(n, i)
 
-#9/12/21
+
+# 9/12/21
 # ===================================================================
 # Simulation Time Handling
 # ===================================================================
@@ -187,12 +213,15 @@ def default_maxtime(default):
     global maxtime
     maxtime = safe_literal_eval(concore_maxtime_file, default)
 
+
 default_maxtime(100)
 
+
 def unchanged():
     """Check if global string `s` is unchanged since last call."""
     return concore_base.unchanged(_mod)
 
+
 # ===================================================================
 # I/O Handling (File + ZMQ)
 # ===================================================================
@@ -228,5 +257,6 @@ def read(port_identifier, name, initstr_val):
 def write(port_identifier, name, val, delta=0):
     concore_base.write(_mod, port_identifier, name, val, delta)
 
-def initval(simtime_val_str): 
+
+def initval(simtime_val_str):
     return concore_base.initval(_mod, simtime_val_str)

tests/test_concore.py

@@ -8,6 +8,7 @@
 # PID Registry Tests (Issue #391)
 # ===================================================================
 
+
 class TestPidRegistry:
     """Tests for the Windows PID registry mechanism that replaces the
     old single-overwrite concorekill.bat approach."""
@@ -21,6 +22,7 @@ def use_temp_dir(self, temp_dir, monkeypatch):
     def test_register_pid_creates_registry_file(self):
         """_register_pid should create concorekill_pids.txt with current PID."""
         from concore import _register_pid, _PID_REGISTRY_FILE
+
         _register_pid()
         assert os.path.exists(_PID_REGISTRY_FILE)
         with open(_PID_REGISTRY_FILE) as f:
@@ -30,6 +32,7 @@ def test_register_pid_creates_registry_file(self):
     def test_register_pid_appends_not_overwrites(self):
         """Multiple calls to _register_pid should append, not overwrite."""
         from concore import _register_pid, _PID_REGISTRY_FILE
+
         # Simulate two different PIDs by writing manually then registering
         with open(_PID_REGISTRY_FILE, "w") as f:
             f.write("11111\n")
@@ -45,6 +48,7 @@ def test_register_pid_appends_not_overwrites(self):
     def test_cleanup_pid_removes_current_pid(self):
         """_cleanup_pid should remove only the current PID from the registry."""
         from concore import _cleanup_pid, _PID_REGISTRY_FILE
+
         current_pid = str(os.getpid())
         with open(_PID_REGISTRY_FILE, "w") as f:
             f.write("99999\n")
@@ -61,6 +65,7 @@ def test_cleanup_pid_deletes_files_when_last_pid(self):
         """When the current PID is the only one left, cleanup should
         remove both the registry file and the kill script."""
         from concore import _cleanup_pid, _PID_REGISTRY_FILE, _KILL_SCRIPT_FILE
+
         current_pid = str(os.getpid())
         with open(_PID_REGISTRY_FILE, "w") as f:
             f.write(current_pid + "\n")
@@ -74,12 +79,14 @@ def test_cleanup_pid_deletes_files_when_last_pid(self):
     def test_cleanup_pid_handles_missing_registry(self):
         """_cleanup_pid should not crash when registry file doesn't exist."""
         from concore import _cleanup_pid, _PID_REGISTRY_FILE
+
         assert not os.path.exists(_PID_REGISTRY_FILE)
         _cleanup_pid()  # Should not raise
 
     def test_write_kill_script_generates_bat_file(self):
         """_write_kill_script should create concorekill.bat with validation logic."""
         from concore import _write_kill_script, _KILL_SCRIPT_FILE, _PID_REGISTRY_FILE
+
         _write_kill_script()
         assert os.path.exists(_KILL_SCRIPT_FILE)
         with open(_KILL_SCRIPT_FILE) as f:
@@ -94,6 +101,7 @@ def test_write_kill_script_generates_bat_file(self):
     def test_multi_node_registration(self):
         """Simulate 3 nodes registering PIDs — all should be present."""
         from concore import _register_pid, _PID_REGISTRY_FILE
+
         fake_pids = ["1204", "1932", "8120"]
         with open(_PID_REGISTRY_FILE, "w") as f:
             for pid in fake_pids:
@@ -109,6 +117,7 @@ def test_multi_node_registration(self):
     def test_cleanup_preserves_other_pids(self):
         """After cleanup, only the current process PID should be removed."""
         from concore import _cleanup_pid, _PID_REGISTRY_FILE
+
         current_pid = str(os.getpid())
         other_pids = ["1111", "2222", "3333"]
         with open(_PID_REGISTRY_FILE, "w") as f:
@@ -123,17 +132,30 @@ def test_cleanup_preserves_other_pids(self):
         for pid in other_pids:
             assert pid in pids
 
-    @pytest.mark.skipif(not hasattr(sys, 'getwindowsversion'),
-                        reason="Windows-only test")
+    @pytest.mark.skipif(
+        not hasattr(sys, "getwindowsversion"), reason="Windows-only test"
+    )
     def test_import_registers_pid_on_windows(self):
-        """On Windows, importing concore should register the PID."""
-        from concore import _PID_REGISTRY_FILE
-        # The import already happened, so just verify the registry exists
-        # in our temp dir (we can't easily test the import-time side effect
-        # since concore was already imported — we test the functions directly)
-        from concore import _register_pid
-        _register_pid()
-        assert os.path.exists(_PID_REGISTRY_FILE)
+        """On Windows, importing concore should register the PID.
+
+        We force a fresh import by removing the cached module so that
+        the module-level registration code runs inside our temp dir.
+        """
+        import importlib
+
+        # Remove cached modules so re-import triggers module-level code
+        for mod_name in ("concore", "concore_base"):
+            sys.modules.pop(mod_name, None)
+
+        import concore  # noqa: F811 – intentional reimport
+
+        assert os.path.exists(concore._PID_REGISTRY_FILE)
+        with open(concore._PID_REGISTRY_FILE) as f:
+            pids = [line.strip() for line in f if line.strip()]
+        assert str(os.getpid()) in pids
+
+        # Restore module for other tests
+        importlib.reload(concore)
 
 
 class TestSafeLiteralEval: