Skip to content

fix(bin): stop away-mode daemon from reading its own pane as permanently busy - #3428

Closed
cm-maple7 wants to merge 2 commits into
kunchenguid:mainfrom
cm-maple7:fm/fm-afk-daemon-delivery-fix
Closed

cm-maple7 wants to merge 2 commits into
kunchenguid:mainfrom
cm-maple7:fm/fm-afk-daemon-delivery-fix

Conversation

@cm-maple7

Copy link
Copy Markdown

Intent

Fix the away-mode supervision daemon's injection wedge (data/fm-afk-inject-wedge/report.md, scout-investigated, root cause proven live). The daemon runs inside the captain's own supervisor pane by design (the /afk skill's no-separate-terminal exception for harnesses with a native in-pane background tool). On Herdr, a live tracked background shell pins the pane's native agent_status to 'working' for as long as the shell is alive, even after the turn has ended and the pane sits idle. pane_is_busy trusted that native busy verdict alone and short-circuited before ever checking the rendered footer, so the daemon read its own supervisor pane as permanently busy and could never deliver an away-mode escalation for its entire lifetime - proven against 7+ hours of production logs (3717 busy deferrals, 0 successful max-defer recoveries in the whole log). Implemented the report's three recommended, priority-ordered fixes: (1) root cause - pane_is_busy now requires the harness-scoped rendered busy footer (the same fm_busy_lines_match signature already used elsewhere to confirm a queued-while-busy submit) to corroborate a native busy verdict before treating the pane as unavailable; a native idle/unknown verdict was never trusted alone either, so this only changes the busy case, and falls back to trusting native only when the pane cannot be captured at all (fail toward not injecting into an unreadable pane). (2) backstop - the max-defer escape must actually escape rather than retrying the identical blocked call forever: inject_msg and escalate_flush gained a forced mode (force=1) that drops only the busy guard while keeping the composer-empty guard (the guard that actually protects against merging with a human's half-typed line), bounding worst-case silent non-delivery to FM_MAX_DEFER_SECS instead of being unbounded; also improved the wedge alarm's active-alert banner to carry the buffered item count and first line instead of only an age and a marker path, since 47 identical contentless banners in one production episode read as noise rather than an alert. (3) independent SIGTERM budget fix - fm_afk_launch_stop's wait for the daemon to exit after SIGTERM was a fixed 10s, but the daemon's cleanup trap can legitimately need up to FM_POLL seconds (15s default) plus flush time to reap its watcher child, which traps TERM but can be sitting in a foreground sleep; the wait now scales as (FM_POLL + 10s margin) so a watcher caught mid-sleep no longer produces a false 'did not exit after SIGTERM' that leaves lifecycle state needlessly preserved for retry. Explicitly out of scope / deferred to the captain as a design decision, not implemented here: the report's 'also worth doing' suggestion to reconsider hosting the away-mode daemon outside the supervisor pane entirely (removing the /afk skill's no-separate-terminal exception for Claude) - that is a bigger design tradeoff the fix above already resolves the concrete bug for. Added new regression tests for all three fixes in tests/fm-daemon.test.sh and tests/fm-afk-launch.test.sh, and updated one existing test that pinned the old (buggy) short-circuit behavior. Two pre-existing test failures were found during verification (tests/fm-wake-queue.test.sh 'a subshell reclaimed its parent's live hold', tests/fm-watch-triage.test.sh 'the fixture captured no process-event result') and confirmed unrelated: neither test file references anything touched by this change, and both failures reproduce identically with these changes fully reverted. They are filed as their own separate task and are explicitly out of scope here.

What Changed

  • pane_is_busy in bin/fm-supervise-daemon.sh now requires the harness-scoped rendered busy footer (fm_busy_lines_match, the same signature fm_backend_herdr_send_text_submit already uses) to corroborate a native busy verdict before treating the supervisor pane as unavailable, falling back to trusting the native verdict only when the pane cannot be captured at all; a native idle/unknown verdict was already not trusted alone.
  • inject_msg and escalate_flush gained a force parameter (used only by the max-defer escape in housekeeping()) that drops the busy guard while keeping the composer-empty guard, bounding worst-case silent non-delivery to FM_MAX_DEFER_SECS; inject_wedge_alarm's active-alert banner now carries the buffered item count and first line instead of only an age and marker path.
  • fm_afk_launch_stop in bin/fm-afk-launch.sh now scales its post-SIGTERM wait with FM_POLL ((FM_POLL + 10) * 4 quarter-second iterations) instead of a fixed 10s budget, so a watcher child caught in a foreground sleep no longer triggers a false "did not exit after SIGTERM".
  • Added regression tests for all three fixes in tests/fm-daemon.test.sh and tests/fm-afk-launch.test.sh, updated one existing test that pinned the old busy-verdict short-circuit, and updated .agents/skills/afk/SKILL.md to document the corroboration requirement and forced max-defer escape.

Risk Assessment

✅ Low: The three fixes are small, well-scoped, and each directly traces to the proven root cause: pane_is_busy now requires the rendered busy footer to corroborate a native busy verdict (falling back to native only when capture fails), the max-defer escape correctly bypasses only the busy guard via a new force parameter while the composer-empty guard (the one that actually prevents corrupting a human's half-typed line) still applies unconditionally, and the SIGTERM wait now scales with FM_POLL to match the watcher's real shutdown budget; all call sites and new regression tests were traced and are consistent with the implementation, and the two pre-existing unrelated test failures were confirmed to share no code path with this change.

Testing

Targeted tests for both changed source files pass in full (180 assertions total, 0 failures), including all new regression coverage for the three priority-ordered fixes described in the intent; a direct manual exercise of pane_is_busy reproduces the original wedge condition (native 'busy' pinned by a live background shell against an actually-idle rendered footer) and confirms the fix now allows delivery in that case while preserving safe behavior for genuinely busy or unreadable panes. The two pre-existing unrelated failures (fm-wake-queue.test.sh, fm-watch-triage.test.sh) called out in the intent were spot-checked and reproduce as described; they are out of scope for this phase per the task instructions and are not reported as findings here.

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • ./tests/fm-daemon.test.sh (127 assertions, exit 0) - includes new tests test_pane_is_busy_herdr_native_busy_uncorroborated_by_rendered_idle, test_pane_is_busy_herdr_falls_back_to_native_when_capture_fails, test_max_defer_forced_escape_bypasses_busy_guard, test_batch_flush_does_not_force_bypass_busy_guard, and the updated test_pane_is_busy_herdr_native_busy_corroborated_by_rendered
  • ./tests/fm-afk-launch.test.sh (53 assertions, exit 0) - includes new unit_stop_wait_exceeds_configured_poll
  • Manual direct invocation of pane_is_busy() (sourced from bin/fm-supervise-daemon.sh) against 3 scenarios: native-busy+idle-footer (reproduces the reported wedge, now resolves as not-busy), native-busy+busy-footer (still correctly reports busy), and native-busy+capture-failure (falls back to native verdict, fails safe)
  • git status --porcelain confirmed no transient artifacts left in the worktree after testing
⚠️ **Document** - 1 info
  • ℹ️ .agents/skills/afk/SKILL.md:106 - .agents/skills/afk/SKILL.md documents the max-defer escape contract twice (Busy-guard section and Injection hardening section) and pane_is_busy behavior likewise appears near duplicated phrasing across the file; both copies were updated to stay accurate for this change, but a follow-up could consolidate into one owner passage with a pointer.
🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: No code changes needed; lint passes after actionlint install
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

A live tracked background shell (the /afk skill's no-separate-terminal
exception) pins Herdr's native agent_status to 'working' for as long as
the shell is alive, even once a turn has ended and the pane sits idle.
pane_is_busy trusted that native 'busy' verdict alone and short-circuited
before ever checking the rendered footer, so the away-mode daemon
permanently read its own supervisor pane as busy and could never deliver
an escalation for its entire lifetime (data/fm-afk-inject-wedge/report.md).

- pane_is_busy now requires the harness-scoped rendered busy footer to
  corroborate a native busy verdict, falling back to native only when the
  pane cannot be captured at all.
- The max-defer escape now actually escapes: inject_msg/escalate_flush
  gained a forced mode that drops the busy guard (keeping the
  composer-empty guard) so unbounded silent non-delivery becomes a
  bounded worst case of FM_MAX_DEFER_SECS. The wedge alarm now carries
  the buffered item count and first line instead of only an age and path.
- fm_afk_launch_stop's SIGTERM wait now scales with FM_POLL plus a
  margin instead of a fixed 10s, since the daemon's cleanup trap can
  legitimately take up to FM_POLL seconds to reap its watcher child.
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The fractional FM_POLL shutdown regression should be fixed before merging because it can recreate the false daemon-exit failure this PR intends to eliminate.

The new shutdown calculation assumes an integer poll interval, while existing watcher usage permits fractional seconds; those values make the wait calculation fail and preserve lifecycle state prematurely.

Files Needing Attention: bin/fm-afk-launch.sh, tests/fm-afk-launch.test.sh

Reviews (1): Last reviewed commit: "no-mistakes(document): Update afk SKILL...." | Re-trigger Greptile

Comment thread bin/fm-afk-launch.sh
# FM_POLL seconds plus the flush. This wait must stay strictly greater
# than that budget or a watcher caught mid-sleep reports a false "did not
# exit after SIGTERM" and leaves lifecycle state preserved for no reason.
stop_wait_iters=$(( (${FM_POLL:-15} + 10) * 4 ))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Fractional poll breaks shutdown wait

When FM_POLL is a fractional interval such as 0.2, the new integer arithmetic expansion fails and skips the post-SIGTERM wait, causing a still-exiting daemon to be falsely reported as not having exited while lifecycle state is preserved.

@cm-maple7

Copy link
Copy Markdown
Author

Withdrawn - opened against upstream by mistake; this change is being landed in our own fork. Apologies for the noise.

@cm-maple7 cm-maple7 closed this Sep 1, 2026
cm-maple7 added a commit to cm-maple7/firstmate that referenced this pull request Sep 1, 2026
Greptile review on PR kunchenguid#3428 caught a real bug in the prior commit: the
stop-wait iteration count used bash arithmetic expansion directly on
FM_POLL, which errors on a non-integer value. Tests already exercise
fractional FM_POLL (0.1, 0.02), so that arithmetic error would silently
skip the post-SIGTERM wait and falsely report a still-exiting daemon as
hung. Compute the iteration count with awk instead, rounding up.
cm-maple7 added a commit to cm-maple7/firstmate that referenced this pull request Sep 1, 2026
Greptile review on PR kunchenguid#3428 caught a real bug in the prior commit: the
stop-wait iteration count used bash arithmetic expansion directly on
FM_POLL, which errors on a non-integer value. Tests already exercise
fractional FM_POLL (0.1, 0.02), so that arithmetic error would silently
skip the post-SIGTERM wait and falsely report a still-exiting daemon as
hung. Compute the iteration count with awk instead, rounding up.
cm-maple7 added a commit to cm-maple7/firstmate that referenced this pull request Sep 1, 2026
Greptile review on PR kunchenguid#3428 caught a real bug in the prior commit: the
stop-wait iteration count used bash arithmetic expansion directly on
FM_POLL, which errors on a non-integer value. Tests already exercise
fractional FM_POLL (0.1, 0.02), so that arithmetic error would silently
skip the post-SIGTERM wait and falsely report a still-exiting daemon as
hung. Compute the iteration count with awk instead, rounding up.
cm-maple7 added a commit to cm-maple7/firstmate that referenced this pull request Sep 1, 2026
* fix(bin): stop the away-mode daemon wedging on its own busy pane

A live tracked background shell (the /afk skill's no-separate-terminal
exception) pins Herdr's native agent_status to 'working' for as long as
the shell is alive, even once a turn has ended and the pane sits idle.
pane_is_busy trusted that native 'busy' verdict alone and short-circuited
before ever checking the rendered footer, so the away-mode daemon
permanently read its own supervisor pane as busy and could never deliver
an escalation for its entire lifetime (data/fm-afk-inject-wedge/report.md).

- pane_is_busy now requires the harness-scoped rendered busy footer to
  corroborate a native busy verdict, falling back to native only when the
  pane cannot be captured at all.
- The max-defer escape now actually escapes: inject_msg/escalate_flush
  gained a forced mode that drops the busy guard (keeping the
  composer-empty guard) so unbounded silent non-delivery becomes a
  bounded worst case of FM_MAX_DEFER_SECS. The wedge alarm now carries
  the buffered item count and first line instead of only an age and path.
- fm_afk_launch_stop's SIGTERM wait now scales with FM_POLL plus a
  margin instead of a fixed 10s, since the daemon's cleanup trap can
  legitimately take up to FM_POLL seconds to reap its watcher child.

* no-mistakes(document): Update afk SKILL.md for pane_is_busy corroboration and forced max-defer escape

* fix(bin): make the SIGTERM wait budget survive fractional FM_POLL

Greptile review on PR kunchenguid#3428 caught a real bug in the prior commit: the
stop-wait iteration count used bash arithmetic expansion directly on
FM_POLL, which errors on a non-integer value. Tests already exercise
fractional FM_POLL (0.1, 0.02), so that arithmetic error would silently
skip the post-SIGTERM wait and falsely report a still-exiting daemon as
hung. Compute the iteration count with awk instead, rounding up.

---------

Co-authored-by: cm-maple7 <cm-maple7@users.noreply.github.com>
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.

1 participant