fix(ddtrace/tracer): settle writer payload before checking it in downgrade race test - #5252
fix(ddtrace/tracer): settle writer payload before checking it in downgrade race test#5252darccio wants to merge 3 commits into
Conversation
…grade race test Nothing re-checks h.payload's protocol except add()/flush() themselves. If all of TestConcurrentAddNeverResurrectsDowngradedPayload's concurrent add() calls finished before refreshAgentFeatures's HTTP round-trip landed, h.payload legitimately still held whatever protocol was last observed -- not a production bug, just nothing having asked it to settle yet. That timing dependency flaked in CI (expected 0.4, got 1). Force one more add() once the downgrade is confirmed applied, so the assertion observes the writer's real settling behavior instead of racing the poll's network I/O. Confirmed by reproducing the failure deterministically with a reduced iteration count (30/30 failing before this fix, 50/50 passing after).
darccio
added
the
AI Generated
Config Audit |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ea1eb3c | Docs | View more details | Give us feedback! |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffc1bc1735
ℹ️ 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".
… the downgrade race test The corrective add() fired right before inspecting w.payload.protocol() ran serially, so it would settle any resurrected v1 payload itself before the assertion looked -- the test could not fail even with the fixed race reintroduced. Replace it with an ordinary flush(), which sends h.payload under its own recorded protocol without repairing it, and assert at most one /v1.0/traces request lands across the storm and that flush.
BenchmarksBenchmark execution time: 2026-08-24 16:48:28 Comparing candidate commit ea1eb3c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 335 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1005f9e7a
ℹ️ 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".
| v1Requests++ | ||
| } | ||
| } | ||
| assert.LessOrEqual(t, v1Requests, 1, "at most one /v1.0/traces request may occur across the storm and its flush -- more indicates a payload resurrected after the downgrade settled") |
There was a problem hiding this comment.
When the downgrade wins before any add has populated the initial v1 payload, the regressed interleaving can still pass this assertion: a v0.4 reader rotates the empty v1 payload without sending it, then a delayed stale v1 reader seals the populated v0.4 payload and resurrects v1. The final flush records that bad payload as the only v1 request, so v1Requests <= 1 succeeds and also replaces the current payload with v0.4 before the post-reset check. Seed a known pre-downgrade payload or otherwise distinguish legitimate pre-downgrade sends so a lone resurrected v1 request is rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — you're right that <=1 was too loose. Fixed in ea1eb3c.
Seeded a real trace into the payload before the storm/downgrade race starts, so it's never empty when the downgrade lands. That closes off the free empty-payload rotation path (no send), which previously could consume the one legitimate v1->v0.4 transition silently — leaving a lone resurrected v1 request looking like the expected one. With the seed, whichever add()/flush() first observes the downgrade must go through a real seal-and-send, so exactly one v1 request is now guaranteed in the correct-code case, and the assertion is tightened from <= 1 to == 1.
Re-verified detection power the same way as before: reintroduced the pre-fix stale-read bug in writer.go — failed 10/10 under -race; reverted — passed 10/10.
…e transition send can't be skipped If the downgrade won the race before any storm add() populated the initial (empty) v1 payload, rotateStalePayload's free-for-empty path would consume that transition with no /v1.0/traces request at all -- so a single resurrected v1 request later would satisfy the "at most one" check while being exactly the regression it should catch. Seed a real trace into the payload before the race starts so it is never empty when the downgrade lands, guaranteeing the transition always goes through a real seal-and-send, and tighten the assertion to exactly one v1 request.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
TestConcurrentAddNeverResurrectsDowngradedPayloadcheckedw.payload.protocol()right after the downgrade was confirmed applied, but nothing forces anadd()/flush()call to happen after the downgrade lands — only those calls re-check the protocoladd()calls (in-memory, fast) finished beforerefreshAgentFeatures's/infoHTTP round-trip completed,h.payloadlegitimately still held the last-observed protocol, and the assertion flaked (seen in CI: expected0.4, got1) — not a production bug, just a test racing its own setupadd()once the downgrade is confirmed applied, so the assertion observes the writer's real settling behavior instead of racing the poll's network I/OInvestigated from a
release-v2.10.xCI failure: https://github.com/DataDog/dd-trace-go/actions/runs/32144929774/job/95736348552Test plan
go test -race -run TestConcurrentAddNeverResurrectsDowngradedPayload -count=10 ./ddtrace/tracerpasses at full scalego vet ./ddtrace/tracerclean