fix(plugin): synchronous shutdown hook for OTel span drain (revert PR #74)#75
Merged
Conversation
Revert PR #74's bus-await mechanism (v0.1.7) and replace with a synchronous shutdown hook invoked from src/index.ts's top-level finally before forceFlush. PR #74 was a no-op in headless 'bcode run' mode because the plugin's bus subscriber fiber gets interrupted by Effect scope teardown before it can process session.idle / server.instance.disposed events — confirmed by A/B trace shape comparison between v0.1.6 and v0.1.7 (identical, turn parent span missing in both). The new path is a direct function call from inside the running finally, so the event loop is alive, no scope race, no bus dependency.
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:258">
P1: Guard the dynamic plugin import in `finally`; an import failure can abort shutdown cleanup and skip `forceFlush`/`process.exit()`.</violation>
</file>
<file name="packages/opencode/src/plugin/index.ts">
<violation number="1" location="packages/opencode/src/plugin/index.ts:258">
P1: A throwing plugin event handler can now terminate the bus subscription fiber because per-hook error isolation was removed.</violation>
<violation number="2" location="packages/opencode/src/plugin/index.ts:271">
P2: Shutdown hooks are added to a global Set but never deregistered on instance disposal, which can retain stale closures and invoke disposed-instance hooks at exit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Fix all with cubic | Re-trigger cubic
Three fixes: 1. (src/index.ts) Wrap the dynamic 'await import(./plugin)' in the top-level finally with try/catch so a module-load failure cannot strand the process before forceFlush + process.exit(). 2. (plugin/index.ts) Re-add per-hook error isolation on the bus event dispatch loop. Reverting PR #74's await also accidentally removed this. Catches sync throws and observes async rejections via .catch(log.error) so one bad plugin can't terminate the subscription fiber for the rest of the process. 3. (plugin/index.ts) Deregister this layer's shutdown hooks from the module-level Set via Effect.addFinalizer so multi-instance TUI mode doesn't accumulate stale closures across project reopens.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace PR #74's bus-based
await session.idle / server.instance.disposedmechanism (shipped in v0.1.7) with a synchronous shutdown hook invoked directly fromsrc/index.ts's top-levelfinally. PR #74 turned out to be a no-op in headlessbcode runmode — the only mode V4 cloud uses — because the plugin's bus subscriber fiber gets interrupted by Effect scope teardown before it can process those events.A/B that triggered the rewrite
Health-check on V4 staging traces in BrowserCode-CLOUD Laminar, identical structure pre- and post-v0.1.7:
a4d10b79(7.83s)00...7029...)034b1b13(532s, 52 turns)00...adbf...)241ebf73(10.9s)00...095f...)PR #74 did not change the trace shape in any measurable way. The leaf LLM spans were always landing thanks to PR #50's existing
forceFlush(3000ms)race. What's been missing all along is the bcode-laminar "turn" parent span thatchat.messagecreates and that's supposed to be ended insession.idle/session.deleted/server.instance.disposed.Why the bus path is unreliable in headless mode
bcode runis non-interactive — callsclient.session.prompt()once, exits.session.idleis published bysession/processor.ts:758when the prompt completes. But after the prompt returns torun.ts, the top-level Effect resolves and scopes start closing.subscribeAllfiber (forked viaEffect.forkScoped) is interrupted at scope close. The fiber may be interrupted before it dequeuessession.idle.server.instance.disposedis published by the Bus's ownInstanceStatefinalizer atbus/index.ts:60-64— and thenPubSub.shutdown(wildcard)is called immediately in the sameEffect.gen. The plugin's subscriber may or may not be alive at that point, and even if it is, the in-flightawait sdk.shutdown()may be cut short by its own fiber being interrupted.session.deletedis never published inbcode runmode at all — the headless command doesn't delete the session.Whatever PR #74's
awaitwould have helped with, the events themselves don't reach the handler reliably. Confirmed empirically: identical trace shape v0.1.6 vs v0.1.7.The fix
Three small surgical changes plus one tiny plugin-side implementation:
1. Add
shutdown?: () => voidto the plugin SDK (packages/plugin/src/index.ts)Additive, no consumer breakage.
2. Expose a module-level shutdown-hook registry (
packages/opencode/src/plugin/index.ts)After plugins finish loading, the layer registers each plugin's optional
shutdowninto the set:Also reverts PR #74's
Effect.promise(async () => { ... await ... })dispatch loop back to upstream'sEffect.sync+void hook["event"]?.(...)— same surface asanomalyco/opencodeagain.3. Invoke the hooks from the top-level finally (
packages/opencode/src/index.ts)The
await import("./plugin")is dynamic to avoid an import cycle (this file already imports./pluginindirectly via commands; the dynamic form makes the dependency explicit at the use site).4. Implement
shutdowninbcode-laminar(packages/bcode-laminar/src/plugin.ts)The existing
session.idle/session.deleted/server.instance.disposedevent handlers stay in place as defense in depth — they're no-ops once the shutdown hook has cleared the map, but they remain useful in long-running TUI mode where the events do reliably fire.Why this works where PR #74 didn't
shutdown()→span.end()on every open turn spanfinallyblock, event loop still aliveprovider.forceFlush()with 3 s timeoutfinally, async-awaited, batch processor drains the just-ended turn spansprocess.exit()No race with Effect scope closure, no bus pubsub, no fiber lifecycle, no reliance on
session.idle/session.deleted/server.instance.disposedfiring. Just a direct function call from inside the finally block.Verification plan
v0.1.8-rc1pre-release tag frommain.v4-worker/Dockerfileto--version 0.1.8-rc1(separate cloud PR, staging-only).received(not inferred-from-children).total_costandtotal_tokenson the trace aggregate are non-zero (Laminar rolls up from received parents).v0.1.8proper; if not, iterate.Yellow-zone accounting
Net delta vs PR #74 is roughly even — we trade the unreliable bus-await yellow edit for a load-bearing sync-shutdown yellow edit. The new code paths are smaller and simpler. Documented in
memory/browsercode/EXCEPTIONS.mdunder "Phase F (cont.) — synchronous plugin shutdown hook".Upstream-able
Yes — this is a generic capability every OTel-based opencode plugin needs. After we've validated it in V4 staging, this is a strong candidate to upstream to
anomalyco/opencode. TheHooks.shutdownfield is additive; thepluginShutdownHooksregistry is the kind of escape hatch upstream is likely to accept once we describe the race it solves.Files
bun run typecheckclean (all 7 packages green).Summary by cubic
Adds a synchronous plugin shutdown hook to reliably end and export OTel “turn” spans in headless
bcode run. Replaces the bus-await path from #74 and hardens plugin dispatch and shutdown safety.shutdown?: () => voidtoHooksinpackages/plugin, and a module-levelpluginShutdownHooksregistry inpackages/opencode/src/plugin/index.ts.packages/opencode/src/index.tstop-levelfinally, then run tracerforceFlush()before exit; wrapped the dynamicawait import("./plugin")in try/catch.shutdowninpackages/bcode-laminar/src/plugin.tstospan.end()any open turn spans.Effect.addFinalizerto avoid stale closures in multi-instance TUI mode.Written for commit 4951144. Summary will update on new commits. Review in cubic