Skip to content

fix(connection): preserve notification response ordering - #129

Open
hallerite wants to merge 2 commits into
agentclientprotocol:mainfrom
hallerite:codex/fix-notification-response-ordering
Open

fix(connection): preserve notification response ordering#129
hallerite wants to merge 2 commits into
agentclientprotocol:mainfrom
hallerite:codex/fix-notification-response-ordering

Conversation

@hallerite

@hallerite hallerite commented Aug 6, 2026

Copy link
Copy Markdown

we had a downstream issue in verifiers where an ACP request could complete before handlers for preceding notifications had finished. we patched it in our codebase by adding a wait, which is not optimal. this is my attempt of upstreaming a better fix.

Summary

  • keep an outgoing request pending until notification handlers for messages received before its response have completed
  • scope the barrier to notifications received after that request began, so notification handlers can still make nested requests without deadlocking
  • record responses immediately before waiting on the barrier, preserving a valid response if the peer closes the transport right afterward

Root cause

Connection publishes notifications to the asynchronous dispatcher, but handles responses directly in the receive loop. A notification followed immediately by its request response could therefore resolve send_request() before the notification handler had updated client state. ACP consumers could observe a completed prompt with its final streamed content still missing.

The response is now stored as soon as it arrives, while send_request() waits on a per-request notification barrier before returning or raising. This keeps wire-order semantics without turning notification dispatch synchronous. Responses to nested requests exclude the notification that initiated them, avoiding a circular wait.

The immediate response storage is important for transport shutdown: an EOF after a valid response can no longer replace that response with ConnectionError while a preceding notification handler is still finishing.

Downstream reproduction: PrimeIntellect-ai/verifiers#2262

Validation

  • make check
  • make test (199 passed, 1 skipped)

@hallerite
hallerite marked this pull request as ready for review August 6, 2026 11:02
@hallerite
hallerite force-pushed the codex/fix-notification-response-ordering branch from 11dcb7d to a966b23 Compare August 6, 2026 11:02
@frostming

Copy link
Copy Markdown
Contributor

Taking this opportunity, I reread the implementation of connection & transport. It has existed since the initial version and was actually translated from TypeScript. I feel it's a bit too complicated. I'm currently discussing with my agent whether it can be simplified, and there should be a refactoring later.

@frostming

frostming commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Please see #132 and check if the issue still exists or you need to update the PR. @hallerite

@hallerite
hallerite force-pushed the codex/fix-notification-response-ordering branch from a966b23 to df4063b Compare August 13, 2026 12:49
@hallerite
hallerite force-pushed the codex/fix-notification-response-ordering branch from df4063b to d07f3e2 Compare August 13, 2026 13:14
@hallerite

Copy link
Copy Markdown
Author

hey @frostming,
rebased my PR onto main. The issue unfortunately still exists.

@frostming

frostming commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

This solution has a problem: you are associating all notifications received after a request is sent with that request. In reality, this is not the case. For example, the session/update notification is only relevant when the session id is the same.

However, to achieve this kind of precise association, the Connection—as a low-level communication abstraction—would need to know too much, which violates the principles of software design.

I feel this is still better suited for the Agent/Client to implement and ensure that, and what we need to provide is a way to tell the caller that the notification has been processed, rather than being completely asynchronous.

What do you think?

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