Skip to content

fix(sweep): tmux - delivery retries, REPL death detection, transcript swap race - #713

Merged
olegbrok merged 2 commits into
mainfrom
fix/sweep-tmux
Jun 11, 2026
Merged

fix(sweep): tmux - delivery retries, REPL death detection, transcript swap race#713
olegbrok merged 2 commits into
mainfrom
fix/sweep-tmux

Conversation

@bradbrok

Copy link
Copy Markdown
Owner

Summary

Part of a full-codebase bug sweep: every finding below came from a multi-agent audit and was independently re-verified against the code before fixing. Fixes are minimal and scoped to the files cited; no two sweep PRs touch the same source file, so they can merge in any order.

Findings fixed

  • medium Tailer offset/buffer clobbered by concurrent set_transcript_path during awaited turn callbacks (src/pinky_daemon/tmux_transcript.py:658)
    • Added a swap-generation counter bumped by path-changing set_transcript_path; _read_and_dispatch re-checks it after each awaited turn callback and discards the rest of the old file's chunk (no offset advance) when a swap landed mid-callback.
  • medium attempt_reconnect never re-primes a wake prompt (src/pinky_daemon/tmux_session.py:5192)
    • attempt_reconnect's success branch now derives wake_reason from the launch flags exactly like force_restart and front-enqueues the orientation wake prompt before restarting the worker.
  • medium Transient tmux command failures silently drop user messages (src/pinky_daemon/tmux_session.py:4092)
    • Worker now treats TimeoutError from delivery as transient (keeps _inflight_turn, retries up to _DELIVERY_TIMEOUT_RETRY_LIMIT=3 with backoff) and, on any permanent give-up of an external turn, routes a delivery-failure notice through _response_callback so the chat user is told the message was dropped.
  • medium Dream runner never detects REPL death (src/pinky_daemon/tmux_dream_runner.py:273)
    • Spawn sets remain-on-exit; _wait_for_result probes pane_dead (or session-gone) each poll and raises a distinct _ReplExitedError with the pane tail, which run() converts to a prompt RunResult error instead of burning the full 1h timeout.
  • medium Dream runner _tmux() has no subprocess timeout (src/pinky_daemon/tmux_dream_runner.py:125)
    • _tmux wraps proc.communicate() in asyncio.wait_for (default 5s, mirrors _TmuxControl._run), kills the process on expiry, and returns rc=124 so callers (including the finally kill-session) handle it as a normal failure instead of hanging forever.
  • low stats['pending_responses'] is ~always False under concurrent dispatch (src/pinky_daemon/tmux_session.py:1407)
    • stats now reports pending_responses = len(_inflight_metas) + _message_queue.qsize() instead of the sub-second _processing paste window.
  • low Dream prompt/result files accumulate unbounded in dreams/ (src/pinky_daemon/tmux_dream_runner.py:145)
    • run()'s finally unlinks prompt/result files after successful runs; failure artifacts are deliberately kept for post-mortem (the nightly-success path was the unbounded growth).

Testing

tmux: python3 -m pytest tests/test_tmux_session.py -q -> 261 passed (includes 6 new regression tests); python3 -m pytest tests/test_tmux_transcript.py tests/test_tmux_dream_runner.py -q -> 58 passed (1 new transcript swap-race test, 3 new dream-runner tests, 1 happy-path test updated for file cleanup); python3 -m pytest tests/test_session_watchdog.py tests/test_agent_status.py tests/test_transport.py tests/test_tmux_context_autorestart.py tests/test_tmux_trust_seed.py tests/test_tmux_container_isolation.py -q -> 179 passed; python3 -m pytest tests/test_api.py -q -k tmux -> 3 passed; python3 -m pytest tests/test_broker.py -q -> 33 passed; ruff check on all 6 changed files -> All checks passed. No failures anywhere.

Generated with Claude Code

…data-loss, dream runner hardening

- tmux_transcript.py: swap-generation guard so a concurrent set_transcript_path during an awaited turn callback discards the old file's remaining chunk instead of leaking dead-session text and corrupting the new offset
- tmux_session.py: attempt_reconnect now front-enqueues an orientation wake prompt after respawn (force_restart/#589 parity), so heartbeat-resurrected agents are not orientationless
- tmux_session.py: worker retries turn delivery on tmux command timeouts (bounded budget) and routes a delivery-failure notice to the sending chat on permanent failures instead of dropping messages silently
- tmux_session.py: stats pending_responses now counts in-flight turns + queue backlog instead of the sub-second paste window, fixing the UI busy badge and the session_watchdog backlog gate
- tmux_dream_runner.py: _tmux subprocess calls get a 5s timeout (kill on expiry) so a hung tmux server cannot wedge the dream task and the #704 overlap guard
- tmux_dream_runner.py: _wait_for_result checks REPL liveness each poll (remain-on-exit + pane_dead probe) and fails fast with the pane tail instead of burning the full 1h timeout
- tmux_dream_runner.py: prompt/result files are deleted after successful runs (kept on failure for post-mortem) so dreams/ no longer grows unbounded
- tests: regression coverage in test_tmux_transcript.py, test_tmux_session.py, test_tmux_dream_runner.py

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread src/pinky_daemon/tmux_dream_runner.py Fixed
Comment thread src/pinky_daemon/tmux_dream_runner.py Fixed

@olegbrok olegbrok left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Barsik review — needs-changes (one major)

Five of seven fixes verified correct and well-tested — the transcript swap race (generation counter + early-return-before-offset-advance) and the orientationless reconnect wake re-prime are genuine bugs with clean fixes. Effort/--effort wiring (#650) untouched. No semantic conflict with #531/#612/#613 (note: #531 and #612 currently carry an identical _verify_repl_survived_launch diff — reconcile those two separately; whichever merges after #713 needs a routine rebase, disjoint hunks).

  1. (major) tmux_session.py:1409-1422 — the pending_responses stats change silently arms the outer session watchdog for tmux. Pre-PR, pending was effectively always 0 so the 10-min warn / 15-min auto-recover never fired for tmux. Post-PR any in-flight turn counts as backlog, and the outer watchdog has NONE of the inner _inflight_watchdog's liveness carve-outs (transcript-recently-grew, _background_tasks_recently_active from #692, live_status idle floor). Concrete regression: a long turn parked on a background Workflow/Agent fan-out (real production pattern — it bit me TODAY at 15:39, mid-review) gets warn-alerted at 10 min and disconnect-recovered mid-turn at 15 min, reintroducing through the outer watchdog exactly the kill #692 removed from the inner one. Fix: count only _message_queue.qsize() toward the backlog gate (expose inflight separately), or surface busy-evidence in stats and gate the watchdog on it.
  2. (minor) tmux_dream_runner.py:317-334 — _repl_alive treats rc=124 probe-timeout as death; one transiently hung tmux probe at 3am kills the night's dream. Require pane_dead==1/'can't find session', or two consecutive dead probes.
  3. (minor) tmux_dream_runner.py:347-353 — liveness probe is in the elif: if the REPL creates a 0-byte result file then dies, death is never detected — burns the full 1h timeout. Probe liveness when size isn't growing too.
  4. (minor) delivery-retry duplicate window: a timeout on the final send-keys Enter where tmux actually processed it re-submits the turn (up to 3×) — side-effecting instructions can run twice; deserves an explicit comment + ideally a capture-pane input check before re-paste.

Fix the watchdog finding (plus ideally the rc=124 false-death) and this is merge-ready.

- stats: pending_responses counts only queue backlog; in-flight turns are
  exposed separately as inflight_turns (UI busy state reads both), so a
  long-running turn no longer arms the outer session watchdog, which lacks
  the inner _inflight_watchdog's liveness carve-outs
- dream runner: _repl_alive is tri-state; an rc=124 probe timeout is
  indeterminate, not death - aborting needs pane_dead/session-gone or two
  consecutive dead probes
- dream runner: probe REPL liveness whenever the result file is not
  growing, so a 0-byte result file plus a dead REPL fails fast instead of
  burning the full timeout
- worker: explicit duplicate-submit comment plus a capture-pane guard
  before re-pasting a timed-out turn; a landed paste is recorded as
  delivered instead of re-submitted
- dream runner: annotate best-effort except clauses (kill race, cleanup)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bradbrok

Copy link
Copy Markdown
Owner Author

All four findings addressed in 1b845bd:

  • Major: took option 1 - stats['pending_responses'] now counts only _message_queue.qsize() (confirmed that is the exact key session_watchdog._take_snapshot gates warn/recover on), restoring pre-PR watchdog behavior for in-flight turns; len(_inflight_metas) exposed separately as stats['inflight_turns'] so the UI keeps accurate busy state. Frontend busy/online detection (Agents.svelte, Dashboard.svelte) sums both - those two files belong to fix(sweep): fe-rest - dead filters/toggles, wizard state leaks, provider key redaction #723 but the hunks are disjoint, merge-tree verified clean both orders.
  • Minor 2: _repl_alive is tri-state - dead only on definitive evidence (pane_dead==1 or can't-find-session); rc=124 probe timeouts are indeterminate and require two consecutive dead probes before aborting the dream.
  • Minor 3: liveness is now probed whenever the result file is not growing (absent, 0-byte, or stalled), so 0-byte-then-death fails fast instead of burning the 1h timeout.
  • Minor 4: explicit DUPLICATE-SUBMIT WINDOW comment plus a capture-pane guard - before re-pasting after a delivery timeout, the worker looks for the prompt head in the pane; if it landed, the retry is skipped. Guard deliberately fails toward retry on ambiguous markers (a false 'landed' would drop a message).
  • CodeQL 500/501: both empty excepts annotated/logged.

382 tests across tmux/watchdog suites pass; full 15-branch octopus suite green locally.

Generated with Claude Code

@olegbrok olegbrok left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Barsik re-review — approve ✅ (hold merge for CI on head SHA)

All four findings substantively fixed at 1b845bd, not renamed-around:

  1. pending_responses = queue.qsize() only; inflight exposed separately as inflight_turns (option A, with a comment naming the watchdog gate). Verified the watchdog's require_backlog gate now short-circuits on long in-flight turns with empty queue — the 15-min mid-turn auto-recover is closed; UI consumers updated to sum both keys.
  2. _repl_alive is tri-state — rc=124 probe-timeout is indeterminate; abort needs definitive pane-death or 2 consecutive failed probes. Both tested.
  3. Liveness probed whenever the result file isn't growing (absent/0-byte/stalled) — 0-byte+dead-REPL detected <5s in test. remain-on-exit at spawn makes pane_dead observable.
  4. _timed_out_turn_landed capture-pane guard before re-paste, with full delivery bookkeeping via _finish_turn_delivery (response routing preserved — checked it's not stats-only).

Non-blocking nits: marker false-positive possible when a prompt's first 40 chars match a previous turn still in scrollback (wake-prompt boilerplate) — worth a comment; stale old-semantics comment at tmux_session.py:132.

Merge condition: Python 3.11/3.12/3.13 test jobs were still in_progress and 'Build + smoke (amd64)' hit a Buildx setup flake at review time — re-run the flake and wait for the head SHA to conclude green before merging.

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