fix(adk): preserve nil agentCancelOpts in stopSignal.check to prevent UntilIdleFor from canceling agent#972
Merged
shentongmartin merged 4 commits intoalpha/09from Apr 17, 2026
Conversation
6c3a472 to
aa29f91
Compare
… UntilIdleFor from canceling agent
stopSignal.check() used append([]AgentCancelOption{}, s.agentCancelOpts...)
which converts nil to a non-nil empty slice. In watchStopSignal's tryCancel,
the nil check (opts == nil) then fails, causing an empty-opts call to
agentCancelFunc — equivalent to CancelImmediate. This means
Stop(UntilIdleFor(...)) spuriously cancels the running agent instead of
deferring the stop until idle.
Change-Id: Ifdbde679ea54bc45c876283ea7a00d6a9bb45e9c
aa29f91 to
00e8898
Compare
…lity comment Add TestStopSignalCheck_NilPreservedUnderConcurrentSignals to turn_loop_test.go, verifying that the nil guard in stopSignal.check() does not race with concurrent signal() calls under the race detector. Add an inline comment to tryCancel's nil check cross-referencing the three-way semantic documented on stopSignal.agentCancelOpts. Change-Id: I8c63db0980bb93a405e936cc2a47fb47586fa7b4
3f6f758 to
9f7a76d
Compare
Move all 12 TestAttack_* functions from attack_test.go into turn_loop_test.go and delete the now-empty file. No test logic changes. Change-Id: Ic0d8ce9ac043b2caebdf103e4df0fac573ec6f5b
9f7a76d to
e8de5a2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha/09 #972 +/- ##
===========================================
Coverage ? 82.29%
===========================================
Files ? 162
Lines ? 20593
Branches ? 0
===========================================
Hits ? 16946
Misses ? 2458
Partials ? 1189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
case 0 and case 1 in TestAttack_ConcurrentStopEscalation_RaceDetector were both calling WithImmediate(). Restore case 0 to bare Stop() to maintain the original 4-mode concurrent mix (bare, immediate, graceful timeout, UntilIdleFor). Change-Id: Id49dadc88b5d2719b0bb479c584216f869dfa86f
e57b092 to
e2506a8
Compare
meguminnnnnnnnn
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Stop(UntilIdleFor(...))spuriously cancels the running agent instead of deferring the stop until idle.Solution
Two fixes in
stopSignal.check()andStop():Nil-preservation in
check():append([]AgentCancelOption{}, nil...)returns[]AgentCancelOption{}(non-nil empty slice), notnil. ThewatchStopSignalgoroutine'stryCancelusesopts == nilto distinguish "no cancel intent" from "CancelImmediate" (empty opts). Added a nil guard socheck()returns nil whenagentCancelOptsis nil.Drop cancel opts when paired with
UntilIdleFor:Stop(UntilIdleFor(d), WithImmediate())is semantically contradictory — "wait for idle" + "cancel now".Stop()now clearsagentCancelOptswhenidleFor > 0. To escalate, users must issue a separateStop(WithImmediate())call.Key Insight
UntilIdleForoperates on a different axis than cancel options. It controls when the loop exits (after idle duration), while cancel options control how the running agent is interrupted. Combining them in the sameStop()call is meaningless: if you're waiting for idle, there's no running agent to cancel. Cancel opts only make sense in a follow-upStop()call that overrides the idle wait:问题
Stop(UntilIdleFor(...))会错误地取消正在运行的 agent,而非等待空闲后再停止。解决方案
在
stopSignal.check()和Stop()中进行了两处修复:check()中的 nil 保留:append([]AgentCancelOption{}, nil...)返回非 nil 的空切片[]AgentCancelOption{}。watchStopSignal的tryCancel通过opts == nil区分"无取消意图"和"CancelImmediate"(空 opts)。添加了 nil 判断,使check()在agentCancelOpts为 nil 时直接返回 nil。UntilIdleFor搭配 cancel opts 时丢弃后者:Stop(UntilIdleFor(d), WithImmediate())语义矛盾——"等待空闲" + "立即取消"。Stop()现在在idleFor > 0时清除agentCancelOpts。要升级停止方式,用户需发起单独的Stop(WithImmediate())调用。核心洞察
UntilIdleFor与 cancel 选项处于不同的语义轴。它控制的是循环何时退出(空闲一段时间后),而 cancel 选项控制的是如何中断正在运行的 agent。在同一个Stop()调用中组合它们没有意义:如果你在等待空闲,就没有正在运行的 agent 需要取消。Cancel opts 只在覆盖空闲等待的后续Stop()调用中才有意义。