Skip to content

fix(fifo): bound the probe-throws-because-gone liveness storm - #598

Open
klabulan wants to merge 1 commit into
awslabs:mainfrom
klabulan:upstream-pr/fifo-probe-gone-breaker
Open

fix(fifo): bound the probe-throws-because-gone liveness storm#598
klabulan wants to merge 1 commit into
awslabs:mainfrom
klabulan:upstream-pr/fifo-probe-gone-breaker

Conversation

@klabulan

Copy link
Copy Markdown
Contributor

The pipe-pane liveness watchdog (services/fifo_reader.py) has an unbounded failure path.

Bug: _check_pipe_liveness() calls probe() (a tmux capture-pane/get_history) as its first act. When the session, window, or the whole tmux server is gone, probe() raises (e.g. libtmux.exc.ObjectDoesNotExist). That exception reaches neither the re-arm-failure bound nor the cold-start bound — both sit downstream of a probe that returned — so it propagates to _watchdog_loops bare except and is re-logged as a full traceback per terminal, per tick, forever. In a real incident a set of gone ghost sessions produced ~578k error lines and a self-amplifying CPU/log load exactly when the host was already unhealthy.

Fix: add a per-terminal consecutive-probe()-failure counter, bounded exactly like the existing re-arm / cold-start give-up paths. After PIPE_LIVENESS_MAX_PROBE_FAILURES (new constant, default 5, env-overridable via CAO_PIPE_LIVENESS_MAX_PROBE_FAILURES) consecutive failures the terminal is dropped from the watchdog with one summary WARNING instead of a per-tick traceback. The counter resets on any successful probe, so a brief transient (a session momentarily unavailable but not gone) never accumulates into a false drop. The new counter is cleaned up in every existing drop path (stop_reader, cold-start give-up, re-arm give-up).

Tests: a probe that always raises is bounded (does not propagate) and drops the terminal after the cap and is never re-probed; a transient failure sequence resets on recovery and never false-drops.

Surfaced while operating CAO in production (harness-control#845).

🤖 Generated with Claude Code

…s-control#845)

The pipe-pane liveness watchdog's _check_pipe_liveness() calls probe() (a tmux
capture-pane/get_history) as its first act. When the session, window, or the whole
tmux server is gone, probe() raises (e.g. libtmux ObjectDoesNotExist). That exception
reaches NEITHER the rearm-failure NOR the cold-start bound (both sit downstream of a
probe that returned), so it propagated to _watchdog_loop's bare except and was re-logged
as a full traceback per terminal per tick, forever -- an unbounded, self-amplifying
log/CPU storm across every ghost terminal exactly when the box is already unhealthy
(live incident: ~578k error lines, a contributor to a near-simultaneous mass teardown).

Add a per-terminal consecutive-probe-failure counter, bounded exactly like the existing
rearm/cold-start give-up paths: catch the probe exception, count it, and after
PIPE_LIVENESS_MAX_PROBE_FAILURES (default 5, env-overridable) drop the terminal from the
watchdog with ONE summary WARNING instead of a per-tick traceback. The counter resets on
any successful probe, so a brief transient never accumulates into a false drop. The new
counter is cleaned up in every existing drop path (stop_reader, cold-start, rearm give-up).

Tests: probe-always-raises is bounded (no propagation) and drops the terminal; a
transient failure sequence resets on recovery and never false-drops.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 5fa5960)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Bounds the FIFO pipe-pane liveness watchdog’s failure mode when the tmux probe itself raises (e.g., session/window/server gone), preventing an unbounded per-tick traceback storm and dropping affected terminals from the watchdog after a capped number of consecutive probe failures.

Changes:

  • Add PIPE_LIVENESS_MAX_PROBE_FAILURES (env-overridable) to cap consecutive liveness probe exceptions.
  • Track per-terminal consecutive probe failures and drop the terminal from watchdog enrollment once the cap is reached; reset the counter on any successful probe.
  • Add tests covering (1) always-raising probes being bounded/dropped and (2) transient probe failures resetting on recovery.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/cli_agent_orchestrator/services/fifo_reader.py Adds per-terminal probe-failure strike counting and bounded give-up path to avoid watchdog traceback storms.
src/cli_agent_orchestrator/constants.py Introduces PIPE_LIVENESS_MAX_PROBE_FAILURES with env override CAO_PIPE_LIVENESS_MAX_PROBE_FAILURES.
test/services/test_fifo_reader.py Adds regression tests for bounded probe exceptions and counter reset behavior.
Suppressed comments (1)

src/cli_agent_orchestrator/services/fifo_reader.py:510

  • The bounded probe-failure logs don't include the underlying exception, which makes it harder to diagnose why probes are failing (e.g. tmux server gone vs. other unexpected errors). Consider including the last exception’s repr in both the retry debug and the final give-up warning so operators get actionable context without a per-tick traceback storm.
                logger.warning(
                    "pipe-pane liveness probe for terminal %s failed %d consecutive "
                    "times (session/window/server gone); dropping it from the watchdog",
                    terminal_id,
                    PIPE_LIVENESS_MAX_PROBE_FAILURES,

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

content = probe()
try:
content = probe()
except Exception:
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0903561). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/cli_agent_orchestrator/services/fifo_reader.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #598   +/-   ##
=======================================
  Coverage        ?   91.32%           
=======================================
  Files           ?      182           
  Lines           ?    24437           
  Branches        ?        0           
=======================================
  Hits            ?    22318           
  Misses          ?     2119           
  Partials        ?        0           
Flag Coverage Δ
unittests 91.32% <96.55%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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