Skip to content

Commit 5e043b1

Browse files
StellaHuang95Copilot
StellaHuang95
and
Copilot
committed
fix: serialize persistent-state writes and concurrent lock releases (review feedback)
- persistentState.set() now joins the clear queue as the new tail so a later clear() cannot be resurrected by a lagging write (review r3858573315 / r3858651583). - Inline lock release() is serialized through a shared in-flight promise so two concurrent releases no longer race and spuriously report ECOMPROMISED; the handle stays retryable after a failed release (review r3858573008 / r3858434379). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b7253a6a-a3c5-4606-b9cf-ec52db1e605f
1 parent bddd428 commit 5e043b1

4 files changed

Lines changed: 134 additions & 55 deletions

File tree

‎src/common/lockfile.apis.ts‎

Lines changed: 61 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,56 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock
7070
}
7171

7272
let state: LockState = 'held';
73+
let releaseInFlight: Promise<void> | undefined;
74+
const performRelease = async (): Promise<void> => {
75+
if (state === 'released' || state === 'retained') {
76+
return;
77+
}
78+
const releaseMarkerPath = path.join(lockPath, releaseMarkerName);
79+
if (state === 'held') {
80+
try {
81+
await fsapi.rename(ownerMarker, releaseMarkerPath);
82+
} catch (error) {
83+
if (hasErrorCode(error, 'ENOENT')) {
84+
throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath);
85+
}
86+
throw error;
87+
}
88+
state = 'releasing';
89+
}
90+
// state === 'releasing': the release marker exists; retire the canonical
91+
// directory. Resumable: if retirement fails and ownership cannot be restored,
92+
// the handle stays 'releasing' so a later release() retries retirement instead
93+
// of leaving the handle unable to make progress.
94+
const retiredPath = getRetiredLockPath(lockPath);
95+
try {
96+
await retireCanonicalLockDirectory(lockPath, retiredPath);
97+
} catch (error) {
98+
const restored = await fsapi
99+
.rename(releaseMarkerPath, ownerMarker)
100+
.then(
101+
() => true,
102+
() => false,
103+
);
104+
if (restored) {
105+
state = 'held';
106+
throw createLockError(
107+
'Failed to retire the lock directory; ownership was restored',
108+
'ELOCKRELEASEFAILED',
109+
lockPath,
110+
error,
111+
);
112+
}
113+
throw createLockError(
114+
'Failed to retire the lock directory; release can be retried',
115+
'ELOCKRELEASEFAILED',
116+
lockPath,
117+
error,
118+
);
119+
}
120+
state = 'released';
121+
await cleanupRetiredLock(retiredPath, releaseMarkerName);
122+
};
73123
return {
74124
retain: async () => {
75125
if (state !== 'held') {
@@ -82,54 +132,18 @@ export async function acquireFileLock(filePath: string, options: AcquireFileLock
82132
throw createLockError('Failed to mark the lock as retained', 'ERETAINFAILED', lockPath);
83133
}
84134
},
85-
release: async () => {
86-
if (state === 'released' || state === 'retained') {
87-
return;
88-
}
89-
const releaseMarkerPath = path.join(lockPath, releaseMarkerName);
90-
if (state === 'held') {
91-
try {
92-
await fsapi.rename(ownerMarker, releaseMarkerPath);
93-
} catch (error) {
94-
if (hasErrorCode(error, 'ENOENT')) {
95-
throw createLockError('Lock ownership was compromised', 'ECOMPROMISED', lockPath);
96-
}
97-
throw error;
98-
}
99-
state = 'releasing';
100-
}
101-
// state === 'releasing': the release marker exists; retire the canonical
102-
// directory. Resumable: if retirement fails and ownership cannot be restored,
103-
// the handle stays 'releasing' so a later release() retries retirement instead
104-
// of leaving the handle unable to make progress.
105-
const retiredPath = getRetiredLockPath(lockPath);
106-
try {
107-
await retireCanonicalLockDirectory(lockPath, retiredPath);
108-
} catch (error) {
109-
const restored = await fsapi
110-
.rename(releaseMarkerPath, ownerMarker)
111-
.then(
112-
() => true,
113-
() => false,
114-
);
115-
if (restored) {
116-
state = 'held';
117-
throw createLockError(
118-
'Failed to retire the lock directory; ownership was restored',
119-
'ELOCKRELEASEFAILED',
120-
lockPath,
121-
error,
122-
);
123-
}
124-
throw createLockError(
125-
'Failed to retire the lock directory; release can be retried',
126-
'ELOCKRELEASEFAILED',
127-
lockPath,
128-
error,
129-
);
135+
release: () => {
136+
// Serialize concurrent release() calls on the same handle: without this,
137+
// two callers can both observe state === 'held' before either owner-marker
138+
// rename completes, and the loser sees ENOENT and reports ECOMPROMISED even
139+
// though the lock was validly released. Sharing one in-flight promise de-dupes
140+
// concurrent calls; clearing it on settle preserves retry-after-failure.
141+
if (!releaseInFlight) {
142+
releaseInFlight = performRelease().finally(() => {
143+
releaseInFlight = undefined;
144+
});
130145
}
131-
state = 'released';
132-
await cleanupRetiredLock(retiredPath, releaseMarkerName);
146+
return releaseInFlight;
133147
},
134148
};
135149
} catch (error) {

‎src/common/persistentState.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,18 @@ class PersistentStateImpl implements PersistentState {
2626
return this.momento.get<T>(key, defaultValue);
2727
}
2828
async set<T>(key: string, value: T): Promise<void> {
29-
await this.clearQueue;
30-
await this.momento.update(key, value);
29+
const operation = this.clearQueue.then(async () => {
30+
await this.momento.update(key, value);
3131

32-
const before = JSON.stringify(value);
33-
const after = JSON.stringify(await this.momento.get<T>(key));
34-
if (before !== after) {
35-
await this.momento.update(key, undefined);
36-
traceError('Error while updating state for key:', key);
37-
}
32+
const before = JSON.stringify(value);
33+
const after = JSON.stringify(await this.momento.get<T>(key));
34+
if (before !== after) {
35+
await this.momento.update(key, undefined);
36+
traceError('Error while updating state for key:', key);
37+
}
38+
});
39+
this.clearQueue = operation.catch(() => undefined);
40+
return operation;
3841
}
3942
async clear(keys?: string[], options?: { readonly preserveKeys?: readonly string[] }): Promise<void> {
4043
const requestedKeys = keys ? [...keys] : undefined;

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,37 @@ suite('lockfile APIs', () => {
273273
assert.strictEqual(await inspectFileLock(targetPath), 'missing');
274274
});
275275

276+
test('serializes concurrent release() calls through a shared in-flight promise', async () => {
277+
const lock = await acquireFileLock(targetPath, OPTIONS);
278+
const originalRename = fsExtra.rename;
279+
let ownerToReleaseRenames = 0;
280+
let signalStarted!: () => void;
281+
const started = new Promise<void>((resolve) => {
282+
signalStarted = resolve;
283+
});
284+
let openGate!: () => void;
285+
const gate = new Promise<void>((resolve) => {
286+
openGate = resolve;
287+
});
288+
sinon.stub(fsExtra, 'rename').callsFake(async (source, destination) => {
289+
if (path.basename(String(destination)).startsWith(FILE_LOCK_RELEASE_MARKER_PREFIX)) {
290+
ownerToReleaseRenames += 1;
291+
signalStarted();
292+
await gate;
293+
}
294+
await originalRename(source, destination);
295+
});
296+
297+
const first = lock.release();
298+
await started;
299+
const second = lock.release();
300+
openGate();
301+
await Promise.all([first, second]);
302+
303+
assert.strictEqual(ownerToReleaseRenames, 1);
304+
assert.strictEqual(await inspectFileLock(targetPath), 'missing');
305+
});
306+
276307
test('retained locks fail fast without waiting for the acquisition timeout', async () => {
277308
const lock = await acquireFileLock(targetPath, OPTIONS);
278309
await lock.retain();

‎src/test/common/persistentState.unit.test.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,37 @@ suite('persistent state clearing', () => {
176176
[undefined, 'written-later'],
177177
);
178178
});
179+
180+
test('serializes a gated write before a later clear so the clear is not resurrected', async () => {
181+
workspace.reset();
182+
const gate = createGate();
183+
workspace.beforeUpdate = async (key, value) => {
184+
if (key === 'race-key' && value === 'written') {
185+
gate.started.resolve();
186+
await gate.release.promise;
187+
}
188+
};
189+
190+
// A write is in flight (gated mid-update)...
191+
const gatedSet = workspaceState.set('race-key', 'written');
192+
await gate.started.promise;
193+
194+
// ...and a clear for the same key is requested after it.
195+
const laterClear = workspaceState.clear(['race-key']);
196+
197+
gate.release.resolve();
198+
await gatedSet;
199+
await laterClear;
200+
201+
// The clear was requested after the write, so it must win: the lagging write
202+
// cannot resurrect the key after the clear resolves.
203+
assert.strictEqual(workspace.values.has('race-key'), false);
204+
assert.strictEqual(await workspaceState.get('race-key'), undefined);
205+
assert.deepStrictEqual(
206+
workspace.updates.filter((update) => update.key === 'race-key').map((update) => update.value),
207+
['written', undefined],
208+
);
209+
});
179210
});
180211

181212
interface TestMemento {

0 commit comments

Comments
 (0)