Skip to content

timers: re-capture the async context when refresh() reactivates a fired timer - #33092

Open
robobun wants to merge 5 commits into
mainfrom
farm/195caeb2/timeout-refresh-als-context
Open

robobun wants to merge 5 commits into
mainfrom
farm/195caeb2/timeout-refresh-als-context

Conversation

@robobun

@robobun robobun commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

timeout.refresh() on a timer whose callback has already run reactivates the timer, but in Bun the callback kept firing in the AsyncLocalStorage context the timer was created in. Node re-binds it to the context of the refresh() caller.

const { AsyncLocalStorage } = require("async_hooks");
const als = new AsyncLocalStorage();
let t;
als.run("A", () => { t = setTimeout(() => console.log(als.getStore()), 1); });
setTimeout(() => { als.run("B", () => t.refresh()); }, 50);
// node: prints "A" then "B"
// bun:  prints "A" then "A"

Against node v26.3.0 (same result with --no-async-context-frame):

scenario node bun
refresh a still-pending timer from B creator's context creator's context
refresh an already-fired timer from B B creator's context
refresh an already-fired timer outside any context undefined creator's context
created with no context, fired, refreshed from B B undefined
refresh() inside the timer's own callback creator's context creator's context

This is the idle/keep-alive timeout idiom (refresh() from per-request contexts onto a connection-scoped timer), so tracing code instrumented for node attributes the timeout to the wrong context under Bun.

Cause

Node's Timeout.prototype.refresh() goes through insertGuarded, which calls initAsyncResource only when the timeout is destroyed (its callback already ran) or has no async id. initAsyncResource stores AsyncContextFrame.current() on the resource, so reactivating a fired timer snapshots the refresh-time context. A still-pending timer is never re-initialized.

In Bun the creation-time context is baked into the AsyncContextFrame wrapper stored as the Timeout's callback, and do_refresh() never touched it.

Fix

When do_refresh() runs on a timer that is already destroyed (fired and not cleared), unwrap the stored AsyncContextFrame and re-snapshot whatever async context is active at the refresh() call via a new AsyncContextFrame__recaptureAsyncContextIfNeeded helper. Pending timers and refresh() from inside the timer's own callback are unchanged, matching Node in both cases.

Verification

  • test/js/node/async_hooks/async-context/async-context-timers-refresh.js is picked up by AsyncLocalStorage-tracking.test.ts, which runs every fixture under both bun and node and requires exit 0 from both. It fails on current Bun (["creator","creator","creator","creator"] instead of ["creator","refresher",null,"again"]) and passes with the fix.
  • bun bd test test/js/node/async_hooks/ passes (113 tests, including the node cross-run).

…ed timer

Node re-initializes a Timeout's async resource when refresh() is called on a
timer whose callback has already run (lib/internal/timers.js insertGuarded),
so the reactivated timer observes the AsyncLocalStorage context of the
refresh() caller. Bun kept the creation-time AsyncContextFrame forever.

do_refresh() now unwraps the stored AsyncContextFrame and re-snapshots the
currently active async context when the timer is already destroyed. Pending
timers and refresh() from inside the timer's own callback are unchanged, which
also matches Node.
@robobun

robobun commented Jun 29, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:15 PM PT - Jun 29th, 2026

❌ @robobun, your commit 363b8ac has 2 failures in Build #66882 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33092

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

bun-33092 --bun

@mintlify

mintlify Bot commented Jun 29, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bun 🟢 Ready View Preview Jun 29, 2026, 5:42 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai

coderabbitai Bot commented Jun 29, 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: 7 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: fd628757-e6d8-4db5-a069-f6db7fe0c363

📥 Commits

Reviewing files that changed from the base of the PR and between 60b24d8 and 363b8ac.

📒 Files selected for processing (4)
  • src/jsc/bindings/AsyncContextFrame.cpp
  • src/jsc/bindings/NodeTimerObject.cpp
  • test/js/node/async_hooks/AsyncLocalStorage.test.ts
  • test/js/node/async_hooks/async-context/async-context-timers-refresh.js

Walkthrough

Adds AsyncContextFrame__recaptureAsyncContextIfNeeded as a C++ FFI function and a Rust JSValue wrapper that re-snapshots the active async context. TimerObjectInternals::do_refresh calls this when the timer is already destroyed, so refresh() captures the caller's AsyncLocalStorage context. Two test files verify the context sequence. Two doc files receive formatting-only fixes.

Changes

AsyncLocalStorage context recapture for Timeout#refresh

Layer / File(s) Summary
C++ FFI: recaptureAsyncContextIfNeeded
src/jsc/bindings/AsyncContextFrame.cpp
New extern "C" function AsyncContextFrame__recaptureAsyncContextIfNeeded that unwraps any existing AsyncContextFrame wrapper from the callback and re-wraps it with the current async context via AsyncContextFrame::withAsyncContextIfNeeded.
Rust JSValue binding
src/jsc/JSValue.rs
New #[inline] method recapture_async_context_if_needed on JSValue that declares and calls the FFI function, returning the updated JSValue.
Timer do_refresh logic
src/runtime/timer/timer_object_internals.rs
In do_refresh, when get_destroyed() is true, reads the current callback and writes back a version produced by recapture_async_context_if_needed before re-strongening this_value and rescheduling.
Tests
test/js/node/async_hooks/AsyncLocalStorage.test.ts, test/js/node/async_hooks/async-context/async-context-timers-refresh.js
Two new test files asserting the sequence of AsyncLocalStorage store values seen by a timer callback across refresh() calls in different contexts and outside any context.

Docs formatting fixes

Layer / File(s) Summary
Markdown formatting
docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx
Reformats the btoa/atob warning code fence and the Web APIs support table whitespace without changing content.

Possibly related PRs

  • oven-sh/bun#33040: Touches the same docs/guides/util/base64.mdx and docs/runtime/web-apis.mdx files with base64/btoa/atob documentation and web-API reference edits.

Suggested reviewers

  • dylan-conway
  • Jarred-Sumner
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main behavior change to async context recapture on fired timer refresh.
Description check ✅ Passed The description is detailed and covers the problem, fix, and verification, though it uses custom headings instead of the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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/js/node/async_hooks/AsyncLocalStorage.test.ts`:
- Around line 129-166: The AsyncLocalStorage timer refresh test covers pending,
after-fired, and unbound refresh paths, but misses the in-callback `refresh()`
case in `AsyncLocalStorage.test.ts` around the `setTimeout().refresh()`
scenario. Extend the existing `callback`/`onFire` flow so one invocation calls
`t.refresh()` before returning, then assert the subsequent rearmed execution
preserves the current store value rather than rebinding or clearing it. Keep the
new coverage aligned with the existing `s.run(...)`, `t.refresh()`, and `seen`
assertions so the behavior change is locked down in the same test.
🪄 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: 706d6e01-8918-47db-a3c1-fae7bccb3392

📥 Commits

Reviewing files that changed from the base of the PR and between fb24aac and 60b24d8.

📒 Files selected for processing (7)
  • docs/guides/util/base64.mdx
  • docs/runtime/web-apis.mdx
  • src/jsc/JSValue.rs
  • src/jsc/bindings/AsyncContextFrame.cpp
  • src/runtime/timer/timer_object_internals.rs
  • test/js/node/async_hooks/AsyncLocalStorage.test.ts
  • test/js/node/async_hooks/async-context/async-context-timers-refresh.js

Comment thread test/js/node/async_hooks/AsyncLocalStorage.test.ts
A refresh() issued while the callback is still running must keep the context
the timer is already bound to (the timeout is not destroyed yet), in Bun and
in Node. Exercises the in_callback branch of get_destroyed() that do_refresh
relies on.
Comment thread src/runtime/timer/timer_object_internals.rs
`timeout._onTimeout` is user-assignable. Wrapping a non-callable in an
AsyncContextFrame made it truthy, so fire() no longer treated falsy values
as cleared, and the not-a-function error path in Bun__JSTimeout__call
returned before restoring the async context, leaving the refresh() caller's
AsyncLocalStorage context installed globally.

Only re-wrap values the timer can invoke (a callable or a Bun.sleep
promise), and restore the async context before that early return.
@robobun

robobun commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for 363b8ac (build 66882): every failing lane is unrelated to this change.

  • darwin-26-aarch64 test-bun (the red X): the job and its retry both exited before running a single test with buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun', on the same agent (darwin-aarch64-26-5-1-1). Build 66866 on this PR failed the same way.
  • test/js/node/test/parallel/test-net-connect-memleak.js on the two Alpine lanes: this assertion (collected not true after gc()) is currently failing across unrelated branches, with the same error annotation in 14 of the last 40 Buildkite builds (66905, 66904, 66899, 66896, 66891, 66884, 66879, ...). The test involves no timers and no AsyncLocalStorage (async context tracking is never enabled in it), and it passed on every non-musl lane in this same build.
  • test/js/bun/util/v8-heap-snapshot.test.ts on ubuntu 25.04 x64-baseline: "main process killed by SIGKILL but no core file found", no failing assertion. Also appears on other branches (for example build 66893).
  • The warning-level annotations (bun-install-registry, spawn-stdin-readable-stream, napi on Windows) were auto-retried flakes.

Every lane that ran the suites covering this change is green, including the debian-13 x64 ASAN lane; test/js/node/async_hooks/ passes on all of them. The review threads from CodeRabbit and Claude are addressed and resolved. This is ready for review.

@robobun

robobun commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

Stale PR review: keep open, rework.

The behaviour is wanted, and main still differs from Node. On 1.4.3-canary (367d939), a timer created in als.run("A") and refreshed from als.run("B") after it fired prints ["A","A"]. Node v26.3.0 prints ["A","B"]. User code does not need to call refresh() to hit this. A socket.setTimeout() idle timer that fired once is re-armed by a later socket.write() (src/js/node/net.ts:2734). The next timeout event then runs in the first context on Bun and in the writer's context on Node. No other open PR fixes this.

The diff is not the shape to merge:

  • It conflicts with main in src/jsc/JSValue.rs, src/jsc/bindings/AsyncContextFrame.cpp and two unrelated docs files from an autofix commit (docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx). Drop the docs files.
  • The new C++ export AsyncContextFrame__recaptureAsyncContextIfNeeded is redundant. Main already has JSValue::without_async_context() (src/jsc/JSValue.rs:1674, backed by AsyncContextFrame__callbackOf) next to the existing wrap helper. The whole fix can live in do_refresh (src/runtime/timer/timer_object_internals.rs:990).
  • The test "does not re-bind a non-callable _onTimeout" passes on the unpatched build, and it waits with setTimeout(..., 20). Remove it, or make it fail without the fix.
  • Since Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590 (merged 2026-09-17), a Bun.ModuleGraph context rides the async context (src/jsc/bindings/ModuleGraph.cpp:244). A timer keeps the context id from its creation (timer_object_internals.rs:306). A re-capture of the whole frame on refresh() can move the callback of a timer into the context of another graph. The rework needs a guard and a test for that case.
  • timers: keep the async context on the Timeout, not in the callback slot #41194 and fix(node:async_hooks): report timer lifecycles #42621 are open and edit the same code. timers: keep the async context on the Timeout, not in the callback slot #41194 removes the AsyncContextFrame wrapper that this diff unwraps. State the landing order in the PR body.

The do_refresh hunk, the fixture async-context-timers-refresh.js and the first test still apply to main.

This branch was successfully deployed

1 active (outdated) deployment
staging - docs — d1de4e15 Deployed Jun 29, 2026 by mintlify[bot]
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.

1 participant