[BACK-2866] Fix Dexcom task failing for newly created Dexcom connections #697
Conversation
darinkrauss
commented
Feb 7, 2024
•
darinkrauss
commented
- Fix Dexcom task failing for newly created Dexcom connections
- Only set CreatedUserID in UsersDataSetsCreate if user AuthDetails
- Always add error to Dexcom tasks whether or not task should fail
- Doubled average period between Dexcom task executions from 60 minutes to 120 minutes.
- Doubled task duration to account for a couple of long running errors observed in logs.
- Update tests
- Update .gitignore and .dockerignore
- https://tidepool.atlassian.net/browse/BACK-2866
- Fix Dexcom task failing for newly created Dexcom connections - Only set CreatedUserID in UsersDataSetsCreate if user AuthDetails - Always add error to Dexcom tasks whether or not task should fail - Update tests - Update .gitignore and .dockerignore - https://tidepool.atlassian.net/browse/BACK-2866
| @@ -51,7 +51,7 @@ func FailTask(l log.Logger, t *task.Task, err error) error { | |||
| return err | |||
| } | |||
|
|
|||
| func shouldTaskError(t *task.Task) bool { | |||
| func shouldTaskFail(t *task.Task) bool { | |||
There was a problem hiding this comment.
I don't think this function handles correctly the case where the t.Data[dexcomTaskRetryField] is not set. When t.Data[dexcomTaskRetryField] is nil, this function should return false instead of true, so the retry counter can be set to 1.
There was a problem hiding this comment.
Yes, that is correct and I intentionally did not fix that yet.
If we make that fix right now, then every Dexcom task that hits a 429 will start down the "retry path" and, after retrying 3 times, will be marked as failed. I really don't want that to happen quite yet. I need to first straighten out what errors cause a formal retry (where the retry counter is incremented; e.g. Dexcom auth errors), what errors retry for "free" (no increment; e.g. Dexcom 429s), and what errors fail immediately (e.g bad data). Furthermore, I need to add code that resets the retry counter on success (as there is no code to do that currently).
| @@ -69,7 +69,9 @@ func UsersDataSetsCreate(dataServiceContext dataService.Context) { | |||
| } | |||
|
|
|||
| dataSet.SetUserID(&targetUserID) | |||
| dataSet.SetCreatedUserID(pointer.FromString(details.UserID())) | |||
| if details.IsUser() { | |||
| dataSet.SetCreatedUserID(pointer.FromString(details.UserID())) | |||
There was a problem hiding this comment.
FYI, details.UserID will be set to the id of the backend client service account, which is a real user in Keycloak.
There was a problem hiding this comment.
Hmm, that's not what I'm seeing. I'm getting an empty string which causes a failure during later validation (CreatedUserID of nil is fine, but an empty string is not).
There was a problem hiding this comment.
I'll double check.
There was a problem hiding this comment.
While the service session token does include the UserID of the backend client service account, it is not being used because the session token is marked as IsServer. Find the relevant code that ignores the UserID for a server session token at
platform/auth/client/external.go
Line 208 in f797630
| if details.IsUser() { | ||
| dataSet.SetCreatedUserID(pointer.FromString(details.UserID())) | ||
| } |
There was a problem hiding this comment.
Would it be simpler, given the validation failure, to do:
| if details.IsUser() { | |
| dataSet.SetCreatedUserID(pointer.FromString(details.UserID())) | |
| } | |
| if userID := details.UserID(); userID != "" { | |
| dataSet.SetCreatedUserID(pointer.FromString(userID)) | |
| } |
There was a problem hiding this comment.
Per discussions in Slack, will leave as-is. Using the system-generated user ID for service session tokens will cause downstream issues. Plus, existing platform code that references the UserID from AuthDetails checks IsUser beforehand.
- Doubled average period between Dexcom task executions from 60 minutes to 120 minutes - Doubled task duration to account for a couple of long running errors observed in logs - Minor changes for local debugging
darinkrauss
force-pushed
the
BACK-2866-fix-dexcom-task-issues.1
branch
from
9f67aa8 to
2a425fc
Compare
|
Merging this on behalf of Darin, since this is a critical update, and Darin is OOO. Squashing as I believe that's what Darin said he normally does. Will combine commit messages in a way that makes as much sense as possible. |
