Skip to content

Commit cb35493

Browse files
authored
fix(bus): acquire PubSub subscription eagerly to close /event race (#27959)
1 parent 5bfd7fd commit cb35493

9 files changed

Lines changed: 659 additions & 43 deletions

File tree

packages/opencode/src/bus/index.ts

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,16 @@ export interface Interface {
3737
properties: BusProperties<D>,
3838
options?: { id?: string },
3939
) => Effect.Effect<void>
40-
readonly subscribe: <D extends BusEvent.Definition>(def: D) => Stream.Stream<Payload<D>>
41-
readonly subscribeAll: () => Stream.Stream<Payload>
40+
// subscribe / subscribeAll are eager: the underlying PubSub subscription is
41+
// acquired in the caller's Scope at `yield*` time. Any publish after the
42+
// yield is delivered, even if stream consumption starts later. The previous
43+
// Stream-returning shape acquired the subscription lazily on first pull,
44+
// opening a race window during which publishes were lost — see
45+
// test/bus/bus-effect.test.ts RACE tests.
46+
readonly subscribe: <D extends BusEvent.Definition>(
47+
def: D,
48+
) => Effect.Effect<Stream.Stream<Payload<D>>, never, Scope.Scope>
49+
readonly subscribeAll: () => Effect.Effect<Stream.Stream<Payload>, never, Scope.Scope>
4250
readonly subscribeCallback: <D extends BusEvent.Definition>(
4351
def: D,
4452
callback: (event: Payload<D>) => unknown,
@@ -109,26 +117,26 @@ export const layer = Layer.effect(
109117
})
110118
}
111119

112-
function subscribe<D extends BusEvent.Definition>(def: D): Stream.Stream<Payload<D>> {
113-
log.info("subscribing", { type: def.type })
114-
return Stream.unwrap(
115-
Effect.gen(function* () {
116-
const s = yield* InstanceState.get(state)
117-
const ps = yield* getOrCreate(s, def)
118-
return Stream.fromPubSub(ps)
119-
}),
120-
).pipe(Stream.ensuring(Effect.sync(() => log.info("unsubscribing", { type: def.type }))))
121-
}
120+
const subscribe = <D extends BusEvent.Definition>(
121+
def: D,
122+
): Effect.Effect<Stream.Stream<Payload<D>>, never, Scope.Scope> =>
123+
Effect.gen(function* () {
124+
log.info("subscribing", { type: def.type })
125+
const s = yield* InstanceState.get(state)
126+
const ps = yield* getOrCreate(s, def)
127+
const subscription = yield* PubSub.subscribe(ps)
128+
yield* Effect.addFinalizer(() => Effect.sync(() => log.info("unsubscribing", { type: def.type })))
129+
return Stream.fromSubscription(subscription)
130+
})
122131

123-
function subscribeAll(): Stream.Stream<Payload> {
124-
log.info("subscribing", { type: "*" })
125-
return Stream.unwrap(
126-
Effect.gen(function* () {
127-
const s = yield* InstanceState.get(state)
128-
return Stream.fromPubSub(s.wildcard)
129-
}),
130-
).pipe(Stream.ensuring(Effect.sync(() => log.info("unsubscribing", { type: "*" }))))
131-
}
132+
const subscribeAll = (): Effect.Effect<Stream.Stream<Payload>, never, Scope.Scope> =>
133+
Effect.gen(function* () {
134+
log.info("subscribing", { type: "*" })
135+
const s = yield* InstanceState.get(state)
136+
const subscription = yield* PubSub.subscribe(s.wildcard)
137+
yield* Effect.addFinalizer(() => Effect.sync(() => log.info("unsubscribing", { type: "*" })))
138+
return Stream.fromSubscription(subscription)
139+
})
132140

133141
function on<T>(pubsub: PubSub.PubSub<T>, type: string, callback: (event: T) => unknown) {
134142
return Effect.gen(function* () {

packages/opencode/src/plugin/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ export const layer = Layer.effect(
243243
}
244244

245245
// Subscribe to bus events, fiber interrupted when scope closes
246-
yield* bus.subscribeAll().pipe(
246+
yield* (yield* bus.subscribeAll()).pipe(
247247
Stream.runForEach((input) =>
248248
Effect.sync(() => {
249249
for (const hook of hooks) {

packages/opencode/src/project/project.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -425,7 +425,7 @@ export const layer: Layer.Layer<
425425

426426
const initState = yield* InstanceState.make(
427427
Effect.fn("Project.initState")(function* (ctx) {
428-
yield* bus.subscribe(Command.Event.Executed).pipe(
428+
yield* (yield* bus.subscribe(Command.Event.Executed)).pipe(
429429
Stream.runForEach((payload) =>
430430
payload.properties.name === Command.Default.INIT ? setInitialized(ctx.project.id) : Effect.void,
431431
),

packages/opencode/src/project/vcs.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,7 @@ export const layer: Layer.Layer<Service, never, Git.Service | Bus.Service> = Lay
298298
const value = { current, root }
299299
log.info("initialized", { branch: value.current, default_branch: value.root?.name })
300300

301-
yield* bus.subscribe(FileWatcher.Event.Updated).pipe(
301+
yield* (yield* bus.subscribe(FileWatcher.Event.Updated)).pipe(
302302
Stream.filter((evt) => evt.properties.file.endsWith("HEAD")),
303303
Stream.runForEach((_evt) =>
304304
Effect.gen(function* () {

packages/opencode/src/server/routes/instance/httpapi/handlers/event.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,11 @@ function eventData(data: unknown): Sse.Event {
2020

2121
function eventResponse(bus: Bus.Interface) {
2222
return Effect.gen(function* () {
23-
const context = yield* Effect.context()
24-
25-
const events = bus.subscribeAll().pipe(
26-
Stream.provideContext(context),
23+
// Subscribe eagerly: the bus subscription is acquired in the request scope
24+
// at this yield, so any publish from now on is queued for the body-pump
25+
// fiber to drain — closing the race where Stream.concat(server.connected,
26+
// lazy-subscribe) used to drop publishes in the prefix-consume window.
27+
const events = (yield* bus.subscribeAll()).pipe(
2728
Stream.takeUntil((event) => event.type === Bus.InstanceDisposed.type),
2829
)
2930
const heartbeat = Stream.tick("10 seconds").pipe(

packages/opencode/src/share/share-next.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,16 +168,20 @@ export const layer = Layer.effect(
168168
fn: (evt: { properties: any }) => Effect.Effect<void, unknown>,
169169
) =>
170170
bus.subscribe(def as never).pipe(
171-
Stream.runForEach((evt) =>
172-
fn(evt).pipe(
173-
Effect.catchCause((cause) =>
174-
Effect.sync(() => {
175-
log.error("share subscriber failed", { type: def.type, cause })
176-
}),
171+
Effect.flatMap((stream) =>
172+
stream.pipe(
173+
Stream.runForEach((evt) =>
174+
fn(evt).pipe(
175+
Effect.catchCause((cause) =>
176+
Effect.sync(() => {
177+
log.error("share subscriber failed", { type: def.type, cause })
178+
}),
179+
),
180+
),
177181
),
182+
Effect.forkScoped,
178183
),
179184
),
180-
Effect.forkScoped,
181185
)
182186

183187
yield* watch(Session.Event.Updated, (evt) =>

packages/opencode/test/bus/bus-effect.test.ts

Lines changed: 92 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ describe("Bus (Effect-native)", () => {
4444
const done = yield* Deferred.make<void>()
4545
const ready = yield* Latch.make()
4646

47-
yield* Stream.runForEach(bus.subscribe(TestEvent.Ping), (evt) =>
47+
yield* Stream.runForEach(yield* bus.subscribe(TestEvent.Ping), (evt) =>
4848
Effect.gen(function* () {
4949
if (evt.properties.value < 0) {
5050
yield* ready.open
@@ -71,7 +71,7 @@ describe("Bus (Effect-native)", () => {
7171
const done = yield* Deferred.make<void>()
7272
const ready = yield* Latch.make()
7373

74-
yield* Stream.runForEach(bus.subscribe(TestEvent.Ping), (evt) =>
74+
yield* Stream.runForEach(yield* bus.subscribe(TestEvent.Ping), (evt) =>
7575
Effect.gen(function* () {
7676
if (evt.properties.value < 0) {
7777
yield* ready.open
@@ -98,7 +98,7 @@ describe("Bus (Effect-native)", () => {
9898
const done = yield* Deferred.make<void>()
9999
const ready = yield* Latch.make()
100100

101-
yield* Stream.runForEach(bus.subscribeAll(), (evt) =>
101+
yield* Stream.runForEach(yield* bus.subscribeAll(), (evt) =>
102102
Effect.gen(function* () {
103103
if (evt.type === TestEvent.Warmup.type) {
104104
yield* ready.open
@@ -129,7 +129,7 @@ describe("Bus (Effect-native)", () => {
129129
const readyA = yield* Latch.make()
130130
const readyB = yield* Latch.make()
131131

132-
yield* Stream.runForEach(bus.subscribe(TestEvent.Ping), (evt) =>
132+
yield* Stream.runForEach(yield* bus.subscribe(TestEvent.Ping), (evt) =>
133133
Effect.gen(function* () {
134134
if (evt.properties.value < 0) {
135135
yield* readyA.open
@@ -140,7 +140,7 @@ describe("Bus (Effect-native)", () => {
140140
}),
141141
).pipe(Effect.forkScoped)
142142

143-
yield* Stream.runForEach(bus.subscribe(TestEvent.Ping), (evt) =>
143+
yield* Stream.runForEach(yield* bus.subscribe(TestEvent.Ping), (evt) =>
144144
Effect.gen(function* () {
145145
if (evt.properties.value < 0) {
146146
yield* readyB.open
@@ -162,6 +162,92 @@ describe("Bus (Effect-native)", () => {
162162
}),
163163
)
164164

165+
// RACE 1: eager subscription means publishing immediately after yield*
166+
// bus.subscribe is delivered. Regression for the old lazy `Stream.unwrap`
167+
// shape where PubSub.subscribe ran on first pull and missed any publish
168+
// in the hand-off window.
169+
it.instance("eager subscribe: publish after yield* is delivered without consumer-activation race", () =>
170+
Effect.gen(function* () {
171+
const bus = yield* Bus.Service
172+
const stream = yield* bus.subscribe(TestEvent.Ping)
173+
174+
// Hand-off window: subscription is alive (we yielded). Publish goes
175+
// straight into the subscription queue, even with no consumer running.
176+
yield* bus.publish(TestEvent.Ping, { value: 99 })
177+
178+
const collected = yield* stream.pipe(
179+
Stream.take(1),
180+
Stream.runCollect,
181+
Effect.timeout("400 millis"),
182+
Effect.option,
183+
)
184+
185+
expect(collected._tag).toBe("Some")
186+
if (collected._tag === "Some") {
187+
const arr = Array.from(collected.value)
188+
expect(arr[0].properties.value).toBe(99)
189+
}
190+
}),
191+
)
192+
193+
// RACE 2: same property for subscribeAll.
194+
it.instance("eager subscribeAll: publish after yield* is delivered", () =>
195+
Effect.gen(function* () {
196+
const bus = yield* Bus.Service
197+
const stream = yield* bus.subscribeAll()
198+
199+
yield* bus.publish(TestEvent.Ping, { value: 42 })
200+
201+
const collected = yield* stream.pipe(
202+
Stream.take(1),
203+
Stream.runCollect,
204+
Effect.timeout("400 millis"),
205+
Effect.option,
206+
)
207+
208+
expect(collected._tag).toBe("Some")
209+
if (collected._tag === "Some") {
210+
const arr = Array.from(collected.value)
211+
expect(arr[0].type).toBe(TestEvent.Ping.type)
212+
}
213+
}),
214+
)
215+
216+
// RACE 3: the /event-handler shape exactly. With eager subscription, the
217+
// bus subscription is alive before Stream.concat ever starts. Publishes
218+
// during the prefix consumption window are queued and delivered.
219+
it.instance("eager subscribe: Stream.concat(initial, subscribe) delivers publish during prefix", () =>
220+
Effect.gen(function* () {
221+
const bus = yield* Bus.Service
222+
const sawInitial = yield* Deferred.make<void>()
223+
const sawPublish = yield* Deferred.make<number>()
224+
225+
type Frame = { marker?: "initial"; value?: number }
226+
const subscriptionStream = yield* bus.subscribe(TestEvent.Ping)
227+
const handlerStream: Stream.Stream<Frame> = Stream.make({ marker: "initial" } as Frame).pipe(
228+
Stream.concat(subscriptionStream.pipe(Stream.map((evt): Frame => ({ value: evt.properties.value })))),
229+
)
230+
231+
yield* Stream.runForEach(handlerStream, (frame) =>
232+
Effect.gen(function* () {
233+
if (frame.marker === "initial") {
234+
Deferred.doneUnsafe(sawInitial, Effect.void)
235+
return
236+
}
237+
if (frame.value !== undefined) Deferred.doneUnsafe(sawPublish, Effect.succeed(frame.value))
238+
}),
239+
).pipe(Effect.forkScoped)
240+
241+
yield* Deferred.await(sawInitial).pipe(Effect.timeout("1 second"))
242+
243+
yield* bus.publish(TestEvent.Ping, { value: 7 })
244+
245+
const got = yield* Deferred.await(sawPublish).pipe(Effect.timeout("1 second"), Effect.option)
246+
expect(got._tag).toBe("Some")
247+
if (got._tag === "Some") expect(got.value).toBe(7)
248+
}),
249+
)
250+
165251
it.live("subscribeAll stream sees InstanceDisposed on disposal", () =>
166252
Effect.gen(function* () {
167253
const dir = yield* tmpdirScoped()
@@ -174,7 +260,7 @@ describe("Bus (Effect-native)", () => {
174260
yield* Effect.gen(function* () {
175261
const bus = yield* Bus.Service
176262

177-
yield* Stream.runForEach(bus.subscribeAll(), (evt) =>
263+
yield* Stream.runForEach(yield* bus.subscribeAll(), (evt) =>
178264
Effect.gen(function* () {
179265
if (evt.type === TestEvent.Warmup.type) {
180266
yield* ready.open

0 commit comments

Comments
 (0)