fix(simulator): stop the emulator on Stop, and close its session on every stop path - #1009
Merged
Merged
Conversation
…very stop path 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) <noreply@anthropic.com>
Contributor
Author
|
Paired with https://github.com/Autonomy-Logic/openplc-web/pull/670 — the shared |
Contributor
WalkthroughSimulator shutdown is centralized in the main process. It closes simulator sessions before stopping emulators across stop, reload, quit, project transition, and startup-failure paths. The frontend and lifecycle tests use the updated behavior. ChangesSimulator lifecycle cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceActivityBar
participant SimulatorSession
participant Emulator
WorkspaceActivityBar->>SimulatorSession: request simulator stop
SimulatorSession->>SimulatorSession: close simulator session
SimulatorSession->>Emulator: stop emulator
Emulator-->>WorkspaceActivityBar: complete shutdown
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 644-654: Wrap the debug cleanup sequence around
debugSession.stopSession() in a try/finally so simulator.stop() always executes,
including when the session cleanup rejects. Keep stopSession() as the first
cleanup attempt, and add a rejection test verifying simulator.stop() is still
called.
In `@src/main/modules/ipc/__tests__/simulator-session.handler.test.ts`:
- Around line 143-146: Update the test around bridge.handleSimulatorLoadFirmware
to mock firmware reading as successful, then configure
simulatorModule.loadAndRun() to throw after startup begins. Assert that the
partial-start cleanup closes the session and stops the emulator, exercising the
handler’s catch path rather than the pre-load failure path.
- Around line 47-71: Update
src/main/modules/ipc/__tests__/simulator-session.handler.test.ts at lines 47-71,
85-115, and 139-146: make createBridge satisfy MainIpcModuleConstructor, replace
the private deviceSession cast in holdLink with typed DeviceSessionManager
spies, and include the required descriptor field in the session fake. Pass a
typed IpcMainInvokeEvent fixture to every handler invocation at the sibling
sites.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65423d4d-0fc3-45f0-8e4d-9ea33972477c
📒 Files selected for processing (3)
src/frontend/components/_organisms/workspace-activity-bar/default.tsxsrc/main/modules/ipc/__tests__/simulator-session.handler.test.tssrc/main/modules/ipc/main.ts
…nt claims 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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/modules/ipc/__tests__/simulator-session.handler.test.ts`:
- Line 49: Update the readFile mock declaration to use Jest 30’s single
function-type generic, defining a callable signature that accepts the existing
arguments and returns Promise<string>; preserve the current mock behavior and
parameter optionality.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11536381-ffa9-4d14-852d-41a236bb49d0
📒 Files selected for processing (2)
src/frontend/components/_organisms/workspace-activity-bar/default.tsxsrc/main/modules/ipc/__tests__/simulator-session.handler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend/components/_organisms/workspace-activity-bar/default.tsx
marconetsf
self-requested a review
August 12, 2026 19:13
marconetsf
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, as its own docstring says — and this button, which owns that job, never made the call. So 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".Confirmed on web before the fix:
simulator.isRunning()still returnedtrueafter pressing Stop. The file is byte-identical across repos, so the desktop editor had it too.Order is now explicit: the debug session lets go of the transport first, then the emulator it was talking to goes away.
2. Every stop path closes the session — main process
Stops were scattered across six sites, and two of them closed nothing:
handleWindowReloadstopped the emulator but left the session open, so main went on holding a simulator session the reloaded renderer knew nothing about. The reload resets the renderer's store todisconnected, so the two disagreed permanently.handleSimulatorLoadFirmware's catch did neither.loadAndRunmarks 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 now route through one
stopSimulator()choke point: close the session, then stop the emulator.The load-failure path is the parity fix for openplc-web, where the worker already cleans up and posts
'stopped'whenloadAndRunthrows, which the web adapter turns into a closed session. The editor's sharedSimulatorModulehas no self-stop of its own, so the catch is where that signal lives here.Verification
src/main/modules/ipc/__tests__/simulator-session.handler.test.ts— 5 cases covering close-then-stop ordering, a non-simulator link being left alone, the reload path, and the throw path. The two covering new behaviour fail without this change (verified by revertingmain.tsand re-running).tsc --noEmitclean; no new lint warnings; Prettier clean.isRunning() === false, sessiondisconnected, and a subsequent Start works. Same code path, driven over IPC here.Not covered
A crash inside the emulator's run loop (
executeBatch) still has no handler in the editor — web catches the equivalent viaworker.onerror. Closing that would mean a try/catch in the byte-identicalSimulatorModule, so I left it out of this PR rather than widen the shared surface. Worth a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit