fix: cancel pending timers on continue-as-new in InMemoryOrchestrationBackend - #328
Open
YunchuWang wants to merge 1 commit into
Open
fix: cancel pending timers on continue-as-new in InMemoryOrchestrationBackend#328YunchuWang wants to merge 1 commit into
YunchuWang wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in the in-memory testing backend where continueAsNew() did not cancel timers scheduled in the prior iteration, allowing stale TimerFired events to leak into the new execution. It also adjusts carryover external event ordering to align with real sidecar behavior.
Changes:
- Track timer handles per orchestration instance and cancel them when processing
continue-as-new. - Reorder continue-as-new “carryover events” so
OrchestratorStarted/ExecutionStartedprecede carried-over events. - Add regression tests covering timer cancellation and carryover event ordering scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/testing/in-memory-backend.ts | Adds per-instance timer tracking/cancellation and fixes carryover event ordering for continue-as-new. |
| packages/durabletask-js/test/in-memory-backend.spec.ts | Adds tests intended to validate timer cancellation and carryover ordering after continue-as-new. |
Comment on lines
+388
to
+399
| const orchestrator: TOrchestrator = async function* (ctx: OrchestrationContext, input: number): any { | ||
| if (input === 1) { | ||
| // First iteration: create a long timer, then immediately continue-as-new. | ||
| // The timer should be cancelled and NOT fire into the second iteration. | ||
| ctx.createTimer(60); // 60-second timer — should be cancelled | ||
| ctx.continueAsNew(2, false); | ||
| } else { | ||
| // Second iteration: create a short timer and wait for it. | ||
| // If the stale timer from iteration 1 leaks, it could complete | ||
| // the wrong task or cause unexpected events. | ||
| yield ctx.createTimer(0.05); // 50ms timer | ||
| iteration2TimerFired = true; |
Comment on lines
+442
to
+445
| expect(state).toBeDefined(); | ||
| expect(state?.runtimeStatus).toEqual(OrchestrationStatus.COMPLETED); | ||
| expect(state?.serializedOutput).toEqual(JSON.stringify("carried-over")); | ||
| }); |
kaibocai
previously approved these changes
Jul 27, 2026
YunchuWang
force-pushed
the
copilot-finds/bug/fix-continue-as-new-timer-leak
branch
from
July 29, 2026 23:28
6f022a1 to
2b5893e
Compare
kaibocai
previously approved these changes
Jul 31, 2026
…nBackend Timer IDs are per-execution sequence numbers that restart at 1 after continue-as-new, so a timer left pending from the previous iteration fires a TimerFired event whose ID collides with the new iteration's first task and completes it early. Cancel the instance's pending timers before resetting state. Rebased onto current main. The test is a rewrite of the original: that version used a 60s stale timer against a 50ms real timer, so the stale timer could never fire within the test and it passed with and without the fix. This version inverts the durations (50ms stale vs 1.5s real) and asserts on elapsed time. Verified on current main: without the fix the orchestration completes in 58ms (expected >1000); with the fix it takes 1562ms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5aaa70df-fae3-4570-aabe-0fc96cb869bc
YunchuWang
force-pushed
the
copilot-finds/bug/fix-continue-as-new-timer-leak
branch
from
July 31, 2026 19:12
2b5893e to
145b771
Compare
bachuv
approved these changes
Jul 31, 2026
bachuv
left a comment
bachuv
left a comment
Contributor
There was a problem hiding this comment.
Approving, but please take a look at the open copilot comments!
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.
Fixes #207.
The bug
InMemoryOrchestrationBackend's continue-as-new handler resets instance history but nevercancels the timers still pending from the previous iteration.
Timer IDs are per-execution sequence numbers that restart at 1 after continue-as-new. So a
timer abandoned by iteration 1 keeps its
setTimeoutalive and later fires aTimerFiredevent whose ID collides with iteration 2's first task — completing it long before it is due.
The fix
Call
this.cancelInstanceTimers(instance.instanceId)before resetting instance state. Thehelper already exists and is used elsewhere in the class; continue-as-new was simply missing it.
Validation — verified against current
mainexpect(elapsedMs).toBeGreaterThan(1000)got58Iteration 2 asks for a 1.5 s timer. Without the fix it finishes in 58 ms, because iteration 1's
abandoned 50 ms timer fires and satisfies it. That 27x gap is the bug.
Full validation: build 0, lint 0, 1177/1177 tests across 67 suites.
Note for reviewers — the test was rewritten, and why
The original test on this branch could not fail. It used:
The stale timer was longer than the entire test, so it could never fire in time to cause the
collision, and the test passed identically with and without the fix. The durations needed to be
inverted.
This version uses a 50 ms stale timer against a 1.5 s real timer and asserts on elapsed
wall time, so a leak shows up as an early completion. The 1000 ms threshold leaves a wide margin
for CI jitter.
Note on the rebase
This branch was rebased onto current
main. The rebase could not be done mechanically: the testrewrite lived in a merge commit, which
git rebasedrops, so replaying the original commit alonewould have silently restored the broken 60 s test. The change was re-applied onto current
mainand re-verified from scratch with the numbers above.