No matching performed if no new keys have been downloaded

[fynngodau]

Describe the bug

For every run of the DownloadDiagnosisKeysTask, the following steps are performed as per the code:

1. sync keys to local cache
2. abort if:
   • last check is too recent according to minimum detection interval
     (4 hours as per config, unlike this comment suggests) or
   • last successful check was less than 24 hours ago and no new keys have been downloaded during the sync in step 1
3. unless aborted, provide all cached diagnosis keys to GMS

This code appears to be faulty, because if new keys are already available in the app's cache during execution of this code, but no further new keys are available for download at the time of execution, then no keys will be handed over to GMS and one hour will be lost before the task is run again to downloads fresh diagnosis keys during its run, which it can then pass to GMS along with all other previously downloaded keys.

Steps to reproduce the issue

Suppose that, during a given hour in a day, an hourly package is not made available, or that the user is offline and therefore cannot retrieve it. Then consider the following diagram:

--- time ----------------------------->
hourly packages available |   # #   #
task executed             |  a a a b s

• a signifies a task that syncs keys but is then aborted because the most recent submission to GMS is too recent.
• b signifies a task that syncs keys, but finds no new ones and therefore aborts – even though keys that had not yet been submitted to GMS are available in cache.
• s signifies a task that successfully finds new keys and submits them to GMS.

The same condition could happen if a user opens the app after an hourly package is made available, but before the minimum interval is up.

Expected behaviour

The task should correctly detect new keys by checking for keys that had not been provided to GMS, not just for freshly downloaded keys.

Additional context

This bug was found by @Elsensee.

[mlenkeit]

@fynngodau thanks for bringing this up.

This was a deliberate decision at the time with the following reasoning:

The current approach gives a high probability that if two devices show the same or similar last updated timestamp on the home screen/tab, that the respective risk was calculated based on the same data (i.e. same set of keys that was submitted to GMS).

I'll try to illustrate what happens otherwise. Let's take your slightly modified example:

--- time ----------------------------->
hourly packages available |   1 2   3
task executed             |  a a a b s

If task b did submit keys to GMS, the semantics of the last updated timestamp becomes tricky:

• technically, the risk was last updated when task b ran. However, if does not give any indication as to what data set was used
• more correct would be something along the lines of last updated at <time of b> with data set as of <time of 2>
• it becomes even more complicated if you also consider the Event Check-In "warnings"

As such complex explanations are not really helpful to the user, we discarded the idea and stuck with the current behavior.

But we're open to reviewing this:

• do you have an idea of how many users are impacted by this? So far, our assumption is that this is rather an edge case.
• what's your take on the potentially misleading text implications if we kept the current last updated text?

[Elsensee]

do you have an idea of how many users are impacted by this? So far, our assumption is that this is rather an edge case.

The PeriodicWorkRequest guarantees that the task runs once per interval, which is 60 minutes. This interval doesn't have to be aligned with the "normal hour" so it's totally possible for it to happen at x:10 and x:50 if I'm correct. Both of these values are within an hour.
Two devices of which one has updated at 3:10pm and the other at 3:50pm will have used the same dataset anyway, so this distinction is not necessary. It only leads to a delay which gets even bigger when the device is then disconnected from (unmetered) wifi. In that case no new hour-packages will be downloaded, further delaying the update (since no new key files are available) while at least four hours of diagnosis keys lay around on the device, not asessed by ENF. (You are right though, in the case that an device gets disconnected from wifi during the 4 hour period, new key files would be on the device and an update could mean that the timestamps on two devices would differ from the datasets being used)

I have no idea how many users are actually impacted by this and how big the impact is considering that at least evey 24h there will be a risk calculation. But to max out the ENF given quota is probably desireable to warn as early as possible (It probably could be a telemetry data point to see how many risk calculations were performed to see how much deviation from 6 there is, but that's another topic I presume)