Files
hermes-webui/tests/test_issue3831_watermark_clear.py
nesquena-hermes 057307b3f9 fix(session): retire stale truncation watermark on new committed turn (#3831) (#3890)
* 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 <noreply@anthropic.com>

* 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 <nesquena@gmail.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-06-09 12:39:32 -07:00

173 lines
7.2 KiB
Python

"""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