Refresh the actor when AssignWorkerStep's write hits a version conflict. - #535
Conversation
05b1f1c to
aa64e92
Compare
aa64e92 to
1a00de2
Compare
1a00de2 to
ef51102
Compare
| // retry applies on top of the concurrent write instead of replaying the same | ||
| // stale version until the backoff is exhausted; an actor that left the | ||
| // resumable states aborts the retry instead of being resurrected to RESUMING. | ||
| func TestAssignWorkerStep_ConflictRefreshesActor(t *testing.T) { |
There was a problem hiding this comment.
For readability of tests, can we use table-driven tests, and run them in parallel if possible?
There was a problem hiding this comment.
updated
| updatedActor, err := s.store.UpdateActor(ctx, state.Actor, state.Actor.GetMetadata().GetVersion()) | ||
| if err != nil { | ||
| return err | ||
| if !errors.Is(err, store.ErrPersistenceRetry) { |
There was a problem hiding this comment.
This isn't necessarily something we need to solve in this PR, but I wonder if a ErrVersionConflct is in order for this scenario. Refetching the actor on EVERY error here doesn't seem quite right to me
There was a problem hiding this comment.
We are not refetching the actor on every error, current ErrPersistenceRetry means right "version mismatch", code.
ef51102 to
2bba276
Compare
…ct, otherwise the retry keeps using the stale version and never succeeds.
2bba276 to
87c19a3
Compare
| switch fresh.GetStatus() { | ||
| case ateapipb.Actor_STATUS_SUSPENDED, ateapipb.Actor_STATUS_PAUSED: | ||
| slog.InfoContext(ctx, "Retrying assignment due to actor version conflict", slog.String("actor", input.Atespace+"/"+input.ActorName)) | ||
| state.Actor = fresh |
There was a problem hiding this comment.
Just a note - the correctness here relies on the fact that no other step before AssignWorkStep modifies state.Actor.
|
I’m ok that this merged, but honestly I don’t think this answers my question #535 (comment) It checks a negative case on an error because that function happens to only use one other one, which is making too many assumptions about the behavior of the underlying function. The errors should describe the behavior. This comment is also fairly scary #535 (comment) |
Understood, to avoid we fold other errors into
Fair - it's only safe because today no step before |
4 participants
Refresh the actor when
AssignWorkerStep's write hits a version conflict so that the workflow won't kept retry with a stale version actor and never succeed.AI assisted on testing.