-
Notifications
You must be signed in to change notification settings - Fork 10
fix: Sub-orchestration watcher uses unintended 30s default timeout in InMemoryOrchestrationBackend #156
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
base: main
Are you sure you want to change the base?
fix: Sub-orchestration watcher uses unintended 30s default timeout in InMemoryOrchestrationBackend #156
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -337,24 +337,29 @@ export class InMemoryOrchestrationBackend { | |
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const timer = setTimeout(() => { | ||
| const waiters = this.stateWaiters.get(instanceId); | ||
| if (waiters) { | ||
| const index = waiters.findIndex((w) => w.resolve === resolve); | ||
| if (index >= 0) { | ||
| waiters.splice(index, 1); | ||
| // When timeoutMs is 0, no timeout is applied — the waiter will only be | ||
| // resolved by a matching state change or rejected by reset(). | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| if (timeoutMs > 0) { | ||
| timer = setTimeout(() => { | ||
| const waiters = this.stateWaiters.get(instanceId); | ||
| if (waiters) { | ||
| const index = waiters.findIndex((w) => w.resolve === resolve); | ||
| if (index >= 0) { | ||
| waiters.splice(index, 1); | ||
| } | ||
| } | ||
| } | ||
| reject(new Error(`Timeout waiting for orchestration '${instanceId}'`)); | ||
| }, timeoutMs); | ||
| reject(new Error(`Timeout waiting for orchestration '${instanceId}'`)); | ||
|
Comment on lines
+344
to
+352
|
||
| }, timeoutMs); | ||
| } | ||
|
|
||
| const waiter: StateWaiter = { | ||
| resolve: (result) => { | ||
| clearTimeout(timer); | ||
| if (timer !== undefined) clearTimeout(timer); | ||
| resolve(result); | ||
| }, | ||
| reject: (error) => { | ||
| clearTimeout(timer); | ||
| if (timer !== undefined) clearTimeout(timer); | ||
| reject(error); | ||
| }, | ||
| predicate, | ||
|
|
@@ -590,8 +595,7 @@ export class InMemoryOrchestrationBackend { | |
| this.waitForState( | ||
| subInstanceId, | ||
| (inst) => this.isTerminalStatus(inst.status), | ||
| // No timeout - sub-orchestration will eventually complete, fail, or be terminated | ||
| // If parent is terminated, we check that when delivering the event | ||
| 0, // No timeout — sub-orchestration will eventually complete, fail, or be terminated | ||
| ) | ||
|
Comment on lines
595
to
599
|
||
| .then((subInstance) => { | ||
| const parentInstance = this.instances.get(parentInstanceId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -146,6 +146,56 @@ describe("In-Memory Backend", () => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(activityCounter).toEqual(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should handle sub-orchestrations with timer delays", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const childWithTimer: TOrchestrator = async function* (ctx: OrchestrationContext): any { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sub-orchestration uses a short timer before returning a result | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield ctx.createTimer(0.1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "child-done"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const parentOrchestrator: TOrchestrator = async function* (ctx: OrchestrationContext): any { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = yield ctx.callSubOrchestrator(childWithTimer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `parent-received-${result}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| worker.addOrchestrator(childWithTimer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| worker.addOrchestrator(parentOrchestrator); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await worker.start(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const id = await client.scheduleNewOrchestration(parentOrchestrator); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const state = await client.waitForOrchestrationCompletion(id, true, 10); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(state).toBeDefined(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(state?.serializedOutput).toEqual(JSON.stringify("parent-received-child-done")); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+150
to
+170
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const childWithTimer: TOrchestrator = async function* (ctx: OrchestrationContext): any { | |
| // Sub-orchestration uses a short timer before returning a result | |
| yield ctx.createTimer(0.1); | |
| return "child-done"; | |
| }; | |
| const parentOrchestrator: TOrchestrator = async function* (ctx: OrchestrationContext): any { | |
| const result = yield ctx.callSubOrchestrator(childWithTimer); | |
| return `parent-received-${result}`; | |
| }; | |
| worker.addOrchestrator(childWithTimer); | |
| worker.addOrchestrator(parentOrchestrator); | |
| await worker.start(); | |
| const id = await client.scheduleNewOrchestration(parentOrchestrator); | |
| const state = await client.waitForOrchestrationCompletion(id, true, 10); | |
| expect(state).toBeDefined(); | |
| expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); | |
| expect(state?.serializedOutput).toEqual(JSON.stringify("parent-received-child-done")); | |
| jest.useFakeTimers(); | |
| try { | |
| const childWithTimer: TOrchestrator = async function* (ctx: OrchestrationContext): any { | |
| // Sub-orchestration uses a timer longer than 30s before returning a result | |
| yield ctx.createTimer(31); | |
| return "child-done"; | |
| }; | |
| const parentOrchestrator: TOrchestrator = async function* (ctx: OrchestrationContext): any { | |
| const result = yield ctx.callSubOrchestrator(childWithTimer); | |
| return `parent-received-${result}`; | |
| }; | |
| worker.addOrchestrator(childWithTimer); | |
| worker.addOrchestrator(parentOrchestrator); | |
| await worker.start(); | |
| const id = await client.scheduleNewOrchestration(parentOrchestrator); | |
| const waitPromise = client.waitForOrchestrationCompletion(id, true, 40); | |
| // Advance fake timers past the 31s child timer to ensure the parent observes completion | |
| jest.advanceTimersByTime(31_000); | |
| const state = await waitPromise; | |
| expect(state).toBeDefined(); | |
| expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); | |
| expect(state?.serializedOutput).toEqual(JSON.stringify("parent-received-child-done")); | |
| } finally { | |
| jest.useRealTimers(); | |
| } |
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.
timeoutMs=0now means “no timeout”. Since this is a non-obvious sentinel value (and differs from common “0 means immediate timeout” expectations), it would be good to reflect it in thewaitForStateJSDoc/parameter description so callers don’t accidentally create an unbounded wait.