review: address AlliBalliBaba's bg-worker thread accounting feedback

Two follow-ups on the per-php_server scoping batch:

- Extract reserveBackgroundWorkerThreads from calculateMaxThreads. The
  bg-worker branch was a dense block of defaults + slot reservation +
  metric registration; lifting it into its own function keeps
  calculateMaxThreads focused on HTTP-worker accounting and makes the
  three lazy-start cases (named/named-with-cap/catch-all) read in one
  place.

- Rename the leftover backgroundScope field to scope on worker,
  workerOpt, frankenPHPContext and FrankenPHPModule. The type was
  already shortened from BackgroundScope to Scope; the field names lag.

Author: nicolas-grekas

bgworker.go

@@ -149,15 +149,15 @@ func buildBackgroundWorkerLookups(workers []*worker, opts []workerOpt) (map[Scop
 			continue
 		}
 
-		scope := o.backgroundScope
+		scope := o.scope
 		lookup, ok := lookups[scope]
 		if !ok {
 			lookup = newBackgroundWorkerLookup()
 			lookups[scope] = lookup
 		}
 
 		w := workers[i]
-		w.backgroundScope = scope
+		w.scope = scope
 		// Pre-allocate so ensure() against an already-running eager worker
 		// has a channel to wait on. Catch-all templates allocate it too but
 		// it stays unused (each lazy-spawned name gets its own slot).
@@ -191,6 +191,43 @@ func buildBackgroundWorkerLookups(workers []*worker, opts []workerOpt) (map[Scop
 	return lookups, nil
 }
 
+// reserveBackgroundWorkerThreads resolves max_threads defaults for bg
+// workers and returns the worst-case thread footprint to add to the pool
+// so ensure() always has a free slot to schedule into. Mutates
+// opt.workers in place. Also pre-registers totalWorkers — without this,
+// a bg-worker-only deployment never initialises the metric and every
+// StartWorker/ReadyWorker call becomes a silent no-op.
+func reserveBackgroundWorkerThreads(opt *opt) int {
+	reserved := 0
+	for i, w := range opt.workers {
+		if !w.isBackgroundWorker {
+			continue
+		}
+		// num=0 max_threads=0 is the lazy-start opt-in. Distinguish a
+		// catch-all (no name / name == file) — which can host up to
+		// defaultMaxBackgroundWorkers distinct lazy-started names —
+		// from a named lazy worker, which only needs one slot.
+		if w.maxThreads == 0 && w.num == 0 {
+			phpName := strings.TrimPrefix(w.name, "m#")
+			if phpName == "" || phpName == w.fileName {
+				opt.workers[i].maxThreads = defaultMaxBackgroundWorkers
+			} else {
+				opt.workers[i].maxThreads = 1
+			}
+		}
+		extra := w.num
+		if extra < 1 {
+			extra = 1
+		}
+		if opt.workers[i].maxThreads > extra {
+			extra = opt.workers[i].maxThreads
+		}
+		reserved += extra
+		metrics.TotalWorkers(w.name, extra)
+	}
+	return reserved
+}
+
 // getLookup returns the background-worker lookup for the calling thread,
 // resolving the scope via worker handler -> request context -> global. A
 // scope with no declared workers falls through to scope 0 so embed-mode
@@ -205,9 +242,9 @@ func getLookup(thread *phpThread) *backgroundWorkerLookup {
 		// via this anonymous interface; classic-mode threads don't and fall
 		// through to the request-context path.
 		if h, ok := thread.handler.(interface{ scopedWorker() *worker }); ok {
-			scope = h.scopedWorker().backgroundScope
+			scope = h.scopedWorker().scope
 		} else if fc, ok := fromContext(thread.context()); ok {
-			scope = fc.backgroundScope
+			scope = fc.scope
 		}
 	}
 	if scope != 0 {

bgworkerscope_test.go

@@ -92,7 +92,7 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
 	// We use a request-scope tag (rather than a worker handler) because
 	// no scope-specific handler is running in the test.
 	fc := newFrankenPHPContext()
-	fc.backgroundScope = scopeA
+	fc.scope = scopeA
 	ctx := context.WithValue(context.Background(), contextKey, fc)
 	thread := &phpThread{}
 	thread.handler = &fakeContextThread{ctx: ctx}

caddy/module.go

@@ -52,7 +52,7 @@ type FrankenPHPModule struct {
 	preparedEnvNeedsReplacement bool
 	logger                      *slog.Logger
 	requestOptions              []frankenphp.RequestOption
-	backgroundScope             frankenphp.Scope
+	scope                       frankenphp.Scope
 	scopeLabelOnce              sync.Once
 }
 
@@ -81,12 +81,11 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 
 	f.assignMercureHub(ctx)
 
-	// Each php_server block gets its own background-worker scope so
-	// workers declared with the same name in different blocks don't
-	// collide. Provision can be called more than once for the same
-	// module; only assign once.
-	if f.backgroundScope == 0 {
-		f.backgroundScope = frankenphp.NextScope()
+	// Each php_server block gets its own scope so workers declared with
+	// the same name in different blocks don't collide. Provision can be
+	// called more than once for the same module; only assign once.
+	if f.scope == 0 {
+		f.scope = frankenphp.NextScope()
 	}
 
 	loggerOpt := frankenphp.WithRequestLogger(f.logger)
@@ -103,7 +102,7 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 		}
 
 		wc.requestOptions = append(wc.requestOptions, loggerOpt)
-		wc.options = append(wc.options, frankenphp.WithWorkerScope(f.backgroundScope))
+		wc.options = append(wc.options, frankenphp.WithWorkerScope(f.scope))
 		f.Workers[i] = wc
 	}
 
@@ -253,7 +252,7 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c
 			return
 		}
 		if label := f.resolveScopeLabel(srv); label != "" {
-			frankenphp.SetScopeLabel(f.backgroundScope, label)
+			frankenphp.SetScopeLabel(f.scope, label)
 		}
 	})
 
@@ -263,7 +262,7 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c
 			opts,
 			frankenphp.WithOriginalRequest(new(ctx.Value(caddyhttp.OriginalRequestCtxKey).(http.Request))),
 			frankenphp.WithWorkerName(workerName),
-			frankenphp.WithRequestScope(f.backgroundScope),
+			frankenphp.WithRequestScope(f.scope),
 		)...,
 	)
 

context.go

@@ -38,9 +38,9 @@ type frankenPHPContext struct {
 	handlerParameters  any
 	handlerReturn      any
 
-	// backgroundScope selects the background-worker isolation boundary used
-	// by ensure() calls made from this request. Zero is the global scope.
-	backgroundScope Scope
+	// scope selects the per-php_server isolation boundary used by ensure()
+	// calls made from this request. Zero is the global scope.
+	scope Scope
 
 	done      chan any
 	startedAt time.Time

frankenphp.go

@@ -157,32 +157,13 @@ func Config() PHPConfig {
 func calculateMaxThreads(opt *opt) (numWorkers int, _ error) {
 	maxProcs := runtime.GOMAXPROCS(0) * 2
 	maxThreadsFromWorkers := 0
-	reservedThreads := 0
+	// Bg workers reserve thread budget separately from HTTP workers so
+	// they don't double-count against the HTTP-worker admission check
+	// further down.
+	reservedThreads := reserveBackgroundWorkerThreads(opt)
 
 	for i, w := range opt.workers {
 		if w.isBackgroundWorker {
-			// Background workers default to one eager thread for an
-			// explicit num. For a num=0 declaration, ensure() lazy-starts
-			// it; we still need to reserve thread budget so getInactivePHPThread
-			// has somewhere to put it. For catch-all declarations,
-			// max_threads is the cap on lazy-started instances; reserve
-			// room for each so ensure() has slots to schedule into.
-			isCatchAll := w.name == "" || w.name == w.fileName
-			if w.maxThreads == 0 && w.num == 0 && isCatchAll {
-				opt.workers[i].maxThreads = defaultMaxBackgroundWorkers
-			}
-			extra := w.num
-			if extra < 1 {
-				extra = 1
-			}
-			if opt.workers[i].maxThreads > extra {
-				extra = opt.workers[i].maxThreads
-			}
-			reservedThreads += extra
-			// Register the expected worker count for metrics too: without
-			// this, a bg-worker-only deployment never initialises
-			// totalWorkers, and StartWorker calls become silent no-ops.
-			metrics.TotalWorkers(w.name, extra)
 			continue
 		}
 

options.go

@@ -51,7 +51,7 @@ type workerOpt struct {
 	onServerStartup        func()
 	onServerShutdown       func()
 	isBackgroundWorker     bool
-	backgroundScope        Scope
+	scope                  Scope
 }
 
 // WithContext sets the main context to use.
@@ -273,14 +273,14 @@ func WithWorkerBackground() WorkerOption {
 	}
 }
 
-// EXPERIMENTAL: WithWorkerScope assigns this worker to a given
-// background-worker scope. Workers in the same scope share a background
-// lookup; each php_server block gets its own scope so workers with the
-// same name in different blocks don't collide. The zero value is the
-// global/embed scope and is the default.
+// EXPERIMENTAL: WithWorkerScope assigns this worker to a given scope.
+// Workers in the same scope share a background lookup; each php_server
+// block gets its own scope so workers with the same name in different
+// blocks don't collide. The zero value is the global/embed scope and is
+// the default.
 func WithWorkerScope(scope Scope) WorkerOption {
 	return func(w *workerOpt) error {
-		w.backgroundScope = scope
+		w.scope = scope
 
 		return nil
 	}

requestoptions.go

@@ -154,13 +154,13 @@ func WithRequestLogger(logger *slog.Logger) RequestOption {
 	}
 }
 
-// WithRequestScope selects the background-worker scope for
-// ensure() calls made from this request. Web-server integrations (such
-// as the Caddy module) can call NextScope per
-// independently-configured block to isolate its set of background workers.
+// WithRequestScope selects the scope for ensure() calls made from this
+// request. Web-server integrations (such as the Caddy module) can call
+// NextScope per independently-configured block to isolate its set of
+// background workers.
 func WithRequestScope(scope Scope) RequestOption {
 	return func(o *frankenPHPContext) error {
-		o.backgroundScope = scope
+		o.scope = scope
 
 		return nil
 	}

worker.go

@@ -33,8 +33,8 @@ type worker struct {
 	onThreadReady          func(int)
 	onThreadShutdown       func(int)
 	queuedRequests         atomic.Int32
-	isBackgroundWorker bool
-	backgroundScope    Scope
+	isBackgroundWorker     bool
+	scope                  Scope
 	// backgroundReady is the bg-worker readiness signal shared across the
 	// worker's threads. Unused on the catch-all template (each lazy-
 	// spawned name gets its own slot in catchAllNames).
@@ -199,7 +199,7 @@ func newWorker(o workerOpt) (*worker, error) {
 		onThreadReady:          o.onThreadReady,
 		onThreadShutdown:       o.onThreadShutdown,
 		isBackgroundWorker:     o.isBackgroundWorker,
-		backgroundScope:        o.backgroundScope,
+		scope:                  o.scope,
 	}
 
 	w.configureMercure(&o)