Skip to content

Commit 2c7460d

Browse files
fix(u5a): handle the reserved slot rejection at creation, keep the observation on the measured token, and propagate the lock-key recheck failure
Three review rows on this pull are live against the content the branch head serves, and each was settled by a row that fails without the change and passes with it. All four new rows were run against the unchanged head first and failed as quoted; after the change the utils suite and the four guardedWrite specs together print 54 test files passed, 993 tests passed and 5 skipped. 1. src/core/tools/guardedWrite.ts. The containment re-check releases the reserved chain slot while an earlier write still holds the path, so the slot work promise was rejected before its own chain entry could await it, and Node reports an unhandled rejection for a promise whose first handler attaches on a later turn. The handler is attached at creation instead; it does not consume the rejection, and the new row asserts both that nothing was reported unhandled and that the refusal still reaches the caller. Pre-fix the row printed that the captured list held one entry while the assertion demanded none. 2. src/core/tools/guardedWrite.ts. The durability-failure refresh fell back to a token read taken after the lock was released, which is the read the record under the lock exists to avoid. The refresh now reports only a token measured under the lock. Pre-fix the new row printed that the token computation was called 2 times expected but 3 times actual. 3. src/utils/fileLock.ts. A failed lock-key re-derivation was swallowed as though it proved the key had not moved, and the retarget path discarded its release failure. A failed inspection now releases and propagates, and a failed release is reported with the key it could not give up and the error itself. Pre-fix the two new rows printed that no error was thrown where an EACCES inspection failure was demanded, and that no console report existed where one was demanded. Gates at this commit: prettier check exit 0 on the five touched files; eslint with prune-suppressions and max-warnings 0 exit 0 with src/eslint-suppressions.json left byte identical to blob f3714cd; tsc noEmit over src/tsconfig.json exit 0. No Stryker directive, suppression entry or exclusion added.
1 parent 20eee8f commit 2c7460d

5 files changed

Lines changed: 189 additions & 4 deletions

File tree

‎src/core/tools/__tests__/guardedWriteMutationGaps.spec.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,28 @@ describe("guardedWrite mutation gaps (epic #1375)", () => {
406406
expect(reg.get(abs("doc.txt"))?.version).toBe("v1")
407407
})
408408

409+
it("does not refresh from a token read taken after the lock is released", async () => {
410+
const reg = new ObservationRegistry()
411+
reg.observe(abs("doc.txt"), "v1", true)
412+
const task = createMockTask({ observationRegistry: reg })
413+
// Three answers in order: the pre-write comparison, the read this publish makes while it
414+
// still holds the lock - which fails here, so nothing is recorded - and the value a read
415+
// taken after the release would report, when a peer can have published in the gap.
416+
mockedComputeVersionToken
417+
.mockResolvedValueOnce("v1")
418+
.mockRejectedValueOnce(errno("EIO"))
419+
.mockResolvedValue("peer-writer-token")
420+
mockedSafeWriteText.mockRejectedValue(new PostCommitDurabilityError(abs("doc.txt"), errno("EIO")))
421+
422+
await expect(guardedWrite(task, "doc.txt", "patched", "edit")).rejects.toThrow(PostCommitDurabilityError)
423+
424+
// Two token reads belong to this flow: the comparison and the under-lock read that failed.
425+
// The third read is the one taken outside the lock, and its value would stand for a publish
426+
// this write never measured, which is the mistake the record under the lock exists to avoid.
427+
expect(mockedComputeVersionToken).toHaveBeenCalledTimes(2)
428+
expect(reg.get(abs("doc.txt"))?.version).toBe("v1")
429+
})
430+
409431
it("keeps a partial observation partial when a durability failure refreshes it", async () => {
410432
const reg = new ObservationRegistry()
411433
reg.observe(abs("doc.txt"), "v1", false)

‎src/core/tools/__tests__/guardedWritePostCheckCancellation.spec.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,5 +302,44 @@ describe("guardedWrite post-check cancellation gaps (epic #1375)", () => {
302302
expect(settled.outcome).toBe("published")
303303
expect(mockedSafeWriteText).toHaveBeenCalledTimes(1)
304304
})
305+
306+
it("leaves no unhandled rejection when the refusal releases a still-queued chain entry", async () => {
307+
// The refusal at guardedWrite.ts line 701 rejects the reserved slot's own work promise while
308+
// this write's chain entry is still queued behind the holder's, so the entry has not reached
309+
// the await at line 150 yet and the rejected promise has no handler on it. Node's
310+
// unhandled-rejection check runs on the current turn, long before the queued turn arrives, so
311+
// the handler has to exist at creation rather than when the entry finally runs.
312+
const unhandled: string[] = []
313+
const listener = (reason: unknown): void => {
314+
if (reason instanceof GuardRejectedError && reason.message === CANCELLED) {
315+
unhandled.push(reason.message)
316+
}
317+
}
318+
process.on("unhandledRejection", listener)
319+
try {
320+
const holder = createCancellableTask()
321+
const held = guardedWrite(holder.task, "new.txt", "data", "create").catch((error: unknown) => error)
322+
await waitUntilOpen()
323+
324+
const canceller = createCancellableTask()
325+
flipOnNextCheck(canceller.abortNow)
326+
const answer = recordAnswer(guardedWrite(canceller.task, "new.txt", "data", "create"))
327+
328+
// Event-loop turns pass here while the holder's publish is still open, which is the window
329+
// the check runs in: the queued entry cannot reach its await before the timer fires.
330+
await delay(30)
331+
expect(unhandled.join(" | ")).toBe("")
332+
333+
release.open()
334+
await held
335+
const settled = await answer.settled
336+
// The handler must not consume the verdict: this caller still receives the refusal, and the
337+
// writer still ran exactly once, for the holder's publish.
338+
expect(asGuardRejection((settled as { rejection?: unknown }).rejection).message).toBe(CANCELLED)
339+
expect(mockedSafeWriteText).toHaveBeenCalledTimes(1)
340+
} finally {
341+
process.off("unhandledRejection", listener)
342+
}
343+
})
305344
})
306345
})

‎src/core/tools/guardedWrite.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,13 @@ function reserveChainSlot(pathKey: string): {
143143
settle = resolve
144144
fail = reject
145145
})
146+
// The refusal path below releases this reservation while an earlier write still holds the
147+
// chain, so the entry under this promise has not reached its await yet and the rejected
148+
// promise has no handler on the turn it is rejected on. Node reports that as an unhandled
149+
// rejection even though the caller's own promise carries the failure. This handler does not
150+
// consume the rejection: the await below still rejects, and settled stays the promise the
151+
// caller receives.
152+
void held.catch(() => {})
146153
// The chain entry opens on its turn and then waits for the work it released, so a later
147154
// writer to the same path cannot overtake this one.
148155
const settled = enqueue(pathKey, async () => {
@@ -732,8 +739,12 @@ export async function guardedWrite(
732739
if (!(error instanceof PostCommitDurabilityError)) {
733740
return
734741
}
735-
const committed =
736-
committedTokenFor(error) ?? (await computeVersionToken(absolutePath).catch(() => undefined))
742+
// Only a token measured while this write still held the lock may stand for this publish.
743+
// A read taken after the release can pick up a peer's newer publish, which is the mistake
744+
// the record above exists to avoid, so an unrecorded durability failure leaves the
745+
// observation on the token the task was shown rather than moving it to a value this
746+
// write never measured.
747+
const committed = committedTokenFor(error)
737748
if (committed !== undefined) {
738749
task.observationRegistry.observe(
739750
absolutePath,

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,3 +288,95 @@ test("retakes the lock when the canonical key moves while the callback waits to
288288
// become stale.
289289
expect(seen).toEqual([aliasPath])
290290
})
291+
292+
test("propagates a lock-key recheck failure instead of running under a key it could not confirm", async () => {
293+
// The re-derivation at fileLock.ts line 206 is what proves the key this call holds is the key a
294+
// peer reaching the same file by another spelling is taking. When the inspection itself fails
295+
// the canonical key is unknown, and an unknown key is not the same fact as an unchanged key:
296+
// canonicalLockPath already refuses to invent a key for exactly this reason on its first call,
297+
// and a walk that fails after the lock is held has to refuse on the same rule rather than let
298+
// the mutation run under a lock no peer is contending for.
299+
vi.mocked(lockfile.lock).mockClear()
300+
vi.mocked(fsPromises.readlink).mockClear()
301+
vi.mocked(fsPromises.realpath).mockClear()
302+
const callerPath = path.join(STORE, "aliasDir", "task.json")
303+
const events: string[] = []
304+
let inspections = 0
305+
vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) => {
306+
inspections += 1
307+
if (inspections > 1) {
308+
throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })
309+
}
310+
return String(target).replace("aliasDir", "realDir")
311+
})
312+
vi.mocked(lockfile.lock).mockImplementation(async () => {
313+
events.push("acquire")
314+
return async () => {
315+
events.push("release")
316+
}
317+
})
318+
319+
let ranCallback = false
320+
const thrown = await withFileLock(callerPath, async () => {
321+
ranCallback = true
322+
events.push("callback")
323+
}).catch((error: unknown) => error)
324+
325+
expect(thrown).toMatchObject({ code: "EACCES" })
326+
// Held, then given up: the callback never ran, so nothing mutated under a key whose canonical
327+
// form this call could not confirm.
328+
expect(events).toEqual(["acquire", "release"])
329+
expect(ranCallback).toBe(false)
330+
})
331+
332+
test("reports a release failure on the retarget path instead of swallowing it", async () => {
333+
// The first acquisition below holds the key that goes stale mid-call, and its release now
334+
// fails. Dropping that failure means a peer keeps waiting on a lock this call believes it gave
335+
// up, with nothing recorded about which key or why. The success path and the error path in this
336+
// same function both report a release failure, so the retarget path reporting nothing is an
337+
// inconsistency rather than a decision.
338+
vi.mocked(lockfile.lock).mockClear()
339+
vi.mocked(fsPromises.readlink).mockClear()
340+
vi.mocked(fsPromises.realpath).mockClear()
341+
const aliasPath = path.join(STORE, "aliasDir", "task.json")
342+
let resolvedDir = "realDirOne"
343+
let acquisitions = 0
344+
const releaseFailure = Object.assign(new Error("EBUSY: lock file still held"), { code: "EBUSY" })
345+
const events: string[] = []
346+
vi.mocked(fsPromises.realpath).mockImplementation(async (target: unknown) =>
347+
String(target).replace("aliasDir", resolvedDir),
348+
)
349+
vi.mocked(lockfile.lock).mockImplementation(async () => {
350+
const acquiredAs = ++acquisitions
351+
events.push("acquire:" + acquiredAs)
352+
if (acquiredAs === 1) {
353+
resolvedDir = "realDirTwo"
354+
}
355+
return async () => {
356+
if (acquiredAs === 1) {
357+
events.push("release:1:failed")
358+
throw releaseFailure
359+
}
360+
events.push("release:2")
361+
}
362+
})
363+
364+
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {})
365+
const seen: string[] = []
366+
await withFileLock(aliasPath, async (absoluteFilePath) => {
367+
events.push("callback")
368+
seen.push(absoluteFilePath)
369+
})
370+
const reports = errorSpy.mock.calls.map((call) => String(call[0]))
371+
const carried = errorSpy.mock.calls.map((call) => call[1])
372+
errorSpy.mockRestore()
373+
374+
// The retarget still happened and the callback still ran, under the new key.
375+
expect(events).toEqual(["acquire:1", "release:1:failed", "acquire:2", "callback", "release:2"])
376+
expect(seen).toEqual([aliasPath])
377+
// The failure is reported once, naming the key that failed to release, and carrying the error
378+
// object itself rather than a paraphrase of it.
379+
expect(reports.length).toBe(1)
380+
expect(reports[0]).toContain(path.join(STORE, "realDirOne", "task.json"))
381+
expect(carried[0]).toBe(releaseFailure)
382+
})

‎src/utils/fileLock.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,9 +203,30 @@ export async function withFileLock<T>(
203203
// contending for, and a concurrent merge could recreate what the callback deletes. Re-derive
204204
// the key under the same rules before running anything, and on a change release, retake the
205205
// lock for the new key, and only then run the callback.
206-
const currentKey = await canonicalLockPath(operationPath).catch(() => lockKey)
206+
// The re-derivation has to answer. A failed inspection leaves the canonical key unknown,
207+
// which is not the same fact as an unchanged key: the mutation would run under a lock no
208+
// peer reaching the file by its canonical spelling contends for. Give up the lock and
209+
// propagate, the same way the first canonicalization above refuses to invent a key.
210+
let currentKey: string
211+
try {
212+
currentKey = await canonicalLockPath(operationPath)
213+
} catch (recheckError: unknown) {
214+
try {
215+
await releaseLock()
216+
} catch (releaseError: unknown) {
217+
console.error(`Failed to release the lock for ${lockKey} after a failed recheck:`, releaseError)
218+
}
219+
throw recheckError
220+
}
207221
if (currentKey !== lockKey) {
208-
await releaseLock().catch(() => undefined)
222+
// The old key may still be held, so report which key this call could not give up and
223+
// carry the failure itself: a peer waiting on that key has to be able to see why the
224+
// release did not complete, rather than wait out the staleness timeout unexplained.
225+
try {
226+
await releaseLock()
227+
} catch (releaseError: unknown) {
228+
console.error(`Failed to release the lock for ${lockKey} before retargeting:`, releaseError)
229+
}
209230
if (attempt >= MAX_LOCK_KEY_RETARGETS) {
210231
throw new Error(
211232
`Lock key for ${operationPath} kept moving across ${MAX_LOCK_KEY_RETARGETS} retargets -- the operation was not run.`,

0 commit comments

Comments
 (0)