From 7ff22436dc4ebeba58fc427505776450a04ea7ac Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 14 Oct 2024 13:04:48 +0200 Subject: [PATCH] [MM-60685] Fix racy TestSyncLdap (#28324) --- server/channels/app/app_iface.go | 2 +- server/channels/app/ldap.go | 9 +++++---- .../channels/app/opentracing/opentracing_layer.go | 4 ++-- server/public/shared/request/context.go | 13 ++++++++++++- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index 84ec484497..921b711024 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -352,7 +352,7 @@ type AppIface interface { // SyncLdap starts an LDAP sync job. // If includeRemovedMembers is true, then members who left or were removed from a team/channel will // be re-added; otherwise, they will not be re-added. - SyncLdap(c request.CTX, includeRemovedMembers bool) + SyncLdap(rctx request.CTX, includeRemovedMembers bool) // SyncPlugins synchronizes the plugins installed locally // with the plugin bundles available in the file store. SyncPlugins() *model.AppError diff --git a/server/channels/app/ldap.go b/server/channels/app/ldap.go index 8840798398..54342204ac 100644 --- a/server/channels/app/ldap.go +++ b/server/channels/app/ldap.go @@ -17,20 +17,21 @@ import ( // SyncLdap starts an LDAP sync job. // If includeRemovedMembers is true, then members who left or were removed from a team/channel will // be re-added; otherwise, they will not be re-added. -func (a *App) SyncLdap(c request.CTX, includeRemovedMembers bool) { +func (a *App) SyncLdap(rctx request.CTX, includeRemovedMembers bool) { + rctx = rctx.Clone() a.Srv().Go(func() { if license := a.Srv().License(); license != nil && *license.Features.LDAP { if !*a.Config().LdapSettings.EnableSync { - c.Logger().Error("LdapSettings.EnableSync is set to false. Skipping LDAP sync.") + rctx.Logger().Error("LdapSettings.EnableSync is set to false. Skipping LDAP sync.") return } ldapI := a.Ldap() if ldapI == nil { - c.Logger().Error("Not executing ldap sync because ldap is not available") + rctx.Logger().Error("Not executing ldap sync because ldap is not available") return } - ldapI.StartSynchronizeJob(c, false, includeRemovedMembers) + ldapI.StartSynchronizeJob(rctx, false, includeRemovedMembers) } }) } diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index 05b678f74d..867cce9439 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -17464,7 +17464,7 @@ func (a *OpenTracingAppLayer) SwitchOAuthToEmail(c request.CTX, email string, pa return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) SyncLdap(c request.CTX, includeRemovedMembers bool) { +func (a *OpenTracingAppLayer) SyncLdap(rctx request.CTX, includeRemovedMembers bool) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.SyncLdap") @@ -17476,7 +17476,7 @@ func (a *OpenTracingAppLayer) SyncLdap(c request.CTX, includeRemovedMembers bool }() defer span.Finish() - a.app.SyncLdap(c, includeRemovedMembers) + a.app.SyncLdap(rctx, includeRemovedMembers) } func (a *OpenTracingAppLayer) SyncPlugins() *model.AppError { diff --git a/server/public/shared/request/context.go b/server/public/shared/request/context.go index 6ebd37ceb5..b7aa525582 100644 --- a/server/public/shared/request/context.go +++ b/server/public/shared/request/context.go @@ -54,9 +54,19 @@ func TestContext(t testing.TB) *Context { return EmptyContext(logger) } -// clone creates a shallow copy of Context, allowing clones to apply per-request changes. +// Clone creates a deep copy of [CTX]. +// It should only be used to pass a [CTX] to a separate goroutine that +// has a longer lifespan than the main goroutine handling the request. +// It should be used sparsely as coping [CTX] is often unnecessary. +func (c *Context) Clone() CTX { + return c.clone() +} + +// clone creates a deep copy of [Context], allowing clones to apply per-request changes. +// It unexported to prevent leaking the [Context] type from the [CTX] interface. func (c *Context) clone() *Context { cCopy := *c + cCopy.session = *c.session.DeepCopy() return &cCopy } @@ -173,4 +183,5 @@ type CTX interface { WithLogger(mlog.LoggerIFace) CTX WithContext(ctx context.Context) CTX With(func(ctx CTX) CTX) CTX + Clone() CTX }