feat(bench): execution-mode connection benchmark + DB-pool sizing guidance (#323)#324
Merged
Merged
Conversation
…dance (#323) ## Summary The "measure first" gate of #323 (elastic pools): a fair, reusable harness that answers "for the same offered concurrency, how many real Postgres connections does threads mode vs async mode actually hold?" — so we can guide users on DB-pool sizing per execution_mode before building any elasticity. The multi-agent design DISPROVED the naive "async saves connections" premise with code evidence: pgbus holds a pgmq connection only for the read/archive SQL round-trip, NOT the job body (executor runs perform_now with zero pgmq conns). So async's connection-density win is real only for I/O-light work; for DB-bound work async is connection-bound like threads. ## What ships - benchmarks/support/execution_mode_harness.rb — reusable, require-able harness. run_cell(mode, pool_size, concurrency, io_profile, job_count, client) -> Result. Fair by construction: identical offered load per pair (assert_fair_pair!), bounded in-flight window = pool capacity (both modes run the same concurrency), real pool checkout (client.pgmq.with_connection), peak_busy = size - available. - benchmarks/execution_modes_bench.rb — 8-cell matrix, rake bench:execution_modes. - docs/performance.md "Execution mode and DB pool sizing" — the guidance, with measured numbers and a sizing table. ## The numbers (measured, local Postgres, 240 jobs @ concurrency 12) async pool=3 io_light : peak_busy=3, ~222 job/s <- 12 fibers on 3 conns, full throughput threads pool=12 io_light: peak_busy=12, ~216 job/s async pool=3 db_bound : peak_busy=3, ~95 job/s <- under-provisioned: ~3.4x throughput collapse async pool=12 db_bound : peak_busy=12, ~324 job/s Key honest finding: NO cell hit pool_timeouts. pool_timeout (5s) dwarfs a 10-30ms checkout, so under-provisioning SERIALISES (throughput collapse), it doesn't error. The guidance says to watch throughput/latency, not error counts. ## Test Coverage - spec/pgbus/execution_mode_harness_spec.rb (16 unit, no DB, runs in CI): Result struct + derivations, IoProfile, summarize percentiles, assert_fair_pair! (anti-rigging), classify_error! tally (never swallow), assert_dedicated_path!, async_yield mode-awareness, PoolSampler busy=size-available + degraded-skip. - spec/integration/execution_mode_harness_spec.rb (3 DB-gated, :integration, auto-skip without PGBUS_DATABASE_URL): run_cell drives real jobs, bigger pool holds more connections (fair pair), async fibers share a small pool on io_light. ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec spec/pgbus — 3036 examples, 0 failures - [x] DB-gated specs pass against real Postgres - [x] docs rake lint clean - [x] rake bench:execution_modes runs clean; numbers captured in docs Refs #323
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesExecution mode benchmarking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Script as execution_modes_bench.rb
participant Harness as ExecutionModeHarness
participant Pool as execution pool
participant Client as Pgbus::Client
participant Sampler as PoolSampler
Script->>Harness: run_cell(mode, pool_size, concurrency, io_profile, job_count, client)
Harness->>Harness: assert_dedicated_path!(client)
Harness->>Harness: warmup!(pool, client, io_profile)
Harness->>Sampler: start_sampling(...)
Harness->>Harness: drive(pool, client, io_profile, job_count)
Harness->>Client: pgmq.with_connection { SELECT pg_sleep(...) }
Harness->>Sampler: stop_sampling(...)
Harness->>Harness: summarize(...)
Harness-->>Script: Result
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/execution_modes_bench.rb`:
- Around line 66-72: The free_backends helper can leak the raw PG connection
when either conn.exec call raises, because conn.close is skipped in that path.
Update free_backends to always close the connection using an ensure-style
cleanup around the PG.connect block, so the connection is released even if exec
fails, and keep preflight! relying on that helper without needing extra cleanup.
In `@benchmarks/support/execution_mode_harness.rb`:
- Around line 125-133: The sampler thread started by start_sampling can outlive
failures in warmup!/drive/summarize because stop_sampling is not guaranteed to
run. Update the execution flow in this harness so the sampling thread is always
stopped in an ensure block, even when drive raises, while keeping the existing
pool.shutdown and pool.wait_for_termination cleanup intact. Use the
start_sampling, stop_sampling, and summarize flow in execution_mode_harness to
place sampler cleanup alongside pool cleanup.
- Around line 172-179: The async_yield helper still branches on Async::Task and
calls task.sleep, but that API is deprecated and unnecessary. Simplify
async_yield to use plain sleep(seconds) unconditionally, since Kernel#sleep is
fiber-aware under the scheduler; remove the Async::Task.current? check and the
task.sleep path while keeping the async_yield method as the entry point.
In `@spec/integration/execution_mode_harness_spec.rb`:
- Around line 66-89: The async integration example in ExecutionModeHarness is
effectively always skipped because async_gem_available? depends on a gem that is
not declared for test execution. Update the test setup so the async gem is
available in the dev/test dependency set, and keep the existing
async_gem_available? / run_cell coverage in execution_mode_harness_spec
exercising the async path instead of silently skipping it.
- Around line 48-64: The peak_busy assertion in the execution_mode_harness_spec
is timing-sensitive because it compares two separate run_cell samples captured
by PoolSampler under load. Update the spec around ExecutionModeHarness.run_cell
and assert_fair_pair! to avoid a strict equality-style timing check, either by
adding a small tolerance/window to the peak_busy comparison or by softening it
to a less flaky expectation. If you keep the check, document in the example that
this db_bound case is intentionally best-effort and may occasionally vary under
CI contention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 127f939c-ffd7-4a7f-89c7-09105ccc5fc3
📒 Files selected for processing (8)
CHANGELOG.mdRakefilebenchmarks/connection_pool_bench.rbbenchmarks/execution_modes_bench.rbbenchmarks/support/execution_mode_harness.rbdocs/performance.mdspec/integration/execution_mode_harness_spec.rbspec/pgbus/execution_mode_harness_spec.rb
…ky spec Five CodeRabbit findings on the execution-mode benchmark: - free_backends leaked the raw PG connection if a query raised (conn.close was skipped, and preflight! rescues but doesn't close). Moved close to an ensure. - run_cell leaked the sampler thread if warmup!/drive/summarize raised: the ensure tore down the pool but never stopped the sampler, which then polled pool_stats forever. Hoisted sampler_thread and stop it in the ensure. - Async::Task#sleep is deprecated in async 2.x (the gem's own @deprecated tag) in favor of Kernel#sleep, which the fiber scheduler already intercepts (kernel_sleep hook) and yields to the reactor — verified: two concurrent sleep(0.2) fibers finish in ~201ms, not 400ms. Deleted the over-engineered async_yield method; run_job now just calls sleep. The async cells still share a small pool (async pool=3 io_light: peak_busy=3, 12 fibers) — behavior unchanged, confirmed against a real DB run. - Hardened the flaky integration assertion: replaced the cross-run peak_busy >= peak_busy timing comparison (could flip if the 5ms sampler missed a peak under GC) with deterministic per-pool ceiling invariants (peak_busy between 1 and pool_size). - Made the async DB spec honest: `async` is not a declared dependency (present only transitively via falcon/async-http dev deps), so the skip message now spells out that a SKIP is not a pass and async execution mode requires adding the gem. ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec spec/pgbus — 3037 examples, 0 failures - [x] DB-gated harness specs pass; rake bench:execution_modes numbers unchanged
6 tasks
mhenrixon
added a commit
that referenced
this pull request
Jul 6, 2026
…#323 spike) (#325) ## Summary A SPIKE proving that the dedicated streams DB pool can be resized under live load by hot-swapping the PGMQ::Client — the feasibility gate before building any elastic control loop. The #324 benchmark showed async workers collapse ~3.4x under a DB-bound burst on a small pool; this proves we can grow/shrink the pool to fix that, correctly and cheaply. connection_pool 2.x cannot resize in place, so growing = build a new PGMQ::Client + atomically swap the reference + drain and close the old one. This ships that swap primitive (opt-in, default-off, no auto-call — the control loop is deferred). ## The correctness spine - **Atomic swap:** all four streams-pool readers (send_stream_message, with_streams_connection, streams_pool_stats, close) go through a guarded accessor backed by Concurrent::AtomicReference — the dedicated path has no mutex, so a bare ivar swap is a data race. Swaps/close serialize on a mutex; reads stay lock-free (one volatile load). - **Drain on an in-flight COUNTER, not available==size (the load-bearing fix the design review caught):** pgmq's with_connection retries a lost connection (checkout->checkin->checkout), so available momentarily == size while a produce is mid-flight. Closing there would raise PoolShuttingDownError on the retry = a genuinely LOST durable broadcast (the PgBouncer/failover mid-INSERT case). Instead ResizablePool brackets each reader op with an AtomicFixnum on the captured pool and drains on THAT reaching zero — immune to the retry, and independent of the connection_pool version. - **Durable broadcasts loss-safe by construction:** produce captures the pool once, runs its whole retrying INSERT on it; both pools connect to the same DB; close waits for inflight==0 first, so a pool is never shut under a live produce. - **Retire = PGMQ::Client#close** (pgmq's own safe shutdown drain: idle conns close now, in-flight self-close on checkin) — never Thread#kill, bounded log-and-abandon like OutboundPump #321 B3. - **Shared-AR path:** hard no-op (streams pool aliases the job pool, pool_size 1). ## Measured (real DB, live load: 4 producers + 1 reader, grow 5->12, shrink 12->5) swap cost : ~12 ms total, ALL drain+close, build ~= 0 (lazy pool) zero loss : 11,790 produced == 11,790 landed zero leak : app-scoped backends 9 -> 9 zero races : 0 errors, 0 pool-closed final size : restored to 5 => ALL GATES PASS — elastic streams pools are FEASIBLE. Bench: benchmarks/pool_swap_bench.rb (rake bench:pool_swap). ## Test Coverage - resizable_pool_spec (13 unit, no DB): ref swap, inflight bracketing, drain waits on inflight NOT available==size (the data-loss regression test), drain-timeout log-and-abandon, telemetry, concurrent-swap serialization, close_current alias guard. - client_swap_spec (8, mocked PGMQ::Client): resize builds+swaps+routes to new pool, closes old, no-op on unchanged size / shared-AR path, invalid size, concurrent reader safety, close-after-swap. - streams_pool_swap_spec (DB-gated): zero lost broadcasts under concurrent grow, zero leaked connections across grow+shrink. - Existing client_spec (301) + streamer (307) pass unchanged — swap-off is byte-identical (one atomic read + counter bump per op). ## Deferred (out of scope) The control loop — saturation sampling, hysteresis, grow/shrink policy, fleet-wide connection budget, any automatic resize call. This spike decides the swap is cheap+correct; the loop is a follow-up. ## Verification - [x] bundle exec rubocop passes (7 files) - [x] bundle exec rspec spec/pgbus — 3058 examples, 0 failures - [x] DB-gated swap spec + bench pass against real Postgres Refs #323
7 tasks
mhenrixon
added a commit
that referenced
this pull request
Jul 6, 2026
) * feat(streams): autoscale config + P1/P2 prerequisites (#323) Groundwork for self-tuning streams-pool autoscaling: - Config (opt-in, default off): streams_pool_autoscale (bool), streams_pool_max (optional hard cap, nil = dynamic fair-share ceiling), streams_pool_autoscale_interval (1.0s), streams_application_name ("pgbus_streams"). streams_pool_size stays the baseline/shrink-floor. Validation rejects an inverted ceiling (max < size) at boot. - P1: tag the streams pool connection with a per-process application_name ("pgbus_streams_<pid>") so the autoscaler can count peer processes via pg_stat_activity — DISTINCT application_name is then an exact process count. - P2: build the streams pool from streams_connection_options (not the job connection_options) so a separate/direct streams DB (streams_database_url/ host/port) is honored — fixes a latent wrong-DB bug in the existing streams pool that only surfaces with a separate streams database. Config-drift + full client/config specs green; no behavior change with autoscale off. * feat(streams): self-tuning PoolAutoscaler + streamer wiring (#323) The control loop on top of the #325 hot-swap primitive. One per web process (a sibling of Heartbeat in the Streamer Instance), opt-in and default-off. Decision function (per tick, priority order): 1. EMERGENCY SHRINK — if live DB free connections are critically low (free < max(5, 0.05·maxc)), resize to baseline NOW, overriding busy_ratio AND cooldown. Keys off `free` (an external DB fact) so it is immune to the post-swap lazy-pool transient (BUG-0) and may fire during cooldown. 2. GROW — busy_ratio ≥ 0.85 sustained 3 samples AND free > grow reserve (max(20, 0.20·maxc)): grow into a bounded FAIR SHARE of headroom, free/(peers·1.5), claiming ¼, capped by STEP_MAX(4) and floor(free/2). Peers = DISTINCT application_name LIKE 'pgbus_streams_%' — the exact live process count (zero-config). 3. SHRINK — busy_ratio < 0.30 sustained 20 samples: step toward baseline. 4. else HOLD. Proven safe (see spec): no multi-process exhaustion even in a synchronized cold-boot herd (four stacked grow guards bound the worst case; grow collapses next tick as pg_stat_activity reflects reality); no grow↔emergency limit cycle (GROW_RESERVE ≥ 4× EMERGENCY_MARGIN → ~15% dead-zone); BUG-0 immunity via the cooldown gate. Fail-soft: a probe timeout → HOLD (never blocks the loop). DB I/O is behind an injected HeadroomProbe seam, so the whole policy is DB-free unit-tested (17 cases incl. convergence-under-concurrent-growth and emergency-shrink-during-cooldown) with a scripted probe + injected clock. Wiring: composed only when streams_pool_autoscale AND !shared_connection? (nil — no thread — otherwise). Started after the heartbeat; stopped FIRST in shutdown! so the actuator quiesces before teardown. Client#with_streams_connection made public so the probe can read pg_stat_activity through the streams pool. Default-off = @Autoscaler nil, zero new threads, byte-identical. Streamer + client specs green (464 examples). * feat(streams): dedicated headroom probe + DB-gated autoscale proof (#323) Two fixes the DB-gated integration test forced, plus the test itself: - FIX (P1 URL encoding): tag_application_name space-appended `application_name=` onto the conn string, which is invalid for postgres:// URI form (it already carries ?options=... from apply_connection_bounds — libpq rejects the second bare key=value in the query). Now mirrors append_connection_bounds: URI form uses ?/& separators, keyword form uses a space. The mocked unit test couldn't catch this (fake PGMQ::Client never parses the string); the DB test did, at construction. - FIX (probe starvation): the HeadroomProbe read through the streams pool. At busy_ratio ~= 1.0 — exactly when the loop wants to grow — its own checkout times out (0 slots free), returns nil, and the loop can NEVER grow. Proven with a churn harness. The probe now uses its OWN dedicated single connection (Client#build_streams_probe_connection: lazy, reconnect-on-failure, tagged with pgbus_streams_<pid> so it counts as this process's peer). One extra backend per web process — the honest cost of a headroom monitor that must work under saturation. This reverses the earlier "reuse the pool connection" choice, which assumed the probe could get a slot; it provably can't at 100%. - DB-gated spec (auto-skips without PGBUS_DATABASE_URL): drives real #tick against a real Client + real HeadroomProbe reading live pg_stat_activity. Proves the pool GROWS under sustained saturation (respecting the hard cap), SHRINKS back to baseline when idle, and raises no PoolTimeout. Injects the clock to fast-forward past the 15s cooldown while all DB I/O stays real. Unit + streamer + client + config specs green (779). * feat(streams): autoscaler bench + docs-kit guidance + CHANGELOG (#323) - benchmarks/pool_autoscale_bench.rb (rake bench:pool_autoscale): measures the tick cost (~0.3ms — negligible at a 1s interval) and grow-under-load against a real DB. Honest framing: it proves the loop is cheap and reacts, NOT a per-op speedup — whether autoscaling beats a well-sized static pool is a burstiness question (#324). - docs-kit Phlex page (performance_tuning.rb): user-facing "Streams pool autoscaling" section — when to enable (bursty SSE), the zero-connection-config model, self-protection (emergency shrink), and the PgBouncer/direct-connection caveat. The contributor markdown runbook (docs/performance.md) gets only the two new bench names, per the docs split (user guidance lives in Phlex). - CHANGELOG: the full feature entry under Unreleased/Added. * refactor(streams): autoscaler as periodic maintenance on the LISTEN connection (#323) Redesign from PR review feedback: no extra connection, no extra thread, a pghero-style periodic check instead of a 1s control loop. Before: a per-process autoscaler thread with its OWN dedicated probe connection, polling every 1s with sustain/cooldown hysteresis. After: - PoolAutoscaler is a PURE DECISION OBJECT — evaluate(headroom) reads streams_pool_stats, decides emergency/grow/shrink/hold, resizes. No thread, no connection, no cooldown, no sustain counters. - The headroom query runs as a throttled MAINTENANCE task on the streamer's EXISTING idle LISTEN connection (Listener gains `maintenance:` + `clock:`, runs it in its health-check window on the listener thread — the only thread that may touch that non-thread-safe connection). ZERO extra connections, zero extra threads. - Cadence: streams_pool_autoscale_interval 1.0s -> 300s (5 min). The slow interval is itself the debounce, so per-sample hysteresis and the 15s cooldown are gone: one grow/shrink step per check; a sustained burst converges over a few checks. This also DISSOLVES the probe-starvation problem #327 had to work around: the query runs on the LISTEN connection (not the streams pool) and evaluate reads pool stats without a checkout, so a 100%-saturated pool no longer blocks the decision — no dedicated probe connection needed. Client#with_streams_connection reverts to private; build_streams_probe_connection is deleted. The fair-share + emergency-shrink SAFETY math and the P1/P2 prerequisites are unchanged. Pure-publisher workers (no streamer) aren't autoscaled — a publisher-triggered check is a planned follow-up; documented, mitigate with a larger static streams_pool_size. Bench: check cost ~0.34ms; grow 3->7->10 (capped). Unit 3091, streamer 255, docs 57, DB-gated 4 — all green. * fix: address PR review feedback (#327) - emergency_shrink: when the DB is critically low on connections but the shrink resize is a no-op (unchanged/shared), log a warning and return :hold instead of silently reporting :emergency_shrink — a failed emergency shrink is the one case where visibility matters most. Adds a covering unit test. - pool_autoscale_bench: capture streams_pool_stats[:size] once per loop iteration instead of querying it twice.
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.
Summary
This is the "measure first" gate of #323 (elastic pools). Before building any elasticity, we needed a fair way to answer: for the same offered concurrency, how many real Postgres connections does threads mode vs async mode actually hold, and what does each deliver? — so we can guide users on DB-pool sizing per
execution_mode.The multi-agent design disproved the naive "async saves connections" premise with code evidence: pgbus holds a pgmq connection only for the
read_batch+archiveSQL round-trip, not the job body (executor.rbrunsperform_nowwith zero pgmq connections). So async's connection-density win is real only for I/O-light work; for DB-bound work async is connection-bound exactly like threads.What ships
benchmarks/support/execution_mode_harness.rb— a reusable,require-able harness.run_cell(mode, pool_size, concurrency, io_profile, job_count, client) → Result. Fair by construction: identical offered load per pair (assert_fair_pair!), a bounded in-flight window equal to the pool capacity (so both modes run at the same concurrency — this also normalises thatAsyncPool#postraises past capacity whileThreadPoolqueues), the real pool checkout (client.pgmq.with_connection), andpeak_busy = size − availablesampled off the pool's own counters (race-free, process-scoped — nopg_stat_activitypolling).benchmarks/execution_modes_bench.rb— an 8-cell matrix,rake bench:execution_modes(indb_benches, requiresPGBUS_DATABASE_URL).docs/performance.md→ "Execution mode and DB pool sizing" — the guidance, with the measured numbers and a sizing table.The numbers (measured locally, 240 jobs @ concurrency 12)
Key honest finding: no cell hit
pool_timeouts. Becausepool_timeout(5 s) dwarfs a 10–30 ms checkout, under-provisioning serialises (throughput collapse), it doesn't error. The docs guidance says to watch throughput/p95, not error counts — a more useful (and less folkloric) result than "tiny pool → timeouts".Test plan
bundle exec rspec spec/pgbus— 3036 examples, 0 failures (16 new unit tests, no DB, run in CI: struct + percentiles, offered-load fairness, error-tally-never-swallow, dedicated-path guard,async_yieldmode-awareness,PoolSamplerbusy = size − available+ degraded-sample skip):integrationspecs (auto-skip withoutPGBUS_DATABASE_URL) —run_celldrives real jobs, bigger pool holds more connections (fair pair), async fibers share a small pool on io_lightbundle exec rubocopclean; docsrake lintcleanrake bench:execution_modesruns clean against real Postgres; numbers captured in docsThe harness logic runs in CI; the numbers come from a manual DB run (run-and-report, never a CI gate — matches the project's "no hard CI perf gate on a flaky threshold" rule).
Deviations & judgment calls
statsreturns{size:, available:}only (noidle/createdkey), so an early-designpeak_createdmetric was algebraically unrecoverable and would have read a nonexistent key. Dropped; the metric ispeak_busy/steady_busywherebusy = size − available.AsyncPool#postraisesExecutionPoolErroroncecapacityunits are in-flight, whereasThreadPoolqueues unlimited posts. The first real run crashed on this. Fix:drive/warmup!submit behind aConcurrent::Semaphoresized to the pool capacity — which is also the fairer model (at mostcapacityjobs run at once in both modes; threads' queue vs async's reject is an implementation detail the driver normalises).pool_timeouts— under-provisioning serialises rather than errors (see above). The guidance was written to that reality, not to the assumed "tiny pool → timeouts".Client#pool_timeout_error?is private. Rather than editclient.rb(no-production-edit rule for a benchmark PR), the harness replicates the one-line marker check (msg.downcase.include?("connection pool timeout")), matchingPOOL_TIMEOUT_MARKERexactly.ASYNC_POOL_CONNECTIONS=3heuristic the bench exists to validate); CI/DB split (logic in CI, numbers from a manual run). Plan defaults:cpu_boundcell dropped (async serialises CPU on one reactor thread — labeling it "≈ threads" would mislead); a stale "fibers share a handful of connections" line inrunning_workers.rbis a separate follow-up docs fix.Refs #323.
Summary by CodeRabbit
rake bench:execution_modesto measure real PostgreSQL backend connection usage across execution modes and pool sizes.