Reuse existing connection when executing each background job

[jtgeibel]

This is a backport of some work done in #4892. Once this lands it should be easy to finally land the port to diesel 2.0!

The 2nd commit includes 2 quick fixes to avoid obtaining 2 connections at the same time (which is now necessary to pass tests). We could do some refactoring to reuse an existing connection instead of dropping and reobtaining one, but these endpoints aren't performance critical so I'm not too worried about it.

[Turbo87]

I'm not sure I fully understand the implications of this PR yet. Do I read this correctly that all the background jobs now share a single database connection? Doesn't that prevent us from running jobs in parallel? 🤔

[jtgeibel]

It will still be possible to run jobs in parallel. The previous design used 2 connections per task that it completed. One connection selects and locks a job row, and it then sits idle while the job (if needed) obtains its own connection to do its work. That is what was causing an issue with our test framework. We were using a reentrant mutex so that each of these connections were actually the same &PgConnection when running tests (but not in production). But now that we need a &mut PgConnection there is no safe way to obtain 2 unique references to the same value even though we stay within the same thread the whole time.

The new design is that each connection locks a job and then enters an inner transaction so that the job can reuse the database connection. Worst case the job only needs access to the deserialized JSON and this connection still sits idle while the task is executed.

The risk, addressed in the 3rd commit, is that we need to take care not to return a connection to the pool while it is still within a transaction. That could happen if the background job panics, so we manually do enough rollbacks to get the connection into the expected state. (Before, if the job panicked, we still had the untouched original connection holding the row locked so that we can update the failure count. And if the job had its own connection, then during a panic I assume the PgConnection is either rolled back correctly or discarded instead of being returned to the pool.)

[weiznich]

The risk, addressed in the 3rd commit, is that we need to take care not to return a connection to the pool while it is still within a transaction. That could happen if the background job panics, so we manually do enough rollbacks to get the connection into the expected state. (Before, if the job panicked, we still had the untouched original connection holding the row locked so that we can update the failure count. And if the job had its own connection, then during a panic I assume the PgConnection is either rolled back correctly or discarded instead of being returned to the pool.)

To be clear here: Diesel 1.4 will happily return a connection from a panicked thread or with an open transaction to the connection pool. Diesel 2.0 introduced several checks to prevent this behavior.

[bors]

☔ The latest upstream changes (presumably #5859) made this pull request unmergeable. Please resolve the merge conflicts.

[Turbo87]

@jtgeibel I've rebased the PR to fix the merge conflicts.

any thoughts on the "infinite loop" comment above?

[bors]

☔ The latest upstream changes (presumably #5558) made this pull request unmergeable. Please resolve the merge conflicts.

[jtgeibel]

As discussed in today's meeting I looked into mem::forgetting the connection to avoid the potential infinite loop. In order to drop the value (and not a reference) it would take a lot of careful refactoring to propagate the error to the right place while handling all the edge cases. Since this is temporary and a robust solution should be much easier with diesel 2.0, I've opted to just leak the thread as well. Meanwhile, in the rare case that we would hit this our existing monitoring should pick up on a job that runs for to long.

[Turbo87]

• controllers/crate_owner_invitations: Avoid second database connection request #5891
• controllers/github: Reuse existing database connection in send_notification_email() call #5890
• auth: Reuse existing database connections #5889

removed the need for the drop() calls. I've rebased the branch on top of these changes. Otherwise this LGTM. Feel free to merge :)

[jtgeibel]

Looks good, merging. One thing your PRs got me thinking about, is that due to the implicit update of the token's last_used_at we won't correctly update this column if the auth check is passed a read-only connection. When used with AuthCheck::only_cookie() this isn't an issue, as this update will run and fail and a later check rejects the authentication because it wasn't via a cookie. Everything ends up consistent.

I checked and there is only one place where we pass a read-only connection to a default auth check. That occurs when passing a following query parameter to the search endpoint. This can probably be made a cookie only option, though I'm not too worried about updating the last_used_at in this particular case.

[Turbo87]

Indeed, that's a good point! 👍