Skip to content

refactor(otel): share span leak/drop lifecycle via glide_core helper - #6232

Draft
xShinnRyuu wants to merge 6 commits into
mainfrom
otel-span-lifecycle-shared-helper
Draft

refactor(otel): share span leak/drop lifecycle via glide_core helper#6232
xShinnRyuu wants to merge 6 commits into
mainfrom
otel-span-lifecycle-shared-helper

Conversation

@xShinnRyuu

@xShinnRyuu xShinnRyuu commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Unifies the OpenTelemetry span raw-pointer lifecycle that every native binding maintained as its own copy, moving it into two shared helpers on glide_core::GlideOpenTelemetry. This closes the gap raised in review of #6227: previously a leak fix or test on one binding's copy did not protect the others, and several copies (JNI, NAPI, PyO3) had no leak coverage at all.

Issue link

This Pull Request is linked to issue: [Java][Flaky Test] testSpan related memory tests
Relates to #6226 (follow-up to #6227)

Features / Behaviour Changes

No public or behavioural change. Public C-ABI, JNI, NAPI and PyO3 signatures are unchanged, and each binding keeps its own pointer guards and logging/error contract.

Implementation

Two new helpers on glide_core::GlideOpenTelemetry (in glide-core/telemetry/src/open_telemetry.rs):

  • leak_span(GlideSpan) -> u64: the Arc::into_raw create path.
  • drop_span_ptr(u64) -> Result<(), GlideOTELError>: validate (is_span_pointer_valid) plus panic-contained Arc::from_raw reclaim path. 0 is a no-op success.

All five bindings route their span create/drop sites through them:

Binding Crate / mechanism Sites
Go ffi/ (CGO) inherits via ffi/src/lib.rs
Python-sync ffi/ (CFFI) inherits via ffi/src/lib.rs
(FFI crate) ffi/src/lib.rs all seven create_*_otel_span tails (the two trace-context tails via the shared span_to_ffi_pointer helper); drop_otel_span
Java java/src/lib.rs (JNI) createLeakedOtelSpan, dropOtelSpan, two inline command/batch root-span drops
Node.js node/rust-client/src/lib.rs (NAPI) create_leaked_otel_span, create_otel_span_with_trace_context, drop_otel_span
Python-async python/glide-async/src/lib.rs (PyO3) create_otel_span, drop_otel_span

Binding-specific behaviour is preserved at each drop site: the JNI span_ptr <= 0 to FFIError guard, Node's BigInt negative/lossless/zero checks, and the PyO3 zero-pointer error log all remain (the shared helper treats 0 as Ok, so each binding keeps its own stricter guard where it had one). The JNI inline drops in executeCommandAsync and executeBatchAsync are if let Ok(span) = span_from_pointer(...) chains that call .end() then route reclamation through drop_span_ptr. When span_from_pointer rejects the pointer the whole chain is skipped, so .end() is not called and no reclamation is attempted; Arc::from_raw on an invalid pointer would be unsafe, so not reclaiming there is correct.

A minor log-text change: the three distinct ffi drop_otel_span invalid-pointer messages collapse into one Err line, but the specific reason is still emitted as a WARN by is_span_pointer_valid, so diagnostic detail is not lost.

Limitations

The new tests prove the shared helper itself does not leak. They do not drive the per-binding entry points or inline drop sites, so they don't prove every binding call site actually invokes the helper: that remains a per-binding concern. The catch_unwind panic-containment branch of drop_span_ptr is also not exercised by a test (a panic can't be triggered safely from a valid pointer). FFI-entry-point leak coverage is provided separately by the tests in #6227.

Testing

  • New glide_core tests: test_leak_and_drop_span_ptr_does_not_leak (Arc strong count returns to baseline across 1000 leak/drop cycles) and test_drop_span_ptr_null_and_invalid.
  • cargo test -p telemetrylib -- --test-threads=1: 18 passed. (Note: test_span_json_exporter is a pre-existing flake under parallel execution due to shared global OTel state; it fails on clean main too, tracked separately.)
  • cargo test --test test_otel_ffi_integration passes; glide-rs and node/rust-client build; python/glide-async passes cargo check (its cargo build linker step requires maturin/libpython and fails identically on clean main).
  • cargo fmt and cargo clippy clean on all touched crates (telemetry, ffi, java, node/rust-client, python/glide-async).

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Linters have been run (make *-lint targets) and Prettier has been run (make prettier-fix).
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.
  • Make sure to update the documentation in the valkey-glide-docs repository if necessary

@xShinnRyuu
xShinnRyuu requested a review from a team as a code owner June 12, 2026 01:34
@xShinnRyuu xShinnRyuu added the Rust ⚙️ Pull requests that update rust code label Jun 12, 2026

@valkey-review-bot valkey-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found one compile-time gap in the FFI test shim introduced by moving these helpers into GlideOpenTelemetry.

Comment thread ffi/src/lib.rs
@xShinnRyuu
xShinnRyuu force-pushed the otel-span-lifecycle-shared-helper branch from 7cf7774 to 0203cdd Compare June 12, 2026 02:57
@xShinnRyuu
xShinnRyuu force-pushed the otel-span-lifecycle-shared-helper branch from 0203cdd to c9047b8 Compare June 12, 2026 08:03
@xShinnRyuu
xShinnRyuu marked this pull request as draft June 12, 2026 17:48
Every native binding maintained its own copy of the OpenTelemetry span
raw-pointer lifecycle (Arc::into_raw on create, validate + Arc::from_raw
on drop), so a fix or test on one copy did not protect the others, and
several copies had no leak coverage at all.

Introduce two shared helpers on glide_core::GlideOpenTelemetry:
- leak_span(GlideSpan) -> u64: Arc::into_raw create path.
- drop_span_ptr(u64) -> Result<(), TraceError>: validate + panic-contained
  Arc::from_raw reclaim path (0 is a no-op success).

Route every binding's span create/drop sites through them:
- ffi/src/lib.rs (Go via CGO, Python-sync via CFFI): all create_*_otel_span
  tails and drop_otel_span.
- java/src/lib.rs (JNI): createLeakedOtelSpan, dropOtelSpan, and the two
  inline command/batch root-span drops (.end() preserved; only the manual
  Arc::from_raw is replaced).
- node/rust-client/src/lib.rs (NAPI): create_leaked_otel_span,
  create_otel_span_with_trace_context, drop_otel_span.
- python/glide-async/src/lib.rs (PyO3): create_otel_span, drop_otel_span.

Public C-ABI, JNI, NAPI and PyO3 signatures are unchanged; each binding
keeps its own pointer guards and logging/error contract by mapping the
helper's Result (e.g. Node's BigInt negative/lossless/zero checks and the
zero-pointer error logs are preserved).

Add glide_core tests for the shared helper: test_leak_and_drop_span_ptr_
does_not_leak (Arc strong count returns to baseline) and
test_drop_span_ptr_null_and_invalid. These prove the shared helper itself
does not leak; verifying that each binding call site invokes it remains a
per-binding concern.

Relates to #6226

Signed-off-by: Thomas Zhou <thomaszhou64@gmail.com>
@xShinnRyuu
xShinnRyuu force-pushed the otel-span-lifecycle-shared-helper branch from c9047b8 to 252af4a Compare June 12, 2026 20:01
Signed-off-by: Thomas Zhou <54688146+xShinnRyuu@users.noreply.github.com>
Signed-off-by: Thomas Zhou <54688146+xShinnRyuu@users.noreply.github.com>
The merge of main into this branch resurrected the pre-refactor
protobuf-driven executeBatchAsync/executeCommandAsync helper
(`execute_command_request_and_complete`) and left a duplicated
protobuf_bridge routing block inside `executeBatchAsync`. Both
reference the `protobuf_bridge` module that main removed in
a758a72 (renamed to `routing`), so `java/src/lib.rs` no longer
compiled.

Restore main's shape:
- Delete the resurrected helper.
- Delete the duplicated protobuf_bridge routing block in
  `executeBatchAsync` so only the `routing::resolve_routing_from_params`
  path remains.
- Route the two inline root-span drops in `executeBatchAsync` and
  `executeCommandAsync` through `GlideOpenTelemetry::drop_span_ptr`,
  which is what the refactor was meant to do in the first place.

Also change `GlideOpenTelemetry::drop_span_ptr`'s error type from
the now-unimported `TraceError` to the wrapping `GlideOTELError`
that the rest of the module returns after main's refactor. The
public shape and semantics are unchanged; only the concrete error
type differs.

Signed-off-by: Thomas Zhou <thomaszhou64@gmail.com>
The two trace-context FFI entry points (create_otel_span_with_trace_context
and create_batch_otel_span_with_trace_context) leak via span_to_ffi_pointer
instead of a direct Arc::into_raw. Route that helper through
GlideOpenTelemetry::leak_span so every FFI create tail exercises the shared
create path.

Signed-off-by: Thomas Zhou <thomaszhou64@gmail.com>
The single-statement drop_span_ptr call fits on one line at 100 columns,
matching the sibling site in executeBatchAsync. rustfmt --check was
splitting the let/unsafe onto two lines.

Signed-off-by: Thomas Zhou <thomaszhou64@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Rust ⚙️ Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant