Repository navigation
Backport over-target admission and the lease leak fix to v0.17 - #1427
Merged
Merged
Conversation
When every running sandbox was at its requests_per_instance target, AcquireLease parked the request and waited for a new sandbox. That went wrong in three ways that compounded on prod (MIR-2100): - Ingress keeps leased capacity cached for five minutes after use, so slots on busy-looking sandboxes rarely came back within the 120s wait, and ReleaseLease never woke anyone when they did. - Each waiter asked for one more sandbox, once, and racing +1s collapsed through the revision checks: 35 waiters produced +3. - A sandbox that finished booting served only the five leases its target allowed; the rest of the queue sat until it timed out. Now a request goes to the least-loaded routable sandbox even when all of them are at target, and that over-target admission kicks off a background scale-up. The scale-up is sized from demand: capacity leased on running sandboxes plus a slot per parked waiter, plus one lease of headroom, over the per-sandbox target. Waiters re-evaluate it whenever they wake, and at most one scale-up per pool is in flight. Requests only wait when nothing in the pool is routable, and once something is, every waiter takes it. An over-target lease is marked Overflow and takes one slot rather than a batch, and ingress uses it for the one request that took it and hands it straight back rather than caching it. Otherwise steady traffic would keep reusing it, pinning the load to the overloaded sandbox after new ones boot and keeping demand inflated. A parked waiter likewise counts as one slot, since a batch-sized slot per request would size the pool for many times its real load at high requests_per_instance. The scale-up also works for a pool the activator hasn't resolved through requestPoolCapacity yet, which is every pool a deploy creates after startup. The scale-up trusts the cached pool size, where the old +1 patched against it and re-read on a revision conflict. A missed scale-down would leave the cache claiming sandboxes that don't exist and block scaling for good. So a re-sync after a watch reconnect refreshes any cached pool the store has moved past, and since a reconnect can still miss an update, the scale-up also re-reads the pool (at most every 30s) when demand looks covered but fewer sandboxes are live than the cache claims. Both only take a newer revision. A waiter that gets a lease leaves the waiter list under the same lock, so demand doesn't count it as both a lease and a waiter. A saturated pool calls the scale-up on every request, under the lock every other app needs. So it returns before any demand math when a scale-up is already in flight, when the pool is at its cap with no re-read due, or for a short backoff after a failed attempt. requestPoolCapacity keeps its +1 behaviour as a floor, so cold starts and existing callers are unchanged. requests_per_instance becomes a scaling target rather than an admission cap, which is what the scaling docs already say it is.
Two bookkeeping bugs in the ingress lease cache, both of which keep activator capacity claimed longer than it should be. ReverseProxy aborts a response whose body copy fails, typically a client hanging up mid-download, by panicking with http.ErrAbortHandler. That skipped releaseLease, so the use was never handed back. A lease that never returns to zero uses never expires, and kept its slots on the sandbox for the sandbox's lifetime. proxyWithLease now releases on the way out of a panic and re-panics. useLease also accepted Uses <= Size, so a lease sized for two requests carried three.
The idle monitor computes the new DesiredInstances from a pool it listed earlier, then patched it with revision 0. A scale-up the activator made in between was silently undone, and the activator's cache went on believing its own number. Patch against the revision from the listing instead. On a conflict the pool is simply looked at again on the next 30s tick.
phinze
force-pushed
the
phinze/backport-1387-v0.17
branch
from
November 4, 2026 17:56
37be329 to
2abcdf4
Compare
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
I think this backport is faithful and ready for a human to look at. I'm not recommending it skip human review, though. It changes the activator's hot path on a release branch, and one deliberate omission has a concrete effect on deploys that someone should sign off on.
What holds up on this tree
- It compiles against release/0.17. Everything the backported code calls exists here:
wakeAllWaiters,poolState.revision,versionRef.strategy, themaxInstancescomputed at the top ofrequestPoolCapacityTo, and thecond/errorsimports inmanager.go.wg.Goneeds Go 1.25+, andgo.modis on 1.26.10. - Overflow leases stay balanced.
AcquireOverflowadds 1 tousedand returns size 1. Ingress hands that same size back, andAutoStrategy.releaseCapacitysubtracts it.RenewLeasereturns the same*Leaseit was given, so a long-running overflow request keeps itsOverflowflag acrossexpireLeasesticks. It doesn't turn into a reusable size-1 lease.- If
invalidateSandboxLeasesorinvalidateAppLeasesdrops an overflow lease mid-request, the laterreleaseLeasewon't find it inar.leases, so it doesn't release it a second time.
- The abort fix releases exactly once. In
proxyWithLease, the deferred recover releases and re-panics only on the panic path. Normal returns still go through the caller'sreleaseLease/invalidateLease. - The scale-down change is self-contained. It patches against the revision from the listing and treats a conflict as "try next tick".
TestScaleDownDoesNotOverwriteConcurrentScaleUppins this.
For a human to decide
- Deploys without 1783c69. On this branch a new auto-mode pool starts at one instance (
controllers/deployment/launcher.go:480,desiredInstances := int64(1)). With over-target admission, every request for the new version that misses the ingress cache goes to that single sandbox instead of parking.checkForSandboxalso deliberately hands every queued waiter to the first routable sandbox. So right after a deploy of a busy service, one fresh sandbox takes all of the new version's traffic until the demand-sized scale-up catches up. That's still better than the 120s hangs this fixes, and 1783c69 seems to exist for exactly this moment. Someone should confirm that post-deploy burst is acceptable for miren.cloud's web service on v0.17.3, or decide to bring 1783c69 along. - The etcd-backed tests haven't run on this branch. You said you're relying on CI for them, so please check that run before merging.
AcquireLeasenow holds the write lock for its whole scan, and waiters register before the capacity request. Existing activator tests may have quietly depended on the old timing.
🍪 full review note · reviewed at 2abcdf4
evanphx
approved these changes
Nov 4, 2026
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
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.
Backports #1387 to release/0.17, minus its last commit.
This is the fix for MIR-2194: miren.cloud hung on every request until we restarted the app, with all 20 web sandboxes healthy and requests failing with "no sandbox became available within 120 seconds". eu1 runs v0.17.2, which doesn't have #1387, so the pool climbed to the 20-instance maximum and requests had nowhere left to go. This is the same failure MIR-2100 described.
Three of #1387's four commits come over. The first two are straight cherry-picks with identical diffs. The third has one test-only conflict:
TestScaleDownDoesNotOverwriteConcurrentScaleUplands after tests from ef2115a on main, which aren't on this line, so I kept the new test and left those out. Itsmanager.gochange applied cleanly.I left out 1783c69 (start a deploy's new pool at the current size). It changes deploy behavior and isn't needed to stop this failure, and #1387 notes it can be backed out by itself.
Tested on this tree with
go testonpkg/concurrency,servers/httpingress,components/activator, andcontrollers/sandboxpool. Four tests fail, and the same four fail on plain v0.17.2: three need etcd from the dev environment, andTestManagerLongStartupFailureBackofffails on a UTC vs local time comparison. I haven't run the etcd-backed tests locally, so those rely on CI.The changelog entry links #1387. The v0.17.3 stamp will come separately, once we know what else the release carries.