From c5835cd2b10e8c0ba0f7b71b9d7035721dbab36e Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Sat, 22 Aug 2026 01:34:01 +0200 Subject: [PATCH] [MM-70290] Run app migrations locked to the master DB (#38084) * [MM-70290] Run app migrations locked to the master DB App migrations write rows and then read them back within the same function. Those read-backs resolve to GetReplica(), so on a licensed server with read replicas configured a replica that has not yet caught up returns zero rows and doAppMigrations aborts startup via mlog.Fatal. Reported after a 10.11.12 -> 11.7.8 upgrade on Aurora PostgreSQL with a reader endpoint, crashing on the Managed Category Properties Setup migration. Restarting is not a reliable workaround: the done flag is written before the failing read, so the short-circuit path re-runs the same replica read and a node can crash-loop while lag persists. Wrapping doAppMigrations in LockToMaster/UnlockFromMaster covers every read in both migration loops, including SqlPropertyGroupStore.Get and SearchPropertyFields, which take no context and so cannot be fixed by per-call-site routing. This mirrors the existing bulk import fix in app/import.go, which locks to master for the same reason while the server is serving live traffic. Co-Authored-By: Claude Opus 5 * Expect LockToMaster in the setup-functions store mock doAppMigrations now locks the store to master, and it runs from NewServer, so every helper that builds a server on the mock store hit an unexpected-call panic. Registering both calls in GetMockStoreForSetupFunctions covers the app, app/email, app/platform and api4 helpers, which all share this mock. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 Co-authored-by: Mattermost Build --- server/channels/app/migrations.go | 6 +++ server/channels/app/migrations_test.go | 57 ++++++++++++++++++++++++++ server/channels/testlib/store.go | 2 + 3 files changed, 65 insertions(+) diff --git a/server/channels/app/migrations.go b/server/channels/app/migrations.go index c1a4ee78f7e..170dc0941d4 100644 --- a/server/channels/app/migrations.go +++ b/server/channels/app/migrations.go @@ -1335,6 +1335,12 @@ func (a *App) DoAppMigrations() { func (s *Server) doAppMigrations() { rctx := request.EmptyContext(s.Log()) + // Migrations read back rows they have just written. Routing those reads to a + // replica that has not caught up yet returns no rows, which the mlog.Fatal + // calls below turn into an aborted startup. + s.Store().LockToMaster() + defer s.Store().UnlockFromMaster() + type migration struct { name string handler func() error diff --git a/server/channels/app/migrations_test.go b/server/channels/app/migrations_test.go index 35e75814cfc..77ff511d9ac 100644 --- a/server/channels/app/migrations_test.go +++ b/server/channels/app/migrations_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/stretchr/testify/require" ) @@ -754,3 +755,59 @@ func stripStatusColors(t *testing.T, attrs model.StringInterface) model.StringIn out["options"] = options return out } + +// lockToMasterSpyStore records when doAppMigrations locks and unlocks the store +// relative to the store access the migrations themselves make. +type lockToMasterSpyStore struct { + store.Store + + mu sync.Mutex + events []string +} + +func (s *lockToMasterSpyStore) record(event string) { + s.mu.Lock() + defer s.mu.Unlock() + s.events = append(s.events, event) +} + +func (s *lockToMasterSpyStore) snapshot() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.events...) +} + +func (s *lockToMasterSpyStore) LockToMaster() { + s.record("lock") + s.Store.LockToMaster() +} + +func (s *lockToMasterSpyStore) UnlockFromMaster() { + s.record("unlock") + s.Store.UnlockFromMaster() +} + +func (s *lockToMasterSpyStore) System() store.SystemStore { + s.record("system") + return s.Store.System() +} + +// Guard, not a reproduction: storetest configures no replicas, so GetReplica already +// returns master and the race cannot be staged here. It pins that doAppMigrations +// brackets its work in the master lock; it does not exercise the routing of individual +// reads, as propertyService holds store handles captured before the spy wraps them. +func TestDoAppMigrationsRunsLockedToMaster(t *testing.T) { + th := Setup(t) + + spy := &lockToMasterSpyStore{Store: th.Server.Store()} + th.Server.SetStore(spy) + t.Cleanup(func() { th.Server.SetStore(spy.Store) }) + + th.Server.doAppMigrations() + + events := spy.snapshot() + require.GreaterOrEqual(t, len(events), 3, "expected migrations to access the store between lock and unlock") + require.Equal(t, "lock", events[0], "migrations must lock to master before touching the store") + require.Equal(t, "unlock", events[len(events)-1], "migrations must release the lock once finished") + require.Contains(t, events[1:len(events)-1], "system", "migration store access must happen while locked to master") +} diff --git a/server/channels/testlib/store.go b/server/channels/testlib/store.go index aa28de161c6..22524512203 100644 --- a/server/channels/testlib/store.go +++ b/server/channels/testlib/store.go @@ -231,6 +231,8 @@ func GetMockStoreForSetupFunctions() *mocks.Store { mockStore.On("PropertyField").Return(&propertyFieldStore) mockStore.On("PropertyValue").Return(&propertyValueStore) mockStore.On("ChannelGuard").Return(&channelGuardStore) + mockStore.On("LockToMaster").Return() + mockStore.On("UnlockFromMaster").Return() return &mockStore }