Freeze HA scoring and document reported production failover

docs/review.md

@@ -0,0 +1,115 @@
+# Freeze review — 2026-09-12
+
+This review preserves the implementation reported as production-tested by the
+operator. No production systems were changed. It is a source/documentation
+freeze, not certification of every failure mode or a byte-level deployment
+attestation. Production hash collection remains outstanding in [testing.md](testing.md).
+
+## Corrections and their reasons
+
+- Added operations.md to distinguish container lifecycle, PostgreSQL lifecycle,
+  Pacemaker state, scores, fencing, controlled switchover and former-primary rejoin.
+- Added testing.md to record the operator's SBD failover/VIP/rewind/rejoin sequence
+  and final three-node roles/scores, separately from local automated checks.
+  Recorded exact repository RA/module/helper/bootstrap hashes without inventing
+  deployed hashes. Included read-only per-node hash collection commands.
+- Required target-role=Promoted, promoted-max=1, Promoted-role VIP colocation and
+  promotion-before-VIP-start ordering in the operating policy. Compatibility
+  tests for absent/Started target roles are not recommendations to change production.
+- Fixed plan-promotion-cleanup.py to emit only temporary node-preference removal
+  and preserve target-role=Promoted. Removed its obsolete inherited-Started
+  check because it no longer clears the direct target role. Updated its exact
+  expected-output test; ambiguity and unrelated-rule protections remain. Added
+  an explicit Promoted-target scheduler case and used that target in fencing
+  scenarios; compatibility cases remain.
+- Explicitly prohibited clearing former-primary failed actions before successful
+  rejoin and verification of streaming recovery. Documented rewind before any
+  explicitly authorized basebackup fallback, and that failed rewind may require
+  further recovery rather than a normal restart.
+- Documented literal env-file quotes and the mismatch between generated pgpass
+  and the RA's direct container environment password. All examples use obvious
+  fake values; no actual password from the handoff was copied into the repository.
+- Updated README and historical handoffs/notes to point to current operations
+  and evidence. Corrected the PID 1 simplification: bootstrap executes the
+  maintenance command, possibly behind Podman's init shim.
+- Included current operating/testing/review documents in source/RPM documentation.
+  Retained the old ELITEDESK installer as a clearly marked historical lab tool:
+  its hostname/cluster/hash guards reject the scoring implementation. It is not
+  a production installer and was not executed.
+- Ignored generated review patches; the commit records the actual files instead
+  of duplicating obsolete proposed commands in patch attachments. Refreshed the
+  checksum manifest. Existing local patch files remain available locally.
+
+The staged scoring, state helper, tests and RPM workflow from the preceding
+implementation work are included in this freeze commit. This review did not
+modify RA/module/scoring-helper or bootstrap code. The frozen original remains
+unchanged, and all 29 original lifecycle bodies still compare byte-for-byte.
+
+## Checks completed in this review
+
+- Structural/default/metadata verification and all 22 differential lifecycle cases.
+- 25 promotion-scoring tests, 16 disconnected-state/monitor tests, 11 synthetic
+  Pacemaker scheduler tests and 4 corrected preference-planner tests: all pass.
+- Nine SQL scenarios against a disposable PostgreSQL 18 container: all pass for
+  the original strict predicate and current observation query. Container removed.
+- Bash syntax and Python AST parsing across repository code and historical installer:
+  pass. ShellCheck and Gitleaks were not installed; no claim of running them is made.
+- Repository/diff review for private keys, recognizable tokens, credential-bearing
+  URLs, and credential assignments, plus manual inspection of matches: no actual
+  secrets found. Matches were variable references, redacted snapshots, or explicit
+  fake examples. No CIB, secret file, .pgpass content or raw environment dump was added.
+- A disposable Podman test reproduced literal env-file quotes using only a fake
+  value. No production credentials or environments were inspected.
+- Fedora 43 development RPM build with the new docs passed; artifacts are in
+  dist/documentation-freeze/ and the log is /tmp/mc-freeze-build.log. The extra
+  Promoted-target scheduler case was subsequently verified separately.
+- Checksum manifest and git diff whitespace checks are verified immediately before
+  commit.
+
+## Privilege and safety boundaries reviewed
+
+The RA does not source host-side PostgreSQL environment/configuration. Its
+root-to-service-user boundary preserves argument arrays, and container commands
+consume intentional injected settings. Authentication secrets are not placed in
+RA command arguments or host-side pgpass files. The stored bootstrap is separate
+and has a different provisioning interface.
+
+The scoring helper uses bounded subprocesses, a root-only runtime directory,
+strictly parsed JSON, per-resource locks and atomic replacement. Its identity
+includes postmaster start time, so a restarted/replaced data instance must stream
+successfully before receiving disconnected eligibility. Notification state uses
+a separate root-owned /run directory and relies on Pacemaker serializing actions
+for the same resource; it is not a distributed locking or fencing mechanism.
+
+Original SQL/shell construction in some lifecycle helpers assumes trusted,
+conventional resource configuration (identifiers, paths, connection fields).
+It is not a general escaping layer for arbitrary user-supplied text. Restrict CIB
+write access and container ownership; do not expose RA parameters to untrusted
+input. No new untrusted input interface was introduced by this review.
+
+## Remaining concerns and explicit limits
+
+- The three production nodes' exact deployed hashes and confirmation that those
+  bytes were used in the fencing test are missing. Source hashes are not a
+  substitute. The runtime-context bootstrap may differ from deployed production.
+- The saved bootstrap's fresh-primary provisioning path passes a replication
+  password through a psql --set argument. That can expose it to process-argument
+  inspection under local /proc permissions. This is a concrete existing exposure,
+  not a claim that the RA rejoin path leaks a password. It must be reviewed against
+  the actual deployed bootstrap before calling provisioning secret-safe; no
+  bootstrap rewrite or production secret change was made in this freeze.
+- Existing bootstrap pgpass generation assumes values needing no additional
+  colon/backslash escaping. Do not treat the documented quote correction as a
+  universal password-encoding solution. Inspect the deployed bootstrap before
+  changing provisioning behavior.
+- Scores are observations, not fencing authorization or atomic promotion guards.
+  IPC failure can leave a prior score; monitor cadence/fencing latency can exceed
+  freshness. Two disconnected candidates are not ranked by cross-node WAL history.
+- Asynchronous replication can lose acknowledged transactions. Neither the test
+  nor the freshness threshold proves a numerical data-loss bound.
+- The reported fenced-failure/rejoin success does not prove every concurrent-write
+  controlled-switchover scenario or optional destructive basebackup fallback.
+- License provenance remains unresolved as documented in packaging/PROVENANCE.md.
+  This source freeze is not a license grant or public-release authorization.
+
+No push or production rollout is part of this review.