From 026acbf9070a7f4251baf2d370ddd7714c8395e0 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Wed, 17 Jun 2026 16:02:58 -0700 Subject: [PATCH] fix(coordinator): address Copilot review on title persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three points from the PR #676 Copilot review: - _coord_display_name ran on a lifecycle-event path and called get_workstream_display_name → get_storage(), which auto-initializes a SQLite .turnstone.db in the CWD when storage isn't initialized yet — a stray-file footgun on early-startup / unit-test paths. Add is_storage_initialized() to the storage registry and skip the DB read (fall back to ws.name) when storage isn't up. (Copilot's "skip when ws.name is non-synthetic" suggestion would have broken alias > title > name, so guard on init state instead.) - Document, on SessionUIBase, that on_aux_usage (storage/metrics, no _ws_lock state) and on_rename (queue/locked fan-out) are safe to call from a concurrent auxiliary thread — the title-gen thread now runs during streaming, and these are the only two UI hooks it touches. No behavior change: the methods were already thread-safe (the same path task_agent sub-agents use); the contract just didn't say so. Add a matching note at the title-trigger site. - Note in _coordinator_rows that the secondary `title` field is best-effort for a live coord outside the limit=200 window (the user-visible `name` stays correct via the uncapped bulk lookup, and the window is unreachable in practice — live coords are max_active- bounded and sort to the top of updated DESC). --- tests/test_coordinator_adapter.py | 15 +++++++++++++++ turnstone/console/coordinator_adapter.py | 10 ++++++++++ turnstone/console/server.py | 7 +++++++ turnstone/core/session.py | 9 +++++++-- turnstone/core/session_ui_base.py | 14 ++++++++++++-- turnstone/core/storage/__init__.py | 2 ++ turnstone/core/storage/_registry.py | 13 +++++++++++++ 7 files changed, 66 insertions(+), 4 deletions(-) diff --git a/tests/test_coordinator_adapter.py b/tests/test_coordinator_adapter.py index 4c2e2fcd..4f407238 100644 --- a/tests/test_coordinator_adapter.py +++ b/tests/test_coordinator_adapter.py @@ -123,6 +123,21 @@ def test_emit_created_seeds_resolved_display_name(tmp_path: Any) -> None: reset_storage() +def test_coord_display_name_skips_uninitialized_storage() -> None: + """_coord_display_name runs on a lifecycle-event path and must NOT trip + get_storage()'s SQLite auto-init (a stray .turnstone.db in the CWD) when + storage isn't initialized — it falls back to the placeholder ws.name and + leaves storage untouched.""" + from turnstone.console.coordinator_adapter import _coord_display_name + from turnstone.core.storage import is_storage_initialized, reset_storage + + reset_storage() + assert not is_storage_initialized() + assert _coord_display_name(_make_ws(name="ws-abcd")) == "ws-abcd" + # The resolution did not auto-initialize storage as a side effect. + assert not is_storage_initialized() + + def test_emit_state_calls_collector_state() -> None: """Post-rich-payload, emit_state passes tokens / context_ratio / activity / activity_state / content kwargs read from ws.ui's diff --git a/turnstone/console/coordinator_adapter.py b/turnstone/console/coordinator_adapter.py index 52e6b310..f418771a 100644 --- a/turnstone/console/coordinator_adapter.py +++ b/turnstone/console/coordinator_adapter.py @@ -24,6 +24,7 @@ from turnstone.core.child_source import ClusterChildSource from turnstone.core.children_registry import ChildrenRegistry from turnstone.core.log import get_logger from turnstone.core.memory import get_workstream_display_name, get_workstream_display_names +from turnstone.core.storage import is_storage_initialized from turnstone.core.workstream import Workstream, WorkstreamKind, WorkstreamState if TYPE_CHECKING: @@ -47,7 +48,16 @@ def _coord_display_name(ws: Workstream) -> str: rehydrated coordinator shows its title in the live cluster tree immediately, rather than reverting to ``ws-xxxx`` until a (for coordinators, rarely-firing) ``on_rename`` event arrives. + + Skips the DB read when storage isn't initialized: this runs on a + lifecycle-event path, and a display-name resolution must never trip + ``get_storage``'s SQLite auto-init side effect (a stray + ``.turnstone.db``) before the host has called ``init_storage`` (the + real cluster always does so at startup — this only bites early / + test call paths). The placeholder ``ws.name`` is the right fallback. """ + if not is_storage_initialized(): + return ws.name return get_workstream_display_name(ws.id) or ws.name diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 0c4341a2..2a39a5f7 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -924,6 +924,13 @@ def _coordinator_rows(request: Request) -> list[dict[str, Any]]: return m.get("alias") or m.get("title") or m.get("name") or fallback def _title(ws_id: str) -> str: + # Best-effort: the secondary ``title`` field is sourced from the + # ``limit=200`` ``meta`` map, so a live coord outside that window + # reports ``""`` here. The user-visible ``name`` stays correct + # (resolved via the uncapped ``live_display`` above, and the UI + # renders ``title || name``); the empty title is harmless and the + # window is unreachable in practice (live coords are bounded by + # ``max_active`` and sort to the top of ``updated DESC``). m = meta.get(ws_id) return str(m.get("title") or "") if m is not None else "" diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 467599e6..105ec477 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -4163,8 +4163,13 @@ class ChatSession: # generated for them. Gate on a real user message: synthetic wake # sends carry no content and ``_generate_title`` would no-op on the # empty/attachment-only case anyway (it needs first-user-message - # text). The background thread snapshots ``self.messages`` so it is - # safe to run concurrently with the streaming turn started below. + # text). Concurrency: this background thread runs alongside the + # streaming turn started below, but safely — it snapshots + # ``self.messages`` for iteration, and the only UI it touches is + # ``on_aux_usage`` (storage/metrics, no ``_ws_lock`` state) and + # ``on_rename`` (queue/locked fan-out), both documented + # auxiliary-thread-safe on ``SessionUIBase``; the provider + client + # handle concurrent requests (the same path ``task_agent`` uses). if not self._title_generated and user_input.strip() and not from_wake: self._title_generated = True threading.Thread(target=self._generate_title, daemon=True).start() diff --git a/turnstone/core/session_ui_base.py b/turnstone/core/session_ui_base.py index 72ae2c91..01d77de3 100644 --- a/turnstone/core/session_ui_base.py +++ b/turnstone/core/session_ui_base.py @@ -201,8 +201,18 @@ class SessionUIBase: methods (and the approval blocking helpers that live on subclasses); HTTP handlers drive ``_register_listener`` / ``_unregister_listener`` / ``resolve_approval`` from the event - loop. All shared state is guarded by ``_listeners_lock`` or - ``threading.Event`` primitives. + loop. All shared state is guarded by ``_listeners_lock`` / + ``_ws_lock`` or ``threading.Event`` primitives. + + Two ``on_*`` methods are additionally safe to call from a + *concurrent* auxiliary thread (e.g. background title generation in + ``ChatSession._generate_title``, or ``task_agent`` sub-agents), even + while the worker thread is mid-stream: :meth:`on_aux_usage` (a + storage ``usage_event`` write + thread-safe metric counters — it + touches none of the ``_ws_lock``-guarded inflight state + :meth:`on_status`/token writers mutate) and :meth:`on_rename` (a + queue / locked fan-out). Keep those two free of unguarded + ``_ws_*`` writes so the auxiliary-thread guarantee holds. """ def __init__(self, ws_id: str = "", user_id: str = "") -> None: diff --git a/turnstone/core/storage/__init__.py b/turnstone/core/storage/__init__.py index 928efb37..37b5a226 100644 --- a/turnstone/core/storage/__init__.py +++ b/turnstone/core/storage/__init__.py @@ -8,6 +8,7 @@ from turnstone.core.storage._registry import ( StorageUnavailableError, get_storage, init_storage, + is_storage_initialized, reset_storage, ) @@ -17,5 +18,6 @@ __all__ = [ "StorageUnavailableError", "get_storage", "init_storage", + "is_storage_initialized", "reset_storage", ] diff --git a/turnstone/core/storage/_registry.py b/turnstone/core/storage/_registry.py index 56066464..c2d636d6 100644 --- a/turnstone/core/storage/_registry.py +++ b/turnstone/core/storage/_registry.py @@ -124,6 +124,19 @@ def get_storage() -> StorageBackend: return _storage +def is_storage_initialized() -> bool: + """Return True when the storage singleton has been initialized. + + Lets callers on lifecycle / early-startup paths consult storage + without tripping :func:`get_storage`'s SQLite auto-init side effect + (which would create ``.turnstone.db`` in the CWD). Use this to guard + a best-effort read that should simply be skipped before the host has + called :func:`init_storage` — never as a substitute for the explicit + init the app's startup performs. + """ + return _storage is not None + + def reset_storage() -> None: """Close and clear the storage backend singleton (for tests).""" global _storage