Detect dev session takeover #6799
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,12 @@ export interface UserError { | |||||
| category: string | ||||||
| } | ||||||
|
|
||||||
| interface DevSessionState { | ||||||
| websocketUrl?: string | null | ||||||
| userId?: string | null | ||||||
| userEmail?: string | null | ||||||
| } | ||||||
|
|
||||||
| type DevSessionResult = | ||||||
| | {status: 'updated' | 'created' | 'aborted'} | ||||||
| | {status: 'remote-error'; error: UserError[]} | ||||||
|
|
@@ -42,6 +48,7 @@ export class DevSession { | |||||
| private readonly bundlePath: string | ||||||
| private readonly appEventsProcessor: SerialBatchProcessor<AppEvent> | ||||||
| private failedEvents: AppEvent[] = [] | ||||||
| private currentSessionState: DevSessionState | null = null | ||||||
|
|
||||||
| private constructor(processOptions: DevSessionProcessOptions, stdout: Writable) { | ||||||
| this.statusManager = processOptions.devSessionStatusManager | ||||||
|
|
@@ -401,6 +408,36 @@ export class DevSession { | |||||
| private async devSessionUpdateWithRetry(payload: DevSessionUpdateOptions): Promise<DevSessionResult> { | ||||||
| const result = await this.options.developerPlatformClient.devSessionUpdate(payload) | ||||||
| const errors = result.devSessionUpdate?.userErrors ?? [] | ||||||
| const devSession = result.devSessionUpdate?.devSession | ||||||
|
|
||||||
| // Check for session takeover | ||||||
| if (devSession && this.currentSessionState) { | ||||||
| const expectedWebsocketUrl = getWebSocketUrl(this.options.url) | ||||||
| const expectedUserId = this.currentSessionState?.userId | ||||||
|
|
||||||
| if (devSession.websocketUrl !== expectedWebsocketUrl || devSession.user?.id !== expectedUserId) { | ||||||
| // Another user has taken over the session | ||||||
| const newUserEmail = devSession.user?.email | ||||||
| await this.logger.error( | ||||||
| `Another developer ${newUserEmail ? `(${newUserEmail}) ` : ''}has taken ownership of this dev preview.`, | ||||||
| ) | ||||||
|
|
||||||
| // Throw AbortError to terminate gracefully | ||||||
| const message = `Another user${ | ||||||
| newUserEmail ? ` (${newUserEmail})` : '' | ||||||
| } has taken ownership of this dev preview. Your preview is no longer active.` | ||||||
|
Comment on lines
+426
to
+428
Contributor
There was a problem hiding this comment. Not a big deal, but the issue I mentioned here is not fixed. If the only change is the websocket URL, we are saying |
||||||
| const nextSteps = ['You can restart by running', {command: 'shopify app dev'}, 'again.'] | ||||||
| throw new AbortError(message, nextSteps) | ||||||
| } | ||||||
|
|
||||||
| // Update stored session state for next comparison | ||||||
| this.currentSessionState = { | ||||||
| websocketUrl: devSession.websocketUrl, | ||||||
| userId: devSession.user?.id ?? null, | ||||||
| userEmail: devSession.user?.email ?? null, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (errors.length) return {status: 'remote-error', error: errors} | ||||||
| return {status: 'updated'} | ||||||
| } | ||||||
|
|
@@ -414,7 +451,31 @@ export class DevSession { | |||||
| private async devSessionCreateWithRetry(payload: DevSessionCreateOptions): Promise<DevSessionResult> { | ||||||
| const result = await this.options.developerPlatformClient.devSessionCreate(payload) | ||||||
| const errors = result.devSessionCreate?.userErrors ?? [] | ||||||
| const warnings = result.devSessionCreate?.warnings ?? [] | ||||||
| const devSession = result.devSessionCreate?.devSession | ||||||
|
|
||||||
| // Store initial session state | ||||||
| if (devSession) { | ||||||
| this.currentSessionState = { | ||||||
| websocketUrl: devSession.websocketUrl, | ||||||
| userId: devSession.user?.id ?? null, | ||||||
| userEmail: devSession.user?.email ?? null, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Display warnings (non-blocking) | ||||||
| if (warnings.length > 0) { | ||||||
| await Promise.all( | ||||||
| warnings.map((warning) => { | ||||||
| const message = warning.code === 'SESSION_TAKEOVER' ? `⚠️ ${warning.message}` : warning.message | ||||||
|
Contributor
There was a problem hiding this comment. Why not just adding the emoji to all the warnings?
Suggested change
|
||||||
| return this.logger.warning(message) | ||||||
| }), | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| // Errors are blocking | ||||||
| if (errors.length) return {status: 'remote-error', error: errors} | ||||||
|
|
||||||
| return {status: 'created'} | ||||||
| } | ||||||
| } | ||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why showing the error twice? I'd get rid of this if we are going to abort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's nice to see something in the log, even if we end up saying the same thing in the final error message. I asked about copy for these messages here and while I didn't explicitly ask if both were warranted, there wasn't push back against having both.