Skip to content

Synchronize native transport fork teardown - #6127

Merged
lloeki merged 11 commits into
masterfrom
lloeki/native-fork-invariant
Aug 6, 2026
Merged

lloeki merged 11 commits into
masterfrom
lloeki/native-fork-invariant

Conversation

@lloeki

@lloeki lloeki commented Jul 30, 2026 •

Copy link
Copy Markdown
Member

AI-generated code disclosure: this PR was implemented with substantial AI assistance and manually reviewed and validated.

What does this PR do?

Makes native transport fork lifecycle dispatch atomic and failure-safe. It coordinates close with in-flight sends and forks, runs every completion callback before re-raising the first error, and prevents finalizer and reentrant-lock deadlocks.

Motivation:

Native transport teardown can race with sends or fork handling, and exceptional callbacks can otherwise leave lifecycle work incomplete. This is the internal safety work tracked by APMSP-3823.

Change log entry

No.

Additional Notes:

How to test the change?

  • Ran the core/main suite: 6,320 examples, 0 failures.
  • Ran the native transport suite: 120 examples, 0 failures, including repeated seeded runs covering concurrent send, fork, close, callback failure, finalizer, and reentrant lifecycle paths.
  • Ran Standard and Steep successfully.

lloeki added 3 commits July 29, 2026 18:21
Fork hooks, sends, and transport teardown share the exporter and its
runtime. Serialize `close` and finalization against active fork
lifecycles and in-flight sends so hooks cannot disappear mid-fork or
the exporter be released during a send.

Use a blocking mock agent to exercise these races deterministically
instead of relying on timing delays.
A failed fork or `:before` callback can leave earlier callbacks holding
resources. Always run the matching `:parent` callbacks, and continue
running cleanup callbacks after one fails before re-raising the first
error.

Defer finalizer hook removal when it interrupts an active fork so the
runtime and lifecycle locks are restored first.
Capture one callback snapshot for the complete fork lifecycle and
register callback triplets atomically. This prevents concurrent
registration or removal from pairing a `:before` callback with the
wrong `:parent` or `:child` cleanup.

Track lock acquisition explicitly so a reentrant `close` cannot
release locks owned by the surrounding fork lifecycle.
@lloeki
lloeki requested a review from a team as a code owner July 30, 2026 11:04
@lloeki lloeki added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jul 30, 2026
@lloeki
lloeki requested review from a team as code owners July 30, 2026 11:04
@lloeki
lloeki requested a review from marcotc July 30, 2026 11:04
@lloeki lloeki added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jul 30, 2026
@lloeki
lloeki requested a review from vpellan July 30, 2026 11:04
@dd-octo-sts dd-octo-sts Bot added core Involves Datadog core libraries tracing labels Jul 30, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Thank you for updating Change log entry section 👏

Visited at: 2026-07-30 12:12:00 UTC

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ad246116c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/datadog/tracing/transport/native.rb Outdated
Comment thread lib/datadog/core/utils/at_fork_monkey_patch.rb Outdated
Comment thread lib/datadog/core/utils/at_fork_monkey_patch.rb Outdated
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 51.85%
• Overall Coverage: 89.99% (-0.04%)

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

@pr-commenter

pr-commenter Bot commented Jul 30, 2026 •

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-08-05 16:25:49

Comparing candidate commit df7082d in PR branch lloeki/native-fork-invariant with baseline commit 0a15637 in branch master.

📊 Benchmarking dashboard

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:tracing - trace.to_digest - Continue

  • unstable throughput [-1382.603op/s; +1616.964op/s] or [-4.681%; +5.475%]

lloeki added 3 commits August 5, 2026 09:40
Rely on per-fork callback snapshots to keep completion hooks alive after transport teardown. This removes the unsynchronised  counter and finalizer polling that could leave  waiting indefinitely while preserving runtime restoration for active forks.
Run all parent cleanup callbacks after a failed fork, but keep the
original exception as the caller-visible failure. Log a secondary cleanup
error instead of allowing it to replace the error that triggered cleanup.
Define the registry snapshot and mutex purpose where they are introduced,
record the signal-trap limitation, and distinguish copied callback sets from
closures that keep exporter state alive.

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

It's a bit hard to read and follow tests, but I would give non-blocking 👍🏼

Comment thread lib/datadog/core/utils/at_fork_monkey_patch.rb Outdated
lloeki added 5 commits August 5, 2026 17:44
Publish callback registry updates with immutable copy-on-write values so
fork dispatch never acquires a Ruby mutex. This preserves one registry view
for each lifecycle and allows ordinary fork dispatch from signal traps.
Associate atomically registered callback triplets within each immutable
registry snapshot. If a before callback fails, run grouped parent cleanup
only for triplets whose setup started while preserving standalone semantics.
Keep the signature synchronized with the immutable callback registry added
for lock-free fork snapshots.
Make every callback dispatch identify the immutable registry view for its
fork lifecycle. This prevents separate stages from accidentally observing
different callback sets and removes a test-only compatibility default.
Expand the raising callback into a multiline block so it satisfies the
repository Style/Semicolon rule.
@lloeki
lloeki merged commit 79c3bbc into master Aug 6, 2026
607 checks passed
@lloeki
lloeki deleted the lloeki/native-fork-invariant branch August 6, 2026 10:11
@dd-octo-sts dd-octo-sts Bot added this to the 2.41.0 milestone Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos core Involves Datadog core libraries tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants