refactor(desktop): externalize port allocation to setup/teardown scripts#1494
Conversation
Move port base allocation from the desktop app's local SQLite DB to a shared file (~/.superset/port-allocations.json) managed by setup and teardown scripts, making port allocation work independently of the desktop app.
|
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:
📝 WalkthroughWalkthroughModularized the setup/teardown tooling into sourced libraries and step scripts under .superset/lib; moved workspace port allocation and ports.json/env emission into the shell-based setup steps; removed desktop-side port allocation/portBase plumbing and deleted the Multi-Worktree plan doc. Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as ".superset/lib/setup/main.sh"
participant AllocFile as "ports.json / allocations file"
participant Desktop as "Desktop App"
participant Terminal as "Terminal Service"
participant Teardown as ".superset/lib/teardown/main.sh"
Setup->>AllocFile: acquire lock, compute/persist BASE mapping for cwd
Setup->>Setup: export SUPERSET_* vars, write workspace .env and ports.json
Desktop->>AllocFile: read ports.json / env at startup
Desktop->>Terminal: request createOrAttach (no portBase arg)
Terminal->>Desktop: session uses env-derived port mapping
Teardown->>AllocFile: acquire lock, remove mapping for cwd
Teardown->>Teardown: report deallocation outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.superset/setup.sh (1)
500-510:⚠️ Potential issue | 🟡 MinorFallback port values are inconsistent with the allocation scheme.
Lines 502-503 use
${API_PORT:-3041}and${ELECTRIC_PORT:-3049}as fallbacks, but withBASE=3000the offsets would yield3001and3009. These fallbacks would only be reached if the local variables from the block above somehow aren't set (shouldn't happen in practice), but the magic numbers are misleading if they ever do apply.
🤖 Fix all issues with AI agents
In @.superset/setup.sh:
- Around line 395-396: The startup sequence currently calls step_start_electric
before step_write_env so allocate_port_base() runs too late causing
SUPERSET_PORT_BASE to be unset when Electric boots; fix by moving the
allocate_port_base() call (or the entire step_write_env execution) earlier in
main() so allocate_port_base() runs before step_start_electric (or reorder the
steps so step_write_env precedes step_start_electric), ensuring
SUPERSET_PORT_BASE is set before Electric starts and ELECTRIC_PORT written to
.env matches the actual container port.
- Around line 350-385: The allocate_port_base function is vulnerable to a race
on port-allocations.json; wrap the entire read-modify-write sequence in an
exclusive lock (e.g., use flock) so two concurrent invocations cannot pick the
same candidate; specifically, acquire a lock (on a dedicated lockfile) before
reading with jq, computing used/candidate, and writing the temp file+mv, then
release the lock; ensure you do not run the locked work in a subshell so
exported SUPERSET_PORT_BASE from allocate_port_base propagates (use a grouped
command { ... } with fd-based flock or perform the work inside the lock and
write the chosen port to a temp file/variable that you then export in the
parent).
In @.superset/teardown.sh:
- Around line 245-271: step_deallocate_port relies on jq but
step_check_dependencies doesn't verify jq; add jq to the dependency list checked
by step_check_dependencies OR add an explicit pre-check inside
step_deallocate_port (use command -v jq >/dev/null 2>&1) and if missing call
warn and step_skipped (or return non-zero) so the function fails loudly instead
of silently on the jq call; reference the existing helper names
step_deallocate_port, step_check_dependencies, warn, and step_skipped when
making the change.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.superset/setup.sh:
- Around line 345-415: The mkdir-based locking in allocate_port_base can leave a
stale lock dir if the process dies; update allocate_port_base (and
step_deallocate_port) to detect and recover stale locks by writing the owner PID
into the lock dir when created, and before waiting check if the lock dir exists
and contains a PID whose process no longer exists (kill -0 PID) and if so remove
the stale lock; additionally set a trap in allocate_port_base to remove the lock
dir on EXIT/ERR/INT/TERM only if this process created it (use a local flag like
created_lock=1) so normal cleanup happens even on termination; alternatively you
may implement flock fallback by attempting flock on a temp lockfile before using
the mkdir+PID approach.
- Line 469: The script crashes under set -u because line using bare
$SUPERSET_PORT_BASE assumes allocate_port_base exported it; change that usage to
a safe expansion or fail early: replace the bare $SUPERSET_PORT_BASE reference
with ${SUPERSET_PORT_BASE:-} (matching step_start_electric) or, alternatively,
check the result of allocate_port_base (and exit from main with an error) so
that SUPERSET_PORT_BASE is guaranteed before continuing; reference
allocate_port_base, SUPERSET_PORT_BASE, step_start_electric and the set -u
option when making the change.
🧹 Nitpick comments (2)
.superset/setup.sh (2)
481-481: LocalELECTRIC_PORTshadows the exported variable fromstep_start_electric.Line 481 declares
local ELECTRIC_PORT=$((BASE + 9)), which shadows theELECTRIC_PORTexported on line 339. While the computed values match today, this coupling is implicit and fragile — if either offset or the export changes independently, the.envand the running container will silently disagree.Consider reusing the already-exported
ELECTRIC_PORTinstead of recomputing it, or at minimum adding a comment noting the intentional duplication.
529-536: Fallback defaults on lines 529–530 may diverge from the computed ports.
api_portfalls back to3041andelectric_portto3049(hardcoded), but thelocaldeclarations on lines 473 and 481 derive these fromBASE. Ifstep_write_envsomehow reaches this point with empty locals (e.g., after theset -ufix adds an early guard), the.dev.varsfile would use stale defaults inconsistent with the allocated base.These fallbacks should ideally also be derived from
SUPERSET_PORT_BASE, or be removed if the early-exit guard makes them unreachable.
| allocate_port_base() { | ||
| local alloc_file="$HOME/.superset/port-allocations.json" | ||
| local lock_dir="$HOME/.superset/port-allocations.lock" | ||
| local start=3000 | ||
| local range=20 | ||
|
|
||
| # Ensure directory and file exist | ||
| mkdir -p "$HOME/.superset" | ||
| if [ ! -f "$alloc_file" ]; then | ||
| echo '{}' > "$alloc_file" | ||
| fi | ||
|
|
||
| # Acquire lock (mkdir is atomic across processes) | ||
| local waited=0 | ||
| local timeout=30 | ||
| while ! mkdir "$lock_dir" 2>/dev/null; do | ||
| if [ "$waited" -ge "$timeout" ]; then | ||
| error "Timed out waiting for port allocation lock: $lock_dir" | ||
| return 1 | ||
| fi | ||
| sleep 1 | ||
| waited=$((waited + 1)) | ||
| done | ||
|
|
||
| local key="$PWD" | ||
| local existing | ||
| if ! existing=$(jq -r --arg k "$key" '.[$k] // empty' "$alloc_file" 2>/dev/null); then | ||
| error "Failed to read port allocations: $alloc_file" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
|
|
||
| if [ -n "$existing" ]; then | ||
| export SUPERSET_PORT_BASE="$existing" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 0 | ||
| fi | ||
|
|
||
| # Collect used port bases | ||
| local used | ||
| if ! used=$(jq -r '[.[]] | sort | .[]' "$alloc_file" 2>/dev/null); then | ||
| error "Failed to parse used port allocations: $alloc_file" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
|
|
||
| # Find first available slot | ||
| local candidate=$start | ||
| while echo "$used" | grep -qx "$candidate" 2>/dev/null; do | ||
| candidate=$((candidate + range)) | ||
| done | ||
|
|
||
| # Write allocation | ||
| local tmp_file="${alloc_file}.tmp.$$" | ||
| if ! jq --arg k "$key" --argjson v "$candidate" '. + {($k): $v}' "$alloc_file" > "$tmp_file"; then | ||
| error "Failed to write updated port allocations" | ||
| rm -f "$tmp_file" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
| if ! mv "$tmp_file" "$alloc_file"; then | ||
| error "Failed to persist port allocations" | ||
| rm -f "$tmp_file" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
|
|
||
| export SUPERSET_PORT_BASE="$candidate" | ||
| rmdir "$lock_dir" 2>/dev/null || true | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Stale lock directory can permanently block port allocation.
The mkdir-based lock has no stale-lock detection. If setup.sh is killed (SIGKILL, OOM, power loss, etc.) between acquiring the lock (line 360) and releasing it (line 413), the lock directory persists indefinitely. All subsequent allocate_port_base and step_deallocate_port calls will time out after 30 seconds and fail.
Consider one of:
- Store the owning PID inside the lock dir and treat it as stale if that PID no longer exists.
- Add a
trapto clean up the lock on ERR/EXIT/INT/TERM (with a guard to only remove if this process created it). - Fall back to
flockif available, withmkdiras a secondary.
Option 1 — PID-based stale detection
# Acquire lock (mkdir is atomic across processes)
local waited=0
local timeout=30
while ! mkdir "$lock_dir" 2>/dev/null; do
+ # Check for stale lock
+ if [ -f "$lock_dir/pid" ]; then
+ local lock_pid
+ lock_pid=$(cat "$lock_dir/pid" 2>/dev/null || echo "")
+ if [ -n "$lock_pid" ] && ! kill -0 "$lock_pid" 2>/dev/null; then
+ warn "Removing stale port allocation lock (PID $lock_pid no longer running)"
+ rmdir "$lock_dir" 2>/dev/null || rm -rf "$lock_dir"
+ continue
+ fi
+ fi
if [ "$waited" -ge "$timeout" ]; then
error "Timed out waiting for port allocation lock: $lock_dir"
return 1
fi
sleep 1
waited=$((waited + 1))
done
+ echo $$ > "$lock_dir/pid"🤖 Prompt for AI Agents
In @.superset/setup.sh around lines 345 - 415, The mkdir-based locking in
allocate_port_base can leave a stale lock dir if the process dies; update
allocate_port_base (and step_deallocate_port) to detect and recover stale locks
by writing the owner PID into the lock dir when created, and before waiting
check if the lock dir exists and contains a PID whose process no longer exists
(kill -0 PID) and if so remove the stale lock; additionally set a trap in
allocate_port_base to remove the lock dir on EXIT/ERR/INT/TERM only if this
process created it (use a local flag like created_lock=1) so normal cleanup
happens even on termination; alternatively you may implement flock fallback by
attempting flock on a temp lockfile before using the mkdir+PID approach.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.superset/lib/setup/steps.sh:
- Line 113: The jq filter interpolates $WORKSPACE_NAME directly into the query
when computing EXISTING_BRANCH, which is unsafe; change the jq invocation in the
EXISTING_BRANCH assignment to pass WORKSPACE_NAME via jq's --arg (e.g., use
--arg workspace "$WORKSPACE_NAME" and reference $workspace inside the filter) so
special characters are escaped and injections prevented, and keep the existing
stderr redirection and .id extraction logic intact.
- Around line 349-370: The port-assignment block in step_write_env assumes
SUPERSET_PORT_BASE is set and will cause an unbound variable error if
allocate_port_base failed; update the start of the port section to guard or fail
fast: check presence of SUPERSET_PORT_BASE (reference symbols: step_write_env,
allocate_port_base, SUPERSET_PORT_BASE, setup_main) and either abort with a
clear error/log and non-zero exit when it is unset, or provide a safe default
before computing WEB_PORT/API_PORT/... so the rest of the function won't hit set
-u; ensure this guard runs before any use of BASE or the local WEB_PORT,
API_PORT, etc.
In @.superset/lib/teardown/steps.sh:
- Around line 93-106: The SIGKILL loop runs immediately after sending SIGTERM,
giving processes no time to exit; add a short configurable grace period (e.g.,
sleep for a few seconds) between the SIGTERM loop and the SIGKILL loop so
processes matching matched_pids can handle SIGTERM and exit before escalation.
Update the teardown logic around the kill_process_tree calls and the counters
(root_killed, force_killed) to sleep (use a variable like TERM_GRACE_SECONDS)
after the first loop and before the second loop so the delay can be tuned.
🧹 Nitpick comments (5)
.gitignore (1)
53-54: Clarify.supersetignore intent; current rule only ignoresports.json.If more generated artifacts exist under
.superset, consider ignoring the folder broadly and explicitly un-ignoring the tracked scripts/config (or remove this rule ifports.jsonshould be committed). This avoids accidental commits as.supersetgrows.✅ Example pattern (if you want to ignore all generated artifacts but keep scripts/config)
# Superset (track scripts/config; ignore generated workspace artifacts) -.superset/ports.json +.superset/* +!.superset/setup.sh +!.superset/teardown.sh +!.superset/lib/ +!.superset/ports.json.superset/lib/common.sh (1)
115-144:print_summaryrelies on caller-initialized arrays.
FAILED_STEPSandSKIPPED_STEPSmust be initialized as arrays beforeprint_summaryis called — underset -u(whichteardown.shenables), referencing an unset array will error. Bothsetup_mainandteardown_maindo initialize these, so this works correctly today. A defensivedeclare -aor${FAILED_STEPS[@]+"${FAILED_STEPS[@]}"}guard would make this more robust against future misuse, but it's not blocking..superset/lib/teardown/steps.sh (1)
128-128:WORKSPACE_NAMEis set as a global variable, potentially leaking into the caller's scope.Line 128 assigns
WORKSPACE_NAMEwithoutlocal, so it modifies/creates a global shell variable. Same issue on line 172 withWORKSPACE_NAME, line 158 withNEON_PROJECT_ID, line 165 withBRANCH_ID, and line 136 withELECTRIC_CONTAINER. Since these are sourced functions, the variables persist across steps. This appears intentional for cross-step usage (e.g.,WORKSPACE_NAMEreused in later steps), but worth confirming it's deliberate rather than accidental..superset/lib/setup/steps.sh (1)
199-211: Unquoted$port_flagrelies on word splitting.Line 205 intentionally leaves
$port_flagunquoted so it splits into-pand3009:3000as separate arguments. This works but is fragile — ShellCheck would flag it (SC2086), and it can break if the value ever contains spaces in unexpected places.Proposed: use an array for docker args
- local port_flag ELECTRIC_PORT=$((SUPERSET_PORT_BASE + 9)) - port_flag="-p $ELECTRIC_PORT:3000" + local -a port_args=(-p "$ELECTRIC_PORT:3000") if ! docker run -d \ --name "$ELECTRIC_CONTAINER" \ - $port_flag \ + "${port_args[@]}" \ -e DATABASE_URL="$DIRECT_URL" \.superset/lib/setup/main.sh (1)
19-57: Steps continue after critical failures, causing cascading errors.The orchestration runs all steps regardless of earlier failures. If
step_load_envfails (e.g.,SUPERSET_ROOT_PATHnot set), every subsequent step that depends on environment variables will also fail, producing a noisy summary. Consider short-circuiting after critical prerequisite steps (env loading, dependency check) to provide clearer diagnostics.Example: early exit on prerequisite failures
# Step 1: Load environment if ! step_load_env; then step_failed "Load environment variables" + print_summary "Setup" + return 1 fi # Step 2: Check dependencies if ! step_check_dependencies; then step_failed "Check dependencies" + print_summary "Setup" + return 1 fi
| fi | ||
|
|
||
| # Now safe to parse with jq - use // empty for fallback | ||
| EXISTING_BRANCH=$(echo "$branches_output" | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id // empty" 2>/dev/null) |
There was a problem hiding this comment.
jq filter uses shell string interpolation — vulnerable to injection and special characters.
$WORKSPACE_NAME is interpolated directly into the jq filter string. If the workspace name contains characters like ", \, or other jq-special characters, this will break the filter or produce incorrect results. Use --arg for safe parameterization.
Proposed fix
- EXISTING_BRANCH=$(echo "$branches_output" | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id // empty" 2>/dev/null)
+ EXISTING_BRANCH=$(echo "$branches_output" | jq -r --arg name "$WORKSPACE_NAME" '.[] | select(.name == $name) | .id // empty' 2>/dev/null)📝 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.
| EXISTING_BRANCH=$(echo "$branches_output" | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id // empty" 2>/dev/null) | |
| EXISTING_BRANCH=$(echo "$branches_output" | jq -r --arg name "$WORKSPACE_NAME" '.[] | select(.name == $name) | .id // empty' 2>/dev/null) |
🤖 Prompt for AI Agents
In @.superset/lib/setup/steps.sh at line 113, The jq filter interpolates
$WORKSPACE_NAME directly into the query when computing EXISTING_BRANCH, which is
unsafe; change the jq invocation in the EXISTING_BRANCH assignment to pass
WORKSPACE_NAME via jq's --arg (e.g., use --arg workspace "$WORKSPACE_NAME" and
reference $workspace inside the filter) so special characters are escaped and
injections prevented, and keep the existing stderr redirection and .id
extraction logic intact.
| # Port allocation for multi-worktree dev instances | ||
| # Each workspace gets a range of 20 ports from its base. | ||
| # Offsets: +0 web, +1 api, +2 marketing, +3 admin, +4 docs, | ||
| # +5 desktop vite, +6 notifications, +7 streams, +8 streams internal, +9 electric, | ||
| # +10 caddy (HTTP/2 reverse proxy for electric-proxy), +11 code inspector, +12 electric-proxy (wrangler dev) | ||
| local BASE=$SUPERSET_PORT_BASE | ||
|
|
||
| # App ports (fixed offsets from base) | ||
| local WEB_PORT=$((BASE)) | ||
| local API_PORT=$((BASE + 1)) | ||
| local MARKETING_PORT=$((BASE + 2)) | ||
| local ADMIN_PORT=$((BASE + 3)) | ||
| local DOCS_PORT=$((BASE + 4)) | ||
| local DESKTOP_VITE_PORT=$((BASE + 5)) | ||
| local DESKTOP_NOTIFICATIONS_PORT=$((BASE + 6)) | ||
| local STREAMS_PORT=$((BASE + 7)) | ||
| local STREAMS_INTERNAL_PORT=$((BASE + 8)) | ||
| local ELECTRIC_PORT=$((BASE + 9)) | ||
| local CADDY_ELECTRIC_PORT=$((BASE + 10)) | ||
| local CODE_INSPECTOR_PORT=$((BASE + 11)) | ||
| local ELECTRIC_PROXY_PORT=$((BASE + 12)) | ||
|
|
There was a problem hiding this comment.
step_write_env will hit an unbound variable error if allocate_port_base failed.
setup_main does not short-circuit on step failure — it records failures and continues. If allocate_port_base fails, SUPERSET_PORT_BASE will be unset. Line 354 (local BASE=$SUPERSET_PORT_BASE) will trigger an unbound variable error under set -u (set in setup.sh). Consider adding a guard or making step_write_env depend explicitly on the port base being available.
Proposed fix: guard at the top of the port section
+ if [ -z "${SUPERSET_PORT_BASE:-}" ]; then
+ error "SUPERSET_PORT_BASE not set (port allocation may have failed)"
+ return 1
+ fi
+
local BASE=$SUPERSET_PORT_BASE📝 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.
| # Port allocation for multi-worktree dev instances | |
| # Each workspace gets a range of 20 ports from its base. | |
| # Offsets: +0 web, +1 api, +2 marketing, +3 admin, +4 docs, | |
| # +5 desktop vite, +6 notifications, +7 streams, +8 streams internal, +9 electric, | |
| # +10 caddy (HTTP/2 reverse proxy for electric-proxy), +11 code inspector, +12 electric-proxy (wrangler dev) | |
| local BASE=$SUPERSET_PORT_BASE | |
| # App ports (fixed offsets from base) | |
| local WEB_PORT=$((BASE)) | |
| local API_PORT=$((BASE + 1)) | |
| local MARKETING_PORT=$((BASE + 2)) | |
| local ADMIN_PORT=$((BASE + 3)) | |
| local DOCS_PORT=$((BASE + 4)) | |
| local DESKTOP_VITE_PORT=$((BASE + 5)) | |
| local DESKTOP_NOTIFICATIONS_PORT=$((BASE + 6)) | |
| local STREAMS_PORT=$((BASE + 7)) | |
| local STREAMS_INTERNAL_PORT=$((BASE + 8)) | |
| local ELECTRIC_PORT=$((BASE + 9)) | |
| local CADDY_ELECTRIC_PORT=$((BASE + 10)) | |
| local CODE_INSPECTOR_PORT=$((BASE + 11)) | |
| local ELECTRIC_PROXY_PORT=$((BASE + 12)) | |
| # Port allocation for multi-worktree dev instances | |
| # Each workspace gets a range of 20 ports from its base. | |
| # Offsets: +0 web, +1 api, +2 marketing, +3 admin, +4 docs, | |
| # +5 desktop vite, +6 notifications, +7 streams, +8 streams internal, +9 electric, | |
| # +10 caddy (HTTP/2 reverse proxy for electric-proxy), +11 code inspector, +12 electric-proxy (wrangler dev) | |
| if [ -z "${SUPERSET_PORT_BASE:-}" ]; then | |
| error "SUPERSET_PORT_BASE not set (port allocation may have failed)" | |
| return 1 | |
| fi | |
| local BASE=$SUPERSET_PORT_BASE | |
| # App ports (fixed offsets from base) | |
| local WEB_PORT=$((BASE)) | |
| local API_PORT=$((BASE + 1)) | |
| local MARKETING_PORT=$((BASE + 2)) | |
| local ADMIN_PORT=$((BASE + 3)) | |
| local DOCS_PORT=$((BASE + 4)) | |
| local DESKTOP_VITE_PORT=$((BASE + 5)) | |
| local DESKTOP_NOTIFICATIONS_PORT=$((BASE + 6)) | |
| local STREAMS_PORT=$((BASE + 7)) | |
| local STREAMS_INTERNAL_PORT=$((BASE + 8)) | |
| local ELECTRIC_PORT=$((BASE + 9)) | |
| local CADDY_ELECTRIC_PORT=$((BASE + 10)) | |
| local CODE_INSPECTOR_PORT=$((BASE + 11)) | |
| local ELECTRIC_PROXY_PORT=$((BASE + 12)) |
🤖 Prompt for AI Agents
In @.superset/lib/setup/steps.sh around lines 349 - 370, The port-assignment
block in step_write_env assumes SUPERSET_PORT_BASE is set and will cause an
unbound variable error if allocate_port_base failed; update the start of the
port section to guard or fail fast: check presence of SUPERSET_PORT_BASE
(reference symbols: step_write_env, allocate_port_base, SUPERSET_PORT_BASE,
setup_main) and either abort with a clear error/log and non-zero exit when it is
unset, or provide a safe default before computing WEB_PORT/API_PORT/... so the
rest of the function won't hit set -u; ensure this guard runs before any use of
BASE or the local WEB_PORT, API_PORT, etc.
| for pid in $matched_pids; do | ||
| if kill -0 "$pid" 2>/dev/null; then | ||
| kill_process_tree "$pid" TERM | ||
| root_killed=$((root_killed + 1)) | ||
| fi | ||
| done | ||
|
|
||
| # Escalate to SIGKILL for any survivors after graceful termination. | ||
| for pid in $matched_pids; do | ||
| if kill -0 "$pid" 2>/dev/null; then | ||
| kill_process_tree "$pid" KILL | ||
| force_killed=$((force_killed + 1)) | ||
| fi | ||
| done |
There was a problem hiding this comment.
No grace period between SIGTERM and SIGKILL.
The SIGKILL loop (lines 101-106) runs immediately after the SIGTERM loop (lines 93-98). Processes typically need a brief window to handle SIGTERM and clean up. Without a delay, SIGKILL will almost always fire for any non-trivial process.
Proposed fix: add a short sleep between signal rounds
for pid in $matched_pids; do
if kill -0 "$pid" 2>/dev/null; then
kill_process_tree "$pid" TERM
root_killed=$((root_killed + 1))
fi
done
+ # Give processes a moment to handle SIGTERM before escalating
+ if [ "$root_killed" -gt 0 ]; then
+ sleep 2
+ fi
+
# Escalate to SIGKILL for any survivors after graceful termination.
for pid in $matched_pids; do📝 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.
| for pid in $matched_pids; do | |
| if kill -0 "$pid" 2>/dev/null; then | |
| kill_process_tree "$pid" TERM | |
| root_killed=$((root_killed + 1)) | |
| fi | |
| done | |
| # Escalate to SIGKILL for any survivors after graceful termination. | |
| for pid in $matched_pids; do | |
| if kill -0 "$pid" 2>/dev/null; then | |
| kill_process_tree "$pid" KILL | |
| force_killed=$((force_killed + 1)) | |
| fi | |
| done | |
| for pid in $matched_pids; do | |
| if kill -0 "$pid" 2>/dev/null; then | |
| kill_process_tree "$pid" TERM | |
| root_killed=$((root_killed + 1)) | |
| fi | |
| done | |
| # Give processes a moment to handle SIGTERM before escalating | |
| if [ "$root_killed" -gt 0 ]; then | |
| sleep 2 | |
| fi | |
| # Escalate to SIGKILL for any survivors after graceful termination. | |
| for pid in $matched_pids; do | |
| if kill -0 "$pid" 2>/dev/null; then | |
| kill_process_tree "$pid" KILL | |
| force_killed=$((force_killed + 1)) | |
| fi | |
| done |
🤖 Prompt for AI Agents
In @.superset/lib/teardown/steps.sh around lines 93 - 106, The SIGKILL loop runs
immediately after sending SIGTERM, giving processes no time to exit; add a short
configurable grace period (e.g., sleep for a few seconds) between the SIGTERM
loop and the SIGKILL loop so processes matching matched_pids can handle SIGTERM
and exit before escalation. Update the teardown logic around the
kill_process_tree calls and the counters (root_killed, force_killed) to sleep
(use a variable like TERM_GRACE_SECONDS) after the first loop and before the
second loop so the delay can be tuned.
1 participant
Summary
~/.superset/port-allocations.json) managed by the setup and teardown scriptsportBasecolumn in the local DB schema is left in place but is no longer written to or read fromChanges
Setup script (
.superset/setup.sh)allocate_port_base()function that reads/writes~/.superset/port-allocations.json, keyed by$PWD, reusing existing allocations or finding the first available slot (start=3000, range=20)allocate_port_baseat the beginning ofstep_write_env()soSUPERSET_PORT_BASEis always setif [ -n "${SUPERSET_PORT_BASE:-}" ]guards around port env vars andports.jsongenerationTeardown script (
.superset/teardown.sh)step_deallocate_port()that removes the$PWDentry from the allocations filemain()as a new step after Neon branch deletionDesktop app — remove all
portBaseplumbingdb-helpers.ts: DeleteallocatePortBase(),START_PORT,PORT_RANGEconstants, unusedascimportcreate.ts: RemoveallocatePortBaseimport andportBasefrom 3 workspace insert callsenv.ts: RemoveportBaseparam frombuildTerminalEnv()andSUPERSET_PORT_BASEinjectiontypes.ts: RemoveportBasefromCreateSessionParamsdaemon-manager.ts: RemoveportBasefrom destructuring andbuildTerminalEnv()callsession.ts: RemoveportBasefrom destructuring andbuildTerminalEnv()callterminal.ts: RemoveportBase: workspace?.portBasefromcreateOrAttach()callTest Plan
bun run typecheckpassesbun run lint:fixpasses.superset/setup.shin a worktree — should create~/.superset/port-allocations.jsonwith an entry.superset/teardown.sh— should remove the entrySUPERSET_PORT_BASEenv var from desktopSummary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation