Skip to content

Commit c480143

Browse files
u917 recut rung 1: the safe-write primitives with the tests that describe them and the inherited delete-semantics expectation
Runner-only test fix: the delete-lock assertion compared the lock key by text against a path this file canonicalized with fs/promises realpath. delete() hands deleteJsonFile a joined path and deleteJsonFile hands it to withFileLock, which only path.resolve-s it, so the spied key carries the spelling the store was built from, while the expectation carried a realpath spelling. On the Windows runner those are two spellings of one file (C:/Users/runneradmin/... and its NTFS 8.3 alias C:/Users/RUNNER~1/...), so the case passed here and failed there at TaskHistoryStore.deleteSemantics.spec.ts:174. The expectation is now pinned by file identity plus case-folded basename, which is how the landed src/utils/__tests__/safeWriteJson.interruption.test.ts compares one file across both spellings, with the call count, arity and function body pinned alongside it.
1 parent f98fcf7 commit c480143

9 files changed

Lines changed: 3190 additions & 30 deletions

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

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,40 @@ function historyFilePath(storagePath: string, taskId: string): string {
5454
return path.join(storagePath, "tasks", taskId, GlobalFileNames.historyItem)
5555
}
5656

57+
// The delete locks the file it unlinks, and the spelling of that path is whatever the
58+
// store was given: `delete` hands `deleteJsonFile` a joined path, `deleteJsonFile` hands it
59+
// to `withFileLock`, and `withFileLock` only `path.resolve`s it -- the canonicalisation
60+
// through fs/promises realpath happens deeper, when the lock file itself is placed, so it
61+
// never reaches this spy. On the Windows runner the two spellings of one temp file differ
62+
// (C:/Users/runneradmin/... and its NTFS 8.3 alias C:/Users/RUNNER~1/...), so comparing the
63+
// key by text holds on a developer box and misses there. File identity is the comparison
64+
// that holds on both spellings, which is how the landed
65+
// src/utils/__tests__/safeWriteJson.interruption.test.ts handles the same hazard.
66+
async function fileIdentity(candidate: string): Promise<string | null> {
67+
try {
68+
const stats = await actualFs.stat(candidate)
69+
return `${stats.dev}:${stats.ino}`
70+
} catch {
71+
return null
72+
}
73+
}
74+
75+
async function lockKeyNamesTheSameFile(lockKey: unknown, expectedPath: string): Promise<boolean> {
76+
if (typeof lockKey !== "string") {
77+
return false
78+
}
79+
// The unlink removes the file and leaves the per-task directory in place, so both sides
80+
// still carry an identity here. Case is folded because it is not significant on Windows,
81+
// where the alias spelling appears.
82+
const expectedDirectory = await fileIdentity(path.dirname(expectedPath))
83+
const keyDirectory = await fileIdentity(path.dirname(lockKey))
84+
return (
85+
expectedDirectory !== null &&
86+
keyDirectory === expectedDirectory &&
87+
path.basename(lockKey).toLowerCase() === path.basename(expectedPath).toLowerCase()
88+
)
89+
}
90+
5791
function storeInternals(store: TaskHistoryStore): {
5892
cache: Map<string, HistoryItem>
5993
taskFileMtimes: Map<string, number>
@@ -152,14 +186,23 @@ describe("TaskHistoryStore best-effort deletion semantics", () => {
152186
const store = createStore()
153187
await store.initialize()
154188
await store.upsert(makeHistoryItem({ id: "locked-delete" }))
189+
// The lock spy is created once for the whole file, so clear it to make the
190+
// assertion below about this delete and not about every case before it.
191+
vi.mocked(withFileLock).mockClear()
155192
onWrite.mockClear()
156193

157194
await expect(store.delete("locked-delete")).resolves.toBeUndefined()
158195

159-
expect(vi.mocked(withFileLock)).toHaveBeenCalledWith(
160-
historyFilePath(storagePath, "locked-delete"),
161-
expect.any(Function),
196+
const lockCalls = vi.mocked(withFileLock).mock.calls
197+
expect(lockCalls).toHaveLength(1)
198+
// The lock is taken for the file the delete unlinked, under a function body: the key
199+
// is pinned by identity rather than by text, and the arity and the body are pinned
200+
// alongside it so an argument dropped or swapped cannot slip through.
201+
expect(lockCalls[0]).toHaveLength(2)
202+
expect(await lockKeyNamesTheSameFile(lockCalls[0][0], historyFilePath(storagePath, "locked-delete"))).toBe(
203+
true,
162204
)
205+
expect(lockCalls[0][1]).toBeTypeOf("function")
163206
await expect(fs.access(historyFilePath(storagePath, "locked-delete"))).rejects.toMatchObject({
164207
code: "ENOENT",
165208
})
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import * as fs from "fs/promises"
2+
import * as os from "os"
3+
import * as path from "path"
4+
5+
import { safeWriteText } from "../safeWriteText"
6+
7+
// No fs mocks in this file: the point is to assert what a real filesystem ends up
8+
// holding after a publish attempt, which the mocked spec cannot show. The failure is
9+
// provoked with real filesystem semantics rather than with a stubbed call.
10+
describe("safeWriteText against a real filesystem", () => {
11+
let dir: string
12+
13+
beforeEach(async () => {
14+
dir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-text-int-"))
15+
})
16+
17+
afterEach(async () => {
18+
await fs.rm(dir, { recursive: true, force: true })
19+
})
20+
21+
it("publishes the new bytes and leaves no staging or backup residue", async () => {
22+
const targetPath = path.join(dir, "target.txt")
23+
await fs.writeFile(targetPath, "old bytes")
24+
25+
// No platform override: the real platform's own durability and ACL steps run.
26+
// A failed icacls restore in a throwaway temp directory is reported, not thrown,
27+
// so the publish still lands.
28+
await safeWriteText(targetPath, "new bytes", { backup: true })
29+
30+
expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes")
31+
expect(await fs.readdir(dir)).toEqual(["target.txt"])
32+
})
33+
34+
it("leaves the target bytes untouched when the commit cannot replace it", async () => {
35+
// A regular file cannot be renamed over a directory, so the commit rename itself fails on a
36+
// real filesystem with no mocking at all. No backup is asked for: with backup:true the run
37+
// stopped one step earlier, at the backup copy hitting the same EISDIR, and the commit rename
38+
// this test is named for never ran.
39+
const targetPath = path.join(dir, "target-dir")
40+
await fs.mkdir(targetPath)
41+
const inside = path.join(targetPath, "payload.txt")
42+
await fs.writeFile(inside, "original bytes")
43+
44+
await expect(safeWriteText(targetPath, "new data")).rejects.toThrow()
45+
46+
// The directory and its content are exactly as they were, and no backup copy
47+
// or staging directory was left behind next to them.
48+
expect(await fs.readFile(inside, "utf8")).toBe("original bytes")
49+
expect(await fs.readdir(dir)).toEqual(["target-dir"])
50+
})
51+
})

0 commit comments

Comments
 (0)