fix(#4767): advance truncation_watermark instead of clearing to None after edit/retry/undo

[AlexeyDsov]

Closes #4767

Problem

After editing a message (or using /retry or /undo), switching to another session and switching back caused the original pre-edit message and its assistant reply to reappear. These ghost messages were also fed into the agent's context window, polluting the conversation.

Root cause

truncation_watermark was cleared to None when a new user turn was committed after edit/retry/undo. Without the watermark, merge_session_messages_append_only() lost the boundary needed to filter out replaced pre-edit rows from the append-only state.db, so they leaked back into the merged transcript on reload.

Fix

Advance truncation_watermark to the newest user message timestamp instead of clearing it. The existing sidecar_advanced_past_watermark guard in the merge logic already allows post-edit state.db rows to merge in, while the advanced watermark keeps filtering replaced pre-edit rows whose timestamps fall below the boundary.

Changes

• api/routes.py: _checkpoint_user_message_for_eager_session_save — advance watermark to new message timestamp
• api/models.py: _append_recovered_pending_turn — advance watermark to recovered timestamp
• api/streaming.py: _retire_truncation_watermark_after_commit → _advance_truncation_watermark_after_commit — find newest user message timestamp and advance watermark; _materialize_pending_user_turn_before_error — advance to recovered timestamp
• tests/test_issue3831_watermark_clear.py: updated to reflect advance semantics
• tests/test_watermark_advance_after_edit.py: new end-to-end tests for edit/retry/undo → new turn → reload scenarios

Related

• Bug: /undo and message edit appear to succeed but have no visual effect #2914 (state.db tail replay after undo/retry/edit)
• Bug: truncation_watermark permanently set by retry/undo causes progressive session message loss via state.db reconciliation #3831 (stale watermark data loss)
• Duplicate messages in chat - both user and AI responses are repeated identically #2616, fix(session): use second-level timestamp granularity in legacy dedup key #2620, Fix for #2914: stop agent from seeing deleted messages after Edit/Undo (from state.db) #3102 (prior related fixes)

AI Assistance

This fix was developed with assistance from Qwen3.6-27B. The AI helped analyze the reconciliation logic, identify root cause, fix code and test scenarios.

[greptile-apps]

Greptile Summary

This PR fixes ghost messages reappearing after edit/retry/undo by advancing truncation_watermark to the new user-message timestamp instead of clearing it to None. It also introduces a companion truncation_boundary field that preserves the original truncation cutoff so that empty-sidecar cold-reload can correctly distinguish a legitimate prefix from a deleted suffix without guessing.

• Watermark semantics change (streaming.py, routes.py, models.py): all four commit-path sites now call _advance_truncation_watermark_after_commit (or inline the equivalent), advancing the watermark rather than nullifying it, while the 0.0 truncate-to-empty sentinel is left untouched by the falsy gate.
• New truncation_boundary field (session_ops.py, routes.py, models.py, webui_session_db.py): set equal to the watermark at truncation time, persisted with the session JSON, propagated through duplicate-session construction and all merge_session_messages_append_only call sites, and used in the new empty-sidecar reconstruction path to filter deleted turns without the backward-scan heuristic.
• Two new test files and extensive updates to the existing watermark test cover the full edit → new-turn → reload round-trip, same-second edit corner cases, multi-turn deletion, and save/load persistence of the new field.

Confidence Score: 4/5

Safe to merge for the targeted ghost-message regression; one already-flagged crash-recovery scenario in the eager-checkpoint path warrants a follow-up.

The core watermark-advance logic is well-reasoned and thoroughly tested across edit, retry, and undo paths. The truncation_boundary plumbing is consistent across persistence, merging, and duplication. The main open concern (noted in prior review threads) is the eager-checkpoint crash window: because the watermark is advanced to the user-message timestamp T at checkpoint time, a crash before the assistant reply is added to the sidecar leaves max_sidecar_timestamp == T and sidecar_advanced_past_watermark evaluating to False, which causes the state.db assistant reply at T+ε to be filtered and permanently lost on recovery — a regression from the prior None-clear behavior.

The sidecar_advanced_past_watermark calculation in api/models.py and the crash-recovery interaction with _checkpoint_user_message_for_eager_session_save in api/routes.py.

Important Files Changed

Filename	Overview
api/models.py	Core logic change: merge_session_messages_append_only gains truncation_boundary parameter and a new empty-sidecar reconstruction path using boundary or backward-scan fallback; sidecar_advanced_past_watermark gets a new dead-code branch; Session model and _SAVED_FIELDS updated with truncation_boundary.
api/streaming.py	Renamed _retire_truncation_watermark_after_commit → _advance_truncation_watermark_after_commit; now walks messages backwards to find the newest user timestamp rather than clearing to None. _materialize_pending_user_turn_before_error similarly advances to recovered_ts.
api/routes.py	Truncate path now sets truncation_boundary = truncation_watermark immediately after computing the watermark; eager-checkpoint path advances watermark to user_msg.get('timestamp') or time.time(); duplicate-session constructor propagates truncation_boundary.
api/session_ops.py	Both retry_last and undo_last now persist truncation_boundary = truncation_watermark immediately after truncation so empty-sidecar recovery can distinguish legitimate prefix from deleted suffix.
api/webui_session_db.py	Adds truncation_boundary to _METADATA_FIELDS so the new field is persisted/restored by the session DB layer.
tests/test_core_data_loss_cases.py	New test file covering: empty-sidecar multi-turn resurrection, same-second assistant-reply guard, save/load round-trip for truncation_boundary, and reconciled_state_db_messages_for_session boundary propagation.
tests/test_watermark_advance_after_edit.py	New end-to-end regression tests for edit/retry/undo → new turn → reload scenarios, verifying watermark advances (not clears) and pre-edit state.db rows remain filtered.
tests/test_issue3831_watermark_clear.py	Existing tests updated to match the advance (not clear) semantics; new tests added for multi-message selection, no-timestamp fallback, and filtering of pre-edit state.db rows with advanced watermark.
tests/test_session_duplicate_fields.py	Two new tests assert that truncation_boundary is copied in duplicate sessions but omitted in branch sessions, consistent with the truncation_watermark policy.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["edit / retry / undo"] --> B["set truncation_watermark = last-kept-ts\nset truncation_boundary = last-kept-ts"]
    B --> C["user sends new turn"]
    C --> D{eager\ncheckpoint?}
    D -- yes --> E["_checkpoint_user_message_for_eager_session_save\nadvance watermark → new msg timestamp"]
    D -- no --> F["_advance_truncation_watermark_after_commit\n(after assistant reply committed)\nadvance watermark → newest user ts"]
    E --> G["session saved to disk"]
    F --> G
    G --> H["session switch / reload"]
    H --> I["merge_session_messages_append_only\n(sidecar + state.db)"]
    I --> J{sidecar\nempty?}
    J -- yes --> K{truncation_boundary\nset?}
    K -- yes --> L["keep msgs ≤ boundary_ts\n+ keep msgs ≥ watermark_ts"]
    K -- no --> M["backward-scan fallback:\ndrop last user+assistant pair"]
    J -- no --> N{sidecar_advanced\npast_watermark?}
    N -- yes --> O["allow state.db rows\nbeyond sidecar tail"]
    N -- no --> P["filter state.db rows\nabove watermark"]
    L --> Q["✅ pre-edit rows filtered\npost-edit rows kept"]
    M --> Q
    O --> Q
    P --> Q

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"}}}%%
flowchart TD
    A["edit / retry / undo"] --> B["set truncation_watermark = last-kept-ts\nset truncation_boundary = last-kept-ts"]
    B --> C["user sends new turn"]
    C --> D{eager\ncheckpoint?}
    D -- yes --> E["_checkpoint_user_message_for_eager_session_save\nadvance watermark → new msg timestamp"]
    D -- no --> F["_advance_truncation_watermark_after_commit\n(after assistant reply committed)\nadvance watermark → newest user ts"]
    E --> G["session saved to disk"]
    F --> G
    G --> H["session switch / reload"]
    H --> I["merge_session_messages_append_only\n(sidecar + state.db)"]
    I --> J{sidecar\nempty?}
    J -- yes --> K{truncation_boundary\nset?}
    K -- yes --> L["keep msgs ≤ boundary_ts\n+ keep msgs ≥ watermark_ts"]
    K -- no --> M["backward-scan fallback:\ndrop last user+assistant pair"]
    J -- no --> N{sidecar_advanced\npast_watermark?}
    N -- yes --> O["allow state.db rows\nbeyond sidecar tail"]
    N -- no --> P["filter state.db rows\nabove watermark"]
    L --> Q["✅ pre-edit rows filtered\npost-edit rows kept"]
    M --> Q
    O --> Q
    P --> Q

Loading

_{Reviews (4): Last reviewed commit: "fix(#4767): review fixes - more edge cas..." | Re-trigger Greptile}

[nesquena-hermes]

Thanks @AlexeyDsov — this is a real bug (ghost pre-edit rows resurrecting + polluting agent context) and "advance the watermark instead of clearing it" is the right direction. I warm-gated it (Codex + full suite). The suite is green (10207), but Codex reproduced two CORE edge cases the suite doesn't cover — both on the core state.db merge path, so they need fixing before this can ship:

1. (CORE) Same-second edit/retry can still resurrect the pre-edit rows — api/models.py:5197

The stale-row filter uses timestamp < truncation_watermark, but the patch advances the watermark to the replacement user message's exact timestamp. With integer-second timestamps, the old pre-edit row's timestamp can tie the watermark and fall through < into the merge → the ghost user+assistant rows reappear on a same-second edit/retry. So the fix works for slow edits but not fast ones.
Fix: make the absent-state-row boundary inclusive (<= truncation_watermark), or advance the watermark to a strictly-greater exclusive boundary. Add a same-timestamp regression test (two turns within the same integer second).

2. (CORE, worse) Empty-sidecar recovery now drops legitimate post-edit rows — api/models.py:5087

The empty-sidecar branch keeps only state.db rows <= watermark. With the watermark now advanced to the new user timestamp, that branch drops the post-edit assistant reply and any later post-edit turns — which reintroduces the exact #3831 data-loss shape this lineage was fixing, just in the other direction. The sidecar_advanced_past_watermark guard you're relying on to let post-edit rows merge cannot fire when the sidecar is empty, so that path is unprotected.
Fix: restore empty-sidecar coverage so committed post-edit turns are recoverable without depending on sidecar_advanced_past_watermark (which is a no-op for an empty sidecar). This likely needs a small design adjustment, not a one-liner — worth confirming the empty-sidecar reload path keeps everything at-or-after the watermark.

These are exactly the two-directional risks this watermark code has to thread: too-low a boundary re-leaks the ghost (#1), too-high drops real data (#2, the #3831 trap). A green suite isn't sufficient here because neither edge (same-second tie, empty-sidecar reload) is currently exercised — please add a regression for each.

Everything else looks sound (the advance-not-clear approach, the commit-path coverage for the non-empty-sidecar case). Marking changes-requested; happy to re-gate the moment both boundaries are fixed + covered. Solid root-cause analysis on this one.

[nesquena-hermes]

Warm-gated: suite green but Codex reproduced 2 CORE edge cases the suite doesn't cover — (1) same-second edit/retry still resurrects the ghost (timestamp tie vs the < boundary), (2) empty-sidecar recovery drops legitimate post-edit rows (reintroduces #3831 data loss). Both on the state.db merge path. Details + exact fixes in the comment.

[nesquena-hermes]

Thanks for the quick turnaround @AlexeyDsov — but the re-push (the "Update api/streaming.py" commit) doesn't reach either CORE finding. Both live in api/models.py (the merge/filter logic), not streaming.py (which only sets the watermark). I checked the new head directly:

1. Same-second tie — still present. api/models.py (~line 5197, the replaced/stale-row filter) still reads:

   timestamp is not None and timestamp < watermark_timestamp

   Since the watermark is advanced to the replacement user message's exact timestamp, an old pre-edit row that shares that integer second ties and falls through < → the ghost still resurrects on a same-second edit/retry. Change this boundary to <= (or advance the watermark to a strictly-greater value), and add a same-second regression.

2. Empty-sidecar data loss — still present. api/models.py (~line 5087, the if not sidecar_messages: branch) still keeps only timestamp <= watermark_timestamp:

   if not sidecar_messages:
       if watermark_timestamp is not None:
           ... and timestamp <= watermark_timestamp

   With the watermark now advanced to the new user timestamp, this drops the post-edit assistant reply + later post-edit turns whenever the sidecar is empty → reintroduces the Bug: truncation_watermark permanently set by retry/undo causes progressive session message loss via state.db reconciliation #3831 data-loss shape. The sidecar_advanced_past_watermark guard you're relying on can't help here because it never fires on an empty sidecar. This branch needs to keep post-edit rows recoverable independent of that guard.

So both fixes belong in api/models.py's merge function (merge_session_messages_append_only / the absent-state-row filter), not in streaming.py. The streaming.py side (advancing the watermark) is already correct. Still changes-requested; ping me when models.py is updated + both edges have regression tests and I'll re-gate.

[nesquena-hermes]

Thanks for the fast turnaround @AlexeyDsov — the 14:19 re-push did converge CORE finding #1 (same-second edit/retry timestamp-tie): I re-gated it (Codex, GPT-5.5, high reasoning, with direct reproduction harnesses) and the same-second non-empty-sidecar filtering verifies clean now. Good fix.

But CORE finding #2 is still live — and the gate reproduced it directly on the current head, so this can't ship yet.

CORE — empty-sidecar recovery still corrupts the transcript — api/models.py:5087

The empty-sidecar branch returns before your new sidecar_advanced_past_watermark logic can run. merge_session_messages_append_only() short-circuits at line 5087:

if not sidecar_messages:
    if watermark_timestamp is not None:
        filtered = [
            msg for msg in state_messages
            if (
                (timestamp := _message_timestamp_as_float(msg)) is not None
                and timestamp <= watermark_timestamp   # <-- line 5093
            )
        ]
    ...
    return deduped   # <-- returns here, never reaches the advanced-watermark guard

Because the watermark is now advanced to the new user-turn timestamp, this branch treats the advanced watermark as an upper ceiling instead of a replaced-tail boundary, so it does both wrong things at once:

• drops the legitimate post-edit assistant reply (and any later post-edit turns), ts > watermark → reintroduces the exact Bug: truncation_watermark permanently set by retry/undo causes progressive session message loss via state.db reconciliation #3831 data-loss shape, and
• keeps the stale pre-edit rows (ts ≤ watermark) → resurrects the ghost.

Direct reproduction on your head (not inferred — run):

from api.models import merge_session_messages_append_only as merge
# empty sidecar (cold reload / crash window before sidecar persists),
# watermark advanced to the new user turn @200, state.db has the full lineage:
state = [
    {"role":"user","content":"first msg","timestamp":50},
    {"role":"assistant","content":"first reply","timestamp":51},
    {"role":"user","content":"original pre-edit","timestamp":100},   # replaced by the edit
    {"role":"assistant","content":"original reply","timestamp":101}, # replaced by the edit
    {"role":"user","content":"edited/new turn","timestamp":200},
    {"role":"assistant","content":"post-edit reply","timestamp":201},# legitimate, MUST survive
]
print([m["content"] for m in merge([], state, truncation_watermark=200.0)])
# ACTUAL (head): ['first msg','first reply','original pre-edit','original reply','edited/new turn']
#   -> 'post-edit reply'@201 DROPPED  +  'original pre-edit'/'original reply' RESURRECTED
# WANTED:        ['first msg','first reply','edited/new turn','post-edit reply']

The sidecar_advanced_past_watermark guard you're relying on to admit post-edit rows cannot fire on an empty sidecar (it's computed from max_sidecar_timestamp, which is None here), so this path is completely unprotected.

Fix direction (both fixes belong in api/models.py)

This one is more than a one-liner — the empty-sidecar branch needs to stop treating an advanced positive watermark as a "keep everything ≤ watermark" ceiling:

1. For a positive advanced watermark + empty sidecar, recover committed post-edit rows at/after the boundary instead of dropping them, while still filtering the replaced pre-edit tail. (The challenge is that with an empty sidecar there's no seen_* signal for which rows were replaced — so consider whether the replaced-tail rows can be identified by the same role/content-key dedup the main path uses, or whether the empty-sidecar path should fall through into the main merge loop rather than short-circuiting.)
2. Preserve the 0.0 truncate-to-empty block-all sentinel (Bug: /undo and message edit appear to succeed but have no visual effect #2914) — that path must keep blocking replay; only a positive advanced watermark gets the new treatment.
3. Add a regression: empty sidecar + advanced watermark + stale pre-edit rows + post-edit assistant/later rows → keeps the post-edit rows without resurrecting the stale ones (i.e. the reproduction above, asserted).

Your focused suite (19 passed) and the existing #2914 suite (12 passed) both stay green — neither exercises the empty-sidecar advanced-watermark case, which is why the green suite isn't sufficient here.

Marking changes-requested. Ping me the moment the empty-sidecar branch is fixed + covered and I'll re-gate immediately — you're one branch away. Really solid root-causing on this lineage.

[nesquena-hermes]

Thanks for the continued iteration @AlexeyDsov — CORE#1 (same-second tie) and my earlier CORE#2 (the basic empty-sidecar post-edit-reply case) are both resolved now; I verified the simple cases pass. But a deep P0 re-gate (Codex GPT-5.5, direct reproduction) found two more reproduced data-loss cases in the empty-sidecar recovery path, and they point at a design limitation rather than a one-liner — so I can't ship this yet. Both were reproduced against the current head (297c88fed7).

CORE-A — empty-sidecar recovery resurrects deleted pre-edit turns when editing an older message with more than one later turn (api/models.py:5127)

The empty-sidecar branch infers the boundary by dropping the last pre-watermark user/assistant pair. But /api/session/truncate (edit/retry/undo) can remove an arbitrary suffix, and the new commit sites advance the only stored boundary (truncation_watermark) to the new user timestamp — overwriting the original truncation point. So on empty-sidecar recovery there's no longer enough information to tell a legitimate prefix from a deleted suffix, and editing an older message (with ≥2 later turns) resurrects the deleted turns.

Root issue: advancing the watermark destroys the very information (the original truncation boundary) that empty-sidecar recovery needs.

Fix direction: persist the original truncation boundary separately when advancing the watermark, and in empty-sidecar recovery keep only state.timestamp <= original_boundary plus state.timestamp >= advanced_watermark — don't infer by dropping one turn.

CORE-B — same-second recovery can silently drop the legitimate post-edit assistant reply (api/models.py:5290)

The equality guard skips any state.db row at timestamp == watermark whose content isn't already in the sidecar. With the sidecar holding only the edited user checkpoint and state.db holding a same-second assistant reply, the merge returns only the user turn — the real reply is dropped.

Fix direction: narrow the equality filter so it can't discard state-only assistant/tool recovery rows, or use the persisted truncate-boundary / turn identity instead of timestamp equality alone.

Why I'm bouncing rather than patching inline

This is the crown-jewel append-only merge on a P0 data-loss path, and CORE-A is a design-level gap (the boundary is overwritten), not a surface bug. Patching it inline at release time is too high-risk — a wrong fix trades one data-loss for another. The clean path is to persist enough boundary/turn identity to distinguish a legitimate prefix from a deleted suffix, then the empty-sidecar recovery becomes well-defined.

The good news: the in-context (non-empty sidecar) path and the simple edit cases are solid now — this is specifically the empty-sidecar recovery reconstruction that needs the original boundary preserved. Codex reproduced both cases directly; happy to share the exact repro scripts if useful. Re-gate as soon as the boundary is persisted + same-second state-only recovery is covered. Keeping changes-requested.

[nesquena-hermes]

Shipped in v0.51.628 (Release WI) — thanks @AlexeyDsov! 🎉 Closes #4767.

Your branch landed the core design — advance truncation_watermark instead of clearing it, and persist the original cutoff as a separate truncation_boundary so empty-sidecar recovery can tell a legitimate prefix from a deleted suffix — plus the same-second CORE-B fix. I built the rest on top of your branch (every commit carries a Co-authored-by: AlexeyDsov trailer) so you keep authorship:

1. CORE-A (route-level) — the default GET /api/session full reload and _merged_session_messages_for_display still passed only the watermark, so a normal full reload fell back to the old heuristic and resurrected deleted suffix turns. Threaded truncation_boundary through every merge call site + added an end-to-end route regression.
2. Empty-sidecar guard (Opus) — at_or_after was kept unconditionally, which is only valid once the watermark is advanced past the boundary; in the just-truncated (boundary == watermark) or legacy (boundary is None) state it resurrected the suffix. Replaced the backward-scan with a provable boundary guard.
3. Checkpoint gate (Codex) — the non-empty-sidecar path then needed care so it (a) keeps a state-only post-edit reply when the sidecar tail equals the watermark, and (b) doesn't resurrect a stale ts > watermark row appearing before the edited checkpoint. Gated the advanced-bypass on checkpoint consumption.

Gate (all three, clean): Codex SAFE TO SHIP (8 data-loss reproductions all converged), Opus must-fixes applied, full suite 10392 passed. Deployed to prod, verified /api/sessions healthy on v0.51.628.

Really solid root-cause direction on a gnarly append-only-merge bug — appreciate the iteration. The new regression tests pin every edge so this class can't silently come back.