Skip to content

Commit eb7e970

Browse files
edelaunaedelauna
committed
fix(lifecycle): narrow PR to rejected-action settlement and dedupe Gemini call IDs
Remove deletion coordination, the generic file-lock wrapper, atomicReplace backup machinery, lock-compromise propagation, and the partial deleteMany write-through. Issue #1714 requires none of them. Restore base safeWriteJson behavior and base store deletion semantics, and restore the pre-existing real-lock write-barrier tests. Reimplement clearPendingActionIfMatching on the base safeWriteJson merge callback. The merge callback reads the persisted record under the per-file lock, a throwing merge writes nothing, so a record deleted by another host still fails settlement closed without recreation. Add a request-unique component to synthesized Gemini tool-call IDs. The per-request counter restarts at zero, so the first new_task call of every request previously persisted the same pending-action ID. Exact-ID settlement matching is unchanged. Drop the tests that only covered removed behavior and add a focused test for request-unique Gemini call IDs.
1 parent 748f8e3 commit eb7e970

10 files changed

Lines changed: 218 additions & 739 deletions

File tree

‎src/api/providers/__tests__/gemini.spec.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,43 @@ describe("GeminiHandler", () => {
332332
})
333333
})
334334

335+
it("generates request-unique tool call IDs across requests (#1714)", async () => {
336+
const metadata = {
337+
taskId: "test-task",
338+
tools: [{ type: "function", function: { name: "new_task", description: "", parameters: {} } }],
339+
} satisfies ApiHandlerCreateMessageMetadata
340+
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Delegate" }]
341+
342+
// Each request restarts the tool-call counter at zero, so the
343+
// synthesized ID must carry a request-unique component to keep
344+
// persisted pending-action IDs distinct across requests.
345+
const firstCallIds: string[] = []
346+
for (let request = 0; request < 2; request++) {
347+
mockGenerateContentStream.mockResolvedValueOnce(
348+
asyncStreamFrom([
349+
{
350+
candidates: [{ content: { parts: [{ functionCall: { name: "new_task", args: {} } }] } }],
351+
},
352+
]),
353+
)
354+
const chunks = await collectStream(handler.createMessage(systemPrompt, messages, metadata))
355+
const partialIds = chunks
356+
.filter((chunk) => chunk.type === "tool_call_partial")
357+
.map((chunk) => (chunk as { id: string }).id)
358+
expect(partialIds.length).toBeGreaterThan(0)
359+
// Both partial chunks of one synthesized call share one ID.
360+
expect(new Set(partialIds).size).toBe(1)
361+
firstCallIds.push(partialIds[0])
362+
}
363+
364+
// The first call of each request must not collide.
365+
expect(firstCallIds[0]).not.toBe(firstCallIds[1])
366+
for (const id of firstCallIds) {
367+
expect(id).toMatch(/^new_task-.+-0$/)
368+
expect(id).not.toBe("new_task-0")
369+
}
370+
})
371+
335372
it("should handle API errors", async () => {
336373
const mockError = new Error("Gemini API error")
337374
;(handler["client"].models.generateContentStream as any).mockRejectedValue(mockError)

‎src/api/providers/gemini.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,10 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
354354
let finalResponse: { responseId?: string } | undefined
355355
let finishReason: string | undefined
356356

357+
// Gemini provides no call ID, so one is synthesized here. The
358+
// request-unique component keeps persisted pending-action IDs (#1714)
359+
// distinct across requests even though the counter restarts at zero.
360+
const toolCallRequestId = crypto.randomUUID()
357361
let toolCallCounter = 0
358362
let hasContent = false
359363
let hasReasoning = false
@@ -398,7 +402,7 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
398402
hasContent = true
399403
// Gemini sends complete function calls in a single chunk
400404
// Emit as partial chunks for consistent handling with NativeToolCallParser
401-
const callId = `${part.functionCall.name}-${toolCallCounter}`
405+
const callId = `${part.functionCall.name}-${toolCallRequestId}-${toolCallCounter}`
402406
const args = JSON.stringify(part.functionCall.args)
403407

404408
// Emit name first

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

Lines changed: 23 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ import deepEqual from "fast-deep-equal"
77
import type { HistoryItem } from "@roo-code/types"
88

99
import { GlobalFileNames } from "../../shared/globalFileNames"
10-
import { LOCK_STALE_MS, withFileLock } from "../../utils/fileLock"
11-
import { safeWriteJson } from "../../utils/safeWriteJson"
10+
import { LOCK_STALE_MS, safeWriteJson } from "../../utils/safeWriteJson"
1211
import { getStorageBasePath } from "../../utils/storage"
1312
import { assertValidTransition, settleRejectedCreateSubtaskAction, type HistoryItemStatus } from "./taskLifecycle"
1413
import { computeHistoryDelta, DeltaRejectedError, mergeHistoryDelta } from "./taskStoreConcurrency"
@@ -267,7 +266,16 @@ export class TaskHistoryStore {
267266
*/
268267
async delete(taskId: string): Promise<void> {
269268
return this.withLock(async () => {
270-
await this.deleteTaskFile(taskId)
269+
this.cache.delete(taskId)
270+
this.taskFileMtimes.delete(taskId)
271+
272+
// Remove per-task file (best-effort)
273+
try {
274+
const filePath = await this.getTaskFilePath(taskId)
275+
await fs.unlink(filePath)
276+
} catch {
277+
// File may already be deleted
278+
}
271279

272280
// Call onWrite callback inside the lock for serialized write-through
273281
if (this.onWrite) {
@@ -281,30 +289,16 @@ export class TaskHistoryStore {
281289
*/
282290
async deleteMany(taskIds: string[]): Promise<void> {
283291
return this.withLock(async () => {
284-
let deletedCount = 0
285-
try {
286-
for (const taskId of taskIds) {
287-
await this.deleteTaskFile(taskId)
288-
deletedCount++
289-
}
290-
} catch (error) {
291-
// Earlier deletions already removed their files and cache entries.
292-
// Await the write-through with the current cache before the
293-
// original deletion error rejects the call, so persisted
294-
// globalState does not keep already-deleted tasks.
295-
if (deletedCount > 0 && this.onWrite) {
296-
try {
297-
await this.onWrite(this.getAll())
298-
} catch (writeError) {
299-
// The deletion failure is the primary error. Report the
300-
// write-through failure without masking it.
301-
console.error(
302-
"[TaskHistoryStore] deleteMany write-through after partial deletion failed:",
303-
writeError,
304-
)
305-
}
292+
for (const taskId of taskIds) {
293+
this.cache.delete(taskId)
294+
this.taskFileMtimes.delete(taskId)
295+
296+
try {
297+
const filePath = await this.getTaskFilePath(taskId)
298+
await fs.unlink(filePath)
299+
} catch {
300+
// File may already be deleted
306301
}
307-
throw error
308302
}
309303

310304
// Call onWrite callback inside the lock for serialized write-through
@@ -889,34 +883,6 @@ export class TaskHistoryStore {
889883
}
890884
}
891885

892-
/**
893-
* Delete one task file under the same advisory lock used by `safeWriteJson`.
894-
* Cache state changes only after the file is absent or unlink succeeds.
895-
*/
896-
private async deleteTaskFile(taskId: string): Promise<void> {
897-
const filePath = await this.getTaskFilePath(taskId)
898-
try {
899-
await withFileLock(filePath, async (absoluteFilePath) => {
900-
try {
901-
await fs.unlink(absoluteFilePath)
902-
} catch (error) {
903-
if (!this.isFileNotFoundError(error)) {
904-
throw error
905-
}
906-
}
907-
})
908-
} catch (error) {
909-
// A missing parent directory prevents lock creation and also proves
910-
// that the history file is absent.
911-
if (!this.isFileNotFoundError(error)) {
912-
throw error
913-
}
914-
}
915-
916-
this.cache.delete(taskId)
917-
this.taskFileMtimes.delete(taskId)
918-
}
919-
920886
// ────────────────────────────── Private: fs.watch ──────────────────────────────
921887

922888
/**
@@ -1122,12 +1088,12 @@ export class TaskHistoryStore {
11221088
let missingDiskRecord = false
11231089
try {
11241090
await safeWriteJson(filePath, cached, {
1125-
createParentDirectory: false,
1126-
atomicReplace: true,
11271091
merge: (existing) => {
11281092
if (!existing || typeof existing !== "object" || !("id" in existing)) {
11291093
// Writing the cached record back would recreate a task
11301094
// another host deleted, so drop the stale entry first.
1095+
// A throwing merge writes nothing, so the deleted
1096+
// record stays deleted.
11311097
missingDiskRecord = true
11321098
this.cache.delete(taskId)
11331099
this.taskFileMtimes.delete(taskId)
@@ -1141,16 +1107,7 @@ export class TaskHistoryStore {
11411107
},
11421108
})
11431109
} catch (error) {
1144-
const missingLockPath =
1145-
error &&
1146-
typeof error === "object" &&
1147-
"code" in error &&
1148-
error.code === "ENOENT" &&
1149-
"path" in error &&
1150-
error.path === `${filePath}.lock`
1151-
if (missingDiskRecord || missingLockPath) {
1152-
this.cache.delete(taskId)
1153-
this.taskFileMtimes.delete(taskId)
1110+
if (missingDiskRecord) {
11541111
throw new Error(
11551112
`[TaskHistoryStore] clearPendingActionIfMatching: task ${taskId} not found in cache`,
11561113
)

0 commit comments

Comments
 (0)