From 8fa6f36982a516dc9dd1b18633e137d81a550c58 Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Wed, 4 Nov 2026 16:34:46 +0800 Subject: [PATCH] =?UTF-8?q?fix(agent):=20=E6=8F=92=E4=BB=B6=E5=B4=A9?= =?UTF-8?q?=E6=BA=83=E7=86=94=E6=96=AD=E4=BB=8E=E4=B8=8D=E7=94=9F=E6=95=88?= =?UTF-8?q?=20=E2=80=94=E2=80=94=20=E5=86=B7=E5=8D=B4=E9=94=9A=E7=82=B9?= =?UTF-8?q?=E7=94=A8=E4=BA=86=E4=BB=8E=E6=9C=AA=E8=B5=8B=E5=80=BC=E7=9A=84?= =?UTF-8?q?=E5=AD=97=E6=AE=B5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 缺陷（写上一个提交的判据时撞出来的） recordCrash 标记 Unhealthy=true，但 isHealthy 的冷却判据是： time.Since(r.LastReload) > reloadCooldown 而 LastReload **只在 markReloaded 里被赋值**（即"重载成功"之后）。 插件刚崩、从未重载时它是**零值**： time.Since(零值) ≈ 2000 年 >> 30s ⇒ 判"冷却期早已过去" ⇒ 立刻把 Unhealthy 置回 false 实测（连打 3 次 recordCrash）： crash #1 → needReload=false healthy=true crash #2 → needReload=false healthy=true crash #3 → needReload=true healthy=true ← 刚标完就自愈 ## 影响 三处「插件不健康则跳过执行」全是**死分支**： task.go:1065 stepToolBegin 中的跳过 task.go:1135 （上一提交刚补的那个分支的另一处同构） toolbatch.go:155 并发批次中的跳过 即：一个正在崩溃循环的插件会被**持续调用**，每次调用再崩一次。 recordCrash 返回的 needReload=true 也没意义 —— 下一句 isHealthy 就说它健康了。 pendingReloads 用同一个零值字段 ⇒ autoReloadPlugins 每次都会尝试重载 **刚崩的**插件，很可能立刻再崩（崩溃循环）。 ## 修法：冷却锚点改为 LastCrash 语义上「熔断」是"刚崩完先别用它，等 30s 再看" —— 观察窗口应锚在 **最近一次崩溃**，而不是"最近一次重载成功"。 isHealthy 锚点 LastReload → LastCrash pendingReloads 锚点 LastReload → LastCrash ## 顺带删掉冗余 unhealthyPlugins() 零生产调用方（全仓 grep 只有定义本身） record.FirstCrash 只写不读（写它的唯一目的是被下面这个判断用， 而那个判断早就改成只看 LastCrash 了） ## 判据（5 条，3 次变异全部实测变红） TestCircuitBreakerActuallyOpens ★ 主判据 变异：锚点改回 LastReload ⇒ "★ 连续 3 次崩溃后插件仍被判健康" TestCircuitBreakerWaitsForCooldownBeforeReload 变异：pendingReloads 锚点改回 LastReload ⇒ "★ 刚崩完就进了重载队列（应等冷却期）：[p]" TestCircuitBreakerRecoversAfterCooldown 变异：删掉恢复分支 ⇒ "★ 冷却期过后仍未恢复健康 —— 插件会被永久停用（比不熔断更糟）" TestCircuitBreakerUnknownPluginIsHealthy 从未崩过的插件不记账 TestCircuitBreakerWindowResets 窗口外的旧崩溃不该累加 ## 判据本身的两个坑（已避开） ① 只断言 recordCrash 的返回值 —— 它本来就返回 true，测了等于没测。 必须断言 **isHealthy 的结论**，那才是被熔断逻辑消费的东西。 ② 不测"冷却之后会恢复" —— 那样把代码改成恒 return false 也能过， 而那就成了永久禁用插件（比不熔断更糟）。 ## 连带：deniedpair_test 的前置条件改回真实状态机 上一提交里，第二条判据只能**手工置 Unhealthy**（因为 recordCrash 标完 立刻被自愈撤销）。现在熔断真生效了，改回用 recordCrash 打满阈值 —— 判据依赖公开状态机而不是私有字段。 ## 验证 go build ./... PASS go vet ./... PASS gofmt -l 干净 go test ./... -count=1 62 包全过，0 FAIL 干净检出（worktree） build + TestCircuitBreaker/TestDenied 全绿 --- internal/agent/core/deniedpair_test.go | 17 +-- internal/agent/core/plugin_health.go | 45 ++++---- internal/agent/core/plugin_health_test.go | 135 ++++++++++++++++++++++ 3 files changed, 169 insertions(+), 28 deletions(-) create mode 100644 internal/agent/core/plugin_health_test.go diff --git a/internal/agent/core/deniedpair_test.go b/internal/agent/core/deniedpair_test.go index 1b6de516..c8086875 100644 --- a/internal/agent/core/deniedpair_test.go +++ b/internal/agent/core/deniedpair_test.go @@ -36,7 +36,6 @@ import ( "context" "strings" "testing" - "time" agentAPI "github.com/JianFeeeee/HologramAgent/internal/agent/api" agentIO "github.com/JianFeeeee/HologramAgent/internal/agent/io" @@ -180,14 +179,16 @@ func TestUnhealthyPluginSkipStillRecordsResult(t *testing.T) { // 这是**另一个独立缺陷**（插件崩溃熔断实际从不生效），不在本次修复范围 // （本次只修 tool 配对）。这里直达状态，把判据聚焦在被测分支上； // 熔断那个缺陷应单独修并单独钉。 - // LastReload 必须设成"刚刚"，否则 isHealthy 的冷却判据 - //（time.Since(LastReload) > reloadCooldown）立刻判为已冷却 ⇒ 自愈。 - a.pluginHealth.mu.Lock() - a.pluginHealth.records[pluginName] = &pluginHealthRecord{ - Unhealthy: true, - LastReload: time.Now(), // 冷却期未过 ⇒ 熔断生效 + // ★ 走**真实状态机**，不直接改字段： + // 现在 isHealthy 的冷却锚点是 LastCrash（见 plugin_health.go）， + // 连续 recordCrash 达到阈值就会进入真正的熔断态。 + // + // 这条判据曾经只能手工置 Unhealthy —— 因为当时 recordCrash 标完 + // 立刻被 isHealthy 的自愈逻辑撤销（那是另一个缺陷，已单独修）。 + // 现在熔断真生效了，就用它自己的入口构造前置条件。 + for i := 0; i < maxPluginCrashes; i++ { + a.pluginHealth.recordCrash(pluginName) } - a.pluginHealth.mu.Unlock() if a.pluginHealth.isHealthy(pluginName) { t.Fatalf("★ 置 Unhealthy 后 %s 仍被判健康，判据前置条件不成立", pluginName) } diff --git a/internal/agent/core/plugin_health.go b/internal/agent/core/plugin_health.go index e146fb07..b93f8af2 100644 --- a/internal/agent/core/plugin_health.go +++ b/internal/agent/core/plugin_health.go @@ -19,7 +19,6 @@ type pluginHealthTracker struct { type pluginHealthRecord struct { CrashCount int - FirstCrash time.Time LastCrash time.Time Unhealthy bool LastReload time.Time @@ -45,7 +44,6 @@ func (t *pluginHealthTracker) recordCrash(plugin string) bool { if now.Sub(r.LastCrash) > crashWindow { r.CrashCount = 0 - r.FirstCrash = now } r.CrashCount++ @@ -61,7 +59,24 @@ func (t *pluginHealthTracker) recordCrash(plugin string) bool { return false } -// isHealthy 检查插件是否健康；冷却期后自动恢复 +// isHealthy 检查插件是否健康。 +// +// ## 冷却期必须从**最近一次崩溃**起算（2026-11-04 修的实测缺陷） +// +// 原实现是 `time.Since(r.LastReload) > reloadCooldown`，而 LastReload +// **只在 markReloaded 里被赋值**（即只在"重载成功"之后）。于是： +// +// recordCrash 标记 Unhealthy=true +// 但 LastReload 仍是零值 +// time.Since(零值) = 约 2000 年 ⇒ 远大于 30s +// ⇒ 判定"冷却期早已过去" ⇒ 立刻置回 healthy +// +// 实测：连续 3 次 recordCrash 后 needReload=true、Unhealthy=true， +// 紧接着 isHealthy 返回 **true**。即**崩溃熔断从不生效**， +// task.go 与 toolbatch.go 里三处「插件不健康则跳过执行」全是死分支。 +// +// 修法：冷却期以 LastCrash 为锚点 —— "刚崩完就先别用它，等 30s 再说"。 +// 这才是熔断的语义（原意是给插件一个恢复窗口，不是给重载计时的）。 func (t *pluginHealthTracker) isHealthy(plugin string) bool { t.mu.Lock() defer t.mu.Unlock() @@ -73,10 +88,11 @@ func (t *pluginHealthTracker) isHealthy(plugin string) bool { if !r.Unhealthy { return true } - if time.Since(r.LastReload) > reloadCooldown { + if time.Since(r.LastCrash) > reloadCooldown { r.Unhealthy = false r.CrashCount = 0 - log.Printf("[plugin] %s: cooldown passed, restored to healthy", plugin) + log.Printf("[plugin] %s: cooldown passed (%v since last crash), restored to healthy", + plugin, reloadCooldown) return true } return false @@ -106,21 +122,10 @@ func (t *pluginHealthTracker) pendingReloads() []string { if !r.Unhealthy { continue } - if now.Sub(r.LastReload) > reloadCooldown { - result = append(result, name) - } - } - return result -} - -// unhealthyPlugins 返回当前所有不健康的插件名 -func (t *pluginHealthTracker) unhealthyPlugins() []string { - t.mu.Lock() - defer t.mu.Unlock() - - var result []string - for name, r := range t.records { - if r.Unhealthy { + // 与 isHealthy 同一锚点（LastCrash）：刚崩完先等冷却，再尝试重载。 + // 原实现用 LastReload（零值 ⇒ 恒为真）⇒ 每次 autoReloadPlugins 都会 + // 尝试重载一个**刚崩的**插件，可能立刻再崩。 + if now.Sub(r.LastCrash) > reloadCooldown { result = append(result, name) } } diff --git a/internal/agent/core/plugin_health_test.go b/internal/agent/core/plugin_health_test.go new file mode 100644 index 00000000..f1871a15 --- /dev/null +++ b/internal/agent/core/plugin_health_test.go @@ -0,0 +1,135 @@ +package core + +// 判据：插件崩溃熔断**真的会熔断**。 +// +// ## 守的是什么（2026-11-04 实测缺陷） +// +// 原实现：recordCrash 标记 Unhealthy=true，isHealthy 却用 +// +// time.Since(r.LastReload) > reloadCooldown +// +// 而 LastReload **只在 markReloaded 里赋值**（重载成功后）。插件刚崩、 +// 从未重载时它是零值，time.Since(零值) ≈ 2000 年 ⇒ 远大于 30s +// ⇒ 判"冷却期早已过去" ⇒ **立刻把 Unhealthy 置回 false**。 +// +// 后果（这条比"少一个保护"严重）： +// +// agent 判定插件不健康 ⇒ 跳过执行、拉起重载 —— 这道保护**从不生效** +// recordCrash 返回 needReload=true 也没用：下一句 isHealthy 就说它健康了 +// +// 实测证据：连续 3 次 recordCrash 后 needReload=true、Unhealthy=true， +// 紧接着 isHealthy 返回 true。 +// +// ## 修法 +// +// 冷却期锚点从 LastReload 改为 **LastCrash** —— "刚崩完先别用它，等 30s"。 +// 这才是熔断语义：给插件一个观察窗口，不是给重载计时。 +// +// ## 这些判据最容易写废的地方 +// +// ① 只断言 recordCrash 返回值 —— 它本来就返回 true，测了等于没测。 +// 必须断言 **isHealthy 的结论**，那才是被熔断逻辑消费的东西。 +// ② 不测"冷却之后会恢复" —— 那样把代码改成"永不恢复"也能过， +// 而那就成了永久禁用插件。 + +import ( + "testing" + "time" +) + +// TestCircuitBreakerActuallyOpens 是主判据。 +// +// 变异：把 isHealthy 的锚点改回 r.LastReload ⇒ 本例红（立刻判健康）。 +func TestCircuitBreakerActuallyOpens(t *testing.T) { + tr := newPluginHealthTracker() + + for i := 0; i < maxPluginCrashes; i++ { + tr.recordCrash("p") + } + + if tr.isHealthy("p") { + t.Fatalf("★ 连续 %d 次崩溃后插件仍被判健康 ——\n"+ + "熔断从不生效，三处「插件不健康则跳过执行」全是死分支。\n"+ + "（原实现用 LastReload 作冷却锚点，零值让冷却显得早已过去）", + maxPluginCrashes) + } +} + +// TestCircuitBreakerWaitsForCooldownBeforeReload 钉住重载队列也要等冷却。 +// +// 原实现同样用 LastReload（零值）⇒ 每次 autoReloadPlugins 都会尝试重载 +// 一个刚崩的插件，很可能立刻再崩（崩溃循环）。 +// +// 变异：pendingReloads 锚点改回 LastReload ⇒ 本例红。 +func TestCircuitBreakerWaitsForCooldownBeforeReload(t *testing.T) { + tr := newPluginHealthTracker() + for i := 0; i < maxPluginCrashes; i++ { + tr.recordCrash("p") + } + if got := tr.pendingReloads(); len(got) != 0 { + t.Fatalf("★ 刚崩完就进了重载队列（应等冷却期）：%v", got) + } +} + +// TestCircuitBreakerRecoversAfterCooldown 钉住"冷却之后要恢复"。 +// +// 没有这条，把 isHealthy 改成恒返回 false 也能让主判据通过 —— +// 那就成了永久禁用插件（比不熔断更糟：一个抖动过的插件永远被停用）。 +// +// 变异：删掉 isHealthy 里的恢复分支（恒 return false）⇒ 本例红。 +func TestCircuitBreakerRecoversAfterCooldown(t *testing.T) { + tr := newPluginHealthTracker() + for i := 0; i < maxPluginCrashes; i++ { + tr.recordCrash("p") + } + if tr.isHealthy("p") { + t.Fatal("前置条件不成立：应当先处于熔断态") + } + + // 把最近崩溃时间往前拨，模拟"冷却期已过"。 + tr.mu.Lock() + tr.records["p"].LastCrash = time.Now().Add(-2 * reloadCooldown) + tr.mu.Unlock() + + if !tr.isHealthy("p") { + t.Fatal("★ 冷却期过后仍未恢复健康 —— 插件会被永久停用（比不熔断更糟）") + } + // 恢复后计数清零，下次崩溃重新从头计数。 + tr.mu.Lock() + cnt := tr.records["p"].CrashCount + tr.mu.Unlock() + if cnt != 0 { + t.Fatalf("★ 恢复后 CrashCount=%d，应清零（否则一次抖动就永久熔断）", cnt) + } +} + +// TestCircuitBreakerUnknownPluginIsHealthy 钉住边界：从没崩过的插件不记账。 +func TestCircuitBreakerUnknownPluginIsHealthy(t *testing.T) { + tr := newPluginHealthTracker() + if !tr.isHealthy("never-seen") { + t.Fatal("★ 从未崩溃的插件被判不健康") + } + if got := tr.pendingReloads(); len(got) != 0 { + t.Fatalf("★ 空表却给出重载队列：%v", got) + } +} + +// TestCircuitBreakerWindowResets 钉住崩溃窗口：稀疏的旧崩溃不该累加成熔断。 +func TestCircuitBreakerWindowResets(t *testing.T) { + tr := newPluginHealthTracker() + for i := 0; i < maxPluginCrashes-1; i++ { + tr.recordCrash("p") + } + // 把上次崩溃拨到窗口之外：下一次崩溃应从 1 重新计数。 + tr.mu.Lock() + tr.records["p"].LastCrash = time.Now().Add(-2 * crashWindow) + tr.mu.Unlock() + + need := tr.recordCrash("p") + if need { + t.Fatal("★ 窗口外的旧崩溃被累加进本次计数（应重新从 1 开始）") + } + if !tr.isHealthy("p") { + t.Fatal("★ 窗口重置后不该处于熔断态") + } +}