Skip to content

Commit 3fcee16

Browse files
author
easonLiangWorldedtech
committed
feat(file-safety): route the merge write through the publish primitive under a canonical lock key (U2, #1375)
Split unit U2 of #1833. The lock key is the resolved publish target, so a symlink alias and its referent share one lock file, and the safety-net cleanup no longer logs the expected miss when the temp file is already gone.
1 parent 67f8a8c commit 3fcee16

3 files changed

Lines changed: 424 additions & 114 deletions

File tree

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
// npx vitest run utils/__tests__/safeWriteJson.lockKey.spec.ts
2+
3+
import * as os from "os"
4+
import path from "path"
5+
import type { BigIntStats } from "fs"
6+
import * as fs from "fs/promises"
7+
import { acquireFileLock } from "../fileLock"
8+
import { safeWriteJson } from "../safeWriteJson"
9+
import { resolveLockKey } from "../../services/file-safety/safeWriteText"
10+
11+
vi.mock("../fileLock", () => ({
12+
acquireFileLock: vi.fn(async () => async () => {}),
13+
}))
14+
15+
vi.mock("fs/promises", async () => {
16+
const actual = await vi.importActual<typeof import("fs/promises")>("fs/promises")
17+
return { ...actual, realpath: vi.fn(), lstat: vi.fn(), readlink: vi.fn() }
18+
})
19+
20+
const mockedRealpath = vi.mocked(fs.realpath)
21+
const mockedLstat = vi.mocked(fs.lstat)
22+
const mockedReadlink = vi.mocked(fs.readlink)
23+
const mockedAcquireFileLock = vi.mocked(acquireFileLock)
24+
25+
const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" })
26+
27+
// Each test creates a real temp directory so the real fs calls still work.
28+
// doubles between tests so an implementation from one test cannot carry over.
29+
const createdDirs: string[] = []
30+
async function makeDir(prefix: string): Promise<string> {
31+
const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix))
32+
createdDirs.push(dir)
33+
return dir
34+
}
35+
36+
beforeEach(() => {
37+
mockedRealpath.mockReset()
38+
mockedLstat.mockReset()
39+
mockedReadlink.mockReset()
40+
mockedAcquireFileLock.mockReset()
41+
})
42+
43+
afterEach(async () => {
44+
for (const dir of createdDirs) {
45+
await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined)
46+
}
47+
createdDirs.length = 0
48+
})
49+
50+
// Only isSymbolicLink() is consulted by the guard, so the double carries just
51+
// that method. The mocks reject asynchronously: a synchronous throw would bypass
52+
// resolvePublishTarget's catch and skip the ENOENT/symlink branch under test.
53+
const symlinkStat = (target: unknown) => ({ isSymbolicLink: () => target === currentLink }) as unknown as BigIntStats
54+
let currentLink = ""
55+
56+
describe("safeWriteJson lock key under a peer commit", () => {
57+
it("waits for the peer instead of rejecting, and locks the referent", async () => {
58+
const order: string[] = []
59+
const dir = await makeDir("lockkey-")
60+
const referent = path.join(dir, "history_item.json")
61+
currentLink = path.join(dir, "link.json")
62+
63+
// The peer writer has renamed the referent away and has not committed yet,
64+
// so the first resolution fails with ENOENT while lstat still reports a
65+
// symbolic link. A strict resolve here rejects the caller before it can ever
66+
// queue behind the peer, and the caller's delta write is lost.
67+
mockedRealpath
68+
.mockImplementationOnce(async () => {
69+
order.push("resolve-failed")
70+
throw enoent
71+
})
72+
.mockImplementation(async (target) => {
73+
order.push("resolve")
74+
// The second call happens under the lock, where the peer has committed.
75+
return target === currentLink ? referent : String(target)
76+
})
77+
mockedLstat.mockImplementation(async (target) => {
78+
order.push("lstat")
79+
return symlinkStat(target)
80+
})
81+
mockedReadlink.mockImplementation(async (target) =>
82+
target === currentLink ? referent : Promise.reject(new Error("not a link")),
83+
)
84+
mockedAcquireFileLock.mockImplementation(async () => {
85+
order.push("lock")
86+
return async () => {}
87+
})
88+
89+
await safeWriteJson(currentLink, { id: "task-1" })
90+
91+
// The lock key is the key every other writer to this file uses, so the caller
92+
// queued behind the peer instead of failing before the lock.
93+
expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent)
94+
expect(order).toEqual(["resolve-failed", "lstat", "resolve", "resolve", "lock", "resolve", "resolve"])
95+
expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" })
96+
})
97+
98+
it("releases the lock when the resolution under the lock rejects", async () => {
99+
const order: string[] = []
100+
let released = false
101+
const dir = await makeDir("lockkey-")
102+
const referent = path.join(dir, "history_item.json")
103+
currentLink = path.join(dir, "link.json")
104+
105+
// A real dangling link: the walk tolerates it so the caller can queue behind
106+
// the peer, but once the lock is held the strict rejection still applies. A
107+
// rejection outside the protected block would leave the lock held until the
108+
// stale timeout for every other writer to the same file.
109+
mockedRealpath.mockImplementation(async () => {
110+
throw enoent
111+
})
112+
mockedLstat.mockImplementation(async (target) => {
113+
order.push("lstat")
114+
return symlinkStat(target)
115+
})
116+
mockedReadlink.mockImplementation(async (target) =>
117+
target === currentLink ? referent : Promise.reject(new Error("not a link")),
118+
)
119+
mockedAcquireFileLock.mockImplementation(async () => {
120+
order.push("lock")
121+
return async () => {
122+
order.push("release")
123+
released = true
124+
}
125+
})
126+
127+
await expect(safeWriteJson(currentLink, { id: "task-1" })).rejects.toThrow(enoent)
128+
expect(released).toBe(true)
129+
// The strict rejection is reached through the ENOENT + symlink branch, not
130+
// through a synchronous throw that skips it.
131+
expect(order).toEqual(["lstat", "lock", "lstat", "release"])
132+
})
133+
134+
it("canonicalizes the parent directory when the file itself is not there yet", async () => {
135+
// fs.realpath canonicalizes every component, including a symlinked ancestor
136+
// directory or a Windows 8.3 short name. If the fallback returns the alias
137+
// directory, the key depends on whether the file exists at the moment the key
138+
// is computed, and a writer that resolved the canonical directory takes a
139+
// different lock for the same file.
140+
const aliasDir = path.join(os.tmpdir(), "alias-dir")
141+
const canonicalDir = path.join(os.tmpdir(), "canonical-dir")
142+
const file = path.join(aliasDir, "history_item.json")
143+
mockedRealpath.mockImplementation(async (target) => {
144+
if (target === file) throw enoent
145+
return canonicalDir
146+
})
147+
mockedLstat.mockImplementation(async () => ({ isSymbolicLink: () => false }) as unknown as BigIntStats)
148+
149+
expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json"))
150+
})
151+
})
152+
153+
it("does not log a cleanup error when the safety net finds the temp file already gone", async () => {
154+
// safeWriteText removes its own temp file on failure, so the safety net in
155+
// safeWriteJson normally finds it gone. That is the expected outcome, not a
156+
// second failure, and it must not be logged as one.
157+
const dir = await makeDir("cleanup-")
158+
const target = path.join(dir, "history_item.json")
159+
currentLink = ""
160+
mockedRealpath.mockImplementation(async (t) => String(t))
161+
mockedLstat.mockImplementation(async (t) => symlinkStat(t))
162+
163+
const renameSpy = vi.spyOn(fs, "rename").mockRejectedValue(new Error("commit rename failed"))
164+
const unlinkSpy = vi.spyOn(fs, "unlink").mockRejectedValue(enoent)
165+
const consoleError = vi.spyOn(console, "error").mockImplementation(() => {})
166+
167+
await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed")
168+
169+
// Only the original failure is reported.
170+
expect(consoleError).toHaveBeenCalledTimes(1)
171+
172+
renameSpy.mockRestore()
173+
unlinkSpy.mockRestore()
174+
consoleError.mockRestore()
175+
})

0 commit comments

Comments
 (0)