Skip to content

Commit d947f8c

Browse files
fix(runs): close start/resume TOCTOU on the continuous-takeover await
/api/start and /api/resume checked isPipelineBusy() synchronously, then awaited the continuous-takeover pause-poll (up to 10s) BEFORE runAudit sets status= 'running'. Two near-simultaneous starts both passed the guard and minted duplicate run numbers / overlapped the shared pipeline run dir. Fix: a synchronous `_auditLaunching` flag claimed BEFORE the takeover await (inside try/finally so it always clears — return, throw, or completion — and can never wedge future starts), with the "Already running" guard now checking isPipelineBusy() || _auditLaunching. The flag is cleared at the end of the takeover block; that's safe because the remaining launch path is synchronous up to status='running' (verified in both handlers, incl. resume's subscription path via runSubPlanTest). Adversarially reviewed: stuck-true impossible; window fully closed in both handlers; the added guard read on the retest endpoints is harmless/beneficial. Test suite green (188/31/45/35).
1 parent 35c9ddb commit d947f8c

1 file changed

Lines changed: 50 additions & 17 deletions

File tree

server.js

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,15 @@ function isPipelineBusy() { return PIPELINE_BUSY_STATUSES.includes(state.status)
533533
// separate continuous-takeover handling (delete, SDK swap).
534534
function isAuditBusy() { return isPipelineBusy() || continuous.status().running; }
535535

536+
// Synchronous "an audit is being launched" guard. /api/start and /api/resume have
537+
// an AWAIT (the continuous-takeover pause-poll) between the isPipelineBusy() check
538+
// and the moment runAudit sets status='running'. Without a flag set before that
539+
// await, two near-simultaneous starts both pass the check, mint duplicate run
540+
// numbers, and overlap on the shared pipeline run dir. Set true before the
541+
// takeover await, cleared once it completes — the rest of the launch is
542+
// synchronous up to status='running', so no concurrent request can interleave.
543+
let _auditLaunching = false;
544+
536545
// Persist Broadcast Live across restarts so the operator's choice survives
537546
// process bounces — without this, every restart silently flips public /live
538547
// back to "paused" even though the admin UI still shows BROADCAST ON.
@@ -2273,16 +2282,28 @@ app.post('/api/start', adminOnly, async (req, res) => {
22732282
const infiniteLoop = !!(req.body?.infiniteLoop || req.query.infiniteLoop);
22742283
const pricingMode = (req.body?.pricingMode === 'hours' || req.query.pricingMode === 'hours') ? 'hours' : 'gigabytes';
22752284

2276-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2285+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
22772286
if (continuous.status().running) {
22782287
if (!req.body?.takeover) {
22792288
return res.status(409).json({ error: 'PUBLIC_RUN_ACTIVE', message: 'A public run is active. Pause it and start an audit?' });
22802289
}
2281-
const pr = continuous.pause();
2282-
if (!pr.ok) return res.status(500).json({ error: 'pause failed: ' + pr.error });
2283-
for (let i = 0; i < 100; i++) {
2284-
if (!continuous.status().running) break;
2285-
await new Promise(r => setTimeout(r, 100));
2290+
// Synchronously claim the launch BEFORE the pause-poll await so a second
2291+
// concurrent start/resume can't slip through the isPipelineBusy() check.
2292+
// try/finally guarantees the flag clears on EVERY path (return or throw) so
2293+
// it can never get stuck true and wedge all future starts. The flag clears
2294+
// at the END of this block — safe ONLY because the remaining launch path is
2295+
// synchronous up to status='running'. DO NOT add an await between here and
2296+
// status='running', or the TOCTOU window silently reopens.
2297+
_auditLaunching = true;
2298+
try {
2299+
const pr = continuous.pause();
2300+
if (!pr.ok) return res.status(500).json({ error: 'pause failed: ' + pr.error });
2301+
for (let i = 0; i < 100; i++) {
2302+
if (!continuous.status().running) break;
2303+
await new Promise(r => setTimeout(r, 100));
2304+
}
2305+
} finally {
2306+
_auditLaunching = false;
22862307
}
22872308
}
22882309
if (!testRun && !MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
@@ -2338,16 +2359,28 @@ app.post('/api/start', adminOnly, async (req, res) => {
23382359

23392360
// Resume CURRENT test from where it left off (skips already-tested nodes).
23402361
app.post('/api/resume', adminOnly, async (req, res) => {
2341-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2362+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
23422363
if (continuous.status().running) {
23432364
if (!req.body?.takeover) {
23442365
return res.status(409).json({ error: 'PUBLIC_RUN_ACTIVE', message: 'A public run is active. Pause it and start an audit?' });
23452366
}
2346-
const pr = continuous.pause();
2347-
if (!pr.ok) return res.status(500).json({ error: 'pause failed: ' + pr.error });
2348-
for (let i = 0; i < 100; i++) {
2349-
if (!continuous.status().running) break;
2350-
await new Promise(r => setTimeout(r, 100));
2367+
// Synchronously claim the launch BEFORE the pause-poll await so a second
2368+
// concurrent start/resume can't slip through the isPipelineBusy() check.
2369+
// try/finally guarantees the flag clears on EVERY path (return or throw) so
2370+
// it can never get stuck true and wedge all future starts. The flag clears
2371+
// at the END of this block — safe ONLY because the remaining launch path is
2372+
// synchronous up to status='running'. DO NOT add an await between here and
2373+
// status='running', or the TOCTOU window silently reopens.
2374+
_auditLaunching = true;
2375+
try {
2376+
const pr = continuous.pause();
2377+
if (!pr.ok) return res.status(500).json({ error: 'pause failed: ' + pr.error });
2378+
for (let i = 0; i < 100; i++) {
2379+
if (!continuous.status().running) break;
2380+
await new Promise(r => setTimeout(r, 100));
2381+
}
2382+
} finally {
2383+
_auditLaunching = false;
23512384
}
23522385
}
23532386
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
@@ -2496,7 +2529,7 @@ app.post('/api/stop', adminOnly, (req, res) => {
24962529
});
24972530

24982531
app.post('/api/retest-skips', adminOnly, async (req, res) => {
2499-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2532+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
25002533
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
25012534
const results = getResults();
25022535
const skipAddrs = results.filter(r => r.actualMbps == null && /unreachable/i.test(r.error || '')).map(r => r.address);
@@ -2530,7 +2563,7 @@ app.post('/api/retest-skips', adminOnly, async (req, res) => {
25302563
});
25312564

25322565
app.post('/api/retest-fails', adminOnly, async (req, res) => {
2533-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2566+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
25342567
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
25352568
const results = getResults();
25362569
const specific = req.body?.addresses;
@@ -2567,7 +2600,7 @@ app.post('/api/retest-fails', adminOnly, async (req, res) => {
25672600

25682601
// DEPRECATED: Plan testing is WIP — hidden from dashboard, endpoint still functional for API callers
25692602
app.post('/api/test-plan', adminOnly, async (req, res) => {
2570-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2603+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
25712604
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
25722605
const { planId } = req.body;
25732606
if (!planId) return res.status(400).json({ error: 'planId required' });
@@ -2620,7 +2653,7 @@ app.get('/api/sub-plans', adminOnly, async (req, res) => {
26202653

26212654
// Sub. Plan mode: run fee-granted plan test — starts as a fresh run with clean counters.
26222655
app.post('/api/test-sub-plan', adminOnly, async (req, res) => {
2623-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2656+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
26242657
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
26252658
const { planId, subscriptionId, granter } = req.body || {};
26262659
if (!planId) return res.status(400).json({ error: 'planId required' });
@@ -2847,7 +2880,7 @@ app.get('/api/transport-cache', adminOnly, (req, res) => {
28472880

28482881
// Auto-retest: analyze failures, retest all retestable nodes in one shot
28492882
app.post('/api/auto-retest', adminOnly, async (req, res) => {
2850-
if (isPipelineBusy()) return res.json({ error: 'Already running' });
2883+
if (isPipelineBusy() || _auditLaunching) return res.json({ error: 'Already running' });
28512884
if (!MNEMONIC) return res.json({ error: 'MNEMONIC not set in .env' });
28522885

28532886
const force = req.body?.force === true;

0 commit comments

Comments
 (0)