Pool Controller v2 - #43
Conversation
Entire-Checkpoint: 01KZRD0B5347GGSYG1V60P1EZZ
![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
Orva/backend/internal/proxy/proxy.go
Line 330 in 4fb0c46
Measure streaming service time after consuming all frames
For a streaming handler, DispatchEx returns as soon as it reads response_start; the later NextFrame loop holds the worker for the actual stream lifetime. Recording latency here therefore measures TTFB rather than service time—a 30-second stream may be sampled as a few milliseconds—so the Little's-Law stable and burst signals under-predict the warm capacity required by streaming workloads. Record the sample after the frame loop terminates, immediately before release.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| s.spawnWG.Add(1) | ||
| go func() { | ||
| defer s.spawnWG.Done() | ||
| defer func() { <-p.spawnSlots }() |
There was a problem hiding this comment.
With a burst that needs more than four new workers, all request nudges can be consumed while spawnSlots is full; when these four spawns finish, this defer releases the slots without waking the controller again. The remaining queued requests therefore wait for the next 2-second tick before another batch of four can start, so a 100-request cold burst can require roughly 48 seconds of ramp-up and exceed the default 30-second request timeout. Nudge the scaler when a slot becomes available while demand remains.
Useful? React with 👍 / 👎.
| if t.cgroupConstrained { | ||
| physical -= total - budget | ||
| } | ||
| physical -= reserved |
There was a problem hiding this comment.
Once workers are running, both MemAvailable and cgroup memory.current already reflect their resident memory, while reserved continues to include every live worker until it is killed. Subtracting the full reservation again here makes the physical gate count live workers twice; for workers using their reservation, admission can stop near 40% of host RAM rather than the documented 80%, producing false memory_capacity limits and capacity timeouts. Only not-yet-visible spawn reservations should be deducted from physical availability; the logical budget can continue using all reservations.
Useful? React with 👍 / 👎.
| CASE WHEN scale_to_zero = 1 THEN 0 WHEN min_warm < 1 THEN 1 ELSE min_warm END, | ||
| max_warm, idle_ttl_s, max_use_count, scale_to_zero | ||
| FROM pool_config`, | ||
| `DROP TABLE pool_config`, |
There was a problem hiding this comment.
Upgrading any database that still has target_concurrency executes this DROP TABLE pool_config, despite the repository's explicit additive-only migration invariant. The controller can simply leave the now-unused column in place and normalize min_warm with an idempotent UPDATE; rebuilding the table also removes independently added indexes or triggers and makes downgrade compatibility unnecessarily unsafe.
AGENTS.md reference: backend/AGENTS.md:L87-L89
Useful? React with 👍 / 👎.
| if cfg.ScaleToZero && cfg.MinWarm != 0 { | ||
| respond.Error(w, http.StatusBadRequest, "VALIDATION", "scale_to_zero=true requires min_warm=0", reqID) | ||
| return |
There was a problem hiding this comment.
When an operator follows the shipped example orva pool set --fn greeter --min-warm 2 --max-warm 20 --scale-to-zero from cli/commands/pool.go:33 or docs/CLI.md:580, this new validation returns HTTP 400 because scale-to-zero now requires min_warm=0. Update both examples to use zero, or retain support for the previously documented combination.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
Capacity measurement
Measured throughput changed from 1,144.24 req/s to 1,063.80 req/s (-7.03%, within the 10% gate), cold starts fell from 0.402% to 0.200%, and measured worker churn fell from 69 spawned/37 killed to 32 spawned/0 killed.