fix(gateway): carry chat_id/thread_id/session_key into /branch child sessions too
Same defect as the compression-rotation fix in the prior commit, found during a full-audit of every create_session() call site per the repo's 'fix the whole bug class, sibling call paths included' contribution guidance. _handle_branch_command() (gateway/slash_commands.py) creates the branched child session via create_session() without chat_id/chat_type/thread_id. The routing columns are only backfilled later, when switch_session() runs at the end of the function and calls _record_gateway_session_peer(). In between, the function copies the parent's conversation history to the new session_id one message at a time, with each append_message() call independently try/excepted (best-effort) — a crash/kill anywhere in that window leaves the branched session permanently unroutable, same failure mode as the compression bug: NULL chat_id/thread_id can never be found by find_latest_gateway_session_for_peer, AND unreachable via /resume's IDOR guard (which requires the row's chat_id/thread_id to match the caller's). Fix: forward source.chat_id/chat_type/thread_id at create_session() time, mirroring the existing correct pattern already used by /title's auto-create path a few hundred lines up in the same file (which has an explicit IDOR-scoping comment justifying it). Tests: tests/gateway/test_branch_routing_columns.py drives the real _handle_branch_command against a real SessionStore + SessionDB (SQLite in tmp_path, no DB/session-store mocks). Patches switch_session to simulate a crash landing before it runs (the exact gap the routing columns need to survive), then asserts the branched child's chat_id/chat_type/thread_id are already correct in state.db at that point. RED verified against unpatched code (assert None == '170829464'), GREEN after the fix. Regression: 102/102 across the new test + pre-existing /branch, session boundary, compression rotation, DM thread seeding, session API, and resume-command suites. Broader tests/gateway/ -k "branch or session_api or resume or topic_mode or session_boundary" sweep: 255/255 passed, 1 (unrelated) skip.
This commit is contained in:
jcjc81
Teknium
parent
3e05d7abf1
commit
3cbeeaa175
|
|
@ -4786,6 +4786,22 @@ class GatewaySlashCommandsMixin:
|
|||
model=(self.config.get("model", {}) or {}).get("default") if isinstance(self.config, dict) else None,
|
||||
model_config={"_branched_from": parent_session_id},
|
||||
parent_session_id=parent_session_id,
|
||||
# Gateway routing columns (#NNNNN): forward the caller's own
|
||||
# chat_id/chat_type/thread_id at CREATE time, same fix as
|
||||
# the compression-rotation bug in
|
||||
# agent/conversation_compression.py. Without this, the
|
||||
# branched child row has NULL routing columns until
|
||||
# switch_session() below calls _record_gateway_session_peer()
|
||||
# — a crash/kill anywhere between here and there (most
|
||||
# plausibly mid-history-copy, since each append_message call
|
||||
# a few lines down is independently best-effort) leaves the
|
||||
# branch permanently unroutable: unreachable by chat/thread
|
||||
# lookup, and unreachable via /resume's IDOR guard too
|
||||
# (which requires the row's chat_id/thread_id to match the
|
||||
# caller's).
|
||||
chat_id=source.chat_id,
|
||||
chat_type=source.chat_type,
|
||||
thread_id=source.thread_id,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error("Failed to create branch session: %s", e)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,133 @@
|
|||
"""Regression test for /branch losing gateway routing columns (#NNNNN).
|
||||
|
||||
``_handle_branch_command`` (gateway/slash_commands.py) creates the branched
|
||||
child session via ``create_session()`` WITHOUT chat_id/chat_type/thread_id —
|
||||
identical in shape to the compression-rotation bug fixed in
|
||||
agent/conversation_compression.py. The routing columns are only written
|
||||
later, when ``switch_session()`` calls ``_record_gateway_session_peer()``
|
||||
after the branch's session_id is live and its transcript has been copied
|
||||
message-by-message.
|
||||
|
||||
A crash/kill landing between create_session() and switch_session() (most
|
||||
plausibly mid-history-copy on a long conversation, since each
|
||||
append_message call is independently try/excepted and best-effort) leaves
|
||||
the branched session permanently unroutable: NULL chat_id/thread_id can
|
||||
never be found by find_latest_gateway_session_for_peer, and the /resume
|
||||
IDOR guard (which requires the row's chat_id/thread_id to match the
|
||||
caller's) can never authorize a manual recovery either.
|
||||
|
||||
This test drives the REAL _handle_branch_command against a REAL SessionStore
|
||||
+ SessionDB (SQLite in a tmp_path, no mocks on the DB/session-store layer)
|
||||
and asserts the branched child's routing columns are present in state.db
|
||||
immediately after create_session() returns — before switch_session() ever
|
||||
runs — closing the gap the same way the compression fix does.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import GatewayConfig, Platform
|
||||
from gateway.platforms.base import MessageEvent
|
||||
from gateway.session import SessionSource, SessionStore, build_session_key
|
||||
from hermes_state import AsyncSessionDB, SessionDB
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def store(tmp_path, monkeypatch):
|
||||
"""Real SessionStore backed by a real SessionDB (SQLite in tmp_path)."""
|
||||
import hermes_state
|
||||
|
||||
monkeypatch.setattr(hermes_state, "DEFAULT_DB_PATH", tmp_path / "state.db")
|
||||
config = GatewayConfig()
|
||||
return SessionStore(sessions_dir=tmp_path, config=config)
|
||||
|
||||
|
||||
def _make_source() -> SessionSource:
|
||||
return SessionSource(
|
||||
platform=Platform.TELEGRAM,
|
||||
user_id="170829464",
|
||||
chat_id="170829464",
|
||||
chat_type="dm",
|
||||
thread_id="544520",
|
||||
)
|
||||
|
||||
|
||||
def _make_event(text: str) -> MessageEvent:
|
||||
return MessageEvent(text=text, source=_make_source(), message_id="m1")
|
||||
|
||||
|
||||
def _make_branch_runner(store: SessionStore):
|
||||
"""Minimal GatewayRunner stub wired to a REAL session_store/session_db."""
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.adapters = {}
|
||||
runner.config = {}
|
||||
runner._background_tasks = set()
|
||||
runner._running_agents = {}
|
||||
runner._running_agents_ts = {}
|
||||
runner._busy_ack_ts = {}
|
||||
runner._pending_approvals = {}
|
||||
runner._update_prompt_pending = {}
|
||||
runner._agent_cache_lock = None
|
||||
runner.session_store = store
|
||||
runner._session_db = AsyncSessionDB(store._db)
|
||||
runner._pending_skills_reload_notes = {}
|
||||
return runner
|
||||
|
||||
|
||||
class TestBranchRoutingColumns:
|
||||
@pytest.mark.asyncio
|
||||
async def test_branched_session_has_routing_columns_before_switch(self, store):
|
||||
"""Simulates a crash/kill landing between create_session() and
|
||||
switch_session() — the exact gap where the branched child's
|
||||
routing columns are missing. switch_session() runs unconditionally
|
||||
at the end of _handle_branch_command and backfills chat_id/
|
||||
chat_type/thread_id via _record_gateway_session_peer(), so checking
|
||||
DB state AFTER the function returns successfully will always look
|
||||
fine. The real bug only bites if the process dies mid-function,
|
||||
before switch_session() gets a chance to run — so we patch
|
||||
switch_session to simulate exactly that crash point, then inspect
|
||||
the child row switch_session would otherwise have fixed up.
|
||||
"""
|
||||
source = _make_source()
|
||||
parent_entry = store.get_or_create_session(source)
|
||||
store._db.append_message(parent_entry.session_id, role="user", content="hello")
|
||||
store._db.append_message(parent_entry.session_id, role="assistant", content="world")
|
||||
|
||||
runner = _make_branch_runner(store)
|
||||
|
||||
captured_new_session_id = {}
|
||||
real_switch_session = store.switch_session
|
||||
|
||||
def _crash_before_switch(session_key, target_session_id):
|
||||
# Simulate the process dying right here — before routing gets
|
||||
# backfilled — by capturing the id and raising instead of
|
||||
# forwarding to the real switch_session().
|
||||
captured_new_session_id["id"] = target_session_id
|
||||
raise RuntimeError("simulated crash before switch_session")
|
||||
|
||||
import unittest.mock as mock
|
||||
|
||||
with mock.patch.object(store, "switch_session", side_effect=_crash_before_switch):
|
||||
with pytest.raises(RuntimeError, match="simulated crash"):
|
||||
await runner._handle_branch_command(_make_event("/branch"))
|
||||
|
||||
new_session_id = captured_new_session_id["id"]
|
||||
assert new_session_id != parent_entry.session_id
|
||||
|
||||
row = store._db.get_session(new_session_id)
|
||||
assert row is not None, "branched child row must exist in state.db"
|
||||
# THIS is the bug: without the fix, these are all None, because
|
||||
# create_session() at branch time never received them — only the
|
||||
# (now-crashed) switch_session() call would have backfilled them.
|
||||
assert row["chat_id"] == "170829464", (
|
||||
"branched session lost chat_id — unroutable if a crash lands "
|
||||
"between create_session() and switch_session()"
|
||||
)
|
||||
assert row["chat_type"] == "dm"
|
||||
assert row["thread_id"] == "544520"
|
||||
|
||||
_ = real_switch_session # silence unused
|
||||
|
||||
Loading…
Reference in New Issue