fix: Blocked members only expect auth core for initial sync #1138
Conversation
There was a problem hiding this comment.
This should be based on role.sync.config === 'blocked' rather than hard-coding this only to the BLOCKED_ROLE. You also need the check in onSyncState for syncState.config.dataToSync e.g.
if (syncState.auth.dataToSync || (syncState.config.dataToSync && role.sync.config === 'allowed'))
There was a problem hiding this comment.
One small change needed to check for isRoleSynced, but also this pointed out what I think is a bug in the NO_ROLE definition which is the likely cause of some race conditions you have been seeing in the tests (config cores being shared when they should not be) because the NO_ROLE allowing config sync is allowing that if there is a delay syncing the actual role. That can be addressed separately to this PR I think, but the isRoleSynced check here will be necessary once that is fixed.
src/mapeo-manager.js
Outdated
| // in the config store - defining the name of the project. | ||
| // TODO: Enforce adding a project name in the invite method | ||
| const isConfigSynced = configState.want === 0 && configState.have > 0 | ||
| if (ownRole.sync.config === 'blocked' && isAuthSynced) return true |
There was a problem hiding this comment.
I think we need to check isRoleSynced here too. However note that currently it looks like Roles.NO_ROLE allows sync of the config cores, which to me seems like a bug, and likely the cause of the race conditions you have been seeing - before any role is synced, the permissions of NO_ROLE is allowing sync of the config core to happen.
There was a problem hiding this comment.
Great catch! Should we maybe set NO_ROLE to disallow sync of the config core?
There was a problem hiding this comment.
Yes, I can't think of a reason why it would be necessary for NO_ROLE to sync the config core.
There was a problem hiding this comment.
deviceInfo is under the config, so maybe that's why?
2 participants
This cuts 30 seconds from the sync tests which were waiting for the blocked member to time out before ✌️
Also cleaned up the test for the case so that the error messages actually tell us which namespaces / cores have assert errors.
Also made sure waitForInitialSync was catching errors everywhere.
Taken from #1092
Closes #1067