Skip to content

fix(core): stop misclassifying detection-probe failures as input stops - #242

Open
Snssn wants to merge 2 commits into
alibaba:mainfrom
Snssn:fix/input-stop-alarm-misclassification
Open

fix(core): stop misclassifying detection-probe failures as input stops#242
Snssn wants to merge 2 commits into
alibaba:mainfrom
Snssn:fix/input-stop-alarm-misclassification

Conversation

@Snssn

@Snssn Snssn commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

AgentDiscoveryService.processEntry() wrapped its whole body in a single try. Any exception from a running entry — including from the detection probe (enabled() / isAvailable()) — was treated as an input stop: the entry was forced to idle and an 'unexpected' stop event was emitted without ever calling entry.stop(). That is a semantic error (a broken probe says nothing about whether the input can still collect) and it leaves discovery state out of sync with reality.

This PR fixes the classification and makes the surviving real alarms diagnosable.

Scope note (updated after review): this is a correctness/hardening fix, not a fix for the volume of INPUT_STOP_ALARM seen in the field. That volume comes from pre-#198 *-agentshell builds and clears by rebuilding and rolling out the artifact — no code change involved. The path corrected here is rare (internal triage: ~3 agents/week emitting the post-#198 (reason=unexpected) format, versus ~134k lines/week of the legacy format).

What changed

processEntry() is split into two phases:

  • Probe phase (enabled / isAvailable) — on exception the entry is left completely untouched: a running entry stays running and keeps collecting, nothing is emitted, and no alarm is raised. It only increments an observability counter (consecutiveProbeErrors) and logs a warning.
  • Act phase (the entry's own start()) — only failures here can mean the input is unhealthy. On a running entry these must both repeat (ERROR_THRESHOLD = 3) and span real time (ERROR_MIN_WINDOW_MS = 60s) before the entry is stopped through the normal stopEntry() path (which calls entry.stop() first, so state and reality agree) and classified unexpected. Because the probe is healthy on this path, the next successful poll restarts the entry — it self-heals rather than going permanently silent.

The time window is required because ERROR_THRESHOLD is a pure count while processEntry() is driven by three unsynchronized sources: the global poll timer (300s), a per-entry timer (user-configurable, 30s by default per config-loader.ts), and unthrottled fs.watch callbacks. Without a window, a sub-second glitch could exhaust three attempts and trip an L3 alarm.

Failures while starting/idle keep the existing behavior: reset to idle and let the next poll retry, emitting nothing.

unexpected stops now carry a sanitized error summary — whitespace collapsed, $HOME masked to ~, truncated to 200 chars — which the orchestrator appends to the alarm message. The (reason=unexpected) suffix is preserved so the existing new-vs-legacy format triage query keeps working.

Test plan

  • Probe keeps throwing for 10 rounds across 20 simulated minutes → entry stays running, entry.stop() never called, zero alarms (guards against stopping the data plane on probe failure).
  • 20 back-to-back lifecycle failures at the same instant (simulating an fs.watch burst) → no stop, no alarm.
  • Sustained lifecycle failures reaching both count and window → entry.stop() called, unexpected emitted with summary.
  • Counter and window both reset after a successful poll; entry self-heals back to running after an unexpected stop.
  • First-start failure → idle, nothing emitted. Normal stops (unavailable/disabled/shutdown) carry no summary.
  • summarizeError folding / truncation / home-dir masking; orchestrator appends summary only when present and only for unexpected.
  • npx vitest run tests/unit/core tests/unit/metrics → 393 passed. tsc --noEmit clean for the changed files.

Follow-ups (deliberately out of scope)

  • Exposing probe_consecutive_errors in the L2 input metrics (touches the metrics schema and server-side fields).
  • Applying the same count+window treatment to the pre-existing consecutiveUnavailable counter, which has the same time blind spot but does not feed an L3 alarm directly.

The processEntry() catch branch treated any exception from a running
entry's lifecycle calls (enabled/isAvailable/runOnActive start) as a
stop: it forced the entry to idle and emitted an 'unexpected' stop
event without ever calling entry.stop(). This both desynced state
(the input was still running, so the next poll re-started it) and
raised a spurious L3 INPUT_STOP_ALARM for a stop that never happened —
the only remaining path that can fire the alarm after PR alibaba#198, and a
false positive at that.

Now a lifecycle exception on a running entry is treated as a probe
failure: the entry keeps its state and no event is emitted until
ERROR_THRESHOLD (3) consecutive failures, after which it is stopped via
the normal stopEntry() path and classified as 'unexpected'. Failures
while starting/idle still reset to idle without emitting.

The 'unexpected' stop now carries a sanitized error summary (whitespace
collapsed, home dir masked to ~, truncated to 200 chars) that the
orchestrator appends to the alarm message, so surviving real alarms are
diagnosable. The (reason=unexpected) suffix is preserved.
@Snssn Snssn changed the title fix(core): distinguish transient probe failures from real input stops fix(alarm): distinguish transient probe failures from real input stops Aug 11, 2026

@ralf0131 ralf0131 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean fix for the false-positive INPUT_STOP_ALARM caused by transient probe failures being misclassified as unexpected input stops.

Key changes reviewed:

  • Transient failure tolerance: Running entries now survive ERROR_THRESHOLD (3) consecutive lifecycle exceptions before being classified as unexpected. At the default 5-min poll interval, this gives ~15 min of tolerance — well-calibrated to distinguish probe hiccups from real stops.

  • Proper lifecycle: The catch block no longer force-resets state to idle for running entries. Instead, it increments consecutiveErrors and returns silently. Only when the threshold is reached does it call stopEntry() (which properly calls entry.stop() first) — fixing the state desync and redundant start() issue.

  • Error summary for diagnosability: summarizeError() produces a sanitized, length-bounded (200 chars), home-dir-masked single-line summary. The orchestrator appends it to the alarm message only for unexpected stops. This is a nice addition — surviving real alarms are now diagnosable without leaking absolute paths.

  • Counter discipline: consecutiveErrors is reset on every success path (running check passes, starting succeeds, disabled/unavailable paths). No risk of stale counters triggering false alarms after recovery.

  • Backward compatible: stopEntry() signature adds optional errorSummary? parameter. Normal stops carry no summary. Orchestrator alarm message only appends summary when present.

  • Test coverage: 7 new tests covering single/reentry exception retention, threshold-triggered stop, counter reset on recovery, first-start failure, no double-start, normal stops without summary, and summarizeError folding/truncation/home-masking.

LGTM — well-targeted fix with good test coverage.


Automated review by github-manager-bot

// entry.stop() first), classifying it as 'unexpected'. Below the
// threshold a running entry keeps its state — we never emit a stop event
// for an input that is still running, and never leave state out of sync.
if (rt.state === 'running') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] 被修复的 rt.state === 'running' catch 分支在当前生产代码中几乎不可达,PR 声称的根因缺少证据支撑。

影响: 该分支只能由四类抛错进入,逐一核查后均不成立:

  1. entry.enabled()(:118)—— 生产实现全为纯取值:orchestrator.ts:1383-1387 isAgentGatedEnabled 仅做对象取值,agent-control-manager.ts:44-53 resolveEnabled/getMode 仅做 map 取值。
  2. await entry.isAvailable()(:119)—— 30+ 个生产入口全部 throw-safe:directoryExists/fileExistsutils/fs-utils.ts:9-28try/catch → false);抽查 5 个 checkAvailability(wukong 含 execFile、workbuddy、kiro-cli-session、qoder-sqlite、codex-transcript)全为 try { … } catch { return false };deploy-detection 走 detectAgentdeployment/detect-utils.ts:7-30)亦全程吞异常。
  3. runOnActive 重入的 entry.start()(:157-160)—— 无任何生产设值点,全仓仅 types/index.ts:245 定义 + 本文件 :157 读取。
  4. 残余唯一路径:this.emit('agent:started')(:134,此时 state 已置 running)的监听器抛错。唯一监听器是 orchestrator.ts:232-234logger.info,而 utils/logger.ts:93-112 无 try/catch —— 但这需要日志流本身已损坏,属极端场景。

由于 base 版本 wasRunning === true 的误报路径与上述完全同源,若该分支不可达,则线上残留的 INPUT_STOP_ALARM 另有根因,本 PR 无法消除它,团队会误判问题已解决。

建议: 请补充线上证据后再合入:附上告警发生时同一进程的 processEntry failed / agent stopped unexpectedly 日志行(含实际 error 文本或栈),以确认告警确实来自该 catch 分支。若确认不可达,建议把本 PR 重新定位为「防御性加固」而非该 Issue 的修复,并另行定位真实根因。


Generated by LoongSuite-Pilot Code Review Agent

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

部分成立:我接受重新定位为「防御性加固 + 正确性修复」,但「不可达」这个前提与线上数据不符。

先说我核实后同意你的部分——这四条我逐一查了,结论与你一致:

  1. enabled() 生产实现均为纯取值。
  2. isAvailable() 确实全线 throw-safe:directoryExists/fileExistsfs-utils.ts:9-28)、抽查的各 checkAvailability、以及 detectAgent 链路(detect-utils.tswalkreaddir/stattry/catch → falsecommandExists 走 callback resolve(!err))。
  3. runOnActive 全仓无生产设值点(仅 types/index.ts:245 定义 + 本文件读取)——确认。
  4. 残余路径是 emit('agent:started') 监听器抛错。

所以这条分支极其罕见,我原描述里「the only remaining trigger」的措辞把「唯一」说成了「常见」,这个批评我接受。

但「不可达」可以被线上数据证伪。 关键点:base 代码中 'unexpected' 只在这一处 catch 分支 emit(stopEntry() 只被以 shutdown/disabled/unavailable 调用),因此告警文本里带 (reason=unexpected) 后缀 只能由该分支产生。内部巡检在 7 天窗口内观察到常规渠道(含 #198 的二进制)有 3 台机器 / 3 行新格式告警input deploy:codex stopped unexpectedly (reason=unexpected)),对照同期不含修复的存量产物 13.4 万行旧格式。也就是说:该分支被执行过,量级约 3 台/周。

我无法满足的部分:我确实拿不到那 3 台机器的 processEntry failed 日志行与 error 文本(告警侧不带异常详情,这正是本 PR 加异常摘要要解决的问题之一),所以无法证明它们具体由哪一条子路径触发。这是个真实的证据缺口,我不装作有。

因此按你的建议调整定位(PR 描述已同步更新):

  • 本 PR 声称消除线上残留的 INPUT_STOP_ALARM 体量。13.4 万行的主因是修复前的 *-agentshell 存量产物,出清靠重打产物 + 推送升级,与代码无关。
  • 本 PR 的价值收敛为三点:(a) 修正一条语义错误的分类路径(把探测异常当成输入停止);(b) 消除该路径带来的状态失同步;(c) 给残留的真实 unexpected 告警补上异常摘要,使下一次发生时可从告警侧直接定位——正好填上上面那个证据缺口。
  • 结合另一条评论的修复,探测异常现在根本不再告警。若那 3 台源于探测抛错,新版本下它们将安静消失;若源于生命周期失败,则会带着异常摘要告警。两种情况都比现状可诊断。

如果你认为在拿到线上日志前不宜合入,我没有异议——可以标 hold,等下一轮巡检拿到带摘要的新格式告警再推进。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fixed] Round 2 复核确认已修复。

证据: PR 定位已调整为「防御性加固 + 正确性修复」(scope note 更新)。commit 6d1bfc4 将 processEntry 拆为两阶段后,探测异常在 Phase 1 catch 中直接 return,完全不触发告警、不改变 entry 状态、不调用 entry.stop()。


Generated by LoongSuite-Pilot Code Review Agent

Comment thread src/core/agent-discovery-service.ts Outdated

const DEFAULT_POLL_MS = 300_000; // 5 minutes
// Consecutive processEntry() exceptions on a running entry before we treat it
// as a real (unexpected) stop. At the default poll interval this is ~15 minutes

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] ERROR_THRESHOLD 是纯次数阈值,注释宣称的「at the default poll interval this is ~15 minutes of sustained failure」不成立,最快可在毫秒级耗尽。

影响: processEntry() 有三个互不同步的触发源,其中两个远快于 5 分钟:

  • globalPollTimer(:75)默认 300s —— 只有这一条符合注释。
  • per-entry pollTimer(:244)实际默认 30sorchestrator.ts:1108 传入 listenerCfg['claude-code-log']?.pollInterval,而 config-loader.ts:377 该项默认 30_000(:362-382 的列表项全为 30_000)。3 次 ≈ 90s,不是 15 分钟。
  • fs.watch 回调(:225)void this.processEntry(rt) 无防抖、无节流。实测:对 watchPath 目录做 10 次连续写入,触发 4 次回调,间隔 <1ms。

因此一次几百毫秒的瞬时故障(日志轮转窗口的 ENOENT、EBUSY、EMFILE)即可连吃 3 次异常,立即 stopEntry('unexpected') 并打 L3 告警 —— 正是本 PR 要消除的误报形态。注释还会误导后续维护者按「15 分钟」理解语义。

建议: 改为「次数 + 时间窗」双条件,并按实际机制改写注释(不要写死分钟数,pollIntervalMs 用户可配)。仓内先例 updater/updater-metrics.ts:207-208 同时使用次数阈值与冷却窗口:

// EntryRuntime 增加 firstErrorAt: number | null
rt.consecutiveErrors++;
rt.firstErrorAt ??= Date.now();
if (rt.consecutiveErrors >= ERROR_THRESHOLD && Date.now() - rt.firstErrorAt >= MIN_ERROR_WINDOW_MS) {
  // …stopEntry
}

并在所有重置 consecutiveErrors 的分支同步重置 firstErrorAt

公平性备注:既有 consecutiveUnavailable(PR #198)有同样的时间盲区(真实使用者 wukong,orchestrator.ts:1331 unavailableThreshold: 3 + watchPaths),本 PR 是沿用既有形态而非独创;但新计数器直通 L3 告警,影响面更大。


Generated by LoongSuite-Pilot Code Review Agent

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复(6d1bfc4b)——注释确实是错的,两处都核实了。

  • setupPolling()(:244)用 rt.entry.pollIntervalMs || DEFAULT_POLL_MS,而 orchestrator.ts 各 input 条目传的是 listenerCfg[...]?.pollIntervalconfig-loader.ts:361-382 该表全部默认 30_000。所以 input 条目是 30s 级,3 次 ≈ 90s,不是 15 分钟。(只有 deploy-detection 条目是 orchestrator.ts:409 硬编码的 300_000。)
  • fs.watch 回调(:225)void this.processEntry(rt) 确认无防抖无节流,突发写入可在毫秒级连续触发。

改动:

  1. 按你给的形态加了时间窗,EntryRuntime 增加 firstErrorAt: number | null,并抽出 resetErrorCounter() 统一重置计数与时间戳(避免漏重置其中之一):
rt.consecutiveErrors++;
rt.firstErrorAt ??= now;
if (rt.consecutiveErrors >= ERROR_THRESHOLD && now - rt.firstErrorAt >= ERROR_MIN_WINDOW_MS) {  }

ERROR_MIN_WINDOW_MS = 60_000

  1. 注释重写为按机制描述,不再写死分钟数——明确点出三个触发源(全局定时器 / 用户可配的 per-entry 定时器 / 无节流的 fs.watch)以及「计数可在毫秒级耗尽,故需叠加时间窗」。

新增测试:连续 20 次瞬时失败(同一时刻,模拟 fs.watch 突发)→ 不停止不告警;只有在计数达标跨过 60s 后才停止。

关于公平性备注:同意 consecutiveUnavailable 有同样的时间盲区。它不直通 L3 告警,我没在这个 PR 里一并改,避免把 wukong 的 unavailableThreshold: 3 行为一起动掉;如果需要对齐可以另开一个 PR。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fixed] Round 2 复核确认已修复。

证据: commit 6d1bfc4 新增 ERROR_MIN_WINDOW_MS=60000 和 firstErrorAt 时间戳,实现 count + time window 双条件。注释重写为按机制描述。新增 burst 测试覆盖 20 次瞬时失败不触发停止。完全按评审建议实现。


Generated by LoongSuite-Pilot Code Review Agent

error: String(err),
});
rt.consecutiveErrors = 0;
await this.stopEntry(rt, 'unexpected', summarizeError(err));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] 达阈值后主动调用 entry.stop(),把「探测函数的健康度」当作「输入的健康度」,构成数据面行为回归。

影响: base 的 catch 只写 rt.state = 'idle',从不调 stop();其后所谓「对仍在运行的 input 冗余 start()实为空操作 —— inputs/base/base-input.ts:41 if (this._running) return;。也就是说 base 在探测抛错时从不中断采集。head 在 :182 走 stopEntry → :204 await rt.entry.stop()input-manager.stopInputBaseInput.stop()base-input.ts:51-61)真正清 timer、跑 onStop(),采集实际停止。

而探测函数与输入自身健康无关(探测只看目录是否存在 / CLI 是否 running)。一旦探测持续抛错,输入被停掉,之后每轮都落在 catch 的 else 分支(state 非 running → 置 idle、不再告警),采集永久静默且只有一条告警;同场景下 base 会继续采集。

建议: 区分故障来源再决定是否动数据面 —— 仅当失败来自输入自身生命周期调用(start/stop)时才停;探测(enabled/isAvailable)连续失败应上报可观测信号(degraded 指标或独立告警类型)而不停采集。至少把「计数阈值」与「是否 stop input」解耦,让探测故障只影响告警、不影响数据面。

补充两项可观测性缺口(同源,建议一并处理):

  1. 亚健康期不可见getStates()(:106-112)不暴露 consecutiveErrors,L2 指标(metrics/metrics-collector.ts:322-341succeed_events / failed_events / input_idle_minutes)也没有对应字段。服务端完全看不到「探测持续失败但仍记为 running」这一状态。建议新增 probe_consecutive_errors 到 L2 input 指标。
  2. 持久故障只报一次:停止后 consecutiveErrors = 0 且 state 转 idle,此后每轮落入 :191-197 的 else 分支(不 emit、不告警),而 AlarmManager.serialize() 每 30s 清空(alarm-manager.ts:97metrics-writer.ts:88),因此永久坏死的 input 在服务端只留历史上那一条告警。仓内已有现成范式:metrics-writer.ts:274-287 recordInfraAlarm() 用 cooldown 窗口重新武装告警,注释明确 "re-arm them after a cooldown window instead of using a once-guard",触发条件同为连续失败计数(:295 updaterConsecutiveFailures >= 2)。建议 INPUT_STOP_ALARM 复用该模式。

Generated by LoongSuite-Pilot Code Review Agent

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复(6d1bfc4b)——这条完全成立,是本轮最重要的问题。

核查确认了你的两个关键前提:

  1. BaseInput.start()base-input.ts:41if (this._running) return; —— 我原先声称修复的「对仍在运行的 input 冗余 start()确实是空操作,base 在探测抛错时从不中断采集。
  2. head 的 stopEntryentry.stop()base-input.ts:51-61)真正 clearInterval + onStop(),采集实际停止。

更糟的是你指出的后半段:停掉之后 state 转 idle,而探测仍在抛错 → 每轮都落在 catch 的 else 分支(置 idle、不 emit),既永不重启也永不再告警。等于我把 base 的「无害误报、采集继续」换成了「采集永久静默 + 只有一条告警」,这是严格更差的失效模式。

按你的建议做了来源区分,processEntry() 拆成两段:

  • Phase 1 探测enabled / isAvailable):异常时完全不动条目(running 保持 running)、不 emit、不告警,仅 consecutiveProbeErrors++logger.warn,然后 return
  • Phase 2 执行(条目自身 start()):异常才可能判 unexpected。此路径下探测是健康的,因此停止后下一轮轮询会走完整 idle → running 重启,自愈而非永久静默

新增两个回归测试守住这条语义:探测连续抛错 10 轮(跨 20 分钟)→ 始终 runningentry.stop() 从未被调用、零告警;以及停止后自愈重启。

顺带修了一个我自己的逻辑 bug:runOnActive 分支原先在 await entry.start() 之前重置计数器,导致反复失败的 start 永远累积不到阈值。现改为仅在调用成功后重置。

两项可观测性建议的处理:

  • 亚健康期可见性:本地已有每轮 logger.warn(含 consecutiveProbeErrors 与错误文本)。probe_consecutive_errors 进 L2 input 指标涉及 metrics-collector schema 与服务端字段,我倾向不塞进这个 PR,建议单独提。若你认为必须同 PR 落地,我可以加。
  • 持久故障只报一次 / cooldown 重新武装:修复后探测故障根本不再告警,这条主要针对的场景消失了;而生命周期故障路径已自愈重启,恢复后再坏会重新计数并再次告警。所以 recordInfraAlarm() 的 cooldown 模式在当前形态下必要性下降,我也倾向另开 issue 讨论,而不是在此 PR 引入。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fixed] Round 2 复核确认已修复。

证据: commit 6d1bfc4 将 processEntry 拆为 Phase 1(探测:enabled/isAvailable 异常完全不动 entry)和 Phase 2(执行:仅生命周期失败可触发 unexpected)。同时修复了 runOnActive 分支计数器提前重置 bug。新增回归测试验证探测连续抛错 10 轮 entry 保持 running、entry.stop() 从未被调用。


Generated by LoongSuite-Pilot Code Review Agent

@linrunqi08

Copy link
Copy Markdown
Collaborator

🔍 Code Review Summary

评审范围: 58b24404145a9fce(4 files, +248/-18),round-1,全量评审。本地已复现 PR 声称的测试结果(43 passed,Node v22)。

Severity Count
Critical 0
High 0
Medium 3
Low 7

(另有 2 条非本 PR 引入的既有缺陷、1 条文档补位建议,不计入门禁。)

Lifecycle Verdict

Check Result
资源释放 PASS
死锁/卡死风险 PASS
状态恢复正确性 PASS

范围声明:三项 PASS 仅覆盖本 PR 新增路径。证据 ——(1)新增的阈值停止路径经 stopEntryawait entry.stop() 真正释放资源,base 的 catch 分支从不释放;(2)无锁结构,保持 running 不阻塞任何后续轮次;(3)阈值停止后 BaseInput._running 已为 false,下轮恢复会完整重跑 start(),用例 does not double-start after an unexpected stop 断言 start() 恰好被调用 2 次。注意:因既有缺陷 I1,「discovery 状态 = running」并不等价于「采集健康」。

Merge Gate(合入门禁)

BLOCK ⛔(阻断级别:Critical / High / MediumLow 不阻断;Lifecycle 任一 FAIL 亦阻断)

本轮 open 的阻断级问题:

  1. Mediumsrc/core/agent-discovery-service.ts:173 — 被修复的 catch 分支在生产代码中几乎不可达(四类抛错入口逐一核查均不成立),PR 声称的根因缺少线上证据;未澄清。
  2. Mediumsrc/core/agent-discovery-service.ts:11ERROR_THRESHOLD 为纯次数阈值,注释「~15 minutes」不成立(per-entry pollInterval 实际默认 30s;fs.watch 回调无防抖,实测 10 次写入触发 4 次回调、间隔 <1ms),瞬时故障可在毫秒级耗尽阈值并打 L3 告警;未修复。
  3. Mediumsrc/core/agent-discovery-service.ts:182 — 达阈值后主动 entry.stop(),把探测健康度当作输入健康度,base 在同场景下从不中断采集(base-input.ts:41 使「冗余 start」实为空操作),构成数据面回归 + 永久静默;未修复/未论证。

总体结论

本 PR 方向正确:base 版本「探测抛异常 ⇒ 判定 input 已停 ⇒ 打 L3 告警,却从未调用 entry.stop()」确实是缺陷,本 PR 让「告警对应的停止一定真实发生过」成为不变量,测试也扎实。

但存在一个逻辑闭环:当前生产代码里这段被修复的 catch 分支几乎不可达(#1),而 base 产生误报的路径与它完全同源。 于是只有两种可能:

两条路都需要作者先补一份线上证据(告警时刻同进程的 processEntry failed / agent stopped unexpectedly 日志行,含实际 error 文本)再决定形态。三项均为低成本可解,澄清后可快速放行。

另外建议单独开 issue 跟进两条既有缺陷,其中 I1base-input.ts:42-48 start() 失败不回滚 _running,导致 input 永久坏死却被记为 running、零告警)是确定性触发、无需并发条件,且本 PR 新增用例在测试层面固化了该终态,值得优先处理。

Highlights(正向实践)

  1. 不变量选得准:让「emit unexpected ⇒ 必然先走过 entry.stop()」成为唯一路径(:182 → :204),从根上消除 base「告警了但没停」的错位。
  2. 状态机分流干净:按 running / 非 running 分流,首次启动失败不误报(:191-197),既有 starting 语义未被破坏。
  3. 测试扎实且贴合意图:新增 12 个用例覆盖保持状态、阈值触发、计数重置、不重复启动、非 unexpected 不带摘要等关键语义。
  4. 主动做了脱敏与长度约束summarizeError 的 homedir 掩码 + 200 字符截断,把仓内既有的裸 String(err) 进告警(updater.ts:310sls-flusher.ts:261)向上抬了一档。
  5. 沿用既有防抖形态consecutiveErrorsfix(core): classify input stop reasons to eliminate false INPUT_STOP_ALARM #198consecutiveUnavailable 结构对称、重置点齐备(仅漏防抖分支一处)。

评审报告详见: code-review/pr-242/final-report.md
Generated by LoongSuite-Pilot Code Review Agent

… threshold

Addresses review feedback on the previous commit.

1. Probe failures no longer stop collection. processEntry() is split into a
   probe phase (enabled/isAvailable) and an act phase (the entry's own
   lifecycle calls). A throwing probe says nothing about whether the input can
   still collect, so it now leaves the entry completely untouched — a running
   entry stays running — and never alarms; it only increments an observability
   counter and logs a warning.

   The previous commit stopped the entry after 3 probe exceptions, which was a
   data-plane regression: BaseInput.start() early-returns when already running,
   so the "redundant start" it claimed to fix was a no-op, while the new
   stopEntry() call really did clear the input's timers. Worse, once stopped the
   entry stayed idle (the probe kept throwing, so it never restarted) and never
   alarmed again — permanently silent collection with a single alarm.

2. The threshold now requires a time window. ERROR_THRESHOLD is a pure count,
   and processEntry() is driven by the global poll timer, a per-entry timer
   (config default 30s, not 300s) and unthrottled fs.watch callbacks, so a
   burst could exhaust 3 attempts in milliseconds. ERROR_MIN_WINDOW_MS (60s)
   additionally requires the failures to span real time. The misleading
   "~15 minutes" comment is corrected.

3. Fixed counter accumulation: the runOnActive branch reset the counter before
   awaiting start(), so a repeatedly-failing start could never accumulate past
   one. The reset now happens only after a successful call.

Only genuine input-lifecycle failures on a running entry can now produce
'unexpected'. Because the probe is healthy on that path, the next successful
poll restarts the entry, so it self-heals instead of going silent.
@Snssn Snssn changed the title fix(alarm): distinguish transient probe failures from real input stops fix(core): stop misclassifying detection-probe failures as input stops Aug 12, 2026

@ralf0131 ralf0131 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Re-review after new commits — the two-phase split (probe vs act) is a clean correctness fix.

Key observations:

  1. Probe/Act separation — The distinction is correct: a broken probe says nothing about whether the input can still collect. Leaving the entry untouched on probe failure is the right behavior.

  2. Time window (ERROR_MIN_WINDOW_MS = 60s) — Essential. Without it, a sub-second burst from fs.watch could exhaust ERROR_THRESHOLD and trip an L3 alarm. The combination of count + time prevents false positives.

  3. summarizeError — Good sanitization: whitespace collapsed, /root/hiclaw-fs/agents/github-loongsuite masked to ~, truncated to 200 chars. This makes alarm messages actionable without leaking paths.

  4. Counter reset logicconsecutiveErrors resets on success, firstErrorAt resets when the counter resets. This is correct and prevents stale state from accumulating.

  5. Test coverage — The tests cover probe failures (no state change), lifecycle failures (threshold + window), burst protection (3 failures in <1s doesn't trip), and counter reset on success. The burst test is particularly important since it validates the time window.

The scope note in the PR description is also helpful — clarifying that this is a correctness fix, not a fix for the volume of INPUT_STOP_ALARM seen in the field.


Automated review by github-manager-bot

export function summarizeError(err: unknown): string {
const home = os.homedir();
let text = String(err).replace(/\s+/g, ' ').trim();
if (home) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] summarizeError(): os.homedir()='/' 在 root 容器中破坏错误摘要

影响: text.split(home).join('~')os.homedir() 返回 '/' 时(root 用户容器、Docker),替换所有斜杠为 ~,将 Error: ENOENT /foo/bar 变为 Error: ENOENT ~foo~bar——丧失诊断价值。已实际验证:

const text = "Error: ENOENT /foo/bar/baz.txt";
text.split("/").join("~"); // => "Error: ENOENT ~foo~bar~baz.txt"

建议: 添加最小长度守卫:

if (home && home.length > 1) {
  text = text.split(home).join('~');
}

或使用更精确的前缀替换:text.replaceAll(home + '/', '~/')


Generated by LoongSuite-Pilot Code Review Agent

@linrunqi08

Copy link
Copy Markdown
Collaborator

🔍 Code Review Summary (Round 2)

评审范围: 58b244046d1bfc4b(4 files, +356/-20),round-2 增量评审。Round 1 的 3 条 Medium 已在 commit 6d1bfc4b 中全部修复并逐条验证。

Severity Count
Critical 0
High 0
Medium 1
Low 5

(另有 1 条 High 既有缺陷BaseInput.start()_running 泄漏)非本 PR 引入,不计入门禁。)

Round 1 技术复核

Comment 问题 技术状态
#r3759717151 catch 分支不可达 / PR 定位 ✅ fixed — PR 定位已调整,探测异常不再告警
#r3759740174 纯次数阈值可毫秒级耗尽 ✅ fixed — 新增时间窗 60s + burst 测试
#r3759740398 探测停数据面回归 ✅ fixed — 两阶段拆分 + 回归测试

Lifecycle Verdict

Check Result
资源释放 PASS
死锁/卡死风险 PASS
状态恢复正确性 PASS

三项 PASS 仅覆盖本 PR 路径。既有缺陷 I1(BaseInput.start()_running 泄漏)使首次启动失败场景下的状态恢复存在隐患,base/head 行为一致,建议单独 issue 跟进。

Merge Gate(合入门禁)

BLOCK ⛔(阻断级别:Critical / High / MediumLow 不阻断;Lifecycle 任一 FAIL 亦阻断)

本轮 open 的阻断级问题:

严重度 文件:行号 现状
Medium src/core/agent-discovery-service.ts:48 summarizeError()os.homedir()='/' 在 root 容器中替换所有斜杠为 ~,破坏错误摘要。修复为一行代码变更。

总体结论

本 PR 质量高,Round 1 的 3 条 Medium 全部修复到位,探测/执行两阶段拆分设计精准,测试覆盖扎实(12 个新测试用例)。仅剩 1 项 Medium(os.homedir()='/' 边缘情况),修复为一行 if (home && home.length > 1) 守卫,修复后可快速放行。

Highlights(正向实践)

  • 探测/执行两阶段分离从根本上消除了「探测坏 → 停采集」的语义错误
  • count + time window 双条件防止 fs.watch 突发误报
  • 自愈设计使停止后自动重启,不再永久静默
  • summarizeError 脱敏让存活告警可立即诊断
  • Round 1 评审修复质量高,每条都有详细回复和代码证据

评审报告详见: code-review/pr-242/final-report.md
Generated by LoongSuite-Pilot Code Review Agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants