Repository navigation
Send Expert chat messages one at a time - #8876
Conversation
Queue every send to the Expert (chat turns, opening a conversation, approval resumes and the instance ready/failed notices) so only one message is in flight per session. A turn is finished when its reply arrives or it is stopped; Stop only ends the turn in flight and the next queued message goes out. Start Over drops the queue.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/expert-build-tracking #8876 +/- ##
=============================================================
Coverage ? 78.53%
=============================================================
Files ? 475
Lines ? 25753
Branches ? 6853
=============================================================
Hits ? 20226
Misses ? 5527
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Question and plan cards already stop being answerable once anything is posted after them. Approval cards stayed clickable, so a decision could be sent for a call the Expert had already cancelled.
cstns
left a comment
cstns
left a comment
There was a problem hiding this comment.
Nice approach, the queue does fix the double-turn thing. I tried it locally and the instance notice now waits for the reply like it should.
Requesting changes mainly for the approval card getting stuck (comment on AnswerWrapper.vue), since that's easy to hit during a build. I'd also like to understand the Stop case before this goes in, because if the Expert isn't done with the stopped turn when the next message arrives, we're back to the same overwrite.
The rest are smaller and fine as follow-ups if you prefer.
| this._clearInFlightUpdates() | ||
| this._agentStore.activeTaskList = null | ||
| // Stopping ends only the turn in flight; whatever is queued goes out next. | ||
| this._drainSendQueue() |
There was a problem hiding this comment.
When I tried this locally, the abort and the next queued message go out in the same millisecond. Do we know the Expert has fully wrapped up the stopped turn (history saved and so on) by the time the next message for that session lands? If not, this could bring back the same overwrite this PR is fixing, just less often. Might be worth waiting for some kind of ack on the abort before letting the queue move.
Also, handleMqttError goes through here too, so on a dropped connection whatever is queued gets flushed into the dead connection and lost. The instance ids are already marked as seen, so those notices never get retried.
There was a problem hiding this comment.
Good catch on both. For the overlap, the frontend can't really know when the Expert has finished saving the stopped turn, so an ack would only make it rarer. I'd rather fix it on the Expert side, by having each turn wait for the previous turn of the same session to finish before it reads the history. I'll follow up on that separately.
For the dropped connection, system notices are now held while the connection is down and go out after the reconnect. A notice whose publish fails is retried once after the reconnect. User messages aren't held, so they can't get stuck behind a connection that never comes back. Fixed in d59ab77.
| :tool-class="answer.toolClass" | ||
| :params="answer.params" | ||
| :status="resolvedToolApprovalStatus" | ||
| :disabled="interactionDisabled" |
There was a problem hiding this comment.
I think this combined with the queue can leave an approval stuck. If a turn ends with an approval card and the instance comes up after that, the "workspace ready" card gets added, so this card is no longer the latest and all four buttons go disabled. But _approvalBatch is still open, and the notice still goes to the Expert while the approval is pending. I reproduced it locally and the card just sits there greyed out with nothing answered. Maybe hold the queue while an approval batch is open, or cancel pending approvals when a system notice goes out (like _handleQuery does)? Not sure what the agent expects to happen if it gets a system notice while it's waiting on approvals.
There was a problem hiding this comment.
Thanks for reproducing this. I went with cancelling: sending a system notice now cancels the open approval batch, same as a user message does. On the Expert side, new input already cancels the dangling tool calls, and the Expert is told that cards go inactive once a newer message appears, so it asks again with a fresh card if it still needs the approval. Fixed in d59ab77.
| this._recentlyCompleted.delete(this._recentlyCompleted.keys().next().value) | ||
| } | ||
| await this.handleMessageResponse(JSON.parse(message.toString())) | ||
| this._drainSendQueue() |
There was a problem hiding this comment.
If handleMessageResponse throws, this never runs and the queued notice sits there until the user sends something or hits Stop. Moving it into a finally might be cheap insurance.
There was a problem hiding this comment.
Agreed, moved the drain into a finally. Fixed in d59ab77.
| if (this._sendDraining) return | ||
| this._sendDraining = true | ||
| try { | ||
| while (this._sendQueue.length && !this.isWaitingForResponse) { |
There was a problem hiding this comment.
isWaitingForResponse reads the in-flight requests of the current agentMode. If someone switches to insights while a support turn is in flight, the queue thinks nothing is pending and lets the notice out. Locally that ended up trying to open the insights MQTT connection, which got a 400, so the user saw a "Something went sideways" bubble and the notice was lost. Edge case, but maybe check the support store directly here, since queued sends are all support anyway?
There was a problem hiding this comment.
Nice find. Each queued send now remembers which agent it was queued for and only runs while that agent is active, so a support notice waits while you're on insights and goes out when you switch back. Insights sends don't wait on support turns either. Fixed in d59ab77.
| _relayedFailedInstanceIds: new Set(), | ||
| // Sends waiting for the turn in flight to settle, so the agent never gets two | ||
| // messages for one session at once. Not persisted. | ||
| _sendQueue: [], |
There was a problem hiding this comment.
Nit: since this lives in state, $reset() on logout wipes the queue without settling the promises. The only caller ignores the result so nothing really breaks, but it might be cleaner to keep the queue outside reactive state (module level or markRaw), since it isn't persisted anyway.
There was a problem hiding this comment.
Made the queue raw, and reset() now resolves anything still queued before $reset(). Fixed in d59ab77.
- Hold system notices while the broker connection is down and send them after the reconnect; a notice whose publish failed goes out once more - Run a queued send only while the agent it was queued for is active - Cancel open approval cards when a system notice goes out - Keep draining when rendering a reply throws - Settle queued sends on reset and keep the queue out of reactive state
This branch was successfully deployed
2 participants
Every message to the Expert is now queued so only one is in flight per session. Before, the instance ready/failed notice was published straight away, so if the instance came up while a reply was still streaming, two turns ran on the same session and each overwrote the other's chat history. That showed up as two separate chats.
Chat turns, opening a conversation, approval resumes and the instance notices all go through one queue in the Expert store. A job is finished when its reply arrives, or when the turn is stopped or errors.
Tests cover a notice arriving mid-turn, order, stop letting the next one through, and Start Over clearing the queue.