Repository navigation
fix(observability): flush OTel spans on exit for opencode run - #30089
astefanutti wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
needs:compliance
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
When will this be merged into dev and release please? |
0efbdb0 to
1f42eeb
Compare
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
|
We need that PR, please reopen it. |
|
Same here! It fixes the bug |
|
Please, reopen it. We need that functionality |
|
Please reopen! We need this urgently. |
|
Very crucial functionality, we need this PR. |
|
We need this PR asap, please reopen. |
|
will look sorry |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The shutdown belongs at a lifecycle boundary, but this import is the wrong dependency direction for that boundary. index.ts runs for every invocation (including --help/--version), while app-runtime.ts statically pulls the full V1 AppLayer; the existing effectCmd code deliberately imports AppRuntime only when a command actually needs it. This change makes the entire application runtime eager just so the top-level finally can dispose it.
Please preserve lazy runtime initialization and dispose only a runtime that was actually acquired. For this specific path, the natural ownership seam is effectCmd, which imports and runs AppRuntime; alternatively expose a lightweight lifecycle/disposer registry that index.ts can call without importing app-runtime.ts. A dynamic import directly in this finally is not sufficient because it would still load the full runtime on exit for commands that never used it.
The flush itself is correct once disposal is attached to that ownership seam.
0fcd7f0 to
038fcc3
Compare
|
@rekram1-node thanks, I've addressed the review and rebased it. |
038fcc3 to
7be1618
Compare
astefanutti
changed the title
github-actions
Bot
added
the
needs:compliance
The BatchSpanProcessor buffers spans and exports them asynchronously. process.exit() in index.ts kills the event loop before the buffer flushes, so `opencode run` never emits traces (#30087). The embedded server and AppRuntime share a single, memoMap-memoized Observability layer (one BatchSpanProcessor referenced by two scopes), so flushing requires releasing both: - effectCmd disposes AppRuntime at the seam that lazily acquires it, so index.ts never eagerly imports the full application graph just to tear it down (addresses the review on #30089). - run.ts disposes the embedded server after each local run mode, so the server stops holding the shared span processor open. Server.Default() now exposes the web handler's dispose(). Adds a subprocess test that runs against a stub OTLP collector with a mocked (fast) LLM, so the flush must come from disposal rather than the processor's periodic export timer. Because shutdown is now graceful, run also dedupes its terminal JSON error record: a rejected prompt surfaces the same failure both synchronously (session.prompt result.error) and via the session.error event drained by the event loop; the old abrupt exit masked the second write. Fixes #30087
7be1618 to
0e3a4df
Compare
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
can we have a review, please? |
|
This branch has been upgraded to V2. We're no longer taking PRs for V1. If you think this is still relevant, please port it over and open it for V2. — from 𝕺𝖕𝖊𝖓𝕮𝖔𝖉𝖊 |
Issue for this PR
Closes #30087
Type of change
What does this PR do?
opencode runnever emits OTel spans:process.exit()inindex.tskills the event loop before theBatchSpanProcessorflushes its buffer.The embedded server and
AppRuntimeshare a single,memoMap-memoizedObservabilitylayer — oneBatchSpanProcessorreferenced by two scopes — so a flush (NodeSdk.shutdown()→forceFlush()) only fires once both scopes are released. This PR disposes both at their ownership seams:effectCmddisposesAppRuntimein itsfinally. This is the seam that already lazily imports the runtime, soindex.tsnever eagerly pulls in the full application graph on every invocation (--help/--version/completion included). This addresses the review feedback on the originalindex.tsapproach.run.tsdisposes the embedded server after each local run mode, so the server stops holding the shared span processor open.Server.Default()now exposes the web handler'sdispose().This converges with the approach in the (now-closed) #39756.
How did you verify your code works?
Added
test/cli/run/run-process.test.ts→ "flushes server telemetry before exiting": spawnsopencode runagainst a stub OTLP collector with a mocked (fast) LLM, and asserts the collector receives/v1/traces. The mock keeps the run short enough that theBatchSpanProcessor's periodic export timer never fires, so the test only passes if disposal forces the flush.Screenshots / recordings
Not a UI change.
Checklist