Skip to content

Fix WSLENV environment variable duplication in ConptyConnection #19167

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

Merged

Conversation

Weichenleeeee123
Copy link
Contributor

Summary

This PR fixes issue #7130 where WT_SESSION and WT_PROFILE_ID environment variables were being duplicated in the WSLENV environment variable when multiple terminal sessions were created.

References and Relevant Issues

Closes #7130

Detailed Description

The previous implementation always appended WT_SESSION:WT_PROFILE_ID: to WSLENV without checking if these variables already existed, causing duplication.

Changes Made:

  • Parse existing WSLENV content to avoid duplicates
  • Only add WT_SESSION and WT_PROFILE_ID if not already present
  • Preserve existing environment variable flags
  • Maintain backward compatibility

Validation Steps Performed

  • Code compiles successfully with dotnet build
  • No build warnings or errors introduced
  • Logic verified through code review
  • Maintains existing functionality while preventing duplication

PR Checklist

Resolves microsoft#7130

Previously, WT_SESSION and WT_PROFILE_ID were always appended to WSLENV
without checking if they already existed, causing duplication when
multiple terminal sessions were created.

This change:
- Parses existing WSLENV content to avoid duplicates
- Only adds WT_SESSION and WT_PROFILE_ID if not already present
- Preserves existing environment variable flags
- Maintains backward compatibility
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Product-Terminal The new Windows Terminal. labels Jul 24, 2025
@Weichenleeeee123
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Weichenleeeee123
Copy link
Contributor Author

Hi @DHowett ,could you take a look when you have time?

@DHowett
Copy link
Member

DHowett commented Jul 25, 2025

Indeed! The team has been busy lately, but we will make sure to review this. Thank you very much for working on it!

@lhecker
Copy link
Member

lhecker commented Jul 28, 2025

I made the following changes to your PR:

  • set -> unordered_set since we don't need the ordering guarantees
  • Concatenating temporary strings and then appending to WSLENV -> Appending straight to WSLENV
  • Avoid a duplicate : separator between the added and existing WSLENV contents

@lhecker lhecker requested review from DHowett and carlos-zamora July 28, 2025 14:58
@Weichenleeeee123
Copy link
Contributor Author

@lhecker Thanks for updating my PR and for your suggestions. I learned a lot from your changes!

@lhecker
Copy link
Member

lhecker commented Jul 28, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-project-automation github-project-automation bot moved this to To Cherry Pick in 1.23 Servicing Pipeline Jul 28, 2025
@DHowett DHowett enabled auto-merge (squash) July 28, 2025 19:08
@DHowett
Copy link
Member

DHowett commented Jul 28, 2025

@lhecker you'll need to fix the audit build 🙂

@lhecker
Copy link
Member

lhecker commented Jul 28, 2025

@Weichenleeeee123 Thank you for the kind words. Normally I would've explained my "minor concerns" and made suggestions, but we're short-staffed and there's more work than we could ever handle. So, I did it myself while reviewing your code to save time. I apologize for that. 🙈🙈

If you could indeed gain something from my changes though, I'm very happy to hear that. Writing fundamental abstractions like til::safe_slice or til::split_iterator can make such code a lot simpler, but I understand that it's not easy to find our existing helpers due to the size of the project. Thank you for submitting the PR!

@DHowett
Copy link
Member

DHowett commented Jul 29, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett DHowett merged commit 7d6e0c8 into microsoft:main Jul 29, 2025
13 of 15 checks passed
@Weichenleeeee123 Weichenleeeee123 deleted the fix/wslenv-duplicate-variables branch July 29, 2025 02:09
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.23 Servicing Pipeline Jul 29, 2025
DHowett pushed a commit that referenced this pull request Jul 29, 2025
This PR fixes issue #7130 where WT_SESSION and WT_PROFILE_ID environment
variables were being duplicated in the WSLENV environment variable when
multiple terminal sessions were created.

The previous implementation always appended WT_SESSION:WT_PROFILE_ID: to
WSLENV without checking if these variables already existed, causing
duplication.

Closes #7130

Co-authored-by: Leonard Hecker <[email protected]>
(cherry picked from commit 7d6e0c8)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgdDF2w
Service-Version: 1.23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
Status: Cherry Picked
Development

Successfully merging this pull request may close these issues.

Duplicate WT_SESSION and WT_PROFILE_ID in WSLENV env var
3 participants