fix(M25): atomic.Pointer for Server.integrationMgr (constructor-goroutine race)
NewServer launches the SyncFileBrowserMounts goroutine (reads integrationMgr) from the constructor, BEFORE main.go's SetIntegrationManager write — so the init-only happens-before that covers the other Set* fields does NOT hold here, making it a genuine data race (handlers.go:358/360/1433 reads vs server.go:162 write). Converted the field to atomic.Pointer[integrations.Manager]; setter Stores, all 3 readers Load(). Regression test reproduces the concurrent access (clean under -race; flags on the pre-fix plain-pointer field). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -355,9 +355,9 @@ func (s *Server) deployHandler(w http.ResponseWriter, r *http.Request, name stri
|
||||
// renders those sections.
|
||||
if alreadyDeployed {
|
||||
// App-to-app integrations
|
||||
if meta.HasIntegrations() && s.integrationMgr != nil {
|
||||
if im := s.integrationMgr.Load(); meta.HasIntegrations() && im != nil {
|
||||
data["HasIntegrations"] = true
|
||||
data["Integrations"] = s.integrationMgr.ListForProvider(meta.Slug)
|
||||
data["Integrations"] = im.ListForProvider(meta.Slug)
|
||||
}
|
||||
|
||||
// Geo-restriction per-app data
|
||||
@@ -1430,8 +1430,8 @@ func (s *Server) syncFileBrowserMounts(resetDBOnChange bool) {
|
||||
}
|
||||
|
||||
// Re-apply active integrations into config.yaml (before container restart)
|
||||
if s.integrationMgr != nil {
|
||||
s.integrationMgr.ReapplyConfigForTarget("filebrowser")
|
||||
if im := s.integrationMgr.Load(); im != nil {
|
||||
im.ReapplyConfigForTarget("filebrowser")
|
||||
}
|
||||
|
||||
// Generate and write compose (includes config.yaml mount)
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
package web
|
||||
|
||||
import (
|
||||
"sync"
|
||||
"testing"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-controller/internal/integrations"
|
||||
)
|
||||
|
||||
// TestIntegrationManagerNoRace is a regression test for M25. NewServer launches
|
||||
// the SyncFileBrowserMounts goroutine (which reads s.integrationMgr) from inside
|
||||
// the constructor, BEFORE main.go calls SetIntegrationManager. With a plain
|
||||
// pointer field that write/read pair is a data race; this test reproduces the
|
||||
// concurrent access and must run clean under `go test -race`.
|
||||
//
|
||||
// On the pre-fix code (plain `integrationMgr *integrations.Manager`, written via
|
||||
// `s.integrationMgr = mgr` and read via `s.integrationMgr`) the race detector
|
||||
// flags this. With the atomic.Pointer field it is clean. Run with:
|
||||
//
|
||||
// go test -race ./internal/web/ -run IntegrationManagerNoRace
|
||||
func TestIntegrationManagerNoRace(t *testing.T) {
|
||||
var s Server // zero value: atomic.Pointer field is usable as-is
|
||||
|
||||
const iters = 2000
|
||||
var wg sync.WaitGroup
|
||||
|
||||
// Writer — mirrors main.go calling SetIntegrationManager after construction.
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
for i := 0; i < iters; i++ {
|
||||
s.SetIntegrationManager(&integrations.Manager{})
|
||||
}
|
||||
}()
|
||||
|
||||
// Reader — mirrors the constructor-launched SyncFileBrowserMounts goroutine
|
||||
// reading the field concurrently with the write above.
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
for i := 0; i < iters; i++ {
|
||||
_ = s.integrationMgr.Load()
|
||||
}
|
||||
}()
|
||||
|
||||
wg.Wait()
|
||||
|
||||
// And the value round-trips.
|
||||
m := &integrations.Manager{}
|
||||
s.SetIntegrationManager(m)
|
||||
if s.integrationMgr.Load() != m {
|
||||
t.Fatal("SetIntegrationManager/Load round-trip mismatch")
|
||||
}
|
||||
}
|
||||
@@ -10,6 +10,7 @@ import (
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-controller/internal/appexport"
|
||||
@@ -56,8 +57,12 @@ type Server struct {
|
||||
// Asset syncer for Hub-managed assets (optional)
|
||||
assetsSyncer *assets.Syncer
|
||||
|
||||
// App-to-app integration manager (optional)
|
||||
integrationMgr *integrations.Manager
|
||||
// App-to-app integration manager (optional).
|
||||
// M25: atomic because the constructor launches the SyncFileBrowserMounts
|
||||
// goroutine (which reads this) BEFORE SetIntegrationManager runs in main.go —
|
||||
// so a plain field would be a data race (the init-only happens-before that
|
||||
// covers the other Set* fields does NOT hold for this one).
|
||||
integrationMgr atomic.Pointer[integrations.Manager]
|
||||
|
||||
// App export/import engine (optional)
|
||||
appExporter *appexport.Exporter
|
||||
@@ -159,7 +164,7 @@ func (s *Server) SetAssetsSyncer(as *assets.Syncer) {
|
||||
|
||||
// SetIntegrationManager sets the app-to-app integration manager.
|
||||
func (s *Server) SetIntegrationManager(mgr *integrations.Manager) {
|
||||
s.integrationMgr = mgr
|
||||
s.integrationMgr.Store(mgr)
|
||||
}
|
||||
|
||||
// SetLogBuffer sets the in-memory log ring buffer for the debug log viewer.
|
||||
|
||||
Reference in New Issue
Block a user