feat: add jitter to max lifetime in connection pool

[priyanshuvaya]

Connection Storm Prevention with Jitter in Connection Pool #1495

Introduce connection jitter to randomize connection timing:

1. Add a jitter parameter to PoolOptions
2. When fetching a new connection from pool, add jitter to maxlifetime
3. This spreads out connection creation/renewal across a time window

Fix: #1495

[priyanshuvaya]

@vietj Changes for the jitter have been added. I have some doubts regarding the test cases. One way to verify this is by creating a pool and fetching a connection. Then, using a timer, check every few milliseconds whether the connection is still active. Once it closes, record the time. Repeat this process three times and ensure that each recorded time is different.

[vietj]

how can we test this ?

[vietj]

@priyanshu-d11 I think we could test with a TCP server that just does nothing and create a pool of 100 connections and perform a connect storm and measure the connection time between the client and server and ensure that we get a spread of the collected latencies ? we could add a min jitter time that could be helpful for testing, e.g. if we set 30 milliseconds then we can check that there are no latencies under 30 milliseconds

[priyanshuvaya]

I have added a test case. Here's what the test does:

• We set up a pool with a maxlifetime of 2 seconds and a jitter of 400ms
• Then we keep track of when connections change (by watching their PIDs)
• We verify that the time between connection changes is somewhere between 1.6s and 2.4s (2s ± 400ms)
• We run it twice to make sure it's consistent

Let me know if you want me to explain any part in more detail or if you have any other idea to test this scenario.

@vietj

[chirag-manwani]

remove star import

[vietj]

we also need a jitterTimeUnit property which should be by default Willis

[priyanshuvaya]

added

[vietj]

I am looking at the contribution, how does it work initially when the pool is empty and many connections attempts can be made simultaneously ?

[chirag-manwani]

Suggested change

	long randomizedJitter = (maxLifetime > 0 && jitter > 0 && maxLifetime > jitter) ? ThreadLocalRandom.current().nextLong(-jitter, jitter + 1) : 0;
	this.lifetimeEvictionTimestamp = maxLifetime > 0 ? System.currentTimeMillis() + maxLifetime + randomizedJitter : Long.MAX_VALUE;
	this.lifetimeEvictionTimestamp = maxLifetime > 0 ? System.currentTimeMillis() + maxLifetime + Math.max(0, ThreadLocalRandom.current().nextLong(0, jitter)) : Long.MAX_VALUE;

[priyanshuvaya]

added this with nextLong(0, jitter+1)

[priyanshuvaya]

@vietj
Modified the code to have jitter added to max lifetime, rather than ranging from (-jitter, jitter+1).

Added a test case which does the following:
This test verifies that our connection pool correctly enforces its configured maxLifetime (5000 ms) and jitter (1 second) settings. It does so by:

Acquiring 50 connections in parallel.

1. For each connection, query the backend process ID (PID) and its connection start time.
2. Closing the connection and then polling PostgreSQL’s pg_stat_activity until the connection is no longer visible (i.e., it has been closed by the pool).
3. Recording the time when the connection is detected as closed.
4. Calculate each connection's duration (close time – start time).
5. Grouping these durations into buckets (each with a width of jitter/5 = 200 ms) based on an offset from max lifetime.
6. Finally, asserting that at least 5 distinct buckets are populated ensures that the jitter setting effectively introduces a spread in connection lifetimes.

[priyanshuvaya]

@vietj While reviewing the test cases, I came across one for a maxLifetime where a connection is created and immediately closed. The test correctly verified that the connection was closed after the expected duration. I tried replicating this by setting the pool size to 10 and requesting multiple connections, but I wasn't able to get it working as expected. Is there a better approach to test this scenario?

[priyanshuvaya]

@vietj Have you checked the test case?

[priyanshuvaya]

@vietj Have you got time to review the test case added?

[chirag-manwani]

@vietj A polite bump up for the review.
I am sure you are aware what this PR does as it is described in the linked issue, and its implementation is fairly straight forward. I'll just add on how it is tested here.
The test case that @priyanshu-d11 has added, tries to create multiple connections at the same time, this is to keep the connection start time as close as possible. The goal is to see an even distribution of connection end time by using the jitter setting. This is done by maintaining buckets of fixed time windows starting from (start_time + max_lifetime) to (start_time + max_lifetime + jitter) and checking that there are non-zero number of connections in all buckets. This test case ensures that if jitter is used, then connections are randomly (and hopefully evenly) distributed across time so that new ones are created at different times.

Please let us know if the above implementation looks good.

Thank you for your time.

[surajgour-d11]

@priyanshu-d11 Do you think this value should be subtracted?
Since, the total eviction time should never exceed maxLifeTime (users expectation)
If yes, need to update corresponding test cases.

[1991santhu]

Not trying to add noise to a review that is already long, but @vietj asked something on 2025-03-20 that I do not think got answered, and it looks load-bearing for whether this closes #1495:

how does it work initially when the pool is empty and many connections attempts can be made simultaneously ?

I think the honest answer is that it does not, and that this is fine — because there are two distinct storms in the original report and this PR addresses one of them.

Recycle storm. Every connection in a pool is created at roughly the same moment and shares one maxLifetime, so they all expire together, close together, and reconnect together. Jittering maxLifetime breaks that, and it does so permanently — once the first recycle is spread, subsequent generations inherit the spread. This is what the PR fixes and it is the more important of the two, because it recurs for the life of the process rather than once.

Initial-connect storm. At startup the pool is empty and N acquisitions arrive together, so N connects are attempted together. maxLifetime jitter cannot help here, because no connection has a lifetime yet. This is the one @priyanshuvaya's original report opens with — 10 instances × 8 verticles all connecting at once — and it stays unaddressed by this change.

Worth separating them explicitly, because they need different mechanisms and have different severity. The initial storm is bounded and self-resolving: it happens once per process start, and DNS-based reader selection re-balances as connections are recycled thereafter — which is precisely what the jitter in this PR enables. The recycle storm is unbounded: without jitter, the skew re-establishes itself on every generation, so a pool that started unlucky stays unlucky forever.

That ordering seems like an argument for merging this rather than widening it. If the initial-connect case still matters afterwards, the mechanism for it is different anyway — a small spread on the acquisition path rather than on expiry — and would be much easier to reason about once lifetimes are no longer synchronised.

On the test difficulty in this thread: measuring elapsed time between connection changes is inherently flaky under CI load. An alternative that avoids wall-clock assertions is to compute the scheduled expiry deltas directly and assert that the set of values is spread rather than identical — no timing, no sleeping, and it fails for the right reason if jitter regresses. Whether that is reachable depends on how the expiry is stored, which I have not checked.

The PR also currently shows as conflicting, so it likely needs a rebase before it can be looked at either way.