From f76cee5f65da9a6888429124cdac1119e575087e Mon Sep 17 00:00:00 2001 From: "Younes (Machine Wisdom)" Date: Sat, 11 Jul 2026 14:15:57 -0400 Subject: [PATCH] refactor: remove tunnel doctor preflight --- README.md | 12 +-- examples/tunnel-gateway/README.md | 3 +- examples/tunnel-gateway/compose.yaml | 1 - src/fava_trails/tunnel_cli.py | 29 +------ tests/test_tunnel_cli.py | 116 ++++++--------------------- 5 files changed, 32 insertions(+), 129 deletions(-) diff --git a/README.md b/README.md index ce6faf3..325a755 100644 --- a/README.md +++ b/README.md @@ -220,16 +220,16 @@ tunnel exposure. Keep `--sync-interval-seconds` at its default `0` when no later autosync is wanted; a positive interval additionally starts the recurring worker without repeating the initial sync. -Before starting the external tunnel, deployments can validate the same private -runtime and optionally the tunnel profile without exposing it: +Before starting the external tunnel, deployments can validate the private +runtime without exposing it: ```bash -fava-trails-tunnel preflight --data-repo /path/to/fava-trails-data --profile fava-trails --tunnel-doctor +fava-trails-tunnel preflight --data-repo /path/to/fava-trails-data --profile fava-trails ``` -`preflight` starts only the loopback HTTP runtime, waits for `/healthz`, runs the -optional doctor, and always stops the private runtime before returning. It never -starts the external tunnel. +`preflight` starts only the loopback HTTP runtime, waits for `/healthz`, and +always stops the private runtime before returning. It never starts the external +tunnel. The tunnel startup wait and `status` command consume `/healthz`; `status` exits non-zero when the supervisor is running but its data is not ready: diff --git a/examples/tunnel-gateway/README.md b/examples/tunnel-gateway/README.md index 6b1dedd..2503190 100644 --- a/examples/tunnel-gateway/README.md +++ b/examples/tunnel-gateway/README.md @@ -8,8 +8,7 @@ Run a non-exposing check before the service is started: docker compose run --rm fava-tunnel \ fava-trails-tunnel preflight \ --data-repo "$FAVA_DATA_REPO" \ - --profile "$FAVA_TUNNEL_PROFILE" \ - --tunnel-doctor + --profile "$FAVA_TUNNEL_PROFILE" ``` The service performs one startup sync and sets the interval to zero, so it does diff --git a/examples/tunnel-gateway/compose.yaml b/examples/tunnel-gateway/compose.yaml index a46f751..56d7539 100644 --- a/examples/tunnel-gateway/compose.yaml +++ b/examples/tunnel-gateway/compose.yaml @@ -11,4 +11,3 @@ services: - --sync-on-start - --sync-interval-seconds - "0" - - --tunnel-doctor diff --git a/src/fava_trails/tunnel_cli.py b/src/fava_trails/tunnel_cli.py index 50ff87b..da19404 100644 --- a/src/fava_trails/tunnel_cli.py +++ b/src/fava_trails/tunnel_cli.py @@ -323,18 +323,6 @@ def _start_http_runtime( ) -def _run_tunnel_doctor(config: GatewayConfig) -> None: - result = subprocess.run( - [config.tunnel_client, "doctor", "--profile", config.profile, "--explain"], - check=False, - text=True, - ) - if result.returncode != 0: - raise subprocess.SubprocessError( - f"tunnel-client doctor failed for profile {config.profile!r}" - ) - - def _start_tunnel_client(config: GatewayConfig) -> subprocess.Popen: return subprocess.Popen( [config.tunnel_client, "run", "--profile", config.profile], @@ -678,10 +666,6 @@ def handle_shutdown(signum, frame): # noqa: ARG001 _wait_for_health(config.health_url, http_process, timeout=args.ready_timeout) print(" Private MCP runtime: ready") - if getattr(args, "tunnel_doctor", False): - _run_tunnel_doctor(config) - print(" tunnel-client doctor: ok") - tunnel_process = _start_tunnel_client(config) if state_dir: _write_metadata( @@ -719,16 +703,13 @@ def handle_shutdown(signum, frame): # noqa: ARG001 def cmd_preflight(args: argparse.Namespace) -> int: - """Validate the private runtime and optional tunnel profile without exposure.""" + """Validate the private runtime without external tunnel exposure.""" http_process: subprocess.Popen | None = None try: - run_doctor = bool(getattr(args, "tunnel_doctor", False)) - config = _load_gateway_config(args, require_tunnel_client=run_doctor) + config = _load_gateway_config(args, require_tunnel_client=False) _check_port_available(config.host, config.port) http_process = _start_http_runtime(config) _wait_for_health(config.health_url, http_process, timeout=args.ready_timeout) - if run_doctor: - _run_tunnel_doctor(config) print("FAVA Trails tunnel preflight: ok") return 0 except (OSError, ValueError, subprocess.SubprocessError, TimeoutError) as exc: @@ -779,8 +760,6 @@ def cmd_start(args: argparse.Namespace) -> int: "--state-dir", str(state_dir), ] - if getattr(args, "tunnel_doctor", False): - command.append("--tunnel-doctor") if getattr(args, "sync_on_start", False): command.append("--sync-on-start") command.extend([ @@ -938,7 +917,6 @@ def build_parser() -> argparse.ArgumentParser: _add_common_args(p_run) p_run.add_argument("--tunnel-client", default="tunnel-client", help="Path to tunnel-client binary") p_run.add_argument("--ready-timeout", type=float, default=20.0, help="Seconds to wait for local MCP readiness") - p_run.add_argument("--tunnel-doctor", action="store_true", help="Run tunnel-client doctor before starting the tunnel") p_run.add_argument("--sync-on-start", action="store_true", help="Require one successful data repo sync before exposing the gateway") p_run.add_argument("--sync-interval-seconds", type=float, default=DEFAULT_SYNC_INTERVAL_SECONDS, help=f"Seconds between data repo syncs; 0 disables tunnel-managed sync (default: {DEFAULT_SYNC_INTERVAL_SECONDS:g})") p_run.add_argument("--sync-timeout-seconds", type=float, default=DEFAULT_SYNC_TIMEOUT_SECONDS, help=f"Seconds before a data repo sync is marked failed (default: {DEFAULT_SYNC_TIMEOUT_SECONDS:g})") @@ -949,7 +927,6 @@ def build_parser() -> argparse.ArgumentParser: _add_common_args(p_start) p_start.add_argument("--tunnel-client", default="tunnel-client", help="Path to tunnel-client binary") p_start.add_argument("--ready-timeout", type=float, default=20.0, help="Seconds to wait for local MCP readiness") - p_start.add_argument("--tunnel-doctor", action="store_true", help="Run tunnel-client doctor before starting the tunnel") p_start.add_argument("--sync-on-start", action="store_true", help="Require one successful data repo sync before exposing the gateway") p_start.add_argument("--sync-interval-seconds", type=float, default=DEFAULT_SYNC_INTERVAL_SECONDS, help=f"Seconds between data repo syncs; 0 disables tunnel-managed sync (default: {DEFAULT_SYNC_INTERVAL_SECONDS:g})") p_start.add_argument("--sync-timeout-seconds", type=float, default=DEFAULT_SYNC_TIMEOUT_SECONDS, help=f"Seconds before a data repo sync is marked failed (default: {DEFAULT_SYNC_TIMEOUT_SECONDS:g})") @@ -957,9 +934,7 @@ def build_parser() -> argparse.ArgumentParser: p_preflight = subparsers.add_parser("preflight", help="Validate the private runtime without starting the external tunnel") _add_common_args(p_preflight) - p_preflight.add_argument("--tunnel-client", default="tunnel-client", help="Path to tunnel-client binary") p_preflight.add_argument("--ready-timeout", type=float, default=20.0, help="Seconds to wait for local MCP readiness") - p_preflight.add_argument("--tunnel-doctor", action="store_true", help="Run tunnel-client doctor after private readiness succeeds") p_preflight.set_defaults(func=cmd_preflight) p_stop = subparsers.add_parser("stop", help="Stop a detached gateway") diff --git a/tests/test_tunnel_cli.py b/tests/test_tunnel_cli.py index 9eb17ce..311e8cb 100644 --- a/tests/test_tunnel_cli.py +++ b/tests/test_tunnel_cli.py @@ -4,7 +4,6 @@ import argparse import json -import subprocess from pathlib import Path from unittest.mock import MagicMock, patch @@ -40,7 +39,6 @@ def _args(**overrides): "mcp_path": DEFAULT_MCP_PATH, "tunnel_client": "tunnel-client", "ready_timeout": 0.1, - "tunnel_doctor": False, "sync_on_start": False, "state_dir": None, "sync_interval_seconds": 0.0, @@ -217,7 +215,7 @@ def test_disabled_sync_health_does_not_query_revision_state(tmp_path, monkeypatc assert "remote_main" not in payload -def test_run_starts_http_and_runs_tunnel_without_doctor_or_autosync_by_default(tmp_path, monkeypatch): +def test_run_starts_http_and_runs_tunnel_without_autosync_by_default(tmp_path, monkeypatch): data_repo = _make_data_repo(tmp_path) monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") args = _args(data_repo=str(data_repo)) @@ -236,14 +234,12 @@ def test_run_starts_http_and_runs_tunnel_without_doctor_or_autosync_by_default(t with patch("fava_trails.tunnel_cli._start_http_runtime", return_value=http_process) as start_http: with patch("fava_trails.tunnel_cli._wait_for_health") as wait_health: with patch("fava_trails.tunnel_cli._sync_data_repo", return_value={"status": "ok"}) as sync: - with patch("subprocess.run") as run: - with patch("fava_trails.tunnel_cli._start_tunnel_client", return_value=tunnel_process) as start_tunnel: - rc = cmd_run(args) + with patch("fava_trails.tunnel_cli._start_tunnel_client", return_value=tunnel_process) as start_tunnel: + rc = cmd_run(args) assert rc == 0 start_http.assert_called_once() wait_health.assert_called_once() - run.assert_not_called() start_tunnel.assert_called_once() sync.assert_not_called() @@ -363,33 +359,6 @@ def test_run_does_not_expose_when_startup_sync_result_is_unstructured(tmp_path, start_tunnel.assert_not_called() -def test_run_checks_doctor_when_requested(tmp_path, monkeypatch): - data_repo = _make_data_repo(tmp_path) - monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") - args = _args(data_repo=str(data_repo), tunnel_doctor=True) - - http_process = MagicMock() - http_process.poll.return_value = None - http_process.pid = 111 - tunnel_process = MagicMock() - tunnel_process.wait.return_value = 0 - tunnel_process.poll.return_value = 0 - tunnel_process.pid = 222 - - with patch("fava_trails.tunnel_cli._find_jj_bin", return_value="/usr/bin/jj"): - with patch("shutil.which", return_value="/usr/bin/tunnel-client"): - with patch("fava_trails.tunnel_cli._check_port_available"): - with patch("fava_trails.tunnel_cli._start_http_runtime", return_value=http_process): - with patch("fava_trails.tunnel_cli._wait_for_health"): - with patch("fava_trails.tunnel_cli._sync_data_repo", return_value={"status": "ok"}): - with patch("subprocess.run", return_value=MagicMock(returncode=0)) as run: - with patch("fava_trails.tunnel_cli._start_tunnel_client", return_value=tunnel_process): - rc = cmd_run(args) - - assert rc == 0 - run.assert_called_once() - - def test_start_writes_state_and_pid(tmp_path, monkeypatch, capsys): data_repo = _make_data_repo(tmp_path) monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") @@ -423,33 +392,6 @@ def fake_popen(*args, **kwargs): assert "http://127.0.0.1:8765/mcp/" in (state_dirs[0] / "metadata.json").read_text() -def test_start_passes_tunnel_doctor_to_supervisor_when_requested(tmp_path, monkeypatch): - data_repo = _make_data_repo(tmp_path) - monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") - monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "state")) - args = _args(data_repo=str(data_repo), tunnel_doctor=True) - - process = MagicMock() - process.pid = 4321 - process.poll.return_value = None - - state_dir = tunnel_cli._state_dir(data_repo.resolve(), DEFAULT_PROFILE) - - def fake_popen(command, *args, **kwargs): - state_dir.mkdir(parents=True, exist_ok=True) - tunnel_cli._ready_file(state_dir).write_text('{"status":"ready"}\n') - assert "--tunnel-doctor" in command - return process - - with patch("fava_trails.tunnel_cli._find_jj_bin", return_value="/usr/bin/jj"): - with patch("shutil.which", return_value="/usr/bin/tunnel-client"): - with patch("fava_trails.tunnel_cli._check_port_available"): - with patch("subprocess.Popen", side_effect=fake_popen): - rc = cmd_start(args) - - assert rc == 0 - - def test_start_forwards_sync_on_start_to_supervisor(tmp_path, monkeypatch): data_repo = _make_data_repo(tmp_path) monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") @@ -752,50 +694,38 @@ def test_tunnel_cli_help_mentions_start(): assert "preflight" in help_text -def test_preflight_starts_only_private_runtime_then_cleans_up(tmp_path, monkeypatch): +@pytest.mark.parametrize( + ("command", "flag"), + [ + ("run", "--tunnel-doctor"), + ("start", "--tunnel-doctor"), + ("preflight", "--tunnel-doctor"), + ("preflight", "--tunnel-client"), + ], +) +def test_tunnel_cli_rejects_removed_tunnel_doctor_flags(command, flag): + with pytest.raises(SystemExit): + tunnel_cli.build_parser().parse_args([command, flag]) + + +def test_preflight_starts_only_private_runtime_without_tunnel_client(tmp_path, monkeypatch): data_repo = _make_data_repo(tmp_path) monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") - args = _args(data_repo=str(data_repo), tunnel_doctor=True) + args = _args(data_repo=str(data_repo)) http_process = MagicMock(pid=111) http_process.poll.return_value = None with patch("fava_trails.tunnel_cli._find_jj_bin", return_value="/usr/bin/jj"): - with patch("shutil.which", return_value="/usr/bin/tunnel-client"): + with patch("shutil.which", return_value=None): with patch("fava_trails.tunnel_cli._check_port_available"): with patch("fava_trails.tunnel_cli._start_http_runtime", return_value=http_process) as start_http: with patch("fava_trails.tunnel_cli._wait_for_health") as wait_health: - with patch("fava_trails.tunnel_cli._run_tunnel_doctor") as doctor: - with patch("fava_trails.tunnel_cli._start_tunnel_client") as start_tunnel: - with patch("fava_trails.tunnel_cli._terminate_process") as terminate: - assert cmd_preflight(args) == 0 + with patch("fava_trails.tunnel_cli._start_tunnel_client") as start_tunnel: + with patch("fava_trails.tunnel_cli._terminate_process") as terminate: + assert cmd_preflight(args) == 0 start_http.assert_called_once() wait_health.assert_called_once() - doctor.assert_called_once() - start_tunnel.assert_not_called() - terminate.assert_called_once_with(http_process) - - -def test_preflight_cleans_private_runtime_when_doctor_fails(tmp_path, monkeypatch): - data_repo = _make_data_repo(tmp_path) - monkeypatch.setenv("OPENROUTER_API_KEY", "test-key") - args = _args(data_repo=str(data_repo), tunnel_doctor=True) - http_process = MagicMock(pid=111) - http_process.poll.return_value = None - - with patch("fava_trails.tunnel_cli._find_jj_bin", return_value="/usr/bin/jj"): - with patch("shutil.which", return_value="/usr/bin/tunnel-client"): - with patch("fava_trails.tunnel_cli._check_port_available"): - with patch("fava_trails.tunnel_cli._start_http_runtime", return_value=http_process): - with patch("fava_trails.tunnel_cli._wait_for_health"): - with patch( - "fava_trails.tunnel_cli._run_tunnel_doctor", - side_effect=subprocess.SubprocessError("doctor failed"), - ): - with patch("fava_trails.tunnel_cli._start_tunnel_client") as start_tunnel: - with patch("fava_trails.tunnel_cli._terminate_process") as terminate: - assert cmd_preflight(args) == 1 - start_tunnel.assert_not_called() terminate.assert_called_once_with(http_process)