This type comes from here, which I believe is based on the WebSocket spec

I agree – now that we have a union-of-disjoint-types, it seems like we just need to check that message.type === 'refreshKeysRequest'

It is actually safer to call mark_keys_as_published before they are actually uploaded since imagine a case when the keyserver crashes after rustAPI.uploadOneTimeKeys completes and before you call mark_keys_as_published. In such a case the same one time keys might be uploaded twice.

What's the risk of uploading the same one-time keys twice? Ideally, the identity service will just dedup.

If we preemptively mark the keys as published, I'm worried that we'll end up having these keys permanently stored in the Olm account. They'll never get used because nobody knows about them. This pattern could lead to unconstrained growth in the size of the Olm account.

Starting transactional db update inside another transactional db update looks a little bit suspicious to me. I think it would be better to create two independent promises one for content and another for notif account and let them run in parallel (await Promise.all). One thing that you will need to change is that in both of those promises you will call mark_keys_as_published. Then once both of the promises complete you will run the final rustApi.uploadOneTimeKeys.

I'd like to understand more about what you're worried about in the current design. It seems like the advantage of doing it in one transaction is that we make sure we don't "save" the new one-time keys until we've confirmed that the identity service RPC is successful.

You mean like?

// For brevity I left out Promise.all
 const { deviceID, otkKeys: contentOneTimeKeys } = await fetchCallUpdateOlmAccount('content', generate_one_time_keys_and_mark_as_published);
 const { otkKeys: notifOneTimeKeys } = await fetchCallUpdateOlmAccount('notifications', generate_one_time_keys_and_mark_as_published);

await rustAPI.uploadOneTimeKeys(
          identityInfo.userId,
          deviceID,
          identityInfo.accessToken,
          contentOneTimeKeys,
          notifOneTimeKeys,
        );

Not a big fan of this, as rustAPI.uploadOneTimeKeys is the most likely to fail. And mark_keys_as_published does have a cost to it filling up the published keys buffer.

It is actually safer to call mark_keys_as_published before they are actually uploaded since imagine a case when the keyserver crashes after rustAPI.uploadOneTimeKeys completes and before you call mark_keys_as_published

On the identity side, each one-time key is an unique item in DDB. So the keys should not be duplicated, but rather overwritten. The potential concern in the current "transaction within a transaction" is that if mark_keys_as_published() fails (or keyserver crashes after upload). There's a chance that those keys were minted to other clients by identity service; before keyserver tries to upload them again. In which case, two or more clients to receive the same one-time-key, which will cause the subsequent create_inbound_session calls by the keyserver to fail.

Likewise, if we do mark_keys_as_published before we upload. Then crashes to keyserver after generation of the keys, but before the upload of the keys, will cause unreachable keys to be put into the one-time-key buffer. If these keys start to overflow the 100 key buffer in Olm, then Olm will start dropping keys which were published. This creates a similar situation to above, where create_inbound_session calls by the keyserver will fail as the one-time-key issued by identity service won't exist in the keyserver's one-time-keys buffer.

Either way, it seems like we will need some way to handle "the one time key you used in this create_outbound_message is invalid, try again". Eventually the invalid keys should be pruned back to a healthy state.

In practice, I don't think this will be much of an issue. A lot of these scenarios require keyserver to be crashing which should be the exception.

My preference is to keep the transaction logic as it is now, because the failure states are a bit more "all or nothing", and the most likely to fail action of rustAPI.uploadOneTimeKeys can fail multiple times without causing the olm accounts getting into a bad state.

The logic of fetchCallUpdateOlmAccount is based on spinlock with limited amount time of runs (we don't lock the call to fetchCallUpdateOlmAccount if there is another one going on - we wait and retry later keeping limited amount of retries.). If we perform too much work inside one fetchCallUpdateOlmAccount we increase the chance that other calls to fetchCallUpdateOlmAccount will fail. This it the reason why I was concerned about making a bested fetchCallUpdateOlmAccount with network request since it looks like a costly operation.

However your comments express that waisting one time key is a bigger concern than increasing the chance of failure of fetchCallUpdateOlmAccount so I will not block this differential on this change.

However one thing that occurred to me: where are the corresponding calls to account.remove_one_time_keys?

same reason as here: https://phab.comm.dev/D7691#inline-58055 - I would prefer to fix this once across the entire tunnelbroker codebase

D9128

Can you create a follow-up task to consider removing this parameter if it's truly unused?

Sure, I was planning to do it: ENG-4928