Skip to content
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

test(auth): flaky credentials cache test #5979

Merged
merged 44 commits into from
Nov 15, 2024

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Nov 12, 2024

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).

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@Hweinstock Hweinstock changed the title test(auth): flaky credentials cache test (WIP IGNORE) test(auth): flaky credentials cache test Nov 15, 2024
@Hweinstock Hweinstock marked this pull request as ready for review November 15, 2024 19:53
@Hweinstock Hweinstock requested a review from a team as a code owner November 15, 2024 19:53

return (
this.loadedCredentialsModificationMillis !== credentialsLastModMillis ||
this.loadedConfigModificationMillis !== configLastModMillis
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't remember if this was discussed already: maybe the delete/modification in the test happened too quickly, so the mtime didn't actually change on the re-created file?

Copy link
Contributor Author

@Hweinstock Hweinstock Nov 15, 2024

Choose a reason for hiding this comment

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

Thats what I suspect as well. When I added a sleep in-between the two, the test appeared to pass.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

The simplification is good in any case.

@Hweinstock Hweinstock merged commit f27a635 into aws:master Nov 15, 2024
26 of 36 checks passed
@Hweinstock Hweinstock deleted the flakyAuthTest branch November 15, 2024 20:55
@justinmk3
Copy link
Contributor

Does this close an open issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants