From 51666984555f2aaf7080ca5f4f9ae8e05f51a74d Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Tue, 17 Aug 2021 15:20:59 -0300 Subject: [PATCH] 18004: Fixes a couple of race condition bugs related to caching remote users. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- lib/controller/integration_test.go | 6 ++-- .../app/models/api_client_authorization.rb | 36 +++++++++++++++---- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go index 8a12f1d2fa..6851442054 100644 --- a/lib/controller/integration_test.go +++ b/lib/controller/integration_test.go @@ -189,7 +189,7 @@ func (s *IntegrationSuite) TestGetCollectionByPDH(c *check.C) { } // Tests bug #18004 -func (s *IntegrationSuite) TestRemoteTokenCacheRace(c *check.C) { +func (s *IntegrationSuite) TestRemoteUserAndTokenCacheRace(c *check.C) { conn1 := s.testClusters["z1111"].Conn() rootctx1, _, _ := s.testClusters["z1111"].RootClients() rootctx2, _, _ := s.testClusters["z2222"].RootClients() @@ -208,7 +208,7 @@ func (s *IntegrationSuite) TestRemoteTokenCacheRace(c *check.C) { defer wg2.Done() wg1.Wait() _, err := conn2.UserGetCurrent(rootctx2, arvados.GetOptions{}) - c.Check(err, check.IsNil) + c.Check(err, check.IsNil, check.Commentf("warm up phase failed")) }() } wg1.Done() @@ -224,7 +224,7 @@ func (s *IntegrationSuite) TestRemoteTokenCacheRace(c *check.C) { wg1.Wait() // Retrieve the remote collection from cluster z2222. _, err := conn2.UserGetCurrent(userctx1, arvados.GetOptions{}) - c.Check(err, check.IsNil) + c.Check(err, check.IsNil, check.Commentf("testing phase failed")) }() } wg1.Done() diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb index 52f2cee064..7c7ed759c6 100644 --- a/services/api/app/models/api_client_authorization.rb +++ b/services/api/app/models/api_client_authorization.rb @@ -319,7 +319,17 @@ class ApiClientAuthorization < ArvadosModel user.last_name = "from cluster #{remote_user_prefix}" end - user.save! + begin + user.save! + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique + Rails.logger.debug("remote user #{remote_user['uuid']} already exists, retrying...") + # Some other request won the race: retry fetching the user record. + user = User.find_by_uuid(remote_user['uuid']) + if !user + Rails.logger.warn("cannot find or create remote user #{remote_user['uuid']}") + return nil + end + end if user.is_invited && !remote_user['is_invited'] # Remote user is not "invited" state, they should be unsetup, which @@ -364,12 +374,24 @@ class ApiClientAuthorization < ArvadosModel exp = [db_current_time + Rails.configuration.Login.RemoteTokenRefresh, remote_token.andand['expires_at']].compact.min scopes = remote_token.andand['scopes'] || ['all'] - auth = ApiClientAuthorization.find_or_create_by(uuid: token_uuid) do |auth| - auth.user = user - auth.api_token = stored_secret - auth.api_client_id = 0 - auth.scopes = scopes - auth.expires_at = exp + begin + retries ||= 0 + auth = ApiClientAuthorization.find_or_create_by(uuid: token_uuid) do |auth| + auth.user = user + auth.api_token = stored_secret + auth.api_client_id = 0 + auth.scopes = scopes + auth.expires_at = exp + end + rescue ActiveRecord::RecordNotUnique + Rails.logger.debug("cached remote token #{token_uuid} already exists, retrying...") + # Some other request won the race: retry just once before erroring out + if (retries += 1) <= 1 + retry + else + Rails.logger.warn("cannot find or create cached remote token #{token_uuid}") + return nil + end end auth.update_attributes!(user: user, api_token: stored_secret, -- 2.30.2