fix(cron): avoid live adapter reuse across event loops - #62956
Conversation
8c94557 to
d7bf2ad
Compare
d7bf2ad to
c65a104
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the adapter ownership concern. The current patch is not wired into the Matrix delivery path it describes.
Problems
- Current main routes Matrix sends through
_send_matrix_via_adapterintools/send_message_tool.py:913-927. That helper directly reuses the live adapter attools/send_message_tool.py:1693-1700, while this PR changes generic_send_via_adapterinstead. The new test directly calls the generic helper, so it does not cover the actual Matrix route. - Gateway cron startup passes the gateway loop to the scheduler (
gateway/run.py:20831-20845), and live cron delivery is scheduled onto that loop incron/scheduler.py:1627-1713. The standard gateway path therefore does not establish the reported cross-loop call into this generic helper.
Suggested changes
- Put any foreign-loop guard in
_send_matrix_via_adapterbefore its live-adapter branch, and test via_send_to_platform(Platform.MATRIX, ...). - Add both foreign-loop fallback and same-loop reuse coverage on that real path; then reassess whether the standalone
aiohttptimeout change has a reachable caller.
Automated hermes-sweeper review.
| "Skipping live %s adapter from a different event loop; using standalone sender", | ||
| platform_name, | ||
| ) | ||
| adapter = None |
There was a problem hiding this comment.
This generic fallback is not reached for Matrix: _send_to_platform dispatches Platform.MATRIX to _send_matrix_via_adapter first, and that helper still directly awaits its live adapter at current-main tools/send_message_tool.py:1693-1700. Move the loop-ownership decision to that Matrix-specific branch and test through _send_to_platform.
8cf1932 to
a755ad9
Compare
44bec38 to
9f87447
Compare
9f87447 to
47d1511
Compare
|
Sorry about that, still tweaking the permissions and rules on this agent. Launched the PR without finishing our live test 🤦 I reworked it accordingly:
|
Five tests for the loop-mismatch behavior added in the parent commit: same-loop direct send, foreign-loop bridge onto the adapter's own loop, send-error surfacing as a dict cron can inspect, unknown-loop legacy fallback, and detection-failure safety. Patterns adapted from NousResearch#62956 and NousResearch#68359 (fake live adapter reporting a session loop; thread-backed foreign loop).
Summary
Cron delivery can reuse the live Matrix adapter from a different asyncio event loop. Because the adapter owns an
aiohttpsession, this can fail with:The failure was reproduced in a live Hermes gateway during manual execution of a Matrix-delivered cron job.
Root cause
There are two cron delivery paths involved:
cron/scheduler.pyroutes gateway cron delivery throughDeliveryRouterand selects the live adapter by default._send_to_platform(Platform.MATRIX, ...)and then_send_matrix_via_adapter().Both paths could reuse a gateway-owned Matrix adapter from cron execution without establishing that the adapter’s HTTP session belonged to the current event loop.
The original revision of this PR changed the generic
_send_via_adapter()helper, but review and live tracing showed that this was not the Matrix path. That change has been removed from this revision.Fix
cron/scheduler.py, cron execution no longer selects a liveruntime_adapter; it uses the standalone/ephemeral delivery path instead._send_matrix_via_adapter():The reconnect/liveness problem for an already-connected Matrix gateway remains outside this PR.
Testing
The focused Matrix/send-message suite passes:
Coverage includes the real
_send_to_platform(Platform.MATRIX, ...)route for:Also verified:
git diff --check;47d1511;theo-daily-curiosityrecordedlast_status: okwithlast_delivery_error: null;Tested on Linux with the repository's Hermes virtual environment.
Related work
This is the Matrix/generic cron counterpart of the event-loop mismatch documented in #18014, although that issue is WeChat-specific and already resolved on current main.
The Matrix cron delivery architecture is related to #3767 and #5271.
Scope
This PR fixes cross-event-loop reuse during cron delivery. It does not implement Matrix reconnect supervision after a dead sync/session; that is a separate failure mode and should be reviewed independently.