feat(benchmark): pool benchmark harness with standalone mock server and per-API QPS tracking - #1518
feat(benchmark): pool benchmark harness with standalone mock server and per-API QPS tracking#1518Pangjiping wants to merge 16 commits into
Conversation
…er-API QPS tracking Standalone Go mock server (lifecycle + execd API per specs/) with configurable per-route response time (uniform/fixed/lognormal), fault injection (create failure, execd failure, poisoned sandboxes), server-side TTL expiry, and exact per-second per-API QPS tracking with full time series. Kotlin/JVM driver runs 7 reproducible scenarios against the mock (cold-start, warm-latency, steady-state, replenish-lag, failure-injection, stale-idle, idle-expiry) and writes JSON + Markdown reports including per-scenario server-side QPS. run.sh orchestrates SDK publish, mock start, and driver run; the mock is reusable from any SDK via ConnectionConfig.
…e build Replace the mavenLocal-published artifact dependency with includeBuild on sdks/sandbox/kotlin so the benchmark always runs the checked-out SDK code. Drops the publishToMavenLocal step from run.sh and the -PsandboxVersion injection.
…ainingTtl, primaryLockTtl, degradedThreshold) Map a production large-pool / high-frequency profile 1:1 onto driver options; document the example profile and scale caveats in README.
…ofile Keeps the high-frequency acquire case realistic without the 1-5s readiness probe capping the sustainable acquire rate.
…/replenish metrics PoolProbe samples JVM threads, heap/GC, and pool health every 500ms during warm-latency and steady-state. Scenarios now report successRate, replenishRatePerSec/killRatePerSec, directCreateRatio, and the client probe block; steady-state idle stats come from the probe.
run.sh creates results/run-<ts>/ holding the mock log, mock config, driver args, reports, per-scenario server timeseries CSVs (alive + per-API QPS) and client probe CSVs (threads/heap/idle/inFlight), plus a raw end-of-run mock stats snapshot. Adds failure classification (readyTimeout/poolNotRunning/ storeUnavailable/...), resize, shutdown-race, store-outage scenarios, partial poisoning, and mock alive gauge (max + per-second series).
…ation awaitTermination(15min) silently truncated 30-min runs to ~15min. Wait for duration+5min and report the actual loader duration so achieved rate is computed against real runtime.
…ncy scaling PoolWarmupDiagnostics records warmup queue-wait, createOneSandbox duration, commit duration, reconcile-tick cadence, in-flight warmup peak, and create failure reasons. Cold-start scenario reports them; --shared-connection-pool flag added to test connection-reuse hypotheses. Diagnosis: at wc=1000 the submission chain fully saturates (in-flight peak 1000, queue wait ~20ms); fill degradation is caused by per-sandbox fresh-TCP connection churn producing intermittent Connection reset failures against the mock listener (accept backlog), amplifying attempts via retries.
…on-pool-size) Replace the boolean flag with a configurable OkHttp ConnectionPool idle size. Evidence: at wc=1000, pool size 0/100/200/500 -> fill 19.9s/9.1s/7.1s/4.3s, connection-reset failures 8024/2696/1341/0, attempt amplification 5x/2.3x/ 1.7x/1x. A properly sized shared pool eliminates the TCP churn entirely.
… not strings The diagnostics data classes fell into the generic toString() branch, so queueWaitMs/createDurationMs etc. were strings in report.json. Convert them explicitly so the warmupPipeline block is machine-readable.
github-actions
Bot
added
sdk/java
sdks
size/L
…pool guidance Adds --shared-connection-pool-size to the driver options table and a 'Warmup throughput and connection reuse' section describing the connection-reset/retry-amplification finding at high warmupConcurrency, the pool-size sweep evidence, and configuration guidance for benchmark and production (including the upcoming SDK default from opensandbox-group#1517).
…ent pool guide Move the problem description, sweep evidence, and configuration guidance (shared ConnectionPool via ConnectionConfig, sizing rule of thumb, upcoming SDK default) to docs/guides/client-pool.md. The benchmark README keeps only a pointer plus the --shared-connection-pool-size repro flag.
github-actions
Bot
added
the
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e533355c29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| private val lock = Any() | ||
| private val queueWaitNanos = ArrayList<Long>() | ||
| private val createNanos = ArrayList<Long>() | ||
| private val commitNanos = ArrayList<Long>() | ||
| private val tickIntervalNanos = ArrayList<Long>() | ||
| private val tickDurationNanos = ArrayList<Long>() | ||
| private val submitBursts = ArrayList<Int>() | ||
| private val failureReasons = LinkedHashMap<String, Long>() |
There was a problem hiding this comment.
When any production JVM uses SandboxPool, the new unconditional PoolWarmupDiagnostics.record* calls append every reconcile/warmup event into these global ArrayLists and the failure map, with no cap and no automatic reset. Because the object lives in the published SDK and is shared across all pools, a long-lived service that continuously replenishes sandboxes will retain one or more samples per warmup forever even if nobody reads diagnostics, so this is a memory leak plus a global lock hot path rather than the documented no-op.
Useful? React with 👍 / 👎.
| "acquires" to acquires, | ||
| "successRate" to successRate(latencyStats), | ||
| "serverCreatedDelta" to createdDelta, | ||
| "hitRatio" to (1.0 - createdDelta.toDouble() / acquires).coerceIn(0.0, 1.0), |
There was a problem hiding this comment.
In the warm-latency scenario, each successful idle acquire is immediately killAndClosed, so the pool asynchronously creates a replacement and stats.created increases even though the acquire was an idle hit. With a fully warm pool this formula can report a low or zero hitRatio purely because refills completed before the stats read, making the primary warm-latency metric depend on background refill speed instead of whether acquires fell through to direct create.
Useful? React with 👍 / 👎.
| "directCreateRatio" to | ||
| ((createdDelta - killedDelta).coerceAtLeast(0).toDouble() / acquires.get().coerceAtLeast(1)), |
There was a problem hiding this comment.
For steady-state, a direct-created sandbox is also killed by the worker after use, so it increments both createdDelta and killedDelta; subtracting kills therefore cancels out the very direct creates this metric is meant to count. In an empty-pool or overloaded run where every acquire falls through to direct create and is then killed, this reports directCreateRatio as 0 instead of 1, hiding the failure mode the benchmark is supposed to surface.
Useful? React with 👍 / 👎.
…ized)
--shared-connection-pool-size now defaults to auto =
max(warmupConcurrency, 200) instead of 0, matching the guidance documented
in docs/guides/client-pool.md. Explicit 0 disables sharing (reproduces the
connection-reset pathology); N sweeps pool sizes. Report shows the resolved
mode ('auto'/0/N).
Replace the auto-derived pool size with a plain fixed default (500 idle slots, 5min keep-alive) so the benchmark follows the documented guidance without any derivation logic. 0 still disables sharing to reproduce the connection-reset pathology; N sweeps sizes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3ecc7d36
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| repeat(cfg.steadyWorkers) { | ||
| threads.submit { | ||
| while (System.nanoTime() < deadline) { | ||
| pacer.waitForSlot() |
There was a problem hiding this comment.
When --acquire-rate-per-min is low relative to --steady-workers and --steady-duration-s, workers can enter the loop before the deadline, reserve slots far in the future in RatePacer, and then still perform pool.acquire() after the scenario duration has expired. For example, 16 workers at 1 acquire/minute can reserve slots out to ~15 minutes during a 60-second run; awaitTermination(duration+300s) can return while non-daemon worker tasks are still sleeping/acquiring, so the benchmark duration and metrics are inflated and the process may keep running long after the configured scenario window.
Useful? React with 👍 / 👎.
| # 4. run the driver (Kotlin SDK is built from source via composite build, | ||
| # see kotlin/settings.gradle.kts) | ||
| echo "== running benchmark driver ==" | ||
| DRIVER_ARGS+=("--report-dir" "${RUN_DIR}") |
There was a problem hiding this comment.
When users set the documented LIFECYCLE_ADDR to run the mock on a non-default port or host, the script health-checks that address but never forwards it to the Kotlin driver, so Cli.parse falls back to http://127.0.0.1:18080 and the run fails unless the user redundantly supplies --mock-base-url. Add the derived http://${LIFECYCLE_ADDR} to DRIVER_ARGS (unless already provided) alongside the report dir so the orchestration works with the advertised environment override.
Useful? React with 👍 / 👎.
… scenario Loaders race the pool fill instead of waiting for idle; fixes the CLI parser for valueless boolean flags (previously swallowed the next --key as its value).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f367f7b033
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| token := r.Header.Get("X-EXECD-ACCESS-TOKEN") | ||
| id := strings.TrimPrefix(token, "mock-token-") | ||
| if token != "" && token == execdToken(id) { |
There was a problem hiding this comment.
When an execd request is missing X-EXECD-ACCESS-TOKEN or sends a malformed token, this condition is skipped and the handler falls through to a 200 response. That means any SDK/client path that drops endpoint headers will appear ready immediately, bypassing the mock's Pending/expired/poisoned checks and invalidating readiness, stale-idle, and cross-SDK header-propagation benchmark results; reject missing or invalid tokens instead of treating them as unauthenticated success.
Useful? React with 👍 / 👎.
| } catch (t: Throwable) { | ||
| System.err.println("scenario $name failed: $t") | ||
| mapOf("error" to (t.message ?: t.toString())) |
There was a problem hiding this comment.
When a scenario throws after starting its pool, this catch records the error and then proceeds to collect stats and run the remaining scenarios. For example, an unguarded acquire in replenish-lag can throw before its pool.shutdown, leaving reconcile/warmup threads and live mock sandboxes active so later QPS/alive metrics are contaminated and the JVM may keep running; either abort here or ensure every scenario shuts its pool down in finally before returning an error section.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds a reproducible, cross-SDK benchmark harness for the sandbox client pool under
tests/benchmark/:specs/, with per-route response-time control (uniform/fixed/lognormal), fault injection (create failures, execd failures, partial sandbox poisoning), server-side TTL expiry, and exact per-second per-API QPS tracking with full time series.--acquire-rate-per-min), replenish-lag, failure-injection, stale-idle, idle-expiry, resize, shutdown-race, store-outage. Reports acquire latency percentiles, success rate + failure classification, pool health (idle trajectory, degraded/backoff, in-flight), replenish throughput, client threads/heap/GC, and per-scenario server QPS.run.shorchestrates mock build/start + driver run; every artifact of a run (reports, per-second timeseries CSVs, client probe CSVs, mock config, args, logs) lands in oneresults/run-<ts>/directory.includeBuild), so the harness always runs the checked-out SDK.SDK companion
PoolWarmupDiagnostics(diagnostic-only, zero-config, resettable) was added to the Kotlin SDK to instrument the warmup pipeline (queue-wait, create duration, commit, tick cadence, in-flight peak, failure reasons). It powered the diagnosis in #1514. It is additive and non-breaking; reviewers may prefer it dropped or formalized separately.Evidence produced
reconcileInterval).Verification
go build/vet on the mock server;./gradlew buildon the driver; full 7+3 scenario runs and a 30-minute steady-state run (2000/min, maxIdle=13815) exercised end-to-end.tests/benchmark/README.md.