fix: F-2026-18797 | [Dual Defense] Universal Client Chain Registry Lifecycle: Stale-Remove Deadlock and Unclosed DB Handles - #325
Merged
Conversation
…tabases (F-2026-18797)
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.
F-2026-18797 Chain registry lifecycle: stale-remove deadlock and unclosed DB handles
Two independent defects on the same path, both confirmed.
Variant A, deadlock
The stale sweep held
chainsMu.RLock()and calledremoveChain, which takeschainsMu.Lock().sync.RWMutexis not reentrant, so the refresh goroutine parks forever while still holding the read lock, and every later reader of the registry blocks behind it.Fixed by collecting the stale ids under the read lock, releasing, then removing. Extracted as
removeStaleChainsso the behaviour is directly testable rather than reachable only through a network fetch.Swept the rest of the file: no other read-locked section calls a method that takes the write lock.
Variant B, leaked handles
getChainDBopens a fresh pool on every call with no cache. Three paths inaddChainreturn after opening without closing (unsupported VM type,NewClientfailure,Startfailure), andremoveChain/StopAlldropped map entries without closing either.A chain that cannot start is retried every refresh tick, so a persistent misconfiguration grows one pool per tick, each holding up to
MaxOpenConns(10)descriptors.Fixed by tracking the handle per chain and closing it on removal and shutdown, plus a deferred close that releases the handle on every path where ownership does not pass to a live client.
A fourth path the finding does not list
ensurePushChainopens its own database and writes the client straight into the registry, bypassingaddChain. Same defect on two failure returns, and on success the handle was never recorded, soStopAllleft the push chain database open.Found by checking what else opens a database rather than stopping at the paths the finding named. Fixed the same way.
Tests
-race.The database opener is now a field so tests can observe handle lifecycle. The first version of the leak test asserted on a map count and passed under mutation, because a failed add never reaches the map, so the count was unchanged whether or not the handle leaked.