Fix/fifo pipeline stalls - #463
Conversation
The cold-start liveness check (harness-control#93) exists to re-arm a pipe-pane forwarder that never started delivering. It is gated on `not ever_delivered`, but `_ever_delivered` was set where bytes are pulled off the FIFO — not where they are published to the event bus. Those are not the same moment. Bytes sit in `pending` until the coalesce window closes, and when the forwarder cold-starts dead, the only bytes ever read are flushed by the `finally:` block at thread teardown. So `ever_delivered` flipped True while the StatusMonitor buffer stayed empty, permanently blinding the one check that would have healed the pipe. The divergence path cannot cover for it either: an idle shell's pane is static, so it never accumulates strikes. Net effect was a dead pipe that nothing re-armed, and wait_for_shell() timing out after 60s on every single `cao launch`. Move the flag to the publish site so it means what the check reads it as — "this pipeline has delivered to consumers". `_last_data_at` stays on the raw read: the divergence check genuinely wants "did the FIFO yield bytes since the last tick", and its semantics are unchanged. Verified with a probe replicating terminal_service's exact sequence: before, 0 events / 0 re-arms across 3/3 runs (matching 4/4 real launch failures); after, events delivered 3/3 with the watchdog firing and self-healing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NOWN tmux silently stops forwarding a pane's output to the FIFO after a burst of alternate-screen redraws (awslabs#388) — Claude Code's Ink TUI is exactly that shape. The rolling buffer then freezes on whatever it last saw, typically the pre-launch shell prompt, which detects as UNKNOWN, while the pane itself renders a healthy agent idling at a ready prompt. The liveness watchdog structurally cannot recover this. Once the TUI finishes painting, the pane is STATIC, so "pane advanced but FIFO silent" never trips — a settled frame is indistinguishable from a genuinely idle terminal. Live-reproduced: exactly one cold-start re-arm, then the watchdog sat silent for 66s (alive and ticking the whole time) while wait_until_status timed out at 60s on an agent that was ready the entire time. Detection was never the problem — on the real captured output, get_status_from_screen returns IDLE and raw get_status returns COMPLETED. CAO simply never saw the bytes. So when the pushed pipeline knows nothing, ask tmux directly. capture-pane is immune to the stall (it reads tmux's own screen) and the providers' detectors already understand composited screen content — it is what _handle_startup_prompts() has always used to spot trust/bypass dialogs while the FIFO was dead. Gated on a cached UNKNOWN, mirroring the PROCESSING escape hatch already in get_status() for the same class of "debounced detection is stuck" problem. It therefore only ever upgrades a non-answer into a real one, and never forks tmux on the hot per-chunk path the class docstring warns about. Verified against real Claude Code running in tmux with no CAO or FIFO involved: get_status_from_screen(capture-pane) -> COMPLETED in 4s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Records all three bugs, why each hid the next, the evidence behind each fix, and the install-layout trap that reverts them (uv tool install is NON-editable — site-packages is a copy, so reinstalling from `main` silently undoes both code fixes). Includes setup steps for a new machine, the debugging notes that were expensive to rediscover (bus.publish silently no-ops without set_loop; _apply_detection only logs on change; the reader spins rather than blocks in os.read), and the wrong turns worth not repeating — chiefly the stale-TUI-regex theory, which is false: both failures occur before or independently of TUI parsing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@reyph can you please help to add PR description about what this change is about ? why this change is required ? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
=======================================
Coverage ? 89.31%
=======================================
Files ? 157
Lines ? 18651
Branches ? 0
=======================================
Hits ? 16659
Misses ? 1992
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses tmux pipe-pane/FIFO pipeline reliability issues that can leave StatusMonitor stuck at UNKNOWN and cause terminal initialization waits to time out, by adding a live-pane (capture-pane) fallback and tightening FIFO “ever delivered” bookkeeping.
Changes:
- Add a
StatusMonitorescape hatch that re-detects status from tmuxcapture-panewhen the cached status isUNKNOWN. - Change FIFO reader
_ever_deliveredsemantics to reflect delivery to consumers (set on publish rather than raw read). - Add a markdown write-up documenting the debugging findings and the stacked failure modes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/cli_agent_orchestrator/services/status_monitor.py | Adds live-pane status detection fallback when FIFO-driven status is stuck at UNKNOWN. |
| src/cli_agent_orchestrator/services/fifo_reader.py | Adjusts _ever_delivered to track event-bus delivery rather than raw FIFO reads. |
| LOCAL-PATCHES.md | Adds a detailed diagnostic note set / reproduction guide for the observed failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| content = get_backend().get_history(session_name, window_name, strip_escapes=True) | ||
| except Exception as e: | ||
| logger.debug(f"live-pane detect [{terminal_id}]: capture-pane failed: {e}") | ||
| return None | ||
|
|
||
| if not content.strip(): | ||
| return None | ||
|
|
||
| try: | ||
| if getattr(provider, "supports_screen_detection", False): | ||
| # capture-pane output IS a composited viewport — exactly what | ||
| # get_status_from_screen expects (escape-free rows). | ||
| return provider.get_status_from_screen(content.split("\n")) | ||
| return provider.get_status(content) | ||
| except Exception as e: | ||
| logger.debug(f"live-pane detect [{terminal_id}]: detection failed: {e}") | ||
| return None |
| # wait_for_shell() timing out at 60s on every launch. | ||
| with self._lock: | ||
| if terminal_id in self._readers: | ||
| self._ever_delivered[terminal_id] = True |
| ```bash | ||
| # 1. Clone + install (uv tool install is NON-editable — it copies the source) | ||
| git clone https://github.com/awslabs/cli-agent-orchestrator.git ~/cli-agent-orchestrator | ||
| cd ~/cli-agent-orchestrator | ||
| git checkout fix/fifo-pipeline-stalls # <- the branch carrying fixes 2 & 3 | ||
| uv tool install . | ||
|
|
||
| # 2. Fix 1 is config, not code. Must be > provider_init_timeout (60). | ||
| cao config set server.mcp_request_timeout 120 | ||
|
|
||
| # 3. Start the server and launch | ||
| nohup cao-server > ~/.aws/cli-agent-orchestrator/logs/server-stdout.log 2>&1 & | ||
| cao launch --agents developer --provider claude_code --auto-approve | ||
| ``` | ||
|
|
||
| If you install from upstream `main` instead of the branch, bugs 2 and 3 come back. | ||
|
|
I already updated the description LOCAL-PATCHES.md is not required |
call-me-ram
left a comment
call-me-ram
left a comment
There was a problem hiding this comment.
Thanks for digging into this, @reyph — and welcome. The Bug-3 investigation found something real: I verified the #397 watchdog's divergence check re-baselines to current pane content whenever the FIFO advances (and on first-observation/cold-start re-arm), so a burst→stall→settle that completes within one 4s interval pins the baseline to the post-stall frame and strikes never accrue. Your 66s-of-silence observation matches that exactly. That's a genuinely good find and worth landing — but not in this shape. Requesting changes on four things:
1. [must-fix] Remove LOCAL-PATCHES.md. It's a personal machine runbook (hardcoded branch checkouts, ~/.local/share/uv/tools/... paths, a "wrong turns" diary) committed to the repo root. You already told @haofeif it isn't required — please drop the commit.
2. [must-fix] Commit d1b86fa ("Bug 2") is a production no-op resting on an impossible diagnosis — drop it or re-justify with a real trace + regression test. The claim is that read bytes "sit in pending until thread teardown" on a cold-dead pipe. They can't: once pending is non-empty, the flush fires on window-elapsed, the 64KB cap, or not readable (fifo_reader.py:354–356 on main) — every read publishes within ~100ms while the thread lives, so moving _ever_delivered from read-site to publish-site can't change any watchdog outcome. Your own debugging notes flag that bus.publish no-ops when bus._loop is None, which is the likely artifact behind the "0 events in 3/3 runs" probe result. The move also leaves _reader_loop's docstring ("recorded right when bytes are pulled off the FIFO") half-false, and main's test_cold_start_end_to_end_via_real_reader_thread pins the semantics you're changing.
3. [must-fix] Zero tests for the new code — codecov shows 40% patch coverage, 21 lines uncovered. _detect_from_live_pane and the UNKNOWN hatch in get_status have no test for the fallback firing, the three swallow-and-return-None exception paths, or the screen-vs-raw provider branch. Every prior PR in this chain (#383/#390/#397) shipped mechanical regression tests; the "146 passed" in the PR body is pre-existing tests only (I re-ran them at your head — they pass because they don't touch the new paths).
4. [should-fix, borderline] The UNKNOWN hatch forks a synchronous capture-pane on hot — sometimes event-loop — paths. Cached UNKNOWN is the normal early state of every healthy launch, and get_status is called directly from async wait_until_status (utils/terminal.py:181), from session listing (once per terminal per request), and from inbox delivery. So every launch now pays a blocking subprocess per 1s poll until first detection, with no backoff on the dead-pane exception path. The PROCESSING escape hatch this mirrors is a pure in-memory re-detect. Gate the fallback on stall evidence (e.g. FIFO silent past a threshold via fifo_manager's liveness state) and/or cache negative results with backoff.
On the design: even with the hatch, the pipe stays dead — get_buffer, LogWriter, and message extraction still see the frozen buffer, so launch "succeeds" and the first assign/handoff fails downstream. The durable fix belongs in the watchdog: add "pane has content + FIFO silent since baseline + composited status frozen at UNKNOWN" as a bounded stall trigger, reusing #397's existing re-arm + CRLF replay machinery. Happy to help scope that.
Also worth extracting: your Bug-1 observation is a real papercut — mcp_request_timeout: 30 < provider_init_timeout: 60 (settings_service.py:165–167) means any 30–60s init always fails client-side while the server succeeds, and the module's own docstring example shows 120/90. A one-line defaults change + test would be a small, immediately mergeable PR if you want a quick win.
Mechanically the diff is clean (black passes, keepalive-fd/non-blocking-open/teardown untouched, no dueling re-arm with the watchdog, probe runs outside the lock). The investigation is good; let's get the fix into the right layer with tests.
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #463 — Fix/fifo pipeline stalls
Summary
This PR addresses real launch failures with cao launch --provider claude_code: it moves the _ever_delivered cold-start liveness flag from the FIFO read site to the publish site (commit d1b86fa), adds a live capture-pane fallback in StatusMonitor.get_status() when the cached status is UNKNOWN (commit 2f666e3), and commits a personal debugging runbook (LOCAL-PATCHES.md). The underlying Bug-3 investigation is genuinely valuable — the maintainer independently confirmed the watchdog's divergence check cannot catch a burst→stall→settle sequence. However, a maintainer review at this exact head requested changes on four items, none of which are addressed, and the live-pane fallback additionally feeds provider screen detectors an input shape they were not calibrated for. Recommendation: Request changes.
Important (should fix)
- [correctness] src/cli_agent_orchestrator/services/status_monitor.py:575,587 —
_detect_from_live_panecallsget_backend().get_history(...)with notail_lines, which captures a 200-line scrollback tail (TMUX_HISTORY_LINES = 200, capture-pane-S -200), and passes it toprovider.get_status_from_screen(). Every existing caller ofget_status_from_screenpasses pyte-composited viewport rows (scr[0].display, status_monitor.py ~250/272), and the base-class contract (providers/base.py:153–176) says screen detectors are purpose-built and calibrated for that shape. 200 lines of scrollback can contain response glyphs / prompt boxes from earlier turns, so a detector scanning for "last completed-response marker" can return a stale answer. The inline comment ("capture-pane output IS a composited viewport") overstates whatget_historyreturns. Fix: bound the capture to the pane's visible height (tail_lines=pane height) or composite through pyte like the existing path. - [consistency] src/cli_agent_orchestrator/services/fifo_reader.py:318–341 — after moving
_ever_deliveredto the publish site, the surviving comment block above the_last_data_atwrite and the_reader_loopdocstring still describe liveness as "recorded the instant bytes are pulled off the FIFO", which now only describes half the state. The existing regression test (test/services/test_fifo_reader.py:735,test_cold_start_end_to_end_via_real_reader_thread) explicitly pins "the reader thread must flip_ever_deliveredonce real bytes are read" — it happens to still pass only because publish follows read within the coalesce window, so the semantic change ships silently under a test asserting the old meaning. If the publish-site move survives re-justification (see Prior feedback), the docstring, comment, and test assertion text all need updating together.
Nits (optional)
- [conventions] PR title — "Fix/fifo pipeline stalls" is the raw branch name; a descriptive title (e.g. "fix(status): fall back to live capture-pane when FIFO pipeline stalls") would follow the repo's conventional-commit style visible in the git log.
Tests
No tests added (codecov reports 40% patch coverage per the maintainer review). The new fallback path, its three exception-swallowing early returns, and the screen-vs-raw provider branch are all uncovered. The one existing test touching the changed semantics (test_cold_start_end_to_end_via_real_reader_thread) passes for incidental timing reasons while asserting the pre-change meaning of _ever_delivered — the opposite of pinning the new behavior. Prior PRs in this chain (#383/#390/#397) all shipped mechanical regression tests; this one should too.
Verification
Dynamic verification did not run for this review. Static verification performed against the PR worktree at head 04ee8a0:
- ✓ VERIFIED — maintainer's Bug-2 rebuttal: flush fires on
not readable(fifo_reader.py:353–357), so the "bytes sit until teardown" mechanism is impossible while the reader thread lives. - ✓ VERIFIED — Bug-1 defaults inversion is real (settings_service.py:165–167 vs docstring example at 206–208).
- ✓ VERIFIED — the UNKNOWN hatch is correctly unreachable on the herdr backend (
supports_event_inbox()returns early at status_monitor.py:606), so the "pipe-pane backend" claim in the comment holds. - ✓ VERIFIED —
LOCAL-PATCHES.mdis still present at the PR head despite the author stating it is not required. - ✗ REFUTED — the inline claim "capture-pane output IS a composited viewport":
get_historywith default arguments captures a 200-line scrollback tail, not the visible viewport (clients/tmux.py:453–458).
Verdict
Request changes — a maintainer review at this head has four unaddressed must-fix items (remove the committed personal runbook, drop or re-justify the no-op d1b86fa commit, add regression tests for the new fallback, and gate the hot-path capture-pane fork), and the fallback additionally feeds screen detectors a 200-line scrollback tail instead of the viewport rows they are calibrated for. The Bug-3 diagnosis itself is sound and worth landing once reshaped per the maintainer's guidance.
Local patches:
cao launch --provider claude_codefailsDiagnosed and fixed 2026-07-17 against CAO
main(a614f32) + Claude Code CLI v2.1.212 on Linux / Python 3.14.Three independent bugs stacked on top of each other. Each one hid the next, so fixing only the first
just moves the failure one stage later. All three must be applied.
TL;DR — setting CAO up on a new machine
If you install from upstream
maininstead of the branch, bugs 2 and 3 come back.The install layout (read this first — it causes the most confusion)
CAO is a
uv toolinstall from a local git checkout, and it is NOT editable:Consequences that will bite you:
uv tool install .rebuilds the copy from whatever the checkout is currently on. Reinstall whilesitting on
mainand you silently revert all patches.mainby many commits.Use
git -C ~/cli-agent-orchestrator log main..origin/main.Verify the two copies agree:
Any difference means the running server is not the code you think it is. Check this first if launches
break again after an update.
Bug 1 — inverted timeout defaults (config fix)
Symptom
Cause
The launch client POSTs with
timeout=mcp_request_timeout(default 30s,cli/commands/launch.py:290),while the server legitimately blocks up to
provider_init_timeout(default 60s) waiting for the agentto reach idle. Any init taking 30–60s therefore always times out client-side — while the server is
working correctly and would have succeeded. The defaults are simply inverted;
get_server_settings()'s owndocstring example shows the intended shape (
mcp_request_timeout: 120aboveprovider_init_timeout: 90).This error is a red herring: it masks the real failure. Fixing it doesn't make launch work, it makes
the actual error visible (a 500).
Fix — config only, no code, no server restart (the value is read client-side per invocation):
cao config set server.mcp_request_timeout 120Persists to
~/.aws/cli-agent-orchestrator/settings.json. Not versioned in git — re-apply per machine.Bug 2 —
_ever_deliveredblinded the cold-start watchdog (commitd1b86fa)Symptom
Note this fails in
wait_for_shell, which runs before Claude Code is ever launched — it is waiting forthe plain shell prompt. Any theory about Claude's TUI is therefore irrelevant here.
Cause
pipe-paneattach often forwards nothing (the known cold-start case, harness-control#93).pendinguntil the coalesce window closes — when the pipeis cold-dead, the only flush is the
finally:block at thread teardown._ever_deliveredwas set where bytes are read, not where they are published.not ever_delivered→ permanently blinded by bytes that never reached aconsumer.
wait_for_shelltimes out.Fix — move
_ever_delivered = Trueto the publish site so it means "delivered to consumers"._last_data_atstays on the raw read (divergence semantics unchanged).Evidence — probe replicating
terminal_service's exact sequence: before, 0 events / 0 re-arms in 3/3runs (matching 4/4 real launch failures); after, events delivered 3/3, watchdog self-heals.
Bug 3 — FIFO stalls on Claude's alternate-screen TUI (commit
2f666e3)Symptom (only visible after bug 2 is fixed)
Meanwhile Claude is completely healthy — banner rendered, trust dialog accepted, idling at a ready prompt.
Cause
tmux silently stops forwarding to the FIFO after a burst of alternate-screen redraws (issue #388) — Claude's
Ink TUI is exactly that shape. The rolling buffer freezes on the pre-launch shell prompt (detects UNKNOWN)
while the pane renders a healthy agent.
The liveness watchdog structurally cannot recover this: once the TUI finishes painting, the pane is
STATIC, so its "pane advanced but FIFO silent" divergence test never trips — a settled frame is
indistinguishable from a genuinely idle terminal. Observed: exactly one cold-start re-arm, then the watchdog
sat silent for 66s while alive and ticking.
Detection was never at fault. On the real captured output,
get_status_from_screen→ IDLE and rawget_status→ COMPLETED. CAO just never saw the bytes.Fix —
StatusMonitor._detect_from_live_pane()+ a hook inget_status(): on a cached UNKNOWN, detectfrom tmux's live
capture-paneinstead of the frozen buffer. Mirrors the pre-existing PROCESSING escapehatch in the same function. Gated on UNKNOWN, so it never forks tmux on the hot per-chunk path.
Evidence — real Claude in tmux with no CAO/FIFO involved:
get_status_from_screen(capture-pane)→COMPLETED in 4s. Tests: 146 passed.
The load-bearing insight
capture-paneis reliable. The FIFO / pipe-pane pipeline is not._handle_startup_prompts()has always usedcapture-paneand has always worked — even while the FIFO wasstone dead and every FIFO-fed consumer was blind. That pipeline has four upstream issues against it (#382
blocked opens / leaked threads, #388 stalled forwarder, harness-control#93 cold start, #148 burst-then-settle)
and elaborate self-healing machinery that still didn't cover this case.
If you need something to be correct, read
capture-pane. Bug 3's fix is an application of exactly this.Debugging notes
~/.aws/cli-agent-orchestrator/logs/cao_<date>.log.Per-terminal raw output:
logs/terminal/<terminal_id>.log(written by LogWriter off the same eventbus — if this file has content but StatusMonitor saw nothing, the data arrived in the teardown flush).
bus.publishsilently no-ops whenbus._loop is None(event_bus.py:61). A standalone repro scriptmust call
bus.set_loop()or every publish vanishes with no error.os.read, it does not block —O_NONBLOCKis genuinely set (verified viaF_GETFLon the live fd). A stack snapshot showingos.readis a spin artifact, not a wedge.fifo-pipe-watchdog; check it exists and its state:for t in /proc/$(pgrep -f bin/cao-server)/task/*; do cat $t/comm; doneParked in
futex_do_wait= healthy, ticking its 4sEvent.wait._apply_detectiononly logs on change — a status stuck at UNKNOWN logs nothing at all, which readsidentically to "nothing is happening".
constants.py): 3s cold-start grace, 4s check interval, 2 strikes to re-arm, max 5cold-start attempts. So a rescue should appear within ~7s; if it hasn't, the watchdog is blind, not slow.
Wrong turns — don't repeat these
providers/claude_code.py" — wrong. The provider explicitly handles v2.1.212chrome:
NEW_TUI_BOX_PATTERN, the●response glyph, the· ✢ * ✶ ✻ ✽spinner cycle, the● high · /effortfooter. Both PyPI 2.3.0 andmainhave it. Bugs 2 and 3 both fail before orindependently of TUI parsing.
cao@cao:~$fine.os.read" — wrong. It spins;O_NONBLOCKis set.Upstreaming
Neither fix is upstream. Both are genuine bugs in
awslabs/cli-agent-orchestratorthat will affect anyonewhose pipe cold-starts or who runs a full-screen TUI provider. A merged PR is the only thing that ends the
patch-carrying — until then, every reinstall risks reverting them.