-
Notifications
You must be signed in to change notification settings - Fork 648
fix(summarizing_conversation_manager): use model stream to generate summary #1653
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
fix(summarizing_conversation_manager): use model stream to generate summary #1653
Conversation
|
/strands review |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review SummaryAssessment: ✅ Approve Key Themes:
Strengths:
Minor Notes:
This is a solid bug fix that follows the project's patterns and maintains code quality. Nice work! 🎉 |
When no dedicated summarization_agent is provided (the default),
_generate_summary() was calling agent('Please summarize...') which
re-enters the full agent pipeline. Because the outer __call__ already
holds _invocation_lock (a threading.Lock), the inner call deadlocks.
Even with a separate event loop thread the lock is not re-entrant.
The fix splits _generate_summary() into two paths:
1. _generate_summary_with_agent() – used when a dedicated
summarization_agent was supplied at init time. Preserves the
existing behaviour (full agent pipeline, tool execution, etc.).
2. _generate_summary_with_model() – the new default path. Calls
agent.model.stream() directly with the summarization system prompt
and processes the response via process_stream(). This bypasses the
agent pipeline entirely, avoiding the lock, metrics reset, trace
span creation and tool-execution overhead.
Tests are updated accordingly:
- MockAgent now wires model.stream() to return proper async stream
events so the default path works in tests.
- New tests verify model.stream() is called directly, with correct
system prompt, with tool_specs=None, and that agent state is never
mutated in the default path.
- Noop-tool and state-restoration tests are moved to the agent path
(summarization_agent provided) where they remain relevant.
b619f0c to
43563cc
Compare
|
Just noting that we need to remember to add tracing/metrics to this. |
afarntrog
left a comment
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.
I would really like to see an integration test for this as well. Testing both cases - agent and direct call.
Description
When
SummarizingConversationManageris used without a separatesummarization_agentand aContextWindowOverflowExceptionoccurs, the agent crashes with:Root cause:
_generate_summary()callsagent("Please summarize this conversation."), which re-entersstream_async()on the same agent instance. The_invocation_lock(threading.Lock, non-reentrant) is already held by the outerstream_async()call, so the inner call fails immediately.The fix: Split
_generate_summary()into two code paths:When a separate
summarization_agentIS provided — keep existing behavior, callsummarization_agent(...). This is safe because it's a different agent instance with its own lock.When NO separate agent is provided (the bug case) — call
agent.model.stream()directly instead ofagent(...). Summarization only needs the model to produce text; it doesn't need tools, callback handlers, tracing, or any other agent machinery. The response stream is processed viaprocess_stream()(the same function the event loop uses internally).This approach:
agent.py— the core stays simpleRLock, no lock release/reacquire hacks)Related Issues
Fixes the re-entrant lock deadlock in
SummarizingConversationManagerwhen no dedicatedsummarization_agentis configured.Documentation PR
N/A — no public API changes; behavior is now correct where it previously crashed.
Type of Change
Bug fix
Testing
How have you tested the change?
37 tests total (28 existing updated + 9 new), all passing:
test_full_agent_pipeline_no_reentrant_lock_on_context_overflow) reproduces the exact bug scenario end-to-end: a realAgentinstance callsagent("prompt")→ model raisesContextWindowOverflowException→reduce_context()fires while the lock is held → summary is generated viamodel.stream()→ event loop retries → agent returns successfully. This test would have caught the original bug.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.