mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(coordinator): address Copilot review on title persistence
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).
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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 ""
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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",
|
||||
]
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user