Repository navigation
Fix flaky exposure shutdown flush test - #9396
Merged
Merged
Conversation
Co-authored-by: btthomas <9452950+btthomas@users.noreply.github.com>
Contributor
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
leoromanovsky
approved these changes
Oct 9, 2026
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
The test retains its checks that Dispose waits for the final blocked send and flushes the queued exposure. The longer allowance accommodates delayed continuations without changing production shutdown behavior.
🤖 Bits Code Review · Commit 08f3d97
btthomas
enabled auto-merge (squash)
October 9, 2026 19:20
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9396) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-10-09 20:31:20 Comparing candidate commit 08f3d97 in PR branch Found 0 performance improvements and 11 performance regressions! Performance is the same for 61 metrics, 0 unstable metrics, 74 known flaky benchmarks, 52 flaky benchmarks without significant changes.
|
btthomas
added a commit
that referenced
this pull request
Oct 13, 2026
<!-- dd-meta
{"pullId":"868cb8f4-5fff-4bc3-8350-e1f82aae9dc3","source":"chat","resourceId":"a2d9735b-f842-47ff-bbc6-6c14a63ead5c","workflowId":"5cfb2560-8f04-4df3-8daf-00ca5ee3a178","codeChangeId":"5cfb2560-8f04-4df3-8daf-00ca5ee3a178","sourceType":"code_channel"}
-->
## Summary of changes
- Make the exposure shutdown flush test use its send gates to control
disposal completion.
- Run its synchronous Dispose call on a dedicated thread so it does not
occupy a worker needed by the send loop or test continuations.
## Reason for change
DisposeFlushesEventsQueuedWhileAnEarlierBatchIsSending assumes Dispose
remains blocked until the test releases the final request. However, the
test configured a finite shutdown timeout. Dispose intentionally stops
waiting when that deadline expires, even if a request remains blocked.
If thread-pool scheduling delays the test's continuation, its assertion
observes a completed Dispose task after the deadline and incorrectly
reports a flush failure. The approximately five-second failed runs match
the original five-second shutdown allowance.
Current master already contains #9396, which raised that allowance to
one minute. That reduces the chance of failure but retains the same
race. The send loop already awaits the final flush, and Dispose waits
for that loop until the configured deadline; the separate
bounded-shutdown test explicitly requires timeout expiry to release
shutdown. This is a test synchronization issue.
## Implementation details
- Configure Timeout.InfiniteTimeSpan for this flush-ordering scenario so
only the controlled request gates can release disposal. The finite waits
that detect missing send/disposal progress and the finally block that
releases both gates remain in place.
- Use TaskCreationOptions.LongRunning with TaskScheduler.Default for the
blocking Dispose call, leaving thread-pool workers available for
asynchronous sends and assertions.
- Preserve all existing assertions, including waiting for the final send
and checking the second exposure's payload. Production shutdown behavior
and the separate bounded-wait test are unchanged.
## Test coverage
- A package-free .NET 8 harness linked the actual ExposureApi, EVP
transport, cache, exposure models, request factory, and ExposureApiTests
sources. The corrected test method passed 100 consecutive executions,
including its payload assertions. The other test methods also passed:
bounded shutdown, faulted/canceled send loops, and enqueue after
disposal.
- In the same harness, withholding the test's final-send observation
until the deadline reproduced Dispose completing while the final request
remained blocked with both the historical five-second allowance and
current master's one-minute allowance. Gate-controlled shutdown remained
blocked across an observation delay exceeding five seconds and completed
after releasing the final send.
- A negative control that removed Dispose's wait was rejected by the
corrected test's existing final-send assertion.
- This isolated harness uses doubles for external dependencies,
JSON/assertion adapters, and test attributes; it is not a full
repository xUnit run. The focused dotnet test command could not start
because global.json requires SDK 11.0.100-rc.1.26425.128 and only SDK
8.0.412 is installed. Validation on the affected .NET Core 3.0/3.1 CI
jobs remains required.
## Other details
<!-- Fixes #{issue} -->
<!-- ⚠️ Note:
Where possible, please obtain 2 approvals prior to merging. Unless
CODEOWNERS specifies otherwise, for external teams it is typically best
to have one review from a team member, and one review from apm-dotnet.
Trivial changes do not require 2 reviews.
MergeQueue is NOT enabled in this repository. If you have write access
to the repo, the PR has 1-2 approvals (see above), and all of the
required checks have passed, you can use the Squash and Merge button to
merge the PR. If you don't have write access, or you need help, reach
out in the #apm-dotnet channel in Slack.
-->
---
PR by Bits - [View session in
Datadog](https://app.datadoghq.com/code/a2d9735b-f842-47ff-bbc6-6c14a63ead5c)
Comment @DataDog to request changes
Co-authored-by: datadog-bits <263423550+datadog-bits@users.noreply.github.com>
Co-authored-by: btthomas <9452950+btthomas@users.noreply.github.com>
dudikeleti
added a commit
that referenced
this pull request
Oct 30, 2026
## Summary of changes Adds Dynamic Instrumentation rules to the `/sdk-review` lens files, so the guardrails in `DebuggerSafetyBoundaries.md` are checked before every push: - `Security-And-Safety.md`: section 3 is now "Dynamic Instrumentation". SEC-3.1 also covers expression-tree property access and static property getters. New rules SEC-3.2 to SEC-3.6 cover runtime-type checks, dictionary keys and compiler-generated names, values computed from redacted values, customer data in evaluation errors and logs, and `Expression.Equal` null checks. - `Performance.md`: new section 5 with PERF-5.1 and PERF-5.2 for per-hit locks and cache hits, and failure output on every hit. - `Correctness.md`: COR-6.4 now says which time bounds are acceptable in tests, flags raising a timeout to fix a flaky test, and limits queued markers to single-consumer queues. This replaces #9414. ## Reason for change The guardrails added to `DebuggerSafetyBoundaries.md` in #9562 are rules we want enforced, and `/sdk-review` is what runs before every push. Keeping them in the lens files instead of `AGENTS.md` keeps `AGENTS.md` small. Each new Dynamic Instrumentation rule comes from a bug fixed in a recent PR, and the COR-6.4 change comes from #9414: | Rule | Fixed in | | --- | --- | | SEC-3.1 (update) | #8878 | | SEC-3.2 runtime type | #9483, #9541, #9547 | | SEC-3.3 dictionary keys and source names | #9536, #9539, #9548 | | SEC-3.4 computed from redacted values | #9485, #9537, #9547 | | SEC-3.5 errors and logs | #9559 | | SEC-3.6 `Expression.ReferenceEqual` | #9541 | | PERF-5.1 per-hit locks, cache hits | #9538 | | PERF-5.2 failure output on every hit | #9536, #9537, #9559 | | COR-6.4 (update) | from #9414, not a bug fix | ## Implementation details SEC-3.1 keeps its ID; the section heading changes from "Customer-process safety" to "Dynamic Instrumentation". The Performance "Don't flag" entry about uncontended per-request locks now points to PERF-5.1, so the two don't conflict for locks shared by every thread that hits a probe. SEC-3.1 still links to `DebuggerSafetyBoundaries.md` for the allowed execution policy. None of these rules depend on #9562, but #9562 points the guide's review checklist to them, so merge this PR first. ## Test coverage Docs only. As in #9472, I ran the rules against commits that introduced the issues. Each run was one independent lens reviewer reading the frozen commit and only its lens file. No reviewer was told which rule was being tested. | Rule | Commit reviewed | Result | | --- | --- | --- | | SEC-3.2 | #8879 | Caught: `ShouldRedactDumpValue(null, expression.Type)` in `ProbeExpressionParser.Dump.cs` decides redaction from the declared type | | SEC-3.4 | #8621 (product code) | Caught: `len(ref password)` and `eq(ref password, "x")` capture values computed from the redaction placeholder | | SEC-3.5 | #8621 (product code) | Caught: capture expression errors send `e.Message` in `evaluationErrors` | | SEC-3.6 | #5543 (product code) | Caught: the `Expression.Equal(expression, Expression.Constant(null))` null check | | PERF-5.1 | #6292 | Caught: every redaction cache hit takes a `ReaderWriterLockSlim` read lock and the LFU policy's write lock, and allocates | | PERF-5.1 | #9538 | Correctly not flagged (the lock-free fix) | | PERF-5.2 | #8725 | Missed: the uncapped metric probe path isn't in that diff | | PERF-5.2 | #8621 (product code) | Flagged a different per-hit failure log in the same block (`Evaluation result should not be null`), not the `Log.Error` in the catch | | COR-6.4 | #9396 | Caught: the flush timeout raised from 5s to 1 minute to fix a flaky test | | COR-6.4 | #9074 | Correctly not flagged (`TaskCompletionSource` with a 30s guard) | Not tested: SEC-3.1's new expression-tree clause and SEC-3.3. Reviewers did point out SEC-3.3's literal-only dictionary key check in code that predates the commits under review. ## Other details I kept only rules a reviewer can match in a diff and that a PR shows were needed. An earlier version had a PERF-5.3 rule for `EvaluationBudget` checks. I dropped it because no PR shows a missed check, and it mostly explained how the budget works; that guidance stays in `DebuggerSafetyBoundaries.md` (#9562). SEC-3.3 was also narrowed to the two shapes a diff shows: dictionary keys checked only when literal, and keywords matched against compiler-generated names.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Reason for change
Under thread-pool starvation, the test's async continuations can run after Dispose's wall-clock shutdown timeout expires. This causes the assertion that Dispose is still waiting for the final batch to fail even though the production behavior is correct. Azure build 210959 reported this failure after 5.02 seconds, matching the previous 5-second allowance.
Implementation details
The longer allowance accommodates scheduling delays while the test coordinates the two blocked sends. All synchronization and assertions remain unchanged, including checking that Dispose waits for the final send and that the queued exposure is flushed. Production shutdown behavior and the separate bounded-wait test remain unchanged.
Test coverage
Source comparison verified that only this test's shutdown allowance and explanatory comment changed. Attempted the focused ExposureApiTests on net8.0, but execution was blocked before build: global.json requires .NET SDK 11.0.100-rc.1.26425.128 and the sandbox only has SDK 8.0.412. No local reproduction or passing test run is claimed; CI validation is still required.
Other details
The affected test was introduced in #9353.
PR by Bits - View session in Datadog
Comment @DataDog to request changes