fix(cli): print the update-available notice once, not on every event-loop drain#1104
Conversation
…loop drain
`process.on("beforeExit", ...)` re-fires every time the event loop
drains, and the handler kicks off a fire-and-forget async telemetry
flush — so on a successful command the user sees the
"Update available: …" notice twice (once after the initial drain, again
after the flush settles). Using `process.once` detaches the listener
after first invocation, fixing the double-print and also preventing a
double-flush of telemetry.
Reported during local testing of `auth login`, but the bug affects every
command (any path where `_flush()` schedules work).
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean fix — process.once is exactly right for the re-entrant beforeExit drain loop. No issues.
vanceingalls
left a comment
There was a problem hiding this comment.
Review
Design is correct. James's diagnosis matches Node's beforeExit semantics exactly: the event fires each time the event loop drains; _flush().catch(...) schedules a microtask continuation that is new event-loop work, so a plain on listener re-fires on every drain until the process actually terminates — causing the double (or infinite) update banner. process.once detaches after first invocation, which is the right primitive and the minimal fix.
Hyperframes pattern checklist:
- Not in
packages/engine, so the ESMrequire()trap is N/A. beforeExithandler uses captured references (_flush?.(),_printUpdateNotice?.()) — not dynamicimport()inside the handler. Pattern is correctly followed, matching the existingexit/uncaughtExceptionhandlers nearby. Clean.- No GSAP. N/A.
One nit (non-blocking):
With on → once, if the first beforeExit fires before the import("./utils/updateCheck.js") dynamic import has resolved and written _printUpdateNotice, the listener is permanently detached and the notice never prints for that run. With the old on listener a late drain after the import settled would have caught it. In practice this is only a concern for commands that drain the event loop so fast that no async work is in flight at all — extremely unlikely on any real command, and the auth-stack path that triggered this report is definitely safe. Not a blocker, just worth knowing.
No unit test: James's justification is solid — beforeExit re-fire behavior is a Node contract, the fix is structural, and a regression would be visually obvious. Agree, nothing to add here.
CI: A few checks (CLI smoke, Windows tests, global-install smoke) were still pending at review time — assuming they pass, this is good to merge.
LGTM. Ship it.
— Vai
What
process.on("beforeExit", …)re-fires every time the event loop drains. The listener kicks off a fire-and-forget async telemetry flush, which schedules new work — so on a successful command the user sees the "Update available: …" notice twice (once after the initial drain, again after the flush settles, and so on).Switched to
process.onceso the listener detaches after first invocation. Side benefit: prevents the double-flush of telemetry on the same path.Why
Reported during local testing of the auth stack (#1081 / #1084), but the bug affects every command — any code path where
_flush()schedules work (which is, by design, every non---jsoninvocation when telemetry is enabled).User-visible symptom:
Test plan
bun run --cwd packages/cli test): 446/446 green.bunx oxlint,bunx oxfmt --check,bunx tsc --noEmit -p packages/cli/tsconfig.json,bunx fallow audit --base origin/mainall clean.Not adding a unit test for the listener wiring itself —
beforeExitre-fire behavior is a Node-level contract that's awkward to mock cleanly and theoncesemantic is structural; future regressions would just bring the double-print back, which is visually obvious.Out of scope
Other commands in the auth stack (#1081 / #1084) — unrelated; this is a separate, narrowly-scoped fix off
mainso it can ship/merge independently.