Skip to content

Commit 1564686

Browse files
authored
fix(cli): keep the service's XDG_STATE_HOME out of tool shells and name the port owner (#58815)
1 parent 585b5a4 commit 1564686

5 files changed

Lines changed: 153 additions & 7 deletions

File tree

‎packages/cli/src/server-process.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { PersistentPty } from "@opencode/schema/persistent-pty"
1212
import { HttpServer } from "effect/http"
1313
import { NetAddress } from "effect/net"
1414
import { Env } from "./env"
15+
import { PortOwner } from "./services/port-owner"
1516
import { ServiceConfig } from "./services/service-config"
1617
import { RetainedImage } from "./services/retained-image"
1718
import { ServiceRegistration } from "./services/service-registration"
@@ -76,7 +77,7 @@ const processEffect = Effect.fnUntraced(function* (options: Options) {
7677
: undefined
7778
if (findIncumbent !== undefined && (yield* findIncumbent) !== undefined) return
7879
// Desktop can launch the service directly without injecting its saved environment.
79-
Object.assign(process.env, config.env)
80+
Object.assign(process.env, ServiceConfig.processEnv(config.env))
8081
// Keep a package-manager or curl install replaceable while the service runs; Desktop updates its own copy.
8182
if (options.mode === "service" && process.platform === "win32" && RetainedImage.installed(global.home))
8283
yield* RetainedImage.retain(global.cache, "service")
@@ -174,9 +175,10 @@ const processEffect = Effect.fnUntraced(function* (options: Options) {
174175
const server = yield* launch.pipe(Effect.catchIf(addressInUse, () => Effect.void))
175176
if (server !== undefined) return server
176177
}
178+
const owner = port === undefined ? undefined : yield* PortOwner.describe(port, hostname)
177179
return yield* Effect.fail(
178180
new Error(
179-
`Managed service port ${port} on ${hostname} is already in use by another process. ` +
181+
`Managed service port ${port} on ${hostname} is already in use by ${owner ?? "another process"}. ` +
180182
"Configure another port with `opencode service set port <port>` and start the service again.",
181183
{ cause: error },
182184
),
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
import { AppProcess } from "@opencode/util/process"
2+
import { Effect } from "effect"
3+
import { ChildProcess } from "effect/process"
4+
import net from "node:net"
5+
6+
export * as PortOwner from "./port-owner"
7+
8+
// Best effort: names the process listening on a TCP port so a failed bind can say who holds it.
9+
// Windows has no cheap equivalent of lsof, so it keeps the generic message.
10+
export const describe = Effect.fnUntraced(function* (port: number, hostname?: string) {
11+
if (process.platform === "win32") return undefined
12+
const appProcess = yield* AppProcess.Service
13+
const run = (command: string, args: ReadonlyArray<string>) =>
14+
appProcess
15+
.run(ChildProcess.make(command, args, { stdin: "ignore", killSignal: "SIGKILL" }), {
16+
timeout: "1 second",
17+
maxOutputBytes: 4096,
18+
})
19+
.pipe(
20+
Effect.map((result) => (result.exitCode === 0 ? result.stdout.toString("utf8").trim() : "")),
21+
Effect.orElseSucceed(() => ""),
22+
)
23+
const pid = listeners(yield* run("lsof", ["-nP", `-iTCP:${port}`, "-sTCP:LISTEN", "-Fpn"])).find((listener) =>
24+
matches(listener.address, hostname),
25+
)?.pid
26+
if (pid === undefined) return undefined
27+
const command = (yield* run("ps", ["-o", "command=", "-p", pid])).replace(/\s+/g, " ")
28+
if (!command) return `PID ${pid}`
29+
return `PID ${pid} (${command.length > 200 ? command.slice(0, 199) + "…" : command})`
30+
})
31+
32+
function listeners(output: string) {
33+
const found: { pid: string; address: string }[] = []
34+
let pid: string | undefined
35+
for (const line of output.split("\n")) {
36+
if (line.startsWith("p")) pid = /^\d+$/.test(line.slice(1)) ? line.slice(1) : undefined
37+
if (line.startsWith("n") && pid !== undefined)
38+
found.push({ pid, address: line.slice(1, line.lastIndexOf(":")).replace(/^\[(.*)\]$/, "$1") })
39+
}
40+
return found
41+
}
42+
43+
// lsof shows wildcard listeners as `*`; those also hold the port for any specific address.
44+
function matches(address: string, hostname: string | undefined) {
45+
const host = hostname?.replace(/^\[(.*)\]$/, "$1")
46+
if (host === undefined || net.isIP(host) === 0 || host === "0.0.0.0" || host === "::") return true
47+
return address === "*" || address === host
48+
}

‎packages/cli/src/services/service-config.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,13 +131,22 @@ const paths = Effect.gen(function* () {
131131
}
132132
})
133133

134+
// Clients find the service at its registration under XDG_STATE_HOME. A saved value would move the
135+
// registration away from them, and children of the service (tool shells, a relaunched Desktop) would
136+
// look for it in the wrong place, so the service keeps the state directory it was launched with.
137+
export function processEnv(env: Info["env"]) {
138+
if (env === undefined) return undefined
139+
const { XDG_STATE_HOME: _state, ...rest } = env
140+
return rest
141+
}
142+
134143
export const options = Effect.fnUntraced(function* (input: { readonly checkVersion?: boolean } = {}) {
135144
const { file, legacyRegistrationFiles } = yield* paths
136145
yield* Effect.forEach(legacyRegistrationFiles, (legacy) => migrateRegistration(legacy, file))
137146
return {
138147
file,
139148
version: input.checkVersion ? OPENCODE_VERSION : undefined,
140-
env: (yield* read()).env,
149+
env: processEnv((yield* read()).env),
141150
command: [
142151
...selfCommand(),
143152
"serve",

‎packages/cli/test/service-lifecycle.test.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,11 @@ serviceTest(
116116
() => undefined,
117117
(error: Error) => error.message,
118118
)
119-
expect(conflict).toContain(`Managed service port ${home.port} on 127.0.0.1 is already in use by another process`)
119+
expect(conflict).toContain(
120+
`Managed service port ${home.port} on 127.0.0.1 is already in use by ${
121+
process.platform !== "win32" && Bun.which("lsof") ? `PID ${process.pid} (` : "another process"
122+
}`,
123+
)
120124
expect(conflict).toContain("opencode service set port <port>")
121125
expect(await servers(home)).toEqual([])
122126
expect(await Bun.file(home.registration).exists()).toBe(false)

‎packages/cli/test/service.test.ts‎

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ import { TestClock } from "effect/testing"
1010
import fs from "node:fs/promises"
1111
import os from "node:os"
1212
import path from "node:path"
13+
import { LayerNode } from "@opencode/util/effect/layer-node"
14+
import { AppProcess } from "@opencode/util/process"
15+
import { PortOwner } from "../src/services/port-owner"
1316
import { ServiceConfig } from "../src/services/service-config"
1417
import { ServiceRegistration } from "../src/services/service-registration"
1518
import { isolatedEnv, transpilerCache } from "./fixture/environment"
@@ -192,6 +195,72 @@ test("service config manages environment variables", async () => {
192195
}
193196
})
194197

198+
test("saved service environment cannot move the registration's state directory", async () => {
199+
expect(ServiceConfig.processEnv(undefined)).toBeUndefined()
200+
expect(ServiceConfig.processEnv({ XDG_STATE_HOME: "/custom/state", XDG_DATA_HOME: "/custom/data", A: "1" })).toEqual({
201+
XDG_DATA_HOME: "/custom/data",
202+
A: "1",
203+
})
204+
205+
const root = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-service-env-state-"))
206+
const layer = Global.layerWith({ config: path.join(root, "config"), state: path.join(root, "state") })
207+
try {
208+
await fs.mkdir(path.join(root, "config"), { recursive: true })
209+
await Bun.write(
210+
path.join(root, "config", ServiceConfig.filename()),
211+
JSON.stringify({ env: { XDG_STATE_HOME: path.join(root, "elsewhere"), A: "1" } }),
212+
)
213+
const options = await Effect.runPromise(
214+
ServiceConfig.options().pipe(Effect.provide(layer), Effect.provide(NodeFileSystem.layer)),
215+
)
216+
expect(options.env).toEqual({ A: "1" })
217+
expect(options.file).toBe(path.join(root, "state", ServiceConfig.filename()))
218+
} finally {
219+
await fs.rm(root, { recursive: true, force: true })
220+
}
221+
})
222+
223+
test.skipIf(process.platform === "win32" || !Bun.which("lsof"))("port owner names the listening process", async () => {
224+
using listener = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: () => new Response() })
225+
using wildcard = Bun.serve({ hostname: "0.0.0.0", port: 0, fetch: () => new Response() })
226+
const describe = (port: number | undefined, hostname?: string) =>
227+
Effect.runPromise(PortOwner.describe(port ?? 0, hostname).pipe(Effect.provide(LayerNode.compile(AppProcess.node))))
228+
expect(await describe(listener.port)).toStartWith(`PID ${process.pid} (`)
229+
expect(await describe(listener.port, "127.0.0.1")).toStartWith(`PID ${process.pid} (`)
230+
expect(await describe(listener.port, "localhost")).toStartWith(`PID ${process.pid} (`)
231+
expect(await describe(listener.port, "127.0.0.2")).toBeUndefined()
232+
expect(await describe(wildcard.port, "127.0.0.2")).toStartWith(`PID ${process.pid} (`)
233+
})
234+
235+
test.skipIf(process.platform === "win32")("port owner gives up on a lookup that ignores SIGTERM", async () => {
236+
const root = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-port-owner-stuck-"))
237+
const PATH = process.env.PATH
238+
try {
239+
await Bun.write(path.join(root, "lsof"), "#!/bin/sh\ntrap '' TERM\nsleep 10\n")
240+
await fs.chmod(path.join(root, "lsof"), 0o755)
241+
process.env.PATH = `${root}${path.delimiter}${PATH}`
242+
const started = Date.now()
243+
const owner = await Promise.race([
244+
Effect.runPromise(PortOwner.describe(1).pipe(Effect.provide(LayerNode.compile(AppProcess.node)))),
245+
Bun.sleep(4_000).then(() => "hung"),
246+
])
247+
expect(owner).toBeUndefined()
248+
expect(Date.now() - started).toBeLessThan(3_000)
249+
} finally {
250+
process.env.PATH = PATH
251+
await fs.rm(root, { recursive: true, force: true })
252+
}
253+
})
254+
255+
test.skipIf(process.platform === "win32")("port owner is unknown for a free port", async () => {
256+
const free = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: () => new Response() })
257+
const port = free.port ?? 0
258+
free.stop(true)
259+
expect(
260+
await Effect.runPromise(PortOwner.describe(port).pipe(Effect.provide(LayerNode.compile(AppProcess.node)))),
261+
).toBeUndefined()
262+
})
263+
195264
test("service filenames share release channels and identify preview channels", () => {
196265
expect(ServiceConfig.filename("latest")).toBe("service.json")
197266
expect(ServiceConfig.filename("dev")).toBe("service.json")
@@ -548,7 +617,7 @@ test("unresponsive managed port occupancy reports a bounded conflict", async ()
548617
await fs.mkdir(path.dirname(registration), { recursive: true })
549618
await fs.writeFile(
550619
path.join(root, "config", "opencode", "service-local.json"),
551-
JSON.stringify({ port: listener.port }),
620+
JSON.stringify({ port: listener.port, env: { XDG_STATE_HOME: path.join(root, "elsewhere") } }),
552621
)
553622
const stale = {
554623
id: "stale",
@@ -558,8 +627,17 @@ test("unresponsive managed port occupancy reports a bounded conflict", async ()
558627
password: "stale",
559628
}
560629
await fs.writeFile(registration, JSON.stringify(stale))
630+
// The port lookup is a child of the service, so it shows the state directory tools would inherit.
631+
const lsof = process.platform === "win32" ? null : Bun.which("lsof")
632+
if (lsof) {
633+
await Bun.write(
634+
path.join(root, "bin", "lsof"),
635+
`#!/bin/sh\nprintf %s "$XDG_STATE_HOME" > "${path.join(root, "inherited-state")}"\nexec "${lsof}" "$@"\n`,
636+
)
637+
await fs.chmod(path.join(root, "bin", "lsof"), 0o755)
638+
}
561639
const contender = Bun.spawn([process.execPath, path.join(import.meta.dir, "../src/index.ts"), "serve", "--service"], {
562-
env: serviceEnv(root),
640+
env: { ...serviceEnv(root), PATH: `${path.join(root, "bin")}${path.delimiter}${process.env.PATH}` },
563641
stderr: "pipe",
564642
stdout: "pipe",
565643
})
@@ -569,7 +647,12 @@ test("unresponsive managed port occupancy reports a bounded conflict", async ()
569647
const exitCode = await Promise.race([contender.exited, Bun.sleep(20_000).then(() => undefined)])
570648
expect(exitCode).toBe(1)
571649
const output = (await new Response(contender.stdout).text()) + (await new Response(contender.stderr).text())
572-
expect(output).toContain(`Managed service port ${listener.port} on 127.0.0.1 is already in use by another process`)
650+
expect(output).toContain(
651+
`Managed service port ${listener.port} on 127.0.0.1 is already in use by ${
652+
lsof ? `PID ${process.pid} (` : "another process"
653+
}`,
654+
)
655+
if (lsof) expect(await Bun.file(path.join(root, "inherited-state")).text()).toBe(path.join(root, "state"))
573656
expect(await Bun.file(registration).json()).toEqual(stale)
574657
} finally {
575658
contender.kill("SIGTERM")

0 commit comments

Comments
 (0)