Skip to content

fix: enforce span limit in curl_multi_exec and DDTrace\start_span code paths#3691

Merged
Leiyks merged 2 commits into
masterfrom
arulleau/fix-curl-multi-span-limit-bypass
Mar 6, 2026
Merged

fix: enforce span limit in curl_multi_exec and DDTrace\start_span code paths#3691
Leiyks merged 2 commits into
masterfrom
arulleau/fix-curl-multi-span-limit-bypass

Conversation

@Leiyks

@Leiyks Leiyks commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Summary

DD_TRACE_SPANS_LIMIT was correctly enforced in the hook system (uhook.c, uhook_legacy.c, uhook_attributes.c) but bypassed in the curl_multi_exec code paths, causing unbounded span creation and OOM when curl multi is called repeatedly (e.g. AWS SDK SQS workloads on ECS Fargate).

Fixes APMS-18744.

Root cause

  • dd_multi_inject_headers() (ext/handlers_curl.c): PHP 8 curl multi creates one DDTRACE_INTERNAL_SPAN per handle on every curl_multi_exec call without checking the limit. With ~100k SQS calls (each using curl multi internally), this produced 100k+ unconstrained spans — the primary cause of the reported OOM.
  • dd_inject_distributed_tracing_headers_multi() (ext/handlers_curl_php7.c): same issue on PHP 7.

Fix

Add !ddtrace_tracer_is_limited() guards to both curl multi paths. When the span limit is reached, the paths fall through to header injection only (no span created) — distributed tracing propagation is preserved, only span tracking is skipped.

DD_TRACE_SPANS_LIMIT was correctly enforced in the hook system
(uhook.c, uhook_legacy.c, uhook_attributes.c) but bypassed in three
other code paths, causing unbounded span creation and OOM when
curl_multi_exec is called repeatedly (e.g. AWS SDK SQS workloads).

Missing checks:
- dd_multi_inject_headers() (ext/handlers_curl.c): PHP 8 curl multi
  creates one INTERNAL_SPAN per handle without checking the limit.
  This is the primary path causing the reported customer OOM.
- dd_inject_distributed_tracing_headers_multi() (ext/handlers_curl_php7.c):
  same issue on PHP 7.
- dd_start_span() / DDTrace_start_trace_span() (ext/ddtrace.c):
  PHP-level DDTrace\start_span() and DDTrace\start_trace_span() APIs
  open real spans without checking the limit.

Fix: add !ddtrace_tracer_is_limited() guards to each path. The curl
multi paths fall through to header injection only (no span) when
limited, preserving distributed tracing propagation. The PHP-level
APIs return a dummy span (matching the disabled-tracing behaviour).

Fixes: APMS-18744
@Leiyks
Leiyks requested a review from a team as a code owner March 6, 2026 12:55
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Mar 6, 2026

Copy link
Copy Markdown

⚠️ Tests

Fix all issues with BitsAI or with Cursor

⚠️ Warnings

🧪 1029 Tests failed

testSearchPhpBinaries from integration.DDTrace\Tests\Integration\PHPInstallerTest (Datadog) (Fix with Cursor)
Risky Test
phpvfscomposer://tests/vendor/phpunit/phpunit/phpunit:52
testSimplePushAndProcess from laravel-58-test.DDTrace\Tests\Integrations\Laravel\V5_8\QueueTest (Datadog) (Fix with Cursor)
Risky Test
phpvfscomposer://tests/vendor/phpunit/phpunit/phpunit:60
testSimplePushAndProcess from laravel-8x-test.DDTrace\Tests\Integrations\Laravel\V8_x\QueueTest (Datadog) (Fix with Cursor)
DDTrace\Tests\Integrations\Laravel\V8_x\QueueTest::testSimplePushAndProcess
Test code or tested code printed unexpected output: spanLinksTraceId: 69aad57a00000000bd10566b097cae82
tid: 69aad57a00000000
hexProcessTraceId: bd10566b097cae82
hexProcessSpanId: 6e990da572b76246
processTraceId: 13623483890516405890
processSpanId: 7969416019854189126

phpvfscomposer://tests/vendor/phpunit/phpunit/phpunit:106
View all

ℹ️ Info

❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bf6cd22 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

@codecov-commenter

codecov-commenter commented Mar 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.08%. Comparing base (0d2b2ff) to head (bf6cd22).
⚠️ Report is 164 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3691      +/-   ##
==========================================
- Coverage   62.17%   62.08%   -0.09%     
==========================================
  Files         141      141              
  Lines       13352    13352              
  Branches     1746     1746              
==========================================
- Hits         8302     8290      -12     
- Misses       4259     4269      +10     
- Partials      791      793       +2     

see 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d2b2ff...bf6cd22. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DDTrace\start_span() and DDTrace\start_trace_span() are user-facing
APIs that intentionally bypass the span limit — the limit applies to
auto-instrumentation (hooks) only. Guarding these APIs with
ddtrace_tracer_is_limited() broke:

- testTracerFlushedWhenSpanLimitExceeded: the test explicitly verifies
  that user-created spans work even when DD_TRACE_SPANS_LIMIT is hit.
- Guzzle integration tests: isolateTracer() uses start_trace_span()
  internally to create isolated trace contexts; when closed_spans_count
  accumulated past the limit across PHPUnit tests, no new stack was
  created and all spans in the isolated test were lost.

The curl multi code paths in handlers_curl.c and handlers_curl_php7.c
remain fixed (the actual OOM culprit reported in APMS-18744).

@bwoebi bwoebi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should do this, yes.

@pr-commenter

pr-commenter Bot commented Mar 6, 2026

Copy link
Copy Markdown

Benchmarks [ tracer ]

Benchmark execution time: 2026-03-06 14:31:49

Comparing candidate commit bf6cd22 in PR branch arulleau/fix-curl-multi-span-limit-bypass with baseline commit 0d2b2ff in branch master.

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

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟩 execution_time [-5.296µs; -3.844µs] or [-4.820%; -3.499%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟩 execution_time [-4.051µs; -2.849µs] or [-3.714%; -2.612%]

@Leiyks
Leiyks merged commit b33ea57 into master Mar 6, 2026
2061 of 2063 checks passed
@Leiyks
Leiyks deleted the arulleau/fix-curl-multi-span-limit-bypass branch March 6, 2026 15:26
@github-actions github-actions Bot added this to the 1.17.0 milestone Mar 6, 2026
Leiyks added a commit that referenced this pull request Jul 3, 2026
…ans (APMS-19944) (#4030)

* fix(curl): enforce DD_TRACE_SPANS_LIMIT for curl_multi_exec parent spans (APMS-19944)

The curl_multi_exec integration opens its parent span via the user-facing
DDTrace\start_span(), which intentionally bypasses DD_TRACE_SPANS_LIMIT
(see #3691). Distributed-header injection then flags that span NOT_DROPPABLE,
so the post-hook's try_drop_span() can never reclaim it. In long-running CLI
workloads that repeatedly call curl_multi_exec (e.g. AWS SDK SQS batches),
one un-droppable span accumulates per call, growing memory 1:1 with calls
until OOM.

Skip creating the parent span when the tracer is already limited. Distributed
tracing headers are still injected by the C handler against the active span,
so propagation is preserved, and the child request spans are already limited.

Fixes: APMS-19944

* test(curl): drop flaky is_limited assertion from span-limit test

dd_trace_tracer_is_limited() reflects the open+closed span counters at the
moment of the call, which depends on end-of-run flush/cleanup timing and
varies across CI environments and PHP versions (7.3/7.4/8.2-ASAN all reported
bool(false) at script end). The span-count assertion (BOUNDED) is the real
regression guard: with the fix the retained curl_multi_exec spans stay capped
near DD_TRACE_SPANS_LIMIT, without it they scale 1:1 with iterations. Bump to
100 iterations for a clearer margin and drop the fragile check.

* test(curl): remove comments from span-limit test

* fix(curl): check span limit at top of curl_multi_exec hook

Address review: move the ddtrace_tracer_is_limited() guard to the start of
the begin hook so the limited path returns immediately, skipping the args
lookup and ObjectKVStore reuse check. Spans for a multi-op are only created
on the first curl_multi_exec call, so a new handle has nothing to reuse when
limited; the boundary case (parent created just before the limit) is still
finalized by CurlSpanInfo::__destruct().
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