Skip to content

Fix #5532: /api/session/clear sets truncation watermark (P0 data loss) - #5553

Closed
nesquena-hermes wants to merge 4 commits into
masterfrom
fix-5532-clear-watermark
Closed

nesquena-hermes wants to merge 4 commits into
masterfrom
fix-5532-clear-watermark

Conversation

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Closes #5532 (P0 data loss).

POST /api/session/clear cleared the sidecar transcript but never recorded a
truncation watermark
, so state.db messages resurrected on the next
/api/session read: history reappeared after clear + refresh, and a continued
conversation still carried the full pre-clear context into the model.

Root cause

The /api/session/clear handler (api/routes.py) did:

s.messages = []
s.tool_calls = []
apply_session_title_rename(s, "Untitled")
s.save()

It never set s.truncation_watermark. The append-only state.db merge
(merge_session_messages_append_only, reached via
reconciled_state_db_messages_for_session) treats an unset / None watermark
as "keep everything and just dedup". Because state.db is append-only, the
cleared transcript was still there, so the merge re-added all of it on the next
read — the exact data-loss symptom in the issue.

The sibling destructive op /api/session/truncate does not have this bug:
it calls truncate_session_at_keep(session, keep) in api/session_ops.py,
which sets truncation_watermark = truncation_boundary = _truncation_watermark_for(kept_messages).

Fix

A full clear is just a truncate that keeps zero messages. Route /clear
through the same helper the truncate handler uses:

truncate_session_at_keep(s, 0)   # empties messages + context_messages,
                                 # sets watermark = boundary = 0.0
s.tool_calls = []
apply_session_title_rename(s, "Untitled")
s.save()

_truncation_watermark_for([]) == 0.0, which is the #2914 "truncate-to-empty"
sentinel
that blocks all state.db replay. The merge/read path is
untouched — /clear and /truncate now set the marker identically, so the
merge contract is not forked.

Second half of the issue (continued-context) — verified

The issue notes that a continued conversation retained pre-clear context. That
is fixed by the same change: truncate_session_at_keep(s, 0) empties
context_messages in lockstep with messages (via
truncate_context_for_display_keep(..., keep=0) -> []), so the model-facing
context no longer carries the cleared turns. A regression test asserts
context_messages == [] after clear.

Tests

tests/test_issue5532_clear_truncation_watermark.py (drives the real
POST /api/session/clear route via handle_post, mirroring the #2914
integration harness):

  • test_clear_endpoint_sets_truncation_watermark — watermark & boundary set to 0.0
  • test_clear_empties_context_messages — second-half fix: context_messages == []
  • test_clear_then_read_does_not_resurrect_state_db_messages — subsequent
    state.db merge returns EMPTY (no resurrection)
  • test_clear_matches_truncate_to_empty_marker — /clear and
    /truncate(keep=0) produce an identical marker (no forked merge logic)

Fail-without-fix confirmed: all four fail on origin/master
(truncation_watermark stays None, resurrection occurs) and pass with this
change.

$ .venv/bin/python3 -m pytest tests/test_issue5532_clear_truncation_watermark.py -q
....                                                                     [100%]
4 passed in 3.80s

Related watermark suite stays green:

$ .venv/bin/python3 -m pytest tests/test_issue5532_clear_truncation_watermark.py \
    tests/test_issue2914_truncation_watermark.py tests/test_issue3831_watermark_clear.py -q
30 passed

…loss)

The /api/session/clear handler wiped s.messages and s.tool_calls but never
set truncation_watermark. The append-only state.db merge
(merge_session_messages_append_only via reconciled_state_db_messages_for_session)
treats an unset/None watermark as "keep everything and dedup", so the next
/api/session read resurrected every cleared turn from state.db:

  * history reappeared after clear + refresh, and
  * because context_messages also survived the clear, a continued turn still
    carried the full pre-clear context into the model.

The sibling /api/session/truncate handler is not affected: it calls
truncate_session_at_keep(session, keep), which sets
truncation_watermark = truncation_boundary = _truncation_watermark_for(kept).

Fix: route /clear through the SAME helper with keep=0. A full clear is a
truncate that keeps zero messages, so the watermark becomes
_truncation_watermark_for([]) == 0.0 — the #2914 "truncate-to-empty" sentinel
that blocks ALL state.db replay — and context_messages is emptied in lockstep.
The merge contract is not forked; /clear and /truncate now set the marker
identically. The read/merge path is untouched.

Tests: tests/test_issue5532_clear_truncation_watermark.py drives the real
POST /api/session/clear route and asserts the watermark/boundary are set to
0.0, context_messages is emptied, and a subsequent state.db merge returns
EMPTY (no resurrection). All four tests fail on origin/master (watermark stays
None) and pass with this fix.
@greptile-apps

greptile-apps Bot commented Jul 4, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

Routes POST /api/session/clear through the existing truncate_session_at_keep(s, 0) helper so it sets truncation_watermark = truncation_boundary = 0.0, closing the gap that caused cleared messages to resurrect from the append-only state.db on the next read.

  • Core fix (api/routes.py): replace the manual s.messages = [] assignment with truncate_session_at_keep(s, 0), which also empties context_messages and stamps the truncation watermark that the merge path requires to honor the clear.
  • Codex gates (api/routes.py): two follow-on hazards are closed — compression-lineage detachment (nulls parent_session_id + anchor fields so a compressed-continuation parent snapshot can't resurface the pre-clear history on display), and stale-backup removal (s.path.with_suffix(".json.bak").unlink(missing_ok=True) so startup recovery can't undo the clear across a restart).
  • Tests (tests/test_issue5532_clear_truncation_watermark.py): six regression tests drive the real handle_post route end-to-end and cover watermark presence, context_messages erasure, no state.db resurrection, marker parity with /truncate, compression-lineage detachment, and backup-survival protection.

Confidence Score: 5/5

Safe to merge — the change is a targeted rerouting of /clear through an already-proven helper, with no modifications to the merge/read path itself.

The fix is minimal and mechanically sound: truncate_session_at_keep(s, 0) is the same code path already exercised by /truncate, so no new logic is introduced for the critical merge contract. The three follow-on hazards (watermark, compression lineage, stale backup) are each independently tested end-to-end against the real route handler. RFC line-number drift is the only other change and is a documentation-only update with consistent +46 offsets throughout.

No files require special attention.

Important Files Changed

Filename Overview
api/routes.py Replaces the bare s.messages = [] assignment in /api/session/clear with truncate_session_at_keep(s, 0) and adds compression-lineage nulling and stale-backup removal; logic is correct and mirrors the sibling /truncate handler.
tests/test_issue5532_clear_truncation_watermark.py Six end-to-end regression tests exercising the real handle_post route; harness pattern mirrors existing #2914 suite, stubs are minimal and correct (CSRF, eviction), assertions cover all failure modes described in the issue.
docs/rfcs/session-sse-contract-v1.md Mechanically updates route.py line-number references (+46 across all anchors, consistent with the 46-line expansion of the clear handler); no semantic content changed.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant C as Client
    participant R as routes.py /api/session/clear
    participant SO as session_ops.truncate_session_at_keep
    participant S as Session object
    participant DB as state.db (append-only)
    participant BAK as .json.bak
    participant REC as session_recovery

    C->>R: "POST /api/session/clear {session_id}"
    R->>SO: "truncate_session_at_keep(s, keep=0)"
    SO->>S: "s.messages = []"
    SO->>S: "s.context_messages = []"
    SO->>S: "s.truncation_watermark = 0.0 (sentinel)"
    SO->>S: "s.truncation_boundary = 0.0"
    R->>S: "s.tool_calls = []"
    R->>S: "s.parent_session_id = None (detach compression lineage)"
    R->>S: "s.compression_anchor_* = None"
    R->>S: apply_session_title_rename(s, Untitled)
    R->>S: s.save()
    R->>BAK: "unlink(.json.bak, missing_ok=True)"
    Note over BAK: Prevents startup recovery from resurrecting cleared transcript

    C->>R: GET /api/session (next read)
    R->>DB: reconciled_state_db_messages_for_session(s)
    Note over DB: watermark=0.0 blocks ALL replay
    DB-->>R: [] (empty — no resurrection)
    R-->>C: "{messages: []}"

    REC->>BAK: check .json.bak on startup
    Note over REC: .bak is gone → no restore attempt
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant C as Client
    participant R as routes.py /api/session/clear
    participant SO as session_ops.truncate_session_at_keep
    participant S as Session object
    participant DB as state.db (append-only)
    participant BAK as .json.bak
    participant REC as session_recovery

    C->>R: "POST /api/session/clear {session_id}"
    R->>SO: "truncate_session_at_keep(s, keep=0)"
    SO->>S: "s.messages = []"
    SO->>S: "s.context_messages = []"
    SO->>S: "s.truncation_watermark = 0.0 (sentinel)"
    SO->>S: "s.truncation_boundary = 0.0"
    R->>S: "s.tool_calls = []"
    R->>S: "s.parent_session_id = None (detach compression lineage)"
    R->>S: "s.compression_anchor_* = None"
    R->>S: apply_session_title_rename(s, Untitled)
    R->>S: s.save()
    R->>BAK: "unlink(.json.bak, missing_ok=True)"
    Note over BAK: Prevents startup recovery from resurrecting cleared transcript

    C->>R: GET /api/session (next read)
    R->>DB: reconciled_state_db_messages_for_session(s)
    Note over DB: watermark=0.0 blocks ALL replay
    DB-->>R: [] (empty — no resurrection)
    R-->>C: "{messages: []}"

    REC->>BAK: check .json.bak on startup
    Note over REC: .bak is gone → no restore attempt
Loading

Reviews (4): Last reviewed commit: "fix(#5532): resolve CI — ruff F841 + rea..." | Re-trigger Greptile

Codex gate on the initial fix found a real reachable resurrection edge: a
compressed-continuation child persists its archived transcript in a parent
sidecar marked pre_compression_snapshot, and _webui_sidecar_lineage_messages_for_display()
stitches that parent back for display, merging the child with
truncation_watermark=None (api/routes.py:8061-8064). So the 0.0 truncate-to-empty
sentinel we set on the CHILD does NOT stop the PARENT snapshot from resurrecting
the pre-clear transcript on refresh — /clear on a compressed continuation still
leaked history.

Fix: on /clear, detach the compression lineage (parent_session_id +
compression_anchor_visible_idx/message_key/summary) so the cleared child no
longer resolves a pre_compression_snapshot parent to stitch.

Test: test_clear_detaches_compression_snapshot_parent asserts the display-lineage
path returns [] after clearing a child whose parent is pre_compression_snapshot
(precondition proves the stitch surfaces it before clear). Fail-without-fix
verified. All 5 tests green.
…resurrect it (Codex gate r2)

Codex re-gate found a third resurrection path: s.save() on /clear writes a
pre-clear .json.bak (messages shrank to []), and recover_all_sessions_on_startup
restores any session whose .bak has MORE messages than the live file
(session_recovery.py:234, bak_count > live_count) WITHOUT consulting the live
truncation_watermark==0.0 — so after a WebUI restart the cleared transcript was
restored from the backup (with a None watermark). Codex verified end to end:
live messages 0 -> recovery recommend:restore -> live 1, watermark None.

Fix: after the /clear save, unlink the pre-clear .json.bak with the SAME guarded
pattern manual-compress (routes.py:22325) and delete already use. An intentional
full clear must not be undoable by startup recovery.

Test: test_clear_survives_startup_recovery drives the real /clear route then
recover_all_sessions_on_startup() and asserts messages stay [], context stays [],
watermark stays 0.0, and no .bak survives. Fail-without-fix verified. All 6
tests green.
…chors

Two CI failures on the prior head, both real:
1. Lint (ruff forward-gate): F841 unused local 'models' in
   test_clear_endpoint_sets_truncation_watermark — dropped the assignment
   (the other test that DOES use models.SESSION_DIR is unchanged).
2. test_issue4812_session_sse_contract_rfc shard-0: this PR's +46 lines in
   api/routes.py shifted the symbol definitions the SSE-contract RFC pins by
   absolute line number (_handle_session_events_stream 16177->16223, the
   run-journal parse/replay/runner_event_id anchors +46 each). Realigned every
   affected api/routes.py:NNNN anchor in docs/rfcs/session-sse-contract-v1.md to
   current lines (the route + heartbeat anchors below the insertion point are
   unchanged). This is the known #5542 line-anchor brittleness; de-brittling the
   test itself stays scoped to #5542.

Verified: ruff clean on changed files; test_issue4812 + test_issue5532 = 39
passed.
@nesquena-hermes

Copy link
Copy Markdown
Collaborator Author

Cross-ref for merge ordering: #5569 (de-brittle #5542) converts the SSE-contract RFC to cite api/routes.py symbols by name instead of :NNNN line numbers, and removes the line anchors entirely.

This PR (#5553) currently includes a small RFC hunk that realigns those line numbers (+46, to survive this PR's routes.py insertion) so the pre-#5569 line-anchor test stays green. That realignment becomes obsolete once #5569 lands.

No code conflict either way — both only touch the RFC's citation style. The P0 code fix in this PR is independent of #5569.

@nesquena-hermes nesquena-hermes added the size:L Large PR (>10 files or >250 LOC) label Jul 4, 2026
nesquena-hermes added a commit that referenced this pull request Jul 4, 2026
Release — clear-conversation P0 data-loss (#5532, #5556, #5553)
@nesquena-hermes

Copy link
Copy Markdown
Collaborator Author

Shipped in v0.51.860 🎉 as part of the combined #5532 fix (primary PR #5556 by @rodboev).

Your improvements were folded in and credited (Co-authored-by): the shared truncate_session_at_keep(s,0) helper-reuse and — the key Codex-caught deeper path — detaching the pre_compression_snapshot parent lineage so a compressed-continuation child can't resurrect its pre-clear transcript via the parent display-stitch (which merges with watermark=None, bypassing the child's 0.0 sentinel). Guarded to detach only compression-snapshot parents so ordinary fork links survive. Both your test file and #5556's ship. Closing as combined-and-shipped — thank you.

pull Bot pushed a commit to AmirulAndalib/hermes-webui that referenced this pull request Jul 4, 2026
…uilt on de-brittled base)

Rebuilt the combined nesquena#5556+nesquena#5553 fix on v0.51.859 (now has the flake fix + the
nesquena#5542 RFC de-brittle, so no anchor-test collateral). Nathan's call: rodboev's
nesquena#5556 primary + fold self-built nesquena#5553 improvements, credit both.

Clear handler now:
- routes through shared truncate_session_at_keep(s,0) (single source of truth,
  sets watermark=_truncation_watermark_for([])==0.0, the nesquena#2914 sentinel that
  blocks state.db append-merge replay)
- detaches compression lineage ONLY when the parent is a pre_compression_snapshot
  (Codex-caught: preserve genuine fork parent links for nesting + "Forked from")
- rodboev's persisted-clear read-back verification + stale .bak removal

RFC conflict resolved in favor of the de-brittled (symbol-anchor) master version.
Ships both PRs' test files (state_db_replay + clear_truncation_watermark incl the
fork-preservation regression). Opus (on the amended tree): both resurrection paths
closed, detach lineage-safe, verification sound — SHIP. 3 non-blocking follow-ups
filed (nesquena#5570 .bak crash-window, nesquena#5571 fork-stitch corner, nesquena#5572 messaging clear).

Co-authored-by: rodboev <rodboev@users.noreply.github.com>
@nesquena-hermes
nesquena-hermes deleted the fix-5532-clear-watermark branch July 6, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L Large PR (>10 files or >250 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/api/session/clear does not delete messages from state.db — history survives clear+refresh

1 participant