Skip to content

test(appsec): make server startup failures diagnosable and port reuse safe - #19674

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits into
mainfrom
christophe-papazian/appsec-server-startup-flake
Aug 14, 2026
Merged

test(appsec): make server startup failures diagnosable and port reuse safe#19674
gh-worker-dd-mergequeue-cf854d[bot] merged 2 commits into
mainfrom
christophe-papazian/appsec-server-startup-flake

Conversation

@christophe-papazian

@christophe-papazian christophe-papazian commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

APPSEC-69623

All 13 quarantined tests here fail in appsec_application_server, not in their own bodies. The recorded error is the "Server failed to start" assertion, and the runs with a duration cluster at 22.0–22.6s against a ~13s startup budget.

The failures are undebuggable. The assertion interpolated getattr(server_process, "stdout", None)None for a multiprocessing.Process, a stream rather than text for a Popen — so every one reports a literal None. It now reports what is not already in the captured output: exit code (None = too slow, non-zero = died), whether the port is still taken, and the command.

Port 8050 is shared by 31 tests, these suites run serially, and teardown is best effort (SIGTERM to the process group, join(timeout=5), each step in except/pass). A gunicorn worker outliving that leaves the port taken and the next test can't bind. Now waits for the port both before starting and after tearing down.

The suites never ran. The first push was green with zero test events for any of the 13 tests: tests/appsec/appsec_utils.py matched 15 suites but none of the appsec_integrations_* ones that import it, because @appsec covers ddtrace/appsec/* source only and each suite lists just its own test directory. The second commit adds the file to the six suites whose tests import it (grep-verified: flask_tests 7 files, fastapi_tests 2, django_tests 1, plus iast_packages and iast_tdd_propagation). It now matches 21 suites; suitespec-check passes both gates.

Notes:

  • The port wait tests whether the port can be bound, not whether it accepts connections. connect() reports a port free once a bound server's listen backlog fills — exactly the wedged state worth catching — and opens real connections to a live server. My first version did use connect() and wrongly reported "released" after 0.3s against a still-bound socket.
  • Warns rather than raises when the port never frees; raising in teardown would turn one leak into a failure across every appsec server suite.
  • No per-worker port offsets: these suites have no -n in the riotfile, so the collision is sequential, not concurrent.
  • The newly triggered suites also hold quarantined tests this branch does not fix — the SCA reachability pair (fixed in test(iast): assert SCA reachability on the event that recorded the call #19671) and test_django_insecure_cookie_secure (an index_aspect bug). Deliberately left unkeyed rather than un-quarantined on a commit that would not fix them.

Blast radius: appsec_application_server backs all five server context managers, and the suitespec change widens what CI runs on such edits — both intended.

🤖 Generated with Claude Code

@christophe-papazian christophe-papazian added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 13, 2026
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codeowners resolved as

Resolved from the full PR diff against main using the target branch CODEOWNERS file.
CODEOWNERS team requests not listed below are not required by the current file set.

No remaining files require a CODEOWNERS review.

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 13, 2026

Copy link
Copy Markdown

Circular import analysis

⚠️ Existing circular imports

There are 5 circular imports that already exist on the base branch and have not been changed by this PR.

ddtrace.contrib.internal.pytorch._distributed -> ddtrace.contrib.internal.pytorch._rank_root -> ddtrace.contrib.internal.pytorch._distributed
ddtrace.contrib.internal.django.patch -> ddtrace.contrib.internal.django.response -> ddtrace.contrib.internal.django.patch
ddtrace.llmobs -> ddtrace.llmobs._evaluators -> ddtrace.llmobs._evaluators.format -> ddtrace.llmobs._experiment -> ddtrace.llmobs
ddtrace.errortracking._handled_exceptions.bytecode_injector -> ddtrace.errortracking._handled_exceptions.callbacks -> ddtrace.errortracking._handled_exceptions.collector -> ddtrace.errortracking._handled_exceptions.bytecode_reporting -> ddtrace.errortracking._handled_exceptions.bytecode_injector
ddtrace.appsec._asm_request_context -> ddtrace.appsec._iast._iast_request_context_base -> ddtrace.appsec._iast._iast_env -> ddtrace.appsec._iast.reporter -> ddtrace.appsec._exploit_prevention.stack_traces -> ddtrace.appsec._asm_request_context

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 13, 2026

Copy link
Copy Markdown

Dependency direction analysis

⚠️ Existing dependency direction violations

There are 255 dependency direction violations that already exist on the base branch and have not been changed by this PR.

Show existing violations (showing 5 of 255 highest severity)
ddtrace.internal.tracemethods -×-> ddtrace.trace  (internal-core -> product:tracing, score=134)
ddtrace.profiling.scheduler -×-> ddtrace.trace  (product:profiling -> product:tracing, score=132)
ddtrace.llmobs._integrations.anthropic -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=132)
ddtrace.llmobs._integrations.crewai -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=132)
ddtrace.profiling.collector.pytorch -×-> ddtrace.trace  (product:profiling -> product:tracing, score=132)

To see all violations, download the layers-base.json and layers-pr.json artifacts from this CI job and run:

uv run --script scripts/import-analysis/layers.py compare layers-base.json layers-pr.json

Comment thread tests/appsec/appsec_utils.py Dismissed
@datadog-official

datadog-official Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

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

christophe-papazian and others added 2 commits August 13, 2026 16:52
… safe DD_KUBWIR DD_FQAL2Q DD_F20P7M DD_WQNXQ5 DD_D07T24 DD_6TE3BU DD_U55XLN DD_4FSP2S DD_G9FTB2 DD_W2G1F6 DD_3LVVS3 DD_UJ438X DD_1AVE6M

Every one of these quarantined tests fails in appsec_application_server, not in its
own body: the recorded error is the "Server failed to start" assertion, and the
runs that recorded a duration cluster at 22.0-22.6s against a ~13s startup budget.

Two problems:

- The assertion interpolated getattr(server_process, "stdout", None), which is
  None for a multiprocessing.Process and a stream rather than text for a Popen,
  so all 13 report a literal "None" and cannot be debugged. Report instead the
  facts that are not already in the captured output: the exit code (None means
  too slow, non-zero means it died), whether the port is still taken, and the
  command.
- 31 tests share port 8050 and these suites run serially, while teardown is best
  effort: SIGTERM to the process group then join(timeout=5), each step wrapped in
  except/pass. A gunicorn worker outliving that leaves the port taken and the next
  test cannot bind. Wait for the port before starting and after tearing down.

The wait tests whether the port can be bound rather than whether it accepts
connections: connect() reports a port as free once a bound server's listen backlog
fills, which is exactly the wedged state worth detecting, and it opens real
connections to a live server.

Warn rather than raise when the port never frees; raising in teardown would turn a
leak into a failure of every appsec server suite.

APPSEC-69623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nges DD_KUBWIR DD_FQAL2Q DD_F20P7M DD_WQNXQ5 DD_D07T24 DD_6TE3BU DD_U55XLN DD_4FSP2S DD_G9FTB2 DD_W2G1F6 DD_3LVVS3 DD_UJ438X DD_1AVE6M

The previous commit changed tests/appsec/appsec_utils.py and none of the 13 tests
it targets ran: the file matched 15 suites but not one of the appsec_integrations_*
suites that import it, because @appsec covers ddtrace/appsec/* source only and each
suite lists just its own test directory.

Add the file to the paths of the six suites whose tests import it, so a change to
the shared server helper is exercised by the suites that depend on it. It now
matches 21 suites instead of 15.

The keys are the 13 quarantined tests this branch targets. The suites now triggered
also hold quarantined tests this branch does not fix (the SCA reachability pair,
fixed in another PR, and test_django_insecure_cookie_secure, an index_aspect bug),
so those are deliberately left unkeyed rather than un-quarantined on a commit that
would not fix them.

APPSEC-69623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@christophe-papazian
christophe-papazian force-pushed the christophe-papazian/appsec-server-startup-flake branch from 075997e to a487a79 Compare August 13, 2026 14:52
@christophe-papazian
christophe-papazian marked this pull request as ready for review August 13, 2026 15:16
@christophe-papazian
christophe-papazian requested a review from a team as a code owner August 13, 2026 15:16
@christophe-papazian
christophe-papazian marked this pull request as draft August 13, 2026 15:29
@christophe-papazian
christophe-papazian marked this pull request as ready for review August 14, 2026 08:18
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit a24b7b3 into main Aug 14, 2026
485 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the christophe-papazian/appsec-server-startup-flake branch August 14, 2026 09:02
christophe-papazian added a commit that referenced this pull request Aug 14, 2026
tests/appsec/app.py matched only the threats suites, none of which run the
integration tests that actually serve it, so the shutdown change in the previous
commit shipped without any of them running. Same gap that #19674 closed for
appsec_utils.py. 15 -> 19 matched suites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/no-changelog A changelog entry is not required for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants