Skip to content

fix(mattermost): avoid aiohttp timeout context in sends - #39877

Closed
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix-mattermost-cron-timeout-task
Closed

fix(mattermost): avoid aiohttp timeout context in sends#39877
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix-mattermost-cron-timeout-task

Conversation

@yinkev

@yinkev yinkev commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove aiohttp.ClientTimeout from Mattermost send, upload, and download paths
  • keep bounded request behavior by wrapping Mattermost HTTP operations in asyncio.wait_for()
  • cover gateway MattermostAdapter.send() and standalone cron delivery so scheduled Mattermost sends do not hit aiohttp's timeout task-context error

Fixes #39836.

Tests

  • .venv/bin/python -m pytest tests/gateway/test_mattermost.py::TestMattermostSend tests/tools/test_send_message_missing_platforms.py::TestSendMattermost -q -o addopts= --tb=short
  • .venv/bin/python -m pytest tests/gateway/test_mattermost.py tests/tools/test_send_message_missing_platforms.py tests/gateway/test_send_multiple_images.py::TestMattermostMultiImage tests/gateway/test_media_download_retry.py::TestMattermostSendUrlAsFile -q -o addopts= --tb=short
  • .venv/bin/python -m py_compile plugins/platforms/mattermost/adapter.py tests/gateway/test_mattermost.py tests/tools/test_send_message_missing_platforms.py
  • /opt/homebrew/bin/ruff check plugins/platforms/mattermost/adapter.py tests/gateway/test_mattermost.py tests/tools/test_send_message_missing_platforms.py
  • git diff --check

Duplicate check

  • gh search prs --repo NousResearch/hermes-agent --state open '39836 Mattermost ClientTimeout timeout context cron'
  • gh search prs --repo NousResearch/hermes-agent --state open 'Mattermost "Timeout context manager should be used inside a task"'

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins labels Jun 5, 2026
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026

@teknium1 teknium1 left a comment

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.

Thanks for tracing the standalone Mattermost delivery path.

Problems

  • The reported no-task mechanism is not demonstrated on current main. Cron executes the standalone sender through asyncio.run() in cron/scheduler.py:1902-1905; the submitted standalone test likewise calls it through asyncio.run() in tests/tools/test_send_message_missing_platforms.py:108.
  • The new tests assert that ClientTimeout is not called, but do not reproduce the reported error or exercise the cron dispatch path.
  • The issue identifies _standalone_send, while this diff also changes live gateway helpers, downloads, uploads, and the adapter session timeout in plugins/platforms/mattermost/adapter.py.

Suggested changes

  • Add a regression that reproduces the failure through the real cron standalone-delivery path.
  • If confirmed, scope the fix to _standalone_send and preserve the existing bounded timeout there; keep live gateway timeout behavior unchanged unless independently reproduced.

Automated hermes-sweeper review.

@@ -166,7 +166,8 @@ async def test_send_calls_api_post(self):

self.adapter._session.post = MagicMock(return_value=mock_resp)

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.

This only asserts that ClientTimeout is absent. Please add a regression that reproduces the reported cron standalone-delivery failure; the existing standalone test already uses asyncio.run(), so it does not establish the claimed missing-task context.

@yinkev

yinkev commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing this broad implementation because the reported mechanism is not reproducible on the current cron path: scheduler delivery enters through asyncio.run(), which creates a task, and a fresh aiohttp ClientSession with ClientTimeout works in that context. The patch also changes gateway upload/download/session timeouts beyond the reported standalone sender. A future fix needs a current-main cron-path regression that reproduces the actual failure first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Mattermost cron job delivery fails with "Timeout context manager should be used inside a task"

3 participants