From 7317e29d8270a3ab0216b2091a1c8ed730912da0 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 24 Jun 2026 18:08:42 +0000 Subject: [PATCH] fix(#4767): guard empty-sidecar reconstruction against not-advanced watermark (Opus gate) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opus deep-gate found a latent data-loss the Codex harnesses missed: the empty-sidecar branch kept at_or_after (ts>=watermark) UNCONDITIONALLY, which is only valid once the watermark was ADVANCED strictly past the original cutoff (a new turn committed -> boundary < watermark). In the not-advanced states the content above the watermark is the deleted suffix, so it got resurrected — the same data-loss class this fix exists to kill. Reachable via crash/cold-load metadata-vs-sidecar divergence. - Keep at_or_after ONLY when boundary < watermark (genuinely advanced). - boundary == watermark (just truncated) OR boundary is None (legacy: a positive watermark always meant 'frozen at cutoff' pre-#4767) -> conservative ts<=watermark filter; never resurrects. - Replaced the backward-scan heuristic with the provable boundary guard. - Removed the now-dead 'or (not sidecar_messages and watermark>0)' clause in the non-empty-sidecar path (unreachable; empty sidecar returns earlier). - Added 2 regression tests (boundary==watermark, legacy None-boundary), both proven non-vacuous; updated 2 contributor tests that assumed advanced-watermark-without-boundary (an unreachable state) to pass the boundary the real persistence path always writes. Co-authored-by: AlexeyDsov --- api/models.py | 75 +++++++++++----------- tests/test_core_data_loss_cases.py | 61 +++++++++++++++++- tests/test_issue3831_watermark_clear.py | 10 ++- tests/test_watermark_advance_after_edit.py | 13 +++- 4 files changed, 114 insertions(+), 45 deletions(-) diff --git a/api/models.py b/api/models.py index 6b2424a5..eec7c88b 100644 --- a/api/models.py +++ b/api/models.py @@ -5135,45 +5135,48 @@ def merge_session_messages_append_only( # Truncate-to-empty sentinel (#2914) — block all replay. return [] else: - # Positive watermark advanced after edit/retry/undo (#4767). - # Without a sidecar there's no seen_content_keys to check against, - # so we reconstruct the correct transcript from state.db alone. + # Positive watermark after edit/retry/undo (#4767). Without a + # sidecar there's no seen_content_keys to check against, so we + # reconstruct the correct transcript from state.db alone. # - # Use truncation_boundary (the original truncate cutoff) to - # distinguish legitimate prefix from deleted suffix. Without it, - # fall back to the backward-scan heuristic (drops last user+assistant - # pair) which works for the common single-turn case. + # `at_or_after` (ts >= watermark) is legitimate POST-EDIT content + # ONLY when the watermark was ADVANCED strictly past the original + # truncate cutoff — i.e. a new turn was committed after the edit, so + # truncation_boundary (the original cutoff) is strictly below the + # advanced watermark. In that state we keep the legitimate prefix + # (ts <= boundary) plus the post-edit tail (ts >= watermark) and drop + # the deleted (boundary, watermark) suffix. + # + # In every OTHER state the content above the watermark is the deleted + # suffix, NOT post-edit content, so keeping it would resurrect deleted + # turns (the exact data-loss this fix exists to kill): + # * boundary == watermark — just truncated, no new turn committed + # yet (e.g. crash/cold-load with metadata-vs-sidecar divergence); + # * boundary is None — legacy session saved before this field + # existed. In the pre-#4767 model committing a turn CLEARED the + # watermark to None, so a persisted positive watermark always + # meant "frozen at cutoff, not advanced". + # For all of those, fall back to the conservative pre-#4767 filter + # `ts <= watermark`, which never resurrects a deleted suffix. boundary_ts = _message_timestamp_as_float({"timestamp": truncation_boundary}) - at_or_after = [] - pre_watermark = [] - for msg in state_messages: - ts = _message_timestamp_as_float(msg) - if ts is not None and ts >= watermark_timestamp: - at_or_after.append(msg) - else: - pre_watermark.append(msg) - if boundary_ts is not None: - # Use the persisted boundary: keep only messages at or before it. + if boundary_ts is not None and boundary_ts < watermark_timestamp: pre_legitimate = [ - m for m in pre_watermark + m for m in state_messages if (ts := _message_timestamp_as_float(m)) is not None and ts <= boundary_ts ] + at_or_after = [ + m for m in state_messages + if (ts := _message_timestamp_as_float(m)) is not None + and ts >= watermark_timestamp + ] filtered = pre_legitimate + at_or_after else: - # Fallback: backward scan (works when exactly one turn was - # deleted — the common edit/retry/undo case). - i = len(pre_watermark) - 1 - while i >= 0: - role = str(pre_watermark[i].get("role", "")).lower() - if role == "assistant": - i -= 1 - elif role == "user": - i -= 1 # skip this user message too (the replaced prompt) - break - else: - i -= 1 # tool messages etc. — skip - filtered = pre_watermark[:i + 1] + at_or_after + filtered = [ + m for m in state_messages + if (ts := _message_timestamp_as_float(m)) is not None + and ts <= watermark_timestamp + ] # Deduplicate true duplicates (same role, content, exact timestamp) # without collapsing legitimately-repeated identical turns (#3346). @@ -5270,14 +5273,8 @@ def merge_session_messages_append_only( # advanced), allow state rows newer than the sidecar tail to merge. sidecar_advanced_past_watermark = ( watermark_timestamp is not None - and ( - (max_sidecar_timestamp is not None - and max_sidecar_timestamp > watermark_timestamp) - # If the sidecar is empty but watermark > 0, the session has - # advanced (a new user turn was committed). Treat this as - # advanced so post-edit state.db rows are not dropped. - or (not sidecar_messages and watermark_timestamp > 0) - ) + and max_sidecar_timestamp is not None + and max_sidecar_timestamp > watermark_timestamp ) if ( watermark_timestamp is not None diff --git a/tests/test_core_data_loss_cases.py b/tests/test_core_data_loss_cases.py index 7392d0f6..1fc3fc2d 100644 --- a/tests/test_core_data_loss_cases.py +++ b/tests/test_core_data_loss_cases.py @@ -73,7 +73,9 @@ def test_core_a_empty_sidecar_resurrects_deleted_turns(): def test_core_a_single_deleted_turn_still_works(): - """Sanity check: backward-scan handles one deleted turn correctly.""" + """Sanity check: an advanced watermark with the original cutoff persisted + as truncation_boundary keeps the prefix + post-edit tail and drops the one + deleted turn.""" state = [ _msg("user", "first msg", 50.0), _msg("assistant", "first reply", 51.0), @@ -83,8 +85,10 @@ def test_core_a_single_deleted_turn_still_works(): _msg("assistant", "post-edit reply", 201.0), ] + # Original cutoff kept through the first reply (@51); a new turn was then + # committed (@200), advancing the watermark. boundary (51) < watermark (200). merged = models.merge_session_messages_append_only( - [], state, truncation_watermark=200.0 + [], state, truncation_watermark=200.0, truncation_boundary=51.0 ) contents = [m["content"] for m in merged] @@ -96,6 +100,59 @@ def test_core_a_single_deleted_turn_still_works(): ) +def test_core_a_not_advanced_watermark_equals_boundary_does_not_resurrect(): + """Just-truncated state (boundary == watermark, no new turn committed yet): + everything above the watermark is the deleted suffix and must NOT be kept. + + Reachable via crash/cold-load metadata-vs-sidecar divergence — the exact + scenario the empty-sidecar branch serves. Opus gate found that the prior + code swept the suffix into `at_or_after` here, resurrecting deleted turns + (the same data-loss class this fix exists to kill).""" + state = [ + _msg("user", "u1", 50.0), + _msg("assistant", "a1", 51.0), + _msg("user", "deleted-u2", 100.0), + _msg("assistant", "deleted-a2", 101.0), + _msg("user", "deleted-u3", 150.0), + _msg("assistant", "deleted-a3", 151.0), + ] + + merged = models.merge_session_messages_append_only( + [], state, truncation_watermark=51.0, truncation_boundary=51.0 + ) + + contents = [m["content"] for m in merged] + assert contents == ["u1", "a1"], ( + f"watermark==boundary must keep only ts<=watermark, got {contents}" + ) + + +def test_core_a_legacy_none_boundary_frozen_watermark_does_not_resurrect(): + """Legacy session (truncation_boundary is None) with a positive watermark: + in the pre-#4767 model a persisted positive watermark always meant + 'frozen at cutoff' (committing a turn cleared it to None), so the safe + behavior is the conservative ts<=watermark filter — no resurrection, and + the legitimately-kept first turn survives.""" + state = [ + _msg("user", "u1", 50.0), + _msg("assistant", "a1", 51.0), + _msg("user", "deleted-u2", 100.0), + _msg("assistant", "deleted-a2", 101.0), + _msg("user", "deleted-u3", 150.0), + _msg("assistant", "deleted-a3", 151.0), + ] + + merged = models.merge_session_messages_append_only( + [], state, truncation_watermark=51.0 # boundary defaults to None + ) + + contents = [m["content"] for m in merged] + assert contents == ["u1", "a1"], ( + f"legacy None-boundary frozen watermark must keep only ts<=watermark " + f"(no resurrection, keep u1), got {contents}" + ) + + # ─── Same-second assistant reply dropped ────────────────────────────────────── diff --git a/tests/test_issue3831_watermark_clear.py b/tests/test_issue3831_watermark_clear.py index 43eb9af4..ea6eb061 100644 --- a/tests/test_issue3831_watermark_clear.py +++ b/tests/test_issue3831_watermark_clear.py @@ -138,7 +138,13 @@ def test_advanced_watermark_keeps_post_edit_state_rows(): def test_active_watermark_still_filters_replaced_tail_empty_sidecar(): """A positive watermark (advanced to the new user-turn timestamp) with an empty sidecar must suppress the replaced pre-edit tail while keeping - post-edit rows (#4767 / CORE finding #2).""" + post-edit rows (#4767 / CORE finding #2). + + A genuinely-advanced session persists the original cutoff as + ``truncation_boundary`` (@100, the last kept reply before the replaced tail) + alongside the advanced watermark (@200, the new committed turn). The + reconstruction keeps ts <= boundary plus ts >= watermark and drops the + replaced (boundary, watermark) suffix.""" state = _rows( ("user", "q1", 50), ("assistant", "a1", 100), @@ -148,7 +154,7 @@ def test_active_watermark_still_filters_replaced_tail_empty_sidecar(): ("assistant", "a2-new", 300), # post-edit reply -> kept ) merged = models.merge_session_messages_append_only( - [], state, truncation_watermark=200.0 + [], state, truncation_watermark=200.0, truncation_boundary=100.0 ) assert [m["content"] for m in merged] == ["q1", "a1", "edited-q2", "a2-new"] diff --git a/tests/test_watermark_advance_after_edit.py b/tests/test_watermark_advance_after_edit.py index c109a132..bc5641d5 100644 --- a/tests/test_watermark_advance_after_edit.py +++ b/tests/test_watermark_advance_after_edit.py @@ -353,7 +353,16 @@ def test_empty_sidecar_advanced_watermark_no_ghost_no_data_loss(): resurrecting stale pre-edit ghosts (#4767). Reproduction: cold reload / crash recovery before sidecar persists. - state.db has the full lineage including replaced pre-edit rows.""" + state.db has the full lineage including replaced pre-edit rows. + + A genuinely-advanced session persists the original cutoff as + ``truncation_boundary`` (here @51, the last kept reply) while the watermark + is advanced to the new committed turn (@200). The reconstruction keeps the + legitimate prefix (ts <= boundary) plus the post-edit tail (ts >= watermark) + and drops the replaced (boundary, watermark) suffix. Passing the boundary is + required — an advanced watermark with NO boundary is the unreachable + legacy/frozen state (see the not-advanced regression tests in + test_core_data_loss_cases.py).""" state = [ {"role": "user", "content": "first msg", "timestamp": 50}, {"role": "assistant", "content": "first reply", "timestamp": 51}, @@ -363,7 +372,7 @@ def test_empty_sidecar_advanced_watermark_no_ghost_no_data_loss(): {"role": "assistant", "content": "post-edit reply", "timestamp": 201}, ] merged = models.merge_session_messages_append_only( - [], state, truncation_watermark=200.0 + [], state, truncation_watermark=200.0, truncation_boundary=51.0 ) contents = [m["content"] for m in merged] wanted = ["first msg", "first reply", "edited/new turn", "post-edit reply"]