Skip to content

fix(plugin): drop scope-close deregistration of shutdown hooks#76

Merged
Alezander9 merged 1 commit into
mainfrom
alex/v4-shutdown-hook-no-deregister
May 17, 2026
Merged

fix(plugin): drop scope-close deregistration of shutdown hooks#76
Alezander9 merged 1 commit into
mainfrom
alex/v4-shutdown-hook-no-deregister

Conversation

@Alezander9
Copy link
Copy Markdown
Member

@Alezander9 Alezander9 commented May 17, 2026

What

Drop the Effect.addFinalizer that deregisters plugin shutdown hooks on InstanceState scope close. Fix 3 from the cubic review on PR #75 was a lifecycle regression.

Why

Verified on staging V4 after PR #75 shipped as v0.1.8-rc1: trace 0ee0b31c-4970-c840-be54-d7bf5da53b37 from a real cloud bcode run still has the exact same problem PR #75 was supposed to fix — top_span_landed=False, total_cost=0, all session.llm children point to one orphan parent (the bcode-laminar "turn" span) that never landed.

The synchronous shutdown hook from PR #75 was never being invoked. Root cause is the lifecycle, not the hook itself:

yargs.handler {                              // src/index.ts try
  effectCmd.handler {
    try {
      AppRuntime.runPromise(run command)     // bus events, plugin events, etc.
    } finally {
      AppRuntime.runPromise(store.dispose(ctx))   //   ← plugin layer scope closes HERE
                                                  //     my Effect.addFinalizer runs HERE
                                                  //     pluginShutdownHooks.delete(hook) HERE
    }
  }
}                                            // src/index.ts top-level finally
finally {
  for (const hook of pluginShutdownHooks) hook()   //   ← Set is EMPTY → no-op
  forceFlush()                                      //   ← nothing queued, exports nothing
  process.exit()
}

So the turn span never gets .end()'d, never gets queued for export, and the top-level forceFlush(3000ms) drains nothing useful.

pluginShutdownHooks is intentionally module-level — that's the entire point: the Set must outlive the Effect scopes so the host can reach it from outside the runtime. A scope-close finalizer defeats that.

Fix

Just drop the Effect.addFinalizer. Multi-instance TUI bloat (the cubic reviewer's concern) is the lesser evil: rare in practice, closures are tiny, and the cost is bounded by the number of instances opened over a TUI session lifetime. Comment block updated so the next reviewer doesn't re-add the dereregistration.

Diff

-        const registered: Array<() => void> = []
         for (const hook of hooks) {
-          if (!hook.shutdown) continue
-          pluginShutdownHooks.add(hook.shutdown)
-          registered.push(hook.shutdown)
+          if (hook.shutdown) pluginShutdownHooks.add(hook.shutdown)
         }
-        yield* Effect.addFinalizer(() =>
-          Effect.sync(() => {
-            for (const fn of registered) pluginShutdownHooks.delete(fn)
-          }),
-        )

Verification plan

  1. Merge → tag v0.1.8-rc2 → bump cloud staging Dockerfile to rc2.
  2. Submit a real cloud V4 task.
  3. Pull trace, expect top_span_landed=True for the bcode-laminar turn parent and total_cost > 0 on the trace aggregate.
  4. If green → v0.1.8 proper, then bump prod.

Refs


Summary by cubic

Stop deregistering plugin shutdown hooks on scope close so they survive InstanceState teardown and run at process exit. This fixes orphan spans and ensures telemetry flushes correctly.

  • Bug Fixes
    • Removed Effect.addFinalizer that deleted hooks on scope close.
    • Hooks stay in module-level pluginShutdownHooks until the top-level finally executes them.
    • Prevents unlanded parent spans and zero-cost traces seen in v0.1.8-rc1.
    • Added a comment explaining the lifecycle to avoid reintroducing deregistration.

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

Fix 3 from the cubic review on PR #75 was a regression. The plugin-layer scope (under InstanceState) closes BEFORE the top-level finally in src/index.ts runs, via store.dispose(ctx) in effect-cmd's own finally. A scope-close finalizer empties pluginShutdownHooks before the host can iterate it, defeating the entire feature.

Verified live: v0.1.8-rc1 in staging cloud V4 still produced orphan turn parents (trace 0ee0b31c-..., top_span_landed=False, total_cost=0, all session.llm children referencing one unlanded parent span).

Multi-instance TUI mode bloat from accumulated closures is acceptable -- rare in practice, closures are tiny, and the hooks Set is intentionally module-level so it outlives Effect scopes.
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.

No issues found across 1 file

Re-trigger cubic

@Alezander9 Alezander9 merged commit 351e645 into main May 17, 2026
3 checks passed
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