Merge remote-tracking branch 'origin/master' into pr6092-gate
This commit is contained in:
commit
ea3cba3823
13 files changed
+578
-150
No files matched your search
@@ -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)
|
||||
|
||||
+17
-1
@@ -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)
|
||||
|
||||
+97
-10
@@ -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
|
||||
|
||||
+10
-4
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
+9
-8
@@ -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.
|
||||
|
||||
@@ -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). |
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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 |
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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)
|
||||
@@ -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"
|
||||
Reference in new issue
Block a user