Skip to content

Commit 8457e46

Browse files
slaFFikslaFFik
authored
Use proper-lockfile for atomic instance lock, fix #11
Replace the manual PID/alive check with proper-lockfile.lock(), which acquires the lock atomically via mkdir. The old implementation had a TOCTOU window between isAlive() in preflight and the writeFile() in server.listen() — two concurrent starts could both pass the check and race on session.json. - Lock is now a directory at <storageRoot>/.lock; pid/port move to a sidecar `.lock.meta.json` (legacy `.lock` files are migrated away). - LOCK_STALE_MS is 5min so a debugger pause / brief sleep can't let a second invocation reclaim the lock; manual `rm -rf <dir> <meta>` is documented in the `instance-running` hint. - Release runs on early-startup failures, on listen() bind errors via `server.once('error', ...)`, and on writeLockMeta rejections; release errors are logged instead of swallowed. - Tests cover acquire/release, ELOCKED, legacy file migration, and sidecar cleanup.
1 parent fa92a23 commit 8457e46

3 files changed

Lines changed: 275 additions & 105 deletions

File tree

‎src/server/index.ts‎

Lines changed: 100 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { fileURLToPath } from 'node:url';
33
import { dirname, resolve } from 'node:path';
44
import getPort from 'get-port';
55
import open from 'open';
6-
import { runPreflight, writeLock, releaseLock } from './preflight.js';
6+
import { runPreflight, writeLockMeta } from './preflight.js';
77
import { SessionStore } from './session.js';
88
import { RunManager } from './runManager.js';
99
import { createRouter } from './routes.js';
@@ -28,85 +28,113 @@ async function main(): Promise<void> {
2828
process.exit(1);
2929
}
3030

31-
const sessionStore = await SessionStore.load(preflight.storageRoot, cwd);
32-
const port = await getPort({ port: [DEFAULT_PREF_PORT, 6801, 6802, 6803, 6804, 0] });
33-
const auth = makeAuthContext(port);
31+
// The lock is owned from this point forward; release it on any failure so
32+
// a botched startup doesn't leave a stale lock blocking subsequent runs
33+
// until the staleness window expires.
34+
try {
35+
const sessionStore = await SessionStore.load(preflight.storageRoot, cwd);
36+
const port = await getPort({ port: [DEFAULT_PREF_PORT, 6801, 6802, 6803, 6804, 0] });
37+
const auth = makeAuthContext(port);
3438

35-
const runManager = new RunManager({
36-
claudeBin,
37-
cwd,
38-
storageRoot: preflight.storageRoot,
39-
session: sessionStore,
40-
});
41-
await runManager.init();
39+
const runManager = new RunManager({
40+
claudeBin,
41+
cwd,
42+
storageRoot: preflight.storageRoot,
43+
session: sessionStore,
44+
});
45+
await runManager.init();
4246

43-
const webRoot = resolveWebRoot();
44-
const handler = createRouter({ auth, session: sessionStore, runManager, webRoot, cwd });
47+
const webRoot = resolveWebRoot();
48+
const handler = createRouter({ auth, session: sessionStore, runManager, webRoot, cwd });
4549

46-
const server = createServer((req, res) => {
47-
Promise.resolve(handler(req, res)).catch((err) => {
48-
log.error('http.handler-rejected', { error: (err as Error).message });
49-
try {
50-
res.statusCode = 500;
51-
res.end();
52-
} catch {
53-
/* */
54-
}
50+
const server = createServer((req, res) => {
51+
Promise.resolve(handler(req, res)).catch((err) => {
52+
log.error('http.handler-rejected', { error: (err as Error).message });
53+
try {
54+
res.statusCode = 500;
55+
res.end();
56+
} catch {
57+
/* */
58+
}
59+
});
5560
});
56-
});
5761

58-
server.listen(port, '127.0.0.1', async () => {
59-
await writeLock(preflight.lockFilePath, process.pid, port);
60-
const url = `${auth.origin}/?t=${auth.token}`;
61-
log.info('server.listening', { url, pid: process.pid });
62-
console.log(`mdredd listening at ${url}`);
63-
if (shouldOpen) {
64-
open(url).catch((err) => {
65-
console.log(`(could not open browser automatically: ${err.message})`);
66-
});
67-
}
68-
});
62+
// Bind failures (EADDRINUSE/EACCES) surface via the 'error' event, not
63+
// the listen callback. Without this handler Node would emit an unhandled
64+
// 'error' and exit while still holding the proper-lockfile lock,
65+
// blocking restarts until the stale window expires.
66+
server.once('error', (err) => {
67+
console.error(`mdredd: failed to bind ${port}: ${err.message}`);
68+
log.error('server.listen-error', { port, error: err.message });
69+
void preflight.releaseLock().finally(() => process.exit(1));
70+
});
6971

70-
// 5s for runners to drain (each runner self-bounds at SIGTERM+2s SIGKILL),
71-
// plus 3s slack for lockfile / FS work. Anything still hanging past the hard
72-
// timer is force-exited so a stuck child can never wedge the server.
73-
const STOP_RUNNERS_TIMEOUT_MS = 5_000;
74-
const HARD_SHUTDOWN_TIMEOUT_MS = 8_000;
72+
server.listen(port, '127.0.0.1', () => {
73+
// Keep this callback synchronous so a writeLockMeta rejection cannot
74+
// escape as an unhandled promise rejection. On failure release the
75+
// lock (we just acquired it but never wrote the sidecar) and exit.
76+
writeLockMeta(preflight.lockFilePath, process.pid, port)
77+
.then(() => {
78+
const url = `${auth.origin}/?t=${auth.token}`;
79+
log.info('server.listening', { url, pid: process.pid });
80+
console.log(`mdredd listening at ${url}`);
81+
if (shouldOpen) {
82+
open(url).catch((err) => {
83+
console.log(`(could not open browser automatically: ${err.message})`);
84+
});
85+
}
86+
})
87+
.catch((err) => {
88+
console.error(`mdredd: could not write lock metadata: ${(err as Error).message}`);
89+
log.error('server.lock-meta-failed', { error: (err as Error).message });
90+
void preflight.releaseLock().finally(() => process.exit(1));
91+
});
92+
});
93+
94+
// 5s for runners to drain (each runner self-bounds at SIGTERM+2s SIGKILL),
95+
// plus 3s slack for lockfile / FS work. Anything still hanging past the hard
96+
// timer is force-exited so a stuck child can never wedge the server.
97+
const STOP_RUNNERS_TIMEOUT_MS = 5_000;
98+
const HARD_SHUTDOWN_TIMEOUT_MS = 8_000;
7599

76-
let shuttingDown = false;
77-
const shutdown = async (sig: string): Promise<void> => {
78-
if (shuttingDown) return;
79-
shuttingDown = true;
80-
log.info('server.shutdown', { signal: sig, activeRuns: runManager.activeCount() });
81-
const hardTimer = setTimeout(() => {
82-
log.error('server.shutdown-forced-exit', { reason: 'hard timeout exceeded' });
83-
process.exit(1);
84-
}, HARD_SHUTDOWN_TIMEOUT_MS);
85-
hardTimer.unref();
86-
try {
87-
// Stop accepting new HTTP traffic and drop existing keep-alive/SSE
88-
// connections so the server's `listening` socket and the SSE keepers
89-
// don't hold the event loop open.
90-
server.close();
91-
server.closeAllConnections?.();
92-
const result = await runManager.stopAll(STOP_RUNNERS_TIMEOUT_MS);
93-
if (result.timedOut) {
94-
log.warn('server.shutdown.stopAll-timeout', { stopped: result.stopped });
100+
let shuttingDown = false;
101+
const shutdown = async (sig: string): Promise<void> => {
102+
if (shuttingDown) return;
103+
shuttingDown = true;
104+
log.info('server.shutdown', { signal: sig, activeRuns: runManager.activeCount() });
105+
const hardTimer = setTimeout(() => {
106+
log.error('server.shutdown-forced-exit', { reason: 'hard timeout exceeded' });
107+
process.exit(1);
108+
}, HARD_SHUTDOWN_TIMEOUT_MS);
109+
hardTimer.unref();
110+
try {
111+
// Stop accepting new HTTP traffic and drop existing keep-alive/SSE
112+
// connections so the server's `listening` socket and the SSE keepers
113+
// don't hold the event loop open.
114+
server.close();
115+
server.closeAllConnections?.();
116+
const result = await runManager.stopAll(STOP_RUNNERS_TIMEOUT_MS);
117+
if (result.timedOut) {
118+
log.warn('server.shutdown.stopAll-timeout', { stopped: result.stopped });
119+
}
120+
await preflight.releaseLock();
121+
} catch (err) {
122+
log.error('server.shutdown-error', { error: (err as Error).message });
123+
} finally {
124+
clearTimeout(hardTimer);
95125
}
96-
await releaseLock(preflight.lockFilePath);
97-
} catch (err) {
98-
log.error('server.shutdown-error', { error: (err as Error).message });
99-
} finally {
100-
clearTimeout(hardTimer);
101-
}
102-
process.exit(0);
103-
};
104-
process.on('SIGINT', () => {
105-
void shutdown('SIGINT');
106-
});
107-
process.on('SIGTERM', () => {
108-
void shutdown('SIGTERM');
109-
});
126+
process.exit(0);
127+
};
128+
process.on('SIGINT', () => {
129+
void shutdown('SIGINT');
130+
});
131+
process.on('SIGTERM', () => {
132+
void shutdown('SIGTERM');
133+
});
134+
} catch (err) {
135+
await preflight.releaseLock().catch(() => undefined);
136+
throw err;
137+
}
110138
}
111139

112140
function resolveWebRoot(): string {

‎src/server/preflight.ts‎

Lines changed: 76 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import { homedir, tmpdir } from 'node:os';
44
import { join } from 'node:path';
55
import { pathExists, atomicWriteFile, ensureDir, readJsonIfExists } from './fsUtil.js';
66
import { JUDGE_MODEL, PROJECT_MARKERS, STORAGE_DIR_NAME } from '@shared/constants.js';
7-
import { readdir, readFile, writeFile, unlink } from 'node:fs/promises';
7+
import { readdir, readFile, stat, unlink, writeFile } from 'node:fs/promises';
8+
import lockfile from 'proper-lockfile';
89
import { log } from './log.js';
910

1011
const execFileAsync = promisify(execFile);
@@ -18,9 +19,18 @@ export interface PreflightInput {
1819
export interface PreflightResult {
1920
storageRoot: string;
2021
lockFilePath: string;
21-
ownedLock: boolean;
22+
releaseLock: () => Promise<void>;
2223
}
2324

25+
// Threshold (ms) past which a lockfile is considered stale and may be
26+
// reclaimed. proper-lockfile refreshes the lockfile's mtime every stale/2
27+
// while the holder is alive; after a crash it is reclaimable after `stale` ms.
28+
// Set generously: a long event-loop stall, debugger pause, or brief system
29+
// sleep should not cause a second invocation to consider the lock stale and
30+
// race with the original — that would defeat the single-instance guarantee.
31+
// Users can always reclaim manually via the hint in `instance-running`.
32+
const LOCK_STALE_MS = 5 * 60_000;
33+
2434
export class PreflightError extends Error {
2535
code: string;
2636
hint: string | undefined;
@@ -39,9 +49,9 @@ export async function runPreflight(input: PreflightInput): Promise<PreflightResu
3949
const lockFilePath = join(storageRoot, '.lock');
4050
await ensureDir(storageRoot);
4151
await ensureAutoGitignore(storageRoot);
42-
await acquireLock(lockFilePath);
52+
const releaseLock = await acquireLock(storageRoot, lockFilePath);
4353
await recoverAbandonedRuns(storageRoot);
44-
return { storageRoot, lockFilePath, ownedLock: true };
54+
return { storageRoot, lockFilePath, releaseLock };
4555
}
4656

4757
async function checkClaudeCli(bin: string): Promise<void> {
@@ -242,41 +252,75 @@ async function ensureAutoGitignore(storageRoot: string): Promise<void> {
242252
await atomicWriteFile(gitignorePath, '*\n!.gitignore\n');
243253
}
244254

245-
async function acquireLock(lockFilePath: string): Promise<void> {
246-
const existing = await readJsonIfExists<{ pid: number; port: number; startedAt: string }>(
247-
lockFilePath,
248-
);
249-
if (existing && isAlive(existing.pid)) {
250-
throw new PreflightError(
251-
'instance-running',
252-
`Another mdredd instance appears to be running (pid ${existing.pid}, port ${existing.port}).`,
253-
`Close it first, or remove ${lockFilePath} if you are sure it is stale.`,
254-
);
255-
}
256-
if (existing) {
257-
log.info('preflight.stale-lock-recovered', { pid: existing.pid });
258-
await unlink(lockFilePath).catch(() => undefined);
259-
}
260-
}
261-
262-
export async function writeLock(lockFilePath: string, pid: number, port: number): Promise<void> {
263-
await writeFile(
264-
lockFilePath,
265-
JSON.stringify({ pid, port, startedAt: new Date().toISOString() }, null, 2),
266-
);
255+
function lockMetaPath(lockFilePath: string): string {
256+
return `${lockFilePath}.meta.json`;
267257
}
268258

269-
export async function releaseLock(lockFilePath: string): Promise<void> {
270-
await unlink(lockFilePath).catch(() => undefined);
259+
// Older versions wrote a JSON file at `.lock`. proper-lockfile uses that path
260+
// as a directory (mkdir-based), so a stale legacy file would block startup
261+
// forever. Remove it before attempting to acquire.
262+
async function migrateLegacyLock(lockFilePath: string): Promise<void> {
263+
try {
264+
const st = await stat(lockFilePath);
265+
if (st.isFile()) {
266+
await unlink(lockFilePath).catch(() => undefined);
267+
log.info('preflight.legacy-lock-removed', { path: lockFilePath });
268+
}
269+
} catch {
270+
// not present — nothing to migrate
271+
}
271272
}
272273

273-
function isAlive(pid: number): boolean {
274+
export async function acquireLock(
275+
storageRoot: string,
276+
lockFilePath: string,
277+
): Promise<() => Promise<void>> {
278+
await migrateLegacyLock(lockFilePath);
279+
const metaPath = lockMetaPath(lockFilePath);
280+
let release: () => Promise<void>;
274281
try {
275-
process.kill(pid, 0);
276-
return true;
282+
release = await lockfile.lock(storageRoot, {
283+
lockfilePath: lockFilePath,
284+
stale: LOCK_STALE_MS,
285+
realpath: false,
286+
});
277287
} catch (err) {
278-
return (err as NodeJS.ErrnoException).code === 'EPERM';
288+
if ((err as { code?: string }).code === 'ELOCKED') {
289+
const meta = await readJsonIfExists<{ pid: number; port: number }>(metaPath);
290+
const info = meta ? ` (pid ${meta.pid}, port ${meta.port})` : '';
291+
throw new PreflightError(
292+
'instance-running',
293+
`Another mdredd instance appears to be running${info}.`,
294+
`Close it first, or, if you are sure it is stale, remove the lock directory and its sidecar: rm -rf ${lockFilePath} ${metaPath}`,
295+
);
296+
}
297+
throw err;
279298
}
299+
return async () => {
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+
}
311+
await unlink(metaPath).catch(() => undefined);
312+
};
313+
}
314+
315+
export async function writeLockMeta(
316+
lockFilePath: string,
317+
pid: number,
318+
port: number,
319+
): Promise<void> {
320+
await atomicWriteFile(
321+
lockMetaPath(lockFilePath),
322+
JSON.stringify({ pid, port, startedAt: new Date().toISOString() }, null, 2),
323+
);
280324
}
281325

282326
async function recoverAbandonedRuns(storageRoot: string): Promise<void> {

0 commit comments

Comments
 (0)