diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bec8aa2..a7c98a35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,16 @@ ## [Unreleased] +### Changed + +- **Docs: reconciled the assistant-reply lifecycle RFCs/contracts with shipped behavior.** The Stable Assistant Turn Anchors and Live-to-Final RFCs, `docs/CONTRACTS.md`, and the Phase 0 architecture inventory were updated from "proposed/scaffold" framing to "accepted/implemented" to match what actually ships (single assistant-turn owner projecting one `activity_scene_v1` into Compact Worklog / Transparent Stream / Final answer). Documentation-only — no runtime behavior or CI-gate change; the one code touch is a corrected comment header in `static/assistant_turn_anchors.js`. Remaining hardening stays tracked under #3400. Thanks @franksong2702. (#6144) + ### Fixed +- **Background subagents no longer fail to persist when the parent's cached agent is reused mid-turn.** A parent session's `SessionDB` handle is shared by reference with the background subagents it spawns (`delegate_tool`), but the cached-agent reuse path unconditionally closed that handle when the parent got a new turn / server-side wakeup — so every still-running child then failed `append_message` with `'NoneType' object has no attribute 'execute'` (observed 600+ times under one parent). The reuse path now keeps a still-open handle (closing only the unused fresh one, preserving the #1421 FD-leak fix) and only replaces a dead/missing one. The credential self-heal path was hardened the same way: if rebuilding the handle fails it degrades cleanly to a lazy reinit instead of reusing a closed handle. Thanks @carlotestor. (#6143) + +- **The CLI/cron session-list projection cache is now bounded.** `_CLI_SESSIONS_CACHE` was an unbounded plain dict of deep-copied session-list projections whose fingerprint-folded key advances on every streamed message, so orphaned heavy copies accumulated on a long-lived server until the next structural clear. It's now an `OrderedDict` capped at 8 entries with drop-oldest LRU eviction (recency refresh on hit), mirroring the existing `_CLAUDE_CODE_PARSE_CACHE` / `_SIDECAR_METADATA_CACHE` pattern; TTL stays the freshness control. Thanks @rh-id. (#6140) + - **Models selected from a list-shaped custom-provider allowlist now route to the right provider.** A `custom_providers[].models` allowlist can be written as either a mapping or a list, and the model picker accepted both — but runtime routing only read mapping keys, so a model chosen from a *list*-shaped allowlist could silently stay on the active/default provider instead of its own. The supported-model-ID extraction is now centralized in one helper used by both catalog construction and routing, so the picker and runtime can no longer drift. Existing mapping-shaped and list-of-strings configs are unaffected. Thanks @franksong2702. (#6127, #6121) - **Stale sidebar approval attention badge now clears once its gateway approval has drained.** A parent session could keep showing the red approval "attention" dot in the sidebar after the underlying gateway approval was already resolved/gone — the live gateway queue drained but the sidebar summary kept the mirrored approval alive. `_session_attention_summary()` now reconciles the gateway pending-mirror before counting attention (the same self-healing repair the approval-pending route already does), so the badge clears correctly. Live approvals stay visible and actionable and the clarify fallback is unchanged. This is the WebUI read-path mitigation; the child-session context-propagation root cause stays tracked in #6100. Thanks @rodboev. (#6117, #6100) diff --git a/api/models.py b/api/models.py index 48a879a1..49888ad2 100644 --- a/api/models.py +++ b/api/models.py @@ -63,7 +63,16 @@ _CLI_SESSIONS_CACHE_STREAMING_TTL_SECONDS = 45.0 _CLI_SESSIONS_CACHE_LOCK = threading.Lock() _CLI_SESSIONS_CACHE_INFLIGHT: "dict[tuple, threading.Event]" = {} _CLI_SESSIONS_CACHE_INVALIDATION_VERSION = 0 -_CLI_SESSIONS_CACHE = {} +# LRU-bounded (drop-oldest) so a long-lived process under churn — where the +# state.db fingerprint advances on every streamed message and the structural +# clear-on-mutation listener doesn't fire for every fingerprint advance — can't +# accumulate orphaned heavy deepcopies. Each value is a copy.deepcopy() of the +# full CLI/cron session list (the expensive projection behind #4842/#4672), so +# the cap is deliberately small. TTL is still the primary freshness control; +# the cap is the backstop that the plain dict previously lacked. Mirrors the +# _CLAUDE_CODE_PARSE_CACHE / _SIDECAR_METADATA_CACHE LRU pattern. +_CLI_SESSIONS_CACHE: "collections.OrderedDict[tuple, tuple]" = collections.OrderedDict() +_CLI_SESSIONS_CACHE_MAX_ENTRIES = 8 _CLI_SESSIONS_CACHE_WAIT_SECONDS = 0.25 # Event waits that keep stale rows visible while a rebuild is in flight. _CLI_SESSIONS_CACHE_STALE_WAIT_SECONDS = 0.10 @@ -6405,6 +6414,9 @@ def _cache_cli_sessions_if_current( invalidation_stamp, _copy_cli_sessions(sessions), ) + _CLI_SESSIONS_CACHE.move_to_end(cache_key) + while len(_CLI_SESSIONS_CACHE) > _CLI_SESSIONS_CACHE_MAX_ENTRIES: + _CLI_SESSIONS_CACHE.popitem(last=False) return True @@ -6423,6 +6435,8 @@ def _copy_fresh_cli_sessions_cache_entry(cache_key: tuple): return None if cached_expires_at <= time.monotonic(): return None + # LRU: a fresh hit is the most-recently-used entry. + _CLI_SESSIONS_CACHE.move_to_end(cache_key) return _copy_cli_sessions(cached_sessions) @@ -7272,6 +7286,8 @@ def get_cli_sessions( if cached_stamp != _CLI_SESSIONS_CACHE_INVALIDATION_VERSION: _CLI_SESSIONS_CACHE.pop(cache_key, None) elif cached_expires_at > now: + # LRU: a fresh hit is the most-recently-used entry. + _CLI_SESSIONS_CACHE.move_to_end(cache_key) return _copy_cli_sessions(cached_sessions) else: stale_sessions = _copy_cli_sessions(cached_sessions) diff --git a/api/streaming.py b/api/streaming.py index 1b8fc70b..7ef3dd99 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -6266,6 +6266,65 @@ def _last_resort_sync_from_core(session, stream_id, agent_lock): ) +def _session_db_is_open(session_db) -> bool: + """True when *session_db* still has a live sqlite connection. + + SessionDB.close() sets ``_conn = None``. Subagents capture the parent's + SessionDB object by reference at spawn time (delegate_tool), so closing + that object mid-parent-turn makes every subsequent child + ``append_message`` fail with + ``'NoneType' object has no attribute 'execute'``. + """ + if session_db is None: + return False + return getattr(session_db, "_conn", None) is not None + + +def _adopt_session_db_for_cached_agent(agent, new_session_db): + """Attach a SessionDB to a reused cached agent without breaking subagents. + + Historical behaviour (PR #1421 FD-leak fix): create a fresh SessionDB every + stream request and close the previous handle before replacing + ``agent._session_db``. That stops EMFILE growth, but a server-side wakeup + / new turn for the same parent session will close the shared handle while + background subagents are still writing into it. + + Policy now: + - If the cached agent already holds an *open* SessionDB, keep it and close + the unused new handle (no FD leak; live subagents keep working). + - If the existing handle is missing or already closed, adopt *new_session_db*. + - If *new_session_db* is None, leave the existing handle alone. + """ + if agent is None: + return new_session_db + existing = getattr(agent, "_session_db", None) + if new_session_db is None: + return existing + if existing is new_session_db: + return existing + if _session_db_is_open(existing): + try: + new_session_db.close() + except Exception: + # Same observability as _replace_session_db_in_kwargs: a failed + # close here reintroduces the EMFILE pressure PR #1421 fixed. + logger.debug( + "Failed to close unused session_db handle in adopt helper", + exc_info=True, + ) + return existing + if existing is not None: + try: + existing.close() + except Exception: + logger.debug( + "Failed to close previous session_db handle in adopt helper", + exc_info=True, + ) + agent._session_db = new_session_db + return new_session_db + + def _build_session_db_for_stream(state_db_path): """Build a per-request SessionDB handle for WebUI session search. @@ -6281,12 +6340,36 @@ def _build_session_db_for_stream(state_db_path): def _replace_session_db_in_kwargs(agent_kwargs, state_db_path): - """Build a fresh SessionDB and replace ``agent_kwargs['session_db']`` safely.""" + """Build a fresh SessionDB and replace ``agent_kwargs['session_db']`` safely. + + Does not close an existing open handle that may still be shared with live + subagents; only replaces when the prior handle is missing or already closed. + """ if not isinstance(agent_kwargs, dict): return None _old_session_db = agent_kwargs.get("session_db") _next_session_db = _build_session_db_for_stream(state_db_path) + if _next_session_db is None: + # Replacement construction failed. Keep the prior handle only if it is + # still open (live subagents may hold it by reference); otherwise + # degrade cleanly to None — as master did — so the rebuilt agent lazily + # reinitialises its SessionDB instead of reusing a closed handle and + # failing every persist/search with + # "'NoneType' object has no attribute 'execute'". + if _session_db_is_open(_old_session_db): + return _old_session_db + agent_kwargs["session_db"] = None + return None + if _session_db_is_open(_old_session_db): + # Keep the live handle; discard the unused new one. + try: + if _next_session_db is not _old_session_db: + _next_session_db.close() + except Exception: + logger.debug("Failed to close unused session_db handle during self-heal") + agent_kwargs["session_db"] = _old_session_db + return _old_session_db if _old_session_db is not None and _old_session_db is not _next_session_db: try: _old_session_db.close() @@ -8297,15 +8380,19 @@ def _run_agent_streaming( if 'prefill_messages' in _agent_kwargs and hasattr(agent, 'prefill_messages'): agent.prefill_messages = list(_agent_kwargs.get('prefill_messages') or []) if _session_db is not None: - # Close any previously held SessionDB connection before - # replacing it. Without this, each streaming request creates - # a new SessionDB whose WAL handles leak indefinitely, - # eventually causing EMFILE crashes (#streaming FD leak). - if hasattr(agent, '_session_db') and agent._session_db is not None: - try: - agent._session_db.close() - except Exception: - pass + # Prefer reusing a still-open SessionDB on the cached + # agent. Closing it mid-turn breaks background + # subagents that hold a reference to the same object + # (delegate_tool copies parent._session_db by ref) — + # they then fail with + # 'NoneType' object has no attribute 'execute'. + # When the existing handle is already closed/missing, + # adopt the fresh per-request SessionDB (and close the + # dead one) so we still avoid the EMFILE FD-leak from + # PR #1421. + _session_db = _adopt_session_db_for_cached_agent( + agent, _session_db + ) agent._session_db = _session_db if hasattr(agent, '_api_call_count'): agent._api_call_count = 0 diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 0094a5c2..e7bcc6fc 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -29,13 +29,19 @@ contributor guidance; it does not change runtime behavior or CI gates. repairs. Start here for narrow fixes that keep the existing WebUI execution path. - [`docs/rfcs/live-to-final-assistant-replies.md`](rfcs/live-to-final-assistant-replies.md): - proposed product model for long-running assistant replies, live process text, + accepted product model for long-running assistant replies, live process text, tool activity, recovery, terminal outcomes, and final-answer boundaries. Start here for UI/UX changes to running-session assistant reply rendering. +- [`docs/rfcs/stable-assistant-turn-anchors.md`](rfcs/stable-assistant-turn-anchors.md): + implemented presentation/reconciliation model that attaches live, settled, + replayed, and recovered activity to one assistant-turn owner and projects one + `activity_scene_v1` into Compact Worklog, Transparent Stream, or Final answer + only. Remaining hardening stays tracked under #3400. - [`docs/architecture/stable-assistant-turn-anchor-phase0.md`](architecture/stable-assistant-turn-anchor-phase0.md): - current Phase 0 inventory for the Stable Assistant Turn Anchors work under - #3926. Use this before wiring anchor helpers into live SSE, replay, - settlement, `INFLIGHT`, or `renderMessages()` paths. + cumulative implementation inventory for the Stable Assistant Turn Anchors + work under #3926. Use it to distinguish shipped wiring from historical slice + boundaries before changing live SSE, replay, settlement, `INFLIGHT`, or + `renderMessages()` paths. - [`docs/rfcs/canonical-session-resolution.md`](rfcs/canonical-session-resolution.md): proposed contract for resolving URL routes, query parameters, localStorage, sidebar rows, and compression-lineage IDs to one canonical visible session diff --git a/docs/architecture/stable-assistant-turn-anchor-phase0.md b/docs/architecture/stable-assistant-turn-anchor-phase0.md index b09ef65a..ceea2023 100644 --- a/docs/architecture/stable-assistant-turn-anchor-phase0.md +++ b/docs/architecture/stable-assistant-turn-anchor-phase0.md @@ -1,12 +1,16 @@ # Stable Assistant Turn Anchors Phase 0 Inventory -This inventory implements the first non-visual slice of -[`stable-assistant-turn-anchors.md`](../rfcs/stable-assistant-turn-anchors.md). -It documents the current per-turn state layers and the event-shape contract that -future anchor phases must consume. It does not claim that anchors are wired into -streaming or rendering yet. +This inventory began as the first non-visual slice of +[`stable-assistant-turn-anchors.md`](../rfcs/stable-assistant-turn-anchors.md) and +now records the shipped phase progression. Current `master` wires Anchor-owned +`activity_scene_v1` data into live and settled Compact Worklog, Transparent +Stream, journal hydration, session re-entry, and transcript-backed persistence. -## RFC Phase Progress +The slice-by-slice sections below are retained as implementation history. +Present-tense statements such as “unwired” or “future phase” describe the point +when that slice landed; they do not override the current coverage summary above. + +## Shipped RFC Phase Progress - The #3962 Phase 0 scaffold shipped through #3977 / v0.51.359: inventory the current state layers, encode the owner seed, and pin the source classification diff --git a/docs/rfcs/README.md b/docs/rfcs/README.md index 921bb2d4..d728461d 100644 --- a/docs/rfcs/README.md +++ b/docs/rfcs/README.md @@ -47,16 +47,17 @@ First-time contributor RFCs should be discussed in an issue before opening a PR. replay, compression, and session metadata coherent during active and recovered WebUI runs. - [`live-to-final-assistant-replies.md`](live-to-final-assistant-replies.md) - — #3400 product model for long-running assistant replies, live process prose, - tool activity, recovery, terminal outcomes, and the final-answer boundary. + — #3400 accepted product model for long-running assistant replies, live + process prose, tool activity, recovery, terminal outcomes, display + projections, and the final-answer boundary. - [`transparent-stream-activity-mode.md`](transparent-stream-activity-mode.md) - — #3820 opt-in display mode for power users who need a transparent, - chronological Thinking / progress / tool-call stream alongside the default - Compact Worklog model. + — #3820 implemented opt-in display mode for power users who need a + transparent, chronological Thinking / progress / tool-call stream alongside + the default Compact Worklog and opt-in Final answer only projections. - [`stable-assistant-turn-anchors.md`](stable-assistant-turn-anchors.md) — #3926 - proposed frontend presentation/reconciliation model for anchoring live - assistant activity, settled final answers, replay, and Compact/Transparent - render modes to one assistant turn. + implemented frontend presentation/reconciliation model for anchoring live + assistant activity, settled final answers, replay, and all activity display + modes to one assistant turn; remaining hardening is tracked under #3400. - [`canonical-session-resolution.md`](canonical-session-resolution.md) — #2361 focused contract for resolving URL, query parameter, localStorage, sidebar, and compression-lineage session IDs to one canonical visible chat target. diff --git a/docs/rfcs/live-to-final-assistant-replies.md b/docs/rfcs/live-to-final-assistant-replies.md index 68e8093d..6564390e 100644 --- a/docs/rfcs/live-to-final-assistant-replies.md +++ b/docs/rfcs/live-to-final-assistant-replies.md @@ -3,6 +3,7 @@ - **Status:** Accepted (parent contract; implementation tracked in [#3400](https://github.com/nesquena/hermes-webui/issues/3400)) - **Author:** @franksong2702 - **Created:** 2026-06-03 +- **Updated:** 2026-07-16 - **Tracking issue:** [#3400](https://github.com/nesquena/hermes-webui/issues/3400) ## Background: Long-Running Sessions Are The Anchor @@ -204,6 +205,43 @@ Requirements: - Very long final answers remain complete and readable. They should not be hidden inside the activity summary or replaced by a progress/status artifact. +### Activity display and disclosure addenda + +The accepted lifecycle now has three presentation strategies over the same +assistant-turn activity data: + +- **Compact Worklog** remains the default. Its top-level Worklog is expanded + while live so progress remains visible, and the user may collapse or reopen it. + Nested tool/reasoning detail remains progressively disclosed. +- **Transparent Stream** is opt-in and renders the same ordered activity as + chronological rows. It does not create a second live or settled owner. +- **Final answer only** is opt-in (`hide_all_activity` in persisted settings). + It suppresses activity rows without deleting the persisted Anchor scene or + changing the final-answer owner. + +For normal completed turns, the settled Compact Worklog remains collapsed by +default. When a terminal error-family turn has actual Worklog content, the +Worklog defaults open so readable partial work is not hidden behind the error +outcome. The current disclosure error family is `error`, `no_response`, +`degraded`, `connection_lost`, `tool_limit_reached`, and +`compression_exhausted`; cancelled turns and the parent `interrupted` terminal +outcome keep their separate semantics. An explicit user disclosure choice wins +over these defaults and may be restored across a render rebuild. + +`degraded` and `connection_lost` are Anchor-level reconstruction/transport +outcomes defined in +[`stable-assistant-turn-anchors.md`](stable-assistant-turn-anchors.md), not +additional canonical product states in the table below. In this parent +contract, `connection_lost` is the transport-specific Anchor form of an +interruption, while `degraded` is the explicit recovery-state counterpart to the +restoring/degraded path. They use error-family disclosure only because they can +leave readable partial Worklog content without a normal final answer. + +These are presentation and disclosure rules only. They do not change reply +ownership, terminal classification, durable transcript truth, or the requirement +that all three strategies converge across live, settle, reload, session switch, +and reconnect. + ### Recovery and replay Refresh, reconnect, session switching, and replay should preserve the same @@ -473,7 +511,9 @@ for current open/merged/superseded status. | Track | Scope | Current vehicle | | --- | --- | --- | | Parent product RFC | Define the long-running live-to-final assistant reply lifecycle and review checklist. | This RFC; tracking issue [#3400](https://github.com/nesquena/hermes-webui/issues/3400). | -| First reply lifecycle implementation | Live process prose, quiet tool activity, settled activity summary above final answer, replay/reattach consistency, live-only compression status, supporting stream ownership fixes. | [#3401](https://github.com/nesquena/hermes-webui/pull/3401). | +| First reply lifecycle implementation | Live process prose, quiet tool activity, settled activity summary above final answer, replay/reattach consistency, live-only compression status, supporting stream ownership fixes. | [#3401](https://github.com/nesquena/hermes-webui/pull/3401), absorbed through [#3741](https://github.com/nesquena/hermes-webui/pull/3741). | +| Activity display projections | Compact Worklog by default, opt-in Transparent Stream, and opt-in Final answer only over the same assistant-turn data. | [#3820](https://github.com/nesquena/hermes-webui/issues/3820), [`transparent-stream-activity-mode.md`](transparent-stream-activity-mode.md), tracking issue [#3400](https://github.com/nesquena/hermes-webui/issues/3400). | +| Assistant-turn presentation ownership | Normalize live, settled, replayed, and recovered activity into one Anchor / `activity_scene_v1`, with legacy rendering limited to historical or non-anchor compatibility. | [#3926](https://github.com/nesquena/hermes-webui/issues/3926), [`stable-assistant-turn-anchors.md`](stable-assistant-turn-anchors.md); core implementation shipped through the #4411/#4564 and #5242/#5243 capstones. | | Terminal/no-final stabilization | Compression exhausted, tool-tail/no-final transcript shape, context-compaction marker suppression, terminal error routing. | [#3315](https://github.com/nesquena/hermes-webui/issues/3315), [#3316](https://github.com/nesquena/hermes-webui/pull/3316). | | Cancel ownership hardening | Frontend cancel should close its own SSE source and clear only its own busy state. | [#3344](https://github.com/nesquena/hermes-webui/issues/3344), [#3345](https://github.com/nesquena/hermes-webui/pull/3345). | | Early-cancel startup race | Backend cancel should still interrupt the worker when the SSE registry detached before startup fully settled. | [#3475](https://github.com/nesquena/hermes-webui/issues/3475), [#3476](https://github.com/nesquena/hermes-webui/pull/3476). | diff --git a/docs/rfcs/stable-assistant-turn-anchors.md b/docs/rfcs/stable-assistant-turn-anchors.md index 7fea9e30..9c522519 100644 --- a/docs/rfcs/stable-assistant-turn-anchors.md +++ b/docs/rfcs/stable-assistant-turn-anchors.md @@ -1,12 +1,35 @@ # Stable Assistant Turn Anchors for Live-to-Final Rendering -- **Status:** Proposed +- **Status:** Implemented - **Author:** @franksong2702 - **Created:** 2026-06-10 +- **Updated:** 2026-07-16 - **Tracking issue:** [#3926](https://github.com/nesquena/hermes-webui/issues/3926) - **Parent contract:** [Live-to-Final Assistant Replies](./live-to-final-assistant-replies.md) ([#3400](https://github.com/nesquena/hermes-webui/issues/3400)) - **Related RFCs:** [Transparent Stream](./transparent-stream-activity-mode.md), [Hermes Run Adapter Contract](./hermes-run-adapter-contract.md), [WebUI Run State Consistency Contract](./webui-run-state-consistency-contract.md), [Turn Journal](./turn-journal.md), [Pending Intent Controls](./webui-pending-intent-controls.md) +## Implementation Status + +The RFC's core presentation/reconciliation path is implemented. Current +`master` includes the event normalizer and registry, settled final-answer +projection, ordered `activity_scene_v1`, live Compact Worklog rendering, +Transparent Stream rendering, run-journal hydration, session re-entry, +transcript-backed scene persistence, and explicit fallback ownership for +historical or non-anchor transcripts. + +`Implemented` does not mean every adjacent hardening item is complete. The +remaining work is tracked under [#3400](https://github.com/nesquena/hermes-webui/issues/3400) +and should land as separate slices: + +- replace presentation-layer text/prefix de-echo heuristics with provenance or + event identity without deleting legitimate repeated model output; +- make newer journal/replay evidence explicitly outrank stale `INFLIGHT` + first-paint state; +- finish stable `run_id` versus transport `stream_id` separation; +- complete the Phase 6 Artifact/side-effect ownership path; +- remove each legacy fallback only after its historical transcript shape can + hydrate an Anchor reliably. + ## Problem The Live-to-Final redesign made the user-facing product model clearer: @@ -763,6 +786,11 @@ Implementation PRs may combine adjacent low-risk phases when they preserve behavior and include coverage. Settlement, reconstruction, and display-mode changes should remain independently reviewable. +Implementation reconciliation (2026-07-16): the core Phase 0-5 path has shipped. +Phase 6 and the hardening items listed in **Implementation Status** remain +follow-up work; the phase descriptions below are retained as the rollout +contract that produced the current implementation. + ### Phase 0: RFC and inventory - Land this RFC as design guidance. @@ -902,6 +930,11 @@ Any implementation PR against this RFC should answer: ## Open Questions +The first-slice and Transparent Stream sequencing questions below are retained +as historical implementation rationale. Current open hardening questions are +identity fallback depth, Artifact/side-effect ownership, and how far active-turn +DOM rebuilds can be reduced without weakening recovery compatibility. + ### First implementation slice size Should the first implementation PR stop at hidden anchor scaffolding, or should diff --git a/docs/rfcs/transparent-stream-activity-mode.md b/docs/rfcs/transparent-stream-activity-mode.md index c844b578..9909c634 100644 --- a/docs/rfcs/transparent-stream-activity-mode.md +++ b/docs/rfcs/transparent-stream-activity-mode.md @@ -1,12 +1,31 @@ # Transparent Stream — A Chronological Activity Display Mode -- **Status:** Accepted (direction confirmed by @nesquena; RFC merged via [#3862](https://github.com/nesquena/hermes-webui/pull/3862)) +- **Status:** Implemented (opt-in display mode; follow-up hardening tracked in [#3400](https://github.com/nesquena/hermes-webui/issues/3400)) - **Author:** @franksong2702 - **Created:** 2026-06-09 -- **Updated:** 2026-06-09 +- **Updated:** 2026-07-16 - **Tracking issue:** [#3820](https://github.com/nesquena/hermes-webui/issues/3820) - **Parent contract:** [Live-to-Final Assistant Replies](./live-to-final-assistant-replies.md) ([#3400](https://github.com/nesquena/hermes-webui/issues/3400) / #3401 / #3741) +## Current projection family + +The shipped `chat_activity_display_mode` setting now has three explicit values: + +- `compact_worklog` — default, results-first presentation; +- `transparent_stream` — opt-in chronological activity rows; +- `hide_all_activity` — opt-in **Final answer only** presentation. + +All three are render strategies over the same Assistant Turn Anchor and +`activity_scene_v1`; they do not create separate activity ownership or change +the final-answer boundary. Final answer only suppresses activity presentation +while preserving the persisted scene so another mode, settle, or reload can +reconstruct the same turn. + +The original proposal and rollout notes below are retained as design history. +Statements describing a missing selector, missing settled branch, or unwired +live/replay renderer describe the tree when this RFC was written, not current +`master`. + ## Problem After the live-to-final redesign (#3401/#3741), the execution trace of a turn @@ -104,21 +123,22 @@ Plus two boundaries: - **Not full transcript virtualization.** Performance hardening for very long transparent transcripts (#3714) is acknowledged but scoped to a later slice. -## Proposal +## Implemented proposal ### 1. A new, dedicated preference -A new preference, independent of `simplified_tool_calling`: +The shipped preference is independent of `simplified_tool_calling`: ```python # api/config.py preferences -"chat_activity_display_mode": "compact_worklog", # | "transparent_stream" +"chat_activity_display_mode": "compact_worklog", # | "transparent_stream" | "hide_all_activity" ``` - Default `compact_worklog` — existing users see no change. - Surfaced in Settings as an explicit **segmented choice** (not a checkbox): - **Compact Worklog** — default; results-first, supporting activity quiet. - **Transparent Stream** — advanced; every step shown chronologically. + - **Final answer only** — activity hidden; final answer remains visible. - The legacy `Compact tool activity` checkbox is marked deprecated. It is not the seam for this feature. @@ -128,6 +148,7 @@ predicate used everywhere: ```js function chatActivityMode(){ return window._chatActivityDisplayMode || 'compact_worklog'; } function isTransparentStream(){ return chatActivityMode() === 'transparent_stream'; } +function isFinalAnswerOnlyMode(){ return chatActivityMode() === 'hide_all_activity'; } ``` ### 2. Decouple "event sequence" from "grouping strategy" diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index 6a3c839f..a14bc520 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -3,8 +3,9 @@ - **Status:** Proposed - **Author:** @franksong2702 - **Created:** 2026-05-16 +- **Updated:** 2026-07-16 - **Tracking issue:** [#2361](https://github.com/nesquena/hermes-webui/issues/2361) -- **Related architecture:** [#1925](https://github.com/nesquena/hermes-webui/issues/1925), [`hermes-run-adapter-contract.md`](hermes-run-adapter-contract.md) +- **Related architecture:** [#1925](https://github.com/nesquena/hermes-webui/issues/1925), [`hermes-run-adapter-contract.md`](hermes-run-adapter-contract.md), [`stable-assistant-turn-anchors.md`](stable-assistant-turn-anchors.md) ## Problem @@ -46,6 +47,21 @@ while WebUI still has multiple overlapping state stores. - Do not reopen already-fixed narrow bugs. - Do not make this a catch-all for unrelated UI polish. +## Current implementation relationship + +Stable Assistant Turn Anchors now implement the presentation/reconciliation +portion of this contract for one assistant turn. The run journal and settled +transcript provide durable observations; the Anchor registry and +`activity_scene_v1` reconcile those observations into Compact Worklog, +Transparent Stream, or Final answer only; `S.messages`, `INFLIGHT`, renderer +caches, and DOM remain projections or recovery caches rather than independent +semantic owners. + +This RFC remains `Proposed` because its broader cross-layer contract also covers +model-context reconstruction, compression handoff, session metadata, and future +runtime-adapter migration. Shipped Anchor coverage strengthens invariants 2, 3, +and 5; it does not mark every run-state boundary implemented. + ## State Layers | Layer | Purpose | Source-of-truth expectation | Must not do | diff --git a/static/assistant_turn_anchors.js b/static/assistant_turn_anchors.js index 7c11ca1f..17c69ba0 100644 --- a/static/assistant_turn_anchors.js +++ b/static/assistant_turn_anchors.js @@ -1,9 +1,9 @@ -// Stable Assistant Turn Anchors scaffold (#3926). +// Stable Assistant Turn Anchors implementation (#3926 / #3400). // -// This file defines the current ownership inventory, event classifications, and -// small owner helpers. It does not register anchors globally. The only renderer -// wiring in this slice is settled assistant final-answer projection; live -// streaming, replay hydration, tools, and DOM ownership remain unwired. +// This file defines the ownership inventory, event normalization, registry, and +// activity-scene projection consumed by current live, settled, replay-hydration, +// and session-reentry paths. Runtime execution and transport remain outside the +// Anchor's presentation/reconciliation ownership boundary. (function(){ const ROOT=(typeof window!=='undefined')?window:globalThis; diff --git a/tests/test_cli_sessions_cache_cap.py b/tests/test_cli_sessions_cache_cap.py new file mode 100644 index 00000000..7d2af868 --- /dev/null +++ b/tests/test_cli_sessions_cache_cap.py @@ -0,0 +1,89 @@ +"""Tests: ``_CLI_SESSIONS_CACHE`` is LRU-bounded (drop-oldest). + +Regression: the CLI/cron session projection cache was a plain ``dict`` with TTL +storage but NO size cap and only lazy, same-key eviction. Each value is a +``copy.deepcopy()`` of the full CLI/cron session list (the expensive projection +behind #4842/#4672). The cache key folds in a state.db content fingerprint that +advances on every streamed message, so the next poll builds a NEW key; the +previous key's heavy deepcopy was never looked up again, so its lazy-expiry path +never ran. A long-lived process under churn could accumulate orphaned heavy +deepcopies until the next structural ``clear_cli_sessions_cache()``. + +The cache is now an ``OrderedDict`` capped at ``_CLI_SESSIONS_CACHE_MAX_ENTRIES`` +with drop-oldest on write (mirroring ``_CLAUDE_CODE_PARSE_CACHE``) and +``move_to_end`` on fresh hits. TTL remains the primary freshness control; the +cap is the backstop the plain dict lacked. +""" +from __future__ import annotations + +import api.models as models +from api.models import ( + _CLI_SESSIONS_CACHE_MAX_ENTRIES, + _cache_cli_sessions_if_current, + _cli_sessions_cache_invalidation_stamp, + _copy_fresh_cli_sessions_cache_entry, +) + + +def _reset_cache(): + with models._CLI_SESSIONS_CACHE_LOCK: + models._CLI_SESSIONS_CACHE.clear() + + +def test_cache_is_bounded_drops_oldest_on_write(): + """Inserting more than MAX_ENTRIES distinct keys keeps len == MAX and evicts + the OLDEST (least-recently-used) entry.""" + _reset_cache() + stamp = _cli_sessions_cache_invalidation_stamp() + cap = _CLI_SESSIONS_CACHE_MAX_ENTRIES + assert cap >= 1 + + # Insert cap + 5 distinct keys, each a distinct payload so we can track them. + for i in range(cap + 5): + key = (f"profile-{i}",) + ok = _cache_cli_sessions_if_current( + key, ttl=60.0, invalidation_stamp=stamp, sessions=[{"id": i}] + ) + assert ok + + with models._CLI_SESSIONS_CACHE_LOCK: + assert len(models._CLI_SESSIONS_CACHE) == cap + keys = list(models._CLI_SESSIONS_CACHE.keys()) + # The oldest 5 (profile-0..4) were evicted; the newest `cap` remain. + assert keys[0] == ("profile-5",) + assert keys[-1] == (f"profile-{cap + 5 - 1}",) + assert ("profile-0",) not in keys + + +def test_fresh_hit_marks_entry_most_recently_used(): + """A fresh cache hit moves the entry to the end (most-recently-used), so a + frequently-read entry is NOT evicted over colder ones.""" + _reset_cache() + stamp = _cli_sessions_cache_invalidation_stamp() + cap = _CLI_SESSIONS_CACHE_MAX_ENTRIES + + # Fill the cache exactly. + for i in range(cap): + _cache_cli_sessions_if_current( + (f"k{i}",), ttl=60.0, invalidation_stamp=stamp, sessions=[{"id": i}] + ) + # Touch k0 (the oldest) so it becomes most-recently-used. + served = _copy_fresh_cli_sessions_cache_entry(("k0",)) + assert served == [{"id": 0}] + # Now insert one more; the eviction victim should be k1, NOT k0. + _cache_cli_sessions_if_current( + ("k_new",), ttl=60.0, invalidation_stamp=stamp, sessions=[{"id": 999}] + ) + with models._CLI_SESSIONS_CACHE_LOCK: + keys = list(models._CLI_SESSIONS_CACHE.keys()) + assert ("k0",) in keys # survived because it was just used + assert ("k1",) not in keys # evicted as the new oldest + assert ("k_new",) in keys + + +def test_cache_is_ordered_dict_not_plain_dict(): + """The regression: the cache used to be a plain dict with no ordering. It + must now be an OrderedDict to support move_to_end / popitem(last=False).""" + import collections + + assert isinstance(models._CLI_SESSIONS_CACHE, collections.OrderedDict) diff --git a/tests/test_v050259_sessiondb_fd_leak.py b/tests/test_v050259_sessiondb_fd_leak.py index 3b80ac97..12bfd90e 100644 --- a/tests/test_v050259_sessiondb_fd_leak.py +++ b/tests/test_v050259_sessiondb_fd_leak.py @@ -1,18 +1,25 @@ -"""Regression tests for v0.50.259 — SessionDB FD leak fixes from PR #1421 -plus Opus pre-release advisor follow-up extending the fix to LRU eviction. +"""Regression tests for SessionDB FD-leak fixes (PR #1421) plus the +subagent shared-handle race (close-under-live-subagents). -The bug: `_run_agent_streaming` created a new `SessionDB` per request and +History +------- +PR #1421: `_run_agent_streaming` created a new `SessionDB` per request and replaced the cached agent's `_session_db` without closing the old one. After ~73 messages on a long-lived agent, leaked FDs exhausted the 256 FD -default limit causing `EMFILE` crashes. +default limit causing `EMFILE` crashes. Fix: close the previous handle +when it is safe to replace it. -Fix path 1 (PR #1421 by @wali-reheman): close `agent._session_db` before -replacing it on the cached-agent reuse path. +Follow-up (this change): always-close-before-replace is *not* safe when +background subagents still hold a reference to the same SessionDB object +(delegate_tool copies ``parent._session_db`` by ref). A server-side wakeup +/ new turn for the parent session was closing the shared handle mid-child- +run, producing: -Fix path 2 (this PR Opus follow-up): same shape on the LRU eviction path. -When `SESSION_AGENT_CACHE.popitem(last=False)` evicts an old agent, its -`_session_db` is dropped on the floor and only released when GC eventually -finalizes the agent — which on a long-running server may be never. + Session DB append_message failed: 'NoneType' object has no attribute 'execute' + +Policy now (``_adopt_session_db_for_cached_agent``): +- existing handle still open → keep it, close the unused *new* handle +- existing handle missing/closed → adopt the new handle (close dead one) """ from __future__ import annotations @@ -25,38 +32,56 @@ import pytest REPO = Path(__file__).resolve().parents[1] -# ── 1: source-level pin: cached-agent-reuse path closes _session_db ───────── +# ── 1: source-level pin: cached-agent reuse uses the adopt helper ────────── -def test_cached_agent_reuse_closes_old_session_db(): - """The cached-agent reuse path in `_run_agent_streaming` MUST close the - old `_session_db` before replacing it. Without this, every streaming - request leaks a SessionDB connection (3 FDs once WAL is active).""" +def test_cached_agent_reuse_uses_adopt_helper(): + """Cached-agent reuse must go through `_adopt_session_db_for_cached_agent` + so a still-open SessionDB is reused (subagent-safe) and only a dead handle + is closed+replaced (still EMFILE-safe).""" src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8") - # Find the cached-agent reuse block (where callbacks are refreshed). - # The replacement of agent._session_db must be preceded by a close(). reuse_idx = src.find("Refresh per-turn callbacks") assert reuse_idx != -1, "cached-agent reuse block missing" block = src[reuse_idx : reuse_idx + 2500] - # Must close before replace. - assert "agent._session_db.close()" in block, ( - "cached-agent reuse path must call agent._session_db.close() before " - "replacing it. Without this, FDs leak on every streaming request " - "and the server EMFILE-crashes after ~73 messages. See PR #1421." + assert "_adopt_session_db_for_cached_agent" in block, ( + "cached-agent reuse path must call _adopt_session_db_for_cached_agent " + "instead of unconditionally closing agent._session_db. Unconditional " + "close breaks background subagents that share the handle by reference." ) - # And the close must come BEFORE the replacement (lexically). - close_idx = block.find("agent._session_db.close()") - replace_idx = block.find("agent._session_db = _session_db") - assert close_idx != -1 and replace_idx != -1 - assert close_idx < replace_idx, ( - "close() must lexically precede the assignment so the old connection " - "is released before the reference is rebound." + assert "agent._session_db = _session_db" in block, ( + "reuse path must still assign the adopted SessionDB onto the agent" + ) + # The old unconditional-close pattern must not remain in the reuse block. + assert "agent._session_db.close()" not in block, ( + "unconditional agent._session_db.close() in the reuse path is the " + "subagent race; close is now owned by _adopt_session_db_for_cached_agent" ) -# ── 2: source-level pin: LRU eviction path also closes _session_db ────────── +def test_adopt_and_is_open_helpers_exist(): + src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8") + assert "def _session_db_is_open(" in src + assert "def _adopt_session_db_for_cached_agent(" in src + # self-heal path must also refuse to close a still-open handle + replace_idx = src.find("def _replace_session_db_in_kwargs") + assert replace_idx != -1 + block = src[replace_idx : replace_idx + 1200] + assert "_session_db_is_open" in block, ( + "_replace_session_db_in_kwargs must guard on _session_db_is_open so " + "credential self-heal cannot close a handle live subagents share" + ) + # adopt helper must log failed closes (not bare `pass`) so EMFILE pressure + # from a failed close is diagnosable — matches _replace_session_db_in_kwargs. + adopt_idx = src.find("def _adopt_session_db_for_cached_agent") + assert adopt_idx != -1 + adopt_block = src[adopt_idx : adopt_idx + 1800] + assert 'Failed to close unused session_db handle in adopt helper' in adopt_block + assert "logger.debug" in adopt_block + + +# ── 2: source-level pin: LRU eviction path still closes _session_db ──────── def test_lru_eviction_closes_evicted_agent_session_db(): @@ -66,27 +91,21 @@ def test_lru_eviction_closes_evicted_agent_session_db(): on GC finalization which may never run on a long-lived server. Fix: capture the evicted entry, close its agent's `_session_db` before - dropping the reference. + dropping the reference. (Eviction is a true session boundary — no live + subagents are expected to still be writing into that agent.) """ src = (REPO / "api" / "streaming.py").read_text(encoding="utf-8") - # The eviction site uses popitem(last=False). The evicted entry must be - # captured (not discarded with `_`) and the agent's _session_db closed. eviction_idx = src.find("Evicted LRU agent from cache") assert eviction_idx != -1, "LRU eviction debug log missing" - # Look in a window around the eviction. block = src[max(0, eviction_idx - 1500) : eviction_idx + 200] - # Negative pattern: the old `evicted_sid, _ = ...` discard form must NOT - # be present — that's the bug shape. assert "evicted_sid, _ = SESSION_AGENT_CACHE.popitem" not in block, ( - "LRU eviction must capture the evicted entry so the agent\'s " + "LRU eviction must capture the evicted entry so the agent's " "_session_db can be closed. The `evicted_sid, _ = ...` discard form " "is the original bug shape." ) - # Positive pattern: eviction must call the lifecycle helper, and the helper - # must close the evicted agent's SessionDB after provider teardown. assert "_close_evicted_agent_at_session_boundary(_evicted_sid, _evicted_agent)" in block, ( "LRU eviction must route the evicted agent through the session-boundary " "close helper." @@ -104,16 +123,7 @@ def test_lru_eviction_closes_evicted_agent_session_db(): def test_session_db_close_is_idempotent(): - """`SessionDB.close()` must be safe to call multiple times. The fix - relies on this — if a future code path closes the same `_session_db` - after we've swapped it, the second close is a benign no-op. - - Skipped when hermes_state is not on the import path (e.g. on the GH - Actions runner that only has the WebUI repo, not the agent repo). - The source-level pin in test_cached_agent_reuse_closes_old_session_db - catches revert of the close() call; this test only adds runtime - confirmation when both repos are co-located. - """ + """`SessionDB.close()` must be safe to call multiple times.""" import importlib.util if importlib.util.find_spec("hermes_state") is None: pytest.skip("hermes_state not on import path (CI-only — agent repo not present)") @@ -123,65 +133,123 @@ def test_session_db_close_is_idempotent(): with tempfile.TemporaryDirectory() as tmpd: db_path = Path(tmpd) / "test.db" db = SessionDB(db_path=db_path) - # Force connection open by issuing a query. with db._lock: db._conn.execute("SELECT 1") - # First close db.close() assert db._conn is None - # Second close — must not raise. db.close() assert db._conn is None - # Third close — still safe. db.close() -# ── 4: behavioral: cached-agent reuse with mock agent ─────────────────────── +# ── 4: behavioral: adopt helper keeps open handles, closes dead ones ─────── -def test_cached_agent_reuse_calls_close_on_old_session_db(): - """End-to-end: simulate the cached-agent reuse code path with a mock - agent and verify the mock SessionDB.close() is called when _session_db - is replaced. Pins the runtime behavior, not just the source pattern.""" - import sys - sys.path.insert(0, str(REPO)) +class _MockSessionDB: + def __init__(self, name, open_=True): + self.name = name + self.close_calls = 0 + # Mirror SessionDB: open → _conn is truthy; closed → _conn is None. + self._conn = object() if open_ else None - class MockSessionDB: - def __init__(self, name): - self.name = name - self.close_calls = 0 - def close(self): - self.close_calls += 1 + def close(self): + self.close_calls += 1 + self._conn = None - class MockAgent: - def __init__(self, db): - self._session_db = db - self.stream_delta_callback = None - self.tool_progress_callback = None - self._api_call_count = 0 - self._interrupted = False - self._interrupt_message = None - # Simulate the inner block of the cached-agent reuse path. We replicate - # the pattern manually rather than importing _run_agent_streaming - # directly because that function has many other side effects. - old_db = MockSessionDB("old") - new_db = MockSessionDB("new") - agent = MockAgent(old_db) +class _MockAgent: + def __init__(self, db): + self._session_db = db + self.stream_delta_callback = None + self.tool_progress_callback = None + self._api_call_count = 0 + self._interrupted = False + self._interrupt_message = None - # Mirror the production code path: - if hasattr(agent, "_session_db") and agent._session_db is not None: - try: - agent._session_db.close() - except Exception: - pass - agent._session_db = new_db - assert old_db.close_calls == 1, ( - "old SessionDB must be closed exactly once when replaced on cached agent" - ) - assert new_db.close_calls == 0, "new SessionDB should not be closed" +def _import_adopt_helpers(): + """Import the production helpers. + + No source-slicing / exec fallback: that path breaks as soon as the helpers + reference module-level names (e.g. ``logger.debug``) or another def is + inserted between the markers. Prefer a real import; skip the behavioral + suite when the package cannot be imported (CI without full deps). + Source-level pins above still catch reverts without importing streaming. + """ + try: + from api.streaming import ( # type: ignore + _adopt_session_db_for_cached_agent, + _session_db_is_open, + ) + except Exception as exc: + pytest.skip(f"api.streaming helpers not importable: {exc}") + return _session_db_is_open, _adopt_session_db_for_cached_agent + + +def test_adopt_reuses_open_session_db_and_closes_new(): + """Live (open) existing handle must be kept; unused new handle closed. + + This is the subagent-safe path: children hold a reference to `old_db`. + """ + _is_open, adopt = _import_adopt_helpers() + old_db = _MockSessionDB("old", open_=True) + new_db = _MockSessionDB("new", open_=True) + agent = _MockAgent(old_db) + + result = adopt(agent, new_db) + + assert result is old_db + assert agent._session_db is old_db + assert old_db.close_calls == 0, "must not close the live shared handle" + assert new_db.close_calls == 1, "unused per-request handle must be closed (FD leak)" + assert _is_open(old_db) is True + assert _is_open(new_db) is False + + +def test_adopt_replaces_closed_session_db(): + """Dead existing handle is closed (idempotent) and replaced with the new one.""" + _is_open, adopt = _import_adopt_helpers() + old_db = _MockSessionDB("old", open_=False) + new_db = _MockSessionDB("new", open_=True) + agent = _MockAgent(old_db) + + result = adopt(agent, new_db) + + assert result is new_db assert agent._session_db is new_db + assert old_db.close_calls == 1 + assert new_db.close_calls == 0 + + +def test_adopt_handles_missing_existing(): + _is_open, adopt = _import_adopt_helpers() + new_db = _MockSessionDB("new", open_=True) + agent = _MockAgent(None) + agent._session_db = None + + result = adopt(agent, new_db) + + assert result is new_db + assert agent._session_db is new_db + assert new_db.close_calls == 0 + + +def test_cached_agent_reuse_calls_adopt_semantics(): + """End-to-end mirror of the production reuse block using the real helper.""" + _is_open, adopt = _import_adopt_helpers() + old_db = _MockSessionDB("old", open_=True) + new_db = _MockSessionDB("new", open_=True) + agent = _MockAgent(old_db) + _session_db = new_db + + # Mirror production: + if _session_db is not None: + _session_db = adopt(agent, _session_db) + agent._session_db = _session_db + + assert agent._session_db is old_db + assert old_db.close_calls == 0 + assert new_db.close_calls == 1 # ── 5: behavioral: LRU eviction with mock agents ──────────────────────────── @@ -192,24 +260,12 @@ def test_lru_eviction_closes_evicted_session_db(): SessionDB.close() is called.""" import collections - class MockSessionDB: - def __init__(self, name): - self.name = name - self.close_calls = 0 - def close(self): - self.close_calls += 1 - - class MockAgent: - def __init__(self, db): - self._session_db = db - cache = collections.OrderedDict() - db1, db2, db3 = MockSessionDB("a"), MockSessionDB("b"), MockSessionDB("c") - cache["sid-a"] = (MockAgent(db1), "sig1") - cache["sid-b"] = (MockAgent(db2), "sig2") - cache["sid-c"] = (MockAgent(db3), "sig3") + db1, db2, db3 = _MockSessionDB("a"), _MockSessionDB("b"), _MockSessionDB("c") + cache["sid-a"] = (_MockAgent(db1), "sig1") + cache["sid-b"] = (_MockAgent(db2), "sig2") + cache["sid-c"] = (_MockAgent(db3), "sig3") - # Mirror the production eviction path with MAX=2: MAX = 2 while len(cache) > MAX: evicted_sid, evicted_entry = cache.popitem(last=False) @@ -220,8 +276,59 @@ def test_lru_eviction_closes_evicted_session_db(): except Exception: pass - # First-inserted entry (sid-a) was evicted. assert "sid-a" not in cache assert db1.close_calls == 1, "evicted agent's SessionDB must be closed exactly once" assert db2.close_calls == 0, "remaining agents' SessionDBs must not be touched" assert db3.close_calls == 0 + + +# ── 6: self-heal path must not reuse a CLOSED handle when the rebuild fails ── + + +def _import_replace_helper(): + """Import the real credential-self-heal SessionDB replacer.""" + try: + from api.streaming import _replace_session_db_in_kwargs # type: ignore + except Exception as exc: + pytest.skip(f"api.streaming not importable: {exc}") + return _replace_session_db_in_kwargs + + +def test_replace_degrades_to_none_when_rebuild_fails_and_old_is_closed(monkeypatch): + """Credential self-heal regression (Codex gate finding on PR #6143). + + When ``_build_session_db_for_stream`` returns None (rebuild failed) AND the + prior handle is already CLOSED, ``_replace_session_db_in_kwargs`` must leave + ``agent_kwargs['session_db'] = None`` — as master did — so the rebuilt agent + lazily reinitialises. Retaining the closed handle (the pre-fix behaviour) + makes every persist/search fail with + ``'NoneType' object has no attribute 'execute'`` while the chat continues. + """ + import api.streaming as streaming + + _replace = _import_replace_helper() + monkeypatch.setattr(streaming, "_build_session_db_for_stream", lambda _p: None) + + old_db = _MockSessionDB("old", open_=False) # already closed + kwargs = {"session_db": old_db} + result = _replace(kwargs, "/tmp/does-not-matter.db") + + assert result is None, "must not hand back a closed handle when rebuild fails" + assert kwargs["session_db"] is None, "kwargs must degrade to None (clean lazy reinit)" + + +def test_replace_keeps_open_handle_when_rebuild_fails(monkeypatch): + """Inverse: a still-OPEN prior handle (held by live subagents) is retained + when the rebuild fails — do not orphan a live shared connection.""" + import api.streaming as streaming + + _replace = _import_replace_helper() + monkeypatch.setattr(streaming, "_build_session_db_for_stream", lambda _p: None) + + old_db = _MockSessionDB("old", open_=True) # still live (subagents hold it) + kwargs = {"session_db": old_db} + result = _replace(kwargs, "/tmp/does-not-matter.db") + + assert result is old_db, "a live handle must be kept when the rebuild fails" + assert kwargs["session_db"] is old_db + assert old_db.close_calls == 0, "must not close a live shared handle"