Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c5e437327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (true) { | ||
| const tasksInProgress = []; | ||
| for (let i = 0; i < parallel; ++i) { | ||
| const now = time.now(); | ||
| const now = typeof getCurrentTime === 'function' ? getCurrentTime() : time.now(); | ||
| const task = await this.findOneAndUpdate( |
There was a problem hiding this comment.
Keep custom clock consistent with execute() timing
When startPolling() supplies getCurrentTime, the poller now computes now from that custom clock, but execute() still uses time.now() for startedRunningAt and the schedulingTimeoutAt check. That creates inconsistent timing if callers advance a fake clock (e.g., for tests or demos): tasks that are overdue under the custom clock can still execute because execute() compares schedulingTimeoutAt against real time. This is a behavioral regression compared to the previous global time.now override; if custom time is meant to control polling, consider threading the same clock into execute() (or at least the scheduling timeout check) so timeouts behave predictably under getCurrentTime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors the polling time handling in the task system to support custom clock functions, preventing mutations to the global time module and enabling safer concurrent polling scenarios with independent time sources.
Changes:
- Thread
getCurrentTimeoption fromstartPolling()through topoll()andexpireTimedOutTasks()functions - Add comprehensive test coverage for the custom clock functionality
- Document the
getCurrentTimeoption in README with a clear example
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/task.test.js | Adds test case verifying that startPolling() correctly uses a custom getCurrentTime function for task scheduling and execution |
| src/taskSchema.js | Updates startPolling(), expireTimedOutTasks(), and poll() to accept and use a custom getCurrentTime function instead of mutating the global time module |
| README.md | Documents the new getCurrentTime polling option with an example showing how to provide a custom clock for testing or demos |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Motivation
startPolling()so multiple pollers can safely run in the same process.getCurrentTimefunction when provided.Dateinstance from the example and make the README example return a freshDateper call.Description
getCurrentTimeoption throughstartPolling()intodoPoll(),poll()andexpireTimedOutTasks()instead of overwritingtime.now, implemented insrc/taskSchema.js.poll()usegetCurrentTime()(when provided) to computenowfor selecting pending tasks and settingstartedRunningAt/timeoutAt.expireTimedOutTasks()to accept anoptionsobject and usegetCurrentTime()(when provided) to determine expirations.README.mdexample to return a newDateinstance each call:getCurrentTime: () => new Date(currentTime).allows startPolling() to use getCurrentTime()intest/task.test.jsto verify the custom clock is invoked and tasks run while using the provided clock.Testing
allows startPolling() to use getCurrentTime()was added/modified intest/task.test.jsto assert the stubbedgetCurrentTimeis called and that the scheduled task runs and succeeds.Codex Task