refactor: replace Bun APIs with Node.js equivalents#984
Conversation
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 98702ba. Configure here.
| return await new Promise<number>((resolve) => { | ||
| proc.on("close", (code) => resolve(code ?? 1)); | ||
| proc.on("error", () => resolve(1)); | ||
| }); |
There was a problem hiding this comment.
Spawn errors silently swallowed, breaking retry logic
High Severity
In Node.js, child_process.spawn emits ENOENT and EBUSY as asynchronous 'error' events rather than throwing synchronously (unlike Bun.spawn). The proc.on("error", () => resolve(1)) handler silently resolves the promise with exit code 1, so the catch block's ENOENT-to-UpgradeError translation and EBUSY retry-with-backoff logic are now unreachable dead code. The function will return 1 instead of retrying on Windows antivirus locks or showing a user-friendly message for missing binaries. The error handler needs to reject instead of resolve so the catch block can inspect the error.
Reviewed by Cursor Bugbot for commit 98702ba. Configure here.
…ith Node.js APIs Replace all Bun-specific API calls in src/ with Node.js equivalents, continuing the Bun → Node.js migration. Groups replaced: - Bun.file().text/json/exists/stat/size/lastModified → node:fs/promises - Bun.write() → writeFile from node:fs/promises - Bun.write(dest, Bun.file(src)) → copyFile from node:fs/promises - Bun.which() → new src/lib/which.ts helper (uses 'command -v' on Unix) - Bun.spawn/spawnSync → spawn/spawnSync from node:child_process - Bun.sleep() → setTimeout from node:timers/promises - Bun.Glob → picomatch (already a devDependency) - Bun.randomUUIDv7() → uuidv7 package (already a devDependency) - Bun.semver.order() → semver.compare (already a devDependency) Remaining Bun APIs (Group D — to be addressed separately): - Bun.file().writer() in bspatch.ts and upgrade.ts - Bun.zstdCompress/DecompressSync in sourcemaps.ts and bspatch.ts - Bun.mmap() in bspatch.ts - Bun.CryptoHasher in bspatch.ts - bun:sqlite fallback in sqlite.ts (removed when tests migrate to Vitest) 30 files changed, 7012 tests pass, 0 failures.
98702ba to
cc0803a
Compare
Codecov Results 📊✅ 7012 passed | Total: 7012 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ❌ Patch coverage is 78.19%. Project has 14194 uncovered lines. Files with missing lines (8)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 77.12% 77.13% +0.01%
==========================================
Files 321 322 +1
Lines 61993 62061 +68
Branches 0 0 —
==========================================
+ Hits 47808 47867 +59
- Misses 14185 14194 +9
- Partials 0 0 —Generated by Codecov Action |
| * so that subsequent bare `sentry cli upgrade` calls use the same channel. | ||
| */ | ||
|
|
||
| import { spawn } from "node:child_process"; |
There was a problem hiding this comment.
Node.js spawn errors swallowed in Promise, making EBUSY retry and ENOENT detection dead code
Unlike Bun.spawn(), Node.js child_process.spawn() never throws synchronously — ENOENT and EBUSY are delivered as error events on the child process. The proc.on("error", () => resolve(1)) handler silently resolves with exit code 1, so the catch block (with its ENOENT→UpgradeError translation and EBUSY retry loop) is unreachable dead code. Change the error handler to reject(err) so errors propagate into the catch block.
Evidence
spawnfromnode:child_process(imported at line 17) returns aChildProcessimmediately without throwing; errors arrive viaproc.on('error', …), not as exceptions from thespawn()call itself.spawnWithRetry(line 423) wraps spawn in a try/catch that expects synchronous throws — valid forBun.spawn()but not for Node.js.proc.on('error', () => resolve(1))at line 436 converts every spawn error (ENOENT, EBUSY, EACCES) into a successful promise resolution with exit code 1, bypassing the catch block entirely.- The EBUSY retry loop (lines 453–460) — the stated purpose of the function and specifically needed for Windows Defender scanning — therefore never executes.
- The ENOENT→
UpgradeError('execution_failed')path (lines 448–453) also never executes, losing the actionable error message in favour of a generic 'Setup failed with exit code 1'.
Also found at 1 additional location
src/commands/cli/upgrade.ts:434-437
Identified by Warden find-bugs · HM4-JQ6
|
|
||
| import { spawn } from "node:child_process"; | ||
| import { chmodSync, realpathSync, statSync, unlinkSync } from "node:fs"; | ||
| import { writeFile } from "node:fs/promises"; |
There was a problem hiding this comment.
streamDecompressToFile still uses Bun.file().writer() after Node.js migration
streamDecompressToFile (line 557) calls Bun.file(destPath).writer(), which will throw ReferenceError: Bun is not defined at runtime under Node.js — replace with import { createWriteStream } from 'node:fs' and pipe chunks into a WriteStream.
Evidence
streamDecompressToFileat line 557 callsBun.file(destPath).writer(), a Bun-specific API.- This function is called by both
downloadNightlyToPathanddownloadStableToPath, covering all binary download paths. - The PR adds
writeFileandsetTimeoutimports but leavesBun.file().writer()untouched. - Running under Node.js will produce
ReferenceError: Bun is not definedon any upgrade attempt that decompresses a gzip download. - The JSDoc comment at line 544 explicitly references
Bun.file().writer()as the implementation strategy, confirming it was never converted.
Identified by Warden find-bugs · YQ3-LVT
| proc.on("error", noop); | ||
|
|
||
| // Give browser time to open, then detach | ||
| await Bun.sleep(500); | ||
| await setTimeout(500); | ||
| proc.unref(); | ||
| return true; | ||
| } catch { |
There was a problem hiding this comment.
Spawn errors silently absorbed by noop while function still returns true
When spawn emits an error event (e.g., ENOENT due to the TOCTOU window), noop silently absorbs it but the function still returns true, causing callers like openOrShowUrl to print "Opening in browser..." and skip the URL/QR-code fallback even though no browser was opened.
Evidence
proc.on("error", noop)registers a no-op that discards anyErrorpassed to it — the error is never surfaced to the surrounding scope.await setTimeout(500)does not await process exit or observe whether an error was emitted; it simply waits 500 ms unconditionally.return trueon line 70 is always reached regardless of whether an error event fired during the 500 ms window.- The caller
openOrShowUrlbranches on the return value:true→ prints "Opening in browser..." and returns without showing the QR fallback; the fallback is only shown whenopenBrowserreturnsfalse. - Node.js
spawnalways emits ENOENT/EACCES as an asyncerrorevent — it never throws from thespawn()call itself — so the surroundingtry/catchcannot catch these cases either.
Identified by Warden find-bugs · HK4-FVB
| semverCompare(version, currentVersion) === 1 && | ||
| semverCompare(version, targetVersion) !== 1 | ||
| ) { | ||
| chainTags.push({ tag, version }); | ||
| } | ||
| } | ||
|
|
||
| // Sort by version (chronological for nightlies) | ||
| chainTags.sort((a, b) => Bun.semver.order(a.version, b.version)); | ||
| chainTags.sort((a, b) => semverCompare(a.version, b.version)); |
There was a problem hiding this comment.
semver.compare throws on invalid version strings where Bun.semver.order returned null
Replace semverCompare(version, ...) calls with a null-safe wrapper (e.g. semver.valid(version) ? semverCompare(...) : null) or catch the TypeError, so malformed GHCR tag versions are silently skipped rather than crashing filterAndSortChainTags.
Evidence
Bun.semver.order(v1, v2)returnsnullfor invalid semver inputs;null === 1isfalse, so invalid tags were silently skipped in the loop.- The npm
semverpackage'scompare(v1, v2)(v7.x) constructsnew SemVer(v)internally and throwsTypeError: Invalid SemVer versionfor any non-conforming string. versionat line 466 is derived fromtag.slice(PATCH_TAG_PREFIX.length)with no prior semver validity guard — any unexpected GHCR tag (e.g.patch-latest,patch-) would produce a non-semver string.currentVersionandtargetVersionare caller-supplied strings (from the running binary's version header) and are not validated before being passed in.- The
.sort()comparator at line 477 also callssemverComparedirectly; a throw there would propagate uncaught fromfilterAndSortChainTags.
Identified by Warden find-bugs · ARU-GUD
| // Record mtime for cache invalidation | ||
| sourceMtimes[filename] = file.lastModified; | ||
| const stats = await stat(filepath); | ||
| sourceMtimes[filename] = Math.floor(stats.mtimeMs); |
There was a problem hiding this comment.
TOCTOU: stat() called after readFile() may cache mismatched mtime and DSN content
Calling stat() after readFile() creates a race window: if the file is modified between the two calls, the cache stores the new file's mtime alongside the old file's DSN, causing future cache lookups to treat stale DSN data as fresh.
Evidence
readFile(filepath)captures file content at time T1 (line 84).stat(filepath)is called separately at time T2 (line 88), after content is already processed.- If the file is modified between T1 and T2,
stats.mtimeMsreflects the new write, not the content that was read. sourceMtimes[filename]is then stored with this newer mtime (line 89), paired with the DSN extracted from the old content.- On the next cache check, the stored mtime matches the actual file mtime, so the cache returns the stale DSN without re-scanning.
Identified by Warden find-bugs · L54-AGR
| @@ -1,4 +1,6 @@ | |||
| import { spawn } from "node:child_process"; | |||
| import { addBreadcrumb } from "@sentry/node-core/light"; | |||
| import { whichSync } from "../../which.js"; | |||
There was a problem hiding this comment.
whichSync returns path with trailing \r on Windows when where.exe finds multiple matches
When where.exe outputs multiple results (CRLF line endings), stdout.trim().split("\n")[0] strips only the trailing CRLF of the last line — the first line retains its \r, so spawn receives e.g. C:\path\node.exe\r and fails with ENOENT. The ?? command.executable fallback in runSingleCommand never fires because whichSync returns a non-null (but corrupt) path. Fix: use (stdout.trim().split("\n")[0] ?? "").trimEnd() || null in src/lib/which.ts:61.
Evidence
whichSyncinsrc/lib/which.ts:40-61callsexecFileSync("where.exe", [command])which outputs CRLF-separated lines on Windows.stdout.trim()only strips leading/trailing whitespace from the whole string; inner\r\nseparators become\r\nwith the final\ngone — leaving\rat the end of each non-last line.split("\n")[0]returns the first element, e.g."C:\\Windows\\System32\\cmd.exe\r"with a trailing\r.runSingleCommand(outside hunk, line ~80) useswhichSync(command.executable) ?? command.executable; since the return value is non-null, the raw-name fallback never activates and the malformed path is forwarded tospawn.spawnwith a path containing\rwill throw ENOENT, caught by the outertry/catchreturning{ exitCode: 1 }.
Also found at 1 additional location
src/lib/which.ts:63
Identified by Warden find-bugs · LXH-SZS
| let timedOut = false; | ||
| const timer = setTimeout(() => { | ||
| const timer = globalThis.setTimeout(() => { | ||
| timedOut = true; | ||
| child.kill(); | ||
| }, timeoutMs); |
There was a problem hiding this comment.
Timer not cleared when Promise.all rejects, child.kill() fires on dead process
If Promise.all rejects (e.g. readNodeStream rejects because child.stdout emits an error when the executable isn't found), execution jumps to the catch block and clearTimeout(timer) is never called. The timer fires later, sets timedOut = true, and calls child.kill() on an already-terminated process, which can throw an uncaught ESRCH exception and crash the process. Move clearTimeout(timer) into a finally block to fix this.
Evidence
readNodeStream(command-utils.ts:311) attachesstream.on('error', reject), so any error event onchild.stdoutorchild.stderrrejects the promise returned byreadSpawnOutput.- When the executable is not found (ENOENT), Node.js emits an
errorevent on both theChildProcessand onchild.stdout/child.stderr, triggering this rejection path. clearTimeout(timer)appears only after thePromise.allawait (line 104, context-after), not in afinallyblock, so it is skipped entirely whenPromise.allrejects.- The timer callback then fires after
timeoutMs, callingchild.kill()on a process that has already exited;process.kill(pid, ...)throwsESRCHif the PID no longer exists, producing an uncaught exception in the timer callback.
Identified by Warden find-bugs · J5Z-JEX
| import { existsSync } from "node:fs"; | ||
| import { access, readFile, writeFile } from "node:fs/promises"; | ||
| import { basename, delimiter, join } from "node:path"; | ||
| import { whichSync } from "./which.js"; |
There was a problem hiding this comment.
whichSync calls execFileSync with no timeout, blocking the event loop indefinitely
Both the Unix (/bin/sh -c 'command -v …') and Windows (where.exe) branches in the new whichSync helper omit a timeout option, so a hung shell (e.g. NFS stall, slow filesystem) blocks the entire Node.js process forever.
Evidence
which.tsline 40:execFileSync("where.exe", [command], { encoding, stdio, env })— notimeout.which.tsline 50:execFileSync("/bin/sh", ["-c", …], { encoding, stdio, env })— notimeout.execFileSyncis synchronous and blocks the Node.js event loop for its entire duration; without atimeoutthe process can hang indefinitely.isBashAvailableinshell.ts(line ~295) callswhichSync("bash")inline, making any caller (e.g. completions setup) susceptible to the same hang.
Identified by Warden find-bugs · 96U-DKF
| } catch { | ||
| // File doesn't exist yet — start with empty content | ||
| } | ||
|
|
There was a problem hiding this comment.
Inner catch silently discards all readFile errors, risking GITHUB_PATH content loss
The catch swallows every readFile error, not just ENOENT. If the file exists but is temporarily unreadable (e.g., a transient permission issue), content stays "" and the subsequent writeFile overwrites the file with only the new directory — silently losing all previously-appended PATH entries.
Evidence
- The replaced
Bun.file(path).exists()guard returnedfalseonly when the file did not exist; any other I/O error would propagate. - The new inner
catch {}(lines 302-305) has no condition — it catches all errors, includingEACCES,EIO, etc. - When
contentis""and the outerwriteFilesucceeds, every previous entry in$GITHUB_PATHis permanently discarded. - The outer
catchonly fires ifwriteFilealso fails, so data loss is possible without any returned error signal.
Identified by Warden find-bugs · R6N-BLP
…writer) (#986) ## Summary Fourth step of the Bun → Node.js migration (follows #967, #970, #984). Replaces the remaining Bun-specific APIs in `src/` — the "Group D" items that required more careful handling. ### Changes **`src/lib/bspatch.ts`** (rewritten): - `Bun.zstdDecompressSync()` → `zstdDecompressSync()` from `node:zlib` - `DecompressionStream('zstd')` → `createZstdDecompress()` from `node:zlib` piped through a Node Readable→Web ReadableStream bridge - `Bun.mmap()` → removed entirely; uses `readFile()` for both paths (copy-then-read and direct-read fallback) - `Bun.CryptoHasher("sha256")` → `createHash("sha256")` from `node:crypto` - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/upgrade.ts`**: - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/api/sourcemaps.ts`**: - `Bun.zstdCompress(buf, { level: 3 })` → `zstdCompress()` from `node:zlib` **`src/lib/telemetry/zstd-transport.ts`**: - Removed `globalThis.Bun.zstdCompress` dynamic access and `BunZstdHost` type - Replaced with direct `zstdCompress` from `node:zlib` (always available in Node 22.15+) - Removed belt-and-braces fallback (zstd can't disappear at runtime with `node:zlib`) - `hasZstdSupport()` now checks `typeof zstdCompressCb === "function"` directly **Tests updated** (`test/lib/telemetry/zstd-transport.test.ts`): - Removed 4 tests for `globalThis.Bun.zstdCompress` fallback paths (no longer applicable) - Replaced `Bun.zstdDecompress` with `zstdDecompressSync` in assertions ### Result **Zero non-comment `Bun.*` API calls remain in `src/`.** The only `bun:` reference left is the `require("bun:sqlite")` fallback in `sqlite.ts`, which will be removed when the test runner migrates to Vitest. All 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
## Summary Consolidates fixes for review comments across PRs #967, #970, #984, #986, superseding #988. ### Critical - **Bump `engines.node` to `>=22.15`** — `node:zlib` zstd APIs require 22.15+. Node 20 is EOL. Updated `dist/bin.cjs` runtime version check to match. - **Simplify zstd imports** — replaced feature-detection dance with direct `import { zstdCompress } from "node:zlib"` now that >=22.15 is guaranteed. ### High - **Fix spawn error handling in `upgrade.ts`** — `proc.on("error", () => resolve(1))` discarded the error object, making ENOENT/EBUSY detection dead code. Now properly rejects with the error. ### Medium - **Fix `sqlite.ts` ROLLBACK** — if ROLLBACK throws, the original transaction error was lost. Now wrapped in try/catch. - **Guard `semver.compare`** in `delta-upgrade.ts` with `semver.valid()` to avoid throwing on invalid version strings. - **Narrow catch in `shell.ts`** `addToGitHubPath` — only catch ENOENT, re-throw EACCES/EPERM. - **Add WriteStream backpressure** in `upgrade.ts` `streamDecompressToFile` — check `write()` return value, await `'drain'`. - **Add `setup-node@v6`** with Node 22 to E2E CI job. ### Low - **Fix `whichSync` Windows CRLF** — split on `/\r?\n/` instead of `\n` to strip trailing `\r` from `where.exe` output. ### Test results All 7014 tests pass, 0 failures. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>


Summary
Third step of the Bun → Node.js migration (follows #967, #970). Replaces all Bun-specific API calls in
src/with Node.js equivalents.Changes
File I/O (18 files):
Bun.file(path).text()→readFile(path, "utf-8")fromnode:fs/promisesBun.file(path).json()→JSON.parse(await readFile(path, "utf-8"))Bun.file(path).exists()→access(path).then(() => true, () => false)Bun.file(path).stat()→stat(path)fromnode:fs/promisesBun.write(path, content)→writeFile(path, content)fromnode:fs/promisesBun.write(dest, Bun.file(src))→copyFile(src, dest)fromnode:fs/promisesProcess/System (9 files + 1 new):
Bun.which()→ newsrc/lib/which.tshelper usingcommand -v(POSIX) /where(Windows)Bun.spawn()→spawn()fromnode:child_processwith Promise-wrapped exit codeBun.spawnSync()→spawnSync()fromnode:child_processBun.sleep()→setTimeout()fromnode:timers/promisesUtilities (6 files):
Bun.Glob→picomatch(already a devDependency)Bun.randomUUIDv7()→uuidv7()fromuuidv7packageBun.semver.order()→compare()fromsemverpackageWhat's NOT in this PR (Group D — separate follow-up)
These Bun APIs in
bspatch.tsandupgrade.tsrequire more careful handling:Bun.file().writer()— streaming file writer (needsfs.createWriteStream)Bun.zstdCompress/DecompressSync— zstd compression (needsnode:zlib22.15+)Bun.mmap()— memory-mapped files (has existing fallback)Bun.CryptoHasher— streaming hash (needscrypto.createHash)Test results
All 7012 unit tests pass, 0 failures.