Skip to content

Commit 23fbe61

Browse files
mchwangclaude
andcommitted
Keep held runner locks referenced so GC cannot release them
A caller that dropped the returned RunnerLock let its SQLite connection be garbage-collected, which released the OS lock while the runner was still alive. Found by a CI failure of the cross-process lock test on #60. The new test drops the lock and forces GC in a child, and fails without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent d8f2d99 commit 23fbe61

2 files changed

Lines changed: 15 additions & 1 deletion

File tree

‎runner/recovery.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ function assertLocal(path: string): void {
2929
if (NETWORK_FILESYSTEMS.has(Number(statfsSync(path).type))) throw new Error(`${path} is on a network filesystem; the runner lock needs a local filesystem.`);
3030
}
3131

32+
/**
33+
* Strong references to every held lock connection. Without this, a caller that drops the returned RunnerLock lets the
34+
* connection be garbage-collected, and closing it silently releases the OS lock while the runner is still alive.
35+
*/
36+
const heldLocks = new Set<DatabaseSync>();
3237
export interface RunnerLock {
3338
readonly file: { dev: bigint; ino: bigint };
3439
/** Step 4: the database path still names the locked file. Call after the Store opens. */
@@ -67,6 +72,7 @@ export function acquireRunnerLock(databasePath: string, options: { lockRoot?: st
6772
throw error;
6873
}
6974
const file = { dev: st.dev, ino: st.ino }, heldDb = db;
75+
heldLocks.add(heldDb);
7076
let released = false;
7177
return {
7278
file,
@@ -75,7 +81,7 @@ export function acquireRunnerLock(databasePath: string, options: { lockRoot?: st
7581
if (now.isSymbolicLink() || !now.isFile() || now.dev !== file.dev || now.ino !== file.ino || now.nlink !== 1n)
7682
throw new Error('The database path changed while opening. Refusing to start.');
7783
},
78-
release() { if (released) return; released = true; heldDb.close(); closeSync(fd); },
84+
release() { if (released) return; released = true; heldLocks.delete(heldDb); heldDb.close(); closeSync(fd); },
7985
};
8086
} catch (error) { db?.close(); closeSync(fd); throw error; }
8187
}

‎test/runner-recovery.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,14 @@ describe('runner lock', () => {
3939
expect(again.file).toEqual(first.file);
4040
expect(existsSync(join(locksDir, `${first.file.dev}-${first.file.ino}.runner-lock`))).toBe(true);
4141
});
42+
it('keeps holding the lock when the caller drops the returned object and garbage collection runs', async () => {
43+
const d = dir(), locksDir = join(d, 'locks'), path = join(d, 'db.sqlite');
44+
const recovery = fileURLToPath(new URL('../runner/recovery.ts', import.meta.url));
45+
const child = spawn(process.execPath, ['--expose-gc', '-e', `import(${JSON.stringify(recovery)}).then(m => { m.acquireRunnerLock(${JSON.stringify(path)}, { lockRoot: ${JSON.stringify(locksDir)} }); for (let i = 0; i < 5; i++) globalThis.gc(); setTimeout(() => { globalThis.gc(); console.log('held'); }, 50); setInterval(() => {}, 1000); })`], { stdio: ['ignore', 'pipe', 'inherit'] });
46+
children.push(child);
47+
await new Promise<void>(resolve => child.stdout!.on('data', chunk => { if (String(chunk).includes('held')) resolve(); }));
48+
expect(() => acquireRunnerLock(path, { lockRoot: locksDir })).toThrow(LockHeld);
49+
});
4250
it('is released by the OS when the holding process is killed', async () => {
4351
const d = dir(), locksDir = join(d, 'locks'), path = join(d, 'db.sqlite');
4452
const recovery = fileURLToPath(new URL('../runner/recovery.ts', import.meta.url));

0 commit comments

Comments
 (0)