Repository navigation
Don't reattach restored terminal editors to another window's terminal - #341692
Merged
Anthony Kim (anthonykim1) merged 6 commits intoOct 15, 2026
Merged
Conversation
Persistent process IDs restart from 1 when the pty host restarts, but terminal editors keep the IDs they were saved with. When a restored editor had no revived process, it reattached by its saved ID, which can now belong to a running terminal in another window. Pass the pid saved with the editor and only reattach by the original ID when that process still has the same pid; otherwise a new terminal is created. Also register PtyService's RPC trace listeners so the service can be created in leak-checked tests. Fixes #341502
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No actionable defects remain in the scoped attachment safeguard; reported validation was not rerun.
0 open findings
What changed in this PR
Prevents restored local terminal editors from attaching to another window’s terminal when persistent process IDs are reused, addressing #341502.
Changes:
- Validates the saved PID before attaching to an original, non-revived process.
- Preserves revived-process and ID-only attachment behavior.
- Adds regression tests and registers trace listeners for disposal.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/test/browser/terminalProcessManager.test.ts | Tests saved PID forwarding. |
| src/vs/workbench/contrib/terminal/electron-browser/localTerminalBackend.ts | Checks PID on fallback attachment. |
| src/vs/workbench/contrib/terminal/browser/terminalProcessManager.ts | Passes the saved PID during restoration. |
| src/vs/platform/terminal/test/node/ptyService.test.ts | Tests matching, mismatched, and omitted PIDs. |
| src/vs/platform/terminal/node/ptyService.ts | Rejects PID mismatches and tracks listener disposal. |
| src/vs/platform/terminal/node/ptyHostService.ts | Forwards the optional PID. |
| src/vs/platform/terminal/common/terminal.ts | Updates attachment contracts and documentation. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Relaunching installs the new persistent process before its shell reports a pid, so a terminal editor saved in between paired the new ID with the previous shell's pid, and restoring it then refused the intended process. Clear the pid when the new process is installed, so it's saved as unknown until the shell is ready. Also cover the local backend's reattach routing with tests and note that only the local backend checks the pid.
Anthony Kim (anthonykim1)
marked this pull request as ready for review
October 15, 2026 16:43
Contributor
Author
|
Megan Rogge (@meganrogge) This is an interesting one |
Anthony Kim (anthonykim1)
enabled auto-merge (squash)
October 15, 2026 16:52
Comment on lines
+243
to
+244
| // The original ID may belong to another window's process after a pty host restart | ||
| return await this.attachToProcess(id, pid > 0 ? pid : undefined); |
Collaborator
There was a problem hiding this comment.
why would this happen?
Contributor
Author
There was a problem hiding this comment.
For example, workspace B can have a saved terminal { id: 21, pid: 72265 } . After a restart, workspace A can create a different terminal that is assigned ID 21. If B is then reopened and there is no revival mapping, the old code falls back to ID 21 and attaches to A’s terminal. Comparing the saved PID would prevent that.
roblourens
approved these changes
Oct 15, 2026
Contributor
|
Base:
|
Anthony Kim (anthonykim1)
deleted the
anthonykim1/verify-restored-terminal-pid
branch
October 15, 2026 21:09
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
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.
Resolves: #341502
attachToRevivedProcess.0), and for terminals moved between windows or picked with Attach to Session, which still attach by ID only.0instead of pairing the new ID with the previous shell's pid, which the check above would refuse.pidtoIPtyService.attachToProcess, checked on the pty host, and forward it throughPtyHostService.PtyService's RPC trace listeners so the service can be constructed in leak-checked tests.LocalTerminalBackendso its reattach routing can be tested.attachToProcessdirectly and are not changed here; theattachToRevivedProcessdocs now say only the local backend checks the pid.How to test
sleep 600in the third.Expected: B starts new terminals, A's terminals stay in A, and
sleepkeeps running. The Pty Host log showsPersistent process reconnection "N" failed Pty N has pid X, expected Y. Before this change, B showed A's running terminals as editor tabs, and typing in or closing them affected A.Also check that reloading keeps the same shells, that Terminal: Relaunch Active Terminal in an editor terminal followed by a reload reattaches to the relaunched shell, and that quitting and reopening restores the terminal history.
Validation:
./scripts/test.sh --run src/vs/platform/terminal/test/node/ptyService.test.ts --run src/vs/workbench/contrib/terminal/test/browser/terminalProcessManager.test.ts --run src/vs/workbench/contrib/terminal/test/electron-browser/localTerminalBackend.test.ts: 17 passing. The new tests fail without the changes: without clearing the pid, a relaunched terminal reports{ id: 2, pid: 1001 }before it's ready, and with the previous backend routing a mismatched pid attaches to the other process../scripts/test.sh --runGlob "**/terminal/**/*.test.js": 763 passing, 14 pending.--runGlob "**/terminalContrib/**/*.test.js": 2749 passing, 176 pending.--runGlob "**/contrib/tasks/**/*.test.js": 90 passing, 5 pending.npm run typecheck-client: passed.npm run eslint -- <changed files>: passed.Pty 2 has pid 5226, expected 99559), started new terminals in B, and left A's terminals andsleeprunning.start(--inspect-ptyhost), the periodic save stored the editor as{"id":4,"pid":0}. After killing the renderer and choosing Reopen, the editor reattached the relaunched shell. On the previous commit, the same steps stored the previous shell's pid, refused the relaunched shell, and started a second one.Inspirations from:
pid, and0when it was unknown, come fromTerminalInputSerializer._toJson.PersistentTerminalProcess.start.PtyService._reviveTerminalProcess, which starts a new process and maps the old ID to it per workspace forgetRevivedPtyNewId.findRevivedIdfollowsTerminalEditorService.reviveInput, the only place that sets it.persistentProcessId, which reports the new ID as soon ascreateProcessinstalls the process, whileshellProcessIdonly changes in its ready handler.AbstractStorageService.doFlushWhenIdle.ErrorNoTelemetrymessage style comes fromPtyService._throwIfNoPty.localTerminalBackend.test.tsfollowlocalPty.test.ts.ReflectfollowsterminalProcess.test.ts.