Repository navigation
Conversation
Orphaned forge3 processes accumulated: instances with PPID=1 survived the menu bar app indefinitely, each still LISTENing on a loopback port (four observed locally, aged 9 minutes to 2 days, on ports 9754/9756/9759/9760). Root cause. `ForgeProcessHost.stop()` sent `kill(-pid, SIGTERM)` and scheduled the SIGKILL escalation 3 seconds later via `queue.asyncAfter`. forge3 never honours SIGTERM -- every one of the 652 healthy stops in the local log required the escalation -- so the kill depended entirely on the app surviving those 3 seconds. forge3 is also spawned into its own process group (POSIX_SPAWN_SETPGROUP), so it receives no group- or session-scoped signal when the app dies, and macOS has no PR_SET_PDEATHSIG equivalent. If the app died inside that window (force quit, crash, logout, Sparkle relaunch), the pending escalation died with it and forge3 leaked. Nothing swept orphans at startup, and the port allocator silently bind-probed past them, so the leak was invisible. The log signature is an orphan having `Started`/`Stopping` lines with no following `sending SIGKILL` or `exited with status`. Fix. Introduce `ForgeLifecycleGuardian`: the same binary re-executed in an internal mode. The app spawns the guardian, which spawns forge3 into the guardian-owned process group, and the app holds a framed control socket. Closing that socket -- which the kernel does unconditionally on app death, including SIGKILL -- makes the guardian terminate and reap the whole group. The guardian in turn exits as soon as forge3 exits, so the app observes a normal child exit. Group kills are ordered against PID reuse by observing forge3 with waitid(WNOWAIT) so the PGID stays pinned until the sole reap. This makes the coupling one-way, which is the intended invariant: app dies -> forge3 dies (except uncatchable kills of the guardian) forge3 dies -> app survives and restarts it The restart half is deliberate. The branch this work is ported from also made a forge3 failure terminate the menu app, deleting `scheduleRestart` and gutting `restartAfterReadinessFailure`. That policy is not adopted: `ServiceSupervisor` is unchanged and keeps full ownership of restart behaviour, including the exit-status-75 self-update handshake, which that policy would have turned into an app quit. `ServiceController` likewise no longer escalates a failed phase to termination. Verified with `testGuardianControlEOFKillsServiceGroupWithinBound`, which models app crash via control-channel EOF against a SIGTERM-ignoring process with a descendant, and asserts both die within 1.5s -- alongside the existing restart tests, which still pass unchanged.
Collaborator
Author
|
fixed in #45 |
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.
The bug
Orphaned
forge3processes accumulate. Locally I found four withPPID=1, aged 9 minutes to 2 days, each stillLISTENing on a loopback port (9754/9756/9759/9760) while the menu bar app was not running.The log signature of an orphan is a
Started+Stoppingpair with nothing after it:versus a healthy stop, which always produces three lines:
Root cause
ForgeProcessHost.stop()sentkill(-pid, SIGTERM)and scheduled the SIGKILL escalation 3 seconds later on its serial queue. Three facts combine:POSIX_SPAWN_SETPGROUP+posix_spawnattr_setpgroup(&attr, 0)), so it receives no group- or session-scoped signal when the app dies. macOS has noPR_SET_PDEATHSIGequivalent. This was chosen so the app cankill(-pid, …)forward to clean up descendants, but it severs the kernel's backward safety net.Nothing swept orphans at startup, and
LoopbackEndpoint.allocate()silently bind-probes past occupied ports, so the leak never surfaced. The port number was effectively a leak counter.Two plausible-looking theories were investigated and ruled out by the logs: the
reapExitedProcessLockedearly-return hang (zeroTermination watchdog expiredand zeroCould not reaplines), and restart churn invalidating a pending escalation (startLockedguards onpid == 0, andpidis only cleared afterwaitpidsucceeds, so a new spawn cannot pre-empt a live process's escalation).The fix
ForgeLifecycleGuardian— the same binary re-executed in an internal mode. The app spawns the guardian; the guardian spawns forge3 into the guardian-owned process group; the app holds a framed control socket.Closing that socket — which the kernel does unconditionally on app death, including SIGKILL — makes the guardian terminate and reap the entire group. Conversely the guardian exits as soon as forge3 exits, so the app sees an ordinary child exit.
Group kills are ordered against PID reuse by observing forge3 with
waitid(..., WNOWAIT), which pins the PGID until the single authoritative reap.This is strictly stronger than the previous in-app escalation, because it no longer depends on the app surviving any window.
The invariant
The restart half is a deliberate divergence. The branch this work is ported from also made a forge3 failure terminate the menu app: it deleted
scheduleRestartentirely and guttedrestartAfterReadinessFailureintostopAfterReadinessFailure, withAppDelegatecallingNSApp.terminate(nil)on unexpected exit.That policy is not adopted here.
ServiceSupervisor.swiftis deliberately unchanged — it keeps full ownership of restart behaviour, including the exit-status-75 self-update handshake, which the fail-fast policy would have turned into an app quit.ServiceControllerlikewise no longer escalates a failed phase to termination.Worth flagging for review: this conflict is semantic, not textual. The two sides edited different methods toward opposing goals, so a plain merge would have produced a clean result with
scheduleRestartsurviving but no longer called.Verification
swift buildclean;swift test150 tests, 1 skipped, 0 failures.testGuardianControlEOFKillsServiceGroupWithinBoundmodels app crash via control-channel EOF against atrap '' TERMprocess with a descendant, and asserts both die within 1.5s.testGuardianFailureKillsPersistentLeaderAndDescendantAndReportsUnexpectedExitcovers guardian SIGKILL.testUnexpectedExitSchedulesRestartWithNewEndpoint,testUpdateInstalledExitRestartsImmediately,testRepeatedUpdateInstalledExitUsesBackoff.scripts/test-packaging.shpasses.assemble-app.shinstalls only the app executable intoContents/MacOS/, so the guardian's dev-onlyForgeRuntimeLeaseTestHelperpreference cannot be reached in a shipped bundle; production always falls through toBundle.main.executableURL.Removed
testAppEntrypointLifecycleFailureTerminationProbeExitsPromptlyand the--forge-internal-lifecycle-termination-probeentry point existed solely to assert the fail-fast policy, so they were dropped with it. A comment inProcessIntegrationTestsrecords why.Not addressed
Pre-existing orphans are not swept at startup — this change prevents new ones but does not reclaim the four already running. A startup sweep would need PID-reuse validation (boot UUID + start time + executable path) to be safe, which is worth doing separately.