From 5a7033a6fb814783a0948d8ef6b3a6c8659a719d Mon Sep 17 00:00:00 2001 From: CC#3 Kora Runtime Date: Sat, 23 May 2026 14:22:42 -0700 Subject: [PATCH] =?UTF-8?q?chore(kora):=20KR-TEST-STABILITY-XDIST=20?= =?UTF-8?q?=E2=80=94=20fix=20email=20handler=20xdist=20flake?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CC#1 flagged in PR #149: tests/kora_cli/handlers/test_email_ inbound_handler.py flakes under pytest-xdist (4-6 failures with HANDLED_RECEIVED vs filtered_paused mismatch). Serial run: clean. # Root cause Two test files installed an OperationalStateHolder via direct ``h_mod._HOLDER = holder`` assignment without teardown reset: - tests/kora_cli/test_listeners/test_mcp_tools_stop_control.py (KR-MCP-STOP-CONTROL ST1, PR #142) — _holder_with_state helper mutates _HOLDER 13 times across the suite. No cleanup. - tests/kora_cli/test_listeners/test_mcp_audit_on_denial.py (KR-MCP-AUDIT-ON-DENIAL, PR #150) — success-path test installs an ACTIVE holder. No cleanup. Why xdist surfaces it: xdist workers are separate processes, so state doesn't cross workers. But WITHIN a single worker tests run serially in scheduled order. When the scheduler put one of these mutating tests on the same worker as test_email_inbound_handler AND the mutating test ran first, the dirty PAUSED/ACTIVE holder persisted. The email handler's state-gate (_check_state_gate in email_inbound_handler.py:509) then saw the stale PAUSED holder and returned HANDLED_FILTERED_PAUSED instead of HANDLED_RECEIVED. Why serial doesn't flake: pytest's default test ordering puts the handlers/ tree BEFORE the test_listeners/ tree alphabetically, so the mutating tests never run before the email tests in serial mode. xdist's worksteal scheduler doesn't preserve that ordering. # Fix Three autouse ``_reset_operational_state_holder`` fixtures (one per affected file) that set ``h_mod._HOLDER = None`` at both setup and teardown: - test_mcp_tools_stop_control.py — fixes the bleed source - test_mcp_audit_on_denial.py — fixes the second bleed source - test_email_inbound_handler.py — DEFENSIVE belt-and-suspenders so a future test author who forgets the teardown doesn't re-poison the email handler tests The defensive autouse in test_email_inbound_handler.py is the documented pattern: any test file whose tests assume a fresh ``OperationalStateHolder`` singleton should opt-in via this fixture pattern, even if today's leaking-sources are all fixed. Not changing production code (the singleton design itself stays — spec §3 explicit non-scope). The fix is test-side only. # Verification Repro confirmed pre-fix: 3-4 out of 15 xdist runs failed. Post-fix verification (xdist): - 30/30 xdist runs of alerts + test_listeners + handlers + audit + clients (627 tests) — all green - 5/5 explicit spec-mandated xdist runs — all green - Full repo (tests/kora_cli + tests/agent) xdist: 9171 passed, 43 failed — failures all pre-existing in test_anthropic_adapter.py / test_backup.py / test_gateway_service.py / test_web_server*.py / test_model_switch_custom_providers.py (zero failures in test_email_inbound_handler.py) Serial regression unchanged: 627/627 still green. # Pattern for future test authors When mutating a module-level singleton in tests: GOOD: monkeypatch.setattr(h_mod, "_HOLDER", holder) — pytest auto-restores at test teardown. ALSO GOOD: an autouse fixture that resets the singleton at setup AND teardown (the pattern landed here). BAD: ``h_mod._HOLDER = holder`` — the assignment persists past the test boundary and can bleed across the xdist worker into any other test in the same worker scheduled later. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../handlers/test_email_inbound_handler.py | 31 +++++++++++++++++++ .../test_mcp_audit_on_denial.py | 20 ++++++++++++ .../test_mcp_tools_stop_control.py | 26 ++++++++++++++++ 3 files changed, 77 insertions(+) diff --git a/tests/kora_cli/handlers/test_email_inbound_handler.py b/tests/kora_cli/handlers/test_email_inbound_handler.py index 42608031b114..5c9660c28d19 100644 --- a/tests/kora_cli/handlers/test_email_inbound_handler.py +++ b/tests/kora_cli/handlers/test_email_inbound_handler.py @@ -106,6 +106,37 @@ def _isolate(tmp_path, monkeypatch): return tmp_path +@pytest.fixture(autouse=True) +def _reset_operational_state_holder(): + """Self-protective reset of the ``OperationalStateHolder`` + singleton — KR-TEST-STABILITY-XDIST. + + The handler's state-gate (``_check_state_gate`` in + ``email_inbound_handler.py``) calls ``get_holder()`` to decide + whether to drop the message as ``filtered_paused`` / + ``filtered_stopped``. State-gate tests in this file (lines + 151-200) ``patch`` the accessor; the other ~30 tests expect + the singleton to be ``None`` so ``get_holder()`` returns + ``None`` and the handler proceeds. + + Under pytest-xdist, OTHER test files in the same worker may + install a non-None holder (e.g. + ``test_mcp_tools_stop_control.py`` from KR-MCP-STOP-CONTROL ST1 + installed PAUSED holders without teardown reset before this + bucket landed). A bleed surfaces as flaky + ``HANDLED_RECEIVED vs filtered_paused`` assertions. + + This fixture is BELT-AND-SUSPENDERS: the leaking test files + now also reset their holders, but a future test author who + forgets the teardown won't poison email-handler tests. + """ + from agent import operational_state_holder as h_mod + + h_mod._HOLDER = None + yield + h_mod._HOLDER = None + + def _log_path(tmp_path: Path) -> Path: return tmp_path / "email_inbound_log.jsonl" diff --git a/tests/kora_cli/test_listeners/test_mcp_audit_on_denial.py b/tests/kora_cli/test_listeners/test_mcp_audit_on_denial.py index a65ff3573262..27d8e05791c8 100644 --- a/tests/kora_cli/test_listeners/test_mcp_audit_on_denial.py +++ b/tests/kora_cli/test_listeners/test_mcp_audit_on_denial.py @@ -57,6 +57,26 @@ def _reset_caller_cache(): mcp_caller_auth._reset_cache_for_tests() +@pytest.fixture(autouse=True) +def _reset_operational_state_holder(): + """Reset the module-level ``OperationalStateHolder`` singleton + between tests — KR-TEST-STABILITY-XDIST. + + ``test_successful_call_does_not_emit_denial_audit`` installs an + ACTIVE holder via direct ``h_mod._HOLDER = ...`` so the pause + executor can transition. Without this autouse reset, that + holder persists across worker boundaries to subsequent tests in + the same xdist worker — surfacing as ``test_email_inbound_ + handler.py`` flakes where the state-gate sees an unexpected + holder. + """ + from agent import operational_state_holder as h_mod + + h_mod._HOLDER = None + yield + h_mod._HOLDER = None + + @pytest.fixture def empty_caps_token(monkeypatch, tmp_path): """Caller authenticated but with NO caps — every mutating tool denies.""" diff --git a/tests/kora_cli/test_listeners/test_mcp_tools_stop_control.py b/tests/kora_cli/test_listeners/test_mcp_tools_stop_control.py index 69d2d4e843d1..1eb63c7de113 100644 --- a/tests/kora_cli/test_listeners/test_mcp_tools_stop_control.py +++ b/tests/kora_cli/test_listeners/test_mcp_tools_stop_control.py @@ -48,6 +48,32 @@ def _reset_caller_cache(): mcp_caller_auth._reset_cache_for_tests() +@pytest.fixture(autouse=True) +def _reset_operational_state_holder(): + """Reset the module-level ``OperationalStateHolder`` singleton + between tests — KR-TEST-STABILITY-XDIST. + + The ``_holder_with_state`` helper below installs a holder via + direct ``h_mod._HOLDER = holder`` assignment (rather than + ``monkeypatch.setattr``) because most tests use the helper for + its side effect WITHOUT taking ``monkeypatch`` as a fixture arg. + Without this autouse reset, a test that sets the holder to + PAUSED leaks into the next test on the same xdist worker — + surfacing as ``test_email_inbound_handler.py`` flakes where the + state-gate sees a stale PAUSED holder and returns + ``filtered_paused`` instead of ``received``. + + Resetting at BOTH setup and teardown is intentional: a previous + test in the same worker may have left a dirty holder, AND this + test may dirty the holder. Either path catches the leak. + """ + from agent import operational_state_holder as h_mod + + h_mod._HOLDER = None + yield + h_mod._HOLDER = None + + @pytest.fixture def authorized_token(monkeypatch, tmp_path): """Caller with BOTH pause + resume caps (no full transition cap)."""