-
Notifications
You must be signed in to change notification settings - Fork 41
actually check DLQ #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
0f05e7c
actually check DLQ
maxdml 6950cbe
nits
maxdml c8253a1
nit + remove flaky test
maxdml d719794
add log entry when nothing is recovered at Launch time
maxdml 1f79c54
fix flaky test: tasks of the same priority must be dequeued in the sa…
maxdml a474ec9
debug
maxdml 05041a4
disable test cache + -v
maxdml eb72c75
force time between created_at
maxdml 8a21ec4
debug
maxdml 19ee252
simplify
maxdml 0f51ea9
simplify
maxdml File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,7 +213,7 @@ func TestWorkflowQueues(t *testing.T) { | |
|
|
||
| // Check that the workflow hits DLQ after re-running max retries | ||
| handles := make([]WorkflowHandle[any], 0, dlqMaxRetries+1) | ||
| for i := range dlqMaxRetries { | ||
| for range dlqMaxRetries { | ||
| recoveryHandles, err := recoverPendingWorkflows(dbosCtx.(*dbosContext), []string{"local"}) | ||
| require.NoError(t, err, "failed to recover pending workflows") | ||
| assert.Len(t, recoveryHandles, 1, "expected 1 handle") | ||
|
|
@@ -223,10 +223,25 @@ func TestWorkflowQueues(t *testing.T) { | |
| handles = append(handles, handle) | ||
| status, err := handle.GetStatus() | ||
| require.NoError(t, err, "failed to get status of recovered workflow handle") | ||
| if i == dlqMaxRetries { | ||
| // On the last retry, the workflow should be in DLQ | ||
| assert.Equal(t, WorkflowStatusRetriesExceeded, status.Status, "expected workflow status to be %s", WorkflowStatusRetriesExceeded) | ||
| assert.Equal(t, WorkflowStatusPending, status.Status, "expected workflow to be in PENDING status after recovery") | ||
| } | ||
|
|
||
| dlqHandle, err := recoverPendingWorkflows(dbosCtx.(*dbosContext), []string{"local"}) | ||
| require.NoError(t, err, "failed to recover pending workflows") | ||
| assert.Len(t, dlqHandle, 1, "expected 1 handle in DLQ") | ||
| retries := 0 | ||
| for { | ||
| dlqStatus, err := dlqHandle[0].GetStatus() | ||
| require.NoError(t, err, "failed to get status of DLQ workflow handle") | ||
| if dlqStatus.Status != WorkflowStatusRetriesExceeded && retries < 10 { | ||
| time.Sleep(1 * time.Second) // Wait a bit before checking again | ||
| retries++ | ||
| continue | ||
| } | ||
| require.NoError(t, err, "failed to get status of DLQ workflow handle") | ||
| assert.Equal(t, WorkflowStatusRetriesExceeded, dlqStatus.Status, "expected workflow to be in DLQ after max retries exceeded") | ||
| handles = append(handles, dlqHandle[0]) | ||
| break | ||
|
Comment on lines
+226
to
+244
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we were not actually testing the DLQ behavior in this test -- now we do |
||
| } | ||
|
|
||
| // Check the workflow completes | ||
|
|
@@ -258,8 +273,8 @@ func TestWorkflowQueues(t *testing.T) { | |
| require.Error(t, err, "expected ConflictingWorkflowError when enqueueing same workflow ID on different queue, but got none") | ||
|
|
||
| // Check that it's the correct error type | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
|
|
||
| assert.Equal(t, ConflictingWorkflowError, dbosErr.Code, "expected error code to be ConflictingWorkflowError") | ||
|
|
||
|
|
@@ -301,8 +316,8 @@ func TestWorkflowQueues(t *testing.T) { | |
| require.Error(t, err, "expected error when enqueueing workflow with same deduplication ID") | ||
|
|
||
| // Check that it's the correct error type and message | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
| assert.Equal(t, QueueDeduplicated, dbosErr.Code, "expected error code to be QueueDeduplicated") | ||
|
|
||
| expectedMsgPart := fmt.Sprintf("Workflow %s was deduplicated due to an existing workflow in queue %s with deduplication ID %s", wfid2, dedupQueue.Name, dedupID) | ||
|
|
@@ -416,14 +431,14 @@ func TestQueueRecovery(t *testing.T) { | |
| castedResult, ok := result.([]int) | ||
| require.True(t, ok, "expected result to be of type []int for root workflow, got %T", result) | ||
| expectedResult := []int{0, 1, 2, 3, 4} | ||
| assert.True(t, equal(castedResult, expectedResult), "expected result %v, got %v", expectedResult, castedResult) | ||
| assert.Equal(t, expectedResult, castedResult, "expected result %v, got %v", expectedResult, castedResult) | ||
| } | ||
| } | ||
|
|
||
| result, err := handle.GetResult() | ||
| require.NoError(t, err, "failed to get result from original handle") | ||
| expectedResult := []int{0, 1, 2, 3, 4} | ||
| assert.True(t, equal(result, expectedResult), "expected result %v, got %v", expectedResult, result) | ||
| assert.Equal(t, expectedResult, result, "expected result %v, got %v", expectedResult, result) | ||
|
|
||
| assert.Equal(t, int64(queuedSteps*2), atomic.LoadInt64(&recoveryStepCounter), "expected recoveryStepCounter to be %d", queuedSteps*2) | ||
|
|
||
|
|
@@ -432,7 +447,7 @@ func TestQueueRecovery(t *testing.T) { | |
| require.NoError(t, err, "failed to rerun workflow") | ||
| rerunResult, err := rerunHandle.GetResult() | ||
| require.NoError(t, err, "failed to get result from rerun handle") | ||
| assert.True(t, equal(rerunResult, expectedResult), "expected result %v, got %v", expectedResult, rerunResult) | ||
| assert.Equal(t, expectedResult, rerunResult, "expected result %v, got %v", expectedResult, rerunResult) | ||
|
|
||
| assert.Equal(t, int64(queuedSteps*2), atomic.LoadInt64(&recoveryStepCounter), "expected recoveryStepCounter to remain %d", queuedSteps*2) | ||
|
|
||
|
|
@@ -794,9 +809,7 @@ func TestQueueTimeouts(t *testing.T) { | |
| queuedWaitForCancelWorkflow := func(ctx DBOSContext, _ string) (string, error) { | ||
| // This workflow will wait indefinitely until it is cancelled | ||
| <-ctx.Done() | ||
| if !errors.Is(ctx.Err(), context.Canceled) && !errors.Is(ctx.Err(), context.DeadlineExceeded) { | ||
| assert.True(t, errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded), "workflow was cancelled, but context error is not context.Canceled nor context.DeadlineExceeded: %v", ctx.Err()) | ||
| } | ||
| assert.True(t, errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded), "workflow was cancelled, but context error is not context.Canceled nor context.DeadlineExceeded: %v", ctx.Err()) | ||
| return "", ctx.Err() | ||
| } | ||
| RegisterWorkflow(dbosCtx, queuedWaitForCancelWorkflow) | ||
|
|
@@ -808,8 +821,8 @@ func TestQueueTimeouts(t *testing.T) { | |
| // Workflow should get AwaitedWorkflowCancelled DBOSError | ||
| _, err = handle.GetResult() | ||
| require.Error(t, err, "expected error when waiting for enqueued workflow to complete, but got none") | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
| assert.Equal(t, AwaitedWorkflowCancelled, dbosErr.Code, "expected error code to be AwaitedWorkflowCancelled") | ||
|
|
||
| // enqueud workflow should have been cancelled | ||
|
|
@@ -877,8 +890,8 @@ func TestQueueTimeouts(t *testing.T) { | |
| require.Error(t, err, "expected error but got none") | ||
|
|
||
| // Check the error type | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
|
|
||
| assert.Equal(t, AwaitedWorkflowCancelled, dbosErr.Code, "expected error code to be AwaitedWorkflowCancelled") | ||
|
|
||
|
|
@@ -905,8 +918,8 @@ func TestQueueTimeouts(t *testing.T) { | |
| require.Error(t, err, "expected error but got none") | ||
|
|
||
| // Check the error type | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
|
|
||
| assert.Equal(t, AwaitedWorkflowCancelled, dbosErr.Code, "expected error code to be AwaitedWorkflowCancelled") | ||
|
|
||
|
|
@@ -934,8 +947,8 @@ func TestQueueTimeouts(t *testing.T) { | |
| require.Error(t, err, "expected error but got none") | ||
|
|
||
| // Check the error type | ||
| dbosErr, ok := err.(*DBOSError) | ||
| require.True(t, ok, "expected error to be of type *DBOSError, got %T", err) | ||
| var dbosErr *DBOSError | ||
| require.ErrorAs(t, err, &dbosErr, "expected error to be of type *DBOSError, got %T", err) | ||
|
|
||
| assert.Equal(t, AwaitedWorkflowCancelled, dbosErr.Code, "expected error code to be AwaitedWorkflowCancelled") | ||
|
|
||
|
|
@@ -1051,6 +1064,7 @@ func TestPriorityQueue(t *testing.T) { | |
| require.NoError(t, err) | ||
| wfHandles = append(wfHandles, handle6) | ||
|
|
||
| time.Sleep(10 * time.Millisecond) // Avoid collisions in created_at... | ||
| handle7, err := RunAsWorkflow(dbosCtx, testWorkflow, 7, WithQueue(priorityQueue.Name)) | ||
| require.NoError(t, err) | ||
| wfHandles = append(wfHandles, handle7) | ||
|
|
@@ -1070,5 +1084,16 @@ func TestPriorityQueue(t *testing.T) { | |
| assert.Equal(t, expectedOrder, wfPriorityList, "expected workflow execution order %v, got %v", expectedOrder, wfPriorityList) | ||
| mu.Unlock() | ||
|
|
||
| // Verify that handle6 and handle7 workflows were dequeued in FIFO order | ||
| // by checking that their StartedAt time is in the correct order (6 is before 7) | ||
| status6, err := handle6.GetStatus() | ||
| require.NoError(t, err, "failed to get status for workflow 6") | ||
| status7, err := handle7.GetStatus() | ||
| require.NoError(t, err, "failed to get status for workflow 7") | ||
|
|
||
| assert.True(t, status6.StartedAt.Before(status7.StartedAt), | ||
| "expected workflow 6 to be dequeued before workflow 7, but got 6 started at %v (created at %v) and 7 started at %v (created at %v)", | ||
| status6.StartedAt, status6.CreatedAt, status7.StartedAt, status7.CreatedAt) | ||
|
|
||
| require.True(t, queueEntriesAreCleanedUp(dbosCtx), "expected queue entries to be cleaned up after priority queue test") | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -572,7 +572,7 @@ func RunAsWorkflow[P any, R any](ctx DBOSContext, fn GenericWorkflowFunc[P, R], | |
| } | ||
| }() | ||
|
|
||
| typedHandle := newWorkflowHandle[R](handle.dbosContext, handle.workflowID, typedOutcomeChan) | ||
| typedHandle := newWorkflowHandle(handle.dbosContext, handle.workflowID, typedOutcomeChan) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not necessary because inferred from |
||
|
|
||
| return typedHandle, nil | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This disables "caching" for the tests...