Skip to content

Commit 7bfbb96

Browse files
committed
fix(sessions): void ownership on any short request history read
The compaction decorator inherits SessionABC.session_settings = None rather than proxying the session it wraps, so a limit configured on the wrapped backend, as in SQLiteSession(session_settings=SessionSettings(limit=2)), resolved as None above the wrapper while the backend still applied it to get_items(None) and returned only the newest turns. The guard that voids the run's ownership token asked whether a limit resolved at the wrapper, so it never fired: the token had been captured over the full store, its count still matched at persist time, and the response recorded a boundary spanning history its request never carried. The previous_response_id compaction keyed on that response then replaced the covered prefix and deleted turns the server side history never contained, with no limit visible anywhere above the backend and no concurrency involved. Stop asking where a limit was configured. The check now sits after both read paths and voids the token whenever the history read for the request differs in length from the count the token captured, wherever the difference came from. That covers a limit on the wrapped session, any other backend that windows its own reads, and any future setting of that shape, and it removes the resolved limit condition rather than adding a second one beside it. The comparison is against the raw returned list, before the normalization and dedupe steps that legitimately merge items with no coverage lost, and against the underlying count the token captured, so an ordinary session's unlimited read matches item for item and still records. A limit generous enough to admit the whole store matches too, and the control regressions for both pin that. The wrapped limit regression fails before the fix with the whole store replaced by the compaction output and every prior turn gone.
1 parent 225ee41 commit 7bfbb96

3 files changed

Lines changed: 99 additions & 27 deletions

File tree

‎src/agents/memory/openai_responses_compaction_session.py‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,16 @@ class _SessionOwnershipToken:
5252
5353
A token can also be voided outright through ``invalidate``. The runner
5454
calls it when the run's request input provably stops covering the whole
55-
stored history: a windowed read left the oldest items out, a session
56-
input callback rebuilt the input, a call_model_input_filter rewrote the
57-
request, a handoff input filter rewrote the accumulated history mid
58-
run, or handoff history nesting collapsed it into a rendered transcript. Counts cannot describe what the server saw in any of these cases,
59-
so a voided token never records. Every persist that carries a token, in
60-
whatever state, marks the session as boundary managed, which makes every
61-
compaction keyed on the run's responses skip before the billed call
62-
whenever nothing was recorded for them.
55+
stored history: the request history read returned a different number of
56+
items than the token captured, a session input callback rebuilt the
57+
input, a call_model_input_filter rewrote the request, a handoff input
58+
filter rewrote the accumulated history mid run, or handoff history
59+
nesting collapsed it into a rendered transcript. Counts cannot describe
60+
what the server saw in any of these cases, so a voided token never
61+
records. Every persist that carries a token, in whatever state, marks the
62+
session as boundary managed, which makes every compaction keyed on the
63+
run's responses skip before the billed call whenever nothing was recorded
64+
for them.
6365
"""
6466

6567
__slots__ = ("count", "generation", "invalidated")

‎src/agents/run_internal/session_persistence.py‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -405,12 +405,12 @@ async def prepare_input_with_session(
405405
406406
``ownership_token`` is the token the runner captured for this read. The
407407
preparation below is where the run's request input can silently stop
408-
covering the stored history: a resolved ``limit`` windows the history read,
409-
and a ``session_input_callback`` rebuilds the input outright. Both cases
410-
void the token here, at the site where the coverage is lost, so the run's
411-
persists record no compaction boundary and its previous_response_id
412-
compactions skip instead of letting a replacement delete prefix items the
413-
server never saw.
408+
covering the stored history: the history read can return a different
409+
number of items than the token counted, and a ``session_input_callback``
410+
rebuilds the input outright. Both cases void the token here, at the site
411+
where the coverage is lost, so the run's persists record no compaction
412+
boundary and its previous_response_id compactions skip instead of letting
413+
a replacement delete prefix items the server never saw.
414414
"""
415415

416416
if session is None:
@@ -426,19 +426,19 @@ async def prepare_input_with_session(
426426
limit=resolved_settings.limit,
427427
wrapper=wrapper,
428428
)
429-
if ownership_token is not None and len(history) != ownership_token.count:
430-
# The windowed read returned something other than the exact items
431-
# the token was captured over, so the request input either leaves
432-
# out the oldest stored items or was built over an interleaved
433-
# write; either way no recorded count could describe what the
434-
# server saw. Void the token so this run records no boundaries.
435-
# When the counts match, the window held the entire store at
436-
# capture time, and the token's generation and count checks at
437-
# persist still prove nothing changed in between, so recording
438-
# stays exactly as sound as an unwindowed read.
439-
ownership_token.invalidate()
440429
else:
441430
history = await _session_get_items(session, wrapper=wrapper)
431+
if ownership_token is not None and len(history) != ownership_token.count:
432+
# The read that feeds the request returned a different number of items
433+
# than the token counted in the store, so the request either leaves
434+
# stored items out or was built over an interleaved write, and no
435+
# recorded count could describe what the server saw. Asking instead
436+
# whether a limit resolved here would miss one configured on a wrapped
437+
# session, whose settings this decorator never exposes, and any other
438+
# backend that windows its own reads. The raw read is compared, before
439+
# normalization and dedupe legitimately merge items; a window that
440+
# admits the whole store still matches it and still records.
441+
ownership_token.invalidate()
442442
is_openai_conversation_session = isinstance(session, OpenAIConversationsSession)
443443
converted_history = [
444444
strip_internal_input_item_metadata(ensure_input_item_format(item)) for item in history

‎tests/memory/test_openai_responses_compaction_session.py‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3388,8 +3388,9 @@ def model_dump(self) -> Any:
33883388
class TestPartialRequestInputCompaction:
33893389
"""Runs whose request input skips stored history must not record boundaries.
33903390
3391-
A resolved session limit windows the history read, a session input
3392-
callback rebuilds the prepared input outright, a call_model_input_filter
3391+
A session limit windows the history read, whether it resolves at the
3392+
wrapper or only inside the wrapped backend, a session input callback
3393+
rebuilds the prepared input outright, a call_model_input_filter
33933394
rewrites any request before it goes out, a handoff input filter rewrites
33943395
the accumulated history mid run, and nested handoff history folds it
33953396
into a rendered transcript. In every case the request carries less than
@@ -3541,6 +3542,75 @@ async def test_session_level_limit_records_nothing_and_compaction_skips(
35413542
mock_client.responses.compact.assert_not_called()
35423543
assert "Skipped compaction for resp_sess" in caplog.text
35433544

3545+
@pytest.mark.asyncio
3546+
async def test_wrapped_session_limit_records_nothing_and_compaction_skips(
3547+
self, caplog: pytest.LogCaptureFixture
3548+
) -> None:
3549+
"""A limit carried by the wrapped backend must fire the guard too.
3550+
3551+
Nothing above the backend knows about this window. The decorator
3552+
inherits ``session_settings = None`` rather than proxying the wrapped
3553+
session's setting, and the run sets no limit either, so the resolved
3554+
settings look unlimited while the backend still applies its own limit
3555+
to the unlimited read and returns only the newest turns. The request
3556+
input therefore omits the oldest stored turns with no limit visible
3557+
anywhere and no concurrency involved, and a boundary recorded over
3558+
the full store would let the previous_response_id replacement delete
3559+
the omitted prefix. The token is voided because the read came back
3560+
shorter than the count it captured, so nothing records, the gate
3561+
arms, the compaction skips before the billed call, and every stored
3562+
item survives.
3563+
"""
3564+
3565+
class WrappedLimitSession(SimpleListSession):
3566+
"""Mirror a backend whose own settings window an unlimited read."""
3567+
3568+
def __init__(self, history: list[TResponseInputItem]) -> None:
3569+
super().__init__(history=history)
3570+
self.session_settings = SessionSettings(limit=2)
3571+
3572+
async def get_items(self, limit: int | None = None) -> list[TResponseInputItem]:
3573+
if limit is None and self.session_settings is not None:
3574+
limit = self.session_settings.limit
3575+
return await super().get_items(limit)
3576+
3577+
prior_turns = self._prior_turns()
3578+
underlying = WrappedLimitSession(history=list(prior_turns))
3579+
session, mock_client = self._make_session(underlying, "wrapped-limit")
3580+
# The window is invisible from the wrapper, which is what made the
3581+
# earlier check on the resolved limit miss this case.
3582+
assert session.session_settings is None
3583+
3584+
model = ScriptedModel(
3585+
steps=[ModelStep(output=[get_text_message("wrapped reply")], response_id="resp_wrap")]
3586+
)
3587+
worker = Agent(name="wrapped_limit_worker", model=model)
3588+
3589+
with caplog.at_level(logging.WARNING, logger="openai-agents.openai.compaction"):
3590+
await Runner.run(worker, "wrapped question", session=session)
3591+
3592+
# The backend really windowed the request: the oldest turns were left
3593+
# out of it, so the server side history cannot contain them.
3594+
assert model.last_call is not None
3595+
request_text = str(model.last_call.input)
3596+
assert "prefix question" not in request_text
3597+
assert "recent question" in request_text
3598+
assert "wrapped question" in request_text
3599+
3600+
# The full history survived, including the prefix the server never
3601+
# saw; deleting it is the data loss under test. Read past the
3602+
# backend's own window so the whole store is inspected.
3603+
stored = await underlying.get_items(limit=100)
3604+
assert stored[: len(prior_turns)] == prior_turns
3605+
stored_text = str(stored)
3606+
assert "wrapped question" in stored_text
3607+
assert "wrapped reply" in stored_text
3608+
3609+
assert session._response_boundaries == {}
3610+
assert session._response_boundaries_ever_recorded is True
3611+
mock_client.responses.compact.assert_not_called()
3612+
assert "Skipped compaction for resp_wrap" in caplog.text
3613+
35443614
@pytest.mark.asyncio
35453615
async def test_filtering_input_callback_records_nothing_and_compaction_skips(
35463616
self, caplog: pytest.LogCaptureFixture

0 commit comments

Comments
 (0)