Skip to content

Commit 256d4f4

Browse files
committed
Log lock release errors and cover acquireLock with tests
Address Copilot review on PR #24: - Surface release() failures via log.warn so operators see why a lock wasn't cleared instead of the wrapper swallowing the error. - Export acquireLock and add coverage for acquire/release, ELOCKED → instance-running (with rm -rf and meta-sidecar hint), legacy `.lock` file migration, and meta sidecar cleanup on release.
1 parent 6bb3885 commit 256d4f4

2 files changed

Lines changed: 114 additions & 3 deletions

File tree

src/server/preflight.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ async function migrateLegacyLock(lockFilePath: string): Promise<void> {
271271
}
272272
}
273273

274-
async function acquireLock(
274+
export async function acquireLock(
275275
storageRoot: string,
276276
lockFilePath: string,
277277
): Promise<() => Promise<void>> {
@@ -297,7 +297,17 @@ async function acquireLock(
297297
throw err;
298298
}
299299
return async () => {
300-
await release().catch(() => undefined);
300+
try {
301+
await release();
302+
} catch (err) {
303+
// Surface release failures so the operator knows why the lock wasn't
304+
// cleared — they will have to wait out the stale window or remove the
305+
// lock directory manually (see the `instance-running` hint).
306+
log.warn('preflight.lock-release-failed', {
307+
path: lockFilePath,
308+
error: (err as Error).message,
309+
});
310+
}
301311
await unlink(metaPath).catch(() => undefined);
302312
};
303313
}

test/preflight.spec.ts

Lines changed: 102 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
import { authSmokeTest, PreflightError } from '../src/server/preflight.js';
1+
import { mkdtemp, mkdir, readdir, rm, stat, writeFile } from 'node:fs/promises';
2+
import { tmpdir } from 'node:os';
3+
import { join } from 'node:path';
4+
import { acquireLock, authSmokeTest, PreflightError } from '../src/server/preflight.js';
25

36
const fakeBin = new URL('./fake-claude.mjs', import.meta.url).pathname;
47

@@ -83,4 +86,102 @@ await scenario('authSmokeTest: spawn-error path when binary is missing', async (
8386
);
8487
});
8588

89+
async function withTempStorage<T>(run: (storageRoot: string) => Promise<T>): Promise<T> {
90+
const dir = await mkdtemp(join(tmpdir(), 'mdredd-lock-'));
91+
try {
92+
return await run(dir);
93+
} finally {
94+
await rm(dir, { recursive: true, force: true });
95+
}
96+
}
97+
98+
await scenario('acquireLock: acquires, then re-acquires after release', async () => {
99+
await withTempStorage(async (storageRoot) => {
100+
const lockPath = join(storageRoot, '.lock');
101+
const release1 = await acquireLock(storageRoot, lockPath);
102+
const stat1 = await stat(lockPath);
103+
if (!stat1.isDirectory()) {
104+
throw new Error('expected lockfile path to be a directory after acquire');
105+
}
106+
await release1();
107+
// Second acquire on the same storage root should succeed once the first
108+
// has been released — proves the wrapper actually clears the lock.
109+
const release2 = await acquireLock(storageRoot, lockPath);
110+
await release2();
111+
});
112+
});
113+
114+
await scenario(
115+
'acquireLock: surfaces ELOCKED as instance-running with rm -rf hint',
116+
async () => {
117+
await withTempStorage(async (storageRoot) => {
118+
const lockPath = join(storageRoot, '.lock');
119+
const release = await acquireLock(storageRoot, lockPath);
120+
try {
121+
const err = await expectPreflightError(
122+
async () => {
123+
await acquireLock(storageRoot, lockPath);
124+
},
125+
'instance-running',
126+
'rm -rf',
127+
);
128+
if (!err.hint || !err.hint.includes(`${lockPath}.meta.json`)) {
129+
throw new Error(`expected hint to mention meta sidecar, got: ${err.hint}`);
130+
}
131+
} finally {
132+
await release();
133+
}
134+
});
135+
},
136+
);
137+
138+
await scenario('acquireLock: legacy `.lock` file is migrated away', async () => {
139+
await withTempStorage(async (storageRoot) => {
140+
const lockPath = join(storageRoot, '.lock');
141+
// Older versions wrote `.lock` as a regular JSON file. proper-lockfile
142+
// uses the same path as a mkdir-based directory, so a stale legacy file
143+
// would block startup forever — acquire must remove it first.
144+
await writeFile(lockPath, '{"pid":1,"port":6800}');
145+
const release = await acquireLock(storageRoot, lockPath);
146+
try {
147+
const st = await stat(lockPath);
148+
if (!st.isDirectory()) {
149+
throw new Error('expected `.lock` to be a directory after migration');
150+
}
151+
} finally {
152+
await release();
153+
}
154+
});
155+
});
156+
157+
await scenario('acquireLock: release removes the lock directory and meta sidecar', async () => {
158+
await withTempStorage(async (storageRoot) => {
159+
const lockPath = join(storageRoot, '.lock');
160+
const metaPath = `${lockPath}.meta.json`;
161+
const release = await acquireLock(storageRoot, lockPath);
162+
// Simulate writeLockMeta having run during normal startup so we can
163+
// verify release() removes the sidecar too.
164+
await writeFile(metaPath, '{"pid":1,"port":6800,"startedAt":""}');
165+
await release();
166+
const entries = await readdir(storageRoot);
167+
if (entries.includes('.lock')) {
168+
throw new Error(`expected .lock to be gone after release, got: ${entries.join(', ')}`);
169+
}
170+
if (entries.includes('.lock.meta.json')) {
171+
throw new Error(`expected meta sidecar to be removed, got: ${entries.join(', ')}`);
172+
}
173+
});
174+
});
175+
176+
await scenario('acquireLock: storageRoot must exist before acquire', async () => {
177+
// Sanity check that the helper relies on the caller having ensured the
178+
// directory — this matches the runPreflight() ordering (ensureDir first).
179+
await withTempStorage(async (parent) => {
180+
const storageRoot = join(parent, 'nested');
181+
await mkdir(storageRoot, { recursive: true });
182+
const release = await acquireLock(storageRoot, join(storageRoot, '.lock'));
183+
await release();
184+
});
185+
});
186+
86187
console.log('\nAll preflight smoke scenarios passed.');

0 commit comments

Comments
 (0)