Skip to content

Commit f27a635

Browse files
authored
test(auth): flaky credentials cache test (#5979)
## Problem - ran 1000 times in CI, only failed on Windows about 30-40% of the time. - appears to be a windows `fs` race condition. We delete the credentials file, then create a new one. If the order of these operations is not preserved, the test can fail. - Adding `waitUntil` within the `fs` operations, such that each operation must succeed before returning, does not fix the problem. - The credentials file IS updating in time since if we read it right after generating and before assertion, it is the correct data. This then implies that the credentials are in fact caching despite the changes to the credentials file. - applying `waitUntil` with the condition being that the contents of the file changed, also did not work since the file content is updating. - adding certain log statements causes it not to be flaky. - While the contents of the file are updating, the stats of the file are not. This is important, because we use the last modified date to determine if credentials should refresh in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/auth/providers/sharedCredentialsProviderFactory.ts#L37-L45. - **On windows, it appears the `fs.stat` is unreliable, as we can fully rewrite the file and its `mtime` and `ctime` fail to update (at least 10+ seconds, potentially never)** - We can't rely on `fs.stat` on windows machine to determine if credentials need a refresh. We either need a new mechanism or don't cache on windows. ## Solution - Avoid `fs.stat` altogether and refresh by reading the file each time. - Added benefit: removes state for tracking modified time, and simplifies the credentials code. - Makes a single `fs` call instead of two (two `fs.stat` versus one `fs.read`). --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent c75e210 commit f27a635

File tree

2 files changed

+3
-42
lines changed

2 files changed

+3
-42
lines changed

packages/core/src/auth/providers/sharedCredentialsProviderFactory.ts

Lines changed: 1 addition & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3,47 +3,23 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
import fs from '../../shared/fs/fs'
76
import { getLogger, Logger } from '../../shared/logger'
87
import { loadSharedCredentialsSections, updateAwsSdkLoadConfigEnvVar } from '../credentials/sharedCredentials'
98
import { CredentialsProviderType } from './credentials'
109
import { BaseCredentialsProviderFactory } from './credentialsProviderFactory'
1110
import { SharedCredentialsProvider } from './sharedCredentialsProvider'
12-
import { getCredentialsFilename, getConfigFilename } from '../credentials/sharedCredentialsFile'
1311

1412
export class SharedCredentialsProviderFactory extends BaseCredentialsProviderFactory<SharedCredentialsProvider> {
1513
private readonly logger: Logger = getLogger()
1614

17-
private loadedCredentialsModificationMillis?: number
18-
private loadedConfigModificationMillis?: number
19-
2015
public async refresh(): Promise<void> {
21-
if (await this.needsRefresh()) {
22-
await this.loadSharedCredentialsProviders()
23-
}
16+
await this.loadSharedCredentialsProviders()
2417
}
2518

2619
public override getProviderType(): CredentialsProviderType | undefined {
2720
return SharedCredentialsProvider.getProviderType()
2821
}
2922

30-
protected override resetProviders() {
31-
this.loadedCredentialsModificationMillis = undefined
32-
this.loadedConfigModificationMillis = undefined
33-
34-
super.resetProviders()
35-
}
36-
37-
private async needsRefresh(): Promise<boolean> {
38-
const credentialsLastModMillis = await this.getLastModifiedMillis(getCredentialsFilename())
39-
const configLastModMillis = await this.getLastModifiedMillis(getConfigFilename())
40-
41-
return (
42-
this.loadedCredentialsModificationMillis !== credentialsLastModMillis ||
43-
this.loadedConfigModificationMillis !== configLastModMillis
44-
)
45-
}
46-
4723
private async loadSharedCredentialsProviders(): Promise<void> {
4824
this.resetProviders()
4925

@@ -52,9 +28,6 @@ export class SharedCredentialsProviderFactory extends BaseCredentialsProviderFac
5228
const errors = result.errors.map((e) => e.message).join('\t\n')
5329
getLogger().warn(`credentials: errors while parsing:\n%s`, errors)
5430
}
55-
56-
this.loadedCredentialsModificationMillis = await this.getLastModifiedMillis(getCredentialsFilename())
57-
this.loadedConfigModificationMillis = await this.getLastModifiedMillis(getConfigFilename())
5831
await updateAwsSdkLoadConfigEnvVar()
5932

6033
getLogger().verbose(
@@ -79,13 +52,4 @@ export class SharedCredentialsProviderFactory extends BaseCredentialsProviderFac
7952
this.addProvider(provider)
8053
}
8154
}
82-
83-
private async getLastModifiedMillis(filepath: string): Promise<number | undefined> {
84-
try {
85-
const stat = await fs.stat(filepath)
86-
return stat.mtime
87-
} catch (err) {
88-
return undefined
89-
}
90-
}
9155
}

packages/core/src/test/credentials/provider/sharedCredentialsProviderFactory.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ describe('SharedCredentialsProviderFactory', async function () {
120120
)
121121
})
122122

123-
it('refresh does not reload from file if the file has not changed', async function () {
123+
it('refresh always reloads from file', async function () {
124124
const sut = new SharedCredentialsProviderFactory()
125125

126126
// First load
@@ -129,10 +129,7 @@ describe('SharedCredentialsProviderFactory', async function () {
129129
// Expect: No reload
130130
await sut.refresh()
131131

132-
assert.ok(
133-
loadSharedCredentialsSectionsStub.calledOnce,
134-
'Credentials should have only been loaded from disk once'
135-
)
132+
assert.ok(loadSharedCredentialsSectionsStub.calledTwice, 'Credentials should have loaded from disk twice')
136133
})
137134

138135
it('refresh reloads from file if the file has changed', async function () {

0 commit comments

Comments
 (0)