Skip to content

Commit 01db476

Browse files
committed
fix(smus): unble to refresh status on newly created App bug
1 parent cbdbfa8 commit 01db476

File tree

5 files changed

+98
-10
lines changed

5 files changed

+98
-10
lines changed

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,11 @@ import { getIcon, IconPath } from '../../shared/icons'
1111
import { generateSpaceStatus, updateIdleFile, startMonitoringTerminalActivity, ActivityCheckInterval } from './utils'
1212
import { UserActivity } from '../../shared/extensionUtilities'
1313
import { getLogger } from '../../shared/logger/logger'
14+
import { ToolkitError } from '../../shared/errors'
1415
import { SpaceStatus, RemoteAccess } from './constants'
1516

17+
const logger = getLogger('sagemaker')
18+
1619
export class SagemakerSpace {
1720
public label: string = ''
1821
public contextValue: string = ''
@@ -34,6 +37,10 @@ export class SagemakerSpace {
3437
}
3538

3639
public updateSpace(spaceApp: SagemakerSpaceApp) {
40+
// Edge case when this.spaceApp.App is null, returned by ListApp API for a Space that is not connected to for over 24 hours
41+
if (!this.spaceApp.App) {
42+
this.spaceApp.App = spaceApp.App
43+
}
3744
this.setSpaceStatus(spaceApp.Status ?? '', spaceApp.App?.Status ?? '')
3845
// Only update RemoteAccess property to minimize impact due to minor structural differences between variables
3946
if (this.spaceApp.SpaceSettingsSummary && spaceApp.SpaceSettingsSummary?.RemoteAccess) {
@@ -107,13 +114,18 @@ export class SagemakerSpace {
107114
DomainId: this.spaceApp.DomainId,
108115
SpaceName: this.spaceApp.SpaceName,
109116
})
110-
111-
const app = await this.client.describeApp({
112-
DomainId: this.spaceApp.DomainId,
113-
AppName: this.spaceApp.App?.AppName,
114-
AppType: this.spaceApp?.SpaceSettingsSummary?.AppType,
115-
SpaceName: this.spaceApp.SpaceName,
116-
})
117+
// get app using ListApps API, with given DomainId and SpaceName
118+
let app
119+
if (this.spaceApp.DomainId && this.spaceApp.SpaceName) {
120+
app = await this.client.listAppForSpace(this.spaceApp.DomainId, this.spaceApp.SpaceName)
121+
} else {
122+
logger.error(
123+
`updateSpaceAppStatus: unable to get app, [DomainId: ${this.spaceApp.DomainId}], [SpaceName: ${this.spaceApp.SpaceName}]`
124+
)
125+
throw new ToolkitError(
126+
`Cannot update app status without [DomainId: ${this.spaceApp.DomainId} and SpaceName: ${this.spaceApp.SpaceName}]`
127+
)
128+
}
117129

118130
// AWS DescribeSpace API returns full details with property names like 'SpaceSettings'
119131
// but our internal SagemakerSpaceApp type expects 'SpaceSettingsSummary' (from ListSpaces API)
@@ -195,7 +207,6 @@ export class SagemakerSpace {
195207
* Sets up user activity monitoring for SageMaker spaces
196208
*/
197209
export async function setupUserActivityMonitoring(extensionContext: vscode.ExtensionContext): Promise<void> {
198-
const logger = getLogger()
199210
logger.info('setupUserActivityMonitoring: Starting user activity monitoring setup')
200211

201212
const tmpDirectory = '/tmp/'

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,13 @@ export class SagemakerClient extends ClientWrapper<SageMakerClient> {
128128
return this.makeRequest(DeleteAppCommand, request)
129129
}
130130

131+
public async listAppForSpace(domainId: string, spaceName: string): Promise<AppDetails | undefined> {
132+
const appsList = await this.listApps({ DomainIdEquals: domainId, SpaceNameEquals: spaceName })
133+
.flatten()
134+
.promise()
135+
return appsList[0] // At most one App for one SagemakerSpace
136+
}
137+
131138
public async startSpace(spaceName: string, domainId: string, skipInstanceTypePrompts: boolean = false) {
132139
let spaceDetails: DescribeSpaceCommandOutput
133140

packages/core/src/test/awsService/sagemaker/explorer/sagemakerSpaceNode.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,13 @@ describe('SagemakerSpaceNode', function () {
112112
it('updates space app status', async function () {
113113
const describeSpaceStub = sinon.stub(SagemakerClient.prototype, 'describeSpace')
114114
describeSpaceStub.resolves({ SpaceName: 'TestSpace', Status: 'InService', $metadata: {} })
115-
describeAppStub.resolves({ AppName: 'TestApp', Status: 'InService', $metadata: {} })
115+
116+
const listAppForSpaceStub = sinon.stub(SagemakerClient.prototype, 'listAppForSpace')
117+
listAppForSpaceStub.resolves({ AppName: 'TestApp', Status: 'InService' })
116118

117119
await testSpaceAppNode.updateSpaceAppStatus()
118120

119121
sinon.assert.calledOnce(describeSpaceStub)
120-
sinon.assert.calledOnce(describeAppStub)
122+
sinon.assert.calledOnce(listAppForSpaceStub)
121123
})
122124
})

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ describe('SagemakerSpace', function () {
6363

6464
mockClient.describeSpace.resolves(mockDescribeSpaceResponse)
6565
mockClient.describeApp.resolves(mockDescribeAppResponse)
66+
mockClient.listAppForSpace.resolves(mockDescribeAppResponse)
6667

6768
const space = new SagemakerSpace(mockClient as any, 'us-east-1', mockSpaceApp)
6869
const updateSpaceSpy = sinon.spy(space, 'updateSpace')
@@ -125,5 +126,37 @@ describe('SagemakerSpace', function () {
125126
assert.strictEqual(updateSpaceArgs.OwnershipSettingsSummary, undefined)
126127
assert.strictEqual(updateSpaceArgs.SpaceSharingSettingsSummary, undefined)
127128
})
129+
130+
it('should use listAppForSpace when AppName is not available', async function () {
131+
const mockDescribeSpaceResponse = {
132+
SpaceName: 'test-space',
133+
Status: 'InService',
134+
DomainId: 'test-domain',
135+
$metadata: { requestId: 'test-request-id' },
136+
}
137+
138+
const mockAppFromList = {
139+
AppName: 'listed-app',
140+
Status: 'InService',
141+
$metadata: { requestId: 'test-request-id' },
142+
}
143+
144+
mockClient.describeSpace.resolves(mockDescribeSpaceResponse)
145+
mockClient.listAppForSpace.resolves(mockAppFromList)
146+
147+
// Create space without App.AppName
148+
const spaceWithoutAppName: SagemakerSpaceApp = {
149+
...mockSpaceApp,
150+
App: undefined,
151+
}
152+
153+
const space = new SagemakerSpace(mockClient as any, 'us-east-1', spaceWithoutAppName)
154+
await space.updateSpaceAppStatus()
155+
156+
// Verify listAppForSpace was called instead of describeApp
157+
assert.ok(mockClient.listAppForSpace.calledOnce)
158+
assert.ok(mockClient.listAppForSpace.calledWith('test-domain', 'test-space'))
159+
assert.ok(mockClient.describeApp.notCalled)
160+
})
128161
})
129162
})

packages/core/src/test/shared/clients/sagemakerClient.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,41 @@ describe('SagemakerClient.listSpaceApps', function () {
173173
})
174174
})
175175

176+
describe('SagemakerClient.listAppForSpace', function () {
177+
const region = 'test-region'
178+
let client: SagemakerClient
179+
let listAppsStub: sinon.SinonStub
180+
181+
beforeEach(function () {
182+
client = new SagemakerClient(region)
183+
listAppsStub = sinon.stub(client, 'listApps')
184+
})
185+
186+
afterEach(function () {
187+
sinon.restore()
188+
})
189+
190+
it('returns first app for given domain and space', async function () {
191+
const appDetails: AppDetails[] = [
192+
{ AppName: 'app1', DomainId: 'domain1', SpaceName: 'space1', AppType: AppType.CodeEditor },
193+
]
194+
listAppsStub.returns(intoCollection([appDetails]))
195+
196+
const result = await client.listAppForSpace('domain1', 'space1')
197+
198+
assert.strictEqual(result?.AppName, 'app1')
199+
sinon.assert.calledWith(listAppsStub, { DomainIdEquals: 'domain1', SpaceNameEquals: 'space1' })
200+
})
201+
202+
it('returns undefined when no apps found', async function () {
203+
listAppsStub.returns(intoCollection([[]]))
204+
205+
const result = await client.listAppForSpace('domain1', 'space1')
206+
207+
assert.strictEqual(result, undefined)
208+
})
209+
})
210+
176211
describe('SagemakerClient.waitForAppInService', function () {
177212
const region = 'test-region'
178213
let client: SagemakerClient

0 commit comments

Comments
 (0)