Skip to content

fix(plugin): await session.idle + server.instance.disposed handlers#74

Merged
Alezander9 merged 3 commits into
mainfrom
alex/await-disposal-plugin-events
May 16, 2026
Merged

fix(plugin): await session.idle + server.instance.disposed handlers#74
Alezander9 merged 3 commits into
mainfrom
alex/await-disposal-plugin-events

Conversation

@Alezander9
Copy link
Copy Markdown
Member

@Alezander9 Alezander9 commented May 16, 2026

Summary

Await plugin event handlers for session.idle and server.instance.disposed so lifecycle plugins (e.g. bcode-laminar) can finish draining before process.exit().

Why

packages/opencode/src/index.ts:252-266 (PR #50) added a top-level provider.forceFlush() race before process.exit() to recover the final agent LLM span. That helped, but forceFlush() only exports ended spans. The deeper bug — already called out in PR #50's own comment — is that bus dispatch at packages/opencode/src/plugin/index.ts:249 is void hook["event"]?.(...), so the bcode-laminar plugin's session.idle and server.instance.disposed handlers (which span.end() the open turn span and await processor.forceFlush() / await sdk.shutdown()) are fire-and-forget. The turn span is therefore still open when forceFlush() runs, so it never reaches the wire before process.exit() kills the in-flight export.

Verified empirically in V4 cloud staging: long-task runs land every turn span except the last one in BrowserCode-CLOUD Laminar. Exact symptom of the existing comment.

Change

-        // Subscribe to bus events, fiber interrupted when scope closes
+        // Subscribe to bus events, fiber interrupted when scope closes.
+        // session.idle and server.instance.disposed are plugins' only chance to
+        // drain async work (e.g. OTel span exporters) before src/index.ts's
+        // top-level finally runs forceFlush and calls process.exit() — await
+        // those handlers; keep the rest fire-and-forget for throughput.
         yield* bus.subscribeAll().pipe(
           Stream.runForEach((input) =>
-            Effect.sync(() => {
+            Effect.promise(async () => {
+              const awaitHook = input.type === "server.instance.disposed" || input.type === "session.idle"
               for (const hook of hooks) {
-                void hook["event"]?.({ event: input as any })
+                const ret = hook["event"]?.({ event: input as any })
+                if (awaitHook && ret) {
+                  try {
+                    await ret
+                  } catch (err) {
+                    log.error("plugin event hook failed", { error: err })
+                  }
+                }
               }
             }),
           ),
           Effect.forkScoped,
         )

Narrow scope by design:

  • Only the two disposal-class events block on plugin handlers. session.message, session.created, etc. keep their existing fire-and-forget semantics — no latency increase, no new deadlock surface, no ordering changes for the hot paths.
  • Handler errors are caught and logged via the same log.error pattern the config hook already uses, so one bad plugin can't stall the bus.
  • The PR fix(laminar): drain OTel processors before process.exit() to stop losing final agent spans #50 forceFlush race upstairs stays in place as a generic safety net for any future OTel plugin.

Verification plan

  1. Land this PR; cut bcode v0.1.7 from main.
  2. Bump cloud/backend/sandboxes/v4-worker/Dockerfile to --version 0.1.7; CP auto-deploys to AgentCore.
  3. Re-run V4 staging long-task smoke (~30s+ runtime, multiple turns).
  4. Confirm BrowserCode-CLOUD Laminar shows the complete trace tree including the final turn, with all AI-SDK sub-spans nested correctly.

Yellow-zone note

Documented in memory/browsercode/EXCEPTIONS.md under "Phase F (cont.) — await disposal/idle plugin events". This is a real upstream-grade fix (PR #50's comment already identifies the underlying bug). Upstreamable to anomalyco/opencode once we've validated it in production for a week.


Summary by cubic

Await plugin handlers for session.idle and server.instance.disposed so lifecycle plugins (e.g., bcode-laminar) can end the final turn span and flush OTel before process exit. Also log errors from fire‑and‑forget handlers to avoid unhandled rejections.

  • Bug Fixes
    • Await promises for session.idle and server.instance.disposed; all other events stay fire-and-forget.
    • Wrap the whole invocation in try/catch to catch sync throws and awaited rejections; keep the subscription fiber and other plugins running.
    • For fire-and-forget events, attach .catch(log.error) to handler promises to surface async failures instead of unhandledRejection.
    • Keep top-level provider.forceFlush() as a fallback.

Written for commit e008c31. Summary will update on new commits. Review in cubic

PR #50 added a top-level forceFlush race to recover the final span on exit, but forceFlush only exports ended spans. The plugin's session.idle and server.instance.disposed handlers (which end the open turn span and drain the SDK) were fire-and-forget via 'void hook[event]?.(...)', so the final turn span was still open when forceFlush ran. Narrow await: only those two disposal-class events. All other events stay fire-and-forget.
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 1 file

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/plugin/index.ts">

<violation number="1" location="packages/opencode/src/plugin/index.ts:257">
P2: Synchronous exceptions from `hook["event"]` are not caught, so a throwing plugin can fail the bus subscription fiber instead of being logged and isolated.</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

Comment thread packages/opencode/src/plugin/index.ts Outdated
Per PR #74 review: the previous patch only wrapped 'await ret' in try/catch, so a synchronous throw from hook[event] would escape the Effect.promise callback as a defect and kill the Stream.runForEach fiber — silently disabling every plugin's event handler for the rest of the process. Wrap the whole invocation.
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 1 file (changes from recent commits).

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/plugin/index.ts">

<violation number="1" location="packages/opencode/src/plugin/index.ts:259">
P2: Non-awaited plugin event Promises can reject without being caught, so async handler failures on non-disposal events are still unhandled and not logged.</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

Comment thread packages/opencode/src/plugin/index.ts Outdated
… them

Per PR #74 review: for non-disposal events the handler's promise is not awaited, so an async rejection becomes an unhandledRejection — Node logs it generically but our log.error never fires, hiding which plugin/event broke. Attach .catch(log.error) on the discard path. Sync throws and the awaited path still go through the outer try/catch.
@Alezander9 Alezander9 merged commit 41277ae into main May 16, 2026
3 checks passed
Alezander9 pushed a commit that referenced this pull request May 17, 2026
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.
Alezander9 added a commit that referenced this pull request May 17, 2026
fix(plugin): synchronous shutdown hook for OTel span drain (revert PR #74)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant