Skip to content

event loop: count queued concurrent_tasks in is_event_loop_alive() - #36686

Closed
robobun wants to merge 2 commits into
mainfrom
farm/7f0d7c45/event-loop-concurrent-tasks-liveness
Closed

event loop: count queued concurrent_tasks in is_event_loop_alive()#36686
robobun wants to merge 2 commits into
mainfrom
farm/7f0d7c45/event-loop-concurrent-tasks-liveness

Conversation

@robobun

@robobun robobun commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes the residual silent exit in #11453.

Repro

(async () => {
  await new Promise(r => setImmediate(r));
  await crypto.subtle.digest("SHA-256", new Uint8Array(32));
  console.log("resolved");
})();

Before: prints nothing, exits 0. Node prints resolved.

The same gap is what makes @edgedb/generate intermittently exit 0 right after Connecting to database...: edgedb's rawConn._waitForMessage does sock.ref() / await / sock.unref() around each read, and the client runs SCRAM via crypto.subtle in between (Bun exposes a global crypto, so the client's adapter.crypto.node picks browserCrypto). With the TLS socket unref()'d during the HMAC/digest work, the only thing that should be holding the loop is the pending crypto.subtle task.

Cause

ScriptExecutionContext::postTaskTo routes through Bun__queueTaskConcurrently which pushes into EventLoop.concurrent_tasks and wakes the loop, but takes no ref. Two WebCrypto paths reach here without any other ref:

  • the <64 byte digest fast path in CryptoAlgorithmSHA{1,224,256,384,512,3}.cpp, which computes the hash synchronously on the main thread and posts the callback via postTaskTo;
  • the completion leg of dispatchAlgorithmOperation (used by sign/verify/encrypt/decrypt/large digests), where ConcurrentCppTask::run_owned runs unref_concurrently() after postTaskTo has enqueued the result; between the two there is a window where concurrent_ref == 0 with the result task sitting only in concurrent_tasks.

is_event_loop_alive_excluding_immediates() checked el.tasks (the drained FIFO), has_pending_refs() and the platform loop's active counter, but not el.concurrent_tasks. So the run loop could evaluate liveness as false and exit with the crypto.subtle promise still pending. run_command.rs already had a one-off || tick_concurrent_with_count() > 0 guard at the initial post-load check; the main while is_event_loop_alive() loop and the other callers did not.

Fix

Include !el.concurrent_tasks.is_empty() in is_event_loop_alive_excluding_immediates(). UnboundedQueue::is_empty is a single Acquire load of back, and pop_batch swaps back to null, so this is an accurate and cheap read from the main thread.

Verification

  • USE_SYSTEM_BUN=1 bun test test/regression/issue/11453.test.ts: 3 fail (empty stdout, exit 0)
  • bun bd test test/regression/issue/11453.test.ts: 3 pass
  • edgedb-shaped TLS loop (50 runs of tls.connect + ref/unref around reads + 4096 crypto.subtle.sign iterations): 43/50 before, 50/50 after

Fixes #11453

ScriptExecutionContext::postTaskTo (used by the small-input crypto.subtle
digest fast path and every WebCrypto work-queue completion) enqueues into
EventLoop.concurrent_tasks without a paired ref_concurrently(). The liveness
check only looked at the drained el.tasks FIFO and has_pending_refs(), so a
process could see zero and exit with a crypto.subtle promise still pending
in concurrent_tasks.

This is the residual behind #11453: edgedb's rawConn does
sock.ref()/await/sock.unref() around reads and runs SCRAM via crypto.subtle
in between (Bun exposing a global crypto makes the client pick its
browserCrypto adapter). With the TLS socket unref'd during the HMAC/digest
work, the only thing holding the loop was the WebCrypto task, which the
liveness check did not see.

Fixes #11453
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0472a4c6-5b6a-440f-90f0-f5e3d83a48b2

📥 Commits

Reviewing files that changed from the base of the PR and between b2b4444 and c4ac238.

📒 Files selected for processing (1)
  • test/regression/issue/11453.test.ts

Walkthrough

The VM event-loop liveness check now includes pending concurrent tasks. Regression tests verify that crypto.subtle.digest resolves after timer yields and socket reference changes.

Changes

Event-loop liveness

Layer / File(s) Summary
Count pending concurrent tasks
src/jsc/VirtualMachine.rs, test/regression/issue/11453.test.ts
The VM remains alive when concurrent_tasks is non-empty. Regression tests verify digest completion, clean subprocess output, exit code 0, and server cleanup.

Possibly related PRs

  • oven-sh/bun#36020: Both changes update event-loop liveness handling for concurrent tasks.
  • oven-sh/bun#36453: Both changes modify event-loop behavior in src/jsc/VirtualMachine.rs.
  • oven-sh/bun#36479: Both changes address pending concurrent tasks during asynchronous operations.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: counting queued concurrent tasks in event-loop liveness checks.
Description check ✅ Passed The description explains the bug, cause, fix, reproduction, and verification results, covering the template requirements.
Linked Issues check ✅ Passed The change directly addresses issue #11453 by preventing premature exit while WebCrypto concurrent tasks remain queued.
Out of Scope Changes check ✅ Passed The source change and regression tests are directly related to the event-loop liveness bug described in issue #11453.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/issue/11453.test.ts`:
- Around line 2-12: Remove the detailed regression and root-cause comments,
retaining only the issue URL comment in test/regression/issue/11453.test.ts.
Remove the fast-path rationale at lines 19-20 and socket-flow rationale at lines
59-61; no direct changes are needed beyond comment removal.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e533478f-7f4a-4715-be9c-d6c1647a6d1b

📥 Commits

Reviewing files that changed from the base of the PR and between a7838c5 and b2b4444.

📒 Files selected for processing (2)
  • src/jsc/VirtualMachine.rs
  • test/regression/issue/11453.test.ts

Comment thread test/regression/issue/11453.test.ts Outdated
@robobun

robobun commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:05 AM PT - Aug 1st, 2026

@robobun, your commit c4ac238 has 1 failures in Build #87038 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36686

That installs a local version of the PR into your bun-36686 executable, so you can run:

bun-36686 --bun

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. event_loop: stop re-draining thread-pool completions mid-tick #36548 - Contains the identical concurrent_tasks liveness fix in VirtualMachine.rs as a sub-fix within a larger event-loop ordering refactor

🤖 Generated with Claude Code

@robobun

robobun commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

The VirtualMachine.rs line here is the same one #36548 adds as part of its larger tick() re-drain refactor. That PR arrived at the same liveness fix from the test-fs-read-stream-pos.js ordering side and does not reference #11453.

Keeping this open as the minimal fix for #11453: it is a strict subset of #36548 on the source side, and the regression test here covers the deterministic <64-byte digest fast path and the edgedb sock.ref()/unref() pattern, which #36548's WebCrypto keep-alive test does not exercise. Whichever lands first, the other rebases cleanly; if #36548 lands first I'll rebase this down to the test only.

Comment thread src/jsc/VirtualMachine.rs
Comment thread test/regression/issue/11453.test.ts Outdated
@robobun

robobun commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

CI on c4ac238 (build 87038): the new test/regression/issue/11453.test.ts passed on every lane. All red is unrelated to this diff:

  • test/cli/install/bun-upgrade.test.ts on windows-aarch64: "Canary builds are not available for this platform yet" (the bun-windows-aarch64.zip release asset is missing); reported for main-break triage.
  • Everything else is tagged [flaky] by the scraper (passed on retry or when run alone): compile-windows-metadata, node-module-module, request-clone-leak, inspect-error-leak, elysia stream, 36577, 22650-shell-crash, 04011, tty-reopen-after-stdin-eof, and the ETXTBSY variant of bun-upgrade.

Ready for review; a retrigger will not clear the windows-aarch64 upgrade lane while the release asset is absent.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: the same one-line change (counting queued concurrent tasks in is_event_loop_alive_excluding_immediates) landed on main as part of #37075 on 2026-08-08.

test/regression/issue/11453.test.ts from this branch passes unmodified against current main (04148c8, debug build, 3 pass / 0 fail on repeated runs) and against the 1.4.0 canary (da3851e). Closing #11453 as well.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@edgedb/generate scripts not executing properly with bun version 1.1.10

2 participants