[bugfix] updater watchdog restart grace and stale PID false alarms#135
[bugfix] updater watchdog restart grace and stale PID false alarms#135Monsooooon wants to merge 1 commit into
Conversation
| }); | ||
| return { status: 'healthy', reason: processState.reason }; | ||
| } | ||
| this.recordServiceAlarm(processState.reason); |
There was a problem hiding this comment.
[Medium] missing-process 路径在调用 restart() 之前无条件 recordServiceAlarm()，冷却期内仍每 tick 记 SERVICE_NOT_RUNNING_ALARM。
影响: Issue 678505 主告警（UPDATER_FAILURE）噪声已压住，但同场景的 service 告警仍可能刷屏。
建议: 与 failureAlarmMessage 对称——仅在真正执行重启时记 service alarm，或在 cooldown / postRestartGraceUntil 内跳过 recordServiceAlarm。
| return this.restart('missing-heartbeat', reason, reason); | ||
| } | ||
|
|
||
| if (heartbeat.pid !== processState.pid && process.platform !== 'win32') { |
There was a problem hiding this comment.
[Medium] pid-mismatch 且 heartbeatProcessLooksHealthy 为真时直接 healthy 返回；若 pid 文件进程本身也是 updater（commandOk），可能留下双 updater，且不 reconcile pid 文件。
影响: 看门狗静默；metrics-collector 仍只看 pid 文件（updaterPidAlive），观测与恢复语义分裂。
建议: heartbeat 胜出时写回/调用轻量 reconcile（或仅在 pid 文件进程 非 updater 时信任 heartbeat）；双 updater 时仍应重启收敛。
| return this.isUpdaterCommand(command); | ||
| } | ||
|
|
||
| private commandForAlarm(command: string | undefined): string { |
There was a problem hiding this comment.
[Medium] commandForAlarm 脱敏仅匹配 token|key|secret|password|... + =/空白，漏掉常见 --accessKeySecret=、Bearer … 等形态。
影响: command mismatch 告警上送 SLS 时可能泄露凭证片段。
建议: 扩展规则或复用 src/mask 能力；至少覆盖 --*secret* / --*key* / Bearer。
🔍 Code Review Summary
总体结论发现 5 个非阻断问题（3 Medium + 2 Low），可合入但建议后续改进。 评审报告详见: |
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
LGTM. Solid bugfix for updater watchdog false alarms.
Summary
PR adds a post-restart grace window, defers recordFailureAlarm to restart() (only fires when restart actually proceeds, not during cooldown), and trusts a fresh heartbeat process as healthy when the PID file is stale — addressing real operational noise from the watchdog.
Review Notes
- Grace window logic (
postRestartGraceUntil+inGraceWindow) is clean and correctly cleared on healthy confirmation. heartbeatProcessLooksHealthy()thoroughly checks freshness + PID liveness + command match before trusting heartbeat over PID file.commandForAlarm()adds basic credential redaction to alarm messages — a nice security improvement (the existing reviewer noted the regex could be extended forBearerand other patterns; agree as a follow-up).- Test coverage is good: post-restart grace, no-repeat-alarms-during-cooldown, and fresh-heartbeat-healthy-when-pid-stale all covered.
- The 3 Medium issues flagged by @linrunqi08 (service alarm not deferred in missing-process path, no PID file reconciliation on heartbeat trust, redaction gaps) are valid improvement suggestions but non-blocking for this merge.
All CI checks passing (Node 18/20/22 + CodeQL). CLA signed. Mergeable: clean.
Automated review by github-manager-bot
|
📋 Ready to merge — approved 120h ago, CI passing, no merge conflicts. This PR has been approved and all checks pass, but hasn't been merged yet. Pinging maintainers for follow-up. Automated notification by github-manager-bot |
3 participants
Description:
Summary
UPDATER_FAILURE_ALARMrecords while restart attempts are rate-limited.Testing
npm test -- tests/unit/core/updater-watchdog.test.tsnpm run typecheck