Skip to content

Commit 8fb0a8b

Browse files
StellaHuang95Copilot
StellaHuang95
and
Copilot
committed
Make lock reclamation generation-safe
Claim exact stale or retained lock markers before inline cache cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
1 parent 9cf1938 commit 8fb0a8b

3 files changed

Lines changed: 176 additions & 53 deletions

File tree

‎src/common/lockfile.apis.ts‎

Lines changed: 82 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ export interface AcquiredFileLock {
1818

1919
export const FILE_LOCK_DIR_SUFFIX = '.lock';
2020
export const FILE_LOCK_OWNER_MARKER_PREFIX = 'owner-';
21+
export const FILE_LOCK_RETAINED_MARKER_PREFIX = 'retained-';
22+
/** Legacy retained marker. It remains recognizable but cannot be safely reclaimed. */
2123
export const FILE_LOCK_RETAINED_MARKER = 'retained';
2224

2325
export type ProcessLiveness = 'live' | 'dead' | 'unavailable';
@@ -40,7 +42,7 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock
4042
lockPath,
4143
`${FILE_LOCK_OWNER_MARKER_PREFIX}${process.pid}-${crypto.randomBytes(16).toString('hex')}`,
4244
);
43-
const retainedMarker = path.join(lockPath, FILE_LOCK_RETAINED_MARKER);
45+
const retainedMarker = path.join(lockPath, getRetainedMarkerName(path.basename(ownerMarker)));
4446
const deadline = Date.now() + options.timeoutMs;
4547

4648
while (true) {
@@ -69,22 +71,9 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock
6971
}
7072
state = 'retained';
7173
try {
72-
await fsapi.writeFile(retainedMarker, '', { flag: 'wx' });
73-
} catch (error) {
74-
if (hasErrorCode(error, 'EEXIST')) {
75-
return;
76-
}
77-
try {
78-
await fsapi.rename(ownerMarker, retainedMarker);
79-
} catch (renameError) {
80-
if (!hasErrorCode(renameError, 'EEXIST')) {
81-
throw createLockError(
82-
'Failed to mark the lock as retained',
83-
'ERETAINFAILED',
84-
lockPath,
85-
);
86-
}
87-
}
74+
await fsapi.rename(ownerMarker, retainedMarker);
75+
} catch (_error) {
76+
throw createLockError('Failed to mark the lock as retained', 'ERETAINFAILED', lockPath);
8877
}
8978
},
9079
release: async () => {
@@ -119,72 +108,117 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock
119108
}
120109

121110
export async function inspectFileLock(filePath: string, options?: InspectFileLockOptions): Promise<FileLockState> {
111+
return (await inspectFileLockSnapshot(filePath, options)).state;
112+
}
113+
114+
interface FileLockSnapshot {
115+
readonly state: FileLockState;
116+
readonly marker?: string;
117+
readonly markerKind?: 'owner' | 'retained';
118+
}
119+
120+
async function inspectFileLockSnapshot(
121+
filePath: string,
122+
options?: InspectFileLockOptions,
123+
): Promise<FileLockSnapshot> {
122124
const lockPath = getFileLockPath(filePath);
123125

124126
let stat;
125127
try {
126128
stat = await fsapi.lstat(lockPath);
127129
} catch (error) {
128130
if (hasErrorCode(error, 'ENOENT')) {
129-
return 'missing';
131+
return { state: 'missing' };
130132
}
131133
throw error;
132134
}
133135

134136
if (!stat.isDirectory() || stat.isSymbolicLink()) {
135-
return 'malformed';
137+
return { state: 'malformed' };
136138
}
137139

138140
const entries = await fsapi.readdir(lockPath);
139141
const ownerEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX));
142+
const generationRetainedEntries = entries.filter((entry) => entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX));
140143
const retainedEntries = entries.filter((entry) => entry === FILE_LOCK_RETAINED_MARKER);
141144
const unknownEntries = entries.filter(
142-
(entry) => !entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX) && entry !== FILE_LOCK_RETAINED_MARKER,
145+
(entry) =>
146+
!entry.startsWith(FILE_LOCK_OWNER_MARKER_PREFIX) &&
147+
!entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX) &&
148+
entry !== FILE_LOCK_RETAINED_MARKER,
143149
);
144150

145-
if (unknownEntries.length > 0 || ownerEntries.length > 1 || retainedEntries.length > 1) {
146-
return 'malformed';
151+
if (
152+
unknownEntries.length > 0 ||
153+
ownerEntries.length > 1 ||
154+
generationRetainedEntries.length > 1 ||
155+
retainedEntries.length > 1 ||
156+
generationRetainedEntries.length + retainedEntries.length > 1 ||
157+
generationRetainedEntries.length + ownerEntries.length > 1
158+
) {
159+
return { state: 'malformed' };
147160
}
148161
if (retainedEntries.length === 1) {
149-
return 'retained';
162+
return { state: 'retained' };
163+
}
164+
if (generationRetainedEntries.length === 1) {
165+
const retainedPid = parseMarkerPid(generationRetainedEntries[0], FILE_LOCK_RETAINED_MARKER_PREFIX);
166+
if (retainedPid === undefined) {
167+
return { state: 'malformed' };
168+
}
169+
return { state: 'retained', marker: generationRetainedEntries[0], markerKind: 'retained' };
150170
}
151171
if (ownerEntries.length === 1) {
152-
const ownerPid = parseOwnerPid(ownerEntries[0]);
172+
const ownerPid = parseMarkerPid(ownerEntries[0], FILE_LOCK_OWNER_MARKER_PREFIX);
153173
if (ownerPid === undefined) {
154-
return 'malformed';
174+
return { state: 'malformed' };
155175
}
156176
const liveness = await (options?.checkProcessLiveness ?? getProcessLiveness)(ownerPid);
157177
if (liveness === 'dead') {
158-
return 'stale';
178+
return { state: 'stale', marker: ownerEntries[0], markerKind: 'owner' };
159179
}
160-
return liveness === 'live' ? 'held' : 'unavailable';
180+
return { state: liveness === 'live' ? 'held' : 'unavailable', marker: ownerEntries[0], markerKind: 'owner' };
161181
}
162-
return 'orphaned';
182+
return { state: 'orphaned' };
163183
}
164184

165185
/**
166-
* Move a stale or retained lock out of the lock name before a replacement owner is acquired.
167-
* The rename prevents a newly-created lock from being removed based on an earlier inspection.
186+
* Claim and remove the exact observed stale or retained generation without releasing the lock directory.
168187
*/
169-
export async function reclaimFileLock(filePath: string): Promise<boolean> {
188+
export async function reclaimFileLock(filePath: string, options?: InspectFileLockOptions): Promise<boolean> {
170189
const lockPath = getFileLockPath(filePath);
171-
const state = await inspectFileLock(filePath);
172-
if (state !== 'stale' && state !== 'retained') {
190+
const snapshot = await inspectFileLockSnapshot(filePath, options);
191+
if (
192+
(snapshot.state !== 'stale' && snapshot.state !== 'retained') ||
193+
!snapshot.marker ||
194+
!snapshot.markerKind
195+
) {
173196
return false;
174197
}
175198

176-
const quarantinedLockPath = `${lockPath}.reclaimed-${process.pid}-${crypto.randomBytes(16).toString('hex')}`;
199+
const claimedMarker = path.join(
200+
lockPath,
201+
`.reclaim-${process.pid}-${crypto.randomBytes(16).toString('hex')}-${snapshot.marker}`,
202+
);
177203
try {
178-
await fsapi.rename(lockPath, quarantinedLockPath);
204+
await fsapi.rename(path.join(lockPath, snapshot.marker), claimedMarker);
179205
} catch (error) {
180-
if (hasErrorCode(error, 'ENOENT')) {
206+
if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'EEXIST')) {
181207
return false;
182208
}
183209
throw error;
184210
}
185211

186-
await fsapi.remove(quarantinedLockPath);
187-
return true;
212+
try {
213+
await fsapi.unlink(claimedMarker);
214+
await fsapi.rmdir(lockPath);
215+
return true;
216+
} catch (error) {
217+
if (hasErrorCode(error, 'ENOENT') || hasErrorCode(error, 'ENOTEMPTY')) {
218+
return false;
219+
}
220+
throw error;
221+
}
188222
}
189223

190224
export async function getProcessLiveness(pid: number): Promise<ProcessLiveness> {
@@ -204,8 +238,10 @@ export async function getProcessLiveness(pid: number): Promise<ProcessLiveness>
204238

205239
async function isRetainedLock(lockPath: string): Promise<boolean> {
206240
try {
207-
await fsapi.lstat(path.join(lockPath, FILE_LOCK_RETAINED_MARKER));
208-
return true;
241+
const entries = await fsapi.readdir(lockPath);
242+
return entries.some(
243+
(entry) => entry === FILE_LOCK_RETAINED_MARKER || entry.startsWith(FILE_LOCK_RETAINED_MARKER_PREFIX),
244+
);
209245
} catch (error) {
210246
if (hasErrorCode(error, 'ENOENT')) {
211247
return false;
@@ -220,8 +256,12 @@ function hasErrorCode(error: unknown, code: string): boolean {
220256
);
221257
}
222258

223-
function parseOwnerPid(entry: string): number | undefined {
224-
const match = entry.match(new RegExp(`^${escapeRegExp(FILE_LOCK_OWNER_MARKER_PREFIX)}(\\d+)-`));
259+
function getRetainedMarkerName(ownerMarker: string): string {
260+
return `${FILE_LOCK_RETAINED_MARKER_PREFIX}${ownerMarker.slice(FILE_LOCK_OWNER_MARKER_PREFIX.length)}`;
261+
}
262+
263+
function parseMarkerPid(entry: string, prefix: string): number | undefined {
264+
const match = entry.match(new RegExp(`^${escapeRegExp(prefix)}(\\d+)-.+$`));
225265
if (!match) {
226266
return undefined;
227267
}

‎src/test/common/lockfile.apis.unit.test.ts‎

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@ import {
1212
acquireFileLock,
1313
AcquireFileLockOptions,
1414
FILE_LOCK_OWNER_MARKER_PREFIX,
15+
FILE_LOCK_RETAINED_MARKER,
16+
FILE_LOCK_RETAINED_MARKER_PREFIX,
1517
getFileLockPath,
1618
inspectFileLock,
19+
reclaimFileLock,
1720
} from '../../common/lockfile.apis';
1821

1922
const OPTIONS: AcquireFileLockOptions = {
@@ -171,29 +174,29 @@ suite('lockfile APIs', () => {
171174
assert.ok(Date.now() - startedAt < 1_000);
172175
const lockPath = `${path.resolve(targetPath)}.lock`;
173176
const retainedEntries = await fs.readdir(lockPath);
174-
assert.ok(retainedEntries.includes('retained'));
175-
assert.strictEqual(retainedEntries.filter((entry) => entry.startsWith('owner-')).length, 1);
177+
assert.strictEqual(retainedEntries.length, 1);
178+
assert.ok(retainedEntries[0].startsWith(`${FILE_LOCK_RETAINED_MARKER_PREFIX}${process.pid}-`));
176179

177180
await lock.release();
178181
assert.deepStrictEqual(await fs.readdir(lockPath), retainedEntries);
179182
});
180183

181-
test('falls back to renaming the owner marker when the retained sentinel cannot be written', async () => {
184+
test('atomically converts the owner marker into a generation-specific retained marker', async () => {
182185
const lock = await acquireFileLock(targetPath, OPTIONS);
183-
sinon.stub(fsExtra, 'writeFile').rejects(Object.assign(new Error('write failed'), { code: 'EACCES' }));
184186

185187
await lock.retain();
186188

187189
const lockPath = `${path.resolve(targetPath)}.lock`;
188-
assert.deepStrictEqual(await fs.readdir(lockPath), ['retained']);
190+
const entries = await fs.readdir(lockPath);
191+
assert.strictEqual(entries.length, 1);
192+
assert.ok(entries[0].startsWith(`${FILE_LOCK_RETAINED_MARKER_PREFIX}${process.pid}-`));
189193
await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => {
190194
return error.code === 'ELOCKRETAINED';
191195
});
192196
});
193197

194-
test('remains fail-closed when neither retained-marker strategy succeeds', async () => {
198+
test('remains fail-closed when retaining the generation marker fails', async () => {
195199
const lock = await acquireFileLock(targetPath, OPTIONS);
196-
sinon.stub(fsExtra, 'writeFile').rejects(Object.assign(new Error('write failed'), { code: 'EACCES' }));
197200
sinon.stub(fsExtra, 'rename').rejects(Object.assign(new Error('rename failed'), { code: 'EBUSY' }));
198201

199202
await assert.rejects(lock.retain(), (error: NodeJS.ErrnoException) => error.code === 'ERETAINFAILED');
@@ -233,6 +236,70 @@ suite('lockfile APIs', () => {
233236
assert.strictEqual(await inspectFileLock(targetPath), 'retained');
234237
});
235238

239+
test('reclaims a generation-specific retained lock', async () => {
240+
const lock = await acquireFileLock(targetPath, OPTIONS);
241+
await lock.retain();
242+
243+
assert.strictEqual(await reclaimFileLock(targetPath), true);
244+
assert.strictEqual(await inspectFileLock(targetPath), 'missing');
245+
const replacement = await acquireFileLock(targetPath, OPTIONS);
246+
await replacement.release();
247+
});
248+
249+
test('refuses to reclaim the ambiguous legacy retained marker', async () => {
250+
const lockPath = getFileLockPath(targetPath);
251+
await fs.ensureDir(lockPath);
252+
await fs.writeFile(path.join(lockPath, `${FILE_LOCK_OWNER_MARKER_PREFIX}${process.pid}-legacy`), '');
253+
await fs.writeFile(path.join(lockPath, FILE_LOCK_RETAINED_MARKER), '');
254+
255+
assert.strictEqual(await inspectFileLock(targetPath), 'retained');
256+
assert.strictEqual(await reclaimFileLock(targetPath), false);
257+
assert.strictEqual(await fs.pathExists(path.join(lockPath, FILE_LOCK_RETAINED_MARKER)), true);
258+
await assert.rejects(acquireFileLock(targetPath, OPTIONS), (error: NodeJS.ErrnoException) => {
259+
return error.code === 'ELOCKRETAINED';
260+
});
261+
});
262+
263+
test('does not touch a new generation when a delayed reclaimer loses its marker claim', async () => {
264+
const lockPath = getFileLockPath(targetPath);
265+
const staleMarker = `${FILE_LOCK_OWNER_MARKER_PREFIX}424242-dead`;
266+
await fs.ensureDir(targetPath);
267+
await fs.ensureDir(lockPath);
268+
await fs.writeFile(path.join(lockPath, staleMarker), '');
269+
const rename = fsExtra.rename;
270+
let releaseFirstClaim: (() => void) | undefined;
271+
let firstClaimStarted: (() => void) | undefined;
272+
const firstClaim = new Promise<void>((resolve) => {
273+
firstClaimStarted = resolve;
274+
});
275+
const releaseClaim = new Promise<void>((resolve) => {
276+
releaseFirstClaim = resolve;
277+
});
278+
let renameCount = 0;
279+
sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => {
280+
renameCount += 1;
281+
if (renameCount === 1) {
282+
firstClaimStarted!();
283+
await releaseClaim;
284+
}
285+
await rename(source, destination);
286+
});
287+
288+
const staleInspection = { checkProcessLiveness: sinon.stub().resolves('dead') };
289+
const delayedReclaimer = reclaimFileLock(targetPath, staleInspection);
290+
await firstClaim;
291+
assert.strictEqual(await reclaimFileLock(targetPath, staleInspection), true);
292+
const replacement = await acquireFileLock(targetPath, OPTIONS);
293+
const replacementEntries = await fs.readdir(lockPath);
294+
295+
releaseFirstClaim!();
296+
assert.strictEqual(await delayedReclaimer, false);
297+
assert.deepStrictEqual(await fs.readdir(lockPath), replacementEntries);
298+
assert.strictEqual(await fs.pathExists(targetPath), true);
299+
300+
await replacement.release();
301+
});
302+
236303
test('classifies a dead owner marker as stale using the liveness probe', async () => {
237304
const lockPath = getFileLockPath(targetPath);
238305
await fs.ensureDir(lockPath);

‎src/test/managers/builtin/inlineScript/envManager.unit.test.ts‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2334,18 +2334,34 @@ suite('InlineScriptEnvManager', () => {
23342334
assert.strictEqual(await manager.get(uri), environment);
23352335
});
23362336

2337-
test('clears a retained lock and its corresponding cache entry', async () => {
2337+
test('clears a generation-specific retained lock and its corresponding cache entry', async () => {
2338+
lockStub.restore();
2339+
const retainedCacheDir = envDir().fsPath;
2340+
await fs.outputFile(venvPythonPath(retainedCacheDir), '');
2341+
const lock = await lockfileApis.acquireFileLock(retainedCacheDir, {
2342+
timeoutMs: 0,
2343+
retryIntervalMs: 1,
2344+
});
2345+
await lock.retain();
2346+
2347+
await manager.clearCache();
2348+
2349+
assert.strictEqual(await fs.pathExists(retainedCacheDir), false);
2350+
assert.strictEqual(await fs.pathExists(lockfileApis.getFileLockPath(retainedCacheDir)), false);
2351+
});
2352+
2353+
test('refuses to clear a legacy retained lock conservatively', async () => {
23382354
lockStub.restore();
23392355
const retainedCacheDir = envDir().fsPath;
23402356
const retainedLockPath = lockfileApis.getFileLockPath(retainedCacheDir);
23412357
await fs.outputFile(venvPythonPath(retainedCacheDir), '');
23422358
await fs.ensureDir(retainedLockPath);
23432359
await fs.writeFile(path.join(retainedLockPath, 'retained'), '');
23442360

2345-
await manager.clearCache();
2361+
await assert.rejects(manager.clearCache(), /incomplete or malformed/);
23462362

2347-
assert.strictEqual(await fs.pathExists(retainedCacheDir), false);
2348-
assert.strictEqual(await fs.pathExists(retainedLockPath), false);
2363+
assert.strictEqual(await fs.pathExists(retainedCacheDir), true);
2364+
assert.strictEqual(await fs.pathExists(retainedLockPath), true);
23492365
});
23502366

23512367
test('clears a stale owner lock and its corresponding cache entry', async () => {

0 commit comments

Comments
 (0)