Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Open
tboy1337 wants to merge 52 commits intodbfixtures:mainfrom
Open
Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182tboy1337 wants to merge 52 commits intodbfixtures:mainfrom
tboy1337 wants to merge 52 commits intodbfixtures:mainfrom
Conversation
Open
fizyk
reviewed
Sep 4, 2025
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPostgreSQLExecutor gains Windows-specific start/stop/status handling with separate pg_ctl command templates and quoting rules; Windows uses argv-list subprocess invocation, synchronous start, env merging, timeout and error raising, and a terminate→wait→kill fallback. Tests and CI workflows are updated to exercise and locate pg_ctl on Windows. ChangesWindows compatibility and executor changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Executor as PostgreSQLExecutor
participant Subprocess as subprocess
participant PGCtl as pg_ctl
participant OS as OS/process
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 mirakuru _process
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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 |
- test_executor_init_bad_tmp_path: add parents=True to mkdir so nested path creation works on Windows (backslash in path literal is a separator, creating two levels that need parents=True) - executor.py stop(): set self._process = None after Windows terminate so mirakuru state is clean and stopped() context manager can call start() again without timing out - test_postgres_loader_in_ini: convert backslashes to forward slashes before writing Windows path into pytest.ini to prevent configparser from mangling the value as escape sequences Made-with: Cursor
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 `@tests/test_executor.py`:
- Around line 219-223: The current assertions allow unquoted paths to pass;
change them to require the quoted fragments explicitly by asserting
f'"{datadir}"' in command and f'"{logfile_path}"' in command (replace the two
existing assertions that use the OR checks). Locate the assertions around
executor.command in test_executor.py (where variables datadir and logfile_path
are used) and update them to assert the quoted strings directly.
🪄 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: 77025966-d8c7-40a4-b4c9-cafbc1c93a6a
📒 Files selected for processing (3)
pytest_postgresql/executor.pytests/test_executor.pytests/test_postgres_options_plugin.py
- Override check_subprocess() in PostgreSQLExecutor to use pg_ctl status instead of checking whether the launcher subprocess (pg_ctl start -w) is still alive. pg_ctl start -w exits as soon as the server is ready, so mirakuru's default polling loop always timed out when stopped() tried to restart the server on Windows. Using pg_ctl status makes the start/stopped() cycle reliable on all platforms. - Wrap the psycopg.connect() call in assert_executor_start_stop() with retry() so that transient 'the database system is starting up' errors are tolerated. Under parallel xdist runs multiple PostgreSQL instances start simultaneously; the TCP port opens before recovery completes, causing an immediate connect to fail even though the server is running. Made-with: Cursor
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_executor.py (1)
226-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the quoting assertion strict.
These
orchecks still pass ifdatadirorlogfileis emitted bare, so the test can miss the quoting regression it is meant to catch.Proposed fix
- assert str(datadir) in command or f'"{datadir}"' in command - assert str(logfile_path) in command or f'"{logfile_path}"' in command + assert f'"{datadir}"' in command + assert f'"{logfile_path}"' in command🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` around lines 226 - 229, The test currently allows either quoted or unquoted paths so it won't catch regressions; change the two assertions to require the quoted forms only by replacing the logic that checks `str(datadir) in command or f'"{datadir}"' in command` and `str(logfile_path) in command or f'"{logfile_path}"' in command` with strict assertions that `f'"{datadir}"' in executor.command` and `f'"{logfile_path}"' in executor.command` (referencing `executor.command`, `datadir`, and `logfile_path` to locate the lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_executor.py`:
- Around line 226-229: The test currently allows either quoted or unquoted paths
so it won't catch regressions; change the two assertions to require the quoted
forms only by replacing the logic that checks `str(datadir) in command or
f'"{datadir}"' in command` and `str(logfile_path) in command or
f'"{logfile_path}"' in command` with strict assertions that `f'"{datadir}"' in
executor.command` and `f'"{logfile_path}"' in executor.command` (referencing
`executor.command`, `datadir`, and `logfile_path` to locate the lines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c58e41c-ba00-4d3d-862b-f296230c6c92
📒 Files selected for processing (2)
pytest_postgresql/executor.pytests/test_executor.py
- Updated assertions in `test_executor_with_special_chars_in_all_paths` to ensure that both `datadir` and `logfile_path` are properly quoted in the command string. This change enhances the reliability of the tests by explicitly checking for the presence of quoted paths, improving compatibility with special characters. 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 `@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
Comment on lines
+32
to
+38
| - uses: actions/checkout@v6 | ||
| - name: Set up Pipenv on python ${{ matrix.python-version }} | ||
| uses: fizyk/actions-reuse/.github/actions/pipenv-setup@v4.4.4 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| allow-prereleases: true | ||
| - uses: ankane/setup-postgres@v1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".github/workflows/single-postgres-windows.yml"
echo "All action references:"
rg -nP '^\s*-\s*uses:\s*[^ ]+@[^ ]+' "$file"
echo
echo "Action references NOT pinned to a 40-char SHA:"
rg -nP '^\s*-\s*uses:\s*[^ ]+@(?![0-9a-fA-F]{40}\b)[^ ]+' "$file"Repository: dbfixtures/pytest-postgresql
Length of output: 389
🏁 Script executed:
cat -n ".github/workflows/single-postgres-windows.yml"Repository: dbfixtures/pytest-postgresql
Length of output: 3503
Pin all external actions to immutable commit SHAs.
Using mutable tags for uses: leaves the workflow open to upstream tag retargeting and supply-chain drift. Replace version tags with full-length commit SHA pins (optionally with a trailing comment for the tag/release).
Seven actions currently use mutable tags across this workflow:
- Line 32:
actions/checkout@v6 - Line 34:
fizyk/actions-reuse/.github/actions/pipenv-setup@v4.4.4 - Line 38:
ankane/setup-postgres@v1 - Lines 60, 64:
fizyk/actions-reuse/.github/actions/pipenv-run@v4.4.4 - Line 67:
actions/upload-artifact@v7 - Line 73:
codecov/codecov-action@v6.0.0
Suggested update pattern
- - uses: actions/checkout@v6
+ - uses: actions/checkout@<40-char-commit-sha> # v6
- uses: fizyk/actions-reuse/.github/actions/pipenv-setup@v4.4.4
+ uses: fizyk/actions-reuse/.github/actions/pipenv-setup@<40-char-commit-sha> # v4.4.4
- - uses: ankane/setup-postgres@v1
+ - uses: ankane/setup-postgres@<40-char-commit-sha> # v1
- uses: fizyk/actions-reuse/.github/actions/pipenv-run@v4.4.4
+ uses: fizyk/actions-reuse/.github/actions/pipenv-run@<40-char-commit-sha> # v4.4.4
- - uses: actions/upload-artifact@v7
+ - uses: actions/upload-artifact@<40-char-commit-sha> # v7
- uses: codecov/codecov-action@v6.0.0
+ uses: codecov/codecov-action@<40-char-commit-sha> # v6.0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/single-postgres-windows.yml around lines 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.
Member
There was a problem hiding this comment.
@coderabbitai if dependabot handles it, defer that to separate issue
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Dependabot configuration
fd -i 'dependabot.yml' .github/ --exec cat {}
echo "---"
fd -i 'dependabot.yaml' .github/ --exec cat {}
Unrecoverable error: Failed to clone repository
🏁 Script executed:
#!/bin/bash
# Fetch dependabot config via GitHub API
gh api repos/dbfixtures/pytest-postgresql/contents/.github/dependabot.yml \
--jq '.content' | base64 -d 2>/dev/null || echo "NOT FOUND"
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Try fetching dependabot config via raw GitHub URL
curl -s -o /dev/null -w "%{http_code}" \
"https://raw.githubusercontent.com/dbfixtures/pytest-postgresql/main/.github/dependabot.yml"
echo ""
curl -s "https://raw.githubusercontent.com/dbfixtures/pytest-postgresql/main/.github/dependabot.yml"
Length of output: 340
@fizyk, confirmed — .github/dependabot.yml on main already has package-ecosystem: github-actions configured (weekly, Sundays). Would you like me to open a follow-up GitHub issue to track pinning the action references in this new workflow to immutable commit SHAs?
Member
There was a problem hiding this comment.
yes, that's what I asked. Not sure how that would help, with my resources being a dependabot and auto-merge, with no real time for manual review od those updates. Well at least setup-posgres updates would be more visible.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo dbfixtures/pytest-postgresql \
--title "Pin GitHub Actions to immutable commit SHAs in single-postgres-windows.yml" \
--body "## Summary
The new workflow \`.github/workflows/single-postgres-windows.yml\` (introduced in PR `#1182`) references external actions using mutable version tags rather than immutable commit SHAs. Using mutable tags leaves the workflow open to upstream tag retargeting and supply-chain drift.
Dependabot (\`.github/dependabot.yml\`) is already configured with \`package-ecosystem: github-actions\` and will raise PRs to keep these pinned; this issue tracks the initial pinning.
## Actions to pin
| Step | Current reference | Notes |
|---|---|---|
| checkout | \`actions/checkout@v6\` | |
| pipenv-setup | \`fizyk/actions-reuse/.github/actions/pipenv-setup@v4.4.4\` | |
| setup-postgres | \`ankane/setup-postgres@v1\` | |
| pipenv-run (×2) | \`fizyk/actions-reuse/.github/actions/pipenv-run@v4.4.4\` | |
| upload-artifact | \`actions/upload-artifact@v7\` | |
| codecov-action | \`codecov/codecov-action@v6.0.0\` | |
## Suggested approach
Replace each mutable tag with the full 40-character commit SHA of that release, leaving the tag in a trailing comment for readability, e.g.:
\`\`\`yaml
- uses: actions/checkout@<40-char-sha> # v6
\`\`\`
Once pinned, Dependabot will keep them up to date automatically via PRs.
---
Backlink: PR `#1182`, comment https://github.com/dbfixtures/pytest-postgresql/pull/1182#discussion_r3173411799
Requested by: `@fizyk`" \
--assignee fizyk
Length of output: 60
@fizyk, I've opened issue #1331 to track the initial pinning. You're right that with Dependabot + auto-merge the ongoing maintenance is largely hands-off — the main practical benefit is that ankane/setup-postgres updates will show up as explicit PRs rather than silently picking up whatever the tag points to.
- 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
|
|
||
| # Should call pg_ctl stop and Windows terminate | ||
| mock_subprocess.assert_called_once_with( | ||
| ["/path/to/pg_ctl", "stop", "-D", "/tmp/data", "-m", "f"], |
Member
There was a problem hiding this comment.
If I'm not mistaken, the timeout is missing, and we'll be all set.
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