DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block is deprecated without replacement. If the `throw` came from `Timeout.timeout(duration)`, pass an exception class as a second argument so it doesn't use `throw` to abort its block. This results in the transaction being committed, but in the next release of Rails it will rollback. (called from public_send at /srv/gitlab/lib/gitlab/database/load_balancing/connection_proxy.rb:127)
If you are unsure about the correct group, please do not leave the issue without a group label, and refer to
GitLab's shared responsibility functionality guidelines
for more information on how to triage this kind of issue.
This issue was automatically tagged with the label ~"group::application performance" by TanukiStan, a machine learning classification model, with a probability of 0.27.
If this label is incorrect, please tag this issue with the correct group label as well as automation:ml wrong to help TanukiStan learn from its mistakes.
It seems that the deprecation warning was brought back in https://github.com/rails/rails/pull/44607 and should be available in 7.0.4.3. No silent rollbacks when transaction block is exited via return/break/throw.
Introducing will help with return and break but it could be trickier for throw because it can be used in methods deeper in the stack. However, with Cop/BanCatchThrow we've forbidden the use of throw anyway.
In order to avoid break (and return) in Model.transaction blocks one could:
Extract code into methods and use guards (early return) instead
Refactor/restructure related conditionals in the block
For example:
# badProject.transactiondosome_codebreakifok?even_more_codeend# good - Extract methoddeflogic_in_methodsome_codereturnifok?even_more_codeendProject.transactiondologic_in_methodend# good - refactor conditionalsProject.transactiondosome_codeunlessok?even_more_codeendend
Edit: This found one more offense because it also checks with_lock next to transaction:
Offenses:app/services/ci/delete_unit_tests_service.rb:28:9: C: Rails/TransactionExitStatement: Exit statement break is not allowed. Use raise (rollback) or next (commit). break if ids.empty? ^^^^^app/services/concerns/update_repository_storage_methods.rb:18:7: C: Rails/TransactionExitStatement: Exit statement return is not allowed. Use raise (rollback) or next (commit). return ServiceResponse.success unless repository_storage_move.scheduled? # rubocop:disable Cop/AvoidReturnFromBlocks ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^app/services/projects/create_service.rb:234:11: C: Rails/TransactionExitStatement: Exit statement break is not allowed. Use raise (rollback) or next (commit). break if @project.import? ^^^^^app/services/work_items/delete_task_service.rb:25:9: C: Rails/TransactionExitStatement: Exit statement break is not allowed. Use raise (rollback) or next (commit). break ::ServiceResponse.error(message: replacement_result.errors, http_status: 422) if replacement_result.error? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ee/app/workers/gitlab_subscriptions/refresh_seats_worker.rb:46:9: C: Rails/TransactionExitStatement: Exit statement break is not allowed. Use raise (rollback) or next (commit). break unless subscription ^^^^^ee/lib/gitlab/geo/replicator.rb:207:11: C: Rails/TransactionExitStatement: Exit statement break is not allowed. Use raise (rollback) or next (commit). break if results.rows.empty? ^^^^^30859 files inspected, 6 offenses detected
I also fixed one instance not caught by RuboCop in our seeds. In db/fixtures/development/24_forks.rbreturn was used within Gitlab::Seeder.quiet do that creates an transaction I believe.
The error message doesn't give enough stack trace:
DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block isdeprecated without replacement. If the `throw` came from`Timeout.timeout(duration)`, pass an exception class as a secondargument so it doesn't use `throw` to abort its block. This resultsin the transaction being committed, but in the next release of Railsit will rollback. (called from public_send at /srv/gitlab/lib/gitlab/database/load_balancing/connection_proxy.rb:127)
We may need to monkey-patch to extend the stack trace
$ bin/rspec ./spec/requests/api/commit_statuses_spec.rb[1:2:1:3:2:3:1,1:2:1:4:2,1:2:1:7:2:1,1:2:1:9:1,1:2:1:10:1] --order definedwarning: parser/current is loading parser/ruby31, which recognizes 3.1.3-compliant syntax, but you are running 3.1.4.Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.Run options: include {:focus=>true, :ids=>{"./spec/requests/api/commit_statuses_spec.rb"=>["1:2:1:3:2:3:1", "1:2:1:4:2", "1:2:1:7:2:1", "1:2:1:9:1", "1:2:1:10:1"]}}Test environment set up in 5.980939848 secondsAPI::CommitStatuses POST /projects/:id/statuses/:sha developer user with all optional parameters when updating a commit status when the `state` parameter is sent the same does not update the commit status when retrying a commit status retries the commit status (FAILED - 1) when updating a protected ref with user as maintainer creates commit status when target URL is an invalid address responds with bad request status and validation errors (FAILED - 2) when target URL is an unsupported scheme responds with bad request status and validation errors (FAILED - 3)Failures: 1) API::CommitStatuses POST /projects/:id/statuses/:sha developer user when retrying a commit status retries the commit status Failure/Error: expect(CommitStatus.count).to eq 2 expected: 2 got: 3 (compared using ==) # ./spec/requests/api/commit_statuses_spec.rb:429:in `block (5 levels) in <top (required)>' # ./spec/spec_helper.rb:415:in `block (3 levels) in <top (required)>' # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware' # ./spec/spec_helper.rb:406:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:402:in `block (3 levels) in <top (required)>' # ./lib/gitlab/application_context.rb:61:in `with_raw_context' # ./spec/spec_helper.rb:402:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:243:in `block (2 levels) in <top (required)>' # ./spec/support/sidekiq.rb:17:in `block (3 levels) in <main>' # ./spec/support/sidekiq.rb:8:in `gitlab_sidekiq_inline' # ./spec/support/sidekiq.rb:17:in `block (2 levels) in <main>' # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>' 2) API::CommitStatuses POST /projects/:id/statuses/:sha developer user when target URL is an invalid address responds with bad request status and validation errors Failure/Error: expect(response).to have_gitlab_http_status(:bad_request) expected the response to have status code :bad_request but it was 403. The response was: {"message":"403 Forbidden"} # ./spec/requests/api/commit_statuses_spec.rb:499:in `block (5 levels) in <top (required)>' # ./spec/spec_helper.rb:415:in `block (3 levels) in <top (required)>' # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware' # ./spec/spec_helper.rb:406:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:402:in `block (3 levels) in <top (required)>' # ./lib/gitlab/application_context.rb:61:in `with_raw_context' # ./spec/spec_helper.rb:402:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:243:in `block (2 levels) in <top (required)>' # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>' 3) API::CommitStatuses POST /projects/:id/statuses/:sha developer user when target URL is an unsupported scheme responds with bad request status and validation errors Failure/Error: expect(response).to have_gitlab_http_status(:bad_request) expected the response to have status code :bad_request but it was 403. The response was: {"message":"403 Forbidden"} # ./spec/requests/api/commit_statuses_spec.rb:514:in `block (5 levels) in <top (required)>' # ./spec/spec_helper.rb:415:in `block (3 levels) in <top (required)>' # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware' # ./spec/spec_helper.rb:406:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:402:in `block (3 levels) in <top (required)>' # ./lib/gitlab/application_context.rb:61:in `with_raw_context' # ./spec/spec_helper.rb:402:in `block (2 levels) in <top (required)>' # ./spec/spec_helper.rb:243:in `block (2 levels) in <top (required)>' # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (3 levels) in <main>' # ./spec/support/database/prevent_cross_joins.rb:62:in `with_cross_joins_prevented' # ./spec/support/database/prevent_cross_joins.rb:108:in `block (2 levels) in <main>'Finished in 13.41 seconds (files took 11.02 seconds to load)5 examples, 3 failuresFailed examples:rspec ./spec/requests/api/commit_statuses_spec.rb:426 # API::CommitStatuses POST /projects/:id/statuses/:sha developer user when retrying a commit status retries the commit statusrspec ./spec/requests/api/commit_statuses_spec.rb:498 # API::CommitStatuses POST /projects/:id/statuses/:sha developer user when target URL is an invalid address responds with bad request status and validation errorsrspec ./spec/requests/api/commit_statuses_spec.rb:513 # API::CommitStatuses POST /projects/:id/statuses/:sha developer user when target URL is an unsupported scheme responds with bad request status and validation errors[TEST PROF INFO] Time spent in factories: 00:02.479 (14.95% of total time)
Also, I notice we do disallow Using return, breakorthrow to exit a transaction block but DeprecationException is a StandardError so it gets rescued by application code
You can see the error if we add below, then run rspec ./spec/requests/api/commit_statuses_spec.rb
diff --git a/config/initializers/00_deprecations.rb b/config/initializers/00_deprecations.rbindex 8aff095d88bc..11022999c90d 100644--- a/config/initializers/00_deprecations.rb+++ b/config/initializers/00_deprecations.rb@@ -18,7 +18,7 @@ else ActiveSupport::Deprecation.silenced = false ActiveSupport::Deprecation.behavior = [:stderr, :notify]- ActiveSupport::Deprecation.disallowed_behavior = :raise+ ActiveSupport::Deprecation.disallowed_behavior = [:stderr, :raise] rails7_deprecation_warnings = [ # https://gitlab.com/gitlab-org/gitlab/-/issues/366910$ bundle exec rspec /Users/tkuah/code/gdk-ee/gitlab/spec/requests/api/commit_statuses_spec.rbwarning: parser/current is loading parser/ruby31, which recognizes 3.1.3-compliant syntax, but you are running 3.1.4.Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.Run options: include {:focus=>true}All examples were filtered out; ignoring {:focus=>true}Test environment set up in 7.329075 seconds..........................DEPRECATION WARNING: Using `return`, `break` or `throw` to exit a transaction block isdeprecated without replacement. If the `throw` came from`Timeout.timeout(duration)`, pass an exception class as a secondargument so it doesn't use `throw` to abort its block. This resultsin the transaction being committed, but in the next release of Railsit will rollback. (called from public_send at /Users/tkuah/code/gdk-ee/gitlab/lib/gitlab/database/load_balancing/connection_proxy.rb:127)...F.....FF.*...
@tkuah@engwan@splattael interesting, I haven't identified that any of the throws are triggered in state_machines gem; however, I see that the deprecation warning is raised when the transaction hasn't been completed correctly.
@igor.drozdov it can be removed because we've managed to attach ci_builds as a partition to the p_ci_builds partitioned table in the past weekend and I don't think that self-managed have enough traffic to trigger the deadlocks that we saw in production.