Skip to content

Commit

Permalink
Revert download retry logic
Browse files Browse the repository at this point in the history 
Clear download errors once at start. If downloads fail because of
decryption errors, they'll remain in an error state until restart,
preventing any GPG notifications.
  • Loading branch information
@rmol
rmol committed Apr 21, 2020
1 parent eb49eca commit d6ea335
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 66 deletions.
16 changes: 0 additions & 16 deletions securedrop_client/db.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,6 @@ def __str__(self) -> str:
def __repr__(self) -> str:
return '<Message {}: {}>'.format(self.uuid, self.filename)

def download_failed_since(self, interval: datetime.timedelta) -> bool:
"""
Returns True if the message has a download error and was updated since `interval` ago.
"""
return (
self.download_error and self.last_updated > (datetime.datetime.utcnow() - interval)
)

def location(self, data_dir: str) -> str:
'''
Return the full path to the Message's file.
Expand Down Expand Up @@ -336,14 +328,6 @@ def __str__(self) -> str:
def __repr__(self) -> str:
return '<Reply {}: {}>'.format(self.uuid, self.filename)

def download_failed_since(self, interval: datetime.timedelta) -> bool:
"""
Returns True if the reply has a download error and was updated since `interval` ago.
"""
return (
self.download_error and self.last_updated > (datetime.datetime.utcnow() - interval)
)

def location(self, data_dir: str) -> str:
'''
Return the full path to the Reply's file.
Expand Down
33 changes: 11 additions & 22 deletions securedrop_client/logic.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,9 @@ class Controller(QObject):
"""
source_deletion_failed = pyqtSignal(str)

# if a download fails, it won't be retried for this long
download_failure_retry_interval = datetime.timedelta(minutes=10)

def __init__(
self, hostname: str, gui, session_maker: sessionmaker,
home: str, proxy: bool = True, qubes: bool = True,
download_failure_retry_interval: datetime.timedelta = None
home: str, proxy: bool = True, qubes: bool = True
) -> None:
"""
The hostname, gui and session objects are used to coordinate with the
Expand Down Expand Up @@ -305,13 +301,6 @@ def __init__(
# Path to the file containing the timestamp since the last sync with the server
self.last_sync_filepath = os.path.join(home, 'sync_flag')

if isinstance(download_failure_retry_interval, datetime.timedelta):
self.download_failure_retry_interval = download_failure_retry_interval

logger.info(
f"Controller download failure retry interval is {self.download_failure_retry_interval}"
)

@property
def is_authenticated(self) -> bool:
return self.__is_authenticated
Expand Down Expand Up @@ -347,6 +336,8 @@ def setup(self):
self.export.moveToThread(self.export_thread)
self.export_thread.start()

storage.clear_download_errors(self.session)

def call_api(self,
api_call_func,
success_callback,
Expand Down Expand Up @@ -643,11 +634,10 @@ def download_new_messages(self) -> None:
self.set_status(_('Retrieving new messages'), 2500)

for message in new_messages:
if message.download_failed_since(self.download_failure_retry_interval):
if logger.isEnabledFor(logging.DEBUG):
logger.debug(
f"Download of message {message.uuid} failed recently; not retrying yet."
)
if message.download_error:
logger.info(
f"Download of message {message.uuid} failed since client start; not retrying."
)
else:
self._submit_download_job(type(message), message.uuid)

Expand Down Expand Up @@ -680,11 +670,10 @@ def on_message_download_failure(self, exception: DownloadException) -> None:
def download_new_replies(self) -> None:
replies = storage.find_new_replies(self.session)
for reply in replies:
if reply.download_failed_since(self.download_failure_retry_interval):
if logger.isEnabledFor(logging.DEBUG):
logger.debug(
f"Download of reply {reply.uuid} failed recently; not retrying yet."
)
if reply.download_error:
logger.info(
f"Download of reply {reply.uuid} failed since client start; not retrying."
)
else:
self._submit_download_job(type(reply), reply.uuid)

Expand Down
10 changes: 10 additions & 0 deletions securedrop_client/storage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,3 +611,13 @@ def mark_all_pending_drafts_as_failed(session: Session) -> List[DraftReply]:
session.commit()

return pending_drafts


def clear_download_errors(session: Session) -> None:
"""
Clears all File, Message, or Reply download errors.
"""
session.execute("""UPDATE files SET download_error_id = null;""")
session.execute("""UPDATE messages SET download_error_id = null;""")
session.execute("""UPDATE replies SET download_error_id = null;""")
session.commit()
37 changes: 9 additions & 28 deletions tests/test_logic.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def test_Controller_init(homedir, config, mocker, session_maker):
assert co.api_threads == {}


def test_Controller_setup(homedir, config, mocker, session_maker):
def test_Controller_setup(homedir, config, mocker, session_maker, session):
"""
Ensure the application is set up with the following default state:
Using the `config` fixture to ensure the config is written to disk.
Expand Down Expand Up @@ -1293,10 +1293,7 @@ def test_Controller_download_new_messages_skips_recent_failures(
"""
Test that `download_new_messages` skips recently failed downloads.
"""
co = Controller(
"http://localhost", mocker.MagicMock(), session_maker, homedir,
download_failure_retry_interval=datetime.timedelta(hours=1)
)
co = Controller("http://localhost", mocker.MagicMock(), session_maker, homedir)
co.api = "Api token has a value"

# record the download failures
Expand All @@ -1306,38 +1303,28 @@ def test_Controller_download_new_messages_skips_recent_failures(

message = factory.Message(source=factory.Source())
message.download_error = download_error
message.last_updated = datetime.datetime.utcnow()
session.commit()

mocker.patch("securedrop_client.storage.find_new_messages", return_value=[message])
api_job_queue = mocker.patch.object(co, "api_job_queue")
mocker.patch("securedrop_client.logic.logger.isEnabledFor", return_value=logging.DEBUG)
debug_logger = mocker.patch("securedrop_client.logic.logger.debug")
info_logger = mocker.patch("securedrop_client.logic.logger.info")

co.download_new_messages()

api_job_queue.enqueue.assert_not_called()
debug_logger.call_args_list[0][0][0] == (
f"Download of message {message.uuid} failed recently; not retrying yet."
info_logger.call_args_list[0][0][0] == (
f"Download of message {message.uuid} failed since client start; not retrying."
)

message.last_updated = (datetime.datetime.utcnow() - co.download_failure_retry_interval * 2)
session.commit()

co.download_new_messages()
api_job_queue.enqueue.assert_called_once()


def test_Controller_download_new_replies_skips_recent_failures(
mocker, session, session_maker, homedir, download_error_codes
):
"""
Test that `download_new_replies` skips recently failed downloads.
"""
co = Controller(
"http://localhost", mocker.MagicMock(), session_maker, homedir,
download_failure_retry_interval=datetime.timedelta(hours=1)
)
co = Controller("http://localhost", mocker.MagicMock(), session_maker, homedir)
co.api = "Api token has a value"

# record the download failures
Expand All @@ -1353,21 +1340,15 @@ def test_Controller_download_new_replies_skips_recent_failures(
mocker.patch("securedrop_client.storage.find_new_replies", return_value=[reply])
api_job_queue = mocker.patch.object(co, "api_job_queue")
mocker.patch("securedrop_client.logic.logger.isEnabledFor", return_value=logging.DEBUG)
debug_logger = mocker.patch("securedrop_client.logic.logger.debug")
info_logger = mocker.patch("securedrop_client.logic.logger.info")

co.download_new_replies()

api_job_queue.enqueue.assert_not_called()
debug_logger.call_args_list[0][0][0] == (
f"Download of reply {reply.uuid} failed recently; not retrying yet."
info_logger.call_args_list[0][0][0] == (
f"Download of reply {reply.uuid} failed since client start; not retrying."
)

reply.last_updated = (datetime.datetime.utcnow() - co.download_failure_retry_interval * 2)
session.commit()

co.download_new_replies()
api_job_queue.enqueue.assert_called_once()


def test_Controller_on_message_downloaded_success(mocker, homedir, session_maker):
"""
Expand Down

0 comments on commit d6ea335

Please sign in to comment.