mirror of
https://github.com/mattermost/mattermost.git
synced 2026-08-26 13:17:29 -05:00
[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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Mattermost Build <build@mattermost.com>
This commit is contained in:
co-authored by
Claude Opus 5
Mattermost Build
parent
c864f8de12
commit
c5835cd2b1
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user