fix: prevent postinstall infinite loop on bun install#1064
Conversation
📝 WalkthroughWalkthroughThe postinstall script in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
The postinstall script was spawning hundreds of electron-builder processes in an infinite loop. This happened because: 1. postinstall runs `bun run --filter=@superset/desktop install:deps` 2. install:deps runs `electron-builder install-app-deps` 3. electron-builder triggers nested bun installs 4. Those installs re-trigger postinstall, repeating the cycle This fix adds an environment variable guard to detect and prevent recursive postinstall invocations. Fixes superset-sh#1063 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
spanishflu-est1918
force-pushed
the
fix/postinstall-infinite-loop
branch
from
54f7804 to
1691204
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/postinstall.sh`:
- Around line 1-16: Enable strict shell error handling at the top of the script
so failures in sherif or the bun command abort the postinstall: add POSIX-safe
strict mode (set -euo pipefail) before any commands that can fail, keep the
SUPERSET_POSTINSTALL_RUNNING guard and export as-is, and ensure sherif and the
bun install line run under that strict mode so any non-zero exit will terminate
the script immediately.
| #!/bin/bash | ||
| # Prevent infinite recursion during postinstall | ||
| # electron-builder install-app-deps can trigger nested bun installs | ||
| # which would re-run postinstall, spawning hundreds of processes | ||
|
|
||
| if [ -n "$SUPERSET_POSTINSTALL_RUNNING" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| export SUPERSET_POSTINSTALL_RUNNING=1 | ||
|
|
||
| # Run sherif for workspace validation | ||
| sherif | ||
|
|
||
| # Install native dependencies for desktop app | ||
| bun run --filter=@superset/desktop install:deps |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
bash strict mode set -e set -u set -o pipefail best practices error handling
💡 Result:
What “strict mode” does (and what it doesn’t)
Common pattern:
set -euo pipefailset -e(errexit): exit when an unhandled command fails, but it is intentionally skipped in several contexts (tests inif/while/until, most&&/||lists, non-last pipeline elements unlesspipefail, and when status is inverted with!). So you still need explicit checks where you expect failures. [2]set -u(nounset): treat unset variables as an error. [1]set -o pipefail: a pipeline fails if any element fails (specifically: returns the last non‑zero status, or 0 if all succeed). Disabled by default. [1]
Best-practice pattern (robust + debuggable errors)
1) Prefer explicit error handling + a trap, not blind reliance on set -e
Use a single error handler that prints context:
#!/usr/bin/env bash
set -Eeuo pipefail
die() { echo "ERROR: $*" >&2; exit 1; }
trap 'die "Command failed (exit=$?) at ${BASH_SOURCE[0]}:${LINENO}: ${BASH_COMMAND}"' ERRWhy -E? It helps ERR traps propagate into functions/subshell contexts (without it, you’ll miss failures in places you thought were covered). [2]
2) When failure is expected, handle it explicitly
Under -e, don’t “probe” with commands that might fail unless you capture/branch on the status:
grep -q pattern file || echo "not found (this is ok)"
# or
if ! grep -q pattern file; then
echo "not found"
fiThis aligns with “always check return values” guidance. [3]
3) Be careful with pipefail + early-terminating consumers (head, grep -q)
With pipefail, producers can die with SIGPIPE when the downstream command exits early; that can abort your script even though the overall behavior is fine. You may need to explicitly allow it, or restructure:
# allow SIGPIPE-ish failures if you truly don’t care:
set +e
big_producer | head -n 1
set -e(Or avoid such pipelines / use tools designed for it.)
4) Avoid “hidden” errexit holes ShellCheck warns about
Example: ! cmd can suppress errexit unexpectedly unless you explicitly exit/check status. [4]
set -e
! some_check || exit 15) If you need POSIX sh, don’t use pipefail
pipefail is not POSIX; either require bash, or implement an alternative. [5]
Key takeaway
set -euo pipefail is a good baseline, but robust Bash comes from: (a) explicit checks where failure is expected, (b) an ERR trap for actionable diagnostics, and (c) knowing the documented contexts where errexit/pipefail don’t behave like exceptions. [1][2][3]
Sources: GNU Bash manual for pipefail/nounset and errexit/ERR-trap exceptions [1][2], Google Shell Style Guide on checking return values and error reporting [3], ShellCheck wiki on ! vs errexit and portability of pipefail [4][5].
🏁 Script executed:
# First, let's see the full postinstall.sh file to understand the complete context
cat -n scripts/postinstall.shRepository: superset-sh/superset
Length of output: 609
🏁 Script executed:
# Let's check if there are other bash scripts in the repo to see patterns for error handling
fd -e sh -e bash . --type f | head -20Repository: superset-sh/superset
Length of output: 205
🏁 Script executed:
# Check if postinstall.sh is referenced in package.json or other build configs
rg -i "postinstall" --max-count 10Repository: superset-sh/superset
Length of output: 653
🏁 Script executed:
# Check if other scripts in the same location use set -e or strict mode
head -5 scripts/lint.shRepository: superset-sh/superset
Length of output: 204
🏁 Script executed:
# Check the desktop build script for comparison
head -10 apps/desktop/create-release.shRepository: superset-sh/superset
Length of output: 407
Fail fast on sherif/bun errors (enable strict mode).
Without set -e (and ideally -u -o pipefail), a failed sherif command will not terminate the script, allowing subsequent commands to execute even after validation fails. Since this script runs as an npm/bun postinstall hook, such failures should halt the installation. Add strict mode to catch errors early.
♻️ Proposed change
#!/bin/bash
+set -euo pipefail
+
# Prevent infinite recursion during postinstall
# electron-builder install-app-deps can trigger nested bun installs
# which would re-run postinstall, spawning hundreds of processes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| # Prevent infinite recursion during postinstall | |
| # electron-builder install-app-deps can trigger nested bun installs | |
| # which would re-run postinstall, spawning hundreds of processes | |
| if [ -n "$SUPERSET_POSTINSTALL_RUNNING" ]; then | |
| exit 0 | |
| fi | |
| export SUPERSET_POSTINSTALL_RUNNING=1 | |
| # Run sherif for workspace validation | |
| sherif | |
| # Install native dependencies for desktop app | |
| bun run --filter=@superset/desktop install:deps | |
| #!/bin/bash | |
| set -euo pipefail | |
| # Prevent infinite recursion during postinstall | |
| # electron-builder install-app-deps can trigger nested bun installs | |
| # which would re-run postinstall, spawning hundreds of processes | |
| if [ -n "$SUPERSET_POSTINSTALL_RUNNING" ]; then | |
| exit 0 | |
| fi | |
| export SUPERSET_POSTINSTALL_RUNNING=1 | |
| # Run sherif for workspace validation | |
| sherif | |
| # Install native dependencies for desktop app | |
| bun run --filter=@superset/desktop install:deps |
🤖 Prompt for AI Agents
In `@scripts/postinstall.sh` around lines 1 - 16, Enable strict shell error
handling at the top of the script so failures in sherif or the bun command abort
the postinstall: add POSIX-safe strict mode (set -euo pipefail) before any
commands that can fail, keep the SUPERSET_POSTINSTALL_RUNNING guard and export
as-is, and ensure sherif and the bun install line run under that strict mode so
any non-zero exit will terminate the script immediately.
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
I can't consistently reproduce it but since this is a harmless/reversible change that doesn't affect behavior I can merge. Thanks @spanishflu-est1918
2 participants
Summary
bun installProblem
When running
bun install, the postinstall script spawns 370+electron-builder install-app-depsprocesses in an infinite loop, causing the install to never complete.Root cause:
electron-builder install-app-depstriggers nested bun installs which re-run postinstall, creating a recursive loop.Solution
Created a
scripts/postinstall.shwrapper that:SUPERSET_POSTINSTALL_RUNNINGenvironment variableTest plan
bun installfrom a clean clone - should complete without hangingbun run build- should build the desktop app successfullyFixes #1063
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.