Page MenuHomePhabricator

[keyserver] Make keyserver handle one-time key refresh requests sequentially
ClosedPublic

Authored by ashoat on Mar 24 2024, 7:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 1, 11:28 PM
Unknown Object (File)
Fri, Nov 1, 11:28 PM
Unknown Object (File)
Fri, Nov 1, 11:28 PM
Unknown Object (File)
Fri, Nov 1, 11:11 PM
Unknown Object (File)
Mon, Oct 14, 9:29 PM
Unknown Object (File)
Mon, Oct 14, 9:29 PM
Unknown Object (File)
Mon, Oct 14, 9:29 PM
Unknown Object (File)
Mon, Oct 14, 9:29 PM
Subscribers
None

Details

Summary

Currently, if the keyserver dequeues multiple one-time key refresh requests (from identity via Tunnelbroker) in rapid succession, it will process them in parallel.

This is bad because uploadNewOneTimeKeys relies on fetchUpdateOlmAccount, which is not architected to handle multiple parallel requests very well. Basically only one of the set of parallel requests will success per each round of fetchUpdateOlmAccount's retry. Since there are 5 rounds, this means only 5 will ever succeed, regardless of how many one-time key refresh requests are sent in parallel.

Even though these rounds don't succeed in persisting the new OTKs to the keyserver's local Olm account, they do succeed in being published to the identity service. This results in the identity service having a bunch of OTKs that are not in the keyserver's local Olm account, which leads to an olm_session_creation_failure when a client attempts to auth with that keyserver.

This diff addresses part of the issue by making one-time key refresh requests execute in sequence rather than in parallel.

Test Plan

I tested and made sure that one-time key requests were processed one-at-a-time after this change

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
keyserver/src/socket/tunnelbroker-socket.js
156–159 ↗(On Diff #38282)

Not sure if this is an issue, but this code might create an unbounded chain of promises (that's probably fine) because we never clear the promise (set this.oneTimeKeysPromise = null).

This revision is now accepted and ready to land.Mar 25 2024, 6:43 AM
keyserver/src/socket/tunnelbroker-socket.js
156–159 ↗(On Diff #38282)

I'm hoping it's not an issue... it would add a lot of complexity to this code if we need to reset this.oneTimeKeysPromise to null