Fix stale cron lock files blocking scheduled feed updates (#161)

Replace the PID-file locking in feed_updater and cleanup_entries with a
kernel-managed flock (LOCK_EX|LOCK_NB). The old check only tested whether
/tmp/update_feeds_<frequency>.lock existed, so a process killed before its
finally-block ran (OOM kill, container restart) left a stale lock behind
that silently skipped every subsequent run of that frequency. The flock is
released automatically by the kernel when the process exits for any reason,
so stale locks can no longer occur; the lock file now only records the
holder PID for debugging.

Author: versun

core/management/commands/cleanup_entries.py

@@ -1,5 +1,6 @@
 # Remove entries greater than feed.max_posts
 
+import fcntl
 import os
 import sys
 import time
@@ -19,29 +20,34 @@ class Command(BaseCommand):
     def handle(self, *args, **options):
         lock_file_path = "/tmp/cleanup_entries.lock"
 
-        if os.path.exists(lock_file_path):
-            self.stdout.write(
-                self.style.WARNING(
-                    f"{current_time}: Cleanup process is already running. Exiting."
+        # Kernel-managed flock: released automatically on process exit (even on
+        # crash/OOM/container stop), so it can never go stale (see issue #161).
+        with open(lock_file_path, "w") as lock_file:
+            try:
+                fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
+            except BlockingIOError:
+                self.stdout.write(
+                    self.style.WARNING(
+                        f"{current_time}: Cleanup process is already running. Exiting."
+                    )
                 )
-            )
-            sys.exit(0)
-
-        try:
-            with open(lock_file_path, "w") as f:
-                f.write(str(os.getpid()))
-
-            cleanup_all_feeds()
-            self.stdout.write(
-                self.style.SUCCESS(f"{current_time}: Successfully cleaned up all feeds")
-            )
-        except Exception as e:
-            logger.exception(f"Command cleanup_entries failed: {str(e)}")
-            self.stderr.write(self.style.ERROR(f"Error: {str(e)}"))
-            sys.exit(1)
-        finally:
-            if os.path.exists(lock_file_path):
-                os.remove(lock_file_path)
+                sys.exit(0)
+
+            # Record the holder PID for debugging only; the flock itself is the lock.
+            lock_file.write(str(os.getpid()))
+            lock_file.flush()
+
+            try:
+                cleanup_all_feeds()
+                self.stdout.write(
+                    self.style.SUCCESS(
+                        f"{current_time}: Successfully cleaned up all feeds"
+                    )
+                )
+            except Exception as e:
+                logger.exception(f"Command cleanup_entries failed: {str(e)}")
+                self.stderr.write(self.style.ERROR(f"Error: {str(e)}"))
+                sys.exit(1)
 
 
 def cleanup_feed_entries(feed: Feed):

core/management/commands/feed_updater.py

@@ -1,3 +1,4 @@
+import fcntl
 import logging
 import sys
 from itertools import chain
@@ -52,33 +53,37 @@ def handle(self, *args, **options):
 
         lock_file_path = f"/tmp/update_feeds_{target_frequency.replace(' ', '_')}.lock"
 
-        if os.path.exists(lock_file_path):
-            self.stdout.write(
-                self.style.WARNING(
-                    f"{current_time}: Another update process for frequency '{target_frequency}' is already running. Exiting."
+        # Use a kernel-managed flock instead of a plain PID file: the lock is
+        # released automatically when the process exits for ANY reason (crash,
+        # OOM kill, container stop), so it can never go stale and block all
+        # future runs of this frequency (issue #161).
+        with open(lock_file_path, "w") as lock_file:
+            try:
+                fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
+            except BlockingIOError:
+                self.stdout.write(
+                    self.style.WARNING(
+                        f"{current_time}: Another update process for frequency '{target_frequency}' is already running. Exiting."
+                    )
                 )
-            )
-            sys.exit(0)
+                sys.exit(0)
 
-        try:
-            # Create lock file
-            with open(lock_file_path, "w") as f:
-                f.write(str(os.getpid()))
-
-            update_feeds_for_frequency(simple_update_frequency=target_frequency)
-            self.stdout.write(
-                self.style.SUCCESS(
-                    f"{current_time}: Successfully updated feeds for frequency: {target_frequency}"
+            # Record the holder PID for debugging only; the flock, not this
+            # file's existence, is what actually excludes other processes.
+            lock_file.write(str(os.getpid()))
+            lock_file.flush()
+
+            try:
+                update_feeds_for_frequency(simple_update_frequency=target_frequency)
+                self.stdout.write(
+                    self.style.SUCCESS(
+                        f"{current_time}: Successfully updated feeds for frequency: {target_frequency}"
+                    )
                 )
-            )
-        except Exception as e:
-            logger.exception(f"Command update_feeds_for_frequency failed: {str(e)}")
-            self.stderr.write(self.style.ERROR(f"Error: {str(e)}"))
-            sys.exit(1)
-        finally:
-            # Ensure lock file is removed
-            if os.path.exists(lock_file_path):
-                os.remove(lock_file_path)
+            except Exception as e:
+                logger.exception(f"Command update_feeds_for_frequency failed: {str(e)}")
+                self.stderr.write(self.style.ERROR(f"Error: {str(e)}"))
+                sys.exit(1)
 
 
 def update_single_feed(feed: Feed):

core/tests/test_cleanup_entries.py

@@ -1,5 +1,6 @@
 from django.test import TestCase, override_settings
 from unittest.mock import patch, Mock, mock_open, call
+import fcntl
 import os
 import tempfile
 import shutil
@@ -244,24 +245,11 @@ def setUp(self):
                 original_content=f"Content {i}",
             )
 
-    @patch("core.management.commands.cleanup_entries.os.path.exists")
+    @patch("core.management.commands.cleanup_entries.fcntl")
     @patch("core.management.commands.cleanup_entries.open", new_callable=mock_open)
     @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
-    @patch("core.management.commands.cleanup_entries.os.remove")
-    def test_command_success(self, mock_remove, mock_cleanup, mock_file, mock_exists):
+    def test_command_success(self, mock_cleanup, mock_file, mock_fcntl):
         """Test successful command execution."""
-        # Mock lock file doesn't exist initially, but exists after creation
-        mock_exists.side_effect = [False, True]
-
-        # Mock file operations
-        mock_file.return_value.__enter__.return_value.write.return_value = None
-
-        # Mock cleanup function
-        mock_cleanup.return_value = None
-
-        # Mock os.remove
-        mock_remove.return_value = None
-
         # Capture stdout
         from io import StringIO
 
@@ -274,18 +262,18 @@ def test_command_success(self, mock_remove, mock_cleanup, mock_file, mock_exists
         # Verify cleanup was called
         mock_cleanup.assert_called_once()
 
-        # Verify lock file was created and removed
-        mock_file.assert_called_once()
-        mock_remove.assert_called_once()
+        # Verify the flock was acquired
+        mock_fcntl.flock.assert_called_once()
 
         # Verify success message
         self.assertIn("Successfully cleaned up all feeds", out.getvalue())
 
-    @patch("core.management.commands.cleanup_entries.os.path.exists")
-    def test_command_already_running(self, mock_exists):
-        """Test command when cleanup is already running."""
-        # Mock lock file exists
-        mock_exists.return_value = True
+    @patch("core.management.commands.cleanup_entries.fcntl")
+    @patch("core.management.commands.cleanup_entries.open", new_callable=mock_open)
+    @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
+    def test_command_already_running(self, mock_cleanup, mock_file, mock_fcntl):
+        """Test command when cleanup is already running (flock held elsewhere)."""
+        mock_fcntl.flock.side_effect = BlockingIOError()
 
         # Capture stdout
         from io import StringIO
@@ -303,30 +291,20 @@ def test_command_already_running(self, mock_exists):
         # Verify warning message
         self.assertIn("Cleanup process is already running", out.getvalue())
 
-    @patch("core.management.commands.cleanup_entries.os.path.exists")
+        # Verify cleanup did not run
+        mock_cleanup.assert_not_called()
+
+    @patch("core.management.commands.cleanup_entries.fcntl")
     @patch("core.management.commands.cleanup_entries.open", new_callable=mock_open)
     @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
-    @patch("core.management.commands.cleanup_entries.os.remove")
     @patch("core.management.commands.cleanup_entries.logger")
     def test_command_exception_handling(
-        self, mock_logger, mock_remove, mock_cleanup, mock_file, mock_exists
+        self, mock_logger, mock_cleanup, mock_file, mock_fcntl
     ):
         """Test command exception handling."""
-        # Mock lock file doesn't exist initially, but exists after creation
-        mock_exists.side_effect = [False, True]
-
-        # Mock file operations
-        mock_file.return_value.__enter__.return_value.write.return_value = None
-
         # Mock cleanup function to raise exception
         mock_cleanup.side_effect = Exception("Test error")
 
-        # Mock os.remove
-        mock_remove.return_value = None
-
-        # Mock logger to suppress output
-        mock_logger.exception.return_value = None
-
         # Capture stderr
         from io import StringIO
 
@@ -343,42 +321,54 @@ def test_command_exception_handling(
         # Verify error message
         self.assertIn("Test error", err.getvalue())
 
-        # Verify lock file was still removed
-        mock_remove.assert_called_once()
 
-    @patch("core.management.commands.cleanup_entries.os.path.exists")
-    @patch("core.management.commands.cleanup_entries.open", new_callable=mock_open)
-    @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
-    @patch("core.management.commands.cleanup_entries.os.remove")
-    @patch("core.management.commands.cleanup_entries.logger")
-    def test_command_lock_file_cleanup_on_exception(
-        self, mock_logger, mock_remove, mock_cleanup, mock_file, mock_exists
-    ):
-        """Test that lock file is cleaned up even when exception occurs."""
-        # Mock lock file doesn't exist initially, but exists after creation
-        mock_exists.side_effect = [False, True]
+class CleanupEntriesLockRegressionTests(TestCase):
+    """Regression tests for issue #161 using a real lock file on disk."""
 
-        # Mock file operations
-        mock_file.return_value.__enter__.return_value.write.return_value = None
+    lock_path = "/tmp/cleanup_entries.lock"
 
-        # Mock cleanup function to raise exception
-        mock_cleanup.side_effect = Exception("Test error")
+    def tearDown(self):
+        if os.path.exists(self.lock_path):
+            os.remove(self.lock_path)
 
-        # Mock os.remove
-        mock_remove.return_value = None
+    @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
+    def test_stale_lock_file_does_not_block_cleanup(self, mock_cleanup):
+        """A leftover lock file from a dead process must not block the next run."""
+        # Simulate the stale lock left behind by a killed process (issue #161):
+        # the file exists and contains a dead PID, but nobody holds the flock.
+        with open(self.lock_path, "w") as f:
+            f.write("999999")
 
-        # Mock logger to suppress output
-        mock_logger.exception.return_value = None
+        command = Command()
+        with patch.object(command, "stdout"), patch.object(command, "stderr"):
+            command.handle()
 
-        # Mock stderr to suppress error output
-        from io import StringIO
+        mock_cleanup.assert_called_once()
 
-        mock_stderr = StringIO()
-        self.command.stderr = mock_stderr
+    @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
+    def test_lock_held_by_live_process_blocks_cleanup(self, mock_cleanup):
+        """While a live process holds the flock, the command exits without cleaning."""
+        with open(self.lock_path, "w") as holder:
+            fcntl.flock(holder.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
 
-        # Execute command
-        with self.assertRaises(SystemExit):
-            self.command.handle()
+            command = Command()
+            with patch.object(command, "stdout"), patch.object(command, "stderr"):
+                with self.assertRaises(SystemExit) as ctx:
+                    command.handle()
+            self.assertEqual(ctx.exception.code, 0)
+
+        mock_cleanup.assert_not_called()
 
-        # Verify lock file was removed in finally block
-        mock_remove.assert_called_once()
+    @patch("core.management.commands.cleanup_entries.cleanup_all_feeds")
+    def test_lock_released_after_failed_run(self, mock_cleanup):
+        """A failed run releases the lock, so the next run still proceeds."""
+        command = Command()
+        with patch.object(command, "stdout"), patch.object(command, "stderr"):
+            mock_cleanup.side_effect = Exception("boom")
+            with self.assertRaises(SystemExit):
+                command.handle()
+
+            mock_cleanup.side_effect = None
+            command.handle()
+
+        self.assertEqual(mock_cleanup.call_count, 2)