Skip to content

Commit 2051ec7

Browse files
author
linux_name2
committed
fix: cleanup edge cases and deduplicate subvol lookup
- fstab.py: clean up temp file on write/fsync OSError in _atomic_write - atomic-gc: cache current_subvol once in rm command (match garbage_collect) - atomic-rebuild-uki: only rmdir if umount succeeded in cleanup_rebuild - tests: add fsync failure cleanup test and rebuild-uki cleanup behavioral test
1 parent 1f06834 commit 2051ec7

5 files changed

Lines changed: 97 additions & 4 deletions

File tree

‎bin/atomic-gc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ case "$COMMAND" in
8181
acquire_lock
8282
[[ ${#ARGS[@]} -eq 0 ]] && { echo "ERROR: Specify generation(s) to delete" >&2; exit 1; }
8383
ensure_btrfs_mounted || exit 1
84+
current_subvol=$(get_current_subvol)
8485
# Validate all targets exist before deleting any
8586
for gen_id in "${ARGS[@]}"; do
8687
if [[ ! -f "${ESP}/EFI/Linux/arch-${gen_id}.efi" ]] && \
@@ -98,7 +99,7 @@ case "$COMMAND" in
9899
warn_orphan_homes "${ARGS[@]}"
99100
fi
100101
for gen_id in "${ARGS[@]}"; do
101-
delete_generation "$gen_id" "$DRY_RUN"
102+
delete_generation "$gen_id" "$DRY_RUN" "$current_subvol"
102103
done
103104
;;
104105
gc)

‎bin/atomic-rebuild-uki‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,9 @@ acquire_lock
107107
cleanup_rebuild() {
108108
set +e
109109
[[ -n "${MOUNT_DIR:-}" ]] && {
110-
mountpoint -q "$MOUNT_DIR" 2>/dev/null && umount "$MOUNT_DIR"
111-
rmdir "$MOUNT_DIR" 2>/dev/null
110+
if mountpoint -q "$MOUNT_DIR" 2>/dev/null && umount "$MOUNT_DIR"; then
111+
rmdir "$MOUNT_DIR" 2>/dev/null || true
112+
fi
112113
}
113114
mountpoint -q "$BTRFS_MOUNT" 2>/dev/null && umount "$BTRFS_MOUNT"
114115
[[ -n "${LOCK_FD:-}" ]] && exec {LOCK_FD}>&-

‎lib/atomic/fstab.py‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,13 @@ def _atomic_write(path: Path, entries: list) -> None:
171171
try:
172172
os.write(fd, content)
173173
os.fsync(fd)
174-
finally:
174+
except OSError:
175175
os.close(fd)
176+
try:
177+
tmp.unlink(missing_ok=True)
178+
except OSError:
179+
pass
180+
raise
176181

177182
try:
178183
os.chown(tmp, original_stat.st_uid, original_stat.st_gid)

‎tests/test_fstab.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -474,3 +474,25 @@ def fail_chmod_on_tmp(file_path, mode, *args, **kwargs):
474474
# Verify that the temp file was cleaned up despite the error
475475
leftovers = list(tmp_path.glob(".fstab.*.tmp"))
476476
assert leftovers == [], f"Temp files left behind: {leftovers}"
477+
478+
def test_atomic_write_fsync_failure_cleanup(self, tmp_path, monkeypatch):
479+
"""Simulate fsync failure to verify temp file cleanup and OSError propagation."""
480+
path = tmp_path / "fstab"
481+
path.write_text("UUID=x / btrfs rw,subvol=old 0 0\n")
482+
483+
original_fsync = os.fsync
484+
485+
def fail_fsync(fd):
486+
raise OSError("Simulated fsync failure")
487+
488+
monkeypatch.setattr(os, "fsync", fail_fsync)
489+
490+
with pytest.raises(OSError, match="Simulated fsync failure"):
491+
update_fstab(str(path), "old", "new")
492+
493+
# Verify that the temp file was cleaned up despite the error
494+
leftovers = list(tmp_path.glob(".fstab.*.tmp"))
495+
assert leftovers == [], f"Temp files left behind: {leftovers}"
496+
497+
# Verify original fstab is unchanged
498+
assert path.read_text() == "UUID=x / btrfs rw,subvol=old 0 0\n"

‎tests/test_rebuild_uki.sh‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,70 @@ grep -q 'rmdir.*MOUNT_DIR' "$SCRIPT" && ok "cleanup removes MOUNT_DIR" || fail "
134134
grep -q 'umount.*BTRFS_MOUNT' "$SCRIPT" && ok "cleanup unmounts BTRFS" || fail "cleanup unmounts BTRFS"
135135
assert_contains "cleanup closes lock" 'LOCK_FD' "$_script_content"
136136

137+
# ── Cleanup trap: behavioral test — rmdir only if umount succeeds ──
138+
139+
section "Cleanup trap: behavioral test"
140+
141+
_RMDIR_LOG="${TESTDIR}/rmdir_calls.log"
142+
_UMOUNT_LOG="${TESTDIR}/umount_calls.log"
143+
144+
# Test 1: umount fails → rmdir must NOT be called
145+
make_mock umount "echo \"\$*\" >> '${_UMOUNT_LOG}'; exit 1"
146+
make_mock rmdir "echo \"\$*\" >> '${_RMDIR_LOG}'; exit 0"
147+
make_mock mountpoint "exit 0"
148+
149+
_TEST_SCRIPT="${TESTDIR}/test-cleanup"
150+
cat > "$_TEST_SCRIPT" << 'EOF'
151+
#!/bin/bash
152+
set -euo pipefail
153+
MOUNT_DIR="/tmp/fake-mount-dir"
154+
BTRFS_MOUNT="/run/atomic/temp_btrfs"
155+
LOCK_FD=""
156+
EOF
157+
158+
# Append the actual cleanup function from the real script
159+
sed -n '/^cleanup_rebuild()/,/^}/p' "$SCRIPT" >> "$_TEST_SCRIPT"
160+
161+
cat >> "$_TEST_SCRIPT" << 'EOF'
162+
trap cleanup_rebuild EXIT
163+
exit 0
164+
EOF
165+
chmod +x "$_TEST_SCRIPT"
166+
167+
PATH="${MOCK_BIN}:${PATH}" bash "$_TEST_SCRIPT" 2>/dev/null || true
168+
169+
if [[ -f "$_UMOUNT_LOG" ]]; then
170+
ok "cleanup calls umount (fail path)"
171+
else
172+
fail "cleanup calls umount (fail path)"
173+
fi
174+
175+
if [[ ! -f "$_RMDIR_LOG" ]]; then
176+
ok "rmdir NOT called when umount fails"
177+
else
178+
fail "rmdir NOT called when umount fails (called with: $(cat "$_RMDIR_LOG"))"
179+
fi
180+
181+
# Test 2: umount succeeds → rmdir IS called
182+
rm -f "$_RMDIR_LOG" "$_UMOUNT_LOG"
183+
184+
make_mock umount "echo \"\$*\" >> '${_UMOUNT_LOG}'; exit 0"
185+
make_mock rmdir "echo \"\$*\" >> '${_RMDIR_LOG}'; exit 0"
186+
187+
PATH="${MOCK_BIN}:${PATH}" bash "$_TEST_SCRIPT" 2>/dev/null || true
188+
189+
if [[ -f "$_UMOUNT_LOG" ]]; then
190+
ok "cleanup calls umount (success path)"
191+
else
192+
fail "cleanup calls umount (success path)"
193+
fi
194+
195+
if [[ -f "$_RMDIR_LOG" ]]; then
196+
ok "rmdir called when umount succeeds"
197+
else
198+
fail "rmdir called when umount succeeds"
199+
fi
200+
137201
# ── UKI path construction: verify in real script ──
138202

139203
section "UKI path construction: verify in real script"

0 commit comments

Comments
 (0)