Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/opencode/src/file/watcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ export namespace FileWatcher {
return { sub }
},
async (state) => {
state.sub?.unsubscribe()
if (!state.sub) return
Instance.trackPromises([state.sub.unsubscribe()])
},
)

Expand Down
10 changes: 6 additions & 4 deletions packages/opencode/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ import { GithubCommand } from "./cli/cmd/github"
import { ExportCommand } from "./cli/cmd/export"
import { AttachCommand } from "./cli/cmd/attach"

const cancel = new AbortController()

process.on("unhandledRejection", (e) => {
Log.Default.error("rejection", {
e: e instanceof Error ? e.message : e,
Expand Down Expand Up @@ -129,6 +127,10 @@ try {
if (formatted) UI.error(formatted)
if (formatted === undefined) UI.error("Unexpected error, check log file at " + Log.file() + " for more details")
process.exitCode = 1
} finally {
// Some subprocesses don't react properly to SIGTERM and similar signals.
// Most notably, some docker-container-based MCP servers don't handle such signals unless
// run using `docker run --init`.
// Explicitly exit to avoid any hanging subprocesses.
process.exit();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any other cases besides the docker mcps?

Can't we just track the pid from transports and send a SIGKILL if they are still running post close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a shell script wrapping an stdio MCP that has the same behavior. I can send it to you later today. I can imagine there can be other cases where an MCP misbehaves in a similar way. I have seen a few MCP server implementations and the general vibe is not high quality or well thought out.

I would like to avoid using SIGKILL. Fact of the matter is, the actual MCP server process receives SIGTERM when process.exit() is called. I confirmed that by running strace on the container's PID. But from the POV of OC, the child process is not the MCP server, but rather the docker client managing the MCP server.

The PID from the transports is an implementation detail - I would like to avoid using that too.

Any subprocess not under our control can cause the hang. The process.exit is a panacea, and as long as we do proper awaits, it won't cause any issues. Which we do for the most part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell script + how I use it in opencode.json, as promised: https://gist.github.com/veracioux/3a9fe2ea06fca0d16852481030d9297e

}

cancel.abort()
4 changes: 1 addition & 3 deletions packages/opencode/src/lsp/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,7 @@ export namespace LSP {
}
},
async (state) => {
for (const client of state.clients) {
await client.shutdown()
}
Instance.trackPromises(state.clients.map(client => client.shutdown()))
},
)

Expand Down
4 changes: 1 addition & 3 deletions packages/opencode/src/mcp/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,7 @@ export namespace MCP {
}
},
async (state) => {
for (const client of Object.values(state.clients)) {
client.close()
}
Instance.trackPromises(Object.values(state.clients).map(client => client.close()))
},
)

Expand Down
2 changes: 1 addition & 1 deletion packages/opencode/src/project/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export async function InstanceBootstrap() {
await Plugin.init()
Share.init()
Format.init()
LSP.init()
await LSP.init()
FileWatcher.init()
File.init()
}
6 changes: 6 additions & 0 deletions packages/opencode/src/project/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ export const Instance = {
state<S>(init: () => S, dispose?: (state: Awaited<S>) => Promise<void>): () => S {
return State.create(() => Instance.directory, init, dispose)
},
/**
* Track promises, making sure they have settled before Instance disposal is allowed to complete.
*/
trackPromises(promises: Promise<any>[]) {
State.trackPromises(Instance.directory, promises)
},
async dispose() {
await State.dispose(Instance.directory)
},
Expand Down
69 changes: 58 additions & 11 deletions packages/opencode/src/project/state.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
import { Log } from "@/util/log"

export namespace State {
interface Entry {
state: any
dispose?: (state: any) => Promise<void>
}

const entries = new Map<string, Map<any, Entry>>()
const recordsByKey = new Map<string, { trackedPromises: Set<Promise<any>>, entries: Map<any, Entry> }>()

export function create<S>(root: () => string, init: () => S, dispose?: (state: Awaited<S>) => Promise<void>) {
return () => {
const key = root()
let collection = entries.get(key)
if (!collection) {
collection = new Map<string, Entry>()
entries.set(key, collection)
let record = recordsByKey.get(key)
if (!record) {
record = {
entries: new Map<string, Entry>(),
trackedPromises: new Set<Promise<any>>(),
}
recordsByKey.set(key, record)
}
const exists = collection.get(init)
const exists = record.entries.get(init)
if (exists) return exists.state as S
const state = init()
collection.set(init, {
record.entries.set(init, {
state,
dispose,
})
Expand All @@ -26,9 +31,51 @@ export namespace State {
}

export async function dispose(key: string) {
for (const [_, entry] of entries.get(key)?.entries() ?? []) {
if (!entry.dispose) continue
await entry.dispose(await entry.state)
}
const record = recordsByKey.get(key)
if (!record) return

let disposalFinished = false

setTimeout(() => {
if (!disposalFinished) {
Log.Default.warn("waiting for state disposal to complete... (this is usually a saving operation or subprocess shutdown)")
}
}, 1000).unref()

setTimeout(() => {
if (!disposalFinished) {
Log.Default.warn("state disposal is taking an unusually long time - if it does not complete in a reasonable time, please report this as a bug")
}
}, 10000).unref()

await Promise.allSettled(
[...record.entries.values()]
.map(async (entry) => await entry.dispose?.(await entry.state)),
).then((results) => {
for (const result of results) {
if (result.status === "rejected") {
Log.Default.error("Error while disposing state:", result.reason)
}
}
})

await Promise.allSettled([...record.trackedPromises.values()])
.then((results) => {
for (const result of results) {
if (result.status === "rejected") {
Log.Default.error("Error while disposing state:", result.reason)
}
}
})

disposalFinished = true
}

/**
* Track promises, making sure they have settled before state disposal is allowed to complete.
*/
export function trackPromises<T>(key: string, promises: Promise<T>[]) {
for (const promise of promises)
recordsByKey.get(key)!.trackedPromises.add(promise)
}
}
18 changes: 10 additions & 8 deletions packages/opencode/src/session/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,15 @@ export namespace SessionPrompt {
(messages) => insertReminders({ messages, agent }),
)
if (step === 0)
ensureTitle({
session,
history: msgs,
message: userMsg,
providerID: model.providerID,
modelID: model.info.id,
})
Instance.trackPromises([
ensureTitle({
session,
history: msgs,
message: userMsg,
providerID: model.providerID,
modelID: model.info.id,
})
])
step++
await processor.next()
await using _ = defer(async () => {
Expand Down Expand Up @@ -1635,7 +1637,7 @@ export namespace SessionPrompt {
thinkingBudget: 0,
}
}
generateText({
await generateText({
maxOutputTokens: small.info.reasoning ? 1500 : 20,
providerOptions: {
[small.providerID]: options,
Expand Down