Skip to content

Commit a6537e6

Browse files
hannesrudolphhannesrudolph
committed
fix: prevent parent task state loss during orchestrator delegation
- Snapshot arrays with structuredClone before JsonStreamStringify to prevent lazy-read race - Return boolean from save methods; only clear userMessageContent on success - Make getTaskWithId non-destructive (return [] instead of deleting task on missing file) - Add try/catch around JSON.parse in read paths - Add retrySaveApiConversationHistory with exponential backoff - Add comprehensive test coverage (113 tests pass) Fixes #11172
1 parent 7db9b8c commit a6537e6

File tree

7 files changed

+127
-8
lines changed

7 files changed

+127
-8
lines changed

‎src/core/task-persistence/__tests__/apiMessages.spec.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,36 @@ describe("apiMessages.readApiMessages", () => {
5151
.catch(() => false)
5252
expect(stillExists).toBe(true)
5353
})
54+
55+
it("returns [] when file contains valid JSON that is not an array", async () => {
56+
const taskId = "task-non-array-api"
57+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
58+
await fs.mkdir(taskDir, { recursive: true })
59+
const filePath = path.join(taskDir, "api_conversation_history.json")
60+
await fs.writeFile(filePath, JSON.stringify("hello"), "utf8")
61+
62+
const result = await readApiMessages({
63+
taskId,
64+
globalStoragePath: tmpBaseDir,
65+
})
66+
67+
expect(result).toEqual([])
68+
})
69+
70+
it("returns [] when fallback file contains valid JSON that is not an array", async () => {
71+
const taskId = "task-non-array-fallback"
72+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
73+
await fs.mkdir(taskDir, { recursive: true })
74+
75+
// Only write the old fallback file, NOT the new one
76+
const oldPath = path.join(taskDir, "claude_messages.json")
77+
await fs.writeFile(oldPath, JSON.stringify({ key: "value" }), "utf8")
78+
79+
const result = await readApiMessages({
80+
taskId,
81+
globalStoragePath: tmpBaseDir,
82+
})
83+
84+
expect(result).toEqual([])
85+
})
5486
})

‎src/core/task-persistence/__tests__/taskMessages.spec.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,4 +83,19 @@ describe("taskMessages.readTaskMessages", () => {
8383

8484
expect(result).toEqual([])
8585
})
86+
87+
it("returns [] when file contains valid JSON that is not an array", async () => {
88+
const taskId = "task-non-array-json"
89+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
90+
await fs.mkdir(taskDir, { recursive: true })
91+
const filePath = path.join(taskDir, "ui_messages.json")
92+
await fs.writeFile(filePath, JSON.stringify("hello"), "utf8")
93+
94+
const result = await readTaskMessages({
95+
taskId,
96+
globalStoragePath: tmpBaseDir,
97+
})
98+
99+
expect(result).toEqual([])
100+
})
86101
})

‎src/core/task-persistence/apiMessages.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,13 @@ export async function readApiMessages({
5151
const fileContent = await fs.readFile(filePath, "utf8")
5252
try {
5353
const parsedData = JSON.parse(fileContent)
54-
if (Array.isArray(parsedData) && parsedData.length === 0) {
54+
if (!Array.isArray(parsedData)) {
55+
console.warn(
56+
`[readApiMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`,
57+
)
58+
return []
59+
}
60+
if (parsedData.length === 0) {
5561
console.error(
5662
`[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`,
5763
)
@@ -70,7 +76,13 @@ export async function readApiMessages({
7076
const fileContent = await fs.readFile(oldPath, "utf8")
7177
try {
7278
const parsedData = JSON.parse(fileContent)
73-
if (Array.isArray(parsedData) && parsedData.length === 0) {
79+
if (!Array.isArray(parsedData)) {
80+
console.warn(
81+
`[readApiMessages] Parsed OLD data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${oldPath}`,
82+
)
83+
return []
84+
}
85+
if (parsedData.length === 0) {
7486
console.error(
7587
`[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`,
7688
)

‎src/core/task-persistence/taskMessages.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,14 @@ export async function readTaskMessages({
2424

2525
if (fileExists) {
2626
try {
27-
return JSON.parse(await fs.readFile(filePath, "utf8"))
27+
const parsedData = JSON.parse(await fs.readFile(filePath, "utf8"))
28+
if (!Array.isArray(parsedData)) {
29+
console.warn(
30+
`[readTaskMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`,
31+
)
32+
return []
33+
}
34+
return parsedData
2835
} catch (error) {
2936
console.warn(
3037
`[readTaskMessages] Failed to parse ${filePath} for task ${taskId}, returning empty: ${error instanceof Error ? error.message : String(error)}`,

‎src/core/task/Task.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1270,7 +1270,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12701270
private async saveApiConversationHistory(): Promise<boolean> {
12711271
try {
12721272
await saveApiMessages({
1273-
messages: [...this.apiConversationHistory],
1273+
messages: structuredClone(this.apiConversationHistory),
12741274
taskId: this.taskId,
12751275
globalStoragePath: this.globalStoragePath,
12761276
})
@@ -1283,10 +1283,26 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12831283

12841284
/**
12851285
* Public wrapper to retry saving the API conversation history.
1286+
* Uses exponential backoff: up to 3 attempts with delays of 100 ms, 500 ms, 1500 ms.
12861287
* Used by delegation flow when flushPendingToolResultsToHistory reports failure.
12871288
*/
12881289
public async retrySaveApiConversationHistory(): Promise<boolean> {
1289-
return this.saveApiConversationHistory()
1290+
const delays = [100, 500, 1500]
1291+
1292+
for (let attempt = 0; attempt < delays.length; attempt++) {
1293+
await new Promise<void>((resolve) => setTimeout(resolve, delays[attempt]))
1294+
console.warn(
1295+
`[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`,
1296+
)
1297+
1298+
const success = await this.saveApiConversationHistory()
1299+
1300+
if (success) {
1301+
return true
1302+
}
1303+
}
1304+
1305+
return false
12901306
}
12911307

12921308
// Cline Messages
@@ -1353,7 +1369,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
13531369
private async saveClineMessages(): Promise<boolean> {
13541370
try {
13551371
await saveTaskMessages({
1356-
messages: [...this.clineMessages],
1372+
messages: structuredClone(this.clineMessages),
13571373
taskId: this.taskId,
13581374
globalStoragePath: this.globalStoragePath,
13591375
})

‎src/core/task/__tests__/Task.persistence.spec.ts‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,13 @@ describe("Task persistence", () => {
270270
})
271271

272272
it("returns false on failure", async () => {
273-
mockSaveApiMessages.mockRejectedValueOnce(new Error("disk full"))
273+
vi.useFakeTimers()
274+
275+
// All 3 retry attempts must fail for retrySaveApiConversationHistory to return false
276+
mockSaveApiMessages
277+
.mockRejectedValueOnce(new Error("fail 1"))
278+
.mockRejectedValueOnce(new Error("fail 2"))
279+
.mockRejectedValueOnce(new Error("fail 3"))
274280

275281
const task = new Task({
276282
provider: mockProvider,
@@ -279,8 +285,36 @@ describe("Task persistence", () => {
279285
startTask: false,
280286
})
281287

282-
const result = await task.retrySaveApiConversationHistory()
288+
const promise = task.retrySaveApiConversationHistory()
289+
await vi.runAllTimersAsync()
290+
const result = await promise
291+
283292
expect(result).toBe(false)
293+
expect(mockSaveApiMessages).toHaveBeenCalledTimes(3)
294+
295+
vi.useRealTimers()
296+
})
297+
298+
it("succeeds on 2nd retry attempt", async () => {
299+
vi.useFakeTimers()
300+
301+
mockSaveApiMessages.mockRejectedValueOnce(new Error("fail 1")).mockResolvedValueOnce(undefined) // succeeds on 2nd try
302+
303+
const task = new Task({
304+
provider: mockProvider,
305+
apiConfiguration: mockApiConfig,
306+
task: "test task",
307+
startTask: false,
308+
})
309+
310+
const promise = task.retrySaveApiConversationHistory()
311+
await vi.runAllTimersAsync()
312+
const result = await promise
313+
314+
expect(result).toBe(true)
315+
expect(mockSaveApiMessages).toHaveBeenCalledTimes(2)
316+
317+
vi.useRealTimers()
284318
})
285319

286320
it("snapshots the array before passing to saveApiMessages", async () => {

‎src/core/webview/ClineProvider.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3201,6 +3201,9 @@ export class ClineProvider
32013201
console.error(
32023202
`[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`,
32033203
)
3204+
vscode.window.showWarningMessage(
3205+
"Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.",
3206+
)
32043207
}
32053208
}
32063209
} catch (error) {

0 commit comments

Comments
 (0)