Process End()

[koden-km]

What change does this introduce?

Ending a process is now permanent.

Why make this change?

This change is to match the new ADR-24 spec.

Is there anything you are unsure about?

• I'm not sure if the process adaptor.go should be checking the instance HasEnded flag before creating the scope and panic from there rather than doing it in HandleMessage()?
• Not adding ended: inst.HasEnded, to the process scope creation still passes tests. Another test case might be needed?

What issues does this relate to?

Fixes #660

[jmalloc]

• Not adding ended: inst.HasEnded, to the process scope creation still passes tests. Another test case might be needed?

I think this behavior makes sense as is. We never expect handling to get this far if the process has already ended. Therefore, the only purpose of scope.ended is to signal that we have ended the process within this scope, which aligns with its usage to cause the two new panics, and to pass true to the HasEnded flag when saving the instance.

All that said, I think we might have a race condition here when it comes to handling events. This line here

verity/handler/process/adaptor.go

Line 83 in 32970c0

if !exists && !p.ScheduledFor.IsZero() {

handles the "ignoring" for timeouts, but as best I can tell there's nothing that handles it for events. I can't recall off the top of my head whether events are added to the queue for processes, or if they're fed directly from event streams, but I'm also not sure if that matters.

• I'm not sure if the process adaptor.go should be checking the instance HasEnded flag before creating the scope and panic from there rather than doing it in HandleMessage()?

I think this relates to the last paragraph, but I'm not sure which panic you're referring to.

[jmalloc]

This may not be necessary, as you pointed out, adding it doesn't change any behavior. If we do need more test cases, it would be to guarantee that both events and timeouts are ignored for ended processes (these may already exist?)

[jmalloc]

See #665 (comment)

[koden-km]

Should I remove this ended initialisation?

[koden-km]

I think this relates to the last paragraph, but I'm not sure which panic you're referring to.

The panic I ment was the one in the handler that is given the created scope. I guess i'm more referring to what happens on the next time the process gets sent an event after having ended from handling a previous event/timeout. Perhaps Dogma already knows not call it again?

[jmalloc]

Perhaps Dogma already knows not call it again?

No, something here needs to check the ended flag before routing the message (it's already doing it for timeouts).

[koden-km]

I've added a check for ended process before handling new messages. Not quite sure if that covers what you mentioned though?

[jmalloc]

Suggested change

	if exists && inst.HasEnded {
	if inst.HasEnded {

Pretty sure this can be simplified and the if above this one can then be removed.

[jmalloc]

Also, I don't think there's any existing test for this logic, and certainly not for events specifically.

[koden-km]

Can't remove the if block above without breaking a timeout test for a not yet existing instance.

[koden-km]

I've removed the exists check from that line and added another test to verify messages to an already ended process get ignored.