Skip to content

Commit d6fa711

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
test(file-safety,utils,core): pin the order and flush the rows raised on 2357's head
Four of the seven rows CodeRabbit left open on 4d60c40 are test-side, so they land here rather than being explained away. Each one was run against a production control first, so the assertion is known to bite for the reason the row named and not for some unrelated drift. - utils/__tests__/fileLock.spec.ts, row 4509350112 (retarget ordering). The double logged nothing, so the row proved only which keys were asked for and that the callback ran once. Acquisitions and the one release now log around the same 30 ms hold the release uses and the callback logs into the same list, which is the ported semantics of the same row on 2359: acquire:1:started, acquire:1:completed, release:started, release:completed, acquire:2:started, acquire:2:completed, callback. Control at src/utils/fileLock.ts:198 (acquisition promise not awaited before the callback) turns this row red - 1 failed | 10 passed (11) - with callback landing before acquire:2:completed and nothing else in the list moving; the byte-identical restore returns it to green. - utils/__tests__/fileLock.spec.ts, row 4509350106 (reject path through the other entry point). The reviewer is right that withFileLock calls canonicalLockPath and _lockCanonicalKey directly and never passes through acquireFileLock, so the two existing rejection rows cannot see a fallback added only to the exported entry point. Two rows now drive EACCES and a code-less realpath failure through acquireFileLock itself and assert no lock was taken. Control: canonicalLockPath(filePath).catch(() => path.resolve(filePath)) at src/utils/fileLock.ts:164 - 2 failed | 9 passed (11), failing exactly those two rows while the withFileLock rows stay green, which is the blind spot the row described. - services/file-safety/__tests__/safeWriteText.spec.ts, row 4509350103 (ancestor flush). The row asserted which directories were opened with "r"; opening and closing without flushing would have passed. The double now hands out distinct descriptors (901 deep, 902 nested, 903 root) and the row asserts each descriptor is fsynced and closed in the same deepest-first order. Control at src/services/file-safety/safeWriteText.ts:328 (handle opened and closed, never flushed): this row red with "expected [] to deeply equal [901, 902, 903]"; two existing fsync rows go red with it, which is expected - they assert the same fsync from the other side. - core/task/__tests__/Task.spec.ts, row 4509350094 (fence before the abort settles). abortTask now started rather than awaited and the first increment asserted while its promise is pending, then awaited for the existing 0 -> 2 assertion. Control: defer requestAbort one microtask inside Task.abortTask - this row red with "expected +0 to be 1" (a concurrent-abort row also trips, because making the method async changes the memoized promise identity, which is an artifact of the control shape and not of this assertion). Measured here at this commit with @roo-code/* mapped onto the tree under test: fileLock.spec 11 passed (11), safeWriteText.spec 83 passed (83), Task.spec 213 passed (213), whole misc lane 121 files / 2138 passed | 17 skipped (2155), tsc --noEmit -p . exit 0, eslint --max-warnings=0 --no-warn-ignored exit 0 and prettier --check clean on the committed bytes (the payload .husky/pre-commit would have charged, which does not run here because core.hooksPath points at .husky/_). Core and services were re-run at file scope for the two files they own. The gate's own functions report 770 counted executable lines with these three files changed in the tree and 770 at 4d60c40 before them: a test-only commit cannot move that number, because packageForPath returns undefined for __tests__ paths. The three production files were each restored byte-identical after their control run.
1 parent 4d60c40 commit d6fa711

3 files changed

Lines changed: 97 additions & 4 deletions

File tree

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4834,7 +4834,15 @@ describe("Cline", () => {
48344834

48354835
expect(task.cancellationGeneration).toBe(0)
48364836

4837-
await task.abortTask()
4837+
// Started, not awaited. requestAbort is synchronous and raises the fence before any of the
4838+
// abort work runs (Task.abortTask calls it before it hands back abortPromise), so the increment
4839+
// has to be visible while the abort promise is still pending. Asserting the generation only
4840+
// after the await would still pass if the increment landed at the end of the teardown, which is
4841+
// exactly the delay that lets a write parked on its path chain publish after the task is gone.
4842+
const aborting = task.abortTask()
4843+
expect(task.cancellationGeneration).toBe(1)
4844+
4845+
await aborting
48384846

48394847
expect(task.abort).toBe(true)
48404848
// Disposal never marks a task abandoned; only an explicit abandon does.

‎src/services/file-safety/__tests__/safeWriteText.spec.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1656,6 +1656,12 @@ describe("safeWriteText", () => {
16561656
// The outermost component mkdir had to create, in the spelling Node answers with.
16571657
vi.mocked(fs.mkdir).mockResolvedValue(nested)
16581658
vi.mocked(fsSync.openSync).mockClear()
1659+
// Distinct descriptors per directory, so a flush is attributable to the directory it belongs
1660+
// to rather than being one indistinguishable call on an undefined handle.
1661+
const dirDescriptors: Record<string, number> = { [deep]: 901, [nested]: 902, [root]: 903 }
1662+
vi.mocked(fsSync.openSync).mockImplementation(function (p: unknown) {
1663+
return dirDescriptors[path.resolve(String(p))] ?? 1
1664+
})
16591665

16601666
await safeWriteText(targetPath, "new data", { platform: "linux" })
16611667

@@ -1668,6 +1674,29 @@ describe("safeWriteText", () => {
16681674
return String(call[0])
16691675
})
16701676
expect(dirSyncs).toEqual([deep, nested, root])
1677+
// The open list alone only proves handles were taken. The durable effect is the flush: a
1678+
// production change that opened each created ancestor and closed it without fsyncing would leave
1679+
// this row green while the chain it just created stayed volatile, so each descriptor the open
1680+
// list names is pinned to its own flush and its own close, in the same deepest-first order.
1681+
const dirFdSet = new Set([901, 902, 903])
1682+
const dirFlushes = vi
1683+
.mocked(fsSync.fsyncSync)
1684+
.mock.calls.filter(function (call: unknown[]) {
1685+
return dirFdSet.has(call[0] as number)
1686+
})
1687+
.map(function (call: unknown[]) {
1688+
return call[0]
1689+
})
1690+
expect(dirFlushes).toEqual([901, 902, 903])
1691+
const dirCloses = vi
1692+
.mocked(fsSync.closeSync)
1693+
.mock.calls.filter(function (call: unknown[]) {
1694+
return dirFdSet.has(call[0] as number)
1695+
})
1696+
.map(function (call: unknown[]) {
1697+
return call[0]
1698+
})
1699+
expect(dirCloses).toEqual([901, 902, 903])
16711700
})
16721701

16731702
// A row about what the Windows filesystem reports, so it belongs to the Windows leg: it is

‎src/utils/__tests__/fileLock.spec.ts‎

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,32 @@ describe("fileLock - canonical lock keys", () => {
107107
expect(lockfile.lock).not.toHaveBeenCalled()
108108
})
109109

110+
// The two rows above reach the lock through withFileLock, which calls canonicalLockPath and
111+
// _lockCanonicalKey directly and never passes through acquireFileLock. A fallback added only to
112+
// that exported entry point - canonicalize, and on any failure lock the caller's own spelling -
113+
// would leave both of them green while a direct caller (safeWriteJson, task-history deletion)
114+
// started taking a lock no peer was contending for. Same rejections, direct entry point.
115+
test("propagates an EACCES realpath failure through the acquireFileLock entry point", async () => {
116+
vi.mocked(fsPromises.realpath).mockRejectedValueOnce(
117+
Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }),
118+
)
119+
const callerPath = path.join(STORE, "aliasDir", "task.json")
120+
121+
await expect(acquireFileLock(callerPath)).rejects.toMatchObject({ code: "EACCES" })
122+
123+
// Nothing was locked, so nothing ran under a key no peer was contending for.
124+
expect(lockfile.lock).not.toHaveBeenCalled()
125+
}) // end acquireFileLock EACCES row
126+
127+
test("propagates a code-less realpath failure through the acquireFileLock entry point", async () => {
128+
vi.mocked(fsPromises.realpath).mockRejectedValueOnce(new Error("unknown failure"))
129+
const callerPath = path.join(STORE, "aliasDir", "task.json")
130+
131+
await expect(acquireFileLock(callerPath)).rejects.toThrow("unknown failure")
132+
133+
expect(lockfile.lock).not.toHaveBeenCalled()
134+
}) // end acquireFileLock code-less row
135+
110136
test("canonicalizes the nearest existing ancestor when the file does not exist yet", async () => {
111137
// The file and its parent are absent, so realpath reports ENOENT for them; the deepest
112138
// existing ancestor is resolved and the missing components are re-appended.
@@ -233,20 +259,50 @@ test("retakes the lock when the canonical key moves while the callback waits to
233259
vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) =>
234260
String(target).replace("aliasDir", resolvedDir),
235261
)
262+
// Each acquisition and the one release are logged into ONE list around the same 30 ms
263+
// hold, and the callback logs into it too, so the row can pin one order end to end instead of
264+
// only the keys and a callback count. The release double throws away the second release on
265+
// purpose: only the first acquisition's lock is still held when the retarget happens, and the
266+
// row is about the order, not about releasing a lock the helper already dropped.
267+
const events: string[] = []
236268
vi.mocked(lockfile.lock).mockImplementation(async () => {
237-
acquisitions++
238-
if (acquisitions === 1) resolvedDir = "realDirTwo"
239-
return async () => {}
269+
const acquiredAs = ++acquisitions
270+
events.push("acquire:" + acquiredAs + ":started")
271+
if (acquiredAs === 1) resolvedDir = "realDirTwo"
272+
await new Promise<void>((resolve) => setTimeout(resolve, 30))
273+
events.push("acquire:" + acquiredAs + ":completed")
274+
return async () => {
275+
if (acquiredAs !== 1) {
276+
return
277+
}
278+
events.push("release:started")
279+
await new Promise<void>((resolve) => setTimeout(resolve, 30))
280+
events.push("release:completed")
281+
}
240282
})
241283

242284
const seen: string[] = []
243285
await withFileLock(aliasPath, async (absoluteFilePath) => {
244286
seen.push(absoluteFilePath)
287+
events.push("callback")
245288
})
246289

247290
const keys = vi.mocked(lockfile.lock).mock.calls.map((call) => String(call[0]))
248291
expect(keys).toEqual([path.join(STORE, "realDirOne", "task.json"), path.join(STORE, "realDirTwo", "task.json")])
249292
// The callback still receives the caller's own spelling, and it ran exactly once, after the
250293
// retarget - not under the key that had already become stale.
251294
expect(seen).toEqual([aliasPath])
295+
// The order is the assertion this row was raised about. The stale key's release has to finish
296+
// before the replacement is asked for, and the callback has to run only once the replacement is
297+
// held: either half regressing on its own used to pass here with the key list and the callback
298+
// count both unchanged, because the old double logged nothing.
299+
expect(events).toEqual([
300+
"acquire:1:started",
301+
"acquire:1:completed",
302+
"release:started",
303+
"release:completed",
304+
"acquire:2:started",
305+
"acquire:2:completed",
306+
"callback",
307+
])
252308
})

0 commit comments

Comments
 (0)