Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restarting the pty host breaks logging completely #185875

Closed
Tyriar opened this issue Jun 22, 2023 · 15 comments · Fixed by #186471
Closed

Restarting the pty host breaks logging completely #185875

Tyriar opened this issue Jun 22, 2023 · 15 comments · Fixed by #186471
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug regression Something that used to work is now broken terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 22, 2023

Needs #185254 to be fixed to reproduce the issue

Repro:

  1. Set log level of "Pty Host" specifically to trace
  2. Create a terminal to verify trace logs show up in pty host channel
  3. Run the restart pty host command
  4. Create a terminal to verify trace logs show up in pty host channel, 🐛 they don't show up
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. labels Jun 22, 2023
@Tyriar Tyriar added this to the June 2023 milestone Jun 22, 2023
@Tyriar Tyriar self-assigned this Jun 22, 2023
@Tyriar Tyriar changed the title Restarting the pty host when log level trace is set on only that channel will end up reverting to info Restarting the pty host breaks logging completely Jun 22, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Jun 22, 2023

@sandy081 some help would be appreciated here. When the process is restarted the logging just stops working completely.

Here's where the utility process environment is created:

const config: { [key: string]: string } = {
...deepClone(process.env),
VSCODE_LAST_PTY_ID: String(lastPtyId),
VSCODE_AMD_ENTRYPOINT: 'vs/platform/terminal/node/ptyHostMain',
VSCODE_PIPE_LOGGING: 'true',
VSCODE_VERBOSE_LOGGING: 'true', // transmit console logs from server to client,
VSCODE_RECONNECT_GRACE_TIME: String(this._reconnectConstants.graceTime),
VSCODE_RECONNECT_SHORT_GRACE_TIME: String(this._reconnectConstants.shortGraceTime),
VSCODE_RECONNECT_SCROLLBACK: String(this._reconnectConstants.scrollback),
};

Here's the logger initialization on the pty host proc:

const loggerService = new LoggerService(getLogLevel(environmentService), environmentService.logsHome);
server.registerChannel(TerminalIpcChannels.Logger, new LoggerChannel(loggerService, () => DefaultURITransformer));
const logger = loggerService.createLogger('ptyhost', { name: localize('ptyHost', "Pty Host") });
const logService = new LogService(logger, [new ConsoleLogger()]);

If you try repro you'll need this change #185882

@sandy081
Copy link
Member

I can reproduce it. 👀

@sandy081
Copy link
Member

sandy081 commented Jun 23, 2023

It seems pty process is creating the log file in a new folder instead in the logs home folder.

image

It is because a new logsPath folder is getting passed to the pty process from main process. Fixing that would fix this.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 23, 2023

Weird. Thanks, I'll look into that

@Tyriar Tyriar modified the milestones: June 2023, July 2023 Jun 26, 2023
@sandy081 sandy081 added the regression Something that used to work is now broken label Jun 28, 2023
@sandy081 sandy081 modified the milestones: July 2023, June 2023 Jun 28, 2023
@sandy081
Copy link
Member

This is causing following regression - only last 5 log sessions are being retained instead of 10. Hence moved it to current milestone and prepared a fix for it.

sandy081 added a commit that referenced this issue Jun 28, 2023
sandy081 added a commit that referenced this issue Jun 28, 2023
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jun 28, 2023
@meganrogge meganrogge added the verified Verification succeeded label Jun 29, 2023
@meganrogge
Copy link
Contributor

meganrogge commented Jun 29, 2023

so the logs do show up, but the terminal doesn't seem to get created correctly - I cannot type in it.

EDIT: after restarting the pty host, I cannot type in any of the terminals.

Screen.Recording.2023-06-29.at.11.29.30.AM.mov

2023-06-29 11:29:17.471 [trace] node-pty.IPty#onData �]2;meganrogge@Megans-MacBook-Pro:~/Repos/vscode��]1;~/Repos/vscode�
2023-06-29 11:29:17.471 [trace] node-pty.IPty#onData �]633;D�
2023-06-29 11:29:17.473 [trace] node-pty.IPty#onData �]633;P;Cwd=/Users/meganrogge/Repos/vscode�
2023-06-29 11:29:17.478 [debug] CommandDetectionCapability#handleCommandFinished 0 0 undefined {"commandFinishedMarker":{"line":0,"isDisposed":false,"_disposables":[{"_listeners":[null],"_disposed":false},{},{},{},{}],"_id":151,"_onDispose":{"_listeners":[null],"_disposed":false}}}
2023-06-29 11:29:17.903 [trace] node-pty.IPty#onData 
�[0m�[27m�[24m�[J�]633;A��[01;32m➜  �[36mvscode�[00m �[01;34mgit:(�[31mmerogge/help-dispose�[34m)�[00m �]633;B��[K
2023-06-29 11:29:17.904 [trace] node-pty.IPty#onData �[?1h�=
2023-06-29 11:29:17.904 [trace] node-pty.IPty#onData �[?2004h
2023-06-29 11:29:17.912 [debug] CommandDetectionCapability#handlePromptStart 0 0
2023-06-29 11:29:17.912 [debug] CommandDetectionCapability#handleCommandStart 37 0
2023-06-29 11:29:48.316 [trace] [RPC Request] PtyService#setTerminalLayoutInfo({"workspaceId":"dbca792c1c3bf28acacb2a58cb095ae0","tabs":[{"isActive":true,"activePersistentProcessId":14,"terminals":[{"relativeSize":0.2495075508864084,"terminal":1},{"relativeSize":0.2495075508864084,"terminal":14},{"relativeSize":0.2495075508864084,"terminal":13},{"relativeSize":0.25147734734077476,"terminal":2}]}]})
2023-06-29 11:29:48.316 [trace] [RPC Response] PtyService#setTerminalLayoutInfo undefined

@meganrogge
Copy link
Contributor

the terminal created before the pty host restarted receives input but does not display it. the one created after it restarted does not receive it.

input.mov

@meganrogge
Copy link
Contributor

I don't see this log

this._logService.debug('Starting pty host');

@meganrogge
Copy link
Contributor

I ran it again and now can type in the terminals, but there are no logs happening when I type or create a new terminal
Screenshot 2023-06-29 at 11 56 29 AM

@Tyriar Tyriar removed this from the June 2023 milestone Jun 29, 2023
@Tyriar Tyriar added this to the July 2023 milestone Jun 29, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Jun 29, 2023

As long as the regression @sandy081 fixed in #185875 (comment) is good, this is low priority. Will check it out again in July.

@Tyriar Tyriar reopened this Jun 29, 2023
@Tyriar Tyriar removed the verified Verification succeeded label Jun 29, 2023
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Jun 29, 2023
@Tyriar
Copy link
Member Author

Tyriar commented Jun 30, 2023

Flaky repro, but sometimes a window might not ack the restart. See the bg window correctly shows the restart and relaunches, but the fg doesn't:

Screenshot 2023-06-30 at 8 02 18 am

Restarting again fixes it, I can't repro any problem with the log not working though.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 30, 2023

Weird, when this happens data events aren't getting fired for just one window:

image

@Tyriar
Copy link
Member Author

Tyriar commented Jun 30, 2023

On input in the broken terminal:

Screenshot 2023-06-30 at 8 31 15 am

@Tyriar
Copy link
Member Author

Tyriar commented Jun 30, 2023

The 2 windows are connected to 2 different pty hosts:

Screenshot 2023-06-30 at 8 38 10 am

@Tyriar
Copy link
Member Author

Tyriar commented Jun 30, 2023

I'm closing this issue off again as the logging problem was fixed in the linked commit. I investigated the problem @meganrogge hit and created some more targeted issues to fix:

@Tyriar Tyriar closed this as completed Jun 30, 2023
@Tyriar Tyriar added the verified Verification succeeded label Jun 30, 2023
@Tyriar Tyriar modified the milestones: July 2023, June 2023 Jun 30, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug regression Something that used to work is now broken terminal-process Problems launching processes, managing ptys, exiting, process leaks, etc. verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants