fix(pty): stop the write loop spinning, without introducing the shutdown hang (#17355)

[mrveiss]

Thinking Path

Shard 10 of the backend suite was hitting the 60-minute CI ceiling. Per-shard
durations showed it at ~11 minutes of real work -- third-slowest, not an
outlier -- so the ceiling was being hit by a hang, not by contention.

_write_loop called input_queue.get_nowait() and continued on
queue.Empty with no sleep, no timeout, and no blocking get anywhere in its
body. An idle session re-enters the loop immediately and burns 100% of a core
-- one per LIVE terminal, no leak required. Measured on the pre-fix loop
against a counting queue: 845,748 calls in 0.4 seconds.

Its docstring said "Wait for input without timeout" -- what the loop was
meant to do, and the exact opposite of get_nowait. That is why the defect
survived review: a reader checking "does this block properly?" found a comment
saying it did.

The obvious fix is a trap. A plain blocking get() looks correct against
cleanup(), which does put a None sentinel -- but it parks forever on three
paths that stop this loop without one:

• the sentinel's put sits inside a try/except that swallows to
  logger.debug, after running = False is already set;
• an abandoned session never calls cleanup() at all;
• the loop's second condition, master_fd is not None, is signalled by
  nothing, and a parked thread re-checks neither term.

So the wait is bounded, not blocking.

What Changed

File	Change
autobot-backend/services/simple_pty.py	get_nowait() → get(timeout=_WRITE_WAIT_TIMEOUT_S); new module constant from TimingConstants.MICRO_DELAY, not a literal; cleanup()'s two pass statements now describe what actually happens
autobot-backend/services/simple_pty_write_loop_17355_test.py	New. 3 tests + a fake queue that models a real blocking get()

_WRITE_WAIT_TIMEOUT_S is not a latency budget -- get() returns the
moment input is queued. It bounds only how often an idle loop wakes, and how
long after running = False the thread notices.

cleanup()'s pass statements claimed the threads exit on the running flag.
That was true only because the writer spun: the spin was load-bearing for a
documented shutdown contract nobody had decided to rely on. No join() is
added, deliberately -- cleanup() is reached from async request paths, and a
blocking join there would stall an event loop to save milliseconds of thread
lifetime.

Verification

Three tests, each mutation-tested against the committed fixture:

Test	Mutation that must fail it	Result
idle loop does not spin	restore get_nowait()	fails at 845,748 calls
stops on running = False, no sentinel	plain blocking get()	fails (parks)
stops on master_fd going None	plain blocking get()	fails (parks)

Restored: 3 passed. The fake queue's get(timeout=None) never returns when
nothing is queued, so the blocking-fix mutation runs against real code rather
than being asserted in a comment.

Provenance of the numbers above

Separating what is authoritative here from what is not, because this repo does
not accept a local run as verification and the table above would otherwise read
as if it did:

Claim	Where it comes from	Authoritative?
the loop spins	the source: get_nowait() + continue on Empty, with no sleep, no timeout= and no blocking get( anywhere in the loop body	yes — static, checkable by reading the diff
a blocking get() parks	the three no-sentinel paths, each identified in the source and named in Thinking Path	yes — static
the 3 tests pass	the pre-push hook's own vitest/pytest run on this push	yes — hook-registered
845,748 calls / 0.4 s	a local run against a fake queue	no — design aid only
each mutation fails	local runs of mutated copies	no — a mutation cannot be pushed, so no sanctioned run exists

The two unauthoritative rows are why the fix is argued from the source rather
than from the measurement: the call count tells you the spin is fast, the
source is what tells you it spins at all. Treat 845,748 as an illustration of
scale, not as evidence. CI is the authority on whether the suite passes.

The number the bound is worth, stated rather than implied

The PR said the constant comes from TimingConstants.MICRO_DELAY "rather than a
literal" and never said what it is. That is the one figure a reviewer needs,
because a bound that is too small makes this change cosmetic:

autobot_shared/ssot_constants.py:580:    MICRO_DELAY = 0.1

	idle wake rate
before (get_nowait() + continue)	~2,100,000 / sec (845,748 in 0.4s)
after (get(timeout=0.1))	10 / sec

Roughly a 200,000x reduction, not a smaller spin. TimingConstants carries no
env-var backing for any of its members, so an SSOT constant is the class's own
convention here; the env-var rule in CLAUDE.md is scoped to cache TTLs, and this
is a poll bound, not a TTL.

Model Used

Claude Opus 5 (1M context).

---

Refs #17355

Deliberately Refs, not Closes. #17355 asks for two things: the spin, and
"why the threads outlive their session at all — a spin is only expensive
because the leak keeps it alive, and the leak is the other half." This PR fixes
the spin only. An idle loop now costs nothing, so the leak stops being
expensive — but it is still a leak, and it is still unfixed. #17355 stays open
for that half rather than being closed on a partial delivery.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 90f7a03e-8529-483c-930d-167b2f4c7641

📥 Commits

Reviewing files that changed from the base of the PR and between 1174bd2 and 702983b.

📒 Files selected for processing (2)

• autobot-backend/services/simple_pty.py
• autobot-backend/services/simple_pty_write_loop_17355_test.py

 __________________________________________________________
< Nice abstraction. Shame about the reality underneath it. >
 ----------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Correcting the provenance of my own evidence, before a reviewer has to.

The Verification table originally presented 845,748 calls in 0.4s and "each mutation fails" as if they were verification. They are local runs, and per this repo's rule a local result is not CI's result — for a mutation there is no sanctioned run at all, because a mutated config cannot be pushed to get one.

The body now separates the two explicitly. Nothing in the fix changes: the argument was always built on the source (get_nowait() + continue with no wait in the loop body; three paths that stop the loop without a sentinel), which is checkable by reading the diff. The call count was only ever illustrating scale, and it should not have been sitting in a column headed Verification.