diff --git a/keel/commands/open_console.py b/keel/commands/open_console.py index 539f2ab..395ebe0 100644 --- a/keel/commands/open_console.py +++ b/keel/commands/open_console.py @@ -19,6 +19,7 @@ from __future__ import annotations import webbrowser +from typing import NoReturn import click @@ -41,11 +42,15 @@ help="Print the address without launching a browser.", ) def open_cmd(port: int, no_browser: bool) -> None: - """Print (and open) the address of a running `keel serve`, token included.""" + """Print (and open) the address of a running `keel serve`, token included. + + The deployment is the one the CURRENT DIRECTORY belongs to (or `KEEL_HOME`), the same way + every other bare invocation resolves state -- so run this from the deployment, not from a + checkout of the source. + """ record = runtime.live_record(port) if record is None: _refuse(port) - return url = runtime.url_for(record) click.echo(f"Opening the keel console at:\n\n {url}\n") @@ -62,25 +67,56 @@ def open_cmd(port: int, no_browser: bool) -> None: click.echo(f"(could not launch a browser: {exc})") -def _refuse(port: int) -> None: +def _refuse(port: int) -> NoReturn: """Say which of the three absences this is, and what to do about each. A stale record is reported as a STOPPED server rather than as no server: the file is evidence that one ran, and telling an operator "nothing is serving" when a crashed process left its record behind hides the thing they most need to know. """ + # WHERE IT LOOKED, in every branch. The first real install of the console daemons succeeded + # and read as a failure: `keel open` was run from a source checkout, which is itself a + # deployment root (it has a `keel.db` and a `config.yaml`), so it searched that tree while + # four healthy servers wrote to `~/keel/run/`. The message listed three explanations and the + # true one was not among them -- it could not be, because it never said where it had looked. + # + # This command takes no `--db` and no `--config`, so nothing in its signature hints that the + # answer depends on the working directory. The path is the hint. + searched = runtime.run_dir() + stale = runtime.read_record(port) if stale is not None: + # WHICH of the two liveness checks failed. `live_record` requires a live pid AND something + # answering on the port, and this branch used to assert the first had failed regardless -- + # measured saying "its process is gone" about the very process printing the sentence. A + # server that is alive but not yet listening, or bound to a host other than the one + # recorded, is a bind problem, and telling its operator it crashed sends them to the wrong + # investigation. This codebase refuses that kind of confident wrong claim elsewhere + # (`_session_banner` renders nothing rather than name a mode it cannot verify). + pid = runtime.recorded_pid(stale) + if runtime.process_alive(pid): + raise click.ClickException( + f"a keel server was recorded on port {port} and its process ({pid}) is still " + f"running, but nothing answers on {stale.get('host', '?')}:{port}.\n\n" + " That is a server that failed to bind, or one still starting. Its log says " + f"which: `Address already in use` is the common one.\n\n" + f" The record is in {searched}." + ) raise click.ClickException( - f"a keel server was recorded on port {port} but its process is gone -- it crashed or " - "was killed. Its token died with it; start a new one with `keel serve` (or " - "`launchctl kickstart` the agent that runs it)." + f"a keel server was recorded on port {port} but its process ({pid}) is gone -- it " + "crashed or was killed. Its token died with it; start a new one with `keel serve` " + f"(or `launchctl kickstart` the agent that runs it).\n\n" + f" The record is in {searched}." ) raise click.ClickException( f"no recorded keel server on port {port}.\n\n" - " If one is running, it was started from a terminal -- an interactive `keel serve` " + f" Looked in {searched} -- the deployment resolved from $KEEL_HOME if that is set, " + "and otherwise from the current directory. If your server is a different deployment, run " + "this from ITS directory: `keel open` takes the same bare-invocation path as everything " + "else, so a source checkout resolves to the checkout.\n\n" + " If one is running here, it was started from a terminal -- an interactive `keel serve` " "deliberately records nothing, and its URL is printed in that terminal. Only a detached " - "server (launchd, or any run whose stdout is not a terminal) leaves a record here, " - "because that is the case where nobody can read the printed line.\n\n" + "server (launchd, or any run whose stdout is not a terminal) leaves a record, because " + "that is the case where nobody can read the printed line.\n\n" f" If none is running, start one: `keel serve --port {port}`." ) diff --git a/keel/web/runtime.py b/keel/web/runtime.py index d3774e3..da4ae4e 100644 --- a/keel/web/runtime.py +++ b/keel/web/runtime.py @@ -173,9 +173,32 @@ def read_record(port: int) -> dict[str, Any] | None: return parsed -def _process_alive(pid: int) -> bool: +def recorded_pid(record: dict[str, Any]) -> int: + """The pid a record claims, or `0` when it claims nothing usable. + + ONE conversion, because two is how a bug got in (#763 review). `live_record` wrapped its copy + in `try`/`except` and `_refuse` repeated it bare, so a record carrying `"pid": "not-a-number"` + -- which `read_record` admits, since it validates only that a token is present -- raised + `ValueError` out of the refusal path. That is the one path whose entire job is to fail + gracefully. + + `0` is the safe answer for anything unparseable: `process_alive` refuses it before signalling + (`os.kill(0, ...)` would hit this process's own group), so an unreadable pid reports as not + running rather than as anything else. + """ + try: + return int(record.get("pid", 0) or 0) + except TypeError, ValueError, OverflowError: + return 0 + + +def process_alive(pid: int) -> bool: """Is `pid` a process this user could signal? + Public since #763: `keel open`'s refusal needs it to tell a server that CRASHED from one + that is alive but not answering on its port -- two different investigations, and asserting + the wrong one sends an operator looking in the wrong place. + `signal 0` is the standard existence check: it validates the pid and permissions without delivering anything. `pid <= 0` is refused before the call, because `os.kill(0, 0)` signals the whole process GROUP -- which on a bad record would be this process, and would report the @@ -225,11 +248,7 @@ def live_record(port: int) -> dict[str, Any] | None: record = read_record(port) if record is None: return None - try: - pid = int(record.get("pid", 0)) - except TypeError, ValueError: - return None - if not _process_alive(pid): + if not process_alive(recorded_pid(record)): return None # AND something must answer on the port (#759 review). A pid check alone is not liveness: keel # dies, the OS hands that pid to anything else, and the record reads as live -- so `keel open` diff --git a/tests/web/test_open_command.py b/tests/web/test_open_command.py index ff75938..c14d064 100644 --- a/tests/web/test_open_command.py +++ b/tests/web/test_open_command.py @@ -8,6 +8,7 @@ from __future__ import annotations +import json import os from pathlib import Path @@ -310,3 +311,123 @@ def _mine(_signum: int, _frame: object) -> None: assert signal.getsignal(signal.SIGTERM) is _mine, "serve kept the handler it installed" finally: signal.signal(signal.SIGTERM, previous) + + +# -- saying WHERE it looked (found by an install that worked) ------------------------------------- + + +def test_the_refusal_names_the_directory_it_searched(home: Path) -> None: + """The reason the first real install read as a failure when it had actually succeeded. + + `keel open` resolves the deployment from the CURRENT DIRECTORY (`state_root`), and the dev + repo is itself a deployment root -- it has a `keel.db` and a `config.yaml`. So a command + chained as `cd && ... && keel open` searched `/run/` while four healthy daemons + were writing to `~/keel/run/`. The message offered three explanations and not the true one, + because none of them could be: it never said where it had looked. + + A path in the refusal turns that from a hunt into a glance. + """ + port = _a_closed_port() + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert str(runtime.run_dir()) in result.output, result.output + + +def test_the_stale_refusal_names_it_too(home: Path) -> None: + """The other branch. A crashed server and a wrong directory are different problems and an + operator staring at either one needs the same fact to tell them apart.""" + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="Z9-stale-Z9", interactive=False) + path = runtime.record_path(port) + path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0')) + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert str(runtime.run_dir()) in result.output, result.output + assert "Z9-stale-Z9" not in result.output, "a stale token was printed anyway" + + +def test_the_help_says_the_deployment_comes_from_the_working_directory() -> None: + """`keel open` takes no `--db` and no `--config`, so nothing in its signature hints that the + answer depends on where you are standing. That is invisible until it bites.""" + result = CliRunner().invoke(cli, ["open", "--help"]) + assert result.exit_code == 0 + lowered = result.output.lower() + assert "directory" in lowered or "deployment" in lowered, result.output + + +def test_a_live_pid_that_is_not_listening_is_not_reported_as_dead(home: Path) -> None: + """`live_record` has had TWO failure modes since the port probe arrived, and the refusal knew + about one. + + Measured against a record whose pid was the running test process: the message said "its + process is gone -- it crashed or was killed" about the very process printing it. A server that + is alive but not yet listening, or bound to a host other than the one recorded, got told it + had crashed -- pointing the operator at the wrong investigation entirely. + """ + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert "crashed" not in result.output, result.output + assert str(os.getpid()) in result.output, "the message does not name the pid it checked" + assert str(port) in result.output + + +def test_a_dead_pid_still_reads_as_a_server_that_stopped(home: Path) -> None: + """The other branch, and the one that must not be lost while fixing the first.""" + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + path = runtime.record_path(port) + path.write_text(path.read_text().replace('"pid": ' + str(os.getpid()), '"pid": 0')) + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exit_code != 0 + assert "crashed" in result.output or "gone" in result.output, result.output + + +def test_the_refusal_is_typed_as_never_returning() -> None: + """Both branches raise, so the `return` that used to follow the call was unreachable. + `NoReturn` lets a type checker prove that rather than a reader assuming it.""" + import typing + + from keel.commands.open_console import _refuse + + assert typing.get_type_hints(_refuse).get("return") is typing.NoReturn + + +def test_the_refusal_does_not_claim_the_path_came_from_the_directory_when_it_may_not( + home: Path, +) -> None: + """`state_root` honours `KEEL_HOME` FIRST, and this fixture sets it -- so "the deployment this + directory belongs to" was false for exactly the reader most likely to have set it.""" + port = _a_closed_port() + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert "KEEL_HOME" in result.output, result.output + + +def test_a_malformed_pid_refuses_cleanly_instead_of_raising(home: Path) -> None: + """The refusal path is the one that must never traceback, and it was the one that did. + + `live_record` wrapped the pid conversion in `try/except (TypeError, ValueError)`; `_refuse` + repeated the conversion bare, and `read_record` validates only that a token is present. So a + record carrying `"pid": "not-a-number"` produced `ValueError: invalid literal for int()` out + of the code whose docstring promises the opposite -- "`keel open` must not traceback at an + operator whose server has just died". + """ + port = _a_closed_port() + runtime.record_serving(host="127.0.0.1", port=port, token="tok", interactive=False) + path = runtime.record_path(port) + path.write_text(json.dumps({**json.loads(path.read_text()), "pid": "not-a-number"})) + + result = CliRunner().invoke(cli, ["open", "--port", str(port), "--no-browser"]) + assert result.exception is None or isinstance(result.exception, SystemExit), result.exception + assert result.exit_code != 0 + assert "Error:" in result.output + + +def test_one_place_parses_the_recorded_pid(home: Path) -> None: + """Two conversions of one field is how the bug above existed: `live_record` guarded its copy + and `_refuse` did not. `recorded_pid` is the single answer both ask for.""" + assert runtime.recorded_pid({"pid": 42}) == 42 + assert runtime.recorded_pid({"pid": "7"}) == 7 + for junk in ({}, {"pid": None}, {"pid": "not-a-number"}, {"pid": [1]}, {"pid": 1.5e400}): + assert runtime.recorded_pid(junk) == 0, junk