proxy_instance_serial_ws() uses select! with non-cancel-safe future(s)

[jgallagher]

In proxy_instance_serial_ws(), we construct four futures over which we will tokio::select!. In any given loop iteration, at least two of the futures are instances of Fuse::terminated() (and therefore ignored by select!). The possible combinations of non-Fuse::terminated() futures are:

1. nexus_read (a StreamExt::next() future) and nexus_write(a SinkExt::send() future)
2. nexus_read and propolis_read (a future from InstanceSerialConsoleHelper::recv() in propolis)
3. propolis_write (a future from support::send in propolis which is a wrapper around SinkExt::send()) alone
4. propolis_write and nexus_write

Case 3 is fine because there is only one possible future for select! to choose, so there is no possibility for cancellation. But in the other three cases, select! will cancel one of the two futures once it chooses one. In terms of cancel safety, none of these futures is explicitly documented; however:

• I believe (hope?) that StreamExt::next() is cancel safe, because the equivalent method in tokio-stream's StreamExt is explicitly documented as cancel safe.
• I believe SinkExt::send() is not cancel safe: if select!ing over a SinkExt::send() future and a different arm is chosen, the item being sent to the sink will be lost. (A reproducer; I'll also file an issue on futures-util to confirm / try to get it documented. EDIT: filed Documenting cancel safety of SinkExt::send() rust-lang/futures-rs#2754)
• I'm not sure whether InstanceSerialConsoleHelper::recv() is cancel safe, but based on it containing multiple .await points I suspect it is not: if an item is pulled out of self.ws_stream and the future is dropped at a later await point, I believe the item will be lost.

I'm not sure at the moment how to address this; I'm just skimming through omicron's select!s looking for suspicious cancel safety issues. CC @lifning

[sunshowers]

I'm not sure whether InstanceSerialConsoleHelper::recv() is cancel safe, but based on it containing multiple .await points I suspect it is not: if an item is pulled out of self.ws_stream and the future is dropped at a later await point, I believe the item will be lost.

Indeed InstanceSerialConsoleHelper::recv wasn't cancel-safe. oxidecomputer/propolis#435 addresses that, and once it lands and omicron is updated to that version, it should be cancel-safe.

[sunshowers]

I believe SinkExt::send() is not cancel safe: if select!ing over a SinkExt::send() future and a different arm is chosen, the item being sent to the sink will be lost. (A reproducer; I'll also file an issue on futures-util to confirm / try to get it documented. EDIT: filed rust-lang/futures-rs#2754)

I think the best solution here is to define a version of SinkExt::send that's cancel-safe. I've outlined a proposal here: rust-lang/futures-rs#2754 (comment)

[sunshowers]

Current status:

• propolis_conn.recv() will become cancel-safe once Update Omicron to latest propolis with InstanceSerialConsoleHelper #3391 lands.
• I'm actively working on making the send branches cancel-safe via https://github.com/oxidecomputer/cancel-safe-futures/

[sunshowers]

The remaining uses have been tackled in #3411. We should be good with respect to cancel safety now.