From 1c38397af59f99d1ab94fd2838dab326366635de Mon Sep 17 00:00:00 2001 From: ohdearquant <122793010+ohdearquant@users.noreply.github.com> Date: Sun, 27 Sep 2026 23:50:48 -0400 Subject: [PATCH] cli: own an explicit --log before the session starts; a lock read that fails is a problem row `lion chat --log X` checked that X did not exist and then started the session; the log was created later, when the console first wrote. Two starts naming the same path in that window both passed the check and shared one log. The path is now created here, exclusively, as `fresh_log` does for the automatic name, and a second start naming it is refused; a start that fails before anything is logged removes the log it created, which is now the only log it can have. `_held` read every failed `flock` as a holder, and an `os.open` or `os.read` that failed escaped `resident`, so one resident's unreadable pid file took the whole table down. Only a refused shared lock is a holder now; any other failure raises, and `resident` writes it on that resident's row as its problem, serving false, pid unknown. `controls_info` reads the same failure as busy, so a restart takes `force` rather than cutting a wake short on a guess. Two help strings and one ADR pointer said what the code does not: the patch is applied with plain `git apply`, the areas registry carries `chair` and `desks`, and the `lion context` command's tests are in tests/test_context.py. --- apps/cli/lion_cli/chat.py | 12 ++++-- apps/cli/lion_cli/main.py | 4 +- apps/cli/tests/test_chat_command.py | 29 ++++++++++++++ .../016-command/ADR-0016-the-lion-command.md | 2 +- hub/areas.py | 28 +++++++++---- tests/test_areas.py | 39 +++++++++++++++++++ 6 files changed, 101 insertions(+), 13 deletions(-) diff --git a/apps/cli/lion_cli/chat.py b/apps/cli/lion_cli/chat.py index 3b831ba..c39408c 100644 --- a/apps/cli/lion_cli/chat.py +++ b/apps/cli/lion_cli/chat.py @@ -278,8 +278,14 @@ async def chat_main(a: argparse.Namespace) -> None: prior = resolve_chat(chats, a.resume) if a.resume else None # before this session's own log exists if a.log: log = Path(a.log) - if log.exists(): - raise SystemExit(f"{log} exists; `lion chat -r {log}` continues it, another --log starts fresh") + log.parent.mkdir(parents=True, exist_ok=True) + try: + with log.open("x"): # created here, exclusively, as `fresh_log` does: one session owns a log + pass + except FileExistsError: + raise SystemExit( + f"{log} exists; `lion chat -r {log}` continues it, another --log starts fresh" + ) from None chat_id = log.stem else: chat_id, log = fresh_log(chats, time.strftime("%Y%m%d-%H%M%S")) @@ -288,7 +294,7 @@ async def chat_main(a: argparse.Namespace) -> None: except BaseException: # a start that failed before anything was logged (a bad hooks.toml, no image) must not leave # an empty log as `-c`'s next target: the previous history would be lost to the next session - if log.exists() and log.stat().st_size == 0: # --log included: an existing path was refused above + if log.exists() and log.stat().st_size == 0: # both paths created the log above, so it is ours log.unlink() raise diff --git a/apps/cli/lion_cli/main.py b/apps/cli/lion_cli/main.py index fbb7cdc..9d9a01a 100644 --- a/apps/cli/lion_cli/main.py +++ b/apps/cli/lion_cli/main.py @@ -60,7 +60,7 @@ def parser() -> argparse.ArgumentParser: ch.add_argument( "--apply", action="store_true", - help="apply the box's patch to this directory at the end (git apply --3way); default: print + save", + help="apply the box's patch to this directory at the end (git apply); default: print + save", ) ch.add_argument( "--code", @@ -92,7 +92,7 @@ def parser() -> argparse.ArgumentParser: "areas", help="the residents by area: serving, in a wake, wakes, today's runs and cost" ) ar.add_argument( - "--file", default="~/.lionagi/areas.toml", help="the registry: [area.] chair, home, agents" + "--file", default="~/.lionagi/areas.toml", help="the registry: [area.] chair, desks" ) ar.add_argument( "--serve", type=int, default=None, metavar="PORT", help="serve the page and /api/areas on PORT" diff --git a/apps/cli/tests/test_chat_command.py b/apps/cli/tests/test_chat_command.py index 1e641a3..a4df563 100644 --- a/apps/cli/tests/test_chat_command.py +++ b/apps/cli/tests/test_chat_command.py @@ -333,3 +333,32 @@ def test_two_consoles_in_one_second_get_two_logs_and_an_existing_log_is_refused( assert a[1].read_text() == "kept\n" with pytest.raises(SystemExit, match="exists"): asyncio.run(chat_main(parser().parse_args(["chat", "--dir", str(tmp_path), "--log", str(a[1])]))) + + +def test_an_explicit_log_is_owned_before_the_session_starts_and_a_failed_start_leaves_none( + tmp_path, monkeypatch +): + import lion_cli.chat as cli + + log = tmp_path / "logs" / "mine.jsonl" + seen = [] + + async def stub(a, directory, chats, prior, chat_id, log): + seen.append((chat_id, log.exists())) # the log is this session's before anything else runs + if directory.name == "fail": + raise RuntimeError("no image") + log.write_text("row\n") + + monkeypatch.setattr(cli, "_chat_main", stub) + args = ["chat", "--dir", str(tmp_path), "--log", str(log)] + asyncio.run(chat_main(parser().parse_args(args))) + assert seen == [("mine", True)] and log.read_text() == "row\n" + with pytest.raises(SystemExit, match="exists"): # a second start naming the same log is refused + asyncio.run(chat_main(parser().parse_args(args))) + assert seen == [("mine", True)] and log.read_text() == "row\n" + fail = tmp_path / "fail" + fail.mkdir() + other = tmp_path / "logs" / "other.jsonl" + with pytest.raises(RuntimeError, match="no image"): + asyncio.run(chat_main(parser().parse_args(["chat", "--dir", str(fail), "--log", str(other)]))) + assert not other.exists() and log.read_text() == "row\n" # its own empty log removed, no other diff --git a/docs/adr/016-command/ADR-0016-the-lion-command.md b/docs/adr/016-command/ADR-0016-the-lion-command.md index c24f240..4eefa4e 100644 --- a/docs/adr/016-command/ADR-0016-the-lion-command.md +++ b/docs/adr/016-command/ADR-0016-the-lion-command.md @@ -142,7 +142,7 @@ chat quietly on an interrupt. `context` takes its verbs as a second level of sub - **Landing evidence**: `apps/cli/tests/test_chat_command.py`: the chat flags read through `parser()`; `tests/test_areas.py`: the command printing the table from a given registry; - `tests/test_checkpoint.py`: `lion context` set, checkpoint, show, restore and status through the + `tests/test_context.py`: `lion context` set, checkpoint, show, restore and status through the command. ### D2: `agent_main`, `land` and `arm` in `apps/cli/lion_cli/agent.py` ^d2 diff --git a/hub/areas.py b/hub/areas.py index 73075ab..a5bf4bf 100644 --- a/hub/areas.py +++ b/hub/areas.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio +import errno import fcntl import html import json @@ -125,17 +126,22 @@ def load_areas(path: Path | str = DEFAULT_REGISTRY) -> list[Area]: def _held(pid_file: Path) -> tuple[bool, int | None]: - """(locked by a live process, the pid it wrote): the lock is the claim, the pid is for people.""" - if not pid_file.exists(): + """(locked by a live process, the pid it wrote): the lock is the claim, the pid is for people. + Only a refused shared lock is a holder; a pid file that cannot be opened, read or locked at all + raises its OSError, for the caller to say, rather than reading as serving or as stopped.""" + try: + fd = os.open(pid_file, os.O_RDONLY) + except FileNotFoundError: return False, None - fd = os.open(pid_file, os.O_RDONLY) try: text = os.read(fd, 64).decode(errors="replace").strip() pid = int(text) if text.isdigit() else None try: fcntl.flock(fd, fcntl.LOCK_SH | fcntl.LOCK_NB) # a holder has it exclusive: this fails - except OSError: - return True, pid + except OSError as e: + if e.errno in (errno.EWOULDBLOCK, errno.EAGAIN): + return True, pid + raise fcntl.flock(fd, fcntl.LOCK_UN) return False, pid finally: @@ -157,7 +163,11 @@ def resident(area: Area, directory: str, today: str | None = None) -> Resident: role = "desk" if "desk" in cfg else "chores" except (OSError, tomllib.TOMLDecodeError) as e: problem.append(f"chores.toml: {e}") - serving, pid = _held(d / "agent.pid") + try: + serving, pid = _held(d / "agent.pid") + except OSError as e: # one resident's unreadable lock is its own problem, not the whole table's + serving, pid = False, None + problem.append(f"agent.pid: {e}") cursor: dict[str, Any] = {} spend: dict[str, Any] = {} notes = d / "notes" / "agent.json" @@ -299,7 +309,11 @@ def controls_info(directory: str) -> dict: launchd_label = label if isinstance(label, str) and label else None idle = True pid_file = d / "agent.pid" - if _held(pid_file)[0]: # a wake on an in-process backend has no child to find + try: + running = _held(pid_file)[0] + except OSError: # an unreadable lock says nothing, so it reads as busy, as an unread pgrep does + running, idle = False, False + if running: # a wake on an in-process backend has no child to find # idle only on a cursor that names a finished wake: a first wake starts before any cursor # exists, and an unreadable one says nothing, so both read as busy, as an unread pgrep does try: diff --git a/tests/test_areas.py b/tests/test_areas.py index 443c848..688866b 100644 --- a/tests/test_areas.py +++ b/tests/test_areas.py @@ -870,6 +870,45 @@ def test_a_running_wake_under_a_held_lock_is_not_idle_without_a_model_child(tmp_ os.close(fd) +def test_a_pid_file_that_cannot_be_read_or_locked_is_that_residents_problem_and_no_holder( + tmp_path, monkeypatch +): + """Only a refused shared lock is a holder. A lock call that fails some other way, or a pid file + that cannot be opened, is said on that resident's row; the other residents still get theirs, + and the control surface reads the unknown as busy.""" + import errno + + from hub.areas import _held + + d = agent_dir(tmp_path, "desk", role="desk", cursor={"wake": 3, "outcome": "Quiet"}, spend=None) + other = agent_dir(tmp_path, "chores", role="chores", cursor={"wake": 1, "outcome": "Quiet"}, spend=None) + (d / "agent.pid").write_text("424242") + area = Area("x", "lambda:x", [str(d), str(other)]) + assert _held(tmp_path / "gone.pid") == (False, None) # vanished between a listing and the read + real = fcntl.flock + + def no_locks(fd, op): + if op & fcntl.LOCK_SH: + raise OSError(errno.ENOLCK, "No locks available") + return real(fd, op) + + monkeypatch.setattr(fcntl, "flock", no_locks) + r = resident(area, str(d), TODAY) + assert r.serving is False and r.pid is None # neither claim survives a lock call that failed + assert r.problem == f"agent.pid: [Errno {errno.ENOLCK}] No locks available" + assert controls_info(str(d))["idle"] is False # unknown is not idle: a restart would take `force` + monkeypatch.setattr(fcntl, "flock", real) + assert resident(area, str(d), TODAY).problem == "" # the control: the same file, locks working + os.chmod(d / "agent.pid", 0) + try: + rows = status([area], TODAY) + assert [r.problem.split(":")[0] for r in rows] == ["agent.pid", ""] and rows[0].serving is False + assert controls_info(str(d))["idle"] is False + finally: + os.chmod(d / "agent.pid", 0o644) + assert [r.problem for r in status([area], TODAY)] == ["", ""] + + def test_a_replayed_once_answers_the_first_result_and_acts_once(tmp_path, monkeypatch): record = tmp_path / "argv.txt" monkeypatch.setattr(areas_cli, "_LAUNCHCTL", str(recorder(tmp_path / "launchctl", record, 0)))