`ctx->txn_do()` needs to happen in a transaction block.

[wolfsage]

_save_states() was happening outside of a transaction! This meant it could run and fail to save states even if the changes were successfully committed to the DB! To avoid that we now wrap the entire call in a transaction instead of just the supplied code.

[mmcclimon]

Seems sensible to me, though I don't promise to fully understand the test.

[wolfsage]

Let's see...

So typically, creating a new account creates base state tables for all entities in the account.

However, if you've added new entity types, exiting accounts won't have those rows prefilled in.

This means get_collection_lock() will just succeed without actually holding a lock since no row exists.

So two clients come along and at the same time call setFoso (where no Foo state row exists). They both will try to INSERT into the states table, and one of them will fail.

Before this commit, that INSERT happened outside of a transaction, and fail, so the entity rows would be created even though the client got an internalError, thinking nothing was committed to the DB.

With this commit, we moved the state saving to inside of the transaction, so if this condition arises, the second client will receive a 'tryAgain' error instead of an internal error, and the rows it had created will be rolled back and not committed.

The test mimics this scenario by clearing out the state tables, setting up two clients to create cookies at the same time, then unlocking them at the same time so when they fire they each try to create state rows at the end.

One of them will succeed and get a cookiesSet response. The other will fail and get a tryAgain response. And only 1 cookie should have been successfully created in the DB.

[wolfsage]

@rjbs Can you give this one a look too? Thanks

[rjbs]

Okay. Go for it.