Skip terminate tests on Windows (os.killpg/getpgid are POSIX-only)

- Code now falls back to process.terminate() on Windows
- Tests use create=True to mock POSIX-only functions on Windows
- Only integration test with real subprocess skipped on Windows

Author: janosh

src/custodian/vasp/jobs.py

@@ -712,31 +712,32 @@ def constrained_opt_run(
     def terminate(self, directory="./") -> None:
         """Kill all VASP processes associated with the current job.
 
-        Sends SIGTERM to the entire process group to ensure all MPI workers
-        are terminated, not just the parent process (srun/mpirun). Falls back
-        to SIGKILL if processes don't terminate gracefully within timeout.
+        On POSIX, sends SIGTERM/SIGKILL to entire process group to ensure all
+        MPI workers are terminated. On Windows, falls back to process.terminate().
         """
         pid = self._vasp_process.pid
 
-        # Check if process already finished
         if self._vasp_process.poll() is not None:
             logger.warning(f"Process {pid} already terminated")
             return
 
-        # Get process group ID
-        try:
-            pgid = os.getpgid(pid)
-        except ProcessLookupError:
-            logger.warning(f"Process {pid} not found (already dead)")
-            return
-
-        # Send SIGTERM to process group for graceful shutdown
-        logger.info(f"Sending SIGTERM to process group {pgid}")
-        try:
-            os.killpg(pgid, signal.SIGTERM)
-        except ProcessLookupError:
-            logger.warning(f"Process group {pgid} not found (already dead)")
-            return
+        # POSIX: kill entire process group; Windows: just terminate parent
+        use_pgid = hasattr(os, "killpg")
+        if use_pgid:
+            try:
+                pgid = os.getpgid(pid)
+            except ProcessLookupError:
+                logger.warning(f"Process {pid} not found (already dead)")
+                return
+            logger.info(f"Sending SIGTERM to process group {pgid}")
+            try:
+                os.killpg(pgid, signal.SIGTERM)
+            except ProcessLookupError:
+                logger.warning(f"Process group {pgid} not found (already dead)")
+                return
+        else:
+            logger.info(f"Terminating process {pid}")
+            self._vasp_process.terminate()
 
         # Wait for graceful termination
         try:
@@ -746,12 +747,13 @@ def terminate(self, directory="./") -> None:
         except subprocess.TimeoutExpired:
             pass
 
-        # Force kill with SIGKILL
-        logger.warning(f"SIGTERM timeout ({self.terminate_timeout}s), sending SIGKILL to process group {pgid}")
-        try:
-            os.killpg(pgid, signal.SIGKILL)
-        except ProcessLookupError:
-            pass
+        # Force kill
+        logger.warning(f"Timeout ({self.terminate_timeout}s), force killing {pid}")
+        if use_pgid:
+            try:
+                os.killpg(pgid, signal.SIGKILL)  # type: ignore[possibly-undefined]
+            except ProcessLookupError:
+                pass
         self._vasp_process.kill()
         self._vasp_process.wait()
         logger.info(f"Process {pid} force-killed")

tests/vasp/test_jobs.py

@@ -3,6 +3,7 @@
 import shutil
 import signal
 import subprocess
+import sys
 from glob import glob
 from types import SimpleNamespace
 from typing import TYPE_CHECKING
@@ -227,7 +228,7 @@ def test_gamma_checks(self) -> None:
 
 
 class TestVaspJobTerminate:
-    """Tests for VaspJob.terminate() process group termination."""
+    """Tests for VaspJob.terminate() - cross-platform with POSIX process group support."""
 
     @pytest.fixture
     def mocks(self) -> "Generator[SimpleNamespace, None, None]":
@@ -238,8 +239,8 @@ def mocks(self) -> "Generator[SimpleNamespace, None, None]":
 
         with (
             patch("custodian.vasp.jobs.logger") as logger,
-            patch("os.killpg") as killpg,
-            patch("os.getpgid", return_value=67890),
+            patch("os.killpg", create=True) as killpg,
+            patch("os.getpgid", return_value=67890, create=True),
         ):
             yield SimpleNamespace(job=job, process=process, logger=logger, killpg=killpg)
 
@@ -259,7 +260,7 @@ def test_graceful_sigterm(self, mocks: SimpleNamespace) -> None:
         mocks.logger.info.assert_any_call("Sending SIGTERM to process group 67890")
         mocks.logger.info.assert_any_call("Process 12345 terminated gracefully")
         mocks.killpg.assert_called_once_with(67890, signal.SIGTERM)
-        mocks.process.wait.assert_called_once_with(timeout=10.0)  # default timeout
+        mocks.process.wait.assert_called_once_with(timeout=10.0)
         mocks.process.kill.assert_not_called()
 
     def test_force_kill_after_timeout(self, mocks: SimpleNamespace) -> None:
@@ -268,7 +269,7 @@ def test_force_kill_after_timeout(self, mocks: SimpleNamespace) -> None:
         mocks.process.wait.side_effect = [subprocess.TimeoutExpired("vasp", 10), None]
         mocks.job.terminate()
 
-        mocks.logger.warning.assert_called_with("SIGTERM timeout (10.0s), sending SIGKILL to process group 67890")
+        mocks.logger.warning.assert_called_with("Timeout (10.0s), force killing 12345")
         mocks.logger.info.assert_any_call("Process 12345 force-killed")
         assert mocks.killpg.call_count == 2
         mocks.process.kill.assert_called_once()
@@ -281,12 +282,12 @@ def test_custom_timeout(self, mocks: SimpleNamespace) -> None:
         mocks.job.terminate()
 
         mocks.process.wait.assert_any_call(timeout=60.0)
-        mocks.logger.warning.assert_called_with("SIGTERM timeout (60.0s), sending SIGKILL to process group 67890")
+        mocks.logger.warning.assert_called_with("Timeout (60.0s), force killing 12345")
 
     def test_process_not_found_on_getpgid(self, mocks: SimpleNamespace) -> None:
         """ProcessLookupError when getting PGID."""
         mocks.process.poll.return_value = None
-        with patch("os.getpgid", side_effect=ProcessLookupError):
+        with patch("os.getpgid", side_effect=ProcessLookupError, create=True):
             mocks.job.terminate()
 
         mocks.logger.warning.assert_called_with("Process 12345 not found (already dead)")
@@ -300,10 +301,11 @@ def test_process_group_not_found_on_sigterm(self, mocks: SimpleNamespace) -> Non
 
         mocks.logger.warning.assert_called_with("Process group 67890 not found (already dead)")
 
+    @pytest.mark.skipif(sys.platform == "win32", reason="sleep command and PGID not available")
     def test_integration_with_real_process(self) -> None:
-        """Integration test with real subprocess."""
+        """Integration test with real subprocess (POSIX only)."""
         vasp_job = VaspJob.__new__(VaspJob)
-        vasp_job.terminate_timeout = 10.0  # Set since __new__ bypasses __init__
+        vasp_job.terminate_timeout = 10.0
         real_process = subprocess.Popen(["sleep", "30"], start_new_session=True)
         vasp_job._vasp_process = real_process
         original_pgid = os.getpgid(real_process.pid)