fix: drop attached clients that stop reading by warelik · Pull Request #436 · shell-pool/shpool

warelik · 2026-09-22T06:29:40Z

Problem

If an attach client stays connected but stops reading, the daemon's shell->client thread blocks in write() with no timeout. The write timeout set in write_reply is cleared before streaming starts. Two things follow:

The shell freezes. Once the pty fills up, everything running in the session blocks on output.
The session can't be taken back. It stays attached. shpool detach and shpool attach -f both time out when they try to hand the disconnect to the blocked thread. attach -f then reports session '...' already has a terminal which remains attached even after attempting to detach it. The only way out is to find and kill the stale shpool attach process by hand.

We hit this with an IDE terminal (Cursor Remote-SSH) that was left behind when the window lost its connection. The IDE's server kept the terminal alive but stopped draining it, so the attach process inside it blocked writing its stdout. The daemon log showed sending client detach to shell->client for <session>: "SendTimeoutError(..)" for every detach attempt. The comment above the detach handler in server.rs already describes this client ("a stalled ssh window, a suspended laptop"): the handler no longer wedges the daemon, but the session itself stays stuck.

Fix

Write timeout. Writes to the attached client get a 10 second socket timeout (CLIENT_WRITE_TIMEOUT). A slow client is not affected, because any progress restarts the timeout. Only a client that accepts nothing for 10 seconds is dropped.
Dropping the client. When a write fails, including on the timeout, the daemon shuts the client socket down instead of only dropping its handle. The client->shell thread and the attach process then see EOF, bidi_stream returns, the session becomes disconnected, and the shell keeps writing into the output spool.
- Before this change, a timed-out heartbeat or MaybeSwitch write fell into the unexpected IO error arm, which ends the shell->client thread. It is now handled like a hangup.
Session restore. The restore dump stops at the first failed chunk. Otherwise each further chunk could wait for another timeout.

10 seconds is a judgment call. It is long enough to ride out a network hiccup and short enough that a frozen shell or a failing attach -f doesn't last. I'm happy to change it or make it configurable.

Testing

New force_attach_stalled_client: floods a session whose attach client never reads its stdout, waits for the session to become disconnected, then force-attaches and checks that it is the same shell and that it still responds. It fails on master (the stalled client was never dropped) and passes with this change in about 11 s.
Full cargo test on master and on this branch. The only failures are the same 4 on both, and they need programs that aren't on my test host: forward_env_live_reload_{zsh,fish} and prompt_prefix_zsh need zsh or fish, output_flood needs hexdump.
A local, uncommitted run where the client reads 4 KiB every 3 s during a flood. The client stayed attached for 33 s, and the daemon logged no timeouts.
cargo +nightly fmt -- --check and cargo clippy --all-targets -- -D warnings are clean.

AI disclosure

This change was made with Claude Code: tracing the hang on a live system, the patch and the test. It was verified by running the tests described above.

🤖 Generated with Claude Code

google-cla · 2026-09-22T06:29:57Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

A client that stays connected but stops draining its socket, e.g. an IDE terminal that was orphaned when its window lost the remote connection, used to park the shell->client thread in write() forever. Once the pty filled up the shell froze, and the session stayed attached: detach and attach -f both time out handing the disconnect to the blocked thread, so the session could only be recovered by finding and killing the stale attach process. Give writes to the client a 10 second timeout. When a write makes no progress for that long, treat the client as gone and shut its socket down, so the client->shell thread and the attach process see EOF, the session becomes disconnected, and the shell keeps running into the output spool. Stop the session-restore dump at the first failed chunk too, since each further chunk could otherwise block for another timeout. force_attach_stalled_client floods a session whose attach client never reads its stdout and checks that the session is released and the shell is still usable. It fails without this change.

ethanpailes

Thanks for finding the bug, this seems like the right approach, just a few things to address.

ethanpailes · 2026-09-22T15:27:24Z

+/// Reports whether a client write failed because it hit
+/// CLIENT_WRITE_TIMEOUT. Depending on the platform, an expired
+/// socket write timeout surfaces as either kind.
+fn is_client_write_timeout(err: &io::Error) -> bool {


This seems like generic functionality, so let's name it accordingly. is_socket_write_timeout would describe what it does better.

ethanpailes · 2026-09-22T15:31:43Z

+        .attach("sh1", AttachArgs { force: true, ..Default::default() })
+        .context("attaching from tty2")?;
+    let mut line_matcher2 = tty2.line_matcher()?;
+    tty2.run_raw(vec![3])?; // ^C the flood


nit: can you give a useful name to this magic number with a constant?

ethanpailes · 2026-09-22T15:33:28Z

+    // output) forever. That takes the daemon's client write timeout,
+    // and the exponential backoff in wait_until_list_matches polls too
+    // sparsely around that point, so poll at a fixed interval instead.
+    let deadline = time::Instant::now() + time::Duration::from_secs(20);


Having the test block for 10s is not great. The test suite already takes a while to run because of all the process juggling, and this guy will immediately become a long pole. To address this, and also make this feature more configurable, let's make this write timeout an option in the config file, and then set it to something short like 1s in this test.

ethanpailes · 2026-09-22T15:34:34Z

+// a stalled ssh window) would otherwise park the shell->client thread in
+// write() forever, freezing the shell once the pty fills up and keeping
+// the session attached so that neither detach nor attach -f can take it.
+const CLIENT_WRITE_TIMEOUT: time::Duration = time::Duration::from_secs(10);


10s seems like a solid default, but let's make this optionally configurable by adding an entry to config.rs. That will also allow the test to run in a more reasonable timeframe.

ethanpailes · 2026-09-22T15:36:13Z

Oh, also, in the future, please don't delete the PR template or use AI to generate a PR description. AI is fine for code, but human communication should still be hand-written in the shpool project. (Though it would be fine to include a block with some prefix like "this is what my agent says about the change")

ethanpailes · 2026-09-22T15:50:52Z

The macos CI env is kinda flaky, so I kicked off another test run.

warelik force-pushed the fix-stalled-client-write-timeout branch from 8ae2796 to 5390fab Compare September 22, 2026 06:30

warelik force-pushed the fix-stalled-client-write-timeout branch from 5390fab to 8c73450 Compare September 22, 2026 06:55

ethanpailes reviewed Sep 22, 2026

View reviewed changes

Navigation Menu

fix: drop attached clients that stop reading - #436

fix: drop attached clients that stop reading#436
warelik wants to merge 1 commit into
shell-pool:masterfrom
warelik:fix-stalled-client-write-timeout

warelik commented Sep 22, 2026

Uh oh!

google-cla Bot commented Sep 22, 2026

Uh oh!

ethanpailes left a comment

Uh oh!

ethanpailes Sep 22, 2026

Uh oh!

ethanpailes Sep 22, 2026

Uh oh!

ethanpailes Sep 22, 2026 •

edited

Loading

Uh oh!

ethanpailes Sep 22, 2026

Uh oh!

ethanpailes commented Sep 22, 2026 •

edited

Loading

Uh oh!

ethanpailes commented Sep 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

warelik commented Sep 22, 2026

Problem

Fix

Testing

AI disclosure

Uh oh!

google-cla Bot commented Sep 22, 2026

Uh oh!

ethanpailes left a comment

Choose a reason for hiding this comment

Uh oh!

ethanpailes Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

ethanpailes Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

ethanpailes Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethanpailes Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

ethanpailes commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanpailes commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ethanpailes Sep 22, 2026 •

edited

Loading

ethanpailes commented Sep 22, 2026 •

edited

Loading

ethanpailes commented Sep 22, 2026 •

edited

Loading