Repository navigation
fix: [BUG] SummarizingConversationManager's pin_first does not work correctly with FileSessionManager and S3SessionManager (#4027) [replica of harness-sdk PR 4066] - #37
Conversation
…ds-agents#4027) Two regressions from SummarizingConversationManager(pin_first=N): 1. After compaction, FileSessionManager/S3SessionManager restore skipped the pinned head: list_messages(offset=removed_message_count) used the pre-summarization offset, which now coincides with the still-live pinned messages. The resumed agent lost the first N messages (test_a_resumed_conversation_is_the_one_the_agent_had). 2. A failing summary generation mutated bookkeeping (removed_message_count, _pin_first_applied, _summary_message) before raising, so the next compaction attempt saw a corrupted state (test_a_resumed_conversation_ keeps_what_no_summary_replaced). Fix: * _pin_first_applied now round-trips through get_state/restore_from_session. * apply_management snapshots the three mutable fields and rolls back on any exception so a failed summary leaves the agent untouched. * RepositorySessionManager.initialize re-fetches the pinned head from the session repository and prepends it to the restored transcript; the list_messages offset shifts by +pin_first so the tail does not overlap with the reattached head. Regression tests cover both bugs end-to-end (roll-back, pinned-restore, no-pin-baseline, no-summary-yet). 89/89 tests pass in strands-py/tests/strands/{agent/test_summarizing_conversation_manager, session/test_repository_session_manager}.py. Refs: strands-agents#4027 style: apply ruff formatting
Address review feedback on PR strands-agents#4066: Finding 1: Persist pinned_head_count (the actual count of preserved messages) in the base ConversationManager class rather than a boolean. This fixes: - pin_first changed between runs (different count → wrong restore) - pinned head ends mid tool-pair (count differs from configured pin_first) Finding 2: Re-apply pin markers to restored pinned messages so the next compaction respects them. Without this, the first restore worked but the next compaction summarized away the 'pinned' messages. Finding 3: When the tail comes back empty but the store is non-empty, seed the append cursor from the last stored message to prevent overwrites. Finding 4: Add tests for legacy state migration (missing pinned_head_count defaults to 0) and port the issue strands-agents#4027 end-to-end File-backed repro. Finding 5: Replace weak retry test with one that induces failure then succeeds, verifying exact transcript and removed_message_count. Style: Make _summarize_oldest atomic by reordering mutations after summary generation succeeds, removing the try/rollback pattern.
Constructor pin_first can disagree with the stored head size. Add a restore test that would pass if we still used the constructor value, plus an empty-tail append that must not restart message ids. Co-authored-by: Cursor <cursoragent@cursor.com>
…rst-session-restore # Conflicts: # strands-py/src/strands/session/repository_session_manager.py # strands-py/tests/strands/session/test_repository_session_manager.py
test_initialize_pinned_messages_no_summary_yet was vacuous: replacing the gate (if pinned_head_count > 0:) with (if True:) left all tests green because list_messages(limit=0) returns empty in all implementations. Redesign the test to spy on list_messages and assert that no call with limit=0 is issued when pinned_head_count == 0. The gate's purpose is to suppress the pinned-head fetch for pre-compaction sessions; the spy makes that invariant enforceable — the test now fails when the gate is replaced with (if True:).
test_reduce_context_retry_after_failure_records_correct_count exercised the proactive path (e=None) for both the failure and the retry calls. The test never proved it was a retry-after-failure in the reactive sense, and no assertion on model.stream.call_count meant the failure call could be skipped without breaking the test. Fix: pass e=overflow for the first (failing) call so it exercises the reactive path which re-raises the error. Assert model.stream.call_count==2 after both calls to prove the failure call was not skipped.
Replaces the private pin-first flag with the public persisted count the reviewer asked for, and lets SlidingWindowConversationManager reuse it. Reviewer item mapping: - Blocking 2 (public pinned_head_count, carried in get_state/restore_from_session, mirrors removed_message_count): PASS. SlidingWindowConversationManager reads the persisted count instead of its own `_pin_first_applied`, which closes strands-agents#4087's shared defect for that manager too; verified by running its tests, not asserted. - Fold request (fold test_initialize_restores_summary_without_pin_first into the offset-restore test, keep the exact-transcript assert): PASS. Evidence: - pytest tests/strands/session tests/strands/agent/conversation_manager -> 306 passed in 78.93s - pytest (pin_first / sliding-window blast radius: test_conversation_manager.py, test_agent_context_manager.py, test_agent_model_state.py, test_tracking_id_history_invariant.py, _context_manager/test_context_manager.py) -> 132 passed in 53.13s - ruff check, ruff format --check, mypy on the changed source: rc=0 each - the downgrade guard is non-vacuous: coercing the persisted count to a boolean at conversation_manager.py:158 fails test_initialize_uses_persisted_pinned_head_count_not_constructor ("At index 1 diff: 'summary' != 'pinned-1'"); reverted, the same selection passes 2/2.
…count is_pinned protects a pinned message's adjacent tool-pair partner as well, so trimming keeps more leading messages than pin_first names. The sliding window recorded min(pin_first, len), so with pin_first=2 and an opening tool call the live head was 3 messages but the persisted count said 2 — a restored session reattached too few, offset past the real toolResult, and the session manager fabricated an interrupted-tool result in its place. Count the protected prefix instead, and cover it with a unit test plus two FileSessionManager restore tests (plain trim and pinned tool pair).
The restore path carried a private copy of pin_message; call the compression helper instead so the pin marker is written in one place. Also documents pinned_head_count on the base ConversationManager alongside removed_message_count.
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v4-reason (optimized) review — run af1746, Opus 4.8
Assessment: comment
Assessment: Comment
The fix is well-structured: making the summarization bookkeeping atomic (mutate only after _generate_summary succeeds) and persisting a pinned_head_count so restore can both reattach the live pinned head and offset the tail past it. Test coverage is strong, including legacy-state, empty-tail, pin-marker, and retry-after-failure cases.
Review Categories
- Correctness: One edge case worth confirming —
pinned_head_countis set from the pinned subset within the summarize range, which could under-count if the split point ever lands inside the pinned prefix on a later compaction. The prior-summaryremoved_count -= 1also assumes the old summary is into_summarize. - Performance: Empty-tail fallback lists the entire store just to read the last message id.
- API surface: New public
pinned_head_count+ persisted-state key — confirm API review/label. - Testing: Primary restore test asserts text-only; consider verifying full shape/markers.
Solid, carefully reasoned fix with thoughtful regression tests.
| # All mutations below this point: summary generation succeeded, commit the changes. | ||
| removed_count = len(to_summarize) | ||
| if self._summary_message: | ||
| removed_count -= 1 |
There was a problem hiding this comment.
Important
Issue: self.pinned_head_count = len(protected_to_preserve) is an unconditional assignment to the count of pinned messages found only within [0, messages_to_summarize_count). On a later compaction where the adjusted messages_to_summarize_count does not span the entire live pinned head (e.g. the split point lands inside the pinned prefix), protected_to_preserve would under-count the still-live pinned messages, and the persisted pinned_head_count would shrink below the real head — causing restore to reattach too few head messages and offset the tail incorrectly.
Suggestion: Consider deriving the count from the full protected prefix of the resulting transcript (e.g. count_protected_prefix(agent.messages) after the splice), or assert that messages_to_summarize_count always covers the pinned head. Please confirm the split-point adjustment can never land inside the pinned prefix.
| self._latest_agent_message[agent.agent_id] = session_messages[-1] | ||
| elif pinned_head_count > 0 or offset > 0: | ||
| all_messages = self.session_repository.list_messages( | ||
| session_id=self.session_id, |
There was a problem hiding this comment.
Suggestion
Issue: The empty-tail fallback issues an unbounded list_messages(...) with no limit purely to read the last stored message's message_id. On a large store this materializes the entire message history just to recover one id, which can be expensive during restore.
Suggestion: If the repository API supports it, fetch only the final record (e.g. a reverse/last-N query or a dedicated "latest message" lookup) instead of listing everything. If no such API exists, a brief comment noting the cost tradeoff would help future readers.
| self._summary_message = self._generate_summary(to_summarize, agent) | ||
| # Assign tracking id to the summary message since it bypasses the append method. | ||
| _ensure_tracking_id(self._summary_message) | ||
| # All mutations below this point: summary generation succeeded, commit the changes. |
There was a problem hiding this comment.
Suggestion
Issue: removed_count -= 1 subtracts for the previous summary message based solely on self._summary_message being set, assuming that prior summary was always inside to_summarize. If a prior summary message is not part of the current to_summarize partition (for instance if it ended up protected/pinned or outside the summarized range), this subtraction over-decrements removed_message_count and the restore offset drifts.
Suggestion: Guard the decrement on the prior summary actually being present in to_summarize rather than merely existing, e.g. only subtract when self._summary_message in to_summarize. This behavior predates the PR, but since the atomicity rework touches this exact block it's worth confirming.
|
Live Strandly's actual comments on the original PR 4066 (sanitized copy: mentions escaped, links backticked) Review (CHANGES_REQUESTED):
Review (APPROVED):
Review (APPROVED):
Review (CHANGES_REQUESTED):
Inline at
Inline at
Inline at
Inline at
Inline at
Inline at
|
Replica of
https://github.com/strands-agents/harness-sdk/pull/4066(backticked: no cross-reference). Below: the v4-reason optimized review and a sanitized copy of live Strandly's actual comments.