fix(memory): Improve pagination behavior in get_last_k_turns() and list_messages() - #209
Conversation
| # Process events to group into turns | ||
| turns = [] | ||
| current_turn = [] | ||
| next_token = None |
There was a problem hiding this comment.
suggestion (non-blocking): This pagination logic (lines 805-857) is nearly identical to client.py lines 1108-1157. Consider extracting into a shared helper to reduce duplication and maintenance burden. Future bug fixes would need to be applied to both locations otherwise.
| """Test get_last_k_turns auto-calculates max_results based on k.""" | ||
| with patch("boto3.Session") as mock_boto_client: | ||
| mock_client_instance = MagicMock() | ||
| mock_boto_client.return_value = mock_client_instance |
There was a problem hiding this comment.
issue: This test mocks manager.list_events, but the new implementation in session.py no longer calls list_events() - it directly calls self._data_plane_client.list_events(). This mock won't intercept the actual calls.
Should mock manager._data_plane_client.list_events instead, similar to how test_client.py mocks client.gmdp_client.list_events.
| try: | ||
| max_results = (limit + offset) if limit else 100 | ||
| if fetch_all: | ||
| max_results = 10000 |
There was a problem hiding this comment.
nit: The value 10000 is a magic number. Consider extracting to a named constant (e.g., MAX_FETCH_ALL_RESULTS = 10000) for clarity and maintainability.
| @@ -786,52 +786,82 @@ | |||
| k: int = 5, | |||
| branch_name: Optional[str] = None, | |||
| include_parent_branches: bool = False, | |||
There was a problem hiding this comment.
thought: client.py uses include_branches while session.py uses include_parent_branches. This inconsistency predates this PR but is worth noting for future cleanup.
798e504 to
6d7f19e
Compare
…ist_messages() - get_last_k_turns(): Auto-calculate max_results based on k (max(100, k*3)) - list_messages(): Add fetch_all parameter to fetch all messages (up to 10000) - Backward compatible: default behavior unchanged
- Extract shared pagination logic into pagination.py helper - Fix test mocks to use _data_plane_client.list_events - Add MAX_FETCH_ALL_RESULTS constant (10000) in strands session_manager - Rename include_branches to include_parent_branches in client.py for consistency - Add comprehensive tests for pagination helper
6d7f19e to
355da39
Compare
| agent_id: str, | ||
| limit: Optional[int] = None, | ||
| offset: int = 0, | ||
| fetch_all: bool = False, |
There was a problem hiding this comment.
For fetch_all, we aren't really fetching all issue, just whatever MAX_FETCH_ALL_RESULTS is. Therefore, we should just remove this field.
| raise SessionException(f"Session ID mismatch: expected {self.config.session_id}, got {session_id}") | ||
|
|
||
| try: | ||
| max_results = (limit + offset) if limit else 100 |
There was a problem hiding this comment.
Let's keep this line but have MAX_FETCH_ALL_RESULTS which is 10000
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def paginate_turns( |
There was a problem hiding this comment.
I don't really seeing anyone using this function besides MemoryClient and Session class. I would either move into either class or both classes.
| branch_name: Optional[str] = None, | ||
| include_branches: bool = False, | ||
| max_results: int = 100, | ||
| include_parent_branches: bool = False, |
There was a problem hiding this comment.
If include_branches is already commited in the code, renaming this will introduce a breaking change
- Remove fetch_all parameter from list_messages (misleading name) - Use MAX_FETCH_ALL_RESULTS (10000) as default when no limit specified - Remove pagination.py module, inline logic into client.py and session.py - Revert include_branches rename to avoid breaking change
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
=======================================
Coverage ? 90.50%
=======================================
Files ? 35
Lines ? 3412
Branches ? 507
=======================================
Hits ? 3088
Misses ? 179
Partials ? 145
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Apply automatic formatting fixes identified by ruff format pre-commit hook. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
get_last_k_turns()in bothMemoryClientandMemorySessionManagernow automatically paginates untilkturns are found whenmax_resultsis not specifiedlist_messages()inAgentCoreMemorySessionManageraddsfetch_allparameter to retrieve all messages instead of being limited to 100max_resultsparameter behaves the sameProblem
Previously,
get_last_k_turns(k=200)would only fetch 100 events (the defaultmax_results) regardless of thekvalue requested. This caused issues when users needed more than 100 events worth of conversation turns.Similarly,
list_messages(limit=None)was hardcoded to return only 100 messages, making it impossible to retrieve all messages in a session.Changes
client.py&session.pymax_resultsdefault from100toNonemax_results=None, automatically paginates untilkturns are foundmax_resultsis explicitly provided, respects that limit (backward compatible)strands/session_manager.pyfetch_all: bool = Falseparameter tolist_messages()fetch_all=True, fetches up to 10,000 messagesTests
test_get_last_k_turns_auto_pagination- verifies pagination continues until k turns foundtest_get_last_k_turns_explicit_max_results- verifies backward compatibilitytest_list_messages_fetch_all- verifies fetch_all parameter works correctlyTest plan
pytest tests/bedrock_agentcore/memory/)Closes #206