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/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") +} 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 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..d90491373 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,7 +437,10 @@ func (h *Server) useLease(ctx context.Context, app string) (*lease, error) { } for _, l := range ar.leases { - if l.Uses <= l.Lease.Size { + if l.Lease.Overflow { + continue + } + if l.Uses < l.Lease.Size { l.Uses++ l.LastUsed = time.Now() return l, nil @@ -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. @@ -1042,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 @@ -1109,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. @@ -1209,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 260d418d7..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" @@ -531,3 +534,104 @@ 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)) + } +} + +// 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) + } +}