Conversation
…allback The returned function was a bare closure: .enabled was undefined (so the documented "if (log.enabled) expensive()" gate never ran), the optional second argument was ignored, and .name was "". Match the Node.js lib/internal/util/debuglog.js surface: return a "logger" function with an "enabled" accessor backed by the same NODE_DEBUG test, resolve the underlying impl lazily on first call, and pass it to the optional callback once (with its own .enabled and .name === "debug"). Output bytes are unchanged.
|
Status: diff is green; CI red on unrelated lanes. Reproduced with
Ready for a maintainer to merge. |
WalkthroughChangesThe debuglog behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:40 AM PT - Jul 18th, 2026
❌ @robobun, your commit fef1cd2 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 34606That installs a local version of the PR into your bun-34606 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit: making emitWarningIfNeeded lazy breaks src/js/node/http.ts:14, which calls require("node:util").debuglog("http") purely for the side-effect warning and discards the result. With this change the warning never fires, so test/js/node/test/parallel/test-http-debug.js will fail. That call site (and any similar ones) needs updating in the same PR.
Also checked: the if (enabled === false) short-circuit in logger when .enabled is read before the first call on a disabled section — verifiers concluded the callback-skip there is acceptable.
Extended reasoning...
The new debuglog defers debuglogImpl (and therefore emitWarningIfNeeded) until the returned logger is actually invoked, matching Node's lazy shape. But Bun's src/js/node/http.ts relies on the old eager behavior: it calls debuglog("http") at module load and throws away the return value, solely to trigger the "can expose sensitive data" process warning under NODE_DEBUG=http. After this PR that line becomes a no-op, and the existing Node-parallel test test-http-debug.js (which asserts the warning appears in stderr) will regress. This is a same-class sibling site that should be fixed alongside the util.ts change.
…till passes
The sensitive-data warning now fires in debuglog() itself (once per set),
not inside the lazily-resolved impl. src/js/node/http.ts creates
debuglog("http") at load time and discards it; with the warning deferred
to the first call it never fired.
Also guard the disabled-logger short-circuit so the optional callback is
still invoked once, and filter stderr for debuglog-prefixed lines instead
of asserting it is empty.
There was a problem hiding this comment.
LGTM — focused Node compat fix for util.debuglog(); the earlier stderr-empty nit was addressed in 3bca31f.
What was reviewed:
- Traced the new lazy-resolve/
loggerwrapper against Node'slib/internal/util/debuglog.js—.enabledaccessor,.name, and one-shot callback semantics match for both enabled and disabled sections. - Checked the shared
noopfor cross-section contamination: only disabled sections reach it, so redefining its.enabledgetter (alwaysfalse, configurable) is harmless. - Confirmed the eager
emitWarningIfNeeded(deduped viawarnedSets) keepstest/js/node/test/parallel/test-http-debug.jspassing givensrc/js/node/http.tsdiscards the returned logger. - Tests spawn with
bunEnv, drain both pipes, filter stderr by section prefix, and assert exit code last.
Extended reasoning...
Overview
This PR reworks util.debuglog() in src/js/node/util.ts (~50 lines) to match Node.js's lib/internal/util/debuglog.js: the returned function now has .name === "logger" and an .enabled accessor, and the optional second-argument callback is invoked once on first call with the resolved impl (.name === "debug", own .enabled). The stderr output format and NODE_DEBUG regex are unchanged. Two new subprocess tests in test/js/node/util/util.test.js cover the enabled and disabled paths.
Security risks
None. This is a pure-JS builtin-module change to a diagnostics helper. No untrusted-input parsing was added; set.toUpperCase() and debugEnvRegex.test are unchanged from before. The format.$apply thisArg changed from cjs_exports to undefined, which is inert since format doesn't use this.
Level of scrutiny
Medium — it rewrites a small builtin function rather than tweaking a line, so I traced the closure/reassignment flow carefully. The resolved && !enabled early-return short-circuits repeat noop calls; the debug self-reassignment on first call correctly swaps the initializer for the cached impl on subsequent enabled calls. The shared noop is only ever reached by disabled sections, so repeatedly defining a configurable .enabled getter returning false on it is safe. The only in-tree caller (src/js/node/http.ts:14) discards the return value, so the change from returning a cached function to a fresh logger wrapper per call has no internal fallout.
Other factors
The one deliberate deviation from Node — emitting the HTTP/HTTP2 warning eagerly at debuglog() time rather than on first log — is called out in the commit title and is required because Bun's http.ts never actually logs through the returned function; test-http-debug.js would otherwise regress. My earlier review comment about expect(stderr).toBe("") was addressed: the second test now filters for section-prefixed lines like the first. Tests follow harness conventions (bunEnv spread, concurrent pipe drain, exit-code asserted last) and the PR description reports USE_SYSTEM_BUN=1 fails / bun bd passes.
There was a problem hiding this comment.
LGTM — focused Node compat fix; the earlier stderr-empty nit is addressed.
What was reviewed:
debuglogrewrite against Node'slib/internal/util/debuglog.jsshape:.enabledaccessor,.name, lazy impl resolution, callback fired once for both enabled and disabled sections.emitWarningIfNeededtiming: still eager (matches pre-PR behavior viawarnedSetsdedup), sotest-http-debug.jsis unaffected.- Shared
noopfor disabled sections getting.enableddefined on it — all closures captureenabled=false, so redefines are idempotent. - Test subprocess pattern: pipes drained concurrently, stderr filtered for debuglog-prefixed lines only, exit code asserted last.
Extended reasoning...
Overview
Rewrites util.debuglog() in src/js/node/util.ts (~50 lines) to match Node's documented contract: the returned function now has .name === "logger" and an .enabled accessor, and the optional second-argument callback is invoked once on first call with the resolved impl (.name === "debug", own .enabled). Adds two subprocess tests in test/js/node/util/util.test.js covering enabled/disabled sections, callback contract, descriptor shape, and stderr output.
Security risks
None. This is a debug-logging helper gated on NODE_DEBUG; no auth, crypto, filesystem, or network surface. The existing emitWarningIfNeeded sensitive-data warning for HTTP/HTTP2 is preserved and still fires eagerly (deduplicated via a module-level warnedSets Set, replacing the old !debugs[set] guard that served the same purpose).
Level of scrutiny
Low-to-medium. Built-in JS module code, but the change is a straight port of Node's lib/internal/util/debuglog.js structure onto Bun's existing debugEnvRegex/format/console.error plumbing. The "%s %d: %s" output line is byte-identical to before. The only behavior-adjacent choice — keeping the HTTP warning eager rather than lazy like upstream Node — preserves Bun's pre-PR behavior and the existing test-http-debug.js assertion, which is the right call for a compat-fill PR.
Other factors
- I checked the shared module-level
noopused for all disabled sections: when a callback is supplied,.enabledisdefineProperty'd onto it with a getter closing overenabled. Sincenoopis only reached whenenabled === false, every such redefine installs an equivalent getter — no cross-section leakage. Node similarly shares a singlenoopfor disabled sections. format.$apply(cjs_exports, …)→format.$apply(undefined, …)is a no-op (formatignoresthis).- The prior inline review comment about
expect(stderr).toBe("")was addressed in 3bca31f — the disabled-section test now filters stderr forDBGSECT|NOTENABLED|ALSONOT-prefixed lines before asserting emptiness. - Tests follow harness conventions (
bunEnvspread,await using, concurrent pipe drain, exit code last) and the PR description confirmsUSE_SYSTEM_BUN=1fails /bun bdpasses.
…ently ASAN-debug child startup is slow enough that two sequential 5s-default tests timed out under gate load. Run both NODE_DEBUG variants in one test with a 20s timeout; assertions are unchanged.
There was a problem hiding this comment.
LGTM — the earlier stderr-empty concern was addressed in ad5d75a, and the remaining inline note is just the 20_000 test-timeout nit. The debuglog rewrite mirrors Node's lib/internal/util/debuglog.js closely and the sole internal caller (src/js/node/http.ts:14, which relies on the eager emitWarningIfNeeded) is unchanged in behavior.
Extended reasoning...
Overview
This PR rewrites util.debuglog() in src/js/node/util.ts to return a logger function with a working .enabled accessor and .name === "logger", and to invoke the optional second-argument callback once with the resolved impl (.name === "debug", own .enabled). It adds a subprocess-based test in test/js/node/util/util.test.js covering both the NODE_DEBUG=dbgsect and NODE_DEBUG="" paths, callback semantics, descriptor shape, and the util.debug === util.debuglog alias.
Security risks
None. Pure builtin-JS change; no native code, no new I/O surface, no user-controlled parsing beyond the pre-existing NODE_DEBUG regex which is unchanged.
Level of scrutiny
Medium — a ~50-line Node-compat rewrite of a low-risk diagnostic helper. I traced the closure/lazy-resolve flow: first call reassigns debug to the cached impl (or shared noop), fires the callback once, then applies; subsequent calls hit the resolved impl directly (or short-circuit when disabled). The shared noop and cached debugs[set] having .enabled redefined by multiple callbacks is safe because the property is configurable and enabled is deterministic per set. emitWarningIfNeeded remains eager (as it was before this PR), so src/js/node/http.ts's bare debuglog("http") call and test-http-debug.js are unaffected. Spot-checked existing Node parallel-test consumers (test-net-throttle.js, test-child-process-fork.js) — they use the plain const debug = util.debuglog('test'); debug(...) pattern, which the new logger wrapper handles identically.
Other factors
My prior review's expect(stderr).toBe("") concern was addressed — both subprocess results now filter stderr for debuglog-prefixed lines via the shared run() helper. The one remaining finding this run is the explicit 20_000 per-test timeout, which test/CLAUDE.md says not to set; it's a nit with no correctness impact. Test coverage is thorough (enabled + disabled, callback fires once, .name/.enabled on both the logger and the callback arg, descriptor shape, alias), pipes are drained concurrently, and bunEnv is spread when overriding NODE_DEBUG.
There was a problem hiding this comment.
LGTM — all three prior review rounds are addressed and the current revision traces correctly against Node's lib/internal/util/debuglog.js.
What was reviewed:
.enabledaccessor,.name, and callback-once semantics for both enabled and disabled sections; the sharednoopredefinition is safe (configurable: true, alwaysfalse).emitWarningIfNeededstill fires eagerly atdebuglog()time (pre-existing Bun behavior), sosrc/js/node/http.ts:14andtest-http-debug.jsare unaffected.- Test spawns drain stdout/stderr/exited concurrently, filter stderr for debuglog-prefixed lines only, and the timeout override is justified with a comment per the ASAN gate failure.
Extended reasoning...
Overview
Rewrites util.debuglog(set, cb) in src/js/node/util.ts to mirror Node's lib/internal/util/debuglog.js: returns a logger closure with a .name === "logger" and an .enabled accessor, lazily resolves the underlying impl on first call, and invokes the optional callback once with the resolved function (.name "debug" when enabled, "noop" when disabled). Adds a subprocess test in test/js/node/util/util.test.js covering both NODE_DEBUG states.
Security risks
None. debuglog is a diagnostic helper that writes formatted strings to stderr; no new external input is parsed, no auth/crypto/permissions surface is touched. The emitWarningIfNeeded path (HTTP/HTTP2 sensitive-data warning) is preserved with identical timing to the previous implementation.
Level of scrutiny
Moderate. This is a built-in JS module (src/js/), which REVIEW.md flags as hot-path code, but debuglog() itself is called once per section at module-load time and the returned logger short-circuits for disabled sections after the first call — matching Node's own allocation profile. The change is a focused Node-compat fix (documented .enabled was missing), not a redesign.
Other factors
This PR has been through three prior review iterations from me (stderr-empty assertion → filtered; per-test timeout → kept with justifying comment after an ASAN gate failure; disabled-path .name "debug" → "noop"), all resolved in the current head fef1cd2. I re-traced the lazy-resolve logic: the resolved && !enabled fast path is correct, debug is reassigned to the cached impl on first call so subsequent enabled calls go straight through, and redefining .enabled on the shared noop cannot throw because the descriptor is configurable: true and every disabled section closes over enabled === false. The only internal caller (src/js/node/http.ts:14) relies solely on the eager-warning side effect, which is preserved via the new warnedSets guard. Test coverage asserts the full contract (names, enabled flag, callback count, descriptor shape, util.debug alias, stderr output) for both NODE_DEBUG variants, and the bug-hunting pass found nothing new.
|
Superseded by #40353 from a community contributor. That PR is current with main and implements the same fix: the |
1 participant
util.debuglog()emitted byte-identical output to Node.js but returned a bare closure:.enabledwasundefined, the optional callback was never invoked, and.namewas"".Repro
.enabledis the documented gate for skipping expensive debug-only work;undefinedis falsy, so guarded diagnostics silently never run under Bun even withNODE_DEBUGset. The callback is the documented hook for caching the optimized logger.Fix
Mirror
lib/internal/util/debuglog.jsinsrc/js/node/util.ts:loggerfunction (.name === "logger") with anenabledaccessor (configurable,enumerable) driven by the existingNODE_DEBUGregex..name === "debug", own.enabledaccessor). The callback fires for disabled sections too, matching Node.emitWarningIfNeededand the"%s %d: %s"stderr line are unchanged.Function names are set via
Object.definePropertybecause the builtin bundler renames local bindings.Verification
New
util.debuglogcases intest/js/node/util/util.test.jsspawn a child with and withoutNODE_DEBUGand assert.name,.enabled, the callback contract, theenableddescriptor shape,util.debug === util.debuglog, and the stderr output.USE_SYSTEM_BUN=1 bun test test/js/node/util/util.test.js -t debuglog: 2 failbun bd test test/js/node/util/util.test.js: 195 pass[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file