Skip to content

Commit 04b26cf

Browse files
authored
fix(desktop): pin the Host port and stop reporting non-updatable builds as errors (#87)
## Related Issue No issue — both defects were reported directly from the packaged app. ## Problem **1. Every launch was a fresh browser profile.** `spawnPythinkerServer` started the Host with `--port 0`, so the window loaded `http://127.0.0.1:<random>` on every launch. `localStorage` is keyed by origin, so the web UI read an empty store each time. The visible symptom was the first-run onboarding dialog appearing on every launch, but it was never an onboarding bug — **every** persisted preference was being discarded: theme, colour scheme, UI font size, permission mode, thinking level, plan mode, dynamic-workflow and goal mode, starred models, unread state, and the active workspace. **2. Settings showed a red update error.** ``` Update error: ENOENT: no such file or directory, open '.../Pythinker.app/Contents/Resources/app-update.yml' ``` A locally packed build (`electron-builder --dir`) sets `app.isPackaged = true` but carries no `app-update.yml`, so the first check threw and surfaced as a failure. A build that simply cannot self-update should say so calmly. ## What changed - **Fixed Host port**: 24827 packaged, 24828 in development, with a validated `PYTHINKER_DESKTOP_PORT` override that fails loudly on a bad value rather than silently reverting to a default. - **No fallback port, by design.** A fallback would reintroduce the same silent data loss on exactly the machines most likely to hit a collision. A bind failure now shows a dialog naming the port and the override, offering **Retry** (rebuilds the supervisor and retries the same port, so the user can free it and continue) or **Quit**. - **Development pins a port too.** Otherwise "works in dev, loses settings in prod" stays invisible during normal development, which is the divergence that produced this report. - **Updater precheck**: `initUpdater`, `checkForUpdatesNow` and `quitAndInstallNow` all check for `app-update.yml` under `resourcesPath` first and report `disabled` when it is absent. No listeners are wired and no timers are scheduled on such a build. ### Design alternatives considered and rejected These were argued out before implementing, and the reasons are recorded here so they are not re-litigated later: - **Move preferences out of `localStorage`** into desktop-owned storage via IPC. Fixes today's keys, but leaves the next `localStorage` use silently broken and forks behaviour between the browser and Electron. - **Serve the renderer from a custom `app://` scheme.** `apps/pythinker-web/src/api/config.ts` derives both the HTTP base and the WebSocket URL from `window.location.origin`, so this forces endpoint injection plus a CORS and WebSocket-origin story into a client that has none — an architecture change to fix a storage bug. Worth revisiting only if fixed ports prove to fail in the field. - **A bounded fallback (try N, N+1, N+2).** Rejected above. ## Verification Run in this branch's worktree: - `pnpm --filter @pymodel/pythinker-desktop exec vitest run` — **85 passed** (79 on the base plus 6 new). Each new test was watched failing against the old behaviour before the source changed. - `pnpm --filter @pymodel/pythinker-desktop run typecheck` — exit 0. - `pnpm run lint` — exit 0. New coverage: `resolveDesktopPort` defaults per build type, a valid override winning, invalid overrides (non-numeric and out-of-range) throwing rather than falling back, `isPortInUseError` classification, the port reaching the spawned argv, and `initUpdater` staying `disabled` with no events wired when `app-update.yml` is absent. ## Known follow-up (not in this PR) Auto-update is still broken in **shipped** builds for a separate reason: `pythinker-code` publishes CLI releases continuously, so `/releases/latest` resolves to a CLI release with no `latest-mac.yml`, and electron-updater's public GitHub provider follows exactly that. The fix is to publish desktop releases to their own repository; that lands separately because it needs a cross-repo publishing credential. ## Checklist - [x] I have read the CONTRIBUTING document. - [x] I have linked a related issue, or explained the problem above. - [x] I have added tests that prove my feature works. - [x] Ran `gen-changesets` skill, or this PR needs no changeset. — `@pymodel/pythinker-desktop` is private and changeset-ignored. - [ ] Ran `gen-docs` skill, or this PR needs no doc update.
1 parent fa82753 commit 04b26cf

5 files changed

Lines changed: 168 additions & 26 deletions

File tree

apps/desktop/src/host-supervisor.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,26 @@ const DEFAULT_SHUTDOWN_TIMEOUT_MS = 5_000
99
const TASKKILL_TIMEOUT_MS = 5_000
1010
const MAX_STARTUP_OUTPUT_CHARS = 32_768
1111

12+
export const DESKTOP_PACKAGED_PORT = 24_827
13+
export const DESKTOP_DEV_PORT = 24_828
14+
15+
/** Resolve the fixed Host port, with an optional validated environment override. */
16+
export function resolveDesktopPort(env: NodeJS.ProcessEnv, isPackaged: boolean): number {
17+
const value = env['PYTHINKER_DESKTOP_PORT']
18+
if (value === undefined) return isPackaged ? DESKTOP_PACKAGED_PORT : DESKTOP_DEV_PORT
19+
20+
const port = Number(value)
21+
if (!/^\d+$/u.test(value) || !Number.isInteger(port) || port < 1 || port > 65_535) {
22+
throw new Error(`PYTHINKER_DESKTOP_PORT must be an integer from 1 to 65535; received ${JSON.stringify(value)}`)
23+
}
24+
return port
25+
}
26+
27+
/** Return whether Host output reports that its fixed port is already occupied. */
28+
export function isPortInUseError(message: string): boolean {
29+
return /EADDRINUSE|address already in use/iu.test(message)
30+
}
31+
1232
/** Incremental parser for the Web Host's canonical readiness line. */
1333
export interface ReadinessParser {
1434
/**
@@ -257,6 +277,8 @@ export interface SpawnPythinkerServerOptions {
257277
readonly cwd: string
258278
/** Frozen environment for the Host process. */
259279
readonly env: NodeJS.ProcessEnv
280+
/** Fixed loopback port for the Host server. */
281+
readonly port: number
260282
/** Run the Electron executable as its bundled Node runtime. */
261283
readonly electronRunAsNode?: boolean
262284
}
@@ -272,7 +294,7 @@ function streamAdapter(stream: NodeJS.ReadableStream): HostChild['stdout'] {
272294
}
273295

274296
/**
275-
* Spawn the production Pythinker server on an OS-assigned loopback port.
297+
* Spawn the production Pythinker server on a fixed loopback port.
276298
* @param options - Node runtime, built CLI and process environment.
277299
* @returns The child handle consumed by {@link createHostSupervisor}.
278300
*/
@@ -286,7 +308,7 @@ export function spawnPythinkerServer(options: SpawnPythinkerServerOptions): Host
286308
'run',
287309
'--foreground',
288310
'--port',
289-
'0',
311+
String(options.port),
290312
'--log-level',
291313
'error',
292314
], {

apps/desktop/src/main.ts

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,13 @@ import {
2323
setCrashPhase,
2424
track,
2525
} from '@pymodel/pythinker-telemetry'
26-
import { createHostSupervisor, spawnPythinkerServer, type HostSupervisor } from './host-supervisor'
26+
import {
27+
createHostSupervisor,
28+
isPortInUseError,
29+
resolveDesktopPort,
30+
spawnPythinkerServer,
31+
type HostSupervisor,
32+
} from './host-supervisor'
2733
import { createSplashWindow } from './splash'
2834
import {
2935
checkForUpdatesNow,
@@ -303,6 +309,7 @@ async function boot(): Promise<void> {
303309
if (bootQuitPromise !== undefined) return
304310
initializeDesktopTelemetry()
305311
const paths = hostPaths()
312+
const port = resolveDesktopPort(process.env, app.isPackaged)
306313
assertHostArtifacts(paths)
307314
const splash = createSplashWindow(desktopResources('splash'))
308315
const destroySplash = (): void => {
@@ -313,26 +320,43 @@ async function boot(): Promise<void> {
313320
const trayFrames = loadTrayImages()
314321
createTray(trayFrames)
315322
stopTrayAnimation = startTrayAnimation(trayFrames)
316-
host = createHostSupervisor({
317-
spawnHost: () => spawnPythinkerServer({
318-
...paths,
319-
env: {
320-
...process.env,
321-
PYTHINKER_DESKTOP: '1',
323+
for (;;) {
324+
host = createHostSupervisor({
325+
spawnHost: () => spawnPythinkerServer({
326+
...paths,
327+
env: {
328+
...process.env,
329+
PYTHINKER_DESKTOP: '1',
330+
},
331+
port,
332+
}),
333+
log: chunk => process.stderr.write(chunk),
334+
onUnexpectedExit: ({ code, signal }) => {
335+
console.error(`desktop Host exited unexpectedly (code ${String(code)}, signal ${String(signal)})`)
336+
void requestAppQuit()
322337
},
323-
}),
324-
log: chunk => process.stderr.write(chunk),
325-
onUnexpectedExit: ({ code, signal }) => {
326-
console.error(`desktop Host exited unexpectedly (code ${String(code)}, signal ${String(signal)})`)
327-
void requestAppQuit()
328-
},
329-
})
330-
try {
331-
hostOrigin = await host.start()
332-
track('desktop_server_ready')
333-
} catch (error) {
334-
track('desktop_server_failed')
335-
throw error
338+
})
339+
try {
340+
hostOrigin = await host.start()
341+
track('desktop_server_ready')
342+
break
343+
} catch (error) {
344+
track('desktop_server_failed')
345+
const message = error instanceof Error ? error.message : String(error)
346+
if (!isPortInUseError(message)) throw error
347+
348+
const result = await dialog.showMessageBox({
349+
type: 'error',
350+
buttons: ['Retry', 'Quit'],
351+
defaultId: 0,
352+
cancelId: 1,
353+
title: `${APP_NAME} needs its fixed port`,
354+
message: `${APP_NAME} cannot use port ${String(port)}. It needs this fixed port so settings persist between launches. Free the port, or set PYTHINKER_DESKTOP_PORT to an unused port.`,
355+
})
356+
if (result.response === 0) continue
357+
await requestAppQuit()
358+
return
359+
}
336360
}
337361
stopTrayAnimation?.()
338362
stopTrayAnimation = undefined

apps/desktop/src/updater.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
1-
import { readFileSync, writeFileSync } from 'node:fs'
1+
import { existsSync, readFileSync, writeFileSync } from 'node:fs'
22
import { join } from 'node:path'
33
import { app, type BrowserWindow } from 'electron'
44
import electronUpdater from 'electron-updater'
55

66
const { autoUpdater } = electronUpdater
77
const UPDATE_SETTINGS_FILE = 'update-settings.json'
8+
const UPDATES_UNAVAILABLE_MESSAGE = 'Updates are not available for this build'
89
const INITIAL_CHECK_DELAY_MS = 10_000
910
const CHECK_INTERVAL_MS = 4 * 60 * 60 * 1_000
1011

@@ -117,6 +118,20 @@ function clearTimers(): void {
117118
checkInterval = undefined
118119
}
119120

121+
function hasUpdateConfig(): boolean {
122+
return existsSync(join(process.resourcesPath, 'app-update.yml'))
123+
}
124+
125+
function disableUpdates(): UpdateState {
126+
updateState({
127+
status: 'disabled',
128+
message: UPDATES_UNAVAILABLE_MESSAGE,
129+
version: undefined,
130+
percent: undefined,
131+
})
132+
return state
133+
}
134+
120135
function scheduleChecks(): void {
121136
if (checkInterval !== undefined) return
122137
checkInterval = setInterval(() => {
@@ -181,6 +196,10 @@ export function initUpdater(
181196
clearTimers()
182197

183198
if (!app.isPackaged) return
199+
if (!hasUpdateConfig()) {
200+
disableUpdates()
201+
return
202+
}
184203

185204
try {
186205
autoUpdater.autoDownload = settings.autoUpdate
@@ -232,6 +251,7 @@ export async function checkForUpdatesNow(): Promise<UpdateState> {
232251
updateState({ status: 'disabled' })
233252
return state
234253
}
254+
if (!hasUpdateConfig()) return disableUpdates()
235255

236256
try {
237257
autoUpdater.autoDownload = true
@@ -251,6 +271,7 @@ export function quitAndInstallNow(): UpdateState {
251271
updateState({ status: 'disabled' })
252272
return state
253273
}
274+
if (!hasUpdateConfig()) return disableUpdates()
254275

255276
try {
256277
updateTelemetryTrack('desktop_update_install')

apps/desktop/tests/host-supervisor.spec.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
createReadinessParser,
66
type HostChild,
77
} from '../src/host-supervisor'
8+
import * as hostSupervisor from '../src/host-supervisor'
89

910
vi.mock('node:child_process', { spy: true })
1011

@@ -112,6 +113,34 @@ describe('desktop Host readiness', () => {
112113
})
113114
})
114115

116+
describe('desktop Host port', () => {
117+
it('uses fixed ports for packaged and development builds without an override', () => {
118+
expect(hostSupervisor.DESKTOP_PACKAGED_PORT).toBe(24_827)
119+
expect(hostSupervisor.DESKTOP_DEV_PORT).toBe(24_828)
120+
expect(hostSupervisor.resolveDesktopPort({}, true)).toBe(24_827)
121+
expect(hostSupervisor.resolveDesktopPort({}, false)).toBe(24_828)
122+
})
123+
124+
it('uses a valid port override for packaged and development builds', () => {
125+
const env = { PYTHINKER_DESKTOP_PORT: '45231' }
126+
127+
expect(hostSupervisor.resolveDesktopPort(env, true)).toBe(45_231)
128+
expect(hostSupervisor.resolveDesktopPort(env, false)).toBe(45_231)
129+
})
130+
131+
it.each(['not-a-port', '70000'])('rejects an invalid port override: %s', (value) => {
132+
expect(() => hostSupervisor.resolveDesktopPort({ PYTHINKER_DESKTOP_PORT: value }, true))
133+
.toThrow(new RegExp(`PYTHINKER_DESKTOP_PORT.*${value}`, 'u'))
134+
})
135+
136+
it('detects output that reports a port collision', () => {
137+
expect(hostSupervisor.isPortInUseError(
138+
'listen EADDRINUSE: address already in use 127.0.0.1:24827',
139+
)).toBe(true)
140+
expect(hostSupervisor.isPortInUseError('desktop Host exited before readiness (code 1, signal null)')).toBe(false)
141+
})
142+
})
143+
115144
describe('desktop Host supervisor', () => {
116145
it('starts one child for concurrent callers and returns its stdout readiness URL', async () => {
117146
const child = new FakeHostChild()
@@ -296,6 +325,7 @@ describe('desktop Host process', () => {
296325
cliEntry: '/Applications/Pythinker.app/Contents/Resources/host/node_modules/@pymodel/pythinker-code/dist/launcher.mjs',
297326
cwd: '/Users/tester',
298327
env: { PYTHINKER_DESKTOP: '1' },
328+
port: 24_827,
299329
electronRunAsNode: true,
300330
})
301331

@@ -307,7 +337,7 @@ describe('desktop Host process', () => {
307337
'run',
308338
'--foreground',
309339
'--port',
310-
'0',
340+
'24827',
311341
'--log-level',
312342
'error',
313343
],
@@ -341,6 +371,7 @@ describe('desktop Host process', () => {
341371
cliEntry: '/tmp/launcher.mjs',
342372
cwd: '/tmp',
343373
env: {},
374+
port: 24_827,
344375
})
345376
host.kill('SIGTERM')
346377

@@ -379,6 +410,7 @@ describe('desktop Host process', () => {
379410
cliEntry: '/tmp/launcher.mjs',
380411
cwd: '/tmp',
381412
env: {},
413+
port: 24_827,
382414
})
383415
host.kill('SIGTERM')
384416

@@ -404,6 +436,7 @@ describe('desktop Host process', () => {
404436
cliEntry: '/tmp/launcher.mjs',
405437
cwd: '/tmp',
406438
env: {},
439+
port: 24_827,
407440
})
408441
host.kill('SIGTERM')
409442

apps/desktop/tests/updater.spec.ts

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,49 @@ import { afterEach, describe, expect, it, vi } from 'vitest'
66
vi.mock('electron', () => ({
77
app: {
88
isPackaged: false,
9-
getPath: () => '',
9+
getPath: vi.fn(() => ''),
10+
once: vi.fn(),
1011
},
1112
}))
1213

1314
vi.mock('electron-updater', () => ({
14-
default: { autoUpdater: {} },
15+
default: {
16+
autoUpdater: {
17+
on: vi.fn(),
18+
checkForUpdates: vi.fn(),
19+
quitAndInstall: vi.fn(),
20+
},
21+
},
1522
}))
1623

24+
import { app } from 'electron'
25+
import electronUpdater from 'electron-updater'
1726
import {
27+
getUpdateState,
28+
initUpdater,
1829
readUpdateSettings,
1930
trackUpdateTransition,
2031
writeUpdateSettings,
2132
type UpdateState,
2233
} from '../src/updater'
2334

2435
const temporaryDirectories: string[] = []
36+
const resourcesPathDescriptor = Object.getOwnPropertyDescriptor(process, 'resourcesPath')
37+
const { autoUpdater } = electronUpdater
2538

2639
afterEach(() => {
2740
for (const directory of temporaryDirectories.splice(0)) {
2841
rmSync(directory, { recursive: true, force: true })
2942
}
43+
Object.defineProperty(app, 'isPackaged', { configurable: true, value: false })
44+
vi.mocked(app.getPath).mockReset()
45+
vi.mocked(app.getPath).mockReturnValue('')
46+
if (resourcesPathDescriptor === undefined) {
47+
Reflect.deleteProperty(process, 'resourcesPath')
48+
} else {
49+
Object.defineProperty(process, 'resourcesPath', resourcesPathDescriptor)
50+
}
51+
vi.clearAllMocks()
3052
})
3153

3254
function temporaryDirectory(): string {
@@ -76,3 +98,23 @@ describe('update telemetry transitions', () => {
7698
])
7799
})
78100
})
101+
102+
describe('packaged builds without update metadata', () => {
103+
it('disables updates without wiring updater events', () => {
104+
const directory = temporaryDirectory()
105+
writeUpdateSettings(directory, { autoUpdate: false })
106+
vi.mocked(app.getPath).mockReturnValue(directory)
107+
Object.defineProperty(app, 'isPackaged', { configurable: true, value: true })
108+
Object.defineProperty(process, 'resourcesPath', { configurable: true, value: directory })
109+
110+
initUpdater(() => undefined)
111+
112+
expect(getUpdateState()).toMatchObject({
113+
status: 'disabled',
114+
message: 'Updates are not available for this build',
115+
autoUpdate: false,
116+
})
117+
expect(autoUpdater.on).not.toHaveBeenCalled()
118+
expect(app.once).not.toHaveBeenCalled()
119+
})
120+
})

0 commit comments

Comments
 (0)