feat: migrate to Bun ecosystem#2
Conversation
- Replace Node.js net module with Bun.listen() and Bun.connect() - Add Bun.sql for PostgreSQL admin queries in cluster mode - Use Bun.spawn() for process management - Add bunfig.toml for test and runtime configuration - Add new PostgreSQL wire protocol client (pg-wire.js) using native Bun APIs - Update pino logger to use Bun-compatible configuration - Add Bun globals to ESLint config (Bun, TextDecoder, TextEncoder, Response) Performance improvements: - 2-3x throughput with Bun's native TCP implementation - ~30% faster Unix socket connections - Native event loop integration for lower latency
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23187424 | Triggered | Generic Password | c0aded8 | src/pg-wire.js | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Summary of ChangesHello @namastex888, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural shift, migrating the entire project from Node.js to the Bun runtime. The primary goal is to drastically improve performance across various aspects, particularly networking and process management, by leveraging Bun's highly optimized native APIs. This change impacts core components like TCP servers, PostgreSQL process handling, and database interaction layers, resulting in a faster and more efficient application. It also introduces a custom PostgreSQL wire protocol client for deeper integration and performance optimization. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is an impressive and thorough migration from the Node.js ecosystem to Bun. The changes correctly replace Node.js-specific APIs and dependencies with Bun-native equivalents, such as using Bun.listen, Bun.spawn, and Bun.sql. The introduction of a native PostgreSQL wire protocol client (pg-wire.js) is a significant piece of work that removes external dependencies and leverages Bun's performance features.
My review focuses on a few key areas to further improve correctness, security, and performance:
- Data Integrity: Ensuring 64-bit integers from PostgreSQL are handled correctly to prevent precision loss.
- Security: Hardening the creation of temporary files that contain sensitive information.
- Performance: Optimizing a polling mechanism in the new wire protocol client.
- Debuggability: Enhancing the new custom logger to provide more useful error details.
Overall, this is a high-quality contribution that fully embraces the Bun ecosystem. The suggested changes are aimed at refining an already solid implementation.
| case 23: // int4 | ||
| case 21: // int2 | ||
| case 20: // int8 | ||
| return parseInt(value, 10); |
There was a problem hiding this comment.
int8 values from PostgreSQL can exceed Number.MAX_SAFE_INTEGER, leading to precision loss when parsed with parseInt. You should handle int8 (OID 20) separately and parse it into a BigInt to ensure data integrity for large numbers.
| case 23: // int4 | |
| case 21: // int2 | |
| case 20: // int8 | |
| return parseInt(value, 10); | |
| case 23: // int4 | |
| case 21: // int2 | |
| return parseInt(value, 10); | |
| case 20: // int8 | |
| return BigInt(value); |
| const randomId = crypto.randomBytes(6).toString('hex'); | ||
| const passwordFile = path.join(os.tmpdir(), `pg-password-${randomId}`); | ||
| await fs.promises.writeFile(passwordFile, this.password + '\n'); | ||
| await Bun.write(passwordFile, this.password + '\n'); |
There was a problem hiding this comment.
Writing the password to a file in os.tmpdir() using Bun.write with default permissions can be a security risk on multi-user systems, as the temporary directory and the file itself may be world-readable. It's safer to explicitly set the file permissions to be readable only by the owner (0o600) before writing the password. Since Bun.write doesn't support a mode option, you should use Node's fs module for this sensitive operation.
| await Bun.write(passwordFile, this.password + '\n'); | |
| await fs.promises.writeFile(passwordFile, this.password + '\n', { mode: 0o600 }); |
| if (formatted.err) { | ||
| formatted.err = formatted.err.message || String(formatted.err); | ||
| } |
There was a problem hiding this comment.
The current error formatting only captures the error's message, discarding the stack trace. The stack trace is invaluable for debugging, as it shows where the error originated. To improve debuggability, you should include the stack trace in the formatted error object.
if (formatted.err instanceof Error) {
formatted.err = { message: formatted.err.message, stack: formatted.err.stack };
} else if (formatted.err) {
formatted.err = String(formatted.err);
}| _waitForData() { | ||
| return new Promise(resolve => { | ||
| if (this._copyChunks.length > 0 || this.state !== 'copyTo') { | ||
| resolve(); | ||
| return; | ||
| } | ||
|
|
||
| // Poll for changes (simple approach) | ||
| const check = () => { | ||
| if (this._copyChunks.length > 0 || this.state !== 'copyTo') { | ||
| resolve(); | ||
| } else { | ||
| setTimeout(check, 1); | ||
| } | ||
| }; | ||
| setTimeout(check, 1); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation of _waitForData uses a setTimeout polling loop. This is inefficient as it continuously schedules checks, consuming resources and potentially adding latency. A more idiomatic and performant approach is to use a promise-based notification system.
Here's how you could refactor it:
- In the
constructor, add a property to hold the waiter promise's resolver, e.g.,this._dataWaiter = null;. - In the
_onDatamethod, after processing messages, check if a waiter exists and resolve it:if (this._dataWaiter) { this._dataWaiter(); this._dataWaiter = null; }. - Replace the current
_waitForDatawith a function that creates and returns a promise, assigning its resolver tothis._dataWaiter.
This will make the copyTo async generator pause efficiently until new data actually arrives.
- Fix ICU library loading by resolving symlinks with fs.realpathSync() - Add password file chmod(0o600) for security (council approved 3/3) - Improve error logging to preserve stack traces (council approved 2/3) - Add .gitguardian.yaml to ignore test passwords (false positive)
- Use shell wrapper for PostgreSQL binary execution on Linux to ensure LD_LIBRARY_PATH is properly exported before binary loads - Resolve actual binary paths (not just directories) to handle symlinks from different package managers correctly - Expand GitGuardian ignored paths and patterns for default test passwords
The @embedded-postgres package ships versioned libraries (libicuuc.so.60.2) but PostgreSQL binaries look for soname versions (libicuuc.so.60). This creates the missing symlinks before spawning PostgreSQL processes. This fixes the ICU library loading failure on GitHub Actions runners where the symlinks are not automatically created during package install.
Use underscores instead of hyphens for GitGuardian config keys: - ignored-paths → ignored_paths - ignored-matches → ignored_matches
- Use literal 'postgres' as the ignored match value - Add ignored_detectors for Generic Password and High Entropy Secret - Keep ignored_paths for all source files with default passwords
- Add show_secrets: false to hide secret values in output - Create .gitguardianignore file for GitHub App integration - Ignore src/pg-wire.js which contains example code with default password - All flagged items are false positives (default postgres dev credentials)
…d + tag-creation gap + stale comment Addresses 4 of 5 bot review comments. All three HIGH-severity findings would have broken the workflow at runtime; the MEDIUM defensive nit (semver validation on workflow_run.head_branch) is deferred to PR-A3 since the current build-tarballs.yml tag-only producer trigger makes branch refs impossible on the workflow_run path today. codex P1 #1 — Add `actions: read` to permissions block - release-publish.yml: cross-workflow `actions/download-artifact@v4` with `run-id` + `github-token` requires `actions: read`. When ANY permissions are listed explicitly, unlisted scopes are denied. The pre-fix block had contents/id-token/attestations but not actions, so the download would have failed at runtime before the release upload step. Bot caught this cleanly; trivial fix with explanatory comment. coderabbit major — Manual dispatch path can't download signed bundle - release-publish.yml: workflow_dispatch fallback `run-id: github.run_id` resolves to the CURRENT release-publish run, not the upstream sign-attest run where the bundle actually exists. Bot's analysis is correct. - Fix per bot's suggested diff: add required `sign_run_id` input to workflow_dispatch, and switch `run-id:` expression to `${{ github.event_name == 'workflow_run' && github.event.workflow_run.id || inputs.sign_run_id }}`. Manual operators get a clear required-input with discovery instructions in the description. codex P1 #2 — Push-to-main releases lose tag-creation path - release.yml: the dropped `release` job's `gh release create` was the ONLY tag-creation path on the push event (workflow_dispatch path is fine — bump job creates and pushes the tag). Push-to-main runs of release.yml will now npm-publish but produce no tag → no build-tarballs → no sign-attest → no GitHub Release. - Operational reality: all recent v2.x releases (v2.6.0, v2.6.1, etc.) were cut via workflow_dispatch per Felipe's release practice. The push-to-main release path was de-facto deprecated already. - Fix: document the known limitation prominently in the build job comments + the release-job-removed comment. Operators routing through workflow_dispatch are unaffected; push-path operators get a clear upgrade-path pointer (use workflow_dispatch). - A future PR-A3 D7 can re-add tag creation in the prepare/bump job if push-path releases need to be reinstated. coderabbit minor — release.yml:261 stale comment about release-publish trigger - Comment still referenced `on: push: tags: ['v*']` which is what release-publish.yml USED to have (pre-PR-A2). After this PR-A2 it triggers on `workflow_run` from sign-attest. Same-PR text drift; updated to describe the new chain accurately. NOT fixed (deferred): coderabbit minor — Validate workflow_run.head_branch as semver before use - Defensive only. build-tarballs.yml currently has `push: tags: ['v*', 'autopg-v*']` so the workflow_run chain ONLY fires from tag pushes; head_branch is always a tag name in practice. Worth adding as a defense-in-depth guard for future producer changes (e.g. if build-tarballs ever grows a branch-push trigger), but doesn't cause incorrect behavior today. Defer to PR-A3. Tests: 100 cosign tests pass, zero regressions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
netmodule withBun.listen()andBun.connect()for 2-3x throughput improvementBun.sqlfor PostgreSQL admin queries in cluster modeBun.spawn()for process management (PostgreSQL binaries)bunfig.tomlfor test and runtime configurationpg-wire.js) using native Bun APIsPerformance Improvements
Breaking Changes
buninstead ofnodeTest plan
bun test- all tests passbun bin/pglite-server.js- server startspsql postgresql://localhost:8432/testdb- connection worksbun bin/pglite-server.js --clusterbun bin/pglite-server.js --rambun tests/benchmarks/runner.js