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

Users might start upwards replication from 0 after upgrade #6920

Closed
dianabarsan opened this issue Feb 11, 2021 · 7 comments
Closed

Users might start upwards replication from 0 after upgrade #6920

dianabarsan opened this issue Feb 11, 2021 · 7 comments
Assignees
Labels
Priority: 1 - High Blocks the next release. Type: Bug Fix something that isn't working as intended

Comments

@dianabarsan
Copy link
Member

dianabarsan commented Feb 11, 2021

Describe the bug
Due to PouchDB internal algorithms and non-deterministic minification results (either from Uglify-js used pre 3.11 or Terser used post 3.11), users could restart upwards replication from 0 after an upgrade.

To Reproduce
Reproducing is pretty tricky as it involves changing some code in way that will change certain variable names in the minified source. I've been pretty successfull in unwillingly triggering this in the migration branch. I'll add a comment if I find an 100% reliable way, but for now:

  1. Generate a production build.
  2. Login as an offline user in a browser. Fully replicate, leave the browser tab open, watching the network.
  3. Make some code changes, but don't touch the filter function. Build again. Hopefully your changes will shuffle variable names in the minified source.
  4. Sync with the offline user. Reload.
  5. Watching the network tab, sync.
  6. See _revs_diff requests (the batch size is 100, so if your user has 500 docs, you should see 5 _revs_diff requests).

Expected behavior
Users should not start upwards replication from 0. Seeing one _revs_diff request is normal - and should contain references to the latest docs that the user has downloaded from the server: probably only the service-worker-cache.

Environment

  • App: webapp
  • Version: since this depends on internal workings of external libraries (PouchDB & Uglify-js/Terser), it's impossible to tell how long this has been going on without inspecting each release's minified JS and see how the filter function looked like. If between two releases there'd be changes (even a single character), then the bug would occur when upgrading.

Additional context
PouchDB generates a (hopefully) unique and stable replication ID for each replication, and uses some of the replication config options into compiling it.
https://github.com/pouchdb/pouchdb/blob/1db1700382604ab0dc3ddb337096d99641ea12d8/packages/node_modules/pouchdb-generate-replication-id/src/index.js#L15
One of the config options that is used to generate a replication id is the provided filter function. PouchDb calls the provided value (be it a function, or a string) toString method to get a value to use.
The result of the toString call is not deterministic, because the actual code that runs is minified for production use.
So, if between two versions, the minified string representation of the filter function changes, even if no changes have been made to the function itself, the newly generated replication ids will be different from the existent and cause a restart in replication.

Fortunately, upwards replication is relatively cheap, as it involves hitting an unfiltered CouchDB endpoint (_revs_diff) with every pair of doc _id + _rev that exists on the user's device. For the overwhelming majority of cases, responses from the server will be empty (because the docs already exist on the server as well).

@dianabarsan dianabarsan added Type: Bug Fix something that isn't working as intended Priority: 1 - High Blocks the next release. labels Feb 11, 2021
@dianabarsan dianabarsan self-assigned this Feb 11, 2021
latin-panda pushed a commit that referenced this issue Feb 11, 2021
)

Related ticket: #6920

On this commit:
Given that Terser doesn't provide a way to select pieces of code that it should not minify and any other solution was substantially more complex or risky, overloading toString seems like the lesser of most evils.

Adds e2e test to check the filter function for an offline user.
Fixes some problems in other e2e tests
@dianabarsan
Copy link
Member Author

The fix has been merged into the angular migration branch.

dianabarsan added a commit that referenced this issue Feb 24, 2021
)

Related ticket: #6920

On this commit:
Given that Terser doesn't provide a way to select pieces of code that it should not minify and any other solution was substantially more complex or risky, overloading toString seems like the lesser of most evils.

Adds e2e test to check the filter function for an offline user.
Fixes some problems in other e2e tests
dianabarsan added a commit that referenced this issue Feb 25, 2021
)

Related ticket: #6920

On this commit:
Given that Terser doesn't provide a way to select pieces of code that it should not minify and any other solution was substantially more complex or risky, overloading toString seems like the lesser of most evils.

Adds e2e test to check the filter function for an offline user.
Fixes some problems in other e2e tests
@newtewt newtewt self-assigned this Mar 4, 2021
@newtewt
Copy link
Contributor

newtewt commented Mar 4, 2021

@dianabarsan I have some questions before starting. The repro steps are for a dev env. I am looking to recreate this in our test environment. To understand it a bit better. If the filter function was modified. Would that cause the user to start over? In the case of testing in medic-os. I can use different braches but if the statement about modifying is true will I need to be sure that the random branch I upgrade from didn't have a change to the filter function?

@dianabarsan
Copy link
Member Author

dianabarsan commented Mar 4, 2021

When you upgrade from 3.10 to 3.11, it will happen, even with this fix (because the system will compare the old filter function with the new, and they'll be different).

In 3.10 and prior, editing the filter function would most definitely trigger a re-sync. We had control over that (and could avoid editing it), but the act editing other code could change the minified version of the filter function, without us having any control over it.

From 3.11 onward, even editing the filter function should not trigger a re-sync.

@newtewt
Copy link
Contributor

newtewt commented Mar 4, 2021

I see, my thought then is to create a branch off master. Then upgrade from master to the newly created branch. It shouldn't re-sync since it has the latest changes.

@dianabarsan
Copy link
Member Author

Cool!

@newtewt
Copy link
Contributor

newtewt commented Mar 4, 2021

I followed the upgrade as described above. Watching the network tab on my offline user. I see two _revs_diff calls after the upgrade and sync. Both only have 7 docs in the request payload for feedback. One has no response the other has the seven feedback docs.

Is there a header that sets the batch size? I don't see it is why I'm asking.

@newtewt
Copy link
Contributor

newtewt commented Mar 4, 2021

In our slack disucssion, this was expected to happen since the meta db needed to sync. The Medic db didn't have a revs_diff call to update. I'm closing this as it is already merged.

@newtewt newtewt closed this as completed Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 - High Blocks the next release. Type: Bug Fix something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

2 participants