feat(pools)!: constructor-only configuration, one acquisition budget

[loks0n]

Follows the discussion on #93. Reviewed the whole surface against .claude/skills/package-design.

The problem

Five knobs decided how long pop() could block, and nothing named the total:

createConnection:  reconnectAttempts=3 x (connect + reconnectSleep=1s)
pop:               retryAttempts=3 x (that + synchronizedTimeout=3s + retrySleep=1s)

Against a 3s connect timeout that is ~44 seconds to fail one acquisition. Nobody chose 44 seconds — it is the product of defaults nobody multiplied out. And every consumer picks different numbers:

edge/app/init.php       setReconnectAttempts(15)   -> ~3 minutes
cloud/registers.php     setReconnectAttempts(1)    -> cache/lock/timelimit only
cloud/Factory.php       setReconnectAttempts(1)
skills-for-developers   setReconnectAttempts(5)->setRetryAttempts(3)   (docs teach it)

Appwrite capped cache, lock and timelimit and left the DB pools at defaults — the pools on the critical path for every request. That is what a default plus an optional override does, given time.

What changes

One budget. timeout is a required constructor argument and is the entire wait a caller may spend. It is the pool's only timeout, and it matches the name Adapter::pop() already used for the same concept. pop() creates when there is capacity, otherwise waits out the remainder, then throws. Required rather than defaulted, so it cannot be silently inherited, and it appears at every construction site where a reviewer sees it.

No creation retries, no replacement. Retrying a failed connect belongs in init: only the caller knows whether a failure is worth retrying, and a breaker composed there carries state between calls in a way an attempt counter cannot. packages/circuit-breaker is a sibling.

Retrying inside the pool was also anti-pooling — the capacity slot stays reserved for the whole attempt sequence, turning a shared resource into a private speculative wait.

Constructor-only readonly configuration. Twelve setters removed across Pool, Connection and Group, plus the getters that existed only to read them back.

Pool         23 -> 10 public methods
Connection    9 -> 3
Group         8 -> 5

Connection is final readonly. Identity, resource and pool fixed at construction, so it cannot be repointed while another coroutine holds it. The pool is no longer nullable, so returning a connection cannot fail for want of an owner — two exception paths gone.

Also found while reviewing

• destroy() created the replacement itself, so discarding a connection blocked on a connect and could fail for reasons unrelated to the connection being discarded. It now frees capacity and lets the next pop() create one, matching the pool's documented lazy initialisation.
• The Swoole adapter's docblock said 0 meant a non-blocking pop. Swoole reads a non-positive channel timeout as wait forever. With the retry loop removed this surfaced immediately as hung coroutines and channel is destroyed, consumers will be discarded warnings — the old loop had been hiding it. Now clamped to a single short poll.
• release() took a ?float $start so callers could hand telemetry back in. The pool measures use time itself now, keyed by connection id, and Group gets correct per-pool timing without threading timestamps.
• Adapter::pop() takes a float, so sub-second budgets survive the seam.
• The contract's docblocks apologised for arguments adapters ignore ($size "ignored", $timeout "ignored", synchronized "does not provide mutual exclusion"). They now state what a non-concurrent adapter honestly does — with one execution context nothing can return a resource while you wait, so returning immediately is the correct implementation. I concluded the contract itself is sound and left its shape alone.
• Runtime guards for size and acquireTimeout, throwing rather than annotating.
• PHPStan at level: max, clean, no ignores. Level max surfaced a real test bug: an untyped $resources[] widened a pool's generic to Pool<mixed>; fixed by returning the object directly rather than round-tripping through the array.
• Rector extends the root with the remaining stable prepared sets, matching storage.
• getters?/setters? added to the Vale vocabulary.

Result

             src   -516 / +288      net -228
             all  -1009 / +623      net -386
public surface     halved
test suite         47s -> 0.4s

The suite got 100x faster because the time it spent was retry sleeps.

78 tests, 318 assertions. bin/monorepo check pools, test pools and validate all pass. Vale reports the same 138 pre-existing findings elsewhere and none in packages/pools.

Deliberately not changed

• Group::use()'s parallel arrays are awkward but correct, and rewriting them is orthogonal to this review.
• isEmpty() / isFull() naming reads backwards at first (isFull() means nothing is checked out) but is consistent with count(), which reports obtainable connections. I reimplemented isFull() as count() === size for symmetry rather than renaming.
• release() stays public despite being @internal, because Group needs it.

Dependents

All three direct dependents are updated in the same commit and pass --linked.

• client and queue construct pools in their test suites. client's factory helper now takes a Closure to match the tightened init type. Their src/ needed nothing — both wrappers only use use(), which is unchanged.
• messaging needed a source change. It built its pool with the adapter passed as pool:, and sized it min(count($requests), MAX_CONCURRENT_REQUESTS) — which is zero for an empty batch. The new size guard catches that; a zero-capacity pool could never create a connection, it just failed later and less clearly. It now passes adapter:, clamps the size, and states its timeout (a slot per request, so acquisition never queues).
• Constraints bumped to ^2.0 in all three. Without that, a composer update after release would install pools 1.x against 2.x-shaped source.

As the tests workflow notes, changing a package and its dependents together leaves the registry combination unbuildable until the sibling releases — which is why the dependents run linked here rather than registry-resolved.

Downstream, after release

Four call sites outside the monorepo need mechanical updates once pools/2.0.0 exists: edge/app/init.php (which has the setReconnectAttempts(15) ≈ 3-minute case), cloud/registers.php, cloud/Factory.php, and the utopia-pools-expert skill doc, which currently teaches the removed knobs.

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

Constructor-only pool configuration replaces layered retry settings with a single acquisition wait timeout.

• Refactors pool capacity accounting, lazy connection creation, destruction, release timing, and telemetry.
• Makes connections immutable and updates Stack and Swoole adapters for floating-point timeouts.
• Updates client, messaging, and queue dependents for the pools 2.x API.
• Expands tests, documentation, PHPStan strictness, Rector configuration, and Vale vocabulary.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
packages/pools/src/Pools/Pool.php	Implements constructor-only configuration, immediate creation-error propagation, synchronized capacity reservations, lazy replacement, and pool-owned timing; the three previously reported defects are resolved or invalidated by the clarified timeout contract.
packages/pools/src/Pools/Connection.php	Replaces mutable connection wrappers with immutable owner-bound connections and direct reclaim/destroy delegation.
packages/pools/src/Pools/Adapter/Swoole.php	Widens pop timeouts to floats and prevents zero-budget channel operations from becoming unbounded waits.
packages/pools/src/Pools/Group.php	Adapts grouped acquisition and reverse-order release to immutable connections and pool-owned timing.
packages/messaging/src/Utopia/Messaging/Adapter.php	Migrates messaging pool construction to the 2.x API, guards empty request batches, and supplies an explicit acquisition timeout.
packages/pools/README.md	Documents the new constructor API, acquisition semantics, immediate creation failures, lazy replacement, and 1.x migration steps.

_{Reviews (4): Last reviewed commit: "feat(pools)!: constructor-only configura..." | Re-trigger Greptile}

[loks0n]

All three valid, and one was a bug I introduced. Fixed.

Destroy corrupts reserved capacity — real defect

Correct, and the worst of the three. The decrement sat outside the guard:

if (isset($this->active[$connection->id])) {
    unset($this->active[$connection->id]);
}
--$this->reserved;          // unconditional

Destroying the same connection twice, or one belonging to another pool, drove reserved below the truth, so count() over-reported and the pool would create beyond size. The release now happens only for a connection this pool actually holds.

Regression test added, and verified it catches the defect — with the old code a size-2 pool reports:

Failed asserting that 3 is identical to 2

Worth noting the 1.x destroyConnection() had the same unconditional decrement, so this is a long-standing bug rather than new — but it was in code I rewrote, so it belongs in this PR.

Creation failures wait unnecessarily — right, and it contradicted my own design

Also correct, and the sharper point is that it was philosophically inconsistent: falling through to wait out the budget after our own create failed is a retry in disguise, which is the exact thing this PR removes. It also contradicted the README.

init's exception now propagates untouched. Better than wrapping it, too — callers keep the original type, so a PDOException stays catchable as one instead of becoming a generic pool Exception. $creationError and its previous threading are gone.

Two tests asserted the old wrapping and were updated. One of them needed care: widening its catch to \Throwable would also have caught PHPUnit's own fail(), so it now counts throws instead of calling fail() inside the loop.

Creation bypasses acquisition deadline — real, but not enforceable

Correct that pop() can exceed timeout when init is slow. The pool cannot fix this: there is no way to interrupt a blocking PDO connect from outside it, and refusing to create once the deadline has passed would mean timeout: 0.0 could never create anything.

So this is a documentation defect rather than a code one — I claimed a budget the pool cannot enforce. Now stated honestly:

timeout bounds how long pop() waits for a connection to become available. It does not cover time spent inside init. The pool cannot interrupt a blocking connect, so give init its own connect timeout if you need the total bounded — for PDO that is PDO::ATTR_TIMEOUT.

The @param and the pop() docblock say the same, and pop() now documents @throws \Throwable for whatever init threw.

---

80 tests, 326 assertions. check pools clean at PHPStan level max with no ignores, Vale clean, and client, queue and messaging all still pass --linked.

[greptile-apps]

Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on August 8. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.

[abnegate]

Why float instead of int millis/nanos?

[loks0n]

I'd say because seconds is more human understandable, downsides regarding overflow/arithmetic don't apply as we don't transform it, just passed to sleep

[abnegate]

Can we keep the null check instead of type check?

[loks0n]

Rector rule does this, we can disable the rule monorepo wide

[loks0n]

#97

added some detail here for discussion