test: demonstrate enrollment_id race condition under concurrent retries - #7647
Draft
ycombinator wants to merge 5 commits into
Draft
test: demonstrate enrollment_id race condition under concurrent retries#7647ycombinator wants to merge 5 commits into
ycombinator wants to merge 5 commits into
Conversation
When multiple enrollment requests with the same enrollment_id arrive concurrently (before ES has indexed the first document), the search-based deduplication in fleet-server misses them and creates duplicate ghost agent records. This is the root cause of the ghost agents observed in 30k scale tests. The race window is the ES refresh interval (default 1s) — or much larger on Serverless Elasticsearch where writes go through object storage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an integration test intended to reliably reproduce the known enrollment_id deduplication race during concurrent enrollment retries, by disabling .fleet-agents auto-refresh and asserting that only a single agent record exists after concurrent enrollments.
Changes:
- Add
Test_Agent_Enrollment_Id_Raceto run multiple concurrent enrollments with the sameenrollment_id. - Temporarily set
.fleet-agentsrefresh_intervalto-1, then manually refresh and count matching documents.
Suppressed comments (1)
internal/pkg/server/fleet_integration_test.go:889
- Indices.Refresh also returns a response body that should be closed; additionally, a non-2xx refresh will not necessarily surface as a Go error, so the status code should be checked to avoid silently continuing with a failed refresh.
// Trigger a manual refresh so all committed documents become searchable.
_, err = esClient.Indices.Refresh(esClient.Indices.Refresh.WithIndex(dl.FleetAgents))
require.NoError(t, err)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+834
to
+845
| _, err = esClient.Indices.PutSettings( | ||
| bytes.NewBufferString(`{"index":{"refresh_interval":"-1"}}`), | ||
| esClient.Indices.PutSettings.WithIndex(dl.FleetAgents), | ||
| ) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| // Restore default refresh interval. | ||
| _, _ = esClient.Indices.PutSettings( | ||
| bytes.NewBufferString(`{"index":{"refresh_interval":"1s"}}`), | ||
| esClient.Indices.PutSettings.WithIndex(dl.FleetAgents), | ||
| ) | ||
| }) |
Comment on lines
+804
to
+808
| func Test_Agent_Enrollment_Id_Race(t *testing.T) { | ||
| const ( | ||
| enrollmentID = "race-test-enrollment-id" | ||
| concurrency = 5 | ||
| ) |
Comment on lines
+891
to
+895
| t.Cleanup(func() { | ||
| for _, id := range agentIDs { | ||
| _ = srv.bulker.Delete(ctx, dl.FleetAgents, id) | ||
| } | ||
| }) |
Replace the refresh_interval:-1 approach with one that matches production configuration: send the first enrollment, sleep briefly within the 1s default refresh window, then fire concurrent retries before the document is searchable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
internal/pkg/server/fleet_integration_test.go:815
- This test is timing-dependent (relies on a fixed 100ms sleep landing inside ES’ near-real-time refresh window). That makes it inherently non-deterministic and likely to be flaky or fail consistently on environments where the race exists. If this is intended as a diagnostic reproducer rather than a stable regression test, it should be skipped/gated so the integration suite remains reliable; if intended as a regression test, it should control ES refresh behavior to make the window deterministic.
func Test_Agent_Enrollment_Id_Race(t *testing.T) {
const (
enrollmentID = "race-test-enrollment-id"
concurrency = 5
)
internal/pkg/server/fleet_integration_test.go:855
io.ReadAllerrors are ignored here, which can mask truncated bodies and lead to confusing JSON unmarshal failures. Handle the read error explicitly.
return "", fmt.Errorf("unexpected status %d", res.StatusCode)
}
p, _ := io.ReadAll(res.Body)
var response api.EnrollResponse
internal/pkg/server/fleet_integration_test.go:894
- The manual index refresh call should use the test context so it can be cancelled on timeout/shutdown (avoids potential hangs in CI if Elasticsearch becomes unresponsive).
// Trigger a manual refresh so all committed documents become searchable.
_, err = esClient.Indices.Refresh(esClient.Indices.Refresh.WithIndex(dl.FleetAgents))
require.NoError(t, err)
Contributor
TL;DRBuildkite failed in Remediation
Investigation detailsRoot Cause
Specifically, the Buildkite diff shows:
Evidence
Verification
Follow-up
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (5)
internal/pkg/server/fleet_integration_test.go:813
- This test is designed to demonstrate a suspected race; as written it will fail in environments where the bug exists, which will make the integration test suite non-green. Consider skipping/gating it until the deduplication behavior is fixed, so CI remains stable while still keeping the reproducer in-tree.
func Test_Agent_Enrollment_Id_Race(t *testing.T) {
const (
enrollmentID = "race-test-enrollment-id"
concurrency = 5
internal/pkg/server/fleet_integration_test.go:898
- The Elasticsearch refresh response body isn't closed. go-elasticsearch responses should be closed to avoid leaking connections/file descriptors during the integration test suite.
_, err = esClient.Indices.Refresh(esClient.Indices.Refresh.WithIndex(dl.FleetAgents))
require.NoError(t, err)
internal/pkg/server/fleet_integration_test.go:884
- The errgroup context (gCtx) is intentionally unused, which adds noise and suggests the group cancels requests when it currently does not. Either use gCtx for the enrollment request contexts or drop the WithContext variant.
g, gCtx := errgroup.WithContext(ctx)
for i := 0; i < concurrency; i++ {
g.Go(func() error {
_ = gCtx
id, err := doEnroll()
internal/pkg/server/fleet_integration_test.go:856
- io.ReadAll error is ignored here; if the response body read fails, json.Unmarshal will likely report a misleading error (or decode partial data). Handle the read error explicitly.
This issue also appears on line 897 of the same file.
p, _ := io.ReadAll(res.Body)
var response api.EnrollResponse
internal/pkg/server/fleet_integration_test.go:894
- All concurrent enrollment errors are treated as non-fatal, including cases where every concurrent request fails (e.g., due to rate limiting). In that case the test can pass without actually exercising the race window. Add an assertion that at least one concurrent enrollment succeeded (len(agentIDs) > 1).
require.NoError(t, g.Wait())
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (8)
internal/pkg/server/fleet_integration_test.go:904
- The errgroup context is unused and enrollment errors are intentionally swallowed, which can let the test pass without actually exercising the concurrent retry path (e.g., if all concurrent enrollments fail). Consider removing the unused context and returning errors to fail fast so the test reliably validates the intended behavior.
g, gCtx := errgroup.WithContext(ctx)
for i := 0; i < concurrency; i++ {
g.Go(func() error {
_ = gCtx
id, err := doEnroll()
internal/pkg/server/fleet_integration_test.go:856
- The cleanup PutSettings call discards the response and never closes the response body. Even if you intentionally ignore errors in cleanup, it’s still important to close the response body to avoid leaking HTTP connections during the test run.
// Restore default refresh interval.
_, _ = esClient.Indices.PutSettings(
strings.NewReader(`{"index":{"refresh_interval":null}}`),
esClient.Indices.PutSettings.WithIndex(dl.FleetAgents),
)
internal/pkg/server/fleet_integration_test.go:137
- The limiter middleware returns HTTP 429 (Too Many Requests) for rate limiting (see internal/pkg/limit/error.go), not 503. The comment should reflect the actual status code so future readers understand what’s being disabled.
// WithNoEnrollRateLimit disables the enroll rate limiter so concurrent enrollment
// requests are not rejected with 503, allowing race conditions to be observed.
internal/pkg/server/fleet_integration_test.go:816
- This test’s implementation (setting refresh_interval=5s and sleeping exactly 5s at the refresh boundary) doesn’t match the PR description, which states it uses the default ES refresh behavior and sleeps briefly (~100ms) to stay inside the near-real-time window. Please align either the PR description or the test (and ideally avoid boundary timing since it makes the test nondeterministic/flaky).
// The test sets refresh_interval=5s on .fleet-agents to replicate Serverless
// Elasticsearch conditions (Serverless defaults to 5s, vs 1s for standard ES),
// then waits exactly 5s before firing retries — matching Horde's first backoff
// wait (EnrollBackoffInit=5s, attempt=0). The retry lands right at the edge of
// the refresh window, making the race timing-dependent.
internal/pkg/server/fleet_integration_test.go:919
- Like PutSettings, Refresh can return an HTTP error response with err == nil. The response body is also currently discarded without being closed. Capture the response, close the body, and assert it wasn’t an error response.
// Trigger a manual refresh so all committed documents become searchable.
_, err = esClient.Indices.Refresh(esClient.Indices.Refresh.WithIndex(dl.FleetAgents))
require.NoError(t, err)
internal/pkg/server/fleet_integration_test.go:936
- The Count API can also return HTTP error responses with err == nil; adding an IsError() check makes failures much easier to diagnose and avoids decoding non-success bodies as if they were valid count responses.
require.NoError(t, err)
defer countRes.Body.Close()
var countBody struct {
internal/pkg/server/fleet_integration_test.go:876
- The error from io.ReadAll is currently ignored. If the read fails, json.Unmarshal will run on partial/empty data and may mask the real failure. Handle the ReadAll error explicitly.
This issue also appears on line 900 of the same file.
if res.StatusCode != http.StatusOK {
return "", fmt.Errorf("unexpected status %d", res.StatusCode)
}
p, _ := io.ReadAll(res.Body)
internal/pkg/server/fleet_integration_test.go:850
- go-elasticsearch only returns a non-nil error for transport-level failures; HTTP 4xx/5xx responses come back as a Response with err == nil. This PutSettings call also discards/never closes the response body, which can leak connections. Capture the response, close it, and assert it’s not an error response (e.g., require.False(t, res.IsError()), and defer res.Body.Close()).
This issue also appears on line 852 of the same file.
_, err = esClient.Indices.PutSettings(
strings.NewReader(`{"index":{"refresh_interval":"5s"}}`),
esClient.Indices.PutSettings.WithIndex(dl.FleetAgents),
)
require.NoError(t, err)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (7)
internal/pkg/server/fleet_integration_test.go:904
for range concurrencydoes not compile becauserangerequires an array/slice/map/string/channel, not an int. This also leavesgCtxunused (worked around with_ = gCtx).
g, gCtx := errgroup.WithContext(ctx)
for range concurrency {
g.Go(func() error {
_ = gCtx
id, err := doEnroll()
internal/pkg/server/fleet_integration_test.go:888
- This test sets
refresh_intervalto 5s and then sleeps for 5s before firing the “retry” enrollments. Sleeping for the full refresh interval makes the race highly timing-dependent (and often eliminates the intended window), and it adds a fixed 5s to every integration run. To keep retries inside the non-searchable window, the sleep should be well below the refresh interval (and/or assert the first doc isn’t searchable before starting retries).
// Send the first enrollment, then wait exactly 5s — matching Horde's first
// backoff wait (EnrollBackoffInit=5s, attempt=0, jitter collapses to zero).
// With refresh_interval=5s, the retry lands right at the edge of the refresh
// window: whether the race triggers depends on where in the 5s cycle the
// first enrollment landed.
internal/pkg/server/fleet_integration_test.go:850
- The
PutSettingscall ignores the HTTP response object. With go-elasticsearch,errcan be nil while the response indicates an error (e.g., auth / bad request). Not checkingIsError()and not closing the response body can make failures harder to debug and leak connections in tests.
_, err = esClient.Indices.PutSettings(
strings.NewReader(`{"index":{"refresh_interval":"5s"}}`),
esClient.Indices.PutSettings.WithIndex(dl.FleetAgents),
)
require.NoError(t, err)
internal/pkg/server/fleet_integration_test.go:936
- The Count call should also check
countRes.IsError()before decoding the body. Otherwise an Elasticsearch error response (4xx/5xx) can be decoded as an empty struct and produce a misleading assertion failure.
countRes, err := esClient.Count(
esClient.Count.WithContext(ctx),
esClient.Count.WithIndex(dl.FleetAgents),
esClient.Count.WithBody(bytes.NewBufferString(fmt.Sprintf(
`{"query":{"term":{"enrollment_id":%q}}}`, enrollmentID,
))),
)
require.NoError(t, err)
internal/pkg/server/fleet_integration_test.go:919
- The manual
Indices.Refreshcall also ignores the HTTP response object. It’s better to close the response body and assertIsError()==falseso failures don’t get silently treated as successful refreshes.
// Trigger a manual refresh so all committed documents become searchable.
_, err = esClient.Indices.Refresh(esClient.Indices.Refresh.WithIndex(dl.FleetAgents))
require.NoError(t, err)
internal/pkg/server/fleet_integration_test.go:143
- WithNoEnrollRateLimit sets EnrollLimit.Interval/Burst to 0, but Fleet.Run calls cfg.LoadServerLimits(), which treats zero values as “unset” and merges env defaults back in. That means the enroll rate limiter will still be enabled and can still reject concurrent enroll requests with 503.
func WithNoEnrollRateLimit() Option {
return func(cfg *config.Config) error {
cfg.Inputs[0].Server.Limits.EnrollLimit.Interval = 0
cfg.Inputs[0].Server.Limits.EnrollLimit.Burst = 0
return nil
internal/pkg/server/fleet_integration_test.go:816
- The PR description says this test uses the default Elasticsearch refresh behavior (no refresh_interval override) and a short sleep (100ms). The implementation here overrides
.fleet-agentstorefresh_interval=5sand (before the sleep fix) waited 5s. Please align either the PR description or the test implementation so readers know which scenario is being exercised (standard ES defaults vs serverless-like refresh).
This issue also appears in the following locations of the same file:
- line 884
- line 900
- line 917
- line 929
// The test sets refresh_interval=5s on .fleet-agents to replicate Serverless
// Elasticsearch conditions (Serverless defaults to 5s, vs 1s for standard ES),
// then waits exactly 5s before firing retries — matching Horde's first backoff
// wait (EnrollBackoffInit=5s, attempt=0). The retry lands right at the edge of
// the refresh window, making the race timing-dependent.
5 tasks
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.
Theory
The
enrollment_iddeduplication mechanism in fleet-server may be racy. When multiple enrollment requests with the sameenrollment_idarrive concurrently, the search-based lookup (dl.FindAgent(..., QueryAgentByEnrollmentID, ...)) issues an ES search query, which is subject to near-real-time indexing latency (default 1s refresh interval). If retries arrive before ES has indexed the first document, the search returns nothing for each request and each creates a new ghost agent record.This would explain the 26 ghost agents observed in the 30k checkin scale test build #6778 (see the "Remaining issue" section of this comment), even after the cache regression fix — despite Horde already sending
enrollment_idon every retry.If true, the race window would be even larger on Serverless Elasticsearch, where the write path goes through object storage (S3), making indexing latency significantly higher than the standard 1s refresh interval.
The test
Test_Agent_Enrollment_Id_Raceattempts to verify this theory using the default ES configuration (norefresh_intervaloverride) to match production conditions:N=5concurrent enrollment requests with the sameenrollment_idinto that window_refreshIf the theory is correct, the test will fail — demonstrating that the
enrollment_idmechanism does not reliably prevent duplicates under concurrent retries against default ES configuration.Related
enrollment_idmechanism — potentially racy as theorized here)id+replace_token— uses GET by document ID, not subject to this race)id+replace_tokenin Horde)