diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c5fe894..d8ae2aa1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ ### Fixed +- **The frontend now reads the `msg_limit` ceiling from `/api/session` metadata instead of hand-mirroring the constant.** Following the ceiling clamp shipped in exp-v0.52.98, the backend advertises its `_MAX_MSG_LIMIT` as `_msg_limit_max` in every `/api/session` response, and the frontend reads it dynamically (`_msgLimitMax`, a module-scope value defaulting to the static fallback), so the two can no longer drift and the mirrored constant's drift-guard test is no longer needed. Older backends that omit the field fall back to the built-in default, so mixed-version deployments are unaffected. Thanks @webtecnica. (#6214, #6177) + - **`GET /api/session?msg_limit=` is now bounded by a server-side ceiling, and the frontend paginates around it without dropping rows.** A client could request `msg_limit=1000000` (or an outline-jump path asked for `9999`) and force the server to assemble and serialize an unbounded message payload. The backend now clamps `?msg_limit=` to `[1, 500]` via a dedicated `_parse_msg_limit()` helper and sets the existing `_messages_truncated` signal when it clamps; the bare no-`msg_limit` path (branch/undo/jump-to-start) still returns the full transcript. The frontend mirrors the ceiling: `_loadOlderMessages` grows its tail window below the ceiling and switches to bounded `msg_before` backward paging once the server would clamp, and the outline jump uses the bare full-transcript path instead of the old `msg_limit=9999` hack. Two silent row-loss regressions found during review were fixed before ship: a raw-row-heavy `msg_before` page that textually repeated the current tail could be misclassified as the cumulative tail and wholesale-replace it (losing older rows), and a same-session refresh above the ceiling could shrink an already-loaded >500-row transcript to the last 500 — both now route through the correct paging/full-transcript path. Thanks @rh-id. (#6152, #6154, #6177) - **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) diff --git a/api/routes.py b/api/routes.py index 4d1c6776..9f55a661 100644 --- a/api/routes.py +++ b/api/routes.py @@ -12769,6 +12769,7 @@ def handle_get(handler, parsed) -> bool: _truncated = load_messages and msg_limit is not None and _messages_offset > 0 raw["_messages_truncated"] = _truncated raw["_messages_offset"] = _messages_offset + raw["_msg_limit_max"] = _MAX_MSG_LIMIT _t4 = _time.monotonic() if _diag: _diag.stage("t4_after_compact_and_merge") if effective_model: diff --git a/static/sessions.js b/static/sessions.js index 5ec423bc..8bacdb90 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -2935,12 +2935,17 @@ const _INITIAL_MSG_LIMIT = 30; // ceiling. _loadOlderMessages grows its msg_limit tail window by // +_INITIAL_MSG_LIMIT each load; once growth would exceed this ceiling the // server clamps and the tail stops growing, so we switch to msg_before paging -// (a fixed-size backward page keyed off _oldestIdx) instead. If you change the -// backend _MAX_MSG_LIMIT, change this value to match — otherwise load-older -// silently stalls again for long sessions. The durable fix (so this mirror -// isn't needed) is to expose the ceiling in the /api/session metadata response -// and read it here; see tracking issue for that follow-up. +// (a fixed-size backward page keyed off _oldestIdx) instead. This const is the +// static FALLBACK default only — the live ceiling is read from the /api/session +// `_msg_limit_max` metadata into _msgLimitMax below (#6177), so the two can no +// longer drift. Keep this fallback value roughly in sync with the backend +// _MAX_MSG_LIMIT for the mixed-version case where the server omits the field. const _MSG_LIMIT_MAX = 500; +// Live server-advertised msg_limit ceiling. Declared at module scope with the +// static fallback so the reload-width paths (_ensureMessagesLoaded / +// _loadOlderMessages) always read a defined value even before the first +// /api/session response lands; refreshed from `_msg_limit_max` on each load. +let _msgLimitMax = _MSG_LIMIT_MAX; let _sameSessionForceReloadHint = null; function _currentLoadedRenderableMessageCount(){ @@ -3052,7 +3057,7 @@ async function _ensureMessagesLoaded(sid, opts) { // window exceeds the ceiling, fall back to the bare full-transcript request // (no msg_limit / no expand_renderable) so a same-session refresh never drops // already-loaded older rows (Codex gate #6154, silent row-loss). - const boundedReloadLimit = (reloadLimit && reloadLimit <= _MSG_LIMIT_MAX) ? reloadLimit : null; + const boundedReloadLimit = (reloadLimit && reloadLimit <= _msgLimitMax) ? reloadLimit : null; const reloadLimitParam = boundedReloadLimit ? `&msg_limit=${boundedReloadLimit}` : ''; // Older frontends used expand_renderable=1 to request visible-row expansion. // The server now counts msg_limit by visible transcript rows by default; keep @@ -3072,6 +3077,7 @@ async function _ensureMessagesLoaded(sid, opts) { if (!data || !data.session) return; _messagesTruncated = !!data.session._messages_truncated; _oldestIdx = data.session._messages_offset || 0; + _msgLimitMax = data.session._msg_limit_max || _MSG_LIMIT_MAX; // #3162: `msgs` is reassigned below by the #3018 ephemeral-field carry-forward, // so it must be `let`, not `const`. The `const` form threw a TypeError inside // _ensureMessagesLoaded() that surfaced as a "Failed to load conversation messages" @@ -3570,7 +3576,7 @@ async function _loadOlderMessages() { // arbitrarily long transcripts. (This is the same paging request the // race-fallback below uses, proven correct there.) const requestedLimit = Math.max(_INITIAL_MSG_LIMIT, (S.messages || []).length + _INITIAL_MSG_LIMIT); - const useBeforePaging = requestedLimit >= _MSG_LIMIT_MAX; + const useBeforePaging = requestedLimit >= _msgLimitMax; const data = useBeforePaging ? await api( `/api/session?session_id=${encodeURIComponent(sid)}&messages=1&resolve_model=0&msg_before=${_oldestIdx}&msg_limit=${_INITIAL_MSG_LIMIT}`, diff --git a/tests/test_cross_session_message_load_isolation.py b/tests/test_cross_session_message_load_isolation.py index d15f2512..204b4055 100644 --- a/tests/test_cross_session_message_load_isolation.py +++ b/tests/test_cross_session_message_load_isolation.py @@ -215,6 +215,12 @@ function createEnvironment() { // the fetch URL drops msg_limit/expand_renderable and mismatches the // enqueued buildMessageUrl(), stalling the ordered api() harness. globalThis._MSG_LIMIT_MAX = 500; + // #6177: _msgLimitMax is a module-scope `let` (live server-advertised ceiling, + // defaulting to _MSG_LIMIT_MAX). It's read by _ensureMessagesLoaded's + // boundedReloadLimit and _loadOlderMessages's useBeforePaging; the harness + // injects only the extracted functions, not module-level lets, so define it + // here or those reads resolve undefined -> wrong fetch URL -> ordered-api stall. + globalThis._msgLimitMax = 500; globalThis._currentMessageRenderWindowSize = () => 1; globalThis._messageRenderableMessageCount = () => 2; diff --git a/tests/test_msg_limit_ceiling_drift.py b/tests/test_msg_limit_ceiling_drift.py deleted file mode 100644 index 13fd92ed..00000000 --- a/tests/test_msg_limit_ceiling_drift.py +++ /dev/null @@ -1,71 +0,0 @@ -"""Stopgap drift guard: the frontend ``_MSG_LIMIT_MAX`` mirror must match the -backend ``_MAX_MSG_LIMIT`` ceiling. - -PR #6152 introduced the server-side ``_MAX_MSG_LIMIT`` ceiling on -``GET /api/session ?msg_limit=``. PR #6154 added a hand-mirrored -``_MSG_LIMIT_MAX`` in ``static/sessions.js`` so the frontend knows when to -switch from the growing-tail strategy to ``msg_before`` paging. If the two -drift, load-older silently stalls at the old value — the exact regression #6154 -fixed. The durable fix (#6177) exposes the ceiling via ``/api/session`` metadata -so the mirror isn't needed; until then, this static-source assertion catches the -drift. - -Placement note: this test ships in #6152 (where ``_MAX_MSG_LIMIT`` is defined). -The frontend ``_MSG_LIMIT_MAX`` only exists once #6154 lands, so the assertion -SKIPS when the JS constant is absent — it activates automatically once both -branches are on master, and bites if the two values ever diverge. -""" -from __future__ import annotations - -import re -from pathlib import Path - -ROOT = Path(__file__).resolve().parents[1] - - -def _backend_max_msg_limit() -> int: - """Extract ``_MAX_MSG_LIMIT = `` from api/routes.py.""" - src = (ROOT / "api" / "routes.py").read_text(encoding="utf-8") - m = re.search(r"^_MAX_MSG_LIMIT\s*=\s*(\d+)\s*$", src, re.MULTILINE) - assert m, "_MAX_MSG_LIMIT not found in api/routes.py" - return int(m.group(1)) - - -def _frontend_msg_limit_max(): - """Extract ``_MSG_LIMIT_MAX = `` from static/sessions.js, or return - None when the constant is absent (PR #6154 not yet landed).""" - sessions = ROOT / "static" / "sessions.js" - if not sessions.exists(): - return None - src = sessions.read_text(encoding="utf-8") - m = re.search(r"const\s+_MSG_LIMIT_MAX\s*=\s*(\d+)\s*;", src) - return int(m.group(1)) if m else None - - -def test_frontend_msg_limit_max_matches_backend_ceiling(): - """When both constants exist (i.e. #6152 + #6154 have both landed), the - frontend mirror MUST equal the backend ceiling or load-older silently stalls - for long sessions. Skips gracefully when the JS constant is absent (the - #6152-only state) so this test ships atomically with the backend constant - without depending on the #6154 branch.""" - backend = _backend_max_msg_limit() - frontend = _frontend_msg_limit_max() - if frontend is None: - # PR #6154 (the frontend mirror) hasn't landed yet — skip rather than - # fail. Once both are on master this branch is exercised and bites on - # any drift. - import pytest - pytest.skip("_MSG_LIMIT_MAX not yet present in static/sessions.js " - "(lands in #6154); drift guard activates once both are on master") - assert frontend == backend, ( - f"frontend _MSG_LIMIT_MAX ({frontend}) != backend _MAX_MSG_LIMIT ({backend}): " - f"load-older will silently stall at the wrong value. Update both together, " - f"or land #6177 (metadata-exposure) to retire the mirror." - ) - - -def test_backend_max_msg_limit_is_reasonable(): - """The backend ceiling must exist and stay in a sane range (the frontend's - real pagination grows by ~30, so anything in [100, 2000] is generous).""" - backend = _backend_max_msg_limit() - assert 100 <= backend <= 2000, f"_MAX_MSG_LIMIT out of expected range: {backend}" diff --git a/tests/test_session_msg_limit_ceiling.py b/tests/test_session_msg_limit_ceiling.py index b83138e9..4c7da575 100644 --- a/tests/test_session_msg_limit_ceiling.py +++ b/tests/test_session_msg_limit_ceiling.py @@ -64,3 +64,34 @@ def test_parse_msg_limit_zero_and_negative_clamp_to_one(): for msg_limit=0 gets a 1-row window, not the full transcript.""" assert _parse_msg_limit("0") == 1 assert _parse_msg_limit("-5") == 1 + + +# ── #6177: metadata-decoupling — the frontend reads the ceiling from the +# /api/session `_msg_limit_max` field instead of a hand-mirrored constant. ── + +from pathlib import Path + +_ROUTES_SRC = (Path(__file__).resolve().parents[1] / "api" / "routes.py").read_text(encoding="utf-8") +_SESSIONS_JS = (Path(__file__).resolve().parents[1] / "static" / "sessions.js").read_text(encoding="utf-8") + + +def test_backend_exposes_msg_limit_max_in_session_response(): + """The /api/session handler advertises the ceiling as `_msg_limit_max` so the + frontend never has to hand-mirror _MAX_MSG_LIMIT (#6177 decoupling).""" + assert 'raw["_msg_limit_max"] = _MAX_MSG_LIMIT' in _ROUTES_SRC + + +def test_frontend_declares_live_ceiling_at_module_scope_with_fallback(): + """`_msgLimitMax` MUST be declared at module scope (not an implicit global) + with the static fallback, so the reload-width paths read a DEFINED value + before the first /api/session response lands — otherwise a cold load reads + `undefined`, drops msg_limit, and full-loads every session.""" + assert "let _msgLimitMax = _MSG_LIMIT_MAX;" in _SESSIONS_JS + # refreshed from the response metadata, falling back when the server omits it + assert "_msgLimitMax = data.session._msg_limit_max || _MSG_LIMIT_MAX;" in _SESSIONS_JS + + +def test_frontend_reload_width_paths_read_the_live_ceiling(): + """Both reload-width decisions read the live `_msgLimitMax`, not the mirror.""" + assert "reloadLimit <= _msgLimitMax" in _SESSIONS_JS # _ensureMessagesLoaded + assert "requestedLimit >= _msgLimitMax" in _SESSIONS_JS # _loadOlderMessages diff --git a/tests/test_session_unread_dot_on_visit.py b/tests/test_session_unread_dot_on_visit.py index bd6f5dbd..df358d3f 100644 --- a/tests/test_session_unread_dot_on_visit.py +++ b/tests/test_session_unread_dot_on_visit.py @@ -343,6 +343,8 @@ let _sessionCompletionUnread = null; let _messagesTruncated = false; let _oldestIdx = 0; let _messageRenderWindowSize = 0; +const _MSG_LIMIT_MAX = 500; +let _msgLimitMax = _MSG_LIMIT_MAX; let _pendingCarryForwardSnapshot = null; let _loadingSessionId = 'open'; let _loadSessionGeneration = 0; diff --git a/tests/test_webui_external_refresh_frontend.py b/tests/test_webui_external_refresh_frontend.py index fe6679c1..a3b6c9d9 100644 --- a/tests/test_webui_external_refresh_frontend.py +++ b/tests/test_webui_external_refresh_frontend.py @@ -431,7 +431,9 @@ def test_same_session_force_reload_keeps_loaded_transcript_width_hint(): # ceiling; an over-ceiling hint would be clamped by the backend and could # silently shrink an already-loaded transcript, so it falls back to the bare # full-transcript path (#6152/#6154 ceiling; Codex gate silent row-loss fix). - assert "const boundedReloadLimit = (reloadLimit && reloadLimit <= _MSG_LIMIT_MAX) ? reloadLimit : null;" in SESSIONS_JS + # #6177: the ceiling is now read from /api/session metadata into _msgLimitMax + # (module-scope let, default _MSG_LIMIT_MAX) instead of the mirrored const. + assert "const boundedReloadLimit = (reloadLimit && reloadLimit <= _msgLimitMax) ? reloadLimit : null;" in SESSIONS_JS assert "const reloadLimitParam = boundedReloadLimit ? `&msg_limit=${boundedReloadLimit}` : '';" in SESSIONS_JS assert "if (_ownsLoad()) _clearSameSessionForceReloadHint(sid);" in SESSIONS_JS