fix(service): re-register missing daemon services - #245
Conversation
Code review summaryVerdict: PASS. No unresolved Critical, High, or Medium findings. The review covered service ownership, lifecycle recovery, privilege boundaries, open-source packaging compatibility, and Windows parity. Two High findings found during review were fixed before publication:
Validation passed: Shell syntax, 16/16 service re-registration tests, 14/14 updater watchdog tests, typecheck, build, and diff check. Residual Low risks are limited to existing cross-edition updater payload residue and the absence of live Windows/init-manager E2E in this local environment. |
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Fix daemon service re-registration when service definitions are missing or fail to start, covering launchd, systemd user/system, init.d, and Windows Task Scheduler. Includes comprehensive test coverage for all re-registration paths.
Highlights
- Error propagation: All critical installer commands now use
|| return 1, ensuring failures are caught instead of silently continuing - Liveness verification: After service start, process liveness is verified before reporting success
- Independent re-registration: Collector and updater are re-registered independently, preventing one from blocking the other
- Non-interactive sudo scoping:
_PILOT_SUDO_NONINTERACTIVEflag keeps self-heal elevation non-interactive without affecting normal install behavior - Updater guard: Checks for
updater-daemon.jsexistence before attempting updater service install (important for open-source packages that omit the updater)
LGTM — solid fix with excellent test coverage.
Automated review by github-manager-bot
| fi | ||
| # A concrete manager now owns this daemon. Even if its first liveness | ||
| # check is slow, starting a nohup copy could create duplicate collectors. | ||
| if [ "$_restarted" = false ] && [ "$_new_init" != "none" ]; then |
There was a problem hiding this comment.
[High] :716 守卫条件 [ "$_restarted" = false ] && [ "$_new_init" != "none" ] 未区分「注册成功但进程未起来」与「注册失败」，而注释声称的前提是 A concrete manager now owns this daemon —— 注册失败时该前提不成立，却同样 exit 1。
影响: collector 在 :630/:632-645 已被 pkill + kill PID_FILE 杀掉；注册失败后 :720-742 的 nohup 兜底变成不可达代码，collector 保持死亡。相对 base 是行为回归 —— base/head 对照实验：base ⚠️ collector restarted (nohup fallback, self-heal failed) exit=0 进程存活；head ❌ Service manager failed to restart collector (init_type=systemd-user) exit=1 进程不存活。
两个放大因素：
exit 1发生在:751-759的setsid ... restart-updater调度之前 → updater 也不会重启，出现「版本指针=新、进程=旧」的不一致。- 调用方
src/updater/updater.ts:1043只logger.warn不重抛，:284/:290仍上报collector_restarted并把consecutiveFailures归零；而src/metrics/metrics-writer.ts:265只在init_type ∈ {nohup, unknown}时告警 → 无告警、无重试，采集停止无人知晓。
:864 的 updater 侧同构。测试 :318-349 的 it.each 只覆盖「注册成功」这一支，本场景零覆盖。
建议: 用「是否真的注册成功」而非「detect 出了具体管理器」作为守卫依据：
_registered=false
if _PILOT_SUDO_NONINTERACTIVE=true autostart_install_collector_only "false" 2>>"$LOG_FILE"; then
_registered=true
sleep 1
...
fi
if [ "$_restarted" = false ] && [ "$_registered" = true ]; then
echo "❌ Service manager failed to restart collector (registered as $_new_init)" >&2
exit 1
fi也可复用已有的接管探测（is_managed_by_launchd / is_managed_by_systemd_user / is_managed_by_systemd_system / is_managed_by_initd，:321-340）作为「确实被接管」的判据。
Generated by LoongSuite-Pilot Code Review Agent
| _new_init=$(detect_init_system "false") | ||
| if [ "$_new_init" != "none" ]; then | ||
| if _PILOT_SUDO_NONINTERACTIVE=true autostart_install_collector_only "false" 2>>"$LOG_FILE"; then | ||
| sleep 1 |
There was a problem hiding this comment.
[Medium] 自愈后只等 sleep 1 就用 is_running 判活，但 is_running（:120-130）只读 PID 文件、无 pgrep 兜底，而 cmd_run（:387-404）把 PID 文件写在最后一步（:402）—— 之前还要跑 resolve_node，后者对每个候选执行 "$bin" --version（_node_is_suitable:186）。
影响: 本机实测 6 个候选探测约 3.3s，远超 1s 窗口。「1s 后没起来」是常态而非异常，健康安装会被误判并推入破坏性自愈路径，叠加 :716 守卫后直接 exit 1。测试里 sleep 被 mock，CI 永远看不到这个时序。
建议: 改为有界轮询，或让 is_running 在 PID 文件缺失时回退 pgrep：
for _i in $(seq 1 15); do
is_running && break
sleep 1
doneWindows 侧已有现成范式可抄：Wait-ForCollectorHeartbeat -TimeoutSeconds 30（scripts/loongsuite-pilot.ps1:323）。
Generated by LoongSuite-Pilot Code Review Agent
|
|
||
| case "$init_system" in | ||
| launchd) | ||
| launchctl unload -w "$LAUNCHD_PLIST" 2>/dev/null || true |
There was a problem hiding this comment.
[Medium] 自愈块被提到 case "$init_type" 之外后，launchd 分支对健康安装也会执行 launchctl unload -w，而 -w 在 macOS 上写入的是持久化 Disabled override（跨重启/重新登录生效）。
影响: 常见路径是二次 kill + plist churn（KeepAlive 通常能恢复）；尾部风险是 unload -w 成功、_write_launchd_plist 或 load -w 失败 → agent 既未加载、又被持久禁用，重新登录也不会回来。base 对具体 init-type 从不触碰 plist，这是本 PR 新引入的可达路径。
建议: 先用 launchctl print gui/$(id -u)/<label> 或已有的 is_managed_by_launchd 判断服务定义是否真的缺失，缺失才重写；若必须重载，用不带 -w 的 unload/load（或 bootout/bootstrap），并在 load 失败时显式 launchctl enable 回滚 Disabled 状态。
Generated by LoongSuite-Pilot Code Review Agent
| initd) | ||
| _write_initd_script "$target_user" | ||
| _write_initd_script "$target_user" || return 1 | ||
| _register_initd_boot "loongsuite-pilot-${target_user}" |
There was a problem hiding this comment.
[Medium] initd 分支把「注册成功」写进了 init-type，但两个关键步骤都不校验：_register_initd_boot（:1592-1601）在 chkconfig / update-rc.d 都不存在时只 echo 一句警告，恒返回 0；:1643 的 /etc/init.d/... start 又是 || true。随后 :1644 照样写入 init-type=initd 并返回 0。
影响: 记录了一个「既没启动本 daemon、也没注册开机自启」的管理器归属。这个假的具体 init-type 会被 detect_init_system:272-285 的白名单无条件短路信任，于是每次 restart 都命中 :716 的硬失败守卫，形成永不回退 nohup 的稳定失败吸引子。
对比：同函数的 systemd 分支顺序是正确的（echo > INIT_TYPE_FILE 在 enable --now 之后），initd 分支没有对齐。
建议: _register_initd_boot 在两个工具都缺失时 return 1；:1643 的 start 改为 || return 1；并把 echo "initd" > "$INIT_TYPE_FILE" 移到两者都成功之后。:1686-1690 的 updater 侧同构。
Generated by LoongSuite-Pilot Code Review Agent
| maybe_sudo systemctl daemon-reload &>/dev/null | ||
| maybe_sudo systemctl enable --now "loongsuite-pilot-${target_user}.service" &>/dev/null | ||
| echo "systemd-system" > "$INIT_TYPE_FILE" | ||
| _write_systemd_system_unit "$target_user" || return 1 |
There was a problem hiding this comment.
[Medium] 自愈路径（:704/:852）新引入了无人值守的提权写入：在 NOPASSWD 主机上，一次失败的 restart 最多触发 5 次 sudo -n（mkdir -p /etc/systemd/system、tee unit、daemon-reload、enable --now），且失败后没有任何回滚。
影响: base 在具体 init-type 的 restart 路径上零提权写入，这是新增的写入面（不构成提权，User=<target_user> 未变）。部分失败会在 /etc/systemd/system/ 留下 root 所有、未 enable 的残留 unit，而 init-type 未变 → 后续每次 restart 都重写一遍；updater-watchdog 约每 ≥10 分钟重试，失败的 sudo -n 持续堆积 authpriv 日志。管理员手工改过的 unit 也会被静默覆盖。
建议: 写入前先判断服务定义是否真的缺失（test -f unit + systemctl cat），存在则不重写；tee 写 unit 改为「临时文件 + mv」保证原子性，任一步失败清理残留；给自愈加最小重试间隔（在 $DATA_DIR 记录上次自愈时间戳，10 分钟内不重复重写）。
Generated by LoongSuite-Pilot Code Review Agent
| Write-Host "collector self-healed: registered with Task Scheduler" | ||
| $restarted = $true | ||
| } | ||
| try { |
There was a problem hiding this comment.
[Medium] Windows 侧自愈同样不再受 $initType 限制，于是健康的 taskscheduler 安装在一次 1s liveness 误判后，就会经历 Stop-OrphanProcesses + schtasks /Delete（Install-CollectorTask 内部 :407 还会再删一次）+ 重新 Register-PilotTask。
影响: Register-PilotTask 先试 Interactive 再试 S4U，在 GPO 限制「登录为批处理作业」或 Access Denied 的机器上两者都会失败；此时任务已被删除、重建失败 → 机器上一个任务都不剩，采集彻底停止且不再开机自启。base 的 $initType 门禁使这条路径不可达。:901-914 的 updater 侧同构。
建议: 重注册前先 Get-ScheduledTask -TaskName <name> -EA SilentlyContinue 判断定义是否真的缺失；确需重建时先 Export-ScheduledTask 备份 XML，Register-PilotTask 全部失败时用备份回滚，并把失败原因写入日志与告警。
Generated by LoongSuite-Pilot Code Review Agent
| Write-Host "Self-heal failed: $($_.Exception.Message)" -ForegroundColor Yellow | ||
| } | ||
| if (-not $restarted) { | ||
| if ($initType -in @("background", "unknown", "")) { |
There was a problem hiding this comment.
[Medium] Windows 侧缺少 POSIX :716 的对偶守卫。当 $initType ∈ {background, unknown, ""} 且自愈的 Install + Start 都成功、只是 Get-TaskRunning 在 1s 内为 false 时，控制流会继续落到 Start-Process 兜底。
影响: 已注册并已启动的 Scheduled Task 之外再拉起第二个 collector，形成重复采集 —— MultipleInstances IgnoreNew 只约束 task 实例、不约束 Start-Process 拉起的副本。更糟的是 Set-Content init-type "taskscheduler"（:811）只在 liveness 为真的分支执行，init-type 保持旧值 → 每次 restart 都会重演。这正是 :504-509 注释所描述的事故类型；而测试 :408 的 expect(body.slice(fallbackGuard)).toContain('Start-Process') 反而把这个兜底钉死成了预期行为。
建议: 与 POSIX 对齐：自愈注册成功后不再走 Start-Process，改用已存在的 Wait-ForCollectorHeartbeat（:323）等待，超时则报错退出；并把 Set-Content init-type 移到注册成功后无条件执行，避免状态不收敛。同步修正测试断言，使其钉「注册成功后不 fallback」。
Generated by LoongSuite-Pilot Code Review Agent
| expect(result.stderr).toContain('Service manager failed to restart updater (init_type=initd)'); | ||
| }); | ||
|
|
||
| it.each([ |
There was a problem hiding this comment.
[Medium] 新增的 16 个用例无法钉住本 PR 的核心行为。变异测试（node 22，16/16 基线全绿）证明：① 把 sh:703 的自愈门禁回滚成「仅 legacy init-type 才自愈」→ 16/16 仍全绿；② 把 sh:666/:813 的 start 退出码检查回滚成 || true → 16/16 仍全绿；③ 删掉 systemd-user 分支的 || return 1 → 16/16 仍全绿。
影响: 核心修复可被静默回滚而 CI 全绿。本 it.each 只覆盖「注册成功」这一支，正是 sh:716 回归的漏网场景；uname 被 mock 成 Linux，launchd 与 initd 分支从未执行；Windows 段（:370-411）全是 indexOf/正则的源码文本断言，不执行任何 PowerShell。另外 :189-195 的 id() mock 在 String.raw 下 "\${1:-}" 是字面量，永不等于 -u，该 mock 从未生效。
建议: 补三类真实执行用例：
- 自愈注册失败（让
autostart_install_collector_only返回 1）时断言仍走 nohup 兜底且进程存活； - 注册成功但 liveness 延迟时断言进程数恒为 1；
- 参数化
uname覆盖Darwin/ initd 分支。
Windows 侧改为 pwsh -NoProfile -Command 注入 schtasks / Get-ScheduledTask stub 做行为断言，替换文本断言。
建议把变异测试作为验收门禁：上述三个回滚中任意一个都必须导致用例失败。
Generated by LoongSuite-Pilot Code Review Agent
🔍 Code Review SummaryReviewed at
Lifecycle Verdict
资源释放 FAIL
状态恢复正确性 FAIL
Merge Gate（合入门禁）BLOCK ⛔ —— 阻断合入，待修复后重评（阻断级别： 未解决的阻断项：
细节见对应行的 inline 评论。 总体结论修复方向正确：把「本机用哪个服务管理器」与「本 daemon 的服务定义是否存在」解耦，是对根因的准确修复；顺带修掉了「start 失败误报成功」「注册失败仍固化 init-type」两个真实缺陷； 阻断原因集中在为防重复进程而新增的守卫上，两侧问题正好相反：
CI 全绿不构成反证：变异测试显示，回滚本 PR 的自愈门禁（
另有两处不在本 PR diff 范围内、但已成为承重结构的问题，建议同 PR 或紧随其后处理：
Low（不阻断，建议随手修）
Highlights（正向实践）
评审报告详见: |
🔍 CLM-Focused Re-Review (Windows Constrained Language Mode)Reviewed at Language-Level CLM Safety: PASS ✅All PS1 constructs in the diff are CLM-compatible:
Static CLM test: 33/33 passed ( WDAC Environmental Concern: Destructive Re-Registration [Medium]Not a CLM violation, but a behavioral risk specific to WDAC-enforced machines. The PR removes the
Result on WDAC+GPO machine: Working Compare with Suggested fix: Pre-check before destructive re-registration: if (-not (Get-TaskExists $TASK_NAME_COLLECTOR)) {
$ok = Install-CollectorTask $nodeBin
# ... existing logic
}Only re-register when the task is genuinely absent. This preserves the PR's intent (repair missing task definitions) while avoiding destructive churn on healthy WDAC installs where the liveness check was just slow. This reinforces Finding #6 from the previous review round — the CLM/WDAC angle makes the "delete then fail to re-register" scenario more probable because GPO restrictions and WDAC policy verification overhead are co-located. Minor:
|
3 participants
What changed
updater-daemon.jsfrom creating an empty updater service.nohupprocess.Why
Collector and updater share one
init-typestate file but have separate service definitions. After collector self-heal persisted a concrete manager, updater restart could incorrectly assume its own service was registered, fail to start, and emitpid file is missing; no matching process found.Impact
Restart self-heal now repairs only the missing daemon service and reports success only after the process is observable. Open-source installs continue to omit the updater service when the updater payload is not shipped.
Validation
Validation boundaries
.env.e2eis absent.