[DPE-9685] Release storage on teardown (#1827)

* fix(charm): release storage on unit teardown

The charm implemented no removal hooks, so on unit teardown the
charmed-postgresql snap services kept the Juju storage mounts busy.
Juju's unmount then failed with "target is busy", leaving storage
stuck detaching and blocking machine and model removal (only
destroy-model --force could clear it).

Stop the workload in the storage-detaching hook, which Juju runs
before stop, so the mounts are free by the time Juju unmounts them.
This stops every charmed-postgresql snap service plus the charm's
topology-observer and log-rotation processes, and is idempotent
across the per-storage detaching events.

Fixes #1550.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* test(integration): reproduce #1550 teardown on juju 4.0

The storage-detaching regression only manifests on Juju 4.0: 3.6 masks it
with a cleanup_storage shortcut that removes still-Dying storage, and on
4.0 only rootfs (machine-scoped) storage reproduces the stuck unmount, so
running the teardown check on 3.6 proves nothing. Move it out of
test_storage.py into its own spread task pinned to a juju40 variant with
rootfs storage (force-deployed because the charm still declares
assumes: juju < 4).

Also make the list_storage adapter tolerate Juju 4.0's empty list-storage
output, which it prints instead of "{}" for a model with no storage.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* fix(charm): only release storage on full app teardown

Stopping the workload in storage-detaching also fired on scale-down
(remove-unit), where the surviving leader still needs the departing
unit's Patroni reachable to remove it from the raft cluster. Stopping it
early broke that reconfiguration, leaving the cluster unable to elect a
primary ("Primary unit not found") and failing the HA/scaling
integration tests.

The storage unmount only actually hangs on full teardown
(remove-application/destroy-model), so guard the handler on
planned_units() == 0. On scale-down it now does nothing, restoring the
pre-fix cluster behaviour, while destroy-model/remove-application still
release the storage.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* docs(charm): trim storage-detaching comments and fix copyright year

Match the one-liner docstring density of the surrounding handlers instead
of an 8-line block (the why lives in the commit that added the guard),
and date the new test file 2026 (it was copied as 2025).

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* fix(charm): disable snap services on teardown, not just stop

stop() leaves the snap services enabled, so a mid-teardown restart of the unit could re-enable them and re-grab the storage mounts before Juju finishes unmounting. stop(disable=True) prevents that.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

* test(spread): run storage-detaching on juju36 as a no-regression guard

The task ran only on juju40 (where #1550 reproduces). Add juju36 so the teardown change (stopping/disabling the snap in storage-detaching) is also exercised on 3.6, where the bug is masked by force-removal but a new breakage would still surface.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

---------

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>

Author: marceloneppel

src/charm.py

@@ -409,6 +409,10 @@ def __init__(self, *args):
         # incorrectly
         # https://canonical-charm-refresh.readthedocs-hosted.com/latest/add-to-charm/status/#implementation
         self.framework.observe(self.on.collect_unit_status, self._reconcile_refresh_status)
+        for storage_name in self.meta.storages:
+            self.framework.observe(
+                self.on[storage_name].storage_detaching, self._on_storage_detaching
+            )
         self.cluster_name = self.app.name
         self._member_name = self.unit.name.replace("/", "-")
         self._certs_path = "/usr/local/share/ca-certificates"
@@ -2628,6 +2632,21 @@ 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.
+        if self.app.planned_units() > 0:
+            return
+        self._observer.stop_observer()
+        self._rotate_logs.stop_log_rotation()
+        try:
+            # Disable too, so a mid-teardown restart of the unit can't re-enable the
+            # services and re-grab the storage mounts before Juju finishes unmounting.
+            snap.SnapCache()[charm_refresh.snap_name()].stop(disable=True)
+        except snap.SnapError:
+            logger.exception("Failed to stop charmed-postgresql snap services")
+
     def _is_storage_attached(self) -> bool:
         """Returns if storage is attached."""
         try:

src/rotate_logs.py

@@ -5,6 +5,7 @@
 
 import logging
 import os
+import signal
 import subprocess
 from typing import TYPE_CHECKING
 
@@ -56,3 +57,14 @@ def start_log_rotation(self):
 
         self._charm.unit_peer_data.update({"rotate-logs-pid": f"{pid}"})
         logging.info(f"Started rotate logs process with PID {pid}")
+
+    def stop_log_rotation(self) -> None:
+        """Stop the running rotate logs process if we have previously started it."""
+        if stored_pid := self._charm.unit_peer_data.get("rotate-logs-pid"):
+            pid = int(stored_pid)
+            try:
+                os.kill(pid, signal.SIGINT)
+                logging.info(f"Stopped rotate logs process with PID {pid}")
+                self._charm.unit_peer_data.update({"rotate-logs-pid": ""})
+            except OSError:
+                pass

tests/integration/adapters.py

@@ -379,7 +379,8 @@ def destroy_unit(
     def list_storage(self, filesystem: bool = False, volume: bool = False) -> list[TStorageInfo]:
         """Lists storage details."""
         raw = self._juju.cli("list-storage", "--format", "json")
-        json_ = json.loads(raw)
+        # Juju 4.0 prints nothing (not `{}`) for a model with no storage.
+        json_ = json.loads(raw) if raw.strip() else {}
         ret = []
         for storage_key, storage_details in json_.get("storage", {}).items():
             ret.append({"key": storage_key, **storage_details})

tests/integration/test_storage_detaching.py

@@ -0,0 +1,47 @@
+#!/usr/bin/env python3
+# Copyright 2026 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import logging
+
+import pytest
+
+from .adapters import JujuFixture
+from .jubilant_helpers import DATABASE_APP_NAME
+
+logger = logging.getLogger(__name__)
+
+# rootfs (machine-scoped) is the only pool that reproduces the stuck unmount on
+# Juju 4.0: the detachable lxd pool destroys the container before detaching.
+ROOTFS_STORAGE = dict.fromkeys(("archive", "data", "logs", "temp"), "rootfs")
+
+
+@pytest.mark.abort_on_fail
+def test_storage_released_on_removal(juju: JujuFixture, charm):
+    """Graceful 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.
+    """
+    juju.ext.model.deploy(
+        charm,
+        num_units=1,
+        config={"profile": "testing"},
+        storage=ROOTFS_STORAGE,
+        force=True,
+    )
+    juju.ext.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active")
+
+    logger.info("Removing %s and waiting for a clean teardown", DATABASE_APP_NAME)
+    juju.ext.model.remove_application(
+        DATABASE_APP_NAME,
+        block_until_done=True,
+        destroy_storage=True,
+        timeout=15 * 60,
+    )
+
+    detaching = [
+        storage for storage in juju.ext.model.list_storage() if storage["life"] == "detaching"
+    ]
+    assert not detaching, f"storage stuck detaching after removal: {detaching}"

tests/spread/test_storage_detaching.py/task.yaml

@@ -0,0 +1,11 @@
+summary: test_storage_detaching.py
+environment:
+  TEST_MODULE: test_storage_detaching.py
+  # #1550 only reproduces on Juju 4.0 (3.6 masks it by removing still-Dying storage),
+  # so juju40 is the real regression test. Run juju36 too as a no-regression guard:
+  # stopping/disabling the snap in storage-detaching must not break 3.6 teardown.
+  CONCIERGE_JUJU_CHANNEL/juju40: 4.0/stable
+execute: |
+  tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results"
+artifacts:
+  - allure-results

tests/unit/test_charm.py

@@ -125,6 +125,42 @@ def test_on_install(harness):
         assert isinstance(harness.model.unit.status, WaitingStatus)
 
 
+def test_on_storage_detaching(harness):
+    with (
+        patch("charm.snap.SnapCache") as _snap_cache,
+        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,
+    ):
+        _selected_snap = _snap_cache.return_value.__getitem__.return_value
+
+        # 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.
+        _planned_units.return_value = 2
+        storage_id = harness.add_storage("data", 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()
+
+        # 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()
+
+            storage_id = harness.add_storage(storage_name, attach=True)[0]
+            harness.detach_storage(storage_id)
+
+            _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):
     result = harness.charm.patroni_scrape_config()
 

tests/unit/test_rotate_logs.py

@@ -1,5 +1,6 @@
 # Copyright 2024 Canonical Ltd.
 # See LICENSE file for licensing details.
+import signal
 from unittest.mock import Mock, PropertyMock, patch
 
 import pytest
@@ -96,3 +97,33 @@ def test_start_log_rotation_already_running(harness):
         harness.charm.rotate_logs.start_log_rotation()
         _kill.assert_called_once_with(1234, 0)
         _popen.assert_called_once()
+
+
+def test_stop_log_rotation(harness):
+    with (
+        patch("os.kill") as _kill,
+        patch.object(MockCharm, "_peers", new_callable=PropertyMock) as _peers,
+    ):
+        # Nothing is done when the peer relation (and so the stored PID) is gone.
+        _peers.return_value = None
+        harness.charm.rotate_logs.stop_log_rotation()
+        _kill.assert_not_called()
+
+        # Nothing is done when there is no process running.
+        _peers.return_value = Mock(data={harness.charm.unit: {}})
+        harness.charm.rotate_logs.stop_log_rotation()
+        _kill.assert_not_called()
+
+        # The process is killed and the stored PID is cleared.
+        peer_data = {"rotate-logs-pid": "1"}
+        _peers.return_value = Mock(data={harness.charm.unit: peer_data})
+        harness.charm.rotate_logs.stop_log_rotation()
+        _kill.assert_called_once_with(1, signal.SIGINT)
+        assert peer_data["rotate-logs-pid"] == ""
+        _kill.reset_mock()
+
+        # A dead process doesn't break the script.
+        _peers.return_value = Mock(data={harness.charm.unit: {"rotate-logs-pid": "1"}})
+        _kill.side_effect = OSError
+        harness.charm.rotate_logs.stop_log_rotation()
+        _kill.assert_called_once_with(1, signal.SIGINT)