Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate sync and project invites #362

Merged
merged 76 commits into from
Nov 28, 2023
Merged

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Oct 27, 2023

This PR integrates work done in #361, #360, #358 and #356 and fixes #233 (Add project method to download auth + config cores).

The manager.addProject() method now waits for the config and auth cores to sync before the project is added to the invitee's device, this ensures they have the presets and fields necessary to collect data, and they have the capability records that enable them to sync with others in the project who have not yet synced with the invitor. This does mean that a device could end up invited to the project as far as existing members are concerned, but that project not actually be added to the invitee's device. Have opened a separate issue #363 to address this situation later.

More broadly this PR also integrates earlier sync and discovery work by connecting invites, peer discovery and sync. This integration uncovered several bugs and implementation errors that have been resolved here. This PR also changes how sync is controlled - previously we used ReplicationStateMachine and the coreManager.replicate() method, but now we use the new SyncApi methods which authorises peers, starts auth, config and blobIndex sync automatically, and starts data and blob sync when the user calls project.$sync.start(). This change broke a lot of unit tests that relied on the core manager sync, so I have implemented a new coreManager.replicate() function for tests that replicates all cores in the project. I moved some unit tests to end-to-end tests (e.g. all the PeerSyncController unit tests are now covered in the e2e tests), but more could probably be changed to e2e tests in the future.

The full list of changes and fixes in this PR:

  • fix: include the creator capabilities in capabilities.getAll()
  • fix: track core key requests so that they are only sent once (when a core is added to the replication stream, connected peers will see its discovery key, but they need the core key to sync. They request the key for a given discovery key via an extension message, and the peer will send it if they verify the peer is a member of the project). The key requests are tracked so that if there is no response before a peer disconnects, the key will be re-requested next time it is seen.
  • start downloading cores on initialization. This is how Hypercore does it for non-sparse cores. Downloading is not peer specific - it is a state across all replicated peers.
  • fix: remove download methods from peerSyncController, because these act on all peer connections so now we just auto-download on initialization (we control downloading by selectively replicating cores).
  • chore: set infinite max listeners for many eventEmitters to avoid console warnings, because often every connected peer adds an event listener - this is not a memory leak like the warning suggests.
  • chore: normalized some CoreManager methods to return a CoreRecord (hypercore, key and namespace) instead of just a Hypercore.
  • chore: removed the previous replication method and the replication state machine from CoreManager, because this is now handled by the sync api.
  • fix: moved handleDiscoveryKey to PeerSyncController, because it needs to be aware of sync state to replicate cores that we already have, but are replicated by a peer who has just received them (edge-case when multiple peers are syncing)
  • chore: added a replicate function to CoreManager for unit tests - it automatically adds all cores in a project and starts replicating them (via coreStore.replicate).
  • fix: fixed a (major) bug where newly received cores were never added to the indexer, so synced data was never indexed.
  • feat: created LocalPeers and LocalDiscovery instances on MapeoManager and linked them so that newly connected peers are connected to LocalPeers.
  • feat: made the addProject() method wait for initial sync - only resolves and adds the project if the role record (authorizing project membership) and project config has synced (see new internal #waitForInitialSync() method)
  • feat: new methods to start and stop peer discovery - it did not seem to make sense to include these in start() and stop() methods that start and stop the server - the server should start and stop when the app goes into the background, which can be controlled by the backend, but discovery should probably be controlled by the user / frontend, so starting and stopping discovery needs to be methods that can be called from the frontend.
  • feat: always replicate the creator core with any locally discovered peers, but only attempt to replicate the rest of the project if the connected device can validate that they have the project key (this is validated when trying to replicate the creator core, so any peer that is emitted by peer-add from the creator core we know has the project key).
  • fix: only send the haves bitfield once the core is ready (otherwise the bitifeld can be empty)
  • feat: add dataToSync: boolean and coreCount: number properties to the sync state

For now this simplifies the API (because we are only supporting local
sync, not remote sync over the internet) to:

- `project.$sync.getState()`
- `project.$sync.start()`
- `project.$sync.stop()`
- Events
    - `sync-state`

It's currently not possible to stop local discovery, nor is it possible
to stop sync of the metadata namespaces (auth, config, blobIndex). The
start and stop methods stop the sync of the data and blob namespaces.

Fixes #134. Stacked on #360, #358 and #356.
Fixes Add project method to download auth + config cores #233

Rather than call this inside the `client.addProject()` method, instead I
think it is better for the API consumer to call
`project.$waitForInitialSync()` after adding a project, since this
allows the implementer to give user feedback about what is happening.
@gmaclennan gmaclennan self-assigned this Oct 27, 2023
@gmaclennan gmaclennan linked an issue Oct 27, 2023 that may be closed by this pull request
2 tasks
@gmaclennan gmaclennan changed the title feat: Add project.$waitForInitialSync() method feat: wait for initial sync when adding a project Oct 27, 2023
@gmaclennan gmaclennan marked this pull request as draft October 27, 2023 09:03
@gmaclennan
Copy link
Member Author

Failing tests need to be addressed

gmaclennan added a commit that referenced this pull request Nov 28, 2023
* main:
  fix: fix file storage race condition (#388)
  feat: remove project.ready() (#392)
  feat: integrate sync and project invites (#362)
gmaclennan added a commit that referenced this pull request Nov 28, 2023
* main:
  fix: wait for index idle before returning data (#389)
  fix: fix file storage race condition (#388)
  feat: remove project.ready() (#392)
  feat: integrate sync and project invites (#362)
gmaclennan added a commit that referenced this pull request Nov 29, 2023
* main:
  fix: wait for index idle before returning data (#389)
  fix: fix file storage race condition (#388)
  feat: remove project.ready() (#392)
  feat: integrate sync and project invites (#362)
gmaclennan added a commit that referenced this pull request Nov 29, 2023
* main:
  fix: wait for index idle before returning data (#389)
  fix: fix file storage race condition (#388)
  feat: remove project.ready() (#392)
  feat: integrate sync and project invites (#362)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write end-to-end tests for sync work Add project method to download auth + config cores
2 participants