Take RemoteStore offline during user change #3193
Conversation
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/credentialchange
branch
from
115a17e to
00c9136
Compare
| logDebug(LOG_TAG, 'RemoteStore restarting streams for new credential'); | ||
| await this.restartNetwork(); | ||
|
|
||
| this.networkEnabled = false; |
There was a problem hiding this comment.
This makes me uneasy. Previously networkEnabled was only used to signal user intent (around the enableNetwork and disableNetwork calls). Reusing this here means that a public call to enableNetwork can disrupt this logic. I don't think this is desirable, but perhaps that's something you intended?
There was a problem hiding this comment.
This matches the code in restartNetwork() and is needed to silence the asserts that would trigger if the network stream closes without an error.
There was a problem hiding this comment.
I understand that this is needed to silence assertions in the current implementation, but the larger point I'm after here is that this potentially doesn't actually work.
Consider this sequence of events:
- There's a credential change
- IndexedDB fails during
syncEngine.handleUserChange, leading that block to be retried later - The user calls
enableNetwork, disabling the guard rails in this implementation - The user performs a listen which may successfully start a stream with the new identity before we've completed the transition within the sync engine.
Or as an alternative to the last step, the device encounters a network transition that prompts restartNetwork to run, again breaking the assumption in here that we won't allow the network to restart until the credential change transition has completed.
What I'm after is that by mixing the intent of the networkEnabled signal between what the user asked for (enableNetwork) and what's possible (IndexedDB is failing, so the streams should be shut down), we open ourselves up to having to reason about these circumstances.
To be clear: the use of networkEnabled in restartNetwork is also suspicious, but it's less immediately problematic because the interval where it's being manipulated is so short. While we're in the background and IndexedDB is failing it could be hours before the sync engine succeeds, which suggests we need a better way to handle this. When this was the only abuse of the user signal this seemed like an ~OK hack. Now that we've got multiple kinds of things potentially interacting with it, the negative effect has multiplied.
So, unless I'm grossly misunderstanding what's happening here, I think I'm proposing we:
- only track user intent in
networkEnabled - track network-transition-induced disablement separately from
networkEnabled - track identity-change-induced disablement separately from both
- only allow network access when all conditions are independently satisfied.
- adjust the assertions to account for the fact that there can be multiple reasons why we might disable network access, not just that the user requested it.
This way these events can happen all at once or in any order and we won't accidentally enable the network during an event ordering we didn't anticipate.
Another possible alternative, rather than tracking each condition independently, is to track a cumulative failure counter. I think you still need to track networkEnabled separately because user intent has to be idempotent, but you could make each of the three cases (IndexedDB failed, restart in progress, credential change in progress) increment a count on the initial condition and decrement once it's resolved. Only once all network-preventing actions have completed would we be able to start the stream.
I haven't fully thought this through, but this also seems to help with the case where multiple events of the same type happen before the first event is resolved. For example: if multiple credential change events happen before the syncEngine recovers, we won't start to enable the network after the first completes because the second in-flight credential change prevents it.
There was a problem hiding this comment.
Thanks for the long insightful comment! I am convinced that re-using this flag was not the right idea. While we could go with a counter, I have a feeling that it will go out of sync at some point, and we will not be able to figure out why. In the end, I used something similar to what the realtime database does:
(but instead of strings I used enum constants)As for having multiple user changes, since they are schedule via the retryable operations, only one user change can be active at a time.
schmidt-sebastian
force-pushed
the
mrschmidt/credentialchange
branch
from
97ec8e9 to
fb9f23b
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/credentialchange
branch
2 times, most recently
from
3b34952 to
a1faa32
Compare
| @@ -860,10 +860,12 @@ export class SyncEngine implements RemoteSyncer { | |||
| ); | |||
| } | |||
|
|
|||
| async handleCredentialChange(user: User): Promise<void> { | |||
| async handleUserChange(user: User): Promise<void> { | |||
There was a problem hiding this comment.
There are still plenty of other references to handleCredentialChange and CredentialChangeListener. It seems preferable to keep these consistent, no? That way you can search for credentialchange and find all the places we do something about this.
There was a problem hiding this comment.
I renamed the RemoteStore and SyncEngine method to handleCredentialChange. LocalStore and SharedClientState still use handleUserChange since SyncEngine "swallows" no-op user changes.
This changes the user change to be much more explicit:
Replaces #3192
Fixes #3179