cschleiden / go-workflows

Embedded durable workflows for Golang similar to DTFx/Cadence/Temporal
https://cschleiden.github.io/go-workflows/
MIT License
233 stars 49 forks source link

Prevent creating multiple workflow instances with the same instance id #260

Closed cschleiden closed 11 months ago

cschleiden commented 1 year ago

Two subsequent CreateWorkflowInstance calls with the same id should result in an error.

Discussed in https://github.com/cschleiden/go-workflows/discussions/259

decibelcooper commented 1 year ago

thought: perhaps it's best to follow Temporal's lead and allow the user to set an ID reuse policy

A simplified binary allow/reject duplicate option should satisfy most use cases. Could probably simply be used as a hint for whether or not to generate a random execution ID? Or would it be reasonable to expose the execution ID to the client API?

EDIT: Side note: it currently feels a little awkward to serialize a workflow instance for later reference, e.g. to cancel a workflow at a later time. The reason being that the workflow instance is passed to the client API as an alias to a core struct that also includes parent/child relationship. I bring this up because if execution ID is exposed to the user in a cleaner way, it may also help the user to serialize references to workflow instances.

decibelcooper commented 1 year ago

Another thing to note is that instance ID reuse has an impact on signaling.

Today, the behavior for signaling instances with multiple active executions is undefined at the Backend level. Looking anecdotally at the mysql backend, I see that we just snag one execution to signal. It might be reasonable to instead perform a broadcast of sorts, so that all active executions receive the signal.

lyuboxa commented 1 year ago

When I encountered this first, I looked at the potential options to enable some uniqueness in the schema without interfering too much with the codebase. Unfortunately, making instance id unique individually has implications for functionalities like ContinueAsNew where the instance id is reused with an updated execution id. This can be addressed in code though with some safety from the database.

This led me to think that decoupling instance definition and execution may provide more flexibility.

thought: perhaps it's best to follow Temporal's lead and allow the user to set an ID reuse policy

The reuse policy definition is a good way to provide optionality, 👍

cschleiden commented 11 months ago

Today, the behavior for signaling instances with multiple active executions is undefined at the Backend level. Looking anecdotally at the mysql backend, I see that we just snag one execution to signal. It might be reasonable to instead perform a broadcast of sorts, so that all active executions receive the signal.

There should only be one active execution at any point in time. If there are multiple, that's a bug and not as designed.

cschleiden commented 11 months ago

Closed in #288