fix(metrics): reset global metrics collector during teardown to avoid test pollution

[Sanderhoff-alt]

Summary

Fixes #3780.

This PR resolves a process-global metrics collector leak that caused nondeterministic TypeError failures across LLM provider test suites during concurrent pytest-xdist execution.

---

1. Problem Architecture & Root Cause Analysis

1.1 State Leak Lifecycle

During pytest-xdist test execution, when a worker process executes a test that initializes the FastAPI application lifespan (e.g. integration or HTTP tests), create_metrics_collector() mutates module-level global state _metrics_collector. Because neither application shutdown nor pytest teardown restored this reference, the worker process remained "poisoned" with an active MetricsCollector instance for all subsequent tests scheduled on that worker.

sequenceDiagram
    autonumber
    participant Worker as xdist Worker Process
    participant TestApp as Test (Starts FastAPI App)
    participant Lifespan as FastAPI Lifespan
    participant Metrics as hindsight_api.metrics
    participant ProviderTest as Subsequent Provider Test

    Note over Worker,Metrics: Initial State: _metrics_collector = NoOpMetricsCollector()
    Worker->>TestApp: Execute test
    TestApp->>Lifespan: Startup
    Lifespan->>Metrics: create_metrics_collector()
    Metrics-->>Worker: Global _metrics_collector = MetricsCollector() (LIVE)
    TestApp->>Lifespan: Shutdown (Previously NO metrics cleanup)
    Note over Metrics: Leaked: _metrics_collector remains MetricsCollector()

    Worker->>ProviderTest: Execute provider test on same worker
    ProviderTest->>Metrics: get_metrics_collector()
    Metrics-->>ProviderTest: Returns active MetricsCollector() (POISONED)
    ProviderTest->>Metrics: record_llm_call(cached_input_tokens=MagicMock)
    Metrics-->>ProviderTest: TypeError: '>' not supported between 'MagicMock' and 'int'

Loading

1.2 Failure Mechanism in LLM Provider Mocks

Provider unit tests mock LLM responses with MagicMock. When _usage_from_openai_response extracts cached_tokens:

getattr(usage.prompt_tokens_details, "cached_tokens", 0) or 0

On a MagicMock, attribute traversal auto-instantiates child MagicMock instances. When passed to the active MetricsCollector, if cached_input_tokens > 0 raises a TypeError.

Component	Default / Isolated Test State	Poisoned Worker State	Result
_metrics_collector	NoOpMetricsCollector()	MetricsCollector()	Global state leak across tests
Token evaluation	No-op (arguments ignored)	cached_input_tokens > 0	Comparison executed against mock
Mock response	response.usage = MagicMock()	response.usage = MagicMock()	cached_input_tokens is MagicMock
Test outcome	PASSED	FAILED (TypeError)	Flaky failures dependent on xdist worker assignment

---

2. Technical Design & Solution

flowchart TD
    subgraph CoreModule ["Core Module (hindsight_api/metrics.py)"]
        GlobalVar["_metrics_collector<br/>(Process-Global State)"]
        ResetAPI["reset_metrics_collector()"]
        NoOpInst["NoOpMetricsCollector()"]
        ResetAPI -->|reassigns to| GlobalVar
        NoOpInst -.->|default value| GlobalVar
    end

    subgraph ProdLifecycle ["Production / App Lifecycle (api/http.py)"]
        ShutdownHook["FastAPI Lifespan Teardown<br/>(on app shutdown)"] -->|invokes| ResetAPI
    end

    subgraph TestIsolation ["Test Environment (tests/conftest.py)"]
        AutouseFixture["Autouse Fixture<br/>_cleanup_leaked_metrics_collector"]
        AutouseFixture -->|1. Setup: snapshot & reset to NoOp| GlobalVar
        AutouseFixture -->|2. Teardown: restore previous state| GlobalVar
    end

Loading

2.1 Changes Implemented

1. hindsight_api/metrics.py:

   • Implemented reset_metrics_collector() to explicitly restore _metrics_collector = NoOpMetricsCollector().

2. hindsight_api/api/http.py:

   • Added reset_metrics_collector() to the FastAPI lifespan teardown stage after memory.close().

3. tests/conftest.py:

   • Added _cleanup_leaked_metrics_collector as an autouse fixture (mirroring the established _cleanup_leaked_span_recorders pattern) to guarantee per-test isolation under xdist.

4. tests/test_metrics.py:

   • Added unit test coverage for reset_metrics_collector() and cross-test fixture isolation.

---

3. Verification & Validation

3.1 Test Execution Matrix

Test Suite / Target	Concurrency	Status
tests/test_metrics.py	-n 0 (Single-process sequential)	36 passed
tests/test_reasoning_effort_explicit_config.py	-n 4 (xdist concurrent)	13 passed
Impacted provider suites (test_reasoning_effort_*, test_tool_path_*, test_llm_extra_body, test_lmstudio_*, test_tags_visibility, test_metrics)	-n 4 (xdist concurrent)	202 passed

3.2 Pre-commit Checks

• lint.sh: All lints passed.
• vulture & knip: Passed.

[strix-security]

Strix Security Review

Warning

This pull request has 2 commits after the last Strix review (7679a1f). Strix has not reviewed these changes.
Automatic review on push is off for this repository. To review the latest changes, tag @strix-security in a comment, or turn on re-review on push.

No security issues found.

_{Updated for 7679a1f.}

---

Reviewed by Strix
_{Re-run review · Configure security review settings}

[ebarkhordar]

I ran the new ordered pair against 1d8bf11c in a clean python:3.11-slim container. It passes, but it cannot fail for the reason its docstring gives: the fixture resets the global to NoOpMetricsCollector at setup, so this assert is satisfied by the setup half whether or not the teardown restore ever runs.

Four runs of test_creates_collector_without_explicit_reset plus this test, -p no:randomly -n 0, each editing only tests/conftest.py:

as shipped                                     2 passed
teardown restore replaced with `pass`          2 passed
setup reset line deleted, restore kept         2 passed
both halves removed                            1 failed, 1 passed

So either half alone keeps the pair green, and the xdist_group pinning is not buying the ordering guarantee it describes. An autouse fixture that also resets at setup cannot be observed by a sibling test, so nothing scheduled after the leaking test can pin the restore.

The smallest honest edit is to drop test_creates_collector_without_explicit_reset, test_collector_is_noop_in_subsequent_test and the xdist_group marker, and keep test_create_and_reset_metrics_collector, which does exercise reset_metrics_collector() directly. If you want the fixture itself pinned, pytester running a two-test session in a subprocess is the mechanism that can actually see the teardown.

Separately, #3800 is open against the same issue and adds its own autouse fixture to the same region of tests/conftest.py, so whichever lands second will conflict there.

[Sanderhoff-alt]

Good catch and great analysis on the setup fixture behavior! Updated to drop the redundant paired isolation tests and the xdist_group marker, keeping test_create_and_reset_metrics_collector in 749024c. Thanks!

[ebarkhordar]

Thanks for the quick turnaround. 749024c looks right to me: test_create_and_reset_metrics_collector calls reset_metrics_collector() directly, which is the part a sibling test structurally could not observe.