Skip to content

daemon+web: continuous stuck-badge detector + orphan badge arbitration - #13

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-17_stuck-badge-detector
Open

ranxianglei wants to merge 1 commit into
masterfrom
2026-09-17_stuck-badge-detector

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What

Closes #12

The weekly recurrence class ("badge says processing, process is long dead") is rooted in one gap: after a processing badge is written, nothing continuously verifies whether it is true. The 5-min observer only covers issues owned by this daemon and trusts the engine's own memory/DB state — orphan badges (records wiped, session gone, process evaporated) are invisible to it forever.

This adds the runtime truth-check layer on top of the existing observer (which stays untouched as the fast path):

  1. Truth source swap — a processing badge is validated by three independent signals, not engine self-report: ① session pid alive (kill -0) ② recent session output (last_output_at) ③ recent model traffic (output-token delta). All three false ⇒ stuck.
  2. Continuous detector — 1-min sweep enumerates all processing badges fleet-wide via web (GET /api/v1/ai-status?status=processing), re-checks web state, then verifies signals for engine-known sessions. Badges with no engine record go straight to arbitration on the comment stream: bot delivery found ⇒ flip completed; none ⇒ flip idle + [system] notice "状态已复位,如需继续请回复" (conservative: status flip only, no auto-requeue).
  3. TTL semantics — every processing write now stamps expected_heartbeat_at; an un-renewed badge expires into stale regardless of what the engine believes. Live sessions renew it during the sweep.
  4. Observability — every sweep action is logged with reason; admin badge-monitor view shows the current processing list + per-badge three-signal verdicts (alive/stale/orphan/skipped).

Changes

  • web: issues.ai_status_since column (stamped on every ai_status change), /api/v1/ai-status endpoint, badge monitor view
  • daemon: config knobs (WORK_BADGE_SWEEP_MS default 60s, TTLs 10/10/15 min), sweepStuckBadges() + exported pure helpers (evaluateBadgeSignals, decideBadgeAction, BADGE_RESET_MARKER), heartbeat renewal, sweep-state getter

Verification

  • New unit matrix (signal evaluation, action decision, reset-notice marker) + integration harness with a mock web machine: dead-pid+no-output flip, live-pid keep (+heartbeat renewed), orphan+delivery→completed, orphan+no-delivery→idle+notice (posted exactly once), foreign-owner skip, and timer-driven auto-flip well under the spec's 2-minute bound (flip observed ~1s after record wipe)
  • Full suites: daemon tsc --noEmit clean, 277/277 pass; web tsc --noEmit clean, 182 pass / 9 fail — the 9 failures are pre-existing on master (verified identical failure list via stash-baseline run)

Notes:

  • Reverted an incidental bun.lock churn from bun install: committed lock says ework-aio 0.5.80 while packages/aio/package.json says 0.5.81 — pre-existing drift, left out of this diff to keep it focused.
  • No auto-requeue on reset (per spec's conservative first pass); the [system] notice invites the user to reply, which re-triggers normal pickup.

#12)

- web: issues.ai_status_since column stamped on every ai_status change;
  GET /api/v1/ai-status?status=processing fleet-wide badge endpoint;
  admin badge monitor view (badge list + per-badge sweep verdicts)
- daemon: 1-min badge sweep enumerates all processing badges via the
  tracker, re-checks web truth, then verifies three independent signals
  (pid kill -0, session output age, model token delta) for engine-known
  sessions; badges with no engine record go straight to arbitration on
  the comment stream (bot delivery -> completed, else idle + [system]
  reset notice with dedup marker); every processing write now carries an
  expected_heartbeat_at TTL that expires into 'stale' if not renewed
- daemon: sweep state exposed via getBadgeSweepState() for the admin view
- tests: unit matrix for signal evaluation/action decision + integration
  harness with mock web covering flip/keep/orphan-completed/orphan-reset/
  foreign-owner-skip/timer-driven auto-flip

Closes #12
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework

[bot] 🏷 收到,正在审查本 PR(不重做):拉取分支核对代码、跑测试套件、确认 CI/合并状态,稍后给出结论。

@ranxianglei

ranxianglei commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 ## Review conclusion: ✅ ready to merge

Reviewed PR #13 (head 69ba4cf, single commit on base 1d5057f) — did not redo the work, only verified branch state, CI, diff cleanliness, and re-ran the suites locally.

CI & mergeability

  • pull_request run on head 69ba4cf: success — all three jobs green (type-check + unit tests across packages, Docker E2E install/lifecycle/migration, Docker image smoke).
  • GitHub reports mergeable: true, mergeable_state: clean. Master has moved 7 commits past the PR base; the only file overlap is packages/web/src/store.ts, and a local test-merge of head + master tip is conflict-free.

Diff cleanliness

  • True diff vs merge-base: 16 files, +1083/-3 — every file on-topic (daemon sweep engine + exported pure helpers + config knobs + optional tracker method; web listing endpoint + ai_status_since column + admin badge-monitor view; new test file). No lockfile churn, no CI/supervisord edits. The description's note about leaving the pre-existing bun.lock drift out checks out — nothing lock-related in the diff.

Test verification (local runs)

  • daemon (head + master tip merged): tsc --noEmit clean; 277/277 pass — reproduces the PR's claim exactly.
  • web (head + master tip merged): tsc --noEmit clean; 185 pass / 9 fail. Also ran a pure-master baseline: identical failing set (7 wake-logins/AI-settings + 2 upsertUpstreamSync tests) — all 9 pre-existing on master, zero new failures introduced by this PR.

Code review highlights

  • Sweep core (packages/daemon/src/opencode.ts): reentrancy guard (badgeSweepRunning); re-verifies web state before any flip and fails closed (fetch error ⇒ no-op, never clobbers a concurrent fixup); foreign-owner skip via ownerDaemonId; releaseDeadOwners() runs before enumeration so dead-daemon orphans become visible; reset notice deduped by scanning the comment stream for the <!-- badge-reset --> marker; heartbeat armed on every processing-write path (session create / spawn / state→running / observer convergence) with 30 s-throttled renewal on output so chatty runs never expire.
  • Semantics: TTL expiry dominates the three signals — an expired heartbeat forces stale even with a live pid. That is exactly what spec item 3 asks ("regardless of what the engine believes") and it is pinned by an explicit unit test ("expired heartbeat forces stale even with a live pid"). Consequence worth knowing: a genuinely live process in one silent generation longer than badgeTtlMs (default 10 min, zero stdout) will briefly show idle + the reset notice, then self-heals within one observer cycle (~5 min) when the fast-path observer converges the badge back to processing. Spec-compliant; noted for awareness, not a defect.
  • Web side: /api/v1/ai-status sits behind the standard auth gate with parameterized SQL; ai_status_since is stamped only when the status actually flips (repeated same-status writes don't restart the age clock — important, otherwise the observer would keep the clock alive forever); legacy rows ('') sort oldest-first, which biases unknown-age badges toward arbitration — the safe direction given the pre-flip re-verification. Admin /admin/badges view is fully HTML-escaped, admin-gated, and degrades gracefully (reachable:false card) when a daemon doesn't answer.
  • Backward compat: sessions without expected_heartbeat_at fall back to pure signal logic; trackers without listBadges degrade to per-issue verification (optional interface method); both sqlite and mysql schemas get the additive column; destroy() clears the new timer and maps.
  • Good reuse: delivery arbitration uses the same hasRecoveryDelivery detector as restart recovery, with the prompt-time baseline (oldest non-terminal message → badge since → epoch 0) matching the recovery semantics.

Non-blocking observations (no changes requested)

  1. The model-traffic probe fires only when pid-dead AND output-stale (cheap-first), so signal ③ mostly matters for dead-pid cases; the 15-min model TTL rarely bites for live pids. Fine as designed.
  2. Two consecutive sweeps could theoretically race the notice dedup (first comment not yet visible when the second scans) → at most one duplicate [system] notice per sweep-interval window. Harmless.
  3. Daemon /api/badges is unauthenticated, same as the rest of the CLI HTTP API (/api/processes etc.) — consistent with the existing surface, and the payload is low-sensitivity (issue refs + verdicts).

All four spec items from #12 are implemented as written (three-signal truth check, 1-min fleet-wide sweep + orphan arbitration, TTL semantics, observability), the conservative no-auto-requeue stance is honored, and every verification claim in the description reproduced locally. Nothing found that blocks.

中文摘要:已核对 PR #13 的 CI(head 全绿、mergeable clean)、diff 整洁度(16 文件全部在题内,无锁文件/CI 污染)并本地复跑测试(daemon 277/277 通过;web 的 9 个失败经纯 master 基线比对确认为既有问题,本 PR 零新增失败);代码审查未发现阻塞项,TTL 优先于三信号的语义与规格一致且有测试锁定——可以合并。

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.

daemon+web: processing 徽标会说谎——持续卡死探测器 + 孤儿徽标仲裁(一周复发类的根治)

1 participant