CaroFernando commented on code in PR #8753:
URL: https://github.com/apache/texera/pull/8753#discussion_r4343788612
##########
agent-service/src/agent/texera-agent.ts:
##########
@@ -498,17 +498,23 @@ export class TexeraAgent {
}
async sendMessage(userMessage: string, messageSource?: "chat" | "feedback"):
Promise<AgentMessageResult> {
- const messageId =
`msg-${this.agentId}-${++this.messageCounter}-${Date.now()}`;
- let stepIndex = 0;
-
- await this.refreshWorkflowFromBackend();
-
+ // State flips synchronously, before the first await below, so a second
+ // prompt arriving while this one is mid-flight (e.g. during
+ // refreshWorkflowFromBackend) sees GENERATING and is rejected instead of
+ // interleaving with this run's head/abortController/currentMessageId
(#8711).
+ if (this.state !== AgentStateEnum.AVAILABLE) {
Review Comment:
Fixed in c15d5a2. stop() now returns early unless state is GENERATING. The
WS stop handler also used to broadcast STOPPING unconditionally, which would
have left the client showing an idle agent as stopping, so it now broadcasts
agent.getState(). New tests:
unit: an idle stop is a no-op, and a stop while generating still moves to
STOPPING
unit: sendMessage succeeds after an idle stop
WS: an idle stop reports AVAILABLE
All three fail with the guard removed. Checked live too: POST /stop on an
idle agent, then a prompt, runs normally.
##########
agent-service/src/agent/texera-agent.ts:
##########
@@ -498,17 +498,23 @@ export class TexeraAgent {
}
async sendMessage(userMessage: string, messageSource?: "chat" | "feedback"):
Promise<AgentMessageResult> {
- const messageId =
`msg-${this.agentId}-${++this.messageCounter}-${Date.now()}`;
- let stepIndex = 0;
-
- await this.refreshWorkflowFromBackend();
-
+ // State flips synchronously, before the first await below, so a second
+ // prompt arriving while this one is mid-flight (e.g. during
+ // refreshWorkflowFromBackend) sees GENERATING and is rejected instead of
+ // interleaving with this run's head/abortController/currentMessageId
(#8711).
+ if (this.state !== AgentStateEnum.AVAILABLE) {
+ throw new Error("Agent is busy processing another message");
+ }
+ this.state = AgentStateEnum.GENERATING;
this.abortController = new AbortController();
- this.state = AgentStateEnum.GENERATING;
+ const messageId =
`msg-${this.agentId}-${++this.messageCounter}-${Date.now()}`;
+ let stepIndex = 0;
this.currentMessageId = messageId;
+ await this.refreshWorkflowFromBackend();
Review Comment:
Agreed, moved refreshWorkflowFromBackend() inside the try in c15d5a2, so the
finally now resets state no matter what.
##########
agent-service/src/agent/texera-agent.spec.ts:
##########
@@ -817,6 +817,32 @@ describe("sendMessage", () => {
expect(observed).toEqual([AgentState.GENERATING]);
expect(agent.getState()).toBe(AgentState.AVAILABLE);
});
+
+ test("a second sendMessage while one is in flight is rejected, not
interleaved (#8711)", async () => {
+ let release!: () => void;
+ const gate = new Promise<void>(r => (release = r));
+ const model = new MockLanguageModelV4({
+ doGenerate: async () => {
+ await gate;
+ return {
+ content: [{ type: "text", text: "x" }],
+ finishReason: finish("stop"),
+ usage: usage(1, 1),
+ warnings: [],
+ } as any;
+ },
+ });
+ const agent = makeAgentWith(model);
+ const first = agent.sendMessage("first");
+ await new Promise(r => setTimeout(r, 5));
Review Comment:
Removed in c15d5a2, so the test now pins the ordering fix as well as the
guard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
