P20-1549: Fix User.find_channel_owner test

[artem-vavilov]

Issue

=ERROR["test_find_channel_owner_returns_nil_for_channel_with_no_owner", "UserTest", 670.325951756]
740 | test_find_channel_owner_returns_nil_for_channel_with_no_owner#UserTest (670.33s)
741 | Minitest::UnexpectedError:         Sequel::DatabaseError: Mysql2::Error::TimeoutError: Timeout waiting for a response from the last query. (waited 30 seconds)
742 | test/testing/projects_test_utils.rb:31:in `with_storage_id_for'
743 | test/testing/projects_test_utils.rb:9:in `with_channel_for'
744 | test/testing/projects_test_utils.rb:35:in `with_anonymous_channel'
745 | test/models/user_test.rb:4246:in `block in <class:UserTest>'
746 | test/testing/setup_all_and_teardown_all.rb:36:in `run'

Links

• JIRA task: P20-1549
• Related PR: skip flaky unit test for channel with no owner #66713

[Copilot]

Pull request overview

This PR fixes a flaky test that was timing out by refactoring the storage ID management approach. Instead of manually creating and cleaning up storage IDs, the code now uses database transactions with automatic rollback to ensure proper cleanup and prevent database contention.

Key Changes:

• Replaced manual storage ID lifecycle management with transactional approach using rollback: :always
• Re-enabled previously skipped flaky test find_channel_owner returns nil for channel with no owner

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
dashboard/test/testing/projects_test_utils.rb	Refactored with_storage_id_for to use database transactions with automatic rollback instead of manual cleanup
dashboard/test/models/user_test.rb	Removed skip statement to re-enable the previously flaky test

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

To use Codex here, create a Codex account and connect to github.

[davidsbailey]

Hey Artem, nice work tracking this down! While the passing drone runs are convincing, I'm not sure I understand what exactly the problem was and why the new code is better. can you help me understand the fix? additionally, is there a test case you could add for with_storage_id_for that would help clarify what the issue was and make sure we don't regress?

[davidsbailey]

@artem-vavilov apologies but please ignore the requests in my previous comment. after reading copilot's summary, I'm convinced that you're doing this the right way and that no one is going to try to undo it, and it doesn't need unit tests just to prove a db transaction is working. LGTM

[artem-vavilov]

Hey Artem, nice work tracking this down! While the passing drone runs are convincing, I'm not sure I understand what exactly the problem was and why the new code is better. can you help me understand the fix? additionally, is there a test case you could add for with_storage_id_for that would help clarify what the issue was and make sure we don't regress?

Not entirely sure what the root cause was. The Sequel request executed inside with_storage_id_for would occasionally time out and fail.

I addressed this by wrapping the logic in a transaction and always rolling it back. This avoids committing any new records and immediately reverts the changes, instead of issuing a separate DELETE, which should be more efficient and safer under contention.