fix(sagemaker): singleton window bug by aws-ajangg · Pull Request #8385 · aws/aws-toolkit-vscode

aws-ajangg · 2025-12-04T00:03:32Z

Problem

When user connects to space, it creates a duplicate window for the same space

Solution

Assign unique identifier to workspace. This will open same external window when connecting to same workspace.

Notes

Test cases added in separate pr with unit tests
Retested locally for most recent changes with new vsix

Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

amazon-inspector-ohio · 2025-12-04T00:03:36Z

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

github-actions · 2025-12-04T00:03:56Z

This pull request modifies code in src/* but no tests were added/updated.
- Confirm whether tests should be added or ensure the PR description explains why tests are not required.
This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
- Note: beta or "experiment" features that have active users should announce fixes in the changelog.
- If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

amazon-inspector-ohio · 2025-12-04T00:04:08Z

✅ I finished the code review, and didn't find any security or code quality issues.

aws-asolidu · 2025-12-04T00:16:48Z

packages/core/src/awsService/sagemaker/model.ts

                  const hyperPodEnv: NodeJS.ProcessEnv = {
                      AWS_REGION: region,
-                      SESSION_ID: session || '',
+                      SESSION_ID: hostname || '',


why can't this be session?

hostname also uses session but session is randomly generated in the presigned url and this is what's causing a new external window to be opened when the user tries to connect to the workspace.

After discussing with @edwardps, we decided to use a deterministic unique identifier for the hostname instead

edwardps · 2025-12-04T00:32:13Z

packages/core/src/awsService/sagemaker/commands.ts

+        if (domain === '' && eksClusterArn) {
+            const { accountId, region, clusterName } = parseEKSClusterArn(eksClusterArn)
            connectionType = 'sm_hp'
+            session = `${workspaceName}_${namespace}_${clusterName}_${region}_${accountId}`


${workspaceName}_${namespace}

Please check the naming convention(limits) to confirm the characters allow by k8s can be accepted as ssh hostname.

Added k8s naming convention validation

Added k8s naming convention validation

This is not to validation if it's k8s compliant. The comment is about if there is any letters allowed by k8s by not allowed by ssh as a ssh hostname.

misunderstood the ask. corrected to validate if session is compliant with ssh

aws-jasakshi · 2026-01-12T23:46:11Z

packages/core/src/awsService/sagemaker/commands.ts

        let connectionType = 'sm_dl'
-        if (domain === '') {
+        if (!domain && eksClusterArn && workspaceName && namespace) {
+            const { accountId, region, clusterName } = parseEKSClusterArn(eksClusterArn)


nit - can you use parseArn instead ? may need to import @aws-sdk/util-arn-parser

nit addressed

aws-jasakshi · 2026-01-12T23:56:51Z

Any testing or unit tests ? Can you update description if done already

aakashmandavilli96

LGTM , have you tested this locally? and also some Lint checks are failing are they expected to fail ?

aws-ajangg · 2026-01-13T18:54:39Z

@aakashmandavilli96
Updated description: tested locally with vsix.
As for failing lint checks, it's coming from SMUS and codewhisperer

edwardps · 2026-01-13T18:59:36Z

packages/core/src/awsService/sagemaker/commands.ts

+): string {
+    const sanitize = (str: string): string =>
+        str
+            .replace(/[^a-zA-Z0-9.-]/g, '-')


if capital case will cause issue, should we keep hostname all lower case?

ssh hostname naming convention is case-insensitive but keeping it lowercase-only is also a safe approach. reverted sanitation to use lower case characters only

edwardps · 2026-01-13T19:00:58Z

packages/core/src/awsService/sagemaker/commands.ts

+        str
+            .replace(/[^a-zA-Z0-9.-]/g, '-')
+            .replace(/^-+|-+$/g, '')
+            .substring(0, 50)


which attribute could exceed 50? if it's truncated, does that break our logic?

workspaceName, namespace, and clusterName could exceed 50. I adjusted limits for each attribute based on the max allotted characters per attribute

const components = [ sanitize(workspaceName, 63), // K8s limit sanitize(namespace, 63), // K8s limit sanitize(clusterName, 63), // HP cluster limit sanitize(region, 16), // Longest AWS region limit sanitize(accountId, 12), // Fixed ].filter((c) => c.length > 0) // Total: 63 + 63 + 63 + 16 + 12 + 4 separators + 3 chars for hostname header = 224 < 253 (max limit)

edwardps · 2026-01-13T19:02:25Z

packages/core/src/awsService/sagemaker/commands.ts

+
+    const session = components
+        .join('_')
+        .substring(0, 253)


Seems the account could be truncated when length is > 253. Is hostname used as unique string or used to carry over the information e.g. for reconnecting?

hostname is used as a unique string. with the adjustment of character allotment, no attribute will be truncated

ashishrp-aws · 2026-01-14T20:52:48Z

packages/core/src/shared/clients/kubectlClient.ts

+            const modifiedUrl = url.toString()
+            getLogger().info(`Connection Type: ${connectionType}`)
+            getLogger().info(`Modified Presigned URL: ${modifiedUrl}`)
+            return { type: connectionType || 'vscode-remote', url: modifiedUrl }


nit: why is the URL parsing and string conversion necessary? if so, can we add try and catch for URL parsing of presignedURL?

URL parsing is necessary because previously the eks cluster name from the eksClusterArn was used as an attribute to create a unique identifier for each session. However, the eks cluster name exceed the max char limit which was at risk of being truncated and might affect other cases like reconnect. After discussing with team, a decision was made to use the hyperpod cluster name from the clusterArn which has a satisfied the max limit.

the url string conversion is necessary in order to parse the attributes in the presigned url. without the conversion, a url object would be returned and cause a type error

URL parsing and string conversion already exist in try/catch. Are you suggesting to add another try catch inside the existing one?

laileni-aws · 2026-01-14T20:56:54Z

packages/core/src/awsService/sagemaker/commands.ts

+            const { accountId, region, clusterName } = parseArn(clusterArn)
            connectionType = 'sm_hp'
+            session = `${workspaceName}_${namespace}_${clusterName}_${region}_${accountId}`
+            if (!isValidSshHostname(session)) {


nit: avoid nested if

const sessionCandidate = `${workspaceName}_${namespace}_${clusterName}_${region}_${accountId}`; session = isValidSshHostname(sessionCandidate) ? sessionCandidate : createValidSshSession(workspaceName, namespace, clusterName, region, accountId);

nested if has been refactored

laileni-aws · 2026-01-14T20:59:33Z

packages/core/src/shared/clients/kubectlClient.ts


-            getLogger().info(`Connection Type: ${connectionType}`)
-            getLogger().info(`Presigned URL: ${presignedUrl}`)
+            const url = new URL(presignedUrl)


nit:
What if presignedUrl is undefined ? I know we catch the error but I think this will be to generic.

added safeguard to throw specific error if presignedUrl is undefined

manodnyab · 2026-01-14T22:10:31Z

packages/core/src/awsService/sagemaker/commands.ts

    }
 }

+function parseArn(arn: string): { accountId: string; region: string; clusterName: string } {


can we add tests for these utils?

tests are added to another pr with unit tests

tests are added to another pr with unit tests

TODO

Fix singleton window bug

c754300

aws-ajangg requested a review from a team as a code owner December 4, 2025 00:03

aws-asolidu reviewed Dec 4, 2025

View reviewed changes

edwardps reviewed Dec 4, 2025

View reviewed changes

aws-ajangg force-pushed the fix-singleton-window-bug branch from 83cf892 to 9ec8341 Compare December 4, 2025 01:03

k8s naming convention validation

b025fb0

aws-ajangg force-pushed the fix-singleton-window-bug branch from 9ec8341 to b025fb0 Compare December 4, 2025 18:55

aws-ajangg requested review from aws-asolidu and edwardps December 4, 2025 20:19

aws-jasakshi reviewed Jan 12, 2026

View reviewed changes

validate character compliance for ssh hostname

d851720

aws-ajangg force-pushed the fix-singleton-window-bug branch from 0a9e8db to d851720 Compare January 13, 2026 00:10

use arn parser util package

5bd3fa2

aakashmandavilli96 approved these changes Jan 13, 2026

View reviewed changes

edwardps reviewed Jan 13, 2026

View reviewed changes

Address char limit so hostname is not truncated

97465b7

edwardps approved these changes Jan 14, 2026

View reviewed changes

Merge branch 'master' into fix-singleton-window-bug

1cae5f4

ashishrp-aws approved these changes Jan 14, 2026

View reviewed changes

ashishrp-aws reviewed Jan 14, 2026

View reviewed changes

laileni-aws reviewed Jan 14, 2026

View reviewed changes

laileni-aws approved these changes Jan 14, 2026

View reviewed changes

Add safeguard for undefined presignedUrl and refactoring

f7832f2

manodnyab reviewed Jan 14, 2026

View reviewed changes

ashishrp-aws approved these changes Jan 14, 2026

View reviewed changes

laileni-aws merged commit b698a4f into aws:master Jan 15, 2026
32 of 34 checks passed

Conversation

aws-ajangg commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Notes

Uh oh!

amazon-inspector-ohio bot commented Dec 4, 2025

Uh oh!

github-actions bot commented Dec 4, 2025

Uh oh!

amazon-inspector-ohio bot commented Dec 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-ajangg Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-ajangg Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-jasakshi commented Jan 12, 2026

Uh oh!

aakashmandavilli96 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-ajangg commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashishrp-aws Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

aws-ajangg commented Dec 4, 2025 •

edited

Loading

aws-ajangg Dec 4, 2025 •

edited

Loading

aws-ajangg Jan 13, 2026 •

edited

Loading

aakashmandavilli96 left a comment •

edited

Loading

aws-ajangg commented Jan 13, 2026 •

edited

Loading

ashishrp-aws Jan 14, 2026 •

edited

Loading