From 92c790ffe5515e83e4c746f798ac92a3aa20c71f Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sun, 16 Aug 2026 22:37:00 -0700 Subject: [PATCH] =?UTF-8?q?Batch=20release=20exp-v0.52.235=20=E2=80=94=20c?= =?UTF-8?q?ancelling-run=20reattach=20(#7096)=20+=20GPT-5.6=20max=20reason?= =?UTF-8?q?ing=20(#7083)=20+=20test=20order-independence=20(#7101)=20(#710?= =?UTF-8?q?2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(tests): make mtime_invalidation and glm_5_3 tests order-independent (#7100) * fix: expose max reasoning for GPT-5.6 models * fix(recovery): do not reattach cancelling runs ACTIVE_RUNS tracks worker lifecycle, which is deliberately broader than "a turn a browser may attach to". cancel_stream() keeps the row as phase="cancelling" while the worker unwinds so a successor turn cannot start on top of it, but the client has already reached a terminal state for that stream: its run journal ends in a terminal event. The recovery lookups treated every same-session ACTIVE_RUNS row as attachable. An idle session holding a cancelling row therefore received a recovered server_turn_started on every /api/session/stream subscription: the client attached, replayed the terminal event, tore the renderer down, resubscribed, and the server replayed the same frame again. The result is an endless attach/replay loop that rebuilds the transcript repeatedly. Separate the two meanings instead of narrowing one call site: - api/config.py gains active_run_is_attachable() and active_run_cancel_is_stale() as the shared predicates. - active_stream_id_for_session() (browser recovery) returns attachable rows only; _session_has_active_turn() (busy check) keeps counting a fresh cancellation so a successor cannot overlap the unwinding worker. - _live_active_stream_id() applies the same rule to the hidden-tab status poller, on both the STREAMS and ACTIVE_RUNS paths. - routes._cancelled_run_is_stale() now delegates to the shared predicate rather than keeping a parallel copy of the staleness rule. - A cancelling row past a bounded unwind window with no live STREAMS channel is reclaimed from ACTIVE_RUNS and its stream owner released, so a wedged worker cannot suppress background wakeups forever. Age alone does not reclaim a row that still owns a live channel. Staleness anchors on cancelled_at, falling back to started_at, so a long-running turn cancelled moments ago is never treated as an orphan. tests/test_cancelling_run_not_attachable.py covers both directions of each rule. The six behavioral tests fail on the unpatched tree and pass with the fix; two targeted mutations (forcing the attachability predicate true, and disabling the staleness reaper) each turn the suite red. * docs(rfc): document cancellation attach/admission split Records the runtime contract introduced by the recovery fix in the WebUI run-state consistency RFC, so the distinction is discoverable instead of living only in code comments. - Adds ACTIVE_RUNS to the State Layers table as the worker-lifecycle registry, explicitly not the set of runs a browser may attach to. - Adds invariant 9: lifecycle-busy is not client-attachable. Cancellation splits the two meanings, recovery paths must exclude cancelling rows, and admission checks must keep counting them. - Documents the bounded cancellation-unwind window: reclamation needs both age and the absence of a live STREAMS channel, and staleness is anchored on the cancellation timestamp. - Extends the review checklist with the admission-vs-attachment question and the evidence required when changing a reclamation window. * docs(changelog): note #7096 cancelling-run reattach, #7083 GPT-5.6 max reasoning, #7101 test order-independence --------- Co-authored-by: webtecnica Co-authored-by: Abdulrahman Elkenany Co-authored-by: allenliang2022 Co-authored-by: n --- CHANGELOG.md | 6 + api/background_process.py | 107 ++++++++--- api/config.py | 115 ++++++++--- api/routes.py | 15 +- api/session_ops.py | 15 +- .../webui-run-state-consistency-contract.md | 26 +++ tests/test_cancelling_run_not_attachable.py | 178 ++++++++++++++++++ tests/test_glm_5_3_catalog.py | 14 ++ ...est_reasoning_effort_model_capabilities.py | 61 ++++-- tests/test_ttl_cache.py | 74 +++++--- 10 files changed, 508 insertions(+), 103 deletions(-) create mode 100644 tests/test_cancelling_run_not_attachable.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ec4e50b..6538610b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ ### Fixed +- **Cancelling runs are no longer reattached on recovery.** A background run in the middle of cancellation could be resurrected by a reconnect/recovery reattach, producing a dying run that could double-deliver or churn. Recovery paths now exclude runs whose phase is `cancelling` (they stay busy but are not reattached), while genuinely-live runs still reattach; stale-cancelling reclamation is bounded (age ≥ 180s and absent from active streams) and idempotent. Thanks @allenliang2022. (#7096) + +- **GPT-5.6 models expose their full reasoning range.** The four GPT-5.6 model IDs support the `max` reasoning-effort level, but the capability filter capped them below that. The ceiling is now lifted for exactly those IDs on the OpenAI-family lanes (older GPT-5 and o-series ceilings unchanged), and `max` is offered only when the authoritative configured/live catalog includes it. Thanks @boudywho. (#7083) + +- **Test suite: two model-catalog tests are now order-independent.** `test_mtime_invalidation` depended on sibling-test cache state and `test_glm_5_3_in_models_payload_for_zai_provider` depended on the installed agent-core catalog version, so both failed spuriously under isolation/sharding. They now set up their own preconditions. Thanks @webtecnica. (#7101) + - **Binary files in the workspace no longer truncate when served on Windows.** The shared anchored file-open helper omitted the platform's `O_BINARY` flag, so on Windows a binary workspace file (video, image, archive) was read in text mode and truncated at its first `0x1A` (Ctrl-Z) byte — a 5.2 MB video could arrive as a few hundred bytes, breaking Range requests and playback. Non-directory anchored opens now set `O_BINARY` where the platform provides it (a no-op on Linux/macOS). Thanks @allenliang2022. (#7077) - **Unstyled native form controls now use the UI font.** Bare `button`, `input`, `select`, and `textarea` elements fell back to the browser default font instead of the app's UI font, and the markdown table filter lost the UI font after its `font: inherit` shorthand. Both now route through `--font-ui`; mono, conversation, and skin-scoped controls are unchanged. Thanks @starship-s. (#6871) diff --git a/api/background_process.py b/api/background_process.py index 5c1aba6f..c8dbb1d6 100644 --- a/api/background_process.py +++ b/api/background_process.py @@ -271,6 +271,77 @@ def subscribe_to_session_channel( return ch, q +# Bounded window a cancelling worker may stay lifecycle-busy before an entry +# with no live SSE channel is treated as an orphan. Matches the unwind ceiling +# used by the chat-start successor guard in ``api.routes``. +_ACTIVE_RUN_CANCEL_UNWIND_SECONDS = 180.0 + + +def _active_run_ids_for_session( + session_id: str, + *, + attachable_only: bool, +) -> list[str]: + """Return this session's run stream ids under the requested semantics. + + ``attachable_only`` selects browser-recovery semantics and drops every + ``phase="cancelling"`` row: cancellation is already terminal for the client + even while the worker unwinds. Busy checks pass ``False`` so a freshly + cancelled run still blocks a successor for its bounded unwind window. + + A cancelling row past that window with no live ``STREAMS`` channel is an + orphan: it is dropped from ``ACTIVE_RUNS`` and its stream-owner entry is + released, so a wedged worker cannot suppress background wakeups forever. + """ + from api import config as _cfg + + sid = str(session_id or "").strip() + if not sid: + return [] + try: + with _cfg.STREAMS_LOCK: + live_stream_ids = set((_cfg.STREAMS or {}).keys()) + now = time.time() + matches: list[str] = [] + stale_keys: list[str] = [] + with _cfg.ACTIVE_RUNS_LOCK: + for run_key, meta in list((_cfg.ACTIVE_RUNS or {}).items()): + if not isinstance(meta, dict) or meta.get("session_id") != sid: + continue + stream_id = str(meta.get("stream_id") or run_key or "").strip() + if not stream_id: + continue + cancelling = not _cfg.active_run_is_attachable(meta) + stale_cancel = ( + cancelling + and _cfg.active_run_cancel_is_stale( + meta, + grace_seconds=_ACTIVE_RUN_CANCEL_UNWIND_SECONDS, + now=now, + ) + and run_key not in live_stream_ids + and stream_id not in live_stream_ids + ) + if stale_cancel: + stale_keys.append(run_key) + continue + if attachable_only and cancelling: + continue + matches.append(stream_id) + for run_key in stale_keys: + (_cfg.ACTIVE_RUNS or {}).pop(run_key, None) + for run_key in stale_keys: + _cfg.unregister_stream_owner(run_key) + return matches + except Exception: + logger.debug( + "ACTIVE_RUNS lookup failed for %s", + sid, + exc_info=True, + ) + return [] + + def active_stream_id_for_session(session_id: str) -> Optional[str]: """Return the stream_id of the live run for *session_id*, or None. @@ -288,25 +359,14 @@ def active_stream_id_for_session(session_id: str) -> Optional[str]: Keys on ACTIVE_RUNS (worker-lifecycle registry) — the same source ``_session_has_active_turn`` / ``_emit_to_session_streams`` already trust - to map a stream back to its owning session. Returns the first matching - stream_id (a session has at most one live run; cancel/reconnect can - briefly hold two — either is a valid attach target, the frontend dedupes - by stream_id). + to map a stream back to its owning session — but returns only rows that + are still ATTACHABLE. A ``phase="cancelling"`` row stays lifecycle-busy + while its worker unwinds, yet the client already reached a terminal state + for that run, so replaying ``server_turn_started`` for it makes the tab + attach, receive the terminal event, resubscribe, and loop forever. """ - from api import config as _cfg - - try: - with _cfg.ACTIVE_RUNS_LOCK: - for _stream_id, meta in (_cfg.ACTIVE_RUNS or {}).items(): - if isinstance(meta, dict) and meta.get("session_id") == session_id: - return str(_stream_id) - except Exception: - logger.debug( - "active_stream_id_for_session lookup failed for %s", - session_id, - exc_info=True, - ) - return None + matches = _active_run_ids_for_session(session_id, attachable_only=True) + return matches[0] if matches else None def persisted_message_count_for_session(session_id: str) -> Optional[int]: @@ -1518,16 +1578,7 @@ def _session_has_active_turn(session_id: str) -> bool: ``_start_chat_stream_for_session``'s own active-stream guard — i.e. the same lock /api/chat/start uses is the authoritative race backstop. """ - from api import config as _cfg - - try: - with _cfg.ACTIVE_RUNS_LOCK: - for _stream_id, meta in (_cfg.ACTIVE_RUNS or {}).items(): - if isinstance(meta, dict) and meta.get("session_id") == session_id: - return True - except Exception: - logger.debug("ACTIVE_RUNS active-turn check failed", exc_info=True) - return False + return bool(_active_run_ids_for_session(session_id, attachable_only=False)) def _start_server_side_wakeup_turn( diff --git a/api/config.py b/api/config.py index 44e8a48d..1428829c 100644 --- a/api/config.py +++ b/api/config.py @@ -3515,6 +3515,24 @@ def _zai_glm_thinking_toggle_supported(model_id: str, provider_id: str) -> bool return cls in {"effort", "thinking"} +_OPENAI_FAMILY_REASONING_PROVIDERS = frozenset({ + "openai-codex", "openai", "openai-api", + "azure-foundry", "azure-openai", "azure", +}) + +_GPT_5_6_REASONING_MODELS = frozenset({ + "gpt-5.6", + "gpt-5.6-sol", + "gpt-5.6-terra", + "gpt-5.6-luna", +}) + + +def _is_gpt_5_6_reasoning_model(bare_model: str) -> bool: + """Return whether an OpenAI-family model uses GPT-5.6's max ladder.""" + return str(bare_model or "").strip().lower() in _GPT_5_6_REASONING_MODELS + + def _filter_reasoning_efforts_for_provider( efforts: list[str], model_id: str, @@ -3529,15 +3547,15 @@ def _filter_reasoning_efforts_for_provider( normalized = list(dict.fromkeys(normalized)) provider = _resolve_provider_alias(str(provider_id or "").strip().lower()) bare = _strip_provider_hint_for_reasoning(model_id).lower().rsplit("/", 1)[-1] - # OpenAI-family lanes (Codex, direct OpenAI, Azure Foundry) cap GPT-5 at xhigh - # and o-series at high — 'max' is a WebUI-only level none of them accept. - if provider in {"openai-codex", "openai", "openai-api", "azure-foundry", "azure-openai", "azure"}: + # OpenAI-family lanes cap pre-GPT-5.6 GPT-5 models at xhigh and o-series at + # high. GPT-5.6's alias and Sol/Terra/Luna variants natively accept max. + if provider in _OPENAI_FAMILY_REASONING_PROVIDERS: if bare.startswith(("o1", "o3", "o4")): return [eff for eff in normalized if eff in {"low", "medium", "high"}] - if bare.startswith("gpt-5"): + if bare.startswith("gpt-5") and not _is_gpt_5_6_reasoning_model(bare): return [eff for eff in normalized if eff != "max"] - # 'max' is a WebUI-level ceiling; providers whose native ladder tops out lower - # must NOT advertise it, otherwise a stored/CLI 'max' degrades WORSE than the + # Providers whose native ladder tops out below 'max' must NOT advertise it, + # otherwise a stored/CLI 'max' degrades WORSE than the # prior max->xhigh coercion (Gemini's adapter treats unknown 'max' as medium; # pre-adaptive Anthropic manual-thinking lacks a 'max' budget and falls to 8k). # Dropping 'max' here lets the existing downgrade ladder land on xhigh/high. @@ -3884,11 +3902,11 @@ def resolve_model_reasoning_efforts( """Return supported reasoning-effort levels for *model_id*, or [] if none. Always passes the sourced list through _filter_reasoning_efforts_for_provider - so the hard provider ceilings (openai-codex/openai/azure GPT-5 cap at xhigh, - Gemini + pre-adaptive/cloud-hosted Claude cap at xhigh) are applied uniformly - — the UI dropdown (which gates options on this list) and coercion therefore - agree: 'max' is offered ONLY for models whose native ladder genuinely includes - it, and is stripped everywhere it would be rejected/mishandled. + so the hard provider ceilings (OpenAI-family GPT-5 before 5.6 at xhigh and + o-series at high; Gemini + pre-adaptive/cloud-hosted Claude at xhigh) are + applied uniformly. The UI dropdown and coercion therefore agree: ``max`` is + retained for GPT-5.6 and other models whose native ladder includes it, and + stripped where it would be rejected or mishandled. """ raw = _resolve_model_reasoning_efforts_impl(model_id, provider_id, base_url) if not raw: @@ -4081,16 +4099,17 @@ def coerce_reasoning_effort_for_model( # Hard provider ceilings must win regardless of what the sourced capability # list says. resolve_model_reasoning_efforts() draws from hermes_cli / # models.dev / heuristics, and those can (a) return [] for an unrecognized - # model or (b) wrongly advertise a WebUI-only level like 'max' for a provider + # model or (b) wrongly advertise 'max' for a provider # whose native ladder tops out lower. _filter_reasoning_efforts_for_provider - # encodes the known ceilings (openai-codex gpt-5, Gemini, pre-adaptive - # Anthropic all cap below 'max'); if it actively EXCLUDES the requested level, - # honor that ceiling and degrade down the ladder even when the sourced list is - # empty or (mistakenly) includes the level. This keeps a stored/CLI 'max' from - # reaching an adapter that would silently downgrade it worse than xhigh/high - # (Gemini→medium, legacy Claude manual-thinking→8k). For providers with NO - # ceiling rule the filter returns the full list unchanged, so genuinely - # unknown models still preserve the configured effort (#3505 behavior). + # encodes the known ceilings (OpenAI-family GPT-5 before 5.6, Gemini, and + # pre-adaptive Anthropic all cap below 'max'); if it actively EXCLUDES the + # requested level, honor that ceiling and degrade down the ladder even when + # the sourced list is empty or (mistakenly) includes the level. This keeps a + # stored/CLI 'max' from reaching an adapter that would silently downgrade it + # worse than xhigh/high (Gemini→medium, legacy Claude manual-thinking→8k). + # GPT-5.6 is intentionally not capped. For providers with NO ceiling rule the + # filter returns the full list unchanged, so genuinely unknown models still + # preserve the configured effort (#3505 behavior). ceiling = _filter_reasoning_efforts_for_provider( list(VALID_REASONING_EFFORTS), str(model_id or ""), str(provider_id or "") ) @@ -4108,15 +4127,15 @@ def coerce_reasoning_effort_for_model( # both for models KNOWN not to support reasoning AND for models we simply # don't recognize (custom providers, aggregator-rewritten ids, brand-new # releases). Coercion exists to avoid sending a level a KNOWN-incompatible - # model rejects (e.g. openai-codex gpt-5 'max', o1/o3/o4 above 'high') - + # model rejects (e.g. pre-5.6 GPT-5 'max', o1/o3/o4 above 'high') - # those paths return a NON-empty clamped set, so the degrade ladder below # still applies. When the set is empty we can't tell "unsupported" from # "unknown", so preserve the user's configured effort verbatim where it is # still valid. (#3505 review) # # EXCEPTION for 'max' (the #3505 default-deny refinement, maintainer call - # 2026-07-11): 'max' is a WebUI-only level ABOVE the universally-safe ceiling - # 'xhigh'. A genuinely unknown/custom provider will 400 on it. So when the + # 2026-07-11): 'max' is ABOVE the universally-safe ceiling 'xhigh'. A + # genuinely unknown/custom provider will 400 on it. So when the # capability list is empty AND the provider is not one we recognize as # reasoning-capable, degrade 'max' -> 'xhigh' rather than send an unsupported # supra-ceiling level. But do NOT degrade for a RECOGNIZED reasoning provider @@ -9146,6 +9165,56 @@ LAST_RUN_FINISHED_AT: float | None = None SERVER_START_TIME = time.time() +def active_run_is_attachable(run_entry) -> bool: + """Return whether a run row still represents renderable live work. + + ``ACTIVE_RUNS`` tracks WORKER LIFECYCLE, which is deliberately broader than + "a turn a browser may attach to": ``cancel_stream()`` leaves the row in + ``phase="cancelling"`` while the worker unwinds so a successor cannot start + on top of it. That row is already terminal from the client's perspective — + its run journal ends in a terminal event — so recovery paths that hand a + stream id to the renderer must exclude it. Otherwise every fresh + ``/api/session/stream`` subscription replays ``server_turn_started`` for a + cancelled run, the client attaches, consumes the terminal event, tears the + renderer down and resubscribes, and the loop repeats indefinitely. + + Non-dict entries stay attachable so callers that store an opaque marker are + unaffected; production registrations are dicts carrying ``phase``. + """ + return not ( + isinstance(run_entry, dict) + and str(run_entry.get("phase") or "").strip() == "cancelling" + ) + + +def active_run_cancel_is_stale( + run_entry, + *, + grace_seconds: float, + now: float | None = None, +) -> bool: + """Return whether a cancelling worker outlived its bounded unwind window. + + The age anchor is ``cancelled_at`` rather than the original ``started_at`` + so a long-running turn that was just cancelled is never mistaken for an + orphan; ``started_at`` remains the fallback for rows created before the + cancellation timestamp existed. Callers own the grace window because the + tolerated unwind differs per surface. + """ + if not isinstance(run_entry, dict): + return False + if str(run_entry.get("phase") or "").strip() != "cancelling": + return False + anchor = run_entry.get("cancelled_at") or run_entry.get("started_at") + if not anchor: + return False + try: + age = (time.time() if now is None else float(now)) - float(anchor) + return age >= float(grace_seconds) + except (TypeError, ValueError): + return False + + def register_active_run(stream_id: str, **metadata) -> None: """Mark a WebUI agent worker as alive until its outer finally exits.""" if not stream_id: diff --git a/api/routes.py b/api/routes.py index 7b22a1ad..5c91ec9d 100644 --- a/api/routes.py +++ b/api/routes.py @@ -2934,16 +2934,15 @@ def _cancelled_run_is_stale(run_entry) -> bool: phase="cancelling". ``started_at`` is accepted as a fallback anchor so runs cancelled before the stamp was introduced are still reclaimed eventually. """ - if not isinstance(run_entry, dict) or run_entry.get("phase") != "cancelling": - return False - anchor = run_entry.get("cancelled_at") or run_entry.get("started_at") - if not anchor: - return False try: - age = time.time() - float(anchor) - except (TypeError, ValueError): + from api import config as _live_config + + return _live_config.active_run_cancel_is_stale( + run_entry, + grace_seconds=_STALE_CANCELLED_RUN_GRACE_SECONDS, + ) + except Exception: return False - return age >= _STALE_CANCELLED_RUN_GRACE_SECONDS def _clear_stale_stream_state(session) -> bool: diff --git a/api/session_ops.py b/api/session_ops.py index 83035381..982e13b5 100644 --- a/api/session_ops.py +++ b/api/session_ops.py @@ -30,19 +30,26 @@ def _live_active_stream_id(session) -> str | None: the hidden-tab poller) would make a client attach its renderer to a stream that never emits — a permanent fake "thinking" state. Liveness test mirrors routes._clear_stale_stream_state: live iff present in STREAMS (open SSE - channel) or ACTIVE_RUNS (worker bookkeeping). + channel) or ACTIVE_RUNS (worker bookkeeping) — except that a + ``phase="cancelling"`` run is excluded on both paths, because the worker may + still be unwinding while the client already reached a terminal state for + that stream. """ stream_id = getattr(session, 'active_stream_id', None) if not stream_id: return None try: from api import config as _cfg + with _cfg.ACTIVE_RUNS_LOCK: + _active_run_present = stream_id in (_cfg.ACTIVE_RUNS or {}) + _active_run_entry = (_cfg.ACTIVE_RUNS or {}).get(stream_id) + if _active_run_present and not _cfg.active_run_is_attachable(_active_run_entry): + return None with _cfg.STREAMS_LOCK: if stream_id in _cfg.STREAMS: return stream_id - with _cfg.ACTIVE_RUNS_LOCK: - if stream_id in (_cfg.ACTIVE_RUNS or {}): - return stream_id + if _active_run_present: + return stream_id except Exception: # On any introspection failure, fail SAFE (report no live stream) rather # than surfacing a possibly-stale id. diff --git a/docs/rfcs/webui-run-state-consistency-contract.md b/docs/rfcs/webui-run-state-consistency-contract.md index a14bc520..a3d5a3e0 100644 --- a/docs/rfcs/webui-run-state-consistency-contract.md +++ b/docs/rfcs/webui-run-state-consistency-contract.md @@ -70,6 +70,7 @@ and 5; it does not mark every run-state boundary implemented. | Model context / `context_messages` | Supplies conversation state to the agent | Must include the current visible user turn unless deliberately excluded with a user-visible reason | Let the agent resume from context that contradicts what the user can see | | Pending turn metadata | Bridges submitted-but-not-yet-finalized user input | Must identify the user turn and stream that own active work | Become a permanent duplicate transcript row after recovery | | Live stream / SSE | Delivers active runtime events to the browser | Must remain an observation path, not the only durable truth for already-emitted events | Lose the visible scene on refresh, reconnect, or session switch | +| Worker lifecycle registry (`ACTIVE_RUNS`) | Tracks whether a worker still occupies the session, so a successor turn cannot start on top of it | Broader than "attachable UI work": a cancelled worker stays registered while it unwinds | Be read directly as the set of runs a browser may attach to | | Run journal / replay | Rebuilds emitted runtime events after reconnect or restart | Must be cursor-safe and idempotent | Duplicate assistant text, thinking text, tool cards, or compression cards | | Compression summary / handoff | Gives the agent recovery context after automatic compression | Must remain agent-facing recovery material unless explicitly rendered as history | Pollute the active turn or become implicit current user intent | | Live UI scene/cache | Preserves expanded rows, in-progress cards, local scroll, and transient grouping | May optimize presentation but must be rebuildable or degradable from transcript/replay | Become the only place where chronological ordering exists | @@ -121,6 +122,26 @@ and 5; it does not mark every run-state boundary implemented. 8. **Every mutation names its layer.** A PR touching streaming, recovery, context reconstruction, compression, replay, or sidebar metadata should state which layer it changes and what regression proves the invariant still holds. +9. **Lifecycle-busy is not client-attachable.** `ACTIVE_RUNS` answers "may a new + turn start?", not "may a browser attach a renderer?". Cancellation splits the + two: `cancel_stream()` keeps the row as `phase="cancelling"` so a successor + cannot overlap the unwinding worker, but the client has already reached a + terminal state for that stream because its run journal ends in a terminal + event. Recovery paths that hand a stream id to a renderer — session SSE + recovery and hidden-tab status polling — must therefore exclude cancelling + rows, while busy/admission checks must keep counting them. Reading the + registry with a single meaning resurrects a cancelled run on every fresh + subscription: the client attaches, consumes the terminal event, tears the + renderer down, resubscribes, and the loop repeats indefinitely. + + Because a cancelling row can otherwise persist forever, cancellation unwind is + bounded: a cancelling row older than that window **and** owning no live + `STREAMS` channel is reclaimed from `ACTIVE_RUNS` along with its stream-owner + entry, so a wedged worker cannot suppress background wakeups permanently. + Reclamation requires both conditions — age alone must not evict a row that + still owns a live channel. Staleness is measured from the cancellation + timestamp (falling back to run start), so a long-running turn cancelled + moments ago is never mistaken for an orphan. ## Review Checklist @@ -138,6 +159,11 @@ context reconstruction, or session metadata: interim assistant text, tool cards, compression cards, and terminal states? - Can this change move a session in the sidebar without meaningful user or assistant activity? +- Does this change read `ACTIVE_RUNS` for admission ("may a turn start?") or for + attachment ("may a browser render this?"), and does it use the matching + predicate for that question? +- If it introduces or changes a reclamation window, what proves an in-flight + cancellation is not evicted early, and that a wedged one is eventually freed? - Can automatic compression or recovery text become visible active-turn content? - What test or manual evidence proves the invariant? diff --git a/tests/test_cancelling_run_not_attachable.py b/tests/test_cancelling_run_not_attachable.py new file mode 100644 index 00000000..37677750 --- /dev/null +++ b/tests/test_cancelling_run_not_attachable.py @@ -0,0 +1,178 @@ +"""A cancelling run is lifecycle-busy but NOT attachable live UI work. + +``ACTIVE_RUNS`` tracks worker lifecycle. ``cancel_stream()`` deliberately keeps +the row as ``phase="cancelling"`` while the worker unwinds so a successor turn +cannot start on top of it. The client, however, has already reached a terminal +state for that stream: its run journal ends in a terminal event. + +Before this fix the recovery lookups treated every same-session ``ACTIVE_RUNS`` +row as attachable, so an idle session whose sidecar had already cleared +``active_stream_id`` still received a recovered ``server_turn_started`` for the +cancelled stream on EVERY ``/api/session/stream`` subscription. The client +attached, replayed the terminal event, tore the renderer down, resubscribed, and +the server replayed the same frame again — an endless attach/replay loop. + +These tests pin both directions of the resulting contract: + +* browser recovery excludes cancelling rows, while busy checks still keep a + FRESH cancellation busy (a successor must not overlap the unwinding worker); +* a cancelling row past the bounded unwind window with no live ``STREAMS`` + channel is reclaimed, so a wedged worker cannot suppress wakeups forever; +* a cancelling row that still owns a live ``STREAMS`` channel is NOT reclaimed + on age alone. +""" + +import sys +import time +from pathlib import Path +from types import SimpleNamespace + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT)) + +from api import background_process as bp # noqa: E402 +from api import config as cfg # noqa: E402 +from api.session_ops import _live_active_stream_id # noqa: E402 + + +def _clear(stream_id: str) -> None: + cfg.unregister_active_run(stream_id) + with cfg.STREAMS_LOCK: + cfg.STREAMS.pop(stream_id, None) + + +def test_running_run_is_still_attachable_and_busy(): + """Control: an ordinary running turn keeps both semantics unchanged.""" + sid = "sess-running-control" + stream_id = "stream-running-control" + cfg.register_active_run(stream_id, session_id=sid, phase="running") + try: + assert bp.active_stream_id_for_session(sid) == stream_id + assert bp._session_has_active_turn(sid) is True + assert _live_active_stream_id( + SimpleNamespace(session_id=sid, active_stream_id=stream_id) + ) == stream_id + finally: + _clear(stream_id) + + +def test_fresh_cancelling_run_is_not_attachable_but_stays_busy(): + """The loop-producing case: recovery must not replay a cancelled run. + + The same row must still count as busy so a successor cannot start while the + cancelled worker is unwinding. + """ + sid = "sess-cancelling-fresh" + stream_id = "stream-cancelling-fresh" + cfg.register_active_run( + stream_id, + session_id=sid, + phase="cancelling", + cancelled_at=time.time() - 5.0, + ) + try: + assert bp.active_stream_id_for_session(sid) is None + assert bp._session_has_active_turn(sid) is True + assert stream_id in cfg.ACTIVE_RUNS + finally: + _clear(stream_id) + + +def test_hidden_tab_status_does_not_resurrect_a_cancelling_run(): + """/api/session/status must not hand a cancelling stream to the poller.""" + sid = "sess-cancelling-status" + stream_id = "stream-cancelling-status" + cfg.register_active_run(stream_id, session_id=sid, phase="cancelling") + try: + assert _live_active_stream_id( + SimpleNamespace(session_id=sid, active_stream_id=stream_id) + ) is None + finally: + _clear(stream_id) + + +def test_hidden_tab_status_ignores_cancelling_run_with_live_stream_channel(): + """A cancelling row is terminal for the client even if STREAMS still holds it.""" + sid = "sess-cancelling-status-live-channel" + stream_id = "stream-cancelling-status-live-channel" + cfg.register_active_run(stream_id, session_id=sid, phase="cancelling") + with cfg.STREAMS_LOCK: + cfg.STREAMS[stream_id] = object() + try: + assert _live_active_stream_id( + SimpleNamespace(session_id=sid, active_stream_id=stream_id) + ) is None + finally: + _clear(stream_id) + + +def test_stale_cancelling_orphan_stops_blocking_and_is_reclaimed(): + """Past the unwind window with no live channel, the row is an orphan.""" + sid = "sess-cancelling-stale" + stream_id = "stream-cancelling-stale" + cfg.register_active_run( + stream_id, + session_id=sid, + phase="cancelling", + cancelled_at=time.time() - 240.0, + ) + cfg.register_stream_owner(stream_id, sid) + try: + assert bp._session_has_active_turn(sid) is False + assert stream_id not in cfg.ACTIVE_RUNS + assert cfg.stream_owner_session_id(stream_id) is None + finally: + _clear(stream_id) + + +def test_stale_cancelling_run_with_live_channel_is_not_reclaimed(): + """Reverse control: age alone must not reap a row that still owns a channel.""" + sid = "sess-cancelling-stale-live-channel" + stream_id = "stream-cancelling-stale-live-channel" + cfg.register_active_run( + stream_id, + session_id=sid, + phase="cancelling", + cancelled_at=time.time() - 240.0, + ) + with cfg.STREAMS_LOCK: + cfg.STREAMS[stream_id] = object() + try: + assert bp._session_has_active_turn(sid) is True + assert stream_id in cfg.ACTIVE_RUNS + finally: + _clear(stream_id) + + +def test_cancel_staleness_anchors_on_cancel_time_not_run_start(): + """A long-running turn cancelled just now must not read as stale.""" + entry = { + "session_id": "sess-anchor", + "phase": "cancelling", + "started_at": time.time() - 4000.0, + "cancelled_at": time.time() - 5.0, + } + assert cfg.active_run_cancel_is_stale(entry, grace_seconds=180.0) is False + + legacy = { + "session_id": "sess-anchor-legacy", + "phase": "cancelling", + "started_at": time.time() - 4000.0, + } + assert cfg.active_run_cancel_is_stale(legacy, grace_seconds=180.0) is True + + running = { + "session_id": "sess-anchor-running", + "phase": "running", + "started_at": time.time() - 4000.0, + } + assert cfg.active_run_cancel_is_stale(running, grace_seconds=180.0) is False + + +def test_attachability_predicate_edges(): + """Non-dict/opaque entries stay attachable; only cancelling is excluded.""" + assert cfg.active_run_is_attachable({"phase": "running"}) is True + assert cfg.active_run_is_attachable({"phase": "cancelling"}) is False + assert cfg.active_run_is_attachable({"phase": " cancelling "}) is False + assert cfg.active_run_is_attachable({}) is True + assert cfg.active_run_is_attachable(object()) is True diff --git a/tests/test_glm_5_3_catalog.py b/tests/test_glm_5_3_catalog.py index 40b612eb..03dab107 100644 --- a/tests/test_glm_5_3_catalog.py +++ b/tests/test_glm_5_3_catalog.py @@ -197,6 +197,20 @@ def test_glm_5_3_in_models_payload_for_zai_provider(tmp_path, monkeypatch): except Exception: pass + # Pin the agent-core catalog: get_available_models() sources the zai + # model list from the INSTALLED hermes-cli core (via + # _read_live_provider_model_ids -> provider_model_ids), not from the + # repo's static _PROVIDER_MODELS. On a box whose installed core predates + # glm-5.3 this test previously failed spuriously ("glm-5.3 missing from + # zai group models"). Stub the core catalog to a deterministic empty + # list so the repo's own _PROVIDER_MODELS fallback is exercised — this + # tests WebUI catalog propagation, not the installed core version. + try: + import hermes_cli.models as hm + monkeypatch.setattr(hm, "provider_model_ids", lambda _pid: []) + except Exception: + pass + result = c.get_available_models() c.reload_config() diff --git a/tests/test_reasoning_effort_model_capabilities.py b/tests/test_reasoning_effort_model_capabilities.py index 2a4e7edb..910c1197 100644 --- a/tests/test_reasoning_effort_model_capabilities.py +++ b/tests/test_reasoning_effort_model_capabilities.py @@ -3,6 +3,23 @@ from api import config as cfg +GPT_5_6_MODELS = ( + "gpt-5.6", + "gpt-5.6-sol", + "gpt-5.6-terra", + "gpt-5.6-luna", +) + +OPENAI_FAMILY_PROVIDERS = ( + "openai", + "openai-api", + "openai-codex", + "azure", + "azure-openai", + "azure-foundry", +) + + def test_cursor_acp_models_do_not_support_reasoning_effort_levels(): assert cfg.resolve_model_reasoning_efforts( "cursor/composer-2.5", @@ -40,6 +57,21 @@ def test_openai_codex_max_effort_is_clamped_before_streaming(): ) == "xhigh" +def test_openai_family_gpt56_models_expose_and_preserve_max(): + for provider in OPENAI_FAMILY_PROVIDERS: + for model in GPT_5_6_MODELS: + efforts = cfg.resolve_model_reasoning_efforts( + f"@{provider}:{model}", + provider_id=provider, + ) + assert "max" in efforts, f"{model} on {provider} must expose max" + assert cfg.coerce_reasoning_effort_for_model( + "max", + f"@{provider}:{model}", + provider_id=provider, + ) == "max", f"{model} on {provider} must preserve max" + + def test_unsupported_xhigh_degrades_to_high_not_disabled(): # o1/o3/o4 on openai-codex cap at low/medium/high. A configured xhigh (or # max) must clamp DOWN to the highest supported level (high), not silently @@ -79,8 +111,8 @@ def test_coerce_preserves_effort_for_unrecognized_model(): "some-unknown-model-xyz", provider_id="some-custom-provider", ) == "high" - # #3505 default-deny refinement (maintainer 2026-07-11): 'max' is a supra- - # ceiling WebUI-only level, so on an UNRECOGNIZED provider it degrades to + # #3505 default-deny refinement (maintainer 2026-07-11): 'max' is above the + # universally safe ceiling, so on an UNRECOGNIZED provider it degrades to # xhigh (a truly-unknown provider would 400 on max). All OTHER levels still # preserve verbatim below. assert cfg.coerce_reasoning_effort_for_model( @@ -434,16 +466,18 @@ def test_max_effort_preserved_for_adaptive_anthropic_and_deepseek(): ) == "max" -def test_max_degrades_across_all_openai_family_lanes(): - # 'max' is WebUI-only; direct OpenAI, openai-api, and Azure Foundry GPT-5 all - # cap at xhigh (o-series at high), not just openai-codex. (#4627 re-gate) - for prov in ("openai", "openai-api", "azure-foundry", "openai-codex"): - assert cfg.coerce_reasoning_effort_for_model( - "max", model_id="gpt-5.1", provider_id=prov - ) == "xhigh", f"gpt-5 on {prov} must degrade max->xhigh" - assert cfg.coerce_reasoning_effort_for_model( - "max", model_id="o3", provider_id=prov - ) == "high", f"o-series on {prov} must degrade max->high" +def test_max_degrades_for_pre_gpt56_and_o_series_across_openai_family_lanes(): + # GPT-5 models before 5.6 cap at xhigh, while o1/o3/o4 cap at high, across + # direct OpenAI, ChatGPT/Codex, and Azure provider aliases. + for provider in OPENAI_FAMILY_PROVIDERS: + for model in ("gpt-5", "gpt-5.1", "gpt-5.5"): + assert cfg.coerce_reasoning_effort_for_model( + "max", model_id=model, provider_id=provider + ) == "xhigh", f"{model} on {provider} must degrade max->xhigh" + for model in ("o1", "o3-mini", "o4-mini"): + assert cfg.coerce_reasoning_effort_for_model( + "max", model_id=model, provider_id=provider + ) == "high", f"{model} on {provider} must degrade max->high" def test_max_degrades_for_azure_bedrock_hosted_legacy_claude(): @@ -461,7 +495,7 @@ def test_max_degrades_for_azure_bedrock_hosted_legacy_claude(): def test_max_degrades_on_unknown_provider_but_other_levels_preserved(): # #3505 default-deny refinement (maintainer call 2026-07-11): 'max' is above - # the universal ceiling, so an unknown/custom provider (empty capability list) + # the universally safe ceiling, so an unknown/custom provider (empty capability list) # must degrade 'max'->'xhigh' rather than send an unsupported level — while all # other levels keep the conservative preserve-verbatim behavior. assert cfg.coerce_reasoning_effort_for_model( @@ -560,4 +594,3 @@ def test_qwen_prefixed_alias_reasoning_detection(): f"{model} must remain reasoning-capable (DeepSeek-R1 hybrid, " f"Qwen 2.x must not shadow the DeepSeek detector)" ) - diff --git a/tests/test_ttl_cache.py b/tests/test_ttl_cache.py index 42a7d99c..33c2a0b6 100644 --- a/tests/test_ttl_cache.py +++ b/tests/test_ttl_cache.py @@ -111,39 +111,61 @@ def test_ttl_expiry(): def test_mtime_invalidation(): """Populate the cache, then change _cfg_mtime to simulate a config file change on disk. The next call should invalidate the cache and re-scan. + + This test is fully self-contained: it invalidates both memory and disk + caches first, then populates the cache via a priming call, then verifies + that an mtime mismatch triggers a cache rebuild. It does not depend on + any sibling test's execution order. """ - _reset_cache() + # Fully invalidate (memory + disk) so we start from a known clean state. + config.invalidate_models_cache() + + # Force the synchronous (unbounded) rebuild path so the priming call is + # guaranteed to have published the cache when it returns. With the + # default bounded budget (_LIVE_REBUILD_BUDGET_SECONDS=4.0), a cold + # rebuild on a slow/loaded box can exceed the budget and return a + # static fallback WITHOUT populating _available_models_cache — which + # made this test fail when run in isolation (it previously only passed + # because a sibling test had already warmed the cache in-process). + original_budget = config._LIVE_REBUILD_BUDGET_SECONDS + config._LIVE_REBUILD_BUDGET_SECONDS = 0.0 + real_mtime = 0.0 - # Ensure _cfg_mtime matches file so first call doesn't re-scan due to mtime try: - real_mtime = config.Path(config._get_config_path()).stat().st_mtime - except OSError: - real_mtime = 0.0 - config._cfg_mtime = real_mtime + # Ensure _cfg_mtime matches file so first call doesn't re-scan due to mtime + try: + real_mtime = config.Path(config._get_config_path()).stat().st_mtime + except OSError: + real_mtime = 0.0 + config._cfg_mtime = real_mtime - # First call populates cache - result1 = config.get_available_models() - assert config._available_models_cache is not None + # Priming call: populate the cache (both memory and disk) + result1 = config.get_available_models() + assert config._available_models_cache is not None, ( + "Priming call should populate the in-memory cache" + ) + assert config._available_models_cache_ts > 0.0, ( + "Priming call should set a valid cache timestamp" + ) - # Simulate config.yaml changed on disk by setting _cfg_mtime to 0 - # (which won't match the actual file mtime) - config._cfg_mtime = 0.0 + # Simulate config.yaml changed on disk by setting _cfg_mtime to 0 + # (which won't match the actual file mtime) + config._cfg_mtime = 0.0 - # The next call should detect mtime mismatch, reload, and invalidate cache - old_cache = config._available_models_cache - old_ts = config._available_models_cache_ts + # The next call should detect mtime mismatch, reload, and invalidate cache + result2 = config.get_available_models() - result2 = config.get_available_models() - - # Cache must have been refreshed — timestamp advanced since we reset it - # to 0.0 on invalidation. - assert config._available_models_cache_ts > 0.0, ( - "Cache timestamp should be updated after invalidation + rebuild" - ) - - # Restore - config._cfg_mtime = real_mtime - _reset_cache() + # Cache must have been refreshed — timestamp advanced since we reset it + # to 0.0 on invalidation. + assert config._available_models_cache_ts > 0.0, ( + "Cache timestamp should be updated after invalidation + rebuild" + ) + # Both calls must return a valid /api/models payload. + assert "groups" in result1 and "groups" in result2 + finally: + config._LIVE_REBUILD_BUDGET_SECONDS = original_budget + config._cfg_mtime = real_mtime + config.invalidate_models_cache() # ── 4. test_deepcopy_isolation ────────────────────────────────────────────