Skip to content

appsec: fix bailouts during rshutdown#3941

Open
cataphract wants to merge 2 commits into
glopes/request-exec-outside-reqfrom
glopes/rshutdown-bailout
Open

appsec: fix bailouts during rshutdown#3941
cataphract wants to merge 2 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 cataphract requested a review from a team as a code owner May 29, 2026 16:50
Copy link
Copy Markdown

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

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: fd5b17a39c

ℹ️ 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 on lines +6 to +7
import com.datadog.appsec.php.docker.LogFile
import com.datadog.appsec.php.docker.PhpFpm
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Add the missing Docker helper classes

These new imports reference com.datadog.appsec.php.docker.LogFile and PhpFpm, but the commit does not add either class; a repo-wide rg shows the docker test package only contains the existing helpers (AppSecContainer, FailOnUnmatchedTraces, InspectContainerHelper, etc.). As a result, the appsec integration test sources that now import/use these helpers (NginxFpmTests, RoadRunnerTests, ZtsGshutdownTests, and the FPM sampling traits/classes) will fail to compile before any tests run.

Useful? React with 👍 / 👎.

@datadog-official
Copy link
Copy Markdown

datadog-official Bot commented May 29, 2026

Pipelines  Tests

Fix all issues with BitsAI

⚠️ Warnings

🚦 87 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | appsec code coverage   View in Datadog   GitLab

🔧 Fix in code (Fix with Cursor). 6 failed tests due to warnings about accessing array offsets on null in http_client_ip_generation_01.php and user_tracking_do_nothing_from_set_user.php.

DataDog/apm-reliability/dd-trace-php | appsec integration tests (helper-rust): [test7.4-release]   View in Datadog   GitLab

🔧 Fix in code (Fix with Cursor). Compilation error in request_lifecycle.c:301:17: request for member 'val' in something not a structure or union.

DataDog/apm-reliability/dd-trace-php | appsec integration tests: [test7.0-release-zts]   View in Datadog   GitLab

🔧 Fix in code (Fix with Cursor). Compilation error in request_lifecycle.c:301: request for member 'val' in something not a structure or union.

View all 87 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

Useful? React with 👍 / 👎

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

@cataphract cataphract force-pushed the glopes/rshutdown-bailout branch from fd5b17a to d0d93de Compare May 29, 2026 16:59
cataphract and others added 2 commits May 29, 2026 18:16
After a successful command exchange (e.g. request_init), the helper
has entered its inner request loop and is waiting for the matching
request_shutdown. If _imsg_destroy() reports a msgpack framing error
at that point, the code was returning dd_error.

dd_error does not trigger dd_helper_close_conn(), so the connection
stays open while the helper is blocked in the inner loop. On the next
request the extension sends request_init into the inner loop, which
the Rust helper treats as an unexpected command and aborts.

Return dd_network instead, which causes the caller to close and
abandon the connection. This keeps helper and extension state in sync
even in the presence of malformed trailers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@cataphract cataphract force-pushed the glopes/rshutdown-bailout branch from d0d93de to 9cdff75 Compare May 29, 2026 17:18
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented May 29, 2026

Benchmarks [ appsec ]

Benchmark execution time: 2026-05-29 17:56:48

Comparing candidate commit 9cdff75 in PR branch glopes/rshutdown-bailout with baseline commit 4d74189 in branch glopes/request-exec-outside-req.

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

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.

1 participant