-
Notifications
You must be signed in to change notification settings - Fork 233
Fix infinite 401 retry loop when app logs resubscribe fails #7055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||||||||
| import {renderJsonLogs} from './render-json-logs.js' | ||||||||||||
| import {pollAppLogs} from './poll-app-logs.js' | ||||||||||||
| import {handleFetchAppLogsError} from '../utils.js' | ||||||||||||
| import {handleFetchAppLogsError, MAX_CONSECUTIVE_RESUBSCRIBE_FAILURES} from '../utils.js' | ||||||||||||
| import {testDeveloperPlatformClient} from '../../../models/app/app.test-data.js' | ||||||||||||
| import {outputInfo, outputResult} from '@shopify/cli-kit/node/output' | ||||||||||||
| import {describe, expect, vi, test, beforeEach, afterEach} from 'vitest' | ||||||||||||
|
|
@@ -101,6 +101,67 @@ | |||||||||||
| expect(outputResult).not.toHaveBeenCalled() | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
| test('should retry at throttle interval when handleFetchAppLogsError returns null token', async () => { | ||||||||||||
| const mockErrorResponse = { | ||||||||||||
| errors: [{status: 401, message: 'Unauthorized'}], | ||||||||||||
| } | ||||||||||||
| const pollAppLogsMock = vi.fn().mockResolvedValue(mockErrorResponse) | ||||||||||||
| vi.mocked(pollAppLogs).mockImplementation(pollAppLogsMock) | ||||||||||||
| const throttleRetryInterval = 60000 | ||||||||||||
| const handleFetchAppLogsErrorMock = vi.fn(() => { | ||||||||||||
| return Promise.resolve({nextJwtToken: null, retryIntervalMs: throttleRetryInterval, resubscribeFailed: false}) | ||||||||||||
| }) | ||||||||||||
| vi.mocked(handleFetchAppLogsError).mockImplementation(handleFetchAppLogsErrorMock) | ||||||||||||
|
|
||||||||||||
| const storeNameById = new Map<string, string>() | ||||||||||||
| storeNameById.set('1', 'storeName') | ||||||||||||
| await renderJsonLogs({ | ||||||||||||
| pollOptions: {cursor: 'cursor', filters: {status: undefined, sources: undefined}, jwtToken: 'jwtToken'}, | ||||||||||||
| options: { | ||||||||||||
| variables: {shopIds: [], apiKey: ''}, | ||||||||||||
| developerPlatformClient: testDeveloperPlatformClient(), | ||||||||||||
| }, | ||||||||||||
| storeNameById, | ||||||||||||
| organizationId: 'organizationId', | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
| expect(handleFetchAppLogsError).toHaveBeenCalled() | ||||||||||||
| expect(pollAppLogs).toHaveBeenCalled() | ||||||||||||
| expect(setTimeout).toHaveBeenCalledWith(expect.any(Function), throttleRetryInterval) | ||||||||||||
|
Check failure on line 130 in packages/app/src/cli/services/app-logs/logs-command/render-json-logs.test.ts
|
||||||||||||
| expect(vi.getTimerCount()).toEqual(1) | ||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Improvement: The test sets Suggestion: Add an assertion on
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in fa2d200 — added |
||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
| test('should stop polling after MAX consecutive resubscribe failures', async () => { | ||||||||||||
| const mockErrorResponse = { | ||||||||||||
| errors: [{status: 401, message: 'Unauthorized'}], | ||||||||||||
| } | ||||||||||||
| const pollAppLogsMock = vi.fn().mockResolvedValue(mockErrorResponse) | ||||||||||||
| vi.mocked(pollAppLogs).mockImplementation(pollAppLogsMock) | ||||||||||||
| const handleFetchAppLogsErrorMock = vi.fn(() => { | ||||||||||||
| return Promise.resolve({nextJwtToken: null, retryIntervalMs: 60000, resubscribeFailed: true}) | ||||||||||||
| }) | ||||||||||||
| vi.mocked(handleFetchAppLogsError).mockImplementation(handleFetchAppLogsErrorMock) | ||||||||||||
|
|
||||||||||||
| const storeNameById = new Map<string, string>() | ||||||||||||
| storeNameById.set('1', 'storeName') | ||||||||||||
| await renderJsonLogs({ | ||||||||||||
| pollOptions: {cursor: 'cursor', filters: {status: undefined, sources: undefined}, jwtToken: 'jwtToken'}, | ||||||||||||
| options: { | ||||||||||||
| variables: {shopIds: [], apiKey: ''}, | ||||||||||||
| developerPlatformClient: testDeveloperPlatformClient(), | ||||||||||||
| }, | ||||||||||||
| storeNameById, | ||||||||||||
| organizationId: 'organizationId', | ||||||||||||
| consecutiveResubscribeFailures: MAX_CONSECUTIVE_RESUBSCRIBE_FAILURES - 1, | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
| expect(handleFetchAppLogsError).toHaveBeenCalled() | ||||||||||||
| expect(outputInfo).toHaveBeenCalledWith( | ||||||||||||
| JSON.stringify({message: 'App log streaming session has expired. Please restart your dev session.'}), | ||||||||||||
| ) | ||||||||||||
| expect(vi.getTimerCount()).toEqual(0) | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
| test('should handle error response and retry as expected', async () => { | ||||||||||||
| const mockErrorResponse = { | ||||||||||||
| errors: [{status: 500, message: 'Server Error'}], | ||||||||||||
|
|
@@ -110,9 +171,11 @@ | |||||||||||
| const mockRetryInterval = 1000 | ||||||||||||
| const handleFetchAppLogsErrorMock = vi.fn((input) => { | ||||||||||||
| input.onUnknownError(mockRetryInterval) | ||||||||||||
| return new Promise<{retryIntervalMs: number; nextJwtToken: string | null}>((resolve, _reject) => { | ||||||||||||
| resolve({nextJwtToken: 'new-jwt-token', retryIntervalMs: mockRetryInterval}) | ||||||||||||
| }) | ||||||||||||
| return new Promise<{retryIntervalMs: number; nextJwtToken: string | null; resubscribeFailed: boolean}>( | ||||||||||||
| (resolve, _reject) => { | ||||||||||||
| resolve({nextJwtToken: 'new-jwt-token', retryIntervalMs: mockRetryInterval, resubscribeFailed: false}) | ||||||||||||
| }, | ||||||||||||
| ) | ||||||||||||
| }) | ||||||||||||
| vi.mocked(handleFetchAppLogsError).mockImplementation(handleFetchAppLogsErrorMock) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
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.
🐛 Bug: This test's assertions —
failingResubscribewas called andvi.getTimerCount() === 1— are satisfied by two different code paths. Without the new try-catch inhandleFetchAppLogsError, the rejected promise fromonResubscribe()propagates up topollAppLogs's outer catch block (line 131 of poll-app-logs.ts), which also schedules a retry timer (at 5s instead of 60s). So removing the fix entirely would not cause this test to fail. Two missing assertions compound the problem: (1) no verification that the inner catch path was taken, and (2) no verification that the retry interval is 60s rather than 5s — which is the key behavioral change this fix introduces.Suggestion: Add assertions that pin down the exact code path and interval. For example:
The
outputDebugspy confirms the inner catch was entered (not the outer catch, which callsoutputWarn). ThesetTimeoutspy confirms the 60s interval, following the pattern inusePollAppLogs.test.tsxline 414.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.
Resolved in fa2d200 — added
outputDebugspy,outputWarnspy (verifying the inner catch triggersonThrottle→outputWarn, not the outer catch), andsetTimeoutspy asserting the 60s interval.