From 39c1cd8d5e845e91d77123d8b9a53fdb4e99f81b Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 28 Oct 2026 15:28:37 +0000 Subject: [PATCH 1/4] Fix race conditions when running tests in separate worktrees --- .github/copilot-instructions.md | 9 + .github/workflows/ci.yml | 8 +- CONTRIBUTING.md | 12 ++ Makefile | 14 +- README.md | 6 +- docs/E2E_TESTING.md | 5 + docs/TESTING.md | 49 ++++- scripts/pg-common.sh | 22 ++ scripts/pg-reset.sh | 5 +- scripts/pg-start.sh | 17 +- scripts/pg-stop.sh | 6 +- scripts/test-all-local.sh | 4 + scripts/test-coverage.sh | 4 + scripts/test-e2e-local.sh | 3 + scripts/test-epoch-race.sh | 4 + scripts/test-shutdown.sh | 4 + scripts/test-unit.sh | 8 +- scripts/test-upgrade.sh | 5 + scripts/with-pg-lock.py | 78 +++++++ tests/test_pg_lock.py | 360 ++++++++++++++++++++++++++++++++ 20 files changed, 603 insertions(+), 20 deletions(-) create mode 100644 scripts/with-pg-lock.py create mode 100644 tests/test_pg_lock.py diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ab092a4a..607a6eca 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -57,6 +57,15 @@ tail -f ~/.pgrx/17.log ./scripts/pg-stop.sh ``` +Local runners require Python 3 and automatically coordinate across worktrees +using a shared PostgreSQL file lock. Use the scripts above rather than raw +`cargo pgrx test`; a waiting message is normal when another run owns the lock. +For low-level commands, use `python3 scripts/with-pg-lock.py [args...]`. +Never delete the lock file to bypass a waiter. `--keep` does not reserve the +server after the outer command exits; use a +[locked debugging shell](../docs/TESTING.md#concurrent-local-runs) when needed. +Build/check-only commands do not need the lock. + ## Critical Patterns ### Schema-Qualify Extension-Owned SQL diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00789612..1e25ad35 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,6 +61,9 @@ jobs: - name: Test upgrade lifecycle harness run: python3 -m unittest discover -s scripts -p test_upgrade_lifecycle.py + - name: Test local PostgreSQL locking + run: python3 -m unittest discover -s tests -p test_pg_lock.py + make_install_smoke: name: Source Install Checks runs-on: ubuntu-latest @@ -76,6 +79,9 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Test local PostgreSQL locking + run: python3 -m unittest discover -s tests -p test_pg_lock.py + - name: Install Rust stable run: | rustup toolchain install stable --profile minimal @@ -297,7 +303,7 @@ jobs: run: scripts/pgspot-gate.sh /tmp/pg_durable-install.sql - name: Run unit tests - run: cargo pgrx test pg${{ matrix.pg_version }} + run: ./scripts/test-unit.sh --pg-version ${{ matrix.pg_version }} - name: Run E2E tests (all startup policies) id: e2e_tests diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d721c285..623eb8f9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,6 +46,18 @@ tail -f ~/.pgrx/17.log If you cannot run the targeted check locally, say so in the PR and CI will still run the full suite. +### Concurrent worktrees + +Local test scripts require Python 3 and automatically queue for exclusive access +to the shared PostgreSQL installation. Use these scripts rather than invoking +`cargo pgrx test` directly. Builds and checks that do not install the extension +can still run in parallel. + +`--keep` leaves the server running but releases the lock when the command exits; +a queued run may then stop or reset it. See +[concurrent local runs](docs/TESTING.md#concurrent-local-runs) for wrapping +low-level commands or reserving PostgreSQL throughout a debugging session. + ### Full pre-PR checks Before opening a pull request, run the checks relevant to your change: diff --git a/Makefile b/Makefile index 701174e3..2db08bfa 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ EXTRA_FEATURES ?= ACR_REGISTRY ?= myregistry.azurecr.io ACR_IMAGE ?= pg_durable -.PHONY: all build package install uninstall test test-unit test-e2e test-regress pg-clean docker-build docker-push pg-install META.json pgxn-zip install-pgrx pgrx-init +.PHONY: all build package install uninstall test test-unit test-e2e test-regress _test-regress pg-clean docker-build docker-push pg-install META.json pgxn-zip install-pgrx pgrx-init # Default target all: package @@ -52,7 +52,7 @@ test: # Run only pgrx unit tests test-unit: - ./scripts/test.sh --unit + ./scripts/test-unit.sh --pg-version $(subst pg,,$(PG_VERSION)) # Run only E2E tests (Docker-based) test-e2e: @@ -70,7 +70,7 @@ docker-push: docker-build # Run local development server run: - cargo pgrx run pg17 + python3 ./scripts/with-pg-lock.py cargo pgrx run pg17 # Clean build artifacts (renamed to avoid PGXS conflict) pg-clean: @@ -80,15 +80,19 @@ pg-clean: # Install extension locally (renamed to avoid PGXS conflict) pg-install: - cargo pgrx install + python3 ./scripts/with-pg-lock.py cargo pgrx install # Run pg_regress tests (convenience target) # Override version: make test-regress PG_VERSION=pg18 test-regress: + python3 ./scripts/with-pg-lock.py $(MAKE) _test-regress + +# Keep reset, installation, and installcheck in one critical section. +_test-regress: @echo "Resetting PostgreSQL..." ./scripts/pg-reset.sh $(subst pg,,$(PG_VERSION)) @echo "Starting PostgreSQL with PGDATABASE=contrib_regression..." - PGDATABASE=contrib_regression ./scripts/pg-start.sh --pg-version $(subst pg,,$(PG_VERSION)) + PGDATABASE=contrib_regression ./scripts/pg-start.sh --build --pg-version $(subst pg,,$(PG_VERSION)) @echo "Running pg_regress tests..." PGHOST=$(HOME)/.pgrx PGUSER=postgres PG_CONFIG=$$(cargo pgrx info pg-config $(PG_VERSION)) $(MAKE) -e installcheck diff --git a/README.md b/README.md index 3fa2c5bc..91e8b733 100644 --- a/README.md +++ b/README.md @@ -308,12 +308,16 @@ GRANT pg_durable_user TO app_backend, etl_service; All pull requests must pass the following checks before merging: 1. **Format Check** — `cargo fmt --check` -2. **Clippy & Tests** — `cargo clippy`, unit tests (`cargo pgrx test pg17`), pg_regress tests, and E2E tests +2. **Clippy & Tests** — `cargo clippy`, unit tests (`./scripts/test-unit.sh`), pg_regress tests, and E2E tests The CI workflow is defined in [.github/workflows/ci.yml](.github/workflows/ci.yml). It uses pgrx to download and manage PostgreSQL. ## Testing +Local test runners require Python 3 and automatically queue across worktrees +sharing PostgreSQL. See [concurrent local runs](docs/TESTING.md#concurrent-local-runs) +for details, including how to reserve the server while debugging with `--keep`. + pg_durable has two test suites: ### pg_regress Tests (Standard PostgreSQL Regression Tests) diff --git a/docs/E2E_TESTING.md b/docs/E2E_TESTING.md index 0f5c3e2d..d7d3090a 100644 --- a/docs/E2E_TESTING.md +++ b/docs/E2E_TESTING.md @@ -168,6 +168,11 @@ SELECT 'TEST PASSED: my_test_name' AS result; ## Debugging Failed Tests +`--keep` leaves PostgreSQL running but does not reserve it after the runner exits. +Another worktree's queued tests may stop or reset it. Use a +[locked debugging shell](TESTING.md#concurrent-local-runs) to keep exclusive +access throughout an investigation. + ### View PostgreSQL logs ```bash diff --git a/docs/TESTING.md b/docs/TESTING.md index 45b6ed77..d8c02631 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -14,6 +14,48 @@ This guide covers all testing scenarios for pg_durable. | Stop PostgreSQL | `./scripts/pg-stop.sh` | | Deploy to ACR | `./scripts/deploy-acr.sh` | +## Concurrent local runs + +Local test runners require Python 3 (standard library only). They automatically +take an exclusive file lock at `~/.pgrx/pg_durable-test.lock` before using the +shared PostgreSQL installation. You can start tests from several worktrees at +once: competing commands print a waiting message and queue instead of replacing +each other's extension binaries or resetting each other's databases. + +The lock covers installation, tests, and cleanup. Unit, local E2E, upgrade, +coverage, worker lifecycle, and aggregate runners use it, as do `make test-unit`, +`make test-regress`, `make pg-install`, `make run`, and the local start/stop/reset +scripts. Nested runners share the outer command's lock. All PostgreSQL versions +share one lock because some diagnostic files are also shared across versions. +Build/check-only commands and Docker tests do not take this local lock. + +This is cooperative locking: every participating worktree must contain the +updated runners. Direct `cargo pgrx` commands, `make installcheck`, and manual +database access do not automatically coordinate. Wrap such commands when they +use the shared local installation: + +```bash +python3 scripts/with-pg-lock.py cargo pgrx test pg17 +``` + +`--keep` still exits after the tests and leaves PostgreSQL running, but it does +**not** reserve that server. A queued command may immediately stop or reset it. +To keep exclusive access while debugging, open a locked shell and perform both +the test run and investigation inside it: + +```bash +python3 scripts/with-pg-lock.py bash +./scripts/test-e2e-local.sh --keep --verbose 05_monitoring_and_explain +# Inspect the database or logs here. +./scripts/pg-stop.sh +exit +``` + +The operating system releases the lock when the owning command exits, including +on failure. A server left running does not retain it. The empty lock file stays +on disk; do not delete it to unblock a waiter, since that would let two commands +lock different files at the same path. + ## Test Suites Overview pg_durable has **three test suites**: @@ -57,7 +99,7 @@ Runs Rust unit tests using `cargo pgrx test`. These test individual functions in Standard PostgreSQL regression tests for core DSL functionality. These tests are fast, deterministic, and use PostgreSQL's industry-standard testing framework. ```bash -# Recommended: reset the dedicated local cluster, install, and run all tests +# Recommended: lock the local cluster, reset, rebuild/install, and run all tests make test-regress # Run against an already-running disposable test cluster @@ -148,6 +190,11 @@ alongside the handwritten E2E suite. Existing CI runner checks and live E2E jobs cover all families without any generator or new CI job. **Investigation mode (`--keep`):** + +The lock is released when the runner exits. When sharing PostgreSQL with other +worktrees, use the [locked debugging shell](#concurrent-local-runs) to prevent a +queued run from replacing the server during investigation. + ```bash # Run tests, keep server running ./scripts/test-e2e-local.sh --keep 04_parallel diff --git a/scripts/pg-common.sh b/scripts/pg-common.sh index e362e7c2..5bf5ad41 100644 --- a/scripts/pg-common.sh +++ b/scripts/pg-common.sh @@ -4,6 +4,28 @@ PG_DURABLE_TEST_HTTP_DOMAINS='*.blob.core.windows.net,*.blob.storage.azure.net,*.queue.core.windows.net,*.table.core.windows.net,*.file.core.windows.net,*.azurewebsites.net,*.azurecontainerapps.io,*.azurecontainer.io,*.azure-api.net,*.documents.azure.com,*.servicebus.windows.net,*.openai.azure.com,*.cognitiveservices.azure.com,*.services.ai.azure.com,*.vault.azure.net,*.redis.cache.windows.net,*.database.windows.net,*.kusto.windows.net,*.azurefd.net,*.azureedge.net,*.azure-devices.net,*.trafficmanager.net,*.cloudapp.azure.com,api.github.com,httpbingo.org' +# Save arguments before parsing so lock acquisition can re-exec the same runner. +PG_DURABLE_LOCK_ARGS=("$@") + +lock_local_postgres() { + if [ "${_PG_DURABLE_LOCK_EXEC_PID:-}" = "$$" ]; then + unset _PG_DURABLE_LOCK_EXEC_PID + # Bash defers these traps until the foreground command finishes. Keep + # the lock while a build/test child is still using the installation. + trap 'exit 129' HUP + trap 'exit 130' INT + trap 'exit 143' TERM + return + fi + + exec python3 "$(dirname "${BASH_SOURCE[0]}")/with-pg-lock.py" \ + bash "$1" "${PG_DURABLE_LOCK_ARGS[@]}" +} + +warn_unreserved_postgres() { + echo "Warning: PostgreSQL is not reserved after the outer command exits; a queued run may stop or reset it." >&2 +} + resolve_pgrx_environment() { local pg_major="$1" diff --git a/scripts/pg-reset.sh b/scripts/pg-reset.sh index 668c6e9a..71e32a8d 100755 --- a/scripts/pg-reset.sh +++ b/scripts/pg-reset.sh @@ -15,6 +15,10 @@ PG_MAJOR="${1:-17}" SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +. "$SCRIPT_DIR/pg-common.sh" +lock_local_postgres "$SCRIPT_DIR/pg-reset.sh" + DATA_DIR="$HOME/.pgrx/data-$PG_MAJOR" cd "$PROJECT_DIR" @@ -31,4 +35,3 @@ rm -f "$HOME/.pgrx/${PG_MAJOR}.log" echo -e "\033[0;32mPostgreSQL data and logs wiped.\033[0m" echo "" echo "Run ./scripts/pg-start.sh to start fresh." - diff --git a/scripts/pg-start.sh b/scripts/pg-start.sh index 737cbea9..8fbcd636 100755 --- a/scripts/pg-start.sh +++ b/scripts/pg-start.sh @@ -12,6 +12,12 @@ set -e +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +# shellcheck source=./pg-common.sh +. "$SCRIPT_DIR/pg-common.sh" + PG_MAJOR="${PG_MAJOR:-17}" BUILD_MODE="auto" @@ -52,12 +58,7 @@ while [[ $# -gt 0 ]]; do esac done -SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" -PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" - -# shellcheck source=./pg-common.sh -. "$SCRIPT_DIR/pg-common.sh" - +lock_local_postgres "$SCRIPT_DIR/pg-start.sh" resolve_pgrx_environment "$PG_MAJOR" cd "$PROJECT_DIR" @@ -76,7 +77,7 @@ fi if [ "$BUILD_MODE" != "skip" ]; then echo -e "\033[0;33mBuilding and installing extension...\033[0m" - cargo pgrx install --pg-config "$PG_CONFIG" 2>&1 | grep -v "^warning:" || true + cargo pgrx install --pg-config "$PG_CONFIG" fi echo -e "\033[0;33mPreparing PostgreSQL data directory...\033[0m" @@ -99,4 +100,4 @@ echo " tail -f ~/.pgrx/${PG_MAJOR}.log" echo "" echo -e "\033[0;36mStop:\033[0m" echo " ./scripts/pg-stop.sh" - +warn_unreserved_postgres diff --git a/scripts/pg-stop.sh b/scripts/pg-stop.sh index 07df8986..2f827a73 100755 --- a/scripts/pg-stop.sh +++ b/scripts/pg-stop.sh @@ -14,6 +14,9 @@ set -e +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + STOP_LOCAL=false STOP_DOCKER=false PG_MAJOR="${PG_MAJOR:-17}" @@ -58,6 +61,8 @@ NC='\033[0m' # Stop local pgrx PostgreSQL if [ "$STOP_LOCAL" = true ]; then + lock_local_postgres "$SCRIPT_DIR/pg-stop.sh" + PGRX_HOME="$HOME/.pgrx" PG_CTL=$(ls $PGRX_HOME/$PG_MAJOR.*/pgrx-install/bin/pg_ctl 2>/dev/null | head -1) DATA_DIR="$PGRX_HOME/data-$PG_MAJOR" @@ -96,4 +101,3 @@ if [ "$STOP_DOCKER" = true ]; then echo "Docker container is not running" fi fi - diff --git a/scripts/test-all-local.sh b/scripts/test-all-local.sh index ddabd4b6..abe93c5e 100755 --- a/scripts/test-all-local.sh +++ b/scripts/test-all-local.sh @@ -22,6 +22,8 @@ set -e SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + cd "$PROJECT_DIR" # Defaults @@ -52,6 +54,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-all-local.sh" + # Colors RED='\033[0;31m' GREEN='\033[0;32m' diff --git a/scripts/test-coverage.sh b/scripts/test-coverage.sh index bd1daad5..4e534569 100644 --- a/scripts/test-coverage.sh +++ b/scripts/test-coverage.sh @@ -35,6 +35,8 @@ set -e SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + # Defaults HTML=false WITH_UNIT=false @@ -70,6 +72,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-coverage.sh" + # Colors RED='\033[0;31m' GREEN='\033[0;32m' diff --git a/scripts/test-e2e-local.sh b/scripts/test-e2e-local.sh index 9b7672e1..7b3e74c0 100755 --- a/scripts/test-e2e-local.sh +++ b/scripts/test-e2e-local.sh @@ -313,6 +313,8 @@ if ! [[ "$REPEAT_COUNT" =~ ^[0-9]+$ ]] || [ "$REPEAT_COUNT" -lt 1 ]; then exit 1 fi +lock_local_postgres "$SCRIPT_DIR/test-e2e-local.sh" + PG_PORT="$((28800 + PG_VERSION))" DATA_DIR="$PGRX_HOME/data-$PG_VERSION" LOG_FILE="$PGRX_HOME/$PG_VERSION.log" @@ -363,6 +365,7 @@ cleanup() { echo "Connect: $PSQL -h localhost -p $PG_PORT -d $PG_DB" echo "Logs: tail -f $LOG_FILE" echo "Stop: ./scripts/pg-stop.sh" + warn_unreserved_postgres } trap cleanup EXIT diff --git a/scripts/test-epoch-race.sh b/scripts/test-epoch-race.sh index a509e580..5130cde8 100755 --- a/scripts/test-epoch-race.sh +++ b/scripts/test-epoch-race.sh @@ -34,6 +34,8 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + PG_VERSION="${PG_VERSION:-17}" PAUSE_MS=4000 VERBOSE=false @@ -54,6 +56,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-epoch-race.sh" + PG_PORT="$((28800 + PG_VERSION))" PGRX_HOME="$HOME/.pgrx" DATA_DIR="$PGRX_HOME/data-$PG_VERSION" diff --git a/scripts/test-shutdown.sh b/scripts/test-shutdown.sh index b0448e97..ef96af55 100755 --- a/scripts/test-shutdown.sh +++ b/scripts/test-shutdown.sh @@ -27,6 +27,8 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + PG_VERSION="${PG_VERSION:-17}" STOP_TIMEOUT=20 # A clean stop measures ~1.2s, dominated by SHUTDOWN_CHECK_INTERVAL. 3s leaves @@ -55,6 +57,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-shutdown.sh" + PG_PORT="$((28800 + PG_VERSION))" PGRX_HOME="$HOME/.pgrx" DATA_DIR="$PGRX_HOME/data-$PG_VERSION" diff --git a/scripts/test-unit.sh b/scripts/test-unit.sh index c0ee4a2c..1cce5429 100755 --- a/scripts/test-unit.sh +++ b/scripts/test-unit.sh @@ -16,7 +16,10 @@ set -e -cd "$(dirname "$0")/.." +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + +cd "$SCRIPT_DIR/.." PG_VERSION="17" TEST_FILTER="" @@ -40,6 +43,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-unit.sh" + echo "================================================" echo "pg_durable Unit Tests (pgrx) — PG${PG_VERSION}" if [ -n "$FEATURES" ]; then @@ -59,4 +64,3 @@ if [ -n "$TEST_FILTER" ]; then else cargo pgrx test "pg${PG_VERSION}" $FEATURES_ARG fi - diff --git a/scripts/test-upgrade.sh b/scripts/test-upgrade.sh index 28ddd2f5..2544483f 100755 --- a/scripts/test-upgrade.sh +++ b/scripts/test-upgrade.sh @@ -28,6 +28,8 @@ set -e SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" PROJECT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" +. "$SCRIPT_DIR/pg-common.sh" + # Defaults PG_VERSION="17" KEEP_RUNNING=false @@ -59,6 +61,8 @@ while [[ $# -gt 0 ]]; do esac done +lock_local_postgres "$SCRIPT_DIR/test-upgrade.sh" + # pgrx settings PGRX_HOME="$HOME/.pgrx" PG_PORT="$((28800 + PG_VERSION))" @@ -257,6 +261,7 @@ cleanup() { echo -e "${GREEN}PostgreSQL left running on port $PG_PORT${NC}" echo "Connect: $PSQL -h localhost -p $PG_PORT -d $PG_DB" echo "Stop: ./scripts/pg-stop.sh" + warn_unreserved_postgres fi } trap cleanup EXIT diff --git a/scripts/with-pg-lock.py b/scripts/with-pg-lock.py new file mode 100644 index 00000000..0bdd0318 --- /dev/null +++ b/scripts/with-pg-lock.py @@ -0,0 +1,78 @@ +#!/usr/bin/env python3 +# Copyright (c) Microsoft Corporation. +# Licensed under the PostgreSQL License. +"""Run a command with exclusive access to the user's shared test PostgreSQL.""" + +import argparse +import errno +import fcntl +import os +from pathlib import Path +import sys + + +def inherited_lock(lock_path): + if os.environ.get("_PG_DURABLE_LOCK_PATH") != str(lock_path): + return False + try: + owner = int(os.environ.get("_PG_DURABLE_LOCK_OWNER", "0")) + except ValueError: + return False + if owner <= 0: + return False + try: + os.kill(owner, 0) + except ProcessLookupError: + return False + return True + + +def exec_command(command): + os.environ["_PG_DURABLE_LOCK_EXEC_PID"] = str(os.getpid()) + try: + os.execvp(command[0], command) + except OSError as error: + print(f"pg_durable: cannot execute {command[0]}: {error}", file=sys.stderr) + sys.exit(127 if error.errno == errno.ENOENT else 126) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("command", nargs=argparse.REMAINDER) + command = parser.parse_args().command + if command[:1] == ["--"]: + command = command[1:] + if not command: + parser.error("a command is required") + + # Use one lock for all versions: runners also share version-independent logs. + lock_path = (Path.home() / ".pgrx" / "pg_durable-test.lock").resolve() + if inherited_lock(lock_path): + exec_command(command) + + lock_path.parent.mkdir(parents=True, exist_ok=True) + with lock_path.open("a") as lock: + try: + fcntl.lockf(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + print(f"pg_durable: waiting for shared PostgreSQL lock: {lock_path}", + file=sys.stderr, flush=True) + fcntl.lockf(lock, fcntl.LOCK_EX) + + # POSIX record locks survive exec, but are not inherited by forked + # processes. Unlike flock, a postmaster left by --keep cannot retain this + # lock after the runner exits. Exec also preserves signals and exit codes. + os.set_inheritable(lock.fileno(), True) + os.environ["_PG_DURABLE_LOCK_PATH"] = str(lock_path) + os.environ["_PG_DURABLE_LOCK_OWNER"] = str(os.getpid()) + exec_command(command) + + +if __name__ == "__main__": + try: + main() + except KeyboardInterrupt: + sys.exit(130) + except OSError as error: + print(f"pg_durable: cannot acquire PostgreSQL test lock: {error}", file=sys.stderr) + sys.exit(1) diff --git a/tests/test_pg_lock.py b/tests/test_pg_lock.py new file mode 100644 index 00000000..00a3c0a2 --- /dev/null +++ b/tests/test_pg_lock.py @@ -0,0 +1,360 @@ +#!/usr/bin/env python3 +# Copyright (c) Microsoft Corporation. +# Licensed under the PostgreSQL License. +"""Test cross-worktree PostgreSQL coordination without PostgreSQL or Cargo.""" + +import fcntl +import json +import os +from pathlib import Path +import select +import shutil +import signal +import subprocess +import sys +import tempfile +import time +import unittest + + +ROOT = Path(__file__).resolve().parents[1] +WRAPPER = ROOT / "scripts" / "with-pg-lock.py" +RUNNERS = [ + "test-unit.sh", "test-e2e-local.sh", "test-upgrade.sh", "test-all-local.sh", + "test-coverage.sh", "test-shutdown.sh", "test-epoch-race.sh", + "pg-start.sh", "pg-stop.sh", "pg-reset.sh", +] +HOLD = "import sys; print('entered', flush=True); sys.stdin.readline()" + + +class PostgresLockTests(unittest.TestCase): + def setUp(self): + temp = tempfile.TemporaryDirectory() + self.addCleanup(temp.cleanup) + self.root = Path(temp.name) + self.home = self.root / "home" + self.lock_path = self.home / ".pgrx" / "pg_durable-test.lock" + self.lock_path.parent.mkdir(parents=True) + self.env = { + key: value for key, value in os.environ.items() + if not key.startswith("_PG_DURABLE_LOCK_") + } + self.env.update(HOME=str(self.home), PYTHONUNBUFFERED="1") + self.bin = self.root / "bin" + self.bin.mkdir() + self.env["PATH"] = str(self.bin) + os.pathsep + self.env["PATH"] + self.write_program(self.bin / "cargo", """ +import json, os, sys +from pathlib import Path +if os.environ.get("CARGO_RECORD"): + Path(os.environ["CARGO_RECORD"]).write_text(json.dumps({ + "args": sys.argv[1:], "cwd": os.getcwd(), + })) +if os.environ.get("HOLD_CARGO"): + sys.stdin.readline() +sys.exit(int(os.environ.get("CARGO_EXIT", "0"))) +""") + + def write_program(self, path, code): + path.write_text(f"#!{sys.executable}\n{code}", encoding="utf-8") + path.chmod(0o755) + + def worktree(self, name): + root = self.root / name + scripts = root / "scripts" + scripts.mkdir(parents=True) + for name in [*RUNNERS, "pg-common.sh", "with-pg-lock.py"]: + shutil.copy2(ROOT / "scripts" / name, scripts / name) + shutil.copy2(ROOT / "Makefile", root / "Makefile") + return root + + def start(self, command, *, cwd=None, env=None, wrapped=True): + if wrapped: + command = [sys.executable, str(WRAPPER), *command] + process = subprocess.Popen( + command, cwd=cwd, env={**self.env, **(env or {})}, + stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + text=True, start_new_session=True, + ) + + def cleanup(): + # Each fixture owns its process group, including any simulated server. + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass + process.communicate(timeout=10) + + self.addCleanup(cleanup) + return process + + def read_line(self, stream): + self.assertTrue(select.select([stream], [], [], 10)[0], "Timed out waiting for output") + return stream.readline().strip() + + def assert_waiting(self, process): + self.assertIn("waiting for shared PostgreSQL lock:", self.read_line(process.stderr)) + self.assertIsNone(process.poll()) + + def assert_finished(self, process, code=0, input=None): + output, error = process.communicate(input=input, timeout=10) + self.assertEqual(process.returncode, code, output + error) + return output, error + + def wait_for_file(self, path): + deadline = time.monotonic() + 10 + while not path.exists(): + self.assertLess(time.monotonic(), deadline, f"Timed out waiting for {path}") + time.sleep(0.01) + + def hold_lock(self): + lock = self.lock_path.open("a") + self.addCleanup(lock.close) + fcntl.lockf(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + return lock + + def test_worktrees_queue_even_for_different_postgres_versions(self): + first = self.worktree("first worktree") + second = self.worktree("second worktree") + first_record = self.root / "first.json" + second_record = self.root / "second.json" + holder = self.start( + ["bash", "./test-unit.sh", "--pg-version", "18", + "--features", "test-hooks", "filter with spaces"], + cwd=first / "scripts", wrapped=False, + env={"CARGO_RECORD": str(first_record), "HOLD_CARGO": "1", "CARGO_EXIT": "23"}, + ) + self.wait_for_file(first_record) + waiter = self.start( + ["bash", "scripts/test-unit.sh", "--pg-version", "17", "second"], + cwd=second, wrapped=False, env={"CARGO_RECORD": str(second_record)}, + ) + self.assert_waiting(waiter) + self.assertFalse(second_record.exists()) + self.assert_finished(holder, 23, input="\n") + self.assert_finished(waiter) + self.assertEqual(json.loads(first_record.read_text()), { + "args": ["pgrx", "test", "pg18", "--features", "test-hooks", "--", "filter with spaces"], + "cwd": str(first), + }) + self.assertEqual(json.loads(second_record.read_text())["args"], + ["pgrx", "test", "pg17", "--", "second"]) + + def test_nested_runners_keep_the_outer_lock(self): + worktree = self.worktree("nested") + holder = self.start([sys.executable, "-c", """ +import subprocess, sys +for _ in range(2): + subprocess.run(["bash", "scripts/test-unit.sh", "nested"], + check=True, stdout=subprocess.DEVNULL) +print("entered", flush=True) +sys.stdin.readline() +"""], cwd=worktree) + self.assertEqual(self.read_line(holder.stdout), "entered") + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + self.assert_finished(holder, input="\n") + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_surviving_server_does_not_retain_lock(self): + pid_file = self.root / "server.pid" + holder = self.start([sys.executable, "-c", """ +import subprocess, sys +from pathlib import Path +child = subprocess.Popen( + [sys.executable, "-c", "import time; time.sleep(60)"], + close_fds=False, stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, +) +Path(sys.argv[1]).write_text(str(child.pid)) +""", str(pid_file)]) + self.assert_finished(holder) + server_pid = int(pid_file.read_text()) + os.kill(server_pid, 0) + next_run = self.start([sys.executable, "-c", "print('next')"]) + self.assertEqual(self.assert_finished(next_run)[0].strip(), "next") + os.kill(server_pid, 0) + self.assertTrue(self.lock_path.exists(), "Never unlink a reusable lock file") + + def test_exit_cleanup_remains_locked(self): + holder = self.start(["bash", "-c", """ +trap 'echo cleanup; read -r line' EXIT +exit 7 +"""]) + self.assertEqual(self.read_line(holder.stdout), "cleanup") + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + self.assert_finished(holder, 7, input="\n") + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_signals_release_the_owner_lock(self): + for sig in (signal.SIGTERM, signal.SIGINT, signal.SIGKILL): + with self.subTest(signal=sig): + holder = self.start([sys.executable, "-c", HOLD]) + self.assertEqual(self.read_line(holder.stdout), "entered") + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + holder.send_signal(sig) + holder.communicate(timeout=10) + self.assertNotEqual(holder.returncode, 0) + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_signalled_runner_keeps_lock_until_foreground_child_finishes(self): + worktree = self.worktree("interrupted unit") + for sig in (signal.SIGHUP, signal.SIGINT, signal.SIGTERM): + with self.subTest(signal=sig): + record = self.root / f"cargo-{sig}.json" + holder = self.start( + ["bash", "scripts/test-unit.sh"], cwd=worktree, wrapped=False, + env={"CARGO_RECORD": str(record), "HOLD_CARGO": "1"}, + ) + self.wait_for_file(record) + holder.send_signal(sig) + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + self.assert_finished(holder, 128 + sig, input="\n") + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_interrupted_waiter_never_executes_command(self): + lock = self.hold_lock() + marker = self.root / "unexpected" + waiter = self.start([sys.executable, "-c", + "from pathlib import Path; import sys; Path(sys.argv[1]).touch()", + str(marker)]) + self.assert_waiting(waiter) + waiter.send_signal(signal.SIGINT) + self.assert_finished(waiter, 130) + lock.close() + self.assertFalse(marker.exists()) + self.assert_finished(self.start([sys.executable, "-c", "pass"])) + + def test_stale_inherited_owner_does_not_bypass_lock(self): + old = self.start([sys.executable, "-c", + "import json, os; print(json.dumps(dict(os.environ)))"]) + inherited = json.loads(self.assert_finished(old)[0]) + lock = self.hold_lock() + waiter = self.start([sys.executable, "-c", "pass"], env=inherited) + self.assert_waiting(waiter) + lock.close() + self.assert_finished(waiter) + + def test_independent_homes_do_not_block(self): + holder = self.start([sys.executable, "-c", HOLD]) + self.assertEqual(self.read_line(holder.stdout), "entered") + self.assert_finished(self.start( + [sys.executable, "-c", "pass"], env={"HOME": str(self.root / "other home")}, + )) + self.assertIsNone(holder.poll()) + self.assert_finished(holder, input="\n") + + def test_lock_errors_fail_before_command(self): + other_home = self.root / "invalid home" + other_home.mkdir() + (other_home / ".pgrx").touch() + process = self.start([sys.executable, "-c", "print('unexpected')"], + env={"HOME": str(other_home)}) + output, error = self.assert_finished(process, 1) + self.assertEqual(output, "") + self.assertIn("cannot acquire PostgreSQL test lock:", error) + + def test_exec_failure_releases_lock(self): + process = self.start(["/nonexistent-pg-durable-test-command"]) + self.assertIn("cannot execute", self.assert_finished(process, 127)[1]) + self.assert_finished(self.start([sys.executable, "-c", "pass"])) + + def test_all_local_entry_points_wait_before_touching_postgres(self): + worktree = self.worktree("entrypoints") + self.hold_lock() + for runner in RUNNERS: + with self.subTest(runner=runner): + process = self.start(["bash", f"scripts/{runner}"], + cwd=worktree, wrapped=False) + self.assert_waiting(process) + process.send_signal(signal.SIGTERM) + process.communicate(timeout=10) + self.assertEqual(process.returncode, -signal.SIGTERM) + for target in ("test-unit", "test-regress", "pg-install", "run"): + with self.subTest(target=target): + process = self.start(["make", "--no-print-directory", target], + cwd=worktree, wrapped=False) + self.assert_waiting(process) + os.killpg(process.pid, signal.SIGTERM) + process.communicate(timeout=10) + self.assertNotEqual(process.returncode, 0) + + def test_help_does_not_wait(self): + worktree = self.worktree("help") + self.hold_lock() + for runner in ("test-e2e-local.sh", "pg-start.sh", + "test-shutdown.sh", "test-epoch-race.sh"): + with self.subTest(runner=runner): + process = self.start(["bash", f"scripts/{runner}", "--help"], + cwd=worktree, wrapped=False) + output, error = self.assert_finished(process) + self.assertIn("Usage:", output) + self.assertNotIn("waiting for", error) + + def test_docker_only_stop_does_not_wait(self): + worktree = self.worktree("docker") + self.write_program(self.bin / "docker", "") + self.hold_lock() + self.assert_finished(self.start(["bash", "scripts/pg-stop.sh", "--docker"], + cwd=worktree, wrapped=False)) + + def test_forced_install_failure_does_not_start_another_worktrees_binary(self): + worktree = self.worktree("failed build") + config = self.lock_path.parent / "config.toml" + config.write_text(f'[configs]\npg17 = "{self.bin / "pg_config"}"\n') + process = self.start(["bash", "scripts/pg-start.sh", "--build"], + cwd=worktree, wrapped=False, env={"CARGO_EXIT": "23"}) + self.assert_finished(process, 23) + self.assertFalse((self.lock_path.parent / "data-17").exists()) + + def test_make_regress_holds_lock_through_installcheck(self): + worktree = self.worktree("regress") + record = self.root / "regress.jsonl" + probe = """ +import fcntl, json, os, sys +with open(os.environ["_PG_DURABLE_LOCK_PATH"], "a") as lock: + try: + fcntl.lockf(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + pass + else: + sys.exit("regression step ran without the outer lock") +with open(os.environ["REGRESS_RECORD"], "a") as record: + record.write(json.dumps(sys.argv) + "\\n") +""" + for helper in ("pg-reset.sh", "pg-start.sh"): + self.write_program(worktree / "scripts" / helper, probe) + make = self.bin / "nested-make" + self.write_program(make, """ +import os, sys +if sys.argv[1:] == ["_test-regress"]: + os.execv(os.environ["REAL_MAKE"], [os.environ["REAL_MAKE"], "_test-regress"]) +""" + probe + """ +print("entered", flush=True) +sys.stdin.readline() +""") + holder = self.start( + ["make", "--no-print-directory", "test-regress", f"MAKE={make}", "PG_VERSION=pg18"], + cwd=worktree, wrapped=False, + env={"REGRESS_RECORD": str(record), "REAL_MAKE": shutil.which("make")}, + ) + deadline = time.monotonic() + 10 + while not record.exists() or len(record.read_text().splitlines()) < 3: + self.assertIsNone(holder.poll(), "make exited before installcheck") + self.assertLess(time.monotonic(), deadline, "make never reached installcheck") + time.sleep(0.01) + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + self.assert_finished(holder, input="\n") + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + calls = [json.loads(line) for line in record.read_text().splitlines()] + self.assertEqual(calls[0][1:], ["18"]) + self.assertEqual(calls[1][1:], ["--build", "--pg-version", "18"]) + self.assertEqual(calls[2][1:], ["-e", "installcheck"]) + + +if __name__ == "__main__": + unittest.main() From af9c728fbd286975ac5887e553ffc3435f8013c9 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 28 Oct 2026 12:02:53 -0400 Subject: [PATCH 2/4] Resolve temporary directory path in setUp method Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- tests/test_pg_lock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_pg_lock.py b/tests/test_pg_lock.py index 00a3c0a2..d0416525 100644 --- a/tests/test_pg_lock.py +++ b/tests/test_pg_lock.py @@ -31,7 +31,7 @@ class PostgresLockTests(unittest.TestCase): def setUp(self): temp = tempfile.TemporaryDirectory() self.addCleanup(temp.cleanup) - self.root = Path(temp.name) + self.root = Path(temp.name).resolve() self.home = self.root / "home" self.lock_path = self.home / ".pgrx" / "pg_durable-test.lock" self.lock_path.parent.mkdir(parents=True) From 25d8e11447ee39e9ffe2b8cc356a838d969fe088 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 28 Oct 2026 16:50:25 +0000 Subject: [PATCH 3/4] Fix CI --- scripts/pg-common.sh | 3 +- scripts/test-e2e-local.sh | 2 +- scripts/test-epoch-race.sh | 2 +- scripts/test-shutdown.sh | 2 +- tests/test_pg_lock.py | 67 +++++++++++++++++++++++++++++++++----- 5 files changed, 63 insertions(+), 13 deletions(-) diff --git a/scripts/pg-common.sh b/scripts/pg-common.sh index 5bf5ad41..3cfe5096 100644 --- a/scripts/pg-common.sh +++ b/scripts/pg-common.sh @@ -18,8 +18,9 @@ lock_local_postgres() { return fi + # Bash 3.2 treats an empty array as unset under nounset. exec python3 "$(dirname "${BASH_SOURCE[0]}")/with-pg-lock.py" \ - bash "$1" "${PG_DURABLE_LOCK_ARGS[@]}" + bash "$1" ${PG_DURABLE_LOCK_ARGS[@]+"${PG_DURABLE_LOCK_ARGS[@]}"} } warn_unreserved_postgres() { diff --git a/scripts/test-e2e-local.sh b/scripts/test-e2e-local.sh index 7b3e74c0..c0e77ccc 100755 --- a/scripts/test-e2e-local.sh +++ b/scripts/test-e2e-local.sh @@ -110,7 +110,7 @@ CYAN='\033[0;36m' NC='\033[0m' print_usage() { - sed -n '/^# BEGIN_USAGE/,/^# END_USAGE/{ /^# BEGIN_USAGE/d; /^# END_USAGE/d; s/^# \{0,1\}//; p }' "$0" + sed -n '/^# BEGIN_USAGE/,/^# END_USAGE/{ /^# BEGIN_USAGE/d; /^# END_USAGE/d; s/^# \{0,1\}//; p; }' "$0" } contains_value() { diff --git a/scripts/test-epoch-race.sh b/scripts/test-epoch-race.sh index 5130cde8..046e48c7 100755 --- a/scripts/test-epoch-race.sh +++ b/scripts/test-epoch-race.sh @@ -49,7 +49,7 @@ while [[ $# -gt 0 ]]; do --verbose|-v) VERBOSE=true; shift ;; --help|-h) - sed -n '/^# Usage:/,/^[^#]/{ /^[^#]/d; s/^# \{0,1\}//; p }' "$0" + sed -n '/^# Usage:/,/^[^#]/{ /^[^#]/d; s/^# \{0,1\}//; p; }' "$0" exit 0 ;; *) echo "Unknown option: $1"; exit 1 ;; diff --git a/scripts/test-shutdown.sh b/scripts/test-shutdown.sh index ef96af55..0ee422dd 100755 --- a/scripts/test-shutdown.sh +++ b/scripts/test-shutdown.sh @@ -50,7 +50,7 @@ while [[ $# -gt 0 ]]; do --verbose|-v) VERBOSE=true; shift ;; --help|-h) - sed -n '/^# Usage:/,/^[^#]/{ /^[^#]/d; s/^# \{0,1\}//; p }' "$0" + sed -n '/^# Usage:/,/^[^#]/{ /^[^#]/d; s/^# \{0,1\}//; p; }' "$0" exit 0 ;; *) echo "Unknown option: $1"; exit 1 ;; diff --git a/tests/test_pg_lock.py b/tests/test_pg_lock.py index d0416525..e1aac9e2 100644 --- a/tests/test_pg_lock.py +++ b/tests/test_pg_lock.py @@ -15,6 +15,7 @@ import tempfile import time import unittest +from unittest.mock import Mock, call, patch ROOT = Path(__file__).resolve().parents[1] @@ -27,6 +28,46 @@ HOLD = "import sys; print('entered', flush=True); sys.stdin.readline()" +def cleanup_process(process): + # Each fixture owns its process group, including any simulated server. + try: + try: + os.killpg(process.pid, signal.SIGKILL) + except PermissionError: + # macOS rejects zombie-only groups. Reap our child before retrying. + if process.poll() is None: + raise + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass + process.communicate(timeout=10) + + +class ProcessCleanupTests(unittest.TestCase): + def test_reaps_zombie_before_retrying_group_cleanup(self): + for result in (None, ProcessLookupError()): + with self.subTest(result=result): + process = Mock(pid=1234) + process.poll.return_value = 0 + with patch("os.killpg", side_effect=[PermissionError(), result]) as kill_group: + cleanup_process(process) + self.assertEqual(kill_group.call_args_list, + [call(1234, signal.SIGKILL), call(1234, signal.SIGKILL)]) + process.poll.assert_called_once_with() + process.communicate.assert_called_once_with(timeout=10) + + def test_group_permission_errors_are_not_suppressed(self): + for returncode in (None, 0): + with self.subTest(returncode=returncode): + process = Mock(pid=1234) + process.poll.return_value = returncode + with patch("os.killpg", side_effect=PermissionError) as kill_group: + with self.assertRaises(PermissionError): + cleanup_process(process) + self.assertEqual(kill_group.call_count, 1 if returncode is None else 2) + process.communicate.assert_not_called() + + class PostgresLockTests(unittest.TestCase): def setUp(self): temp = tempfile.TemporaryDirectory() @@ -77,15 +118,7 @@ def start(self, command, *, cwd=None, env=None, wrapped=True): text=True, start_new_session=True, ) - def cleanup(): - # Each fixture owns its process group, including any simulated server. - try: - os.killpg(process.pid, signal.SIGKILL) - except ProcessLookupError: - pass - process.communicate(timeout=10) - - self.addCleanup(cleanup) + self.addCleanup(cleanup_process, process) return process def read_line(self, stream): @@ -113,6 +146,22 @@ def hold_lock(self): fcntl.lockf(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) return lock + def test_strict_runner_preserves_empty_and_quoted_arguments(self): + worktree = self.worktree("arguments") + runner = worktree / "scripts" / "lock-args.sh" + runner.write_text(""" +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +. "$SCRIPT_DIR/pg-common.sh" +lock_local_postgres "$SCRIPT_DIR/lock-args.sh" +python3 -c 'import json, sys; print(json.dumps(sys.argv[1:]))' "$@" +""") + for args in ([], [""], ["two words", '"quoted"', "*", "$HOME", "\u03bb"]): + with self.subTest(args=args): + process = self.start(["bash", str(runner), *args], wrapped=False) + output, _ = self.assert_finished(process) + self.assertEqual(json.loads(output), args) + def test_worktrees_queue_even_for_different_postgres_versions(self): first = self.worktree("first worktree") second = self.worktree("second worktree") From cc8db51830254b7733f4088a789c4546778dece1 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 28 Oct 2026 18:55:01 +0000 Subject: [PATCH 4/4] Fix signal handling --- .github/copilot-instructions.md | 5 +- CONTRIBUTING.md | 3 +- Makefile | 5 +- README.md | 2 +- docs/E2E_TESTING.md | 5 +- docs/TESTING.md | 48 ++++++---- scripts/with-pg-lock.py | 139 +++++++++++++++++++++++++++-- tests/test_pg_lock.py | 152 ++++++++++++++++++++++++++++++-- 8 files changed, 319 insertions(+), 40 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 607a6eca..c0fc713c 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -61,9 +61,10 @@ Local runners require Python 3 and automatically coordinate across worktrees using a shared PostgreSQL file lock. Use the scripts above rather than raw `cargo pgrx test`; a waiting message is normal when another run owns the lock. For low-level commands, use `python3 scripts/with-pg-lock.py [args...]`. +The wrapper supports non-interactive commands only, including `bash -c`. Never delete the lock file to bypass a waiter. `--keep` does not reserve the -server after the outer command exits; use a -[locked debugging shell](../docs/TESTING.md#concurrent-local-runs) when needed. +server after the outer command exits; group non-interactive diagnostics into the +[same locked command](../docs/TESTING.md#concurrent-local-runs) when needed. Build/check-only commands do not need the lock. ## Critical Patterns diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 623eb8f9..91dd1d71 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -56,7 +56,8 @@ can still run in parallel. `--keep` leaves the server running but releases the lock when the command exits; a queued run may then stop or reset it. See [concurrent local runs](docs/TESTING.md#concurrent-local-runs) for wrapping -low-level commands or reserving PostgreSQL throughout a debugging session. +non-interactive commands and diagnostics. Interactive investigations require +coordination with other worktrees. ### Full pre-PR checks diff --git a/Makefile b/Makefile index 2db08bfa..0d448b91 100644 --- a/Makefile +++ b/Makefile @@ -70,7 +70,8 @@ docker-push: docker-build # Run local development server run: - python3 ./scripts/with-pg-lock.py cargo pgrx run pg17 + @echo "Warning: interactive development is not protected by the test lock; coordinate with other worktrees." >&2 + cargo pgrx run pg17 # Clean build artifacts (renamed to avoid PGXS conflict) pg-clean: @@ -110,7 +111,7 @@ help: @echo " installcheck - Run pg_regress tests (requires PostgreSQL running, via PGXS)" @echo " docker-build - Build Docker image" @echo " docker-push - Build and push to ACR" - @echo " run - Start local pgrx dev server" + @echo " run - Start an interactive pgrx dev session (not locked)" @echo " pg-clean - Clean build artifacts" @echo " install - Install prebuilt artifacts for the selected PostgreSQL" @echo " uninstall - Remove installed artifacts for the selected PostgreSQL" diff --git a/README.md b/README.md index 91e8b733..43b2db23 100644 --- a/README.md +++ b/README.md @@ -316,7 +316,7 @@ The CI workflow is defined in [.github/workflows/ci.yml](.github/workflows/ci.ym Local test runners require Python 3 and automatically queue across worktrees sharing PostgreSQL. See [concurrent local runs](docs/TESTING.md#concurrent-local-runs) -for details, including how to reserve the server while debugging with `--keep`. +for queueing behavior, cancellation, and `--keep` limitations. pg_durable has two test suites: diff --git a/docs/E2E_TESTING.md b/docs/E2E_TESTING.md index d7d3090a..86532c78 100644 --- a/docs/E2E_TESTING.md +++ b/docs/E2E_TESTING.md @@ -170,8 +170,9 @@ SELECT 'TEST PASSED: my_test_name' AS result; `--keep` leaves PostgreSQL running but does not reserve it after the runner exits. Another worktree's queued tests may stop or reset it. Use a -[locked debugging shell](TESTING.md#concurrent-local-runs) to keep exclusive -access throughout an investigation. +[locked non-interactive command](TESTING.md#concurrent-local-runs) for checks and +diagnostics that need exclusive access. Coordinate with other worktrees before +investigating interactively. ### View PostgreSQL logs diff --git a/docs/TESTING.md b/docs/TESTING.md index d8c02631..c2a829b7 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -16,7 +16,8 @@ This guide covers all testing scenarios for pg_durable. ## Concurrent local runs -Local test runners require Python 3 (standard library only). They automatically +Local test runners require Python 3 (standard library only) and the system `ps` +command, available on Linux and macOS. They automatically take an exclusive file lock at `~/.pgrx/pg_durable-test.lock` before using the shared PostgreSQL installation. You can start tests from several worktrees at once: competing commands print a waiting message and queue instead of replacing @@ -24,7 +25,7 @@ each other's extension binaries or resetting each other's databases. The lock covers installation, tests, and cleanup. Unit, local E2E, upgrade, coverage, worker lifecycle, and aggregate runners use it, as do `make test-unit`, -`make test-regress`, `make pg-install`, `make run`, and the local start/stop/reset +`make test-regress`, `make pg-install`, and the local start/stop/reset scripts. Nested runners share the outer command's lock. All PostgreSQL versions share one lock because some diagnostic files are also shared across versions. Build/check-only commands and Docker tests do not take this local lock. @@ -38,23 +39,35 @@ use the shared local installation: python3 scripts/with-pg-lock.py cargo pgrx test pg17 ``` -`--keep` still exits after the tests and leaves PostgreSQL running, but it does -**not** reserve that server. A queued command may immediately stop or reset it. -To keep exclusive access while debugging, open a locked shell and perform both -the test run and investigation inside it: +The wrapper supports **non-interactive commands only**, including `bash -c`. +Group checks and non-interactive diagnostics into one command to keep the lock +between them: ```bash -python3 scripts/with-pg-lock.py bash -./scripts/test-e2e-local.sh --keep --verbose 05_monitoring_and_explain -# Inspect the database or logs here. -./scripts/pg-stop.sh -exit +python3 scripts/with-pg-lock.py bash -e -c \ + './scripts/test-unit.sh; ./scripts/test-e2e-local.sh' ``` -The operating system releases the lock when the owning command exits, including -on failure. A server left running does not retain it. The empty lock file stays -on disk; do not delete it to unblock a waiter, since that would let two commands -lock different files at the same path. +`--keep` still exits after the tests and leaves PostgreSQL running, but it does +**not** reserve that server. A queued command may immediately stop or reset it. +Coordinate with other worktrees before investigating interactively. `make run` +starts an unprotected interactive session; interactive shells and database +clients are not supported inside the wrapper. + +The supervisor forwards SIGHUP, SIGINT, and SIGTERM to the command's process +group and holds the lock until every live process in that group exits, including +cleanup after the command itself exits. Work that ignores cancellation keeps the +lock until it finishes; there is no automatic forced-kill timeout. Use these +signals rather than killing only the supervisor with SIGKILL, which bypasses +cleanup. + +PostgreSQL started by `pg_ctl` creates a separate session, so a kept server does +not retain the lock. Other work that deliberately leaves the supervised process +group is outside this protection: do not detach tasks that use the shared +installation. If process monitoring fails, the supervisor reports the error and +keeps the lock until monitoring recovers and confirms that the work has stopped. +The empty lock file stays on disk; do not delete it to unblock a waiter, since +that would let two commands lock different files at the same path. ## Test Suites Overview @@ -192,8 +205,9 @@ cover all families without any generator or new CI job. **Investigation mode (`--keep`):** The lock is released when the runner exits. When sharing PostgreSQL with other -worktrees, use the [locked debugging shell](#concurrent-local-runs) to prevent a -queued run from replacing the server during investigation. +worktrees, coordinate any interactive investigation before starting another run. +Non-interactive diagnostics can be included in the +[same locked command](#concurrent-local-runs) as the tests. ```bash # Run tests, keep server running diff --git a/scripts/with-pg-lock.py b/scripts/with-pg-lock.py index 0bdd0318..da4389c0 100644 --- a/scripts/with-pg-lock.py +++ b/scripts/with-pg-lock.py @@ -1,14 +1,18 @@ #!/usr/bin/env python3 # Copyright (c) Microsoft Corporation. # Licensed under the PostgreSQL License. -"""Run a command with exclusive access to the user's shared test PostgreSQL.""" +"""Run a non-interactive command with exclusive access to the shared test PostgreSQL.""" import argparse +from collections import deque import errno import fcntl import os from pathlib import Path +import signal +import subprocess import sys +import time def inherited_lock(lock_path): @@ -36,6 +40,127 @@ def exec_command(command): sys.exit(127 if error.errno == errno.ENOENT else 126) +def live_processes(): + try: + result = subprocess.run( + ["ps", "-axo", "pid=,pgid=,stat="], check=True, capture_output=True, text=True, + ) + except (OSError, subprocess.CalledProcessError) as error: + detail = str(error) + if isinstance(error, subprocess.CalledProcessError) and error.stderr.strip(): + detail += f": {error.stderr.strip()}" + raise RuntimeError(f"cannot inspect process groups with ps: {detail}") from error + + if not result.stdout.strip(): + raise RuntimeError("ps returned an empty process listing") + try: + # An orphaned zombie can keep a group alive without doing any more work. + processes = {} + for line in result.stdout.splitlines(): + pid, group, state = line.split() + pid = int(pid) + group = int(group) + if not state.startswith("Z"): + processes[pid] = group + return processes + except ValueError as error: + raise RuntimeError("ps returned an invalid process listing") from error + + +def supervise_command(command): + # Fail before starting work if process-group monitoring is unavailable. + live_processes() + pending = deque() + cancelled_by = None + command_state_changed = False + + def remember_signal(signum, _frame): + nonlocal cancelled_by + cancelled_by = signum + pending.append(signum) + + def child_changed(_signum, _frame): + nonlocal command_state_changed + command_state_changed = True + + previous_handlers = { + signum: signal.signal(signum, remember_signal) + for signum in (signal.SIGHUP, signal.SIGINT, signal.SIGTERM) + } + previous_handlers[signal.SIGCHLD] = signal.signal(signal.SIGCHLD, child_changed) + had_error = False + last_error = None + + def write_status(message): + nonlocal had_error + try: + print(f"pg_durable: {message}", file=sys.stderr, flush=True) + except OSError: + # Losing the diagnostic pipe must not release a live command's lock. + had_error = True + + def report_error(message): + nonlocal had_error, last_error + had_error = True + if message != last_error: + write_status(f"{message}; retaining the PostgreSQL test lock") + last_error = message + + try: + if cancelled_by is not None: + return 128 + cancelled_by + try: + # The inherited-lock path sets the re-exec marker to the child's PID. + child = subprocess.Popen( + [sys.executable, str(Path(__file__).resolve()), "--", *command], + start_new_session=True, + ) + except OSError as error: + raise RuntimeError(f"cannot start the supervised command: {error}") from error + + reported_wait = False + while True: + signal_error = None + while pending: + signum = pending.popleft() + write_status(f"forwarding {signal.Signals(signum).name} to process " + f"group {child.pid}; waiting for work to finish") + try: + os.killpg(child.pid, signum) + except ProcessLookupError: + pass + except OSError as error: + signal_error = f"cannot signal process group {child.pid}: {error}" + + if command_state_changed: + try: + processes = live_processes() + if child.pid not in processes.values(): + break + except RuntimeError as error: + report_error(str(error)) + else: + if child.pid not in processes and not reported_wait: + write_status(f"waiting for remaining work in process group {child.pid}") + reported_wait = True + if signal_error is not None: + report_error(signal_error) + time.sleep(0.1) + + # Keep the leader unreaped until its group stops, reserving the PGID + # while cancellation might still signal surviving descendants. + returncode = child.wait() + finally: + for signum, handler in previous_handlers.items(): + signal.signal(signum, handler) + + if cancelled_by is not None: + return 128 + cancelled_by + if had_error: + return 1 + return returncode if returncode >= 0 else 128 - returncode + + def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("command", nargs=argparse.REMAINDER) @@ -59,20 +184,20 @@ def main(): file=sys.stderr, flush=True) fcntl.lockf(lock, fcntl.LOCK_EX) - # POSIX record locks survive exec, but are not inherited by forked - # processes. Unlike flock, a postmaster left by --keep cannot retain this - # lock after the runner exits. Exec also preserves signals and exit codes. - os.set_inheritable(lock.fileno(), True) + # Only the supervisor owns the lock, not the command or a kept server. os.environ["_PG_DURABLE_LOCK_PATH"] = str(lock_path) os.environ["_PG_DURABLE_LOCK_OWNER"] = str(os.getpid()) - exec_command(command) + return supervise_command(command) if __name__ == "__main__": try: - main() + sys.exit(main()) except KeyboardInterrupt: sys.exit(130) except OSError as error: print(f"pg_durable: cannot acquire PostgreSQL test lock: {error}", file=sys.stderr) sys.exit(1) + except RuntimeError as error: + print(f"pg_durable: {error}", file=sys.stderr) + sys.exit(1) diff --git a/tests/test_pg_lock.py b/tests/test_pg_lock.py index e1aac9e2..283a3629 100644 --- a/tests/test_pg_lock.py +++ b/tests/test_pg_lock.py @@ -85,8 +85,13 @@ def setUp(self): self.bin.mkdir() self.env["PATH"] = str(self.bin) + os.pathsep + self.env["PATH"] self.write_program(self.bin / "cargo", """ -import json, os, sys +import json, os, signal, sys from pathlib import Path +if os.environ.get("CARGO_CANCEL_FILE"): + def cancelled(signum, _frame): + Path(os.environ["CARGO_CANCEL_FILE"]).write_text(str(signum)) + for signum in (signal.SIGHUP, signal.SIGINT, signal.SIGTERM): + signal.signal(signum, cancelled) if os.environ.get("CARGO_RECORD"): Path(os.environ["CARGO_RECORD"]).write_text(json.dumps({ "args": sys.argv[1:], "cwd": os.getcwd(), @@ -118,7 +123,15 @@ def start(self, command, *, cwd=None, env=None, wrapped=True): text=True, start_new_session=True, ) - self.addCleanup(cleanup_process, process) + def cleanup(): + if process.poll() is None: + process.terminate() + try: + process.communicate(timeout=10) + except subprocess.TimeoutExpired: + cleanup_process(process) + + self.addCleanup(cleanup) return process def read_line(self, stream): @@ -210,13 +223,25 @@ def test_surviving_server_does_not_retain_lock(self): holder = self.start([sys.executable, "-c", """ import subprocess, sys from pathlib import Path +# pg_ctl starts the postmaster in a separate session. child = subprocess.Popen( [sys.executable, "-c", "import time; time.sleep(60)"], - close_fds=False, stdin=subprocess.DEVNULL, + close_fds=False, start_new_session=True, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) Path(sys.argv[1]).write_text(str(child.pid)) """, str(pid_file)]) + + def stop_server(): + if pid_file.exists(): + server_pid = int(pid_file.read_text()) + self.assertGreater(server_pid, 0) + try: + os.kill(server_pid, signal.SIGKILL) + except ProcessLookupError: + pass + + self.addCleanup(stop_server) self.assert_finished(holder) server_pid = int(pid_file.read_text()) os.kill(server_pid, 0) @@ -237,15 +262,14 @@ def test_exit_cleanup_remains_locked(self): self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") def test_signals_release_the_owner_lock(self): - for sig in (signal.SIGTERM, signal.SIGINT, signal.SIGKILL): + for sig in (signal.SIGHUP, signal.SIGTERM, signal.SIGINT): with self.subTest(signal=sig): holder = self.start([sys.executable, "-c", HOLD]) self.assertEqual(self.read_line(holder.stdout), "entered") waiter = self.start([sys.executable, "-c", "print('next')"]) self.assert_waiting(waiter) holder.send_signal(sig) - holder.communicate(timeout=10) - self.assertNotEqual(holder.returncode, 0) + self.assert_finished(holder, 128 + sig) self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") def test_signalled_runner_keeps_lock_until_foreground_child_finishes(self): @@ -253,17 +277,116 @@ def test_signalled_runner_keeps_lock_until_foreground_child_finishes(self): for sig in (signal.SIGHUP, signal.SIGINT, signal.SIGTERM): with self.subTest(signal=sig): record = self.root / f"cargo-{sig}.json" + cancelled = self.root / f"cancelled-{sig}" holder = self.start( ["bash", "scripts/test-unit.sh"], cwd=worktree, wrapped=False, - env={"CARGO_RECORD": str(record), "HOLD_CARGO": "1"}, + env={"CARGO_RECORD": str(record), "HOLD_CARGO": "1", + "CARGO_CANCEL_FILE": str(cancelled)}, ) self.wait_for_file(record) holder.send_signal(sig) + self.wait_for_file(cancelled) waiter = self.start([sys.executable, "-c", "print('next')"]) self.assert_waiting(waiter) self.assert_finished(holder, 128 + sig, input="\n") + self.assertEqual(int(cancelled.read_text()), sig) + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_cancelled_wrapped_shell_waits_for_descendant_cleanup(self): + worktree = self.worktree("cancelled shell") + for close_stderr in (False, True): + with self.subTest(close_stderr=close_stderr): + record = self.root / f"cargo-{close_stderr}.json" + cancelled = self.root / f"cancelled-{close_stderr}" + holder = self.start( + ["bash", "-c", "bash scripts/test-unit.sh; :"], cwd=worktree, + env={"CARGO_RECORD": str(record), "HOLD_CARGO": "1", + "CARGO_CANCEL_FILE": str(cancelled)}, + ) + self.wait_for_file(record) + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + if close_stderr: + holder.stderr.close() + holder.stderr = None + holder.send_signal(signal.SIGTERM) + self.wait_for_file(cancelled) + self.assertIsNone(holder.poll()) + with self.assertRaises(subprocess.TimeoutExpired): + waiter.wait(timeout=0.2) + if close_stderr: + holder.communicate(input="\n", timeout=10) + self.assertNotEqual(holder.returncode, 0) + else: + self.assert_finished(holder, 143, input="\n") + self.assertEqual(int(cancelled.read_text()), signal.SIGTERM) self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + def test_terminated_command_does_not_release_surviving_descendants(self): + worktree = self.worktree("terminated shell") + record = self.root / "cargo.json" + shell_pid_file = self.root / "shell.pid" + holder = self.start( + ["bash", "-c", 'echo "$$" > "$SHELL_PID_FILE"; bash scripts/test-unit.sh; :'], + cwd=worktree, + env={"CARGO_RECORD": str(record), "HOLD_CARGO": "1", + "SHELL_PID_FILE": str(shell_pid_file)}, + ) + self.wait_for_file(record) + shell_pid = int(shell_pid_file.read_text()) + self.assertNotEqual(shell_pid, holder.pid) + os.kill(shell_pid, signal.SIGTERM) + self.assertIn("waiting for remaining work", self.read_line(holder.stderr)) + state = subprocess.check_output( + ["ps", "-p", str(shell_pid), "-o", "stat="], text=True, + ).strip() + self.assertTrue(state.startswith("Z"), state) + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.assert_waiting(waiter) + self.assert_finished(holder, 143, input="\n") + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + + def test_invalid_process_listing_fails_before_command(self): + for output in ("", "not-a-pid Z\n"): + with self.subTest(output=output): + self.write_program(self.bin / "ps", f"print({output!r}, end='')") + process = self.start([sys.executable, "-c", "print('unexpected')"]) + actual, error = self.assert_finished(process, 1) + self.assertEqual(actual, "") + self.assertIn("process listing", error) + + def test_monitor_failure_retains_lock_until_observation_recovers(self): + fail = self.root / "fail-monitor" + observed = self.root / "monitor-failed" + recovered = self.root / "monitor-recovered" + real_ps = shutil.which("ps") + self.assertIsNotNone(real_ps) + self.write_program(self.bin / "ps", f""" +import os, sys +from pathlib import Path +if Path({str(fail)!r}).exists() and not Path({str(recovered)!r}).exists(): + Path({str(observed)!r}).touch() + sys.exit("simulated ps failure") +os.execv({real_ps!r}, [{real_ps!r}, *sys.argv[1:]]) +""") + holder = self.start([sys.executable, "-c", HOLD + """ +from pathlib import Path +Path(sys.argv[1]).touch() +""", str(fail)]) + self.assertEqual(self.read_line(holder.stdout), "entered") + waiter = self.start([sys.executable, "-c", "print('next')"]) + self.addCleanup(recovered.touch) + self.assert_waiting(waiter) + holder.stdin.write("\n") + holder.stdin.flush() + self.wait_for_file(observed) + self.assertIsNone(holder.poll()) + with self.assertRaises(subprocess.TimeoutExpired): + waiter.wait(timeout=0.2) + recovered.touch() + self.assertIn("retaining the PostgreSQL test lock", self.assert_finished(holder, 1)[1]) + self.assertEqual(self.assert_finished(waiter)[0].strip(), "next") + def test_interrupted_waiter_never_executes_command(self): lock = self.hold_lock() marker = self.root / "unexpected" @@ -322,7 +445,7 @@ def test_all_local_entry_points_wait_before_touching_postgres(self): process.send_signal(signal.SIGTERM) process.communicate(timeout=10) self.assertEqual(process.returncode, -signal.SIGTERM) - for target in ("test-unit", "test-regress", "pg-install", "run"): + for target in ("test-unit", "test-regress", "pg-install"): with self.subTest(target=target): process = self.start(["make", "--no-print-directory", target], cwd=worktree, wrapped=False) @@ -331,6 +454,19 @@ def test_all_local_entry_points_wait_before_touching_postgres(self): process.communicate(timeout=10) self.assertNotEqual(process.returncode, 0) + def test_interactive_make_run_is_explicitly_unprotected(self): + worktree = self.worktree("interactive run") + record = self.root / "cargo.json" + self.hold_lock() + process = self.start( + ["make", "--no-print-directory", "run"], cwd=worktree, wrapped=False, + env={"CARGO_RECORD": str(record)}, + ) + _, error = self.assert_finished(process) + self.assertIn("not protected by the test lock", error) + self.assertNotIn("waiting for", error) + self.assertEqual(json.loads(record.read_text())["args"], ["pgrx", "run", "pg17"]) + def test_help_does_not_wait(self): worktree = self.worktree("help") self.hold_lock()