Skip to content

Commit 9ccc2bc

Browse files
thymikeeApex by Callstack
andcommitted
fix(host-kit): a lock claim is identified by its token, not only by its process
Two records can name the same process and still be different acquisitions of the same path, which is the distinction a release and a reclaim both need. Each acquisition publishes a random claim token with its owner record: release matches the token rather than pid and start time alone, and a reclaim compares the token of the record inside the directory it moved with the one it judged before renaming. The publication-ownership rule now says what it means for the process lock: its owner record goes through the shared publication owner and it writes no file by hand, while its renames are addressed only between the lock path and the reclaimed name. Reclaiming a lock directory is a different claim of ownership from publishing a file into one. Co-authored-by: Apex by Callstack <noreply@callstack.com>
1 parent 7d4b8f3 commit 9ccc2bc

3 files changed

Lines changed: 109 additions & 18 deletions

File tree

packages/host-kit/src/internal/process-lock.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,44 @@ test('release leaves a lock whose record names a different process', async () =>
162162
assert.equal(fs.existsSync(ownerFilePath), true);
163163
});
164164

165+
test('release leaves a lock that a new acquisition of the same process republished', async () => {
166+
const lockDirPath = path.join(tmpDir, 'reacquired.lock');
167+
const ownerFilePath = path.join(lockDirPath, 'owner.json');
168+
const owner = currentProcessOwner();
169+
const release = await acquireProcessLock({ lockDirPath, owner });
170+
171+
// Same pid, same start time: the only thing that can tell this record from ours is the
172+
// claim written with it. Removing the directory would hand the new holder's lock away.
173+
fs.writeFileSync(
174+
ownerFilePath,
175+
JSON.stringify({ ...owner, acquiredAtMs: Date.now(), claimToken: 'a-different-claim' }),
176+
);
177+
await release();
178+
179+
assert.equal(
180+
(JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string }).claimToken,
181+
'a-different-claim',
182+
);
183+
});
184+
185+
test('a reacquired lock publishes a claim that its predecessor cannot reuse', async () => {
186+
const lockDirPath = path.join(tmpDir, 'claim-token.lock');
187+
const ownerFilePath = path.join(lockDirPath, 'owner.json');
188+
const first = await acquireProcessLock({ lockDirPath, owner: currentProcessOwner() });
189+
const firstToken = (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string })
190+
.claimToken;
191+
await first();
192+
193+
const second = await acquireProcessLock({ lockDirPath, owner: currentProcessOwner() });
194+
const secondToken = (JSON.parse(fs.readFileSync(ownerFilePath, 'utf8')) as { claimToken: string })
195+
.claimToken;
196+
await second();
197+
198+
assert.equal(typeof firstToken, 'string');
199+
assert.equal(typeof secondToken, 'string');
200+
assert.notEqual(firstToken, secondToken);
201+
});
202+
165203
test('acquireProcessLock does not evict a live owner whose owner.json is malformed', async () => {
166204
const lockDirPath = path.join(tmpDir, 'malformed.lock');
167205
fs.mkdirSync(lockDirPath);

packages/host-kit/src/internal/process-lock.ts

Lines changed: 52 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,17 @@ export type ProcessLockOwner = {
1818
acquiredAtMs: number;
1919
};
2020

21+
/**
22+
* One acquisition of a lock. The token says which: two records can name the same process
23+
* and still be different claims on the same path, which is what a release and a reclaim
24+
* have to tell apart.
25+
*/
26+
export type ProcessLockOwnerRecord = ProcessLockOwner & {
27+
claimToken: string | null;
28+
};
29+
2130
type ProcessLockOwnerReading =
22-
| { kind: 'owner'; owner: ProcessLockOwner }
31+
| { kind: 'owner'; owner: ProcessLockOwnerRecord }
2332
| { kind: 'unwritten' }
2433
| { kind: 'unreadable' };
2534

@@ -39,15 +48,16 @@ export async function acquireProcessLock(params: {
3948
const description = params.description ?? 'process lock';
4049

4150
fs.mkdirSync(path.dirname(lockDirPath), { recursive: true });
51+
const claim: ProcessLockOwnerRecord = { ...owner, claimToken: crypto.randomUUID() };
4252

4353
while (Date.now() < deadline) {
4454
try {
4555
fs.mkdirSync(lockDirPath);
46-
writeProcessLockOwner(ownerFilePath, owner);
56+
writeProcessLockOwner(ownerFilePath, claim);
4757
let released = false;
4858
return async () => {
4959
if (released) return;
50-
const outcome = releaseProcessLock(lockDirPath, ownerFilePath, owner);
60+
const outcome = releaseProcessLock(lockDirPath, ownerFilePath, claim);
5161
if (outcome !== 'unverified') {
5262
released = true;
5363
return;
@@ -84,7 +94,7 @@ function staleLockHint(lockDirPath: string): string {
8494
return `Remove ${lockDirPath} once you have confirmed no live process holds it, then retry.`;
8595
}
8696

87-
function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner): void {
97+
function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwnerRecord): void {
8898
publishFileSync({
8999
destination: ownerFilePath,
90100
contents: JSON.stringify(owner),
@@ -99,12 +109,15 @@ function writeProcessLockOwner(ownerFilePath: string, owner: ProcessLockOwner):
99109
function releaseProcessLock(
100110
lockDirPath: string,
101111
ownerFilePath: string,
102-
owner: ProcessLockOwner,
112+
claim: ProcessLockOwnerRecord,
103113
): 'removed' | 'not-owner' | 'unverified' {
104114
const reading = readProcessLockOwner(ownerFilePath);
105115
if (reading.kind === 'unreadable') return 'unverified';
106-
if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, owner))
116+
if (reading.kind === 'unwritten' || !ownerIdentityMatches(reading.owner, claim))
107117
return 'not-owner';
118+
// The same process can hold this path twice in sequence, and a reclaim that moved our
119+
// directory aside leaves a record behind that names us as though nothing had happened.
120+
if (reading.owner.claimToken !== claim.claimToken) return 'not-owner';
108121
fs.rmSync(lockDirPath, { recursive: true, force: true });
109122
return 'removed';
110123
}
@@ -125,7 +138,10 @@ function clearStaleProcessLock(
125138
// owner record, so its age is the only evidence available about it.
126139
if (!lockStats.isDirectory()) {
127140
return reclaimWhenAbandoned(lockStats, ownerGraceMs)
128-
? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats)
141+
? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, {
142+
stats: lockStats,
143+
claimToken: null,
144+
})
129145
: false;
130146
}
131147

@@ -134,7 +150,10 @@ function clearStaleProcessLock(
134150
if (isLiveProcessLockOwner(reading.owner)) {
135151
return false;
136152
}
137-
return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats);
153+
return reclaimProcessLockDirectory(lockDirPath, ownerFilePath, {
154+
stats: lockStats,
155+
claimToken: reading.owner.claimToken,
156+
});
138157
}
139158
// A record we cannot read leaves an owner whose identity is unknown, which is not
140159
// evidence of death. Only a record that is genuinely absent lets the directory's
@@ -143,7 +162,10 @@ function clearStaleProcessLock(
143162
return false;
144163
}
145164
return reclaimWhenAbandoned(lockStats, ownerGraceMs)
146-
? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, lockStats)
165+
? reclaimProcessLockDirectory(lockDirPath, ownerFilePath, {
166+
stats: lockStats,
167+
claimToken: null,
168+
})
147169
: false;
148170
}
149171

@@ -161,7 +183,7 @@ function reclaimWhenAbandoned(lockStats: fs.Stats, ownerGraceMs: number): boolea
161183
function reclaimProcessLockDirectory(
162184
lockDirPath: string,
163185
ownerFilePath: string,
164-
judged: fs.Stats,
186+
judged: JudgedLock,
165187
): boolean {
166188
const asidePath = reclaimedLockPath(lockDirPath);
167189
try {
@@ -199,22 +221,28 @@ function reclaimWithoutTheRename(
199221

200222
/**
201223
* A rename addresses whatever stands at the path now, not the directory whose record was
202-
* read. A contender that reclaimed first and published a live owner in the meantime has
203-
* put a different directory there, so what arrived is compared against what was judged:
204-
* the same inode, and no live owner inside.
224+
* read. A contender that reclaimed first and claimed the path again has put a different
225+
* directory there, so what arrived is compared against what was judged: the same inode,
226+
* no live owner inside, and the claim token that was read before the rename.
205227
*/
206-
function reclaimedLockIsTheOneJudged(asidePath: string, judged: fs.Stats): boolean {
228+
function reclaimedLockIsTheOneJudged(asidePath: string, judged: JudgedLock): boolean {
207229
let moved: fs.Stats;
208230
try {
209231
moved = fs.statSync(asidePath);
210232
} catch {
211233
return false;
212234
}
213-
if (moved.ino !== judged.ino || moved.dev !== judged.dev) return false;
235+
if (moved.ino !== judged.stats.ino || moved.dev !== judged.stats.dev) return false;
214236
const reading = readProcessLockOwner(path.join(asidePath, OWNER_FILE_NAME));
215-
return !(reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner));
237+
if (reading.kind === 'owner' && isLiveProcessLockOwner(reading.owner)) return false;
238+
return readClaimToken(reading) === judged.claimToken;
216239
}
217240

241+
type JudgedLock = {
242+
stats: fs.Stats;
243+
claimToken: string | null;
244+
};
245+
218246
/**
219247
* Returns a directory that turned out to belong to someone else. Failure is not a
220248
* licence to delete it: its holder's own release reports an unreadable owner rather than
@@ -259,7 +287,11 @@ function readProcessLockOwner(ownerFilePath: string): ProcessLockOwnerReading {
259287
return owner ? { kind: 'owner', owner } : { kind: 'unreadable' };
260288
}
261289

262-
function parseProcessLockOwner(contents: string): ProcessLockOwner | null {
290+
function readClaimToken(reading: ProcessLockOwnerReading): string | null {
291+
return reading.kind === 'owner' ? reading.owner.claimToken : null;
292+
}
293+
294+
function parseProcessLockOwner(contents: string): ProcessLockOwnerRecord | null {
263295
let parsed: unknown;
264296
try {
265297
parsed = JSON.parse(contents);
@@ -275,6 +307,9 @@ function parseProcessLockOwner(contents: string): ProcessLockOwner | null {
275307
pid: record.pid as number,
276308
startTime: typeof record.startTime === 'string' ? record.startTime : null,
277309
acquiredAtMs: record.acquiredAtMs as number,
310+
// A record written before claims were tokenized names a process without saying which
311+
// acquisition it was, which no release can match and no reclaim can be blamed for.
312+
claimToken: typeof record.claimToken === 'string' ? record.claimToken : null,
278313
};
279314
}
280315

src/daemon/__tests__/atomic-publish-ownership.test.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,13 @@ const SIMPLE_PUBLISHERS = [
1111
new URL('../session-script-writer.ts', import.meta.url),
1212
new URL('../../../packages/platform-apple/src/runner/runner-lease.ts', import.meta.url),
1313
new URL('../../remote/remote-connection-state.ts', import.meta.url),
14-
new URL('../../../packages/host-kit/src/internal/process-lock.ts', import.meta.url),
1514
] as const;
1615

16+
const PROCESS_LOCK_SOURCE = new URL(
17+
'../../../packages/host-kit/src/internal/process-lock.ts',
18+
import.meta.url,
19+
);
20+
1721
test('simple same-directory publishers use the shared atomic publish owner', () => {
1822
for (const sourcePath of SIMPLE_PUBLISHERS) {
1923
const source = fs.readFileSync(sourcePath, 'utf8');
@@ -22,6 +26,20 @@ test('simple same-directory publishers use the shared atomic publish owner', ()
2226
}
2327
});
2428

29+
// The process lock publishes a file and reclaims a directory, which are two different
30+
// claims of ownership: only the first belongs to the publication owners above.
31+
test('the process lock publishes its owner record without publishing files by hand', () => {
32+
const source = fs.readFileSync(PROCESS_LOCK_SOURCE, 'utf8');
33+
assert.match(source, /publishFileSync/);
34+
assert.doesNotMatch(source, /fs\.writeFileSync\s*\(/);
35+
});
36+
37+
test('the process lock renames only between the lock path and its reclaimed name', () => {
38+
const source = fs.readFileSync(PROCESS_LOCK_SOURCE, 'utf8');
39+
const renamed = [...source.matchAll(/fs\.renameSync\(([^)]*)\)/g)].map((match) => match[1]);
40+
assert.deepEqual(renamed.sort(), ['asidePath, lockDirPath', 'lockDirPath, asidePath']);
41+
});
42+
2543
test('durable publishers share the host-kit durable publication owner', () => {
2644
const sourcePaths = [
2745
new URL('../../../packages/capture-kit/src/durable-capture/store.ts', import.meta.url),

0 commit comments

Comments
 (0)