Skip to content

Prevent localmove.py from excess logging #471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgarciabriseno merged 2 commits into from
Feb 5, 2026
Merged

Prevent localmove.py from excess logging #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cd3ee6
Prevent localmove.py from excess logging
Feb 4, 2026
1a21bcf
Merge branch 'main' into logging
dgarciabriseno Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions install/__test__/test_localmove_retry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
29 changes: 23 additions & 6 deletions install/helioviewer/hvpull/downloader/localmove.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
Expand All @@ -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...

Expand All @@ -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)