From b38b0521fb85ad18b34f283ff7f846000871fd1f Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 16 Oct 2026 16:53:13 -0500 Subject: [PATCH 1/4] Admit over target instead of parking requests in the activator 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. --- components/activator/activator.go | 538 ++++++++++++++------ components/activator/activator_test.go | 8 +- components/activator/soft_admission_test.go | 325 ++++++++++++ pkg/concurrency/strategy.go | 7 + servers/httpingress/httpingress.go | 24 + servers/httpingress/lease_test.go | 37 ++ 6 files changed, 795 insertions(+), 144 deletions(-) create mode 100644 components/activator/soft_admission_test.go diff --git a/components/activator/activator.go b/components/activator/activator.go index f44170fc7..9ea376ed8 100644 --- a/components/activator/activator.go +++ b/components/activator/activator.go @@ -96,6 +96,10 @@ type Lease struct { Size int URL string + + // Overflow marks a lease admitted over target. It should carry only the + // request that acquired it, so over-target load doesn't outlive it. + Overflow bool } func (l *Lease) Version() *core_v1alpha.AppVersion { @@ -189,6 +193,15 @@ type localActivator struct { // Map key is verKey (version + service), value is list of channels to notify newSandboxChans map[verKey][]chan struct{} + // scalingUp marks pools with a background scaleUpForDemand in flight + scalingUp map[entity.Id]bool + + // poolRecheckedAt throttles scaleUpForDemand's store re-reads per pool + poolRecheckedAt map[entity.Id]time.Time + + // scaleRetryAt holds off scaleUpForDemand for a pool after a failed attempt + scaleRetryAt map[entity.Id]time.Time + // invalidationCh signals httpingress when a sandbox becomes non-RUNNING invalidationCh chan SandboxInvalidation @@ -240,67 +253,43 @@ func (a *localActivator) SetPoolCreator(pc PoolCreator) { func (a *localActivator) AcquireLease(ctx context.Context, ver *core_v1alpha.AppVersion, service string) (*Lease, error) { key := verKey{ver.ID.String(), service} - // Try to find an available sandbox with capacity (read lock for scanning) - a.mu.RLock() + a.mu.Lock() versionRef, ok := a.versions[key] - var candidateSandbox *sandbox hasPending := false if ok { - // Look up the pool's sandboxes - ps, poolOk := a.poolSandboxes[versionRef.poolID] - if poolOk && len(ps.sandboxes) > 0 { - a.log.Debug("checking existing sandboxes in pool", "app", ver.App, "version", ver.Version, "pool", versionRef.poolID, "sandboxes", len(ps.sandboxes)) - - // Scan for a sandbox with capacity - start := rand.Int() % len(ps.sandboxes) - for i := 0; i < len(ps.sandboxes); i++ { - s := ps.sandboxes[(start+i)%len(ps.sandboxes)] - if ver.EphemeralLabel != "" && len(s.sandbox.Spec.Volume) > 0 { - a.mu.RUnlock() - return nil, ephemeralx.ErrDisksUnsupported + if ps, poolOk := a.poolSandboxes[versionRef.poolID]; poolOk { + if ver.EphemeralLabel != "" { + for _, s := range ps.sandboxes { + if len(s.sandbox.Spec.Volume) > 0 { + a.mu.Unlock() + return nil, ephemeralx.ErrDisksUnsupported + } } - if s.sandbox.Status == compute_v1alpha.RUNNING && s.tracker.HasCapacity() && s.url != "" { - candidateSandbox = s - break + } + + if lease, overTarget := a.acquireFromPoolLocked(ps, ver, service); lease != nil { + a.mu.Unlock() + if overTarget { + // Every running sandbox is at its requests_per_instance target. + // Serve the request anyway and grow the pool in the background: + // the target is a scaling signal, not an admission cap. + a.log.Debug("all sandboxes at target, admitting over target", "app", ver.App, "version", ver.Version, "sandbox", lease.sandbox.ID) + a.scaleUpForDemand(ver, service) } - // Track if we have PENDING sandboxes (being created/booting) + return lease, nil + } + + for _, s := range ps.sandboxes { if s.sandbox.Status == compute_v1alpha.PENDING { hasPending = true + break } } } } - a.mu.RUnlock() - - // If we found a candidate, acquire write lock and double-check status and capacity - if candidateSandbox != nil { - a.mu.Lock() - // Double-check status and capacity (may have changed between locks) - if candidateSandbox.sandbox.Status == compute_v1alpha.RUNNING && - candidateSandbox.tracker.HasCapacity() && - candidateSandbox.url != "" { - leaseSize := candidateSandbox.tracker.AcquireLease() - candidateSandbox.lastRenewal = time.Now() - - lease := &Lease{ - ver: ver, - sandbox: candidateSandbox.sandbox, - ent: candidateSandbox.ent, - pool: service, // Pool identifier is now the service name - service: service, - Size: leaseSize, - URL: candidateSandbox.url, - } - used := candidateSandbox.tracker.Used() - a.mu.Unlock() - a.log.Debug("reusing sandbox", "app", ver.App, "version", ver.Version, "sandbox", candidateSandbox.sandbox.ID, "used", used) - return lease, nil - } - a.mu.Unlock() - // Status changed or capacity was taken between RLock and Lock, fall through to pool request - } + a.mu.Unlock() - // No available sandboxes with capacity + // Nothing is routable yet if hasPending { // We have PENDING sandboxes booting - wait for them instead of creating more a.log.Info("no running sandboxes available, but pending sandboxes exist - waiting", @@ -310,7 +299,7 @@ func (a *localActivator) AcquireLease(ctx context.Context, ver *core_v1alpha.App return a.waitForSandbox(ctx, ver, service, false) } - // No RUNNING or PENDING sandboxes - need to scale up via pool. + // No routable or PENDING sandboxes - need to scale up via pool. // Ephemeral preview deploys are capped at one instance by EphemeralStrategy // (MaxInstances=1), so requestPoolCapacity won't scale them past their single // sandbox — it returns the at-cap pool and the caller waits for it. @@ -356,11 +345,30 @@ func (a *localActivator) waitForSandbox(ctx context.Context, ver *core_v1alpha.A a.mu.RUnlock() } - var pool *compute_v1alpha.SandboxPool + // Register before requesting capacity so this waiter counts toward the + // demand that sizes the request. + notifyChan := make(chan struct{}, 1) + a.mu.Lock() + if a.newSandboxChans == nil { + a.newSandboxChans = make(map[verKey][]chan struct{}) + } + a.newSandboxChans[key] = append(a.newSandboxChans[key], notifyChan) + a.mu.Unlock() + + // Clean up the channel on exit + defer func() { + a.mu.Lock() + a.removeWaiterLocked(key, notifyChan) + a.mu.Unlock() + close(notifyChan) + }() + if incrementPool { - // Request additional capacity from pool - var err error - pool, err = a.requestPoolCapacity(ctx, ver, service) + a.mu.RLock() + target := a.demandTargetLocked(key) + a.mu.RUnlock() + + pool, err := a.requestPoolCapacityTo(ctx, ver, service, target) if err != nil { return nil, fmt.Errorf("failed to request pool capacity: %w", err) } @@ -376,29 +384,6 @@ func (a *localActivator) waitForSandbox(ctx context.Context, ver *core_v1alpha.A "service", service) } - // Register notification channel for this wait - notifyChan := make(chan struct{}, 1) - a.mu.Lock() - a.newSandboxChans[key] = append(a.newSandboxChans[key], notifyChan) - a.mu.Unlock() - - // Clean up the channel on exit - defer func() { - a.mu.Lock() - chans := a.newSandboxChans[key] - for i, ch := range chans { - if ch == notifyChan { - a.newSandboxChans[key] = append(chans[:i], chans[i+1:]...) - break - } - } - if len(a.newSandboxChans[key]) == 0 { - delete(a.newSandboxChans, key) - } - a.mu.Unlock() - close(notifyChan) - }() - // Cap the per-request wait at 120s. The default PortWaitTimeout is 15s and // a first-time image pull on a node that hasn't seen the image can run // 30-90s, plus container setup. The previous 50s cap was too aggressive @@ -413,60 +398,43 @@ func (a *localActivator) waitForSandbox(ctx context.Context, ver *core_v1alpha.A defer ticker.Stop() // Helper to check for available sandbox + // Once anything in the pool is routable, every waiter takes it: a + // sandbox that just booted serves the whole queue rather than the few + // leases its requests_per_instance target would allow, and the pool keeps + // growing in the background from the demand those waiters represent. checkForSandbox := func() (*Lease, bool) { - a.mu.RLock() + a.mu.Lock() versionRef, ok := a.versions[key] - var candidateSandbox *sandbox - if ok { - // Look up the pool's sandboxes - ps, poolOk := a.poolSandboxes[versionRef.poolID] - if poolOk && len(ps.sandboxes) > 0 { - // Try to find a sandbox with capacity. - // Require url != "" — a sandbox can briefly be tracked as RUNNING - // before the watcher has populated its URL, and returning a Lease - // with an empty URL would cause the proxy to fail downstream. - // Mirrors the predicate in AcquireLease. - start := rand.Int() % len(ps.sandboxes) - for i := 0; i < len(ps.sandboxes); i++ { - s := ps.sandboxes[(start+i)%len(ps.sandboxes)] - if s.sandbox.Status == compute_v1alpha.RUNNING && s.tracker.HasCapacity() && s.url != "" { - candidateSandbox = s - break - } - } - } + if !ok { + a.mu.Unlock() + return nil, false } - a.mu.RUnlock() - - if candidateSandbox != nil { - a.mu.Lock() - // Double-check status, capacity, and URL (may have changed between locks) - if candidateSandbox.sandbox.Status == compute_v1alpha.RUNNING && - candidateSandbox.tracker.HasCapacity() && - candidateSandbox.url != "" { - leaseSize := candidateSandbox.tracker.AcquireLease() - candidateSandbox.lastRenewal = time.Now() - a.mu.Unlock() - - a.log.Info("acquired lease from pool sandbox", - "app", ver.App, - "version", ver.Version, - "sandbox", candidateSandbox.sandbox.ID, - "service", service) - - return &Lease{ - ver: ver, - sandbox: candidateSandbox.sandbox, - ent: candidateSandbox.ent, - pool: service, - service: service, - Size: leaseSize, - URL: candidateSandbox.url, - }, true - } + ps, poolOk := a.poolSandboxes[versionRef.poolID] + if !poolOk { a.mu.Unlock() + return nil, false } - return nil, false + lease, overTarget := a.acquireFromPoolLocked(ps, ver, service) + if lease != nil { + // Its lease now counts toward demand, so it must stop counting + // as a waiter under the same lock. + a.removeWaiterLocked(key, notifyChan) + } + a.mu.Unlock() + if lease == nil { + return nil, false + } + + a.log.Info("acquired lease from pool sandbox", + "app", ver.App, + "version", ver.Version, + "sandbox", lease.sandbox.ID, + "service", service, + "over_target", overTarget) + if overTarget { + a.scaleUpForDemand(ver, service) + } + return lease, true } for { @@ -475,6 +443,9 @@ func (a *localActivator) waitForSandbox(ctx context.Context, ver *core_v1alpha.A return lease, nil } + // The queue may have grown since capacity was last requested. + a.scaleUpForDemand(ver, service) + // Check if all sandboxes have failed (no RUNNING, no PENDING) // If so, fail fast instead of waiting for timeout. // @@ -559,6 +530,278 @@ func (a *localActivator) waitForSandbox(ctx context.Context, ver *core_v1alpha.A } } +// removeWaiterLocked drops a parked waiter's channel. It is a no-op if the +// channel is already gone. The caller must hold a.mu for writing. +func (a *localActivator) removeWaiterLocked(key verKey, notifyChan chan struct{}) { + chans := a.newSandboxChans[key] + for i, ch := range chans { + if ch == notifyChan { + a.newSandboxChans[key] = append(chans[:i], chans[i+1:]...) + break + } + } + if len(a.newSandboxChans[key]) == 0 { + delete(a.newSandboxChans, key) + } +} + +// acquireFromPoolLocked leases a routable sandbox from the pool, preferring one +// under its requests_per_instance target and otherwise falling back to the +// least-loaded routable sandbox. overTarget reports the fallback, which callers +// treat as a signal to grow the pool. It returns nil when nothing in the pool +// is routable. The caller must hold a.mu for writing. +// +// Routable requires url != "": a sandbox can briefly be tracked as RUNNING +// before the watcher has populated its URL, and a Lease with an empty URL +// would fail in the proxy. +func (a *localActivator) acquireFromPoolLocked(ps *poolSandboxes, ver *core_v1alpha.AppVersion, service string) (lease *Lease, overTarget bool) { + if len(ps.sandboxes) == 0 { + return nil, false + } + + var chosen, leastLoaded *sandbox + start := rand.Int() % len(ps.sandboxes) + for i := range len(ps.sandboxes) { + s := ps.sandboxes[(start+i)%len(ps.sandboxes)] + if s.sandbox.Status != compute_v1alpha.RUNNING || s.url == "" { + continue + } + if s.tracker.HasCapacity() { + chosen = s + break + } + if leastLoaded == nil || s.tracker.Used() < leastLoaded.tracker.Used() { + leastLoaded = s + } + } + if chosen == nil { + if leastLoaded == nil { + return nil, false + } + chosen, overTarget = leastLoaded, true + } + + var leaseSize int + if overTarget { + leaseSize = chosen.tracker.AcquireOverflow() + } else { + leaseSize = chosen.tracker.AcquireLease() + } + chosen.lastRenewal = time.Now() + + return &Lease{ + ver: ver, + sandbox: chosen.sandbox, + ent: chosen.ent, + pool: service, + service: service, + Size: leaseSize, + URL: chosen.url, + Overflow: overTarget, + }, overTarget +} + +// demandTargetLocked sizes the pool for its current demand: the capacity +// leased out on running sandboxes plus a slot for every parked waiter (each is +// one request), and one lease of headroom so a pool that is exactly full still +// asks for one more sandbox. It returns 0 when the version isn't mapped to a pool yet. The +// caller must hold a.mu. +func (a *localActivator) demandTargetLocked(key verKey) int64 { + versionRef, ok := a.versions[key] + if !ok || versionRef.strategy == nil { + return 0 + } + ps, ok := a.poolSandboxes[versionRef.poolID] + if !ok { + return 0 + } + + perSandbox := versionRef.strategy.InitializeTracker().Max() + leaseSize := versionRef.strategy.LeaseSize() + if perSandbox < 1 || leaseSize < 1 { + return 0 + } + + demand := leaseSize + for _, s := range ps.sandboxes { + if s.sandbox.Status == compute_v1alpha.RUNNING { + demand += s.tracker.Used() + } + } + for k, chans := range a.newSandboxChans { + if ref, ok := a.versions[k]; ok && ref.poolID == versionRef.poolID { + demand += len(chans) + } + } + + target := int64((demand + perSandbox - 1) / perSandbox) + return min(target, int64(versionRef.strategy.MaxInstances())) +} + +// maxDemandScaleAttempts bounds one scaleUpForDemand worker, in case the +// cached pool never catches up with its own patches. +const maxDemandScaleAttempts = 10 + +// poolRecheckInterval throttles the store re-read scaleUpForDemand does when +// the cached pool size is larger than what is actually running. +const poolRecheckInterval = 30 * time.Second + +// scaleRetryBackoff holds off scaleUpForDemand after a failed attempt, so a +// pool that can't grow (crash cooldown, say) doesn't retry on every request. +const scaleRetryBackoff = 10 * time.Second + +// scaleUpForDemand grows the pool toward demandTargetLocked without blocking +// the caller. It is cheap when the pool is already big enough, so the hot path +// can call it on every over-target admission, and at most one scale-up per pool +// runs at a time. +// +// It trusts the cached DesiredInstances, which a missed pool update can leave +// too high. When demand looks covered but fewer sandboxes are live than the +// cache claims, it re-reads the pool from the store before deciding. +func (a *localActivator) scaleUpForDemand(ver *core_v1alpha.AppVersion, service string) { + key := verKey{ver.ID.String(), service} + + // check reports the demand target, whether it exceeds the cached size, + // and whether the cached size should be re-read first. + check := func() (poolID entity.Id, target int64, scale, recheck bool) { + versionRef, ok := a.versions[key] + if !ok { + return "", 0, false, false + } + ps := a.poolSandboxes[versionRef.poolID] + // A pool created after startup isn't in a.pools until + // requestPoolCapacityTo resolves it. + var desired int64 + if state, ok := a.pools[key]; ok { + if state.inProgress || state.pool == nil { + return "", 0, false, false + } + desired = state.pool.DesiredInstances + } else if ps != nil && ps.pool != nil { + desired = ps.pool.DesiredInstances + } else { + return "", 0, false, false + } + var live int64 + if ps != nil { + for _, s := range ps.sandboxes { + if s.sandbox.Status == compute_v1alpha.RUNNING || s.sandbox.Status == compute_v1alpha.PENDING { + live++ + } + } + } + recheck = live < desired && time.Since(a.poolRecheckedAt[versionRef.poolID]) >= poolRecheckInterval + // A saturated pool at its cap calls this on every request, so skip + // the demand math when there is nothing it could change. + if !recheck && versionRef.strategy != nil && desired >= int64(versionRef.strategy.MaxInstances()) { + return versionRef.poolID, 0, false, false + } + target = a.demandTargetLocked(key) + if target > desired { + return versionRef.poolID, target, true, false + } + return versionRef.poolID, target, false, recheck + } + + a.mu.Lock() + if ref, ok := a.versions[key]; ok && (a.scalingUp[ref.poolID] || time.Now().Before(a.scaleRetryAt[ref.poolID])) { + a.mu.Unlock() + return + } + poolID, _, scale, recheck := check() + if !scale && !recheck { + a.mu.Unlock() + return + } + if a.scalingUp == nil { + a.scalingUp = make(map[entity.Id]bool) + } + a.scalingUp[poolID] = true + a.mu.Unlock() + + go func() { + // The demand check and clearing scalingUp share a lock, so demand that + // arrives while a request is in flight is picked up by this loop + // rather than dropped by callers that saw scalingUp set. + for attempt := 0; ; attempt++ { + a.mu.Lock() + _, target, scale, recheck := check() + if recheck && attempt < maxDemandScaleAttempts { + if a.poolRecheckedAt == nil { + a.poolRecheckedAt = make(map[entity.Id]time.Time) + } + a.poolRecheckedAt[poolID] = time.Now() + a.mu.Unlock() + a.refreshPoolFromStore(poolID) + continue + } + if !scale || attempt >= maxDemandScaleAttempts { + delete(a.scalingUp, poolID) + a.mu.Unlock() + if scale { + a.log.Warn("pool still below demand after scaling", "app", ver.App, "service", service, "pool", poolID, "target", target) + } + return + } + a.mu.Unlock() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + _, err := a.requestPoolCapacityTo(ctx, ver, service, target) + cancel() + if err != nil { + a.mu.Lock() + delete(a.scalingUp, poolID) + if a.scaleRetryAt == nil { + a.scaleRetryAt = make(map[entity.Id]time.Time) + } + a.scaleRetryAt[poolID] = time.Now().Add(scaleRetryBackoff) + a.mu.Unlock() + a.log.Warn("failed to scale pool for demand", "app", ver.App, "service", service, "pool", poolID, "target", target, "error", err) + return + } + } + }() +} + +// refreshPoolFromStore re-reads a pool and updates the cached copies the store +// has moved past. Revisions guard the update, so a read that races a newer +// patch or watch event never moves the cache backward. +func (a *localActivator) refreshPoolFromStore(poolID entity.Id) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + res, err := a.eac.Get(ctx, poolID.String()) + if err != nil { + a.log.Warn("failed to re-read pool", "pool", poolID, "error", err) + return + } + var pool compute_v1alpha.SandboxPool + pool.Decode(res.Entity().Entity()) + rev := res.Entity().Revision() + + a.mu.Lock() + defer a.mu.Unlock() + + newest := true + for _, state := range a.pools { + if state.inProgress || state.pool == nil || state.pool.ID != poolID { + continue + } + if state.revision < rev { + if state.pool.DesiredInstances != pool.DesiredInstances { + a.log.Info("re-read pool corrected cached DesiredInstances", "pool", poolID, "cached", state.pool.DesiredInstances, "stored", pool.DesiredInstances) + } + state.pool = &pool + state.revision = rev + } else { + newest = false + } + } + if ps, ok := a.poolSandboxes[poolID]; ok && newest { + ps.pool = &pool + } +} + // registerVersionPoolLocked records the version->pool mapping and ensures an // (initially empty) poolSandboxes entry exists for the pool. The caller must // hold a.mu for writing. @@ -604,6 +847,15 @@ func (a *localActivator) registerVersionPoolLocked( // a request before DeploymentLauncher has finished creating the pool. // Uses a sentinel pattern to prevent duplicate capacity requests from concurrent callers. func (a *localActivator) requestPoolCapacity(ctx context.Context, ver *core_v1alpha.AppVersion, service string) (*compute_v1alpha.SandboxPool, error) { + return a.requestPoolCapacityTo(ctx, ver, service, 0) +} + +// requestPoolCapacityTo is requestPoolCapacity with a demand floor: it raises +// DesiredInstances by at least one, and up to minDesired when that is higher, +// capped at the strategy's maximum. Sizing the request from demand is what lets +// a burst of waiters ask for several sandboxes at once instead of collapsing +// into a single +1 through the revision checks below. +func (a *localActivator) requestPoolCapacityTo(ctx context.Context, ver *core_v1alpha.AppVersion, service string, minDesired int64) (*compute_v1alpha.SandboxPool, error) { key := verKey{ver.ID.String(), service} // Resolve config and build the strategy once at entry: the cap comes from @@ -680,7 +932,7 @@ poolLoop: // This ensures concurrent goroutines that all saw the same initial value // will calculate the same target a.mu.Lock() - newDesired := state.pool.DesiredInstances + 1 + newDesired := min(max(state.pool.DesiredInstances+1, minDesired), maxInstances) a.mu.Unlock() const maxRetries = 3 @@ -736,7 +988,7 @@ poolLoop: state.pool = &freshPool state.revision = freshPoolEnt.Entity().Revision() a.pools[key] = state - newDesired = freshPool.DesiredInstances + 1 + newDesired = min(max(freshPool.DesiredInstances+1, minDesired), maxInstances) a.mu.Unlock() continue @@ -953,7 +1205,7 @@ poolLoop: return foundPool, nil } - newDesired := foundPool.DesiredInstances + 1 + newDesired := min(max(foundPool.DesiredInstances+1, minDesired), maxInstances) a.mu.Unlock() @@ -1755,19 +2007,12 @@ func (a *localActivator) recoverPools(ctx context.Context) error { a.mu.Lock() // Cache pool state (for sentinel pattern in requestPoolCapacity). - // Create-only: at startup the map is empty so this seeds every pool, but - // when recoverPools runs again as a live re-sync (watch reconnect) we must - // not clobber an in-progress creation sentinel or a freshly-patched - // revision held by a concurrent requestPoolCapacity. watchPools keeps - // existing entries' DesiredInstances fresh; here we only fill gaps. - // - // Deliberately we do NOT refresh an already-cached entry here, even though - // the missed-event window is exactly when its DesiredInstances/revision - // could have gone stale. A refresh is unsafe: this re-sync's store read can - // itself be older than an in-memory revision a concurrent requestPoolCapacity - // just patched, so overwriting could move the cache backward. The stale case - // self-heals instead — the next increment hits a revision conflict on Patch, - // clears the entry, and re-reads (see the ErrConflict path above). + // At startup the map is empty so this seeds every pool. On a live + // re-sync (watch reconnect) it also refreshes entries that went stale + // while the watch was down, which matters because scaleUpForDemand + // trusts the cached DesiredInstances. Only a newer revision replaces an + // entry, so this read can't move the cache behind a patch a concurrent + // requestPoolCapacity just made, and creation sentinels are left alone. key := verKey{versionID.String(), pool.Service} if _, ok := a.pools[key]; !ok { a.pools[key] = &poolState{ @@ -1776,6 +2021,12 @@ func (a *localActivator) recoverPools(ctx context.Context) error { inProgress: false, } } + for _, state := range a.pools { + if !state.inProgress && state.pool != nil && state.pool.ID == pool.ID && state.revision < ent.Revision() { + state.pool = &pool + state.revision = ent.Revision() + } + } // Initialize empty poolSandboxes entry (sandboxes will be added by recoverSandboxes) poolID := pool.ID @@ -2015,6 +2266,9 @@ func (a *localActivator) removePoolFromTracking(poolID entity.Id) { a.mu.Lock() defer a.mu.Unlock() + delete(a.poolRecheckedAt, poolID) + delete(a.scaleRetryAt, poolID) + // Remove poolSandboxes entry if _, ok := a.poolSandboxes[poolID]; ok { delete(a.poolSandboxes, poolID) diff --git a/components/activator/activator_test.go b/components/activator/activator_test.go index 7b1c4e267..6fbc6c110 100644 --- a/components/activator/activator_test.go +++ b/components/activator/activator_test.go @@ -649,6 +649,9 @@ func TestActivatorKeepsDeadSandbox(t *testing.T) { func TestActivatorFailsFastWhenAllSandboxesDead(t *testing.T) { log := slog.New(slog.NewTextHandler(io.Discard, nil)) + server, cleanup := testutils.NewInMemEntityServer(t) + defer cleanup() + testVer := &core_v1alpha.AppVersion{ ID: entity.Id("ver-1"), App: entity.Id("app-1"), @@ -696,6 +699,7 @@ func TestActivatorFailsFastWhenAllSandboxesDead(t *testing.T) { poolID := entity.Id("pool-1") activator := &localActivator{ log: log, + eac: server.EAC, versions: map[verKey]*versionPoolRef{ {"ver-1", "web"}: { ver: testVer, @@ -706,7 +710,7 @@ func TestActivatorFailsFastWhenAllSandboxesDead(t *testing.T) { }, poolSandboxes: map[entity.Id]*poolSandboxes{ poolID: { - pool: &compute_v1alpha.SandboxPool{ID: poolID}, + pool: &compute_v1alpha.SandboxPool{ID: poolID, DesiredInstances: 1}, sandboxes: []*sandbox{deadSandbox}, service: "web", strategy: strategy, @@ -834,7 +838,7 @@ func TestActivatorPendingSandboxAwareness(t *testing.T) { strategy: strategy, } activator.poolSandboxes[poolID] = &poolSandboxes{ - pool: &compute_v1alpha.SandboxPool{ID: poolID}, + pool: &compute_v1alpha.SandboxPool{ID: poolID, DesiredInstances: 1}, sandboxes: []*sandbox{pendingSb}, service: "web", strategy: strategy, diff --git a/components/activator/soft_admission_test.go b/components/activator/soft_admission_test.go new file mode 100644 index 000000000..aa1e3e24f --- /dev/null +++ b/components/activator/soft_admission_test.go @@ -0,0 +1,325 @@ +package activator + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "miren.dev/runtime/api/compute/compute_v1alpha" + "miren.dev/runtime/api/core/core_v1alpha" + "miren.dev/runtime/pkg/concurrency" + "miren.dev/runtime/pkg/entity" + "miren.dev/runtime/pkg/entity/testutils" +) + +// softAdmissionFixture is an auto-mode (requests_per_instance=10) version with +// a real pool entity, so background scale-ups land in the store where the test +// can read them back. +type softAdmissionFixture struct { + server *testutils.InMemEntityServer + activator *localActivator + ver *core_v1alpha.AppVersion + poolID entity.Id + key verKey + strategy concurrency.ConcurrencyStrategy +} + +func newSoftAdmissionFixture(t *testing.T, desired int64) *softAdmissionFixture { + t.Helper() + ctx := context.Background() + + server, cleanup := testutils.NewInMemEntityServer(t) + t.Cleanup(cleanup) + + appID, err := server.Client.Create(ctx, "test-app", &core_v1alpha.App{}) + require.NoError(t, err) + + ver := &core_v1alpha.AppVersion{ + App: appID, + Version: "v1", + ImageUrl: "test:latest", + Config: core_v1alpha.Config{ + Services: []core_v1alpha.Services{{ + Name: "web", + ServiceConcurrency: core_v1alpha.ServiceConcurrency{ + Mode: "auto", + RequestsPerInstance: 10, + }, + }}, + }, + } + verID, err := server.Client.Create(ctx, "test-ver", ver) + require.NoError(t, err) + ver.ID = verID + + pool := &compute_v1alpha.SandboxPool{ + Service: "web", + SandboxSpec: compute_v1alpha.SandboxSpec{Version: ver.ID}, + ReferencedByVersions: []entity.Id{ver.ID}, + DesiredInstances: desired, + } + poolID, err := server.Client.Create(ctx, "test-pool", pool) + require.NoError(t, err) + pool.ID = poolID + + res, err := server.EAC.Get(ctx, poolID.String()) + require.NoError(t, err) + + strategy := concurrency.NewStrategy(&ver.Config.Services[0].ServiceConcurrency) + key := verKey{ver.ID.String(), "web"} + + a := &localActivator{ + log: testutils.TestLogger(t), + eac: server.EAC, + versions: map[verKey]*versionPoolRef{key: {ver: ver, poolID: poolID, service: "web", strategy: strategy}}, + poolSandboxes: map[entity.Id]*poolSandboxes{poolID: {pool: pool, service: "web", strategy: strategy}}, + pools: map[verKey]*poolState{key: {pool: pool, revision: res.Entity().Revision()}}, + newSandboxChans: make(map[verKey][]chan struct{}), + } + + return &softAdmissionFixture{server: server, activator: a, ver: ver, poolID: poolID, key: key, strategy: strategy} +} + +// addSandbox tracks a sandbox in the pool with `leases` leases already handed out. +func (f *softAdmissionFixture) addSandbox(t *testing.T, name string, status compute_v1alpha.SandboxStatus, url string, leases int) *sandbox { + t.Helper() + tracker := f.strategy.InitializeTracker() + for range leases { + tracker.AcquireLease() + } + sb := &sandbox{ + sandbox: &compute_v1alpha.Sandbox{ + ID: entity.Id("sandbox/" + name), + Status: status, + Spec: compute_v1alpha.SandboxSpec{Version: f.ver.ID}, + }, + ent: entity.Blank(), + url: url, + tracker: tracker, + } + f.activator.mu.Lock() + ps := f.activator.poolSandboxes[f.poolID] + ps.sandboxes = append(ps.sandboxes, sb) + f.activator.mu.Unlock() + return sb +} + +func (f *softAdmissionFixture) storedDesired(t *testing.T) int64 { + t.Helper() + res, err := f.server.EAC.Get(context.Background(), f.poolID.String()) + require.NoError(t, err) + var pool compute_v1alpha.SandboxPool + pool.Decode(res.Entity().Entity()) + return pool.DesiredInstances +} + +// When every running sandbox is at its requests_per_instance target, the +// request is admitted to the least-loaded one right away instead of waiting +// for a new sandbox, and the pool grows in the background. +func TestAcquireLeaseAdmitsOverTargetAndScales(t *testing.T) { + f := newSoftAdmissionFixture(t, 2) + f.addSandbox(t, "busier", compute_v1alpha.RUNNING, "http://busier", 6) + f.addSandbox(t, "lighter", compute_v1alpha.RUNNING, "http://lighter", 5) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + lease, err := f.activator.AcquireLease(ctx, f.ver, "web") + require.NoError(t, err) + require.NotNil(t, lease) + assert.Equal(t, "http://lighter", lease.URL, "over-target admission should pick the least-loaded sandbox") + assert.True(t, lease.Overflow, "a lease admitted over target is marked as overflow") + assert.Equal(t, 1, lease.Size, "an overflow lease takes one slot, not a batch") + + // 22 slots leased + 2 headroom over 10 per sandbox → 3 sandboxes. + require.Eventually(t, func() bool { return f.storedDesired(t) == 3 }, 5*time.Second, 20*time.Millisecond) +} + +// A burst on a saturated pool asks for enough sandboxes to cover it, rather +// than the single +1 that racing increments used to collapse into. +func TestOverTargetBurstScalesWithDemand(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + f.addSandbox(t, "only", compute_v1alpha.RUNNING, "http://only", 5) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var wg sync.WaitGroup + for range 20 { + wg.Go(func() { + lease, err := f.activator.AcquireLease(ctx, f.ver, "web") + assert.NoError(t, err) + assert.NotNil(t, lease) + }) + } + wg.Wait() + + // 10 + 20 single-slot overflow leases = 30, + 2 headroom → ceil(32/10) = 4. + require.Eventually(t, func() bool { return f.storedDesired(t) == 4 }, 5*time.Second, 20*time.Millisecond) +} + +// Waiters parked on a cold pool are all served once a sandbox becomes +// routable, not just the five its target would allow. +func TestParkedWaitersAllServedWhenSandboxBecomesRoutable(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + sb := f.addSandbox(t, "booting", compute_v1alpha.PENDING, "", 0) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + const waiters = 12 + errs := make(chan error, waiters) + for range waiters { + go func() { + lease, err := f.activator.waitForSandbox(ctx, f.ver, "web", false) + if err == nil && lease == nil { + err = fmt.Errorf("nil lease") + } + errs <- err + }() + } + + require.Eventually(t, func() bool { + f.activator.mu.RLock() + defer f.activator.mu.RUnlock() + return len(f.activator.newSandboxChans[f.key]) == waiters + }, 2*time.Second, 10*time.Millisecond) + + f.activator.mu.Lock() + sb.sandbox.Status = compute_v1alpha.RUNNING + sb.url = "http://booted" + f.activator.mu.Unlock() + f.activator.wakeAllWaiters() + + for range waiters { + select { + case err := <-errs: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("parked waiter was not served after the sandbox became routable") + } + } + + // 12 waiters sized the pool while parked: (12 + 2) / 10 → 2. + require.Eventually(t, func() bool { return f.storedDesired(t) >= 2 }, 5*time.Second, 20*time.Millisecond) +} + +func TestDemandTarget(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + a := f.activator + + target := func() int64 { + a.mu.RLock() + defer a.mu.RUnlock() + return a.demandTargetLocked(f.key) + } + + assert.Equal(t, int64(1), target(), "an empty pool still wants one sandbox for the request asking") + + f.addSandbox(t, "a", compute_v1alpha.RUNNING, "http://a", 5) + assert.Equal(t, int64(2), target(), "a full sandbox plus headroom wants a second") + + f.addSandbox(t, "b", compute_v1alpha.PENDING, "", 0) + assert.Equal(t, int64(2), target(), "pending sandboxes count through DesiredInstances, not demand") + + a.mu.Lock() + for range 10 { + a.newSandboxChans[f.key] = append(a.newSandboxChans[f.key], make(chan struct{}, 1)) + } + a.mu.Unlock() + assert.Equal(t, int64(3), target(), "parked waiters add a slot each: (10+10+2)/10") + + a.mu.Lock() + for range 400 { + a.newSandboxChans[f.key] = append(a.newSandboxChans[f.key], make(chan struct{}, 1)) + } + a.mu.Unlock() + assert.Equal(t, int64(concurrency.MaxPoolSize), target(), "demand is capped at the strategy maximum") +} + +// A pool created after startup is known only through its sandboxes, not +// a.pools, and over-target admission still has to grow it. +func TestOverTargetScalesPoolNotYetCached(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + f.activator.mu.Lock() + delete(f.activator.pools, f.key) + f.activator.mu.Unlock() + f.addSandbox(t, "only", compute_v1alpha.RUNNING, "http://only", 5) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + lease, err := f.activator.AcquireLease(ctx, f.ver, "web") + require.NoError(t, err) + require.NotNil(t, lease) + assert.True(t, lease.Overflow, "a lease admitted over target is marked as overflow") + + // 12 slots leased + 2 headroom over 10 per sandbox → 2 sandboxes. + require.Eventually(t, func() bool { return f.storedDesired(t) == 2 }, 5*time.Second, 20*time.Millisecond) +} + +// A re-sync after a watch disconnect refreshes a cached pool whose size +// changed in the store meanwhile. Otherwise a missed scale-down leaves the +// cache claiming more sandboxes than exist, and over-target admission never +// scales back up. +func TestResyncRefreshesStalePoolSize(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + ctx := context.Background() + + f.activator.mu.Lock() + state := f.activator.pools[f.key] + stale := *state.pool + stale.DesiredInstances = 20 + state.pool = &stale + f.activator.mu.Unlock() + + _, err := f.server.EAC.Patch(ctx, []entity.Attr{ + {ID: entity.DBId, Value: entity.AnyValue(f.poolID)}, + {ID: compute_v1alpha.SandboxPoolDesiredInstancesId, Value: entity.AnyValue(int64(1))}, + }, 0) + require.NoError(t, err) + + require.NoError(t, f.activator.recoverPools(ctx)) + + f.addSandbox(t, "only", compute_v1alpha.RUNNING, "http://only", 5) + + lease, err := f.activator.AcquireLease(ctx, f.ver, "web") + require.NoError(t, err) + require.NotNil(t, lease) + + require.Eventually(t, func() bool { return f.storedDesired(t) == 2 }, 5*time.Second, 20*time.Millisecond) +} + +// A pool update the watch never delivered can leave the cache claiming more +// sandboxes than exist. Over-target admission re-reads the pool when fewer +// sandboxes are live than the cache claims, and scales from the real size. +func TestOverTargetRereadsPoolWhenCacheOverstatesIt(t *testing.T) { + f := newSoftAdmissionFixture(t, 1) + ctx := context.Background() + + f.activator.mu.Lock() + state := f.activator.pools[f.key] + stale := *state.pool + stale.DesiredInstances = 20 + state.pool = &stale + f.activator.mu.Unlock() + + _, err := f.server.EAC.Patch(ctx, []entity.Attr{ + {ID: entity.DBId, Value: entity.AnyValue(f.poolID)}, + {ID: compute_v1alpha.SandboxPoolDesiredInstancesId, Value: entity.AnyValue(int64(1))}, + }, 0) + require.NoError(t, err) + + f.addSandbox(t, "only", compute_v1alpha.RUNNING, "http://only", 5) + + lease, err := f.activator.AcquireLease(ctx, f.ver, "web") + require.NoError(t, err) + require.NotNil(t, lease) + + require.Eventually(t, func() bool { return f.storedDesired(t) == 2 }, 5*time.Second, 20*time.Millisecond) +} diff --git a/pkg/concurrency/strategy.go b/pkg/concurrency/strategy.go index 44ebbd02c..076a35c31 100644 --- a/pkg/concurrency/strategy.go +++ b/pkg/concurrency/strategy.go @@ -30,6 +30,13 @@ func (t *ConcurrencyTracker) AcquireLease() int { return size } +// AcquireOverflow allocates one slot past capacity for a request admitted +// over target. Such a lease carries only that request, so it isn't batched. +func (t *ConcurrencyTracker) AcquireOverflow() int { + t.used++ + return 1 +} + func (t *ConcurrencyTracker) ReleaseLease(size int) { t.strategy.releaseCapacity(t, size) } diff --git a/servers/httpingress/httpingress.go b/servers/httpingress/httpingress.go index 8a815ac29..f6147dfa7 100644 --- a/servers/httpingress/httpingress.go +++ b/servers/httpingress/httpingress.go @@ -397,6 +397,7 @@ type lease struct { Uses int LastUsed time.Time Lease *activator.Lease + app string } func (h *Server) retainLease(ctx context.Context, app string, l *activator.Lease) *lease { @@ -407,6 +408,7 @@ func (h *Server) retainLease(ctx context.Context, app string, l *activator.Lease Lease: l, Uses: 1, LastUsed: time.Now(), + app: app, } ar, ok := h.apps[app] @@ -435,6 +437,9 @@ func (h *Server) useLease(ctx context.Context, app string) (*lease, error) { } for _, l := range ar.leases { + if l.Lease.Overflow { + continue + } if l.Uses <= l.Lease.Size { l.Uses++ l.LastUsed = time.Now() @@ -450,6 +455,25 @@ func (h *Server) releaseLease(ctx context.Context, lease *lease) { defer h.mu.Unlock() lease.Uses-- + + // Cached, an overflow lease would keep load pinned to its sandbox after + // new ones boot. + if lease.Lease.Overflow && lease.Uses <= 0 { + ar, ok := h.apps[lease.app] + if !ok { + return + } + for i, l := range ar.leases { + if l == lease { + h.aa.ReleaseLease(context.WithoutCancel(ctx), l.Lease) + ar.leases = append(ar.leases[:i], ar.leases[i+1:]...) + break + } + } + if len(ar.leases) == 0 { + delete(h.apps, lease.app) + } + } } // invalidateLease removes a lease from the cache entirely. diff --git a/servers/httpingress/lease_test.go b/servers/httpingress/lease_test.go index 260d418d7..7dfa6bd2b 100644 --- a/servers/httpingress/lease_test.go +++ b/servers/httpingress/lease_test.go @@ -531,3 +531,40 @@ func TestSandboxDeathAcrossMultipleApps(t *testing.T) { t.Errorf("expected 2 ReleaseLease calls, got %d", aa.releaseCount) } } + +// Overflow leases are never reused and go back to the activator when their +// request finishes. +func TestOverflowLeaseReleasedAfterItsRequest(t *testing.T) { + aa := &mockActivator{} + srv := newTestServer(aa) + ctx := context.Background() + + overflow := srv.retainLease(ctx, "app/myapp", &activator.Lease{Size: 2, URL: "http://10.0.0.2:3000", Overflow: true}) + normal := srv.retainLease(ctx, "app/myapp", &activator.Lease{Size: 2, URL: "http://10.0.0.1:3000"}) + srv.releaseLease(ctx, normal) + + got, err := srv.useLease(ctx, "app/myapp") + if err != nil { + t.Fatal(err) + } + if got != normal { + t.Fatalf("overflow lease must not be reused, got %v", got) + } + srv.releaseLease(ctx, got) + + srv.releaseLease(ctx, overflow) + + aa.mu.Lock() + released := append([]string(nil), aa.releasedURLs...) + aa.mu.Unlock() + if len(released) != 1 || released[0] != "http://10.0.0.2:3000" { + t.Fatalf("expected only the overflow lease released to the activator, got %v", released) + } + + srv.mu.Lock() + leases := srv.apps["app/myapp"].leases + srv.mu.Unlock() + if len(leases) != 1 || leases[0] != normal { + t.Fatalf("expected only the normal lease left in the cache, got %d leases", len(leases)) + } +} From 28f3b22f0e9e6a65508e2c8899b221937b6e5c41 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 16 Oct 2026 16:53:13 -0500 Subject: [PATCH 2/4] Stop ingress lease uses from leaking or overrunning Size 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. --- servers/httpingress/httpingress.go | 22 ++++++++-- servers/httpingress/lease_test.go | 67 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/servers/httpingress/httpingress.go b/servers/httpingress/httpingress.go index f6147dfa7..d90491373 100644 --- a/servers/httpingress/httpingress.go +++ b/servers/httpingress/httpingress.go @@ -440,7 +440,7 @@ func (h *Server) useLease(ctx context.Context, app string) (*lease, error) { if l.Lease.Overflow { continue } - if l.Uses <= l.Lease.Size { + if l.Uses < l.Lease.Size { l.Uses++ l.LastUsed = time.Now() return l, nil @@ -1066,7 +1066,7 @@ func (h *Server) serveAuthenticatedRequest(w http.ResponseWriter, req *http.Requ req = req.WithContext(ctx) // On non-final attempts, suppress error response so we can retry writeErr := attempt == maxRetries - err = h.proxyToLease(w, req, curLease.Lease.URL, targetAppId.String(), *appName, writeErr, requestTimeout) + err = h.proxyWithLease(w, req, curLease, targetAppId.String(), *appName, writeErr, requestTimeout) if err != nil && isProxyConnectionError(err) { // Cached lease pointed at a dead sandbox — invalidate all app // leases (they likely all point to the same dead sandbox) and retry @@ -1133,7 +1133,7 @@ func (h *Server) serveAuthenticatedRequest(w http.ResponseWriter, req *http.Requ req = req.WithContext(ctx) // Fresh lease — always write error response (no retry on fresh lease failure) - err = h.proxyToLease(w, req, actLease.URL, targetAppId.String(), *appName, true, requestTimeout) + err = h.proxyWithLease(w, req, localLease, targetAppId.String(), *appName, true, requestTimeout) if err != nil { // Connection error on a fresh lease - the sandbox may have died // between lease acquisition and proxy. Invalidate immediately. @@ -1233,6 +1233,22 @@ func (h *Server) logRequestFromStats(appEntityID, appName string, stats httputil // captures the error but does NOT write to the ResponseWriter, allowing the caller // to retry with a fresh lease. This is safe because connection errors happen during // TCP dial, before any response bytes are sent. +// proxyWithLease proxies to a cached lease's sandbox and hands the lease's +// use back if the proxy panics. ReverseProxy aborts a response whose body copy +// fails (a client hanging up mid-download) by panicking with +// http.ErrAbortHandler, which skips the caller's releaseLease. Each one leaked +// a use, and a lease that never returns to zero uses never expires, so its +// sandbox capacity stayed claimed for the sandbox's lifetime. +func (h *Server) proxyWithLease(w http.ResponseWriter, req *http.Request, l *lease, appEntityID, appName string, writeErrorResponse bool, requestTimeout time.Duration) error { + defer func() { + if r := recover(); r != nil { + h.releaseLease(context.Background(), l) + panic(r) + } + }() + return h.proxyToLease(w, req, l.Lease.URL, appEntityID, appName, writeErrorResponse, requestTimeout) +} + func (h *Server) proxyToLease(w http.ResponseWriter, req *http.Request, targetURL, appEntityID, appName string, writeErrorResponse bool, requestTimeout time.Duration) error { targetParsed, err := url.Parse(targetURL) if err != nil { diff --git a/servers/httpingress/lease_test.go b/servers/httpingress/lease_test.go index 7dfa6bd2b..b52ca19cc 100644 --- a/servers/httpingress/lease_test.go +++ b/servers/httpingress/lease_test.go @@ -4,6 +4,9 @@ import ( "context" "errors" "log/slog" + "net/http" + "net/http/httptest" + "strings" "sync" "testing" "time" @@ -568,3 +571,67 @@ func TestOverflowLeaseReleasedAfterItsRequest(t *testing.T) { t.Fatalf("expected only the normal lease left in the cache, got %d leases", len(leases)) } } + +// A cached lease serves at most Size concurrent requests. The check used to be +// Uses <= Size, which let a lease sized for two requests carry three. +func TestUseLeaseStopsAtSize(t *testing.T) { + srv := newTestServer(&mockActivator{}) + ctx := context.Background() + + ll := srv.retainLease(ctx, "app/myapp", &activator.Lease{Size: 2, URL: "http://10.0.0.1:3000"}) + + second, err := srv.useLease(ctx, "app/myapp") + if err != nil || second != ll { + t.Fatalf("expected the cached lease for the second request, got %v, %v", second, err) + } + + third, err := srv.useLease(ctx, "app/myapp") + if err != nil { + t.Fatal(err) + } + if third != nil { + t.Fatalf("expected no cached lease past Size, got one with Uses=%d", third.Uses) + } +} + +type failingWriter struct { + header http.Header +} + +func (f *failingWriter) Header() http.Header { return f.header } +func (f *failingWriter) WriteHeader(int) {} +func (f *failingWriter) Write([]byte) (int, error) { return 0, errors.New("client went away") } + +// When the client hangs up mid-response, ReverseProxy panics with +// http.ErrAbortHandler. The lease's use must still be handed back, or the lease +// never drains to zero uses and never expires. +func TestProxyWithLeaseReleasesOnAbort(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(strings.Repeat("x", 64<<10))) + })) + defer backend.Close() + + srv := newTestServer(&mockActivator{}) + srv.transport = http.DefaultTransport.(*http.Transport).Clone() + ctx := context.Background() + + ll := srv.retainLease(ctx, "app/myapp", &activator.Lease{Size: 2, URL: backend.URL}) + + req := httptest.NewRequest("GET", "/", nil) + req = req.WithContext(context.WithValue(req.Context(), http.ServerContextKey, &http.Server{})) + + func() { + defer func() { + if r := recover(); r != http.ErrAbortHandler { + t.Fatalf("expected the ErrAbortHandler panic to propagate, got %v", r) + } + }() + srv.proxyWithLease(&failingWriter{header: http.Header{}}, req, ll, "app/myapp", "myapp", true, 0) + }() + + srv.mu.Lock() + defer srv.mu.Unlock() + if ll.Uses != 0 { + t.Fatalf("expected the aborted request's use to be released, got Uses=%d", ll.Uses) + } +} From 7ad7abbc46746c8cc8c8828ed2b36479037818a1 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 16 Oct 2026 16:53:13 -0500 Subject: [PATCH 3/4] Make scale-down conditional on the pool revision it read 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. --- controllers/sandboxpool/manager.go | 16 ++++++-- controllers/sandboxpool/manager_test.go | 50 ++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/controllers/sandboxpool/manager.go b/controllers/sandboxpool/manager.go index d3eef4c4b..04d240d01 100644 --- a/controllers/sandboxpool/manager.go +++ b/controllers/sandboxpool/manager.go @@ -661,7 +661,7 @@ func (m *Manager) checkAllPoolsForScaleDown(ctx context.Context) error { minInstances := int64(strategy.MinInstances()) // Check if this pool has idle sandboxes that should be retired - if err := m.checkPoolForScaleDown(ctx, &pool, scaleDownDelay, minInstances); err != nil { + if err := m.checkPoolForScaleDown(ctx, &pool, ent.Revision(), scaleDownDelay, minInstances); err != nil { m.log.Error("failed to check pool for scale-down", "pool", pool.ID, "error", err) @@ -673,7 +673,13 @@ func (m *Manager) checkAllPoolsForScaleDown(ctx context.Context) error { // checkPoolForScaleDown examines a single pool and decrements DesiredInstances // if there are idle sandboxes that exceed the ScaleDownDelay threshold. -func (m *Manager) checkPoolForScaleDown(ctx context.Context, pool *compute_v1alpha.SandboxPool, scaleDownDelay time.Duration, minInstances int64) error { +// +// The write is conditional on revision, the pool revision the caller read. +// newDesired is computed from that read, so writing it unconditionally could +// undo a scale-up the activator made in between, and the activator's own +// cache would then disagree with the store. On a conflict the pool +// is simply re-examined on the next tick. +func (m *Manager) checkPoolForScaleDown(ctx context.Context, pool *compute_v1alpha.SandboxPool, revision int64, scaleDownDelay time.Duration, minInstances int64) error { // Get all sandboxes for this pool sandboxes, err := m.listSandboxes(ctx, pool) if err != nil { @@ -721,7 +727,11 @@ func (m *Manager) checkPoolForScaleDown(ctx context.Context, pool *compute_v1alp entity.Int64(compute_v1alpha.SandboxPoolDesiredInstancesId, newDesired), } - if _, err := m.eac.Patch(ctx, attrs, 0); err != nil { + if _, err := m.eac.Patch(ctx, attrs, revision); err != nil { + if errors.Is(err, cond.ErrConflict{}) { + m.log.Debug("pool changed since scale-down check, retrying next tick", "pool", pool.ID) + return nil + } return fmt.Errorf("failed to update pool desired instances: %w", err) } } diff --git a/controllers/sandboxpool/manager_test.go b/controllers/sandboxpool/manager_test.go index 9eea432cd..2acacb204 100644 --- a/controllers/sandboxpool/manager_test.go +++ b/controllers/sandboxpool/manager_test.go @@ -647,7 +647,9 @@ func TestManagerZeroValuePersistence(t *testing.T) { } pool.DesiredInstances = 5 - err = manager.checkPoolForScaleDown(ctx, pool, 1*time.Second, 0) + poolRes, err := server.EAC.Get(ctx, poolID.String()) + require.NoError(t, err) + err = manager.checkPoolForScaleDown(ctx, pool, poolRes.Entity().Revision(), 1*time.Second, 0) require.NoError(t, err) updatedPool = getPool(t, ctx, server, poolID) @@ -1828,3 +1830,49 @@ func TestManagerHealthySandboxResetPersists(t *testing.T) { assert.Equal(t, int64(0), updated.ConsecutiveCrashCount) assert.True(t, updated.CooldownUntil.IsZero()) } + +// A scale-down computed from a stale read must not overwrite a scale-up that +// landed in between. +func TestScaleDownDoesNotOverwriteConcurrentScaleUp(t *testing.T) { + ctx := context.Background() + server, cleanup := testutils.NewInMemEntityServer(t) + defer cleanup() + + pool := &compute_v1alpha.SandboxPool{ + Service: "web", + DesiredInstances: 3, + SandboxSpec: compute_v1alpha.SandboxSpec{ + Version: entity.Id("ver-1"), + Container: []compute_v1alpha.SandboxSpecContainer{{Image: "test:latest"}}, + }, + } + poolID, err := server.Client.Create(ctx, "test-pool", pool) + require.NoError(t, err) + pool.ID = poolID + + for i := range 3 { + _, err = server.Client.Create(ctx, fmt.Sprintf("sb-%d", i), &compute_v1alpha.Sandbox{ + Status: compute_v1alpha.RUNNING, + LastActivity: time.Now().Add(-10 * time.Minute), + Spec: pool.SandboxSpec, + }, entityserver.WithLabels(types.LabelSet("service", pool.Service, "pool", poolID.String()))) + require.NoError(t, err) + } + + staleRes, err := server.EAC.Get(ctx, poolID.String()) + require.NoError(t, err) + staleRevision := staleRes.Entity().Revision() + + // The activator scales up after the monitor read the pool. + _, err = server.EAC.Patch(ctx, []entity.Attr{ + entity.Ref(entity.DBId, poolID), + entity.Int64(compute_v1alpha.SandboxPoolDesiredInstancesId, 6), + }, staleRevision) + require.NoError(t, err) + + manager := NewManager(testutils.TestLogger(t), server.EAC) + require.NoError(t, manager.checkPoolForScaleDown(ctx, pool, staleRevision, time.Second, 0)) + + assert.Equal(t, int64(6), getPool(t, ctx, server, poolID).DesiredInstances, + "the scale-up must survive a scale-down computed from the earlier read") +} From 2abcdf46a75f9f295144607885d5b1c0e50e00fe Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 4 Nov 2026 10:36:21 -0600 Subject: [PATCH 4/4] Changelog for the over-target admission backport --- docs/docs/changelog.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/docs/changelog.md b/docs/docs/changelog.md index 961782266..5dd31e2ea 100644 --- a/docs/docs/changelog.md +++ b/docs/docs/changelog.md @@ -11,6 +11,9 @@ All notable changes to Miren Runtime will be documented in this file. ## Unreleased *release/0.17* +**Bug Fixes** +- **Busy autoscaled services keep serving at capacity instead of hanging** - When every sandbox hit its `requests_per_instance` target, requests parked in the activator waiting for a new sandbox, and failed with "no sandbox became available within 120 seconds" even when the sandboxes had room. Once a service reached its maximum of 20 instances, no new sandbox could come, so requests hung until they timed out. Requests now go to the least-loaded sandbox and the pool grows in the background. At the maximum, requests keep spreading across the sandboxes a service has. A response cut off partway, by a client hanging up or a silent stream timing out, also no longer leaks capacity for the life of the sandbox, and a scale-down can no longer overwrite a scale-up that happened at the same moment. ([#1387](https://github.com/mirendev/runtime/pull/1387)) + --- ## v0.17.2