Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/amazonq/src/extensionNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ async function getAuthState(): Promise<Omit<AuthUserState, 'source'>> {
try {
// May call connection validate functions that try to refresh the token.
// This could result in network errors.
authState = (await AuthUtil.instance.getChatAuthState(false)).codewhispererChat
authState = (await AuthUtil.instance._getChatAuthState(false)).codewhispererChat
} catch (err) {
if (
isNetworkError(err) &&
Expand Down
15 changes: 1 addition & 14 deletions packages/core/src/auth/sso/ssoAccessTokenProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { randomUUID } from '../../shared/crypto'
import { getExtRuntimeContext } from '../../shared/vscode/env'
import { showInputBox } from '../../shared/ui/inputPrompter'
import { AmazonQPromptSettings, DevSettings, PromptSettings, ToolkitPromptSettings } from '../../shared/settings'
import { debounce, onceChanged } from '../../shared/utilities/functionUtils'
import { onceChanged } from '../../shared/utilities/functionUtils'
import { NestedMap } from '../../shared/utilities/map'
import { asStringifiedStack } from '../../shared/telemetry/spans'
import { showViewLogsMessage } from '../../shared/utilities/messages'
Expand Down Expand Up @@ -97,20 +97,7 @@ export abstract class SsoAccessTokenProvider {
this.reAuthState.set(this.profile, { reAuthReason: `invalidate():${reason}` })
}

/**
* Sometimes we get many calls at once and this
* can trigger redundant disk reads, or token refreshes.
* We debounce to avoid this.
*
* NOTE: The property {@link getTokenDebounced()} does not work with being stubbed for tests, so
* this redundant function was created to work around that.
*/
public async getToken(): Promise<SsoToken | undefined> {
return this.getTokenDebounced()
}
private getTokenDebounced = debounce(() => this._getToken(), 50)
/** Exposed for testing purposes only */
public async _getToken(): Promise<SsoToken | undefined> {
const data = await this.cache.token.load(this.tokenCacheKey)
SsoAccessTokenProvider.logIfChanged(
indent(
Expand Down
17 changes: 16 additions & 1 deletion packages/core/src/codewhisperer/util/authUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import { telemetry } from '../../shared/telemetry/telemetry'
import { asStringifiedStack } from '../../shared/telemetry/spans'
import { withTelemetryContext } from '../../shared/telemetry/util'
import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands'
import { throttle } from 'lodash'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have a backlog item to remove dependency on lodash for performance reasons. The work was started here: #5157

Copy link
Contributor Author

@nkomonen-amazon nkomonen-amazon Feb 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussions w/ our move to the LSP work we will probably be able to drop this code, getting rid of the dependency. Also there is currently no existing throttle implementation. We will either need to build our own or add a new dependency. Ticket for removing lodash is here: https://taskei.amazon.dev/tasks/IDE-10293

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW it looks like throttle just uses debounce under the hood: https://github.com/lodash/lodash/blob/npm/throttle.js


/** Backwards compatibility for connections w pre-chat scopes */
export const codeWhispererCoreScopes = [...scopesCodeWhispererCore]
Expand Down Expand Up @@ -402,9 +403,23 @@ export class AuthUtil {
*
* By default, network errors are ignored when determining auth state since they may be silently
* recoverable later.
*
* THROTTLE: This function is called in rapid succession by Amazon Q features and can lead to
* a barrage of disk access and/or token refreshes. We throttle to deal with this.
*
* Note we do an explicit cast of the return type due to Lodash types incorrectly indicating
* a FeatureAuthState or undefined can be returned. But since we set `leading: true`
* it will always return FeatureAuthState
*/
public getChatAuthState = throttle(() => this._getChatAuthState(), 2000, {
leading: true,
}) as () => Promise<FeatureAuthState>
/**
* IMPORTANT: Only use this if you do NOT want to swallow network errors, otherwise use {@link getChatAuthState()}
* @param ignoreNetErr swallows network errors
*/
@withTelemetryContext({ name: 'getChatAuthState', class: authClassName })
public async getChatAuthState(ignoreNetErr: boolean = true): Promise<FeatureAuthState> {
public async _getChatAuthState(ignoreNetErr: boolean = true): Promise<FeatureAuthState> {
// The state of the connection may not have been properly validated
// and the current state we see may be stale, so refresh for latest state.
if (ignoreNetErr) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,20 +156,6 @@ describe('SsoAccessTokenProvider', function () {
assert.strictEqual(cachedToken, undefined)
})

it('concurrent calls are debounced', async function () {
const validToken = createToken(hourInMs)
await cache.token.save(startUrl, { region, startUrl, token: validToken })
const actualGetToken = sinon.spy(sut, '_getToken')

const result = await Promise.all([sut.getToken(), sut.getToken(), sut.getToken()])

// Subsequent other calls were debounced so this was only called once
assert.strictEqual(actualGetToken.callCount, 1)
for (const r of result) {
assert.deepStrictEqual(r, validToken)
}
})

describe('Exceptions', function () {
it('drops expired tokens if failure was a client-fault', async function () {
const exception = new UnauthorizedClientException({ message: '', $metadata: {} })
Expand Down
Loading