Connection pool sizing safety rests on invariants nothing enforces

[hongwei1]

Connection pool sizing safety rests on invariants nothing enforces

Summary

Our database connection handling is safe today because of three numbers that must stay
consistent with each other. That relationship is documented in one source comment and is
enforced nowhere — not by a startup check, not by a test, not by the props template.

This is structurally the same class of problem that produced the September 2023
StackOverflowError incident: the code was correct only while an unwritten invariant held,
and a later change removed the invariant silently.

The failure mode today is far less severe than it was in 2023 (a bounded timeout instead of
a dead JVM), so this is not urgent. It is worth fixing while the reasoning is still fresh.

Background: why the 2023 incident happened

Before HikariCP, connection pooling came from Lift's own StandardDBVendor / ProtoDBVendor.
Its newConnection waited for a free connection by recursing:

case Nil =>
  wait(50L)
  // ... optionally expand the pool ...
  newConnection(name)   // not tail-recursive

That recursion was not a defect on its own. It was safe because of an invariant supplied by a
different part of the framework: Lift Mapper tracks its connection in a ThreadLocal with a
reference count (DB.scala: threadStore, ConnectionHolder(conn, cnt, ...)), so nested
DB.use calls on the same thread reuse the same connection (cnt + 1) and the connection is
returned to the pool only when the count falls back to one.

The consequence: one synchronous request could only ever hold one connection. No request
held a connection while waiting for a second one, so hold-and-wait was impossible, every wait
was guaranteed to end, and recursion depth stayed proportional to how long the slowest
in-flight request took. Under that invariant the failure mode was slowness, not death.

Asynchronous execution removed the invariant without touching the pooling code. A Future
runs on a worker thread whose threadStore is empty, so it does not reuse the caller's
connection — it asks the pool for its own. One request could now hold one connection while
queueing for another. That is hold-and-wait. Once the pool could deadlock, a bounded wait
became an unbounded one, and the pre-existing recursion turned it into a stack overflow.

Relevant history:

Commit	Date	Change
02a80421d	2023-09-16	Copy Lift's StandardDBVendor into the codebase as CustomDBVendor (framework code cannot be patched in place)
17261bbf1	2023-09-18	Return Failure(...) instead of throwing RuntimeException on pool exhaustion
a41c0f178	2023-09-19	Raise the pool default from 4 to 20
eae8d829c	2023-10-08	Replace the copied vendor with HikariCP; delete DatabaseConnectionPoolScheduler.scala

What HikariCP fixed, and what it did not

HikariCP structurally eliminated the fatal mode. HikariPool.getConnection waits in a
do { connectionBag.borrow(timeout, MILLISECONDS) } while (timeout > 0L) loop and throws a
timeout exception when the budget is exhausted. Stack depth is constant; exhaustion surfaces
as SQLTransientConnectionException after hikari.connectionTimeout (currently 30s) rather
than as an unbounded recursion.

Hold-and-wait itself was not removed. It is still the documented behaviour, in
obp-api/src/main/scala/bootstrap/liftweb/CustomDBVendor.scala:31-34:

// Default 20: each request holds its transaction connection for its whole lifetime,
// so a pool of 10 exhausts at ~5 concurrent requests (rate-limit queries need a 2nd connection).
val maximumPoolSize = APIUtil.getPropsAsIntValue("hikari.maximumPoolSize", 20)

A request holds its transaction connection for its whole lifetime while a rate-limit query on
cache miss needs a second one. We mitigate by sizing the pool, not by removing the pattern.

Several changes have since attacked the duration side of hold-and-wait, which is the right
direction:

Commit	Change
05a7b71bd	Defer connection acquisition until the first DB call, so REST/SOAP-only endpoints never touch the pool
8f9ce5e15	Skip the transaction for GET/HEAD endpoints
25c3e2af1	Narrow transaction scope to business logic; move read-only validation outside it
54c271c70	Run blocking JDBC/gRPC on a dedicated bounded pool instead of the CPU-sized global one

The current problem

Three independently configurable numbers have to remain consistent:

Value	Default	Location
hikari.maximumPoolSize (main pool)	20	bootstrap/liftweb/CustomDBVendor.scala:34
stored_procedure_connector.poolMaxSize (second pool)	20	code/bankconnectors/storedprocedure/StoredProcedureUtils.scala:38
blocking_io_pool.size (threads that block on acquisition)	50	code/api/util/BlockingIoExecutionContext.scala:16

The relationship is stated only in a comment in BlockingIoExecutionContext.scala:10-13:

The default of 50 covers the sum of the Hikari maximumPoolSize values it serves
(main pool 20 + stored-procedure pool 20) with headroom; override via the
blocking_io_pool.size prop if the Hikari pools are resized.

Nothing enforces this. Grepping for blocking_io_pool outside its own definition returns
nothing: no startup validation, no test, no entry in the props template pairing the three.
An operator who raises hikari.maximumPoolSize to 40 to relieve pressure — the exact action
the first comment invites — silently under-provisions the thread pool that does the blocking
acquisition, and the mitigation makes throughput worse rather than better.

This is the 2023 shape again at a smaller scale: correctness resting on an invariant that
lives in a comment.

Proposed remediation

Ordered by cost. Any one of these closes the gap; the first is probably enough.

1. Validate at startup. On boot, if blocking_io_pool.size is smaller than
   hikari.maximumPoolSize + stored_procedure_connector.poolMaxSize, log a warning naming all
   three values and the prop to change. Cheap, no behaviour change, turns a silent
   misconfiguration into a visible one.

2. Derive instead of duplicate. Default blocking_io_pool.size to the sum of the two pool
   sizes plus headroom rather than to a hardcoded 50, keeping the explicit prop as an override.
   The invariant then holds by construction unless someone deliberately overrides it.

3. Document the three together. The props template currently introduces these values in
   separate places. A single block that states the relationship, with the recommended
   arithmetic, would make the coupling discoverable to whoever tunes the pool.

4. Add a regression test asserting the derived default satisfies the inequality, so a
   future change to either pool default fails the build rather than production.

Longer term, the hold-and-wait pattern itself is worth revisiting: if the rate-limit lookup on
cache miss can be served without a second connection while the request transaction is open,
the sizing rule stops being load-bearing at all.

References

• obp-api/src/main/scala/bootstrap/liftweb/CustomDBVendor.scala
• obp-api/src/main/scala/code/bankconnectors/storedprocedure/StoredProcedureUtils.scala
• obp-api/src/main/scala/code/api/util/BlockingIoExecutionContext.scala
• obp-api/src/main/scala/code/api/util/http4s/RequestScopeConnection.scala (states the
  underlying ThreadLocal problem in its header comment)
• Lift net/liftweb/db/DB.scala: threadStore (line 55), ConnectionHolder (line 135),
  reference-count reuse (lines 299-300), release only at cnt == 1 (line 312)
• HikariCP HikariPool.getConnection (lines 154-179)