Ensure transaction tracking finishes when reconnecting

[ipc103]

Motivation / Background

We noticed a couple of spots where transaction tracking events were potentially incorrect.

• When the connection reconnects with restore_transactions: true, we were keeping the original start time for the transaction. In this case, it makes more sense to treat these as two separate transactions for tracking purposes.
• If you manually call reconnect! within a transaction opened via being_transaction rather than transaction do, we were never marking the event as finished and thus never broadcasting the event. This isn't public API and likely isn't very common, but we did run into one scenario in a test file that was doing this in a setup block.

Detail

We added a line to the reset_transactions method to mark any open transactions as finished. We went back and forth on the naming for this new method, but eventually settled on suspend_transactions since the transactions may potentially be restarted.

Additional information

This is a followup to #49192 and #49262

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[rafaelfranca]

Why we need to use defined? here and not in the line below?

[ipc103]

Good question - since this method is called in initialize without the restore option, the @transaction_manager was always initialized before being accessed. Maybe it would be better to do something like this here:

if restore
  @transaction_manager.suspend_transactions
  old_state = @transaction_manager if @transaction_manager.restorable?
end

but I'm going to rework this based on the additional feedback, so I'll remove this line entirely.

[matthewd]

Agree naming is hard here... but I don't like "suspend". "detach" is probably the closest active verb I can come up with ("abandon" feels too final), but perhaps "lost" is a better fit: it's something that happened to us, not something we want to do. Or maybe "break"?

In the current state of things¹, I'd much prefer that the finish live in restore!, to avoid propagating knowledge of the "we've been disconnected, but might try to fix it" pseudo-state -- right now that knowledge doesn't go deeper than reset_transaction, and the TransactionManager (and Transaction) live in blissful ignorance until the moment they're asked to restore. But that obviously only works for the restore-true case. 😕

It's certainly true that Transaction is already rather sensitive to its caller knowing what methods are permissible to call at any given time, but I think most existing possible confusions will produce an exception at some point. It seems like TransactionInstrumenter#start-when-started and TransactionInstrumenter#finish-when-not-started should almost always be errors (modulo some currently-absent if materialized? checks)... if it would be practical to make those raise, I think that would at least keep it closer to "sensitive, but it'll break" rather than slipping in to "sensitive, and we just have to be careful"?

Footnotes

1. I can see a future where we keep a known-broken-and-unrestorable transaction on the txn stack until we have unwound to the corresponding point in the ruby stack, and outright refuse to touch the connection as long as it's there... but that means (a bunch of edge-case behaviour [improvements, but] changes, and) more actual state transitions. ↩