Fix node tracer shutdown exceeds shutdownTimeout - #7112
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: 5172adf The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The PR title promises a fix, but the current diff only adds the failing reproduction test. The implementation change in packages/opentelemetry/src/NodeSdk.ts is still required.
Reviewed changes
- Added one regression test in
packages/opentelemetry/test/OtelTracer.test.tsthat verifiesNodeSdk.layerTracerProviderrelease completes withinshutdownTimeouteven when the underlyingSpanProcessor.shutdownPromise does not resolve.
⚠️ Missing implementation fix
The test correctly fails against main and captures the bug. The fix should move shutdown inside the timed Promise so the timeout actually bounds the release.
In NodeSdk.ts, the current release is:
Effect.promise(() => provider.forceFlush()).pipe(
Effect.ensuring(Effect.promise(() => provider.shutdown())),
...
Effect.timeoutOption(config?.shutdownTimeout ?? 3000)
)Because timeoutOption wraps the ensuring chain, interruption still waits for provider.shutdown() to settle, so a stuck shutdown exceeds shutdownTimeout.
The sibling OtelLogger.ts already uses the pattern that avoids this:
Effect.promise(() => provider.forceFlush().finally(() => provider.shutdown())).pipe(
...
Effect.timeoutOption(config?.shutdownTimeout ?? 3000)
)Adopting the same .finally()-based Promise for the tracer provider release should make the new test pass.
Technical details
# NodeSdk tracer release timeout bug
## Affected sites
- packages/opentelemetry/src/NodeSdk.ts:77-82 — release combines `forceFlush` and `shutdown` with `ensuring`, placing `shutdown` outside the reach of `timeoutOption`.
## Required outcome
- `NodeSdk.layerTracerProvider` release must complete within `shutdownTimeout` even if `provider.shutdown()` never resolves.
- `provider.shutdown()` should still be attempted after `provider.forceFlush()`.
## Suggested approach
Mirror `packages/opentelemetry/src/OtelLogger.ts:187-190`: wrap a single Promise chain `provider.forceFlush().finally(() => provider.shutdown())` with `Effect.ignore`, `Effect.interruptible`, and `Effect.timeoutOption`.ℹ️ Same shape exists in WebSdk
WebSdk.layerTracerProvider uses the same Effect.ensuring construction without any shutdownTimeout config. That is pre-existing and out of scope here, but worth noting if the fix is intended to cover both platforms.
ℹ️ Nitpicks
- Add a changeset under
.changeset/once the implementation fix lands; runtime behavior changes require one. - Consider strengthening the test assertion at
packages/opentelemetry/test/OtelTracer.test.ts:60to also verify the release succeeded, e.g.assert.isTrue(Exit.isSuccess(completed)), so a failure exit cannot accidentally satisfy the test.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
The new commits address all three points from the prior review.
Reviewed changes
- Implemented the tracer provider release fix in
packages/opentelemetry/src/NodeSdk.ts: replaced theEffect.ensuringshutdown pattern with a singleEffect.promise(() => provider.forceFlush().finally(() => provider.shutdown()))chain, keeping it insideEffect.timeoutOptionso the timeout bounds a stuck shutdown. - Strengthened the regression test in
packages/opentelemetry/test/OtelTracer.test.ts: addedassert.isTrue(Exit.isSuccess(completed))so the test fails if release exits with an error rather than succeeding. - Added the patch changeset in
.changeset/eff-547-node-tracer-timeout.mdfor@effect/opentelemetry.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
Important
The implementation fix is correct and consistent with the logger provider, but commit 5172adf removed the only regression test for this fix, leaving the bounded-shutdown behavior with no automated coverage.
Reviewed changes
Since the prior pullfrog review (approved at e329306), one commit landed:
- Removed the tracer shutdown regression test in
packages/opentelemetry/test/OtelTracer.test.ts— deleted the "bounds tracer provider release by shutdownTimeout" case, which was the sole automated verification for this fix.
⚠️ Fix ships without regression coverage
The entire point of this PR is that NodeSdk.layerTracerProvider release now completes within shutdownTimeout even when a span processor's shutdown() Promise never resolves. Removing the test that proved that means nothing will guard the behavior — a future refactor back to the Effect.ensuring construction (or a dropped .finally()) would silently reintroduce the original bug. If the test was dropped because it proved flaky under the test clock, a more reliable variant or a short note recording why it was removed would preserve the intent; the fix is small and targeted enough that dedicated verification is worth retaining.
The change in NodeSdk.ts:77 itself (forceFlush().finally(() => shutdown()) inside timeoutOption) matches OtelLogger.ts:187 exactly, and the changeset accurately describes the runtime behavior change.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
2 participants
Summary
shutdownTimeout, including a slow or never-resolvingshutdown().forceFlush()by using the same timed Promise-chain pattern as the logger provider.@effect/opentelemetry.Validation
pnpm test --run packages/opentelemetry/testpnpm lintpnpm checkAudit provenance
b206fa5d7655c1634c9993410a9203f6616a5ca2relsem-node-tracer-shutdown-timeoutCloses EFF-547