Skip to content

Commit 866e10d

Browse files
author
easonLiangWorldedtech
committed
rebuild unit U9 on the corrected U7
1 parent 0e66025 commit 866e10d

2 files changed

Lines changed: 301 additions & 7 deletions

File tree

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

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { historyItemSchema, type HistoryItem } from "@roo-code/types"
99
import { GlobalFileNames } from "../../shared/globalFileNames"
1010
import { LOCK_STALE_MS, withFileLock } from "../../utils/fileLock"
1111
import { safeWriteJson } from "../../utils/safeWriteJson"
12+
import { resolveLockKey } from "../../services/file-safety/safeWriteText"
1213
import { getStorageBasePath } from "../../utils/storage"
1314
import { assertValidTransition, settleRejectedCreateSubtaskAction, type HistoryItemStatus } from "./taskLifecycle"
1415
import { computeHistoryDelta, DeltaRejectedError, mergeHistoryDelta } from "./taskStoreConcurrency"
@@ -280,7 +281,10 @@ export class TaskHistoryStore {
280281
// Remove per-task file (best-effort)
281282
try {
282283
const filePath = await this.getTaskFilePath(taskId)
283-
await withFileLock(filePath, (absoluteFilePath) => fs.unlink(absoluteFilePath))
284+
// Lock the resolved publish target, not the path as spelled: proper-lockfile
285+
// keys the lock by the path it is given, so an alias and its referent would
286+
// take two locks for one file. The unlink still removes the named path.
287+
await withFileLock(await this.lockKeyFor(filePath), () => fs.unlink(filePath))
284288
} catch {
285289
// File may already be deleted
286290
}
@@ -308,7 +312,9 @@ export class TaskHistoryStore {
308312
// Remove per-task file (best-effort)
309313
try {
310314
const filePath = await this.getTaskFilePath(taskId)
311-
await withFileLock(filePath, (absoluteFilePath) => fs.unlink(absoluteFilePath))
315+
// Same lock key as delete(): the resolved referent, while the unlink
316+
// still removes the path the caller named.
317+
await withFileLock(await this.lockKeyFor(filePath), () => fs.unlink(filePath))
312318
} catch {
313319
// File may already be deleted
314320
}
@@ -323,6 +329,15 @@ export class TaskHistoryStore {
323329

324330
// ────────────────────────────── Reconciliation ──────────────────────────────
325331

332+
/**
333+
* The lock key a writer would use for a task file. resolvePublishTarget refuses
334+
* a dangling link, so the delete paths and the liveness probe walk the chain
335+
* themselves to find the lock held at the referent.
336+
*/
337+
private async lockKeyFor(taskFilePath: string): Promise<string> {
338+
return resolveLockKey(taskFilePath)
339+
}
340+
326341
/**
327342
* Scan task directories and fix any drift between disk and cache.
328343
*
@@ -375,7 +390,9 @@ export class TaskHistoryStore {
375390
// held for the entire write, so its presence means a
376391
// write is in progress — keep the task live.
377392
try {
378-
const lockPath = (await this.getTaskFilePath(taskId)) + ".lock"
393+
// Probe the same key the writer locks: safeWriteJson locks the resolved
394+
// publish target, so an alias and its referent share one lock file.
395+
const lockPath = (await this.lockKeyFor(await this.getTaskFilePath(taskId))) + ".lock"
379396
const lockStat = await fs.stat(lockPath)
380397
if (Date.now() - lockStat.mtimeMs < LOCK_STALE_MS) {
381398
liveIds.add(taskId)

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

Lines changed: 281 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,13 @@ function historyFilePath(storagePath: string, taskId: string): string {
5252
return path.join(storagePath, "tasks", taskId, GlobalFileNames.historyItem)
5353
}
5454

55+
// The lock key is canonicalized through the parent directory, so the expected key is
56+
// the canonical directory plus the basename rather than the path built from
57+
// os.tmpdir(), which can be an 8.3 short path on the Windows runner.
58+
async function canonicalKey(filePath: string): Promise<string> {
59+
return path.join(await actualFs.realpath(path.dirname(filePath)), path.basename(filePath))
60+
}
61+
5562
function storeInternals(store: TaskHistoryStore): {
5663
cache: Map<string, HistoryItem>
5764
taskFileMtimes: Map<string, number>
@@ -71,6 +78,7 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
7178
storagePath = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-delete-semantics-"))
7279
stores = []
7380
onWrite = vi.fn().mockResolvedValue(undefined)
81+
vi.mocked(withFileLock).mockClear()
7482
vi.mocked(withFileLock).mockImplementation(actualFileLock.withFileLock)
7583
vi.mocked(fs.unlink).mockImplementation(actualFs.unlink)
7684
})
@@ -97,12 +105,15 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
97105
await store.upsert(makeHistoryItem({ id: "locked-delete" }))
98106
onWrite.mockClear()
99107

108+
// The lock key is the resolved publish target, so the expected key is
109+
// captured through realpath before the delete: on Windows os.tmpdir()
110+
// can be an 8.3 short path (C:\Users\RUNNER~1) that realpath expands
111+
// to the long form, and the file is gone after the unlink.
112+
const lockKey = await fs.realpath(historyFilePath(storagePath, "locked-delete"))
113+
100114
await expect(store.delete("locked-delete")).resolves.toBeUndefined()
101115

102-
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(
103-
historyFilePath(storagePath, "locked-delete"),
104-
expect.any(Function),
105-
)
116+
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(lockKey, expect.any(Function))
106117
await expect(fs.access(historyFilePath(storagePath, "locked-delete"))).rejects.toMatchObject({
107118
code: "ENOENT",
108119
})
@@ -173,6 +184,248 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
173184
expect(store.get("never-existed")).toBeUndefined()
174185
expect(onWrite).toHaveBeenCalledTimes(1)
175186
})
187+
188+
it("locks the resolved publish target while unlinking the path it was given", async () => {
189+
const store = createStore()
190+
await store.initialize()
191+
await store.upsert(makeHistoryItem({ id: "alias-del" }))
192+
const aliasPath = historyFilePath(storagePath, "alias-del")
193+
const referentPath = path.join(storagePath, "tasks", "alias-del", "referent-history.json")
194+
195+
// Real symlinks are unavailable in this CI lane, so the alias is
196+
// simulated through realpath, as in the safeWriteJson lock test.
197+
// Only the file resolves through the alias; the directory is canonicalized by the
198+
// the real fs exactly as in production, so the key matches the canonical directory
199+
const realpathSpy = vi
200+
.spyOn(fs, "realpath")
201+
.mockImplementation(async (target) =>
202+
target === aliasPath ? referentPath : actualFs.realpath(String(target)),
203+
)
204+
try {
205+
await expect(store.delete("alias-del")).resolves.toBeUndefined()
206+
} finally {
207+
realpathSpy.mockRestore()
208+
}
209+
210+
// One lock for the underlying file, keyed by the referent; the unlink
211+
// still targets the path the store named.
212+
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(await canonicalKey(referentPath), expect.any(Function))
213+
// Assert the unlink target itself: locking the referent while unlinking the
214+
// referent instead of the alias would keep the dangling link in place.
215+
expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(aliasPath)
216+
expect(vi.mocked(fs.unlink)).not.toHaveBeenCalledWith(referentPath)
217+
})
218+
219+
it("waits on the peer's lock at the referent when the link is dangling", async () => {
220+
// delete() must resolve the chain itself: resolvePublishTarget refuses a
221+
// dangling link, and the old code treated that rejection as "already deleted"
222+
// so the link was never removed.
223+
const store = createStore()
224+
await store.initialize()
225+
await store.upsert(makeHistoryItem({ id: "alias-dangling" }))
226+
const aliasPath = historyFilePath(storagePath, "alias-dangling")
227+
const referentPath = path.join(storagePath, "tasks", "alias-dangling", "referent-history.json")
228+
// Only the file resolves through the alias; the directory is canonicalized by the
229+
// the real fs exactly as in production, so the key matches the canonical directory
230+
const enoent = Object.assign(new Error("ENOENT"), { code: "ENOENT" })
231+
const realpathSpy = vi.spyOn(fs, "realpath").mockImplementation(async (target) => {
232+
if (target === aliasPath) throw enoent
233+
return actualFs.realpath(String(target))
234+
})
235+
const lstatSpy = vi
236+
.spyOn(fs, "lstat")
237+
.mockResolvedValue({ isSymbolicLink: () => true } as unknown as import("fs").Stats)
238+
const readlinkSpy = vi.spyOn(fs, "readlink").mockImplementation(async (p) => {
239+
if (p === aliasPath) return "referent-history.json"
240+
throw new Error("not a symbolic link")
241+
})
242+
try {
243+
await expect(store.delete("alias-dangling")).resolves.toBeUndefined()
244+
} finally {
245+
realpathSpy.mockRestore()
246+
lstatSpy.mockRestore()
247+
readlinkSpy.mockRestore()
248+
}
249+
250+
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(await canonicalKey(referentPath), expect.any(Function))
251+
expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(aliasPath)
252+
})
253+
})
254+
255+
describe("reconcile()", () => {
256+
it("keeps a cached task live when its file is absent but the lock is held at the resolved referent", async () => {
257+
const store = createStore()
258+
await store.initialize()
259+
await store.upsert(makeHistoryItem({ id: "alias-live" }))
260+
const aliasPath = historyFilePath(storagePath, "alias-live")
261+
const referentPath = path.join(storagePath, "tasks", "alias-live", "referent-history.json")
262+
263+
// Real symlinks are unavailable in this CI lane, so the alias is
264+
// simulated through realpath, as in the safeWriteJson lock test.
265+
// Only the file resolves through the alias; the directory is canonicalized by the
266+
// the real fs exactly as in production, so the key matches the canonical directory
267+
const realpathSpy = vi
268+
.spyOn(fs, "realpath")
269+
.mockImplementation(async (target) =>
270+
target === aliasPath ? referentPath : actualFs.realpath(String(target)),
271+
)
272+
try {
273+
// The rename window: the referent is momentarily missing, so the cached
274+
// file cannot be stat'd while the peer holds the lock at the key it locks.
275+
await actualFs.rm(aliasPath, { force: true })
276+
await actualFs.writeFile(referentPath + ".lock", "")
277+
await store.reconcile()
278+
} finally {
279+
realpathSpy.mockRestore()
280+
}
281+
282+
// The probe must look at the same key the writer locks, otherwise a live
283+
// task is evicted from the cache while its write is still in progress.
284+
expect(storeInternals(store).cache.has("alias-live")).toBe(true)
285+
})
286+
287+
it("keeps a cached task live when the alias is dangling during the rename window", async () => {
288+
// resolvePublishTarget refuses a dangling link because a writer must not publish
289+
// through the link path, but this probe runs exactly in that window, so it reads
290+
// the link one level itself to find the lock the writer holds at the referent.
291+
const store = createStore()
292+
await store.initialize()
293+
await store.upsert(makeHistoryItem({ id: "dangling-live" }))
294+
const aliasPath = historyFilePath(storagePath, "dangling-live")
295+
const referentPath = path.join(storagePath, "tasks", "dangling-live", "referent-history.json")
296+
297+
const realpathSpy = vi
298+
.spyOn(fs, "realpath")
299+
.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
300+
const lstatSpy = vi
301+
.spyOn(fs, "lstat")
302+
.mockResolvedValue({ isSymbolicLink: () => true } as unknown as import("fs").Stats)
303+
// A relative link target, so the probe must resolve it against the link's
304+
// directory rather than the process working directory.
305+
const readlinkSpy = vi.spyOn(fs, "readlink").mockImplementation(async (p) => {
306+
if (p === aliasPath) return "referent-history.json"
307+
throw new Error("not a symbolic link")
308+
})
309+
try {
310+
await actualFs.rm(aliasPath, { force: true })
311+
await actualFs.writeFile(referentPath + ".lock", "")
312+
await store.reconcile()
313+
} finally {
314+
realpathSpy.mockRestore()
315+
lstatSpy.mockRestore()
316+
readlinkSpy.mockRestore()
317+
}
318+
319+
expect(storeInternals(store).cache.has("dangling-live")).toBe(true)
320+
})
321+
322+
it("follows the whole link chain to the key the writer locked", async () => {
323+
// realpath resolves the whole chain, so it fails when the final referent is
324+
// momentarily renamed to its backup. The probe must walk the chain, not just
325+
// its first link, or it looks for a lock the writer never took.
326+
const store = createStore()
327+
await store.initialize()
328+
await store.upsert(makeHistoryItem({ id: "chain-live" }))
329+
const aliasPath = historyFilePath(storagePath, "chain-live")
330+
const dir = path.dirname(aliasPath)
331+
const referentPath = path.join(dir, "referent-history.json")
332+
const realpathSpy = vi
333+
.spyOn(fs, "realpath")
334+
.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
335+
const lstatSpy = vi
336+
.spyOn(fs, "lstat")
337+
.mockResolvedValue({ isSymbolicLink: () => true } as unknown as import("fs").Stats)
338+
const readlinkSpy = vi.spyOn(fs, "readlink").mockImplementation(async (p) => {
339+
if (p === aliasPath) return "nested-link.json"
340+
if (p === path.join(dir, "nested-link.json")) return "referent-history.json"
341+
throw new Error("not a symbolic link")
342+
})
343+
try {
344+
await actualFs.rm(aliasPath, { force: true })
345+
await actualFs.writeFile(referentPath + ".lock", "")
346+
await store.reconcile()
347+
} finally {
348+
realpathSpy.mockRestore()
349+
lstatSpy.mockRestore()
350+
readlinkSpy.mockRestore()
351+
}
352+
353+
expect(storeInternals(store).cache.has("chain-live")).toBe(true)
354+
})
355+
356+
it("probes the key the bounded walk actually reached on a long chain", async () => {
357+
// A chain longer than the hop limit: the walk stops at the limit, so the probe
358+
// must look at the key it reached, not at the far end of the chain.
359+
const store = createStore()
360+
await store.initialize()
361+
await store.upsert(makeHistoryItem({ id: "long-chain" }))
362+
const startPath = historyFilePath(storagePath, "long-chain")
363+
const dir = path.dirname(startPath)
364+
const hops = Array.from({ length: 9 }, (_, index) => path.join(dir, "h" + (index + 1) + ".json"))
365+
const reachedPath = hops[7]
366+
const realpathSpy = vi
367+
.spyOn(fs, "realpath")
368+
.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
369+
const lstatSpy = vi
370+
.spyOn(fs, "lstat")
371+
.mockResolvedValue({ isSymbolicLink: () => true } as unknown as import("fs").Stats)
372+
const readlinkSpy = vi.spyOn(fs, "readlink").mockImplementation(async (p) => {
373+
const index = [startPath, ...hops].indexOf(String(p))
374+
if (index === -1 || index === hops.length) throw new Error("not a symbolic link")
375+
return hops[index]
376+
})
377+
try {
378+
await actualFs.rm(startPath, { force: true })
379+
await actualFs.writeFile(reachedPath + ".lock", "")
380+
await store.reconcile()
381+
} finally {
382+
realpathSpy.mockRestore()
383+
lstatSpy.mockRestore()
384+
readlinkSpy.mockRestore()
385+
}
386+
387+
expect(storeInternals(store).cache.has("long-chain")).toBe(true)
388+
})
389+
390+
it("terminates on a link cycle instead of holding the store lock forever", async () => {
391+
// Two links that point at each other: every readlink succeeds, so an unbounded
392+
// walk would never release the store lock.
393+
const store = createStore()
394+
await store.initialize()
395+
await store.upsert(makeHistoryItem({ id: "cycle-live" }))
396+
const aPath = historyFilePath(storagePath, "cycle-live")
397+
const bPath = path.join(path.dirname(aPath), "b.json")
398+
const realpathSpy = vi
399+
.spyOn(fs, "realpath")
400+
.mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))
401+
const lstatSpy = vi
402+
.spyOn(fs, "lstat")
403+
.mockResolvedValue({ isSymbolicLink: () => true } as unknown as import("fs").Stats)
404+
const readlinkSpy = vi.spyOn(fs, "readlink").mockImplementation(async (p) => {
405+
if (p === aPath) return "b.json"
406+
// Point back to the real alias basename so the walk actually loops; returning
407+
// a name that is not the alias stops the walk after two hops and never reaches
408+
// the hop limit this test is about.
409+
if (p === bPath) return path.basename(aPath)
410+
throw new Error("not a symbolic link")
411+
})
412+
try {
413+
await actualFs.rm(aPath, { force: true })
414+
await store.reconcile()
415+
// Assert inside the try: mockRestore() clears the call counts, so checking after
416+
// the finally block would read zero. Eight hops means the walk looped on the
417+
// cycle instead of stopping at the first non-link.
418+
expect(readlinkSpy).toHaveBeenCalledTimes(8)
419+
} finally {
420+
realpathSpy.mockRestore()
421+
lstatSpy.mockRestore()
422+
readlinkSpy.mockRestore()
423+
}
424+
425+
// The walk is bounded, so reconcile returned; the probe looked at the key it
426+
// reached after the bounded hops, found no lock there, and evicted the task.
427+
expect(storeInternals(store).cache.has("cycle-live")).toBe(false)
428+
})
176429
})
177430

178431
describe("deleteMany()", () => {
@@ -239,5 +492,29 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
239492
const writtenIds = (onWrite.mock.calls[0][0] as HistoryItem[]).map((item) => item.id)
240493
expect(writtenIds).toEqual([])
241494
})
495+
496+
it("locks the resolved publish target for every item while unlinking the given paths", async () => {
497+
const store = createStore()
498+
await store.initialize()
499+
await store.upsert(makeHistoryItem({ id: "alias-batch", ts: 1000 }))
500+
const aliasPath = historyFilePath(storagePath, "alias-batch")
501+
const referentPath = path.join(storagePath, "tasks", "alias-batch", "referent-history.json")
502+
503+
// Only the file resolves through the alias; the directory is canonicalized by the
504+
// the real fs exactly as in production, so the key matches the canonical directory
505+
const realpathSpy = vi
506+
.spyOn(fs, "realpath")
507+
.mockImplementation(async (target) =>
508+
target === aliasPath ? referentPath : actualFs.realpath(String(target)),
509+
)
510+
try {
511+
await expect(store.deleteMany(["alias-batch"])).resolves.toBeUndefined()
512+
} finally {
513+
realpathSpy.mockRestore()
514+
}
515+
516+
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(await canonicalKey(referentPath), expect.any(Function))
517+
expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(aliasPath)
518+
})
242519
})
243520
})

0 commit comments

Comments
 (0)