Reviewer regression (reproduced): _STATE_DB_DISPLAY_ROW_BACKSTOP (newest-50k SQL
cap) was applied UNCONDITIONALLY to the GET /api/session display path's state.db
read. But merge_session_messages_append_only needs the rows at/around the
session's truncation_boundary (the preserved prefix) to reconcile correctly. A
newest-N-only cap drops those boundary rows for a >N-row session and corrupts
the merge (silently losing the preserved prefix), and also prevents older-page
(msg_before) requests from ever reaching them. So a pathological-but-real >50k-
row compressed session got a corrupted merge / unreachable older history rather
than a clean bound.
Fix: gate the backstop on the SAME conditions
_state_db_since_timestamp_for_limited_display uses to decide a read is
boundary-free — not msg_before paging, and no truncation_watermark /
truncation_boundary. Those cases stay on the full (uncapped) read; the backstop
is now a defensive net for the common uncompressed initial-tail path only.
Extracted into _state_db_backstop_limit_for_display for direct test coverage.
The get_state_db_session_messages(limit=) signature + newest-rows subquery are
unchanged — only the display-path APPLICATION of the cap is gated.
Test: tests/test_state_db_backstop_boundary_gate.py — backstop applies with no
boundary prefix; SKIPPED when truncation_boundary / truncation_watermark set or
msg_before paging; empty-string markers treated as unset (matching the existing
helper). Existing backstop + reconciliation suites (53 tests) unchanged.
Model Used: builtin:zai-coding-plan/GLM-5.2 (ZCode agent).
Reviewer regression (reproduced): the broadcast drop-oldest loop did
put_nowait -> on Full, get_nowait -> on Empty, break. But Empty here means a
concurrent consumer (the SSE handler thread) DRAINED the queue between our Full
and get_nowait — the queue now has space, which is exactly the condition to
retry the put. `break` silently discarded `item`, and when `item` was a terminal
frame (stream_end/error/cancel) the subscriber never received it, leaving the
client attached indefinitely (spinner-forever). The "# cap 0 defensive guard;
never reached" comment was the flawed assumption: any concurrent drain makes
the Empty branch reachable at any cap.
Fix: on queue.Empty, `continue` (retry the put into the now-empty queue) in
both the broadcast path and the replay path (for consistency; the replay path
runs under self._lock with a fresh queue so it's unreachable in practice, but
the same race-safe rule applies). No infinite-loop risk: each iteration either
succeeds a put (break) or succeeds a get (progress); the continue only fires on
Empty, after which the next put has space.
Test: test_terminal_frame_survives_concurrent_drain_during_drop_oldest
deterministically simulates the interleaving with a custom queue whose first
post-Full get_nowait raises Empty (as if a concurrent consumer drained it),
then runs BOTH the pre-fix `break` logic (drops the terminal frame — proving
the test bites) and the post-fix `continue` logic (delivers it).
Model Used: builtin:zai-coding-plan/GLM-5.2 (ZCode agent).
Codex gate on #6143 found a SILENT regression the reuse-path fix introduced:
_replace_session_db_in_kwargs early-returned the OLD handle when the rebuild
failed, but master assigned None. If the old handle was already CLOSED, both
credential self-heal callers (streaming.py:9032/10253) then built a fresh agent
around a closed SessionDB -> every persist/search fails with 'NoneType has no
attribute execute' while the chat keeps going. Guard on _session_db_is_open:
keep a live handle (subagents hold it by ref), else degrade to None (clean lazy
reinit, master behaviour). +2 regression tests.
Co-authored-by: carlotestor <carlotestor@users.noreply.github.com>
Memory: get_state_db_session_messages had no SQL LIMIT — it full-scanned the
state.db messages table and fetchall()'d into a Python list. The display path
(GET /api/session) already has a careful since_timestamp optimization that
bounds the common uncompressed tail load, but it deliberately bails (returns
None) on compressed sessions (truncation_watermark/boundary) and msg_before
paging — in those cases the read used to full-scan with no cap. A
pathological/huge state.db could then materialize unbounded rows into memory.
Add an opt-in `limit` parameter to get_state_db_session_messages (applied as a
SQL LIMIT that keeps the NEWEST rows via a DESC subquery re-sorted ASC). It is
a BACKSTOP, not a semantic window: the display window counts visible rows
post-reconciliation, so a true window LIMIT here would corrupt the
sidecar/state.db merge (see _state_db_since_timestamp_for_limited_display,
which deliberately does NOT SQL-LIMIT raw rows for exactly that reason).
Thread a generous _STATE_DB_DISPLAY_ROW_BACKSTOP (50000) from the GET /api/session
display caller only. The full-history model-context callers (reconciliation at
models.py:3437, context reconstruction at :8576, new-turn context at
streaming.py:8443) do NOT pass a limit — their semantics require the full
history, unchanged.
Test: tests/test_state_db_read_backstop.py — no-limit returns full history
oldest-first (model-context contract unchanged); limit=N keeps the newest N
re-sorted oldest-first; limit > row count returns all; limit<=0 clamps to 1
(newest); invalid limit falls back to unbounded; limit composes with
since_timestamp. Existing state.db active-filter + reconciliation suites (54
tests) unchanged.
Contract Routing
- Task type: memory-bound fix (unbounded state.db row materialization on the
display path's bail cases).
- Touched areas: state.db message reader (api/models.py) + GET /api/session
display caller (api/routes.py). NOT the model-context callers.
- Relevant public docs: AGENTS.md, CONTRIBUTING.md, docs/CONTRACTS.md,
docs/rfcs/webui-run-state-consistency-contract.md (Model context layer).
- State layer mutated: none (read-only). The model-context read path is
UNCHANGED (no limit passed) — Invariant 1 (visible current turns enter model
context) and the reconciliation contract are preserved. Only the display path
gets the defensive cap, which does not affect what the agent receives.
- Scope boundaries: this is narrower than a naive "add LIMIT everywhere" —
_state_db_since_timestamp_for_limited_display's docstring explains why a
semantic window LIMIT would corrupt the merge; this cap is purely a
pathological-input safety net above any legitimate session size.
Release note: GET /api/session's state.db read now has a defensive row backstop,
capping memory when the bounded since_timestamp optimization bails on
compressed/paged sessions.
Model Used: builtin:zai-coding-plan/GLM-5.2 (ZCode agent).
- Log failed closes in _adopt_session_db_for_cached_agent with
logger.debug(..., exc_info=True) instead of bare pass, matching
_replace_session_db_in_kwargs so a failed close (EMFILE risk) is
diagnosable.
- Drop the fragile exec/source-slice fallback in the test helper import;
real import or pytest.skip. Source-level pins still cover reverts
without importing streaming.
Background subagents (delegate_tool) capture parent_agent._session_db by
reference. The cached-agent reuse path created a fresh SessionDB every
stream request and unconditionally closed the previous handle (PR #1421
EMFILE fix). A server-side wakeup / new turn for the same parent session
could therefore close the shared handle while children were still
flushing messages, producing:
Session DB append_message failed: 'NoneType' object has no attribute 'execute'
Reproduce (observed in production):
1. Parent WebUI session runs delegate_task with N background subagents
2. Parent turn tears down / idle-hook redelivers a deferred wakeup
3. New stream for the parent adopts a fresh SessionDB and closes the old one
4. Live subagents' append_message hits conn=None
Fix: _adopt_session_db_for_cached_agent reuses a still-open handle (and
closes the unused new one — still EMFILE-safe) and only replaces when
the existing handle is missing or already closed. Same guard applied to
_replace_session_db_in_kwargs (credential self-heal). LRU eviction still
closes at true session boundaries.
Tests updated to pin the adopt helper and the subagent-safe semantics.
Frank Song