diff --git a/agent/title_generator.py b/agent/title_generator.py index 4a406b22ec40..9dcdf1e8963c 100644 --- a/agent/title_generator.py +++ b/agent/title_generator.py @@ -138,6 +138,53 @@ def generate_title( return None +def _persist_session_title(session_db, session_id, title): + """Persist a generated title, recovering from duplicate-title collisions. + + The write goes through ``set_auto_title_if_empty`` (predicate + write in + one transaction) so a manual ``/title`` set while LLM generation was in + flight is never overwritten — a plain ``set_session_title`` fallback keeps + older stores working. ``set_session_title`` raises ValueError when the + title would collide with another session (the unique-title index). Rather + than swallow it and leave the session untitled (#50537), append a #N + suffix via get_next_title_in_lineage() when the store supports lineage + dedup; otherwise re-raise so the caller can decide. + + Returns the title actually persisted, or None when a concurrent manual + title won the race (nothing was written). + """ + atomic_fn = getattr(session_db, "set_auto_title_if_empty", None) + + def _set(t): + if atomic_fn is not None: + if not atomic_fn(session_id, t): + # Predicate failed: a title appeared while generation was in + # flight (manual /title wins), or the session vanished. + logger.debug( + "Skipping auto-generated session title because a title " + "was set while generation was in flight" + ) + return None + return t + ok = session_db.set_session_title(session_id, t) + if ok is False: + raise RuntimeError( + f"session {session_id} not found when storing title" + ) + return t + + try: + return _set(title) + except ValueError: + next_title_fn = getattr(session_db, "get_next_title_in_lineage", None) + if next_title_fn is None: + raise + deduped = next_title_fn(title) + if not deduped or deduped == title: + raise + return _set(deduped) + + def auto_title_session( session_db, session_id: str, @@ -237,15 +284,13 @@ def _auto_title_session( return try: - if not session_db.set_auto_title_if_empty(session_id, title): - logger.debug( - "Skipping auto-generated session title because a title was set while generation was in flight" - ) + persisted = _persist_session_title(session_db, session_id, title) + if persisted is None: return - logger.debug("Auto-generated session title: %s", title) + logger.debug("Auto-generated session title: %s", persisted) if title_callback is not None: try: - title_callback(title) + title_callback(persisted) except Exception: logger.debug("Auto-title callback failed", exc_info=True) except Exception as e: diff --git a/cron/scheduler.py b/cron/scheduler.py index 63c45d898e4d..8c52377e2134 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -48,6 +48,45 @@ from hermes_time import now as _hermes_now logger = logging.getLogger(__name__) +def _set_cron_session_title(session_db, session_id, base_title): + """Robustly title a finished cron session before it is closed. + + Centralizes the title write so the cron finally block can guarantee a + non-blank, unique title is persisted before end_session()/close() tear + the connection down (issues #50535, #50536, #50537): + + - #50535: never leaves the session blank. base_title already carries a + cron-id fallback for nameless jobs; this also guards a failed write. + - #50537: a duplicate title makes set_session_title raise ValueError (the + unique-title index). Recover by appending a #N suffix via + get_next_title_in_lineage() when supported, instead of swallowing the + error and ending up untitled. If lineage dedup is unavailable, raise. + - #50536: this runs synchronously in the cron finally block ahead of the + session close, so no in-flight title write can race the close. + + Returns the title actually persisted, or None if nothing could be set. + """ + if not session_db or not session_id: + return None + title = (base_title or "").strip() + if not title: + return None + try: + session_db.set_session_title(session_id, title) + return title + except ValueError: + # Title collision against the unique-title index. Fall back to the + # next title in the lineage (base #2, base #3, ...) when supported. + next_title_fn = getattr(session_db, "get_next_title_in_lineage", None) + if next_title_fn is None: + raise + deduped = next_title_fn(title) + if not deduped or deduped == title: + raise + session_db.set_session_title(session_id, deduped) + return deduped + + def _summarize_cron_failure_for_delivery(job: dict, error: str | None) -> str: """Return a compact one-line failure message for chat delivery. @@ -3500,18 +3539,39 @@ def run_job( for _var_name in _cron_delivery_vars: _VAR_MAP[_var_name].set("") if _session_db: - # Title the cron session from the job (name → short prompt → id) so - # sidebars/history show a meaningful label instead of the injected - # "[IMPORTANT: …]" hint that is the session's first message. Set here - # (not at create time) so the agent's own INSERT keeps model / - # system_prompt; this only UPDATEs the title column. The run-time - # suffix keeps it unique against the sessions.title index across runs. + # Title the cron session from the job (name -> id) and PERSIST it + # BEFORE end_session()/close() tear the connection down, so the + # close can never run over an in-flight title write (#50536). The + # run-time suffix keeps it unique against the sessions.title index + # across runs; _set_cron_session_title dedupes (#50537) and the + # except-fallback below guarantees a non-blank title (#50535). try: _title_base = " ".join(job_name.split())[:60].strip() or f"cron {job_id}" _cron_title = f"{_title_base} · {_hermes_now().strftime('%b %d %H:%M')}" - _session_db.set_session_title(_cron_session_id, _cron_title) + if not _set_cron_session_title(_session_db, _cron_session_id, _cron_title): + # Helper returned None (blank base) -> use the id fallback. + _set_cron_session_title( + _session_db, _cron_session_id, f"cron {job_id}" + ) except (Exception, KeyboardInterrupt) as e: - logger.debug("Job '%s': failed to set cron session title: %s", job_id, e) + logger.debug( + "Job '%s': failed to set cron session title: %s", job_id, e + ) + # Last-resort: never leave the session blank (#50535). Try the + # next free title in the lineage, then a bare id-stamped title. + for _fallback in ( + getattr(_session_db, "get_next_title_in_lineage", lambda b: b)( + f"cron {job_id}" + ), + f"cron {job_id} {_cron_session_id[-6:]}", + ): + try: + if _set_cron_session_title( + _session_db, _cron_session_id, _fallback + ): + break + except (Exception, KeyboardInterrupt): + continue try: _session_db.end_session(_cron_session_id, "cron_complete") except (Exception, KeyboardInterrupt) as e: diff --git a/tests/agent/test_title_generator.py b/tests/agent/test_title_generator.py index 0dd288d20ed8..9d9af4362724 100644 --- a/tests/agent/test_title_generator.py +++ b/tests/agent/test_title_generator.py @@ -1,5 +1,6 @@ """Tests for agent.title_generator — auto-generated session titles.""" +import pytest from unittest.mock import MagicMock, patch @@ -437,3 +438,46 @@ class TestMaybeAutoTitle: def test_skips_if_no_session_db(self): maybe_auto_title(None, "sess-1", "hello", "response", []) # no db + + +class TestAutoTitleDuplicateHandling: + """Duplicate auto-title handling and not-found hardening (#50537).""" + + def test_dedupes_duplicate_title_via_lineage(self): + db = MagicMock() + db.get_session_title.return_value = None + db.set_session_title.side_effect = [ValueError("in use"), True] + db.get_next_title_in_lineage.return_value = "Debugging Import Error #2" + with patch( + "agent.title_generator.generate_title", + return_value="Debugging Import Error", + ): + seen = [] + auto_title_session(db, "sess-1", "hi", "hello", title_callback=seen.append) + db.get_next_title_in_lineage.assert_called_once_with("Debugging Import Error") + assert db.set_session_title.call_args_list[-1][0] == ( + "sess-1", + "Debugging Import Error #2", + ) + # callback fires with the actually-persisted (deduped) title + assert seen == ["Debugging Import Error #2"] + + def test_swallows_value_error_without_lineage_support(self): + # No get_next_title_in_lineage -> ValueError propagates out of the + # persist helper but auto_title_session still swallows it (no crash). + db = MagicMock(spec=["get_session_title", "set_session_title"]) + db.get_session_title.return_value = None + db.set_session_title.side_effect = ValueError("in use") + with patch( + "agent.title_generator.generate_title", return_value="Dup Title" + ): + auto_title_session(db, "sess-1", "hi", "hello") # must not raise + + def test_not_found_raises_runtime_error_internally(self): + # set_session_title returning False (session vanished) -> RuntimeError + # in the persist helper, swallowed by auto_title_session, no callback. + from agent.title_generator import _persist_session_title + db = MagicMock() + db.set_session_title.return_value = False + with pytest.raises(RuntimeError): + _persist_session_title(db, "missing", "Some Title") diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index cd720ad21c52..f5317bf2820f 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -4942,3 +4942,43 @@ class TestMultiTargetDeliveryContinuesOnFailure: assert "a@example.com" in result assert "b@example.com" in result assert mock_pool.submit.call_count == 2 + + +class TestSetCronSessionTitle: + """Robust cron session titling: #50535/#50536/#50537.""" + + def test_sets_title_when_no_collision(self): + from cron.scheduler import _set_cron_session_title + db = MagicMock() + db.set_session_title.return_value = True + out = _set_cron_session_title(db, "sess-1", "Nightly Synthesis") + assert out == "Nightly Synthesis" + db.set_session_title.assert_called_once_with("sess-1", "Nightly Synthesis") + + def test_dedupes_on_duplicate_title(self): + # First write collides (ValueError); helper falls back to lineage #N. + from cron.scheduler import _set_cron_session_title + db = MagicMock() + db.set_session_title.side_effect = [ValueError("in use"), True] + db.get_next_title_in_lineage.return_value = "Nightly Synthesis #2" + out = _set_cron_session_title(db, "sess-1", "Nightly Synthesis") + assert out == "Nightly Synthesis #2" + db.get_next_title_in_lineage.assert_called_once_with("Nightly Synthesis") + + def test_reraises_when_no_lineage_support(self): + from cron.scheduler import _set_cron_session_title + db = MagicMock(spec=["set_session_title"]) + db.set_session_title.side_effect = ValueError("in use") + with pytest.raises(ValueError): + _set_cron_session_title(db, "sess-1", "Dup") + + def test_returns_none_for_blank_base(self): + from cron.scheduler import _set_cron_session_title + db = MagicMock() + assert _set_cron_session_title(db, "sess-1", " ") is None + db.set_session_title.assert_not_called() + + def test_returns_none_without_db_or_session(self): + from cron.scheduler import _set_cron_session_title + assert _set_cron_session_title(None, "sess-1", "X") is None + assert _set_cron_session_title(MagicMock(), "", "X") is None