Transaction commital within tasks and savepoint

[imothee]

Hey all, wondering if anyone else has run into this issue where they're trying to persist database changes in a task using the transaction.

I had assumed that underway would handle the committing if the task succeeded (and it does for the task itself) but I was getting savepoint rollbacks that I couldn't work out why it was happening.

It looks like if I call tx.commit at the end of the task magic happens and it persists properly but if I don't the savepoint is reset and then the transaction commits.

let execute_tx = tx.begin().await?;

tokio::select! {
    result = self.task.execute(execute_tx, input) => {
        match result {
            Ok(_) => {
                in_progress_task.mark_succeeded(&mut tx).await?;
            }
            // ...
        }
    }
    // ... timeout logic ...
}

execute_tx.commit();

I think the issue here is self.task.execute(execute_tx, input).

The execute_tx variable is moved into the select! branch.

If the branch for result completes, execute_tx is consumed and implicitly dropped at the end of the match block.
Because sqlx::Transaction implements a Drop handler that performs a rollback if the transaction is still active, the execute_tx sub-transaction is rolled back when it goes out of scope and is dropped.
The top-level transaction tx is then committed by underway's subsequent logic, but it no longer includes your INSERT. This leaves a permanent record in the underway.task table that doesn't correspond to a successful database operation in your table.

Is this expected behavior, should I be committing myself or should we be expecting underway to be committing on our behalf.

[maxcountryman]

should I be committing myself or should we be expecting underway to be committing on our behalf

My thought here is it's probably a better API for callers to commit explicitly. That said, yes it's confusing that the worker itself seems to be committing the transactions when in practice that doesn't work it might seem it should.

I'm open to different approaches.

[imothee]

Totally fine, it just caught me by surprise and I didn't see it clearly documented anywhere that there was an internal task snapshot to commit.