diff --git a/cmd/gc/build_desired_state.go b/cmd/gc/build_desired_state.go index fe46ec017c..5b906b029a 100644 --- a/cmd/gc/build_desired_state.go +++ b/cmd/gc/build_desired_state.go @@ -357,7 +357,7 @@ func buildDesiredStateWithSessionBeads( } poolWorkBeads := filterAssignedWorkBeadsForPoolDemand(cfg, cityPath, sessionBeads.Open(), assignedWorkBeads, assignedWorkStoreRefs) bp.assignedWorkBeads = poolWorkBeads - poolDesiredStates := ComputePoolDesiredStatesTraced(cfg, poolWorkBeads, sessionBeads.Open(), scaleCheckCounts, trace) + poolDesiredStates := ComputePoolDesiredStatesDebouncedTraced(cfg, poolWorkBeads, sessionBeads.Open(), scaleCheckCounts, poolScaleCheckPartialTemplates, trace) for _, poolState := range poolDesiredStates { cfgAgent := findAgentByTemplate(cfg, poolState.Template) if cfgAgent == nil { diff --git a/cmd/gc/city_runtime.go b/cmd/gc/city_runtime.go index 34adaf0d8e..616fceece9 100644 --- a/cmd/gc/city_runtime.go +++ b/cmd/gc/city_runtime.go @@ -1601,8 +1601,8 @@ func (cr *CityRuntime) beadReconcileTick(ctx context.Context, result DesiredStat if poolDesired == nil { poolWorkBeads := filterAssignedWorkBeadsForPoolDemand(cr.cfg, cr.cityPath, sessionBeads.Open(), assignedWorkBeads, assignedWorkStoreRefs) poolDesired = retainScaleCheckPartialPoolDesired( - PoolDesiredCounts(ComputePoolDesiredStatesTraced( - cr.cfg, poolWorkBeads, sessionBeads.Open(), result.ScaleCheckCounts, trace)), + PoolDesiredCounts(ComputePoolDesiredStatesDebouncedTraced( + cr.cfg, poolWorkBeads, sessionBeads.Open(), result.ScaleCheckCounts, result.PoolScaleCheckPartialTemplates, trace)), sessionBeads, result.PoolScaleCheckPartialTemplates, ) @@ -2133,8 +2133,8 @@ func (cr *CityRuntime) controlDispatcherTick(ctx context.Context) { open := filterSessionBeadsByName(updated, cfgNames) poolWorkBeads := filterAssignedWorkBeadsForPoolDemand(filteredCfg, cr.cityPath, open, wfcResult.AssignedWorkBeads, wfcResult.AssignedWorkStoreRefs) poolDesired := retainScaleCheckPartialPoolDesired( - PoolDesiredCounts(ComputePoolDesiredStates( - filteredCfg, poolWorkBeads, open, wfcResult.ScaleCheckCounts)), + PoolDesiredCounts(ComputePoolDesiredStatesDebounced( + filteredCfg, poolWorkBeads, open, wfcResult.ScaleCheckCounts, wfcResult.PoolScaleCheckPartialTemplates)), newSessionBeadSnapshot(open), wfcResult.PoolScaleCheckPartialTemplates, ) @@ -2298,8 +2298,8 @@ func (cr *CityRuntime) loadDemandSnapshot( } poolWorkBeads := filterAssignedWorkBeadsForPoolDemand(cr.cfg, cr.cityPath, openSessionBeads, result.AssignedWorkBeads, result.AssignedWorkStoreRefs) result.PoolDesiredCounts = retainScaleCheckPartialPoolDesired( - PoolDesiredCounts(ComputePoolDesiredStatesTraced( - cr.cfg, poolWorkBeads, openSessionBeads, result.ScaleCheckCounts, trace)), + PoolDesiredCounts(ComputePoolDesiredStatesDebouncedTraced( + cr.cfg, poolWorkBeads, openSessionBeads, result.ScaleCheckCounts, result.PoolScaleCheckPartialTemplates, trace)), sessionBeads, result.PoolScaleCheckPartialTemplates, ) diff --git a/cmd/gc/cmd_start.go b/cmd/gc/cmd_start.go index b5e3041553..5ad4974798 100644 --- a/cmd/gc/cmd_start.go +++ b/cmd/gc/cmd_start.go @@ -879,8 +879,8 @@ func doStartStandalone(args []string, controllerMode bool, stdout, stderr io.Wri dt := newDrainTracker() poolWorkBeads := filterAssignedWorkBeadsForPoolDemand(cfg, cityPath, open, dsResult.AssignedWorkBeads, dsResult.AssignedWorkStoreRefs) poolDesired := retainScaleCheckPartialPoolDesired( - PoolDesiredCounts(ComputePoolDesiredStates( - cfg, poolWorkBeads, open, dsResult.ScaleCheckCounts)), + PoolDesiredCounts(ComputePoolDesiredStatesDebounced( + cfg, poolWorkBeads, open, dsResult.ScaleCheckCounts, dsResult.PoolScaleCheckPartialTemplates)), sessionBeads, dsResult.PoolScaleCheckPartialTemplates, ) diff --git a/cmd/gc/main_test.go b/cmd/gc/main_test.go index 29655e3be1..9217fe7d33 100644 --- a/cmd/gc/main_test.go +++ b/cmd/gc/main_test.go @@ -216,6 +216,11 @@ func TestMain(m *testing.M) { } configureFSPressureForTests() configureSupervisorHooksForTests() + // Disable the pool scale_check confirm window for tests so existing + // single-call assertions about spawn behavior continue to hold without + // having to seed the per-template history. Debouncer-specific tests opt + // back in via setPoolScaleCheckDebouncerWindowForTesting. + setPoolScaleCheckDebouncerWindowForTesting(1) testscript.Main(newDoltLeakGuardedTestingM(m, testTempRoot, testTempRoot, gcHome, runtimeDir, providerStubDir, sharedTestFormulaDir, sharedTestCityDir), map[string]func(){ "gc": func() { configureTestscriptEnvDefaults() diff --git a/cmd/gc/pool_desired_state.go b/cmd/gc/pool_desired_state.go index d41a66d9d1..80da674f11 100644 --- a/cmd/gc/pool_desired_state.go +++ b/cmd/gc/pool_desired_state.go @@ -75,6 +75,38 @@ func ComputePoolDesiredStatesTraced( return computePoolDesiredStates(cfg, assignedWorkBeads, sessionBeads, scaleCheckCounts, trace) } +// ComputePoolDesiredStatesDebounced wraps ComputePoolDesiredStates with the +// process-scoped scale_check confirm window. Reconciler call sites should +// prefer this over the raw entry point so a single-tick flap on +// scaleCheckCounts (e.g. a transient assignee release from an upstream race) +// can't trigger a fresh pool spawn that has nothing to do. partialTemplates +// is the set of templates whose probe failed this tick — those pass through +// the debouncer unchanged so transient probe errors don't poison the window. +func ComputePoolDesiredStatesDebounced( + cfg *config.City, + assignedWorkBeads []beads.Bead, + sessionBeads []beads.Bead, + scaleCheckCounts map[string]int, + partialTemplates map[string]bool, +) []PoolDesiredState { + debounced := debouncePoolScaleCheckCounts(scaleCheckCounts, partialTemplates) + return computePoolDesiredStates(cfg, assignedWorkBeads, sessionBeads, debounced, nil) +} + +// ComputePoolDesiredStatesDebouncedTraced is the traced variant of +// ComputePoolDesiredStatesDebounced. See that function for the semantics. +func ComputePoolDesiredStatesDebouncedTraced( + cfg *config.City, + assignedWorkBeads []beads.Bead, + sessionBeads []beads.Bead, + scaleCheckCounts map[string]int, + partialTemplates map[string]bool, + trace *sessionReconcilerTraceCycle, +) []PoolDesiredState { + debounced := debouncePoolScaleCheckCounts(scaleCheckCounts, partialTemplates) + return computePoolDesiredStates(cfg, assignedWorkBeads, sessionBeads, debounced, trace) +} + func computePoolDesiredStates( cfg *config.City, assignedWorkBeads []beads.Bead, diff --git a/cmd/gc/pool_scale_check_debouncer.go b/cmd/gc/pool_scale_check_debouncer.go new file mode 100644 index 0000000000..cede45ca4a --- /dev/null +++ b/cmd/gc/pool_scale_check_debouncer.go @@ -0,0 +1,128 @@ +package main + +import "sync" + +// defaultPoolScaleCheckConfirmWindow is the number of consecutive scale_check +// samples a pool template's count must clear before the reconciler acts on a +// non-zero value as new demand. The pathology this prevents: a single tick +// where the scale_check shell returns 1 (because the bead briefly appeared +// unassigned via some upstream race) is enough to spawn a fresh pool session, +// which then runs work_query, finds nothing actually claimable, and drains — +// repeating every ~tick interval. Confirming over a window collapses that +// loop without changing steady-state demand semantics. +// +// 3 is the smallest window that suppresses the dominant single-tick flap +// while still tolerating a single dropped sample inside an otherwise +// sustained run. The window applies per template, so a slow reconciler tick +// cadence (multi-second) yields a confirm latency on the order of N ticks +// before legitimate new demand is acted on — acceptable for pool spawn +// because in-flight session beads from a prior tick continue to count as +// "spent" new demand via poolInFlightNewRequests during the wait. +const defaultPoolScaleCheckConfirmWindow = 3 + +// poolScaleCheckDebouncer holds the per-template history of recent +// scale_check samples used to confirm new pool demand. Methods are safe +// for concurrent use. +type poolScaleCheckDebouncer struct { + mu sync.Mutex + windowN int + history map[string][]int +} + +// defaultPoolScaleCheckDebouncerSingleton is the process-scoped debouncer +// used by the reconciler. One gascity process supervises one city, so a +// package-level singleton keyed only by agent template name is sufficient +// to isolate history within the process. +// +// Tests should reset this via resetPoolScaleCheckDebouncerForTesting (and +// optionally setPoolScaleCheckDebouncerWindowForTesting to shrink the +// window) to avoid cross-test bleed from accumulated history. +var defaultPoolScaleCheckDebouncerSingleton = newPoolScaleCheckDebouncer(defaultPoolScaleCheckConfirmWindow) + +func newPoolScaleCheckDebouncer(windowN int) *poolScaleCheckDebouncer { + if windowN < 1 { + windowN = 1 + } + return &poolScaleCheckDebouncer{ + windowN: windowN, + history: make(map[string][]int), + } +} + +// debounce records this tick's raw scale_check counts and returns the +// confirmed counts the reconciler should act on. For each template: +// - If the template is in partialTemplates, the raw count passes through +// and history is left untouched: a probe failure isn't a real +// observation of demand, and admitting it would poison the window. +// - Otherwise the sample is appended to the per-template ring. While the +// ring has fewer than windowN samples, the raw count passes through — +// so a cold-start supervisor still acts on demand without first +// accumulating windowN ticks of history. Once the ring is full, the +// returned count is the minimum across the window: a single tick at 0 +// in an otherwise positive window forces 0, killing the 1-tick flap. +func (d *poolScaleCheckDebouncer) debounce(raw map[string]int, partialTemplates map[string]bool) map[string]int { + d.mu.Lock() + defer d.mu.Unlock() + + out := make(map[string]int, len(raw)) + for template, count := range raw { + if partialTemplates[template] { + out[template] = count + continue + } + h := d.history[template] + h = append(h, count) + if len(h) > d.windowN { + h = h[len(h)-d.windowN:] + } + d.history[template] = h + + if len(h) < d.windowN { + out[template] = count + continue + } + + minCount := h[0] + for _, v := range h[1:] { + if v < minCount { + minCount = v + } + } + out[template] = minCount + } + return out +} + +// debouncePoolScaleCheckCounts is the package-level entry point the +// reconciler uses to apply the singleton's confirm window to a fresh +// scale_check sample. +func debouncePoolScaleCheckCounts(raw map[string]int, partialTemplates map[string]bool) map[string]int { + return defaultPoolScaleCheckDebouncerSingleton.debounce(raw, partialTemplates) +} + +// resetPoolScaleCheckDebouncerForTesting clears the singleton's per-template +// history. Tests that exercise the reconciler across multiple buildDesiredState +// or ComputePoolDesiredStatesDebounced calls should reset to keep the window +// deterministic. +func resetPoolScaleCheckDebouncerForTesting() { + defaultPoolScaleCheckDebouncerSingleton.mu.Lock() + defer defaultPoolScaleCheckDebouncerSingleton.mu.Unlock() + defaultPoolScaleCheckDebouncerSingleton.history = make(map[string][]int) +} + +// setPoolScaleCheckDebouncerWindowForTesting overrides the singleton's +// confirm window. windowN<=1 makes the debouncer a no-op (it returns the +// current sample unchanged), which is the default for cmd/gc tests so +// existing assertions about single-call spawn behavior remain valid. +// Returns the previous window for restoration. +func setPoolScaleCheckDebouncerWindowForTesting(windowN int) int { + if windowN < 1 { + windowN = 1 + } + defaultPoolScaleCheckDebouncerSingleton.mu.Lock() + defer defaultPoolScaleCheckDebouncerSingleton.mu.Unlock() + prev := defaultPoolScaleCheckDebouncerSingleton.windowN + defaultPoolScaleCheckDebouncerSingleton.windowN = windowN + defaultPoolScaleCheckDebouncerSingleton.history = make(map[string][]int) + return prev +} diff --git a/cmd/gc/pool_scale_check_debouncer_test.go b/cmd/gc/pool_scale_check_debouncer_test.go new file mode 100644 index 0000000000..715f851668 --- /dev/null +++ b/cmd/gc/pool_scale_check_debouncer_test.go @@ -0,0 +1,156 @@ +package main + +import ( + "reflect" + "testing" +) + +// withPoolScaleCheckDebouncerWindow installs a fresh debouncer at the given +// window for one test and restores the prior window on cleanup. Use this +// inside debouncer-specific tests because main_test.go's TestMain pins the +// singleton at window=1 so unrelated tests don't get debouncing applied. +func withPoolScaleCheckDebouncerWindow(t *testing.T, windowN int) { + t.Helper() + prev := setPoolScaleCheckDebouncerWindowForTesting(windowN) + t.Cleanup(func() { + setPoolScaleCheckDebouncerWindowForTesting(prev) + }) +} + +func TestPoolScaleCheckDebouncer_ColdStartPassesThroughBelowWindow(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if want := map[string]int{"local-core.builder": 1}; !reflect.DeepEqual(got, want) { + t.Fatalf("first sample: got %v, want %v (history smaller than window must pass raw count)", got, want) + } + got = debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if want := map[string]int{"local-core.builder": 1}; !reflect.DeepEqual(got, want) { + t.Fatalf("second sample: got %v, want %v (still below window)", got, want) + } +} + +func TestPoolScaleCheckDebouncer_SteadyDemandConfirmsAfterWindow(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + for i := 0; i < 3; i++ { + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if got["local-core.builder"] != 1 { + t.Fatalf("tick %d: got %d, want 1 (every sample is 1, min over window must be 1)", i, got["local-core.builder"]) + } + } + // Tenth tick still 1 — window slides forward, but min stays 1. + for i := 0; i < 10; i++ { + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if got["local-core.builder"] != 1 { + t.Fatalf("sustained tick %d: got %d, want 1", i, got["local-core.builder"]) + } + } +} + +func TestPoolScaleCheckDebouncer_SuppressesSingleTickFlap(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + // Mirror the fm-0ubml4 pathology: scale_check flaps 1, 0, 1, 0, 1, 0... + // Once the window is full (after 3 samples), the minimum over any window + // containing at least one 0 must be 0, which kills the spawn loop. + samples := []int{1, 0, 1, 0, 1, 0} + results := make([]int, 0, len(samples)) + for _, s := range samples { + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": s}, nil) + results = append(results, got["local-core.builder"]) + } + // First two samples pass through (window not yet full). + // Samples 3+ are min over [1,0,1]=0, [0,1,0]=0, [1,0,1]=0, [0,1,0]=0. + want := []int{1, 0, 0, 0, 0, 0} + if !reflect.DeepEqual(results, want) { + t.Fatalf("flap sequence: got %v, want %v", results, want) + } +} + +func TestPoolScaleCheckDebouncer_PartialTemplatesPassThroughAndDoNotPoisonWindow(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + // A partial sample must not be recorded into the per-template history — + // otherwise a probe failure (count defaults to 0) would force min=0 for + // the next two ticks even after the probe recovers and reports steady 1. + for i := 0; i < 3; i++ { + got := debouncePoolScaleCheckCounts( + map[string]int{"local-core.builder": 0}, + map[string]bool{"local-core.builder": true}, + ) + if got["local-core.builder"] != 0 { + t.Fatalf("partial tick %d: got %d, want 0 (partial passes raw through)", i, got["local-core.builder"]) + } + } + // Three real samples of 1 should now confirm — partial ticks weren't + // recorded, so the window fills cleanly from real samples. + for i := 0; i < 3; i++ { + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if got["local-core.builder"] != 1 { + t.Fatalf("post-partial real tick %d: got %d, want 1", i, got["local-core.builder"]) + } + } +} + +func TestPoolScaleCheckDebouncer_WindowOfOneIsNoOp(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 1) + + // Window=1 is what TestMain installs so existing tests behave as before. + // Whatever the raw count is, it must be returned unchanged on every tick. + for _, s := range []int{1, 0, 5, 0, 1} { + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": s}, nil) + if got["local-core.builder"] != s { + t.Fatalf("window=1 sample %d: got %d, want %d (debouncer must be a no-op)", s, got["local-core.builder"], s) + } + } +} + +func TestPoolScaleCheckDebouncer_PerTemplateIsolation(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + // Builder flaps; planner is steady. Each template's history must be + // independent — the planner's confirmed count must not collapse to 0 + // just because the builder's window saw a 0 sample. + type tick struct { + builder int + planner int + } + ticks := []tick{ + {builder: 1, planner: 1}, + {builder: 0, planner: 1}, + {builder: 1, planner: 1}, + {builder: 0, planner: 1}, + } + for i, tk := range ticks { + got := debouncePoolScaleCheckCounts( + map[string]int{"local-core.builder": tk.builder, "local-core.planner": tk.planner}, + nil, + ) + if got["local-core.planner"] != 1 { + t.Fatalf("tick %d planner: got %d, want 1 (steady demand must not be debounced by sibling template's flap)", i, got["local-core.planner"]) + } + } +} + +func TestPoolScaleCheckDebouncer_ResetClearsHistory(t *testing.T) { + withPoolScaleCheckDebouncerWindow(t, 3) + + // Fill the window with 0s — without reset, the next real sample of 1 + // would be suppressed to 0 by the min over [0,0,1]. + for i := 0; i < 3; i++ { + debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 0}, nil) + } + got := debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if got["local-core.builder"] != 0 { + t.Fatalf("pre-reset: got %d, want 0 (history [0,0,1] → min=0 confirms suppression)", got["local-core.builder"]) + } + + resetPoolScaleCheckDebouncerForTesting() + + // After reset, the next sample is a cold start again — passes through. + got = debouncePoolScaleCheckCounts(map[string]int{"local-core.builder": 1}, nil) + if got["local-core.builder"] != 1 { + t.Fatalf("post-reset: got %d, want 1 (history is empty, raw count must pass through)", got["local-core.builder"]) + } +}