Repository navigation
fix(dev): stop caching unsigned bundles and reclaim stale dev copies - #15247
Merged
Merged
Conversation
Two dev-runner cache bugs, found while reviewing #15183. 1. The marker that marks a bundle reusable was written even when the ad-hoc codesign failed, so an unsigned bundle was cached permanently. The warning scrolled past once and every later launch silently reused it -- losing notification delivery and the stable cdhash that keeps safeStorage from re-prompting, i.e. it would resurface the exact bug #15183 fixes, but sticky. Write the marker only after a successful sign. A failure now costs a re-copy per launch until signing works, and still does not block `pnpm dev`. Mutation-verified: pointing codesign at a nonexistent binary produces the warning and leaves no marker behind. 2. out/electron-dev only ever cleared the directory it was about to rebuild, so siblings from renamed branches and past Electron upgrades were never reclaimed -- measured at 143 directories / 38GB across one set of worktrees. Prune siblings on every prepare, skipping any directory a live process is running from. The liveness check deliberately does not use pgrep: macOS pgrep has no -a (a Linux procps extension) and silently prints bare PIDs, which reads as "nothing is running" and deletes a live bundle. An earlier draft did exactly that and destroyed a running instance's bundle in testing. It now parses `ps -Awwo command=`, and skips pruning entirely if that call fails, since treating a failure as "nothing live" is the dangerous direction. Verified: a second dev instance leaves a concurrently running instance's bundle intact, and reclaims it once that instance exits.
Contributor
|
Caution Review failedThe pull request is closed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded bundle-selection helpers that protect the current bundle, live bundles, and recent marker-less bundles. Added tests for path boundaries, spaces, path spellings, aged bundles, and empty results. The Electron development runner now scans live processes and prunes stale bundles during reuse and creation. Failed signing does not write the reuse marker. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9384a1e6-a842-4ad6-a5a8-ec57efe0e237
📒 Files selected for processing (3)
config/scripts/dev-electron-bundle-cache.mjsconfig/scripts/dev-electron-bundle-cache.test.tsconfig/scripts/run-electron-vite-dev.mjs
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review.
Review found a P0 in the liveness check -- the same shape and the same dangerous direction as the pgrep bug it replaced. The regex `/\/\S*\/out\/electron-dev\/[^\s\/]+/g` cannot cross a space, so any path containing one either extracted a truncated prefix that never matched the real directory, or matched nothing at all. Both yield "nothing is running", and the prune then deletes a live instance's bundle out from under it: /Users/me/My Projects/orca/out/electron-dev/aaa111 -> truncated -> deletes live bundle /Users/me/dev repo/out/electron-dev/aaa111 -> no match -> deletes live bundle No path on the machine this was developed on contains a space, so no amount of local testing would have surfaced it. Stop parsing. Search the raw `ps` output for each candidate directory instead. That is immune to spaces and shell-significant characters, and the trailing slash in the needle preserves the `<dir>2` vs `<dir>` boundary for free. Tests now cover the seam that has now shipped two bugs: a live path containing spaces, and a true boundary case. Both mutation-verified -- dropping the trailing slash and reintroducing the extraction regex each fail a test. Re-verified end to end that a second instance leaves a running instance's bundle intact.
Review found a race this PR introduced. Between mkdirSync and the marker write there is a ~270MB cpSync taking tens of seconds, during which the directory exists, has no marker, and cannot appear in the process table because its instance has not launched yet. A second instance starting on another branch saw it as neither current nor live and deleted it mid-copy, breaking the first build. Reachable in a workflow this repo's developers use daily -- start a cold build on a new branch, then start a cache-hit instance elsewhere. Marker-less and recent is exactly the mid-build signature, so protect it. Age is what separates it from a build that crashed partway, which would otherwise be protected forever. All three guards are mutation-verified: dropping the in-flight window, dropping the trailing slash, and ignoring liveness each fail tests. Verified end to end that a pruning instance keeps a live bundle and a mid-build directory while still reclaiming old debris.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/scripts/dev-electron-bundle-cache.test.ts (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the test comments.
Use the test names for expected behavior. Remove comments that restate the assertion. Reduce rationale comments to one line.
config/scripts/dev-electron-bundle-cache.test.ts#L33-L34: remove the comment because the test name states the risk and behavior.config/scripts/dev-electron-bundle-cache.test.ts#L46-L47: reduce the regression rationale to one line.config/scripts/dev-electron-bundle-cache.test.ts#L75-L77: remove the implementation walkthrough or reduce it to one line.config/scripts/dev-electron-bundle-cache.test.ts#L90-L91: reduce the age-based rationale to one line.As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c09156d9-1466-49aa-b875-e2f2e9dc1498
📒 Files selected for processing (3)
config/scripts/dev-electron-bundle-cache.mjsconfig/scripts/dev-electron-bundle-cache.test.tsconfig/scripts/run-electron-vite-dev.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- config/scripts/run-electron-vite-dev.mjs
Included review availability: Your plan includes up to 10 reviews per rolling hour; 2 remain after this review.
Adversarial review found two more ways to destroy something in use. 1. Making the marker conditional on a successful sign turned that bundle into a PERMANENT cache miss, and the rebuild path's `rmSync(distDir)` had no liveness check. So after a failed sign, every later `pnpm dev` in that worktree deleted the directory a running instance was executing from -- introduced by this PR, since previously the marker was always written and the bundle was simply reused. Reuse it again when a live process is running from it, which is what the runner did before. Verified end to end with codesign sabotaged: instance A starts with no marker, B reuses instead of rebuilding, A's bundle survives and A keeps running. 2. Liveness compared raw bytes, but macOS realpaths /tmp to /private/tmp while `ps` preserves whatever spelling a process was launched with. A worktree under /tmp could therefore read as "not running" and have its live bundle deleted. Check both spellings; that can only ever protect more. Also corrects the in-flight window comment. A directory's mtime only changes when a top-level entry is created, so it freezes once <app>.app appears -- the window runs from build start, not last activity. Full builds measure 130-200s against a 15 minute window, so the margin is 5-10x, but it is a cliff and should say so. All guards mutation-verified with a pattern-existence assertion, after an earlier mutation run reported "caught" when the pattern had silently failed to match.
Review found the reuse guard checked only that the executable exists, while the cache path additionally requires Chromium's resources (icudtl.dat, the keyboard layout helper) because a bundle missing them blank-crashes. The two predicates disagreed, so the guard could accept a bundle the cache path rejects. That mattered because liveness matches any process holding the path, including an orphaned crashpad helper. A blank-crashing bundle would therefore look "in use" forever, get reused, crash again, and never rebuild -- wedging `pnpm dev` with no way out but a manual rm -rf. Apply the same completeness check, and say in the warning how to force a rebuild. Verified with a real orphan process holding the directory: healthy bundle + orphan -> reused, preserved (the guard's purpose) bundle missing icudtl.dat + orphan -> rebuilt, not wedged
The previous commit reused copiedAppIsUsable's resource list for the reuse guard, which also requires orca-keyboard-layout. That helper is built by swiftc in a non-fatal try/catch, so on a Mac without full Xcode it never exists -- the guard would then always fail and fall through to rmSync, deleting a live instance's bundle. That is not a regression against main, which already gated copiedAppIsUsable on the same file, but the guard existed precisely to protect those launches and this took it away again. Split the two questions. icudtl.dat decides whether the bundle can run at all, so it gates reuse; the keyboard-layout helper is optional and its absence only degrades a keyboard feature, so it stays in copiedAppIsUsable (fully provisioned) and out of the reuse guard (runnable). Verified with a real orphan process holding the directory: healthy bundle -> reused, preserved no keyboard helper (no swiftc) -> reused, preserved missing icudtl.dat -> rebuilt, not wedged
Contributor
Author
|
@coderabbitai review |
Contributor
|
1 of 2 tasks
paidaxingyo666
pushed a commit
to paidaxingyo666/Manta
that referenced
this pull request
Aug 21, 2026
dallascrilley
pushed a commit
to dallascrilley/orca
that referenced
this pull request
Aug 27, 2026
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #15183 (now merged). Two dev-runner cache bugs found while reviewing it.
1. Unsigned bundles were cached permanently
The marker that marks a bundle reusable was written outside the codesign
try/catch:So a failed sign cached an unsigned bundle that was never rebuilt. The warning scrolls past once; every later launch silently reuses it. macOS refuses Notification Center registration for invalidly-signed apps, and with no stable cdhash the Keychain ACL can't match — it would resurface the exact prompt #15183 fixes, but sticky.
Now the marker is written only after a successful sign. A failure costs a re-copy per launch until signing works, and still doesn't block
pnpm dev— the documented non-fatal behavior is preserved.Mutation-verified: pointing codesign at a nonexistent binary produces the warning and leaves no marker.
2. Stale bundles were never reclaimed
out/electron-devonly ever cleared the directory it was about to rebuild. Siblings from renamed branches and past Electron upgrades accumulated forever — 143 directories / 38 GB measured across one set of worktrees, at ~270 MB each.Now siblings are pruned on every prepare, skipping any directory a live process is running from.
The liveness check deliberately avoids
pgrepMy first draft used
pgrep -af. macOSpgrephas no-a— that's a Linux procps extension. It silently ignores it and prints bare PIDs, so the path regex matched nothing,livePathscame back empty, every bundle looked stale, and the prune deleted a running instance's bundle. Caught in testing:It now parses
ps -Awwo command=(-wwso the command column isn't truncated), and skips pruning entirely if that call fails — treating a failure as "nothing live" is the dangerous direction.…and it's reclaimed once that instance exits. Pruning is scoped to the worktree's own
out/electron-dev, so it never touches another worktree.Tests
selectStaleDevBundleDirsis a pure function with 4 cases, including a path-boundary case so…/aaaa2isn't treated as in-use just because it prefixes…/aaaa. Full suite 1072 passing; typecheck, oxlint, max-lines ratchet clean.