From ff95934f0778556d17c94dda00fd58af2ca59b37 Mon Sep 17 00:00:00 2001 From: sudacode Date: Thu, 19 Mar 2026 21:32:51 -0700 Subject: [PATCH] fix(launcher): address newest PR review feedback --- launcher/commands/command-modules.test.ts | 58 +++++++++++++++++++ launcher/commands/stats-command.ts | 39 +++++++++---- launcher/mpv.test.ts | 68 +++++++++++++++++++++++ launcher/mpv.ts | 10 +++- 4 files changed, 164 insertions(+), 11 deletions(-) diff --git a/launcher/commands/command-modules.test.ts b/launcher/commands/command-modules.test.ts index c926a97..aa4289e 100644 --- a/launcher/commands/command-modules.test.ts +++ b/launcher/commands/command-modules.test.ts @@ -409,3 +409,61 @@ test('stats cleanup command fails if attached app exits before startup response' }); }, /Stats app exited before startup response \(status 2\)\./); }); + +test('stats command aborts pending response wait when app exits before startup response', async () => { + const context = createContext(); + context.args.stats = true; + let aborted = false; + + await assert.rejects(async () => { + await runStatsCommand(context, { + createTempDir: () => '/tmp/subminer-stats-test', + joinPath: (...parts) => parts.join('/'), + runAppCommandAttached: async () => 2, + waitForStatsResponse: async (_responsePath, signal) => + await new Promise((resolve) => { + signal?.addEventListener( + 'abort', + () => { + aborted = true; + resolve({ ok: false, error: 'aborted' }); + }, + { once: true }, + ); + }), + removeDir: () => {}, + }); + }, /Stats app exited before startup response \(status 2\)\./); + + assert.equal(aborted, true); +}); + +test('stats cleanup command aborts pending response wait when app exits before startup response', async () => { + const context = createContext(); + context.args.stats = true; + context.args.statsCleanup = true; + context.args.statsCleanupVocab = true; + let aborted = false; + + await assert.rejects(async () => { + await runStatsCommand(context, { + createTempDir: () => '/tmp/subminer-stats-test', + joinPath: (...parts) => parts.join('/'), + runAppCommandAttached: async () => 2, + waitForStatsResponse: async (_responsePath, signal) => + await new Promise((resolve) => { + signal?.addEventListener( + 'abort', + () => { + aborted = true; + resolve({ ok: false, error: 'aborted' }); + }, + { once: true }, + ); + }), + removeDir: () => {}, + }); + }, /Stats app exited before startup response \(status 2\)\./); + + assert.equal(aborted, true); +}); diff --git a/launcher/commands/stats-command.ts b/launcher/commands/stats-command.ts index 09c17d2..7ce11cb 100644 --- a/launcher/commands/stats-command.ts +++ b/launcher/commands/stats-command.ts @@ -20,7 +20,10 @@ type StatsCommandDeps = { logLevel: LauncherCommandContext['args']['logLevel'], label: string, ) => Promise; - waitForStatsResponse: (responsePath: string) => Promise; + waitForStatsResponse: ( + responsePath: string, + signal?: AbortSignal, + ) => Promise; removeDir: (targetPath: string) => void; }; @@ -31,9 +34,15 @@ const defaultDeps: StatsCommandDeps = { joinPath: (...parts) => path.join(...parts), runAppCommandAttached: (appPath, appArgs, logLevel, label) => runAppCommandAttached(appPath, appArgs, logLevel, label), - waitForStatsResponse: async (responsePath) => { + waitForStatsResponse: async (responsePath, signal) => { const deadline = Date.now() + STATS_STARTUP_RESPONSE_TIMEOUT_MS; while (Date.now() < deadline) { + if (signal?.aborted) { + return { + ok: false, + error: 'Cancelled waiting for stats dashboard startup response.', + }; + } try { if (fs.existsSync(responsePath)) { return JSON.parse(fs.readFileSync(responsePath, 'utf8')) as StatsCommandResponse; @@ -66,6 +75,16 @@ export async function runStatsCommand( const tempDir = resolvedDeps.createTempDir('subminer-stats-'); const responsePath = resolvedDeps.joinPath(tempDir, 'response.json'); + const createResponseWait = () => { + const controller = new AbortController(); + return { + controller, + promise: resolvedDeps + .waitForStatsResponse(responsePath, controller.signal) + .then((response) => ({ kind: 'response' as const, response })), + }; + }; + try { const forwarded = args.statsCleanup ? ['--stats', '--stats-response-path', responsePath] @@ -102,19 +121,19 @@ export async function runStatsCommand( } if (!args.statsCleanup && !args.statsStop) { + const responseWait = createResponseWait(); const startupResult = await Promise.race([ - resolvedDeps - .waitForStatsResponse(responsePath) - .then((response) => ({ kind: 'response' as const, response })), + responseWait.promise, attachedExitPromise.then((status) => ({ kind: 'exit' as const, status })), ]); if (startupResult.kind === 'exit') { if (startupResult.status !== 0) { + responseWait.controller.abort(); throw new Error( `Stats app exited before startup response (status ${startupResult.status}).`, ); } - const response = await resolvedDeps.waitForStatsResponse(responsePath); + const response = await responseWait.promise.then((result) => result.response); if (!response.ok) { throw new Error(response.error || 'Stats dashboard failed to start.'); } @@ -130,20 +149,20 @@ export async function runStatsCommand( return true; } const attachedExitPromiseCleanup = attachedExitPromise; + const responseWait = createResponseWait(); const startupResult = await Promise.race([ - resolvedDeps - .waitForStatsResponse(responsePath) - .then((response) => ({ kind: 'response' as const, response })), + responseWait.promise, attachedExitPromiseCleanup.then((status) => ({ kind: 'exit' as const, status })), ]); if (startupResult.kind === 'exit') { if (startupResult.status !== 0) { + responseWait.controller.abort(); throw new Error( `Stats app exited before startup response (status ${startupResult.status}).`, ); } - const response = await resolvedDeps.waitForStatsResponse(responsePath); + const response = await responseWait.promise.then((result) => result.response); if (!response.ok) { throw new Error(response.error || 'Stats dashboard failed to start.'); } diff --git a/launcher/mpv.test.ts b/launcher/mpv.test.ts index d6b0a36..16dbe0d 100644 --- a/launcher/mpv.test.ts +++ b/launcher/mpv.test.ts @@ -9,15 +9,45 @@ import type { Args } from './types'; import { cleanupPlaybackSession, findAppBinary, + launchTexthookerOnly, parseMpvArgString, runAppCommandCaptureOutput, shouldResolveAniSkipMetadata, + stopOverlay, startOverlay, state, waitForUnixSocketReady, } from './mpv'; import * as mpvModule from './mpv'; +class ExitSignal extends Error { + code: number; + + constructor(code: number) { + super(`exit:${code}`); + this.code = code; + } +} + +function withProcessExitIntercept(callback: () => void): ExitSignal { + const originalExit = process.exit; + try { + process.exit = ((code?: number) => { + throw new ExitSignal(code ?? 0); + }) as typeof process.exit; + callback(); + } catch (error) { + if (error instanceof ExitSignal) { + return error; + } + throw error; + } finally { + process.exit = originalExit; + } + + throw new Error('expected process.exit'); +} + function createTempSocketPath(): { dir: string; socketPath: string } { const baseDir = path.join(process.cwd(), '.tmp', 'launcher-mpv-tests'); fs.mkdirSync(baseDir, { recursive: true }); @@ -71,6 +101,44 @@ test('parseMpvArgString preserves empty quoted tokens', () => { ]); }); +test('launchTexthookerOnly exits non-zero when app binary cannot be spawned', () => { + const error = withProcessExitIntercept(() => { + launchTexthookerOnly('/definitely-missing-subminer-binary', makeArgs()); + }); + + assert.equal(error.code, 1); +}); + +test('stopOverlay logs a warning when stop command cannot be spawned', () => { + const originalWrite = process.stdout.write; + const writes: string[] = []; + const overlayProc = { + killed: false, + kill: () => true, + } as unknown as NonNullable; + + try { + process.stdout.write = ((chunk: string | Uint8Array) => { + writes.push(Buffer.isBuffer(chunk) ? chunk.toString('utf8') : String(chunk)); + return true; + }) as typeof process.stdout.write; + state.stopRequested = false; + state.overlayManagedByLauncher = true; + state.appPath = '/definitely-missing-subminer-binary'; + state.overlayProc = overlayProc; + + stopOverlay(makeArgs({ logLevel: 'warn' })); + + assert.ok(writes.some((text) => text.includes('Failed to stop SubMiner overlay'))); + } finally { + process.stdout.write = originalWrite; + state.stopRequested = false; + state.overlayManagedByLauncher = false; + state.appPath = ''; + state.overlayProc = null; + } +}); + test('waitForUnixSocketReady returns false when socket never appears', async () => { const { dir, socketPath } = createTempSocketPath(); try { diff --git a/launcher/mpv.ts b/launcher/mpv.ts index b10a3fa..1a1095d 100644 --- a/launcher/mpv.ts +++ b/launcher/mpv.ts @@ -703,6 +703,9 @@ export function launchTexthookerOnly(appPath: string, args: Args): never { stdio: 'inherit', env: buildAppEnv(), }); + if (result.error) { + fail(`Failed to launch texthooker mode: ${result.error.message}`); + } process.exit(result.status ?? 0); } @@ -716,10 +719,15 @@ export function stopOverlay(args: Args): void { const stopArgs = ['--stop']; if (args.logLevel !== 'info') stopArgs.push('--log-level', args.logLevel); - spawnSync(state.appPath, stopArgs, { + const result = spawnSync(state.appPath, stopArgs, { stdio: 'ignore', env: buildAppEnv(), }); + if (result.error) { + log('warn', args.logLevel, `Failed to stop SubMiner overlay: ${result.error.message}`); + } else if (typeof result.status === 'number' && result.status !== 0) { + log('warn', args.logLevel, `SubMiner overlay stop command exited with status ${result.status}`); + } if (state.overlayProc && !state.overlayProc.killed) { try {