Skip to content

fix(executor): ignore recurring TimeoutJobs in is_empty#4956

Closed
ashnaaseth2325-oss wants to merge 1 commit intoboa-dev:mainfrom
ashnaaseth2325-oss:fix/promise-microtask-checkpoint
Closed

fix(executor): ignore recurring TimeoutJobs in is_empty#4956
ashnaaseth2325-oss wants to merge 1 commit intoboa-dev:mainfrom
ashnaaseth2325-oss:fix/promise-microtask-checkpoint

Conversation

@ashnaaseth2325-oss
Copy link
Copy Markdown

Summary

This PR fixes an issue where SimpleJobExecutor could fail to terminate when setInterval is active.

Previously, SimpleJobExecutor::is_empty() only checked whether the timeout_jobs map itself was empty:

self.timeout_jobs.borrow().is_empty()

Because setInterval continuously schedules new TimeoutJobs, the queue never became empty. As a result, the executor always believed work was pending and run_jobs() could never exit.

However, TimeoutJob already exposes a recurring flag intended to indicate jobs that should not block executor termination.

Fix

Two small adjustments connect the existing design to the executor logic.

First, the executor now ignores recurring timeout jobs when determining completion:

self.timeout_jobs
    .borrow()
    .values()
    .flatten()
    .all(TimeoutJob::is_recurring)

Second, the initial interval scheduling now uses TimeoutJob::recurring to ensure consistent behavior with later reschedules.

Result

run_jobs() now exits correctly once all finite (non-recurring) work has completed, while setInterval continues to function normally.

Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>
@jedel1043
Copy link
Copy Markdown
Member

This was the old behaviour, but it was patched up in #4891 to return to blocking if timeout jobs are still pending. It's much easier to maintain vs having to check all edge cases of termination.

@jedel1043 jedel1043 closed this Mar 8, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 8, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,713 49,710 -3
Ignored 2,262 2,262 0
Failed 988 991 +3
Panics 0 0 0
Conformance 93.86% 93.86% -0.01%
Broken tests (3):
test/built-ins/Atomics/waitAsync/true-for-timeout.js (previously Passed)
test/built-ins/Atomics/waitAsync/returns-result-object-value-is-promise-resolves-to-timed-out.js (previously Passed)
test/built-ins/Atomics/waitAsync/bigint/true-for-timeout.js (previously Passed)

Tested main commit: 5994303fbdc61f8cfa941a244d98ce6bfbd3ec5b
Tested PR commit: 121a7bbf559bd16c257975d6540a994678144e44
Compare commits: 5994303...121a7bb

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 8, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.81%. Comparing base (6ddc2b4) to head (121a7bb).
⚠️ Report is 782 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/job.rs 71.42% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4956       +/-   ##
===========================================
+ Coverage   47.24%   57.81%   +10.56%     
===========================================
  Files         476      557       +81     
  Lines       46892    61061    +14169     
===========================================
+ Hits        22154    35302    +13148     
- Misses      24738    25759     +1021     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ashnaaseth2325-oss
Copy link
Copy Markdown
Author

Thanks for the clarification!
I wasn't aware that #4891 intentionally restored the blocking behaviour when timeout jobs are pending. That makes sense regarding the edge cases around termination.
Appreciate the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants