vscode: Reattached to the wrong terminal
I just reloaded a window with my vscode-jupyter repo open, and it attached to a terminal which is already open in my vscode repo, so now this terminal is open in two windows:
The right window is the window that originally owned this terminal
One thing about this terminal is that its cwd is some other folder outside of both workspaces, if that matters.
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Reactions: 3
- Comments: 62 (56 by maintainers)
Commits related to this issue
- Prevent terminals being attached to by multiple windows I was no able to reproduce the issue where users were seeing a terminal get reconnected to from multiple windows. This is a defensive change th... — committed to microsoft/vscode by Tyriar 2 years ago
- Prevent terminals being attached to by multiple windows I was no able to reproduce the issue where users were seeing a terminal get reconnected to from multiple windows. This is a defensive change th... — committed to microsoft/vscode by Tyriar 2 years ago
- Change log level for reconnect related logs Part of #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Always throw when attaching to orphan processes Part of #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Always throw when attaching to orphan processes Part of #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Fix windows reconnecting to the wrong pty Fixes #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Change log level for reconnect related logs Part of #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Always throw when attaching to orphan processes Part of #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Fix windows reconnecting to the wrong pty Fixes #133542 — committed to microsoft/vscode by Tyriar 2 years ago
- Add debug logs for reattaching to wrong terminal Part of #133542 — committed to microsoft/vscode by Tyriar a year ago
- Log errors in getTerminalLayoutInfo Part of #133542 — committed to microsoft/vscode by Tyriar a year ago
- Prevent the same terminal being reconnected to twice Part of #186922 Part of #133542 — committed to microsoft/vscode by Tyriar a year ago
- Use workspace ID in revive pty id map Fixes #133542 — committed to microsoft/vscode by Tyriar a year ago
Ok, I figured this out. What a journey! Here is the fairly reliable repro:
./scripts/codeaandbaandband close all terminals./scripts/codea: Open 3 terminals and type “a”, “b”, “c” into them respectively, this helps identify them to ensure they’re in the correct order laterb: Open 3 terminals and type “d”, “e”, “f” into them respectively./scripts/codeaandbshould revive the processes and have all terminals in the correct order they were created (this works)aand windowbat the same timeaandbshould revive the processes and have all terminals in the correct order they were created 🐛Note that this is a timing issue and has a chance to not reproduce the problem.
This is what is meant to happen when exiting and restarting the pty host process. Terminal processes are revived (aka. re-created) and their actual pty IDs for this pty host process are mapped to, starting at 1. Notice below how the new IDs start from 1, they just represent the order they are created in which shouldn’t matter when reviving.
After that, when reloading windows, terminal processes do not get revived but reconnected to. So an
undefinedmapping is expected:What is happening and what is causing this bug as well as a related issue[^1] is that the revived pty ID is cached, so if reloading happens to use a
newIdthat is mapped to a different pty ID in_revivedPtyIdMap, it will reconnect to that one instead:The above results in:
Then on reload the pty ID mapping occurs instead of being undefined, which causes a reconnect to the wrong pty.
So in the above they get re-mapped, shuffling the order within a window and potentially connecting to a different window’s ptys:
[^1]: The folder’s terminals may not get restored at with the warning “Couldn’t get layout info, a terminal was probably disconnected” in the pty host output channel. This started happening due to one of the previous workarounds to prevent 2 windows attaching to the same pty.
With @meganrogge’s logs in #186922, I was able to reproduce the problem!
The repro is very complicated, it is essentially trying to get into a state where 2+ windows have “old pty ids” that conflict with one another, and then increasing the latency of the pty host in order to force the race condition to show itself.
codecode <folder A>The terminal with
new id undefinedend up attaching to the wrong pty.The fix for this is simple, just add workspace ID as part of the key of the old id, so
1becomes<folder B hash>-1. This should make conflicts impossible to occur.Yep, it’s not working atm for remote terminals, so:
Thanks @alexr00, it seems there’s some inconsistency with the layout info and the revived procs, so I suspect this silently error swallowing now:
https://github.com/microsoft/vscode/blob/eafde5a28081c115bb0b06e5199be8f2822a737c/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts#L301-L303
Added logs for that here: https://github.com/microsoft/vscode/pull/186732, I’ll merge for July so you might not be able to test for a week but when you do please also grab the Terminal output log as this is renderer side.
I tried to repeat my setup before updating, and it did happen again. Setup:
After update:
Logs:
I spent a few hours looking into this, here are my findings:
Couldn't get layout info, a terminal was probably disconnected Could not find pty on pty host, I tried and tried but could not reproduce this on Insiders or OSS 🙁lastPtyIdshould be correct and while I found it could cause issues when the pty host restarts (https://github.com/microsoft/vscode/issues/186645), it seems like it should work after a restartlastPtyIdwould be a lot simpler if we moved the id to being a guid instead of a number, but that would be a shame as we then need to send the guid along the wire for pretty much every request to the pty host_revivedPtyIdMapcould get stale/invalid values. Both a thorough review of the code and injecting timeouts (random, differing by window) into important calls didn’t cause problemsCurrent plan is to wait for https://github.com/microsoft/vscode/issues/186645 to merge and await a repro with these logs present, then analyze them. I’ll also likely be continuing to fiddle with terminal reconnection in debt week to improve perf further and reduce redundancies.
I just ran into this now:
Pty logs from Window 2:
Yes, I see this every time when restarting to install a new version (Gear > Restart to Update) or when changing the setting
"update.mode":It’s a little suspicious that the
_revivedPtyIdMapentry doesn’t get deleted:https://github.com/microsoft/vscode/blob/b134ea9ec2b3dbbcc8d56c93c93fec1932432496/src/vs/platform/terminal/node/ptyService.ts#L378-L381
That determines
wasRevivedhere which can bypass the orphan check:https://github.com/microsoft/vscode/blob/b134ea9ec2b3dbbcc8d56c93c93fec1932432496/src/vs/platform/terminal/node/ptyService.ts#L396-L399
Just ran into this again. I have the same terminal restored in two distinct windows. I ran into this after restarting to update Insiders on macOS (I had 6 folders opened). The Pty Host log contains the following:
https://user-images.githubusercontent.com/5047891/182702530-97d5a741-9f64-4a17-aa25-dc4e9f75f957.mp4
Maybe @joaomoreno might know if there’s a way to easily simulate a restart caused by an update. Alternatively, maybe you can try changing a setting like
"update.mode"which shows the restart prompt. I think this also leads to the terminals attaching incorrectly.@Tyriar @meganrogge Could the needed log statement to troubleshoot this on the terminal side be converted to an
[info]log. That might help track down this bug sooner.Just had this happen again, but once again I didn’t have trace logging on. I’ll try to leave it turned on for next time. Pty Host logs:
Window logs:
We should review the code around terminal adoption of orphan processes, I suspect the issue is that it’s not transactional.
Yes it would be very helpful @alexr00. I spent 90 mins yesterday trying to reproduce to no avail. If you could enable trace logging too in case you repro, that might give the insight we need
In case it is important to the issue, I’m just saying that the drop-on-document terminal that was wrongly attached to the vscode window hadn’t yet been attached to the drop-on-document window, because it was from some earlier session.
Sorry, I cursed myself because this happened again today but I probably won’t have consistent repro steps. I reloaded the window with my vscode workspace and got a terminal from this extension sample which is also open in another window, although the terminal panel had not been made visible in that other window’s session yet.
I reloaded the window several more times and always got the right terminal after that.
@meganrogge exiting the application (cmd+q) and reopening it is essentially what happens on update