[DPE-9685] Release storage on scale-down

src/charm.py

@@ -2633,11 +2633,14 @@ def _install_snap_package(
             raise
 
     def _on_storage_detaching(self, _) -> None:
-        """Stop the workload so Juju can unmount the storage on app teardown."""
-        # On scale-down the surviving cluster still needs this unit's Patroni to
-        # remove it from raft; only stop when the whole app is going away.
+        """Stop the workload so Juju can unmount the storage on teardown or scale-down."""
+        # storage-detaching runs before the relation-departed/stop hooks and nothing
+        # else stops the snap, so stop it here or the mount stays busy. (3.6 force-removes
+        # still-Dying storage and masks this; 4.0 leaves it for Juju to unmount.) On
+        # scale-down drop this unit from raft via a surviving peer first — a node can't
+        # remove itself, and once stopped the leader can no longer resolve its member IP.
         if self.app.planned_units() > 0:
-            return
+            self._remove_from_raft_via_peer()
         self._observer.stop_observer()
         self._rotate_logs.stop_log_rotation()
         try:
@@ -2647,6 +2650,32 @@ def _on_storage_detaching(self, _) -> None:
         except snap.SnapError:
             logger.exception("Failed to stop charmed-postgresql snap services")
 
+    def _remove_from_raft_via_peer(self) -> None:
+        """Drop this departing unit from raft using a surviving peer's endpoint.
+
+        pysyncobj rejects a node removing itself from its own endpoint, so the
+        removal is issued against a peer. When several units are removed together the
+        first peer tried may itself be departing (snap already stopped), so try each
+        peer until one reachable raft node accepts the removal — any live node routes
+        it to the leader, and on scale-down at least one survivor stays up.
+        """
+        if not self._peers:
+            return
+        for unit in self._peers.units:
+            peer_ip = self._get_unit_ip(unit)
+            if not peer_ip:
+                continue
+            try:
+                self._patroni.remove_raft_member(
+                    f"{self.state.unit_ip}:{RAFT_PORT}",
+                    remote_address=f"{peer_ip}:{RAFT_PORT}",
+                    set_raft_flags=False,
+                )
+                return
+            except RemoveRaftMemberFailedError:
+                continue
+        logger.warning("Failed to remove departing unit from raft: no reachable peer")
+
     def _is_storage_attached(self) -> bool:
         """Returns if storage is attached."""
         try:

tests/integration/test_storage_detaching.py

@@ -3,7 +3,9 @@
 # See LICENSE file for licensing details.
 
 import logging
+import time
 
+import jubilant
 import pytest
 
 from .adapters import JujuFixture
@@ -17,22 +19,81 @@
 
 
 @pytest.mark.abort_on_fail
-def test_storage_released_on_removal(juju: JujuFixture, charm):
-    """Graceful removal must release the storage so teardown completes.
+def test_storage_released_on_scale_down(juju: JujuFixture, charm):
+    """Scale-down (remove-unit) must release the departing units' storage.
 
-    Regression test for canonical/postgresql-operator#1550: without stopping the
-    workload in the ``storage-detaching`` hook the snap keeps the storage mounts
-    busy, so Juju's unmount fails ("target is busy") and teardown hangs.
+    Regression test for canonical/postgresql-operator#1550 on the scale-down
+    path. Removing two units at once also exercises dropping each departing unit
+    from raft via a surviving peer before it stops (the first peer tried may
+    itself be departing); a minority removal keeps quorum, so the survivors must
+    settle back to active.
     """
     juju.ext.model.deploy(
         charm,
-        num_units=1,
+        num_units=4,
         config={"profile": "testing"},
         storage=ROOTFS_STORAGE,
         force=True,
     )
     juju.ext.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active")
 
+    doomed = {unit.name for unit in juju.ext.model.applications[DATABASE_APP_NAME].units[-2:]}
+    logger.info("Removing %s at once; the two survivors must keep quorum", sorted(doomed))
+    for unit in sorted(doomed):
+        juju.ext.model.destroy_unit(unit, destroy_storage=True)
+
+    # Concurrent removal on Juju 4.0 can transiently error a departing unit on a canceled
+    # hook ("context canceled" as the uniter tears it down) and stall its removal. Resolve
+    # those on the departing units — benign, they are going away — and keep waiting; a
+    # survivor erroring is a real failure. The #1550 symptom is storage stuck "detaching".
+    resolved: set[str] = set()
+    deadline = time.monotonic() + 15 * 60
+    while time.monotonic() < deadline:
+        units = juju.status().apps[DATABASE_APP_NAME].units or {}
+        for name, unit in units.items():
+            if not unit.is_error:
+                continue
+            if name in doomed:
+                if name not in resolved:
+                    logger.info(
+                        "resolving transient error on departing %s: %s",
+                        name,
+                        unit.workload_status.message,
+                    )
+                    try:
+                        juju.cli("resolve", name)
+                    except jubilant.CLIError:
+                        # Benign race: the unit's state changed between the status check
+                        # and resolve (already removed, or no longer in error). The poll
+                        # keeps waiting; a genuinely stuck removal fails on the timeout.
+                        logger.debug("resolve no-op for %s (state changed)", name)
+                    resolved.add(name)
+            else:
+                raise AssertionError(
+                    f"survivor {name} errored during scale-down: {unit.workload_status.message}"
+                )
+        if len(units) <= 2 and all(u.is_active for u in units.values()):
+            break
+        time.sleep(10)
+    units = juju.status().apps[DATABASE_APP_NAME].units or {}
+    assert len(units) == 2, f"expected 2 survivors after scale-down, got {sorted(units)}"
+    assert all(u.is_active for u in units.values()), (
+        f"survivors not active: {[(n, u.workload_status.current) for n, u in units.items()]}"
+    )
+
+    detaching = [s for s in juju.ext.model.list_storage() if s["life"] == "detaching"]
+    assert not detaching, f"storage stuck detaching after scale-down: {detaching}"
+
+
+@pytest.mark.abort_on_fail
+def test_storage_released_on_removal(juju: JujuFixture):
+    """Full removal must release the storage so teardown completes.
+
+    Regression test for canonical/postgresql-operator#1550: without stopping the
+    workload in the ``storage-detaching`` hook the snap keeps the storage mounts
+    busy, so Juju's unmount fails ("target is busy") and teardown hangs. Reuses
+    the two-unit cluster the scale-down test above leaves behind.
+    """
     logger.info("Removing %s and waiting for a clean teardown", DATABASE_APP_NAME)
     juju.ext.model.remove_application(
         DATABASE_APP_NAME,

tests/unit/test_charm.py

@@ -131,34 +131,79 @@ def test_on_storage_detaching(harness):
         patch("charm.ClusterTopologyObserver.stop_observer") as _stop_observer,
         patch("charm.RotateLogs.stop_log_rotation") as _stop_log_rotation,
         patch.object(harness.charm.app, "planned_units") as _planned_units,
+        patch("charm.Patroni.remove_raft_member") as _remove_raft_member,
+        patch(
+            "charm.PostgresqlOperatorCharm._get_unit_ip", return_value="2.2.2.2"
+        ) as _get_unit_ip,
+        patch(
+            "single_kernel_postgresql.core.state.CharmState.unit_ip",
+            new_callable=PropertyMock,
+            return_value="1.1.1.1",
+        ),
     ):
         _selected_snap = _snap_cache.return_value.__getitem__.return_value
+        rel_id = harness.model.get_relation(PEER_RELATION).id
+
+        def _reset():
+            for _m in (_stop_observer, _stop_log_rotation, _selected_snap, _remove_raft_member):
+                _m.reset_mock()
 
-        # Scale-down (units remain): the surviving cluster still needs this unit's
-        # Patroni reachable to remove it from raft, so the workload must NOT be
-        # stopped here.
+        # Scale-down but no surviving peer to ask: skip the raft removal, yet still
+        # stop so Juju can unmount the departing unit's storage.
         _planned_units.return_value = 2
-        storage_id = harness.add_storage("data", attach=True)[0]
+        storage_id = harness.add_storage("archive", attach=True)[0]
         harness.detach_storage(storage_id)
-        _stop_observer.assert_not_called()
-        _stop_log_rotation.assert_not_called()
-        _selected_snap.stop.assert_not_called()
+        _remove_raft_member.assert_not_called()
+        _selected_snap.stop.assert_called_once_with(disable=True)
+        _reset()
 
-        # Full teardown (no units remain): release the storage by stopping the
-        # background processes and disabling (so a mid-teardown restart can't
-        # re-grab the mount) all the snap services.
-        _planned_units.return_value = 0
-        for storage_name in ("archive", "data", "logs", "temp"):
-            _stop_observer.reset_mock()
-            _stop_log_rotation.reset_mock()
-            _selected_snap.reset_mock()
+        # Scale-down with a surviving peer: a node cannot remove itself from its own
+        # raft endpoint, so drop this unit from raft via the peer, then stop.
+        harness.add_relation_unit(rel_id, f"{harness.charm.app.name}/1")
+        storage_id = harness.add_storage("data", attach=True)[0]
+        harness.detach_storage(storage_id)
+        _remove_raft_member.assert_called_once_with(
+            "1.1.1.1:2222", remote_address="2.2.2.2:2222", set_raft_flags=False
+        )
+        _stop_observer.assert_called_once_with()
+        _stop_log_rotation.assert_called_once_with()
+        _selected_snap.stop.assert_called_once_with(disable=True)
+        _reset()
+
+        # Concurrent scale-down where the first peer tried is itself a departing unit
+        # whose raft endpoint is already down (removal fails against it): fall through to
+        # a still-live survivor and drop this member via that peer's endpoint instead of
+        # leaking it. Distinct peer IPs prove the retry lands on the live survivor.
+        harness.add_relation_unit(rel_id, f"{harness.charm.app.name}/2")
+        _get_unit_ip.side_effect = ["2.2.2.2", "3.3.3.3"]
+        _remove_raft_member.side_effect = [RemoveRaftMemberFailedError, None]
+        storage_id = harness.add_storage("logs", attach=True)[0]
+        harness.detach_storage(storage_id)
+        assert _remove_raft_member.call_count == 2
+        _remove_raft_member.assert_called_with(
+            "1.1.1.1:2222", remote_address="3.3.3.3:2222", set_raft_flags=False
+        )
+        _selected_snap.stop.assert_called_once_with(disable=True)
+        _get_unit_ip.side_effect = None
+        _reset()
 
-            storage_id = harness.add_storage(storage_name, attach=True)[0]
-            harness.detach_storage(storage_id)
+        # Scale-down where every peer's raft endpoint is unreachable: log and still
+        # stop rather than blocking the storage release.
+        _remove_raft_member.side_effect = RemoveRaftMemberFailedError
+        storage_id = harness.add_storage("temp", attach=True)[0]
+        harness.detach_storage(storage_id)
+        assert _remove_raft_member.call_count == 2
+        _selected_snap.stop.assert_called_once_with(disable=True)
+        _reset()
 
-            _stop_observer.assert_called_once_with()
-            _stop_log_rotation.assert_called_once_with()
-            _selected_snap.stop.assert_called_once_with(disable=True)
+        # Full teardown (no units remain): stop, no raft removal needed.
+        _planned_units.return_value = 0
+        storage_id = harness.add_storage("data", attach=True)[0]
+        harness.detach_storage(storage_id)
+        _remove_raft_member.assert_not_called()
+        _stop_observer.assert_called_once_with()
+        _stop_log_rotation.assert_called_once_with()
+        _selected_snap.stop.assert_called_once_with(disable=True)
 
 
 def test_patroni_scrape_config(harness):