daemon: treat heartbeat ack timeout like the send timeout - #406
Conversation
|
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. |
|
Please disclose your AI usage, sign the CLA, and include a regression test in the shpool/tests/regression tree. |
aec29d4 to
64f6385
Compare
|
@googlebot I signed it! |
|
Looks like the CLA bot is still grumpy. If an agent harness made the commit for you, sometimes they leave author droppings in the commits that interfear with the CLA check. You might want to make sure that the commits have the email address you've got associated with your github account and nothing else listed for the author. I didn't see any sort of "co-authored by claude" when I clicked into the commit but maybe it's not visible. |
|
Also, this needs a new case in the regression test suite. |
de3f8d9 to
307f3be
Compare
|
Sorry, I missed your test requests from Friday. All three have tests now:
Each one fails on master and passes with its patch. Writing the test found a real problem with this patch. The test stalls the shell->client thread past the ack deadline using a new |
| // is still parked in send(). Take that stale ack before | ||
| // asking for a fresh heartbeat, or the two threads never | ||
| // line up again. | ||
| let _ = shell_to_client_ctl.heartbeat_ack.try_recv(); |
There was a problem hiding this comment.
This is not the right fix here. I think this comment is diagnosing a real problem, but just unconditionally consuming an ack before sending a request is not a good approach. If another thread is in the middle of and RPC sequence (although maybe this is fine because of the shell_to_client_ctl lock), we could steal their ack, but also there could be arbitrarily many queued acks that we need to deal with.
Probably what we should be doing instead is have an atomic counter in the controller that callers peel off of to send on the channel rpcs as an extra request_id field and that the shell to client thread must echo back in the ack. When reading acks we can then discard any that don't have a matching request id.
There was a problem hiding this comment.
Right, the blind drain leaned on the single-consumer and ctl-lock invariants instead of making the discard provably safe. Implemented what you described: the controller holds an atomic counter, each heartbeat carries a request_id, the shell->client thread echoes it back in the ack, and reads discard any ack whose id doesn't match. The regression test still fails on master and passes with this.
| } | ||
|
|
||
| /// Scan lines until one matches the given regex, returning its captures. | ||
| pub fn scan_until_captures_re(&mut self, re: &str) -> anyhow::Result<Vec<Option<String>>> { |
There was a problem hiding this comment.
Looks like this is in another patch as well. Let's wait until that guy merges and rebase on top with this guy.
| Ok(()) | ||
| } | ||
|
|
||
| /// A session whose shell has already died while nothing was attached still |
There was a problem hiding this comment.
Oh, actually is it looks like you just got your PRs a bit tangled up here.
There was a problem hiding this comment.
Sorry, that was my branch juggling. Untangled now: this PR only touches shell.rs, server.rs, and the regression test. The kill test and the line_matcher change live only in #405.
307f3be to
7e7650a
Compare
| // line up again. The request id check is what makes | ||
| // discarding safe: only acks for requests we already | ||
| // abandoned can appear here. | ||
| while let Ok((stale_id, _)) = shell_to_client_ctl.heartbeat_ack.try_recv() { |
There was a problem hiding this comment.
We should not need this now that we have the continue at the normal recv point. As I mentioned with my last feedback, this will cause a logical race.
There was a problem hiding this comment.
Dropped it. It did need one more change to work: with the ack channel as a rendezvous channel, the shell->client thread parks in send() with the abandoned ack and never gets back to its select loop, so the next heartbeat send times out too and the session stays wedged. I confirmed that by deleting the drain on its own and watching the regression test fail.
So the ack channel now has one slot of buffer. The shell->client thread deposits the abandoned ack and carries on, and the id check at the recv point discards it on the next pass, which is the structure you asked for with no pre-send drain.
| // a heartbeat to check if the client is still listening. The payload | ||
| // is a request id that must be echoed back in the ack. | ||
| pub heartbeat: crossbeam_channel::Sender<u64>, | ||
| // The request id of the heartbeat this ack answers, and true if the |
There was a problem hiding this comment.
This reads a bit strangely. How about "and a flag that is true if ...", also let's explain the point of the request id.
There was a problem hiding this comment.
Reworded to "and a flag that is true if ...", and the field docs now explain the id: the heartbeat thread gives up waiting for a late ack, so one can still turn up afterwards, and the id is what lets the next read tell that ack apart from its own and discard it.
A shell->client thread that is busy (generating a large session restore buffer is the usual cause) can miss the ack deadline. Failing the heartbeat thread there unwinds the thread scope and takes the session's shell->client thread with it, so the session can never be attached again and whatever runs inside it blocks forever on its next write. A client that is really gone is still caught by the failing write to it. Giving up on an ack means a late one can still arrive, so each heartbeat carries a request id that the ack echoes back and reads discard any ack whose id does not match. The ack channel gets a slot of buffer as well, so the shell->client thread deposits the abandoned ack and returns to its select loop rather than parking in send() forever. Covered by regression::slow_heartbeat_ack_does_not_wedge_session, which stalls the shell->client thread past the ack deadline via a new daemon-wrote-heartbeat test hook.
7e7650a to
2b886f0
Compare
2 participants
Issue Link
(none)
AI Policy Ack
I have read the AI Policy. This patch was written with Claude Code, working from failures in my own shpool deployment. During this review round the first version of the fix proved wrong and the new regression test caught it:
regression::slow_heartbeat_ack_does_not_wedge_sessionfails without the current fix.This PR was:
Description
spawn_heartbeattreats a send timeout as "handler busy" and continues,but the ack timeout on the same 300 ms budget is fatal: it unwinds the
thread scope and destroys the session's
shell->clientthread, afterwhich every attach fails (
error shuffling bytes: attaching new client stream to shell->client thread) and the program inside blocks forever onits next write. A genuinely dead client is still detected by the
client-stream write failure path.
Observed repeatedly in production under load (multi-hour interactive
sessions lost, with the log signature above). Now reproduced
deterministically by the regression test, which stalls the shell->client
thread past the ack deadline via a new
daemon-wrote-heartbeattest hook.Because the ack channel is a rendezvous channel, an ack abandoned by a
timed-out request also has to be taken off the channel later or the two
threads never line up again. Each heartbeat therefore carries a request id
from an atomic counter on the controller; the shell->client thread echoes
it back in the ack, stale acks are discarded by id, and an ack can never
be consumed by a request it does not answer.