review: address dunglas's review batch

- BackgroundScope -> Scope (and NextScope / WithRequestScope /
  WithWorkerScope), keeping the type opaque + reusable for other
  per-server isolation contexts (Mercure hubs, future Prometheus
  labels). No behaviour change.
- $_SERVER['FRANKENPHP_WORKER'] now carries the user-facing worker
  name instead of "1". The doc has always told callers to rely on
  presence, not value. Drops the parallel FRANKENPHP_WORKER_NAME
  injection.
- backgroundWorkerReady.abortErr keeps the full error (not just its
  message), so the wrap chain survives into ensure() callers.
- Cap-error wording: drop the Caddyfile-specific "max_threads on the
  catch-all" phrasing for "increase max threads or declare it as a
  named worker", to read cleanly when the user is on the JSON config.
- Constants merged into a single block; getLookup folds the worker /
  background-worker handler cases via a small anonymous interface
  (Go's type switch can't fan out type-asserted access otherwise).
- Stub doc nits: third-person godoc ("Declares" / "Returns").
- Test ergonomics: requireSentinelEventually wraps the duplicated
  Eventually glue; require.NoFileExists, require.ErrorContains,
  assert.WithinDuration replace ad-hoc combinations.
- Renamed background_worker_*_test.go to bgworker*_test.go to match
  the bgworker.go source file (no underscores in file-name prefixes).

Author: nicolas-grekas

bgworker.go

@@ -13,14 +13,16 @@ import (
 	"unsafe"
 )
 
-// defaultMaxBackgroundWorkers is the default safety cap for catch-all
-// background workers when the user doesn't set max_threads. Caps the
-// number of distinct lazy-started instances from a single catch-all.
-const defaultMaxBackgroundWorkers = 16
-
-// defaultEnsureTimeout is the default deadline applied when ensure() is
-// called without an explicit timeout.
-const defaultEnsureTimeout = 30 * time.Second
+const (
+	// defaultMaxBackgroundWorkers is the default safety cap for catch-all
+	// background workers when the user doesn't set max_threads. Caps the
+	// number of distinct lazy-started instances from a single catch-all.
+	defaultMaxBackgroundWorkers = 16
+
+	// defaultEnsureTimeout is the default deadline applied when ensure() is
+	// called without an explicit timeout.
+	defaultEnsureTimeout = 30 * time.Second
+)
 
 // backgroundWorkerReady is the per-instance readiness signal ensure()
 // blocks on. ready closes once the worker calls
@@ -32,7 +34,7 @@ type backgroundWorkerReady struct {
 
 	aborted   chan struct{}
 	abortOnce sync.Once
-	abortErr  string
+	abortErr  error
 
 	// bootFailure carries the most recent pre-readiness crash so a timing-
 	// out ensure() can report it instead of a generic "didn't call
@@ -67,26 +69,26 @@ func (r *backgroundWorkerReady) markReady() {
 // (max_consecutive_failures hit before readiness). Idempotent.
 func (r *backgroundWorkerReady) abort(err error) {
 	r.abortOnce.Do(func() {
-		r.abortErr = err.Error()
+		r.abortErr = err
 		close(r.aborted)
 	})
 }
 
-// BackgroundScope is an opaque per-php_server isolation boundary; the
+// Scope is an opaque per-php_server isolation boundary; the
 // zero value is the global/embed scope. Obtain values via
-// NextBackgroundWorkerScope.
-type BackgroundScope uint64
+// NextScope.
+type Scope uint64
 
-var backgroundScopeCounter atomic.Uint64
+var scopeCounter atomic.Uint64
 
-// NextBackgroundWorkerScope returns a fresh scope value. Each php_server
+// NextScope returns a fresh scope value. Each php_server
 // block should call this once during provisioning.
-func NextBackgroundWorkerScope() BackgroundScope {
-	return BackgroundScope(backgroundScopeCounter.Add(1))
+func NextScope() Scope {
+	return Scope(scopeCounter.Add(1))
 }
 
 // backgroundLookups maps scope -> lookup. Scope 0 is the global/embed scope.
-var backgroundLookups map[BackgroundScope]*backgroundWorkerLookup
+var backgroundLookups map[Scope]*backgroundWorkerLookup
 
 // backgroundWorkerLookup resolves a user-facing worker name to its *worker;
 // catchAll is the fallback when byName misses.
@@ -113,8 +115,8 @@ func (l *backgroundWorkerLookup) resolve(name string) *worker {
 // lookup. Per-scope name collisions are caught here because bg workers
 // intentionally skip the global workersByName map (so two scopes can share
 // a user-facing name).
-func buildBackgroundWorkerLookups(workers []*worker, opts []workerOpt) (map[BackgroundScope]*backgroundWorkerLookup, error) {
-	lookups := make(map[BackgroundScope]*backgroundWorkerLookup)
+func buildBackgroundWorkerLookups(workers []*worker, opts []workerOpt) (map[Scope]*backgroundWorkerLookup, error) {
+	lookups := make(map[Scope]*backgroundWorkerLookup)
 
 	for i, o := range opts {
 		if !o.isBackgroundWorker {
@@ -171,17 +173,15 @@ func getLookup(thread *phpThread) *backgroundWorkerLookup {
 	if backgroundLookups == nil {
 		return nil
 	}
-	var scope BackgroundScope
+	var scope Scope
 	if thread != nil {
-		switch handler := thread.handler.(type) {
-		case *workerThread:
-			scope = handler.worker.backgroundScope
-		case *backgroundWorkerThread:
-			scope = handler.worker.backgroundScope
-		default:
-			if fc, ok := fromContext(thread.context()); ok {
-				scope = fc.backgroundScope
-			}
+		// Both *workerThread and *backgroundWorkerThread expose their *worker
+		// 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
+		} else if fc, ok := fromContext(thread.context()); ok {
+			scope = fc.backgroundScope
 		}
 	}
 	if scope != 0 {
@@ -228,13 +228,13 @@ func ensureBackgroundWorker(thread *phpThread, bgWorkerName string, timeout time
 
 	if catchAll.catchAllCap > 0 && len(catchAll.catchAllNames) >= catchAll.catchAllCap {
 		catchAll.catchAllMu.Unlock()
-		return fmt.Errorf("cannot start background worker %q: limit of %d reached (increase max_threads on the catch-all background worker or declare it as a named worker)", bgWorkerName, catchAll.catchAllCap)
+		return fmt.Errorf("cannot start background worker %q: limit of %d reached (increase max threads or declare it as a named worker)", bgWorkerName, catchAll.catchAllCap)
 	}
 
 	t := getInactivePHPThread()
 	if t == nil {
 		catchAll.catchAllMu.Unlock()
-		return fmt.Errorf("no available PHP thread for background worker (increase max_threads)")
+		return fmt.Errorf("no available PHP thread for background worker (increase max threads)")
 	}
 
 	r := newBackgroundWorkerReady()
@@ -264,7 +264,7 @@ func lazyStartNamedWorker(w *worker) error {
 	}
 	t := getInactivePHPThread()
 	if t == nil {
-		return fmt.Errorf("no available PHP thread for background worker (increase max_threads)")
+		return fmt.Errorf("no available PHP thread for background worker (increase max threads)")
 	}
 	convertToBackgroundWorkerThread(t, w, w.name, w.backgroundReady)
 	w.bgLazyStarted = true
@@ -287,7 +287,7 @@ func waitForBackgroundWorkerReady(name string, r *backgroundWorkerReady, timeout
 	case <-r.ready:
 		return nil
 	case <-r.aborted:
-		return fmt.Errorf("background worker %q failed to start: %s", name, r.abortErr)
+		return fmt.Errorf("background worker %q failed to start: %w", name, r.abortErr)
 	case <-timer.C:
 		return formatEnsureTimeoutError(name, r, timeout)
 	}

background_worker_batch_test.go bgworkerbatch_test.gobackground_worker_batch_test.go renamed to bgworkerbatch_test.go

@@ -12,18 +12,19 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// sentinelExists is a predicate suitable for assert.Eventually that returns
-// true once the named file appears on disk.
-func sentinelExists(path string) func() bool {
-	return func() bool {
+// requireSentinelEventually asserts that `path` appears on disk before the
+// deadline. Wraps require.Eventually so call sites stay short.
+func requireSentinelEventually(t testing.TB, path string, msgAndArgs ...any) {
+	t.Helper()
+	require.Eventually(t, func() bool {
 		_, err := os.Stat(path)
 		return err == nil
-	}
+	}, 5*time.Second, 10*time.Millisecond, msgAndArgs...)
 }
 
 // TestEnsureBackgroundWorkerNamedLazy declares a num=0 named worker, then
 // calls ensure() to lazy-start it. The fixture writes a sentinel named
-// after FRANKENPHP_WORKER_NAME so we can confirm the right instance ran.
+// after FRANKENPHP_WORKER so we can confirm the right instance ran.
 func TestEnsureBackgroundWorkerNamedLazy(t *testing.T) {
 	cwd, _ := os.Getwd()
 	testDataDir := cwd + "/testdata/"
@@ -40,12 +41,10 @@ func TestEnsureBackgroundWorkerNamedLazy(t *testing.T) {
 	t.Cleanup(Shutdown)
 
 	// num=0 means no eager start: the sentinel should not exist yet.
-	_, err := os.Stat(filepath.Join(tmp, "bg-lazy"))
-	require.True(t, os.IsNotExist(err), "lazy worker should not have started yet")
+	require.NoFileExists(t, filepath.Join(tmp, "bg-lazy"), "lazy worker should not have started yet")
 
 	require.NoError(t, ensureBackgroundWorker(nil, "bg-lazy", 5*time.Second))
-	assert.Eventually(t, sentinelExists(filepath.Join(tmp, "bg-lazy")),
-		5*time.Second, 10*time.Millisecond,
+	requireSentinelEventually(t, filepath.Join(tmp, "bg-lazy"),
 		"ensure() should have lazy-started the named bg worker")
 }
 
@@ -73,8 +72,7 @@ func TestEnsureBackgroundWorkerCatchAll(t *testing.T) {
 	}
 
 	for _, name := range []string{"job-a", "job-b"} {
-		assert.Eventually(t, sentinelExists(filepath.Join(tmp, name)),
-			5*time.Second, 10*time.Millisecond,
+		requireSentinelEventually(t, filepath.Join(tmp, name),
 			"catch-all instance %q should have written its sentinel", name)
 	}
 }
@@ -100,9 +98,10 @@ func TestEnsureBackgroundWorkerCatchAllCap(t *testing.T) {
 	require.NoError(t, ensureBackgroundWorker(nil, "cap-a", 5*time.Second))
 	require.NoError(t, ensureBackgroundWorker(nil, "cap-b", 5*time.Second))
 
-	err := ensureBackgroundWorker(nil, "cap-c", 5*time.Second)
-	require.Error(t, err, "third ensure must hit the catch-all cap")
-	assert.Contains(t, err.Error(), "limit of 2 reached")
+	require.ErrorContains(t,
+		ensureBackgroundWorker(nil, "cap-c", 5*time.Second),
+		"limit of 2 reached",
+		"third ensure must hit the catch-all cap")
 }
 
 // TestEnsureBackgroundWorkerUndeclared confirms ensure() on an undeclared
@@ -119,9 +118,9 @@ func TestEnsureBackgroundWorkerUndeclared(t *testing.T) {
 	))
 	t.Cleanup(Shutdown)
 
-	err := ensureBackgroundWorker(nil, "other-name", 5*time.Second)
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), "no background worker configured for name")
+	require.ErrorContains(t,
+		ensureBackgroundWorker(nil, "other-name", 5*time.Second),
+		"no background worker configured for name")
 }
 
 // TestEnsureBackgroundWorkerConcurrent confirms the doc claim that ensure()
@@ -189,15 +188,16 @@ func TestEnsureBackgroundWorkerTimeout(t *testing.T) {
 
 	start := time.Now()
 	err := ensureBackgroundWorker(nil, "bg-no-handle", 1*time.Second)
-	elapsed := time.Since(start)
+	deadline := start.Add(1 * time.Second)
 
-	require.Error(t, err, "ensure() must time out when the worker never calls frankenphp_get_worker_handle()")
-	assert.Contains(t, err.Error(), "did not call frankenphp_get_worker_handle()", "timeout error must point at the readiness boundary")
+	require.ErrorContains(t, err,
+		"did not call frankenphp_get_worker_handle()",
+		"ensure() must time out at the readiness boundary")
 	// Lower bound: timer must actually have run; allow a little slop for
-	// timer scheduling. Upper bound: hard cap so a regression that lets
-	// ensure() block forever doesn't hang the suite.
-	assert.GreaterOrEqual(t, elapsed, 900*time.Millisecond, "ensure() must wait the full timeout")
-	assert.Less(t, elapsed, 5*time.Second, "ensure() must return within a small slack window after the timeout")
+	// timer scheduling.
+	assert.GreaterOrEqual(t, time.Since(start), 900*time.Millisecond, "ensure() must wait the full timeout")
+	// Upper bound: ensure() returned close to the deadline (didn't hang).
+	assert.WithinDuration(t, deadline, time.Now(), 4*time.Second, "ensure() must return within a small slack window after the timeout")
 }
 
 // TestEnsureBackgroundWorkerBootFailure declares a worker whose entrypoint
@@ -221,9 +221,10 @@ func TestEnsureBackgroundWorkerBootFailure(t *testing.T) {
 	err := ensureBackgroundWorker(nil, "bg-boot-fail", 5*time.Second)
 	require.Error(t, err, "ensure() must surface the boot failure")
 	msg := err.Error()
-	// One of two paths: either the abort fired (cap reached) and the
-	// error mentions max_consecutive_failures, or the timeout fired with
-	// the bootFailureInfo attached so the message mentions exit status.
-	matches := strings.Contains(msg, "exit status") || strings.Contains(msg, "max_consecutive_failures") || strings.Contains(msg, "failed to start")
-	assert.True(t, matches, "ensure() error must reflect the boot crash, got: %s", msg)
+	// One of two paths: either the abort fired (cap reached) and the error
+	// mentions max_consecutive_failures, or the timeout fired with the
+	// bootFailureInfo attached so the message mentions exit status.
+	assert.True(t,
+		strings.Contains(msg, "exit status") || strings.Contains(msg, "max_consecutive_failures") || strings.Contains(msg, "failed to start"),
+		"ensure() error must reflect the boot crash, got: %s", msg)
 }

background_worker_ensure_test.go bgworkerensure_test.gobackground_worker_ensure_test.go renamed to bgworkerensure_test.go

@@ -46,16 +46,12 @@ func TestBackgroundWorkerRestartForceKillsStuckThread(t *testing.T) {
 	// when the drain fires - that's the only way to prove the
 	// force-kill code path was exercised, not the stop-pipe EOF path
 	// (the fixture doesn't open the stop pipe at all).
-	require.Eventually(t, func() bool {
-		_, err := os.Stat(sentinel)
-		return err == nil
-	}, 5*time.Second, 25*time.Millisecond, "bg worker never entered sleep()")
+	requireSentinelEventually(t, sentinel, "bg worker never entered sleep()")
 
 	start := time.Now()
 	RestartWorkers()
-	elapsed := time.Since(start)
-
-	// Test grace period (2s) + slack for signal dispatch and drain completion.
-	const budget = 5 * time.Second
-	assert.Less(t, elapsed, budget, "drain must force-kill the stuck bg worker within the grace period")
+	// Drain budget = grace period (2s) + slack for signal dispatch and
+	// drain completion.
+	assert.WithinDuration(t, start, time.Now(), 5*time.Second,
+		"drain must force-kill the stuck bg worker within the grace period")
 }

background_worker_internal_test.go bgworkerinternal_test.gobackground_worker_internal_test.go renamed to bgworkerinternal_test.go

@@ -11,11 +11,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TestNextBackgroundWorkerScopeIsDistinct verifies the scope counter
+// TestNextScopeIsDistinct verifies the scope counter
 // hands out unique values on consecutive calls.
-func TestNextBackgroundWorkerScopeIsDistinct(t *testing.T) {
-	a := NextBackgroundWorkerScope()
-	b := NextBackgroundWorkerScope()
+func TestNextScopeIsDistinct(t *testing.T) {
+	a := NextScope()
+	b := NextScope()
 	assert.NotEqual(t, a, b, "consecutive scopes must differ")
 	assert.NotZero(t, a, "scopes must be non-zero (zero is the global scope)")
 	assert.NotZero(t, b, "scopes must be non-zero (zero is the global scope)")
@@ -29,20 +29,20 @@ func TestBackgroundWorkerSameNameDifferentScope(t *testing.T) {
 	cwd, _ := os.Getwd()
 	testDataDir := cwd + "/testdata/"
 
-	scopeA := NextBackgroundWorkerScope()
-	scopeB := NextBackgroundWorkerScope()
+	scopeA := NextScope()
+	scopeB := NextScope()
 
 	tmp := t.TempDir()
 
 	require.NoError(t, Init(
 		WithWorkers("shared", testDataDir+"background-worker-named.php", 1,
 			WithWorkerBackground(),
-			WithWorkerBackgroundScope(scopeA),
+			WithWorkerScope(scopeA),
 			WithWorkerEnv(map[string]string{"BG_SENTINEL_DIR": tmp + "/a"}),
 		),
 		WithWorkers("shared", testDataDir+"background-worker-named.php", 1,
 			WithWorkerBackground(),
-			WithWorkerBackgroundScope(scopeB),
+			WithWorkerScope(scopeB),
 			WithWorkerEnv(map[string]string{"BG_SENTINEL_DIR": tmp + "/b"}),
 		),
 		WithNumThreads(4),
@@ -68,8 +68,8 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
 	cwd, _ := os.Getwd()
 	testDataDir := cwd + "/testdata/"
 
-	scopeA := NextBackgroundWorkerScope()
-	scopeB := NextBackgroundWorkerScope()
+	scopeA := NextScope()
+	scopeB := NextScope()
 
 	tmp := t.TempDir()
 	dirA := filepath.Join(tmp, "a")
@@ -80,12 +80,12 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
 	require.NoError(t, Init(
 		WithWorkers("", testDataDir+"background-worker-named.php", 0,
 			WithWorkerBackground(),
-			WithWorkerBackgroundScope(scopeA),
+			WithWorkerScope(scopeA),
 			WithWorkerEnv(map[string]string{"BG_SENTINEL_DIR": dirA}),
 		),
 		WithWorkers("", testDataDir+"background-worker-named.php", 0,
 			WithWorkerBackground(),
-			WithWorkerBackgroundScope(scopeB),
+			WithWorkerScope(scopeB),
 			WithWorkerEnv(map[string]string{"BG_SENTINEL_DIR": dirB}),
 		),
 		WithNumThreads(4),
@@ -121,8 +121,7 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
 
 	// Wait for the worker to write its sentinel so we know the right
 	// fixture ran (sanity check that env was inherited from scope A).
-	assert.Eventually(t, sentinelExists(filepath.Join(dirA, "job-a")),
-		3*time.Second, 10*time.Millisecond,
+	requireSentinelEventually(t, filepath.Join(dirA, "job-a"),
 		"scope A catch-all instance must write its sentinel under scope A's BG_SENTINEL_DIR")
 }
 

background_worker_pool_test.go bgworkerpool_test.gobackground_worker_pool_test.go renamed to bgworkerpool_test.go

@@ -51,7 +51,7 @@ type FrankenPHPModule struct {
 	preparedEnvNeedsReplacement bool
 	logger                      *slog.Logger
 	requestOptions              []frankenphp.RequestOption
-	backgroundScope             frankenphp.BackgroundScope
+	backgroundScope             frankenphp.Scope
 }
 
 // CaddyModule returns the Caddy module information.
@@ -84,7 +84,7 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 	// collide. Provision can be called more than once for the same
 	// module; only assign once.
 	if f.backgroundScope == 0 {
-		f.backgroundScope = frankenphp.NextBackgroundWorkerScope()
+		f.backgroundScope = frankenphp.NextScope()
 	}
 
 	loggerOpt := frankenphp.WithRequestLogger(f.logger)
@@ -101,7 +101,7 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 		}
 
 		wc.requestOptions = append(wc.requestOptions, loggerOpt)
-		wc.options = append(wc.options, frankenphp.WithWorkerBackgroundScope(f.backgroundScope))
+		wc.options = append(wc.options, frankenphp.WithWorkerScope(f.backgroundScope))
 		f.Workers[i] = wc
 	}
 
@@ -251,7 +251,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.WithRequestBackgroundScope(f.backgroundScope),
+			frankenphp.WithRequestScope(f.backgroundScope),
 		)...,
 	)