Tear shells down off the event loop (destroy_session froze it) #2

Merged

airaneel merged 1 commit from teardown-off-loop into main

2026-07-18 02:21:56 +03:00

airaneel commented

2026-07-18 02:20:04 +03:00

Owner

destroy_session froze the whole event loop.

It ran sendintr() + cleanup() — signalling the shell and closing its pty — straight from the async handler. Closing a pty is synchronous end to end, so a heartbeat ticking every 10ms got zero ticks through for the entire teardown. On the shared server that freezes every other chat's tool call until the pty is gone. The same synchronous teardown runs when babash_initialize resets a known chat_id (create_chat cleans up the old workspace before building the new one).

Fix

close_shell runs sendintr()+cleanup() in a worker thread; create_chat is dispatched to a thread too.
Spawn gets the same treatment for consistency, though it was never the freeze: the BashState constructor already defers the pty fork to its own thread, so only a process-table scan for orphaned babash screens runs on the caller (~18ms in isolation, but on every spawn, growing with process/screen count on a busy server). spawn_shell moves that off the loop as well.

Honest scoping

Two things I corrected while investigating:

The freeze is teardown, not spawn. An earlier draft claimed spawn blocked the loop for ~1.5s; measurement disproved it (spawn keeps the loop responsive; only teardown froze it — 0 heartbeats). The docstrings and the test now say so.
The forkpty DeprecationWarning is pre-existing, not introduced here. babash has always forked the pty inside BashState._init_thread, so the warning fires on a plain main-thread spawn too. It's inherent to spawning ptys in a threaded process; pexpect forks-then-execs, so the window is pexpect's to manage. Left untouched — suppressing a real warning would hide a real (if low, long-standing) signal.

Guard

test_no_loop_blocking: the teardown assertion fails on the old code (zero heartbeats) and passes now. The spawn assertion is a looser smoke check — spawn never froze the loop outright, so it can't separate old from new as sharply, and the test says that rather than pretending otherwise.

95 tests, mypy --strict and ruff clean.

🤖 Generated with Claude Code

`destroy_session` froze the whole event loop. It ran `sendintr()` + `cleanup()` — signalling the shell and closing its pty — straight from the async handler. Closing a pty is synchronous end to end, so a heartbeat ticking every 10ms got **zero** ticks through for the entire teardown. On the shared server that freezes every other chat's tool call until the pty is gone. The same synchronous teardown runs when `babash_initialize` resets a known `chat_id` (`create_chat` cleans up the old workspace before building the new one). ## Fix - `close_shell` runs `sendintr()`+`cleanup()` in a worker thread; `create_chat` is dispatched to a thread too. - Spawn gets the same treatment for consistency, though it was never the freeze: the `BashState` constructor already defers the pty fork to its own thread, so only a process-table scan for orphaned babash screens runs on the caller (~18ms in isolation, but on every spawn, growing with process/screen count on a busy server). `spawn_shell` moves that off the loop as well. ## Honest scoping Two things I corrected while investigating: - **The freeze is teardown, not spawn.** An earlier draft claimed spawn blocked the loop for ~1.5s; measurement disproved it (spawn keeps the loop responsive; only teardown froze it — 0 heartbeats). The docstrings and the test now say so. - **The `forkpty` DeprecationWarning is pre-existing, not introduced here.** babash has always forked the pty inside `BashState._init_thread`, so the warning fires on a plain main-thread spawn too. It's inherent to spawning ptys in a threaded process; pexpect forks-then-execs, so the window is pexpect's to manage. Left untouched — suppressing a real warning would hide a real (if low, long-standing) signal. ## Guard `test_no_loop_blocking`: the teardown assertion **fails on the old code** (zero heartbeats) and passes now. The spawn assertion is a looser smoke check — spawn never froze the loop outright, so it can't separate old from new as sharply, and the test says that rather than pretending otherwise. 95 tests, `mypy --strict` and `ruff` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

airaneel added 1 commit

2026-07-18 02:20:04 +03:00

fix: tear shells down off the event loop (destroy_session froze it)

Python Test / test (3.11) (pull_request) Has been cancelled

Details

Python Test / test (3.12) (pull_request) Has been cancelled

Details

Python Test / test (3.13) (pull_request) Has been cancelled

Details

Mypy strict / typecheck (3.11) (pull_request) Has been cancelled

Details

Mypy strict / typecheck (3.12) (pull_request) Has been cancelled

Details

Mypy strict / typecheck (3.13) (pull_request) Has been cancelled

Details

fb36f6b6fd

destroy_session ran sendintr()+cleanup() — signalling the shell and closing its
pty — straight from the async handler. Closing a pty is synchronous end to end,
so it stalled the whole event loop: a heartbeat ticking every 10ms got *zero*
ticks through for the entire teardown. On the shared server that freezes every
other chat's tool call until the pty is gone. The same synchronous teardown runs
when babash_initialize resets a known chat_id (create_chat cleans up the old
workspace before building the new one).

close_shell now runs sendintr()+cleanup() in a worker thread, and create_chat is
dispatched to a thread too.

Spawn gets the same treatment, though it was never the freeze: the BashState
constructor already defers the pty fork to its own thread and returns quickly, so
only a process-table scan for orphaned babash screens runs on the caller (~18ms
in isolation, but it runs on every spawn and grows with the number of processes
and stale screens on a busy server). spawn_shell moves that off the loop as well.

Not touched: the forkpty-in-a-thread DeprecationWarning. babash has always forked
the pty inside BashState's _init_thread, so it fires on a plain main-thread spawn
too — it is pre-existing and inherent to spawning ptys in a threaded process, not
introduced here, and pexpect forks-then-execs so the window is pexpect's to manage.

Regression guard (test_no_loop_blocking): the teardown assertion fails on the old
code (zero heartbeats) and passes now. The spawn assertion is a looser smoke check
— spawn never froze the loop outright, so it cannot separate old from new as
sharply, and the test says so rather than pretending otherwise.

mypy --strict and ruff clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

airaneel merged commit d00b6678f0 into main

2026-07-18 02:21:56 +03:00

airaneel deleted branch teardown-off-loop

2026-07-18 02:21:56 +03:00

airaneel referenced this pull request from a commit

2026-07-18 02:21:57 +03:00

Merge pull request 'Tear shells down off the event loop (destroy_session froze it)' (#2) from teardown-off-loop into main