Fix timeout scheduling edge case and cancellation handling in SimpleJobExecutor#4886
Fix timeout scheduling edge case and cancellation handling in SimpleJobExecutor#4886mrhapile wants to merge 2 commits intoboa-dev:mainfrom
Conversation
…obExecutor Signed-off-by: mrhapile <allinonegaming3456@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes SimpleJobExecutor edge cases where timeout jobs scheduled exactly at now were deferred and where cancelled timeout jobs could still execute.
Changes:
- Adjust timeout scheduling logic so jobs at exactly
nowexecute in the current tick. - Add a dispatch-time cancellation guard to prevent cancelled jobs from running.
- Add regression tests for “exactly now” scheduling and cancellation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/engine/src/job.rs | Fixes the split_off boundary behavior for now and adds a cancellation guard before calling timeout jobs. |
| core/engine/src/job/tests.rs | Adds regression tests covering the now boundary case and cancelled timeout execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Advance time realistically using system sleep so it becomes "< now" | ||
| std::thread::sleep(std::time::Duration::from_millis(10)); |
There was a problem hiding this comment.
Using std::thread::sleep in unit tests is prone to flakiness (slow CI machines, timer granularity) and slows the test suite. Since this codebase already supports injecting a Clock, prefer a controllable test clock (e.g., a ManualClock backed by Cell<JsInstant> that you can advance in the test) so the job reliably becomes due without sleeping.
| if ran.get() { | ||
| panic!("Cancelled timeout job executed anyway! Bug confirmed."); | ||
| } |
There was a problem hiding this comment.
This can be expressed as an assert!(!ran.get(), ...) (or assert_eq!(ran.get(), false, ...)) to keep the failure style consistent with other tests and produce cleaner assertion output.
| if ran.get() { | |
| panic!("Cancelled timeout job executed anyway! Bug confirmed."); | |
| } | |
| assert!( | |
| !ran.get(), | |
| "Cancelled timeout job executed anyway! Bug confirmed." | |
| ); |
| assert!( | ||
| ran.get(), | ||
| "Timeout job scheduled at exactly 'now' did NOT execute! Bug confirmed: off-by-one in split_off" | ||
| ); |
There was a problem hiding this comment.
The assertion message reads like a debugging note ("Bug confirmed") rather than stating the expected behavior. Consider rephrasing to a neutral expectation-focused message (e.g., "timeout scheduled at exactly now should execute in the same tick") to keep future failures clear and less tied to a specific underlying implementation detail.
Test262 conformance changes
Tested main commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4886 +/- ##
===========================================
+ Coverage 47.24% 57.29% +10.05%
===========================================
Files 476 555 +79
Lines 46892 60602 +13710
===========================================
+ Hits 22154 34722 +12568
- Misses 24738 25880 +1142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let mut jobs_to_keep = timeouts_borrow.split_off(&now); | ||
| // `split_off` keeps keys `>= now`. We want exactly `now` to execute in the | ||
| // current tick, so we move it back into `timeouts_borrow` (which becomes `jobs_to_run`). | ||
| if let Some(jobs_at_now) = jobs_to_keep.remove(&now) { |
There was a problem hiding this comment.
If there is more than 1 job due now, this would only run one of them, right>
There was a problem hiding this comment.
No — it will run all of them.
timeout_jobs is a BTreeMap<JsInstant, Vec<TimeoutJob>>, so remove(&now) returns the entire vector of jobs scheduled at that timestamp, and the executor then iterates over all of them.
|
Might be superseeded by #4891 |
After seeing the [avalanche](#4886) [of](#4833) [issues](#4785) [that](#4749) [our](#4782) executors have, I don't think it's worth being too smart about when to exit the loop. Folks who wanna have that behaviour can just implement their own `JobExecutor`. Thus, this reverts the implementation to the old behaviour of blocking when having pending timeout and interval jobs. Fortunately, `run_jobs_async` exists, so we can use it in our CLI to intersperse executing jobs with executing parsing and execution. And just for good measure, it also adds a `stop` cancellation token to `SimpleJobExecutor`, in case folks still want to use it but that also want to remotely stop the event loop's execution from another thread.
|
Superseeded by #4891 |
This PR fixes two issues in
SimpleJobExecutor.nowcould be deferred due to thebehavior of
BTreeMap::split_off.did not verify
job.is_cancelled()before dispatch.Fix:
nowback into the execution set.Two regression tests were added:
All tests pass locally (
cargo test,cargo clippy).