Token transfer messages are delayed in batch assembly

[awrichar]

When a token transfer is sent with a message attachment, the message is first created in "staged" state, and then moved to "ready" once the transfer succeeds. On the sender node, this "ready" message is not immediately picked up for processing, but instead waits for the timeout that polls for new messages.

Seems like this could be because Orchestrator only listens for when messages are created, but this is actually a message being updated (because it's just changing state to "ready") - so it isn't picked up.

firefly/internal/orchestrator/persistence_events.go

Line 38 in f56d701

or.batch.NewMessages() <- sequence

[peterbroadhurst]

Agree description above is spot on. Investigating a solution.

[peterbroadhurst]

Thought I was really close with this one, having worked out the shoulder-tap mechanism - then realized a complex aspect to the problem - and actually I'm not sure we would ever send the message in some circumstances.

We keep track of which messages we've assembled into batches, by simply moving a sequence offset forwards.
Unlike with pins we don't update the messages as they're assembed into a batch - rather we update them as the batch seals.

In this case, something is potentially dropping in behind the most recent assembled batch, but if we rewind we might re-read ones we've put into the batch. So I need to think about this a bit more.

[peterbroadhurst]

At the moment this is how the batching logic works:

• The assembly loop of each batch dispatcher is able to keep accepting work as long as the last batch has been successfully dispatched.
• The assembly loop passes work to the persistence loop, which has as many slots in its channel input as there are slots in the batch.
• The persistence loop keeps continually recording data to the database.
• The offset on the batch manager that's reading from the messages queue, is updated as soon as the assembly loop picks up the message

This design evolved from a similar design in the previous generation of Asset Trail technology.

Given this most recent issue, and some inefficiency and crash recovery challenges that exist in the design after it's port over to FireFly and evolution, I'm planning an updated design.

[peterbroadhurst]

This is my current thought of the new batch logic:

• We remove one of the loops
• Persistence only happens once for each batch in the final stage when the batch is sealed
• Dispatching is async to the assembler
• The assembler only allows one batch to be in assembly before it blocks waiting for the dispatch completion
• The batch manager keeps track of all offsets dispatched to each active batch processor
• When dispatch is completed for an offset, that state is passed all the way back to the processor
• The manager updates the persisted offset, when the lowest offset dispatched to all processors is complete
• The manager keeps an in-memory offset to use for each DB polling cycle, which might be ahead of the persisted one
• The manager can rewind at any time, because everything will either:
  • Be persisted with a batch ID so skipped
  • Be already in-flight

[peterbroadhurst]

I've parked the rewind-on-db-update changes on this branch:
https://github.com/kaleido-io/firefly/tree/fix-421

Will hope to come back to this overall issue later today.

[peterbroadhurst]

Ok - been thinking a bit more about this.
Now everything said above is true, and I believe the architectural change above is worthwhile on the V1.0 timeframe (for efficiency, and restart resliience).

However, I'm also concerned as I think about it, that we've done something wonky by allowing the change in state of a message to cause it to be picked up out of order.

In this case there's no determinism in the order of that message. It might get picked up in any order into a batch, and on restart/recovery it's possible for that order to be different. This feels wrong to me.

So I think the immediate fix for this issue (which would be safer to backport to v0.11.x) is for when the message transitions into "ready" it gets assigned a new sequence that is further forwards in the stream. As if it were a new message.

This means a delete+recreate in a SQL world, but there should be similar constructs in all other databases to allow the most recent sequence to be allocated to the message.

[awrichar]

So I think the immediate fix for this issue (which would be safer to backport to v0.11.x) is for when the message transitions into "ready" it gets assigned a new sequence that is further forwards in the stream. As if it were a new message.

This is absolutely correct. A "staged" message is not relevant at all for sequencing; it's just been stuffed away for (possible) future use. The "ready" state is the significant one that indicates a message is ready to get in the queue.