Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Merged
Conversation
Closed
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
January 21, 2026 07:59
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
January 21, 2026 10:24
b02c61b to
4a0b22e
Compare
fizyk
reviewed
Jan 23, 2026
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
January 29, 2026 05:40
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: 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
fizyk
approved these changes
May 12, 2026
Member
|
Thanks @tboy1337 ! |
|
Thanks so much for this! It's a huge help to be able to run this on Windows directly |
fizyk
pushed a commit
that referenced
this pull request
Jul 14, 2026
* feat: add async support for PostgreSQL janitor and loader - Introduced AsyncDatabaseJanitor for managing database state asynchronously. - Added async loading capabilities with build_loader_async and sql_async functions. - Updated factories to include async versions of PostgreSQL fixtures. - Enhanced tests to cover async functionality for janitor and loader. - Updated pyproject.toml to include optional dependencies for async support. * test: enhance async tests for Database Janitor and Loader - Added tests for custom database name handling in AsyncDatabaseJanitor. - Implemented tests for database initialization and dropping with various configurations. - Included tests for callable resolution in build_loader and build_loader_async with dot-separated paths. - Improved error handling tests for sql_async when aiofiles is not installed. * fix: improve error handling for missing optional dependencies in async functions - Updated the loader and client to check for aiofiles and pytest_asyncio imports at runtime, raising ImportError with clear messages if they are not installed. - Refactored tests to mock the absence of these dependencies more effectively, ensuring proper error handling is validated. * fix: resolve double plugin registration, add asyncio_mode and default postgresql_async fixture Made-with: Cursor * Revert "fix: resolve double plugin registration, add asyncio_mode and default postgresql_async fixture" This reverts commit 5c92310. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: enhance async fixture and retry tests - Updated the postgresql_async fixture to include loop_scope for better async handling. - Added xdist_group marker to the test_postgres_terminate_connection_async for improved test organization. - Refactored retry tests to mock sleep and current time, ensuring accurate timeout handling and retry logic validation. * refactor: update connection handling in AsyncDatabaseJanitor - Changed isolation level and autocommit settings to direct attribute assignment instead of using methods. - Removed unnecessary mock methods in test for improved clarity and performance. * fix: add space in SQL query for connection termination - Added a space at the end of the SQL query string in both DatabaseJanitor and AsyncDatabaseJanitor to ensure proper formatting. * refactor: improve PostgreSQLExecutor and AsyncDatabaseJanitor handling - Updated PostgreSQLExecutor to conditionally omit Unix socket parameters on Windows. - Enhanced error handling in PostgreSQLExecutor for Windows compatibility. - Refactored AsyncDatabaseJanitor to use async methods for setting isolation level and autocommit. - Updated tests to mock async connection methods for better compatibility. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * revert: remove Windows-specific changes deferred to PR-1182 Remove Windows executor compatibility changes (log_destination quoting, unix_socket_directories platform guard, os.killpg AttributeError catch) and the Windows-only event_loop_policy fixture from conftest.py. These belong in the separate Windows compatibility PR #1182. Made-with: Cursor * refactor: enhance SQL query handling in DatabaseJanitor and AsyncDatabaseJanitor - Updated SQL query construction to use psycopg.sql for better safety and readability. - Refactored database creation, alteration, and dropping methods to utilize composable SQL objects. - Improved test assertions by rendering SQL commands for clarity in test outputs. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: improve error handling in postgresql_async fixture - Updated the postgresql_async fixture to conditionally use pytest_asyncio if available, enhancing compatibility with synchronous fixtures. - Added tests to ensure that the fixture does not raise errors at creation time when pytest_asyncio is absent, while still raising an ImportError during usage without pytest_asyncio. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: enhance postgresql_async fixture to provide synchronous stub when pytest-asyncio is absent - Updated the postgresql_async fixture to return a synchronous stub that raises ImportError if pytest-asyncio is not available, preventing coroutine-related warnings. - Modified tests to verify that the synchronous stub is correctly registered and raises the appropriate error when used without pytest-asyncio. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * feat: add async testing support and improve documentation - Added `pytest-asyncio` and `aiofiles` as development dependencies for async testing. - Updated README to include instructions for async tests with `psycopg.AsyncConnection`. - Enhanced documentation for async fixtures and their usage. - Improved error handling in the `postgresql_async` fixture to provide a synchronous stub when `pytest-asyncio` is absent. - Added tests to verify async functionality and ensure proper teardown of resources. * refactor: streamline temporary directory creation in postgresql_proc and enhance SQL rendering in tests - Moved the temporary directory creation and data directory preparation to the try block in the postgresql_proc function for better error handling. - Updated the SQL rendering function to utilize the as_string method for pgsql.Composable objects, simplifying the code and improving readability. * Fix xdist CI failures and raise patch coverage for async changes. Use the shared basetemp parent under xdist for port-lock tests, align async template tests with the sync xdist group, and add unit tests for sql_async, drop_test_database, and setup-failure port cleanup. Co-authored-by: Cursor <cursoragent@cursor.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Replace mock coverage tests with integration tests for async paths. Exercise sql_async via AsyncDatabaseJanitor against live Postgres and add a pointed_pytester drop_test_database async example mirroring the sync integration test. Co-authored-by: Cursor <cursoragent@cursor.com> * Fix postgresql_oldest and Windows CI failures for async support. Use a psycopg 3.0-compatible SQL render fallback in janitor tests and set WindowsSelectorEventLoopPolicy in the plugin so psycopg async works on Windows. Co-authored-by: Cursor <cursoragent@cursor.com> * Replace mock AsyncDatabaseJanitor SQL tests with Postgres integration tests. Exercise init, drop, template flags, and TEMPLATE cloning against live PostgreSQL and remove _render_sql mock infrastructure. Co-authored-by: Cursor <cursoragent@cursor.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix mypy errors in janitor integration test helpers. Inline AsyncConnection.connect kwargs instead of dict unpacking so strict mypy accepts the connection parameters. Co-authored-by: Cursor <cursoragent@cursor.com> * Update pytest-asyncio dependency to version 0.24 in Pipfile and pyproject.toml; enhance documentation for async extra to reflect the new version requirement. * Enhance README and client.py documentation for async fixtures, specifying minimum pytest-asyncio version 0.24. Update error messages to reflect new version requirement and clarify async fixture behavior. * Update error message in test for pytest-asyncio version requirement to use escaped regex pattern. * Update pytest-postgresql plugin to set Windows event loop policy conditionally based on asyncio plugin presence; refactor password handling in janitor tests for consistency. * Refactor password handling in janitor tests to address linting warning S105. * Refactor fixture scope handling in PostgreSQL factories by removing the scope parameter and setting defaults directly in the fixture decorators. Update README to clarify async fixture behavior and dependencies. Remove unused types module. * Update README to remove unnecessary version details for async fixtures and modify test cases to handle port initialization correctly by passing None instead of -1. This change ensures better compatibility with various configurations. * Update pytest-asyncio dependency to version 1.4 across configuration files and documentation. Enhance README to reflect changes in async fixture requirements and clarify Windows event loop handling for compatibility with psycopg async. * Enhance README to clarify the handling of Windows event loop policy with pytest-asyncio and update test cases to ensure compatibility with the new loop-factory hook. Add optional hook implementation for pytest-asyncio loop factories. * Refactor Windows event loop handling in pytest-postgresql plugin to improve compatibility with asyncio. Introduce helper functions to check platform and Python version, and update tests to reflect these changes. * Refactor pytest-asyncio loop factory implementation in the Windows context to ensure it is only registered when on a Windows platform. Update tests to verify the correct behavior of the loop factory registration based on the operating system. * Add support for optional pytest-asyncio import in plugin.py This update introduces a conditional import for pytest-asyncio, allowing the plugin to function without it. The version check for pytest-asyncio has been refactored to improve clarity and maintain compatibility with versions below 1.4. This change enhances the plugin's flexibility in various environments. * ci: re-trigger workflow after runner communication flake on 3.10 The postgresql_oldest / postgres (3.10) job failed due to a hosted runner losing communication after 46m; all other matrix jobs passed. Co-authored-by: Cursor <cursoragent@cursor.com> * Refactor loader functions to remove maxsplit in regex for import path parsing This change simplifies the regex used in `build_loader` and `build_loader_async` functions, allowing for deeper nested import paths without limiting the split. Additionally, new tests have been added to verify the correct behavior of these functions with deeply nested import paths. * Enhance README and plugin.py to clarify Windows event loop handling for psycopg async Updated the README to explain the necessity of using a SelectorEventLoop on Windows due to incompatibility with the ProactorEventLoop. Added details on pytest-asyncio's loop-factory hook and its role in configuring the event loop. In plugin.py, improved comments and documentation regarding the registration of the SelectorEventLoop factory, ensuring clarity on its purpose and usage in async tests on Windows. * Address fizyk review: CI for async extra, consolidate build_loader Add dedicated CI jobs that install pytest-postgresql[async] and run async-marked tests on Ubuntu and Windows. Remove duplicate build_loader_async in favour of build_loader(sql_loader=sql_async) and align optional pytest_asyncio imports with the loader.py pattern. * ci: harden workflow permissions and checkout credentials Add least-privilege permissions blocks to tests.yml and async-postgres.yml, and set persist-credentials: false on checkout in the async extra workflow. * ci: run async-extra jobs without waiting on postgresql_18 * ci: scope workflow permissions to individual jobs * ci: drop actions:write and harden async pytest env; sort factories __all__ * fix(ci): use GHA expressions for async pytest paths on Windows Shell env vars like \ are empty under pwsh on Windows runners. Expand POSTGRESQL_EXEC and basetemp via GitHub Actions expressions before the shell runs, matching single-postgres-windows.yml. * fix: guard port lock cleanup when port selection fails early * fix: ensure foreign port lock files are preserved on exhausted retries * fix: suppress exception chaining in port selection error message * fix: enhance async fixture handling and improve Windows event loop configuration - Updated README to clarify the automatic configuration of `SelectorEventLoop` for `postgresql_async` tests on Windows. - Improved error message in GitHub Actions workflow for PostgreSQL path verification. - Refactored plugin logic to ensure the selector loop factory is only registered for tests using the `postgresql_async` fixture. - Added a stub for async fixture when `pytest-asyncio` is missing or outdated, with corresponding tests to validate behavior. * fix: enhance async fixture support and improve documentation - Updated README to include details on using `AsyncDatabaseJanitor` and the fallback behavior for older `pytest-asyncio` versions on Windows. - Refactored async fixture handling in the plugin to ensure proper loop factory registration for tests using `postgresql_async`. - Added new tests to validate async fixture functionality and ensure compatibility with custom isolation levels. - Improved error handling in GitHub Actions workflows for PostgreSQL path detection on Windows. * fix: enhance async fixture documentation and improve janitor configuration - Updated README to include instructions for creating async client fixtures with `postgresql_async`. - Refactored janitor classes to utilize a shared configuration dataclass for improved maintainability. - Adjusted GitHub Actions workflows to ensure proper permissions for async jobs. - Removed deprecated function for checking pytest-asyncio support, streamlining async fixture handling. * fix: update license file reference and enhance documentation for async fixtures - Changed license reference in `pyproject.toml` from LICENSE to COPYING.lesser. - Expanded README with detailed instructions on async fixture usage and configuration. - Updated architecture diagrams to include async fixture representations. - Added tests to validate async fixture behavior and error handling for missing dependencies. * docs: update rendered mermaid diagram [skip ci] * fix: enhance async fixture behavior and improve documentation - Updated README to clarify the behavior of `postgresql_async` and its interaction with other asyncio tests on Windows. - Improved documentation on the usage of `AsyncDatabaseJanitor` and SQL file loading with async fixtures. - Refactored `AsyncDatabaseJanitor` to skip setting isolation level when it is None. - Added tests to ensure correct behavior of async cursor regarding isolation levels and validate loop factory registration for async tests. * fix: handle None isolation level in database connections - Updated DatabaseJanitor to set isolation level only if it is not None. - Enhanced postgresql and postgresql_async functions to conditionally set isolation level for database connections. - Modified tests to verify correct behavior of custom isolation levels by checking the transaction isolation setting. * fix: correct typos and update PostgreSQL version references - Fixed the spelling of "Miscellaneous" in `pyproject.toml`. - Updated README to change "test" to "tests" and modified PostgreSQL version references from 13 to 14. - Adjusted `password` type in `config.py` and `executor.py` to allow None. - Updated default PostgreSQL executable path in `plugin.py` to reflect version 14. - Changed default PostgreSQL version in `conftest.py` to 14. * refactor: streamline janitor configuration with shared protocol - Introduced a new `_JanitorConfigTarget` protocol to define shared janitor configuration attributes. - Created `_apply_janitor_config` function to centralize the application of configuration settings to janitor instances. - Updated `DatabaseJanitor` and `AsyncDatabaseJanitor` to utilize the new configuration application method, improving code maintainability. * refactor: simplify PostgreSQL path detection and enhance test structure - Replaced manual PostgreSQL path detection in GitHub Actions workflows with a reusable action for improved maintainability. - Updated architecture diagram to reflect changes in the async workflow. - Refactored test for dropping the test database to streamline the process and improve clarity by introducing a helper function. * docs: update rendered mermaid diagram [skip ci] * fix: resolve CI failures for mypy, port-lock test, and Windows initdb * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: use initdb.exe on Windows and avoid xdist port collisions in teardown test * fix: stabilize xdist port-lock tests and Windows async CI initdb * ci: use separate Windows step for async test bootstrap * fix: unify Windows async CI pytest command with Linux The bootstrap step combined test_postgresql_proc with -m asyncio, which deselected the sync test and caused pytest to exit with no tests selected. Rely on the initdb hardening in executor.py for asyncio-first setup. * fix: prepare initdb parent directory on Windows without pre-creating pgdata Windows initdb fails when it must mkdir through an existing pytest basetemp directory. Ensure only the fixture parent exists, unset PGDATA in the env, and let initdb create the data directory leaf itself. * fix: restore pg_ctl initdb on Windows and bootstrap async CI session Direct initdb.exe calls fail when pgdata lives under an existing pytest basetemp directory. Use pg_ctl initdb like main, register a bootstrap mark for test_postgresql_proc, and run it before asyncio tests on Windows CI. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: pass only locale env to initdb and order bootstrap tests first mirakuru merges os.environ into envvars, so inheriting envvars passed CI PGDATA to initdb on Windows. Return explicit locale overrides instead and run bootstrap-marked tests before asyncio tests that need postgresql_proc. * fix: merge process env for initdb while unsetting inherited PGDATA Windows initdb crashes without PATH and other process variables, but inheriting PGDATA from setup-postgres breaks cluster initialization. * fix: make postgresql_proc a non-generator session fixture pytest-asyncio wraps generator session fixtures for async tests, which broke Windows initdb when asyncio tests initialized postgresql_proc first. Use explicit start/stop with addfinalizer instead of yield. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: align Windows async CI initdb with main executor behavior Use mirakuru envvars for initdb like main, drop custom basetemp on Windows async CI, and assert fixture tempdirs exist before PostgreSQL starts. * fix: use flat Windows pgdata path outside pytest temp parents initdb on Windows fails when pgdata is nested under existing pytest temp directories. Store the cluster data directory directly under the system temp folder instead. * fix: keep Windows postgres logs beside flat pgdata paths Resolve long paths for pg_ctl and store log files next to the Windows cluster directory to avoid short-path mismatches during server start. * fix: default Windows asyncio tests to SelectorEventLoop Python 3.14 still creates ProactorEventLoop via asyncio.new_event_loop, which breaks psycopg async janitor and client tests in the async CI job. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: satisfy mypy in port-lock tests and stabilize Windows plugin tests Patch supports_loop_factories in the legacy policy unit test and drop the subprocess smoke database before reusing the parent PostgreSQL port. * fix: make janitor drop idempotent and stabilize legacy policy test Skip drop when the target database is missing so --postgresql-drop-test-database works for client fixtures that run before the database exists. Compare policy type in the legacy Windows pytest_configure test instead of object identity. * fix: refactor Windows initdb command test to use monkeypatch Updated the test for building the initdb command on Windows to utilize monkeypatching for platform detection, improving test reliability and clarity. * fix: enhance PostgreSQLExecutor environment handling and improve test cleanup Refactor the _initdb_env method to merge environment variables while ensuring PGDATA is excluded. Update the postgresql_proc fixture to ensure the executor is stopped and resources are released properly during cleanup, even in case of setup failures. Add tests to verify that the executor stops correctly when initialization fails and that cleanup is performed reliably. * refactor: improve PostgreSQLExecutor initialization and enhance test structure Refactor the PostgreSQLExecutor to format initdb options correctly for different platforms and streamline the initialization process. Update the test suite to ensure proper handling of temporary directories and logging, particularly on Windows. Remove deprecated bootstrap markers and enhance the README for clarity on asyncio test configurations. Additionally, improve cleanup procedures in tests to ensure resources are released reliably. * Fix CI failures for Windows pg_ctl log paths and docker async load test. Keep Windows PostgreSQL logfiles on the system temp volume to avoid Access denied when pytest basetemp is on another drive, and assert the loaded schema exists instead of requiring token rows. * ci: re-trigger workflows after stuck xdist runner on postgresql_18 (3.10) * Fix xdist teardown hang by skipping rmtree while PostgreSQL is running. Guard clean_directory() when pg_ctl status reports a live server and make fixture cleanup best-effort so xdist workers do not block on locked data directories. * fix: improve password handling in PostgreSQLExecutor initialization Refactor the password handling mechanism in the PostgreSQLExecutor to use a temporary file with a defined deletion strategy. This change ensures that the password file is properly closed and deleted after use, enhancing security and resource management during the initialization process. * fix: improve async handling in janitor and update test structure Refactor the AsyncDatabaseJanitor to properly await results from synchronous loaders and enhance the test suite with isolated temporary directories for port-lock sentinel files. Update CI workflows to streamline test execution on different operating systems and improve cleanup procedures in the postgresql_proc fixture. * ci: harden oldest-postgres workflow against xdist hangs Add a 30-minute job timeout, use runner.temp basetemp for pytest (parity with single-postgres), and re-trigger after stuck postgresql_oldest (3.10). * ci: add 30-minute job timeout to all PostgreSQL test workflows * fix: enhance error handling and resource management in PostgreSQLExecutor Refactor the PostgreSQLExecutor to improve error handling during cleanup and initialization processes. Ensure that the cleanup methods log failures appropriately and maintain the state of the executor. Update tests to verify that resources are released correctly, even in the event of errors, and that timeout handling is properly implemented in subprocess calls. * fix: update initdb command options for Windows compatibility Refactor the initdb command in the Windows compatibility tests to include a locale option. This change ensures that the command is built correctly for Windows environments, enhancing compatibility and reliability in the test suite. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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
3 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