Skip to content

Commit fd69cd7

Browse files
fix(ports): follow-ups from CodeRabbit review of the merged port-fallback work
Three findings on code that landed via #62: - Gateway startup now resolves only AFTER the socket binds and rejects (clearing server + resetting boundGatewayPort) on a bind error. Before, startModelServer resolved right after scheduling listen, so a bind failure was swallowed, left a dead server set, and IPC reported a false 'restart succeeded'. Callers already .catch it. - Wiring test now guards against the fixed GATEWAY_PORT constant reappearing in setup (its stated intent) instead of LLAMA_PORT, which was never the concern there. - Product name in comments: 'Off Grid instance' -> 'Off Grid AI Desktop instance'. Wiring guards extended for the await-the-listen/reject-on-error shape.
1 parent df4e3b8 commit fd69cd7

4 files changed

Lines changed: 46 additions & 12 deletions

File tree

src/main/__tests__/port-fallback-wiring.test.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,21 @@ describe('model-server.ts — the gateway itself falls back off a held port', ()
5757
it('scans for a free gateway port with pickFreePort before listening', () => {
5858
expect(src).toMatch(/boundGatewayPort = \(await pickFreePort\(port\)\)/)
5959
// It binds the LIVE chosen port, not the fixed GATEWAY_PORT constant.
60-
expect(src).toMatch(/server\.listen\(boundGatewayPort/)
60+
expect(src).toMatch(/\.listen\(boundGatewayPort/)
6161
})
6262

6363
it('exposes the live gateway port via getGatewayPort()', () => {
6464
expect(src).toMatch(/getGatewayPort\(\): number\s*{\s*return boundGatewayPort/)
6565
})
6666

67+
it('startup resolves only after bind and rejects (clearing state) on a bind error', () => {
68+
// A bind failure must reject and drop the dead server, not resolve a false success that leaves
69+
// `server` set forever. Guard the await-the-listen shape: reject on error, null the server.
70+
expect(src).toMatch(/await new Promise<void>\(\(resolve, reject\)/)
71+
expect(src).toMatch(/once\('listening'/)
72+
expect(src).toMatch(/catch[\s\S]*?server = null[\s\S]*?throw e/)
73+
})
74+
6775
it('self-report routes advertise the LIVE bound port, never the preferred param', () => {
6876
// After a fallback, /, /health, /openapi.json, /docs, /v1 must point clients at the port the
6977
// gateway actually bound (boundGatewayPort), not the stale preferred `port` it may have moved off.
@@ -85,6 +93,8 @@ describe('setup.ts — health pings read the LIVE ports, never fixed constants',
8593

8694
it('pings the live gateway port via getGatewayPort(), not a GATEWAY_PORT constant', () => {
8795
expect(src).toMatch(/pingJson\(getGatewayPort\(\)\)/)
88-
expect(src).not.toMatch(/\bLLAMA_PORT\b/)
96+
// Guard the actual regression this describes: setup must not reach for the fixed gateway-port
97+
// constant again (it reads the live getGatewayPort()); LLAMA_PORT was never the concern here.
98+
expect(src).not.toMatch(/\bGATEWAY_PORT\b/)
8999
})
90100
})

src/main/free-port.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Gradual-increment free-port fallback for the loopback services (llama-server, gateway, media).
22
// The ports in shared/ports.ts are PREFERRED, not mandatory: if another app owns one (LM Studio on
3-
// :8439, a second Off Grid instance, anything else), we must not fight over it or dead-end — we scan
3+
// :8439, a second Off Grid AI Desktop instance, anything else), we must not fight over it or dead-end — we scan
44
// upward (+1, +2, …) for the first free port and bind there. The candidate math is pure (unit-
55
// tested); the socket probe is injected so selection is testable without real sockets.
66

src/main/media-server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ export class LoopbackMediaServer {
6868
}
6969

7070
private async listenOnFreePort(): Promise<void> {
71-
// The preferred media port (MEDIA_PORT) may be taken by another Off Grid instance; scan upward
71+
// The preferred media port (MEDIA_PORT) may be taken by another Off Grid AI Desktop instance; scan upward
7272
// for a free one. requestedPort 0 = let the OS assign (tests) — inherently free. urlFor() serves
7373
// the LIVE boundPort, so downstream links follow wherever it bound.
7474
const target =

src/main/model-server.ts

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -930,7 +930,7 @@ async function handleImageEdit(
930930
}
931931

932932
// ─── Server ──────────────────────────────────────────────────────────────────
933-
/** The port the gateway actually bound. Falls back off GATEWAY_PORT when it's taken (a 2nd Off Grid
933+
/** The port the gateway actually bound. Falls back off GATEWAY_PORT when it's taken (a 2nd Off Grid AI Desktop
934934
* instance); consumers (setup health ping, the Gateway UI) must read this LIVE value. */
935935
let boundGatewayPort = GATEWAY_PORT
936936
export function getGatewayPort(): number {
@@ -1220,15 +1220,39 @@ export async function startModelServer(port = GATEWAY_PORT): Promise<void> {
12201220
server.headersTimeout = 0 // no cap on time-to-headers
12211221
server.keepAliveTimeout = 60_000
12221222

1223-
server.on('error', (e) => console.error('[model-server]', e))
12241223
// The gateway has no authentication. Bind the socket itself to loopback so no
12251224
// route can become LAN-accessible through a missing per-handler authorization check.
1226-
server.listen(boundGatewayPort, GATEWAY_HOST, () => {
1227-
console.log(
1228-
`[model-server] multimodal gateway at http://${GATEWAY_HOST}:${boundGatewayPort}/v1`
1229-
)
1230-
})
1231-
startingGateway = false
1225+
// Resolve only AFTER the socket actually binds; a bind failure must reject and clear state so
1226+
// callers (which .catch it) can surface the failure and a later restart can retry — rather than a
1227+
// resolved promise leaving a dead `server` set forever and IPC reporting a false success.
1228+
const listening = server
1229+
try {
1230+
await new Promise<void>((resolve, reject) => {
1231+
const onError = (e: Error): void => {
1232+
listening.removeListener('listening', onListening)
1233+
reject(e)
1234+
}
1235+
const onListening = (): void => {
1236+
listening.removeListener('error', onError)
1237+
// Hand ongoing runtime errors to the logger now that startup succeeded.
1238+
listening.on('error', (e) => console.error('[model-server]', e))
1239+
console.log(
1240+
`[model-server] multimodal gateway at http://${GATEWAY_HOST}:${boundGatewayPort}/v1`
1241+
)
1242+
resolve()
1243+
}
1244+
listening.once('error', onError)
1245+
listening.once('listening', onListening)
1246+
listening.listen(boundGatewayPort, GATEWAY_HOST)
1247+
})
1248+
} catch (e) {
1249+
// Bind failed — drop the dead server and reset so a retry starts clean.
1250+
server = null
1251+
boundGatewayPort = GATEWAY_PORT
1252+
throw e
1253+
} finally {
1254+
startingGateway = false
1255+
}
12321256
}
12331257

12341258
export function stopModelServer(): void {

0 commit comments

Comments
 (0)