diff --git a/install/__test__/test_localmove_retry.py b/install/__test__/test_localmove_retry.py new file mode 100644 index 000000000..8cf6c5662 --- /dev/null +++ b/install/__test__/test_localmove_retry.py @@ -0,0 +1,57 @@ +"""Tests for LocalFileMove retry limit functionality""" +import unittest +from unittest.mock import patch, MagicMock +from queue import Queue + +from helioviewer.hvpull.downloader.localmove import LocalFileMove, MAX_RETRY_ATTEMPTS + + +class TestLocalFileMoveRetry(unittest.TestCase): + """Test that file move retries are limited to MAX_RETRY_ATTEMPTS""" + + def setUp(self): + self.queue = Queue() + self.mover = LocalFileMove("/tmp/incoming", self.queue) + + @patch('shutil.move') + @patch('os.path.exists', return_value=True) + def test_successful_move_does_not_retry(self, mock_exists, mock_move): + """A successful move should not add anything to the queue""" + self.mover.process(["server1", 100, "/source/file.jp2"]) + + mock_move.assert_called_once() + self.assertTrue(self.queue.empty()) + + @patch('shutil.move', side_effect=IOError("File busy")) + @patch('os.path.exists', return_value=True) + def test_failed_move_retries_up_to_max(self, mock_exists, mock_move): + """A failing move should retry MAX_RETRY_ATTEMPTS times then give up""" + # First attempt (retry_count=0) + self.mover.process(["server1", 100, "/source/file.jp2"]) + + # Should be re-queued with retry_count=1 + self.assertEqual(self.queue.qsize(), 1) + item = self.queue.get() + self.assertEqual(item[3], 1) # retry_count = 1 + + # Second attempt (retry_count=1) + self.mover.process(item) + item = self.queue.get() + self.assertEqual(item[3], 2) # retry_count = 2 + + # Third attempt (retry_count=2) + self.mover.process(item) + item = self.queue.get() + self.assertEqual(item[3], 3) # retry_count = 3 + + # Fourth attempt (retry_count=3) - should give up, not re-queue + self.mover.process(item) + self.assertTrue(self.queue.empty(), "Should not retry after MAX_RETRY_ATTEMPTS") + + def test_max_retry_attempts_is_three(self): + """Verify the constant is set to 3""" + self.assertEqual(MAX_RETRY_ATTEMPTS, 3) + + +if __name__ == '__main__': + unittest.main() diff --git a/install/helioviewer/hvpull/downloader/localmove.py b/install/helioviewer/hvpull/downloader/localmove.py index fdae3b981..b234ebf5b 100644 --- a/install/helioviewer/hvpull/downloader/localmove.py +++ b/install/helioviewer/hvpull/downloader/localmove.py @@ -6,6 +6,9 @@ import shutil from .downloader_interface import Downloader +# Maximum number of retry attempts per file before giving up +MAX_RETRY_ATTEMPTS = 3 + class LocalFileMove(Downloader): def __init__(self, incoming, queue): """Creates a new LocalFileMover""" @@ -16,9 +19,17 @@ def __init__(self, incoming, queue): # self.queue def process(self, item): - """Downloads the file at the specified URI""" - #ValueError: too many values to unpack - server, percent, uri = item + """Downloads the file at the specified URI + + Item format: [server, percent, uri] or [server, percent, uri, retry_count] + The retry_count is optional and defaults to 0 for backward compatibility. + """ + # Support both old format (3 elements) and new format (4 elements with retry count) + if len(item) == 3: + server, percent, uri = item + retry_count = 0 + else: + server, percent, uri, retry_count = item # @TODO: compute path to download file to... @@ -38,8 +49,14 @@ def process(self, item): t2 = time.time() logging.info("(%s) locally moved %s to %s", server, uri, filepath) except IOError: - # If download fails, add back into queue and try again later - logging.warning("Failed to move %s. Adding to end of queue to retry later.", uri) - self.queue.put([server, percent, uri]) + # If move fails, check retry count before re-queuing + if retry_count < MAX_RETRY_ATTEMPTS: + retry_count += 1 + logging.warning("Failed to move %s. Adding to end of queue to retry later (attempt %d/%d).", + uri, retry_count, MAX_RETRY_ATTEMPTS) + self.queue.put([server, percent, uri, retry_count]) + else: + logging.error("Failed to move %s after %d attempts. Giving up on this file.", + uri, MAX_RETRY_ATTEMPTS) except: logging.warning("Failed to move %s.", uri)