fix(panic-isolation): extend SafeGo coverage to meshnode/firewall/cert/ddns loops

[suantea]

概述

承接 #115 的 SafeGo 机制，把其余长驻 ticker 循环也纳入 panic 隔离，彻底落实「部分崩溃不影响核心」。

改动

• meshnode 心跳循环、firewall 定时自动同步、cert 续期/ACME 流程两个循环、ddns 每任务循环，全部从裸 go 换成 svcutil.SafeGo（panic 自动重启 + 指数退避）
• 短命连接协程（如 storage SFTP 连接处理）保持原样，无需隔离

验证

• go build / go vet / 全量 go test ./... 通过

[pikachuren]

🙏 感谢 @suantea 提交！
🤖 AI 自动审核声明：本评审报告由 AI 自动生成，当前使用 DeepSeek-V4.1-Flash 模型进行分析。
⚠️ AI 分析结果仅供参考，可能存在误判或遗漏。如您发现任何问题或有不同意见，欢迎随时提出讨论和纠正。
⚠️ 重要提醒：即使 AI 评审认为代码质量良好且建议合并，最终是否合并仍需由项目维护者进行人工判定。项目维护者会综合考虑代码质量、项目规划、技术方向、团队资源等多方面因素做出是否合并的决策。

📖 PR #118 — fix(panic-isolation): extend SafeGo coverage to meshnode/firewall/cert/ddns loops

🎯 结论

结论：🔄 Request Changes（仅因 1 个 P1 的"重启后任务生命周期错乱"，修掉即可 Approve）

评分：功能 ⭐⭐⭐⭐ | 最小改动 ⭐⭐⭐⭐⭐ | 前向兼容 ⭐⭐⭐⭐⭐ | 方案设计 ⭐⭐⭐

变更：4 文件（service/meshnode/manager.go +2/-1、service/firewall/manager.go +3/-2、service/cert/manager.go +3/-2、service/ddns/manager.go +6/-1，own-diff +14 / -6，共 105 行；行号以自身增量的 + 侧为准）

说明：GitHub 累积 diff 显示 16 文件 / +527（含 #113–#117）；本评审只针对 git diff 63f5046 4176cb0。

整体方案（2-3 句）：把 4 处长驻循环从裸 go 换成 #115 定稿的 svcutil.SafeGo(m.log, name, true, fn)，动机是"承接 #115，让部分崩溃不影响核心"，范围克制、逐点对应，没有顺手重构。逐条核对后可以确认：这 4 处都没有把 wg.Add / defer wg.Done() 放进会被重启的函数体内（详见下方），所以不存在"重启后 WaitGroup 计数变负"的问题。真正需要处理的是一处语义冲突：DDNS 的 runDDNS 在 defer 里做"停止任务"的收尾，而 SafeGo 的重启会重复执行这段收尾，导致重启后的任务在面板上显示已停止、且再也停不下来。另外 3 处（firewall 1 处、cert 2 处）循环没有退出通道，与 #115 注释里"fn 需自己响应 stopCh 退出"的约定不完全一致（但不会造成关闭死锁，见下文澄清）。

核心改动清单（3-6 条）：

1. service/meshnode/manager.go:50 — go m.heartbeatLoop() → svcutil.SafeGo(m.log, "meshnode.heartbeat", true, m.heartbeatLoop)；循环体 manager.go:68-75 已响应 m.ctx.Done()（Stop() 会 m.cancel()，manager.go:55-58）。
2. service/firewall/manager.go:159-166 — 匿名 goroutine 包进 SafeGo(..., "firewall.autosync", true, ...)；ticker 建在 fn 内部（重启即全新状态），但 for range ticker.C（manager.go:162）没有退出通道。
3. service/cert/manager.go:82-83 — autoRenewLoop / acmeFlowLoop 分别包进 SafeGo；两个循环 manager.go:94 / manager.go:107 同样是 for range ticker.C，manager 内既无 ctx 也无 Stop()。
4. service/ddns/manager.go:82-84 — 每任务循环包进 SafeGo(m.log, fmt.Sprintf("ddns.task-%d", id), true, …)；manager.go:146 每轮上报 BeatEngineHeartbeat("ddns")，循环体 manager.go:141-144 响应 ctx.Done()。
5. 四处均使用 m.log（各 manager 自己的 logger），与 #115 在 monitor 包使用 logrus.StandardLogger() 的做法不一致——本 PR 这一侧更合适（见 P2 跨 PR 提示）。

问题清单：

• [P1] backend/service/ddns/manager.go:124-128（+ 侧，与 manager.go:82-84 组合触发） — runDDNS 的 defer 里做的是任务终止收尾（m.entries.Delete(id) + DB status = "stopped" + 日志"已停止"），但 runDDNS 现在是"会被 SafeGo 重启的函数"。触发条件：doUpdate 或循环体里任意一次 panic（新引入的 DDNS 供应商解析、JSON 解析等都是可能来源）→ recover → defer 执行 → 条目被删、DB 变 stopped → SafeGo 退避后重新启动 runDDNS，此时：

  • GetStatus(id)（manager.go:105-110）走 entries → 返回 "stopped"，面板显示已停止，但更新循环还在跑；
  • 用户点"停止" → Stop(id)（manager.go:95-102）在 entries 里找不到条目 → 只改 DB，无法取消那个孤儿 goroutine；
  • 用户再点"启动" → Start(id) 新建 ctx 并 entries.Store → 同一个任务两个循环并存，双重更新 + 双重写库。

  最小改法：让收尾只在"真正被取消/退出"时执行，panic 重启路径不清理（保持 entries 可被 Stop 取消）：

  defer func() {
      if ctx.Err() == nil {   // panic 重启时 ctx 未被取消，不做终止收尾
          return
      }
      m.entries.Delete(id)
      m.db.Model(&model.DDNSTask{}).Where("id = ?", id).Update("status", "stopped")
      m.log.Infof("[DDNS][%d] 已停止", id)
  }()

  （或把这段收尾从 runDDNS 移到 Start 外层、由 Stop 负责 —— 只要它与"可重启的 fn"解耦即可。）

• [P1] backend/service/firewall/manager.go:159-166、backend/service/cert/manager.go:82-83（+ 侧） — 这 3 处循环没有任何退出通道：for range ticker.C 只能靠进程退出结束，Manager 里既没有 ctx/cancel 也没有 Stop()，main.go:391-392 的 registerStopHandlers(...) 参数表里也没有 firewallMgr / certMgr。

  先澄清一个流传的说法：这里不会出现 "Stop() 的 wg.Wait() 永久阻塞" —— 已逐行核对，firewall.Manager 与 cert.Manager 结构体（firewall/manager.go:18-26、cert/manager.go:69-74）里没有 sync.WaitGroup，也没有任何 Stop 方法，因此当前没有可被阻塞的 Wait()。真实影响是两点：(1) 无法优雅停止，正常关停/未来做 in-process 重载时会泄漏 goroutine；(2) 一旦后续有人按 linereg 的写法给这两个 manager 补上 wg.Add(1) + defer wg.Done() + Stop(){ wg.Wait() }，就会立刻变成永久阻塞。

  ⚠️ 跨 PR 提示（不是本 PR 的问题，但它就在这条栈的下层）：#115 已经把"wg.Add(1) 在 Start() 里、defer wg.Done() 在被重启的 fn 里、Stop() 里 wg.Wait()"这个组合写进了两处 —— service/monitor/probe.go:104-106（p.wg.Add(1) + fn 内 defer p.wg.Done()，Stop() 在 probe.go:74 调 p.wg.Wait()）和 service/linereg/linereg.go:343-344/358-359（m.wg.Add(1) + m.run 内 defer m.wg.Done()，Stop() 在 linereg.go:354 调 m.wg.Wait()）。在 restart=true 下，一次 panic 会让 Done() 被重复调用 → 计数变负（Done() 触发 sync: negative WaitGroup counter panic，被 SafeGo 自己 recover）→ 之后 wg.Wait() 因计数不为 0 而永久阻塞（lineregMgr.Stop() / monitorMgr.Stop() 都在 main.go:391-392 的 registerStopHandlers 里），即优雅关闭会挂住。建议在合并这条栈之前统一修掉：把 wg.Add(1) 移到需要计数的不可重启外层（例如各自 SafeGo 外再包一层只跑一次的 goroutine），或干脆不用 WaitGroup 而用"ctx 取消 + 完成 channel"来等一轮退出。

  对本 PR 而言，建议顺手把退出通道补上，与 #115 的注释约定保持一致：

  // Manager 增加字段：ctx context.Context; cancel context.CancelFunc（NewManager 中建）
  svcutil.SafeGo(m.log, "firewall.autosync", true, func() {
      ticker := time.NewTicker(30 * time.Minute)
      defer ticker.Stop()
      for {
          select {
          case <-m.ctx.Done():
              return
          case <-ticker.C:
              m.log.Info("[Firewall] 定时触发系统防火墙规则同步")
              m.SyncSystemRulesAsync()
          }
      }
  })

• [P2] backend/service/ddns/manager.go:146（+ 侧） — 心跳 key 用了固定的 "ddns"，而 SafeGo 的 name 是 fmt.Sprintf("ddns.task-%d", id)。多个 DDNS 任务共用同一个心跳槽位，一个任务健康就会掩盖另一个任务的死亡；另外首次上报要等到第一轮 tick，多数任务间隔 ≥300s，因此启动后较长时间内该 key 不存在（此时不计入检查）。是否考虑把 key 改成与任务绑定（fmt.Sprintf("ddns.task-%d", id)），并按任务自己的间隔设定宽限期？这一条与 PR #117 的 3 分钟固定阈值叠加后会出现误报，详见 PR-117 报告 [P0]。

• [P2] backend/service/cert/manager.go:82-83、service/firewall/manager.go:159（+ 侧） — 心跳覆盖面与 SafeGo 覆盖面不一致：本 PR 只给 ddns 加了上报，cert（2 个循环）、firewall、meshnode 仍未上报，/system/health 看不到它们的存活。要么都补上，要么在 svcutil 的注释里说明"仅覆盖部分引擎"，以免看门狗误以为已全覆盖。

• [P2] 与 #115 的重启策略有关（非本 PR 引入，但被本 PR 放大） — svcutil.SafeGo 的重启次数无上限、退避在单次 SafeGo 调用内只增不减且成功后不重置（svcutil/safego.go:26-40：backoff 是闭包局部变量，一旦升到 60s 就固定 60s），失败时每轮都打印完整 debug.Stack()。本 PR 把这种"永续重启"从 4 个循环扩到 8 个，确定性 panic 下会变成每 60s 一条堆栈的长期噪声（尚不构成日志风暴）。是否考虑在 SafeGo 里加上"连续 N 次 panic 后放弃并只打一条 error" + "稳定运行超过 X 后重置退避"？这块归 #115 的代码，建议后续统一处理。

• [P2] service/meshnode/manager.go:19（+ 侧） — 导入块被拆成 logrus 与 svcutil 两个分组（manager.go:16-20），与同包其它文件的 import 分组风格略不一致，属风格问题，可忽略。

产品视角评估（3-5 句）：把剩下几个长驻循环纳入 panic 隔离，是"稳定性质变"的延续工作，用户可感知的收益是"某个引擎出错不再整体挂掉"，方向没有问题，改动量也控制得很好（+14/-6）。产品层面唯一需要补齐的是状态一致性：DDNS 在 panic 重启后会呈现"UI 显示已停止、后台仍在跑、按钮点了没反应"的状态，这对运维是最难排查的一类现象，建议先修掉再合。另外从"用户能不能看见"的角度，本 PR 与 #117 刚好是配套的 —— 建议在补心跳覆盖面时优先覆盖 firewall/cert，让自检端点真正能反映这几个引擎的状态。

兼容性/迁移风险：无破坏性变更。4 处都是同一进程内的 goroutine 启动方式替换，无 API / 配置 / schema 变化；行为差异仅在"panic 后是否继续运行"（原本是整个进程或该 goroutine 直接消失），对用户是纯改善。需要留意的只有 P1 那条 DDNS 生命周期语义。

值得肯定的点（1-3 条）：

1. 逐条核对通过：meshnode（manager.go:50）、firewall（manager.go:159）、cert（manager.go:82-83）、ddns（manager.go:82-84）四处都没有把 wg.Add/defer wg.Done() 放进会被重启的函数里（只有 checkAllNodes/checkPeerLatencies 内部有局部 WaitGroup，作用域与重启无关，meshnode/manager.go:86-94 / 158-179），这一点避免了 #115 留下的那类隐患。
2. ticker 全部建在传给 SafeGo 的 fn 内部（firewall/manager.go:160、cert/manager.go:88/101、ddns/manager.go:135），符合 #115 注释里"重启时从函数头重新执行、每次都是全新状态"的约定，重启后不会出现 ticker 泄漏或立即触发。
3. 四处都用各 manager 自己的 m.log 而不是全局 logger，panic 栈会进入对应模块的 DB 日志，比 logrus.StandardLogger() 更利于事后排查（这一侧比 #115 更规范）。

建议操作理由（1 句）：范围克制、无死锁、无 WaitGroup 隐患，只需修掉 DDNS 重启后"显示已停止但仍在运行、且无法停止"的生命周期冲突，并顺手给 firewall/cert 的循环补一个退出通道，就可以合并。

[suantea]

状态说明：本分支暂未 restack

按维护者建议（#87 上「每个 PR 都基于 main 重新创建，PR 之间的修改不要重叠」）推进 restack 时，本分支被识别为栈依赖，不能独立重建，故本次未推送。

原因：本分支相对 main 的自身增量依赖 svcutil.SafeGo / svcutil.BeatEngineHeartbeat，该符号由 #115（fix/panic-isolation）引入。单独 cherry-pick 到 main 会编译失败：

service/ddns/manager.go:82:10: undefined: svcutil.SafeGo
service/ddns/manager.go:146:12: undefined: svcutil.BeatEngineHeartbeat
service/firewall/manager.go:159:10: undefined: svcutil.SafeGo

根因：本分支此前的 merge-base 停在 dc2877b（#80 合并点），且 #113–#118 的 commit 同时挂在多条分支上 —— 原始依赖链是 #113 → #114 → #115 → #116 → #117 → #118（逐条叠放），并非 6 条各自独立。这也正是维护者所说「PR 之间的修改不要重叠」的问题所在。

已验证：原始分支（未 restack）go build ./... 通过；单独 restack 到 main 则失败（同上错误）。

下一步：需要先合并 #115（fix/panic-isolation，已 restack 并推送，新 head 76445d8），或把本分支改为基于 restack 后的 #115 叠放。请维护者指点采用哪种方式；确认后我会立即重建本分支。

[suantea]

尝试独立化失败：本 PR 无法拆出对 #115 的依赖，建议并入 #115 或等其合并

按维护者建议（#87「每个 PR 都基于 main 重新创建，PR 之间的修改不要重叠」）尝试把本 PR 重建为基于 main 的独立分支，实测不可行，故本次未改动本分支（仍保持原 head 4176cb0）。

实测结果

git cherry-pick 成功，但构建失败：

service/firewall/manager.go:159:10: undefined: svcutil.SafeGo
service/ddns/manager.go:82:10:       undefined: svcutil.SafeGo
service/ddns/manager.go:146:12:      undefined: svcutil.BeatEngineHeartbeat
service/meshnode/manager.go:50:10:   undefined: svcutil.SafeGo

根因

本 PR 的全部改动（4 个文件 / 14 行）就是为 svcutil.SafeGo 增加调用点：

backend/service/cert/manager.go     | 5 +++--
backend/service/ddns/manager.go     | 6 +++++-
backend/service/firewall/manager.go | 5 +++--
backend/service/meshnode/manager.go | 4 +++-

而 SafeGo / BeatEngineHeartbeat 都不在 main 上 —— main 的 backend/pkg/svcutil/ 只有 Windows 服务相关函数（IsWindowsService / RunService / InstallService / UninstallService / StartService / StopService / runCmd），七个都没有 SafeGo。这些函数由 #115（fix/panic-isolation）引入。

因此本 PR 无法在不依赖 #115 的前提下存在：抽掉 SafeGo，本 PR 就没有内容了。这与 #117 不同 —— #117 的 GetHealth 有独立内核（DB 读写探针），心跳只是其中一项，所以能独立化。

建议处理方式（请维护者选择）

1. 等 fix(panic-isolation): SafeGo isolates engine loop panics with auto-restart #115 合并后再推：本 PR 保持 #115 → #118 的栈依赖；fix(panic-isolation): SafeGo isolates engine loop panics with auto-restart #115 已 restack 并推送，新 head 76445d8。
2. 把本 PR 并入 fix(panic-isolation): SafeGo isolates engine loop panics with auto-restart #115：两者本是同一次 panic 隔离改造的两半（fix(panic-isolation): SafeGo isolates engine loop panics with auto-restart #115 引入 SafeGo，fix(panic-isolation): extend SafeGo coverage to meshnode/firewall/cert/ddns loops #118 扩展覆盖范围），合并为一个 PR 更符合"一个 PR 只做一件事"。我倾向这个。

请指教采用哪种方式，确认后我立即执行。

[pikachuren]

🙏 感谢 @suantea 提交！
🤖 AI 自动审核声明。⚠️ 最终合并决策由项目维护者判定。

🎯 结论

🔄 Request Changes — ddns 心跳 key 硬编码会隐藏个体故障，建议先修复

📖 概要

扩展 SafeGo 覆盖到 meshnode/firewall/cert/ddns 循环

🚨 关键问题

P0：

• ⚠️ ddns/manager.go SafeGo 实例名按任务 ID 区分（ddns.task-%d），但心跳调用 BeatEngineHeartbeat("ddns") 硬编码固定 key。若配置了多个 DDNS 任务，只要有一个任务正常打点，其余任务即使已陷入 panic-重启循环，健康检查的 engine_ddns 依然显示 ok，个体故障被完全掩盖。这是与 #115（monitor/probe）同一类问题被重新引入的新实例，建议改为 BeatEngineHeartbeat(fmt.Sprintf("ddns-%d", id))。

P1：

• 💡 meshnode/firewall/cert 三个新覆盖的循环均未接入 BeatEngineHeartbeat，健康检查对这几个引擎完全没有可观测性，"扩展 SafeGo 覆盖"和"扩展健康监控覆盖"两件事没有同步完成。

（继承自 #115 的 SafeGo 本身缺陷：堆栈记录位置错误、backoff 不重置——不再重复展开，建议与 #115 一并修复）

🎯 结论：🔄 Request Changes — ddns 心跳 key 硬编码是会直接导致"故障被隐藏"的实质性 bug，建议先修复再合并