Skip to content

fix(cli): don't advance the flush cursor past on-disk rows /undo failed to soft-delete - #93775

Closed
chandlerm923 wants to merge 1 commit into
NousResearch:mainfrom
chandlerm923:fix/undo-persistence-atomicity
Closed

chandlerm923 wants to merge 1 commit into
NousResearch:mainfrom
chandlerm923:fix/undo-persistence-atomicity

Conversation

@chandlerm923

Copy link
Copy Markdown

Summary

undo_last() always truncates conversation_history in memory (intentional, 3f7d1c801d). When the DB soft-delete raises, the flush cursor was reset to the truncated length anyway, assuming the DB agreed. It doesn't — those rows stay active=1. The next flush appends new rows next to them, so undone content resurfaces on reload/resume and stays a hit in FTS search.

Changes

  • cli.py: gate the flush-cursor reset on whether the soft-delete actually succeeded (db_reconciled). Swallowed exception → WARNING + one-line user notice.
  • tests/cli/test_undo_persistence_atomicity.py: 7 tests covering success/failure/no-DB paths.

Validation

Check Result
new tests 7/7 pass
Sabotage run 4/7 fail as expected, restore → 7/7 green
test_slash_undo_title_robustness.py 3/3, no regression

…ed to soft-delete

undo_last() always truncates conversation_history in memory (unchanged,
intentional design — see 3f7d1c8's "keep the in-memory undo but skip
the soft-delete"). But when the DB soft-delete raises (cross-session
handle, non-user target skew, DB locked, etc.), the flush cursor
(agent._last_flushed_db_idx) was still unconditionally reset to the
truncated length, silently assuming the DB agreed. It doesn't: those rows
stay active=1 on disk. The next flush then appends new rows alongside
them instead of replacing them, so the "undone" content resurfaces on
reload/resume/crash and stays a hit in FTS search — while the failure
itself was only ever logged at DEBUG.

Track whether the on-disk transcript is actually known to agree
(db_reconciled) and gate the cursor reset on it. No DB in play at all is
still harmless (nothing to diverge from). Upgrade the swallowed exception
to a WARNING and surface a one-line notice to the user instead of silence.

Sabotage-verified: 4/7 new tests fail without this fix (the 3 success/
no-DB controls still pass unaffected).
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/sessions Session lifecycle, resume, persistence, history P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 24, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Overall: a carefully-reasoned fix for a subtle persistence bug — gating the flush-cursor advance on a db_reconciled flag so a failed soft-delete can no longer leave _last_flushed_db_idx ahead of still-active rows (which is exactly how "undone" content reappeared in FTS and on resume). The user-visible warning upgrades visibility from DEBUG to WARNING+stdout. The test matrix covers success, ValueError, generic exception, empty recents, and no-DB. Observations:

  1. cli.py:10278 — db_reconciled = self._session_db is None or not self.session_id treats "no session id" as reconciled even when _session_db exists. If the session id is absent mid-teardown but the DB has the rows, the cursor still advances. Whether that's reachable depends on call sites; consider narrowing the inital value to self._session_db is None (mirroring the actual condition under which the DB cannot diverge), and letting the not session_id case reconcile via the explicit skip branch.

  2. cli.py:10300 — On ValueError the log level went from debug to WARNING and the message now includes the session id. Good. The printed "(!) Could not confirm this on disk" line is helpful, but the WARNING log and the printed line live in different branches of the try/except: a bare Exception (DB locked) logs WARNING and prints the user warning, while ValueError does the same. Fine. Just note that empty recents logs nothing at WARNING and only prints the user line — silent at log level for a state that is still "undone on disk." Consider a logger.info there so all three unreconciled modes leave some log trace.

  3. cli.py:10334 — The cursor reset is guarded by db_reconciled and hasattr(...). This means after a failed soft-delete, _last_flushed_db_idx retains its stale-but-higher value, so the next flush appends rows starting after the "undone" ones. That's the intended fix behavior, but the old "undone" rows plus new turn rows will then coexist as both active in the DB (until a later successful rewind). The user warning covers this, yet it would be worth a docstring sentence noting that the fix intentionally tolerates mixed active rows after failure, rather than attempting a repair.

  4. tests/cli/test_undo_persistence_atomicity.py:67 — _EmptyRecentsDB.rewind_to_message raises AssertionError if called, which is good. Consider asserting in test_undo_does_not_advance_flush_cursor_when_no_recent_messages_on_disk that no prefill was applied either (since db_text was None), pinning that the empty-recents path doesn't silently mutate the composer state either.

Minor: the new test module imports _make_cli from tests.cli.test_cli_init; if that helper's fixture expectations drift this test will fail for unrelated reasons. A comment noting which attributes of the CLI object the tests depend on would help future maintenance.

@chandlerm923

Copy link
Copy Markdown
Author

Resolved on current main by #93784 (carrying #81234). The upstream implementation now completes the durable rewind before publishing the shorter in-memory history and returns without mutating history when persistence fails, so the flush cursor can no longer advance past unreconciled on-disk rows. This supersedes the narrower guard in this PR, and the branch now conflicts with that implementation. Closing as superseded — thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants