This is super weird :-/

This is the part that feels very underspecified:

In other words, the command handler service host must be able to recover to a known consistent state after cancelling your work,

What defines a 'known consistent state'. It sounds like it's saying: if you bubble cancellation up, you can't have mutated anything on the UI at all.

If that's a requirement, i'd say we need to go further here and we must make all handlers go through a path that simply does not allow cancellation to ever bleed out (as 'no mutation on cancellation' has never been a metaphor we've supported for handlers).

Tagging @jasonmalinowski

The weird thign here is that editor is both cancelling, but also then deciding taht after cancelling that they want to do something. That feels weird ot me as i would expect that once a user cancels something, nothing else happens at that point. If they do feel that things should rollback, i would recommend that they themslves start a transaction and they rollback to the starting point so that everything returns back and there doesn't need to be this handshake between us :(

If that's a requirement, i'd say we need to go further here and we must make all handlers go through a path that simply does not allow cancellation to ever bleed out (as 'no mutation on cancellation' has never been a metaphor we've supported for handlers).

Most handlers already do the right thing today, there's just a couple that have fallen through the cracks. Furthermore, after addressing Sam's feedback, it seems like we can't just take a "never let cancellation bleed out" approach, as different handlers will need to catch the exception at different locations.

If they do feel that things should rollback, i would recommend that they themslves start a transaction and they rollback to the starting point so that everything returns back and there doesn't need to be this handshake between us :(

I agree that this would be best for long-term, however I feel like this would require significant rework/design on Editor side for how they do things. For now, I feel like this is at least a short-term solution that mitigates the problem.
@olegtk Do you have a perspective on this? Would Editor ever consider doing the rollback work to the starting point for command handlers instead of leaving the work up to individual languages?

Does CompleteStatementCommandHandler (and potentially other command handlers) need a similar change?

Hmm, I don't think so. The main issue in the problematic handlers was that nextCommandHandler() would be called, and that call could modify the text buffer, and then after that call we could throw cancellation in the handler, which violates the Editor API guidelines and could mess up the current state of things. CompleteStatementCommandHandler doesn't do any work that may trigger cancellation after nextCommandHandler() is called, so I think it's safe.

@CyrusNajmabadi Do you have any further concerns with this PR? I agree that the work to potentially rollback changes would ideally be done on the Editor side, however that seems like it would take quite a bit of planning and work on their end. At least for now, I feel like this PR is a good mitigation to the issues being observed.

Effectively it feels fragile. let me review again.

Talked with Cyrus/Sam offline - seems like this is OK to be merged as a temporary solution, however in the near future we'd like to make cancellation handling in command handlers more robust so that each handler does not need to deal with cancellation individually. This is tracked by #52528.