diff --git a/spec/graph_replication_retry_spec.cr b/spec/graph_replication_retry_spec.cr new file mode 100644 index 00000000..98f73888 --- /dev/null +++ b/spec/graph_replication_retry_spec.cr @@ -0,0 +1,152 @@ +require "./helper" + +module PlaceOS::Api + describe GraphReplicationRetry do + replication_lag_error = -> do + Office365::Exception.new( + HTTP::Status::NOT_FOUND, + {error: {code: "Request_ResourceNotFound", message: "Resource 'x' does not exist or one of its queried reference-property objects are not present."}}.to_json, + "Not Found" + ) + end + + app_not_backed_error = -> do + Office365::Exception.new( + HTTP::Status::BAD_REQUEST, + {error: {code: "Request_BadRequest", message: "The appId 'x' of the service principal does not reference a valid application object.", details: [{code: "NoBackingApplicationObject", message: "The appId 'x' of the service principal does not reference a valid application object.", target: "appId"}]}}.to_json, + "Bad Request" + ) + end + + directory_busy_error = -> do + Office365::Exception.new( + HTTP::Status::NOT_FOUND, + {error: {code: "Directory_ObjectNotFound", message: "Unable to read the company information from the directory."}}.to_json, + "Not Found" + ) + end + + stale_scope_error = -> do + Office365::Exception.new( + HTTP::Status::BAD_REQUEST, + {error: {code: "Request_BadRequest", message: "Property api.preAuthorizedApplications.delegatedPermissionIds has a Permission Id that cannot be found in the AppPermissions sets.", details: [{code: "InvalidValue", message: "Property api.preAuthorizedApplications.delegatedPermissionIds has a Permission Id that cannot be found in the AppPermissions sets.", target: "api.preAuthorizedApplications.delegatedPermissionIds"}]}}.to_json, + "Bad Request" + ) + end + + duplicate_create_error = -> do + Office365::Exception.new( + HTTP::Status::CONFLICT, + {error: {code: "Request_MultipleObjectsWithSameKeyValue", message: "The service principal cannot be created, updated, or restored because the service principal name x is already in use.", details: [{code: "ObjectConflict", message: "already in use", target: "servicePrincipalNames"}]}}.to_json, + "Conflict" + ) + end + + it "returns the block value when the call succeeds" do + GraphReplicationRetry.run { 42 }.should eq 42 + end + + it "retries a get-or-create that double-created off a stale read" do + attempts = 0 + value = GraphReplicationRetry.run(backoff: {0, 0, 0}) do + attempts += 1 + raise duplicate_create_error.call if attempts < 2 + :converged + end + value.should eq :converged + attempts.should eq 2 + end + + it "retries pre-authorization racing the scope it references" do + attempts = 0 + value = GraphReplicationRetry.run(backoff: {0, 0, 0}) do + attempts += 1 + raise stale_scope_error.call if attempts < 2 + :materialised + end + value.should eq :materialised + attempts.should eq 2 + end + + it "does not retry InvalidValue errors on other properties" do + attempts = 0 + expect_raises(Office365::Exception) do + GraphReplicationRetry.run(backoff: {0, 0}) do + attempts += 1 + raise Office365::Exception.new( + HTTP::Status::BAD_REQUEST, + {error: {code: "Request_BadRequest", message: "bad", details: [{code: "InvalidValue", message: "bad", target: "identifierUris"}]}}.to_json, + "Bad Request" + ) + end + end + attempts.should eq 1 + end + + it "retries transient directory reads while the tenant replicates" do + attempts = 0 + value = GraphReplicationRetry.run(backoff: {0, 0, 0}) do + attempts += 1 + raise directory_busy_error.call if attempts < 2 + :materialised + end + value.should eq :materialised + attempts.should eq 2 + end + + it "retries service principal creation racing the application object" do + attempts = 0 + value = GraphReplicationRetry.run(backoff: {0, 0, 0}) do + attempts += 1 + raise app_not_backed_error.call if attempts < 3 + :materialised + end + value.should eq :materialised + attempts.should eq 3 + end + + it "retries replication lag until the object materialises" do + attempts = 0 + value = GraphReplicationRetry.run(backoff: {0, 0, 0}) do + attempts += 1 + raise replication_lag_error.call if attempts < 3 + :materialised + end + value.should eq :materialised + attempts.should eq 3 + end + + it "gives up once the backoff schedule is exhausted" do + attempts = 0 + expect_raises(Office365::Exception, /Request_ResourceNotFound/) do + GraphReplicationRetry.run(backoff: {0, 0}) do + attempts += 1 + raise replication_lag_error.call + end + end + attempts.should eq 3 + end + + it "does not retry other graph errors" do + attempts = 0 + expect_raises(Office365::Exception) do + GraphReplicationRetry.run(backoff: {0, 0}) do + attempts += 1 + raise Office365::Exception.new(HTTP::Status::FORBIDDEN, {error: {code: "Authorization_RequestDenied", message: "denied"}}.to_json, "Forbidden") + end + end + attempts.should eq 1 + end + + it "does not retry 404s that are not replication lag" do + attempts = 0 + expect_raises(Office365::Exception) do + GraphReplicationRetry.run(backoff: {0, 0}) do + attempts += 1 + raise Office365::Exception.new(HTTP::Status::NOT_FOUND, "gone", "Not Found") + end + end + attempts.should eq 1 + end + end +end diff --git a/src/placeos-rest-api/controllers/tenant_consent.cr b/src/placeos-rest-api/controllers/tenant_consent.cr index a2287f60..d91562cc 100644 --- a/src/placeos-rest-api/controllers/tenant_consent.cr +++ b/src/placeos-rest-api/controllers/tenant_consent.cr @@ -61,8 +61,6 @@ module PlaceOS::Api create_outlook_config(auth_app[:client_id]) strat.update!(client_id: auth_app[:client_id], client_secret: auth_app[:client_secret]) update_auth(authority, strat.id.as(String)) - ensure - update_app_redirect_uri(false) end else Log.warn { {message: "Admin declined consent", error: error.to_s, description: error_description.to_s} } @@ -81,11 +79,13 @@ module PlaceOS::Api app = Office365::Application.single_tenant_app("PlaceOS Bookings Visualiser") .add_required_resource(ra) - created_app = client.create_application(app) + created_app = GraphReplicationRetry.run { client.create_application(app) } Log.debug { {message: "App registerd with Application permissions", tenant: tenant_id, client_id: created_app.app_id.as(String)} } ra.each do |resource| - client.application_add_app_role_assignment(created_app.app_id.as(String), resource["id"]) + GraphReplicationRetry.run do + client.application_add_app_role_assignment(created_app.app_id.as(String), resource["id"]) + end end created_app.app_id.as(String) @@ -107,11 +107,13 @@ module PlaceOS::Api .add_web_redirect_uri("https://#{domain}/auth/oauth2/callback?id=#{strat_id}") .add_required_resource(ra) - created_app = client.create_application(app) + created_app = GraphReplicationRetry.run { client.create_application(app) } Log.debug { {message: "App registerd with Delegated permissions", tenant: tenant_id, client_id: created_app.app_id.as(String)} } - client.application_add_oauth2_permission_grant(created_app.app_id.as(String), "Calendars.ReadWrite Calendars.ReadWrite.Shared Group.Read.All User.Read.All offline_access openid profile") - secret = client.application_add_pwd(created_app.app_id.as(String), "PlaceOS User Auth Secret") + GraphReplicationRetry.run do + client.application_add_oauth2_permission_grant(created_app.app_id.as(String), "Calendars.ReadWrite Calendars.ReadWrite.Shared Group.Read.All User.Read.All offline_access openid profile") + end + secret = GraphReplicationRetry.run { client.application_add_pwd(created_app.app_id.as(String), "PlaceOS User Auth Secret") } {client_id: created_app.app_id.as(String), client_secret: secret.secret_text.as(String)} end @@ -121,7 +123,7 @@ module PlaceOS::Api private def update_app_redirect_uri(add : Bool = true) : Nil client = get_client - app = client.get_application(PLACE_APP_CLIENT_ID, "id,web") + app = GraphReplicationRetry.run { client.get_application(PLACE_APP_CLIENT_ID, "id,web") } app_redirect_uris = app.web.try &.redirect_uris || [] of String return nil if add && app_redirect_uris.includes?(redirect_url) @@ -135,7 +137,7 @@ module PlaceOS::Api app.web.not_nil!.redirect_uris = app_redirect_uris web = {"web" => app.web} begin - client.update_application(PLACE_APP_CLIENT_ID, web.to_json) + GraphReplicationRetry.run { client.update_application(PLACE_APP_CLIENT_ID, web.to_json) } rescue ex : Office365::Exception return nil if already_exists_error?(ex.http_body) raise ex @@ -144,7 +146,7 @@ module PlaceOS::Api private def add_outlook_plugin_auth(app_id : String) : Nil client = get_client - app = client.get_application(app_id) + app = GraphReplicationRetry.run { client.get_application(app_id) } app_redirect_uris = app.web.try &.redirect_uris || [] of String app_redirect_uris.push("#{domain_url}/outlook/#/book/spaces") @@ -171,7 +173,7 @@ module PlaceOS::Api ], }, } - client.update_application(app_id, updated.to_json) + GraphReplicationRetry.run { client.update_application(app_id, updated.to_json) } updated = { "api": { @@ -185,7 +187,7 @@ module PlaceOS::Api ], }, } - client.update_application(app_id, updated.to_json) + GraphReplicationRetry.run { client.update_application(app_id, updated.to_json) } end private def create_outlook_repo : Nil diff --git a/src/placeos-rest-api/utilities/graph-replication-retry.cr b/src/placeos-rest-api/utilities/graph-replication-retry.cr new file mode 100644 index 00000000..a81ba234 --- /dev/null +++ b/src/placeos-rest-api/utilities/graph-replication-retry.cr @@ -0,0 +1,75 @@ +require "json" +require "office365" + +module PlaceOS::Api + # Microsoft Graph is eventually consistent: a directory object that was just + # created (an application registration or service principal) is not always + # visible to an immediately following request that references it. Graph + # signals this as a 404 with the error code `Request_ResourceNotFound`. + # + # Retry exactly that case with a backoff; every other error propagates + # untouched. + module GraphReplicationRetry + Log = ::Log.for(self) + + # seconds between attempts, ~35s in total. Replication typically settles + # within a few seconds but has been observed (sandbox tenant, 2026-08-04) + # taking 25s+. + BACKOFF = {1, 2, 4, 8, 8, 12} + + def self.run(backoff = BACKOFF, & : -> T) : T forall T + attempt = 0 + loop do + begin + return yield + rescue error : ::Office365::Exception + delay = backoff[attempt]? + raise error unless delay && replication_lag?(error) + attempt += 1 + Log.warn { "graph resource not replicated yet (attempt #{attempt}), retrying in #{delay}s" } + sleep delay.seconds + end + end + end + + def self.replication_lag?(error : ::Office365::Exception) : Bool + body = JSON.parse(error.http_body) + case error.http_status + when .not_found? + # Request_ResourceNotFound: a just-created object is not yet visible + # to a request referencing it. + # Directory_ObjectNotFound ("Unable to read the company information + # from the directory"): transient directory read failure while the + # tenant is busy replicating - documented by Microsoft as retryable. + body.dig?("error", "code").try(&.as_s?).in?("Request_ResourceNotFound", "Directory_ObjectNotFound") + when .bad_request? + # NoBackingApplicationObject: a service principal cannot be created + # because the application object registered moments earlier has not + # replicated yet. + # InvalidValue on preAuthorizedApplications.delegatedPermissionIds: + # the PATCH that added the permission scope moments earlier has not + # replicated, so the follow-up PATCH fails validation against a stale + # copy of the application. Deliberately narrow - a generic InvalidValue + # retry would mask real validation errors. + details = body.dig?("error", "details").try(&.as_a?) || [] of JSON::Any + details.any? do |detail| + code = detail["code"]?.try(&.as_s?) + target = detail["target"]?.try(&.as_s?) + code == "NoBackingApplicationObject" || + (code == "InvalidValue" && target == "api.preAuthorizedApplications.delegatedPermissionIds") + end + when .conflict? + # Request_MultipleObjectsWithSameKeyValue on a service principal + # create: the preceding existence check read a stale replica that did + # not yet list the service principal created moments earlier, so the + # get-or-create tried to create it twice. Retrying converges - the + # next read eventually sees the object and the create is skipped. + body.dig?("error", "code").try(&.as_s?) == "Request_MultipleObjectsWithSameKeyValue" + else + false + end + rescue JSON::ParseException + false + end + end +end