test: release the transports and clients the tests construct - #1048
Merged
Conversation
opensearchtransport.New starts two ticker goroutines on every call: the
node-stats poller (NodeStatsInterval: 0 means auto-derive, not disabled)
and the cluster-health refresh loop (healthCheckRate is derived from the
server core count and is never zero). Close is the only thing that stops
either one, so a test that constructs a *Transport and never releases it
leaves live tickers running for the remaining life of the test binary,
where they perturb process-wide measurements such as the package's
zero-allocation assertions.
Register t.Cleanup(func() { _ = tp.Close() }) at all 151 construction
sites. Sites that already closed their transport are left alone, as is the
one site where New is expected to fail and returns a nil transport
(TestNewCancelsDNSResolverOnError in dnscache_internal_test.go).
Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
…ation TestClassify_ZeroAlloc and TestNewRequestEventZeroAlloc use testing.AllocsPerRun, a process-wide allocation differential. Neither source file carried a build tag, so both compiled into every configuration including -tags integration,core, sharing a binary with the live-cluster tests whose transports poll node stats and cluster health in the background. That is what flaked TestClassify_ZeroAlloc in CI. Move each test, unchanged, into a //go:build !integration file so the differential only ever runs where nothing else allocates concurrently. The two tests need separate files because their sources sit in different packages: classify_extra_test.go is opensearchtransport_test and observer_response_internal_test.go is opensearchtransport. The remaining tests in both source files stay untagged; only the differentials need the constraint. Add TestCloseReapsBackgroundPollers as the durable guard: it asserts via the goroutine dump that New starts the node-stats and cluster-health pollers and that Close reaps both. Verified to fail when Close is stubbed to skip its context cancellation. Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
Same goroutine contract as the opensearchtransport sweep: a transport
starts a node-stats poller and a cluster-health refresh loop that only
Close stops, so a client left unreleased leaks two tickers for the rest of
the test binary's life. opensearch.Client.Close and
opensearchapi.Client.Close are the release points.
Register a release at all 26 remaining hand-written sites: t.Cleanup in
tests, b.Cleanup in benchmarks, defer in examples (Example functions have
no *testing.T). ExampleNewClient_logger discarded its client entirely; it
now binds and releases it.
Three kinds of site are deliberately untouched:
- opensearchapi/testutil.NewClient returns a sync.Once-shared client the
package owns; closing it from one test would break every later test.
- TestClientCustomTransport already reclaims its NewDefaultClient
transport, and BenchmarkClient already closes per iteration.
- Nothing relies on BulkIndexer.Close: all three opensearchutil sites
pass a client in, so implicitClient is false and the test owns it. In
the integration test the client release is registered before the
index-delete cleanup so LIFO ordering leaves the client alive for the
delete.
Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
Thirteen cases in bulk_indexer_internal_test.go build an opensearchapi client and hand it to BulkIndexerConfig.Client. A supplied client is not owned by the indexer, so implicitClient stays false and BulkIndexer.Close leaves it open, exactly as TestBulkIndexerOwnClientFlag asserts. Each one left its transport polling node stats and cluster health for the rest of the test binary. Register the same t.Cleanup close used elsewhere on this branch. The one remaining unclosed client in the file is deliberate: that case forces implicitClient to true so the indexer owns it, then asserts the close happened. Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
…it binary TestCloseReapsBackgroundPollers asserts the poller frames are gone from the process-wide runtime.Stack dump after Close. That only holds where no other live transport is polling, the same constraint that already keeps the zero-allocation assertions out of the live-cluster binaries, so this file now carries !integration too. The doc comment states the constraint once and covers both the build tag and the absence of t.Parallel. Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
The counts predated the bulk indexer fixups: 153 sites in opensearchtransport and 39 elsewhere. The exclusion also described the wrong case. A client the caller supplies is never owned by the indexer, so the surviving unclosed site is the one that forces implicitClient to true and asserts the indexer closed it. Note that the poller guard carries !integration for the same reason the allocation assertions do. Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>
gocritic's exitAfterDefer failed the lint job on the two examples this
branch gave a `defer func() { _ = client.Close() }()`. log.Fatalf calls
os.Exit, which does not unwind the stack, so every Fatalf placed after the
defer would have skipped the close the branch added -- the leak the branch
set out to fix, still leaking on the error path.
Switch those calls to log.Panicf, which runs deferred functions on its way
out. Examples have no *testing.T, so require.NoError and t.Cleanup are not
available here; panicking is what the neighbouring osprom and osotel
examples already do.
Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
ryanyuan
approved these changes
Aug 7, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1048 +/- ##
==========================================
- Coverage 61.35% 61.31% -0.04%
==========================================
Files 666 666
Lines 59841 59841
==========================================
- Hits 36713 36693 -20
- Misses 21226 21242 +16
- Partials 1902 1906 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
2 participants
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.
opensearchtransport.Newstarts two ticker goroutines on every call: the node-stats poller (NodeStatsInterval: 0means auto-derive, not disabled) and the cluster-health refresh loop (healthCheckRateis derived from the server core count and is never zero).Closeis the only thing that stops either one, so a test that constructs a transport or client and never releases it leaves two live tickers running for the remaining life of the test binary, where they perturb process-wide measurements.Every construction site now registers a release:
t.Cleanupin tests,b.Cleanupin benchmarks,deferin examples (Examplefunctions have no*testing.T).TestClassify_ZeroAllocandTestNewRequestEventZeroAllocusetesting.AllocsPerRun, a process-wide allocation differential. Neither source file carried a build tag, so both compiled into every configuration including-tags integration,core, sharing a binary with the live-cluster tests whose transports poll node stats and cluster health in the background. That is what flakedTestClassify_ZeroAllocin CI.New
TestCloseReapsBackgroundPollersreads the goroutine dump to assert thatNewstarts both pollers and thatClosereaps them, so the leak cannot return silently. Verified to fail whenCloseis stubbed to skip its context cancellation. It carries the same!integrationconstraint, because the process-wide dump only settles in a binary where no other live transport is polling, and it omitst.Parallelfor the same reason.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.