bug(pty): simple_pty's write loop spins with no wait, so every leaked session burns a core

What

autobot-backend/services/simple_pty.py runs two background threads per session. They handle waiting very differently, and only one of them waits at all:

def _read_loop(self) -> None:
    while self.running and self.master_fd is not None:
        ...
        if poll_readable(fd, _READ_POLL_TIMEOUT_MS):   # bounded wait

def _write_loop(self) -> None:
    """Background thread to write to PTY"""
    while self.running and self.master_fd is not None:
        try:
            text = self.input_queue.get_nowait()       # returns immediately
            ...
        except queue.Empty:
            continue                                   # ... straight back round

The read loop waits up to _READ_POLL_TIMEOUT_MS per iteration, so an idle session costs almost nothing. The write loop calls get_nowait() and continues on queue.Empty with no sleep, no timeout and no blocking get anywhere in the loop body — verified: zero occurrences of sleep, timeout= or a blocking get( between the two loop definitions. An idle write loop therefore spins at 100% of a core for as long as the thread lives.

The docstring says "Wait for input without timeout". get_nowait() is the opposite of waiting — the intent is stated and not implemented.

Why it matters beyond one core

Threads that outlive their session multiply it. A faulthandler dump from a CI shard that hit its 60-minute ceiling (job 107398043182, three dumps at 23:20:48, 23:48:48, 00:00:30) shows a population of these threads still alive:

Thread-N (_read_loop):  autobot_shared/fd_poll.py:59 in poll_readable
                        autobot-backend/services/simple_pty.py:211 in _read_loop
Thread-N (_write_loop): autobot-backend/services/simple_pty.py:233/245 in _write_loop

Each surviving _write_loop is a spinning thread. On a 2–4 core GitHub-hosted runner, a handful of them starves whatever else shares the VM.

What this does and does not explain

Supports: that shard's 6× slowdown. Measured on a healthy completed run, shard 10 takes 11m37s; the timed-out job ran past 60 minutes. These are GitHub-hosted runners and timeout-minutes counts from job start, so cross-PR contention shows up as queueing, not as slower execution on a dedicated VM. Spinning threads inside the job do explain slower execution.

Does not establish: that the collection frames in the same dump (repo_tests/collected_test_model.py) were blocked on these threads. A dump shows state, not a dependency edge. CPU starvation does not need a dependency edge, which is why the mechanism above stands on the source rather than on the dump.

Suggested fix

Replace the spin with a blocking wait that still notices shutdown:

text = self.input_queue.get(timeout=_WRITE_WAIT_TIMEOUT_S)

queue.Empty then means "nothing arrived within the budget" instead of "nothing was there this instant", the loop re-checks self.running each budget, and an idle session costs nothing. Whoever takes this should also look at 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.

Provenance

Surfaced from #16516's faulthandler diagnostic during a CI hang investigation on PR #17341; the dump was obtained by autobot-ai-f8, the source analysis is mine. Not caused by that PR — its head was unchanged and five other runs on the same commit completed.

This is a production defect, not a CI one — and it does not need a leak to cost anything

Verified after autobot-ai-f8 traced reachability. Two corrections to how the original report framed it, both making the issue sharper rather than milder.

1. It is reachable from live paths, not test scaffolding. Six production modules construct it:

autobot-backend/api/terminal.py
autobot-backend/api/terminal_handlers.py
autobot-backend/services/pty_command.py
autobot-backend/services/agent_terminal/command_executor.py
autobot-backend/services/agent_terminal/service.py
autobot-backend/services/agent_terminal/session_manager.py

and simple_pty.py:187-190 starts one writer_thread per session, daemon=True:

self.reader_thread = threading.Thread(target=self._read_loop, daemon=True)
self.writer_thread = threading.Thread(target=self._write_loop, daemon=True)

2. The cost is unconditional, and the original framing under-stated it by hanging it on leaked threads. cleanup() (:369-371) does set self.running = False, so a properly closed session's threads exit at their next loop check. That means:

While a session is alive and idle, its write loop burns 100% of a core. No leak required. A user with a terminal open and typing nothing costs a core continuously, and n idle sessions cost n cores.
Accumulation past session end requires cleanup() not to run — an abandoned session, an exception on the close path, or a test that constructs a PTY and never cleans up. That is the likely shape of the population seen in the CI dump, but it is a second, separate problem.

So the severity does not rest on the leak at all: the steady-state cost of an idle terminal session is one saturated core. The leak only multiplies something already wrong.

One irony worth recording for whoever fixes it: the spin is what makes shutdown responsive. Because the loop never blocks, it notices self.running = False on its very next iteration. A naive fix that swaps get_nowait() for an unbounded get() would remove the burn and introduce a shutdown hang. The fix has to be get(timeout=...) — bounded — so the loop keeps re-checking self.running each budget. That is precisely why the docstring's stated intent ("Wait for input without timeout") is not merely wrong but dangerous as a specification: implemented literally, it hangs close.

Unchanged from the original report: the mechanism for the CI 6× slowdown still rests on the source rather than on the dump, and the dump still does not establish that collection was blocked on these threads. Two independent facts pointing the same way, not one overclaimed chain.

For whoever fixes this: the obvious fix passes the obvious test and hangs in the field

cleanup() does two things, and the second one is what makes a naive fix look correct:

def cleanup(self) -> None:
    self.running = False

    # Signal writer thread to stop
    try:
        self.input_queue.put(None)
    except Exception as e:
        logger.debug("Failed to signal writer thread: %s", e)

So a plain blocking self.input_queue.get() would work when cleanup() runs — the None sentinel wakes it. A fixer who reads cleanup(), sees the sentinel and concludes "shutdown is signalled, a blocking get is fine" has consulted a real mechanism that honestly answers "does cleanup wake the thread?" while the question that matters is "does every path that stops this loop wake the thread?".

It does not, for three reasons — and each one hangs a thread forever under a plain get():

The sentinel is best-effort. The put sits inside a try/except that swallows the failure at logger.debug level. running = False is already set by then, so on that path the loop is told to stop and never woken to notice.
An abandoned session never calls cleanup() at all. Nothing else sends the sentinel. This is also the shape that produced the thread population in the CI faulthandler dump.
The loop condition has a second term. It is while self.running and self.master_fd is not None, so fd invalidation is also meant to stop it — and a thread parked in get() never returns to re-evaluate either term.

That is why the fix must be bounded:

text = self.input_queue.get(timeout=_WRITE_WAIT_TIMEOUT_S)

The loop then re-checks both conditions every budget regardless of what the caller did or failed to do, and queue.Empty means "nothing arrived in that budget" rather than "nothing was there this instant".

And the docstring has to change with it. "Wait for input without timeout" is not merely inaccurate about the present — implemented faithfully it produces exactly the hang described above. It is a specification that breaks close. A fix that leaves it in place hands the next reader the same trap in the same words.

Credit where due: the sentinel's role in making the naive fix look safe was spotted by autobot-ai-f8; the swallowing try/except and the second loop term are the parts that make it hang anyway.

The spin is load-bearing for a documented shutdown contract — the complete hang set

cleanup() ends like this (simple_pty.py:403-409):

        # Wait for threads to complete naturally
        if self.reader_thread and self.reader_thread.is_alive():
            # Signal shutdown via running flag, thread will exit naturally
            pass  # Thread will exit when running=False
        if self.writer_thread and self.writer_thread.is_alive():
            # Thread will exit when running=False and queue is processed
            pass

Two pass statements whose entire justification is a comment asserting the thread will notice running = False. There is no join() anywhere — the section is headed "Wait for threads to complete naturally" and does no waiting at all; cleanup() returns immediately and the threads are left to notice on their own.

That assertion is true today only because the write loop never blocks. The spin is not an incidental inefficiency — it is what makes a documented shutdown contract hold. And the writer's comment says the thread exits "when running=False and queue is processed", which get_nowait() satisfies trivially and a blocking get() does not.

So the trap has a fourth lure, and it is the worst-placed one: a fixer who asks "is shutdown handled?" finds an explicit comment at the exact site their change breaks, saying yes.

The complete hang set under a naive blocking `get()`

#	Path	Why the thread never wakes
1	Swallowed sentinel	`cleanup()`'s `input_queue.put(None)` sits in a `try/except → logger.debug`; `running = False` is already set, so the loop is told to stop and never woken
2	Abandoned session	nothing calls `cleanup()`, so no sentinel is ever sent — the shape that produced the thread population in the CI dump
3	fd invalidation	the condition is `while self.running and self.master_fd is not None`; a thread parked in `get()` re-evaluates neither term
4	The `pass` contract above	`cleanup()` does not `join()`, and its comments promise an exit that only a non-blocking loop delivers

Only get(timeout=...) survives all four, because it re-checks both loop conditions every budget regardless of what the caller did, failed to do, or documented.

What a complete fix therefore includes

self.input_queue.get(timeout=_WRITE_WAIT_TIMEOUT_S) — bounded, not unbounded.
Rewriting "Wait for input without timeout", which is a specification that breaks close if followed.
Rewriting the two pass comments, or replacing them with an actual join(timeout=...) — a documented wait that does not wait is how the spin became load-bearing without anyone deciding it should be.

Verified sites: only two running = False exist — :90 (__init__) and :371 (cleanup), so cleanup() is the sole deliberate stop. The pass block was found by autobot-ai-f8.

fix(pty): stop the write loop spinning, without introducing the shutd…

The spin half is fixed; the leak half is now located, not hypothetical

PR #17367 fixes the spin only, so this issue stays open — it asks for two things
and the second was "why the threads outlive their session at all".

Review of that PR turned the second half from a suspicion into a coordinate.

The abandoned-session path is real, and here it is

autobot-backend/services/agent_terminal/session_manager.py:429-453 —
close_session() deletes the AgentTerminalSession bookkeeping entry and the
Redis key, and never calls simple_pty_manager.close_session() or
pty.cleanup(). It is reached from service.py:123-126.

The call it is missing exists elsewhere in the same package:
command_executor.py:107-110 (_force_close_pty_session) does call it. So this
is not a facility that was never built — it is one call path that does not use
it.

Consequence: the session's bookkeeping disappears while its PTY threads keep
running, with nothing left holding a reference that would ever set
running = False. That is the leak.

What #17367 changed about it, precisely

	before	after
an abandoned write loop costs	~2,100,000 wakes/sec (a full core)	10 wakes/sec
does it still leak	yes	yes

The thread still lives forever. It is now almost free instead of ruinous, which
is why the spin was worth fixing separately — but a leak that got cheap is still
a leak, and a long-lived process accumulates them.

Also verified while there

master_fd is assigned in exactly two places: simple_pty.py:88 (__init__,
to None) and :408 (cleanup(), to None). Nothing else ever sets it. So
the loop's master_fd is not None condition is signalled only by a cleanup()
that the abandoned path never calls — the two halves of this issue are the same
gap seen from two directions.

Remaining work for this issue

session_manager.close_session() releases the PTY, not just the
bookkeeping — via the existing _force_close_pty_session path rather than
a second implementation.
A test that an abandoned session's threads actually terminate; asserting
close_session() was called is not the same claim.
Audit the other AgentTerminalSession teardown paths for the same
omission, rather than fixing only the one the review happened to reach.

Line-number pin, so the two readings above do not look like a disagreement.

My earlier comment cited simple_pty.py:88 and :408; those are on PR #17367's branch, which adds lines above them. On merged main at 554a15b9cd the same two assignments are:

81:        self.master_fd = None
385:            self.master_fd = None

Verified with git show origin/main:autobot-backend/services/simple_pty.py. Two assignments, both to None, both in code the abandoned-session teardown path never reaches — the claim is unchanged, only the coordinates move with the branch. Cite the SHA alongside a line number in this file until #17367 lands, after which main's numbers become the PR branch's.

fix(pty): stop the write loop spinning, without introducing the shutd…

Spin half delivered — PR #17367, squash-merged as `d8561e36c4`

This issue stays open. It asks for two things and this delivers one, which
is why the PR carried Refs rather than Closes and why no merge keyword closed
it.

Verified against merged `main` @ `d8561e36c4`

 41:  _WRITE_WAIT_TIMEOUT_S = TimingConstants.MICRO_DELAY
256:  text = self.input_queue.get(timeout=_WRITE_WAIT_TIMEOUT_S)

MICRO_DELAY is 0.1 (autobot_shared/ssot_constants.py:580), so:

	idle wake rate
before	~2,100,000 / sec (845,748 measured in 0.4s)
after	10 / sec

The wait is bounded rather than blocking, deliberately: three paths stop this
loop without a sentinel, and a plain get() parks forever on all three. It adds
no keystroke latency — get(timeout=...) returns the instant an item is queued;
the 100ms bounds only the idle wake and how long after running = False the
thread notices.

Three tests landed with it (simple_pty_write_loop_17355_test.py), each
mutation-checked against both regressions: restoring get_nowait() fails the
spin test at ~845k calls, and switching to a plain blocking get() fails both
exit tests by parking past join(timeout=2.0).

Not a leftover: output_queue.get_nowait() survives at :322. Different
queue, different job — a non-blocking poll for output, correct as it stands.
Only the input loop was spinning.

What remains, and it is now a coordinate rather than a suspicion

The thread leak. services/agent_terminal/session_manager.py close_session()
drops the bookkeeping entry and the Redis key and never releases the PTY, so
nothing is left to set running = False. Full detail and criteria in the
comment above; the line numbers there are pinned to 554a15b9cd and shift by
main's current offset.

The spin fix changes the cost of that leak, not its existence:

	before	now
an abandoned write loop costs	a full core	10 wakes/sec
does it still leak	yes	yes

A leak that got cheap is still a leak, and a long-lived process accumulates them.

Filed separately from the same review

#17381 — three pre-existing defects in simple_pty.py, none introduced
here: os.write on a non-blocking fd raising a retryable BlockingIOError is
caught by a broad except and permanently ends the write loop; os.write's
return value is discarded so a large paste truncates silently, possibly
mid-UTF-8; plus a TOCTOU in cleanup() and MICRO_DELAY serving two unrelated
timings.

Closed — fixed with a bounded wait and a dedicated guard, verified at `ba03b68c74`

autobot-backend/services/simple_pty.py:304 _write_loop now waits:

text = self.input_queue.get(timeout=_WRITE_WAIT_TIMEOUT_S)

The get_nowait() hot spin is gone — the defect was one saturated core per live
terminal on an idle session, whether or not anything ever leaked.

The docstring at :305-318 is the part worth preserving, because it rules out the
obvious alternative fix. A plain blocking get() looks correct against cleanup(),
which does send a None sentinel — and it would park forever on three paths that
stop this loop without one: the sentinel's put sits inside a swallowing try/except
after running = False is already set; an abandoned session never calls cleanup();
and the loop's second condition, master_fd is not None, is signalled by nothing. The
bounded timeout is what lets both loop conditions be re-checked regardless of what the
caller did or failed to do.

That is the tonight-recurring shape — the plausible repair destroys the property — caught
before it shipped and written down at the site rather than in a review thread.

Guarded: autobot-backend/services/simple_pty_write_loop_17355_test.py exists and
names the issue, so the spin cannot return unnoticed. Every issue closed cleanly in this
pass shipped its fix with a standing check; the ones that shipped a fix alone are the
ones still open.

No remainder.

Navigation Menu

Repository navigation

bug(pty): simple_pty's write loop spins with no wait, so every leaked session burns a core #17355

What

Why it matters beyond one core

What this does and does not explain

Suggested fix

Provenance

This is a production defect, not a CI one — and it does not need a leak to cost anything

For whoever fixes this: the obvious fix passes the obvious test and hangs in the field

The spin is load-bearing for a documented shutdown contract — the complete hang set

The complete hang set under a naive blocking `get()`

What a complete fix therefore includes

The spin half is fixed; the leak half is now located, not hypothetical

The abandoned-session path is real, and here it is

What #17367 changed about it, precisely

Also verified while there

Remaining work for this issue

Spin half delivered — PR #17367, squash-merged as `d8561e36c4`

Verified against merged `main` @ `d8561e36c4`

What remains, and it is now a coordinate rather than a suspicion

Filed separately from the same review

Closed — fixed with a bounded wait and a dedicated guard, verified at `ba03b68c74`

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

bug(pty): simple_pty's write loop spins with no wait, so every leaked session burns a core #17355

Description

What

Why it matters beyond one core

What this does and does not explain

Suggested fix

Provenance

Activity

mrveiss commented on Sep 24, 2026

This is a production defect, not a CI one — and it does not need a leak to cost anything

mrveiss commented on Sep 24, 2026

For whoever fixes this: the obvious fix passes the obvious test and hangs in the field

mrveiss commented on Sep 24, 2026

The spin is load-bearing for a documented shutdown contract — the complete hang set

The complete hang set under a naive blocking get()

What a complete fix therefore includes

mrveiss commented on Sep 24, 2026

The spin half is fixed; the leak half is now located, not hypothetical

The abandoned-session path is real, and here it is

What #17367 changed about it, precisely

Also verified while there

Remaining work for this issue

mrveiss commented on Sep 24, 2026

mrveiss commented on Sep 24, 2026

Spin half delivered — PR #17367, squash-merged as d8561e36c4

Verified against merged main @ d8561e36c4

What remains, and it is now a coordinate rather than a suspicion

Filed separately from the same review

mrveiss commented on Oct 3, 2026

Closed — fixed with a bounded wait and a dedicated guard, verified at ba03b68c74

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

The complete hang set under a naive blocking `get()`

Spin half delivered — PR #17367, squash-merged as `d8561e36c4`

Verified against merged `main` @ `d8561e36c4`

Closed — fixed with a bounded wait and a dedicated guard, verified at `ba03b68c74`