Sign up
View stargazersSign up

fix(desktop): dev supervisor stops when its parent process dies#23571

merged6
main
fix/issue-19109
opened20d agobyJJulius Marminge
3 files changed
View on GitHubTwo-way sync enabledPR last imported 2h ago

Problem

apps/desktop/scripts/dev-electron.mjs, the desktop dev supervisor, only stopped on SIGINT, SIGTERM or SIGHUP. If its parent was killed without forwarding a signal (kill -9, a crash, a terminal that died first), the supervisor was reparented and kept running. It relaunched Electron on every rebuild and crash, and later vp run dev:desktop runs ended up with several supervisors on the same T3 home, closing each other's windows.

Repro on Linux (Xvfb), main 8d3b061121: start vp run dev:desktop --home-dir <dev home> and kill -9 the vp pack --watch process. The supervisor stays alive, reparented to the user's subreaper, and touch apps/desktop/dist-electron/main.cjs starts a new Electron. After a second vp run dev:desktop, two node scripts/dev-electron.mjs processes run at once.

Change

  • dev-electron.mjs saves process.ppid at startup and checks it once a second with an unref'd timer. When the value changes, the supervisor runs its normal shutdown(129), which stops Electron and its child tree, then exits. The check compares against the saved value instead of 1 because on Linux orphans go to a subreaper. It is skipped on Windows, which never reparents.
  • apps/desktop/vite.config.ts changes the tsdown onSuccess command to exec node scripts/dev-electron.mjs on non-Windows hosts. tsdown runs onSuccess through /bin/sh -c. On Debian and Ubuntu that shell is dash, which forks rather than exec'ing, so a resident sh stayed between vp pack and the supervisor and hid the parent's death. With exec, the supervisor is a direct child of vp pack. tsdown's tree-kill on rebuild still sends it SIGTERM.

Scope and approval

Fixes #19109 GitCafe: https://git.cafe/pingdotgg/t3code/issues/19109 Triaged bug via the automated issue flow; a maintainer confirmed it on the issue and labelled it accepted.

Verification

  • vp test run apps/desktop/scripts/electron-launcher.test.mjs: 8 passed. The new test, dev supervisor stops Electron and exits once its parent process goes away, runs the real dev-electron.mjs startup with a fake clock and a ppid getter. On main it fails with AssertionError: supervisor kept Electron running after its parent process went away: expected [] to include 'SIGTERM'.

  • vp lint on the three touched files: clean.

  • Runtime check of the real vp run dev:desktop tree on Linux (Xvfb, Electron 44, non-TTY, dash as /bin/sh):

    Process treeAfter kill -KILL <vp pack pid>
    main 8d3b061121vp pack --watch → /bin/sh -c node scripts/dev-electron.mjs → node scripts/dev-electron.mjs → electronsh reparented to subreaper 1452. The supervisor stays alive, and touch dist-electron/main.cjs starts a new Electron (PID 710825 → 711522).
    this branch 85ebfe32d2vp pack --watch → node scripts/dev-electron.mjs → electronThe supervisor and Electron are gone within 2 s, and the log shows before-quit received.

    On this branch, a rebuild (touch apps/desktop/src/main.ts) still replaces the supervisor through tsdown's SIGTERM, and the new supervisor is again a direct child of vp pack.

  • Not verified: macOS and Windows. macOS /bin/sh already exec'd the last command, and exec keeps that. Windows keeps the previous command and has no parent check.

Built with Claude Opus 5.5 (implementation, tests) and Claude Sonnet 5.5 (review) via the Claude Code harness in T3 Code.

🤖 Generated with Claude Code

Independent proof audit

Verdict: PASS_WITH_NOTES. The matrix ran on main 8d3b061 and head 85ebfe3 on Linux (dash /bin/sh, plus bash in a private mount namespace). Exit codes come from strace.

Rowmainhead
SIGKILL vp packOrphaned supervisor keeps relaunching Electron (18 relaunches), never exitsexit_group(129) at +1.5 s, session empty by +2 s
Same, after a rebuildBug reproduced129 at +1.55 s
SIGTERM vp pack onlySupervisor orphaned, never exits129 at +1.5 s (improvement)
SIGINT to the process group / SIGTERM to the session130 / 143130 / 143, same timing
RebuildNew supervisor under a new sh -cNew supervisor is a direct child of vp pack
/bin/sh = bashNo sh, bug still reproduces129 at +1.7 s / +2.1 s
unref timern/aDoes not keep the process alive (snippet exits in 0.04 s; control is held until timeout)
SIGKILL the grandparentSurvivesSurvives (out of scope: only the direct parent is watched)
Verification log
  • Proof audit pass 1 (Claude Opus 5.5): GAPS against the original transcript (no exit code, no Ctrl-C/SIGTERM rows, no bash row, optimistic 2 s). All of these are covered by the capture above.
  • Capture (Claude Opus 5.5): every row ran on both trees; main reproduces the bug, head passes every row.
  • Proof audit pass 2 (Claude Opus 5.5): PASS_WITH_NOTES. Row by row against the files, SHAs match, head row 1 rerun independently (129 at +2.32 s).
  • Second review (Claude Opus 5.5): VERDICT: MERGE. Low nits: the "without a shell in between" comment does not hold on the dev:electron task path, where the check is a no-op; the test does not assert exit code 129; the new it sits outside the describe block.
  • Unverified: macOS and Windows (from code: macOS reparents to launchd, so the check fires; Windows is unchanged with no parent check).
  • Evidence by Claude Opus 5.5 via the Claude Code harness in T3 Code.

Checks

22 successful · 19 skipped
All checks passed
2df1d9e

Added 3 commits

  • fix(desktop): launch the dev supervisor without a shell in between85ebfe3
  • test(desktop): reproduce dev-electron supervisor outliving its parent (#19109)9984f37
  • fix(desktop): dev supervisor exits when its parent goes awayd7af024
MmacroscopeappBot20d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This is a focused desktop development-process fix that monitors the supervisor's parent and preserves the process relationship across the pack hook, with dedicated test coverage. Human review is required because the PR also adds a line-level lint-diagnostic suppression.

No code changes detected at 2df1d9e. Prior analysis still applies.

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

CcoderabbitaiBot20d ago
Synced from GitHub

Explain Change →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between 422d377725964a0b9487825e2bc1d3223522121b and 2df1d9e6fa991e4f38fe795404b98d434c884c27.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The desktop development launcher selects its post-pack command by platform. On non-Windows platforms, the supervisor checks its parent PID every second and initiates shutdown if it changes. Startup tests verify that the supervisor signals the Electron child after the parent PID changes.

Changes: Electron supervisor lifecycle

Layer / File(s)Summary
Platform-specific launcher command
apps/desktop/vite.config.ts		The post-pack command runs the Node script directly on Windows and uses exec on other platforms. The command is set as onSuccess only when desktop development is enabled.
Parent PID monitoring and shutdown
apps/desktop/scripts/dev-electron.mjs, apps/desktop/scripts/electron-launcher.test.mjs		On non-Windows platforms, the supervisor checks whether its parent PID has changed and initiates shutdown with exit code 129 if it has. Tests use a fake clock to verify that the supervisor signals the Electron child after the parent PID changes.

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant Parent
3 participant Supervisor as dev-electron.mjs
4 participant Child as Electron child
5 Parent--xSupervisor: Parent process exits
6 Supervisor->>Supervisor: Detect changed process.ppid
7 Supervisor->>Child: Send SIGTERM
8 Supervisor->>Supervisor: Exit with code 129

Suggested reviewers: maria-rcks, t3dotgg

Merge Risk: External image Minimal · previous review 422d3

The change makes the desktop development supervisor exit when its direct parent dies. No merge-blocking risk was found. macOS and Windows behavior was not exercised.

Pre-merge checks: Passed 4

Check nameStatusExplanation
Title checkPassedThe title is concise, uses the repository's conventional format, and clearly describes the primary change: stopping the desktop development supervisor when its parent process dies.
Description checkPassedThe description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure, implementation, linked issue, test results, runtime evidence, limitations…
Linked Issues checkPassedActive direct issue [#19109] requires the stale desktop development supervisor to stop when its parent disappears. dev-electron.mjs records the startup process.ppid, checks it every second on non-…
Out of Scope Changes checkPassedThe changes remain within [#19109]. The exec command removes the intermediate shell that could hide direct-parent death. The shared startup harness and fake clock support regression coverage for the…
Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure, implementation, linked issue, test results, runtime evidence, limitations, and agent usage. The approval evidence is summarized rather than quoting a specific maintainer comment, but the description is otherwise complete.

Full details: Linked Issues check

Explanation

Active direct issue [#19109] requires the stale desktop development supervisor to stop when its parent disappears. dev-electron.mjs records the startup process.ppid, checks it every second on non-Windows hosts, and calls shutdown(129) when the parent changes. vite.config.ts uses exec on non-Windows hosts so the supervisor directly observes the vp pack parent. The regression test verifies SIGTERM delivery and supervisor exit after parent reparenting. The changes do not need to implement stale-supervisor cleanup through cleanupStaleDevApps() or address [#19110].

Full details: Out of Scope Changes check

Explanation

The changes remain within [#19109]. The exec command removes the intermediate shell that could hide direct-parent death. The shared startup harness and fake clock support regression coverage for the parent-death behavior. The Windows command path preserves existing behavior because Windows does not reparent processes.

Finishing Touches

Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
External image Recent review info

External image Run configuration

External image Commits

Reviewing files that changed from the base of the PR and between 910d41eb6e56560762cf6cdb5dd2c6a33acfc68a and 422d377725964a0b9487825e2bc1d3223522121b.

External image Files selected for processing: 1

  • apps/desktop/vite.config.ts

Limit details: You’ve used all 10 included reviews currently available.

Kkvnloo7d ago
Synced from GitHub

Note

Written by claude-opus-5-5 (Claude Code), reviewed by gpt-6.1-sol (Hermes), on behalf of Kevin

Independent check at 85ebfe32d2 on Linux with native bash /bin/sh and repo-local vite-plus 1.3.0.

The direct-parent fix works in the original runtime checks: killing pack-bin.js --watch stopped the supervisor after about 2.5 seconds in both head runs; the base supervisor survived in both runs and relaunched Electron after touching main.cjs.

One process-name qualification for the repro: the pnpm-installed vp produced this tree:

1node …/vite-plus/bin/vp pack --watch
2└─ node …/vite-plus/dist/pack-bin.js --watch
3 └─ node scripts/dev-electron.mjs
4 └─ electron

The process to kill for the supported direct-parent row here is pack-bin.js, not the outer bin/vp wrapper. The original extra row reported that killing the wrapper leaves its child and supervisor running. That matches the PR's explicitly out-of-scope grandparent row, not a failure of the direct-parent guarantee.

Steps: use T3CODE_DESKTOP_DEV=1 node_modules/.bin/vp pack --watch in apps/desktop, a private HOME, a headless X server, and the required server bundle/dev HTTP endpoint. Record the parent PID before sending SIGKILL.

Not checked: full vp run dev:desktop topology, macOS, Windows, global native vp.

Added 1 commit

  • Merge origin/main into fix/issue-19109910d41e
JJulius Marminge16h ago

Evidence

Ran the real Electron desktop on Linux/Xvfb at 1920×1080 with fresh isolated homes and seeding disabled. Recorded the supervisor's direct parent, killed only that PID with SIGKILL, waited 8 seconds, then touched apps/desktop/dist-electron/main.cjs. Repeated after a source rebuild created a replacement supervisor. The footer shows live PID observations; only the setup machine label was redacted.

BeforeAfter
3a8c250cb0910d41eb6e
Initial supervisor recording: orphan supervisor survives and Electron reopens.Initial supervisor recording: Electron closes, supervisor exits, and the file touch causes no relaunch.
After rebuild recording: replacement supervisor survives and reopens Electron.After rebuild recording: replacement supervisor exits and Electron stays closed.

Reproduction driver · Process transcript · Video redaction script

AI-assisted; verified with the tests and tools listed above.

Not shown:

  • macOS parent-death behavior was not exercised; this comparison used Linux.

Added 1 commit

  • Merge branch 'main' into fix/issue-19109422d377
JJulius Marminge5h ago

The automated pipeline stopped on this pull request and will not merge it:

someone else pushed to fix/issue-19109 (head is now 422d377725); the pipeline does not build on commits it did not review

It stays open for a person to finish or close.

Added 1 commit

  • Merge branch 'main' into fix/issue-191092df1d9e