feat(postgres): add Restic remote backups and restore validation - #563

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

tests/unit/test_postgres_restore_validation.py (6)

455-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the redundant negative assertion.

Lines 452-454 already assert owner == "root", group == "postgres", and mode == "0710". The tuple comparison against ("root", "0700") cannot fail unless one of those three assertions already failed. It adds noise without adding coverage.

🤖 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/unit/test_postgres_restore_validation.py` around lines 455 - 458,
Remove the redundant tuple inequality assertion following the existing owner,
group, and mode assertions in the work_root validation test, leaving those
direct assertions unchanged.

---

465-481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert tag propagation through apply.tags for the include_role.

The test checks name, when, service fields, and timer fields. It does not check apply.tags. The include at ansible/roles/postgres/tasks/sub_tasks/backup_restore_validation_setup.yml lines 151-158 must carry postgres_backup_restore_validation_setup and postgres_backup_restore_validation_run into the systemd_jobs role. If that propagation is dropped, a tagged run silently skips timer and unit creation, and no test fails.

💚 Proposed test addition

     assert include["ansible.builtin.include_role"]["name"] == "systemd_jobs"
     assert "inventory_hostname == postgres_backup_restore_validation_host" in include["when"]
+    applied_tags = include["ansible.builtin.include_role"]["apply"]["tags"]
+    assert "postgres_backup_restore_validation_setup" in applied_tags
+    assert "postgres_backup_restore_validation_run" in applied_tags
+    assert set(applied_tags) == set(include["tags"])

As per coding guidelines: "For dynamic Ansible includes, place selection tags on the outer task, propagate required tags through apply.tags, and add regression coverage when missing propagation could skip safety, validation, or lifecycle tasks."

🤖 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/unit/test_postgres_restore_validation.py` around lines 465 - 481,
Update test_systemd_job_exists_only_on_designated_host_and_uses_network_online
to inspect include["ansible.builtin.include_role"]["apply"]["tags"] and assert
it contains postgres_backup_restore_validation_setup and
postgres_backup_restore_validation_run, covering tag propagation to the
systemd_jobs role.

Source: Coding guidelines

---

374-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a timeout to the validator subprocess calls.

subprocess.run has no timeout. The rendered script waits on flock, pg_ctl -w -t 60, and the fake restic. If any step hangs, the test blocks until the CI job limit and produces no useful diagnostic. A bounded timeout converts the hang into a TimeoutExpired failure at the exact test.

♻️ Proposed refactor

     return subprocess.run(
         [str(script)],
         check=False,
         capture_output=True,
         text=True,
         env=environment,
+        timeout=120,
     )

🤖 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/unit/test_postgres_restore_validation.py` around lines 374 - 380,
Update the validator subprocess invocation using subprocess.run to include a
bounded timeout that covers the expected flock, pg_ctl, and fake restic
execution while preventing indefinite hangs. Keep the existing output capture,
environment, and return-code behavior unchanged so timeout failures surface at
the specific test.

---

639-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for duplicate stream tags.

The parametrised mutators cover unsafe IDs, missing and invalid backup-id, unexpected paths, and malformed tags. They do not cover the duplicate-tag rejection at lines 449-450 of ansible/roles/postgres/templates/postgres-logical-backup-restore-validate.sh.j2, which raises "duplicate stream tags". That branch is currently unexercised.

💚 Proposed test addition

         (lambda item, root: item.update({"tags": "malformed"}), "malformed tags"),
+        (
+            lambda item, root: item.update(
+                {
+                    "tags": [
+                        "postgres-logical-backup",
+                        "postgres-logical-backup",
+                        "cluster=pg-cluster",
+                        f"backup-id={BACKUP_ID_A}",
+                    ]
+                }
+            ),
+            "duplicate stream tags",
+        ),

🤖 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/unit/test_postgres_restore_validation.py` around lines 639 - 664, Add a
parametrized mutator to the existing validation cases that produces duplicate
stream tags and expects the “duplicate stream tags” message. Use the same
item/root setup as the other mutators and target the duplicate-tag rejection in
the restore validation test coverage.

---

351-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for a non-empty postgres_backup_remote_options list.

Every test renders the template with postgres_backup_remote_options=[]. The RESTIC_OPTIONS loop at lines 31-35 of ansible/roles/postgres/templates/postgres-logical-backup-restore-validate.sh.j2 is therefore never rendered with content, and the single-quote escaping in that loop is never proven. Render one case with an option that contains a single quote and assert that the rendered script still passes bash -n and that the option reaches the fake restic command log.

As per path instructions: "Tests must prove behavior rather than merely duplicate implementation strings; static tests alone are insufficient for normalization, rendering, merge behavior, adapter portability, and runtime contracts."

🤖 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/unit/test_postgres_restore_validation.py` around lines 351 - 352,
Extend the restore-validation test coverage to render a non-empty
postgres_backup_remote_options value containing a single quote. Assert the
generated script passes bash -n and that the escaped option is received by the
fake restic command log, exercising the RESTIC_OPTIONS rendering path rather
than only the empty-list case.

Source: Path instructions

---

939-947: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the formatting-coupled assertions with behavioral checks.

Line 947 counts the exact string "process_belongs_to_validator_pgdata \\", including the trailing backslash continuation. A reformat of the template that preserves behavior breaks this test. Lines 940-941 have the same problem for the dirname guard and the run-directory regex.

The surrounding tests at lines 963-1022 already prove the intended behavior: an exact match stops the cluster once, and every mismatch fails closed without a stop. Prefer those. If the double identity check must be enforced, assert on the identifier alone rather than on its line continuation.

As per path instructions: "Tests must prove behavior rather than merely duplicate implementation strings; static tests alone are insufficient for normalization, rendering, merge behavior, adapter portability, and runtime contracts."

🤖 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/unit/test_postgres_restore_validation.py` around lines 939 - 947,
Replace the formatting-sensitive source-string assertions in the restore
validation test with behavioral coverage using the existing tests around exact
matches and mismatches, removing checks for the dirname guard and run-directory
regex text. If retaining the identity-check assertion, verify only the
identifier’s presence or behavior, not its line-continuation formatting, while
preserving the fail-closed and single-stop guarantees.

Source: Path instructions

ansible/roles/postgres/templates/postgres-logical-backup-restore-validate.sh.j2 (2)

815-818: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the restored relation count instead of discarding it.

The query result goes to /dev/null. The check proves only that the temporary cluster answers SQL after pg_restore. A dump that restores zero relations passes the validation and reports success. Capture the count and reject zero.

♻️ Proposed refactor

-  psql_value \
-    "$VALIDATION_DATABASE" \
-    'SELECT 1; SELECT count(*) FROM pg_catalog.pg_class WHERE relkind IN ('"'"'r'"'"', '"'"'p'"'"', '"'"'v'"'"', '"'"'m'"'"');' \
-    > /dev/null
+  restored_relation_count="$(
+    psql_value \
+      "$VALIDATION_DATABASE" \
+      'SELECT count(*) FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relkind IN ('"'"'r'"'"', '"'"'p'"'"', '"'"'v'"'"', '"'"'m'"'"') AND n.nspname NOT IN ('"'"'pg_catalog'"'"', '"'"'information_schema'"'"');'
+  )"
+  [[ "$restored_relation_count" =~ ^[0-9]+$ ]] ||
+    fail "Restored database $source_database returned an unreadable relation count."

Do not fail on a zero count unconditionally. Some clusters hold legitimately empty databases. Consider logging the count and adding an opt-in minimum.

🤖 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
`@ansible/roles/postgres/templates/postgres-logical-backup-restore-validate.sh.j2`
around lines 815 - 818, Update the validation flow around the pg_class count
query to capture and log the restored relation count instead of redirecting it
to /dev/null. Add an opt-in minimum-count check that rejects counts below the
configured threshold, while allowing legitimately empty databases when no
minimum is configured.

---

561-568: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Consider bounding the dump scan against symlinked directories.

artifact.rglob("*.dump") walks the restored tree. In Python versions before 3.13, rglob follows directory symlinks, so a restored artifact that contains a symlink loop makes the scan run without bound. The artifact comes from the Restic repository, so this needs a corrupted or hostile repository to trigger. The scan runs as root before any other structural rejection.

An os.walk(..., followlinks=False) based scan removes the risk.

🤖 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
`@ansible/roles/postgres/templates/postgres-logical-backup-restore-validate.sh.j2`
around lines 561 - 568, Replace the artifact.rglob("*.dump") traversal with an
os.walk-based scan that uses followlinks=False, while preserving relative path
generation, regular-file validation, and exact manifest comparison in the
actual_dumps flow. Ensure symlinked directories are not traversed.

ansible/roles/postgres/tasks/sub_tasks/backup_restore_validation_setup.yml (1)

136-145: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider validating the rendered script before installation.

The template writes an executable that systemd runs unattended. Add validate: bash -n %s so a broken render fails at deploy time instead of at the next timer run.

♻️ Proposed refactor

     dest: "{{ postgres_backup_restore_validation_script_path }}"
     owner: root
     group: root
     mode: "0755"
+    validate: bash -n %s

🤖 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 `@ansible/roles/postgres/tasks/sub_tasks/backup_restore_validation_setup.yml`
around lines 136 - 145, Update the “Backup restore validation | Install
host-local runner” template task to validate the rendered script with Bash
syntax checking before installation by adding the appropriate validate setting
using the temporary-file placeholder. Keep the existing template source,
destination, ownership, permissions, and conditions unchanged.

tests/unit/test_postgres_remote_backup.py (1)

518-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prove reserved-name rejection by behavior, not by substring counts.

The assertion counts implementation strings. A prefix match such as - ENV also matches a longer name, so the test can pass without the guard existing. The parametrized test at lines 535-559 covers only three of the ten names.

Extend the parametrized rejection test to every reserved name and keep the static check only as a secondary signal.

As per coding guidelines: "Tests must prove behavior rather than merely duplicate implementation strings; static tests alone are insufficient for normalization, rendering, merge behavior, adapter portability, and runtime contracts."

🤖 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/unit/test_postgres_remote_backup.py` around lines 518 - 532, Extend the
parametrized reserved-name rejection test near the existing three-name coverage
to include all ten names listed in
test_reserved_backend_environment_names_cover_shell_startup_and_loader_controls.
Keep that behavioral test as the primary verification for every name, and retain
the source-count assertion only as a secondary static check, tightening its
matching if needed to avoid prefix collisions.

Source: Coding guidelines

ansible/roles/postgres/tasks/sub_tasks/backup_remote_action.yml (1)

21-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

changed_when always reports changed for the maintenance action.

Line 35 marks every maintenance run as changed. A maintenance run that forgets no snapshot is a no-op. Idempotent reporting needs a distinct runner signal, for example POSTGRES_BACKUP_REMOTE_RESULT=MAINTENANCE_COMPLETE=<removed-count>, and a matching pattern here.

As per coding guidelines: "Use ... idempotent tasks, and accurate changed_when and failed_when behavior."

🤖 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 `@ansible/roles/postgres/tasks/sub_tasks/backup_remote_action.yml` around lines
21 - 35, Update changed_when for the maintenance branch so it no longer
unconditionally reports changes; match a runner result signal such as
POSTGRES_BACKUP_REMOTE_RESULT=MAINTENANCE_COMPLETE=<removed-count>, requiring a
positive removed count. Preserve the existing init and upload conditions and
align the maintenance pattern with the runner’s emitted output.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes