diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 002d09d..b0eedc5 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -30,10 +30,10 @@ Acceptance criteria: ### Replace PID-file process killing with a real lock -- [ ] Use `fcntl.flock()` and hold the descriptor for the full operation. -- [ ] Keep PID and process-start metadata for diagnostics only. -- [ ] Remove process killing from `--force` behavior. -- [ ] Verify lock ownership before releasing or replacing metadata. +- [x] Use `fcntl.flock()` and hold the descriptor for the full operation. +- [x] Keep PID and process-start metadata for diagnostics only. +- [x] Remove process killing from `--force` behavior. +- [x] Verify lock ownership before releasing or replacing metadata. Acceptance criteria: diff --git a/curateipsum/backup.py b/curateipsum/backup.py index 46e930e..46ffc35 100644 --- a/curateipsum/backup.py +++ b/curateipsum/backup.py @@ -1,14 +1,21 @@ """ Module with backup functions. """ -import errno import logging import os import shutil -import signal import time from datetime import datetime, timedelta -from typing import Optional, Iterable, Union +from typing import Dict, Optional, Iterable, Union + +try: + import fcntl +except ImportError: + # Not available on Windows. cli.py's own platform check reports this + # cleanly before any locking function here is ever called; importing + # fcntl lazily/optionally lets that check run instead of crashing at + # `from curateipsum import backup` time. + fcntl = None from curateipsum import fs @@ -81,109 +88,115 @@ def _date_from_backup(backup_entry: os.DirEntry) -> datetime: return datetime.strptime(backup_entry.name, BACKUP_ENT_FMT) -def _pid_exists(pid: int) -> bool: - """Check whether pid exists in the current process table.""" - if pid == 0: - # According to "man 2 kill" PID 0 has a special meaning: - # it refers to <> so we don't want to go any further. - # If we get here it means this UNIX platform *does* have - # a process with id 0. - return True +# Open lock file descriptors currently held by this process, keyed by the +# absolute path of the backups directory they lock. The kernel associates +# an flock() with this exact open file description, so the descriptor must +# stay open for the full duration of the operation: closing it (including +# on process crash or signal) is what releases the lock. +_held_locks: Dict[str, int] = {} + + +_LOCK_METADATA_MAX_BYTES = 256 + + +def _read_lock_metadata(lock_file_path: str) -> str: + """ + Read lock file content for diagnostic logging only. Never raises, and + never trusted for anything beyond a human-readable log line: the read + is capped and newlines are stripped so a stale or forged lock file + can't inject bogus log lines or force an unbounded read. + """ try: - os.kill(pid, 0) - except OSError as err: - if err.errno == errno.ESRCH: - # ESRCH == No such process - return False - elif err.errno == errno.EPERM: - # EPERM clearly means there's a process to deny access to - return True - else: - # According to "man 2 kill" possible error values are - # (EINVAL, EPERM, ESRCH) therefore we should never get - # here. If we do let's be explicit in considering this - # an error. - raise err - else: - return True + with open(lock_file_path, "r") as f: + content = f.read(_LOCK_METADATA_MAX_BYTES) + except OSError: + return "" + return content.replace("\n", " ").replace("\r", " ").strip() def set_backups_lock(backups_dir: str, force: bool = False) -> bool: """ - Set lock file to prevent multiple backups running at the same time. - Lock file contains PID of the process that created it. - Return false if previous backup is still running and force flag is not set. + Acquire an exclusive kernel (flock) lock to prevent multiple backups + running at the same time. The lock file also holds the PID and start + time of the holder, kept for diagnostics only - they play no part in + deciding whether the lock can be acquired. + + If another process already holds the lock: + - with force=False, return False immediately. + - with force=True, block until that process releases it (normal exit, + signal, or crash all release the kernel lock), then acquire it. """ - lock_file_path = os.path.join(backups_dir, LOCK_FILE) - - # Try to create lock file atomically - try: - fd = os.open(lock_file_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644) - os.write(fd, str(os.getpid()).encode()) - os.close(fd) - return True - except FileExistsError: - # Lock file already exists, check if process is still running - pass - - # Read existing lock file - try: - with open(lock_file_path, "r") as f: - content = f.read().strip() - if not content: - raise ValueError("Lock file is empty") - pid = int(content) - except (ValueError, IOError) as e: - _lg.warning("Corrupted lock file (%s), removing and retrying", e) - try: - os.unlink(lock_file_path) - except OSError: - pass - return set_backups_lock(backups_dir, force) - - if _pid_exists(pid): - if not force: - _lg.warning( - "Previous backup is still in progress (PID: %d), exiting", pid - ) - return False - - _lg.warning( - "Previous backup is still in progress (PID: %d), " - "but force flag is set, attempting graceful termination", pid - ) - # Try SIGTERM first for graceful shutdown - try: - os.kill(pid, signal.SIGTERM) - _lg.info("Sent SIGTERM to process %d, waiting 5 seconds", pid) - time.sleep(5) - - # Check if process is still running - if _pid_exists(pid): - _lg.warning("Process %d did not terminate, sending SIGKILL", pid) - os.kill(pid, signal.SIGKILL) - time.sleep(1) # Brief wait for SIGKILL to take effect - except OSError as e: - _lg.error("Failed to kill process %d: %s", pid, e) - return False - - # Remove stale lock file and retry - try: - os.unlink(lock_file_path) - except OSError as e: - _lg.error("Failed to remove lock file: %s", e) + backups_dir_abs = os.path.abspath(backups_dir) + if backups_dir_abs in _held_locks: + _lg.error("This process already holds the backup lock for %s", + backups_dir) return False - return set_backups_lock(backups_dir, force) + lock_file_path = os.path.join(backups_dir_abs, LOCK_FILE) + + fd = os.open(lock_file_path, os.O_CREAT | os.O_RDWR, 0o644) + + flock_flags = fcntl.LOCK_EX if force else fcntl.LOCK_EX | fcntl.LOCK_NB + try: + fcntl.flock(fd, flock_flags) + except BlockingIOError: + held_by = _read_lock_metadata(lock_file_path) + _lg.warning( + "Previous backup is still in progress (%s), exiting", held_by + ) + os.close(fd) + return False + except OSError as err: + _lg.error("Failed to acquire backup lock: %s", err) + os.close(fd) + return False + + # Lock acquired: replace metadata, it belongs to us now. If writing it + # fails, don't leave the lock held forever - release and report failure. + try: + os.ftruncate(fd, 0) + os.lseek(fd, 0, os.SEEK_SET) + metadata = "%d\n%s\n" % (os.getpid(), datetime.now().isoformat()) + os.write(fd, metadata.encode()) + os.fsync(fd) + except OSError as err: + _lg.error("Failed to write backup lock metadata: %s", err) + try: + fcntl.flock(fd, fcntl.LOCK_UN) + finally: + os.close(fd) + return False + + _held_locks[backups_dir_abs] = fd + return True def release_backups_lock(backups_dir: str): - """Remove lock file.""" - lock_file_path = os.path.join(backups_dir, LOCK_FILE) - if os.path.exists(lock_file_path): - os.unlink(lock_file_path) + """ + Release the kernel lock held by this process for backups_dir, if any. + Only releases a lock this process actually acquired via + set_backups_lock() - verified by looking up our own held descriptor, + never by trusting the lock file's on-disk content. + """ + backups_dir_abs = os.path.abspath(backups_dir) + fd = _held_locks.pop(backups_dir_abs, None) + if fd is None: + _lg.debug("No lock held by this process for %s, nothing to release", + backups_dir) + return + + try: + fcntl.flock(fd, fcntl.LOCK_UN) + finally: + os.close(fd) + # Deliberately not unlinking the lock file: flock() locks an inode, not + # a path. Unlinking it here while another process is blocked opening + # the same path (force=True) would let that process (or a third one) + # create a fresh inode at the same path and acquire an independent + # lock, defeating mutual exclusion. The file is small and harmless to + # leave in place; only its content (diagnostics) gets overwritten by + # whoever next holds the lock. def set_backup_marker(backup_entry: Union[os.DirEntry, fs.PseudoDirEntry]): diff --git a/curateipsum/cli.py b/curateipsum/cli.py index fd4701a..8110c7d 100755 --- a/curateipsum/cli.py +++ b/curateipsum/cli.py @@ -44,7 +44,8 @@ def main(): parser.add_argument("-f", "--force", action="store_true", default=False, - help="Force run when previous backup is still in process") + help="Wait for previous backup to finish instead of " + "exiting immediately if one is still running") parser.add_argument("--external-rsync", action="store_true", default=False, diff --git a/tests/test_backups.py b/tests/test_backups.py index 51d725b..43ec16d 100644 --- a/tests/test_backups.py +++ b/tests/test_backups.py @@ -1,5 +1,9 @@ +import fcntl import os -import signal +import subprocess +import sys +import threading +import time import pytest from unittest import mock from datetime import datetime @@ -384,7 +388,12 @@ class TestBackupCleanup: check_backups(expected) class TestBackupLock: - """Test suite for backup lock file functionality.""" + """Test suite for the fcntl.flock()-based backup lock.""" + + @staticmethod + def _read_lock_pid(lock_path: str) -> int: + with open(lock_path, "r") as f: + return int(f.readline().strip()) def test_lock_creation(self, backup_dir): """Test that lock file is created with current PID""" @@ -394,193 +403,208 @@ class TestBackupLock: lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) assert os.path.exists(lock_path) + assert self._read_lock_pid(lock_path) == os.getpid() - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() + bk.release_backups_lock(str(backup_dir)) - def test_lock_prevents_concurrent_backup(self, backup_dir): - """Test that second lock acquisition is blocked""" - backup_dir.mkdir() - # First lock should succeed - result1 = bk.set_backups_lock(str(backup_dir)) - assert result1 - - # The second lock should fail (same process trying to lock again) - # Write a different PID to simulate another process - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - with open(lock_path, "w") as f: - f.write(str(os.getpid())) - - result2 = bk.set_backups_lock(str(backup_dir), force=False) - assert not result2 - - def test_stale_lock_is_removed(self, backup_dir): - """Test that lock from non-existent process is cleaned up""" - backup_dir.mkdir() - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - - # Create lock with non-existent PID - with open(lock_path, "w") as f: - f.write("999999") - - # Lock should succeed by removing stale lock - result = bk.set_backups_lock(str(backup_dir)) - assert result - - # Verify new lock has current PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() - - def test_corrupted_lock_is_handled(self, backup_dir): - """Test that corrupted lock file is handled gracefully""" - backup_dir.mkdir() - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - - # Create corrupted lock file (non-numeric content) - with open(lock_path, "w") as f: - f.write("not a number") - - # Lock should succeed by removing corrupted lock - result = bk.set_backups_lock(str(backup_dir)) - assert result - - # Verify new lock has current PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() - - def test_empty_lock_is_handled(self, backup_dir): - """Test that empty lock file is handled gracefully""" - backup_dir.mkdir() - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - - # Create the empty lock file - open(lock_path, "w").close() - - # Lock should succeed by removing empty lock - result = bk.set_backups_lock(str(backup_dir)) - assert result - - # Verify new lock has current PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() - - def test_lock_release(self, backup_dir): - """Test that lock file is properly released""" + def test_lock_metadata_includes_start_time(self, backup_dir): + """Test that lock file records a parseable start timestamp""" backup_dir.mkdir() bk.set_backups_lock(str(backup_dir)) lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) + + with open(lock_path, "r") as f: + lines = f.read().splitlines() + assert int(lines[0]) == os.getpid() + datetime.fromisoformat(lines[1]) # must not raise + + bk.release_backups_lock(str(backup_dir)) + + def test_lock_prevents_concurrent_backup(self, backup_dir): + """ + Test that a second holder of the kernel lock blocks a non-forced + acquisition attempt, using a real flock() held on a separate file + descriptor to stand in for another process. + """ + backup_dir.mkdir() + lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) + + other_fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(other_fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + os.write(other_fd, b"12345\n2024-01-01T00:00:00\n") + + try: + result = bk.set_backups_lock(str(backup_dir), force=False) + assert not result + finally: + fcntl.flock(other_fd, fcntl.LOCK_UN) + os.close(other_fd) + + @pytest.mark.parametrize("garbage", ["999999", "not a number", ""]) + def test_lock_acquired_despite_garbage_content(self, backup_dir, garbage): + """ + A leftover lock file with stale/corrupted/empty content must not + block acquisition: the kernel lock, not the file content, is what + decides ownership. The old process's death already released it. + """ + backup_dir.mkdir() + lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) + with open(lock_path, "w") as f: + f.write(garbage) + + result = bk.set_backups_lock(str(backup_dir)) + assert result + assert self._read_lock_pid(lock_path) == os.getpid() + + bk.release_backups_lock(str(backup_dir)) + + def test_lock_release(self, backup_dir): + """Test that a held lock can be released and re-acquired""" + backup_dir.mkdir() + assert bk.set_backups_lock(str(backup_dir)) + lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) assert os.path.exists(lock_path) bk.release_backups_lock(str(backup_dir)) - assert not os.path.exists(lock_path) + + # lock file itself is intentionally left in place (see comment in + # release_backups_lock); the kernel lock must be free though + assert bk.set_backups_lock(str(backup_dir)) + bk.release_backups_lock(str(backup_dir)) def test_release_nonexistent_lock(self, backup_dir): - """Test that releasing non-existent lock doesn't raise error""" + """Releasing a lock never acquired by this process is a no-op""" backup_dir.mkdir() # Should not raise any exception bk.release_backups_lock(str(backup_dir)) - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - assert not os.path.exists(lock_path) + def test_reacquiring_own_lock_does_not_leak_fd(self, backup_dir): + """ + A second set_backups_lock() call for a directory this process + already holds must fail cleanly instead of silently overwriting + the tracked descriptor (which would leak the original fd and let + release_backups_lock() close only the newest one, unlocking + nothing). + """ + backup_dir.mkdir() + assert bk.set_backups_lock(str(backup_dir)) + try: + assert not bk.set_backups_lock(str(backup_dir)) + finally: + bk.release_backups_lock(str(backup_dir)) - @mock.patch(f"{bk.__name__}.time.sleep") - @mock.patch(f"{bk.__name__}.os.kill") - @mock.patch(f"{bk.__name__}._pid_exists") - def test_force_lock_with_sigterm_success(self, mock_pid_exists, mock_kill, - mock_sleep, backup_dir): - """Test force flag sends SIGTERM and acquires lock when process stops""" + # The lock must be fully free after a single release. + assert bk.set_backups_lock(str(backup_dir)) + bk.release_backups_lock(str(backup_dir)) + + def test_release_only_releases_own_lock(self, backup_dir): + """ + release_backups_lock() must only release a lock this process + acquired via set_backups_lock(), never one it merely knows about + through the lock file's on-disk content. + """ backup_dir.mkdir() lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - # Create lock with PID 12345 - with open(lock_path, "w") as f: - f.write("12345") + other_fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(other_fd, fcntl.LOCK_EX | fcntl.LOCK_NB) - # Simulate process exists initially, then stops after SIGTERM - mock_pid_exists.side_effect = [True, False] + try: + # We never successfully acquired this lock ourselves. + bk.release_backups_lock(str(backup_dir)) + + # The other holder's lock must still be intact: a fresh + # non-blocking acquisition attempt must still fail. + assert not bk.set_backups_lock(str(backup_dir), force=False) + finally: + fcntl.flock(other_fd, fcntl.LOCK_UN) + os.close(other_fd) + + def test_force_waits_for_lock_release_without_killing(self, backup_dir): + """ + force=True must block until the current holder releases the lock + on its own, and must never send a signal to do it. + """ + backup_dir.mkdir() + lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) + + other_fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(other_fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + os.write(other_fd, b"12345\n2024-01-01T00:00:00\n") + + def _release_other_after_delay(): + time.sleep(0.5) + fcntl.flock(other_fd, fcntl.LOCK_UN) + os.close(other_fd) + + releaser = threading.Thread(target=_release_other_after_delay) + + with mock.patch("os.kill") as mock_kill: + releaser.start() + start = time.monotonic() + result = bk.set_backups_lock(str(backup_dir), force=True) + elapsed = time.monotonic() - start + releaser.join() - result = bk.set_backups_lock(str(backup_dir), force=True) assert result + # Must have actually waited for the release, not raced past it. + assert elapsed >= 0.4 + mock_kill.assert_not_called() - # Verify SIGTERM was sent - mock_kill.assert_called_once_with(12345, signal.SIGTERM) + assert self._read_lock_pid(lock_path) == os.getpid() + bk.release_backups_lock(str(backup_dir)) - # Verify we waited after SIGTERM - assert mock_sleep.call_count == 1 - mock_sleep.assert_any_call(5) - - # Verify new lock has current PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() - - @mock.patch(f"{bk.__name__}.time.sleep") - @mock.patch(f"{bk.__name__}.os.kill") - @mock.patch(f"{bk.__name__}._pid_exists") - def test_force_lock_requires_sigkill(self, mock_pid_exists, mock_kill, - mock_sleep, backup_dir): - """Test force flag escalates to SIGKILL when SIGTERM fails""" + def test_multiprocess_only_one_holder_and_crash_releases_lock( + self, backup_dir + ): + """ + End-to-end proof with a real second process (not a mock): + - only one process can hold the lock at a time. + - killing the holder with SIGKILL (simulating a crash) releases + the kernel lock automatically, with no PID/metadata inspection + needed on our side, and no other process is ever signaled. + """ backup_dir.mkdir() lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - # Create lock with PID 12345 - with open(lock_path, "w") as f: - f.write("12345") + holder_script = ( + "import fcntl, os, sys, time;" + "fd = os.open(sys.argv[1], os.O_CREAT | os.O_RDWR, 0o644);" + "fcntl.flock(fd, fcntl.LOCK_EX);" + "print('locked', flush=True);" + "time.sleep(30)" + ) + holder = subprocess.Popen( + [sys.executable, "-c", holder_script, lock_path], + stdout=subprocess.PIPE, text=True, + ) + try: + # Wait for the holder to actually acquire the lock. + line = holder.stdout.readline() + assert line.strip() == "locked" - # Simulate process survives SIGTERM, dies after SIGKILL - mock_pid_exists.side_effect = [True, True, False] + # A second, non-forced acquisition attempt must fail while the + # holder is alive. + assert not bk.set_backups_lock(str(backup_dir), force=False) - result = bk.set_backups_lock(str(backup_dir), force=True) - assert result + # Simulate the holder crashing. Do this outside the os.kill + # patch below: Popen.kill() itself calls os.kill() and we only + # want to observe *our* code's behavior, not intercept the + # test's own process teardown. + holder.kill() + holder.wait(timeout=5) - # Verify both SIGTERM and SIGKILL were sent - assert mock_kill.call_count == 2 - mock_kill.assert_any_call(12345, signal.SIGTERM) - mock_kill.assert_any_call(12345, signal.SIGKILL) - - # Verify sleep was called twice (5s after SIGTERM, 1s after SIGKILL) - assert mock_sleep.call_count == 2 - mock_sleep.assert_any_call(5) - mock_sleep.assert_any_call(1) - - # Verify new lock has current PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == os.getpid() - - @mock.patch(f"{bk.__name__}.time.sleep") - @mock.patch(f"{bk.__name__}.os.kill") - @mock.patch(f"{bk.__name__}._pid_exists") - def test_force_lock_handles_kill_failure(self, mock_pid_exists, mock_kill, - mock_sleep, backup_dir): - """Test force flag handles os.kill() failures gracefully""" - backup_dir.mkdir() - lock_path = os.path.join(str(backup_dir), bk.LOCK_FILE) - - # Create lock with PID 12345 - with open(lock_path, "w") as f: - f.write("12345") - - # Simulate process exists - mock_pid_exists.return_value = True - - # Simulate permission error when trying to kill - mock_kill.side_effect = OSError("Permission denied") - - result = bk.set_backups_lock(str(backup_dir), force=True) - assert not result # Should fail - - # Verify SIGTERM was attempted - mock_kill.assert_called_once_with(12345, signal.SIGTERM) - - # Lock should still exist with old PID - with open(lock_path, "r") as f: - pid = int(f.read().strip()) - assert pid == 12345 + with mock.patch("os.kill") as mock_kill: + # The kernel released the lock on process exit; we should + # be able to acquire it now without having sent any signal + # to the (now-dead) holder PID ourselves. + assert bk.set_backups_lock(str(backup_dir), force=False) + mock_kill.assert_not_called() + finally: + bk.release_backups_lock(str(backup_dir)) + if holder.poll() is None: + holder.kill() + holder.wait(timeout=5) class TestBackupIteration: diff --git a/tests/test_integration.py b/tests/test_integration.py index b7ee016..828bbc8 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -852,8 +852,10 @@ class TestErrorRecovery: # Release lock bk.release_backups_lock(str(backups_dir)) - # Lock should be removed - assert not os.path.exists(lock_path) + # Lock should be free again (re-acquirable), even though the lock + # file itself is intentionally left in place + assert bk.set_backups_lock(str(backups_dir)) + bk.release_backups_lock(str(backups_dir)) # Restore original function and verify backup can proceed monkeypatch.undo() @@ -915,8 +917,10 @@ class TestErrorRecovery: assert os.path.exists(lock_path) bk.release_backups_lock(str(backups_dir)) - # Lock should be removed - assert not os.path.exists(lock_path) + # Lock should be free again (re-acquirable), even though the lock + # file itself is intentionally left in place + assert bk.set_backups_lock(str(backups_dir)) + bk.release_backups_lock(str(backups_dir)) def test_permission_error_during_cleanup( self, integration_dirs, monkeypatch