diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index ca7be581..25a7d9a0 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -615,24 +615,35 @@ def logs(args, context: Optional[Context] = None) -> int: if _mode(directory) == "native": logs_dir = directory / "logs" wanted = args.services or ["api", "worker"] + # Opened before the first read and kept: a line written between printing what is there and + # starting to follow would otherwise appear in neither. + handles = {} for service in wanted: path = logs_dir / f"{service}.log" print(f"=== {path}") - if path.is_file(): - lines = path.read_text(encoding="utf-8", errors="replace").splitlines() - print("\n".join(lines[-args.tail:] if args.tail else lines)) - else: + if not path.is_file(): print("(nothing logged yet)") + continue + handle = path.open("r", encoding="utf-8", errors="replace") + lines = handle.read().splitlines() + print("\n".join(lines[-args.tail:] if args.tail else lines)) + handles[service] = handle if args.follow: - return _follow(logs_dir, wanted) + return _follow(logs_dir, wanted, handles) + for handle in handles.values(): + handle.close() return 0 options = (["--follow"] if args.follow else []) + (["--tail", str(args.tail)] if args.tail else []) return context.docker.compose(directory, "logs", *options, *args.services, check=False).returncode -def _follow(logs_dir: Path, services: list) -> int: - """Print new lines from each service's log until the terminal interrupts, prefixed by service.""" - handles: dict = {} +def _follow(logs_dir: Path, services: list, handles: Optional[dict] = None) -> int: + """Print new lines from each service's log until the terminal interrupts, prefixed by service. + + ``handles`` are the files already read, positioned where that read left off, so nothing written + in between is skipped. Files that did not exist yet are opened as they appear. + """ + handles = dict(handles or {}) try: while True: for service in services: @@ -734,7 +745,8 @@ def dev(args, context: Optional[Context] = None) -> int: "`docsgpt dev` runs the code in a source checkout, and this is an installed package. " "Clone the repository and run it from there, or use `docsgpt up --native` to run this copy." ) - if not _port_is_free(args.port): + port = _port_number(args.port, "--port") + if not _port_is_free(port): raise DeployError( f"port {args.port} is already in use, so the API cannot bind it. Stop what is on it " f"(a previous `docsgpt dev`, or `docsgpt down` for an install), or pass --port." @@ -879,7 +891,25 @@ def _check_provider(env: Mapping[str, str]) -> Check: return Check("provider", "ok", "the DocsGPT public API (no key needed)") if not (env.get("API_KEY") or env.get("OPENAI_API_KEY")): return Check("provider", "fail", f"{provider} is configured but no API_KEY is set") - return Check("provider", "ok", f"{provider}{' at ' + env['OPENAI_BASE_URL'] if env.get('OPENAI_BASE_URL') else ''}") + endpoint = _endpoint(env["OPENAI_BASE_URL"]) if env.get("OPENAI_BASE_URL") else "" + return Check("provider", "ok", f"{provider}{' at ' + endpoint if endpoint else ''}") + + +def _redis_to_check(args, env: Mapping[str, str]) -> dict: + """The Redis endpoints to ping: all three from --redis-url when given, else what .env holds. + + Overriding only the broker would still ping a stale result backend or cache, and the check + fails on the first endpoint that does not answer -- so --redis-url would appear not to work. + """ + if args.redis_url: + names = {"CELERY_BROKER_URL": "broker", "CELERY_RESULT_BACKEND": "results", "CACHE_REDIS_URL": "cache"} + return {names[key]: value for key, value in _redis_urls(args.redis_url).items()} + pairs = ( + ("broker", env.get("CELERY_BROKER_URL")), + ("results", env.get("CELERY_RESULT_BACKEND")), + ("cache", env.get("CACHE_REDIS_URL")), + ) + return {key: value for key, value in pairs if value} def doctor(args, context: Optional[Context] = None) -> int: @@ -898,13 +928,7 @@ def doctor(args, context: Optional[Context] = None) -> int: Check("settings", "ok" if env_path.is_file() else "warn", f"{env_path}" if env_path.is_file() else f"{env_path} does not exist yet; defaults are in use"), _check_postgres(args.postgres_uri or env.get("POSTGRES_URI")), - _check_redis({ - key: value for key, value in ( - ("broker", args.redis_url or env.get("CELERY_BROKER_URL")), - ("results", env.get("CELERY_RESULT_BACKEND")), - ("cache", env.get("CACHE_REDIS_URL")), - ) if value - }), + _check_redis(_redis_to_check(args, env)), _check_provider(env), ] diff --git a/docsgpt/deploy/dev.py b/docsgpt/deploy/dev.py index a552646e..18c4e485 100644 --- a/docsgpt/deploy/dev.py +++ b/docsgpt/deploy/dev.py @@ -181,6 +181,7 @@ def run( colour = out.isatty() if colour is None else colour lock = threading.Lock() running: list[tuple[Child, object]] = [] + pumps: list[threading.Thread] = [] try: for child in children: process = spawn( @@ -195,7 +196,9 @@ def run( start_new_session=os.name != "nt", ) running.append((child, process)) - threading.Thread(target=_pump, args=(child, process, out, lock, colour), daemon=True).start() + pump = threading.Thread(target=_pump, args=(child, process, out, lock, colour), daemon=True) + pump.start() + pumps.append(pump) while True: for child, process in running: @@ -214,3 +217,7 @@ def run( return 0 finally: _stop(running, grace, sleep) + # Join the readers: a child's last lines are still in flight when it exits, and dropping + # them loses exactly the output that says why it stopped. + for pump in pumps: + pump.join(timeout=grace) diff --git a/tests/deploy/test_dev.py b/tests/deploy/test_dev.py index a8344954..5426cbd6 100644 --- a/tests/deploy/test_dev.py +++ b/tests/deploy/test_dev.py @@ -123,6 +123,17 @@ class TestRun: assert "api | hello" in out.getvalue() assert code == 1, "a child that ends by itself ends the session, however it exited" + def test_output_in_flight_is_not_lost_when_run_returns(self, tmp_path): + """The reader is a thread: without joining it, a child's last lines can never be printed.""" + out = io.StringIO() + lines = [f"line {number}\n" for number in range(200)] + children = [dev.Child(name="api", command=["x"], cwd=tmp_path)] + code = dev.run(children, out=out, spawn=self._spawn([FakeProcess(lines, code=0)]), + sleep=lambda _: None, colour=False) + assert code == 1 + printed = out.getvalue() + assert "line 0" in printed and "line 199" in printed, "every line the child wrote is printed" + def test_a_failing_child_returns_its_code(self, tmp_path): out = io.StringIO() children = [dev.Child(name="api", command=["false"], cwd=tmp_path)] diff --git a/tests/deploy/test_doctor.py b/tests/deploy/test_doctor.py index 1a55ce84..252af2fa 100644 --- a/tests/deploy/test_doctor.py +++ b/tests/deploy/test_doctor.py @@ -1,5 +1,6 @@ """`docsgpt doctor`, `restart`, following native logs, and settings that apply themselves.""" +import argparse import socket import pytest @@ -49,6 +50,18 @@ class TestDevCommand: _run(["dev", "--port", str(port)], _context()) assert started == [], "nothing is spawned when the port is taken" + @pytest.mark.parametrize("port", ["0", "-1", "70000"]) + def test_a_port_argparse_accepts_but_a_socket_cannot_use(self, monkeypatch, tmp_path, port): + """argparse takes any integer: 0 would serve on an ephemeral port while printing 0.""" + from docsgpt.deploy import dev as dev_module + + self._checkout(monkeypatch, tmp_path) + started = [] + monkeypatch.setattr(dev_module, "run", lambda children, **kwargs: started.append(children) or 0) + with pytest.raises(DeployError, match="not a port number"): + _run(["dev", "--port", port], _context()) + assert started == [] + def test_a_busy_mock_llm_port_is_refused_before_anything_starts(self, monkeypatch, tmp_path): """It starts first and the rest are pointed at it, so a clash cannot wait until spawn time.""" from docsgpt.deploy import dev as dev_module @@ -184,6 +197,33 @@ class TestFollowLogs: assert "old line" not in printed, "it starts at the end, like tail -f" +class TestLogsTransition: + def test_a_line_written_between_the_read_and_the_follow_is_not_lost(self, tmp_path, capsys, monkeypatch): + """The read and the follow used to open the file twice, and whatever landed between was gone.""" + services = FakeServices() + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + logs = tmp_path / "logs" + (logs / "api.log").write_text("first\n", encoding="utf-8") + + def sleep(_): + raise KeyboardInterrupt + + original = commands._follow + + def follow(logs_dir, wanted, handles=None): + # Written after the read, before following starts. + (logs / "api.log").open("a", encoding="utf-8").write("during\n") + monkeypatch.setattr(commands.time, "sleep", sleep) + return original(logs_dir, wanted, handles) + + monkeypatch.setattr(commands, "_follow", follow) + assert _run(["logs", "-f", "api", "--dir", str(tmp_path)], _native_context(services)) == 0 + printed = capsys.readouterr().out + assert "first" in printed + assert "during" in printed, "the line written during the handover has to appear" + + class TestChecks: def test_the_public_api_needs_no_key(self): check = commands._check_provider({"LLM_PROVIDER": "docsgpt"}) @@ -201,6 +241,17 @@ class TestChecks: assert check.level == "ok" assert "8090" in check.detail + def test_a_provider_base_url_is_printed_without_its_credentials(self): + """An OpenAI-compatible endpoint can carry userinfo, and doctor prints its detail.""" + check = commands._check_provider( + {"LLM_PROVIDER": "openai", "API_KEY": "x", + "OPENAI_BASE_URL": "https://someone:sEcReTtOkEn@models.example.com/v1"} + ) + assert check.level == "ok" + assert "sEcReTtOkEn" not in check.detail + assert "someone" not in check.detail + assert "models.example.com" in check.detail + def test_services_are_named_for_the_install(self, tmp_path): names = _names(tmp_path) assert commands._chosen_services(names, []) == list(names) @@ -400,6 +451,26 @@ class TestMigrationHead: assert head[0].isdigit(), head +class TestRedisSelection: + def test_redis_url_replaces_every_endpoint(self): + """Overriding only the broker would still ping a stale backend, and one failure fails all.""" + args = argparse.Namespace(redis_url="redis://given:6379/5") + stale = { + "CELERY_BROKER_URL": "redis://old:6379/0", + "CELERY_RESULT_BACKEND": "redis://old:6379/1", + "CACHE_REDIS_URL": "redis://old:6379/2", + } + chosen = commands._redis_to_check(args, stale) + assert set(chosen) == {"broker", "results", "cache"} + assert all("old" not in url for url in chosen.values()), chosen + assert chosen["broker"].endswith("/5") and chosen["cache"].endswith("/7") + + def test_without_the_flag_the_settings_are_used(self): + args = argparse.Namespace(redis_url=None) + chosen = commands._redis_to_check(args, {"CELERY_BROKER_URL": "redis://localhost:6379/0"}) + assert chosen == {"broker": "redis://localhost:6379/0"} + + class TestDoctor: def _only(self, monkeypatch, postgres, redis): monkeypatch.setattr(commands, "_check_postgres", lambda uri: postgres)