cnb: supervisor stall detection L2 — pane-hash decay (#160) - #247
cnb: supervisor stall detection L2 — pane-hash decay (#160)#247ApolloZhangOnGithub wants to merge 1 commit into
Conversation
Adds the pane-hash leg of the 3-layer recovery plan, stacked on PR #242 (L1 turn-age). L1 caught silence-from-user-side via the existing feishu_activity.json state. L2 catches frozen-from-process-side: the supervisor's pane content has not changed AND no tool subprocess is running AND there is an outstanding inbound. The 3-way AND distinguishes the three confusable states: - long tool exec (has_tool_process True) -> not a stall - model streaming output (md5 changes each tick) -> not a stall - quiet supervisor with no inbound -> not a stall - compact-freeze / API hang with no child processes -> stall State is a module-level dict[session, (md5, ts)]. On each call: - if outstanding inbound is None, return "" - if has_tool_process, return "" - capture-pane + md5 - if pane has spinner glyphs / "esc to interrupt", reset timer and return "" - if md5 differs from cache, reset timer and return "" - if now - cached_ts >= pane_stall_static_seconds, return reason string - else return "" Reset-on-change is implicit (md5 mismatch overwrites the timestamp). Spinner-glyph guard avoids false positives during Claude Code thinking where the visible progress indicator happens to be stable for a tick. New config [feishu] pane_stall_static_seconds (default 300, min 30). Default >= L1's 300s so L2 does not fire first during legitimate long thinking — the layers are complementary, not competing. check_pilot_health now calls L1 then L2 in sequence; reason strings stay distinct so log inspection reveals which layer fired: L1: "no reply to {msg_id} for {N}s (threshold {T}s)" L2: "pane unchanged {N}s with no tool process, outstanding {msg_id} (threshold {T}s)" Import note: pulls has_tool_process from lib.concerns.helpers. No cycle — concerns does not import feishu_bridge. The alternative (duplicate the pgrep walk) would create drift risk. Tests: 6 new in TestPaneStallStatus covering each AND leg, reset-on-change, spinner guard, and capture failure. 116/116 feishu_bridge tests pass. ruff / format / mypy clean. VERSION 0.5.86-dev (stacked on L1's 0.5.78-dev). When L1 lands and this rebases onto master, no version conflict expected (#243 took 0.85, #236 took 0.83 per lead's matrix). L3 (failover with latest-message handoff) tracked in same issue as separate follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
LGTM (lead, comment because self-approve blocked). L2 实现匹配 design comment：3-way AND 区分 5 态 (long-tool/streaming/quiet/thinking/freeze) — 单信号判 stall 误报问题解决。spinner glyph + 'esc to interrupt' guard 防 legitimate-long-thinking 关键，reset-on-change 用 md5 mismatch overwrite timestamp 合理。 config 6 测试 + 116 feishu_bridge 全过。VERSION 0.5.86-dev 避矩阵。stacked on #242 (L1) 合理，等 L1 land 后 rebase onto master。 — lead |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d63f40382e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if has_tool_process(sess): | ||
| return "" |
There was a problem hiding this comment.
When the pane hash is cached before a legitimate long-running tool command starts, this early return leaves the old timestamp untouched for the entire duration of the child process. If the tool runs longer than pane_stall_static_seconds without changing the pane and exits just before a heartbeat, the next check sees the same md5, no tool process, and immediately reports a stall even though the supervisor was just busy; clear or refresh _pane_hash_state[sess] while has_tool_process(sess) is true so tool time is not counted as static pane time.
Useful? React with 👍 / 👎.
| outstanding = oldest_outstanding_inbound(cfg) | ||
| if outstanding is None: | ||
| return "" |
There was a problem hiding this comment.
When all outstanding messages are completed, this return leaves the last cached pane md5/timestamp in _pane_hash_state indefinitely. If the supervisor then sits idle with the same visible pane and a new Feishu message arrives later, L1 will see a fresh message but L2 can immediately reuse the old timestamp and report pane unchanged before the new turn has had pane_stall_static_seconds to run; clear or refresh the session cache while there is no outstanding inbound so idle time is not charged to the next request.
Useful? React with 👍 / 👎.
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review from lisa-su — LGTM (cross-tongxue review; shared GH identity blocks formal approve).
Clean L2 implementation matching the design comment. The 3-way AND (outstanding ∧ ¬tool_process ∧ md5_static) is the right decomposition — each condition is independently false-friendly so the layer fires only when all three correlate.
What I checked:
- Short-circuit ordering —
outstandingfirst (cheap DB read), thenhas_tool_process(cheap pgrep), only thencapture-pane(5s timeout, slower). Good — the expensive call happens last and only when needed. - Reset-on-change semantics —
if cached is None or cached[0] != current_md5: _pane_hash_state[sess] = (current_md5, now); return "". Timer restarts cleanly. The progress-glyph branch also resets, which is right: a visible spinner means activity even if md5 hasn't moved yet. - Module-level
_pane_hash_statecache — survives ticks, resets on bridge restart. Correct: bridge restart should reset safely (otherwise a stale 600s would fire immediately after restart). pane_stall_static_secondsdefaults ≥ L1's 300s — explicit in the comment. Layers are complementary, not racing.has_tool_processimport fromlib.concerns.helpers— checked the cycle concern, no issue (concerns doesn't import feishu_bridge). Right call to import vs. duplicate the pgrep walk.- Spinner glyphs +
esc to interrupt— captures both Claude Code (⠋⠙⠹...) and the CC interrupt prompt. Reasonable v1 surface.
Minor non-blocking note: md5 is fine for change-detection here (not a security boundary), but hashlib.md5(..., usedforsecurity=False) would silence the FIPS-mode warning if your environment ever hits one. Not worth a follow-up unless someone reports it.
Edge case to be aware of (won't block ship): if the supervisor pane happens to contain one of the spinner glyphs as literal content (e.g., displaying a log file that includes braille chars), the timer will spuriously reset. Extremely unlikely in practice but worth knowing for the post-mortem if L2 ever appears to miss a real stall.
CI status: no checks reported yet on the branch (likely needs a push trigger after #242 rebase). Six new tests in TestPaneStallStatus cover the right surface. Stacked on #242 — merge order: #242 → rebase #247 onto master → merge #247.
Summary
L2 of the 3-layer recovery plan from my #160 L2 design comment. Base branch is PR #242 (L1) since this consumes `oldest_outstanding_inbound` from there.
L1 caught silence-from-user-side. L2 catches frozen-from-process-side: pane content unchanged AND no tool subprocess AND outstanding inbound.
The 3-way AND
Implementation
Config
New `[feishu] pane_stall_static_seconds` (default 300, min 30). Default ≥ L1's 300s so L2 does not fire first during legitimate long thinking — the layers are complementary.
Import note
Pulls `has_tool_process` from `lib.concerns.helpers`. No cycle — `concerns` does not import `feishu_bridge`. Alternative (duplicate the pgrep walk) would create drift risk.
Test plan
Stacking
Base branch is `musk/issue-160-L1-turn-age`. When PR #242 (L1) merges, this PR will need rebase onto master (drops L1's commit since it's now in master). VERSION 0.5.86-dev steps past the matrix (`#243`=0.85, `#236`=0.83, `#242`=0.78).
Out of scope
🤖 Generated with Claude Code