Skip to content

Commit 0b5ecd1

Browse files
saravmajesticclaude
andcommitted
fix: [AI-7743] keep Bus mirror off the question Deferred critical path
Address review feedback (flagged by all reviewers): the `Bus.publish` mirrors added for /event (webview) clients sat on the Deferred critical path. Since `Effect.promise` converts a promise rejection into an unrecoverable fiber defect, a `Bus.publish` rejection could abort the fiber before `Deferred.succeed`/`Deferred.fail` ran — leaving the awaiting `Question.ask()` hung on "Thinking…", the exact failure this PR fixes. - Add a best-effort `mirror()` helper: `Effect.promise(() => Bus.publish(...))` recovered with `Effect.catchCause` to a logged warning, so a publish failure can never abort core question settlement. - Settle the Deferred FIRST, then mirror, in `reply()` and `reject()`. - `ask()` uses `mirror()` too, so a publish failure can't abort it before the cleanup finalizer is registered. - Define the Bus mirror schemas from the structured Effect schemas via the `zod` adapter (`zod(Request)` / `zod(Replied)` / `zod(Rejected)`) so the generated /event OpenAPI payloads match the SDK types (no `any`/loose shapes). - Log (not silently swallow) failures in the instance-dispose cleanup. - Document the intentional GlobalBus double-emit (verified harmless in-repo). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent cd96fea commit 0b5ecd1

1 file changed

Lines changed: 52 additions & 42 deletions

File tree

packages/opencode/src/question/index.ts

Lines changed: 52 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { makeRuntime } from "@/effect/run-service"
1010
import { registerDisposer } from "@/effect/instance-registry"
1111
import { Bus } from "@/bus"
1212
import { BusEvent } from "@/bus/bus-event"
13+
import { zod } from "@/util/effect-zod"
1314
import z from "zod"
1415
// altimate_change end
1516

@@ -105,29 +106,31 @@ export const Event = {
105106
// never reached the webview → `pendingQuestions` stayed empty → the mcp-add
106107
// question card had no request id to reply with → "submit does nothing".
107108
// Publish these via `Bus.publish` too so they reach /event like every other
108-
// webview-visible event. (Schemas are loose — Bus.publish does not re-validate;
109-
// they exist for the `type` string and typing.)
110-
const BusAsked = BusEvent.define(
111-
"question.asked",
112-
z.object({
113-
id: QuestionID.zod,
114-
sessionID: SessionID.zod,
115-
questions: z.array(z.any()),
116-
tool: z.object({ messageID: MessageID.zod, callID: z.string() }).optional(),
117-
}),
118-
)
119-
const BusReplied = BusEvent.define(
120-
"question.replied",
121-
z.object({
122-
sessionID: SessionID.zod,
123-
requestID: QuestionID.zod,
124-
answers: z.array(z.array(z.string())),
125-
}),
126-
)
127-
const BusRejected = BusEvent.define(
128-
"question.rejected",
129-
z.object({ sessionID: SessionID.zod, requestID: QuestionID.zod }),
130-
)
109+
// webview-visible event.
110+
//
111+
// Reuse the structured Effect schemas (via the `zod` adapter) so the generated
112+
// `/event` OpenAPI payloads match the SDK's typed shapes rather than `any`.
113+
//
114+
// Note: `EventV2Bridge.listen` already forwards these EventV2 events to GlobalBus
115+
// and `Bus.publish` re-emits to GlobalBus too, so `/global/event` consumers see
116+
// each question event twice (different top-level ids). This is intentional and
117+
// verified harmless in-repo — TUI `sync.tsx` reconciles by request id,
118+
// `notifications.ts` dedupes by id, and the trace consumer ignores question
119+
// events. External subscribers that don't dedupe are the only residual concern.
120+
const BusAsked = BusEvent.define("question.asked", zod(Request))
121+
const BusReplied = BusEvent.define("question.replied", zod(Replied))
122+
const BusRejected = BusEvent.define("question.rejected", zod(Rejected))
123+
124+
// Best-effort Bus mirror. A fire-and-forget /event notification must NEVER be
125+
// able to abort core question settlement: `Effect.promise` turns a promise
126+
// rejection into an unrecoverable fiber defect, and on the Deferred critical
127+
// path that would skip `Deferred.succeed`/`Deferred.fail` and re-hang the tool
128+
// on "Thinking…" — the exact failure this PR fixes. Recover any cause to a
129+
// logged warning so publication can never block settlement.
130+
const mirror = <D extends BusEvent.Definition>(def: D, properties: z.output<D["properties"]>) =>
131+
Effect.promise(() => Bus.publish(def, properties)).pipe(
132+
Effect.catchCause((cause) => Effect.logWarning("question bus mirror failed", { type: def.type, cause })),
133+
)
131134
// altimate_change end
132135

133136
export class RejectedError extends Schema.TaggedErrorClass<RejectedError>()("QuestionRejectedError", {}) {
@@ -202,7 +205,11 @@ export const layer = Layer.effect(
202205
if (!map) return
203206
pendingByDir.delete(directory)
204207
for (const { deferred } of map.values()) {
205-
await Effect.runPromise(Deferred.fail(deferred, new RejectedError())).catch(() => {})
208+
await Effect.runPromise(
209+
Deferred.fail(deferred, new RejectedError()).pipe(
210+
Effect.catchCause((cause) => Effect.logWarning("question cleanup failed on dispose", { cause })),
211+
),
212+
)
206213
}
207214
})
208215
yield* Effect.addFinalizer(() => Effect.sync(off))
@@ -229,11 +236,16 @@ export const layer = Layer.effect(
229236
}
230237
pending.set(id, { info, deferred })
231238
yield* events.publish(Event.Asked, info)
232-
// altimate_change start — also publish on the Bus wildcard so the IDE webview
239+
// altimate_change start — also mirror on the Bus wildcard so the IDE webview
233240
// (subscribed to /event) receives question.asked and can answer the card.
234-
yield* Effect.promise(() =>
235-
Bus.publish(BusAsked, { id, sessionID: input.sessionID, questions: [...input.questions], tool: input.tool }),
236-
)
241+
// Best-effort: a publish failure must not abort ask() before it registers
242+
// the cleanup finalizer below (see `mirror`).
243+
yield* mirror(BusAsked, {
244+
id,
245+
sessionID: input.sessionID,
246+
questions: [...input.questions],
247+
tool: input.tool,
248+
})
237249
// altimate_change end
238250

239251
return yield* Effect.ensuring(
@@ -263,16 +275,15 @@ export const layer = Layer.effect(
263275
requestID: existing.info.id,
264276
answers: input.answers.map((a) => [...a]),
265277
})
266-
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
267-
yield* Effect.promise(() =>
268-
Bus.publish(BusReplied, {
269-
sessionID: existing.info.sessionID,
270-
requestID: existing.info.id,
271-
answers: input.answers.map((a) => [...a]),
272-
}),
273-
)
274-
// altimate_change end
275278
yield* Deferred.succeed(existing.deferred, input.answers)
279+
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients,
280+
// AFTER settling the Deferred and best-effort so a publish failure can't re-hang ask().
281+
yield* mirror(BusReplied, {
282+
sessionID: existing.info.sessionID,
283+
requestID: existing.info.id,
284+
answers: input.answers.map((a) => [...a]),
285+
})
286+
// altimate_change end
276287
})
277288

278289
const reject = Effect.fn("Question.reject")(function* (requestID: QuestionID) {
@@ -290,12 +301,11 @@ export const layer = Layer.effect(
290301
sessionID: existing.info.sessionID,
291302
requestID: existing.info.id,
292303
})
293-
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients.
294-
yield* Effect.promise(() =>
295-
Bus.publish(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id }),
296-
)
297-
// altimate_change end
298304
yield* Deferred.fail(existing.deferred, new RejectedError())
305+
// altimate_change start — mirror on the Bus wildcard for /event (webview) clients,
306+
// AFTER settling the Deferred and best-effort so a publish failure can't strand it.
307+
yield* mirror(BusRejected, { sessionID: existing.info.sessionID, requestID: existing.info.id })
308+
// altimate_change end
299309
})
300310

301311
const list = Effect.fn("Question.list")(function* () {

0 commit comments

Comments
 (0)