Skip to content

appsec: fix bailouts during rshutdown#3941

Merged
cataphract merged 72 commits into
glopes/request-exec-outside-reqfrom
glopes/rshutdown-bailout
Jun 9, 2026
Merged

appsec: fix bailouts during rshutdown#3941
cataphract merged 72 commits into
glopes/request-exec-outside-reqfrom
glopes/rshutdown-bailout

Conversation

@cataphract

Copy link
Copy Markdown
Contributor

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

cataphract and others added 30 commits April 27, 2026 12:10
- List bridge jobs alongside regular jobs so failed triggers like
  deploy_to_reliability_env show up in --list-jobs output
- paginated_get reads X-Total-Pages from the first response and fetches
  all remaining pages in parallel instead of sequentially (~3x speedup
  on paginated endpoints)
- get_child_pipelines fetches sibling children in parallel
- Merge success/failure/counter passes over all_jobs into one loop
- api_get now returns headers as a third tuple element
- Remove dead 401/403 re-check in discover_pipeline
- Remove .replace("Z", "+00:00") workaround (Python 3.11+ handles Z)
- Fix any -> Any type annotation
- Bridge/trigger jobs and regular jobs with allow_failure: true are now
  counted in allow_fail= on the status line instead of failed=, and no
  FAILED: line is printed for them. fetch_all_jobs now also fetches
  bridge jobs so their allow_failure flag is visible.
- show pipeline name (from bridge job) and job IDs in --list-jobs output
- merge pipeline/job/bridge discovery into one pass
  (discover_pipelines_and_jobs) eliminating double bridge fetches and
  separate fetch_pipeline_statuses calls
- fix compute_duration to use GitLab's duration field (not created_at
  queue time)
- replace sys.exit() in api_get/discover_pipeline with GitLabError
  exception
- open success.txt/failure.txt once per poll cycle instead of per job
- warn when paginated_get receives a non-list response
- remove unused allow_fail_count
The canonical fixture PHPT explicitly enumerates the FFE classes it
requires before instantiating the Datadog FeatureFlags client. The
shared evaluation-completed envelope/hook added on this branch made
Client::createWithDependencies() reference NoopEvaluationCompletedHook,
EvaluationCompletedHook, and EvaluationCompleted, but the test helper
had not been updated, so the packaged/extension PHPT failed with
"Class DDTrace\FeatureFlags\Internal\NoopEvaluationCompletedHook not
found" before any fixture case ran.

Add the three new files to require_feature_flag_api so the PHPT
matches the runtime class graph used by Client::evaluate().
The canonical fixture PHPT explicitly enumerates the FFE classes it
requires before instantiating the Datadog FeatureFlags client. The
shared evaluation-completed envelope/hook added on this branch made
Client::createWithDependencies() reference NoopEvaluationCompletedHook,
EvaluationCompletedHook, and EvaluationCompleted, but the test helper
had not been updated, so the packaged/extension PHPT failed with
"Class DDTrace\FeatureFlags\Internal\NoopEvaluationCompletedHook not
found" before any fixture case ran.

Add the three new files to require_feature_flag_api so the PHPT
matches the runtime class graph used by Client::evaluate().
Adds Mermaid sources and rendered PNGs for the hook (this) PR plus a
README documenting the regeneration workflow.

- `docs/php-ffe-stack/stack-pr3909.mmd` + `.png` — 4-PR stack with this
  PR highlighted (M1 done; EVP and metrics as siblings to come).
- `docs/php-ffe-stack/system-pr3909.mmd` + `.png` — target system
  architecture; this PR contributes the EvaluationCompletedHook +
  OpenFeature provider hook surface. All downstream nodes (writers,
  sidecar FFI, sidecar process, backends) marked future.
- `docs/php-ffe-stack/README.md` — npx invocation for regenerating
  PNGs locally; PR-by-PR diagram table; architectural rule note.

The architectural rule encoded in the system diagram (all I/O via the
libdatadog sidecar) is the same rule Bob applied to PR #3910. See
DataDog/libdatadog#2026 for the sidecar-side support.
Per Bob's PR review (2026-05-22), the tracer extension must perform no
I/O outside the sidecar. Replaces the raw-socket `AgentExposureTransport`
with `SidecarExposureTransport`, which forwards exposure batches to the
libdatadog sidecar via a new native PHP function `\DDTrace\send_ffe_exposures`
that calls the `ddog_sidecar_send_ffe_exposures` FFI added in
DataDog/libdatadog#2026.

PHP side:

- Delete `Internal/Exposure/AgentExposureTransport.php` (raw socket
  POST to the Agent EVP proxy).
- Add `Internal/Exposure/SidecarExposureTransport.php` that JSON-encodes
  the batch and calls `\DDTrace\send_ffe_exposures()`. Fire-and-forget;
  the sidecar handles retries.
- Update `ExposureWriter::createDefault()` to instantiate the sidecar
  transport.
- Drop the obsolete `testAgentTransportBuildsAgentEvpRequest` PHPUnit
  test (HTTP construction now lives in libdatadog, covered by
  `cargo test -p datadog-sidecar ffe_flusher`).
- Add `Internal/DefaultEvaluationCompletedHook` and
  `Internal/CompositeEvaluationCompletedHook` so production callers go
  through a composite hook factory. In this PR the composite contains
  only `ExposureHook`; the metrics PR (#3911) contributes
  `EvaluationMetricHook` and the file conflict at merge resolves by
  combining both. Update `Client::create()` to call
  `DefaultEvaluationCompletedHook::create()`.

C/Rust bridge:

- Declare `ddog_ByteSlice` (and underlying `ddog_Slice_U8`) in
  `components-rs/common.h` for the metrics path; declare both
  `ddog_sidecar_send_ffe_exposures` and `ddog_sidecar_send_ffe_metrics`
  in `components-rs/sidecar.h`.
- Add C wrappers `ddtrace_sidecar_send_ffe_exposures(zend_string *)`
  and `ddtrace_sidecar_send_ffe_metrics(zend_string *endpoint,
  zend_string *payload_bytes)` in `ext/sidecar.{h,c}` that call the FFI
  with the current sidecar transport + instance id + queue id.
- Declare native PHP functions `\DDTrace\send_ffe_exposures(string): bool`
  and `\DDTrace\send_ffe_metrics(string, string): bool` in
  `ext/ddtrace.stub.php`; add corresponding arginfo entries and
  `ZEND_FUNCTION` registrations in `ext/ddtrace_arginfo.h`; implement
  `PHP_FUNCTION(DDTrace_send_ffe_exposures)` and
  `PHP_FUNCTION(DDTrace_send_ffe_metrics)` in `ext/ddtrace.c`.
- Bump `libdatadog` submodule to FFE branch tip `29762335c` (which
  provides both FFIs). The submodule will be bumped to the libdatadog
  main commit once #2026 merges.

Docs:

- Add `docs/php-ffe-stack/{stack,system}-pr3910.{mmd,png}` for this PR.

Validation:

- `php vendor/bin/phpunit --config phpunit.xml tests/api/Unit/FeatureFlags`
  → 41 tests, 174 assertions, OK.
- libdatadog sidecar tests (`cargo test -p datadog-sidecar ffe_flusher`)
  → 3 passed, on the pinned submodule commit.
- Mermaid PNGs regenerate via `npx @mermaid-js/mermaid-cli`.

`make test_featureflags` and `make test_c TESTS=tests/ext/ffe/...` will
run in CI; running them locally requires rebuilding the extension which
is gated behind libdatadog #2026 merging.
Pulls in libdatadog commit `875ec8f0e` ("fix(sidecar): dispatch FFE
actions before application-entry check") so the EVP exposure batch sent
via `ddog_sidecar_send_ffe_exposures` is no longer silently dropped
when the PHP runtime hasn't yet registered the application against the
sidecar's `QueueId`. Same fix is on the sibling PR #3911 (`2a48c4987")
for the OTLP metric path.

Without this submodule bump, `Test_Feature_Flag_Parametric_Exposures::test_php_ffe_exposure_event`
sees zero EVP POSTs at the test-agent because the sidecar's
`enqueue_actions` handler discards the `FfeExposures` action under
the `Entry::Occupied` gate.
…macOS+colima

`build-debug-artifact` produces a `/output` bind mount via `-v
${TMP_OUT}:/output`. On macOS+colima only paths under $HOME are
mounted into the Linux VM; the macOS default `/var/folders/...` temp
dir is not, so writes from inside the container to `/output` land in
the VM and never propagate back to the host. The script exits without
error and the user's binaries dir is left with whatever stale artifact
was there before.

Pin TMP_OUT/TMP_PKG under $OUTPUT_DIR (which the user just passed as
an absolute, usable destination) so the bind mount is always on a
host-visible path. Inherits cleanup via the existing EXIT trap.

The same fix is independently on the M3 branch (#3911) as part of
e74b050; bringing it to the M2 branch (#3910) so both branches are
buildable on macOS without a `TMPDIR=$HOME/...` override.
…gh rez

Same three fixes as the parallel commit on the M3 (PR #3911) branch:

1. Quote the YAML `title:` so the `#PR-number` is not parsed as a
   comment (previous render had the title truncated at "current =").
2. `flowchart LR` → `flowchart TD` on system diagrams so vertical
   PHP-process → host → backend lanes stack vertically instead of
   getting squeezed into one wide row.
3. Render with `-w 2400 -H 2400 --scale 3 -b white` (~1900×2000 stack,
   ~3000×4600 system) instead of ~600px default.
…gh rez

Same three fixes as on the M2 (#3910) and M3 (#3911) sibling branches:

1. Quote the YAML `title:` so the `#PR-number` survives parsing
   (otherwise YAML treats the `#` as a comment and the title renders
   as "PHP FFE 4-PR stack — current =" with the rest missing).
2. `flowchart LR` → `flowchart TD` on the system diagram so the
   PHP-process / host-sidecar / backend lanes stack vertically.
3. Render at 2400×2400 `--scale 3` instead of ~600px default.
Until the FFE self-telemetry channel lands, exposure batch drops are
counted in an in-memory `$dropped` field that is only observable via
`droppedCount()` (used by tests) and not surfaced anywhere in
production. That's silent-data-loss territory.

Add a one-time `error_log()` breadcrumb on the first drop in a process
plus TODO(FFE-self-telemetry) markers at both drop sites (buffer
overflow + transport flush failure). The `maybeWarnFirstDrop` helper
and the warning go away when the real drop-counter metric is wired up.
… drop

The default `ExposureHook` only registers a `register_shutdown_function`
flush. In PHP-FPM/Apache that fires at the end of each request, so the
buffer empties between requests and the `bufferLimit` cap is effectively
per-request. In long-running PHP runtimes — Swoole, RoadRunner,
FrankenPHP/Octane, CLI worker loops — the shutdown callback only fires
when the worker process exits. The buffer fills once and every subsequent
unique exposure is silently dropped for the rest of the worker's
lifetime (hours or days).

Flush inline when the buffer hits `bufferLimit`. The flush call clears
the buffer regardless of transport success, so the append below always
finds capacity. If the inline flush itself fails (transport error), the
already-buffered events are counted as dropped — same semantics as an
explicit failed flush — and the new event is buffered for the next
flush attempt.

Test `testFullBufferDropsWithoutPoisoningDedupCache` renamed and rewritten
to assert the new behavior: `droppedCount() === 0` after a buffer-full
record() that previously would have dropped. New test
`testFullBufferAutoFlushPropagatesTransportFailure` covers the case
where the inline flush itself fails.

Caught by Codex (P2) review.
morrisonlevi and others added 13 commits June 4, 2026 14:27
* Emit feature_flag.evaluations through libdatadog sidecar OTLP forwarder

Adds the M3 evaluation-metrics layer on top of the hook PR (#3909) as a
sibling of the EVP exposures PR (#3910). Records `feature_flag.evaluations`
for both PHP 7 (DD Client hook) and PHP 8 (OpenFeature SDK hook); both
paths share `EvaluationMetricHook::sharedWriter()` for unified
aggregation. OTLP/protobuf payloads are encoded in PHP via the existing
`OtlpMetricEncoder` and delivered to the user-configured OTLP HTTP
metrics intake through the libdatadog sidecar (`ddog_sidecar_send_ffe_metrics`
FFI added in DataDog/libdatadog#2026).

This branch is force-pushed (user-authorized one-time exception to the
no-force-push rule, 2026-05-23) to restructure history away from being
linearly stacked on the M2 exposures PR (#3910). The PR now stacks
directly on the hook PR (#3909) as a sibling of the EVP PR.

PHP side:

- Add `Internal/Metric/EvaluationMetricWriter` with bounded series
  aggregation, drop accounting, and shutdown flush.
- Add `Internal/Metric/EvaluationMetricHook` (DD Client hook) and
  `OtlpMetricEncoder` (PHP 7-safe protobuf encoding).
- Add `Internal/Metric/SidecarOtlpMetricsTransport` that calls
  `\DDTrace\send_ffe_metrics()` (FFI declared in #3910). Endpoint
  resolution: `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT`, falling back to
  `OTEL_EXPORTER_OTLP_ENDPOINT + /v1/metrics`, default
  `http://localhost:4318/v1/metrics`.
- Add `DDTrace\OpenFeature\EvalMetricsHook` implementing
  `OpenFeature\interfaces\hooks\Hook` (after + error stages), registered
  on `DataDogProvider` via `setHooks()`.
- `DataDogProvider` constructs its internal DD `Client` with
  `DefaultEvaluationCompletedHook::createWithoutMetric()` so the
  OpenFeature path records the metric via the OpenFeature hook (PR 3911
  scope) and NOT via the DD Client hook — preventing double-counting.
  PHP 7 path keeps recording via the DD Client hook.
- Add `Internal/CompositeEvaluationCompletedHook` and
  `Internal/DefaultEvaluationCompletedHook` (metric-only composite).
  This is the merge-conflict point with PR #3910's `[ExposureHook]`
  composite — second merge resolves by combining both hooks.
- Update `Client::create()` to call `DefaultEvaluationCompletedHook::create()`.
- Drop the obsolete `testOtlpTransportBuildsHttpProtobufRequest` PHPUnit
  test (HTTP construction now lives in libdatadog, covered by
  `cargo test -p datadog-sidecar ffe_metrics_flusher`).
- Add `_files_openfeature.php` entry for `EvalMetricsHook.php`.

C/Rust bridge: the `\DDTrace\send_ffe_metrics()` native function, its C
wrapper `ddtrace_sidecar_send_ffe_metrics()`, and the
`ddog_sidecar_send_ffe_metrics` FFI declaration in `components-rs/sidecar.h`
were already added in #3910. This PR's branch picks up those changes
once #3910 merges (or via the same libdatadog submodule pin during
review). For development locally the libdatadog submodule is pinned to
the FFE branch tip (`29762335c`).

Docs:

- Add `docs/php-ffe-stack/{stack,system}-pr3911.{mmd,png}` per the
  4-PR documentation convention.

Validation:

- `php vendor/bin/phpunit --config phpunit.xml tests/api/Unit/FeatureFlags`
  → 40 tests, 160 assertions, OK.
- Mermaid PNGs regenerate via `npx @mermaid-js/mermaid-cli`.

`make test_featureflags`, OpenFeature PHPUnit, and ffe-dogfooding
end-to-end validation will run in CI / are validated separately by
FOLLOW-05 Steps 4–5.

* Fix OTLP endpoint resolution for the test-agent / fallback path

The SidecarOtlpMetricsTransport::resolveEndpoint() default ("http://localhost:4318/v1/metrics")
doesn't match the system-tests parametric setup, where the PHP test
client receives DD_AGENT_HOST=<test_agent_container> but no
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. The previous OtlpHttpMetricTransport
(replaced by this transport) derived the OTLP endpoint from DD_AGENT_HOST
+ port 4318. Restore that fallback so system-tests
test_php_ffe_evaluation_metric finds the test-agent OTLP intake.

Resolution order (now matches the old transport):
1. OTEL_EXPORTER_OTLP_METRICS_ENDPOINT (explicit)
2. OTEL_EXPORTER_OTLP_ENDPOINT + /v1/metrics
3. DD_AGENT_HOST + :4318/v1/metrics
4. localhost:4318/v1/metrics

* Add native send_ffe_exposures + send_ffe_metrics C+Rust bridge

The C/Rust bridge for the new native PHP functions
\DDTrace\send_ffe_exposures() and \DDTrace\send_ffe_metrics() lives on
the M2 EVP exposures PR (#3910) because that PR introduced the bridge
when refactoring the exposure transport. PR #3911 (this PR) needs the
same bridge for its OTLP metrics transport — without it the
SidecarOtlpMetricsTransport silently drops batches because
function_exists('\\DDTrace\\send_ffe_metrics') is false.

Adds the same bridge files here so the M3 branch is independently
compilable. At merge time the two PRs will conflict at the file level on
these bridge files; resolution is deduplication (the bridge is identical
in both PRs by design).

Files added/modified:
- components-rs/sidecar.h: declares ddog_sidecar_send_ffe_exposures and
  ddog_sidecar_send_ffe_metrics FFIs.
- components-rs/common.h: declares ddog_ByteSlice typedef for the
  metrics payload.
- ext/sidecar.h, ext/sidecar.c: C wrappers
  ddtrace_sidecar_send_ffe_exposures() and ddtrace_sidecar_send_ffe_metrics().
- ext/ddtrace.stub.php, ext/ddtrace_arginfo.h, ext/ddtrace.c: declares
  the native PHP functions and the PHP_FUNCTION implementations.

* Update libdatadog submodule to pick up FFE dispatch fix

Pulls in libdatadog commit `875ec8f0e` ("fix(sidecar): dispatch FFE
actions before application-entry check"). Without this fix, the
`SidecarOtlpMetricsTransport::send()` call from PHP would silently
no-op for short-lived processes: the sidecar received the
`FfeMetrics` action but dropped it because the `Entry::Occupied`
gate on the application metadata had not yet fired.

This unblocks the parametric system-test
`Test_Feature_Flag_Parametric_Evaluation_Metrics::test_php_ffe_evaluation_metric`
which exercises the full PHP -> sidecar -> OTLP-HTTP-intake path
end-to-end. Local result: 26/27 FFE-scoped parametric tests pass
(remaining failure is the EVP exposure test, which lives on the
M2 PR #3910 branch).

* docs(ffe): quote diagram titles, switch system to TD, re-render at high rez

Three fixes to the per-PR FFE diagrams:

1. **Titles were truncated to "PHP FFE 4-PR stack — current =" with the
   PR number missing.** Mermaid uses the YAML frontmatter for the title
   block, and YAML treats unquoted `#` as the start of a comment, so
   `title: PHP FFE 4-PR stack — current = #3911 (OTLP metrics)` got
   parsed as just `PHP FFE 4-PR stack — current =`. Quoting the title
   keeps the `#PR-number` portion intact.

2. **System diagrams switched from `flowchart LR` to `flowchart TD`.**
   LR forced the PHP-process / host / backend lanes into a single
   very-wide row that rendered as an unreadable horizontal strip on PR
   pages. TD stacks them vertically and keeps the per-lane subgraphs
   readable.

3. **Re-rendered at 2400×2400 with `--scale 3`** (~1800×2000 stack,
   ~3000×4500 system) instead of the default ~600px width. PR-page
   thumbnails render legibly and zoomed-in detail stays sharp.

README's regeneration recipe updated with all three knobs and a
note on why the `title:` quoting matters.

* docs(ffe): drop 'Hook seam' wording, use 'Hook layer'

* DataDogProvider: never lose the metric hook when a user calls setHooks()

`AbstractProvider::setHooks(array $hooks)` REPLACES the hook list, so
registering our `EvalMetricsHook` via `setHooks([$metricsHook])` in the
constructor would silently drop our metric emission as soon as a user
configures their own provider-level hooks.

Override `getHooks()` to always prepend the Datadog metric hook to the
caller-supplied list. The user can register their own provider hooks
freely (`$provider->setHooks($theirHooks)`) and we still record
`feature_flag.evaluations` on every OpenFeature evaluation.

Adds a unit test that constructs a DataDogProvider, calls
`$provider->setHooks([$userHook])`, then asserts
`$provider->getHooks()` returns `[EvalMetricsHook, $userHook]`.

* SidecarOtlpMetricsTransport: drop custom DD_AGENT_HOST OTLP fallback

`resolveEndpoint()` previously fell back to
`http://$DD_AGENT_HOST:4318/v1/metrics` when neither OTel env var was
set, and also peeled off `unix://` prefixes and wrapped IPv6 hosts in
brackets. That extra path was Datadog-specific and undocumented — it
existed to make the parametric system-tests pass without configuring
the standard OTel env, and to be "helpful" for callers who set
DD_AGENT_HOST but not OTEL_EXPORTER_OTLP_METRICS_ENDPOINT.

Strip it. Follow the OpenTelemetry environment-variable spec only:

  1. OTEL_EXPORTER_OTLP_METRICS_ENDPOINT — used as-is
  2. OTEL_EXPORTER_OTLP_ENDPOINT — append /v1/metrics
  3. http://localhost:4318/v1/metrics — OTel spec default

If a Datadog Agent's OTLP intake is the target, the caller sets
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT explicitly.

System-tests parametric metric test still passes: the parametric test
client now injects OTEL_EXPORTER_OTLP_METRICS_ENDPOINT pointing at the
test-agent OTLP listener (companion change in
DataDog/system-tests:leo.romanovsky/pr-g-php-ffe-scaffold). FFE-scoped
parametric: 26/27 (only the expected exposure_event failure remains —
M3 branch has no exposure code path).

* EvaluationMetricWriter: flush-on-full so long-running runtimes don't silently drop

When the series map reaches `seriesLimit` (default 1000 unique
attribute-sets), the writer was returning false and incrementing
`dropped` for every new key. In PHP-FPM/Apache that was harmless because
`register_shutdown_function` fires per request and the map empties at
the end of each request. In long-running PHP runtimes — Swoole,
RoadRunner, FrankenPHP/Octane, CLI worker loops — the shutdown function
only fires when the worker process exits, so the series map filled
once and then every new unique attribute-set was silently dropped for
the rest of the worker's lifetime (hours or days).

Flush inline when the cap is hit so the new key fits. Same fix as the
parallel commit on the M2 (PR #3910) ExposureWriter.

Test `testSeriesOverflowDropsNewSeriesButKeepsExistingSeries` renamed
and rewritten to assert the new auto-flush behavior:
`droppedCount() === 0` after three records that would have dropped one
under the old code; transport receives three batches instead of one.

Caught by Codex review on the parallel M2 commit.

* chore(ffe): remove generated stack diagrams

* fix(ffe): tidy evaluation metrics branch base

* chore(ffe): update libdatadog sidecar dependency

* Avoid FFE metric stress argument explosion

* chore(ffe): update libdatadog sidecar dependency

* chore(ffe): update libdatadog sidecar dependency

* chore(ffe): update libdatadog sidecar dependency

* chore(ffe): update libdatadog sidecar dependency

* Fix FFE metric sidecar activation

* Update FFE metrics dependency and result ABI

* Bump libdatadog metric runtime

* Honor Datadog agent config for FFE metrics

* Move FFE metric buffering into tracer

* Remove FFE evaluation RC polling

* test: add unix domain sockets test

* Fix FFE metrics OTLP endpoint handling

* Fix FFE metrics UDS test sidecar setup

* Fix FFE evaluation metrics transport

* Fix FFE metrics UDS header assertion

* Update supported config metadata for OTLP metrics

* Align OTLP endpoint metadata with registry

* fix: start sidecar for FFE evaluation metrics

* Update libdatadog submodule now that PR has merged

* fix: keep FFE config thread-local

* Move ffe_metric_buffer to tracer/

* Simplify configuration for otel metrics, make it first class

Also build the endpoint fully in rust code for simplicity

* Avoid zend_string flip-flopping

* fix: support OTEL metrics endpoint config

* docs: explain PHP FFE metric recording

* Revert "fix: support OTEL metrics endpoint config"

This reverts commit c5934af.

* Fix missing OTEL_

* Fix compilation

Signed-off-by: Bob Weinand <[email protected]>

* Fix otel zai config

Signed-off-by: Bob Weinand <[email protected]>

* Fix tests

---------

Signed-off-by: Bob Weinand <[email protected]>
Co-authored-by: Levi Morrison <[email protected]>
Co-authored-by: Bob Weinand <[email protected]>
* Fix some span stats assumptions

* Properly handle version propagation

Signed-off-by: Bob Weinand <[email protected]>

* Fix tests

---------

Signed-off-by: Bob Weinand <[email protected]>
* Fix RC notifications in forks

* Fix double runtime-id generation

Signed-off-by: Bob Weinand <[email protected]>

* Add pcntl requirement

---------

Signed-off-by: Bob Weinand <[email protected]>
…3925)

* feat(appsec): collect security-testing headers on HTTP entry spans (APPSEC-62412)

Unconditionally collect x-datadog-endpoint-scan and x-datadog-security-test
HTTP request headers as http.request.headers.* tags on the service entry span,
independent of DD_TRACE_HEADER_TAGS and AppSec enablement. Also forwarded to
the inferred proxy span. Covers all PHP SAPIs (Apache/fpm, CLI, FrankenPHP,
RoadRunner) via the C extension and Swoole via its PHP integration.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Address review: move header collection to tags.c, drop CHANGELOG

- Move x-datadog-endpoint-scan and x-datadog-security-test collection
  to appsec/src/extension/tags.c via _relevant_basic_headers, following
  the existing pattern for http.request.headers.* tags
- Remove duplicate logic from ext/serializer.c (keep transfer_meta_data
  calls for inferred proxy span forwarding)
- Replace tracer phpt tests with an AppSec extension test using
  add_all/add_basic_ancillary_tags
- Remove CHANGELOG entry (added at release time)

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Address review comments on security-testing headers

- Add DD_TAG_HTTP_REQH_ENDPOINT_SCAN / DD_TAG_HTTP_REQH_SECURITY_TEST
  constants so tag-name strings are not duplicated across the sec_headers
  table and the transfer_meta_data calls
- Add DD_UNCONDITIONAL_SERVER_HEADER macro to reduce verbosity of the
  struct initializer, following reviewer suggestion
- Restore serializer.c collection (unconditional, per RFC) alongside
  tags.c which covers the AppSec-loaded path
- Remove CHANGELOG entry (added at release time per reviewer)
- Remove --GET-- from phpt tests (unnecessary)
- Add AppSec extension test using add_all/add_basic_ancillary_tags
- Update ancillary_tags.phpt canonical test to include the two new headers

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Remove redundant AppSec tags.c collection

Since the headers are already collected unconditionally by
ext/serializer.c (always loaded), adding them to AppSec's
_relevant_basic_headers is unnecessary. Remove both the
tags.c addition and its dedicated test.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Revert ancillary_tags.phpt to master state

The x-datadog headers were removed from tags.c _relevant_basic_headers,
so ancillary_tags.phpt no longer expects them in the output.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Fix stale comment in serializer.c

Remove reference to appsec/src/extension/tags.c that was reverted.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Inject security-testing headers into DD_TRACE_HEADER_TAGS at startup

Instead of two dedicated zend_hash_str_find calls per request, inject
x-datadog-endpoint-scan and x-datadog-security-test into the
DD_TRACE_HEADER_TAGS hash table at module startup. They then ride the
existing HTTP_* header loop in dd_add_header_to_meta with zero extra
per-request overhead, and also scale automatically to any future SAPI
integration that honours DD_TRACE_HEADER_TAGS.

- Remove ddtrace_alter_DD_TRACE_HEADER_TAGS from INI_CHANGE_DYNAMIC_CONFIG
  and implement it manually so it re-injects the headers after any
  runtime config change (ini_set / remote config)
- Remove the sec_headers block and DD_UNCONDITIONAL_SERVER_HEADER macro
  from serializer.c; keep DD_TAG_HTTP_REQH_* constants and the
  transfer_meta_data calls for the inferred proxy span
- Remove the explicit isset checks from SwooleIntegration.php; the
  existing DD_TRACE_HEADER_TAGS loop now covers the Swoole path too

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Use a custom parser for DD_TRACE_HEADER_TAGS to inject security headers

The memoized HashTable is made immutable (IS_ARRAY_IMMUTABLE, refcount > 1)
by zai_config_intern_zval immediately after decoding, so any post-decode
modification triggers HT_ASSERT_RC1. The correct hook is the custom parser,
which runs with a freshly allocated HashTable at refcount == 1 -- before
interning -- so zend_hash_str_add is unconditionally safe.

- Change DD_TRACE_HEADER_TAGS type to CUSTOM(MAP) with dd_parse_header_tags
  as the .parser; the parser calls the standard SET_OR_MAP_LOWERCASE decoder
  then adds x-datadog-endpoint-scan and x-datadog-security-test (APPSEC-62412)
- ddtrace_alter_DD_TRACE_HEADER_TAGS now only handles remote config
  notification; security headers are already in new_value from the parser
- Remove the sec_headers block from serializer.c and the explicit isset
  checks from SwooleIntegration.php; the existing header_tags loop
  now covers both paths automatically

Cost: two O(1) zend_hash_str_add calls per decode (once at startup, once
per ini_set). Zero overhead at every regular RINIT.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Fix unit tests for DD_TRACE_HEADER_TAGS to include security headers

dd_parse_header_tags now unconditionally adds x-datadog-endpoint-scan
and x-datadog-security-test to every decoded value, so tests asserting
exact array contents must include them.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Fix ZVAL_EMPTY_STRING vs ZVAL_EMPTY_PSTRING in dd_parse_header_tags

When persistent=true, the HashTable uses ZVAL_INTERNAL_PTR_DTOR whose
destructor path can call pefree on string values. Using ZVAL_EMPTY_STRING
(which resolves to CG(empty_string), a non-persistent interned string)
causes a segfault on PHP 7.0/7.1 when the persistent HT is destroyed.

Match the pattern already used in zai/config/config_decode.c lines
160/189: ZVAL_EMPTY_PSTRING for persistent HTs, ZVAL_EMPTY_STRING for
non-persistent ones.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Revert to ZVAL_EMPTY_STRING — ZVAL_EMPTY_PSTRING causes double-free

ZVAL_EMPTY_PSTRING calls zend_string_init("", 0, 1) which allocates a
fresh non-interned persistent string with refcount=1. When shared across
two HT buckets via ZVAL_COPY_VALUE (no refcount increment), the
ZVAL_INTERNAL_PTR_DTOR destructor frees it on the first bucket and then
crashes on the second (double-free / zend_string.c assertion).

ZVAL_EMPTY_STRING uses zend_empty_string (IS_INTERNED_STRING_EX type,
not IS_REFCOUNTED), so ZVAL_INTERNAL_PTR_DTOR sees Z_REFCOUNTED=false
and skips freeing entirely — safe to share across multiple buckets on
all PHP versions (7.0–8.x).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Fix dd_parse_header_tags: prepend headers to value string for safe interning

Directly calling zend_hash_str_add on the decoded HashTable triggered
zend_new_interned_string_permanent assertions because ZAI calls
zend_interned_strings_switch_storage(0) (permanent mode) before
zai_config_intern_zval, and zend_new_interned_string_permanent requires
the input to have IS_STR_PERSISTENT in a specific internal state.

Instead, prepend "x-datadog-endpoint-scan,x-datadog-security-test" to
the raw value string and let zai_config_decode_value / SET_OR_MAP_LOWERCASE
handle key creation and interning internally. ZAI's decoder handles all
PHP versions and thread modes (including PHP 7.0/7.1 ZTS) correctly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Append security headers instead of prepend in dd_parse_header_tags

Prepending put x-datadog-endpoint-scan and x-datadog-security-test
before user-configured headers, breaking ConfigurationTest.php
assertions that expect user headers first. Appending keeps user-
configured headers at the head of the iteration order.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Clarify comments in ddtrace_alter_DD_TRACE_HEADER_TAGS

- Make clear that new_value (decoded) has security headers but new_str
  (raw user INI string passed to RC) intentionally does not
- Shorten the datadog_config_first_rinit comment

Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>

* Avoid string manipulation stuff

Signed-off-by: Bob Weinand <[email protected]>

* Simplify dynamic config handling

Signed-off-by: Bob Weinand <[email protected]>

* Simplify dynamic config handling

Signed-off-by: Bob Weinand <[email protected]>

---------

Signed-off-by: Bob Weinand <[email protected]>
Co-authored-by: Claude Sonnet 4.6 (1M context) <[email protected]>
Co-authored-by: Bob Weinand <[email protected]>
* Add dynamic_service DBM propagation mode

* Fix use-after-free in set_container_tags_hash test function

* revert bad change

* Properly fix container tag replacing

Signed-off-by: Bob Weinand <[email protected]>

---------

Signed-off-by: Bob Weinand <[email protected]>
Co-authored-by: Bob Weinand <[email protected]>
Signed-off-by: Bob Weinand <[email protected]>
This became quite unergonomic with the downcast, but looks like there was a reason for the boxing.

Otherwise ShmLimiter references weird addresses.

Signed-off-by: Bob Weinand <[email protected]>
* test(tracing): SpanStack::$active unset corruption

* fix(tracing): forbid unsetting 'active' member

* fix(tracing): PHP 7 compat zend_std_unset_property

* fix(tracing): guard active and parent consistently

* fix: PHP 7.0 compat

Co-authored-by: Bob Weinand <[email protected]>

---------

Co-authored-by: Bob Weinand <[email protected]>
* fix(tracing): sandbox should save/restore jit_trace_num

* test(tracing): sandbox regression for jit_trace_num
- Link extension against pthreads (Threads::Threads) to resolve
  undefined symbol: pthread_once on some platforms
- Fix CMake deprecation warning: BYPRODUCT -> BYPRODUCTS
- Add DD_APPSEC_ENABLED=1 env to user_tracking set_user phpt tests
- Update http_client_ip_generation_01 response format
- Update rinit_fail_malformed_resp expected output: malformed responses
  now return dd_network, closing the connection (failed_count=1)
@cataphract
cataphract requested review from a team as code owners June 9, 2026 12:43
@cataphract
cataphract requested review from greghuels, sameerank and tabgok and removed request for a team June 9, 2026 12:43
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

If you need to update snapshots, please refer to CONTRIBUTING.md

cataphract and others added 2 commits June 9, 2026 14:04
- PG(last_error_message) is char* in all PHP 7.x; became zend_string*
  only in PHP 8.0. Fix the version guard from < 70100 to < 80000.
- zend_set_memory_limit() returns void in PHP 8.0 (changed back to
  bool in PHP 8.1). Guard the return-value check accordingly.
- Expand single-line function bodies to satisfy clang-format.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
CI uses clang-format-20 which does not allow single-line function bodies
even when AllowShortBlocksOnASingleLine is true. Expand three offending
functions to multi-line form.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
@cataphract
cataphract merged commit f2d6771 into glopes/request-exec-outside-req Jun 9, 2026
369 of 1776 checks passed
@cataphract
cataphract deleted the glopes/rshutdown-bailout branch June 9, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants