Skip to content

fix: clean up session operation queues - #2140

Merged
baixiaohang merged 2 commits into
agent-team-foundation:mainfrom
Coooder-Crypto:fix/session-operation-queue-cleanup
Aug 3, 2026
Merged

fix: clean up session operation queues#2140
baixiaohang merged 2 commits into
agent-team-foundation:mainfrom
Coooder-Crypto:fix/session-operation-queue-cleanup

Conversation

@Coooder-Crypto

@Coooder-Crypto Coooder-Crypto commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the per-socket session tail map with a keyed operation queue that compares and removes the same wrapped promise
  • retain live tails across WebSocket close so already-dispatched handlers cannot bypass per-session FIFO ordering
  • add regression coverage for FIFO ordering, arbitrary chat-key cleanup, and the close/resume race

Why

The previous map stored next.finally(...) but compared it with the unwrapped next. That comparison could never succeed, so every historical (agentId, chatId) key remained for the socket lifetime.

Completed keys now reclaim themselves during long-lived connections. In-flight tails are deliberately retained through close until they settle, preserving ordering for handlers that passed their route check before close; the queue is then collectible with its socket owner.

Closes #1722.

Validation

  • pnpm check
  • pnpm typecheck
  • 24 focused queue and WebSocket tests

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

All contributors are covered by the First Tree CLA.
Posted by the CLA Assistant Lite bot.

@Coooder-Crypto

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Coooder-Crypto
Coooder-Crypto marked this pull request as ready for review August 3, 2026 07:41
github-actions Bot added a commit that referenced this pull request Aug 3, 2026

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at 3901798. The keyed queue preserves per-session FIFO ordering, removes only the matching completed tail, and releases socket-owned keys on close. The regression tests cover ordering, arbitrary-key reclamation, and owner cleanup. No blockers found.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: request changes

  • Rationale: clearing the queue on socket close can let an already-dispatched message handler bypass an in-flight operation for the same session, breaking the FIFO guarantee this queue exists to preserve.

Risk level: B-high

  • Path baseline: packages/server/** outside the listed high-risk services -> B-low
  • Semantic lift: ws-client.ts owns core per-session state/event ordering -> B-high

PR summary

  • Author / repo: Coooder-Crypto / agent-team-foundation/first-tree
  • Problem: long-lived Client WebSocket connections retain every historical (agentId, chatId) queue key, so normal session churn and arbitrary chat ids grow per-socket memory indefinitely.
  • Approach: extract the per-session promise tail into a keyed queue that deletes the exact wrapped tail after it settles, and clear the queue when the owning socket closes.
  • Impacted modules: server agent WebSocket session processing and a new keyed async-operation utility.

Review findings
❌ 1. sessionOpQueue.clear() can split one session's FIFO while message callbacks are still in flight. Every session message first awaits ensureAgentStillRoutedHere(), whose local-binding check happens before its database await. The socket can close during that await, clear the queue, and leave an earlier same-key operation running. If the database check then returns true, the session:event path does not re-check boundAgents before calling chainSessionOp, so it starts from an empty tail and can overlap or reorder with the earlier operation. This reintroduces the exact persistence/cleanup race that the FIFO is meant to prevent. Please either avoid clearing live tails (completed keys now reclaim themselves, and the per-socket queue becomes collectible with its owner) or make closure reject/serialize late enqueues safely, and add a WebSocket-level regression that holds one same-key operation open while a pre-close message handler resumes after close. [packages/server/src/api/agent/ws-client.ts:2151]

Action taken

  • Submitted request changes.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correction to my earlier approval: the close-time sessionOpQueue.clear() introduces a FIFO-breaking race. A session:event handler can pass the pre-await local route check, pause in ensureAgentStillRoutedHere(), then resume after close has cleared both boundAgents and the queue. Because that branch does not require boundInfo before calling chainSessionOp, it starts from an empty tail and can overlap an older same-session operation. Please either retain live tails until they settle (the owner and queue are collectible afterward) or make close reject/serialize late enqueues, with a WebSocket-level regression for this interleaving.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at 1e58b80. The close-time clear() and the unsafe queue API are removed, so live same-session tails remain serialized while completed keys still reclaim themselves. The new WebSocket regression covers a session event whose route check resumes after close and verifies it waits for the blocked prior state operation. The previous blocker is resolved; no new blockers found.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: approve

  • Rationale: the revised implementation reclaims settled per-session queue keys without clearing live tails, so it fixes the leak while preserving same-session FIFO across socket close.

Risk level: B-high

  • Path baseline: packages/server/** outside the listed high-risk services -> B-low
  • Semantic lift: ws-client.ts owns core per-session state/event ordering -> B-high

PR summary

  • Author / repo: Coooder-Crypto / agent-team-foundation/first-tree
  • Problem: long-lived Client WebSocket connections retain every historical (agentId, chatId) queue key, causing per-socket memory growth.
  • Approach: replace the faulty inline tail map with one keyed queue that removes only its matching settled tail, while retaining in-flight tails across close until already-dispatched handlers finish.
  • Impacted modules: server agent WebSocket session processing and keyed async-operation utility.

Review findings
✅ 1. The prior FIFO-breaking close-time clear() has been removed.
✅ 2. Settled keys still reclaim themselves without introducing a second queue path.
✅ 3. The WebSocket regression covers a route check resuming after close and verifies that it remains behind the prior same-session operation.

Action taken

  • Approved exact head 1e58b8065aec5ccfc4140d2af0c45404355d919e with maintainer authorization.

@baixiaohang
baixiaohang merged commit a729b63 into agent-team-foundation:main Aug 3, 2026
15 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PERF-059][Medium] Session operation queue entries are never removed

3 participants