Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure fresh test db's aren't accidentally deleted by do_cleanup #2454

Merged
merged 1 commit into from May 4, 2023

Conversation

phlip9
Copy link
Contributor

@phlip9 phlip9 commented Apr 13, 2023

I recently encountered a panic in our CI where a test db created by the #[sqlx::test] macro was seemingly missing, right after it was created and right before we connected to it.

thread panicked at 'failed to connect to test database: Database(PgDatabaseError { severity: Fatal, code: "3D000", message: "database \"_sqlx_test_38\" does not exist", detail: None, hint: None, position: None, where: None, schema: None, table: None, column: None, data_type: None, constraint: None, file: Some("postinit.c"), line: Some(885), routine: Some("InitPostgres") })',
    at sqlx-core/src/testing/mod.rs:243:10
stack backtrace:

   # ..
   4: sqlx_core::testing::setup_test_db::{{closure}}
             at sqlx-core/src/testing/mod.rs:240:20
   5: sqlx_core::testing::run_test::{{closure}}
             at sqlx-core/src/testing/mod.rs:216:63
   # ..
// file: sqlx-core/src/testing/mod.rs

fn run_test<DB, F, Fut>(args: TestArgs, test_fn: F) -> Fut::Output
where
    // ..
{
    crate::rt::test_block_on(async move {
        let test_context = DB::test_context(&args) // << db is created
            .await
            .expect("failed to connect to setup test database");

        setup_test_db::<DB>(&test_context.connect_opts, &args).await; // << PANIC
        // ..
    })
}

async fn setup_test_db<DB: Database>(/* .. *) {
    let mut conn = copts
        .connect()
        .await
        .expect("failed to connect to test database"); // << PANIC - db doesn't exist!
    // ..
}

Fortunately, it wasn't too hard to find the problem:

If the first test thread is a bit slow after it acquires the DO_CLEANUP permit, it can accidentally delete a fresh test db created by another thread right before that other thread has a chance to open its connection.

I have a branch (phlip9@60a5955) with some thread::sleep's sprinkled around that manages to reproduce this panic almost 100% of the time. Run the failing test with ./x.py -e postgres_15_tokio --test postgres-test-db-race or ./x.py -e mysql_8_tokio --test mysql-test-db-race.

The fix in this PR simply records the current timestamp before we acquire the DO_CLEANUP permit and only cleans up test db's created before then.

Note: the next commit in that branch contains the fix, so we can see that the issue is resolved:

phlip9@1d78ba2

If the first test thread is a bit slow after it acquires the
`DO_CLEANUP` permit, it can accidentally delete a fresh test db created
by another thread right before that other thread has a chance to open
its connection.

This fix simply records the current timestamp _before_ we acquire the
`DO_CLEANUP` permit and only cleans up test db's created before then.
@phlip9
Copy link
Contributor Author

phlip9 commented Apr 19, 2023

cc @abonander : )

@abonander abonander merged commit 1227d5d into launchbadge:main May 4, 2023
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants