Skip to content

test(appsec): bound the django test app flush and fix its suitespec gap DD_U55XLN - #19700

Open
christophe-papazian wants to merge 3 commits into
mainfrom
christophe-papazian/django-shutdown-gevent-hang
Open

test(appsec): bound the django test app flush and fix its suitespec gap DD_U55XLN#19700
christophe-papazian wants to merge 3 commits into
mainfrom
christophe-papazian/django-shutdown-gevent-hang

Conversation

@christophe-papazian

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

Copy link
Copy Markdown
Contributor

APPSEC-69623

Two independent changes, neither claiming to be the fix for the flake.

Bounds the django test app's trace flush. /shutdown called tracer.shutdown() with no timeout, documented as "block until flushing has successfully completed". An unbounded wait in a fixture teardown holds the job to the suite timeout rather than failing one test, and a lost gevent notification can only park an untimed wait forever — a timed one still wakes on its hub timer. tests/appsec/app.py already passed a timeout, so django was the outlier.

Adds tests/appsec/app.py to the four suites that serve it. It matched only the threats suites, so changes to it ran none of the integration tests. Same gap #19674 closed for appsec_utils.py.

The actual fix for U55XLN is upstream: gevent 26.8.0 ships gevent/gevent#2199, and our appsec django venvs are pinned to 25.9.1. A lockfile refresh is coming separately. Thanks @florentinl for both catches — the flask 10 → 5 change is reverted, since it only reduced the flush budget for suites that do assert on test-agent payloads.

DD_U55XLN

The /shutdown endpoints called tracer.shutdown() with no timeout (django) or
with the same 10s the client uses (flask). Under a gunicorn gevent worker the
flush can block the hub the writer needs, so the request read-timed-out and the
test failed with the server still alive and the port still bound.

Bound both flushes to 5s, below the client's 10s timeout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@christophe-papazian christophe-papazian added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 14, 2026
@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 14, 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.

tests/appsec/integrations/django_tests/django_app/urls.py               @DataDog/asm-python
tests/appsec/suitespec.yml                                              @DataDog/asm-python

@cit-pr-commenter-54b7da

cit-pr-commenter-54b7da Bot commented Aug 14, 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 14, 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.collector.stack -×-> ddtrace.trace  (product:profiling -> product:tracing, score=132)
ddtrace.llmobs._integrations.bedrock_agents -×-> ddtrace.trace  (product:llmobs -> product:tracing, score=132)
ddtrace.profiling.scheduler -×-> ddtrace.trace  (product:profiling -> product:tracing, score=132)
ddtrace.llmobs._integrations.litellm -×-> ddtrace.trace  (product:llmobs -> 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

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>
@christophe-papazian
christophe-papazian marked this pull request as ready for review August 14, 2026 13:26
@christophe-papazian
christophe-papazian requested a review from a team as a code owner August 14, 2026 13:26
@florentinl

florentinl commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Could you explain how it fixes the flaky test ?
Intuitively, reaching the timeout on a tracer shutdown means things have not been properly flushed and the test will fail because it is missing its expected signals.
It looks more like a fail early mechanism than a fix to me.

@florentinl

florentinl commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Maybe unrelated (and with some shameless self-promotion 😁): if this is a gevent deadlock issue, could it have been fixed upstream by this PR? It’s included in the latest gevent release, 26.8.0. Maybe we could try regenerating the Riot requirements against that version.

Reverts the flask app.py timeout change: its 10s already matched the caller's,
and cutting it to 5s reduced the flush budget for suites that do assert on
test-agent payloads, with no evidence it helped anything.

Keeps the django bound, rejustified. It is not the fix for the flake -- gevent
26.8.0 is, via gevent/gevent#2199 -- but an unbounded wait in a fixture teardown
holds the job to the suite timeout instead of failing one test, and a lost
gevent notification can only park an untimed wait forever.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@christophe-papazian christophe-papazian changed the title test(appsec): bound tracer shutdown in test app endpoints DD_U55XLN test(appsec): bound the django test app flush and fix its suitespec gap DD_U55XLN Aug 14, 2026
@christophe-papazian

Copy link
Copy Markdown
Contributor Author

You're right on both counts, thanks — I've narrowed the PR.

On "fail early, not a fix": agreed, and the flask half is reverted. To answer the "shouldn't it fail for missing signals?" part specifically: /shutdown runs after the yield in appsec_utils.py:439-448, via client.get_ignored(...), and the server is SIGTERM'd immediately afterwards either way — so for this suite the flush isn't load-bearing and its only influence on pass/fail is that a raised exception becomes a teardown AssertionError. But that argues my change was harmless, not that it was a fix, which is your point. And tests/appsec/app.py is the default app for other suites that do assert on test-agent payloads, so cutting its budget from 10s to 5s was a real risk for no measured benefit. Reverted.

What I've kept is only the django side, and only as hygiene: tracer.shutdown() there had no timeout, i.e. "block until flushing has successfully completed". An unbounded wait in a fixture teardown holds the job to the 30m suite timeout instead of failing one test, and tests/appsec/app.py already passed a timeout, so django was the odd one out.

On gevent#2199 — I think you've found the actual root cause, and the shameless self-promotion is fully earned. It lines up unusually well:

  • 26.8.0's changelog: "Fixed a semaphore acquired and released by a hubless native thread failing to wake greenlets waiting on the semaphore's owning hub."
  • Upstream says the practical trigger came from dd-trace-py via logging: under monkey-patching, handler locks are gevent semaphores.
  • ddtrace/internal/threads.py:12-17 deliberately grabs the original C locks "in case they get patched by monkey-patching libraries like gevent", and PeriodicThread is the C one — so our writer threads are genuine hubless OS threads, and they log.
  • The failing job ran -k gevent --log-level debug, i.e. maximum contention on that handler lock.
  • The diagnostics said port_still_bound=True pid=4711 exit_code=None — server alive and healthy, only the /shutdown greenlet stuck. A parked greenlet, not a dead worker.

And our pins are behind: every appsec django + gunicorn + gevent lockfile is on gevent==25.9.1, and the two flask_testagent ones on 26.4.0.

I've regenerated them via scripts/regenerate-riot-latest.sh to confirm it resolves: 33 venvs move to 26.8.0, and the 5 py3.9 ones land on 26.7.0 (26.8.0 requires >=3.10, so that happens automatically). Two things mean it wants its own PR rather than riding along here:

  1. check_lockfile_cooldown.py currently fails — sqlparse==0.6.0 is 19.4h old against the 2-day cooldown — so it can't land until that ages out.
  2. The recompile is not gevent-only. These lockfiles are stale enough that it also brings gunicorn 23.0.0 → 26.0.0 (major, and it's the server under test), django 6.0 → 6.1, and pytest → 9.1.1. Worth landing where a red pipeline is attributable.

So: this PR stays as teardown hygiene plus the suitespec gap, and the gevent refresh follows separately with DD_U55XLN on it so the quarantined test gets its 21 runs against the real fix.

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