-
Notifications
You must be signed in to change notification settings - Fork 153
fix(server): prevent ResultManager from leaking mutable Task references. #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,7 +32,9 @@ export class ResultManager { | |||||
| // The ExecutionEventQueue will stop after a message event. | ||||||
| } else if (event.kind === 'task') { | ||||||
| const taskEvent = event as Task; | ||||||
| this.currentTask = { ...taskEvent }; // Make a copy | ||||||
| // Deep clone so that subsequent copy-on-write updates never alias the | ||||||
| // caller's event object (including its history/artifacts arrays). | ||||||
| this.currentTask = structuredClone(taskEvent); | ||||||
|
|
||||||
| // Ensure the latest user message is in history if not already present | ||||||
| if (this.latestUserMessage) { | ||||||
|
|
@@ -47,107 +49,75 @@ export class ResultManager { | |||||
| await this.saveCurrentTask(); | ||||||
| } else if (event.kind === 'status-update') { | ||||||
| const updateEvent = event as TaskStatusUpdateEvent; | ||||||
| let base: Task | undefined; | ||||||
| if (this.currentTask && this.currentTask.id === updateEvent.taskId) { | ||||||
| this.currentTask.status = updateEvent.status; | ||||||
| if (updateEvent.status.message) { | ||||||
| // Add message to history if not already present | ||||||
| if ( | ||||||
| !this.currentTask.history?.find( | ||||||
| (msg) => msg.messageId === updateEvent.status.message!.messageId | ||||||
| ) | ||||||
| ) { | ||||||
| this.currentTask.history = [ | ||||||
| ...(this.currentTask.history || []), | ||||||
| updateEvent.status.message, | ||||||
| ]; | ||||||
| } | ||||||
| } | ||||||
| await this.saveCurrentTask(); | ||||||
| base = this.currentTask; | ||||||
| } else if (!this.currentTask && updateEvent.taskId) { | ||||||
| // Potentially an update for a task we haven't seen the 'task' event for yet, | ||||||
| // or we are rehydrating. Attempt to load. | ||||||
| const loaded = await this.taskStore.load(updateEvent.taskId, this.serverCallContext); | ||||||
| if (loaded) { | ||||||
| this.currentTask = loaded; | ||||||
| this.currentTask.status = updateEvent.status; | ||||||
| if (updateEvent.status.message) { | ||||||
| if ( | ||||||
| !this.currentTask.history?.find( | ||||||
| (msg) => msg.messageId === updateEvent.status.message!.messageId | ||||||
| ) | ||||||
| ) { | ||||||
| this.currentTask.history = [ | ||||||
| ...(this.currentTask.history || []), | ||||||
| updateEvent.status.message, | ||||||
| ]; | ||||||
| } | ||||||
| } | ||||||
| await this.saveCurrentTask(); | ||||||
| } else { | ||||||
| base = await this.taskStore.load(updateEvent.taskId, this.serverCallContext); | ||||||
| if (base === undefined) { | ||||||
| console.warn( | ||||||
| `ResultManager: Received status update for unknown task ${updateEvent.taskId}` | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
| if (base !== undefined) { | ||||||
| // Copy-on-write: build a new Task so any prior reference handed out | ||||||
| // (e.g. via getCurrentTask()) keeps showing its snapshot. | ||||||
| const next: Task = { ...base, status: updateEvent.status }; | ||||||
| if (updateEvent.status.message !== undefined) { | ||||||
| const exists = next.history?.some( | ||||||
| (msg) => msg.messageId === updateEvent.status.message?.messageId | ||||||
| ); | ||||||
| if (!exists) { | ||||||
| next.history = [...(next.history || []), updateEvent.status.message]; | ||||||
| } | ||||||
| } | ||||||
| this.currentTask = next; | ||||||
| await this.saveCurrentTask(); | ||||||
| } | ||||||
| // If it's a final status update, the ExecutionEventQueue will stop. | ||||||
| // The final result will be the currentTask. | ||||||
| } else if (event.kind === 'artifact-update') { | ||||||
| const artifactEvent = event as TaskArtifactUpdateEvent; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the status update, the
Suggested change
References
|
||||||
| let base: Task | undefined; | ||||||
| if (this.currentTask && this.currentTask.id === artifactEvent.taskId) { | ||||||
| if (!this.currentTask.artifacts) { | ||||||
| this.currentTask.artifacts = []; | ||||||
| base = this.currentTask; | ||||||
| } else if (!this.currentTask && artifactEvent.taskId) { | ||||||
| // No task in memory — try to rehydrate from the store. | ||||||
| base = await this.taskStore.load(artifactEvent.taskId, this.serverCallContext); | ||||||
| if (base === undefined) { | ||||||
| console.warn( | ||||||
| `ResultManager: Received artifact update for unknown task ${artifactEvent.taskId}` | ||||||
| ); | ||||||
| } | ||||||
| const existingArtifactIndex = this.currentTask.artifacts.findIndex( | ||||||
| } | ||||||
| if (base !== undefined) { | ||||||
| // Copy-on-write: rebuild the artifacts array so any prior reference | ||||||
| // handed out (e.g. via getCurrentTask()) keeps showing its snapshot. | ||||||
| const artifacts = [...(base.artifacts || [])]; | ||||||
| const idx = artifacts.findIndex( | ||||||
| (art) => art.artifactId === artifactEvent.artifact.artifactId | ||||||
| ); | ||||||
| if (existingArtifactIndex !== -1) { | ||||||
| if (artifactEvent.append) { | ||||||
| // Basic append logic, assuming parts are compatible | ||||||
| // More sophisticated merging might be needed for specific part types | ||||||
| const existingArtifact = this.currentTask.artifacts[existingArtifactIndex]; | ||||||
| existingArtifact.parts.push(...artifactEvent.artifact.parts); | ||||||
| if (artifactEvent.artifact.description) | ||||||
| existingArtifact.description = artifactEvent.artifact.description; | ||||||
| if (artifactEvent.artifact.name) existingArtifact.name = artifactEvent.artifact.name; | ||||||
| if (artifactEvent.artifact.metadata) | ||||||
| existingArtifact.metadata = { | ||||||
| ...existingArtifact.metadata, | ||||||
| ...artifactEvent.artifact.metadata, | ||||||
| }; | ||||||
| } else { | ||||||
| this.currentTask.artifacts[existingArtifactIndex] = artifactEvent.artifact; | ||||||
| } | ||||||
| if (idx === -1) { | ||||||
| artifacts.push(artifactEvent.artifact); | ||||||
| } else if (artifactEvent.append) { | ||||||
| const existing = artifacts[idx]; | ||||||
| artifacts[idx] = { | ||||||
| ...existing, | ||||||
| parts: [...existing.parts, ...artifactEvent.artifact.parts], | ||||||
| description: artifactEvent.artifact.description ?? existing.description, | ||||||
| name: artifactEvent.artifact.name ?? existing.name, | ||||||
| metadata: artifactEvent.artifact.metadata | ||||||
| ? { ...existing.metadata, ...artifactEvent.artifact.metadata } | ||||||
| : existing.metadata, | ||||||
| }; | ||||||
| } else { | ||||||
| this.currentTask.artifacts.push(artifactEvent.artifact); | ||||||
| artifacts[idx] = artifactEvent.artifact; | ||||||
| } | ||||||
| this.currentTask = { ...base, artifacts }; | ||||||
| await this.saveCurrentTask(); | ||||||
| } else if (!this.currentTask && artifactEvent.taskId) { | ||||||
| // Similar to status update, try to load if task not in memory | ||||||
| const loaded = await this.taskStore.load(artifactEvent.taskId, this.serverCallContext); | ||||||
| if (loaded) { | ||||||
| this.currentTask = loaded; | ||||||
| if (!this.currentTask.artifacts) this.currentTask.artifacts = []; | ||||||
| // Apply artifact update logic (as above) | ||||||
| const existingArtifactIndex = this.currentTask.artifacts.findIndex( | ||||||
| (art) => art.artifactId === artifactEvent.artifact.artifactId | ||||||
| ); | ||||||
| if (existingArtifactIndex !== -1) { | ||||||
| if (artifactEvent.append) { | ||||||
| this.currentTask.artifacts[existingArtifactIndex].parts.push( | ||||||
| ...artifactEvent.artifact.parts | ||||||
| ); | ||||||
| } else { | ||||||
| this.currentTask.artifacts[existingArtifactIndex] = artifactEvent.artifact; | ||||||
| } | ||||||
| } else { | ||||||
| this.currentTask.artifacts.push(artifactEvent.artifact); | ||||||
| } | ||||||
| await this.saveCurrentTask(); | ||||||
| } else { | ||||||
| console.warn( | ||||||
| `ResultManager: Received artifact update for unknown task ${artifactEvent.taskId}` | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| import { describe, beforeEach, it, expect } from 'vitest'; | ||
| import { MockTaskStore } from './mocks/task_store.mock.js'; | ||
| import { ResultManager } from '../../src/server/index.js'; | ||
| import { Message, Task, TaskArtifactUpdateEvent, TaskStatusUpdateEvent } from '../../src/index.js'; | ||
|
|
||
| describe('ResultManager isolation', () => { | ||
| let mockTaskStore: MockTaskStore; | ||
| let resultManager: ResultManager; | ||
|
|
||
| const taskId = 't-1'; | ||
| const contextId = 'c-1'; | ||
|
|
||
| beforeEach(() => { | ||
| mockTaskStore = new MockTaskStore(); | ||
| resultManager = new ResultManager(mockTaskStore); | ||
| }); | ||
|
|
||
| const createBaseTaskEvent = (overrides: Partial<Task> = {}): Task => ({ | ||
| kind: 'task', | ||
| id: taskId, | ||
| contextId, | ||
| status: { state: 'submitted', timestamp: '2026-01-01T00:00:00.000Z' }, | ||
| history: [], | ||
| artifacts: [], | ||
| ...overrides, | ||
| }); | ||
|
|
||
| it('snapshot returned by getCurrentTask() is not mutated by a later status-update', async () => { | ||
| await resultManager.processEvent(createBaseTaskEvent()); | ||
| const snap = resultManager.getCurrentTask(); | ||
| expect(snap.status.state).toBe('submitted'); | ||
|
|
||
| const update: TaskStatusUpdateEvent = { | ||
| kind: 'status-update', | ||
| taskId, | ||
| contextId, | ||
| status: { state: 'working', timestamp: '2026-01-01T00:00:01.000Z' }, | ||
| final: false, | ||
| }; | ||
| await resultManager.processEvent(update); | ||
|
|
||
| expect(snap.status.state).toBe('submitted'); | ||
| }); | ||
|
|
||
| it('snapshot returned by getCurrentTask() is not mutated by a later artifact-update', async () => { | ||
| await resultManager.processEvent(createBaseTaskEvent()); | ||
| const snap = resultManager.getCurrentTask(); | ||
| expect(snap.artifacts).toEqual([]); | ||
|
|
||
| const update: TaskArtifactUpdateEvent = { | ||
| kind: 'artifact-update', | ||
| taskId, | ||
| contextId, | ||
| artifact: { artifactId: 'a-1', parts: [{ kind: 'text', text: 'hello' }] }, | ||
| }; | ||
| await resultManager.processEvent(update); | ||
| expect(snap.artifacts).toEqual([]); | ||
| }); | ||
|
|
||
| it('source task event arrays are not mutated by subsequent updates', async () => { | ||
| const userMessage: Message = { | ||
| kind: 'message', | ||
| role: 'user', | ||
| messageId: 'user-m-1', | ||
| parts: [{ kind: 'text', text: 'hi' }], | ||
| }; | ||
| resultManager.setContext(userMessage); | ||
|
|
||
| const history: Message[] = []; | ||
| const artifacts: Task['artifacts'] = []; | ||
| const taskEvent = createBaseTaskEvent({ history, artifacts }); | ||
| await resultManager.processEvent(taskEvent); | ||
|
|
||
| const statusUpdate: TaskStatusUpdateEvent = { | ||
| kind: 'status-update', | ||
| taskId, | ||
| contextId, | ||
| status: { | ||
| state: 'working', | ||
| timestamp: '2026-01-01T00:00:01.000Z', | ||
| message: { | ||
| kind: 'message', | ||
| role: 'agent', | ||
| messageId: 'agent-m-1', | ||
| parts: [{ kind: 'text', text: 'working' }], | ||
| }, | ||
| }, | ||
| final: false, | ||
| }; | ||
| await resultManager.processEvent(statusUpdate); | ||
|
|
||
| const artifactUpdate: TaskArtifactUpdateEvent = { | ||
| kind: 'artifact-update', | ||
| taskId, | ||
| contextId, | ||
| artifact: { artifactId: 'a-1', parts: [{ kind: 'text', text: 'out' }] }, | ||
| }; | ||
| await resultManager.processEvent(artifactUpdate); | ||
|
|
||
| expect(history).toEqual([]); | ||
| expect(artifacts).toEqual([]); | ||
| expect(taskEvent.status.state).toBe('submitted'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fully isolate the
ResultManagerstate from subsequent mutations by the executor and to avoid a potential race condition during theawaitat line 58, consider deep-cloning theupdateEvent. Currently,next.status(line 68) aliases the status object from the event, meaning any later mutation of that object by the executor would be reflected inResultManager's internal state and subsequent snapshots.References
structuredClonefor deep copying objects instead ofJSON.parse(JSON.stringify())to ensure thatundefinedfields are preserved.