diagnostics_channel: pass non-thenable tracePromise return values through

[robobun]

tracingChannel().tracePromise(fn, ctx) with a function that returns a non-thenable wrapped the return value in a promise and published a spurious asyncStart/asyncEnd pair.

Node's documented contract is that a non-promise return value is passed through as-is and only the sync events are published; the async phase is reserved for actual promise settlement. This matters because diagnostics_channel instrumentation wraps existing functions that may have a sync fast path, and on Bun that wrapper changed the function's return type from a value to a promise.

Repro

const dc = require("node:diagnostics_channel");

const tc = dc.tracingChannel("repro:tp");
const events = [];
for (const n of ["start", "end", "asyncStart", "asyncEnd", "error"]) tc[n].subscribe(() => events.push(n));

const ret = tc.tracePromise(() => 42, {});
console.log("returned:", ret && ret.constructor.name, ret);
setTimeout(() => console.log("published events:", events.join(",")), 20);

node: returned: Number 42         published events: start,end
bun : returned: Promise Promise   published events: start,end,asyncStart,asyncEnd

Cause

tracePromise coerced the callback's return value with Promise.resolve() before chaining onto it:

let promise = fn.$apply(thisArg, args);
// Convert thenables to native promises
if (!(promise instanceof Promise)) {
  promise = PromiseResolve(promise);
}
return PromisePrototypeThen(promise, resolve, reject);

So every return value became a promise, and resolve/reject (which publish the async events) always ran.

Fix

In src/js/node/diagnostics_channel.ts, branch on whether the return value is thenable:

• non-thenable: set context.result, publish end, return the value unchanged, and emit the same process.emitWarning node emits (tracePromise was called with the function 'x', which returned a non-thenable.)
• thenable: call .then(resolve, reject) on it directly, which preserves the type of custom thenables rather than replacing them with a native promise

The now-unused PromiseResolve binding is removed.

Verification

Six tracePromise tests added to test/js/node/diagnostics_channel/diagnostics_channel.test.ts, covering the non-thenable return, undefined return, native promise, custom thenable, rejected promise, and a synchronous throw. Every assertion was first run against node v26.3.0 (the version Bun reports as process.versions.node) so the expected return values, event orderings, context contents, and warning text match node exactly.

Three of the six fail on current main; all six pass with the fix. The vendored test-diagnostics-channel-* tests in test/js/node/test/parallel/ still pass.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 2 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: 4cf2dda1-6ba6-4e17-9309-f26be4a944e3

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and 2f2f873.

📒 Files selected for processing (2)

• src/js/node/diagnostics_channel.ts
• test/js/node/diagnostics_channel/diagnostics_channel.test.ts

---

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

[robobun]

^{Updated 8:45 AM PT - Jul 6th, 2026}

⏳ @robobun, your commit 2f2f873 is still building in Build #69079, but has 1 failures so far (All Failures):

• package.json - code 1 on 🐧 25.04 aarch64

[github-actions]

This PR may be a duplicate of:

1. node:diagnostics_channel: sync with Node 26 + subsystem channels #32628 - Broader "sync with Node 26" PR that includes the same tracePromise non-thenable pass-through fix and emitNonThenableWarning implementation

🤖 Generated with Claude Code

[claude]

LGTM — small, well-tested Node compat fix verified against upstream behavior.

Extended reasoning...

Overview

This PR fixes tracingChannel().tracePromise() in src/js/node/diagnostics_channel.ts to match Node.js semantics: when the wrapped function returns a non-thenable, the value is now passed through unchanged (with a process.emitWarning) instead of being coerced to a Promise and triggering spurious asyncStart/asyncEnd events. The logic change is ~10 lines; the rest is six new tests in test/js/node/diagnostics_channel/diagnostics_channel.test.ts replacing a test.todo placeholder.

Security risks

None. This is pure-JS diagnostics instrumentation with no auth, crypto, filesystem, or network surface. The only new side effect is a process.emitWarning call, which mirrors Node.

Level of scrutiny

Low-to-moderate. It's a focused Node-compat behavioral fix in a built-in JS module — no native code, no memory management, no cross-platform concerns. The previous behavior (always wrapping in Promise.resolve()) was clearly divergent from Node, and the new thenable check (typeof result?.then !== 'function') plus direct .then() invocation is the standard pattern Node uses. The finally block still publishes end on the non-thenable path, so event ordering is preserved.

Other factors

• Test coverage is thorough: non-thenable, undefined, native promise, custom thenable (verifying return type is preserved), rejected promise, and synchronous throw — with the PR description stating every assertion was cross-checked against Node v26.3.0.
• The now-unused PromiseResolve binding is cleaned up.
• No CODEOWNERS entry covers this path, no outstanding reviewer comments, and the bug-hunting system found no issues.

[robobun]

The duplicate flag is accurate: #32628 (draft) contains this same tracePromise change as one piece of a full Node 26 port of diagnostics_channel (BoundedChannel, RunStoresScope, and the module/net/child_process/worker_threads subsystem channels, +3305/-185 across 58 files).

The two converge on identical semantics for this method: the same typeof result?.then !== "function" predicate, the same warning text, and the same result.then(resolve, reject) pass-through so custom thenables keep their type.

What differs is scope and state. #32628 has been in draft since June 23, was last pushed June 29, and currently conflicts with main. This PR is ~10 lines in one method plus six tests.

Happy to close this one if #32628 is close to landing. If it isn't, this fixes the reported behavior on its own, and whichever merges second should be a trivial conflict confined to that one method.

[claude]

LGTM — small, well-tested Node compat fix; the cookie-map CI failure is unrelated to this change.

Extended reasoning...

Overview

This PR changes ~10 lines in TracingChannel.prototype.tracePromise in src/js/node/diagnostics_channel.ts to match Node.js semantics: when the traced function returns a non-thenable, the value is passed through unchanged (with only start/end published and a process.emitWarning fired), and when it returns a thenable, .then() is called on it directly rather than coercing through Promise.resolve(). The now-unused PromiseResolve binding is removed. Six new tests replace the test.todo placeholder in the TracingChannel describe block, covering non-thenable, undefined, native promise, custom thenable, rejected promise, and synchronous throw — all cross-checked against Node v26.3.0 per the PR description.

Security risks

None. diagnostics_channel is an observability/instrumentation API with no auth, crypto, filesystem, or network surface. The change narrows behavior (stops wrapping values in promises) rather than expanding it.

Level of scrutiny

Low-to-moderate. This is a focused Node-compat correction in a built-in JS module, directly mirroring the upstream Node implementation (same typeof result?.then !== "function" predicate, same warning text). The duplicate-PR bot confirmed #32628 uses identical semantics for this method, which independently corroborates the approach. The finally { end.publish(context) } block is unchanged, so the sync-path event ordering (start, end) is preserved. Optional chaining on result?.then correctly handles null/undefined returns.

Other factors

• The CI failure is test/js/bun/cookie/cookie-map.test.ts, which is unrelated to this diff and stems from the base branch (commit 48ff9eb on main touched cookie-map assertions).
• The overlap with draft PR #32628 has already been surfaced and addressed by the author in-thread; whichever lands second has a trivial one-method conflict. That's a merge-order call, not a correctness concern.
• Tests use unique channel names per case, clean up the warning listener in finally, and assert exact event orderings and warning text — good coverage of the variant matrix.
• No CODEOWNERS entry for this path.

[claude]

LGTM — small, well-tested Node compat fix that mirrors upstream tracePromise semantics.

Extended reasoning...

Overview

This PR touches two files: src/js/node/diagnostics_channel.ts (~10 lines of logic in TracingChannel.prototype.tracePromise plus a small emitNonThenableWarning helper and removal of the unused PromiseResolve binding) and test/js/node/diagnostics_channel/diagnostics_channel.test.ts (six new tests replacing a test.todo placeholder). The fix changes tracePromise to detect non-thenable return values via typeof result?.then !== "function", pass them through unchanged with only the sync start/end events published, and emit the same process.emitWarning text Node emits. Thenables now have .then() called directly, preserving custom thenable types instead of coercing to a native Promise.

Security risks

None. This is pure JavaScript in a Node compat module with no auth, crypto, filesystem, or network surface. The only new observable side effect is a process.emitWarning call, which mirrors Node.

Level of scrutiny

Low-to-moderate. It's a Node.js compatibility fix in a built-in JS module — the reference implementation is Node itself, and the PR description states every assertion was verified against Node v26.3.0. The logic is a straightforward branch on thenable-ness; the finally { end.publish(context) } block is unchanged so the sync event ordering is preserved on all paths (non-thenable, thenable, and sync throw). No native code, no memory or GC concerns.

Other factors

Test coverage is thorough: non-thenable value, undefined, native promise, custom synchronous thenable, rejected promise, and synchronous throw — each asserting exact event ordering, context contents, and warning text. Tests clean up the process.on("warning") listener in finally. The overlap with draft PR #32628 has already been surfaced and addressed by the author; whichever lands second is a trivial conflict in one method. No CODEOWNERS entry covers this path and no outstanding reviewer comments are pending.

[robobun]

CI status: the diff is green, the red lanes are unrelated

Build 69079 finished at 242 passed, 1 failed. test/js/node/diagnostics_channel/diagnostics_channel.test.ts runs and passes in CI (12 pass, 0 fail), and no failure in any of the three builds this PR has seen touches code it changes. Details, since three bounces is enough to look suspicious:

Build 68851 (1a388993): test/js/bun/cookie/cookie-map.test.ts failed on every lane. The branch point sat between 028f210 (made Bun.Cookie emit an IMF-fixdate) and 48ff9eb (updated the cookie-map assertions to match), so the branch had the runtime change without the test update. Fixed by rebasing onto current main; the failure is gone.

Build 69053 (2b1d0db2): test/js/node/test/sequential/test-net-listen-shared-ports.js segfaulted at 0x1A600000012 on windows 11 aarch64, 1 of 115 completed jobs. It's a cluster.fork() + shared-port EADDRINUSE + worker.kill() teardown race, and the crashing process never loads diagnostics_channel (its builtins are cluster, net, child_process, worker_threads). Nothing in src/ calls tracePromise. It did not recur.

Build 69079 (2f2f8731): the only failing job, ubuntu 25.04 aarch64 - test-bun, died during dependency install before a single test ran:

error: failed to download bun-tracestrings@github:oven-sh/bun.report#912ca63: 404 Not Found
https://api.github.com/repos/oven-sh/bun.report/tarball/912ca63

bun-tracestrings is a pre-existing devDependency in the root package.json, which this PR does not modify. The 43 expired jobs are ones that never got an agent once the build was already marked failed.

Tests that failed and then passed on retry in that build (package.js, bun-install, proxy-stress-adversarial, webview-chrome, napi, net-mongodb-pattern-leak, in-process-cron) are all unrelated as well; none of them mention diagnostics_channel.

I've used my one re-roll (the ci: retrigger commit), so I'm not going to keep pushing empty commits. The diff is two files: ~10 lines in TracingChannel.prototype.tracePromise plus six tests. Happy to rebase again if that helps, but this needs a maintainer to take it from here.