Skip to content

fix(library-config): update Linux process context#2237

Open
cataphract wants to merge 1 commit into
glopes/otel-process-ctx-macos-winfrom
glopes/otel-process-ctx-linux-update
Open

fix(library-config): update Linux process context#2237
cataphract wants to merge 1 commit into
glopes/otel-process-ctx-macos-winfrom
glopes/otel-process-ctx-linux-update

Conversation

@cataphract

@cataphract cataphract commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This is 2/3 in the stacked process-context series and depends on #2228.

It adapts the Linux implementation after the code-organization layer:

  • makes MADV_DONTFORK best-effort instead of treating it as a publishing failure;
  • detects whether the process-context mapping is available after a fork, rather than relying on a stored publisher PID;
  • avoids reading or unmapping inherited state when the mapping is not present in the child;
  • updates the Unix copy-pipe implementation and comments to match the current behavior;
  • changes the default feature set to the writer, while keeping the reader available as an opt-in feature.

Why?

#2228 intentionally preserves the old Linux behavior while introducing the shared reader/writer structure. This PR applies the Linux behavior changes separately so they can be reviewed without being mixed with either the file reorganization or the macOS/Windows implementation.

Impact

Linux keeps the same platform-agnostic process-context API, with corrected fork handling and the final feature defaults. This layer does not add macOS or Windows support.

Validation

Validated independently at this commit with:

  • cargo check -p libdd-library-config --all-features;
  • cargo check -p libdd-library-config-ffi;
  • cargo nextest run -p libdd-library-config --all-features --no-fail-fast;
  • cargo nextest run -p libdd-library-config --no-default-features --features process-context-reader --no-fail-fast.

Stack

  1. refactor(library-config): reorganize Linux process context #2228 — reorganize the Linux process context
  2. fix(library-config): update Linux process context #2237 — update the Linux implementation
  3. feat(library-config): add macOS and Windows process context #2238 — add macOS and Windows support

@cataphract cataphract requested a review from a team as a code owner July 14, 2026 17:57
@cataphract cataphract requested review from vpellan and removed request for a team July 14, 2026 17:57
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

📚 Documentation Check Results

⚠️ 265 documentation warning(s) found

📦 libdd-library-config - 265 warning(s)


Updated: 2026-07-14 23:07:23 UTC | Commit: 64ca9ca | missing-docs job results

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

🔒 Cargo Deny Results

⚠️ 1 issue(s) found, showing only errors (advisories, bans, sources)

📦 libdd-library-config - 1 error(s)

Show output
error[unsound]: Rand is unsound with a custom logger using `rand::rng()`
   ┌─ /home/runner/work/libdatadog/libdatadog/Cargo.lock:46:1
   │
46 │ rand 0.8.5 registry+https://github.com/rust-lang/crates.io-index
   │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ unsound advisory detected
   │
   ├ ID: RUSTSEC-2026-0097
   ├ Advisory: https://rustsec.org/advisories/RUSTSEC-2026-0097
   ├ It has been reported (by @lopopolo) that the `rand` library is [unsound](https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#soundness-of-code--of-a-library) (i.e. that safe code using the public API can cause Undefined Behaviour) when all the following conditions are met:
     
     - The `log` and `thread_rng` features are enabled
     - A [custom logger](https://docs.rs/log/latest/log/#implementing-a-logger) is defined
     - The custom logger accesses `rand::rng()` (previously `rand::thread_rng()`) and calls any `TryRng` (previously `RngCore`) methods on `ThreadRng`
     - The `ThreadRng` (attempts to) reseed while called from the custom logger (this happens every 64 kB of generated data)
     - Trace-level logging is enabled or warn-level logging is enabled and the random source (the `getrandom` crate) is unable to provide a new seed
     
     `TryRng` (previously `RngCore`) methods for `ThreadRng` use `unsafe` code to cast `*mut BlockRng<ReseedingCore>` to `&mut BlockRng<ReseedingCore>`. When all the above conditions are met this results in an aliased mutable reference, violating the Stacked Borrows rules. Miri is able to detect this violation in sample code. Since construction of [aliased mutable references is Undefined Behaviour](https://doc.rust-lang.org/stable/nomicon/references.html), the behaviour of optimized builds is hard to predict.
   ├ Announcement: https://github.com/rust-random/rand/pull/1763
   ├ Solution: Upgrade to >=0.10.1 OR <0.10.0, >=0.9.3 OR <0.9.0, >=0.8.6 (try `cargo update -p rand`)
   ├ rand v0.8.5
     └── libdd-library-config v3.0.0

advisories FAILED, bans ok, sources ok

Updated: 2026-07-14 23:09:17 UTC | Commit: 64ca9ca | dependency-check job results

@pr-commenter

pr-commenter Bot commented Jul 14, 2026

Copy link
Copy Markdown

Benchmarks

Comparison

Candidate

Candidate benchmark details

Baseline

Baseline benchmark details

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 80.26%
Overall Coverage: 74.47% (-0.03%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 97138bc | Docs | Datadog PR Page | Give us feedback!

@cataphract cataphract force-pushed the glopes/otel-process-ctx-macos-win branch from 1a04bb0 to e6af895 Compare July 14, 2026 18:20
@cataphract cataphract force-pushed the glopes/otel-process-ctx-linux-update branch from e5a142d to a9e0f0e Compare July 14, 2026 18:20
@dd-octo-sts

dd-octo-sts Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Artifact Size Benchmark Report

aarch64-alpine-linux-musl
Artifact Baseline Commit Change
/aarch64-alpine-linux-musl/lib/libdatadog_profiling.so 7.88 MB 7.88 MB 0% (0 B) 👌
/aarch64-alpine-linux-musl/lib/libdatadog_profiling.a 86.15 MB 86.16 MB +.01% (+9.55 KB) 🔍
aarch64-unknown-linux-gnu
Artifact Baseline Commit Change
/aarch64-unknown-linux-gnu/lib/libdatadog_profiling.a 97.40 MB 97.41 MB +.01% (+10.98 KB) 🔍
/aarch64-unknown-linux-gnu/lib/libdatadog_profiling.so 10.62 MB 10.62 MB +0% (+88 B) 👌
libdatadog-x64-windows
Artifact Baseline Commit Change
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.dll 25.54 MB 25.54 MB 0% (0 B) 👌
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.lib 88.44 KB 88.44 KB 0% (0 B) 👌
/libdatadog-x64-windows/debug/dynamic/datadog_profiling_ffi.pdb 185.33 MB 185.34 MB +0% (+16.00 KB) 👌
/libdatadog-x64-windows/debug/static/datadog_profiling_ffi.lib 957.25 MB 957.26 MB +0% (+10.35 KB) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.dll 8.35 MB 8.35 MB 0% (0 B) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.lib 88.44 KB 88.44 KB 0% (0 B) 👌
/libdatadog-x64-windows/release/dynamic/datadog_profiling_ffi.pdb 24.72 MB 24.72 MB 0% (0 B) 👌
/libdatadog-x64-windows/release/static/datadog_profiling_ffi.lib 49.21 MB 49.21 MB -0% (-228 B) 👌
libdatadog-x86-windows
Artifact Baseline Commit Change
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.dll 22.19 MB 22.19 MB -0% (-512 B) 👌
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.lib 89.82 KB 89.82 KB 0% (0 B) 👌
/libdatadog-x86-windows/debug/dynamic/datadog_profiling_ffi.pdb 189.78 MB 189.77 MB -0% (-8.00 KB) 👌
/libdatadog-x86-windows/debug/static/datadog_profiling_ffi.lib 945.93 MB 945.94 MB +0% (+10.35 KB) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.dll 6.46 MB 6.46 MB 0% (0 B) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.lib 89.82 KB 89.82 KB 0% (0 B) 👌
/libdatadog-x86-windows/release/dynamic/datadog_profiling_ffi.pdb 26.54 MB 26.54 MB 0% (0 B) 👌
/libdatadog-x86-windows/release/static/datadog_profiling_ffi.lib 46.82 MB 46.82 MB -0% (-200 B) 👌
x86_64-alpine-linux-musl
Artifact Baseline Commit Change
/x86_64-alpine-linux-musl/lib/libdatadog_profiling.a 76.96 MB 76.97 MB +0% (+5.83 KB) 👌
/x86_64-alpine-linux-musl/lib/libdatadog_profiling.so 8.81 MB 8.81 MB 0% (0 B) 👌
x86_64-unknown-linux-gnu
Artifact Baseline Commit Change
/x86_64-unknown-linux-gnu/lib/libdatadog_profiling.a 92.37 MB 92.38 MB +0% (+5.37 KB) 👌
/x86_64-unknown-linux-gnu/lib/libdatadog_profiling.so 10.72 MB 10.72 MB +.03% (+3.79 KB) 🔍

@cataphract cataphract force-pushed the glopes/otel-process-ctx-macos-win branch from e6af895 to 89a636d Compare July 14, 2026 23:03
@cataphract cataphract force-pushed the glopes/otel-process-ctx-linux-update branch from a9e0f0e to 97138bc Compare July 14, 2026 23:05

@yannham yannham 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.

Thanks for stress-testing and fixing the fork case! Mostly non-blocking comments. If I understand correctly, if MADV_DONTFORK isn't supported and a fork happens, we don't have to do anything special because update, publish or unmap would touch the copy-on-write copy of the child. The only issue being that for some small time interval the child will appear as having the same context as its parent, instead of its own (which is why we are using MADV_DONTFORK in the first place). Is that more or less correct?

// should anything have been written already we would get a short write
// However, this is not the case for macOS, despite what its manual
// says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186
pipe_dirty: true,

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.

Should it then be cfg-gated, instead of using true for both platforms? Or maybe we don't really care, because putting true for Linux just causes the pipe to be reinitialized which is fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)

@@ -63,7 +60,7 @@ impl super::HeaderMemoryHolder for MemMapping {
}

fn after_fork(self) {

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.

What's the use-case for after_fork? If it's the same as drop, and drop is called automatically anyway when we exit a scope, it looks like it's a no-op

(*header)
.payload_size
.store(payload_size, Ordering::Relaxed);
(*header)

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.

Any reason for the re-ordering here? Not that it's important, since both accesses are Relaxed, this shouldn't change anything from the point of view of the reader.

Comment on lines 256 to 264
// If we've been forked, we need to prevent the mapping from being dropped
// normally, as it would try to unmap a region that isn't mapped anymore in the
// child process, or worse, could have been remapped to something else in the
// meantime.
//
// To do so, we get the old handler back in `local_handler` and prevent `mapping`
// from being dropped specifically.
swap(&mut local_handler, handler);
local_handler.mapping.after_fork();

@yannham yannham Jul 16, 2026

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 wonder if all the comment and the local_handler dance is relevant anymore. Now that you've pushed the check to the unmap implementation, I think we can just use swap and call it a day.

@ivoanjo ivoanjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like to know more about the MADV_DONTFORK change before we apply it 👀

// should anything have been written already we would get a short write
// However, this is not the case for macOS, despite what its manual
// says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186
pipe_dirty: true,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)

Comment on lines +28 to +29
/// `Some(pid)` when `MADV_DONTFORK` succeeded, otherwise `None`.
only_for_pid: Option<u32>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wait, when can MADV_DONTFORK fail on Linux? It's not clear from the comments/PR description why we're treating it as might-fail now.

In particular, the spec and other implementations are requiring MADV_DONTFORK; this is not to say that the spec is correct -- perhaps there's a good reason to change this, which is why I'm asking.

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.

3 participants