Skip to content

fix(tts): preserve Telegram voice bubble when session ContextVar is cleared - #37027

Closed
chromalinx wants to merge 2 commits into
NousResearch:mainfrom
chromalinx:fix/tts-telegram-opus-contextvar-clean
Closed

fix(tts): preserve Telegram voice bubble when session ContextVar is cleared#37027
chromalinx wants to merge 2 commits into
NousResearch:mainfrom
chromalinx:fix/tts-telegram-opus-contextvar-clean

Conversation

@chromalinx

Copy link
Copy Markdown
Contributor

What this PR does

A tool worker thread that runs without the gateway's session ContextVar (a follow-up turn triggered from inside a tool — auto-skill review, memory sweep, cron-style background — that bypasses propagate_context_to_thread) used to make the TTS tool's platform check return empty and skip the MP3→Opus ffmpeg conversion. The result was an .mp3 audio attachment on Telegram instead of a voice bubble.

Two changes restore the bubble:

  1. gateway/session_context.set_session_vars now mirrors the active session vars to os.environ for non-empty values. The mirror outlives the request, which is the desired outcome for the background-turn case.
  2. tools/tts_tool.text_to_speech_tool resolves HERMES_SESSION_PLATFORM from get_session_env first, then falls back to os.environ. The ContextVar stays the source of truth for concurrency-safe access; os.environ only fills the gap when a worker thread has lost ContextVar inheritance.

A regression test (tests/tools/test_tts_opus_routing.py) covers the clear_session_vars + os.environ mirror combination.

3 files changed, +145 / -1.

…leared

When a tool worker thread runs without the gateway's session ContextVar
(e.g. a follow-up turn triggered from inside a tool \u2014 auto-skill review,
memory sweep, cron-style background \u2014 that bypasses
propagate_context_to_thread), the TTS tool's platform check used to
return "" and skip the MP3\u2192Opus ffmpeg conversion. The result was an
.mp3 audio attachment on Telegram instead of a voice bubble.

Two changes restore the bubble:

1. gateway/session_context.set_session_vars now mirrors the active
   session vars to os.environ as a side effect. The mirror is only
   written for non-empty values so unrelated processes inheriting the
   env don't get a stale platform pinned. clear_session_vars keeps its
   current behavior (does not clear the env mirror) \u2014 the env copy
   outlives the request, which is the desired outcome for the
   background-turn case the fix targets.

2. tools/tts_tool.text_to_speech_tool now resolves HERMES_SESSION_PLATFORM
   from get_session_env first, then falls back to os.environ. This
   keeps the ContextVar the source of truth for concurrency-safe access
   while letting worker threads that lost the ContextVar still see the
   active platform.

A regression test in test_tts_opus_routing.py covers the
clear_session_vars + os.environ mirror combination (the exact state a
post-turn background TTS call would observe).
@liuhao1024

Copy link
Copy Markdown
Contributor

I found one issue that looks worth fixing before merge.

gateway/session_context.py — the PR writes session context to os.environ in set_session_vars(), which re-introduces the exact process-global race condition that session_context.py was designed to prevent. The module docstring explicitly documents this:

Because os.environ is process-global, Message A's value was silently overwritten by Message B before Message A's agent finished running.

The PR's os.environ writes are subject to the same race. When two sessions run concurrently (e.g., Telegram + Discord), the second session's set_session_vars overwrites HERMES_SESSION_PLATFORM before the first session's TTS tool reads it. The first session's clear_session_vars resets the ContextVar to "", then the TTS tool falls back to os.environ and picks up the second session's platform value.

Additionally, clear_session_vars does not clean up the os.environ values this PR adds, so stale values persist after the session handler exits.

Why it matters: Telegram voice bubbles silently degrade to MP3 when a concurrent Discord session overwrites the platform env var. This is intermittent and hard to reproduce in single-session testing.

Suggested fix: Instead of writing to os.environ, propagate the ContextVar to worker threads that bypass propagate_context_to_thread. Python's contextvars.copy_context() captures all ContextVars at spawn time. Alternatively, the TTS tool could accept the platform as an explicit parameter from the caller (like the vision tool PR #37028 does for provider/model) rather than reading it from global state.

@chromalinx

Copy link
Copy Markdown
Contributor Author

Fixed in d015fa1. The previous patch mirrored session vars to os.environ in set_session_vars to keep worker threads working - but that re-introduced the exact process-global race that session_context.py was built to prevent, and the values leaked past clear_session_vars too.\n\nNew behaviour:\n\n* set_session_vars no longer writes to os.environ.\n* text_to_speech_tool no longer falls back to os.environ when the ContextVar is empty.\n* The platform is resolved exclusively from the per-task ContextVar (race-free by construction).\n* The regression test in tests/tools/test_tts_opus_routing.py is updated to cover the exact race scenario you described: two concurrent sessions, the cleared one stays on MP3 even when the other session has HERMES_SESSION_PLATFORM set in os.environ.\n\nTrade-off: a tool worker thread that has lost ContextVar inheritance and happens to invoke TTS will no longer produce an Opus bubble. That scenario was the original motivation for the mirror, but it is much rarer than the cross-session race and only happens for background follow-up turns that need voice output.

@alt-glitch alt-glitch added type/bug Something isn't working tool/tts Text-to-speech and transcription platform/telegram Telegram bot adapter P2 Medium — degraded but workaround exists labels Jun 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: overlaps the existing cluster of open TTS->Telegram-voice-bubble fixes (#28793, #31937, #32539, #34779), all rooted in HERMES_SESSION_PLATFORM being cleared/unset before the TTS step. This PR targets a distinct trigger (background-turn ContextVar loss) but the fixes interact — worth coordinating so they don't conflict.

@chromalinx

chromalinx commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for flagging the cluster — checked all 4 before opening. Quick read of the overlap and where PR #37027 fits:

PR Trigger Touches
#28793 _clear_session_env() runs before adapter.send() → auto-TTS sees empty platform gateway/platforms/base.py, tools/tts_tool.py
#31937 get_session_platform() not bound during auto-TTS from gateway gateway/platforms/base.py, gateway/run.py, gateway/session_context.py
#32539 send_voice_reply always sends as document, not voice note gateway/platforms/base.py, gateway/run.py
#34779 session context not bound when auto-TTS runs in the platform send pipeline gateway/platforms/base.py
#37027 (this) os.environ write in set_session_vars() races between concurrent sessions; thread workers that bypass propagate_context_to_thread lose the ContextVar tools/tts_tool.py

Distinct trigger: the other 4 are "scope/binding" bugs (auto-TTS path runs without the session context in scope). This PR is a "race/state" bug — even when the context IS in scope, two concurrent sessions can stomp each other via os.environ. Liuhao1024 caught the race in review; d015fa1 resolves it by removing the os.environ mirror entirely and reading the platform exclusively from the per-task ContextVar.

Trade-off: a tool worker thread that bypasses propagate_context_to_thread and invokes TTS will now silently produce MP3 instead of Opus. I called that out in the d015fa1 commit message. alt-glitch, if you want a follow-up that handles that case, the options I see are (a) contextvars.copy_context() at the gateway boundary so spawn_pool threads inherit the ContextVar, or (b) accept the platform as an explicit TTS arg from the caller (same shape as #37028 for vision). Happy to layer it on.

No file conflict in gateway/platforms/base.py (shared surface in 3 of 4 other PRs) because this PR does not touch it. Cleanest merge order: land this first, then the binding fixes will see a race-free get_session_platform() to bind. Happy to rebase if any of those PRs want to consume the new shape.

@chromalinx

Copy link
Copy Markdown
Contributor Author

@alt-glitch gentle ping — addressed all feedback. Cluster overlap noted and tabulated (you + #28793/#31937/#32539/#34779). Trade-off on the cross-session race vs. worker-thread ContextVar loss called out explicitly. No file conflict in gateway/platforms/base.py.

Test fixes pushed to the PR branches. Happy to rebase/adjust if anything still blocks merge.

@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the open TTS→Telegram voice-bubble fix cluster (#28793, #31937, #32539), all rooted in HERMES_SESSION_PLATFORM being cleared/unset before the TTS step. This PR resolves platform from get_session_env with an os.environ fallback for worker threads that lost ContextVar inheritance (the earlier set_session_vars os.environ mirror that re-introduced a process-global race was reverted in d015fa1). Competing approach — maintainer picks the canonical fix.

@teknium1

Copy link
Copy Markdown
Contributor

Closing as implemented on main. This is an automated hermes-sweeper review.

  • b3b88a279b970c20d83ad8003a1e96e6a5fb0f76 introduced the sentinel-based session-context behavior: after clear_session_vars(), get_session_env() returns the explicit empty ContextVar value rather than falling back to stale process-global environment state (gateway/session_context.py:217-327).
  • TTS already resolves its platform through that helper (tools/tts_tool.py:2200-2202).
  • The existing regression coverage verifies the same cleared-context/no-environment-fallback invariant (tests/gateway/test_session_env.py:124-139).
  • Current concurrent tool dispatch also propagates the parent ContextVars into worker threads (agent/tool_executor.py:681-687, tools/thread_context.py:64-105).

Thanks for the focused race analysis and for addressing the environment-mirror concern in the follow-up commit.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 13, 2026
@chromalinx
chromalinx deleted the fix/tts-telegram-opus-contextvar-clean branch July 19, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:implemented-on-main Sweeper: behavior already present on current main tool/tts Text-to-speech and transcription type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants