fix(gateway): rebind Telegram DM topic on every session-switch surface - #106227
jean-luc2305 wants to merge 1 commit into
Conversation
Topic mode persists a (chat_id, thread_id) -> session_id row in telegram_dm_topic_bindings so reopening a topic resumes the right Hermes session and the retire/archive helpers resolve the correct topic to delete. /new already pairs its switch with _record_telegram_topic_binding, but four OTHER surfaces call SessionStore.switch_session WITHOUT the paired rebind, so the binding goes stale (keeps pointing at the old session) or is never written: 1. /resume (_handle_resume_command) 2. /branch (_handle_branch_command) 3. CLI-handoff worker (_process_handoff) 4. async-delegation completion (_resolve_async_delegation_session) A stale binding is a retire-safety hazard: the archiver resolves its delete target from the binding row, so a wrong row means deleting the wrong Telegram topic. It also drove one thread 15 sessions deep on a single stale row via the handoff worker. Fix: add GatewayTopicThreadsMixin._rebind_telegram_topic_after_switch (guarded by _is_telegram_topic_lane, off-loop via asyncio.to_thread, best-effort like the /new path) and call it after switch_session in all four surfaces. The async-delegation compression branch (advance_compression_session) already syncs the binding, so only its plain switch_session path is rebound. No schema change. Adds tests/gateway/test_telegram_topic_binding_switch_surfaces.py driving each surface against a real SessionStore + SessionDB with topic mode enabled and asserting the binding follows the switch.
andrexibiza
left a comment
There was a problem hiding this comment.
The switch-surface coverage is pointed at the right class, but the new helper still leaves the retire-safety invariant fail-open. _rebind_telegram_topic_after_switch() catches every persistence failure, logs it, and returns after the route has already moved. The PR itself establishes that a stale (chat_id, thread_id) -> session_id row is mutation authority for archive/delete and can therefore target the wrong Telegram topic. Once switch_session succeeds, silently retaining the old durable binding is not a best-effort telemetry failure; it is a split-brain owner state.
Please make the paired transition settlement-safe: either the rebind must succeed before the switch surface publishes success, or a failed rebind must explicitly invalidate/fence the stale binding so it cannot be consumed as delete/resume authority. Add a failure-path regression that forces _record_telegram_topic_binding to fail after the routing move and proves the old binding cannot remain actionable. The four GREEN happy-path tests currently cannot detect this class.
Related: #58850 (issue: |
Summary
Topic mode persists a
(chat_id, thread_id) -> session_idrow intelegram_dm_topic_bindingsso reopening a Telegram DM topic resumes the right Hermes session, and so the retire/archive helpers resolve the correct topic to delete./newalready pairs itsswitch_sessionwith_record_telegram_topic_binding(regression:test_new_inside_telegram_topic_rewrites_binding_to_new_session), but four other surfaces callSessionStore.switch_sessionWITHOUT the paired rebind, so the binding goes stale (keeps pointing at the old session) or is never written:/resume—_handle_resume_command(gateway/slash_commands_session.py)/branch—_handle_branch_command(gateway/slash_commands_session.py)_process_handoff(gateway/run_startup.py)_resolve_async_delegation_session(gateway/run_notifications.py)A stale binding is a retire-safety hazard: the archiver resolves its delete target from the binding row, so a wrong row means deleting the wrong Telegram topic. In practice it also drove one thread 15 sessions deep on a single stale row via the handoff worker.
Fix
Add
GatewayTopicThreadsMixin._rebind_telegram_topic_after_switch(source, session_entry)— guarded by_is_telegram_topic_lane(no-op outside a Telegram DM topic lane), run off-loop viaasyncio.to_thread, best-effort like the/newpath — and call it afterswitch_sessionin all four surfaces.The async-delegation compression branch (
advance_compression_session) already syncs the binding, so only its plainswitch_sessionpath is rebound. No schema change.Tests
Adds
tests/gateway/test_telegram_topic_binding_switch_surfaces.py, which drives each of the four surfaces against a realSessionStore+SessionDB(SQLite in tmp_path, topic mode enabled) and asserts the binding follows the switch.test_telegram_topic_mode.py+test_branch_routing_columns.pystill green; broadertests/gateway -k "topic or branch or resume or handoff or delegation or notification or slash"sweep: 518 passed, 2 skipped.Checklist
fix(gateway): ...)