Fix the goroutine leaks tracked in the leak regression test - #11322
Draft
prathyushpv wants to merge 19 commits into
Draft
Fix the goroutine leaks tracked in the leak regression test#11322prathyushpv wants to merge 19 commits into
prathyushpv wants to merge 19 commits into
Conversation
Removes every TODO ignore from goleakOpts in tests/leakcheck by fixing the leaks they covered: - RPCFactory now owns the gRPC connections it dials and closes them on shutdown. Nothing released them before, so every connection's gRPC goroutines (and the membership resolver listening for changes) outlived the cluster. - The operator and admin handlers dialed a local frontend/operator connection on every request and dropped it. They now create one on first use and reuse it; only a successful client is cached so a failure stays retryable. - The SDK client factory closes the shared system client. Clients handed out by NewClient share its connection and the SDK only closes that connection once every derived client is closed, so the factory tracks the ones its callers leave open and releases them first. - The batcher activity closes the SDK client it creates per execution. - parentclosepolicy.Processor kept no reference to its SDK worker and had no Stop, so the worker ran until the process exited. - The version check returned on a non-200 response before its deferred Body.Close, pinning the HTTP connection's read and write loops. - The functional test infra releases the matching client's connection cache and the RPC factory backing it.
Follow-up to the goroutine-leak fixes, from review feedback. Reuse the local frontend connection in RPCFactory instead of caching a client in each handler. The target is fixed and nothing closes the connection, so one sync.OnceValue there replaces the per-handler caches, covers the connection NewLocalAdminClientWithTimeout was also dropping, and leaves the frontend handlers untouched. clientFactory.Close no longer claims the dial-once. Doing so made GetSystemClient hand out a nil client that no caller checks, let a post-shutdown NewClient dial a fresh connection and Fatal on failure, and could block the stop hook behind an in-flight dial. The shutdown state is now an explicit flag, the system client is published under the lock, and NewClient holds that lock across creation so adding a reference to the shared connection cannot interleave with releasing them. Stop the SDK workers the scanner starts. Nothing did, so the pollers ran until the process exited; the ignores this branch removed only passed because the functional tests disable every scanner. Drop connections already shut down when tracking a new one. The history pool and matching cache close their own connections when a host leaves the ring, and the factory kept the closed ones reachable for the process lifetime. Add the tests that were missing: RPCFactory.Close (connections shut down, idempotent, dialed-after-close, closed-by-owner, eviction) and the version-check body close, which leaks the HTTP read and write loops on a non-200 with a body. Both fail if their fix is reverted.
NewClient no longer hands back the shared system client once the factory is closed. Between Close setting the flag and closing that client there is a window where it still works, so a caller could have issued calls against temporal-system instead of the namespace it asked for, and its own Close would have raced the factory's. It now builds the client the caller asked for and releases it immediately. GetSystemClient no longer dials once the factory is closed. It used to retry for a minute against a frontend that was going away and then abort the process from inside the stop hook; it now hands back a lazy client whose calls fail. Close is idempotent on its own rather than relying on the SDK's guard, which only holds for sequential calls, and map membership replaces the per-client sync.Once as the token that decides who closes a derived client. Scanner worker start and tracking move into one helper, so a started worker is always tracked. Tests: the connection sweep assertion no longer hands a live grpc.ClientConn to a testify equality helper, which deep-compared gRPC's mutable internals and failed under -race. Adds coverage for the memoized local frontend connection, for the factory releasing derived clients before the system client, and for the parentclosepolicy worker being stopped. Each fails if its fix is reverted.
NewClient checks for shutdown before deriving from the system client. Deriving fetches capabilities over that client's connection, which is closed by then, so the error aborted the process from inside the stop hook — the failure the previous round set out to remove. The shutdown path now builds a lazy client, which skips the fetch. A lazy system client that fails to build is fatal rather than warned about, so GetSystemClient cannot publish a nil client and leave callers to dereference it. Close releases derived clients while holding the lock, which is what its caller-facing comment already claimed, and clears the map rather than nilling it so no path can insert into a nil map. Tests: the assertion that closing an already-closed connection stays quiet was vacuous, because a test logger never fails on Warn; it now counts matches. Routes the SDK factory tests through NewClient and NewWorker against a real listener, which is what the two bugs above needed to be caught, and switches them to gomock to match the rest of the repo. Tightens a scanner expectation that permitted zero calls.
The SDK factory test fixture dialed a system client and never closed it, stranding a connection and its goroutines in the test binary — two of the very goroutines this branch removed ignores for. The fixture now closes the factory, and the package verifies with goleak so the next one is caught here rather than in the functional suite. Adds a test pinning the lock NewClient holds across NewClientFromExisting: the SDK reads the shared reference count there while Close writes it, so deriving outside the lock races. Detection is probabilistic but the invariant had no guard at all, and it is the kind a later reader would reasonably try to simplify away. The fixture also failed silently on a bad dial, because a noop logger's Fatal does nothing; it now uses a logger that fails the test. Asserts the worker test really is handed a wrapper, so the unwrap it guards cannot rot while the assertion stays vacuously true. Drops a comment describing the bug rather than the code.
The waitgroup form the linter asks for, which CI caught against its own base revision. The test factory passes its logger to the constructor rather than assigning the field afterwards, so the logger the SDK itself uses is covered too; assigning after construction left that one on the noop logger, which is the silent failure the logger was added to remove. Two comments describe what the code does now.
TestPostInfo left its server running, which the neighbouring leak test's baseline then absorbed, weakening that assertion. The race test's comment now says what it actually detects: dropping the lock reliably, narrowing it only sometimes.
The batch activity's client release was the last NewClient caller with no test, and neither the unit tests nor the leak test could see it: an unreleased derived client adds no goroutines, only a retained map entry. A closed factory returning a lazy client rather than dialing had no test either, and the behaviour it protects is a minute of retries followed by an abort partway through shutdown. Repeats the interleaving in the NewClient/Close race test. One pass detected a narrowed lock about a quarter of the time, which is not detection.
The retry around the system client dial cannot be cancelled, so checking for shutdown only before it left a Close that landed mid-dial still reaching for a frontend that was going away until the policy expired, and then aborting the process. The check now runs on every attempt, which also leaves one place that builds a lazy client instead of two. The leak test touches the matching client, so the connection cache and the RPC factory behind it are built and their teardown is covered. Removing that teardown was previously detected by nothing. Asserts that Close leaves the system client set, and that it releases the tracked connections, both of which the code claims and neither of which was checked. Repeats the NewClient/Close interleaving more times, since detection at twenty was load-dependent.
The existing test closes the factory before the first attempt, which the shape this replaced already handled, so the per-attempt check had nothing holding it. The new test waits for a dial to fail before closing, so the check under test is the one inside the retry. Declares the closure's error once, as the rest of the repo does, and describes what touching the matching client actually starts: its membership watcher and partition cache, not a populated connection cache.
The test previously dialed a port it had bound and released, which another test in the package could rebind to a listener that answers, leaving the dial to succeed and the test waiting for a failure that never came. Binding port 1 needs privileges, so nothing here can claim it. Drops a doc comment clause that restated the production comment.
A logger whose Fatal returns reaches this with nothing to publish, and closing a nil client panics.
The shutdown branch closes the client it hands back, which panics when NewLazyClient failed and the logger's Fatal returned instead of exiting.
The comment on Close claims it, and nilling the field passed the package.
Storing a nil client made Close panic while releasing derived clients, before it reached the system client, leaking the connection this change exists to release. Both sites now return the way dial does in common/rpc. The invariant about Close is asserted in the test for Close.
Publishing the system client and tracking a dialed connection each take a lock that Close also holds, and dropping either was detected by nothing. Both tests report the two accesses under -race when the lock goes.
The check has no cancellation: Stop only closes a channel and the request carries no context, so a server that accepts a request and withholds the response strands the transport goroutines whose goleak suppressions this branch removes.
NewClient on a shut down factory returned a client it had already closed, so the caller's Close was a second close of the same client. Return it wrapped and untracked instead, so releasing it is a no-op. Also check for shutdown before dialing a system client that would only be closed again, replace the already-closed flag in Close with an early return, and take connsLock once in trackConn. The matching RPC factory in testcore released nothing: matching's own Stop already closes the cached connections, so the leak suite is unchanged without it.
The timeout added in the previous commit had no test: a stalled server now proves Call returns and leaves no transport goroutines behind, which is what lets the persistConn ignores stay out of goleakOpts. callTimeout becomes a var so the test does not have to wait 30s. Assert that a shut down factory does not dial a system client, and close the post-shutdown client concurrently rather than asserting it does not panic, which it would not do either way. Three comments claimed more than the code does: the pre-check does not stop a dial (the check inside the retry already does), repeated Close would only race if it were concurrent, and gomock fails on any unexpected Stop rather than on one for a worker that never started. Dropping the matching client reference on teardown released nothing and made the accessor panic instead of returning a stopped client.
This was referenced Aug 6, 2026
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
1 participant
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.
What changed?
Removed every TODO ignore from
goleakOptsintests/leakcheck/leak_test.goand fixed the leaks they covered. The one remaining ignore is by design: sqlite keeps a*sql.DBper file DSN for the process lifetime.RPCFactoryowns the gRPC connections it dials and closes them on shutdown; nothing released them before, so each connection's goroutines and the membership resolver watching for changes outlived the cluster. Connections their owner already closed are dropped when the next one is dialed, so hosts that left the ring don't stay reachable from the factory.CreateLocalFrontendGRPCConnectionreturns one connection instead of dialing per call. The operator and admin handlers were dialing a fresh connection on every request and discarding it, andclient_beandiscarded one too.NewClientshare its connection and the SDK only closes that connection once every derived client is closed, so the factory releases the ones its callers leave open. After shutdown it hands back a lazy client rather than deriving from the closed one, and it re-checks for shutdown on every retry attempt, so aCloselanding mid-dial stops the retry instead of chasing a departing frontend until the policy expires and then aborting the process. That post-shutdown client is returned untracked, so a caller closing it releases nothing rather than closing it a second time.parentclosepolicy.Processorandscanner.Scannerstop the SDK workers they start; neither kept a reference before, so the pollers ran until the process exited.Why?
Two of these leaked per request or per activity execution rather than per cluster, so they accumulated for the lifetime of a server process, not just across tests. The rest made
TestClusterShutdownLeakunable to detect a new per-cluster leak, because the ignores were broad enough to hide it.How did you test it?
make leak-testat the CI settings reportsno unexpected goroutineswith the ignores removed. Each leak was located by instrumenting the suspected owner and comparing created-versus-released counts; one early hypothesis (a per-namespace worker start-after-stop race) was ruled out that way rather than patched.The leak suite does not exercise the scanner workers or the batch activity, since scanner roles are disabled in the test config, so those two fixes rest on their unit tests rather than on
make leak-test.New unit tests cover
RPCFactory.Closeand the connection sweep, the memoized local frontend connection, the SDK factory releasing derived clients before the system client, theNewWorkerunwrap (a panic if it regresses), the lockNewClientholds againstClose, a closed factory not dialing, aCloselanding mid-dial stopping the retry, the batch activity closing its client,Processor.Stop, and the version-check body. Each was verified to fail when its fix is reverted.common/sdkverifies with goleak so a leak in that package surfaces there rather than in the functional suite.Potential risks
The connection lifecycle changes are the notable ones.
RPCFactory.CloseandclientFactory.Closerun as fx stop hooks registered where the factories are constructed, so they fire after the services that depend on them have stopped. Double-closing is handled: gRPC returnsCanceled, and the SDK's guard against a repeatedCloseis only safe sequentially, so the factory and a caller never close the same client.Sharing one local frontend connection means every local frontend, admin and operator client in a process multiplexes on it. Every dial gets
round_robinfromrpc.Dial's default service config, so the connection keeps one subconnection per frontend host and load spreads as hosts join and leave. Callers must not close it; the client bean receives it asgrpc.ClientConnInterface, which has noClose.Follow-up, not addressed here:
RPCFactory.Closedoes not shut down the local frontend HTTP client's idle connections. They are bounded by a 90s idle timeout and no leak-test scenario exercises them today, but now that thenet/http.(*persistConn)ignores are gone, a future test that fires a nexus callback would see them linger. Note this is not a one-lineCloseIdleConnections: in the membership-URL branch the transport is wrapped inroundTripper, which does not implement it, so that call would silently do nothing.Also reclassified: the
objectleakexpectation forsdkClient*was a TODO blamed on the goroutine leak. Those objects are the SDK's process-wide default data and failure converters and its shared sticky workflow cache, reached through the client's eager dispatcher, so the expectation is by design rather than a symptom.Two more follow-ups, both pre-existing and neither a regression here.
NewRemoteAdminClientWithTimeoutdials a connection per call and returns only the client, so the operator and admin handlers accumulate one connection per request for the process lifetime; tracking means those are closed at shutdown rather than never, which is what the leak suite sees, but bounding them needs that constructor to hand back a closer. AndServerImpl.Startdoes not unwind the children that started when a later one fails, so constructor-started client goroutines outlive a failed startup — production exits on that path, so it only bites in-process.