UTD Hook | UTD will never be reported if the timeline controller is dropped before the max_delay

[BillCarsonFr]

When a UTD is reported, the hook manager will spawn a task to be executed after max_delay:

matrix-rust-sdk/crates/matrix-sdk-ui/src/unable_to_decrypt_hook.rs

Lines 233 to 236 in 9b6de4e

	let handle = spawn(async move {
	// Wait for the given delay.
	sleep(max_delay).await;

However, if the hook is dropped before the expiration of this timer the UTD will never be reported:

matrix-rust-sdk/crates/matrix-sdk-ui/src/unable_to_decrypt_hook.rs

Lines 315 to 318 in 9b6de4e

	impl Drop for UtdHookManager {
	fn drop(&mut self) {
	// Cancel all the outstanding delayed tasks to report UTDs.
	//

The current default for that delay is 60s. This particularly affects clients like EXI that don't keep timelines around; EXA has a LRU cache of the last 16 opened timelines.

Some notes:

• We would want to keep the reporting task around (surviving the hook lifetime), but then we might not detect late decryption anymore because this is done by the timeline controller
• Maybe we should move that code to the event cache
• As a stop gap we could reduce the max delay to around 10s? would reduce the chances of having it dropped

[bnjbvr]

Thanks for opening an issue. Since we've also discussed that in private: IMO it would be fine to NOT drop the pending tasks in the Drop impl of the UtdHookManager, since they're bound to be relatively short-lived anyways (at most, for the grace period duration, which we can assume is a reasonable choice by the end users of this API).

[BillCarsonFr]

Thanks for opening an issue. Since we've also discussed that in private: IMO it would be fine to NOT drop the pending tasks in the Drop impl of the UtdHookManager, since they're bound to be relatively short-lived anyways (at most, for the grace period duration, which we can assume is a reasonable choice by the end users of this API).

The problem is that in that case there will be no attempt to retry decryption in case of late keys (or from backup)

[richvdh]

The problem is that in that case there will be no attempt to retry decryption in case of late keys (or from backup)

The UtdHookManager isn't responsible for retrying decryption anyway, is it?

[BillCarsonFr]

The problem is that in that case there will be no attempt to retry decryption in case of late keys (or from backup)

The UtdHookManager isn't responsible for retrying decryption anyway, is it?

No that's right. It is the timeline controller doing it at the moment.
The UtdHookManager will never have this responsability, but it should move out from the the timeline controller to the event cache.

[richvdh]

So to be clear: the problem with Benji's suggestion is not so much that there will be no attempt to retry decryption (that is the case anyway). The problem is that, given there will be no attempt to retry decryption, then dropping the pending tasks will mean we report the UTD anyway, even if we could have decrypted the event.

[bnjbvr]

For what it's worth, this is another good reason to move the retry-decryption + UTD-reporting logic over to the event cache, since it would make this problem nonexistent (the event cache's lifetime is the entire app's lifetime, after you've subscribed to it).