-
Notifications
You must be signed in to change notification settings - Fork 888
fix: solve #2570 — restart terminal-host daemon on app startup to fix stale macOS security session #2571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+386
−0
Closed
fix: solve #2570 — restart terminal-host daemon on app startup to fix stale macOS security session #2571
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,303 @@ | ||
| import { beforeEach, describe, expect, it, mock, spyOn } from "bun:test"; | ||
| import { EventEmitter } from "node:events"; | ||
|
|
||
| /** | ||
| * Tests for TerminalHostClient.restartDaemon() | ||
| * | ||
| * Verifies that on app restart, the client shuts down any existing daemon | ||
| * and spawns a fresh one. This ensures the new daemon inherits the current | ||
| * user's security session context — critical on macOS where Fast User | ||
| * Switching can leave a stale daemon with a degraded security context | ||
| * (causing TLS/Keychain failures for Go binaries, `security` CLI, etc.). | ||
| * | ||
| * See: https://github.com/anthropics/superset/issues/2570 | ||
| */ | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Mocks | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const fsState = { | ||
| existingPaths: new Set<string>(), | ||
| writtenFiles: new Map<string, string>(), | ||
| unlinkedPaths: new Set<string>(), | ||
| readFiles: new Map<string, string>(), | ||
| }; | ||
|
|
||
| mock.module("node:fs", () => { | ||
| const realFs = require("node:fs"); | ||
| const overrides = { | ||
| existsSync: (path: string) => fsState.existingPaths.has(path), | ||
| readFileSync: (path: string, ...args: unknown[]) => { | ||
| const content = fsState.readFiles.get(path); | ||
| if (content !== undefined) return content; | ||
| return realFs.readFileSync(path, ...args); | ||
| }, | ||
| writeFileSync: (path: string, data: string) => { | ||
| fsState.writtenFiles.set(path, data); | ||
| fsState.existingPaths.add(path); | ||
| }, | ||
| unlinkSync: (path: string) => { | ||
| fsState.unlinkedPaths.add(path); | ||
| fsState.existingPaths.delete(path); | ||
| }, | ||
| mkdirSync: () => {}, | ||
| chmodSync: () => {}, | ||
| statSync: () => ({ mtimeMs: 0 }), | ||
| openSync: () => 3, | ||
| closeSync: () => {}, | ||
| }; | ||
| return { | ||
| ...realFs, | ||
| ...overrides, | ||
| default: { ...realFs, ...overrides }, | ||
| }; | ||
| }); | ||
|
|
||
| let connectFn: (path: string) => EventEmitter; | ||
|
|
||
| mock.module("node:net", () => ({ | ||
| connect: (path: string) => connectFn(path), | ||
| })); | ||
|
|
||
| mock.module("electron", () => ({ | ||
| app: { | ||
| isPackaged: false, | ||
| getAppPath: () => "/mock/app", | ||
| }, | ||
| })); | ||
|
|
||
| mock.module("shared/constants", () => ({ | ||
| SUPERSET_DIR_NAME: ".superset-test", | ||
| })); | ||
|
|
||
| const spawnCalls: Array<{ cmd: string; args: string[] }> = []; | ||
|
|
||
| /** Called by spawn mock to simulate daemon creating its socket file. */ | ||
| let onSpawn: (() => void) | null = null; | ||
|
|
||
| const mockChildProcess = new EventEmitter(); | ||
| Object.assign(mockChildProcess, { | ||
| pid: 99999, | ||
| unref: () => {}, | ||
| }); | ||
|
|
||
| mock.module("node:child_process", () => { | ||
| const realCp = require("node:child_process"); | ||
| const overrides = { | ||
| spawn: (cmd: string, args: string[]) => { | ||
| spawnCalls.push({ cmd, args }); | ||
| onSpawn?.(); | ||
| return mockChildProcess; | ||
| }, | ||
| }; | ||
| return { ...realCp, ...overrides, default: { ...realCp, ...overrides } }; | ||
| }); | ||
|
|
||
| const { TerminalHostClient } = await import("./client"); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const HOME = require("node:os").homedir(); | ||
| const SUPERSET_HOME = require("node:path").join(HOME, ".superset-test"); | ||
| const SOCKET_PATH = require("node:path").join( | ||
| SUPERSET_HOME, | ||
| "terminal-host.sock", | ||
| ); | ||
| const TOKEN_PATH = require("node:path").join( | ||
| SUPERSET_HOME, | ||
| "terminal-host.token", | ||
| ); | ||
| const PID_PATH = require("node:path").join(SUPERSET_HOME, "terminal-host.pid"); | ||
| const DAEMON_SCRIPT = "/mock/app/dist/main/terminal-host.js"; | ||
|
|
||
| function resetState() { | ||
| fsState.existingPaths.clear(); | ||
| fsState.writtenFiles.clear(); | ||
| fsState.unlinkedPaths.clear(); | ||
| fsState.readFiles.clear(); | ||
| spawnCalls.length = 0; | ||
| onSpawn = null; | ||
| } | ||
|
|
||
| function createMockSocket( | ||
| responseMap: Record<string, unknown> = {}, | ||
| ): EventEmitter { | ||
| const socket = new EventEmitter() as EventEmitter & { | ||
| write: (data: string) => boolean; | ||
| destroy: () => void; | ||
| unref: () => void; | ||
| setEncoding: (enc: string) => void; | ||
| remoteAddress: string; | ||
| }; | ||
| socket.remoteAddress = "mock"; | ||
| socket.unref = () => {}; | ||
| socket.setEncoding = () => {}; | ||
| socket.destroy = () => {}; | ||
|
|
||
| socket.write = (data: string) => { | ||
| try { | ||
| const req = JSON.parse(data.trim()); | ||
| const payload = responseMap[req.type]; | ||
| if (payload !== undefined) { | ||
| const response = JSON.stringify({ | ||
| id: req.id, | ||
| ok: true, | ||
| payload, | ||
| }); | ||
| setTimeout(() => socket.emit("data", `${response}\n`), 0); | ||
| } | ||
| } catch { | ||
| // Not NDJSON | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| setTimeout(() => socket.emit("connect"), 0); | ||
| return socket; | ||
| } | ||
|
|
||
| function createFailingSocket(): EventEmitter { | ||
| const socket = new EventEmitter() as EventEmitter & { | ||
| write: (data: string) => boolean; | ||
| destroy: () => void; | ||
| unref: () => void; | ||
| setEncoding: (enc: string) => void; | ||
| }; | ||
| socket.write = () => true; | ||
| socket.destroy = () => {}; | ||
| socket.unref = () => {}; | ||
| socket.setEncoding = () => {}; | ||
| setTimeout(() => socket.emit("error", new Error("ECONNREFUSED")), 0); | ||
| return socket; | ||
| } | ||
|
|
||
| const HELLO_RESPONSE = { | ||
| protocolVersion: 2, | ||
| daemonVersion: "1.0.0", | ||
| daemonPid: 99999, | ||
| }; | ||
|
|
||
| /** Set up onSpawn to simulate the daemon creating its socket + token files. */ | ||
| function simulateDaemonSpawn(token = "new-token-xyz") { | ||
| onSpawn = () => { | ||
| fsState.existingPaths.add(SOCKET_PATH); | ||
| fsState.existingPaths.add(TOKEN_PATH); | ||
| fsState.readFiles.set(TOKEN_PATH, token); | ||
| }; | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| describe("TerminalHostClient.restartDaemon", () => { | ||
| beforeEach(() => { | ||
| resetState(); | ||
| }); | ||
|
|
||
| it("shuts down a running daemon and removes stale files before spawning fresh", async () => { | ||
| // Existing daemon files | ||
| fsState.existingPaths.add(SOCKET_PATH); | ||
| fsState.existingPaths.add(TOKEN_PATH); | ||
| fsState.existingPaths.add(PID_PATH); | ||
| fsState.existingPaths.add(DAEMON_SCRIPT); | ||
| fsState.readFiles.set(TOKEN_PATH, "test-token-abc"); | ||
| fsState.readFiles.set(PID_PATH, "12345"); | ||
|
|
||
| simulateDaemonSpawn(); | ||
|
|
||
| let connectCount = 0; | ||
| connectFn = () => { | ||
| connectCount++; | ||
|
|
||
| // 1st connect: tryConnectControl for graceful shutdown | ||
| if (connectCount === 1) { | ||
| return createMockSocket({ | ||
| hello: HELLO_RESPONSE, | ||
| shutdown: { success: true }, | ||
| }); | ||
| } | ||
|
|
||
| // 2nd: waitForDaemonShutdown isSocketLive check — daemon gone | ||
| if (connectCount === 2) { | ||
| fsState.existingPaths.delete(SOCKET_PATH); | ||
| return createFailingSocket(); | ||
| } | ||
|
|
||
| // 3rd+: new daemon sockets (control + stream after spawn) | ||
| return createMockSocket({ hello: HELLO_RESPONSE }); | ||
| }; | ||
|
|
||
| const client = new TerminalHostClient(); | ||
| await client.restartDaemon(); | ||
|
|
||
| // Graceful shutdown was sent (socket accepted the shutdown request) | ||
| // and old token file was cleaned up (removed either by daemon or cleanup) | ||
| expect(fsState.unlinkedPaths.has(TOKEN_PATH)).toBe(true); | ||
|
|
||
| // New daemon was spawned | ||
| expect(spawnCalls.length).toBeGreaterThanOrEqual(1); | ||
| expect(spawnCalls[0]?.args[0]).toContain("terminal-host.js"); | ||
|
|
||
| client.dispose(); | ||
| }); | ||
|
|
||
| it("falls back to SIGTERM via PID file when socket is not connectable", async () => { | ||
| fsState.existingPaths.add(SOCKET_PATH); | ||
| fsState.existingPaths.add(PID_PATH); | ||
| fsState.existingPaths.add(TOKEN_PATH); | ||
| fsState.existingPaths.add(DAEMON_SCRIPT); | ||
| fsState.readFiles.set(PID_PATH, "12345"); | ||
|
|
||
| const killSpy = spyOn(process, "kill").mockImplementation(() => true); | ||
| simulateDaemonSpawn(); | ||
|
|
||
| let connectCount = 0; | ||
| connectFn = () => { | ||
| connectCount++; | ||
|
|
||
| // 1st: tryConnectControl fails (daemon unresponsive) | ||
| if (connectCount === 1) { | ||
| return createFailingSocket(); | ||
| } | ||
|
|
||
| // 2nd: waitForDaemonShutdown — socket gone | ||
| if (connectCount === 2) { | ||
| fsState.existingPaths.delete(SOCKET_PATH); | ||
| return createFailingSocket(); | ||
| } | ||
|
|
||
| // 3rd+: new daemon | ||
| return createMockSocket({ hello: HELLO_RESPONSE }); | ||
| }; | ||
|
|
||
| const client = new TerminalHostClient(); | ||
| await client.restartDaemon(); | ||
|
|
||
| expect(killSpy).toHaveBeenCalledWith(12345, "SIGTERM"); | ||
| expect(spawnCalls.length).toBeGreaterThanOrEqual(1); | ||
|
|
||
| killSpy.mockRestore(); | ||
| client.dispose(); | ||
| }); | ||
|
|
||
| it("spawns fresh daemon when no existing daemon is running", async () => { | ||
| // No socket/pid/token files — clean state | ||
| fsState.existingPaths.add(DAEMON_SCRIPT); | ||
| simulateDaemonSpawn("fresh-token"); | ||
|
|
||
| connectFn = () => { | ||
| // All connects go to the freshly spawned daemon | ||
| return createMockSocket({ hello: HELLO_RESPONSE }); | ||
| }; | ||
|
|
||
| const client = new TerminalHostClient(); | ||
| await client.restartDaemon(); | ||
|
|
||
| expect(spawnCalls.length).toBeGreaterThanOrEqual(1); | ||
| client.dispose(); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1411,6 +1411,65 @@ export class TerminalHostClient extends EventEmitter { | |||||||||||
| return response; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Restart the daemon by shutting down any existing instance and spawning a fresh one. | ||||||||||||
| * This ensures the new daemon inherits the current user's security session context, | ||||||||||||
| * which is critical on macOS when Fast User Switching changes the console owner. | ||||||||||||
| * | ||||||||||||
| * Sessions in the old daemon are lost, but the cold restore mechanism recovers | ||||||||||||
| * terminal scrollback from disk history. | ||||||||||||
| */ | ||||||||||||
| async restartDaemon(): Promise<void> { | ||||||||||||
| // Disconnect any existing sockets first | ||||||||||||
| this.resetConnectionState({ emitDisconnected: false }); | ||||||||||||
|
|
||||||||||||
| // Try graceful shutdown first | ||||||||||||
| const connected = await this.tryConnectControl(); | ||||||||||||
| if (connected) { | ||||||||||||
| try { | ||||||||||||
| const token = this.readAuthToken(); | ||||||||||||
| try { | ||||||||||||
| await this.authenticateControl({ token }); | ||||||||||||
| await this.sendRequest<EmptyResponse>("shutdown", { | ||||||||||||
| killSessions: false, | ||||||||||||
| }); | ||||||||||||
| } catch (error) { | ||||||||||||
| if (this.isProtocolMismatchError(error)) { | ||||||||||||
| this.resetConnectionState({ emitDisconnected: false }); | ||||||||||||
| await this.shutdownLegacyDaemon({ killSessions: false }); | ||||||||||||
| } else { | ||||||||||||
| // Auth or send failed - fall back to SIGTERM | ||||||||||||
| this.killDaemonFromPidFile(); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } catch { | ||||||||||||
| // Token missing - fall back to SIGTERM | ||||||||||||
| this.killDaemonFromPidFile(); | ||||||||||||
| } finally { | ||||||||||||
| this.resetConnectionState({ emitDisconnected: false }); | ||||||||||||
| } | ||||||||||||
| } else { | ||||||||||||
| // Socket not connectable but may still exist - try SIGTERM | ||||||||||||
| this.killDaemonFromPidFile(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| await this.waitForDaemonShutdown(); | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Check that shutdown actually completed before deleting daemon files and reconnecting, otherwise restart can leave two daemons running. Prompt for AI agents
Suggested change
|
||||||||||||
|
|
||||||||||||
| // Clean up stale files | ||||||||||||
| for (const path of [SOCKET_PATH, PID_PATH, TOKEN_PATH]) { | ||||||||||||
| try { | ||||||||||||
| if (existsSync(path)) unlinkSync(path); | ||||||||||||
| } catch { | ||||||||||||
| // Best effort | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Spawn fresh daemon and connect | ||||||||||||
| await this.connectAndAuthenticate(); | ||||||||||||
| this.connectionState = ConnectionState.CONNECTED; | ||||||||||||
| this.emit("connected"); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Shutdown the daemon if it's currently running, without spawning a new one. | ||||||||||||
| * Returns true if daemon was running and shutdown was sent, false if no daemon was running. | ||||||||||||
|
|
||||||||||||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Avoid PID-file SIGTERM when no daemon socket exists; stale PID files can kill unrelated processes.
Prompt for AI agents