Terminate the TTD target on the engine thread - #1182
Merged
Merged
Conversation
DbgEng clients are thread-affine, and ExitDispatch() is the only call documented as safe from another thread. Quit() was calling TerminateCurrentProcess() from the requesting thread while the engine thread sat in DispatchCallbacks(), which faults inside WinDbg 1.2606's new V8-based JSProvider.dll. Quit() now only raises m_terminateRequested and wakes the engine loop, which performs the terminate itself. Reproduced with a harness mirroring the adapter: 5/5 crashes before, 0/10 after on 1.2606, and 0/3 on 1.2603. WinDbg 1.2402 and 1.2603 never crashed either way. Fixes #1129 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
fuzyll
reviewed
Aug 17, 2026
fuzyll
left a comment
Contributor
There was a problem hiding this comment.
I'm not going to mark this "request changes" because I'm not super confident about my claims or the effect they'll have. But, I'm also not sure we should accept this as-is, either.
Review feedback on #1182. m_terminateRequested was only ever cleared by EngineLoop() consuming it, but the controller reuses one adapter object across launches, so a request the engine loop never got to (the trace reaching its end first, say) stayed latched and would kill the next session's target at its first control operation. Clear it once per session, at the top of EngineLoop(): both the launch and the attach path run it on the engine thread, and the worker queue cannot deliver a Quit() before the launch operation completes, so no live request can be lost there. Also log a failed terminate. Quit() has already reported success to the controller by then, so the engine loop is the only place the failure can be surfaced. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
fuzyll
approved these changes
Aug 17, 2026
fuzyll
left a comment
Contributor
There was a problem hiding this comment.
I still think there has to be something better we can do when the debug adapter is stuck, but at least if we're able to warn/error there, it's useful.
Also appears the other issue I raised is now addressed. If we're confident the ExecStatus() is fine, I have no remaining issues with this PR. Thanks!
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
2 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.
DbgEng clients are thread-affine, and
ExitDispatch()is the only call documented as safe from another thread.Quit()was callingTerminateCurrentProcess()from the requesting thread while the engine thread sat inDispatchCallbacks(), which faults inside WinDbg 1.2606's new V8-basedJSProvider.dll.Quit()now only raisesm_terminateRequestedand wakes the engine loop, which performs the terminate itself.Reproduced with a harness mirroring the adapter (callbacks, worker thread, engine loop): 5/5 crashes before, 0/10 after on 1.2606, and 0/3 on 1.2603. WinDbg 1.2402 and 1.2603 never crashed either way, so the pinned version was already a valid mitigation.
Untested path: if the trace is replaying rather than sitting at a break,
Quit()falls back toSetInterrupt(); the harness always stopped at a break.Fixes #1129
🤖 Generated with Claude Code