bugfix: Race condition on agent.Active field.#314
Conversation
`agent.Active` is a plain `bool` read and written from multiple goroutines without any mutex or atomic protection. A goroutine can read a stale value of `Active` while another goroutine is setting it to `false`. This leads to tasks being dispatched to terminated agents, or active agents being incorrectly skipped. Under the Go memory model, unsynchronized bool access is a data race.
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency bug in the Teamserver’s Agent lifecycle handling by eliminating unsynchronized access to the agent “active” state, preventing tasks/tunnels/terminal actions from being dispatched to terminated agents.
Changes:
- Replaced
Agent.Active boolwith anatomic.Booland addedIsActive()/SetActive()accessors. - Updated call sites to use
IsActive()checks andSetActive(false)on termination/restore. - Adjusted
axc2imports in touched files to use an explicitadaptiximport name.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AdaptixServer/core/server/utils.go | Changes Agent active state to atomic.Bool and adds accessors. |
| AdaptixServer/core/server/ts_tunnels.go | Uses agent.IsActive() to prevent starting tunnels for inactive agents. |
| AdaptixServer/core/server/ts_terminal.go | Uses agent.IsActive() to prevent terminal channel creation for inactive agents. |
| AdaptixServer/core/server/ts_agent.go | Initializes active state via SetActive(true) and terminates via SetActive(false); replaces active checks. |
| AdaptixServer/core/server/server.go | Restores agent active state via SetActive(true/false) based on persisted mark. |
| AdaptixServer/core/server/mgr_task.go | Prevents task creation for inactive agents via IsActive(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mu sync.RWMutex | ||
| data adaptix.AgentData | ||
| Extender adaptix.ExtenderAgent | ||
| Tick bool | ||
| Active bool | ||
| active atomic.Bool |
There was a problem hiding this comment.
Agent.Tick remains a plain bool but is read/written from multiple goroutines (e.g., set in TsAgentProcessData and polled/reset in the background TsAgentTickUpdate loop started from server.go). This is still a data race under the Go memory model. Consider protecting Tick with the same synchronization approach as active (e.g., atomic.Bool with IsTicked/SetTicked, or guard it with Agent.mu).
`agent.Tick` is a plain `bool` read and written from multiple goroutines without any mutex or atomic protection. `TsAgentProcessData` writes `Tick = true` from listener/handler goroutines while `TsAgentTickUpdate` concurrently reads and resets it in a background loop (800ms interval).
agent.Activeis a plainboolread and written from multiple goroutines without any mutex or atomic protection.A goroutine can read a stale value of
Activewhile another goroutine is setting it tofalse. This leads to tasks being dispatched to terminated agents, or active agents being incorrectly skipped. Under the Go memory model, unsynchronized bool access is a data race.Protect
Activewithsync.Mutexor useatomic.Bool. All reads and writes must go through the same synchronization.