diff --git a/README.md b/README.md index 43d6b1cdc0..d3f303f383 100644 --- a/README.md +++ b/README.md @@ -2039,7 +2039,7 @@ uv sync --extra browser uv run playwright install chromium ``` -Then uncomment the `group: browser` tool entries in `config.yaml` (`browser_navigate`, `browser_snapshot`, `browser_click`, `browser_type`, `browser_get_text`, `browser_back`, `browser_screenshot`, `browser_close`). `make dev` / Docker startup detects an enabled `browser_navigate` tool and preserves the `browser` extra on dependency syncs. The Gateway fails startup if browser control is configured but Playwright is missing, and `/api/features` hides the Browser UI unless the backend can actually serve it. Keep `headless: true` and `allow_private_addresses: false` for anything but local, trusted debugging. Attaching to an existing Chrome with `cdp_url` cannot enforce DeerFlow's subresource/redirect SSRF guard and therefore fails closed unless `allow_unguarded_cdp: true` explicitly acknowledges that risk; use it only with a trusted local browser. Browser sessions are process-local; keep `GATEWAY_WORKERS=1` while this tool group is enabled because ordinary uvicorn worker dispatch does not provide thread affinity. +Then uncomment the `group: browser` tool entries in `config.yaml` (`browser_navigate`, `browser_snapshot`, `browser_click`, `browser_type`, `browser_get_text`, `browser_back`, `browser_screenshot`, `browser_close`). `make dev` / Docker startup detects an enabled `browser_navigate` tool and preserves the `browser` extra on dependency syncs. The Gateway fails startup if browser control is configured but Playwright is missing, and `/api/features` hides the Browser UI unless the backend can actually serve it. Keep `headless: true` and `allow_private_addresses: false` for anything but local, trusted debugging. Attaching to an existing Chrome with `cdp_url` cannot enforce DeerFlow's subresource/redirect SSRF guard and therefore fails closed unless `allow_unguarded_cdp: true` explicitly acknowledges that risk; use it only with a trusted local browser. Browser sessions are process-local; keep the Gateway to a single worker process while this tool group is enabled because ordinary uvicorn worker dispatch does not provide thread affinity. That means `GATEWAY_WORKERS=1`, and on the launches that pass uvicorn no worker count at all (`backend/Dockerfile`, `scripts/serve.sh`) also `WEB_CONCURRENCY` unset or `1`, since uvicorn takes the process count from it. Existing, non-mock Custom Agent chats expose the same Browser Live controls when browser control is available and the agent either leaves `tool_groups` unrestricted or includes the `browser` group. An explicit tool-group allowlist without `browser` keeps those controls hidden. diff --git a/backend/app/channels/dedupe_store.py b/backend/app/channels/dedupe_store.py index f7525fbd82..effb949816 100644 --- a/backend/app/channels/dedupe_store.py +++ b/backend/app/channels/dedupe_store.py @@ -198,12 +198,38 @@ class PostgresInboundDedupeStore: logger.exception("PostgresInboundDedupeStore.release failed; key left for TTL expiry (fail-open)") -def _gateway_workers() -> int: - """Mirror deps._enforce_postgres_for_multi_worker's worker detection.""" - try: - return int(os.environ.get("GATEWAY_WORKERS", "1") or 1) - except (TypeError, ValueError): - return 1 +# The gateway worker count has two spellings: ``GATEWAY_WORKERS``, which +# docker-compose forwards as ``--workers``, and ``WEB_CONCURRENCY``, which uvicorn reads +# when no count is passed at all (``backend/Dockerfile``, ``scripts/serve.sh``). Read +# both, otherwise a multi-worker pod that shares no dedupe state reports itself as +# single-worker. +_WORKER_COUNT_ENV_VARS = ("GATEWAY_WORKERS", "WEB_CONCURRENCY") + + +def _gateway_workers() -> tuple[int, str]: + """Resolve the Gateway worker count the same way the startup gates do. + + Returns the count and the name of the variable that set it, so a warning can name + the knob the operator actually has to change. A blank value means "unset". + + This is a second reader of the same two spellings, not an import of + ``app/gateway/deps.py``'s helper, so the two can drift; and it differs on purpose + from ``app/gateway/routers/channel_connections.py``, which turns an unparsable value + into 0 and refuses, while here an unparsable value is skipped and only a resolved + count above 1 warns. + """ + for name in _WORKER_COUNT_ENV_VARS: + raw = os.environ.get(name) + if not raw or not raw.strip(): + continue + try: + return int(raw), name + except (TypeError, ValueError): + # Do not let an unparsable ``GATEWAY_WORKERS`` mask a real count in + # ``WEB_CONCURRENCY``: on the launchers that pass no ``--workers`` the + # former never reaches uvicorn, so the latter is what starts the processes. + continue + return 1, _WORKER_COUNT_ENV_VARS[0] def _build_postgres_store() -> InboundDedupeStore: @@ -242,7 +268,8 @@ def make_inbound_dedupe_store(app_config: Any | None = None) -> InboundDedupeSto db_backend = getattr(db, "backend", None) db_is_postgres = db_backend == "postgres" - multi_worker = _gateway_workers() > 1 + workers, worker_env = _gateway_workers() + multi_worker = workers > 1 if backend == "postgres": if not db_is_postgres: @@ -255,7 +282,7 @@ def make_inbound_dedupe_store(app_config: Any | None = None) -> InboundDedupeSto if backend == "memory": if multi_worker: logger.warning( - "dedupe_storage=memory with GATEWAY_WORKERS>1: inbound webhook dedupe " + f"dedupe_storage=memory with {worker_env}>1: inbound webhook dedupe " "is per-pod and will NOT drop redeliveries routed to a different replica. " "Use dedupe_storage=postgres (or remove the setting to let 'auto' pick it) " "for multi-worker deployments. See issue #4120." diff --git a/backend/docs/CONFIGURATION.md b/backend/docs/CONFIGURATION.md index ae1fa4ad6c..0c89eb0e3d 100644 --- a/backend/docs/CONFIGURATION.md +++ b/backend/docs/CONFIGURATION.md @@ -516,7 +516,7 @@ Notes: - **Upgrade note:** before upgrading a deployment with `GATEWAY_WORKERS > 1` and `scheduler.enabled: true`, either run the scheduler on exactly one Gateway worker or enable `scheduler.multi_instance: true` with shared Postgres, `run_ownership.heartbeat_enabled: true`, and `run_events.backend: db`. The startup gate now rejects the unsafe combination instead of allowing it to start silently. - **Upgrade note:** in multi-instance mode, `max_concurrent_runs` is cluster-wide rather than per Pod and counts `launching`/`running` occurrences. Waiting `queued` rows remain outside the execution cap; capacity does not multiply with the replica count. - **Upgrade note:** `scheduler.multi_instance` and its related scheduler, ownership, and run-event settings are startup-only. Restart all Gateway Pods together after changing them; a ConfigMap update without a coordinated restart leaves the running service on its previous mode. -- Multi-worker deployments (`GATEWAY_WORKERS > 1`) must use the Postgres database backend, enable run ownership heartbeats, and set `run_events.backend: db`. SQLite silently ignores row-level locks, while memory and JSONL run-event stores are process-local and cannot enforce singleton delivery receipts across workers; startup rejects these combinations. The process-local agentic browser tool group is incompatible with multiple Gateway workers; keep `GATEWAY_WORKERS=1` while `browser_navigate` is enabled. Browser control also requires the backend `browser` extra (`cd backend && uv sync --extra browser && uv run playwright install chromium`); startup detects enabled browser config and fails fast when Playwright is missing, and `/api/features` reports `browser_control.enabled=false` until the runtime is available. +- Multi-worker deployments (`GATEWAY_WORKERS > 1`) must use the Postgres database backend, enable run ownership heartbeats, and set `run_events.backend: db`. SQLite silently ignores row-level locks, while memory and JSONL run-event stores are process-local and cannot enforce singleton delivery receipts across workers; startup rejects these combinations. The process-local agentic browser tool group is incompatible with multiple Gateway workers; keep the Gateway to one worker process while `browser_navigate` is enabled — `GATEWAY_WORKERS=1`, plus `WEB_CONCURRENCY` unset or `1` on launches that pass uvicorn no worker count (`backend/Dockerfile`, `scripts/serve.sh`), where uvicorn takes the process count from it. Browser control also requires the backend `browser` extra (`cd backend && uv sync --extra browser && uv run playwright install chromium`); startup detects enabled browser config and fails fast when Playwright is missing, and `/api/features` reports `browser_control.enabled=false` until the runtime is available. - The MVP supports thread reuse and fresh-thread-per-run execution modes. - Create/update accept optional `assistant_id` (`lead_agent` by default, or an existing custom agent for the task owner). - Create/update accept `once`, `cron`, and `interval`. Interval uses `schedule_spec.every_seconds` (UTC `now + N`, no missed-beat catch-up). N is at least `min_once_delay_seconds` (default 60) and at most 30 days. diff --git a/backend/packages/harness/deerflow/community/browser_automation/session.py b/backend/packages/harness/deerflow/community/browser_automation/session.py index 2db3bc9e2e..d1a6d061ad 100644 --- a/backend/packages/harness/deerflow/community/browser_automation/session.py +++ b/backend/packages/harness/deerflow/community/browser_automation/session.py @@ -143,16 +143,49 @@ _DEFAULT_MAX_SESSIONS = 32 _DEFAULT_IDLE_TIMEOUT_S = 30 * 60.0 -def browser_multi_worker_error(workers: int | None = None) -> str | None: - """Return the fail-closed reason for process-local browser sessions.""" - if workers is None: +# ``GATEWAY_WORKERS`` is the documented knob and the one docker-compose forwards as +# ``--workers``; ``backend/Dockerfile`` and ``scripts/serve.sh`` start uvicorn with no +# worker count at all, and uvicorn then takes the count from ``WEB_CONCURRENCY``. Both +# must be read, or this gate stays inert in exactly the deployment it exists to refuse. +# ``app/gateway/routers/channel_connections.py`` reads the same two spellings and treats a +# blank value as unset the same way, but deliberately differs on unparsable input: that +# route resolves it to 0 and refuses, while this one falls through to the next spelling +# and stays inert, because uvicorn rejects ``--workers abc`` itself before serving. +_WORKER_COUNT_ENV_VARS = ("GATEWAY_WORKERS", "WEB_CONCURRENCY") + + +def _worker_count_from_env() -> tuple[int, str]: + """Resolve the worker count and the variable that set it; a blank value is unset.""" + for name in _WORKER_COUNT_ENV_VARS: + raw = os.environ.get(name) + if not raw or not raw.strip(): + continue try: - workers = int(os.environ.get("GATEWAY_WORKERS", "1")) + return int(raw), name except (TypeError, ValueError): - workers = 1 + # Unparsable here must not hide a valid count in the other spelling: an + # exported ``GATEWAY_WORKERS=abc`` never reaches uvicorn on the launchers + # that pass no ``--workers``, so ``WEB_CONCURRENCY`` is still what decides + # how many processes start. + continue + return 1, _WORKER_COUNT_ENV_VARS[0] + + +def browser_multi_worker_error(workers: int | None = None, env_name: str | None = None) -> str | None: + """Return the fail-closed reason for process-local browser sessions. + + ``env_name`` is the spelling the caller resolved the count from; the refusal has to + name it, because an operator who only ever set ``WEB_CONCURRENCY`` cannot act on a + message about ``GATEWAY_WORKERS``. Without it the name is resolved from the + environment, which is the same source the count came from. + """ + if workers is None: + workers, env_name = _worker_count_from_env() + elif env_name is None: + env_name = _worker_count_from_env()[1] if workers <= 1: return None - return f"GATEWAY_WORKERS={workers} cannot enable agentic browser tools: browser sessions are process-local and uvicorn does not provide thread affinity. Set GATEWAY_WORKERS=1 or disable the browser_navigate tool." + return f"{env_name}={workers} cannot enable agentic browser tools: browser sessions are process-local and uvicorn does not provide thread affinity. Set {env_name}=1 or disable the browser_navigate tool." def ensure_browser_worker_compatibility() -> None: diff --git a/backend/tests/test_browser_automation.py b/backend/tests/test_browser_automation.py index 147c2b5bb3..66a2868413 100644 --- a/backend/tests/test_browser_automation.py +++ b/backend/tests/test_browser_automation.py @@ -26,9 +26,22 @@ from deerflow.community.browser_automation.session import ( BrowserSessionManager, PageSnapshot, SnapshotElement, + browser_multi_worker_error, ) +@pytest.fixture(autouse=True) +def isolated_worker_env(monkeypatch): + """Keep the suite independent of the invoking shell's worker count. + + ``WEB_CONCURRENCY`` is a common ambient convention on uvicorn/gunicorn/PaaS hosts, + and either spelling now decides whether process-local browser sessions are refused. + The tests that exercise the refusal set the spelling they mean to exercise. + """ + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.delenv("WEB_CONCURRENCY", raising=False) + + @pytest.fixture def public_dns(): """Keep mocked tool calls offline while exercising the real URL safety check.""" @@ -882,11 +895,22 @@ class TestSessionManager: async def test_get_session_rejects_runtime_multi_worker_browser_use(self, monkeypatch): monkeypatch.setenv("GATEWAY_WORKERS", "2") + monkeypatch.delenv("WEB_CONCURRENCY", raising=False) manager = BrowserSessionManager() with pytest.raises(RuntimeError, match="process-local"): manager.get_session("thread-a") + async def test_get_session_rejects_worker_count_from_the_uvicorn_fallback(self, monkeypatch): + # backend/Dockerfile and scripts/serve.sh start uvicorn with no --workers, so + # WEB_CONCURRENCY is what really decides the process count here too. + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.setenv("WEB_CONCURRENCY", "2") + manager = BrowserSessionManager() + + with pytest.raises(RuntimeError, match=r"WEB_CONCURRENCY=2 cannot enable agentic browser tools"): + manager.get_session("thread-a") + async def test_cdp_requires_explicit_unguarded_trust_opt_in(self): manager = BrowserSessionManager() @@ -948,6 +972,58 @@ class TestSessionManager: assert session.active_refs == 0 +@pytest.mark.parametrize( + ("gateway_workers", "web_concurrency", "expected"), + [ + pytest.param(None, "4", 4, id="uvicorn-fallback-counts"), + pytest.param("2", None, 2, id="documented-knob-counts"), + pytest.param("1", "4", 1, id="documented-knob-wins-when-both-set"), + pytest.param(" ", "3", 3, id="blank-is-unset"), + pytest.param("abc", "4", 4, id="unparsable-knob-does-not-mask-the-fallback"), + pytest.param(None, None, 1, id="nothing-set"), + pytest.param(None, "auto", 1, id="uvicorn_rejects_it_so_stay_inert"), + ], +) +def test_worker_count_resolution_reads_both_spellings(monkeypatch, gateway_workers, web_concurrency, expected): + for name, value in (("GATEWAY_WORKERS", gateway_workers), ("WEB_CONCURRENCY", web_concurrency)): + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, value) + + assert session_mod._worker_count_from_env()[0] == expected + + +def test_worker_count_error_names_the_variable_that_set_the_count(monkeypatch): + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.setenv("WEB_CONCURRENCY", "4") + + error = browser_multi_worker_error() + + assert error is not None + assert error.startswith("WEB_CONCURRENCY=4 ") + assert "Set WEB_CONCURRENCY=1" in error + + +def test_worker_count_error_names_the_fallback_for_a_passed_in_count(monkeypatch): + # The Gateway startup gate resolves the count and calls this with the number only; + # the refusal still has to name the knob the operator set, or its advice would + # silence the gate while uvicorn keeps starting the extra processes. + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.setenv("WEB_CONCURRENCY", "4") + + assert browser_multi_worker_error(4).startswith("WEB_CONCURRENCY=4 ") + + +def test_worker_count_error_keeps_naming_the_documented_knob_when_passed_in(monkeypatch): + # With neither spelling in the environment there is nothing to attribute the count + # to, so a passed-in number keeps naming GATEWAY_WORKERS as it always has. + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.delenv("WEB_CONCURRENCY", raising=False) + + assert browser_multi_worker_error(3).startswith("GATEWAY_WORKERS=3 ") + + def test_resolve_session_always_reads_browser_navigate_config(): """Launch config is deterministic regardless of which tool runs first. diff --git a/backend/tests/test_features_router.py b/backend/tests/test_features_router.py index 70a33d0e02..5b870ea412 100644 --- a/backend/tests/test_features_router.py +++ b/backend/tests/test_features_router.py @@ -9,6 +9,19 @@ from app.gateway.deps import get_config from app.gateway.routers import features +@pytest.fixture(autouse=True) +def isolated_worker_env(monkeypatch): + """Keep the feature surface independent of the invoking shell's worker count. + + ``browser_capability()`` consults the Gateway worker count, which uvicorn takes + from ``WEB_CONCURRENCY`` on the launches that pass no ``--workers``. An exported + value there would otherwise make the browser-control assertions below report a + refusal the code under test did not produce. + """ + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.delenv("WEB_CONCURRENCY", raising=False) + + def _app_with_config( *, agents_api_enabled: bool, diff --git a/backend/tests/test_inbound_dedupe.py b/backend/tests/test_inbound_dedupe.py index fa662d6de3..47a8098f63 100644 --- a/backend/tests/test_inbound_dedupe.py +++ b/backend/tests/test_inbound_dedupe.py @@ -18,6 +18,18 @@ from app.channels.dedupe_store import ( ) +@pytest.fixture(autouse=True) +def isolated_worker_env(monkeypatch): + """Keep config-resolution tests independent of the invoking shell. + + ``WEB_CONCURRENCY`` is a common ambient convention on uvicorn/gunicorn/PaaS hosts, + and it now decides whether a deployment counts as multi-worker here too. The tests + that exercise that path set the spelling they mean to exercise. + """ + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.delenv("WEB_CONCURRENCY", raising=False) + + class _FakeDedupe: def __init__(self, backend: str) -> None: self.backend = backend # plain string, mimics DedupeStorageBackend.value @@ -106,6 +118,60 @@ def test_factory_warns_when_auto_resolves_memory_under_multi_worker(monkeypatch, assert any("Multi-worker deployment detected but dedupe_storage=auto" in r.message for r in caplog.records) +def test_factory_warns_under_the_uvicorn_worker_fallback(monkeypatch, caplog): + import logging + + # backend/Dockerfile and scripts/serve.sh start uvicorn with no --workers, so + # WEB_CONCURRENCY is what decides how many processes share no dedupe state. + monkeypatch.delenv("GATEWAY_WORKERS", raising=False) + monkeypatch.setenv("WEB_CONCURRENCY", "2") + app = _FakeApp("memory", "sqlite") + with caplog.at_level(logging.WARNING): + store = make_inbound_dedupe_store(app) + assert isinstance(store, MemoryInboundDedupeStore) + assert any("dedupe_storage=memory with WEB_CONCURRENCY>1" in r.message for r in caplog.records) + + +def test_factory_warns_when_an_unparsable_knob_hides_the_fallback_count(monkeypatch, caplog): + import logging + + # GATEWAY_WORKERS never reaches uvicorn on the launchers that pass no --workers, so + # an unusable value there must not report the deployment as single-worker while + # WEB_CONCURRENCY still starts the processes. + monkeypatch.setenv("GATEWAY_WORKERS", "abc") + monkeypatch.setenv("WEB_CONCURRENCY", "4") + app = _FakeApp("memory", "sqlite") + with caplog.at_level(logging.WARNING): + make_inbound_dedupe_store(app) + assert any("dedupe_storage=memory with WEB_CONCURRENCY>1" in r.message for r in caplog.records) + + +def test_factory_prefers_the_documented_knob_when_both_are_set(monkeypatch, caplog): + import logging + + # docker-compose forwards GATEWAY_WORKERS as --workers, which outranks the + # WEB_CONCURRENCY uvicorn would otherwise read. + monkeypatch.setenv("GATEWAY_WORKERS", "1") + monkeypatch.setenv("WEB_CONCURRENCY", "4") + app = _FakeApp("memory", "sqlite") + with caplog.at_level(logging.WARNING): + make_inbound_dedupe_store(app) + assert not any("dedupe_storage=memory with" in r.message for r in caplog.records) + + +def test_factory_treats_a_blank_gateway_workers_as_unset(monkeypatch, caplog): + import logging + + # The compose command spells the fallback ${GATEWAY_WORKERS:-1}, so an exported + # empty value means "unset" rather than "no workers". + monkeypatch.setenv("GATEWAY_WORKERS", " ") + monkeypatch.setenv("WEB_CONCURRENCY", "3") + app = _FakeApp("auto", "sqlite") + with caplog.at_level(logging.WARNING): + make_inbound_dedupe_store(app) + assert any("Multi-worker deployment detected" in r.message for r in caplog.records) + + # --------------------------------------------------------------------------- # PostgresInboundDedupeStore (unit, mocked session factory) # --------------------------------------------------------------------------- diff --git a/config.example.yaml b/config.example.yaml index 9ecb84246c..2cd87ad194 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -1129,7 +1129,9 @@ tools: # # # CDP handshake. If so, use headless: false to launch # # # Playwright's own visible browser instead. # # Multi-worker note: browser sessions live in one Gateway worker's memory. - # # Keep GATEWAY_WORKERS=1 while this tool group is enabled. + # # Keep the Gateway to one worker process while this tool group is enabled: + # # GATEWAY_WORKERS=1, and WEB_CONCURRENCY unset or 1 on the launches that pass + # # uvicorn no --workers (backend/Dockerfile, scripts/serve.sh). # - name: browser_snapshot # group: browser # use: deerflow.community.browser_automation.tools:browser_snapshot_tool diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 8d2dd6b55f..c0e0341699 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -306,10 +306,12 @@ Memory backend async boundary: CI runs these regression tests for every pull request via [.github/workflows/backend-unit-tests.yml](../.github/workflows/backend-unit-tests.yml). -Agentic browser sessions are process-local. The Gateway startup safety gate rejects -`GATEWAY_WORKERS > 1` when `browser_navigate` is configured, because ordinary -uvicorn worker dispatch does not provide thread affinity for browser tools, REST -navigation, and the Live WebSocket. +Agentic browser sessions are process-local. Browser use is refused when the Gateway runs +more than one worker process, because ordinary uvicorn worker dispatch does not provide +thread affinity for browser tools, REST navigation, and the Live WebSocket. Keep +`GATEWAY_WORKERS=1`, and on the launchers that pass uvicorn no `--workers` +(`scripts/serve.sh`, `backend/Dockerfile`) keep `WEB_CONCURRENCY` unset or `1` too — that +is where uvicorn takes the process count from. Browser Live screenshots remain JPEG bytes inside the harness and the Gateway's bounded, drop-oldest frame queue. WebSocket clients that request