Skip to content

fix(ci): bundle the Electron ratchet as one entry so shard 2 stops running out of memory - #26053

Closed
brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/electron-ratchet-one-bundle
Closed

brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/electron-ratchet-one-bundle

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 0 0 0 0
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​7 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3

ELI5

Since 10-06, the second of the five unit-test jobs in PR Checks ("test / tests node 24 2/5") has been dying on many PRs, main included. No test fails. The job just stops with "The runner has received a shutdown signal" or exit code 143. One test, the check that keeps Electron out of the headless runtime, was asking the bundler to build about 2,600 separate bundles at once. That took about 16 GB of memory. This PR asks for one combined bundle instead. The check gives the same answers and needs about 0.6 GB.

What Changed

What you see: CI only. Shard 2 should stop being killed partway through, and the ratchet check (pnpm run check:runtime-electron-ratchet and its test file) runs in under a second instead of 3–90 s. Nothing in the app changes.

The problem: config/scripts/check-runtime-electron-ratchet.mjs makes sure nothing reachable from the Orca runtime, or from the structured-chat code that has to run in orcad, imports electron. To do that it bundles those files with esbuild (the JavaScript bundler) and reads esbuild's metafile, which is the list of every module the bundle pulled in and what each one imports. Since #24988 every file in the structured-chat directories is its own entry point, about 2,565 of them. With write: false, esbuild built and returned 2,565 complete bundles even though the script only reads the metafile. The bundles also had to be copied into the vitest worker that runs the test.

In the five killed shard-2 runs that were examined, this test file was the only one still running at the moment of the kill: runs 37551764911 (jobs 112581734247, 112573011087), 37554604023 (job 112579486715), 37559551986 (job 112604598018) and 37537601326 (job 112523900418). In passing shard-2 runs (jobs 112585301440, 112524356695) the file took about 92 s, and its "passes on the tree" test took about 76 s against a 120 s timeout. The checked-in timing for the file is 2.5 s.

The mechanism: collectElectronImporters now hands esbuild a single generated entry (stdin) that contains one import "<absolute path>" line per entry point. esbuild follows the same import graph from the same files, so the metafile lists the same modules with the same imports. It produces one bundle instead of 2,565. The outdir placeholder is gone, because esbuild only needed it for multiple entry points.

Measured on macOS (arm64, 64 GB): /usr/bin/time -l node config/scripts/check-runtime-electron-ratchet.mjs, with RSS also sampled every 0.1 s by ps, since esbuild runs as a separate child process that time -l doesn't count.

esbuild process peak RSS node process peak RSS combined peak wall time
Before (origin/main, 3 runs) 14.3–14.8 GB 1.8–2.4 GB ~16.1 GB 3.3–8.9 s
After (3 runs) 0.54–0.56 GB 73 MB ~0.63 GB 0.5–0.8 s

The test file alone, run locally: 17/17 pass in 1.7 s.

On this PR's CI (head 6be1282, run 37566129324): shard 2 (test / tests node 24 2/5) passed. check-runtime-electron-ratchet.test.mjs took 3.4 s there, against ~92 s in passing shard-2 runs before. The Enforce runtime Electron-import ratchet step took ~1 s.

What I did not measure: memory on the Linux arm64 CI runner itself. GitHub's standard ubuntu-24.04-arm runner has 16 GB. A ~16 GB peak from one test, alongside the other vitest workers in the shard, is consistent with the runner running out of memory and being shut down, but I have no runner-side memory trace. This PR's own CI run is the check: shard 2 should finish, and this file should take a few seconds.

Why

The results are identical, checked before and after. For each of four trees I ran the old per-entry build and the new single-entry build and compared the metafiles directly:

Tree Modules in metafile (old / new) Same module set Modules whose import list differs Electron importers reported (old = new)
Current tree 5,920 / 5,920 yes 0 none
import 'electron' added to src/main/native-chat/transcript-read-cache.ts (lane-only file the runtime doesn't load) 5,920 / 5,920 yes 0 src/main/native-chat/transcript-read-cache.ts
import … from 'electron/main' added to src/main/ipc/worktree-change-invalidators.ts (reached only through orca-runtime.ts) 5,920 / 5,920 yes 0 src/main/ipc/worktree-change-invalidators.ts
Both of the above (the second as require('electron')) 5,920 / 5,920 yes 0 both files

The script never reported import chains or which entry point reached Electron, only the importing modules, so the single entry loses no information. The existing test that adds an Electron import to transcript-read-cache.ts in memory still fails the check with the expected + src/main/native-chat/transcript-read-cache.ts line.

Alternatives considered:

  • Raise the test timeout or move the file to its own job. Doesn't fix the memory. The check would still need ~16 GB anywhere it runs, including pnpm lint on a laptop.
  • Bundle the entries in batches. Lowers the peak but still builds thousands of bundles that nobody reads.
  • Skip the real-tree test in vitest. Removes the coverage the gate exists for. The transitive edges only show up on the real tree.

Linked Issue

None. Maintainer PR for a CI failure on main.

Visual Proof

N/A. This is a CI-only change to a lint/test script, with nothing visible in the app. The before/after evidence is the memory and time table above.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • node config/scripts/check-runtime-electron-ratchet.mjs → ok — 0 entries, unchanged. (before and after).

  • vitest run --config config/vitest.config.ts config/scripts/check-runtime-electron-ratchet.test.mjs → 17/17 pass. The existing tests already cover a seeded lane violation, a violation through a node_modules package, and the real tree, so no test needed to change.

  • The old-vs-new metafile comparison above (current tree plus three seeded trees).

  • oxlint and oxfmt --check clean on the changed file.

  • Platforms: macOS locally, Linux in CI. Windows: entry paths go into the generated entry through JSON.stringify, which escapes backslashes, and esbuild resolves absolute Windows paths. Not run on Windows: CI runs this check only on Linux.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Author: @BrennanKB5.

  • Security: none. A build-time script only.
  • SSH / mobile / wire compatibility: untouched.
  • Performance: the ratchet check drops from ~16 GB and several seconds to ~0.6 GB and under a second.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e488339b-f0e4-43e0-a432-e0222ef23ed9
📥 Commits

Reviewing files that changed from the base of the PR and between 70c6876 and 26d7feb.

📒 Files selected for processing (1)
  • config/scripts/check-runtime-electron-ratchet.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

collectElectronImporters now passes entry-point imports to esbuild as one stdin module, resolved from ROOT with the JavaScript loader. The build continues to bundle without writing output, and the code continues to use the metafile to identify Electron importers.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 26d7f

The change appears ready to merge. Reported checks show unchanged importer results, and Windows path resolution is supported by the pinned esbuild version.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the change: bundle the Electron ratchet as one entry to reduce CI memory use.
Description check ✅ Passed The description covers the change, reason, mechanism, testing, and CI evidence. It explains that no issue is linked because this is a maintainer PR, and marks visual proof as not applicable with a rea…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Superseded by #26049, which landed the same fix on main (one bundle through a single stdin entry instead of ~2,590 separate entries). Closing this one.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant