Skip to content

Commit 7d5f527

Browse files
hannesrudolphhannesrudolph
committed
fix: race conditions in subtask delegation system
- Remove initialStatus from taskMetadata rebuild to prevent saveClineMessages from clobbering delegation status on every save - Add per-task delegation metadata files (delegationMeta.ts) as cross-process-safe source of truth using safeWriteJson with proper-lockfile - Add delegationInProgress reentrancy guard to prevent UI actions from interleaving with async delegation transitions - Wrap child.start() in try/catch with parent status repair on failure - Cancel debouncedEmitTokenUsage in Task.dispose() to prevent zombie callbacks
1 parent 4438fda commit 7d5f527

File tree

9 files changed

+733
-276
lines changed

9 files changed

+733
-276
lines changed

‎src/__tests__/history-resume-delegation.spec.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ vi.mock("../core/task-persistence", () => ({
3434
saveApiMessages: vi.fn().mockResolvedValue(undefined),
3535
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
3636
}))
37+
vi.mock("../core/task-persistence/delegationMeta", () => ({
38+
readDelegationMeta: vi.fn().mockResolvedValue(null),
39+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
40+
}))
3741

3842
import { ClineProvider } from "../core/webview/ClineProvider"
3943
import { readTaskMessages } from "../core/task-persistence/taskMessages"
@@ -145,6 +149,7 @@ describe("History resume delegation - parent metadata transitions", () => {
145149
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
146150
}),
147151
updateTaskHistory: vi.fn().mockResolvedValue([]),
152+
log: vi.fn(),
148153
} as unknown as ClineProvider
149154

150155
// Start with existing messages in history
@@ -228,6 +233,7 @@ describe("History resume delegation - parent metadata transitions", () => {
228233
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
229234
}),
230235
updateTaskHistory: vi.fn().mockResolvedValue([]),
236+
log: vi.fn(),
231237
} as unknown as ClineProvider
232238

233239
// Include an assistant message with new_task tool_use to exercise the tool_result path
@@ -314,6 +320,7 @@ describe("History resume delegation - parent metadata transitions", () => {
314320
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
315321
}),
316322
updateTaskHistory: vi.fn().mockResolvedValue([]),
323+
log: vi.fn(),
317324
} as unknown as ClineProvider
318325

319326
// No assistant tool_use in history
@@ -549,6 +556,7 @@ describe("History resume delegation - parent metadata transitions", () => {
549556
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
550557
}),
551558
updateTaskHistory: vi.fn().mockResolvedValue([]),
559+
log: vi.fn(),
552560
} as unknown as ClineProvider
553561

554562
vi.mocked(readTaskMessages).mockResolvedValue([])
@@ -748,6 +756,7 @@ describe("History resume delegation - parent metadata transitions", () => {
748756
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
749757
}),
750758
updateTaskHistory: vi.fn().mockResolvedValue([]),
759+
log: vi.fn(),
751760
} as unknown as ClineProvider
752761

753762
// Mock read failures or empty returns

‎src/__tests__/provider-delegation.spec.ts‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22

33
import { describe, it, expect, vi } from "vitest"
44
import { RooCodeEventName } from "@roo-code/types"
5+
6+
vi.mock("../core/task-persistence/delegationMeta", () => ({
7+
readDelegationMeta: vi.fn().mockResolvedValue(null),
8+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
9+
}))
10+
511
import { ClineProvider } from "../core/webview/ClineProvider"
612

713
describe("ClineProvider.delegateParentAndOpenChild()", () => {
@@ -48,6 +54,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
4854
updateTaskHistory,
4955
handleModeSwitch,
5056
log: vi.fn(),
57+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
5158
} as unknown as ClineProvider
5259

5360
const params = {
@@ -70,11 +77,20 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
7077
startTask: false,
7178
})
7279

73-
// Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus)
74-
expect(updateTaskHistory).toHaveBeenCalledTimes(1)
80+
// Metadata persistence - child gets "active" status, parent gets "delegated" status
81+
expect(updateTaskHistory).toHaveBeenCalledTimes(2)
82+
83+
// Child set to "active" (first call)
84+
const childSaved = updateTaskHistory.mock.calls[0][0]
85+
expect(childSaved).toEqual(
86+
expect.objectContaining({
87+
id: "child-1",
88+
status: "active",
89+
}),
90+
)
7591

76-
// Parent set to "delegated"
77-
const parentSaved = updateTaskHistory.mock.calls[0][0]
92+
// Parent set to "delegated" (second call)
93+
const parentSaved = updateTaskHistory.mock.calls[1][0]
7894
expect(parentSaved).toEqual(
7995
expect.objectContaining({
8096
id: "parent-1",
@@ -130,6 +146,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
130146
updateTaskHistory,
131147
handleModeSwitch,
132148
log: vi.fn(),
149+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
133150
} as unknown as ClineProvider
134151

135152
await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, {
@@ -139,7 +156,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
139156
mode: "code",
140157
})
141158

142-
// Verify ordering: createTask → updateTaskHistory → child.start
143-
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
159+
// Verify ordering: createTask → updateTaskHistory (child) → updateTaskHistory (parent) → child.start
160+
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "updateTaskHistory", "child.start"])
144161
})
145162
})

‎src/__tests__/removeClineFromStack-delegation.spec.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
// npx vitest run __tests__/removeClineFromStack-delegation.spec.ts
22

33
import { describe, it, expect, vi } from "vitest"
4+
5+
vi.mock("../core/task-persistence/delegationMeta", () => ({
6+
readDelegationMeta: vi.fn().mockResolvedValue(null),
7+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
8+
}))
9+
410
import { ClineProvider } from "../core/webview/ClineProvider"
511

612
describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
@@ -38,6 +44,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
3844
log: vi.fn(),
3945
getTaskWithId,
4046
updateTaskHistory,
47+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
4148
}
4249

4350
return { provider, childTask, updateTaskHistory, getTaskWithId }
@@ -183,6 +190,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
183190
log: vi.fn(),
184191
getTaskWithId: vi.fn(),
185192
updateTaskHistory: vi.fn(),
193+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
186194
}
187195

188196
// Should not throw
@@ -263,6 +271,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
263271
log: vi.fn(),
264272
getTaskWithId,
265273
updateTaskHistory,
274+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
266275
}
267276

268277
// Simulate what delegateParentAndOpenChild does: pop B with skipDelegationRepair

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

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest"
2+
import * as os from "os"
3+
import * as path from "path"
4+
import * as fs from "fs/promises"
5+
6+
// Mocks (use hoisted to avoid initialization ordering issues)
7+
const hoisted = vi.hoisted(() => ({
8+
safeWriteJsonMock: vi.fn().mockResolvedValue(undefined),
9+
}))
10+
vi.mock("../../../utils/safeWriteJson", () => ({
11+
safeWriteJson: hoisted.safeWriteJsonMock,
12+
}))
13+
14+
// Import after mocks
15+
import { readDelegationMeta, saveDelegationMeta } from "../delegationMeta"
16+
import type { DelegationMeta } from "../delegationMeta"
17+
18+
let tmpBaseDir: string
19+
20+
beforeEach(async () => {
21+
hoisted.safeWriteJsonMock.mockClear()
22+
tmpBaseDir = await fs.mkdtemp(path.join(os.tmpdir(), "roo-test-delegation-"))
23+
})
24+
25+
describe("delegationMeta.readDelegationMeta", () => {
26+
it("returns null when file does not exist (backward compat)", async () => {
27+
const result = await readDelegationMeta({
28+
taskId: "task-no-file",
29+
globalStoragePath: tmpBaseDir,
30+
})
31+
32+
expect(result).toBeNull()
33+
})
34+
35+
it("returns parsed delegation metadata from file", async () => {
36+
const taskId = "task-with-meta"
37+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
38+
await fs.mkdir(taskDir, { recursive: true })
39+
const filePath = path.join(taskDir, "task_metadata.json")
40+
41+
const meta: DelegationMeta = {
42+
status: "delegated",
43+
delegatedToId: "child-1",
44+
awaitingChildId: "child-1",
45+
childIds: ["child-1"],
46+
}
47+
await fs.writeFile(filePath, JSON.stringify(meta), "utf8")
48+
49+
const result = await readDelegationMeta({
50+
taskId,
51+
globalStoragePath: tmpBaseDir,
52+
})
53+
54+
expect(result).toEqual(meta)
55+
})
56+
57+
it("filters out unknown keys from parsed data", async () => {
58+
const taskId = "task-extra-keys"
59+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
60+
await fs.mkdir(taskDir, { recursive: true })
61+
const filePath = path.join(taskDir, "task_metadata.json")
62+
63+
await fs.writeFile(
64+
filePath,
65+
JSON.stringify({
66+
status: "active",
67+
unknownField: "should-be-filtered",
68+
childIds: ["c1"],
69+
}),
70+
"utf8",
71+
)
72+
73+
const result = await readDelegationMeta({
74+
taskId,
75+
globalStoragePath: tmpBaseDir,
76+
})
77+
78+
expect(result).toEqual({ status: "active", childIds: ["c1"] })
79+
expect(result).not.toHaveProperty("unknownField")
80+
})
81+
82+
it("returns null when file contains invalid JSON", async () => {
83+
const taskId = "task-corrupt"
84+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
85+
await fs.mkdir(taskDir, { recursive: true })
86+
const filePath = path.join(taskDir, "task_metadata.json")
87+
await fs.writeFile(filePath, "{not valid json!!!", "utf8")
88+
89+
const result = await readDelegationMeta({
90+
taskId,
91+
globalStoragePath: tmpBaseDir,
92+
})
93+
94+
expect(result).toBeNull()
95+
})
96+
97+
it("returns null when file contains a non-object value", async () => {
98+
const taskId = "task-non-object"
99+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
100+
await fs.mkdir(taskDir, { recursive: true })
101+
const filePath = path.join(taskDir, "task_metadata.json")
102+
await fs.writeFile(filePath, JSON.stringify("hello"), "utf8")
103+
104+
const result = await readDelegationMeta({
105+
taskId,
106+
globalStoragePath: tmpBaseDir,
107+
})
108+
109+
expect(result).toBeNull()
110+
})
111+
112+
it("returns null when file contains an array", async () => {
113+
const taskId = "task-array"
114+
const taskDir = path.join(tmpBaseDir, "tasks", taskId)
115+
await fs.mkdir(taskDir, { recursive: true })
116+
const filePath = path.join(taskDir, "task_metadata.json")
117+
await fs.writeFile(filePath, JSON.stringify([1, 2, 3]), "utf8")
118+
119+
const result = await readDelegationMeta({
120+
taskId,
121+
globalStoragePath: tmpBaseDir,
122+
})
123+
124+
expect(result).toBeNull()
125+
})
126+
})
127+
128+
describe("delegationMeta.saveDelegationMeta", () => {
129+
it("writes delegation metadata via safeWriteJson", async () => {
130+
const meta: DelegationMeta = {
131+
status: "delegated",
132+
delegatedToId: "child-1",
133+
awaitingChildId: "child-1",
134+
childIds: ["child-1"],
135+
}
136+
137+
await saveDelegationMeta({
138+
taskId: "task-1",
139+
globalStoragePath: tmpBaseDir,
140+
meta,
141+
})
142+
143+
expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1)
144+
const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0]
145+
expect(persisted).toEqual(meta)
146+
})
147+
148+
it("filters out unknown keys before writing", async () => {
149+
const meta = {
150+
status: "active",
151+
unknownField: "should-be-filtered",
152+
childIds: ["c1"],
153+
} as any
154+
155+
await saveDelegationMeta({
156+
taskId: "task-2",
157+
globalStoragePath: tmpBaseDir,
158+
meta,
159+
})
160+
161+
expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1)
162+
const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0]
163+
expect(persisted).toEqual({ status: "active", childIds: ["c1"] })
164+
expect(persisted).not.toHaveProperty("unknownField")
165+
})
166+
167+
it("writes to the correct file path", async () => {
168+
await saveDelegationMeta({
169+
taskId: "task-path-check",
170+
globalStoragePath: tmpBaseDir,
171+
meta: { status: "completed" },
172+
})
173+
174+
expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1)
175+
const [filePath] = hoisted.safeWriteJsonMock.mock.calls[0]
176+
expect(filePath).toContain(path.join("tasks", "task-path-check", "task_metadata.json"))
177+
})
178+
})

0 commit comments

Comments
 (0)