Skip to content

fix(server): buffer ACP session updates received before session registration - #3324

Open
desmond-rai wants to merge 1 commit into
getpaseo:mainfrom
desmond-rai:fix/acp-session-notification-race
Open

fix(server): buffer ACP session updates received before session registration#3324
desmond-rai wants to merge 1 commit into
getpaseo:mainfrom
desmond-rai:fix/acp-session-notification-race

Conversation

@desmond-rai

@desmond-rai desmond-rai commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Problem

An ACP agent may emit session-scoped notifications immediately after the session/new response - before the client's response continuation has assigned sessionId (ACPAgentSession.initializeNewSession). sessionUpdate() silently drops those notifications because params.sessionId !== this.sessionId while this.sessionId is still null.

Concrete user-visible failure: an ACP agent that pushes its available_commands_update right after session/new (Hermes advertises installed skills as slash commands this way) never has its command batch cached, so listCommands() returns [] forever and the slash-command popup stays empty for the whole session. The same drop eats usage_update and any other early notification.

Observed on a live daemon log at session creation:

session/update → available_commands_update → dropped (session id not yet assigned)
list_commands_response → commands: []

Fix

Buffer session updates that arrive while sessionId is still unassigned (capped at 100 per session) and replay them in arrival order immediately after sessionId is assigned in initializeNewSession. Updates addressed to a different session id are still discarded, and post-registration routing is unchanged.

Tests

Three new regression tests in acp-agent.test.ts (ACPAgentSession pre-registration session updates):

  • available_commands_update delivered synchronously with the session/new response is applied and returned by listCommands() (fails before the fix: returns [])
  • multiple buffered updates replay in arrival order
  • buffered updates addressed to a different session id are ignored

Verification

  • Red-green: the two delivery tests fail on the base commit, pass with the fix
  • acp-agent.test.ts: 99/99 passed
  • packages/server typecheck: clean
  • Full workspace lint/format/typecheck via lefthook pre-commit: green
  • Full server unit suite (npm run test:unit): 4768 passed, 11 failed in unrelated Hub/websocket/git-service files. Reconciliation: 8 of 11 are 30s+ timeouts under parallel-file load and pass in isolation; workspace-service-port-allocator fails identically on the clean base commit (pre-existing); workspace-git-service.observation is load-flaky (passes on isolated rerun). None import or exercise ACP provider code.
  • Provider e2e suites requiring live CLIs (codex/opencode/claude auth) not run; unrelated to this change

Note: the same race exists one layer up in the shared-process router proposed in #3194; a companion commit there covers that path.

@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR buffers up to 100 ACP session notifications received before session registration and replays matching notifications in arrival order.

  • Adds a pre-registration notification queue to ACPAgentSession.
  • Replays matching notifications after assigning the new session ID.
  • Adds regression coverage for command updates, ordering, and mismatched session IDs.

Confidence Score: 4/5

The PR is not yet safe to merge because the session/new snapshot can still overwrite newer mode and configuration updates replayed from the buffer.

Buffered current_mode_update and config_option_update notifications mutate and emit newer state before applySessionState writes the older response snapshot over the same fields without a corrective event.

Files Needing Attention: packages/server/src/server/agent/providers/acp-agent.ts

Important Files Changed

Filename Overview
packages/server/src/server/agent/providers/acp-agent.ts Adds bounded buffering and replay for pre-registration ACP notifications; the previously reported replay-versus-snapshot ordering issue remains.
packages/server/src/server/agent/providers/acp-agent.test.ts Adds focused behavioral tests for early command updates, replay order, and mismatched session filtering.

Sequence Diagram

sequenceDiagram
  participant A as ACP agent
  participant S as ACPAgentSession
  A-->>S: session/update before registration
  S->>S: Buffer notification
  A-->>S: session/new response
  S->>S: Assign sessionId
  S->>S: Replay matching notifications
  S->>S: Apply session response state
Loading

Reviews (2): Last reviewed commit: "fix(server): buffer ACP session updates ..." | Re-trigger Greptile

Comment thread packages/server/src/server/agent/providers/acp-agent.ts
…tration

ACP agents may emit session-scoped notifications immediately after the
session/new response - before the client's response continuation has
assigned the session id. ACPAgentSession.sessionUpdate dropped those
updates silently because params.sessionId did not yet match, so an agent
that pushes its available_commands_update right after session/new (for
example Hermes, which advertises installed skills as slash commands)
never had its commands cached and the slash-command popup stayed empty.

Buffer up to 100 pre-registration session updates and replay them in
arrival order once sessionId is assigned; updates addressed to a
different session id are still discarded.
@desmond-rai
desmond-rai force-pushed the fix/acp-session-notification-race branch from 09babda to f0fac80 Compare August 13, 2026 13:18
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.

1 participant