-
Notifications
You must be signed in to change notification settings - Fork 327
fix: singleton WorkflowService + mark task failed on load error #153
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.
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
Open
devteamaegis
wants to merge
1
commit into
browser-use:main
Choose a base branch
from
devteamaegis:fix/workflow-service-singleton-and-load-failure-status
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+231
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
222 changes: 222 additions & 0 deletions
222
workflows/backend/tests/test_service_singleton_and_load_failure.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,222 @@ | ||
| """ | ||
| Tests for two bugs in the workflow backend service: | ||
|
|
||
| 1. get_service() singleton — previously returned a *new* WorkflowService on every call, | ||
| so active_tasks / workflow_tasks / cancel_events were lost between HTTP requests. | ||
|
|
||
| 2. Workflow load failure — previously the exception handler called print() and returned | ||
| without marking the task as 'failed', leaving it permanently stuck as 'running'. | ||
| """ | ||
|
|
||
| import asyncio | ||
| import importlib | ||
| import sys | ||
| import types | ||
| import unittest | ||
| from pathlib import Path | ||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Minimal stubs for heavy optional deps so the backend module imports cleanly. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def _make_stub(name: str): | ||
| mod = types.ModuleType(name) | ||
| sys.modules[name] = mod | ||
| return mod | ||
|
|
||
|
|
||
| for _name in [ | ||
| "browser_use", | ||
| "browser_use.browser", | ||
| "browser_use.browser.browser", | ||
| "browser_use.llm", | ||
| "workflow_use", | ||
| "workflow_use.controller", | ||
| "workflow_use.controller.service", | ||
| "workflow_use.workflow", | ||
| "workflow_use.workflow.service", | ||
| "aiofiles", | ||
| "yaml", | ||
| ]: | ||
| if _name not in sys.modules: | ||
| _make_stub(_name) | ||
|
|
||
| # Provide realistic-enough stub objects | ||
| sys.modules["browser_use.browser.browser"].Browser = MagicMock | ||
| sys.modules["browser_use.llm"].ChatBrowserUse = MagicMock | ||
| sys.modules["workflow_use.controller.service"].WorkflowController = MagicMock | ||
|
|
||
| _MockWorkflow = MagicMock | ||
| sys.modules["workflow_use.workflow.service"].Workflow = _MockWorkflow | ||
|
|
||
| # aiofiles stub that makes open() an async context manager | ||
| _aiofiles_mock = MagicMock() | ||
| _aiofiles_open_cm = MagicMock() | ||
| _aiofiles_file = AsyncMock() | ||
| _aiofiles_file.write = AsyncMock() | ||
| _aiofiles_file.readlines = AsyncMock(return_value=[]) | ||
| _aiofiles_file.seek = AsyncMock() | ||
| _aiofiles_open_cm.__aenter__ = AsyncMock(return_value=_aiofiles_file) | ||
| _aiofiles_open_cm.__aexit__ = AsyncMock(return_value=False) | ||
| _aiofiles_mock.open = MagicMock(return_value=_aiofiles_open_cm) | ||
| sys.modules["aiofiles"] = _aiofiles_mock | ||
|
|
||
| sys.modules["yaml"].safe_load = MagicMock(return_value={}) | ||
| sys.modules["yaml"].dump = MagicMock(return_value="") | ||
|
|
||
| # Add the backend package to sys.path so relative imports resolve | ||
| _backend_dir = Path(__file__).parent.parent | ||
| sys.path.insert(0, str(_backend_dir.parent)) | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Import the modules under test *after* stubs are in place. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| import importlib as _il | ||
|
|
||
| # Force a clean import so the module-level singleton starts as None. | ||
| if "backend.routers" in sys.modules: | ||
| del sys.modules["backend.routers"] | ||
| if "backend.service" in sys.modules: | ||
| del sys.modules["backend.service"] | ||
| if "backend.views" in sys.modules: | ||
| del sys.modules["backend.views"] | ||
|
|
||
| from backend import routers as _routers_mod # noqa: E402 | ||
| from backend.routers import get_service # noqa: E402 | ||
| from backend.service import WorkflowService # noqa: E402 | ||
| from backend.views import TaskInfo # noqa: E402 | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Helper: reset the module-level singleton between tests. | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def _reset_singleton(): | ||
| _routers_mod._service_instance = None | ||
|
|
||
|
|
||
| # =========================================================================== | ||
| # Test Suite 1 — get_service() singleton | ||
| # =========================================================================== | ||
|
|
||
| class TestGetServiceSingleton(unittest.TestCase): | ||
| """get_service() must return the same WorkflowService instance on every call.""" | ||
|
|
||
| def setUp(self): | ||
| _reset_singleton() | ||
|
|
||
| def tearDown(self): | ||
| _reset_singleton() | ||
|
|
||
| def test_returns_workflow_service_instance(self): | ||
| svc = get_service() | ||
| self.assertIsInstance(svc, WorkflowService) | ||
|
|
||
| def test_same_instance_on_repeated_calls(self): | ||
| """Every call within a request cycle must return the exact same object.""" | ||
| svc1 = get_service() | ||
| svc2 = get_service() | ||
| self.assertIs(svc1, svc2, "get_service() must return a singleton, not a new instance") | ||
|
|
||
| def test_task_state_survives_across_calls(self): | ||
| """Task state written via one call must be readable via a subsequent call.""" | ||
| svc = get_service() | ||
| svc.active_tasks["task-abc"] = TaskInfo(status="running", workflow="demo.workflow.json") | ||
|
|
||
| # Simulate a second HTTP request hitting get_service() | ||
| svc2 = get_service() | ||
| self.assertIn("task-abc", svc2.active_tasks) | ||
| self.assertEqual(svc2.active_tasks["task-abc"].status, "running") | ||
|
|
||
| def test_singleton_reset_creates_new_instance(self): | ||
| svc1 = get_service() | ||
| _reset_singleton() | ||
| svc2 = get_service() | ||
| self.assertIsNot(svc1, svc2) | ||
|
|
||
|
|
||
| # =========================================================================== | ||
| # Test Suite 2 — load failure marks task as 'failed' | ||
| # =========================================================================== | ||
|
|
||
| class TestWorkflowLoadFailureStatus(unittest.IsolatedAsyncioTestCase): | ||
| """When Workflow.load_from_file raises, active_tasks[task_id].status must be 'failed'.""" | ||
|
|
||
| def _make_service(self): | ||
| svc = WorkflowService.__new__(WorkflowService) | ||
| svc.tmp_dir = Path("/tmp/fake_wf_dir") | ||
| svc.log_dir = Path("/tmp/fake_wf_dir/logs") | ||
| svc.active_tasks = {} | ||
| svc.workflow_tasks = {} | ||
| svc.cancel_events = {} | ||
| svc.llm_instance = MagicMock() | ||
| svc.browser_instance = MagicMock() | ||
| svc.controller_instance = MagicMock() | ||
| return svc | ||
|
|
||
| async def test_load_failure_sets_status_to_failed(self): | ||
| svc = self._make_service() | ||
|
|
||
| from backend.views import WorkflowExecuteRequest | ||
|
|
||
| request = WorkflowExecuteRequest(name="bad.workflow.json", inputs={}) | ||
| task_id = "test-task-001" | ||
| cancel_event = asyncio.Event() | ||
|
|
||
| # Patch Workflow.load_from_file to raise | ||
| load_error = FileNotFoundError("workflow schema invalid") | ||
| sys.modules["workflow_use.workflow.service"].Workflow.load_from_file = MagicMock( | ||
| side_effect=load_error | ||
| ) | ||
|
|
||
| # Patch _write_log so we don't touch the filesystem | ||
| svc._write_log = AsyncMock() | ||
|
|
||
| # Patch Path.exists to return True (so the workflow_path check passes) | ||
| with patch.object(Path, "exists", return_value=True): | ||
| await svc.run_workflow_in_background(task_id, request, cancel_event) | ||
|
|
||
| task_info = svc.active_tasks.get(task_id) | ||
| self.assertIsNotNone(task_info, "active_tasks entry must exist after run") | ||
| self.assertEqual( | ||
| task_info.status, | ||
| "failed", | ||
| f"Expected status 'failed' after load error, got '{task_info.status}'", | ||
| ) | ||
| self.assertIsNotNone(task_info.error, "error field must be populated") | ||
| self.assertIn("workflow schema invalid", task_info.error) | ||
|
|
||
| async def test_load_failure_logs_error(self): | ||
| """The load error must be written to the log, not silently dropped.""" | ||
| svc = self._make_service() | ||
|
|
||
| from backend.views import WorkflowExecuteRequest | ||
|
|
||
| request = WorkflowExecuteRequest(name="bad.workflow.json", inputs={}) | ||
| task_id = "test-task-002" | ||
| cancel_event = asyncio.Event() | ||
|
|
||
| sys.modules["workflow_use.workflow.service"].Workflow.load_from_file = MagicMock( | ||
| side_effect=ValueError("corrupt yaml") | ||
| ) | ||
|
|
||
| log_calls = [] | ||
|
|
||
| async def capture_log(path, msg): | ||
| log_calls.append(msg) | ||
|
|
||
| svc._write_log = capture_log | ||
|
|
||
| with patch.object(Path, "exists", return_value=True): | ||
| await svc.run_workflow_in_background(task_id, request, cancel_event) | ||
|
|
||
| error_logged = any("corrupt yaml" in msg for msg in log_calls) | ||
| self.assertTrue(error_logged, "Load error must appear in the log output") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Module-level mocking of
aiofilesandyamlleaks stubbed modules into the shared test process and can break unrelated tests.Prompt for AI agents