Fix onTerminate being ignored on attach sessions - #2075
Merged
Rich Chiodo (rchiodo) merged 1 commit intoSep 22, 2026
Merged
Rich Chiodo (rchiodo) merged 1 commit into
Rich Chiodo (rchiodo) merged 1 commit into
Conversation
"onTerminate" is a string option whose graceful value is "KeyboardInterrupt".
launch_request validates it with str, and the debug server reads it as a
string too (pydevd_process_net_command_json.py uses
args.get("onTerminate", "kill")). attach_request validates it with bool.
json.of_type(bool) does not reject a string, it coerces it, so
"KeyboardInterrupt" arrives as True and the comparison that follows is
False. _forward_terminate_request therefore stays False on every attach
session, and no value of "onTerminate" can turn the option on.
The visible effect is in terminate_request. On launch it delegates to the
server, which interrupts the main thread, so finally blocks, atexit handlers
and buffered output all get their chance. On attach it always falls through
to session.finalize(terminate_debuggee=True), which kills the process and
its subprocesses. The client accepts the option and then ignores it.
Validate it as str, same as launch. The regression test drives the real
attach handler with a parsed DAP packet and checks the flag that
terminate_request reads. It stops the handler at the first argument check
after "onTerminate", so it opens no sockets.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
| on_terminate = request("onTerminate", str, optional=True) | ||
|
|
||
| if on_terminate: | ||
| self._forward_terminate_request = on_terminate == "KeyboardInterrupt" |
Contributor
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 src/debugpy/adapter/clients.py:452
A rejected attach with "onTerminate": "KeyboardInterrupt" leaves this flag enabled if the client retries without onTerminate, causing termination to forward Ctrl+C instead of using the default hard kill. Assign unconditionally with self._forward_terminate_request = on_terminate == "KeyboardInterrupt" and add a retry regression case.
[verified]
Rich Chiodo (rchiodo)
approved these changes
Sep 22, 2026
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
Contributor
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
added
the
review-auto:approved
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Bill Schnurr (bschnurr)
approved these changes
Sep 22, 2026
Bill Schnurr (bschnurr)
left a comment
Bill Schnurr (bschnurr)
left a comment
Member
There was a problem hiding this comment.
Approved via Review Center.
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.
Fixes #2074
attach_requestvalidates"onTerminate"withboolwhilelaunch_requestvalidates it withstr, and both compare the result against the string"KeyboardInterrupt".json.of_type(bool)doesn't reject a string, it coerces it, so on attach the option arrives asTrueand the comparison is alwaysFalse._forward_terminate_requeststaysFalseon every attach session, which leaves the delegate branch interminate_requestunreachable from attach. The client accepts the option and then kills the debuggee throughsession.finalize(..., terminate_debuggee=True), with no error anywhere to say the option was dropped."onTerminate"is a string option. The debug server reads it that way (pydevd_process_net_command_json.pyusesargs.get("onTerminate", "kill")), and pydevd's coverage for it only goes through launch (tests_python/test_debugger_json.pycallswrite_launch(onTerminate=...)), which is why the attach side stayed quiet.What changed:
src/debugpy/adapter/clients.py:attach_requestvalidates"onTerminate"withstr, matchinglaunch_request. One word.tests/debugpy/adapter/test_clients.py: new parametrizedtest_attach_honors_on_terminatecovering"KeyboardInterrupt","kill", and the option being absent. It drives the realattach_requestwith a parsed DAP packet and reads back_forward_terminate_request."listen"and"connect"mutual exclusion check, which is the first check after the option is read, so it opens no socket and spawns no session._FakeSessiongained alauncher = Noneclass attribute.Client.launcherproxies to the session, and the shared start request wrapper checks it before dispatching.Testing, on macOS 26.6.2 with CPython 3.10.6 and
PYTHONPATH=$PWD/src:python3 -m pytest tests/debugpy/adapter/ -q -p no:cacheprovider -o addopts=gives 7 passed in 1.57s.python3 -m pytest tests/debugpy/common/test_messaging.py -k "not fuzz" -q -p no:cacheprovider -o addopts=gives 13 passed, 1 deselected in 2.65s.python3 -m pytest tests/debugpy/common/test_json.py tests/debugpy/common/test_singleton.py tests/debugpy/common/test_socket.py -q -p no:cacheprovider -o addopts=gives 12 passed in 2.88s.python3 -m ruff check ., which is what the Lint stage runs, passes on the whole tree.python3 -m flake8 src/debugpy/adapter/clients.py tests/debugpy/adapter/test_clients.pyis clean, exit 0.python3 -m black --check --diffon both files reports exactly the same hunks it reports for those files onmain, so nothing new was added. Both already failblack --checkupstream and I left that alone rather than mixing reformatting into this change.src/debugpy/adapter/clients.pyrestored frommainand the new test kept,test_attach_honors_on_terminate[arguments0-True]fails withassert False is True, 1 failed and 2 passed. The other two cases pass againstmainas well, since they assertFalse.I didn't run
tox, sincecommands_prebuilds the attach binaries and this change needs no rebuild.launch_requestandattach_requestnow read"onTerminate"with the same three lines. Happy to hoist them into one helper so they can't drift again, but I kept this to the one word so the fix stays easy to review.