Don't limit the DownloadDiagnosisKeysTask to once a day (EXPOSUREAPP-3771) #1616

ralfgehrer · 2020-11-16T13:41:39Z

Expected result:

Regular exposure checks are visible in the exposure log around every 4 hours (increased frequency for exposure checks)

Note for testing:

only time travel into the future, please.

…3771)

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/storage/TracingRepository.kt

chiljamgossow · 2020-11-16T14:07:35Z

I couldn't test on a device (do not have one yet)

harambasicluka · 2020-11-16T14:30:35Z

Will do the device testing for @chiljamgossow .

d4rken · 2020-11-16T14:38:16Z

...n-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/DownloadDiagnosisKeysTask.kt

        )
        return (LocalData.lastTimeDiagnosisKeysFromServerFetch() == null ||
-            currentDate.withTimeAtStartOfDay() != lastFetch.withTimeAtStartOfDay()).also {
+            currentDate.isAfter(lastFetch)).also {


What effect will this have? Won't this always be true, unless we have a sub-milliseconds racecondition (unlikely).
timeStamper.nowUTC will always be after LocalData.lastTimeDiagnosisKeysFromServerFetch().

i agree. what harm would it do if we just delete the complete condition?

I think it's already effectively removed, because currentDate.isAfter(lastFetch) will almost always return true.

Yes, my thinking too. The check solely protects us from someone time traveling backward.
IMHO all these checks should not be implemented by the Task but rather by the KeyPackageSyncTool.

Yes, I tested again and it seems it is always returning true.

Removed it entirely while ensuring that the task is only being run once every hour.

d4rken · 2020-11-16T14:40:06Z

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/storage/TracingRepository.kt

+        val keysWereNotAlreadyRetrieved =
            LocalData.lastTimeDiagnosisKeysFromServerFetch() == null ||
-                currentDate.withTimeAtStartOfDay() != lastFetch.withTimeAtStartOfDay()
+                currentDate.isAfter(lastFetch)


Same here, against what case of unwated execution does the check of currentDate.isAfter(lastFetch) protect us?

see above. Same check.

Implemented this differently now. Making sure the task is only run once every hour.

d4rken · 2020-11-16T14:50:56Z

It should be tested how often the task is now executed, especially when opening/closing the UI.

We should observe the webrequests in the log. Even if we run the task more often:

KeyPackageSyncTool should not make a network request unless new day or hour packages are expected (based on last data and current timestamp)
AppConfig will get called each time. This may still be an issue, because despite our current caching we will still load the app-config headers from the server to compare the ETag against the one we have cached.

After that, before submission we would evaluate these conditions and abort if called too often:

cwa-app-android/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/diagnosiskeys/download/DownloadDiagnosisKeysTask.kt

Lines 90 to 103 in 996121d

    
           if (exposureConfig.maxExposureDetectionsPerUTCDay == 0) { 
        
               Timber.tag(TAG).w("Exposure detections are disabled! maxExposureDetectionsPerUTCDay=0") 
        
               return object : Task.Result {} 
        
           } 
        
           if (wasLastDetectionPerformedRecently(now, exposureConfig, trackedExposureDetections)) { 
        
               // At most one detection every 6h 
        
               return object : Task.Result {} 
        
           } 
        
           if (hasRecentDetectionAndNoNewFiles(now, keySyncResult, trackedExposureDetections)) { 
        
               //  Last check was within 24h, and there are no new files. 
        
               return object : Task.Result {} 
        
           }

it seems D4rken is correct about the condition always returning true

harambasicluka

Could you please add some tests. Will let it run over the night and check the logs tomorrow.

harambasicluka · 2020-11-16T18:31:03Z

clean install, that's what I see directly after the onboarding

I only got the "Unknown Risk" after a force close.

ralfgehrer · 2020-11-16T22:23:19Z

clean install, that's what I see directly after the onboarding

I only got the "Unknown Risk" after a force close.

Fixed.

mlenkeit · 2020-11-17T10:08:00Z

Download looks better, I'll keep it running for some time.
But after I installed the client from this branch (over 1.7.0-RC3), the risk card changed from red to green and from 4 encounters to zero without updating the timestamp. Is that just another fix that's missing in this branch here?

See screenshot below:
right at 10:30 with 1.7.0-RC3 and left at 11:00 with this branch.

harambasicluka

Coding looks fine, ticket is solved, tested it over night, logs & exposure logs in os settings looks as expected. Also the initial wrong state is resolved.

d4rken · 2020-11-17T10:24:19Z

But after I installed the client from this branch (over 1.7.0-RC3), the risk card changed from red to green and from 4 encounters to zero without updating the timestamp. Is that just another fix that's missing in this branch here?

And it never changed back to the red card?

mlenkeit · 2020-11-17T10:25:59Z

But after I installed the client from this branch (over 1.7.0-RC3), the risk card changed from red to green and from 4 encounters to zero without updating the timestamp. Is that just another fix that's missing in this branch here?

And it never changed back to the red card?

I'll tell you in 30 minutes when the next Risk Calculation runs...

mlenkeit · 2020-11-17T11:06:05Z

But after I installed the client from this branch (over 1.7.0-RC3), the risk card changed from red to green and from 4 encounters to zero without updating the timestamp. Is that just another fix that's missing in this branch here?

And it never changed back to the red card?

It's a bit chaotic.

previous risk calculation was at 7:56, red with 4 encounters
next one should run no earlier than 11:56
at 11:05, I installed this branch and the card turned green with 0 encounters
at 11:58, I opened the app, nothing happened
at 12:01, I terminated the app, opened it again
client downloaded new hour package as expected, runs the risk calculation (which should have run at 11:58 already), returns to a red card with 4 encounters; timestamp did not update (still shows 07:56)

I have logs, videos, etc.

mlenkeit · 2020-11-17T11:14:58Z

And another navigation inside the app leads to:

ralfgehrer · 2020-11-17T11:42:02Z

But after I installed the client from this branch (over 1.7.0-RC3), the risk card changed from red to green and from 4 encounters to zero without updating the timestamp. Is that just another fix that's missing in this branch here?

And it never changed back to the red card?

It's a bit chaotic.

previous risk calculation was at 7:56, red with 4 encounters

next one should run no earlier than 11:56

at 11:05, I installed this branch and the card turned green with 0 encounters

at 11:58, I opened the app, nothing happened

at 12:01, I terminated the app, opened it again

client downloaded new hour package as expected, runs the risk calculation (which should have run at 11:58 already), returns to a red card with 4 encounters; timestamp did not update (still shows 07:56)

I have logs, videos, etc.

At 11:05 - the download task run - hence it won't run for another hour (with the current implementation) IF the app is not terminated (next "regular" download would be at 12:05)

At 12:01 - app was killed, download triggered by opening the app - next download should be expected at 13:01

To my understanding, the download task runs as expected BUT there seems to be some issue regarding the UI states and their update.

sonarqubecloud · 2020-11-17T15:24:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

0.0% Coverage
0.0% Duplication

Don't limit the DownloadDiagnosisKeysTask to once a day (EXPOSUREAPP-…

f5db423

…3771)

ralfgehrer added the maintainers Tag pull requests created by maintainers label Nov 16, 2020

ralfgehrer added this to the 1.7.0 milestone Nov 16, 2020

ralfgehrer requested a review from a team November 16, 2020 13:41

chiljamgossow self-assigned this Nov 16, 2020

chiljamgossow previously approved these changes Nov 16, 2020

View reviewed changes

Corona-Warn-App/src/main/java/de/rki/coronawarnapp/storage/TracingRepository.kt Show resolved Hide resolved

Oliver-Zimmerman self-assigned this Nov 16, 2020

harambasicluka added the prio PRs to review first. label Nov 16, 2020

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

996121d

harambasicluka self-assigned this Nov 16, 2020

Oliver-Zimmerman previously approved these changes Nov 16, 2020

View reviewed changes

d4rken self-requested a review November 16, 2020 14:38

d4rken suggested changes Nov 16, 2020

View reviewed changes

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

4e263dc

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

6b02bda

ralfgehrer added the do not merge label Nov 16, 2020

Start DownloadDiagnosisKeysTask only once every hour (EXPOSUREAPP-3771)

31fda22

ralfgehrer dismissed chiljamgossow’s stale review via 31fda22 November 16, 2020 18:12

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

e48a9fd

harambasicluka reviewed Nov 16, 2020

View reviewed changes

d4rken mentioned this pull request Nov 16, 2020

Refactor app config to source, check validity and improve edgecase handling (EXPOSUREAPP-3774,EXPOSUREAPP-3771) #1623

Merged

Fix initial state after fresh install (EXPOSUREAPP-3771)

321fd43

ralfgehrer removed the do not merge label Nov 16, 2020

ralfgehrer and others added 3 commits November 16, 2020 23:27

please ktlint and small fix

972f40b

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

261f052

Remove debug notification

eb71ca1

harambasicluka previously approved these changes Nov 17, 2020

View reviewed changes

Use task state instead of a dedicated retrieving status indicator

0481fa8

ralfgehrer dismissed harambasicluka’s stale review via 0481fa8 November 17, 2020 13:54

remove empty line

0b07742

d4rken approved these changes Nov 17, 2020

View reviewed changes

Merge branch 'release/1.7.x' into fix/3771-increased-exposue-check

944f57f

harambasicluka approved these changes Nov 17, 2020

View reviewed changes

harambasicluka merged commit 87a5f7a into release/1.7.x Nov 17, 2020

harambasicluka deleted the fix/3771-increased-exposue-check branch November 17, 2020 16:18

Don't limit the DownloadDiagnosisKeysTask to once a day (EXPOSUREAPP-3771) #1616

Don't limit the DownloadDiagnosisKeysTask to once a day (EXPOSUREAPP-3771) #1616

Uh oh!

Conversation

ralfgehrer commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

chiljamgossow commented Nov 16, 2020

Uh oh!

harambasicluka commented Nov 16, 2020

Uh oh!

d4rken Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-cwa Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

d4rken Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

ralfgehrer Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

Oliver-Zimmerman Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

ralfgehrer Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

d4rken Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

ralfgehrer Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

ralfgehrer Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d4rken commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harambasicluka left a comment

Choose a reason for hiding this comment

Uh oh!

harambasicluka commented Nov 16, 2020

Uh oh!

ralfgehrer commented Nov 16, 2020

Uh oh!

mlenkeit commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harambasicluka left a comment

Choose a reason for hiding this comment

Uh oh!

d4rken commented Nov 17, 2020

Uh oh!

mlenkeit commented Nov 17, 2020

Uh oh!

mlenkeit commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlenkeit commented Nov 17, 2020

Uh oh!

ralfgehrer commented Nov 17, 2020

Uh oh!

sonarqubecloud bot commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ralfgehrer commented Nov 16, 2020 •

edited

Loading

d4rken Nov 16, 2020 •

edited

Loading

ralfgehrer Nov 16, 2020 •

edited

Loading

d4rken commented Nov 16, 2020 •

edited

Loading

mlenkeit commented Nov 17, 2020 •

edited

Loading

mlenkeit commented Nov 17, 2020 •

edited

Loading