Skip to content

Commit 8208336

Browse files
committed
fix(mothership): cancel desktop tools a signed-out turn starts late, guard the shown-once license key
- A turn's stream now binds to the session it started in. Its tool events can arrive after sign-out stops every desktop tool, and each one then gets an already-aborted lease instead of a fresh controller. A turn started after sign-in runs normally. - The generated license key counts as an unsaved change, so leaving the Licenses tab asks first, and confirming drops it.
1 parent 14e6c0a commit 8208336

6 files changed

Lines changed: 205 additions & 30 deletions

File tree

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
import { describe, expect, it } from 'vitest'
22
import {
3-
leaseDesktopTool,
3+
desktopToolTurn,
44
stopAllDesktopTools,
55
stopDesktopTools,
66
} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
77

88
describe('desktop tool leases', () => {
99
it('cancels every running tool of the stopped turn and no other turn', () => {
10-
const first = leaseDesktopTool('turn-a')
11-
const second = leaseDesktopTool('turn-a')
12-
const other = leaseDesktopTool('turn-b')
10+
const first = desktopToolTurn('turn-a').lease()
11+
const second = desktopToolTurn('turn-a').lease()
12+
const other = desktopToolTurn('turn-b').lease()
1313

1414
stopDesktopTools('turn-a', 'user_stop')
1515

@@ -21,9 +21,9 @@ describe('desktop tool leases', () => {
2121
})
2222

2323
it('keeps a turn reachable by Stop while any of its tools still runs', () => {
24-
const settled = leaseDesktopTool('turn-c')
25-
const running = leaseDesktopTool('turn-c')
26-
for (let turn = 0; turn < 500; turn++) leaseDesktopTool(`busy-${turn}`).release()
24+
const settled = desktopToolTurn('turn-c').lease()
25+
const running = desktopToolTurn('turn-c').lease()
26+
for (let turn = 0; turn < 500; turn++) desktopToolTurn(`busy-${turn}`).lease().release()
2727

2828
settled.release()
2929
settled.release()
@@ -33,11 +33,11 @@ describe('desktop tool leases', () => {
3333
})
3434

3535
it('gives a turn whose tools all settled a fresh lifetime for its next tool', () => {
36-
const settled = leaseDesktopTool('turn-d')
36+
const settled = desktopToolTurn('turn-d').lease()
3737
settled.release()
3838
stopDesktopTools('turn-d', 'user_stop')
3939

40-
const next = leaseDesktopTool('turn-d')
40+
const next = desktopToolTurn('turn-d').lease()
4141

4242
expect(settled.signal.aborted).toBe(false)
4343
expect(next.signal).not.toBe(settled.signal)
@@ -46,9 +46,9 @@ describe('desktop tool leases', () => {
4646
})
4747

4848
it('does not let a tool that settles after Stop release a newer lease on the turn', () => {
49-
const stopped = leaseDesktopTool('turn-e')
49+
const stopped = desktopToolTurn('turn-e').lease()
5050
stopDesktopTools('turn-e', 'user_stop')
51-
const next = leaseDesktopTool('turn-e')
51+
const next = desktopToolTurn('turn-e').lease()
5252

5353
stopped.release()
5454
stopDesktopTools('turn-e', 'user_stop')
@@ -57,17 +57,35 @@ describe('desktop tool leases', () => {
5757
})
5858

5959
it('cancels the running tools of every turn when the session ends', () => {
60-
const first = leaseDesktopTool('turn-f')
61-
const second = leaseDesktopTool('turn-g')
60+
const first = desktopToolTurn('turn-f').lease()
61+
const second = desktopToolTurn('turn-g').lease()
6262

6363
stopAllDesktopTools('signed_out')
64-
const next = leaseDesktopTool('turn-f')
6564

6665
expect(first.signal.aborted).toBe(true)
6766
expect(second.signal.aborted).toBe(true)
6867
expect(first.signal.reason).toBe('signed_out')
69-
expect(next.signal.aborted).toBe(false)
7068
first.release()
69+
second.release()
70+
})
71+
72+
it('cancels a tool a turn of the ended session starts after sign-out', () => {
73+
const turn = desktopToolTurn('turn-h')
74+
stopAllDesktopTools('signed_out')
75+
76+
const late = turn.lease()
77+
78+
expect(late.signal.aborted).toBe(true)
79+
expect(late.signal.reason).toBe('signed_out')
80+
late.release()
81+
})
82+
83+
it('runs the tools of a turn started after the session ended', () => {
84+
stopAllDesktopTools('signed_out')
85+
86+
const next = desktopToolTurn('turn-i').lease()
87+
88+
expect(next.signal.aborted).toBe(false)
7189
next.release()
7290
})
7391
})

‎apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ interface RunningTurnTools {
1515
*/
1616
const runningTurns = new Map<string, RunningTurnTools>()
1717

18+
/** Aborted by `stopAllDesktopTools`, then replaced, so each signed-in session has its own. */
19+
let session = new AbortController()
20+
1821
/** A running desktop tool's hold on its turn. */
1922
interface DesktopToolLease {
2023
/** Aborted only by the user's Stop of the turn, or by signing out. */
@@ -23,13 +26,31 @@ interface DesktopToolLease {
2326
release(): void
2427
}
2528

29+
/** A turn's stream, bound to the session it started in. */
30+
export interface DesktopToolTurn {
31+
/** Starts one desktop tool for the turn. */
32+
lease(): DesktopToolLease
33+
}
34+
35+
/**
36+
* Binds a turn's stream to the current session. Take it once, when the stream starts: its tool
37+
* events can still arrive after a sign-out, and each of them then gets an already-aborted lease.
38+
*/
39+
export function desktopToolTurn(streamId: string): DesktopToolTurn {
40+
const startedIn = session.signal
41+
return {
42+
lease: () =>
43+
startedIn.aborted ? { signal: startedIn, release() {} } : leaseDesktopTool(streamId),
44+
}
45+
}
46+
2647
/**
2748
* Starts a desktop tool (a browser action, a local file read or import) for a turn. Only the
2849
* user's Stop of that turn, or signing out (`stopAllDesktopTools`), cancels it: replacing the
2950
* stream reader, leaving the chat view, or stopping another chat's turn leaves it running to
3051
* finish and report its own result.
3152
*/
32-
export function leaseDesktopTool(streamId: string): DesktopToolLease {
53+
function leaseDesktopTool(streamId: string): DesktopToolLease {
3354
let turn = runningTurns.get(streamId)
3455
if (!turn) {
3556
turn = { stop: new AbortController(), running: 0 }
@@ -57,9 +78,12 @@ export function stopDesktopTools(streamId: string, reason: string): void {
5778

5879
/**
5980
* Cancels every leased desktop tool running in this tab (browser actions, local file reads and
60-
* imports), so none outlives the session that started it.
81+
* imports), and every one a turn of this session starts later, so none outlives the session
82+
* that started it.
6183
*/
6284
export function stopAllDesktopTools(reason: string): void {
85+
session.abort(reason)
86+
session = new AbortController()
6387
for (const turn of runningTurns.values()) turn.stop.abort(reason)
6488
runningTurns.clear()
6589
}

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ import { initTerminalTransport } from '@/lib/terminal/transport'
9393
import { getQueryClient } from '@/app/_shell/providers/get-query-client'
9494
import { chatUrl } from '@/app/workspace/[workspaceId]/home/hooks/chat-url'
9595
import {
96-
leaseDesktopTool,
96+
type DesktopToolTurn,
97+
desktopToolTurn,
9798
stopDesktopTools,
9899
} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
99100
import { useFilePreviewController } from '@/app/workspace/[workspaceId]/home/hooks/preview'
@@ -518,10 +519,10 @@ function startClientBrowserTool(
518519
toolArgs: Record<string, unknown>,
519520
scopeId: string,
520521
eventTs?: string,
521-
turnStreamId?: string
522+
desktopTurn?: DesktopToolTurn
522523
): void {
523524
if (!isCurrentBrowserToolName(toolName)) return
524-
const lease = turnStreamId ? leaseDesktopTool(turnStreamId) : undefined
525+
const lease = desktopTurn?.lease()
525526
void executeBrowserToolOnClient(
526527
toolCallId,
527528
toolName,
@@ -1654,7 +1655,7 @@ export function useChat(
16541655
toolCallId: string,
16551656
toolName: string,
16561657
toolArgs: Record<string, unknown>,
1657-
turnStreamId: string | undefined
1658+
desktopTurn: DesktopToolTurn | undefined
16581659
) => {
16591660
if (
16601661
!isNativeFileTool(toolName) &&
@@ -1666,7 +1667,7 @@ export function useChat(
16661667
return
16671668
}
16681669
handledClientLocalFilesystemToolIds.add(toolCallId)
1669-
const lease = turnStreamId ? leaseDesktopTool(turnStreamId) : undefined
1670+
const lease = desktopTurn?.lease()
16701671
const options = {
16711672
workspaceId,
16721673
chatId: chatIdRef.current ?? selectedChatIdRef.current,
@@ -2297,7 +2298,7 @@ export function useChat(
22972298
shouldContinue?: () => boolean
22982299
}
22992300
) => {
2300-
const turnStreamId = streamIdRef.current
2301+
const desktopTurn = streamIdRef.current ? desktopToolTurn(streamIdRef.current) : undefined
23012302
const activityTracker = getResourceActivityTracker(
23022303
expectedGen ?? streamGenRef.current,
23032304
options?.targetChatId
@@ -2318,7 +2319,7 @@ export function useChat(
23182319
eventTs?: string
23192320
) => {
23202321
const scopeId = activityScopeId()
2321-
startClientBrowserTool(toolCallId, toolName, toolArgs, scopeId, eventTs, turnStreamId)
2322+
startClientBrowserTool(toolCallId, toolName, toolArgs, scopeId, eventTs, desktopTurn)
23222323
}
23232324
const startClientTerminalToolForStream = (
23242325
toolCallId: string,
@@ -2351,7 +2352,7 @@ export function useChat(
23512352
removeResource,
23522353
startClientWorkflowTool,
23532354
startClientLocalFilesystemTool: (toolCallId, toolName, toolArgs) =>
2354-
startClientLocalFilesystemTool(toolCallId, toolName, toolArgs, turnStreamId),
2355+
startClientLocalFilesystemTool(toolCallId, toolName, toolArgs, desktopTurn),
23552356
startClientBrowserTool: startClientBrowserToolForStream,
23562357
startClientTerminalTool: startClientTerminalToolForStream,
23572358
startBrowserAgentRun: startBrowserAgentRunForStream,
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act, type ChangeEventHandler, type ReactNode } from 'react'
5+
import { emcnMock } from '@sim/testing/mocks/emcn.mock'
6+
import { NuqsTestingAdapter } from 'nuqs/adapters/testing'
7+
import { createRoot, type Root } from 'react-dom/client'
8+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const { mockGenerate } = vi.hoisted(() => ({ mockGenerate: vi.fn() }))
11+
12+
vi.mock('@sim/emcn', () => ({
13+
...emcnMock,
14+
Badge: ({ children }: { children?: ReactNode }) => <span>{children}</span>,
15+
Button: ({ children, ...props }: { children?: ReactNode }) => (
16+
<button type='button' {...props}>
17+
{children}
18+
</button>
19+
),
20+
ChipCopyInput: ({ value }: { value?: string }) => (
21+
<input data-testid='license-key' readOnly value={value ?? ''} />
22+
),
23+
ChipInput: ({
24+
value,
25+
onChange,
26+
placeholder,
27+
}: {
28+
value?: string
29+
onChange?: ChangeEventHandler<HTMLInputElement>
30+
placeholder?: string
31+
}) => <input placeholder={placeholder} value={value ?? ''} onChange={onChange} />,
32+
ChipModalTabs: () => <div />,
33+
ChipSelect: () => <div />,
34+
Label: ({ children }: { children?: ReactNode }) => <span>{children}</span>,
35+
Skeleton: () => <div />,
36+
}))
37+
38+
vi.mock('@/app/workspace/[workspaceId]/settings/components/settings-panel', () => ({
39+
SettingsPanel: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
40+
}))
41+
42+
vi.mock('@/app/workspace/[workspaceId]/settings/components/settings-empty-state', () => ({
43+
SettingsEmptyState: () => null,
44+
}))
45+
46+
vi.mock('@/hooks/queries/mothership-admin', () => ({
47+
useGenerateLicense: () => ({ mutate: mockGenerate, isPending: false, error: null }),
48+
useMothershipLicenses: () => ({ data: undefined, isLoading: false }),
49+
useMothershipRequests: () => ({ data: undefined, isLoading: false }),
50+
useMothershipUserBreakdown: () => ({ data: undefined, isLoading: false }),
51+
}))
52+
53+
import { Mothership } from '@/app/workspace/[workspaceId]/settings/components/mothership/mothership'
54+
import { useSettingsDirtyStore } from '@/stores/settings/dirty/store'
55+
56+
let container: HTMLDivElement
57+
let root: Root
58+
59+
beforeEach(() => {
60+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
61+
useSettingsDirtyStore.getState().reset()
62+
mockGenerate.mockImplementation(
63+
(_input: unknown, options: { onSuccess: (result: { license_key: string }) => void }) =>
64+
options.onSuccess({ license_key: 'sim_license_once' })
65+
)
66+
container = document.createElement('div')
67+
document.body.appendChild(container)
68+
root = createRoot(container)
69+
act(() =>
70+
root.render(
71+
<NuqsTestingAdapter searchParams='?tab=licenses'>
72+
<Mothership />
73+
</NuqsTestingAdapter>
74+
)
75+
)
76+
})
77+
78+
afterEach(() => {
79+
act(() => root.unmount())
80+
container.remove()
81+
})
82+
83+
function type(placeholder: string, value: string) {
84+
const input = container.querySelector<HTMLInputElement>(`input[placeholder="${placeholder}"]`)
85+
expect(input).not.toBeNull()
86+
act(() => {
87+
Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value')?.set?.call(
88+
input,
89+
value
90+
)
91+
input?.dispatchEvent(new Event('input', { bubbles: true }))
92+
})
93+
}
94+
95+
function generateKey() {
96+
type('e.g. Acme Corp', 'Acme')
97+
type('Signed order form or written approval', 'order-1')
98+
const generate = Array.from(container.querySelectorAll('button')).find(
99+
(button) => !button.disabled
100+
)
101+
act(() => generate?.click())
102+
}
103+
104+
function licenseKey() {
105+
return container.querySelector<HTMLInputElement>('[data-testid="license-key"]')?.value
106+
}
107+
108+
describe('Mothership license generation', () => {
109+
it('asks before leaving while the shown-once license key is on screen', () => {
110+
generateKey()
111+
const leave = vi.fn()
112+
113+
const left = useSettingsDirtyStore.getState().requestLeave(leave)
114+
115+
expect(licenseKey()).toBe('sim_license_once')
116+
expect(left).toBe(false)
117+
expect(leave).not.toHaveBeenCalled()
118+
})
119+
120+
it('drops the license key when the admin confirms leaving', () => {
121+
generateKey()
122+
const leave = vi.fn()
123+
useSettingsDirtyStore.getState().requestLeave(leave)
124+
125+
act(() => useSettingsDirtyStore.getState().confirmLeave())
126+
127+
expect(leave).toHaveBeenCalledOnce()
128+
expect(licenseKey()).toBeUndefined()
129+
expect(useSettingsDirtyStore.getState().isDirty).toBe(false)
130+
})
131+
})

‎apps/sim/app/workspace/[workspaceId]/settings/components/mothership/mothership.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,9 +365,10 @@ function LicensesTab({ environment }: { environment: MothershipEnv }) {
365365
setNewName('')
366366
setNewExpiry('')
367367
setApprovalReference('')
368+
setGeneratedKey(null)
368369
}, [])
369370
useSettingsUnsavedGuard({
370-
isDirty: Boolean(newName.trim() || newExpiry || approvalReference.trim()),
371+
isDirty: Boolean(generatedKey || newName.trim() || newExpiry || approvalReference.trim()),
371372
navigationBlocked: generateLicense.isPending,
372373
onDiscard: discardDraft,
373374
})
@@ -382,8 +383,8 @@ function LicensesTab({ environment }: { environment: MothershipEnv }) {
382383
},
383384
{
384385
onSuccess: (result) => {
385-
setGeneratedKey(result.license_key)
386386
discardDraft()
387+
setGeneratedKey(result.license_key)
387388
},
388389
}
389390
)

‎apps/sim/stores/index.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ vi.mock('@/stores/reset-all-stores', () => {
1313
return { resetAllStores: mockResetAllStores }
1414
})
1515

16-
import { leaseDesktopTool } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
16+
import { desktopToolTurn } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
1717
import { clearUserData, RECENT_IMPERSONATIONS_STORAGE_KEY } from '@/stores'
1818

1919
expect(mockModuleLoaded).not.toHaveBeenCalled()
@@ -98,8 +98,8 @@ describe('clearUserData', () => {
9898
})
9999

100100
it('cancels desktop tools still running for the signed-out identity', async () => {
101-
const localRead = leaseDesktopTool('turn-before-sign-out')
102-
const browserAction = leaseDesktopTool('other-turn-before-sign-out')
101+
const localRead = desktopToolTurn('turn-before-sign-out').lease()
102+
const browserAction = desktopToolTurn('other-turn-before-sign-out').lease()
103103
mockResetAllStores.mockImplementationOnce(() => {
104104
throw new Error('Chunk unavailable')
105105
})

0 commit comments

Comments
 (0)