Race condition with OrderProposal feed

[nickevansuk]

There is currently a race condition in the specification as follows:

• OrderProposal is created
• OrderProposal is approved
  (Orders feed updated with OrderProposal)
• OrderProposal is booked, and creates a new Order
  (OrderProposal is deleted from orders feed)
• Order is cancelled
  (Orders feed updated with Order)

The reason for the previous OrderProposal is set to "state": "deleted" in the Orders feed at B is to prevent a race condition where B is also updated in the Orders feed at the same time as it is updated by the response from B.

In order to create one Orders feed, the same database table must be used to represent both OrderProposal and Order:

• Orders use a broker supplied UUID as their primary key
• This means that storing the OrderProposal and Order in a table requires either (a) the primary key to be a compound key of "Order type" and "UUID" or (b) the same row in the table to be reused for both.
• (b) is chosen, there's a risk that OrderProposal deletions are not picked up upon immediate cancellation, reusing the same UUID risks creating issues for downstream brokers storing both Orders and OrderProposals.

This is potentially resolved by simply allowing the Order to overwrite the OrderProposal in the RPDE feed if it has the same UUID. However, this requires Brokers to treat an Order as replacement to an OrderProposal with the same UUID, which is a "hack" on the abstraction, and is likely to create problems as the specification evolves.

This also creates implementation challenges for systems that store the Order and OrderProposal in different tables, as they need to be combined into one table to power an single RPDE feed.

The cleanest solution to this is likely to create a new feed: /order-proposals-feed which contains only OrderProposal.

Advantages:

• Creates flexibility for implementing systems, which can power both feeds from a single Orders table, or from two different tables, depending on their internal architecture.
• Removes issues around race conditions for (b) above
• Removes requirement for implementing a compound key for every implementation
• Clearly maintains the semantics of a "deleted item" in the feeds - as it's clear that either an "Order" or "OrderProposal" is being deleted depending on the feed
• Ensures the @id namespaces of the Order and OrderProposal are also separated into different feeds, which ensures a pure conceptual separation between the two types.

Disadvantages:

• Requires more polling in production

[lukehesluke]

@nickevansuk and I discussed this in a call and concluded that the OrderProposal feed approach was likely the most appropriate

Proposal

New RPDE feed: /order-proposals-rpde, in which all OrderProposal updates appear. This includes OrderProposals which have been accepted by the Seller (orderProposalStatus=https://openactive.io/SellerAccepted) and deleted OrderProposals which have been deleted due to calling B and everything else.

OrderProposals MUST no longer be in the Orders Feed (/orders-rpde)

Rationale

Following the logic of RPDE, it doesn't make sense to have two separate kinds of items in the same RPDE feed. OrderProposals and Orders have different types and different IDs - they represent different objects. An OrderProposal is an "Order under consideration" and an Order is a "Booked Order". One ends where the other starts.

So, could we instead just say that an OrderProposal is an Order at an earlier state? e.g. an Order with orderStatus=OrderProposal

If we did this, then an "Order" would be created in the feed when an asynchronous action happens to a proposed Order (e.g. Seller Approval). When the Broker calls B on this Order, what happens to it in the feed? Does it stay in the feed or get deleted?

The Booking System doesn't produce Orders in the feed at every B call for 2 reasons:

1. So that each action has exactly one consequence - either an object appearning in an RPDE feed or an object being returned. This ensures that a Broker only needs to do one or the other and will not incur potential race conditions.
2. Reduce the amount of traffic in the Orders feeds. We want to keep this low so that vital information like cancellations & access passes can be processed quickly.

So either we (a). change the orderStatus=OrderProposal Order in the feed to orderStatus=BookedOrder and have this B call not return anything or we (b) Delete the Order in the feed, which may be confusing and may itself lead to race conditions.

So how about we do a). so that B-after-P just returns HTTP 200 with no data?

Perhaps we would make this a different endpoint e.g. "C" as it now has a significantly different contract to "B". This would still lead to increased traffic in Orders feeds for Booking Systems which only support approvals.

[nathansalter]

Not sure I agree with this. The modelling in the library is very clear that OrderProposal is a subtype of Order as seen here. Indeed the Order can be seen as a single entity which transitions through several states as it progresses through the booking process (OrderQuote -> OrderProposal -> Order). As the same ID is used throughout the process when transitioning the state, it makes perfect sense as a broker to simply update the type of the Proposal to Order when it appears in the feed.

Following the logic of RPDE, it doesn't make sense to have two separate kinds of items in the same RPDE feed

Definitely don't agree with this. Each item has a type property, so it seems obvious to me that this is intentional. You can have both FacilityUse and IndividualFacilityUse in the same feed, so why not Orders and Proposals?

Although it's theoretically possible for the race condition to exist, this again is only when the Order and Proposal are stored as separate objects in your data store. If you're crawling the feed, you will find the Proposal in the feed, update the Order, and then call the next page.

[nickevansuk]

Hey @nathansalter the issue is that the current spec has the OrderProposal and Order in the same feed as separate items, but they are often driven from the same item in the data store:

[At B] The Broker stores the response to B to completely replace their previously stored OrderProposal, and the previous OrderProposal is set to "state": "deleted" in the Orders feed.

To achieve this the booking system needs to have two items in their data store: one for the OrderProposal and one for the Order. This creates an artificial limitation on the architecture of the booking system.

Updating the type of the OrderProposal to Order at B creates a race condition and violates the principles of data flow in the spec: if a Broker is processing the feed it must not receive any updates that are a direct response to an action it performed, which is the reason for this:

The OrderProposal from P must not be present in the Orders feed until it is updated at least once.

If there's an update coming from a feed, that update originated from the Booking System, if there's a response from B or P that update originated from the Broker.

Allowing feed updates that are triggered by the Broker (and hence may conflict with the B/P response) both increases feed update volume and creates potential for race conditions and implementation issues...

[nathansalter]

In this case, wouldn't it make more sense to drop the requirement to delete the OrderProposal? So that the seller is no longer required to keep separate items in the data store. Also as a page signifies a point in time, you could never have a Proposal and an Order in the same page, because the Order cannot be in both states at the same time.

if a Broker is processing the feed it must not receive any updates that are a direct response to an action it performed

But this is a requirement in the spec? After B, you expect to see your created Order in the orders feed to guarantee that it's been placed correctly. So it should appear in a subsequent page.

I think I misread this section when building our Orders feed (Schools plus also do the same, so I think it must be a common misconception)

this list must include all, and only, Orders and OrderProposals that have been updated by the Booking System subsequent to successful completion of B or P, respectively

Can be read as expecting Orders and Proposals to be in the feed after being placed/completed.

I'm also not sure I agree with not just always returning the Order in the orders feed, as you have no idea what state the seller has their Order objects in if they've not been updated. It also breaks the RPDE spec, as you can no longer reset your feed to get up to date with the system. Assuming you have some data loss it seems reasonable to me to reset your feed to pull all your Orders back, but you're now missing any Order which has not been updated.

[nickevansuk]

Great point on spec clarity, can see how this can be misread! Suggest this should be updated for clarity.

as you can no longer reset your feed to get up to date with the system

Ah so to simplify implementation for the Booking System, the Orders Feed is already specified as a partial representation of the overall Order:

The Order/OrderProposal properties included in the Orders feed must be a subset of the Order/OrderProposal properties returned in the response from B or P as specified below (effectively making the Orders feed a series of partial update requests for the Broker).

So as it stands there's no way to get a full view of Orders following data loss on the Broker side (so the Broker should treat their Orders as if they were their own). Although the Orders feed in theory could be specified to include all the data returned by B/P, this would only be to support full data restore scenario. Given the Broker is already responsible for Payments and other crucial data not stored in the Booking System, even a "full restore" from feed would not be sufficient to restore the Broker state. Hence if it is not possible to fully restore the Broker state, the value in allowing what would be a partial data restore from the Orders feed is greatly diminished.

as you have no idea what state the seller has their Order objects in if they've not been updated

The response from P/B provides the latest state of the objects? Sorry if I'm not following here.

[nathansalter]

The response from P/B provides the latest state of the objects? Sorry if I'm not following here.

Yes I misworded this, your confusion is correct 😆

What I mean is as the Seller you don't know the state of the Order in the Broker's system. The Broker has sent the Order to the Seller, but may have made other changes since which have not yet been transmitted to the Seller. I think we're coming from different positions on this. I think the Seller should 'own' the Order, but it looks like the implication is that the Broker owns it instead?

Filtering out the Orders which have not yet been updated requires a property added to the Order in the Seller's data store such as updatedByBroker which holds a boolean flag set to false by default. This seems an odd requirement, to have a field in the database which is only ever used once then is completely irrelevant.

[nickevansuk]

No worries :) Ok so: https://openactive.io/open-booking-api/EditorsDraft/1.0CR3/#roles-and-responsibilities-overview

In terms of 'ownership'... the Booking System is the system of record for the Order ("Order" as defined by the spec, which excludes custom attributes that might have been added by the Broker), however the Broker is the only organisation with full visibility of everything related to a particular "Order", and may associate additional custom broker-side attributes to it for e.g. Payment and Invoicing. If the Broker updated their representation of the Order (excluding custom broker-side attributes) but did not update the Booking System, that would create an inconsistency (not sure what use cases there might be for this?).

Depending on what we're meaning by 'ownership' and whether we're talking about the Order itself, or the broader "Order" that includes all associated attributes, i'm not clear how the Booking System could fully 'own' something they only have partial visibility of?

Filtering out the Orders which have not yet been updated requires a property added to the Order in the Seller's data store such as updatedByBroker which holds a boolean flag set to false by default. This seems an odd requirement, to have a field in the database which is only ever used once then is completely irrelevant.

Note sure I'm following? An "feedStatus" enum on each Order of e.g. "NotVisible", "Visible", "Archived", "Deleted" (which covers the feed retention policy, different from the 'deleted' state of the record) would allow the lifecycle of the record to be represented? It would be "NotVisible" after B, "Visible" following a cancellation or other action, then "Archived" after a specified period, and "Deleted" following Order Deletion.

[nathansalter]

Depending on what we're meaning by 'ownership'

When I mean ownership I mean about a Single Source of Truth. Although both sides can update the Order, it is definitely 'owned' by the Venue (Seller), as that's the location where the bookings will take place. Payments, Invoices and Receipts definitely make sense to be owned by the Broker, as if there's a dispute over any discrepancies the Venue holds the 'correct' information. An example of this would be if the Customer contacted the venue to change their details.

As for the feedStatus enum, that makes a lot of sense but is non-trivial to implement in a booking system. I'll give examples related to Bookteq as I can provide more detail if required. Essentially it's not simple to know what is updating a specific record, but let's go into a bit more detail.

Basically during and after the booking process we have systems which may affect the Order and trigger an update. Things such as payments, customer details changing, adding order notes etc. Some of these directly relate to the OA\Order state, and others are simply metadata required for the Venue to manage the bookings.

To show updated Orders in the RPDE feeds we need to keep an updatedAt value in the database. Every time the Order (or related objects, such as the OrderItems) are updated, this updates this updatedAt value. When updating this value we would also need to update the feedStatus value as well to say that this change should be shown in the feed. The problem this causes is that changing an Order from an OrderQuote to an actual Order is just an update like this. Other processes then make further updates to the other sub-entities for the Order (OrderItem, Customer etc). Each of those processes would then need to know which changes should be shown in the feed and which shouldn't. This causes Openactive-specific logic to bleed into the main domain of the booking system, which is something I'd really like to avoid.

Remember that the Order isn't a single item in the database. With a single Booking, we have about half a dozen different entities in the Database which reflect the Order's state. Each of those would have to cascade the feedStatus to determine if a specific update should be shown in the feed.

It's definitely possible that I'm massively overcomplicating this, but I can't see a simple way of handling this in the booking system that we have.

On a further point, I'm not sure I understand the reasoning behind this restriction in the first place. You mention that if you're processing a feed you shouldn't see updates related to your changes. But why not? I'm interested about what problems this could cause, especially as our current implementations don't hide these changes.

[nickevansuk]

Further considerations based on a call just now with @thill-odi and @nathansalter:

• In order prevent race conditions where e.g. the access code gets set by an async process triggered on the Booking System by B, we may want to delay all RPDE Order updates by e.g. 2 minutes from B, to ensure that the update does not land before the booking completes on the Broker side.
• Alternatively a timestamp/version number could be used within the Order to help the Broker to determine which update is the latest for the subset of properties that may be updated by the Orders Feed.

[nathansalter]

Some clarifications and notes discussed in the call:

Source of Truth

There are two sources of truth for an Order. This doesn't cause any conflict, as the sources of truth for different parameters never collides. For all the Opportunity related fields and sub-objects, the Seller is the source of truth, and for the transactional properties (payment, invoice, receipt etc.) the Broker is the source of truth. The Broker cannot update an Order after B with any of the Seller properties, and vice-versa. The list of allowed properties for the Seller to update can be found here.

Race Condition with system updates

The Race condition between B and the Order appearing in the feed after updates may be caused by a Seller if they have automatic processes which update the Order. An example of this is external systems for managing Passcodes for automated pin entry systems. Once the Order has been placed, a messenger queue picks up for each OrderItem which requires a Passcode. It then contacts an external system to generate a new code before updating the OrderItem. The Order now appears in the RPDE feed. This entire process could take <100ms in theory so the potential that the Broker has not yet finished persisting B to the data store is there.

Question surrounding B and P not appearing in feeds

This is to ensure that race conditions between B or P and those items appearing in the feed is reduced. Also from a purist standpoint, the RPDE feed is for updates, rather than full state of a system. Especially in this case as many properties of the Order must not be provided by the Seller. Many properties provided in B are for the transaction and are not relevant for further updates of the Order entity. A Broker cannot look at an RPDE feed of orders and restore the state of their system as it doesn't provide enough information (by design) to rebuild the state of the Broker's system.

Migration Path

Also from the call, although this is a breaking change for existing implementations, there will be no changes to the libraries to enforce a single type of items in an RPDE feed. Therefore the migration path for Brokers will be to filter the current combined feed to whichever feed they are processing at that moment. Sellers should update to supply this new feed as soon as possible, for ease of implementation with many brokers.