diff --git a/agent/title_generator.py b/agent/title_generator.py index f865aa02d0221..9dcdf1e8963c8 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,11 +284,13 @@ def _auto_title_session( return try: - session_db.set_session_title(session_id, title) - logger.debug("Auto-generated session title: %s", title) + persisted = _persist_session_title(session_db, session_id, title) + if persisted is None: + return + 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 63c45d898e4da..8c52377e21347 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -48,6 +48,45 @@ 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 _heartbeat_run_claim_if_due(): 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/hermes_state.py b/hermes_state.py index 1e39458229448..4c90d5b0ee1d9 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -3222,16 +3222,24 @@ def _is_compression_ancestor( ).fetchone() return row is not None - def set_session_title(self, session_id: str, title: str) -> bool: - """Set or update a session's title. - - Returns True if session was found and title was set. - Raises ValueError if title is already in use by another session, - or if the title fails validation (too long, invalid characters). - Empty/whitespace-only strings are normalized to None (clearing the title). - """ + def _set_session_title( + self, + session_id: str, + title: str, + *, + only_if_empty: bool, + ) -> bool: title = self.sanitize_title(title) + def _do(conn): + if only_if_empty: + current = conn.execute( + "SELECT title FROM sessions WHERE id = ?", + (session_id,), + ).fetchone() + if current is None or current["title"] is not None: + return 0 + if title: # Check uniqueness (allow the same session to keep its own title) cursor = conn.execute( @@ -3263,14 +3271,35 @@ def _do(conn): raise ValueError( f"Title '{title}' is already in use by session {conflict_id}" ) + predicate = " AND title IS NULL" if only_if_empty else "" cursor = conn.execute( - "UPDATE sessions SET title = ? WHERE id = ?", + f"UPDATE sessions SET title = ? WHERE id = ?{predicate}", (title, session_id), ) return cursor.rowcount + rowcount = self._execute_write(_do) return rowcount > 0 + def set_session_title(self, session_id: str, title: str) -> bool: + """Set or update a session's title. + + Returns True if session was found and title was set. + Raises ValueError if title is already in use by another session, + or if the title fails validation (too long, invalid characters). + Empty/whitespace-only strings are normalized to None (clearing the title). + """ + return self._set_session_title(session_id, title, only_if_empty=False) + + def set_auto_title_if_empty(self, session_id: str, title: str) -> bool: + """Set an auto-generated title only when the current title is NULL. + + The predicate and write run in one transaction so a concurrent manual + rename cannot be overwritten. Validation and uniqueness behavior match + :meth:`set_session_title`. + """ + return self._set_session_title(session_id, title, only_if_empty=True) + def get_session_title(self, session_id: str) -> Optional[str]: """Get the title for a session, or None.""" with self._lock: diff --git a/scripts/release.py b/scripts/release.py index 4b96072c87d3e..809c0e2bc9297 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -365,6 +365,7 @@ "157689911+itsflownium@users.noreply.github.com": "itsflownium", "dirtyren@users.noreply.github.com": "dirtyren", "s96919@gmail.com": "s96919", + "rasitakyol@hotmail.com": "rasitakyol", "yakimenkoleksander228@gmail.com": "doxe0x", "a54983334@163.com": "Code-suphub", "78542984+Code-suphub@users.noreply.github.com": "Code-suphub", diff --git a/tests/agent/test_title_generator.py b/tests/agent/test_title_generator.py index 50aa01951abe0..bb0c9627a16a2 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 @@ -9,6 +10,7 @@ maybe_auto_title, _title_language, ) +from hermes_state import SessionDB class TestGenerateTitle: @@ -240,14 +242,42 @@ def test_skips_if_title_exists(self): def test_generates_and_sets_title(self): db = MagicMock() db.get_session_title.return_value = None + db.set_auto_title_if_empty.return_value = True with patch("agent.title_generator.generate_title", return_value="New Title"): auto_title_session(db, "sess-1", "hi", "hello") - db.set_session_title.assert_called_once_with("sess-1", "New Title") + db.set_auto_title_if_empty.assert_called_once_with("sess-1", "New Title") + + def test_does_not_overwrite_title_set_immediately_before_conditional_write( + self, tmp_path + ): + db = SessionDB(tmp_path / "state.db") + db.create_session(session_id="sess-1", source="cli") + seen = [] + + def generate_after_manual_title(*_args, **_kwargs): + db.set_session_title("sess-1", "Manual Title") + return "Auto Title" + + with patch( + "agent.title_generator.generate_title", + side_effect=generate_after_manual_title, + ): + auto_title_session( + db, + "sess-1", + "hi", + "hello", + title_callback=seen.append, + ) + + assert db.get_session_title("sess-1") == "Manual Title" + assert seen == [] def test_invokes_title_callback_after_setting_title(self): db = MagicMock() db.get_session_title.return_value = None + db.set_auto_title_if_empty.return_value = True seen = [] with patch("agent.title_generator.generate_title", return_value="Readable Session"): auto_title_session( @@ -257,7 +287,7 @@ def test_invokes_title_callback_after_setting_title(self): "hi there", title_callback=seen.append, ) - db.set_session_title.assert_called_once_with("sess-1", "Readable Session") + db.set_auto_title_if_empty.assert_called_once_with("sess-1", "Readable Session") assert seen == ["Readable Session"] def test_skips_if_generation_fails(self): @@ -266,7 +296,7 @@ def test_skips_if_generation_fails(self): with patch("agent.title_generator.generate_title", return_value=None): auto_title_session(db, "sess-1", "hi", "hello") - db.set_session_title.assert_not_called() + db.set_auto_title_if_empty.assert_not_called() def test_never_raises_when_body_throws(self): """Daemon-thread target must swallow ALL exceptions (e.g. the @@ -408,3 +438,78 @@ def test_skips_if_no_response(self): 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 + # Atomic write path: collision raises ValueError, retry persists. + db.set_auto_title_if_empty.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_auto_title_if_empty.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_dedupes_duplicate_title_via_lineage_legacy_store(self): + # Store without set_auto_title_if_empty: same dedup via the plain + # set_session_title fallback. + db = MagicMock( + spec=["get_session_title", "set_session_title", "get_next_title_in_lineage"] + ) + 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) + assert db.set_session_title.call_args_list[-1][0] == ( + "sess-1", + "Debugging Import Error #2", + ) + 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_manual_title_race_skips_without_callback(self): + # Atomic predicate fails (manual /title landed while generation was in + # flight) -> nothing persisted, no callback fired. + from agent.title_generator import _persist_session_title + db = MagicMock() + db.set_auto_title_if_empty.return_value = False + assert _persist_session_title(db, "sess-1", "Some Title") is None + db.set_session_title.assert_not_called() + + def test_not_found_raises_runtime_error_internally(self): + # Legacy store (no atomic write): 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(spec=["get_session_title", "set_session_title"]) + 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 cd720ad21c52f..f5317bf2820f6 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -4942,3 +4942,43 @@ def test_all_targets_fail_returns_combined_errors(self): 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 diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 658f31478be99..c12b17a7f805d 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -2990,6 +2990,12 @@ def test_update_title(self, db): session = db.get_session("s1") assert session["title"] == "Updated Title" + def test_auto_title_only_sets_an_empty_title(self, db): + db.create_session(session_id="s1", source="cli") + assert db.set_auto_title_if_empty("s1", "Generated Title") is True + assert db.set_auto_title_if_empty("s1", "Replacement Title") is False + assert db.get_session_title("s1") == "Generated Title" + def test_title_in_search_sessions(self, db): db.create_session(session_id="s1", source="cli") db.set_session_title("s1", "Debugging Auth")