Guard PresenceManager.Start against duplicate sweep goroutines

[AbirAbbas]

Summary

• Guards PresenceManager.Start with sync.Once, mirroring the existing stopOnce pattern so only one sweep loop is ever spawned per instance.
• Adds two regression tests covering goroutine-count idempotency and at-most-once expire-callback firing across sweep cycles.
• Closes [Control Plane] PresenceManager.Start can spawn duplicate sweep goroutines #431

Changes

• control-plane/internal/services/presence_manager.go — added startOnce sync.Once field and wrapped Start() body so duplicate calls are no-ops.
• control-plane/internal/services/presence_manager_test.go — added TestPresenceManager_Start_Idempotent (asserts no extra goroutines across repeated Start() calls) and TestPresenceManager_ExpireCallback_OncePerExpiration (asserts the expire callback fires at most once per expired lease across multiple sweep cycles).

Test plan

• go vet ./... is clean.
• All 19 TestPresenceManager* tests pass.
• Full control-plane/internal/services suite passes under -race with -gcflags=all=-d=checkptr=0 (pre-existing boltdb v1.3.1 checkptr incompatibility with Go 1.25 race detector exists on main and is unrelated to this change).
• Temporarily reverted the startOnce guard and confirmed TestPresenceManager_Start_Idempotent fails with delta=3 (3 extra goroutines from duplicate Start() calls), proving the regression test catches the bug; restored the guard.

🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField

[github-actions]

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface	Current	Baseline	Δ
control-plane	86.90%	87.40%	↓ -0.50 pp	🟡
sdk-go	92.40%	92.00%	↑ +0.40 pp	🟢
sdk-python	93.82%	93.73%	↑ +0.09 pp	🟢
sdk-typescript	91.05%	90.42%	↑ +0.63 pp	🟢
web-ui	84.75%	84.79%	↓ -0.04 pp	🟡
aggregate	85.54%	85.75%	↓ -0.21 pp	🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

[github-actions]

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface	Touched lines	Patch coverage	Status
control-plane	3	100.00%	✅
sdk-go	0	—	➖ no changes
sdk-python	0	—	➖ no changes
sdk-typescript	0	—	➖ no changes
web-ui	0	—	➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

[AbirAbbas]

⚠️ Superseded — disregard this review

This "Looks Good / 0 findings" verdict came from a run where the review harness crashed silently (misconfigured provider binary) — no review dimensions actually executed. See the follow-up PR-AF review below (submitted 14:48 UTC) for the real result: 7 findings (4 important, advisories only, non-blocking).

Automated multi-agent code review · PR-AF built with AgentField

[AbirAbbas]

🟢 PR-AF Review — Safe to Merge — Advisories Only

✅ Safe to merge. 7 advisory notes below — non-blocking, safe to address in a follow-up.

Automated multi-agent code review · PR-AF built with AgentField

7 findings · 🚫 0 blocking · 💬 7 advisory · 🔴 0 critical · 🟠 4 important · 🔵 2 suggestions · ⚪ 1 nitpicks

PR Overview

Summary

• Guards PresenceManager.Start with sync.Once, mirroring the existing stopOnce pattern so only one sweep loop is ever spawned per instance.
• Adds two regression tests covering goroutine-count idempotency and at-most-once expire-callback firing across sweep cycles.
• Closes #431

Changes

• control-plane/internal/services/presence_manager.go — added startOnce sync.Once field and wrapped Start() body so duplicate calls are no-ops.
• control-plane/internal/services/presence_manager_test.go — added TestPresenceManager_Start_Idempotent (asserts no extra goroutines across repeated Start() calls) and TestPresenceManager_ExpireCallback_OncePerExpiration (asserts the expire callback fires at most once per expired lease across multiple sweep cycles).

Test plan

• go vet ./... is clean.
• All 19 TestPresenceManager* tests pass.
• Full control-plane/internal/services suite passes under -race with -gcflags=all=-d=checkptr=0 (pre-existing boltdb v1.3.1 checkptr incompatibility with Go 1.25 race detector exists on main and is unrelated to this change).
• Temporarily reverted the startOnce guard and confirmed TestPresenceManager_Start_Idempotent fails with delta=3 (3 extra goroutines from duplicate Start() calls), proving the regression test catches the bug; restored the guard.

🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField

Key Findings

7 advisory finding(s) surfaced as non-blocking:

• 🟠 Presence worker starts after failed lease recovery (control-plane/internal/services/presence_manager.go:62)
• 🟠 Negative sweep intervals can panic the launched presence loop (control-plane/internal/services/presence_manager.go:63)
• 🟠 Presence-manager cleanup closes storage before the sweep loop is guaranteed to exit (control-plane/internal/services/presence_manager_test.go:485)
• 🟠 Global goroutine count is not isolated from GC worker creation (control-plane/internal/services/presence_manager_test.go:451)
• 🔵 The Eventually predicate counts its own goroutine, so startup is not actually verified (control-plane/internal/services/presence_manager_test.go:432)
• … and 2 more (see All Findings by Severity)

Files with findings: control-plane/internal/services/presence_manager.go, control-plane/internal/services/presence_manager_test.go

All Findings by Severity

🟠 Important (4)

• Presence worker starts after failed lease recovery control-plane/internal/services/presence_manager.go:62
• Negative sweep intervals can panic the launched presence loop control-plane/internal/services/presence_manager.go:63
• Presence-manager cleanup closes storage before the sweep loop is guaranteed to exit control-plane/internal/services/presence_manager_test.go:485
• Global goroutine count is not isolated from GC worker creation control-plane/internal/services/presence_manager_test.go:451

🔵 Suggestion (2)

• The Eventually predicate counts its own goroutine, so startup is not actually verified control-plane/internal/services/presence_manager_test.go:432
• Exercise Start contention, not only sequential idempotency control-plane/internal/services/presence_manager_test.go:429

⚪ Nitpick (1)

• Describe the callback invariant as an offline transition control-plane/internal/services/presence_manager_test.go:456

Review Process Details

Dimensions Analyzed (4):

• One-shot start lifecycle and startup ordering — 3 file(s)
• Concurrent Start synchronization and worker lifecycle — 4 file(s)
• Reliability of the goroutine-count regression test — 2 file(s)
• Deterministic concurrency regression coverage — 1 file(s)

Meta-Dimension Lenses (3):

• Semantic — 3 dimension(s), 91% coverage confidence
• Mechanical — 2 dimension(s), 93% coverage confidence
• Systemic — 2 dimension(s), 91% coverage confidence

Cross-Reference & Adversary Analysis:

• 3 finding(s) adversarially tested: 3 confirmed, 0 challenged

Pipeline Stats

Metric	Value
Duration	1202.8s
Agent invocations	26
Coverage iterations	1
Estimated cost	N/A (provider does not report cost)
Budget exhausted	No
PR type	bugfix
Complexity	low

Review ID: rev_8090a7c933a6

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Defensive programming issue lacking concrete demonstration of data loss, corruption, or irreversible state damage

Presence worker starts after failed lease recovery

_{🟠 IMPORTANT · confidence 99%}

Gate presenceManager.Start() on successful recovery to prevent the sync.Once from consuming the one allowed start with unrecovered leases.

AgentFieldServer.Start unconditionally invokes PresenceManager.Start even when RecoverFromDatabase returns an error. Because Start is guarded by sync.Once, the worker launches without persisted leases and can never be restarted after recovery is retried.

Evidence

Changed location, control-plane/internal/services/presence_manager.go:62-64: pm.startOnce.Do(func() { go pm.loop() }). Startup location, control-plane/internal/server/server.go:582-586: if err := s.presenceManager.RecoverFromDatabase(ctx, s.storage); err != nil { logger.Logger.Error().Err(err).Msg("Failed to recover presence leases from database") } followed unconditionally by go s.presenceManager.Start().

💡 Suggested Fix

Gate presenceManager.Start() on successful recovery (and return/retry recovery before starting it), so the one allowed start cannot occur with unrecovered persisted leases.

---

Consistency Verifier · confidence 99%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: gate error

Negative sweep intervals can panic the launched presence loop

_{🟠 IMPORTANT · confidence 98%}

Reject or fix negative SweepInterval values in NewPresenceManager before Start spawns pm.loop(). NewPresenceManager only substitutes a default when SweepInterval == 0, retaining negative durations. When Start launches pm.loop(), the call to time.NewTicker(pm.config.SweepInterval) at lines 156-165 panics because the constructor requires a positive duration.

Evidence

Changed location, control-plane/internal/services/presence_manager.go:61-64: func (pm *PresenceManager) Start() { pm.startOnce.Do(func() { go pm.loop() }) }.

Other end, control-plane/internal/services/presence_manager.go:39-58: if config.SweepInterval == 0 { config.SweepInterval = config.HeartbeatTTL / 3; if config.SweepInterval < time.Second { config.SweepInterval = time.Second } } and stopCh: make(chan struct{}),. This handles only a zero interval, not a negative one.

Other end, control-plane/internal/services/presence_manager.go:67-70 closes that same channel: close(pm.stopCh). loop at lines 156-165 uses ticker := time.NewTicker(pm.config.SweepInterval) and case <-pm.stopCh:.

💡 Suggested Fix

Validate SweepInterval <= 0 in NewPresenceManager and replace it with the positive default (or reject invalid configuration), so every Start-launched loop can safely construct its ticker.

---

Consistency Verifier · confidence 98%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: gate error

Presence-manager cleanup closes storage before the sweep loop is guaranteed to exit

_{🟠 IMPORTANT · confidence 98%}

Make PresenceManager.Stop wait for the worker loop to exit before returning, or ensure setupPresenceManagerTest cleanup waits for loop completion before closing the provider. Stop() closes stopCh without waiting for the worker to exit, so cleanup can invoke provider.Close(ctx) while the loop is still executing checkExpirations. This runs markInactive and calls statusManager.UpdateAgentStatus against the closed provider.

Evidence

Changed location: presence_manager_test.go:485 calls pm.Start(). Test setup at lines 37-40 is t.Cleanup(func() { presenceManager.Stop(); _ = provider.Close(ctx) }). Shutdown implementation at presence_manager.go:67-71 is func (pm *PresenceManager) Stop() { pm.stopOnce.Do(func() { close(pm.stopCh) }) }. The worker loop at lines 156-165 can run pm.checkExpirations() on a ticker tick, and checkExpirations calls pm.markInactive(nodeID) (lines 188-190); markInactive calls pm.statusManager.UpdateAgentStatus(...) (line 226). No wait/join occurs between closing stopCh and closing the provider.

💡 Suggested Fix

Make PresenceManager.Stop wait for loop to exit (for example, with a sync.WaitGroup or done channel), then keep cleanup as Stop() followed by provider.Close(ctx). Alternatively, explicitly wait for loop completion in the test cleanup before closing the provider.

---

Consistency Verifier · confidence 98%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Flaky test pattern in internal test file; does not affect production code, security, data integrity, or API contracts.

Global goroutine count is not isolated from GC worker creation

_{🟠 IMPORTANT · confidence 97%}

Instrument the manager's loop directly instead of sampling runtime.NumGoroutine(). The assertion at presence_manager_test.go:447-453 permits only a one-goroutine delta, but the Go runtime spawns GC background workers (one per P) whenever garbage collection starts. A GC cycle between samples can increase the count by more than one and cause spurious failures.

Evidence

Changed location (control-plane/internal/services/presence_manager_test.go:447-453): afterMultiple := runtime.NumGoroutine() followed by delta := afterMultiple - afterFirst and assert.LessOrEqual(t, delta, 1, ...). Harness (control-plane/internal/services/presence_manager_test.go:37-40): cleanup only registers presenceManager.Stop() and provider.Close(ctx) for test completion. Go runtime (C:\Program Files\Go\src\runtime\mgc.go:1672-1695): func gcBgMarkStartWorkers() ensures a background GC G for each P and loops for gcBgMarkWorkerCount < gomaxprocs { ... go gcBgMarkWorker(ready) ... }.

💡 Suggested Fix

Avoid process-wide goroutine counting: expose or instrument the manager’s loop creation/state and assert that repeated Start calls do not create another loop. If retaining this approach, force and settle GC before both samples, but that remains less reliable.

---

Consistency Verifier · confidence 97%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Flawed test logic is a test-quality issue that does not break the build, introduce a reachable vulnerability, or regress production behavior.

The Eventually predicate counts its own goroutine, so startup is not actually verified

_{🔵 SUGGESTION · confidence 99%}

Replace the goroutine-count assertions in TestPresenceManager_Start_Idempotent with a manager-owned synchronization signal. require.Eventually at line 432 runs its predicate in a new goroutine, so line 433's runtime.NumGoroutine() includes that helper goroutine and can exceed before even if pm.Start() at line 429 launches no worker. Consequently, the later delta <= 1 assertion at lines 451-452 passes despite Start being a no-op, making the test unable to verify idempotency or that any worker was created.

Evidence

Step 1: TestPresenceManager_Start_Idempotent records before := runtime.NumGoroutine() at line 427 and calls pm.Start() at line 429. Consider the reachable regression where Start is accidentally changed to a no-op.
Step 2: Line 432 invokes require.Eventually; the repository pins github.com/stretchr/testify v1.11.1, whose assert.Eventually implementation creates checkCond := func() { ch <- condition() } and executes go checkCond() before reading the predicate result.
Step 3: The predicate at line 433 executes inside that newly created checkCond goroutine, so its runtime.NumGoroutine() includes at least that goroutine, which did not exist when before was recorded.
Step 4: The predicate can return true even with no pm.loop goroutine. After Eventually returns, the assertion helper goroutine has exited; with all subsequent Start calls also no-ops, lines 451-452 accept delta == 0. Thus the test passes although Start failed to start the worker it claims to verify.

💡 Suggested Fix

Replace goroutine-count observation with a manager-owned synchronization signal: have the loop signal after it has initialized its ticker (and expose that only through an internal test seam), then assert exactly one signal after a concurrent wave of Start callers. Pair it with a loop-done signal so cleanup can wait for Stop to finish. This directly attributes startup to this manager and exercises the sync.Once path without relying on scheduler/global-runtime state.

---

Test/runtime mechanics · confidence 99%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: This is a test-coverage gap for concurrent behavior, not a demonstrated production bug, security vulnerability, or regression.

Exercise Start contention, not only sequential idempotency

_{🔵 SUGGESTION · confidence 97%}

Replace the sequential pm.Start() calls at lines 429, 440, 441, and 442 with concurrent callers released from a WaitGroup barrier, and assert exactly one manager-scoped loop entry. The current test invokes Start serially, so a plain if !started { started = true; go pm.loop() } guard would pass while still allowing concurrent callers to observe !started and launch duplicate pm.loop() goroutines. That leaves the one-worker-per-manager contract untested under real contention and gives false confidence the guard is safe.

Evidence

Step 1: PresenceManager.Start is callable by multiple goroutines and is intended to ensure one go pm.loop() launch. Step 2: this test invokes pm.Start() at lines 429, 440, 441, and 442 sequentially, with no WaitGroup or synchronized release. Step 3: a non-synchronized check-then-set guard would observe the started state on every later sequential call and therefore leave afterMultiple - afterFirst within the asserted bound. Step 4: when real callers invoke that same guard concurrently, they can all observe the pre-start state and each launch pm.loop(), violating the one-worker-per-manager contract; this test would not prevent that regression.

💡 Suggested Fix

Replace the sequential follow-up calls with concurrently released Start callers (using WaitGroup synchronization, as in TestPresenceManager_ConcurrentAccess) and observe a test-local, manager-scoped loop-entry signal/count. Keep that hook unexported and narrowly scoped to testability rather than introducing public production lifecycle indirection.

---

Concurrency test architecture and isolation · confidence 97%

_{🤖 Reviewed by AgentField PR-AF}

[AbirAbbas]

Note

Advisory — non-blocking. Safe to merge and address in a follow-up.

Why non-blocking: Inaccurate test description is a documentation issue, not a functional defect.

Describe the callback invariant as an offline transition

_{⚪ NITPICK · confidence 98%}

Clarify that the callback fires once per offline transition, not once per sweep cycle. The current description implies markInactive runs on every tick, yet MarkedOffline prevents subsequent appends until Touch resets the flag at line 81. This mischaracterization suggests duplicate-loop coverage that the test does not exercise.

Evidence

Step 1: the test starts the manager only once at line 485, so it cannot exercise duplicate Start handling. Step 2: checkExpirations appends a node only when !lease.MarkedOffline (presence_manager.go lines 175-180), and calls markInactive only for appended nodes (lines 188-190). Step 3: subsequent sweep cycles do not append an already marked-offline lease, whereas Touch resets MarkedOffline to false (line 81). Step 4: the actual invariant is one callback for an offline transition, including across later ticks until a fresh touch; “per sweep cycle” conflicts with that contract.

💡 Suggested Fix

Change the description to say the callback fires once per transition to MarkedOffline, including across later sweep cycles until a fresh Touch creates a new expiration transition.

---

Concurrency test architecture and isolation · confidence 98%

_{🤖 Reviewed by AgentField PR-AF}