-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(memory): compact the full stored history of a limited session #4628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,8 +156,20 @@ def _resolve_compaction_mode_for_response( | |
| response_id: str | None, | ||
| store: bool | None, | ||
| requested_mode: OpenAIResponsesCompactionMode | None, | ||
| session_items: list[TResponseInputItem], | ||
| ) -> _ResolvedCompactionMode: | ||
| mode = requested_mode or self.compaction_mode | ||
| if mode != "input": | ||
| settings = self.underlying_session.session_settings | ||
| limit = settings.limit if settings is not None else None | ||
| if limit is not None and len(session_items) > max(limit, 0): | ||
| if mode == "previous_response_id": | ||
| raise ValueError( | ||
| "OpenAIResponsesCompactionSession cannot use previous_response_id " | ||
| "compaction when the underlying session retrieval limit hides local " | ||
| "history; use compaction_mode='input' instead." | ||
| ) | ||
| return "input" | ||
| if ( | ||
| mode == "auto" | ||
| and store is None | ||
|
|
@@ -189,10 +201,13 @@ async def run_compaction( | |
| self._last_unstored_response_id = None | ||
| else: | ||
| store = None | ||
|
|
||
| compaction_candidate_items, session_items = await self._ensure_compaction_candidates() | ||
| resolved_mode = self._resolve_compaction_mode_for_response( | ||
| response_id=self._response_id, | ||
| store=store, | ||
| requested_mode=requested_mode, | ||
| session_items=session_items, | ||
| ) | ||
|
|
||
| if resolved_mode == "previous_response_id" and not self._response_id: | ||
|
|
@@ -201,8 +216,6 @@ async def run_compaction( | |
| "when using previous_response_id compaction." | ||
| ) | ||
|
|
||
| compaction_candidate_items, session_items = await self._ensure_compaction_candidates() | ||
|
|
||
| force = args.get("force", False) if args else False | ||
| should_compact = force or self.should_trigger_compaction( | ||
| { | ||
|
|
@@ -391,6 +404,7 @@ async def _defer_compaction(self, response_id: str, store: bool | None = None) - | |
| response_id=response_id, | ||
| store=store, | ||
| requested_mode=None, | ||
| session_items=session_items, | ||
| ) | ||
| should_compact = self.should_trigger_compaction( | ||
| { | ||
|
|
@@ -442,7 +456,11 @@ async def _ensure_compaction_candidates( | |
| if self._compaction_candidate_items is not None and self._session_items is not None: | ||
| return (self._compaction_candidate_items[:], self._session_items[:]) | ||
|
|
||
| history = _normalize_compaction_session_items(await self.underlying_session.get_items()) | ||
| # Bypass SessionSettings.limit so compaction sees stored history, not just the | ||
| # retrieval window. Replacement still writes over the full store. | ||
| history = _normalize_compaction_session_items( | ||
| await self._get_all_underlying_session_items() | ||
| ) | ||
|
Comment on lines
+461
to
+463
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the underlying session has AGENTS.md reference: AGENTS.md:L201-L203 Useful? React with 👍 / 👎. |
||
| candidates = select_compaction_candidate_items(history) | ||
| self._compaction_candidate_items = candidates | ||
| self._session_items = history | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
compaction_mode="previous_response_id"wraps a session with a small retrieval limit, this condition raises as soon as stored history exceeds that limit—even if the default threshold has not been reached or a customshould_trigger_compactionhook would returnFalse. Because the runner invokesrun_compactionafter persisting a successful response, such runs now fail after observable work despite no compaction being due; reject this configuration before the run starts, or raise only after the decision hook selects compaction.AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.