Skip to content

fix(agent): catch persist errors in finalize_turn to avoid losing tur… - #43057

Closed
MorAlekss wants to merge 2 commits into
NousResearch:mainfrom
MorAlekss:fix/turn-finalizer-persist-session-graceful
Closed

fix(agent): catch persist errors in finalize_turn to avoid losing tur…#43057
MorAlekss wants to merge 2 commits into
NousResearch:mainfrom
MorAlekss:fix/turn-finalizer-persist-session-graceful

Conversation

@MorAlekss

Copy link
Copy Markdown
Contributor

Summary

Wraps _drop_trailing_empty_response_scaffolding and _persist_session
in finalize_turn() with a try/except block. When session persistence
fails due to a disk or SQLite error, the turn result is still returned to
the caller instead of propagating an unhandled exception.


Root cause

finalize_turn() in agent/turn_finalizer.py calls
_drop_trailing_empty_response_scaffolding() and _persist_session()
without any error handling. If _persist_session() raises (e.g.
OSError: disk full, sqlite3.OperationalError: database is locked),
the exception propagates up through run_conversation() with no catch
at that level either. The caller receives an exception instead of the
expected result dict, the turn response is never delivered to the
user, and the session is not saved.


Behavioral change

Before: any exception in _persist_session() caused finalize_turn()
to raise, losing the turn result and the user's response entirely.

After: exceptions from both _drop_trailing_empty_response_scaffolding()
and _persist_session() are caught as a pair. A logger.warning is
emitted with the error details, and finalize_turn() continues to
return the result dict. The user receives the response for this turn
even if the session was not persisted to SQLite.


What changed

agent/turn_finalizer.py

  • Wrapped lines 142-143 (_drop_trailing_empty_response_scaffolding and
    _persist_session) in try/except Exception as exc: logger.warning(...)
  • Both calls kept in the same block so that _persist_session is never
    called with unclean messages if _drop_trailing_empty_response_scaffolding
    raises first

tests/agent/test_turn_finalizer.py (new file)

  • Added test_finalize_turn_survives_persist_session_failure: patches
    _persist_session to raise OSError("disk full"), verifies that
    finalize_turn returns a valid result dict and emits the expected
    warning

What is NOT changed

  • finalize_turn() return contract unchanged: always returns result dict
  • All hook try/except blocks (transform_llm_output, post_llm_call) unchanged
  • _persist_session internals unchanged
  • No behavior change when persistence succeeds
  • except Exception scope is consistent with the file's existing
    graceful-degradation pattern used in transform_llm_output and
    post_llm_call hooks

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — clean ✅

Reviewed the full diff including the new test file. The change wraps _drop_trailing_empty_response_scaffolding + _persist_session in a try/except so finalize_turn always returns a result dict even on disk/IO failures.

What I checked:

  • ✅ Both calls are inside the same try block — if scaffolding drop fails, persist is also skipped (correct: no point persisting stale state)
  • ✅ Warning log includes the exception message for debugging
  • ✅ Test covers OSError("disk full") and verifies the result dict is returned
  • ✅ Test verifies _persist_session was called (confirming the exception is from persist, not from a missing call)
  • ✅ The caller (conversation_loop.py) already handles the result dict independently of persist success

Design consideration: This is intentional graceful degradation — the user already received their response; persist failure should not crash the turn. The warning-level log ensures the failure is diagnosable from agent.log.

No issues found. LGTM.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jun 9, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused persistence-failure fix. This is now implemented on current main by the stronger merged cleanup guard from #50009.

  • agent/turn_finalizer.py:191-234 catches failures from the session-cleanup/persistence sequence, including _persist_session, so they do not escape finalize_turn.
  • agent/turn_finalizer.py:464-465 returns cleanup failure details alongside the normal result, preserving final_response.
  • tests/agent/test_turn_finalizer_cleanup_guard.py:138-172 covers persistence failures both alone and alongside the other fallible post-loop cleanup steps.
  • The implementation shipped in 9f67ba1b0182db31c0bcd08718f681a074373c16 (fix(agent): guard finalize_turn cleanup chain so it never drops the response (#50009)).

Automated hermes-sweeper review.

@teknium1 teknium1 closed this Jul 14, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants