Skip to content

fix: Clearer error message when connection is broken before messages are sent#89

Merged
benbrandt merged 4 commits intoagentclientprotocol:mainfrom
anaslimem:fix-rpc-request-hang-on-send-failure
Mar 28, 2026
Merged

fix: Clearer error message when connection is broken before messages are sent#89
benbrandt merged 4 commits intoagentclientprotocol:mainfrom
anaslimem:fix-rpc-request-hang-on-send-failure

Conversation

@anaslimem
Copy link
Copy Markdown
Contributor

Summary

Fix: Return error immediately when connection closes before request is sent

Problem

When the connection to the agent closes before a request can be sent, the caller would hang forever waiting for a response that would never arrive.

Solution

Changed the request flow to try sending FIRST, and only insert into pending responses if successful. If send fails, return an error immediately instead of creating orphan state.

Changes

  • src/agent-client-protocol/src/rpc.rs: Modified request() to send before creating pending response state
  • Added .boxed() to both async branches to handle type unification

Before vs After

Before After
Insert pending → Try send → Remove on failure Try send → Return error if failed → Insert pending
Caller hangs forever Caller gets clear error message

Comment on lines +106 to +131
{
self.pending_responses.lock().unwrap().remove(&id);
return async move {
drop(rx.await);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we still wait for the receiver in this case if we just drop the output anyway?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that's unnecessary It was leftover from an earlier attempt since we check send failure before inserting into pending_responses there's no sender that could ever complete the channel
We can just return the error directly

Copy link
Copy Markdown
Member

@benbrandt benbrandt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I made the control flow able to return earlier. So we don't need to box an async move around an error.

But I also wonder how meaningfully this changed the behavior in hindsight... We already got a disconnected error in the await, and we never left things in pending_responses if it failed to send anyway....

@benbrandt
Copy link
Copy Markdown
Member

Like... We only removed from the pending response hashmap if we got a response from the other side, am I wrong?

@anaslimem
Copy link
Copy Markdown
Contributor Author

You're right that we remove from pending_responses on send failure (not just on response received) so the behavior is the same just with a more accurate error message now.

@benbrandt benbrandt changed the title fix avoid hanging forever when send fails fix: Clearer error message when connection is broken before messages are sent Mar 28, 2026
@benbrandt benbrandt enabled auto-merge (squash) March 28, 2026 06:22
@benbrandt benbrandt merged commit 116f378 into agentclientprotocol:main Mar 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants