From d86967379bd21ac5faa8dd85a554a981b68a6e39 Mon Sep 17 00:00:00 2001 From: Vaughn Friesen Date: Fri, 29 May 2026 19:55:33 +0800 Subject: [PATCH 1/2] docs(batch): clarify config zero-value and clamp semantics Correct and complete the ConfigValues / Config godoc to match the engine's actual behavior (batch.go fixConfig + waitForItems): - Config.Get: the min/max clamp applies only when the corresponding max is greater than zero (non-zero); a zero max is unset, so no clamping occurs. - MaxTime == 0 and MaxItems == 0 mean unset/disabled (no max-time flush, no max-count trigger) -- documented explicitly since zero is counter-intuitive. - MinTime == 0 means no minimum-time wait; MinItems == 0 is treated as 1 by the engine (at least one item per batch). Documentation only; no behavioral change. Co-Authored-By: Claude Opus 4.8 (1M context) --- batch/config.go | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/batch/config.go b/batch/config.go index 5fd8c62..15c042b 100644 --- a/batch/config.go +++ b/batch/config.go @@ -16,8 +16,13 @@ import ( type Config interface { // Get returns the values for configuration. // - // If MinItems > MaxItems or MinTime > MaxTime, the min value will be - // set to the maximum value. + // The batch engine clamps conflicting min/max values, but only when the + // corresponding maximum is greater than zero (non-zero). Specifically: + // if MaxItems > 0 and MinItems > MaxItems, MinItems is reduced to + // MaxItems; if MaxTime > 0 and MinTime > MaxTime, MinTime is reduced to + // MaxTime. When the maximum is zero it is treated as unset (see + // ConfigValues.MaxItems and ConfigValues.MaxTime), so no clamping occurs + // and the min value is used as-is. // // If the config values may be modified during batch processing, Get // must properly handle concurrency issues. @@ -34,6 +39,10 @@ type ConfigValues struct { // of items was specified and that number is reached before MinTime; // in that case those items will be processed right away. // + // A value of 0 means there is no minimum-time wait: a batch becomes + // eligible as soon as MinItems is satisfied, without waiting for any + // time to elapse. + // // This parameter is useful to prevent processing very small batches // too frequently when items arrive at a slow but steady rate. MinTime time.Duration `json:"minTime"` @@ -45,6 +54,10 @@ type ConfigValues struct { // items is available, or if all items have been read and are ready // to process. // + // A value of 0 is treated by the engine as 1: at least one item is + // processed per batch. MinItems is therefore never effectively zero at + // runtime. + // // This parameter helps optimize processing by ensuring batches are // large enough to amortize the overhead of processing across multiple items. MinItems uint64 `json:"minItems"` @@ -53,6 +66,11 @@ type ConfigValues struct { // processing. Once that time has been reached, items will be processed // whether or not MinItems items are available. // + // A value of 0 means unset / disabled: there is NO maximum-time flush + // and the underlying timer is not started. This is counter-intuitive + // (zero may read like "flush immediately"), but a zero MaxTime imposes + // no upper bound on how long a batch may wait for MinItems. + // // This parameter ensures that items don't wait in the queue for too long, // which is important for latency-sensitive applications. MaxTime time.Duration `json:"maxTime"` @@ -61,6 +79,12 @@ type ConfigValues struct { // before processing. Once that number of items is available, they will // be processed whether or not MinTime has been reached. // + // A value of 0 means unset / disabled: there is NO maximum-count + // trigger. This is counter-intuitive (zero may read like "flush on the + // first item"), but a zero MaxItems imposes no upper bound on batch + // size; batches are then bounded only by MinItems, MaxTime, or the + // source being exhausted. + // // This parameter prevents the system from accumulating too many items // in a single batch, which could lead to memory pressure or processing // spikes. From 1705ef2987e97171e0518d01e98e5e242bd70453 Mon Sep 17 00:00:00 2001 From: Vaughn Friesen Date: Fri, 29 May 2026 19:55:41 +0800 Subject: [PATCH 2/2] test(batch): lock config value/concurrency contract Add contract-locking tests at the config-type level that assert existing behavior, so future regressions against the documented contract are caught: - ConstantConfig/DynamicConfig Get faithfully round-trips zero values (zero carries documented unset/disabled meaning; the config holder must not rewrite it -- that is the engine's job). - Update/UpdateBatchSize/UpdateTiming accept zero and extreme (max-uint64, max-duration, min>max) values without panicking or rejecting them (backwards-compat guard: no validation at the config layer). - DynamicConfig concurrent Get/Update/UpdateBatchSize/UpdateTiming under -race to lock thread-safety across every exported mutator. These pass immediately by design; no behavior changed. Tests stay at the config-type level and assert no batch-flush/engine behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- batch/config_test.go | 152 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/batch/config_test.go b/batch/config_test.go index 2c7377d..4513c21 100644 --- a/batch/config_test.go +++ b/batch/config_test.go @@ -1,6 +1,7 @@ package batch import ( + "math" "sync" "testing" "time" @@ -117,3 +118,154 @@ func TestDynamicConfig_ConcurrentAccess(t *testing.T) { } wg.Wait() } + +// --- Contract-locking tests --- +// +// The tests below assert behavior that already exists; they lock the +// documented contract (config.go) so future changes that would silently +// break it are caught. They are expected to pass immediately because no +// behavioral code changed alongside them. + +// TestConstantConfig_Get_RoundTripZeroValues locks the contract that +// ConstantConfig.Get faithfully returns explicitly-supplied zero values. +// Zero values carry documented "unset/disabled" meaning (see ConfigValues +// godoc); the config type itself must not silently rewrite them — that +// interpretation belongs to the engine, not the config holder. +func TestConstantConfig_Get_RoundTripZeroValues(t *testing.T) { + zero := ConfigValues{ + MinItems: 0, + MaxItems: 0, + MinTime: 0, + MaxTime: 0, + } + + got := NewConstantConfig(&zero).Get() + + if got != zero { + t.Errorf("ConstantConfig.Get altered zero values: expected %+v, got %+v", zero, got) + } +} + +// TestDynamicConfig_Get_RoundTripZeroValues locks the same zero-value +// round-trip contract for DynamicConfig.Get. +func TestDynamicConfig_Get_RoundTripZeroValues(t *testing.T) { + zero := ConfigValues{ + MinItems: 0, + MaxItems: 0, + MinTime: 0, + MaxTime: 0, + } + + got := NewDynamicConfig(&zero).Get() + + if got != zero { + t.Errorf("DynamicConfig.Get altered zero values: expected %+v, got %+v", zero, got) + } +} + +// TestDynamicConfig_Mutators_AcceptZeroAndLargeValues is a backwards-compat +// guard: Update, UpdateBatchSize, and UpdateTiming must accept zero values +// and extreme (max-uint64 / very large duration) values without panicking or +// rejecting them, and Get must round-trip exactly what was set. This locks the +// hard constraint that config setters perform no validation that could break +// existing callers. +func TestDynamicConfig_Mutators_AcceptZeroAndLargeValues(t *testing.T) { + cases := []struct { + name string + values ConfigValues + }{ + { + name: "all zero", + values: ConfigValues{}, + }, + { + name: "max uint64 counts", + values: ConfigValues{ + MinItems: math.MaxUint64, + MaxItems: math.MaxUint64, + MinTime: 0, + MaxTime: 0, + }, + }, + { + name: "max duration times", + values: ConfigValues{ + MinItems: 0, + MaxItems: 0, + MinTime: time.Duration(math.MaxInt64), + MaxTime: time.Duration(math.MaxInt64), + }, + }, + { + name: "min greater than max (no clamping at config layer)", + values: ConfigValues{ + MinItems: 1000, + MaxItems: 1, + MinTime: 10 * time.Second, + MaxTime: 1 * time.Second, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Update: replaces all values at once. + cfgUpdate := NewDynamicConfig(nil) + cfgUpdate.Update(tc.values) + if got := cfgUpdate.Get(); got != tc.values { + t.Errorf("Update round-trip mismatch: expected %+v, got %+v", tc.values, got) + } + + // UpdateBatchSize + UpdateTiming: the split setters must reach the + // same state, with no clamping or rejection at the config layer. + cfgSplit := NewDynamicConfig(nil) + cfgSplit.UpdateBatchSize(tc.values.MinItems, tc.values.MaxItems) + cfgSplit.UpdateTiming(tc.values.MinTime, tc.values.MaxTime) + if got := cfgSplit.Get(); got != tc.values { + t.Errorf("UpdateBatchSize/UpdateTiming round-trip mismatch: expected %+v, got %+v", tc.values, got) + } + }) + } +} + +// TestDynamicConfig_ConcurrentReadWrite_AllMutators locks thread-safety of +// DynamicConfig across every exported mutator (not just UpdateBatchSize) under +// the race detector. Run with -race to exercise the contract. +func TestDynamicConfig_ConcurrentReadWrite_AllMutators(t *testing.T) { + cfg := NewDynamicConfig(&ConfigValues{ + MinItems: 1, + MaxItems: 10, + MinTime: 1 * time.Second, + MaxTime: 5 * time.Second, + }) + + const iterations = 200 + var wg sync.WaitGroup + + for i := 0; i < iterations; i++ { + wg.Add(4) + go func(i int) { + defer wg.Done() + cfg.UpdateBatchSize(uint64(i), uint64(i*10)) + }(i) + go func(i int) { + defer wg.Done() + cfg.UpdateTiming(time.Duration(i)*time.Millisecond, time.Duration(i*2)*time.Millisecond) + }(i) + go func(i int) { + defer wg.Done() + cfg.Update(ConfigValues{ + MinItems: uint64(i), + MaxItems: uint64(i + 1), + MinTime: time.Duration(i) * time.Microsecond, + MaxTime: time.Duration(i+1) * time.Microsecond, + }) + }(i) + go func() { + defer wg.Done() + _ = cfg.Get() + }() + } + + wg.Wait() +}