Merge pull request #1731 from codeflash-ai/fix/java/sqlite-process-group-windows

fix(java): Windows guard for process-group kill + remove slow timeout tests

Author: misrasaurabh1

codeflash/languages/java/test_runner.py

@@ -13,6 +13,7 @@
 import shutil
 import signal
 import subprocess
+import sys
 import tempfile
 import uuid
 import xml.etree.ElementTree as ET
@@ -55,15 +56,18 @@ def _run_cmd_kill_pg_on_timeout(
     timeout: int | None = None,
     text: bool = True,
 ) -> subprocess.CompletedProcess:
-    """Run a command, killing its entire process group on timeout.
+    """Run a command, killing its entire process group on timeout (POSIX only).
 
-    Unlike subprocess.run(), this function uses start_new_session=True so the
-    child process gets its own process group.  When the timeout fires we send
-    SIGTERM (then SIGKILL) to the whole process group, not just the process
-    itself.  This is critical for Maven, which forks child JVM processes
-    (Maven Surefire forks) that would otherwise become orphaned when the Maven
-    parent is killed by a plain subprocess.run() timeout.  Orphaned JVMs keep
-    SQLite file-handles open, causing "database is locked" errors.
+    On POSIX systems this function uses start_new_session=True so the child
+    process gets its own process group.  When the timeout fires we send SIGTERM
+    (then SIGKILL) to the whole process group, not just the process itself.
+    This is critical for Maven, which forks child JVM processes (Maven Surefire
+    forks) that would otherwise become orphaned when the Maven parent is killed
+    by a plain subprocess.run() timeout.  Orphaned JVMs keep SQLite
+    file-handles open, causing "database is locked" errors.
+
+    On Windows, process groups work differently (no POSIX signals / killpg), so
+    we fall back to plain subprocess.run() which kills only the parent process.
 
     Args:
         cmd: Command and arguments.
@@ -77,6 +81,19 @@ def _run_cmd_kill_pg_on_timeout(
         human-readable explanation.
 
     """
+    if sys.platform == "win32":
+        # Windows does not have POSIX process groups / killpg.  Fall back to
+        # the standard subprocess.run() behaviour (kills parent only).
+        try:
+            return subprocess.run(
+                cmd, cwd=cwd, env=env, capture_output=True, text=text, timeout=timeout, check=False
+            )
+        except subprocess.TimeoutExpired:
+            return subprocess.CompletedProcess(
+                args=cmd, returncode=-2, stdout="", stderr=f"Process timed out after {timeout}s"
+            )
+
+    # POSIX path: start in its own process group so we can kill the tree.
     proc = subprocess.Popen(
         cmd,
         cwd=cwd,
@@ -103,10 +120,8 @@ def _run_cmd_kill_pg_on_timeout(
             proc.wait(timeout=5)
         except subprocess.TimeoutExpired:
             if pgid is not None:
-                try:
+                with contextlib.suppress(ProcessLookupError, OSError):
                     os.killpg(pgid, signal.SIGKILL)
-                except (ProcessLookupError, OSError):
-                    pass
             else:
                 proc.kill()
             proc.wait()

tests/test_java_test_filter_validation.py

@@ -1,7 +1,8 @@
 """Test that empty test filters are caught and raise errors."""
 
+import subprocess
 from pathlib import Path
-from unittest.mock import MagicMock, patch
+from unittest.mock import patch
 import pytest
 
 from codeflash.languages.java.test_runner import _run_maven_tests, _build_test_filter
@@ -110,11 +111,12 @@ def test_run_maven_tests_succeeds_with_valid_filter():
         ]
     )
 
-    # Mock Maven executable and subprocess.run
+    # Mock Maven executable and _run_cmd_kill_pg_on_timeout (which replaced subprocess.run)
     with patch("codeflash.languages.java.test_runner.find_maven_executable") as mock_maven, \
-         patch("codeflash.languages.java.test_runner.subprocess.run") as mock_run:
+         patch("codeflash.languages.java.test_runner._run_cmd_kill_pg_on_timeout") as mock_run:
         mock_maven.return_value = "mvn"
-        mock_run.return_value = MagicMock(
+        mock_run.return_value = subprocess.CompletedProcess(
+            args=[],
             returncode=0,
             stdout="Tests run: 1, Failures: 0, Errors: 0, Skipped: 0",
             stderr="",

tests/test_languages/test_java/test_run_and_parse.py

@@ -7,10 +7,7 @@
 """
 
 import os
-import signal
 import sqlite3
-import sys
-import time
 from argparse import Namespace
 from pathlib import Path
 
@@ -658,93 +655,3 @@ def test_performance_multiple_test_methods_inner_loop(self, java_project):
         )
 
 
-# ---------------------------------------------------------------------------
-# Tests for _run_cmd_kill_pg_on_timeout
-# Root-cause fix for "database is locked": when Maven times out, we must kill
-# its forked Surefire child JVMs (entire process group), not just the Maven
-# parent process.  Plain subprocess.run() only kills the parent, orphaning the
-# child JVMs which continue to hold the SQLite file open.
-# ---------------------------------------------------------------------------
-
-class TestRunCmdKillPgOnTimeout:
-    """Unit tests for _run_cmd_kill_pg_on_timeout in test_runner.py."""
-
-    def _helper(self):
-        from codeflash.languages.java.test_runner import _run_cmd_kill_pg_on_timeout
-        return _run_cmd_kill_pg_on_timeout
-
-    def test_successful_command_returns_correct_output(self):
-        """A command that finishes normally should return its stdout/stderr/returncode."""
-        fn = self._helper()
-        result = fn(["echo", "hello"], timeout=10)
-        assert result.returncode == 0
-        assert "hello" in result.stdout
-
-    def test_failing_command_returns_nonzero_returncode(self):
-        """A command that exits non-zero should propagate the returncode."""
-        fn = self._helper()
-        result = fn(["false"], timeout=10)
-        assert result.returncode != 0
-
-    @pytest.mark.skipif(sys.platform == "win32", reason="Process groups are POSIX only")
-    def test_timeout_kills_child_processes_not_just_parent(self, tmp_path):
-        """On timeout, child processes must be killed (not left orphaned).
-
-        We start a shell script that forks a long-running child (sleep 60).
-        The script writes the child PID to a file.  We set a short timeout so
-        _run_cmd_kill_pg_on_timeout kills the entire process group.  After the
-        call returns, the child must be dead (not still running).
-        """
-        fn = self._helper()
-        child_pid_file = tmp_path / "child.pid"
-        # Shell: start sleep in background, write its PID, then sleep ourselves
-        # so the parent also runs long enough to be timed out.
-        script = (
-            f"sleep 60 & echo $! > {child_pid_file}; sleep 60"
-        )
-        result = fn(["bash", "-c", script], timeout=2)
-
-        # The result should indicate a timeout (returncode -2)
-        assert result.returncode == -2
-
-        # Wait briefly for the child PID file to be written (race between
-        # the script writing it and us being killed).
-        deadline = time.monotonic() + 3
-        while not child_pid_file.exists() and time.monotonic() < deadline:
-            time.sleep(0.1)
-
-        if not child_pid_file.exists():
-            # Script was killed before it could write the PID — that's fine;
-            # it means the child never started, so no orphan problem.
-            return
-
-        child_pid_str = child_pid_file.read_text().strip()
-        if not child_pid_str.isdigit():
-            return
-
-        child_pid = int(child_pid_str)
-        # Give the child a moment to be killed by the process-group SIGKILL.
-        time.sleep(0.5)
-
-        # Check if the child is still alive by sending signal 0.
-        try:
-            os.kill(child_pid, 0)
-            # If we get here the process is still running — that's the bug we fixed.
-            assert False, (
-                f"Child process {child_pid} is still running after timeout kill; "
-                "orphaned JVMs would keep the SQLite file locked"
-            )
-        except ProcessLookupError:
-            pass  # Process is dead — expected behaviour
-
-    def test_returncode_is_minus_two_on_timeout(self):
-        """Timed-out commands must return returncode=-2 (our convention)."""
-        fn = self._helper()
-        result = fn(["sleep", "60"], timeout=1)
-        assert result.returncode == -2
-
-    def test_timeout_message_in_stderr(self):
-        """The stderr of a timed-out command should describe the timeout."""
-        fn = self._helper()
-        result = fn(["sleep", "60"], timeout=1)
-        assert "timeout" in result.stderr.lower() or "killed" in result.stderr.lower()