Skip to content

Commit 5804537

Browse files
committed
issue/presence-start-guard: guard PresenceManager.Start against duplicate sweep goroutines
Start() now uses sync.Once (mirroring the existing stopOnce pattern) to ensure only one sweep loop is ever spawned per PresenceManager instance. Adds two regression tests: - TestPresenceManager_Start_Idempotent: counts goroutines before/after multiple Start() calls to assert no duplicate sweep loops spawn. - TestPresenceManager_ExpireCallback_OncePerExpiration: counts callback invocations across multiple sweep cycles to assert the callback fires at most once per expired node lease. Verification: temporarily reverted the startOnce guard and confirmed TestPresenceManager_Start_Idempotent fails with delta=3 (3 extra goroutines spawned by duplicate Start() calls), proving the regression test catches the bug. Restored the guard; all presence manager tests pass and go vet is clean.
1 parent 054a7d1 commit 5804537

2 files changed

Lines changed: 102 additions & 5 deletions

File tree

control-plane/internal/services/presence_manager.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,11 @@ type PresenceManager struct {
2727
statusManager *StatusManager
2828
config PresenceManagerConfig
2929

30-
leases map[string]*presenceLease
31-
mu sync.RWMutex
32-
stopCh chan struct{}
33-
stopOnce sync.Once
30+
leases map[string]*presenceLease
31+
mu sync.RWMutex
32+
stopCh chan struct{}
33+
stopOnce sync.Once
34+
startOnce sync.Once
3435

3536
expireCallback func(string)
3637
}
@@ -58,7 +59,9 @@ func NewPresenceManager(statusManager *StatusManager, config PresenceManagerConf
5859
}
5960

6061
func (pm *PresenceManager) Start() {
61-
go pm.loop()
62+
pm.startOnce.Do(func() {
63+
go pm.loop()
64+
})
6265
}
6366

6467
func (pm *PresenceManager) Stop() {

control-plane/internal/services/presence_manager_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ package services
22

33
import (
44
"context"
5+
"runtime"
56
"sync"
7+
"sync/atomic"
68
"testing"
79
"time"
810

@@ -407,3 +409,95 @@ func TestPresenceManager_RecoverFromDatabase_SkipsNilNodes(t *testing.T) {
407409
// Verify the valid agent has a lease
408410
assert.True(t, pm.HasLease("valid-agent"))
409411
}
412+
413+
// TestPresenceManager_Start_Idempotent verifies that calling Start() multiple
414+
// times does not spawn additional sweep goroutines. Regression test for the
415+
// duplicate sweep goroutine guard.
416+
func TestPresenceManager_Start_Idempotent(t *testing.T) {
417+
pm, _ := setupPresenceManagerTest(t)
418+
419+
// Use long intervals so no sweeps fire during the test. This isolates the
420+
// goroutine count to the sweep loop itself (no callback goroutines).
421+
pm.config.HeartbeatTTL = 1 * time.Hour
422+
pm.config.SweepInterval = 1 * time.Hour
423+
pm.config.HardEvictTTL = 1 * time.Hour
424+
425+
// Allow the runtime goroutine count to settle before measuring.
426+
time.Sleep(50 * time.Millisecond)
427+
before := runtime.NumGoroutine()
428+
429+
pm.Start()
430+
431+
// The first Start() must spawn exactly one sweep goroutine.
432+
require.Eventually(t, func() bool {
433+
return runtime.NumGoroutine() > before
434+
}, 500*time.Millisecond, 5*time.Millisecond,
435+
"first Start() should spawn a sweep goroutine")
436+
437+
afterFirst := runtime.NumGoroutine()
438+
439+
// Calling Start() multiple times must NOT spawn additional goroutines.
440+
pm.Start()
441+
pm.Start()
442+
pm.Start()
443+
444+
// Give any (incorrectly) spawned goroutines time to start.
445+
time.Sleep(100 * time.Millisecond)
446+
447+
afterMultiple := runtime.NumGoroutine()
448+
449+
// Tolerate ±1 for runtime fluctuations (GC, finalizer goroutines, etc.).
450+
// With the bug present, afterMultiple would exceed afterFirst by ~3.
451+
delta := afterMultiple - afterFirst
452+
assert.LessOrEqual(t, delta, 1,
453+
"multiple Start() calls should not spawn additional sweep goroutines (delta=%d)", delta)
454+
}
455+
456+
// TestPresenceManager_ExpireCallback_OncePerExpiration verifies that the
457+
// expireCallback fires at most once per expired node lease per sweep cycle.
458+
// Even across multiple sweep cycles, a single expired lease (without a fresh
459+
// Touch in between) must only trigger the callback once.
460+
func TestPresenceManager_ExpireCallback_OncePerExpiration(t *testing.T) {
461+
pm, provider := setupPresenceManagerTest(t)
462+
463+
// Register the agent in storage so UpdateAgentStatus can look it up.
464+
// Without this, markInactive returns early before invoking the callback.
465+
ctx := context.Background()
466+
nodeID := "node-once-per-expire"
467+
require.NoError(t, provider.RegisterAgent(ctx, &types.AgentNode{
468+
ID: nodeID,
469+
BaseURL: "http://localhost:9999",
470+
LastHeartbeat: time.Now(),
471+
}))
472+
473+
var callbackCount int32
474+
pm.SetExpireCallback(func(id string) {
475+
atomic.AddInt32(&callbackCount, 1)
476+
})
477+
478+
// Short intervals for fast, deterministic sweeps across multiple cycles.
479+
pm.config.HeartbeatTTL = 500 * time.Millisecond
480+
pm.config.SweepInterval = 100 * time.Millisecond
481+
// Long hard-evict TTL so the lease stays around (we want to verify no
482+
// duplicate callbacks fire while the lease lingers in MarkedOffline state).
483+
pm.config.HardEvictTTL = 1 * time.Hour
484+
485+
pm.Start()
486+
487+
// Touch with an already-expired timestamp so the first sweep marks it
488+
// offline and invokes the callback.
489+
pm.Touch(nodeID, "", time.Now().Add(-10*time.Second))
490+
491+
// Wait for the callback to fire at least once.
492+
require.Eventually(t, func() bool {
493+
return atomic.LoadInt32(&callbackCount) >= 1
494+
}, 3*time.Second, 50*time.Millisecond,
495+
"expire callback should fire at least once")
496+
497+
// Wait through multiple additional sweep cycles to ensure no duplicate
498+
// callbacks fire for the same expired lease.
499+
time.Sleep(500 * time.Millisecond)
500+
501+
assert.Equal(t, int32(1), atomic.LoadInt32(&callbackCount),
502+
"expire callback should fire exactly once per expired node lease")
503+
}

0 commit comments

Comments
 (0)