In bug 1250531, we failed to take into account that we don't immediately re-upload the local record after applying the commands, so when we do the next Sync we re-apply the commands on the server. The end result is that commands are applied each sync - which is bad :( Kit and I have a fix for this that should be ready tomorrow.

Review commit: https://reviewboard.mozilla.org/r/44247/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44247/

https://reviewboard.mozilla.org/r/44247/#review40911 ::: services/sync/tests/unit/test_clients_engine.js:847 (Diff revision 1) > server.stop(run_next_test); > } > } > }); > > +add_test(function test_send_uri_ack() { Unfortunately, the preceding test is now failing for me locally, with an `NS_ERROR_CONNECTION_REFUSED`. I'm wondering if there's a bit of state we're not cleaning up somewhere, or the runs are stomping on one another.

Comment on attachment 8738008 [details] MozReview Request: Bug 1262021 - Ensure remote commands are applied once per sync. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44247/diff/1-2/

The most recent revision, which clobbers the cache in `test_receive_display_uri`, fixed the failure locally. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e40e965425c

Comment on attachment 8738008 [details] MozReview Request: Bug 1262021 - Ensure remote commands are applied once per sync. r?markh https://reviewboard.mozilla.org/r/44247/#review41093

Comment on attachment 8738008 [details] MozReview Request: Bug 1262021 - Ensure remote commands are applied once per sync. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44247/diff/2-3/

Comment on attachment 8738008 [details] MozReview Request: Bug 1262021 - Ensure remote commands are applied once per sync. r?markh Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44247/diff/3-4/

https://reviewboard.mozilla.org/r/44247/#review41143 ::: services/sync/modules/engines/clients.js:29 (Diff revision 4) > const CLIENTS_TTL_REFRESH = 604800; // 7 days > > const SUPPORTED_PROTOCOL_VERSIONS = ["1.1", "1.5"]; > > function hasDupeCommand(commands, action) { > + if (!commands) { For the case where there aren't any local or remote commands, if the recipient removed them. ::: services/sync/modules/engines/clients.js:512 (Diff revision 4) > - this.engine.localCommands = record.commands; > - else { > + }, > + > + _updateRemoteRecord(record) { > - let currentRecord = this._remoteClients[record.id]; > + let currentRecord = this._remoteClients[record.id]; > - if (currentRecord && currentRecord.commands) { > - // Merge commands. > + if (!currentRecord || !currentRecord.commands || > + !this.engine.isLocallyModified(record.id)) { This is the relevant change to avoid reuploading commands that a remote client has received and deleted. If we haven't modified it, we just accept the server's data as canonical.

Comment on attachment 8738008 [details] MozReview Request: Bug 1262021 - Ensure remote commands are applied once per sync. r?markh https://reviewboard.mozilla.org/r/44247/#review41203 ::: services/sync/modules/engines.js:1259 (Diff revision 4) > this._delete.ids = [id]; > else > this._delete.ids.push(id); > }, > > + isLocallyModified(id) { as discussed, let's keep inlining this given it is only true during a sync (othertimes we'd need to check the tracker) ::: services/sync/tests/unit/test_clients_engine.js:886 (Diff revision 4) > + args: ["https://example.com", fakeSenderID, "Yak Herders Anonymous"], > + }]; > + server.insertWBO("foo", "clients", new ServerWBO(engine.localID, encryptPayload(ourPayload), now)); > + > + _("Sync again"); > + engine.lastSync = now - 10; I think we should remove this given it's no longer needed (and if it *is* still needed, we are probably missing something else and need to dig deeper)