daemon: never run the detach handshake under the shells lock - #413
Open
dob323 wants to merge 1 commit into
Open
Conversation
This was referenced Aug 17, 2026
ethanpailes
reviewed
Aug 17, 2026
ethanpailes
left a comment
ethanpailes
left a comment
Contributor
There was a problem hiding this comment.
Looks pretty good, can you rebase against HEAD? My comments are mostly just nits.
Thanks for the fix!
| // immediately stop the client so nothing drains. | ||
| attach_proc.run_cmd("yes | head -c 8000000; echo flood-done")?; | ||
| let client_pid = attach_proc.proc.id().to_string(); | ||
| let stopped = Command::new("kill") |
Contributor
There was a problem hiding this comment.
Lets use https://docs.rs/nix/0.31.3/nix/sys/signal/fn.kill.html instead
| // Both halves are bounded, matching the session-message detach path. | ||
| // A session that cannot complete the handshake in time is reported as | ||
| // not attached rather than being allowed to stall the daemon. | ||
| let mut detached_sessions = vec![]; |
Contributor
There was a problem hiding this comment.
nit: Vec::with_capacity(targets.len())
| // publish; we just update the lifecycle state eagerly so a concurrent | ||
| // list() reflects the detach immediately. | ||
| if !detached_sessions.is_empty() { | ||
| let _s = span!(Level::INFO, "timestamp_lock(shells)").entered(); |
Contributor
There was a problem hiding this comment.
why "timestamp_lock"? I think "hook_lock" would be a better description
client_connection/client_connection_ack are rendezvous channels, so the exchange only completes while the shell->client thread sits in its select loop. A client whose socket stopped draining (a stalled ssh window, a suspended laptop) leaves that thread blocked in write(), and handle_detach ran the exchange while still holding the global shells lock -- one unresponsive client wedged every list, attach, detach and kill in the daemon. Resolve names to Arc'd ctl handles under the lock, drop it, then run each handshake with the same bounded send/recv timeouts the session-message detach path already uses. A session that cannot complete the handshake is reported as not attached instead of being allowed to stall the daemon. Regression test: stop the attach client with SIGSTOP, flood the socket buffers, detach. On unpatched master the daemon wedges and the test times out; patched, detach answers and a follow-up list completes.
dob323
force-pushed
the
fix/detach-shells-lock
branch
from
August 17, 2026 20:15
60cc844 to
455150a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
client_connection/client_connection_ackexchange is a rendezvous, so it only completes while the shell->client thread sits in its select loop. A client whose socket has stopped draining (a stalled ssh window, a suspended laptop) leaves that thread blocked inwrite(), andhandle_detachran the exchange while still holding the global shells lock — one unresponsive client wedged every list, attach, detach and kill in the daemon.This resolves the requested names to
Arcd ctl handles under the shells lock, drops it, then runs each handshake with the same bounded send/recv timeouts the session-message detach path already uses. A session that cannot complete the handshake in time is reported as not attached instead of being allowed to stall the daemon.Regression test
regression::detach_of_stalled_client_does_not_wedge_daemon: SIGSTOP the attach client, flood the socket buffers, detach. It times out on master and passes with the patch.Found by stress-testing shpool 0.11.0 under a session manager that drives many concurrent sessions; we have been running this fix in production for a week. AI disclosure, as requested on #406: drafted with an AI assistant, reviewed and tested by a human before submission.