fix(sdk): replace env::set_var with direct filter in FFI logging setup#3302
fix(sdk): replace env::set_var with direct filter in FFI logging setup#3302
Conversation
Replace `env::set_var("RUST_LOG", ...)` in `dash_sdk_enable_logging`
with direct `tracing_subscriber::EnvFilter` construction and subscriber
initialization. The previous approach was a data race when called after
the Tokio runtime (or any other threads) had started, since
`env::set_var` is unsound in multi-threaded POSIX processes and has been
deprecated as safe since Rust 1.66.
The new implementation constructs the same per-crate filter directives
in-process and passes them directly to `tracing_subscriber::fmt` via
`EnvFilter::new()`, which never reads or writes environment variables.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced environment-variable-based logging with in-process tracing-subscriber setup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3302 +/- ##
============================================
- Coverage 79.93% 79.93% -0.01%
============================================
Files 2861 2861
Lines 280230 280236 +6
============================================
Hits 223993 223993
- Misses 56237 56243 +6
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "7f4d96d07d78bf6fda77bf80eb0586cdea652cbda5b27323796c1df1e578d4de"
)Xcode manual integration:
|
|
@lklimek can you review this one? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk-ffi/src/lib.rs (1)
206-214: Log message may be misleading when subscriber is already installed.When
try_init()returnsErr(subscriber already exists), the "logging enabled" message is still emitted via the existing subscriber, not the one we tried to create. This can mislead callers into thinking our per-crate filter was applied.Per the context snippets,
rs-drive-abcimay initialize its own subscriber viatry_init()before this FFI function runs, making this scenario likely in integrated deployments.Consider either:
- Conditionally logging based on
try_init()result, or- Adjusting the message to clarify when the filter was actually applied.
♻️ Suggested improvement
- // Initialize the global tracing subscriber. `try_init` returns Err if a - // subscriber is already installed; we intentionally ignore that so that - // calling this function more than once is harmless. - let _ = tracing_subscriber::fmt::fmt() + // Initialize the global tracing subscriber. `try_init` returns Err if a + // subscriber is already installed; we intentionally ignore that so that + // calling this function more than once is harmless. + let installed = tracing_subscriber::fmt::fmt() .with_env_filter(filter) - .try_init(); + .try_init() + .is_ok(); - tracing::info!(level = log_level, "logging enabled"); + if installed { + tracing::info!(level = log_level, "logging enabled with per-crate filter"); + } else { + tracing::debug!(level = log_level, "subscriber already installed; per-crate filter not applied"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/lib.rs` around lines 206 - 214, The current code unconditionally emits tracing::info! after calling tracing_subscriber::fmt().with_env_filter(filter).try_init(), which can mislead when try_init() returns Err (subscriber already installed); update the init sequence to capture the Result from tracing_subscriber::fmt().fmt().with_env_filter(filter).try_init() and conditionally log: on Ok(...) emit the existing "logging enabled" message (including the applied filter or log_level if desired), and on Err(...) emit a clear message that the subscriber was already installed and the per-crate filter was not applied (or that existing settings remain); use the try_init() result to drive which message is logged so callers of the FFI see accurate information.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk-ffi/src/lib.rs`:
- Around line 206-214: The current code unconditionally emits tracing::info!
after calling tracing_subscriber::fmt().with_env_filter(filter).try_init(),
which can mislead when try_init() returns Err (subscriber already installed);
update the init sequence to capture the Result from
tracing_subscriber::fmt().fmt().with_env_filter(filter).try_init() and
conditionally log: on Ok(...) emit the existing "logging enabled" message
(including the applied filter or log_level if desired), and on Err(...) emit a
clear message that the subscriber was already installed and the per-crate filter
was not applied (or that existing settings remain); use the try_init() result to
drive which message is logged so callers of the FFI see accurate information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a90f1cad-073c-4a0e-8ea7-eb4b6f683a7c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
packages/rs-sdk-ffi/Cargo.tomlpackages/rs-sdk-ffi/src/lib.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Solid security fix replacing unsafe env::set_var with direct EnvFilter construction. The core change is correct and thread-safe. Two real issues: the filter omits the FFI crate's own tracing target (rs_sdk_ffi), and the regression test reintroduces the same class of unsafe env mutation this PR eliminates.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-ffi/src/lib.rs`:
- [SUGGESTION] lines 197-200: FFI crate's own tracing events are silently filtered out
The filter string includes `rs_sdk` but not `rs_sdk_ffi`. In tracing's EnvFilter, `rs_sdk` matches targets like `rs_sdk::foo` (child modules delimited by `::`) but does NOT match `rs_sdk_ffi` — that's a distinct crate target. This means `tracing::info!(...)` at line 213 (implicit target `rs_sdk_ffi`) is silently dropped, and any future diagnostics emitted from this crate are invisible. The old code had the same omission (RUST_LOG also lacked rs_sdk_ffi), so this isn't a regression, but since we're rebuilding the filter from scratch it's a good opportunity to fix it.
- [SUGGESTION] lines 231-239: Regression test reintroduces unsafe env mutation that the PR is eliminating
The test calls `unsafe { std::env::remove_var("RUST_LOG") }` with a safety comment claiming the binary is single-threaded. This is incorrect: `cargo test` runs `#[test]` functions on parallel threads by default, so `enable_logging_does_not_set_env_var` may execute concurrently with `enable_logging_is_thread_safe` (which itself spawns additional threads). This is the same class of data race the PR is fixing.
Since the new implementation never touches RUST_LOG, the test can simply assert that `std::env::var("RUST_LOG").is_err()` without the remove_var preamble — or guard with `env::var_os` before asserting and skip if already set.
- Add rs_sdk_ffi to the tracing filter string so the FFI crate's own events are not silently dropped - Only emit "logging enabled" info log on successful try_init (not on idempotent re-calls where the level is silently discarded) - Remove unsafe env::remove_var from test — skip instead if RUST_LOG is already set, avoiding the same data race class the PR is fixing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean follow-up addressing all three prior review findings. The filter string now includes rs_sdk_ffi, the tracing::info! call is correctly gated on successful subscriber initialization, and the test no longer uses unsafe { env::remove_var } — replaced with a safe early-return guard. No new issues.
Reviewed commit: 1ea0a91
Use try_from_default_env() to honor RUST_LOG when set, falling back to the hardcoded per-crate filter. This reads but never writes env vars, preserving thread safety. Also clarify that tracing-subscriber's built-in tracing-log bridge handles log crate compatibility without needing a separate env_logger init. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the rs-sdk-ffi logging initialization to avoid std::env::set_var("RUST_LOG", ...) (which is unsound in multi-threaded contexts) by configuring a tracing_subscriber filter directly inside the FFI entrypoint.
Changes:
- Replaced
RUST_LOGmutation with an in-processtracing_subscriber::EnvFilter+fmtsubscriber initialization. - Added
tracing-subscriberas a direct dependency ofrs-sdk-ffiwithenv-filterandfmtfeatures. - Added unit tests asserting
dash_sdk_enable_loggingdoesn’t setRUST_LOGand can be called concurrently.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/rs-sdk-ffi/src/lib.rs | Builds an explicit filter string and initializes a global tracing subscriber via try_init(), plus adds unit tests. |
| packages/rs-sdk-ffi/Cargo.toml | Adds tracing-subscriber dependency needed by the new logging setup. |
| Cargo.lock | Locks the new tracing-subscriber dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-targeted security fix. Replaces the unsafe env::set_var("RUST_LOG") data race with direct tracing_subscriber::EnvFilter construction and try_init() for idempotent subscriber registration. All six review agents (general, security, FFI) independently confirm the fix is correct: the data race is eliminated, try_init() is the right choice for FFI, repeated calls are safe no-ops, and RUST_LOG is still respected via read-only try_from_default_env(). The rs_sdk_ffi crate was also added to the filter directives (previously missing). Tests cover both the no-env-mutation guarantee and concurrent thread safety.
Reviewed commit: 7599f5f
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Issue being fixed or feature implemented
Security audit found that
dash_sdk_enable_loggingcallsstd::env::set_var("RUST_LOG", ...)which is a data race in multi-threaded contexts. Since Rust 1.66+,env::set_varis deprecated as safe because it is unsound on POSIX systems when other threads may concurrently read environment variables. The Tokio runtime may already have worker threads active when this function is called.What was done?
env::set_var("RUST_LOG", ...)with directtracing_subscriber::EnvFilter::new(filter_string)constructiontracing_subscriber::fmtsubscriber is initialized directly with the filter viatry_init(), which is safe to call multiple times (subsequent calls are no-ops)tracing-subscriberwithenv-filterandfmtfeatures as a direct dependency ofrs-sdk-ffiHow Has This Been Tested?
enable_logging_does_not_set_env_varthat verifies RUST_LOG is never set by the functionenable_logging_is_thread_safethat calls the function concurrently from multiple threads to confirm no panics or data racesrs-sdk-ffilib tests passcargo clippy -p rs-sdk-fficlean (no warnings)cargo fmt --allcleanBreaking Changes
None. The function signature and behavior are unchanged -- logging output is identical, only the internal mechanism for configuring the tracing subscriber has changed.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation