fix(smus): unble to refresh status on newly created App bug #8258

PotatoWKY · 2025-11-05T01:42:35Z

Problem

Remote connection failed due to not able to refresh the status of a newly created App because it is not mapped to the space corresponding to it.

Solution

ListApp instead of DescribeApp when the space doesn't have a App mapping to it

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.

github-actions · 2025-11-05T01:42:47Z

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.

dylanraws · 2025-11-06T17:22:13Z

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

+        let app
+        if (this.spaceApp.App?.AppName) {
+            app = await this.client.describeApp({
+                DomainId: this.spaceApp.DomainId,
+                AppName: this.spaceApp.App?.AppName,
+                AppType: this.spaceApp?.SpaceSettingsSummary?.AppType,
+                SpaceName: this.spaceApp.SpaceName,
+            })
+        } else if (this.spaceApp.DomainId && this.spaceApp.SpaceName) {
+            app = await this.client.getAppBySpace(this.spaceApp.DomainId, this.spaceApp.SpaceName)
+        }


Can we add unit tests?

dylanraws · 2025-11-06T17:23:01Z

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

+        if (!this.spaceApp.App) {
+            this.spaceApp.App = spaceApp.App
+        }


Under what circumstances will this apply, and what problem is this addressing?

this resolve the edge case when this.spaceApp.App is null.
where it is unable to update the status of the app if app is null because current logic to update the app is only updating it's status as a string, if the App object itself is null, then we can not update its status

public setSpaceStatus(spaceStatus: string, appStatus: string) { this.spaceApp.Status = spaceStatus if (this.spaceApp.App) { this.spaceApp.App.Status = appStatus } }

vpbhargav · 2025-11-06T18:24:43Z

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

    }

    public updateSpace(spaceApp: SagemakerSpaceApp) {
+        if (!this.spaceApp.App) {


Can you add a comment to clarify when this will happen, finding it hard to understand.

I know not from this change but spaceApp.App just feels very confusing, if there is scope to rename to better variables, we should.

True, do you have any suggestions to replace "spaceApp", basically spaceApp is a space that have a App as one of the variables attached to it.

we would also need to change the name for the SagemakerSpaceApp type:

export interface SagemakerSpaceApp extends SpaceDetails { App?: AppDetails DomainSpaceKey: string }

what about type: "SagemakerSpaceDetails", variable just: "space" or "spaceDetails"?

Just noticed that SagemakerSpace is the class name, so the current naming can be more confusing, like:

SagemakerSpace.spaceApp.App.AppName

a lot of spaces and apps

what about:
SagemakerSpace.spaceDetails.appDetails.AppName

vpbhargav · 2025-11-06T18:26:29Z

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

        this.contextValue = this.getContext()
    }

    public updateSpace(spaceApp: SagemakerSpaceApp) {


When all is this invoked and why does it help in this case?

vpbhargav · 2025-11-06T18:27:07Z

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

-            SpaceName: this.spaceApp.SpaceName,
-        })
+        let app
+        if (this.spaceApp.App?.AppName) {


Please add comments to clarify such conditional statements. Helps preserve context

vpbhargav · 2025-11-06T18:28:34Z

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

+            })
+        } else if (this.spaceApp.DomainId && this.spaceApp.SpaceName) {
+            app = await this.client.getAppBySpace(this.spaceApp.DomainId, this.spaceApp.SpaceName)
+        }


else? Are there other possible cases?

There are no other cases, gonna add error handling for else case

vpbhargav · 2025-11-06T18:29:15Z

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

        return this.makeRequest(DeleteAppCommand, request)
    }

+    public async getAppBySpace(domainId: string, spaceName: string): Promise<AppDetails | undefined> {


Nit : Can just rename this to listAppsForSpace so it describes what it does.

vpbhargav · 2025-11-06T18:30:03Z

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

+        const appsList = await this.listApps({ DomainIdEquals: domainId, SpaceNameEquals: spaceName })
+            .flatten()
+            .promise()
+        return appsList[0]


I guess currently it is one app for a space, add a comment to clarify maybe.

yes current one app for one space, gonna add inline comment

vpbhargav · 2025-11-07T17:10:10Z

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

        // AWS DescribeSpace API returns full details with property names like 'SpaceSettings'
        // but our internal SagemakerSpaceApp type expects 'SpaceSettingsSummary' (from ListSpaces API)
        // We destructure and rename properties to maintain type compatibility


Given this, should we just make the above List alone? IIUC they are calling DescribeApp and then converting response to mach ListApps response structure? Am I understanding this wrong?

Might simplify the above logic if so, thoughts?

I see your point.

I think we may want to do use ListSpaces alone for spaces two:
now we are converting SpaceSettings (DescribeSpace) to SpaceSettingsSummary (ListSpace).

so we get rid of the DescribeApp, and only ListApps?:

let app if (this.spaceApp.DomainId && this.spaceApp.SpaceName) { app = await this.client.listAppForSpace(this.spaceApp.DomainId, this.spaceApp.SpaceName) } else { logger.error( `updateSpaceAppStatus: unable to get app, [AppName: ${this.spaceApp.App?.AppName}], [DomainId: ${this.spaceApp.DomainId}], [SpaceName: ${this.spaceApp.SpaceName}]` ) throw new ToolkitError( `Cannot update app status without [AppName: ${this.spaceApp.App?.AppName}] or [DomainId: ${this.spaceApp.DomainId} and SpaceName: ${this.spaceApp.SpaceName}]` ) }

vpbhargav · 2025-11-08T01:06:25Z

packages/core/src/test/awsService/sagemaker/sagemakerSpace.test.ts

            assert.strictEqual(updateSpaceArgs.SpaceSharingSettingsSummary, undefined)
        })
+
+        it('should use listAppForSpace when AppName is not available', async function () {


No longer the same condition but feel free to update in some other PR.

vpbhargav

Please test for both SMUS and SM AI thoroughly.

Will-ShaoHua · 2025-11-10T20:52:38Z

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

+        let app
+        if (this.spaceApp.DomainId && this.spaceApp.SpaceName) {
+            app = await this.client.listAppForSpace(this.spaceApp.DomainId, this.spaceApp.SpaceName)
+        } else {
+            logger.error(
+                `updateSpaceAppStatus: unable to get app, [DomainId: ${this.spaceApp.DomainId}], [SpaceName: ${this.spaceApp.SpaceName}]`
+            )
+            throw new ToolkitError(
+                `Cannot update app status without [DomainId: ${this.spaceApp.DomainId} and SpaceName: ${this.spaceApp.SpaceName}]`
+            )
+        }


nit

const app = (this.spaceApp.DomainId && this.spaceApp.SpaceName) ? await this.client.listAppForSpace(this.spaceApp.DomainId, this.spaceApp.SpaceName) : undefined if (!app) { logger.error( `updateSpaceAppStatus: unable to get app, [DomainId: ${this.spaceApp.DomainId}], [SpaceName: ${this.spaceApp.SpaceName}]` ) throw new ToolkitError( `Cannot update app status without [DomainId: ${this.spaceApp.DomainId} and SpaceName: ${this.spaceApp.SpaceName}]` ) }

PotatoWKY requested a review from a team as a code owner November 5, 2025 01:42

PotatoWKY changed the title ~~fix(SMUS): unble to refresh status on newly created App bug~~ fix(smus): unble to refresh status on newly created App bug Nov 5, 2025

PotatoWKY closed this Nov 5, 2025

PotatoWKY reopened this Nov 5, 2025

dylanraws reviewed Nov 6, 2025

View reviewed changes

vpbhargav reviewed Nov 6, 2025

View reviewed changes

dylanraws approved these changes Nov 7, 2025

View reviewed changes

vpbhargav reviewed Nov 7, 2025

View reviewed changes

PotatoWKY force-pushed the master branch 2 times, most recently from bb83761 to 01db476 Compare November 8, 2025 00:27

vpbhargav reviewed Nov 8, 2025

View reviewed changes

vpbhargav approved these changes Nov 8, 2025

View reviewed changes

Will-ShaoHua approved these changes Nov 10, 2025

View reviewed changes

Will-ShaoHua reviewed Nov 10, 2025

View reviewed changes

PotatoWKY force-pushed the master branch 3 times, most recently from 8a84918 to 7fd8d39 Compare November 10, 2025 23:39

fix(smus): unble to refresh status on newly created App bug

a0e0e1f

PotatoWKY force-pushed the master branch from 7fd8d39 to a0e0e1f Compare November 10, 2025 23:47

XiaoxuanLu approved these changes Nov 11, 2025

View reviewed changes

Will-ShaoHua merged commit 7a03794 into aws:master Nov 11, 2025
31 checks passed

fix(smus): unble to refresh status on newly created App bug #8258

fix(smus): unble to refresh status on newly created App bug #8258

Conversation

PotatoWKY commented Nov 5, 2025

Problem

Solution

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PotatoWKY Nov 6, 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!

PotatoWKY Nov 6, 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!

PotatoWKY Nov 6, 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!

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!

vpbhargav left a comment

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

Development

Uh oh!

5 participants

PotatoWKY Nov 6, 2025 •

edited

Loading

PotatoWKY Nov 6, 2025 •

edited

Loading

PotatoWKY Nov 6, 2025 •

edited

Loading