Skip to content

fix(tests): make mtime_invalidation and glm_5_3 robust (#7100) - #7101

Closed
webtecnica wants to merge 1 commit into
nesquena:masterfrom
webtecnica:fix/7100-test-robustness
Closed

webtecnica wants to merge 1 commit into
nesquena:masterfrom
webtecnica:fix/7100-test-robustness

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

What Changed

Hardened two fragile tests so the full suite no longer produces spurious failures under SHARDED=1 / isolated runs:

tests/test_ttl_cache.py::test_mtime_invalidation

  • Made the test fully self-contained: it now invalidates both memory and disk caches first, then primes the cache via an explicit call inside the test body, instead of relying on cache state established by an earlier sibling test (test_cache_hit_within_ttl / test_ttl_expiry).
  • The priming call now forces the synchronous (unbounded) rebuild path by temporarily setting _LIVE_REBUILD_BUDGET_SECONDS = 0.0, so the cache is guaranteed to be published before the first assertion runs. Previously, a cold rebuild on a slow/loaded box could exceed the 4s budget and return a static fallback without populating _available_models_cache, making the test fail when picked up by a shard without its predecessors.
  • Added explicit assertions on the priming call (_available_models_cache is not None, _available_models_cache_ts > 0.0) so failures are diagnosed at the exact precondition, not downstream.

tests/test_glm_5_3_catalog.py::test_glm_5_3_in_models_payload_for_zai_provider

  • get_available_models() sources the zai model list from the installed hermes-cli core catalog (_read_live_provider_model_idsprovider_model_ids), not from the repo's static _PROVIDER_MODELS. On a box whose installed core predates glm-5.3 (e.g. only glm-5/5.1/5.2), the test failed with "glm-5.3 missing from zai group models".
  • The test now stubs the core catalog (provider_model_ids[]) so the repo's own _PROVIDER_MODELS fallback is exercised deterministically — verifying WebUI catalog propagation, independent of the installed agent-core version. This matches the issue's suggested fix (a): "pin/stub the core catalog in the test".

Verification

  • pytest tests/test_ttl_cache.py::test_mtime_invalidationPASSED in isolation (previously order-dependent).
  • pytest tests/test_glm_5_3_catalog.py::test_glm_5_3_in_models_payload_for_zai_providerPASSED (previously FAILED on this box: installed core lacks glm-5.3).
  • pytest tests/test_ttl_cache.py tests/test_glm_5_3_catalog.py13 passed (full-file run intact, no sibling regressions).

Closes #7100

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes two catalog/cache tests less dependent on test order and the locally installed Hermes core version.

  • Stubs the live Z.ai core catalog so the WebUI fallback catalog is exercised deterministically.
  • Fully resets and synchronously primes the models cache before checking mtime invalidation.

Confidence Score: 4/5

The PR appears safe to merge, with a non-blocking gap in the mtime test’s ability to detect whether the second call actually rebuilds the cache.

The test isolation changes are sound overall, but the mtime regression check can pass from state established before the operation it is intended to verify.

Files Needing Attention: tests/test_ttl_cache.py

Important Files Changed

Filename Overview
tests/test_glm_5_3_catalog.py Pins the live provider catalog to an empty list so the test deterministically exercises the repository fallback.
tests/test_ttl_cache.py Makes cache setup and cleanup self-contained, but the post-mismatch assertion does not prove that invalidation rebuilt the primed cache.

Reviews (1): Last reviewed commit: "fix(tests): make mtime_invalidation and ..." | Re-trigger Greptile

Comment thread tests/test_ttl_cache.py
Comment on lines +159 to +162
# to 0.0 on invalidation.
assert config._available_models_cache_ts > 0.0, (
"Cache timestamp should be updated after invalidation + rebuild"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Assert the cache was rebuilt

The priming call already establishes that _available_models_cache_ts is positive, so repeating that assertion after the mtime mismatch does not distinguish a rebuild from reuse of the primed cache. Compare the cache identity or timestamp across the second call so an mtime-invalidation regression cannot pass undetected.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Reviewed the diff at tests/test_ttl_cache.py and tests/test_glm_5_3_catalog.py against api/config.py on origin/master. Both hardenings are correct and target the real failure mechanisms.

test_mtime_invalidation — forced-sync priming is the right fix

The order dependency was real: a shard picking up this test without a sibling having warmed the in-process cache could hit the bounded rebuild path and return a static fallback without publishing _available_models_cache, so the first assert config._available_models_cache is not None blew up. Setting _LIVE_REBUILD_BUDGET_SECONDS = 0.0 routes the priming call through the legacy synchronous branch, which is the only path that unconditionally publishes before returning:

# api/config.py:8190-8222 (budget <= 0 branch)
if _LIVE_REBUILD_BUDGET_SECONDS <= 0:
    ...
    with _cache_build_cv:
        published_at = time.monotonic()
        _available_models_cache = result
        _available_models_cache_ts = published_at

So the two new precondition asserts (_available_models_cache is not None, _ts > 0.0) are guaranteed to hold under the forced-sync path. The try/finally restoring _LIVE_REBUILD_BUDGET_SECONDS, _cfg_mtime, and calling invalidate_models_cache() keeps global module state clean even on assertion failure — good hygiene given this is process-global.

test_glm_5_3 — stubbing provider_model_ids is effective because the import is lazy

The zai group flows through the generic branch, not a special-case, so it asks the installed core first:

# api/config.py:7822-7828
else:
    raw_models = _models_from_live_provider_ids(
        pid, _read_live_provider_model_ids(pid),
    )
if not raw_models:
    raw_models = copy.deepcopy(_PROVIDER_MODELS.get(pid, []))

Worth calling out why monkeypatch.setattr(hm, "provider_model_ids", lambda _pid: []) actually works here: _read_live_provider_model_ids re-imports the symbol lazily on every call (from hermes_cli.models import provider_model_ids as _provider_model_ids at config.py:6397), so patching the module attribute is picked up. If the import had been bound at module load, the patch would have missed and the test would still read the installed core. With the stub returning [] for every candidate, _read_live_provider_model_ids returns [], raw_models stays empty, and the _PROVIDER_MODELS["zai"] fallback (which carries glm-5.3 at config.py:1739) is exercised deterministically — exactly the "test WebUI propagation, not installed core" intent.

CI is green (lint + browser-smoke + the sharded test (3.x, N) matrix). No concerns; this is a clean test-only change that removes real sharded-gate noise. LGTM.

nesquena-hermes added a commit that referenced this pull request Aug 17, 2026
….6 max reasoning (#7083) + test order-independence (#7101) (#7102)

* 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 <webtecnica@gmail.com>
Co-authored-by: Abdulrahman Elkenany <boudy.elkenany123@gmail.com>
Co-authored-by: allenliang2022 <allenliang2022@users.noreply.github.com>
Co-authored-by: n <a@n>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in experimental release exp-v0.52.235 (closes #7100). test_mtime_invalidation and test_glm_5_3_in_models_payload_for_zai_provider are now order-independent and pass under isolation/sharding. Thanks @webtecnica!

alai04 pushed a commit to alai04/hermes-webui that referenced this pull request Aug 31, 2026
… + GPT-5.6 max reasoning (nesquena#7083) + test order-independence (nesquena#7101) (nesquena#7102)

* fix(tests): make mtime_invalidation and glm_5_3 tests order-independent (nesquena#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 nesquena#7096 cancelling-run reattach, nesquena#7083 GPT-5.6 max reasoning, nesquena#7101 test order-independence

---------

Co-authored-by: webtecnica <webtecnica@gmail.com>
Co-authored-by: Abdulrahman Elkenany <boudy.elkenany123@gmail.com>
Co-authored-by: allenliang2022 <allenliang2022@users.noreply.github.com>
Co-authored-by: n <a@n>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test robustness: test_mtime_invalidation (intra-file order dep) + test_glm_5_3 (installed-core dep) fail under isolation/sharding

2 participants