Skip to content

Commit bd7c00b

Browse files
authored
Merge pull request #2791 from max-svistunov/lcore-4219-compaction-buffered-turn-lost
LCORE-4219: keep the buffered turns after a compaction
2 parents 1f8fc0d + 6cbb0c4 commit bd7c00b

5 files changed

Lines changed: 446 additions & 36 deletions

File tree

‎src/utils/conversation_compaction.py‎

Lines changed: 95 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,9 @@
2020
2121
* **Marker items track the boundary.** Each compaction writes the summary into
2222
the conversation as a recognizable *marker* message (a message whose text
23-
starts with ``MARKER_SENTINEL``). The items after the last marker are the
24-
recent verbatim turns; the marker texts are the additive summaries. This is
23+
starts with ``MARKER_SENTINEL``) recording how many leading items it covers.
24+
The items past that count are the recent verbatim turns; the marker texts are
25+
the additive summaries. This is
2526
lightspeed's own bookkeeping — OGX never interprets it (we no longer
2627
pass ``conversation`` to inference once a marker exists).
2728
@@ -42,6 +43,7 @@
4243
"""
4344

4445
import asyncio
46+
import re
4547
from collections.abc import AsyncIterator, Sequence
4648
from contextlib import asynccontextmanager
4749
from dataclasses import dataclass
@@ -92,6 +94,16 @@
9294
the model (:func:`_summary_input_message`).
9395
"""
9496

97+
MARKER_COVERS_PREFIX = "[covers:"
98+
"""Prefix of the item count a marker records, e.g. ``[covers:4]``.
99+
100+
A marker is appended to the end of the conversation, after the turns the
101+
compaction kept verbatim, so its position does not mark the boundary between
102+
summarized history and recent turns. The count does: it is how many leading
103+
conversation items the summary covers (LCORE-4219). Markers written before
104+
that fix carry no count and fall back to their position.
105+
"""
106+
95107

96108
# Per-conversation locks (R11). A request that triggers compaction holds the
97109
# conversation's lock across the summarization LLM call so concurrent requests
@@ -197,22 +209,71 @@ def is_marker_item(item: Any) -> bool:
197209
return extract_message_text(item).startswith(MARKER_SENTINEL)
198210

199211

212+
def _split_marker_text(item: Any) -> tuple[Optional[int], str]:
213+
"""Split a marker item into its covered-item count and its summary text.
214+
215+
The count is absent on markers written before LCORE-4219; the caller then
216+
falls back to the marker's position in the conversation.
217+
"""
218+
body = extract_message_text(item)[len(MARKER_SENTINEL) :].strip()
219+
match = re.match(rf"{re.escape(MARKER_COVERS_PREFIX)}(\d+)\]\s*", body)
220+
if match is None:
221+
return None, body
222+
return int(match.group(1)), body[match.end() :]
223+
224+
200225
def _summary_text_of(item: Any) -> str:
201226
"""Extract the summary text from a marker item (sentinel stripped)."""
202-
return extract_message_text(item)[len(MARKER_SENTINEL) :].strip()
227+
return _split_marker_text(item)[1]
203228

204229

205230
def _items_after_last_marker(items: list[Any]) -> list[Any]:
206-
"""Return the conversation items that follow the last summary marker.
231+
"""Return the conversation items that are not covered by any summary.
232+
233+
These are the recent turns replayed verbatim. When there is no marker the
234+
whole list is returned (no compaction has happened yet).
235+
236+
A marker records how many leading items its summary covers, because it is
237+
appended to the end of the conversation, *after* the turns the compaction
238+
deliberately kept verbatim (the buffer). Taking the items that follow the
239+
marker's position would drop those turns from every later request
240+
(LCORE-4219): they are in no summary and would never reach the model
241+
again. Markers written before that fix carry no count, so they keep the
242+
old position-based boundary.
207243
208-
These are the recent turns kept verbatim. When there is no marker the whole
209-
list is returned (no compaction has happened yet).
244+
Marker items themselves are never part of the result: their text is
245+
replayed as a summary, not as a conversation turn.
210246
"""
211-
last = -1
247+
last_index = -1
248+
covered: Optional[int] = None
212249
for index, item in enumerate(items):
213250
if is_marker_item(item):
214-
last = index
215-
return items[last + 1 :]
251+
last_index = index
252+
covered, _ = _split_marker_text(item)
253+
if last_index < 0:
254+
return items
255+
boundary = covered if covered is not None else last_index + 1
256+
return [item for item in items[boundary:] if not is_marker_item(item)]
257+
258+
259+
def _covered_item_count(items: list[Any], keep_items: list[Any]) -> int:
260+
"""Return how many leading items of *items* a new summary covers.
261+
262+
That is the position of the first kept turn, so the count is a raw index
263+
into the stored items, markers included. It cannot be derived by
264+
subtracting the kept turns from the total: markers are filtered out of the
265+
recent turns, so an older marker may sit among the items being kept, and
266+
subtracting would then place the boundary past a turn that is still
267+
verbatim (which would drop it, the bug LCORE-4219 fixes). An empty buffer
268+
means the summary covers everything stored so far.
269+
"""
270+
if not keep_items:
271+
return len(items)
272+
first_kept = keep_items[0]
273+
for index, item in enumerate(items):
274+
if item is first_kept:
275+
return index
276+
return len(items) - len(keep_items)
216277

217278

218279
def _marker_summaries(items: list[Any]) -> list[str]:
@@ -361,16 +422,26 @@ async def _write_summary_marker(
361422
client: AsyncOgxClient,
362423
conversation_id: str,
363424
summary_text: str,
425+
covered_items: int,
364426
) -> None:
365-
"""Write the summary into the conversation as a recognizable marker message."""
427+
"""Write the summary into the conversation as a recognizable marker message.
428+
429+
``covered_items`` is how many leading conversation items the summary
430+
covers. It is recorded in the marker because the marker is appended to the
431+
end of the conversation, after the turns kept verbatim in the buffer, so
432+
the marker's own position cannot mark the boundary (LCORE-4219).
433+
"""
366434
await client.items.create(
367435
conversation_id,
368436
add_items_request=build_add_items_request(
369437
[
370438
{
371439
"type": "message",
372440
"role": "user",
373-
"content": f"{MARKER_SENTINEL} {summary_text}",
441+
"content": (
442+
f"{MARKER_SENTINEL} {MARKER_COVERS_PREFIX}{covered_items}] "
443+
f"{summary_text}"
444+
),
374445
}
375446
]
376447
),
@@ -516,8 +587,18 @@ async def _persist_new_summary_chunk( # pylint: disable=too-many-arguments,too-
516587
user_id: str,
517588
skip_user_id_check: bool,
518589
) -> None:
519-
"""Persist a fresh summary chunk: write the OGX marker + best-effort cache."""
520-
await _write_summary_marker(client, conversation_id, summary.summary_text)
590+
"""Persist a fresh summary chunk: write the OGX marker + best-effort cache.
591+
592+
``summary.summarized_through_turn`` is the number of leading conversation
593+
items the summary covers; it is recorded in the marker so later requests
594+
keep replaying the buffered turns (LCORE-4219).
595+
"""
596+
await _write_summary_marker(
597+
client,
598+
conversation_id,
599+
summary.summary_text,
600+
summary.summarized_through_turn,
601+
)
521602
_store_cached_summary(cache, user_id, conversation_id, summary, skip_user_id_check)
522603

523604

@@ -659,12 +740,11 @@ async def apply_compaction( # pylint: disable=too-many-arguments,too-many-posit
659740
encoding_name=encoding_name,
660741
)
661742
if old_items:
662-
already = len(items) - len(recent_items)
663743
summary = await summarize_chunk(
664744
client,
665745
model,
666746
old_items,
667-
summarized_through_turn=already + len(old_items),
747+
summarized_through_turn=_covered_item_count(items, keep_items),
668748
encoding_name=encoding_name,
669749
)
670750
await _persist_new_summary_chunk(

‎tests/e2e/features/conversation-compaction.feature‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,17 @@ Feature: Conversation compaction
88
context window, older turns are summarized before the request reaches
99
the model. The compaction fixtures register a 2000-token window with a
1010
10% threshold and keep one recent turn verbatim, so a long third query
11-
is what crosses it: turn one ends up in the summary, turn two stays in
12-
the verbatim buffer, and the third query asks for a fact from each.
13-
A fourth query states one more fact without summarizing anything (only
14-
one turn follows the summary), and the fifth summarizes the long third
15-
turn into a second summary. It asks for the datacenter name, which only
16-
the first summary holds (the third query asks for the other two names,
17-
so its answer never repeats it), so the second summary must not replace
18-
the first.
11+
is what crosses it first: turn one is summarized, turn two stays in the
12+
verbatim buffer, and the third query asks for a fact from each.
13+
14+
From there every query summarizes the turn that has just fallen out of
15+
the buffer, so the fifth query runs against three summaries plus the
16+
verbatim fourth turn: the first holds the datacenter name, the second
17+
the team name (the turn the third query had kept verbatim), the third
18+
the long query itself, and the buffer holds the database name. The
19+
fifth query asks for those three names, which no later answer repeats,
20+
so it passes only if no summary replaced an earlier one and no
21+
buffered turn was dropped on the way (LCORE-4219).
1922

2023
Background:
2124
Given The service is started locally
@@ -24,7 +27,7 @@ Feature: Conversation compaction
2427
And the Lightspeed stack configuration directory is "tests/e2e/configuration"
2528

2629

27-
Scenario: the third query crosses the threshold, a second summary keeps the first, recall and history survive
30+
Scenario: the third query crosses the threshold, later summaries keep the earlier ones, recall and history survive
2831
Given The service uses the lightspeed-stack-compaction.yaml configuration
2932
And the active model has a registered context window
3033
And The service is restarted
@@ -37,7 +40,7 @@ Feature: Conversation compaction
3740
And I store conversation details
3841
When I use "query" to ask question with same conversation_id
3942
"""
40-
{"query": "My application namespace is called blue-lagoon. Remember that name too and reply with OK only.", "model": "{MODEL}", "provider": "{PROVIDER}"}
43+
{"query": "My application namespace is called blue-lagoon and the team that owns it is called lantern-ops. Remember both names too and reply with OK only.", "model": "{MODEL}", "provider": "{PROVIDER}"}
4144
"""
4245
Then The status code of the response is 200
4346
And The response context_status is "full"
@@ -59,20 +62,21 @@ Feature: Conversation compaction
5962
And The response context_status is "summarized"
6063
When I use "query" to ask question with same conversation_id
6164
"""
62-
{"query": "What is the name of my datacenter and what is the name of my database? Reply with the two names only, separated by a comma.", "model": "{MODEL}", "provider": "{PROVIDER}"}
65+
{"query": "What is the name of my datacenter, what is the name of the team that owns my namespace, and what is the name of my database? Reply with the three names only, separated by commas.", "model": "{MODEL}", "provider": "{PROVIDER}"}
6366
"""
6467
Then The status code of the response is 200
6568
And The response context_status is "summarized"
6669
And The response contains following fragments
6770
| Fragments in LLM response |
6871
| north-quarry |
72+
| lantern-ops |
6973
| green-harbor |
7074
When I use REST API conversation endpoint with conversation_id from above using HTTP GET method
7175
Then The status code of the response is 200
7276
And The conversation history includes the following user queries
73-
| User query |
74-
| My OpenShift cluster is named aurora-prod-7 and it runs in the datacenter called north-quarry. Remember both names and reply with OK only. |
75-
| My application namespace is called blue-lagoon. Remember that name too and reply with OK only. |
77+
| User query |
78+
| My OpenShift cluster is named aurora-prod-7 and it runs in the datacenter called north-quarry. Remember both names and reply with OK only. |
79+
| My application namespace is called blue-lagoon and the team that owns it is called lantern-ops. Remember both names too and reply with OK only. |
7680

7781

7882
Scenario: the native stream announces compaction on the query that crosses the threshold

‎tests/integration/endpoints/_compaction_helpers.py‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from models.config import CompactionConfiguration
1515
from models.database.conversations import UserConversation
1616
from tests.integration.conftest import InMemoryConversationStore
17-
from utils.conversation_compaction import MARKER_SENTINEL
17+
from utils.conversation_compaction import MARKER_COVERS_PREFIX, MARKER_SENTINEL
1818

1919
EXISTING_CONV_ID = "22222222-2222-2222-2222-222222222222"
2020
CONV_ID_LLAMA = f"conv_{EXISTING_CONV_ID}"
@@ -29,11 +29,18 @@ def msg(role: str, text: str) -> OpenAIResponseMessage:
2929
return OpenAIResponseMessage(role=cast(Any, role), content=text)
3030

3131

32-
def marker(text: str) -> OpenAIResponseMessage:
33-
"""Build a compaction summary marker message."""
32+
def marker(text: str, covers: int | None = None) -> OpenAIResponseMessage:
33+
"""Build a compaction summary marker message.
34+
35+
``covers`` is the number of leading conversation items the summary covers,
36+
which the runtime records in every marker it writes (LCORE-4219). Omit it
37+
to build a pre-LCORE-4219 marker, the shape existing conversations still
38+
hold; :func:`verify_store_content` ignores the difference.
39+
"""
40+
covers_part = "" if covers is None else f"{MARKER_COVERS_PREFIX}{covers}] "
3441
return OpenAIResponseMessage(
3542
role="user",
36-
content=f"{MARKER_SENTINEL} {text}",
43+
content=f"{MARKER_SENTINEL} {covers_part}{text}",
3744
)
3845

3946

@@ -78,6 +85,21 @@ async def collect_items(
7885
return items
7986

8087

88+
def _without_covers(value: Any) -> Any:
89+
"""Drop a marker's ``[covers:N]`` bookkeeping from message content.
90+
91+
Markers record how many items their summary covers (LCORE-4219). The
92+
number depends on where a conversation stands, so comparisons here are on
93+
the summary text; tests that care about the number assert it directly.
94+
"""
95+
if not isinstance(value, str) or not value.startswith(MARKER_SENTINEL):
96+
return value
97+
body = value[len(MARKER_SENTINEL) :].strip()
98+
if not body.startswith(MARKER_COVERS_PREFIX):
99+
return value
100+
return f"{MARKER_SENTINEL} {body[body.index(']') + 1 :].strip()}"
101+
102+
81103
def verify_store_content(
82104
actual: list[OpenAIResponseMessage], expected: list[OpenAIResponseMessage]
83105
) -> bool:
@@ -86,7 +108,8 @@ def verify_store_content(
86108
return False
87109

88110
return all(
89-
getattr(a, field, None) == getattr(b, field, None)
111+
_without_covers(getattr(a, field, None))
112+
== _without_covers(getattr(b, field, None))
90113
for a, b in zip(actual, expected)
91114
for field in ("content", "role", "type")
92115
)

0 commit comments

Comments
 (0)