diff --git a/server/install_server.sh b/server/install_server.sh index d908e248..002079cd 100755 --- a/server/install_server.sh +++ b/server/install_server.sh @@ -158,6 +158,7 @@ Environment=REPLAY_SEGMENT_DIR=$REPLAY_SEGMENT_DIR ExecStart=$NODE_BIN $SERVER_DIR/index.js Restart=on-failure RestartSec=2 +SuccessExitStatus=130 143 [Install] WantedBy=multi-user.target diff --git a/server/src/services/audioForwardService/hooks.js b/server/src/services/audioForwardService/hooks.js index 0f6dc97b..25c4854a 100644 --- a/server/src/services/audioForwardService/hooks.js +++ b/server/src/services/audioForwardService/hooks.js @@ -31,15 +31,13 @@ function registerAudioForwardHooks(deps) { stopWorker(roverId); return; } - if (action === 'upsert' && serviceEnabled) { - if (whipOwners.has(roverId)) { - return; - } - try { - ensureWorker(roverId); - } catch (err) { - setState(roverId, { state: 'error', source: 'init', error: err.message, startedAt: null }); - } + if (action === 'upsert' && serviceEnabled && !workers.has(roverId)) { + // A rover coming online should not create ffmpeg publishers by itself. + // The audio worker is intentionally lazy because uploads, mic forwarding, + // and automatic sounds are the moments that actually need a media pipe; + // keeping idle ffmpeg children around made restarts depend on processes + // that may never have been used by an operator. + setState(roverId, { state: 'offline', source: 'none', error: null, startedAt: null }); } }); diff --git a/server/src/services/audioForwardService/index.js b/server/src/services/audioForwardService/index.js index 686d6ce7..da37f84e 100644 --- a/server/src/services/audioForwardService/index.js +++ b/server/src/services/audioForwardService/index.js @@ -95,6 +95,7 @@ const workerEngine = createAudioForwardWorkerEngine({ const { ensureWorker, stopWorker, + stopAllWorkers, playUploadedAudio, playServerAudioFile, stopPlayback, @@ -104,6 +105,21 @@ const { startSilenceWriter, } = workerEngine; +function installShutdownHooks() { + const shutdown = (signal) => { + // Audio forwarding owns long-lived ffmpeg publisher/writer pairs. Stop them + // synchronously on process signals so a systemd restart does not have to + // wait for orphaned media workers to notice that their parent is gone. + stopAllWorkers(signal || 'process-exit'); + }; + + process.once('exit', () => shutdown('exit')); + process.once('SIGINT', () => shutdown('SIGINT')); + process.once('SIGTERM', () => shutdown('SIGTERM')); +} + +installShutdownHooks(); + registerAudioForwardHooks({ io, roverManager, diff --git a/server/src/services/audioForwardService/workerEngine.js b/server/src/services/audioForwardService/workerEngine.js index e96cf386..f72aa8af 100644 --- a/server/src/services/audioForwardService/workerEngine.js +++ b/server/src/services/audioForwardService/workerEngine.js @@ -80,14 +80,26 @@ function createAudioForwardWorkerEngine(deps) { } function stopProc(proc, graceMs = 1200) { - if (!proc || proc.killed) return; + if (!proc || proc.exitCode != null || proc.signalCode != null) return; + let exited = false; + const markExited = () => { + exited = true; + }; + // ChildProcess.killed only means Node successfully sent a signal, not that + // ffmpeg actually exited. Track the real exit event so FIFO/SRT hangs still + // get escalated to SIGKILL instead of making systemd wait for its timeout. + proc.once('exit', markExited); try { proc.kill('SIGTERM'); } catch { + proc.off('exit', markExited); return; } setTimeout(() => { - if (!proc.killed) { + // The timer intentionally checks our exit flag rather than proc.killed. + // proc.killed flips to true immediately after SIGTERM, which was the bug + // that prevented stubborn ffmpeg processes from being force-killed. + if (!exited) { try { proc.kill('SIGKILL'); } catch { @@ -394,6 +406,19 @@ function createAudioForwardWorkerEngine(deps) { setState(roverId, { state: 'offline', source: 'none', error: null, startedAt: null }); } + function stopAllWorkers(reason = 'shutdown') { + // Copy the keys before stopping because stopWorker mutates the workers map. + // Shutdown is a process-wide lifecycle event, so every rover-owned ffmpeg + // process must be asked to exit before the parent Node process disappears. + const roverIds = Array.from(workers.keys()); + if (roverIds.length) { + logger.info('Stopping all audio forward workers', { reason, roverIds }); + } + roverIds.forEach((roverId) => { + stopWorker(roverId); + }); + } + function writeUploadFile(roverId, payload = {}) { const { name, mime, dataBase64 } = payload || {}; const ext = extFromUpload(name, mime); @@ -497,6 +522,7 @@ function createAudioForwardWorkerEngine(deps) { return { ensureWorker, stopWorker, + stopAllWorkers, playUploadedAudio, playServerAudioFile, stopPlayback, diff --git a/server/src/services/kinectService/hardware.js b/server/src/services/kinectService/hardware.js index d602fbd7..18b6e3e3 100644 --- a/server/src/services/kinectService/hardware.js +++ b/server/src/services/kinectService/hardware.js @@ -8,6 +8,7 @@ const sharp = require('sharp'); const WORKER_PATH = process.env.KINECT_WORKER || path.join(__dirname, 'native', 'kinect_worker'); const CAPTURE_TIMEOUT_MS = 12000; const WORKER_STDERR_LOG_INTERVAL_MS = 5000; +const PROCESS_SIGNAL_EXIT_DELAY_MS = 1700; let worker = null; let stdoutBuffer = Buffer.alloc(0); @@ -230,14 +231,23 @@ async function getWorkerStatus() { function installShutdownHooks() { const shutdown = () => stopWorker(); + const exitAfterServiceCleanup = (code) => { + // Several services install signal handlers that synchronously start cleanup + // and schedule short force-kill fallbacks for child processes. Exiting here + // immediately would prevent those timers from running, so the Kinect bridge + // leaves a small process-wide grace window before forcing Node down. + setTimeout(() => { + process.exit(code); + }, PROCESS_SIGNAL_EXIT_DELAY_MS); + }; process.once('exit', shutdown); process.once('SIGINT', () => { shutdown(); - process.exit(130); + exitAfterServiceCleanup(130); }); process.once('SIGTERM', () => { shutdown(); - process.exit(143); + exitAfterServiceCleanup(143); }); }