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(ScopeLabel(w.scope), 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,40 +157,21 @@ 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(ScopeLabel(w.backgroundScope), w.name, extra)
 			continue
 		}
 
 		if w.num <= 0 {
 			// https://github.com/php/frankenphp/issues/126
 			opt.workers[i].num = maxProcs
 		}
-		metrics.TotalWorkers(ScopeLabel(w.backgroundScope), w.name, w.num)
+		metrics.TotalWorkers(ScopeLabel(w.scope), w.name, w.num)
 
 		numWorkers += opt.workers[i].num
 

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
 	}

threadbackgroundworker.go

@@ -136,7 +136,7 @@ func (handler *backgroundWorkerThread) setupScript() {
 	if handler.runtimeName == "" {
 		handler.runtimeName = handler.worker.name
 	}
-	metrics.StartWorker(ScopeLabel(handler.worker.backgroundScope), handler.runtimeName)
+	metrics.StartWorker(ScopeLabel(handler.worker.scope), handler.runtimeName)
 
 	opts := append([]RequestOption(nil), handler.worker.requestOptions...)
 	C.frankenphp_set_background_worker(C._Bool(true))
@@ -185,7 +185,7 @@ func (handler *backgroundWorkerThread) afterScriptExecution(exitStatus int) {
 		return
 	}
 
-	server := ScopeLabel(worker.backgroundScope)
+	server := ScopeLabel(worker.scope)
 
 	// Cooperative exit: re-run, reset backoff.
 	if exitStatus == 0 {

threadworker.go

@@ -110,7 +110,7 @@ func (handler *workerThread) scopedWorker() *worker { return handler.worker }
 func (handler *workerThread) drain() {}
 
 func setupWorkerScript(handler *workerThread, worker *worker) {
-	metrics.StartWorker(ScopeLabel(worker.backgroundScope), worker.name)
+	metrics.StartWorker(ScopeLabel(worker.scope), worker.name)
 
 	// Create a dummy request to set up the worker
 	fc, err := newDummyContext(
@@ -149,7 +149,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) {
 		handler.thread.contextMu.Unlock()
 	}
 
-	server := ScopeLabel(worker.backgroundScope)
+	server := ScopeLabel(worker.scope)
 
 	// on exit status 0 we just run the worker script again
 	if exitStatus == 0 && !handler.isBootingScript {
@@ -224,7 +224,7 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) {
 		}
 
 		// worker is truly ready only after reaching frankenphp_handle_request()
-		metrics.ReadyWorker(ScopeLabel(handler.worker.backgroundScope), handler.worker.name)
+		metrics.ReadyWorker(ScopeLabel(handler.worker.scope), handler.worker.name)
 	}
 
 	// max_requests reached: signal reboot for full ZTS cleanup

worker.go

@@ -34,7 +34,7 @@ type worker struct {
 	onThreadShutdown       func(int)
 	queuedRequests         atomic.Int32
 	isBackgroundWorker     bool
-	backgroundScope        Scope
+	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)
@@ -350,7 +350,7 @@ func (worker *worker) isAtThreadLimit() bool {
 }
 
 func (worker *worker) handleRequest(ch contextHolder) error {
-	server := ScopeLabel(worker.backgroundScope)
+	server := ScopeLabel(worker.scope)
 	metrics.StartWorkerRequest(server, worker.name)
 
 	runtime.Gosched()