Sync Pitfall

[bausmeier]

As I've been going through the current implementation of server sync, and thinking about approaches to peer-to-peer sync, I've come across a potential pitfall which I think needs to be highlighted.

The current FHIR engine implementation tracks local changes using JSON Patches, and synchronises these with the server using the FHIR patch interaction.

I've written up the following scenario to try and illustrate the issue:

---

When starting with this example patient:

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "1"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "1111111111",
      "use": "home"
    },
    {
      "system": "phone",
      "value": "2222222222",
      "use": "work"
    },
    {
      "system": "email",
      "value": "[email protected]"
    }
  ]
}

If one client changes the patient's work phone number it results in the following patch:

[
  {
    "op": "replace",
    "path": "/telecom/1/value",
    "value": "3333333333"
  }
]

And if a different client removes the patient's home phone number it results in the following patch:

[
  {
    "op": "remove",
    "path": "/telecom/0"
  }
]

Then there are two possible outcomes depending on the order in which the patches are synced to the server or from peer to peer.

If they are synced in the order listed above, then the result is what we'd expect:

(The home phone number has been removed, and the work phone number has been updated.)

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "3"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "3333333333",
      "use": "work"
    },
    {
      "system": "email",
      "value": "[email protected]"
    }
  ]
}

But if they're synced in the opposite order, then we end up with an invalid result:

(The home phone number has been removed, but the value of the email address was updated instead of that of the work phone number.)

{
  "resourceType": "Patient",
  "id": "1",
  "meta": {
    "versionId": "3"
  },
  "telecom": [
    {
      "system": "phone",
      "value": "2222222222",
      "use": "work"
    },
    {
      "system": "email",
      "value": "3333333333"
    }
  ]
}

It does not matter when each of the changes was made, only the order in which they were synced.

---

To avoid ending up with resources in unexpected states, patches need to be applied conditionally based on the version of the resource which each client was making changes to. This is pointed out in the FHIR docs:

Processing PATCH operations may be very version sensitive. For this reason, servers SHALL support conditional PATCH, which works exactly the same as specified for update in Concurrency Management. Clients SHOULD always consider using version specific PATCH operations so that inappropriate actions are not executed.

Doing this, however, introduces the possibility of conflicts, and the need for conflict resolution. It also negates some of the benefits I'd perceived in using PATCH over PUT.

I've read through the discussion in #472 relating to conflicts where the point was made that conflicts are rare and can be dealt with operationally. I agree with the decision there, but as pointed out above - without special consideration by implementers, the current API allows for implementations which can result in data corruption rather than conflicts. At the moment neither the reference app nor the other implementations which I've looked at take this behaviour into consideration. So I see this as a bit of a footgun, and I'm wondering if there's something which can be done to guide implementers into the pit of success.

Is there a good place to document this? And should the reference app be updated to apply optimistic locking as a good practice?

Any other thoughts are welcome too since this is something which needs to be considered in the implementation of P2P sync.

[bausmeier]

I'm thinking that we'd need to add a versionId argument to the DataSource.update function signature in order to support conditional updates. There doesn't seem to be any handling of version ids at the moment though, and I haven't looked into what else would be required to implement that. It would also be a breaking change for current users of the SDK.

[bausmeier]

Going through the FHIR HTTP docs again to confirm what I said on the call, I see that:

The create interaction specifies:

If the request body includes a meta, the server SHALL ignore the existing versionId and lastUpdated values. The server SHALL populate the id, meta.versionId and meta.lastUpdated with the new correct values.

The server returns a 201 Created HTTP status code, and SHALL also return a Location header which contains the new Logical Id and Version Id of the created resource version:

Location: [base]/[type]/[id]/_history/[vid]

where [id] and [vid] are the newly created id and version id for the resource version. The Location header should be as specific as possible - if the server understands versioning, the version is included.

The update interaction specifies:

If the server supports versions, it SHALL populate the meta.versionId and meta.lastUpdated with the new correct values.

If the interaction is successful, the server SHALL return either a 200 OK HTTP status code if the resource was updated, or a 201 Created status code if the resource was created (or brought back to life/re-created), with a Last-Modified header, and an ETag header which contains the new versionId of the resource.

And the patch interaction is processed in the same was as the update interaction.

So as long as the server supports versions, my suggestion above should work by allowing the client to provide the id of the version of the resource which is being patched in the If-Match HTTP header to prevent invalid or lost updates as per https://hl7.org/fhir/http.html#concurrency. If the server doesn't support versions, then we're back to where we currently are.

[bausmeier]

In terms of an implementation the questions that I have are:

1. Where would we store the versionId of the resource? Do we add it to ResourceEntity alongside resourceId? Or do we try and take it from the meta.versionId of the resource itself?
2. How would the implementations of the DataSource.insert and DataSource.update methods pass the new versionId that they get from the server back to the FHIR engine so that it can be stored?

[bausmeier]

@jingtang10 I added more of my thinking around a potential solution above. Hope that helps. I'd be happy to take a stab at implementing a solution, but I've got other priorities at the moment so that would have to wait a while.

[jingtang10]

Thanks so much Brett for raising this. This is a critical issue we need to resolve as a priority.

Your examples highlight a very specific problem. But I think this problem is actually more genenral: any conflict (even just two patches editing the same field) would result in the same kind of problem.

We originally wanted to implement patch because in a way it almost has a built-in conflict resolution mechanism (two clients sending two patches updating two different fields of the same resource would not actually generate any conflict, the conflict is "automatically" resolved, but two clients sending two updates would for sure cause conflicts on the server). But what you have highlighted here is that there will be cases where this "auto resolution" causes unforeseen problems (like in your example, you end up updating a different item in a repeated field if someone else changes the index of the item...)

@aditya-07 do you have anything to add?

[bausmeier]

Thanks for the response and background you provided. I'd imagined that the "built-in conflict resolution" was the reason that the patch interaction was chosen in the first place - as you've pointed out.

There are cases when multiple patches can be applied safely, but especially when dealing with clinical data I see the patch operation as a risky default without also applying optimistic locking. An implementer who is sure that the changes that they are applying are safe, and that resources on the server will not be modified in unsafe ways by any other apps or services, could still take advantage of this approach, but in general I'd say this is not the case so it should be opt-in rather than opt-out.

Unfortunately if optimistic locking is applied it means that even unrelated changes to the same resource will result in conflicts and a large benefit of using patches is lost. The only remaining benefit of using patch over update that I then see is that the request body size will often be smaller when sending a patch than when sending the whole updated resource. And in the case of server sync this is probably still an important benefit.

[DiegoOLopez]

To avoid data corruption in peer-to-peer sync, it's important to apply patches conditionally based on the resource version. This can be done by using version-specific PATCH operations, preventing inappropriate actions. It would also be beneficial to implement optimistic locking to prevent accidental overwrites. Conflicts are rare, but conflict resolution mechanisms should still be included. Documenting these practices and updating the reference app to include them would help avoid these problems.