From 2d97f478554f491769fc57ae43bd99ce093e53a5 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 12 Aug 2026 12:32:09 -0400 Subject: [PATCH 1/2] fix(simulator): stop the emulator on Stop, and close its session on every stop path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes, one ownership rule: the emulator and the session it serves start and end together. 1. Stop actually stops the emulator (shared surface) The Stop button ended the debug session, logged "Simulator stopped." and left the emulator running. `stopSession()` deliberately does not stop it (a debug session is a consumer of the emulator, not its owner — see its docstring), and this button, which owns that job, never made the call. The avr8js loop kept re-scheduling itself and burning a core for the rest of the session, unreachable from the UI because the button had already flipped back to "Start". `workspace-activity-bar/default.tsx` is byte-identical with openplc-web and carries the same change there; the two must land together for the Shared Surface Sync gate. 2. Every stop path closes the session (main process) Stops were scattered, and two of them closed nothing: - `handleWindowReload` stopped the emulator but left the session open, so main went on holding a simulator session the reloaded renderer knew nothing about. - `handleSimulatorLoadFirmware`'s catch did neither. `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. All six sites now route through one `stopSimulator()` choke point that closes the session first, then stops the emulator. The load-failure path is the parity fix for openplc-web, where the worker already cleans up and reports 'stopped' when `loadAndRun` throws. Adds `simulator-session.handler.test.ts`: 5 cases over ordering, the non-simulator link left alone, the reload path and the throw path. The two covering new behaviour fail without this change. Co-Authored-By: Claude Opus 5 (1M context) --- .../workspace-activity-bar/default.tsx | 18 ++- .../simulator-session.handler.test.ts | 152 ++++++++++++++++++ src/main/modules/ipc/main.ts | 37 ++++- 3 files changed, 198 insertions(+), 9 deletions(-) create mode 100644 src/main/modules/ipc/__tests__/simulator-session.handler.test.ts diff --git a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx index 47a547008..c41914962 100644 --- a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx +++ b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx @@ -636,7 +636,23 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa const handleSimulatorControl = useCallback(async (): Promise => { try { if (simulatorRunning) { + // Two things end here, in this order. + // + // 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. await debugSession.stopSession() + + // Then the emulator itself — which 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: it kept + // re-scheduling itself and burning a core for the rest of the session, + // with no way to reach it from the UI because the button had already + // flipped back to "Start". + await simulator.stop() + setSimulatorRunning(false) addLog({ id: crypto.randomUUID(), level: 'info', message: 'Simulator stopped.' }) } else { @@ -649,7 +665,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..d827c573b --- /dev/null +++ b/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts @@ -0,0 +1,152 @@ +/** + * 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), +})) + +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) +}) + +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', async () => { + const bridge = createBridge() + const close = holdLink(bridge, 'simulator') + + // An unreadable hex path fails inside the handler's try, standing in for any + // throw during start — `loadAndRun` marks the emulator running before it + // finishes wiring, so a throw after that point used to leak one. + const result = await bridge.handleSimulatorLoadFirmware({} as never, '/nonexistent/simulator.hex') + + 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. */ From 885b7da21e73ab3ebb6474e5141cecda9ba18123 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Wed, 12 Aug 2026 14:14:39 -0400 Subject: [PATCH 2/2] fix(review): unconditional emulator stop, and test the leak the comment claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #1009 / #670. Stop the emulator from a `finally` (shared surface, mirrors openplc-web). Sequencing `simulator.stop()` after a plain `await debugSession.stopSession()` left the emulator running whenever the teardown rejected — control jumped to the catch, which only logs. Reproduced on web by injecting a rejecting `debugger.disconnect`: emulator still running, debug panel up on frozen values, `simulatorRunning` stuck true, every retry failing identically. Nothing settled, which is worse than the bug this branch fixes. The nearest trigger is a throwing `onDisconnected` subscriber, which this repo's debugger adapter re-invokes inside its own catch — the second throw escapes. Clear `debugSessionRidesDeviceRef` on the manual Stop path (shared surface). `stopSession()` hides the debugger first, so the drop handler's `isDebuggerVisible` gate returns before it reaches the reset. Fix the load-firmware test so it exercises the leak its comment describes. It threw on `fs.readFile`, which runs BEFORE `loadAndRun` — so the emulator was never marked running and the assertion passed only because `stopSimulator()` is unconditional. It would have stayed green if the real post-`loadAndRun` leak regressed. `fs/promises` is now stubbed so the read succeeds and `loadAndRun` throws, putting the throw where the leak was; the read-failure case is kept as a separate test, since the catch cannot tell the two apart. Both fail with the `stopSimulator()` call removed. 303 suites / 6404 tests pass. The modbus-rtu-client flake and the jest worker teardown warning both reproduce on a clean `development`. Co-Authored-By: Claude Opus 5 (1M context) --- .../workspace-activity-bar/default.tsx | 43 +++++++++++++------ .../simulator-session.handler.test.ts | 39 +++++++++++++++-- 2 files changed, 66 insertions(+), 16 deletions(-) diff --git a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx index c41914962..1966a169e 100644 --- a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx +++ b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx @@ -636,22 +636,41 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa const handleSimulatorControl = useCallback(async (): Promise => { try { if (simulatorRunning) { - // Two things end here, in this order. + // 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. - await debugSession.stopSession() - - // Then the emulator itself — which 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: it kept - // re-scheduling itself and burning a core for the rest of the session, - // with no way to reach it from the UI because the button had already - // flipped back to "Start". - await simulator.stop() + // + // 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.' }) diff --git a/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts b/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts index d827c573b..33452a8e8 100644 --- a/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts +++ b/src/main/modules/ipc/__tests__/simulator-session.handler.test.ts @@ -42,6 +42,13 @@ 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 { @@ -75,6 +82,7 @@ beforeEach(() => { simulatorModule.loadAndRun.mockReset() simulatorModule.stop.mockReset() simulatorModule.isRunning.mockReset().mockReturnValue(true) + readFile.mockReset().mockResolvedValue(':00000001FF') }) afterEach(() => { @@ -136,15 +144,38 @@ describe('window:reload', () => { }) describe('simulator:load-firmware', () => { - it('stops the emulator and closes the session when the start throws', async () => { + 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')) - // An unreadable hex path fails inside the handler's try, standing in for any - // throw during start — `loadAndRun` marks the emulator running before it - // finishes wiring, so a throw after that point used to leak one. 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)