Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Open
tboy1337 wants to merge 54 commits into
Open
Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182tboy1337 wants to merge 54 commits into
tboy1337 wants to merge 54 commits into
Conversation
Open
fizyk
reviewed
Sep 4, 2025
Contributor
📝 WalkthroughWalkthroughPostgreSQLExecutor gains Windows-aware start/stop/status handling and new command templates; subprocess calls use argv lists on Windows, Windows-specific ChangesWindows compatibility and executor changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor as PostgreSQLExecutor
participant Subprocess as subprocess
participant PGCtl as pg_ctl
Caller->>Executor: start()
alt Windows
Executor->>Subprocess: run(['pg_ctl','start','-w','-D', datadir, ...], shell=False, env=merged)
Subprocess->>PGCtl: launch pg_ctl
PGCtl-->>Subprocess: exit status
Subprocess-->>Executor: CompletedProcess
Executor-->>Caller: return self or raise on timeout/error
else Unix
Executor->>Subprocess: run(shell_command_string, shell=True, env=merged)
Subprocess->>PGCtl: launch pg_ctl
PGCtl-->>Subprocess: exit status
Subprocess-->>Executor: CompletedProcess
Executor-->>Caller: return self or raise on timeout/error
end
sequenceDiagram
participant Caller
participant Executor as PostgreSQLExecutor
participant Subprocess as subprocess
participant PGCtl as pg_ctl
participant Proc as PostgresProcess
Caller->>Executor: stop(sig=None)
Executor->>Subprocess: check_output(['pg_ctl','stop','-D', datadir, '-m', 'f'], shell=False)
Subprocess->>PGCtl: execute stop
PGCtl-->>Subprocess: exit status
Subprocess-->>Executor: CompletedProcess
alt Windows or fallback (no os.killpg)
Executor->>Proc: _windows_terminate_process(proc)
Proc->>Executor: terminate()
Executor->>Proc: wait(timeout)
alt wait times out
Proc->>Executor: TimeoutExpired
Executor->>Proc: kill()
Proc->>Executor: wait()
end
Executor->>Executor: clear process handle
else Unix (normal)
Executor->>Executor: delegate to TCPExecutor.stop()
end
Executor-->>Caller: stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 263-283: In stop(self, sig: Optional[int] = None, exp_sig:
Optional[int] = None) replace the subprocess.check_output call that builds a
shell string with a list-style invocation (e.g., [self.executable, "stop", "-D",
self.datadir, "-m", "f"]) and remove shell=True so subprocess runs the
executable directly; this avoids breaking paths with spaces and removes the
shell injection risk while keeping the same arguments and behavior before the
platform-specific termination logic (_windows_terminate_process / super().stop).
- Around line 53-66: The _get_base_command constructs the -o string with
unix_socket_directories unquoted which breaks for paths with spaces; update
_get_base_command to detect whitespace in the unixsocketdir variable and wrap
that value in double quotes (or escape quotes) when building the -o option so
the produced fragment becomes -c unix_socket_directories="/path/with space"
(referencing the unix_socket_directories token and BASE_PROC_START_COMMAND
assignment), and add a unit test that passes a datadir/unixsocketdir containing
a space to ensure the command line includes the quoted socket directory (or
alternatively, add explicit validation/documentation and raise a clear error
when whitespace is present).
🧹 Nitpick comments (1)
pytest_postgresql/executor.py (1)
231-262: Tidy up the termination helper API surface.
sigis unused in_windows_terminate_process, and_unix_terminate_processis currently unused. Either wire_unix_terminate_processintostop()for symmetry and usesig, or drop/rename the parameter to avoid dead code and lint noise.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
5d4fb91 to
efa4633
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 35-39: The test assertions in tests/test_windows_compatibility.py
don't match the actual string produced by _get_base_command(): update the
assertions to expect the escaped-quote form for unix_socket_directories (i.e.,
include the backslash-escaped double quotes around {unixsocketdir}) and assert
that the non-quoted form is not present; specifically change the positive
assertion referencing unix_socket_directories to look for
unix_socket_directories=\"{unixsocketdir}\" and keep the negative assertion for
unix_socket_directories='{unixsocketdir}' to ensure consistency with
_get_base_command().
- Around line 189-204: The test's expected_parts entry for
unix_socket_directories doesn't match the actual command string (which includes
escaped quotes), so update the expectation in the test to match the
implementation: replace the "-c unix_socket_directories=C:/temp/socket" entry
with the escaped-quote form that appears in executor.command (e.g., -c
unix_socket_directories=\"C:/temp/socket\"), keeping the rest of the
expected_parts and the loop over executor.command unchanged; reference the test
variable executor.command and the expected_parts list when making the change.
🧹 Nitpick comments (2)
pytest_postgresql/executor.py (2)
25-25: Unused importsignal.The
signalmodule is imported but not used anywhere in the file. This appears to be a leftover from development.🧹 Suggested fix
-import signal
250-269: Good refactor to list-based subprocess call.The change from shell string to list format (
[self.executable, "stop", "-D", self.datadir, "-m", "f"]) properly addresses the previous security concern and handles paths with spaces correctly.One minor observation: the
AttributeErrorfallback (lines 263-268) uses string matching"killpg" in str(e)which is somewhat fragile. Consider whether this fallback is still needed given the explicitplatform.system()check now handles Windows upfront.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
b02c61b to
4a0b22e
Compare
fizyk
reviewed
Jan 23, 2026
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
059e4bc to
54140ac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 226-246: The parameter sig in _windows_terminate_process is
intentionally unused and triggers Ruff ARG002; either rename it to _sig to
indicate it’s intentionally unused or keep sig and add a local lint suppression
(e.g., append a noqa comment for ARG002) where the function is defined to
silence the warning; update the function signature (or add the noqa) in the
_windows_terminate_process definition and run linters to confirm the ARG002
warning is resolved.
fizyk
approved these changes
Jan 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/single-postgres.yml (1)
82-86:⚠️ Potential issue | 🟡 MinorArtifact upload path is Unix-only.
/tmp/pytest-of-runner/**on line 86 won't match anything on Windows. This is gated behindif: failure()so it won't fail the build, but Windows test failure artefacts will be silently lost. Consider adding a platform-conditional path or using${{ runner.temp }}.
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 59-62: The workflow hardcodes POSTGRESQL_EXEC to
/usr/lib/postgresql/... which breaks macOS runners; change the step that sets
POSTGRESQL_EXEC (the block under the if: runner.os != 'Windows') to detect
pg_ctl on Unix-like systems instead of a Debian path — use command -v pg_ctl and
if that fails try macOS Homebrew locations (e.g.,
/opt/homebrew/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl and
/usr/local/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl) or set a fallback
to the current Debian path; ensure the final value exported as POSTGRESQL_EXEC
is the first existing pg_ctl found so CI on macos_postgres_* with
ankane/setup-postgres works correctly.
🧹 Nitpick comments (4)
pytest_postgresql/executor.py (2)
226-246: Broadly swallowing exceptions in cleanup is acceptable but consider logging.The
except (OSError, AttributeError): passon lines 244–246 silently discards allOSErrorvariants (including permission errors) andAttributeError. For a teardown helper this is pragmatic, but a debug-level log would aid troubleshooting when processes fail to terminate.
261-266: String-basedAttributeErrormatching is fragile.Checking
"killpg" in str(e)couples the fallback to the exact wording of CPython's error message, which could change across versions or implementations. Consider catching the missing attribute directly.Proposed fix
except AttributeError as e: - # Fallback for edge cases where os.killpg doesn't exist - if "killpg" in str(e): + # Fallback for edge cases where os.killpg doesn't exist (e.g. Windows) + if not hasattr(os, "killpg"): self._windows_terminate_process(sig) else: raisetests/conftest.py (1)
7-9: Remove the commented-out import rather than leaving dead code.The commented-out line still triggers a Ruff warning (RUF100) for the stale
noqadirective. Since the comment on line 8 already explains the rationale, the old import line adds no value.Proposed fix
# Plugin is registered via entry point in pyproject.toml - no need to import here -# from pytest_postgresql.plugin import * # noqa: F403,F401tests/test_windows_compatibility.py (1)
74-98: Patch target forsubprocess.check_outputshould be where it's looked up.The patch
patch("subprocess.check_output")works becauseexecutor.pyusesimport subprocessand thensubprocess.check_output(...), so patching the module attribute is fine. However, best practice is to patch where the name is used:pytest_postgresql.executor.subprocess.check_output. This avoids accidentally affecting other code if tests are ever run in parallel or the import style changes.This applies to all test methods in this class that patch
subprocess.check_output.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 127-155: The test fails on Unix because os.killpg exists so the
stop() except branch re-raises; to exercise the fallback, remove killpg from the
executor module's os before calling stop: inside the test's with-block, import
sys and call delattr(sys.modules['pytest_postgresql.executor'].os, "killpg") (or
use monkeypatch.delattr on that module's os) so that hasattr(os, "killpg") is
False when PostgreSQLExecutor.stop() runs; keep references to
PostgreSQLExecutor.stop, os.killpg, and _windows_terminate_process to locate the
code to change.
🧹 Nitpick comments (3)
pytest_postgresql/executor.py (2)
32-34: Logger initialisation breaks import grouping.The
logger = logging.getLogger(__name__)statement is sandwiched between twomirakuruimports. Move it after all top-level imports to keep the import block contiguous and PEP 8–compliant.Suggested reordering
from mirakuru import TCPExecutor - -logger = logging.getLogger(__name__) from mirakuru.exceptions import ProcessFinishedWithError from packaging.version import parse from pytest_postgresql.exceptions import ExecutableMissingException, PostgreSQLUnsupported + +logger = logging.getLogger(__name__)
229-254:_windows_terminate_processlooks good overall; minor nit on redundantpass.The graceful-then-force-kill approach is sound. However, the
passon line 254 is redundant — thelogger.debug(...)call already satisfies the except block body.Suggested fix
logger.debug( "Exception during Windows process termination: %s: %s", type(e).__name__, e, ) - passtests/test_windows_compatibility.py (1)
74-98: Windows stop test: consider asserting the exact arguments passed tocheck_output.The test asserts
mock_subprocess.assert_called_once()but doesn't verify the arguments. Sincestop()now uses list-form args ([self.executable, "stop", "-D", self.datadir, "-m", "f"]), asserting the call args would catch regressions in the command construction.Optional enhancement
# Should call pg_ctl stop and Windows terminate - mock_subprocess.assert_called_once() + mock_subprocess.assert_called_once_with( + ["/path/to/pg_ctl", "stop", "-D", "/tmp/data", "-m", "f"], + ) mock_terminate.assert_called_once()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 244-246: Replace the blocking self.process.wait() after
self.process.kill() with a timed wait and a fallback path: call
self.process.wait(timeout=...) (or poll in a short loop) and catch
subprocess.TimeoutExpired, then log a warning and avoid hanging (optionally
attempt a forceful termination or continue without blocking); update the code
around self.process.kill() / self.process.wait() in executor.py to use a timeout
and warning on failure to reap the process.
🧹 Nitpick comments (1)
tests/test_windows_compatibility.py (1)
74-100: Consider a more precise mock assertion for_windows_terminate_process.
mock_terminate.assert_called_once()on line 99 does not verify the arguments. Sincestop()callsself._windows_terminate_process(sig)(wheresigdefaults toNone), usingassert_called_once_with(None)would make the test stricter and catch regressions if the call signature changes.Proposed fix
- mock_terminate.assert_called_once() + mock_terminate.assert_called_once_with(None)
fizyk
approved these changes
Feb 11, 2026
fizyk
reviewed
Feb 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 50-62: The Windows step "Detect PostgreSQL path on Windows"
currently may leave POSTGRESQL_EXEC unset if neither the hardcoded Test-Path
lookup nor Get-Command finds pg_ctl; update the pwsh run block so that after
attempting both Test-Path and Get-Command you check whether $pgPath is set and,
if not, write an explicit error and exit non‑zero (e.g., write-host/error then
exit 1) so downstream steps that rely on POSTGRESQL_EXEC (the env var) fail fast
and consistently rather than receiving an empty --postgresql-exec.
Contributor
Author
|
@coderabbitai resume |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_executor.py`:
- Around line 30-39: The call to retry(...) that invokes psycopg.connect(...)
returns a live connection that is currently discarded; capture that result
(e.g., assign to a variable like conn) and ensure it is explicitly closed
(conn.close()) or used in a context manager so the readiness connection opened
by retry is released; update the test around the retry(...) invocation in
tests/test_executor.py to store the returned connection and close it after the
readiness check to avoid leaving backend sessions open.
- Around line 231-236: Update the Unix branch assertion to verify the full
quoted socket-directory value is present in executor.command rather than just
the flag; use the test's socket_dir (or the variable used to set the path in
this test) to build the expected substring like
unix_socket_directories='{socket_dir}' and assert that exact string is in
executor.command so missing or misquoted paths fail the test; modify the
assertion in tests/test_executor.py to reference socket_dir and check for the
complete quoted value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b587b22e-db3c-4050-9434-910d79853e32
📒 Files selected for processing (1)
tests/test_executor.py
- assert_executor_start_stop: capture conn returned by retry() and call conn.close() so the readiness backend session is released promptly instead of waiting for GC. - test_executor_with_special_chars_in_all_paths: strengthen the Unix unix_socket_directories assertion to include the actual socket_dir path value, so a missing or mis-quoted path fails the test. - PostgreSQLExecutor.start(): on Windows, run pg_ctl start -w synchronously via shell=True rather than delegating to mirakuru's subprocess polling. pg_ctl exits as soon as the server is ready, so mirakuru's check_subprocess loop always sees a dead launcher process and times out; bypassing it removes the 60-second timeout failure seen in test_noproc_cached_version and related Windows CI jobs. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/executor.py`:
- Around line 179-190: In the Windows branch inside start(), stop using
shell=True with self.command; instead invoke pg_ctl with an argv list and pass
the configured environment: replace the subprocess.run(self.command, shell=True,
check=False) call with a list-form invocation (no shell) that constructs the
same arguments currently interpolated into self.command and call
subprocess.run(..., check=False, env=self.envvars). Mirror the approach used in
stop() (list-form invocation) and ensure you reference/construct the argument
list from the same pieces (datadir, logfile, startparams, postgres_options) so
quoting is handled safely and locale/env vars from self.envvars are applied.
- Around line 270-280: The check_subprocess method currently always returns
self.running(), which breaks launcher subprocess failure detection on
non-Windows; modify check_subprocess(self) to detect platform (Windows vs
others) and only return self.running() on Windows, while on non-Windows call and
return super().check_subprocess() so mirakuru's launcher subprocess checks
remain in effect (reference: check_subprocess, running(),
super().check_subprocess(), and the start()/stopped() behavior described).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cae2e93c-381d-459e-943a-df6dfa7a6801
📒 Files selected for processing (2)
pytest_postgresql/executor.pytests/test_executor.py
- start() Windows branch: replace shell=True/self.command with an argv list built from the same pieces used in WINDOWS_PROC_START_COMMAND (executable, datadir, port, logfile, startparams, postgres_options). Pass env=self.envvars so locale variables reach pg_ctl. Mirrors the list-form invocation already used in stop(). - check_subprocess(): only return self.running() on Windows (where start() is synchronous and mirakuru's loop is bypassed). On all other platforms delegate to super().check_subprocess() so TCPExecutor's port-reachability check and subprocess-alive check remain in effect, enabling fast ProcessFinishedWithError detection when pg_ctl fails to start. - Add missing shlex import. Made-with: Cursor
for more information, see https://pre-commit.ci
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/executor.py`:
- Around line 191-202: The Windows start branch uses subprocess.run(args,
check=False, env=self.envvars) without a timeout so a stuck pg_ctl can block
indefinitely; update the Windows start path in executor.py to pass the same
configured timeout used by the non-Windows path (e.g., self.timeout or executor
timeout variable) to subprocess.run(..., timeout=...), catch
subprocess.TimeoutExpired, ensure the process is terminated/cleanup is done and
propagate or raise an appropriate exception so the executor timeout is honored
(refer to the call site subprocess.run(...) and the surrounding start logic that
builds args and uses self.startparams).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1df4829d-3130-4d06-8c02-7ae0a0858af0
📒 Files selected for processing (1)
pytest_postgresql/executor.py
subprocess.run() on the Windows synchronous start path had no timeout, so a stuck pg_ctl could block indefinitely. Pass self._timeout (the configured mirakuru timeout in seconds) to subprocess.run(..., timeout=). Catch subprocess.TimeoutExpired — subprocess.run already kills the process before re-raising, so no extra cleanup is needed — and translate it to mirakuru.exceptions.TimeoutExpired so callers see the same exception type produced by the non-Windows path. Also imports TimeoutExpired from mirakuru.exceptions alongside the existing ProcessFinishedWithError import. Made-with: Cursor
…nv/posix Bug fixes - executor.py running(): add capture_output=True to subprocess.run so pg_ctl status output no longer leaks to the terminal on every call; previously the change from getstatusoutput() dropped the implicit capture. - test_windows_compatibility.py: remove the module-level pytestmark that skipped every test on non-Windows. All tests in the file use patch() to mock platform.system() and subprocess calls, so none of them require a real Windows host; the blanket skip was silently hiding the entire template and process-management test suite from Linux/macOS CI runs. The unused sys import is removed along with the mark. Inline comment fixes - oldest-postgres.yml: narrow the 'install libpq' step condition to also require runner.os == 'Linux' so 'sudo apt install' is not attempted on non-Linux runners when the matrix entry contains PyPy. - executor.py Windows start() branch: build merged_env from os.environ.copy() updated with self.envvars so system-level PATH and other vital variables are inherited rather than replaced by the locale-only dict. - executor.py Windows start() branch: pass posix=False to shlex.split(self.startparams) so Windows backslashes in startparams are treated as literals rather than POSIX escape characters. Nitpick - newsfragments/1182.feature.rst: rewrite as a concise user-facing summary without internal symbol names. Made-with: Cursor
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pytest_postgresql/executor.py (1)
135-150: ⚡ Quick winDerive the Windows
-opayload from one builder.The Windows options string is assembled once for
executor.commandand again instart(). Those copies can drift independently, and this test module mostly asserts the template string rather than the argv that actually runs. A small shared helper would keep the two paths aligned.Also applies to: 189-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_postgresql/executor.py` around lines 135 - 150, The Windows "-o" payload is built twice (once when constructing executor.command and again in start()), which risks drift; add a single helper method (e.g., _build_windows_o_payload or _format_windows_start_args) in the executor class that takes the same inputs used in the existing format (executable, datadir, port, unixsocketdir, logfile, startparams, postgres_options) and returns the prepared Windows -o string, then replace the two ad-hoc constructions with calls to that helper and use its return value when populating WINDOWS_PROC_START_COMMAND and when assembling argv in start(); ensure the helper applies the same escaping (unixsocketdir.replace("'", "''")) and the same conditional for postgres_options so both locations produce identical payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pytest_postgresql/executor.py`:
- Around line 135-150: The Windows "-o" payload is built twice (once when
constructing executor.command and again in start()), which risks drift; add a
single helper method (e.g., _build_windows_o_payload or
_format_windows_start_args) in the executor class that takes the same inputs
used in the existing format (executable, datadir, port, unixsocketdir, logfile,
startparams, postgres_options) and returns the prepared Windows -o string, then
replace the two ad-hoc constructions with calls to that helper and use its
return value when populating WINDOWS_PROC_START_COMMAND and when assembling argv
in start(); ensure the helper applies the same escaping
(unixsocketdir.replace("'", "''")) and the same conditional for postgres_options
so both locations produce identical payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13c4d41f-61ed-46b7-80be-cdfa1465b533
📒 Files selected for processing (4)
.github/workflows/oldest-postgres.ymlnewsfragments/1182.feature.rstpytest_postgresql/executor.pytests/test_windows_compatibility.py
✅ Files skipped from review due to trivial changes (1)
- newsfragments/1182.feature.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/oldest-postgres.yml
- Introduced a new static method _windows_pg_options to centralize the construction of the -o argument for the Windows pg_ctl start invocation, ensuring consistent command arguments across initialization and start methods. - Updated the command template in the Windows start branch to utilize the new helper method, improving clarity and maintainability. - Enhanced tests to verify that the command template and generated options do not include single quotes, aligning with Windows command requirements.
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/executor.py`:
- Around line 366-369: The killpg AttributeError fallback currently calls
self._windows_terminate_process(sig) but does not clear the process handle;
modify the except AttributeError branch in the executor to reset self._process
to None after calling _windows_terminate_process so the executor does not retain
a stale mirakuru process handle (i.e., ensure the same clearing behavior as the
normal Windows branch that follows the call to self._windows_terminate_process).
- Around line 199-210: The tokenisation of quoted Windows start parameters is
incorrect because shlex.split is called with posix=False, which preserves
literal quote characters so subprocess argv mode sends quotes to pg_ctl; update
the call in the args construction (where startparams is processed) to use
shlex.split(self.startparams, posix=True) so quotes are treated as parsing
syntax and removed, ensuring tokens like the value passed to _windows_pg_options
/ pg_ctl are correctly quoted for spaces; keep the rest of the args assembly
(self.executable, "start", "-D", self.datadir, "-o", pg_options, "-l",
self.logfile, *...) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea57b7c3-f5a3-48fb-b33f-02f8c2eb1336
📒 Files selected for processing (2)
pytest_postgresql/executor.pytests/test_windows_compatibility.py
…e in PostgreSQLExecutor - Changed shlex.split(self.startparams) to use posix=True for proper handling of backslashes in start parameters on Windows. - Set self._process to None after terminating the process in the Windows branch to ensure clean state management.
…indows compatibility - Changed the artifact name in the GitHub Actions workflow for PostgreSQL on Windows to include 'windows' for clarity. - Updated shlex.split in PostgreSQLExecutor to use posix=False, ensuring proper handling of backslashes in start parameters on Windows.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pytest_postgresql/executor.py (1)
209-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip quoting syntax before appending Windows
startparams.Line 209 still uses
shlex.split(..., posix=False), which preserves outer double quotes in the resulting tokens. In argv mode those quotes become part of the argument value, so quoted params such as-o "-c config_file=C:\Program Files\...\postgresql.conf"can still break on Windows.🔧 Minimal fix
- *shlex.split(self.startparams, posix=False), + *shlex.split(self.startparams, posix=True),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_postgresql/executor.py` at line 209, The code currently uses shlex.split(self.startparams, posix=False) which preserves outer double-quotes and causes quoted values to include the quotes in argv mode on Windows; change the split to use posix=True (i.e., shlex.split(self.startparams, posix=True)) or otherwise strip surrounding quote characters from each token before appending self.startparams to the command vector so items like -o "-c config_file=..." produce a clean token without embedded quotes; update the code path that builds the argv for Windows where self.startparams is appended (search for the use of self.startparams in pytest_postgresql/executor.py) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/single-postgres-windows.yml:
- Around line 32-38: Replace mutable action tags with full commit SHAs for each
external action use: update actions/checkout@v6,
fizyk/actions-reuse/.github/actions/pipenv-setup@v4.4.4,
ankane/setup-postgres@v1, both
fizyk/actions-reuse/.github/actions/pipenv-run@v4.4.4 entries,
actions/upload-artifact@v7, and codecov/codecov-action@v6.0.0 to their
corresponding full-length commit SHAs (you can obtain the SHA from each action's
GitHub repo by selecting the tag/release commit) and leave an optional comment
noting the original tag; ensure the YAML values remain under uses: and no other
changes are made to the action inputs or step names.
In `@pytest_postgresql/executor.py`:
- Around line 352-354: The subprocess.check_output call that runs
[self.executable, "stop", "-D", self.datadir, "-m", "f"] can block indefinitely;
add the timeout argument using self._timeout and catch
subprocess.TimeoutExpired, converting/handling it the same way the start()
method does (i.e., translate the TimeoutExpired into the same error flow used
there) so that Windows process termination and the parent class stop logic
always run; update the stop() implementation around the subprocess.check_output
call to pass timeout=self._timeout and handle subprocess.TimeoutExpired
accordingly.
---
Duplicate comments:
In `@pytest_postgresql/executor.py`:
- Line 209: The code currently uses shlex.split(self.startparams, posix=False)
which preserves outer double-quotes and causes quoted values to include the
quotes in argv mode on Windows; change the split to use posix=True (i.e.,
shlex.split(self.startparams, posix=True)) or otherwise strip surrounding quote
characters from each token before appending self.startparams to the command
vector so items like -o "-c config_file=..." produce a clean token without
embedded quotes; update the code path that builds the argv for Windows where
self.startparams is appended (search for the use of self.startparams in
pytest_postgresql/executor.py) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d03399e2-ca88-4dfe-a4ce-42e9d09e642e
📒 Files selected for processing (2)
.github/workflows/single-postgres-windows.ymlpytest_postgresql/executor.py
- Added wait_for_postgres() call after pg_ctl start to ensure the server is ready for connections. - Implemented timeout handling in the stop() method to raise TimeoutExpired if the stop command exceeds the configured timeout.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
fizyk
reviewed
May 5, 2026
…ssertion - Added timeout parameter to the mock_subprocess call in the TestWindowsCompatibility class to ensure that the timeout handling is properly tested in the pg_ctl stop command.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_windows_compatibility.py (1)
623-624: ⚡ Quick winStrengthen the Unix stop-path assertion to lock in safe subprocess invocation.
At Line 623,
assert_called_once()is too loose: a regression back to string/shell-style invocation could still pass. Please assert the exact argv list and timeout (as done in the Windows variant) so this test enforces the security contract on Unix too.Proposed test tightening
- mock_subprocess.assert_called_once() + mock_subprocess.assert_called_once_with( + ["/path/to/pg_ctl", "stop", "-D", "/tmp/data", "-m", "f"], + timeout=executor._timeout, + ) mock_super_stop.assert_called_once_with(None, None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_windows_compatibility.py` around lines 623 - 624, Replace the loose mock_subprocess.assert_called_once() with an exact argument assertion to ensure subprocess is invoked with the list-style argv and the expected timeout: use mock_subprocess.assert_called_once_with([...expected argv list...], timeout=EXPECTED_TIMEOUT) (mirroring the Windows-path assertion style), and keep mock_super_stop.assert_called_once_with(None, None) unchanged; update the test to reference the same expected argv and timeout constants/values used elsewhere in the test to lock in the safe Unix invocation contract for mock_subprocess.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_windows_compatibility.py`:
- Around line 623-624: Replace the loose mock_subprocess.assert_called_once()
with an exact argument assertion to ensure subprocess is invoked with the
list-style argv and the expected timeout: use
mock_subprocess.assert_called_once_with([...expected argv list...],
timeout=EXPECTED_TIMEOUT) (mirroring the Windows-path assertion style), and keep
mock_super_stop.assert_called_once_with(None, None) unchanged; update the test
to reference the same expected argv and timeout constants/values used elsewhere
in the test to lock in the safe Unix invocation contract for mock_subprocess.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0e0a8f8-97b7-4828-9130-d9ad652e59db
📒 Files selected for processing (1)
tests/test_windows_compatibility.py
…timeout - Modified the mock_subprocess assertion in the TestWindowsCompatibility class to include the expected timeout parameter for the pg_ctl stop command, ensuring proper timeout handling is tested.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_windows_compatibility.py (1)
560-570: ⚡ Quick winStrengthen timeout verification in force-kill test
This test checks sequence/counts, but it doesn’t assert that
wait()is called with the configured timeout. Add an argument assertion so regressions to blocking waits are caught.Suggested test tightening
executor._windows_terminate_process() # Should call terminate, wait (timeout), then kill, then wait again mock_process.terminate.assert_called_once() mock_process.kill.assert_called_once() assert mock_process.wait.call_count == 2 + first_wait_call = mock_process.wait.call_args_list[0] + assert first_wait_call.kwargs.get("timeout") == executor._timeout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_windows_compatibility.py` around lines 560 - 570, The test should assert that mock_process.wait is invoked with the configured timeout to prevent regressions to blocking waits: after calling executor._windows_terminate_process(), add assertions on mock_process.wait call arguments (via mock_process.wait.call_args_list or equivalent) to verify the first wait() call was invoked with the timeout value 5 and the subsequent wait() call was invoked without a timeout (blocking), referencing mock_process.wait and executor._windows_terminate_process to locate the correct place to add these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_windows_compatibility.py`:
- Around line 560-570: The test should assert that mock_process.wait is invoked
with the configured timeout to prevent regressions to blocking waits: after
calling executor._windows_terminate_process(), add assertions on
mock_process.wait call arguments (via mock_process.wait.call_args_list or
equivalent) to verify the first wait() call was invoked with the timeout value 5
and the subsequent wait() call was invoked without a timeout (blocking),
referencing mock_process.wait and executor._windows_terminate_process to locate
the correct place to add these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fccf7f81-94fc-4e6a-a63d-bf7b3ff21478
📒 Files selected for processing (1)
tests/test_windows_compatibility.py
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
2 participants
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.
Refactors
PostgreSQLExecutorto improve Windows compatibility and security for PostgreSQL process management. This change addresses platform-specific issues with process termination and command formatting that could cause failures on Windows systems.Changes
_get_base_commandmethod toBASE_PROC_START_COMMANDclass attribute with cross-platform compatible syntax (removed single quotes around PostgreSQL config values that caused issues on Windows)_windows_terminate_processmethod that attempts graceful shutdown before force-killing stuck processesstop()method to use list arguments instead ofshell=Trueto prevent shell injection vulnerabilitiesos.killpgAttributeError on WindowsTest plan
tests/test_windows_compatibility.pywith 7 test caseskillpgAttributeErrorSummary by CodeRabbit
Improvements
Tests
CI
Documentation