[dannyward630]

Summary

• Preserve an active swap file and its /etc/fstab entry when swapoff fails and the path remains listed in /proc/swaps.
• Include swapoff stderr in the refusal; retain stale inactive fstab cleanup behavior.
• Document the failure behavior and cover both active and inactive cases, including a swap file known only from /proc/swaps.

Related issue

Closes #666

Validation

• Tests cover active, inactive, unreadable, and /proc/swaps-only cases.
• Documentation updated.
• Security impact considered: unknown paths remain refused before any root unlink.
• Trust boundary preserved: the daemon remains the only privileged executor.
• TMPDIR=/private/tmp bash tests/release/rmswap.test.sh — passed.
• bash -n tests/release/rmswap.test.sh and git diff --check — passed.
• All commits carry the required DCO sign-off.
• All required GitHub Actions checks pass at 62ea7ffe; container-smoke is skipped as configured.

Assisted by Codex (GPT-6).

Notes for Reviewers

If swapoff fails for a path that is no longer active, the helper still allows cleanup of its stale file and fstab entry. A failed test stub now returns stderr only when the helper captures it, so dropping the pipe makes the regression fail.

[vladimirrott]

Reviewed at 8076dc0c.

Thanks, Danny. The third commit is the part I'd have asked for if you hadn't got there first: active_swap_files() returns None when /proc/swaps can't be read, so the helper refuses when it can't tell, and the stale-entry cleanup runs after a confirmed "not active" and nowhere else. With swapoff's stderr in the refusal, the operator now sees the ENOMEM line that #666 was about.

I ran tests/release/rmswap.test.sh and the Rust suite in a no-network container at 8076dc0c, then on a trial merge with the security fixes from #703, which landed on main this evening. Your branch merges with no conflicts, and both trees pass, including #703's new newline-anchoring test for the helpers. Then I broke op_rmswap a few ways to see the test go red:

change to the helper	result
delete the returncode != 0 block	7 assertions red
treat an unreadable /proc/swaps as "not active"	4 red (case 6)
refuse every swapoff failure, stale entries too	3 red (case 4)
drop swapoff's stderr from the message	2 red

One thing before I merge. Line 87 adds active-swap to the fstab fixture, which cases 5 and 6 need. As a side effect, you no longer have a test for the /proc/swaps half of known_swap_files(): with line 425 changed to known = set(), the test stays green, and on main the same change turns three assertions red. A third path listed in the swaps fixture and absent from fstab fixes it. I tried this and the mutant goes red again:

active_only = os.path.join(d, "active-only-swap")  # in /proc/swaps, not in fstab
# ...add it to the files you create and to the swaps fixture,
# point case 1's "missed the active /proc/swaps file" check at it, then:

# 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")

Optional, your call: the stub at line 57 hands back stderr whatever subprocess.run asked for, so switching stderr=subprocess.PIPE to DEVNULL in the helper still passes. In production that change would crash on .decode() of None. If the stub returns stderr for stderr=subprocess.PIPE and None otherwise, the test pins it.

I'll write the CHANGELOG line at merge.

[vladimirrott]

Heads-up on a change that landed after you opened this: main now requires a DCO sign-off on every commit (#713, merged in #732). Each commit needs a Signed-off-by: Your Name <you@example.com> trailer whose address matches the commit's author, and the new dco check blocks this branch until its commits carry one. From your branch:

git fetch https://github.com/lacs-project/sysknife.git main
git rebase --signoff FETCH_HEAD
git push --force-with-lease

That also brings the branch up to date with main, and git commit -s adds the trailer to new commits. Sorry for changing the rules mid-review; nothing else about this PR changes.

[vladimirrott]

Reviewed at 62ea7ffe. Approved.

Thanks, Danny. Case 8 is the test I described, and you went one step further: the new check that active-only-swap never appears in the fstab fixture means nobody can later add it there and hand case 8 back to the fstab path. The stub change is right too. It now returns stderr only when the helper asks for subprocess.PIPE.

I ran tests/release/rmswap.test.sh in a no-network container at 62ea7ffe and put back the two mutants from my last review:

change to the helper	result
none	ok, exit 0
known = set() in known_swap_files()	3 failures, starting FAIL: known_swap_files missed the /proc/swaps-only file
swapoff with stderr=subprocess.DEVNULL	AttributeError: 'NoneType' object has no attribute 'decode', exit 1

With the file restored, the test passed again. The other three commits differ from what I reviewed at 8076dc0c only by their sign-off trailers (git range-diff), and shellcheck is clean on the test.

The branch sits one commit behind main, the 0.29.0 release, which only bumps versions and merges with no conflict. GitHub wants it current before merging, so please rebase once more (git rebase --signoff onto main, as before). I'll merge when the checks on that push are green, provided the rebase changes nothing outside the merge and the rmswap mutants above still fail. If anything else shows up in the diff, I'll review it again first. I'll write the CHANGELOG line at merge.