Skip to content

Commit 60629d4

Browse files
anandgupta42claude
andcommitted
fix: address code review findings for crash trace flush
- thread.ts: re-raise signal after cleanup to restore default termination behavior (prevents process from hanging) - worker.ts: replace one-shot `hasFlushed` guard with "preserve first reason" pattern so later flushes still run - worker.ts: defer `sessionTraces.delete()` until `endTrace()` completes so crash flush can still reach in-flight traces Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 60039f8 commit 60629d4

2 files changed

Lines changed: 46 additions & 24 deletions

File tree

packages/opencode/src/cli/cmd/tui/thread.ts

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -154,21 +154,30 @@ export const TuiThreadCommand = cmd({
154154

155155
// altimate_change start — crash: flush worker traces on signals
156156
// Bun Workers don't receive OS signals — only the main thread does.
157-
// On SIGINT/SIGTERM/SIGHUP, terminate the worker and briefly wait so
158-
// its "exit" handler fires and flushes all active session traces to disk.
159-
// Bun.sleepSync gives the worker thread time to process the termination
160-
// before the main thread continues to process.exit().
161-
const emergencyTerminate = () => {
162-
try {
163-
worker.terminate()
164-
Bun.sleepSync(50)
165-
} catch {
166-
// best-effort — crash handler must never throw
157+
// On SIGINT/SIGTERM/SIGHUP, terminate the worker so its "exit" handler
158+
// fires and flushes all active session traces to disk.
159+
// After cleanup, remove the handler and re-raise the signal to restore
160+
// default behavior (process termination). This avoids swallowing signals
161+
// which would leave the process running after a kill.
162+
const makeSignalHandler = (signal: NodeJS.Signals) => {
163+
const handler = () => {
164+
try {
165+
worker.terminate()
166+
Bun.sleepSync(50)
167+
} catch {
168+
// best-effort — crash handler must never throw
169+
}
170+
process.off(signal, handler)
171+
process.kill(process.pid, signal)
167172
}
173+
return handler
168174
}
169-
process.on("SIGINT", emergencyTerminate)
170-
process.on("SIGTERM", emergencyTerminate)
171-
process.on("SIGHUP", emergencyTerminate)
175+
const onSigint = makeSignalHandler("SIGINT")
176+
const onSigterm = makeSignalHandler("SIGTERM")
177+
const onSighup = makeSignalHandler("SIGHUP")
178+
process.on("SIGINT", onSigint)
179+
process.on("SIGTERM", onSigterm)
180+
process.on("SIGHUP", onSighup)
172181
// altimate_change end
173182

174183
let stopped = false
@@ -179,9 +188,9 @@ export const TuiThreadCommand = cmd({
179188
process.off("unhandledRejection", error)
180189
process.off("SIGUSR2", reload)
181190
// altimate_change start — crash: remove emergency handlers on clean shutdown
182-
process.off("SIGINT", emergencyTerminate)
183-
process.off("SIGTERM", emergencyTerminate)
184-
process.off("SIGHUP", emergencyTerminate)
191+
process.off("SIGINT", onSigint)
192+
process.off("SIGTERM", onSigterm)
193+
process.off("SIGHUP", onSighup)
185194
// altimate_change end
186195
await withTimeout(client.call("shutdown", undefined), 5000).catch((error) => {
187196
Log.Default.warn("worker shutdown failed", {

packages/opencode/src/cli/cmd/tui/worker.ts

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,15 @@ function getOrCreateTrace(sessionID: string): Trace | null {
114114

115115
const startEventStream = (input: { directory: string; workspaceID?: string }) => {
116116
if (eventStream.abort) eventStream.abort.abort()
117-
// Clear stale per-stream trace state before starting a new stream instance
117+
// altimate_change start — crash: flush stale traces before clearing
118+
// Flush any in-flight traces synchronously before clearing — endTrace() is
119+
// async and a crash during the gap would lose trace data.
118120
for (const [, trace] of sessionTraces) {
119121
void trace.endTrace().catch(() => {})
120122
}
121123
sessionTraces.clear()
122124
sessionUserMsgIds.clear()
125+
// altimate_change end
123126

124127
const abort = new AbortController()
125128
eventStream.abort = abort
@@ -242,9 +245,15 @@ const startEventStream = (input: { directory: string; workspaceID?: string }) =>
242245
if (status === "idle" && sid) {
243246
const trace = sessionTraces.get(sid)
244247
if (trace) {
245-
void trace.endTrace().catch(() => {})
246-
sessionTraces.delete(sid)
247-
sessionUserMsgIds.delete(sid)
248+
// altimate_change start — crash: defer deletion until endTrace() completes
249+
// Keep the trace in sessionTraces during async teardown so
250+
// flushAllTracesSync() can still reach it if a crash occurs
251+
// while endTrace() is in flight.
252+
void trace.endTrace().catch(() => {}).finally(() => {
253+
sessionTraces.delete(sid)
254+
sessionUserMsgIds.delete(sid)
255+
})
256+
// altimate_change end
248257
}
249258
}
250259
}
@@ -338,13 +347,17 @@ Rpc.listen(rpc)
338347
// NOTE: Bun Workers do NOT receive OS signals (SIGINT, SIGTERM, SIGHUP) —
339348
// those are delivered only to the main thread. Signal-based flush is handled
340349
// in thread.ts by terminating the worker, which triggers the "exit" event here.
341-
let hasFlushed = false
350+
let firstFlushReason: string | undefined
342351
function flushAllTracesSync(reason: string) {
343-
if (hasFlushed) return
344-
hasFlushed = true
352+
// Preserve the most specific reason from the first flush (e.g., the uncaught
353+
// exception message) even if a later handler (exit) calls again with a
354+
// generic reason. Subsequent calls still flush — new traces may have been
355+
// created since the first call.
356+
const effectiveReason = firstFlushReason ?? reason
357+
firstFlushReason ??= reason
345358
for (const [, trace] of sessionTraces) {
346359
try {
347-
trace.flushSync(reason)
360+
trace.flushSync(effectiveReason)
348361
} catch {
349362
// flushSync is best-effort — must never throw in an exit handler
350363
}

0 commit comments

Comments
 (0)