fix: clean up session operation queues - #2140
Conversation
|
All contributors are covered by the First Tree CLA. |
|
I have read the CLA Document and I hereby sign the CLA |
yuezengwu
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.tsowns 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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.tsowns 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
1e58b8065aec5ccfc4140d2af0c45404355d919ewith maintainer authorization.
Summary
Why
The previous map stored
next.finally(...)but compared it with the unwrappednext. 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 checkpnpm typecheck