refactor: replace boolean endInvocation flag with atomic.Bool to enable cross-context state propagation#1135
refactor: replace boolean endInvocation flag with atomic.Bool to enable cross-context state propagation#1135hanorik wants to merge 1 commit into
Conversation
…le cross-context state propagation
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Thanks for this — the value-copy propagation bug is real, and the red→green tests (TestSubAgentCallbackEndInvocationPropagated, TestSequentialAgent_SubAgentEndInvocationPropagated) capture it well. I reproduced it locally: both new tests fail on main and pass with this change, the full go test -race ./... suite is green (one unrelated remoteagent timing flake), and there's no public API change. Keeping EndInvocationPtr() off the public interface via a type assertion is a nice touch.
I left a few inline notes. The main things I'd like to see before merge:
- Live path isn't covered —
sequentialAgent.RunLivedoesn't have theEnded()short-circuit thatRunnow has (inline). - ParallelAgent semantics need a deliberate decision — sharing one flag across siblings changes isolation behavior (inline).
initEndInvocationisn't thread-safe — worth eager-initializing (inline).- Test coverage —
loopagent,parallelagent(shared atomic), andworkflow/agent_node.goare changed but untested; could you add cases for those? - Rebase on
main— the branch predates therunner.NewInMemoryaddition, so an API diff against currentmainlooks misleadingly like a removal; a rebase clears it. (It also sets up the small follow-up that #370 describes.)
Overall the core mechanism and the sequential propagation are solid — nice work.
| func (a *sequentialAgent) Run(ctx agent.InvocationContext) iter.Seq2[*session.Event, error] { | ||
| return func(yield func(*session.Event, error) bool) { | ||
| for _, subAgent := range ctx.Agent().SubAgents() { | ||
| if ctx.Ended() { |
There was a problem hiding this comment.
Could the same guard go in RunLive? It iterates sub-agents without checking ctx.Ended(), so a sub-agent ending the invocation won't stop the following ones in live/streaming mode — the fix would then cover both paths.
| Agent: subAgent, | ||
| UserContent: ctx.UserContent(), | ||
| RunConfig: ctx.RunConfig(), | ||
| EndInvocation: endInvPtr, |
There was a problem hiding this comment.
Sharing the parent's atomic across all siblings means one sibling ending the invocation can stop the others mid-flight (through the new Ended() checks in nested sequential/loop agents). That's a change from the "isolated" behavior documented for this agent, and from adk-python, where the workflow agents don't consult end_invocation at all. If cross-sibling termination is intended, a short comment on the rationale would help; if not, consider giving each sibling its own flag and reconciling back to the parent after the group completes.
| endInvocation *atomic.Bool | ||
| } | ||
|
|
||
| func (c *invocationContext) initEndInvocation() { |
There was a problem hiding this comment.
This lazy if == nil { new(...) } is a check-then-set on the pointer field, so it isn't thread-safe on its own. It's fine today because production contexts are eager-initialized in NewInvocationContext and the only lazily-initialized type is test-only — but in a change whose purpose is safe shared state, eager-initializing at construction (and dropping the lazy path) would remove the footgun and simplify the call sites.
Fixes an issue where
EndInvocation()called inside sub-agent callbacks or child nodes did not propagate to parent/sibling contexts becauseEndInvocationwas stored as a value-copiedbool.Changes
EndInvocation boolwith*atomic.BoolacrossInvocationContextimplementations (agentandinternal/context).EndInvocationPtr() *atomic.Boolso derived child contexts (WithContext,WithICDelta,parallelagent,workflow.AgentNode) share the same underlying atomic flag.sequentialagentandloopagentto checkctx.Ended()and stop execution when a sub-agent ends the invocation.Testing Plan
sequentialagent,agent, andinternal/contextpackages verifying thatEndInvocation()propagates across child/sub-agent contexts.go test -race ./....