From 057307b3f92b484c825cd5cdbbc68e44f23c9656 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 9 Jun 2026 12:39:32 -0700 Subject: [PATCH] fix(session): retire stale truncation watermark on new committed turn (#3831) (#3890) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(session): retire stale truncation watermark on new committed turn (#3831) retry_last / undo_last / the Edit-truncate handler set truncation_watermark to suppress the *replaced* tail from the append-only state.db merge. Session.save() deliberately never auto-clears it (#2914), but nothing retired it when the user then sent a genuinely NEW turn either — so it froze at the old edit boundary. A frozen watermark then dropped post-watermark state.db rows whenever the sidecar was later reconstructed empty (recovery/reconcile), permanently losing the turns sent after the edit (state.db still had them). Retire a POSITIVE watermark to None once the new user turn is COMMITTED to session.messages — at the success-merge (3 sites), eager-checkpoint, error/ recovery materialization, and cold-load repair commit points. Not at chat-start: in deferred mode the new row isn't in messages yet, so a merge in that window would resurrect the replaced tail (the max-sidecar guard hasn't risen past the old boundary). Once committed, max_sidecar_timestamp rises past the replaced tail and the merge suppresses it without the watermark, so retiring is safe. Cleared to None, never 0.0 — 0.0 is the truncate-to-empty sentinel (#2914) that must keep blocking all state replay, so the clear is falsy-gated. Closes #3831 * chore(changelog): clarify watermark-retirement timing to commit-time Greptile review noted the original phrase "retires the watermark at the start of a new user turn" was timing-imprecise. The retirement actually fires when the new turn is durably committed to session.messages — at the agent-result merge, the eager user-message checkpoint, or the cold-load recovery commit. Reword for accuracy; semantics unchanged. Co-Authored-By: Claude Opus 4.7 * test(#3831): add regression tests for the two inline watermark-clear paths (greptile P2) Cover the error/cancel materialization path (_materialize_pending_user_turn_before_error) and the eager first-turn checkpoint path (_checkpoint_user_message_for_eager_session_save), which inline the falsy-gated watermark clear instead of calling the tested helper. The error path is precisely the #3831 failure mode (recovery/reconcile after a crash), so a dedicated regression test closes that gap. Both assert a positive watermark clears to None while the 0.0 truncate-to-empty sentinel (#2914) is preserved. --------- Co-authored-by: nesquena-hermes <[email protected]> Co-authored-by: Nathan Esquenazi Co-authored-by: Claude Opus 4.7 --- CHANGELOG.md | 1 + api/models.py | 6 + api/routes.py | 9 ++ api/streaming.py | 28 ++++ tests/test_issue3831_watermark_clear.py | 172 ++++++++++++++++++++++++ 5 files changed, 216 insertions(+) create mode 100644 tests/test_issue3831_watermark_clear.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a3836823..5d1a6fdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Fixed - **Sidebar no longer stalls for seconds on a state.db that is missing the agent's message index.** The CLI-session scan orders candidate sessions by a correlated `MAX(timestamp)` subquery over `messages`, which collapses to a full per-session table scan when `idx_messages_session ON messages(session_id, timestamp)` is absent — stalling `/api/sessions` for several seconds on every refresh (the 5s cache TTL never settles, so repeated "Slow WebUI request still running" warnings appear). A normally-migrated hermes-agent state.db has this index, but a db that lost its migrations (older hermes-agent, or a hand-rebuilt/reimported db) does not. WebUI now primes the index with `CREATE INDEX IF NOT EXISTS` before the scan — a no-op when it already exists, and a ~20ms self-heal otherwise (measured 13.3s → 0.009s on a no-index 8k-session db). Best-effort: degrades silently on a read-only db, a locked db, or a minimal schema without a `timestamp` column. (#3887) +- **A stale truncation watermark no longer hides messages sent after a retry/undo/edit.** `retry`, `undo`, and the Edit-truncate handler set a `truncation_watermark` to suppress the *replaced* tail from the append-only state.db merge, but nothing cleared it when the user then sent a genuinely new turn — so it froze at the old edit boundary. A frozen watermark then dropped the post-edit turns whenever the sidecar was later reconstructed empty (recovery/reconcile), permanently losing them from the WebUI transcript even though state.db still had the full data. WebUI now retires the watermark once the new turn is durably committed to `session.messages` — at the agent-result merge, the eager user-message checkpoint, or the cold-load recovery commit (the edit boundary is historical once a real turn lands). Cleared to `None`, distinct from the `0.0` truncate-to-empty sentinel, so the #2914 "don't replay a deleted transcript" behavior is unchanged. (#3831) ## [v0.51.341] — 2026-06-09 — Release LE (stale thinking-dot placeholder fix) diff --git a/api/models.py b/api/models.py index 3ee491fb..8063977c 100644 --- a/api/models.py +++ b/api/models.py @@ -389,6 +389,12 @@ def _append_recovered_pending_turn(session, *, timestamp: int | None = None) -> recovered['attachments'] = list(session.pending_attachments) session.messages.append(recovered) _append_recovered_turn_to_context(session, recovered) + # The new user turn is now committed to messages (#3831): retire a positive + # truncation watermark from a prior retry/undo/edit so it can't freeze at the + # old edit boundary and drop these post-edit turns on a later empty-sidecar + # reconcile. None, never 0.0 (the truncate-to-empty sentinel, #2914). + if getattr(session, 'truncation_watermark', None): + session.truncation_watermark = None return recovered diff --git a/api/routes.py b/api/routes.py index f078ad2f..470e939d 100644 --- a/api/routes.py +++ b/api/routes.py @@ -11630,6 +11630,15 @@ def _checkpoint_user_message_for_eager_session_save(s, msg: str, attachments, st if attachments: user_msg["attachments"] = list(attachments) s.messages.append(user_msg) + # The new user turn is now committed to messages (#3831): a positive + # truncation watermark from a prior retry/undo/edit has been superseded and + # must retire, else it freezes at the old edit boundary and later drops these + # post-edit turns on an empty-sidecar reconcile. Safe here (not at chat-start) + # because the row is durably in messages, so the merge's max-sidecar guard now + # suppresses the replaced tail without the watermark. Cleared to None — never + # 0.0, which is the truncate-to-empty sentinel (#2914). + if getattr(s, "truncation_watermark", None): + s.truncation_watermark = None def _is_default_or_empty_session_title(title) -> bool: diff --git a/api/streaming.py b/api/streaming.py index 9c80b664..ff07a72d 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -3817,6 +3817,23 @@ def _stream_writeback_can_supersede_recovery_marker(session, msg_text): return False +def _retire_truncation_watermark_after_commit(session) -> None: + """Clear a positive truncation watermark once a new user turn is committed + to ``session.messages`` (#3831). + + retry/undo/Edit set a positive watermark to suppress the *replaced* tail from + the append-only state.db merge; Session.save() deliberately never auto-clears + it (#2914). But nothing retired it when the user then sent a NEW turn, so it + froze at the old edit boundary and later dropped those post-edit turns on an + empty-sidecar reconcile. Once the new turn is durably in messages the merge's + max-sidecar guard suppresses the replaced tail without the watermark, so it is + safe — and necessary — to retire it here. Cleared to None, never 0.0 (the + truncate-to-empty sentinel that must keep blocking replay, #2914). + """ + if getattr(session, 'truncation_watermark', None): + session.truncation_watermark = None + + def _merge_display_messages_after_agent_result(previous_display, previous_context, result_messages, msg_text): """Keep UI transcript durable while allowing model context to compact. @@ -4412,6 +4429,14 @@ def _materialize_pending_user_turn_before_error(session) -> bool: if pending_attachments: recovered['attachments'] = list(pending_attachments) session.messages.append(recovered) + # The new user turn is now committed to messages (#3831): retire a positive + # truncation watermark left over from a prior retry/undo/edit so it cannot + # freeze at the old edit boundary and later drop these post-edit turns on an + # empty-sidecar reconcile. Cleared to None — never 0.0 (the truncate-to-empty + # sentinel, #2914). Safe here because the row is durably in messages, so the + # merge's max-sidecar guard suppresses the replaced tail without the watermark. + if getattr(session, 'truncation_watermark', None): + session.truncation_watermark = None return True @@ -6444,6 +6469,7 @@ def _run_agent_streaming( _restore_display_reasoning_metadata(_previous_messages, _result_messages), msg_text, ) + _retire_truncation_watermark_after_commit(s) # #3831 # Strip XML tool-call blocks from assistant message content. # DeepSeek and some other providers emit ... # in the raw response text; this must be removed before the content is @@ -6723,6 +6749,7 @@ def _run_agent_streaming( _restore_reasoning_metadata(_previous_messages, _result_messages), msg_text, ) + _retire_truncation_watermark_after_commit(s) # #3831 # Skip the error block — jump directly to the # normal post-result persistence path by # leaving _assistant_added truthy (set below). @@ -7671,6 +7698,7 @@ def _run_agent_streaming( _restore_reasoning_metadata(_previous_messages, _result_messages), msg_text, ) + _retire_truncation_watermark_after_commit(s) # #3831 s.save() logger.info('[webui] self-heal (except path): retry succeeded') return # skip error emission diff --git a/tests/test_issue3831_watermark_clear.py b/tests/test_issue3831_watermark_clear.py new file mode 100644 index 00000000..8e1c2ec5 --- /dev/null +++ b/tests/test_issue3831_watermark_clear.py @@ -0,0 +1,172 @@ +"""Regression tests for #3831 — stale truncation_watermark data loss. + +Root cause: retry_last / undo_last / the Edit-truncate handler set +``truncation_watermark`` to suppress the *replaced* tail from the append-only +state.db merge. ``Session.save()`` deliberately does not auto-clear it (#2914), +but nothing cleared it when the user then sent a genuinely new turn either — so +it froze at the old edit boundary. A frozen watermark then dropped post-watermark +state.db rows whenever the sidecar was later reconstructed empty +(recovery/reconcile), permanently losing the turns sent after the edit. + +Fix: retire a *positive* watermark to ``None`` once the new user turn is +COMMITTED to ``session.messages`` (the success-merge, eager-checkpoint, and +recovery/cold-load commit points) — NOT at chat-start. Clearing at chat-start +was unsafe: in deferred mode (the default) the new user row isn't in messages +yet, so a merge in that window would resurrect the replaced tail (the +max-sidecar guard hasn't risen past the old boundary). Once the row is committed, +``max_sidecar_timestamp`` rises past the replaced tail and the merge suppresses +it without the watermark, so retiring it is both safe and necessary. + +``None`` is distinct from ``0.0``: ``0.0`` is the truncate-to-empty sentinel +(#2914) that must keep blocking all state replay, so the clear is falsy-gated and +never touches ``0.0``. +""" +import api.models as models +import api.streaming as streaming + + +def _rows(*specs): + return [ + {"role": role, "content": content, "timestamp": ts} + for (role, content, ts) in specs + ] + + +class _FakeSession: + def __init__(self, watermark): + self.truncation_watermark = watermark + self.messages = [] + self.context_messages = [] + self.pending_user_message = None + self.pending_attachments = None + self.pending_started_at = None + + +# --- The fix: a committed new turn retires a stale positive watermark --------- + +def test_retire_helper_clears_positive_watermark(): + s = _FakeSession(100.0) # stale, from a prior retry/undo/edit + s.messages = _rows(("user", "new turn", 200)) + streaming._retire_truncation_watermark_after_commit(s) + assert s.truncation_watermark is None + + +def test_retire_helper_leaves_zero_watermark_untouched(): + """0.0 is the truncate-to-empty sentinel (#2914), not a stale boundary — the + falsy guard must leave it alone so #2914 replay-blocking is preserved.""" + s = _FakeSession(0.0) + s.messages = _rows(("user", "new turn", 200)) + streaming._retire_truncation_watermark_after_commit(s) + assert s.truncation_watermark == 0.0 + + +def test_retire_helper_noop_when_unset(): + s = _FakeSession(None) + streaming._retire_truncation_watermark_after_commit(s) + assert s.truncation_watermark is None + + +def test_recovery_commit_clears_watermark(): + """The cold-load / recovery commit path (_append_recovered_pending_turn) + retires a stale positive watermark when it materializes the pending turn.""" + s = _FakeSession(100.0) + s.pending_user_message = "recovered new turn" + s.pending_attachments = None + models._append_recovered_pending_turn(s, timestamp=200) + assert s.truncation_watermark is None + # The pending turn was committed to messages. + assert any(m.get("content") == "recovered new turn" for m in s.messages) + + +# --- The effect: cleared (None) watermark stops dropping post-edit turns ------- + +def test_cleared_watermark_keeps_post_edit_state_rows_with_empty_sidecar(): + """Once the watermark is cleared (None), an empty-sidecar reconcile keeps the + genuinely-new post-edit turns instead of dropping them (the #3831 outcome).""" + state = _rows( + ("user", "q1", 50), + ("assistant", "a1", 100), # old edit boundary + ("user", "q2-new", 200), # sent AFTER the retry/edit + ("assistant", "a2-new", 300), + ) + merged = models.merge_session_messages_append_only( + [], state, truncation_watermark=None + ) + assert [m["content"] for m in merged] == ["q1", "a1", "q2-new", "a2-new"] + + +# --- The guardrails: the fix must NOT regress #2914/#3102 --------------------- + +def test_active_watermark_still_filters_replaced_tail_empty_sidecar(): + """A still-active (positive) watermark with an empty sidecar still suppresses + rows above the boundary — the replaced/edited tail (#2914). The fix only + changes WHEN the watermark is retired, never the merge semantics.""" + state = _rows( + ("user", "q1", 50), + ("assistant", "a1", 100), + ("user", "replaced-q2", 200), # above watermark -> filtered + ("assistant", "replaced-a2", 300), + ) + merged = models.merge_session_messages_append_only( + [], state, truncation_watermark=100.0 + ) + assert [m["content"] for m in merged] == ["q1", "a1"] + + +def test_zero_watermark_still_blocks_all_replay_empty_sidecar(): + """A 0.0 watermark on a truncate-to-empty session must STILL block all state + replay (#2914) — unchanged by this fix.""" + state = _rows( + ("user", "only prompt", 1.0), + ("assistant", "only reply", 2.0), + ) + merged = models.merge_session_messages_append_only( + [], state, truncation_watermark=0.0 + ) + assert merged == [] + + +# --- Inline commit-path coverage (greptile): the two sites that inline the ----- +# --- falsy-gated clear instead of calling the helper must behave identically. -- + +def test_error_path_materialize_clears_positive_watermark(): + """The error/cancel materialization path (_materialize_pending_user_turn_before_error) + inlines the watermark clear. #3831 was triggered on recovery/reconcile after a + crash, so the error path is precisely the failure mode — lock it down: a pending + user turn committed on the error path retires a stale positive watermark.""" + s = _FakeSession(100.0) # stale, from a prior retry/undo/edit + s.pending_user_message = "new turn after edit" + appended = streaming._materialize_pending_user_turn_before_error(s) + assert appended is True + assert s.truncation_watermark is None + assert any(m.get("content") == "new turn after edit" for m in s.messages) + + +def test_error_path_materialize_preserves_zero_sentinel(): + """The error-path inline must use the same falsy guard as the helper: the 0.0 + truncate-to-empty sentinel (#2914) is preserved, not cleared.""" + s = _FakeSession(0.0) + s.pending_user_message = "new turn" + streaming._materialize_pending_user_turn_before_error(s) + assert s.truncation_watermark == 0.0 + + +def test_eager_checkpoint_clears_positive_watermark(): + """The eager first-turn checkpoint path (_checkpoint_user_message_for_eager_session_save + in routes.py) inlines the same clear. A committed user turn retires a stale + positive watermark; the 0.0 sentinel is preserved.""" + import api.routes as routes + + s = _FakeSession(100.0) + routes._checkpoint_user_message_for_eager_session_save( + s, "eager new turn", None, started_at=200.0 + ) + assert s.truncation_watermark is None + assert any(m.get("content") == "eager new turn" for m in s.messages) + + s0 = _FakeSession(0.0) + routes._checkpoint_user_message_for_eager_session_save( + s0, "eager new turn", None, started_at=200.0 + ) + assert s0.truncation_watermark == 0.0 +