From 7698405f5841fd2d3bb1f029c1c7fb03f6d26ecc Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Thu, 30 Apr 2026 15:45:28 -0700 Subject: [PATCH] Surface handler-returned errors in automation last_error The "Clean Search History" automation card kept showing a stale 'DownloadOrchestrator' object has no attribute 'base_url' error even after the underlying handler bug was fixed in 77d20e9. Root cause is in the engine, not that handler: AutomationEngine only captured uncaught exceptions into last_error. Handlers that report failure by RETURNING {'status': 'error', ...} were treated as successful from the engine's perspective, so subsequent gracefully-failing runs never updated the row to reflect the current state. Both the timer (run_automation) and event (_handle_event_trigger) paths now extract the error string from a result whose status is 'error', falling through 'error' -> 'reason' -> 'message' -> a placeholder so last_error is never None on actual failures regardless of which key the handler chose. Existing behaviour for raised exceptions and successful runs is preserved. Also normalizes _auto_clean_search_history's return key from 'reason' to 'error' so older deployed engines that only check the canonical key still see the failure. Adds 7 regression tests covering every result shape the engine might receive. --- core/automation_engine.py | 23 ++- .../automation/test_handler_error_storage.py | 134 ++++++++++++++++++ web_server.py | 2 +- 3 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 tests/automation/test_handler_error_storage.py diff --git a/core/automation_engine.py b/core/automation_engine.py index 92837476..a1789af4 100644 --- a/core/automation_engine.py +++ b/core/automation_engine.py @@ -515,7 +515,17 @@ class AutomationEngine: # Update run stats (no reschedule — event triggers don't use timers) last_result = json.dumps({k: v for k, v in merged.items() if not k.startswith('_')}) - error = result.get('error') if result.get('status') == 'error' else None + # Surface every failure mode to last_error: handlers in this codebase use + # 'error', 'reason', or 'message' interchangeably when returning gracefully. + if result.get('status') == 'error': + error = ( + result.get('error') + or result.get('reason') + or result.get('message') + or 'Handler reported failure' + ) + else: + error = None self.db.update_automation_run(automation_id, error=error, last_result=last_result) if self._history_record_fn: @@ -609,6 +619,17 @@ class AutomationEngine: try: result = handler_info['handler'](action_config) or {} logger.info(f"Automation '{auto['name']}' (id={automation_id}) executed: {result.get('status', 'ok')}") + # Handlers may signal failure by RETURNING {'status': 'error', ...} instead of + # raising. Surface that to the DB so `last_error` reflects every failure mode, + # not just uncaught exceptions. Falls back through ('error', 'reason', 'message') + # because handlers in this codebase aren't consistent about which key they set. + if result.get('status') == 'error': + error = ( + result.get('error') + or result.get('reason') + or result.get('message') + or 'Handler reported failure' + ) except Exception as e: error = str(e) result = {'status': 'error', 'error': error} diff --git a/tests/automation/test_handler_error_storage.py b/tests/automation/test_handler_error_storage.py new file mode 100644 index 00000000..e7eaeefd --- /dev/null +++ b/tests/automation/test_handler_error_storage.py @@ -0,0 +1,134 @@ +"""Regression tests for AutomationEngine handler-error storage. + +The Discord-reported "Clean Search History" error +(`'DownloadOrchestrator' object has no attribute 'base_url'`) stayed +visible on the automation card long after the underlying handler bug +was fixed because the engine only stored uncaught exceptions in +``last_error``. Handlers that report failure by RETURNING +``{'status': 'error', ...}`` were treated as successful from the +engine's perspective, so subsequent successful runs never cleared the +stale error. + +These tests pin the new behaviour: every reported failure mode +(``status=error`` with any of ``error`` / ``reason`` / ``message``, +or no key at all) must surface to ``update_automation_run`` so the +DB row reflects reality and a successful next run clears it. +""" + +from unittest.mock import MagicMock + +import pytest + +from core.automation_engine import AutomationEngine + + +@pytest.fixture +def engine_with_handler(): + """Build an AutomationEngine with a stub DB and a stub handler we can swap. + + Returns a tuple of (engine, db_mock, set_handler) where set_handler + swaps in the handler that the next run_automation call will execute. + """ + db_mock = MagicMock() + db_mock.get_automation.return_value = { + 'id': 1, + 'name': 'Clean Search History', + 'enabled': True, + 'action_type': 'clean_search_history', + 'action_config': '{}', + 'trigger_type': 'interval_hours', + 'trigger_config': '{"hours": 1}', + } + db_mock.update_automation_run = MagicMock(return_value=True) + + engine = AutomationEngine(db_mock) + engine._running = True + + handler_holder = {'fn': lambda config: {'status': 'completed'}} + + def set_handler(fn): + handler_holder['fn'] = fn + + engine._action_handlers['clean_search_history'] = { + 'handler': lambda config: handler_holder['fn'](config), + 'guard': None, + } + return engine, db_mock, set_handler + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +def test_successful_run_clears_last_error(engine_with_handler) -> None: + """A clean run must save error=None so any stale stored error clears.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'completed'}) + engine.run_automation(1, skip_delay=True) + db_mock.update_automation_run.assert_called_once() + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') is None + + +def test_handler_returning_error_key_stores_it(engine_with_handler) -> None: + """status=error with an 'error' key must populate last_error.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'error', 'error': "no attribute 'base_url'"}) + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') == "no attribute 'base_url'" + + +def test_handler_returning_reason_key_stores_it(engine_with_handler) -> None: + """Older handlers use 'reason' instead of 'error'. Must still surface.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'error', 'reason': 'slskd unreachable'}) + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') == 'slskd unreachable' + + +def test_handler_returning_message_key_stores_it(engine_with_handler) -> None: + """Some action handlers use 'message'. Must still surface.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'error', 'message': 'rate limited'}) + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') == 'rate limited' + + +def test_handler_returning_error_status_with_no_message_stores_placeholder( + engine_with_handler, +) -> None: + """status=error with no detail key must still record SOMETHING so + last_error is non-null and the UI can flag the run as failed.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'error'}) + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') == 'Handler reported failure' + + +def test_handler_raising_exception_still_stores_error(engine_with_handler) -> None: + """The original behaviour (uncaught exceptions get caught + stored) + must keep working — this is the case that originally surfaced the + Discord-reported AttributeError before the fix.""" + engine, db_mock, set_handler = engine_with_handler + + def raising_handler(config): + raise AttributeError("'DownloadOrchestrator' object has no attribute 'base_url'") + set_handler(raising_handler) + + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') and 'base_url' in kwargs['error'] + + +def test_skipped_status_records_no_error(engine_with_handler) -> None: + """status=skipped is a normal outcome, must not look like a failure.""" + engine, db_mock, set_handler = engine_with_handler + set_handler(lambda config: {'status': 'skipped', 'reason': 'not configured'}) + engine.run_automation(1, skip_delay=True) + kwargs = db_mock.update_automation_run.call_args.kwargs + assert kwargs.get('error') is None diff --git a/web_server.py b/web_server.py index c300d46d..ef7b5970 100644 --- a/web_server.py +++ b/web_server.py @@ -2004,7 +2004,7 @@ def _register_automation_handlers(): log_line='No cleanup needed', log_type='skip') return {'status': 'completed'} except Exception as e: - return {'status': 'error', 'reason': str(e)} + return {'status': 'error', 'error': str(e)} def _auto_clean_completed_downloads(config): """Clear completed downloads and empty directories."""