From cb5f439cce41a2741a6b9974a308a765e50f00ad Mon Sep 17 00:00:00 2001 From: Godopu Date: Sun, 18 Oct 2026 15:46:04 +0900 Subject: [PATCH] fix(opencode): a purge takes its child sessions' 1.x rows, the background service alone holds no session, XDG_DATA_HOME is followed (Issue #60, review) - Review 614b6263 (codex, P2): `opencode session delete` removes the child sessions with their parent, but their 1.x rows stayed and the parent's residual was empty. The child sessions (session_v2 and a 1.x session table, any depth) are taken before the delete; the 1.x tables (message, part, todo, session_share, session) are cleaned for all of them, and the residual also reports a child still on record (a failed delete). - The in-use check counted any process holding the shared opencode.db; the per-user 2.x background service (`serve --service`) holds it for every session, so every opencode purge was refused while it ran (it does on this Mac). It no longer counts; any other holder and a command line naming the session still do. - The data dir follows the caller's XDG_DATA_HOME when ctx is the caller's own home (else /.local/share), for debug paths, the fallback and the residual scan (review 614b6263 / f4cedbae N4). - resident_children = ('opencode',): the --standalone server stays the TUI's idle child; what runs under it still counts (review 1abe0a90). - Tests: child sessions on a migrated database, a failed delete, the service holder, XDG, resident children, drift C pinning the parent over a newer child session (review f4cedbae N2), and the real-binary test no longer depends on PATH (N1). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff --- .../skills/lib_py/agents/adapters/opencode.py | 145 +++++++++++++----- tests/test_issue31_purge_residual.py | 8 +- tests/test_issue60_opencode_v2.py | 120 +++++++++++++++ tests/test_tier2_component.py | 13 +- 4 files changed, 245 insertions(+), 41 deletions(-) diff --git a/.agents/skills/lib_py/agents/adapters/opencode.py b/.agents/skills/lib_py/agents/adapters/opencode.py index 3d89c9b..2480d22 100644 --- a/.agents/skills/lib_py/agents/adapters/opencode.py +++ b/.agents/skills/lib_py/agents/adapters/opencode.py @@ -1,8 +1,15 @@ import os, sys, sqlite3, shutil, subprocess +from urllib.parse import quote from typing import Optional, Any, List -from lib_py.agents.base import BaseAgentAdapter, DiscoveryContext, scan_names, db_rows_naming +from lib_py.agents.base import BaseAgentAdapter, DiscoveryContext, scan_names, db_rows_naming, conversation_in_use from lib_py.verify_session import workspace_key +# Tables a 1.x opencode wrote and a migration to 2.x keeps but no longer +# writes (`opencode session delete` leaves them), and their session column. +_LEGACY_TABLES = (("message", "session_id"), ("messages", "session_id"), ("part", "session_id"), + ("parts", "session_id"), ("todo", "session_id"), ("session_share", "session_id"), + ("session", "id"), ("sessions", "id")) + _PROVIDER_ENV_VARS = ( 'ANTHROPIC_API_KEY', 'OPENAI_API_KEY', @@ -59,6 +66,12 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): # blanks after the ┃) or the `Build auto · ` status line. return r'^\s{20,}\S|^\s*(Build|Plan)\b.*·' + @property + def resident_children(self) -> tuple: + # --standalone keeps its server, `opencode serve --stdio --port 0`, as + # the TUI's child while idle (measured on 2.0.32, Issue #60). + return ('opencode',) + def _bin(self, ctx: DiscoveryContext) -> Optional[str]: bin_path = shutil.which("opencode") if bin_path: @@ -71,6 +84,15 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): return candidate_bin return None + def _data_home(self, ctx: DiscoveryContext) -> str: + """The XDG data dir opencode uses for ctx's home: the caller's + XDG_DATA_HOME when ctx is the caller's own home, else /.local/share.""" + ambient = os.environ.get("XDG_DATA_HOME") + own = os.path.realpath(ctx.home_dir or "") == os.path.realpath(os.path.expanduser("~")) + if ambient and (own or not ctx.home_dir): + return ambient + return os.path.join(ctx.home_dir, ".local/share") + def _run(self, ctx: DiscoveryContext, args: list, timeout: int): """Run the opencode CLI against ctx's home (its data dir follows HOME and XDG_DATA_HOME); None when there is no binary.""" @@ -81,7 +103,7 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): if ctx.home_dir: env["HOME"] = ctx.home_dir env["HOME_DIR"] = ctx.home_dir - env["XDG_DATA_HOME"] = os.path.join(ctx.home_dir, ".local/share") + env["XDG_DATA_HOME"] = self._data_home(ctx) cwd = ctx.workspace or ctx.cwd or os.getcwd() return subprocess.run([bin_path] + args, capture_output=True, text=True, timeout=timeout, env=env, cwd=cwd if os.path.exists(cwd) else None) @@ -98,7 +120,7 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): return parts[1].strip() except Exception: pass - return f"{ctx.home_dir}/.local/share/opencode/opencode.db" + return os.path.join(self._data_home(ctx), "opencode", "opencode.db") def artifact_path(self, uuid: str, ctx: DiscoveryContext) -> str: db = self._resolve_db(ctx) @@ -133,10 +155,15 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): def purge_artifacts(self, uuid: str, ctx: DiscoveryContext) -> list: purged = [] db = self._resolve_db(ctx) - if db and os.path.exists(db) and self._has_session(db, uuid): + if not db or not os.path.exists(db): + return purged + # The child sessions are taken before opencode deletes them with + # their parent: their 1.x rows are this purge's to remove too. + ids = [uuid] + self._descendants(db, uuid) + if self._has_session(db, uuid): # opencode deletes a 2.x session with its child sessions and every - # row it keeps for them; --standalone so no background service is - # started for it. + # 2.x row it keeps for them; --standalone so no background service + # is started for it. try: res = self._run(ctx, ["session", "delete", "--standalone", "--", uuid], timeout=60) if res is None: @@ -148,33 +175,47 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): f"{(res.stderr or res.stdout).strip()}\n") except Exception as e: sys.stderr.write(f"WARN: purge opencode: session delete {uuid} failed: {e}\n") - if db and os.path.exists(db): - # Rows a 1.x opencode left in the tables a migration keeps. - try: - conn = sqlite3.connect(db) - cursor = conn.cursor() - cursor.execute("SELECT name FROM sqlite_master WHERE type='table'") - tables = {row[0] for row in cursor.fetchall()} - if "message" in tables: - conn.execute("DELETE FROM message WHERE session_id=?", (uuid,)) - if "messages" in tables: - conn.execute("DELETE FROM messages WHERE session_id=?", (uuid,)) - if "part" in tables: - conn.execute("DELETE FROM part WHERE session_id=?", (uuid,)) - if "parts" in tables: - conn.execute("DELETE FROM parts WHERE session_id=?", (uuid,)) - if "session" in tables: - conn.execute("DELETE FROM session WHERE id=?", (uuid,)) - if "sessions" in tables: - conn.execute("DELETE FROM sessions WHERE id=?", (uuid,)) - conn.commit() - conn.close() - if tables & {"message", "messages", "part", "parts", "session", "sessions"}: - purged.append(f"sqlite rows for session: {uuid}") - except Exception as e: - sys.stderr.write(f"WARN: purge opencode db records failed: {e}\n") + # The rows a 1.x opencode left (a migration keeps those tables). + try: + conn = sqlite3.connect(db) + tables = {row[0] for row in conn.execute("SELECT name FROM sqlite_master WHERE type='table'")} + legacy = [(t, c) for t, c in _LEGACY_TABLES if t in tables] + for t, c in legacy: + for i in ids: + conn.execute(f'DELETE FROM "{t}" WHERE "{c}"=?', (i,)) + conn.commit() + conn.close() + if legacy: + kids = f" and {len(ids) - 1} child session(s)" if len(ids) > 1 else "" + purged.append(f"sqlite rows for session: {uuid}{kids}") + except Exception as e: + sys.stderr.write(f"WARN: purge opencode db records failed: {e}\n") return purged + def _descendants(self, db: str, uuid: str) -> list: + """Child sessions (any depth) of uuid in session_v2 and a 1.x session table.""" + found, todo = [], [uuid] + try: + conn = sqlite3.connect(f"file:{quote(os.path.abspath(db))}?mode=ro", uri=True) + except Exception: + return found + try: + tables = {r[0] for r in conn.execute("SELECT name FROM sqlite_master WHERE type='table'")} + sources = [t for t in ("session_v2", "session") if t in tables + and "parent_id" in {r[1] for r in conn.execute(f'PRAGMA table_info("{t}")')}] + while todo: + cur = todo.pop() + for t in sources: + for (cid,) in conn.execute(f'SELECT id FROM "{t}" WHERE parent_id=?', (cur,)): + if cid and cid != uuid and cid not in found: + found.append(cid) + todo.append(cid) + except Exception: + pass + finally: + conn.close() + return found + def _has_session(self, db: str, uuid: str) -> bool: try: conn = sqlite3.connect(db) @@ -186,11 +227,43 @@ class OpenCodeAgentAdapter(BaseAgentAdapter): return False def residual_artifacts(self, uuid: str, ctx: DiscoveryContext) -> list: - # Issue #31: whatever still names the session (event-sourced rows a - # 1.x opencode left, a failed `session delete`) is reported. - found = scan_names(f"{ctx.home_dir}/.local/share/opencode", uuid) - return found + db_rows_naming(self._resolve_db(ctx), uuid, ("session_id", "aggregate_id"), - id_tables=(("session", "id"), ("session_v2", "id"))) + # Issue #31: whatever still names the session or a child session still + # on record (a failed `session delete`) is reported. + db = self._resolve_db(ctx) + found = scan_names(os.path.join(self._data_home(ctx), "opencode"), uuid) + for i in [uuid] + (self._descendants(db, uuid) if db and os.path.isfile(db) else []): + rows = db_rows_naming(db, i, ("session_id", "aggregate_id"), + id_tables=(("session", "id"), ("session_v2", "id"))) + found += rows if i == uuid else [f"{r} [child session {i}]" for r in rows] + return found + + def conversation_in_use(self, uuid: str, ctx: DiscoveryContext) -> bool: + """A process naming the session, or an opencode process holding the + shared database. The per-user background service (`serve --service`) + holds it for every session and runs none of MAM's (MAM spawns + --standalone), so it alone does not make a session in use.""" + if conversation_in_use(uuid, []): + return True + db = self._resolve_db(ctx) + if not db or not os.path.exists(db) or not shutil.which("lsof"): + return False + try: + pids = subprocess.run(["lsof", "-t", db], capture_output=True, text=True, timeout=10).stdout.split() + except Exception: + return True + for pid in pids: + try: + cmd = subprocess.run(["ps", "-o", "command=", "-p", pid], capture_output=True, text=True, + timeout=10).stdout + except Exception: + return True + if cmd.strip() and "serve --service" not in cmd: + return True + return False + + def in_use_hints(self, uuid: str, ctx: DiscoveryContext) -> list: + return super().in_use_hints(uuid, ctx) + [ + f"another opencode session holding {self._resolve_db(ctx)}: lsof {self._resolve_db(ctx)}"] # opencode 2.x: the default agent is build (`--agent` is gone from the # TUI), and --standalone keeps the server a child of the pane's TUI diff --git a/tests/test_issue31_purge_residual.py b/tests/test_issue31_purge_residual.py index 414dc35..618262b 100644 --- a/tests/test_issue31_purge_residual.py +++ b/tests/test_issue31_purge_residual.py @@ -896,10 +896,10 @@ def _db(path: Path, ddl: list, rows: list): conn.close() -def test_opencode_residual_reports_event_rows(home, tmp_path, monkeypatch): +def test_opencode_purge_leaves_no_rows(home, tmp_path, monkeypatch): # A database migrated from 1.x (Issue #60): `opencode session delete` - # (the fake, as measured on 2.0.32) removes the 2.x rows, the purge - # deletes the 1.x session/message/part rows, and todo is reported. + # (the fake, as measured on 2.0.32) removes the 2.x rows and the purge + # deletes the rows of the 1.x tables 2.x no longer writes. u, o = f"ses_{uuidlib.uuid4().hex}", f"ses_{uuidlib.uuid4().hex}" db = home / ".local" / "share" / "opencode" / "opencode.db" ddl = ["CREATE TABLE session (id TEXT PRIMARY KEY)", @@ -931,7 +931,7 @@ def test_opencode_residual_reports_event_rows(home, tmp_path, monkeypatch): assert adapter.purge_artifacts(u, ctx) == [f"opencode session (with its child sessions): {u}", f"sqlite rows for session: {u}"] - assert adapter.residual_artifacts(u, ctx) == [f"{db} (todo.session_id: 1 rows)"] + assert adapter.residual_artifacts(u, ctx) == [] assert len(adapter.residual_artifacts(o, ctx)) == 8 # the other session is untouched: one entry per table diff --git a/tests/test_issue60_opencode_v2.py b/tests/test_issue60_opencode_v2.py index fcd5ba0..17adc6d 100644 --- a/tests/test_issue60_opencode_v2.py +++ b/tests/test_issue60_opencode_v2.py @@ -18,6 +18,7 @@ import shutil import sqlite3 import subprocess import sys +import time from pathlib import Path import pytest @@ -145,6 +146,122 @@ def test_a_failed_delete_is_warned_and_reported(oc, monkeypatch, capsys, broken) assert f"{adapter._resolve_db(ctx)} (session_v2.id: 1 rows)" in adapter.residual_artifacts("ses_main", ctx) +def test_purge_takes_the_1x_rows_of_the_child_sessions_too(oc): + """A migrated database: the child sessions opencode deletes with their + parent keep 1.x rows that only the purge removes (review 614b6263).""" + adapter, ctx, db, ws, calls = oc + conn = sqlite3.connect(str(db)) + conn.execute("CREATE TABLE session (id TEXT PRIMARY KEY, parent_id TEXT)") + conn.execute("CREATE TABLE message (id TEXT PRIMARY KEY, session_id TEXT)") + conn.execute("CREATE TABLE todo (id TEXT, session_id TEXT)") + conn.commit() + conn.close() + tree = [("ses_main", None), ("ses_child", "ses_main"), ("ses_grand", "ses_child"), ("ses_other", None)] + for sid, parent in tree: + _add(db, sid, ws, 2_000_000, parent=parent) + conn = sqlite3.connect(str(db)) + conn.execute("INSERT INTO session VALUES (?, ?)", (sid, parent)) + conn.execute("INSERT INTO message VALUES (?, ?)", (f"m-{sid}", sid)) + conn.execute("INSERT INTO todo VALUES ('t', ?)", (sid,)) + conn.commit() + conn.close() + + assert any(r.endswith("[child session ses_grand]") for r in adapter.residual_artifacts("ses_main", ctx)) + purged = adapter.purge_artifacts("ses_main", ctx) + assert purged == ["opencode session (with its child sessions): ses_main", + "sqlite rows for session: ses_main and 2 child session(s)"] + for sid in ("ses_main", "ses_child", "ses_grand"): + assert adapter.residual_artifacts(sid, ctx) == [], sid + # the unrelated session is untouched: session_v2, session_message, event_sequence, session, message, todo + assert len(adapter.residual_artifacts("ses_other", ctx)) == 6 + + +def test_a_failed_delete_reports_the_child_sessions_left(oc, monkeypatch): + adapter, ctx, db, ws, calls = oc + _add(db, "ses_main", ws, 2_000_000) + _add(db, "ses_child", ws, 2_000_000, parent="ses_main") + monkeypatch.setattr(adapter, "_bin", lambda ctx: None) + adapter.purge_artifacts("ses_main", ctx) + residual = adapter.residual_artifacts("ses_main", ctx) + assert f"{db} (session_v2.id: 1 rows) [child session ses_child]" in residual + assert f"{db} (session_v2.id: 1 rows)" in residual + + +def _holder(tmp_path, db, *argv): + """A live process holding `db` open, with `argv` on its command line.""" + ready = tmp_path / f"ready-{len(argv)}" + code = ("import sys, time; f = open(sys.argv[1]); open(sys.argv[2], 'w').close(); time.sleep(60)") + p = subprocess.Popen([sys.executable, "-c", code, str(db), str(ready), *argv]) + for _ in range(100): + if ready.exists(): + break + time.sleep(0.05) + return p + + +@pytest.mark.skipif(shutil.which("lsof") is None, reason="no lsof") +def test_the_background_service_alone_does_not_hold_a_session(oc, tmp_path): + """The per-user service keeps the shared database open for every session + (2.x); MAM runs none of its sessions there.""" + adapter, ctx, db, ws, calls = oc + _add(db, "ses_main", ws, 2_000_000) + assert adapter.conversation_in_use("ses_main", ctx) is False + service = _holder(tmp_path, db, "serve", "--service") + try: + assert adapter.conversation_in_use("ses_main", ctx) is False + other = _holder(tmp_path, db, "--auto", "--standalone") # another session's TUI or its server + try: + assert adapter.conversation_in_use("ses_main", ctx) is True + finally: + other.kill() + other.wait() + finally: + service.kill() + service.wait() + named = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(60)", "--session", "ses_main"]) + try: + time.sleep(0.2) + assert adapter.conversation_in_use("ses_main", ctx) is True + finally: + named.kill() + named.wait() + + +def test_the_data_dir_follows_the_callers_xdg_data_home_only_for_its_own_home(tmp_path, monkeypatch): + adapter = get_adapter("opencode") + monkeypatch.setattr(adapter, "_bin", lambda ctx: None) + own, other, xdg = tmp_path / "own", tmp_path / "other", tmp_path / "xdg" + monkeypatch.setenv("HOME", str(own)) + monkeypatch.setenv("XDG_DATA_HOME", str(xdg)) + ws = str(tmp_path) + assert adapter._resolve_db(DiscoveryContext(workspace=ws, agent_name="opencode", home_dir=str(own))) \ + == f"{xdg}/opencode/opencode.db" + assert adapter._resolve_db(DiscoveryContext(workspace=ws, agent_name="opencode", home_dir=str(other))) \ + == f"{other}/.local/share/opencode/opencode.db" + monkeypatch.delenv("XDG_DATA_HOME") + assert adapter._resolve_db(DiscoveryContext(workspace=ws, agent_name="opencode", home_dir=str(own))) \ + == f"{own}/.local/share/opencode/opencode.db" + + +def test_the_standalone_server_is_a_resident_child_but_its_tools_are_work(): + """--standalone keeps `opencode serve --stdio` under the TUI while idle; + the stall watcher skips it but still sees what runs under it.""" + from lib_py.stall_watch import background_children + resident = get_adapter("opencode").resident_children + tui = subprocess.Popen(["bash", "-c", "(exec -a opencode sleep 300) & exec sleep 300"], start_new_session=True) + busy = subprocess.Popen(["bash", "-c", "(exec -a opencode bash -c 'sleep 300; :') & exec sleep 300"], + start_new_session=True) + try: + time.sleep(0.5) + assert background_children(tui.pid, ()) != [] + assert background_children(tui.pid, resident) == [] + assert background_children(busy.pid, resident) != [] + finally: + for p in (tui, busy): + os.killpg(p.pid, 9) # the whole group: children and grandchildren + p.wait() + + def test_spawn_and_resume_never_pass_agent_and_run_standalone(): """The shell copies of the opencode command line match the adapter.""" adapter = get_adapter("opencode") @@ -214,6 +331,9 @@ def test_real_opencode_2x_contract(tmp_path, monkeypatch): json.dumps({"title": "mam child", "parentID": parent})).stdout)["data"]["id"] adapter = get_adapter("opencode") + # The binary was found with the real HOME; under the test's HOME only a + # PATH entry would find it (review f4cedbae N1). + monkeypatch.setattr(adapter, "_bin", lambda ctx: REAL) ctx = DiscoveryContext(workspace=ws_real, agent_name="opencode", home_dir=str(home)) db = adapter._resolve_db(ctx) assert db == str(home / ".local" / "share" / "opencode" / "opencode.db") diff --git a/tests/test_tier2_component.py b/tests/test_tier2_component.py index dd632a7..0228ff6 100644 --- a/tests/test_tier2_component.py +++ b/tests/test_tier2_component.py @@ -1249,6 +1249,17 @@ def test_comp_opencode_create_and_reconcile_and_stop(mam_sandbox, mock_herdr, mo assert s["opencode_session_id_own"] is None assert s["session_id_source"] == "pending-discovery" + # A subagent's session in the same directory, newer than the TUI's own + # (opencode 2.x, Issue #60): drift C must pin the top-level session. + import sqlite3 + odb = os.path.join(os.environ["HOME"], ".local", "share", "opencode", "opencode.db") + conn = sqlite3.connect(odb) + (parent, directory, created), = conn.execute("SELECT id, directory, time_created FROM session_v2").fetchall() + conn.execute("INSERT INTO session_v2 (id, parent_id, directory, time_created) VALUES (?, ?, ?, ?)", + (f"{parent}-child", parent, directory, created + 1000)) + conn.commit() + conn.close() + # Run reconcile.sh to trigger drift-C materialization reconcile_script = mam_sandbox / ".agents" / "skills" / "multi-agent-mux-monitor" / "scripts" / "reconcile.sh" res_rec = subprocess.run(["bash", str(reconcile_script), "--once", "--emit-diff"], capture_output=True, text=True, cwd=str(mam_sandbox), env={**os.environ, "WORKSPACE_ROOT": str(mam_sandbox)}) @@ -1259,7 +1270,7 @@ def test_comp_opencode_create_and_reconcile_and_stop(mam_sandbox, mock_herdr, mo with open(yaml_path) as f: data_after = yaml.safe_load(f) s_after = [x for x in data_after.get("herdr_sessions", []) if x.get("name") == session_name][0] - assert s_after.get("opencode_session_id_own") is not None + assert s_after.get("opencode_session_id_own") == parent assert s_after.get("last_visible_status") in ("pinned", "resume_verified") # Stop session