Skip to content

Fix due-now timeout jobs in executor and CLI#4785

Closed
Nakshatra480 wants to merge 13 commits intoboa-dev:mainfrom
Nakshatra480:fix-timeout-now
Closed

Fix due-now timeout jobs in executor and CLI#4785
Nakshatra480 wants to merge 13 commits intoboa-dev:mainfrom
Nakshatra480:fix-timeout-now

Conversation

@Nakshatra480
Copy link
Copy Markdown
Contributor

@Nakshatra480 Nakshatra480 commented Mar 1, 2026

Closes #4782

Summary

Fixes timeout jobs scheduled at the current instant so setTimeout(0) does not get skipped. Ensures due-now jobs are drained and pending checks treat equality as due.

Changes

  • Drain timeout jobs at now instead of treating them as future.
  • Update pending timeout checks to include equality.

Tests

  • cargo test
  • cargo fmt -- --check
  • cargo clippy --all-features --all-targets

Fix due-now timeout jobs by draining equality, prevent premature exit…
1ff1185 
…s, and align CLI/examples across executors with corrected scheduling semantics logic
@Nakshatra480 Nakshatra480 requested a review from a team as a code owner March 1, 2026 16:02
@Nakshatra480 Nakshatra480 marked this pull request as draft March 1, 2026 16:13
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2026 •
edited
Loading

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,666 49,666 0
Ignored 2,284 2,284 0
Failed 1,013 1,013 0
Panics 0 0 0
Conformance 93.77% 93.77% 0.00%

Tested main commit: 5cbf23323e1be945022cd1e6332cbacc22e9a2c8
Tested PR commit: cc8ab1fed07bd6452e662ec0b90b355f9dfa1e28
Compare commits: 5cbf233...cc8ab1f

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 1, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 44.44444% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.05%. Comparing base (6ddc2b4) to head (cc8ab1f).
⚠️ Report is 762 commits behind head on main.

Files with missing lines Patch % Lines
cli/src/main.rs 0.00% 9 Missing ⚠️
core/engine/src/job.rs 88.88% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4785      +/-   ##
==========================================
+ Coverage   47.24%   57.05%   +9.81%     
==========================================
  Files         476      555      +79     
  Lines       46892    60613   +13721     
==========================================
+ Hits        22154    34585   +12431     
- Misses      24738    26028    +1290

☔ 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.

@Nakshatra480 Nakshatra480 marked this pull request as ready for review March 1, 2026 16:39
@Nakshatra480
Copy link
Copy Markdown
Contributor Author

Nakshatra480 commented Mar 1, 2026 •
edited
Loading

Hey @jedel1043 @nekevss could you please take a look and review?

  • I tightened the timeout handling so due‑now jobs are always drained instead of being skipped.
  • The logic is aligned in both the executor and the CLI to keep behavior consistent.
  • This avoids missing immediate timeouts while keeping the change minimal and safe.
  • Test262 results are unchanged overall, with one previously failing test now passing.

@Nakshatra480 Nakshatra480 marked this pull request as draft March 1, 2026 17:40
Fix due-now timeout jobs in example event loops
a974570 
Apply the same split_off(&now) fix to smol_event_loop.rs and
tokio_event_loop.rs so that timeout jobs scheduled at exactly
are drained instead of being kept as future jobs.
@Nakshatra480 Nakshatra480 marked this pull request as ready for review March 1, 2026 18:11
@zhuzhu81998
Copy link
Copy Markdown
Contributor

zhuzhu81998 commented Mar 1, 2026

Test262 results are unchanged overall, with one previously failing test now passing.

That one test (test/staging/sm/Date/toISOString-01.js) is actually fixed by #4776. It is just that the main branch conformance results is ran only once 24h and you presumably have #4776 already included in your pr.

@Nakshatra480
Copy link
Copy Markdown
Contributor Author

Nakshatra480 commented Mar 1, 2026

Test262 results are unchanged overall, with one previously failing test now passing.

That one test (test/staging/sm/Date/toISOString-01.js) is actually fixed by #4776. It is just that the main branch conformance results is ran only once 24h and you presumably have #4776 already included in your pr.

Ah, that makes sense! I was briefly confused when I saw that result. Thanks for the heads up and clarifying how the main branch conformance reporting works.

@Nakshatra480
Copy link
Copy Markdown
Contributor Author

Nakshatra480 commented Mar 1, 2026

Hi @jedel1043 i have fixed the due-now timeout jobs. Can you pls review this PR.
Thanks for your time😊

@hansl
Copy link
Copy Markdown
Contributor

hansl commented Mar 5, 2026

I'd like to see a test for this PR. Do you think you could make one real quick?

@Nakshatra480 Nakshatra480 marked this pull request as draft March 5, 2026 21:59
@Nakshatra480 Nakshatra480 marked this pull request as ready for review March 5, 2026 22:06
@Nakshatra480
Copy link
Copy Markdown
Contributor Author

Nakshatra480 commented Mar 5, 2026

Hey @hansl the due-now timeout jobs are fixed and also added a test for this as per your feedback. Can you review this PR now?
Thank you

@jedel1043 jedel1043 mentioned this pull request Mar 6, 2026
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 6, 2026
@jedel1043
Simplify executor implementations (#4891)
51e910f 
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.
@jedel1043
Copy link
Copy Markdown
Member

jedel1043 commented Mar 6, 2026

Superseeded by #4891

@jedel1043 jedel1043 closed this Mar 6, 2026
@Nakshatra480 Nakshatra480 deleted the fix-timeout-now branch March 6, 2026 22:58
@Nakshatra480
Copy link
Copy Markdown
Contributor Author

Nakshatra480 commented Mar 6, 2026

Superseeded by #4891

Sorry for wasting your time😭

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

timeout jobs scheduled for now can be skipped (setTimeout(0) case)

4 participants

@Nakshatra480 @zhuzhu81998 @hansl @jedel1043