Skip to content

Fix flaky auth tests#92

Merged
nathanjcochran merged 2 commits into
mainfrom
nathan/fix-flaky-auth-tests
Oct 28, 2025
Merged

Fix flaky auth tests#92
nathanjcochran merged 2 commits into
mainfrom
nathan/fix-flaky-auth-tests

Conversation

@nathanjcochran
Copy link
Copy Markdown
Member

@nathanjcochran nathanjcochran commented Oct 28, 2025

This fixes some flaky auth tests that were sometimes in failing in CI (for example, see here).

The issue manifested as an unexpected EOF from the local OAuth callback server, which was causing the tests to fail here when simulating a request from the user's browser:

--- FAIL: TestAuthLogin_OAuth_SingleProject (0.16s)
    auth_test.go:366: Mock browser opening URL: http://127.0.0.1:42739/oauth/authorize?client_id=45e1b16d-e435-4049-97b2-8daad150818c&code_challenge=QtzkvFiFpRa2QZlWnzHuvtn_Ad3qP3hCDl8wuKoXMkY&code_challenge_method=S256&redirect_uri=http%3A%2F%2Flocalhost%3A35441%2Fcallback&response_type=code&state=UsgJbg3qKUKKMhKgRKM1zY5wHZuv5Onp
    auth_test.go:419: Mock browser making callback request to: http://localhost:35441/callback?code=test-auth-code&state=UsgJbg3qKUKKMhKgRKM1zY5wHZuv5Onp
    auth_test.go:261: Mock server received token exchange request
    auth_test.go:423: Mock callback request failed: Get "http://localhost:35441/callback?code=test-auth-code&state=UsgJbg3qKUKKMhKgRKM1zY5wHZuv5Onp": EOF

I believe it was happening because we were previously doing a non-graceful shutdown of the local HTTP server that handles the callback request. The shutdown was happening after we had already returned a redirect and returned the result on the resultChan, so the code looked valid. However, because the http.ResponseWriter buffers data before flushing it to the underlying TCP connection, and we were doing a hard/non-graceful shutdown, the TCP connection was sometimes being closed before the redirect response had actually been flushed, resulting in an EOF for the caller. I heard no reports of this happening outside of CI, but it's possible it could have happened in the wild too.

This fixes it by doing a graceful shutdown (which waits for any in-progress requests to complete) instead of a hard shutdown.

@nathanjcochran nathanjcochran self-assigned this Oct 28, 2025
Comment on lines -413 to -415
// Sleep to ensure the OAuth callback server is listening
// This prevents "EOF" errors in CI when the server hasn't started yet
time.Sleep(100 * time.Millisecond)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was just a confused attempt to fix the issue previously, but isn't actually needed. Because we open the net.Listener synchronously here, before we open the browser here, it should never be the case that the server isn't ready to handle incoming connections by the time this function executes.

Comment on lines -171 to -183
func (s *oauthServer) Close() error {
cls := func(closer io.Closer) error {
if err := closer.Close(); err != nil && !errors.Is(err, net.ErrClosed) {
return err
}
return nil
}

return errors.Join(
cls(s.server),
cls(s.listener),
)
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I decided to get rid of this function entirely and just call http.Server.Shutdown() directly above. The shutdown method already closes the listener internally, so we shouldn't need to do it ourselves, like it was previously being done here.

@nathanjcochran nathanjcochran merged commit 6e0d726 into main Oct 28, 2025
2 checks passed
@nathanjcochran nathanjcochran deleted the nathan/fix-flaky-auth-tests branch October 28, 2025 21:32
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