Skip to content

Fix asyncio reentrant-lock deadlock in monitoring#8

Open
akeyx wants to merge 19 commits into
vinit-devops:mainfrom
akeyx:fix/monitoring-reentrant-lock-deadlock
Open

Fix asyncio reentrant-lock deadlock in monitoring#8
akeyx wants to merge 19 commits into
vinit-devops:mainfrom
akeyx:fix/monitoring-reentrant-lock-deadlock

Conversation

@akeyx

@akeyx akeyx commented Jun 17, 2026

Copy link
Copy Markdown

Summary

The health_check tool hangs forever because of a non-reentrant asyncio.Lock deadlock in monitoring.py.

ConnectionMonitor.get_all_connections_status() and MetricsCollector.get_all_metrics() acquire their asyncio.Lock and then, while still holding it, call get_connection_status() / get_metric(), which try to acquire the same lock again. asyncio.Lock is not reentrant, so the second acquire blocks on a lock the same task already holds, deadlocking permanently.

This surfaces through get_comprehensive_status() (used by health_check), which calls get_all_connections_status() both directly and via the connection-monitor health check.

Fix

Snapshot the keys under the lock, release it, then call the per-item helpers (which re-acquire the lock) outside the critical section.

Verification

Isolation repro confirmed both methods deadlocked before the change and return immediately after:

[ 0.00s] OK    get_all_connections_status() -> {'x': {...}}
[ 0.00s] OK    get_all_metrics() -> {'c': {...}}

Notes

  • Diff also includes repo-standard formatting applied by the project's own black/isort/whitespace pre-commit hooks.
  • mypy and flake8 hooks were skipped on commit because both are broken on the current Python (mypy: types-all depends on the yanked types-pkg-resources; flake8/pyflakes: module 'ast' has no attribute 'Str'). Neither is related to this change.

nicolagi and others added 19 commits February 25, 2026 13:16
When search_logs is called without a time_zone, the SearchRequest model
sets time_zone=None. This None value gets passed into search_params as
timeZone=None. The validator then tries to type-check None as a string,
which raises an APIParameterError.

The fix skips None values during validation so they are treated as
"not provided", allowing the default value (UTC) to be applied in the
subsequent defaults loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Sumo Logic search jobs API (/api/v1/search/jobs) requires time
values as epoch milliseconds, not ISO 8601 strings. Sending ISO format
results in: "The 'from' field contains an invalid time."

This changes to_sumo_api_format and to_sumo_time_format to output epoch
milliseconds, and adds epoch time pattern validation to the API
validator so it accepts the format it now produces.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The search results endpoint always fetched /messages, which fails for
aggregate queries (count by, sum by, etc.) with "requireRawMessages is
false". Aggregate query results are served from /records instead.

This changes get_search_results to try /records first, then fall back
to /messages. Both endpoints are handled gracefully when empty, so
queries that match nothing return an empty result set instead of
raising an error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The tool layer passes filter_type to the API client, but the client
method didn't accept it. Add the parameter and filter client-side
since the Sumo Logic API doesn't support it as a query param.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Sumo Logic Collector API does not support filter_type as a query
parameter. Remove it from the tool schema, tool method, and API client
rather than faking it with client-side filtering.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two changes to the sumologic-api circuit breaker:

1. resilience.py: reset failure_count to 0 on a success in CLOSED
   state. Previously failure_count was monotonic in CLOSED state, so
   any process accumulating ~5 transient errors over its lifetime
   was guaranteed to trip the breaker eventually — "5 ever" rather
   than "5 consecutive." That's the wrong semantics for an MCP
   server doing dozens of small queries per session.

2. api_client.py: narrow expected_exception from bare Exception to
   the same transient set the retry layer already uses (APIError,
   RateLimitError, TimeoutError, ConnectionError, OSError,
   httpx.RequestError). User errors like 4xx-from-bad-query no
   longer count toward opening the breaker. Updated
   CircuitBreakerConfig.expected_exception's type annotation to
   reflect that a tuple is supported (Python's except clause
   accepts either form at runtime).

Validated with three async test cases:
- failure_count resets on success in CLOSED state
- non-transient exceptions are propagated but don't count
- 3 consecutive transient errors still open the breaker (intended
  behavior preserved)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… API

Previously get_monitor_status called non-existent status endpoints. Derive
status from the Monitors Management API instead:

- Single monitor: read /api/v1/monitors/{id} and normalize its status array.
- All monitors: delegate to list_monitors (folder recursion) and build a
  status record per monitor, skipping folder entries.

Add helpers _unwrap_monitor_item, _normalize_monitor_status (maps the raw
status array + isDisabled to Normal/Triggered/Disabled/Unknown with a
severity ranking), and _build_monitor_status_record. Timing fields
(lastTriggered/triggerCount24h/last|nextEvaluation) are reported as
unavailable since the API does not expose live evaluation data.

Also:
- Fix LogRecord 'name' attribute collision in get_monitor logging.
- Remove unreachable dead code in the tool-layer get_monitor_status.
- Add 15 unit tests for the status normalization helpers.
- Track tests/test_*.py despite the broad test_*.py ignore rule.
Replace deprecated @validator with @field_validator (and
@model_validator(mode="after") for cross-field consistency checks),
swap class-based Config for model_config = ConfigDict(...), drop
non-functional env= kwargs from Field definitions, and use min_length
in place of min_items.

Resolves all PydanticDeprecatedSince20 warnings; full test suite passes
with -W error::DeprecationWarning.
feat(monitors): implement get_monitor_status from the Monitors Management API
fix: migrate Pydantic v1 syntax to v2 to silence deprecation warnings
- Implement execute_raw_request on SumoLogicAPIClient to support raw requests using configured authentication, resilience, and rate-limiting

- Add api_call MCP tool to APITools and register it in server.py

- Add api subcommand to main.py supporting optional -X/--method switch (defaulting to GET), params, headers, and request body

- Write unit tests in test_api_tools.py

- Document tool and CLI subcommand in README.md
feat: add generic api command and api_call MCP tool
…point gracefully

- Add nonlocal search_fields declaration inside _search_monitors_impl to prevent UnboundLocalError

- Handle 404 in get_monitor_history gracefully by returning empty data and warning metadata since public Sumo Logic REST APIs do not support it

- Make endpoint validation in config.py lenient by stripping trailing /api, /api/v1, /api/v2 paths and trailing slashes

- Accept list response formats for search alerts fallback without triggering warning

- Add unit tests in test_config.py
fix: resolve monitor tools scope issue and handle missing history endpoint gracefully
feat: Add folder management MCP tools and client endpoints
ConnectionMonitor.get_all_connections_status() and
MetricsCollector.get_all_metrics() acquired their non-reentrant
asyncio.Lock and then, while still holding it, called
get_connection_status() / get_metric() which try to acquire the same
lock again. asyncio.Lock is not reentrant, so these calls deadlocked
forever.

This hung the health_check tool, since get_comprehensive_status()
invokes get_all_connections_status() (directly and via the
connection_monitor health check).

Fix: snapshot the keys under the lock, then release it before calling
the per-item helpers that re-acquire it.
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.

2 participants