fix(ddtrace/tracer): recover from stale-idle UDS conn races#4809
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b204b77 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-06-12 10:32:12 Comparing candidate commit b204b77 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 297 metrics, 2 unstable metrics, 1 flaky benchmarks without significant changes.
|
The unit test in ddtrace/tracer/transport_test.go (Layer A) proves the
APMS-19533 fix works against a synthetic close-every-conn HTTP server. This
harness is the Layer B end-to-end check: same fix, but driven against the
customer's exact agent version (datadog/agent:7.77.1) over a real UDS socket
with stale-conn injection that mimics the agent silently killing idle
keep-alive conns under backpressure.
Layout:
internal/apps/staleidle-soak/
main.go load driver (creates spans, captures statsd + log lines)
proxy/main.go UDS pass-through that closes each conn after one
response, reproducing the customer's failure shape
against the real agent
compose.yml datadog/agent:7.77.1 with DD_APM_RECEIVER_SOCKET
bind-mounted to the host
soak.sh orchestrator with three scenarios (B.1/B.2/B.3)
README.md run instructions + A/B procedure
.gitignore results/ (per-run JSON, not committed)
A/B results (5000 spans/sec, 50 concurrent goroutines):
| Scenario | spans_created | flush_traces | traces_dropped | lost_trace_log |
|-----------|---------------|--------------|----------------|----------------|
| B.1 base | 299980 | 299980 | 0 | 0 |
| B.1 fix | 299979 | 299979 | 0 | 0 |
| B.2 base | 300000 | 299700 | 300 | 1 |
| B.2 fix | 299973 | 299973 | 0 | 0 |
| B.3 base | 1499439 | 1499189 | 250 | 1 |
| B.3 fix | 1499951 | 1499951 | 0 | 0 |
Baseline B.2/B.3 reproduce the customer-visible `lost N traces ... EOF`
log line on a real agent. The fix takes both to zero across 60s and 300s
runs (1.5M spans, no drops, no error log lines, no api_errors metric).
Refs: APMS-19533
Refs: #4809
Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Adds a `--mode hang` to staleidle-proxy that accepts the request but never responds, and a B.4 scenario that drives it. This characterizes the edge of the APMS-19533 fix: connection-teardown errors (B.2/B.3: broken pipe / connection reset / EOF) are recovered, but a hung/unresponsive agent produces `context deadline exceeded`, which the fix intentionally does not retry. Verified: B.4 drops traces on both baseline and patched (no change), versus B.2 where baseline drops and patched recovers fully. Confirms the fix targets the customer's reported error family (teardown), not agent unresponsiveness. Refs: APMS-19533 Refs: #4809 Environment: Datadog workspace Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The unit test in ddtrace/tracer/transport_test.go (Layer A) proves the
APMS-19533 fix works against a synthetic close-every-conn HTTP server. This
harness is the Layer B end-to-end check: same fix, but driven against the
customer's exact agent version (datadog/agent:7.77.1) over a real UDS socket
with stale-conn injection that mimics the agent silently killing idle
keep-alive conns under backpressure.
Layout:
internal/apps/staleidle-soak/
main.go load driver (creates spans, captures statsd + log lines)
proxy/main.go UDS pass-through that closes each conn after one
response, reproducing the customer's failure shape
against the real agent
compose.yml datadog/agent:7.77.1 with DD_APM_RECEIVER_SOCKET
bind-mounted to the host
soak.sh orchestrator with three scenarios (B.1/B.2/B.3)
README.md run instructions + A/B procedure
.gitignore results/ (per-run JSON, not committed)
A/B results (5000 spans/sec, 50 concurrent goroutines):
| Scenario | spans_created | flush_traces | traces_dropped | lost_trace_log |
|-----------|---------------|--------------|----------------|----------------|
| B.1 base | 299980 | 299980 | 0 | 0 |
| B.1 fix | 299979 | 299979 | 0 | 0 |
| B.2 base | 300000 | 299700 | 300 | 1 |
| B.2 fix | 299973 | 299973 | 0 | 0 |
| B.3 base | 1499439 | 1499189 | 250 | 1 |
| B.3 fix | 1499951 | 1499951 | 0 | 0 |
Baseline B.2/B.3 reproduce the customer-visible `lost N traces ... EOF`
log line on a real agent. The fix takes both to zero across 60s and 300s
runs (1.5M spans, no drops, no error log lines, no api_errors metric).
Refs: APMS-19533
Refs: #4809
Adds a `--mode hang` to staleidle-proxy that accepts the request but never responds, and a B.4 scenario that drives it. This characterizes the edge of the APMS-19533 fix: connection-teardown errors (B.2/B.3: broken pipe / connection reset / EOF) are recovered, but a hung/unresponsive agent produces `context deadline exceeded`, which the fix intentionally does not retry. Verified: B.4 drops traces on both baseline and patched (no change), versus B.2 where baseline drops and patched recovers fully. Confirms the fix targets the customer's reported error family (teardown), not agent unresponsiveness. Refs: APMS-19533 Refs: #4809 Environment: Datadog workspace
16aa81f to
3fe9fba
Compare
|
@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: 9a143e1f6b
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR hardens the tracer’s UDS HTTP transport against “stale idle” keep-alive connection races (agent silently drops an idle UDS conn, next POST fails with EPIPE/ECONNRESET) by making POST requests replayable to net/http and adding a narrow application-level retry for the mid-write window where stdlib won’t retry. It also adds an end-to-end soak harness to reproduce/verify the failure mode against a pinned real agent version over UDS.
Changes:
- Mark trace and stats POSTs replayable (
Idempotency-Key+GetBody) and route through a newdoWithStaleConnRetryhelper for transient teardown errors. - Add a new unit test that deterministically reproduces stale-idle UDS failures with concurrent burst traffic.
- Add an
internal/apps/staleidle-soak/docker-compose-based harness (driver + failure-injecting proxy + orchestrator script) for end-to-end verification againstdatadog/agent:7.77.1.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ddtrace/tracer/transport.go |
Adds replayability (Idempotency-Key, GetBody) and doWithStaleConnRetry + transient-conn error classification. |
ddtrace/tracer/transport_test.go |
Adds a concurrent UDS test server that closes connections after one response to validate recovery. |
internal/apps/staleidle-soak/main.go |
Load driver that emits JSON results and captures tracer statsd counters + error logs. |
internal/apps/staleidle-soak/proxy/main.go |
UDS pass-through proxy that injects close/hang failure modes for deterministic reproduction. |
internal/apps/staleidle-soak/soak.sh |
Orchestrates agent startup, proxy, driver execution, and artifact collection. |
internal/apps/staleidle-soak/compose.yml |
Pins agent image/version and exposes APM receiver UDS socket via bind mount. |
internal/apps/staleidle-soak/README.md |
Documents scenarios, usage, and pass criteria for A/B verification. |
internal/apps/staleidle-soak/.gitignore |
Ignores soak run artifacts (results/). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…body cleanup Two issues addressed from Copilot review threads on PR #4809: 1. io.NopCloser in GetBody suppressed Close() on the rewound body, breaking the payloadV1 two-bit pool-handoff: pv1StateBodyClosed was never set via the HTTP retry path, so payloadV1 could not return to its pool after the stdlib's nothingWrittenError rewind. Return p directly — payloadV04.Close is a no-op and payloadV1.Close calls handoff(pv1StateBodyClosed) which is idempotent via atomic.Or. 2. In doWithStaleConnRetry, close any prior response body before calling GetBody and retrying. Per Go docs resp is nil when err is a transport error, so this guard is a no-op today, but makes the retry loop correct by construction for any future code path where resp != nil alongside a transient error.
The trace-agent recycles idle keep-alive UDS connections under backpressure. The tracer's next request on a pooled-but-dead connection fails immediately with `write: broken pipe` or `read: connection reset by peer`, dropping the payload. Prior fix #4484 added retry loops gated on sendRetries (default 0), so customers without WithSendRetries got no retry at all. Fix — two layers: 1. Make each POST replayable so net/http transparently retries on a fresh connection before the error reaches the caller: - Set req.GetBody (http.NewRequest doesn't auto-detect the custom payload type) so the stdlib can rewind the body on retry. - Set an Idempotency-Key header so net/http.Request.isReplayable returns true and the stdlib's nothingWrittenError path silently retries. - Return the payload directly from GetBody (not via io.NopCloser) so payloadV1.Close() is called by the HTTP transport, completing the two-bit pool-return handoff (pv1StateBodyClosed via atomic.Or). 2. Add doWithStaleConnRetry: up to 3 extra attempts (4 total) on transient connection errors (EPIPE, ECONNRESET, ECONNABORTED, net.ErrClosed, plus a cross-platform string fallback for Windows WSA errors). Covers the residual mid-write window where stdlib refuses to replay because pc.nwrite > startBytesWritten (golang/go#19943). Retrying is safe: the agent deduplicates traces by span ID and ignores the Idempotency-Key header, so a duplicate payload is harmless. Also adds internal/apps/staleidle-soak/: an end-to-end verification harness that runs the fix against agent v7.77.1 over a real UDS socket. Scenarios B.2/B.3 (stale-conn injection) show baseline dropping ~250+ traces per run; patched delivers 0 dropped across 1.5M+ spans. B.4 (hung agent / timeout) is a boundary scenario that both baseline and patched drop, documenting the fix targets connection teardown, not agent unresponsiveness.
59f4490 to
579626b
Compare
|
Related prior work: #3991 After opening this PR, we found #3991 (by @lattwood), which targets the same failure class and is still open. It covers This PR is a superset of #3991: both endpoints, stdlib retry + application-level |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3faf51a53d
ℹ️ 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".
| // connection when an idle UDS conn was silently closed by the agent. | ||
| // http.NewRequest already populates req.GetBody for *bytes.Buffer bodies. | ||
| req.Header.Set(idempotencyKeyHeader, newIdempotencyKey()) | ||
| resp, err := t.doWithStaleConnRetry(req) |
There was a problem hiding this comment.
When the stats endpoint accepts a POST and then the connection is reset before the client reads the response, this call retries the same ClientStatsPayload even though the agent ignores the idempotency header and client stats payloads have no span-ID deduplication. That can double-count stats under exactly the mid-request reset path doWithStaleConnRetry adds for errors after bytes may already have been written; unlike traces, these aggregate stats are not safe to replay blindly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The concern is correct. Distinguishing the two failure shapes:
- EPIPE / write-side ECONNRESET: the write failed, bytes never reached the agent. Retry is unambiguously safe.
- Read-side ECONNRESET: the write succeeded, the agent received and processed the payload, then reset the connection before we read the response. A retry here delivers the same
ClientStatsPayloadtwice, which can overcount metrics for that flush window.
The agent has no span-ID-style dedup for stats, so the double-count is real.
We're accepting this trade-off for two reasons:
- The read-side reset is the rarer path — the stdlib
Idempotency-Key+GetBodymechanism already recovers from pre-write failures transparently. The app-level retry indoWithStaleConnRetryonly fires when stdlib refuses to retry (bytes already in flight). The overlap between "bytes written, agent processed, connection reset before response" and "triggers doWithStaleConnRetry" is a narrow window. - A transient one-window overcount self-corrects on the next flush. Silently dropping stats is worse.
Adding an inline comment at this call site to document the trade-off so future readers don't rediscover it.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
…onn retry budget (#4908) ## What does this PR do - Add `syscall.ENOTCONN` to `isTransientConnError` (alongside `EPIPE`/`ECONNRESET`/`ECONNABORTED`/`net.ErrClosed`), and add a `"socket is not connected"` string fallback for Windows `WSAENOTCONN` parity. - Raise `staleConnRetryAttempts` from `3` → `6` (7 total attempts) to absorb the Linux `net.ErrClosed` burst-exhaustion path. ## Motivation `TestUDSTransportRecoversFromStaleIdleConn` was flaking at ~1.25%/run on macOS CI (#4809 introduced the test and the retry fix). The flake had two distinct failure modes: **macOS (dominant, reproduced):** On Darwin, the same stale-idle UDS write race that produces `EPIPE` on Linux surfaces instead as `ENOTCONN` (`"socket is not connected"`). `isTransientConnError` did not recognise `ENOTCONN`, so `doWithStaleConnRetry` returned immediately without retrying and the request was lost. **Linux (rarer, the originally-reported CI error):** `net.ErrClosed` is correctly classified and retried, but under the 400-request synthetic burst the idle pool can keep handing out stale conns, occasionally exhausting all 4 original attempts before landing on a fresh connection. ### Verification Reproduced baseline flake with a compiled test binary: **10/80 batches failed** (count=10 each), 100% ENOTCONN. After the fix: **0/80 batches failed**. ``` # baseline (main): 10/80 batches failed # after fix: 0/80 batches failed ``` - `go test -race -run 'TestUDSTransportRecoversFromStaleIdleConn' -count=20 ./ddtrace/tracer/` — clean - Full transport regression set from #4809 test plan — all green - `go build ./ddtrace/tracer/... && go vet ./ddtrace/tracer/...` — clean Co-authored-by: kakkoyun <[email protected]> Co-authored-by: dario.castane <[email protected]>
Summary
net/httpcan transparently recover from idle UDS connections silently dropped by the agent: setreq.GetBody(custom payload type thathttp.NewRequestdoesn't auto-detect) and anIdempotency-Keyheader on bothhttpTransport.sendandhttpTransport.sendStats.doWithStaleConnRetry(up to 3 extra attempts (4 total)) onEPIPE/ECONNRESET/net.ErrClosedto cover the residual mid-write window where stdlib refuses to retry — because once any byte hits the wire, the error is no longer classified asnothingWrittenError(see golang/go#19943).writer.go:178—attempt+1%5 == 0parsed asattempt + 1, so the periodic "failure sending traces" log never fired.internal/apps/staleidle-soak/that runs the fix end-to-end against the customer's exact agent version (datadog/agent:7.77.1) over a real UDS socket.Motivation
Following up on #4484, (APMS-19533) still saw transport errors after upgrading to
v2.8.2:PR #4484 added retry loops to
writer.goandstats.go, but both are gated onsendRetries(default0). Customers who don't explicitly opt in get the loop budget of 1 attempt — i.e. no retry — so any stale-idle race drops a payload.The fundamental issue: stdlib won't auto-replay a POST after a transport error unless it can both rewind the body and trust the call is idempotent. Without
GetBody(custom payload) and withoutIdempotency-Key,net/http.Request.isReplayablereturnsfalseand stdlib silently swallows the recovery path.Why two layers
Empirically (see
TestUDSTransportRecoversFromStaleIdleConn),Idempotency-Key + GetBodyalone fixes ~80% of the failures. The remaining ~20% arewrite: broken pipemid-request: stdlib seespc.nwrite > startBytesWrittenand won't classify it asnothingWrittenError, so it refuses to retry even thoughisReplayable()would now return true. The application-level retry covers that residual window.Retrying is safe: the agent dedupes traces by span ID and ignores
Idempotency-Key, so a duplicate payload is harmless. We only retry on the narrow set of errors that unambiguously signal a peer-side connection teardown.Test plan
Layer A — unit test against synthetic close-every-conn server
TestUDSTransportRecoversFromStaleIdleConn(new) — UDS server that abruptly closes conns after responding (noConnection: closeheader), 400 concurrent POSTs acrosssend+sendStats. Onmain: 30–45% of requests fail with the exact error strings the customer reports. With this PR: 10/10 runs green.go test -race -run "^(TestStatsFlushRetries|TestTraceWriter|TestApi|TestUDS|TestTraceCountHeader|TestDefaultHeaders|TestExternalEnvironment|TestFetchAgentFeaturesContainerTagsHash|TestWithUDS|TestWithHTTPClient|TestResolveAgentAddr|TestTransportResponse)$" ./ddtrace/tracer/— all green.go build ./ddtrace/tracer/...+go vet ./ddtrace/tracer/...— clean.Layer B — end-to-end harness against
datadog/agent:7.77.1over real UDSNew under
internal/apps/staleidle-soak/: a docker-compose'd agent pinned to the customer's version, a Go load driver, a UDS pass-through proxy that injects failure, and an orchestrator script that runs four scenarios on both branches for A/B comparison. B.1–B.3 are pass/fail; B.4 is a boundary scenario that documents the edge of what the fix covers.B.2 / B.3 inject the customer's failure mode — the agent silently closes idle keep-alive UDS conns (proxy closes each conn after one response). They reproduce the customer's exact log shape:
With the patch applied, every span across 1.5 M+ creations on the longest soak is delivered (
spans_created == flush_traces), with zero error-log lines and zeroapi_errors.B.4 is deliberately not a pass/fail test. The proxy (
--mode hang) accepts the request but never responds, simulating a hung/unresponsive agent. The tracer times out (context deadline exceeded) — a non-teardown error the fix intentionally does not retry (retrying against a wedged agent only stacks load). B.4 therefore drops on both baseline and patched, which is the point: it documents that the fix targets connection teardown (the customer's observed error family) and not agent unresponsiveness. The customer's reported errors (APMS-19533) are all teardown; none are timeouts.The harness is checked in so anyone can re-run the same verification (see
internal/apps/staleidle-soak/README.md).Refs