diff --git a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx index 47a547008..1966a169e 100644 --- a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx +++ b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx @@ -636,7 +636,42 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa const handleSimulatorControl = useCallback(async (): Promise => { try { if (simulatorRunning) { - await debugSession.stopSession() + // Two things end here, in this order, and the emulator's end is not + // conditional on the debug session's. + // + // The debug session goes first: it is a CONSUMER of the emulator, so it + // has to let go of the transport before the thing on the other end + // disappears. + // + // The emulator goes second, from a `finally`, because stopping it is + // this button's job and nothing else's. `stopSession()` deliberately + // does not do it (see its docstring: a debug session is not the owner of + // the thing it talks to), so with this call missing "Stop" ended the + // debug session, logged "Simulator stopped." and left the avr8js loop + // running — re-scheduling itself and burning a core for the rest of the + // session, unreachable because the button had flipped back to "Start". + // + // Sequencing it after a plain `await` reintroduced the same leak on the + // error path: anything that rejects inside the teardown (today only a + // throwing `onDisconnected` subscriber, which is why nothing hits it + // yet) skipped straight to the catch below, which only logs. The + // emulator kept running, `simulatorRunning` stayed true, and every + // retry failed identically — worse than the original bug, because + // nothing settled at all. `finally` keeps the order and drops the + // condition. + try { + await debugSession.stopSession() + } finally { + await simulator.stop() + } + + // The session this debug session was riding is gone, so the claim that it + // rides one goes with it. The drop handler cannot clear it on this path: + // `stopSession()` has already hidden the debugger, so the handler's + // `isDebuggerVisible` gate returns before it reaches the reset — leaving + // the ref stale-`true` for a session that no longer exists. + debugSessionRidesDeviceRef.current = false + setSimulatorRunning(false) addLog({ id: crypto.randomUUID(), level: 'info', message: 'Simulator stopped.' }) } else { @@ -649,7 +684,7 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa pendingSimulatorDebugRef.current = false addLog({ id: crypto.randomUUID(), level: 'error', message: `Simulator control error: ${getErrorMessage(error)}` }) } - }, [debugSession, simulatorRunning, addLog]) + }, [debugSession, simulator, simulatorRunning, addLog]) // --------------------------------------------------------------------------- // MD5 verification — runs after debug compilation for non-simulator diff --git a/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts b/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts new file mode 100644 index 000000000..33452a8e8 --- /dev/null +++ b/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts @@ -0,0 +1,183 @@ +/** + * The simulator's session lifecycle, in the main process. + * + * What is worth testing HERE is the ownership rule rather than the emulator + * itself: starting the emulator opens this target's session and stopping it + * closes it, every stop path routes through the one choke point, and the order + * is session-then-emulator so the debug client is dropped before the thing it + * talks to disappears. + * + * The paths that used to leak — a window reload and a start that threw — are the + * reason this file exists: both left a running emulator behind, and the reload + * also left main holding a session the reloaded renderer knew nothing about. + */ + +import MainProcessBridge from '../main' + +jest.mock('electron', () => ({ + app: { getPath: jest.fn(() => '/tmp'), quit: jest.fn() }, + dialog: {}, + nativeTheme: { shouldUseDarkColors: false, themeSource: 'system' }, + shell: { openExternal: jest.fn() }, +})) + +jest.mock('@root/backend/editor/ethercat', () => ({ ESIService: jest.fn() })) +jest.mock('@root/backend/editor/library-manager/desktop-catalog-transport', () => ({ + createDesktopCatalogTransport: jest.fn(() => ({})), +})) +jest.mock('@root/backend/editor/utils/runtime-https-config', () => ({ getRuntimeHttpsOptions: jest.fn(() => ({})) })) +jest.mock('@root/backend/shared/ethercat/esi-parser-main', () => ({ parseESIDeviceFull: jest.fn() })) +jest.mock('@root/backend/shared/library/public-catalog-client', () => ({ listPublicLibraries: jest.fn() })) +jest.mock('../../../../backend/editor/library-manager', () => ({ + LibraryManagerModule: jest.fn(() => ({ loadEnabledArchives: jest.fn(() => ({ archives: [], missing: [] })) })), +})) +jest.mock('../../../../backend/editor/package-manager', () => ({ PackageManagerModule: jest.fn(() => ({})) })) +jest.mock('../../../../backend/editor/services', () => ({ + logger: { error: jest.fn(), info: jest.fn(), warn: jest.fn() }, +})) +jest.mock('../../../../backend/editor/utils', () => ({ getOpenProjectPath: jest.fn(), getProjectPath: jest.fn() })) + +const simulatorModule = { loadAndRun: jest.fn(), stop: jest.fn(), isRunning: jest.fn(() => true) } +jest.mock('../../../../backend/shared/simulator/simulator-module', () => ({ + SimulatorModule: jest.fn(() => simulatorModule), +})) + +// The handler reads the hex off disk before it starts anything. Stubbing the read +// to SUCCEED is what lets a test put the throw where the leak actually was — +// after `loadAndRun` has marked the emulator running. A failing read throws +// before that and proves nothing about it. +const readFile = jest.fn, [string, string?]>() +jest.mock('fs/promises', () => ({ readFile: (...args: [string, string?]) => readFile(...args) })) + +type Bridge = MainProcessBridge + +function createBridge(): Bridge { + return new MainProcessBridge({ + ipcMain: {}, + mainWindow: { + isDestroyed: jest.fn(() => false), + isMaximized: jest.fn(() => false), + webContents: { reload: jest.fn(), send: jest.fn() }, + }, + projectService: {}, + store: { get: jest.fn(() => undefined) }, + menuBuilder: {}, + pouService: {}, + compilerModule: {}, + hardwareModule: { isSerialPortPresent: jest.fn(() => true) }, + } as never) +} + +/** + * Pretend the session manager holds a link of the given transport, and record + * whether it is closed. Returns the recorder for the closing side. + */ +function holdLink(bridge: Bridge, transport: 'simulator' | 'serial') { + const session = (bridge as unknown as { deviceSession: { getLink: () => unknown; close: () => void } }).deviceSession + jest.spyOn(session, 'getLink').mockReturnValue({ transport }) + return jest.spyOn(session, 'close').mockImplementation(() => undefined) +} + +beforeEach(() => { + simulatorModule.loadAndRun.mockReset() + simulatorModule.stop.mockReset() + simulatorModule.isRunning.mockReset().mockReturnValue(true) + readFile.mockReset().mockResolvedValue(':00000001FF') +}) + +afterEach(() => { + jest.restoreAllMocks() +}) + +describe('simulator:stop', () => { + it('closes the session and stops the emulator', async () => { + const bridge = createBridge() + const close = holdLink(bridge, 'simulator') + + await expect(bridge.handleSimulatorStop({} as never)).resolves.toEqual({ success: true }) + + expect(close).toHaveBeenCalledTimes(1) + expect(simulatorModule.stop).toHaveBeenCalledTimes(1) + }) + + it('closes the session BEFORE stopping the emulator', async () => { + const bridge = createBridge() + const order: string[] = [] + const session = (bridge as unknown as { deviceSession: { getLink: () => unknown; close: () => void } }) + .deviceSession + jest.spyOn(session, 'getLink').mockReturnValue({ transport: 'simulator' }) + jest.spyOn(session, 'close').mockImplementation(() => { + order.push('session-closed') + }) + simulatorModule.stop.mockImplementation(() => order.push('emulator-stopped')) + + await bridge.handleSimulatorStop({} as never) + + expect(order).toEqual(['session-closed', 'emulator-stopped']) + }) + + it('leaves a non-simulator session alone', async () => { + const bridge = createBridge() + const close = holdLink(bridge, 'serial') + + await bridge.handleSimulatorStop({} as never) + + // A cabled board's link is not the emulator's to close, but the emulator + // still stops. + expect(close).not.toHaveBeenCalled() + expect(simulatorModule.stop).toHaveBeenCalledTimes(1) + }) +}) + +describe('window:reload', () => { + it('takes the session down with the emulator', () => { + const bridge = createBridge() + const close = holdLink(bridge, 'simulator') + + bridge.handleWindowReload() + + // The reload resets the renderer's store to 'disconnected'; a session left + // open here would be one only the main process still believes in. + expect(close).toHaveBeenCalledTimes(1) + expect(simulatorModule.stop).toHaveBeenCalledTimes(1) + }) +}) + +describe('simulator:load-firmware', () => { + it('stops the emulator and closes the session when the start throws AFTER it is running', async () => { + const bridge = createBridge() + const close = holdLink(bridge, 'simulator') + + // The leak this covers: `loadAndRun` marks the emulator running before it + // finishes wiring, so a throw from that point on left one running with no + // session behind it. The read must succeed for the throw to land there — + // throwing on the read instead would exercise a path where the emulator was + // never started, and would stay green even if this leak came back. + simulatorModule.loadAndRun.mockImplementation(() => { + throw new Error('avr8js refused the hex') + }) + + const result = await bridge.handleSimulatorLoadFirmware({} as never, '/tmp/simulator.hex') + + expect(readFile).toHaveBeenCalledTimes(1) + expect(simulatorModule.loadAndRun).toHaveBeenCalledTimes(1) + expect(result).toEqual({ success: false, error: 'avr8js refused the hex' }) + expect(simulatorModule.stop).toHaveBeenCalledTimes(1) + expect(close).toHaveBeenCalledTimes(1) + }) + + it('stops the emulator and closes the session when the read fails before it starts', async () => { + const bridge = createBridge() + const close = holdLink(bridge, 'simulator') + readFile.mockRejectedValue(new Error('ENOENT')) + + const result = await bridge.handleSimulatorLoadFirmware({} as never, '/nonexistent/simulator.hex') + + // Nothing was started, so there is nothing to leak — but the cleanup still + // has to be harmless, because the catch cannot tell the two apart. + expect(simulatorModule.loadAndRun).not.toHaveBeenCalled() + expect(result.success).toBe(false) + expect(simulatorModule.stop).toHaveBeenCalledTimes(1) + expect(close).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/modules/ipc/main.ts b/src/main/modules/ipc/main.ts index 387d3c5ff..c2f047871 100644 --- a/src/main/modules/ipc/main.ts +++ b/src/main/modules/ipc/main.ts @@ -1487,7 +1487,7 @@ class MainProcessBridge implements MainIpcModule { } } handleAppQuit = () => { - this.simulatorModule.stop() + this.stopSimulator() if (this.mainWindow) { this.mainWindow.destroy() } @@ -1579,7 +1579,10 @@ class MainProcessBridge implements MainIpcModule { } } handleWindowReload = () => { - this.simulatorModule.stop() + // The reload wipes the renderer's store back to 'disconnected', so the + // session has to go with the emulator — otherwise main keeps holding an open + // simulator session that the reloaded UI has no idea about. + this.stopSimulator() this.mainWindow?.webContents.reload() } handleWindowRebuildMenu = () => { @@ -2573,8 +2576,7 @@ class MainProcessBridge implements MainIpcModule { /** Stops the simulator and notifies the renderer so it can update UI state. */ private stopSimulatorAndNotify(): void { if (this.simulatorModule.isRunning()) { - this.closeSimulatorSession() - this.simulatorModule.stop() + this.stopSimulator() this.mainWindow?.webContents.send('simulator:stopped') } } @@ -2825,26 +2827,45 @@ class MainProcessBridge implements MainIpcModule { this.toDeviceLinkCandidates([{ connectionType: 'simulator', connectionParams: {} }]), ) if (!opened.ok) { - this.simulatorModule.stop() + this.stopSimulator() const reason = opened.attempts.map((attempt) => attempt.error).join('; ') return { success: false, error: reason || 'The simulator did not answer its debug protocol' } } this.debuggerConnectionType = 'simulator' return { success: true } } catch (error) { + // A start that threw part-way still leaves state behind: `loadAndRun` + // marks the emulator running before it finishes wiring, so a throw after + // that point leaked a running emulator with no session — and no button to + // reach it, because the renderer never learned it had started. Web ends up + // in the right place through its worker, which cleans up and reports + // 'stopped' when `loadAndRun` throws; this is the editor's counterpart. + this.stopSimulator() return { success: false, error: getErrorMessage(error) } } } /** * Stop the emulator entirely — the simulator's Stop button means "stop the - * simulator", not "stop the program it is running". The session closes first so - * the client is dropped before the thing it talks to disappears. + * simulator", not "stop the program it is running". */ handleSimulatorStop = (_event: IpcMainInvokeEvent): Promise<{ success: boolean }> => { + this.stopSimulator() + return Promise.resolve({ success: true }) + } + + /** + * The one way the emulator stops: session first, then the emulator, so the + * debug client is dropped before the thing it talks to disappears. + * + * Every stop path routes through here on purpose. The session is a consumer of + * the emulator, so an emulator that goes away while its session stays open + * leaves the renderer gated on a session whose target no longer exists — which + * a window reload and a failed start both used to do. + */ + private stopSimulator(): void { this.closeSimulatorSession() this.simulatorModule.stop() - return Promise.resolve({ success: true }) } /** Close the session if it is the simulator's. No-op for any other target. */