Skip to content

fix(server): bundle Bun platform packages so one effect instance serves CORS - #9118

Open
lnieuwenhuis wants to merge 5 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/bun-single-effect-instance
Open

lnieuwenhuis wants to merge 5 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/bun-single-effect-instance

Conversation

@lnieuwenhuis

@lnieuwenhuis lnieuwenhuis commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Since the CLI bundling change in #5877, dist/bin.mjs inlines effect but kept @effect/platform-bun and @effect/sql-sqlite-bun external. A Bun-hosted server therefore loaded a second effect from node_modules. Effect attaches CORS, compression, and auth headers to real responses through a pre-response handler stored in a module-level WeakMap, so the bundled copy wrote handlers the platform server's copy never read. OPTIONS preflight still worked because it is answered inline, but every actual response lost Access-Control-Allow-Origin and gzip, which is why the desktop app could not connect to a remote Bun-run environment on a separate origin.

The fix externalizes only Bun's own module namespace (bun, bun:*) and bundles both platform packages, so one effect graph serves the whole server. The two packages move to devDependencies. Because correctness now depends on rolldown keeping bun:sqlite in a chunk reached only through import(), the server build gains a post-build check that walks the emitted chunk graph from both entries along static edges and fails if any eagerly loaded chunk imports a Bun module. Rolldown only warns on that merge and exits 0, so the check reads the artifact instead of trusting the bundler.

Verification: in the built artifact the single pre-response-handler WeakMap lives in the shared chunk that both bin.mjs and the BunHttpServer chunk import, and no bare @effect/*-bun runtime import remains. A probe bundled with the repo's own build predicates and run under Bun showed Access-Control-Allow-Origin: * and Content-Encoding: gzip restored on GET, and the real CLI under Node shows no regression. Bundle size grows by about 0.5%. The full server cannot run under Bun on Windows (the Bun PTY adapter is unsupported there), and the freshly built CLI has now passed the Linux Bun runtime smoke described below. A negative control confirmed the new build check rejects a bundle where bun:sqlite is hoisted into bin.mjs, which otherwise fails at load under Node.

Closes #8878

Claude Fable 5.1 via Claude Code

Fresh verification (2026-09-06): built the server/web artifacts and passed the isolated Linux Bun 1.4.0 runtime smoke for CORS, gzip body equivalence, pairing Set-Cookie, and authenticated cookie replay. The smoke is now wired into CI. 20 focused packaging tests passed. Published-package installation was not exercised; Windows Bun startup still has the existing unsupported PTY limitation.


Note

Medium Risk
Changes core server CLI bundling and build-time validation; a mistaken static import of Bun adapters would fail builds or break Node entrypoints, but the new eager-import scan is meant to catch that before release.

Overview
Fixes missing CORS, gzip, and auth on Bun-hosted servers when the desktop app talks to a remote environment on another origin. External @effect/platform-bun and @effect/sql-sqlite-bun had been loading a second effect beside the bundle, so pre-response handlers (WeakMap-keyed) never applied to real responses.

Bundling policy in cli-external-packages.ts now inlines those two packages and only keeps bun / bun:* external. They move from dependencies to devDependencies in apps/server/package.json. Inlining pulls bun:sqlite into the artifact, which must stay behind dynamic import() so node bin.mjs still works.

A post-bundle guard runs in the server CLI build step: findEagerBunRuntimeImports walks static edges from bin.mjs and service-launcher.mjs and fails with ServerCliEagerBunImportError if any eagerly loaded chunk references a Bun runtime module (rolldown only warns). Tests cover the scanner and updated bundle expectations; desktop self-containment comments point at this check for the Bun adapters.

Reviewed by Cursor Bugbot for commit bff7b06. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Bundle @effect/platform-bun and @effect/sql-sqlite-bun instead of externalizing them

  • Removes @effect/platform-bun and @effect/sql-sqlite-bun from CLI external prefixes in cli-external-packages.ts so they are bundled rather than left external. Bun runtime modules (bun, bun:*) remain external.
  • Adds findEagerBunRuntimeImports to walk the emitted chunks' static import graph from entry chunks and report any reachable Bun runtime module imports.
  • Adds assertNoEagerBunImports to the build command in cli.ts, which fails the build with ServerCliEagerBunImportError if any eagerly loaded chunk imports a Bun runtime module.
  • Updates tests in cli-external-packages.test.ts to expect the two adapter packages bundled and to cover the new eager-import detection logic.
  • Risk: isExternalCliDependency now bundles @effect/platform-bun and @effect/sql-sqlite-bun; if any code statically imports Bun runtime modules through these packages, the build will fail via assertNoEagerBunImports rather than silently producing a broken bundle.
📊 Macroscope summarized bff7b06. 4 files reviewed, 1 issue evaluated, 1 issue filtered, 0 comments posted

🗂️ Filtered Issues

apps/server/scripts/cliErrors.ts — 0 comments posted, 1 evaluated, 1 filtered
  • line 80: ServerCliEagerBunImportError also reports violations for the bare specifier bun (as exercised by the new scanner test), but its message always says the failure is ERR_UNSUPPORTED_ESM_URL_SCHEME for a bun: URL. A static bare bun import is package resolution and Node reports ERR_MODULE_NOT_FOUND, so this build failure gives an incorrect diagnosis for one of the explicitly detected violation types. [ Out of scope (post-validation triage) ]

Summary by CodeRabbit

  • Bug Fixes

    • Improved server runtime compatibility across Node and Bun environments.
    • Prevented incompatible Bun-only modules from being loaded during server startup.
    • Improved server packaging and runtime module resolution.
    • Added validation to detect unsafe runtime imports before packaging completes.
  • Tests

    • Added automated server startup checks covering readiness, CORS, compression, authentication, error reporting, graceful shutdown, and temporary-file cleanup.
    • Expanded validation for session handling and browser-session pairing.

…es CORS

Since 0.0.34 a Bun-hosted server omits `Access-Control-Allow-Origin` from
real GET/POST responses while answering OPTIONS preflight correctly, so a
desktop app cannot reach a remote environment on a separate HTTPS origin.
Node-hosted servers are fine. Compression and the auth refresh / DPoP /
cloud credential headers are broken the same way and for the same reason.

The CORS code did not change. The CLI bundling change did. `dist/bin.mjs`
inlines `effect`, but `@effect/platform-bun` and `@effect/sql-sqlite-bun`
stayed external so the bundler would never have to resolve `bun:sqlite`.
Under Bun that external `BunHttpServer` loads a second `effect` from
node_modules beside the bundle. Effect keys each request's pre-response
handler off a module-level WeakMap in
`effect/unstable/http/internal/preResponseHandler`: `HttpMiddleware.cors`
and `compression` write to it, and the platform server's `toHandled` reads
it when sending the response. With two `effect` instances the bundled copy
writes handlers the node_modules copy never reads. Preflight survives
because `cors` answers OPTIONS inline without touching the WeakMap.
`@effect/platform-node` is bundled into the same graph, which is why Node
never saw this.

Externalize Bun's own module namespace (`bun`, `bun:*`) instead of the
packages that import it. That is all the bundler could not resolve, and
nothing has to resolve it under Node either: every Bun import sits behind a
`typeof Bun !== "undefined"` dynamic import, so it lands in a chunk only a
Bun-hosted server loads. The two packages also leave `dependencies`, since
nothing resolves them at runtime any more.

That trades one silent failure for another, so it is now checked. Inlining
moves `bun:sqlite` into the bundle, and it is only harmless while its chunk
stays reachable solely through `import()`; statically reachable, every
`node bin.mjs` dies with ERR_UNSUPPORTED_ESM_URL_SCHEME. Rolldown merges a
dynamic import into its importer with an INEFFECTIVE_DYNAMIC_IMPORT warning
and exit 0, and removing these packages from `dependencies` is not a
backstop either — the desktop self-containment probe runs
`node bin.mjs --version` and never takes the Bun branch. So
`assertNoEagerBunImports` in `apps/server/scripts/cli.ts` walks the emitted
chunk graph from `bin.mjs` and `service-launcher.mjs` following static
edges only and fails the build on any Bun specifier in that set. It runs on
every PR through `vp run build:desktop`, unlike the desktop probe.

Verified by bundling one probe that mirrors `server.ts`'s conditional
`BunHttpServer` import plus `http.ts`'s cors and compression middleware,
built both ways and run under Bun 1.2.15. Packages external: no
`access-control-allow-origin`, no `content-encoding`, 9728 bytes. Packages
bundled: `access-control-allow-origin: *`, `content-encoding: gzip`, 81
bytes. Preflight identical in both. The shipped bundle has no bare
`@effect/*-bun` import left, keeps one `bun:sqlite` external in the chunk
Node never loads, and defines the pre-response WeakMap exactly once. The
real CLI still serves CORS and sets `vary: Accept-Encoding` under Node.
Bundle JS grows 8,662,684 -> 8,705,534 bytes while `bin.mjs` itself drops
180,952; the npm install loses 537 KB of now-unused packages.

The new check passes on a real build (11 eagerly loaded chunks, no Bun
modules). Adding a static `@effect/sql-sqlite-bun/SqliteClient` import to
`bin.ts` made it report `bin.mjs -> bun:sqlite` and exit 1, and the bundle
it rejected does fail under Node with ERR_UNSUPPORTED_ESM_URL_SCHEME.

`vp test run scripts/lib/cli-external-packages.test.ts` (20) passes and
`apps/server` typecheck is clean. `scripts/build-desktop-artifact.test.ts`
has 7 failures on Windows (symlink EPERM, WSL archive, macOS entitlements)
that reproduce identically on an unmodified tree.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 1, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR changes the production server bundle boundary and Bun runtime behavior, affecting CORS, compression, and authentication-header handling, while adding a new runtime smoke harness. It also introduces file-level suppressions for two static-analysis diagnostics, so the changes warrant human review.

You can add or adjust custom eligibility rules. Learn more.

@derektrimm

Copy link
Copy Markdown
Contributor

@juliusmarminge re your note on #8878 about browser-session cookie emission: here is a manual Linux check of this branch that includes it.

Repo-built apps/server/dist, comparing upstream main at 04efa7907 with the same main plus this PR merged on top (635625d42), so the only source difference is this change. Bun 1.4.0 and Node v22.23.2, fresh --base-dir per run, server bound to 0.0.0.0 so the auth policy reports remote-reachable, and a fresh credential from node bin.mjs auth pairing create for each pairing run.

Build Runtime GET /.well-known/t3/environment with Origin GET / with accept-encoding: gzip POST /api/auth/browser-session
main Node Access-Control-Allow-Origin: * Content-Encoding: gzip, Vary: Accept-Encoding 200, authenticated: true, Set-Cookie present
main Bun header absent both headers absent 200, authenticated: true, Set-Cookie absent
main + PR Node Access-Control-Allow-Origin: * Content-Encoding: gzip, Vary: Accept-Encoding 200, authenticated: true, Set-Cookie present
main + PR Bun Access-Control-Allow-Origin: * Content-Encoding: gzip, Vary: Accept-Encoding 200, authenticated: true, Set-Cookie present

The main/Bun pairing response is the #7756 shape exactly: 200 with authenticated: true and no Set-Cookie (Cache-Control and Pragma are missing from that response too). Replaying the issued cookie against GET /api/auth/session authenticated in every run that had one. The Bun cells were repeated three times each with identical headers.

This is symptom-level evidence from the repo build with workspace node_modules, not an npm pack install of the published package, and not in-repo regression coverage.

…-instance

# Conflicts:
#	apps/server/package.json
#	pnpm-lock.yaml
@derektrimm

Copy link
Copy Markdown
Contributor

A Bun runtime smoke for the browser-session cookie case discussed in #8878 is available here: derektrimm@a90d0ef. It runs the built CLI under Bun 1.4.0 in the Check job and asserts CORS, gzip, and the pairing Set-Cookie. The smoke fails on main at bfef973d9 at the CORS assertion and passes with bff7b06 plus this commit. The commit is based on bff7b06 and can be cherry-picked if useful.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6bfac720-74ab-41f8-bade-45533101c6ce

📥 Commits

Reviewing files that changed from the base of the PR and between 87fabb0 and 0029e74.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • apps/server/package.json
  • scripts/server-runtime-smoke.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The build bundles Bun adapter packages, externalizes Bun runtime modules, validates generated chunks, and runs a Bun server smoke test in CI. The smoke test covers CORS, compression, sessions, authentication, errors, termination, and cleanup.

Changes

Bun runtime validation

Layer / File(s) Summary
Bun module externalization and graph scanning
scripts/lib/cli-external-packages.ts, scripts/lib/cli-external-packages.test.ts
Bun runtime specifiers are externalized directly. Bun adapter packages are bundled. Static chunk traversal detects eagerly reachable Bun imports and ignores dynamic imports.
Server build validation and dependency wiring
apps/server/scripts/cli.ts, apps/server/scripts/cliErrors.ts, apps/server/package.json, scripts/build-desktop-artifact.ts
The server build validates required entry chunks and rejects eager Bun imports with ServerCliEagerBunImportError. Bun adapter packages move to development dependencies.
Bun runtime smoke test and CI execution
scripts/server-runtime-smoke.ts, .github/workflows/ci.yml
The smoke test starts the built server and checks CORS, compression, sessions, authentication, errors, termination, and cleanup. CI runs it under Bun 1.4.0.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant Bun
  participant BuiltServer
  participant HTTPClient
  CI->>Bun: Start server-runtime-smoke.ts
  Bun->>BuiltServer: Launch built server
  BuiltServer-->>Bun: Emit startup token
  Bun-->>HTTPClient: Serve HTTP requests
  HTTPClient->>BuiltServer: Check CORS, sessions, compression, and authentication
  BuiltServer-->>HTTPClient: Return response headers and bodies
  Bun->>BuiltServer: Terminate process
  Bun->>Bun: Remove temporary directory
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 0029e

The Bun build and runtime smoke coverage validate the restored server behavior without identifying an outstanding merge-blocking issue.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary change: bundling Bun platform packages so the server uses one Effect instance for CORS handling.
Description check ✅ Passed The description clearly explains the problem, implementation, verification, risks, limitations, and linked issue. It omits the explicit checklist and UI section, but these omissions are non-critical b…
Linked Issues check ✅ Passed Issue #8878 requires Access-Control-Allow-Origin on actual responses for Bun-hosted servers. The PR bundles the Bun Effect adapters into the server CLI and keeps Bun runtime modules external. The ru…
Out of Scope Changes check ✅ Passed The CLI bundling changes, eager Bun-import validation, dependency updates, packaging tests, CI smoke test, and launcher-context filtering support reliable Bun server execution and verification for iss…
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. (2 skipped: 2 u…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@shivamhwp

Copy link
Copy Markdown
Collaborator

Note: GPT-6 on behalf of shivam (@shivamhwp).

The smoke script needs to remove T3_SERVICE_LAUNCHER_CONTEXT from its child environment too. A command started inside a launcher-managed T3 session inherits that variable, and the current filter only removes T3CODE_* and VITE_DEV_SERVER_URL. The standalone Bun child then exits in ServiceLauncherClient.resolveStartup with a version mismatch, or an unavailable IPC channel when versions match, before checking CORS or cookies. Exclude T3_SERVICE_LAUNCHER_CONTEXT when constructing env; this child has no launcher IPC connection.

@derektrimm

Copy link
Copy Markdown
Contributor

Confirmed. The launcher spawns its child with that variable set (apps/server/src/serviceLauncher.ts:406), resolveStartup in apps/server/src/cloud/serviceLauncherClient.ts:119-133 fails on it with version-mismatch or ipc-unavailable, and ServerEnvironment.ts:194 runs that check during every serve startup, so a smoke started from inside a launcher-managed session dies before it listens. CI runners never carry the variable, which is why the Check job passes. Adding key !== "T3_SERVICE_LAUNCHER_CONTEXT" to the filter at scripts/server-runtime-smoke.ts:89 covers it.

@cursor

cursor Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@lnieuwenhuis

Copy link
Copy Markdown
Contributor Author

Fixed in 0029e74. The standalone smoke child now excludes T3_SERVICE_LAUNCHER_CONTEXT. CI runs the existing Bun smoke with a synthetic inherited launcher context so this cannot silently regress on runners without launcher state.

Verified against the rebuilt server with Bun 1.4.0: both a mismatched child version and the current version without IPC pass CORS, gzip, pairing-cookie issuance, and authenticated cookie replay. Removing the filter reproduces the reported launcher-version startup failure. The 20 packaging tests, scripts/server typechecks, targeted lint, and Node bundle --version check pass. Also merged current upstream main and regenerated the lockfile, retaining the new platform-node-shared runtime dependency.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Access-Control-Allow-Origin missing from responses since 0.0.34 — desktop app cannot connect to a remote environment on a separate HTTPS origin

3 participants