Skip to content

fix(tui-gateway): retry and log unflushed-message persist failures before teardown discards them - #79109

Open
sakshamzip2-sys wants to merge 1 commit into
NousResearch:mainfrom
sakshamzip2-sys:fix/tui-teardown-persist-data-loss
Open

fix(tui-gateway): retry and log unflushed-message persist failures before teardown discards them#79109
sakshamzip2-sys wants to merge 1 commit into
NousResearch:mainfrom
sakshamzip2-sys:fix/tui-teardown-persist-data-loss

Conversation

@sakshamzip2-sys

Copy link
Copy Markdown

What does this PR do?

Fixes a silent data-loss path in TUI session teardown. tui_gateway/server.py's
_finalize_session sets its idempotency guard before doing any of its cleanup work:

if not session or session.get("_finalized"):
    return
session["_finalized"] = True

Everything after that — including the step that persists unsaved conversation turns to disk
— runs after the guard is already latched, and the persist call is wrapped in a bare
except Exception: pass:

if agent is not None and hasattr(agent, "_persist_session"):
    snapshot = getattr(agent, "_session_messages", None)
    if snapshot:
        try:
            agent._persist_session(snapshot)
        except Exception:
            pass

_teardown_session (the caller) then unconditionally calls agent.close(), and
AIAgent.close() unconditionally does self._session_messages = [] as a documented step of
its teardown sequence.

Chain a transient persist failure (a momentary SQLite lock — the exact scenario the
surrounding comment says this persist call exists for, referencing issue #13121) with this
ordering: persist fails silently → the _finalized guard means it can never be retried
through _finalize_session again → agent.close() runs anyway and wipes the only remaining
copy of the messages. The conversation just vanishes — no exception surfaces, no log line,
nothing to tell an operator this happened. A user who force-quits, or whose connection drops
at the wrong moment during a transient DB lock, silently loses that turn.

Reproduced this concretely (not just from reading the code) with a mock agent whose
_persist_session raises once, run through the real _teardown_session: the 2-message
conversation ends up gone from both disk (never persisted) and memory (wiped by close()),
with no exception, no log output, and _teardown_session returning normally.

Related Issue

No existing issue found for this specific defect. I searched both open and merged PRs and
issues (gh search prs/gh search issues: "teardown", "finalize_session", "unflushed",
"_session_messages", "_finalized retry", plus a broad "teardown data loss" sweep). The
closest related PR is #62052 ("fix(tui): persist dashboard/TUI conversations on WS
disconnect/restart", merged), which fixed a different bug — the persist call writing
zero rows because snapshot/conversation_history aliased the same list — and is already
present on main (this PR builds on top of that fix). #62052 didn't address what happens
when the persist call actually raises, which is what this PR fixes.

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tui_gateway/server.py:
    • _finalize_session: on a persist exception, log it loudly (logger.error(..., exc_info=True)) instead of swallowing it, and record session["_persist_failed"] = True.
    • _teardown_session: before calling agent.close(), if _persist_failed is set, retry
      the persist once. If the retry succeeds, clear the flag and proceed as normal. If it also
      fails, log loudly (naming how many messages are about to be discarded) and still call
      agent.close() — teardown must not hang forever waiting for a database to recover.
    • Does not change when _finalized is set, and does not touch any of the ~6 other
      unrelated best-effort cleanup steps in _finalize_session (the on_session_end plugin
      hook, memory commit, ending the DB row, interrupting delegations, closing the
      slash-worker subprocess) — those are notifications/cleanup, not the sole record of a
      conversation, and hardening all of them uniformly felt like scope creep for a first,
      reviewable PR. Happy to follow up on those separately if useful.
  • tests/tui_gateway/test_finalize_session_persist.py: five new tests in two new classes —
    persist failure is logged and flagged; the happy path doesn't flag a failure; a successful
    retry clears the flag before close() runs; a failed retry logs loudly and leaves the flag
    set; the common case (no flagged failure) skips the retry entirely.

How to Test

  1. Simulate a transient persist failure: an agent whose _persist_session raises once, with
    unflushed _session_messages.
  2. Run it through the real _teardown_session.
  3. Before this fix: the messages are gone from both disk and memory, with no log output and
    no error surfaced anywhere.
  4. After this fix: the failure is logged, _teardown_session retries the persist once before
    agent.close() runs, and if the retry also fails, a loud log line names exactly how many
    messages are about to be discarded.

Ran locally (macOS, shared dev venv, pytest 9.0.2):

  • pytest tests/tui_gateway/test_finalize_session_persist.py → 14 passed (9 pre-existing + 5
    new).
  • pytest tests/test_tui_gateway_server.py -k "teardown or finalize or close_session or session_close" → 12 passed, 505 deselected (targeted subset by name, not the full 500+ test
    file, to keep load down — a maintainer's CI should run the whole file).
  • pytest tests/tui_gateway/test_session_reclaim_notify.py tests/tui_gateway/test_delegation_session_lifecycle.py → 21 passed (both call
    _teardown_session directly and were unaffected).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — I did not run the full suite (tests/test_tui_gateway_server.py alone is 500+ tests). See "How to Test" above for the targeted runs I did perform.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys changed
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure Python control-flow change, no platform-specific code touched
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, not a tool

Notes for reviewers

  • Scope is deliberately narrow: this fixes the one severe, concretely reproducible data-loss
    path (persist failure + _finalized latch + unconditional close()), not the general
    "every step in _finalize_session is best-effort and swallows exceptions" pattern. Said
    explicitly here rather than hidden as an oversight.
  • The retry-once-then-log-loud approach in _teardown_session is a deliberately small design
    choice, not a full retry-queue/backoff mechanism — trading robustness for a minimal diff
    and preserving the existing single-call idempotency contract both functions document. A
    more thorough mechanism (e.g. background retry after teardown completes) is a reasonable
    follow-up if you'd prefer it.
  • I did not exhaustively grep every call site of _finalize_session/_teardown_session
    across the whole repo for a path that might bypass the ordering this fix assumes — I
    confirmed both are documented in their own docstrings as the single chokepoints for this
    lifecycle, and checked the test files this PR touches.

…fore teardown discards them

_finalize_session sets its _finalized idempotency guard before running
its cleanup steps, including the one that persists any unflushed
conversation turns via agent._persist_session(). That persist call was
wrapped in a bare except Exception: pass, so a transient failure (e.g. a
momentary SQLite lock) was swallowed with no log line and no signal to
the caller. Because _finalized was already set, the failure could never
be retried through _finalize_session again. _teardown_session then
unconditionally calls agent.close(), which unconditionally clears
agent._session_messages - so a transient persist failure followed by the
normal teardown sequence permanently and silently discarded the only
copy of that turn's messages, with nothing in the logs to diagnose it
after the fact.

_finalize_session now logs the persist failure loudly and records
session["_persist_failed"] = True. _teardown_session checks that flag
before calling agent.close() and retries the persist once; if the retry
succeeds the flag clears and teardown proceeds normally, and if it also
fails the discard is logged loudly (naming how many messages are lost)
before still calling agent.close(), so teardown never hangs waiting on a
database to recover. Neither function's _finalized idempotency contract
changes.

This is a minimal, scoped fix for the one severe, concretely
reproducible data-loss path - it does not change the other best-effort
cleanup steps in _finalize_session (on_session_end hook, memory commit,
ending the DB row, interrupting delegations, closing the slash-worker),
which are notifications/cleanup rather than the sole record of a
conversation.

Adds five regression tests to
tests/tui_gateway/test_finalize_session_persist.py covering: a persist
failure is logged and flagged, the happy path does not flag a failure,
a successful retry clears the flag before close() runs, a failed retry
logs loudly and leaves the flag set, and the common case (no flagged
failure) skips the retry entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant