fix: prevent test worker crash during completion error handling#305
Open
YunchuWang wants to merge 2 commits into
Open
Conversation
…when completeOrchestration fails completeOrchestration throws when the instance has been purged or the backend reset, but completeActivity silently returns for the same case. When the test worker's processOrchestration catch block calls completeOrchestration on a missing instance, the unhandled exception propagates to runProcessingLoop and kills it. All subsequent work items are silently dropped. Fix both the root cause and the symptom: - completeOrchestration now returns silently for missing instances, consistent with completeActivity - processOrchestration's catch block wraps completeOrchestration in a nested try-catch for defense-in-depth Add two new tests: - Worker continues processing after backend reset during execution - completeOrchestration silently ignores purged instances Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the in-memory testing stack so TestOrchestrationWorker doesn’t stop processing when orchestration completion happens after a backend reset/purge (or when completion fails while handling an orchestration failure), aligning behavior with completeActivity and adding regression coverage.
Changes:
- Make
InMemoryOrchestrationBackend.completeOrchestration()a no-op when the target instance is missing (e.g., purged/reset). - Add a defensive
try/catchinTestOrchestrationWorker.processOrchestration()to prevent a secondary completion failure from terminating the processing loop. - Add tests intended to cover reset/completion race scenarios and missing-instance completion behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/testing/in-memory-backend.ts | Treat missing instances during orchestration completion as already removed (no-op instead of throwing). |
| packages/durabletask-js/src/testing/test-worker.ts | Prevent worker loop crash if completeOrchestration throws while reporting orchestration failure. |
| packages/durabletask-js/test/in-memory-backend.spec.ts | Add/extend regression tests around reset/purge + completion behavior. |
…n race (microsoft#174) Simplify the fix for the TestOrchestrationWorker processing-loop crash down to the minimal necessary change: InMemoryOrchestrationBackend.completeOrchestration() returns a no-op when the target instance is missing (purged/reset), mirroring completeActivity(). Drop two changes flagged in review: - Remove the blanket try/catch around the failure-path completeOrchestration call in test-worker.ts. With the root fix a missing instance no longer throws, so the catch only served to swallow real errors (e.g. history-size limit), which could hide backend bugs and cause silent hangs. - Remove the "continue processing after backend reset during orchestration execution" test. It reset the backend only after the first orchestration had fully completed, so it never reproduced the microsoft#174 race and passed even without the fix. Keep the "silently ignore completeOrchestration for purged instances" test, which deterministically validates the root fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d175a43f-c852-49a2-a0de-63db579a8e3a
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes #174
Testing
npm test -w @microsoft/durabletask-js -- --runTestsByPath test/in-memory-backend.spec.tsnpm run build:core