fix: recover stale PostgreSQL cache locks

Author: renecannao

scripts/pg_compat/common.py

@@ -18,6 +18,7 @@
 COMMIT_PATTERN = re.compile(r"^[0-9a-f]{40}$")
 PG_VERSION_PATTERN = re.compile(r"^[0-9]+(?:\.[0-9]+)+$")
 SHA256_PATTERN = re.compile(r"^[0-9a-f]{64}$")
+ASCII_CONTROL_PATTERN = re.compile(r"[\x00-\x1f\x7f]")
 
 
 def _require_nonempty_string(value, name):
@@ -33,6 +34,8 @@ def _validate_version_pin(version, role):
 
     _require_nonempty_string(version["branch"], f"{prefix}.branch")
     _require_nonempty_string(version["pg_version"], f"{prefix}.pg_version")
+    if ASCII_CONTROL_PATTERN.search(version["branch"]):
+        raise ValueError(f"{prefix}.branch must not contain ASCII control characters")
 
     if not isinstance(version["commit"], str) or not COMMIT_PATTERN.fullmatch(
         version["commit"]

scripts/pg_compat/fetch_libpg_query.sh

@@ -6,8 +6,13 @@ PINS="${PG_COMPAT_PINS:-${ROOT}/tests/pg_compat/upstream_pins.json}"
 CACHE="${PG_COMPAT_CACHE:-/tmp/parsersql-pg-compat}"
 WITH_POSTGRES_SOURCE=0
 LOCK_DIR="${CACHE}/.pg_compat.lock"
-LOCK_OWNER="$$-${RANDOM}-${RANDOM}"
+LOCK_OWNER_PATTERN=$'^([1-9][0-9]*)\t([A-Za-z0-9][A-Za-z0-9._-]*)\n$'
+LOCK_OWNER_RECORD=
+LOCK_OWNER_TMP=
+LOCK_RECLAIM_DIR=
+LOCK_RECLAIM_FILE=
 LOCK_HELD=0
+FILE_CONTENT=
 
 usage() {
     echo "Usage: $0 [--with-postgres-source]" >&2
@@ -27,25 +32,138 @@ fi
 
 cd "$ROOT"
 
-release_lock() {
-    if [[ "$LOCK_HELD" -ne 1 || ! -d "$LOCK_DIR" ]]; then
+read_file_exact() {
+    local path="$1"
+
+    if [[ ! -f "$path" ]]; then
+        return 1
+    fi
+    FILE_CONTENT="$(cat "$path"; printf '\034')" || return 1
+    FILE_CONTENT="${FILE_CONTENT%$'\034'}"
+}
+
+restore_reclaim_owner() {
+    if [[ -z "$LOCK_RECLAIM_FILE" || ! -f "$LOCK_RECLAIM_FILE" ]]; then
+        if [[ -n "$LOCK_RECLAIM_DIR" ]]; then
+            rmdir "$LOCK_RECLAIM_DIR" 2>/dev/null || true
+            LOCK_RECLAIM_DIR=
+            LOCK_RECLAIM_FILE=
+        fi
         return
     fi
-    if [[ -f "$LOCK_DIR/owner" ]] && [[ "$(cat "$LOCK_DIR/owner")" == "$LOCK_OWNER" ]]; then
-        rm -f "$LOCK_DIR/owner"
-        rmdir "$LOCK_DIR" 2>/dev/null || true
+    if [[ -d "$LOCK_DIR" && ! -e "$LOCK_DIR/owner" ]]; then
+        if ln "$LOCK_RECLAIM_FILE" "$LOCK_DIR/owner" 2>/dev/null; then
+            rm -f "$LOCK_RECLAIM_FILE"
+            rmdir "$LOCK_RECLAIM_DIR" 2>/dev/null || true
+            LOCK_RECLAIM_DIR=
+            LOCK_RECLAIM_FILE=
+        fi
+        return
+    fi
+    rm -f "$LOCK_RECLAIM_FILE"
+    rmdir "$LOCK_RECLAIM_DIR" 2>/dev/null || true
+    LOCK_RECLAIM_DIR=
+    LOCK_RECLAIM_FILE=
+}
+
+release_lock() {
+    restore_reclaim_owner
+    if [[ "$LOCK_HELD" -eq 1 && -d "$LOCK_DIR" ]]; then
+        if [[ ! -e "$LOCK_DIR/owner" ]]; then
+            rmdir "$LOCK_DIR" 2>/dev/null || true
+        elif read_file_exact "$LOCK_DIR/owner" &&
+            [[ "$FILE_CONTENT" == "$LOCK_OWNER_RECORD" ]]; then
+            rm -f "$LOCK_DIR/owner"
+            rmdir "$LOCK_DIR" 2>/dev/null || true
+        fi
+    fi
+    if [[ -n "$LOCK_OWNER_TMP" ]]; then
+        rm -f "$LOCK_OWNER_TMP"
+        LOCK_OWNER_TMP=
     fi
     LOCK_HELD=0
 }
 
+reclaim_stale_lock() {
+    local initial_owner
+    local owner_pid
+    local reclaim_dir
+
+    if ! read_file_exact "$LOCK_DIR/owner"; then
+        return 1
+    fi
+    initial_owner="$FILE_CONTENT"
+    if [[ ! "$initial_owner" =~ $LOCK_OWNER_PATTERN ]]; then
+        return 1
+    fi
+    owner_pid="${BASH_REMATCH[1]}"
+    if kill -0 "$owner_pid" 2>/dev/null; then
+        return 1
+    fi
+    if ! read_file_exact "$LOCK_DIR/owner" ||
+        [[ "$FILE_CONTENT" != "$initial_owner" ]]; then
+        return 1
+    fi
+
+    if ! reclaim_dir="$(mktemp -d "${CACHE}/.pg_compat.reclaim.XXXXXX")"; then
+        return 1
+    fi
+    LOCK_RECLAIM_DIR="$reclaim_dir"
+    LOCK_RECLAIM_FILE="${LOCK_RECLAIM_DIR}/owner"
+    if ! mv "$LOCK_DIR/owner" "$LOCK_RECLAIM_FILE" 2>/dev/null; then
+        rmdir "$LOCK_RECLAIM_DIR" 2>/dev/null || true
+        LOCK_RECLAIM_DIR=
+        LOCK_RECLAIM_FILE=
+        return 1
+    fi
+    if ! read_file_exact "$LOCK_RECLAIM_FILE" ||
+        [[ "$FILE_CONTENT" != "$initial_owner" ]]; then
+        restore_reclaim_owner
+        return 1
+    fi
+    if ! rmdir "$LOCK_DIR" 2>/dev/null; then
+        restore_reclaim_owner
+        return 1
+    fi
+
+    rm -f "$LOCK_RECLAIM_FILE"
+    rmdir "$LOCK_RECLAIM_DIR" 2>/dev/null || true
+    LOCK_RECLAIM_DIR=
+    LOCK_RECLAIM_FILE=
+}
+
+publish_lock() {
+    if ! mv "$LOCK_OWNER_TMP" "$LOCK_DIR/owner"; then
+        echo "Unable to publish PostgreSQL compatibility cache lock owner" >&2
+        exit 1
+    fi
+    LOCK_OWNER_TMP=
+}
+
 acquire_lock() {
+    local lock_token
+
     mkdir -p "$CACHE"
-    if ! mkdir "$LOCK_DIR" 2>/dev/null; then
-        echo "PostgreSQL compatibility cache is locked: ${CACHE}" >&2
-        exit 1
+    LOCK_OWNER_TMP="$(mktemp "${CACHE}/.pg_compat.owner.XXXXXX")"
+    lock_token="${LOCK_OWNER_TMP##*.pg_compat.owner.}"
+    LOCK_OWNER_RECORD="$$"$'\t'"${lock_token}"$'\n'
+    printf '%s' "$LOCK_OWNER_RECORD" > "$LOCK_OWNER_TMP"
+
+    if mkdir "$LOCK_DIR" 2>/dev/null; then
+        LOCK_HELD=1
+        publish_lock
+        return
+    fi
+    if reclaim_stale_lock && mkdir "$LOCK_DIR" 2>/dev/null; then
+        LOCK_HELD=1
+        publish_lock
+        return
     fi
-    printf '%s\n' "$LOCK_OWNER" > "$LOCK_DIR/owner"
-    LOCK_HELD=1
+
+    rm -f "$LOCK_OWNER_TMP"
+    LOCK_OWNER_TMP=
+    echo "PostgreSQL compatibility cache is locked: ${CACHE}" >&2
+    exit 1
 }
 
 trap release_lock EXIT

tests/pg_compat/test_common.py

@@ -128,6 +128,24 @@ def test_rejects_malformed_pin_values(self):
                     ):
                         load_pins(path)
 
+    def test_rejects_ascii_control_characters_in_branch(self):
+        with tempfile.TemporaryDirectory() as directory:
+            for codepoint in (*range(0x20), 0x7F):
+                with self.subTest(codepoint=codepoint):
+                    pins = valid_pins()
+                    pins["versions"]["previous"]["branch"] = (
+                        f"17-{chr(codepoint)}latest"
+                    )
+                    path = self.write_pins(
+                        Path(directory) / str(codepoint), pins
+                    )
+
+                    with self.assertRaisesRegex(
+                        ValueError,
+                        r"versions\.previous\.branch.*ASCII control",
+                    ):
+                        load_pins(path)
+
 
 class ParseMakefileVersionTest(unittest.TestCase):
     def test_parses_pg_version_and_numeric_version(self):
@@ -310,6 +328,13 @@ def run_fetch(self, environment, *arguments, timeout=10):
             timeout=timeout,
         )
 
+    def create_lock(self, cache, owner=None):
+        lock_dir = cache / ".pg_compat.lock"
+        lock_dir.mkdir(parents=True)
+        if owner is not None:
+            (lock_dir / "owner").write_text(owner, encoding="utf-8")
+        return lock_dir
+
     def test_bad_download_is_not_published_and_poisoned_cache_recovers(self):
         good_content = b"good archive"
         expected_hash = hashlib.sha256(good_content).hexdigest()
@@ -392,6 +417,67 @@ def test_concurrent_fetch_is_rejected_without_mutating_cache(self):
             )
             self.assertFalse((cache / ".pg_compat.lock").exists())
 
+    def test_valid_dead_pid_lock_is_recovered(self):
+        with tempfile.TemporaryDirectory() as directory:
+            _, cache, _, environment = self.create_harness(
+                directory, valid_pins()
+            )
+            dead_process = subprocess.Popen(["true"])
+            dead_process.wait(timeout=5)
+            self.create_lock(cache, f"{dead_process.pid}\tstale-token\n")
+
+            result = self.run_fetch(environment)
+
+            self.assertEqual(result.returncode, 0, result.stderr)
+            self.assertFalse((cache / ".pg_compat.lock").exists())
+
+    def test_live_pid_lock_is_rejected_without_reclamation(self):
+        with tempfile.TemporaryDirectory() as directory:
+            root, cache, _, environment = self.create_harness(
+                directory, valid_pins()
+            )
+            owner = f"{os.getpid()}\tlive-token\n"
+            lock_dir = self.create_lock(cache, owner)
+            git_called = root / "git-called"
+            environment["STUB_GIT_CALLED"] = str(git_called)
+
+            result = self.run_fetch(environment)
+
+            self.assertNotEqual(result.returncode, 0)
+            self.assertIn("cache is locked", result.stderr)
+            self.assertEqual(
+                (lock_dir / "owner").read_text(encoding="utf-8"), owner
+            )
+            self.assertFalse(git_called.exists())
+
+    def test_malformed_or_missing_owner_lock_is_rejected_without_reclamation(self):
+        cases = (None, "not-a-valid-owner\n")
+        with tempfile.TemporaryDirectory() as directory:
+            for index, owner in enumerate(cases):
+                with self.subTest(owner=owner):
+                    harness_root = Path(directory) / str(index)
+                    harness_root.mkdir()
+                    root, cache, _, environment = self.create_harness(
+                        harness_root, valid_pins()
+                    )
+                    lock_dir = self.create_lock(cache, owner)
+                    git_called = root / "git-called"
+                    environment["STUB_GIT_CALLED"] = str(git_called)
+
+                    result = self.run_fetch(environment)
+
+                    self.assertNotEqual(result.returncode, 0)
+                    self.assertIn("cache is locked", result.stderr)
+                    self.assertTrue(lock_dir.is_dir())
+                    if owner is None:
+                        self.assertFalse((lock_dir / "owner").exists())
+                    else:
+                        self.assertEqual(
+                            (lock_dir / "owner").read_text(encoding="utf-8"),
+                            owner,
+                        )
+                    self.assertFalse(git_called.exists())
+
     def test_terminated_fetch_releases_lock_without_continuing(self):
         with tempfile.TemporaryDirectory() as directory:
             root, cache, _, environment = self.create_harness(