From bf5ceff26926b63a07b0ae4ac1ecccdddbbf7c8b Mon Sep 17 00:00:00 2001 From: Brandon Date: Wed, 26 Aug 2026 22:30:35 -0700 Subject: [PATCH] Fix flaky sign_out teardown in six test suites (#3208) Six suites (passkey, MFA, SnapTrade, categorize, onboarding and the Active Storage authorization integration tests) share a sign_out helper that deletes the user's sessions through the controller, one HTTP request per session, iterating in unspecified order. The moment the loop deletes the session the test itself is signed in with, every later request in the loop is unauthenticated and silently deletes nothing, so whichever sessions happen to sort after it survive. The sessions fixture belongs to the same user these suites use, so a surviving fixture row then fails every assertion that expects the user to have no sessions. Row order usually favors the fixture, which is why the suites usually pass. Under parallel CI they fail a few times a week, always in this file family, always with the fixture session as the leftover. Forcing newest-first order reproduces it deterministically on current main: ten of the fifteen passkey tests fail. Teardown hygiene is not the behavior under test, so the helpers now destroy the sessions directly, which no order can break. All six suites run green three times in a row. --- test/controllers/mfa_controller_test.rb | 9 ++++++--- test/controllers/onboardings_controller_test.rb | 9 ++++++--- test/controllers/passkey_sessions_controller_test.rb | 7 ++++++- test/controllers/snaptrade_items_controller_test.rb | 9 ++++++--- .../transactions/categorizes_controller_test.rb | 7 ++++++- test/integration/active_storage_authorization_test.rb | 4 +++- 6 files changed, 33 insertions(+), 12 deletions(-) diff --git a/test/controllers/mfa_controller_test.rb b/test/controllers/mfa_controller_test.rb index 39aa2953a..ea098eba9 100644 --- a/test/controllers/mfa_controller_test.rb +++ b/test/controllers/mfa_controller_test.rb @@ -8,9 +8,12 @@ class MfaControllerTest < ActionDispatch::IntegrationTest end def sign_out - @user.sessions.each do |session| - delete session_path(session) - end + # Deleting sessions through the controller de-authenticates the request the + # moment our own session dies, so every later delete in the loop is a + # silent no-op and whichever sessions sort after it survive. The order is + # unspecified, which made every suite that signs out this way flaky. + # Teardown hygiene is not the behavior under test, so destroy directly. + @user.sessions.destroy_all end test "redirects to root if MFA already enabled" do diff --git a/test/controllers/onboardings_controller_test.rb b/test/controllers/onboardings_controller_test.rb index d1bfbe4cf..43e72da67 100644 --- a/test/controllers/onboardings_controller_test.rb +++ b/test/controllers/onboardings_controller_test.rb @@ -210,8 +210,11 @@ end private def sign_out - @user.sessions.each do |session| - delete session_path(session) - end + # Deleting sessions through the controller de-authenticates the request the + # moment our own session dies, so every later delete in the loop is a + # silent no-op and whichever sessions sort after it survive. The order is + # unspecified, which made every suite that signs out this way flaky. + # Teardown hygiene is not the behavior under test, so destroy directly. + @user.sessions.destroy_all end end diff --git a/test/controllers/passkey_sessions_controller_test.rb b/test/controllers/passkey_sessions_controller_test.rb index 07b1d8e23..7d17e84fe 100644 --- a/test/controllers/passkey_sessions_controller_test.rb +++ b/test/controllers/passkey_sessions_controller_test.rb @@ -226,7 +226,12 @@ class PasskeySessionsControllerTest < ActionDispatch::IntegrationTest end def sign_out - @user.sessions.each { |session| delete session_path(session) } + # Deleting sessions through the controller de-authenticates the request the + # moment our own session dies, so every later delete in the loop is a + # silent no-op and whichever sessions sort after it survive. The order is + # unspecified, which made every suite that signs out this way flaky. + # Teardown hygiene is not the behavior under test, so destroy directly. + @user.sessions.destroy_all end def with_webauthn_config(rp_id:, allowed_origins:) diff --git a/test/controllers/snaptrade_items_controller_test.rb b/test/controllers/snaptrade_items_controller_test.rb index 4f9f83284..7274ac139 100644 --- a/test/controllers/snaptrade_items_controller_test.rb +++ b/test/controllers/snaptrade_items_controller_test.rb @@ -7,9 +7,12 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest end def sign_out - @user.sessions.each do |session| - delete session_path(session) - end + # Deleting sessions through the controller de-authenticates the request the + # moment our own session dies, so every later delete in the loop is a + # silent no-op and whichever sessions sort after it survive. The order is + # unspecified, which made every suite that signs out this way flaky. + # Teardown hygiene is not the behavior under test, so destroy directly. + @user.sessions.destroy_all end # A deployment with a confidential OAuth client: both the browser redirect and diff --git a/test/controllers/transactions/categorizes_controller_test.rb b/test/controllers/transactions/categorizes_controller_test.rb index 8c40f3286..a1e96b250 100644 --- a/test/controllers/transactions/categorizes_controller_test.rb +++ b/test/controllers/transactions/categorizes_controller_test.rb @@ -176,7 +176,12 @@ class Transactions::CategorizesControllerTest < ActionDispatch::IntegrationTest private def sign_out - @user.sessions.each { |s| delete session_path(s) } + # Deleting sessions through the controller de-authenticates the request the + # moment our own session dies, so every later delete in the loop is a + # silent no-op and whichever sessions sort after it survive. The order is + # unspecified, which made every suite that signs out this way flaky. + # Teardown hygiene is not the behavior under test, so destroy directly. + @user.sessions.destroy_all end # POST /transactions/categorize diff --git a/test/integration/active_storage_authorization_test.rb b/test/integration/active_storage_authorization_test.rb index 9c30d4f91..0a118df08 100644 --- a/test/integration/active_storage_authorization_test.rb +++ b/test/integration/active_storage_authorization_test.rb @@ -288,7 +288,9 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest private def sign_out(user) - user.sessions.each { |session| delete session_path(session) } + # Deleting through the controller de-authenticates mid-loop and later + # deletes silently no-op; destroy directly, no order to break. + user.sessions.destroy_all end def with_protected_record_types(*types)