fix: use deterministic time in createTimer instead of Date.now()#146
Open
YunchuWang wants to merge 2 commits intomainfrom
Open
fix: use deterministic time in createTimer instead of Date.now()#146YunchuWang wants to merge 2 commits intomainfrom
YunchuWang wants to merge 2 commits intomainfrom
Conversation
Fixes #131 The WhenAllTask constructor redundantly re-initialized _completedTasks and _failedTasks to 0 after calling super(tasks). Since CompositeTask's constructor already initializes these fields and then processes pre-completed children via onChildCompleted(), the reset wiped out the correct count, causing WhenAllTask to never complete when some children were already complete at construction time. Also removes _failedTasks reset (dead code - never incremented anywhere). Added 8 unit tests for WhenAllTask covering: - Empty task array - All pending children completing - Fail-fast on child failure - Pre-completed children (the bug scenario) - All children pre-completed - Pre-failed child - Post-fail-fast completion - Pending tasks count tracking
Replace Date.now() with this._currentUtcDatetime.getTime() in RuntimeOrchestrationContext.createTimer() to ensure the timer fire-at time is computed deterministically from the orchestration time rather than wall clock time. This fixes a determinism contract violation during replay. Fixes #134 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| import { WhenAllTask } from "../src/task/when-all-task"; | ||
| import { CompletableTask } from "../src/task/completable-task"; | ||
| import { Task } from "../src/task/task"; |
There was a problem hiding this comment.
Pull request overview
This PR fixes an orchestrator determinism violation by computing relative timer deadlines from the orchestration’s deterministic time (currentUtcDateTime) rather than wall-clock time, and also includes a small WhenAllTask correctness fix + new unit tests around WhenAllTask behavior.
Changes:
- Make
RuntimeOrchestrationContext.createTimer(number)computefireAtfromthis._currentUtcDatetimeinstead ofDate.now()to preserve determinism during replay. - Remove
WhenAllTaskcounter re-initialization (documenting why it must not happen aftersuper()). - Add a new Jest spec file covering
WhenAllTaskedge-cases (empty array, pre-completed children, fail-fast, pending count).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/durabletask-js/src/worker/runtime-orchestration-context.ts | Switches relative timer scheduling to use deterministic orchestration time. |
| packages/durabletask-js/src/task/when-all-task.ts | Prevents counter reset after CompositeTask processes pre-completed children. |
| packages/durabletask-js/test/when-all-task.spec.ts | Adds unit tests validating WhenAllTask completion/fail-fast semantics and counters. |
Comments suppressed due to low confidence (1)
packages/durabletask-js/src/worker/runtime-orchestration-context.ts:304
- The determinism fix in
createTimer(number)is important but currently not covered by a targeted unit test. Add a test that sets a knowncurrentUtcDateTime(viaOrchestratorStarted), callsctx.createTimer(<seconds>), and asserts the generated timer action’sfireAtequalsstartTime + seconds, including across replay.
// If a number is passed, we use it as the number of seconds to wait
// we use instanceof Date as number is not a native Javascript type
if (!(fireAt instanceof Date)) {
fireAt = new Date(this._currentUtcDatetime.getTime() + fireAt * 1000);
}
const action = ph.newCreateTimerAction(id, fireAt);
this._pendingActions[action.getId()] = action;
Comment on lines
+45
to
+56
| // Issue #131: WhenAllTask constructor resets _completedTasks counter | ||
| it("should complete correctly when constructed with pre-completed children", () => { | ||
| const child1 = new CompletableTask<number>(); | ||
| const child2 = new CompletableTask<number>(); | ||
| const child3 = new CompletableTask<number>(); | ||
|
|
||
| // Complete child1 and child2 before constructing WhenAllTask | ||
| child1.complete(10); | ||
| child2.complete(20); | ||
|
|
||
| const task = new WhenAllTask([child1, child2, child3]); | ||
|
|
There was a problem hiding this comment.
|
|
||
| import { WhenAllTask } from "../src/task/when-all-task"; | ||
| import { CompletableTask } from "../src/task/completable-task"; | ||
| import { Task } from "../src/task/task"; |
There was a problem hiding this comment.
Unused import Task will fail repo linting (@typescript-eslint/no-unused-vars is configured as an error). Remove the import or use it.
Suggested change
| import { Task } from "../src/task/task"; |
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
Replaces
Date.now()withthis._currentUtcDatetime.getTime()inRuntimeOrchestrationContext.createTimer()so that the timer fire-at time is computed deterministically from the orchestration time rather than wall clock time.Problem
createTimer()usedDate.now()when converting a relative seconds value to an absoluteDate. This is a determinism contract violation — during replay,Date.now()returns the current wall clock time (which differs from the original execution time), producing a different timer fire-at value.Fix
Use
this._currentUtcDatetime.getTime()instead, which returns the deterministic orchestration time set byOrchestratorStartedevents. This is consistent with how other Durable Task SDKs handle relative timer delays.Fixes #134