mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-24 12:54:48 -06:00
perf/webui-transcript-windowing
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
2169559d6e |
feat(projects): governed project containers — memory scope, grouping, manage UI (#724)
* feat(projects): governed project containers — memory scope, grouping, manage UI
A workstream can attach to a project: a first-class, shareable resource
container that owns a `project` memory scope, groups conversations, and is
managed from the console.
Storage / migration 062: projects + project_members tables, workstreams.
project_id, and the memory type default project→general; grants
project.{create,read,write,delete} (admin-default).
Recall + writes: project memory is recalled iff the workstream is attached AND
the user has access (owner ∨ member ∨ public-for-read), resolved once at session
construction; coordinators recall it too. New saves default to the project when
attached + writable; the save and delete paths are write-gated; deleting a
project purges its scoped memory; archived projects aren't recalled.
Access = RBAC capability ∧ per-project ACL (auth.resolve_project_access, a
single-fetch resolver); visibility changes, member management, and delete are
owner-only.
API: project CRUD routes on both the server and console; project_id threaded
through workstream creation, spawn inheritance, the cluster-create proxy, the
dashboard / snapshot / coordinator row builders, and the collector deltas.
UI: a project picker with an inline "+ New project" creator in every creation
box (console launcher + standalone dialog + dashboard); group-by-project in the
rail; a project badge in the composer and on dashboard rows; a console manage
tab (list + create/edit + members shelves). The admin Memories view gains
coordinator/project scope filters and human scope labels (name, not hex). The
memory tool schema documents the project scope and the attach-aware default.
* fix(projects): client refresh hardening, creator race guard, SDK project_id
Addresses PR #724 review feedback plus two bugs found while validating it.
- projects.js refreshProjects: a non-OK status (e.g. 403 when the caller
lacks project.read) or a network/parse error no longer blanks the cache
or masquerades as "no projects" -- the prior cache is preserved, the
failure is recorded (new projectsError()) and warned. Honors the
long-standing "a transient error can't blank the rail" docstring.
- projects.js _fp: the fingerprint separators were raw control bytes,
which made git treat the whole file as binary (no reviewable diff).
Rewritten as escape sequences instead of raw bytes -- behavior is
byte-identical at runtime.
- project_creator.js: createProject() could reject unhandled (authFetch
throws on network/401; r.json() throws on a non-JSON body), leaving the
widget stuck busy/disabled. Added a .catch, plus a generation guard so a
create whose widget was cancelled/reopened mid-flight drops its result
instead of selecting a project the user backed out of.
- types.ts: add project_id to CreateWorkstreamRequest / WorkstreamInfo /
DashboardWorkstream to match the server schemas (was SDK-invisible).
- test_project_api.py: move side-effecting HTTP calls out of asserts so
the requests run even under python -O.
* fix(projects): JSON.stringify the cache fingerprint, drop control-byte separators
_fp joined fields/rows on raw NUL/SOH bytes, which made projects.js read as binary to git. Replace with a collision-proof, escape-free JSON.stringify encoding -- same change-detection semantics, zero embedded control characters.
|
||
|
Patrick Buckley
|
3f106f98b2 |
fix(metacog): apply-pass fixes from pre-push full-stack review
Round-2 review caught 11 confirmed findings on the 3-commit metacog stack; this commit applies them. * **bug-1 (major)**: Wake source tag was leaking onto real user messages flushed during a wake send. ``_append_user_turn`` and ``send`` now take an explicit ``from_wake: bool`` parameter — only the wake's synthesized first turn passes True, so ``_flush_queued_messages``'s real user input no longer inherits the audit tag. Regression test pins the contract. * **perf-1 (major)**: ``CoordinatorIdleObserver._maybe_enqueue`` was issuing list_workstreams + visible_memory_count storage queries before the cheap cooldown gate could short-circuit. New ``_cooldown_allows`` read-only peek runs first; storage queries only fire when cooldown actually allows the nudge. * **q-1 (major)**: Added the missing coord-side integration test that exercises ``CoordinatorIdleObserver`` + ``IdleNudgeWatcher`` together in the production install order against a real ``SessionManager``, protecting the subscription-order contract from silent regression. * **perf-2/3 (minor)**: Cap check moved above ``_last_assistant_used_wait``; ``_fire_counts`` restructured as ``dict[str, dict[str, int]]`` keyed by ws_id so the leave-IDLE existence check is O(1). * **perf-4 (minor)**: ``NudgeQueue.drain`` fast-paths the all-match case (the common one for chat-loop drain seams) by swapping ``self._items`` directly instead of allocating a fresh ``kept`` deque + per-entry append. * **perf-5 (minor)**: Wake's synthesized empty user turn no longer writes a content-empty row to the conversations table — the ``_source`` audit tag isn't column-backed and the side-channel reminder is stripped before persist, so the row would carry nothing. * **q-3 (minor)**: Split ``IdleNudgeWatcher`` + ``install_*`` / ``shutdown_*`` helpers out of ``metacognition.py`` into the new ``turnstone/core/idle_nudge_watcher.py``; metacog stays a static-template module. * **sec-1 (nit)**: Widened ``_sanitize_child_name``'s control-char regex to cover Unicode bidi-overrides, zero-width chars, line/paragraph separators, BOM, and tag chars. * **q-4/q-5 (nits)**: Docstring referenced the wrong peek primitive (``has_pending`` → ``len()``); ``_last_assistant_used_wait``'s ``session`` parameter now typed ``ChatSession``. 5571 non-live tests pass; ruff + mypy clean. |
||
|
Patrick Buckley
|
3308e3645a |
fix(core): scope rehydrate fallback to manager, fix resume orphan
Address Copilot feedback on PR #465: 1. The has_alias fallback in both session_factories silently rewrote any unknown caller-supplied alias to the default, including on the fresh-create path where the create handler maps the factory's ValueError to a 503 with operator-friendly text. A typo in body.model would now silently start a workstream on the default instead of telling the caller their requested model could not be resolved. Move the fallback out of the factories: each factory raises again on unknown aliases, and SessionManager filters stale aliases out of the rehydrate path via a new ``model_validator`` constructor kwarg (production wiring passes ``registry.has_alias`` on both interactive and coordinator). 2. ChatSession.resume()'s elif branch flipped self.model to the persisted model name even when the alias was unresolvable, leaving the session paired with the constructor's default provider/client but a removed model name — a broken state whose next API call fails. Drop the model copy: keep the constructor's coherent default (provider + model + capabilities) and just log the unreachable saved values so the missing alias is auditable. Tests: - Move stale-alias coverage from the factory level into SessionManager (tests/test_session_manager.py): validator drops stale aliases before reaching build_session; live aliases pass through unchanged. - tests/test_sessions.py renamed test_resume_restores_model → test_resume_keeps_defaults_when_alias_unresolvable to match the new contract. |
||
|
Patrick Buckley
|
0a43bed3d5 |
fix(core): preserve workstream model + config on rehydrate
SessionManager.open() was calling build_session(ws) without a model arg on the rehydrate path. The session_factory then resolved the *current* default alias, ChatSession.__init__'s _save_config() (INSERT OR REPLACE per-key) clobbered the persisted workstream_config with those defaults, and the subsequent resume() "restored" what was now the default — silently resetting model_alias, model, temperature, reasoning_effort, max_tokens, skill, creative_mode, instructions, token_budget, and notify_on_complete on every reopen and every service restart, for both interactive and coordinator workstreams. Three layers: 1. SessionManager.open() now reads workstream_config via self._storage.load_workstream_config(ws_id) and threads the saved model_alias into build_session(ws, model=saved_alias). 2. ChatSession.__init__ now skips its initial _save_config() when a workstream_config row already exists for self._ws_id — protects every other persisted knob without having to plumb each one through the adapter signature, and catches any future construction path that forgets to thread model through build_session. 3. Both session_factories (server.py interactive, console session_factory.py coordinator) now treat an unknown caller- supplied alias the same as an unset alias: fall back to the runtime default rather than raising. Without this, a workstream pinned to an alias an operator has since removed from the registry would 500 on every reopen — defeating the "best effort restore, default if the original is gone" contract this fix is meant to deliver. Mirrors _effective_default_alias's existing has_alias guard against a stale ConfigStore default. |
||
|
Patrick Buckley
|
38a0d9c3b6 |
feat(coord): Stage 3 SessionManager Children primitive lift + cluster bus push paths
Lift the Children primitive out of CoordinatorAdapter into universal SessionManager core primitives, replace the fragile poll + state-event piggyback paths with first-class cluster bus event types for inline approval delivery, and clean up the resulting frontend reducer. Architecture - New `turnstone/core/children_registry.py` — universal parent → children + reverse-lookup primitive with atomic `add_child` (returns parent UI for race-free dispatch). Lifted from `CoordinatorAdapter`. - New `turnstone/core/child_source.py` — `ChildSource` Protocol with `SameNodeChildSource` (in-process via SessionManager state observer) and `ClusterChildSource` (cross-node via ClusterCollector listener). - `SessionManager._on_state_change` upgraded to multi-subscriber (`subscribe_to_state` / `unsubscribe_from_state`) under a dedicated lock; CLI consumer migrated. - `CoordinatorAdapter` shrunk: 731 → ~640 LOC. Children data lives in the registry; fan-out lives in ClusterChildSource. Backward-compat property facades dropped; tests updated to use the registry surface. Cluster bus event vocabulary - New event types `intent_verdict`, `approval_resolved`, `approve_request` flow through both `ClusterCollector._apply_delta` (translation from node SSE) and `emit_console_ws_*` (synthesis on console pseudo-node). - `CoordinatorAdapter._dispatch_child_event` re-emits as `child_ws_intent_verdict` / `child_ws_approval_resolved` / `child_ws_approve_request` on the parent coord's SSE stream. - New `_broadcast_intent_verdict` / `_broadcast_approval_resolved` / `_broadcast_approve_request` no-op hooks on `SessionUIBase`. WebUI pushes to the global queue; ConsoleCoordinatorUI pushes to the collector. `approve_tools` calls `_broadcast_approve_request` right after setting `_pending_approval` so the items reach the coord tree immediately, eliminating the bulk-fetch race. Cleanups - `pending_approval_detail` piggyback on `ws_state` / `cluster_state` removed end-to-end. Bulk fetch + explicit verdict / approve-request push are the canonical carriers. - Browser `_judgePollTick` 90-second poll loop deleted; push path is authoritative. - `urgent` flag on `scheduleLiveFetch` deleted (only caller was 409 retry; replaced with `invalidateLiveBadge` + standard schedule). - Console `_fetch_live_block` derives `pending_approval` from a disjunction (`activity_state="approval"` OR `state="attention"` OR detail present) so the bulk fetch can't return false during the state-transition race window. - Coord-side merge guard in `flushLiveFetches` no longer clobbered: `handleChildState` only stamps `sseUpdatedAt` when authoritatively clearing detail. - `child_locality` capability flag removed (was inert dead code). Reliability - Selective drop on listener queue overflow: critical event types (verdicts, approvals, ws_closed, child_ws_*) evict one oldest item to make room rather than dropping themselves on a full queue. Best-effort events (state ticks, content tokens, status, activity) drop as before. Applied to `SessionUIBase._enqueue`, `ClusterCollector._fanout`, and the `WebUI._global_queue` puts in the new broadcast hooks. - `_state_subscribers` snapshot under a dedicated lock so concurrent subscribe / unsubscribe during dispatch can't shift the iterator. UX / a11y - Loading placeholder in renderChildRow keeps row height stable while the bulk fetch is in-flight (sr-friendly aria-label). - Focus preservation across `_renderChildrenNow` (capture + restore by row + marker) and across targeted `_updateChildRow` swaps. - Layout-shift transition on the approval block max-height; respects `prefers-reduced-motion`. - Sidebar pending count: `(N children · M pending)`. - Risk pill `aria-label` spells out level + confidence for SR users. - Per-coord SSE listener queue depth surfaced in the status bar (`queue N/500`) with color escalation (warn at >50%, danger at >80%). Tests - 305+ test changes across 8 files. New unit tests for `ChildrenRegistry`, `ChildSource` (both impls + multi-subscriber observer), the new collector emit + apply_delta cases, the dispatch cases for new event types, the broadcast hook overrides on both WebUI and ConsoleCoordinatorUI, and the focus / placeholder / pending-count frontend assertions in `test_coordinator_page.py`. 5024 passed, ruff + mypy clean. |
||
|
Patrick Buckley
|
0debc5d061 |
fix(session_manager): scope orphan reaper by services.last_heartbeat
Replaces the ``node_id == self_node_id`` orphan-scoping heuristic from earlier on this branch with liveness-based scoping using ``services.last_heartbeat``. The heuristic was wrong for the post-#384 world: PR #384 (refactor: replace hash-ring rebalancer with rendezvous hashing) deleted the rebalancer that used to keep workstreams.node_id pointing at a live node. Without it, ``workstreams.node_id`` is now stamped at create time and never updated, so in containerized deployments with dynamic hostnames a dead pod's rows have ``node_id`` matching no surviving service — they'd accumulate forever under the old heuristic. services.last_heartbeat is the same primitive the rendezvous router uses for routing. Reusing it here keeps reap scoping aligned with routing: dead pods' rows fall out of the live set after the heartbeat window and become reapable; alive pods' rows stay protected as long as they heartbeat. Mechanics: - ``bulk_close_stale_orphans`` parameter renamed ``node_id: str | None`` → ``live_node_ids: list[str] | None``. The WHERE clause becomes ``(node_id IS NULL OR node_id NOT IN live_node_ids)``. ``None`` skips the filter entirely (single-process / tests / operator backfill). ``[]`` treats every row as unprotected. - ``SessionManager.close_idle`` pass 2 calls ``storage.list_services(self._service_type)`` to enumerate live peers, passes their service_ids as ``live_node_ids``. ``_service_type`` is derived from ``self.kind`` (INTERACTIVE→"server", COORDINATOR→"console") via a module-level mapping — no constructor param, so production wiring can't miswire the kind/service_type pairing. - list_services failure → pass 2 is skipped this tick (conservative; never reap when liveness state is unknown). Pass 1 still runs. - ``workstreams.node_id`` with NULL value is always eligible — defends against ANSI ``NULL NOT IN (...)`` evaluating to NULL (not TRUE) and silently protecting orphans forever. - Migration 048 simplified to ``(kind, updated)``; the new query's ``NOT IN (small list)`` predicate against an unbounded-cardinality column doesn't index well, so leading ``node_id`` would just add write cost. Tests cover the live-services protection (own/dead/null cases), the empty-peers reap-all case, the list_services-failure conservative fallback, both kind/service_type pairings (interactive→"server", coordinator→"console"), and the combined live_node_ids + exclude_ws_ids filter matrix. |
||
|
Patrick Buckley
|
1405afe079 |
fix(session_manager): close DB-orphan workstreams in close_idle
Real bug: workstream rows accumulate in non-closed states (idle, thinking, attention, running) when their owning process restarts or crashes. Empirical diagnosis on a live deployment found ~60 stuck coord rows in DB invisible to the in-memory-keyed dashboard, plus 100+ interactive rows older than the 2h timeout (one stuck "thinking" for 2 weeks — impossible across a process restart). Root cause: close_idle iterates self._workstreams.values() — only the loaded subset. Anything left behind by a prior process incarnation sits in DB forever because nothing ever re-loads it. This commit gives close_idle a second pass. Pass 1 (existing, unchanged): close loaded IDLE rows whose ws.last_active (monotonic) is past timeout. IDLE-only so legitimately- attentive rows (waiting for user response) stay live. Pass 2 (new): bulk-close DB rows of this manager's kind whose updated is past the wall-clock cutoff and which aren't currently loaded. Closes the broader BULK_CLOSE_STATE_VALUES set — any matching row is by definition not loaded by any process and cannot be in a live interaction. Scoped by self._node_id so a sibling node can't reap rows we own (multi-node interactive correctness). No emit_closed — never-loaded rows have no SSE listeners expecting them. Lock invariant: pass 1 holds self._lock briefly to snapshot victims and pop them (existing behavior). Pass 2 holds self._lock briefly to snapshot the loaded keys, then releases before the DB UPDATE so a slow reaper query can't block create/get/set_state. Also fixes a same-process race in open(): the rehydrate path read DB, released the manager lock, then re-acquired to install — a concurrent pass 2 between the two acquisitions snapshots loaded keys without the in-flight ws_id, and could clobber its DB row to closed. open() now calls touch_workstream(ws_id) on rehydrate so the row's updated is fresh against any pass-2 cutoff. Pure timestamp write is safe against concurrent close() (close still wins on the state column). Three new tests cover the DB orphan pass (basic, exclude-loaded, kind filter) plus node_id scoping (own/foreign rows, None-skips-filter) and the open() rehydrate touch. |
||
|
Patrick Buckley
|
d15f182b80 |
fix(coord): tree UI not updating when LLM deletes workstream (#429)
* fix(coord): tree UI not updating when LLM deletes workstream The coord LLM's `delete_workstream` tool wiped the storage row but fired no SSE event, so a long-lived dashboard tab kept the deleted child visible (with its last-known idle/closed state) until a full reload. A coordinator that spawns→completes→deletes children would leave an ever-growing tree. Fix: add `SessionManager.delete()` that drops the in-memory slot if present and emits `ws_closed` with `reason="deleted"` (mirrors `close()`'s shape). Wire `delete_workstream_endpoint` to call it after the storage delete succeeds, snapshotting the workstream's name into the event payload before the row is wiped. The cluster collector → coord adapter chain re-emits as `child_ws_closed`; the browser's existing `handleChildClosed` already keys on `reason === "deleted"` to mark the row, so no JS changes needed. Event emit is best-effort — a fan-out failure logs a warning but doesn't roll back the storage delete (the row is already gone). * fix(coord): apply Copilot review feedback on PR #429 - server.py: clarify that ``name`` is forwarded to mgr.delete only (not into the audit detail) — comment previously claimed both. - test_session_manager.py: extract ``mgr.delete(ws_id)`` to a local before asserting (CodeQL: no side-effecting calls inside ``assert``, which would be stripped under ``python -O``). - test_workstream_endpoints.py: docstring said "Yield" but the fixture ``return``s; switch to "Return". |
||
|
Patrick Buckley
|
c77b237033 |
refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair (#417)
* refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair Eliminates the phantom create→close pair on coord rollback that was documented as a known limitation in PR #416. The pair surfaced on the cluster events stream when a multipart workstream-create request failed attachment validation: coord's ``mgr.create`` fired ``emit_created`` synchronously, then the rollback called ``mgr.close`` which fired ``emit_closed``. Cluster consumers had to reconcile via the collector's diff path. Post-fix, a rejected upload produces zero events. API changes on ``SessionManager``: - ``create(..., defer_emit_created: bool = False)`` — when True, skip the trailing ``emit_created`` so the caller can run additional post-create work (attachment validation in the lifted HTTP handler) before advertising the workstream. Default preserves the existing "advertise immediately" contract for direct callers (test fixtures, CLI REPL, channel adapters). - ``commit_create(ws)`` — fires the deferred ``emit_created`` event after the caller's post-create work confirms the workstream should be advertised. Synchronous; the wrapped work is in-memory and non-blocking on every kind (interactive: documented no-op stub; coord: dict updates under a lock + ``queue.put_nowait`` fan-out). - ``discard(ws_id)`` — releases the in-memory slot + cleans up the UI WITHOUT firing ``emit_closed``. Distinct from ``close`` which advertises the transition; ``discard`` is for the rollback case where the workstream's existence was never advertised. Storage-row deletion stays a separate concern (caller invokes ``delete_workstream``), mirroring ``mgr.create``'s split between slot reservation and ``register_workstream``. Caller-bug detection: ``Workstream._emit_created_fired`` is set inside ``create`` (non-deferred path) and ``commit_create``; ``discard`` logs ``session_mgr.discard.after_emit_created`` warning when invoked on an already-advertised workstream. Slot is still released so capacity isn't stranded. Lifted ``make_create_handler`` updated to use the deferred bracket: pass ``defer_emit_created=True``, validate uploaded attachments, then ``mgr.commit_create(ws)`` on success / ``mgr.discard(ws.id)`` on failure. Ordering invariants (``commit_create`` BEFORE ``audit_emit`` and ``post_install`` so any state events the worker fires reach the cluster collector for an already-known ws_id) are documented in the handler docstring. Tests: - 5 new ``SessionManager`` unit tests (defer skips emit, commit fires it, commit no-ops without emitter, discard releases without emit_closed, discard returns False on unknown id). - 2 caller-bug regression tests (commit_create after discard pins the silent re-emit behaviour; discard after non-deferred create asserts the warning fires + slot still releases). - 1 coord regression test asserting the cluster collector sees zero events when attachment validation fails. ``/review`` pipeline run; M1 (test gap on caller-bug paths) + Mi1 (no runtime guard for already-advertised) + Mi2 (``_make_manager`` event_emitter override) + Mi3 / N2 (duplicated comments + ordering invariant) + N1 (drop ``to_thread`` on ``commit_create``) all addressed. 4509 tests passing; ruff + mypy clean. * fix(core): apply Copilot + code-quality review feedback on PR #417 Copilot review: - ``Workstream._emit_created_fired`` comment claimed the flag was "set under the manager's _lock-protected emit", but the actual ordering set it OUTSIDE the lock. Comment updated to describe the real synchronization (non-deferred ``create`` sets it immediately before ``emit_created``; ``commit_create`` sets it under the manager lock alongside the tracked-ws check). - ``commit_create`` had no guard against duplicate calls, post-discard calls, or calls on workstreams not tracked by this manager — any of those would have fired duplicate or phantom ``ws_created`` events. Added a guard symmetric to ``discard``'s after-emit warning: under ``self._lock``, check ``_emit_created_fired`` + ``_workstreams.get(ws.id) is ws``, no-op + log a warning (``session_mgr.commit_create.already_fired`` / ``session_mgr.commit_create.untracked``) on either failure. The emit itself still runs outside the lock so coord's collector fan-out doesn't couple to the manager mutex. - ``test_commit_create_after_discard_is_caller_bug_no_op`` was internally inconsistent — name + docstring said "must not re-emit" but the assertion expected the re-emit. Renamed to ``test_commit_create_after_discard_is_no_op`` and updated to assert the new no-op + warning behaviour. New test ``test_commit_create_is_idempotent_on_duplicate_call`` pins the second-commit-call code path: exactly one ``ws_created`` event fires, second call short-circuits via the guard with a ``commit_create.already_fired`` warning. Code-quality bot review (3 findings, identical pattern): - Three test ``assert`` statements wrapped side-effecting calls (``assert mgr.discard(ws_id) is True/False``); under ``python -O`` the asserts strip and the side-effect strips with them. Refactored all three to assign the result to a local first, assert on the local. No behaviour change. 4510 tests passing; ruff + mypy clean. |
||
|
Patrick Buckley
|
48c9ad2a40 |
refactor(core): split SessionKindAdapter Protocol into construction +… (#412)
* refactor(core): split SessionKindAdapter Protocol into construction + emission (Stage 2 P3)
The single ``SessionKindAdapter`` Protocol that ``SessionManager``
takes is split into two:
* ``SessionKindAdapter`` — kind / build_ui / build_session /
cleanup_ui. Required for every kind. The shared lifecycle
manager always delegates here for construction + cleanup.
* ``SessionEventEmitter`` — emit_created / emit_state /
emit_rehydrated / emit_closed. **Optional**, wired through a new
``event_emitter: SessionEventEmitter | None = None`` kwarg on
``SessionManager``. Reserved for future kinds whose lifecycle
transitions don't fan out anywhere; both production kinds wire
one today.
Both production adapters implement both Protocols. The interactive
lifespan (``server.py``) and console lifespan
(``console/server.py``) pass their adapter as both ``adapter`` and
``event_emitter`` — production behaviour is unchanged. Six lifecycle
sites in ``SessionManager`` (create / open eviction / open rehydrate /
close / set_state / close_idle / _reserve_and_install_locked unwind)
now call ``self._event_emitter.emit_*(...)`` guarded by
``if self._event_emitter is not None``.
InteractiveAdapter asymmetry preserved + documented:
* ``emit_closed`` stays load-bearing — it's the **sole** transport
path for ``ws_closed`` onto the process-wide global SSE queue
(Stage 1 consolidated emission from the create handler here so
there's exactly one emission point; ``name`` powers the
frontend's eviction toast).
* ``emit_created`` / ``emit_state`` / ``emit_rehydrated`` are
documented no-op stubs (``del ws[, state]``). Those events fire
from out-of-band paths — the create HTTP handler enqueues
``ws_created`` directly onto ``global_queue`` *after* attachment
validation (so a rejected upload doesn't surface a phantom
create→close pair); ``WebUI._broadcast_state`` emits the full
``ws_state`` payload (tokens + context_ratio + activity) via the
``SessionUI.on_state_change`` callback chain. The stubs exist
solely to satisfy ``SessionEventEmitter`` Protocol so the
adapter can be wired as the manager's ``event_emitter`` for the
``emit_closed`` path. Each stub has a 1-line inline rationale to
match the in-repo convention (``coordinator_adapter.py:210``).
Test scaffolding:
* ``tests/test_session_manager.py`` — ``_make_manager`` and
``_make_with_writer`` wire ``FakeAdapter`` as both ``adapter``
and ``event_emitter`` for production parity; the standalone
``test_create_uses_configured_node_id`` does the same.
``FakeAdapter.emit_rehydrated`` now records as
``_Event("rehydrated", ...)`` rather than conflating with
``"created"``, and ``test_open_resurrects_closed_state`` asserts
against ``events_of("rehydrated")`` so a regression where the
manager fires the wrong call on the open path actually fails.
* ``tests/_coord_test_helpers.py`` and
``tests/test_coordinator_end_to_end.py`` — wire
``CoordinatorAdapter`` as both args.
* Six interactive test fixtures (``test_skills.py``,
``test_prompt_templates_runtime.py`` x2, ``test_model_registry.py``,
``test_server_authz.py``, ``test_server_attachments_on_create.py``)
— wire ``event_emitter=adapter`` so they match the production
wiring, removing the footgun where a future contributor adds a
``gq.get_nowait()`` assertion and silently loses the only
``ws_closed`` transport.
* ``tests/test_interactive_adapter.py`` — drops the three
tautological no-op-emit_* tests (``test_emit_created_is_noop``,
``test_emit_state_is_noop``, ``test_emit_rehydrated_is_noop``);
keeps the four ``emit_closed`` tests (real behaviour).
Lint + mypy clean. 4475 tests passing.
* docs(core): correct SessionKindAdapter + SessionEventEmitter docstrings to match implementation
Two Copilot review threads on PR #412 caught the same real
discrepancy: my P3 docstrings on ``SessionKindAdapter`` and
``SessionEventEmitter`` described an *intent* — "interactive
doesn't implement ``SessionEventEmitter``; the manager skips emit
calls when no emitter is wired" — that doesn't match the actual
wiring. ``InteractiveAdapter`` does implement both Protocols and
``server.py`` does pass it as ``event_emitter``; only the three
no-op stubs (``emit_created`` / ``emit_state`` / ``emit_rehydrated``)
are dead, while ``emit_closed`` is load-bearing.
Updated both docstrings to:
* State that both production adapters implement both Protocols.
* Explain the asymmetry is in *which* emit methods carry real
bodies (coord: 4; interactive: 1, with 3 documented stubs because
the out-of-band paths — create handler ``ws_created`` after
attachment validation, ``WebUI._broadcast_state`` carrying the
richer ``ws_state`` payload — fire those events).
* Clarify the ``if self._event_emitter is not None`` guard exists
for the kwarg-omitted case (tests that don't care about events,
reserved for future kinds whose transitions don't fan out
anywhere).
Docstring-only change. Lint + mypy clean; the 75 tests in
test_session_manager + test_interactive_adapter + test_coordinator_adapter
pass.
Resolves the two Copilot review threads on PR #412 (commits
PRRC_kwDORcMomM67VyPD, PRRC_kwDORcMomM67VyPI).
|
||
|
Patrick Buckley
|
a8cd9444b1 |
fix(server): apply Copilot + code-quality review feedback
PR #410 review pass: * **session_worker**: ``except BaseException`` → ``except Exception`` in ``_runner`` (code-quality bot). Daemon threads don't receive SystemExit/KeyboardInterrupt, so the wider catch was unjustified defensive style. Same defense-in-depth for unexpected ``run()`` exceptions; doesn't widen scope to runtime signals. * **session_worker**: ``threading.Thread()`` construction moved inside the spawn branch under ``ws._lock`` (Copilot). The enqueue path no longer allocates and then discards a Thread object on each call against a busy workstream. Thread() construction is microsecond-cheap, so the lock-window growth is negligible vs. the saved allocation churn. * **lifespans**: ``state_writer.shutdown()`` (and the console equivalent) now run via ``asyncio.to_thread`` so the daemon- thread join + sync DB drain don't block the event loop and delay other teardown tasks (Copilot, ×2). * **tests**: five remaining ``writer._flush_once()`` calls switched to the public ``writer.flush()`` API across test_session_manager.py (4) and test_state_writer.py (1) (Copilot, ×5). Tests no longer depend on private internals. |
||
|
Patrick Buckley
|
8240e32704 |
test(core): regression tests for state_writer + close ordering
Five new tests under ``TestSessionManagerWithStateWriter`` exercise the bug-3 invariant under write-behind: * set_state buffers via the writer (long flush_interval → no sync write until drain). * set_state(ERROR) flushes synchronously. * close after a buffered transient writes 'closed' as the final state — the buffered 'running' must NOT be flushed to storage AFTER close's sync 'closed' write. * close_idle exhibits the same invariant. * set_state arriving AFTER close short-circuits on ws._closed and never reaches the buffer. |
||
|
Patrick Buckley
|
c837e3fa6d |
feat(core): Stage 1 SessionManager unification (#408)
* feat(core): scaffold SessionManager + SessionKindAdapter Protocol Stage 1 step 1 — pure addition, no production wiring. Defines the shape later steps will port the shared mechanics onto: slot accounting, per-ws-id refcounted rehydrate locks, kind-agnostic lifecycle; kind-specific event transport + session construction on the adapter. Pruned from the earlier Protocol draft (see design brief): per-kind permission_scope (static handler map is simpler), allows_child_spawn / quota_policy (deleted in #403), on_child_spawned (coordinator tool owns children registry), allows_active_focus / active_id / switch (frontend owns the active-tab state). * feat(core): port shared session-lifecycle mechanics onto SessionManager Stage 1 step 2. Adds create / open / close / set_state / close_idle / get / list_all / count on top of the Step 1 scaffolding. Pure addition — still no production wiring; the new class doesn't replace any call sites yet. Concurrency shape is ported from CoordinatorManager (the more- complete side): single-phase slot reservation under the manager lock, per-ws refcounted open-lock to serialize concurrent lazy rehydrate, placeholder workstreams count toward max_active but can't evict each other. WSM's two-phase eviction outside the lock is not carried over; it had a window where a burst of creates could silently exceed max_active. Deletions (vs. the union of the two old managers): - "refuse to close last workstream" guard — handled by the dashboard; only existed to protect the now-deleted default startup workstream. - active_id / switch / get_active — frontend owns focus; server-side duplicate state is gone. - _active_coords presence cache — defer measurement to Step 4; if it pays for itself at realistic cluster sizes, the CoordinatorAdapter can maintain it by observing emit_* calls. - Children registry + reverse index — coordinator tool owns this, manager stays kind-agnostic. Skill resolution (name → template_id + applied_version) is now shared via SessionManager._resolve_skill, so WSM's pre-resolve-at- callsite pattern and CM's internal-lookup pattern converge. Callers pass the skill name; the manager does the lookup once. 26 smoke tests cover create eviction + overflow, concurrent-create cap, persist/session rollback, open for missing/deleted/wrong- kind/wrong-user rows, concurrent-open serialization, close unblocks UI + emits closed, set_state + storage + adapter observer, close_idle, list_all ordering, count, eviction fires adapter transport, node_id passthrough. * feat(core): add InteractiveAdapter for SessionManager Stage 1 step 3. Adapter that bridges SessionManager to the node's interactive transport: - emit_created/state/closed → pushes onto the process-wide SSE global_queue (same shape current server.py handlers produce inline) - cleanup_ui → ports WorkstreamManager._cleanup_ui body: unblock _approval_event / _plan_event / _fg_event, broadcast ws_closed to per-UI listener queues (with full-queue fallback), cancel + close the session - build_ui/build_session → delegate to injected factories (ui_factory builds WebUI, session_factory is the existing closure from server.py with judge_model + memory_config captures) Also extends SessionKindAdapter.build_session with **extra passthrough so interactive callers can pass judge_model per-call without polluting the manager API; and adds a reason= kwarg to emit_closed so the frontend's "evicted" special-case keeps working (frontend doesn't differentiate "idle" from "closed", so close_idle collapses into close()). 14 new adapter tests cover wire payload shape, queue.Full tolerance, cleanup_ui event unblocking + listener broadcast + queue-full fallback, session cancel+close, graceful handling of stub UIs / None session, and kwarg passthrough to the session factory. * feat(console): add CoordinatorAdapter for SessionManager Stage 1 step 4. Coordinator-side SessionKindAdapter implementation: - emit_created/state/closed → delegate to the existing ClusterCollector.emit_console_ws_* methods (same wire shape the old CoordinatorManager emitted inline) - cleanup_ui → ports the listener-queue + approval/plan event unblocks from CoordinatorManager._cleanup, with queue-full fallback so an unresponsive browser tab can't wedge close - build_ui/build_session → delegate to injected factories; session factory doesn't accept client_type so we strip it at the adapter boundary Collector emission exceptions are swallowed (same policy as today's inline fan-out — dashboard lag on one tick is preferable to breaking the lifecycle path). Intentionally out of scope: the children registry (_children / _child_to_coord) stays in the coordinator tool when wired in Step 5; the _active_coords lock-free presence cache is deferred pending a measurement at realistic cluster sizes. 10 new tests cover transport payloads, collector-exception tolerance, cleanup_ui event unblock + listener broadcast + queue-full eviction, construction passthrough. * feat(server): wire interactive server.py to SessionManager Stage 1 step 5a. Production-path swap: WorkstreamManager → SessionManager(InteractiveAdapter(...)). - Construction at server startup: build the adapter with the process-wide global_queue, a WebUI ui_factory closure, and the existing session_factory. SessionManager gets storage + max_active. - Default startup workstream wiring removed (the CLI-REPL leftover flagged in the handoff's "Convergence is also a pruning opportunity" section). --resume now lazily creates a workstream scoped to the resumed content; no workstream at all if --resume isn't given. The dashboard handles the 0-ws state. - HTTP handler mgr.create() calls switched to the new kw-only signature (user_id, name, model, skill, ws_id, client_type, judge_model, parent_ws_id). ui_factory/skill_id/skill_version/kind no longer threaded through — adapter handles UI construction and manager resolves skill internally. - Dropped the mgr.last_evicted block in the /new handler (adapter emits ws_closed:evicted automatically on capacity eviction). - mgr.max_workstreams → mgr.max_active. - Added active_id / switch / switch_by_index / get_active / index_of / eviction_count to SessionManager because turnstone/cli.py uses them extensively; the handoff's "delete unless there's a live caller" rule flips here — CLI is a live caller. Test fixtures across 9 files updated to build SessionManager + InteractiveAdapter rather than WorkstreamManager. test_workstream.py stays unchanged (it tests WSM directly; it'll be deleted in step 5d alongside the class itself). Full pytest: 4528 passed. Ruff + mypy clean. Next: 5b (console-side wiring, with the children-registry relocation to the coordinator tool). * feat(console): wire console server to SessionManager Stage 1 step 5b. Production-path swap: CoordinatorManager → SessionManager(CoordinatorAdapter(...)). - CoordinatorAdapter now owns the coord-specific bits that were bolted onto the old CoordinatorManager: the children registry (forward + reverse index), the lock-free active-coords presence cache, the cluster-event fan-out thread, and the worker-dispatch path (send / _spawn_worker). The shared SessionManager stays kind-agnostic. - Added CoordinatorAdapter.attach(mgr) for late-binding the owning manager (the manager's ctor takes the adapter, so the dependency has to break here). Used inside _rebuild_children_registry for the tenant- filtered SQL query, inside send/dispatch for mgr.get(ws_id), and inside the fan-out seed path for mgr.list_all(). - emit_created now seeds the children registry + active-coords slot AND calls _rebuild_children_registry (covers both create — empty query — and open/rehydrate, where the subtree is persisted). emit_closed drops both entries. Collapses the three old call-sites in CoordinatorManager's create/open/close into one per-event hook. - Console server.py builds the manager via: coord_adapter = CoordinatorAdapter(collector=..., ...) coord_mgr = SessionManager(coord_adapter, storage=..., max_active=..., node_id=ClusterCollector.CONSOLE_PSEUDO_NODE_ID) coord_adapter.attach(coord_mgr) ConsoleCoordinatorUI._coord_mgr = coord_mgr app.state.coord_adapter = coord_adapter - HTTP handler call-site updates: - coord_mgr.create drops initial_message; the handler now calls coord_adapter.send(ws.id, initial_message) after create so the worker spawn stays out of the shared manager. - coord_mgr.open_admin(ws_id) → coord_mgr.open(ws_id, user_id="", admin=True). Matches SessionManager.open's unified signature. - coord_mgr.list_for_user(uid) inlined as a list comp on list_all() (SessionManager doesn't expose the filter; two callers). - coord_mgr.children_snapshot / send → coord_adapter.*. - coord_mgr.cancel stays (now lives on SessionManager from 5a). - ConsoleCoordinatorUI.on_state_change now flows state transitions through ConsoleCoordinatorUI._coord_mgr.set_state, mirroring the WebUI pattern. The old _on_state_observer / _on_rename_observer closures the manager used to install are dead code now; leaving the fields in place for 5d cleanup. - Lifespan shutdown calls coord_adapter.shutdown() (was coord_mgr. shutdown()) and resets ConsoleCoordinatorUI._coord_mgr on teardown. Test fixture updates in _coord_test_helpers, test_coordinator_end_to_end, test_coordinator_endpoints, test_phase6_endpoints: build SessionManager + CoordinatorAdapter in _build_mgr, set app.state.coord_adapter, switch mgr.register_children / mgr.children_snapshot tests to mgr._adapter.*, and rewrite test_open_admin_uses_open_admin to assert the unified open(user_id="", admin=True) call shape. Full pytest: 4486 passed. Ruff + mypy clean. Next: 5d (remove CoordinatorManager + WorkstreamManager class bodies and their test files). * feat(core): delete WorkstreamManager + CoordinatorManager classes Stage 1 step 5c + 5d. Final step of the unification — the legacy classes and their test files go away now that every production caller has been ported. - Delete turnstone/console/coordinator.py entirely (CoordinatorManager class + the _enqueue_on_ui helper, which CoordinatorAdapter now hosts its own copy of). - Trim turnstone/core/workstream.py to just the Workstream dataclass + WorkstreamKind + WorkstreamState. ~385 lines of WorkstreamManager logic gone; the remaining shape is pure data types shared by both managers. - Delete tests/test_workstream.py (WSM-specific) and tests/test_coordinator_manager.py (CM-specific). - Wire turnstone/cli.py to SessionManager + InteractiveAdapter, same pattern as turnstone/server.py. The CLI's WorkstreamTerminalUI uses manager.set_state + manager.active_id — both preserved on SessionManager (CLI is a live caller that keeps the focus API honest, per the handoff's "delete unless it pulls its weight" rule). - Add an optional manager-level ``_on_state_change`` observer hook restored for the CLI's background-attention notification (the web path uses the adapter's emit_state; this hook covers callers that don't consume SSE). - Drop dead ``_on_state_observer`` / ``_on_rename_observer`` fields from ConsoleCoordinatorUI — the old CoordinatorManager installed them; SessionManager/CoordinatorAdapter handle fan-out directly. Vulture @ 80% confidence: zero unused symbols across the new SessionManager + adapter files. Ruff + mypy clean (170 files). Full pytest (excluding tests/live): 4414 passed. Net across the whole Stage 1 branch: one unified SessionManager + adapter Protocol replaces two ~500-line parallel managers + a ~600-line CoordinatorManager, and the interactive + coordinator transports stay cleanly separated at the adapter boundary. * refactor(auth): drop workstream row-level ownership gates Turnstone is a trusted-team tool (per #400). user_id stays as metadata for audit + display; it no longer rejects requests. Scope- level auth via admin.workstreams / admin.coordinator tokens is the only gate now. Solves sec-1 (cross-tenant delete via collision on caller-supplied ws_id, because the gate was half-implemented) and sec-2 (blank-sub JWT bypass on empty-owner rows). Net: 359 lines of defensive empty-string comparisons and admin=True bypass plumbing deleted. * fix(core): serialize set_state vs close + worker spawn Three concurrency fixes from the multi-stage review: - bug-3: set_state now looks up ws under self._lock and gates its storage write on ws._closed (a new tombstone flag). close() sets ws._closed=True and does its storage write under ws._lock. A set_state that acquires ws._lock after close sees the tombstone and skips its write instead of resurrecting the closed row. - bug-1: _spawn_worker wraps the check-and-spawn in ws._lock so two concurrent send() HTTP requests can't both observe "no live worker" and start duplicate worker threads on the same ChatSession. - bug-2: replaces Thread.is_alive() as the reuse gate with an explicit ws._worker_running flag. The flag is set before the worker thread starts and cleared in its finally block — both under ws._lock. Using is_alive() left a narrow window where the worker could exit between the check and a queue_message call, stranding the user's message with no consumer. perf-2 (lock-held-across-DB-write) is accepted as-is: per-ws serialization of state transitions behind a DB round-trip is real cost but bounded — a given ws's state flips happen sequentially on its worker thread anyway. Dropping ws._lock around the DB write would reintroduce the bug-3 race. Full pytest: 4401 passed. Ruff + mypy clean. * refactor(core): drop _resolve_skill from SessionManager Skill resolution (name → template_id + applied_version) moves out of the shared manager and back to the HTTP handlers that own the create request. The interactive handler already resolved skill_data + applied_skill_version for other purposes (model override, judge config, post-create session seed) and was passing the name to SessionManager which then redundantly re-resolved via get_skill_by_name + count_skill_versions — two wasted DB round-trips per create on a user-visible latency path. - SessionManager.create: accepts skill_id + skill_version as already-resolved kwargs; _resolve_skill helper deleted. - turnstone/server.py create_workstream: passes the skill_id / applied_skill_version it already computed. - turnstone/console/server.py coordinator_create: pre-resolves inline (parity with interactive) before calling coord_mgr.create. Fixes perf-1 (redundant skill queries per create), q-4 (divergent skill-version computation between manager and handler), q-5 (coordinator-specific lookup on the shared manager surface). Full pytest: 4401 passed. Ruff + mypy clean. * refactor(adapters): extract shared cleanup_ui + drop dead child-registry methods Both InteractiveAdapter.cleanup_ui and CoordinatorAdapter.cleanup_ui (plus their _broadcast_ws_closed_to_listeners helpers) were byte-identical. Pull them into turnstone/core/adapters/_ui_cleanup.py:cleanup_session_ui so the two adapters delegate to one implementation. Also drop CoordinatorAdapter.register_children (only test callers — now use _seed_children in tests/_coord_test_helpers.py) and _add_child (zero callers anywhere). * refactor(adapters): symmetric attach() + fail-loud on unattached manager Add InteractiveAdapter.attach(manager) + .manager property mirroring the coord-side pattern. CLI (cli.py) now uses cli_adapter.attach(manager) instead of the _mgr_ref list-ref late-binding hack; server.py picks up the same call for consistency. CoordinatorAdapter.send / _rebuild_children_registry / _prime_children_from_snapshot no longer silently return when self._manager is None — raise RuntimeError so a forgotten attach() at startup fails loud instead of dropping the whole fan-out. * docs: replace stale WorkstreamManager / CoordinatorManager references Both classes were deleted in 965e0b6; prose docstrings across the codebase still named them. Update to SessionManager (or describe the collapsed-into-one-class architecture where the distinction matters). Leaves the 'Ported from …' historical markers in session_manager.py / coordinator_adapter.py / interactive_adapter.py intact — those are deliberate pointers back to the pre-unification code. * fix(core): atomic close_if_idle + batch pop under one lock bug-5: SessionManager.close_idle re-checked ws.state == IDLE outside the lock, so a pending tool result could flip state IDLE→RUNNING between the snapshot and close() acquiring self._lock. Add _close_if_idle_locked that tests state + pops under self._lock. perf-5: drop the per-victim self._lock acquisition; collect + pop the whole batch in one acquisition, then run cleanup_ui / storage write / emit_closed outside the lock. * perf(coord): split emit_created / emit_rehydrated to skip storage query on fresh creates CoordinatorAdapter.emit_created was unconditionally calling _rebuild_children_registry (storage.list_workstreams with parent_ws_id=... limit=10001) on every create, even for fresh-create paths that provably have zero children. Add emit_rehydrated to the SessionKindAdapter Protocol. SessionManager .create still calls emit_created; .open (lazy rehydrate) now calls emit_rehydrated. CoordinatorAdapter.emit_created seeds the registry + fan-out but skips the rebuild; emit_rehydrated seeds + rebuilds + fans out. InteractiveAdapter.emit_rehydrated delegates to emit_created (no children-registry on the interactive transport). * perf(coord): fold _active_coords into _children_lock + mutate payload in place perf-4: _active_coords used a copy-on-write dict-swap pattern so the fan-out dispatch could read it lock-free, but _dispatch_child_event already re-validates the parent under _children_lock anyway — the lock-free snapshot was premature. Replace with a plain dict read+write both under _children_lock; install and remove collapse to one-liners. Value also drops the user_id half — dead after |