Skip to content

Commit 592f4b2

Browse files
author
daewoongoh
committed
fix: preserve manual title rename over generated title across hosts
Decide the generated-title write from the persisted record inside the per-file lock instead of the store cache, so a rename committed by another provider's store is never overwritten.
1 parent d798435 commit 592f4b2

3 files changed

Lines changed: 140 additions & 15 deletions

File tree

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1186,6 +1186,78 @@ export class TaskHistoryStore {
11861186
})
11871187
}
11881188

1189+
/**
1190+
* Disk-authoritative conditional write for a generated task title.
1191+
*
1192+
* The decision reads the persisted record inside the per-file advisory
1193+
* lock's merge callback, so a manual rename committed by another store
1194+
* (another extension host) after this store's cache was last refreshed is
1195+
* preserved instead of overwritten: every provider owns a separate
1196+
* TaskHistoryStore with a separate in-process lock and cache, so a
1197+
* cache-only existing-title check is not sufficient. An existing disk
1198+
* title — a manual rename or an earlier generated title — always wins,
1199+
* and the record's other disk fields are preserved when the generated
1200+
* title is applied.
1201+
*
1202+
* @returns The authoritative disk record and whether the generated title
1203+
* was applied.
1204+
* @throws If the task ID is not present in the cache, so the caller can retry.
1205+
*/
1206+
public async setGeneratedTitleIfUnset(
1207+
taskId: string,
1208+
title: string,
1209+
): Promise<{ item: HistoryItem; applied: boolean }> {
1210+
return this.withLock(async () => {
1211+
const cached = this.cache.get(taskId)
1212+
if (!cached) {
1213+
throw new Error(`[TaskHistoryStore] setGeneratedTitleIfUnset: task ${taskId} not found in cache`)
1214+
}
1215+
const trimmed = title.trim()
1216+
if (!trimmed) {
1217+
return { item: cached, applied: false }
1218+
}
1219+
1220+
const filePath = await this.getTaskFilePath(taskId)
1221+
const incoming: HistoryItem = { ...cached, title: trimmed }
1222+
// Seed the decision from the cache, then let the locked disk record
1223+
// override it inside the merge callback. When the per-file lock
1224+
// cannot hand back the disk record (a safeWriteJson double that
1225+
// never invokes `merge`), the cache decision stands.
1226+
let applied = !cached.title
1227+
let authoritative: HistoryItem = cached.title ? cached : incoming
1228+
await safeWriteJson(filePath, incoming, {
1229+
merge: (existing) => {
1230+
if (existing && typeof existing === "object" && "id" in existing) {
1231+
const disk = existing as HistoryItem
1232+
if (disk.title) {
1233+
// A manual rename or an earlier generated title
1234+
// from any host is already persisted; keep it.
1235+
applied = false
1236+
authoritative = disk
1237+
return disk
1238+
}
1239+
// Apply only the title onto the locked disk record so
1240+
// fields written by another host are preserved.
1241+
applied = true
1242+
authoritative = { ...disk, title: trimmed }
1243+
return authoritative
1244+
}
1245+
// No readable disk record; fall back to writing the
1246+
// cached item with the title applied, matching the
1247+
// delta-write fallback for a missing file.
1248+
applied = true
1249+
authoritative = incoming
1250+
return incoming
1251+
},
1252+
})
1253+
this.cache.set(taskId, authoritative)
1254+
if (this.onWrite) {
1255+
await this.onWrite(this.getAll())
1256+
}
1257+
return { item: authoritative, applied }
1258+
})
1259+
}
1260+
11891261
// ────────────────────────────── Private: Write lock ──────────────────────────────
11901262

11911263
/**

‎src/core/task-persistence/__tests__/TaskHistoryStore.realConcurrency.spec.ts‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,4 +421,65 @@ describe("TaskHistoryStore real cross-host locking", () => {
421421
await fs.rm(storagePath, { recursive: true, force: true })
422422
}
423423
})
424+
425+
it("preserves a manual rename committed by another store over a stale-cache generated title", async () => {
426+
const storagePath = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-generated-title-"))
427+
const storeA = new TaskHistoryStore(storagePath)
428+
const storeB = new TaskHistoryStore(storagePath)
429+
const filePath = path.join(storagePath, "tasks", "shared-task", "history_item.json")
430+
431+
try {
432+
await storeA.initialize()
433+
await storeA.upsert(item("shared-task"))
434+
// storeB hydrates its cache before the rename, so its cached copy
435+
// never sees the manual title.
436+
await storeB.initialize()
437+
438+
// The manual rename commits to disk through store A before store
439+
// B attempts the generated-title write.
440+
await storeA.atomicReadAndUpdate("shared-task", (current) => ({ ...current, title: "Manual Title" }))
441+
442+
const result = await storeB.setGeneratedTitleIfUnset("shared-task", "Generated Title")
443+
444+
expect(result.applied).toBe(false)
445+
expect(result.item.title).toBe("Manual Title")
446+
// The stale cache is refreshed with the authoritative disk record.
447+
expect(storeB.get("shared-task")?.title).toBe("Manual Title")
448+
expect(JSON.parse(await fs.readFile(filePath, "utf8"))).toMatchObject({ title: "Manual Title" })
449+
} finally {
450+
storeA.dispose()
451+
storeB.dispose()
452+
await fs.rm(storagePath, { recursive: true, force: true })
453+
}
454+
})
455+
456+
it("applies the generated title onto the locked disk record when no disk title exists", async () => {
457+
const storagePath = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-generated-title-apply-"))
458+
const storeA = new TaskHistoryStore(storagePath)
459+
const storeB = new TaskHistoryStore(storagePath)
460+
const filePath = path.join(storagePath, "tasks", "shared-task", "history_item.json")
461+
462+
try {
463+
await storeA.initialize()
464+
await storeA.upsert(item("shared-task"))
465+
await storeB.initialize()
466+
467+
// Another host persists an unrelated field; storeB's cache stays
468+
// stale, so the apply must read the locked disk record.
469+
await storeA.atomicReadAndUpdate("shared-task", (current) => ({ ...current, totalCost: 42 }))
470+
471+
const result = await storeB.setGeneratedTitleIfUnset("shared-task", "Generated Title")
472+
473+
expect(result.applied).toBe(true)
474+
expect(result.item).toMatchObject({ title: "Generated Title", totalCost: 42 })
475+
expect(JSON.parse(await fs.readFile(filePath, "utf8"))).toMatchObject({
476+
title: "Generated Title",
477+
totalCost: 42,
478+
})
479+
} finally {
480+
storeA.dispose()
481+
storeB.dispose()
482+
await fs.rm(storagePath, { recursive: true, force: true })
483+
}
484+
})
424485
})

‎src/core/webview/ClineProvider.ts‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,28 +2833,20 @@ export class ClineProvider
28332833

28342834
/*
28352835
* Applies an automatically generated title for a history item (issue #2333).
2836-
* The existing-title check and the write happen inside one atomic store
2837-
* update, so a manual rename queued ahead of this call is never overwritten
2838-
* by the generated title. Returns true when the title was applied, false
2839-
* when the item already had a title. Throws when the task is not in the
2840-
* store yet so the caller can retry.
2836+
* The existing-title check reads the persisted record inside the same
2837+
* per-file lock as the write, so a manual rename committed by another
2838+
* provider's store after this store's cache was last refreshed is never
2839+
* overwritten by the generated title. Returns true when the title was
2840+
* applied, false when the item already had a title. Throws when the task
2841+
* is not in the store yet so the caller can retry.
28412842
*/
28422843
async setGeneratedTaskTitle(id: string, title: string): Promise<boolean> {
28432844
const trimmed = title.trim()
28442845
if (!trimmed) {
28452846
return false
28462847
}
28472848

2848-
let applied = false
2849-
await this.taskHistoryStore.atomicReadAndUpdate(id, (historyItem) => {
2850-
// Preserve any title set before this write: a manual rename, or an
2851-
// earlier generated title, always wins over a later generated one.
2852-
if (historyItem.title) {
2853-
return historyItem
2854-
}
2855-
applied = true
2856-
return { ...historyItem, title: trimmed }
2857-
})
2849+
const { applied } = await this.taskHistoryStore.setGeneratedTitleIfUnset(id, trimmed)
28582850

28592851
if (applied) {
28602852
this.recentTasksCache = undefined

0 commit comments

Comments
 (0)