fix: preserve active swap files when swapoff fails

crates/sysknife-brain/src/planning_tools/propose_plan.rs

@@ -194,7 +194,7 @@ this is runtime status, NOT the saved configuration; on Ubuntu the saved config
     ("AddSwap",
      "create a swap file, enable it, and persist to /etc/fstab — params: file* (absolute path), size_mb* (integer MB); High risk"),
     ("RemoveSwap",
-     "disable a swap file, remove it, and drop its /etc/fstab entry — param: file* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk"),
+     "disable and remove a swap file after swapoff succeeds, then drop its /etc/fstab entry; if swapoff fails while it remains active or active state cannot be read, refuse and preserve both — param: file* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk"),
     // apt pinning (preferences.d) — Ubuntu/Debian only
     ("GetAptPins",
      "show apt pin priorities (apt-cache policy) — param: package (optional); read-only"),

docs/action-reference.md

@@ -127,7 +127,7 @@ Every row is derived from the live code: the command from each action's `ActionS
 | `AddMount` | `sudo /usr/lib/sysknife/mount-edit --op mount --device /dev/sdb1 --mountpoint /mnt/data --fstype ext4 --options defaults` | High | All | – | – | mount a device and persist it to /etc/fstab with nofail — params: device\* (/dev/.., UUID=.., LABEL=.., //host/share, or host:/export), mountpoint\* (absolute; not a system dir), fstype\* (ext4/xfs/btrfs/vfat/nfs/cifs/…), options (csv, optional); High risk |
 | `RemoveMount` | `sudo /usr/lib/sysknife/mount-edit --op unmount --mountpoint /mnt/data` | High | All | – | – | unmount and drop the /etc/fstab entry for a mountpoint — param: mountpoint\*; High risk |
 | `AddSwap` | `sudo /usr/lib/sysknife/mount-edit --op addswap --file /swapfile --size-mb 2048` | High | All | – | – | create a swap file, enable it, and persist to /etc/fstab — params: file\* (absolute path), size_mb\* (integer MB); High risk |
-| `RemoveSwap` | `sudo /usr/lib/sysknife/mount-edit --op rmswap --file /swapfile` | High | All | – | – | disable a swap file, remove it, and drop its /etc/fstab entry — param: file\* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk |
+| `RemoveSwap` | `sudo /usr/lib/sysknife/mount-edit --op rmswap --file /swapfile` | High | All | – | – | disable and remove a swap file after swapoff succeeds, then drop its /etc/fstab entry; if swapoff fails while it remains active or active state cannot be read, refuse and preserve both — param: file\* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk |
 
 ## Log management
 

packaging/sysknife-mount-edit

@@ -400,6 +400,23 @@ def op_addswap(args) -> None:
         write_fstab(append_fstab_entry(lines, entry))
 
 
+def active_swap_files():
+    """Absolute paths currently active as swap FILES, excluding partitions."""
+    active = set()
+    try:
+        with open(PROC_SWAPS, "r") as fh:
+            next(fh, None)  # header: Filename Type Size Used Priority
+            for line in fh:
+                fields = line.split()
+                if len(fields) >= 2 and fields[1] == "file":
+                    active.add(fields[0])
+    except OSError:
+        # A missing or unreadable proc file cannot prove that a swap path is
+        # inactive. known_swap_files still uses fstab to identify candidates.
+        return None
+    return active
+
+
 def known_swap_files() -> set:
     """Absolute paths this host actually knows as swap FILES.
 
@@ -408,17 +425,8 @@ def known_swap_files() -> set:
     is configured to have). Swap *partitions* are excluded from both — they are
     block devices, never something to unlink — as are commented-out fstab lines.
     """
-    known = set()
-    try:
-        with open(PROC_SWAPS, "r") as fh:
-            next(fh, None)  # header: Filename Type Size Used Priority
-            for line in fh:
-                fields = line.split()
-                if len(fields) >= 2 and fields[1] == "file":
-                    known.add(fields[0])
-    except OSError:
-        # No /proc/swaps (a container, a test harness): fstab alone decides.
-        pass
+    active = active_swap_files()
+    known = active if active is not None else set()
     for line in read_fstab():
         if line.lstrip().startswith("#"):
             continue
@@ -446,7 +454,18 @@ def op_rmswap(args) -> None:
     # have accepted the link as swap in the first place.
     if os.path.islink(path):
         die(f"{path!r} is a symlink, not a swap file; refusing to remove it")
-    subprocess.run([SWAPOFF, path], check=False, stderr=subprocess.DEVNULL)
+    swapoff = subprocess.run([SWAPOFF, path], check=False, stderr=subprocess.PIPE)
+    if swapoff.returncode != 0:
+        active = active_swap_files()
+        if active is None or path in active:
+            detail = swapoff.stderr.decode(errors="replace").strip()
+            message = (
+                f"swapoff failed for active or unverifiable swap file {path!r}; "
+                "leaving the file and fstab entry intact"
+            )
+            if detail:
+                message += f": {detail}"
+            die(message)
     lines = read_fstab()
     kept = remove_fstab_lines(lines, fstab_field1, path)
     if len(kept) != len(lines):

tests/release/rmswap.test.sh

@@ -15,7 +15,9 @@ SCRIPT="$ROOT/packaging/sysknife-mount-edit"
 [ -f "$SCRIPT" ] || { echo "FAIL: $SCRIPT not found"; exit 1; }
 
 python3 - "$SCRIPT" <<'PY'
+import contextlib
 import importlib.util
+import io
 import os
 import sys
 import tempfile
@@ -43,20 +45,31 @@ class Args:
 
 
 class FakeCompleted:
-    returncode = 0
-    stderr = b""
+    def __init__(self, returncode=0, stderr=b""):
+        self.returncode = returncode
+        self.stderr = stderr
 
 
-def run_rmswap(path):
-    """Run op_rmswap with swapoff stubbed out. Returns the SystemExit code, or
-    None if it ran to completion."""
+def run_rmswap(path, swapoff_returncode=0, swapoff_stderr=b""):
+    """Run op_rmswap with swapoff stubbed out; return its exit code and stderr."""
     real_run = mod.subprocess.run
-    mod.subprocess.run = lambda *a, **k: FakeCompleted()
+    result = FakeCompleted(swapoff_returncode, swapoff_stderr)
+
+    def fake_run(*args, **kwargs):
+        if kwargs.get("stderr") == mod.subprocess.PIPE:
+            return result
+        return FakeCompleted(swapoff_returncode, None)
+
+    mod.subprocess.run = fake_run
+    captured_stderr = io.StringIO()
     try:
-        mod.op_rmswap(Args(path))
-        return None
-    except SystemExit as exc:
-        return exc.code
+        with contextlib.redirect_stderr(captured_stderr):
+            try:
+                mod.op_rmswap(Args(path))
+                code = None
+            except SystemExit as exc:
+                code = exc.code
+        return code, captured_stderr.getvalue()
     finally:
         mod.subprocess.run = real_run
 
@@ -67,21 +80,24 @@ with tempfile.TemporaryDirectory() as d:
     victim = os.path.join(d, "shadow")
     declared = os.path.join(d, "declared-swap")
     active = os.path.join(d, "active-swap")
+    active_only = os.path.join(d, "active-only-swap")
 
     with open(victim, "w") as fh:
         fh.write("root:!:19000:0:99999:7:::\n")
-    for p in (declared, active):
+    for p in (declared, active, active_only):
         with open(p, "w") as fh:
             fh.write("swapdata")
 
     with open(fstab, "w") as fh:
         fh.write("UUID=1234\t/\text4\tdefaults\t0\t1\n")
         fh.write(f"{declared}\tnone\tswap\tsw,nofail\t0\t0\n")
+        fh.write(f"{active}\tnone\tswap\tsw,nofail\t0\t0\n")
         # A commented-out entry must not make a path "known".
         fh.write(f"#{victim}\tnone\tswap\tsw,nofail\t0\t0\n")
     with open(swaps, "w") as fh:
         fh.write("Filename\t\t\t\tType\t\tSize\t\tUsed\t\tPriority\n")
         fh.write(f"{active}\t\t\t\tfile\t\t2097148\t\t0\t\t-2\n")
+        fh.write(f"{active_only}\t\t\t\tfile\t\t2097148\t\t0\t\t-1\n")
         fh.write("/dev/sda2\t\t\t\tpartition\t2097148\t\t0\t\t-3\n")
 
     mod.FSTAB = fstab
@@ -94,13 +110,17 @@ with tempfile.TemporaryDirectory() as d:
         failures.append("known_swap_files missed the fstab-declared swap file")
     if active not in known:
         failures.append("known_swap_files missed the active /proc/swaps file")
+    if active_only not in known:
+        failures.append("known_swap_files missed the /proc/swaps-only file")
+    if any(mod.fstab_field1(line) == active_only for line in mod.read_fstab()):
+        failures.append("/proc/swaps-only test file unexpectedly appeared in fstab")
     if victim in known:
         failures.append("known_swap_files treated a commented-out fstab line as a swap file")
     if "/dev/sda2" in known:
         failures.append("known_swap_files treated a swap partition as a removable swap file")
 
     # 2. THE BUG: an unrelated root-owned file must not be deleted.
-    code = run_rmswap(victim)
+    code, _ = run_rmswap(victim)
     if code in (None, 0):
         failures.append(f"op_rmswap did NOT refuse an unknown path (exit {code!r})")
     if not os.path.exists(victim):
@@ -113,24 +133,73 @@ with tempfile.TemporaryDirectory() as d:
     os.symlink(victim, link)
     with open(fstab, "a") as fh:
         fh.write(f"{link}\tnone\tswap\tsw,nofail\t0\t0\n")
-    code = run_rmswap(link)
+    code, _ = run_rmswap(link)
     if code in (None, 0):
         failures.append(f"op_rmswap did NOT refuse a symlinked swap path (exit {code!r})")
 
-    # 4. The action must still work: a genuine, declared swap file is removed.
-    code = run_rmswap(declared)
+    # 4. A stale fstab entry is not active swap. A failed swapoff is expected
+    #    here, so the file and its declaration can still be removed.
+    code, _ = run_rmswap(
+        declared,
+        swapoff_returncode=1,
+        swapoff_stderr=b"not an active swap area",
+    )
     if code not in (None, 0):
         failures.append(f"op_rmswap refused a legitimate declared swap file (exit {code!r})")
     if os.path.exists(declared):
         failures.append("op_rmswap did not remove a legitimate declared swap file")
+    if any(mod.fstab_field1(line) == declared for line in mod.read_fstab()):
+        failures.append("op_rmswap kept the fstab entry for an inactive swap file")
+
+    # 5. A failed swapoff for an active file must preserve both the file and its
+    #    fstab entry and surface the utility's stderr.
+    code, message = run_rmswap(
+        active,
+        swapoff_returncode=1,
+        swapoff_stderr=b"device is busy",
+    )
+    if code in (None, 0):
+        failures.append(f"op_rmswap did NOT refuse a failed swapoff for an active file (exit {code!r})")
+    if not os.path.exists(active):
+        failures.append("op_rmswap DELETED an active swap file after swapoff failed")
+    if not any(mod.fstab_field1(line) == active for line in mod.read_fstab()):
+        failures.append("op_rmswap removed the fstab entry after swapoff failed")
+    if "device is busy" not in message:
+        failures.append("op_rmswap did not include swapoff stderr in its refusal")
+
+    # 6. If /proc/swaps cannot be read, a failed swapoff is not evidence that
+    #    the active path is safe to remove. Preserve the file and fstab entry.
+    mod.PROC_SWAPS = os.path.join(d, "missing-swaps")
+    code, message = run_rmswap(
+        active,
+        swapoff_returncode=1,
+        swapoff_stderr=b"cannot inspect active swaps",
+    )
+    if code in (None, 0):
+        failures.append(f"op_rmswap treated unreadable /proc/swaps as inactive (exit {code!r})")
+    if not os.path.exists(active):
+        failures.append("op_rmswap deleted a swap file when it could not inspect /proc/swaps")
+    if not any(mod.fstab_field1(line) == active for line in mod.read_fstab()):
+        failures.append("op_rmswap removed the fstab entry when /proc/swaps was unreadable")
+    if "cannot inspect active swaps" not in message:
+        failures.append("op_rmswap did not include swapoff stderr when /proc/swaps was unreadable")
 
-    # 5. ...and so is one that is only known because it is currently active.
-    code = run_rmswap(active)
+    mod.PROC_SWAPS = swaps
+
+    # 7. A successful swapoff still removes a genuinely active file.
+    code, _ = run_rmswap(active)
     if code not in (None, 0):
         failures.append(f"op_rmswap refused a legitimate active swap file (exit {code!r})")
     if os.path.exists(active):
         failures.append("op_rmswap did not remove a legitimate active swap file")
 
+    # 8. A file known only from /proc/swaps (no fstab line) is still removable.
+    code, _ = run_rmswap(active_only)
+    if code not in (None, 0):
+        failures.append(f"op_rmswap refused a swap file known only from /proc/swaps (exit {code!r})")
+    if os.path.exists(active_only):
+        failures.append("op_rmswap did not remove a swap file known only from /proc/swaps")
+
 if failures:
     for f in failures:
         print("FAIL:", f)